Closed Bug 945429 (CVE-2014-1515) Opened 11 years ago Closed 10 years ago

Do not handle file: paths by downloading them

Categories

(Firefox for Android Graveyard :: General, defect)

Product:
Firefox for Android Graveyard
Component:
General
Platform:
All
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox25 wontfix, firefox26 wontfix, firefox27 wontfix, firefox28+ verified, firefox29+ verified, firefox30+ verified, firefox-esr24 unaffected, b2g18 unaffected, b2g-v1.2 unaffected, b2g-v1.3 unaffected, b2g-v1.3T unaffected, b2g-v1.4 unaffected, fennec28+)

Status:
VERIFIED FIXED
Milestone:
Firefox 30
Tracking Flags:
Tracking Status
firefox25 --- wontfix
firefox26 --- wontfix
firefox27 --- wontfix
firefox28 + verified
firefox29 + verified
firefox30 + verified
firefox-esr24 --- unaffected
b2g18 --- unaffected
b2g-v1.2 --- unaffected
b2g-v1.3 --- unaffected
b2g-v1.3T --- unaffected
b2g-v1.4 --- unaffected
fennec 28+ ---

People

(Reporter: rnewman, Assigned: rnewman)

References

Details

(Keywords: csectype-disclosure, sec-high, Whiteboard: [adv-main28+] bounty nomination for bug 944373 reporter)

Attachments

(2 files, 1 obsolete file)

Prompt-to-download
272.54 KB, image/png
mfinkle
: feedback+
Details
Proposed patch. v2
6.50 KB, patch
wesj
: review+
mfinkle
: feedback+
abillings
: approval-mozilla-beta+
Details | Diff | Splinter Review 
This is a key aspect of the attack in Bug 944373: a page is loaded which includes an iframe with a src attribute pointing to a file in the profile directory. Firefox responds by downloading the (potentially sensitive) file to the SD card, which is world-readable.
I'm pretty sure we don't "download" file:// urls on desktop firefox, is this an android-only thing? Why was that added if so?
Keywords: csectype-disclosure, sec-high
Beats me. Presumably it's our default handling for files that we don't otherwise know how to display.

