1652360 - (CVE-2020-15650) Path traversal in upload feature allows internal file overwrite

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Pedro Oliveira]

Summary

A missing sanitisation of file names allows a malicious file picker to overwrite any file in Firefox's internal directory.

Environment

• Device: HTC m8
• OS version: Android 9
• Package name: org.mozilla.firefox
• App version: 68.10.1

---

Proof of concept

Pre-conditions:

• PoC installed (see attachments)
• Firefox installed
• Server running the attached HTML page (or directly open it)

Steps:

1. Open the webpage containing the upload logic
2. Tap Upload and choose Files
3. Choose mozillatraverse as the file picker
4. Restart Firefox
5. Check PoC public app directory /sdcard/Android/app.kans.mozillatraverse/files/

Result

profile.ini is overwritten and now points to the location of the profile to another apps directory

Expected result

There should be no private file overwrite.

---

Detailed explanation

When the PoC app is chosen as the picker, the following URI is returned to Firefox:

content://app.kans.mozillatraverse.provider

This URI is a content:// URI that belongs to the PoC app itself. Firefox will process this URI and queries the contents of the cursor which are:

_display name	_size
/../../../../../../../../../data/data/org.mozilla.firefox/files/mozilla/profiles.ini	123

Firefox will create a temporary file with the contents of content://app.kans.mozillatraverse.provider and to create this temporary file, it uses the _display_name column to create a file with the same name.

Because the contents of this column are not sanitised, the path concatenation leads to a path traversal:

String fileName = cursor.getString(0);
//...
tempDir = FileUtils.createTempDir(cacheDir, "tmp_");
File file = new File(tempDir, fileName);

This causes the contents of profile.ini to be overwritten with:

[Profile0]
Name=default
Default=1
IsRelative=0
Path=/../../../../../../../../../../sdcard/Android/data/app.kans.mozillatraverse/files/


[General]
StartWithLastProfile=1

After restarting Firefox, sensitive information is now stored and under controll of the malicious app.

---

Vulnerable code

• FilePickerResultHandler

String fileName = cursor.getString(0);

---

Remediation

Firefox should sanitise the contents of this column before using them:

String fileName = cursor.getString(0);
// sanitize
int index = fileName.lastIndexOf(separatorChar)
fileName = index < prefixLength ? path.substring(prefixLength) : path.substring(index + 1)

---

Attachments

• PoC.zip - Source code of PoC
• poc.apk - Compiled binary
• index.html - HTML file uploaded
• video.mp4 - Video showing exploit in action

---

Impact

A malicious file picker could deploy a targeted attack for Firefox (by calling getCallingActivity for example) and tamper any file inside the private directory of Firefox.

In this PoC, I've changed the path of the profile to another one. Any file can be affected by this.

Comment 1

[Pedro Oliveira]

Attachment: poc.apk

Comment 2

[Pedro Oliveira]

Attachment: index.html

Comment 3

[Pedro Oliveira]

Attachment: poc.zip

Comment 4

[Pedro Oliveira]

Video can be seen here:

https://drive.google.com/drive/folders/1T1bv__YQ91lW1JIiAZ508fKFPiYeuMNB?usp=sharing

Comment 6

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

At first glance this looks to be the same as Bug 1617928. Petru, can you look?

Comment 7

[Pedro Oliveira]

Fenix is vulnerable too as seen here:

https://github.com/mozilla-mobile/android-components/blob/master/components/browser/engine-gecko-nightly/src/main/java/mozilla/components/browser/engine/gecko/prompt/GeckoPromptDelegate.kt#L540

Comment 8

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1652360 - Sanitize file picker file names; r?snorp

Comment 9

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

Petru, thank you for the patch. For completeness, would you mind leaving a comment here what this change does and how it remediates this attack?

Comment 10

[Petru-Mugurel Lingurar [:petru]]

Try build for testing - https://treeherder.mozilla.org/#/jobs?repo=try&revision=089fb67663cfd1774847836f62b8ce3204ee57bb

