Obtain entropy later in crypto operations, when possible.

This makes Android Keystore crypto operations defer pulling entropy
from provided SecureRandom until KeyStore.finish, where appropriate.
Such as when performing asymmetric encryption or generating
signatures.

Bug: 18088752
Change-Id: I4a897754e9a846214cf0995c5514f98cf0edd76b

9 files changed, 86 insertions, 40 deletions

diff --git a/keystore/java/android/security/keystore/AndroidKeyStoreCipherSpiBase.java b/keystore/java/android/security/keystore/AndroidKeyStoreCipherSpiBase.java
index 19375a2..d2d5850 100644
--- a/keystore/java/android/security/keystore/AndroidKeyStoreCipherSpiBase.java
+++ b/keystore/java/android/security/keystore/AndroidKeyStoreCipherSpiBase.java
@@ -368,7 +368,10 @@ abstract class AndroidKeyStoreCipherSpiBase extends CipherSpi implements KeyStor
 
         byte[] output;
         try {
-            output = mMainDataStreamer.doFinal(input, inputOffset, inputLen);
+            byte[] additionalEntropy =
+                    KeyStoreCryptoOperationUtils.getRandomBytesToMixIntoKeystoreRng(
+                            mRng, getAdditionalEntropyAmountForFinish());
+            output = mMainDataStreamer.doFinal(input, inputOffset, inputLen, additionalEntropy);
         } catch (KeyStoreException e) {
             switch (e.getErrorCode()) {
                 case KeymasterDefs.KM_ERROR_INVALID_INPUT_LENGTH:
@@ -667,21 +670,37 @@ abstract class AndroidKeyStoreCipherSpiBase extends CipherSpi implements KeyStor
 
     /**
      * Returns the amount of additional entropy (in bytes) to be provided to the KeyStore's
-     * {@code begin} operation.
+     * {@code begin} operation. This amount of entropy is typically what's consumed to generate
+     * random parameters, such as IV.
      *
-     * <p>For decryption, this should be {@code 0} because decryption should not be consuming any
-     * entropy. For encryption, this value should match (or exceed) the amount of Shannon entropy of
-     * the ciphertext produced by this cipher assuming the key, the plaintext, and all explicitly
-     * provided parameters to {@code Cipher.init} are known. For example, for AES CBC encryption
-     * with an explicitly provided IV this should be {@code 0}, whereas for the case where IV is
-     * generated by the KeyStore's {@code begin} operation this should be {@code 16}. For RSA with
-     * OAEP this should be the size of the OAEP hash output. For RSA with PKCS#1 padding this should
-     * be the size of the padding string or could be raised (for simplicity) to the size of the
-     * modulus.
+     * <p>For decryption, the return value should be {@code 0} because decryption should not be
+     * consuming any entropy. For encryption, the value combined with
+     * {@link #getAdditionalEntropyAmountForFinish()} should match (or exceed) the amount of Shannon
+     * entropy of the ciphertext produced by this cipher assuming the key, the plaintext, and all
+     * explicitly provided parameters to {@code Cipher.init} are known. For example, for AES CBC
+     * encryption with an explicitly provided IV the return value should be {@code 0}, whereas for
+     * the case where IV is generated by the KeyStore's {@code begin} operation it should be
+     * {@code 16}.
      */
     protected abstract int getAdditionalEntropyAmountForBegin();
 
     /**
+     * Returns the amount of additional entropy (in bytes) to be provided to the KeyStore's
+     * {@code finish} operation. This amount of entropy is typically what's consumed by encryption
+     * padding scheme.
+     *
+     * <p>For decryption, the return value should be {@code 0} because decryption should not be
+     * consuming any entropy. For encryption, the value combined with
+     * {@link #getAdditionalEntropyAmountForBegin()} should match (or exceed) the amount of Shannon
+     * entropy of the ciphertext produced by this cipher assuming the key, the plaintext, and all
+     * explicitly provided parameters to {@code Cipher.init} are known. For example, for RSA with
+     * OAEP the return value should be the size of the OAEP hash output. For RSA with PKCS#1 padding
+     * the return value should be the size of the padding string or could be raised (for simplicity)
+     * to the size of the modulus.
+     */
+    protected abstract int getAdditionalEntropyAmountForFinish();
+
+    /**
      * Invoked to add algorithm-specific parameters for the KeyStore's {@code begin} operation.
      *
      * @param keymasterArgs keystore/keymaster arguments to be populated with algorithm-specific
diff --git a/keystore/java/android/security/keystore/AndroidKeyStoreECDSASignatureSpi.java b/keystore/java/android/security/keystore/AndroidKeyStoreECDSASignatureSpi.java
index 335da07..d19a766 100644
--- a/keystore/java/android/security/keystore/AndroidKeyStoreECDSASignatureSpi.java
+++ b/keystore/java/android/security/keystore/AndroidKeyStoreECDSASignatureSpi.java
@@ -117,7 +117,7 @@ abstract class AndroidKeyStoreECDSASignatureSpi extends AndroidKeyStoreSignature
     }
 
     @Override
-    protected int getAdditionalEntropyAmountForBegin() {
-        return (isSigning()) ? mGroupSizeBytes : 0;
+    protected int getAdditionalEntropyAmountForSign() {
+        return mGroupSizeBytes;
     }
 }
diff --git a/keystore/java/android/security/keystore/AndroidKeyStoreHmacSpi.java b/keystore/java/android/security/keystore/AndroidKeyStoreHmacSpi.java
index f31c06d..f7c184c 100644
--- a/keystore/java/android/security/keystore/AndroidKeyStoreHmacSpi.java
+++ b/keystore/java/android/security/keystore/AndroidKeyStoreHmacSpi.java
@@ -232,7 +232,10 @@ public abstract class AndroidKeyStoreHmacSpi extends MacSpi implements KeyStoreC
 
         byte[] result;
         try {
-            result = mChunkedStreamer.doFinal(null, 0, 0);
+            result = mChunkedStreamer.doFinal(
+                    null, 0, 0,
+                    null // no additional entropy needed -- HMAC is deterministic
+                    );
         } catch (KeyStoreException e) {
             throw new ProviderException("Keystore operation failed", e);
         }
diff --git a/keystore/java/android/security/keystore/AndroidKeyStoreRSACipherSpi.java b/keystore/java/android/security/keystore/AndroidKeyStoreRSACipherSpi.java
index d33692a..6abdf19 100644
--- a/keystore/java/android/security/keystore/AndroidKeyStoreRSACipherSpi.java
+++ b/keystore/java/android/security/keystore/AndroidKeyStoreRSACipherSpi.java
@@ -99,6 +99,11 @@ abstract class AndroidKeyStoreRSACipherSpi extends AndroidKeyStoreCipherSpiBase
         }
 
         @Override
+        protected final int getAdditionalEntropyAmountForFinish() {
+            return 0;
+        }
+
+        @Override
         @NonNull
         protected KeyStoreCryptoOperationStreamer createMainDataStreamer(
                 KeyStore keyStore, IBinder operationToken) {
@@ -142,7 +147,8 @@ abstract class AndroidKeyStoreRSACipherSpi extends AndroidKeyStoreCipherSpiBase
             }
 
             @Override
-            public byte[] doFinal(byte[] input, int inputOffset, int inputLength)
+            public byte[] doFinal(byte[] input, int inputOffset, int inputLength,
+                    byte[] additionalEntropy)
                     throws KeyStoreException {
                 if (inputLength > 0) {
                     mInputBuffer.write(input, inputOffset, inputLength);
@@ -165,7 +171,7 @@ abstract class AndroidKeyStoreRSACipherSpi extends AndroidKeyStoreCipherSpiBase
                             "Message size (" + bufferedInput.length + " bytes) must be smaller than"
                             + " modulus (" + mModulusSizeBytes + " bytes)");
                 }
-                return mDelegate.doFinal(paddedInput, 0, paddedInput.length);
+                return mDelegate.doFinal(paddedInput, 0, paddedInput.length, additionalEntropy);
             }
         }
     }
@@ -207,6 +213,11 @@ abstract class AndroidKeyStoreRSACipherSpi extends AndroidKeyStoreCipherSpiBase
 
         @Override
         protected final int getAdditionalEntropyAmountForBegin() {
+            return 0;
+        }
+
+        @Override
+        protected final int getAdditionalEntropyAmountForFinish() {
             return (isEncrypting()) ? getModulusSizeBytes() : 0;
         }
     }
@@ -361,6 +372,11 @@ abstract class AndroidKeyStoreRSACipherSpi extends AndroidKeyStoreCipherSpiBase
 
         @Override
         protected final int getAdditionalEntropyAmountForBegin() {
+            return 0;
+        }
+
+        @Override
+        protected final int getAdditionalEntropyAmountForFinish() {
             return (isEncrypting()) ? mDigestOutputSizeBytes : 0;
         }
     }
diff --git a/keystore/java/android/security/keystore/AndroidKeyStoreRSASignatureSpi.java b/keystore/java/android/security/keystore/AndroidKeyStoreRSASignatureSpi.java
index 898336d..954b71a 100644
--- a/keystore/java/android/security/keystore/AndroidKeyStoreRSASignatureSpi.java
+++ b/keystore/java/android/security/keystore/AndroidKeyStoreRSASignatureSpi.java
@@ -36,7 +36,7 @@ abstract class AndroidKeyStoreRSASignatureSpi extends AndroidKeyStoreSignatureSp
         }
 
         @Override
-        protected final int getAdditionalEntropyAmountForBegin() {
+        protected final int getAdditionalEntropyAmountForSign() {
             // No entropy required for this deterministic signature scheme.
             return 0;
         }
@@ -92,8 +92,8 @@ abstract class AndroidKeyStoreRSASignatureSpi extends AndroidKeyStoreSignatureSp
         }
 
         @Override
-        protected final int getAdditionalEntropyAmountForBegin() {
-            return (isSigning()) ? SALT_LENGTH_BYTES : 0;
+        protected final int getAdditionalEntropyAmountForSign() {
+            return SALT_LENGTH_BYTES;
         }
     }
 
diff --git a/keystore/java/android/security/keystore/AndroidKeyStoreSignatureSpiBase.java b/keystore/java/android/security/keystore/AndroidKeyStoreSignatureSpiBase.java
index f072ae7..5cdcc41 100644
--- a/keystore/java/android/security/keystore/AndroidKeyStoreSignatureSpiBase.java
+++ b/keystore/java/android/security/keystore/AndroidKeyStoreSignatureSpiBase.java
@@ -198,15 +198,14 @@ abstract class AndroidKeyStoreSignatureSpiBase extends SignatureSpi
 
         KeymasterArguments keymasterInputArgs = new KeymasterArguments();
         addAlgorithmSpecificParametersToBegin(keymasterInputArgs);
-        byte[] additionalEntropy = KeyStoreCryptoOperationUtils.getRandomBytesToMixIntoKeystoreRng(
-                appRandom, getAdditionalEntropyAmountForBegin());
 
         OperationResult opResult = mKeyStore.begin(
                 mKey.getAlias(),
                 mSigning ? KeymasterDefs.KM_PURPOSE_SIGN : KeymasterDefs.KM_PURPOSE_VERIFY,
                 true, // permit aborting this operation if keystore runs out of resources
                 keymasterInputArgs,
-                additionalEntropy);
+                null // no additional entropy for begin -- only finish might need some
+                );
         if (opResult == null) {
             throw new KeyStoreConnectException();
         }
@@ -311,7 +310,11 @@ abstract class AndroidKeyStoreSignatureSpiBase extends SignatureSpi
         byte[] signature;
         try {
             ensureKeystoreOperationInitialized();
-            signature = mMessageStreamer.doFinal(EmptyArray.BYTE, 0, 0);
+
+            byte[] additionalEntropy =
+                    KeyStoreCryptoOperationUtils.getRandomBytesToMixIntoKeystoreRng(
+                            appRandom, getAdditionalEntropyAmountForSign());
+            signature = mMessageStreamer.doFinal(EmptyArray.BYTE, 0, 0, additionalEntropy);
         } catch (InvalidKeyException | KeyStoreException e) {
             throw new SignatureException(e);
         }
@@ -388,15 +391,14 @@ abstract class AndroidKeyStoreSignatureSpiBase extends SignatureSpi
 
     /**
      * Returns the amount of additional entropy (in bytes) to be provided to the KeyStore's
-     * {@code begin} operation.
+     * {@code finish} operation when generating a signature.
      *
-     * <p>For signature verification, this should be {@code 0} because verification should not be
-     * consuming any entropy. For signature generation, this value should match (or exceed) the
-     * amount of Shannon entropy of the produced signature assuming the key and the message are
-     * known. For example, for ECDSA signature this should be the size of {@code R}, whereas for the
-     * RSA signature with PKCS#1 padding this should be {@code 0}.
+     * <p>This value should match (or exceed) the amount of Shannon entropy of the produced
+     * signature assuming the key and the message are known. For example, for ECDSA signature this
+     * should be the size of {@code R}, whereas for the RSA signature with PKCS#1 padding this
+     * should be {@code 0}.
      */
-    protected abstract int getAdditionalEntropyAmountForBegin();
+    protected abstract int getAdditionalEntropyAmountForSign();
 
     /**
      * Invoked to add algorithm-specific parameters for the KeyStore's {@code begin} operation.
diff --git a/keystore/java/android/security/keystore/AndroidKeyStoreUnauthenticatedAESCipherSpi.java b/keystore/java/android/security/keystore/AndroidKeyStoreUnauthenticatedAESCipherSpi.java
index 47cd1d1..76804a9 100644
--- a/keystore/java/android/security/keystore/AndroidKeyStoreUnauthenticatedAESCipherSpi.java
+++ b/keystore/java/android/security/keystore/AndroidKeyStoreUnauthenticatedAESCipherSpi.java
@@ -210,6 +210,11 @@ class AndroidKeyStoreUnauthenticatedAESCipherSpi extends AndroidKeyStoreCipherSp
     }
 
     @Override
+    protected final int getAdditionalEntropyAmountForFinish() {
+        return 0;
+    }
+
+    @Override
     protected final void addAlgorithmSpecificParametersToBegin(
             @NonNull KeymasterArguments keymasterArgs) {
         if ((isEncrypting()) && (mIvRequired) && (mIvHasBeenUsed)) {
diff --git a/keystore/java/android/security/keystore/KeyStoreCryptoOperationChunkedStreamer.java b/keystore/java/android/security/keystore/KeyStoreCryptoOperationChunkedStreamer.java
index 47b4996..9957e79 100644
--- a/keystore/java/android/security/keystore/KeyStoreCryptoOperationChunkedStreamer.java
+++ b/keystore/java/android/security/keystore/KeyStoreCryptoOperationChunkedStreamer.java
@@ -35,8 +35,8 @@ import java.io.IOException;
  * amount of data in one go because the operations are marshalled via Binder. Secondly, the update
  * operation may consume less data than provided, in which case the caller has to buffer the
  * remainder for next time. The helper exposes {@link #update(byte[], int, int) update} and
- * {@link #doFinal(byte[], int, int) doFinal} operations which can be used to conveniently implement
- * various JCA crypto primitives.
+ * {@link #doFinal(byte[], int, int, byte[]) doFinal} operations which can be used to conveniently
+ * implement various JCA crypto primitives.
  *
  * <p>Bidirectional chunked streaming of data via a KeyStore crypto operation is abstracted away as
  * a {@link Stream} to avoid having this class deal with operation tokens and occasional additional
@@ -60,7 +60,7 @@ class KeyStoreCryptoOperationChunkedStreamer implements KeyStoreCryptoOperationS
          * Returns the result of the KeyStore {@code finish} operation or null if keystore couldn't
          * be reached.
          */
-        OperationResult finish();
+        OperationResult finish(byte[] additionalEntropy);
     }
 
     // Binder buffer is about 1MB, but it's shared between all active transactions of the process.
@@ -192,7 +192,7 @@ class KeyStoreCryptoOperationChunkedStreamer implements KeyStoreCryptoOperationS
     }
 
     @Override
