The default bug view has changed. See this FAQ.
Bug 899702 (CVE-2013-1731)

Android looks for egltrace.so in public directory

RESOLVED FIXED in Firefox 24

Status

()

Core
Graphics
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: vlad, Assigned: gw280)

Tracking

({sec-high})

Trunk
mozilla26
ARM
Android
sec-high
Points:
---

Firefox Tracking Flags

(firefox23 wontfix, firefox24+ fixed, firefox25+ fixed, firefox26+ fixed, firefox-esr17 unaffected, b2g18 unaffected)

Details

(Whiteboard: [adv-main24+])

Attachments

(1 attachment, 1 obsolete attachment)

On Android, we optionally load "egltrace.so" to load GL tracing functionality.  However, we do that unconditionally, and do it from a location that may be world-writable ("/data/local").

This opens up the possibility of a malicious third-party app dropping an egltrace.so in /data/local, which would then allow it full access to the Firefox app's data.  Very little permissions (and nothing unusual) are needed to allow an app to write to /data/local.
Created attachment 783282 [details] [diff] [review]
0001-Bug-899702-Only-attempt-load-of-egltrace.so-when-a-p.patch
Attachment #783282 - Flags: review?(vladimir)
(Reporter)

Updated

4 years ago
Attachment #783282 - Flags: review?(vladimir) → review+
(Reporter)

Comment 2

4 years ago
Comment on attachment 783282 [details] [diff] [review]
0001-Bug-899702-Only-attempt-load-of-egltrace.so-when-a-p.patch

