From c40355f469dc6761c262705ed534af638980da1d Mon Sep 17 00:00:00 2001 From: Alex Johnston Date: Tue, 13 Apr 2021 11:56:20 +0100 Subject: DPMS setDeviceOwner access control * Check whether the caller is system or has permission to set the device owner before checking whether the package exists on device. Manual testing * Call DPMS setDeviceOwner with an installed package but not as the system caller * Call DPMS setDeviceOwner with an uninstalled package and not as the system caller * Both calls should give the same error message and logs. Bug: 184525740 Test: manual testing atest com.android.server.devicepolicy.DevicePolicyManagerTest atest com.android.cts.devicepolicy.DeviceOwnerTest Change-Id: I5654ef420ff44af01d475595609bb85fec132ce1 --- .../server/devicepolicy/DevicePolicyManagerService.java | 14 +++++--------- .../server/devicepolicy/DevicePolicyManagerTest.java | 10 ++++++++++ 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java b/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java index 00a1786d28db..7b6637766258 100644 --- a/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java +++ b/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java @@ -8094,20 +8094,16 @@ public class DevicePolicyManagerService extends BaseIDevicePolicyManager { + " as device owner for user " + userId); return false; } - if (admin == null - || !isPackageInstalledForUser(admin.getPackageName(), userId)) { - throw new IllegalArgumentException("Invalid component " + admin - + " for device owner"); - } + Preconditions.checkArgument(admin != null); final CallerIdentity caller = getCallerIdentity(); synchronized (getLockObject()) { enforceCanSetDeviceOwnerLocked(caller, admin, userId); + Preconditions.checkArgument(isPackageInstalledForUser(admin.getPackageName(), userId), + "Invalid component " + admin + " for device owner"); final ActiveAdmin activeAdmin = getActiveAdminUncheckedLocked(admin, userId); - if (activeAdmin == null - || getUserData(userId).mRemovingAdmins.contains(admin)) { - throw new IllegalArgumentException("Not active admin: " + admin); - } + Preconditions.checkArgument(activeAdmin != null && !getUserData( + userId).mRemovingAdmins.contains(admin), "Not active admin: " + admin); // Shutting down backup manager service permanently. toggleBackupServiceActive(UserHandle.USER_SYSTEM, /* makeActive= */ false); diff --git a/services/tests/servicestests/src/com/android/server/devicepolicy/DevicePolicyManagerTest.java b/services/tests/servicestests/src/com/android/server/devicepolicy/DevicePolicyManagerTest.java index 6113e4c8ff0f..c2c550dee350 100644 --- a/services/tests/servicestests/src/com/android/server/devicepolicy/DevicePolicyManagerTest.java +++ b/services/tests/servicestests/src/com/android/server/devicepolicy/DevicePolicyManagerTest.java @@ -1304,6 +1304,16 @@ public class DevicePolicyManagerTest extends DpmTestBase { @Test public void testSetDeviceOwner_failures() throws Exception { // TODO Test more failure cases. Basically test all chacks in enforceCanSetDeviceOwner(). + // Package doesn't exist and caller is not system + assertExpectException(SecurityException.class, + /* messageRegex= */ "Calling identity is not authorized", + () -> dpm.setDeviceOwner(admin1, "owner-name", UserHandle.USER_SYSTEM)); + + // Package exists, but caller is not system + setUpPackageManagerForAdmin(admin1, DpmMockContext.CALLER_SYSTEM_USER_UID); + assertExpectException(SecurityException.class, + /* messageRegex= */ "Calling identity is not authorized", + () -> dpm.setDeviceOwner(admin1, "owner-name", UserHandle.USER_SYSTEM)); } @Test -- cgit v1.2.3-59-g8ed1b