844832 - (CVE-2013-0798) Installation directory rights allows all

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

Date: Mon, 25 Feb 2013 10:47:20 +0900
From: Shuichiro Suzuki <[e-mail]>
To: [e-mail]
Subject: Security Bug in Firefox for Mobile(Android)
-----//-----
Dear Sir/Madam

Hi. I'm a security researcher in Fourteenforty Research Institute in Japan.
I found a security related bug in Firefox for Android and report it here.


Abstract:
The installation of the Firefox for Android (FFA) makes app_tmp directory
world readable and writable(777).
With this configuration other applications(might be a malcode) can replace
any addons installed through FFA. This leads installing mal-addons without
any awareness from users.

Scenario:
A user installs an android app (which is actually malware) and run it.
Then the user tries to install a legitimate addon from Mozilla Addons site(https://addons.mozilla.org).
FFA download the addon into temporary directory(app_tmp dir) and keep it there and show dialog to ask if
the user really installs it.
At this point the malware running background can replace the addon in the temporary directory since the app_tmp directory permission is 777.
When the user tap on "yes" on the dialog FFA installs the replaced addon with the name of the original(downloaded) addon's name.
Since addons for FFA has a great power of stealing any data in the browser this may leads security problem.

Technical Detail:
The main problem here is that the app_tmp directory's permission is 777.
I do not know how this directory is used but this can be 700 (Since it seems that other apps do not need to know what is in the dir)
And also you should ensure that downloaded and installed addons are the same thing.

Others:
Android has a class "FileObserver" which notifies an application when a file or directory content is changed.
The malware can use this class with path "/data/data/org.mozilla.firefox/app_tmp". This enables the malware
to replace the addon downloaded by FFA just after download is finished and just before the user tap on "yes" button on the installation dialog.
The malware do not need any special permission.

This may not a problem in Windows platform since its security model is different from Android one.
In Windows any software other than Firefox may be able to replace any addons within the same user's context.
Since security concept of the Android is different from Windows's one I consider this as a security problem.

Best regards,

Shuichiro Suzuki
Fourteenforty Research Institute
[e-mail]

Comment 1

[Kevin Brosnan [Ex-Mozilla]]

CC'ed some mobile developers to find out what /app_tmp/ is used for.

Comment 2

[Gian-Carlo Pascutto [:gcp]]

Crash dumps, and probably indeed plugin installation as the reporter says:

shell@android:/ # ll /data/data/org.mozilla.firefox_beta/                      
drwxrwx--x app_100  app_100           2012-10-01 16:52 app_plugins
drwxrwxrwx app_100  app_100           2013-02-25 21:34 app_tmp
drwxrwx--x app_100  app_100           2013-02-21 20:54 cache
drwx------ app_100  app_100           2012-06-08 10:15 components
drwxrwx--x app_100  app_100           2013-01-11 13:42 databases
drwx------ app_100  app_100           2012-04-18 21:46 extensions
drwxrwx--x app_100  app_100           2012-04-18 21:46 files
drwxr-xr-x system   system            2013-02-21 08:34 lib
-rw------- app_100  app_100        80 2012-06-06 00:43 removed-files
drwx------ app_100  app_100           2012-12-07 20:41 res
drwxrwx--x app_100  app_100           2013-02-26 08:00 shared_prefs

There is indeed zero reason why that should be world-writable (and probably not readable either).

Comment 3

[Gian-Carlo Pascutto [:gcp]]

Note how we intentionally silenced the warning that we shouldn't be doing this:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/mozglue/GeckoLoader.java.in#101

Comment 4

[Gian-Carlo Pascutto [:gcp]]

This goes way back, all current versions are affected:

http://hg.mozilla.org/mozilla-central/rev/70f37cb8fdde#l1.157

Kats, Blassey, do you remember any reason why this had to be WORLD_WRITABLE?

Comment 5

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch 1. Make tmpdir user-read-write-only.

Changing the permissions of the old dir appears to be tricky (Android itself only provides the functions on API level 9 and up), so let's just create a new dir with the right perms.

Given that there shouldn't really be anything in there, and we don't be accessing it again, I think we don't need to bother with removing the old one?

Comment 6

[Gian-Carlo Pascutto [:gcp]]

Installing add-ons still works with this patch, can't see any reason an app with another userid would want to poke in our files anyway.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

(In reply to Gian-Carlo Pascutto (:gcp) from comment #4)
> Kats, Blassey, do you remember any reason why this had to be WORLD_WRITABLE?

I don't know. I wasn't even around when that was added. My only interaction with file permissions in app_tmp has been in bug 799686 (with followup in bug 806486) where I ensure the about:memory dump that gets written to app_tmp is world-readable. That also implicitly assumes that the app_tmp folder itself is world-readable, so that the file can be pulled off the device.

(In reply to Gian-Carlo Pascutto (:gcp) from comment #5)
> Changing the permissions of the old dir appears to be tricky (Android itself
> only provides the functions on API level 9 and up), so let's just create a
> new dir with the right perms.

I think it still needs to be world-readable, so that the about:memory dumps can be read from it.

> Given that there shouldn't really be anything in there, and we don't be
> accessing it again, I think we don't need to bother with removing the old
> one?

This seems like a reasonable approach given the API level restriction. We could also do a "migration" where we move all the contents of the old one into the new one and optionally then replace the old one with the new one. More risky to get the patch right but a better chance of things not breaking afterwards. With this patch I know I'll have to update my AWSY harness, not sure if there are other pieces of code out that there assume the dir name as well.

Comment 8

[Gian-Carlo Pascutto [:gcp]]

The tricky thing here is that I didn't see any way to probe the existence of the old directory without creating it. Context.getDir doesn't have a flag to just return the File object without actually creating it. So removing the old dir cleanly isn't possible unless we make assumptions on the directory structure Android creates (bah!).

Alternatively, we can do getDir("tmp", MODE_PRIVATE), and then fire off a chmod in native code to make sure it's no longer world-writable if it was. I have to say it's not all that appealing either.

>I think it still needs to be world-readable, so that the about:memory dumps can be read from it.

Can we put these elsewhere, to avoid nasty surprises later? Can't you write those to external storage (which has no permissions AFAIK)? I mean having a world-accessible dir there just for debugging memory dumps? Blah. Or can we make it so this only gets created as world-readable if you APK has the debugging flag set?

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Discussed this on IRC with gcp. We decided to move the about:memory dumps to somewhere on /sdcard (I'm going to use the downloads dir for convenience, since we should already have an environment variable that points there) and gcp will updated the java-side code to switch to a new tmp dir that is not readable.

Adding :njn as an FYI. I will file a separate bug for moving the about:memory dump.

Comment 10

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch 1. Make new tempdir and remove old one.

Comment 11

[Gian-Carlo Pascutto [:gcp]]

Given that exploitation requires the user to install malware on the local device, or get malware through another exploit, before he can use this vulnerability, this looks like sec-moderate to me.

Comment 12

[Mark Goodwin [:mgoodwin]]

(In reply to Gian-Carlo Pascutto (:gcp) from comment #11)
> Given that exploitation requires the user to install malware on the local
> device, or get malware through another exploit, before he can use this
> vulnerability, this looks like sec-moderate to me.

It's true the user needs to do those things but it's still dangerous; an app that's only been granted filesystem access can effectively gain all fennec permissions using this hole.

Comment 13

[Wesley Johnston (:wesj)]

I believe we also force addon installs to be signed or the require the user to opt in (through a second prompt) to allowing installs from third parties. But Mossop can speak better to that.

Comment 14

[Gian-Carlo Pascutto [:gcp]]

Oh, I'm not saying this isn't a dangerous bug. It's a full bypass of Android sandboxing! But our highest security ratings are for remote-exploitable holes, and this bug does require installing malicious software on the device first, so it's more of a "stepping stone".

Talking in terms of remote vulnerability, it can turn a remote vulnerability in almost any Android app (system or user) into a Firefox vulnerability.

Comment 15

[Mark Goodwin [:mgoodwin]]

(In reply to Gian-Carlo Pascutto (:gcp) from comment #14)
> But our highest security ratings are for remote-exploitable
> holes, and this bug does require installing malicious software on the device
> first, so it's more of a "stepping stone".

I see what you're saying. Understood.

Comment 16

[Mark Goodwin [:mgoodwin]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #9)
> Discussed this on IRC with gcp. We decided to move the about:memory dumps to
> somewhere on /sdcard 

putting about:memory dumps on /sdcard will allow any application to determine the open tabs at time of crash, will it not?

Comment 17

[Gian-Carlo Pascutto [:gcp]]

>putting about:memory dumps on /sdcard will allow any application to determine the 
>open tabs at time of crash, will it not?

Yes, and intentionally so, because Android doesn't allow the file to be accessed without root permissions otherwise. So if we want anything except Firefox to read it, we must make it work-readable. Maybe that part can be activated by a pref for our AWSY testing, what do you think kats?

Note that this was already possible because of this bug, so if you want to discuss this further, check out bug 845342 or perhaps file a new one, because I don't think it should block this (more serious) bug.

Comment 18

[Mark Goodwin [:mgoodwin]]

(In reply to Gian-Carlo Pascutto (:gcp) from comment #17)
> I don't think it should block this (more serious) bug.

I agree.

Comment 19

[Kartikaya Gupta (email:[e-mail])]

(In reply to Mark Goodwin [:mgoodwin] from comment #16)
> putting about:memory dumps on /sdcard will allow any application to
> determine the open tabs at time of crash, will it not?

The dumps are not dumped at time of crash.

Comment 20

[Gian-Carlo Pascutto [:gcp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3e1d43059729

Comment 21

[Kartikaya Gupta (email:[e-mail])]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #19)
> (In reply to Mark Goodwin [:mgoodwin] from comment #16)
> > putting about:memory dumps on /sdcard will allow any application to
> > determine the open tabs at time of crash, will it not?
> 
> The dumps are not dumped at time of crash.

Although thinking about it a bit more, we do expose a broadcast intent hook to dump this data (which is what I'm using for AWSY). So it's possible that another app could broadcast the org.mozilla.gecko.MEMORY_DUMP intent and then read the dump file from the SD card. The same problem may apply to desktop FF - if you send it a SIGRTMIN signal ("kill -s SIGRTMIN <pid>" on Linux) it will dump the same memory report file to the system tmp directory (/tmp for me). Is this something we need to fix?

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3e1d43059729

Comment 23

[Gian-Carlo Pascutto [:gcp]]

Any advice on how far we should uplift this? (And bug 845342).

Comment 24

[Mark Goodwin [:mgoodwin]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #21)
> Is this something we need to fix?

Yeah, we should look at this.

Comment 25

[Daniel Veditz [:dveditz]]

although rated "sec-moderate" in practice because it would require installing a malicious app to take advantage of this, the impact given such an app is sec-high. This fix is safe enough to uplift.

Comment 27

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 718433 [details] [diff] [review]
Patch 1. Make new tempdir and remove old one.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Very old committ.
User impact if declined: Sec-moderate/Sec-high hole
Testing completed (on m-c, etc.): Landed on m-c a few days ago.
Risk to taking this patch (and alternatives if risky): Android only, only affects a tmpdir.
String or UUID changes made by this patch:

Comment 28

[bhavana bajaj [:bajaj]]

Comment on attachment 718433 [details] [diff] [review]
Patch 1. Make new tempdir and remove old one.

Approving the patch for uplift per comment# 25 and this affects android only.Please let QA know if there is any additional verification needed here other than the m-c testing.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/01c33ed5c6e0

This is going to need a beta-specific patch.

Comment 30

[Gian-Carlo Pascutto [:gcp]]

For QA: on a rooted phone, before the update, go to the profile dir and observe that there is a world-writable app_tmp (see comment 2). After the update, this directory should be gone, and there should be NON-world-writable app_tmpdir instead.

Comment 31

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch 1b. Rebased to mozilla-beta

I suppose I can carry over the a+, but gonna ask for r? and a second pair of eyes just in case.

Comment 32

[Gian-Carlo Pascutto [:gcp]]

https://hg.mozilla.org/releases/mozilla-beta/rev/fa2986e8a22c