1050690 - (CVE-2014-1566) Download arbitrary files to SD card via additional slashes in file: URI

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Yu Dongsong]

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

Steps to reproduce:

If we input the URL in the address bar such as:
file:///data/data/org.mozilla.firefox/files/mozilla/xxxxxxxx.default/cookies.sqlite
the download will not be invoked because the update in Firefox 28.0.1
But we can simply add ‘/’ before folders in the URL:
file:////data/data/org.mozilla.firefox/files/mozilla/gnxblohb.default/cookies.sqlite


Actual results:

The cookies will be automatically downloaded in the SDcard so that all apps in the devices get access to the cookies which save users privacy information.


Expected results:

No matter what users input in the address bar, the cookies should not be downloaded into SDcard.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

bug 945429 should have fixed this. Is it not robust enough?

Comment 2

[Richard Newman [:rnewman]]

(In reply to Mark Finkle (:mfinkle) from comment #1)
> bug 945429 should have fixed this.

It did: from Comment 0: "the download will not be invoked because the update in Firefox 28.0.1".

> Is it not robust enough?

I thought we normalized the URL, but apparently not enough!

Comment 3

[Wesley Johnston (:wesj)]

Looks like nsIFile.compare just uses a strcmp function, which fails here because of the extra '/'. Is this a platform bug?
http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsLocalFileUnix.cpp#1744

Comment 4

[Richard Newman [:rnewman]]

The nsIIOService trick we use to resolve the URL doesn't clean up in this case.

let url = makeURI("file:////Users/rnewman/");
url.schemeIs("file");        // true
let ioSvc = Cc["@mozilla.org/network/io-service;1"].getService(Components.interfaces.nsIIOService);
let resolved = ioSvc.newChannelFromURI(url).URI;
url.equals(resolved);        // true
resolved.path;               // "//Users/rnewman/"

And indeed that causes containment to be incorrect:

let subdir       = makeURI("file:////Users/rnewman/foo/").QueryInterface(Ci.nsIFileURL).file;
let dirSlashed   = makeURI("file:////Users/").QueryInterface(Ci.nsIFileURL).file;
let dirUnslashed = makeURI("file:///Users/").QueryInterface(Ci.nsIFileURL).file;

dirSlashed.contains(subdir, true);      // true
dirUnslashed.contains(subdir, true);    // false

even though a path with an arbitrary number of leading slashes is still readable.

Comment 5

[Richard Newman [:rnewman]]

Attachment: Proposed patch. v1

I haven't tested this in-product yet, but I have tested the normalization itself:

let parent = makeURI("file:///Users/").QueryInterface(Ci.nsIFileURL).file;
let child = makeURI("file://////Users/rnewman/").QueryInterface(Ci.nsIFileURL).file;

parent.contains(child, true);       // false
child.normalize();
parent.contains(child, true);       // true

Comment 6

[Wesley Johnston (:wesj)]

Comment on attachment 8470119 [details] [diff] [review]
Proposed patch. v1

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

::: mobile/android/components/HelperAppDialog.js
@@ +70,5 @@
>        // If it's in our app directory or profile directory, we never ever
>        // want to do anything with it, including saving to disk or passing the
>        // file to another application.
>        let file = url.QueryInterface(Ci.nsIFileURL).file;
> +      file.normalize();

You might add a comment here so this isn't removed by someone in the future.

Comment 7

[Richard Newman [:rnewman]]

https://hg.mozilla.org/integration/fx-team/rev/8e80d6b6f9af

Comment 8

[Richard Newman [:rnewman]]

Comment on attachment 8470119 [details] [diff] [review]
Proposed patch. v1

Approval Request Comment
[Feature/regressing bug #]:
  Omission from Bug 945429.

[User impact if declined]:
  See Bug 945429.

[Describe test coverage new/current, TBPL]:
  Verified by hand.

[Risks and why]: 
  Should be zero risk. Would take for release if that happens.

[String/UUID change made/needed]:
  None.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8e80d6b6f9af

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Richard, why particular reason you are not requesting an uplift to ESR31? FYI, ESR is going to be used for the ARMv6/Android 2.2 fennec release.

Comment 11

[Richard Newman [:rnewman]]

Comment on attachment 8470119 [details] [diff] [review]
Proposed patch. v1

(In reply to Sylvestre Ledru [:sylvestre] from comment #10)
> Richard, why particular reason you are not requesting an uplift to ESR31?
> FYI, ESR is going to be used for the ARMv6/Android 2.2 fennec release.

No reason why not.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6d461156b945
https://hg.mozilla.org/releases/mozilla-beta/rev/6b9281a057b7
https://hg.mozilla.org/releases/mozilla-esr31/rev/50f390ac20b0

Comment 13

[Aaron Train [:aaronmt]]

# start -a android.intent.action.VIEW -n org.mozilla.fennec/.App -d file:////data/data/org.mozilla.fennec/files/mozilla/of48tywi.default/cookies.sqlite

'Couldn't find an app to open this link'

Comment 14

[Daniel Veditz [:dveditz]]

We should also test that normalize deals with extra slashes elsewhere in the path, as well as path traversal attempts (/./ and /../ etc)

Comment 16

[Daniel Veditz [:dveditz]]

We're awarding the bounty because this is equivalent to the other problem, and on older versions of Android there are still avenues for malicious apps to learn the salted profile name.

Comment 17

[Aaron Train [:aaronmt]]

(In reply to Daniel Veditz [:dveditz] from comment #14)
> We should also test that normalize deals with extra slashes elsewhere in the
> path, as well as path traversal attempts (/./ and /../ etc)

As far as my testing here in local profile file downloading, these cases seem covered with the normalize() call.