671389 - Implement CSP sandbox directive

Status: RESOLVED FIXED

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

Description

[Eli Grey (:sephr)]

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.

Comment 1

[Eli Grey (:sephr)]

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

Comment 2

[Devdatta Akhawe [:devd]]

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.

Comment 3

[Devdatta Akhawe [:devd]]

Attachment: csp-sandbox-enforcement

Implements CSP Sandbox directive in enforcement mode only

Comment 4

[Devdatta Akhawe [:devd]]

This patch requires that the patches for bug 341604 be imported first.

Comment 5

[Devdatta Akhawe [:devd]]

Webkit doesn't support a report only mode for sandbox either; and I conjecture that IE doesn't either.

Comment 6

[Devdatta Akhawe [:devd]]

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 7

[Ian Melven :imelven]

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

Comment 8

[Devdatta Akhawe [:devd]]

Attachment: Merging imelven's feedback

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

Comment 9

[Ian Melven :imelven]

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

Comment 10

[Devdatta Akhawe [:devd]]

Attachment: added support for policy intersection

Asking for geekboy's review, since he is familiar with the CSP code.

Comment 11

[Sid Stamm [:geekboy or :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).

Comment 12

[Devdatta Akhawe [:devd]]

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

Comment 13

[Devdatta Akhawe [:devd]]

Attachment: Implement CSP sandbox directive support

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?

Comment 14

[Devdatta Akhawe [:devd]]

Attachment: Implement CSP sandbox directive support

I was mistakenly attaching the flag to the docshell instead of the document. This is now fixed.

Comment 15

[Devdatta Akhawe [:devd]]

Attachment: Implement CSP sandbox directive support

Moved initCSP back down, since it doesn't need to be moved any more.

Comment 16

[Devdatta Akhawe [:devd]]

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 17

[Devdatta Akhawe [:devd]]

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.

Comment 18

[Ian Melven :imelven]

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

Comment 19

[Devdatta Akhawe [:devd]]

Attachment: Strawman MakeNull patch

Comment 20

[Devdatta Akhawe [:devd]]

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.

Comment 21

[Boris Zbarsky [:bzbarsky]]

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.

Comment 22

[Devdatta Akhawe [:devd]]

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?

Comment 23

[Devdatta Akhawe [:devd]]

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.

Comment 24

[Boris Zbarsky [:bzbarsky]]

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

Comment 25

[Devdatta Akhawe [:devd]]

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.

Comment 26

[Boris Zbarsky [:bzbarsky]]

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

Comment 27

[Ian Melven :imelven]

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

Comment 28

[Devdatta Akhawe [:devd]]

> How are you going from outer to inner for the _argument_ to equals/subsumes?

duh .. yes. thanks for pointing this out.

Comment 29

[Devdatta Akhawe [:devd]]

Attachment: Patch to add support for making a principal null at runtime

Comment 30

[Devdatta Akhawe [:devd]]

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?

Comment 31

[Boris Zbarsky [: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)...

Comment 32

[Devdatta Akhawe [:devd]]

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

Comment 33

[Boris Zbarsky [:bzbarsky]]

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

Comment 34

[Boris Zbarsky [:bzbarsky]]

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

Comment 35

[Devdatta Akhawe [:devd]]

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.

Comment 36

[Ian Melven :imelven]

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

Comment 37

[Tanvi Vyas[:tanvi]]

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

Comment 38

[Devdatta Akhawe [:devd]]

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?

Comment 39

[Ian Melven :imelven]

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

Comment 40

[Devdatta Akhawe [:devd]]

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.

Comment 41

[Boris Zbarsky [:bzbarsky]]

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.

Comment 42

[Devdatta Akhawe [:devd]]

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 43

[Devdatta Akhawe [:devd]]

Attachment: Move CSP code to before principal reset

Comment 44

[Devdatta Akhawe [:devd]]

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.

Comment 45

[Ian Melven :imelven]

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 ?

Comment 46

[Devdatta Akhawe [:devd]]

Attachment: Move CSP code to before principal reset

Comment 47

[Devdatta Akhawe [:devd]]

Attachment: Add support for CSP Sandbox

Comment 48

[Devdatta Akhawe [:devd]]

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

Comment 49

[Boris Zbarsky [:bzbarsky]]

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.

Comment 50

[Devdatta Akhawe [:devd]]

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!

Comment 51

[Boris Zbarsky [:bzbarsky]]

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

Comment 52

[Devdatta Akhawe [:devd]]

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

Comment 53

[Devdatta Akhawe [:devd]]

Attachment: Move CSP code to before principal reset

Comment 54

[Boris Zbarsky [: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 55

[Boris Zbarsky [:bzbarsky]]

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.

Comment 56

[Devdatta Akhawe [:devd]]

Attachment: Move CSP code to before principal reset

Comment 57

[Devdatta Akhawe [:devd]]

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 58

[Boris Zbarsky [:bzbarsky]]

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.

Comment 59

[Devdatta Akhawe [:devd]]

Attachment: Move CSP code to before principal reset

Comment 60

[Devdatta Akhawe [:devd]]

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

Comment 61

[Boris Zbarsky [:bzbarsky]]

nullptr, please.

Comment 62

[Devdatta Akhawe [:devd]]

Attachment: Move CSP code to before principal reset

use nullptr instead of null

Comment 63

[Devdatta Akhawe [:devd]]

Attachment: Add support for CSP Sandbox

Comment 64

[Devdatta Akhawe [:devd]]

Attachment: Add support for CSP Sandbox

Comment 65

[Devdatta Akhawe [:devd]]

Comment on attachment 659276 [details] [diff] [review]
Move CSP code to before principal reset

this patch obsoleted by bug 790747

Comment 67

[Deian Stefan]

Attachment: 0001-Bug-671386-Part-1-Implement-CSP-sandbox-directive.patch

Comment 68

[Deian Stefan]

Attachment: 0002-Bug-671386-Part-2-tests-for-CSP-sandbox-directive-ge.patch

Comment 69

[Deian Stefan]

Attachment: 0003-Bug-671389-Part-3-tests-for-CSP-sandbox-directive-in.patch

Comment 70

[Deian Stefan]

Attachment: 0004-Bug-671389-Part-4-tests-for-CSP-sandbox-directive-wo.patch

Comment 71

[Deian Stefan]

Attachment: 0005-Bug-671389-Part-5-tests-for-CSP-sandbox-directive-sa.patch

Comment 72

[Deian Stefan]

Attachment: 0006-Bug-671389-Part-6-tests-for-CSP-sandbox-directive-na.patch

Comment 73

[Deian Stefan]

Attachment: 0007-Bug-671389-Part-7-tests-for-CSP-sandbox-directive-pl.patch

Comment 74

[Deian Stefan]

Attachment: 0008-Bug-671389-Part-8-tests-for-CSP-sandbox-directive-in.patch

Comment 75

[Deian Stefan]

Attachment: 0009-Bug-671389-Part-9-tests-for-CSP-sandbox-directive.patch

Comment 76

[Ian Melven :imelven]

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 ?

Comment 77

[Bob Owen (:bobowen)]

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

Comment 78

[Ian Melven :imelven]

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

[Ian Melven :imelven]

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)

Comment 80

[Ian Melven :imelven]

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 81

[Ian Melven :imelven]

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

Comment 82

[Ian Melven :imelven]

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.

Comment 83

[Ian Melven :imelven]

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.

Comment 84

[Deian Stefan]

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.

Comment 85

[Ian Melven :imelven]

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

Comment 86

[Deian Stefan]

Attachment: 0001-Bug-671386-Part-1-Implement-CSP-sandbox-directive.patch

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

Comment 87

[Deian Stefan]

References: <[e-mail]/> <[e-mail]/>
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...

Comment 88

[Deian Stefan]

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

Comment 89

[Deian Stefan]

Attachment: 0002-Bug-671389-Part-2-Export-document-sandbox-flags-to-c.patch

Comment 90

[Deian Stefan]

Attachment: 0003-Bug-671389-Part-3-tests-for-CSP-sandbox-directive.patch

Comment 91

[Deian Stefan]

Attachment: 0004-Bug-671389-Part-4-Extend-CSP-tests-for-iframe-sandbo.patch

Comment 92

[Deian Stefan]

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?

Comment 93

[Deian Stefan]

> 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

Comment 94

[Ian Melven :imelven]

Deian, I'll try to get to this and do another pass this weekend :)

Comment 95

[Ian Melven :imelven]

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

Comment 96

[Ian Melven :imelven]

(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/[e-mail]/msg30133.html is the thread that contains the discussion and reasoning at the time if you're curious :)

Comment 97

[Ian Melven :imelven]

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

[Ian Melven :imelven]

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

Comment 99

[Ian Melven :imelven]

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

[Ian Melven :imelven]

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 ?

Comment 101

[Deian Stefan]

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

Comment 102

[Deian Stefan]

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

Comment 103

[Deian Stefan]

(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/[e-mail]/msg30133.html is the
> thread that contains the discussion and reasoning at the time if you're
> curious :)


Thanks!

Comment 104

[Ian Melven :imelven]

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

Comment 105

[homakov]

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 ?

Comment 106

[Ian Melven :imelven]

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

Comment 107

[Ian Melven :imelven]

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

Comment 108

[homakov]

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

Comment 109

[Garrett Robinson [:grobinson]]

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

Comment 110

[Bob Owen (:bobowen)]

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.

Comment 111

[Bob Owen (:bobowen)]

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

Comment 112

[Bob Owen (:bobowen)]

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

Comment 113

[Sid Stamm [:geekboy or :sstamm]]

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 114

[Sid Stamm [:geekboy or :sstamm]]

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 115

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

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

Comment 116

[Deian Stefan]

Attachment: 0002-Bug-671389-Part-2-Export-document-sandbox-flags-to-c.patch

Olli, Ian: thanks for the review. I addressed the comments in this patch.

Comment 117

[Ian Melven :imelven]

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

Comment 118

[Deian Stefan]

Attachment: 0003-Bug-671389-Part-3-tests-for-CSP-sandbox-directive.patch

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.

Comment 119

[Deian Stefan]

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

Comment 120

[Deian Stefan]

Thanks again to Dev: http://lists.w3.org/Archives/Public/public-webappsec/2013Jun/0118.html

Comment 121

[Ian Melven :imelven]

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

Comment 122

[Sid Stamm [:geekboy or :sstamm]]

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.

Comment 123

[Mike West]

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.

Comment 124

[Bob Owen (:bobowen)]

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

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

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.

Comment 126

[Sid Stamm [:geekboy or :sstamm]]

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

[Sid Stamm [:geekboy or :sstamm]]

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.

Comment 128

[Mike West]

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

[Bob Owen (:bobowen)]

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.

Comment 130

[Deian Stefan]

Attachment: 0001-Bug-671386-Part-1-Implement-CSP-sandbox-directive.patch

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.

Comment 131

[Deian Stefan]

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

Comment 132

[Deian Stefan]

Attachment: 0003-Bug-671389-Part-3-tests-for-CSP-sandbox-directive.patch

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

Comment 133

[Deian Stefan]

Comment on attachment 8362081 [details] [diff] [review]
0002-Bug-671389-Part-2-Export-document-sandbox-flags-to-c.patch

Carrying r+ from smaug.

Comment 134

[Deian Stefan]

Attachment: 0004-Bug-671389-Part-4-Extend-CSP-tests-for-iframe-sandbo.patch

Some more tests. Also for this patch I renamed the test from _bug886164_ etc to _csp_sandbox.

Comment 135

[Bob Owen (:bobowen)]

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.

Comment 136

[Deian Stefan]

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.

Comment 137

[Bob Owen (:bobowen)]

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

[Sid Stamm [:geekboy or :sstamm]]

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

Comment 139

[Garrett Robinson [:grobinson]]

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)

Comment 140

[Ian Melven :imelven]

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

Comment 141

[Deian Stefan]

Attachment: 0001-Bug-671386-Part-1-Implement-CSP-sandbox-directive.patch

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.

Comment 142

[Deian Stefan]

Attachment: 0002-Bug-671389-Part-2-Export-document-sandbox-flags-to-c.patch

Rebased.

Carrying r+ from smaug.

Comment 143

[Deian Stefan]

Attachment: 0003-Bug-671389-Part-3-Tests-for-CSP-sandbox-directive.patch

Addressed Bob's comments (thanks!) and added top-level test.

Comment 144

[Deian Stefan]

Attachment: 0004-Bug-671389-Part-4-Extend-CSP-tests-for-iframe-sandbo.patch

Rebased.

Comment 145

[Bob Owen (:bobowen)]

Attachment: example tests

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.

Comment 146

[Deian Stefan]

Attachment: test_iframe_sandbox_csp.html

I'd like to replace the 0003*.patch with this, but I don't like the setTimeout; any thoughts on how to replace this?

Comment 147

[Garrett Robinson [:grobinson]]

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.

Comment 148

[Garrett Robinson [:grobinson]]

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

Comment 149

[Deian Stefan]

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

[Garrett Robinson [:grobinson]]

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.

Comment 151

[Deian Stefan]

Attachment: 0003-Bug-671389-Part-3-Tests-for-CSP-sandbox-directive.patch

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

Comment 152

[Deian Stefan]

Attachment: 0003-Bug-671389-Part-3-Tests-for-CSP-sandbox-directive.patch

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.

Comment 153

[Deian Stefan]

Attachment: 0004-Bug-671389-Part-4-Extend-CSP-tests-for-iframe-sandbo.patch

Rebased.

Comment 154

[Garrett Robinson [: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).

Comment 155

[Garrett Robinson [:grobinson]]

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

Comment 156

[Deian Stefan]

Attachment: 0003-Bug-671389-Part-3-Tests-for-CSP-sandbox-directive.patch

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.

Comment 157

[Deian Stefan]

Attachment: 0003-Bug-671389-Part-3-Tests-for-CSP-sandbox-directive.patch

Forgot file_iframe_sandbox_csp.html. Sorry about that.

Comment 158

[Deian Stefan]

Attachment: 0004-Bug-671389-Part-4-Extend-CSP-tests-for-iframe-sandbo.patch

Addressed comments. Much cleaner :)

Comment 159

[Garrett Robinson [: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?

Comment 160

[Deian Stefan]

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

[Garrett Robinson [:grobinson]]

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

Comment 162

[Ian Melven :imelven]

I think at this point all the pieces of this have r+ ? Time for a try run ? :)

Comment 163

[Deian Stefan]

Attachment: 0001-Bug-671386-Part-1-Implement-CSP-sandbox-directive-r-.patch

Comment 164

[Deian Stefan]

Attachment: 0002-Bug-671389-Part-2-Export-document-sandbox-flags-to-c.patch

Comment 165

[Deian Stefan]

Attachment: 0003-Bug-671389-Part-3-Tests-for-CSP-sandbox-directive-r-.patch

Comment 166

[Deian Stefan]

Attachment: 0004-Bug-671389-Part-4-Extend-CSP-tests-for-iframe-sandbo.patch

Comment 167

[Deian Stefan]

Attachment: 0001-Bug-671389-Part-1-Implement-CSP-sandbox-directive-r-.patch

Mislabeled as bug 671386. Sorry.

Comment 168

[Garrett Robinson [:grobinson]]

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

Comment 169

[Deian Stefan]

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?

Comment 170

[Garrett Robinson [: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.

Comment 171

[Deian Stefan]

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

Comment 172

[Garrett Robinson [:grobinson]]

fyi, we're seeing a lot of weirdness with frames on B2G (e.g. bug 1006781 and bug bug 1009632)

Comment 173

[Devdatta Akhawe [:devd]]

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?

Comment 174

[Devdatta Akhawe [:devd]]

(pressed submit too soon)

I meant to say .. "I am hopeful the tests can still be reused"

Comment 175

[Ian Melven :imelven]

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

Comment 176

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 177

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 178

[Deian Stefan]

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

Comment 179

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 180

[Deian Stefan]

Attachment: 0001-Bug-671389-Part-1-Implement-CSP-sandbox-directive.patch

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.

Comment 181

[Deian Stefan]

Attachment: 0002-Bug-671389-Part-2-Export-document-sandbox-flags-to-c.patch

Carrying r+ from smaug. This patch is just rebasing old implementation.

Comment 182

[Deian Stefan]

Attachment: 0003-Bug-671389-Part-3-Tests-for-CSP-sandbox-directive-r-.patch

Carrying r+ from grobinson. Again, just rebase.

Comment 183

[Deian Stefan]

Attachment: 0004-Bug-671389-Part-4-Extend-CSP-tests-for-iframe-sandbo.patch

Carrying r+ from grobinson. Again, just rebase.

Comment 184

[Christoph Kerschbaumer [:ckerschb]]

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.

Comment 185

[Christoph Kerschbaumer [:ckerschb]]

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!

Comment 186

[Chris Novakovic]

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?

Comment 187

[Devdatta Akhawe [:devd]]

@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!

Comment 188

[Deian Stefan]

Attachment: 0001-Bug-671389-Part-1-Implement-CSP-sandbox-directive.patch

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.

Comment 189

[Deian Stefan]

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

Comment 190

[Chris Novakovic]

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.

Comment 191

[Deian Stefan]

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

Comment 192

[Deian Stefan]

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

Comment 193

[Chris Novakovic]

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

Comment 194

[Christoph Kerschbaumer [:ckerschb]]

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.

Comment 195

[Chris Novakovic]

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/

Comment 196

[Deian Stefan]

Attachment: 0001-Bug-671389-Part-1-Implement-CSP-sandbox-directive-r-.patch

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

Comment 197

[Deian Stefan]

Attachment: 0002-Bug-671389-Part-2-Export-document-sandbox-flags-to-c.patch

rebase & removing some trailing spaces

Comment 198

[Deian Stefan]

Attachment: 0003-Bug-671389-Part-3-Tests-for-CSP-sandbox-directive-r-.patch

rebase & rm trailing spaces

Comment 199

[Deian Stefan]

Attachment: 0004-Bug-671389-Part-4-Extend-CSP-tests-for-iframe-sandbo.patch

rebase & rm trailing spaces

Comment 200

[Deian Stefan]

Attachment: 0001-Bug-671389-Part-1-Implement-CSP-sandbox-directive-r-.patch

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

Comment 201

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

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

Comment 202

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 203

[Devdatta Akhawe [:devd]]

What about multiple policies, each with a sandbox directive?

Comment 204

[Deian Stefan]

Attachment: 0001-Bug-671389-Part-1-Implement-CSP-sandbox-directive-r-.patch

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.

Comment 205

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 206

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

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

[Deian Stefan]

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.

Comment 208

[Christoph Kerschbaumer [:ckerschb]]

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?

Comment 209

[Deian Stefan]

Attachment: 0001-Bug-671389-Part-1-Implement-CSP-sandbox-directive-r-.patch

Addressed all the comments. Left the nullptr check as per our IRC discussion.

Comment 210

[Deian Stefan]

Attachment: 0002-Bug-671389-Part-2-Export-document-sandbox-flags-to-c.patch

Line no's changed because of first patch.

Comment 211

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

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.

Comment 212

[Deian Stefan]

Attachment: 0001-Bug-671389-Part-1-Implement-CSP-sandbox-directive-r-.patch

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.

Comment 213

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

(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

Comment 214

[Deian Stefan]

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

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

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

Comment 216

[Deian Stefan]

Attachment: 0001-Bug-671389-Part-1-Implement-CSP-sandbox-directive-r-.patch

Comment 217

[Deian Stefan]

Attachment: 0003-Bug-671389-Part-3-Tests-for-CSP-sandbox-directive-r-.patch

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

Comment 218

[Deian Stefan]

Attachment: 0001-Bug-671389-Part-1-Implement-CSP-sandbox-directive-r-.patch

Rebase.

Comment 219

[Deian Stefan]

Attachment: 0003-Bug-671389-Part-3-Tests-for-CSP-sandbox-directive-r-.patch

Rebased.

Comment 220

[Ian Melven :imelven]

Deian, looks like this is ready to go ?

Comment 221

[Deian Stefan]

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

Comment 222

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

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.

Comment 223

[Deian Stefan]

Attachment: 0001-Bug-671389-Part-1-Implement-CSP-sandbox-directive-r-.patch

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

Comment 224

[Ian Melven :imelven]

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

Comment 225

[Sid Stamm [:geekboy or :sstamm]]

Deian: is this ready to land?  it looks all reviewed...

Comment 226

[Ian Melven :imelven]

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

Comment 227

[Deian Stefan]

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

Comment 228

[Deian Stefan]

Attachment: 0001-Bug-671389-Part-1-Implement-CSP-sandbox-directive-r-.patch

Comment 229

[Deian Stefan]

Attachment: 0002-Bug-671389-Part-2-Export-document-sandbox-flags-to-c.patch

Comment 230

[Deian Stefan]

Attachment: 0003-Bug-671389-Part-3-Tests-for-CSP-sandbox-directive-r-.patch

Comment 231

[Deian Stefan]

Attachment: 0004-Bug-671389-Part-4-Extend-CSP-tests-for-iframe-sandbo.patch

Comment 232

[Deian Stefan]

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

Comment 233

[Christoph Kerschbaumer [:ckerschb]]

(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

Comment 234

[Deian Stefan]

Attachment: 0001-Bug-671389-Part-1-Implement-CSP-sandbox-directive-r-.patch

Just added 'explicity' to the nsCSPSandboxFlags constructor.

Comment 235

[Deian Stefan]

Attachment: 0002-Bug-671389-Part-2-Export-document-sandbox-flags-to-c.patch

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

Comment 236

[Deian Stefan]

Attachment: 0002-Bug-671389-Part-2-Export-document-sandbox-flags-to-c.patch

Fixes comment to test count.

Comment 237

[Ian Melven :imelven]

:D Very exciting ! 

Thanks again for all your patience and persistence on this one, Deian !

Comment 238

[Devdatta Akhawe [:devd]]

woot! this is awesome!

Comment 239

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cd3e227df9dc
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d6021f66c49
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f8d62109bfe
https://hg.mozilla.org/integration/mozilla-inbound/rev/b782435e5640

Comment 240

[Chris Novakovic]

Thanks for persisting with this, Devdatta/Deian - it was definitely worth it!

Comment 241

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cd3e227df9dc
https://hg.mozilla.org/mozilla-central/rev/8d6021f66c49
https://hg.mozilla.org/mozilla-central/rev/0f8d62109bfe
https://hg.mozilla.org/mozilla-central/rev/b782435e5640

Comment 242

[Ryan VanderMeulen [:RyanVM]]

Backed out for causing bug 1129953.
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d8a696af76a

Comment 243

[Ryan VanderMeulen [:RyanVM]]

Confirmed green post-backout.

Comment 244

[Devdatta Akhawe [:devd]]

:(

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.

Comment 245

[Deian Stefan]

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.

Comment 246

[Boris Zbarsky [:bzbarsky]]

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

Comment 247

[Boris Zbarsky [:bzbarsky]]

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.

Comment 248

[Deian Stefan]

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

Comment 249

[Bobby Holley (:bholley)]

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

Comment 250

[Christoph Kerschbaumer [:ckerschb]]

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?

Comment 251

[Deian Stefan]

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

Comment 252

[Devdatta Akhawe [:devd]]

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

Comment 253

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 254

[Boris Zbarsky [:bzbarsky]]

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

Comment 255

[Bobby Holley (:bholley)]

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?

Comment 257

[Devdatta Akhawe [:devd]]

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.

Comment 258

[Boris Zbarsky [:bzbarsky]]

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

Comment 259

[Deian Stefan]

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?