341604 - (framesandbox) Implement HTML5 sandbox attribute for IFRAMEs

Status: RESOLVED FIXED

(Core :: Security, enhancement)

Description

[nrlz]

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

Comment 1

[Jesse Ruderman]

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

Comment 2

[Anne (:annevk)]

Triggering it with javascript would make it more standards compliant? :-O

FWIW, there is no such feature anywhere (yet).

Comment 3

[Hixie (not reading bugmail)]

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

Comment 5

[Ian Macfarlane]

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.

Comment 6

[Gervase Markham [:gerv]]

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

Gerv

Comment 8

[Daniel Veditz [:dveditz]]

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?

Comment 9

[Daniel Veditz [:dveditz]]

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.

Comment 11

[Jean-Yves Perrier [:teoli]]

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?

Comment 12

[Adam Barth]

> 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.

Comment 13

[Artur Adib]

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?

Comment 14

[Leman Bennett [Omega]]

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

Comment 15

[Xavier]

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 ?

Comment 16

[Ian Melven :imelven]

(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.

Comment 17

[Ian Melven :imelven]

Attachment: WIP - experimental prototype v1

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.

Comment 18

[Ian Melven :imelven]

Attachment: WIP - experimental prototype

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

Comment 19

[Ian Melven :imelven]

Attachment: WIP - experimental prototype

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 :)

Comment 20

[Ian Melven :imelven]

Attachment: WIP - experimental prototype

a possible implementation for 'allow-forms'

still working on 'allow-top-navigation' - and there are still many details to iron out after that

Comment 21

[Ian Melven :imelven]

Comment on attachment 575617 [details] [diff] [review]
WIP - experimental prototype

this patch passes the browserscope.org iframe sandbox check fwiw :)

Comment 22

[Ian Melven :imelven]

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.

Comment 23

[Ian Melven :imelven]

Attachment: WIP - experimental prototype

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

Comment 24

[Ian Melven :imelven]

Attachment: WIP - experimental prototype

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 25

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 26

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

> 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 27

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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).

Comment 28

[Ian Melven :imelven]

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)

Comment 29

[Ian Melven :imelven]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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.

Comment 30

[Ian Melven :imelven]

(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 ! :)

Comment 31

[Ian Melven :imelven]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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.

