899702 - (CVE-2013-1731) Android looks for egltrace.so in public directory

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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) (needinfo me!)]

Attachment: 0001-Bug-899702-Only-attempt-load-of-egltrace.so-when-a-p.patch

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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] (needinfo for best results)]

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]]

Sounds high or critical to me.  I'm not sure how much requiring local privilege mitigates this, ratings-wise.

Comment 5

[George Wright (:gw280) (needinfo me!)]

How would you like us to go about landing this, and do we want it on aurora/beta/release?

Comment 6

[Milan Sreckovic [:milan] (needinfo for best results)]

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) (needinfo me!)]

Attachment: 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) (needinfo me!)]

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 - ex-MoCo]]

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 - ex-MoCo]]

Vlad, can you check this in so it doesn't have to wait until George is back?

Comment 11

[Milan Sreckovic [:milan] (needinfo for best results)]

George is back now.

Comment 12

[George Wright (:gw280) (needinfo me!)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5ed0a7162516

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5ed0a7162516

Comment 14

[Daniel Veditz [:dveditz]]

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]]

Needinfo on George to help with approval request on branches here.

Comment 16

[George Wright (:gw280) (needinfo me!)]

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]]

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 - ex-MoCo]]

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) (needinfo me!)]

Oh right, yes, that's true. Let's cancel the ESR request then :)

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/42d7eed1d9bc
https://hg.mozilla.org/releases/mozilla-beta/rev/cc61aa3e37ff