From ed2ff0f5da52fc81aa03e01dbcd0baa45103026a Mon Sep 17 00:00:00 2001 From: Nate Myren Date: Mon, 30 Nov 2020 12:06:26 -0800 Subject: [PATCH] Revoke storage on SDK downgrade or new full storage request When an app downgrades from above API 28 to below, or when an app begins requesting full storage in api 29, revoke storage permissions Test: Atest StorageEscalationTest Bug: 171430330 Merged-In: Ia6bd5658b77a26ab15fea013ec875050457473e0 Change-Id: Ia6bd5658b77a26ab15fea013ec875050457473e0 --- .../server/pm/PackageManagerService.java | 2 + .../permission/PermissionManagerService.java | 65 +++++++++++++++++++ .../PermissionManagerServiceInternal.java | 11 ++++ 3 files changed, 78 insertions(+) diff --git a/services/core/java/com/android/server/pm/PackageManagerService.java b/services/core/java/com/android/server/pm/PackageManagerService.java index 84213092081d9..40ba5c9d9b4e9 100644 --- a/services/core/java/com/android/server/pm/PackageManagerService.java +++ b/services/core/java/com/android/server/pm/PackageManagerService.java @@ -12330,6 +12330,8 @@ public class PackageManagerService extends IPackageManager.Stub if (hasOldPkg) { mPermissionManager.revokeRuntimePermissionsIfGroupChanged(pkg, oldPkg, allPackageNames, mPermissionCallback); + mPermissionManager.revokeStoragePermissionsIfScopeExpanded(pkg, oldPkg, + mPermissionCallback); } if (hasPermissionDefinitionChanges) { mPermissionManager.revokeRuntimePermissionsIfPermissionDefinitionChanged( 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 2b3a4700a18e4..79c86c167cf9e 100644 --- a/services/core/java/com/android/server/pm/permission/PermissionManagerService.java +++ b/services/core/java/com/android/server/pm/permission/PermissionManagerService.java @@ -150,6 +150,9 @@ public class PermissionManagerService { private static final int USER_PERMISSION_FLAGS = FLAG_PERMISSION_USER_SET | FLAG_PERMISSION_USER_FIXED; + /** All storage permissions */ + private static final List STORAGE_PERMISSIONS = new ArrayList<>(); + /** If the permission of the value is granted, so is the key */ private static final Map FULLER_PERMISSION_MAP = new HashMap<>(); @@ -158,6 +161,9 @@ public class PermissionManagerService { Manifest.permission.ACCESS_FINE_LOCATION); FULLER_PERMISSION_MAP.put(Manifest.permission.INTERACT_ACROSS_USERS, Manifest.permission.INTERACT_ACROSS_USERS_FULL); + STORAGE_PERMISSIONS.add(Manifest.permission.READ_EXTERNAL_STORAGE); + STORAGE_PERMISSIONS.add(Manifest.permission.WRITE_EXTERNAL_STORAGE); + STORAGE_PERMISSIONS.add(Manifest.permission.ACCESS_MEDIA_LOCATION); } /** Lock to protect internal data access */ @@ -589,6 +595,50 @@ public class PermissionManagerService { return protectionLevel; } + /** + * If the app is updated, and has scoped storage permissions, then it is possible that the + * app updated in an attempt to get unscoped storage. If so, revoke all storage permissions. + * @param newPackage The new package that was installed + * @param oldPackage The old package that was updated + */ + private void revokeStoragePermissionsIfScopeExpanded( + @NonNull PackageParser.Package newPackage, + @NonNull PackageParser.Package oldPackage, + @NonNull PermissionCallback permissionCallback) { + boolean downgradedSdk = oldPackage.applicationInfo.targetSdkVersion >= Build.VERSION_CODES.Q + && newPackage.applicationInfo.targetSdkVersion < Build.VERSION_CODES.Q; + boolean upgradedSdk = oldPackage.applicationInfo.targetSdkVersion < Build.VERSION_CODES.Q + && newPackage.applicationInfo.targetSdkVersion >= Build.VERSION_CODES.Q; + boolean newlyRequestsLegacy = !upgradedSdk + && !oldPackage.applicationInfo.hasRequestedLegacyExternalStorage() + && newPackage.applicationInfo.hasRequestedLegacyExternalStorage(); + + if (!newlyRequestsLegacy && !downgradedSdk) { + return; + } + + final int callingUid = Binder.getCallingUid(); + final int userId = UserHandle.getUserId(newPackage.applicationInfo.uid); + int numRequestedPermissions = newPackage.requestedPermissions.size(); + for (int i = 0; i < numRequestedPermissions; i++) { + PermissionInfo permInfo = getPermissionInfo(newPackage.requestedPermissions.get(i), + newPackage.packageName, 0, callingUid); + if (permInfo == null || !STORAGE_PERMISSIONS.contains(permInfo.name)) { + continue; + } + + EventLog.writeEvent(0x534e4554, "171430330", newPackage.applicationInfo.uid, + "Revoking permission " + permInfo.name + " from package " + + newPackage.packageName + " as either the sdk downgraded " + + downgradedSdk + " or newly requested legacy full storage " + + newlyRequestsLegacy); + + revokeRuntimePermission(permInfo.name, newPackage.packageName, + false, userId, permissionCallback); + } + + } + /** * 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 @@ -3105,6 +3155,21 @@ public class PermissionManagerService { public boolean isPermissionsReviewRequired(@NonNull Package pkg, @UserIdInt int userId) { return PermissionManagerService.this.isPermissionsReviewRequired(pkg, userId); } + + /** + * If the app is updated, and has scoped storage permissions, then it is possible that the + * app updated in an attempt to get unscoped storage. If so, revoke all storage permissions. + * @param newPackage The new package that was installed + * @param oldPackage The old package that was updated + */ + public void revokeStoragePermissionsIfScopeExpanded( + @NonNull PackageParser.Package newPackage, + @NonNull PackageParser.Package oldPackage, + @NonNull PermissionCallback permissionCallback) { + PermissionManagerService.this.revokeStoragePermissionsIfScopeExpanded(newPackage, + oldPackage, permissionCallback); + } + @Override public void revokeRuntimePermissionsIfGroupChanged( @NonNull PackageParser.Package newPackage, 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 40ff4a2317514..46fa4ffdbd451 100644 --- a/services/core/java/com/android/server/pm/permission/PermissionManagerServiceInternal.java +++ b/services/core/java/com/android/server/pm/permission/PermissionManagerServiceInternal.java @@ -126,6 +126,17 @@ public abstract class PermissionManagerServiceInternal extends PermissionManager @NonNull ArrayList allPackageNames, @NonNull PermissionCallback permissionCallback); + /** + * If the app is updated, and has scoped storage permissions, then it is possible that the + * app updated in an attempt to get unscoped storage. If so, revoke all storage permissions. + * @param newPackage The new package that was installed + * @param oldPackage The old package that was updated + */ + public abstract void revokeStoragePermissionsIfScopeExpanded( + @NonNull PackageParser.Package newPackage, + @NonNull PackageParser.Package oldPackage, + @NonNull PermissionCallback permissionCallback); + /** * Add all permissions in the given package. *