1208946 - (CVE-2016-1955) Firefox leaks URL invoked by other origins via CSP violation reports

Status: VERIFIED FIXED

(Core :: DOM: Security, defect)

Description

[Muneaki Nishimura]

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

Steps to reproduce:

1. Open an attacker's page https://mallory.csrf.jp/intent/csp/ by
Firefox for Android.
This page is protected by following CSP header.
Content-Security-Policy-Report-Only:
default-src 'self' https:; script-src 'self' 'unsafe-inline'; report-uri log.php
And the page opens target website (e.g. alice.csrf.jp) in an iframe.

2. Click a link "Click to Open Google Maps" in the iframe. Then the
Google Maps app is launched from alice.csrf.jp

3. Open https://mallory.csrf.jp/intent/csp/report.txt to show the CSP
violation report sent to the attacker.


Actual results:

Then you can see the detailed contents of intent: URL in the report like this.
"blocked-uri":"intent://maps.google.com/maps?um=1&ie=UTF-8&fb=1&sll=52.54114,13.44009&sspn=5.146615,11.2856986&q=Prenzlauer+Berg,+Germany&entry=s"


Expected results:

When you try the steps on Chrome the value of blocked-uri in the
report is "intent" only. Firefox should do so.

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

I don't know enough about CSP to know if these are the expected results – Daniel?

Comment 2

[Muneaki Nishimura]

My further investigation revealed that this is more general issue regarding to GUID URL schemes of CSP that is required in section 4.4 of CSP2 spec.
http://www.w3.org/TR/CSP2/#violation-reports

The following URL is a PoC that steals visited user's private contact information.
http://mallory.csrf.jp/intent/csp/contacts.html

When you click tel:, skype: or mailto: links in an iframe (suppose an external SNS is opened by the frame) the contact information in the links are sent to the attacker's server via violation reports.

Then Chrome sends these schemes only e.g., 'tel' 'skype' 'mailto'.

Comment 3

[Muneaki Nishimura]

Thanks for handling this. It seems that this bug affects not only Fennec but Firefox for PC.
Could someone change the title and affected components of this bug appropriately?

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

We should double-check that Fennec is fixed after this bug is fixed, however.

NI Matt to help delegate to desktop team.

Comment 5

[Muneaki Nishimura]

I think this is more severe. My further testing found that any types of other origin URL loaded in an iframe can be leaked.

For example, when you click a link text "Place (Bing Map)" in following link.
http://mallory.csrf.jp/intent/csp/contacts.html
The Bing Map URL containing user's location is sent to the mallory as a report. Here, the Bing Map URL is generated by other origin server (e.g. alice.csrf.jp) so the parent page should not know.

http://www.bing.com/mapspreview?&ty=18&q=tokyo&satid=id.sid%...

As far as I tried Chrome always removes significant parts of URL in other origin.

Comment 6

[Daniel Veditz [:dveditz]]

There are two conditions when the spec says the frame-src directive comes into play: the protected resource (the page with the CSP policy) specifying or changing the iframe's source from outside, and the iframe itself navigating from within. The latter crept into the spec in version 2. It's a bit asymmetric since we don't have any directives that control top-level navigation and so our reporting didn't really think about that case.

I don't think we have hooks in the right places, but ideally externally setting the iframe src property (via script, or present in the original HTML) from the containing page ("protected resource") reports the full new URL if it's blocked, but changes from within the iframe should be treated the way we do redirects -- only show the origin (or even just the scheme for non-hierarchical schemes) if it's not same-origin. If you squint, the user clicking on a link, or a script navigating after a setTimeout() are just "slow redirects" as far as the outer page is concerned.

http://www.w3.org/TR/CSP/#source-list-paths-and-redirects

Comment 7

[Matthew N. [:MattN]]

This falls under the platform (security) team responsibilities, not the Firefox front-end team

Comment 8

[Daniel Veditz [:dveditz]]

Christoph: We're probably violating the reporting spec here. Can you check on this?

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

Thanks for reporting. The whole stripping of URIs before sending reports is actually completely broken, we should really get that fixed. I'll upload a patch in a minute.

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1208946_strip_uris_for_reporting.patch

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1208946_strip_uris_for_reporting_tests.patch

Dan, I extended test_csp_reports.js to include tests, so that:
* only a uri's scheme is reported for globally unique identifiers
* fragment parts get removed of a uri
* cross-origin uri's are stripped down to the origin.

Further, I had to update some other tests, e.g. within test_report_for_import.html

Finally, I tried to update test_bug836922_npolicies.html because it was testing for the wrong behavior, but that test is too complicated to update, so I had to remove the cross-origin bits.

Anyway, the newly added tests within test_csp_reports.js should provide all the test coverage we need.

Comment 12

[Daniel Veditz [:dveditz]]

Comment on attachment 8699669 [details] [diff] [review]
bug_1208946_strip_uris_for_reporting.patch

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

r- to address my concerns. Either way is easy but we need to decide which.

