Closed Bug 899702 (CVE-2013-1731) Opened 6 years ago Closed 6 years ago

Android looks for egltrace.so in public directory

Categories

(Core :: Graphics, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26
Tracking Status
firefox23 --- wontfix
firefox24 + fixed
firefox25 + fixed
firefox26 + fixed
firefox-esr17 --- unaffected
b2g18 --- unaffected

People

(Reporter: vlad, Assigned: gw280)

References

Details

(Keywords: sec-high, Whiteboard: [adv-main24+])

Attachments

(1 file, 1 obsolete file)

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.
Attachment #783282 - Flags: review?(vladimir) → review+
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.
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]
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)
Whiteboard: [checkin after 8/20] → [adv-main24+]
Alias: CVE-2013-1731
https://hg.mozilla.org/mozilla-central/rev/5ed0a7162516
Status: NEW → RESOLVED
Closed: 6 years ago
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-criticalsec-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)
Attachment #784432 - Flags: approval-mozilla-esr17?
Group: gfx-core-security
Group: core-security
You need to log in before you can comment on or make changes to this bug.