782581 - (CVE-2013-1727) Subverting Same-Origin Policy for Local Contents by Symbolic Link

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Takeshi Terada]

Attachment: 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

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

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

[Wesley Johnston (:wesj)]

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

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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

[Wesley Johnston (:wesj)]

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

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

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

[Wesley Johnston (:wesj)]

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

[Wesley Johnston (:wesj)]

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

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(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

[Wesley Johnston (:wesj)]

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

[Takeshi Terada]

Attachment: eclipse project's archive of a sample malicious app

Comment 11

[Takeshi Terada]

(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

[Wesley Johnston (:wesj)]

Ahh. Great. Managed to reproduce this now. Thanks

Comment 13

[Wesley Johnston (:wesj)]

Attachment: 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

[Johnny Stenback (:jst)]

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

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

[Daniel Veditz [:dveditz]]

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

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

Based on comment 8, we would like a platform fix for this issue. Kicking to Brian.

Comment 18

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Sorry, I will not be able to work on this for a while.

Comment 19

[Alex Keybl [:akeybl]]

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

[Josh Aas]

Is this really sec-high if it requires malicious software to be installed?

Comment 21

[Josh Aas]

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

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

Comment 23

[Brad Lassey [:blassey] (use needinfo?)]

Why is this marked as sec moderate? The user's system has to already be compromised for this to be exploitable.

Comment 24

[David Bolter [:davidb] (NeedInfo me for attention)]

Please feel free to lower the rating as far as I'm concerned.

Comment 25

[Brad Lassey [:blassey] (use needinfo?)]

(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

[Daniel Veditz [:dveditz]]

Comment on attachment 709172 [details] [diff] [review]
patch

r+=dveditz

Comment 27

[Brad Lassey [:blassey] (use needinfo?)]

what is the current policy for landing patches for security bugs?

Comment 28

[Boris Zbarsky [:bzbarsky]]

You ask for sec-approval and see what the answer is.

Comment 29

[Brad Lassey [:blassey] (use needinfo?)]

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

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 709172 [details] [diff] [review]
patch

sec-approval+ for m-c.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f3b7d0043d91

Comment 32

[Alex Keybl [:akeybl]]

We don't typically track/fix sec-low on branches

Comment 33

[Daniel Veditz [:dveditz]]

(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".