::: dom/security/nsCSPContext.cpp
@@ +675,5 @@
> + *
> + * @param aURI
> + *        The uri to be stripped for reporting
> + * @param aProtectedResourcePrincipal
> + *        The loadingPricnipal of the protected resource

typo: Principal

@@ +692,5 @@
> +    (NS_SUCCEEDED(aURI->SchemeIs("http", &isHttp)) && isHttp) ||
> +    (NS_SUCCEEDED(aURI->SchemeIs("https", &isHttp)) && isHttp);
> +  if (!isHttp) {
> +    // if we are not dealing with http/https then it must
> +    // be a globally unique identifier.

This comment is incorrect, there are many non-http hierarchical schemes that are not "globally unique identifiers". The _code_ here might be all we care about (how often is ftp: going to come up, or an add-on going to add back gopher: support?) but it's not really what the spec was talking about.

We could either
1) leave the code alone but change the comment to document that we only care about http: origins. Not strictly spec compliant but I bet there won't be any tests for ftp: and it's really all we care about.
2) change the code to check for a hierarchical scheme instead, maybe by checking to see if the nsIURI has a host? If it doesn't have a host return the scheme only, otherwise fall into the same-origin checks below.

Comment 13

[Daniel Veditz [:dveditz]]

Comment on attachment 8699694 [details] [diff] [review]
bug_1208946_strip_uris_for_reporting_tests.patch

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

r+

depending on the decision about the code in the other patch you could add a test for ftp://example.com/somefile to make sure the results are as expected.

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1208946_strip_uris_for_reporting.patch

(In reply to Daniel Veditz [:dveditz] from comment #12)
> We could either
> 1) leave the code alone but change the comment to document that we only care
> about http: origins. Not strictly spec compliant but I bet there won't be
> any tests for ftp: and it's really all we care about.

I think you are right, hence I updated the comment to reflect that we basically only care about http and https. I think that makes the most sense and we should definitely not try to over-engineer the problem we are trying to solve.

Thanks Dan!

Comment 15

[Daniel Veditz [:dveditz]]

Comment on attachment 8707667 [details] [diff] [review]
bug_1208946_strip_uris_for_reporting.patch

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

I can live with that. r=dveditz

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/92ab6c992c02
https://hg.mozilla.org/mozilla-central/rev/b93fead054f5

Comment 17

[Christoph Kerschbaumer [:ckerschb]]

Dan, incorrect stripping of URLs for reports has apparently been a problem for a long time by now. Given that the test update for dom/security/test/csp/test_report_for_import.html within this bug has originally landed in 43 [1] indicates that we have had that problem for a long time. My feeling tells me that we have had that problem even before we switched to the C++ backend.

What do you think, how far should we uplift? Given that the severity is sec-moderate and we are pretty late in the cycle I would suggest uplifting to 45 is sufficient, but I would like to hear your opinion.

[1] http://hg.mozilla.org/mozilla-central/rev/02c6d6aef163

Comment 18

[Daniel Veditz [:dveditz]]

uplift to 45 is fine. We're building 44 release candidates this week and this is definitely not a "stop-ship" kind of bug that can halt and hop on that train.

Comment 19

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8707667 [details] [diff] [review]
bug_1208946_strip_uris_for_reporting.patch

Approval Request Comment
URI stripping when sending CSP reports seems to have been completely broken for a long time, this bug fixes the issue.

