From def7ffdb054777b792e23e96b2eee1010d1404f2 Mon Sep 17 00:00:00 2001 From: Bo Zhu Date: Fri, 5 Jan 2018 14:50:52 -0800 Subject: [PATCH] Check the public key bytes of veriferPublicKey matche the ones in vaultParams Test: adb shell am instrument -w -e package com.android.server.locksettings.recoverablekeystore com.android.frameworks.servicestests/android.support.test.runner.AndroidJUnitRunner Change-Id: Iac26de202be88c8328619b03dd59ab742aa7e4c3 --- .../RecoverableKeyStoreManager.java | 31 ++++++++++--- .../RecoverableKeyStoreManagerTest.java | 43 ++++++++++++++++++- 2 files changed, 68 insertions(+), 6 deletions(-) diff --git a/services/core/java/com/android/server/locksettings/recoverablekeystore/RecoverableKeyStoreManager.java b/services/core/java/com/android/server/locksettings/recoverablekeystore/RecoverableKeyStoreManager.java index 3379f83d52c8d..9865079a79fac 100644 --- a/services/core/java/com/android/server/locksettings/recoverablekeystore/RecoverableKeyStoreManager.java +++ b/services/core/java/com/android/server/locksettings/recoverablekeystore/RecoverableKeyStoreManager.java @@ -58,6 +58,7 @@ import java.security.PublicKey; import java.security.UnrecoverableKeyException; import java.security.spec.InvalidKeySpecException; import java.security.spec.X509EncodedKeySpec; +import java.util.Arrays; import java.util.HashMap; import java.util.List; import java.util.Locale; @@ -323,6 +324,23 @@ public class RecoverableKeyStoreManager { "Only a single KeyStoreRecoveryMetadata is supported"); } + PublicKey publicKey; + try { + publicKey = KeySyncUtils.deserializePublicKey(verifierPublicKey); + } catch (NoSuchAlgorithmException e) { + // Should never happen + throw new RuntimeException(e); + } catch (InvalidKeySpecException e) { + throw new ServiceSpecificException(ERROR_BAD_X509_CERTIFICATE, "Not a valid X509 key"); + } + // The raw public key bytes contained in vaultParams must match the ones given in + // verifierPublicKey; otherwise, the user secret may be decrypted by a key that is not owned + // by the original recovery service. + if (!publicKeysMatch(publicKey, vaultParams)) { + throw new ServiceSpecificException(ERROR_BAD_X509_CERTIFICATE, + "The public keys given in verifierPublicKey and vaultParams do not match."); + } + byte[] keyClaimant = KeySyncUtils.generateKeyClaimant(); byte[] kfHash = secrets.get(0).getSecret(); mRecoverySessionStorage.add( @@ -331,7 +349,6 @@ public class RecoverableKeyStoreManager { try { byte[] thmKfHash = KeySyncUtils.calculateThmKfHash(kfHash); - PublicKey publicKey = KeySyncUtils.deserializePublicKey(verifierPublicKey); return KeySyncUtils.encryptRecoveryClaim( publicKey, vaultParams, @@ -341,9 +358,8 @@ public class RecoverableKeyStoreManager { } catch (NoSuchAlgorithmException e) { Log.wtf(TAG, "SecureBox algorithm missing. AOSP must support this.", e); throw new ServiceSpecificException(ERROR_UNEXPECTED_MISSING_ALGORITHM, e.getMessage()); - } catch (InvalidKeySpecException | InvalidKeyException e) { - throw new ServiceSpecificException(ERROR_BAD_X509_CERTIFICATE, - "Not a valid X509 key"); + } catch (InvalidKeyException e) { + throw new ServiceSpecificException(ERROR_BAD_X509_CERTIFICATE, e.getMessage()); } } @@ -352,7 +368,7 @@ public class RecoverableKeyStoreManager { * service. * * @param sessionId The session ID used to generate the claim. See - * {@link #startRecoverySession(String, byte[], byte[], byte[], List, int)}. + * {@link #startRecoverySession(String, byte[], byte[], byte[], List)}. * @param encryptedRecoveryKey The encrypted recovery key blob returned by the remote vault * service. * @param applicationKeys The encrypted key blobs returned by the remote vault service. These @@ -509,4 +525,9 @@ public class RecoverableKeyStoreManager { RecoverableKeyStoreLoader.PERMISSION_RECOVER_KEYSTORE, "Caller " + Binder.getCallingUid() + " doesn't have RecoverKeyStore permission."); } + + private boolean publicKeysMatch(PublicKey publicKey, byte[] vaultParams) { + byte[] encodedPublicKey = SecureBox.encodePublicKey(publicKey); + return Arrays.equals(encodedPublicKey, Arrays.copyOf(vaultParams, encodedPublicKey.length)); + } } diff --git a/services/tests/servicestests/src/com/android/server/locksettings/recoverablekeystore/RecoverableKeyStoreManagerTest.java b/services/tests/servicestests/src/com/android/server/locksettings/recoverablekeystore/RecoverableKeyStoreManagerTest.java index 26f27b47e3872..c0efb20b1a7ef 100644 --- a/services/tests/servicestests/src/com/android/server/locksettings/recoverablekeystore/RecoverableKeyStoreManagerTest.java +++ b/services/tests/servicestests/src/com/android/server/locksettings/recoverablekeystore/RecoverableKeyStoreManagerTest.java @@ -96,7 +96,26 @@ public class RecoverableKeyStoreManagerTest { private static final byte[] TEST_SALT = getUtf8Bytes("salt"); private static final byte[] TEST_SECRET = getUtf8Bytes("password1234"); private static final byte[] TEST_VAULT_CHALLENGE = getUtf8Bytes("vault_challenge"); - private static final byte[] TEST_VAULT_PARAMS = getUtf8Bytes("vault_params"); + private static final byte[] TEST_VAULT_PARAMS = new byte[] { + // backend_key + (byte) 0x04, (byte) 0xb8, (byte) 0x00, (byte) 0x11, (byte) 0x18, (byte) 0x98, (byte) 0x1d, + (byte) 0xf0, (byte) 0x6e, (byte) 0xb4, (byte) 0x94, (byte) 0xfe, (byte) 0x86, (byte) 0xda, + (byte) 0x1c, (byte) 0x07, (byte) 0x8d, (byte) 0x01, (byte) 0xb4, (byte) 0x3a, (byte) 0xf6, + (byte) 0x8d, (byte) 0xdc, (byte) 0x61, (byte) 0xd0, (byte) 0x46, (byte) 0x49, (byte) 0x95, + (byte) 0x0f, (byte) 0x10, (byte) 0x86, (byte) 0x93, (byte) 0x24, (byte) 0x66, (byte) 0xe0, + (byte) 0x3f, (byte) 0xd2, (byte) 0xdf, (byte) 0xf3, (byte) 0x79, (byte) 0x20, (byte) 0x1d, + (byte) 0x91, (byte) 0x55, (byte) 0xb0, (byte) 0xe5, (byte) 0xbd, (byte) 0x7a, (byte) 0x8b, + (byte) 0x32, (byte) 0x7d, (byte) 0x25, (byte) 0x53, (byte) 0xa2, (byte) 0xfc, (byte) 0xa5, + (byte) 0x65, (byte) 0xe1, (byte) 0xbd, (byte) 0x21, (byte) 0x44, (byte) 0x7e, (byte) 0x78, + (byte) 0x52, (byte) 0xfa, + // counter_id + (byte) 0x31, (byte) 0x32, (byte) 0x33, (byte) 0x34, (byte) 0x00, (byte) 0x00, (byte) 0x00, + (byte) 0x00, + // device_parameter + (byte) 0x78, (byte) 0x56, (byte) 0x34, (byte) 0x12, (byte) 0x00, (byte) 0x00, (byte) 0x00, + (byte) 0x0, + // max_attempts + (byte) 0x0a, (byte) 0x00, (byte) 0x00, (byte) 0x00}; private static final int TEST_GENERATION_ID = 2; private static final int TEST_USER_ID = 10009; private static final int KEY_CLAIMANT_LENGTH_BYTES = 16; @@ -248,6 +267,28 @@ public class RecoverableKeyStoreManagerTest { } } + @Test + public void startRecoverySession_throwsIfPublicKeysMismatch() throws Exception { + byte[] vaultParams = TEST_VAULT_PARAMS.clone(); + vaultParams[1] ^= (byte) 1; // Flip 1 bit + try { + mRecoverableKeyStoreManager.startRecoverySession( + TEST_SESSION_ID, + TEST_PUBLIC_KEY, + vaultParams, + TEST_VAULT_CHALLENGE, + ImmutableList.of( + new KeyStoreRecoveryMetadata( + TYPE_LOCKSCREEN, + TYPE_PASSWORD, + KeyDerivationParameters.createSHA256Parameters(TEST_SALT), + TEST_SECRET))); + fail("should have thrown"); + } catch (ServiceSpecificException e) { + assertThat(e.getMessage()).contains("do not match"); + } + } + @Test public void recoverKeys_throwsIfNoSessionIsPresent() throws Exception { try {