Bug 944374 (CVE-2014-1506)

Security vulnerability: CrashReporter Path Traversal

RESOLVED FIXED in Firefox 28

Status

()

defect
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: roeeh, Assigned: bnicholson)

Tracking

({csectype-disclosure, sec-moderate})

25 Branch
Firefox 30
x86_64
Android
Points:
---
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(firefox28 fixed, firefox29 fixed, firefox30 fixed, firefox-esr24 unaffected, b2g-v1.3 fixed)

Details

(Whiteboard: [reporter-external][adv-main28+])

Attachments

(2 attachments, 2 obsolete attachments)

Reporter

Description

6 years ago
Posted file firefoxleakage.pdf
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/31.0.1650.57 Safari/537.36

Steps to reproduce:

Technical details can be found in the attached whitepaper.


Actual results:

Technical details can be found in the attached whitepaper.


Expected results:

Technical details can be found in the attached whitepaper.
Reporter

Comment 1

6 years ago
The referenced weak randomness vulnerability in the whitepaper is Bug 944373.
OS: Windows 7 → Android
Flags: sec-bounty?
Whiteboard: [reporter-external]
So the gist here is that other apps installed on the device can invoke the CrashReporter intent with arbitrary file paths and get us to do stupid things. We should probably make a few changes here:
1) Reject minidump file paths that aren't in our application data directory. This should neuter most attacks here.
2) Possibly just hardcode the server URL into the CrashReporter app instead of reading it from the .extra file, so that even if a bad file gets in it can't force us to submit data to a third party.

These are more interesting on Android than desktop since on desktop anyone who could invoke the crashreporter app with an arbitrary path already has the ability to execute arbitrary code on your machine. On Android apps are expected to be sandboxed from each other so the threat is data leakage from our app.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(dveditz)
Ted: would this be yours to fix, or can you nominate someone on the Fennec team?
Assignee: nobody → ted
Flags: needinfo?(dveditz) → needinfo?(ted)
I'm not real familiar with the Android crashreporter client, bnicholson has done work on it in the past. If he can't take this on blassey should be able to find someone.
Flags: needinfo?(ted)
Reassigning in an attempt to get some movement.
Assignee: ted → bnicholson
Assignee

Comment 7

5 years ago
trivial updates
Attachment #8373801 - Attachment is obsolete: true
Attachment #8373801 - Flags: review?(blassey.bugs)
Attachment #8373809 - Flags: review?(blassey.bugs)
Reporter

Comment 8

5 years ago
I am afraid the suggested fix is not robust enough as the attacker can either
(1) Conduct a client-side DoS attack on Firefox by moving arbitrary files under the app dir to the pending path (See Section IV.B)
(2) Leak arbitrary files under the app dir by indirectly injecting the "ServerURL" directive into the file. (See Section IV.A.2.2).

Is there any reason why the CrashReporter activity is exported in the Android Manifest file?
If not, a simple fix would be to make it non-exported (and also make sure that there is no more complex data-flow which can taint the minidumpPath extra).

WDYT?
Comment on attachment 8373809 [details] [diff] [review]
Abort if crash file is not descendant of application files dir

Review of attachment 8373809 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/CrashReporter.java
@@ +119,5 @@
>  
>          String passedMinidumpPath = getIntent().getStringExtra(PASSED_MINI_DUMP_KEY);
>          File passedMinidumpFile = new File(passedMinidumpPath);
> +
> +        if (!isDescendantOf(passedMinidumpFile, getFilesDir())) {

The one thing I want to confirm before this lands is crash reports for non-default profiles (like our testing profiles on sd card) pass this test

@@ +471,5 @@
>      private String unescape(String string) {
>          return string.replaceAll("\\\\\\\\", "\\").replaceAll("\\\\n", "\n").replaceAll("\\\\t", "\t");
>      }
> +
> +    private boolean isDescendantOf(File file, File dir) {

seems like you could also do file.getCannonicalPath().startsWith(dir.getCannonicalPath())
Attachment #8373809 - Flags: review?(blassey.bugs) → review+
Assignee

Comment 10

5 years ago
Here's an alternate fix that doesn't export CrashReporter as suggested in comment 8. This should prevent attackers from passing in other files owned by us (e.g., files in the cache dir). Things I've tested with this patch:

* Triggering the crash reporter on 4.2+ device (resulting in androidUserSerial code path here: http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/nsExceptionHandler.cpp#738) still works
* Triggering crash reporter on < 4.2 device (non-androidUserSerial code path above) still works
* Launching the intent from another activity, which fails as expected with a "not exported" SecurityException

Anything else worth testing?
Attachment #8374403 - Flags: review?(blassey.bugs)
Attachment #8374403 - Flags: review?(blassey.bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/dfde6f7df7f5
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
Do we want/need to backport this at all?
Flags: sec-bounty? → sec-bounty+
Awarding a bounty for this sec-moderate bug because malicious Android apps abound. It may not be as widespread as a web-page drive-by attack but you could convince lots of people to install and try any half-interesting-looking game type thing.
Assignee

Updated

5 years ago
Attachment #8373809 - Attachment is obsolete: true
Assignee

Comment 16

5 years ago
Comment on attachment 8374403 [details] [diff] [review]
Alt fix: Don't export CrashReporter activity

[Approval Request Comment]
Bug caused by (feature/regressing bug #): This has always been a vulnerability of CrashReporter.
User impact if declined: Malicious apps could manipulate the crash reporter
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): Low risk; prevents activities outside of Fennec from invoking the crash reporter.
String or IDL/UUID changes made by this patch: None
Attachment #8374403 - Flags: approval-mozilla-beta?
Attachment #8374403 - Flags: approval-mozilla-aurora?
Brian, what about ESR 24? if this vulnerability was always present, I think it should be backported too.
Thanks
Flags: needinfo?(bnicholson)
Attachment #8374403 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
We don't ship an ESR version of Firefox for Android, AFAIK.
Right. Sorry, I didn't notice that it is an Android only issue.
Attachment #8374403 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Whiteboard: [reporter-external] → [reporter-external][adv-main28+]
Alias: CVE-2014-1506
Group: core-security
You need to log in before you can comment on or make changes to this bug.