mfinkle, do you have more context?
Flags: needinfo?(mark.finkle)
I have no idea and need more context. HelperAppDialog handles most of our "download and launch" logic:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/HelperAppDialog.js
Flags: needinfo?(mark.finkle) → needinfo?(wjohnston)
Desktop also offers to download file: url's that it doesn't know how to open. We just don't have a prompt like they do. Salted profile names means it would be fairly difficult to get to something in the profile. I can't see bug 944373.
Flags: needinfo?(wjohnston)
(In reply to Wesley Johnston (:wesj) from comment #4)
> Desktop also offers to download file: url's that it doesn't know how to
> open. We just don't have a prompt like they do. Salted profile names means
> it would be fairly difficult to get to something in the profile. I can't see
> bug 944373.

Our salting is critically flawed (and always has been, FSVO "flawed"). That's Bug 944373.

Bug 944373 describes an attack that uses flawed salts, intents, and automatic downloads to read arbitrary data from inside the profile directory without user involvement.

They guess the salt, send an intent to open a simple local page like

<html><body><iframe src="file:///data/data/org.mozilla.firefox/1234.default/browser.db"/></body></html>

and then read your browser DB off the SD card, where we automatically save it. Nice. They can do this for your password DB, local storage, etc. etc.

The resolution of that bug will be "fix salting". (Though god forbid we've ever accidentally printed the profile directory to logcat.)

The resolution of this bug needs to be "don't automatically save file:/// URIs".
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
Comment on attachment 8342572 [details] [diff] [review]
Don't download files from the profile directory. WIP, v1

This patch is the kernel of a fix: you can still browse your profile directory, view RDF files etc., but if you -- or another app -- asks you to open a file that Firefox can't internally display, we refuse outright.

There's no UI for this -- we just flicker a tiny bit and go nowhere. Acceptable for this corner-case UX, I think, at least for now.
Attachment #8342572 - Flags: feedback?(mark.finkle)
I think we need to be smarter here. i.e. we should prompt on downloads, like everyone else does. Otherwise, we're going to keep hitting cases where we're downloading something we shouldn't (using our cookies and logins) and putting it in public storage.
(In reply to Wesley Johnston (:wesj) from comment #8)
> I think we need to be smarter here. i.e. we should prompt on downloads, like
> everyone else does. Otherwise, we're going to keep hitting cases where we're
> downloading something we shouldn't (using our cookies and logins) and
> putting it in public storage.

I would accept prompting for download if a user has browsed to the location manually (i.e., they typed it or used the file browser and clicked "browser.db" themselves).

I also see value in prompting for all downloads, which would ameliorate some of the attack you just described.

I don't see any value in allowing an intent to automatically download a file from inside our app directory. I don't know of any use case that that leakage supports.

I don't think adding a prompt in this case changes the word 'automatically': that's classic "blame the user" UX. "You clicked OK, so it's not a security bug!"  Small comfort to the user who just got pwned.

So a two-parter: one, reject intents that refer to files inside our application package; two, turn on prompting for downloads.

Does that sound good to you?
Comment on attachment 8342572 [details] [diff] [review]
Don't download files from the profile directory. WIP, v1

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

I waffle a bit. I don't really want special paths through here. I also really don't want to prompt on all downloads. You've got malware. On any other platform that means you're in much more trouble than Android. But on Android we have a chance to save you....

::: mobile/android/components/HelperAppDialog.js
@@ +22,5 @@
>  HelperAppLauncherDialog.prototype = {
>    classID: Components.ID("{e9d277a0-268a-4ec2-bb8c-10fdf3e44611}"),
>    QueryInterface: XPCOMUtils.generateQI([Ci.nsIHelperAppLauncherDialog]),
>  
> +  show: function(aLauncher, aContext, aReason) {

Don't do this.

@@ +30,5 @@
>      let apps = HelperApps.getAppsForUri(aLauncher.source, {
>        mimeType: aLauncher.MIMEInfo.MIMEType,
>      });
>  
> +    if (aLauncher.source.startsWith("file:")) {

This likely doesn't work. source is an nsIURI, not a string.

@@ +33,5 @@
>  
> +    if (aLauncher.source.startsWith("file:")) {
> +        // If it's in our profile directory, we never ever want to do anything
> +        // with it, including saving to disk or passing the file to another
> +        // application. Bug 945429.

I don't think we want security bugs linked to in source code.

@@ +34,5 @@
> +    if (aLauncher.source.startsWith("file:")) {
> +        // If it's in our profile directory, we never ever want to do anything
> +        // with it, including saving to disk or passing the file to another
> +        // application. Bug 945429.
> +        if (aLauncher.source.startsWith("file:///data/data/org.mozilla")) {

Same here. You should cancel the download if we're not going to open it. We should show a toast or something. We can remove that if we want to uplift this. Or we could reuse the string we use for protocols [1]. "Couldn't find an app to open this link".

I'd almost rather we just pref this black list though. i.e. we add a pref "browser.download.blacklist = comma,separated,regex,list"? If there are users who love to download from their profile, they can do the hard work to flip it.

http://mxr.mozilla.org/mozilla-central/source/mobile/android/locales/en-US/chrome/handling.properties#5

@@ +37,5 @@
> +        // application. Bug 945429.
> +        if (aLauncher.source.startsWith("file:///data/data/org.mozilla")) {
> +            return;
> +        }
> +    } else {

You likely want to fall through to here if this isn't a protected dir. i.e. no else.
(In reply to Wesley Johnston (:wesj) from comment #8)
> I think we need to be smarter here. i.e. we should prompt on downloads, like
> everyone else does. Otherwise, we're going to keep hitting cases where we're
> downloading something we shouldn't (using our cookies and logins) and
> putting it in public storage.

Does everyone else prompt for file downloads? Chrome does not prompt for all files. It seems to prompt for APKs but not much else.

I agree about stopping downloads from the profile folder though.
Comment on attachment 8342572 [details] [diff] [review]
Don't download files from the profile directory. WIP, v1

>diff --git a/mobile/android/components/HelperAppDialog.js b/mobile/android/components/HelperAppDialog.js

Just building on Wes' feedback

>+    if (aLauncher.source.startsWith("file:")) {

Like Wes said, this is an nsIURI, so use: if (aLauncher.source.schemeIs("file"))

>+        if (aLauncher.source.startsWith("file:///data/data/org.mozilla")) {

if you want to convert the nsIRUI to an nsIFile, use:
let file = aLauncher.source.QueryInterface(Ci.nsIFileURL).file;

if you want a way to get the nsIFile of the current application folder, use:
let appRoot = FileUtils.getFile("XREExeF", []);

nsIFile.path returns a string, so you could use:

if (file.path.startsWith(appRoot.path)) { ... }


In general, with Wes' feedback too, this approach is something we should move forward, we just need to get comfortable with it.
Attachment #8342572 - Flags: feedback?(mark.finkle) → feedback+
(In reply to Mark Finkle (:mfinkle) from comment #12)

> if you want to convert the nsIRUI to an nsIFile, use:
> let file = aLauncher.source.QueryInterface(Ci.nsIFileURL).file;
> 
> if you want a way to get the nsIFile of the current application folder, use:
> let appRoot = FileUtils.getFile("XREExeF", []);
> 
> nsIFile.path returns a string, so you could use:
> 
> if (file.path.startsWith(appRoot.path)) { ... }

nsIFile.contains makes this cleaner:
http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsIFile.idl?force=1#307

if (appRoot.contains(file, true)) { ... }
Thanks for the detailed reviews. I did say this was the kernel of a fix :P

I'd like to add another layer in our external intent handling. Thoughts?
(In reply to Richard Newman [:rnewman] from comment #14)
> Thanks for the detailed reviews. I did say this was the kernel of a fix :P
> 
> I'd like to add another layer in our external intent handling. Thoughts?

Sounds OK to me
Flags: sec-bounty?
Whiteboard: bounty nomination for bug 944373 reporter
Flags: sec-bounty? → sec-bounty+
Does the suggested patch also block files that can be handled by Firefox from being rendered? If not, it should. 

Oftentimes the adversary can indirectly inject arbitrary content into files found under the profile directory. If the adversary can inject a JavaScript payload into a Firefox-viewable file, he can leak it as well.
(In reply to Roee Hay from comment #17)
> Does the suggested patch also block files that can be handled by Firefox
> from being rendered? If not, it should. 

No.
 
> Oftentimes the adversary can indirectly inject arbitrary content into files
> found under the profile directory.

If you mean "an app can write into your profile dir, then open a page from there", I don't believe that to be the case -- Android will only allow that write if the attacker is also a Mozilla app.

If you mean "an app can open an arbitrary page, and thus write to the profile directory", then that would require an escalation of privileges for content.

> If the adversary can inject a JavaScript
> payload into a Firefox-viewable file, he can leak it as well.

The only concern would be if those files were loaded with chrome privileges, which they won't be if they're loaded from somewhere an attacker can write. That's no different to loading a page off the web, or a file off the disk on desktop, right?
I indeed meant the latter, "an app can open an arbitrary page, and thus write to the profile directory".
For example, just for clarification, a bad app can easily inject a JS payload into the cache by simply opening Firefox on an attacker's controlled website. 

Correct me if I am wrong, but privilege of escalation to chrome is not required (i.e. the attacker opens the file using the file:// URI) if the JS payload tries to leak the opened page itself which may contain sensitive information from other parties. For instance, let's hypothetically assume that the cookies database file is 'viewable', then if the attacker injected a JavaScript payload into it (by opening a website with a cookie name or value set to that payload), he could leak the cookies database.
Attached image Prompt-to-download — 
Re prompting for APK downloads: does this screenshot represent a user experience we're happy shipping?

(My device does not offer me Package Installer, even with the correct MIME type of application/vnd.android.package-archive.)
Attachment #8352092 - Flags: feedback?(mark.finkle)
This patch does the following:

* Checks whether the requested URL represents a non-downloadable file. At present, that means a file: URI rooted in the profile dir or the app dir (aiming for this to not break if a user's profile directory is stored elsewhere). If so, we cancel the request and attempt to show a toast: "Couldn't find an app to open this link".

Note the in-file TODO for using a prefs blacklist per Wes's suggestion. I'd like to punt that to a follow-up, at which point I'll file it and get a bug #.

* Checks whether the requested URL is an APK (by MIME type). If so, the usual one-matching-app logic is skipped, and an app selection prompt is always shown (see screenshot). I lightly commented this code for the suckers that come after us.

I wanted to use the supplied context argument to find the NativeWindow, but couldn't get it to work. I cribbed the getMostRecentWindow trick instead from ContentDispatchChooser.

I opted to use `url` over `aURL` because this file mixes the two. I'm happy to clean up the rest of the file's coding conventions if you wish.

No tests yet; I've manually tested the positive case, but I figured it's better to get this out the door than to let it languish.

mfinkle gets review, 'cos it's Christmas.
Attachment #8342572 - Attachment is obsolete: true
Attachment #8352307 - Flags: review?(mark.finkle)
(In reply to Richard Newman [:rnewman] from comment #20)
> Created attachment 8352092 [details]
> Prompt-to-download
> 
> Re prompting for APK downloads: does this screenshot represent a user
> experience we're happy shipping?
> 
> (My device does not offer me Package Installer, even with the correct MIME
> type of application/vnd.android.package-archive.)

This is one possible UI. I was thinking of something that let the user know that they were downloading a file that could be executed and be malicious. Chrome uses a UI like this to give the user a sense of warning.

Also, CC'ing and NI'ing Ian.
Flags: needinfo?(ibarlow)
(In reply to Mark Finkle (:mfinkle) from comment #22)

> This is one possible UI. I was thinking of something that let the user know
> that they were downloading a file that could be executed and be malicious.

I'm assuming that this is something we want to uplift, so can we break out a better UI into a second part?
Comment on attachment 8352092 [details]
Prompt-to-download

I'm treating this as the upliftable approach
Attachment #8352092 - Flags: feedback?(mark.finkle) → feedback+
Comment on attachment 8352307 [details] [diff] [review]
Proposed patch. v2

>diff --git a/mobile/android/components/HelperAppDialog.js b/mobile/android/components/HelperAppDialog.js

>+  getNativeWindow: function () {
>+    try {
>+      let win = Services.wm.getMostRecentWindow("navigator:browser");
>+      if (win && win.NativeWindow) {
>+        return win.NativeWindow;
>+      }
>+    } catch (e) {
>+    }
>+    return null;
>+  },

>   show: function hald_show(aLauncher, aContext, aReason) {

>+        Services.console.logStringMessage("Refusing download, but can't show a toast.");

XPCOM JS components should have dump(...) available. If you want the console, I'm OK with keeping it.

This approach seems fine to me, but I want Wes to have a look at the logic. Again, this is a upliftable approach.
Attachment #8352307 - Flags: review?(wjohnston)
Attachment #8352307 - Flags: review?(mark.finkle)
Attachment #8352307 - Flags: feedback+
Comment on attachment 8352307 [details] [diff] [review]
Proposed patch. v2

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

r+ if the jar file thing is ok... maybe there's nothing in jar's worth worrying about..

::: mobile/android/components/HelperAppDialog.js
@@ +38,5 @@
> +
> +  /**
> +   * Returns true if `url` represents a file: URL that we don't wish to download.
> +   */
> +  isNonDownloadableFile: function (url) {

_canDownload or _shouldDownload

@@ +39,5 @@
> +  /**
> +   * Returns true if `url` represents a file: URL that we don't wish to download.
> +   */
> +  isNonDownloadableFile: function (url) {
> +    if (!url.schemeIs("file")) {

Do we need to worry about jar files in the profile?

@@ +67,5 @@
> +  /**
> +   * Returns true if `launcher` represents a download for which we wish
> +   * to prompt.
> +   */
> +  isPromptWorthy: function (launcher) {

_shouldPrompt

@@ +138,5 @@
>      }
>  
> +    // If there's only one choice, and we don't want to prompt, go right ahead
> +    // and choose that app automatically.
> +    if (!requiresDownloadPrompt && (apps.length === 1)) {

Just do the this.shouldPrompt(launcher) call here.
Attachment #8352307 - Flags: review?(wjohnston) → review+
Is there any progress on releasing this fix?
(In reply to Roee Hay from comment #27)
> Is there any progress on releasing this fix?

Not yet; I've been working on other high-priority items. I should be circling back to this soon.
Group: firefox-core-security
(In reply to Richard Newman [:rnewman] from comment #28)

> Not yet; I've been working on other high-priority items. I should be
> circling back to this soon.

A month later and the patch has been reivewed for two months. Can we get this sec-high fixed soon?
This is lower priority than blocking-29 Sync work. This will be fixed soon, but not today.
I've revved my patch for this, and I'm currently working on a small extension, which Gavin is helping with. We should open a channel for some kinds of URI, and allow the channel to resolve to the innermost URI. This will stop any attacks that use a non-file scheme to eventually read a file.
I tested:

* Triggering a download from the web:

adb shell am start -a android.intent.action.VIEW -n org.mozilla.fennec_rnewman/.App -d "https://phs.googlecode.com/files/Download%20File%20Test.zip"

-- causes a download to start unprompted.

* Triggering an APK launch from local storage:

adb shell am start -a android.intent.action.VIEW -n org.mozilla.fennec_rnewman/.App -d "file:///mnt/sdcard/i386.apk"

-- causes an app picker prompt, including for Fennec to "Download", which if tapped causes a download. Tapping outside the dialog does not result in a download.

* Fetching three things from the profile:

adb shell am start -a android.intent.action.VIEW -n org.mozilla.fennec_rnewman/.App -d "file:///data/data/org.mozilla.fennec_rnewman/files/mozilla/g11xq8bf.default/"
adb shell am start -a android.intent.action.VIEW -n org.mozilla.fennec_rnewman/.App -d "file:///data/data/org.mozilla.fennec_rnewman/files/mozilla/g11xq8bf.default/browser.db"
adb shell am start -a android.intent.action.VIEW -n org.mozilla.fennec_rnewman/.App -d "file:///data/data/org.mozilla.fennec_rnewman/files/mozilla/g11xq8bf.default/times.json"


The directory and the JSON file result in browsing, not download. The download attempt on the DB shows "Couldn't find an app to download this link".


I believe this is all desired behavior, so I'm landing this now.
needinfo on me for uplift.
Flags: needinfo?(ibarlow) → needinfo?(rnewman)
https://hg.mozilla.org/mozilla-central/rev/fc9096b43f0b
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
> +  _canDownload: function (url, alreadyResolved=false) {
> +    Services.console.logStringMessage("_canDownload: " + url);

I'd like to remove this for leaking the URL, which we try to never do in logcats.

In fact, I'd like to remove all the "_canDownload:" debugging output. It's a bit too much noise.
My bad. That was my test logging that was supposed to be in a separate mq patch, but wasn't…

https://hg.mozilla.org/integration/fx-team/rev/709e9207641d
Comment on attachment 8352307 [details] [diff] [review]
Proposed patch. v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
  Fo'eva.

User impact if declined: 
  Security impact: malicious applications will be able to extract files from a user's profile directory, assuming that the malicious application can guess the profile salt.

Testing completed (on m-c, etc.): 
  Baked for a while. UI flows hand-tested.

Risk to taking this patch (and alternatives if risky): 
  Potential regressions in download handling. No alternatives.

String or IDL/UUID changes made by this patch:
  None.
Attachment #8352307 - Flags: approval-mozilla-aurora?
Flags: needinfo?(rnewman)
tracking-fennec: --- → 28+
Comment on attachment 8352307 [details] [diff] [review]
Proposed patch. v2

Changing the "aurora" approval to "beta" since the trains have moved one track and the "aurora" of that request is Firefox 29, which is now on beta.
Attachment #8352307 - Flags: approval-mozilla-aurora? → approval-mozilla-beta+
If we spin a 28.0.1 for video issues, would we take this?
Flags: needinfo?(abillings)
I don't think so but that would be up to release management.
Flags: needinfo?(abillings) → needinfo?(release-mgmt)
Actually, this looks low-risk enough to take as a ride-along with this mobile-only 28.0.1 so I'll consider it if it can be uplifted today.
status-firefox28: wontfix → affected
tracking-firefox28: --- → +
Flags: needinfo?(rnewman)
a=lsblakk on IRC

https://hg.mozilla.org/releases/mozilla-release/rev/532abd0fff60
status-b2g-v1.3T: --- → unaffected
status-firefox28: affected → fixed
Flags: needinfo?(rnewman)
Flags: needinfo?(release-mgmt)
Whiteboard: bounty nomination for bug 944373 reporter → [adv-main28+] bounty nomination for bug 944373 reporter
Alias: CVE-2014-1515
Verified; merely attempted to download e.g, browser.db from my profile and got the 'Couldn't find an app to open this file' message and *no download* on patched channels. WFM.
Status: RESOLVED → VERIFIED
status-firefox28: fixed → verified
status-firefox29: fixed → verified
status-firefox30: fixed → verified
Group: firefox-core-security
Blocks: CVE-2014-1566
Blocks: 1074505
Group: core-security
Depends on: CVE-2015-7186
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: