Closed Bug 1652360 (CVE-2020-15650) Opened 1 year ago Closed 1 year ago

Path traversal in upload feature allows internal file overwrite

Categories

(Firefox for Android Graveyard :: General, defect)

defect

Tracking

(firefox-esr6879+ verified)

VERIFIED FIXED
Tracking Status
firefox-esr68 79+ verified

People

(Reporter: kanytu, Assigned: petru)

References

Details

(Keywords: sec-moderate, Whiteboard: [reporter-external] [client-bounty-form][adv-esr68.11+])

Attachments

(5 files)

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

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.

Flags: sec-bounty?
Attached file poc.apk
Attached file index.html
Attached file poc.zip
URL: --
Group: firefox-core-security → mobile-core-security
Type: task → defect
Component: Security → General
Flags: needinfo?(sarentz)
Product: Firefox → Firefox for Android
Duplicate of this bug: 1652359
Flags: needinfo?(sarentz)

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

Flags: needinfo?(petru.lingurar)
Assignee: nobody → petru.lingurar

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?

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

Flags: needinfo?(petru.lingurar)

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

Flags: needinfo?(sarentz)

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

Flags: needinfo?(sarentz)

Thank you all for moving so quickly on this.

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.

(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 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:
Attachment #9163237 - Flags: approval-mozilla-esr68?

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

Approved for Fennec 68.11.

Attachment #9163237 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+
Group: mobile-core-security → core-security-release
Status: UNCONFIRMED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED

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.

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.

Status: RESOLVED → VERIFIED

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

Flags: sec-bounty? → sec-bounty+

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.

Flags: needinfo?(fbraun)
Flags: needinfo?(fbraun)
Whiteboard: [reporter-external] [client-bounty-form] [verif?] → [reporter-external] [client-bounty-form][adv-esr68.11+]

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

Group: core-security-release
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.