Last Comment Bug 782581 - (CVE-2013-1727) Subverting Same-Origin Policy for Local Contents by Symbolic Link
(CVE-2013-1727)
: Subverting Same-Origin Policy for Local Contents by Symbolic Link
Status: RESOLVED FIXED
[adv-main24+]
: sec-moderate
Product: Core
Classification: Components
Component: General (show other bugs)
: Trunk
: ARM Android
-- normal (vote)
: mozilla24
Assigned To: Jim Chen [:jchen] [:darchons]
:
:
Mentors:
: 891876 964055 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-14 02:53 PDT by Takeshi Terada
Modified: 2014-11-19 20:11 PST (History)
17 users (show)
dveditz: sec‑bounty+
ryanvm: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
wontfix
wontfix
-
affected
-
affected
fixed
wontfix
-
wontfix
-


Attachments
vulnerability report (4.69 KB, text/plain)
2012-08-14 02:53 PDT, Takeshi Terada
no flags Details
eclipse project's archive of a sample malicious app (692.74 KB, application/octet-stream)
2012-09-15 07:19 PDT, Takeshi Terada
no flags Details
Patch v1 (1018 bytes, patch)
2012-09-26 11:10 PDT, Wesley Johnston (:wesj)
dveditz: review-
Details | Diff | Splinter Review
patch (775 bytes, patch)
2013-02-01 11:49 PST, Brad Lassey [:blassey] (use needinfo?)
dveditz: review+
abillings: sec‑approval+
Details | Diff | Splinter Review

Description User image Takeshi Terada 2012-08-14 02:53:11 PDT
Created attachment 651674 [details]
vulnerability report

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:14.0) Gecko/20100101 Firefox/14.0.1
Build ID: 20120713134347

Steps to reproduce:

I created an example of malicious Android app
that steals Firefox's private files such as a file storing Cookie.
(Please see the attached file.)


Actual results:

The malicious app gained access to Firefox's private files.
(Please see the attached file.)


Expected results:

The access should have been prohibited.
(Please see the attached file.)
Comment 1 User image Mark Finkle (:mfinkle) (use needinfo?) 2012-09-13 10:43:15 PDT
Wes - We need to figure out what we need to block to fix this attack. Looks like we need to stop local file access onNewIntent or something.
Comment 2 User image Wesley Johnston (:wesj) 2012-09-13 11:02:50 PDT
Brian, any idea how/if desktop prevents this? Or is the feeling on desktop that, once an app is installed your profile is up for grabs? That's not assumed on Android.

Correct behaviour seems like it would be to look at the target of symbolic links for file uri's and use that to determine whether the file can be loaded.
Comment 3 User image Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-09-13 11:08:07 PDT
What is the use case for Firefox on Android supporting file:// URIs at all?

(In reply to Wesley Johnston (:wesj) from comment #2)
> Brian, any idea how/if desktop prevents this? Or is the feeling on desktop
> that, once an app is installed your profile is up for grabs? That's not
> assumed on Android.
> 
> Correct behaviour seems like it would be to look at the target of symbolic
> links for file uri's and use that to determine whether the file can be
> loaded.

Better to ask Dan. Perhaps desktop has had the same issues in the past (or present!)?
Comment 4 User image Wesley Johnston (:wesj) 2012-09-13 11:24:43 PDT
I use file uri's frequently for testing/development. I have a feeling that some of our test infrastructure might too? I'd like to keep support if possible. Being able to send them through intents makes automated testing easier as well. One script can push the right files to the device and load them in Fennec. But there are other workarounds for that...
Comment 5 User image Mark Finkle (:mfinkle) (use needinfo?) 2012-09-13 13:29:25 PDT
Wes - I think we could do a symlink check in BrowserCLH.js

Desktop Firefox does a check for a nsIFileURL and then tests to see if it exists:
http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserContentHandler.js#50

We could do the same, but also check for a symlink, here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/BrowserCLH.js#43

using uri.file.isSymlink()
Comment 6 User image Wesley Johnston (:wesj) 2012-09-13 13:55:16 PDT
I think the XHR is the bigger problem here (although I'm kinda surprised Android allows symlinking against something you don't have permission to access at all....)
Comment 7 User image Wesley Johnston (:wesj) 2012-09-13 13:56:50 PDT
How do we feel about forbidding file uri's that point to symlinked files at all? That seems.... likely to break stuff for desktop users.
Comment 8 User image Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-09-13 15:45:33 PDT
(In reply to Mark Finkle (:mfinkle) from comment #5)
> Wes - I think we could do a symlink check in BrowserCLH.js
> 
> Desktop Firefox does a check for a nsIFileURL and then tests to see if it
> exists:

Let's please put this kind of security check in a centralized place (platform), and not put into product-specific code, if it is at all reasonable to do so. 

I thought that the same-origin policy file file:// was "same directory (or a subdirectory) as the base HTML page?" If so, then that code should be handling symlinks in a reasonable way. If not, then this seems like a general problem, not Fennec-specific.

I understand the benefits of file:// support for testing/development. But, actual web content should not be using file:// at all, right? If so, I don't know if it is worth trying to support file://, especially since B2G doesn't (right?).
Comment 9 User image Wesley Johnston (:wesj) 2012-09-14 16:44:35 PDT
I wrote up a patch for this, but I can't actually reproduce this bug on any of the devices I own or even on Desktop.

On my Android devices, 1.) Opening the files from the malicious apps profile fails and 2.) Even if I move the file to the sdcard creating the symlink to something in data/data/org.mozilla.firefox also fails like I would expect it to.

