1505412 - CSP-RO reports violations for dynamically loaded scripts with valid nonce if URL redirects

Status: RESOLVED FIXED

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

Description

[robclap8]

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

Steps to reproduce:

* Opened console
* Ticked preserve log
* Visited https://arturjanc.com/cgi-bin/ff63-nonce-redirect.py


Actual results:

An exception gets logged and reported:
Content Security Policy: The page’s settings blocked the loading of a resource at https://payments.google.com/payments/v4/js/integrator.js (“script-src”).

This is due to the fact that the script is dynamically loaded and redirects to "https://www.gstatic.com/..."

This looks like a special case of bug 1469150 which was marked as fixed. Apparently it is still there for dynamically loaded scripts.

This is not just a security issue for fingerprinting, but also a functional bug as it would break applications that use nonces correctly.


Expected results:

No violation reported.

Comment 1

[:Gijs (he/him)]

Why does this need to be marked security sensitive? Based on the report, we are too aggressive at blocking things, but that's not a security risk, right?

Comment 2

[robclap8]

Mainly for this https://diary.shift-js.info/csp-fingerprinting/ and similar attacks

Comment 3

[:Gijs (he/him)]

(In reply to robclap8 from comment #2)
> Mainly for this https://diary.shift-js.info/csp-fingerprinting/ and similar
> attacks

Thanks for providing the additional detail.

So clearly, as per comment #0, it is wrong that we block the load, and that issue can be addressed in this bug.

However, given this has now bitten us twice, is there any way we can address the reporting issue here in a separate bug? Or would these "leaks" be indistinguishable from "legitimate" CSP issues that we do need to report? :ckerschb?

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

Thanks for the report. I took a quick look what the underlying problem here is. In fact it's unrelated to bug 1469150 because our implementation already blocks the initial load of the dynamically inserted script, not the redirect of it.

Taking a closer look, the page ships a CSP of:
> script-src 'nonce-foobar'; base-uri 'none'; worker-src 'self'; object-src 'none';

and the script part looks like:
> <script nonce="foobar" id="asdf">
>   var scr = document.createElement('script');
>   scr.nonce = "foobar";
>   scr.src = "https://payments.google.com/payments/v4/js/integrator.js";
>   document.body.appendChild(scr);
> </script>

* The inline script with id "asdf" is whitelisted by the nonce of "foobar".
* Then we start loading "https://payments.google.com"
* Our implementation does not take the nonce attribute of the dynamically loaded script into account and tries to perform a whitelist match
* Since there is no entry for https://payments.google.com, Firefox blocks that load.
* Obviously that is not correct and the dynamically loaded script should be allowed to load.

For the sake of completeness, this is not a regression because that never worked correctly in Firefox!

Now my question is the following:
In case the CSP would also contain the keyword strict-dynamic (and the dynamically loaded script would not set a nonce attribute) then we would allow the load because the dynamically loaded script is not parser inserted, right?

Now what is the correct behavior, should we look at the nonce attribute of the dynamically loaded script or should it be allowed because it's not parser inserted? To me the spec is not clear in case the CSP does only contain nonces, see: https://www.w3.org/TR/CSP3/

Personally I think we could allow the load because it's not parser inserted and the 'outer' script is whitelisted. Basically the same reasoning that holds true for the strict-dynamic case would also hold true for the nonce only case!

Artur, what's your take?

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

(In reply to :Gijs (he/him) from comment #3)
> However, given this has now bitten us twice, is there any way we can address
> the reporting issue here in a separate bug?

As explained in my previous comment, this bug is a different problem. Nevertheless I share your concerns.

> Or would these "leaks" be
> indistinguishable from "legitimate" CSP issues that we do need to report?
> :ckerschb?

The problem, as you already indicated, is that those reports/blockages are indistinguishable from legitimate CSP issues :-(

Comment 6

[robclap8]

I believe the correct behavior would be:
* If strict-dynamic is specified, execute the dynamically added script
* If strict-dynamic is not there, AND the nonce has been manually propagated (in the example `src.nonce = "foobar";`) execute the added script
* If the policy is nonce-only and the nonce was not propagated, block the script

Also, if you go here: https://arturjanc.com/cgi-bin/ff63-nonce-redirect2.py you will find that FF63 already allows scripts for strict-dynamic policies.

IMHO just blessing all scripts added by nonced ones would be too broad and would get the "strict-dynamic" behavior even for nonce-only policies, which would degrade security.

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

(In reply to robclap8 from comment #6)
> I believe the correct behavior would be:
> * If strict-dynamic is specified, execute the dynamically added script
> * If strict-dynamic is not there, AND the nonce has been manually propagated
> (in the example `src.nonce = "foobar";`) execute the added script
> * If the policy is nonce-only and the nonce was not propagated, block the
> script

Yes, that sounds plausible to me as well. Though I could also see the benefit of allowing all dynamically added scripts if the outer script is whitelisted.

As far as our implementation goes, that behavior is hard to accomplish with the current setup. In fact we don't have that information when evaluating CSP within content policies. We would have to fix up the callsite and pass the nonce information manually.

All I leave it with that at the moment and will discuss with folks in the triage meeting early next week how to move forward with this bug.

Comment 8

[robclap8]

Hi, 

I investigated this further and I come with good news and bad news:

Good news is: this bug report is not correct, the issue is just in report-only policies, regardless the script being dynamically loaded or not, sorry about the mess.

Minimal repro: serve 
this string
    `<script src="https://payments.google.com/payments/v4/js/integrator.js" data-payments-main nonce="foobar">`
with this policy:
    "content-security-policy-report-only", "script-src 'nonce-foobar' 'unsafe-inline' 'unsafe-eval'; object-src 'none';"

https://play.golang.org/p/HmJqhpitY2j

This triggers a violation and logs that a report will be sent (and sends it if a URL is provided).

If instead the policy is enforced, the violation is not triggered and no report is sent (the script loads correctly)

So this is actually a spurious CSP report and not more.

Bad news is:
This is still a security issue for the above reasons. I will open a separated bug to keep track of that as this one is just not correct.

Comment 9

[robclap8]

About what :ckerschb wrote, it seems like FireFox already behaves correctly if the nonce is set with "script.setAttribute" instead of "script.nonce=", could you confirm this statement? Is the issue with setting the nonce field or with the logic that handles the nonce value?

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

(In reply to robclap8 from comment #8)
> Hi, 
> 
> I investigated this further and I come with good news and bad news:
> 
> Good news is: this bug report is not correct, the issue is just in
> report-only policies, regardless the script being dynamically loaded or not,
> sorry about the mess.

Wait, I could still reproduce the problem described within comment 0 and I see that the script is being blocked. What do you mean by 'the issue is just in report only policies'?

When I ran the STRs from comment 0, I sill see:
>Content Security Policy: The page’s settings blocked the loading of a resource at
> https://payments.google.com/payments/v4/js/integrator.js (“script-src”).

which show that we block that script even though we shouldn't. Right? What changed? It seems there is still a bug even though not related to any kinds of redirect.

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

(In reply to robclap8 from comment #9)
> About what :ckerschb wrote, it seems like FireFox already behaves correctly
> if the nonce is set with "script.setAttribute" instead of "script.nonce=",
> could you confirm this statement? Is the issue with setting the nonce field
> or with the logic that handles the nonce value?

Is that the case? That would surprise me to be honest. I think it shouldn't make a difference and it should also not work correctly if you set the nonce using .setAttribute.

The problem I was referring to is that our content policies only have the outer script element available and don't have access to the dynamically generated script (using our current setup). So it shouldn't make a difference how you set the nonce, our setup can not access any attributes on the newly generated script, because we don't pass the right context.

Comment 12

[arturjanc]

Hello everyone!

This bug has confused us immensely but I think we've finally figured out what's going on.

First, I'd like to apologize for focusing on the .nonce property assignment initially -- this is unrelated to the current bug which manifests itself also when loading scripts from markup.

The current bug seems to be related to the fact that, if:
1. An application sets a Report-Only CSP AND
2. The policy has a script-src 'nonce-...' without 'strict-dynamic' AND
3. The application loads an external sourced script (via a <script src="..."> element with the correct nonce) AND
4. The response to the request for the script is a redirect

then the browser will report a CSP violation. If the script in #4 does not redirect, then no violation will be sent.

The following repro cases with a Content-Security-Policy-Report-Only should illustrate this:
https://www.arturjanc.com/cgi-bin/ff63-nonce-noredirect-reporting.py  -> [OK] doesn't trigger a violation
https://www.arturjanc.com/cgi-bin/ff63-nonce-redirect-reporting.py  -> [bug] triggers a violation

Compare to the same application logic with an enforcing Content-Security-Policy:
https://www.arturjanc.com/cgi-bin/ff63-nonce-noredirect-enforcing.py  -> [OK] doesn't trigger a violation
https://www.arturjanc.com/cgi-bin/ff63-nonce-redirect-enforcing.py  -> [OK] doesn't trigger a violation

So basically it seems like the Report-Only mode sends spurious violations for external scripts which issue a redirect. This doesn't appear to be a problem in enforcing mode.

Comment 13

[arturjanc]

As to the issue of setting script.nonce = "..." vs. script.setAttribute('nonce', "..."), I'm fairly sure Firefox currently supports manually setting the nonce via setAttribute (see https://www.arturjanc.com/cgi-bin/ff-nonce-setattribute.py -- the script with the attribute loads correctly, but the one with the nonce property does not).

So, to disambiguate, I'd say that there are two separate issues we're discussing: 
- The spurious CSP report in Report-Only mode outlined in #12 which also appears to have security consequences,
- The fact that Firefox doesn't allow assignments to the nonce IDL property to bless scripts, and requires setting an attribute on the element (some discussion about a related issue is in https://bugzilla.mozilla.org/show_bug.cgi?id=1389421).

Does this seem like a reasonable summary?

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

(In reply to arturjanc from comment #13)
> So, to disambiguate, I'd say that there are two separate issues we're
> discussing: 
> - The spurious CSP report in Report-Only mode outlined in #12 which also
> appears to have security consequences

Ok, that makes sense now and we should keep this bug to investigate that problem. I'll re-open that bug and change the title.

> - The fact that Firefox doesn't allow assignments to the nonce IDL property
> to bless scripts, and requires setting an attribute on the element (some
> discussion about a related issue is in
> https://bugzilla.mozilla.org/show_bug.cgi?id=1389421).

Right - so let's keep that issue separated and we can deal with it in Bug 1389421.

Comment 15

[Christoph Kerschbaumer [:ckerschb]]

For the sake of completeness, the correct STRs are in comment 12.

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

So, it seems in the redirect case we just hardcode 'true' for sendViolationReports where in fact we should ask the loadinfo if we should report or not, see:
https://searchfox.org/mozilla-central/source/dom/security/nsCSPService.cpp#341

and also
https://searchfox.org/mozilla-central/source/dom/security/nsCSPService.cpp#367

Comment 17

[robclap8]

Hi, is there any update on this? This still happens in FF65.

Comment 18

[Christoph Kerschbaumer [:ckerschb]]

(In reply to robclap8 from comment #17)

Hi, is there any update on this? This still happens in FF65.

Sorry for the lag on this one - I'll try to get to it ASAP.

Comment 19

[Christoph Kerschbaumer [:ckerschb]]

Probably (most likely) that bug got fixed by Bug 965637 or some of its dependencies (prework) - if so, we should land a test within this bug.

Comment 20

[Sebastian Streich [:sstreich]]

Attachment: Bug 1505412 - Skip CSP-Nonce check for CSP-Ro+Redirected Channels r=ckerschb

Comment 21

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/983ff93a11ad7c08fd65a507dedf01526e4ae682

Comment 22

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for debug-test-verify-e10s failure at dom/security/test/csp/test_bug1505412.html:

https://hg.mozilla.org/integration/autoland/rev/170d5cc410a478d14953f22217eda164fc71e87b

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception%2Cusercancel%2Cretry%2Csuperseded&revision=983ff93a11ad7c08fd65a507dedf01526e4ae682
Failure log dom/security/test/csp/test_bug1505412.html: https://treeherder.mozilla.org/logviewer.html#?job_id=261963462&repo=autoland
[task 2019-08-16T08:35:25.628Z] 08:35:25 INFO - TEST-START | dom/security/test/csp/test_bug1505412.html
[...]
[task 2019-08-16T08:35:40.017Z] 08:35:40 INFO - GECKO(1084) | [Parent 1084, StreamTrans #6] WARNING: 'NS_FAILED(rv)', file /builds/worker/workspace/build/src/modules/libjar/nsJARChannel.cpp, line 371
[task 2019-08-16T08:35:40.017Z] 08:35:40 INFO - GECKO(1084) | [Parent 1084, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80520012: file /builds/worker/workspace/build/src/modules/libjar/nsJARChannel.cpp, line 994
[task 2019-08-16T08:40:25.641Z] 08:40:25 INFO - TEST-INFO | started process screentopng
[task 2019-08-16T08:40:25.877Z] 08:40:25 INFO - TEST-INFO | screentopng: exit 0
[task 2019-08-16T08:40:25.878Z] 08:40:25 INFO - TEST-UNEXPECTED-FAIL | dom/security/test/csp/test_bug1505412.html | Test timed out.
[task 2019-08-16T08:40:25.879Z] 08:40:25 INFO - SimpleTest.ok@SimpleTest/SimpleTest.js:275:18
[task 2019-08-16T08:40:25.879Z] 08:40:25 INFO - reportError@SimpleTest/TestRunner.js:121:22
[task 2019-08-16T08:40:25.880Z] 08:40:25 INFO - TestRunner._checkForHangs@SimpleTest/TestRunner.js:142:18
[task 2019-08-16T08:40:25.880Z] 08:40:25 INFO - setTimeout handlerTestRunner._checkForHangs@SimpleTest/TestRunner.js:170:15
[task 2019-08-16T08:40:25.880Z] 08:40:25 INFO - setTimeout handlerTestRunner._checkForHangs@SimpleTest/TestRunner.js:170:15
[task 2019-08-16T08:40:25.881Z] 08:40:25 INFO - setTimeout handlerTestRunner._checkForHangs@SimpleTest/TestRunner.js:170:15
[task 2019-08-16T08:40:25.881Z] 08:40:25 INFO - setTimeout handlerTestRunner._checkForHangs@SimpleTest/TestRunner.js:170:15
[task 2019-08-16T08:40:25.881Z] 08:40:25 INFO - setTimeout handlerTestRunner._checkForHangs@SimpleTest/TestRunner.js:170:15
[task 2019-08-16T08:40:25.881Z] 08:40:25 INFO - setTimeout handlerTestRunner._checkForHangs@SimpleTest/TestRunner.js:170:15
[task 2019-08-16T08:40:25.882Z] 08:40:25 INFO - setTimeout handlerTestRunner._checkForHangs@SimpleTest/TestRunner.js:170:15
[task 2019-08-16T08:40:25.882Z] 08:40:25 INFO - setTimeout handlerTestRunner._checkForHangs@SimpleTest/TestRunner.js:170:15
[task 2019-08-16T08:40:25.882Z] 08:40:25 INFO - setTimeout handlerTestRunner._checkForHangs@SimpleTest/TestRunner.js:170:15
[task 2019-08-16T08:40:25.882Z] 08:40:25 INFO - setTimeout handlerTestRunner._checkForHangs@SimpleTest/TestRunner.js:170:15
[task 2019-08-16T08:40:25.883Z] 08:40:25 INFO - TestRunner.resetTests@SimpleTest/TestRunner.js:406:14
[task 2019-08-16T08:40:25.883Z] 08:40:25 INFO - TestRunner.runNextTest@SimpleTest/TestRunner.js:492:22
[task 2019-08-16T08:40:25.883Z] 08:40:25 INFO - TestRunner.testUnloaded@SimpleTest/TestRunner.js:686:20
[task 2019-08-16T08:40:25.883Z] 08:40:25 INFO - @SimpleTest/iframe-between-tests.html:11:10

Also perma failing a mochitest chunk on Linux x64 debug + Quantumrender debug: https://treeherder.mozilla.org/logviewer.html#?job_id=261974908&repo=autoland

Last folder executed is dom/security/test/sri/

Comment 23

[Sebastian Streich [:sstreich]]

Fixed the test, should work now :)

Comment 24

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/4bedbc45e2316500b07cb6f7320b9424683db818

Comment 25

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out because test_bug1505412.html fails:

https://hg.mozilla.org/integration/autoland/rev/dd1ba2689a3b98d3f6b4bb34342766d7085e42d3

Push which ran failing test: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=4fe91f01854e23e26382d491b4feae4bca13a222
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=262961545&repo=autoland

TEST-UNEXPECTED-FAIL | dom/security/test/csp/test_bug1505412.html | Test timed out.

Comment 26

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/2c2a72e8e4aa6a8ab414b92d75db9f3316c85b2c

Comment 27

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/2c2a72e8e4aa

Comment 28

[Ryan VanderMeulen [:RyanVM]]

If you want to nominate this test for a Beta backport, it probably wouldn't be a bad idea.