>--- a/gfx/gl/GLLibraryEGL.cpp
>+++ b/gfx/gl/GLLibraryEGL.cpp
>@@ -32,6 +32,15 @@ static const char *sExtensionNames[] = {
> 
> static PRLibrary* LoadApitraceLibrary()
> {
>+    static sUseApitraceInitialized = false;
>+    static sUseApitrace = false;
>+    if (!sUseApitraceInitialized) {
>+        sUseApitrace = Preferences::GetBool("gfx.apitrace.enabled", false);'
>+    }

You're missing a sUseApitraceInitialized = true;
Comment on attachment 783282 [details] [diff] [review]
0001-Bug-899702-Only-attempt-load-of-egltrace.so-when-a-p.patch

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

Drive by review...

::: gfx/gl/GLLibraryEGL.cpp
@@ +35,5 @@
> +    static sUseApitraceInitialized = false;
> +    static sUseApitrace = false;
> +    if (!sUseApitraceInitialized) {
> +        sUseApitrace = Preferences::GetBool("gfx.apitrace.enabled", false);
> +    }

Nit - static bool sUse..., rather than skipping the type?
Vlad already mentioned not setting sUseApitraceInitialized inside the if

@@ +38,5 @@
> +        sUseApitrace = Preferences::GetBool("gfx.apitrace.enabled", false);
> +    }
> +
> +    if (!sUseApitrace)
> +        return nullptr;

{ }?
Sounds high or critical to me.  I'm not sure how much requiring local privilege mitigates this, ratings-wise.
Keywords: sec-critical
How would you like us to go about landing this, and do we want it on aurora/beta/release?
George, set sec-approval ?, once approved, land on central.  Then we can decide if we want to uplift it, by asking for the relevant approval-... flag.
Created attachment 784432 [details] [diff] [review]
0001-Bug-899702-Only-attempt-load-of-egltrace.so-when-a-p.patch

Updated version of the patch with a few cosmetic changes (basically addressing Milan's review comments). Carrying forward r+ from Vlad.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Easily.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

It's not explicitly referred to, and the motivation for the change isn't listed as a security issue, but it's fairly easy to see where the security flaw lies.

Which older supported branches are affected by this flaw?

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?

This patch should apply to all relevant branches.

How likely is this patch to cause regressions; how much testing does it need?

Unlikely. No testing required.
Attachment #783282 - Attachment is obsolete: true
Attachment #784432 - Flags: sec-approval?
Attachment #784432 - Flags: review+
Ugh, I accidentally left a few hunks from all.js in the updated patch. I will not land those, disregard.
Comment on attachment 784432 [details] [diff] [review]
0001-Bug-899702-Only-attempt-load-of-egltrace.so-when-a-p.patch

Sec-approval+ for checkin on 8/21 or later. We want to take this later in the cycle to reduce exposure.
Attachment #784432 - Flags: sec-approval? → sec-approval+
Whiteboard: [checkin after 8/20]
status-firefox23: --- → wontfix
status-firefox24: --- → affected
status-firefox25: --- → affected
status-firefox26: --- → affected
tracking-firefox24: --- → ?
tracking-firefox25: --- → ?
tracking-firefox26: --- → +

Updated

4 years ago
tracking-firefox24: ? → +
tracking-firefox25: ? → +
Vlad, can you check this in so it doesn't have to wait until George is back?
Flags: needinfo?(vladimir)
George is back now.
Flags: needinfo?(vladimir) → needinfo?(gwright)
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ed0a7162516
Flags: needinfo?(gwright)
Whiteboard: [checkin after 8/20] → [adv-main24+]
Alias: CVE-2013-1731
https://hg.mozilla.org/mozilla-central/rev/5ed0a7162516
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox26: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Lowering this to sec-high because "critical" generally means a purely remote attack; for this one you'd have to first convince the user to install a malicious app. But still super bad because we know the app store is infested with malicious apps and it's not implausible that you could convince a user to install packages from other locations.
Keywords: sec-critical → sec-high
Needinfo on George to help with approval request on branches here.
Flags: needinfo?(gwright)
Comment on attachment 784432 [details] [diff] [review]
0001-Bug-899702-Only-attempt-load-of-egltrace.so-when-a-p.patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: malicious app could hijack firefox and do anything, so long as the user explicitly installs said malicious app
Fix Landed on Version: 26
Risk to taking this patch (and alternatives if risky): none
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

[Approval Request Comment]
Regression caused by (bug #): 674753
User impact if declined: malicious app could hijack firefox and do anything, so long as the user explicitly installs said malicious app
Testing completed (on m-c, etc.): none required
Risk to taking this patch (and alternatives if risky): none
String or IDL/UUID changes made by this patch: none
Attachment #784432 - Flags: approval-mozilla-release?
Attachment #784432 - Flags: approval-mozilla-esr17?
Attachment #784432 - Flags: approval-mozilla-beta?
Attachment #784432 - Flags: approval-mozilla-aurora?
Comment on attachment 784432 [details] [diff] [review]
0001-Bug-899702-Only-attempt-load-of-egltrace.so-when-a-p.patch

Please go ahead with the branch uplifts asap.
Attachment #784432 - Flags: approval-mozilla-release?
Attachment #784432 - Flags: approval-mozilla-release-
Attachment #784432 - Flags: approval-mozilla-beta?
Attachment #784432 - Flags: approval-mozilla-beta+
Attachment #784432 - Flags: approval-mozilla-aurora?
Attachment #784432 - Flags: approval-mozilla-aurora+
George, we really don't need this on ESR, do we? This is an Android specific issue and there is no ESR release on Android.
Oh right, yes, that's true. Let's cancel the ESR request then :)
Flags: needinfo?(gwright)
(Assignee)

Updated

4 years ago
Attachment #784432 - Flags: approval-mozilla-esr17?
https://hg.mozilla.org/releases/mozilla-aurora/rev/42d7eed1d9bc
https://hg.mozilla.org/releases/mozilla-beta/rev/cc61aa3e37ff
status-firefox24: affected → fixed
status-firefox25: affected → fixed
Group: gfx-core-security
status-b2g18: --- → unaffected
status-firefox-esr17: --- → unaffected
Depends on: 946860
Group: core-security
You need to log in before you can comment on or make changes to this bug.