Bugzilla

Reporter

Comment 2

•

6 years ago

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

Flags: needinfo?(robclap8)

:Gijs (he/him)

Comment 3

•

6 years ago

(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?

Flags: needinfo?(ckerschb)

Daniel Veditz [:dveditz]

Updated

•

6 years ago

Keywords: csectype-disclosure, sec-moderate

Comment 4

•

6 years ago

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?

Flags: needinfo?(arturjanc)

Comment 5

•

6 years ago

(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 :-(

Flags: needinfo?(ckerschb)

Updated

•

6 years ago

Blocks: csp-w3c-2

Reporter

Comment 6

•

6 years ago

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

•

6 years ago

(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.

Reporter

Comment 8

•

6 years ago

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.

Status: UNCONFIRMED → RESOLVED

Closed: 6 years ago

Resolution: --- → INVALID

Reporter

Comment 9

•

6 years ago

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?

Flags: needinfo?(ckerschb)

Comment 10

•

6 years ago

(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

•

6 years ago

(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.

Flags: needinfo?(ckerschb)

arturjanc

Comment 12

•

6 years ago

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.

Flags: needinfo?(arturjanc)

arturjanc

Comment 13

•

6 years ago

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

•

6 years ago

(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.

Status: RESOLVED → REOPENED

Ever confirmed: true

Resolution: INVALID → ---

Summary: CSP: Scripts with valid nonce get blocked if URL redirects AND script is dynamically loaded → CSP-RO reports violations for dynamically loaded scripts with valid nonce if URL redirects

Comment 15

•

6 years ago

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

Daniel Veditz [:dveditz]

Updated

•

6 years ago

User Story: (updated)

Comment 16

•

6 years ago

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

Updated

•

6 years ago

Assignee: nobody → ckerschb

Priority: -- → P2

Whiteboard: [domsecurity-active]

Reporter

Comment 17

•

5 years ago

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

Comment 18

•

5 years ago

(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

•

5 years ago

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.

Assignee: ckerschb → streich.mobile

Assignee

Comment 20

•

5 years ago

Attached file Bug 1505412 - Skip CSP-Nonce check for CSP-Ro+Redirected Channels r=ckerschb — Details

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

Assignee

Updated

•

5 years ago

Keywords: checkin-needed

Comment 21

•

5 years ago

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

Keywords: checkin-needed

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

Comment 22

•

5 years ago

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
ug1505412.html: https://treeherder.mozilla.org/logviewer.html#?job_id=261963462&repo=autoland
INFO - TEST-START | dom/security/test/csp/test_bug1505412.html
INFO - GECKO(1084) | [Parent 1084, StreamTrans #6] WARNING: 'NS_FAILED(rv)', file /builds/worker/workspace/build/src/modules/libjar/nsJARChannel.cpp, line 371
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
INFO - TEST-INFO | started process screentopng
INFO - TEST-INFO | screentopng: exit 0
INFO - TEST-UNEXPECTED-FAIL | dom/security/test/csp/test_bug1505412.html | Test timed out.
INFO - SimpleTest.ok@SimpleTest/SimpleTest.js:275:18
INFO - reportError@SimpleTest/TestRunner.js:121:22
INFO - TestRunner._checkForHangs@SimpleTest/TestRunner.js:142:18
INFO - setTimeout handlerTestRunner._checkForHangs@SimpleTest/TestRunner.js:170:15
INFO - setTimeout handlerTestRunner._checkForHangs@SimpleTest/TestRunner.js:170:15
INFO - setTimeout handlerTestRunner._checkForHangs@SimpleTest/TestRunner.js:170:15
INFO - setTimeout handlerTestRunner._checkForHangs@SimpleTest/TestRunner.js:170:15
INFO - setTimeout handlerTestRunner._checkForHangs@SimpleTest/TestRunner.js:170:15
INFO - setTimeout handlerTestRunner._checkForHangs@SimpleTest/TestRunner.js:170:15
INFO - setTimeout handlerTestRunner._checkForHangs@SimpleTest/TestRunner.js:170:15
INFO - setTimeout handlerTestRunner._checkForHangs@SimpleTest/TestRunner.js:170:15
INFO - setTimeout handlerTestRunner._checkForHangs@SimpleTest/TestRunner.js:170:15
INFO - setTimeout handlerTestRunner._checkForHangs@SimpleTest/TestRunner.js:170:15
INFO - TestRunner.resetTests@SimpleTest/TestRunner.js:406:14
INFO - TestRunner.runNextTest@SimpleTest/TestRunner.js:492:22
INFO - TestRunner.testUnloaded@SimpleTest/TestRunner.js:686:20
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/

Flags: needinfo?(streich.mobile)

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

Assignee

Comment 23

•

5 years ago

Fixed the test, should work now :)

Flags: needinfo?(streich.mobile)

Keywords: checkin-needed

Comment 24

•

5 years ago

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

Keywords: checkin-needed

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

Comment 25

•

5 years ago

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.

Flags: needinfo?(streich.mobile)