1245795 - (CVE-2016-9061) API Key (glocation) in broadcast protected with signature-level permission can be accessed by an application installed beforehand that defines the same permissions

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Ken Okuyama]

Attachment: poc-apikey.apk

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/48.0.2564.97 Safari/537.36

Steps to reproduce:

1. Launch an emulator (or device) whose API level is 19( or lower)
2. Install attached poc app (poc-apikey.apk) with adb
3. Install firefox for android (fennec) with adb
4. Launch and use firefox app


Actual results:

The poc app can receive a broadcast protedted with "org.mozilla.fennec.permission.PER_ANDROID_PACKAGE".
So, the poc app can read API Key in the broadcast.


Expected results:

The broadcast cannot be read from app whose signature is different from Mozilla's one.
Or the broadcast will change so that it does not have api key.

Comment 1

[James Willcox (:snorp) ([e-mail]) (he/him)]

Ken, can you attach the source for your apk?

Comment 2

[James Willcox (:snorp) ([e-mail]) (he/him)]

Ah, source might not be very interesting. I guess the problem is that the poc app declares the permission first, and this somehow confuses the Android permission system. The Fennec permission protection is set to 'signature', but apparently that is not respected if the permission has already been declared elsewhere. Not sure what we can really do about this.

Comment 3

[James Willcox (:snorp) ([e-mail]) (he/him)]

There is a good description of the problem here: http://blog.trendmicro.com/trendlabs-security-intelligence/android-custom-permissions-leak-user-data/

This does appear to be a bug in Android, but we may be able to work around it by adding UID checks.

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

Actually, since we're broadcasting an intent here, I don't know if there is anything we can do other than not broadcast this...

Comment 5

[Nick Alexander :nalexander [he/him] (PTO Feb 24-March 3)]

This is the same as Bug 1229681, and I'm irritated that I didn't address these usages at the same time.

It's easy to address, though: make everything android:exported="false" as appropriate and remove the permissions.

This is a hold-over from the android-sync github repo days, when we had (mostly during development) multiple packages communicating in this way; and from the Old Sync days, when the account type was shared between Android packages.  That's no longer the case, so this can be addressed more simply.

Comment 6

[Ken Okuyama]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #1)
> Ken, can you attach the source for your apk?

Hi, James

Here is my poc source code.

Comment 7

[Ken Okuyama]

Attachment: Poc(source).zip

Here is my poc source code.

Comment 8

[Ken Okuyama]

