From 2bcdad95330c75e3122d0736f1a40acd521dc243 Mon Sep 17 00:00:00 2001 From: Robert Berry Date: Thu, 18 Jan 2018 12:53:29 +0000 Subject: [PATCH] Use RecoverySession object to hide session IDs (redux) Session IDs are an implementation detail that the framework can (and should) abstract away. This was previously reverted due to breaking master. Test: adb shell am instrument -w -e package com.android.server.locksettings.recoverablekeystore com.android.frameworks.servicestests/android.support.test.runner.AndroidJUnitRunner Change-Id: I4427c818348c054ada39d799b6da3b739f27eba9 --- .../security/keystore/RecoveryClaim.java | 54 ++++++++++++++ .../security/keystore/RecoveryManager.java | 39 +++++++--- .../security/keystore/RecoverySession.java | 71 +++++++++++++++++++ .../internal/widget/ILockSettings.aidl | 1 + .../locksettings/LockSettingsService.java | 4 ++ .../RecoverableKeyStoreManager.java | 7 ++ .../storage/RecoverySessionStorage.java | 10 +++ .../RecoverableKeyStoreManagerTest.java | 38 ++++++++++ .../storage/RecoverySessionStorageTest.java | 41 +++++++++++ 9 files changed, 254 insertions(+), 11 deletions(-) create mode 100644 core/java/android/security/keystore/RecoveryClaim.java create mode 100644 core/java/android/security/keystore/RecoverySession.java diff --git a/core/java/android/security/keystore/RecoveryClaim.java b/core/java/android/security/keystore/RecoveryClaim.java new file mode 100644 index 0000000000000..6f566af1dc7df --- /dev/null +++ b/core/java/android/security/keystore/RecoveryClaim.java @@ -0,0 +1,54 @@ +/* + * Copyright (C) 2018 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package android.security.keystore; + +/** + * An attempt to recover a keychain protected by remote secure hardware. + * + * @hide + */ +public class RecoveryClaim { + + private final RecoverySession mRecoverySession; + private final byte[] mClaimBytes; + + RecoveryClaim(RecoverySession recoverySession, byte[] claimBytes) { + mRecoverySession = recoverySession; + mClaimBytes = claimBytes; + } + + /** + * Returns the session associated with the recovery attempt. This is used to match the symmetric + * key, which remains internal to the framework, for decrypting the claim response. + * + * @return The session data. + */ + public RecoverySession getRecoverySession() { + return mRecoverySession; + } + + /** + * Returns the encrypted claim's bytes. + * + *

This should be sent by the recovery agent to the remote secure hardware, which will use + * it to decrypt the keychain, before sending it re-encrypted with the session's symmetric key + * to the device. + */ + public byte[] getClaimBytes() { + return mClaimBytes; + } +} diff --git a/core/java/android/security/keystore/RecoveryManager.java b/core/java/android/security/keystore/RecoveryManager.java index f01ed06f43b7e..a381a2cea4b76 100644 --- a/core/java/android/security/keystore/RecoveryManager.java +++ b/core/java/android/security/keystore/RecoveryManager.java @@ -23,6 +23,7 @@ import android.content.pm.PackageManager.NameNotFoundException; import android.os.RemoteException; import android.os.ServiceManager; import android.os.ServiceSpecificException; +import android.util.Log; import com.android.internal.widget.ILockSettings; @@ -36,6 +37,7 @@ import java.util.Map; * @hide */ public class RecoveryManager { + private static final String TAG = "RecoveryController"; /** Key has been successfully synced. */ public static final int RECOVERY_STATUS_SYNCED = 0; @@ -380,30 +382,31 @@ public class RecoveryManager { * @param vaultChallenge Data passed from server for this recovery session and used to prevent * replay attacks * @param secrets Secrets provided by user, the method only uses type and secret fields. - * @return Binary blob with recovery claim. It is encrypted with verifierPublicKey and contains - * a proof of user secrets, session symmetric key and parameters necessary to identify the - * counter with the number of failed recovery attempts. + * @return The recovery claim. Claim provides a b binary blob with recovery claim. It is + * encrypted with verifierPublicKey and contains a proof of user secrets, session symmetric + * key and parameters necessary to identify the counter with the number of failed recovery + * attempts. * @throws BadCertificateFormatException if the {@code verifierPublicKey} is in an incorrect * format. * @throws InternalRecoveryServiceException if an unexpected error occurred in the recovery * service. */ - public @NonNull byte[] startRecoverySession( - @NonNull String sessionId, + @NonNull public RecoveryClaim startRecoverySession( @NonNull byte[] verifierPublicKey, @NonNull byte[] vaultParams, @NonNull byte[] vaultChallenge, @NonNull List secrets) throws BadCertificateFormatException, InternalRecoveryServiceException { try { + RecoverySession recoverySession = RecoverySession.newInstance(this); byte[] recoveryClaim = mBinder.startRecoverySession( - sessionId, + recoverySession.getSessionId(), verifierPublicKey, vaultParams, vaultChallenge, secrets); - return recoveryClaim; + return new RecoveryClaim(recoverySession, recoveryClaim); } catch (RemoteException e) { throw e.rethrowFromSystemServer(); } catch (ServiceSpecificException e) { @@ -417,8 +420,8 @@ public class RecoveryManager { /** * Imports keys. * - * @param sessionId Id for recovery session, same as in - * {@link #startRecoverySession(String, byte[], byte[], byte[], List)}. + * @param session Related recovery session, as originally created by invoking + * {@link #startRecoverySession(byte[], byte[], byte[], List)}. * @param recoveryKeyBlob Recovery blob encrypted by symmetric key generated for this session. * @param applicationKeys Application keys. Key material can be decrypted using recoveryKeyBlob * and session. KeyStore only uses package names from the application info in {@link @@ -429,14 +432,14 @@ public class RecoveryManager { * @throws InternalRecoveryServiceException if an error occurs internal to the recovery service. */ public Map recoverKeys( - @NonNull String sessionId, + @NonNull RecoverySession session, @NonNull byte[] recoveryKeyBlob, @NonNull List applicationKeys) throws SessionExpiredException, DecryptionFailedException, InternalRecoveryServiceException { try { return (Map) mBinder.recoverKeys( - sessionId, recoveryKeyBlob, applicationKeys); + session.getSessionId(), recoveryKeyBlob, applicationKeys); } catch (RemoteException e) { throw e.rethrowFromSystemServer(); } catch (ServiceSpecificException e) { @@ -450,6 +453,20 @@ public class RecoveryManager { } } + /** + * Deletes all data associated with {@code session}. Should not be invoked directly but via + * {@link RecoverySession#close()}. + * + * @hide + */ + void closeSession(RecoverySession session) { + try { + mBinder.closeSession(session.getSessionId()); + } catch (RemoteException | ServiceSpecificException e) { + Log.e(TAG, "Unexpected error trying to close session", e); + } + } + /** * Generates a key called {@code alias} and loads it into the recoverable key store. Returns the * raw material of the key. diff --git a/core/java/android/security/keystore/RecoverySession.java b/core/java/android/security/keystore/RecoverySession.java new file mode 100644 index 0000000000000..f78551fdf194a --- /dev/null +++ b/core/java/android/security/keystore/RecoverySession.java @@ -0,0 +1,71 @@ +/* + * Copyright (C) 2018 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package android.security.keystore; + +import java.security.SecureRandom; + +/** + * Session to recover a {@link KeychainSnapshot} from the remote trusted hardware, initiated by a + * recovery agent. + * + * @hide + */ +public class RecoverySession implements AutoCloseable { + + private static final int SESSION_ID_LENGTH_BYTES = 16; + + private final String mSessionId; + private final RecoveryManager mRecoveryManager; + + private RecoverySession(RecoveryManager recoveryManager, String sessionId) { + mRecoveryManager = recoveryManager; + mSessionId = sessionId; + } + + /** + * A new session, started by {@code recoveryManager}. + */ + static RecoverySession newInstance(RecoveryManager recoveryManager) { + return new RecoverySession(recoveryManager, newSessionId()); + } + + /** + * Returns a new random session ID. + */ + private static String newSessionId() { + SecureRandom secureRandom = new SecureRandom(); + byte[] sessionId = new byte[SESSION_ID_LENGTH_BYTES]; + secureRandom.nextBytes(sessionId); + StringBuilder sb = new StringBuilder(); + for (byte b : sessionId) { + sb.append(Byte.toHexString(b, /*upperCase=*/ false)); + } + return sb.toString(); + } + + /** + * An internal session ID, used by the framework to match recovery claims to snapshot responses. + */ + String getSessionId() { + return mSessionId; + } + + @Override + public void close() { + mRecoveryManager.closeSession(this); + } +} diff --git a/core/java/com/android/internal/widget/ILockSettings.aidl b/core/java/com/android/internal/widget/ILockSettings.aidl index 6ed8973ea3e63..274239b218ced 100644 --- a/core/java/com/android/internal/widget/ILockSettings.aidl +++ b/core/java/com/android/internal/widget/ILockSettings.aidl @@ -81,4 +81,5 @@ interface ILockSettings { in List secrets); Map/**/ recoverKeys(in String sessionId, in byte[] recoveryKeyBlob, in List applicationKeys); + void closeSession(in String sessionId); } diff --git a/services/core/java/com/android/server/locksettings/LockSettingsService.java b/services/core/java/com/android/server/locksettings/LockSettingsService.java index 7bd1ae9e9f00d..07ea51be4ba08 100644 --- a/services/core/java/com/android/server/locksettings/LockSettingsService.java +++ b/services/core/java/com/android/server/locksettings/LockSettingsService.java @@ -2028,6 +2028,10 @@ public class LockSettingsService extends ILockSettings.Stub { vaultParams, vaultChallenge, secrets); } + public void closeSession(@NonNull String sessionId) throws RemoteException { + mRecoverableKeyStoreManager.closeSession(sessionId); + } + @Override public Map recoverKeys(@NonNull String sessionId, @NonNull byte[] recoveryKeyBlob, @NonNull List applicationKeys) 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 801dc54862e04..59855beec625e 100644 --- a/services/core/java/com/android/server/locksettings/recoverablekeystore/RecoverableKeyStoreManager.java +++ b/services/core/java/com/android/server/locksettings/recoverablekeystore/RecoverableKeyStoreManager.java @@ -435,6 +435,13 @@ public class RecoverableKeyStoreManager { } } + /** + * Destroys the session with the given {@code sessionId}. + */ + public void closeSession(@NonNull String sessionId) throws RemoteException { + mRecoverySessionStorage.remove(Binder.getCallingUid(), sessionId); + } + public void removeKey(@NonNull String alias) throws RemoteException { int uid = Binder.getCallingUid(); int userId = UserHandle.getCallingUserId(); diff --git a/services/core/java/com/android/server/locksettings/recoverablekeystore/storage/RecoverySessionStorage.java b/services/core/java/com/android/server/locksettings/recoverablekeystore/storage/RecoverySessionStorage.java index f7633e4cee8df..0e66746f4160b 100644 --- a/services/core/java/com/android/server/locksettings/recoverablekeystore/storage/RecoverySessionStorage.java +++ b/services/core/java/com/android/server/locksettings/recoverablekeystore/storage/RecoverySessionStorage.java @@ -72,6 +72,16 @@ public class RecoverySessionStorage implements Destroyable { mSessionsByUid.get(uid).add(entry); } + /** + * Deletes the session with {@code sessionId} created by app with {@code uid}. + */ + public void remove(int uid, String sessionId) { + if (mSessionsByUid.get(uid) == null) { + return; + } + mSessionsByUid.get(uid).removeIf(session -> session.mSessionId.equals(sessionId)); + } + /** * Removes all sessions associated with the given recovery agent uid. * 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 767472ab808eb..402a37c420a7a 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 @@ -282,6 +282,44 @@ public class RecoverableKeyStoreManagerTest { assertEquals(KEY_CLAIMANT_LENGTH_BYTES, entry.getKeyClaimant().length); } + @Test + public void closeSession_closesASession() throws Exception { + mRecoverableKeyStoreManager.startRecoverySession( + TEST_SESSION_ID, + TEST_PUBLIC_KEY, + TEST_VAULT_PARAMS, + TEST_VAULT_CHALLENGE, + ImmutableList.of( + new KeychainProtectionParams( + TYPE_LOCKSCREEN, + TYPE_PASSWORD, + KeyDerivationParams.createSha256Params(TEST_SALT), + TEST_SECRET))); + + mRecoverableKeyStoreManager.closeSession(TEST_SESSION_ID); + + assertEquals(0, mRecoverySessionStorage.size()); + } + + @Test + public void closeSession_doesNotCloseUnrelatedSessions() throws Exception { + mRecoverableKeyStoreManager.startRecoverySession( + TEST_SESSION_ID, + TEST_PUBLIC_KEY, + TEST_VAULT_PARAMS, + TEST_VAULT_CHALLENGE, + ImmutableList.of( + new KeychainProtectionParams( + TYPE_LOCKSCREEN, + TYPE_PASSWORD, + KeyDerivationParams.createSha256Params(TEST_SALT), + TEST_SECRET))); + + mRecoverableKeyStoreManager.closeSession("some random session"); + + assertEquals(1, mRecoverySessionStorage.size()); + } + @Test public void startRecoverySession_throwsIfBadNumberOfSecrets() throws Exception { try { diff --git a/services/tests/servicestests/src/com/android/server/locksettings/recoverablekeystore/storage/RecoverySessionStorageTest.java b/services/tests/servicestests/src/com/android/server/locksettings/recoverablekeystore/storage/RecoverySessionStorageTest.java index 6f93fe409e48b..0f95748a8ea7f 100644 --- a/services/tests/servicestests/src/com/android/server/locksettings/recoverablekeystore/storage/RecoverySessionStorageTest.java +++ b/services/tests/servicestests/src/com/android/server/locksettings/recoverablekeystore/storage/RecoverySessionStorageTest.java @@ -19,6 +19,8 @@ package com.android.server.locksettings.recoverablekeystore.storage; import static junit.framework.Assert.fail; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; import android.support.test.filters.SmallTest; import android.support.test.runner.AndroidJUnit4; @@ -87,6 +89,45 @@ public class RecoverySessionStorageTest { assertZeroedOut(entry.getKeyClaimant()); } + @Test + public void remove_deletesSpecificSession() { + RecoverySessionStorage storage = new RecoverySessionStorage(); + storage.add(TEST_USER_ID, new RecoverySessionStorage.Entry( + TEST_SESSION_ID, + lskfHashFixture(), + keyClaimantFixture(), + vaultParamsFixture())); + storage.add(TEST_USER_ID, new RecoverySessionStorage.Entry( + "some other session", + lskfHashFixture(), + keyClaimantFixture(), + vaultParamsFixture())); + + storage.remove(TEST_USER_ID, TEST_SESSION_ID); + + assertNull(storage.get(TEST_USER_ID, TEST_SESSION_ID)); + } + + @Test + public void remove_doesNotDeleteOtherSessions() { + String otherSessionId = "some other session"; + RecoverySessionStorage storage = new RecoverySessionStorage(); + storage.add(TEST_USER_ID, new RecoverySessionStorage.Entry( + TEST_SESSION_ID, + lskfHashFixture(), + keyClaimantFixture(), + vaultParamsFixture())); + storage.add(TEST_USER_ID, new RecoverySessionStorage.Entry( + otherSessionId, + lskfHashFixture(), + keyClaimantFixture(), + vaultParamsFixture())); + + storage.remove(TEST_USER_ID, TEST_SESSION_ID); + + assertNotNull(storage.get(TEST_USER_ID, TEST_SESSION_ID)); + } + @Test public void destroy_overwritesLskfHashMemory() { RecoverySessionStorage storage = new RecoverySessionStorage();