diff options
4 files changed, 78 insertions, 27 deletions
diff --git a/core/java/android/security/keystore/recovery/RecoveryController.java b/core/java/android/security/keystore/recovery/RecoveryController.java index 604156145f1c..aa9d1c0a82a3 100644 --- a/core/java/android/security/keystore/recovery/RecoveryController.java +++ b/core/java/android/security/keystore/recovery/RecoveryController.java @@ -266,8 +266,8 @@ public class RecoveryController { /** * Sets a listener which notifies recovery agent that new recovery snapshot is available. {@link - * #getRecoveryData} can be used to get the snapshot. Note that every recovery agent can have at - * most one registered listener at any time. + * #getKeyChainSnapshot} can be used to get the snapshot. Note that every recovery agent can + * have at most one registered listener at any time. * * @param intent triggered when new snapshot is available. Unregisters listener if the value is * {@code null}. @@ -292,12 +292,13 @@ public class RecoveryController { * in vaultParams {@link RecoverySession#start(CertPath, byte[], byte[], List)}. * * @param serverParams included in recovery key blob. - * @see #getRecoveryData + * @see #getKeyChainSnapshot * @throws InternalRecoveryServiceException if an unexpected error occurred in the recovery * service. */ @RequiresPermission(android.Manifest.permission.RECOVER_KEYSTORE) - public void setServerParams(byte[] serverParams) throws InternalRecoveryServiceException { + public void setServerParams(@NonNull byte[] serverParams) + throws InternalRecoveryServiceException { try { mBinder.setServerParams(serverParams); } catch (RemoteException e) { @@ -321,7 +322,7 @@ public class RecoveryController { * Returns a list of aliases of keys belonging to the application. */ @RequiresPermission(android.Manifest.permission.RECOVER_KEYSTORE) - public List<String> getAliases() throws InternalRecoveryServiceException { + public @NonNull List<String> getAliases() throws InternalRecoveryServiceException { try { Map<String, Integer> allStatuses = mBinder.getRecoveryStatus(); return new ArrayList<>(allStatuses.keySet()); @@ -355,7 +356,7 @@ public class RecoveryController { * service. */ @RequiresPermission(android.Manifest.permission.RECOVER_KEYSTORE) - public void setRecoveryStatus(String alias, int status) + public void setRecoveryStatus(@NonNull String alias, int status) throws InternalRecoveryServiceException { try { mBinder.setRecoveryStatus(alias, status); @@ -390,7 +391,7 @@ public class RecoveryController { * service. */ @RequiresPermission(android.Manifest.permission.RECOVER_KEYSTORE) - public int getRecoveryStatus(String alias) throws InternalRecoveryServiceException { + public int getRecoveryStatus(@NonNull String alias) throws InternalRecoveryServiceException { try { Map<String, Integer> allStatuses = mBinder.getRecoveryStatus(); Integer status = allStatuses.get(alias); @@ -605,7 +606,7 @@ public class RecoveryController { * <p>A recovery session is required to restore keys from a remote store. */ @RequiresPermission(android.Manifest.permission.RECOVER_KEYSTORE) - public RecoverySession createRecoverySession() { + public @NonNull RecoverySession createRecoverySession() { return RecoverySession.newInstance(this); } diff --git a/core/java/android/security/keystore/recovery/RecoverySession.java b/core/java/android/security/keystore/recovery/RecoverySession.java index 208b9b2c2dd8..cf8a9ddf257e 100644 --- a/core/java/android/security/keystore/recovery/RecoverySession.java +++ b/core/java/android/security/keystore/recovery/RecoverySession.java @@ -49,7 +49,8 @@ public class RecoverySession implements AutoCloseable { private final String mSessionId; private final RecoveryController mRecoveryController; - private RecoverySession(RecoveryController recoveryController, String sessionId) { + private RecoverySession(@NonNull RecoveryController recoveryController, + @NonNull String sessionId) { mRecoveryController = recoveryController; mSessionId = sessionId; } @@ -58,14 +59,14 @@ public class RecoverySession implements AutoCloseable { * A new session, started by the {@link RecoveryController}. */ @RequiresPermission(android.Manifest.permission.RECOVER_KEYSTORE) - static RecoverySession newInstance(RecoveryController recoveryController) { + static @NonNull RecoverySession newInstance(RecoveryController recoveryController) { return new RecoverySession(recoveryController, newSessionId()); } /** * Returns a new random session ID. */ - private static String newSessionId() { + private static @NonNull String newSessionId() { SecureRandom secureRandom = new SecureRandom(); byte[] sessionId = new byte[SESSION_ID_LENGTH_BYTES]; secureRandom.nextBytes(sessionId); 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 429b4b564429..e75722af95ae 100644 --- a/services/core/java/com/android/server/locksettings/recoverablekeystore/RecoverableKeyStoreManager.java +++ b/services/core/java/com/android/server/locksettings/recoverablekeystore/RecoverableKeyStoreManager.java @@ -47,6 +47,7 @@ import android.util.Log; import com.android.internal.annotations.VisibleForTesting; import com.android.internal.util.HexDump; +import com.android.internal.util.Preconditions; import com.android.server.locksettings.recoverablekeystore.certificate.CertUtils; import com.android.server.locksettings.recoverablekeystore.certificate.SigXml; import com.android.server.locksettings.recoverablekeystore.storage.ApplicationKeyStorage; @@ -246,11 +247,11 @@ public class RecoverableKeyStoreManager { @NonNull String rootCertificateAlias, @NonNull byte[] recoveryServiceCertFile, @NonNull byte[] recoveryServiceSigFile) throws RemoteException { - if (recoveryServiceCertFile == null || recoveryServiceSigFile == null) { - Log.d(TAG, "The given cert or sig file is null"); - throw new ServiceSpecificException( - ERROR_BAD_CERTIFICATE_FORMAT, "The given cert or sig file is null."); + if (rootCertificateAlias == null) { + Log.e(TAG, "rootCertificateAlias is null"); } + Preconditions.checkNotNull(recoveryServiceCertFile, "recoveryServiceCertFile is null"); + Preconditions.checkNotNull(recoveryServiceSigFile, "recoveryServiceSigFile is null"); SigXml sigXml; try { @@ -293,11 +294,11 @@ public class RecoverableKeyStoreManager { /** * Gets all data necessary to recover application keys on new device. * - * @return recovery data + * @return KeyChain Snapshot. + * @throws ServiceSpecificException if no snapshot is pending. * @hide */ - public @NonNull - KeyChainSnapshot getKeyChainSnapshot() + public @NonNull KeyChainSnapshot getKeyChainSnapshot() throws RemoteException { checkRecoverKeyStorePermission(); int uid = Binder.getCallingUid(); @@ -327,7 +328,7 @@ public class RecoverableKeyStoreManager { throw new UnsupportedOperationException(); } - public void setServerParams(byte[] serverParams) throws RemoteException { + public void setServerParams(@NonNull byte[] serverParams) throws RemoteException { checkRecoverKeyStorePermission(); int userId = UserHandle.getCallingUserId(); int uid = Binder.getCallingUid(); @@ -340,8 +341,9 @@ public class RecoverableKeyStoreManager { /** * Sets the recovery status of key with {@code alias} to {@code status}. */ - public void setRecoveryStatus(String alias, int status) throws RemoteException { + public void setRecoveryStatus(@NonNull String alias, int status) throws RemoteException { checkRecoverKeyStorePermission(); + Preconditions.checkNotNull(alias, "alias is null"); mDatabase.setRecoveryStatus(Binder.getCallingUid(), alias, status); } @@ -366,6 +368,7 @@ public class RecoverableKeyStoreManager { @NonNull @KeyChainProtectionParams.UserSecretType int[] secretTypes) throws RemoteException { checkRecoverKeyStorePermission(); + Preconditions.checkNotNull(secretTypes, "secretTypes is null"); int userId = UserHandle.getCallingUserId(); int uid = Binder.getCallingUid(); long updatedRows = mDatabase.setRecoverySecretTypes(userId, uid, secretTypes); @@ -497,7 +500,14 @@ public class RecoverableKeyStoreManager { @NonNull List<KeyChainProtectionParams> secrets) throws RemoteException { checkRecoverKeyStorePermission(); - + if (rootCertificateAlias == null) { + Log.e(TAG, "rootCertificateAlias is null"); + } + Preconditions.checkNotNull(sessionId, "invalid session"); + Preconditions.checkNotNull(verifierCertPath, "verifierCertPath is null"); + Preconditions.checkNotNull(vaultParams, "vaultParams is null"); + Preconditions.checkNotNull(vaultChallenge, "vaultChallenge is null"); + Preconditions.checkNotNull(secrets, "secrets is null"); CertPath certPath; try { certPath = verifierCertPath.getCertPath(); @@ -543,6 +553,9 @@ public class RecoverableKeyStoreManager { @NonNull List<WrappedApplicationKey> applicationKeys) throws RemoteException { checkRecoverKeyStorePermission(); + Preconditions.checkNotNull(sessionId, "invalid session"); + Preconditions.checkNotNull(encryptedRecoveryKey, "encryptedRecoveryKey is null"); + Preconditions.checkNotNull(applicationKeys, "encryptedRecoveryKey is null"); int uid = Binder.getCallingUid(); RecoverySessionStorage.Entry sessionEntry = mRecoverySessionStorage.get(uid, sessionId); if (sessionEntry == null) { @@ -669,10 +682,13 @@ public class RecoverableKeyStoreManager { * Destroys the session with the given {@code sessionId}. */ public void closeSession(@NonNull String sessionId) throws RemoteException { + checkRecoverKeyStorePermission(); + Preconditions.checkNotNull(sessionId, "invalid session"); mRecoverySessionStorage.remove(Binder.getCallingUid(), sessionId); } public void removeKey(@NonNull String alias) throws RemoteException { + Preconditions.checkNotNull(alias, "alias is null"); int uid = Binder.getCallingUid(); int userId = UserHandle.getCallingUserId(); @@ -690,6 +706,7 @@ public class RecoverableKeyStoreManager { * @return grant alias, which caller can use to access the key. */ public String generateKey(@NonNull String alias) throws RemoteException { + Preconditions.checkNotNull(alias, "alias is null"); int uid = Binder.getCallingUid(); int userId = UserHandle.getCallingUserId(); @@ -728,8 +745,9 @@ public class RecoverableKeyStoreManager { */ public String importKey(@NonNull String alias, @NonNull byte[] keyBytes) throws RemoteException { - if (keyBytes == null || - keyBytes.length != RecoverableKeyGenerator.KEY_SIZE_BITS / Byte.SIZE) { + Preconditions.checkNotNull(alias, "alias is null"); + Preconditions.checkNotNull(keyBytes, "keyBytes is null"); + if (keyBytes.length != RecoverableKeyGenerator.KEY_SIZE_BITS / Byte.SIZE) { Log.e(TAG, "The given key for import doesn't have the required length " + RecoverableKeyGenerator.KEY_SIZE_BITS); throw new ServiceSpecificException(ERROR_INVALID_KEY_FORMAT, @@ -772,6 +790,7 @@ public class RecoverableKeyStoreManager { * @return grant alias, which caller can use to access the key. */ public String getKey(@NonNull String alias) throws RemoteException { + Preconditions.checkNotNull(alias, "alias is null"); int uid = Binder.getCallingUid(); int userId = UserHandle.getCallingUserId(); return getAlias(userId, uid, alias); 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 4664cad62d11..0d6d525b7fc6 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 @@ -242,8 +242,8 @@ public class RecoverableKeyStoreManagerTest { try { mRecoverableKeyStoreManager.importKey(TEST_ALIAS, /*keyBytes=*/ null); fail("should have thrown"); - } catch (ServiceSpecificException e) { - assertThat(e.getMessage()).contains("not contain 256 bits"); + } catch (NullPointerException e) { + assertThat(e.getMessage()).contains("is null"); } } @@ -410,7 +410,7 @@ public class RecoverableKeyStoreManagerTest { ROOT_CERTIFICATE_ALIAS, /*recoveryServiceCertFile=*/ null, TestData.getSigXml()); fail("should have thrown"); - } catch (ServiceSpecificException e) { + } catch (NullPointerException e) { assertThat(e.getMessage()).contains("is null"); } } @@ -422,7 +422,7 @@ public class RecoverableKeyStoreManagerTest { ROOT_CERTIFICATE_ALIAS, TestData.getCertXml(), /*recoveryServiceSigFile=*/ null); fail("should have thrown"); - } catch (ServiceSpecificException e) { + } catch (NullPointerException e) { assertThat(e.getMessage()).contains("is null"); } } @@ -575,6 +575,16 @@ public class RecoverableKeyStoreManagerTest { } @Test + public void closeSession_throwsIfNullSession() throws Exception { + try { + mRecoverableKeyStoreManager.closeSession(/*sessionId=*/ null); + fail("should have thrown"); + } catch (NullPointerException e) { + assertThat(e.getMessage()).contains("invalid"); + } + } + + @Test public void startRecoverySession_throwsIfBadNumberOfSecrets() throws Exception { try { mRecoverableKeyStoreManager.startRecoverySession( @@ -897,6 +907,16 @@ public class RecoverableKeyStoreManagerTest { } @Test + public void setRecoverySecretTypes_throwsIfNullTypes() throws Exception { + try { + mRecoverableKeyStoreManager.setRecoverySecretTypes(/*types=*/ null); + fail("should have thrown"); + } catch (NullPointerException e) { + assertThat(e.getMessage()).contains("is null"); + } + } + + @Test public void setRecoverySecretTypes_updatesShouldCreateSnapshot() throws Exception { int uid = Binder.getCallingUid(); int userId = UserHandle.getCallingUserId(); @@ -930,6 +950,16 @@ public class RecoverableKeyStoreManagerTest { assertThat(statuses).containsEntry(alias, status2); // updated } + @Test + public void setRecoveryStatus_throwsIfNullAlias() throws Exception { + try { + mRecoverableKeyStoreManager.setRecoveryStatus(/*alias=*/ null, /*status=*/ 100); + fail("should have thrown"); + } catch (NullPointerException e) { + assertThat(e.getMessage()).contains("is null"); + } + } + private static byte[] encryptedApplicationKey( SecretKey recoveryKey, byte[] applicationKey) throws Exception { return KeySyncUtils.encryptKeysWithRecoveryKey(recoveryKey, ImmutableMap.of( |