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)

4 files changed, 134 insertions, 12 deletions

diff --git a/services/core/java/com/android/server/pm/PackageManagerService.java b/services/core/java/com/android/server/pm/PackageManagerService.java
index 0446d8ea726d..c55ab9a3a2e9 100644
--- a/services/core/java/com/android/server/pm/PackageManagerService.java
+++ b/services/core/java/com/android/server/pm/PackageManagerService.java
@@ -284,6 +284,7 @@ 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;
@@ -11638,12 +11639,17 @@ public class PackageManagerService extends IPackageManager.Stub
                 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);
             }
 
             N = pkg.instrumentation.size();
@@ -11687,7 +11693,10 @@ public class PackageManagerService extends IPackageManager.Stub
                 }
             }
 
-            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.
@@ -11698,9 +11707,17 @@ public class PackageManagerService extends IPackageManager.Stub
                 // 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 1d002efc546f..6b550f2b9a94 100644
--- a/services/core/java/com/android/server/pm/permission/BasePermission.java
+++ b/services/core/java/com/android/server/pm/permission/BasePermission.java
@@ -81,6 +81,8 @@ public final class BasePermission {
 
     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
@@ -133,6 +135,11 @@ public final class BasePermission {
     public Signature[] getSourceSignatures() {
         return sourcePackageSetting.getSignatures();
     }
+
+    public boolean isPermissionDefinitionChanged() {
+        return mPermissionDefinitionChanged;
+    }
+
     public int getType() {
         return type;
     }
@@ -150,6 +157,10 @@ public final class BasePermission {
         this.sourcePackageSetting = sourcePackageSetting;
     }
 
+    public void setPermissionDefinitionChanged(boolean shouldOverride) {
+        mPermissionDefinitionChanged = shouldOverride;
+    }
+
     public int[] computeGids(int userId) {
         if (perUser) {
             final int[] userGids = new int[gids.length];
@@ -286,6 +297,7 @@ public final class BasePermission {
             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());
@@ -301,6 +313,7 @@ public final class BasePermission {
                     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;
                 }
             }
@@ -308,6 +321,7 @@ public final class BasePermission {
         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
@@ -351,6 +365,11 @@ public final class BasePermission {
         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/PermissionManagerInternal.java b/services/core/java/com/android/server/pm/permission/PermissionManagerInternal.java
index a042fedf8b47..185e0e1fda5f 100644
--- a/services/core/java/com/android/server/pm/permission/PermissionManagerInternal.java
+++ b/services/core/java/com/android/server/pm/permission/PermissionManagerInternal.java
@@ -107,13 +107,18 @@ public abstract class PermissionManagerInternal {
             @NonNull ArrayList<String> allPackageNames,
             @NonNull PermissionCallback permissionCallback);
 
+    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.
      */
-    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,
@@ -189,4 +194,4 @@ public abstract class PermissionManagerInternal {
 
     /** HACK HACK methods to allow for partial migration of data to the PermissionManager class */
     public abstract @Nullable BasePermission getPermissionTEMP(@NonNull String permName);
-}
\ No newline at end of file
+}
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 c51a72406b53..79b2636481b3 100644
--- a/services/core/java/com/android/server/pm/permission/PermissionManagerService.java
+++ b/services/core/java/com/android/server/pm/permission/PermissionManagerService.java
@@ -469,8 +469,74 @@ public class PermissionManagerService {
         }
     }
 
-    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,
+                            callingUid, userId);
+                    final int flags = getPermissionFlags(permName, packageName, callingUid,
+                             userId);
+                    final int flagMask = PackageManager.FLAG_PERMISSION_SYSTEM_FIXED
+                            | PackageManager.FLAG_PERMISSION_POLICY_FIXED
+                            | PackageManager.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, callingUid, 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);
 
@@ -492,19 +558,24 @@ public class PermissionManagerService {
                     }
                 }
 
+                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) {
@@ -2064,9 +2135,19 @@ public class PermissionManagerService {
             PermissionManagerService.this.revokeRuntimePermissionsIfGroupChanged(newPackage,
                     oldPackage, allPackageNames, permissionCallback);
         }
+
+        @Override
+        public void revokeRuntimePermissionsIfPermissionDefinitionChanged(
+                @NonNull List<String> permissionsToRevoke,
+                @NonNull ArrayList<String> allPackageNames,
+                @NonNull PermissionCallback permissionCallback) {
+            PermissionManagerService.this.revokeRuntimePermissionsIfPermissionDefinitionChanged(
+                    permissionsToRevoke, allPackageNames, permissionCallback);
+        }
+
         @Override
-        public void addAllPermissions(Package pkg, boolean chatty) {
-            PermissionManagerService.this.addAllPermissions(pkg, chatty);
+        public List<String> addAllPermissions(Package pkg, boolean chatty) {
+            return PermissionManagerService.this.addAllPermissions(pkg, chatty);
         }
         @Override
         public void addAllPermissionGroups(Package pkg, boolean chatty) {