Closed Bug 1049289 Opened 5 years ago Closed 5 years ago

CSP does not strip uri fragments before sending reports

Categories

(Core :: DOM: Security, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla34
Tracking Status
firefox33 --- verified
firefox34 --- verified
firefox-esr31 --- wontfix

People

(Reporter: ckerschb, Assigned: ckerschb)

References

Details

(Keywords: sec-low, Whiteboard: [adv-main33-])

Attachments

(2 files, 5 obsolete files)

The new CSP implementation does not strip fragments for:
* document-uri
* blocked-uri
before sending CSP reports as the spec requires [1].

It actually surprises me that this slipped through the cracks when doing reviews. Anyway, the old implementation handled that correctly.

Since this is a security critical bug, that needs uplifting once fixed.

[1] http://www.w3.org/TR/CSP/#report-uri
Uploading a testcase to highlight the problem. Currently I am modifying that testcase for bug 1033423. Once that landed we can massage that part of the test into the new testcase after bug 1033423 landed.
Classifying the problem as sec-low; people might get harmed if the fragment is not stripped properly from the uris to be sent out in the csp-report.
Keywords: sec-low
Exported the patches the wrong way round :-(
Attachment #8468188 - Attachment is obsolete: true
Attachment #8468190 - Attachment is obsolete: true
Attachment #8468188 - Flags: review?(sstamm)
Here we go.
Attachment #8468196 - Flags: review?(sstamm)
Assignee: nobody → mozilla
Comment on attachment 8468196 [details] [diff] [review]
bug_1049289_strip_fragments.patch

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

Yep, this looks right.  Fragment should be omitted from blocked-uri and document-uri.  Please finish the tests before landing this.
Attachment #8468196 - Flags: review?(sstamm) → review+
Comment on attachment 8468194 [details] [diff] [review]
bug_1049289_fragments_tests.patch

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

If it's not too hard, probably wouldn't hurt to test non-fragment URIs too (in addition to this "cspframe" with a fragment.

::: content/base/test/csp/test_csp_report.html
@@ +102,5 @@
>  
>  // load the resource which will generate a CSP violation report
>  // save this for last so that our listeners are registered.
>  // ... this loads the testbed of good and bad requests.
> +document.getElementById("cspframe").src = testFile + fragment;

You'll want to test a blocked-uri with fragment too (make the blocked image have a fragment).
Sid, can you review this again? Looking at the spec of CSP 1.1. [1] reveals that we also have to strip fragments for 'source-file' in case it's a uri. In our implementation LogViolationDetailes accepts a String as 'source-file'. I think it's to error prone to update callsites, hence I rather try to convert the string to a URI and strip the fragment if necessary. I think you agree, right?

[1] http://www.w3.org/TR/CSP11/#violation-report-source-file
Attachment #8468194 - Attachment is obsolete: true
Attachment #8468196 - Attachment is obsolete: true
Attachment #8469515 - Flags: review?(sstamm)
Attached patch bug_1049289_tests.patch (obsolete) — Splinter Review
Our testcase is not set up to test the fragment stripping on blocked-uri, because we are trying to load an inline-src. Hence the blocked-uri is 'self'. We can test the correct stripping for document-uri and also source-file though. I don't think it's worth re-arranging the test completely to test fragment stripping on blocked-uri as well. Any objections?
Attachment #8469517 - Flags: review?(sstamm)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #9)
> Any objections?

Can you duplicate the test to cover the other two cases?  It's not good coverage if its only testing the one stripping case.
Comment on attachment 8469515 [details] [diff] [review]
bug_1049289_strip_fragments_v2.patch

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

::: content/base/src/nsCSPContext.cpp
@@ +647,5 @@
> +    if (sourceURI) {
> +      nsAutoCString spec;
> +      sourceURI->GetSpecIgnoringRef(spec);
> +      aSourceFile = NS_ConvertUTF8toUTF16(spec);
> +    }

Instead of making a URI out of the string, can you instead modify this value higher up the call stack where it's still a URI?  I'd rather do the extra conversions: uri->string ... LogViolationDetails(): uri->string.  That is error prone.

The source file is passed as the second argument to LogViolationDetails, and the callsites can probably strip the fragment.  Or we can change it to allow URIs (might be more cumbersome).

http://mxr.mozilla.org/mozilla-central/ident?i=LogViolationDetails
Attachment #8469515 - Flags: review?(sstamm)
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #11)
> Comment on attachment 8469515 [details] [diff] [review]
> bug_1049289_strip_fragments_v2.patch
> 
> Review of attachment 8469515 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/base/src/nsCSPContext.cpp
> @@ +647,5 @@
> > +    if (sourceURI) {
> > +      nsAutoCString spec;
> > +      sourceURI->GetSpecIgnoringRef(spec);
> > +      aSourceFile = NS_ConvertUTF8toUTF16(spec);
> > +    }
> 
> Instead of making a URI out of the string, can you instead modify this value
> higher up the call stack where it's still a URI?  I'd rather do the extra
> conversions: uri->string ... LogViolationDetails(): uri->string.  That is
> error prone.
> 
> The source file is passed as the second argument to LogViolationDetails, and
> the callsites can probably strip the fragment.  Or we can change it to allow
> URIs (might be more cumbersome).
> 
> http://mxr.mozilla.org/mozilla-central/ident?i=LogViolationDetails

I strongly disagree! We can of course change the callsistes, and I also have a patch at hand that does this. But what if we have more callers to LogViolationsDetails at some point and forget to strip the fragement at the callsite? I would rather do the stripping shortly before the reports are sent to make sure they are stripped.
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #10)
> (In reply to Christoph Kerschbaumer [:ckerschb] from comment #9)
> > Any objections?
> 
> Can you duplicate the test to cover the other two cases?  It's not good
> coverage if its only testing the one stripping case.

Tests two out of three at the moment :-). Also, if we make callsites be responsible for passing a stripped uri, we would have to generate a testcase for every single codepath to make sure we test all the different scenarios.

What is so error prone about trying to generate a URI out of a string, and if it is a valid URI, strip the fragment? Seems way safer to me than relying on callsites to pass a stripped URI. Not?
Flags: needinfo?(sstamm)
Comment on attachment 8469517 [details] [diff] [review]
bug_1049289_tests.patch

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

::: content/base/test/csp/test_csp_report.html
@@ +36,5 @@
>  
>  window.checkResults = function(reportObj) {
>    var cspReport = reportObj["csp-report"];
>  
> +  // The following uri's should be stripped before reporting:

Nit: "uri's" -> "uris' fragments"
Attachment #8469517 - Flags: review?(sstamm) → review+
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #13)
> Tests two out of three at the moment :-). Also, if we make callsites be
> responsible for passing a stripped uri, we would have to generate a testcase
> for every single codepath to make sure we test all the different scenarios.

Sounds like a good idea!  Maybe for a follow-up bug, but better test coverage is always good.

> What is so error prone about trying to generate a URI out of a string, and
> if it is a valid URI, strip the fragment? Seems way safer to me than relying
> on callsites to pass a stripped URI. Not?

We've run into issues of converting URIs to strings to URIs (see bug 570505) in the past and it came about because of code iteration; as CSP evolved the back and forth conversions introduced more bugs.

Anyway, you make a good case for not requiring the call-sites to do this.  I agree. Is there a way to just strip anything after a # in the string (without creating a URI) or is that too error prone?
Flags: needinfo?(sstamm)
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #15)
> We've run into issues of converting URIs to strings to URIs (see bug 570505)
> in the past and it came about because of code iteration;

I see, now I know why you don't necessarily like that approach.

> Anyway, you make a good case for not requiring the call-sites to do this.  I
> agree. Is there a way to just strip anything after a # in the string
> (without creating a URI) or is that too error prone?

A 'aSourceFile' holds the name of a file, which technically could contain '#'s. So removing everything after a '#' could strip half of the filename. I agree, it's probably a rare case, but possible after all. I do get your point, but I think the best solution is that we try to convert it into a URI and strip the fragment part :-(
Comment on attachment 8469515 [details] [diff] [review]
bug_1049289_strip_fragments_v2.patch

As discussed in person earlier, I am going to flag you again for review. In the end the solution in this patch is the least error prone way of stripping the fragment part of a uri.
Attachment #8469515 - Flags: review?(sstamm)
Comment on attachment 8469515 [details] [diff] [review]
bug_1049289_strip_fragments_v2.patch

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

::: content/base/src/nsCSPContext.cpp
@@ +647,5 @@
> +    if (sourceURI) {
> +      nsAutoCString spec;
> +      sourceURI->GetSpecIgnoringRef(spec);
> +      aSourceFile = NS_ConvertUTF8toUTF16(spec);
> +    }

Ok, you've convinced me; the other way is potentially more problematic to maintain.  For now, lets do this and we can revisit fixing logViolationDetails later (we've already discussed encapsulating violation reporting, but it's hard and for another day).
Attachment #8469515 - Flags: review?(sstamm) → review+
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #14)
> > +  // The following uri's should be stripped before reporting:
> 
> Nit: "uri's" -> "uris' fragments"

Fixed!
Attachment #8469517 - Attachment is obsolete: true
Attachment #8476455 - Flags: review+
Status: NEW → ASSIGNED
Comment on attachment 8469515 [details] [diff] [review]
bug_1049289_strip_fragments_v2.patch

Approval Request Comment
[Feature/regressing bug #]: no regression
[User impact if declined]: Users have the fragment part of url leaked in the CSP report.
[Describe test coverage new/current, TBPL]: added testcase which already landed on inbound.
[Risks and why]: low risk, because it only affects csp reports.
[String/UUID change made/needed]: none
Attachment #8469515 - Flags: approval-mozilla-aurora?
Comment on attachment 8476455 [details] [diff] [review]
bug_1049289_tests_v2.patch

Approval Request Comment
[Feature/regressing bug #]: no regression
[User impact if declined]: Users have the fragment part of url leaked in the CSP report.
[Describe test coverage new/current, TBPL]: added testcase which already landed on inbound.
[Risks and why]: low risk, because it only affects csp reports.
[String/UUID change made/needed]: none
Attachment #8476455 - Flags: approval-mozilla-aurora?
Attachment #8469515 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8476455 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Reproduced issue on Fx33, 2014-08-04. I can see the URI fragments in the blocked-uri and document-uri values within the violation report. 
Verified fixed on Fx33, 2014-10-03.
Verified fixed on Fx34, 2014-10-06.
Status: RESOLVED → VERIFIED
Whiteboard: [adv-main33+]
Whiteboard: [adv-main33+] → [adv-main33-]
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.