1069762 - (CVE-2014-1591) CSP violation report contains sensitive data of other origin after redirect

Status: VERIFIED FIXED

(Core :: DOM: Security, defect)

Description

[Muneaki Nishimura]

User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/37.0.2062.120 Safari/537.36

Steps to reproduce:

1) Login to Facebook
2) Launch http://mallory.csrf.jp/cspleakage/
3) The page has one object tag in a page and the object tag accesses https://facebook.com/profile.php
4) Facebook redirects user agent to his/her profile page i.e., https://www.facebook.com/muneaki.nishimura in my case.


Actual results:

When a user launches the page http://mallory.csrf.jp/cspleakage/ , CSP violation report is posted to the report-uri because the object tag violates it's security policy.
Then, the "blocked-uri" parameter contains path and query string values of facebook.com, i.e., my name "muneaki.nishimura" is also recorded.

Suppose http://mallory.csrf.jp/cspleakage/ is a phishing site, then, user name who visited to the page can be identified by blocked-uri field.

As a proof of concept of this phenomenon, you can check the CSP report which sent by your browser from following page.
http://mallory.csrf.jp/cspleakage/log.txt



Expected results:

"blocked-uri" parameter has not to contain path and query string parameter of other origin than protected resources because it can be used for retrieving sensitive information e.g., OAuth token or session ID.

Similar issue was once fixed on Firefox 14 (see below).
https://www.mozilla.org/security/announce/2012/mfsa2012-53.html

However, there are still remaining issues in the latest branch.

Comment 1

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

Attachment: log.txt

Comment 2

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

it looks like the relevant piece here is this bit from the log.txt that's being generated

09/19/2014 02:23:03 pm
{"csp-report":{"blocked-uri":"https://www.facebook.com/muneaki.nishimura","document-uri":"http://mallory.csrf.jp/cspleakage/","original-policy":"default-src http://mallory.csrf.jp; report-uri http://mallory.csrf.jp/cspleakage/log.php","referrer":"","violated-directive":"default-src http://mallory.csrf.jp"}}

Comment 3

[Daniel Veditz [:dveditz]]

When the back-end was reimplemented in C++ we reintroduced bug 767778 -- don't show more than origin after a redirect. This appears to be my fault for not writing a test for it.

Comment 4

[Sid Stamm [:geekboy or :sstamm]]

Chris: this is related to bug 1049289, but has to do with stripping cross-origin "blocked-uri" down to the origin level.

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #4)
> Chris: this is related to bug 1049289, but has to do with stripping
> cross-origin "blocked-uri" down to the origin level.

Mhm, interesting, I wonder if other browsers have the same problem, because the spec only mentions that the fragment has to be removed[1]:

> blocked-uri
>    URI of the resource that was prevented from loading due to the policy violation, with any <fragment> component removed

Probably we should not only remove the fragment here:
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsCSPContext.cpp#608
but also everything after the origin.

[1] http://www.w3.org/TR/CSP/#report-uri

Comment 6

[Sid Stamm [:geekboy or :sstamm]]

It seems pretty clear about cross-origin privacy issues for blocked URIs and redirects:

"If violation reports contained the full blocked URL, the violation report might contain sensitive information contained in the redirected URI, such as session identifiers or purported identities. For this reason, the user agent includes only the origin of the blocked URI."

http://www.w3.org/TR/CSP/#violation-reports

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #6)
> It seems pretty clear about cross-origin privacy issues for blocked URIs and
> redirects:
> 
> "If violation reports contained the full blocked URL, the violation report
> might contain sensitive information contained in the redirected URI, such as
> session identifiers or purported identities. For this reason, the user agent
> includes only the origin of the blocked URI."
> 
> http://www.w3.org/TR/CSP/#violation-reports

Oh well, then we know what to do.

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1069762.patch

Yet another argument we have to propagate around :-( anyway, doing so seems like the easiest/best solution for fwiw.

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1069762_tests.patch

Unfortunately there was not an easy way to incorporate a test for blocked-uri into test_csp_reports.html because it uses a blocked-uri of 'self'. At least I was able to resue some of the test-setup from test_csp_path_matching_redirect.html.

Comment 10

[Daniel Veditz [:dveditz]]

The new C++ implementation was turned on in Firefox 33 (bug 925004)

Comment 11

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8507190 [details] [diff] [review]
bug_1069762.patch

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

I think this patch can be simplified quite a bit by utilizing a null check on aOriginalURI.

::: content/base/src/nsCSPContext.cpp
@@ +548,5 @@
>                            uint32_t aViolatedPolicyIndex,
>                            nsAString& aSourceFile,
>                            nsAString& aScriptSample,
> +                          uint32_t aLineNum,
> +                          bool aWasRedirected)

