From 762658fad1676930dc21e0478ffa694bb1279398 Mon Sep 17 00:00:00 2001 From: Winson Date: Tue, 13 Apr 2021 17:14:51 -0700 Subject: [PATCH] Enforce isSystem for app-link SysConfig Only apps that are preloaded on the system are allowed to be auto approved. This prevents package name takeover for optional packages that are still configured in the XML. Bug: 166697328 Test: atest DomainVerificationPackageTest Test: manual verify pm get-app-links on a device Change-Id: Ic055db3c07ff876600402c3ed0fe66b30cebe09e --- .../domain/DomainVerificationService.java | 30 ++-- .../domain/DomainVerificationEnforcerTest.kt | 1 + .../DomainVerificationManagerApiTest.kt | 1 + .../domain/DomainVerificationPackageTest.kt | 131 ++++++++++++++---- .../DomainVerificationSettingsMutationTest.kt | 1 + ...inVerificationUserSelectionOverrideTest.kt | 1 + 6 files changed, 120 insertions(+), 45 deletions(-) diff --git a/services/core/java/com/android/server/pm/verify/domain/DomainVerificationService.java b/services/core/java/com/android/server/pm/verify/domain/DomainVerificationService.java index f0fdad016d557..a3e1a9cdc5243 100644 --- a/services/core/java/com/android/server/pm/verify/domain/DomainVerificationService.java +++ b/services/core/java/com/android/server/pm/verify/domain/DomainVerificationService.java @@ -892,7 +892,7 @@ public class DomainVerificationService extends SystemService boolean hasAutoVerifyDomains = newDomainsSize > 0; boolean needsBroadcast = - applyImmutableState(pkgName, newStateMap, newAutoVerifyDomains); + applyImmutableState(newPkgSetting, newStateMap, newAutoVerifyDomains); sendBroadcast = hasAutoVerifyDomains && needsBroadcast; @@ -943,7 +943,8 @@ public class DomainVerificationService extends SystemService pkgState = new DomainVerificationPkgState(pkgName, domainSetId, hasAutoVerifyDomains); } - boolean needsBroadcast = applyImmutableState(pkgState, domains); + boolean needsBroadcast = + applyImmutableState(newPkgSetting, pkgState.getStateMap(), domains); if (needsBroadcast && !isPendingOrRestored) { // TODO(b/159952358): Test this behavior // Attempt to preserve user experience by automatically verifying all domains from @@ -990,22 +991,17 @@ public class DomainVerificationService extends SystemService } } - private boolean applyImmutableState(@NonNull DomainVerificationPkgState pkgState, - @NonNull ArraySet autoVerifyDomains) { - return applyImmutableState(pkgState.getPackageName(), pkgState.getStateMap(), - autoVerifyDomains); - } - /** * Applies any immutable state as the final step when adding or migrating state. Currently only - * applies {@link SystemConfig#getLinkedApps()}, which approves all domains for a package. + * applies {@link SystemConfig#getLinkedApps()}, which approves all domains for a system app. * * @return whether or not a broadcast is necessary for this package */ - private boolean applyImmutableState(@NonNull String packageName, + private boolean applyImmutableState(@NonNull PackageSetting pkgSetting, @NonNull ArrayMap stateMap, @NonNull ArraySet autoVerifyDomains) { - if (mSystemConfig.getLinkedApps().contains(packageName)) { + if (pkgSetting.isSystem() + && mSystemConfig.getLinkedApps().contains(pkgSetting.getName())) { int domainsSize = autoVerifyDomains.size(); for (int index = 0; index < domainsSize; index++) { stateMap.put(autoVerifyDomains.valueAt(index), @@ -1318,7 +1314,7 @@ public class DomainVerificationService extends SystemService if (pkgSetting == null || pkgSetting.getPkg() == null) { continue; } - resetDomainState(pkgState, pkgSetting.getPkg()); + resetDomainState(pkgState.getStateMap(), pkgSetting); } } else { int size = packageNames.size(); @@ -1329,7 +1325,7 @@ public class DomainVerificationService extends SystemService if (pkgSetting == null || pkgSetting.getPkg() == null) { continue; } - resetDomainState(pkgState, pkgSetting.getPkg()); + resetDomainState(pkgState.getStateMap(), pkgSetting); } } } @@ -1341,9 +1337,8 @@ public class DomainVerificationService extends SystemService /** * Reset states that are mutable by the domain verification agent. */ - private void resetDomainState(@NonNull DomainVerificationPkgState pkgState, - @NonNull AndroidPackage pkg) { - ArrayMap stateMap = pkgState.getStateMap(); + private void resetDomainState(@NonNull ArrayMap stateMap, + @NonNull PackageSetting pkgSetting) { int size = stateMap.size(); for (int index = size - 1; index >= 0; index--) { Integer state = stateMap.valueAt(index); @@ -1363,7 +1358,8 @@ public class DomainVerificationService extends SystemService } } - applyImmutableState(pkgState, mCollector.collectValidAutoVerifyDomains(pkg)); + applyImmutableState(pkgSetting, stateMap, + mCollector.collectValidAutoVerifyDomains(pkgSetting.getPkg())); } @Override 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 2d852e5017b45..886b2e03a1042 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 @@ -338,6 +338,7 @@ class DomainVerificationEnforcerTest { whenever(readUserState(0)) { PackageUserState() } whenever(readUserState(1)) { PackageUserState() } whenever(getInstantApp(anyInt())) { false } + whenever(isSystem()) { false } } } 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 3838f68fcf227..0fe39132f865d 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 @@ -527,6 +527,7 @@ class DomainVerificationManagerApiTest { whenever(firstInstallTime) { 0L } whenever(readUserState(0)) { pkgUserState0() } whenever(readUserState(1)) { pkgUserState1() } + whenever(isSystem()) { false } } private fun DomainVerificationService.addPackages(vararg pkgSettings: PackageSetting) = 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 1097c45f8a9ff..8540b8afdd645 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 @@ -100,6 +100,70 @@ class DomainVerificationPackageTest { .containsExactly(pkg1.getName()) } + @Test + fun addPackageSystemConfigured() { + val pkg1 = mockPkgSetting(PKG_ONE, UUID_ONE, SIGNATURE_ONE, isSystemApp = false) + val pkg2 = mockPkgSetting(PKG_TWO, UUID_TWO, SIGNATURE_TWO, isSystemApp = true) + + val service = makeService( + systemConfiguredPackageNames = ArraySet(setOf(pkg1.getName(), pkg2.getName())), + pkg1, pkg2 + ) + service.addPackage(pkg1) + service.addPackage(pkg2) + + service.getInfo(pkg1.getName()).apply { + assertThat(packageName).isEqualTo(pkg1.getName()) + assertThat(identifier).isEqualTo(pkg1.domainSetId) + assertThat(hostToStateMap).containsExactlyEntriesIn( + mapOf( + DOMAIN_1 to STATE_NO_RESPONSE, + DOMAIN_2 to STATE_NO_RESPONSE, + ) + ) + } + + service.getUserState(pkg1.getName()).apply { + assertThat(packageName).isEqualTo(pkg1.getName()) + assertThat(identifier).isEqualTo(pkg1.domainSetId) + assertThat(isLinkHandlingAllowed).isEqualTo(true) + assertThat(user.identifier).isEqualTo(USER_ID) + assertThat(hostToStateMap).containsExactlyEntriesIn( + mapOf( + DOMAIN_1 to DOMAIN_STATE_NONE, + DOMAIN_2 to DOMAIN_STATE_NONE, + ) + ) + } + + service.getInfo(pkg2.getName()).apply { + assertThat(packageName).isEqualTo(pkg2.getName()) + assertThat(identifier).isEqualTo(pkg2.domainSetId) + assertThat(hostToStateMap).containsExactlyEntriesIn( + mapOf( + DOMAIN_1 to STATE_UNMODIFIABLE, + DOMAIN_2 to STATE_UNMODIFIABLE, + ) + ) + } + + service.getUserState(pkg2.getName()).apply { + assertThat(packageName).isEqualTo(pkg2.getName()) + assertThat(identifier).isEqualTo(pkg2.domainSetId) + assertThat(isLinkHandlingAllowed).isEqualTo(true) + assertThat(user.identifier).isEqualTo(USER_ID) + assertThat(hostToStateMap).containsExactlyEntriesIn( + mapOf( + DOMAIN_1 to DOMAIN_STATE_VERIFIED, + DOMAIN_2 to DOMAIN_STATE_VERIFIED, + ) + ) + } + + assertThat(service.queryValidVerificationPackageNames()) + .containsExactly(pkg1.getName(), pkg2.getName()) + } + @Test fun addPackageRestoredMatchingSignature() { // language=XML @@ -457,42 +521,52 @@ class DomainVerificationPackageTest { getDomainVerificationUserState(pkgName, USER_ID) .also { assertThat(it).isNotNull() }!! + private fun makeService( + systemConfiguredPackageNames: ArraySet = ArraySet(), + vararg pkgSettings: PackageSetting + ) = makeService(systemConfiguredPackageNames = systemConfiguredPackageNames) { + pkgName -> pkgSettings.find { pkgName == it.getName() } + } + private fun makeService(vararg pkgSettings: PackageSetting) = - makeService { pkgName -> pkgSettings.find { pkgName == it.getName()} } + makeService { pkgName -> pkgSettings.find { pkgName == it.getName() } } - private fun makeService(pkgSettingFunction: (String) -> PackageSetting? = { null }) = - DomainVerificationService(mockThrowOnUnmocked { - // Assume the test has every permission necessary - whenever(enforcePermission(anyString(), anyInt(), anyInt(), anyString())) - whenever(checkPermission(anyString(), anyInt(), anyInt())) { - PackageManager.PERMISSION_GRANTED - } - }, mockThrowOnUnmocked { - whenever(linkedApps) { ArraySet() } - }, mockThrowOnUnmocked { - whenever(isChangeEnabledInternalNoLogging(anyLong(), any())) { true } - }).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 - whenever(callingUid) { Process.ROOT_UID } - whenever(callingUserId) { 0 } - - mockPackageSettings { - pkgSettingFunction(it) - } - }) + private fun makeService( + systemConfiguredPackageNames: ArraySet = ArraySet(), + pkgSettingFunction: (String) -> PackageSetting? = { null } + ) = DomainVerificationService(mockThrowOnUnmocked { + // Assume the test has every permission necessary + whenever(enforcePermission(anyString(), anyInt(), anyInt(), anyString())) + whenever(checkPermission(anyString(), anyInt(), anyInt())) { + PackageManager.PERMISSION_GRANTED } + }, mockThrowOnUnmocked { + whenever(this.linkedApps) { systemConfiguredPackageNames } + }, mockThrowOnUnmocked { + whenever(isChangeEnabledInternalNoLogging(anyLong(), any())) { true } + }).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 + whenever(callingUid) { Process.ROOT_UID } + whenever(callingUserId) { 0 } + + mockPackageSettings { + pkgSettingFunction(it) + } + }) + } private fun mockPkgSetting( pkgName: String, domainSetId: UUID, signature: String, - domains: List = listOf(DOMAIN_1, DOMAIN_2) + domains: List = listOf(DOMAIN_1, DOMAIN_2), + isSystemApp: Boolean = false ) = mockThrowOnUnmocked { val pkg = mockThrowOnUnmocked { whenever(packageName) { pkgName } @@ -528,5 +602,6 @@ class DomainVerificationPackageTest { whenever(firstInstallTime) { 0L } whenever(readUserState(USER_ID)) { PackageUserState() } whenever(signatures) { arrayOf(Signature(signature)) } + whenever(isSystem) { isSystemApp } } } 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 26e8d286cabe9..6a75795a0a885 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 @@ -238,6 +238,7 @@ class DomainVerificationSettingsMutationTest { whenever(readUserState(0)) { PackageUserState() } whenever(readUserState(10)) { PackageUserState() } whenever(getInstantApp(anyInt())) { false } + whenever(isSystem()) { false } } } 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 52ae7a5356571..3e2853cf24a10 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 @@ -145,6 +145,7 @@ class DomainVerificationUserStateOverrideTest { whenever(firstInstallTime) { 0L } whenever(readUserState(0)) { PackageUserState() } whenever(readUserState(1)) { PackageUserState() } + whenever(isSystem()) { false } } @Test