Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 6c406256 authored by Lucas Silva's avatar Lucas Silva
Browse files

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
parent ef38b3af

services/core/java/com/android/server/compat/overrides/AppCompatOverridesParser.java

+68 −3
Original line number Diff line number Diff line
@@ -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 @@ final class AppCompatOverridesParser { 
     * 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 @@ final class AppCompatOverridesParser { 
     *

     * <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);
@@ -251,6 +271,51 @@ final class AppCompatOverridesParser { 
        return overridesToAdd;

    }



    /**

     * 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:

services/core/java/com/android/server/compat/overrides/AppCompatOverridesService.java

+3 −3
Original line number Diff line number Diff line
@@ -196,8 +196,8 @@ public final class AppCompatOverridesService { 
    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 @@ public final class AppCompatOverridesService { 
                    break;

            }

        }

    };

    }

}

services/tests/mockingservicestests/src/com/android/server/compat/overrides/AppCompatOverridesParserTest.java

+88 −12
Original line number Diff line number Diff line
@@ -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 android.util.ArraySet; 


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 @@ public class AppCompatOverridesParserTest { 


    @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 @@ public class AppCompatOverridesParserTest { 


    @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 @@ public class AppCompatOverridesParserTest { 
    @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 @@ public class AppCompatOverridesParserTest { 
    @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 @@ public class AppCompatOverridesParserTest { 
    @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);