924708 - content-security-policy-report-only is being enforced instead of just reporting problems

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Matthew N. [:MattN]]

I have a page which has the following header:

content-security-policy-report-only: policy-uri /csp

where /csp contains:

default-src 'self'; report-uri /csp; frame-src 'self' https://example.com; img-src 'self' *.example.com *.google-analytics.com https://example.com data:; object-src 'self' video.example.com releases.flowplayer.org; script-src 'self' https://ssl.google-analytics.com http://www.google-analytics.com;

The policy is being enforced and inline scripts are being blocked even though this is a report-only policy.

Last good nightly: 2013-09-12
First bad nightly: 2013-09-13

Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a4e9c9c9dbf9&tochange=b9029b1de410

Testcase coming up.

Comment 1

[Matthew N. [:MattN]]

Use the testcase at http://bugs.mattn.ca/924708/

You should see "Test passed" in working builds and "Test failed" otherwise.

Comment 2

[Sid Stamm [:geekboy or :sstamm]]

While tracking down the cause of this bug I noticed there's an extra (unused) argument in LogPolicyViolations:
http://mxr.mozilla.org/mozilla-central/source/content/base/src/contentSecurityPolicy.js#206

But yeah, my money's on this line (missing reportOnly argument):
http://mxr.mozilla.org/mozilla-central/source/content/base/src/CSPUtils.jsm#726

Can't wait until we can deprecate the old X- syntax and maintain only one parser... soon.

MattN: BTW, policy-uri is not in the W3C CSP spec, and may or may not be supported in the future.

Comment 3

[Matthew N. [:MattN]]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #2)
> But yeah, my money's on this line (missing reportOnly argument):
> http://mxr.mozilla.org/mozilla-central/source/content/base/src/CSPUtils.
> jsm#726

I thought so too but that doesn't seem to be enough.

> MattN: BTW, policy-uri is not in the W3C CSP spec, and may or may not be
> supported in the future.

Yeah, I noticed and was disappointed. I like being able to manage policies for all of my subdomains in one central script.

Comment 4

[Garrett Robinson [:grobinson]]

Attachment: 1.patch

Fix all failures to pass the reportOnly parameter to "fail-closed" invocations of CSPRep.fromString and CSPRep.fromStringSpecCompliant. I refactored both of these functions to take reportOnly as the second argument rather than the list, since the other intervening arguments are not necessary in the "fail-closed" cases.

Comment 5

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Garrett Robinson [:grobinson] from comment #4)
> Created attachment 815146 [details] [diff] [review]
> 1.patch

Does this fix the problem?  Was the policy invalid and thus causing a "fail closed" situation?

Comment 6

[Garrett Robinson [:grobinson]]

This does fix the problem (checked with the test suite from Comment 1). The policy was not invalid, but we were failing to pass reportOnly to the code that handles policy-uri. This patch covers that case, and all the others as well.

Comment 7

[Sid Stamm [:geekboy or :sstamm]]

Ok, cool.  We probably need a mochitest for this to avoid the same regression when we make changes to the policy engine in the future.

Comment 8

[Garrett Robinson [:grobinson]]

Attachment: 1-test.patch

Comment 9

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 815146 [details] [diff] [review]
1.patch

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

Please fix the missing reportOnly argument when calling the CSPPolicyURIListener constructor too (see comment 2).

::: content/base/src/CSPUtils.jsm
@@ +262,1 @@
>    aCSPR._reportOnlyMode = reportOnly;

You can avoid the typeof check if you use:
aCSPR._reportOnlyMode = !!reportOnly;

But maybe that's a little weird.

@@ +521,1 @@
>    aCSPR._reportOnlyMode = reportOnly;

Same as above.  Opportunity to do !!reportOnly.

Comment 10

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 815193 [details] [diff] [review]
1-test.patch

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

Nice test.

::: content/base/test/csp/file_CSP_policyuri_regression_from_multipolicy.html
@@ +1,4 @@
> +<!doctype html>
> +<html>
> +  <body>
> +    <div id=testdiv></div>

Nit: add something here so if the script doesn't execute, the mochitests says something more informative than "Got ,".  Maybe add "Script Didn't Run" inside the div or something.

Comment 11

[Garrett Robinson [:grobinson]]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #2)
> While tracking down the cause of this bug I noticed there's an extra
> (unused) argument in LogPolicyViolations:
> http://mxr.mozilla.org/mozilla-central/source/content/base/src/
> contentSecurityPolicy.js#206

Yeah, I've noticed that before too :P We'll get around to fixing it eventually (perhaps with the rewrite).

Comment 12

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #9)
> Please fix the missing reportOnly argument when calling the
> CSPPolicyURIListener constructor too (see comment 2).

Looks like you already got it.  Sorry.

Comment 13

[Matthew N. [:MattN]]

Is this waiting on something to land? This needs to get uplifted soon.

Comment 14

[Garrett Robinson [:grobinson]]

Attachment: 2.patch

r+ from sstamm. Updated patch with commit message for landing.

Comment 15

[Garrett Robinson [:grobinson]]

Attachment: 2-test.patch

r+ from sstamm. Fixes nit from Comment 10. Adds commit message.

Comment 16

