From e36fe6bf46b832178bef636fdddc98c1407ca869 Mon Sep 17 00:00:00 2001 From: Janis Danisevskis Date: Tue, 10 Aug 2021 17:47:43 -0700 Subject: [PATCH 1/2] Keystore 2.0 SPI: Fix contract between equals and hashCode 2 This fixes the contract between equals and hashCode in AndroidKeystorePublicKey. The previous fix made only a reference comparisson between certificate blobs. In this patch java.util.Arrays is used to compare and compute the hash of the array. Bug: 196118021 Test: See following CL. Change-Id: I2b8b7e740fb377de39fd21f763e15cb00024b2fc --- .../security/keystore2/AndroidKeyStorePublicKey.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/keystore/java/android/security/keystore2/AndroidKeyStorePublicKey.java b/keystore/java/android/security/keystore2/AndroidKeyStorePublicKey.java index 4842984e8c6ab..0b3be327b521d 100644 --- a/keystore/java/android/security/keystore2/AndroidKeyStorePublicKey.java +++ b/keystore/java/android/security/keystore2/AndroidKeyStorePublicKey.java @@ -23,7 +23,7 @@ import android.system.keystore2.KeyDescriptor; import android.system.keystore2.KeyMetadata; import java.security.PublicKey; -import java.util.Objects; +import java.util.Arrays; /** * {@link PublicKey} backed by Android Keystore. @@ -62,8 +62,8 @@ public abstract class AndroidKeyStorePublicKey extends AndroidKeyStoreKey implem int result = 1; result = prime * result + super.hashCode(); - result = prime * result + ((mCertificate == null) ? 0 : mCertificate.hashCode()); - result = prime * result + ((mCertificateChain == null) ? 0 : mCertificateChain.hashCode()); + result = prime * result + Arrays.hashCode(mCertificate); + result = prime * result + Arrays.hashCode(mCertificateChain); return result; } @@ -83,7 +83,7 @@ public abstract class AndroidKeyStorePublicKey extends AndroidKeyStoreKey implem */ final AndroidKeyStorePublicKey other = (AndroidKeyStorePublicKey) obj; - return Objects.equals(mCertificate, other.mCertificate) && Objects.equals(mCertificateChain, + return Arrays.equals(mCertificate, other.mCertificate) && Arrays.equals(mCertificateChain, other.mCertificateChain); } } From 5fe5f2def000915e514ca8286fe39e06c52bf5eb Mon Sep 17 00:00:00 2001 From: Janis Danisevskis Date: Wed, 1 Sep 2021 14:47:20 -0700 Subject: [PATCH 2/2] Test for contract between AndroidKeyStoreKey hash and equals. Test: atest KeystoreTests Bug: 196118021 Merged-In: Ic6e60752faa986debe3d325f54242cffaa03b336 Change-Id: Ic6e60752faa986debe3d325f54242cffaa03b336 --- .../keystore2/AndroidKeyStoreKey.java | 8 +- .../keystore2/AndroidKeyStoreSpiTest.java | 114 ++++++++++++++++++ 2 files changed, 121 insertions(+), 1 deletion(-) diff --git a/keystore/java/android/security/keystore2/AndroidKeyStoreKey.java b/keystore/java/android/security/keystore2/AndroidKeyStoreKey.java index b24a22dbc0ec2..16f732f63bf7e 100644 --- a/keystore/java/android/security/keystore2/AndroidKeyStoreKey.java +++ b/keystore/java/android/security/keystore2/AndroidKeyStoreKey.java @@ -22,6 +22,8 @@ import android.system.keystore2.Authorization; import android.system.keystore2.Domain; import android.system.keystore2.KeyDescriptor; +import com.android.internal.annotations.VisibleForTesting; + import java.security.Key; /** @@ -46,7 +48,11 @@ public class AndroidKeyStoreKey implements Key { // We do not include this member in comparisons. private final KeyStoreSecurityLevel mSecurityLevel; - AndroidKeyStoreKey(@NonNull KeyDescriptor descriptor, + /** + * @hide + */ + @VisibleForTesting + public AndroidKeyStoreKey(@NonNull KeyDescriptor descriptor, long keyId, @NonNull Authorization[] authorizations, @NonNull String algorithm, diff --git a/keystore/tests/src/android/security/keystore2/AndroidKeyStoreSpiTest.java b/keystore/tests/src/android/security/keystore2/AndroidKeyStoreSpiTest.java index 1bd3069f483a2..f96c39c879fd6 100644 --- a/keystore/tests/src/android/security/keystore2/AndroidKeyStoreSpiTest.java +++ b/keystore/tests/src/android/security/keystore2/AndroidKeyStoreSpiTest.java @@ -24,7 +24,13 @@ import static org.mockito.Mockito.when; import android.security.KeyStore2; import android.security.KeyStoreException; +import android.security.KeyStoreSecurityLevel; +import android.system.keystore2.Authorization; +import android.system.keystore2.Domain; +import android.system.keystore2.KeyDescriptor; +import android.system.keystore2.KeyMetadata; +import org.junit.Assert; import org.junit.Before; import org.junit.Test; import org.mockito.Mock; @@ -52,4 +58,112 @@ public class AndroidKeyStoreSpiTest { verify(mKeystore2).list(anyInt(), anyLong()); } + @Mock + private KeyStoreSecurityLevel mKeystoreSecurityLevel; + + private static KeyDescriptor descriptor() { + final KeyDescriptor keyDescriptor = new KeyDescriptor(); + keyDescriptor.alias = "key"; + keyDescriptor.blob = null; + keyDescriptor.domain = Domain.APP; + keyDescriptor.nspace = -1; + return keyDescriptor; + } + + private static KeyMetadata metadata(byte[] cert, byte[] certChain) { + KeyMetadata metadata = new KeyMetadata(); + metadata.authorizations = new Authorization[0]; + metadata.certificate = cert; + metadata.certificateChain = certChain; + metadata.key = descriptor(); + metadata.modificationTimeMs = 0; + metadata.keySecurityLevel = 1; + return metadata; + } + + private static byte[] bytes(String string) { + return string.getBytes(); + } + + class MyPublicKey extends AndroidKeyStorePublicKey { + MyPublicKey(String cert, String chain, KeyStoreSecurityLevel securityLevel) { + super(descriptor(), metadata(cert.getBytes(), chain.getBytes()), "N/A".getBytes(), + "RSA", securityLevel); + } + + @Override + AndroidKeyStorePrivateKey getPrivateKey() { + return null; + } + } + + private AndroidKeyStorePublicKey makePrivateKeyObject(String cert, String chain) { + return new MyPublicKey(cert, chain, mKeystoreSecurityLevel); + } + + @Test + public void testKeystoreKeysAdhereToContractBetweenEqualsAndHashCode() throws Exception { + AndroidKeyStoreKey key1 = new AndroidKeyStoreKey(descriptor(), 1, new Authorization[0], + "RSA", mKeystoreSecurityLevel); + AndroidKeyStoreKey key2 = new AndroidKeyStoreKey(descriptor(), 2, new Authorization[0], + "RSA", mKeystoreSecurityLevel); + AndroidKeyStoreKey key1_clone = new AndroidKeyStoreKey(descriptor(), 1, + new Authorization[0], "RSA", mKeystoreSecurityLevel); + + assertThat("Identity should yield true", key1.equals(key1)); + Assert.assertEquals("Identity should yield same hash codes", + key1.hashCode(), key1.hashCode()); + assertThat("Identity should yield true", key2.equals(key2)); + Assert.assertEquals("Identity should yield same hash codes", + key2.hashCode(), key2.hashCode()); + assertThat("Different keys should differ", !key1.equals(key2)); + Assert.assertNotEquals("Different keys should have different hash codes", + key1.hashCode(), key2.hashCode()); + + assertThat("Same keys should yield true", key1.equals(key1_clone)); + assertThat("Same keys should yield true", key1_clone.equals(key1)); + Assert.assertEquals("Same keys should yield same hash codes", + key1.hashCode(), key1_clone.hashCode()); + + assertThat("anything.equal(null) should yield false", !key1.equals(null)); + assertThat("anything.equal(null) should yield false", !key2.equals(null)); + assertThat("anything.equal(null) should yield false", !key1_clone.equals(null)); + } + + @Test + public void testKeystorePublicKeysAdhereToContractBetweenEqualsAndHashCode() throws Exception { + AndroidKeyStorePublicKey key1 = makePrivateKeyObject("myCert1", "myChain1"); + AndroidKeyStorePublicKey key2 = makePrivateKeyObject("myCert2", "myChain1"); + AndroidKeyStorePublicKey key3 = makePrivateKeyObject("myCert1", "myChain3"); + AndroidKeyStorePublicKey key1_clone = makePrivateKeyObject("myCert1", "myChain1"); + + assertThat("Identity should yield true", key1.equals(key1)); + Assert.assertEquals("Identity should yield same hash codes", + key1.hashCode(), key1.hashCode()); + assertThat("Identity should yield true", key2.equals(key2)); + Assert.assertEquals("Identity should yield same hash codes", + key2.hashCode(), key2.hashCode()); + assertThat("Identity should yield true", key3.equals(key3)); + Assert.assertEquals("Identity should yield same hash codes", + key3.hashCode(), key3.hashCode()); + assertThat("Different keys should differ", !key1.equals(key2)); + Assert.assertNotEquals("Different keys should have different hash codes", + key1.hashCode(), key2.hashCode()); + assertThat("Different keys should differ", !key1.equals(key3)); + Assert.assertNotEquals("Different keys should have different hash codes", + key1.hashCode(), key3.hashCode()); + assertThat("Different keys should differ", !key2.equals(key3)); + Assert.assertNotEquals("Different keys should have different hash codes", + key2.hashCode(), key3.hashCode()); + + assertThat("Same keys should yield true", key1.equals(key1_clone)); + assertThat("Same keys should yield true", key1_clone.equals(key1)); + Assert.assertEquals("Same keys should yield same hash codes", + key1.hashCode(), key1_clone.hashCode()); + + assertThat("anything.equal(null) should yield false", !key1.equals(null)); + assertThat("anything.equal(null) should yield false", !key2.equals(null)); + assertThat("anything.equal(null) should yield false", !key3.equals(null)); + assertThat("anything.equal(null) should yield false", !key1_clone.equals(null)); + } }