Closed Bug 671389 Opened 13 years ago Closed 8 years ago

Implement CSP sandbox directive

Categories

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

enhancement

Tracking

()

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

People

(Reporter: sephr, Assigned: pacaro)

References

(Blocks 2 open bugs, )

Details

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

Attachments

(4 files, 97 obsolete files)

12.62 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
14.78 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
21.66 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
28.83 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
The HTML5 sandboxing features just won't cut it for anything that isn't text/html. As it stands, it's impossible to sandbox an image/svg+xml game a user uploads to your SVG game site, as HTML5 only introduces a text/html-sandboxed media type. I know that CSP isn't specifically intended for sandboxing user generated content, but it feels like the most appropriate place for sandboxing features, as opposed to introducing a new media type.
Blocks: CSP
Component: DOM: Core & HTML → Security
QA Contact: general → toolkit
I just noticed the "sandbox" directive in the CSP draft at http://dvcs.w3.org/hg/content-security-policy/raw-file/tip/csp-specification.dev.html#sandbox so I have changed the bug summary to "Implement CSP sandbox directive".
Summary: Merge HTML5 sandboxing features with CSP → Implement CSP sandbox directive
Severity: normal → enhancement
Component: Security → DOM: Core & HTML
QA Contact: toolkit → general
Depends on: framesandbox
Assignee: nobody → dev.akhawe+mozilla
I propose breaking this up into two: implementing the sandbox directive in enforcement mode and in monitoring mode. If I am not wrong, it will be a lot of work to implement the monitoring mode. The enforcement mode implementation can be pretty quick. Thoughts ? In particular, imelven how much work do you think it would be to implement the monitoring mode? If its a lot of work, I am not even sure its worth the effort.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attached patch csp-sandbox-enforcement (obsolete) — Splinter Review
Implements CSP Sandbox directive in enforcement mode only
This patch requires that the patches for bug 341604 be imported first.
Webkit doesn't support a report only mode for sandbox either; and I conjecture that IE doesn't either.
A note that we should check that if the sandbox flags already exist, then the CSP value should only lock it down further, not make it looser (and vice versa)
Comment on attachment 635029 [details] [diff] [review] csp-sandbox-enforcement Review of attachment 635029 [details] [diff] [review]: ----------------------------------------------------------------- Looks like this is definitely on the right track - thanks for hacking on it ! ::: content/base/public/nsIContentSecurityPolicy.idl @@ +49,5 @@ > > /** > + * Checks whether the sandbox directive is set > + */ > + readonly attribute boolean isSandbox; nit : would you consider hasSandbox ? @@ +146,5 @@ > + /** > + * Delegate method called by the service when the protected document is loaded. > + * Responds with whether the sandbox directive is set, and fills in the value. > + */ > + boolean shouldSandbox(out AString sandboxValue); what's the difference between the return of this vs isSandbox ? why do you need both ? ::: content/base/src/CSPUtils.jsm @@ +151,5 @@ > > this._allowEval = false; > this._allowInlineScripts = false; > + this._isSandbox = false; > + this._sandboxFlags = ""; maybe a nit, but these aren't the same as the actual flags, they're really the optional opt-in strings @@ +230,4 @@ > continue directive; > } > > + if (dirname === CSPRep.SANDBOX_DIRECTIVE){ i would suggest a big comment header like the other directives have @@ +232,5 @@ > > + if (dirname === CSPRep.SANDBOX_DIRECTIVE){ > + aCSPR._isSandbox = true; > + aCSPR._sandboxFlags = dirvalue; > + CSPWarning(" setting sandbox to true with dirvalue : "+dirvalue); why is this a warning ? seems like more of a CSPDdebug message ? @@ +517,5 @@ > newRep._allowInlineScripts = this.allowsInlineScripts > && aCSPRep.allowsInlineScripts; > > + newRep._isSandbox = this.isSandbox || aCSPRep.isSandbox; > + //sandbox flags separated by a space a more verbose comment might be clearer here ::: content/base/src/contentSecurityPolicy.js @@ +80,5 @@ > } > > ContentSecurityPolicy.prototype = { > + //classID: Components.ID("{AB36A2BF-CB32-4AA6-AB41-6B4E4444A221}"), > + classID: Components.ID("{62cae874-f163-4b8c-a3e9-ce4d88e9a59c}"), you'll need to remove the commented out bits like this in the final patch ::: content/base/src/nsDocument.cpp @@ +31,5 @@ > #include "nsIScriptRuntime.h" > #include "nsCOMArray.h" > > +#include "nsSandboxFlags.h" > + you don't actually need this unless you're using the defines, i think ? @@ +2503,5 @@ > } > } > > + nsAutoString sandboxFlags; > + bool shouldSandbox=false; meta-nit : please use the same spacing around = and , etc as is used in the rest of the file @@ +2506,5 @@ > + nsAutoString sandboxFlags; > + bool shouldSandbox=false; > + rv = mCSP->ShouldSandbox(sandboxFlags,&shouldSandbox); > + NS_ENSURE_SUCCESS(rv,rv); > + if(shouldSandbox){ the null principal etc should only happen if SANDBOXED_ORIGIN is set, won't this be taken care of later in the usual spot for iframe sandbox once you set the docshell flags ? @@ +2509,5 @@ > + NS_ENSURE_SUCCESS(rv,rv); > + if(shouldSandbox){ > + nsCOMPtr<nsIPrincipal> ownerPrincipal = do_CreateInstance("@mozilla.org/nullprincipal;1"); > + nsContentUtils::SetUpChannelOwner(ownerPrincipal, mChannel, chanURI, false,true); > + docShell->SetSandboxFlags(nsContentUtils::ParseSandboxAttributeToFlags(sandboxFlags)); this implies sandboxFlags is really the string attribute and not the flags - a little confusing perhaps
Attachment #635029 - Flags: feedback+
Attached patch Merging imelven's feedback (obsolete) — Splinter Review
@imelven: All good points, and all fixed. Thanks. I have adopted the name sandboxAttrValue instead of sandboxFlags. The isSandbox addition to the IDL was a mistake, and not needed. I have removed it. I think the setsandboxflags function in your iframe_sandbox patch should be modified to be "make it only stronger than what it is already". Or I can do the hackery here, but I think it is better in setSandboxFlags. What do you think?
Attachment #635029 - Attachment is obsolete: true
(In reply to Devdatta Akhawe from comment #8) > > I think the setsandboxflags function in your iframe_sandbox patch should be > modified to be "make it only stronger than what it is already". Or I can do > the hackery here, but I think it is better in setSandboxFlags. What do you > think? that's a good thought, i'm wondering about what to do if the sandbox attribute is removed ? In that case we want to clear all flags - i suppose we could treat that as a special case though. I'll look at doing this in the iframe sandbox patch. Thanks !
Asking for geekboy's review, since he is familiar with the CSP code.
Attachment #635535 - Attachment is obsolete: true
Attachment #635818 - Flags: review?(sstamm)
Comment on attachment 635818 [details] [diff] [review] added support for policy intersection Review of attachment 635818 [details] [diff] [review]: ----------------------------------------------------------------- Good start! Please update your diff-maker to show 8 lines of context (https://developer.mozilla.org/En/Creating_a_patch). It's hard to tell why some of the code changes were necessary because of lack of context. Much of this is nits, but there are a few things to do yet: * Please check format of things like if statements and line-length of comments * You'll also need a super-review for the idl changes. * Tests for this (will depend on the iframe-sandbox code). ::: content/base/public/nsIContentSecurityPolicy.idl @@ +138,5 @@ > in ACString aMimeType, > in nsISupports aExtra); > + /** > + * Delegate method called by the service when the protected document is loaded. > + * Responds with whether the sandbox directive is set, and fills in the value. Please describe the parameters and return value in depth; see other documentation comments in this file for examples. Also, please include a blank line between the previous declaration and this comment block. ::: content/base/src/CSPUtils.jsm @@ +232,5 @@ > > + // SANDBOX DIRECTIVE //////////////////////////////////////////////// > + // We don't parse the value of the sandbox directive, since there is > + // already native code that does this for iframe sandbox > + if (dirname === CSPRep.SANDBOX_DIRECTIVE){ Nit: space between ) and { @@ +235,5 @@ > + // already native code that does this for iframe sandbox > + if (dirname === CSPRep.SANDBOX_DIRECTIVE){ > + aCSPR._isSandbox = true; > + aCSPR._sandboxAttrValue = dirvalue; > + CSPdebug(" setting sandbox to true with dirvalue : "+dirvalue); nit: spaces before and after + operator. No space in beginning of message. @@ +438,5 @@ > } > + > + if(this._isSandbox){ > + dirs.push("sandbox "+ this._sandboxAttrValue); > + } Nit: Format this if signature like the others in this file: --> if (foo) { Please make sure this is consistent in the rest of your js edits. @@ +521,5 @@ > && aCSPRep.allowsInlineScripts; > > + newRep._isSandbox = this.isSandbox || aCSPRep.isSandbox; > + //sandbox attribute values are separated by space: the iframe sandbox takes care of repeated values > + newRep._sandboxAttrValue = this._sandboxAttrValue + " "+ aCSPRep._sandboxAttrValue; I'm not sure just concatenating the two values is the best option. You'll have to either parse and intersect the sandbox policies here, or have to keep the policies separate until they are parsed. ::: content/base/src/contentSecurityPolicy.js @@ +103,4 @@ > return this._reportOnlyMode || this._policy.allowsEvalInScripts; > }, > > + nit: remove duplicate blank line. @@ +451,5 @@ > + CSPdebug("shouldsandbox: "+this._policy.isSandbox); > + aSandboxAttrValue.value = this._policy.sandboxAttrValue ; > + CSPdebug("sandbox attribute value: "+aSandboxAttrValue.value+"|"); > + return this._reportOnlyMode || this._policy.isSandbox; > + }, nit: blank line between this function and the next one. ::: content/base/src/nsDocument.cpp @@ +2374,5 @@ > > mChannel = aChannel; > > + nsresult rv = InitCSP(); > + NS_ENSURE_SUCCESS(rv, rv); I assume this move of InitCSP() is because of where some iframe-sandbox-specific code gets added to nsDocument::StartDocumentLoad(). @@ +2504,5 @@ > + nsAutoString sandboxAttrValue; > + bool shouldSandbox = false; > + rv = mCSP->ShouldSandbox(sandboxAttrValue,&shouldSandbox); > + NS_ENSURE_SUCCESS(rv,rv); > + if(shouldSandbox){ Nit: Space after if and after closed parens: --> if (foo) { @@ +2508,5 @@ > + if(shouldSandbox){ > + PRUint32 sandboxFlags; > + docShell->GetSandboxFlags(&sandboxFlags); > + sandboxFlags |= nsContentUtils::ParseSandboxAttributeToFlags( > + sandboxAttrValue); Nit: Don't wrap the parameter list. This is probably where you'll want to get the multiple sandbox policies and parse them (or maybe it's better to parse the sandbox policies earlier and store the parsed flags in the CSP).
Attachment #635818 - Flags: review?(sstamm) → review-
@geekboy: thanks for the review. I think there is a flaw in the code with the handling of multiple CSP policies. I think the call needs to return an array of all the policies seen so far, and intersect (OR) them in the C++ code. One of the reasons for moving the InitCSP call was to let the existing code take care of setting the sandbox flags. (the original patch had a SetupChannelOwner call inside InitCSP. Looking at how we might add support for CSP via the meta tag, I think the clean way to do it would be to just call the InitCSP function if we see a META with X-Content-Security-Policy http-equiv As such, it makes sense to force the channelOwner in the InitCSP code itself. thoughts, particularly from imelven?
I think I have addressed all of Sid's comments and nits. Thanks for pointing out the error in the sandbox intersection logic, that was pretty lame on my part. One question: I am using an nsIArray now. Who is in charge of freeing it?
Attachment #635818 - Attachment is obsolete: true
Attachment #637207 - Flags: review?(sstamm)
I was mistakenly attaching the flag to the docshell instead of the document. This is now fixed.
Attachment #637207 - Attachment is obsolete: true
Attachment #637207 - Flags: review?(sstamm)
Moved initCSP back down, since it doesn't need to be moved any more.
Attachment #637985 - Attachment is obsolete: true
Note that the sandbox patch doesn't check the document's sandbox flags for disabling plugins; instead that is done on the docshell. This means, the current patch doesn't block plugins. I have left a comment on the bug, but an ugly solution is to just implicitly add a 'object-src: 'none'" if we have a CSP sandbox header without allow-plugins.
Comment on attachment 637988 [details] [diff] [review] Implement CSP sandbox directive support Obsoleting this patch: need to implement either a 'make this principal null' interface on nsIPrincipal, or need to move the initCSP code to much earlier. In any case, the patch is wrong. I will wait for sandbox to land before moving further on this.
Attachment #637988 - Attachment is obsolete: true
(In reply to Devdatta Akhawe from comment #17) > Comment on attachment 637988 [details] [diff] [review] > Implement CSP sandbox directive support > > Obsoleting this patch: need to implement either a 'make this principal null' > interface on nsIPrincipal, or need to move the initCSP code to much earlier. > In any case, the patch is wrong. I will wait for sandbox to land before > moving further on this. Dev - fyi, bug 341604 is getting pretty close to landing, so I've started looking at this. We've moved the priority up to a P2 on the security roadmap, since we need this to have a full sandboxing solution. The plugin check has been moved to the document now so that part should be taken care of. Meanwhile, it would be great if anyone wanted to start porting the tests for bug 341604 over to test CSP sandbox - Sid and I talked about this and it basically boils down to creating a ^headers^ file for each sandboxed document used in the iframe sandbox tests that does the same sandboxing as the iframe sandbox attribute does in those tests and creating new main test pages for each set of CSP sandbox tests. I will try to make this happen if nobody else wants to pick it up.
Attached patch Strawman MakeNull patch (obsolete) — Splinter Review
Attachment #656931 - Flags: review?(imelven)
Here's a sample patch that adds a MakeNull call to nsIPrincipal. @imelven: can you please take a look, and see if it makes sense? I would prefer to get feedback whether this is the right approach, before actually completing all the patches. bz: can you also take a look? It is a little different from what you had suggested: instead of creating a new nsNullPrincipal, I just added a mIsNull flag. With this makenull, the meta tag support should also be pretty easy to support.
You need to fix things like GetOrigin at least, right? Your Equals() check became asymmetric; that's probably a bug. Various other nits about casing and spacing and whatnot, but those are less important than deciding whether we think we can whack all the moles on nsPrincipal sufficiently. As in, decide what every single API method should do for the mIsNull case.
hmm .. you are right. This what a mole thing looks like a bad idea: maybe just create a new NullPrincipal, check if it exits and for *ALL* calls just forward it to the newly created nullprincipal?
err .. submitted too early s/exits/exists bz: do you suspect any issues with that approach? If not, I will go ahead and create a patch that does this.
That might work better, as long as you still have to make Equals() and Subsumes work right (i.e. check object-identity before forwarding, verify that they work correctly when the RHS is the outer nsIPrincipal).
By verify, do you mean write tests? Definitely need those for such a major change. Cause, if I am not wrong, shouldn't both functions work without any special changes? The 'this' value inside the inner nsNullPrincipal's equals/subsumes function should point to the same inner nsNullPrincipal if the outer nsPrincipal is the same, and point to different nsnulls if the outer is not the same.
> By verify, do you mean write tests? That too, but also just by code inspection. > The 'this' value inside the inner nsNullPrincipal's equals/subsumes function should point > to the same inner nsNullPrincipal if the outer nsPrincipal is the same How are you going from outer to inner for the _argument_ to equals/subsumes?
(In reply to Boris Zbarsky (:bz) from comment #24) > That might work better, as long as you still have to make Equals() and > Subsumes work right (i.e. check object-identity before forwarding, verify > that they work correctly when the RHS is the outer nsIPrincipal). fwiw, this outer/inner approach is exacly what i was going to suggest. due to previous experiences with other code bases, anything where we have to 'whack a mole' wrt security is pretty terrifying to me ;) tests for this inner / outer principal split definitely sound like a requirement, in addition to the csp sandbox tests itself we will need (these should basically be the iframe sandbox tests but sending csp headers instead of using the sandbox attribute.
> How are you going from outer to inner for the _argument_ to equals/subsumes? duh .. yes. thanks for pointing this out.
Attachment #656931 - Attachment is obsolete: true
Attachment #656931 - Flags: review?(imelven)
Here's a new patch. Does this seem right? Subsumes just calls equals, so I didn't change it. Also, I didn't make any changes to getorigin, getdomain, getURI calls. Can someone else confirm if that sounds about right?
Attachment #657090 - Flags: review?(bzbarsky)
Comment on attachment 657090 [details] [diff] [review] Patch to add support for making a principal null at runtime The Equals() impl is still wrong when |this| has no mNullPrincipal but aOther has an mNullPrincipal. Why _not_ change GetDomain, GetURI, GetOrigin? New method should be lowercase in the IDL. Don't you need to fix Read and Write as well? That's actually possibly the biggest problem with this approach: it can break certain session restore cases. :( For what it's worth, I still think we should simply not support going to a null principal in a meta tag, and fix this the other way (moving the CSP parsing to before we set up the document principal)...
Attachment #657090 - Flags: review?(bzbarsky) → review-
> The Equals() impl is still wrong when |this| has no mNullPrincipal but > aOther has an mNullPrincipal. > yup. Will fix. > Why _not_ change GetDomain, GetURI, GetOrigin? > I wasn't sure about what should be the return value of these functions once they are made null. A principal for http://www.foobar.com/1.html should still return this URI even if the principal is null, right? Similarly for domains and origin? It is just the security checks that change? > > Don't you need to fix Read and Write as well? That's actually possibly the > biggest problem with this approach: it can break certain session restore > cases. :(ss > Ok. I don't know what read/write do: my assumption was that I only need to worry about things inside nsiprincipal.idl, as all other things are internal. Does this system not sound right? I ignore read/write because they are not in the nsiprincipal idl. > For what it's worth, I still think we should simply not support going to a > null principal in a meta tag, and fix this the other way (moving the CSP > parsing to before we set up the document principal)... I like the "makeNull" as a call to support in general: a mechanism for a principal to "drop" privileges is important to have imho. But to implement CSP sandbox, it makes sense to do what is quick and clean. How far back would CSP initialization have to be moved?
> A principal for http://www.foobar.com/1.html should still return this URI even if the > principal is null, right? That _really_ depends.... I would assume it's a security hole to do that until proven otherwise because someone will assume they can use the principal URI for some security check. Certainly .origin is used for security checks; that is its only reason for existence. > Ok. I don't know what read/write do: They serialize and deserialize a principal. > I ignore read/write because they are not in the nsiprincipal idl. They're on another interface that principals implement. They don't just implement nsIPrincipal! > I like the "makeNull" as a call to support in general My concern is that getting it right is _HARD_ as the above discussion shows. CSP _parsing_ would need to move back to before the GetChannelPrincipal call in nsDocument::Reset. So the ordering would be: 1) parse CSP. 2) Use the result to determine what principal to use. 3) Store the parsed CSP on the resulting principal.
And the point is, we parse CSP right now in InitCSP, which is called at the end of StartDocumentLoad. Reset() is called a bit earlier in the same method. So if we break up InitCSP into two methods, one of which does the actual init of mCSP and one of which does the setting of mCSP on the principal, I think it should all just work...
ok. Let me do this: 1. Rewrite the CSP header sandbox support by moving/breaking-up initCSP so that it figures out the sandbox status before reset is called. 2. Create a new bug for meta sandbox support, in which I will import this patch and hack on it. Even if it is hard, I still think it is worth working on and trying to get right.
(In reply to Devdatta Akhawe from comment #35) > ok. Let me do this: > > 1. Rewrite the CSP header sandbox support by moving/breaking-up initCSP so > that it figures out the sandbox status before reset is called. > > 2. Create a new bug for meta sandbox support, in which I will import this > patch and hack on it. Even if it is hard, I still think it is worth working > on and trying to get right. sounds like a solid plan - my personal preference FWIW is to do CSP sandbox first as you suggest, since then we have a complete solution to sandboxing documents (iframe sandbox + CSP sandbox for the case where the framed content is access directly).
(In reply to Devdatta Akhawe from comment #35) > 2. Create a new bug for meta sandbox support, in which I will import this > patch and hack on it. Even if it is hard, I still think it is worth working > on and trying to get right. I'm not sure we will need this. Meta tag support for csp is in csp 1.1. We have had discussions in the webappsec working group about not honoring the sandbox directive if it is set in a meta tag policy. Nothing has been decided for sure yet, as the 1.1 spec is still in the really early stages.
Whiteboard: [CSP 1.1]
It seems the call to reset depends on aReset but InitCSP doesn't depend on aReset. I don't know the context of the code that well: is it ok to just remove the aReset check?
(In reply to Devdatta Akhawe from comment #38) > It seems the call to reset depends on aReset but InitCSP doesn't depend on > aReset. I don't know the context of the code that well: is it ok to just > remove the aReset check? I don't understand why you need to remove the aReset check - you just need to make sure that the CSP has been set up to the point where ::Reset knows it should put a null principal on the document, don't you ?
Even if I setup CSP, if the ::Reset call is never made (because of the if condition), then a nullPrincipal won't be set, right? To be clear, I can think of a number of ways to solve this: I don't want to make a guess though, and rather would prefer knowing what is the context of this call and what's the right thing to do.
The whole point of aReset is to say that you should not reset! aReset == false is only used in the following cases: 1) Document for a DOMParser 2) Document for cross-site XHR In both cases the caller has already set up a specific principal for the document. It's not clear to me how, or whether, CSP headers should affect those cases.
hmm.. as an aside, why are we are calling InitCSP for cases with aReset==false ? Anyways, I think we can get away with not doing anything for CSP sandbox in the 2 cases. If it was called for same-site XHR, then maybe there was something to think about.
Comment on attachment 657605 [details] [diff] [review] Move CSP code to before principal reset Requesting review on patch that moves the CSP code. The actual sandbox patch (previously proposed examples are in this bug) will check hasSandbox, and change the arguments to ::Reset appropriately.
Attachment #657605 - Flags: review?(bzbarsky)
I think we might want to only honor sandbox when it's in the 1.0 spec compliant header - see bug 783049 what do you think, Sid ?
Attachment #657605 - Attachment is obsolete: true
Attachment #657605 - Flags: review?(bzbarsky)
Attachment #657090 - Attachment is obsolete: true
@imelven: Can you take a look at and see how this interacts if the docshell is sandboxed-origin? I think it is all right, but a second pair of eyes would be nice.
Attachment #657692 - Flags: review?(bzbarsky)
Attachment #657693 - Flags: review?(imelven)
Comment on attachment 657692 [details] [diff] [review] Move CSP code to before principal reset The NodePrincipal() check in InitCSP() makes no sense with this patch. I'm not that happy with the move of the mChannel set (and also the RetrieveRelevantHeaders, because that can change various document stat) to before the Reset call. Probably better to pass the channel to InitCSP instead of using mChannel in it, with a comment explaining why. Using "mCSP" for an on-stack variable is weird. Just call it "csp". Also, if you're going to write stuff into a COMPtr, you probably want to pass an nsIContentSecurityPolicy** to InitCSP. GetPrincipal() on documents never returns null, fwiw.
Attachment #657692 - Flags: review?(bzbarsky) → review-
Attachment #657693 - Flags: review?(imelven) → review?(sstamm)
thanks for the review. > The NodePrincipal() check in InitCSP() makes no sense with this patch. Can you elaborate why? It seems we still want to make sure that we don't do anything for system principals? > I'm not that happy with the move of the mChannel set (and also the > RetrieveRelevantHeaders, because that can change various document stat) to > before the Reset call. Probably better to pass the channel to InitCSP > instead of using mChannel in it, with a comment explaining why. > So, the initcsp code will then use the channel to get the headers it wants? Can you elaborate on why the move to the new location bad? > Using "mCSP" for an on-stack variable is weird. Just call it "csp". > > Also, if you're going to write stuff into a COMPtr, you probably want to > pass an nsIContentSecurityPolicy** to InitCSP. > > GetPrincipal() on documents never returns null, fwiw. thanks .. all fixed!
> Can you elaborate why? Because it's now happening before the principal of the document has been set up, which means the principal of the document is always a null principal. > So, the initcsp code will then use the channel to get the headers it wants? Yes. > Can you elaborate on why the move to the new location bad? Because it makes assumptions about which members Reset() will and will not reset.
> Because it's now happening before the principal of the document has been set > up, which means the principal of the document is always a null principal. > hmm; so I should move it to the part of the code that attaches the csp policy (and not attach in case it is a system URI ) ? Not sure if this case can actually ever happen.
Attachment #657692 - Attachment is obsolete: true
Attachment #658691 - Flags: review?(bzbarsky)
> Not sure if this case can actually ever happen. It can certainly happen if someone tries hard enough; we have APIs that can make it happen. It _shouldn't_ be done because it's kinda insecure, of course....
Comment on attachment 658691 [details] [diff] [review] Move CSP code to before principal reset Important issues: * This patch crashes on non-HTTP channels. How was this patch tested? * The patch changes behavior for the byte-to-unicode conversion: we used to byte-inflate, but now we treat the bytes as UTF-8. Which does the spec say to do? Nits: * This is still null-checking GetPrincipal. * Please have a space in "//Copy". * Please put aCSP as the second argument of InitCSP. * Please fix the indentation on your GetResponseHeader calls. Especially the first one. * Do you still need to CSP headers in RetrieveRelevantHeaders? * Why use NS_IF_ADDREF on a pointer you know is non-null? It would be better to just swap() the nsCOMPtr into aCSP once you're done setting it up.
Attachment #658691 - Flags: review?(bzbarsky) → review-
Attachment #658691 - Attachment is obsolete: true
Attachment #659032 - Flags: review?(bzbarsky)
Thanks! I think I fixed all the issues; I used NS_ConvertASCIIToUTF16 for the headers, per our discussion. I am not certain of the indentation: I just used vim's default, with the settings given at the top of the file. Does it look right?
Comment on attachment 659032 [details] [diff] [review] Move CSP code to before principal reset >+ if (httpChannel == NULL) { if (!httpChannel) { Also, set *aCSP to null before that return NS_OK, please. >+ Please don't add the stray blank line. r=me with those.
Attachment #659032 - Flags: review?(bzbarsky) → review+
Attachment #659032 - Attachment is obsolete: true
Comment on attachment 659265 [details] [diff] [review] Move CSP code to before principal reset thanks! I added a *aCSP=NULL to the top of InitCSP, so that it will default to null unless we reach the csp.forget call. Removed the new line too. Carrying over the r+ from bz
Attachment #659265 - Flags: review+
nullptr, please.
use nullptr instead of null
Attachment #659265 - Attachment is obsolete: true
Attachment #659276 - Flags: review+
Attachment #657693 - Attachment is obsolete: true
Attachment #657693 - Flags: review?(sstamm)
Attachment #659416 - Attachment is obsolete: true
Attachment #659430 - Flags: review?(sstamm)
Depends on: 790747
Comment on attachment 659276 [details] [diff] [review] Move CSP code to before principal reset this patch obsoleted by bug 790747
Attachment #659276 - Attachment is obsolete: true
Priority: -- → P3
Assignee: dev.akhawe+mozilla → nobody
Assignee: nobody → deian
Attachment #659430 - Attachment is obsolete: true
Attachment #659430 - Flags: review?(sstamm)
Attachment #8355167 - Flags: review?(sstamm)
Attachment #8355167 - Flags: review?(ian.melven)
Hey Deian, thanks for your work on CSP sandbox - it's awesome to see it moving along. I'll take a look as soon as I can. Bob Owen has been doing a _lot_ of work on sandbox navigation lately (including some excellent tests) as well as implementing allow-popups so I've cc'd him as well - Bob, can you take a look through Deian's patches when you have time ?
(In reply to Ian Melven :imelven from comment #76) > Bob, can you take a > look through Deian's patches when you have time ? Hopefully, I'll get some time next week. bholley has asked me to re-write my tests for bug 785310, which I hope to start in the morning. Once I've got them out of the way I'll take a look.
(In reply to Bob Owen from comment #77) > (In reply to Ian Melven :imelven from comment #76) > > Bob, can you take a > > look through Deian's patches when you have time ? > > Hopefully, I'll get some time next week. > bholley has asked me to re-write my tests for bug 785310, which I hope to > start in the morning. > Once I've got them out of the way I'll take a look. Thanks, I'll try to take a look myself this weekend :D
Comment on attachment 8355168 [details] [diff] [review] 0002-Bug-671386-Part-2-tests-for-CSP-sandbox-directive-ge.patch Review of attachment 8355168 [details] [diff] [review]: ----------------------------------------------------------------- It looks like these are pretty much 1 for 1 copies of the existing iframe sandbox tests for the HTML attribute. It would be great if there was some way to not have to duplicate all the tests, but I'm not sure there's an easy way of doing so. One reason at least for doing so is making sure that both the CSP and HTML sandbox tests stay in sync and don't diverge. For example, Bob is working on fixing navigation via window.location in bug 785310 and is adding a fair amount of tests, he would then need to duplicate all those tests for a CSP based sandbox attribute if we follow that model. I know Garrett (grobinson) has been working on CSP test stuff as well so I've cc'd him. I'm not sure if it makes more sense to have these tests live with the CSP tests or the iframe sandbox tests (which is where they are now). Since CSP sandbox is about sandboxing documents in the same way as iframe sandbox, I wonder if this approach is the right one. Really we should have one set of tests which test that a sandboxed document, whether via CSP or the sandbox attribute, is correctly restricted from doing thing it shouldn't be doing. All we really care about beyond that is that the sandbox attribute is setting the flags appropriately based on its value. I'm interested to hear what Bob, Garrett and others have to say on this issue as well. I think the most important thing to test for CSP sandbox is the parsing of the attribute. I made a comment in the general test file about the tests for parsing of the HTML sandbox attribute - some of the things it says are being tested not being tested. Note that the CSP spec may differ for what whitespace separators are acceptable for the sandbox directive within a CSP policy vs in the HTML iframe sandbox attribute. Parsing the attribute from a CSP is what you're really adding with this work so I think there should definitely be some more testing around, for example making sure sandbox is correctly handed when specified with other directives. ::: content/html/content/test/file_iframe_sandbox_csp_c_if3.html @@ +8,5 @@ > + > +</head> > +<script type="text/javascript"> > + function doStuff() { > + dump("*** c_if3 has loaded\n"); nit: avoid committing dump() statements (this is probably something you copied over from an existing test and is likely my fault originally :) ) ::: content/html/content/test/file_iframe_sandbox_csp_c_if4.html @@ +12,5 @@ > + window.parent.ok_wrapper(result, desc); > + } > + > + function doStuff() { > + // try to open a new window via target="_blank", window.open(), and showModalDialog() i feel like these tests may have changed/been expanded/possibly been superceded by Bob's navigation work, but he will know better. @@ +22,5 @@ > + window.open("about:blank"); > + } catch (error) { > + threw = true; > + } > + nit : whitespace (again probably was in the original file) @@ +31,5 @@ > + window.showModalDialog("about:blank"); > + } catch(error) { > + threw = true; > + } > + nit : whitespace (again probably was in the original file) ::: content/html/content/test/file_iframe_sandbox_csp_c_if6.html @@ +11,5 @@ > + function ok(result, desc) { > + window.parent.ok_wrapper(result, desc); > + window.parent.postMessage({ok: result, desc: desc}, "*"); > + } > + nit : whitespace (again probably was in the original file) ::: content/html/content/test/file_iframe_sandbox_csp_c_if6.html^headers^ @@ +1,1 @@ > +Content-Security-Policy: sandbox allow-same-origin allow-scripts nit : trailing whitespace ::: content/html/content/test/test_iframe_sandbox_csp_general.html @@ +76,5 @@ > + > + // fails if bad > + // 8) test that script in an event listener (img onerror) cannot run in an iframe sandboxed without "allow-scripts" > + // (done in file_iframe_sandbox_csp_c_if2.html which has sandbox='') > + nit : whitespace (again probably was in the original file) @@ +117,5 @@ > + // 16) test that a sandboxed iframe can't open a new window using window.ShowModalDialog > + // this is done via file_iframe_sandbox_csp_c_if4.html which is sandboxed with "allow-scripts" and "allow-same-origin" > + // the window it attempts to open calls window.opener.ok(false, ...) and file_iframe_c_if4.html has an ok() > + // function that calls window.parent.ok_wrapper > + nit : whitespace (again probably was in the original file) @@ +122,5 @@ > + // passes twice if good > + // 17) test that a sandboxed iframe can access same-origin documents and run scripts when its sandbox attribute > + // is separated with two spaces > + // done via file_iframe_sandbox_csp_c_if6.html which is sandboxed with " allow-scripts allow-same-origin " > + nit : whitespace (again probably was in the original file) @@ +125,5 @@ > + // done via file_iframe_sandbox_csp_c_if6.html which is sandboxed with " allow-scripts allow-same-origin " > + > + // passes twice if good > + // 18) test that a sandboxed iframe can access same-origin documents and run scripts when its sandbox attribute > + // is separated with tabs This doesn't seems true - the headers for file_iframe_sandbox_csp_if6 are :Content-Security-Policy: sandbox allow-same-origin allow-scripts This is only testing what test #18 is looking for. It doesn't look like #19 #20 and #21 are being tested. Note what I said in the main comment, we have to be sure we handle what the CSP spec says is acceptable here, which may or may not be the same as what is allowed by the HTML spec for iframe sandbox. @@ +127,5 @@ > + // passes twice if good > + // 18) test that a sandboxed iframe can access same-origin documents and run scripts when its sandbox attribute > + // is separated with tabs > + // done via file_iframe_sandbox_csp_c_if6.html which is sandboxed with "&#x09;allow-scripts&#x09;allow-same-origin&#x09;" > + nit : whitespace (again probably was in the original file) @@ +132,5 @@ > + // passes twice if good > + // 19) test that a sandboxed iframe can access same-origin documents and run scripts when its sandbox attribute > + // is separated with line feeds > + // done via file_iframe_sandbox_csp_c_if6.html which is sandboxed with "&#x0a;allow-scripts&#x0a;allow-same-origin&#x0a;" > + nit : whitespace (again probably was in the original file) @@ +137,5 @@ > + // passes twice if good > + // 20) test that a sandboxed iframe can access same-origin documents and run scripts when its sandbox attribute > + // is separated with form feeds > + // done via file_iframe_sandbox_csp_c_if6.html which is sandboxed with "&#x0c;allow-scripts&#x0c;allow-same-origin&#x0c;" > + nit : whitespace (again probably was in the original file)
Attachment #8355168 - Flags: review?(ian.melven)
Also I see you add some tests in patch 9 which test the intersection of an iframe sandbox attribute and a CSP sandbox attribute - I think that's a very important thing to test as well, nice work :) I would assume that the intersection logic is the same as it is for the sandbox attribute when CSP isn't involed : you can only further restrict and never loosen permissions.
Comment on attachment 8355169 [details] [diff] [review] 0003-Bug-671389-Part-3-tests-for-CSP-sandbox-directive-in.patch Review of attachment 8355169 [details] [diff] [review]: ----------------------------------------------------------------- The overall comment that I don't think duplicating all the tests of the restrictions is necessarily the right way to go here applies to these tests as well. ::: content/html/content/test/test_iframe_sandbox_csp_inheritance.html @@ +14,5 @@ > +/** Test for Bug 671389 - Implement CSP sandbox directive **/ > +/** Inheritance Tests **/ > + > +SimpleTest.waitForExplicitFinish(); > + nit: whitespace @@ +35,5 @@ > + > + completedTests++; > + > + if (result) { > + passedTests++; nit: weird indentation @@ +36,5 @@ > + completedTests++; > + > + if (result) { > + passedTests++; > + } nit: whitespace @@ +37,5 @@ > + > + if (result) { > + passedTests++; > + } > + nit: whitespace @@ +39,5 @@ > + passedTests++; > + } > + > + if (completedTests == 4) { > + is(passedTests, 4, "there should be 3 passed inheritance tests"); These numbers do not match up, looks like there's 4 'passes if good' tests in this file. @@ +48,5 @@ > +function doTest() { > + // fails if bad > + // 1) an iframe with no sandbox attribute inside an iframe that has sandbox = "" > + // should not be able to execute scripts (cannot ever loosen permissions) > + // (done by file_iframe_sandbox_csp_a_if2.html contained within file_iframe_sandbox_csp_a_if1.html) file_iframe_sandbox_csp_a_if2.html has headers of "Content-Security-Policy: sandbox" so the description does not match the test. @@ +53,5 @@ > + > + // fails if bad > + // 2) an iframe with sandbox = "allow-scripts" inside an iframe that has sandbox = "" > + // should not be able to execute scripts (cannot ever loosen permissions) > + // (done by file_iframe_sandbox_csp_a_if2.html contained within file_iframe_sandbox_csp_a_if1.html) this description can't be true since the header for file_iframe_sandbox_csp_a_if2.html has headers of "Content-Security-Policy: sandbox" @@ +86,5 @@ > + // to execute scripts > + // (done by file_iframe_sandbox_csp_a_if2.html contained within file_iframe_sandbox_csp_a_if3.html) > + > + // passes if good > + // 9) make sure that changing the sandbox flags on an iframe (if_8) doesn't affect This test is a little strange from a CSP perspective - at the moment, there's no way to change the CSP of a document after load. I suppose it could be a test for the case when a document is sandboxed via a CSP and then has its sandbox attribute changed, but that might make more sense being bundled in with the CSP+sandbox attribute intersection tests ? @@ +90,5 @@ > + // 9) make sure that changing the sandbox flags on an iframe (if_8) doesn't affect > + // the sandboxing of subloads of content within that iframe > + var if_8 = document.getElementById('if_8'); > + if_8.sandbox = 'allow-scripts'; > + if_8.contentWindow.doSubload(); nit: trailing whitespace
Attachment #8355169 - Flags: review?(ian.melven)
Comment on attachment 8355170 [details] [diff] [review] 0004-Bug-671389-Part-4-tests-for-CSP-sandbox-directive-wo.patch Review of attachment 8355170 [details] [diff] [review]: ----------------------------------------------------------------- The overall comment that I don't think duplicating all the tests of the restrictions is necessarily the right way to go here applies to these tests as well. As a side note, there was a recent clarification around workers and CSP in the current 1.1 spec : https://github.com/w3c/webappsec/commit/63534a54245df586bf02d44ceab07e6611450d15 but this pertains to the policy applied to workers, which is not being tested here.
Attachment #8355170 - Flags: review?(ian.melven)
Comment on attachment 8355167 [details] [diff] [review] 0001-Bug-671386-Part-1-Implement-CSP-sandbox-directive.patch Review of attachment 8355167 [details] [diff] [review]: ----------------------------------------------------------------- Overall, looks good. A few small comments. I also flagged Garrett to take a look. However, I don't think there's enough tests of the more CSP specific bits of this functionality, for example, intersecting multiple policies with and without sandbox directives etc. ::: content/base/public/nsIContentSecurityPolicy.idl @@ +204,5 @@ > + * Delegate method called by the service when the protected document is loaded. > + * Responds with whether the sandbox directive is set, and fills in the value. > + * > + * @param sandboxFlags [out] > + * Set to the sandbox flags accompaning the sandbox directive. nit: typo - 'accompanying' ::: content/base/src/CSPUtils.jsm @@ +1824,5 @@ > + * Class to model sandbox flag list > + */ > + > +this.SandboxFlags = function SandboxFlags(flags) { > + this._flags = flags.sort(); why are these sorted ? is that per spec ? (just curious) @@ +1832,5 @@ > + /** > + * Generates canonical string representation of the > + * SandboxFlags. > + */ > + toString : function() { i assume this is used when outputting the CSP policy for warning/error messages and the like.. @@ +1843,5 @@ > + * another SandboxFlags > + * @returns > + * true if they have the same flags > + */ > + equals: function(that) { i assume this is used for the CSP logic that does interesections and such of directives... ::: content/base/src/contentSecurityPolicy.js @@ +806,5 @@ > + let hasSandbox = false; > + let flags = []; > + // If there are multiple policies with the "sandbox" directive, > + // all the sandbox flags are intersected. NOTE: this corresponds > + // to the _least_ flexible policy. good comment ! i might say 'permissive' instead of 'flexible' to be even clearer. ::: content/base/src/nsDocument.cpp @@ +2603,4 @@ > } > > // Figure out if we need to apply an app default CSP or a CSP from an app manifest > + nsCOMPtr<nsIPrincipal> principal = NodePrincipal(); any particular reason you changed to a nsCOMPtr here ? @@ +2755,4 @@ > } > } > > + // ----- Set sandbox flags It might be worth commenting that any sandbox flags set via iframe sandbox will already be set at this point since that happens before InitCSP is called. @@ +2761,5 @@ > + rv = csp->ShouldSandbox(strFlags, &shouldSandbox); > + NS_ENSURE_SUCCESS(rv, rv); > + > + if (shouldSandbox) { > + nsAttrValue attr; attr.ParseAtomArray(strFlags); nit: these should be on separate lines @@ +2767,5 @@ > + > + if (mSandboxFlags & SANDBOXED_ORIGIN) { > + // Reset the document principal to a null principal > + principal = do_CreateInstance("@mozilla.org/nullprincipal;1"); > + SetPrincipal(principal); bz or smaug or bholley etc. should probably look at this bit to verify doing this here is OK - i'm worried about changing the principal that has already been set, although that's based on a vague recollection that I had to avoid that when I originally did iframe sandbox.
Attachment #8355167 - Flags: review?(ian.melven) → review?(grobinson)
Thanks for looking at these! (I am boarding a plane now, so only replying to the test comments; will reply to comment 83 and fix the things you pointed out tonight/tomorrow.) Indeed the tests are copy-paste of the existing ones, only modified to add headers and new files (with corresponding headers) for cases when the attribute is changed. I am not happy with this approach either, but I went with it mainly to test the existing implementation. I think your proposal of just testing the parsing of the headers and setting the correct flags is the better way to go (we may need to expose an API that returns the document sandbox flags). This means we can get rid of all these tests; we only need to test the intersection of CSP and iframe sandbox.
(In reply to Deian Stefan from comment #84) > Thanks for looking at these! (I am boarding a plane now, so only replying to > the test comments; will reply to comment 83 and fix the things you pointed > out tonight/tomorrow.) No problem, thanks for working on this ! > Indeed the tests are copy-paste of the existing ones, only modified to > add headers and new files (with corresponding headers) for cases when > the attribute is changed. I am not happy with this approach either, > but I went with it mainly to test the existing implementation. That makes a lot of sense :) > I think your proposal of just testing the parsing of the headers and > setting the correct flags is the better way to go (we may need to > expose an API that returns the document sandbox flags). This means we > can get rid of all these tests; we only need to test the intersection > of CSP and iframe sandbox. Well, we sort-of have an API for getting something's sandbox flags (for an iframe at least).. I was wondering about this while reading your patches - when something is sandboxed via CSP, will that (and should it) update the value of the sandbox attribute ? For example, I have a document containing an iframe (a.html) and a.html is served with Content-Security-Policy: sandbox allow-scripts. If I get a.sandbox's value from JS, will it (should it) say 'allow-scripts' ? As an aside, this is another case where it's a giant pain that we can't tell from reading .sandbox whether something is sandboxed or not, because the value is an empty string :(
(In reply to Ian Melven :imelven from comment #83) Thanks for taking a look! Fixed the things you mentioned (only replying to some of the comments below): > However, I don't think there's enough tests of the more CSP specific bits of > this functionality, for example, intersecting multiple policies with and > without sandbox directives etc. Agreed, will add these tests. > ::: content/base/src/CSPUtils.jsm > @@ +1824,5 @@ > > + * Class to model sandbox flag list > > + */ > > + > > +this.SandboxFlags = function SandboxFlags(flags) { > > + this._flags = flags.sort(); > > why are these sorted ? is that per spec ? (just curious) We need to do a list comparison in 'equals'. I am doing it in the constructor for performance reasons. Since the flags are not inspectable, this only comes up when we print the CSP policy to console. We don't need to do it this way, but if we care about leaving the order intact then we should also keep duplicate and unknown flags as well, IMO. Thoughts? > @@ +1832,5 @@ > > + /** > > + * Generates canonical string representation of the > > + * SandboxFlags. > > + */ > > + toString : function() { > > i assume this is used when outputting the CSP policy for warning/error > messages and the like.. That's right, updated comment to reflect usage. > @@ +1843,5 @@ > > + * another SandboxFlags > > + * @returns > > + * true if they have the same flags > > + */ > > + equals: function(that) { > > i assume this is used for the CSP logic that does interesections and such of > directives... Like toString, CSPRep's 'equals' expects each directive to define an equals function. > ::: content/base/src/nsDocument.cpp > @@ +2603,4 @@ > > } > > > > // Figure out if we need to apply an app default CSP or a CSP from an app manifest > > + nsCOMPtr<nsIPrincipal> principal = NodePrincipal(); > > any particular reason you changed to a nsCOMPtr here ? We need a nsCOMPtr when we create a new nullprincipal. This allows us to set csp on 'principal', whether it was changed or not.
Attachment #8355167 - Attachment is obsolete: true
Attachment #8355167 - Flags: review?(sstamm)
Attachment #8355167 - Flags: review?(grobinson)
References: <bug-671389-469131@https.bugzilla.mozilla.org/> <bug-671389-469131-NEs9ox8HXs@https.bugzilla.mozilla.org/> User-Agent: Notmuch/0.16 (http://notmuchmail.org) Emacs/24.3.1 (x86_64-unknown-linux-gnu) > I think your proposal of just testing the parsing of the headers and >> setting the correct flags is the better way to go (we may need to >> expose an API that returns the document sandbox flags). This means we >> can get rid of all these tests; we only need to test the intersection >> of CSP and iframe sandbox. > > Well, we sort-of have an API for getting something's sandbox flags (for an > iframe at least).. I was wondering about this while reading your patches - when > something is sandboxed via CSP, will that (and should it) update the value of > the sandbox attribute ? > > For example, I have a document containing an iframe (a.html) and a.html is > served with Content-Security-Policy: sandbox allow-scripts. If I get > a.sandbox's value from JS, will it (should it) say 'allow-scripts' ? No it will not. Doing this in the general sense seems unsafe. For example, if a.com includes an iframe with sandbox attribute (allow-scripts) from b.com, the latter can leak information to a.com by not setting one of the flags (e.g. allow-scripts). Now a.com can inspect the attribute and learn 1 bit. Of course this is already a problem (if we leave implementation as is): allow-same-origin can be used to leak a bit (just by checking contentDocument). I think it would be useful for us to amend the spec to precisely describe the semantics -- what do you think? > As an aside, this is another case where it's a giant pain that we can't tell > from reading .sandbox whether something is sandboxed or not, because the value > is an empty string :( Our current implementation is no longer a string, but to-spec DOMSettableToken. I agree and IMO the attribute should be something along the lines of optional DOMSettableToke. Using getAttribute to check if the attribute was set is ugly...
(In reply to Ian Melven :imelven from comment #82) > As a side note, there was a recent clarification around workers and CSP in > the current 1.1 spec : > https://github.com/w3c/webappsec/commit/ > 63534a54245df586bf02d44ceab07e6611450d15 > but this pertains to the policy applied to workers, which is not being > tested here. Do we have a bug for this? (We now need InitCSP-like functionality for workers.)
Flags: needinfo?(grobinson)
Attachment #8355168 - Attachment is obsolete: true
Attachment #8355169 - Attachment is obsolete: true
Attachment #8355170 - Attachment is obsolete: true
Attachment #8355171 - Attachment is obsolete: true
Attachment #8355172 - Attachment is obsolete: true
Attachment #8355173 - Attachment is obsolete: true
Attachment #8355175 - Attachment is obsolete: true
Attachment #8355177 - Attachment is obsolete: true
Attachment #8357482 - Flags: review?(ian.melven)
Attachment #8357483 - Flags: review?(ian.melven)
Comment on attachment 8356290 [details] [diff] [review] 0001-Bug-671386-Part-1-Implement-CSP-sandbox-directive.patch Following up on Comment 86: what are your thoughts on sorting the flags, keeping duplicates, or invalid flags?
Attachment #8356290 - Flags: review?(grobinson)
> No it will not. Doing this in the general sense seems unsafe. For > example, if a.com includes an iframe with sandbox attribute > (allow-scripts) from b.com, the latter can leak information to a.com by > not setting one of the flags (e.g. allow-scripts). Now a.com can inspect > the attribute and learn 1 bit. Of course this is already a problem (if > we leave implementation as is): allow-same-origin can be used to leak > a bit (just by checking contentDocument). I think it would be useful > for us to amend the spec to precisely describe the semantics -- what > do you think? Following up, our implementation follows the semantics detailed here (thanks Dev for the ref!): http://lists.w3.org/Archives/Public/public-web-security/2011Nov/0012.html
Deian, I'll try to get to this and do another pass this weekend :)
(In reply to Deian Stefan from comment #86) > > > > +this.SandboxFlags = function SandboxFlags(flags) { > > > + this._flags = flags.sort(); > > > > why are these sorted ? is that per spec ? (just curious) > > We need to do a list comparison in 'equals'. I am doing it in the > constructor for performance reasons. Since the flags are not > inspectable, this only comes up when we print the CSP policy to > console. We don't need to do it this way, but if we care about leaving > the order intact then we should also keep duplicate and unknown flags > as well, IMO. Thoughts? I don't think it makes sense to keep duplicate an unknown flags. Sorting to make equals faster makes sense. I was just curious what the reasoning was here, you could maybe add a comment to explain the .sort perhaps. I suppose sorting could cause a problem if someone set 'sandbox' to an unsorted list value and then compared that to the set value, but that seems like a bit of an edge case I suppose. > > i assume this is used when outputting the CSP policy for warning/error > > messages and the like.. > > That's right, updated comment to reflect usage. awesome, thanks ! > > > + nsCOMPtr<nsIPrincipal> principal = NodePrincipal(); > > > > any particular reason you changed to a nsCOMPtr here ? > > We need a nsCOMPtr when we create a new nullprincipal. This allows us > to set csp on 'principal', whether it was changed or not. ah got it, thanks !
(In reply to Deian Stefan from comment #87) > > > For example, I have a document containing an iframe (a.html) and a.html is > > served with Content-Security-Policy: sandbox allow-scripts. If I get > > a.sandbox's value from JS, will it (should it) say 'allow-scripts' ? > > No it will not. Doing this in the general sense seems unsafe. For > example, if a.com includes an iframe with sandbox attribute > (allow-scripts) from b.com, the latter can leak information to a.com by > not setting one of the flags (e.g. allow-scripts). Now a.com can inspect > the attribute and learn 1 bit. Of course this is already a problem (if > we leave implementation as is): allow-same-origin can be used to leak > a bit (just by checking contentDocument). I think it would be useful > for us to amend the spec to precisely describe the semantics -- what > do you think? yes, i think the spec should define what happens here (either or both of the WHATWG HTML spec and the CSP spec). However, in your example, if a.com uses <iframe sandbox='allow-scripts'> to load a document from b.com which doesn't use CSP sandbox to set allow-scripts, the document will still have allow-scripts so I don't see what's being learned here - I may be misunderstanding your example. If allow-same-origin is set i think the game is essentially over anyways since you can use window.eval() in that case and pretty much do whatever you want. As always, I'm curious what Chrome does here wrt the sandbox attribute of an iframe mirroring the value of the framed document's CSP. Maybe Mike West knows off hand (I have cc'd him and will ping him :) ) > Our current implementation is no longer a string, but to-spec > DOMSettableToken. I agree and IMO the attribute should be something > along the lines of optional DOMSettableToke. Using getAttribute to check > if the attribute was set is ugly... yeah, we had that discussion around getAttribute on WHATWG. optional DOMSettableTokenList makes sense to me. I suppose it's good to be per spec here instead of using a string as I previously had done (and as Chrome had done at the time) - http://www.mail-archive.com/whatwg@lists.whatwg.org/msg30133.html is the thread that contains the discussion and reasoning at the time if you're curious :)
(In reply to Deian Stefan from comment #88) > (In reply to Ian Melven :imelven from comment #82) > > As a side note, there was a recent clarification around workers and CSP in > > the current 1.1 spec : > > https://github.com/w3c/webappsec/commit/ > > 63534a54245df586bf02d44ceab07e6611450d15 > > but this pertains to the policy applied to workers, which is not being > > tested here. > > Do we have a bug for this? (We now need InitCSP-like functionality for > workers.) not as far as I know. Probably best to file one if you don't mind.
Comment on attachment 8357482 [details] [diff] [review] 0002-Bug-671389-Part-2-Export-document-sandbox-flags-to-c.patch Review of attachment 8357482 [details] [diff] [review]: ----------------------------------------------------------------- Olli, could you take a look at this to see what you think please ? ::: content/base/public/nsIDocument.h @@ +522,5 @@ > > /** > + * Get string representation of sandbox flags. > + */ > + void GetSandboxFlags(nsAString& aFlags); this is a little confusing because there's a uint32_t GetSandboxFlags() as well. Maybe GetSandboxFlagsAsString ? bz or khuey or smaug might have more informed or stronger opinions here. ::: content/base/src/nsContentUtils.cpp @@ +986,5 @@ > +#define IF_FLAG(flag, atom) \ > + if (!(flags & flag)) { \ > + nsAutoString tmp; \ > + nsGkAtoms::atom->ToString(tmp); \ > + attr.Append(tmp + NS_LITERAL_STRING(" "));\ obviously there will be an extra trailing space here.. ::: dom/webidl/Document.webidl @@ +351,5 @@ > > +// Extension to give chrome JS the ability to get the underlying > +// sandbox flag attribute > +partial interface Document { > + [ChromeOnly] readonly attribute DOMString? sandboxFlags; i assume we need to do this so the chrome CSP JS can access these :)
Attachment #8357482 - Flags: review?(ian.melven)
Attachment #8357482 - Flags: review?(bugs)
Attachment #8357482 - Flags: review+
(In reply to Ian Melven :imelven from comment #98) > Comment on attachment 8357482 [details] [diff] [review] > 0002-Bug-671389-Part-2-Export-document-sandbox-flags-to-c.patch > > ::: dom/webidl/Document.webidl > @@ +351,5 @@ > > > > +// Extension to give chrome JS the ability to get the underlying > > +// sandbox flag attribute > > +partial interface Document { > > + [ChromeOnly] readonly attribute DOMString? sandboxFlags; > > i assume we need to do this so the chrome CSP JS can access these :) ah, I see this is actually so the tests can check the flag as you mentioned, since with your approach we can't use the iframe's sandbox value. I read http://lists.w3.org/Archives/Public/public-web-security/2011Nov/0012.html (which predates Firefox's iframe sandbox implementation) and the other messages in that thread (thanks and thanks to Dev for the reference - it was extremely helpful in understanding the reasoning here). So yes, I agree with the implementation the way you have it and needing this chrome only accessor :)
Comment on attachment 8357483 [details] [diff] [review] 0003-Bug-671389-Part-3-tests-for-CSP-sandbox-directive.patch Review of attachment 8357483 [details] [diff] [review]: ----------------------------------------------------------------- Overall, I think this is an improvement. file_iframe_sandbox_csp_c_if2.html is the only real question I have and there's a few nits about comments. I've flagged Bob to take a look to see what he thinks as he's been spending a lot of time with iframe sandbox tests :) ::: content/html/content/test/file_iframe_sandbox_csp_c_if2.html @@ +16,5 @@ > + } > +</script> > +<script src='file_iframe_sandbox_fail.js'></script> > +<body onLoad='window.parent.postmessage({ok: false, desc: "documents sandboxed without allow-scripts should NOT be able to run script from event handlers"}, "*");doStuff();'> > + I am sandboxed with no permissions this is not true, the ^headers^ file for this has allow-scripts - i also don't understand how these tests don't fail since they're supposed to fail if the scripts can run (which they should be able to do with allow-scripts in the header) ::: content/html/content/test/file_iframe_sandbox_csp_c_if4.html @@ +36,5 @@ > + ok(threw, "window.showModalDialog threw a JS exception and was not allowed"); > + } > +</script> > +<body onLoad="doStuff()"> > + I am sandboxed but with "allow-scripts allow-same-origin" the ^headers^ file also specifies allow-top-navigation fwiw, that shouldn't affect these tests though. ::: content/html/content/test/file_iframe_sandbox_csp_if2.html @@ +16,5 @@ > + } > +</script> > +<script src='file_iframe_sandbox_fail.js'></script> > +<body onLoad='window.parent.postmessage({ok: false, desc: "documents sandboxed without allow-scripts should NOT be able to run script from event handlers"}, "*");doStuff();'> > + I am sandboxed with "allow-same-origin" and also allow-top-navigation but that again shouldn't affect these tests ::: content/html/content/test/test_iframe_sandbox_csp.html @@ +68,5 @@ > +// 1-7) make sure that CSP header does not bypass iframe attribute > +// file_iframe_sandbox_csp_c_if1.html performs the same tests as file_iframe_sandbox_csp_c_if1.html > +// file_iframe_sandbox_csp_c_if2.html performs the same tests as file_iframe_sandbox_csp_c_if2.html > +// file_iframe_sandbox_csp_c_if3.html performs the same tests as file_iframe_sandbox_csp_c_if3.html > +// file_iframe_sandbox_csp_c_if4.html performs the same tests as file_iframe_sandbox_csp_c_if4.html these comments seem like they should be more like file_iframe_sandbox_csp_c_if1.html performs the same tests as file_iframe_sandbox_c_if1.html ? ie. the _csp_ files perform the same tests as their non CSP counterparts ? @@ +70,5 @@ > +// file_iframe_sandbox_csp_c_if2.html performs the same tests as file_iframe_sandbox_csp_c_if2.html > +// file_iframe_sandbox_csp_c_if3.html performs the same tests as file_iframe_sandbox_csp_c_if3.html > +// file_iframe_sandbox_csp_c_if4.html performs the same tests as file_iframe_sandbox_csp_c_if4.html > + > +// fails if bad or passes six nit: passes six times @@ +73,5 @@ > + > +// fails if bad or passes six > +// 8) make sure that CSP header sets the right flags > +// file_iframe_sandbox_csp_if1.html checks allow-same-origin and allow-scripts > +// file_iframe_sandbox_csp_if2.html fails if JS runs; doTest("if_7",...) checks allow-same-origin and allow-top-navigation should this be doTest("if_6", ...) ? @@ +76,5 @@ > +// file_iframe_sandbox_csp_if1.html checks allow-same-origin and allow-scripts > +// file_iframe_sandbox_csp_if2.html fails if JS runs; doTest("if_7",...) checks allow-same-origin and allow-top-navigation > +// file_iframe_sandbox_csp_if3.html checks allow-forms and allow-scripts > +// file_iframe_sandbox_csp_if4.html fails if JS runs; doTest("if_8",...) checks empty > +// file_iframe_sandbox_csp_if5.html fails if any flags were set because doTest("if_9",..." checks that the flags are null @@ +101,5 @@ > +<iframe id="if_5" src="file_iframe_sandbox_csp_if1.html" height="10" width="10"></iframe> > +<iframe id="if_6" src="file_iframe_sandbox_csp_if2.html" height="10" width="10" onload='doTest("if_6", ["allow-same-origin", "allow-top-navigation"]);'></iframe> > +<iframe id="if_7" src="file_iframe_sandbox_csp_if3.html" height="10" width="10"></iframe> > +<iframe id="if_8" src="file_iframe_sandbox_csp_if4.html" height="10" width="10" onload='doTest("if_8", [])'></iframe> > +<iframe id="if_9" src="file_iframe_sandbox_csp_if5.html" height="10" width="10" onload='doTest("if_9", null)'></iframe> it looks like the checks to see if the flags are correct are done in some of the subframes (the ones that can execute script) as well as from this parent document - any particular reason for that ?
Attachment #8357483 - Flags: review?(ian.melven)
Attachment #8357483 - Flags: review?(bobowencode)
Attachment #8357483 - Flags: feedback+
Ian, thanks a lot for all the feedback! (In reply to Ian Melven :imelven from comment #98) > this is a little confusing because there's a uint32_t GetSandboxFlags() as > well. Maybe GetSandboxFlagsAsString ? bz or khuey or smaug might have more > informed or stronger opinions here. Yes, I think that that's the right way to go. > ::: content/base/src/nsContentUtils.cpp > @@ +986,5 @@ > > +#define IF_FLAG(flag, atom) \ > > + if (!(flags & flag)) { \ > > + nsAutoString tmp; \ > > + nsGkAtoms::atom->ToString(tmp); \ > > + attr.Append(tmp + NS_LITERAL_STRING(" "));\ > > obviously there will be an extra trailing space here.. Will fix (should clean up a function in the tests as well).
(In reply to Ian Melven :imelven from comment #100) > Overall, I think this is an improvement. file_iframe_sandbox_csp_c_if2.html > is the only real question I have and there's a few nits about comments. I've > flagged Bob to take a look to see what he thinks as he's been spending a lot > of time with iframe sandbox tests :) Thanks again! > ::: content/html/content/test/file_iframe_sandbox_csp_c_if2.html > @@ +16,5 @@ > > + } > > +</script> > > +<script src='file_iframe_sandbox_fail.js'></script> > > +<body onLoad='window.parent.postmessage({ok: false, desc: "documents sandboxed without allow-scripts should NOT be able to run script from event handlers"}, "*");doStuff();'> > > + I am sandboxed with no permissions > > this is not true, the ^headers^ file for this has allow-scripts - i also > don't understand how these tests don't fail > since they're supposed to fail if the scripts can run (which they should be > able to do with allow-scripts in the header) The ^headers^ file has allow-scripts, but the iframe attribute does not: > <iframe sandbox="" id="if_2" src="file_iframe_sandbox_csp_c_if2.html" height="10" width="10"> > ::: content/html/content/test/file_iframe_sandbox_csp_c_if4.html > @@ +36,5 @@ > > + ok(threw, "window.showModalDialog threw a JS exception and was not allowed"); > > + } > > +</script> > > +<body onLoad="doStuff()"> > > + I am sandboxed but with "allow-scripts allow-same-origin" > > the ^headers^ file also specifies allow-top-navigation fwiw, that shouldn't > affect these tests though. Will fix. The allow-top-navigation was added mostly arbitrarily to make sure that parsing of the flag works. > ::: content/html/content/test/test_iframe_sandbox_csp.html > @@ +68,5 @@ > > +// 1-7) make sure that CSP header does not bypass iframe attribute > > +// file_iframe_sandbox_csp_c_if1.html performs the same tests as file_iframe_sandbox_csp_c_if1.html > > +// file_iframe_sandbox_csp_c_if2.html performs the same tests as file_iframe_sandbox_csp_c_if2.html > > +// file_iframe_sandbox_csp_c_if3.html performs the same tests as file_iframe_sandbox_csp_c_if3.html > > +// file_iframe_sandbox_csp_c_if4.html performs the same tests as file_iframe_sandbox_csp_c_if4.html > > these comments seem like they should be more like > file_iframe_sandbox_csp_c_if1.html performs the same tests as > file_iframe_sandbox_c_if1.html ? Yes, sorry for the stupid mistakes. > @@ +73,5 @@ > > + > > +// fails if bad or passes six > > +// 8) make sure that CSP header sets the right flags > > +// file_iframe_sandbox_csp_if1.html checks allow-same-origin and allow-scripts > > +// file_iframe_sandbox_csp_if2.html fails if JS runs; doTest("if_7",...) checks allow-same-origin and allow-top-navigation > > should this be doTest("if_6", ...) ? Yep, sorry again about the carelessness. > @@ +101,5 @@ > > +<iframe id="if_5" src="file_iframe_sandbox_csp_if1.html" height="10" width="10"></iframe> > > +<iframe id="if_6" src="file_iframe_sandbox_csp_if2.html" height="10" width="10" onload='doTest("if_6", ["allow-same-origin", "allow-top-navigation"]);'></iframe> > > +<iframe id="if_7" src="file_iframe_sandbox_csp_if3.html" height="10" width="10"></iframe> > > +<iframe id="if_8" src="file_iframe_sandbox_csp_if4.html" height="10" width="10" onload='doTest("if_8", [])'></iframe> > > +<iframe id="if_9" src="file_iframe_sandbox_csp_if5.html" height="10" width="10" onload='doTest("if_9", null)'></iframe> > > it looks like the checks to see if the flags are correct are done in some of > the subframes (the ones that can execute script) > as well as from this parent document - any particular reason for that ? Since some of the subframes do not have allow-scripts set, we need a way to test that the other flags are set correctly and doing it in the parent document seems like the natural approach; did you have an alternative approach in mind?
(In reply to Ian Melven :imelven from comment #96) > (In reply to Deian Stefan from comment #87) > > > > > For example, I have a document containing an iframe (a.html) and a.html is > > > served with Content-Security-Policy: sandbox allow-scripts. If I get > > > a.sandbox's value from JS, will it (should it) say 'allow-scripts' ? > > > > No it will not. Doing this in the general sense seems unsafe. For > > example, if a.com includes an iframe with sandbox attribute > > (allow-scripts) from b.com, the latter can leak information to a.com by > > not setting one of the flags (e.g. allow-scripts). Now a.com can inspect > > the attribute and learn 1 bit. Of course this is already a problem (if > > we leave implementation as is): allow-same-origin can be used to leak > > a bit (just by checking contentDocument). I think it would be useful > > for us to amend the spec to precisely describe the semantics -- what > > do you think? > > yes, i think the spec should define what happens here (either or both of the > WHATWG HTML spec and the CSP spec). I'll file bugs (same goes for that of comment 97). > However, in your example, if a.com uses > <iframe sandbox='allow-scripts'> to load a document from b.com which doesn't > use CSP sandbox to set allow-scripts, the document will still have > allow-scripts so I don't see what's being learned here - I may be > misunderstanding your example. If a.com includes an iframe as such: <iframe sandbox='allow-scripts' src="https://b.com/ifr.html"> and if ifr.html^headers^ is: Content-Security-Policy: sandbox If we update the attribute flags, then a.com learns that ifr.html did not have the "allow-scripts" flag. (This is not really a big deal since b.com can communicate with a.com in general anyway.) More importantly, we can't really change the attribute since this will affect future documents that are loaded in that iframe (and if it doesn't, but the attribute getter returns the effective sandbox flags, the semantics are a bit confusing IMO). > As always, I'm curious what Chrome does > here wrt the sandbox attribute of an iframe mirroring the value of the > framed document's CSP. Maybe Mike West knows off hand (I have cc'd him and > will ping him :) ) Just checked, Chrome does not update the flags. > yeah, we had that discussion around getAttribute on WHATWG. optional > DOMSettableTokenList makes sense to me. I suppose it's good to be per spec > here instead of using a string as I previously had done (and as Chrome had > done at the time) - > http://www.mail-archive.com/whatwg@lists.whatwg.org/msg30133.html is the > thread that contains the discussion and reasoning at the time if you're > curious :) Thanks!
(In reply to Deian Stefan from comment #102) > (In reply to Ian Melven :imelven from comment #100) > > Thanks again! No problem ! Thanks for all the clarifications. > > it looks like the checks to see if the flags are correct are done in some of > > the subframes (the ones that can execute script) > > as well as from this parent document - any particular reason for that ? > > Since some of the subframes do not have allow-scripts set, we need a way to > test that the other flags are set correctly and doing it in the parent > document seems like the natural approach; did you have an alternative > approach in mind? Nope, it makes sense that the documents without allow-scripts set need the parent to check the flags.
Attachment #8356290 - Flags: review?(sstamm)
Great we finally implemented it. But what are we gonna do with http://homakov.blogspot.com/2014/01/using-content-security-policy-for-evil.html ?
(In reply to Deian Stefan from comment #103) > > > yes, i think the spec should define what happens here (either or both of the > > WHATWG HTML spec and the CSP spec). > > I'll file bugs (same goes for that of comment 97). Thanks ! > > However, in your example, if a.com uses > > <iframe sandbox='allow-scripts'> to load a document from b.com which doesn't > > use CSP sandbox to set allow-scripts, the document will still have > > allow-scripts so I don't see what's being learned here - I may be > > misunderstanding your example. > > If a.com includes an iframe as such: > > <iframe sandbox='allow-scripts' src="https://b.com/ifr.html"> > > and if ifr.html^headers^ is: > > Content-Security-Policy: sandbox > > If we update the attribute flags, then a.com learns that ifr.html did > not have the "allow-scripts" flag. (This is not really a big deal since > b.com can communicate with a.com in general anyway.) ah, I see, thanks for elaborating. > More importantly, we can't really change the attribute since this will > affect future documents that are loaded in that iframe (and if it > doesn't, but the attribute getter returns the effective sandbox flags, > the semantics are a bit confusing IMO). right, this is what the thread you (and Dev) pointed to explains - this makes a lot of sense to me. Adam Barth also clarified on twitter that the flags set via CSP sandbox don't affect the .sandbox attribute - as he put it, 'CSP sandbox and the attribute are both inputs to the sandboxing algorithm but neither is an output".
(In reply to homakov from comment #105) > Great we finally implemented it. > > But what are we gonna do with > http://homakov.blogspot.com/2014/01/using-content-security-policy-for-evil. > html ? These are some good issues but don't relate to the CSP sandbox directive (as far as I can see, please correct me if I'm mistaken) so this bug isn't the best place to discuss them. Since they're spec/standard issues (as you said in your post), I'd suggest bringing them up on the W3C webappsec WG (which is responsible for the CSP spec) mailing list, see http://www.w3.org/2011/webappsec/ for details.
Technically it could be fixed in standard but there's no need to wait for it IMO. Simply fire "onlaod" event and remove report-uri directive... I mean, consider doing it first of all :) Someone should create a bug so I don't have to go this process from scratch
(In reply to Deian Stefan from comment #88) > Do we have a bug for this? (We now need InitCSP-like functionality for > workers.) This is partially implemented (check out dom/workers/ScriptLoader.cpp:108). It currently uses the same policy as the parent, which was the old 1.0 behavior. This will change in 1.1, and I will make a bug for it once the spec changes had solidified a bit more. Unfortuntately, the current implementation only enforces/reports 'unsafe-eval' against Workers. We should also support connect-src (bug 929292) and possibly other directives as well.
Flags: needinfo?(grobinson)
Comment on attachment 8357483 [details] [diff] [review] 0003-Bug-671389-Part-3-tests-for-CSP-sandbox-directive.patch Review of attachment 8357483 [details] [diff] [review]: ----------------------------------------------------------------- Sorry that it has taken a while for me to look at this. I've had lots of admin tasks in the run up to my first week. Generally looking good. I think it would be worth having a test that checks that all the directive flags get reflected in the document. Also, given that you've created a function to expose the documents flags, would it be worth using that instead of the copy of the *_c_if* tests. It would make it easier to test various combinations as well - to make sure that neither the CSP nor iframe sandboxing overrides the other. I've done this as feedback, as I don't think I'm the right person for the review. ::: content/html/content/test/file_iframe_sandbox_csp_c_if1.html @@ +16,5 @@ > + > + document.getElementById('a_form').submit(); > + > + // trigger the javascript: url test > + sendMouseEvent({type:'click'}, 'a_link'); Would this be better before the form submit? ::: content/html/content/test/file_iframe_sandbox_csp_c_if4.html @@ +18,5 @@ > + sendMouseEvent({type:'click'}, 'target_blank'); > + > + var threw = false; > + try { > + window.open("about:blank"); If this didn't throw, wouldn't it leave a window open? @@ +27,5 @@ > + ok(threw, "window.open threw a JS exception and was not allowed"); > + > + threw = false; > + try { > + window.showModalDialog("about:blank"); Again I think the window would be left open. ::: content/html/content/test/file_iframe_sandbox_csp_c_if4.html^headers^ @@ +1,1 @@ > +Content-Security-Policy: sandbox allow-same-origin allow-scripts allow-top-navigation Would it be worth adding allow-popups here, to be sure that it doesn't override the iframe attr? ::: content/html/content/test/test_iframe_sandbox_csp.html @@ +50,5 @@ > + if (a === null || b === null) { return false; } > + if (a.length !== b.length) { return false; } > + for (var i in a) { > + if (a[i] !== b[i]) { return false; } > + } I think this relies on the order in which you are adding the strings in the SandboxFlagsToStringAttribute function. While this will work, it might break unexpectedly, if someone changes that function. @@ +97,5 @@ > +<iframe sandbox="" id="if_2" src="file_iframe_sandbox_csp_c_if2.html" height="10" width="10"></iframe> > +<iframe sandbox="allow-forms allow-scripts" id="if_3" src="file_iframe_sandbox_csp_c_if3.html" height="10" width="10"></iframe> > +<iframe sandbox="allow-same-origin allow-scripts" id="if_4" src="file_iframe_sandbox_csp_c_if4.html" height="10" width="10"></iframe> > + > +<iframe id="if_5" src="file_iframe_sandbox_csp_if1.html" height="10" width="10"></iframe> I wonder if it worth keeping all of the flag checking (here and for if_7 below) in this parent document. That way you don't need to copy the eq, getSandboxFlags and doTest functions into those documents.
Attachment #8357483 - Flags: review?(bobowencode) → feedback+
(In reply to Bob Owen (:bobowen) from comment #110) > > + for (var i in a) { > > + if (a[i] !== b[i]) { return false; } > > + } > > I think this relies on the order in which you are adding the strings in the > SandboxFlagsToStringAttribute function. > While this will work, it might break unexpectedly, if someone changes that > function. No Bob, you're wrong ... read the for loop properly.
(In reply to Bob Owen (:bobowen) from comment #111) > (In reply to Bob Owen (:bobowen) from comment #110) > > I think this relies on the order in which you are adding the strings in the > > SandboxFlagsToStringAttribute function. > > While this will work, it might break unexpectedly, if someone changes that > > function. > > No Bob, you're wrong ... read the for loop properly. Actually Bob, I think you'll find I'm correct (sorry for the churn).
What are we doing about report-only sandboxes? Imagine a document with two CSPs (one report only, one enforced) both with different sandbox flags. What do we do? Do we bail out and apply no sandbox? Do we send reports for sandbox violations of the one policy and enforce the other? Do we enforce both? This gets complicated.
Comment on attachment 8356290 [details] [diff] [review] 0001-Bug-671386-Part-1-Implement-CSP-sandbox-directive.patch Review of attachment 8356290 [details] [diff] [review]: ----------------------------------------------------------------- Waiting to finish the review until we've settled the open issue about sandbox reporting and enforcing for report-only. ::: content/base/src/CSPUtils.jsm @@ +258,5 @@ > + "allow-popups", > + "allow-same-origin", > + "allow-scripts", > + "allow-top-navigation" > +]; Is there a way to harness the existing sandbox parser/constants instead of recapturing them here, or is it necessary to put another enum here? @@ +1825,5 @@ > + */ > + > +this.SandboxFlags = function SandboxFlags(flags) { > + this._flags = flags.sort(); > +} Please call the class CSPSandboxFlags to follow the naming convention for other classes in this file. @@ +1835,5 @@ > + */ > + toString : function() { > + return this._flags.join(" "); > + }, > + /** Nit: newline between }, and /**
Comment on attachment 8357482 [details] [diff] [review] 0002-Bug-671389-Part-2-Export-document-sandbox-flags-to-c.patch >+ /** >+ * A helper function that returns a string attribute corresponding to the >+ * sandbox flags. >+ * >+ * @param flags the sandbox flags >+ * @param attr the attribute corresponding to the flags (null if flags is 0) >+ */ >+ static void SandboxFlagsToStringAttribute(uint32_t flags, nsAString& attr); aFlags, aAttr Though, I don't understand why "attribute". Just SandboxFlagsToString(uint32_t aFlags, nsAString& aString) >+/** >+ * A helper function that returns a string attribute corresponding to the >+ * sandbox flags. >+ * >+ * @param flags the sandbox flags >+ * @param attr the attribute corresponding to the flags (null if flags is 0) >+ */ >+void >+nsContentUtils::SandboxFlagsToStringAttribute(uint32_t flags, nsAString& attr) ditto, and add documentation only to .h I think something like the following >+#define IF_FLAG(flag, atom) \ >+ if (!(aFlags & flag)) { \ >+ if (!aString.IsEmpty()) { \ >+ aString.Append(NS_NAMED_LITERAL_STRING(" ")); \ >+ } \ >+ aString.Append(nsDependentAtomString(nsGkAtoms::atom)); \ >+ } That way the result won't contain space at the end And please add some test for the document.sandboxFlags
Attachment #8357482 - Flags: review?(bugs) → review-
Olli, Ian: thanks for the review. I addressed the comments in this patch.
Attachment #8357482 - Attachment is obsolete: true
Attachment #8361307 - Flags: review?(bugs)
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #113) > What are we doing about report-only sandboxes? what a great question ! ;) IE and Chrome both have implemented CSP sandbox so as always it would be interesting to know what they do for sandbox in report-only. I can't find anything specific that discusses this situation in the spec. It may also be worth asking on the W3C webappsec list.
Ian & Bob: thanks for looking at this! (In reply to Bob Owen (:bobowen) from comment #110) > Generally looking good. > I think it would be worth having a test that checks that all the directive > flags get reflected in the document. Added. > Also, given that you've created a function to expose the documents flags, > would it be worth using that instead of the copy of the *_c_if* tests. > It would make it easier to test various combinations as well - to make sure > that neither the CSP nor iframe sandboxing overrides the other. > > ::: content/html/content/test/file_iframe_sandbox_csp_c_if1.html > @@ +16,5 @@ > > + > > + document.getElementById('a_form').submit(); > > + > > + // trigger the javascript: url test > > + sendMouseEvent({type:'click'}, 'a_link'); > > Would this be better before the form submit? > > ::: content/html/content/test/file_iframe_sandbox_csp_c_if4.html > @@ +18,5 @@ > > + sendMouseEvent({type:'click'}, 'target_blank'); > > + > > + var threw = false; > > + try { > > + window.open("about:blank"); > > If this didn't throw, wouldn't it leave a window open? > > @@ +27,5 @@ > > + ok(threw, "window.open threw a JS exception and was not allowed"); > > + > > + threw = false; > > + try { > > + window.showModalDialog("about:blank"); > > Again I think the window would be left open. > > ::: content/html/content/test/file_iframe_sandbox_csp_c_if4.html^headers^ > @@ +1,1 @@ > > +Content-Security-Policy: sandbox allow-same-origin allow-scripts allow-top-navigation > > Would it be worth adding allow-popups here, to be sure that it doesn't > override the iframe attr? I left these as is since they're 1-to-1 with the existing iframe sandbox code and these tests were meant to just do the same but with headers instead of the iframe attribute. > ::: content/html/content/test/test_iframe_sandbox_csp.html > @@ +50,5 @@ > > + if (a === null || b === null) { return false; } > > + if (a.length !== b.length) { return false; } > > + for (var i in a) { > > + if (a[i] !== b[i]) { return false; } > > + } > > I think this relies on the order in which you are adding the strings in the > SandboxFlagsToStringAttribute function. > While this will work, it might break unexpectedly, if someone changes that > function. Fixed. > @@ +97,5 @@ > > +<iframe sandbox="" id="if_2" src="file_iframe_sandbox_csp_c_if2.html" height="10" width="10"></iframe> > > +<iframe sandbox="allow-forms allow-scripts" id="if_3" src="file_iframe_sandbox_csp_c_if3.html" height="10" width="10"></iframe> > > +<iframe sandbox="allow-same-origin allow-scripts" id="if_4" src="file_iframe_sandbox_csp_c_if4.html" height="10" width="10"></iframe> > > + > > +<iframe id="if_5" src="file_iframe_sandbox_csp_if1.html" height="10" width="10"></iframe> > > I wonder if it worth keeping all of the flag checking (here and for if_7 > below) in this parent document. > That way you don't need to copy the eq, getSandboxFlags and doTest functions > into those documents. Good suggestion, fixed to do it this way.
Attachment #8357483 - Attachment is obsolete: true
Attachment #8361344 - Flags: feedback?(bobowencode)
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #113) > What are we doing about report-only sandboxes? > > Imagine a document with two CSPs (one report only, one enforced) both with > different sandbox flags. > > What do we do? Do we bail out and apply no sandbox? Do we send reports for > sandbox violations of the one policy and enforce the other? Do we enforce > both? This gets complicated. (In reply to Ian Melven :imelven from comment #117) > (In reply to Sid Stamm [:geekboy or :sstamm] from comment #113) > > What are we doing about report-only sandboxes? > > what a great question ! ;) > > IE and Chrome both have implemented CSP sandbox so as always it would be > interesting to know what they do for sandbox in report-only. I can't find > anything specific that discusses this situation in the spec. It may also be > worth asking on the W3C webappsec list. Agreed! I think these are spec-level questions. I will check what Chrome is doing today. My approach was to go about it as suggested in comment 2: first implement in enforcement mode. But I do need to check what happens when you have a report-only and an enforce header: I suspect---and want at least---the "stricter": enforce.
(In reply to Deian Stefan from comment #120) > Thanks again to Dev: > http://lists.w3.org/Archives/Public/public-webappsec/2013Jun/0118.html That seems to imply that sandbox is ignored when in the Report-Only header - it would definitely be good to get all this in the spec...
In the patch, please add some comments clearly explaining the design decision you've made about report-only mode and sandbox. This'll help clear up why the logic is the way it is (intersect sandboxes for non-report-only policies), and make it easier to understand. Probably wouldn't hurt to toss out CSPWarnings when encountering sandbox in the report-only policies so that developers know it will be disregarded. I asked because I want to make sure we do this in a sane way. Sounds pretty sane.
The requirements in the spec are all predicated with "When enforcing the `sandbox` directive...". Report-only mode doesn't "enforce", it "monitors"... the sandbox directive should be ignored when in a report-only header. It's pretty obtusely written, though: https://github.com/w3c/webappsec/commit/2cc237a696e982be59886c8f2ba0ed2d84f22c81 should clean it up a bit. Unfortunately, it was obtuse enough that we screwed it up in Blink: https://codereview.chromium.org/133073007 is up for review to fix our behavior.
(In reply to Deian Stefan from comment #118) Thanks Deian. > I left these as is since they're 1-to-1 with the existing iframe > sandbox code and these tests were meant to just do the same but with > headers instead of the iframe attribute. I'm not quite clear what you are trying to test here then. The comment: |// 1-7) make sure that CSP header does not bypass iframe attribute| seems to suggest you are testing whether the two ways of specifying sandboxing play nicely together and don't override each other's security restrictions. The first two test files partially do this, but the second two just have extra allows in the headers, that wouldn't affect the things being attempted in the files anyway. With the flags exposed on the document you can test different combinations on sandbox attribute and CSP and make sure the most restrictive set of flags is always applied. The existing tests should then cover the correct behaviour once we know the flags are correct on the document. > > I think this relies on the order in which you are adding the strings in the > > SandboxFlagsToStringAttribute function. > > While this will work, it might break unexpectedly, if someone changes that > > function. > > Fixed. Thanks - trailing space crept in on the |a.sort()| line.
Comment on attachment 8361307 [details] [diff] [review] 0002-Bug-671389-Part-2-Export-document-sandbox-flags-to-c.patch Could you change the name of the eq to equalFlags or some such. + // both attributes should be either null or have the same flags No attributes there. just arrays of flags. space before and after '+' in js code.
Attachment #8361307 - Flags: review?(bugs) → review+
(In reply to Mike West from comment #123) > The requirements in the spec are all predicated with "When enforcing the > `sandbox` directive...". Report-only mode doesn't "enforce", it > "monitors"... the sandbox directive should be ignored when in a report-only > header. Thanks Mike, but does "monitor" suggest we watch the iframe do things and then send CSP violation reports when the sandbox requirements get violated (lots of work) or that we just ignore the sandbox directive? I'd much rather just drop the directive.
Comment on attachment 8356290 [details] [diff] [review] 0001-Bug-671386-Part-1-Implement-CSP-sandbox-directive.patch Review of attachment 8356290 [details] [diff] [review]: ----------------------------------------------------------------- Please update the patch to include my suggestions from comment 114 and comment 122, then re-flag me and grobinson for review. grobinson: if you already started review, go ahead and finish, but I'm clearing the flag in case you haven't so you don't have to go at it again for the next patch.
Attachment #8356290 - Flags: review?(sstamm)
Attachment #8356290 - Flags: review?(grobinson)
Attachment #8356290 - Flags: review-
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #126) > Thanks Mike, but does "monitor" suggest we watch the iframe do things and > then send CSP violation reports when the sandbox requirements get violated > (lots of work) or that we just ignore the sandbox directive? I'd much > rather just drop the directive. Blink drops it (now). We don't have any mechanism of monitoring theoretical sandbox violations, and I don't believe we intend to add any. The spec now reads "Note that the sandbox directive will be ignored when monitoring a policy...".
Comment on attachment 8361344 [details] [diff] [review] 0003-Bug-671389-Part-3-tests-for-CSP-sandbox-directive.patch Review of attachment 8361344 [details] [diff] [review]: ----------------------------------------------------------------- Sorry, forgot to set the flag in comment 124.
Attachment #8361344 - Flags: feedback?(bobowencode) → feedback+
Thanks for the review. (In reply to Sid Stamm [:geekboy or :sstamm] from comment #114) > Comment on attachment 8356290 [details] [diff] [review] > 0001-Bug-671386-Part-1-Implement-CSP-sandbox-directive.patch > > Review of attachment 8356290 [details] [diff] [review]: > ----------------------------------------------------------------- > > Waiting to finish the review until we've settled the open issue about > sandbox reporting and enforcing for report-only. > > ::: content/base/src/CSPUtils.jsm > @@ +258,5 @@ > > + "allow-popups", > > + "allow-same-origin", > > + "allow-scripts", > > + "allow-top-navigation" > > +]; > > Is there a way to harness the existing sandbox parser/constants instead of > recapturing them here, or is it necessary to put another enum here? Not really. The cleanest thing I can think of is using nsGkAtoms::allowsameorigin, etc., but as far as I know these are not accessible to JS. Suggestions? > @@ +1825,5 @@ > > + */ > > + > > +this.SandboxFlags = function SandboxFlags(flags) { > > + this._flags = flags.sort(); > > +} > > Please call the class CSPSandboxFlags to follow the naming convention for > other classes in this file. Fixed. > @@ +1835,5 @@ > > + */ > > + toString : function() { > > + return this._flags.join(" "); > > + }, > > + /** > > Nit: newline between }, and /** Fixed. (In reply to Sid Stamm [:geekboy or :sstamm] from comment #122) > In the patch, please add some comments clearly explaining the design > decision you've made about report-only mode and sandbox. This'll help clear > up why the logic is the way it is (intersect sandboxes for non-report-only > policies), and make it easier to understand. > > Probably wouldn't hurt to toss out CSPWarnings when encountering sandbox in > the report-only policies so that developers know it will be disregarded. Done both.
Attachment #8356290 - Attachment is obsolete: true
Attachment #8362078 - Flags: review?(sstamm)
Attachment #8362078 - Flags: review?(grobinson)
(In reply to Bob Owen (:bobowen) from comment #124) > The first two test files partially do this, but the second two just have > extra allows in the headers, that wouldn't affect the things being attempted > in the files anyway. > > With the flags exposed on the document you can test different combinations > on sandbox attribute and CSP and make sure the most restrictive set of flags > is always applied. > The existing tests should then cover the correct behaviour once we know the > flags are correct on the document. Thanks, you are right. I updated the tests to be more exhaustive and address this. Mind taking another look? > Thanks - trailing space crept in on the |a.sort()| line. Fixed.
Attachment #8361307 - Attachment is obsolete: true
Attachment #8362081 - Flags: feedback?(bobowencode)
(In reply to Deian Stefan from comment #131) > Created attachment 8362081 [details] [diff] [review] > 0002-Bug-671389-Part-2-Export-document-sandbox-flags-to-c.patch > > (In reply to Bob Owen (:bobowen) from comment #124) > > The first two test files partially do this, but the second two just have > > extra allows in the headers, that wouldn't affect the things being attempted > > in the files anyway. > > > > With the flags exposed on the document you can test different combinations > > on sandbox attribute and CSP and make sure the most restrictive set of flags > > is always applied. > > The existing tests should then cover the correct behaviour once we know the > > flags are correct on the document. > > Thanks, you are right. I updated the tests to be more exhaustive and > address this. Mind taking another look? > > > Thanks - trailing space crept in on the |a.sort()| line. > > Fixed. Woops, that comment was supposed to accompany this patch. For the other patch I just addressed Olli's comment 125 and carrying r+.
Attachment #8361344 - Attachment is obsolete: true
Attachment #8362082 - Flags: feedback?(bobowencode)
Comment on attachment 8362081 [details] [diff] [review] 0002-Bug-671389-Part-2-Export-document-sandbox-flags-to-c.patch Carrying r+ from smaug.
Attachment #8362081 - Flags: feedback?(bobowencode)
Some more tests. Also for this patch I renamed the test from _bug886164_ etc to _csp_sandbox.
Attachment #8357484 - Attachment is obsolete: true
Attachment #8357484 - Flags: review?(grobinson)
Attachment #8362083 - Flags: review?(grobinson)
Comment on attachment 8362082 [details] [diff] [review] 0003-Bug-671389-Part-3-tests-for-CSP-sandbox-directive.patch Review of attachment 8362082 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Deian Stefan from comment #131) > Thanks, you are right. I updated the tests to be more exhaustive and > address this. Mind taking another look? No problem, it's good for me to start to get involved with some of the CSP stuff. I'm still not totally sure about the copying of the other sandbox tests rather than just checking the flags on the document. However, the *_csp_c_* tests now test that neither the attribute nor CSP can override the other for some flags, so that's good. As, the flags are all specified together, it's reasonable to assume that the same should follow for other flags. My reading of the spec suggests you can use CSP to sandbox any document. Do we have tests elsewhere for documents opened in normal windows? Should probably test old fashioned frames and <object>s loaded with an HTML document as well. e.g. <object data="wibble.html></object> ::: content/html/content/test/file_iframe_sandbox_csp_c_if3.html @@ +21,5 @@ > + var thing = indexedDB.open("sandbox"); > + ok(false, "documents sandboxed without allow-same-origin should NOT be able to access indexedDB"); > + } > + > + catch(e) { nit: catch should come after the curly brace above. ::: content/html/content/test/file_iframe_sandbox_csp_c_if4.html @@ +14,5 @@ > + } > + > + function doStuff() { > + ok(true, "a document sandboxed with allow-same-origin and allow-scripts should be same origin with its parent and able to run scripts " + > + "regardless of what kind of whitespace was used in its sandbox attribute"); It looks like a normal space in the attribute to me. ::: content/html/content/test/test_iframe_sandbox_csp.html @@ +68,5 @@ > +// otherwise described here > + > +// fails if bad or passes ten times > +// 1-7) make sure that CSP header does not bypass iframe attribute (first 2 frames) and vice-versa (latter 2 frames): > +// file_iframe_sandbox_csp_c_if1.html performs the same tests as file_iframe_sandbox_c_if1.html nit: trailing space.
Attachment #8362082 - Flags: feedback?(bobowencode) → feedback+
Thanks, will fix. (In reply to Bob Owen (:bobowen) from comment #135) > I'm still not totally sure about the copying of the other sandbox tests > rather than just checking the flags on the document. Which ones are you referring to? I don't think we're copying any tests anymore. The *_csp_if1.html* through *_csp_if5.html* just checks that the document has the correct flags set. The *_csp_if6.html* through *_csp_if10.html* ensure that report-only-header with sandbox attribute does not affect flags -- all it checks is the set document flags. > My reading of the spec suggests you can use CSP to sandbox any document. > Do we have tests elsewhere for documents opened in normal windows? > Should probably test old fashioned frames and <object>s loaded with an HTML > document as well. > e.g. <object data="wibble.html></object> Nope, I actually wanted to add this, but forgot over the iteration -- I'll add some tests for top-level pages.
(In reply to Deian Stefan from comment #136) > Which ones are you referring to? I don't think we're copying any tests > anymore. The *csp_c_* ones, I know they're not a direct copy any more.
Comment on attachment 8362078 [details] [diff] [review] 0001-Bug-671386-Part-1-Implement-CSP-sandbox-directive.patch Review of attachment 8362078 [details] [diff] [review]: ----------------------------------------------------------------- r=me with nits. Thanks for sticking with it, this has been a bit of a slog, I know. ::: content/base/src/CSPUtils.jsm @@ +770,5 @@ > + continue directive; > + } > + var options = dirvalue.split(/\s+/); > + var flags = []; > + options.forEach(function (opt) { Maybe put a quick comment before the forEach call to summarize what this does: "add any new sandbox flags to 'flags' and warn about invalid flags" ::: content/base/src/contentSecurityPolicy.js @@ +121,4 @@ > } > > ContentSecurityPolicy.prototype = { > + classID: Components.ID("{ac81c1ab-89f3-4887-bdf5-4a4ae327a1d8}"), Any particular reason you're changing the Class ID here and in the Manifest? I don't think it's necessary. The UUID bump in the IDL is necessary, though. @@ +802,5 @@ > + // report-only sandbox flags. > + for (let idx = 0; idx < this._policies.length; idx++) { > + let policy = this._policies[idx]; > + > + if (!policy.hasSandbox || policy._reportOnlyMode) { continue; } nit: no braces and put "continue" on its own line for consistency in this file for one-line if bodies. ::: dom/locales/en-US/chrome/security/csp.properties @@ +57,5 @@ > # eval is a name and should not be localized. > scriptFromStringBlocked = An attempt to call JavaScript from a string (by calling a function like eval) has been blocked > +# LOCALIZATION NOTE (ignoringReportOnlyDirective): > +# %1$S is the directive that is ignore in report-only mode. > +ignoringReportOnlyDirective = Ignoring directive (%1$S) in report-only mode Nit: this doesn't explain why. Maybe "Directive %1$S is not valid in report-only mode and will be ignored."
Attachment #8362078 - Flags: review?(sstamm) → review+
Comment on attachment 8362078 [details] [diff] [review] 0001-Bug-671386-Part-1-Implement-CSP-sandbox-directive.patch Review of attachment 8362078 [details] [diff] [review]: ----------------------------------------------------------------- If we're implementing CSP sandbox as "part of CSP 1.1", then we should pref off this code (security.csp.experimentalEnabled) until we've finished implementing all of the 1.1 features and a ready to flip the pref. On other other hand, the sandbox directive is an optional part of CSP 1.0, and Chrome already has it, so I think we should just go ahead and enable it by default. I second Sid's comment on making the console message more detailed. ::: content/base/src/CSPUtils.jsm @@ +250,4 @@ > CSPRep.OPTIONS_DIRECTIVE = "options"; > CSPRep.ALLOW_DIRECTIVE = "allow"; > > +// Sandbox directive for our CSP 1.1 spec compliant implementation. Nit: s/1.1/1.0 ::: content/base/src/nsDocument.cpp @@ +2780,5 @@ > rv = principal->SetCsp(csp); > NS_ENSURE_SUCCESS(rv, rv); > #ifdef PR_LOGGING > PR_LOG(gCspPRLog, PR_LOG_DEBUG, > + ("Inserted CSP into principal %p", principal.get())); Why use .get(), instead of just a dereference? (just curious, not saying this is wrong)
Attachment #8362078 - Flags: review?(grobinson) → review+
(In reply to Garrett Robinson [:grobinson] from comment #139) > > On other other hand, the sandbox directive is an optional part of CSP 1.0, and Chrome > already has it, so I think we should just go ahead and enable it by default. For what it's worth, I'm +1 on this :) IE already has CSP sandbox as well, again fwiw.
Tahnks for taking a look, guys! Carrying r+ from sstam and grobinson. (In reply to Sid Stamm [:geekboy or :sstamm] from comment #138) > Any particular reason you're changing the Class ID here and in the Manifest? > I don't think it's necessary. The UUID bump in the IDL is necessary, though. Nope, reverted; only chaning UUID in IDL now. (In reply to Garrett Robinson [:grobinson] from comment #139) > Comment on attachment 8362078 [details] [diff] [review] > 0001-Bug-671386-Part-1-Implement-CSP-sandbox-directive.patch > > Review of attachment 8362078 [details] [diff] [review]: > ----------------------------------------------------------------- > > If we're implementing CSP sandbox as "part of CSP 1.1", then we should pref > off this code (security.csp.experimentalEnabled) until we've finished > implementing all of the 1.1 features and a ready to flip the pref. On other > other hand, the sandbox directive is an optional part of CSP 1.0, and Chrome > already has it, so I think we should just go ahead and enable it by default. Kept it in by default (+1 Ian's comment). > ::: content/base/src/nsDocument.cpp > @@ +2780,5 @@ > > rv = principal->SetCsp(csp); > > NS_ENSURE_SUCCESS(rv, rv); > > #ifdef PR_LOGGING > > PR_LOG(gCspPRLog, PR_LOG_DEBUG, > > + ("Inserted CSP into principal %p", principal.get())); > > Why use .get(), instead of just a dereference? (just curious, not saying > this is wrong) 'principal' is a nsCOMPtr, so we need .get() to get at the underlying address.
Attachment #8362078 - Attachment is obsolete: true
Rebased. Carrying r+ from smaug.
Attachment #8362081 - Attachment is obsolete: true
Addressed Bob's comments (thanks!) and added top-level test.
Attachment #8362082 - Attachment is obsolete: true
Attachment #8367584 - Flags: review?(grobinson)
Rebased.
Attachment #8362083 - Attachment is obsolete: true
Attachment #8362083 - Flags: review?(grobinson)
Attachment #8367586 - Flags: review?(grobinson)
Attached file example tests (obsolete) —
Deian, I did some thinking about how these tests could be changed to use your idea of exposing of the flags on the document to the full. Hopefully this makes it really clear what the inputs to the tests are and what the expected results should be. It uses a server side js file to return the headers we want, so it also means that you don't have to add extra files for each test case. I put a couple of examples in there and I haven't got your patches at the moment, so I'm not totally sure they'll run properly, but hopefully you'll get the idea.
Attached patch test_iframe_sandbox_csp.html (obsolete) — Splinter Review
I'd like to replace the 0003*.patch with this, but I don't like the setTimeout; any thoughts on how to replace this?
Attachment #8377803 - Flags: feedback?(grobinson)
Attachment #8377803 - Attachment is patch: true
Attachment #8377803 - Attachment mime type: text/html → text/plain
I thought making the attachment a patch would bring up the normal review interface, but it did not. I really like the approach in test_iframe_sandbox_csp.html. When I run it, I get 7 test failures though - is that expected? I'll need to dig in to finding a way to get rid of that setTimeout. It seems like there might be a difference between iframe.onload and iframe window/document's onload, but I'd need to set up an experiment to confirm.
> When I run it, I get 7 test failures though - is that expected? I take that back, I didn't have the .sjs from Bob's exampleTests patch. They all pass now. I've put everything together in a new patch that cleanly applies. Calling it 0003-alt for now, in case you haven't finished merging the test cases for 0003 or something.
Attachment #8367730 - Attachment is obsolete: true
Attachment #8377803 - Attachment is obsolete: true
Attachment #8377803 - Flags: feedback?(grobinson)
(In reply to Garrett Robinson [:grobinson] from comment #148) > Created attachment 8387966 [details] [diff] [review] > 0003-alt-Bug-671389-Part-3-Tests-for-CSP-sandbox-directive.patch > > > When I run it, I get 7 test failures though - is that expected? > > I take that back, I didn't have the .sjs from Bob's exampleTests patch. They > all pass now. I've put everything together in a new patch that cleanly > applies. Calling it 0003-alt for now, in case you haven't finished merging > the test cases for 0003 or something. Thanks for taking a look. And, sorry, not sure how I missed adding the sjs to the patch. Test wise: that's all of 003 actually, I merged all the tests we had before. (004- just modifies an old test, so using Bob's suggestion won't work.) I would like to address the setTimeout issue, so I'll spend some time this weekend on it.
Comment on attachment 8367584 [details] [diff] [review] 0003-Bug-671389-Part-3-Tests-for-CSP-sandbox-directive.patch Review of attachment 8367584 [details] [diff] [review]: ----------------------------------------------------------------- Clearing review flag. We're dropping this patch in favor of the approach used in 0003-alt.
Attachment #8367584 - Flags: review?(grobinson)
Sorry for the delay. This fixes the setTimeout issue (idiot mistake on my part: checkFlags did not return a function). Additionally, it includes the top-level sandbox test. I need to rebase all the patches, but shouldn't really affect review. Should we move the test to `content/base/test/csp/` from `content/html/content/test`?
Attachment #8367584 - Attachment is obsolete: true
Attachment #8387966 - Attachment is obsolete: true
Attachment #8396801 - Flags: review?(grobinson)
Simplified top-level sandbox test to just check flags (need allow-scripts and allow-same-origin, unfortunately, but I tested without these manually), rebased, and moved to csp directory.
Attachment #8396801 - Attachment is obsolete: true
Attachment #8396801 - Flags: review?(grobinson)
Attachment #8399176 - Flags: review?(grobinson)
Rebased.
Attachment #8367586 - Attachment is obsolete: true
Attachment #8367586 - Flags: review?(grobinson)
Attachment #8399177 - Flags: review?(grobinson)
Comment on attachment 8399176 [details] [diff] [review] 0003-Bug-671389-Part-3-Tests-for-CSP-sandbox-directive.patch Review of attachment 8399176 [details] [diff] [review]: ----------------------------------------------------------------- Can you reuse (and extend) test/csp/file_csp_testserver.sjs instead of adding file_set_csp_headers.sjs? You would need to add the ability to set a report-only header (trivial). The existing script is more reusable because it allows you to pass a filename as a variable in the query string and it will return the contents of that file in the response. For test_iframe_sandbox_csp_top_1.html, it would be good to add a comment explaining why it's a separate test (because you want to test the csp directive on a document that isn't an iframe, but you have to set certain sandbox flags in order to be able to run tests in a sandbox). ::: content/base/test/csp/test_iframe_sandbox_csp.html @@ +14,5 @@ > + > + SimpleTest.waitForExplicitFinish(); > + > + function eqFlags(a, b) { > + // both a and b should be either null or have the array same flags nit: clarify this comment @@ +21,5 @@ > + if (a.length !== b.length) { return false; } > + var a_sorted = a.sort(); > + var b_sorted = b.sort(); > + for (var i in a_sorted) { > + if (a_sorted[i] !== b_sorted[i]) { console.log("FAIL D!"); return false; } remove this debugging message, or make it clearer. @@ +28,5 @@ > + } > + > + function getSandboxFlags(doc) { > + var flags = doc.sandboxFlagsAsString; > + if (flags === null) { return null; } do you need this line? @@ +40,5 @@ > + // - cspReportOnly: [null] or string corresponding to the CSP report-only sandbox flags > + // Above, we use [brackets] to denote default values. > + function CSPFlagsTest(desc, opts) { > + function ifundef(x, v) { > + return (x !== undefined) ? x : null; I think you meant s/null/v @@ +169,5 @@ > + } > + ].map(function(t) { return (new CSPFlagsTest(t.desc,t)); }); > + > + > + var testCaseIndex = -1; nit: I would init to 0, and increment it after you call runTest on the testCase (clearer IMHO).
Attachment #8399176 - Flags: review?(grobinson) → review-
Comment on attachment 8399177 [details] [diff] [review] 0004-Bug-671389-Part-4-Extend-CSP-tests-for-iframe-sandbo.patch Review of attachment 8399177 [details] [diff] [review]: ----------------------------------------------------------------- These tests could be significantly DRY'ed out using file_csp_testserver.js. You'll still need file_csp_sandbox_[1-12].html, but you can get rid of the ^headers^ files. Pass the header and .html file name to the testserver, and it will return a response with that file's contents and the given header. This will also allow you to consolidate more useful info in window.tests. You should also be able to create and load the iframes using loops instead of individually, with a similar list of tests objects like you are using in the other patch. The tests themselves look great! If we can just clean up the framework a bit, this will be ready to land. ::: content/base/test/csp/test_csp_sandbox.html @@ +87,5 @@ > + > +// a postMessage handler that is used by sandboxed iframes without > +// 'allow-same-origin' to communicate pass/fail back to this main page. > +// it expects to be called with an object like {ok: true/false, desc: > +// <description of the test> which it then forwards to ok() Nit: missing closing }. Also, this comment could be more clearly formatted/written. Can you answer the question of "why?" (why do we need a postMessage handler at all?) That is IMHO generally useful for anybody who looks at this code later. @@ +184,5 @@ > + // ... otherwise, finish > + window.examiner.remove(); > + cspTestsDone = true; > + if (iframeSandboxTestsDone) { > + SimpleTest.finish(); You also need to call window.examiner.remove(). I'd recommend making a little cleanup() function, since you need to do the same thing in the mirror case (iframeSandboxTests finish after cspTests).
Attachment #8399177 - Flags: review?(grobinson) → review-
Addressed all the comments. If there is a dummy file that we use for iframes then we should be also able to get rid of file_iframe_sandbox_csp.html.
Attachment #8399176 - Attachment is obsolete: true
Attachment #8401556 - Flags: review?(grobinson)
Forgot file_iframe_sandbox_csp.html. Sorry about that.
Attachment #8401556 - Attachment is obsolete: true
Attachment #8401556 - Flags: review?(grobinson)
Attachment #8401557 - Flags: review?(grobinson)
Addressed comments. Much cleaner :)
Attachment #8399177 - Attachment is obsolete: true
Attachment #8401663 - Flags: review?(grobinson)
Comment on attachment 8401557 [details] [diff] [review] 0003-Bug-671389-Part-3-Tests-for-CSP-sandbox-directive.patch Review of attachment 8401557 [details] [diff] [review]: ----------------------------------------------------------------- r=me if you either get rid of allow-forms for test_iframe_sandbox_csp_top_1.html, or explain why you need it. ::: content/base/test/csp/file_csp_testserver.sjs @@ +40,5 @@ > // Deliver the CSP policy encoded in the URI > response.setHeader("Content-Security-Policy", csp, false); > > + // Deliver the CSP report-only policy encoded in the URI > + response.setHeader("Content-Security-Policy-Report-Only", cspRO, false); It'd be nice to gate this setHeader and the previous one, e.g. "if (csp) { response.setHeader(...) }" so the behavior is explicit. However, since we ignore empty CSP headers, this shouldn't cause any issues. ::: content/base/test/csp/test_iframe_sandbox_csp_top_1.html @@ +10,5 @@ > +Since we need to load the SimpleTest files, we have to set the > +allow-same-origin flag. Additionally, we set the allow-scripts flag > +since we need JS to check the flags. > + > +CSP header: Content-Security-Policy: sandbox allow-forms allow-scripts allow-same-origin Why do you need allow-forms?
Attachment #8401557 - Flags: review?(grobinson) → review+
(In reply to Garrett Robinson [:grobinson] from comment #159) > It'd be nice to gate this setHeader and the previous one, e.g. "if (csp) { > response.setHeader(...) }" so the behavior is explicit. However, since we > ignore empty CSP headers, this shouldn't cause any issues. Agreed, we should do this for csp, cspRO and file. > Why do you need allow-forms? We don't. I just added allow-forms to make sure that *some other flag* is correctly set. (Really, we may want to extend this to more flags/permutation or just remove allow-forms.)
Comment on attachment 8401663 [details] [diff] [review] 0004-Bug-671389-Part-4-Extend-CSP-tests-for-iframe-sandbo.patch Review of attachment 8401663 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comments addressed. Great work, Deian! ::: content/base/test/csp/file_csp_sandbox_pass.js @@ +1,2 @@ > +function ok(result, desc) { > + window.parent.postMessage({ok: result, desc: desc}, "*"); Just a note, I don't think you need to set "*" - the test (parent of the iframe that loads this) is guaranteed to be at http://mochi.test:8888 (see http://dxr.mozilla.org/mozilla-central/source/build/pgo/server-locations.txt). You don't need to change this because it doesn't actually affect anything :) ::: content/base/test/csp/test_csp_sandbox.html @@ +121,5 @@ > +var completedTests = 0; > +var passedTests = 0; > + > +var totalTests = (function() { > + var nrTests = 0; Nit: more descriptive variable name? @@ +125,5 @@ > + var nrTests = 0; > + for(var i = 0; i < testCases.length; i++) { > + nrTests += Object.keys(testCases[i].results).length; > + } > + return nrTests+12; Why +12? Magic number needs a comment. @@ +230,5 @@ > + // Run tests: > + for(var i = 0; i < testCases.length; i++) { > + runTest(testCases[i]); > + } > + Nit: extra newline
Attachment #8401663 - Flags: review?(grobinson) → review+
I think at this point all the pieces of this have r+ ? Time for a try run ? :)
Attachment #8367580 - Attachment is obsolete: true
Attachment #8367582 - Attachment is obsolete: true
Attachment #8401557 - Attachment is obsolete: true
Attachment #8401663 - Attachment is obsolete: true
Mislabeled as bug 671386. Sorry.
Attachment #8402270 - Attachment is obsolete: true
Seems like we both ran try (https://tbpl.mozilla.org/?tree=Try&rev=43a6e39f5cb8). Anyway, finally had a chance to look at the B2G failures. Indeed the sandbox flag is not enforced in the browser app when coming from the CSP header, but it seems like this is not the only thing that is not enforced. Specifically, I just tested a simple CSP policy (default-src 'self') to see if inline scripts are blocked, and they are not. Is this expected?
Flags: needinfo?(grobinson)
(In reply to Deian Stefan from comment #169) > Is this expected? Definitely not, that warrants scrutiny. Can you test if the CSP is being created correctly (in nsDocument::InitCSP)? There was a bug a while back that had to do with conflicts between CSP from headers and CSP from a manifest, which may have regressed. The other possible problem is that the 1.0 parser isn't on by default on B2G. Your tests set the .speccompliant pref, but there are some other issues with 1.0 on B2G that are being tracked in bug 858787, and could be causing problems here.
Flags: needinfo?(grobinson)
(In reply to Garrett Robinson [:grobinson] from comment #170) > (In reply to Deian Stefan from comment #169) > > > Is this expected? > > Definitely not, that warrants scrutiny. Can you test if the CSP is being > created correctly (in nsDocument::InitCSP)? There was a bug a while back > that had to do with conflicts between CSP from headers and CSP from a > manifest, which may have regressed. The speccompliant pref was off, sorry about that (didn't realize it wasn't on by default until you mentioned). > The other possible problem is that the 1.0 parser isn't on by default on > B2G. Your tests set the .speccompliant pref, but there are some other issues > with 1.0 on B2G that are being tracked in bug 858787, and could be causing > problems here. I tested different flags manually and things seem to be fine on B2G, I think this may be a mochitest issue. Looking at various mochitest.ini files it seems like a lot of iframe sandbox tests are skipped since they fail on B2G desktop. See, e.g.,: http://dxr.mozilla.org/mozilla-central/source/content/html/content/test/mochitest.ini?from=content/html/content/test/mochitest.ini#430 Thoughts on how to proceed?
fyi, we're seeing a lot of weirdness with frames on B2G (e.g. bug 1006781 and bug bug 1009632)
Priority: P3 → P1
Is anyone still working on this? I believe this is blocking CSP1.1 compliance? It seems the change to C++ CSP code means that the actual patch is no longer valid and a whole new patch is needed. Are the tests still valid though?
(pressed submit too soon) I meant to say .. "I am hopeful the tests can still be reused"
(In reply to Devdatta Akhawe [:devd] from comment #174) > (pressed submit too soon) > > I meant to say .. "I am hopeful the tests can still be reused" I don't see why they couldn't be. Seems like this stalled out because of B2G tests - it looks like some of that has been resolved now from looking at some of the linked bugs and their linked bugs. But yeah, this would need to be ported to the new parser.
(In reply to Ian Melven :imelven from comment #175) > I don't see why they couldn't be. Seems like this stalled out because of B2G > tests - it looks like some of that has been resolved now from looking at > some of the linked bugs and their linked bugs. But yeah, this would need to > be ported to the new parser. Deian, are you still working on this? Otherwise I can take over and integrate into the new parser.
Flags: needinfo?(deian)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #176) > (In reply to Ian Melven :imelven from comment #175) > > I don't see why they couldn't be. Seems like this stalled out because of B2G > > tests - it looks like some of that has been resolved now from looking at > > some of the linked bugs and their linked bugs. But yeah, this would need to > > be ported to the new parser. > > Deian, are you still working on this? Otherwise I can take over and > integrate into the new parser. If you want, we can also chat and I can just assist you when integrating your changes into the new C++ backend of CSP.
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #177) > (In reply to Christoph Kerschbaumer [:ckerschb] from comment #176) > > (In reply to Ian Melven :imelven from comment #175) > > > I don't see why they couldn't be. Seems like this stalled out because of B2G > > > tests - it looks like some of that has been resolved now from looking at > > > some of the linked bugs and their linked bugs. But yeah, this would need to > > > be ported to the new parser. > > > > Deian, are you still working on this? Otherwise I can take over and > > integrate into the new parser. > > If you want, we can also chat and I can just assist you when integrating > your changes into the new C++ backend of CSP. Sorry for the delayed response -- been super busy lately. I think that I can spend some time on this next week (towards the end of the week). If that sounds reasonable for you guys I'm happy to keep hacking on it -- otherwise please feel free to take over. (Either way, I'd be happy to review any patches).
Flags: needinfo?(deian)
(In reply to Deian Stefan from comment #178) > (In reply to Christoph Kerschbaumer [:ckerschb] from comment #177) > > (In reply to Christoph Kerschbaumer [:ckerschb] from comment #176) > > > (In reply to Ian Melven :imelven from comment #175) > > > > I don't see why they couldn't be. Seems like this stalled out because of B2G > > > > tests - it looks like some of that has been resolved now from looking at > > > > some of the linked bugs and their linked bugs. But yeah, this would need to > > > > be ported to the new parser. > > > > > > Deian, are you still working on this? Otherwise I can take over and > > > integrate into the new parser. > > > > If you want, we can also chat and I can just assist you when integrating > > your changes into the new C++ backend of CSP. > > Sorry for the delayed response -- been super busy lately. I think > that I can spend some time on this next week (towards the end of the > week). If that sounds reasonable for you guys I'm happy to keep > hacking on it -- otherwise please feel free to take over. (Either way, > I'd be happy to review any patches). That's great, go for it. I am on PTO next week, but feel free to ping me whenever you need help finding things in the new parser.
Sorry for the delay with this. One thing that is a bit ugly with this patch is that we're concatenating the source lists (actually sandbox flags) to just use the existing parser. There may a be a cleaner way to do this, but I figure I'd run this by you first.
Attachment #8402274 - Attachment is obsolete: true
Attachment #8515783 - Flags: review?(mozilla)
Carrying r+ from smaug. This patch is just rebasing old implementation.
Attachment #8402271 - Attachment is obsolete: true
Carrying r+ from grobinson. Again, just rebase.
Attachment #8402272 - Attachment is obsolete: true
Carrying r+ from grobinson. Again, just rebase.
Attachment #8402273 - Attachment is obsolete: true
Comment on attachment 8515783 [details] [diff] [review] 0001-Bug-671389-Part-1-Implement-CSP-sandbox-directive.patch Review of attachment 8515783 [details] [diff] [review]: ----------------------------------------------------------------- Hey Deian, overall looks pretty good to me. Thanks for working on this. Clearing the review-flag because I would like to see it another time please. In general it would be great to have solid tests for that, one set of tests that uses all of the different flags available for the sandbox directive. ::: dom/base/nsCSPContext.cpp @@ +1105,5 @@ > +NS_IMETHODIMP > +nsCSPContext::ShouldSandbox(nsAString& outSandboxFlags, bool* outShouldSandbox) > +{ > + if (outShouldSandbox == nullptr) { > + return NS_ERROR_FAILURE; I don't think you need that check, the String ref can never be a nullptr. @@ +1111,5 @@ > + *outShouldSandbox = false; > + outSandboxFlags.Truncate(); > + for (uint32_t i = 0; i < mPolicies.Length(); i++) { > + *outShouldSandbox = mPolicies[i]->getSandboxFlags(outSandboxFlags); > + if (*outShouldSandbox) { Maybe it's easier if you flip the statements around. if (!*outShouldSandbox) { continue; } I guess you want continue right? Not return NS_OK like you do in the else branch. Otherwise you would just inspect the first policy, but I assume you want to inspect all of the policies for sandbox flags. @@ +1115,5 @@ > + if (*outShouldSandbox) { > + if (mPolicies[i]->getReportOnlyFlag()) { > + // sandbox directive ignored in report-only mode > + *outShouldSandbox = false; > + outSandboxFlags.Truncate(); Does *outShouldSandbox ever contain something else than 'false'; same with outSandboxFlags - they are initialized at the top but only overwritten with the same values. @@ +1119,5 @@ > + outSandboxFlags.Truncate(); > + > + nsAutoString policy; > + mPolicies[i]->toString(policy); > + const char16_t* params[] = { policy.get() }; Nit: Move the params argument closer to CSP_LogLocalizedStr. ::: dom/base/nsCSPParser.cpp @@ +748,5 @@ > > +// Helper function for parsing sandbox flags. This function solely > +// concatenates all the source list tokens (the sandbox flags) so the > +// attribute parser (nsContentUtils::ParseSandboxAttributeToFlags) can > +// use. Nit, if you add a comment on top of a method, please use: /* * ... */ @@ +756,5 @@ > + nsAutoString flags; > + > + // This is a bit ugly since we're reversing the work of the CSP > + // tokenizer, but simpler than keeping track of the consistency of > + // two parsers. You can remove that comment. @@ +759,5 @@ > + // tokenizer, but simpler than keeping track of the consistency of > + // two parsers. > + // > + // remember, srcs start at index 1 > + for (uint32_t i = 1; i < mCurDir.Length(); i++) { Please use uint32_t len = mCurDir.Length(); and then use len in the for-loop avoding to always call the expensive .Length() function. @@ +766,5 @@ > + CSPPARSERLOG(("nsCSPParser::sandboxFlagList, mCurToken: %s, mCurValue: %s", > + NS_ConvertUTF16toUTF8(mCurToken).get(), > + NS_ConvertUTF16toUTF8(mCurValue).get())); > + > + if (CSP_IsValidSandboxFlag(mCurToken)) { Please flip the cases around. if (!CSP_IsValidSandboxFlag(mCurToken)) { print warning, and continue; } @@ +768,5 @@ > + NS_ConvertUTF16toUTF8(mCurValue).get())); > + > + if (CSP_IsValidSandboxFlag(mCurToken)) { > + flags.Append(mCurToken); > + if (i != mCurDir.Length() - 1) { Also here you can then use if (i != (length -1) { ... } @@ +773,5 @@ > + flags.AppendASCII(" "); > + } > + } else { > + const char16_t* params[] = { mCurToken.get() }; > + logWarningErrorToConsole(nsIScriptError::warningFlag, "ignoringUnknownOption", Maybe we should add "couldntParseInvalidSandboxToken" to csp.propoerties which is probably more descriptive than ignoringUnkonwOption. @@ +795,4 @@ > if (CSP_IsDirective(mCurDir[0], CSP_REPORT_URI)) { > reportURIList(outSrcs); > return; > + } else if (CSP_IsDirective(mCurDir[0], CSP_SANDBOX)) { Nit: Please don't use an else if here, because it's not an either ... or ... Please add a comment and then use: if (CSP_IsDirective(...) { ... } ::: dom/base/nsCSPParser.h @@ +130,4 @@ > bool subHost(); // helper function to parse subDomains > bool subPath(nsCSPHostSrc* aCspHost); // helper function to parse paths > void reportURIList(nsTArray<nsCSPBaseSrc*>& outSrcs); // helper function to parse report-uris > + void sandboxFlagList(nsTArray<nsCSPBaseSrc*>& outSrcs); // helper function to parse sandbox flags Super Nit: Please align the // helper comments above with your comment. ::: dom/base/nsCSPUtils.cpp @@ +619,5 @@ > > +/* ===== nsCSPSandboxFlags ===================== */ > + > +nsCSPSandboxFlags::nsCSPSandboxFlags(const nsAString& aFlags) > + :mFlags(aFlags) Nit: Space between : and mFlags ::: dom/base/nsContentUtils.cpp @@ +1238,5 @@ > * a CSP directive) and converts it to the set of flags used internally. > * > + * NOTE: if new sandbox flags are added to the spec, CSPUtils.h should be > + * updated as well. > + * Super nit: Please add the Note at the end of the comment, after the arguments. ::: dom/base/nsDocument.cpp @@ +2968,5 @@ > + // Reset the document principal to a null principal > + principal = do_CreateInstance("@mozilla.org/nullprincipal;1"); > + SetPrincipal(principal); > + } > + } For that part we do need to have Sid take a look at it and then also a DOM peer. Leaving that part for the second round of reviews.
Attachment #8515783 - Flags: review?(mozilla)
Just a quick note, whenever you carry over the r+ from someone when uploading a new patch, please also set that flag in the 'details' of the patch. It's a great way of indicating what patches are already r+ by just looking at the patches on top of the bug. Thanks!
I've run our browser security testing tool (https://browseraudit.com, currently under development) on the latest Firefox 34 beta with these patches (making minor changes to file paths) and it flags an error to do with the way the patched browser handles the empty sandbox directive. After reading the CSP specification, we included a test to check that a Content-Security-Policy header with the value "sandbox" --- i.e., with no flags following the directive --- is interpreted as a request from the server that the browser add a sandbox attribute with an empty value to every iframe on the page. This leads to the most restrictive environment possible inside the iframe. (This seems to be the way Chromium interprets the CSP spec, as this is the behaviour I see in Chromium 40.0.2205.0.) However, with these Firefox patches, this Content-Security-Policy header is interpreted as if the sandbox directive was never included at all; effectively, no sandbox attribute is added to any iframe on the page, leading to the most permissive environment possible inside the iframe. Which do you think is the "correct" behaviour?
@chris this is great! What is the correct behavior is really in the iframe sandbox spec and not in CSP? What happens with a similar, nested test but inside a sandbox iframe? If the behavior between what happens with iframes and what happens with the CSP header is different, then this is a bug in this patch. If not, then that is a separate bug in the implementation of iframe sandbox and you should file it!
Thanks very much for taking a look! I think I addressed most comments in the updated patch. This new patch also takes sandbox directives across multiple headers into consideration. (In reply to Christoph Kerschbaumer [:ckerschb] from comment #184) > ::: dom/base/nsDocument.cpp > @@ +2968,5 @@ > > + // Reset the document principal to a null principal > > + principal = do_CreateInstance("@mozilla.org/nullprincipal;1"); > > + SetPrincipal(principal); > > + } > > + } > > For that part we do need to have Sid take a look at it and then also a DOM > peer. Leaving that part for the second round of reviews. Sid r+ this part before, but sounds reasonable to me.
Attachment #8515783 - Attachment is obsolete: true
Attachment #8521123 - Flags: review?(mozilla)
(In reply to Chris Novakovic from comment #186) > I've run our browser security testing tool (https://browseraudit.com, > currently under development) on the latest Firefox 34 beta with these > patches (making minor changes to file paths) and it flags an error to do > with the way the patched browser handles the empty sandbox directive. > > After reading the CSP specification, we included a test to check that a > Content-Security-Policy header with the value "sandbox" --- i.e., with no > flags following the directive --- is interpreted as a request from the > server that the browser add a sandbox attribute with an empty value to every > iframe on the page. This leads to the most restrictive environment possible > inside the iframe. (This seems to be the way Chromium interprets the CSP > spec, as this is the behaviour I see in Chromium 40.0.2205.0.) > > However, with these Firefox patches, this Content-Security-Policy header is > interpreted as if the sandbox directive was never included at all; > effectively, no sandbox attribute is added to any iframe on the page, > leading to the most permissive environment possible inside the iframe. > > Which do you think is the "correct" behaviour? The Chromium behavior sounds right IIRC from reading the spec (will double check though). Chris: I've tested this myself before and have not actually run into the issue that you are describing. Can you upload an example that is failing?
Flags: needinfo?(chris)
I've built some minimum working examples (essentially the same as the failing tests on browseraudit.com, but with all of the reporting mechanisms stripped away) that should help make it clearer what's happening: http://www.doc.ic.ac.uk/~cnovakov/csp-sandbox-test/attribute.html - web page containing an iframe with an empty "sandbox" attribute; No "Content-Security-Policy" HTTP header is served alongside the page. The iframe embeds a web page that attempts to execute a script: Chromium 40.0.2205.0: script execution blocked, with console output: "Blocked script execution in 'iframe.html' because the document's frame is sandboxed and the 'allow-scripts' permission is not set." Firefox 33.x: script execution blocked, with no console output Firefox 34.0b6 with Deian's patches: script execution blocked, with no console output http://www.doc.ic.ac.uk/~cnovakov/csp-sandbox-test/cspheader.cgi - web page containing an iframe; "Content-Security-Policy: sandbox" HTTP header is served alongside the page. The iframe embeds a web page that attempts to execute a script: Chromium 40.0.2205.0: script execution blocked, with console output: "Blocked script execution in 'iframe.html' because the document's frame is sandboxed and the 'allow-scripts' permission is not set." Firefox 33.x: script execution allowed, with console output: "Content Security Policy: Failed to parse unrecognised source sandbox" Firefox 34.0b6 with Deian's patches: script execution allowed, with console output: "Content Security Policy: Failed to parse unrecognized source sandbox" It therefore seems that Firefox does the right thing when the "sandbox" attribute is explicitly added to an iframe, but doesn't when the empty sandbox policy is implied by the "sandbox" directive in the CSP HTTP header (modulo my question about whether this is the behaviour expected by the CSP spec: I think it probably is, and the Chromium guys interpret it that way too). As far as the reason for this behaviour is concerned, it could be that when Firefox parses CSP HTTP headers, it always expects directives to have one or more tokens immediately following it (that's certainly true until you consider this edge-case). This theory is backed up by the console error message: it seems that a "source" is expected to follow "sandbox". I'm afraid I don't know enough about Firefox's internals to know what to do about this, though, sorry. If you'd like further information, or if you want me to test a revised patchset, feel free to drop me a needinfo.
Flags: needinfo?(chris)
(In reply to Chris Novakovic from comment #190) > If you'd like further information, or if you want me to test a revised > patchset, feel free to drop me a needinfo. No, this is perfect, thanks!
(In reply to Deian Stefan from comment #191) > (In reply to Chris Novakovic from comment #190) > > If you'd like further information, or if you want me to test a revised > > patchset, feel free to drop me a needinfo. > > No, this is perfect, thanks! Mind running the test with the updated patch please? [Server is down ATM.] We have tests for this and I seprately implemented this myself to double check. And, thanks for looking into this!
Flags: needinfo?(chris)
(In reply to Deian Stefan from comment #192) > Mind running the test with the updated patch please? For some reason the updated patches, although applying cleanly to the Firefox 34 betas after modifying the same file paths as in the previous patch, break compilation: which branch should I actually be applying these patches to? (Alternatively, you could take a look at the test results at browseraudit.com for your patched version: the relevent tests are in "Content Security Policy -> sandbox".)
Flags: needinfo?(chris) → needinfo?(deian)
Comment on attachment 8521123 [details] [diff] [review] 0001-Bug-671389-Part-1-Implement-CSP-sandbox-directive.patch Review of attachment 8521123 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. We still need a reviewer for the nsDocument bits. Please address my comments and r=me. Great Stuff! Also, Bug 1089912 moved the CSP code into dom/security so you have to rebase your patches. ::: dom/base/nsCSPContext.cpp @@ +1105,5 @@ > +nsCSPContext::GetCSPSandboxFlags(uint32_t *outSandboxFlags) > +{ > + if (outSandboxFlags == nullptr) { > + return NS_ERROR_FAILURE; > + } You can remove that check. @@ +1106,5 @@ > +{ > + if (outSandboxFlags == nullptr) { > + return NS_ERROR_FAILURE; > + } > + *outSandboxFlags = 0; Would it make sense to add a flag here: http://mxr.mozilla.org/mozilla-central/source/dom/base/nsSandboxFlags.h#14 instead of using 0? nsCSPPolicy::getSandboxFlags() could also return that flag as default instead of 0. ::: dom/base/nsCSPUtils.cpp @@ +1000,5 @@ > +nsCSPPolicy::getSandboxFlags() const > +{ > + for (uint32_t i = 0; i < mDirectives.Length(); i++) { > + if (mDirectives[i]->equals(CSP_SANDBOX)) { > + nsAutoString flags; Nit: Please move the declaration of the string outside of the loop and just use flags.Truncate() inside the loop.
Attachment #8521123 - Flags: review?(mozilla) → review+
Another very minor thing I noticed in 0001-Bug-671389-Part-1-Implement-CSP-sandbox-directive.patch: > +# LOCALIZATION NOTE (couldntParseInvalidSandboxFlag): > +# %1$S is the option that could not be understood > +couldntParseInvalidSandboxFlag = Coudn't parse invalid sandbox flag %1$S s/Coudn't/Couldn't/
Flags: needinfo?(deian)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #194) > Comment on attachment 8521123 [details] [diff] [review] > 0001-Bug-671389-Part-1-Implement-CSP-sandbox-directive.patch > > Review of attachment 8521123 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good to me. We still need a reviewer for the nsDocument bits. > Please address my comments and r=me. Thanks for looking at this. (And, thanks Chris) Will ask smaug or bz. Addressed most. > ::: dom/base/nsCSPContext.cpp > @@ +1105,5 @@ > > +nsCSPContext::GetCSPSandboxFlags(uint32_t *outSandboxFlags) > > +{ > > + if (outSandboxFlags == nullptr) { > > + return NS_ERROR_FAILURE; > > + } > > You can remove that check. I left this since we call this function in nsDocument from C++ land, so it is possible for it to be nullptr. > @@ +1106,5 @@ > > +{ > > + if (outSandboxFlags == nullptr) { > > + return NS_ERROR_FAILURE; > > + } > > + *outSandboxFlags = 0; > > Would it make sense to add a flag here: > http://mxr.mozilla.org/mozilla-central/source/dom/base/nsSandboxFlags.h#14 > instead of using 0? > > nsCSPPolicy::getSandboxFlags() could also return that flag as default > instead of 0. I like this. Added SANDBOXED_NONE.
Attachment #8521123 - Attachment is obsolete: true
Attachment #8523223 - Flags: review+
rebase & removing some trailing spaces
Attachment #8515784 - Attachment is obsolete: true
Attachment #8523224 - Flags: review+
rebase & rm trailing spaces
Attachment #8515785 - Attachment is obsolete: true
Attachment #8523226 - Flags: review+
rebase & rm trailing spaces
Attachment #8515786 - Attachment is obsolete: true
Attachment #8523227 - Flags: review+
Try with previous patch: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=dafa8f8b0efa Burned because of unsused static variable. This patch moved it to the .cpp file. Try with this patch: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=3044323fcd2f
Attachment #8523223 - Attachment is obsolete: true
Attachment #8524131 - Flags: review?(bugs)
Comment on attachment 8524131 [details] [diff] [review] 0001-Bug-671389-Part-1-Implement-CSP-sandbox-directive-r-.patch >@@ -2954,11 +2954,30 @@ nsDocument::InitCSP(nsIChannel* aChannel) > } > } > >+ // ----- Set sandbox flags according to CSP header >+ // The document may already have some sandbox flags set (e.g., if the >+ // document is an iframe with the sandbox attribute set). If we have a CSP >+ // sandbox directive, intersect the CSP sandbox flags with the existing >+ // flags. This corresponds to the _least_ permissive policy. >+ uint32_t cspSandboxFlags = SANDBOXED_NONE; >+ nsAutoString strFlags; You don't ever use this variable. >+# LOCALIZATION NOTE (couldntParseInvalidSandboxFlag): >+# %1$S is the option that could not be understood >+couldntParseInvalidSandboxFlag = Couldn't parse invalid sandbox flag %1$S couldNotParseInvalidSandboxFlag might be a tiny bit easier to review >+nsCSPContext::GetCSPSandboxFlags(uint32_t *outSandboxFlags) uint32_t* aOutSandboxFlags (* goes with the type, and arguments start with 'a') >+static const char* CSPStrSandboxFlags[] = { >+ "allow-same-origin", >+ "allow-forms", >+ "allow-scripts", >+ "allow-top-navigation", >+ "allow-pointer-lock", >+ "allow-popups" >+}; We really shouldn't have a copy of the list here. Somehow move http://hg.mozilla.org/mozilla-central/annotate/084441e904d1/dom/base/nsContentUtils.cpp#l1262 outside the .cpp file and then in CSP_IsValidSandboxFlag just compare the atom values to aFlag. (nsIAtom has Equals method) >+nsCSPSandboxFlags::toString(nsAString& outStr) const aOutStr >+nsCSPPolicy::getSandboxFlags() const >+{ >+ nsAutoString flags; >+ for (uint32_t i = 0; i < mDirectives.Length(); i++) { >+ if (mDirectives[i]->equals(CSP_SANDBOX)) { Is it possible to have several sandbox directives? (If so, what the behavior should be) >+class nsCSPSandboxFlags : public nsCSPBaseSrc { { goes to its own line. r- because I don't know the answer to "Is it possible to have several sandbox directives?" and because of CSPStrSandboxFlags[].
Attachment #8524131 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #201) > >+nsCSPContext::GetCSPSandboxFlags(uint32_t *outSandboxFlags) > uint32_t* aOutSandboxFlags > > (* goes with the type, and arguments start with 'a') I agree that the * goes with the type, but in general we prefix outgoing arguments with out. I think he is following that coding style practiced which we also use throughout CSP. I think 'outSandboxFlags' should be ok to use. > >+static const char* CSPStrSandboxFlags[] = { > >+ "allow-same-origin", > >+ "allow-forms", > >+ "allow-scripts", > >+ "allow-top-navigation", > >+ "allow-pointer-lock", > >+ "allow-popups" > >+}; > We really shouldn't have a copy of the list here. > Somehow move > http://hg.mozilla.org/mozilla-central/annotate/084441e904d1/dom/base/ > nsContentUtils.cpp#l1262 outside the > .cpp file and then in CSP_IsValidSandboxFlag just compare the atom values to > aFlag. > (nsIAtom has Equals method) I think that's reasonable if easy to do. If possible we should try to avoid a duplication of that list. > >+nsCSPPolicy::getSandboxFlags() const > >+{ > >+ nsAutoString flags; > >+ for (uint32_t i = 0; i < mDirectives.Length(); i++) { > >+ if (mDirectives[i]->equals(CSP_SANDBOX)) { > Is it possible to have several sandbox directives? > (If so, what the behavior should be) If we encounter a duplicate directive during parsing, we ignore the second directive and its source-expressions. See http://mxr.mozilla.org/mozilla-central/source/dom/security/nsCSPParser.cpp#798 To answer your question, it's not possible to have several sandbox directives. That part looks good to me.
What about multiple policies, each with a sandbox directive?
Thanks both, sorry for the trivial stuff. (In reply to Olli Pettay [:smaug] from comment #201) > couldNotParseInvalidSandboxFlag might be a tiny bit easier to review Agreed, but I left as is since the rest of the CSP errors use couldnt instead of couldNot. (In reply to Christoph Kerschbaumer [:ckerschb] from comment #202) > (In reply to Olli Pettay [:smaug] from comment #201) > > >+nsCSPContext::GetCSPSandboxFlags(uint32_t *outSandboxFlags) > > uint32_t* aOutSandboxFlags > > > > (* goes with the type, and arguments start with 'a') > > I agree that the * goes with the type, but in general we prefix outgoing > arguments with out. I think he is following that coding style practiced > which we also use throughout CSP. I think 'outSandboxFlags' should be ok to > use. Attached is my attempt to address this. Let me know what you think. > What about multiple policies, each with a sandbox directive? Handled, takes union -- most restricting.
Attachment #8524131 - Attachment is obsolete: true
Attachment #8524991 - Flags: feedback?(bugs)
(In reply to Devdatta Akhawe [:devd] from comment #203) > What about multiple policies, each with a sandbox directive? That's ok, we can handle multiple policies each defining a sandbox (or whatever) directive, see: http://mxr.mozilla.org/mozilla-central/source/dom/security/nsCSPContext.cpp#169 but one policy is not allowed to define the same directive twice. We also log a warning to the console in such a case. Thanks Dev for following this bug!
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #202) but in general we prefix outgoing > arguments with out. That is then not following Mozilla coding style. But if rest of the code in that files uses out*, fine.
Comment on attachment 8524991 [details] [diff] [review] 0001-Bug-671389-Part-1-Implement-CSP-sandbox-directive-r-.patch Christoph, mind taking a look at this also? Changed enough to be worth a glance.
Attachment #8524991 - Flags: review?(mozilla)
Comment on attachment 8524991 [details] [diff] [review] 0001-Bug-671389-Part-1-Implement-CSP-sandbox-directive-r-.patch Review of attachment 8524991 [details] [diff] [review]: ----------------------------------------------------------------- Hey Deian, thanks for rewriting/updating. I like this approach better. You still need smaug to review everything besides the CSP bits, r=me on those. Please don't forget to update the reviewers in the patch comment before landing. I left some comments, mostly nits. ::: dom/interfaces/security/nsIContentSecurityPolicy.idl @@ +210,5 @@ > + * Delegate method called by the service when the protected document is loaded. > + * Returns the most restricting sandbox flags contained in CSP policies. > + * > + * @return > + * sandbox flags or 0 if no sandbox directive exists Nit: maybe you can add a link to the different flags available that might be returned here. ::: dom/security/nsCSPContext.cpp @@ +1132,5 @@ > +nsCSPContext::GetCSPSandboxFlags(uint32_t* aOutSandboxFlags) > +{ > + if (aOutSandboxFlags == nullptr) { > + return NS_ERROR_FAILURE; > + } As last time, you don't need that check :-). It's not an issue, look at all the other methods in this file, do they use that check? Please remove. ::: dom/security/nsCSPParser.cpp @@ +762,5 @@ > + nsAutoString flags; > + > + uint32_t length = mCurDir.Length(); > + // remember, srcs start at index 1 > + for (uint32_t i = 1; i < length; i++) { You can use mCurDir.Length() in the loop header. If I requested that change initially than I was probably biased by reviewing JS code. No need to query the length outside the loop header. ::: dom/security/nsCSPUtils.cpp @@ +1034,5 @@ > + > + return nsContentUtils::ParseSandboxAttributeToFlags(&attr); > + } > + } > + return 0; Nit: Can we use SANDBOXED_NONE here as default return value?
Attachment #8524991 - Flags: review?(mozilla) → review+
Addressed all the comments. Left the nullptr check as per our IRC discussion.
Attachment #8524991 - Attachment is obsolete: true
Attachment #8524991 - Flags: feedback?(bugs)
Attachment #8528127 - Flags: review?(bugs)
Line no's changed because of first patch.
Attachment #8523224 - Attachment is obsolete: true
Attachment #8528128 - Flags: review+
Comment on attachment 8528127 [details] [diff] [review] 0001-Bug-671389-Part-1-Implement-CSP-sandbox-directive-r-.patch s/SandboxFlagAttrs/sSandboxFlagAttrs/ and s/SandboxFlagValues/sSandboxFlagValues/ >+ >+ uint32_t len = (sizeof(SandboxFlagAttrs) / sizeof(SandboxFlagAttrs[0])); Couldn't you use ArrayLength(SandboxFlagAttrs) here? Same also in nsContentUtils::IsValidSandboxFlag >+nsContentUtils::IsValidSandboxFlag(const nsAString& aFlag) >+{ >+ uint32_t len = (sizeof(SandboxFlagAttrs) / sizeof(SandboxFlagAttrs[0])); >+ for (uint32_t i = 0; i < len; i++) { >+ if ((*SandboxFlagAttrs[i])->Equals(aFlag)) { Do we need to care about caseness here. In the previous patch you had (aFlag.LowerCaseEqualsASCII(CSPStrSandboxFlags[i])) >+ /** >+ * A helper function that checks if a string matches a valid sandbox >+ * flag. >+ * >+ * @param aFlag the potential sandbox flag >+ * @return true if the flag is a sandbox flag >+ */ >+ static bool IsValidSandboxFlag(const nsAString& aFlag); So the comment should say whether the method does case-insensitive check or case-sensitive. (and we need to have tests for both cases) >+# LOCALIZATION NOTE (couldntParseInvalidSandboxFlag): >+# %1$S is the option that could not be understood >+couldntParseInvalidSandboxFlag = Couldn't parse invalid sandbox flag %1$S I think 'couldNotParseInvalidSandboxFlag' would be easier to read in code. I could take still one look.
Attachment #8528127 - Flags: review?(bugs) → review-
Thanks for taking a look! (In reply to Olli Pettay [:smaug] from comment #211) > Comment on attachment 8528127 [details] [diff] [review] > 0001-Bug-671389-Part-1-Implement-CSP-sandbox-directive-r-.patch > > > s/SandboxFlagAttrs/sSandboxFlagAttrs/ > and > s/SandboxFlagValues/sSandboxFlagValues/ Fixed. > >+ uint32_t len = (sizeof(SandboxFlagAttrs) / sizeof(SandboxFlagAttrs[0])); > Couldn't you use ArrayLength(SandboxFlagAttrs) here? Same also in > nsContentUtils::IsValidSandboxFlag Sorry, this was using the convention used in the CSP code. Fixed. > >+nsContentUtils::IsValidSandboxFlag(const nsAString& aFlag) > >+{ > >+ uint32_t len = (sizeof(SandboxFlagAttrs) / sizeof(SandboxFlagAttrs[0])); > >+ for (uint32_t i = 0; i < len; i++) { > >+ if ((*SandboxFlagAttrs[i])->Equals(aFlag)) { > Do we need to care about caseness here. > In the previous patch you had > (aFlag.LowerCaseEqualsASCII(CSPStrSandboxFlags[i])) > > >+ /** > >+ * A helper function that checks if a string matches a valid sandbox > >+ * flag. > >+ * > >+ * @param aFlag the potential sandbox flag > >+ * @return true if the flag is a sandbox flag > >+ */ > >+ static bool IsValidSandboxFlag(const nsAString& aFlag); > > So the comment should say whether the method does case-insensitive check or > case-sensitive. Thanks for catching this. Should be case-insensitive. Fixed. > (and we need to have tests for both cases) > >+# LOCALIZATION NOTE (couldntParseInvalidSandboxFlag): > >+# %1$S is the option that could not be understood > >+couldntParseInvalidSandboxFlag = Couldn't parse invalid sandbox flag %1$S > I think 'couldNotParseInvalidSandboxFlag' would be easier to read in code. I left this as is -- all the other CSP error use couldnt.
Attachment #8528127 - Attachment is obsolete: true
Attachment #8528486 - Flags: review?(bugs)
(In reply to Olli Pettay [:smaug] from comment #211) > So the comment should say whether the method does case-insensitive check or > case-sensitive. > (and we need to have tests for both cases) Make sure the tests cover lowercase, uppercase and mixed case
(In reply to Olli Pettay [:smaug] from comment #213) > (In reply to Olli Pettay [:smaug] from comment #211) > > So the comment should say whether the method does case-insensitive check or > > case-sensitive. > > (and we need to have tests for both cases) > Make sure the tests cover lowercase, uppercase and mixed case Yep, updating other patches that contain the tests.
Comment on attachment 8528486 [details] [diff] [review] 0001-Bug-671389-Part-1-Implement-CSP-sandbox-directive-r-.patch >+ uint32_t len = (sizeof(sSandboxFlagAttrs) / sizeof(sSandboxFlagAttrs[0])); ArrayLength >+bool >+nsContentUtils::IsValidSandboxFlag(const nsAString& aFlag) >+{ >+ for (uint32_t i = 0; i < ArrayLength(sSandboxFlagAttrs); i++) { >+ if (EqualsIgnoreASCIICase(nsDependentAtomString(*sSandboxFlagAttrs[i]), aFlag)) { aFlag.EqualsIgnoreCase(nsDependentAtomString(*sSandboxFlagAttrs[i])) would be more common. >+ if (cspSandboxFlags & SANDBOXED_ORIGIN) { >+ // If the new CSP sandbox flags do not have the allow-same-origin flag >+ // reset the document principal to a null principal >+ principal = do_CreateInstance("@mozilla.org/nullprincipal;1"); >+ SetPrincipal(principal); >+ } So this part should work just fine with document.open()/write()/close() but please add a test. So, test how document.open()/write()/close() work after a sandboxed document has been loaded. (document.open()/write()/close() needs to happen after load event) This testing is really must have thing. >+ const char16_t* params[] = { policy.get() }; >+ CSP_LogLocalizedStr(NS_ConvertUTF8toUTF16("ignoringReportOnlyDirective").get(), Wouldn't NS_LITERAL_STRING("ignoringReportOnlyDirective").get() work here? Or even just MOZ_UTF16("ignoringReportOnlyDirective")
Attachment #8528486 - Flags: review?(bugs) → review+
Attachment #8528486 - Attachment is obsolete: true
Attachment #8528749 - Flags: review+
Added test for document.write() that checks if document principals is the same, before and after the write. New addition is: file_iframe_sandbox_csp_document_write.html
Attachment #8523226 - Attachment is obsolete: true
Attachment #8528751 - Flags: review?(bugs)
Attachment #8528751 - Flags: review?(bugs) → review+
Rebase.
Attachment #8528749 - Attachment is obsolete: true
Attachment #8529280 - Flags: review+
Rebased.
Attachment #8528751 - Attachment is obsolete: true
Attachment #8529282 - Flags: review+
Deian, looks like this is ready to go ?
(In reply to Ian Melven :imelven from comment #220) > Deian, looks like this is ready to go ? Almost! Try is here: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c133c0502644 Looks like we need to go back to sizeof vs. ArrayLength since the latter can't be statically asserted.
Flags: needinfo?(bugs)
nsCSPParser.cpp:772:10: error: 'nsContentUtils Sounds like a missing #include And if static_assert cause issues for other cases, just use MOZ_ASSERT. ArrayLength is really a lot easier to read than sizeof/sizeof hacks.
Flags: needinfo?(bugs)
Fixed,thanks. Try seems good on windows: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d9f1ceeced72 This patch fixes the burning linux debug (was missing include).
Attachment #8529280 - Attachment is obsolete: true
Attachment #8530468 - Flags: review+
Looks like a failing iframe sandbox test in mochitest-2 : 1063 INFO TEST-UNEXPECTED-FAIL | /tests/dom/html/test/test_iframe_sandbox_general.html | default sandbox attribute should be an empty string - Result logged after SimpleTest.finish() 1064 INFO TEST-UNEXPECTED-FAIL | /tests/dom/html/test/test_iframe_sandbox_general.html | documents sandboxed with allow-forms should be able to submit forms - Result logged after SimpleTest.finish() 1065 INFO TEST-UNEXPECTED-FAIL | /tests/dom/html/test/test_iframe_sandbox_general.html | documents sandboxed with allow-same-origin SHOULD be able to access indexedDB - Result logged after SimpleTest.finish() A lot of purple in Windows 8 x64 debug...
Deian: is this ready to land? it looks all reviewed...
Component: DOM: Core & HTML → DOM: Security
Flags: needinfo?(deian)
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #225) > Deian: is this ready to land? it looks all reviewed... Ready to land in terms of reviews, but a failing test on try, see comment 224 - not sure if the orange is related ? Need a try push and to fix any test errors it causes (if they're actually caused by these changes).
Flags: needinfo?(deian)
(In reply to Ian Melven :imelven from comment #226) > (In reply to Sid Stamm [:geekboy or :sstamm] from comment #225) > > Deian: is this ready to land? it looks all reviewed... > > Ready to land in terms of reviews, but a failing test on try, see comment > 224 - not sure if the orange is related ? Need a try push and to fix any > test errors it causes (if they're actually caused by these changes). I'll rebase and look into it today.
Attachment #8530468 - Attachment is obsolete: true
Attachment #8554069 - Flags: review+
Attachment #8528128 - Attachment is obsolete: true
Attachment #8529282 - Attachment is obsolete: true
Attachment #8554071 - Flags: review+
Attachment #8523227 - Attachment is obsolete: true
Attachment #8554072 - Flags: review+
Attachment #8554070 - Flags: review+
Try still failing. Seems likethe issue is with the new test. I'll dig into the changes, but it's a bit hard to debug this since the mochitest is passing on my machine. Any suggestions? https://treeherder.mozilla.org/#/jobs?repo=try&revision=f63fb6af4c70
Flags: needinfo?(mozilla)
(In reply to Deian Stefan from comment #232) > Try still failing. Seems likethe issue is with the new test. I'll dig into > the changes, but it's a bit hard to debug this since the mochitest is > passing on my machine. Any suggestions? > > https://treeherder.mozilla.org/#/jobs?repo=try&revision=f63fb6af4c70 Try to wrap the cleanup into a separate function, that should fix it, see e.g.: http://mxr.mozilla.org/mozilla-central/source/dom/base/test/csp/test_nonce_source.html?force=1#61
Flags: needinfo?(mozilla)
Just added 'explicity' to the nsCSPSandboxFlags constructor.
Attachment #8554069 - Attachment is obsolete: true
Attachment #8558966 - Flags: review+
Miscounted number of tests, so try failed because SimpleTest.finish() was called too early. This was a trivial fix. My apologies. New try is good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a144cdbdc6a7
Attachment #8554070 - Attachment is obsolete: true
Attachment #8558970 - Flags: review+
Fixes comment to test count.
Attachment #8558970 - Attachment is obsolete: true
Attachment #8558973 - Flags: review+
Keywords: checkin-needed
:D Very exciting ! Thanks again for all your patience and persistence on this one, Deian !
woot! this is awesome!
Thanks for persisting with this, Devdatta/Deian - it was definitely worth it!
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Confirmed green post-backout.
Depends on: 1129953
:( It seems we have have seen issues with b2g tests and this patch in the past (see prev comments). IMHO, the sandbox directive is pretty powerful and important (Firefox is the only browser that doesn't support it). @deian do you have time to take a look at the test failures? If the failures are too complicated to fix, maybe we should disable CSP sandbox support for b2g? It seems a bunch of iframe sandbox tests are already skipped on b2g per comments above.
I'm running try on b2g now. It's a bit hard to tell from the log exactly what's going on/how this breaks. I can track it down when I have more time, but maybe the easiest thing to move forward is create a new bug for b2g and disable it for b2g for now as you're suggesting.
So some data on how this breaks. It breaks because the object passed to the JSAutoCompartment ctor from the DeserializeArrayBuffer in TCPSocketChild is null. The caller is TCPSocketChild::RecvCallback which is passing mWindowObj. So where does mWindowObj come from? It comes from TCPSocketChild::SetSocketAndWindow or TCPSocketChild::SendOpen which both do: mWindowObj = js::CheckedUnwrap(&aWindowObj.toObject()); So here's a question. Are you possibly changing the principal of a document after it's corresponding Window has already been created? If so, I would not be terribly surprised if we get weird security check failures after that point...
And in particular, what you may want to do is add logging to those two TCPSocketChild functions to see whether the CheckedUnwrap is in fact returning null. And if so, log the principal of aWindowObj, as well as the principal of the thing UncheckedUnwrap returns.
(In reply to Boris Zbarsky [:bz] from comment #247) > And in particular, what you may want to do is add logging to those two > TCPSocketChild functions to see whether the CheckedUnwrap is in fact > returning null. And if so, log the principal of aWindowObj, as well as the > principal of the thing UncheckedUnwrap returns. Thanks, Boris. We are setting the document principal (seee https://bugzilla.mozilla.org/attachment.cgi?id=8558966&action=diff#a/dom/base/nsDocument.cpp_sec3). This is helpful. I will dig into this as soon as I can.
(In reply to Deian Stefan from comment #248) > (In reply to Boris Zbarsky [:bz] from comment #247) > > And in particular, what you may want to do is add logging to those two > > TCPSocketChild functions to see whether the CheckedUnwrap is in fact > > returning null. And if so, log the principal of aWindowObj, as well as the > > principal of the thing UncheckedUnwrap returns. > > Thanks, Boris. We are setting the document principal (seee > https://bugzilla.mozilla.org/attachment.cgi?id=8558966&action=diff#a/dom/ > base/nsDocument.cpp_sec3). This is helpful. I will dig into this as soon as > I can. You can't just change the principal of a document without reloading it or coordinating with the existing window - otherwise we have no way to update the principal on the JSCompartment. Does that happen somewhere?
Deian, how are things? You got so close in landing this one, any plan/chance you want to pick up that work again and get it landed? Or would you rather like me to find someone who is going to finish that up for you?
Flags: needinfo?(deian)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #250) > Deian, how are things? You got so close in landing this one, any plan/chance > you want to pick up that work again and get it landed? Or would you rather > like me to find someone who is going to finish that up for you? I would love too, but unfortunately have too much on my plate. If there is someone that can help that would be greatly appreciated.
Flags: needinfo?(deian)
@bholley any pointers to how we would reload or update the principal on the JSCompartment? Kinda hard to repro the b2g failures in local development.
(In reply to Devdatta Akhawe [:devd] from comment #252) > @bholley any pointers to how we would reload or update the principal on the > JSCompartment? Kinda hard to repro the b2g failures in local development. Bobby, it seems Dev is willing to make some progress on that bug. Can you provide some pointers?
Flags: needinfo?(bobbyholley)
> any pointers to how we would reload or update the principal on the JSCompartment I don't believe there are any provisions for doing that right now, since nothing in the web platform up until this point has required it. In particular, the nsIPrincipal is just inheriting from JSPrincipals, and as far as Gecko is concerned what the JS engine does with JSPrincipals is a black box. They might hold pointers to them in various places or whatnot... So this is infrastructure that would need to be added somehow. How hard that is really depends on SpiderMonkey implementation details. Now in practice it looks to me like JSScript just gets its principal from the compartment, so that's pretty hopeful. So you will need to do the following, at least: 1) Add some JSAPI or jsfriendapi to change the principals of an existing compartment. 2) When doing the CSP sandbox thing, change the principal of the window's compartment, not just that of the document. And then do some of the dance that nsPrincipal::SetDomain does in terms of recomputing wrappers. 3) Audit whether there are other places in SpiderMonkey that are not the compartment that hold on to principals. 4) Figure out how this should work when document.open() creates a new Window: does it inherit the sandboxing from the document that open() got called for, or is it not sandboxed, or is this not a situation we need to worry about because sandboxing means no one except the sandboxed document can do that anyway? Whatever is decided here, make the spec say that.
Taking a step back here - are we sure we actually want to update the principal on the compartment? That's a pretty drastic thing to do, and I would really rather avoid it if at all possible. Is there a reason we can't do this work before nsGlobalWindow::SetNewDocument is called?
Flags: needinfo?(bobbyholley)
My theory is that, yes it should be doable because we do it for the iframe sandbox case. I am finding it a bit hard to follow through exactly where SetNewDocument is called and where StartDocumentLoad is called (which then calls InitCSP). It seems like we need to call this initCSP before SetNewDocument is called. @deian what do you think? You are more familiar with the patch. I am happy to do the grunt work on this, but would appreciate your help with the changes.
Flags: needinfo?(deian)
> I am finding it a bit hard to follow through exactly where SetNewDocument is called It's called from nsDocumentViewer::InitInternal as part of document viewer setup. This, in turn, would typically be called from nsDocShell::SetupNewViewer. You should be able to add things there, or to its single caller (Embed) as needed, right? StartDocumentLoad is called before this, typically from nsContentDLF::CreateDocument which is what creates the nsIContentViewer. So I would typically expect InitCSP to happen before SetNewDocument...
I'm happy to take a look at this next week and just try to get things working with this new information from Boris and Bobby. @devd: reasonable?
Flags: needinfo?(deian)
sounds great! thanks!
Whiteboard: [CSP 1.1] → [CSP 1.1],[domsecurity-backlog]
Deian, I suppose the only thing that blocked us from landing this bug was B2G treeherder issues back then, right? Any chance you wanna rebase and try to land? Otherwise I'll try to find someone else to give it a try. Would be nice to have that landed!
Flags: needinfo?(deian)
(In reply to Christoph Kerschbaumer [:ckerschb, back on April 11th] from comment #261) > Deian, I suppose the only thing that blocked us from landing this bug was > B2G treeherder issues back then, right? Any chance you wanna rebase and try > to land? Otherwise I'll try to find someone else to give it a try. Would be > nice to have that landed! This would make my life much easier! I'll rebase accordingly. If I don't get to it by the time you're back though I don't want to hold this up so please feel free to assign it to someone else.
Flags: needinfo?(deian)
Uploaded 3/4. Tried rebasing again tonight (these patches are ~10 days old), but noticed that the nsBaseSrc API changed -- mind taking a look at https://bugzilla.mozilla.org/attachment.cgi?id=8745222&action=diff#a/dom/security/nsCSPUtils.h_sec3 and giving me some pointers to where the new interface is documented/explained?
Flags: needinfo?(ckerschb)
Hey Deian, you probably figured that you need to extend nsCSPSandboxFlags by the following method: > bool visit(nsCSPSrcVisitor* aVisitor) const; which was introdocuded in Bug 1254194 - Support CSP in WebExtensions You can look at other classes within nsCSPUtils, how they implement the visitor pattern. On how to exactly implement the logic within AddonContentPolicy [1] I have to defer to Kris (:kmag). He should also probably review those bits before you land the patches here. Thanks for rebasing and working on that bug. Would be awesome to finally get the CSP sandbox up and running. [1] http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/AddonContentPolicy.cpp#207
Flags: needinfo?(ckerschb)
Deian, also, we landed CSP meta support in the meantime, so you probably also have to update the parser to ignore the sandbox directive [1] when delivered through meta [2]. I think I added a test that should fail anyway in case you would forget to update that. [1] http://mxr.mozilla.org/mozilla-central/source/dom/security/nsCSPParser.cpp#1005 [2] https://www.w3.org/TR/CSP/#directive-sandbox
oops, please ignore my last. I'm in the middle of rebasing the patches.
Rebased
Attachment #8762777 - Flags: review?(ckerschb)
Attachment #8762777 - Flags: review?(bugs)
Rebased
Attachment #8762778 - Flags: review?(bugs)
Rebased
Attachment #8762779 - Flags: review?(garrett.f.robinson+mozilla)
Attachment #8762779 - Flags: review?(bugs)
Rebased
Attachment #8762780 - Flags: review?(garrett.f.robinson+mozilla)
So are the patches here just rebased, already reviewed patches? Want to upload interdiffs?
These are the 4 patches as posted by Deian, rebased to work cleanly with the tip. All the existing and new csp tests all pass locally for me
And the previous ones were reviewed (r+)? And does the rebasing include any significant changes to the code? interdiffs would be nice to ease reviewing.
I'm having great difficulty in creating an interdiff because majority of the old patches cannot be applied against the current code I mostly had to rebase by hand, partly because of changes made in Bug 1270648 which deleted the nsContentUtils::ParseSandboxAttributeToFlags method and incorporated its logic into HTMLIFrameElement::GetSandboxFlags I moved the logic back into nsContentUtils::ParseSandboxAttributeToFlags, but kept the mechanism of including IframeSandboxKeywordList.h within a local definition for SANDBOX_KEYWORD, rather than reintroducing the cumbersome lists of atoms and flags which would otherwise need to be kept synchronized. So nsContentUtils::ParseSandboxAttributeToFlags, nsContentUtils::IsValidSandboxFlag, and nsContentUtils::SandboxFlagsToString all use this same mechanism, rather than the previous mechanism of enumerating through arrays There was a minor change needed to make the code ignore the sandbox directive when read from meta, but this just used the existing mechanism, and the test that :ckerschb added to find this did fail without that change (as expected) and now passes.
ok, thanks. Interdiff fails annoyingly often. I'll do it manually then (comparing the old and new patches in separate tabs or in an editor or something).
Attachment #8554072 - Attachment is obsolete: true
Attachment #8745222 - Attachment is obsolete: true
Attachment #8745223 - Attachment is obsolete: true
Attachment #8745224 - Attachment is obsolete: true
Attachment #8762747 - Attachment is obsolete: true
Comment on attachment 8762779 [details] [diff] [review] Patch 3 - Tests for CSP sandbox directive Stealing those from Garrett - not sure if I'll get to it this week though!
Attachment #8762779 - Flags: review?(garrett.f.robinson+mozilla) → review?(ckerschb)
Attachment #8762780 - Flags: review?(garrett.f.robinson+mozilla) → review?(ckerschb)
Ok, clearly some bugs to fix, I'll (hopefully) sort these out today
Comment on attachment 8762777 [details] [diff] [review] Patch 1 - Implement CSP sandbox directive clearing the review request while waiting for new patches which fix the issues on try (perhaps the issues are in the tests?)
Attachment #8762777 - Flags: review?(bugs)
The issues are in the code, and localization strings. I'll try to get the new patches up this weekend.
(In reply to Boris Zbarsky [:bz] from comment #258) > > 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... Given that it looks like this may not have always been the case for B2G, would it be a good idea to add something like the following to the top of InitCSP? MOZ_ASSERT(!mScriptGlobalObject, "CSP must be initialized before our script global is set.");
Flags: needinfo?(bzbarsky)
> Given that it looks like this may not have always been the case for B2G I recall that InitCSP in general used to be called at a slightly different time. But yes, adding such an assert is a good idea.
Flags: needinfo?(bzbarsky)
Comment on attachment 8762779 [details] [diff] [review] Patch 3 - Tests for CSP sandbox directive Review of attachment 8762779 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/security/test/csp/mochitest.ini @@ +163,5 @@ > !/image/test/mochitest/blue.png > file_meta_whitespace_skipping.html > + test_iframe_sandbox_csp.html > + test_iframe_sandbox_csp_top_1.html^headers^ > + file_iframe_sandbox_csp_document_write.html nit: remove the '_csp_' from the filename, those are now already bundled within /test/csp/ @@ +171,5 @@ > [test_connect-src.html] > [test_CSP.html] > [test_allow_https_schemes.html] > skip-if = buildapp == 'b2g' #no ssl support > [test_bug663567.html] Are you sure this works? As far as I know the main testfile needs to be added in square brackets (usually appended at the end of the list, like in this case: [test_iframe_sandbox.html] and support files need to be added (at the end of the list) under 'support-files =', like in this case: file_iframe_sandbox_document_write.html and the ^headers^ file is served for the file that is named the same, I don't see any file that is named: test_iframe_sandbox_csp_top_1.html hence I am not sure how the headers could be served correctly. Am I missing something?
Attachment #8762779 - Flags: review?(ckerschb)
Comment on attachment 8762780 [details] [diff] [review] Patch 4 - Extend CSP tests for iframe sandbox with CSP sandbox directive tests Review of attachment 8762780 [details] [diff] [review]: ----------------------------------------------------------------- Haven't looked closely as of now (but seems to be fine); since you are already on it updating the all the patches, please fix my nits and feel free to reflag me - thanks! ::: dom/security/test/csp/mochitest.ini @@ +177,5 @@ > + file_csp_sandbox_8.html > + file_csp_sandbox_9.html > + file_csp_sandbox_10.html > + file_csp_sandbox_11.html > + file_csp_sandbox_12.html nit: please remove the '_csp_' from all the files, not needed anymore since the files are now bundled within dom/security/test/csp @@ +264,5 @@ > [test_block_all_mixed_content_frame_navigation.html] > tags = mcb > [test_form_action_blocks_url.html] > [test_meta_whitespace_skipping.html] > +[test_csp_sandbox.html] same here - thanks!
Attachment #8762780 - Flags: review?(ckerschb)
Fix mochitest.ini and rename files - AFAICT this was never previously correct.
Attachment #8762779 - Attachment is obsolete: true
Attachment #8763668 - Flags: review+
Fix names
Attachment #8762780 - Attachment is obsolete: true
Attachment #8763669 - Flags: review+
Paul - thanks for working on this. Would you mind adding the MOZ_ASSERT I mentioned in comment 287 to the top of InitCSP as part of the first patch. This should help to ensure that nsDocument::InitCSP is only called before nsGlobalWindow::SetNewDocument as per bholley's comment 255.
Assignee: deian → pacaro
Flags: needinfo?(pacaro)
Bob - yes, I have this in progress. When I've tested locally, I'll attach the patch (a couple of hours likely)
Adds MOZ_ASSERT per comments by :bobowen
Attachment #8763392 - Attachment is obsolete: true
Flags: needinfo?(pacaro)
AFAICT the failures are all intermittent or build errors. I believe that this is now ready for review.
Flags: needinfo?(ckerschb)
Flags: needinfo?(bugs)
Then ask reviews :) Could you perhaps explain what has changed in the patches? (maybe even interdiffs if possible)
Flags: needinfo?(bugs)
(In reply to Paul C Roberts from comment #297) > AFAICT the failures are all intermittent or build errors. I believe that > this is now ready for review. Look like intermittents to me as well.
Flags: needinfo?(ckerschb)
Comment on attachment 8764001 [details] [diff] [review] Patch 1 - Implement CSP sandbox directive @smaug see https://bugzilla.mozilla.org/show_bug.cgi?id=671389#c278 for whats changed: the interdiff is tricky and this is mostly a hand rebase.
Attachment #8764001 - Flags: review?(ckerschb)
Attachment #8764001 - Flags: review?(bugs)
Attachment #8763394 - Flags: review?(ckerschb)
Attachment #8763394 - Flags: review?(bugs)
Attached patch Implement CSP sandbox directive (obsolete) — Splinter Review
# This is a manually created interdiff between the last patch submitted by :deian and myself diff a/dom/base/nsContentUtils.cpp b/dom/base/nsContentUtils.cpp --- a/dom/base/nsContentUtils.cpp +++ b/dom/base/nsContentUtils.cpp @@ @@ nsContentUtils::GetParserService() return sParserService; } -static nsIAtom** sSandboxFlagAttrs[] = { - &nsGkAtoms::allowsameorigin, // SANDBOXED_ORIGIN - &nsGkAtoms::allowforms, // SANDBOXED_FORMS - &nsGkAtoms::allowscripts, // SANDBOXED_SCRIPTS | SANDBOXED_AUTOMATIC_FEATURES - &nsGkAtoms::allowtopnavigation, // SANDBOXED_TOPLEVEL_NAVIGATION - &nsGkAtoms::allowpointerlock, // SANDBOXED_POINTER_LOCK - &nsGkAtoms::alloworientationlock, // SANDBOXED_ORIENTATION_LOCK - &nsGkAtoms::allowpopups // SANDBOXED_AUXILIARY_NAVIGATION -}; - -static const uint32_t sSandboxFlagValues[] = { - SANDBOXED_ORIGIN, // allow-same-origin - SANDBOXED_FORMS, // allow-forms - SANDBOXED_SCRIPTS | SANDBOXED_AUTOMATIC_FEATURES, // allow-scripts - SANDBOXED_TOPLEVEL_NAVIGATION, // allow-top-navigation - SANDBOXED_POINTER_LOCK, // allow-pointer-lock - SANDBOXED_ORIENTATION_LOCK, // alloworientationlock - SANDBOXED_AUXILIARY_NAVIGATION // allow-popups -}; - /** * A helper function that parses a sandbox attribute (of an <iframe> or a CSP * directive) and converts it to the set of flags used internally. * * @param aSandboxAttr the sandbox attribute * @return the set of flags (SANDBOXED_NONE if aSandboxAttr is * null) */ uint32_t nsContentUtils::ParseSandboxAttributeToFlags(const nsAttrValue* aSandboxAttr) +{ - // No sandbox attribute, no sandbox flags. - if (!aSandboxAttr) { return SANDBOXED_NONE; } + if (!aSandboxAttr) { + return SANDBOXED_NONE; + } - // Start off by setting all the restriction flags. - uint32_t out = SANDBOXED_NAVIGATION @@ @@ Presumably all the SANDBOXED_* flags were here... - | SANDBOXED_ORIENTATION_LOCK - | SANDBOXED_DOMAIN; - - MOZ_ASSERT(ArrayLength(sSandboxFlagAttrs) == ArrayLength(sSandboxFlagValues), - "Lengths of SandboxFlagAttrs and SandboxFlagvalues do not match"); - - // For each flag: if it's in the attribute, update the (out) flag - for (uint32_t i = 0; i < ArrayLength(sSandboxFlagAttrs); i++) { - if (aSandboxAttr->Contains(*sSandboxFlagAttrs[i], eIgnoreCase)) { - out &= ~(sSandboxFlagValues[i]); - } - } + uint32_t out = SANDBOX_ALL_FLAGS; + +#define SANDBOX_KEYWORD(string, atom, flags) \ + if (aSandboxAttr->Contains(nsGkAtoms::atom, eIgnoreCase)) { \ + out &= ~(flags); \ + } +#include "IframeSandboxKeywordList.h" +#undef SANDBOX_KEYWORD + return out; } /** * 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. */ bool nsContentUtils::IsValidSandboxFlag(const nsAString& aFlag) { - for (uint32_t i = 0; i < ArrayLength(sSandboxFlagAttrs); i++) { - if (EqualsIgnoreASCIICase(nsDependentAtomString(*sSandboxFlagAttrs[i]), aFlag)) { - return true; - } - } +#define SANDBOX_KEYWORD(string, atom, flags) \ + if (EqualsIgnoreASCIICase(nsDependentAtomString(nsGkAtoms::atom), aFlag)) { \ + return true; \ + } +#include "IframeSandboxKeywordList.h" +#undef SANDBOX_KEYWORD return false; } nsIBidiKeyboard* nsContentUtils::GetBidiKeyboard() { diff --git a/dom/base/nsContentUtils.h b/dom/base/nsContentUtils.h --- a/dom/base/nsContentUtils.h +++ b/dom/base/nsContentUtils.h @@ @@ Nothing to see here diff --git a/dom/base/nsDocument.cpp b/dom/base/nsDocument.cpp --- a/dom/base/nsDocument.cpp +++ b/dom/base/nsDocument.cpp @@ @@ nsDocument::ApplySettingsFromCSP(bool aS NS_ENSURE_SUCCESS_VOID(rv); } } } nsresult nsDocument::InitCSP(nsIChannel* aChannel) { + MOZ_ASSERT(!mScriptGlobalObject, + "CSP must be initialized before mScriptGlobalObject is set!"); if (!CSPService::sCSPEnabled) { MOZ_LOG(gCspPRLog, LogLevel::Debug, ("CSP is disabled, skipping CSP init for document %p", this)); return NS_OK; } nsAutoCString tCspHeaderValue, tCspROHeaderValue; diff --git a/dom/base/nsSandboxFlags.h b/dom/base/nsSandboxFlags.h --- a/dom/base/nsSandboxFlags.h +++ b/dom/base/nsSandboxFlags.h @@ @@ Nothing to see here diff --git a/dom/html/HTMLIFrameElement.cpp b/dom/html/HTMLIFrameElement.cpp --- a/dom/html/HTMLIFrameElement.cpp +++ b/dom/html/HTMLIFrameElement.cpp @@ @@ HTMLIFrameElement::UnsetAttr(int32_t aNa } uint32_t HTMLIFrameElement::GetSandboxFlags() { const nsAttrValue* sandboxAttr = GetParsedAttr(nsGkAtoms::sandbox); // No sandbox attribute, no sandbox flags. if (!sandboxAttr) { - return 0; + return SANDBOXED_NONE; } - // Start off by setting all the restriction flags. - uint32_t out = SANDBOX_ALL_FLAGS; - -// Macro for updating the flag according to the keywords -#define SANDBOX_KEYWORD(string, atom, flags) \ - if (sandboxAttr->Contains(nsGkAtoms::atom, eIgnoreCase)) { out &= ~(flags); } -#include "IframeSandboxKeywordList.h" -#undef SANDBOX_KEYWORD + uint32_t out = nsContentUtils::ParseSandboxAttributeToFlags(sandboxAttr); if (GetParsedAttr(nsGkAtoms::allowfullscreen) || GetParsedAttr(nsGkAtoms::mozallowfullscreen)) { out &= ~SANDBOXED_FULLSCREEN; } return out; } iff --git a/dom/interfaces/security/nsIContentSecurityPolicy.idl b/dom/interfaces/security/nsIContentSecurityPolicy.idl --- a/dom/interfaces/security/nsIContentSecurityPolicy.idl +++ b/dom/interfaces/security/nsIContentSecurityPolicy.idl @@ @@ interface nsIURI; typedef unsigned short CSPDirective; -[scriptable, builtinclass, uuid(471c83ab-5710-45ab-ac1a-f5bbe40a6820)] +[scriptable, builtinclass, uuid(b3c4c0ae-bd5e-4cad-87e0-8d210dbb3f9f)] interface nsIContentSecurityPolicy : nsISerializable { /** @@ -55,16 +55,17 @@ interface nsIContentSecurityPolicy : nsI const unsigned short BLOCK_ALL_MIXED_CONTENT = 19; const unsigned short REQUIRE_SRI_FOR = 20; - const unsigned short SANDBOX_DIRECTIVE = 20; + const unsigned short SANDBOX_DIRECTIVE = 21; /** * Accessor method for a read-only string version of the policy at a given * index. */ diff --git a/dom/locales/en-US/chrome/security/csp.properties b/dom/locales/en-US/chrome/security/csp.properties --- a/dom/locales/en-US/chrome/security/csp.properties +++ b/dom/locales/en-US/chrome/security/csp.properties @@ @@ # LOCALIZATION NOTE (ignoringDirectiveWithNoValues): # %1$S is the name of a CSP directive that requires additional values (e.g., 'require-sri-for') ignoringDirectiveWithNoValues = Ignoring ‘%1$S‘ since it does not contain any parameters. # LOCALIZATION NOTE (ignoringReportOnlyDirective): -# %1$S is the directive that is ignore in report-only mode. +# %1$S is the directive that is ignored in report-only mode. -ignoringReportOnlyDirective = Ignoring sandbox directive when delivered in a report-only policy '%1$S'. +ignoringReportOnlyDirective = Ignoring sandbox directive when delivered in a report-only policy ‘%1$S’ # LOCALIZATION NOTE (deprecatedDirective): # %1$S is the name of the deprecated directive, %2$S is the name of the replacement. deprecatedDirective = Directive ‘%1$S’ has been deprecated. Please use directive ‘%2$S’ instead. # LOCALIZATION NOTE (couldntParseInvalidSandboxFlag): # %1$S is the option that could not be understood -couldntParseInvalidSandboxFlag = Couldn't parse invalid sandbox flag %1$S +couldntParseInvalidSandboxFlag = Couldn’t parse invalid sandbox flag ‘%1$S’ diff --git a/dom/security/nsCSPContext.cpp b/dom/security/nsCSPContext.cpp --- a/dom/security/nsCSPContext.cpp +++ b/dom/security/nsCSPContext.cpp @@ @@ Nothing to see here diff --git a/dom/security/nsCSPParser.cpp b/dom/security/nsCSPParser.cpp --- a/dom/security/nsCSPParser.cpp +++ b/dom/security/nsCSPParser.cpp @@ @@ nsCSPParser::directiveName() if (mDeliveredViaMetaTag && ((CSP_IsDirective(mCurToken, nsIContentSecurityPolicy::REPORT_URI_DIRECTIVE)) || - (CSP_IsDirective(mCurToken, nsIContentSecurityPolicy::FRAME_ANCESTORS_DIRECTIVE)))) { + (CSP_IsDirective(mCurToken, nsIContentSecurityPolicy::FRAME_ANCESTORS_DIRECTIVE)) || + (CSP_IsDirective(mCurToken, nsIContentSecurityPolicy::SANDBOX_DIRECTIVE)))) { // log to the console to indicate that meta CSP is ignoring the directive const char16_t* params[] = { mCurToken.get() }; logWarningErrorToConsole(nsIScriptError::warningFlag, diff --git a/dom/security/nsCSPParser.h b/dom/security/nsCSPParser.h --- a/dom/security/nsCSPParser.h +++ b/dom/security/nsCSPParser.h @@ @@ Nothing to see here diff --git a/dom/security/nsCSPUtils.cpp b/dom/security/nsCSPUtils.cpp --- a/dom/security/nsCSPUtils.cpp +++ b/dom/security/nsCSPUtils.cpp @@ @@ nsCSPSandboxFlags::~nsCSPSandboxFlags() } + +bool +nsCSPSandboxFlags::visit(nsCSPSrcVisitor* aVisitor) const +{ + return false; +} void @@ -948,16 +974,21 @@ nsCSPDirective::toDomCSPStruct(mozilla:: case nsIContentSecurityPolicy::CHILD_SRC_DIRECTIVE: outCSP.mChild_src.Construct(); outCSP.mChild_src.Value() = mozilla::Move(srcs); return; + case nsIContentSecurityPolicy::SANDBOX_DIRECTIVE: + outCSP.mSandbox.Construct(); + outCSP.mSandbox.Value() = mozilla::Move(srcs); + return; + // REFERRER_DIRECTIVE and REQUIRE_SRI_FOR are handled in nsCSPPolicy::toDomCSPStruct() diff --git a/dom/security/nsCSPUtils.h b/dom/security/nsCSPUtils.h --- a/dom/security/nsCSPUtils.h +++ b/dom/security/nsCSPUtils.h @@ @@ class nsCSPReportURI : public nsCSPBaseSrc { nsCOMPtr<nsIURI> mReportURI; }; /* =============== nsCSPSandboxFlag ============ */ class nsCSPSandboxFlags : public nsCSPBaseSrc { public: explicit nsCSPSandboxFlags(const nsAString& aFlags); virtual ~nsCSPSandboxFlags(); + bool visit(nsCSPSrcVisitor* aVisitor) const; void toString(nsAString& outStr) const; private: nsString mFlags; }; /* =============== nsCSPDirective ============= */ class nsCSPDirective {
Attachment #8765577 - Flags: review?(ckerschb)
Attachment #8765577 - Flags: review?(bugs)
Comment on attachment 8764001 [details] [diff] [review] Patch 1 - Implement CSP sandbox directive Since I don't know which patch to review, clearing the request.
Attachment #8764001 - Flags: review?(bugs)
Sorry about that, marking the old patches as obsolete.
Attachment #8764001 - Attachment is obsolete: true
Attachment #8765577 - Attachment is obsolete: true
Attachment #8764001 - Flags: review?(ckerschb)
Attachment #8765577 - Flags: review?(ckerschb)
Attachment #8765612 - Flags: review?(ckerschb)
Attachment #8765612 - Flags: review?(bugs)
Comment on attachment 8763394 [details] [diff] [review] Patch 2 - Export document sandbox flags to chrome JS I wonder why null and not empty string when no flags. But up to you.
Attachment #8763394 - Flags: review?(bugs) → review+
Comment on attachment 8765612 [details] [diff] [review] Patch 1 - Implement CSP sandbox directive >+ * Delegate method called by the service whern the protected document is loaded. whern? >+ * Returns the intersection of all the sandbox flags contained in CSP >+ * policies. This is the most restricting sandbox policy. I don't understand what "This is the most restricting sandbox policy" refers to. Clarify or drop it >+ignoringReportOnlyDirective = Ignoring sandbox directive when delivered in a report-only policy â%1$Sâ >+couldntParseInvalidSandboxFlag = Couldnât parse invalid sandbox flag â%1$Sâ please check that the error message looks right. It probably does, and it is just bugzilla which shows these lines oddly. > // CSP delivered via meta tag should ignore the following directives: > // report-uri, frame-ancestors, and sandbox, see: > // http://www.w3.org/TR/CSP11/#delivery-html-meta-element Not about this bug, but it was such a mistake to support csp using meta tag, as these special cases hint. >+void >+nsCSPSandboxFlags::toString(nsAString& outStr) const >+{ >+ outStr.Append(mFlags); Assign >+nsCSPPolicy::getSandboxFlags() const >+{ >+ nsAutoString flags; >+ for (uint32_t i = 0; i < mDirectives.Length(); i++) { >+ if (mDirectives[i]->equals(nsIContentSecurityPolicy::SANDBOX_DIRECTIVE)) { >+ flags.Truncate(); I would just define nsAutoString flags; here. >+ mDirectives[i]->toString(flags); >+ >+ if (flags.IsEmpty()) { >+ return SANDBOX_ALL_FLAGS; >+ } >+ >+ nsAttrValue attr; >+ attr.ParseAtomArray(flags); So it is defined that DOM attribute parsing and directive parsing are exactly the same? I wish dom/security used Mozilla coding style, but better to not fix that issue in this bug.
Attachment #8765612 - Flags: review?(bugs) → review+
+ * all the source list tokens (the sandbox flags) os the attribute parser "os"? (In reply to Olli Pettay [:smaug] (high review load, please consider other reviewers) from comment #306) > So it is defined that DOM attribute parsing and directive parsing are > exactly the same? ah, we create the string which is then passed to DOM attribute parsing. fine.
Attachment #8763394 - Flags: review?(ckerschb) → review+
Comment on attachment 8765612 [details] [diff] [review] Patch 1 - Implement CSP sandbox directive Review of attachment 8765612 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/security/nsCSPContext.cpp @@ +1314,5 @@ > +nsCSPContext::GetCSPSandboxFlags(uint32_t* aOutSandboxFlags) > +{ > + if (aOutSandboxFlags == nullptr) { > + return NS_ERROR_FAILURE; > + } nit: if (!aOutSandboxFlags) { return NS_ERROR_FAILURE; } ::: dom/security/nsCSPParser.cpp @@ +1006,5 @@ > + > + if (!nsContentUtils::IsValidSandboxFlag(mCurToken)) { > + const char16_t* params[] = { mCurToken.get() }; > + logWarningErrorToConsole(nsIScriptError::warningFlag, "couldntParseInvalidSandboxFlag", > + params, ArrayLength(params)); nit: indendation; 80 chars lines only ::: dom/security/nsCSPParser.h @@ +145,2 @@ > nsAString& outDecStr); > + void sandboxFlagList(nsTArray<nsCSPBaseSrc*>& outSrcs); // helper function to parse sandbox flags Nit: maybe remove 'function' or also 'helper function' so we don't exceed the 80 char line limit.
Attachment #8765612 - Flags: review?(ckerschb) → review+
Olli, can you review webidl changes or do we need to have the .webidl change in a separate patch? I remember my push bounced once because I didn't have the right reviewer set in the commit message that is allowed to review webidl changes.
Flags: needinfo?(bugs)
Whiteboard: [CSP 1.1],[domsecurity-backlog] → [CSP 1.1],[domsecurity-active]
Which webidl changes? I did review already one patch which had webidl changes.
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug] (high review load, please consider other reviewers) from comment #310) > Which webidl changes? I did review already one patch which had webidl > changes. Sorry for not being precise. Within patch 1, there is a change to dom/webidl/CSPDictionaries.webidl. If r=smaug, does inbound then accept the patch to land? Just wanna make sure the push to inbound doesn't bounce.
Flags: needinfo?(bugs)
r=smaug should allow any webidl changes, and part 2 had some change, and also part 1. Both have r=smaug :)
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug] (high review load, please consider other reviewers) from comment #312) > r=smaug should allow any webidl changes, and part 2 had some change, and > also part 1. Both have r=smaug :) awesome :-) Let's land this!!!
Fixes nits from cr by :smaug and :ckerschb diff --git a/dom/interfaces/security/nsIContentSecurityPolicy.idl b/dom/interfaces/security/nsIContentSecurityPolicy.idl --- a/dom/interfaces/security/nsIContentSecurityPolicy.idl +++ b/dom/interfaces/security/nsIContentSecurityPolicy.idl @@ -154,19 +154,19 @@ interface nsIContentSecurityPolicy : nsI * shouldReportViolations is true as well. * @return * Whether or not the effects of the eval call should be allowed * (block the call if false). */ boolean getAllowsEval(out boolean shouldReportViolations); /** - * Delegate method called by the service whern the protected document is loaded. - * Returns the intersection of all the sandbox flags contained in CSP - * policies. This is the most restricting sandbox policy. + * Delegate method called by the service when the protected document is loaded. + * Returns the union of all the sandbox flags contained in CSP policies. This is the most + * restrictive interpretation of flags set in multiple policies. * See nsSandboxFlags.h for the possible flags. * * @return * sandbox flags or SANDBOXED_NONE if no sandbox directive exists */ uint32_t getCSPSandboxFlags(); /** diff --git a/dom/security/nsCSPContext.cpp b/dom/security/nsCSPContext.cpp --- a/dom/security/nsCSPContext.cpp +++ b/dom/security/nsCSPContext.cpp @@ -1308,17 +1308,17 @@ nsCSPContext::ToJSON(nsAString& outCSPin return NS_ERROR_FAILURE; } return NS_OK; } NS_IMETHODIMP nsCSPContext::GetCSPSandboxFlags(uint32_t* aOutSandboxFlags) { - if (aOutSandboxFlags == nullptr) { + if (!aOutSandboxFlags) { return NS_ERROR_FAILURE; } *aOutSandboxFlags = SANDBOXED_NONE; for (uint32_t i = 0; i < mPolicies.Length(); i++) { uint32_t flags = mPolicies[i]->getSandboxFlags(); // current policy doesn't have sandbox flag, check next policy diff --git a/dom/security/nsCSPParser.cpp b/dom/security/nsCSPParser.cpp --- a/dom/security/nsCSPParser.cpp +++ b/dom/security/nsCSPParser.cpp @@ -983,35 +983,36 @@ nsCSPParser::reportURIList(nsTArray<nsCS // Create new nsCSPReportURI and append to the list. nsCSPReportURI* reportURI = new nsCSPReportURI(uri); outSrcs.AppendElement(reportURI); } } /* Helper function for parsing sandbox flags. This function solely concatenates - * all the source list tokens (the sandbox flags) os the attribute parser - * (nsContentUtils::ParseSandboxAttributeToFlags) can user them. + * all the source list tokens (the sandbox flags) so the attribute parser + * (nsContentUtils::ParseSandboxAttributeToFlags) can parse them. */ void nsCSPParser::sandboxFlagList(nsTArray<nsCSPBaseSrc*>& outSrcs) { nsAutoString flags; // remember, srcs start at index 1 for (uint32_t i = 1; i < mCurDir.Length(); i++) { mCurToken = mCurDir[i]; CSPPARSERLOG(("nsCSPParser::sandboxFlagList, mCurToken: %s, mCurValue: %s", NS_ConvertUTF16toUTF8(mCurToken).get(), NS_ConvertUTF16toUTF8(mCurValue).get())); if (!nsContentUtils::IsValidSandboxFlag(mCurToken)) { const char16_t* params[] = { mCurToken.get() }; - logWarningErrorToConsole(nsIScriptError::warningFlag, "couldntParseInvalidSandboxFlag", + logWarningErrorToConsole(nsIScriptError::warningFlag, + "couldntParseInvalidSandboxFlag", params, ArrayLength(params)); continue; } flags.Append(mCurToken); if (i != mCurDir.Length() - 1) { flags.AppendASCII(" "); } diff --git a/dom/security/nsCSPUtils.cpp b/dom/security/nsCSPUtils.cpp --- a/dom/security/nsCSPUtils.cpp +++ b/dom/security/nsCSPUtils.cpp @@ -1378,20 +1378,19 @@ nsCSPPolicy::getDirectiveAsString(CSPDir /* * Helper function that returns the underlying bit representation of sandbox * flags. The function returns SANDBOXED_NONE if there are no sandbox * directives. */ uint32_t nsCSPPolicy::getSandboxFlags() const { - nsAutoString flags; for (uint32_t i = 0; i < mDirectives.Length(); i++) { if (mDirectives[i]->equals(nsIContentSecurityPolicy::SANDBOX_DIRECTIVE)) { - flags.Truncate(); + nsAutoString flags; mDirectives[i]->toString(flags); if (flags.IsEmpty()) { return SANDBOX_ALL_FLAGS; } nsAttrValue attr; attr.ParseAtomArray(flags);
Attachment #8765612 - Attachment is obsolete: true
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #308) > ::: dom/security/nsCSPParser.h > @@ +145,2 @@ > > nsAString& outDecStr); > > + void sandboxFlagList(nsTArray<nsCSPBaseSrc*>& outSrcs); // helper function to parse sandbox flags > > Nit: maybe remove 'function' or also 'helper function' so we don't exceed > the 80 char line limit. I didn't fix this because this is following the style of the rest of the file, the alternative would be substantial reformatting of the file...
(In reply to Olli Pettay [:smaug] (high review load, please consider other reviewers) from comment #306) > Comment on attachment 8765612 [details] [diff] [review] > Patch 1 - Implement CSP sandbox directive > > >+ignoringReportOnlyDirective = Ignoring sandbox directive when delivered in a report-only policy â%1$Sâ > > >+couldntParseInvalidSandboxFlag = Couldnât parse invalid sandbox flag â%1$Sâ > please check that the error message looks right. It probably does, and it is > just bugzilla which shows these lines oddly. > Yeah, this is correct, the original patch was using simple ' (Apostrophe 0x27) (which causes a test to fail and request that ‘ (Left Single Quotation Mark 0x2018) and ’ (Right Single Quotation Mark 0x2019) be used instead. > > >+void > >+nsCSPSandboxFlags::toString(nsAString& outStr) const > >+{ > >+ outStr.Append(mFlags); > Assign > I left this as-is because that follows the semantic of the other classes derived from nsCSPBaseSrc > > So it is defined that DOM attribute parsing and directive parsing are > exactly the same? > Yes, the spec for CSP sandbox directive simply points to the iframe sandbox attribute
rebased.
Attachment #8766035 - Attachment is obsolete: true
rebased
Attachment #8763394 - Attachment is obsolete: true
rebase
Attachment #8763668 - Attachment is obsolete: true
Try looks good. Failures seem all intermittent. Let's check this in!!
Comment on attachment 8766061 [details] [diff] [review] Patch 4 - Extend CSP tests for iframe sandbox with CSP sandbox directive tests Re-setting the r+ from smaug and ckerschb for all of those patches!
Attachment #8766061 - Flags: review+
Alright, let's do this - awesome - thanks a lot to Paul, Dev and Deian!
Keywords: checkin-needed
has problems to apply: Rename patch 'Patch 1 - Implement CSP sandbox directive' (8766056) (r)/overwrite (o)? o renamed 671389 -> 0001-bug-671389-implement-csp-sandbox-directive.patch applying 0001-bug-671389-implement-csp-sandbox-directive.patch patching file dom/security/nsCSPParser.cpp Hunk #1 FAILED at 0 Hunk #3 FAILED at 1043 2 out of 4 hunks FAILED -- saving rejects to file dom/security/nsCSPParser.cpp.rej patch failed, unable to continue (try -v) patch failed, rejects left in working directory
Flags: needinfo?(pacaro)
Keywords: checkin-needed
Rebased
Attachment #8766056 - Attachment is obsolete: true
Flags: needinfo?(pacaro)
Hm.. anyone know how to carry over smaug's r+ over to the rebased patch? I only seem to be able to add my own r+
Comment on attachment 8766360 [details] [diff] [review] Patch 1 - Implement CSP sandbox directive Rebased, carrying over r+ from smaug and ckerschb.
Attachment #8766360 - Flags: review+
Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/a7788974daac Implement CSP sandbox directive. r=ckerschb r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/daa3bad7dcf5 Export document sandbox flags to chrome JS. r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/1ed6f9d0f4a0 Tests for CSP sandbox directive. r=grobinson, r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/bbc6c20f5520 Extend CSP tests for iframe sandbox with CSP sandbox directive tests r=grobinson
Keywords: checkin-needed
The two strings added in this bug end without a period, while the surrounding ones do. Is that wanted? http://hg.mozilla.org/mozilla-central/diff/a7788974daac/dom/locales/en-US/chrome/security/csp.properties
I have no idea what the standard recommendations are. The other strings in the file are somewhat inconsistent, and these are used for logging only. The strings both end in a quoted directive value, an additional period after the final quote seemed unnecessary to me.
https://hg.mozilla.org/mozilla-central/diff/a7788974daac/dom/locales/en-US/chrome/security/csp.properties > +# LOCALIZATION NOTE (ignoringReportOnlyDirective): > +# %1$S is the directive that is ignored in report-only mode. > +ignoringReportOnlyDirective = Ignoring sandbox directive when delivered in a report-only policy ‘%1$S’ What exactly will be used as replacement for %1$S? Sandbox is the directive that is being ignored if I understand correctly.
Yes, the sandbox directive is ignored. The %1$S is replaced by the contents of the CSP policy containing the ignored sandbox directive
You mean by sandbox policy directive, sandbox policy directive and its value, the value of the sandbox policy directive only or something else?
It's possible that the console spew added in this bug caused sites to UA-sniff and send us bogus (because not forward-compatible) sandbox values. See bug 1306384.
Depends on: 1333532
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: