From 49e179cb7f983cae566d35e15e10ca9de0085c44 Mon Sep 17 00:00:00 2001 From: Hai Zhang Date: Wed, 30 Jun 2021 22:44:51 -0700 Subject: [PATCH] Fix allowlisting restricted permissions for secondary users on app upgrade. restorePermissionState() runs for all users and drops any restricted permission grant without an allowlist, however we may not have the allowlist ready for other users when package manager notifies us of a package install for one of the installed users, as the callback API is (and should be) per-user. To be more specific about this issue: The way this used to work is very fragile and relies on the implementation detail that things happen in restorePermissionState() with the following order: 1. Implicit permissions are considered new if there wasn't any permission state for it. 2. Then permission restrictions are applied and since a new permission state won't have any allowlist, the APPLY_RESTRICTION flag is added and the permission is made revoked. 3. setInitialGrantForNewImplicitPermissionsLocked() is called after permission restrictions are applied, and doesn't respect the APPLY_RESTRICTION flag, so that the new implicit permission is granted. And if the allowlist is added in time before the next restorePermissionState() call, the permission restriction mechanism will see the allowlist and not revoke the implicit permission grant. However since our callback API from package manager is per-user, and we want to properly support adding instead of replacing the allowlist for each user (the existing allowlist for each user can be different so addition has to be done with get-add-set per-user), the allowlist addition has to be per-user, and this would introduce a restorePermissionState() call that's too early for the other users. Thanks to the refactoring that happened in S, the permission state objects are now per-UID instead of per-package, so that the updates are actually being perform per-user now, and we just need to expose the capability for restorePermissionState() to be per-user instead of hard-coded to run for all users. Then we can prepare the permission state and allowlist for each user independently and fix this issue. Fixes: 191381905 Test: atest SplitPermissionTest --user-type secondary_user Test: atest CtsPermission2TestCases Test: atest CtsPermission2TestCases --user-type secondary_user Test: atest CtsPermission3TestCases Test: atest CtsPermission3TestCases --user-type secondary_user Change-Id: I6973fd54281a3b15e5206db286eab8b918709a19 --- .../permission/PermissionManagerService.java | 37 ++++++++++++------- 1 file changed, 23 insertions(+), 14 deletions(-) diff --git a/services/core/java/com/android/server/pm/permission/PermissionManagerService.java b/services/core/java/com/android/server/pm/permission/PermissionManagerService.java index 38e9d3ec34e3a..30eccca48ee93 100644 --- a/services/core/java/com/android/server/pm/permission/PermissionManagerService.java +++ b/services/core/java/com/android/server/pm/permission/PermissionManagerService.java @@ -2557,16 +2557,17 @@ public class PermissionManagerService extends IPermissionManager.Stub { *
  • During app update the state gets restored from the last version of the app
    • * * - *

    This restores the permission state for all users. - * * @param pkg the package the permissions belong to * @param replace if the package is getting replaced (this might change the requested * permissions of this package) * @param packageOfInterest If this is the name of {@code pkg} add extra logging * @param callback Result call back + * @param filterUserId If not {@link UserHandle.USER_ALL}, only restore the permission state for + * this particular user */ private void restorePermissionState(@NonNull AndroidPackage pkg, boolean replace, - @Nullable String packageOfInterest, @Nullable PermissionCallback callback) { + @Nullable String packageOfInterest, @Nullable PermissionCallback callback, + @UserIdInt int filterUserId) { // IMPORTANT: There are two types of permissions: install and runtime. // Install time permissions are granted when the app is installed to // all device users and users added in the future. Runtime permissions @@ -2584,7 +2585,8 @@ public class PermissionManagerService extends IPermissionManager.Stub { return; } - final int[] userIds = getAllUserIds(); + final int[] userIds = filterUserId == UserHandle.USER_ALL ? getAllUserIds() + : new int[] { filterUserId }; boolean runtimePermissionsRevoked = false; int[] updatedUserIds = EMPTY_INT_ARRAY; @@ -3874,7 +3876,8 @@ public class PermissionManagerService extends IPermissionManager.Stub { if (updatePermissions) { // Update permission of this app to take into account the new allowlist state. - restorePermissionState(pkg, false, pkg.getPackageName(), mDefaultPermissionCallback); + restorePermissionState(pkg, false, pkg.getPackageName(), mDefaultPermissionCallback, + userId); // If this resulted in losing a permission we need to kill the app. if (oldGrantedRestrictedPermissions == null) { @@ -4028,14 +4031,17 @@ public class PermissionManagerService extends IPermissionManager.Stub { * * @param packageName The package that is updated * @param pkg The package that is updated, or {@code null} if package is deleted + * @param filterUserId If not {@link UserHandle.USER_ALL}, only restore the permission state for + * this particular user */ - private void updatePermissions(@NonNull String packageName, @Nullable AndroidPackage pkg) { + private void updatePermissions(@NonNull String packageName, @Nullable AndroidPackage pkg, + @UserIdInt int filterUserId) { // If the package is being deleted, update the permissions of all the apps final int flags = (pkg == null ? UPDATE_PERMISSIONS_ALL | UPDATE_PERMISSIONS_REPLACE_PKG : UPDATE_PERMISSIONS_REPLACE_PKG); - updatePermissions( - packageName, pkg, getVolumeUuidForPackage(pkg), flags, mDefaultPermissionCallback); + updatePermissions(packageName, pkg, getVolumeUuidForPackage(pkg), flags, + mDefaultPermissionCallback, filterUserId); } /** @@ -4057,7 +4063,8 @@ public class PermissionManagerService extends IPermissionManager.Stub { (fingerprintChanged ? UPDATE_PERMISSIONS_REPLACE_PKG | UPDATE_PERMISSIONS_REPLACE_ALL : 0); - updatePermissions(null, null, volumeUuid, flags, mDefaultPermissionCallback); + updatePermissions(null, null, volumeUuid, flags, mDefaultPermissionCallback, + UserHandle.USER_ALL); } finally { PackageManager.uncorkPackageInfoCache(); } @@ -4106,12 +4113,14 @@ public class PermissionManagerService extends IPermissionManager.Stub { * all volumes * @param flags Control permission for which apps should be updated * @param callback Callback to call after permission changes + * @param filterUserId If not {@link UserHandle.USER_ALL}, only restore the permission state for + * this particular user */ private void updatePermissions(final @Nullable String changingPkgName, final @Nullable AndroidPackage changingPkg, final @Nullable String replaceVolumeUuid, @UpdatePermissionFlags int flags, - final @Nullable PermissionCallback callback) { + final @Nullable PermissionCallback callback, @UserIdInt int filterUserId) { // TODO: Most of the methods exposing BasePermission internals [source package name, // etc..] shouldn't be needed. Instead, when we've parsed a permission that doesn't // have package settings, we should make note of it elsewhere [map between @@ -4147,7 +4156,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { // Only replace for packages on requested volume final String volumeUuid = getVolumeUuidForPackage(pkg); final boolean replace = replaceAll && Objects.equals(replaceVolumeUuid, volumeUuid); - restorePermissionState(pkg, replace, changingPkgName, callback); + restorePermissionState(pkg, replace, changingPkgName, callback, filterUserId); }); } @@ -4156,7 +4165,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { final String volumeUuid = getVolumeUuidForPackage(changingPkg); final boolean replace = ((flags & UPDATE_PERMISSIONS_REPLACE_PKG) != 0) && Objects.equals(replaceVolumeUuid, volumeUuid); - restorePermissionState(changingPkg, replace, changingPkgName, callback); + restorePermissionState(changingPkg, replace, changingPkgName, callback, filterUserId); } Trace.traceEnd(TRACE_TAG_PACKAGE_MANAGER); } @@ -4824,7 +4833,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { private void onPackageInstalledInternal(@NonNull AndroidPackage pkg, @NonNull PermissionManagerServiceInternal.PackageInstalledParams params, @UserIdInt int userId) { - updatePermissions(pkg.getPackageName(), pkg); + updatePermissions(pkg.getPackageName(), pkg, userId); addAllowlistedRestrictedPermissionsInternal(pkg, params.getAllowlistedRestrictedPermissions(), FLAG_PERMISSION_WHITELIST_INSTALLER, userId); @@ -4871,7 +4880,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { resetRuntimePermissionsInternal(pkg, userId); return; } - updatePermissions(packageName, null); + updatePermissions(packageName, null, userId); if (sharedUserPkgs.isEmpty()) { removeUidStateAndResetPackageInstallPermissionsFixed(appId, packageName, userId); } else {