Bug 1208946 (CVE-2016-1955)

Firefox leaks URL invoked by other origins via CSP violation reports

VERIFIED FIXED in Firefox 45

Status

()

defect
VERIFIED FIXED
4 years ago
2 years ago

People

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

Tracking

({sec-moderate})

unspecified
mozilla46
Points:
---
Dependency tree / graph
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(firefox44 wontfix, firefox45+ verified, firefox46 verified, firefox-esr38 wontfix)

Details

(Whiteboard: [adv-main45+] verify on both Fennec and Desktop when fixed)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
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.
I don't know enough about CSP to know if these are the expected results – Daniel?
Flags: needinfo?(dveditz)
Flags: sec-bounty?
(Reporter)

Comment 2

4 years ago
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'.
(Assignee)

Updated

4 years ago
Keywords: sec-moderate
(Reporter)

Comment 3

4 years ago
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?
We should double-check that Fennec is fixed after this bug is fixed, however.

NI Matt to help delegate to desktop team.
Component: General → General
Flags: needinfo?(MattN+bmo)
Product: Firefox for Android → Firefox
Summary: Fennec leaks intent: URL invoked by other origins via CSP violation reports → Firefox leaks URL invoked by other origins via CSP violation reports
Version: Firefox 44 → unspecified
(Reporter)

Comment 5

4 years ago
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.
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
Group: firefox-core-security → dom-core-security
Component: General → DOM: Security
Flags: needinfo?(dveditz)
Product: Firefox → Core
Whiteboard: verify on both Fennec and Desktop when fixed
This falls under the platform (security) team responsibilities, not the Firefox front-end team
Flags: needinfo?(MattN+bmo)
Christoph: We're probably violating the reporting spec here. Can you check on this?
Flags: needinfo?(mozilla)
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.
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Flags: needinfo?(mozilla)
Attachment #8699669 - Flags: review?(dveditz)
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.
Attachment #8699694 - Flags: review?(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.
Attachment #8699669 - Flags: review?(dveditz) → review-
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.
Attachment #8699694 - Flags: review?(dveditz) → review+
(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!
Attachment #8699669 - Attachment is obsolete: true
Attachment #8707667 - Flags: review?(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
Attachment #8707667 - Flags: review?(dveditz) → review+
https://hg.mozilla.org/mozilla-central/rev/92ab6c992c02
https://hg.mozilla.org/mozilla-central/rev/b93fead054f5
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
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
Flags: needinfo?(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.
Flags: needinfo?(dveditz)
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
Attachment #8707667 - Flags: approval-mozilla-aurora?
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.
Attachment #8699694 - Flags: approval-mozilla-aurora?
Kamil, any chance you could verify that this works correctly on Fennec now?
Flags: needinfo?(kjozwiak)
Group: dom-core-security → core-security-release
Comment on attachment 8699694 [details] [diff] [review]
bug_1208946_strip_uris_for_reporting_tests.patch

Fix a sec issue, has tests, taking it.
Attachment #8699694 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8707667 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: sec-bounty? → sec-bounty+
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.
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?
Flags: needinfo?(kjozwiak) → needinfo?(mozilla)
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!
Flags: needinfo?(mozilla)
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.
Status: RESOLVED → VERIFIED
Whiteboard: verify on both Fennec and Desktop when fixed → [adv-main45+] verify on both Fennec and Desktop when fixed
Alias: CVE-2016-1955
(Reporter)

Comment 28

3 years ago
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.
Flags: needinfo?(abillings)
Advisory is live now but I can edit it.
Flags: needinfo?(abillings)
What is "nishimunea"?
Flags: needinfo?(sdna.muneaki.nishimura)
(Reporter)

Comment 31

3 years ago
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.
Flags: needinfo?(sdna.muneaki.nishimura)
(Assignee)

Updated

3 years ago
Depends on: 1291967
(Assignee)

Updated

3 years ago
Depends on: 1298505
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.