1617928 - (CVE-2020-6828) Firefox for Android - Directory Traversal can lead to network hijacking

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[gnehsoah]

Attachment: poc.zip

It has been found the firefox android application accepts Intents from third-parties. When a crafted Intent containing a URI pointing to a custom-defined ContentProvider is sent, the application queries the ContentProvider to fetch files. This allows overwriting files under the private application folder.
By exploiting this vulnerability, it is possible to overwrite /data/data/org.mozilla.firefox/files/mozilla/profiles.ini and put a user.js file into the user's directory which can lead to network hijacking.

Vulnerability in this method:
org.mozilla.gecko.util.ContentUriUtils.getTempFilePathFromContentUri

Firefox Version: 68.5.0

Steps to reproduce:

1. see the screen record

Comment 1

[gnehsoah]

Here is the screen record https://drive.google.com/open?id=1q6IQP8SCcpqtUTZ-Wb6z73Y8vWcFdLzM

Comment 2

[Johann Hofmann [:johannh]]

Stefan, Snorp, can you please take a look at this? I can't really say how viable an attack through crafted intents is, but overwriting arbitrary prefs/files in the app directory is almost as bad as it can get.

Thanks!

Comment 3

[James Willcox (:snorp) ([e-mail]) (he/him)]

Ugh. Yup, this is pretty bad.

It looks like in order to fix, we should just use the leaf file name here: https://searchfox.org/mozilla-central/rev/b2ccce862ef38d0d150fcac2b597f7f20091a0c7/mobile/android/geckoview/src/main/java/org/mozilla/gecko/util/FileUtils.java#321

If we don't do this, the filename can look like "../../files/mozilla/profiles.ini", which causes us to overwrite our own profiles.ini, etc. We should treat it as "profiles.ini" instead.

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

Petru, do you have time to write a patch for this? See comment #3.

Comment 5

[Frederik Braun [:freddy]]

This affects Fennec and Fenix, doesn't it?

We ought to patch both.

Comment 6

[Frederik Braun [:freddy]]

Sebastian tells me this only affects Fennec.
From what I understand, we want to roll out fenix gradually to existing beta users, which means we can't easily ship a security update without delaying that. But updating to would fix this problem for beta population over the course of the next ~2 weeks.
This would still leave fennec release users affected until we start the roll out in early April (~5 weeks from now).

Maybe we could do the fenix roll-out for beta and fix fennec-release directly after beta has gone to 100% (i.e., after those two weeks)?
I'd still very much like to hear what st3fan and Dan think.

Comment 7

[Petru-Mugurel Lingurar [:petru]]

(In reply to James Willcox (:snorp) ([e-mail]) (he/him) from comment #4)

Petru, do you have time to write a patch for this? See comment #3.

Sure!
This is interesting...

Comment 8

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Marking this a P1 - lets try to ship this in the next release.

Comment 9

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1617928 - Sanitize "content" uri filenames; r?AndreiLazar,snorp

For loading a file exposed through a provider Fennec will first copy the file
in it's cache folder (internal storage).
Tricking Fennec into thinking the file name should contain forward slashes will
result in saving the file to a different than intended location potentially
overwriting important application data.

To mitigate this attack vector we'll always check for forward slashes in the
filename and if so always keep just the leaf.

Comment 10

[Petru-Mugurel Lingurar [:petru]]

James, I see FileUtils still exists in m-c.
If it needs this change also I can file a bug and post a similar patch for it.

Comment 11

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Petru-Mugurel Lingurar[:petru] from comment #10)

James, I see FileUtils still exists in m-c.
If it needs this change also I can file a bug and post a similar patch for it.

That would be great. I could see us trying to use it in the future to handle content://.

Comment 12

[Daniel Veditz [:dveditz]]

Sebastian tells me this only affects Fennec.

The vuln combination, maybe, because we block it at the intents end. We should fix FileUtils for both just in case there's another or future way to get there in other GeckoView products (what snorp said in comment 11).

Does shipping Focus respond to or ignore those intents?

Comment 13

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Daniel Veditz [:dveditz] from comment #12)

Sebastian tells me this only affects Fennec.

The vuln combination, maybe, because we block it at the intents end. We should fix FileUtils for both just in case there's another or future way to get there in other GeckoView products (what snorp said in comment 11).

Does shipping Focus respond to or ignore those intents?

It does not support content:// at the moment.

Comment 14

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 9130800 [details]
Bug 1617928 - Sanitize "content" uri filenames; r?AndreiLazar,snorp

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high - privacy escalation
• User impact if declined: Network hijacking, settings overridden.
• Fix Landed on Version:
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Small change
• String or UUID changes made by this patch:

Comment 15

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 9130800 [details]
Bug 1617928 - Sanitize "content" uri filenames; r?AndreiLazar,snorp

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Medium effort
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
• Which older supported branches are affected by this flaw?: All branches. Bug was probably introduced in bug 1499618
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: The solution is simple.
• How likely is this patch to cause regressions; how much testing does it need?: Very low chances of regressions. Testing based on the provided poc should be enough.

Comment 16

[Tom Ritter [:tjr]]

Comment on attachment 9130800 [details]
Bug 1617928 - Sanitize "content" uri filenames; r?AndreiLazar,snorp

Per discussion this is going to land under a public bug, with the comments changes so it looks less like a security fix.

Comment 17

[Petru-Mugurel Lingurar [:petru]]

As a public ticket I created bug 1622781.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/c94534b3902ac8d78f186ac239eca3502f843f8a

Comment 19

[Tom Ritter [:tjr]]

Attachment: advisory.txt