Closed Bug 1245791 (CVE-2016-5299) Opened 8 years ago Closed 8 years ago

Firefox AuthToken in broadcast protected with signature-level permission can be accessed by an application installed beforehand that defines the same permissions

Categories

(Firefox for Android Graveyard :: Firefox Accounts, defect)

47 Branch
defect
Not set
normal

Tracking

(firefox49 wontfix, firefox-esr45 unaffected, firefox50 fixed)

RESOLVED FIXED
Firefox 50
Tracking Status
firefox49 --- wontfix
firefox-esr45 --- unaffected
firefox50 --- fixed

People

(Reporter: kenken0980, Assigned: Grisha)

Details

(Keywords: sec-moderate, Whiteboard: [post-critsmash-triage][adv-main50+])

Attachments

(5 files)

Attached file 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.
Flags: sec-bounty?
Attached file poc(source).zip
added poc source codes.
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.
Assignee: nobody → nalexander
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(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.
Nick won't be able to fix this any time soon. Grisha, could you look into this?
Flags: needinfo?(gkruglov)
Assignee: nalexander → gkruglov
Flags: needinfo?(gkruglov)
Grisha, any updates here? It has been a month.
Flags: needinfo?(gkruglov)
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.
Bug 1245791 - Pre: Remove unnecessary AccountLoaderNative r=nalexander
Flags: needinfo?(gkruglov)
Attachment #8769002 - Flags: review?(nalexander)
Bug 1245791 - Part 1: Start FxAccountDeletedService directly, not through an intent r=nalexander
Attachment #8769003 - Flags: review?(nalexander)
Bug 1245791 - Part 2: Use LocalBroadcastManager for account change broadcasts r=nalexander
Attachment #8769004 - Flags: review?(nalexander)
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 on attachment 8769002 [details] [diff] [review]
kill-account-type-perm-pre1.patch

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

wfm.
Attachment #8769002 - Flags: review?(nalexander) → review+
Comment on attachment 8769003 [details] [diff] [review]
kill-account-type-perm-part1.patch

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

Looks sensible.
Attachment #8769003 - Flags: review?(nalexander) → review+
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.
Attachment #8769004 - Flags: review?(nalexander) → review+
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
Flags: sec-bounty? → sec-bounty+
Group: firefox-core-security → core-security-release
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main50+]
Alias: CVE-2016-5299
Group: core-security-release
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: