RFC: Check whether user granted signature spoofing permission

[teodly]

Welcome! First of all, thanks @Lanchon for this awesome piece of software and your devotion to free/libre software. Also the patching test suite (bulk-patch-builder/build-all) with many Android versions impressed me.

I've spotted and fixed security issue with signature spoofing patch:

Currently the signature spoofing patch merely checks whether the application has the permission specified in manifest. While it could make sense in older versions of Android, when permissions were granted at the moment of installation, in Android 10 package installer doesn't even show them to the user, instead the apps need to ask for them when they need it or on the first run. (That's how I understand, correct me if I'm wrong.)

It may lead to situation when some app fakes signatures without the user ever noticing.

These commits, inspired by microG patches for recent versions of Android, should close this loophole and make the signature spoofing permission work like all other permissions. To make Android ask the user for this permission, also protectionLevel must be set to dangerous. See my comment here for instructions how to achieve this: #24 (comment)

Video: https://www.youtube.com/watch?v=QvkdxvJlpOU

This pull request drops support for Android<4.1, because these versions don't have the PackageInfo.requestedPermissionsFlags field.

bulk-patch-builder/build-all reported success. Tested on Android 10 patched using NanoDroid patcher.

Maybe there's a better way of doing this. Maybe if we patched PackageParser.generatePackageInfo instead of PackageManagerService.generatePackageInfo we would have the permissions argument exposed, wouldn't need to fiddle with the GET_PERMISSIONS flag and iterate over the requestedPermissions array, and would be even closer to the microG source patch. It may not work with Android<6.0, though.

[Lanchon]

hi @teowoz,

thank you so much for this PR, i really appreciate it.

i'll take a minute for a quick response:

• i need to research this security issue, including understanding it first, and knowing which android versions are affected. but i don't have the time right now.

• i won't accept a PR that drops support for android < 4.1. as android versions progressed, differences in code to patch mounted up, so i needed to split the source code for patches according to versions. in time the splits became too numerous to easily make sense of and maintain. i was breaking the DRY principle. so i merged (and improved) all patch sources using a preprocessor of the java text. the preprocessor uses comment-like sequences to selectively include or exclude lines for android version ranges. there is no need to drop support for any android version; instead differences have to be expressed using this mechanism.

• you imply that the alleged security issue doesn't affect older versions of android, yet your PR seems to affect all versions. i'm hesitant to replace old, proven patches with untested versions including these changes for no good reason. either the changes are needed in a specific android version or they are not. and if they are not, do they provide other benefits? if both answers are no, then i probably won't accept changes to old patches for obvious reasons.

• in your description you seem to imply that the issue is related to the android runtime permission system added in android 6. i know for a fact that the patches work OK in android 6: if the runtime sigspoof permission is not granted to the app, the patch won't allow sigspoof. this requires no special code to query the system: the system already says you don't have a certain permission unless it is runtime-enabled in android 6, if an app targets android 6 or later. otherwise the installer asks for the permission instead. (might this be the issue? this is an android system-wide issue for all apps and permissions. play store protects you from this by not accepting new apps that target android <6, but this protection disappears when you sideload. a system-wide defense would be more appropriate for this than a specific fix for this particular permission. on android, users MUST look at permissions at install times. a mitigation would be to add a separate global toggle to allow pre-6 apps to sigspoof, which users should normally leave unchecked.)

• the commits change lots of things at once, so i can't see what security changes you made. deleted support for old android versions and lots of formatting changes of unchanged code hide the code changes, so i can't see them at a glance. in general, separating formatting/code cleanup changes from functional changes into separate commits greatly simplify code review.

as i said, i don't have time to research the underlying issue you present right now, so this will have to wait, but i wanted to give you some kind of response in the meantime. btw, i'd gladly merge PRs more in line with the comments here... when i have time to research.

thanks again!

[teodly]

Thanks for your reply.

so i merged (and improved) all patch sources using a preprocessor of the java text. the preprocessor uses comment-like sequences to selectively include or exclude lines for android version ranges. there is no need to drop support for any android version; instead differences have to be expressed using this mechanism.

I saw it. However the file GeneratePackageInfoHook.java which requires PackageInfo.requestedPermissionsFlags doesn't have any comments for this preprocessor so I assumed it won't work out of the box and would require change in build script. So I just removed Android versions <4.1 to check whether it makes sense and works at all.

