Add signature checking to AppCompatOverridesService.
As part of SLS allowlist implementation, security team has made
signature checking a requirement for allowlist. This change introduces
an optional string at the start of the flag which is the signature. If
the signature is present, it will be enforced.
The new format for the config string is:
<signature?>~<change-id>:<min-version-code?>:<max-version-code?>:<enabled>
Where signature, min-version-code, max-version-code are all optional
Test: locally on device
Test: atest FrameworksMockingServicesTests:AppCompatOverridesParserTest
Bug: 205279440
Change-Id: Ic78286ff315470ec1a0385231f5033d9c3339bbd
diff --git a/services/core/java/com/android/server/compat/overrides/AppCompatOverridesParser.java b/services/core/java/com/android/server/compat/overrides/AppCompatOverridesParser.java
index 11dc1db..e8762a3 100644
--- a/services/core/java/com/android/server/compat/overrides/AppCompatOverridesParser.java
+++ b/services/core/java/com/android/server/compat/overrides/AppCompatOverridesParser.java
@@ -16,19 +16,24 @@
package com.android.server.compat.overrides;
+import static android.content.pm.PackageManager.CERT_INPUT_SHA256;
import static android.content.pm.PackageManager.MATCH_ANY_USER;
import static java.util.Collections.emptyMap;
import static java.util.Collections.emptySet;
+import android.annotation.Nullable;
import android.app.compat.PackageOverride;
import android.content.pm.ApplicationInfo;
import android.content.pm.PackageManager;
import android.util.ArrayMap;
import android.util.ArraySet;
import android.util.KeyValueListParser;
+import android.util.Pair;
import android.util.Slog;
+import libcore.util.HexEncoding;
+
import java.util.Arrays;
import java.util.Comparator;
import java.util.List;
@@ -178,7 +183,9 @@
* overrides, and returns a map from change ID to {@link PackageOverride} instances to add.
*
* <p>Each change override is in the following format:
- * '<change-id>:<min-version-code?>:<max-version-code?>:<enabled>'.
+ * '<signature?>~<change-id>:<min-version-code?>:<max-version-code?>:<enabled>'.
+ *
+ * <p>The signature is optional, and will only be enforced if included.
*
* <p>If there are multiple overrides that should be added with the same change ID, the one
* that best fits the given {@code versionCode} is added.
@@ -187,14 +194,27 @@
*
* <p>If a change override entry in {@code configStr} is invalid, it will be ignored.
*/
- static Map<Long, PackageOverride> parsePackageOverrides(String configStr, long versionCode,
+ Map<Long, PackageOverride> parsePackageOverrides(String configStr, String packageName,
+ long versionCode,
Set<Long> changeIdsToSkip) {
if (configStr.isEmpty()) {
return emptyMap();
}
PackageOverrideComparator comparator = new PackageOverrideComparator(versionCode);
Map<Long, PackageOverride> overridesToAdd = new ArrayMap<>();
- for (String overrideEntryString : configStr.split(",")) {
+
+ Pair<String, String> signatureAndConfig = extractSignatureFromConfig(configStr);
+ if (signatureAndConfig == null) {
+ return emptyMap();
+ }
+ final String signature = signatureAndConfig.first;
+ final String overridesConfig = signatureAndConfig.second;
+
+ if (!verifySignature(packageName, signature)) {
+ return emptyMap();
+ }
+
+ for (String overrideEntryString : overridesConfig.split(",")) {
List<String> changeIdAndVersions = Arrays.asList(overrideEntryString.split(":", 4));
if (changeIdAndVersions.size() != 4) {
Slog.w(TAG, "Invalid change override entry: " + overrideEntryString);
@@ -252,6 +272,51 @@
}
/**
+ * Extracts the signature from the config string if one exists.
+ *
+ * @param configStr String in the form of <signature?>~<overrideConfig>
+ */
+ @Nullable
+ private static Pair<String, String> extractSignatureFromConfig(String configStr) {
+ final List<String> signatureAndConfig = Arrays.asList(configStr.split("~"));
+
+ if (signatureAndConfig.size() == 1) {
+ // The config string doesn't contain a signature.
+ return Pair.create("", configStr);
+ }
+
+ if (signatureAndConfig.size() > 2) {
+ Slog.w(TAG, "Only one signature per config is supported. Config: " + configStr);
+ return null;
+ }
+
+ return Pair.create(signatureAndConfig.get(0), signatureAndConfig.get(1));
+ }
+
+ /**
+ * Verifies that the specified package was signed with a particular signature.
+ *
+ * @param packageName The package to check.
+ * @param signature The optional signature to verify. If empty, we return true.
+ * @return Whether the package is signed with that signature.
+ */
+ private boolean verifySignature(String packageName, String signature) {
+ try {
+ final boolean signatureValid = signature.isEmpty()
+ || mPackageManager.hasSigningCertificate(packageName,
+ HexEncoding.decode(signature), CERT_INPUT_SHA256);
+
+ if (!signatureValid) {
+ Slog.w(TAG, packageName + " did not have expected signature: " + signature);
+ }
+ return signatureValid;
+ } catch (IllegalArgumentException e) {
+ Slog.w(TAG, "Unable to verify signature " + signature + " for " + packageName, e);
+ return false;
+ }
+ }
+
+ /**
* A {@link Comparator} that compares @link PackageOverride} instances with respect to a
* specified {@code versionCode} as follows:
*
diff --git a/services/core/java/com/android/server/compat/overrides/AppCompatOverridesService.java b/services/core/java/com/android/server/compat/overrides/AppCompatOverridesService.java
index 6aed4b0..7e58b6c 100644
--- a/services/core/java/com/android/server/compat/overrides/AppCompatOverridesService.java
+++ b/services/core/java/com/android/server/compat/overrides/AppCompatOverridesService.java
@@ -196,8 +196,8 @@
private void applyPackageOverrides(String configStr, String packageName, long versionCode,
Set<Long> ownedChangeIds, Set<Long> changeIdsToSkip,
boolean removeOtherOwnedOverrides) {
- Map<Long, PackageOverride> overridesToAdd = AppCompatOverridesParser.parsePackageOverrides(
- configStr, versionCode, changeIdsToSkip);
+ Map<Long, PackageOverride> overridesToAdd = mOverridesParser.parsePackageOverrides(
+ configStr, packageName, versionCode, changeIdsToSkip);
putPackageOverrides(packageName, overridesToAdd);
if (!removeOtherOwnedOverrides) {
@@ -426,5 +426,5 @@
break;
}
}
- };
+ }
}
diff --git a/services/tests/mockingservicestests/src/com/android/server/compat/overrides/AppCompatOverridesParserTest.java b/services/tests/mockingservicestests/src/com/android/server/compat/overrides/AppCompatOverridesParserTest.java
index df19be4..a34db63 100644
--- a/services/tests/mockingservicestests/src/com/android/server/compat/overrides/AppCompatOverridesParserTest.java
+++ b/services/tests/mockingservicestests/src/com/android/server/compat/overrides/AppCompatOverridesParserTest.java
@@ -16,6 +16,8 @@
package com.android.server.compat.overrides;
+import static android.content.pm.PackageManager.CERT_INPUT_SHA256;
+
import static com.google.common.truth.Truth.assertThat;
import static org.mockito.ArgumentMatchers.anyInt;
@@ -31,6 +33,8 @@
import androidx.test.filters.SmallTest;
+import libcore.util.HexEncoding;
+
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
@@ -182,16 +186,18 @@
@Test
public void parsePackageOverrides_emptyConfigNoOwnedChangeIds_returnsEmpty() {
- Map<Long, PackageOverride> result = AppCompatOverridesParser.parsePackageOverrides(
- /* configStr= */ "", /* versionCode= */ 0, /* changeIdsToSkip= */ emptySet());
+ Map<Long, PackageOverride> result = mParser.parsePackageOverrides(
+ /* configStr= */ "", PACKAGE_1, /* versionCode= */ 0, /* changeIdsToSkip= */
+ emptySet());
assertThat(result).isEmpty();
}
@Test
public void parsePackageOverrides_configWithSingleOverride_returnsOverride() {
- Map<Long, PackageOverride> result = AppCompatOverridesParser.parsePackageOverrides(
- /* configStr= */ "123:::true", /* versionCode= */ 5, /* changeIdsToSkip= */
+ Map<Long, PackageOverride> result = mParser.parsePackageOverrides(
+ /* configStr= */ "123:::true", PACKAGE_1, /* versionCode= */
+ 5, /* changeIdsToSkip= */
emptySet());
assertThat(result).hasSize(1);
@@ -201,10 +207,10 @@
@Test
public void parsePackageOverrides_configWithMultipleOverrides_returnsOverrides() {
- Map<Long, PackageOverride> result = AppCompatOverridesParser.parsePackageOverrides(
+ Map<Long, PackageOverride> result = mParser.parsePackageOverrides(
/* configStr= */ "910:3:4:false,78:10::false,12:::false,34:1:2:true,34:10::true,"
+ "56::2:true,56:3:4:false,34:4:8:true,78:6:7:true,910:5::true,"
- + "1112::5:true,56:6::true,1112:6:7:false", /* versionCode= */
+ + "1112::5:true,56:6::true,1112:6:7:false", PACKAGE_1, /* versionCode= */
5, /* changeIdsToSkip= */ emptySet());
assertThat(result).hasSize(6);
@@ -228,8 +234,9 @@
@Test
public void parsePackageOverrides_changeIdsToSkipSpecified_returnsWithoutChangeIdsToSkip() {
ArraySet<Long> changeIdsToSkip = new ArraySet<>(Arrays.asList(34L, 56L));
- Map<Long, PackageOverride> result = AppCompatOverridesParser.parsePackageOverrides(
- /* configStr= */ "12:::true,56:3:7:true", /* versionCode= */ 5, changeIdsToSkip);
+ Map<Long, PackageOverride> result = mParser.parsePackageOverrides(
+ /* configStr= */ "12:::true,56:3:7:true", PACKAGE_1, /* versionCode= */ 5,
+ changeIdsToSkip);
assertThat(result).hasSize(1);
assertThat(result.get(12L)).isEqualTo(
@@ -239,8 +246,77 @@
@Test
public void parsePackageOverrides_changeIdsToSkipContainsAllIds_returnsEmpty() {
ArraySet<Long> changeIdsToSkip = new ArraySet<>(Arrays.asList(12L, 34L));
- Map<Long, PackageOverride> result = AppCompatOverridesParser.parsePackageOverrides(
- /* configStr= */ "12:::true", /* versionCode= */ 5, changeIdsToSkip);
+ Map<Long, PackageOverride> result = mParser.parsePackageOverrides(
+ /* configStr= */ "12:::true", PACKAGE_1, /* versionCode= */ 5, changeIdsToSkip);
+
+ assertThat(result).isEmpty();
+ }
+
+ @Test
+ public void parsePackageOverrides_signatureInvalid() {
+ when(mPackageManager.hasSigningCertificate(PACKAGE_1, HexEncoding.decode("aa"),
+ CERT_INPUT_SHA256)).thenReturn(false);
+
+ Map<Long, PackageOverride> result = mParser.parsePackageOverrides(
+ /* configStr= */ "aa~12:::true,56:::true", PACKAGE_1,
+ /* versionCode= */ 0, /* changeIdsToSkip= */ emptySet());
+
+ assertThat(result).isEmpty();
+ }
+
+ @Test
+ public void parsePackageOverrides_signatureInvalid_oddNumberOfCharacters() {
+ // Valid hex encoding should always be an even number of characters.
+ Map<Long, PackageOverride> result = mParser.parsePackageOverrides(
+ /* configStr= */ "a~12:::true,56:::true", PACKAGE_1,
+ /* versionCode= */ 0, /* changeIdsToSkip= */ emptySet());
+
+ assertThat(result).isEmpty();
+ }
+
+ @Test
+ public void parsePackageOverrides_signatureValid() {
+ when(mPackageManager.hasSigningCertificate(PACKAGE_1, HexEncoding.decode("bb"),
+ CERT_INPUT_SHA256)).thenReturn(true);
+
+ Map<Long, PackageOverride> result = mParser.parsePackageOverrides(
+ /* configStr= */ "bb~12:::true,56:::false", PACKAGE_1,
+ /* versionCode= */ 0, /* changeIdsToSkip= */ emptySet());
+
+ assertThat(result.keySet()).containsExactly(12L, 56L);
+ }
+
+ @Test
+ public void parsePackageOverrides_emptySignature() {
+ Map<Long, PackageOverride> result = mParser.parsePackageOverrides(
+ /* configStr= */ "~12:::true,56:::false", PACKAGE_1,
+ /* versionCode= */ 0, /* changeIdsToSkip= */ emptySet());
+
+ assertThat(result.keySet()).containsExactly(12L, 56L);
+ }
+
+ @Test
+ public void parsePackageOverrides_multipleSignatures() {
+ when(mPackageManager.hasSigningCertificate(PACKAGE_1, HexEncoding.decode("aa"),
+ CERT_INPUT_SHA256)).thenReturn(true);
+ when(mPackageManager.hasSigningCertificate(PACKAGE_1, HexEncoding.decode("bb"),
+ CERT_INPUT_SHA256)).thenReturn(true);
+
+ Map<Long, PackageOverride> result = mParser.parsePackageOverrides(
+ /* configStr= */ "aa~bb~12:::true,56:::false", PACKAGE_1,
+ /* versionCode= */0, /* changeIdsToSkip= */ emptySet());
+
+ assertThat(result).isEmpty();
+ }
+
+ @Test
+ public void parsePackageOverrides_signatureOnly() {
+ when(mPackageManager.hasSigningCertificate(PACKAGE_1, HexEncoding.decode("aa"),
+ CERT_INPUT_SHA256)).thenReturn(true);
+
+ Map<Long, PackageOverride> result = mParser.parsePackageOverrides(
+ /* configStr= */ "aa~", PACKAGE_1,
+ /* versionCode= */ 0, /* changeIdsToSkip= */ emptySet());
assertThat(result).isEmpty();
}
@@ -248,9 +324,9 @@
@Test
public void parsePackageOverrides_someOverridesAreInvalid_returnsWithoutInvalidOverrides() {
// We add a valid entry before and after the invalid ones to make sure they are applied.
- Map<Long, PackageOverride> result = AppCompatOverridesParser.parsePackageOverrides(
+ Map<Long, PackageOverride> result = mParser.parsePackageOverrides(
/* configStr= */ "12:::True,56:1:2:FALSE,56:3:true,78:4:8:true:,C1:::true,910:::no,"
- + "1112:1:ten:true,1112:one:10:true,,1314:7:3:false,34:::",
+ + "1112:1:ten:true,1112:one:10:true,,1314:7:3:false,34:::", PACKAGE_1,
/* versionCode= */ 5, /* changeIdsToSkip= */ emptySet());
assertThat(result).hasSize(2);
