User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.4) Gecko/20060508 Firefox/1.5.0.4 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.4) Gecko/20060508 Firefox/1.5.0.4 IE has an attribute that can be specified on IFRAMEs that can lower the security zone of that iframe only. see: http://msdn.microsoft.com/workshop/author/dhtml/reference/properties/security.asp It can be used to restrict javascript and other dynamic behaviors of content in that iframes. I suggest we implement something similar, which can be a big help for webmail providers as it can provide a fallback mechanism incase they fail to filter out all dynamic (malicious) content from emails. The current approaches to filtering out such content are: 1. filtering out all unsafe tags and styles (difficult) 2. loading the (semi-filtered) content in an iframe from another domain (to trigger the natural cross-domain security protection) With the iframe security attribute, we can have the browser restrict the content which is much more efficient and error-proof. In order to remain standards compliant, perhaps such a feature should be triggered with javascript instead of with HTML markup. Reproducible: Always

Sounds like a good idea. Hixie, is there anything similar in W3C or WhatWG specs?

Triggering it with javascript would make it more standards compliant? :-O FWIW, there is no such feature anywhere (yet).

WHATWG has had several proposals for sandboxing ideas like this. I haven't looked at any in detail yet; Gerv might know more.

Preventing access to parent documents (parent, top, etc) would probably be sufficient for the large majority of use cases (throw a permission denied error to JavaScript trying to access this). This isn't just useful for webmail providers, it's useful in a number of other cases, for example Yahoo! Images use it, and I'm currently writing an internal company tool that would benefit greatly from this. I'm sure there's tons of other use cases for embedding bits of other websites in a page. I would dearly love to have this for Firefox 3.

See http://www.gerv.net/security/content-restrictions/, which allows you to make this sort of restriction. Gerv

We'd probably only implement this if it got standardized in HTML5. Would it make more sense to simply copy IE for broader compatibility, or come up with a more useful (more flexible? more powerful?) set of restrictions but not compatible?

HTML5 now has a "sandbox" attribute, different but the same kind of idea. Since HTML5 is on a standards track that's probably the more useful version of this idea, although it is by no means finalized.

It looks like the HTML5 IFrame's sandbox attribute has been implemented in Webkit (WebKit@51577) and is available in the latest Chrome 4 beta. One question: Should the title of this bug be updated to "[HTML5] Implement IFRAME's sandbox attribute" or should a new bug be filled?

> It looks like the HTML5 IFrame's sandbox attribute has been implemented in > Webkit (WebKit@51577) and is available in the latest Chrome 4 beta. The @sandbox attribute is not available in the beta channel, but it is available in the dev channel, which will become Chrome 5.

Because of its implementation in Chrome/Safari over the last several months, the HTML5 sandbox attribute is already getting a lot of attention from the community. Try Googling "html5 sandbox" to see how many articles are already dedicated to the topic. Is Firefox en route to implementing HTML5's sandbox attribute any time soon?

This has now shown up in IE10's Preview #2.

While the feature is documented here: https://developer.mozilla.org/en/HTML/Element/iframe it does not seem to work, or at least "allow-top-navigation" does not work, since its absence still allows an iframe to modify the top window location. Is there any hope at all ?

(In reply to Xavier from comment #15) > While the feature is documented here: > https://developer.mozilla.org/en/HTML/Element/iframe > it does not seem to work, or at least "allow-top-navigation" does not work, > since its absence still allows an iframe to modify the top window location. > > Is there any hope at all ? i am (very slowly due to other tasks) working on this. there is no sandbox attribute support in Firefox currently. i hope to at least get some prototype/rough/experimental patches up to show possible implementation routes.

a Work In Progress patch - this is an experimental prototype implementing some very basic parts of the iframe sandbox. i'm continuing to iterate on this patch, i'm putting it up here in case others are interested and to possibly collect more feedback on the direction i'm going. there are many parts of the spec still yet to be implemented.

now with less silly bugs ! the next steps are to figure out more of the attribute handling using nsDOMSettableTokenList and add parsing for some of the values, probably starting with allow-scripts and allow-same-domain

this patch adds a possible implementation for allow-scripts and allow-same-origin. i'm looking at allow-top-navigation at the moment and then planning on moving on to allow-forms. there are also many TODO's and questions in this patch still :)

a possible implementation for 'allow-forms' still working on 'allow-top-navigation' - and there are still many details to iron out after that

Comment on attachment 575617 [details] [diff] [review] WIP - experimental prototype this patch passes the browserscope.org iframe sandbox check fwiw :)

Microsoft has a public test suite for HTML5 iframe sandbox at http://samples.msdn.microsoft.com/ietestcenter/#html5Sandbox the current prototype patch passes ~50% of the tests there (some of the failures are due to no allow-top-navigation yet, no cookie/localStorage restrictions, no video autoplay restriction yet, etc as a high level summary of what's still to be done) as part of the work on implementing iframe sandbox, we will need mochitests of our own in the tree to test the functionality as outlined in the HTML5 spec. i will probably end up working on making this happen also.

adds a possible implementation of blocking window.open, <a target = "_blank"> and showModalDialog()

a possible implementation of allow-top-navigation that attempts to also implement the navigation rules this patch passes 20 of the 30 tests MS test suite now. i need to look into why the XHR tests pass - i assume it's because we block XHR for a null principal, which a sandboxed iframe has and we let the iframe have its natural principal when allow-same-domain is specified. the MS video autoplay tests don't work in Firefox since they use MP4/H264 format video which we don't support. the MS autofocus test passes in current Firefox, so i need to look into that more as well. i also would like to have some more testing around frame navigation that the tests in the MS suite (which this current patch passes). i think i'm going start working on the test suite at this point due to the above. it would be awesome if some people could take a look at the patch and tell me if the code is sane at all. there's a fair number of TODO's in it and a lot of questions around what to do if things fail.

Comment on attachment 578799 [details] [diff] [review] WIP - experimental prototype Review of attachment 578799 [details] [diff] [review]: ----------------------------------------------------------------- Drive by comments. These should definitely not be taken as exhaustive. Looks like this doesn't attempt to cover plugins? Also looks like this doesn't attempt to deal with scripts? I expect those are going to be a significant amount of work. We should detect the situation called out in the first warning (and maybe the second too?) at http://www.whatwg.org/specs/web-apps/current-work/multipage/the-iframe-element.html#attr-iframe-sandbox and send a warning to the error console. Reading the spec and the proto-patch here, I think storing the sandbox flags on both the docshell and the document is going to be a source of great pain. I think you're going to want one canonical set of sandbox flags (on the outer window, perhaps?). Also, you really should request f? from a specific person. I gave an f-, but at this stage that is to be expected. Finally, this is going to need lots of tests ... ::: content/base/public/nsContentUtils.h @@ +1838,5 @@ > static bool SetUpChannelOwner(nsIPrincipal* aLoadingPrincipal, > nsIChannel* aChannel, > nsIURI* aURI, > + bool aSetUpForAboutBlank, > + bool aForceOwner); You could make aForceOwner default to false here. @@ +1864,5 @@ > + * > + * @param aAttribute the value of the sandbox attribute as an nsIDOMDOMSettableTokenList > + * @return the set of flags > + */ > + static PRUint32 ParseSandboxAttributeToFlags(nsIDOMDOMSettableTokenList * aAttribute); 80 character lines please. ::: content/base/public/nsIDocument.h @@ +462,5 @@ > } > > /** > + * Set the sandbox flags for this document. This should only happen at > + * at document load, the flags are immutable after being set at load time. Can you assert that this is being called during load? (by checking for READYSTATE_LOADING or something?) @@ +471,5 @@ > + mSandboxFlags = aSandboxFlags; > + } > + > + /** > + * Get the sandox flags for this document sandbox @@ +1786,5 @@ > > + // The sandbox flags on the document. These reflect the value of the sandbox attribute of the > + // associated IFRAME or CSP-protectable content, if existent. These are set at load time and > + //are immutable - see nsDocShell.idl for the possible flags > + PRUint32 mSandboxFlags; nsIDocument needs an IID bump for this. ::: content/base/src/nsContentUtils.cpp @@ +699,5 @@ > + * @param aAttribute the value of the sandbox attribute as an nsIDOMDOMSettableTokenList > + * @return the set of flags > + */ > +PRUint32 > +nsContentUtils::ParseSandboxAttributeToFlags(nsIDOMDOMSettableTokenList * aAttribute) You should rewrite this function to use nsCharSeparatedTokenizer<nsContentUtils::IsHTMLWhitespace>. As it is it'll match things like "allow-scriptsallow-same-origin" which are not allowed per spec. I'd give extra points for putting a typedef nsCharSeparatedTokenizer<nsContentUtils::IsHTMLWhitespace> HTMLSplitOnSpacesTokenizer (or something) in nsContentUtils.h and changing the existing users in the tree. @@ +702,5 @@ > +PRUint32 > +nsContentUtils::ParseSandboxAttributeToFlags(nsIDOMDOMSettableTokenList * aAttribute) > +{ > + // TODO: how to handle errors in this function ? if we're here, something has a sandbox > + // attribute set and expects content to be sandboxed... perhaps consider failing closed ?? You can do this one of three ways: 1) Return an explicit nsresult and pass the flags back in a PRUint32 outparam. 2) Return a clearly bogus value for failure (e.g. -1) 3) Fail closed (-1 effectively does this if somebody forgets the check). What in here is fallible though? Simple tokenization and comparison should be infallible, I think. @@ +711,5 @@ > + out |= nsIDocShell::SANDBOX_FLAGS_SANDBOXED; > + > + bool allowed = false; > + > + // TODO: should i try to use atoms even though Contains wants a string ? Yes, you should use the atoms and do 'if (atom->Equals(token)) ...' ::: content/base/src/nsDocument.cpp @@ +2394,5 @@ > + nsCOMPtr<nsIDocShell> docShell = do_QueryInterface(aContainer); > + > + if (docShell) { > + nsresult rv = docShell->GetSandboxFlags(&mSandboxFlags); > + NS_ENSURE_SUCCESS(rv, rv); ugh tabs ::: content/base/src/nsFrameLoader.cpp @@ +506,5 @@ > + // and it may not be - maybe fail closed (all restrictions) ??? > + } > + > + if (parentSandboxFlags) { > + // do the intersection with the current flags in the docshell! I didn't look at this too closely, I assume that permissions can only be removed, not added, for children? e.g. if I have: - Top Level Doc (A) - Sandboxed Iframe (B) - Sandboxed Iframe (C) And B has permissions 1 and 2 there's no way for C to have permission 3, right? ::: content/html/content/src/nsHTMLFormElement.cpp @@ +691,5 @@ > + PRUint32 sandboxFlags = doc->GetSandboxFlags(); > + > + if (!(sandboxFlags & nsIDocShell::SANDBOX_FLAGS_SANDBOXED) || > + (sandboxFlags & nsIDocShell::SANDBOX_FLAGS_ALLOW_FORM_SUBMIT)) > + rv = DoSubmit(aEvent); Does this check avoid doing form validation and the like when sandboxed? (I think it does). You'll want tests for that. ::: content/html/content/src/nsHTMLIFrameElement.cpp @@ +306,5 @@ > + bool aNotify) > +{ > + if (aName == nsGkAtoms::sandbox && aNameSpaceID == kNameSpaceID_None) { > + // parse the new value of the sandbox attribute, and if we have a docshell > + // set its sandbox flags appropriately When is a sandbox attribute set supposed to take effect? "While the sandbox attribute is specified, the iframe element's nested browsing context must have the flags given in the following list set. In addition, any browsing contexts nested within an iframe, either directly or indirectly, must have all the flags set on them as were set on the iframe's Document's browsing context when the iframe's Document was created." So it sounds like a sandbox attr change should only take effect for the next document in the iframe. Setting them on the docshell immediately means they'll take affect for future subloads even on the same document, right? You'll want either tests to show that this isn't the case or to change the code so that it isn't (and then tests :-)). ::: docshell/base/nsDocShell.cpp @@ +755,5 @@ > mIsBeingDestroyed(false), > mIsExecutingOnLoadHandler(false), > mIsPrintingOrPP(false), > + mSavingOldViewer(false), > + mSandboxFlags(0) Please order intializers in the same order variables are declared (doing otherwise will give warnings on gcc). @@ +2887,5 @@ > + // If our document is sandboxed, we need to do some extra checks > + PRUint32 sandboxFlags = 0; > + > + if (mContentViewer) { > + nsCOMPtr<nsIDocument> doc = mContentViewer->GetDocument(); No need to use an nsCOMPtr here. nsIDocument* doc is fine. @@ +2944,5 @@ > + parentAsItem = tmp; > + } > + } > + } > + Somebody who understand docshell (read bz, smaug) should look at this stuff. In my five minutes of looking at docshell it looks like this logic might fit better in CanAccessItem. @@ +4996,5 @@ > + > +NS_IMETHODIMP > +nsDocShell::GetSandboxFlags(PRUint32 *aSandboxFlags) > +{ > + NS_ENSURE_ARG_POINTER(aSandboxFlags); Usually we don't bother with this. If you pass in null, you crash. @@ +8201,5 @@ > + PRUint32 sandboxFlags = 0; > + sandboxFlags = doc->GetSandboxFlags(); > + if (sandboxFlags & nsIDocShell::SANDBOX_FLAGS_SANDBOXED) { > + return NS_ERROR_FAILURE; > + } Same comment as above about somebody who knows docshell looking at this. @@ +8996,5 @@ > + nsContentUtils::SetUpChannelOwner(ownerPrincipal, channel, aURI, true, true); > + } > + else { > + nsContentUtils::SetUpChannelOwner(ownerPrincipal, channel, aURI, true, false); > + } I think this would be easier to follow with one callsite whose final parameter varies. ::: docshell/base/nsIDocShell.idl @@ +561,5 @@ > + * They are only used when loading new content, sandbox flags are also immutably set on the document > + * when it is loaded. The sandbox flags of a document depend on the sandbox flags on its docshell > + * and of its parent document, if any. > + */ > + attribute unsigned long sandboxFlags; nsIDocShell needs an IID bump for this. @@ +576,5 @@ > + > + /** > + * The content is sandboxed but allowed to have its natural domain. > + */ > + const unsigned long SANDBOX_FLAGS_ALLOW_SAME_DOMAIN = 0x4; please make this ALLOW_SAME_ORIGIN so that the terminology is consistent. ::: dom/interfaces/html/nsIDOMHTMLIFrameElement.idl @@ +68,5 @@ > readonly attribute nsIDOMDocument contentDocument; > readonly attribute nsIDOMWindow contentWindow; > + > + // HTML5 sandbox attribute > + readonly attribute nsIDOMDOMSettableTokenList sandbox; Needs an IID bump.

