From f82f7a77b62412f5538c807354a9750272b20546 Mon Sep 17 00:00:00 2001 From: Hai Zhang Date: Thu, 11 Feb 2021 20:17:58 -0800 Subject: [PATCH] Fix updated privileged app with shared UID loses privileged permission when request in manifest. If two privileged apps share the same UID, one of them requested and got a privileged permission granted while another didn't, both will be granted the privileged permission. However when an update is installed for the factory version of the system app that didn't request the permission, and that updated app starts to request the privileged permission, that request itself can't grant the permission because we don't allow updated system apps to get new privileged permissions and we only trust the factory version. However, the original permission request from the other factory app is still valid. As a result of this, in the refactored logic in S the permission has a chance to stop being granted to both apps depending on the order of the two apps being examined for permission grants. The proper fix to this issue is to reconcile permission not by packages, but by UIDs. However, given where we are in the S timeline, such change will require a lot of refactoring to this critical piece of logic, and may lead to performance regression even if it's okay security-wise. So we should defer the proper refactoring to the next release, either in this existing implementation, or in a new permission subsystem. The pre-S implementation had the logic that if a privileged permission was granted to the factory version of a privileged app, it will unconditionally keep the permission granted. And in the case of shared UID, where the permission state of the factory app is the shared user permission state, this helped resolving this issue. That piece of logic was deemed unnecessary and simplified during S refactoring, but this specific case shows that we actually still need it. So we should bring back that logic in S. However due to the split of permission and package locks, we can not call into package when we are holding the permission lock. This is exactly the case in this piece of logic because it needs both the disabled system package (guarded by package lock), as well as the existing permission state (guarded by permission lock). So we have to split the logic into two parts, that is to store the information from package first, and then after grabbing the permission lock, get the existing permissions state and combine the two to reach a conclusion. Another issue that existed before S was that the logic above was broken by privapp permission allowlisting in O, which returns directly without giving the logic handling updated system apps a chance to run. So we need to fix this as well. Test: Manual Test: Install an updated system app that requests a privileged permission not in the allowlist but granted due to another shared UID app, and make sure that dumpsys package shows that the updated app has the permission both requested and granted Fixes: 177926928 Change-Id: I667a9c2e1b87a15c22046e839b248ec6d4e55ca0 --- .../permission/PermissionManagerService.java | 59 +++++++++++++------ 1 file changed, 41 insertions(+), 18 deletions(-) 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 e486f087535be..71e53d9f1f405 100644 --- a/services/core/java/com/android/server/pm/permission/PermissionManagerService.java +++ b/services/core/java/com/android/server/pm/permission/PermissionManagerService.java @@ -2593,6 +2593,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { ArraySet isPrivilegedPermissionAllowlisted = null; ArraySet shouldGrantSignaturePermission = null; ArraySet shouldGrantInternalPermission = null; + ArraySet shouldGrantPrivilegedPermissionIfWasGranted = new ArraySet<>(); final List requestedPermissions = pkg.getRequestedPermissions(); final int requestedPermissionsSize = requestedPermissions.size(); for (int i = 0; i < requestedPermissionsSize; i++) { @@ -2613,14 +2614,16 @@ public class PermissionManagerService extends IPermissionManager.Stub { isPrivilegedPermissionAllowlisted.add(permissionName); } if (permission.isSignature() && (shouldGrantPermissionBySignature(pkg, permission) - || shouldGrantPermissionByProtectionFlags(pkg, ps, permission))) { + || shouldGrantPermissionByProtectionFlags(pkg, ps, permission, + shouldGrantPrivilegedPermissionIfWasGranted))) { if (shouldGrantSignaturePermission == null) { shouldGrantSignaturePermission = new ArraySet<>(); } shouldGrantSignaturePermission.add(permissionName); } if (permission.isInternal() - && shouldGrantPermissionByProtectionFlags(pkg, ps, permission)) { + && shouldGrantPermissionByProtectionFlags(pkg, ps, permission, + shouldGrantPrivilegedPermissionIfWasGranted)) { if (shouldGrantInternalPermission == null) { shouldGrantInternalPermission = new ArraySet<>(); } @@ -2842,14 +2845,18 @@ public class PermissionManagerService extends IPermissionManager.Stub { isPrivilegedPermissionAllowlisted, permName)) && (CollectionUtils.contains(shouldGrantSignaturePermission, permName) - || ((bp.isDevelopment() || bp.isRole()) + || (((bp.isPrivileged() && CollectionUtils.contains( + shouldGrantPrivilegedPermissionIfWasGranted, + permName)) || bp.isDevelopment() || bp.isRole()) && origState.isPermissionGranted(permName)))) || (bp.isInternal() && (!bp.isPrivileged() || CollectionUtils.contains( isPrivilegedPermissionAllowlisted, permName)) && (CollectionUtils.contains(shouldGrantInternalPermission, permName) - || ((bp.isDevelopment() || bp.isRole()) + || (((bp.isPrivileged() && CollectionUtils.contains( + shouldGrantPrivilegedPermissionIfWasGranted, + permName)) || bp.isDevelopment() || bp.isRole()) && origState.isPermissionGranted(permName))))) { // Grant an install permission. if (uidState.grantPermission(bp)) { @@ -3374,10 +3381,23 @@ public class PermissionManagerService extends IPermissionManager.Stub { if (isInSystemConfigPrivAppPermissions(pkg, permissionName)) { return true; } + if (isInSystemConfigPrivAppDenyPermissions(pkg, permissionName)) { + return false; + } + // Updated system apps do not need to be allowlisted + if (packageSetting.getPkgState().isUpdatedSystemApp()) { + // Let shouldGrantPermissionByProtectionFlags() decide whether the privileged permission + // can be granted, because an updated system app may be in a shared UID, and in case a + // new privileged permission is requested by the updated system app but not the factory + // app, although this app and permission combination isn't in the allowlist and can't + // get the permission this way, other apps in the shared UID may still get it. A proper + // fix for this would be to perform the reconciliation by UID, but for now let's keep + // the old workaround working, which is to keep granted privileged permissions still + // granted. + return true; + } // Only enforce the allowlist on boot - if (!mSystemReady - // Updated system apps do not need to be allowlisted - && !packageSetting.getPkgState().isUpdatedSystemApp()) { + if (!mSystemReady) { final ApexManager apexManager = ApexManager.getInstance(); final String containingApexPackageName = apexManager.getActiveApexPackageNameContainingPackage(packageName); @@ -3386,11 +3406,6 @@ public class PermissionManagerService extends IPermissionManager.Stub { 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"); @@ -3468,7 +3483,8 @@ public class PermissionManagerService extends IPermissionManager.Stub { } private boolean shouldGrantPermissionByProtectionFlags(@NonNull AndroidPackage pkg, - @NonNull PackageSetting pkgSetting, @NonNull Permission bp) { + @NonNull PackageSetting pkgSetting, @NonNull Permission bp, + @NonNull ArraySet shouldGrantPrivilegedPermissionIfWasGranted) { boolean allowed = false; final boolean isPrivilegedPermission = bp.isPrivileged(); final boolean isOemPermission = bp.isOem(); @@ -3480,11 +3496,18 @@ public class PermissionManagerService extends IPermissionManager.Stub { final PackageSetting disabledPs = mPackageManagerInt .getDisabledSystemPackage(pkg.getPackageName()); final AndroidPackage disabledPkg = disabledPs == null ? null : disabledPs.pkg; - if (disabledPkg != null && disabledPkg.getRequestedPermissions().contains( - permissionName)) { - allowed = (isPrivilegedPermission && disabledPkg.isPrivileged()) - || (isOemPermission && canGrantOemPermission(disabledPkg, - permissionName)); + if (disabledPkg != null + && ((isPrivilegedPermission && disabledPkg.isPrivileged()) + || (isOemPermission && canGrantOemPermission(disabledPkg, + permissionName)))) { + if (disabledPkg.getRequestedPermissions().contains(permissionName)) { + allowed = true; + } else { + // If the original was granted this permission, we take + // that grant decision as read and propagate it to the + // update. + shouldGrantPrivilegedPermissionIfWasGranted.add(permissionName); + } } } else { allowed = (isPrivilegedPermission && pkg.isPrivileged())