From e391b19376889ddc6817be94741a6bc2d089bb39 Mon Sep 17 00:00:00 2001 From: Hai Zhang Date: Thu, 4 Feb 2021 01:06:36 -0800 Subject: [PATCH] Fix privapp permission allowlisting. The refactoring in S unintendedly introduced a change that allowed signature|privileged to be granted to platform-signed privileged apps without being in the allowlist XML, so we should revert to the old behavior. The refactoring was done in the hope that we can have one step that handles privileged permission granting. However upon retrospection, we have to do this in two separate steps because the privapp permission allowlist should always be enforced first. This change reverts to the old behavior by looking at both the current code and the R source code, then extracts the privapp permission enforcement as a separate step that happens first, to be used by both signature and the new internal permission protection. Also simplified the check about privileged permission, because vendor privileged permission is always a privileged permission, as made sure in PermissionInfo.fixProtectionLevel(). In the mean time, added the missing privapp permission allowlist entry for Settings and RESTART_WIFI_SUBSYSTEM for device to boot. Fixes: 179309876 Test: manual Change-Id: I93cfe7a4621fc5ac65229d42c7a8ebd825ae8ae5 --- .../internal/util/CollectionUtils.java | 7 + data/etc/com.android.settings.xml | 1 + .../permission/PermissionManagerService.java | 205 +++++++++--------- 3 files changed, 116 insertions(+), 97 deletions(-) diff --git a/core/java/com/android/internal/util/CollectionUtils.java b/core/java/com/android/internal/util/CollectionUtils.java index 488b18db66d69..c6a040c162ab3 100644 --- a/core/java/com/android/internal/util/CollectionUtils.java +++ b/core/java/com/android/internal/util/CollectionUtils.java @@ -46,6 +46,13 @@ import java.util.stream.Stream; public class CollectionUtils { private CollectionUtils() { /* cannot be instantiated */ } + /** + * @see Collection#contains(Object) + */ + public static boolean contains(@Nullable Collection collection, T element) { + return collection != null && collection.contains(element); + } + /** * Returns a list of items from the provided list that match the given condition. * diff --git a/data/etc/com.android.settings.xml b/data/etc/com.android.settings.xml index e473c55ce0108..3fdb0da808758 100644 --- a/data/etc/com.android.settings.xml +++ b/data/etc/com.android.settings.xml @@ -58,5 +58,6 @@ + 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 aff871118a347..9d8a348d2779f 100644 --- a/services/core/java/com/android/server/pm/permission/PermissionManagerService.java +++ b/services/core/java/com/android/server/pm/permission/PermissionManagerService.java @@ -2590,6 +2590,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { boolean runtimePermissionsRevoked = false; int[] updatedUserIds = EMPTY_INT_ARRAY; + ArraySet isPrivilegedPermissionAllowlisted = null; ArraySet shouldGrantSignaturePermission = null; ArraySet shouldGrantInternalPermission = null; final List requestedPermissions = pkg.getRequestedPermissions(); @@ -2604,7 +2605,14 @@ public class PermissionManagerService extends IPermissionManager.Stub { if (permission == null) { continue; } - if (permission.isSignature() && (shouldGrantSignaturePermission(pkg, permission) + if (permission.isPrivileged() + && checkPrivilegedPermissionAllowlist(pkg, ps, permission)) { + if (isPrivilegedPermissionAllowlisted == null) { + isPrivilegedPermissionAllowlisted = new ArraySet<>(); + } + isPrivilegedPermissionAllowlisted.add(permissionName); + } + if (permission.isSignature() && (shouldGrantPermissionBySignature(pkg, permission) || shouldGrantPermissionByProtectionFlags(pkg, ps, permission))) { if (shouldGrantSignaturePermission == null) { shouldGrantSignaturePermission = new ArraySet<>(); @@ -2830,13 +2838,17 @@ public class PermissionManagerService extends IPermissionManager.Stub { if ((bp.isNormal() && shouldGrantNormalPermission) || (bp.isSignature() - && ((shouldGrantSignaturePermission != null - && shouldGrantSignaturePermission.contains(permName)) + && (!bp.isPrivileged() || CollectionUtils.contains( + isPrivilegedPermissionAllowlisted, permName)) + && (CollectionUtils.contains(shouldGrantSignaturePermission, + permName) || ((bp.isDevelopment() || bp.isRole()) && origState.isPermissionGranted(permName)))) || (bp.isInternal() - && ((shouldGrantInternalPermission != null - && shouldGrantInternalPermission.contains(permName)) + && (!bp.isPrivileged() || CollectionUtils.contains( + isPrivilegedPermissionAllowlisted, permName)) + && (CollectionUtils.contains(shouldGrantInternalPermission, + permName) || ((bp.isDevelopment() || bp.isRole()) && origState.isPermissionGranted(permName))))) { // Grant an install permission. @@ -3343,7 +3355,92 @@ public class PermissionManagerService extends IPermissionManager.Stub { return allowed; } - private boolean shouldGrantSignaturePermission(@NonNull AndroidPackage pkg, + private boolean checkPrivilegedPermissionAllowlist(@NonNull AndroidPackage pkg, + @NonNull PackageSetting packageSetting, @NonNull Permission permission) { + if (RoSystemProperties.CONTROL_PRIVAPP_PERMISSIONS_DISABLE) { + return true; + } + final String packageName = pkg.getPackageName(); + if (Objects.equals(packageName, PLATFORM_PACKAGE_NAME)) { + return true; + } + if (!pkg.isPrivileged()) { + return true; + } + if (!Objects.equals(permission.getPackageName(), PLATFORM_PACKAGE_NAME)) { + return true; + } + final String permissionName = permission.getName(); + if (isInSystemConfigPrivAppPermissions(pkg, permissionName)) { + return true; + } + // Only enforce the allowlist on boot + if (!mSystemReady + // Updated system apps do not need to be allowlisted + && !packageSetting.getPkgState().isUpdatedSystemApp()) { + final ApexManager apexManager = ApexManager.getInstance(); + final String containingApexPackageName = + apexManager.getActiveApexPackageNameContainingPackage(packageName); + final boolean isInUpdatedApex = containingApexPackageName != null + && !apexManager.isFactory(apexManager.getPackageInfo(containingApexPackageName, + MATCH_ACTIVE_PACKAGE)); + // Apps that are in updated apexs' do not need to be allowlisted + if (!isInUpdatedApex) { + // it's only a reportable violation if the permission isn't explicitly + // denied + if (isInSystemConfigPrivAppDenyPermissions(pkg, permissionName)) { + return false; + } + Slog.w(TAG, "Privileged permission " + permissionName + " for package " + + packageName + " (" + pkg.getPath() + + ") not in privapp-permissions allowlist"); + if (RoSystemProperties.CONTROL_PRIVAPP_PERMISSIONS_ENFORCE) { + synchronized (mLock) { + if (mPrivappPermissionsViolations == null) { + mPrivappPermissionsViolations = new ArraySet<>(); + } + mPrivappPermissionsViolations.add(packageName + " (" + pkg.getPath() + "): " + + permissionName); + } + } + } + } + return !RoSystemProperties.CONTROL_PRIVAPP_PERMISSIONS_ENFORCE; + } + + private boolean isInSystemConfigPrivAppPermissions(@NonNull AndroidPackage pkg, + @NonNull String permission) { + final SystemConfig systemConfig = SystemConfig.getInstance(); + final Set permissions; + if (pkg.isVendor()) { + permissions = systemConfig.getVendorPrivAppPermissions(pkg.getPackageName()); + } else if (pkg.isProduct()) { + permissions = systemConfig.getProductPrivAppPermissions(pkg.getPackageName()); + } else if (pkg.isSystemExt()) { + permissions = systemConfig.getSystemExtPrivAppPermissions(pkg.getPackageName()); + } else { + permissions = systemConfig.getPrivAppPermissions(pkg.getPackageName()); + } + return CollectionUtils.contains(permissions, permission); + } + + private boolean isInSystemConfigPrivAppDenyPermissions(@NonNull AndroidPackage pkg, + @NonNull String permission) { + final SystemConfig systemConfig = SystemConfig.getInstance(); + final Set permissions; + if (pkg.isVendor()) { + permissions = systemConfig.getVendorPrivAppDenyPermissions(pkg.getPackageName()); + } else if (pkg.isProduct()) { + permissions = systemConfig.getProductPrivAppDenyPermissions(pkg.getPackageName()); + } else if (pkg.isSystemExt()) { + permissions = systemConfig.getSystemExtPrivAppDenyPermissions(pkg.getPackageName()); + } else { + permissions = systemConfig.getPrivAppDenyPermissions(pkg.getPackageName()); + } + return CollectionUtils.contains(permissions, permission); + } + + private boolean shouldGrantPermissionBySignature(@NonNull AndroidPackage pkg, @NonNull Permission bp) { // expect single system package String systemPackageName = ArrayUtils.firstOrNull(mPackageManagerInt.getKnownPackageNames( @@ -3373,8 +3470,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { private boolean shouldGrantPermissionByProtectionFlags(@NonNull AndroidPackage pkg, @NonNull PackageSetting pkgSetting, @NonNull Permission bp) { boolean allowed = false; - final boolean isVendorPrivilegedPermission = bp.isVendorPrivileged(); - final boolean isPrivilegedPermission = bp.isPrivileged() || isVendorPrivilegedPermission; + final boolean isPrivilegedPermission = bp.isPrivileged(); final boolean isOemPermission = bp.isOem(); if (!allowed && (isPrivilegedPermission || isOemPermission) && pkg.isSystem()) { final String permissionName = bp.getName(); @@ -3386,19 +3482,18 @@ public class PermissionManagerService extends IPermissionManager.Stub { final AndroidPackage disabledPkg = disabledPs == null ? null : disabledPs.pkg; if (disabledPkg != null && disabledPkg.getRequestedPermissions().contains( permissionName)) { - allowed = (isPrivilegedPermission && canGrantPrivilegedPermission(disabledPkg, - true, bp)) || (isOemPermission && canGrantOemPermission(disabledPkg, + allowed = (isPrivilegedPermission && disabledPkg.isPrivileged()) + || (isOemPermission && canGrantOemPermission(disabledPkg, permissionName)); } } else { - allowed = (isPrivilegedPermission && canGrantPrivilegedPermission(pkg, false, bp)) + allowed = (isPrivilegedPermission && pkg.isPrivileged()) || (isOemPermission && canGrantOemPermission(pkg, permissionName)); } // In any case, don't grant a privileged permission to privileged vendor apps, if // the permission's protectionLevel does not have the extra 'vendorPrivileged' // flag. - if (allowed && isPrivilegedPermission && !isVendorPrivilegedPermission - && pkg.isVendor()) { + if (allowed && isPrivilegedPermission && !bp.isVendorPrivileged() && pkg.isVendor()) { Slog.w(TAG, "Permission " + permissionName + " cannot be granted to privileged vendor apk " + pkg.getPackageName() + " because it isn't a 'vendorPrivileged' permission."); @@ -3536,90 +3631,6 @@ public class PermissionManagerService extends IPermissionManager.Stub { return mPackageManagerInt.getPackageSetting(sourcePackageName); } - private boolean canGrantPrivilegedPermission(@NonNull AndroidPackage pkg, - boolean isUpdatedSystemApp, @NonNull Permission permission) { - if (!pkg.isPrivileged()) { - return false; - } - final boolean isPlatformPermission = PLATFORM_PACKAGE_NAME.equals( - permission.getPackageName()); - if (!isPlatformPermission) { - return true; - } - if (RoSystemProperties.CONTROL_PRIVAPP_PERMISSIONS_DISABLE) { - return true; - } - final String permissionName = permission.getName(); - if (isInSystemConfigPrivAppPermissions(pkg, permissionName)) { - return true; - } - // Only enforce the allowlist on boot - if (!mSystemReady - // Updated system apps do not need to be allowlisted - && !isUpdatedSystemApp) { - final ApexManager apexManager = ApexManager.getInstance(); - final String packageName = pkg.getPackageName(); - final String containingApexPackageName = - apexManager.getActiveApexPackageNameContainingPackage(packageName); - final boolean isInUpdatedApex = containingApexPackageName != null - && !apexManager.isFactory(apexManager.getPackageInfo(containingApexPackageName, - MATCH_ACTIVE_PACKAGE)); - // Apps that are in updated apexs' do not need to be allowlisted - if (!isInUpdatedApex) { - // it's only a reportable violation if the permission isn't explicitly - // denied - if (isInSystemConfigPrivAppDenyPermissions(pkg, permissionName)) { - return false; - } - Slog.w(TAG, "Privileged permission " + permissionName + " for package " - + packageName + " (" + pkg.getPath() - + ") not in privapp-permissions allowlist"); - if (RoSystemProperties.CONTROL_PRIVAPP_PERMISSIONS_ENFORCE) { - synchronized (mLock) { - if (mPrivappPermissionsViolations == null) { - mPrivappPermissionsViolations = new ArraySet<>(); - } - mPrivappPermissionsViolations.add(packageName + " (" + pkg.getPath() + "): " - + permissionName); - } - } - } - } - return !RoSystemProperties.CONTROL_PRIVAPP_PERMISSIONS_ENFORCE; - } - - private boolean isInSystemConfigPrivAppPermissions(@NonNull AndroidPackage pkg, - @NonNull String permission) { - final SystemConfig systemConfig = SystemConfig.getInstance(); - final Set permissions; - if (pkg.isVendor()) { - permissions = systemConfig.getVendorPrivAppPermissions(pkg.getPackageName()); - } else if (pkg.isProduct()) { - permissions = systemConfig.getProductPrivAppPermissions(pkg.getPackageName()); - } else if (pkg.isSystemExt()) { - permissions = systemConfig.getSystemExtPrivAppPermissions(pkg.getPackageName()); - } else { - permissions = systemConfig.getPrivAppPermissions(pkg.getPackageName()); - } - return permissions != null && permissions.contains(permission); - } - - private boolean isInSystemConfigPrivAppDenyPermissions(@NonNull AndroidPackage pkg, - @NonNull String permission) { - final SystemConfig systemConfig = SystemConfig.getInstance(); - final Set permissions; - if (pkg.isVendor()) { - permissions = systemConfig.getVendorPrivAppDenyPermissions(pkg.getPackageName()); - } else if (pkg.isProduct()) { - permissions = systemConfig.getProductPrivAppDenyPermissions(pkg.getPackageName()); - } else if (pkg.isSystemExt()) { - permissions = systemConfig.getSystemExtPrivAppDenyPermissions(pkg.getPackageName()); - } else { - permissions = systemConfig.getPrivAppDenyPermissions(pkg.getPackageName()); - } - return permissions != null && permissions.contains(permission); - } - private static boolean canGrantOemPermission(AndroidPackage pkg, String permission) { if (!pkg.isOem()) { return false;