> 3) Fail closed (-1 effectively does this if somebody forgets the check). Actually, -1 does not do this (it does the opposite!). I had the polarity of the flags wrong. That said, I think after you rewrite this function nothing it in will be fallible and you won't need to care.

Comment on attachment 578799 [details] [diff] [review] WIP - experimental prototype >--- a/content/base/public/nsIDocument.h >+++ b/content/base/public/nsIDocument.h > /** >+ * Set the sandbox flags for this document. This should only happen at >+ * at document load, the flags are immutable after being set at load time. >+ * @ see nsDocShell.idl for the possible flags nsDocShell.idl doesn't exist. (Three times or so.) >--- a/content/base/src/nsFrameLoader.cpp >+++ b/content/base/src/nsFrameLoader.cpp >+ nsCOMPtr<nsIDocShellTreeItem> curDocShellItem(do_QueryInterface(mDocShell)); 'nsCOMPtr<nsIDocShellTreeItem> curDocShellItem = do_QueryInterface(mDocShell);' >+ // is this an <iframe> with a sandbox attribute or a frame whose parent >+ // is sandboxed ? >+ if (mOwnerContent->IsHTML(nsGkAtoms::iframe) && >+ (parentSandboxFlags || mOwnerContent->GetAttr(kNameSpaceID_None, nsGkAtoms::sandbox, sandboxAttr))) { Instead of this, add a nsHTMLIFrameElement* nsHTMLIFrameElement::FromContent(nsIContent*) and expose a method on nsHTMLIFrameElement that returns the flags, and move the code you added to nsContentUtils there. Also use it in nsHTMLIFrameElement::AfterSetAttr. >+ PRUint32 sandboxFlags = 0; >+ >+ nsCOMPtr<nsIDOMHTMLIFrameElement> iframe = do_QueryInterface(mOwnerContent); >+ >+ if (iframe) { >+ nsCOMPtr<nsIDOMDOMSettableTokenList> sandbox; >+ >+ nsresult result = iframe->GetSandbox(getter_AddRefs(sandbox)); >+ // TODO : what to do if this fails ? (see below also) >+ >+ sandboxFlags = nsContentUtils::ParseSandboxAttributeToFlags(sandbox); >+ // TODO: what to do if this fails ? the content was intended to be sandboxed >+ // and it may not be - maybe fail closed (all restrictions) ??? >+ } >+ >+ if (parentSandboxFlags) { >+ // do the intersection with the current flags in the docshell! Doesn't sandboxFlags &= parentSandboxFlags do the right thing, and if not, why not? >+ } >+ >+ else { } else { >--- a/content/html/content/src/nsHTMLIFrameElement.cpp >+++ b/content/html/content/src/nsHTMLIFrameElement.cpp >+ virtual nsresult AfterSetAttr(PRInt32 aNameSpaceID, nsIAtom* aName, >+ const nsAString* aValue, >+ bool aNotify); Indentation >--- a/docshell/base/nsDocShell.cpp >+++ b/docshell/base/nsDocShell.cpp >+ // If the content is sandboxed, force the passed in owner (which should be >+ // a null principal) to be set on the channel, UNLESS the sandbox allow-same-domain flag is also >+ // set. In that case, allow the content to asssume its "natural domain" as it >+ // normally would. >+ if ((mSandboxFlags & SANDBOX_FLAGS_SANDBOXED) && >+ (mSandboxFlags & SANDBOX_FLAGS_ALLOW_SAME_DOMAIN) != SANDBOX_FLAGS_ALLOW_SAME_DOMAIN) { >+ nsContentUtils::SetUpChannelOwner(ownerPrincipal, channel, aURI, true, true); >+ } >+ else { >+ nsContentUtils::SetUpChannelOwner(ownerPrincipal, channel, aURI, true, false); >+ } nsContentUtils::SetUpChannelOwner(ownerPrincipal, channel, aURI, true, (mSandboxFlags & SANDBOX_FLAGS_SANDBOXED) && (mSandboxFlags & SANDBOX_FLAGS_ALLOW_SAME_DOMAIN) != SANDBOX_FLAGS_ALLOW_SAME_DOMAIN); >--- a/dom/interfaces/html/nsIDOMHTMLIFrameElement.idl >+++ b/dom/interfaces/html/nsIDOMHTMLIFrameElement.idl >- >+ >+ // HTML5 sandbox attribute This interface is specified in HTML, so no need for the comment. Also, leave the two empty lines between the HTML-specified members and the Mozilla extensions. Also check the entire patch for tabs and trailing whitespace, and make sure the comments you add are full sentences (including capital letters and full stops).

Thanks very much for the feedback khuey and Ms2ger ! Adding a note here that on try this crashed in a mochitest : TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/xpinstall/browser_navigateaway2.js | Exited with code 1 during test run PROCESS-CRASH | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/xpinstall/browser_navigateaway2.js | application crashed (minidump found) Thread 0 (crashed)

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #25) > Comment on attachment 578799 [details] [diff] [review] [diff] [details] [review] > WIP - experimental prototype > > Review of attachment 578799 [details] [diff] [review] [diff] [details] [review]: > ----------------------------------------------------------------- > > Drive by comments. These should definitely not be taken as exhaustive. > > Looks like this doesn't attempt to cover plugins? Also looks like this > doesn't attempt to deal with scripts? I expect those are going to be a > significant amount of work. Plugins and scripts are both handled by this patch. The frame loader code does : + if (!(sandboxFlags & nsIDocShell::SANDBOX_FLAGS_ALLOW_SCRIPTS)) { + mDocShell->SetAllowJavascript(false); + } + + // _always_ disallow plugins when sandboxed + mDocShell->SetAllowPlugins(false); > We should detect the situation called out in the first warning (and maybe > the second too?) at > http://www.whatwg.org/specs/web-apps/current-work/multipage/the-iframe- > element.html#attr-iframe-sandbox and send a warning to the error console. This sounds really good to me. > Reading the spec and the proto-patch here, I think storing the sandbox flags > on both the docshell and the document is going to be a source of great pain. > I think you're going to want one canonical set of sandbox flags (on the > outer window, perhaps?). Yeah, i discussed the docshell/document split with bz initially - i need to go back and reread those emails. We also use the flags on the document when doing checks. We may not need to store them on the docshell at all ? It seems like it would be better to just read the current value of the attribute when the load happens and use that as the fixed set of flags. > Also, you really should request f? from a specific person. I gave an f-, > but at this stage that is to be expected. Right. I was going to ping bz, sicking and smaug (those are the folks i have been emailing with discussing this work) to see who were good folks to ask specifically for feedback - i _really_ appreciate you giving drive by feedback before i got to that though ! :) > Finally, this is going to need lots of tests ... I wholeheartedly agree - a test suite is needed for this to land, bz also suggested submitting our tests back to the W3C as well. > ::: content/base/public/nsContentUtils.h > @@ +1838,5 @@ > > static bool SetUpChannelOwner(nsIPrincipal* aLoadingPrincipal, > > nsIChannel* aChannel, > > nsIURI* aURI, > > + bool aSetUpForAboutBlank, > > + bool aForceOwner); > > You could make aForceOwner default to false here. will do. > @@ +1864,5 @@ > > + * > > + * @param aAttribute the value of the sandbox attribute as an nsIDOMDOMSettableTokenList > > + * @return the set of flags > > + */ > > + static PRUint32 ParseSandboxAttributeToFlags(nsIDOMDOMSettableTokenList * aAttribute); > > 80 character lines please. will fix. > ::: content/base/public/nsIDocument.h > @@ +462,5 @@ > > } > > > > /** > > + * Set the sandbox flags for this document. This should only happen at > > + * at document load, the flags are immutable after being set at load time. > > Can you assert that this is being called during load? (by checking for > READYSTATE_LOADING or something?) good idea - i'll look into this. > @@ +471,5 @@ > > + mSandboxFlags = aSandboxFlags; > > + } > > + > > + /** > > + * Get the sandox flags for this document > > sandbox will fix. > @@ +1786,5 @@ > > > > + // The sandbox flags on the document. These reflect the value of the sandbox attribute of the > > + // associated IFRAME or CSP-protectable content, if existent. These are set at load time and > > + //are immutable - see nsDocShell.idl for the possible flags > > + PRUint32 mSandboxFlags; > > nsIDocument needs an IID bump for this. thanks, will fix. > ::: content/base/src/nsContentUtils.cpp > @@ +699,5 @@ > > + * @param aAttribute the value of the sandbox attribute as an nsIDOMDOMSettableTokenList > > + * @return the set of flags > > + */ > > +PRUint32 > > +nsContentUtils::ParseSandboxAttributeToFlags(nsIDOMDOMSettableTokenList * aAttribute) > > You should rewrite this function to use > nsCharSeparatedTokenizer<nsContentUtils::IsHTMLWhitespace>. As it is it'll > match things like "allow-scriptsallow-same-origin" which are not allowed per > spec. > > I'd give extra points for putting a > > typedef nsCharSeparatedTokenizer<nsContentUtils::IsHTMLWhitespace> > HTMLSplitOnSpacesTokenizer (or something) > > in nsContentUtils.h and changing the existing users in the tree. yeah, this patch fails the MS test on whitespace etc. so i knew there were some issues here - thanks for the suggestion. i'm not opposed to doing the HTMLSplitOnSpacesTokenizer approach here, i could split off the work to change existing users and add the typedef as a separate blocking bug and knock that out first. > @@ +702,5 @@ > > +PRUint32 > > +nsContentUtils::ParseSandboxAttributeToFlags(nsIDOMDOMSettableTokenList * aAttribute) > > +{ > > + // TODO: how to handle errors in this function ? if we're here, something has a sandbox > > + // attribute set and expects content to be sandboxed... perhaps consider failing closed ?? > > You can do this one of three ways: > > 1) Return an explicit nsresult and pass the flags back in a PRUint32 > outparam. > 2) Return a clearly bogus value for failure (e.g. -1) > 3) Fail closed (-1 effectively does this if somebody forgets the check). > > What in here is fallible though? Simple tokenization and comparison should > be infallible, I think. yeah - i will look at that code to verify if it can fail when i make the change above. > @@ +711,5 @@ > > + out |= nsIDocShell::SANDBOX_FLAGS_SANDBOXED; > > + > > + bool allowed = false; > > + > > + // TODO: should i try to use atoms even though Contains wants a string ? > > Yes, you should use the atoms and do 'if (atom->Equals(token)) ...' will do. > ::: content/base/src/nsDocument.cpp > @@ +2394,5 @@ > > + nsCOMPtr<nsIDocShell> docShell = do_QueryInterface(aContainer); > > + > > + if (docShell) { > > + nsresult rv = docShell->GetSandboxFlags(&mSandboxFlags); > > + NS_ENSURE_SUCCESS(rv, rv); > > ugh tabs sorry - i was mainly wanting to know if the approach was sane, apologies for the formatting/tabs/whitespace stuff, i knew that needed cleaning up. > ::: content/base/src/nsFrameLoader.cpp > @@ +506,5 @@ > > + // and it may not be - maybe fail closed (all restrictions) ??? > > + } > > + > > + if (parentSandboxFlags) { > > + // do the intersection with the current flags in the docshell! > > I didn't look at this too closely, I assume that permissions can only be > removed, not added, for children? > > e.g. if I have: > > - Top Level Doc (A) > - Sandboxed Iframe (B) > - Sandboxed Iframe (C) > > And B has permissions 1 and 2 there's no way for C to have permission 3, > right? yes - permissions can only be tightened/removed, never added/loosened. ms2ger commented on this below, i know there's a bug in the existing patch with inheritance and i think ms2ger's suggestion is the right one after thinking it over for a bit. > ::: content/html/content/src/nsHTMLFormElement.cpp > @@ +691,5 @@ > > + PRUint32 sandboxFlags = doc->GetSandboxFlags(); > > + > > + if (!(sandboxFlags & nsIDocShell::SANDBOX_FLAGS_SANDBOXED) || > > + (sandboxFlags & nsIDocShell::SANDBOX_FLAGS_ALLOW_FORM_SUBMIT)) > > + rv = DoSubmit(aEvent); > > Does this check avoid doing form validation and the like when sandboxed? (I > think it does). You'll want tests for that. i think it does too - but yes, this should be a test in the suite. Thanks. > ::: content/html/content/src/nsHTMLIFrameElement.cpp > @@ +306,5 @@ > > + bool aNotify) > > +{ > > + if (aName == nsGkAtoms::sandbox && aNameSpaceID == kNameSpaceID_None) { > > + // parse the new value of the sandbox attribute, and if we have a docshell > > + // set its sandbox flags appropriately > > When is a sandbox attribute set supposed to take effect? > > "While the sandbox attribute is specified, the iframe element's nested > browsing context must have the flags given in the following list set. In > addition, any browsing contexts nested within an iframe, either directly or > indirectly, must have all the flags set on them as were set on the iframe's > Document's browsing context when the iframe's Document was created." > > So it sounds like a sandbox attr change should only take effect for the next > document in the iframe. Setting them on the docshell immediately means > they'll take affect for future subloads even on the same document, right? > You'll want either tests to show that this isn't the case or to change the > code so that it isn't (and then tests :-)). yeah, that's my reading of the spec - an attr change only takes effect when the next document is loaded in the iframe. Future subloads are an important case to point out here, thanks - i believe that they should be based on the flags on the document, NOT the docshell. And yes, this needs tests around it for sure ! > ::: docshell/base/nsDocShell.cpp > @@ +755,5 @@ > > mIsBeingDestroyed(false), > > mIsExecutingOnLoadHandler(false), > > mIsPrintingOrPP(false), > > + mSavingOldViewer(false), > > + mSandboxFlags(0) > > Please order intializers in the same order variables are declared (doing > otherwise will give warnings on gcc). will do. > @@ +2887,5 @@ > > + // If our document is sandboxed, we need to do some extra checks > > + PRUint32 sandboxFlags = 0; > > + > > + if (mContentViewer) { > > + nsCOMPtr<nsIDocument> doc = mContentViewer->GetDocument(); > > No need to use an nsCOMPtr here. nsIDocument* doc is fine. will change. > @@ +2944,5 @@ > > + parentAsItem = tmp; > > + } > > + } > > + } > > + > > Somebody who understand docshell (read bz, smaug) should look at this stuff. > In my five minutes of looking at docshell it looks like this logic might fit > better in CanAccessItem. i discussed this over email with bz and asked about putting this in CanAccessItem and in the end, it was decided that it shouldn't. > @@ +4996,5 @@ > > + > > +NS_IMETHODIMP > > +nsDocShell::GetSandboxFlags(PRUint32 *aSandboxFlags) > > +{ > > + NS_ENSURE_ARG_POINTER(aSandboxFlags); > > Usually we don't bother with this. If you pass in null, you crash. heh, ok, i'll drop it for consistency. > @@ +8201,5 @@ > > + PRUint32 sandboxFlags = 0; > > + sandboxFlags = doc->GetSandboxFlags(); > > + if (sandboxFlags & nsIDocShell::SANDBOX_FLAGS_SANDBOXED) { > > + return NS_ERROR_FAILURE; > > + } > > Same comment as above about somebody who knows docshell looking at this. thanks - i'll ping one of the folks you mentioned on this part. > @@ +8996,5 @@ > > + nsContentUtils::SetUpChannelOwner(ownerPrincipal, channel, aURI, true, true); > > + } > > + else { > > + nsContentUtils::SetUpChannelOwner(ownerPrincipal, channel, aURI, true, false); > > + } > > I think this would be easier to follow with one callsite whose final > parameter varies. yeah, ms2ger has a good suggestion below here i'm going to take. > ::: docshell/base/nsIDocShell.idl > @@ +561,5 @@ > > + * They are only used when loading new content, sandbox flags are also immutably set on the document > > + * when it is loaded. The sandbox flags of a document depend on the sandbox flags on its docshell > > + * and of its parent document, if any. > > + */ > > + attribute unsigned long sandboxFlags; > > nsIDocShell needs an IID bump for this. thanks, will do. > @@ +576,5 @@ > > + > > + /** > > + * The content is sandboxed but allowed to have its natural domain. > > + */ > > + const unsigned long SANDBOX_FLAGS_ALLOW_SAME_DOMAIN = 0x4; > > please make this ALLOW_SAME_ORIGIN so that the terminology is consistent. will do. > ::: dom/interfaces/html/nsIDOMHTMLIFrameElement.idl > @@ +68,5 @@ > > readonly attribute nsIDOMDocument contentDocument; > > readonly attribute nsIDOMWindow contentWindow; > > + > > + // HTML5 sandbox attribute > > + readonly attribute nsIDOMDOMSettableTokenList sandbox; > > Needs an IID bump. thanks, will do.

