888172 - CSP 1.0 does not process 'unsafe-inline' or 'unsafe-eval' for default-src

Status: VERIFIED FIXED

(Core :: Security, defect)

Description

[Garrett Robinson [:grobinson]]

The CSP 1.0 spec allows the keywords 'unsafe-inline' and 'unsafe-eval' to be set on the default-src directive. If they are set, and script-src is not defined, then they are applied to scripts, and if style-src is not defined, 'unsafe-inline' is applied to styles.

Section 5.1, Example 3 of the CSP 1.0 and CSP 1.1 specs gives such a policy:

  Content-Security-Policy: default-src 'self'; script-src https://example.com/js/

Our CSP 1.0 implementation does not implement this behavior; the 'unsafe-inline' and 'unsafe-eval' keywords are only honored if they are used in the script-src or style-src directives.

Comment 1

[Garrett Robinson [:grobinson]]

Pasted the wrong example policy above. This is the actual example given in Section 5.1, Example 3:

  Content-Security-Policy: default-src https: 'unsafe-inline' 'unsafe-eval'

Comment 2

[Krzysiek Wróblewski]

This affects a major Polish banking site, https://www.centrum24.pl/ which uses the policy mentioned above.

Comment 3

[Ian Melven :imelven]

(In reply to Krzysiek Wróblewski from comment #2)
> This affects a major Polish banking site, https://www.centrum24.pl/ which
> uses the policy mentioned above.

Thanks for letting us know, Kryzsiek. We are working on a fix for this bug currently.

Comment 4

[Garrett Robinson [:grobinson]]

Attachment: Patch 1

Patch and mocihtest. Pretty straightforward.

Comment 5

[Ian Melven :imelven]

We will probably want to uplift this too before CSP 1.0 hits release in Fx23.

Comment 6

[Ian Melven :imelven]

Comment on attachment 769285 [details] [diff] [review]
Patch 1

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

When this is finished, we will probably want to uplift this to Fx24 (Aurora) and Fx23 (Beta). 

You probably should also write a test that checks that in a policy of |default-src: 'unsafe-inline' ; script-src: self| inline scripts are not
allowed to execute (specific script-src overrides default-src in that case) and maybe the same for styles. It looks like the code
in CSPUtils.jsm handles this case correctly, though. 

r+ if you add the test and and the pushPrefEnv :)

