From e1aa8866ae30bbe306616ce6056252105a37de77 Mon Sep 17 00:00:00 2001 From: Greg Rubin Date: Mon, 24 Aug 2020 19:04:22 +0000 Subject: [PATCH 1/3] Add tests for not releasing partial decryptions --- CHANGELOG.md | 3 ++ .../crypto/provider/test/AesTest.java | 50 +++++++++++++++++++ .../crypto/provider/test/RsaCipherTest.java | 16 ++++++ 3 files changed, 69 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 05b9a18a..d23a0cbd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -47,6 +47,9 @@ For other sizes, there are no documented guarantees of the SunEC behavior. You may need to do a clean build when changing tests. +### Patches +* Ensure unauthenticated plaintext is not released through either [Cipher.doFinal(byte[], int, int, byte[], int)](https://docs.oracle.com/javase/9/docs/api/javax/crypto/Cipher.html#doFinal-byte:A-int-int-byte:A-int-) or [Cipher.doFinal(ByteBuffer, ByteBuffer)](https://docs.oracle.com/javase/9/docs/api/javax/crypto/Cipher.html#doFinal-java.nio.ByteBuffer-java.nio.ByteBuffer-). [PR #123](https://github.com/corretto/amazon-corretto-crypto-provider/pull/123) + ### Maintenance * Upgrade tests to JUnit5. [PR #111](https://github.com/corretto/amazon-corretto-crypto-provider/pull/111) * Upgrade BouncyCastle test dependency 1.65. [PR #110](https://github.com/corretto/amazon-corretto-crypto-provider/pull/110) diff --git a/tst/com/amazon/corretto/crypto/provider/test/AesTest.java b/tst/com/amazon/corretto/crypto/provider/test/AesTest.java index fa363519..a0460ae0 100644 --- a/tst/com/amazon/corretto/crypto/provider/test/AesTest.java +++ b/tst/com/amazon/corretto/crypto/provider/test/AesTest.java @@ -638,6 +638,56 @@ public void testBadAEADTagException() throws Throwable { } } + @Test + public void testBadAEADTagException_noRelease() throws Throwable { + final SecureRandom rnd = TestUtil.MISC_SECURE_RANDOM.get(); + + Cipher c = Cipher.getInstance(ALGO_NAME, PROVIDER_SUN); + GCMParameterSpec algorithmParameterSpec = new GCMParameterSpec(128, randomIV()); + c.init(Cipher.ENCRYPT_MODE, key, algorithmParameterSpec, rnd); + + byte[] plaintext = randomIV(); + byte[] data = c.doFinal(plaintext); + byte[] output = new byte[plaintext.length]; + + for (int bit = 0; bit < data.length * 8; bit++) { + byte[] corruptData = data.clone(); + corruptData[bit / 8] ^= (1 << (bit % 8)); + + Cipher check = Cipher.getInstance(ALGO_NAME, PROVIDER_AMAZON); + check.init(Cipher.DECRYPT_MODE, key, algorithmParameterSpec, rnd); + + assertThrows(AEADBadTagException.class, () -> check.doFinal(corruptData, 0, corruptData.length, output, 0)); + assertArrayEquals(new byte[plaintext.length], output); + } + } + + @Test + public void testBadAEADTagException_noReleaseByteBuffer() throws Throwable { + final SecureRandom rnd = TestUtil.MISC_SECURE_RANDOM.get(); + + Cipher c = Cipher.getInstance(ALGO_NAME, PROVIDER_SUN); + GCMParameterSpec algorithmParameterSpec = new GCMParameterSpec(128, randomIV()); + c.init(Cipher.ENCRYPT_MODE, key, algorithmParameterSpec, rnd); + + byte[] plaintext = randomIV(); + byte[] data = c.doFinal(plaintext); + byte[] output = new byte[plaintext.length]; + + for (int bit = 0; bit < data.length * 8; bit++) { + byte[] corruptData = data.clone(); + corruptData[bit / 8] ^= (1 << (bit % 8)); + ByteBuffer corruptBuff = ByteBuffer.wrap(corruptData); + ByteBuffer outputBuff = ByteBuffer.wrap(output); + + Cipher check = Cipher.getInstance(ALGO_NAME, PROVIDER_AMAZON); + check.init(Cipher.DECRYPT_MODE, key, algorithmParameterSpec, rnd); + + assertThrows(AEADBadTagException.class, () -> check.doFinal(corruptBuff, outputBuff)); + assertArrayEquals(new byte[plaintext.length], output); + } + } + @Test public void whenCipherReusedWithoutReinit_throwsIVReuseException() throws Throwable { final SecureRandom rnd = TestUtil.MISC_SECURE_RANDOM.get(); diff --git a/tst/com/amazon/corretto/crypto/provider/test/RsaCipherTest.java b/tst/com/amazon/corretto/crypto/provider/test/RsaCipherTest.java index 3549842b..162c151c 100644 --- a/tst/com/amazon/corretto/crypto/provider/test/RsaCipherTest.java +++ b/tst/com/amazon/corretto/crypto/provider/test/RsaCipherTest.java @@ -21,6 +21,7 @@ import javax.crypto.spec.SecretKeySpec; import java.math.BigInteger; +import java.nio.ByteBuffer; import java.security.AlgorithmParameters; import java.security.GeneralSecurityException; import java.security.InvalidKeyException; @@ -933,6 +934,21 @@ private void testEncryptDecryptCycle(final Cipher encrypt, final Cipher decrypt, final byte[] ciphertext = encrypt.doFinal(plaintext); final byte[] decrypted = decrypt.doFinal(ciphertext); assertArrayEquals(plaintext, decrypted); + + // Verify no release of data even on bad padding + if (!NO_PADDING.equals(padding)) { + final byte[] result = new byte[ciphertext.length]; // Full size + ciphertext[3] ^= 0x13; // Just twiddle some bits + assertThrows(BadPaddingException.class, () -> decrypt.doFinal(ciphertext, 0, ciphertext.length, result, 0)); + assertArrayEquals(new byte[ciphertext.length], result); + + Arrays.fill(result, (byte) 0); + ByteBuffer ciphertextBuff = ByteBuffer.wrap(ciphertext); + ByteBuffer resultBuff = ByteBuffer.wrap(result); + assertThrows(BadPaddingException.class, () -> decrypt.doFinal(ciphertextBuff, resultBuff)); + assertArrayEquals(new byte[ciphertext.length], result); + } + } private void wrapUnwrap(final Cipher wrap, final Cipher unwrap) throws InvalidKeyException, From c1b03e850a79d00931a28c7e9260dfba0e6e3a5a Mon Sep 17 00:00:00 2001 From: Greg Rubin Date: Mon, 24 Aug 2020 20:49:55 +0000 Subject: [PATCH 2/3] Ensure plaintext is not exposed when AES-GCM tag is wrong --- .../corretto/crypto/provider/AesGcmSpi.java | 240 +++++++++--------- 1 file changed, 119 insertions(+), 121 deletions(-) diff --git a/src/com/amazon/corretto/crypto/provider/AesGcmSpi.java b/src/com/amazon/corretto/crypto/provider/AesGcmSpi.java index 72e00047..cd9b66f5 100644 --- a/src/com/amazon/corretto/crypto/provider/AesGcmSpi.java +++ b/src/com/amazon/corretto/crypto/provider/AesGcmSpi.java @@ -516,70 +516,90 @@ protected byte[] engineDoFinal(byte[] bytes, int offset, int length) throws Ille protected synchronized int engineDoFinal(byte[] bytes, int offset, int length, byte[] output, int outputOffset) throws ShortBufferException, IllegalBlockSizeException, BadPaddingException { - if (bytes == null) bytes = EMPTY_ARRAY; + try { + if (bytes == null) bytes = EMPTY_ARRAY; - boolean overlaps = Utils.arraysOverlap( - bytes, offset, output, outputOffset, Math.max(length , engineGetOutputSize(length)) - ); + boolean overlaps = Utils.arraysOverlap( + bytes, offset, output, outputOffset, Math.max(length , engineGetOutputSize(length)) + ); - if (opMode != NATIVE_MODE_ENCRYPT && opMode != NATIVE_MODE_DECRYPT) { - throw new IllegalStateException("Cipher not initialized"); - } - - if (outputOffset < 0) { - throw new ArrayIndexOutOfBoundsException("Negative output offset"); - } + if (opMode != NATIVE_MODE_ENCRYPT && opMode != NATIVE_MODE_DECRYPT) { + throw new IllegalStateException("Cipher not initialized"); + } - checkOutputBuffer(length, output, outputOffset); - checkArrayLimits(bytes, offset, length); + if (outputOffset < 0) { + throw new ArrayIndexOutOfBoundsException("Negative output offset"); + } - int resultLength = 0; + checkOutputBuffer(length, output, outputOffset); + checkArrayLimits(bytes, offset, length); - if (overlaps) { - // The input and output potentially overlap. We'll need to make sure we copy the input somewhere safe before - // proceeding too much further. + int resultLength = 0; - // Since we need to take care of this on engineUpdate as well, we can just delegate to engineUpdate, which - // will make sure to copy the buffer - on encrypt this is an explicit check, while on decrypt engineUpdate - // unconditionally copies to a temporary buffer. + if (overlaps) { + // The input and output potentially overlap. We'll need to make sure we copy the input somewhere safe before + // proceeding too much further. - resultLength = engineUpdate(bytes, offset, length, output, outputOffset); - outputOffset += resultLength; + // Since we need to take care of this on engineUpdate as well, we can just delegate to engineUpdate, which + // will make sure to copy the buffer - on encrypt this is an explicit check, while on decrypt engineUpdate + // unconditionally copies to a temporary buffer. - // We processed all of the input in engineUpdate. So there's no longer an overlap to deal with. - length = 0; - } + resultLength = engineUpdate(bytes, offset, length, output, outputOffset); + outputOffset += resultLength; - if (opMode == NATIVE_MODE_DECRYPT) { - // If we have some data already, merge them into our temporary buffer. - if (decryptInputBuf.size() != 0) { - engineUpdate(bytes, offset, length); - bytes = decryptInputBuf.getDataBuffer(); - offset = 0; - length = decryptInputBuf.size(); + // We processed all of the input in engineUpdate. So there's no longer an overlap to deal with. + length = 0; } - if (length < tagLength) { - throw new AEADBadTagException("Input too short - need tag"); - } + if (opMode == NATIVE_MODE_DECRYPT) { + // We use this temporary buffer both for our input ciphertext and our output (possibly unauthenticated) plaintext. + // We'll copy it to the actual output location iff decryption completes successfully. + engineUpdate(bytes, offset, length); // Decrypt mode never generates output for updates + final byte[] tempBuffer = decryptInputBuf.getDataBuffer(); + final int tempBufferLength = decryptInputBuf.size(); - final byte[] finalBytes = bytes; - final int finalOffset = offset; - final int finalOutputOffset = outputOffset; - final int finalLength = length; - keyUsageCount++; - if (context != null) { - // We already have a context, so let's reuse it. - return context.use(ptr -> { - return oneShotDecrypt( - ptr, - null, - finalBytes, - finalOffset, - finalLength, + if (tempBufferLength < tagLength) { + throw new AEADBadTagException("Input too short - need tag"); + } - output, - finalOutputOffset, + keyUsageCount++; + final int outLen; + if (context != null) { + // We already have a context, so let's reuse it. + outLen = context.use(ptr -> { + return oneShotDecrypt( + ptr, + null, + tempBuffer, + 0, + tempBufferLength, + + tempBuffer, + 0, + + tagLength, + key, + iv, + + // The cost of calling decryptAADBuf.getDataBuffer() when its buffer is empty is significant for 16-byte + // decrypt operations (approximately a 7% performance hit). To avoid this, we reuse the same empty array + // instead in this common-case path. + decryptAADBuf.size() != 0 ? decryptAADBuf.getDataBuffer() : EMPTY_ARRAY, + decryptAADBuf.size() + ); + }); + } else { + // We don't have an existing context, however we might want to save one + final long[] ptrOut = keyUsageCount > KEY_REUSE_THRESHOLD ? new long[1] : null; + outLen = oneShotDecrypt( + 0, + ptrOut, + tempBuffer, + 0, + tempBufferLength, + + tempBuffer, + 0, tagLength, key, @@ -591,56 +611,51 @@ protected synchronized int engineDoFinal(byte[] bytes, int offset, int length, b decryptAADBuf.size() != 0 ? decryptAADBuf.getDataBuffer() : EMPTY_ARRAY, decryptAADBuf.size() ); - }); - } else { - // We don't have an existing context, however we might want to save one - final long[] ptrOut = keyUsageCount > KEY_REUSE_THRESHOLD ? new long[1] : null; - final int outLen = oneShotDecrypt( - 0, - ptrOut, - bytes, - offset, - length, - - output, - outputOffset, - - tagLength, - key, - iv, - - // The cost of calling decryptAADBuf.getDataBuffer() when its buffer is empty is significant for 16-byte - // decrypt operations (approximately a 7% performance hit). To avoid this, we reuse the same empty array - // instead in this common-case path. - decryptAADBuf.size() != 0 ? decryptAADBuf.getDataBuffer() : EMPTY_ARRAY, - decryptAADBuf.size() - ); - - if (ptrOut != null) { - context = new NativeContext(ptrOut[0]); + + if (ptrOut != null) { + context = new NativeContext(ptrOut[0]); + } } + // Decryption completed successfully. Copy it to the output location + System.arraycopy(tempBuffer, 0, output, outputOffset, outLen); return outLen; - } - } + } // End of Decrypt mode - checkNeedReset(); + // Start of Encrypt mode + checkNeedReset(); - this.needReset = true; - final byte[] finalBytes = bytes; - final int finalOffset = offset; - final int finalOutputOffset = outputOffset; - final int finalLength = length; - if (!contextInitialized) { - // Context has not been initialized, meaning the user called doFinal immediately after init(). In this case - // we make a single native call to perform the encryption operation in one go. - - keyUsageCount++; - if (context != null) { - // Our key, but not our IV has been initialized - return context.use(ptr -> { - return oneShotEncrypt( - ptr, - null, + this.needReset = true; + final byte[] finalBytes = bytes; + final int finalOffset = offset; + final int finalOutputOffset = outputOffset; + final int finalLength = length; + if (!contextInitialized) { + // Context has not been initialized, meaning the user called doFinal immediately after init(). In this case + // we make a single native call to perform the encryption operation in one go. + + keyUsageCount++; + if (context != null) { + // Our key, but not our IV has been initialized + return context.use(ptr -> { + return oneShotEncrypt( + ptr, + null, + finalBytes, + finalOffset, + finalLength, + output, + finalOutputOffset, + tagLength, + key, + iv + ); + }); + } else { + // We don't have an existing context, however we might want to save one + final long[] ptrOut = keyUsageCount > KEY_REUSE_THRESHOLD ? new long[1] : null; + final int outLen = oneShotEncrypt( + 0, + ptrOut, finalBytes, finalOffset, finalLength, @@ -650,29 +665,12 @@ protected synchronized int engineDoFinal(byte[] bytes, int offset, int length, b key, iv ); - }); - } else { - // We don't have an existing context, however we might want to save one - final long[] ptrOut = keyUsageCount > KEY_REUSE_THRESHOLD ? new long[1] : null; - final int outLen = oneShotEncrypt( - 0, - ptrOut, - finalBytes, - finalOffset, - finalLength, - output, - finalOutputOffset, - tagLength, - key, - iv - ); - if (ptrOut != null) { - context = new NativeContext(ptrOut[0]); + if (ptrOut != null) { + context = new NativeContext(ptrOut[0]); + } + return outLen; } - return outLen; - } - } else { - try { + } else { // We need to make sure to add resultLength here; engineUpdate in encrypt mode produces incremental // output (unlike in decrypt mode) and so we need to carry forward whatever amount of data it produced // in our return value. @@ -712,9 +710,9 @@ protected synchronized int engineDoFinal(byte[] bytes, int offset, int length, b context = null; } return resultLength + finalOutputLen; - } finally { - stateReset(); } + } finally { + stateReset(); } } From c751f9f5b3c6340bce175831ec56724263f3ae33 Mon Sep 17 00:00:00 2001 From: Greg Rubin Date: Mon, 24 Aug 2020 20:54:03 +0000 Subject: [PATCH 3/3] Ignore a few more files --- .gitignore | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.gitignore b/.gitignore index ac46a1a6..ff25acdf 100644 --- a/.gitignore +++ b/.gitignore @@ -3,6 +3,9 @@ /.project /.settings/ /.vscode/ +/.idea/ *~ +\#*# *.ipr *.iws +*.iml