Revert "Use RecoverySession object to hide session IDs"
This reverts commit 988c55ce67459553bad517426a924d06a89b059f.
Reason for revert: broke some tests
Change-Id: Ib43099aebc8ff025e052337475bab13445da74eb
diff --git a/core/java/android/security/keystore/RecoveryClaim.java b/core/java/android/security/keystore/RecoveryClaim.java
deleted file mode 100644
index 6f566af..0000000
--- a/core/java/android/security/keystore/RecoveryClaim.java
+++ /dev/null
@@ -1,54 +0,0 @@
-/*
- * 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.
- *
- * <p>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 05c931e..6177cd7 100644
--- a/core/java/android/security/keystore/RecoveryManager.java
+++ b/core/java/android/security/keystore/RecoveryManager.java
@@ -23,7 +23,6 @@
import android.os.RemoteException;
import android.os.ServiceManager;
import android.os.ServiceSpecificException;
-import android.util.Log;
import com.android.internal.widget.ILockSettings;
@@ -37,7 +36,6 @@
* @hide
*/
public class RecoveryManager {
- private static final String TAG = "RecoveryController";
/** Key has been successfully synced. */
public static final int RECOVERY_STATUS_SYNCED = 0;
@@ -373,6 +371,7 @@
* The method generates symmetric key for a session, which trusted remote device can use to
* return recovery key.
*
+ * @param sessionId ID for recovery session.
* @param verifierPublicKey Encoded {@code java.security.cert.X509Certificate} with Public key
* used to create the recovery blob on the source device.
* Keystore will verify the certificate using root of trust.
@@ -381,31 +380,30 @@
* @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 The recovery claim. Claim provides a 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 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.
*/
- @NonNull public RecoveryClaim startRecoverySession(
+ public @NonNull byte[] startRecoverySession(
+ @NonNull String sessionId,
@NonNull byte[] verifierPublicKey,
@NonNull byte[] vaultParams,
@NonNull byte[] vaultChallenge,
@NonNull List<KeychainProtectionParameter> secrets)
throws BadCertificateFormatException, InternalRecoveryServiceException {
try {
- RecoverySession recoverySession = RecoverySession.newInstance(this);
byte[] recoveryClaim =
mBinder.startRecoverySession(
- recoverySession.getSessionId(),
+ sessionId,
verifierPublicKey,
vaultParams,
vaultChallenge,
secrets);
- return new RecoveryClaim(recoverySession, recoveryClaim);
+ return recoveryClaim;
} catch (RemoteException e) {
throw e.rethrowFromSystemServer();
} catch (ServiceSpecificException e) {
@@ -419,8 +417,8 @@
/**
* Imports keys.
*
- * @param session Related recovery session, as originally created by invoking
- * {@link #startRecoverySession(byte[], byte[], byte[], List)}.
+ * @param sessionId Id for recovery session, same as in
+ * {@link #startRecoverySession(String, 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
@@ -431,14 +429,14 @@
* @throws InternalRecoveryServiceException if an error occurs internal to the recovery service.
*/
public Map<String, byte[]> recoverKeys(
- @NonNull RecoverySession session,
+ @NonNull String sessionId,
@NonNull byte[] recoveryKeyBlob,
@NonNull List<WrappedApplicationKey> applicationKeys)
throws SessionExpiredException, DecryptionFailedException,
InternalRecoveryServiceException {
try {
return (Map<String, byte[]>) mBinder.recoverKeys(
- session.getSessionId(), recoveryKeyBlob, applicationKeys);
+ sessionId, recoveryKeyBlob, applicationKeys);
} catch (RemoteException e) {
throw e.rethrowFromSystemServer();
} catch (ServiceSpecificException e) {
@@ -453,20 +451,6 @@
}
/**
- * 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
deleted file mode 100644
index f78551f..0000000
--- a/core/java/android/security/keystore/RecoverySession.java
+++ /dev/null
@@ -1,71 +0,0 @@
-/*
- * 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 b3ef0f2..b2bab6f 100644
--- a/core/java/com/android/internal/widget/ILockSettings.aidl
+++ b/core/java/com/android/internal/widget/ILockSettings.aidl
@@ -81,5 +81,4 @@
in List<KeychainProtectionParameter> secrets);
Map/*<String, byte[]>*/ recoverKeys(in String sessionId, in byte[] recoveryKeyBlob,
in List<WrappedApplicationKey> 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 50ef8e1..879c024 100644
--- a/services/core/java/com/android/server/locksettings/LockSettingsService.java
+++ b/services/core/java/com/android/server/locksettings/LockSettingsService.java
@@ -2029,11 +2029,6 @@
}
@Override
- public void closeSession(@NonNull String sessionId) throws RemoteException {
- mRecoverableKeyStoreManager.closeSession(sessionId);
- }
-
- @Override
public Map<String, byte[]> recoverKeys(@NonNull String sessionId,
@NonNull byte[] recoveryKeyBlob, @NonNull List<WrappedApplicationKey> applicationKeys)
throws RemoteException {
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 01fd95c..0371c7e 100644
--- a/services/core/java/com/android/server/locksettings/recoverablekeystore/RecoverableKeyStoreManager.java
+++ b/services/core/java/com/android/server/locksettings/recoverablekeystore/RecoverableKeyStoreManager.java
@@ -434,13 +434,6 @@
}
}
- /**
- * 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 4c8f66b..f7633e4 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
@@ -91,16 +91,6 @@
}
/**
- * 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));
- }
-
- /**
* Returns the total count of pending sessions.
*
* @hide
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 5244c7a..3715742 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
@@ -283,44 +283,6 @@
}
@Test
- public void closeSession_closesASession() throws Exception {
- mRecoverableKeyStoreManager.startRecoverySession(
- TEST_SESSION_ID,
- TEST_PUBLIC_KEY,
- TEST_VAULT_PARAMS,
- TEST_VAULT_CHALLENGE,
- ImmutableList.of(
- new KeychainProtectionParameter(
- 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 KeychainProtectionParameter(
- 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 {
mRecoverableKeyStoreManager.startRecoverySession(
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 0f95748..6f93fe4 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,8 +19,6 @@
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;
@@ -90,45 +88,6 @@
}
@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();
RecoverySessionStorage.Entry entry = new RecoverySessionStorage.Entry(