::: content/base/src/CSPUtils.jsm
@@ +539,5 @@
>    // styles by specifying either default-src or style-src.
>    if ("default-src" in dirs) {
> +    // Parse the source list (look ahead) so we can set the defaults properly,
> +    // honoring the 'unsafe-inline' and 'unsafe-eval' keywords
> +    var defaultSrcValue = CSPSourceList.fromString(dirs["default-src"], null);

I like using the parser here to see what default-src allows

At first I thought these checks might have made more sense where default-src
is usually parsed but after working through this, I think I see why you did this here: because you need to to handle the case of script-src/style-src being present overriding any unsafe inline/eval in default-src.

::: content/base/test/test_CSP_bug888172.html
@@ +41,5 @@
> +document.getElementById('cspframe').addEventListener('load', checkAllowed, false);
> +</script>
> +</pre>
> +</body>
> +</html>

i guess we probably want to stick a pushPrefEnv in this test as well and not block
on getting 1.0 on in B2G first.

Comment 7

[Garrett Robinson [:grobinson]]

(In reply to Ian Melven :imelven from comment #6)
> Comment on attachment 769285 [details] [diff] [review]
> Patch 1
> 
> Review of attachment 769285 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> When this is finished, we will probably want to uplift this to Fx24 (Aurora)
> and Fx23 (Beta). 
> 
> You probably should also write a test that checks that in a policy of
> |default-src: 'unsafe-inline' ; script-src: self| inline scripts are not
> allowed to execute (specific script-src overrides default-src in that case)
> and maybe the same for styles. It looks like the code
> in CSPUtils.jsm handles this case correctly, though.

Good idea. Is there any way for me to do this without copy-pasting my current mochitest and mochitest ^headers^ file, and just changing the ^headers^ file? Feels wasteful.

> 
> r+ if you add the test and and the pushPrefEnv :)
> 
> ::: content/base/src/CSPUtils.jsm
> @@ +539,5 @@
> >    // styles by specifying either default-src or style-src.
> >    if ("default-src" in dirs) {
> > +    // Parse the source list (look ahead) so we can set the defaults properly,
> > +    // honoring the 'unsafe-inline' and 'unsafe-eval' keywords
> > +    var defaultSrcValue = CSPSourceList.fromString(dirs["default-src"], null);
> 
> I like using the parser here to see what default-src allows
> 
> At first I thought these checks might have made more sense where default-src
> is usually parsed but after working through this, I think I see why you did
> this here: because you need to to handle the case of script-src/style-src
> being present overriding any unsafe inline/eval in default-src.
> 

That's right. We can't do it in the main directives processing loop, since there's no guarantee on the order of the directives in the header.

> ::: content/base/test/test_CSP_bug888172.html
> @@ +41,5 @@
> > +document.getElementById('cspframe').addEventListener('load', checkAllowed, false);
> > +</script>
> > +</pre>
> > +</body>
> > +</html>
> 
> i guess we probably want to stick a pushPrefEnv in this test as well and not
> block
> on getting 1.0 on in B2G first.

Yes.

Comment 8

[Ian Melven :imelven]

(In reply to Garrett Robinson [:grobinson] from comment #7)
>
> > You probably should also write a test that checks that in a policy of
> > |default-src: 'unsafe-inline' ; script-src: self| inline scripts are not
> > allowed to execute (specific script-src overrides default-src in that case)
> > and maybe the same for styles. It looks like the code
> > in CSPUtils.jsm handles this case correctly, though.
> 
> Good idea. Is there any way for me to do this without copy-pasting my
> current mochitest and mochitest ^headers^ file, and just changing the
> ^headers^ file? Feels wasteful.

yeah, it's not great to have a bunch of extra files just for different headers. I did it for the spec compliant tests since the ones using the old header will go away some day when we finally deprecate support for it. The other way to do the tests is to write an .sjs - then you can do things like serve different policies/headers based on query params. Some of the other CSP tests do stuff like this, for example, see http://mxr.mozilla.org/mozilla-central/source/content/base/test/file_CSP_frameancestors.sjs

Comment 9

[Ian Melven :imelven]

(In reply to Krzysiek Wróblewski from comment #2)
> This affects a major Polish banking site, https://www.centrum24.pl/ which
> uses the policy mentioned above.

That actually might be bug 887974 which is fixed in Nightly and was uplifted to Aurora yesterday. It's probably worth checking out that site again with the latest Nightly to see if the problem still exists.

Comment 10

[Ian Melven :imelven]

(In reply to Ian Melven :imelven from comment #9)
> (In reply to Krzysiek Wróblewski from comment #2)
> > This affects a major Polish banking site, https://www.centrum24.pl/ which
> > uses the policy mentioned above.
> 
> That actually might be bug 887974 which is fixed in Nightly and was uplifted
> to Aurora yesterday. It's probably worth checking out that site again with
> the latest Nightly to see if the problem still exists.

Actually, that's not true if they do have both unsafe-inline and unsafe-eval as sources for the default-src directive as Krysiek said in comment 2. A policy like that needs this fix AND the fix from 887974.

Comment 11

[Krzysiek Wróblewski]

(In reply to Ian Melven :imelven from comment #10)
> (In reply to Ian Melven :imelven from comment #9)
> > (In reply to Krzysiek Wróblewski from comment #2)
> > > This affects a major Polish banking site, https://www.centrum24.pl/ which
> > > uses the policy mentioned above.
> > 
> > That actually might be bug 887974 which is fixed in Nightly and was uplifted
> > to Aurora yesterday. It's probably worth checking out that site again with
> > the latest Nightly to see if the problem still exists.
> 
> Actually, that's not true if they do have both unsafe-inline and unsafe-eval
> as sources for the default-src directive as Krysiek said in comment 2. A
> policy like that needs this fix AND the fix from 887974.

Yes, that website is still unusable on the latest Nightly.

Comment 12

[Garrett Robinson [:grobinson]]

Attachment: Patch 2

Incorporates imelven's feedback. Adds pushPrefEnv and adds test cases to check that we do the right thing when default-src with unsafe keywords is mixed with script-src aor style-src. I switched over to using a .sjs file to serve the different tests.

(Unfortunately I had to use a bit of a hack to get the .sjs to serve the HTML of the test in the response. I would be really interested to hear if there is another, better way to do it.)

Comment 13

[Ian Melven :imelven]

Comment on attachment 774380 [details] [diff] [review]
Patch 2

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

Overall, looking good ! A suggestion for how to serve files in the sjs and a few very small things.

::: content/base/test/file_CSP_bug888172.sjs
@@ +6,5 @@
> + * this:
> + *
> + * for line in html.split('\n'):
> + *     print '"' + line.replace('"', '\\"') + '"' + ','
> + */

i looked at a bunch of .sjs files in the tree and it looks like the http server is running chrome JS
so you can use components - see http://mxr.mozilla.org/mozilla-central/source/content/media/test/contentDuration5.sjs for an example
that looks like it might do what you want - worth a try.

::: content/base/test/test_CSP_bug888172.html
@@ +23,5 @@
> +// black means the style wasn't applied, applied styles are green
> +var green = 'rgb(0, 128, 0)';
> +var black = 'rgb(0, 0, 0)';
> +
> +function getElementColorById (doc, id) {

usually there's no space between the function name and its opening paren

@@ +49,5 @@
> +  var testframe = document.getElementById('testframe3');
> +
> +  ok(getElementColorById(testframe, 'unsafe-inline-script-allowed') === green, "Inline script should be allowed");
> +  ok(getElementColorById(testframe, 'unsafe-eval-script-allowed')  === green, "Eval should be allowed");
> +  ok(getElementColorById(testframe, 'unsafe-inline-style-allowed') === black, "Inline style should be blocked");

it's a little confusing to have the element id have 'allowed' in it in the blocked
cases...

@@ +68,5 @@
> +    document.getElementById('testframe2').addEventListener('load', checkDefaultSrcWithScriptSrc, false);
> +
> +    document.getElementById('testframe3').src = 'file_CSP_bug888172.sjs?csp=' +
> +        escape("Content-Security-Policy: default-src 'self' 'unsafe-inline' 'unsafe-eval'; style-src 'self'");
> +    document.getElementById('testframe3').addEventListener('load', checkDefaultSrcWithStyleSrc, false);

nit: usually multiline calls like this use 2 space indentation for the 2nd line when 2 space indentation is used
for the rest of the file

i really like how the policy used is embedded right here in the tests, very easy to review !

Comment 14

[Garrett Robinson [:grobinson]]

Attachment: Patch 3

Uses code from Components to load HTML from a file, which is served by the .sjs. Thanks for that tip! Fixes all style nits.

Comment 15

[Garrett Robinson [:grobinson]]

Attachment: Patch 4

Un-bitrotted. Carrying over r+ from :imelven.

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

Comment 16

[Ian Melven :imelven]

Looks like there's a problem with the CSP xpcshell tests :

14:53:28 WARNING - TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/xpcshell/tests/content/base/test/unit/test_csputils.js | test failed (with xpcshell return code: 0), see following log:
| resource://gre/modules/CSPUtils.jsm:1464 | TypeError: self is undefined - See following stack:
15:24:15 ERROR - Return code: 1

Should be pretty easy to repro locally.

Comment 17

[Garrett Robinson [:grobinson]]

Attachment: Patch 5

Fix failing XPCShell test. Carrying over r+ from imelven.

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

Comment 18

[Garrett Robinson [:grobinson]]

Try run looks good to me.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ff3b2131de12

Comment 20

[Lukas Blakk [:lsblakk] use ?needinfo]

Please nominate for uplift since we're tracking this for FF23 and tomorrow is the last beta build to land in for verification.

Comment 21

[Garrett Robinson [:grobinson]]

Comment on attachment 779281 [details] [diff] [review]
Patch 5

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: CSP 1.0 behavior will not be spec compliant
Testing completed (on m-c, etc.): Green try run on m-c
Risk to taking this patch (and alternatives if risky): Minimal, only affects CSP
String or IDL/UUID changes made by this patch: None

Comment 22

[Garrett Robinson [:grobinson]]

Wasn't sure what to put for "Bug caused by". Let me know if there's anything else I should do.

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ff3b2131de12

Comment 24

[Ian Melven :imelven]

Please make sure this goes into Aurora as well.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/249fdcaeac5a

This has non-trivial merge conflicts with beta.

Comment 26

[Garrett Robinson [:grobinson]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #25)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/249fdcaeac5a
> 
> This has non-trivial merge conflicts with beta.

Due to the lack of 885433 in beta (it was uplifted to Aurora). If we uplift it to beta, this patch will merge cleanly.

Comment 27

[Lukas Blakk [:lsblakk] use ?needinfo]

Approved for uplift to beta in https://bugzilla.mozilla.org/show_bug.cgi?id=885433#c36 so we can get this in cleanly and have the correct CSP 1.0 implementation in FF23

Comment 28

[Sid Stamm [:geekboy or :sstamm]]

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

Comment 29

[Matt Wobensmith [:mwobensmith][:matt:]]

Confirmed pre-1.0 behavior for default-src 'unsafe-inline' and 'unsafe-eval' 
Verified 1.0 fix on FF23, 24 and 25.