[Feature/regressing bug #]:
Not completely certain, but most likely when we introduced the new C++ backed for CSP, which was Bug 951457.

[User impact if declined]:
Sending CSP reports might include user confidential data in fragements, paths, or also data: URIs.

[Describe test coverage new/current, TreeHerder]:
Manually verified but also the test attached to this bug.

[Risks and why]:
Low, because it only affects pages that use CSP reports.

[String/UUID change made/needed]:
no

Comment 20

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8699694 [details] [diff] [review]
bug_1208946_strip_uris_for_reporting_tests.patch

Approval Request Comment
These are the tests which need to be uplifted.

Please see comment 19 for details.

Comment 21

[Christoph Kerschbaumer [:ckerschb]]

Kamil, any chance you could verify that this works correctly on Fennec now?

Comment 22

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8699694 [details] [diff] [review]
bug_1208946_strip_uris_for_reporting_tests.patch

Fix a sec issue, has tests, taking it.

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/481ff8485da5
https://hg.mozilla.org/releases/mozilla-aurora/rev/ac26deb3a325

Comment 24

[Kamil Jozwiak [:kjozwiak]]

Reproduced the issue using the following fennec m-c build:
* https://archive.mozilla.org/pub/mobile/nightly/2015-09-27-03-03-00-mozilla-central-android-api-11/

01/26/2016 10:47:16 am
{"csp-report":{"blocked-uri":"intent://maps.google.com/maps?um=1&ie=UTF-8&fb=1&sll=52.54114,13.44009&sspn=5.146615,11.2856986&q=Prenzlauer+Berg,+Germany&entry=s","document-uri":"https://mallory.csrf.jp/intent/csp/","original-policy":"default-src https://mallory.csrf.jp; script-src https://mallory.csrf.jp 'unsafe-inline'; report-uri https://mallory.csrf.jp/intent/csp/log.php","referrer":"","violated-directive":"default-src https://mallory.csrf.jp"}}

Verified using the following fennec builds:
* https://archive.mozilla.org/pub/mobile/nightly/2016/01/2016-01-25-06-06-32-mozilla-central-android-api-11/
* https://archive.mozilla.org/pub/mobile/nightly/2016/01/2016-01-25-06-06-33-mozilla-aurora-android-api-11/

Followed the STR from comment #0 and ensured that the full URL isn't being included in the report/log, example:

01/26/2016 11:13:07 am
{"csp-report":{"blocked-uri":"intent","document-uri":"https://mallory.csrf.jp/intent/csp/","original-policy":"default-src https://mallory.csrf.jp; script-src https://mallory.csrf.jp 'unsafe-inline'; report-uri https://mallory.csrf.jp/intent/csp/log.php","referrer":"","violated-directive":"default-src https://mallory.csrf.jp"}}

I'll finish verification on desktop tomorrow.

Comment 25

[Kamil Jozwiak [:kjozwiak]]

Reproduced the issue using the following m-c desktop build:
* http://archive.mozilla.org/pub/firefox/nightly/2015/09/2015-09-27-03-03-00-mozilla-central/

I ran my own server (hapi.js/scooter/blankie) and used https://report-uri.io/ to collect the reports. Using the above build, received the following RAW report (the URL also appeared under the "Blocked URI column"):

{
    "csp-report": {
        "blocked-uri": "intent://maps.google.com/maps?um=1&ie=UTF-8&fb=1&sll=52.54114,13.44009&sspn=5.146615,11.2856986&q=Prenzlauer+Berg,+Germany&entry=s",
        "document-uri": "http://localhost:22935/csptest/test.html",
        "original-policy": "connect-src http://localhost:22935; default-src http://localhost:22935; img-src http://localhost:22935; script-src http://localhost:22935 'unsafe-inline'; style-src http://localhost:22935; report-uri https://report-uri.io/report/c846ec585393ac4213a8559e81c4902e/reportOnly",
        "referrer": "",
        "violated-directive": "default-src http://localhost:22935"
    }
}

Went through verification using the following desktop build:
* http://archive.mozilla.org/pub/firefox/nightly/2016/01/2016-01-26-03-02-44-mozilla-central/
* http://archive.mozilla.org/pub/firefox/nightly/2016/01/2016-01-26-00-40-04-mozilla-aurora/

Followed the STR from comment #0 and ensured that the full URL isn't being included in the report/log, example:

{
    "csp-report": {
        "blocked-uri": "intent",
        "document-uri": "http://localhost:22935/csptest/test.html",
        "original-policy": "connect-src http://localhost:22935; default-src http://localhost:22935; img-src http://localhost:22935; script-src http://localhost:22935 'unsafe-inline'; style-src http://localhost:22935; report-uri https://report-uri.io/report/c846ec585393ac4213a8559e81c4902e/reportOnly",
        "referrer": "",
        "violated-directive": "default-src http://localhost:22935"
    }
}

Chris, it seems like fx45 is having build issue via bug # 1242641.. So I had to resort to the following tinderbox-builds for both fennec/desktop:
* http://archive.mozilla.org/pub/mobile/tinderbox-builds/mozilla-beta-android-api-11/1453737022/
* http://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-beta-macosx64/1453762586/

Is this sufficient enough for fx45 verification?

Comment 26

[Christoph Kerschbaumer [:ckerschb]]

Kamil, I tested myself using desktop builds and can verify it works as expected on Desktop. Most importantly was to make sure this works on Fennec (which it does according to comment 24).

Thanks for your help!

Comment 27

[Kamil Jozwiak [:kjozwiak]]

Thanks Chris! Looks like the fx45 builds have finally been fixed. I went through the same verification process on the following builds:

- https://archive.mozilla.org/pub/firefox/candidates/45.0b1-candidates/build1/
- https://archive.mozilla.org/pub/mobile/candidates/45.0b1-candidates/build1/android-api-11/

Marking this as verified as per comment # 24, comment # 25, comment # 26 and comment # 27.

Comment 28

[Muneaki Nishimura]

Thank you for the response to the fix.
If you have plan to publish advisory of this issue, I wonder if you could use following name as a reporter.

Muneaki Nishimura (nishimunea) of Recruit Technologies Co.,Ltd.

Comment 29

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

Advisory is live now but I can edit it.

Comment 30

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

What is "nishimunea"?

Comment 31

[Muneaki Nishimura]

Thank you for writing my name in your advisory.
"nishimunea" is my nickname that comes from two parts of my name NISHI-mura MUNEA-ki.
I usually use it on the internet so it's more identifiable than my real name.