1245791 - (CVE-2016-5299) Firefox AuthToken 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 :: Firefox Accounts, defect)

Description

[Ken Okuyama]

Attachment: poc-authtoken.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-authtoken.apk) with adb
3. Install firefox for android (fennec) with adb
4. Launch firefox and sync firefox account
5. Remove firefox account (from setting app)


Actual results:

The poc app can receive a broadcast that is sent when firefox account is removed.
Because the broadcast has AuthToken data, the poc app can read Authtoken.


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 authtoken.

Comment 1

[Ken Okuyama]

Attachment: poc(source).zip

added poc source codes.

Comment 2

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

Essentially a dupe of Bug 1229681, with the added bonus of a leaked authToken.  I think the authToken can probably be dropped -- the deleted account receiver never grew the functionality to actually *use* it, IIRC.

The fix is the same, and I'll take this.

Thanks for the report Ken -- so many vulns out of one stupid Android decision.

Comment 3

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

(In reply to Nick Alexander :nalexander from comment #2)
> Essentially a dupe of Bug 1229681, with the added bonus of a leaked
> authToken.  I think the authToken can probably be dropped -- the deleted
> account receiver never grew the functionality to actually *use* it, IIRC.

The authToken may be used in Bug 1250782 to delete the client ID, so removing it is still pending investigation.

Comment 4

[:Margaret Leibovic]

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

Comment 5

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

Grisha, any updates here? It has been a month.

Comment 6

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

To address this, we need to remove https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/manifests/FxAccountAndroidManifest_activities.xml.in#23 and make this use LocalBroadcastManager.  Given that https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/fxa/receivers/FxAccountDeletedReceiver.java#29 merely starts the service with the given intent, I would _like_ to just invoke the service directly at https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/fxa/authenticator/FxAccountAuthenticator.java#369.  Unfortunately, there are other consumers of the permission, namely https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/fxa/AccountLoaderNative.java.  Therefore, LocalBroadcastManager is the best approach.  It should be a straight-forward replacement and excision; the only thing that requires thought is where to register the LBM listeners.  You might choose to invoke the service directly on deletion, and change the other permission users to be LBM based.

Comment 7

[:Grisha Kruglov]

Attachment: kill-account-type-perm-pre1.patch

Bug 1245791 - Pre: Remove unnecessary AccountLoaderNative r=nalexander

Comment 8

[:Grisha Kruglov]

Attachment: kill-account-type-perm-part1.patch

Bug 1245791 - Part 1: Start FxAccountDeletedService directly, not through an intent r=nalexander

Comment 9

[:Grisha Kruglov]

Attachment: kill-account-type-perm-part2.patch

Bug 1245791 - Part 2: Use LocalBroadcastManager for account change broadcasts r=nalexander

Comment 10

[:Grisha Kruglov]

Patches above remove any use of PER_ACCOUNT_TYPE permission, by switching to either direct invocation of a service (FxAccountDeletedService) or via the LocalBroadcastManager route for AccountLoader stuff; essentially as Nick described in Comment 6.

It's worth noting that I couldn't get the attached poc-authtoken app to actually receive broadcasts (before my changes - there wouldn't be anything to receive with these changes). I've modified its permissions to target my local dev build, built it from source, and ran it on an API19 emulator, but nothing interesting showed up in logcat.

Comment 11

[:Grisha Kruglov]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=57499bb32019

Comment 12

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

Comment on attachment 8769002 [details] [diff] [review]
kill-account-type-perm-pre1.patch

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

wfm.

Comment 13

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

Comment on attachment 8769003 [details] [diff] [review]
kill-account-type-perm-part1.patch

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

Looks sensible.

Comment 14

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

Comment on attachment 8769004 [details] [diff] [review]
kill-account-type-perm-part2.patch

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

Hmm, given how crufty this is in practice, and how infrequent the system accounts are changed, I might have culled the system message entirely, and just accepted some inconsistency.  (Temporary inconsistency.)

However, this should work just fine.  I can't say I really understand the weak reference, but I trust you.

Comment 15

[:Grisha Kruglov]

https://hg.mozilla.org/integration/fx-team/rev/cb6fd440765295df99d55a68345cd5bf922a4ae0
Bug 1245791 - Pre: Remove unnecessary AccountLoaderNative r=nalexander

https://hg.mozilla.org/integration/fx-team/rev/17ddf667fc027d5fcec126f62d106a1bfc1fe6e9
Bug 1245791 - Part 1: Start FxAccountDeletedService directly, not through an intent r=nalexander

https://hg.mozilla.org/integration/fx-team/rev/d2f8b89a20f510239002e777c35a11a63a1a9252
Bug 1245791 - Part 2: Use LocalBroadcastManager for account change broadcasts r=nalexander

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cb6fd4407652
https://hg.mozilla.org/mozilla-central/rev/17ddf667fc02
https://hg.mozilla.org/mozilla-central/rev/d2f8b89a20f5