Would it be OK to add preprocessor comments in GeneratePackageInfoHook.java?

either the changes are needed in a specific android version or they are not.

They are needed in all Android versions where PackageInfo.requestedPermissions hold all permissions that possibly could be granted, and PackageInfo.requestedPermissionsFlags are used to differentiate between granted and non-granted permissions. I don't know which versions exactly.

do they provide other benefits?

No.

i know for a fact that the patches work OK in android 6: if the runtime sigspoof permission is not granted to the app, the patch won't allow sigspoof. this requires no special code to query the system: the system already says you don't have a certain permission unless it is runtime-enabled in android 6, if an app targets android 6 or later.

It implies that in Android 6, unlike Android 10, PackageInfo.requestedPermissions isn't filled with permissions not yet granted.

a system-wide defense would be more appropriate for this than a specific fix for this particular permission.

I don't think it's necessary. I suppose (but AOSP code review would be needed to confirm it) that the OS uses PackageInfo.requestedPermissionsFlags or gets it on per-user basis like this:

PackageSetting.readUserState(userId).pkg.getPermissions(userId)

see PackageManagerService.generatePackageInfo

a mitigation would be to add a separate global toggle to allow pre-6 apps to sigspoof, which users should normally leave unchecked.

I don't think it's necessary, too. Installing a pre-6 app will make the package installer show the permissions list for the user to confirm.

lots of formatting changes of unchanged code hide the code changes

I'm used to GitLab which allows hiding whitespace changes in single click. You can do the same in the command line after checking out my branch:

git diff -w 050d6319ad2f7b4a0558d8d61ce158d12a83e99c e845eb3e72bae2d20ac9c9907f566289cf53b5ef

-w is a short form of --ignore-all-space

Speaking of Android, I'm not app or system developer (but I have experience with programming on other platforms), just a power user not wanting to have closed source GApps running as system apps. Hence my interest in microG and signature spoofing. I don't know what to do next to (efficiently) test my modifications against other Android versions. Preferably without the need of manual patching and testing on every single version.

[Lanchon]

thanks!

i still don't have time research this properly but i'll answer.

i suppose all files are preprocessed, i don't see a reason why i might have chosen not to do that.

They are needed in all Android versions where PackageInfo.requestedPermissions hold all permissions that possibly could be granted, and PackageInfo.requestedPermissionsFlags are used to differentiate between granted and non-granted permissions. I don't know which versions exactly.

It implies that in Android 6, unlike Android 10, PackageInfo.requestedPermissions isn't filled with permissions not yet granted.

interesting. this contradicts my expectations:

1. i know the permission won't work if not granted. PackageInfo.requestedPermissions only provided granted runtime permissions in older androids and i seriously doubt google would choose to modify this behavior (but, of course, anything could be).

how do i know? when the permission (if runtime) was not granted to a checker app, the app could not spoof. see these issues on the checker app:
Lanchon/sigspoof-checker#2
Lanchon/sigspoof-checker#3
Lanchon/sigspoof-checker#7

2. again, i've no time to investigate now, but a bit of random following from your link:
   https://android.googlesource.com/platform/frameworks/base/+/master/services/core/java/com/android/server/pm/PackageManagerService.java#3987
   leads to:
   https://android.googlesource.com/platform/frameworks/base/+/master/services/core/java/com/android/server/pm/PackageManagerService.java#4018
   https://github.com/aosp-mirror/platform_frameworks_base/blob/master/services/core/java/com/android/server/pm/permission/PermissionsState.java#L344-L346
   https://github.com/aosp-mirror/platform_frameworks_base/blob/master/services/core/java/com/android/server/pm/permission/PermissionsState.java#L256
   https://github.com/aosp-mirror/platform_frameworks_base/blob/master/services/core/java/com/android/server/pm/permission/PermissionsState.java#L286
   https://github.com/aosp-mirror/platform_frameworks_base/blob/master/services/core/java/com/android/server/pm/permission/PermissionsState.java#L732

it doesn't mean anything, but it strongly looks like only grated permissions are returned by PackageInfo.requestedPermissions.

It implies that in Android 6, unlike Android 10, PackageInfo.requestedPermissions isn't filled with permissions not yet granted.

so it looks like your assertion about android 10 is wrong. whatever the security issue might be if any, it looks like using requestedPermissionsFlags is redundant.

however...

