1229681 - (CVE-2016-2810) Content providers protected with signature-level permissions can be accessed by an application installed beforehand that defines the same permissions

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Data Providers, defect)

Description

[Ken Okuyama]

Attachment: poc.apk

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

Steps to reproduce:

1. Launch an emulator (or device) whose API level is 16( or lower)
2. Install attached poc app (poc.apk) with adb
3. Install firefox for android app with adb
4. Launch and use firefox for android app usually (add bookmarks, save passwords, e.t.c.)
5. Launch poc app and press some buttons on UI
   -> poc app tries to access content providers below.
      - org.mozilla.gecko.db.BrowserProvider
      - org.mozilla.gecko.db.PasswordsProvider
      - org.mozilla.gecko.db.FormHistoryProvider

Mechanism:
- In API level 20 or lower, a custom permission defined first is enabled (and duplicated name permissions are ignored).
  -> The poc app defines permissions, whose name are same as firefox defines, as normal level
     - org.mozilla.firefox.permissions.PASSWORD_PROVIDER
     - org.mozilla.firefox.permissions.BROWSER_PROVIDER
     - org.mozilla.firefox.permissions.FORMHISTORY_PROVIDER
- In API level 16 or lower, default value of exported attribute of content provider is true.
  -> Any app can access some content providers in firefox app on API level 16 or lower.


Actual results:

The poc app can read data handled by the content providers.

Note that the poc app only read data, but it can insert, update and delete data if implement functions.


Expected results:

The content providers deny access from app whose signature is different from Mozilla's one.

Comment 1

[Ken Okuyama]

Attachment: poc(sorce).zip

Comment 2

[Richard Newman [:rnewman]]

Interesting find, Ken!

To me this sounds like an Android bug. It sounds like this affects any exported ContentProvider, even those with permissionLevel=signature, simply by virtue of being exported.

In the presence of this approach it's impossible to deliver two APKs and use signature permission levels to have them share data, right?

Our mitigation would be to not export any content providers.


Ken, three questions for you:

* Is this fixed in Android 21+? Your report suggests so, but it's not 100% explicit.

* Have you reported this to the Android project? If so, do you have a reference?

* Have you demonstrated this against other applications? It sounds like the approach would generalize. (It's not necessary for you to disclose the names if you don't want to.)

Comment 3

[Ken Okuyama]

