From 059333b3b043e81898da991170d8a011fc26e8a8 Mon Sep 17 00:00:00 2001 From: Hai Zhang Date: Wed, 18 Nov 2020 01:50:36 -0800 Subject: [PATCH] Replace permission-specific methods with package change callback. This change replaces the following PermissionManagerInternal methods: - revokeRuntimePermissionsIfGroupChanged() - revokeRuntimePermissionsIfPermissionDefinitionChanged() - addAllPermissionGroups() - addAllPermissions() - transferPermissions() with a single onPackageAdded() callback, to ensure we have an API surface that is generic enough for future new permission systems, and exposes details that will remain absolutely necessary due to compatibility. We cannot use a generic package change callback because most of the time, those callbacks should not be blocking the package changes - however permission is different and needs synchronous callbacks. Permission was tightly coupled with package manager, so that we need to keep compatibility with the behavior that the permission state and any permission changes will be ready right after package changes. Meanwhile, permission is the only system component that needs this level of integration with package manager so it doesn't make sense to build a generic package change callback that may be misused by other system components. Bug: 158736025 Test: presubmit Change-Id: Ia4b7acc0579408eca7bcd08d4fdce6108c31c209 --- .../server/pm/PackageManagerService.java | 65 +------ .../server/pm/permission/Permission.java | 7 +- .../permission/PermissionManagerService.java | 179 +++++++++++------- .../PermissionManagerServiceInternal.java | 47 +---- 4 files changed, 118 insertions(+), 180 deletions(-) diff --git a/services/core/java/com/android/server/pm/PackageManagerService.java b/services/core/java/com/android/server/pm/PackageManagerService.java index ad686f2cab898..b11abde5021dd 100644 --- a/services/core/java/com/android/server/pm/PackageManagerService.java +++ b/services/core/java/com/android/server/pm/PackageManagerService.java @@ -246,7 +246,6 @@ import android.database.ContentObserver; import android.graphics.Bitmap; import android.hardware.display.DisplayManager; import android.net.Uri; -import android.os.AsyncTask; import android.os.Binder; import android.os.Build; import android.os.Bundle; @@ -339,7 +338,6 @@ import com.android.internal.os.SomeArgs; import com.android.internal.os.Zygote; import com.android.internal.telephony.CarrierAppUtils; import com.android.internal.util.ArrayUtils; -import com.android.internal.util.CollectionUtils; import com.android.internal.util.ConcurrentUtils; import com.android.internal.util.DumpUtils; import com.android.internal.util.FastXmlSerializer; @@ -11592,22 +11590,6 @@ public class PackageManagerService extends IPackageManager.Stub } pkgSetting.signatures.mSigningDetails = reconciledPkg.signingDetails; - if (!pkg.getAdoptPermissions().isEmpty()) { - // This package wants to adopt ownership of permissions from - // another package. - for (int i = pkg.getAdoptPermissions().size() - 1; i >= 0; i--) { - final String origName = pkg.getAdoptPermissions().get(i); - final PackageSetting orig = mSettings.getPackageLPr(origName); - if (orig != null) { - if (verifyPackageUpdateLPr(orig, pkg)) { - Slog.i(TAG, "Adopting permissions from " + origName + " to " - + pkg.getPackageName()); - mPermissionManager.transferPermissions(origName, pkg.getPackageName()); - } - } - } - } - if (changedAbiCodePath != null && changedAbiCodePath.size() > 0) { for (int i = changedAbiCodePath.size() - 1; i >= 0; --i) { final String codePathString = changedAbiCodePath.get(i); @@ -12854,27 +12836,6 @@ public class PackageManagerService extends IPackageManager.Stub reconciledPkg.prepareResult != null && reconciledPkg.prepareResult.replace; mAppsFilter.addPackage(pkgSetting, isReplace); - // Don't allow ephemeral applications to define new permissions groups. - if ((scanFlags & SCAN_AS_INSTANT_APP) != 0) { - Slog.w(TAG, "Permission groups from package " + pkg.getPackageName() - + " ignored: instant apps cannot define new permission groups."); - } else { - mPermissionManager.addAllPermissionGroups(pkg, chatty); - } - - // If a permission has had its defining app changed, or it has had its protection - // upgraded, we need to revoke apps that hold it - final List permissionsWithChangedDefinition; - // Don't allow ephemeral applications to define new permissions. - if ((scanFlags & SCAN_AS_INSTANT_APP) != 0) { - permissionsWithChangedDefinition = null; - Slog.w(TAG, "Permissions from package " + pkg.getPackageName() - + " ignored: instant apps cannot define new permissions."); - } else { - permissionsWithChangedDefinition = - mPermissionManager.addAllPermissions(pkg, chatty); - } - int collectionSize = ArrayUtils.size(pkg.getInstrumentations()); StringBuilder r = null; int i; @@ -12901,31 +12862,7 @@ public class PackageManagerService extends IPackageManager.Stub } } - boolean hasOldPkg = oldPkg != null; - boolean hasPermissionDefinitionChanges = - !CollectionUtils.isEmpty(permissionsWithChangedDefinition); - if (hasOldPkg || hasPermissionDefinitionChanges) { - // We need to call revokeRuntimePermissionsIfGroupChanged async as permission - // revoke callbacks from this method might need to kill apps which need the - // mPackages lock on a different thread. This would dead lock. - // - // Hence create a copy of all package names and pass it into - // revokeRuntimePermissionsIfGroupChanged. Only for those permissions might get - // revoked. If a new package is added before the async code runs the permission - // won't be granted yet, hence new packages are no problem. - final ArrayList allPackageNames = new ArrayList<>(mPackages.keySet()); - - AsyncTask.execute(() -> { - if (hasOldPkg) { - mPermissionManager.revokeRuntimePermissionsIfGroupChanged(pkg, oldPkg, - allPackageNames); - } - if (hasPermissionDefinitionChanges) { - mPermissionManager.revokeRuntimePermissionsIfPermissionDefinitionChanged( - permissionsWithChangedDefinition, allPackageNames); - } - }); - } + mPermissionManager.onPackageAdded(pkg, (scanFlags & SCAN_AS_INSTANT_APP) != 0, oldPkg); } Trace.traceEnd(TRACE_TAG_PACKAGE_MANAGER); diff --git a/services/core/java/com/android/server/pm/permission/Permission.java b/services/core/java/com/android/server/pm/permission/Permission.java index 0245b28884ead..687e96c3c9dd2 100644 --- a/services/core/java/com/android/server/pm/permission/Permission.java +++ b/services/core/java/com/android/server/pm/permission/Permission.java @@ -399,8 +399,7 @@ public final class Permission { @NonNull public static Permission createOrUpdate(@Nullable Permission permission, @NonNull PermissionInfo permissionInfo, @NonNull AndroidPackage pkg, - @NonNull Collection permissionTrees, boolean isOverridingSystemPermission, - boolean chatty) { + @NonNull Collection permissionTrees, boolean isOverridingSystemPermission) { // Allow system apps to redefine non-system permissions boolean ownerChanged = false; if (permission != null && !Objects.equals(permission.mPermissionInfo.packageName, @@ -437,7 +436,7 @@ public final class Permission { permission.mPermissionInfo = permissionInfo; permission.mReconciled = true; permission.mUid = pkg.getUid(); - if (chatty) { + if (PackageManagerService.DEBUG_PACKAGE_SCANNING) { if (r == null) { r = new StringBuilder(256); } else { @@ -456,7 +455,7 @@ public final class Permission { + permissionInfo.packageName + " ignored: original from " + permission.mPermissionInfo.packageName); } - } else if (chatty) { + } else if (PackageManagerService.DEBUG_PACKAGE_SCANNING) { if (r == null) { r = new StringBuilder(256); } else { 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 6c03a28d03c77..092167d09a798 100644 --- a/services/core/java/com/android/server/pm/permission/PermissionManagerService.java +++ b/services/core/java/com/android/server/pm/permission/PermissionManagerService.java @@ -93,6 +93,7 @@ import android.content.pm.parsing.component.ParsedPermission; import android.content.pm.parsing.component.ParsedPermissionGroup; import android.content.pm.permission.SplitPermissionInfoParcelable; import android.metrics.LogMaker; +import android.os.AsyncTask; import android.os.Binder; import android.os.Build; import android.os.Debug; @@ -133,6 +134,7 @@ import com.android.internal.logging.MetricsLogger; import com.android.internal.logging.nano.MetricsProto.MetricsEvent; import com.android.internal.os.RoSystemProperties; import com.android.internal.util.ArrayUtils; +import com.android.internal.util.CollectionUtils; import com.android.internal.util.DumpUtils; import com.android.internal.util.IntPair; import com.android.internal.util.Preconditions; @@ -2363,14 +2365,9 @@ public class PermissionManagerService extends IPermissionManager.Stub { * * @param newPackage The new package that was installed * @param oldPackage The old package that was updated - * @param allPackageNames All package names - * @param permissionCallback Callback for permission changed */ - private void revokeRuntimePermissionsIfGroupChanged( - @NonNull AndroidPackage newPackage, - @NonNull AndroidPackage oldPackage, - @NonNull ArrayList allPackageNames, - @NonNull PermissionCallback permissionCallback) { + private void revokeRuntimePermissionsIfGroupChangedInternal(@NonNull AndroidPackage newPackage, + @NonNull AndroidPackage oldPackage) { final int numOldPackagePermissions = ArrayUtils.size(oldPackage.getPermissions()); final ArrayMap oldPermissionNameToGroupName = new ArrayMap<>(numOldPackagePermissions); @@ -2403,13 +2400,9 @@ public class PermissionManagerService extends IPermissionManager.Stub { if (newPermissionGroupName != null && !newPermissionGroupName.equals(oldPermissionGroupName)) { final int[] userIds = mUserManagerInt.getUserIds(); - final int numUserIds = userIds.length; - for (int userIdNum = 0; userIdNum < numUserIds; userIdNum++) { - final int userId = userIds[userIdNum]; - - final int numPackages = allPackageNames.size(); - for (int packageNum = 0; packageNum < numPackages; packageNum++) { - final String packageName = allPackageNames.get(packageNum); + mPackageManagerInt.forEachPackage(pkg -> { + final String packageName = pkg.getPackageName(); + for (final int userId : userIds) { final int permissionState = checkPermission(permissionName, packageName, userId); if (permissionState == PackageManager.PERMISSION_GRANTED) { @@ -2422,14 +2415,15 @@ public class PermissionManagerService extends IPermissionManager.Stub { try { revokeRuntimePermissionInternal(permissionName, packageName, - false, callingUid, userId, null, permissionCallback); + false, callingUid, userId, null, + mDefaultPermissionCallback); } catch (IllegalArgumentException e) { Slog.e(TAG, "Could not revoke " + permissionName + " from " + packageName, e); } } } - } + }); } } } @@ -2440,18 +2434,11 @@ public class PermissionManagerService extends IPermissionManager.Stub { * granted permissions must be revoked. * * @param permissionsToRevoke A list of permission names to revoke - * @param allPackageNames All package names - * @param permissionCallback Callback for permission changed */ - private void revokeRuntimePermissionsIfPermissionDefinitionChanged( - @NonNull List permissionsToRevoke, - @NonNull ArrayList allPackageNames, - @NonNull PermissionCallback permissionCallback) { - + private void revokeRuntimePermissionsIfPermissionDefinitionChangedInternal( + @NonNull List permissionsToRevoke) { final int[] userIds = mUserManagerInt.getUserIds(); final int numPermissions = permissionsToRevoke.size(); - final int numUserIds = userIds.length; - final int numPackages = allPackageNames.size(); final int callingUid = Binder.getCallingUid(); for (int permNum = 0; permNum < numPermissions; permNum++) { @@ -2462,15 +2449,14 @@ public class PermissionManagerService extends IPermissionManager.Stub { continue; } } - for (int userIdNum = 0; userIdNum < numUserIds; userIdNum++) { - final int userId = userIds[userIdNum]; - for (int packageNum = 0; packageNum < numPackages; packageNum++) { - final String packageName = allPackageNames.get(packageNum); - final int uid = mPackageManagerInt.getPackageUid(packageName, 0, userId); - if (uid < Process.FIRST_APPLICATION_UID) { - // do not revoke from system apps - continue; - } + mPackageManagerInt.forEachPackage(pkg -> { + final String packageName = pkg.getPackageName(); + final int appId = pkg.getUid(); + if (appId < Process.FIRST_APPLICATION_UID) { + // do not revoke from system apps + return; + } + for (final int userId : userIds) { final int permissionState = checkPermissionImpl(permName, packageName, userId); final int flags = getPermissionFlags(permName, packageName, userId); @@ -2480,6 +2466,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { | FLAG_PERMISSION_GRANTED_BY_ROLE; if (permissionState == PackageManager.PERMISSION_GRANTED && (flags & flagMask) == 0) { + final int uid = UserHandle.getUid(userId, appId); EventLog.writeEvent(0x534e4554, "154505240", uid, "Revoking permission " + permName + " from package " + packageName + " due to definition change"); @@ -2490,18 +2477,18 @@ public class PermissionManagerService extends IPermissionManager.Stub { + packageName + " due to definition change"); try { revokeRuntimePermissionInternal(permName, packageName, - false, callingUid, userId, null, permissionCallback); + false, callingUid, userId, null, mDefaultPermissionCallback); } catch (Exception e) { Slog.e(TAG, "Could not revoke " + permName + " from " + packageName, e); } } } - } + }); } } - private List addAllPermissions(AndroidPackage pkg, boolean chatty) { + private List addAllPermissionsInternal(@NonNull AndroidPackage pkg) { final int N = ArrayUtils.size(pkg.getPermissions()); ArrayList definitionChangedPermissions = new ArrayList<>(); for (int i=0; i= 0; i--) { + final String origName = pkg.getAdoptPermissions().get(i); + if (canAdoptPermissionsInternal(origName, pkg)) { + Slog.i(TAG, "Adopting permissions from " + origName + " to " + + pkg.getPackageName()); + synchronized (mLock) { + mRegistry.transferPermissions(origName, pkg.getPackageName()); + } + } + } } + + // Don't allow ephemeral applications to define new permissions groups. + if (isInstantApp) { + Slog.w(TAG, "Permission groups from package " + pkg.getPackageName() + + " ignored: instant apps cannot define new permission groups."); + } else { + addAllPermissionGroupsInternal(pkg); + } + + // If a permission has had its defining app changed, or it has had its protection + // upgraded, we need to revoke apps that hold it + final List permissionsWithChangedDefinition; + // Don't allow ephemeral applications to define new permissions. + if (isInstantApp) { + permissionsWithChangedDefinition = null; + Slog.w(TAG, "Permissions from package " + pkg.getPackageName() + + " ignored: instant apps cannot define new permissions."); + } else { + permissionsWithChangedDefinition = addAllPermissionsInternal(pkg); + } + + boolean hasOldPkg = oldPkg != null; + boolean hasPermissionDefinitionChanges = + !CollectionUtils.isEmpty(permissionsWithChangedDefinition); + if (hasOldPkg || hasPermissionDefinitionChanges) { + // We need to call revokeRuntimePermissionsIfGroupChanged async as permission + // revoke callbacks from this method might need to kill apps which need the + // mPackages lock on a different thread. This would dead lock. + AsyncTask.execute(() -> { + if (hasOldPkg) { + revokeRuntimePermissionsIfGroupChangedInternal(pkg, oldPkg); + } + if (hasPermissionDefinitionChanges) { + revokeRuntimePermissionsIfPermissionDefinitionChangedInternal( + permissionsWithChangedDefinition); + } + }); + } + } + + private boolean canAdoptPermissionsInternal(@NonNull String oldPackageName, + @NonNull AndroidPackage newPkg) { + final PackageSetting oldPs = mPackageManagerInt.getPackageSetting(oldPackageName); + if (oldPs == null) { + return false; + } + if (!oldPs.isSystem()) { + Slog.w(TAG, "Unable to update from " + oldPs.name + + " to " + newPkg.getPackageName() + + ": old package not in system partition"); + return false; + } + if (mPackageManagerInt.getPackage(oldPs.name) != null) { + Slog.w(TAG, "Unable to update from " + oldPs.name + + " to " + newPkg.getPackageName() + + ": old package still exists"); + return false; + } + return true; } private boolean canPropagatePermissionToInstantApp(@NonNull String permissionName) { @@ -4954,31 +5011,6 @@ public class PermissionManagerService extends IPermissionManager.Stub { return isPermissionsReviewRequiredInternal(packageName, userId); } - @Override - public void revokeRuntimePermissionsIfGroupChanged( - @NonNull AndroidPackage newPackage, - @NonNull AndroidPackage oldPackage, - @NonNull ArrayList allPackageNames) { - PermissionManagerService.this.revokeRuntimePermissionsIfGroupChanged(newPackage, - oldPackage, allPackageNames, mDefaultPermissionCallback); - } - - @Override - public void revokeRuntimePermissionsIfPermissionDefinitionChanged( - @NonNull List permissionsToRevoke, - @NonNull ArrayList allPackageNames) { - PermissionManagerService.this.revokeRuntimePermissionsIfPermissionDefinitionChanged( - permissionsToRevoke, allPackageNames, mDefaultPermissionCallback); - } - - @Override - public List addAllPermissions(AndroidPackage pkg, boolean chatty) { - return PermissionManagerService.this.addAllPermissions(pkg, chatty); - } - @Override - public void addAllPermissionGroups(AndroidPackage pkg, boolean chatty) { - PermissionManagerService.this.addAllPermissionGroups(pkg, chatty); - } @Override public void removeAllPermissions(AndroidPackage pkg, boolean chatty) { PermissionManagerService.this.removeAllPermissions(pkg, chatty); @@ -5313,9 +5345,10 @@ public class PermissionManagerService extends IPermissionManager.Stub { } @Override - public void transferPermissions(@NonNull String oldPackageName, - @NonNull String newPackageName) { - PermissionManagerService.this.transferPermissions(oldPackageName, newPackageName); + public void onPackageAdded(@NonNull AndroidPackage pkg, boolean isInstantApp, + @Nullable AndroidPackage oldPkg) { + Objects.requireNonNull(pkg); + onPackageAddedInternal(pkg, isInstantApp, oldPkg); } @Override diff --git a/services/core/java/com/android/server/pm/permission/PermissionManagerServiceInternal.java b/services/core/java/com/android/server/pm/permission/PermissionManagerServiceInternal.java index df9d0d397c56e..11b6f311b9f03 100644 --- a/services/core/java/com/android/server/pm/permission/PermissionManagerServiceInternal.java +++ b/services/core/java/com/android/server/pm/permission/PermissionManagerServiceInternal.java @@ -276,42 +276,6 @@ public abstract class PermissionManagerServiceInternal extends PermissionManager //@SystemApi(client = SystemApi.Client.SYSTEM_SERVER) public abstract void resetAllRuntimePermissions(@UserIdInt int userId); - /** - * We might auto-grant permissions if any permission of the group is already granted. Hence if - * the group of a granted permission changes we need to revoke it to avoid having permissions of - * the new group auto-granted. - * - * @param newPackage The new package that was installed - * @param oldPackage The old package that was updated - * @param allPackageNames All packages - */ - public abstract void revokeRuntimePermissionsIfGroupChanged( - @NonNull AndroidPackage newPackage, - @NonNull AndroidPackage oldPackage, - @NonNull ArrayList allPackageNames); - - /** - * Some permissions might have been owned by a non-system package, and the system then defined - * said permission. Some other permissions may one have been install permissions, but are now - * runtime or higher. These permissions should be revoked. - * - * @param permissionsToRevoke A list of permission names to revoke - * @param allPackageNames All packages - */ - public abstract void revokeRuntimePermissionsIfPermissionDefinitionChanged( - @NonNull List permissionsToRevoke, - @NonNull ArrayList allPackageNames); - - /** - * Add all permissions in the given package. - *

- * NOTE: argument {@code groupTEMP} is temporary until mPermissionGroups is moved to - * the permission settings. - * - * @return A list of BasePermissions that were updated, and need to be revoked from packages - */ - public abstract List addAllPermissions(@NonNull AndroidPackage pkg, boolean chatty); - public abstract void addAllPermissionGroups(@NonNull AndroidPackage pkg, boolean chatty); public abstract void removeAllPermissions(@NonNull AndroidPackage pkg, boolean chatty); /** @@ -578,10 +542,15 @@ public abstract class PermissionManagerServiceInternal extends PermissionManager @NonNull LegacyPermissionSettings legacyPermissionSettings); /** - * Transfers ownership of permissions from one package to another. + * Callback when a package has been added. + * + * @param pkg the added package + * @param isInstantApp whether the added package is an instant app + * @param oldPkg the old package, or {@code null} if none */ - public abstract void transferPermissions(@NonNull String oldPackageName, - @NonNull String newPackageName); + //@SystemApi(client = SystemApi.Client.SYSTEM_SERVER) + public abstract void onPackageAdded(@NonNull AndroidPackage pkg, boolean isInstantApp, + @Nullable AndroidPackage oldPkg); /** * Check whether a permission can be propagated to instant app.