From 0a7bbec89e51fa1142b9474c67a5b57ba60648df Mon Sep 17 00:00:00 2001 From: Lu Liu Date: Tue, 21 Jun 2022 22:19:58 +0000 Subject: [PATCH 1/6] Add security Policy for verifying signature using sha-256 hash --- .../java/io/grpc/binder/SecurityPolicies.java | 116 +++++++++++++++- .../io/grpc/binder/SecurityPoliciesTest.java | 129 ++++++++++++++++++ 2 files changed, 238 insertions(+), 7 deletions(-) diff --git a/binder/src/main/java/io/grpc/binder/SecurityPolicies.java b/binder/src/main/java/io/grpc/binder/SecurityPolicies.java index 0fd5430ba61..4c8c457c853 100644 --- a/binder/src/main/java/io/grpc/binder/SecurityPolicies.java +++ b/binder/src/main/java/io/grpc/binder/SecurityPolicies.java @@ -24,12 +24,15 @@ import android.os.Build; import android.os.Process; import com.google.common.base.Preconditions; +import com.google.common.base.Predicate; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; +import com.google.common.hash.Hashing; import com.google.errorprone.annotations.CheckReturnValue; import io.grpc.ExperimentalApi; import io.grpc.Status; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collection; import java.util.Iterator; import java.util.List; @@ -40,6 +43,7 @@ public final class SecurityPolicies { private static final int MY_UID = Process.myUid(); + private static final int SHA_256_BYTES_LENGTH = 32; private SecurityPolicies() {} @@ -83,6 +87,21 @@ public static SecurityPolicy hasSignature( packageManager, packageName, ImmutableList.of(requiredSignature)); } + /** + * Creates {@link SecurityPolicy} which checks if the SHA-256 hash of the package signature + * matches {@code requiredSignatureSha256Hash}. + * + * @param packageName the package name of the allowed package. + * @param requiredSignatureSha256Hash the SHA-256 digest of the signature of the allowed package. + * @throws NullPointerException if any of the inputs are {@code null}. + * @throws IllegalArgumentException if {@code requiredSignatureSha256Hash} is not of length 32. + */ + public static SecurityPolicy hasSignatureSha256Hash( + PackageManager packageManager, String packageName, byte[] requiredSignatureSha256Hash) { + return oneOfSignatureSha256Hash( + packageManager, packageName, ImmutableList.of(requiredSignatureSha256Hash)); + } + /** * Creates a {@link SecurityPolicy} which checks if the package signature * matches any of {@code requiredSignatures}. @@ -116,6 +135,39 @@ public Status checkAuthorization(int uid) { }; } + /** + * Creates {@link SecurityPolicy} which checks if the SHA-256 hash of the package signature + * matches any of {@code requiredSignatureSha256Hashes}. + * + * @param packageName the package name of the allowed package. + * @param requiredSignatureSha256Hashes the SHA-256 digests of the signatures of the allowed + * package. + * @throws NullPointerException if any of the inputs are {@code null}. + * @throws IllegalArgumentException if {@code requiredSignatureSha256Hashes} is empty, or if any + * of the {@code requiredSignatureSha256Hashes} are not of length 32. + */ + public static SecurityPolicy oneOfSignatureSha256Hash( + PackageManager packageManager, + String packageName, + ImmutableList requiredSignatureSha256Hashes) { + Preconditions.checkNotNull(packageManager); + Preconditions.checkNotNull(packageName); + Preconditions.checkNotNull(requiredSignatureSha256Hashes); + Preconditions.checkArgument(!requiredSignatureSha256Hashes.isEmpty()); + for (byte[] requiredSignatureSha256Hash : requiredSignatureSha256Hashes) { + Preconditions.checkNotNull(requiredSignatureSha256Hash); + Preconditions.checkArgument(requiredSignatureSha256Hash.length == SHA_256_BYTES_LENGTH); + } + + return new SecurityPolicy() { + @Override + public Status checkAuthorization(int uid) { + return checkUidSha256Signature( + packageManager, uid, packageName, requiredSignatureSha256Hashes); + } + }; + } + private static Status checkUidSignature( PackageManager packageManager, int uid, @@ -132,7 +184,7 @@ private static Status checkUidSignature( continue; } packageNameMatched = true; - if (checkPackageSignature(packageManager, pkg, requiredSignatures)) { + if (checkPackageSignature(packageManager, pkg, requiredSignatures::contains)) { return Status.OK; } } @@ -141,19 +193,50 @@ private static Status checkUidSignature( + packageNameMatched); } + private static Status checkUidSha256Signature( + PackageManager packageManager, + int uid, + String packageName, + ImmutableList requiredSignatureSha256Hashes) { + String[] packages = packageManager.getPackagesForUid(uid); + if (packages == null) { + return Status.UNAUTHENTICATED.withDescription( + "Rejected by (SHA-256 hash signature check) security policy"); + } + boolean packageNameMatched = false; + for (String pkg : packages) { + if (!packageName.equals(pkg)) { + continue; + } + packageNameMatched = true; + if (checkPackageSignature( + packageManager, + pkg, + (signature) -> + checkSignatureSha256HashesMatch(signature, requiredSignatureSha256Hashes))) { + return Status.OK; + } + } + return Status.PERMISSION_DENIED.withDescription( + "Rejected by (SHA-256 hash signature check) security policy. Package name matched: " + + packageNameMatched); + } + /** * Checks if the signature of {@code packageName} matches one of the given signatures. * * @param packageName the package to be checked - * @param requiredSignatures list of signatures. - * @return {@code true} if {@code packageName} has a matching signature. + * @param signatureCheckFunction {@link Function} that takes a signature and verifies if it + * satisfies any signature constraints + * return {@code true} if {@code packageName} has a signature that satisfies {@code + * signatureCheckFunction}. */ @SuppressWarnings("deprecation") // For PackageInfo.signatures @SuppressLint("PackageManagerGetSignatures") // We only allow 1 signature. private static boolean checkPackageSignature( PackageManager packageManager, String packageName, - ImmutableList requiredSignatures) { + Predicate signatureCheckFunction) { PackageInfo packageInfo; try { if (Build.VERSION.SDK_INT >= 28) { @@ -168,7 +251,7 @@ private static boolean checkPackageSignature( : packageInfo.signingInfo.getSigningCertificateHistory(); for (Signature signature : signatures) { - if (requiredSignatures.contains(signature)) { + if (signatureCheckFunction.apply(signature)) { return true; } } @@ -180,7 +263,7 @@ private static boolean checkPackageSignature( return false; } - if (requiredSignatures.contains(packageInfo.signatures[0])) { + if (signatureCheckFunction.apply(packageInfo.signatures[0])) { return true; } } @@ -317,4 +400,23 @@ private static Status checkPermissions( return Status.OK; } -} + + /** + * Checks if the SHA-256 hash of the {@code signature} matches one of the {@code + * expectedSignatureSha256Hashes}. + */ + private static boolean checkSignatureSha256HashesMatch( + Signature signature, List expectedSignatureSha256Hashes) { + for (byte[] hash : expectedSignatureSha256Hashes) { + if (Arrays.equals(hash, getSha256Hash(signature))) { + return true; + } + } + return false; + } + + /** Returns SHA-256 hash of the provided signature. */ + private static byte[] getSha256Hash(Signature signature) { + return Hashing.sha256().hashBytes(signature.toByteArray()).asBytes(); + } +} \ No newline at end of file diff --git a/binder/src/test/java/io/grpc/binder/SecurityPoliciesTest.java b/binder/src/test/java/io/grpc/binder/SecurityPoliciesTest.java index a17162325e6..a26d4dd58af 100644 --- a/binder/src/test/java/io/grpc/binder/SecurityPoliciesTest.java +++ b/binder/src/test/java/io/grpc/binder/SecurityPoliciesTest.java @@ -32,6 +32,7 @@ import androidx.test.core.app.ApplicationProvider; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; +import com.google.common.hash.Hashing; import io.grpc.Status; import java.util.HashMap; import java.util.concurrent.atomic.AtomicInteger; @@ -435,4 +436,132 @@ public Status checkAuthorization(int uid) { return Status.OK; } } + + @Test + public void testHasSignatureSha256Hash_succeedsIfPackageNameAndSignatureHashMatch() + throws Exception { + PackageInfo info = + newBuilder().setPackageName(OTHER_UID_PACKAGE_NAME).setSignatures(SIG2).build(); + installPackages(OTHER_UID, info); + + policy = + SecurityPolicies.hasSignatureSha256Hash( + packageManager, OTHER_UID_PACKAGE_NAME, getSha256Hash(SIG2)); + + // THEN UID for package that has SIG2 will be authorized + assertThat(policy.checkAuthorization(OTHER_UID).getCode()).isEqualTo(Status.OK.getCode()); + } + + @Test + public void testHasSignatureSha256Hash_failsIfPackageNameDoesNotMatch() throws Exception { + PackageInfo info1 = + newBuilder().setPackageName(appContext.getPackageName()).setSignatures(SIG1).build(); + installPackages(MY_UID, info1); + + PackageInfo info2 = + newBuilder() + .setPackageName(OTHER_UID_SAME_SIGNATURE_PACKAGE_NAME) + .setSignatures(SIG1) + .build(); + installPackages(OTHER_UID_SAME_SIGNATURE, info2); + + policy = + SecurityPolicies.hasSignatureSha256Hash( + packageManager, appContext.getPackageName(), getSha256Hash(SIG1)); + + // THEN UID for package that has SIG1 but different package name will not be authorized + assertThat(policy.checkAuthorization(OTHER_UID_SAME_SIGNATURE).getCode()) + .isEqualTo(Status.PERMISSION_DENIED.getCode()); + } + + @Test + public void testHasSignatureSha256Hash_failsIfSignatureHashDoesNotMatch() throws Exception { + PackageInfo info = + newBuilder().setPackageName(OTHER_UID_PACKAGE_NAME).setSignatures(SIG2).build(); + installPackages(OTHER_UID, info); + + policy = + SecurityPolicies.hasSignatureSha256Hash( + packageManager, OTHER_UID_PACKAGE_NAME, getSha256Hash(SIG1)); + + // THEN UID for package that doesn't have SIG1 will not be authorized + assertThat(policy.checkAuthorization(OTHER_UID).getCode()) + .isEqualTo(Status.PERMISSION_DENIED.getCode()); + } + + @Test + public void testOneOfSignatureSha256Hash_succeedsIfPackageNameAndSignatureHashMatch() + throws Exception { + PackageInfo info = + newBuilder().setPackageName(OTHER_UID_PACKAGE_NAME).setSignatures(SIG2).build(); + installPackages(OTHER_UID, info); + + policy = + SecurityPolicies.oneOfSignatureSha256Hash( + packageManager, OTHER_UID_PACKAGE_NAME, ImmutableList.of(getSha256Hash(SIG2))); + + // THEN UID for package that has SIG2 will be authorized + assertThat(policy.checkAuthorization(OTHER_UID).getCode()).isEqualTo(Status.OK.getCode()); + } + + @Test + public void testOneOfSignatureSha256Hash_succeedsIfPackageNameAndOneOfSignatureHashesMatch() + throws Exception { + PackageInfo info = + newBuilder().setPackageName(OTHER_UID_PACKAGE_NAME).setSignatures(SIG2).build(); + installPackages(OTHER_UID, info); + + policy = + SecurityPolicies.oneOfSignatureSha256Hash( + packageManager, + OTHER_UID_PACKAGE_NAME, + ImmutableList.of(getSha256Hash(SIG1), getSha256Hash(SIG2))); + + // THEN UID for package that has SIG2 will be authorized + assertThat(policy.checkAuthorization(OTHER_UID).getCode()).isEqualTo(Status.OK.getCode()); + } + + @Test + public void + testOneOfSignatureSha256Hash_failsIfPackageNameDoNotMatchAndOneOfSignatureHashesMatch() + throws Exception { + PackageInfo info = + newBuilder().setPackageName(OTHER_UID_PACKAGE_NAME).setSignatures(SIG2).build(); + installPackages(OTHER_UID, info); + + policy = + SecurityPolicies.oneOfSignatureSha256Hash( + packageManager, + appContext.getPackageName(), + ImmutableList.of(getSha256Hash(SIG1), getSha256Hash(SIG2))); + + // THEN UID for package that has SIG2 but different package name will not be authorized + assertThat(policy.checkAuthorization(OTHER_UID).getCode()) + .isEqualTo(Status.PERMISSION_DENIED.getCode()); + } + + @Test + public void testOneOfSignatureSha256Hash_failsIfPackageNameMatchAndOneOfSignatureHashesNotMatch() + throws Exception { + PackageInfo info = + newBuilder() + .setPackageName(OTHER_UID_PACKAGE_NAME) + .setSignatures(new Signature("1234")) + .build(); + installPackages(OTHER_UID, info); + + policy = + SecurityPolicies.oneOfSignatureSha256Hash( + packageManager, + appContext.getPackageName(), + ImmutableList.of(getSha256Hash(SIG1), getSha256Hash(SIG2))); + + // THEN UID for package that doesn't have SIG1 or SIG2 will not be authorized + assertThat(policy.checkAuthorization(OTHER_UID).getCode()) + .isEqualTo(Status.PERMISSION_DENIED.getCode()); + } + + private static byte[] getSha256Hash(Signature signature) { + return Hashing.sha256().hashBytes(signature.toByteArray()).asBytes(); + } } From 0793bb6d725bfce418d596464ad9b8576c8f5fc7 Mon Sep 17 00:00:00 2001 From: Lu Liu Date: Wed, 22 Jun 2022 18:35:26 +0000 Subject: [PATCH 2/6] precompute hash value to avoid compute multiple times --- binder/src/main/java/io/grpc/binder/SecurityPolicies.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/binder/src/main/java/io/grpc/binder/SecurityPolicies.java b/binder/src/main/java/io/grpc/binder/SecurityPolicies.java index 4c8c457c853..b7f8c411e96 100644 --- a/binder/src/main/java/io/grpc/binder/SecurityPolicies.java +++ b/binder/src/main/java/io/grpc/binder/SecurityPolicies.java @@ -407,8 +407,9 @@ private static Status checkPermissions( */ private static boolean checkSignatureSha256HashesMatch( Signature signature, List expectedSignatureSha256Hashes) { + byte[] signatureHash = getSha256Hash(signature); for (byte[] hash : expectedSignatureSha256Hashes) { - if (Arrays.equals(hash, getSha256Hash(signature))) { + if (Arrays.equals(hash, signatureHash)) { return true; } } From ef69d45292de16a6f7f69b5d034598a6dced4300 Mon Sep 17 00:00:00 2001 From: Lu Liu Date: Thu, 23 Jun 2022 17:42:23 +0000 Subject: [PATCH 3/6] function > predicate in doc comment --- binder/src/main/java/io/grpc/binder/SecurityPolicies.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/binder/src/main/java/io/grpc/binder/SecurityPolicies.java b/binder/src/main/java/io/grpc/binder/SecurityPolicies.java index b7f8c411e96..edecf42f1e7 100644 --- a/binder/src/main/java/io/grpc/binder/SecurityPolicies.java +++ b/binder/src/main/java/io/grpc/binder/SecurityPolicies.java @@ -226,7 +226,7 @@ private static Status checkUidSha256Signature( * Checks if the signature of {@code packageName} matches one of the given signatures. * * @param packageName the package to be checked - * @param signatureCheckFunction {@link Function} that takes a signature and verifies if it + * @param signatureCheckFunction {@link Predicate} that takes a signature and verifies if it * satisfies any signature constraints * return {@code true} if {@code packageName} has a signature that satisfies {@code * signatureCheckFunction}. From 7174b91e6621ce5e93ae605845c178d6667942b3 Mon Sep 17 00:00:00 2001 From: Lu Liu Date: Thu, 23 Jun 2022 21:14:07 +0000 Subject: [PATCH 4/6] change colletion to list --- binder/src/main/java/io/grpc/binder/SecurityPolicies.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/binder/src/main/java/io/grpc/binder/SecurityPolicies.java b/binder/src/main/java/io/grpc/binder/SecurityPolicies.java index edecf42f1e7..de469460870 100644 --- a/binder/src/main/java/io/grpc/binder/SecurityPolicies.java +++ b/binder/src/main/java/io/grpc/binder/SecurityPolicies.java @@ -114,7 +114,7 @@ public static SecurityPolicy hasSignatureSha256Hash( public static SecurityPolicy oneOfSignatures( PackageManager packageManager, String packageName, - Collection requiredSignatures) { + List requiredSignatures) { Preconditions.checkNotNull(packageManager, "packageManager"); Preconditions.checkNotNull(packageName, "packageName"); Preconditions.checkNotNull(requiredSignatures, "requiredSignatures"); @@ -420,4 +420,4 @@ private static boolean checkSignatureSha256HashesMatch( private static byte[] getSha256Hash(Signature signature) { return Hashing.sha256().hashBytes(signature.toByteArray()).asBytes(); } -} \ No newline at end of file +} From 64600cc2c9d5ca8ac65efe21c29d0a70ed6b4e2d Mon Sep 17 00:00:00 2001 From: Lu Liu Date: Thu, 23 Jun 2022 21:38:40 +0000 Subject: [PATCH 5/6] revert last change: collection > list, change immutablelist to list and copy list to immutablelist --- .../src/main/java/io/grpc/binder/SecurityPolicies.java | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/binder/src/main/java/io/grpc/binder/SecurityPolicies.java b/binder/src/main/java/io/grpc/binder/SecurityPolicies.java index de469460870..dbdf24983d1 100644 --- a/binder/src/main/java/io/grpc/binder/SecurityPolicies.java +++ b/binder/src/main/java/io/grpc/binder/SecurityPolicies.java @@ -114,7 +114,7 @@ public static SecurityPolicy hasSignatureSha256Hash( public static SecurityPolicy oneOfSignatures( PackageManager packageManager, String packageName, - List requiredSignatures) { + Collection requiredSignatures) { Preconditions.checkNotNull(packageManager, "packageManager"); Preconditions.checkNotNull(packageName, "packageName"); Preconditions.checkNotNull(requiredSignatures, "requiredSignatures"); @@ -149,11 +149,14 @@ public Status checkAuthorization(int uid) { public static SecurityPolicy oneOfSignatureSha256Hash( PackageManager packageManager, String packageName, - ImmutableList requiredSignatureSha256Hashes) { + List requiredSignatureSha256Hashes) { Preconditions.checkNotNull(packageManager); Preconditions.checkNotNull(packageName); Preconditions.checkNotNull(requiredSignatureSha256Hashes); Preconditions.checkArgument(!requiredSignatureSha256Hashes.isEmpty()); + + ImmutableList requiredSignaturesHashesImmutable = ImmutableList.copyOf(requiredSignatureSha256Hashes); + for (byte[] requiredSignatureSha256Hash : requiredSignatureSha256Hashes) { Preconditions.checkNotNull(requiredSignatureSha256Hash); Preconditions.checkArgument(requiredSignatureSha256Hash.length == SHA_256_BYTES_LENGTH); @@ -163,7 +166,7 @@ public static SecurityPolicy oneOfSignatureSha256Hash( @Override public Status checkAuthorization(int uid) { return checkUidSha256Signature( - packageManager, uid, packageName, requiredSignatureSha256Hashes); + packageManager, uid, packageName, requiredSignaturesHashesImmutable); } }; } From c8fd05a1c29ea0e6fe75849a5b1391b760efb2b4 Mon Sep 17 00:00:00 2001 From: Lu Liu Date: Thu, 23 Jun 2022 23:50:21 +0000 Subject: [PATCH 6/6] deep copy byte[] list --- binder/src/main/java/io/grpc/binder/SecurityPolicies.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/binder/src/main/java/io/grpc/binder/SecurityPolicies.java b/binder/src/main/java/io/grpc/binder/SecurityPolicies.java index dbdf24983d1..25f215be696 100644 --- a/binder/src/main/java/io/grpc/binder/SecurityPolicies.java +++ b/binder/src/main/java/io/grpc/binder/SecurityPolicies.java @@ -155,12 +155,14 @@ public static SecurityPolicy oneOfSignatureSha256Hash( Preconditions.checkNotNull(requiredSignatureSha256Hashes); Preconditions.checkArgument(!requiredSignatureSha256Hashes.isEmpty()); - ImmutableList requiredSignaturesHashesImmutable = ImmutableList.copyOf(requiredSignatureSha256Hashes); - + ImmutableList.Builder immutableListBuilder = ImmutableList.builder(); for (byte[] requiredSignatureSha256Hash : requiredSignatureSha256Hashes) { Preconditions.checkNotNull(requiredSignatureSha256Hash); Preconditions.checkArgument(requiredSignatureSha256Hash.length == SHA_256_BYTES_LENGTH); + immutableListBuilder.add( + Arrays.copyOf(requiredSignatureSha256Hash, requiredSignatureSha256Hash.length)); } + ImmutableList requiredSignaturesHashesImmutable = immutableListBuilder.build(); return new SecurityPolicy() { @Override