[Ed Morley [:emorley]]

Pushed a bunch of checkin-neededs including this one to try:
https://tbpl.mozilla.org/?tree=Try&rev=503271ae46d1

However there were xpcshell failures, eg:
https://tbpl.mozilla.org/php/getParsedLog.php?id=29555282&tree=Try

Comment 17

[Ed Morley [:emorley]]

Also this is failing:
https://tbpl.mozilla.org/php/getParsedLog.php?id=29556280&tree=Try

Comment 18

[Garrett Robinson [:grobinson]]

(In reply to Ed Morley [:edmorley UTC+1] from comment #17)
> Also this is failing:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=29556280&tree=Try

Ah, we need to disable this test on B2G (because it uses the disabled http-on-* observers). Updating the patch now.

Comment 19

[Garrett Robinson [:grobinson]]

Attachment: 3.patch

r+ from sstamm. XPCShell test failures were due to the reordering of arguments (in the XPCShell tests, self is always passed as the second argument). I decided another re-arrangement of arguments, to CSPRep.fromString(aStr, self, reportOnly, ...), was the neatest way to solve the problem.

Comment 20

[Garrett Robinson [:grobinson]]

Attachment: 3-test.patch

r+ from sstamm. Adds exception so the test doesn't run (and fail due to the lack of an http-on-modify-request obesrver) on B2G.

Comment 21

[Garrett Robinson [:grobinson]]

Try: https://tbpl.mozilla.org/?tree=Try&rev=dd750eaba151

Comment 22

[Garrett Robinson [:grobinson]]

Try (with tests): https://tbpl.mozilla.org/?tree=Try&rev=b18ff29123d6

Comment 23

[Garrett Robinson [:grobinson]]

Try looks good.

Comment 24

[Ed Morley [:emorley]]

(In reply to Garrett Robinson [:grobinson] from comment #22)
> Try (with tests): https://tbpl.mozilla.org/?tree=Try&rev=b18ff29123d6

(In reply to Garrett Robinson [:grobinson] from comment #23)
> Try looks good.

Unfortunately there are still xpcshell failures there.

Comment 26

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Ed Morley [:edmorley UTC+1] from comment #24)
> Unfortunately there are still xpcshell failures there.

Yep, looks like test_csputils.js is failing test_CSPRep_fromString_oneDir :: line 366.  
"allow bar.com; script-src https://foo.com" does not allow "http://bar.com:80" for style, media, image or frame.

Comment 27

[Garrett Robinson [:grobinson]]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #26)
> (In reply to Ed Morley [:edmorley UTC+1] from comment #24)
> > Unfortunately there are still xpcshell failures there.
> 
> Yep, looks like test_csputils.js is failing test_CSPRep_fromString_oneDir ::
> line 366.  
> "allow bar.com; script-src https://foo.com" does not allow
> "http://bar.com:80" for style, media, image or frame.

I thought I fixed this yesterday - I repro'd locally, saw it was due to the order of arguments from the refactor, fixed it, and the test passed locally. Not sure why it's still failing on try (I may have uploaded the wrong patch?) Will investigate momentarily, just finishing up nonce-source now.

Comment 28

[Garrett Robinson [:grobinson]]

Ah - when I re-pushed to try with unit tests turned on, I attached the old patches by mistake. Here's a new try run with the current patches: https://tbpl.mozilla.org/?tree=Try&rev=9510022f7f1e

Comment 29

[Garrett Robinson [:grobinson]]

One xpcshell test was still failing due to the function call refactor, fixed and pushed to try again: https://tbpl.mozilla.org/?tree=Try&rev=e65d5f14928f

Comment 30

[Garrett Robinson [:grobinson]]

Attachment: 4.patch

Fix failing xpcshell tests. Merge "code" patch and "tests" patch into one. r+ from sstamm.

Comment 31

[Garrett Robinson [:grobinson]]

Oops - here's try with unit tests: https://tbpl.mozilla.org/?tree=Try&rev=dbb3a1378053

Comment 32

[Garrett Robinson [:grobinson]]

Try (for real this time) looks good.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/894f66bd6116

Comment 34

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/894f66bd6116

Comment 35

[Garrett Robinson [:grobinson]]

Comment on attachment 823484 [details] [diff] [review]
4.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 836922
User impact if declined: Website developers attempting to use a report-only Content Security Policy may unintentionally block large sets of resources, like all scripts or all styles, breaking the site for end users
Testing completed (on m-c, etc.): Green try on m-c
Risk to taking this patch (and alternatives if risky): Low risk, only affects CSP
String or IDL/UUID changes made by this patch: None

Comment 36

[Garrett Robinson [:grobinson]]

Comment on attachment 823484 [details] [diff] [review]
4.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 836922
User impact if declined: Website developers attempting to use a report-only Content Security Policy may unintentionally block large sets of resources, like all scripts or all styles, breaking the site for end users
Testing completed (on m-c, etc.): Green try on m-c
Risk to taking this patch (and alternatives if risky): Low risk, only affects CSP
String or IDL/UUID changes made by this patch: None

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3c9f15727c31
https://hg.mozilla.org/releases/mozilla-beta/rev/1bd0de861b33

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/1bd0de861b33