(In reply to Ms2ger from comment #27) > Comment on attachment 578799 [details] [diff] [review] [diff] [details] [review] > WIP - experimental prototype Again, thanks very much for jumping on this patch and giving me feedback, it's greatly appreciated :) > >--- a/content/base/public/nsIDocument.h > >+++ b/content/base/public/nsIDocument.h > > /** > >+ * Set the sandbox flags for this document. This should only happen at > >+ * at document load, the flags are immutable after being set at load time. > >+ * @ see nsDocShell.idl for the possible flags > > nsDocShell.idl doesn't exist. (Three times or so.) oops, sorry, will fix. > >--- a/content/base/src/nsFrameLoader.cpp > >+++ b/content/base/src/nsFrameLoader.cpp > >+ nsCOMPtr<nsIDocShellTreeItem> curDocShellItem(do_QueryInterface(mDocShell)); > > 'nsCOMPtr<nsIDocShellTreeItem> curDocShellItem = > do_QueryInterface(mDocShell);' thanks, will fix. > >+ // is this an <iframe> with a sandbox attribute or a frame whose parent > >+ // is sandboxed ? > >+ if (mOwnerContent->IsHTML(nsGkAtoms::iframe) && > >+ (parentSandboxFlags || mOwnerContent->GetAttr(kNameSpaceID_None, nsGkAtoms::sandbox, sandboxAttr))) { > > Instead of this, add a nsHTMLIFrameElement* > nsHTMLIFrameElement::FromContent(nsIContent*) and expose a method on > nsHTMLIFrameElement that returns the flags, and move the code you added to > nsContentUtils there. Also use it in nsHTMLIFrameElement::AfterSetAttr. ok, i took a look at an example FromContent() on another element and I think I understand what you'd like me to do here. I put the code to parse the flags etc in nsContentUtils so it could be used by CSP Sandbox (bug 671389) - does that change your opinion at all on how to rework this ? (that's why i didn't put the flag parsing in the iframe element originally). We also discussed support @sandbox on <frame>, which it seemed like there was interest in (just as an fyi). > >+ PRUint32 sandboxFlags = 0; > >+ > >+ nsCOMPtr<nsIDOMHTMLIFrameElement> iframe = do_QueryInterface(mOwnerContent); > >+ > >+ if (iframe) { > >+ nsCOMPtr<nsIDOMDOMSettableTokenList> sandbox; > >+ > >+ nsresult result = iframe->GetSandbox(getter_AddRefs(sandbox)); > >+ // TODO : what to do if this fails ? (see below also) > >+ > >+ sandboxFlags = nsContentUtils::ParseSandboxAttributeToFlags(sandbox); > >+ // TODO: what to do if this fails ? the content was intended to be sandboxed > >+ // and it may not be - maybe fail closed (all restrictions) ??? > >+ } > >+ > >+ if (parentSandboxFlags) { > >+ // do the intersection with the current flags in the docshell! > > Doesn't sandboxFlags &= parentSandboxFlags do the right thing, and if not, > why not? yeah. as i mentioned above, i think &= is the right thing here. i will fix this. > >+ } > >+ > >+ else { > > } else { > > >--- a/content/html/content/src/nsHTMLIFrameElement.cpp > >+++ b/content/html/content/src/nsHTMLIFrameElement.cpp > >+ virtual nsresult AfterSetAttr(PRInt32 aNameSpaceID, nsIAtom* aName, > >+ const nsAString* aValue, > >+ bool aNotify); > > Indentation will fix, sorry. > >--- a/docshell/base/nsDocShell.cpp > >+++ b/docshell/base/nsDocShell.cpp > >+ // If the content is sandboxed, force the passed in owner (which should be > >+ // a null principal) to be set on the channel, UNLESS the sandbox allow-same-domain flag is also > >+ // set. In that case, allow the content to asssume its "natural domain" as it > >+ // normally would. > >+ if ((mSandboxFlags & SANDBOX_FLAGS_SANDBOXED) && > >+ (mSandboxFlags & SANDBOX_FLAGS_ALLOW_SAME_DOMAIN) != SANDBOX_FLAGS_ALLOW_SAME_DOMAIN) { > >+ nsContentUtils::SetUpChannelOwner(ownerPrincipal, channel, aURI, true, true); > >+ } > >+ else { > >+ nsContentUtils::SetUpChannelOwner(ownerPrincipal, channel, aURI, true, false); > >+ } > > nsContentUtils::SetUpChannelOwner(ownerPrincipal, channel, aURI, true, > (mSandboxFlags & SANDBOX_FLAGS_SANDBOXED) > && > (mSandboxFlags & > SANDBOX_FLAGS_ALLOW_SAME_DOMAIN) != SANDBOX_FLAGS_ALLOW_SAME_DOMAIN); thanks, much cleaner, will fix. > >--- a/dom/interfaces/html/nsIDOMHTMLIFrameElement.idl > >+++ b/dom/interfaces/html/nsIDOMHTMLIFrameElement.idl > >- > >+ > >+ // HTML5 sandbox attribute > > This interface is specified in HTML, so no need for the comment. Also, leave > the two empty lines between the HTML-specified members and the Mozilla > extensions. Thanks, will fix. > Also check the entire patch for tabs and trailing whitespace, and make sure > the comments you add are full sentences (including capital letters and full > stops). Will do - sorry, this patch wasn't totally cleaned up, thanks for the sanity check on the code, i will clean it all up ! :)

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #26) > > 3) Fail closed (-1 effectively does this if somebody forgets the check). > > Actually, -1 does not do this (it does the opposite!). I had the polarity > of the flags wrong. That said, I think after you rewrite this function > nothing it in will be fallible and you won't need to care. re: the polarity of the flags, in addition to all the other stuff i need to address from the feedback from you and ms2ger, i think i want to rework the patch so the flags have the exact same semantics as the HTML5 spec. i think i just resisted their blacklisting approach to sandbox permissions. i think it's most important to follow the spec here though.

(In reply to Ian Melven :imelven from comment #29) > (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #25) > > Comment on attachment 578799 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review] > > WIP - experimental prototype > > > > Review of attachment 578799 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review]: > > ----------------------------------------------------------------- > > > > Drive by comments. These should definitely not be taken as exhaustive. > > > > Looks like this doesn't attempt to cover plugins? Also looks like this > > doesn't attempt to deal with scripts? I expect those are going to be a > > significant amount of work. > > Plugins and scripts are both handled by this patch. The frame loader code > does : > + if (!(sandboxFlags & nsIDocShell::SANDBOX_FLAGS_ALLOW_SCRIPTS)) { > + mDocShell->SetAllowJavascript(false); > + } > + > + // _always_ disallow plugins when sandboxed > + mDocShell->SetAllowPlugins(false); Ok. For plugins you'll want tests for both plugins (embed, object, etc) and plugin documents (a subframe whose URL is a type loaded by a plugin). For JavaScript, does disabling JS on the docshell affect XBL bindings? That'll break video controls, among other things. It looks like SetAllowPlugins(false) and SetAllowJavascript(false) will block plugin and javascript *loads* in addition to execution/instantiation, which I don't believe is the desired behavior here (this is script observable too, at least for plugin subdocuments (do <script>s have load events?)). > > Finally, this is going to need lots of tests ... > > I wholeheartedly agree - a test suite is needed for this to land, bz also > suggested submitting our tests back to the W3C as well. Indeed. > > ::: content/base/src/nsContentUtils.cpp > > @@ +699,5 @@ > > > + * @param aAttribute the value of the sandbox attribute as an nsIDOMDOMSettableTokenList > > > + * @return the set of flags > > > + */ > > > +PRUint32 > > > +nsContentUtils::ParseSandboxAttributeToFlags(nsIDOMDOMSettableTokenList * aAttribute) > > > > You should rewrite this function to use > > nsCharSeparatedTokenizer<nsContentUtils::IsHTMLWhitespace>. As it is it'll > > match things like "allow-scriptsallow-same-origin" which are not allowed per > > spec. > > > > I'd give extra points for putting a > > > > typedef nsCharSeparatedTokenizer<nsContentUtils::IsHTMLWhitespace> > > HTMLSplitOnSpacesTokenizer (or something) > > > > in nsContentUtils.h and changing the existing users in the tree. > > yeah, this patch fails the MS test on whitespace etc. so i knew there were > some issues here - thanks for the suggestion. i'm not opposed to doing the > HTMLSplitOnSpacesTokenizer approach here, i could split off the work to > change existing users and add the typedef as a separate blocking bug and > knock that out first. Yeah, you don't need to do that here (or even before). But filing a bug for it so someone else can do it would be nice ;-) Might be a decent good first bug. > > ::: content/html/content/src/nsHTMLIFrameElement.cpp > > @@ +306,5 @@ > > > + bool aNotify) > > > +{ > > > + if (aName == nsGkAtoms::sandbox && aNameSpaceID == kNameSpaceID_None) { > > > + // parse the new value of the sandbox attribute, and if we have a docshell > > > + // set its sandbox flags appropriately > > > > When is a sandbox attribute set supposed to take effect? > > > > "While the sandbox attribute is specified, the iframe element's nested > > browsing context must have the flags given in the following list set. In > > addition, any browsing contexts nested within an iframe, either directly or > > indirectly, must have all the flags set on them as were set on the iframe's > > Document's browsing context when the iframe's Document was created." > > > > So it sounds like a sandbox attr change should only take effect for the next > > document in the iframe. Setting them on the docshell immediately means > > they'll take affect for future subloads even on the same document, right? > > You'll want either tests to show that this isn't the case or to change the > > code so that it isn't (and then tests :-)). > > yeah, that's my reading of the spec - an attr change only takes effect when > the next document is loaded in the iframe. Future subloads are an important > case to point out here, thanks - i believe that they should be based on the > flags on the document, NOT the docshell. And yes, this needs tests around it > for sure ! Yes, lots of tests. :-) > > @@ +2944,5 @@ > > > + parentAsItem = tmp; > > > + } > > > + } > > > + } > > > + > > > > Somebody who understand docshell (read bz, smaug) should look at this stuff. > > In my five minutes of looking at docshell it looks like this logic might fit > > better in CanAccessItem. > > i discussed this over email with bz and asked about putting this in > CanAccessItem and in the end, it was decided that it shouldn't. Ok.

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #32) > For JavaScript, does disabling JS on the docshell affect XBL bindings? > That'll break video controls, among other things. That is true but it's not Ian's problem.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #33) > > That is true but it's not Ian's problem. But it's a problem that might become more visible once this here is implemented. Bug 449358 is tracking it BTW.

- nsIDocument, nsIDocShell, nsIDOMHTMLIFrameElement IIDs bumped - added nsHTMLIFrameElement::FromContent(nsIContent*) - added PRUint32 nsHTMLIFrameElement::GetSandboxFlags() - to better match the HTML5 spec, break the flags up more, reverse their polarity and rename them. - change sandbox attribute parsing to use nsCharSeparatedTokenizerTemplate<nsContentUtils::IsHTMLWhitespace> - add NULL check to fix crash with chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/xpinstall/browser_navigateaway2.js - lots of other misc. cleanup from feedback from ms2ger and khuey next up: doing some more debugging of the MS test suite tests that I think should be passing but are not - and then moving on to document.cookie and blocking it appropriately. The big outstanding question at the moment : do I want to make the tests for this manual (as the Microsoft test suite is) or attempt to make automated tests ? Manual tests have the problem that they don't always get run on every checkin - automated tests would need (I think) some sort of event that happens when something is blocked (CSP does this with an observable notification, for example) - the con here is this doesn't test the thing attempting to be blocked actually is blocked, just that the event was fired.

Do case insensitive compare when looking at sandbox attribute and comparing it to the appropriate atoms. Makes another of the MS sandbox test cases pass (http://samples.msdn.microsoft.com/ietestcenter/html5/sandbox/sandbox-attribute-value-tokens-separated-by-space-characters.htm)

Also need to re-examine keeping the flags on the docshell and the document. Right now this patch essentially only uses the docshell flags to pass the correct flags on to the document when its loaded. The frame loader always checks (and parses) the actual current value of the sandbox attribute on the IFRAME being loaded and uses this to set the flags on the docshell - if this seems acceptable, the AfterSetAttr() code in nsHTMLIFrameElement needs to be removed.

(In reply to Ian Melven :imelven from comment #35) > Manual tests have the problem that they don't always get run on > every checkin - automated tests would need (I think) some sort of event that > happens when something is blocked (CSP does this with an observable > notification, for example) - the con here is this doesn't test the thing > attempting to be blocked actually is blocked, just that the event was fired. Automated tests of course :-). Manual tests are a bit useless. I think most of the features can be automatically checked fairly easily. In some cases you may need chrome privileges to inject synthetic events into the sandboxed document. Are there any features that you can't think of a way to test automatically?

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #38) > (In reply to Ian Melven :imelven from comment #35) > > Manual tests have the problem that they don't always get run on > > every checkin - automated tests would need (I think) some sort of event that > > happens when something is blocked (CSP does this with an observable > > notification, for example) - the con here is this doesn't test the thing > > attempting to be blocked actually is blocked, just that the event was fired. > > Automated tests of course :-). Manual tests are a bit useless. > > I think most of the features can be automatically checked fairly easily. In > some cases you may need chrome privileges to inject synthetic events into > the sandboxed document. Are there any features that you can't think of a way > to test automatically? Thanks for the feedback ! Can't think of anything specific offhand that I'm certain will be impossible to test automatically, maybe blocking video autoplay ? I'll see how it shakes out when I attempt writing the automated tests.

(In reply to Ian Melven :imelven from comment #39) > Can't think of anything specific offhand that I'm > certain will be impossible to test automatically, maybe blocking video > autoplay ? Reach into the sandboxed document (using chrome privileges if you want to test cross-origin), and add an event listener on the element's "playing" event. Wait for "suspended" to fire to signal that the element has finished downloading, and make sure the "playing" event hasn't fired.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #40) > (In reply to Ian Melven :imelven from comment #39) > > Can't think of anything specific offhand that I'm > > certain will be impossible to test automatically, maybe blocking video > > autoplay ? > > Reach into the sandboxed document (using chrome privileges if you want to > test cross-origin), and add an event listener on the element's "playing" > event. Wait for "suspended" to fire to signal that the element has finished > downloading, and make sure the "playing" event hasn't fired. actually re video autoplay, due to bug 449358 (i assume), video doesn't show up at all when allow-scripts isn't specified when I tested this with an ogg file. I also found another bug : If I do <iframe sandbox="allow-scripts"> <video width="320" height="240" controls="controls" autoplay> <source src="stockholm-vfr.ogg" type="video/ogg" /> </video> </iframe> the video doesn't show up and play as it should. If I move the <video> tag into another HTML file and use <iframe src="video_autoplay.html" sandbox="allow-scripts"> the video shows up and starts playing as expected.

(In reply to Ian Melven :imelven from comment #41) > actually re video autoplay, due to bug 449358 (i assume), video doesn't show > up > at all when allow-scripts isn't specified when I tested this with an ogg > file. Autoplay shouldn't depend on bug 449358, it's all in C++. > I also found another bug : > > If I do > > <iframe sandbox="allow-scripts"> > <video width="320" height="240" controls="controls" autoplay> > <source src="stockholm-vfr.ogg" type="video/ogg" /> > </video> > </iframe> > > the video doesn't show up and play as it should. There you're specifying the <video> as fallback for the <iframe>. Since we support <iframe>, the video is ignored.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #42) > (In reply to Ian Melven :imelven from comment #41) > > Autoplay shouldn't depend on bug 449358, it's all in C++. the video doesn't show up at all if JS is disabled it seems. > There you're specifying the <video> as fallback for the <iframe>. Since we > support <iframe>, the video is ignored. oh, that makes sense. Thanks. It doesn't seem to work that way in Chrome, but that's up to them :)

The spec says "Descendants of iframe elements represent nothing. (In legacy user agents that do not support iframe elements, the contents would be parsed as markup that could act as fallback content.)" So if Chrome shows the video there, that's a Webkit bug. (In reply to Ian Melven :imelven from comment #43) > the video doesn't show up at all if JS is disabled it seems. Odd. Can you reproduce if JS is just turned off in the browser? Anyway, it must be a separate bug.

- possible implementation of blocking reading or writing document.cookie if sandboxed origin flag is set - possible implementation of blocking access to localStorage and sessionStorage if sandboxed origin flag is set - use HTMLSplitOnSpacesTokenizer (thanks Santiago Gimeno!) - only create nsCaseInsensitiveStringComparator once and then reuse it Note : there needs to be a test that document.cookie can be redefined when actually reading or writing the cookie is not allowed due to sandboxing. This was brought up by Devdatta Akhawe. Adam Barth says Chrome considers this bug and will fix it. I believe this patch should allow that but it needs a test.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #44) > > Odd. Can you reproduce if JS is just turned off in the browser? Anyway, it > must be a separate bug. No, I can't - it works fine with JS turned off - my suspicion is it might be something to do with removing the iframe's document's natural origin/principal.

For reference, here's some statements by Hixie on the WHATWG list : "I don't intend to add sandbox="" to <frame> unless implementors find it easier to support than not support." "Adding sandbox="" to <object> has all kinds of weird edge cases because <object> sometimes has a browsing context and sometimes does not, so I do not intend to add this either." There had been some consensus about not adding sandbox to <object> for the very reason Hixie states. <frame> was more up in the air. Based on my work in the attached patch, I don't believe it's easier to support <frame> than not - there were some other objections about adding new attributes to a deprecated tag as well - so I'm not planning on supporting sandbox on <frame>.

> so I'm not planning on supporting sandbox on <frame>. That matches the WebKit implementation.

I spent a bit looking into how XHR works with the current patch. The summary is that (without allow-same-origin) requests via XHR behave like other cross domain XHR requests. The request hits the server but a status of 0 is returned and the response is not available. CORS won't be able to allow these requests unless you could somehow magically know the random domain of the null principal assigned to the sandboxed document. This all seems OK to me at the moment, but I'm open as always to differing opinions/feedback.

(In reply to Ian Melven :imelven from comment #46) > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #44) > > > > Odd. Can you reproduce if JS is just turned off in the browser? Anyway, it > > must be a separate bug. > > No, I can't - it works fine with JS turned off - my suspicion is it might be > something to do with removing the iframe's document's natural > origin/principal. I actually think the bug i was seeing is triggered when you have a <video> tag with a format Firefox can't play and JS turned off (in the sandbox's case, via disabling it on the sandboxed docshell). I wasn't seeing the "Video format or MIME type is not supported" graphic. I can reproduce THIS bug by turning off Javascript via Options. Note to self : file followup bug on this.

(In reply to Ian Melven :imelven from comment #49) > I spent a bit looking into how XHR works with the current patch. The summary > is that (without allow-same-origin) requests via XHR behave like other cross > domain XHR requests. The request hits the server but a status of 0 is > returned and the response is not available. CORS won't be able to allow > these requests unless you could somehow magically know the random domain of > the null principal assigned to the sandboxed document. Even if the server sends "Access-Control-Allow-Origin: *" ? Why, what's the security rationale for that?

(In reply to David-Sarah Hopwood from comment #51) > (In reply to Ian Melven :imelven from comment #49) > > I spent a bit looking into how XHR works with the current patch. The summary > > is that (without allow-same-origin) requests via XHR behave like other cross > > domain XHR requests. The request hits the server but a status of 0 is > > returned and the response is not available. CORS won't be able to allow > > these requests unless you could somehow magically know the random domain of > > the null principal assigned to the sandboxed document. > > Even if the server sends "Access-Control-Allow-Origin: *" ? Why, what's the > security rationale for that? Oh yeah, * should of course be able to allow it - thanks for pointing that out !

Comment on attachment 589638 [details] [diff] [review] WIP - prototype At this point, the patch has (I think) most of the functionality needed. The remaining functionality that doesn't have a possible implementation is blocking autoplay of a <video> within a sandboxed iframe without allow-scripts and not doing an autofocus on an input field within a sandboxed iframe without allow-scripts. I'd like to move on to start working on the test suite that will be needed for this feature. I know this needs TONS of tests (and have a list of a bunch of ones people have suggested earlier). It would be awesome to get feedback on the possible implementation in the patch so far in parallel with this :) The biggest question for me is : do we need nsHTMLIFrameElement::AfterSetAttr still ? at this point the frame loader uses the flags on the parent document and the attributes specified when the <iframe> is loaded. This removes the need (I think) for having a set of flags on the docshell and document and needing to keep them in sync (and also eliminates a race with subloads of the sandboxed <iframe>). It seems cleaner to me, but as always, I'm very open to feedback. Thanks !

This version is more recent and not (or at least hopefully less) bitrotted.

Comment on attachment 594862 [details] [diff] [review] WIP - prototype General comments: 1) Sandbox flag setter on the document seems to be unused. 2) Getter on document should probably just be SandboxFlags. 3) The case-insensitive compares in nsContentUtils::ParseSandboxAttributeToFlags should just use LowerCaseEqualsLiteral with literal ASCII strings, I think. 4) The code duplication in nsFrameLoader::ReallyStartLoadingInternal is bad. Computing sandbox flags up front (and explicitly setting them to 0 for the non-iframe case), then having one clause at the end to set the principal based on those would be better. 5) There are various places where random comments are being removed for no reason or indentation is off. 6) You should document that the attr needs to be parsed as an attr array so that the token list code can work with it. 7) You need to handle a null aValue in AfterSetAttr; that needs to reset the sandbox flags. Note also that aValue is about to change to an nsAttrValue* there... And add the newline at end of file. You also need to handle there being no docshell when AfterSetAttr is called. 8) "using namespace" doesn't belong in headers. Please take that part out. 9) In the docshell code, getting the document from mContentViewer is wrong, I think. Shouldn't we get it from the originating item? I didn't review the rest of that code for now. 10) In the global window code, just use mDoc. 11) Do we really need the various manual checks for SANDBOXED_ORIGIN? Is having the null principal not enough? Most of this looks pretty good, I think.

