1377426 - (CVE-2017-7803) Other CSP rules ignored when specifying sandbox 'allow-scripts'

Status: RESOLVED FIXED

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

Description

[Rhys]

Attachment: firefox-csp-bug.zip

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/58.0.3029.110 Safari/537.36

Steps to reproduce:

I'm using a CSP rule (provided as a header, not meta tag) on a page or iframe, where the page contains a script, as follows:

Content-Security-Policy: sandbox allow-scripts; script-src 'none';

I tested this in Firefox 54 and the 55 beta on Linux 64 bit.




Actual results:

The script executes.


Expected results:

The script should not execute. If you remove the sandbox flag entirely from the CSP rule the script is blocked by the script-src 'none' rule. 

If you swap the identical sandbox rule to the iframe (if using an iframe) as an attribute on the iframe HTML (rather than via CSP) then the script is correctly blocked. My expectation would be for the behaviour here to be the same regardless of how you specify the sandboxing.

The example may seem contradictory (to specify allow-scripts to only then block all scripts) but I actually noticed this when attempting to both sandbox and limit scripts to a particular domain (rather than block off all scripts) but noticed that any script was being allowed through whenever the sandbox rule was present via CSP.

The scope of this issue may actually be wider than only allowing through scripts. 

I've attached a zip that should hopefully illustrate the basic issue (it includes a node/npm http server wrapper for convenience).

Comment 1

[:Gijs (he/him)]

Christoph/Freddy, can you take a look?

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1377426_csp_sandbox_allow_scripts.patch

Thanks for reporting, that is indeed a problem. Too bad that we have tests (Test 5 and 11 within dom/security/test/csp/test_sandbox.html) but actually not testing that exact scenario.

The problem is two fold:
a) we are resetting the principal to a fresh NullPrincipal (if allow-same-origin is not present) but not setting the CSP on that new NullPrincipal.
b) we just check the sandbox flags from the CSP.
I think we should check the merged sandbox flags of the iframe attribute as well as the CSP.

Dan, what do you think, agreed? Once you are fine with the change I'll flag smaug for the dom/ changes to make sure all three of us agree.

Comment 3

[Daniel Veditz [:dveditz]]

I half agree. Definitely need to set the CSP on the new principal! I'm ambivalent about the second check. If there were sandbox flags from elsewhere (which could only be an <iframe> sandbox attribute, right?) wouldn't they already be applied, and the null principal created if necessary? It probably doesn't hurt much to do it your way, but you're guaranteed to always create _two_ null principals for every iframe SANDBOXED_ORIGIN, even when the majority have no CSP. Or at least no CSP sandbox directives; I suppose you wouldn't reach this point if there were no CSP at all.

If you're in an iframe SANDBOXED_ORIGIN and already have a null principal then you don't need to create a second one if the CSP is also SANDBOXED_ORIGIN. You'd have to check for that before you merge the sandbox policies so you could leave it always creating the second null principal in that case if you want. Something like

  bool needNewNullPrincipal = (cspSandboxFlags & SANDBOXED_ORIGIN) && !(mSandboxFlags & SANDBOXED_ORIGIN);
  mSandboxFlags |= cspSandboxFlags;
  if (needNewNullPrincipal) { ... }

My biggest worry about this code predates your patch: the Null principal has no Origin Attributes! A document with a CSP sandbox directive (or in your version of the patch all iframe sandbox docs too!) will escape their container. I think you need to use NullPrincipal::CreateWithInheritedAttributes(principal) instead.

That makes me worry about all the other places in the code that call NullPrincipal::Create() with no attribute arguments. Some are probably internal/chrome things, but I bet at least some are wrong.

Comment 4

[Daniel Veditz [:dveditz]]

created bug 1378552 to audit other uses of NullPrincipal::Create().

Comment 5

[Rhys]

As a slight side-note, we've said a couple of times in these comments about 'merging' the sandbox policies from CSP and the iframe. This might just be me being slightly pedantic with the terminology, but do we agree it should actually be an intersection of the flags rather than a merge?

e.g. if the iframe sets allow-scripts but CSP does not, I'd expect no scripts to be able to run

iframe ['allow-scripts', 'allow-same-origin', 'allow-popups']
CSP ['allow-same-origin']
resultant applied policy ['allow-same-origin']

Comment 6

[Daniel Veditz [:dveditz]]

Calling this "sec-moderate" as a CSP bypass for now, but it's not one an attacker can force on a site so "bypass" isn't exactly the right word for it.

Comment 7

[Rhys]

There is a category of websites where this would probably pose a more serious risk to a user.

