From 50f0f94b7536e047a4b2c48b7d496f6d113bf7bf Mon Sep 17 00:00:00 2001 From: Pavel Grafov Date: Fri, 3 Dec 2021 15:05:41 +0000 Subject: [PATCH] Check caller owns admin for per-admin getters. Currently some functions allow the caller to check policy enforced by a particular admin by passing a non-null "who" argument. This circumvents package visibility rules. There is no legitimate reason for an 3rd party app to query policies for arbitrary admins. With this change whenever "who" is not null, the code will ensure that the admin referenced by "who" is owned by the caller. Exception to the above are methods that are also called by Setting to query policy for a particular admin for policy transparency. For those methods callers with QUERY_ADMIN_POLICY permission are allowed to query per-admin policy: * getMaximumFailedPasswordsForWipe * getMaximumTimeToLock * getPasswordQuality There is no legitimate reason for an 3rd party app to query policies for arbitrary admins. Code search for getPasswordHistoryLength and getPasswordMinimum* methods doesn't return any priviledged usage of these methods inside Android. getPasswordQuality is used by Settings, hence the system uid is exempt. + removed redundant system or root UID checks when querying permission. Bug: 204995407 Test: atest android.devicepolicy.cts.NoAdminLeakingTest Test: atest android.devicepolicy.cts.ResetPasswordWithTokenTest Test: atest com.android.cts.devicepolicy.MixedDeviceOwnerTest#testPasswordRequirementsApi Change-Id: I443ed1f6dcd5e5b161c99dd09a4b2aef9f8ef0a7 --- .../DevicePolicyManagerService.java | 40 ++++++++++++++----- 1 file changed, 29 insertions(+), 11 deletions(-) diff --git a/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java b/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java index bfceeda08e3df..2a0d0086dde56 100644 --- a/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java +++ b/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java @@ -3998,6 +3998,10 @@ public class DevicePolicyManagerService extends BaseIDevicePolicyManager { final CallerIdentity caller = getCallerIdentity(); Preconditions.checkCallAuthorization(hasFullCrossUsersPermission(caller, userHandle)); + // System caller can query policy for a particular admin. + Preconditions.checkCallAuthorization( + who == null || isCallingFromPackage(who.getPackageName(), caller.getUid()) + || canQueryAdminPolicy(caller)); synchronized (getLockObject()) { int mode = PASSWORD_QUALITY_UNSPECIFIED; @@ -4213,7 +4217,7 @@ public class DevicePolicyManagerService extends BaseIDevicePolicyManager { } Preconditions.checkArgumentNonnegative(userHandle, "Invalid userId"); - final CallerIdentity caller = getCallerIdentity(); + final CallerIdentity caller = getCallerIdentity(who); Preconditions.checkCallAuthorization(hasFullCrossUsersPermission(caller, userHandle)); synchronized (getLockObject()) { @@ -4363,7 +4367,7 @@ public class DevicePolicyManagerService extends BaseIDevicePolicyManager { } Preconditions.checkArgumentNonnegative(userHandle, "Invalid userId"); - final CallerIdentity caller = getCallerIdentity(); + final CallerIdentity caller = getCallerIdentity(who); Preconditions.checkCallAuthorization(hasFullCrossUsersPermission(caller, userHandle)); synchronized (getLockObject()) { @@ -4576,7 +4580,7 @@ public class DevicePolicyManagerService extends BaseIDevicePolicyManager { } Preconditions.checkArgumentNonnegative(userHandle, "Invalid userId"); - final CallerIdentity caller = getCallerIdentity(); + final CallerIdentity caller = getCallerIdentity(who); Preconditions.checkCallAuthorization(hasFullCrossUsersPermission(caller, userHandle)); synchronized (getLockObject()) { @@ -4996,6 +5000,10 @@ public class DevicePolicyManagerService extends BaseIDevicePolicyManager { final CallerIdentity caller = getCallerIdentity(); Preconditions.checkCallAuthorization(hasFullCrossUsersPermission(caller, userHandle)); + // System caller can query policy for a particular admin. + Preconditions.checkCallAuthorization( + who == null || isCallingFromPackage(who.getPackageName(), caller.getUid()) + || canQueryAdminPolicy(caller)); synchronized (getLockObject()) { ActiveAdmin admin = (who != null) @@ -5307,6 +5315,10 @@ public class DevicePolicyManagerService extends BaseIDevicePolicyManager { final CallerIdentity caller = getCallerIdentity(); Preconditions.checkCallAuthorization(hasFullCrossUsersPermission(caller, userHandle)); + // System caller can query policy for a particular admin. + Preconditions.checkCallAuthorization( + who == null || isCallingFromPackage(who.getPackageName(), caller.getUid()) + || canQueryAdminPolicy(caller)); synchronized (getLockObject()) { if (who != null) { @@ -5384,7 +5396,7 @@ public class DevicePolicyManagerService extends BaseIDevicePolicyManager { } Preconditions.checkArgumentNonnegative(userId, "Invalid userId"); - final CallerIdentity caller = getCallerIdentity(); + final CallerIdentity caller = getCallerIdentity(who); Preconditions.checkCallAuthorization(hasFullCrossUsersPermission(caller, userId)); if (!mLockPatternUtils.hasSecureLockScreen()) { @@ -7727,6 +7739,10 @@ public class DevicePolicyManagerService extends BaseIDevicePolicyManager { if (!mHasFeature) { return false; } + + final CallerIdentity caller = getCallerIdentity(who); + Preconditions.checkCallAuthorization(hasFullCrossUsersPermission(caller, userHandle)); + if (parent) { Preconditions.checkCallAuthorization( isProfileOwnerOfOrganizationOwnedDevice(getCallerIdentity().getUserId())); @@ -9509,8 +9525,11 @@ public class DevicePolicyManagerService extends BaseIDevicePolicyManager { } private boolean canManageUsers(CallerIdentity caller) { - return isSystemUid(caller) || isRootUid(caller) - || hasCallingOrSelfPermission(permission.MANAGE_USERS); + return hasCallingOrSelfPermission(permission.MANAGE_USERS); + } + + private boolean canQueryAdminPolicy(CallerIdentity caller) { + return hasCallingOrSelfPermission(permission.QUERY_ADMIN_POLICY); } private boolean hasPermission(String permission, int pid, int uid) { @@ -9958,7 +9977,7 @@ public class DevicePolicyManagerService extends BaseIDevicePolicyManager { Objects.requireNonNull(agent, "agent null"); Preconditions.checkArgumentNonnegative(userHandle, "Invalid userId"); - final CallerIdentity caller = getCallerIdentity(); + final CallerIdentity caller = getCallerIdentity(admin); Preconditions.checkCallAuthorization(hasFullCrossUsersPermission(caller, userHandle)); synchronized (getLockObject()) { @@ -10238,8 +10257,8 @@ public class DevicePolicyManagerService extends BaseIDevicePolicyManager { if (!mHasFeature) { return null; } - Preconditions.checkCallAuthorization(canManageUsers(getCallerIdentity()) - || hasCallingOrSelfPermission(permission.QUERY_ADMIN_POLICY)); + final CallerIdentity caller = getCallerIdentity(); + Preconditions.checkCallAuthorization(canManageUsers(caller) || canQueryAdminPolicy(caller)); synchronized (getLockObject()) { List result = null; @@ -10410,8 +10429,7 @@ public class DevicePolicyManagerService extends BaseIDevicePolicyManager { public @Nullable List getPermittedInputMethodsAsUser(@UserIdInt int userId) { final CallerIdentity caller = getCallerIdentity(); Preconditions.checkCallAuthorization(hasFullCrossUsersPermission(caller, userId)); - Preconditions.checkCallAuthorization(canManageUsers(caller) - || hasCallingOrSelfPermission(permission.QUERY_ADMIN_POLICY)); + Preconditions.checkCallAuthorization(canManageUsers(caller) || canQueryAdminPolicy(caller)); final long callingIdentity = Binder.clearCallingIdentity(); try { return getPermittedInputMethodsUnchecked(userId);