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

VERIFIED FIXED in Firefox 23

Status

()

Core
Security
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: grobinson, Assigned: grobinson)

Tracking

(Blocks: 1 bug)

Trunk
mozilla25
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox22 unaffected, firefox23+ verified, firefox24+ verified, firefox25 verified)

Details

(URL)

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

4 years ago
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.
(Assignee)

Updated

4 years ago
(Assignee)

Comment 1

4 years ago
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

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

Comment 3

4 years ago
(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.
(Assignee)

Comment 4

4 years ago
Created attachment 769285 [details] [diff] [review]
Patch 1

Patch and mocihtest. Pretty straightforward.
Attachment #769285 - Flags: review?(sstamm)
Attachment #769285 - Flags: review?(imelven)

Updated

4 years ago
Blocks: 663566
Depends on: 885433

Updated

4 years ago
Assignee: nobody → grobinson
Status: NEW → ASSIGNED

Comment 5

4 years ago
We will probably want to uplift this too before CSP 1.0 hits release in Fx23.
status-firefox22: --- → unaffected
status-firefox23: --- → affected
status-firefox24: --- → affected
tracking-firefox23: --- → ?
tracking-firefox24: --- → ?

Comment 6

4 years ago
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.
Attachment #769285 - Flags: review?(imelven)

Updated

4 years ago
tracking-firefox23: ? → +
tracking-firefox24: ? → +
(Assignee)

Comment 7

4 years ago
(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

4 years ago
(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

4 years ago
(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

4 years ago
(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.
(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.
(Assignee)

Comment 12

4 years ago
Created attachment 774380 [details] [diff] [review]
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.)
Attachment #769285 - Attachment is obsolete: true
Attachment #769285 - Flags: review?(sstamm)
Attachment #774380 - Flags: review?(sstamm)
Attachment #774380 - Flags: review?(imelven)

Comment 13

4 years ago
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 !
Attachment #774380 - Flags: review?(imelven)
(Assignee)

Comment 14

4 years ago
Created attachment 775821 [details] [diff] [review]
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.
Attachment #774380 - Attachment is obsolete: true
Attachment #774380 - Flags: review?(sstamm)
Attachment #775821 - Flags: review?(imelven)

Updated

4 years ago
Attachment #775821 - Flags: review?(imelven) → review+
(Assignee)

Comment 15

4 years ago
Created attachment 776682 [details] [diff] [review]
Patch 4

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

Try: https://tbpl.mozilla.org/?tree=Try&rev=d71bedba66f8
Attachment #775821 - Attachment is obsolete: true
Attachment #776682 - Flags: review+

Comment 16

4 years ago
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.
(Assignee)

Comment 17

4 years ago
Created attachment 779281 [details] [diff] [review]
Patch 5

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

Try: https://tbpl.mozilla.org/?tree=Try&rev=80e4a5a28332
Attachment #776682 - Attachment is obsolete: true
Attachment #779281 - Flags: review+
(Assignee)

Comment 18

4 years ago
Try run looks good to me.
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff3b2131de12
Flags: in-testsuite+
Keywords: checkin-needed
Please nominate for uplift since we're tracking this for FF23 and tomorrow is the last beta build to land in for verification.
Flags: needinfo?(grobinson)
(Assignee)

Comment 21

4 years ago
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
Attachment #779281 - Flags: approval-mozilla-beta?
(Assignee)

Comment 22

4 years ago
Wasn't sure what to put for "Bug caused by". Let me know if there's anything else I should do.
https://hg.mozilla.org/mozilla-central/rev/ff3b2131de12
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Attachment #779281 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 24

4 years ago
Please make sure this goes into Aurora as well.
Attachment #779281 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/249fdcaeac5a

This has non-trivial merge conflicts with beta.
status-firefox24: affected → fixed
status-firefox25: --- → fixed
Keywords: branch-patch-needed
(Assignee)

Comment 26

4 years ago
(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.
Flags: needinfo?(grobinson)
Target Milestone: mozilla25 → ---
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
https://hg.mozilla.org/releases/mozilla-beta/rev/b376b486592b
status-firefox23: affected → fixed
Blocks: 898190
Confirmed pre-1.0 behavior for default-src 'unsafe-inline' and 'unsafe-eval' 
Verified 1.0 fix on FF23, 24 and 25.
Status: RESOLVED → VERIFIED
status-firefox23: fixed → verified
status-firefox24: fixed → verified
status-firefox25: fixed → verified

Updated

4 years ago
Keywords: branch-patch-needed
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.