Comment 32

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Ian Melven :imelven from comment #29)
> (In reply to Kyle Huey [:khuey] ([e-mail]) 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.

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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.

Comment 34

[[Baboo]]

(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.

Comment 35

[Ian Melven :imelven]

Attachment: WIP - reworked prototype

- 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.

Comment 36

[Ian Melven :imelven]

Attachment: WIP - prototype

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)

Comment 37

[Ian Melven :imelven]

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.

Comment 38

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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?

Comment 39

[Ian Melven :imelven]

(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.

Comment 40

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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.

Comment 41

[Ian Melven :imelven]

(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.

Comment 42

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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.

Comment 43

[Ian Melven :imelven]

(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 :)

Comment 44

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 45

[Ian Melven :imelven]

Attachment: WIP - prototype

- 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.

Comment 46

[Ian Melven :imelven]

(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.

Comment 47

[Ian Melven :imelven]

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>.

Comment 48

[Adam Barth]

> so I'm not planning on supporting sandbox on <frame>.

That matches the WebKit implementation.

Comment 49

[Ian Melven :imelven]

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.

Comment 50

[Ian Melven :imelven]

(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.

Comment 51

[Daira Hopwood]

(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?

Comment 52

[Ian Melven :imelven]

(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 53

[Ian Melven :imelven]

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 !

Comment 54

[Ian Melven :imelven]

Attachment: WIP - prototype

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

Comment 55

[Boris Zbarsky [:bzbarsky]]

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.

Comment 56

[Devdatta Akhawe [:devd]]

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.

Comment 57

[Boris Zbarsky [:bzbarsky]]

srcdoc is completely unrelated to sandbox.

Comment 58

[Devdatta Akhawe [:devd]]

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?

Comment 59

[Devdatta Akhawe [:devd]]

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

Comment 60

[Boris Zbarsky [:bzbarsky]]

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.

Comment 61

[Ian Melven :imelven]

(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.

Comment 62

[Ian Melven :imelven]

(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 :)

Comment 63

[Ian Melven :imelven]

Attachment: WIP - iframe sandbox v1

(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)

Comment 64

[Boris Zbarsky [:bzbarsky]]

> 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?

Comment 65

[Ian Melven :imelven]

(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.

Comment 66

[Ian Melven :imelven]

Attachment: WIP - iframe sandbox v2

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.

Comment 67

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 68

[Ian Melven :imelven]

(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' :)

Comment 69

[Ian Melven :imelven]

(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.

Comment 70

[Ian Melven :imelven]

Attachment: WIP - iframe sandbox v3

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.

Comment 71

[Ian Melven :imelven]

Attachment: WIP - iframe sandbox v4

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.

Comment 72

[Ian Melven :imelven]

Attachment: WIP - iframe sandbox v5

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)

Comment 73

[Ian Melven :imelven]

Attachment: iframe sandbox tests - inheritance

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.

Comment 74

[Ian Melven :imelven]

Attachment: iframe sandbox tests - same origin

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 !

Comment 75

[Ian Melven :imelven]

Attachment: iframe sandbox tests - general

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 !

Comment 76

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

> ###!!! ASSERTION: Adding a child where we already have a child? This may
> misbeha
> ve: 'Error', file

I hope you're not hitting bug 645229.

Comment 77

[Ian Melven :imelven]

Attachment: iframe sandbox tests - navigation

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.

Comment 78

[Ian Melven :imelven]

Attachment: iframe sandbox tests - navigation v2

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

Comment 79

[Ian Melven :imelven]

Attachment: iframe sandbox tests - plugins

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 !

Comment 80

[Ian Melven :imelven]

Attachment: WIP - iframe sandbox v6

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.

Comment 81

[Ian Melven :imelven]

(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 :)

Comment 82

[Ian Melven :imelven]

Attachment: iframe sandbox tests - navigation v3

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.

Comment 83

[Ian Melven :imelven]

Attachment: WIP - iframe sandbox v7

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.

Comment 84

[Ian Melven :imelven]

Attachment: iframe sandbox tests - inheritance v2

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.

Comment 85

[Ian Melven :imelven]

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 :)

Comment 86

[Ian Melven :imelven]

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.

Comment 87

[Devdatta Akhawe [:devd]]

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

Comment 88

[Ian Melven :imelven]

(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.

Comment 89

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 90

[Ian Melven :imelven]

(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.

Comment 91

[Devdatta Akhawe [:devd]]

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 ?

Comment 92

[Ian Melven :imelven]

Attachment: iframe sandbox tests - navigation v4

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

Comment 93

[Ian Melven :imelven]

(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.

Comment 94

[Ian Melven :imelven]

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)

Comment 95

[Ian Melven :imelven]

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).

Comment 96

[Ian Melven :imelven]

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.

Comment 97

[Ian Melven :imelven]

Attachment: WIP - iframe sandbox v8

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

Comment 98

[Ian Melven :imelven]

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

Comment 99

[Boris Zbarsky [:bzbarsky]]

Which particular sandbox flags?

Comment 100

[Ian Melven :imelven]

(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

Comment 101

[Boris Zbarsky [:bzbarsky]]

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

Comment 102

[Ian Melven :imelven]

(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 :)

Comment 103

[Ian Melven :imelven]

Attachment: iframe sandbox tests - same origin v2

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

Comment 104

[Ian Melven :imelven]

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

Comment 105

[Ian Melven :imelven]

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'.

Comment 106

[Ian Melven :imelven]

Attachment: 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>.

Comment 107

[Ian Melven :imelven]

(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.

Comment 108

[Ian Melven :imelven]

Attachment: WIP - iframe sandbox v8 - more granular script disabling

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.

Comment 109

[Mardeg]

(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?

Comment 110

[Ian Melven :imelven]

(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.

Comment 111

[Ian Melven :imelven]

Attachment: WIP - iframe sandbox v9

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.

Comment 112

[Ian Melven :imelven]

Attachment: iframe sandbox tests - general v3

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.

Comment 113

[Ian Melven :imelven]

Attachment: WIP - iframe sandbox v10

unbitrot.

Comment 114

[Ian Melven :imelven]

Attachment: workaround for workers needing a string origin

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. :)

Comment 115

[Ian Melven :imelven]

Attachment: CheckMayLoad changes to allow sandboxed workers to load blob and data URIs

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.

Comment 116

[Ian Melven :imelven]

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.

Comment 117

[Ian Melven :imelven]

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

Comment 118

[Ian Melven :imelven]

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 :)

Comment 119

[Ian Melven :imelven]

Attachment: WIP - iframe sandbox v11

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

Comment 120

[Ian Melven :imelven]

Attachment: iframe sandbox tests - general v4

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

Comment 121

[Ian Melven :imelven]

Attachment: iframe sandbox test - workers

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)

Comment 122

[Ian Melven :imelven]

Attachment: iframe sandbox v12

Unbitrot.

Comment 123

[Ian Melven :imelven]

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 :)

Comment 124

[Ian Melven :imelven]

Attachment: iframe sandbox tests - general v5

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.

Comment 125

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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 126

[Ian Melven :imelven]

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

Comment 127

[Ian Melven :imelven]

Attachment: iframe sandbox tests - inheritance v3

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

Comment 128

[Ian Melven :imelven]

Attachment: iframe sandbox tests - same origin v3

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

Comment 129

[Ian Melven :imelven]

Attachment: iframe sandbox tests - navigation v5

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

Comment 130

[Ian Melven :imelven]

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 131

[Johnny Stenback (:jst)]

Comment on attachment 616649 [details] [diff] [review]
iframe sandbox tests - general v5

These tests look good to me!

Comment 132

[Mounir Lamouri (:mounir)]

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 133

[Ian Melven :imelven]

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 134

[Ian Melven :imelven]

Comment on attachment 616405 [details] [diff] [review]
iframe sandbox v12

Talked to jst, r?'ing to smaug

Comment 135

[Ian Melven :imelven]

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 136

[Johnny Stenback (:jst)]

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 137

[Johnny Stenback (:jst)]

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.

Comment 138

[Ian Melven :imelven]

Attachment: iframe sandbox tests - workers v2

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

Comment 139

[Ian Melven :imelven]

Attachment: iframe sandbox v13

unbitrot

Comment 140

[Ian Melven :imelven]

Attachment: CheckMayLoad changes to allow sandboxed workers to load blob and data URIs v2

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 !

Comment 141

[Ian Melven :imelven]

Attachment: iframe sandbox tests - general v6

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 142

[Olli Pettay [:smaug][[e-mail]]]

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 143

[Ian Melven :imelven]

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.

Comment 144

[Ian Melven :imelven]

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

Comment 145

[Ian Melven :imelven]

(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 :)

Comment 146

[Olli Pettay [:smaug][[e-mail]]]

(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.

Comment 147

[Ian Melven :imelven]

(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 ?

Comment 148

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 149

[Ian Melven :imelven]

(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

Comment 150

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

That's probably just bug 754156.

Comment 151

[Ian Melven :imelven]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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.

Comment 152

[Ian Melven :imelven]

Attachment: iframe sandbox tests - general v7

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

Comment 153

[Ian Melven :imelven]

Attachment: CheckMayLoad changes to allow sandboxed workers to load blob and data URIs v3

unbitrot.

Comment 154

[Ian Melven :imelven]

Attachment: iframe sandbox v14

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

Comment 155

[Ian Melven :imelven]

Attachment: iframe sandbox tests - same origin v4

make the changes to tests suggested by Mounir

Comment 156

[Ian Melven :imelven]

Comment on attachment 624220 [details] [diff] [review]
iframe sandbox v14

The inheritance tests are broken - cancelling review for now.

Comment 157

[Ian Melven :imelven]

Attachment: iframe sandbox tests - plugins v2

Make the changes suggested by Mounir

Comment 158

[Ian Melven :imelven]

Attachment: iframe sandbox tests - inheritance v4

Make the changes suggested by Mounir to the inheritance tests.

Comment 159

[Ian Melven :imelven]

Attachment: iframe sandbox v15

Fix the problem with the inheritance tests. Still working on a test for .back (even though smaug said bfcache isn't turned on for subframes, i think it's good to have a test for this) and another test suggested by dveditz that an iframe without 'allow-same-origin' when navigated, gets a new, different null principal than it had before the navigation.

Comment 160

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 625329 [details] [diff] [review]
iframe sandbox v15


>+nsContentUtils::ParseSandboxAttributeToFlags(const nsAString& aSandboxAttrValue)
>+{
>+  // If there's a sandbox attribute at all (and there is if this is being
>+  // called), start off by setting all the restriction flags.
>+  PRUint32 out = SANDBOXED_NAVIGATION |
>+    SANDBOXED_TOPLEVEL_NAVIGATION |
>+    SANDBOXED_PLUGINS | SANDBOXED_ORIGIN |
>+    SANDBOXED_FORMS | SANDBOXED_SCRIPTS |
>+    SANDBOXED_AUTOMATIC_FEATURES;
Nit, would be easier to read this if all the values were aligned.
  PRUint32 out = SANDBOXED_NAVIGATION |
                 SANDBOXED_TOPLEVEL_NAVIGATION |
                 SANDBOXED_PLUGINS |
                 SANDBOXED_ORIGIN |
                 SANDBOXED_FORMS |
                 SANDBOXED_SCRIPTS |
                 SANDBOXED_AUTOMATIC_FEATURES;


>   bool inherit;
>   // We expect URIInheritsSecurityContext to return success for an
>   // about:blank URI, so don't call NS_IsAboutBlank() if this call fails.
>   // This condition needs to match the one in nsDocShell::InternalLoad where
>   // we're checking for things that will use the owner.
>-  if (NS_SUCCEEDED(URIInheritsSecurityContext(aURI, &inherit)) &&
>-      (inherit || (aSetUpForAboutBlank && NS_IsAboutBlank(aURI)))) {
>+  if (aForceOwner || ((NS_SUCCEEDED(URIInheritsSecurityContext(aURI, &inherit)) &&
>+      (inherit || (aSetUpForAboutBlank && NS_IsAboutBlank(aURI)))))) {
>+    // Be certain that aForceOwner is only set for null principals
>+    if (aForceOwner) {
>+      nsCOMPtr<nsIURI> ownerURI;
>+      nsresult rv = aLoadingPrincipal->GetURI(getter_AddRefs(ownerURI));
>+      MOZ_ASSERT(NS_SUCCEEDED(rv) && SchemeIs(ownerURI, NS_NULLPRINCIPAL_SCHEME));
>+    }
The if (aForceOwner) is for the assert only, right? Then it should be inside #ifdef DEBUG


>+  if (iframe) {
>+    sandboxFlags = iframe->GetSandboxFlags();
>+
>+    // The OwnerDoc of an iframe node is the parent document of that <iframe>.
Useless comment.

>+    PRUint32 parentSandboxFlags = iframe->OwnerDoc() ?
OwnerDoc() is never null, otherwise the method would be called GetOwnerDoc()

>+nsHTMLIFrameElement::AfterSetAttr(PRInt32 aNameSpaceID, nsIAtom* aName,
>+                                    const nsAttrValue* aValue,
>+                                    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.
>+    if (mFrameLoader) {
>+      nsCOMPtr<nsIDocShell> docshell = mFrameLoader->GetExistingDocShell();
>+
>+      if (docshell) {
>+        PRUint32 newFlags = 0;
>+        // If a NULL aValue is passed in, we want to clear the sandbox flags
>+        // which we will do by setting them to 0.
>+        if (aValue) {
>+          nsAutoString strValue;
>+          aValue->ToString(strValue);
>+          newFlags = nsContentUtils::ParseSandboxAttributeToFlags(
>+            strValue);
>+        }
>+        docshell->SetSandboxFlags(newFlags);
Add some comment why you don't need to disable plugins here
(you do disabled plugins in nsFrameLoader::ReallyStartLoadingInternal).

>+      }
>+    }
>+  }
>+  return nsGenericHTMLElement::AfterSetAttr(aNameSpaceID, aName, aValue,
>+                                            aNotify);
>+
I hope you have plenty of tests for cases when sandbox attribute is changed dynamically and then
some new page is loaded to iframe.

>+    // Sandboxed document check: javascript: URI's are disabled
>+    // in a sandboxed document unless 'allow-scripts' was specified
>+
>+    // REVIEW - it seems like the original inner window would be the one
>+    // that will execute the javascript with its principal - is this
>+    // assumption correct ?? (and is GetExtantDocument() the right way
>+    // to get the right document ??
>+    nsCOMPtr<nsIDocument> doc = do_QueryInterface(aOriginalInnerWindow->GetExtantDocument());
>+    if (doc && (doc->SandboxFlags() & SANDBOXED_SCRIPTS)) {
>+      return NS_ERROR_DOM_RETVAL_UNDEFINED;
>+    }
There is the inner window check below. So based on that the check looks right.
Though, is it possible to get to this place without a document.
If possible, you should check the sandbox flags of docshell.

...which reminds me, does the patch handle about:blank document properly?
Those documents which are created in nsDocShell::CreateAboutBlankContentViewer


No major problems, but I could take another look, so, new patch please.
This probably requires a review from someone else too.

Comment 161

[Ian Melven :imelven]

(In reply to Olli Pettay [:smaug] from comment #160)
> Comment on attachment 625329 [details] [diff] [review]
> iframe sandbox v15

Thanks for the review ! 

i'll clean up the stuff you pointed out - i'll also test about:blank and maybe add some tests around that. I do have quite a few tests around changing the attribute and then reloading the frame or navigating it elsewhere (including the .back test which I added :) )

I also wanted to call out that this patch experiences some asserts/crashes if built without  the patches for bug 754202, i've seen the assert mentioned in 144 a couple times and it usually is followed with a crash - i haven't had time to debug more to understand exactly what the issue is - but in general, i think this patch needs the fix to bug 754202 to land before landing. I would love to understand better why those patches fix the issue and what exactly is going on, I will try to dig in if i have some time.

Comment 162

[Ian Melven :imelven]

Attachment: iframe sandbox tests - navigation v6

add two tests :

1) verify that when a new document is loaded into a sandboxed iframe that doesn't have 'allow-same-origin' it gets a new null principal, different from the null principal of the previously loaded document

2) load a document with sandbox='allow-scripts', then navigate
it to a new document, then change the sandbox attribute to 'allow-scripts allow-same-origin', navigate back to the original document using .back and verify it can now access its parent, since it is now same origin with it

Comment 163

[Ian Melven :imelven]

one last quick update : i pushed all the patches with the patches for 754202 to try - everything looked good, except the inheritance tests hung and timed out - i can reproduce locally, need to fix them up again.

Comment 164

[Justin Lebar (not reading bugmail)]

FYI, in bug 742944 I'm also adding an nsHTMLIFrameElement.h.  I may beat you to it.  :)

Comment 165

[Ian Melven :imelven]

(In reply to Justin Lebar [:jlebar] from comment #164)
> FYI, in bug 742944 I'm also adding an nsHTMLIFrameElement.h.  I may beat you
> to it.  :)

Thanks for the heads up !

Comment 166

[Ian Melven :imelven]

Attachment: CheckMayLoad changes to allow sandboxed workers to load blob and data URIs v4

Unbitrot.

Comment 167

[Ian Melven :imelven]

Attachment: iframe sandbox v16

Still need to look into the inheritance test hang and address Smaug's review comments, but unbitrotting so Dev can experiment with CSP sandbox.

Comment 168

[Ian Melven :imelven]

Adding 754202 as a dependency. Right now i'm building with this patch queue : 

imelven@WINDOWZ4LYFE ~/src/mozilla
$ hg qseries
iframe_sandbox
iframe_sandbox_worker_string_origin_workaround
iframe_sandbox_check_may_load
Bug-754202-1.patch
Bug-754202-2.patch
Bug-754202-3.patch
Bug-754202-4.patch
Bug-754202-5.patch
Bug-754202-6.patch
Bug-754202-7.patch
Bug-754202-8.patch

Comment 169

[Ian Melven :imelven]

(In reply to Olli Pettay [:smaug] from comment #160)
> Comment on attachment 625329 [details] [diff] [review]
> iframe sandbox v15

>+nsHTMLIFrameElement::AfterSetAttr(PRInt32 aNameSpaceID, nsIAtom* aName,
>+                                    const nsAttrValue* aValue,
>+                                    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.
>+    if (mFrameLoader) {
>+      nsCOMPtr<nsIDocShell> docshell = mFrameLoader->GetExistingDocShell();
>+
>+      if (docshell) {
>+        PRUint32 newFlags = 0;
>+        // If a NULL aValue is passed in, we want to clear the sandbox flags
>+        // which we will do by setting them to 0.
>+        if (aValue) {
>+          nsAutoString strValue;
>+          aValue->ToString(strValue);
>+          newFlags = nsContentUtils::ParseSandboxAttributeToFlags(
>+            strValue);
>+        }
>+        docshell->SetSandboxFlags(newFlags);
> Add some comment why you don't need to disable plugins here
> (you do disabled plugins in nsFrameLoader::ReallyStartLoadingInternal).

is 'We don't disable plugins here because the change in the sandbox attribute
shouldn't take effect until the document is loaded' an acceptable comment ?
 
i'm not clear if you were suggesting i should possibly disable plugins here - 
maybe that would be something to add to the plugin tests - adding a sandbox
attribute to an iframe that formerly didn't have one and making sure that plugins
are disabled on reload. i can at least test it manually.

also you mentioned having someone else do a review - can you suggest someone who would be suitable please ? :)

Comment 170

[Johnny Stenback (:jst)]

Comment on attachment 633207 [details] [diff] [review]
CheckMayLoad changes to allow sandboxed workers to load blob and data URIs v4

This patch looks great. The only risk I see here is the fact that this means we're taking a binary incompatible change to nsIPrincipal, which might cause trouble with binary addons etc, but that alone should IMO not be reason enough not to do this cleanup and pave the way for sandboxed workers to do more stuff.

Comment 171

[Johnny Stenback (:jst)]

Comment on attachment 624214 [details] [diff] [review]
iframe sandbox tests - general v7

- In content/base/src/nsContentUtils.cpp:

   if (URIIsLocalFile(aURI) && aLoadingPrincipal &&
-      NS_SUCCEEDED(aLoadingPrincipal->CheckMayLoad(aURI, false)) &&
+      NS_SUCCEEDED(aLoadingPrincipal->CheckMayLoad(aURI, false, false)) &&

This change doesn't belong in this test patch :)

- In content/html/content/test/file_iframe_sandbox_c_if4.html:

+    threw = false;
+    try {
+      window.showModalDialog("about:blank");
+    } catch(error) {
+      threw = true;
+    }

Beware that showModalDialog() is disabled on android and b2g. Probably doesn't really matter here, so just sayin'.

r=jst

Comment 172

[Johnny Stenback (:jst)]

Comment on attachment 624484 [details] [diff] [review]
iframe sandbox tests - same origin v4

+    // should NOT be able to access document.cookie
+    try {
+      var foo = document.cookie;
+    } catch(error) {
+      ok(true, "a document sandboxed without allow-same-origin should NOT be able to access document.cookie");
+    }
+
+    // should NOT be able to access localStorage
+    try {
+      var foo = window.localStorage;
+    } catch(error) {
+      ok(true, "a document sandboxed without allow-same-origin should NOT be able to access localStorage");
+    }
...

This is more a comment on the functionality here rather than this test, but is this per spec? Seems odd that simply touching a property on the scripts own windnow/document throws an exception... I would've expected hiding information, but not throwing on accessing a property that is the base property of a feature. If this matches the spec and other browsers, then so be it, but it seems it'd be more developer friendly if we simply didn't expose any data through localStorage, or document.cookies was an empty string, etc.

Comment 173

[Devdatta Akhawe [:devd]]

@jst: I think the idea is to avoid a data loss: right now, the document being sandboxed doesn't really have a say in being sandboxed.

Comment 174

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Ian Melven :imelven from comment #169)
> > Add some comment why you don't need to disable plugins here
> > (you do disabled plugins in nsFrameLoader::ReallyStartLoadingInternal).
> 
> is 'We don't disable plugins here because the change in the sandbox attribute
> shouldn't take effect until the document is loaded' an acceptable comment ?
"... until the next document is loaded..."
But yes, looks ok

Comment 175

[Ian Melven :imelven]

(In reply to Johnny Stenback (:jst, [e-mail]) from comment #171)
> Comment on attachment 624214 [details] [diff] [review]
> iframe sandbox tests - general v7

Thanks for reviewing the tests !

> This change doesn't belong in this test patch :)

oops, thanks for catching that.
 
> - In content/html/content/test/file_iframe_sandbox_c_if4.html:
> 
> +    threw = false;
> +    try {
> +      window.showModalDialog("about:blank");
> +    } catch(error) {
> +      threw = true;
> +    }
> 
> Beware that showModalDialog() is disabled on android and b2g. Probably
> doesn't really matter here, so just sayin'.

thanks for the heads up here, as well.

Comment 176

[Ian Melven :imelven]

(In reply to Johnny Stenback (:jst, [e-mail]) from comment #172)
> [code that tests that accessing document.cookie in a sandboxed document throws an exception removed for brevity] 
>
> This is more a comment on the functionality here rather than this test, but
> is this per spec? Seems odd that simply touching a property on the scripts
> own windnow/document throws an exception... I would've expected hiding
> information, but not throwing on accessing a property that is the base
> property of a feature. If this matches the spec and other browsers, then so
> be it, but it seems it'd be more developer friendly if we simply didn't
> expose any data through localStorage, or document.cookies was an empty
> string, etc.

I'll test in a few other browsers. I think I remember that IE does throw an exception in this case. I'm curious now and have previously wondered if this was the correct behavior. 

The spec (http://dev.w3.org/html5/spec/dom.html#sandboxCookies) says : "On getting, if the document is a cookie-free Document object, then the user agent must return the empty string. Otherwise, if the Document's origin is not a scheme/host/port tuple, the user agent must throw a SecurityError exception." which seems like it applies to sandboxed documents, fwiw.

Comment 177

[Ian Melven :imelven]

Attachment: iframe sandbox tests - navigation v7

this patch seems to have been corrupted somehow, i fixed it up. carrying over r+ from jst.

Comment 178

[Ian Melven :imelven]

Attachment: iframe sandbox tests - general v8

remove the .cpp change that snuck into this patch, thanks Johnny !

carrying over the r+

Comment 179

[Daira Hopwood]

(In reply to Ian Melven :imelven from comment #176)
> The spec (http://dev.w3.org/html5/spec/dom.html#sandboxCookies) says : "On
> getting, if the document is a cookie-free Document object, then the user
> agent must return the empty string. Otherwise, if the Document's origin is
> not a scheme/host/port tuple, the user agent must throw a SecurityError
> exception." which seems like it applies to sandboxed documents, fwiw.

Ugh, that's mandating an information leak about whether the document has cookies. Maybe a minor leak, but I don't understand why it should exist: if allow-same-origin is not set, then the clear intent is that no information about cookies should be available.

Comment 180

[Daira Hopwood]

(In reply to David-Sarah Hopwood from comment #179)
> Ugh, that's mandating an information leak about whether the document has
> cookies. Maybe a minor leak, but I don't understand why it should exist: if
> allow-same-origin is not set, then the clear intent is that no information
> about cookies should be available.

Oh, and another reason not to do it that way is that it's a testing hazard for web developers. They test when there are no cookies, it works, then the parent document adds cookies (which has no reason to make any difference), and it breaks because the code in the sandboxed document didn't expect the exception.

Comment 181

[Ian Melven :imelven]

(In reply to David-Sarah Hopwood from comment #180)
> (In reply to David-Sarah Hopwood from comment #179)
> > Ugh, that's mandating an information leak about whether the document has
> > cookies. Maybe a minor leak, but I don't understand why it should exist: if
> > allow-same-origin is not set, then the clear intent is that no information
> > about cookies should be available.
> 
> Oh, and another reason not to do it that way is that it's a testing hazard
> for web developers. They test when there are no cookies, it works, then the
> parent document adds cookies (which has no reason to make any difference),
> and it breaks because the code in the sandboxed document didn't expect the
> exception.

those are good points. the implementation in the patch in this bug throws an exception even when there are no cookies for a site.

Comment 182

[Ian Melven :imelven]

(In reply to Ian Melven :imelven from comment #176)
>
> I'll test in a few other browsers. I think I remember that IE does throw an
> exception in this case. I'm curious now and have previously wondered if this
> was the correct behavior. 

IE 10 and Webkit (Chome) do both throw exceptions (security error) on accessing document.cookie from a sandboxed document, even when the site has not set any cookies.

Comment 183

[Daira Hopwood]

(In reply to Ian Melven :imelven from comment #182)
> IE 10 and Webkit (Chome) do both throw exceptions (security error) on
> accessing document.cookie from a sandboxed document, even when the site has
> not set any cookies.

OK, can someone raise it with WHATWG to get the spec changed for the no-cookie case?

Comment 184

[Ian Melven :imelven]

(In reply to Olli Pettay [:smaug] from comment #160)
> Comment on attachment 625329 [details] [diff] [review]
> iframe sandbox v15
>
> ...which reminds me, does the patch handle about:blank document properly?
> Those documents which are created in
> nsDocShell::CreateAboutBlankContentViewer

I tested manually, and it looks to me like about:blank is correctly sandboxed.

Comment 185

[Ian Melven :imelven]

(In reply to David-Sarah Hopwood from comment #183)
> (In reply to Ian Melven :imelven from comment #182)
> > IE 10 and Webkit (Chome) do both throw exceptions (security error) on
> > accessing document.cookie from a sandboxed document, even when the site has
> > not set any cookies.
> 
> OK, can someone raise it with WHATWG to get the spec changed for the
> no-cookie case?

I posted to the WHATWG list, thanks for bringing this up ! Adam Barth agrees that the existing behavior, which is as you described, makes sense, as do I.

Comment 186

[Ian Melven :imelven]

Attachment: iframe sandbox tests - inheritance v5

Fix the problem with the inheritance tests never finishing - carrying over r+

Comment 187

[Ian Melven :imelven]

Attachment: iframe sandbox v17

I checked manually to make sure plugins behave as expected when adding and removing a sandbox attribute and think I've addressed the previous round of review comments.

Comment 188

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 633735 [details] [diff] [review]
iframe sandbox v17


>+  static PRUint32 ParseSandboxAttributeToFlags(const nsAString&
>+                                                 aSandboxAttr);
Odd indentation. aSandboxAttr should fit in to the previous line.


>+      if (token.LowerCaseEqualsLiteral("allow-scripts")) {
>+        out &= ~SANDBOXED_SCRIPTS;
>+      } else if (token.LowerCaseEqualsLiteral("allow-same-origin")) {
>+        out &= ~SANDBOXED_ORIGIN;
>+      } else if (token.LowerCaseEqualsLiteral("allow-forms")) {
>+        out &= ~SANDBOXED_FORMS;
>+      } else if (token.LowerCaseEqualsLiteral("allow-scripts")) {
>+        // allow-scripts removes both SANDBOXED_SCRIPTS and
>+        // SANDBOXED_AUTOMATIC_FEATURES.
>+        out &= ~SANDBOXED_SCRIPTS;
>+        out &= ~SANDBOXED_AUTOMATIC_FEATURES;
>+      } else if (token.LowerCaseEqualsLiteral("allow-top-navigation")) {
>+        out &= ~SANDBOXED_TOPLEVEL_NAVIGATION;
>+      }
>+    }
>+  }

So why you don't handle allow-popups?
And why you handle allow-scripts twice?
Do you have any tests for not having SANDBOXED_AUTOMATIC_FEATURES?

Comment 189

[Ian Melven :imelven]

allow-popups seems to have made it into the spec - i would like to add it to this initial implementation.

Comment 190

[Ian Melven :imelven]

After a little more thought, i think it makes more sense to focus on landing this initial iframe sandbox support and handling allow-popups in a follow up, i've filed bug 766282 as this follow up. Right now, the current patch for this bug always blocks popups, so this is a 'fail closed' situation - there's just no way to opt in to allowing popups yet. 

As discussed with smaug on irc, there's no automatic features tests yet because automatic features are also intended to be done in a followup, which is bug 752551.

Comment 191

[Ian Melven :imelven]

Attachment: iframe sandbox v18

(In reply to Olli Pettay [:smaug] from comment #188)
> Comment on attachment 633735 [details] [diff] [review]
> iframe sandbox v17
> 
> 
> >+  static PRUint32 ParseSandboxAttributeToFlags(const nsAString&
> >+                                                 aSandboxAttr);
> Odd indentation. aSandboxAttr should fit in to the previous line.

oops, it does, fixed - thanks ! 

> So why you don't handle allow-popups?

filed a followup for allow-popups, see previous comment for details

> And why you handle allow-scripts twice?

oops - thanks for catching this ! fixed in this patch.

> Do you have any tests for not having SANDBOXED_AUTOMATIC_FEATURES?

explained in previous comment

thanks !

Comment 192

[Ian Melven :imelven]

Attachment: workaround for workers needing a string origin v2

unbitrot, carrying over r+

Comment 193

[Ian Melven :imelven]

I was curious just how dependent on 754202 these patches are so i pushed just the iframe sandbox tests to try : https://tbpl.mozilla.org/?tree=Try&rev=b4c4bdc7cb94

the results look pretty good, actually. i'm going to try using a Nightly with iframe sandbox support for a bit to get a better sense of the stability.

Comment 194

[Ian Melven :imelven]

Dev suggested making SetSandboxFlags fail if the flags would be less restrictive than the currently are - with a special case for clearing the flags completely. This seems like a nice little bit of defense in depth, I'll look at adding this some time in the near future, could always be done as a quick followup too.

Comment 195

[Devdatta Akhawe [:devd]]

Further, a 'IntersectSandboxFlags' function that implements the intersection algorithm that you implemented for nested sandboxes, which I can then use for CSP Sandbox.

Comment 196

[Ian Melven :imelven]

(In reply to Devdatta Akhawe from comment #195)
> Further, a 'IntersectSandboxFlags' function that implements the intersection
> algorithm that you implemented for nested sandboxes, which I can then use
> for CSP Sandbox.

that's just a OR between the parent and the child's flags, since the flags always add restrictions, see the code in nsFrameLoader in the patch

Comment 197

[Ian Melven :imelven]

Looking at the try results again I got "15217 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_iframe_sandbox_plugins.html | an unexpected uncaught JS exception reported through window.onerror - TypeError: p.startWatchingInstanceCount is not a function at http://mochi.test:8888/tests/content/html/content/test/test_iframe_sandbox_plugins.html:31" on Android opt.

Comment 198

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 634584 [details] [diff] [review]
iframe sandbox v18

>+
>+      // Set up the actual sandboxing for plugins as specified.
>+      if (sandboxFlags & SANDBOXED_PLUGINS) {
>+        mDocShell->SetAllowPlugins(false);
>+      }
>+    }
>+  }
So it is never possible to enable plugins if they are once disabled for an iframe?


> 
>+  // If this document is sandboxed without 'allow-scripts', abort.
>+  if (mDocument->SandboxFlags() & SANDBOXED_SCRIPTS) {
>+    // REVIEW : this probably isn't the right error..
>+    return NS_ERROR_DOM_SECURITY_ERR;
>+  }
Do we want to return error at all? Would NS_OK work - just don't execute or load the script?

Comment 199

[Devdatta Akhawe [:devd]]

I think throwing a security error is useful. Say I wanted to allow scripts but had a typo and ended up saying allw-scripts. It is useful to debug that during testing if the console shows me a security error "refused to execute scripts"

Comment 200

[Ian Melven :imelven]

(In reply to Olli Pettay [:smaug] from comment #198)
> Comment on attachment 634584 [details] [diff] [review]
> iframe sandbox v18
> 
> >+
> >+      // Set up the actual sandboxing for plugins as specified.
> >+      if (sandboxFlags & SANDBOXED_PLUGINS) {
> >+        mDocShell->SetAllowPlugins(false);
> >+      }
> >+    }
> >+  }
> So it is never possible to enable plugins if they are once disabled for an
> iframe?

as discussed on IRC, if the sandbox attribute is removed from the iframe, plugins should be re-enabled for the next document loaded - reading back through the comments, i realized i tested the other case (adding a sandbox attribute and reloading blocks plugins) but not this case. I'll fix this case and see if I can add a test to the mochitests for it as well. 
 
> > 
> >+  // If this document is sandboxed without 'allow-scripts', abort.
> >+  if (mDocument->SandboxFlags() & SANDBOXED_SCRIPTS) {
> >+    // REVIEW : this probably isn't the right error..
> >+    return NS_ERROR_DOM_SECURITY_ERR;
> >+  }
> Do we want to return error at all? Would NS_OK work - just don't execute or
> load the script?

The closest piece of the spec I can find says :

"If scripting is disabled for browsing context passed to this algorithm, then abort these steps, as if the script did nothing but return void."

which implies to me there should not be an error. It's probably a good idea for me to check what the other browsers do in this case as well.

Comment 201

[Ian Melven :imelven]

(In reply to Ian Melven :imelven from comment #200)
> > > 
> > >+  // If this document is sandboxed without 'allow-scripts', abort.
> > >+  if (mDocument->SandboxFlags() & SANDBOXED_SCRIPTS) {
> > >+    // REVIEW : this probably isn't the right error..
> > >+    return NS_ERROR_DOM_SECURITY_ERR;
> > >+  }
> > Do we want to return error at all? Would NS_OK work - just don't execute or
> > load the script?
> 
> The closest piece of the spec I can find says :
> 
> "If scripting is disabled for browsing context passed to this algorithm,
> then abort these steps, as if the script did nothing but return void."
> 
> which implies to me there should not be an error. It's probably a good idea
> for me to check what the other browsers do in this case as well.

I verified that Chrome and IE 10 don't throw an error in this case, i'll change the patch to return NS_OK as suggested when i clean up the plugin stuff. Thanks for the suggestion here, smaug !

Comment 202

[Daira Hopwood]

It seems to me that the spec (and other browsers) saying not to throw a visible error, does not imply that there can't or shouldn't be a warning on the console. Warnings are purely a quality of implementation issue, and I agree with Devdatta Akhawe in comment:199 that it might help debugging.

Comment 203

[Ian Melven :imelven]

(In reply to David-Sarah Hopwood from comment #202)
> It seems to me that the spec (and other browsers) saying not to throw a
> visible error, does not imply that there can't or shouldn't be a warning on
> the console. Warnings are purely a quality of implementation issue, and I
> agree with Devdatta Akhawe in comment:199 that it might help debugging.

That's a reasonable point, i suggest this would be best as a followup, please feel free to file a bug for the warning, dependent on this bug. There's already another followup bug to add logging when an iframe's sandbox attribute is such that the sandbox could be removed, see bug 752559.

Comment 204

[Daira Hopwood]

(In reply to Ian Melven :imelven from comment #203)
> That's a reasonable point, i suggest this would be best as a followup,
> please feel free to file a bug for the warning, dependent on this bug.

Filed as bug 768664.

Comment 205

[Ian Melven :imelven]

(In reply to David-Sarah Hopwood from comment #204)
> (In reply to Ian Melven :imelven from comment #203)
> > That's a reasonable point, i suggest this would be best as a followup,
> > please feel free to file a bug for the warning, dependent on this bug.
> 
> Filed as bug 768664.

Thank you for filing ! I like that the bug is quite general - and it fits in nicely with our efforts to do more logging around CSP and mixed content issues in the console as well.

Comment 206

[Ian Melven :imelven]

(In reply to Ian Melven :imelven from comment #197)
> Looking at the try results again I got "15217 ERROR TEST-UNEXPECTED-FAIL |
> /tests/content/html/content/test/test_iframe_sandbox_plugins.html | an
> unexpected uncaught JS exception reported through window.onerror -
> TypeError: p.startWatchingInstanceCount is not a function at
> http://mochi.test:8888/tests/content/html/content/test/
> test_iframe_sandbox_plugins.html:31" on Android opt.

i spoke to jmaher on irc and it seems that plugin tests should be skipped on Android. i'll take care of that in the updated plugin test patch i'm working on.

Comment 207

[Ian Melven :imelven]

Attachment: workaround for workers needing a string origin v3

Unbitrot, carrying over r+

Comment 208

[Ian Melven :imelven]

Attachment: iframe sandbox v19

* Add a flag to docshell to track if a sandbox attribute was cleared from a document - if so, plugins need to be re-enabled for the next document loaded in the docshell. I'm open to other ways of doing this, so please let me know if another approach is preferred.

* Change the script blocking to return NS_OK as discussed in comment 200 and comment 201

Comment 209

[Ian Melven :imelven]

Attachment: iframe sandbox tests - plugins v3

Add these tests :

* test that when a plugin is loaded in an unsandboxed iframe, a sandbox attribute is then added to the iframe and the document containing the plugin is reloaded, the plugin does not load in the sandboxed iframe 

* test that when when a sandboxed iframe's sandbox attribute is removed,
and a new document is loaded into the iframe, the plugin loads 

I still need to make these tests be skipped on Android, will do that shortly.

Comment 210

[Ian Melven :imelven]

Attachment: iframe sandbox tests - plugins v4

Skip the plugin tests on Android (thanks for the help jmaher!)

Johnny, not sure if adding the new tests warrants more review so I r?'d to you instead of carrying over the previous r+ :)

Comment 211

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 638541 [details] [diff] [review]
iframe sandbox v19


>+  // 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);
>+
>+    // Additionally, if this docshell had its document's sandbox
>+    // attribute removed prior to this load, we need to re-enable
>+    // plugins for this docshell.
>+    bool sandboxCleared;
>+    if (NS_SUCCEEDED(docShell->GetSandboxCleared(&sandboxCleared)) &&
>+         sandboxCleared) {
>+      docShell->SetAllowPlugins(true);
>+      docShell->SetSandboxCleared(false);
>+    } 
>+  }
This doesn't work if you remove sandbox attribute and add it then back and then load a new document.



>+    // Sandboxed document check: javascript: URI's are disabled
>+    // in a sandboxed document unless 'allow-scripts' was specified.
>+    nsCOMPtr<nsIDocument> doc = do_QueryInterface(aOriginalInnerWindow->GetExtantDocument());
No need for nsCOMPtr if you use aOriginalInnerWindow->GetExtantDoc()

Comment 212

[Ian Melven :imelven]

(In reply to Olli Pettay [:smaug] from comment #211)
> Comment on attachment 638541 [details] [diff] [review]
> iframe sandbox v19
> 
> 
> >+  // 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);
> >+
> >+    // Additionally, if this docshell had its document's sandbox
> >+    // attribute removed prior to this load, we need to re-enable
> >+    // plugins for this docshell.
> >+    bool sandboxCleared;
> >+    if (NS_SUCCEEDED(docShell->GetSandboxCleared(&sandboxCleared)) &&
> >+         sandboxCleared) {
> >+      docShell->SetAllowPlugins(true);
> >+      docShell->SetSandboxCleared(false);
> >+    } 
> >+  }
> This doesn't work if you remove sandbox attribute and add it then back and
> then load a new document.

Gah, thanks for catching that.. I'll fix that. Seems like I can handle that case by doing a SetSandboxCleared(false) in AfterSetAttr if the attribute is being added. 

> >+    // Sandboxed document check: javascript: URI's are disabled
> >+    // in a sandboxed document unless 'allow-scripts' was specified.
> >+    nsCOMPtr<nsIDocument> doc = do_QueryInterface(aOriginalInnerWindow->GetExtantDocument());
> No need for nsCOMPtr if you use aOriginalInnerWindow->GetExtantDoc()

cool, i'll fix that up too.

Thanks for your ongoing reviews !

Comment 213

[Devdatta Akhawe [:devd]]

In bug 671389, we apply the sandbox flags to the document and not the docshell (since in CSP, the sandbox is only for the document). Relying on docshell flags for enforcing the "disallow plugins" requirement, won't work for CSP sandbox. Does anyone have a suggestion for how to fix this concern?

Comment 214

[Ian Melven :imelven]

Attachment: iframe sandbox v20

This fixes the issue discussed in comment 211 and 212 involving adding and removing the sandbox attribute and correctly enabling and disabling plugins. I tested adding and removing the attribute in various combinations and then navigating the sandbox document. 

I did a try run earlier this week and there were problems with some of the tests timing out, I'm looking into that now.

Comment 215

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 639552 [details] [diff] [review]
iframe sandbox v20


>+  // 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);
>+
>+    // Additionally, if this docshell had its document's sandbox
>+    // attribute removed prior to this load, we need to re-enable
>+    // plugins for this docshell.
>+    bool sandboxCleared;
>+    if (NS_SUCCEEDED(docShell->GetSandboxCleared(&sandboxCleared)) &&
>+         sandboxCleared) {
>+      docShell->SetAllowPlugins(true);
>+      docShell->SetSandboxCleared(false);

This looks wrong. What if user has explicitly disabled all the plugins?
Also, I don't understand how the sandboxCleared handles the case when sandbox attribute
isn't removed but just changed.

Comment 216

[Ian Melven :imelven]

(In reply to Olli Pettay [:smaug] from comment #215)
> Comment on attachment 639552 [details] [diff] [review]
> iframe sandbox v20
> 
> 
> >+  // 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);
> >+
> >+    // Additionally, if this docshell had its document's sandbox
> >+    // attribute removed prior to this load, we need to re-enable
> >+    // plugins for this docshell.
> >+    bool sandboxCleared;
> >+    if (NS_SUCCEEDED(docShell->GetSandboxCleared(&sandboxCleared)) &&
> >+         sandboxCleared) {
> >+      docShell->SetAllowPlugins(true);
> >+      docShell->SetSandboxCleared(false);
> 
> This looks wrong. What if user has explicitly disabled all the plugins?

I'm not sure how to handle the 'plugins are explicitly disabled' case. I thought about setting another flag if the sandbox disabled plugins but this doesn't fix the problem. I suppose I could check the pref (i assume there is one) but that seems a little heavyweight? I'll look to see if there's other ways I can tell the user has explicitly disabled plugins.

> Also, I don't understand how the sandboxCleared handles the case when
> sandbox attribute
> isn't removed but just changed.

In this case I think plugins should already be disabled from when the sandbox attribute was first added and sandboxCleared doesn't need to do anything, or am I missing something ?

Comment 217

[Olli Pettay [:smaug][[e-mail]]]

But what if the new value for the sandbox should enable plugins?

Comment 218

[Ian Melven :imelven]

(In reply to Olli Pettay [:smaug] from comment #217)
> But what if the new value for the sandbox should enable plugins?

plugins are always disabled in a sandboxed document, per the spec (unless the plugin is 'securable' which means it understands and obeys any sandboxing restrictions on the document embedding it - afaik no plugin yet does this).

Comment 219

[Ian Melven :imelven]

(In reply to Ian Melven :imelven from comment #216)
> 
> I'm not sure how to handle the 'plugins are explicitly disabled' case. I
> thought about setting another flag if the sandbox disabled plugins but this
> doesn't fix the problem. I suppose I could check the pref (i assume there is
> one) but that seems a little heavyweight? I'll look to see if there's other
> ways I can tell the user has explicitly disabled plugins.

keeler pointed me to some code in nsPluginHost that checks the pref plugin.disable

http://mxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsPluginHost.cpp#318

318   mPluginsDisabled = Preferences::GetBool("plugin.disable", false);

I need to look into the plugin code more to see where the docshell flag is checked to see if there's an issue here, it may be that the pref overrides whether plugins are enabled on the docshell or not (it should, since they will be in the usual, non-sandboxed case).

Comment 220

[Boris Zbarsky [:bzbarsky]]

Why do we need this sandboxcleared stuff at all?

I thought the model was @sandbox sets flags on docshell.  Document inherits docshell flags at creation time.  All checks use the document's flags.  Are we actually using SetAllowPlugins on the docshell to disable plugins as far as sandbox is concerned?  If so, that's wrong.  What we should do instead is find the places that GetAllowPlugins and have those ask the _document_ instead.  And the document can GetAllowPlugins(), and if true check its sandbox flags and so forth.

Comment 221

[Boris Zbarsky [:bzbarsky]]

In particular, you probably just need to fix nsWebBrowserContentPolicy.cpp here, I bet.

Comment 222

[Ian Melven :imelven]

(In reply to Boris Zbarsky (:bz) from comment #221)
> In particular, you probably just need to fix nsWebBrowserContentPolicy.cpp
> here, I bet.

ok, this approach makes a lot of sense and is more consistent with the rest of the functionality and the spec as well. It also addresses Dev's concern with disabling plugins via CSP sandbox and follows the approach he discussed with dveditz, of asking the docshell first and then asking the document (really meaning see if it has SANDBOXED_PLUGINS set)

Thanks Boris !!

Comment 223

[Ian Melven :imelven]

Attachment: iframe sandbox v21

I changed the handling of plugins to be as bz described in comment 220.

In this patch all the callers of GetAllowPlugins always ask the document, which first asks the docshell and then checks if its SANDBOXED_PLUGINS flag is set. 

This passes the plugin tests attached in this bug.

Comment 224

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 642024 [details] [diff] [review]
iframe sandbox v21


>         bool isNewWindow = false;
>         if (!targetDocShell) {
>+            // If the docshell's document is sandboxed and was trying to
>+            // navigate/load a frame it wasn't allowed to access, the
>+            // FindItemWithName above will have returned null for the target
>+            // item - we don't want to actually open a new window in this case
>+            // though. Check if we are sandboxed and bail out here if so.
>+            NS_ENSURE_TRUE(mContentViewer, NS_ERROR_FAILURE);
>+            nsIDocument* doc = mContentViewer->GetDocument();
>+            PRUint32 sandboxFlags = 0;
>+            sandboxFlags = doc->SandboxFlags();
Null check doc


>+  nsCOMPtr<nsIContentViewer> cv;
>+  docShell->GetContentViewer(getter_AddRefs(cv));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  NS_ENSURE_TRUE(cv, NS_ERROR_FAILURE);
>+
>+  nsIDocument * doc = cv->GetDocument();
>+  NS_ENSURE_TRUE(doc, NS_ERROR_FAILURE);
>+
>+  rv = doc->GetAllowPlugins(&tmp);
This code is repeated many times. We really need some helper code.
Perhaps add 
[noscript, notxpcom] pluginsAllowedInCurrentDoc(); 
to nsIDocShell.

Then in C++ you could just call
NS_ENSURE_STATE(docshell->PluginsAllowedInCurrentDoc());

Or 
NS_ENSURE_TRUE(docshell->PluginsAllowedInCurrentDoc(), <the error code you want to use>);

Comment 225

[Ian Melven :imelven]

(In reply to Olli Pettay [:smaug] from comment #224)
> Comment on attachment 642024 [details] [diff] [review]
> iframe sandbox v21
>
> >+            nsIDocument* doc = mContentViewer->GetDocument();
> >+            PRUint32 sandboxFlags = 0;
> >+            sandboxFlags = doc->SandboxFlags();
> Null check doc

will do.
 
> >+  nsCOMPtr<nsIContentViewer> cv;
> >+  docShell->GetContentViewer(getter_AddRefs(cv));
> >+  NS_ENSURE_SUCCESS(rv, rv);
> >+  NS_ENSURE_TRUE(cv, NS_ERROR_FAILURE);
> >+
> >+  nsIDocument * doc = cv->GetDocument();
> >+  NS_ENSURE_TRUE(doc, NS_ERROR_FAILURE);
> >+
> >+  rv = doc->GetAllowPlugins(&tmp);
> This code is repeated many times. We really need some helper code.
> Perhaps add 
> [noscript, notxpcom] pluginsAllowedInCurrentDoc(); 
> to nsIDocShell.
> 
> Then in C++ you could just call
> NS_ENSURE_STATE(docshell->PluginsAllowedInCurrentDoc());
> 
> Or 
> NS_ENSURE_TRUE(docshell->PluginsAllowedInCurrentDoc(), <the error code you
> want to use>);

i like that idea a lot, i will make this change.

I am still having problems with the general and navigation tests - they are hanging in a weird way that I need to look into some more.

Before landing, I also need to sync up with dveditz again to make sure the secreview requirements are addressed.

Additionally, in comment 160, Olli suggested having someone else look at the patch - when I make the changes above I will r? to jst and Johnny can either look at it or suggest a good additional reviewer :)

Comment 226

[Johnny Stenback (:jst)]

Comment on attachment 638823 [details] [diff] [review]
iframe sandbox tests - plugins v4

+function doTest() { 
+	// 1) test that a plugin can't be loaded with <embed> inside a sandboxed <iframe>
+	// (done by file_iframe_sandbox_f_if1.html loaded in if1 below)
+	// 2) test that a plugin can't be loaded with <object> inside a sandboxed <iframe>
+	// (done by file_iframe_sandbox_f_if1.html loaded in if1 below)
+	// 3) test that plugin can't be loaded by a sandboxed <iframe> with src pointing to
+	// a document that is handled by a plugin (done by if_2 below)
+  // 4) test that when a plugin is loaded in an unsandboxed iframe, a sandbox attribute
+  // is then added to the iframe and the document containing the plugin is reloaded,
+  // the plugin does not load in the sandboxed iframe (done with if_3 below)
...

Looks like a mix of spaces and tabs used for indentation here, in this whole file. Maybe go through and remove all tabs to be consistent...

r=jst

Comment 227

[Ian Melven :imelven]

Attachment: iframe sandbox tests - plugins v5

Clean up tabs and whitespace in these tests, thanks jst ! Carrying over the r+

Comment 228

[Ian Melven :imelven]

Attachment: iframe sandbox v22

Make the changes described by smaug in comment 224, including creating pluginsAllowedInCurrentDoc(). Carrying over the r+ but please feel free to take another look !

Comment 229

[Ian Melven :imelven]

Comment on attachment 643557 [details] [diff] [review]
iframe sandbox v22

Olli suggested that someone else look at this - jst: could you take a look or suggest a good second reviewer ? Thanks !

Comment 230

[Ian Melven :imelven]

The problem with the general and navigation tests looks to be that the load event isn't firing, so doTest() never gets called. I'm trying to figure out why...

Comment 231

[Olli Pettay [:smaug][[e-mail]]]

What kinds of tests are you writing? Use pageshow which fires for bfcached documents too.

Comment 232

[Ian Melven :imelven]

(In reply to Olli Pettay [:smaug] from comment #231)
> What kinds of tests are you writing? Use pageshow which fires for bfcached
> documents too.

mochitests - the patches attached to this bug. They were working previously, then I updated m-c and they no longer work. What's weird is that the other sets of tests (plugins, same-origin etc) also all use addLoadEvent() and seem to be working fine. I will give pageshow a try, although it seems like the bfcache shouldn't be an issue ?

Comment 233

[Ian Melven :imelven]

(In reply to Ian Melven :imelven from comment #230)
> The problem with the general and navigation tests looks to be that the load
> event isn't firing, so doTest() never gets called. I'm trying to figure out
> why...

ok, onload isn't firing for iframes without 'allow-scripts'

i'll add a test for this after i fix it, as well.

Comment 234

[Ian Melven :imelven]

(In reply to Ian Melven :imelven from comment #233)
> (In reply to Ian Melven :imelven from comment #230)
> > The problem with the general and navigation tests looks to be that the load
> > event isn't firing, so doTest() never gets called. I'm trying to figure out
> > why...
> 
> ok, onload isn't firing for iframes without 'allow-scripts'
> 
> i'll add a test for this after i fix it, as well.

Actually i'm not sure if that's exactly the problem, since i'm not sure if onload not firing for the child frame means the onload for the top level page won't fire. What i'm seeing is that when iframes without allow-scripts are included in the top level page, the top level's page onload doesn't fire, and the onload for the child frame without 'allow-scripts' also doesn't fire.

Comment 235

[Ian Melven :imelven]

Ok, it's this change to nsScriptLoader.cpp from comment 208 that broke my tests :

// If this document is sandboxed without 'allow-scripts', abort.
if (mDocument->SandboxFlags() & SANDBOXED_SCRIPTS) {
-  return NS_ERROR_DOM_SECURITY_ERR;
+  return NS_OK;
}

it seems that if i return NS_OK here, the onload handlers for the iframe with the sandbox attribute and its parent page don't fire.

i need to dig more.

Comment 236

[Ian Melven :imelven]

I debugged nsScriptLoader some more and tried out returning different values from and setting requests as blocking the parser in ProcessScriptElement and have a very slightly betetr idea what's going on but i'm not sure what the right thing to do here is yet.

Comment 237

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 643557 [details] [diff] [review]
iframe sandbox v22

Review of attachment 643557 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsDocument.h
@@ +566,5 @@
>                                 nsStyleSet* aStyleSet,
>                                 nsIPresShell** aInstancePtrResult);
>    virtual void DeleteShell();
>  
> +  virtual nsresult GetAllowPlugins (bool* aAllowPlugins);

Np space before (

::: content/html/content/src/nsHTMLFormElement.cpp
@@ +656,5 @@
>        return NS_OK;
>      }
> +    // Don't submit if we're in a sandboxed frame and form submit
> +    // is disabled.
> +    return (doc->SandboxFlags() & SANDBOXED_FORMS) ? NS_OK : DoSubmit(aEvent);

I'd prefer

// Don't submit if we're not in a document or if we're in
// a sandboxed frame and form submit is disabled.
if (!doc || (doc->SandboxFlags() & SANDBOXED_FORMS)) {
  return NS_OK;
}
return DoSubmit(aEvent);

@@ +1662,5 @@
> +  // 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 && (doc->SandboxFlags() & SANDBOXED_FORMS)) {
> +      return true;

Two-space indentation

::: content/html/content/src/nsHTMLIFrameElement.cpp
@@ +202,5 @@
>  
> +nsresult
> +nsHTMLIFrameElement::AfterSetAttr(PRInt32 aNameSpaceID, nsIAtom* aName,
> +                                    const nsAttrValue* aValue,
> +                                    bool aNotify)

Indentation is off

::: content/html/content/src/nsHTMLIFrameElement.h
@@ +12,5 @@
> +#include "nsMappedAttributes.h"
> +#include "nsDOMError.h"
> +#include "nsRuleData.h"
> +#include "nsStyleConsts.h"
> +#include "nsContentUtils.h"

I have to say I find it hard to believe that you need all of those in the header.

::: docshell/base/Makefile.in
@@ +89,5 @@
>    -I$(srcdir)/../shistory/src \
>    -I$(topsrcdir)/dom/base \
>    -I$(topsrcdir)/layout/base \
>    -I$(topsrcdir)/xpcom/ds \
> +  -I$(topsrcdir)/content/base/src \

Just put nsSandboxFlags.h in EXPORTS instead.

::: docshell/base/nsDocShell.cpp
@@ +3064,5 @@
> +                        if (tmp && tmp == foundItem) {
> +                            // This is an ancestor, and we are sandboxed.
> +                            // Unless allow-top-navigation is set, we can't allow this.
> +                            if (sandboxFlags & SANDBOXED_TOPLEVEL_NAVIGATION) {
> +                              foundItem = nsnull;

Indentation

@@ +11942,5 @@
> +nsDocShell::PluginsAllowedInCurrentDoc()
> +{
> +  bool pluginsAllowed = false;
> +
> +  if (mContentViewer) {

Early return if mContentViewer or doc is null

@@ +11943,5 @@
> +{
> +  bool pluginsAllowed = false;
> +
> +  if (mContentViewer) {
> +    nsIDocument * doc = mContentViewer->GetDocument();

* to the left

::: dom/base/nsGlobalWindow.cpp
@@ +8230,5 @@
>      }
>  
> +    // If the document has the sandboxed origin flag set
> +    // don't allow access to localStorage.
> +    if (mDoc && (mDoc->SandboxFlags() & SANDBOXED_ORIGIN)) {

Fail open or fail closed when mDoc is null? Ask someone who knows when that happens

::: dom/base/nsPluginArray.cpp
@@ +66,5 @@
>  
>  bool
>  nsPluginArray::AllowPlugins()
>  {
>    bool allowPlugins = false;

No need for this local

::: dom/src/jsurl/Makefile.in
@@ +29,5 @@
>  LOCAL_INCLUDES += \
>  		-I$(srcdir) \
>  		-I$(topsrcdir)/dom/base \
>  		-I$(topsrcdir)/netwerk/base/src \
> +		-I$(topsrcdir)/content/base/src \

Same here

::: dom/src/jsurl/nsJSProtocolHandler.cpp
@@ +194,5 @@
> +    // Sandboxed document check: javascript: URI's are disabled
> +    // in a sandboxed document unless 'allow-scripts' was specified.
> +    nsIDocument* doc = aOriginalInnerWindow->GetExtantDoc();
> +    if (doc && (doc->SandboxFlags() & SANDBOXED_SCRIPTS)) {
> +      return NS_ERROR_DOM_RETVAL_UNDEFINED;

4-space, I think

::: editor/composer/src/nsEditingSession.cpp
@@ +201,2 @@
>  
>    mPluginsEnabled = tmp;

mPluginsEnabled = docShell->PluginsAllowedInCurrentDoc();

::: embedding/browser/webBrowser/nsWebBrowserContentPolicy.cpp
@@ +38,5 @@
>          return NS_OK;
>  
>      nsresult rv;
>      bool allowed = true;
> +    nsCOMPtr<nsIContentViewer> cv;

I have no idea what this is for

@@ +43,4 @@
>  
>      switch (contentType) {
>        case nsIContentPolicy::TYPE_OBJECT:
> +        allowed = shell->PluginsAllowedInCurrentDoc();

You're no initializing rv here, so you're touching bogus memory later in this function.

::: embedding/components/windowwatcher/src/Makefile.in
@@ +29,5 @@
>  FORCE_STATIC_LIB = 1
>  
>  # For nsJSUtils
> +LOCAL_INCLUDES += -I$(topsrcdir)/dom/base \
> +		  -I$(topsrcdir)/content/base/src \

Same

::: embedding/components/windowwatcher/src/nsWindowWatcher.cpp
@@ +586,5 @@
> +      if (doc) {
> +        PRUint32 sandboxFlags = 0;
> +        sandboxFlags = doc->SandboxFlags();
> +
> +        if (sandboxFlags & SANDBOXED_NAVIGATION) {

if (doc && (doc->SandboxFlags() & SANDBOXED_NAVIGATION)) {

Comment 238

[Ian Melven :imelven]

(In reply to :Ms2ger from comment #237)

thanks for the review !

i'll work on cleaning all this up - one thing :

>> +    return (doc->SandboxFlags() & SANDBOXED_FORMS) ? NS_OK : DoSubmit(aEvent);

> I'd prefer
> // Don't submit if we're not in a document or if we're in
> // a sandboxed frame and form submit is disabled.
> if (!doc || (doc->SandboxFlags() & SANDBOXED_FORMS)) {
>   return NS_OK;

smaug asked for this bit to be this way in the review in comment 142. to be honest, personally i prefer the style of your suggestion, i find it easier to read. 

and of course, i still need to figure out what to do with blocking scripts and firing onload correctly as discussed in comment 236.

Comment 239

[Ian Melven :imelven]

Attachment: iframe sandbox v23

* With help from Jonas, fix the onload problems with blocking script mentioned - the general and navigation tests work again

* Address ms2ger's review comments from comment  with help from bz

* Carrying over Olli's r+ but would be happy to have it looked over again if he likes, and r? to jst for review or to find a 2nd reviewer.

Next up : try out the cases dveditz and I discussed during our secreview meeting and see if that uncovers any issues. 

Recent try run (looks quite good) : https://tbpl.mozilla.org/?tree=Try&rev=553e91717ded

Comment 240

[Ian Melven :imelven]

(In reply to Ian Melven :imelven from comment #239)

oops ! should be : 

> * Address ms2ger's review comments from comment 237 with help from bz

Comment 241

[Ian Melven :imelven]

Here is how one of the test cases that dveditz suggested behaves :

<iframe sandbox="" src='javascript:"<html><script>window.alert(1);</script>INNER FRAME FROM JS</html>";'></iframe>

in this case, the frame loads and shows 'INNER FRAME FROM JS' but the alert does not fire, unless 'allow-scripts' is specified for the sandbox attribute. This seems correct to me.

Comment 242

[Ian Melven :imelven]

Another test case from dveditz :

start with <iframe sandbox='allow-same-origin'>
then change this iframe's src via JS to be 'javascript:"<html><script>window.alert(1);</script>INNER FRAME FROM JS</html>";'

this does not work unless the iframe has sandbox='allow-scripts' as well. This does seem a little weird, I was expecting that this would work but the alert would not fire along the lines of comment 241. I'm not too concerned about this, I suppose, but will discuss it with dveditz.

Comment 243

[Boris Zbarsky [:bzbarsky]]

> this does not work unless the iframe has sandbox='allow-scripts' as well. 

I believe this is expected.  Certainly in our implementation, but presumably per spec too.

Comment 244

[Ian Melven :imelven]

(In reply to Boris Zbarsky (:bz) from comment #243)
> > this does not work unless the iframe has sandbox='allow-scripts' as well. 
> 
> I believe this is expected.  Certainly in our implementation, but presumably
> per spec too.

I suppose the difference is the javascript: URI being assigned to src is evaluated in the context of the existing document when there is one whereas for <iframe src="javascript: etc"> there's no existing doc that's been sandboxed yet until the javascript: URI is evaluated ?  That makes some sense to me.

Comment 245

[Boris Zbarsky [:bzbarsky]]

There's an existing doc in the <iframe src="javascript:"> case as well.  I don't see why they aren't behaving identically; they should be.  If they're not, _that_'s a bug.  Imo.

Comment 246

[Ian Melven :imelven]

(In reply to Boris Zbarsky (:bz) from comment #245)
> There's an existing doc in the <iframe src="javascript:"> case as well.  I
> don't see why they aren't behaving identically; they should be.  If they're
> not, _that_'s a bug.  Imo.

ok - that matches my original assumption: both the case with <iframe sandbox="" src="javascript:"> and the case where an existing <iframe sandbox=""> has its src modified by its parent should behave the same. 

Thanks, I'll do some debugging !

Comment 247

[Ian Melven :imelven]

Results of testing another of Dan's ideas : using a data:text/html URI as the source for an iframe with the sandbox attribute. This works as I would expect with embedding script tags in the data URI, at least - they do not execute unless the sandbox attribute specifies 'allow-scripts'.

Comment 248

[Ian Melven :imelven]

(In reply to Ian Melven :imelven from comment #247)
> Results of testing another of Dan's ideas : using a data:text/html URI as
> the source for an iframe with the sandbox attribute. This works as I would
> expect with embedding script tags in the data URI, at least - they do not
> execute unless the sandbox attribute specifies 'allow-scripts'.

A note to myself to make sure origins and 'allow-same-origin' work as it seems they should in these cases as well - ie for javascript: data: and blob URI's as the src of a sandboxed iframe.

Comment 249

[Ian Melven :imelven]

Blob URIs work as I would expect - if you use a Blob URI as the src for an iframe with a sandbox attribute, scripts embedded in the blob do not execute without 'allow-scripts' and the iframe is not same origin with its parent without 'allow-same-origin. I checked that a data: URI as the src of an iframe with a sandbox attribute is also not same origin with its parent unless 'allow-same-origin' is specified. 

Looks like I need to clean up the javascript: discrepancy discussed in comment 246 and test origin stuff for javascript: URI's and then see if Dan has any more ideas :)

Comment 250

[Ian Melven :imelven]

Origins and 'allow-same-origin' with javascript: URIs seems to work correctly.

Comment 251

[Ian Melven :imelven]

(In reply to Ian Melven :imelven from comment #250)
> Origins and 'allow-same-origin' with javascript: URIs seems to work
> correctly.

when using javascript: URI's as the src of a sandboxed iframe, i mean.

Comment 252

[Ian Melven :imelven]

(In reply to Boris Zbarsky (:bz) from comment #245)
> There's an existing doc in the <iframe src="javascript:"> case as well.  I
> don't see why they aren't behaving identically; they should be.  If they're
> not, _that_'s a bug.  Imo.

i understand what's going on with the test cases now, to some degree :

the relevant code is in dom/src/jsurl/nsJSProtocolHandler.cpp's JSThunk::EvaluateScript() :

+    // Sandboxed document check: javascript: URI's are disabled
+    // in a sandboxed document unless 'allow-scripts' was specified.
+    nsIDocument* doc = aOriginalInnerWindow->GetExtantDoc();
+    if (doc && (doc->SandboxFlags() & SANDBOXED_SCRIPTS)) {
+        return NS_ERROR_DOM_RETVAL_UNDEFINED;
+    }
+

the intent of this is to stop stuff like onload='javascript: ...' working when scripts are disabled due to sandboxing. 

The test case with <iframe sandbox='' src='javascript: ...html...'> goes through this code and doc is 'about:blank'

The test case with <iframe sandbox='' src='some_html_file.html'>, if you change the src in an event handler such as a button click or onload, when this code is hit, doc will point to that html document, which will have its sandbox flags set. 

This behavior does seem a little strange - suggestions on how to address it are welcome, I'll keep thinking about it.

Comment 253

[Boris Zbarsky [:bzbarsky]]

You should probably set sandbox flags correctly in CreateAboutBlankContentViewer?

Comment 254

[Ian Melven :imelven]

(In reply to Boris Zbarsky (:bz) from comment #253)
> You should probably set sandbox flags correctly in
> CreateAboutBlankContentViewer?

gah, smaug mentioned that before - when he did, i tested <iframe src='about:blank'> which works as it goes through the frame loader and is correctly sandboxed. The about:blank that's in an iframe before loading any src attribute is not correctly sandboxed as you point out. 

I'm curious about how other browsers handle this case, it seems pretty weird to check if the <iframe>'s sandbox attribute allows scripting to control whether the parent can successfully set its src to be a javascript: URI ?

Comment 255

[Boris Zbarsky [:bzbarsky]]

<iframe src="javascript:"> runs the script in the script context of the iframe, not the parent.  So it should probably apply the iframe's sandbox flags.  But it's worth double-checking what the spec actually says for this case.

Comment 256

[Devdatta Akhawe [:devd]]

Me and imelven chatted about this. 

Let me paste some spec writings:

Note 1:
"When a browsing context is first created, it must be created with a single Document in its session history, whose address is about:blank"
from http://www.whatwg.org/specs/web-apps/current-work/multipage/browsers.html#browsing-context

Note 2:
"These flags only take effect when the nested browsing context of the iframe is navigated."
from http://www.whatwg.org/specs/web-apps/current-work/multipage/the-iframe-element.html#attr-iframe-sandbox


Note 3:
"
If a browsing context is being navigated to a javascript: URL, and the source browsing context for that navigation, if any, has scripting disabled
let result be void
" 

From from http://www.whatwg.org/specs/web-apps/current-work/multipage/webappapis.html#concept-js-deref

-----
the key question is: Is the loading of about:blank on docshell creation (note 1) a navigation event (note 2)? Looking at the way the spec is phrased (note 1), I would say it is not. This means, the about:blank doc doesn't have any sandboxing flags, which means the javascript: should execute.

Note that, even if the src=javascript: executes, the alert below shouldn't execute

<iframe sandbox="" src="javascript:'<script>alert();</script>'">

because when the document with this 1 script node is created, a navigation event happened, and the sandbox flags should be copied, and the alert should be disabled.

----

that said, I think we are going against the spirit of the spec. I think the spec really should be changed to say "When a browsing context is created, navigate it to about:blank"  instead of "create it with about:blank in session history". Alternatively, one could say "create with about:blank in session history" really means "navigate from ⊥ to about:blank". 

The iframe spec for example also says
"The sandbox attribute, when specified, enables a set of extra restrictions on any content hosted by the iframe."

which I would argue, also includes the about:blank first loaded.

Chrome also disables the javascript: for src, which is what we should aim for. Thus I vote for fixing this in aboutblankcontentviewer

Comment 257

[Boris Zbarsky [:bzbarsky]]

> Is the loading of about:blank on docshell creation (note 1) a navigation event

I don't believe it is.

Which means that if you have <iframe sandbox> (no src) and you createElement a <script> and put it inside, it should run, per current spec.  It sounds like you want to not do that, and I tend to agree.  What do other UAs do?

> which means the javascript: should execute.

That depends on the definition of "source browsing context for that navigation", right?  Sounds to me like the spec is saying that it's the scripting state of the source browsing context, not the scripting state of the browsing context that's being navigated, that's relevant here, so it's possible that a javascript: src should execute in some of these cases even if the document it's running against is in fact sandboxed.  Again, worth checking what other UAs do and whether the spec makes sense.

> Note that, even if the src=javascript: executes, the alert below shouldn't execute

Agreed.

> I think the spec really should be changed to say "When a browsing context is created,
> navigate it to about:blank"

No, because "navigate" is a specific, asynchronous, algorithm.

But it might make sense for the spec to say that when this about:blank creation happens the current sandbox flags of the navigation context are applied to the resulting document.

Whatever we decide to do, we should:

1)  Make sure we have tests for all the cases involved in our tree.
2)  Raise any discrepancies between what we do and what the spec says as spec issues.
3)  Contribute the testcases to the spec test suite.

Comment 258

[Devdatta Akhawe [:devd]]

Chrome doesn't execute javascript: URI src if sandbox is set. I think we should replicate that behavior. 

> But it might make sense for the spec to say that when this about:blank
> creation happens the current sandbox flags of the navigation context are
> applied to the resulting document.

+1

Comment 259

[Hixie (not reading bugmail)]

I'll fix the spec. The intent is that the magic initial about:blank also be sandboxed. For various reasons, navigating to it isn't quite the right solution, but I don't recall off-hand what those reasons are. Please assume for now that the spec will be updated to say that the about:blank initial document acts for sandboxing as if the iframe had been navigated to it. (Not sure when I'll be able to actually fix the spec.)

Comment 260

[Ian Melven :imelven]

Thanks devd, bz and hixie for the discussion ! I'm going to make CreateAboutBlankContentViewer apply the sandbox flags and add tests for various cases of <iframe sandbox="" src=javascript: ...> to the general tests in this patch. 

Note to self : have a quick look at data: and blob: again after making this change as well.

Comment 261

[Ian Melven :imelven]

Attachment: iframe sandbox tests - general v9

Add tests for src="javascript:'... some HTML with script call in it ...';" both as the initial src of a sandboxed iframe with and without 'allow-scripts' and as a navigated src of a sandboxed iframe with and without 'allow-scripts'.

Comment 262

[Ian Melven :imelven]

Attachment: CheckMayLoad changes to allow sandboxed workers to load blob and data URIs v5

Unbitrot, carry over r+

Comment 263

[Ian Melven :imelven]

Attachment: workaround for workers needing a string origin v4

Unbitrot, carrying over the r+

Comment 264

[Ian Melven :imelven]

Attachment: iframe sandbox v24

* Apply the docshell's sandbox flags to the document created by CreateAboutBlankContentViewer, this fixes the discrepancy discussed in comment 252 through comment 259, thanks again bz, devd, and hixie :D 

r?'ing to jst for a 2nd review or to find a 2nd reviewer after smaug's r+ 

i will ping dveditz again to see how he feels about this secreview wise. 

My goal is to try and land this in Firefox 17, if possible.

Comment 265

[Ian Melven :imelven]

A try run with the current set of patches : https://tbpl.mozilla.org/?tree=Try&rev=473ab79f4881

Comment 266

[Ian Melven :imelven]

Just spoke to dveditz again about the secreview for this bug and he has said it's ok to land this on nightly with the test suite in this bug :)

When we move on to CSP sandbox, 'allow-popups' or other additions to iframe sandbox, those should be discussed with dveditz and go through secreview again.

Comment 267

[Johnny Stenback (:jst)]

Comment on attachment 649006 [details] [diff] [review]
iframe sandbox v24

Given that we block access to localStorage if the SANDBOXED_ORIGIN flag is set, should we do the same thing for IndexedDB?

r=jst for this patch either way, if need be, we can follow up in separate patches to block access to IndexedDB.

Comment 268

[Robert O'Callahan (:roc) (email my personal email if necessary)]

A sandboxed iframe must definitely not have access to the IndexedDB databases of its origin!

Comment 269

[Ian Melven :imelven]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #268)
> A sandboxed iframe must definitely not have access to the IndexedDB
> databases of its origin!

yeah, jst and i just chatted about that, i'm in total agreement. Going to post to whatwg to bring it up there spec-wise. i assume that Webkit and IE 10 have implemented this restriction too, but we will see :)

I'll add another patch to block indexedDB if 'allow-same-origin' isn't specified and also add some tests around this.

Comment 270

[Ian Melven :imelven]

(In reply to Ian Melven :imelven from comment #269)
>
> I'll add another patch to block indexedDB if 'allow-same-origin' isn't
> specified and also add some tests around this.

indexedDB is blocked in an iframe that's sandboxed without 'allow-same-origin' - the error when you try to open the DB is "UnknownError: The operation failed for reasons unrelated to the database itself and not covered by any other error code.". We could possibly log a better error in this case, but that seems ok to do as a followup. 

I'll add a couple tests to the general tests and also clean up the patches wrt bug #'s in  commit messages etc. to get ready for landing.

Comment 271

[Devdatta Akhawe [:devd]]

In general, any persistent mechanism should automatically fail since we are running in  a NullPrincipal. This includes cookies, localStorage, indexedDb, sessionStorage, the filesystem APIs and so on. If it does succeed (i.e., no JS error is shown), the result won't ever be accessible again. If this is not the case, then that's a separate bug.

We do want to fail loudly so as to prevent data loss. This sucks because every new persistent storage mechanism will require a "sandbox failure mode" patch.

I suggest creating a separate bug that tries to catch all such persistent write failures, and automatically replaces those errors with a "Failed because you are sandboxed"

Comment 272

[Ian Melven :imelven]

(In reply to Devdatta Akhawe from comment #271)
> In general, any persistent mechanism should automatically fail since we are
> running in  a NullPrincipal. This includes cookies, localStorage, indexedDb,
> sessionStorage, the filesystem APIs and so on. If it does succeed (i.e., no
> JS error is shown), the result won't ever be accessible again. If this is
> not the case, then that's a separate bug.
> 
> We do want to fail loudly so as to prevent data loss. This sucks because
> every new persistent storage mechanism will require a "sandbox failure mode"
> patch.
> 
> I suggest creating a separate bug that tries to catch all such persistent
> write failures, and automatically replaces those errors with a "Failed
> because you are sandboxed"

does bug 768664 cover this ?

Comment 273

[Devdatta Akhawe [:devd]]

yes!

Comment 274

[Ian Melven :imelven]

Attachment: iframe sandbox v25

Unbitrot, clean up commit message in patch in preparation for landing.

Carrying over r+

Comment 275

[Ian Melven :imelven]

Attachment: CheckMayLoad changes to allow sandboxed workers to load blob and data URIs v6

Unbitrot, clean up commit message in patch in preparation for landing.

Carrying over r+

Comment 276

[Ian Melven :imelven]

Attachment: workaround for workers needing a string origin v5

Unbitrot, clean up commit message in patch in preparation for landing.

Carrying over r+

Comment 277

[Ian Melven :imelven]

Attachment: iframe sandbox tests - general v10

Clean up commit message in patch in preparation for landing.

Carrying over r+

Comment 278

[Ian Melven :imelven]

Attachment: iframe sandbox tests - inheritance v6

Clean up commit message in patch in preparation for landing.

Carrying over r+

Comment 279

[Ian Melven :imelven]

Attachment: iframe sandbox tests - workers v3

Clean up commit message in patch in preparation for landing.

Carrying over r+

Comment 280

[Ian Melven :imelven]

Attachment: iframe sandbox tests - same origin v5

Clean up commit message in patch in preparation for landing.

Carrying over r+

Comment 281

[Ian Melven :imelven]

Attachment: iframe sandbox tests - navigation v8

Clean up commit message in patch in preparation for landing.

Carrying over r+

Comment 282

[Ian Melven :imelven]

Attachment: iframe sandbox tests - plugins v6

Clean up commit message in patch in preparation for landing.

Carrying over r+

Comment 283

[Ian Melven :imelven]

Attachment: block indexed db in a sandboxed document without 'allow-same-origin' + tests

I decided to explicitly block indexedDB the same way I did localStorage and sessionStorage, it seems safer that way.

I also added tests that indexedDB access is blocked in a sandboxed document without 'allow-same-origin' and allowed in a sandboxed document with 'allow-same-origin'.

Comment 284

[Ian Melven :imelven]

Attachment: 650384: block indexed db in a sandboxed document without 'allow-same-origin' + tests

Add r=jst to commit message to be ready to land :D

Comment 285

[Ian Melven :imelven]

I'll just leave this here for now : https://tbpl.mozilla.org/?tree=Try&rev=56803dd3e7cd

Comment 286

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Looks pretty orange to me.

Comment 287

[Ian Melven :imelven]

(In reply to :Ms2ger from comment #286)
> Looks pretty orange to me.

Yeah, the iframe sandbox plugin tests seem to have broken - it's happening locally for me as well and I'm working on fixing it :)

Comment 288

[Ian Melven :imelven]

I think the plugin tests might be failing because of Bug 781126 - AllowPlugins = false on docshell is no longer working ? | Thunderbird Permanent TEST-UNEXPECTED-FAIL | test-plugins-policy.js | Plugin has not been blocked in message as expected.

Comment 289

[Ian Melven :imelven]

https://tbpl.mozilla.org/?tree=Try&rev=a5e8c5717e80

looking much better than the last time...

Comment 290

[Ian Melven :imelven]

https://hg.mozilla.org/integration/mozilla-inbound/rev/63a4ad84d4c8
https://hg.mozilla.org/integration/mozilla-inbound/rev/9eb507cc06e5
https://hg.mozilla.org/integration/mozilla-inbound/rev/7bd4f494eb06
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5cda7690840
https://hg.mozilla.org/integration/mozilla-inbound/rev/43418ba7faff
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbeb787ced41
https://hg.mozilla.org/integration/mozilla-inbound/rev/58eac2139a58
https://hg.mozilla.org/integration/mozilla-inbound/rev/22442334592f
https://hg.mozilla.org/integration/mozilla-inbound/rev/15963019be1b
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2b98e6e2f9c

Comment 291

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/63a4ad84d4c8
https://hg.mozilla.org/mozilla-central/rev/9eb507cc06e5
https://hg.mozilla.org/mozilla-central/rev/7bd4f494eb06
https://hg.mozilla.org/mozilla-central/rev/b5cda7690840
https://hg.mozilla.org/mozilla-central/rev/43418ba7faff
https://hg.mozilla.org/mozilla-central/rev/fbeb787ced41
https://hg.mozilla.org/mozilla-central/rev/58eac2139a58
https://hg.mozilla.org/mozilla-central/rev/22442334592f
https://hg.mozilla.org/mozilla-central/rev/15963019be1b
https://hg.mozilla.org/mozilla-central/rev/f2b98e6e2f9c

Comment 292

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 650374 [details] [diff] [review]
workaround for workers needing a string origin v5

Review of attachment 650374 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/workers/WorkerPrivate.cpp
@@ +2594,5 @@
>            domain = file;
>          }
> +        // Workaround for workers needing a string domain - will be fixed
> +        // in a followup after this lands.
> +        else if (document->GetSandboxFlags() & SANDBOXED_ORIGIN) {

This was filed?

Comment 293

[Ian Melven :imelven]

(In reply to :Ms2ger from comment #292)
> Comment on attachment 650374 [details] [diff] [review]
> workaround for workers needing a string origin v5
> 
> Review of attachment 650374 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/workers/WorkerPrivate.cpp
> @@ +2594,5 @@
> >            domain = file;
> >          }
> > +        // Workaround for workers needing a string domain - will be fixed
> > +        // in a followup after this lands.
> > +        else if (document->GetSandboxFlags() & SANDBOXED_ORIGIN) {
> 
> This was filed?

Indeed it was, see bug 752529 - there's some patches there r?'d to bz too.

Comment 294

[Jean-Yves Perrier [:teoli]]

Updated: https://developer.mozilla.org/en-US/docs/HTML/Element/iframe
and
https://developer.mozilla.org/en-US/docs/Firefox_17_for_developers

Ian, are you interested in writing a Hacks article about the subject? It is a pretty big feature that will get a lot of interest…

Comment 295

[Ian Melven :imelven]

(In reply to Jean-Yves Perrier [:teoli] from comment #294)
> Updated: https://developer.mozilla.org/en-US/docs/HTML/Element/iframe
> and
> https://developer.mozilla.org/en-US/docs/Firefox_17_for_developers
> 
> Ian, are you interested in writing a Hacks article about the subject? It is
> a pretty big feature that will get a lot of interest…

Jean-Yves - yes, I was planning on writing a post about this feature - there's a few things I think it would be better to finish up first though, I'll drop you a note.

Comment 296

[Paul Rouget [:paul]]

(In reply to Ian Melven :imelven from comment #295)
> Jean-Yves - yes, I was planning on writing a post about this feature

If you do, please make sure it ends up on hacks.mozilla.org (check with Chris Heilmann, or send me an email).

Comment 297

[James Hartig]

I had noticed that you're not testing for window.top.location being prevented when not using allow-top-navigation, but instead you're testing using the target attribute on an anchor tag. Shouldn't window.top.location changing be prevented like it is in IE and Webkit?

I was testing on Aurora (17.0a2 (2012-09-21)) and Nightly (18a1 (2012-09-21)).

Comment 298

[Olli Pettay [:smaug][[e-mail]]]

Uh, yes, please file a new bug and make it block this one.

Comment 299

[Hixie (not reading bugmail)]

FYI, I finally got around to fixing the spec.

Comment 300

[The 8472]

For the purpose of content blocking addons, would it be possible to add a flag that prevents cross-origin communication (postMessage&co) with the sandbox?

The idea is to maintain compartmentalization of a no-cross-origin default policy while whitelisting embedded content that are sufficiently isolated that it might as well be a distinct top level window.