Closed Bug 1069762 (CVE-2014-1591) Opened 5 years ago Closed 5 years ago

CSP violation report contains sensitive data of other origin after redirect

Categories

(Core :: DOM: Security, defect)

35 Branch
x86_64
Windows 8
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla36
Tracking Status
firefox33 --- wontfix
firefox34 + verified
firefox35 + verified
firefox36 + verified
firefox-esr31 --- unaffected
b2g-v1.4 --- unaffected
b2g-v2.0 --- unaffected
b2g-v2.0M --- unaffected
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: sdna.muneaki.nishimura, Assigned: ckerschb)

References

Details

(Keywords: regression, sec-high, Whiteboard: [reporter-external][adv-main34+])

Attachments

(3 files, 3 obsolete files)

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.
Flags: sec-bounty?
Whiteboard: [reporter-external]
Attachment #8492183 - Attachment description: lot.txt → log.txt
Attachment #8492183 - Attachment filename: lot.txt → log.txt
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"}}
Component: Untriaged → DOM: Security
Product: Firefox → Core
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.
Assignee: nobody → mozilla
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: sec-high
See Also: → CVE-2012-1963
Summary: CSP violation report contains sensitive data of other origin → CSP violation report contains sensitive data of other origin after redirect
Chris: this is related to bug 1049289, but has to do with stripping cross-origin "blocked-uri" down to the origin level.
(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
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
(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.
Attached patch bug_1069762.patch (obsolete) — Splinter Review
Yet another argument we have to propagate around :-( anyway, doing so seems like the easiest/best solution for fwiw.
Attachment #8507190 - Flags: review?(sstamm)
Attached patch bug_1069762_tests.patch (obsolete) — Splinter Review
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.
Attachment #8507192 - Flags: review?(sstamm)
Status: NEW → ASSIGNED
The new C++ implementation was turned on in Firefox 33 (bug 925004)
Status: NEW → ASSIGNED
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.
Attachment #8507190 - Flags: review?(sstamm) → review-
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?
Attachment #8507192 - Flags: review?(sstamm) → review+
(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.
(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!
Updated your nits; carrying over r+ from sstamm.
Attachment #8507190 - Attachment is obsolete: true
Attachment #8507192 - Attachment is obsolete: true
Attachment #8508349 - Flags: review+
Attached patch bug_1069762.patch (obsolete) — Splinter Review
Attachment #8508350 - Flags: review?(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.
Attachment #8508350 - Flags: review?(sstamm) → review+
(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.
Attachment #8508350 - Attachment is obsolete: true
Attachment #8508873 - Flags: review+
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?
Flags: needinfo?(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.
Flags: needinfo?(dveditz)
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.
Attachment #8508873 - Flags: sec-approval?
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.
Whiteboard: [reporter-external] → [reporter-external][checkin on 11/04]
Attachment #8508873 - Flags: sec-approval? → sec-approval+
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.
Attachment #8508873 - Flags: approval-mozilla-beta+
Attachment #8508873 - Flags: approval-mozilla-aurora+
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.
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #25)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/6e99a4788c2f

Thanks Ryan!
https://hg.mozilla.org/mozilla-central/rev/6e99a4788c2f
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Flags: sec-bounty? → sec-bounty+
Whiteboard: [reporter-external] → [reporter-external][adv-main34+]
Alias: CVE-2014-1591
Confirmed in Fx33, and verified fixed in Fx34, Fx35 and Fx36, using builds from 2014-11-19.
Group: core-security → core-security-release
Christoph: please land these tests and change the "in-testsuite" flag from '?' to '+'
Group: core-security-release
Flags: needinfo?(mozilla)
(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.
Flags: needinfo?(mozilla)
Flags: in-testsuite?
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.