On Desktop the symlinks are resolved before they reach nsPrincipal::CheckMayLoad (not sure where exactly), so trying to do this there also fails.

What sort of device/Android version are you testing with? Is it a special Android build? Rooted?
Comment 10 User image Takeshi Terada 2012-09-15 07:19:28 PDT
Created attachment 661493 [details]
eclipse project's archive of a sample malicious app
Comment 11 User image Takeshi Terada 2012-09-15 07:22:32 PDT
(In reply to Wesley Johnston (:wesj) from comment #9)
> I wrote up a patch for this, but I can't actually reproduce this bug on any
> of the devices I own or even on Desktop.

I attached an eclipse project's archive of the malicious app I used for test.
--> AttackFirefox1.tar.gz

Could you please try it on your device?

> What sort of device/Android version are you testing with? Is it a special
> Android build? Rooted?

I tested on Android 4.0.4 Galaxy Nexus (not a special build).
And yes, the device is rooted, but I guess rooting does not affect the issue.
Comment 12 User image Wesley Johnston (:wesj) 2012-09-25 17:16:07 PDT
Ahh. Great. Managed to reproduce this now. Thanks
Comment 13 User image Wesley Johnston (:wesj) 2012-09-26 11:10:17 PDT
Created attachment 665061 [details] [diff] [review]
Patch v1

Ahh, so this bug actually depends on the fact that you're overwriting the original file with the symlink. We call codebaseFile->Normalize(), which dereferences the symbolic link on the loaded file, and we essentially are comparing profiles.ini against profiles.ini.

http://mxr.mozilla.org/mozilla-central/source/caps/src/nsPrincipal.cpp#898

I... want to remove the Normalize call here. I think we really want to compare the undereferenced original file against the dereferenced target. If the original file is a symlink to something else.... I don't think we care?

Not really sure who should review this. Picked jst because he's reviewed other code in here. Is there someone better to ask about this?
Comment 14 User image Johnny Stenback (:jst, jst@mozilla.com) 2012-09-26 14:10:02 PDT
Comment on attachment 665061 [details] [diff] [review]
Patch v1

So the reason for this code doing what it's doing today is primarily to protect the user from saving a page off the web to the users disk and then being exploited when opening the file locally. Firefox will however never create symlinks while saving a webpage, so in that sense this change should have no effect, but it will have an effect on other cases where the user is viewing a local HTML file that ended up on the users disk some other way. I don't know how much we realistically need to protect against that case, and if the answer is not at all then the change here seems fine.

But I'm ultimately not the guy to make a call on our policy there, so I'm punting this to dveditz, who's done most of the work on the file:// security model. Given how different the expectations are wrt file:// uri access on Android vs. desktop, I'd even be ok with a #ifdef ANDROID or whatever in this code, if we need to go there.
Comment 15 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-09-26 17:54:10 PDT
Since I was cc'ed in regards to the same-origin policy for file://

The policy is complicated. Let me start out by saying that.

Our same-origin code is generally symmetric. I.e. principal A can only be same-origin with principal B if principal B is same-origin with principal A.

This means that a file can't be same-origin with files only in subdirectories. Since then subdirectories would have to be same-origin with files in parent directories.

However when opening an <iframe>, we do this trick where if a window from file:// opens an <iframe> is pointing to a subdirectory, we give the <iframe> the principal of the parent window. That way those two pages will test same-origin, since they are literally using the same principal.

Additionally we have a "CheckMayLoad" test, where we can test if principal A has the right to load URI C. This isn't a same-origin test, but literally a "can this guy load that URI" test. Principals from file:// return "yes" for that check if the URI to be loaded is a subdirectory.
Comment 16 User image Daniel Veditz [:dveditz] 2012-10-20 00:04:03 PDT
Comment on attachment 665061 [details] [diff] [review]
Patch v1

What we really wanted out of Normalize() was path canonicalization to get rid of /../ etc., not to dereference symbolic links. This will, however, break uses of local files for things like help systems if they're in a directory that's a symbolic link. I'm guessing that's not all that common, but if it happened it might affect some large installations.

Then again the only time this comes up is when used from a document for which a principal is created, and from what I've seen the file is normalized at that time. Can we rely on codebaseFile to be already normalized?

Unfortunately this is the kind of thing where if it goes wrong we only find out about it in a broad release, which makes me jumpy about such a seemingly innocent patch. We either need a bunch of tests for this or we should restrict this change to android only (as ugly as #ifndef would be here).
Comment 17 User image Mark Finkle (:mfinkle) (use needinfo?) 2012-10-25 10:53:26 PDT
Based on comment 8, we would like a platform fix for this issue. Kicking to Brian.
Comment 18 User image Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-10-25 14:37:52 PDT
Sorry, I will not be able to work on this for a while.
Comment 19 User image Alex Keybl [:akeybl] 2012-11-14 16:09:42 PST
Josh - can you help find an assignee for this bug?

I don't think this needs to be tracked for a specific release, considering it requires the user to install malicious software. sec team - please renominate if you disagree.
Comment 20 User image Josh Aas 2012-11-19 08:09:33 PST
Is this really sec-high if it requires malicious software to be installed?
Comment 21 User image Josh Aas 2013-01-28 07:12:12 PST
Our team doesn't normally deal with issues like this, and a fix really depends on what the right behavior on Android is. Over to Brad to find an owner on the Android team.
Comment 22 User image Brad Lassey [:blassey] (use needinfo?) 2013-02-01 11:49:44 PST
Created attachment 709172 [details] [diff] [review]
patch
Comment 23 User image Brad Lassey [:blassey] (use needinfo?) 2013-02-01 11:50:50 PST
Why is this marked as sec moderate? The user's system has to already be compromised for this to be exploitable.
Comment 24 User image David Bolter [:davidb] 2013-02-01 11:54:25 PST
Please feel free to lower the rating as far as I'm concerned.
Comment 25 User image Brad Lassey [:blassey] (use needinfo?) 2013-02-13 07:24:32 PST
(In reply to David Bolter [:davidb] from comment #24)
> Please feel free to lower the rating as far as I'm concerned.

ok, moving to sec-low
Comment 26 User image Daniel Veditz [:dveditz] 2013-05-07 13:51:57 PDT
Comment on attachment 709172 [details] [diff] [review]
patch

r+=dveditz
Comment 27 User image Brad Lassey [:blassey] (use needinfo?) 2013-05-10 12:36:30 PDT
what is the current policy for landing patches for security bugs?
Comment 28 User image Boris Zbarsky [:bz] (still a bit busy) 2013-05-10 12:46:32 PDT
You ask for sec-approval and see what the answer is.
Comment 29 User image Brad Lassey [:blassey] (use needinfo?) 2013-05-10 13:07:24 PDT
Comment on attachment 709172 [details] [diff] [review]
patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
I don't believe so.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No

Which older supported branches are affected by this flaw?
I believe all

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?
Should be the same patch across all branches

How likely is this patch to cause regressions; how much testing does it need?
unlikely.
Comment 30 User image Al Billings [:abillings] 2013-05-10 13:57:54 PDT
Comment on attachment 709172 [details] [diff] [review]
patch

sec-approval+ for m-c.
Comment 31 User image Ryan VanderMeulen [:RyanVM] 2013-05-13 13:56:47 PDT
https://hg.mozilla.org/mozilla-central/rev/f3b7d0043d91
Comment 32 User image Alex Keybl [:akeybl] 2013-05-20 13:20:35 PDT
We don't typically track/fix sec-low on branches
Comment 33 User image Daniel Veditz [:dveditz] 2013-08-29 14:58:29 PDT
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #23)
> Why is this marked as sec moderate? The user's system has to already be
> compromised for this to be exploitable.

Don't we put sensitive profile files in Firefox-owned directories specifically to protect it against malicious apps? This isn't a case of the Android security model breaking down, this is Firefox helping those malicious apps bypass the Android security model.

The ability to steal the user's cookies or passwords from a web page would be sec-critical; the fact that the attack requires installing a malicious app is what lowers it to "moderate".
Comment 36 User image Jim Chen [:jchen] [:darchons] 2013-09-18 06:16:40 PDT
*** Bug 891876 has been marked as a duplicate of this bug. ***
Comment 37 User image Kevin Brosnan [:kbrosnan] 2014-01-27 16:36:02 PST
*** Bug 964055 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.