1049289 - CSP does not strip uri fragments before sending reports

Status: VERIFIED FIXED

(Core :: DOM: Security, defect)

Description

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1049289_strip_fragments.patch

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1049289_fragments_tests.patch

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.

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

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.

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1049289_fragments_tests.patch

Exported the patches the wrong way round :-(

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1049289_strip_fragments.patch

Here we go.

Comment 6

[Sid Stamm [:geekboy or :sstamm]]

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.

Comment 7

[Sid Stamm [:geekboy or :sstamm]]

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

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1049289_strip_fragments_v2.patch

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

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1049289_tests.patch

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?

Comment 10

[Sid Stamm [:geekboy or :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 11

[Sid Stamm [:geekboy or :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);
> +    }

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

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 14

[Sid Stamm [:geekboy or :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"

Comment 15

[Sid Stamm [:geekboy or :sstamm]]

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

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

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

[Christoph Kerschbaumer [:ckerschb]]

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.

Comment 18

[Sid Stamm [:geekboy or :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).

Comment 19

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1049289_tests_v2.patch

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

Comment 20

[Christoph Kerschbaumer [:ckerschb]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/97683b215de6
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0a5f990c361

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/97683b215de6
https://hg.mozilla.org/mozilla-central/rev/c0a5f990c361

Comment 22

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 23

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7321b41a0341
https://hg.mozilla.org/releases/mozilla-aurora/rev/237034de34d9

Comment 25

[Matt Wobensmith [:mwobensmith][:matt:]]

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.