If you imagine any scenario where you run a service that facilitates HTML transfer in some way; like a HTML page designer where the attacker can save some HTML against your service (or any variation on that, jsfiddle etc) and create a URL that they can share to someone else. Maybe your service attempts to turn off javascript by sandboxing the attacker-supplied HTML for safety or limit the URLs of scripts and resources to known good URLs/scripts.

By taking advantage of this issue they'd be able to run potentially malicious js in another user's browser.

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1377426_csp_sandbox_allow_scripts.patch

(In reply to Daniel Veditz [:dveditz] from comment #3)
> If you're in an iframe SANDBOXED_ORIGIN and already have a null principal
> then you don't need to create a second one if the CSP is also
> SANDBOXED_ORIGIN.

In fact that makes sense to me. Just for the sake of completeness, we wouldn't reach that code if a site has no CSP, so we wouldn't create two null principals for every sandboxed iframe, but I agree with your solution, that makes more sense to only create a new NullPrincipal if necessarily required by CSP.

> My biggest worry about this code predates your patch: the Null principal has
> no Origin Attributes! A document with a CSP sandbox directive (or in your
> version of the patch all iframe sandbox docs too!) will escape their
> container.

I haven't paid attention to that when writing the patch, but you are absolutely right, we need to fix that too within this patch and propagate OA correctly for that case.

(In reply to Rhys from comment #5)
> As a slight side-note, we've said a couple of times in these comments about
> 'merging' the sandbox policies from CSP and the iframe. This might just be
> me being slightly pedantic with the terminology, but do we agree it should
> actually be an intersection of the flags rather than a merge?
> 
> e.g. if the iframe sets allow-scripts but CSP does not, I'd expect no
> scripts to be able to run
>
> iframe ['allow-scripts', 'allow-same-origin', 'allow-popups']
> CSP ['allow-same-origin']
> resultant applied policy ['allow-same-origin']

At the moment we use an OR operation to merge the flags >> mSandboxFlags |= cspSandboxFlags; << which means we really 'merge' the flags. I think this is what should be  happening, but that also means that the resultant policy would be ['allow-scripts', 'allow-same-origin', 'allow-popups'].

Comment 9

[Rhys]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #8)

> At the moment we use an OR operation to merge the flags >> mSandboxFlags |=
> cspSandboxFlags; << which means we really 'merge' the flags. I think this is
> what should be  happening, but that also means that the resultant policy
> would be ['allow-scripts', 'allow-same-origin', 'allow-popups'].

If that's the case, doing some local tests that seems to be a different behaviour to Edge and Chrome which appear to require the two policies to agree to apply a loosening flag (I'm trying to track down more info on what's supposed to happen here but not turning up anything useful thus far).

Comment 10

[Daniel Veditz [:dveditz]]

> > but that also means that the resultant policy
> > would be ['allow-scripts', 'allow-same-origin', 'allow-popups'].
> 
> If that's the case, doing some local tests that seems to be a different
> behaviour to Edge and Chrome which appear to require the two policies to
> agree to apply a loosening flag (I'm trying to track down more info on
> what's supposed to happen here but not turning up anything useful thus far).

All the flags are on by default and the "allow-" directives turn them *off*. So allow-scripts (0) ORd with default SANDBOXED_ORIGIN (0x10) is SANDBOXED_ORIGIN, etc. The resulting merged policy will be what we all expect: unless all policies allow something it stays default blocked. See https://searchfox.org/mozilla-central/rev/5dadcbe55b4ddd1e448c06c77390ff6483aa009b/dom/base/nsContentUtils.cpp#1587

Comment 11

[Daniel Veditz [:dveditz]]

Comment on attachment 8884300 [details] [diff] [review]
bug_1377426_csp_sandbox_allow_scripts.patch

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

r=dveditz

The logic is correct and I'm happy with it. I left some comments that are strictly my opinion about readability -- ignore or not as you wish.

::: dom/base/nsDocument.cpp
@@ +2798,1 @@
>    mSandboxFlags |= cspSandboxFlags;

mSandboxFlags isn't used in the below. Could we move it below the if-block so it doesn't confuse what's going on? Otherwise people will try to figure out why it's relevant here. Could even give it its own comment like "merge the CSP sandbox directives into any pre-existing sandbox flags"

@@ +2800,2 @@
>      // If the new CSP sandbox flags do not have the allow-same-origin flag
>      // reset the document principal to a null principal

Readability nit: do we need both comments now? they're mostly saying the same thing. could re-use this inner comment with some tweaks for the outer comment:

  // If the new CSP sandbox flags do not have the allow-same-origin flag
  // reset the document principal to a null principal, unless the document
  // is a sandboxed iframe and already has a new null principal.

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1377426_csp_sandbox_allow_scripts.patch

Thanks Dan. Incorporated your nits. Carrying over r+.

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/43260cd0305b

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Please request Beta/ESR52 approval on this when you're comfortable doing so.

Comment 15

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8885125 [details] [diff] [review]
bug_1377426_csp_sandbox_allow_scripts.patch

Approval Request Comment
The problem itself allows to drop CSP enforcement if certain iframe sandbox flags are set as iframe attribute as well as CSP flag. The reason we should consider this for ESR though is that we don't set OA on the new principal which allows to break out of the container - we should fix that.

[Feature/Bug causing the regression]:
I think this problem has been in our codebase since we introduced CSP.

[User impact if declined]:
CSP enforcement might be dropped in certain conditions. E.g. when special sandbox flags are set as an iframe attribute as well as within the shipped CSP.

[Is this code covered by automated tests?]:
Yes

[Has the fix been verified in Nightly?]:
Not yet.

[Needs manual test from QE? If yes, steps to reproduce]:
No (see comment 0 if needed)
[List of other uplifts needed for the feature/fix]:
none

[Is the change risky?]:
No, small change that only affects CSP.

[Why is the change risky/not risky?]:
see above

[String changes made/needed]:
No


[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: 
see above

Fix Landed on Version:
55

Risk to taking this patch (and alternatives if risky):
String or UUID changes made by this patch: 
No

Comment 16

[Julien Cristau [:jcristau]]

Comment on attachment 8885125 [details] [diff] [review]
bug_1377426_csp_sandbox_allow_scripts.patch

csp bypass fix, beta55+

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/dc00e73c06e1

Comment 18

[Andrei Vaida [:avaida]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #15)
> [Is this code covered by automated tests?]:
> Yes
> 
> [Has the fix been verified in Nightly?]:
> Not yet.
> 
> [Needs manual test from QE? If yes, steps to reproduce]:
> No (see comment 0 if needed)

Setting qe-verify- based on Christoph's assessment on manual testing needs and the fact that this fix has automated coverage.

Comment 19

[Gerry Chang [:gchang]]

Comment on attachment 8885125 [details] [diff] [review]
bug_1377426_csp_sandbox_allow_scripts.patch

Fix a security issue. Let's uplift it to ESR52.3.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

I attempted to land this on ESR52 but had to backout because it doesn't have SetCsp there :(. You can use the commit below as a starting point as it already has some rebasing done.
https://hg.mozilla.org/releases/mozilla-esr52/rev/79adc16a40b7594052325812b1a8339f6f61d09b

Comment 21

[Christoph Kerschbaumer [:ckerschb]]

Attachment: esr53_set_csp_uplift.patch

(In reply to Ryan VanderMeulen [:RyanVM] from comment #20)
> I attempted to land this on ESR52 but had to backout because it doesn't have
> SetCsp there :(. You can use the commit below as a starting point as it
> already has some rebasing done.
> https://hg.mozilla.org/releases/mozilla-esr52/rev/
> 79adc16a40b7594052325812b1a8339f6f61d09b

Ryan, using your rebased patch ^^ as well as also uplifting the SetCSP parts from Bug 1073952 (within this patch) it seems everything works. I ran a bunch of tests locally including test_sandbox.html -> seems to work. Dunno what kind of commit message we would need for this patch though.

Comment 22

[Daniel Veditz [:dveditz]]

Comment on attachment 8888327 [details] [diff] [review]
esr53_set_csp_uplift.patch

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

bug 1073952 is nearly the same as this bug (maybe a little less impact). We should really just take all of it. Landing 7 of 8 chunks and then not actually fixing the old problem just looks silly.

Comment 23

[Christoph Kerschbaumer [:ckerschb]]

Attachment: esr52_rebased_bug_1073952.patch

Freddy, instead of just uplifting the SetCSP bits from Bug 1073952, we decided to take that whole patch (modulo tests). Since the patch did not apply completely cleanly I had to manually rebase the patch. Can you take a look and make sure I haven't missed anything? Thanks.

Comment 24

[Frederik Braun [:freddy]]

Comment on attachment 8888467 [details] [diff] [review]
esr52_rebased_bug_1073952.patch

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

Yep, r+.
Got confused for a second but this is a rebase of bug 1073952 as well as bug 1349517 (a follow up fix that prevented some crashy corner cases).

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/c52d8ce8d5df