i now see a possible unrelated problem. unless android knows the permission explicitly as dangerous, it might assume it is not (on android 6+ where the runtime permissions exist) and grant it to all apps that request it in the manifest without showing the install warning and without dynamically requesting it. this is a problem.

right of the bat, maybe the requesting app can declare its own permission as dangerous (and then we should require that it does), and then the patch should never grant access if the permission is found to be runtime on android 6+.

but... this would break compatibility with all apps that sigspoof right now. and it might make it entirely impossible for a new sigspoof app designed to target android <6 to work in android 6+. so it's not a solution.

i know you mention some stuff regarding this in your OP, but i don't have time to research now.

[Lanchon]

ok and now i see this issue:
#24

the video seems to contradict my position and favor yours.

but if you are right, how is it that people complained about sigspoof checker giving wrong results before it actually requested the permission?

i've one possible explanation: the video might have been taken on a rom haystack-patched over a rom that already had sigspoof of some sort?

anyway, this would be easy to fix in the patch if necessary.

the difficult part is making the patch safe on android 6+ roms that don't declare the permission as dangerous.

[teodly]

but if you are right, how is it that people complained about sigspoof checker giving wrong results before it actually requested the permission?
i've one possible explanation: the video might have been taken on a rom haystack-patched over a rom that already had sigspoof of some sort?

It's simpler I think. From what I see in #2, the sigspoof checker reported disabled spoofing before the permission was granted. In the issues you linked, I don't see people stating explicitly that they use haystack. They probably used some ROM patched with microG patches.

so it looks like your assertion about android 10 is wrong. whatever the security issue might be if any, it looks like using requestedPermissionsFlags is redundant.

After revoking permission for your signature spoofing checker app, I'm able to make my patch output this in log: https://github.com/teowoz/haystack/blob/e845eb3e72bae2d20ac9c9907f566289cf53b5ef/patches-src-gen/sigspoof-core/services.jar/GeneratePackageInfoHook.java#L57 which proves that requestedPermissions does contain this permission even if set to denied. At least in Android 10.

unless android knows the permission explicitly as dangerous, it might assume it is not (on android 6+ where the runtime permissions exist) and grant it to all apps that request it in the manifest without showing the install warning and without dynamically requesting it. this is a problem.

Yes, that's how it looks like from my experiments. When protectionLevel is normal, it is granted automatically and you can't even revoke it in the apps permissions settings UI.

Possible workarounds I see:

• The most elegant solution is patching the framework-res.apk. It would require apktool and signing tools so I suppose it's beyond the scope of haystack.
• Installation of org.spoofing.apk used by NanoDroid. I don't know how secure is it and whether the protectionLevel can be overridden by another app. (I suppose it can't because declaring custom permissions wouldn't make much sense then)
• Patching the system permission logic to force dangerous protectionLevel when it sees the FAKE_PACKAGE_SIGNATURE permission. This would be hacky but easiest to maintain for the users and authors of other tools (e.g. NanoDroid).

I will do experiments once I find time to install emulator and older Android versions on it. But I have other projects to do so it may be matter of a month or half a year ;)

[Lanchon]

In the issues you linked, I don't see people stating explicitly that they use haystack.

lol you're absolutely right! thanks, i'm convinced.

to sum up...

haystack has an issue: the permission is an install permission for all androids. on android 6+ install permissions are granted automatically for apps targeting android 6+, which is very bad.

some people tried to sidestep this issue by making additional changes to the system that result in the permission being runtime. they thought that was it, but haystack does not check for the permission being granted because it was written to handle an install permission.

problem understood, now actions:

1. evolve haystack to check for permission being granted on android 6+ if the permission happens to be runtime.

2. optionally make the permission runtime with a new patch for android 6+.

i know 1) is probably handled by your PR, but i'll look for options before merging when the time comes to deal with this.

for 2), yes, my version of haystack will never patch resources. but yes, it can patch the framework to add a permission group and a permission.

pros: the org.spoofing hack does not seem to be able to properly create a permission group (or super group, however it is called), so you don't get a nice UI, while a patch could provide all that is needed. the hack probably pollutes the app space and is shown as a system app. what happens if this app is disabled? can it be?

cons: same as current UI, it will probably be an english-only solution without i18n support.

thanks for putting me up to speed on these issues. i will took into fixing this if/when i got the time. in the meantime people can enjoy a working system thanks to your fork.