Security Fix (sandboxed iframe)#339
Open
nbriz wants to merge 2 commits into
Open
Conversation
Contributor
Author
|
seems this has another side effect, with a sandboxed null-origin iframe you can't use camera/mic/geo, which means you can't do any of the camera demos. this will require a different approach to the sandboxing logic all-together to fix. I'll push that to this PR (along with other on-going security fixes) once that's ready (will also rename this PR at that point as it'll contain multiple security fixes, not just iframe sandbox stuff) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
previously, a bad actor could create a sketch to exfiltrate a user's localStorage (including LLM keys if target user had those saved) and/or do GitHub things on the target user's account (if they were logged in). We've now blocked that by editing the
main.jsfile so that theNNE.iframegetssandbox="allow-scripts allow-forms allow-popups allow-modals allow-pointer-lock", noallow-same-origin. This gives the iframe a null origin, blocking malicious shared sketches from reading localStorage or making credentialed same-site requests to the GitHub API. Unfortunatley, this simple edit had breaking side-effects. These have now also been addressed:the
tutorial-makerand theproject-fileswidget can't work with a sand-boxed iframe because they need the Service Worker to resolve requests for files/data stored in indexedDB (which it would no longer have access to), so when a student is working on a project (or an educator works on a tutorial) those widgets now remove the sandbox (in_initServiceWorker()for project-files and in_setCustomRendererfor tutorial-maker) then restore it oncloseProject()/_quitTutorial()when returning to sketch mode.the sandbox'd iframe's null origin causes browsers to treat all iframe requests as cross-origin. which means assets that previously loaded fine (like
<img src="cd.gif">, but also fonts in templates and other files served via alias routes) were being blocked. So nowserver.jsandroutes.jshave been edited withAccess-Control-Allow-Origin: *, this is now set on all static file responses. I've doubled checked that this is safe for public content and doesn't open up any new attack vectors (can not be combined with credentialed requests), it just means other folks canfetch()any of our public files (which is fine).a couple
postMessagecalls (for handling bg movement when mouse moves) inutils.jsstopped working because without an explicit targetOrigin, the browser uses the sender's own origin as the default target, but a null-origin iframe can't target 'https://netnet.studio' that way, and the parent can't target 'null' as a valid recipient. we're now passing'*'in the postMessages to bypass that check and let the messages between iframe/netnet through.the special error cases (see
errMsgrinutils.setCustomRenderer) had to be augmented because one special error that was handled by the netitor for checking for CORS issues no longer worked withe the sandboxed iframe. this means removing the_checkForCORSerrlogic from the netitor (since it won't work anymore) and migrating that logic into the customerrMsgrlisteners and accompanying logic in thecode-reviewwidget. This meant some refactoring of that widget (which also came with an extra bug fix: special errors no longer remove other errors from the code review list as it would before). I've added aNOTEcomment with more details in case we need to edit/refactor any of this for other reasons in the future.NOTE: this attack vector is still present if the student is working on a project or working on a tutorial, which isn't an issue if they're working on their own project/tutorial, but can be an issue if they fork someone else's project and/or open someone else's custom tutorial in the tutorial-maker. For this reason I've also added warnings to the convo netnet has with students before they fork a project as well as to the main page of the tutorial maker widget.