Ensure permissions are revoked on state changes
If a permission owner changes, or a permission level is upgraded, revoke
the permission from all packages
Test: Manual
Bug: 154505240
Merged-In: I0dec9eb7c2fecd3147e33e04d3f79f6dffcf7721
Change-Id: I0dec9eb7c2fecd3147e33e04d3f79f6dffcf7721
(cherry picked from commit a28931a09814a89e1c55816c794c1e1f20dc0c91)
(cherry picked from commit bc71724034e5879557d3b05d2efb548e4db470be)
diff --git a/services/core/java/com/android/server/pm/PackageManagerService.java b/services/core/java/com/android/server/pm/PackageManagerService.java
index eaa9f6a..4eeef5d 100644
--- a/services/core/java/com/android/server/pm/PackageManagerService.java
+++ b/services/core/java/com/android/server/pm/PackageManagerService.java
@@ -287,6 +287,7 @@
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;
@@ -12244,12 +12245,17 @@
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<String> 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.packageName
+ " ignored: instant apps cannot define new permissions.");
} else {
- mPermissionManager.addAllPermissions(pkg, chatty);
+ permissionsWithChangedDefinition =
+ mPermissionManager.addAllPermissions(pkg, chatty);
}
int collectionSize = pkg.instrumentation.size();
@@ -12294,7 +12300,10 @@
}
}
- if (oldPkg != null) {
+ 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.
@@ -12305,9 +12314,17 @@
// won't be granted yet, hence new packages are no problem.
final ArrayList<String> allPackageNames = new ArrayList<>(mPackages.keySet());
- AsyncTask.execute(() ->
+ AsyncTask.execute(() -> {
+ if (hasOldPkg) {
mPermissionManager.revokeRuntimePermissionsIfGroupChanged(pkg, oldPkg,
- allPackageNames, mPermissionCallback));
+ allPackageNames, mPermissionCallback);
+ }
+ if (hasPermissionDefinitionChanges) {
+ mPermissionManager.revokeRuntimePermissionsIfPermissionDefinitionChanged(
+ permissionsWithChangedDefinition, allPackageNames,
+ mPermissionCallback);
+ }
+ });
}
}
diff --git a/services/core/java/com/android/server/pm/permission/BasePermission.java b/services/core/java/com/android/server/pm/permission/BasePermission.java
index 6d22faa..63753ba 100644
--- a/services/core/java/com/android/server/pm/permission/BasePermission.java
+++ b/services/core/java/com/android/server/pm/permission/BasePermission.java
@@ -82,6 +82,8 @@
final @PermissionType int type;
+ private boolean mPermissionDefinitionChanged;
+
String sourcePackageName;
// TODO: Can we get rid of this? Seems we only use some signature info from the setting
@@ -134,6 +136,11 @@
public Signature[] getSourceSignatures() {
return sourcePackageSetting.getSignatures();
}
+
+ public boolean isPermissionDefinitionChanged() {
+ return mPermissionDefinitionChanged;
+ }
+
public int getType() {
return type;
}
@@ -151,6 +158,10 @@
this.sourcePackageSetting = sourcePackageSetting;
}
+ public void setPermissionDefinitionChanged(boolean shouldOverride) {
+ mPermissionDefinitionChanged = shouldOverride;
+ }
+
public int[] computeGids(int userId) {
if (perUser) {
final int[] userGids = new int[gids.length];
@@ -330,6 +341,7 @@
boolean chatty) {
final PackageSettingBase pkgSetting = (PackageSettingBase) pkg.mExtras;
// Allow system apps to redefine non-system permissions
+ boolean ownerChanged = false;
if (bp != null && !Objects.equals(bp.sourcePackageName, p.info.packageName)) {
final boolean currentOwnerIsSystem = (bp.perm != null
&& bp.perm.owner.isSystem());
@@ -345,6 +357,7 @@
String msg = "New decl " + p.owner + " of permission "
+ p.info.name + " is system; overriding " + bp.sourcePackageName;
PackageManagerService.reportSettingsProblem(Log.WARN, msg);
+ ownerChanged = true;
bp = null;
}
}
@@ -352,6 +365,7 @@
if (bp == null) {
bp = new BasePermission(p.info.name, p.info.packageName, TYPE_NORMAL);
}
+ boolean wasNormal = bp.isNormal();
StringBuilder r = null;
if (bp.perm == null) {
if (bp.sourcePackageName == null
@@ -395,6 +409,11 @@
if (bp.perm == p) {
bp.protectionLevel = p.info.protectionLevel;
}
+ if (bp.isRuntime() && (ownerChanged || wasNormal)) {
+ // If this is a runtime permission and the owner has changed, or this was a normal
+ // permission, then permission state should be cleaned up
+ bp.mPermissionDefinitionChanged = true;
+ }
if (PackageManagerService.DEBUG_PACKAGE_SCANNING && r != null) {
Log.d(TAG, " Permissions: " + r);
}
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 b3bee6f..2b3a470 100644
--- a/services/core/java/com/android/server/pm/permission/PermissionManagerService.java
+++ b/services/core/java/com/android/server/pm/permission/PermissionManagerService.java
@@ -666,8 +666,74 @@
}
}
- private void addAllPermissions(PackageParser.Package pkg, boolean chatty) {
- final int N = pkg.permissions.size();
+ /**
+ * If permissions are upgraded to runtime, or their owner changes to the system, then any
+ * 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<String> permissionsToRevoke,
+ @NonNull ArrayList<String> allPackageNames,
+ @NonNull PermissionCallback permissionCallback) {
+
+ 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++) {
+ String permName = permissionsToRevoke.get(permNum);
+ BasePermission bp = mSettings.getPermission(permName);
+ if (bp == null || !bp.isRuntime()) {
+ 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;
+ }
+ final int permissionState = checkPermission(permName, packageName,
+ Binder.getCallingUid(), userId);
+ final int flags = getPermissionFlags(permName, packageName,
+ Binder.getCallingUid(), userId);
+ final int flagMask = FLAG_PERMISSION_SYSTEM_FIXED
+ | FLAG_PERMISSION_POLICY_FIXED
+ | FLAG_PERMISSION_GRANTED_BY_DEFAULT;
+ if (permissionState == PackageManager.PERMISSION_GRANTED
+ && (flags & flagMask) == 0) {
+ EventLog.writeEvent(0x534e4554, "154505240", uid,
+ "Revoking permission " + permName + " from package "
+ + packageName + " due to definition change");
+ EventLog.writeEvent(0x534e4554, "168319670", uid,
+ "Revoking permission " + permName + " from package "
+ + packageName + " due to definition change");
+ Slog.e(TAG, "Revoking permission " + permName + " from package "
+ + packageName + " due to definition change");
+ try {
+ revokeRuntimePermission(permName, packageName,
+ false, userId, permissionCallback);
+ } catch (Exception e) {
+ Slog.e(TAG, "Could not revoke " + permName + " from "
+ + packageName, e);
+ }
+ }
+ }
+ }
+ bp.setPermissionDefinitionChanged(false);
+ }
+ }
+
+ private List<String> addAllPermissions(PackageParser.Package pkg, boolean chatty) {
+ final int N = ArrayUtils.size(pkg.permissions);
+ ArrayList<String> definitionChangedPermissions = new ArrayList<>();
for (int i=0; i<N; i++) {
PackageParser.Permission p = pkg.permissions.get(i);
@@ -689,19 +755,24 @@
}
}
+ final BasePermission bp;
if (p.tree) {
- final BasePermission bp = BasePermission.createOrUpdate(
+ bp = BasePermission.createOrUpdate(
mSettings.getPermissionTreeLocked(p.info.name), p, pkg,
mSettings.getAllPermissionTreesLocked(), chatty);
mSettings.putPermissionTreeLocked(p.info.name, bp);
} else {
- final BasePermission bp = BasePermission.createOrUpdate(
+ bp = BasePermission.createOrUpdate(
mSettings.getPermissionLocked(p.info.name),
p, pkg, mSettings.getAllPermissionTreesLocked(), chatty);
mSettings.putPermissionLocked(p.info.name, bp);
}
+ if (bp.isPermissionDefinitionChanged()) {
+ definitionChangedPermissions.add(p.info.name);
+ }
}
}
+ return definitionChangedPermissions;
}
private void addAllPermissionGroups(PackageParser.Package pkg, boolean chatty) {
@@ -3043,9 +3114,19 @@
PermissionManagerService.this.revokeRuntimePermissionsIfGroupChanged(newPackage,
oldPackage, allPackageNames, permissionCallback);
}
+
@Override
- public void addAllPermissions(Package pkg, boolean chatty) {
- PermissionManagerService.this.addAllPermissions(pkg, chatty);
+ public void revokeRuntimePermissionsIfPermissionDefinitionChanged(
+ @NonNull List<String> permissionsToRevoke,
+ @NonNull ArrayList<String> allPackageNames,
+ @NonNull PermissionCallback permissionCallback) {
+ PermissionManagerService.this.revokeRuntimePermissionsIfPermissionDefinitionChanged(
+ permissionsToRevoke, allPackageNames, permissionCallback);
+ }
+
+ @Override
+ public List<String> addAllPermissions(Package pkg, boolean chatty) {
+ return PermissionManagerService.this.addAllPermissions(pkg, chatty);
}
@Override
public void addAllPermissionGroups(Package pkg, boolean chatty) {
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 e5e9598..40ff4a2 100644
--- a/services/core/java/com/android/server/pm/permission/PermissionManagerServiceInternal.java
+++ b/services/core/java/com/android/server/pm/permission/PermissionManagerServiceInternal.java
@@ -114,12 +114,27 @@
@NonNull PermissionCallback permissionCallback);
/**
+ * 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<String> permissionsToRevoke,
+ @NonNull ArrayList<String> allPackageNames,
+ @NonNull PermissionCallback permissionCallback);
+
+ /**
* Add all permissions in the given package.
* <p>
* 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 void addAllPermissions(@NonNull PackageParser.Package pkg, boolean chatty);
+ public abstract List<String> addAllPermissions(@NonNull PackageParser.Package pkg, boolean chatty);
public abstract void addAllPermissionGroups(@NonNull PackageParser.Package pkg, boolean chatty);
public abstract void removeAllPermissions(@NonNull PackageParser.Package pkg, boolean chatty);
public abstract boolean addDynamicPermission(@NonNull PermissionInfo info, boolean async,