Comment 11

[Petru-Mugurel Lingurar [:petru]]

(In reply to Stefan Arentz | :st3fan | ⏰ EST | he/him from comment #9)

Petru, thank you for the patch. For completeness, would you mind leaving a comment here what this change does and how it remediates this attack?

The attack vector used here is the file name returned from the rogue provider that would contain ../ for upstream traversal.
By cleaning that filename to only get the last child from the path upstream traversal would not be possible anymore.
/../../../../../../../../../data/data/org.mozilla.firefox/files/mozilla/profiles.ini becomes just profiles.ini.

Comment 12

[:Gijs (he/him)]

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

Try build for testing - https://treeherder.mozilla.org/#/jobs?repo=try&revision=089fb67663cfd1774847836f62b8ce3204ee57bb

For future reference, please sanitize commit messages for trypushes that involve security bugs (at least don't mention the bug number or "security bug" or whatever). Yes, people watch our commits, and that includes try. There's work for a push hook to catch these cases (ie it would stop you pushing) but unfortunately it got disabled because it broke legit trypushes after people land sec bug fixes on nightly...

Where are we tracking the Fenix fix per comment #7 ? We should probably make sure they get a fix at the same time.

Comment 13

[Petru-Mugurel Lingurar [:petru]]

Added Diana and Sorina for helping us with a quick test in the case a decision to land this is made.

Comment 14

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

Thank you all for moving so quickly on this.

Comment 15

[Sorina Florean [:sflorean]]

Tested on the try build provided here, and I can confirm the fix with LG G7 FIT (Android 8) following the steps provided and the additional pieces of information.

Comment 16

[Arturo Mejia [:amejia]]

(In reply to :Gijs (he/him) from comment #12)

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

Try build for testing - https://treeherder.mozilla.org/#/jobs?repo=try&revision=089fb67663cfd1774847836f62b8ce3204ee57bb

For future reference, please sanitize commit messages for trypushes that involve security bugs (at least don't mention the bug number or "security bug" or whatever). Yes, people watch our commits, and that includes try. There's work for a push hook to catch these cases (ie it would stop you pushing) but unfortunately it got disabled because it broke legit trypushes after people land sec bug fixes on nightly...

Where are we tracking the Fenix fix per comment #7 ? We should probably make sure they get a fix at the same time.

I'm working on the Fenix fix, I will that land it at the same time as the equivalent of the bug 1652364, we are tracking it here

Comment 17

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 9163237 [details]
Bug 1652360 - Sanitize file picker file names; r?snorp

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Important security issue
• User impact if declined: In a certain scenario a malicious app could overwrite Fennec's internal storage.
• Fix Landed on Version:
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Small change, verified by QA on a try build.
• String or UUID changes made by this patch:

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9163237 [details]
Bug 1652360 - Sanitize file picker file names; r?snorp

Approved for Fennec 68.11.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

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

Comment 20

[Arturo Mejia [:amejia]]

The AC fix has been a approved and on process to be merged it can be tracked in this pull request, Fenix will need to update to a new version of AC that contains the patch.

Comment 21

[Sorina Florean [:sflorean]]

Verified as fixed on Fennec RC 68.11.0 with LG G7 FIT (Android 8). For the Fenix issue, we will leave a comment on bug 1652364.

Comment 22

[Pedro Oliveira]

Hey team,

This issue and 1652364 were already fixed in a stable channel (68.11.0, already available in Play Store). Will they be part of any security advisory?

Thanks,
Pedro

Comment 23

[Frederik Braun [:freddy]]

This issue and 1652364 were already fixed in a stable channel (68.11.0, already available in Play Store). Will they be part of any security advisory?

Thank you for pointing out. That was my mistake. I'll fix the advisories soon.

Comment 24

[Frederik Braun [:freddy]]

Attachment: advisory.txt

Comment 25

[Frederik Braun [:freddy]]

Should be up within a couple of hours. Let me know if you don't see them by the end of the day (CEST).