1198422 - CSP should not block inline script if script-src and default-src are not specified

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Comment 1

[Christoph Kerschbaumer [:ckerschb}]

STR: Go to hackpad.com/ which serves two CSP headers:

> default-src 'self' https://* 'unsafe-inline' 'unsafe-eval' data:;
> img-src https: http: data:; object-src 'none';
> frame-ancestors 'self';
> report-uri https://hackpad.com/csp_log;
> referrer origin-when-crossorigin; script-src 'self' 'unsafe-inline'  https://www.dropbox.com/static/api/1/dropins.js https://ajax.googleapis.com/ajax/libs/jquery/1.7.2/jquery.min.js https://ssl.google-analytics.com/ga.js https://cdn.mxpnl.com/libs/mixpanel-2.1.min.js https://connect.facebook.net/en_US/all.js https://js.stripe.com/v1/ https://static.intercomcdn.com/intercom.v1.js https://widget.intercom.io/widget/ https://js.intercomcdn.com/ https://platform.twitter.com/widgets.js https://syndication.twitter.com/ https://gist.github.com/ https://d29bt26wntaesq.cloudfront.net/ 'nonce-276bd8fea730132a0bddbc55d0fa67a55f2b9d00';

and

> frame-ancestors 'self'

Current-behavior: inline-script which uses correct nonce get blocked
Expected-behavior: inline-script which uses correct nonce should be allowed to run

The problem is that the second policy triggers an incorrect fallback mechanism in our implementation. In case there is no script-src or default-src specified the inline-script should be allowed to run, but we are blocking the script, see:
http://mxr.mozilla.org/mozilla-central/source/dom/security/nsCSPUtils.cpp#1050

Why this has become a problem on hackpad.com only now? I am not sure - I suspect they just started serving the second policy and didn't do so previously.

Please note that ignoring unsafe-inline is not responsible for this problem. This problem has existed in our implementation since we started shipping CSP in C++, see Bug 925004.

Please also note, that even once that bug is fixed we sill erroneously report that inline script get blocked, see Bug 1026520. We should really get that fixed as well.

Comment 2

[Christoph Kerschbaumer [:ckerschb}]

Attachment: bug_1198422_ignore_unsafe_inline_test.patch

Comment 3

[Christoph Kerschbaumer [:ckerschb}]

Attachment: bug_1198422_ignore_unsafe_inline.patch

So, the problem only appears in case there are *multiple* policies involved and 'unsafe-inline' should be ignored.
In earlier versions we already allowed the load to succeed when checking for unsafe-inline. Since Bug 1004703 landed, we are ignoring unsafe-inline in case nonce or hash are specified, so we are entering [2] where within the nonce get whitelisted and would allow the load but then the second policy (in that case frame-ancestors) is not a matching directive and hence we return 'false' even though default-src is not even specified. In such a case we have to allow the load.

Very special corner case, thanks Dev for reporting. We should uplift that though since it's a regresssion.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsScriptLoader.cpp#449
[2] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsScriptLoader.cpp#457

Comment 4

[Christoph Kerschbaumer [:ckerschb}]

Please note that we are still incorrectly displaying errors to the console; we really need to get Bug 1026520 fixed ASAP.

Comment 5

[Devdatta Akhawe [:devd]]

thanks!

Comment 6

[Daniel Veditz [:dveditz]]

Comment on attachment 8652629 [details] [diff] [review]
bug_1198422_ignore_unsafe_inline_test.patch

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

r-ish: if this is a test for the specific bug we should label it with a bug number. If we're testing the principle we should expand the test to cover a few more "stand-alone" items (which can be all served together). Maybe adding them all is overkill, but just frame-ancestors seems too little. Don't want to hold up this fix for such a nit (thus r+), but it should be fairly easy to throw at least a few of those in post review.

::: dom/security/test/csp/test_ignore_unsafe_inline.html
@@ +26,4 @@
>   */
>  
> +const POLICY_PREFIX = "default-src 'none'; script-src ";
> +const SECOND_POLICY = "frame-ancestors 'self'";

Would it be worth testing all the things that shouldn't imply a source-loading policy (that is, don't fall back to default-src)?

base-uri
form-action
frame-ancestors
plugin-types
report-uri
sandbox
upgrade-insecure-requests
referrer
block-all-mixed-content

