Closed Bug 888172 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Security, defect)

x86_64
Linux
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla25
Tracking Status
firefox22 --- unaffected
firefox23 + verified
firefox24 + verified
firefox25 --- verified

People

(Reporter: grobinson, Assigned: grobinson)

References

(Blocks 1 open bug, )

Details

Attachments

(1 file, 4 obsolete files)

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.
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'
This affects a major Polish banking site, https://www.centrum24.pl/ which uses the policy mentioned above.
(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.
Attached patch Patch 1 (obsolete) — Splinter Review
Patch and mocihtest. Pretty straightforward.
Attachment #769285 - Flags: review?(sstamm)
Attachment #769285 - Flags: review?(imelven)
Blocks: csp-w3c-1.0
Depends on: 885433
Assignee: nobody → grobinson
Status: NEW → ASSIGNED
We will probably want to uplift this too before CSP 1.0 hits release in Fx23.
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)
(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.
(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
(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.
(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.
Attached patch Patch 2 (obsolete) — Splinter Review
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 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)
Attached patch Patch 3 (obsolete) — Splinter Review
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)
Attachment #775821 - Flags: review?(imelven) → review+
Attached patch Patch 4 (obsolete) — Splinter Review
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+
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.
Attached patch Patch 5Splinter Review
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+
Try run looks good to me.
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)
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?
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
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Attachment #779281 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Please make sure this goes into Aurora as well.
Attachment #779281 - Flags: approval-mozilla-aurora+
(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
Confirmed pre-1.0 behavior for default-src 'unsafe-inline' and 'unsafe-eval' 
Verified 1.0 fix on FF23, 24 and 25.
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.