1.78 KB, patch
|Details | Diff | Splinter Review|
1.91 KB, patch
|Details | Diff | Splinter Review|
Date: Mon, 25 Feb 2013 10:47:20 +0900 From: Shuichiro Suzuki <firstname.lastname@example.org> To: email@example.com 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 firstname.lastname@example.org
5 years ago
CC'ed some mobile developers to find out what /app_tmp/ is used for.
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).
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
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?
Created attachment 718393 [details] [diff] [review] 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?
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.
(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.
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?
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.
Created attachment 718433 [details] [diff] [review] Patch 1. Make new tempdir and remove old one.
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.
(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.
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.
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.
(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.
(In reply to Kartikaya Gupta (email:email@example.com) 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?
>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.
(In reply to Gian-Carlo Pascutto (:gcp) from comment #17) > I don't think it should block this (more serious) bug. I agree.
(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.
(In reply to Kartikaya Gupta (email:firstname.lastname@example.org) 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?
Any advice on how far we should uplift this? (And bug 845342).
(In reply to Kartikaya Gupta (email:email@example.com) from comment #21) > Is this something we need to fix? Yeah, we should look at this.
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 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 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.
https://hg.mozilla.org/releases/mozilla-aurora/rev/01c33ed5c6e0 This is going to need a beta-specific patch.
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.
Created attachment 723633 [details] [diff] [review] 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.