diff --git a/services/core/java/com/android/server/pm/PackageManagerService.java b/services/core/java/com/android/server/pm/PackageManagerService.java index dfe72b26f72ae..317effab37503 100644 --- a/services/core/java/com/android/server/pm/PackageManagerService.java +++ b/services/core/java/com/android/server/pm/PackageManagerService.java @@ -1768,6 +1768,11 @@ public class PackageManagerService extends IPackageManager.Stub public int[] getAllUserIds() { return mUserManager.getUserIds(); } + + @Override + public boolean doesUserExist(@UserIdInt int userId) { + return mUserManager.exists(userId); + } } /** @@ -25693,6 +25698,7 @@ public class PackageManagerService extends IPackageManager.Stub if (!convertedFromPreCreated || !readPermissionStateForUser(userId)) { mPermissionManager.onUserCreated(userId); mLegacyPermissionManager.grantDefaultPermissions(userId); + mDomainVerificationManager.clearUser(userId); } } diff --git a/services/core/java/com/android/server/pm/verify/domain/DomainVerificationEnforcer.java b/services/core/java/com/android/server/pm/verify/domain/DomainVerificationEnforcer.java index 1721a18f4f604..f4bcd3e65913d 100644 --- a/services/core/java/com/android/server/pm/verify/domain/DomainVerificationEnforcer.java +++ b/services/core/java/com/android/server/pm/verify/domain/DomainVerificationEnforcer.java @@ -141,6 +141,12 @@ public class DomainVerificationEnforcer { "Caller is not allowed to edit other users"); } + if (!mCallback.doesUserExist(callingUserId)) { + throw new SecurityException("User " + callingUserId + " does not exist"); + } else if (!mCallback.doesUserExist(targetUserId)) { + throw new SecurityException("User " + targetUserId + " does not exist"); + } + return !mCallback.filterAppAccess(packageName, callingUid, targetUserId); } @@ -161,6 +167,12 @@ public class DomainVerificationEnforcer { Binder.getCallingPid(), callingUid, "Caller is not allowed to edit user selections"); + if (!mCallback.doesUserExist(callingUserId)) { + throw new SecurityException("User " + callingUserId + " does not exist"); + } else if (!mCallback.doesUserExist(targetUserId)) { + throw new SecurityException("User " + targetUserId + " does not exist"); + } + if (packageName == null) { return true; } @@ -184,6 +196,12 @@ public class DomainVerificationEnforcer { } } + if (!mCallback.doesUserExist(callingUserId)) { + throw new SecurityException("User " + callingUserId + " does not exist"); + } else if (!mCallback.doesUserExist(targetUserId)) { + throw new SecurityException("User " + targetUserId + " does not exist"); + } + return !mCallback.filterAppAccess(packageName, callingUid, targetUserId); } @@ -197,6 +215,12 @@ public class DomainVerificationEnforcer { "Caller is not allowed to edit other users"); } + if (!mCallback.doesUserExist(callingUserId)) { + throw new SecurityException("User " + callingUserId + " does not exist"); + } else if (!mCallback.doesUserExist(targetUserId)) { + throw new SecurityException("User " + targetUserId + " does not exist"); + } + return !mCallback.filterAppAccess(packageName, callingUid, targetUserId); } @@ -221,6 +245,12 @@ public class DomainVerificationEnforcer { mContext.enforcePermission( android.Manifest.permission.UPDATE_DOMAIN_VERIFICATION_USER_SELECTION, callingPid, callingUid, "Caller is not allowed to query user selections"); + + if (!mCallback.doesUserExist(callingUserId)) { + throw new SecurityException("User " + callingUserId + " does not exist"); + } else if (!mCallback.doesUserExist(targetUserId)) { + throw new SecurityException("User " + targetUserId + " does not exist"); + } } public interface Callback { @@ -229,5 +259,7 @@ public class DomainVerificationEnforcer { * if the package was not installed */ boolean filterAppAccess(@NonNull String packageName, int callingUid, @UserIdInt int userId); + + boolean doesUserExist(@UserIdInt int userId); } } diff --git a/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationEnforcerTest.kt b/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationEnforcerTest.kt index 6ac9907698312..7e25901301aaa 100644 --- a/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationEnforcerTest.kt +++ b/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationEnforcerTest.kt @@ -51,6 +51,8 @@ import java.io.File import java.util.UUID import java.util.concurrent.atomic.AtomicBoolean import java.util.concurrent.atomic.AtomicInteger +import kotlin.test.assertFailsWith +import kotlin.test.fail @RunWith(Parameterized::class) class DomainVerificationEnforcerTest { @@ -82,47 +84,49 @@ class DomainVerificationEnforcerTest { whenever(filterAppAccess(eq(INVISIBLE_PKG), anyInt(), anyInt())) { true } + whenever(doesUserExist(anyInt())) { (arguments[0] as Int) <= 1 } }) } } - val makeService: (Context) -> Triple = - { - val callingUidInt = AtomicInteger(-1) - val callingUserIdInt = AtomicInteger(-1) + val makeService: (Context) -> Triple = { + val callingUidInt = AtomicInteger(-1) + val callingUserIdInt = AtomicInteger(-1) - val connection: DomainVerificationManagerInternal.Connection = - mockThrowOnUnmocked { - whenever(callingUid) { callingUidInt.get() } - whenever(callingUserId) { callingUserIdInt.get() } - whenever(getPackageSettingLocked(VISIBLE_PKG)) { visiblePkgSetting } - whenever(getPackageLocked(VISIBLE_PKG)) { visiblePkg } - whenever(getPackageSettingLocked(INVISIBLE_PKG)) { invisiblePkgSetting } - whenever(getPackageLocked(INVISIBLE_PKG)) { invisiblePkg } - whenever(schedule(anyInt(), any())) - whenever(scheduleWriteSettings()) - whenever(filterAppAccess(eq(VISIBLE_PKG), anyInt(), anyInt())) { false } - whenever(filterAppAccess(eq(INVISIBLE_PKG), anyInt(), anyInt())) { - true - } + val connection: DomainVerificationManagerInternal.Connection = + mockThrowOnUnmocked { + whenever(callingUid) { callingUidInt.get() } + whenever(callingUserId) { callingUserIdInt.get() } + whenever(getPackageSettingLocked(VISIBLE_PKG)) { visiblePkgSetting } + whenever(getPackageLocked(VISIBLE_PKG)) { visiblePkg } + whenever(getPackageSettingLocked(INVISIBLE_PKG)) { invisiblePkgSetting } + whenever(getPackageLocked(INVISIBLE_PKG)) { invisiblePkg } + whenever(schedule(anyInt(), any())) + whenever(scheduleWriteSettings()) + whenever(filterAppAccess(eq(VISIBLE_PKG), anyInt(), anyInt())) { false } + whenever(filterAppAccess(eq(INVISIBLE_PKG), anyInt(), anyInt())) { + true } - val service = DomainVerificationService( - it, - mockThrowOnUnmocked { whenever(linkedApps) { ArraySet() } }, - mockThrowOnUnmocked { - whenever( - isChangeEnabled( - anyLong(), - any() - ) - ) { true } - }).apply { - setConnection(connection) + whenever(doesUserExist(anyInt())) { (arguments[0] as Int) <= 1 } } - - Triple(callingUidInt, callingUserIdInt, service) + val service = DomainVerificationService( + it, + mockThrowOnUnmocked { whenever(linkedApps) { ArraySet() } }, + mockThrowOnUnmocked { + whenever( + isChangeEnabled( + anyLong(), + any() + ) + ) { true } + }).apply { + setConnection(connection) } + Triple(callingUidInt, callingUserIdInt, service) + } + fun enforcer( type: Type, name: String, @@ -476,24 +480,7 @@ class DomainVerificationEnforcerTest { fun runTestCases(callingUserId: Int, targetUserId: Int, throws: Boolean) { // User selector makes no distinction by UID val allUids = INTERNAL_UIDS + VERIFIER_UID + NON_VERIFIER_UID - if (throws) { - allUids.forEach { - assertFails { - runMethod(target, it, visible = true, callingUserId, targetUserId) - } - } - } else { - allUids.forEach { - runMethod(target, it, visible = true, callingUserId, targetUserId) - } - } - - // User selector doesn't use QUERY_ALL, so the invisible package should always fail - allUids.forEach { - assertFails { - runMethod(target, it, visible = false, callingUserId, targetUserId) - } - } + runCrossUserMethod(allUids, target, callingUserId, targetUserId, throws) } val callingUserId = 0 @@ -530,24 +517,7 @@ class DomainVerificationEnforcerTest { fun runTestCases(callingUserId: Int, targetUserId: Int, throws: Boolean) { // User selector makes no distinction by UID val allUids = INTERNAL_UIDS + VERIFIER_UID + NON_VERIFIER_UID - if (throws) { - allUids.forEach { - assertFails { - runMethod(target, it, visible = true, callingUserId, targetUserId) - } - } - } else { - allUids.forEach { - runMethod(target, it, visible = true, callingUserId, targetUserId) - } - } - - // User selector doesn't use QUERY_ALL, so the invisible package should always fail - allUids.forEach { - assertFails { - runMethod(target, it, visible = false, callingUserId, targetUserId) - } - } + runCrossUserMethod(allUids, target, callingUserId, targetUserId, throws) } val callingUserId = 0 @@ -591,24 +561,10 @@ class DomainVerificationEnforcerTest { fun runTestCases(callingUserId: Int, targetUserId: Int, throws: Boolean) { // Legacy makes no distinction by UID val allUids = INTERNAL_UIDS + VERIFIER_UID + NON_VERIFIER_UID - if (throws) { - allUids.forEach { - assertFails { - runMethod(target, it, visible = true, callingUserId, targetUserId) - } - } - } else { - allUids.forEach { - runMethod(target, it, visible = true, callingUserId, targetUserId) - } - } - - // Legacy doesn't use QUERY_ALL, so the invisible package should always fail - allUids.forEach { - assertFails { - runMethod(target, it, visible = false, callingUserId, targetUserId) - } - } + // The legacy selector does a silent failure when the user IDs don't match, so it + // cannot verify the non-existent user ID check, as it will not throw an Exception. + runCrossUserMethod(allUids, target, callingUserId, targetUserId, throws, + verifyUserIdCheck = false) } val callingUserId = 0 @@ -664,24 +620,8 @@ class DomainVerificationEnforcerTest { fun runTestCases(callingUserId: Int, targetUserId: Int, throws: Boolean) { // Legacy makes no distinction by UID val allUids = INTERNAL_UIDS + VERIFIER_UID + NON_VERIFIER_UID - if (throws) { - allUids.forEach { - assertFailsLegacy { - runMethod(target, it, visible = true, callingUserId, targetUserId) - } - } - } else { - allUids.forEach { - runMethod(target, it, visible = true, callingUserId, targetUserId) - } - } - - // Legacy doesn't use QUERY_ALL, so the invisible package should always fail - allUids.forEach { - assertFailsLegacy { - runMethod(target, it, visible = false, callingUserId, targetUserId) - } - } + runCrossUserMethod(allUids, target, callingUserId, targetUserId, throws, + assertFailsMethod = ::assertFailsLegacy) } val callingUserId = 0 @@ -723,17 +663,8 @@ class DomainVerificationEnforcerTest { fun runTestCases(callingUserId: Int, targetUserId: Int, throws: Boolean) { // Owner querent makes no distinction by UID val allUids = INTERNAL_UIDS + VERIFIER_UID + NON_VERIFIER_UID - if (throws) { - allUids.forEach { - assertFails { - runMethod(target, it, visible = true, callingUserId, targetUserId) - } - } - } else { - allUids.forEach { - runMethod(target, it, visible = true, callingUserId, targetUserId) - } - } + runCrossUserMethod(allUids, target, callingUserId, targetUserId, throws, + verifyInvisiblePkg = false) } val callingUserId = 0 @@ -801,6 +732,71 @@ class DomainVerificationEnforcerTest { return params.runMethod(target, callingUid, callingUserId, userId, packageName, uuid, proxy) } + private fun runCrossUserMethod( + allUids: Iterable, + target: Any, + callingUserId: Int, + targetUserId: Int, + throws: Boolean, + verifyUserIdCheck: Boolean = true, + verifyInvisiblePkg: Boolean = true, + assertFailsMethod: (() -> Any?) -> Unit = ::assertFails, + ) { + if (throws) { + allUids.forEach { + assertFailsMethod { + // When testing a non-user ID failure, send an invalid user ID. + // This ensures the failure occurs before the user ID check is run. + try { + runMethod(target, it, visible = true, callingUserId, 100) + } catch (e: SecurityException) { + if (verifyUserIdCheck) { + e.message?.let { + if (it.contains("user ID", ignoreCase = true) + || it.contains("100")) { + fail( + "Method should not check user existence before permissions" + ) + } + } + } + + // Rethrow to allow normal fail checking logic to run + throw e + } + } + } + } else { + allUids.forEach { + runMethod(target, it, visible = true, callingUserId, targetUserId) + } + } + + if (verifyInvisiblePkg) { + allUids.forEach { + assertFailsMethod { + runMethod(target, it, visible = false, callingUserId, targetUserId) + } + } + } + + if (verifyUserIdCheck) { + // An invalid target user ID should always fail + allUids.forEach { + assertFailsWith(SecurityException::class) { + runMethod(target, it, visible = true, callingUserId, 100) + } + } + + // An invalid calling user ID should always fail, although this cannot happen in prod + allUids.forEach { + assertFailsWith(SecurityException::class) { + runMethod(target, it, visible = true, 100, targetUserId) + } + } + } + } + private fun assertFails(block: () -> Any?) { try { val value = block() diff --git a/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationManagerApiTest.kt b/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationManagerApiTest.kt index ec4775260f33e..0e74b65d25d57 100644 --- a/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationManagerApiTest.kt +++ b/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationManagerApiTest.kt @@ -314,6 +314,8 @@ class DomainVerificationManagerApiTest { }).apply { setConnection(mockThrowOnUnmocked { whenever(filterAppAccess(anyString(), anyInt(), anyInt())) { false } + whenever(doesUserExist(0)) { true } + whenever(doesUserExist(1)) { true } whenever(scheduleWriteSettings()) // Need to provide an internal UID so some permission checks are ignored diff --git a/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationPackageTest.kt b/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationPackageTest.kt index ae1e82823c834..fe3672d06bc03 100644 --- a/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationPackageTest.kt +++ b/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationPackageTest.kt @@ -370,6 +370,8 @@ class DomainVerificationPackageTest { }).apply { setConnection(mockThrowOnUnmocked { whenever(filterAppAccess(anyString(), anyInt(), anyInt())) { false } + whenever(doesUserExist(0)) { true } + whenever(doesUserExist(1)) { true } whenever(scheduleWriteSettings()) // Need to provide an internal UID so some permission checks are ignored diff --git a/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationSettingsMutationTest.kt b/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationSettingsMutationTest.kt index ee87a5f67dd48..377bae15e2d52 100644 --- a/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationSettingsMutationTest.kt +++ b/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationSettingsMutationTest.kt @@ -265,5 +265,7 @@ class DomainVerificationSettingsMutationTest { // This doesn't check for visibility; that's done in the enforcer test whenever(filterAppAccess(anyString(), anyInt(), anyInt())) { false } + whenever(doesUserExist(0)) { true } + whenever(doesUserExist(10)) { true } } } diff --git a/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationUserSelectionOverrideTest.kt b/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationUserSelectionOverrideTest.kt index f4ab182c6f98d..44c1b8f3fbb95 100644 --- a/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationUserSelectionOverrideTest.kt +++ b/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationUserSelectionOverrideTest.kt @@ -75,6 +75,8 @@ class DomainVerificationUserStateOverrideTest { }).apply { setConnection(mockThrowOnUnmocked { whenever(filterAppAccess(anyString(), anyInt(), anyInt())) { false } + whenever(doesUserExist(0)) { true } + whenever(doesUserExist(1)) { true } whenever(scheduleWriteSettings()) // Need to provide an internal UID so some permission checks are ignored