even if we don't support all those yet we should safely ignore them so it can't hurt to add them all.

Comment 7

[Christoph Kerschbaumer [:ckerschb}]

(In reply to Daniel Veditz [:dveditz] from comment #6)
> Would it be worth testing all the things that shouldn't imply a
> source-loading policy (that is, don't fall back to default-src)?

Thanks Dan. I agree with your comment; I can definitely throw in some more testcases, but I think they all belong into test_ignore_unsafe_inline.html, because this is really the bug that caused the problem here. Anyhow, I will add some more test cases, maybe even have a testcase with 3 policies to make sure everything is fine. Thanks!

Comment 8

[Christoph Kerschbaumer [:ckerschb}]

Attachment: bug_1198422_ignore_unsafe_inline_tests.patch

Dan, I updated the test to include 'random' second policies covering:
* frame-ancestors
* base-uri
* form-action
* upgrade-insecure-requests
* referrer
* sandbox [not supported as of now, see Bug 671389]

Comment 9

[Christoph Kerschbaumer [:ckerschb}]

[Tracking Requested - why for this release]:
The problem was introduced within Bug 1004703 which landed in 40 and ignored 'unsafe-inline' if the hash-src or nonce-src are present in the CSP policy. I propose a wontfix for 40 because in order to trigger the regression a page would have to serve *2* CSP policies where for the first one 'unsafe-inline' should be ignored and the second one does *not* provide a default-src directive.

Comment 10

[Daniel Veditz [:dveditz]]

Comment on attachment 8652632 [details] [diff] [review]
bug_1198422_ignore_unsafe_inline.patch

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

r=dveditz

Comment 11

[Christoph Kerschbaumer [:ckerschb}]

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/e37f08a03a936f7c75341d3acc2357c5d8941553
changeset:  e37f08a03a936f7c75341d3acc2357c5d8941553
user:       Christoph Kerschbaumer <[e-mail]>
date:       Tue Aug 25 16:11:04 2015 -0700
description:
Bug 1198422 - CSP: Allow nonce to load if default-src is not specified in second policy (r=dveditz)

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/4c0ca57c9bc7732623b7f6ded67cf237c387d1d1
changeset:  4c0ca57c9bc7732623b7f6ded67cf237c387d1d1
user:       Christoph Kerschbaumer <[e-mail]>
date:       Thu Aug 27 09:02:32 2015 -0700
description:
Bug 1198422 - CSP: Test fallback for nonce-src and hash-src (r=devitz)

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e37f08a03a93
https://hg.mozilla.org/mozilla-central/rev/4c0ca57c9bc7

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Regression, tracking.
Christoph, are you planning to request the uplift to aurora & beta? Thanks

Comment 14

[Christoph Kerschbaumer [:ckerschb}]

Comment on attachment 8652632 [details] [diff] [review]
bug_1198422_ignore_unsafe_inline.patch

Approval Request Comment
[Feature/regressing bug #]:
Bug 1004703 -  CSP should not block inline script if script-src and default-src are not specified

[User impact if declined]:
CSP will incorrectly block content if the page uses hash-src or nonce-src and additionaly delievers *2* CSP policies. So far I have only encountered one web page ever that ships more than one policy.

[Describe test coverage new/current, TreeHerder]:
We landed and automated test with this patch.

[Risks and why]:
Low, since it only affects pages using *2* CSP policies.

[String/UUID change made/needed]:
no

Comment 15

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8652632 [details] [diff] [review]
bug_1198422_ignore_unsafe_inline.patch

Let's take it to aurora. Ritu will make the call for 41.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7f3e8375ff39
https://hg.mozilla.org/releases/mozilla-aurora/rev/8f5c669a887e

Comment 17

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8652632 [details] [diff] [review]
bug_1198422_ignore_unsafe_inline.patch

Christoph, we are getting close to the Beta41 end game and given your comment that you have only encountered one web page ever that ships more than one policy, I feel that there isn't a really strong reason to push it to Beta41.

Unless, I am missing a critical piece of information here, I would prefer to let this ride Aurora to Beta train in a few weeks. Please let me know if there are any concerns.

Comment 18

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

This does not seem to be a FF41 release blocking scenario. Untracked and marked as wontfix for 41. The good news is this fix will be in Aurora42.