Closed Bug 924708 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Security, defect, major)

defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla28
Tracking Status
firefox25 --- unaffected
firefox26 + fixed
firefox27 + fixed
firefox28 --- fixed
b2g-v1.2 --- fixed

People

(Reporter: MattN, Assigned: grobinson)

References

()

Details

(Keywords: regression, testcase, Whiteboard: [good first verify]. see comment #1 for testcase)

Attachments

(1 file, 6 obsolete files)

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.
Use the testcase at http://bugs.mattn.ca/924708/

You should see "Test passed" in working builds and "Test failed" otherwise.
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.
Assignee: nobody → sstamm
Status: NEW → ASSIGNED
(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.
Attached patch 1.patch (obsolete) — Splinter Review
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.
Assignee: sstamm → grobinson
Attachment #815146 - Flags: review?(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?
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.
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.
Attached patch 1-test.patch (obsolete) — Splinter Review
Attachment #815193 - Flags: review?(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.
Attachment #815146 - Flags: review?(sstamm) → review+
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.
Attachment #815193 - Flags: review?(sstamm) → review+
(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).
(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.
Is this waiting on something to land? This needs to get uplifted soon.
Flags: needinfo?(grobinson)
Attached patch 2.patch (obsolete) — Splinter Review
r+ from sstamm. Updated patch with commit message for landing.
Attachment #815146 - Attachment is obsolete: true
Attachment #819867 - Flags: review+
Flags: needinfo?(grobinson)
Attached patch 2-test.patch (obsolete) — Splinter Review
r+ from sstamm. Fixes nit from Comment 10. Adds commit message.
Attachment #815193 - Attachment is obsolete: true
Attachment #819868 - Flags: review+
Keywords: checkin-needed
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
Keywords: checkin-needed
(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.
Attached patch 3.patch (obsolete) — Splinter Review
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.
Attachment #819867 - Attachment is obsolete: true
Attachment #821395 - Flags: review+
Attached patch 3-test.patch (obsolete) — Splinter Review
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.
Attachment #819868 - Attachment is obsolete: true
Attachment #821396 - Flags: review+
Try looks good.
Keywords: checkin-needed
(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.
Keywords: checkin-needed
Duplicate of this bug: 928005
(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.
(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.
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
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
Attached patch 4.patchSplinter Review
Fix failing xpcshell tests. Merge "code" patch and "tests" patch into one. r+ from sstamm.
Attachment #821395 - Attachment is obsolete: true
Attachment #821396 - Attachment is obsolete: true
Attachment #823484 - Flags: review+
Oops - here's try with unit tests: https://tbpl.mozilla.org/?tree=Try&rev=dbb3a1378053
Try (for real this time) looks good.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/894f66bd6116
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
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
Attachment #823484 - Flags: approval-mozilla-aurora?
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
Attachment #823484 - Flags: approval-mozilla-beta?
Attachment #823484 - Flags: approval-mozilla-beta?
Attachment #823484 - Flags: approval-mozilla-beta+
Attachment #823484 - Flags: approval-mozilla-aurora?
Attachment #823484 - Flags: approval-mozilla-aurora+
Duplicate of this bug: 937305
Whiteboard: [good first verify]. see comment #1 for testcase
You need to log in before you can comment on or make changes to this bug.