CSP: Throw a warning when a '*-report-only' header doesn't contain a 'report-uri' directive.

RESOLVED FIXED in mozilla30

Status

()

enhancement
P3
normal
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: freddyb, Assigned: yeukhon)

Tracking

(Blocks 1 bug)

unspecified
mozilla30
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(A mirror of webkit bug https://bugs.webkit.org/show_bug.cgi?id=111208 , inspired by Ashar Javed and Mike West)

I am lazy, so I will just copy&paste the Description from Mike West:

"Apparently people set report-only headers without a report URI, as evidenced by https://twitter.com/soaj1664ashar/status/307450782704365569. We should help them understand that they've basically created an expensive no-op machine."
Thanks for filing this, Freddy !
Blocks: CSP, 792161
Priority: -- → P3
Assignee: nobody → yeukhon
Posted patch report_uri_warning_v1.patch (obsolete) — Splinter Review
Feedback on the warning msg?

I say non-Report-Only header since the fix is used in both the prefixed and unprefixed headers. 

Try is green: https://tbpl.mozilla.org/?tree=Try&rev=4a2e39bf5e2f

Thank you.
Attachment #8365642 - Flags: review?(sstamm)
Component: Developer Tools: Console → Security
Product: Firefox → Core
Comment on attachment 8365642 [details] [diff] [review]
report_uri_warning_v1.patch

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

Once you've simplified the changes to CSPUtils.jsm (details below) and rewritten the warning message, please flag me again.

::: content/base/src/CSPUtils.jsm
@@ +286,5 @@
>    aCSPR._originalText = aStr;
>    aCSPR._innerWindowID = innerWindowFromRequest(docRequest);
>    if (typeof reportOnly === 'undefined') reportOnly = false;
>    aCSPR._reportOnlyMode = reportOnly;
> +  var seenReportURI = false;

You can get rid of the need for this variable (seenReportURI) by simply checking to see if the report-uri directive is set at the end of parsing:

if (aCSPR._reportOnlyMode && !aCSPR._directives.hasOwnProperty(dirname)) {...

::: content/base/test/csp/test_report_uri_missing_in_report_only_header.html
@@ +19,5 @@
> +<script class="testbody" type="text/javascript">
> +var stringBundleService = SpecialPowers.Cc["@mozilla.org/intl/stringbundle;1"]
> +                          .getService(SpecialPowers.Ci.nsIStringBundleService);
> +var localizer = stringBundleService.createBundle("chrome://global/locale/security/csp.properties");
> +var warningMsg = localizer.GetStringFromName("reportURINotInReportOnlyHeader", 0)

If you include any variables in the .properties file, you'll have to make sure the same value is reflected into the localizer here.  If you include the URI of the protected document in the message (which I think we should), you'll have to make sure to include it here too.

::: dom/locales/en-US/chrome/security/csp.properties
@@ +27,5 @@
>  ignoringUnknownOption = Ignoring unknown option %1$S
>  # LOCALIZATION NOTE (reportURInotHttpsOrHttp):
>  # %1$S is the ETLD of the report URI that is not HTTP or HTTPS
>  reportURInotHttpsOrHttp = The report URI (%1$) should be an HTTP or HTTPS URI.
> +reportURINotInReportOnlyHeader = Security policy specify in Report-Only header will not be enforced by the browser. To use the Report-Only header as a mean to experiment CSP and to receive violation reports, specify report-uri directive in the Report-Only header. To enforce the security policy, use a non-Report-Only CSP header instead.

This is too wordy I think.  Report-only headers can be useful for debugging to (that is why we're not treating this as a fatal error).  What we really want to tell the developers is, "caution, nothing will be blocked and violations won't be reported to your server."  So what about:

"""
# %1$S is the site with the Report-Only HTTP header
This site (%1$S) has a Report-Only policy without a report URI.  CSP will not block violations of this policy and cannot report violations of this policy.
"""

If we want to be more detailed, we could also add to the end:

"""
To block violations, use the Content-Security-Policy header.  To receive violation reports, use the report-uri directive.
"""

Also, please add a localization note to identify what value %1$S has.
Attachment #8365642 - Flags: review?(sstamm) → review-
Matej: can you help with words here?  We're trying to warn developers when they use a Report-Only CSP header (means tell me when the page violates my policy, but don't protect my page) without specifying a report-uri (where Firefox will send info when the policy gets violated).  I made a suggestion above, but you know words better than I do.
Flags: needinfo?(Mnovak)
Do we have to repeat "violations of this policy" twice? Here's what I would recommend:

"""
# %1$S is the site with the Report-Only HTTP header
This site (%1$S) has a Report-Only policy without a report URI. CSP will not block and cannot report violations of this policy.
"""

The more detailed version looks fine as is.
Flags: needinfo?(Mnovak)
Posted patch report_uri_warning_v2.patch (obsolete) — Splinter Review
1. Should I make a separate test for X-CSP-RO?

2. Saying "CSP will not block" in the context of missing report-uri directive sounds like "to enforce the policy you should add report-uri". I supposed if we mix the simple and verbose version:

This site (%1$S) has a Report-Only policy without a report URI. To receive violation reports in a Report-Only policy, use the report-uri directive. To block violations, use the Content-Security-Policy header.

3. Should I use selfUri.host or selfUri.hostPort? The latter has port appended which seems to be more accurate (what if the site has multiple apps on different ports?) Maybe an edge case... 

Thanks.
Attachment #8365642 - Attachment is obsolete: true
Attachment #8366258 - Flags: review?(sstamm)
Comment on attachment 8366258 [details] [diff] [review]
report_uri_warning_v2.patch

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

r=me assuming all the CSP tests pass locally.

(In reply to Yeuk Hon Wong [:yeukhon] from comment #6)
> Created attachment 8366258 [details] [diff] [review]
> report_uri_warning_v2.patch
> 
> 1. Should I make a separate test for X-CSP-RO?

No.  We're deprecating the X- headers soon, and would be deleting the test.

> 
> 2. Saying "CSP will not block" in the context of missing report-uri
> directive sounds like "to enforce the policy you should add report-uri". I
> supposed if we mix the simple and verbose version:

<snip>

Nah.  Just add the "more detailed" sentence to the one in this patch and call it good.

> 3. Should I use selfUri.host or selfUri.hostPort? The latter has port
> appended which seems to be more accurate (what if the site has multiple apps
> on different ports?) Maybe an edge case... 

Use prePath (will include scheme/host/port).

::: dom/locales/en-US/chrome/security/csp.properties
@@ +29,5 @@
>  # %1$S is the ETLD of the report URI that is not HTTP or HTTPS
>  reportURInotHttpsOrHttp = The report URI (%1$) should be an HTTP or HTTPS URI.
> +# LOCALIZATION NOTE (reportURInotInReportOnlyHeader):
> +# %1$S is the host of the page with the policy
> +reportURInotInReportOnlyHeader = This site (%1$S) has a Report-Only policy without a report URI. CSP will not block and cannot report violations of this policy.

Is the more detailed info we discussed in comment 4 helpful to developers?  If so, feel free to append it here.
Attachment #8366258 - Flags: review?(sstamm) → review+
Posted file report_debug.patch (obsolete) —
Hi Sid.

I find the following error when I ran all CSP tests.

> [Exception... "[JavaScript Error: "selfUri is null" {file: "resource://gre/modules/CSPUtils.jsm" line: 508}]'

The line number corresponds to the warning I added in my patch. To test what happened, I modified my test file's header a bit and added some logging in CSPUtil.jsm.

The test failed because of that error. I see both log messages appear in stdout. This means, even though we hit the first return, the script continued. I thought the return would return control back to caller immediately?
Flags: needinfo?(sstamm)
Which test case in particular is causing the exception?
Flags: needinfo?(sstamm)
My patch will fail any tests that tests XCSP without allow / default-src because selfUri. This means if you ever access a website whose XCSP does not have allow/default-src, Fx will crash.

For example: https://bugzilla.mozilla.org/show_bug.cgi?id=916446

I can add an additional check, but I don't understand why it will continue after a return.
I don't think it actually continues after the return, rather a recursive call to fromString that doesn't hit that early return. 

The XCSP header requires allow/default-src to be present, and the parser replaces the policy with "default-src 'none'" if it's not.  That's a recursive call to fromString, so what's probably happening is that the first call *does* return before your code, but the inner/second call does not.  It actually creates a policy but without selfUri set.  You could pass selfUri, but it's not required by fromString, so you should probably do the check anyway.

CSPRep.fromString assumes that only calls to other functions need selfUri, so if you're accessing one of its members you need the check.  Should be a simple change:

750     cspWarn(aCSPR, CSPLocalizer.getFormatStr("reportURInotInReportOnlyHeader", [selfUri ? selfUri.host : "undefined"]));
Added the addition checkin. Verified XCSP can pass as well.

Try now looks good: https://tbpl.mozilla.org/?tree=Try&rev=9c6048e37168

Thank you Sid.
Attachment #8366258 - Attachment is obsolete: true
Attachment #8373564 - Attachment is obsolete: true
Comment on attachment 8375188 [details] [diff] [review]
report_uri_warning_v3.patch

Carrying over r+ from Sid.
Attachment #8375188 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4bc1a1d739f4
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.