(In reply to Richard Newman [:rnewman] from comment #2)

> To me this sounds like an Android bug.

This is a known limitation of custom permission since 2012.
Please check sites below. 

 - http://www.mail-archive.com/[e-mail]/msg198707.html
 - http://groups.google.com/group/android-developers/browse_thread/thread/ffdc33045ba9d2bf/cb7727bd08f6e71d?lnk=raot
 - http://code.google.com/p/android/issues/detail?id=25906

> In the presence of this approach it's impossible to deliver two APKs and use
> signature permission levels to have them share data, right?
> 
> Our mitigation would be to not export any content providers.

Setting exported="false" is valid solution in this case.
Otherwise, if apps want to use a signature permission to share data, must check who defines the permission and it is valid.
     
  sample code:

    PackageManager pm = context.getPackageManager();
    PermissionInfo pi = pm.getPermissionInfo("'sigPermName", PackageManager.GET_META_DATA);
    // check packagename is valid
    if (!pi.packageName.equals("validpackagename")) return false;
      // for strictly, check the package signature (like Public Key Pinning Extension for HTTP)
      PackageManager pm = ctx.getPackageManager();
      PackageInfo pkginfo = pm.getPackageInfo(pi.packageName, PackageManager.GET_SIGNATURES);
      Signature sig = pkginfo.signatures[0];
      if( '"sig is invalid' ) return false;
    // check protectionLevel
    if (pi.protectionLevel != PermissionInfo.PROTECTION_SIGNATURE) return false;     
 
> Ken, three questions for you:
> 
> * Is this fixed in Android 21+? Your report suggests so, but it's not 100%
> explicit.

Yes, custom permission install order issue is fixed in Android 21+.
Please see also,
 - https://developer.android.com/about/versions/android-5.0-changes.html#custom_permissions
 
> * Have you reported this to the Android project? If so, do you have a
> reference?

I have reported the issue in 2012, but they answered it is a known limitation (and referred public information above).

> * Have you demonstrated this against other applications? It sounds like the
> approach would generalize. (It's not necessary for you to disclose the names
> if you don't want to.)

I have not demonstrated it except a sample app i made to report.

Comment 4

[Nick Alexander :nalexander [he/him]]

Per comment #2 and #3, the mitigation here is to not export any content providers.  We don't rely on this, and in fact it may be the cause of other tickets (although I can't find a good link).

I'll take this.

Comment 5

[Nick Alexander :nalexander [he/him]]

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

Comment 6

[Nick Alexander :nalexander [he/him]]

Attachment: 1229681.part1.patch

Comment 7

[Nick Alexander :nalexander [he/him]]

Attachment: 1229681.part2.patch

Tested locally by running the background tests and syncing an account.

Comment 8

[Richard Newman [:rnewman]]

Comment on attachment 8699678 [details] [diff] [review]
1229681.part2.patch

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

You should also remove the android:permission attribute from GeckoMessageReceiver, and mark it as non-exported, no?

Comment 9

[Nick Alexander :nalexander [he/him]]

(In reply to Richard Newman [:rnewman] from comment #8)
> Comment on attachment 8699678 [details] [diff] [review]
> 1229681.part2.patch
> 
> Review of attachment 8699678 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You should also remove the android:permission attribute from
> GeckoMessageReceiver, and mark it as non-exported, no?

That was part 1.

Comment 10

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/fx-team/rev/ad6a279c6682741c131fdcef6f1fee3ed7a5972a
Bug 1229681 - Pre: Don't export passwords initialization receiver. r=rnewman

https://hg.mozilla.org/integration/fx-team/rev/04d1a746fe75d18431111cfa50906a4658ffe5af
Bug 1229681 - Don't export any content providers. r=rnewman

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ad6a279c6682
https://hg.mozilla.org/mozilla-central/rev/04d1a746fe75

Comment 12

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

This should have gone through security approval before checkin.

See https://wiki.mozilla.org/Security/Bug_Approval_Process

You can't check in security bugs unless they are trunk only without a security rating.

This needs a bug rating and and the questions in the sec-approval? template answered.

Comment 13

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8699677 [details] [diff] [review]
1229681.part1.patch

This applies to patch 1 and patch 2.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

I think that's Comment 0.  It's not rocket science once you are able to install a named package on the target's Android device.  In theory, installing such a package on the target's Android device is hard.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

I don't really think so; it just looks like code clean up.  An enterprising attacker would observe that the bug itself is not available.

Which older supported branches are affected by this flaw?

All of them.  This pattern is old; we've been exporting these ContentProviders with this security mechanism basically since day one (so 4-5 years back).

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?

No.  Easy to create -- I expect |hg graft| to just work, modulo a file rename or two.  They'll be as risky as this, which is low risk.

How likely is this patch to cause regressions; how much testing does it need?

Low risk for regressions.  I'd like to see a few days/weeks of Nightly use, since it's possible something will come out of the woodwork.

Comment 14

[Nick Alexander :nalexander [he/him]]

abillings: do I need to back this out?

Comment 15

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

Comment on attachment 8699677 [details] [diff] [review]
1229681.part1.patch

No, you don't need to back it out. Once it is in, it is in. At least it isn't a sec-critical issue!

Comment 16

[Matt Wobensmith [:mwobensmith][:matt:]]

Hello Ken, are you able to confirm that this issue has been fixed?

Comment 17

[Ken Okuyama]

(In reply to Matt Wobensmith [:mwobensmith][:matt] from comment #16)
> Hello Ken, are you able to confirm that this issue has been fixed?

Hi, Matt.
I have confirmed that this issue has been fixed with fennec.