Closed Bug 671389 Opened 9 years ago Closed 4 years ago

Implement CSP sandbox directive

Categories

(Core :: DOM: Security, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox38 --- affected
firefox50 --- fixed

People

(Reporter: sephr, Assigned: pacaro)

References

(Blocks 2 open bugs, )

Details

(Keywords: dev-doc-complete, Whiteboard: [CSP 1.1],[domsecurity-active])

Attachments

(4 files, 97 obsolete files)

12.62 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
14.78 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
21.66 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
28.83 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
The HTML5 sandboxing features just won't cut it for anything that isn't text/html. As it stands, it's impossible to sandbox an image/svg+xml game a user uploads to your SVG game site, as HTML5 only introduces a text/html-sandboxed media type.

I know that CSP isn't specifically intended for sandboxing user generated content, but it feels like the most appropriate place for sandboxing features, as opposed to introducing a new media type.
Blocks: CSP
Component: DOM: Core & HTML → Security
QA Contact: general → toolkit
I just noticed the "sandbox" directive in the CSP draft at http://dvcs.w3.org/hg/content-security-policy/raw-file/tip/csp-specification.dev.html#sandbox so I have changed the bug summary to "Implement CSP sandbox directive".
Summary: Merge HTML5 sandboxing features with CSP → Implement CSP sandbox directive
Severity: normal → enhancement
Component: Security → DOM: Core & HTML
QA Contact: toolkit → general
Depends on: framesandbox
Assignee: nobody → dev.akhawe+mozilla
I propose breaking this up into two: implementing the sandbox directive in enforcement mode and in monitoring mode. If I am not wrong, it will be a lot of work to implement the monitoring mode. The enforcement mode implementation can be pretty quick.

Thoughts ? In particular, imelven how much work do you think it would be to implement the monitoring mode? If its a lot of work, I am not even sure its worth the effort.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attached patch csp-sandbox-enforcement (obsolete) — Splinter Review
Implements CSP Sandbox directive in enforcement mode only
This patch requires that the patches for bug 341604 be imported first.
Webkit doesn't support a report only mode for sandbox either; and I conjecture that IE doesn't either.
A note that we should check that if the sandbox flags already exist, then the CSP value should only lock it down further, not make it looser (and vice versa)
Comment on attachment 635029 [details] [diff] [review]
csp-sandbox-enforcement

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

Looks like this is definitely on the right track - thanks for hacking on it !

::: content/base/public/nsIContentSecurityPolicy.idl
@@ +49,5 @@
>  
>    /**
> +   * Checks whether the sandbox directive is set 
> +   */
> +  readonly attribute boolean isSandbox;

nit : would you consider hasSandbox ?

@@ +146,5 @@
> +  /**
> +   * Delegate method called by the service when the protected document is loaded.
> +   * Responds with whether the sandbox directive is set, and fills in the value.
> +   */
> +  boolean shouldSandbox(out AString sandboxValue);

what's the difference between the return of this vs isSandbox ? 
why do you need both ?

::: content/base/src/CSPUtils.jsm
@@ +151,5 @@
>  
>    this._allowEval = false;
>    this._allowInlineScripts = false;
> +  this._isSandbox = false;
> +  this._sandboxFlags = "";

maybe a nit, but these aren't the same as the actual flags, they're really the optional opt-in strings

@@ +230,4 @@
>        continue directive;
>      }
>  
> +    if (dirname === CSPRep.SANDBOX_DIRECTIVE){

i would suggest a big comment header like the other directives have

@@ +232,5 @@
>  
> +    if (dirname === CSPRep.SANDBOX_DIRECTIVE){
> +      aCSPR._isSandbox = true;
> +      aCSPR._sandboxFlags = dirvalue;
> +      CSPWarning(" setting sandbox to true with dirvalue : "+dirvalue);

why is this a warning ? seems like more of a CSPDdebug message ?

@@ +517,5 @@
>      newRep._allowInlineScripts = this.allowsInlineScripts 
>                             && aCSPRep.allowsInlineScripts;
>  
> +    newRep._isSandbox = this.isSandbox || aCSPRep.isSandbox;
> +    //sandbox flags separated by a space

a more verbose comment might be clearer here

::: content/base/src/contentSecurityPolicy.js
@@ +80,5 @@
>  }
>  
>  ContentSecurityPolicy.prototype = {
> +  //classID:          Components.ID("{AB36A2BF-CB32-4AA6-AB41-6B4E4444A221}"),
> +  classID:          Components.ID("{62cae874-f163-4b8c-a3e9-ce4d88e9a59c}"),

you'll need to remove the commented out bits like this in the final patch

::: content/base/src/nsDocument.cpp
@@ +31,5 @@
>  #include "nsIScriptRuntime.h"
>  #include "nsCOMArray.h"
>  
> +#include "nsSandboxFlags.h"
> +

you don't actually need this unless you're using the defines, i think ?

@@ +2503,5 @@
>          }
>      }
>  
> +    nsAutoString sandboxFlags;
> +    bool shouldSandbox=false;

meta-nit : please use the same spacing around = and , etc as is used in the rest of the file

@@ +2506,5 @@
> +    nsAutoString sandboxFlags;
> +    bool shouldSandbox=false;
> +    rv = mCSP->ShouldSandbox(sandboxFlags,&shouldSandbox);
> +    NS_ENSURE_SUCCESS(rv,rv);
> +    if(shouldSandbox){

the null principal etc should only happen if SANDBOXED_ORIGIN is set,
won't this be taken care of later in the usual spot for iframe sandbox
once you set the docshell flags ?

@@ +2509,5 @@
> +    NS_ENSURE_SUCCESS(rv,rv);
> +    if(shouldSandbox){
> +      nsCOMPtr<nsIPrincipal> ownerPrincipal = do_CreateInstance("@mozilla.org/nullprincipal;1");
> +      nsContentUtils::SetUpChannelOwner(ownerPrincipal, mChannel, chanURI, false,true);
> +      docShell->SetSandboxFlags(nsContentUtils::ParseSandboxAttributeToFlags(sandboxFlags));

this implies sandboxFlags is really the string attribute and not the flags - a little confusing perhaps
Attachment #635029 - Flags: feedback+
Attached patch Merging imelven's feedback (obsolete) — Splinter Review
@imelven: All good points, and all fixed. Thanks. I have adopted the name sandboxAttrValue instead of sandboxFlags. The isSandbox addition to the IDL was a mistake, and not needed. I have removed it. 


I think the setsandboxflags function in your iframe_sandbox patch should be modified to be "make it only stronger than what it is already". Or I can do the hackery here, but I think it is better in setSandboxFlags. What do you think?
Attachment #635029 - Attachment is obsolete: true
(In reply to Devdatta Akhawe from comment #8)
> 
> I think the setsandboxflags function in your iframe_sandbox patch should be
> modified to be "make it only stronger than what it is already". Or I can do
> the hackery here, but I think it is better in setSandboxFlags. What do you
> think?

that's a good thought, i'm wondering about what to do if the sandbox attribute is removed ? In that case we want to clear all flags - i suppose we could treat that as a special case though. I'll look at doing this in the iframe sandbox patch. Thanks !
Asking for geekboy's review, since he is familiar with the CSP code.
Attachment #635535 - Attachment is obsolete: true
Attachment #635818 - Flags: review?(sstamm)
Comment on attachment 635818 [details] [diff] [review]
added support for policy intersection

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

Good start!  Please update your diff-maker to show 8 lines of context (https://developer.mozilla.org/En/Creating_a_patch).  It's hard to tell why some of the code changes were necessary because of lack of context.

Much of this is nits, but there are a few things to do yet:
* Please check format of things like if statements and line-length of comments
* You'll also need a super-review for the idl changes.
* Tests for this (will depend on the iframe-sandbox code).

::: content/base/public/nsIContentSecurityPolicy.idl
@@ +138,5 @@
>                        in ACString        aMimeType,
>                        in nsISupports     aExtra);
> +  /**
> +   * Delegate method called by the service when the protected document is loaded.
> +   * Responds with whether the sandbox directive is set, and fills in the value.

Please describe the parameters and return value in depth; see other documentation comments in this file for examples.  Also, please include a blank line between the previous declaration and this comment block.

::: content/base/src/CSPUtils.jsm
@@ +232,5 @@
>  
> +    // SANDBOX DIRECTIVE ////////////////////////////////////////////////
> +    // We don't parse the value of the sandbox directive, since there is 
> +    // already native code that does this for iframe sandbox
> +    if (dirname === CSPRep.SANDBOX_DIRECTIVE){

Nit: space between ) and {

@@ +235,5 @@
> +    // already native code that does this for iframe sandbox
> +    if (dirname === CSPRep.SANDBOX_DIRECTIVE){
> +      aCSPR._isSandbox = true;
> +      aCSPR._sandboxAttrValue = dirvalue;
> +      CSPdebug(" setting sandbox to true with dirvalue : "+dirvalue);

nit: spaces before and after + operator.  No space in beginning of message.

@@ +438,5 @@
>      }
> +
> +    if(this._isSandbox){
> +      dirs.push("sandbox "+ this._sandboxAttrValue);
> +    }

Nit: Format this if signature like the others in this file:
 --> if (foo) {

Please make sure this is consistent in the rest of your js edits.

@@ +521,5 @@
>                             && aCSPRep.allowsInlineScripts;
>  
> +    newRep._isSandbox = this.isSandbox || aCSPRep.isSandbox;
> +    //sandbox attribute values are separated by space: the iframe sandbox takes care of repeated values
> +    newRep._sandboxAttrValue = this._sandboxAttrValue + " "+ aCSPRep._sandboxAttrValue;

I'm not sure just concatenating the two values is the best option.  You'll have to either parse and intersect the sandbox policies here, or have to keep the policies separate until they are parsed.

::: content/base/src/contentSecurityPolicy.js
@@ +103,4 @@
>      return this._reportOnlyMode || this._policy.allowsEvalInScripts;
>    },
>  
> +

nit: remove duplicate blank line.

@@ +451,5 @@
> +        CSPdebug("shouldsandbox: "+this._policy.isSandbox);
> +        aSandboxAttrValue.value = this._policy.sandboxAttrValue ;
> +        CSPdebug("sandbox attribute value: "+aSandboxAttrValue.value+"|");
> +        return this._reportOnlyMode || this._policy.isSandbox;
> +    },

nit: blank line between this function and the next one.

::: content/base/src/nsDocument.cpp
@@ +2374,5 @@
>  
>    mChannel = aChannel;
>  
> +  nsresult rv = InitCSP();
> +  NS_ENSURE_SUCCESS(rv, rv);

I assume this move of InitCSP() is because of where some iframe-sandbox-specific code gets added to nsDocument::StartDocumentLoad().

@@ +2504,5 @@
> +    nsAutoString sandboxAttrValue;
> +    bool shouldSandbox = false;
> +    rv = mCSP->ShouldSandbox(sandboxAttrValue,&shouldSandbox);
> +    NS_ENSURE_SUCCESS(rv,rv);
> +    if(shouldSandbox){

Nit: Space after if and after closed parens:
 --> if (foo) {

@@ +2508,5 @@
> +    if(shouldSandbox){
> +      PRUint32 sandboxFlags;
> +      docShell->GetSandboxFlags(&sandboxFlags);
> +      sandboxFlags |= nsContentUtils::ParseSandboxAttributeToFlags(
> +          sandboxAttrValue);

Nit: Don't wrap the parameter list.

This is probably where you'll want to get the multiple sandbox policies and parse them (or maybe it's better to parse the sandbox policies earlier and store the parsed flags in the CSP).
Attachment #635818 - Flags: review?(sstamm) → review-
@geekboy: thanks for the review. I think there is a flaw in the code with the handling of multiple CSP policies. I think the call needs to return an array of all the policies seen so far, and intersect (OR) them in the C++ code.

One of the reasons for moving the InitCSP call was to let the existing code take care of setting the sandbox flags. (the original patch had a SetupChannelOwner call inside InitCSP. Looking at how we might add support for CSP via the meta tag, I think the clean way to do it would be to just call the InitCSP function if we see a META with X-Content-Security-Policy http-equiv As such, it makes sense to force the channelOwner in the InitCSP code itself. 

thoughts, particularly from imelven?
I think I have addressed all of Sid's comments and nits. Thanks for pointing out the error in the sandbox intersection logic, that was pretty lame on my part.

One question: I am using an nsIArray now. Who is in charge of freeing it?
Attachment #635818 - Attachment is obsolete: true
Attachment #637207 - Flags: review?(sstamm)
I was mistakenly attaching the flag to the docshell instead of the document. This is now fixed.
Attachment #637207 - Attachment is obsolete: true
Attachment #637207 - Flags: review?(sstamm)
Moved initCSP back down, since it doesn't need to be moved any more.
Attachment #637985 - Attachment is obsolete: true
Note that the sandbox patch doesn't check the document's sandbox flags for disabling plugins; instead that is done on the docshell. This means, the current patch doesn't block plugins. I have left a comment on the bug, but an ugly solution is to just implicitly add a 'object-src: 'none'" if we have a CSP sandbox header without allow-plugins.
Comment on attachment 637988 [details] [diff] [review]
Implement CSP sandbox directive support

Obsoleting this patch: need to implement either a 'make this principal null' interface on nsIPrincipal, or need to move the initCSP code to much earlier. In any case, the patch is wrong. I will wait for sandbox to land before moving further on this.
Attachment #637988 - Attachment is obsolete: true
(In reply to Devdatta Akhawe from comment #17)
> Comment on attachment 637988 [details] [diff] [review]
> Implement CSP sandbox directive support
> 
> Obsoleting this patch: need to implement either a 'make this principal null'
> interface on nsIPrincipal, or need to move the initCSP code to much earlier.
> In any case, the patch is wrong. I will wait for sandbox to land before
> moving further on this.

Dev - fyi, bug 341604 is getting pretty close to landing, so I've started looking at this. We've moved the priority up to a P2 on the security roadmap, since we need this to have a full sandboxing solution. The plugin check has been moved to the document now so that part should be taken care of.

Meanwhile, it would be great if anyone wanted to start porting the tests for bug 341604 over to test CSP sandbox - Sid and I talked about this and it basically boils down to creating a ^headers^ file for each sandboxed document used in the iframe sandbox tests that does the same sandboxing as the iframe sandbox attribute does in those tests and creating new main test pages for each set of CSP sandbox tests. I will try to make this happen if nobody else wants to pick it up.
Attached patch Strawman MakeNull patch (obsolete) — Splinter Review
Attachment #656931 - Flags: review?(imelven)
Here's a sample patch that adds a MakeNull call to nsIPrincipal. @imelven: can you please take a look, and see if it makes sense? I would prefer to get feedback whether this is the right approach, before actually completing all the patches. 

bz: can you also take a look? It is a little different from what you had suggested: instead of creating a new nsNullPrincipal, I just added a mIsNull flag.

With this makenull, the meta tag support should also be pretty easy to support.
You need to fix things like GetOrigin at least, right?

Your Equals() check became asymmetric; that's probably a bug.

Various other nits about casing and spacing and whatnot, but those are less important than deciding whether we think we can whack all the moles on nsPrincipal sufficiently.  As in, decide what every single API method should do for the mIsNull case.
hmm .. you are right. This what a mole thing looks like a bad idea: maybe just create a new NullPrincipal, check if it exits and for *ALL* calls just forward it to the newly created nullprincipal?
err .. submitted too early

s/exits/exists

bz: do you suspect any issues with that approach? If not, I will go ahead and create a patch that does this.
That might work better, as long as you still have to make Equals() and Subsumes work right (i.e. check object-identity before forwarding, verify that they work correctly when the RHS is the outer nsIPrincipal).
By verify, do you mean write tests? Definitely need those for such a major change.

Cause, if I am not wrong, shouldn't both functions work without any special changes? The 'this' value inside the inner nsNullPrincipal's equals/subsumes function should point to the same inner nsNullPrincipal if the outer nsPrincipal is the same, and point to different nsnulls if the outer is not the same.
> By verify, do you mean write tests?

That too, but also just by code inspection.

> The 'this' value inside the inner nsNullPrincipal's equals/subsumes function should point
> to the same inner nsNullPrincipal if the outer nsPrincipal is the same

How are you going from outer to inner for the _argument_ to equals/subsumes?
(In reply to Boris Zbarsky (:bz) from comment #24)
> That might work better, as long as you still have to make Equals() and
> Subsumes work right (i.e. check object-identity before forwarding, verify
> that they work correctly when the RHS is the outer nsIPrincipal).

fwiw, this outer/inner approach is exacly what i was going to suggest.  due to previous experiences with other code bases, anything where we have to 'whack a mole' wrt security is pretty terrifying to me ;)

tests for this inner / outer principal split definitely sound like a requirement, in addition to the csp sandbox tests itself we will need (these should basically be the iframe sandbox tests but sending csp headers instead of using the sandbox attribute.
> How are you going from outer to inner for the _argument_ to equals/subsumes?

duh .. yes. thanks for pointing this out.
Attachment #656931 - Attachment is obsolete: true
Attachment #656931 - Flags: review?(imelven)
Here's a new patch. Does this seem right?

Subsumes just calls equals, so I didn't change it. Also, I didn't make any changes to getorigin, getdomain, getURI calls. Can someone else confirm if that sounds about right?
Attachment #657090 - Flags: review?(bzbarsky)
Comment on attachment 657090 [details] [diff] [review]
Patch to add support for making a principal null at runtime

The Equals() impl is still wrong when |this| has no mNullPrincipal but aOther has an mNullPrincipal.

Why _not_ change GetDomain, GetURI, GetOrigin?

New method should be lowercase in the IDL.

Don't you need to fix Read and Write as well?  That's actually possibly the biggest problem with this approach: it can break certain session restore cases.  :(

For what it's worth, I still think we should simply not support going to a null principal in a meta tag, and fix this the other way (moving the CSP parsing to before we set up the document principal)...
Attachment #657090 - Flags: review?(bzbarsky) → review-
> The Equals() impl is still wrong when |this| has no mNullPrincipal but
> aOther has an mNullPrincipal.
> 

yup. Will fix.

> Why _not_ change GetDomain, GetURI, GetOrigin?
> 

I wasn't sure about what should be the return value of these functions once they are made null. A principal for http://www.foobar.com/1.html should still return this URI even if the principal is null, right? Similarly for domains and origin? It is just the security checks that change?


> 
> Don't you need to fix Read and Write as well?  That's actually possibly the
> biggest problem with this approach: it can break certain session restore
> cases.  :(ss 
>

Ok. I don't know what read/write do: my assumption was that I only need to worry about things inside nsiprincipal.idl, as all other things are internal. Does this system not sound right? I ignore read/write because they are not in the nsiprincipal idl.
 
> For what it's worth, I still think we should simply not support going to a
> null principal in a meta tag, and fix this the other way (moving the CSP
> parsing to before we set up the document principal)...

I like the "makeNull" as a call to support in general: a mechanism for a principal to "drop" privileges is important to have imho. But to implement CSP sandbox, it makes sense to do what is quick and clean. How far back would CSP initialization have to be moved?
> A principal for http://www.foobar.com/1.html should still return this URI even if the
> principal is null, right?

That _really_ depends.... I would assume it's a security hole to do that until proven otherwise because someone will assume they can use the principal URI for some security check.  Certainly .origin is used for security checks; that is its only reason for existence.

> Ok. I don't know what read/write do:

They serialize and deserialize a principal.

> I ignore read/write because they are not in the nsiprincipal idl.

They're on another interface that principals implement.  They don't just implement nsIPrincipal!

> I like the "makeNull" as a call to support in general

My concern is that getting it right is _HARD_ as the above discussion shows.

CSP _parsing_ would need to move back to before the GetChannelPrincipal call in nsDocument::Reset.

So the ordering would be: 1) parse CSP.  2) Use the result to determine what principal to use.  3)  Store the parsed CSP on the resulting principal.
And the point is, we parse CSP right now in InitCSP, which is called at the end of StartDocumentLoad.  Reset() is called a bit earlier in the same method.  So if we break up InitCSP into two methods, one of which does the actual init of mCSP and one of which does the setting of mCSP on the principal, I think it should all just work...
ok. Let me do this:

1. Rewrite the CSP header sandbox support by moving/breaking-up initCSP so that it figures out the sandbox status before reset is called.

2. Create a new bug for meta sandbox support, in which I will import this patch and hack on it. Even if it is hard, I still think it is worth working on and trying to get right.
(In reply to Devdatta Akhawe from comment #35)
> ok. Let me do this:
> 
> 1. Rewrite the CSP header sandbox support by moving/breaking-up initCSP so
> that it figures out the sandbox status before reset is called.
> 
> 2. Create a new bug for meta sandbox support, in which I will import this
> patch and hack on it. Even if it is hard, I still think it is worth working
> on and trying to get right.

sounds like a solid plan - my personal preference FWIW is to do CSP sandbox first as you suggest, since then we have a complete solution to sandboxing documents (iframe sandbox + CSP sandbox for the case where the framed content is access directly).
(In reply to Devdatta Akhawe from comment #35)
> 2. Create a new bug for meta sandbox support, in which I will import this
> patch and hack on it. Even if it is hard, I still think it is worth working
> on and trying to get right.

I'm not sure we will need this.  Meta tag support for csp is in csp 1.1.  We have had discussions in the webappsec working group about not honoring the sandbox directive if it is set in a meta tag policy.  Nothing has been decided for sure yet, as the 1.1 spec is still in the really early stages.
Whiteboard: [CSP 1.1]
It seems the call to reset depends on aReset but InitCSP doesn't depend on aReset. I don't know the context of the code that well: is it ok to just remove the aReset check?
(In reply to Devdatta Akhawe from comment #38)
> It seems the call to reset depends on aReset but InitCSP doesn't depend on
> aReset. I don't know the context of the code that well: is it ok to just
> remove the aReset check?

I don't understand why you need to remove the aReset check - you just need to make sure that the CSP has been set up to the point where ::Reset knows it should put a null principal on the document, don't you ?
Even if I setup CSP, if the ::Reset call is never made (because of the if condition), then a nullPrincipal won't be set, right?

To be clear, I can think of a number of ways to solve this: I don't want to make a guess though, and rather would prefer knowing what is the context of this call and what's the right thing to do.
The whole point of aReset is to say that you should not reset!

aReset == false is only used in the following cases:

1)  Document for a DOMParser
2)  Document for cross-site XHR 

In both cases the caller has already set up a specific principal for the document.

It's not clear to me how, or whether, CSP headers should affect those cases.
hmm..  as an aside, why are we are calling InitCSP for cases with aReset==false ?

Anyways, I think we can get away with not doing anything for CSP sandbox in the 2 cases. If it was called for same-site XHR, then maybe there was something to think about.
Comment on attachment 657605 [details] [diff] [review]
Move CSP code to before principal reset

Requesting review on patch that moves the CSP code. The actual sandbox patch (previously proposed examples are in this bug) will check hasSandbox, and change the arguments to ::Reset appropriately.
Attachment #657605 - Flags: review?(bzbarsky)
I think we might want to only honor sandbox when it's in the 1.0 spec compliant header - see bug 783049

what do you think, Sid ?
Attachment #657605 - Attachment is obsolete: true
Attachment #657605 - Flags: review?(bzbarsky)
Attachment #657090 - Attachment is obsolete: true
@imelven: Can you take a look at and see how this interacts if the docshell is sandboxed-origin? I think it is all right, but a second pair of eyes would be nice.
Attachment #657692 - Flags: review?(bzbarsky)
Attachment #657693 - Flags: review?(imelven)
Comment on attachment 657692 [details] [diff] [review]
Move CSP code to before principal reset

The NodePrincipal() check in InitCSP() makes no sense with this patch.

I'm not that happy with the move of the mChannel set (and also the RetrieveRelevantHeaders, because that can change various document stat) to before the Reset call.  Probably better to pass the channel to InitCSP instead of using mChannel in it, with a comment explaining why.

Using "mCSP" for an on-stack variable is weird.  Just call it "csp".

Also, if you're going to write stuff into a COMPtr, you probably want to pass an nsIContentSecurityPolicy** to InitCSP.

GetPrincipal() on documents never returns null, fwiw.
Attachment #657692 - Flags: review?(bzbarsky) → review-
Attachment #657693 - Flags: review?(imelven) → review?(sstamm)
thanks for the review.

> The NodePrincipal() check in InitCSP() makes no sense with this patch.

Can you elaborate why? It seems we still want to make sure that we don't do anything for system principals?
 
> I'm not that happy with the move of the mChannel set (and also the
> RetrieveRelevantHeaders, because that can change various document stat) to
> before the Reset call.  Probably better to pass the channel to InitCSP
> instead of using mChannel in it, with a comment explaining why.
> 

So, the initcsp code will then use the channel to get the headers it wants? Can you elaborate on why the move to the new location bad?

> Using "mCSP" for an on-stack variable is weird.  Just call it "csp".
> 
> Also, if you're going to write stuff into a COMPtr, you probably want to
> pass an nsIContentSecurityPolicy** to InitCSP.
> 
> GetPrincipal() on documents never returns null, fwiw.

thanks .. all fixed!
> Can you elaborate why?

Because it's now happening before the principal of the document has been set up, which means the principal of the document is always a null principal.

> So, the initcsp code will then use the channel to get the headers it wants?

Yes.

> Can you elaborate on why the move to the new location bad?

Because it makes assumptions about which members Reset() will and will not reset.
> Because it's now happening before the principal of the document has been set
> up, which means the principal of the document is always a null principal.
>

hmm; so I should move it to the part of the code that attaches the csp policy (and not attach in case it is a system URI ) ? Not sure if this case can actually ever happen.
Attachment #657692 - Attachment is obsolete: true
Attachment #658691 - Flags: review?(bzbarsky)
> Not sure if this case can actually ever happen.

It can certainly happen if someone tries hard enough; we have APIs that can make it happen.

It _shouldn't_ be done because it's kinda insecure, of course....
Comment on attachment 658691 [details] [diff] [review]
Move CSP code to before principal reset

Important issues:

* This patch crashes on non-HTTP channels.  How was this patch tested?
* The patch changes behavior for the byte-to-unicode conversion: we used to
  byte-inflate, but now we treat the bytes as UTF-8.  Which does the spec say
  to do?

Nits:

* This is still null-checking GetPrincipal.
* Please have a space in "//Copy".
* Please put aCSP as the second argument of InitCSP.
* Please fix the indentation on your GetResponseHeader calls.  Especially the
  first one.
* Do you still need to CSP headers in RetrieveRelevantHeaders?
* Why use NS_IF_ADDREF on a pointer you know is non-null?  It would be better
  to just swap() the nsCOMPtr into aCSP once you're done setting it up.
Attachment #658691 - Flags: review?(bzbarsky) → review-
Attachment #658691 - Attachment is obsolete: true
Attachment #659032 - Flags: review?(bzbarsky)
Thanks! I think I fixed all the issues; I used NS_ConvertASCIIToUTF16 for the headers, per our discussion. I am not certain of the indentation: I just used vim's default, with the settings given at the top of the file. Does it look right?
Comment on attachment 659032 [details] [diff] [review]
Move CSP code to before principal reset

>+    if (httpChannel == NULL) {

  if (!httpChannel) {

Also, set *aCSP to null before that return NS_OK, please.

>+    

Please don't add the stray blank line.

r=me with those.
Attachment #659032 - Flags: review?(bzbarsky) → review+
Attachment #659032 - Attachment is obsolete: true
Comment on attachment 659265 [details] [diff] [review]
Move CSP code to before principal reset

thanks!

I added a *aCSP=NULL to the top of InitCSP, so that it will default to null unless we reach the csp.forget call. Removed the new line too. Carrying over the r+ from bz
Attachment #659265 - Flags: review+
nullptr, please.
use nullptr instead of null
Attachment #659265 - Attachment is obsolete: true
Attachment #659276 - Flags: review+
Attachment #657693 - Attachment is obsolete: true
Attachment #657693 - Flags: review?(sstamm)
Attachment #659416 - Attachment is obsolete: true
Attachment #659430 - Flags: review?(sstamm)
Depends on: 790747
Comment on attachment 659276 [details] [diff] [review]
Move CSP code to before principal reset

this patch obsoleted by bug 790747
Attachment #659276 - Attachment is obsolete: true
Priority: -- → P3
Duplicate of this bug: 845140
Assignee: dev.akhawe+mozilla → nobody
Assignee: nobody → deian
Attachment #659430 - Attachment is obsolete: true
Attachment #659430 - Flags: review?(sstamm)
Attachment #8355167 - Flags: review?(sstamm)
Attachment #8355167 - Flags: review?(ian.melven)
Hey Deian,

thanks for your work on CSP sandbox - it's awesome to see it moving along. I'll take a look as soon as I can. Bob Owen has been doing a _lot_ of work on sandbox navigation lately (including some excellent tests) as well as implementing allow-popups so I've cc'd him as well - Bob, can you take a look through Deian's patches when you have time ?
(In reply to Ian Melven :imelven from comment #76)
> Bob, can you take a
> look through Deian's patches when you have time ?

Hopefully, I'll get some time next week.
bholley has asked me to re-write my tests for bug 785310, which I hope to start in the morning.
Once I've got them out of the way I'll take a look.
(In reply to Bob Owen from comment #77)
> (In reply to Ian Melven :imelven from comment #76)
> > Bob, can you take a
> > look through Deian's patches when you have time ?
> 
> Hopefully, I'll get some time next week.
> bholley has asked me to re-write my tests for bug 785310, which I hope to
> start in the morning.
> Once I've got them out of the way I'll take a look.

Thanks, I'll try to take a look myself this weekend :D
Comment on attachment 8355168 [details] [diff] [review]
0002-Bug-671386-Part-2-tests-for-CSP-sandbox-directive-ge.patch

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

It looks like these are pretty much 1 for 1 copies of the existing iframe sandbox tests for the HTML attribute. It would be great if there was some
way to not have to duplicate all the tests, but I'm not sure there's an easy way of doing so. One reason at least for doing so is making sure that 
both the CSP and HTML sandbox tests stay in sync and don't diverge. For example, Bob is working on fixing navigation via window.location in bug 785310 and is adding
a fair amount of tests, he would then need to duplicate all those tests for a CSP based sandbox attribute if we follow that model.
I know Garrett (grobinson) has been working on CSP test stuff as well so I've cc'd him. I'm not sure if it makes more sense to have 
these tests live with the CSP tests or the iframe sandbox tests (which is where they are now).

Since CSP sandbox is about sandboxing documents in the same way as iframe sandbox, I wonder if this approach is the right one. Really we should have one set
of tests which test that a sandboxed document, whether via CSP or the sandbox attribute, is correctly restricted from doing thing it shouldn't be doing.
All we really care about beyond that is that the sandbox attribute is setting the flags appropriately based on its value. I'm interested to hear what
Bob, Garrett and others have to say on this issue as well.

I think the most important thing to test for CSP sandbox is the parsing of the attribute. I made a comment in the general test file about
the tests for parsing of the HTML sandbox attribute - some of the things it says are being tested not being tested. Note that the CSP
spec may differ for what whitespace separators are acceptable for the sandbox directive within a CSP policy vs in the HTML iframe sandbox
attribute. Parsing the attribute from a CSP is what you're really adding with this work so I think there should definitely be some
more testing around, for example making sure sandbox is correctly handed when specified with other directives.

::: content/html/content/test/file_iframe_sandbox_csp_c_if3.html
@@ +8,5 @@
> +
> +</head>
> +<script type="text/javascript">
> +  function doStuff() {
> +    dump("*** c_if3 has loaded\n");

nit: avoid committing dump() statements (this is probably something you copied over from an existing test and is likely my fault originally :) )

::: content/html/content/test/file_iframe_sandbox_csp_c_if4.html
@@ +12,5 @@
> +    window.parent.ok_wrapper(result, desc);
> +  }
> +
> +  function doStuff() {
> +    // try to open a new window via target="_blank", window.open(), and showModalDialog()

i feel like these tests may have changed/been expanded/possibly been superceded by Bob's navigation work, but he will know better.

@@ +22,5 @@
> +      window.open("about:blank");
> +    } catch (error) {
> +      threw = true;
> +    }
> +    

nit : whitespace (again probably was in the original file)

@@ +31,5 @@
> +      window.showModalDialog("about:blank");
> +    } catch(error) {
> +      threw = true;
> +    }
> +    

nit : whitespace (again probably was in the original file)

::: content/html/content/test/file_iframe_sandbox_csp_c_if6.html
@@ +11,5 @@
> +  function ok(result, desc) {
> +    window.parent.ok_wrapper(result, desc);
> +    window.parent.postMessage({ok: result, desc: desc}, "*");
> +  }
> +  

nit : whitespace (again probably was in the original file)

::: content/html/content/test/file_iframe_sandbox_csp_c_if6.html^headers^
@@ +1,1 @@
> +Content-Security-Policy: sandbox  allow-same-origin  allow-scripts  

nit : trailing whitespace

::: content/html/content/test/test_iframe_sandbox_csp_general.html
@@ +76,5 @@
> +
> +  // fails if bad
> +  // 8) test that script in an event listener (img onerror) cannot run in an iframe sandboxed without "allow-scripts"
> +  // (done in file_iframe_sandbox_csp_c_if2.html which has sandbox='')
> +  

nit : whitespace (again probably was in the original file)

@@ +117,5 @@
> +  // 16) test that a sandboxed iframe can't open a new window using window.ShowModalDialog
> +  // this is done via file_iframe_sandbox_csp_c_if4.html which is sandboxed with "allow-scripts" and "allow-same-origin"
> +  // 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
> +  

nit : whitespace (again probably was in the original file)

@@ +122,5 @@
> +  // passes twice if good
> +  // 17) test that a sandboxed iframe can access same-origin documents and run scripts when its sandbox attribute
> +  // is separated with two spaces 
> +  // done via file_iframe_sandbox_csp_c_if6.html which is sandboxed with "  allow-scripts  allow-same-origin  "
> +  

nit : whitespace (again probably was in the original file)

@@ +125,5 @@
> +  // done via file_iframe_sandbox_csp_c_if6.html which is sandboxed with "  allow-scripts  allow-same-origin  "
> +  
> +  // passes twice if good
> +  // 18) test that a sandboxed iframe can access same-origin documents and run scripts when its sandbox attribute
> +  // is separated with tabs

This doesn't seems true - the headers for file_iframe_sandbox_csp_if6 are :Content-Security-Policy: sandbox  allow-same-origin  allow-scripts

This is only testing what test #18 is looking for. It doesn't look like #19 #20 and #21 are being tested. Note what I said in the
main comment, we have to be sure we handle what the CSP spec says is acceptable here, which may or may not be the same
as what is allowed by the HTML spec for iframe sandbox.

@@ +127,5 @@
> +  // passes twice if good
> +  // 18) test that a sandboxed iframe can access same-origin documents and run scripts when its sandbox attribute
> +  // is separated with tabs
> +  // done via file_iframe_sandbox_csp_c_if6.html which is sandboxed with "&#x09;allow-scripts&#x09;allow-same-origin&#x09;"
> +  

nit : whitespace (again probably was in the original file)

@@ +132,5 @@
> +  // passes twice if good
> +  // 19) test that a sandboxed iframe can access same-origin documents and run scripts when its sandbox attribute
> +  // is separated with line feeds
> +  // done via file_iframe_sandbox_csp_c_if6.html which is sandboxed with "&#x0a;allow-scripts&#x0a;allow-same-origin&#x0a;"
> +  

nit : whitespace (again probably was in the original file)

@@ +137,5 @@
> +  // passes twice if good
> +  // 20) test that a sandboxed iframe can access same-origin documents and run scripts when its sandbox attribute
> +  // is separated with form feeds
> +  // done via file_iframe_sandbox_csp_c_if6.html which is sandboxed with "&#x0c;allow-scripts&#x0c;allow-same-origin&#x0c;"
> +  

nit : whitespace (again probably was in the original file)
Attachment #8355168 - Flags: review?(ian.melven)
Also I see you add some tests in patch 9 which test the intersection of an iframe sandbox attribute and a CSP sandbox attribute - I think that's a very important thing to test as well, nice work :) I would assume that the intersection logic is the same as it is for the sandbox attribute when CSP isn't involed : you can only further restrict and never loosen permissions.
Comment on attachment 8355169 [details] [diff] [review]
0003-Bug-671389-Part-3-tests-for-CSP-sandbox-directive-in.patch

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

The overall comment that I don't think duplicating all the tests of the restrictions is necessarily the right way to go here applies to these tests as well.

::: content/html/content/test/test_iframe_sandbox_csp_inheritance.html
@@ +14,5 @@
> +/** Test for Bug 671389 - Implement CSP sandbox directive **/
> +/** Inheritance Tests **/
> +
> +SimpleTest.waitForExplicitFinish();
> +  

nit: whitespace

@@ +35,5 @@
> +
> +  completedTests++;
> +
> +  if (result) {
> +	  passedTests++;

nit: weird indentation

@@ +36,5 @@
> +  completedTests++;
> +
> +  if (result) {
> +	  passedTests++;
> +  } 

nit: whitespace

@@ +37,5 @@
> +
> +  if (result) {
> +	  passedTests++;
> +  } 
> +     

nit: whitespace

@@ +39,5 @@
> +	  passedTests++;
> +  } 
> +     
> +  if (completedTests == 4) {
> +    is(passedTests, 4, "there should be 3 passed inheritance tests");

These numbers do not match up, looks like there's 4 'passes if good' tests in this file.

@@ +48,5 @@
> +function doTest() {
> +  // fails if bad
> +  // 1) an iframe with no sandbox attribute inside an iframe that has sandbox = ""
> +  // should not be able to execute scripts (cannot ever loosen permissions)
> +  // (done by file_iframe_sandbox_csp_a_if2.html contained within file_iframe_sandbox_csp_a_if1.html)

file_iframe_sandbox_csp_a_if2.html has headers of "Content-Security-Policy: sandbox" so the description does not match the test.

@@ +53,5 @@
> +
> +  // fails if bad
> +  // 2) an iframe with sandbox = "allow-scripts" inside an iframe that has sandbox = ""
> +  // should not be able to execute scripts (cannot ever loosen permissions)
> +  // (done by file_iframe_sandbox_csp_a_if2.html contained within file_iframe_sandbox_csp_a_if1.html)

this description can't be true since the header for file_iframe_sandbox_csp_a_if2.html has headers of "Content-Security-Policy: sandbox"

@@ +86,5 @@
> +  // to execute scripts
> +  // (done by file_iframe_sandbox_csp_a_if2.html contained within file_iframe_sandbox_csp_a_if3.html)
> +
> +  // passes if good
> +  // 9) make sure that changing the sandbox flags on an iframe (if_8) doesn't affect

This test is a little strange from a CSP perspective - at the moment, there's no way to change the CSP of a document after load.

I suppose it could be a test for the case when a document is sandboxed via a CSP and then has its sandbox attribute changed,
but that might make more sense being bundled in with the CSP+sandbox attribute intersection tests ?

@@ +90,5 @@
> +  // 9) make sure that changing the sandbox flags on an iframe (if_8) doesn't affect
> +  // the sandboxing of subloads of content within that iframe
> +  var if_8 = document.getElementById('if_8');
> +  if_8.sandbox = 'allow-scripts';
> +  if_8.contentWindow.doSubload(); 

nit: trailing whitespace
Attachment #8355169 - Flags: review?(ian.melven)
Comment on attachment 8355170 [details] [diff] [review]
0004-Bug-671389-Part-4-tests-for-CSP-sandbox-directive-wo.patch

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

The overall comment that I don't think duplicating all the tests of the restrictions is necessarily the right way to go here applies to these tests as well.

As a side note, there was a recent clarification around workers and CSP in the current 1.1 spec : https://github.com/w3c/webappsec/commit/63534a54245df586bf02d44ceab07e6611450d15
but this pertains to the policy applied to workers, which is not being tested here.
Attachment #8355170 - Flags: review?(ian.melven)
Comment on attachment 8355167 [details] [diff] [review]
0001-Bug-671386-Part-1-Implement-CSP-sandbox-directive.patch

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

Overall, looks good. A few small comments. I also flagged Garrett to take a look.

However, I don't think there's enough tests of the more CSP specific bits of this functionality, for example, intersecting multiple policies with and without sandbox directives etc.

::: content/base/public/nsIContentSecurityPolicy.idl
@@ +204,5 @@
> +   * Delegate method called by the service when the protected document is loaded.
> +   * Responds with whether the sandbox directive is set, and fills in the value.
> +   *
> +   * @param sandboxFlags [out]
> +   *    Set to the sandbox flags accompaning the sandbox directive.

nit: typo - 'accompanying'

::: content/base/src/CSPUtils.jsm
@@ +1824,5 @@
> + * Class to model sandbox flag list
> + */
> +
> +this.SandboxFlags = function SandboxFlags(flags) {
> +  this._flags = flags.sort();

why are these sorted ? is that per spec ? (just curious)

@@ +1832,5 @@
> +  /**
> +   * Generates canonical string representation of the
> +   * SandboxFlags.
> +   */
> +  toString : function() {

i assume this is used when outputting the CSP policy for warning/error messages and the like..

@@ +1843,5 @@
> +   *        another SandboxFlags
> +   * @returns
> +   *        true if they have the same flags
> +   */
> +  equals: function(that) {

i assume this is used for the CSP logic that does interesections and such of directives...

::: content/base/src/contentSecurityPolicy.js
@@ +806,5 @@
> +    let hasSandbox = false;
> +    let flags = [];
> +    // If there are multiple policies with the "sandbox" directive,
> +    // all the sandbox flags are intersected. NOTE: this corresponds
> +    // to the _least_ flexible policy.

good comment ! i might say 'permissive' instead of 'flexible' to be even clearer.

::: content/base/src/nsDocument.cpp
@@ +2603,4 @@
>    }
>  
>    // Figure out if we need to apply an app default CSP or a CSP from an app manifest
> +  nsCOMPtr<nsIPrincipal> principal = NodePrincipal();

any particular reason you changed to a nsCOMPtr here ?

@@ +2755,4 @@
>      }
>    }
>  
> +  // ----- Set sandbox flags

It might be worth commenting that any sandbox flags set via iframe sandbox will already be set at this point since that happens before InitCSP is called.

@@ +2761,5 @@
> +  rv = csp->ShouldSandbox(strFlags, &shouldSandbox);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  if (shouldSandbox) {
> +    nsAttrValue attr; attr.ParseAtomArray(strFlags);

nit: these should be on separate lines

@@ +2767,5 @@
> +
> +    if (mSandboxFlags & SANDBOXED_ORIGIN) {
> +      // Reset the document principal to a null principal
> +      principal = do_CreateInstance("@mozilla.org/nullprincipal;1");
> +      SetPrincipal(principal);

bz or smaug or bholley etc. should probably look at this bit to verify doing this here is OK - i'm worried about changing the principal that has already been set, although that's based on a vague recollection that I had to avoid that when I originally did iframe sandbox.
Attachment #8355167 - Flags: review?(ian.melven) → review?(grobinson)
Thanks for looking at these! (I am boarding a plane now, so only replying to the test comments; will reply to comment 83 and fix the things you pointed out tonight/tomorrow.)

Indeed the tests are copy-paste of the existing ones, only modified to
add headers and new files (with corresponding headers) for cases when
the attribute is changed. I am not happy with this approach either,
but I went with it mainly to test the existing implementation.

I think your proposal of just testing the parsing of the headers and
setting the correct flags is the better way to go (we may need to
expose an API that returns the document sandbox flags). This means we
can get rid of all these tests; we only need to test the intersection
of CSP and iframe sandbox.
(In reply to Deian Stefan from comment #84)
> Thanks for looking at these! (I am boarding a plane now, so only replying to
> the test comments; will reply to comment 83 and fix the things you pointed
> out tonight/tomorrow.)

No problem, thanks for working on this ! 

> Indeed the tests are copy-paste of the existing ones, only modified to
> add headers and new files (with corresponding headers) for cases when
> the attribute is changed. I am not happy with this approach either,
> but I went with it mainly to test the existing implementation.

That makes a lot of sense :)

> I think your proposal of just testing the parsing of the headers and
> setting the correct flags is the better way to go (we may need to
> expose an API that returns the document sandbox flags). This means we
> can get rid of all these tests; we only need to test the intersection
> of CSP and iframe sandbox.

Well, we sort-of have an API for getting something's sandbox flags (for an iframe at least).. I was wondering about this while reading your patches - when something is sandboxed via CSP, will that (and should it) update the value of the sandbox attribute ?

For example, I have a document containing an iframe (a.html) and a.html is served with Content-Security-Policy: sandbox allow-scripts. If I get a.sandbox's value from JS, will it (should it) say 'allow-scripts' ? 

As an aside, this is another case where it's a giant pain that we can't tell from reading .sandbox whether something is sandboxed or not, because the value is an empty string :(
(In reply to Ian Melven :imelven from comment #83)

Thanks for taking a look! Fixed the things you mentioned (only
replying to some of the comments below):

> However, I don't think there's enough tests of the more CSP specific bits of
> this functionality, for example, intersecting multiple policies with and
> without sandbox directives etc.

Agreed, will add these tests.

> ::: content/base/src/CSPUtils.jsm
> @@ +1824,5 @@
> > + * Class to model sandbox flag list
> > + */
> > +
> > +this.SandboxFlags = function SandboxFlags(flags) {
> > +  this._flags = flags.sort();
> 
> why are these sorted ? is that per spec ? (just curious)

We need to do a list comparison in 'equals'. I am doing it in the
constructor for performance reasons.  Since the flags are not
inspectable, this only comes up when we print the CSP policy to
console. We don't need to do it this way, but if we care about leaving
the order intact then we should also keep duplicate and unknown flags
as well, IMO. Thoughts?


> @@ +1832,5 @@
> > +  /**
> > +   * Generates canonical string representation of the
> > +   * SandboxFlags.
> > +   */
> > +  toString : function() {
> 
> i assume this is used when outputting the CSP policy for warning/error
> messages and the like..

That's right, updated comment to reflect usage.

> @@ +1843,5 @@
> > +   *        another SandboxFlags
> > +   * @returns
> > +   *        true if they have the same flags
> > +   */
> > +  equals: function(that) {
> 
> i assume this is used for the CSP logic that does interesections and such of
> directives...

Like toString, CSPRep's 'equals' expects each directive to define an
equals function.


> ::: content/base/src/nsDocument.cpp
> @@ +2603,4 @@
> >    }
> >  
> >    // Figure out if we need to apply an app default CSP or a CSP from an app manifest
> > +  nsCOMPtr<nsIPrincipal> principal = NodePrincipal();
> 
> any particular reason you changed to a nsCOMPtr here ?

We need a nsCOMPtr when we create a new nullprincipal. This allows us
to set csp on 'principal', whether it was changed or not.
Attachment #8355167 - Attachment is obsolete: true
Attachment #8355167 - Flags: review?(sstamm)
Attachment #8355167 - Flags: review?(grobinson)
References: <bug-671389-469131@https.bugzilla.mozilla.org/> <bug-671389-469131-NEs9ox8HXs@https.bugzilla.mozilla.org/>
User-Agent: Notmuch/0.16 (http://notmuchmail.org) Emacs/24.3.1 (x86_64-unknown-linux-gnu)
> I think your proposal of just testing the parsing of the headers and
>> setting the correct flags is the better way to go (we may need to
>> expose an API that returns the document sandbox flags). This means we
>> can get rid of all these tests; we only need to test the intersection
>> of CSP and iframe sandbox.
>
> Well, we sort-of have an API for getting something's sandbox flags (for an
> iframe at least).. I was wondering about this while reading your patches - when
> something is sandboxed via CSP, will that (and should it) update the value of
> the sandbox attribute ?
>
> For example, I have a document containing an iframe (a.html) and a.html is
> served with Content-Security-Policy: sandbox allow-scripts. If I get
> a.sandbox's value from JS, will it (should it) say 'allow-scripts' ? 

No it will not. Doing this in the general sense seems unsafe.  For
example, if a.com includes an iframe with sandbox attribute
(allow-scripts) from b.com, the latter can leak information to a.com by
not setting one of the flags (e.g. allow-scripts). Now a.com can inspect
the attribute and learn 1 bit. Of course this is already a problem (if
we leave implementation as is): allow-same-origin can be used to leak
a bit (just by checking contentDocument). I think it would be useful
for us to amend the spec to precisely describe the semantics -- what
do you think?

> As an aside, this is another case where it's a giant pain that we can't tell
> from reading .sandbox whether something is sandboxed or not, because the value
> is an empty string :(

Our current implementation is no longer a string, but to-spec
DOMSettableToken. I agree and IMO the attribute should be something
along the lines of optional DOMSettableToke. Using getAttribute to check
if the attribute was set is ugly...
(In reply to Ian Melven :imelven from comment #82)
> As a side note, there was a recent clarification around workers and CSP in
> the current 1.1 spec :
> https://github.com/w3c/webappsec/commit/
> 63534a54245df586bf02d44ceab07e6611450d15
> but this pertains to the policy applied to workers, which is not being
> tested here.

Do we have a bug for this? (We now need InitCSP-like functionality for workers.)
Flags: needinfo?(grobinson)
Attachment #8355168 - Attachment is obsolete: true
Attachment #8355169 - Attachment is obsolete: true
Attachment #8355170 - Attachment is obsolete: true
Attachment #8355171 - Attachment is obsolete: true
Attachment #8355172 - Attachment is obsolete: true
Attachment #8355173 - Attachment is obsolete: true
Attachment #8355175 - Attachment is obsolete: true
Attachment #8355177 - Attachment is obsolete: true
Attachment #8357482 - Flags: review?(ian.melven)
Comment on attachment 8356290 [details] [diff] [review]
0001-Bug-671386-Part-1-Implement-CSP-sandbox-directive.patch

Following up on Comment 86: what are your thoughts on sorting the flags, keeping duplicates, or invalid flags?
Attachment #8356290 - Flags: review?(grobinson)
> No it will not. Doing this in the general sense seems unsafe.  For
> example, if a.com includes an iframe with sandbox attribute
> (allow-scripts) from b.com, the latter can leak information to a.com by
> not setting one of the flags (e.g. allow-scripts). Now a.com can inspect
> the attribute and learn 1 bit. Of course this is already a problem (if
> we leave implementation as is): allow-same-origin can be used to leak
> a bit (just by checking contentDocument). I think it would be useful
> for us to amend the spec to precisely describe the semantics -- what
> do you think?

Following up, our implementation follows the semantics detailed here
(thanks Dev for the ref!):

http://lists.w3.org/Archives/Public/public-web-security/2011Nov/0012.html
Deian, I'll try to get to this and do another pass this weekend :)
(In reply to Deian Stefan from comment #86)
>
> > > +this.SandboxFlags = function SandboxFlags(flags) {
> > > +  this._flags = flags.sort();
> > 
> > why are these sorted ? is that per spec ? (just curious)
> 
> We need to do a list comparison in 'equals'. I am doing it in the
> constructor for performance reasons.  Since the flags are not
> inspectable, this only comes up when we print the CSP policy to
> console. We don't need to do it this way, but if we care about leaving
> the order intact then we should also keep duplicate and unknown flags
> as well, IMO. Thoughts?

I don't think it makes sense to keep duplicate an unknown flags. Sorting
to make equals faster makes sense. I was just curious what the reasoning 
was here, you could maybe add a comment to explain the .sort perhaps. I suppose
sorting could cause a problem if someone set 'sandbox' to an unsorted list value
and then compared that to the set value, but that seems like a bit of an edge case I suppose.
 
> > i assume this is used when outputting the CSP policy for warning/error
> > messages and the like..
> 
> That's right, updated comment to reflect usage.

awesome, thanks !

> > > +  nsCOMPtr<nsIPrincipal> principal = NodePrincipal();
> > 
> > any particular reason you changed to a nsCOMPtr here ?
> 
> We need a nsCOMPtr when we create a new nullprincipal. This allows us
> to set csp on 'principal', whether it was changed or not.

ah got it, thanks !
(In reply to Deian Stefan from comment #87)
>
> > For example, I have a document containing an iframe (a.html) and a.html is
> > served with Content-Security-Policy: sandbox allow-scripts. If I get
> > a.sandbox's value from JS, will it (should it) say 'allow-scripts' ? 
> 
> No it will not. Doing this in the general sense seems unsafe.  For
> example, if a.com includes an iframe with sandbox attribute
> (allow-scripts) from b.com, the latter can leak information to a.com by
> not setting one of the flags (e.g. allow-scripts). Now a.com can inspect
> the attribute and learn 1 bit. Of course this is already a problem (if
> we leave implementation as is): allow-same-origin can be used to leak
> a bit (just by checking contentDocument). I think it would be useful
> for us to amend the spec to precisely describe the semantics -- what
> do you think?

yes, i think the spec should define what happens here (either or both of the WHATWG HTML spec and the CSP spec). However, in your example, if a.com uses <iframe sandbox='allow-scripts'> to load a document from b.com which doesn't use CSP sandbox to set allow-scripts, the document will still have allow-scripts so I don't see what's being learned here - I may be misunderstanding your example. If allow-same-origin is set i think the game is essentially over anyways since you can use window.eval() in that case and pretty much do whatever you want. As always, I'm curious what Chrome does here wrt the sandbox attribute of an iframe mirroring the value of the framed document's CSP. Maybe Mike West knows off hand (I have cc'd him and will ping him :) )
 
> Our current implementation is no longer a string, but to-spec
> DOMSettableToken. I agree and IMO the attribute should be something
> along the lines of optional DOMSettableToke. Using getAttribute to check
> if the attribute was set is ugly...

yeah, we had that discussion around getAttribute on WHATWG. optional DOMSettableTokenList makes sense to me. I suppose it's good to be per spec here instead of using a string as I previously had done (and as Chrome had done at the time) - http://www.mail-archive.com/whatwg@lists.whatwg.org/msg30133.html is the thread that contains the discussion and reasoning at the time if you're curious :)
(In reply to Deian Stefan from comment #88)
> (In reply to Ian Melven :imelven from comment #82)
> > As a side note, there was a recent clarification around workers and CSP in
> > the current 1.1 spec :
> > https://github.com/w3c/webappsec/commit/
> > 63534a54245df586bf02d44ceab07e6611450d15
> > but this pertains to the policy applied to workers, which is not being
> > tested here.
> 
> Do we have a bug for this? (We now need InitCSP-like functionality for
> workers.)

not as far as I know. Probably best to file one if you don't mind.
Comment on attachment 8357482 [details] [diff] [review]
0002-Bug-671389-Part-2-Export-document-sandbox-flags-to-c.patch

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

Olli, could you take a look at this to see what you think please ?

::: content/base/public/nsIDocument.h
@@ +522,5 @@
>  
>    /**
> +   * Get string representation of sandbox flags.
> +   */
> +  void GetSandboxFlags(nsAString& aFlags);

this is a little confusing because there's a uint32_t GetSandboxFlags() as well. Maybe GetSandboxFlagsAsString ? bz or khuey or smaug might have more informed or stronger opinions here.

::: content/base/src/nsContentUtils.cpp
@@ +986,5 @@
> +#define IF_FLAG(flag, atom) \
> +  if (!(flags & flag)) { \
> +    nsAutoString tmp; \
> +    nsGkAtoms::atom->ToString(tmp); \
> +    attr.Append(tmp + NS_LITERAL_STRING(" "));\

obviously there will be an extra trailing space here..

::: dom/webidl/Document.webidl
@@ +351,5 @@
>  
> +// Extension to give chrome JS the ability to get the underlying
> +// sandbox flag attribute
> +partial interface Document {
> +  [ChromeOnly] readonly attribute DOMString? sandboxFlags;

i assume we need to do this so the chrome CSP JS can access these :)
Attachment #8357482 - Flags: review?(ian.melven)
Attachment #8357482 - Flags: review?(bugs)
Attachment #8357482 - Flags: review+
(In reply to Ian Melven :imelven from comment #98)
> Comment on attachment 8357482 [details] [diff] [review]
> 0002-Bug-671389-Part-2-Export-document-sandbox-flags-to-c.patch
>
> ::: dom/webidl/Document.webidl
> @@ +351,5 @@
> >  
> > +// Extension to give chrome JS the ability to get the underlying
> > +// sandbox flag attribute
> > +partial interface Document {
> > +  [ChromeOnly] readonly attribute DOMString? sandboxFlags;
> 
> i assume we need to do this so the chrome CSP JS can access these :)

ah, I see this is actually so the tests can check the flag as you mentioned, since with your approach we can't use the iframe's sandbox value. I read http://lists.w3.org/Archives/Public/public-web-security/2011Nov/0012.html (which predates Firefox's iframe sandbox implementation) and the other messages in that thread (thanks and thanks to Dev for the reference - it was extremely helpful in understanding the reasoning here). So yes, I agree with the implementation the way you have it and needing this chrome only accessor :)
Comment on attachment 8357483 [details] [diff] [review]
0003-Bug-671389-Part-3-tests-for-CSP-sandbox-directive.patch

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

Overall, I think this is an improvement. file_iframe_sandbox_csp_c_if2.html is the only real question I have and there's a few nits about comments. I've flagged Bob to take a look to see what he thinks as he's been spending a lot of time with iframe sandbox tests :)

::: content/html/content/test/file_iframe_sandbox_csp_c_if2.html
@@ +16,5 @@
> +  }
> +</script>
> +<script src='file_iframe_sandbox_fail.js'></script>
> +<body onLoad='window.parent.postmessage({ok: false, desc: "documents sandboxed without allow-scripts should NOT be able to run script from event handlers"}, "*");doStuff();'>
> +  I am sandboxed with no permissions

this is not true, the ^headers^ file for this has allow-scripts - i also don't understand how these tests don't fail
since they're supposed to fail if the scripts can run (which they should be able to do with allow-scripts in the header)

::: content/html/content/test/file_iframe_sandbox_csp_c_if4.html
@@ +36,5 @@
> +    ok(threw, "window.showModalDialog threw a JS exception and was not allowed");
> +  }
> +</script>
> +<body onLoad="doStuff()">
> +  I am sandboxed but with "allow-scripts allow-same-origin"

the ^headers^ file also specifies allow-top-navigation fwiw, that shouldn't affect these tests though.

::: content/html/content/test/file_iframe_sandbox_csp_if2.html
@@ +16,5 @@
> +  }
> +</script>
> +<script src='file_iframe_sandbox_fail.js'></script>
> +<body onLoad='window.parent.postmessage({ok: false, desc: "documents sandboxed without allow-scripts should NOT be able to run script from event handlers"}, "*");doStuff();'>
> +  I am sandboxed with "allow-same-origin"

and also allow-top-navigation but that again shouldn't affect these tests

::: content/html/content/test/test_iframe_sandbox_csp.html
@@ +68,5 @@
> +// 1-7) make sure that CSP header does not bypass iframe attribute
> +// file_iframe_sandbox_csp_c_if1.html performs the same tests as file_iframe_sandbox_csp_c_if1.html
> +// file_iframe_sandbox_csp_c_if2.html performs the same tests as file_iframe_sandbox_csp_c_if2.html
> +// file_iframe_sandbox_csp_c_if3.html performs the same tests as file_iframe_sandbox_csp_c_if3.html
> +// file_iframe_sandbox_csp_c_if4.html performs the same tests as file_iframe_sandbox_csp_c_if4.html

these comments seem like they should be more like file_iframe_sandbox_csp_c_if1.html performs the same tests as file_iframe_sandbox_c_if1.html ?

ie. the _csp_ files perform the same tests as their non CSP counterparts ?

@@ +70,5 @@
> +// file_iframe_sandbox_csp_c_if2.html performs the same tests as file_iframe_sandbox_csp_c_if2.html
> +// file_iframe_sandbox_csp_c_if3.html performs the same tests as file_iframe_sandbox_csp_c_if3.html
> +// file_iframe_sandbox_csp_c_if4.html performs the same tests as file_iframe_sandbox_csp_c_if4.html
> +
> +// fails if bad or passes six

nit: passes six times

@@ +73,5 @@
> +
> +// fails if bad or passes six
> +// 8) make sure that CSP header sets the right flags
> +// file_iframe_sandbox_csp_if1.html checks allow-same-origin and allow-scripts
> +// file_iframe_sandbox_csp_if2.html fails if JS runs; doTest("if_7",...) checks allow-same-origin and allow-top-navigation

should this be doTest("if_6", ...) ?

@@ +76,5 @@
> +// file_iframe_sandbox_csp_if1.html checks allow-same-origin and allow-scripts
> +// file_iframe_sandbox_csp_if2.html fails if JS runs; doTest("if_7",...) checks allow-same-origin and allow-top-navigation
> +// file_iframe_sandbox_csp_if3.html checks allow-forms and allow-scripts
> +// file_iframe_sandbox_csp_if4.html fails if JS runs; doTest("if_8",...) checks empty
> +// file_iframe_sandbox_csp_if5.html fails if any flags were set

because doTest("if_9",..." checks that the flags are null

@@ +101,5 @@
> +<iframe id="if_5" src="file_iframe_sandbox_csp_if1.html" height="10" width="10"></iframe>
> +<iframe id="if_6" src="file_iframe_sandbox_csp_if2.html" height="10" width="10" onload='doTest("if_6", ["allow-same-origin", "allow-top-navigation"]);'></iframe>
> +<iframe id="if_7" src="file_iframe_sandbox_csp_if3.html" height="10" width="10"></iframe>
> +<iframe id="if_8" src="file_iframe_sandbox_csp_if4.html" height="10" width="10" onload='doTest("if_8", [])'></iframe>
> +<iframe id="if_9" src="file_iframe_sandbox_csp_if5.html" height="10" width="10" onload='doTest("if_9", null)'></iframe>

it looks like the checks to see if the flags are correct are done in some of the subframes (the ones that can execute script)
as well as from this parent document - any particular reason for that ?
Attachment #8357483 - Flags: review?(ian.melven)
Attachment #8357483 - Flags: review?(bobowencode)
Attachment #8357483 - Flags: feedback+
Ian, thanks a lot for all the feedback!

(In reply to Ian Melven :imelven from comment #98)
> this is a little confusing because there's a uint32_t GetSandboxFlags() as
> well. Maybe GetSandboxFlagsAsString ? bz or khuey or smaug might have more
> informed or stronger opinions here.

Yes, I think that that's the right way to go.

> ::: content/base/src/nsContentUtils.cpp
> @@ +986,5 @@
> > +#define IF_FLAG(flag, atom) \
> > +  if (!(flags & flag)) { \
> > +    nsAutoString tmp; \
> > +    nsGkAtoms::atom->ToString(tmp); \
> > +    attr.Append(tmp + NS_LITERAL_STRING(" "));\
> 
> obviously there will be an extra trailing space here..

Will fix (should clean up a function in the tests as well).
(In reply to Ian Melven :imelven from comment #100)
> Overall, I think this is an improvement. file_iframe_sandbox_csp_c_if2.html
> is the only real question I have and there's a few nits about comments. I've
> flagged Bob to take a look to see what he thinks as he's been spending a lot
> of time with iframe sandbox tests :)

Thanks again!

> ::: content/html/content/test/file_iframe_sandbox_csp_c_if2.html
> @@ +16,5 @@
> > +  }
> > +</script>
> > +<script src='file_iframe_sandbox_fail.js'></script>
> > +<body onLoad='window.parent.postmessage({ok: false, desc: "documents sandboxed without allow-scripts should NOT be able to run script from event handlers"}, "*");doStuff();'>
> > +  I am sandboxed with no permissions
> 
> this is not true, the ^headers^ file for this has allow-scripts - i also
> don't understand how these tests don't fail
> since they're supposed to fail if the scripts can run (which they should be
> able to do with allow-scripts in the header)


The ^headers^ file has allow-scripts, but the iframe attribute does not:

> <iframe sandbox="" id="if_2" src="file_iframe_sandbox_csp_c_if2.html" height="10" width="10">




> ::: content/html/content/test/file_iframe_sandbox_csp_c_if4.html
> @@ +36,5 @@
> > +    ok(threw, "window.showModalDialog threw a JS exception and was not allowed");
> > +  }
> > +</script>
> > +<body onLoad="doStuff()">
> > +  I am sandboxed but with "allow-scripts allow-same-origin"
> 
> the ^headers^ file also specifies allow-top-navigation fwiw, that shouldn't
> affect these tests though.

Will fix. The allow-top-navigation was added mostly arbitrarily to make sure that parsing of the flag works.

> ::: content/html/content/test/test_iframe_sandbox_csp.html
> @@ +68,5 @@
> > +// 1-7) make sure that CSP header does not bypass iframe attribute
> > +// file_iframe_sandbox_csp_c_if1.html performs the same tests as file_iframe_sandbox_csp_c_if1.html
> > +// file_iframe_sandbox_csp_c_if2.html performs the same tests as file_iframe_sandbox_csp_c_if2.html
> > +// file_iframe_sandbox_csp_c_if3.html performs the same tests as file_iframe_sandbox_csp_c_if3.html
> > +// file_iframe_sandbox_csp_c_if4.html performs the same tests as file_iframe_sandbox_csp_c_if4.html
> 
> these comments seem like they should be more like
> file_iframe_sandbox_csp_c_if1.html performs the same tests as
> file_iframe_sandbox_c_if1.html ?

Yes, sorry for the stupid mistakes.

> @@ +73,5 @@
> > +
> > +// fails if bad or passes six
> > +// 8) make sure that CSP header sets the right flags
> > +// file_iframe_sandbox_csp_if1.html checks allow-same-origin and allow-scripts
> > +// file_iframe_sandbox_csp_if2.html fails if JS runs; doTest("if_7",...) checks allow-same-origin and allow-top-navigation
> 
> should this be doTest("if_6", ...) ?

Yep, sorry again about the carelessness.


> @@ +101,5 @@
> > +<iframe id="if_5" src="file_iframe_sandbox_csp_if1.html" height="10" width="10"></iframe>
> > +<iframe id="if_6" src="file_iframe_sandbox_csp_if2.html" height="10" width="10" onload='doTest("if_6", ["allow-same-origin", "allow-top-navigation"]);'></iframe>
> > +<iframe id="if_7" src="file_iframe_sandbox_csp_if3.html" height="10" width="10"></iframe>
> > +<iframe id="if_8" src="file_iframe_sandbox_csp_if4.html" height="10" width="10" onload='doTest("if_8", [])'></iframe>
> > +<iframe id="if_9" src="file_iframe_sandbox_csp_if5.html" height="10" width="10" onload='doTest("if_9", null)'></iframe>
> 
> it looks like the checks to see if the flags are correct are done in some of
> the subframes (the ones that can execute script)
> as well as from this parent document - any particular reason for that ?

Since some of the subframes do not have allow-scripts set, we need a way to test that the other flags are set correctly and doing it in the parent document seems like the natural approach; did you have an alternative approach in mind?
(In reply to Ian Melven :imelven from comment #96)
> (In reply to Deian Stefan from comment #87)
> >
> > > For example, I have a document containing an iframe (a.html) and a.html is
> > > served with Content-Security-Policy: sandbox allow-scripts. If I get
> > > a.sandbox's value from JS, will it (should it) say 'allow-scripts' ? 
> > 
> > No it will not. Doing this in the general sense seems unsafe.  For
> > example, if a.com includes an iframe with sandbox attribute
> > (allow-scripts) from b.com, the latter can leak information to a.com by
> > not setting one of the flags (e.g. allow-scripts). Now a.com can inspect
> > the attribute and learn 1 bit. Of course this is already a problem (if
> > we leave implementation as is): allow-same-origin can be used to leak
> > a bit (just by checking contentDocument). I think it would be useful
> > for us to amend the spec to precisely describe the semantics -- what
> > do you think?
> 
> yes, i think the spec should define what happens here (either or both of the
> WHATWG HTML spec and the CSP spec).

I'll file bugs (same goes for that of comment 97).

> However, in your example, if a.com uses
> <iframe sandbox='allow-scripts'> to load a document from b.com which doesn't
> use CSP sandbox to set allow-scripts, the document will still have
> allow-scripts so I don't see what's being learned here - I may be
> misunderstanding your example.

If a.com includes an iframe as such:

  <iframe sandbox='allow-scripts' src="https://b.com/ifr.html">

and if ifr.html^headers^ is:
 
  Content-Security-Policy: sandbox

If we update the attribute flags, then a.com learns that ifr.html did
not have the "allow-scripts" flag. (This is not really a big deal since
b.com can communicate with a.com in general anyway.)

More importantly, we can't really change the attribute since this will
affect future documents that are loaded in that iframe (and if it
doesn't, but the attribute getter returns the effective sandbox flags,
the semantics are a bit confusing IMO).

> As always, I'm curious what Chrome does
> here wrt the sandbox attribute of an iframe mirroring the value of the
> framed document's CSP. Maybe Mike West knows off hand (I have cc'd him and
> will ping him :) )

Just checked, Chrome does not update the flags.

> yeah, we had that discussion around getAttribute on WHATWG. optional
> DOMSettableTokenList makes sense to me. I suppose it's good to be per spec
> here instead of using a string as I previously had done (and as Chrome had
> done at the time) -
> http://www.mail-archive.com/whatwg@lists.whatwg.org/msg30133.html is the
> thread that contains the discussion and reasoning at the time if you're
> curious :)


Thanks!
(In reply to Deian Stefan from comment #102)
> (In reply to Ian Melven :imelven from comment #100)
>
> Thanks again!

No problem ! Thanks for all the clarifications.

> > it looks like the checks to see if the flags are correct are done in some of
> > the subframes (the ones that can execute script)
> > as well as from this parent document - any particular reason for that ?
> 
> Since some of the subframes do not have allow-scripts set, we need a way to
> test that the other flags are set correctly and doing it in the parent
> document seems like the natural approach; did you have an alternative
> approach in mind?

Nope, it makes sense that the documents without allow-scripts set need the parent to check the flags.
Attachment #8356290 - Flags: review?(sstamm)
Great we finally implemented it.

But what are we gonna do with http://homakov.blogspot.com/2014/01/using-content-security-policy-for-evil.html ?
(In reply to Deian Stefan from comment #103)
>
> > yes, i think the spec should define what happens here (either or both of the
> > WHATWG HTML spec and the CSP spec).
> 
> I'll file bugs (same goes for that of comment 97).

Thanks ! 
 
> > However, in your example, if a.com uses
> > <iframe sandbox='allow-scripts'> to load a document from b.com which doesn't
> > use CSP sandbox to set allow-scripts, the document will still have
> > allow-scripts so I don't see what's being learned here - I may be
> > misunderstanding your example.
> 
> If a.com includes an iframe as such:
> 
>   <iframe sandbox='allow-scripts' src="https://b.com/ifr.html">
> 
> and if ifr.html^headers^ is:
>  
>   Content-Security-Policy: sandbox
> 
> If we update the attribute flags, then a.com learns that ifr.html did
> not have the "allow-scripts" flag. (This is not really a big deal since
> b.com can communicate with a.com in general anyway.)

ah, I see, thanks for elaborating.
 
> More importantly, we can't really change the attribute since this will
> affect future documents that are loaded in that iframe (and if it
> doesn't, but the attribute getter returns the effective sandbox flags,
> the semantics are a bit confusing IMO).

right, this is what the thread you (and Dev) pointed to explains - this makes a lot of sense to me. 
Adam Barth also clarified on twitter that the flags set via CSP sandbox don't affect the .sandbox attribute - as he put it, 'CSP sandbox and the attribute are both inputs to the sandboxing algorithm but neither is an output".
(In reply to homakov from comment #105)
> Great we finally implemented it.
> 
> But what are we gonna do with
> http://homakov.blogspot.com/2014/01/using-content-security-policy-for-evil.
> html ?

These are some good issues but don't relate to the CSP sandbox directive (as far as I can see, please correct me if I'm mistaken) so this bug isn't the best place to discuss them. Since they're spec/standard issues (as you said in your post), I'd suggest bringing them up on the W3C webappsec WG (which is responsible for the CSP spec) mailing list, see http://www.w3.org/2011/webappsec/ for details.
Technically it could be fixed in standard but there's no need to wait for it IMO. Simply fire "onlaod" event and remove report-uri directive... I mean, consider doing it first of all :) Someone should create a bug so I don't have to go this process from scratch
(In reply to Deian Stefan from comment #88)
> Do we have a bug for this? (We now need InitCSP-like functionality for
> workers.)

This is partially implemented (check out dom/workers/ScriptLoader.cpp:108). It currently uses the same policy as the parent, which was the old 1.0 behavior. This will change in 1.1, and I will make a bug for it once the spec changes had solidified a bit more.

Unfortuntately, the current implementation only enforces/reports 'unsafe-eval' against Workers. We should also support connect-src (bug 929292) and possibly other directives as well.
Flags: needinfo?(grobinson)
Comment on attachment 8357483 [details] [diff] [review]
0003-Bug-671389-Part-3-tests-for-CSP-sandbox-directive.patch

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

Sorry that it has taken a while for me to look at this.
I've had lots of admin tasks in the run up to my first week.

Generally looking good.
I think it would be worth having a test that checks that all the directive flags get reflected in the document.

Also, given that you've created a function to expose the documents flags, would it be worth using that instead of the copy of the *_c_if* tests.
It would make it easier to test various combinations as well - to make sure that neither the CSP nor iframe sandboxing overrides the other.

I've done this as feedback, as I don't think I'm the right person for the review.

::: content/html/content/test/file_iframe_sandbox_csp_c_if1.html
@@ +16,5 @@
> +
> +    document.getElementById('a_form').submit();
> +
> +    // trigger the javascript: url test
> +    sendMouseEvent({type:'click'}, 'a_link');

Would this be better before the form submit?

::: content/html/content/test/file_iframe_sandbox_csp_c_if4.html
@@ +18,5 @@
> +    sendMouseEvent({type:'click'}, 'target_blank');
> +
> +    var threw = false;
> +    try {
> +      window.open("about:blank");

If this didn't throw, wouldn't it leave a window open?

@@ +27,5 @@
> +    ok(threw, "window.open threw a JS exception and was not allowed");
> +
> +    threw = false;
> +    try {
> +      window.showModalDialog("about:blank");

Again I think the window would be left open.

::: content/html/content/test/file_iframe_sandbox_csp_c_if4.html^headers^
@@ +1,1 @@
> +Content-Security-Policy: sandbox allow-same-origin allow-scripts allow-top-navigation

Would it be worth adding allow-popups here, to be sure that it doesn't override the iframe attr?

::: content/html/content/test/test_iframe_sandbox_csp.html
@@ +50,5 @@
> +  if (a === null || b === null) { return false; }
> +  if (a.length !== b.length) { return false; }
> +  for (var i in a) {
> +    if (a[i] !== b[i]) { return false; }
> +  }

I think this relies on the order in which you are adding the strings in the SandboxFlagsToStringAttribute function.
While this will work, it might break unexpectedly, if someone changes that function.

@@ +97,5 @@
> +<iframe sandbox="" id="if_2" src="file_iframe_sandbox_csp_c_if2.html" height="10" width="10"></iframe>
> +<iframe sandbox="allow-forms allow-scripts" id="if_3" src="file_iframe_sandbox_csp_c_if3.html" height="10" width="10"></iframe>
> +<iframe sandbox="allow-same-origin allow-scripts" id="if_4" src="file_iframe_sandbox_csp_c_if4.html" height="10" width="10"></iframe>
> +
> +<iframe id="if_5" src="file_iframe_sandbox_csp_if1.html" height="10" width="10"></iframe>

I wonder if it worth keeping all of the flag checking (here and for if_7 below) in this parent document.
That way you don't need to copy the eq, getSandboxFlags and doTest functions into those documents.
Attachment #8357483 - Flags: review?(bobowencode) → feedback+
(In reply to Bob Owen (:bobowen) from comment #110)
> > +  for (var i in a) {
> > +    if (a[i] !== b[i]) { return false; }
> > +  }
> 
> I think this relies on the order in which you are adding the strings in the
> SandboxFlagsToStringAttribute function.
> While this will work, it might break unexpectedly, if someone changes that
> function.

No Bob, you're wrong ... read the for loop properly.
(In reply to Bob Owen (:bobowen) from comment #111)
> (In reply to Bob Owen (:bobowen) from comment #110)
> > I think this relies on the order in which you are adding the strings in the
> > SandboxFlagsToStringAttribute function.
> > While this will work, it might break unexpectedly, if someone changes that
> > function.
> 
> No Bob, you're wrong ... read the for loop properly.

Actually Bob, I think you'll find I'm correct (sorry for the churn).
What are we doing about report-only sandboxes?

Imagine a document with two CSPs (one report only, one enforced) both with different sandbox flags.

What do we do?  Do we bail out and apply no sandbox?  Do we send reports for sandbox violations of the one policy and enforce the other?  Do we enforce both?  This gets complicated.
Comment on attachment 8356290 [details] [diff] [review]
0001-Bug-671386-Part-1-Implement-CSP-sandbox-directive.patch

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

Waiting to finish the review until we've settled the open issue about sandbox reporting and enforcing for report-only.

::: content/base/src/CSPUtils.jsm
@@ +258,5 @@
> +  "allow-popups",
> +  "allow-same-origin",
> +  "allow-scripts",
> +  "allow-top-navigation"
> +];

Is there a way to harness the existing sandbox parser/constants instead of recapturing them here, or is it necessary to put another enum here?

@@ +1825,5 @@
> + */
> +
> +this.SandboxFlags = function SandboxFlags(flags) {
> +  this._flags = flags.sort();
> +}

Please call the class CSPSandboxFlags to follow the naming convention for other classes in this file.

@@ +1835,5 @@
> +   */
> +  toString : function() {
> +    return this._flags.join(" ");
> +  },
> +  /**

Nit: newline between }, and /**
Comment on attachment 8357482 [details] [diff] [review]
0002-Bug-671389-Part-2-Export-document-sandbox-flags-to-c.patch


>+  /**
>+   * A helper function that returns a string attribute corresponding to the
>+   * sandbox flags.
>+   *
>+   * @param flags  the sandbox flags
>+   * @param attr   the attribute corresponding to the flags (null if flags is 0)
>+   */
>+  static void SandboxFlagsToStringAttribute(uint32_t flags, nsAString& attr);
aFlags, aAttr
Though, I don't understand why "attribute".
Just SandboxFlagsToString(uint32_t aFlags, nsAString& aString)



>+/**
>+ * A helper function that returns a string attribute corresponding to the
>+ * sandbox flags.
>+ *
>+ * @param flags  the sandbox flags
>+ * @param attr   the attribute corresponding to the flags (null if flags is 0)
>+ */
>+void
>+nsContentUtils::SandboxFlagsToStringAttribute(uint32_t flags, nsAString& attr)
ditto, and add documentation only to .h


I think something like the following

>+#define IF_FLAG(flag, atom)                                 \
>+  if (!(aFlags & flag)) {                                   \
>+    if (!aString.IsEmpty()) {                               \
>+      aString.Append(NS_NAMED_LITERAL_STRING(" "));         \
>+    }                                                       \
>+    aString.Append(nsDependentAtomString(nsGkAtoms::atom)); \
>+  }

That way the result won't contain space at the end


And please add some test for the document.sandboxFlags
Attachment #8357482 - Flags: review?(bugs) → review-
Olli, Ian: thanks for the review. I addressed the comments in this patch.
Attachment #8357482 - Attachment is obsolete: true
Attachment #8361307 - Flags: review?(bugs)
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #113)
> What are we doing about report-only sandboxes?

what a great question ! ;)

IE and Chrome both have implemented CSP sandbox so as always it would be interesting to know what they do for sandbox in report-only. I can't find anything specific that discusses this situation in the spec. It may also be worth asking on the W3C webappsec list.
Ian & Bob: thanks for looking at this!

(In reply to Bob Owen (:bobowen) from comment #110)
> Generally looking good.
> I think it would be worth having a test that checks that all the directive
> flags get reflected in the document.

Added.

> Also, given that you've created a function to expose the documents flags,
> would it be worth using that instead of the copy of the *_c_if* tests.
> It would make it easier to test various combinations as well - to make sure
> that neither the CSP nor iframe sandboxing overrides the other.
> 

> ::: content/html/content/test/file_iframe_sandbox_csp_c_if1.html
> @@ +16,5 @@
> > +
> > +    document.getElementById('a_form').submit();
> > +
> > +    // trigger the javascript: url test
> > +    sendMouseEvent({type:'click'}, 'a_link');
> 
> Would this be better before the form submit?
> 
> ::: content/html/content/test/file_iframe_sandbox_csp_c_if4.html
> @@ +18,5 @@
> > +    sendMouseEvent({type:'click'}, 'target_blank');
> > +
> > +    var threw = false;
> > +    try {
> > +      window.open("about:blank");
> 
> If this didn't throw, wouldn't it leave a window open?
> 
> @@ +27,5 @@
> > +    ok(threw, "window.open threw a JS exception and was not allowed");
> > +
> > +    threw = false;
> > +    try {
> > +      window.showModalDialog("about:blank");
> 
> Again I think the window would be left open.
> 
> ::: content/html/content/test/file_iframe_sandbox_csp_c_if4.html^headers^
> @@ +1,1 @@
> > +Content-Security-Policy: sandbox allow-same-origin allow-scripts allow-top-navigation
> 
> Would it be worth adding allow-popups here, to be sure that it doesn't
> override the iframe attr?

I left these as is since they're 1-to-1 with the existing iframe
sandbox code and these tests were meant to just do the same but with
headers instead of the iframe attribute.

> ::: content/html/content/test/test_iframe_sandbox_csp.html
> @@ +50,5 @@
> > +  if (a === null || b === null) { return false; }
> > +  if (a.length !== b.length) { return false; }
> > +  for (var i in a) {
> > +    if (a[i] !== b[i]) { return false; }
> > +  }
> 
> I think this relies on the order in which you are adding the strings in the
> SandboxFlagsToStringAttribute function.
> While this will work, it might break unexpectedly, if someone changes that
> function.

Fixed.

> @@ +97,5 @@
> > +<iframe sandbox="" id="if_2" src="file_iframe_sandbox_csp_c_if2.html" height="10" width="10"></iframe>
> > +<iframe sandbox="allow-forms allow-scripts" id="if_3" src="file_iframe_sandbox_csp_c_if3.html" height="10" width="10"></iframe>
> > +<iframe sandbox="allow-same-origin allow-scripts" id="if_4" src="file_iframe_sandbox_csp_c_if4.html" height="10" width="10"></iframe>
> > +
> > +<iframe id="if_5" src="file_iframe_sandbox_csp_if1.html" height="10" width="10"></iframe>
> 
> I wonder if it worth keeping all of the flag checking (here and for if_7
> below) in this parent document.
> That way you don't need to copy the eq, getSandboxFlags and doTest functions
> into those documents.

Good suggestion, fixed to do it this way.
Attachment #8357483 - Attachment is obsolete: true
Attachment #8361344 - Flags: feedback?(bobowencode)
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #113)
> What are we doing about report-only sandboxes?
> 
> Imagine a document with two CSPs (one report only, one enforced) both with
> different sandbox flags.
> 
> What do we do?  Do we bail out and apply no sandbox?  Do we send reports for
> sandbox violations of the one policy and enforce the other?  Do we enforce
> both?  This gets complicated.

(In reply to Ian Melven :imelven from comment #117)
> (In reply to Sid Stamm [:geekboy or :sstamm] from comment #113)
> > What are we doing about report-only sandboxes?
> 
> what a great question ! ;)
> 
> IE and Chrome both have implemented CSP sandbox so as always it would be
> interesting to know what they do for sandbox in report-only. I can't find
> anything specific that discusses this situation in the spec. It may also be
> worth asking on the W3C webappsec list.

Agreed! I think these are spec-level questions. I will check what
Chrome is doing today.

My approach was to go about it as suggested in comment 2: first
implement in enforcement mode.  But I do need to check what happens
when you have a report-only and an enforce header: I suspect---and
want at least---the "stricter": enforce.
(In reply to Deian Stefan from comment #120)
> Thanks again to Dev:
> http://lists.w3.org/Archives/Public/public-webappsec/2013Jun/0118.html

That seems to imply that sandbox is ignored when in the Report-Only header - it would definitely be good to get all this in the spec...
In the patch, please add some comments clearly explaining the design decision you've made about report-only mode and sandbox.  This'll help clear up why the logic is the way it is (intersect sandboxes for non-report-only policies), and make it easier to understand.

Probably wouldn't hurt to toss out CSPWarnings when encountering sandbox in the report-only policies so that developers know it will be disregarded.

I asked because I want to make sure we do this in a sane way.  Sounds pretty sane.
The requirements in the spec are all predicated with "When enforcing the `sandbox` directive...". Report-only mode doesn't "enforce", it "monitors"... the sandbox directive should be ignored when in a report-only header. It's pretty obtusely written, though: https://github.com/w3c/webappsec/commit/2cc237a696e982be59886c8f2ba0ed2d84f22c81 should clean it up a bit.

Unfortunately, it was obtuse enough that we screwed it up in Blink: https://codereview.chromium.org/133073007 is up for review to fix our behavior.
(In reply to Deian Stefan from comment #118)

Thanks Deian.
 
> I left these as is since they're 1-to-1 with the existing iframe
> sandbox code and these tests were meant to just do the same but with
> headers instead of the iframe attribute.

I'm not quite clear what you are trying to test here then.
The comment:

|// 1-7) make sure that CSP header does not bypass iframe attribute|

seems to suggest you are testing whether the two ways of specifying sandboxing play nicely together and don't override each other's security restrictions.

The first two test files partially do this, but the second two just have extra allows in the headers, that wouldn't affect the things being attempted in the files anyway.

With the flags exposed on the document you can test different combinations on sandbox attribute and CSP and make sure the most restrictive set of flags is always applied.
The existing tests should then cover the correct behaviour once we know the flags are correct on the document.

> > I think this relies on the order in which you are adding the strings in the
> > SandboxFlagsToStringAttribute function.
> > While this will work, it might break unexpectedly, if someone changes that
> > function.
> 
> Fixed.

Thanks - trailing space crept in on the |a.sort()| line.
Comment on attachment 8361307 [details] [diff] [review]
0002-Bug-671389-Part-2-Export-document-sandbox-flags-to-c.patch

Could you change the name of the eq to 
equalFlags or some such.

+  // both attributes should be either null or have the same flags
No attributes there. just arrays of flags.

space before and after '+' in js code.
Attachment #8361307 - Flags: review?(bugs) → review+
(In reply to Mike West from comment #123)
> The requirements in the spec are all predicated with "When enforcing the
> `sandbox` directive...". Report-only mode doesn't "enforce", it
> "monitors"... the sandbox directive should be ignored when in a report-only
> header. 

Thanks Mike, but does "monitor" suggest we watch the iframe do things and then send CSP violation reports when the sandbox requirements get violated (lots of work) or that we just ignore the sandbox directive?  I'd much rather just drop the directive.
Comment on attachment 8356290 [details] [diff] [review]
0001-Bug-671386-Part-1-Implement-CSP-sandbox-directive.patch

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

Please update the patch to include my suggestions from comment 114 and comment 122, then re-flag me and grobinson for review.

grobinson: if you already started review, go ahead and finish, but I'm clearing the flag in case you haven't so you don't have to go at it again for the next patch.
Attachment #8356290 - Flags: review?(sstamm)
Attachment #8356290 - Flags: review?(grobinson)
Attachment #8356290 - Flags: review-
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #126)

> Thanks Mike, but does "monitor" suggest we watch the iframe do things and
> then send CSP violation reports when the sandbox requirements get violated
> (lots of work) or that we just ignore the sandbox directive?  I'd much
> rather just drop the directive.

Blink drops it (now). We don't have any mechanism of monitoring theoretical sandbox violations, and I don't believe we intend to add any. The spec now reads "Note that the sandbox directive will be ignored when monitoring a policy...".
Comment on attachment 8361344 [details] [diff] [review]
0003-Bug-671389-Part-3-tests-for-CSP-sandbox-directive.patch

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

Sorry, forgot to set the flag in comment 124.
Attachment #8361344 - Flags: feedback?(bobowencode) → feedback+
Thanks for the review.

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #114)
> Comment on attachment 8356290 [details] [diff] [review]
> 0001-Bug-671386-Part-1-Implement-CSP-sandbox-directive.patch
> 
> Review of attachment 8356290 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Waiting to finish the review until we've settled the open issue about
> sandbox reporting and enforcing for report-only.
> 
> ::: content/base/src/CSPUtils.jsm
> @@ +258,5 @@
> > +  "allow-popups",
> > +  "allow-same-origin",
> > +  "allow-scripts",
> > +  "allow-top-navigation"
> > +];
> 
> Is there a way to harness the existing sandbox parser/constants instead of
> recapturing them here, or is it necessary to put another enum here?

Not really. The cleanest thing I can think of is using
nsGkAtoms::allowsameorigin, etc., but as far as I know these are not
accessible to JS. Suggestions?

 
> @@ +1825,5 @@
> > + */
> > +
> > +this.SandboxFlags = function SandboxFlags(flags) {
> > +  this._flags = flags.sort();
> > +}
> 
> Please call the class CSPSandboxFlags to follow the naming convention for
> other classes in this file.

Fixed.

> @@ +1835,5 @@
> > +   */
> > +  toString : function() {
> > +    return this._flags.join(" ");
> > +  },
> > +  /**
> 
> Nit: newline between }, and /**

Fixed.

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #122)
> In the patch, please add some comments clearly explaining the design
> decision you've made about report-only mode and sandbox.  This'll help clear
> up why the logic is the way it is (intersect sandboxes for non-report-only
> policies), and make it easier to understand.
> 
> Probably wouldn't hurt to toss out CSPWarnings when encountering sandbox in
> the report-only policies so that developers know it will be disregarded.

Done both.
Attachment #8356290 - Attachment is obsolete: true
Attachment #8362078 - Flags: review?(sstamm)
Attachment #8362078 - Flags: review?(grobinson)
(In reply to Bob Owen (:bobowen) from comment #124)
> The first two test files partially do this, but the second two just have
> extra allows in the headers, that wouldn't affect the things being attempted
> in the files anyway.
> 
> With the flags exposed on the document you can test different combinations
> on sandbox attribute and CSP and make sure the most restrictive set of flags
> is always applied.
> The existing tests should then cover the correct behaviour once we know the
> flags are correct on the document.

Thanks, you are right. I updated the tests to be more exhaustive and
address this. Mind taking another look?

> Thanks - trailing space crept in on the |a.sort()| line.

Fixed.
Attachment #8361307 - Attachment is obsolete: true
Attachment #8362081 - Flags: feedback?(bobowencode)
(In reply to Deian Stefan from comment #131)
> Created attachment 8362081 [details] [diff] [review]
> 0002-Bug-671389-Part-2-Export-document-sandbox-flags-to-c.patch
> 
> (In reply to Bob Owen (:bobowen) from comment #124)
> > The first two test files partially do this, but the second two just have
> > extra allows in the headers, that wouldn't affect the things being attempted
> > in the files anyway.
> > 
> > With the flags exposed on the document you can test different combinations
> > on sandbox attribute and CSP and make sure the most restrictive set of flags
> > is always applied.
> > The existing tests should then cover the correct behaviour once we know the
> > flags are correct on the document.
> 
> Thanks, you are right. I updated the tests to be more exhaustive and
> address this. Mind taking another look?
> 
> > Thanks - trailing space crept in on the |a.sort()| line.
> 
> Fixed.

Woops, that comment was supposed to accompany this patch.

For the other patch I just addressed Olli's comment 125 and carrying r+.
Attachment #8361344 - Attachment is obsolete: true
Attachment #8362082 - Flags: feedback?(bobowencode)
Comment on attachment 8362081 [details] [diff] [review]
0002-Bug-671389-Part-2-Export-document-sandbox-flags-to-c.patch

Carrying r+ from smaug.
Attachment #8362081 - Flags: feedback?(bobowencode)
Some more tests. Also for this patch I renamed the test from _bug886164_ etc to _csp_sandbox.
Attachment #8357484 - Attachment is obsolete: true
Attachment #8357484 - Flags: review?(grobinson)
Attachment #8362083 - Flags: review?(grobinson)
Comment on attachment 8362082 [details] [diff] [review]
0003-Bug-671389-Part-3-tests-for-CSP-sandbox-directive.patch

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

(In reply to Deian Stefan from comment #131)

> Thanks, you are right. I updated the tests to be more exhaustive and
> address this. Mind taking another look?

No problem, it's good for me to start to get involved with some of the CSP stuff.

I'm still not totally sure about the copying of the other sandbox tests rather than just checking the flags on the document.
However, the *_csp_c_* tests now test that neither the attribute nor CSP can override the other for some flags, so that's good.
As, the flags are all specified together, it's reasonable to assume that the same should follow for other flags.

My reading of the spec suggests you can use CSP to sandbox any document.
Do we have tests elsewhere for documents opened in normal windows?
Should probably test old fashioned frames and <object>s loaded with an HTML document as well.
e.g. <object data="wibble.html></object>

::: content/html/content/test/file_iframe_sandbox_csp_c_if3.html
@@ +21,5 @@
> +      var thing = indexedDB.open("sandbox");
> +      ok(false, "documents sandboxed without allow-same-origin should NOT be able to access indexedDB");
> +    }
> +
> +    catch(e) {

nit: catch should come after the curly brace above.

::: content/html/content/test/file_iframe_sandbox_csp_c_if4.html
@@ +14,5 @@
> +  }
> +
> +  function doStuff() {
> +    ok(true, "a document sandboxed with allow-same-origin and allow-scripts should be same origin with its parent and able to run scripts " +
> +       "regardless of what kind of whitespace was used in its sandbox attribute");

It looks like a normal space in the attribute to me.

::: content/html/content/test/test_iframe_sandbox_csp.html
@@ +68,5 @@
> +// otherwise described here
> +
> +// fails if bad or passes ten times
> +// 1-7) make sure that CSP header does not bypass iframe attribute (first 2 frames) and vice-versa (latter 2 frames):
> +// file_iframe_sandbox_csp_c_if1.html performs the same tests as file_iframe_sandbox_c_if1.html 

nit: trailing space.
Attachment #8362082 - Flags: feedback?(bobowencode) → feedback+
Thanks, will fix.

(In reply to Bob Owen (:bobowen) from comment #135)
> I'm still not totally sure about the copying of the other sandbox tests
> rather than just checking the flags on the document.

Which ones are you referring to? I don't think we're copying any tests anymore.

The *_csp_if1.html* through *_csp_if5.html* just checks that the
document has the correct flags set.

The *_csp_if6.html* through *_csp_if10.html* ensure that
report-only-header with sandbox attribute does not affect flags -- all
it checks is the set document flags.

> My reading of the spec suggests you can use CSP to sandbox any document.
> Do we have tests elsewhere for documents opened in normal windows?
> Should probably test old fashioned frames and <object>s loaded with an HTML
> document as well.
> e.g. <object data="wibble.html></object>

Nope, I actually wanted to add this, but forgot over the iteration --
I'll add  some tests for top-level pages.
(In reply to Deian Stefan from comment #136)
> Which ones are you referring to? I don't think we're copying any tests
> anymore.

The *csp_c_* ones, I know they're not a direct copy any more.
Comment on attachment 8362078 [details] [diff] [review]
0001-Bug-671386-Part-1-Implement-CSP-sandbox-directive.patch

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

r=me with nits.  Thanks for sticking with it, this has been a bit of a slog, I know.

::: content/base/src/CSPUtils.jsm
@@ +770,5 @@
> +        continue directive;
> +      }
> +      var options = dirvalue.split(/\s+/);
> +      var flags   = [];
> +      options.forEach(function (opt) {

Maybe put a quick comment before the forEach call to summarize what this does: "add any new sandbox flags to 'flags' and warn about invalid flags"

::: content/base/src/contentSecurityPolicy.js
@@ +121,4 @@
>  }
>  
>  ContentSecurityPolicy.prototype = {
> +  classID:          Components.ID("{ac81c1ab-89f3-4887-bdf5-4a4ae327a1d8}"),

Any particular reason you're changing the Class ID here and in the Manifest?  I don't think it's necessary. The UUID bump in the IDL is necessary, though.

@@ +802,5 @@
> +    // report-only sandbox flags.
> +    for (let idx = 0; idx < this._policies.length; idx++) {
> +      let policy = this._policies[idx];
> +
> +      if (!policy.hasSandbox || policy._reportOnlyMode)  { continue; }

nit: no braces and put "continue" on its own line for consistency in this file for one-line if bodies.

::: dom/locales/en-US/chrome/security/csp.properties
@@ +57,5 @@
>  # eval is a name and should not be localized.
>  scriptFromStringBlocked = An attempt to call JavaScript from a string (by calling a function like eval) has been blocked
> +# LOCALIZATION NOTE (ignoringReportOnlyDirective):
> +# %1$S is the directive that is ignore in report-only mode.
> +ignoringReportOnlyDirective = Ignoring directive (%1$S) in report-only mode

Nit: this doesn't explain why.  Maybe "Directive %1$S is not valid in report-only mode and will be ignored."
Attachment #8362078 - Flags: review?(sstamm) → review+
Comment on attachment 8362078 [details] [diff] [review]
0001-Bug-671386-Part-1-Implement-CSP-sandbox-directive.patch

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

If we're implementing CSP sandbox as "part of CSP 1.1", then we should pref off this code (security.csp.experimentalEnabled) until we've finished implementing all of the 1.1 features and a ready to flip the pref. On other other hand, the sandbox directive is an optional part of CSP 1.0, and Chrome already has it, so I think we should just go ahead and enable it by default.

I second Sid's comment on making the console message more detailed.

::: content/base/src/CSPUtils.jsm
@@ +250,4 @@
>  CSPRep.OPTIONS_DIRECTIVE = "options";
>  CSPRep.ALLOW_DIRECTIVE   = "allow";
>  
> +// Sandbox directive for our CSP 1.1 spec compliant implementation.

Nit: s/1.1/1.0

::: content/base/src/nsDocument.cpp
@@ +2780,5 @@
>    rv = principal->SetCsp(csp);
>    NS_ENSURE_SUCCESS(rv, rv);
>  #ifdef PR_LOGGING
>    PR_LOG(gCspPRLog, PR_LOG_DEBUG,
> +         ("Inserted CSP into principal %p", principal.get()));

Why use .get(), instead of just a dereference? (just curious, not saying this is wrong)
Attachment #8362078 - Flags: review?(grobinson) → review+
(In reply to Garrett Robinson [:grobinson] from comment #139)
>
> On other other hand, the sandbox directive is an optional part of CSP 1.0, and Chrome
> already has it, so I think we should just go ahead and enable it by default.

For what it's worth, I'm +1 on this :) IE already has CSP sandbox as well, again fwiw.
Tahnks for taking a look, guys!

Carrying r+ from sstam and grobinson.

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #138)
> Any particular reason you're changing the Class ID here and in the Manifest?
> I don't think it's necessary. The UUID bump in the IDL is necessary, though.

Nope, reverted; only chaning UUID in IDL now.

(In reply to Garrett Robinson [:grobinson] from comment #139)
> Comment on attachment 8362078 [details] [diff] [review]
> 0001-Bug-671386-Part-1-Implement-CSP-sandbox-directive.patch
> 
> Review of attachment 8362078 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> If we're implementing CSP sandbox as "part of CSP 1.1", then we should pref
> off this code (security.csp.experimentalEnabled) until we've finished
> implementing all of the 1.1 features and a ready to flip the pref. On other
> other hand, the sandbox directive is an optional part of CSP 1.0, and Chrome
> already has it, so I think we should just go ahead and enable it by default.

Kept it in by default (+1 Ian's comment).

> ::: content/base/src/nsDocument.cpp
> @@ +2780,5 @@
> >    rv = principal->SetCsp(csp);
> >    NS_ENSURE_SUCCESS(rv, rv);
> >  #ifdef PR_LOGGING
> >    PR_LOG(gCspPRLog, PR_LOG_DEBUG,
> > +         ("Inserted CSP into principal %p", principal.get()));
> 
> Why use .get(), instead of just a dereference? (just curious, not saying
> this is wrong)

'principal' is a nsCOMPtr, so we need .get() to get at the underlying address.
Attachment #8362078 - Attachment is obsolete: true
Rebased.

Carrying r+ from smaug.
Attachment #8362081 - Attachment is obsolete: true
Addressed Bob's comments (thanks!) and added top-level test.
Attachment #8362082 - Attachment is obsolete: true
Attachment #8367584 - Flags: review?(grobinson)
Rebased.
Attachment #8362083 - Attachment is obsolete: true
Attachment #8362083 - Flags: review?(grobinson)
Attachment #8367586 - Flags: review?(grobinson)
Attached file example tests (obsolete) —
Deian, I did some thinking about how these tests could be changed to use your idea of exposing of the flags on the document to the full.

Hopefully this makes it really clear what the inputs to the tests are and what the expected results should be.

It uses a server side js file to return the headers we want, so it also means that you don't have to add extra files for each test case.

I put a couple of examples in there and I haven't got your patches at the moment, so I'm not totally sure they'll run properly, but hopefully you'll get the idea.
Attached patch test_iframe_sandbox_csp.html (obsolete) — Splinter Review
I'd like to replace the 0003*.patch with this, but I don't like the setTimeout; any thoughts on how to replace this?
Attachment #8377803 - Flags: feedback?(grobinson)
Attachment #8377803 - Attachment is patch: true
Attachment #8377803 - Attachment mime type: text/html → text/plain
I thought making the attachment a patch would bring up the normal review interface, but it did not. I really like the approach in test_iframe_sandbox_csp.html. When I run it, I get 7 test failures though - is that expected?

I'll need to dig in to finding a way to get rid of that setTimeout. It seems like there might be a difference between iframe.onload and iframe window/document's onload, but I'd need to set up an experiment to confirm.
> When I run it, I get 7 test failures though - is that expected?

I take that back, I didn't have the .sjs from Bob's exampleTests patch. They all pass now. I've put everything together in a new patch that cleanly applies. Calling it 0003-alt for now, in case you haven't finished merging the test cases for 0003 or something.
Attachment #8367730 - Attachment is obsolete: true
Attachment #8377803 - Attachment is obsolete: true
Attachment #8377803 - Flags: feedback?(grobinson)
(In reply to Garrett Robinson [:grobinson] from comment #148)
> Created attachment 8387966 [details] [diff] [review]
> 0003-alt-Bug-671389-Part-3-Tests-for-CSP-sandbox-directive.patch
> 
> > When I run it, I get 7 test failures though - is that expected?
> 
> I take that back, I didn't have the .sjs from Bob's exampleTests patch. They
> all pass now. I've put everything together in a new patch that cleanly
> applies. Calling it 0003-alt for now, in case you haven't finished merging
> the test cases for 0003 or something.


Thanks for taking a look. And, sorry, not sure how I missed adding the
sjs to the patch. Test wise: that's all of 003 actually, I merged all
the tests we had before.  (004- just modifies an old test, so using
Bob's suggestion won't work.)

I would like to address the setTimeout issue, so I'll spend some time
this weekend on it.
Comment on attachment 8367584 [details] [diff] [review]
0003-Bug-671389-Part-3-Tests-for-CSP-sandbox-directive.patch

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

Clearing review flag. We're dropping this patch in favor of the approach used in 0003-alt.
Attachment #8367584 - Flags: review?(grobinson)
Sorry for the delay.

This fixes the setTimeout issue (idiot mistake on my part: checkFlags did not return a function).

Additionally, it includes the top-level sandbox test.

I need to rebase all the patches, but shouldn't really affect review.

Should we move the test to `content/base/test/csp/` from `content/html/content/test`?
Attachment #8367584 - Attachment is obsolete: true
Attachment #8387966 - Attachment is obsolete: true
Attachment #8396801 - Flags: review?(grobinson)
Simplified top-level sandbox test to just check flags (need allow-scripts and allow-same-origin, unfortunately, but I tested without these manually), rebased, and moved to csp directory.
Attachment #8396801 - Attachment is obsolete: true
Attachment #8396801 - Flags: review?(grobinson)
Attachment #8399176 - Flags: review?(grobinson)
Rebased.
Attachment #8367586 - Attachment is obsolete: true
Attachment #8367586 - Flags: review?(grobinson)
Attachment #8399177 - Flags: review?(grobinson)
Comment on attachment 8399176 [details] [diff] [review]
0003-Bug-671389-Part-3-Tests-for-CSP-sandbox-directive.patch

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

Can you reuse (and extend) test/csp/file_csp_testserver.sjs instead of adding file_set_csp_headers.sjs? You would need to add the ability to set a report-only header (trivial). The existing script is more reusable because it allows you to pass a filename as a variable in the query string and it will return the contents of that file in the response.

For test_iframe_sandbox_csp_top_1.html, it would be good to add a comment explaining why it's a separate test (because you want to test the csp directive on a document that isn't an iframe, but you have to set certain sandbox flags in order to be able to run tests in a sandbox).

::: content/base/test/csp/test_iframe_sandbox_csp.html
@@ +14,5 @@
> +
> +  SimpleTest.waitForExplicitFinish();
> +
> +  function eqFlags(a, b) {
> +    // both a and b should be either null or have the array same flags

nit: clarify this comment

@@ +21,5 @@
> +    if (a.length !== b.length) {  return false; }
> +    var a_sorted = a.sort();
> +    var b_sorted = b.sort();
> +    for (var i in a_sorted) {
> +      if (a_sorted[i] !== b_sorted[i]) { console.log("FAIL D!"); return false; }

remove this debugging message, or make it clearer.

@@ +28,5 @@
> +  }
> +
> +  function getSandboxFlags(doc) {
> +    var flags = doc.sandboxFlagsAsString;
> +    if (flags === null) { return null; }

do you need this line?

@@ +40,5 @@
> +  //  - cspReportOnly: [null] or string corresponding to the CSP report-only sandbox flags
> +  // Above, we use [brackets] to denote default values.
> +  function CSPFlagsTest(desc, opts) {
> +    function ifundef(x, v) {
> +      return (x !== undefined) ? x : null;

I think you meant s/null/v

@@ +169,5 @@
> +    }
> +  ].map(function(t) { return (new CSPFlagsTest(t.desc,t)); });
> +
> +
> +  var testCaseIndex = -1;

nit: I would init to 0, and increment it after you call runTest on the testCase (clearer IMHO).
Attachment #8399176 - Flags: review?(grobinson) → review-
Comment on attachment 8399177 [details] [diff] [review]
0004-Bug-671389-Part-4-Extend-CSP-tests-for-iframe-sandbo.patch

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

These tests could be significantly DRY'ed out using file_csp_testserver.js. You'll still need file_csp_sandbox_[1-12].html, but you can get rid of the ^headers^ files. Pass the header and .html file name to the testserver, and it will return a response with that file's contents and the given header. This will also allow you to consolidate more useful info in window.tests. You should also be able to create and load the iframes using loops instead of individually, with a similar list of tests objects like you are using in the other patch.

The tests themselves look great! If we can just clean up the framework a bit, this will be ready to land.

::: content/base/test/csp/test_csp_sandbox.html
@@ +87,5 @@
> +
> +// a postMessage handler that is used by sandboxed iframes without
> +// 'allow-same-origin' to communicate pass/fail back to this main page.
> +// it expects to be called with an object like {ok: true/false, desc:
> +// <description of the test> which it then forwards to ok()

Nit: missing closing }. Also, this comment could be more clearly formatted/written. Can you answer the question of "why?" (why do we need a postMessage handler at all?) That is IMHO generally useful for anybody who looks at this code later.

@@ +184,5 @@
> +  // ... otherwise, finish
> +  window.examiner.remove();
> +  cspTestsDone = true;
> +  if (iframeSandboxTestsDone) {
> +    SimpleTest.finish();

You also need to call window.examiner.remove(). I'd recommend making a little cleanup() function, since you need to do the same thing in the mirror case (iframeSandboxTests finish after cspTests).
Attachment #8399177 - Flags: review?(grobinson) → review-
Addressed all the comments. If there is a dummy file that we use for iframes then we should be also able to get rid of file_iframe_sandbox_csp.html.
Attachment #8399176 - Attachment is obsolete: true
Attachment #8401556 - Flags: review?(grobinson)
Forgot file_iframe_sandbox_csp.html. Sorry about that.
Attachment #8401556 - Attachment is obsolete: true
Attachment #8401556 - Flags: review?(grobinson)
Attachment #8401557 - Flags: review?(grobinson)
Addressed comments. Much cleaner :)
Attachment #8399177 - Attachment is obsolete: true
Attachment #8401663 - Flags: review?(grobinson)
Comment on attachment 8401557 [details] [diff] [review]
0003-Bug-671389-Part-3-Tests-for-CSP-sandbox-directive.patch

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

r=me if you either get rid of allow-forms for test_iframe_sandbox_csp_top_1.html, or explain why you need it.

::: content/base/test/csp/file_csp_testserver.sjs
@@ +40,5 @@
>    // Deliver the CSP policy encoded in the URI
>    response.setHeader("Content-Security-Policy", csp, false);
>  
> +  // Deliver the CSP report-only policy encoded in the URI
> +  response.setHeader("Content-Security-Policy-Report-Only", cspRO, false);

It'd be nice to gate this setHeader and the previous one, e.g. "if (csp) { response.setHeader(...) }" so the behavior is explicit. However, since we ignore empty CSP headers, this shouldn't cause any issues.

::: content/base/test/csp/test_iframe_sandbox_csp_top_1.html
@@ +10,5 @@
> +Since we need to load the SimpleTest files, we have to set the
> +allow-same-origin flag. Additionally, we set the allow-scripts flag
> +since we need JS to check the flags.
> +
> +CSP header: Content-Security-Policy: sandbox allow-forms allow-scripts allow-same-origin

Why do you need allow-forms?
Attachment #8401557 - Flags: review?(grobinson) → review+
(In reply to Garrett Robinson [:grobinson] from comment #159)
> It'd be nice to gate this setHeader and the previous one, e.g. "if (csp) {
> response.setHeader(...) }" so the behavior is explicit. However, since we
> ignore empty CSP headers, this shouldn't cause any issues.

Agreed, we should do this for csp, cspRO and file.

> Why do you need allow-forms?

We don't. I just added allow-forms to make sure that *some other flag*
is correctly set. (Really, we may want to extend this to more
flags/permutation or just remove allow-forms.)
Comment on attachment 8401663 [details] [diff] [review]
0004-Bug-671389-Part-4-Extend-CSP-tests-for-iframe-sandbo.patch

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

r=me with comments addressed. Great work, Deian!

::: content/base/test/csp/file_csp_sandbox_pass.js
@@ +1,2 @@
> +function ok(result, desc) {
> +  window.parent.postMessage({ok: result, desc: desc}, "*");

Just a note, I don't think you need to set "*" - the test (parent of the iframe that loads this) is guaranteed to be at http://mochi.test:8888 (see http://dxr.mozilla.org/mozilla-central/source/build/pgo/server-locations.txt). You don't need to change this because it doesn't actually affect anything :)

::: content/base/test/csp/test_csp_sandbox.html
@@ +121,5 @@
> +var completedTests = 0;
> +var passedTests = 0;
> +
> +var totalTests = (function() {
> +    var nrTests = 0;

Nit: more descriptive variable name?

@@ +125,5 @@
> +    var nrTests = 0;
> +    for(var i = 0; i < testCases.length; i++) {
> +      nrTests += Object.keys(testCases[i].results).length;
> +    }
> +    return nrTests+12;

Why +12? Magic number needs a comment.

@@ +230,5 @@
> +    // Run tests:
> +    for(var i = 0; i < testCases.length; i++) {
> +      runTest(testCases[i]);
> +    }
> +

Nit: extra newline
Attachment #8401663 - Flags: review?(grobinson) → review+
I think at this point all the pieces of this have r+ ? Time for a try run ? :)
Mislabeled as bug 671386. Sorry.
Attachment #8402270 - Attachment is obsolete: true
Seems like we both ran try (https://tbpl.mozilla.org/?tree=Try&rev=43a6e39f5cb8).

Anyway, finally had a chance to look at the B2G failures. Indeed the
sandbox flag is not enforced in the browser app when coming from the
CSP header, but it seems like this is not the only thing that is not
enforced. Specifically, I just tested a simple CSP policy (default-src
'self') to see if inline scripts are blocked, and they are not. Is
this expected?
Flags: needinfo?(grobinson)
(In reply to Deian Stefan from comment #169)

> Is this expected?

Definitely not, that warrants scrutiny. Can you test if the CSP is being created correctly (in nsDocument::InitCSP)? There was a bug a while back that had to do with conflicts between CSP from headers and CSP from a manifest, which may have regressed.

The other possible problem is that the 1.0 parser isn't on by default on B2G. Your tests set the .speccompliant pref, but there are some other issues with 1.0 on B2G that are being tracked in bug 858787, and could be causing problems here.
Flags: needinfo?(grobinson)
(In reply to Garrett Robinson [:grobinson] from comment #170)
> (In reply to Deian Stefan from comment #169)
> 
> > Is this expected?
> 
> Definitely not, that warrants scrutiny. Can you test if the CSP is being
> created correctly (in nsDocument::InitCSP)? There was a bug a while back
> that had to do with conflicts between CSP from headers and CSP from a
> manifest, which may have regressed.

The speccompliant pref was off, sorry about that (didn't realize it
wasn't on by default until you mentioned).
  
> The other possible problem is that the 1.0 parser isn't on by default on
> B2G. Your tests set the .speccompliant pref, but there are some other issues
> with 1.0 on B2G that are being tracked in bug 858787, and could be causing
> problems here.

I tested different flags manually and things seem to be fine on B2G, I
think this may be a mochitest issue.

Looking at various mochitest.ini files it seems like a lot of iframe
sandbox tests are skipped since they fail on B2G desktop.

See, e.g.,: http://dxr.mozilla.org/mozilla-central/source/content/html/content/test/mochitest.ini?from=content/html/content/test/mochitest.ini#430

Thoughts on how to proceed?
fyi, we're seeing a lot of weirdness with frames on B2G (e.g. bug 1006781 and bug bug 1009632)
Priority: P3 → P1
Is anyone still working on this? I believe this is blocking CSP1.1 compliance?

It seems the change to C++ CSP code means that the actual patch is no longer valid and a whole new patch is needed. Are the tests still valid though?
(pressed submit too soon)

I meant to say .. "I am hopeful the tests can still be reused"
(In reply to Devdatta Akhawe [:devd] from comment #174)
> (pressed submit too soon)
> 
> I meant to say .. "I am hopeful the tests can still be reused"

I don't see why they couldn't be. Seems like this stalled out because of B2G tests - it looks like some of that has been resolved now from looking at some of the linked bugs and their linked bugs. But yeah, this would need to be ported to the new parser.
(In reply to Ian Melven :imelven from comment #175)
> I don't see why they couldn't be. Seems like this stalled out because of B2G
> tests - it looks like some of that has been resolved now from looking at
> some of the linked bugs and their linked bugs. But yeah, this would need to
> be ported to the new parser.

Deian, are you still working on this? Otherwise I can take over and integrate into the new parser.
Flags: needinfo?(deian)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #176)
> (In reply to Ian Melven :imelven from comment #175)
> > I don't see why they couldn't be. Seems like this stalled out because of B2G
> > tests - it looks like some of that has been resolved now from looking at
> > some of the linked bugs and their linked bugs. But yeah, this would need to
> > be ported to the new parser.
> 
> Deian, are you still working on this? Otherwise I can take over and
> integrate into the new parser.

If you want, we can also chat and I can just assist you when integrating your changes into the new C++ backend of CSP.
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #177)
> (In reply to Christoph Kerschbaumer [:ckerschb] from comment #176)
> > (In reply to Ian Melven :imelven from comment #175)
> > > I don't see why they couldn't be. Seems like this stalled out because of B2G
> > > tests - it looks like some of that has been resolved now from looking at
> > > some of the linked bugs and their linked bugs. But yeah, this would need to
> > > be ported to the new parser.
> > 
> > Deian, are you still working on this? Otherwise I can take over and
> > integrate into the new parser.
> 
> If you want, we can also chat and I can just assist you when integrating
> your changes into the new C++ backend of CSP.

Sorry for the delayed response -- been super busy lately. I think
that I can spend some time on this next week (towards the end of the
week). If that sounds reasonable for you guys I'm happy to keep
hacking on it -- otherwise please feel free to take over. (Either way,
I'd be happy to review any patches).
Flags: needinfo?(deian)
(In reply to Deian Stefan from comment #178)
> (In reply to Christoph Kerschbaumer [:ckerschb] from comment #177)
> > (In reply to Christoph Kerschbaumer [:ckerschb] from comment #176)
> > > (In reply to Ian Melven :imelven from comment #175)
> > > > I don't see why they couldn't be. Seems like this stalled out because of B2G
> > > > tests - it looks like some of that has been resolved now from looking at
> > > > some of the linked bugs and their linked bugs. But yeah, this would need to
> > > > be ported to the new parser.
> > > 
> > > Deian, are you still working on this? Otherwise I can take over and
> > > integrate into the new parser.
> > 
> > If you want, we can also chat and I can just assist you when integrating
> > your changes into the new C++ backend of CSP.
> 
> Sorry for the delayed response -- been super busy lately. I think
> that I can spend some time on this next week (towards the end of the
> week). If that sounds reasonable for you guys I'm happy to keep
> hacking on it -- otherwise please feel free to take over. (Either way,
> I'd be happy to review any patches).

That's great, go for it. I am on PTO next week, but feel free to ping me whenever you need help finding things in the new parser.
Sorry for the delay with this. One thing that is a bit ugly with this patch is that we're concatenating the source lists (actually sandbox flags) to just use the existing parser. There may a be a cleaner way to do this, but I figure I'd run this by you first.
Attachment #8402274 - Attachment is obsolete: true
Attachment #8515783 - Flags: review?(mozilla)
Carrying r+ from smaug. This patch is just rebasing old implementation.
Attachment #8402271 - Attachment is obsolete: true
Carrying r+ from grobinson. Again, just rebase.
Attachment #8402272 - Attachment is obsolete: true
Carrying r+ from grobinson. Again, just rebase.
Attachment #8402273 - Attachment is obsolete: true
Comment on attachment 8515783 [details] [diff] [review]
0001-Bug-671389-Part-1-Implement-CSP-sandbox-directive.patch

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

Hey Deian, overall looks pretty good to me. Thanks for working on this. Clearing the review-flag because I would like to see it another time please. In general it would be great to have solid tests for that, one set of tests that uses all of the different flags available for the sandbox directive.

::: dom/base/nsCSPContext.cpp
@@ +1105,5 @@
> +NS_IMETHODIMP
> +nsCSPContext::ShouldSandbox(nsAString& outSandboxFlags, bool* outShouldSandbox)
> +{
> +  if (outShouldSandbox == nullptr) {
> +    return NS_ERROR_FAILURE;

I don't think you need that check, the String ref can never be a nullptr.

@@ +1111,5 @@
> +  *outShouldSandbox = false;
> +  outSandboxFlags.Truncate();
> +  for (uint32_t i = 0; i < mPolicies.Length(); i++) {
> +    *outShouldSandbox = mPolicies[i]->getSandboxFlags(outSandboxFlags);
> +    if (*outShouldSandbox) {

Maybe it's easier if you flip the statements around.
if (!*outShouldSandbox) {
 continue;
}

I guess you want continue right? Not return NS_OK like you do in the else branch. Otherwise you would just inspect the first policy, but I assume you want to inspect all of the policies for sandbox flags.

@@ +1115,5 @@
> +    if (*outShouldSandbox) {
> +      if (mPolicies[i]->getReportOnlyFlag()) {
> +        // sandbox directive ignored in report-only mode
> +        *outShouldSandbox = false;
> +        outSandboxFlags.Truncate();

Does *outShouldSandbox ever contain something else than 'false'; same with outSandboxFlags - they are initialized at the top but only overwritten with the same values.

@@ +1119,5 @@
> +        outSandboxFlags.Truncate();
> +
> +        nsAutoString policy;
> +        mPolicies[i]->toString(policy);
> +        const char16_t* params[] = { policy.get() };

Nit: Move the params argument closer to CSP_LogLocalizedStr.

::: dom/base/nsCSPParser.cpp
@@ +748,5 @@
>  
> +// Helper function for parsing sandbox flags. This function solely
> +// concatenates all the source list tokens (the sandbox flags) so the
> +// attribute parser (nsContentUtils::ParseSandboxAttributeToFlags) can
> +// use.

Nit, if you add a comment on top of a method, please use:
/*
 * ...
 */

@@ +756,5 @@
> +  nsAutoString flags;
> +
> +  // This is a bit ugly since we're reversing the work of the CSP
> +  // tokenizer, but simpler than keeping track of the consistency of
> +  // two parsers.

You can remove that comment.

@@ +759,5 @@
> +  // tokenizer, but simpler than keeping track of the consistency of
> +  // two parsers.
> +  //
> +  // remember, srcs start at index 1
> +  for (uint32_t i = 1; i < mCurDir.Length(); i++) {

Please use
uint32_t len = mCurDir.Length();
and then use len in the for-loop avoding to always call the expensive .Length() function.

@@ +766,5 @@
> +    CSPPARSERLOG(("nsCSPParser::sandboxFlagList, mCurToken: %s, mCurValue: %s",
> +                 NS_ConvertUTF16toUTF8(mCurToken).get(),
> +                 NS_ConvertUTF16toUTF8(mCurValue).get()));
> +
> +    if (CSP_IsValidSandboxFlag(mCurToken)) {

Please flip the cases around.
if (!CSP_IsValidSandboxFlag(mCurToken)) {
  print warning, and continue;

}

@@ +768,5 @@
> +                 NS_ConvertUTF16toUTF8(mCurValue).get()));
> +
> +    if (CSP_IsValidSandboxFlag(mCurToken)) {
> +      flags.Append(mCurToken);
> +      if (i != mCurDir.Length() - 1) {

Also here you can then use
if (i != (length -1) {
...
}

@@ +773,5 @@
> +        flags.AppendASCII(" ");
> +      }
> +    } else {
> +      const char16_t* params[] = { mCurToken.get() };
> +      logWarningErrorToConsole(nsIScriptError::warningFlag, "ignoringUnknownOption",

Maybe we should add "couldntParseInvalidSandboxToken" to csp.propoerties which is probably more descriptive than ignoringUnkonwOption.

@@ +795,4 @@
>    if (CSP_IsDirective(mCurDir[0], CSP_REPORT_URI)) {
>      reportURIList(outSrcs);
>      return;
> +  } else if (CSP_IsDirective(mCurDir[0], CSP_SANDBOX)) {

Nit: Please don't use an else if here, because it's not an either ... or ...
Please add a comment and then use:
if (CSP_IsDirective(...) {
 ...
}

::: dom/base/nsCSPParser.h
@@ +130,4 @@
>      bool subHost();                                       // helper function to parse subDomains
>      bool subPath(nsCSPHostSrc* aCspHost);                 // helper function to parse paths
>      void reportURIList(nsTArray<nsCSPBaseSrc*>& outSrcs); // helper function to parse report-uris
> +    void sandboxFlagList(nsTArray<nsCSPBaseSrc*>& outSrcs); // helper function to parse sandbox flags

Super Nit: Please align the // helper comments above with your comment.

::: dom/base/nsCSPUtils.cpp
@@ +619,5 @@
>  
> +/* ===== nsCSPSandboxFlags ===================== */
> +
> +nsCSPSandboxFlags::nsCSPSandboxFlags(const nsAString& aFlags)
> +  :mFlags(aFlags)

Nit: Space between : and mFlags

::: dom/base/nsContentUtils.cpp
@@ +1238,5 @@
>   * a CSP directive) and converts it to the set of flags used internally.
>   *
> + * NOTE: if new sandbox flags are added to the spec, CSPUtils.h should be
> + * updated as well.
> + *

Super nit: Please add the Note at the end of the comment, after the arguments.

::: dom/base/nsDocument.cpp
@@ +2968,5 @@
> +      // Reset the document principal to a null principal
> +      principal = do_CreateInstance("@mozilla.org/nullprincipal;1");
> +      SetPrincipal(principal);
> +    }
> +  }

For that part we do need to have Sid take a look at it and then also a DOM peer. Leaving that part for the second round of reviews.
Attachment #8515783 - Flags: review?(mozilla)
Just a quick note, whenever you carry over the r+ from someone when uploading a new patch, please also set that flag in the 'details' of the patch. It's a great way of indicating what patches are already r+ by just looking at the patches on top of the bug. Thanks!
I've run our browser security testing tool (https://browseraudit.com, currently under development) on the latest Firefox 34 beta with these patches (making minor changes to file paths) and it flags an error to do with the way the patched browser handles the empty sandbox directive.

After reading the CSP specification, we included a test to check that a Content-Security-Policy header with the value "sandbox" --- i.e., with no flags following the directive --- is interpreted as a request from the server that the browser add a sandbox attribute with an empty value to every iframe on the page. This leads to the most restrictive environment possible inside the iframe. (This seems to be the way Chromium interprets the CSP spec, as this is the behaviour I see in Chromium 40.0.2205.0.)

However, with these Firefox patches, this Content-Security-Policy header is interpreted as if the sandbox directive was never included at all; effectively, no sandbox attribute is added to any iframe on the page, leading to the most permissive environment possible inside the iframe.

Which do you think is the "correct" behaviour?
@chris this is great! What is the correct behavior is really in the iframe sandbox spec and not in CSP? What happens with a similar, nested test but inside a sandbox iframe? If the behavior between what happens with iframes and what happens with the CSP header is different, then this is a bug in this patch. If not, then that is a separate bug in the implementation of iframe sandbox and you should file it!
Thanks very much for taking a look! I think I addressed most comments
in the updated patch. This new patch also takes sandbox directives
across multiple headers into consideration.

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #184)
> ::: dom/base/nsDocument.cpp
> @@ +2968,5 @@
> > +      // Reset the document principal to a null principal
> > +      principal = do_CreateInstance("@mozilla.org/nullprincipal;1");
> > +      SetPrincipal(principal);
> > +    }
> > +  }
> 
> For that part we do need to have Sid take a look at it and then also a DOM
> peer. Leaving that part for the second round of reviews.

Sid r+ this part before, but sounds reasonable to me.
Attachment #8515783 - Attachment is obsolete: true
Attachment #8521123 - Flags: review?(mozilla)
(In reply to Chris Novakovic from comment #186)
> I've run our browser security testing tool (https://browseraudit.com,
> currently under development) on the latest Firefox 34 beta with these
> patches (making minor changes to file paths) and it flags an error to do
> with the way the patched browser handles the empty sandbox directive.
> 
> After reading the CSP specification, we included a test to check that a
> Content-Security-Policy header with the value "sandbox" --- i.e., with no
> flags following the directive --- is interpreted as a request from the
> server that the browser add a sandbox attribute with an empty value to every
> iframe on the page. This leads to the most restrictive environment possible
> inside the iframe. (This seems to be the way Chromium interprets the CSP
> spec, as this is the behaviour I see in Chromium 40.0.2205.0.)
> 
> However, with these Firefox patches, this Content-Security-Policy header is
> interpreted as if the sandbox directive was never included at all;
> effectively, no sandbox attribute is added to any iframe on the page,
> leading to the most permissive environment possible inside the iframe.
> 
> Which do you think is the "correct" behaviour?

The Chromium behavior sounds right IIRC from reading the spec (will double check though).

Chris: I've tested this myself before and have not actually run into the issue that you are describing. Can you upload an example that is failing?
Flags: needinfo?(chris)
I've built some minimum working examples (essentially the same as the failing tests on browseraudit.com, but with all of the reporting mechanisms stripped away) that should help make it clearer what's happening:

http://www.doc.ic.ac.uk/~cnovakov/csp-sandbox-test/attribute.html - web page containing an iframe with an empty "sandbox" attribute; No "Content-Security-Policy" HTTP header is served alongside the page. The iframe embeds a web page that attempts to execute a script:

Chromium 40.0.2205.0: script execution blocked, with console output: "Blocked script execution in 'iframe.html' because the document's frame is sandboxed and the 'allow-scripts' permission is not set."
Firefox 33.x: script execution blocked, with no console output
Firefox 34.0b6 with Deian's patches: script execution blocked, with no console output

http://www.doc.ic.ac.uk/~cnovakov/csp-sandbox-test/cspheader.cgi - web page containing an iframe; "Content-Security-Policy: sandbox" HTTP header is served alongside the page. The iframe embeds a web page that attempts to execute a script:

Chromium 40.0.2205.0: script execution blocked, with console output: "Blocked script execution in 'iframe.html' because the document's frame is sandboxed and the 'allow-scripts' permission is not set."
Firefox 33.x: script execution allowed, with console output: "Content Security Policy: Failed to parse unrecognised source sandbox"
Firefox 34.0b6 with Deian's patches: script execution allowed, with console output: "Content Security Policy: Failed to parse unrecognized source sandbox"

It therefore seems that Firefox does the right thing when the "sandbox" attribute is explicitly added to an iframe, but doesn't when the empty sandbox policy is implied by the "sandbox" directive in the CSP HTTP header (modulo my question about whether this is the behaviour expected by the CSP spec: I think it probably is, and the Chromium guys interpret it that way too).

As far as the reason for this behaviour is concerned, it could be that when Firefox parses CSP HTTP headers, it always expects directives to have one or more tokens immediately following it (that's certainly true until you consider this edge-case). This theory is backed up by the console error message: it seems that a "source" is expected to follow "sandbox". I'm afraid I don't know enough about Firefox's internals to know what to do about this, though, sorry.

If you'd like further information, or if you want me to test a revised patchset, feel free to drop me a needinfo.
Flags: needinfo?(chris)
(In reply to Chris Novakovic from comment #190)
> If you'd like further information, or if you want me to test a revised
> patchset, feel free to drop me a needinfo.

No, this is perfect, thanks!
(In reply to Deian Stefan from comment #191)
> (In reply to Chris Novakovic from comment #190)
> > If you'd like further information, or if you want me to test a revised
> > patchset, feel free to drop me a needinfo.
> 
> No, this is perfect, thanks!

Mind running the test with the updated patch please? [Server is down ATM.]
We have tests for this and I seprately implemented this myself to double check.

And, thanks for looking into this!
Flags: needinfo?(chris)
(In reply to Deian Stefan from comment #192)
> Mind running the test with the updated patch please?

For some reason the updated patches, although applying cleanly to the Firefox 34 betas after modifying the same file paths as in the previous patch, break compilation: which branch should I actually be applying these patches to? (Alternatively, you could take a look at the test results at browseraudit.com for your patched version: the relevent tests are in "Content Security Policy -> sandbox".)
Flags: needinfo?(chris) → needinfo?(deian)
Comment on attachment 8521123 [details] [diff] [review]
0001-Bug-671389-Part-1-Implement-CSP-sandbox-directive.patch

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

Looks good to me. We still need a reviewer for the nsDocument bits.
Please address my comments and r=me.
Great Stuff!

Also, Bug 1089912 moved the CSP code into dom/security so you have to rebase your patches.

::: dom/base/nsCSPContext.cpp
@@ +1105,5 @@
> +nsCSPContext::GetCSPSandboxFlags(uint32_t *outSandboxFlags)
> +{
> +  if (outSandboxFlags == nullptr) {
> +    return NS_ERROR_FAILURE;
> +  }

You can remove that check.

@@ +1106,5 @@
> +{
> +  if (outSandboxFlags == nullptr) {
> +    return NS_ERROR_FAILURE;
> +  }
> +  *outSandboxFlags = 0;

Would it make sense to add a flag here:
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsSandboxFlags.h#14
instead of using 0?

nsCSPPolicy::getSandboxFlags() could also return that flag as default instead of 0.

::: dom/base/nsCSPUtils.cpp
@@ +1000,5 @@
> +nsCSPPolicy::getSandboxFlags() const
> +{
> +  for (uint32_t i = 0; i < mDirectives.Length(); i++) {
> +    if (mDirectives[i]->equals(CSP_SANDBOX)) {
> +      nsAutoString flags;

Nit: Please move the declaration of the string outside of the loop and just use
flags.Truncate() inside the loop.
Attachment #8521123 - Flags: review?(mozilla) → review+
Another very minor thing I noticed in 0001-Bug-671389-Part-1-Implement-CSP-sandbox-directive.patch:

> +# LOCALIZATION NOTE (couldntParseInvalidSandboxFlag):
> +# %1$S is the option that could not be understood
> +couldntParseInvalidSandboxFlag = Coudn't parse invalid sandbox flag %1$S

s/Coudn't/Couldn't/
Flags: needinfo?(deian)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #194)
> Comment on attachment 8521123 [details] [diff] [review]
> 0001-Bug-671389-Part-1-Implement-CSP-sandbox-directive.patch
> 
> Review of attachment 8521123 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me. We still need a reviewer for the nsDocument bits.
> Please address my comments and r=me.

Thanks for looking at this. (And, thanks Chris) Will ask smaug or bz. Addressed most.

> ::: dom/base/nsCSPContext.cpp
> @@ +1105,5 @@
> > +nsCSPContext::GetCSPSandboxFlags(uint32_t *outSandboxFlags)
> > +{
> > +  if (outSandboxFlags == nullptr) {
> > +    return NS_ERROR_FAILURE;
> > +  }
> 
> You can remove that check.

I left this since we call this function in nsDocument from C++ land,
so it is possible for it to be nullptr.

> @@ +1106,5 @@
> > +{
> > +  if (outSandboxFlags == nullptr) {
> > +    return NS_ERROR_FAILURE;
> > +  }
> > +  *outSandboxFlags = 0;
> 
> Would it make sense to add a flag here:
> http://mxr.mozilla.org/mozilla-central/source/dom/base/nsSandboxFlags.h#14
> instead of using 0?
> 
> nsCSPPolicy::getSandboxFlags() could also return that flag as default
> instead of 0.

I like this. Added SANDBOXED_NONE.
Attachment #8521123 - Attachment is obsolete: true
Attachment #8523223 - Flags: review+
rebase & removing some trailing spaces
Attachment #8515784 - Attachment is obsolete: true
Attachment #8523224 - Flags: review+
rebase & rm trailing spaces
Attachment #8515785 - Attachment is obsolete: true
Attachment #8523226 - Flags: review+
rebase & rm trailing spaces
Attachment #8515786 - Attachment is obsolete: true
Attachment #8523227 - Flags: review+
Try with previous patch:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=dafa8f8b0efa

Burned because of unsused static variable. This patch moved it to the .cpp file.

Try with this patch:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=3044323fcd2f
Attachment #8523223 - Attachment is obsolete: true
Attachment #8524131 - Flags: review?(bugs)
Comment on attachment 8524131 [details] [diff] [review]
0001-Bug-671389-Part-1-Implement-CSP-sandbox-directive-r-.patch

>@@ -2954,11 +2954,30 @@ nsDocument::InitCSP(nsIChannel* aChannel)
>     }
>   }
> 
>+  // ----- Set sandbox flags according to CSP header
>+  // The document may already have some sandbox flags set (e.g., if the
>+  // document is an iframe with the sandbox attribute set).  If we have a CSP
>+  // sandbox directive, intersect the CSP sandbox flags with the existing
>+  // flags.  This corresponds to the _least_ permissive policy.
>+  uint32_t cspSandboxFlags = SANDBOXED_NONE;
>+  nsAutoString strFlags;
You don't ever use this variable.


>+# LOCALIZATION NOTE (couldntParseInvalidSandboxFlag):
>+# %1$S is the option that could not be understood
>+couldntParseInvalidSandboxFlag = Couldn't parse invalid sandbox flag %1$S
couldNotParseInvalidSandboxFlag might be a tiny bit easier to review

>+nsCSPContext::GetCSPSandboxFlags(uint32_t *outSandboxFlags)
uint32_t* aOutSandboxFlags

(* goes with the type, and arguments start with 'a')

>+static const char* CSPStrSandboxFlags[] = {
>+  "allow-same-origin",
>+  "allow-forms",
>+  "allow-scripts",
>+  "allow-top-navigation",
>+  "allow-pointer-lock",
>+  "allow-popups"
>+};
We really shouldn't have a copy of the list here.
Somehow move http://hg.mozilla.org/mozilla-central/annotate/084441e904d1/dom/base/nsContentUtils.cpp#l1262 outside the
.cpp file and then in CSP_IsValidSandboxFlag just compare the atom values to aFlag.
(nsIAtom has Equals method)

>+nsCSPSandboxFlags::toString(nsAString& outStr) const
aOutStr

>+nsCSPPolicy::getSandboxFlags() const
>+{
>+  nsAutoString flags;
>+  for (uint32_t i = 0; i < mDirectives.Length(); i++) {
>+    if (mDirectives[i]->equals(CSP_SANDBOX)) {
Is it possible to have several sandbox directives?
(If so, what the behavior should be) 

>+class nsCSPSandboxFlags : public nsCSPBaseSrc {
{ goes to its own line.


r- because I don't know the answer to "Is it possible to have several sandbox directives?" and because of CSPStrSandboxFlags[].
Attachment #8524131 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #201) 
> >+nsCSPContext::GetCSPSandboxFlags(uint32_t *outSandboxFlags)
> uint32_t* aOutSandboxFlags
> 
> (* goes with the type, and arguments start with 'a')

I agree that the * goes with the type, but in general we prefix outgoing arguments with out. I think he is following that  coding style practiced which we also use throughout CSP. I think 'outSandboxFlags' should be ok to use.
 
> >+static const char* CSPStrSandboxFlags[] = {
> >+  "allow-same-origin",
> >+  "allow-forms",
> >+  "allow-scripts",
> >+  "allow-top-navigation",
> >+  "allow-pointer-lock",
> >+  "allow-popups"
> >+};
> We really shouldn't have a copy of the list here.
> Somehow move
> http://hg.mozilla.org/mozilla-central/annotate/084441e904d1/dom/base/
> nsContentUtils.cpp#l1262 outside the
> .cpp file and then in CSP_IsValidSandboxFlag just compare the atom values to
> aFlag.
> (nsIAtom has Equals method)

I think that's reasonable if easy to do. If possible we should try to avoid a duplication of that list.

> >+nsCSPPolicy::getSandboxFlags() const
> >+{
> >+  nsAutoString flags;
> >+  for (uint32_t i = 0; i < mDirectives.Length(); i++) {
> >+    if (mDirectives[i]->equals(CSP_SANDBOX)) {
> Is it possible to have several sandbox directives?
> (If so, what the behavior should be) 

If we encounter a duplicate directive during parsing, we ignore the second directive and its source-expressions.
See http://mxr.mozilla.org/mozilla-central/source/dom/security/nsCSPParser.cpp#798
To answer your question, it's not possible to have several sandbox directives. That part looks good to me.
What about multiple policies, each with a sandbox directive?
Thanks both, sorry for the trivial stuff.

(In reply to Olli Pettay [:smaug] from comment #201) 
> couldNotParseInvalidSandboxFlag might be a tiny bit easier to review

Agreed, but I left as is since the rest of the CSP errors use couldnt
instead of couldNot.

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #202)
> (In reply to Olli Pettay [:smaug] from comment #201) 
> > >+nsCSPContext::GetCSPSandboxFlags(uint32_t *outSandboxFlags)
> > uint32_t* aOutSandboxFlags
> > 
> > (* goes with the type, and arguments start with 'a')
> 
> I agree that the * goes with the type, but in general we prefix outgoing
> arguments with out. I think he is following that  coding style practiced
> which we also use throughout CSP. I think 'outSandboxFlags' should be ok to
> use.

Attached is my attempt to address this. Let me know what you think.

> What about multiple policies, each with a sandbox directive?

Handled, takes union -- most restricting.
Attachment #8524131 - Attachment is obsolete: true
Attachment #8524991 - Flags: feedback?(bugs)
(In reply to Devdatta Akhawe [:devd] from comment #203)
> What about multiple policies, each with a sandbox directive?

That's ok, we can handle multiple policies each defining a sandbox (or whatever) directive, see:
http://mxr.mozilla.org/mozilla-central/source/dom/security/nsCSPContext.cpp#169
but one policy is not allowed to define the same directive twice. We also log a warning to the console in such a case. Thanks Dev for following this bug!
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #202)
but in general we prefix outgoing
> arguments with out.
That is then not following Mozilla coding style. But if rest of the code in that files uses
out*, fine.
Comment on attachment 8524991 [details] [diff] [review]
0001-Bug-671389-Part-1-Implement-CSP-sandbox-directive-r-.patch

Christoph, mind taking a look at this also? Changed enough to be worth a glance.
Attachment #8524991 - Flags: review?(mozilla)
Comment on attachment 8524991 [details] [diff] [review]
0001-Bug-671389-Part-1-Implement-CSP-sandbox-directive-r-.patch

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

Hey Deian, thanks for rewriting/updating. I like this approach better. You still need smaug to review everything besides the CSP bits, r=me on those. Please don't forget to update the reviewers in the patch comment before landing. I left some comments, mostly nits.

::: dom/interfaces/security/nsIContentSecurityPolicy.idl
@@ +210,5 @@
> +    * Delegate method called by the service when the protected document is loaded.
> +    * Returns the most restricting sandbox flags contained in CSP policies.
> +    *
> +    * @return
> +    *    sandbox flags or 0 if no sandbox directive exists

Nit: maybe you can add a link to the different flags available that might be returned here.

::: dom/security/nsCSPContext.cpp
@@ +1132,5 @@
> +nsCSPContext::GetCSPSandboxFlags(uint32_t* aOutSandboxFlags)
> +{
> +  if (aOutSandboxFlags == nullptr) {
> +    return NS_ERROR_FAILURE;
> +  }

As last time, you don't need that check :-). It's not an issue, look at all the other methods in this file, do they use that check? Please remove.

::: dom/security/nsCSPParser.cpp
@@ +762,5 @@
> +  nsAutoString flags;
> +
> +  uint32_t length = mCurDir.Length();
> +  // remember, srcs start at index 1
> +  for (uint32_t i = 1; i < length; i++) {

You can use mCurDir.Length() in the loop header. If I requested that change initially than I was probably biased by reviewing JS code. No need to query the length outside the loop header.

::: dom/security/nsCSPUtils.cpp
@@ +1034,5 @@
> +
> +      return nsContentUtils::ParseSandboxAttributeToFlags(&attr);
> +    }
> +  }
> +  return 0;

Nit: Can we use SANDBOXED_NONE here as default return value?
Attachment #8524991 - Flags: review?(mozilla) → review+
Addressed all the comments. Left the nullptr check as per our IRC discussion.
Attachment #8524991 - Attachment is obsolete: true
Attachment #8524991 - Flags: feedback?(bugs)
Attachment #8528127 - Flags: review?(bugs)
Line no's changed because of first patch.
Attachment #8523224 - Attachment is obsolete: true
Attachment #8528128 - Flags: review+
Comment on attachment 8528127 [details] [diff] [review]
0001-Bug-671389-Part-1-Implement-CSP-sandbox-directive-r-.patch


s/SandboxFlagAttrs/sSandboxFlagAttrs/
and 
s/SandboxFlagValues/sSandboxFlagValues/

>+
>+  uint32_t len = (sizeof(SandboxFlagAttrs) / sizeof(SandboxFlagAttrs[0]));
Couldn't you use ArrayLength(SandboxFlagAttrs) here? Same also in nsContentUtils::IsValidSandboxFlag

>+nsContentUtils::IsValidSandboxFlag(const nsAString& aFlag)
>+{
>+  uint32_t len = (sizeof(SandboxFlagAttrs) / sizeof(SandboxFlagAttrs[0]));
>+  for (uint32_t i = 0; i < len; i++) {
>+    if ((*SandboxFlagAttrs[i])->Equals(aFlag)) {
Do we need to care about caseness here.
In the previous patch you had
(aFlag.LowerCaseEqualsASCII(CSPStrSandboxFlags[i]))

>+  /**
>+   * A helper function that checks if a string matches a valid sandbox
>+   * flag.
>+   *
>+   * @param aFlag  the potential sandbox flag
>+   * @return       true if the flag is a sandbox flag
>+   */
>+  static bool IsValidSandboxFlag(const nsAString& aFlag);

So the comment should say whether the method does case-insensitive check or case-sensitive.
(and we need to have tests for both cases)
>+# LOCALIZATION NOTE (couldntParseInvalidSandboxFlag):
>+# %1$S is the option that could not be understood
>+couldntParseInvalidSandboxFlag = Couldn't parse invalid sandbox flag %1$S
I think 'couldNotParseInvalidSandboxFlag' would be easier to read in code.

I could take still one look.
Attachment #8528127 - Flags: review?(bugs) → review-
Thanks for taking a look!

(In reply to Olli Pettay [:smaug] from comment #211)
> Comment on attachment 8528127 [details] [diff] [review]
> 0001-Bug-671389-Part-1-Implement-CSP-sandbox-directive-r-.patch
> 
> 
> s/SandboxFlagAttrs/sSandboxFlagAttrs/
> and 
> s/SandboxFlagValues/sSandboxFlagValues/

Fixed.

> >+  uint32_t len = (sizeof(SandboxFlagAttrs) / sizeof(SandboxFlagAttrs[0]));
> Couldn't you use ArrayLength(SandboxFlagAttrs) here? Same also in
> nsContentUtils::IsValidSandboxFlag

Sorry, this was using the convention used in the CSP code. Fixed.

> >+nsContentUtils::IsValidSandboxFlag(const nsAString& aFlag)
> >+{
> >+  uint32_t len = (sizeof(SandboxFlagAttrs) / sizeof(SandboxFlagAttrs[0]));
> >+  for (uint32_t i = 0; i < len; i++) {
> >+    if ((*SandboxFlagAttrs[i])->Equals(aFlag)) {
> Do we need to care about caseness here.
> In the previous patch you had
> (aFlag.LowerCaseEqualsASCII(CSPStrSandboxFlags[i]))
> 
> >+  /**
> >+   * A helper function that checks if a string matches a valid sandbox
> >+   * flag.
> >+   *
> >+   * @param aFlag  the potential sandbox flag
> >+   * @return       true if the flag is a sandbox flag
> >+   */
> >+  static bool IsValidSandboxFlag(const nsAString& aFlag);
> 
> So the comment should say whether the method does case-insensitive check or
> case-sensitive.

Thanks for catching this. Should be case-insensitive. Fixed.

> (and we need to have tests for both cases)
> >+# LOCALIZATION NOTE (couldntParseInvalidSandboxFlag):
> >+# %1$S is the option that could not be understood
> >+couldntParseInvalidSandboxFlag = Couldn't parse invalid sandbox flag %1$S
> I think 'couldNotParseInvalidSandboxFlag' would be easier to read in code.

I left this as is -- all the other CSP error use couldnt.
Attachment #8528127 - Attachment is obsolete: true
Attachment #8528486 - Flags: review?(bugs)
(In reply to Olli Pettay [:smaug] from comment #211)
> So the comment should say whether the method does case-insensitive check or
> case-sensitive.
> (and we need to have tests for both cases)
Make sure the tests cover lowercase, uppercase and mixed case
(In reply to Olli Pettay [:smaug] from comment #213)
> (In reply to Olli Pettay [:smaug] from comment #211)
> > So the comment should say whether the method does case-insensitive check or
> > case-sensitive.
> > (and we need to have tests for both cases)
> Make sure the tests cover lowercase, uppercase and mixed case

Yep, updating other patches that contain the tests.
Comment on attachment 8528486 [details] [diff] [review]
0001-Bug-671389-Part-1-Implement-CSP-sandbox-directive-r-.patch

>+  uint32_t len = (sizeof(sSandboxFlagAttrs) / sizeof(sSandboxFlagAttrs[0]));
ArrayLength

>+bool
>+nsContentUtils::IsValidSandboxFlag(const nsAString& aFlag)
>+{
>+  for (uint32_t i = 0; i < ArrayLength(sSandboxFlagAttrs); i++) {
>+    if (EqualsIgnoreASCIICase(nsDependentAtomString(*sSandboxFlagAttrs[i]), aFlag)) {
aFlag.EqualsIgnoreCase(nsDependentAtomString(*sSandboxFlagAttrs[i]))
would be more common.

>+  if (cspSandboxFlags & SANDBOXED_ORIGIN) {
>+    // If the new CSP sandbox flags do not have the allow-same-origin flag
>+    // reset the document principal to a null principal
>+    principal = do_CreateInstance("@mozilla.org/nullprincipal;1");
>+    SetPrincipal(principal);
>+  }
So this part should work just fine with document.open()/write()/close()
but please add a test.
So, test how document.open()/write()/close() work after a sandboxed document has been loaded.
(document.open()/write()/close() needs to happen after load event)
This testing is really must have thing.

>+      const char16_t* params[] = { policy.get() };
>+      CSP_LogLocalizedStr(NS_ConvertUTF8toUTF16("ignoringReportOnlyDirective").get(),
Wouldn't NS_LITERAL_STRING("ignoringReportOnlyDirective").get() work here?
Or even just  MOZ_UTF16("ignoringReportOnlyDirective")
Attachment #8528486 - Flags: review?(bugs) → review+
Attachment #8528486 - Attachment is obsolete: true
Attachment #8528749 - Flags: review+
Added test for document.write() that checks if document principals is the same, before and after the write.

New addition is: file_iframe_sandbox_csp_document_write.html
Attachment #8523226 - Attachment is obsolete: true
Attachment #8528751 - Flags: review?(bugs)
Attachment #8528751 - Flags: review?(bugs) → review+
Rebase.
Attachment #8528749 - Attachment is obsolete: true
Attachment #8529280 - Flags: review+
Rebased.
Attachment #8528751 - Attachment is obsolete: true
Attachment #8529282 - Flags: review+
Deian, looks like this is ready to go ?
(In reply to Ian Melven :imelven from comment #220)
> Deian, looks like this is ready to go ?

Almost!

Try is here: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c133c0502644

Looks like we need to go back to sizeof vs. ArrayLength since the latter can't be statically asserted.
Flags: needinfo?(bugs)
nsCSPParser.cpp:772:10: error: 'nsContentUtils

Sounds like a missing #include

And if static_assert cause issues for other cases, just use MOZ_ASSERT.
ArrayLength is really a lot easier to read than sizeof/sizeof hacks.
Flags: needinfo?(bugs)
Fixed,thanks.

Try seems good on windows:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d9f1ceeced72

This patch fixes the burning linux debug (was missing include).
Attachment #8529280 - Attachment is obsolete: true
Attachment #8530468 - Flags: review+
Looks like a failing iframe sandbox test in mochitest-2 : 

1063 INFO TEST-UNEXPECTED-FAIL | /tests/dom/html/test/test_iframe_sandbox_general.html | default sandbox attribute should be an empty string - Result logged after SimpleTest.finish()
1064 INFO TEST-UNEXPECTED-FAIL | /tests/dom/html/test/test_iframe_sandbox_general.html | documents sandboxed with allow-forms should be able to submit forms - Result logged after SimpleTest.finish()
1065 INFO TEST-UNEXPECTED-FAIL | /tests/dom/html/test/test_iframe_sandbox_general.html | documents sandboxed with allow-same-origin SHOULD be able to access indexedDB - Result logged after SimpleTest.finish()

A lot of purple in Windows 8 x64 debug...
Deian: is this ready to land?  it looks all reviewed...
Component: DOM: Core & HTML → DOM: Security
Flags: needinfo?(deian)
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #225)
> Deian: is this ready to land?  it looks all reviewed...

Ready to land in terms of reviews, but a failing test on try, see comment 224 - not sure if the orange is related ? Need a try push and to fix any test errors it causes (if they're actually caused by these changes).
Flags: needinfo?(deian)
(In reply to Ian Melven :imelven from comment #226)
> (In reply to Sid Stamm [:geekboy or :sstamm] from comment #225)
> > Deian: is this ready to land?  it looks all reviewed...
> 
> Ready to land in terms of reviews, but a failing test on try, see comment
> 224 - not sure if the orange is related ? Need a try push and to fix any
> test errors it causes (if they're actually caused by these changes).

I'll rebase and look into it today.
Attachment #8530468 - Attachment is obsolete: true
Attachment #8554069 - Flags: review+
Attachment #8529282 - Attachment is obsolete: true
Attachment #8554071 - Flags: review+
Attachment #8523227 - Attachment is obsolete: true
Attachment #8554072 - Flags: review+
Attachment #8554070 - Flags: review+
Try still failing. Seems likethe issue is with the new test. I'll dig into the changes, but it's a bit hard to debug this since the mochitest is passing on my machine. Any suggestions?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f63fb6af4c70
Flags: needinfo?(mozilla)
(In reply to Deian Stefan from comment #232)
> Try still failing. Seems likethe issue is with the new test. I'll dig into
> the changes, but it's a bit hard to debug this since the mochitest is
> passing on my machine. Any suggestions?
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=f63fb6af4c70

Try to wrap the cleanup into a separate function, that should fix it, see e.g.:
http://mxr.mozilla.org/mozilla-central/source/dom/base/test/csp/test_nonce_source.html?force=1#61
Flags: needinfo?(mozilla)
Just added 'explicity' to the nsCSPSandboxFlags constructor.
Attachment #8554069 - Attachment is obsolete: true
Attachment #8558966 - Flags: review+
Miscounted number of tests, so try failed because SimpleTest.finish() was called too early. This was a trivial fix. My apologies.

New try is good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a144cdbdc6a7
Attachment #8554070 - Attachment is obsolete: true
Attachment #8558970 - Flags: review+
Fixes comment to test count.
Attachment #8558970 - Attachment is obsolete: true
Attachment #8558973 - Flags: review+
Keywords: checkin-needed
:D Very exciting ! 

Thanks again for all your patience and persistence on this one, Deian !
woot! this is awesome!
Thanks for persisting with this, Devdatta/Deian - it was definitely worth it!
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Confirmed green post-backout.
Depends on: 1129953
:(

It seems we have have seen issues with b2g tests and this patch in the past (see prev comments). IMHO, the sandbox directive is pretty powerful and important (Firefox is the only browser that doesn't support it). @deian do you have time to take a look at the test failures? If the failures are too complicated to fix, maybe we should disable CSP sandbox support for b2g? It seems a bunch of iframe sandbox tests are already skipped on b2g per comments above.
I'm running try on b2g now. It's a bit hard to tell from the log exactly what's going on/how this breaks. I can track it down when I have more time, but maybe the easiest thing to move forward is create a new bug for b2g and disable it for b2g for now as you're suggesting.
So some data on how this breaks.

It breaks because the object passed to the JSAutoCompartment ctor from the DeserializeArrayBuffer in TCPSocketChild is null.  The caller is TCPSocketChild::RecvCallback which is passing mWindowObj.

So where does mWindowObj come from?  It comes from TCPSocketChild::SetSocketAndWindow or TCPSocketChild::SendOpen which both do:

  mWindowObj = js::CheckedUnwrap(&aWindowObj.toObject());

So here's a question.  Are you possibly changing the principal of a document after it's corresponding Window has already been created?  If so, I would not be terribly surprised if we get weird security check failures after that point...
And in particular, what you may want to do is add logging to those two TCPSocketChild functions to see whether the CheckedUnwrap is in fact returning null.  And if so, log the principal of aWindowObj, as well as the principal of the thing UncheckedUnwrap returns.
(In reply to Boris Zbarsky [:bz] from comment #247)
> And in particular, what you may want to do is add logging to those two
> TCPSocketChild functions to see whether the CheckedUnwrap is in fact
> returning null.  And if so, log the principal of aWindowObj, as well as the
> principal of the thing UncheckedUnwrap returns.

Thanks, Boris. We are setting the document principal (seee https://bugzilla.mozilla.org/attachment.cgi?id=8558966&action=diff#a/dom/base/nsDocument.cpp_sec3). This is helpful. I will dig into this as soon as I can.
(In reply to Deian Stefan from comment #248)
> (In reply to Boris Zbarsky [:bz] from comment #247)
> > And in particular, what you may want to do is add logging to those two
> > TCPSocketChild functions to see whether the CheckedUnwrap is in fact
> > returning null.  And if so, log the principal of aWindowObj, as well as the
> > principal of the thing UncheckedUnwrap returns.
> 
> Thanks, Boris. We are setting the document principal (seee
> https://bugzilla.mozilla.org/attachment.cgi?id=8558966&action=diff#a/dom/
> base/nsDocument.cpp_sec3). This is helpful. I will dig into this as soon as
> I can.

You can't just change the principal of a document without reloading it or coordinating with the existing window - otherwise we have no way to update the principal on the JSCompartment. Does that happen somewhere?
Deian, how are things? You got so close in landing this one, any plan/chance you want to pick up that work again and get it landed? Or would you rather like me to find someone who is going to finish that up for you?
Flags: needinfo?(deian)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #250)
> Deian, how are things? You got so close in landing this one, any plan/chance
> you want to pick up that work again and get it landed? Or would you rather
> like me to find someone who is going to finish that up for you?

I would love too, but unfortunately have too much on my plate. If there is someone that can help that would be greatly appreciated.
Flags: needinfo?(deian)
@bholley any pointers to how we would reload or update the principal on the JSCompartment? Kinda hard to repro the b2g failures in local development.
(In reply to Devdatta Akhawe [:devd] from comment #252)
> @bholley any pointers to how we would reload or update the principal on the
> JSCompartment? Kinda hard to repro the b2g failures in local development.

Bobby, it seems Dev is willing to make some progress on that bug. Can you provide some pointers?
Flags: needinfo?(bobbyholley)
> any pointers to how we would reload or update the principal on the JSCompartment

I don't believe there are any provisions for doing that right now, since nothing in the web platform up until this point has required it.  In particular, the nsIPrincipal is just inheriting from JSPrincipals, and as far as Gecko is concerned what the JS engine does with JSPrincipals is a black box.  They might hold pointers to them in various places or whatnot...

So this is infrastructure that would need to be added somehow.  How hard that is really depends on SpiderMonkey implementation details.  Now in practice it looks to me like JSScript just gets its principal from the compartment, so that's pretty hopeful.

So you will need to do the following, at least: 

1)  Add some JSAPI or jsfriendapi to change the principals of an existing compartment.
2)  When doing the CSP sandbox thing, change the principal of the window's compartment,
    not just that of the document.  And then do some of the dance that
    nsPrincipal::SetDomain does in terms of recomputing wrappers.
3)  Audit whether there are other places in SpiderMonkey that are not the compartment that
    hold on to principals.
4)  Figure out how this should work when document.open() creates a new Window: does it
    inherit the sandboxing from the document that open() got called for, or is it not
    sandboxed, or is this not a situation we need to worry about because sandboxing means
    no one except the sandboxed document can do that anyway?  Whatever is decided here,
    make the spec say that.
Taking a step back here - are we sure we actually want to update the principal on the compartment? That's a pretty drastic thing to do, and I would really rather avoid it if at all possible.

Is there a reason we can't do this work before nsGlobalWindow::SetNewDocument is called?
Flags: needinfo?(bobbyholley)
Duplicate of this bug: 1223383
My theory is that, yes it should be doable because we do it for the iframe sandbox case. I am finding it a bit hard to follow through exactly where SetNewDocument is called and where StartDocumentLoad is called (which then calls InitCSP). It seems like we need to call this initCSP before SetNewDocument is called.

@deian what do you think? You are more familiar with the patch. I am happy to do the grunt work on this, but would appreciate your help with the changes.
Flags: needinfo?(deian)
> I am finding it a bit hard to follow through exactly where SetNewDocument is called

It's called from nsDocumentViewer::InitInternal as part of document viewer setup.  This, in turn, would typically be called from nsDocShell::SetupNewViewer.  You should be able to add things there, or to its single caller (Embed) as needed, right?

StartDocumentLoad is called before this, typically from nsContentDLF::CreateDocument which is what creates the nsIContentViewer.

So I would typically expect InitCSP to happen before SetNewDocument...
I'm happy to take a look at this next week and just try to get things working with this new information from Boris and Bobby. @devd: reasonable?
Flags: needinfo?(deian)
sounds great! thanks!