Android/frameworks_base
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Check user ID for domain verification setters

Browse Source 
Validates the calling/target user IDs still exist, to handle race
conditions.

Since the data structures are not locked during the user check, also
adds a call during user creation to wipe the user data if there's any
stale values left. This should never practically happen, but it's a
fairly fast call just to make sure.

Bug: 182416431

Test: atest com.android.server.pm.test.verify.domain

Change-Id: I6cc270fb0e85f0c920b3b71a52e059e818066fac
This commit is contained in:
Winson
2021-03-11 11:53:00 -08:00
parent 173ef94284
commit 3780a17d96
7 changed files with 157 additions and 115 deletions
Download Patch File Download Diff File Expand all files Collapse all files

6
services/core/java/com/android/server/pm/PackageManagerService.java
View File

@@ -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);
}
}

32
services/core/java/com/android/server/pm/verify/domain/DomainVerificationEnforcer.java
View File

@@ -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);
}
}

226
services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationEnforcerTest.kt
View File

@@ -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<AtomicInteger, AtomicInteger, DomainVerificationService> =
{
val callingUidInt = AtomicInteger(-1)
val callingUserIdInt = AtomicInteger(-1)
val makeService: (Context) -> Triple<AtomicInteger, AtomicInteger,
DomainVerificationService> = {
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<String>() } },
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<String>() } },
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<Int>,
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()

2
services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationManagerApiTest.kt
View File

@@ -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

2
services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationPackageTest.kt
View File

@@ -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

2
services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationSettingsMutationTest.kt
View File

@@ -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 }
}
}

2
services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationUserSelectionOverrideTest.kt
View File

@@ -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