-    public byte[] doFinal(byte[] input, int inputOffset, int inputLength)
+    public byte[] doFinal(byte[] input, int inputOffset, int inputLength, byte[] additionalEntropy)
             throws KeyStoreException {
         if (inputLength == 0) {
             // No input provided -- simplify the rest of the code
@@ -204,7 +204,7 @@ class KeyStoreCryptoOperationChunkedStreamer implements KeyStoreCryptoOperationS
         byte[] output = update(input, inputOffset, inputLength);
         output = ArrayUtils.concat(output, flush());
 
-        OperationResult opResult = mKeyStoreStream.finish();
+        OperationResult opResult = mKeyStoreStream.finish(additionalEntropy);
         if (opResult == null) {
             throw new KeyStoreConnectException();
         } else if (opResult.resultCode != KeyStore.NO_ERROR) {
@@ -268,8 +268,8 @@ class KeyStoreCryptoOperationChunkedStreamer implements KeyStoreCryptoOperationS
         }
 
         @Override
-        public OperationResult finish() {
-            return mKeyStore.finish(mOperationToken, null, null);
+        public OperationResult finish(byte[] additionalEntropy) {
+            return mKeyStore.finish(mOperationToken, null, null, additionalEntropy);
         }
     }
 }
diff --git a/keystore/java/android/security/keystore/KeyStoreCryptoOperationStreamer.java b/keystore/java/android/security/keystore/KeyStoreCryptoOperationStreamer.java
index 2fb8f20..1c6de2d 100644
--- a/keystore/java/android/security/keystore/KeyStoreCryptoOperationStreamer.java
+++ b/keystore/java/android/security/keystore/KeyStoreCryptoOperationStreamer.java
@@ -28,12 +28,13 @@ import android.security.KeyStoreException;
  * amount of data in one go because the operations are marshalled via Binder. Secondly, the update
  * operation may consume less data than provided, in which case the caller has to buffer the
  * remainder for next time. The helper exposes {@link #update(byte[], int, int) update} and
- * {@link #doFinal(byte[], int, int) doFinal} operations which can be used to conveniently implement
- * various JCA crypto primitives.
+ * {@link #doFinal(byte[], int, int, byte[]) doFinal} operations which can be used to conveniently
+ * implement various JCA crypto primitives.
  *
  * @hide
  */
 interface KeyStoreCryptoOperationStreamer {
     byte[] update(byte[] input, int inputOffset, int inputLength) throws KeyStoreException;
-    byte[] doFinal(byte[] input, int inputOffset, int inputLength) throws KeyStoreException;
+    byte[] doFinal(byte[] input, int inputOffset, int inputLength, byte[] additionalEntropy)
+            throws KeyStoreException;
 }