Last Comment Bug 899702 - (CVE-2013-1731) Android looks for egltrace.so in public directory
(CVE-2013-1731)
: Android looks for egltrace.so in public directory
Status: RESOLVED FIXED
[adv-main24+]
: sec-high
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: ARM Android
: -- normal (vote)
: mozilla26
Assigned To: George Wright (:gw280) (:gwright)
:
Mentors:
Depends on: 946860
Blocks:
  Show dependency treegraph
 
Reported: 2013-07-30 11:51 PDT by Vladimir Vukicevic [:vlad] [:vladv]
Modified: 2014-11-19 20:11 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
+
fixed
+
fixed
+
fixed
unaffected
unaffected


Attachments
0001-Bug-899702-Only-attempt-load-of-egltrace.so-when-a-p.patch (1.50 KB, patch)
2013-07-30 11:58 PDT, George Wright (:gw280) (:gwright)
vladimir: review+
Details | Diff | Splinter Review
0001-Bug-899702-Only-attempt-load-of-egltrace.so-when-a-p.patch (2.00 KB, patch)
2013-08-01 09:24 PDT, George Wright (:gw280) (:gwright)
gwright: review+
bajaj.bhavana: approval‑mozilla‑aurora+
bajaj.bhavana: approval‑mozilla‑beta+
bajaj.bhavana: approval‑mozilla‑release-
abillings: sec‑approval+
Details | Diff | Splinter Review

Description Vladimir Vukicevic [:vlad] [:vladv] 2013-07-30 11:51:25 PDT
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.
Comment 1 George Wright (:gw280) (:gwright) 2013-07-30 11:58:29 PDT
Created attachment 783282 [details] [diff] [review]
0001-Bug-899702-Only-attempt-load-of-egltrace.so-when-a-p.patch
Comment 2 Vladimir Vukicevic [:vlad] [:vladv] 2013-07-30 12:05:07 PDT
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 3 Milan Sreckovic [:milan] 2013-07-30 14:41:54 PDT
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;

{ }?
Comment 4 Andrew McCreight [:mccr8] 2013-07-31 10:13:13 PDT
Sounds high or critical to me.  I'm not sure how much requiring local privilege mitigates this, ratings-wise.
Comment 5 George Wright (:gw280) (:gwright) 2013-07-31 11:53:37 PDT
How would you like us to go about landing this, and do we want it on aurora/beta/release?
Comment 6 Milan Sreckovic [:milan] 2013-08-01 08:00:18 PDT
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.
Comment 7 George Wright (:gw280) (:gwright) 2013-08-01 09:24:01 PDT
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.
Comment 8 George Wright (:gw280) (:gwright) 2013-08-01 09:24:43 PDT
Ugh, I accidentally left a few hunks from all.js in the updated patch. I will not land those, disregard.
Comment 9 Al Billings [:abillings] 2013-08-08 13:24:12 PDT
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.
Comment 10 Al Billings [:abillings] 2013-08-22 13:17:44 PDT
Vlad, can you check this in so it doesn't have to wait until George is back?
Comment 11 Milan Sreckovic [:milan] 2013-08-27 10:29:20 PDT
George is back now.
Comment 12 George Wright (:gw280) (:gwright) 2013-08-27 10:57:27 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ed0a7162516
Comment 13 Ryan VanderMeulen [:RyanVM] 2013-08-27 18:30:59 PDT
https://hg.mozilla.org/mozilla-central/rev/5ed0a7162516
Comment 14 Daniel Veditz [:dveditz] 2013-08-28 15:39:01 PDT
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.
Comment 15 bhavana bajaj [:bajaj] 2013-08-29 12:58:40 PDT
Needinfo on George to help with approval request on branches here.
Comment 16 George Wright (:gw280) (:gwright) 2013-08-30 10:42:17 PDT
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
Comment 17 bhavana bajaj [:bajaj] 2013-08-30 10:47:53 PDT
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.
Comment 18 Al Billings [:abillings] 2013-08-30 11:39:41 PDT
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.
Comment 19 George Wright (:gw280) (:gwright) 2013-08-30 12:20:20 PDT
Oh right, yes, that's true. Let's cancel the ESR request then :)

Note You need to log in before you can comment on or make changes to this bug.