Forgive me if I am missing something, but I don't see any note on implementing support for srcdoc. Am I missing something, or is this another bug? My search didn't bring up anything.

srcdoc is completely unrelated to sandbox.

Yes, I realize that. But consider this: you want to isolate evil.html and bad.html from each other while showing them on good.com. What do you do? you serve them via iframe sandbox on good.com? But then an attacker could just do <iframe src=good.com/evil.html> on his own page. If you host it on good2.com domain, you protect good.com, but don't isolate evil.html and bad.html. The ony way to achieve this, if I am not wrong, is to make them evil.txt and bad.txt (via content type) and then XHR the content, load it up in an iframe with srcdoc set to the text. As a result, I thought srcdoc is related: srcdoc+sandbox can be a *really* powerful tool. Am I missing any other primitive?

(at least, this is true till csp adopts sandbox directive; which it may or may not. Adam might be able to answer that.

None of that has anything to do with this bug. Please file a separate bug on srcdoc if you want it and there isn't one already. Note that the use case from comment 48 can already be handled via data: URIs.

(In reply to dev from comment #59) > (at least, this is true till csp adopts sandbox directive; which it may or > may not. Adam might be able to answer that. yeah, i was under the impression the use case you bring up (of content asking to be sandboxed itself rather than a page choosing to sandbox it) is addressed via CSP sandbox.

(In reply to Boris Zbarsky (:bz) from comment #55) > Comment on attachment 594862 [details] [diff] [review] > WIP - prototype Thanks very much for the feedback. I still have a question about whether we need AfterSetAttr or not - currently it changes the flags on the docshell when the attribute is changed, but the flags on the docshell are only used to pass along the flags to the document when its created. This introduces a race between the time the load starts and the time the document is created. We do still need to keep the flags on the docshell to pass them to the document but do we need to have this race ? If the attribute is changed, it will still be read on a reload etc which is when the new sandbox flags are supposed to take effect. > 5) There are various places where random comments are being removed for no > reason or > indentation is off. Sorry about this, I see I somehow dropped a comment in nsContentUtils.h. I'll make another pass for indentation etc on the next iteration of the patch :)

(In reply to Boris Zbarsky (:bz) from comment #55) This patch addresses items 1-8 and 10 from your feedback. Thanks ! > 9) In the docshell code, getting the document from mContentViewer is wrong, > I think. Shouldn't we get it from the originating item? I didn't review the rest > of that code for now. Did you mean both places in nsDocShell.cpp ? (ie the FindItemWithName code and InternalLoad code). I assume for the latter I'd use the requestingElement's ownerDocument ? For the former, I think I can use aOriginalRequestor and then use do_GetInterface to get the nsIDocument for that nsIDocShellTreeItem ? > 11) Do we really need the various manual checks for SANDBOXED_ORIGIN? Is > having the null principal not enough? These were driven by making the MS test suite pass mostly. In some cases the null principal is enough (for example, XHR, which didn't need a manual check), in some cases it wasn't (cookies and localStorage, for example). I left AfterSetAttr in for now until we make a decision on whether it's needed (please see comment #62)

> Did you mean both places in nsDocShell.cpp ? No, just the FindItemWithName code, I think. > For the former, I think I can use aOriginalRequestor Yes, precisely. > These were driven by making the MS test suite pass mostly. Ah, ok. Hurray for tests! As far as comment 62, you do need the AfterSetAttr, as far as I can tell. Otherwise nothing will change the docshell flags, and then a navigation in that docshell that's not due to the src on the frame changing (most simply, a link click inside the iframe) won't pick up the new flags, right?

(In reply to Boris Zbarsky (:bz) from comment #64) > > Did you mean both places in nsDocShell.cpp ? > > No, just the FindItemWithName code, I think. > > For the former, I think I can use aOriginalRequestor > > Yes, precisely. ok, great ! i'll make this change in the next iteration. For now, i'm going to put up a version of the patch from earlier today that actually compiles so smaug can take a look at it as he said he will try to in the next couple of days :) > As far as comment 62, you do need the AfterSetAttr, as far as I can tell. > Otherwise nothing will change the docshell flags, and then a navigation in > that docshell that's not due to the src on the frame changing (most simply, > a link click inside the iframe) won't pick up the new flags, right? won't this go through the frame loader again and pick up the new flags (and set them on the docshell) when it reads the iframe's sandbox attribute ? if it doesn't go through the frame loader again, then it won't get the null principal etc. I smell something that needs a test case here at any rate. I also should probably walk through this in the debugger so I better understand the code.

Same as v1 but fixed the typos/parens etc so it compiles. See comment #63 and comment #64 which discuss a change to the FindItemWithName code that this still needs.

> won't this go through the frame loader again No. The only things that go through the frame loader are changes to the src attribute on the iframe. > if it doesn't go through the frame loader again, then it won't get the null principal etc. Good catch. That should get pushed down into the docshell, perhaps.

(In reply to Boris Zbarsky (:bz) from comment #67) > > won't this go through the frame loader again > > No. The only things that go through the frame loader are changes to the src > attribute on the iframe. > > > if it doesn't go through the frame loader again, then it won't get the null principal etc. > > Good catch. That should get pushed down into the docshell, perhaps. This is actually already taken care of in the patch. The null principal is set up in nsDocShell::DoURILoad (if the sandbox flags on the docshell say it should be) which gets called on a link click - so it behaves as we would like. I agree we need the AfterSetAttr now, thanks for working through that with me. There should be no way for an <iframe> contained document to change its flags without having allow-same-origin and allow-scripts as pointed out in the HTML5 spec. That case will get picked up by AfterSetAttr and then if load is triggered, the docshell flags that have been updated should be used. I might also need to move the JS and plugin disabling into the docshell though. Consider the case where an iframe has sandbox="allow-same-origin allow-scripts" and the sandboxed document accesses its parent and removes 'allow-scripts' and then triggers a reload. To summarize, 'this needs a test' :)

(In reply to Ian Melven :imelven from comment #68) > > > > if it doesn't go through the frame loader again, then it won't get the null principal etc. > > > > Good catch. That should get pushed down into the docshell, perhaps. > > This is actually already taken care of in the patch. The null principal is > set up in nsDocShell::DoURILoad (if the sandbox flags on the docshell say it > should be) which gets called on a link click - so it behaves as we would > like. Actually, this isn't quite right - in DoURILoad, SetupChannelOwner will be forced into setting the principal BUT it won't be the null principal as it should be in this case. So this does require further work in the patch.

Made the change in FindItemWithName to use aOriginalRequestor as discussed in comment #63 and comment #64 I found another bug earlier as well, one of the MS tests (http://samples.msdn.microsoft.com/ietestcenter/html5/sandbox/top-navigation-block-target-top.htm) fails when it's run directly from that URL and not wrapped in the main test suite frame. Going to look into that and fix up the null principal on click handling as mentioned in comment #69 next - and then start in on the tests.

This version of the patch moves the null principal assignment into the docshell so null principals are correctly assigned to documents loaded into the sandboxed iframe via for example, clicking a link in a sandboxed document. It also fixes the bug in nsDocShell::FindItemWithName mentioned in comment #70 On to the tests, which I am going to work on in a separate patch.

this makes the change with AfterSetAttr passing nsAttrValue that bz warned me about earlier. i'm working on the tests now and will upload them as a new patch once i get them to a good baseline (and then ask for feedback on them)

After struggling with working on the tests in one big monolithic patch, I've decided to break them up into smaller patches containing subsets of the tests. This should make it easier to get some stuff up here for feedback. There are subsets for same-origin tests and a main/general test containing forms/scripts and generic tests coming after I've split those out of the big patch I was working on. There's also some navigation tests that I was having issues with that helped prompt this decomposition. Some of the subsets of tests have issues/TODO's, I'll note those as I post each patch. There still need to be tests written for plugins. There still need to be a test written for allow-top-navigation (I think this might need to be a browser-chrome test ?). This particular patch contains a set of tests around inheritance of sandboxing. Ideas for more tests to increase coverage are very welcome ! There are no issues I'm aware of with the tests in this patch.

split out from the large test patch i was working on. these all pass and don't seem to have any issues. feedback, especially ideas for additional tests, very welcome !

Here is a patch with the 'general'/misc. tests split out. It includes tests for script execution (allow-scripts), form submsission (allow-forms) and window opening. There is one issue with this patch, the form submit test causes an assertion : \ ###!!! ASSERTION: Adding a child where we already have a child? This may misbeha ve: 'Error', file c:/Users/imelven/src/mozilla/docshell/shistory/src/nsSHEntry.c pp, line 628 LdrLoadDll: Blocking load of '' (SearchPathW didn't find it?) xul!nsDocShell::AddChildSHEntry+0x0000000000000073 (c:\users\imelven\src\mozilla \docshell\base\nsdocshell.cpp, line 3484) xul!nsDocShell::DoAddChildSHEntry+0x00000000000000DC (c:\users\imelven\src\mozil la\docshell\base\nsdocshell.cpp, line 3564) xul!nsDocShell::AddToSessionHistory+0x0000000000000B78 (c:\users\imelven\src\moz illa\docshell\base\nsdocshell.cpp, line 10097) xul!nsDocShell::OnNewURI+0x0000000000000888 (c:\users\imelven\src\mozilla\docshe ll\base\nsdocshell.cpp, line 9515) xul!nsDocShell::OnLoadingSite+0x00000000000000A6 (c:\users\imelven\src\mozilla\d ocshell\base\nsdocshell.cpp, line 9578) xul!nsDocShell::CreateContentViewer+0x000000000000048C (c:\users\imelven\src\moz illa\docshell\base\nsdocshell.cpp, line 7532) xul!nsDSURIContentListener::DoContent+0x0000000000000155 (c:\users\imelven\src\m ozilla\docshell\base\nsdsuricontentlistener.cpp, line 164) xul!nsDocumentOpenInfo::TryContentListener+0x00000000000002F5 (c:\users\imelven\ src\mozilla\uriloader\base\nsuriloader.cpp, line 709) xul!nsDocumentOpenInfo::DispatchContent+0x00000000000002FF (c:\users\imelven\src \mozilla\uriloader\base\nsuriloader.cpp, line 406) xul!nsDocumentOpenInfo::OnStartRequest+0x00000000000001CA (c:\users\imelven\src\ mozilla\uriloader\base\nsuriloader.cpp, line 294) xul!nsHttpChannel::CallOnStartRequest+0x000000000000045B (c:\users\imelven\src\m ozilla\netwerk\protocol\http\nshttpchannel.cpp, line 764) xul!nsHttpChannel::ContinueProcessNormal+0x0000000000000205 (c:\users\imelven\sr c\mozilla\netwerk\protocol\http\nshttpchannel.cpp, line 1260) xul!nsHttpChannel::ProcessNormal+0x0000000000000103 (c:\users\imelven\src\mozill a\netwerk\protocol\http\nshttpchannel.cpp, line 1198) xul!nsHttpChannel::ProcessResponse+0x0000000000000287 (c:\users\imelven\src\mozi lla\netwerk\protocol\http\nshttpchannel.cpp, line 1033) xul!nsHttpChannel::OnStartRequest+0x000000000000028F (c:\users\imelven\src\mozil la\netwerk\protocol\http\nshttpchannel.cpp, line 4186) xul!nsInputStreamPump::OnStateStart+0x00000000000000B1 (c:\users\imelven\src\moz illa\netwerk\base\src\nsinputstreampump.cpp, line 444) xul!nsInputStreamPump::OnInputStreamReady+0x000000000000007D (c:\users\imelven\s rc\mozilla\netwerk\base\src\nsinputstreampump.cpp, line 399) xul!nsInputStreamReadyEvent::Run+0x000000000000004A (c:\users\imelven\src\mozill a\xpcom\io\nsstreamutils.cpp, line 115) xul!nsThread::ProcessNextEvent+0x0000000000000347 (c:\users\imelven\src\mozilla\ xpcom\threads\nsthread.cpp, line 657) xul!NS_ProcessNextEvent_P+0x0000000000000054 (c:\users\imelven\src\mozilla\objdi r-dbg\xpcom\build\nsthreadutils.cpp, line 245) xul!mozilla::ipc::MessagePump::Run+0x00000000000001DF (c:\users\imelven\src\mozi lla\ipc\glue\messagepump.cpp, line 134) xul!MessageLoop::RunInternal+0x000000000000004E (c:\users\imelven\src\mozilla\ip c\chromium\src\base\message_loop.cc, line 209) xul!MessageLoop::RunHandler+0x0000000000000082 (c:\users\imelven\src\mozilla\ipc \chromium\src\base\message_loop.cc, line 202) xul!MessageLoop::Run+0x000000000000001D (c:\users\imelven\src\mozilla\ipc\chromi um\src\base\message_loop.cc, line 176) xul!nsBaseAppShell::Run+0x0000000000000050 (c:\users\imelven\src\mozilla\widget\ xpwidgets\nsbaseappshell.cpp, line 191) xul!nsAppShell::Run+0x0000000000000047 (c:\users\imelven\src\mozilla\widget\wind ows\nsappshell.cpp, line 256) xul!nsAppStartup::Run+0x000000000000006A (c:\users\imelven\src\mozilla\toolkit\c omponents\startup\nsappstartup.cpp, line 295) xul!XRE_main+0x00000000000033B1 (c:\users\imelven\src\mozilla\toolkit\xre\nsappr unner.cpp, line 3564) firefox!do_main+0x00000000000002BC (c:\users\imelven\src\mozilla\browser\app\nsb rowserapp.cpp, line 190) firefox!NS_internal_main+0x0000000000000226 (c:\users\imelven\src\mozilla\browse r\app\nsbrowserapp.cpp, line 277) firefox!wmain+0x0000000000000129 (c:\users\imelven\src\mozilla\toolkit\xre\nswin dowswmain.cpp, line 107) firefox!__tmainCRTStartup+0x00000000000001BF (f:\dd\vctools\crt_bld\self_x86\crt \src\crtexe.c, line 552) firefox!wmainCRTStartup+0x000000000000000F (f:\dd\vctools\crt_bld\self_x86\crt\s rc\crtexe.c, line 371) kernel32!BaseThreadI++DOMWINDOW == 21 (0D46E048) [serial = 21] [outer = 0D46CF60 ] nitThunk+0x0000000000000012 ntdll!RtlInitializeExceptionChain+0x0000000000000063 ntdll!RtlInitializeExceptionChain+0x0000000000000036 any help with this would be greatly appreciated !

> ###!!! ASSERTION: Adding a child where we already have a child? This may > misbeha > ve: 'Error', file I hope you're not hitting bug 645229.

iframe sandbox tests for navigation. these are based on the possible targets mentioned in the HTML spec, aside from target='_top' (which is affected by 'allow-top-navigation'). i'm going to work on tests for _top and plugins next. these navigation tests trigger the same assertion mentioned in comment 75, khuey said on irc that this may not be an issue though, so for now i'll move on to adding more tests.

it helps to remember to add the new files into the patch...

This patch contains a test for plugins not being allowed in sandboxed iframes. It tests loading a plugin via 1) <embed> 2) <object> and 3) a sandboxed iframe with src set to a document of a content type that is handled by a plugin. Thanks to Josh Aas for the tips on the plugin tests !

While working on a test to see if form validation happened when a submit is blocked inside a sandbox without 'allow-forms', i found it did. This is incorrect according to the HTML5 spec. This is a hacky fix sort of - there may be a better way along the lines of preventing the validation event from firing when blocking the actual submit. I'm still working on a mochitest for this - the validation API makes it very difficult to detect if validation has occurred since validation happens on initial load (ie the .validationMessage is set) and changing the content of a field also causes validation to occur. I could see it was happening when testing manually, my only idea right now is to try to detect some sort of styling change to the field when validation occurs on submit. I have a couple more test cases to add to the navigation tests, need to figure out how to finish the top-navigation tests i wrote, and then need to look into how JS workers interact with the sandbox. Discussing with Adam Barth, it seems like we should allow JS workers, but only when 'allow-scripts' and 'allow-same-origin' are specified. I need to look into what happens with this patch and what other browsers do here as well.

(In reply to Ian Melven :imelven from comment #80) > Created attachment 606238 [details] [diff] [review] > WIP - iframe sandbox v6 > I could see it was happening when testing manually, my > only idea right now is to try to detect some sort of styling change to the > field when validation occurs on submit. the style of the input element doesn't seem to change when the validation happens, so this won't work. Still open to suggestions on ways to test if form validation happens from content :)

add two tests to the iframe sandbox navigation tests: 1) If an iframe has sandbox="allow-same-origin allow-scripts", and then then the sandboxed document that it contains accesses its parent and removes 'allow-same-origin' from its sandbox attribute and then triggers a reload, the reloaded sandboxed document should not be able to access its parent. 2)) If an iframe has sandbox="allow-same-origin allow-scripts", and then the sandboxed document that it contains accesses its parent and removes 'allow-scripts' and then triggers a reload of itself, the reloaded sandboxed document should not be able to run a script and access its parent.

while writing navigation tests, i found that the change in comment 72 to account for AfterSetAttr now taking an nsAttrValue needed a little more work.

Improve the inheritance tests slightly and add a new test to make sure that changing the sandbox attribute of an iframe doesn't affect the sandboxing of subloads of content within that iframe.

Just a note that I confirmed by manual testing that the issue raised by Devdatta Akhawe via email(being able to redefine document.cookie, for example, within an iframe with sandbox = 'allow-scripts) is not an issue with the attached iframe sandbox patch :)

Another note that Web Workers work as I expected them to (and the way that Adam Barth and I agreed over email, we thought they would) - if you have 'allow-scripts' and 'allow-same-origin' you can create and start a worker, without both those attributes, you cannot.

Can you expand on the reasons behind that decision? Why not allow workers without allow-same-origin?

(In reply to dev from comment #87) > Can you expand on the reasons behind that decision? Why not allow workers > without allow-same-origin? A worker needs to be same origin with its 'entry script' per the web worker spec. Script inside an iframe sandboxed without 'allow-same-origin' will have a unique origin, and can't be same origin with the worker.

What if the worker uses a data: URI, or a URI for a Blob created with window.createObjectURL?

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #89) > What if the worker uses a data: URI, or a URI for a Blob created with > window.createObjectURL? Good question. I assume these cases work fine in the non-sandboxed world. I'll have to think about this some more.

https://developer.mozilla.org/en/Using_web_workers#Spawning_a_worker has the info you need. Also, I suggest thinking about newer platforms like B2G. It might be useful for a sandboxed app to run things with a worker? Does anyone have a reference (to probably the mailing list thread) as to why web worker scripts were not allowed cross origin ?

add allow-top-navigation tests to the navigation tests.

(In reply to dev from comment #91) > https://developer.mozilla.org/en/Using_web_workers#Spawning_a_worker has the > info you need. > > Also, I suggest thinking about newer platforms like B2G. It might be useful > for a sandboxed app to run things with a worker? Does anyone have a > reference (to probably the mailing list thread) as to why web worker scripts > were not allowed cross origin ? After some discussion with Jonas, i think we agree that workers should be able to use data: URLs without 'allow-same-origin'. Note that Firefox supports using data: URLs in Web Workers in normal, unsandboxed documents, which IE10 and Webkit do not AFAICT. We also think that workers should be able to use Blob URL's as long as the Blob was created by the sandboxed document that creates the worker - if the Blob was created in some other document, sandboxed or not, it will not have the same unique domain as the worker, and should not be allowed. Note that this is different from the Webkit implementation which requires 'allow-same-origin' for a sandboxed document to be able to load a worker with a Blob URL. AFAICT, IE10 does not allow workers to be loaded with Blob URL's, even in non-sandboxed documents. Note that the current patch does not behave this way, in Gecko, workers cannot currently be loaded with content with a null principal, I'm discussing this with some folks currently.

Also, i sent the patch and all the tests to try and it appears to have some hang issues (across multiple platforms) - so the tests need some more work : 84063 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_iframe_sandbox_general.html | Test timed out. 84067 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_iframe_sandbox_inheritance.html | Test timed out. 84072 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_iframe_sandbox_navigation.html | Test timed out. 84085 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_iframe_sandbox_same_origin.html | XHR should work normally in an iframe sandboxed with 'allow-same-origin' 84086 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_iframe_sandbox_same_origin.html | Test timed out. 84087 ERROR TEST-UNEXPECTED-FAIL | (SimpleTest/TestRunner.js) | 4 test timeouts, giving up. 84088 ERROR TEST-UNEXPECTED-FAIL | (SimpleTest/TestRunner.js) | Skipping 228 remaining tests. 84089 INFO TEST-UNEXPECTED-FAIL | (SimpleTest/TestRunner.js) | finished in a non-clean fashion (in /tests/content/html/content/test/test_iframe_sandbox_same_origin.html)

And another note that the type of the attribute is currently being discussed on the WHATWG list : 1) in general, folks don't like [PutForwards=value], which the HTML5 spec has on the DOMSettableTokenList sandbox attribute - but without forwarding to value, e.g. iframe.sandbox = 'allow-scripts' is a silent nop - it works as one would expect in IE10 (which does the forward) and Webkit (which doesn't implement the attribute as a DOMSettableTokenList). Although I don't like PutForwards much either, I really don't like the inconsistency with other implementations here. 2) content authors need to use removeAttribute and hasAttribute to clear or detect the difference between <iframe> and <iframe sandbox=""> since iframe.sandbox = "" is true in both cases. There is discussion about adding another attribute to the DOMSettableTokenList (e.g. iframe.sandbox.present) 3) bz pointed out that as it stands iframe1.sandbox = iframe2.sandbox may behave surprisingly - consider that iframe1 and iframe2 are originally not sandboxed (their sandbox attribute will be ""). Doing this assignment will result in iframe1 being sandboxed with no granted permissions (when the document in iframe1 is reloaded and the new sandboxing is applied).

Also, on reflection (and discussion with Sid Stamm), I think the JS disabling in this patch is too heavy handed. I would like to rework it to use the same choke points CSP does for executing scripts - this should remove the issues with for example, video controls not working in a sandboxed document, and should be a much more useful implementation. Feedback on this idea is very welcome - the immediate concerns seems to be performance impact (but I'm hopeful that in the usual, non sandboxed case, just checking the sandboxFlags member of the document won't be too bad) and whether the CSP choke points cover all the cases we would want to block if allow-scripts isn't specified.

dom/interfaces/html/nsIDOMHTMLIFrameElement.idl bitrotted, updated.

I'm also thinking that maybe it makes more sense for the sandbox flags to be on nsIDocument rather than nsIDocShell.

Which particular sandbox flags?

(In reply to Boris Zbarsky (:bz) from comment #99) > Which particular sandbox flags? the constants - both the docshell and the document have actual flags set on them. ie instead of nsIDocShell::SANDBOXED_ORIGIN it's nsIDocument::SANDBOXED_ORIGIN

Oh, OK. You could also just have a small header file with an enum for the flags in it.

(In reply to Boris Zbarsky (:bz) from comment #101) > Oh, OK. You could also just have a small header file with an enum for the > flags in it. yeah, that might be a little cleaner - maybe in content/base/public/ i'll probably do that when i try to fix up the script blocking, after i sort out this worker stuff :)

i broke the XHR test during the refactor, fixed now, and also cleaned up a bunch of evil whitespace.

some updates : * i now plan to change the implementation of sandbox to be a DOMString after discussion on the WHATWG list * the workers code needs some changes to have Blob and data: URL's work as described in comment 93, i have a hacky solution and am waiting for feedback next, i'm going to rework the script blocking as described in comment 96 and move the flags to their own header as bz suggested in comment 101

Just a quick note that with script blocking reworked to use the same choke points that CSP uses, the issues mentioned in comment 50 and comment 34 are no longer present - that is to say, with the improved script blocking, the 'unsupported media coded' error graphic and video controls all work correctly in a sandboxed document without 'allow-scripts' - i think this is a strong argument for going with this implementation. I'll attach a new patch once I write some more tests - I realized for script blocking we should also test blocking javascript: URLs and <script src=...> in a sandboxed document without 'allow-scripts'.

Add tests for executing script (or blocking it, as appropriate) from <script src=.. > and javascript: urls as well as inline <script>.

(In reply to Ian Melven :imelven from comment #106) > Created attachment 612006 [details] [diff] [review] > iframe sandbox tests - general v2 > > Add tests for executing script (or blocking it, as appropriate) from <script > src=.. > and javascript: urls as well as inline <script>. This javascript: URL test (which is in an onLoad handler) led me to discover that the patch with more specific script blocking allows script in <body onload=.. to run. I verified that it does block javascript: URLs when they're the href of a clicked on <a> tag. I'm sticking it in the bug but need to dig into this.

This is the patch with more granular script disabling - I need to fix script not being blocked in onload handlers in it. This patch also moves the sandbox flags to content/base/src/nsSandboxFlags. They're just consts now, I'm happy to put them in an enum or as static members of a class, etc. if needed.

(In reply to Ian Melven :imelven from comment #108) > I need to fix script not being blocked in onload handlers in it. Will that also include onclick/onkeydown/onhashchange/etc?

(In reply to Mardeg from comment #109) > (In reply to Ian Melven :imelven from comment #108) > > I need to fix script not being blocked in onload handlers in it. > Will that also include onclick/onkeydown/onhashchange/etc? yes - in a sandboxed iframe without 'allow-scripts', no content scripts (inline scripts, <script src=..>, javascript: URLs or event handler scripts should be run.

This iteration of the patch blocks adding event listeners in the same place CSP does and stops onload scripts from working. I need to rework the script blocking tests in 'iframe sandbox tests - general' to better test javascript: urls and add a test for event listeners.

Improve the script blocking tests in the general tests - a better test for script in javascript: urls and an added test for script in event listeners. Next up I'm going to tackle the workers stuff as described in comment 93, including writing tests. This will also require changing the worker code to not use string origins or tackling that in a followup bug.

unbitrot.

This is the patch to let workers run with a null principal (ie in a sandboxed iframe) and work around workers needing a string origin - bz, I just wanted you to be able to have a look at it so you can see what the workaround was - the intent is to clean this up in a followup after iframe sandbox lands with a change to not use the string origin in the worker at all, if that's still alright. :)

This is the CheckMayLoad change that adds a new flag, aAllowIfInheritsPrincipal, as discussed. When discussing with dveditz he mentioned we might want to make this a uint32 instead of a bool to give us some room to expand (which would probably mean folding in the existing report bool as a flag as well) - just mentioning it :) There's some places called out with // REVIEW where I wasn't quite sure what we want to do and would appreciate feedback. This patch allows sandboxed workers to successfully load blob URI's (created by the sandboxed document) and data URI's. Once this and the string origin workaround look alright, I'll fold them into the main iframe sandbox patch.

Note that javascript: URI's are currently (and also with the iframe sandbox/CheckMayLoad changes) unable to be used to load workers, this fails in nsJSThunk::EvaluateScript since the nsJSChannel does not have an owner/principal.

A try run with iframe sandbox v10 patch, the check may load patch and the worker string origin workaround patch, it looks pretty good to me fwiw https://tbpl.mozilla.org/?tree=Try&rev=9c13ae50406e

Next up : * change the implementation of sandbox to be a DOMString instead of of a DOMSettableTokenList Then: * add tests for workers in sandboxed frames and loading them from various kinds of URIs * push to try with all code patches and all tests - this hung last time and timed out, I think I know why, need to tweak the tests to not use window.top since it's different when running in the whole mochitest suite as opposed to one single mochitest After these are done, there's a couple followups to file and then I'd like to start getting feedback/review on the patch and associated tests :)

changes the sandbox attribute to be a string instead of a DOMSettableTokenList

changes the general tests to account for the attribute becoming a string

tests for workers in sandboxed iframes * test that a worker in a sandboxed iframe with 'allow-scripts' can be loaded from a data: URI * test that a worker in a sandboxed iframe with 'allow-scripts' can be loaded from a blob URI created by the sandboxed document itself * test that a worker in a sandboxed iframe with 'allow-scripts' without 'allow-same-origin' cannot load a script via a relative URI done by file_iframe_sandbox_g_if1.html) * when bug 722126 "Can't transfer File objects with postMessage cross domain" is fixed, need to add a test that a worker in a sandboxed iframe with 'allow-scripts' cannot be loaded from a blob URI not created by the sandboxed document itself (marked with a // TODO in this patch)

Unbitrot.

Comment on attachment 615924 [details] [diff] [review] CheckMayLoad changes to allow sandboxed workers to load blob and data URIs Dan, you probably have some thoughts on this too :)

Fix the hang on the try server by not using window.top since it's different when running as part of the suite than when running as a standalone test, this made it through try and looks good : https://tbpl.mozilla.org/?tree=Try&rev=9707b0406fee Kyle, if you have a moment, could you give this set of tests a look over ? The other sets of tests are much along the same lines so any overall feedback based on this set will affect them too. I also need to go through the other tests and fix them up to avoid hangs as was done for this set of tests - when I do that I'll push all the code patches and all the test patches together to try.

Unfortunately its likely to be well into May before I have time to look through these in depth, so if you need this in a reasonable timeframe I suggest looking for someone else :-/

Comment on attachment 616649 [details] [diff] [review] iframe sandbox tests - general v5 Feedback on this set of tests appreciated - feel free to look at the other tests as well, I am in the middle of fixing them up to not use window.top and pushing everything to try

Clean up whitespace, fix hang by using .parent instead of .top to report success/failure.

Clean up whitespace, change tests to use .parent instead of .top to avoid hangs

Clean up whitespace, change the tests to use .parent instead of .top so they work when running the whole test suite, fix up a test that depended on .sandbox.value which doesn't exist any more

A try run with all the code patches and all the tests - looks pretty good to me. https://tbpl.mozilla.org/?tree=Try&rev=d160e769296f

Comment on attachment 616649 [details] [diff] [review] iframe sandbox tests - general v5 These tests look good to me!

Comment on attachment 616649 [details] [diff] [review] iframe sandbox tests - general v5 Review of attachment 616649 [details] [diff] [review]: ----------------------------------------------------------------- Looks generally good. I can't say a lot about the content being tested given that I don't know the spec that well. ::: content/html/content/test/file_iframe_sandbox_c_if1.html @@ +28,5 @@ > +<script src='file_iframe_sandbox_pass.js'></script> > +<body onLoad='ok(true, "documents sandboxed with allow-scripts should be able to run script from event listeners");doStuff();'> > + I am sandboxed but with "allow-scripts" > + > + <form method="get" action="file_iframe_sandbox_form_pass.html" id="a_form"> Why the form should be submitted if 'allow-forms' isn't passed? @@ +31,5 @@ > + > + <form method="get" action="file_iframe_sandbox_form_pass.html" id="a_form"> > + First name: <input type="text" name="firstname"> > + Last name: <input type="text" name="lastname"> > + <input type="submit" onclick="doSubmit()" id="a_button"> Why not just call .submit() directly instead of synthetizing a mouse event? ::: content/html/content/test/file_iframe_sandbox_c_if4.html @@ +21,5 @@ > + // should it ? > + try { > + window.open("file_iframe_sandbox_open_window_fail.html"); > + } catch (error) { > + ok(true, "window.open threw a JS exception and was not allowed"); Note that if it's not throwing, you will not hear about it. You should use something like |var caught = false;|, set it to true in the |catch| block and test that value after the try/catch. Same for the try/catch block below. ::: content/html/content/test/test_iframe_sandbox_general.html @@ +30,5 @@ > + > +function ok_wrapper(result, desc) { > + ok(result, desc); > + > + completedTests++; In general, I think it would have been better to use a specific message like "complete" for each iframe instead of counting how many tests are done and waiting for all of them. It's just about making sure those tests are scaling correctly. In the short term, it's not very important. @@ +41,5 @@ > + //console.log("RESULT : " + result + " DESC : " + desc); > + > + // REVIEW : if one of these tests fails and this isn't called > + // this means this test will hang until the time out happens :( > + // advice on better ways to synchronize things appreciated ! Test failing on timeout is not a problem. You shouldn't worry about that unless you got random failures ;) @@ +43,5 @@ > + // REVIEW : if one of these tests fails and this isn't called > + // this means this test will hang until the time out happens :( > + // advice on better ways to synchronize things appreciated ! > + if (completedTests == 9) { > + ok(passedTests == 9, "There are 9 general tests that should pass"); Please use is(passedTests, 9, "..."); @@ +49,5 @@ > + } > +} > + > +function doTest() { > + SimpleTest.waitForExplicitFinish(); I would prefer that method to be called outside of the load event handler. @@ +124,5 @@ > + // the window it attempts to open calls window.opener.ok(false, ...) and file_iframe_c_if4.html has an ok() > + // function that calls window.parent.ok_wrapper > +} > +</script> > +<body onload="doTest()"> Why not addLoadEvent(doTest)?

Comment on attachment 615917 [details] [diff] [review] workaround for workers needing a string origin This is a workaround for workers needing string origins, which a null principal worker (in a sandboxed document won't have) - the plan is to, in a followup, fix workers to not need this - it looks like they use it for max worker count and also some memory reporting on a first pass. If this patch looks ok, i'll fold it into the main iframe sandbox code patch.

Comment on attachment 616405 [details] [diff] [review] iframe sandbox v12 Talked to jst, r?'ing to smaug

Comment on attachment 615924 [details] [diff] [review] CheckMayLoad changes to allow sandboxed workers to load blob and data URIs jst : this is the CheckMayLoad patch i was talking about - would appreciate a look. thanks !

Comment on attachment 615917 [details] [diff] [review] workaround for workers needing a string origin r=jst to land this and file a followup to do this better later.

Comment on attachment 615924 [details] [diff] [review] CheckMayLoad changes to allow sandboxed workers to load blob and data URIs - In caps/idl/nsIPrincipal.idl: - void checkMayLoad(in nsIURI uri, in boolean report); + void checkMayLoad(in nsIURI uri, in boolean report, + in boolean allowIfInheritsPrincipal); This is a binary incompatible change you need to bump the uuid for this interface. - In nsNullPrincipal::CheckMayLoad(): + // If the caller specified to allow loads of URIs that inherit + // our (null) principal, allow the load if the loadee URI has + // the URI_INHERITS_SECURITY_CONTEXT flag set. + bool doesInheritSecurityContext; + nsresult rv = + NS_URIChainHasFlags(aURI, + nsIProtocolHandler::URI_INHERITS_SECURITY_CONTEXT, + &doesInheritSecurityContext); + + if (NS_SUCCEEDED(rv) && doesInheritSecurityContext) { + return NS_OK; + } This hunk of code is identical to the code in nsPrincipal.cpp. Maybe split this piece out into a static inline helper in nsPrincipal.h? Won't save any code here, but at least the details around this check will be centralized. - In nsPrincipal::CheckMayLoad(): + // Also allow the load if the principal of the URI being checked is exactly + // us ie this. + nsCOMPtr<nsIURIWithPrincipal> uriPrinc = do_QueryInterface(aURI); + if (uriPrinc) { + nsCOMPtr<nsIPrincipal> principal; + uriPrinc->GetPrincipal(getter_AddRefs(principal)); + + if (principal && principal == this) { + return NS_OK; + } + } This check seems unnecessary here, the following same origin check will succeed if the principals are the same. - In nsExternalResourceMap::PendingLoad::StartLoad(): + + // REVIEW : is it really ok to allow javascript: URI's here ?? + AFAIK, yes, this is fine. - In nsXMLHttpRequest::CheckChannelForCrossSiteRequest(): // ...or if this is a same-origin request. if (nsContentUtils::CheckMayLoad(mPrincipal, aChannel)) { return NS_OK; } + // REVIEW: since the check above is on the channel and the one + // below is on the final channel URI, I'm not sure we want to + // pass true for aAllowIfInheritsPrincipal to CheckMayLoad above + // the below could be replaced with a call to CheckMayLoad for + // channelURI passing true for aAllowIfInheritsPrincipal though, + // if we wanted to allow other URI's that inherit the principal of + // their creator as well as data URIs. nsContentUtils::CheckMayLoad() also extracts the final channel URI, so only one URI involved here... - In nsXMLDocument::Load(): nsCOMPtr<nsIPrincipal> principal = NodePrincipal(); if (!nsContentUtils::IsSystemPrincipal(principal)) { - rv = principal->CheckMayLoad(uri, false); + // REVIEW : it might be desirable to pass true as the + // 3rd param here and allow loading of URIs that inherit + // the principal of their creator. Not sure about javascript: + // though. + rv = principal->CheckMayLoad(uri, false, false); This is fairly well a deprecated API (nobody calls document.load() any more), so I'm happy to leave this unchanged. This generally looks good, but I'd like to look over an updated patch, so feedback- until then.

clean up the workers tests with the applicable feedback from Mounir in comment 132, sending for review

unbitrot

Address jst's feedback in comment 137 > - In caps/idl/nsIPrincipal.idl: > This is a binary incompatible change you need to bump the uuid for this > interface. done > - In nsNullPrincipal::CheckMayLoad(): > This hunk of code is identical to the code in nsPrincipal.cpp. Maybe split > this piece out into a static inline helper in nsPrincipal.h? Won't save any > code here, but at least the details around this check will be centralized. i moved this check to be a static in nsPrincipal, i'm happy to avoid duplication of code ! > - In nsPrincipal::CheckMayLoad(): > This check seems unnecessary here, the following same origin check will > succeed if the principals are the same. right, i removed this block > - In nsXMLHttpRequest::CheckChannelForCrossSiteRequest(): > nsContentUtils::CheckMayLoad() also extracts the final channel URI, so only > one URI involved here... thanks for pointing that out - i changed this to call nsContentUtils::CheckMayload and passed true (allow inherited principals) for the new param > This generally looks good, but I'd like to look over an updated patch, so > feedback- until then. Thanks very much for looking this over !

address Mounir's feedback from comment 132 : fix the blocked form test (thanks for catching that!) and do the other cleanup. I couldn't get .submit to work originally, but it seems to work fine now (probably because I don't have display:none on the test content iframe any more). Also add a test that adding an event listener (onerror) to an <img> is blocked.

Comment on attachment 621203 [details] [diff] [review] iframe sandbox v13 > /** >+ * A helper function that parses a sandbox attribute (of an <iframe> or >+ * a CSP directive) and converts it to the set of flags used internally. >+ * >+ * @param aAttribute the value of the sandbox attribute >+ * @return the set of flags >+ */ >+ static PRUint32 ParseSandboxAttributeToFlags(const nsAString& >+ sandboxAttr); Parameters should be in form aParameterName. Same also elsewhere in the patch. >+ >+ if (!sandboxAttrValue.IsEmpty()) { >+ // The separator optional flag is used because the HTML5 spec says any >+ // whitespace is ok as a separator, which is what this does. >+ HTMLSplitOnSpacesTokenizer tokenizer(sandboxAttrValue, ' ', >+ nsCharSeparatedTokenizerTemplate<nsContentUtils::IsHTMLWhitespace>::SEPARATOR_OPTIONAL); You have tests for various whitespaces? \t \n ' ' ? > > #ifdef MOZ_XTF > nsIXTFService* > nsContentUtils::GetXTFService() > { > if (!sXTFService) { > nsresult rv = CallGetService(kXTFServiceCID, &sXTFService); > if (NS_FAILED(rv)) { >@@ -6453,17 +6500,18 @@ nsContentUtils::URIInheritsSecurityConte > aResult); > } > > // static > bool > nsContentUtils::SetUpChannelOwner(nsIPrincipal* aLoadingPrincipal, > nsIChannel* aChannel, > nsIURI* aURI, >- bool aSetUpForAboutBlank) >+ bool aSetUpForAboutBlank, >+ bool aForceOwner) > { > // > // Set the owner of the channel, but only for channels that can't > // provide their own security context. > // > // XXX: It seems wrong that the owner is ignored - even if one is > // supplied) unless the URI is javascript or data or about:blank. > // XXX: If this is ever changed, check all callers for what owners >@@ -6471,23 +6519,27 @@ nsContentUtils::SetUpChannelOwner(nsIPri > // comments in nsDocShell::LoadURI where we fall back on > // inheriting the owner if called from chrome. That would be > // very wrong if this code changed anything but channels that > // can't provide their own security context! > // > // (Currently chrome URIs set the owner when they are created! > // So setting a NULL owner would be bad!) > // >+ // If aForceOwner is true, the owner will be set, even for a channel that >+ // can provide its own security context. This is used for the HTML5 IFRAME >+ // sandbox attribute, so we can force the channel (and its document) to >+ // explicitly have a null principal. Well, is there ever a case when we have aForceOwner == true and aLoadingPrincipal isn't nullprincipal? The current setup is a bit error prone. At least add an assertion (MOZ_ASSERT) that if aForceOwner is true, aLoadingPrincipal is nullprincipal >@@ -2415,16 +2415,25 @@ nsDocument::StartDocumentLoad(const char > FindCharInReadable(';', semicolon, end); > SetContentTypeInternal(Substring(start, semicolon)); > } > > RetrieveRelevantHeaders(aChannel); > > mChannel = aChannel; > >+ // If this document is being loaded by a docshell, copy its sandbox flags >+ // to the document. These are immutable after being set here. >+ nsCOMPtr<nsIDocShell> docShell = do_QueryInterface(aContainer); >+ >+ if (docShell) { >+ nsresult rv = docShell->GetSandboxFlags(&mSandboxFlags); >+ NS_ENSURE_SUCCESS(rv, rv); >+ } So, it is not quite clear to me why both docshell and document have the flags >@@ -473,16 +476,60 @@ nsFrameLoader::ReallyStartLoadingInterna > // Just to be safe, recheck uri. > rv = CheckURILoad(mURIToLoad); > NS_ENSURE_SUCCESS(rv, rv); > > nsCOMPtr<nsIDocShellLoadInfo> loadInfo; > mDocShell->CreateLoadInfo(getter_AddRefs(loadInfo)); > NS_ENSURE_TRUE(loadInfo, NS_ERROR_FAILURE); > >+ // Is this an <iframe> with a sandbox attribute or a parent which is >+ // sandboxed ? >+ nsHTMLIFrameElement * iframe = >+ nsHTMLIFrameElement::FromContent(mOwnerContent); Extra space between nsHTMLIFrameElement and * >+ >+ PRUint32 sandboxFlags = 0; >+ >+ if (iframe) { >+ sandboxFlags = iframe->GetSandboxFlags(); >+ >+ PRUint32 parentSandboxFlags = 0; >+ >+ // Get the parent's sandbox flags, they're needed to determine if the >+ // frame being loaded needs to be sandboxed. >+ nsCOMPtr<nsIDocShellTreeItem> parentDocShellItem; >+ nsCOMPtr<nsIDocShellTreeItem> curDocShellItem = do_QueryInterface(mDocShell); >+ >+ if (NS_SUCCEEDED(curDocShellItem->GetParent(getter_AddRefs(parentDocShellItem)))) { >+ nsCOMPtr<nsIDocument> parentDoc = do_GetInterface(parentDocShellItem); >+ >+ if (parentDoc) { >+ parentSandboxFlags = parentDoc->SandboxFlags(); >+ } >+ } Any reason why not PRUint32 parentSandboxFlags = iframe->OwnerDoc()->GetParentDocument() ? iframe->OwnerDoc()->GetParentDocument()->SandboxFlags() : 0; >+ >+ if (sandboxFlags || parentSandboxFlags) { >+ if (parentSandboxFlags) { do you need this if for anything? >- return DoSubmit(aEvent); >+ // Don't submit if we're in a sandboxed frame and form submit >+ // is disabled. >+ PRUint32 sandboxFlags = doc->SandboxFlags(); >+ >+ if (!(sandboxFlags & SANDBOXED_FORMS)) { >+ return DoSubmit(aEvent); >+ } Maybe return doc->SandboxFlags() & SANDBOXED_FORMS) ? NS_OK : DoSubmit(aEvent); >+ // Don't do validation for a form submit done by a sandboxed document that >+ // doesn't have 'allow-forms', the submit will have been blocked and the >+ // HTML5 spec says we shouldn't validate in this case. >+ nsIDocument* doc = GetCurrentDoc(); >+ if (doc) { >+ PRUint32 sandboxFlags = doc->SandboxFlags(); >+ >+ if (sandboxFlags & SANDBOXED_FORMS) { >+ return true; >+ } No reason for the sandboxFlags variable. Just use the value from SandboxFlags() >+nsHTMLIFrameElement::AfterSetAttr(PRInt32 aNameSpaceID, nsIAtom* aName, >+ const nsAttrValue* aValue, >+ bool aNotify) Are there any tests for bfcached documents? I mean the case when you first load a document to an iframe which has certain sandbox attribute. Then a new document is loaded. Then sandbox attribute is modified, and back() is executed so that iframe gets the old document. >+ // nsIContent >+ virtual bool ParseAttribute(PRInt32 aNamespaceID, >+ nsIAtom* aAttribute, >+ const nsAString& aValue, >+ nsAttrValue& aResult); indentation >+ PRUint32 GetSandboxFlags() { { should be in the next line >+ static nsHTMLIFrameElement* FromContent(nsIContent *aContent) >+ { >+ if (aContent->IsHTML(nsGkAtoms::iframe)) >+ return static_cast<nsHTMLIFrameElement*>(aContent); if (expr) { stmt; } >+ // If the document's sandboxed origin flag is set, access to read cookies >+ // is prohibited. >+ if (mSandboxFlags & SANDBOXED_ORIGIN) >+ return NS_ERROR_DOM_SECURITY_ERR; if (expr) { stmt; } >+ // If the document's sandboxed origin flag is set, access to write cookies >+ // is prohibited. >+ if (mSandboxFlags & SANDBOXED_ORIGIN) >+ return NS_ERROR_DOM_SECURITY_ERR; ditto >@@ -2986,16 +2988,73 @@ nsDocShell::FindItemWithName(const PRUni > foundItem = nsnull; > } > > if (foundItem) { > // We return foundItem here even if it's not an active > // item since all the names we've dealt with so far are > // special cases that we won't bother looking for further. > >+ // If our document is sandboxed, we need to do some extra checks. >+ PRUint32 sandboxFlags = 0; >+ >+ nsCOMPtr<nsIDocument> doc = do_GetInterface(aOriginalRequestor); >+ >+ if (doc) { >+ sandboxFlags = doc->SandboxFlags(); Docshell uses 4 space indentation. (I know, it is strange.) You're mixing 2 and 4 spaces

Comment on attachment 621221 [details] [diff] [review] iframe sandbox tests - general v6 Cancelling review, smaug made a good point that the general tests should check the whitespace handling of the attribute works as spec'd - i've tested this manually using the MS sandbox test suite and it passes but i think this should be in our automated test suite as well. Will re-submit for review when I get these added.

While addressing smaug's review comments and working on adding whitespace tests to the general test suite, I found opening the web console now causes an assertion and then a crash : Assertion failure: principals == JS_GetCompartmentPrincipals((js::GetContextComp artment(cx))), at c:/Users/imelven/src/mozilla/caps/src/nsScriptSecurityManager. cpp:208 TEST-UNEXPECTED-FAIL | automation.py | Exited with code -1073741819 during test run INFO | automation.py | Application ran for: 0:00:18.239000 INFO | automation.py | Reading PID log: c:\users\imelven\appdata\local\temp\tmp6 a8av1pidlog PROCESS-CRASH | automation.py | application crashed (minidump found) looks like this code changed about a month ago as part of the fix to Bug 739825 - i suspect this might be the result of iframe sandbox's use of null principals, i need to debug to see what's going on here

(In reply to Olli Pettay [:smaug] from comment #142) > Comment on attachment 621203 [details] [diff] [review] > iframe sandbox v13 Thanks very much for the feedback - i've cleaned up the patch with all of your suggestions. I will reupload it once I sort out the crash in comment 144 and making sure the cleaned up patch passes the bfcache tests I will add to the navigation tests and the whitespace tests I have added to the general tests - i've tested both of those manually but completely agree they should be in the automated tests for iframe sandbox. > Well, is there ever a case when we have aForceOwner == true and > aLoadingPrincipal isn't > nullprincipal? The current setup is a bit error prone. > At least add an assertion (MOZ_ASSERT) that if aForceOwner is true, > aLoadingPrincipal is nullprincipal no, there is no case like that currently - i get the URI from aLoadingPrincipal and then assert that SchemeIs(loadingURI, NS_NULLPRINCIPAL_SCHEME), does that sound alright ? > So, it is not quite clear to me why both docshell and document have the flags the idea here is to satisfy the spec requiring that changes to the sandbox flags only take place when the document is next loaded - changes to the attribute get put on the docshell via AfterSetAttr and then are set on the document when it's loaded. The flags on the document are always used for checking whether things are allowed or not. eg running scripts. this is discussed a little in comment 62, 64, 65, 67, 68 i'm open to other approaches, for example, the document could keep the flags set via AfterSetAttr in another variable and then move them from the current attribute state to the document itself on load. it seems like we will always need to keep the current 'desired state' (the attribute) separate from the current 'applied state' somehow. > Docshell uses 4 space indentation. (I know, it is strange.) > You're mixing 2 and 4 spaces indentation in docshell ALWAYS gets me :)

(In reply to Ian Melven :imelven from comment #145) > > So, it is not quite clear to me why both docshell and document have the flags > > the idea here is to satisfy the spec requiring that changes to the sandbox > flags only take place when the document is next loaded Ah, ok. Makes sense. Just add some tests for it, especially for bfcached documents.

(In reply to Olli Pettay [:smaug] from comment #146) > (In reply to Ian Melven :imelven from comment #145) > > > So, it is not quite clear to me why both docshell and document have the flags > > > > the idea here is to satisfy the spec requiring that changes to the sandbox > > flags only take place when the document is next loaded > Ah, ok. Makes sense. > Just add some tests for it, especially for bfcached documents. Most definitely - thanks for the suggestion on using back() - i'm going to add a test like this to the navigation tests there's some other tests along these lines in the navigation tests : // 5) When a link is clicked in a sandboxed iframe, the document navigated to is sandboxed // the same as the original document and is not same origin with parent document // (done by file_iframe_sandbox_d_if6.html which simulates a link click and navigates // to file_iframe_sandbox_d_if7.html which attempts to call back into its parent). // 6) An iframe (if_8) has sandbox="allow-same-origin allow-scripts", the sandboxed document // (file_iframe_sandbox_d_if_8.html) that it contains accesses its parent (this file) and removes // 'allow-same-origin' and then triggers a reload. // The document should not be able to access its parent (this file). // 7) An iframe (if_9) has sandbox="allow-same-origin allow-scripts", the sandboxed document // (file_iframe_sandbox_d_if_9.html) that it contains accesses its parent (this file) and removes // 'allow-scripts' and then triggers a reload. // The document should not be able to run a script and access its parent (this file). are there other cases i should add as well ?

Oh, hmm, we don't actually enable bfcache for frames, at least not by default. browser.sessionhistory.cache_subframes controls it. So, perhaps no need to test that after all.

(In reply to Ian Melven :imelven from comment #144) > While addressing smaug's review comments and working on adding whitespace > tests to the general test suite, I found opening the web console now causes > an assertion and then a crash : > > Assertion failure: principals == > JS_GetCompartmentPrincipals((js::GetContextComp > artment(cx))), at > c:/Users/imelven/src/mozilla/caps/src/nsScriptSecurityManager. > cpp:208 the call stack for this looks like : > xul.dll!PushPrincipalCallback(JSContext * cx, JSPrincipals * principals) Line 208 + 0xa bytes C++ mozjs.dll!AutoEnterCompartmentAndPushPrincipal::enter(JSContext * cx, JSObject * target) Line 523 + 0x19 bytes C++ mozjs.dll!JSStructuredCloneWriter::startWrite(const JS::Value & v) Line 566 + 0x15 bytes C++ mozjs.dll!JSStructuredCloneWriter::write(const JS::Value & v) Line 603 + 0xf bytes C++ mozjs.dll!js::WriteStructuredClone(JSContext * cx, const JS::Value & v, unsigned __int64 * * bufp, unsigned int * nbytesp, const JSStructuredCloneCallbacks * cb, void * cbClosure) Line 69 + 0x21 bytes C++ mozjs.dll!JS_WriteStructuredClone(JSContext * cx, JS::Value v, unsigned __int64 * * bufp, unsigned int * nbytesp, const JSStructuredCloneCallbacks * optionalCallbacks, void * closure) Line 5978 + 0x1d bytes C++ mozjs.dll!JSAutoStructuredCloneBuffer::write(JSContext * cx, JS::Value v, const JSStructuredCloneCallbacks * optionalCallbacks, void * closure) Line 6065 + 0x24 bytes C++ xul.dll!nsGlobalWindow::PostMessageMoz(const JS::Value & aMessage, const nsAString_internal & aOrigin, JSContext * aCx) Line 6208 + 0x23 bytes C++ xul.dll!nsGlobalWindow::PostMessageMoz(const JS::Value & aMessage, const nsAString_internal & aOrigin, JSContext * aCx) Line 6116 + 0x6e bytes C++ xul.dll!NS_InvokeByIndex_P(nsISupports * that, unsigned int methodIndex, unsigned int paramCount, nsXPTCVariant * params) Line 103 C++ principals 0x066058fc {refcount=35 debugToken=200546144 } JSPrincipals * compartment->principals principals 0x085a08b4 {refcount=124 debugToken=200546144 } JSPrincipals * i'm looking at bug 728250 to try and learn how to use the debugtokens to get more insight into what's going on here

That's probably just bug 754156.

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #150) > That's probably just bug 754156. Ah, thanks Kyle ! i wasn't sure if it was something introduced by this patch. i'll watch that and bug 754202 as well. right now the crash happens if you try to open the web console, i'll try and use another way to debug the tests i'm adding.

add tests for using the allowed whitespace characters to separate keywords in the value of the sandbox attribute

unbitrot.

Address review comments, see comment 145 for some more details. This version of the patch incorporates all of smaug's suggestions from comment 142.

make the changes to tests suggested by Mounir

Comment on attachment 624220 [details] [diff] [review] iframe sandbox v14 The inheritance tests are broken - cancelling review for now.