34.83 KB, application/zip
409.81 KB, application/x-zip-compressed
3.68 KB, patch
|Details | Diff | Splinter Review|
9.92 KB, patch
|Details | Diff | Splinter Review|
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.
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.)
OS: Unspecified → Android
Summary: content providers protected with signature permission can be accessed from an installed app before → Content providers protected with signature-level permissions can be accessed by an application installed beforehand that defines the same permissions
Whiteboard: [android 21+ unaffected?]
Version: Firefox 42 → Trunk
(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://email@example.com/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; 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.
tracking-fennec: --- → ?
Assignee: nobody → nalexander
tracking-fennec: ? → 43+
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.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Tested locally by running the background tests and syncing an account.
Attachment #8699678 - Flags: review?(rnewman)
Attachment #8699677 - Flags: review?(rnewman) → review+
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?
Attachment #8699678 - Flags: review?(rnewman) → review+
(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.
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
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 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.
Attachment #8699677 - Flags: sec-approval?
abillings: do I need to back this out?
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!
Attachment #8699677 - Flags: sec-approval? → sec-approval+
Flags: sec-bounty? → sec-bounty+
Hello Ken, are you able to confirm that this issue has been fixed?
(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.
Whiteboard: [android 21+ unaffected?] → [android 21+ unaffected?][post-critsmash-triage]
Whiteboard: [android 21+ unaffected?][post-critsmash-triage] → [android 21+ unaffected?][post-critsmash-triage][adv-main46+]
You need to log in before you can comment on or make changes to this bug.