Please don't change the signature of sendreports if you don't need to.  I'm pretty sure aOriginalURI is null if this request wasn't redirected (it comes from AsyncReportViolation via CSPReportSenderRunnable).  You should be able to use that as a hint that it was redirected.

Probably wouldn't hurt to document SendReports with a good documentation comment like AsyncReportViolation has.

@@ +572,5 @@
>      // could be a string or URI
>      if (uri) {
> +      if (aWasRedirected) {
> +        // do not report anything else than the host in case of a redirect, see:
> +        // http://www.w3.org/TR/CSP/#violation-reports

This section says to send the origin, not just the host.  I think you want the prepath here.  Here's how we did it before:
http://mxr.mozilla.org/mozilla-release/source/content/base/src/contentSecurityPolicy.js#463

I don't suggest using the same logic, but you can use nsIURI::prePath to get the origin.

Comment 12

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8507192 [details] [diff] [review]
bug_1069762_tests.patch

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

Test looks good.  I don't agree with using host instead of origin, but I mentioned that in the other patch.  If you change the implementation to strip to prepath (origin) and not host, don't forget to update this test for that.

::: content/base/test/csp/test_csp_blocked_uri_in_reports.html
@@ +19,5 @@
> + * which gets redirected to:
> + *  http://test1.example.com/tests/content/base/test/csp/file_csp_path_matching.js
> + *
> + * The blocked-uri in the csp-report should be:
> + *   test1.example.com

I don't agree... it should be origin, not host, as per the spec.

@@ +26,5 @@
> + *
> + * see also: http://www.w3.org/TR/CSP/#violation-reports
> + *
> + * Note, that we reuse the test-setup from
> + * test_csp_path_matching_redirect.html

Can you factor it out, put it in a .js file and include it in both tests?  (Or is that ridiculous for these two tests?)

@@ +85,5 @@
> +        catch (e) {
> +          ok(false, "Could not parse JSON (exception: " + e + ")");
> +        }
> +        // test for the proper values in the report object
> +        window.checkResults(reportObj);

checkResults is pretty small... why not just inline it here?

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #11)
> Comment on attachment 8507190 [details] [diff] [review]
> bug_1069762.patch
> 
> Review of attachment 8507190 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think this patch can be simplified quite a bit by utilizing a null check
> on aOriginalURI.
> 
> ::: content/base/src/nsCSPContext.cpp
> @@ +548,5 @@
> >                            uint32_t aViolatedPolicyIndex,
> >                            nsAString& aSourceFile,
> >                            nsAString& aScriptSample,
> > +                          uint32_t aLineNum,
> > +                          bool aWasRedirected)
> 
> Please don't change the signature of sendreports if you don't need to.  I'm
> pretty sure aOriginalURI is null if this request wasn't redirected (it comes
> from AsyncReportViolation via CSPReportSenderRunnable).  You should be able
> to use that as a hint that it was redirected.

I wanted to do that initially, but the I figured we always set mSelfURI as the orignialURI. As discussed on IRC, that is not correct. So I refactored that part. I think we both agree that's what we want - just check the code in the patch. Will upload in a minute.
 
> This section says to send the origin, not just the host.  I think you want
> the prepath here.  Here's how we did it before:

Indeed, prePath is what we want - I have to update the test as well to reflect that change.

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #12)
> ::: content/base/test/csp/test_csp_blocked_uri_in_reports.html
> @@ +19,5 @@
> > + * which gets redirected to:
> > + *  http://test1.example.com/tests/content/base/test/csp/file_csp_path_matching.js
> > + *
> > + * The blocked-uri in the csp-report should be:
> > + *   test1.example.com
> 
> I don't agree... it should be origin, not host, as per the spec.

Totally agree!

 
> @@ +26,5 @@
> > + *
> > + * see also: http://www.w3.org/TR/CSP/#violation-reports
> > + *
> > + * Note, that we reuse the test-setup from
> > + * test_csp_path_matching_redirect.html
> 
> Can you factor it out, put it in a .js file and include it in both tests? 
> (Or is that ridiculous for these two tests?)

I think it's not worth the effort :-(

> @@ +85,5 @@
> > +        catch (e) {
> > +          ok(false, "Could not parse JSON (exception: " + e + ")");
> > +        }
> > +        // test for the proper values in the report object
> > +        window.checkResults(reportObj);
> 
> checkResults is pretty small... why not just inline it here?

Inlined it!

Comment 15

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1069762_tests.patch

Updated your nits; carrying over r+ from sstamm.

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1069762.patch

Comment 17

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8508350 [details] [diff] [review]
bug_1069762.patch

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

Yeah, this looks good.

Please document all the nsCSPContext::SendReports() arguments and what they are for or mean in a doc comment so we don't misuse arguments again.