(In reply to Nick Alexander :nalexander from comment #5)
> This is the same as Bug 1229681, and I'm irritated that I didn't address
> these usages at the same time.
> 
> It's easy to address, though: make everything android:exported="false" as
> appropriate and remove the permissions.

As you point out, this is the same as Bug 1229681 from an attacker.
but I think it is only a little different in the perspective of countermeasure.

In Bug 1229681, the receiver must be modified so that it refuse access from other application.(e.g. exported="false")

In this bug, the sender must be corrected so that other application cannot access the intent which is thrown by the sender. (e.g. LocalBroadcastManager)

Comment 9

[Nick Alexander :nalexander [he/him] (PTO Feb 24-March 3)]

(In reply to Ken Okuyama from comment #8)
> (In reply to Nick Alexander :nalexander from comment #5)
> > This is the same as Bug 1229681, and I'm irritated that I didn't address
> > these usages at the same time.
> > 
> > It's easy to address, though: make everything android:exported="false" as
> > appropriate and remove the permissions.
> 
> As you point out, this is the same as Bug 1229681 from an attacker.
> but I think it is only a little different in the perspective of
> countermeasure.
> 
> In Bug 1229681, the receiver must be modified so that it refuse access from
> other application.(e.g. exported="false")
> 
> In this bug, the sender must be corrected so that other application cannot
> access the intent which is thrown by the sender. (e.g. LocalBroadcastManager)

Meh, a distinction without a difference.  This is part of a larger, long running project of separating Beta/Release and Aurora/Fennec apart.  Most of these things are holdovers from when we expected to really share data; that idea is dead.  We'll just do everything locally, android:exported="false", LocalBroadcastManager, and not worry about the signature permissions at all.

Comment 10

[:Margaret Leibovic]

Nick won't be able to fix this any time soon. Grisha, could you look into this?

Comment 11

[Al Billings [:abillings - ex-MoCo]]

Grisha, any updates on this security bug? It has been a month.

Comment 12

[Nick Alexander :nalexander [he/him] (PTO Feb 24-March 3)]

To address this, we need to remove the PER_ANDROID_PACKAGE permission at https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/FennecManifest_permissions.xml.in#24.

First, expunge all traces of OrderedBroadcastHelper.  It's a nice hook into the Android system, but it's never been used and is a security vulnerability waiting to happen.  That's one consumer of PER_ANDROID_PACKAGE gone.

Second, the interaction with the stumbler at https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/preferences/GeckoPreferences.java#1013.  I think we can use the LocalBroadcastManager, or invoke the service directly.  In both cases the stumbler receiver needs to be split into the external intents and this internal intent (see https://dxr.mozilla.org/mozilla-central/source/mobile/android/stumbler/java/org/mozilla/mozstumbler/service/mainthread/PassiveServiceReceiver.java#57 and https://dxr.mozilla.org/mozilla-central/source/mobile/android/stumbler/manifests/StumblerManifest_services.xml.in#13).

Be aware that the stumbler can be compiled out of Fennec, so it's not easy to invoke the service directly *without an Intent filter*, because the stumbler service class may not present.  (I think the build system enforces this, probably crudely!)  Therefore, I think the path is to split the receiver, and then wire up the LBM listener.  Unfortunately, there's no good way for Fennec to talk to the stumbler, for exactly the build system reasons above!  So I'd suggest sending a "safe" Intent on Fennec app startup to the stumbler telling it to register the LBM listener, and then using the LBM broadcast.  (By safe I mean a broadcast that others can trigger and it'll just set up a listener, and even then only once.  If Android gives a way to figure out that your own APp has been launched, this would work -- but I don't think it does.)  That should work even when the stumbler isn't compiled into Fennec.

Comment 13

[:Grisha Kruglov]

Attachment: stumbler.patch

First pass.

Comment 14

[:Grisha Kruglov]

Attachment: no-package-permission-pre1.patch

Pre: Purge OrderedBroadcast

Comment 15

[:Grisha Kruglov]

Attachment: no-package-permission-part1.patch

Part1: Split Stumbler BroadcastReceivers into Local, System and Safe

SafeReceiver is responsible for registering LocalReceiver with a LocalBroadcastManager.
SystemReceiver is responsible for handling BOOT_COMPLETE and EXTERNAL_APPLICATIONS_AVAILABLE intents.
LocalReceiver is responsible for handling passed in Stumbler preferences (enabled state, API key, user agent).

StumblerPreferences are now sent using LocalBroadcastManager, avoiding any possibility of leaking API key.

Comment 16

[Nick Alexander :nalexander [he/him] (PTO Feb 24-March 3)]

Comment on attachment 8768099 [details] [diff] [review]
no-package-permission-pre1.patch

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

So sad.  If it works for you, it works for me!

Comment 17

[Nick Alexander :nalexander [he/him] (PTO Feb 24-March 3)]

Comment on attachment 8768101 [details] [diff] [review]
no-package-permission-part1.patch

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

This is looking solid.  I have a few nits and a few small issues, but overall this is fine.  Test plan must include enabling/disabling the stumbler, and ensuring that we can build with the stumbler off.  (This might not be trivial, since we don't build the stumbler off in automation these days.)

I would like to see a "messaging diagram" in comments, which probably should go in `StumblerManifest_services.xml.in`.  Something like:

Application.onCreate -> system broadcast STUMBLER_PREF.  This broadcast decouples the Fennec App from the Stumbler internal library.  Most systems would use dependency injection mechanism or a message bus for this; we use the system.

Stumbler receives STUMBLER_PREF, registers local broadcast receiver.  It's safe for other apps to maliciously send STUMBLER_PREF -- this doesn't turn things on or off, just (duplicates) the listener, potentially before Fennec even starts.

Fennec starts or Fennec toggles pref -> local broadcast on/off with sensitive information.

Stumbler receives local broadcast on/off with sensitive information, adjusts internal state.

::: mobile/android/stumbler/java/org/mozilla/mozstumbler/service/mainthread/SafeReceiver.java
@@ +21,5 @@
> +
> +    private boolean registeredLocalReceiver = false;
> +
> +    @Override
> +    public void onReceive(Context context, Intent intent) {

Either synchronize this method, or explain why this doesn't need to be synchronized.  (I think it does need to be synchronized :))

::: mobile/android/stumbler/java/org/mozilla/mozstumbler/service/mainthread/SystemReceiver.java
@@ +25,5 @@
> +        }
> +
> +        final String action = intent.getAction();
> +
> +        if (!action.equals(Intent.ACTION_BOOT_COMPLETED) && !action.equals(Intent.ACTION_EXTERNAL_APPLICATIONS_AVAILABLE)) {

In general, `TextUtils.equals` or constant first -- this will NPE if action is null.  (Which might not be possible, but we've seen *all kinds* of Intents.)

::: mobile/android/stumbler/manifests/StumblerManifest_services.xml.in
@@ +5,5 @@
>  
>  <receiver android:name="org.mozilla.mozstumbler.service.uploadthread.UploadAlarmReceiver" />
>  <service android:name="org.mozilla.mozstumbler.service.uploadthread.UploadAlarmReceiver$UploadAlarmService" />
>  
> +<receiver android:name="org.mozilla.mozstumbler.service.mainthread.SafeReceiver">

This can be `android:exported="false"`, right?

@@ +12,4 @@
>    </intent-filter>
>  </receiver>
> +
> +<receiver android:name="org.mozilla.mozstumbler.service.mainthread.SystemReceiver">

This should be `android:exported="true"` for clarity, right?

Comment 18

[:Grisha Kruglov]

Attachment: no-package-permission-part1.patch

Addressed feedback for patch: Split Stumbler BroadcastReceivers into Local, System and Safe

Comment 19

[Nick Alexander :nalexander [he/him] (PTO Feb 24-March 3)]

Comment on attachment 8769021 [details] [diff] [review]
no-package-permission-part1.patch

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

If it works for you, it works for me!  Good job on these sec patches.

Comment 20

[:Grisha Kruglov]

https://hg.mozilla.org/integration/fx-team/rev/3687f36b1f85c09aaa4c47c99a3286fd7975b9ff
Bug 1245795 - Pre: Purge OrderedBroadcast r=nalexander

https://hg.mozilla.org/integration/fx-team/rev/97ac5877d152245c846ce052f2a60e2a250f6b72
Bug 1245795 - Split Stumbler BroadcastReceivers into Local, System and Safe r=nalexander

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3687f36b1f85
https://hg.mozilla.org/mozilla-central/rev/97ac5877d152

Comment 22

[Daniel Veditz [:dveditz]]

The bounty on bug 1229681 was higher than justified by the rating/impact (too much holiday partying?) but we shouldn't hold that against this bug: this gets a bounty too.