From e38dffebbd141fe63c5d1f3c492756a3fb217ea6 Mon Sep 17 00:00:00 2001 From: Hai Zhang Date: Wed, 14 Jul 2021 00:29:41 +0000 Subject: [PATCH 1/2] Revert "Fix allowlisting restricted permissions for secondary users on app" This reverts commit 49e179cb7f983cae566d35e15e10ca9de0085c44. Reason for revert: b/193206356 Bug: 193206356 Change-Id: I8020f0292c1a867ad53278f73296ac2aaef974bd --- .../permission/PermissionManagerService.java | 37 +++++++------------ 1 file changed, 14 insertions(+), 23 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 97ccfe6e7b2c6..0f62445650495 100644 --- a/services/core/java/com/android/server/pm/permission/PermissionManagerService.java +++ b/services/core/java/com/android/server/pm/permission/PermissionManagerService.java @@ -2564,17 +2564,16 @@ 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, - @UserIdInt int filterUserId) { + @Nullable String packageOfInterest, @Nullable PermissionCallback callback) { // 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 @@ -2592,8 +2591,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { return; } - final int[] userIds = filterUserId == UserHandle.USER_ALL ? getAllUserIds() - : new int[] { filterUserId }; + final int[] userIds = getAllUserIds(); boolean runtimePermissionsRevoked = false; int[] updatedUserIds = EMPTY_INT_ARRAY; @@ -3885,8 +3883,7 @@ 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, - userId); + restorePermissionState(pkg, false, pkg.getPackageName(), mDefaultPermissionCallback); // If this resulted in losing a permission we need to kill the app. if (oldGrantedRestrictedPermissions == null) { @@ -4040,17 +4037,14 @@ 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, - @UserIdInt int filterUserId) { + private void updatePermissions(@NonNull String packageName, @Nullable AndroidPackage pkg) { // 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, filterUserId); + updatePermissions( + packageName, pkg, getVolumeUuidForPackage(pkg), flags, mDefaultPermissionCallback); } /** @@ -4072,8 +4066,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { (fingerprintChanged ? UPDATE_PERMISSIONS_REPLACE_PKG | UPDATE_PERMISSIONS_REPLACE_ALL : 0); - updatePermissions(null, null, volumeUuid, flags, mDefaultPermissionCallback, - UserHandle.USER_ALL); + updatePermissions(null, null, volumeUuid, flags, mDefaultPermissionCallback); } finally { PackageManager.uncorkPackageInfoCache(); } @@ -4122,14 +4115,12 @@ 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, @UserIdInt int filterUserId) { + final @Nullable PermissionCallback callback) { // 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 @@ -4165,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, filterUserId); + restorePermissionState(pkg, replace, changingPkgName, callback); }); } @@ -4174,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, filterUserId); + restorePermissionState(changingPkg, replace, changingPkgName, callback); } Trace.traceEnd(TRACE_TAG_PACKAGE_MANAGER); } @@ -4842,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, userId); + updatePermissions(pkg.getPackageName(), pkg); addAllowlistedRestrictedPermissionsInternal(pkg, params.getAllowlistedRestrictedPermissions(), FLAG_PERMISSION_WHITELIST_INSTALLER, userId); @@ -4889,7 +4880,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { resetRuntimePermissionsInternal(pkg, userId); return; } - updatePermissions(packageName, null, userId); + updatePermissions(packageName, null); if (sharedUserPkgs.isEmpty()) { removeUidStateAndResetPackageInstallPermissionsFixed(appId, packageName, userId); } else { From 160508ed0a0cd4abeca09c31edc4e264ac8e6ce6 Mon Sep 17 00:00:00 2001 From: Hai Zhang Date: Wed, 14 Jul 2021 13:25:16 -0700 Subject: [PATCH 2/2] Properly fix allowlisting restricted permissions for secondary users on app upgrade. More context is available in the original fix ag/15166603 which is being reverted due to b/193206356. The root cause of the regression is that unlike one may assume, we do keep a per-user permission state even if an app is never installed in that user, however ag/15166603 broken that assumption because it's only initializing the permission state for the installed users. We actually never respected whether a package is installed for a user when checking or mutating a permission state, and networking has kernel code that relies on the user 0 permission state, so we have to keep the original behavior at least in S. So to properly fix the original b/191381905, we should: 1. Still limit the restorePermissionState() call in setAllowlistedRestrictedPermissionsInternal() to the user that it's operating upon, so that it's not accidentally calling restorePermissionState() before the allowlist is set. 2. Make the onPackageInstalled()/onPackageUninstalled() callbacks called only once so that we do one updatePermissions() for all users to initialize the permission state for all of them. Actually the install/uninstall request can only happen for either one user or USER_ALL, so we can keep the current callback signature and just allow USER_ALL in addition to normal users. BYPASS_INCLUSIVE_LANGUAGE_REASON=Touching space around old API names. Bug: 191381905 Bug: 193206356 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 Test: atest MixedManagedProfileOwnerTest#testAlwaysOnVpn Change-Id: I6f314a65247c35f86ea4b8e8bf9f458c1332e29c --- .../server/pm/PackageManagerService.java | 34 +++---- .../permission/PermissionManagerService.java | 97 +++++++++++-------- 2 files changed, 69 insertions(+), 62 deletions(-) diff --git a/services/core/java/com/android/server/pm/PackageManagerService.java b/services/core/java/com/android/server/pm/PackageManagerService.java index 04771b99c09f5..ee44c10edbf38 100644 --- a/services/core/java/com/android/server/pm/PackageManagerService.java +++ b/services/core/java/com/android/server/pm/PackageManagerService.java @@ -7955,12 +7955,9 @@ public class PackageManagerService extends IPackageManager.Stub } catch (PackageManagerException e) { Slog.w(TAG, "updateAllSharedLibrariesLPw failed: ", e); } - final int[] userIds = mUserManager.getUserIds(); - for (final int userId : userIds) { - mPermissionManager.onPackageInstalled(pkg, - PermissionManagerServiceInternal.PackageInstalledParams.DEFAULT, - userId); - } + mPermissionManager.onPackageInstalled(pkg, + PermissionManagerServiceInternal.PackageInstalledParams.DEFAULT, + UserHandle.USER_ALL); writeSettingsLPrTEMP(); } } catch (PackageManagerException e) { @@ -19213,12 +19210,7 @@ public class PackageManagerService extends IPackageManager.Stub } final int autoRevokePermissionsMode = installArgs.autoRevokePermissionsMode; permissionParamsBuilder.setAutoRevokePermissionsMode(autoRevokePermissionsMode); - for (int currentUserId : allUsersList) { - if (ps.getInstalled(currentUserId)) { - mPermissionManager.onPackageInstalled(pkg, permissionParamsBuilder.build(), - currentUserId); - } - } + mPermissionManager.onPackageInstalled(pkg, permissionParamsBuilder.build(), userId); } res.name = pkgName; res.uid = pkg.getUid(); @@ -21862,10 +21854,8 @@ public class PackageManagerService extends IPackageManager.Stub if (sharedUserPkgs == null) { sharedUserPkgs = Collections.emptyList(); } - for (final int userId : allUserHandles) { - mPermissionManager.onPackageUninstalled(packageName, deletedPs.appId, - deletedPs.pkg, sharedUserPkgs, userId); - } + mPermissionManager.onPackageUninstalled(packageName, deletedPs.appId, + deletedPs.pkg, sharedUserPkgs, UserHandle.USER_ALL); } clearPackagePreferredActivitiesLPw( deletedPs.name, changedUsers, UserHandle.USER_ALL); @@ -22082,11 +22072,12 @@ public class PackageManagerService extends IPackageManager.Stub } } + // The method below will take care of removing obsolete permissions and granting + // install permissions. + mPermissionManager.onPackageInstalled(pkg, + PermissionManagerServiceInternal.PackageInstalledParams.DEFAULT, + UserHandle.USER_ALL); for (final int userId : allUserHandles) { - // The method below will take care of removing obsolete permissions and granting - // install permissions. - mPermissionManager.onPackageInstalled(pkg, - PermissionManagerServiceInternal.PackageInstalledParams.DEFAULT, userId); if (applyUserRestrictions) { mSettings.writePermissionStateForUserLPr(userId, false); } @@ -22409,10 +22400,9 @@ public class PackageManagerService extends IPackageManager.Stub } removeKeystoreDataIfNeeded(mInjector.getUserManagerInternal(), nextUserId, ps.appId); clearPackagePreferredActivities(ps.name, nextUserId); - mPermissionManager.onPackageUninstalled(ps.name, ps.appId, pkg, sharedUserPkgs, - nextUserId); mDomainVerificationManager.clearPackageForUser(ps.name, nextUserId); } + mPermissionManager.onPackageUninstalled(ps.name, ps.appId, pkg, sharedUserPkgs, userId); if (outInfo != null) { if ((flags & PackageManager.DELETE_KEEP_DATA) == 0) { 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 0f62445650495..dfc14bd733dfa 100644 --- a/services/core/java/com/android/server/pm/permission/PermissionManagerService.java +++ b/services/core/java/com/android/server/pm/permission/PermissionManagerService.java @@ -2564,16 +2564,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 @@ -2591,7 +2592,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; @@ -3883,7 +3885,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) { @@ -4156,7 +4159,8 @@ 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, + UserHandle.USER_ALL); }); } @@ -4165,7 +4169,8 @@ 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, + UserHandle.USER_ALL); } Trace.traceEnd(TRACE_TAG_PACKAGE_MANAGER); } @@ -4832,18 +4837,20 @@ public class PermissionManagerService extends IPermissionManager.Stub { private void onPackageInstalledInternal(@NonNull AndroidPackage pkg, @NonNull PermissionManagerServiceInternal.PackageInstalledParams params, - @UserIdInt int userId) { + @UserIdInt int[] userIds) { updatePermissions(pkg.getPackageName(), pkg); - addAllowlistedRestrictedPermissionsInternal(pkg, - params.getAllowlistedRestrictedPermissions(), - FLAG_PERMISSION_WHITELIST_INSTALLER, userId); - final int autoRevokePermissionsMode = params.getAutoRevokePermissionsMode(); - if (autoRevokePermissionsMode == AppOpsManager.MODE_ALLOWED - || autoRevokePermissionsMode == AppOpsManager.MODE_IGNORED) { - setAutoRevokeExemptedInternal(pkg, - autoRevokePermissionsMode == AppOpsManager.MODE_IGNORED, userId); + for (final int userId : userIds) { + addAllowlistedRestrictedPermissionsInternal(pkg, + params.getAllowlistedRestrictedPermissions(), + FLAG_PERMISSION_WHITELIST_INSTALLER, userId); + final int autoRevokePermissionsMode = params.getAutoRevokePermissionsMode(); + if (autoRevokePermissionsMode == AppOpsManager.MODE_ALLOWED + || autoRevokePermissionsMode == AppOpsManager.MODE_IGNORED) { + setAutoRevokeExemptedInternal(pkg, + autoRevokePermissionsMode == AppOpsManager.MODE_IGNORED, userId); + } + grantRequestedRuntimePermissionsInternal(pkg, params.getGrantedPermissions(), userId); } - grantRequestedRuntimePermissionsInternal(pkg, params.getGrantedPermissions(), userId); } private void addAllowlistedRestrictedPermissionsInternal(@NonNull AndroidPackage pkg, @@ -4866,7 +4873,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { private void onPackageUninstalledInternal(@NonNull String packageName, int appId, @Nullable AndroidPackage pkg, @NonNull List sharedUserPkgs, - @UserIdInt int userId) { + @UserIdInt int[] userIds) { // TODO: Move these checks to check PackageState to be more reliable. // System packages should always have an available APK. if (pkg != null && pkg.isSystem() @@ -4877,27 +4884,31 @@ public class PermissionManagerService extends IPermissionManager.Stub { // If we are only marking a system package as uninstalled, we need to keep its // pregranted permission state so that it still works once it gets reinstalled, thus // only reset the user modifications to its permission state. - resetRuntimePermissionsInternal(pkg, userId); + for (final int userId : userIds) { + resetRuntimePermissionsInternal(pkg, userId); + } return; } updatePermissions(packageName, null); - if (sharedUserPkgs.isEmpty()) { - removeUidStateAndResetPackageInstallPermissionsFixed(appId, packageName, userId); - } else { - // Remove permissions associated with package. Since runtime - // permissions are per user we have to kill the removed package - // or packages running under the shared user of the removed - // package if revoking the permissions requested only by the removed - // package is successful and this causes a change in gids. - final int userIdToKill = revokeSharedUserPermissionsForDeletedPackageInternal(pkg, - sharedUserPkgs, userId); - final boolean shouldKill = userIdToKill != UserHandle.USER_NULL; - // If gids changed, kill all affected packages. - if (shouldKill) { - mHandler.post(() -> { - // This has to happen with no lock held. - killUid(appId, UserHandle.USER_ALL, KILL_APP_REASON_GIDS_CHANGED); - }); + for (final int userId : userIds) { + if (sharedUserPkgs.isEmpty()) { + removeUidStateAndResetPackageInstallPermissionsFixed(appId, packageName, userId); + } else { + // Remove permissions associated with package. Since runtime + // permissions are per user we have to kill the removed package + // or packages running under the shared user of the removed + // package if revoking the permissions requested only by the removed + // package is successful and this causes a change in gids. + final int userIdToKill = revokeSharedUserPermissionsForDeletedPackageInternal(pkg, + sharedUserPkgs, userId); + final boolean shouldKill = userIdToKill != UserHandle.USER_NULL; + // If gids changed, kill all affected packages. + if (shouldKill) { + mHandler.post(() -> { + // This has to happen with no lock held. + killUid(appId, UserHandle.USER_ALL, KILL_APP_REASON_GIDS_CHANGED); + }); + } } } } @@ -5172,8 +5183,11 @@ public class PermissionManagerService extends IPermissionManager.Stub { @NonNull PackageInstalledParams params, @UserIdInt int userId) { Objects.requireNonNull(pkg, "pkg"); Objects.requireNonNull(params, "params"); - Preconditions.checkArgumentNonNegative(userId, "userId"); - onPackageInstalledInternal(pkg, params, userId); + Preconditions.checkArgument(userId >= UserHandle.USER_SYSTEM + || userId == UserHandle.USER_ALL, "userId"); + final int[] userIds = userId == UserHandle.USER_ALL ? getAllUserIds() + : new int[] { userId }; + onPackageInstalledInternal(pkg, params, userIds); } @Override @@ -5188,8 +5202,11 @@ public class PermissionManagerService extends IPermissionManager.Stub { @UserIdInt int userId) { Objects.requireNonNull(packageName, "packageName"); Objects.requireNonNull(sharedUserPkgs, "sharedUserPkgs"); - Preconditions.checkArgumentNonNegative(userId, "userId"); - onPackageUninstalledInternal(packageName, appId, pkg, sharedUserPkgs, userId); + Preconditions.checkArgument(userId >= UserHandle.USER_SYSTEM + || userId == UserHandle.USER_ALL, "userId"); + final int[] userIds = userId == UserHandle.USER_ALL ? getAllUserIds() + : new int[] { userId }; + onPackageUninstalledInternal(packageName, appId, pkg, sharedUserPkgs, userIds); } @NonNull