::: content/base/src/nsCSPContext.cpp
@@ +571,5 @@
>      // could be a string or URI
>      if (uri) {
> +      if (aOriginalURI) {
> +        // do not report anything else than the origin in case of a redirect, see:
> +        // http://www.w3.org/TR/CSP/#violation-reports

Please make it really clear that aOriginalURI will be *null* unless the request for content we're trying to load was redirected, in which case it's the first URI.

@@ +593,5 @@
>  
>    // document-uri
> +  nsAutoCString reportDocumentURI;
> +  mSelfURI->GetSpecIgnoringRef(reportDocumentURI);
> +  report.mCsp_report.mDocument_uri = NS_ConvertUTF8toUTF16(reportDocumentURI);

Good catch.

Comment 18

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1069762.patch

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #17)
> Please document all the nsCSPContext::SendReports() arguments and what they
> are for or mean in a doc comment so we don't misuse arguments again.

That's a good idea. Incorporated that suggestion.
Carrying over r+ from sstamm.
This is ready to go.

Comment 19

[Christoph Kerschbaumer [:ckerschb]]

Dan, since this bug is categorized as sec-high how are we going to land that? Do you want me to land on mozilla-central and then get uplift approvals for beta and aurora? Should I land tests as well, or just the patch on mozilla-central?

Comment 20

[Daniel Veditz [:dveditz]]

In general sec-high/critical or unrated security bugs need a sec-approval, so add that to the patch and fill in the form (see https://wiki.mozilla.org/Security/Bug_Approval_Process)

We will probably want to get this on Aurora and Beta after some mozilla-central testing; that's part of what we decide when we process sec-approvals. The other part of what we decide is how long to hold off on landing if the patch looks like it gives the game away on a serious bug (lack-of-testing risk vs. exposure risk).

If the tests are really revelatory then we often don't check those in right away, and instead set the in-testsuite flag to '?'; later you come back and flip it to in-testsuite+ when you check the tests in. If you think it's likely the tests will be forgotten then some people create a second security bug (keyword sec-other) saying "check in tests for bug XXXX". On the other hand, in cases like this bug where the patch seems to make the problem pretty clear then we just throw up our hands and land the tests with the bug.

Comment 21

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8508873 [details] [diff] [review]
bug_1069762.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Fairly easy, see Description of the bug
https://bugzilla.mozilla.org/show_bug.cgi?id=1069762#c0

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Yes, the comments and in the code and also the tests highlight the problem.

Which older supported branches are affected by this flaw?

If not all supported branches, which bug introduced the flaw?
Bug 925004 and its dependents.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

How likely is this patch to cause regressions; how much testing does it need?
Not very likely to cause a regression since the blocked-uri in a csp-report is now only containing the 'prePath' instead of the full uri.

Comment 22

[Al Billings [:abillings - ex-MoCo]]

Giving sec-approval+ to check this in on 11/4. This is three weeks into cycle, which gives us a week to test before we need to uplift to Aurora and Beta. I'd have this go in earlier except the comments on sec-approval? make it clear that the issue is obvious once we're in.

Comment 23

[Daniel Veditz [:dveditz]]

Comment on attachment 8508873 [details] [diff] [review]
bug_1069762.patch

[Triage Comment]
When you check in please change the comment to something that's less "here's a security bug". Maybe

 "Bug 1069762 - CSP violation reports don't match the spec for redirects"
 (or the equivalent "make CSP violation reports match the spec for...")

a=dveditz for landing on Aurora and Beta.

Comment 24

[Daniel Veditz [:dveditz]]

Oh, and as discussed please DON'T check in the test until after we release Fx34. Some people like to indicate this by setting the "in-testsuite" flag to '?' as a reminder, and then set that flag to '+' when they eventually land the test; other people prefer cloning the bug to create a separate "land the test for bug 1069762" report as a reminder. Whichever best fits your workflow is fine.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6e99a4788c2f

Comment 26

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #25)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/6e99a4788c2f

Thanks Ryan!

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c4b670cb5a6d
https://hg.mozilla.org/releases/mozilla-beta/rev/b77384b124a4

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6e99a4788c2f

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/b77384b124a4

Comment 30

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

Confirmed in Fx33, and verified fixed in Fx34, Fx35 and Fx36, using builds from 2014-11-19.

Comment 31

[Daniel Veditz [:dveditz]]

Christoph: please land these tests and change the "in-testsuite" flag from '?' to '+'

Comment 32

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c6ed7442143b

Comment 33

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/c6ed7442143b

Comment 34

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Daniel Veditz [:dveditz] from comment #31)
> Christoph: please land these tests and change the "in-testsuite" flag from
> '?' to '+'

Here we go.