From 750185ba76d8bebdd2c10930be89c2185e288e34 Mon Sep 17 00:00:00 2001 From: Rubin Xu Date: Wed, 4 Nov 2020 13:52:40 +0000 Subject: [PATCH] Remove some getCallerIdentity variants in DevicePolicyManagerService CallerIdentity object should only wrap around caller-provided and authenticated (uid, ComponentName, PackageName) tuple. In some instances the caller does not provide ComponentName or PackageName, and CallerIdentity should keep track of this instead of trying to infer the associated ComponentName or PackageName. This is because there is one-many mapping from uid to package name, so the inference can be imprecise as well as losing information of what the caller supplied originally. Instead all the isXX() access control methods should handle the case that ComponentName or PackageName might be missing. Bug: 167960209 Test: atest OrgOwnedProfileOwnerTest Test: atest MixedDeviceOwnerTest Test: atest MixedManagedProfileOwnerTest Test: atest MixedProfileOwnerTest Test: atest FrameworksServicesTests:DevicePolicyManagerTest Change-Id: I8d2dfeb2e5137ea06f941855d2e5608998a86912 --- .../app/admin/DevicePolicyManager.java | 2 + .../DevicePolicyManagerService.java | 297 ++++++------------ 2 files changed, 98 insertions(+), 201 deletions(-) diff --git a/core/java/android/app/admin/DevicePolicyManager.java b/core/java/android/app/admin/DevicePolicyManager.java index 16ae081049ef..b4eb183d5f10 100644 --- a/core/java/android/app/admin/DevicePolicyManager.java +++ b/core/java/android/app/admin/DevicePolicyManager.java @@ -6436,6 +6436,8 @@ public class DevicePolicyManager { } /** + * Called by a privileged caller holding {@code BIND_DEVICE_ADMIN} permission to retrieve + * the remove warning for the given device admin. * @hide */ public void getRemoveWarning(@Nullable ComponentName admin, RemoteCallback result) { diff --git a/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java b/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java index a6466821ff38..1a4c30f9c6e7 100644 --- a/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java +++ b/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java @@ -1576,172 +1576,65 @@ public class DevicePolicyManagerService extends BaseIDevicePolicyManager { * Creates a new {@link CallerIdentity} object to represent the caller's identity. */ private CallerIdentity getCallerIdentity() { - final int callerUid = mInjector.binderGetCallingUid(); - return new CallerIdentity(callerUid, null, null); + return getCallerIdentity(null, null); } /** * Creates a new {@link CallerIdentity} object to represent the caller's identity. */ - private CallerIdentity getCallerIdentity(@NonNull String callerPackage) { - final int callerUid = mInjector.binderGetCallingUid(); - - if (!isCallingFromPackage(callerPackage, callerUid)) { - throw new SecurityException( - String.format("Caller with uid %d is not %s", callerUid, callerPackage)); - } + private CallerIdentity getCallerIdentity(@Nullable String callerPackage) { - return new CallerIdentity(callerUid, callerPackage, null); + return getCallerIdentity(null, callerPackage); } /** * Creates a new {@link CallerIdentity} object to represent the caller's identity. + * The component name should be an active admin for the calling user. */ @VisibleForTesting - protected CallerIdentity getCallerIdentity(@Nullable ComponentName adminComponent, - @NonNull String callerPackage) { - return adminComponent == null - ? getCallerIdentity(callerPackage) - : getCallerIdentity(adminComponent); + CallerIdentity getCallerIdentity(@Nullable ComponentName adminComponent) { + return getCallerIdentity(adminComponent, null); } /** * Creates a new {@link CallerIdentity} object to represent the caller's identity. - * The component name should be an active admin for the calling user. + * If {@code adminComponent} is provided, it's validated against the list of known + * active admins and caller uid. If {@code callerPackage} is provided, it's validated + * against the caller uid. If a valid {@code adminComponent} is provided but not + * {@code callerPackage}, the package name of the {@code adminComponent} is used instead. */ @VisibleForTesting - protected CallerIdentity getCallerIdentity(@NonNull ComponentName adminComponent) { + CallerIdentity getCallerIdentity(@Nullable ComponentName adminComponent, + @Nullable String callerPackage) { final int callerUid = mInjector.binderGetCallingUid(); - final DevicePolicyData policy = getUserData(UserHandle.getUserId(callerUid)); - ActiveAdmin admin = policy.mAdminMap.get(adminComponent); - if (admin == null) { - throw new SecurityException(String.format("No active admin for %s", adminComponent)); - } - if (admin.getUid() != callerUid) { - throw new SecurityException( - String.format("Admin %s is not owned by uid %d", adminComponent, callerUid)); - } - - return new CallerIdentity(callerUid, adminComponent.getPackageName(), adminComponent); - } - - /** - * Creates a new {@link CallerIdentity} object to represent the caller's identity, which should - * be an admin of a profile on the device. If no component name is provided, look up the - * component name and fill it in for the caller. - * - * Note: this method should only be called when the expected caller is an admin. - * - * @throws SecurityException if the caller is not an active admin. - */ - private CallerIdentity getAdminCallerIdentity(@Nullable ComponentName adminComponent) { - if (adminComponent == null) { - ActiveAdmin admin = getActiveAdminOfCaller(); - if (admin != null) { - return getCallerIdentity(admin.info.getComponent()); - } - throw new SecurityException("Caller is not an active admin"); - } else { - return getCallerIdentity(adminComponent); - } - } - - /** - * Creates a new {@link CallerIdentity} object to represent the caller's identity. If no - * component name is provided, look up the component name and fill it in for the caller. - * - * Note: this method should only be called when the caller may not be an admin. If the caller - * is not an admin, the ComponentName in the returned identity will be null. - */ - private CallerIdentity getNonPrivilegedOrAdminCallerIdentity( - @Nullable ComponentName adminComponent) { - if (adminComponent == null) { - ActiveAdmin admin = getActiveAdminOfCaller(); - if (admin != null) { - adminComponent = admin.info.getComponent(); - } else { - return getCallerIdentity(); + if (callerPackage != null) { + if (!isCallingFromPackage(callerPackage, callerUid)) { + throw new SecurityException( + String.format("Caller with uid %d is not %s", callerUid, callerPackage)); } } - return getCallerIdentity(adminComponent); - } + if (adminComponent != null) { + final DevicePolicyData policy = getUserData(UserHandle.getUserId(callerUid)); + ActiveAdmin admin = policy.mAdminMap.get(adminComponent); - /** - * Creates a new {@link CallerIdentity} object to represent the caller's identity. If no - * package name is provided, look up the package name and fill it in for the caller. - * - * Note: this method should only be called when the expected caller is an admin. - * - * @throws SecurityException if the caller is not an active admin. - */ - private CallerIdentity getAdminCallerIdentityUsingPackage(@Nullable String callerPackage) { - if (callerPackage == null) { - ActiveAdmin admin = getActiveAdminOfCaller(); - if (admin != null) { - return getCallerIdentity(admin.info.getPackageName()); + if (admin == null) { + throw new SecurityException(String.format( + "No active admin for %s", adminComponent)); } - throw new SecurityException("Caller is not an active admin"); - } - return getCallerIdentity(callerPackage); - } - - /** - * Creates a new {@link CallerIdentity} object to represent the caller's identity. If no - * package name is provided, look up the package name and fill it in for the caller. - */ - private CallerIdentity getNonPrivilegedOrAdminCallerIdentityUsingPackage( - @Nullable String callerPackage) { - if (callerPackage == null) { - ActiveAdmin admin = getActiveAdminOfCaller(); - if (admin != null) { - callerPackage = admin.info.getPackageName(); + if (admin.getUid() != callerUid) { + throw new SecurityException(String.format( + "Admin %s is not owned by uid %d", adminComponent, callerUid)); + } + if (callerPackage != null) { + Preconditions.checkArgument(callerPackage.equals(adminComponent.getPackageName())); } else { - return getCallerIdentity(); + callerPackage = adminComponent.getPackageName(); } } - return getCallerIdentity(callerPackage); - } - - /** - * Creates a new {@link CallerIdentity} object to represent the caller's identity. - * If an {@code adminComponent} is specified, then the caller must be an admin and - * the provided component name must match the caller's UID. - * - * If a package name is provided, then the caller doesn't have to be an admin, and the - * provided package must belong to the caller's UID. - * - * If neither is provided, the caller identity is returned as-is. - * - * Note: this method should only be called when the caller may not be an admin. If the caller - * is not an admin, the ComponentName in the returned identity will be null. - */ - private CallerIdentity getNonPrivilegedOrAdminCallerIdentity( - @Nullable ComponentName adminComponent, - @Nullable String callerPackage) { - if (adminComponent != null) { - return getCallerIdentity(adminComponent); - } - - return getNonPrivilegedOrAdminCallerIdentityUsingPackage(callerPackage); - } - /** - * Retrieves the active admin of the caller. This method should not be called directly and - * should only be called by {@link #getAdminCallerIdentity}, - * {@link #getNonPrivilegedOrAdminCallerIdentity}, {@link #getAdminCallerIdentityUsingPackage} - * or {@link #getNonPrivilegedOrAdminCallerIdentityUsingPackage}. - */ - private ActiveAdmin getActiveAdminOfCaller() { - final int callerUid = mInjector.binderGetCallingUid(); - final DevicePolicyData policy = getUserData(UserHandle.getUserId(callerUid)); - for (ActiveAdmin admin : policy.mAdminList) { - if (admin.getUid() == callerUid) { - return admin; - } - } - return null; + return new CallerIdentity(callerUid, callerPackage, adminComponent); } /** @@ -4307,7 +4200,7 @@ public class DevicePolicyManagerService extends BaseIDevicePolicyManager { @Override @PasswordComplexity public int getPasswordComplexity(boolean parent) { - final CallerIdentity caller = getNonPrivilegedOrAdminCallerIdentity(null); + final CallerIdentity caller = getCallerIdentity(); DevicePolicyEventLogger .createEvent(DevicePolicyEnums.GET_USER_PASSWORD_COMPLEXITY_LEVEL) .setStrings(parent ? CALLED_FROM_PARENT : NOT_CALLED_FROM_PARENT, @@ -4315,15 +4208,18 @@ public class DevicePolicyManagerService extends BaseIDevicePolicyManager { mInjector.binderGetCallingUid())) .write(); - Preconditions.checkCallAuthorization(!parent || (isDeviceOwner(caller) - || isProfileOwner(caller) || isSystemUid(caller)), - "Only profile owner, device owner and system may call this method."); enforceUserUnlocked(caller.getUserId()); - Preconditions.checkCallAuthorization( - hasCallingOrSelfPermission(REQUEST_PASSWORD_COMPLEXITY) - || isDeviceOwner(caller) || isProfileOwner(caller), - "Must have " + REQUEST_PASSWORD_COMPLEXITY - + " permission, or be a profile owner or device owner."); + if (parent) { + Preconditions.checkCallAuthorization( + isDeviceOwner(caller) || isProfileOwner(caller) || isSystemUid(caller), + "Only profile owner, device owner and system may call this method on parent."); + } else { + Preconditions.checkCallAuthorization( + hasCallingOrSelfPermission(REQUEST_PASSWORD_COMPLEXITY) + || isDeviceOwner(caller) || isProfileOwner(caller), + "Must have " + REQUEST_PASSWORD_COMPLEXITY + + " permission, or be a profile owner or device owner."); + } synchronized (getLockObject()) { final int credentialOwner = getCredentialOwner(caller.getUserId(), parent); @@ -4342,7 +4238,7 @@ public class DevicePolicyManagerService extends BaseIDevicePolicyManager { Preconditions.checkArgument(allowedModes.contains(passwordComplexity), "Provided complexity is not one of the allowed values."); - final CallerIdentity caller = getAdminCallerIdentity(null); + final CallerIdentity caller = getCallerIdentity(); Preconditions.checkCallAuthorization(isDeviceOwner(caller) || isProfileOwner(caller)); Preconditions.checkArgument(!calledOnParent || isProfileOwner(caller)); @@ -4386,11 +4282,11 @@ public class DevicePolicyManagerService extends BaseIDevicePolicyManager { return PASSWORD_COMPLEXITY_NONE; } - final CallerIdentity caller = getAdminCallerIdentity(null); + final CallerIdentity caller = getCallerIdentity(); Preconditions.checkCallAuthorization( isDeviceOwner(caller) || isProfileOwner(caller)); - Preconditions.checkArgument(!calledOnParent || hasProfileOwner(caller.getUserId())); + Preconditions.checkArgument(!calledOnParent || isProfileOwner(caller)); synchronized (getLockObject()) { final ActiveAdmin requiredAdmin = getParentOfAdminIfRequired( @@ -5626,15 +5522,18 @@ public class DevicePolicyManagerService extends BaseIDevicePolicyManager { public List getDelegatedScopes(ComponentName who, String delegatePackage) throws SecurityException { Objects.requireNonNull(delegatePackage, "Delegate package is null"); - final CallerIdentity caller = getNonPrivilegedOrAdminCallerIdentity(who, delegatePackage); + final CallerIdentity caller = getCallerIdentity(who); // Ensure the caller may call this method: - // * Either it's an admin - // * Or it's an app identified by its calling package name (the - // getNonPrivilegedOrAdminCallerIdentity method validated the UID and package match). - Preconditions.checkCallAuthorization( - (caller.hasAdminComponent() && (isProfileOwner(caller) || isDeviceOwner(caller))) - || delegatePackage != null); + // * Either it's a profile owner / device owner, if componentName is provided + // * Or it's an app querying its own delegation scopes + if (caller.hasAdminComponent()) { + Preconditions.checkCallAuthorization(isProfileOwner(caller) || isDeviceOwner(caller)); + } else { + Preconditions.checkCallAuthorization(isPackage(caller, delegatePackage), + String.format("Caller with uid %d is not %s", caller.getUid(), + delegatePackage)); + } synchronized (getLockObject()) { final DevicePolicyData policy = getUserData(caller.getUserId()); // Retrieve the scopes assigned to delegatePackage, or null if no scope was given. @@ -5924,12 +5823,16 @@ public class DevicePolicyManagerService extends BaseIDevicePolicyManager { @Override public boolean isAlwaysOnVpnLockdownEnabled(ComponentName admin) throws SecurityException { - Objects.requireNonNull(admin, "ComponentName is null"); - - final CallerIdentity caller = getNonPrivilegedOrAdminCallerIdentity(admin); - Preconditions.checkCallAuthorization((caller.hasAdminComponent() - && (isDeviceOwner(caller) || isProfileOwner(caller))) - || hasCallingPermission(PERMISSION_MAINLINE_NETWORK_STACK)); + final CallerIdentity caller; + if (hasCallingPermission(PERMISSION_MAINLINE_NETWORK_STACK)) { + // TODO: CaptivePortalLoginActivity erroneously calls this method with a non-admin + // ComponentName, so we have to use a separate code path for it: + // getCallerIdentity(admin) will throw if the admin is not in the known admin list. + caller = getCallerIdentity(); + } else { + caller = getCallerIdentity(admin); + Preconditions.checkCallAuthorization(isDeviceOwner(caller) || isProfileOwner(caller)); + } return mInjector.binderWithCleanCallingIdentity( () -> mInjector.getConnectivityManager().isVpnLockdownEnabled(caller.getUserId())); @@ -6209,6 +6112,10 @@ public class DevicePolicyManagerService extends BaseIDevicePolicyManager { return getFrpManagementAgentUid() != -1; } + /** + * Called by a privileged caller holding {@code BIND_DEVICE_ADMIN} permission to retrieve + * the remove warning for the given device admin. + */ @Override public void getRemoveWarning(ComponentName comp, final RemoteCallback result, int userHandle) { if (!mHasFeature) { @@ -6649,13 +6556,13 @@ public class DevicePolicyManagerService extends BaseIDevicePolicyManager { * active admins. */ @Override - public boolean getStorageEncryption(ComponentName who, int userHandle) { + public boolean getStorageEncryption(@Nullable ComponentName who, int userHandle) { if (!mHasFeature) { return false; } Preconditions.checkArgumentNonnegative(userHandle, "Invalid userId"); - final CallerIdentity caller = getAdminCallerIdentity(who); + final CallerIdentity caller = getCallerIdentity(who); Preconditions.checkCallAuthorization(hasFullCrossUsersPermission(caller, userHandle)); synchronized (getLockObject()) { @@ -6689,12 +6596,9 @@ public class DevicePolicyManagerService extends BaseIDevicePolicyManager { } Preconditions.checkArgumentNonnegative(userHandle, "Invalid userId"); - final CallerIdentity caller = getAdminCallerIdentityUsingPackage(callerPackage); + final CallerIdentity caller = getCallerIdentity(callerPackage); Preconditions.checkCallAuthorization(hasFullCrossUsersPermission(caller, userHandle)); - // It's not critical here, but let's make sure the package name is correct, in case - // we start using it for different purposes. - ensureCallerPackage(callerPackage); final ApplicationInfo ai; try { @@ -7571,6 +7475,10 @@ public class DevicePolicyManagerService extends BaseIDevicePolicyManager { return isProfileOwner(caller) && caller.getUserHandle().isSystem(); } + private boolean isPackage(CallerIdentity caller, String packageName) { + return isCallingFromPackage(packageName, caller.getUid()); + } + @Override public ComponentName getDeviceOwnerComponent(boolean callingUserOnly) { if (!mHasFeature) { @@ -8521,8 +8429,7 @@ public class DevicePolicyManagerService extends BaseIDevicePolicyManager { return true; } - private void enforceCanCallLockTaskLocked(ComponentName who) { - final CallerIdentity caller = getAdminCallerIdentity(who); + private void enforceCanCallLockTaskLocked(CallerIdentity caller) { Preconditions.checkCallAuthorization(isProfileOwner(caller) || isDeviceOwner(caller)); final int userId = caller.getUserId(); @@ -8531,22 +8438,6 @@ public class DevicePolicyManagerService extends BaseIDevicePolicyManager { } } - private void ensureCallerPackage(@Nullable String packageName) { - if (packageName == null) { - enforceSystemCaller("omit package name"); - } else { - final int callingUid = mInjector.binderGetCallingUid(); - final int userId = mInjector.userHandleGetCallingUserId(); - try { - final ApplicationInfo ai = mIPackageManager.getApplicationInfo( - packageName, 0, userId); - Preconditions.checkState(ai.uid == callingUid, "Unmatching package name"); - } catch (RemoteException e) { - // Shouldn't happen - } - } - } - private boolean isCallerWithSystemUid() { return UserHandle.isSameApp(mInjector.binderGetCallingUid(), Process.SYSTEM_UID); } @@ -8842,7 +8733,7 @@ public class DevicePolicyManagerService extends BaseIDevicePolicyManager { Objects.requireNonNull(agent, "agent null"); Preconditions.checkArgumentNonnegative(userHandle, "Invalid userId"); - final CallerIdentity caller = getAdminCallerIdentity(admin); + final CallerIdentity caller = getCallerIdentity(); Preconditions.checkCallAuthorization(hasFullCrossUsersPermission(caller, userHandle)); synchronized (getLockObject()) { @@ -10551,10 +10442,11 @@ public class DevicePolicyManagerService extends BaseIDevicePolicyManager { throws SecurityException { Objects.requireNonNull(who, "ComponentName is null"); Objects.requireNonNull(packages, "packages is null"); + final CallerIdentity caller = getCallerIdentity(who); synchronized (getLockObject()) { - enforceCanCallLockTaskLocked(who); - final int userHandle = mInjector.userHandleGetCallingUserId(); + enforceCanCallLockTaskLocked(caller); + final int userHandle = caller.getUserId(); setLockTaskPackagesLocked(userHandle, new ArrayList<>(Arrays.asList(packages))); } } @@ -10571,10 +10463,11 @@ public class DevicePolicyManagerService extends BaseIDevicePolicyManager { @Override public String[] getLockTaskPackages(ComponentName who) { Objects.requireNonNull(who, "ComponentName is null"); + final CallerIdentity caller = getCallerIdentity(who); + final int userHandle = caller.getUserId(); - final int userHandle = mInjector.binderGetCallingUserHandle().getIdentifier(); synchronized (getLockObject()) { - enforceCanCallLockTaskLocked(who); + enforceCanCallLockTaskLocked(caller); final List packages = getUserData(userHandle).mLockTaskPackages; return packages.toArray(new String[packages.size()]); } @@ -10601,9 +10494,10 @@ public class DevicePolicyManagerService extends BaseIDevicePolicyManager { Preconditions.checkArgument(hasHome || !hasNotification, "Cannot use LOCK_TASK_FEATURE_NOTIFICATIONS without LOCK_TASK_FEATURE_HOME"); - final int userHandle = mInjector.userHandleGetCallingUserId(); + final CallerIdentity caller = getCallerIdentity(who); + final int userHandle = caller.getUserId(); synchronized (getLockObject()) { - enforceCanCallLockTaskLocked(who); + enforceCanCallLockTaskLocked(caller); setLockTaskFeaturesLocked(userHandle, flags); } } @@ -10618,9 +10512,10 @@ public class DevicePolicyManagerService extends BaseIDevicePolicyManager { @Override public int getLockTaskFeatures(ComponentName who) { Objects.requireNonNull(who, "ComponentName is null"); - final int userHandle = mInjector.userHandleGetCallingUserId(); + final CallerIdentity caller = getCallerIdentity(who); + final int userHandle = caller.getUserId(); synchronized (getLockObject()) { - enforceCanCallLockTaskLocked(who); + enforceCanCallLockTaskLocked(caller); return getUserData(userHandle).mLockTaskFeatures; } } @@ -11069,7 +10964,7 @@ public class DevicePolicyManagerService extends BaseIDevicePolicyManager { @Override public boolean setStatusBarDisabled(ComponentName who, boolean disabled) { - final CallerIdentity caller = getAdminCallerIdentity(who); + final CallerIdentity caller = getCallerIdentity(who); Preconditions.checkCallAuthorization(isProfileOwner(caller) || isDeviceOwner(caller)); int userId = caller.getUserId(); @@ -12832,7 +12727,7 @@ public class DevicePolicyManagerService extends BaseIDevicePolicyManager { } final Set affiliationIds = new ArraySet<>(ids); - final CallerIdentity caller = getAdminCallerIdentity(admin); + final CallerIdentity caller = getCallerIdentity(admin); Preconditions.checkCallAuthorization(isProfileOwner(caller) || isDeviceOwner(caller)); final int callingUserId = caller.getUserId(); @@ -13787,7 +13682,7 @@ public class DevicePolicyManagerService extends BaseIDevicePolicyManager { if (token == null || token.length < 32) { throw new IllegalArgumentException("token must be at least 32-byte long"); } - final CallerIdentity caller = getAdminCallerIdentity(admin); + final CallerIdentity caller = getCallerIdentity(admin); Preconditions.checkCallAuthorization(isProfileOwner(caller) || isDeviceOwner(caller)); synchronized (getLockObject()) { @@ -13811,7 +13706,7 @@ public class DevicePolicyManagerService extends BaseIDevicePolicyManager { if (!mHasFeature || !mLockPatternUtils.hasSecureLockScreen()) { return false; } - final CallerIdentity caller = getAdminCallerIdentity(admin); + final CallerIdentity caller = getCallerIdentity(admin); Preconditions.checkCallAuthorization(isProfileOwner(caller) || isDeviceOwner(caller)); synchronized (getLockObject()) { @@ -13836,7 +13731,7 @@ public class DevicePolicyManagerService extends BaseIDevicePolicyManager { if (!mHasFeature || !mLockPatternUtils.hasSecureLockScreen()) { return false; } - final CallerIdentity caller = getAdminCallerIdentity(admin); + final CallerIdentity caller = getCallerIdentity(admin); Preconditions.checkCallAuthorization(isProfileOwner(caller) || isDeviceOwner(caller)); synchronized (getLockObject()) { @@ -13861,7 +13756,7 @@ public class DevicePolicyManagerService extends BaseIDevicePolicyManager { } Objects.requireNonNull(token); - final CallerIdentity caller = getAdminCallerIdentity(admin); + final CallerIdentity caller = getCallerIdentity(admin); Preconditions.checkCallAuthorization(isProfileOwner(caller) || isDeviceOwner(caller)); synchronized (getLockObject()) { -- GitLab