From 481d4277f702fed98e630200dbb146a44c953d89 Mon Sep 17 00:00:00 2001 From: SalusaSecondus Date: Fri, 11 Sep 2020 15:00:50 -0700 Subject: [PATCH 01/14] Restrict EC generation configured by size (#127) * Restrict EC generation configured by size * Wordsmithing the changelog --- CHANGELOG.md | 11 +++++ build.gradle | 2 +- .../corretto/crypto/provider/EcGen.java | 42 +++++++++---------- 3 files changed, 32 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 597dbe02..0c67bc3a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,16 @@ # Changelog +## 1.6.0 (Unreleased) +### Breaking Change +In accordance with our [versioning policy](https://github.com/corretto/amazon-corretto-crypto-provider/blob/master/VERSIONING.rst), +this release contains a low-risk breaking change. For details please see the [1.5.0](#150) section of this document. +This change only impacts libraries that generate EC keys using the +[KeyPairGenerator.initialize(int keysize)](https://docs.oracle.com/javase/8/docs/api/java/security/KeyPairGenerator.html#initialize-int-) +method. + +### Improvements +* Stricter guarantees about which curves are used for EC key generation. [PR #127](https://github.com/corretto/amazon-corretto-crypto-provider/pull/127) + ## 1.5.0 ### Breaking Change Warning In accordance with our [versioning policy](https://github.com/corretto/amazon-corretto-crypto-provider/blob/master/VERSIONING.rst), diff --git a/build.gradle b/build.gradle index 82d9cb12..a87ea8b4 100644 --- a/build.gradle +++ b/build.gradle @@ -7,7 +7,7 @@ plugins { } group = 'software.amazon.cryptools' -version = '1.5.0' +version = '1.6.0' def openssl_version = '1.1.1g' def opensslSrcPath = "${buildDir}/openssl/openssl-${openssl_version}" diff --git a/src/com/amazon/corretto/crypto/provider/EcGen.java b/src/com/amazon/corretto/crypto/provider/EcGen.java index 3abda797..e5d8f9e8 100644 --- a/src/com/amazon/corretto/crypto/provider/EcGen.java +++ b/src/com/amazon/corretto/crypto/provider/EcGen.java @@ -166,30 +166,28 @@ private static byte[] encodeSpec(final AlgorithmParameterSpec spec) { public void initialize(final int keysize, final SecureRandom random) throws InvalidParameterException { try { - final String curveName = "secp" + keysize + "r1"; // Explicitly list default curves // Mapping from OpenJDK - // TODO: Uncomment following code at version 1.6.0 -// final String curveName; -// switch (keysize) { -// case 192: -// curveName = "secp192r1"; // NIST P-192 -// break; -// case 224: -// curveName = "secp224r1"; // NIST P-224 -// break; -// case 256: -// curveName = "secp256r1"; // NIST P-256 -// break; -// case 384: -// curveName = "secp384r1"; // NIST P-384 -// break; -// case 521: -// curveName = "secp521r1"; // NIST P-521 -// break; -// default: -// throw new InvalidParameterException("No default NIST prime curve for keysize " + keysize); -// } + final String curveName; + switch (keysize) { + case 192: + curveName = "secp192r1"; // NIST P-192 + break; + case 224: + curveName = "secp224r1"; // NIST P-224 + break; + case 256: + curveName = "secp256r1"; // NIST P-256 + break; + case 384: + curveName = "secp384r1"; // NIST P-384 + break; + case 521: + curveName = "secp521r1"; // NIST P-521 + break; + default: + throw new InvalidParameterException("No default NIST prime curve for keysize " + keysize); + } initialize(new ECGenParameterSpec(curveName), random); } catch (final InvalidAlgorithmParameterException ex) { throw new InvalidParameterException(ex.getMessage()); From 479ca5808fb45ef7a1471372617bcc4bfe837979 Mon Sep 17 00:00:00 2001 From: Greg Rubin Date: Fri, 11 Sep 2020 22:03:41 +0000 Subject: [PATCH 02/14] Add version gating to some tests --- .gitignore | 1 + CHANGELOG.md | 3 ++ .../crypto/provider/test/AesTest.java | 4 ++ .../crypto/provider/test/HmacTest.java | 4 +- .../crypto/provider/test/RsaCipherTest.java | 51 ++++++++++++------- 5 files changed, 43 insertions(+), 20 deletions(-) diff --git a/.gitignore b/.gitignore index ff25acdf..ef64a2e6 100644 --- a/.gitignore +++ b/.gitignore @@ -9,3 +9,4 @@ *.ipr *.iws *.iml +*.orig diff --git a/CHANGELOG.md b/CHANGELOG.md index 0c67bc3a..21226825 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,9 @@ method. ### Improvements * Stricter guarantees about which curves are used for EC key generation. [PR #127](https://github.com/corretto/amazon-corretto-crypto-provider/pull/127) +### Patches +* Add version gating to some tests introduced in 1.5.0 [PR #128](https://github.com/corretto/amazon-corretto-crypto-provider/pull/128) + ## 1.5.0 ### Breaking Change Warning In accordance with our [versioning policy](https://github.com/corretto/amazon-corretto-crypto-provider/blob/master/VERSIONING.rst), diff --git a/tst/com/amazon/corretto/crypto/provider/test/AesTest.java b/tst/com/amazon/corretto/crypto/provider/test/AesTest.java index a0460ae0..f54c1f15 100644 --- a/tst/com/amazon/corretto/crypto/provider/test/AesTest.java +++ b/tst/com/amazon/corretto/crypto/provider/test/AesTest.java @@ -640,6 +640,8 @@ public void testBadAEADTagException() throws Throwable { @Test public void testBadAEADTagException_noRelease() throws Throwable { + assumeMinimumVersion("1.5.0", AmazonCorrettoCryptoProvider.INSTANCE); + final SecureRandom rnd = TestUtil.MISC_SECURE_RANDOM.get(); Cipher c = Cipher.getInstance(ALGO_NAME, PROVIDER_SUN); @@ -664,6 +666,8 @@ public void testBadAEADTagException_noRelease() throws Throwable { @Test public void testBadAEADTagException_noReleaseByteBuffer() throws Throwable { + assumeMinimumVersion("1.5.0", AmazonCorrettoCryptoProvider.INSTANCE); + final SecureRandom rnd = TestUtil.MISC_SECURE_RANDOM.get(); Cipher c = Cipher.getInstance(ALGO_NAME, PROVIDER_SUN); diff --git a/tst/com/amazon/corretto/crypto/provider/test/HmacTest.java b/tst/com/amazon/corretto/crypto/provider/test/HmacTest.java index 63e87327..8cbaa577 100644 --- a/tst/com/amazon/corretto/crypto/provider/test/HmacTest.java +++ b/tst/com/amazon/corretto/crypto/provider/test/HmacTest.java @@ -378,8 +378,10 @@ public byte[] getEncoded() { assertThrows(InvalidAlgorithmParameterException.class, () -> mac.init(validKey, new IvParameterSpec(new byte[0]))); assertThrows(InvalidKeyException.class, () -> mac.init(pubKey)); assertThrows(InvalidKeyException.class, () -> mac.init(badFormat)); - assertThrows(InvalidKeyException.class, () -> mac.init(nullFormat)); assertThrows(InvalidKeyException.class, () -> mac.init(nullEncoding)); + + TestUtil.assumeMinimumVersion("1.5.0", AmazonCorrettoCryptoProvider.INSTANCE); + assertThrows(InvalidKeyException.class, () -> mac.init(nullFormat)); } } diff --git a/tst/com/amazon/corretto/crypto/provider/test/RsaCipherTest.java b/tst/com/amazon/corretto/crypto/provider/test/RsaCipherTest.java index d7acce8d..e1039ce6 100644 --- a/tst/com/amazon/corretto/crypto/provider/test/RsaCipherTest.java +++ b/tst/com/amazon/corretto/crypto/provider/test/RsaCipherTest.java @@ -137,6 +137,20 @@ public void jce2nativeNoPadding1024() throws GeneralSecurityException { testJce2Native(NO_PADDING, 1024); } + private void assertProperWrongInputSizeException(GeneralSecurityException ex) throws GeneralSecurityException { + // Behavior changed as of version 1.5.0 and sometimes we need tests to run + // against older versions. + if (TestUtil.versionCompare("1.5.0", NATIVE_PROVIDER) <= 0) { + if (!(ex instanceof IllegalBlockSizeException)) { + throw ex; + } + } else { + if (!(ex instanceof BadPaddingException)) { + throw ex; + } + } + } + @Test public void noPaddingSizes() throws GeneralSecurityException { final Cipher nativeEncrypt = Cipher.getInstance(NO_PADDING, NATIVE_PROVIDER); @@ -146,8 +160,8 @@ public void noPaddingSizes() throws GeneralSecurityException { try { nativeEncrypt.doFinal(plaintext); fail("Expected IllegalBlockSizeException"); - } catch (final IllegalBlockSizeException ex) { - // expected + } catch (final GeneralSecurityException ex) { + assertProperWrongInputSizeException(ex); } plaintext = new byte[1024 / 8]; @@ -165,8 +179,8 @@ public void noPaddingSizes() throws GeneralSecurityException { try { nativeEncrypt.doFinal(plaintext); fail("Expected IllegalBlockSizeException"); - } catch (final IllegalBlockSizeException ex) { - // expected + } catch (final GeneralSecurityException ex) { + assertProperWrongInputSizeException(ex); } plaintext = new byte[2048 / 8]; @@ -184,8 +198,8 @@ public void noPaddingSizes() throws GeneralSecurityException { try { nativeEncrypt.doFinal(plaintext); fail("Expected IllegalBlockSizeException"); - } catch (final IllegalBlockSizeException ex) { - // expected + } catch (final GeneralSecurityException ex) { + assertProperWrongInputSizeException(ex); } plaintext = new byte[4096 / 8]; @@ -355,18 +369,17 @@ public void Pkcs1PaddingSizes() throws GeneralSecurityException { try { nativeC.doFinal(plaintext); fail("Expected IllegalBlockSizeException"); - } catch (final IllegalBlockSizeException ex) { - // expected + } catch (final GeneralSecurityException ex) { + assertProperWrongInputSizeException(ex); } - nativeC.init(Cipher.ENCRYPT_MODE, PAIR_2048.getPublic()); plaintext = getPlaintext(2048 / 8 - 10); try { nativeC.doFinal(plaintext); fail("Expected IllegalBlockSizeException"); - } catch (final IllegalBlockSizeException ex) { - // expected + } catch (final GeneralSecurityException ex) { + assertProperWrongInputSizeException(ex); } nativeC.init(Cipher.ENCRYPT_MODE, PAIR_4096.getPublic()); @@ -375,8 +388,8 @@ public void Pkcs1PaddingSizes() throws GeneralSecurityException { try { nativeC.doFinal(plaintext); fail("Expected IllegalBlockSizeException"); - } catch (final IllegalBlockSizeException ex) { - // expected + } catch (final GeneralSecurityException ex) { + assertProperWrongInputSizeException(ex); } } @@ -419,8 +432,8 @@ public void OaepSha1PaddingSizes() throws GeneralSecurityException { try { nativeC.doFinal(plaintext); fail("Expected IllegalBlockSizeException"); - } catch (final IllegalBlockSizeException ex) { - // expected + } catch (final GeneralSecurityException ex) { + assertProperWrongInputSizeException(ex); } nativeC.init(Cipher.ENCRYPT_MODE, PAIR_2048.getPublic()); @@ -429,8 +442,8 @@ public void OaepSha1PaddingSizes() throws GeneralSecurityException { try { nativeC.doFinal(plaintext); fail("Expected IllegalBlockSizeException"); - } catch (final IllegalBlockSizeException ex) { - // expected + } catch (final GeneralSecurityException ex) { + assertProperWrongInputSizeException(ex); } nativeC.init(Cipher.ENCRYPT_MODE, PAIR_4096.getPublic()); @@ -439,8 +452,8 @@ public void OaepSha1PaddingSizes() throws GeneralSecurityException { try { nativeC.doFinal(plaintext); fail("Expected IllegalBlockSizeException"); - } catch (final IllegalBlockSizeException ex) { - // expected + } catch (final GeneralSecurityException ex) { + assertProperWrongInputSizeException(ex); } } From a469ecf82193f7cf157b23638799c69e5baabb27 Mon Sep 17 00:00:00 2001 From: Greg Rubin Date: Wed, 9 Sep 2020 22:10:19 +0000 Subject: [PATCH 03/14] Fix NPE in TestResultLogger --- .../amazon/corretto/crypto/provider/test/TestResultLogger.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tst/com/amazon/corretto/crypto/provider/test/TestResultLogger.java b/tst/com/amazon/corretto/crypto/provider/test/TestResultLogger.java index 7c2ccef9..691a7748 100644 --- a/tst/com/amazon/corretto/crypto/provider/test/TestResultLogger.java +++ b/tst/com/amazon/corretto/crypto/provider/test/TestResultLogger.java @@ -74,7 +74,7 @@ private void printNotice(final String notice, ExtensionContext context, String d StringWriter causeText = new StringWriter(); if (cause != null) { try (PrintWriter printWriter = new PrintWriter(causeText)) { - printWriter.append(" @ ").append(getFailureLocation(cause).toString()); + printWriter.append(" @ ").append(String.valueOf(getFailureLocation(cause))); // Don't print out traces for Assert.* failures which throw subclasses of AssertionError. // Just for thrown exceptions if (AssertionError.class.equals(cause.getClass()) || From 40666e8b9997560cea5b3bcb07274ed0ac257830 Mon Sep 17 00:00:00 2001 From: Greg Rubin Date: Wed, 9 Sep 2020 22:13:31 +0000 Subject: [PATCH 04/14] Add constant time logic --- CMakeLists.txt | 2 + .../crypto/provider/ConstantTime.java | 69 +++++++++++ .../provider/test/ConstantTimeTests.java | 117 ++++++++++++++++++ 3 files changed, 188 insertions(+) create mode 100644 src/com/amazon/corretto/crypto/provider/ConstantTime.java create mode 100644 tst/com/amazon/corretto/crypto/provider/test/ConstantTimeTests.java diff --git a/CMakeLists.txt b/CMakeLists.txt index 1ab27ae8..c1096c19 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -107,6 +107,7 @@ set(ACCP_SRC src/com/amazon/corretto/crypto/provider/AccessibleByteArrayOutputStream.java src/com/amazon/corretto/crypto/provider/AesCtrDrbg.java src/com/amazon/corretto/crypto/provider/AesGcmSpi.java + src/com/amazon/corretto/crypto/provider/ConstantTime.java src/com/amazon/corretto/crypto/provider/EcGen.java src/com/amazon/corretto/crypto/provider/EvpKeyAgreement.java src/com/amazon/corretto/crypto/provider/EvpKeyType.java @@ -528,6 +529,7 @@ add_jar( tst/com/amazon/corretto/crypto/provider/test/AESGenerativeTest.java tst/com/amazon/corretto/crypto/provider/test/AesCtrDrbgTest.java tst/com/amazon/corretto/crypto/provider/test/AesTest.java + tst/com/amazon/corretto/crypto/provider/test/ConstantTimeTests.java tst/com/amazon/corretto/crypto/provider/test/EcGenTest.java tst/com/amazon/corretto/crypto/provider/test/EvpKeyAgreementTest.java tst/com/amazon/corretto/crypto/provider/test/EvpKeyAgreementSpecificTest.java diff --git a/src/com/amazon/corretto/crypto/provider/ConstantTime.java b/src/com/amazon/corretto/crypto/provider/ConstantTime.java new file mode 100644 index 00000000..eed8af92 --- /dev/null +++ b/src/com/amazon/corretto/crypto/provider/ConstantTime.java @@ -0,0 +1,69 @@ +// Copyright Amazon.com Inc. or its affiliates. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package com.amazon.corretto.crypto.provider; + +/** + * Contains several constant time utilities + */ +final class ConstantTime { + private ConstantTime() { + // Prevent instantiation + } + + /** + * Equivalent to {@code val != 0 ? 1 : 0} + */ + static final int isNonZero(int val) { + return ((val | -val) >>> 31) & 0x01; // Unsigned bitshift + } + + /** + * Equivalent to {@code val == 0 ? 1 : 0} + */ + static final int isZero(int val) { + return 1 - isNonZero(val); + } + + /** + * Equivalent to {@code val < 0 ? 1 : 0} + */ + static final int isNegative(int val) { + return (val >>> 31) & 0x01; + } + + /** + * Equivalent to {@code x == y ? 1 : 0} + */ + static final int equal(int x, int y) { + final int difference = x - y; + // Difference is 0 iff x == y + return isZero(difference); + } + + /** + * Equivalent to {@code x > y ? 1 : 0} + */ + static final int gt(int x, int y) { + // Convert to long to avoid underflow + final long xl = x; + final long yl = y; + final long difference = yl - xl; + // If xl > yl, then difference is negative. + // Thus, we can just return the sign-bit + return (int) ((difference >>> 63) & 0x01); // Unsigned bitshift + } + + /** + * Equivalent to {@code selector != 0 ? a : b} + */ + static final int select(int selector, int a, int b) { + final int mask = isZero(selector) - 1; + // Mask == -1 (all bits 1) iff selector != 0 + // Mask == 0 (all bits 0) iff selector == 0 + + final int combined = a ^ b; + + return b ^ (combined & mask); + } +} diff --git a/tst/com/amazon/corretto/crypto/provider/test/ConstantTimeTests.java b/tst/com/amazon/corretto/crypto/provider/test/ConstantTimeTests.java new file mode 100644 index 00000000..f71ebe4d --- /dev/null +++ b/tst/com/amazon/corretto/crypto/provider/test/ConstantTimeTests.java @@ -0,0 +1,117 @@ +// Copyright Amazon.com Inc. or its affiliates. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package com.amazon.corretto.crypto.provider.test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.params.provider.Arguments.arguments; + +import java.util.ArrayList; +import java.util.List; + +import org.junit.jupiter.api.parallel.Execution; +import org.junit.jupiter.api.parallel.ExecutionMode; +import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; + + +// Note: We don't actually test that these methods are constant time, just that they give the correct answers. +@ExtendWith(TestResultLogger.class) +@Execution(ExecutionMode.CONCURRENT) +public class ConstantTimeTests { + // A few common values which when combined can trigger edge cases + private static final int[] TEST_VALUES = {Integer.MIN_VALUE, Integer.MIN_VALUE + 1, -2, -1, 0, 1, 2, Integer.MAX_VALUE - 1, Integer.MAX_VALUE}; + private static final Class CONSTANT_TIME_CLASS; + static { + try { + CONSTANT_TIME_CLASS = Class.forName("com.amazon.corretto.crypto.provider.ConstantTime"); + } catch (Exception ex) { + throw new RuntimeException(ex); + } + } + + + public static List testPairs() { + final List result = new ArrayList<>(); + for (int a : TEST_VALUES) { + for (int b : TEST_VALUES) { + result.add(arguments(a, b)); + } + } + return result; + } + + public static int[] testSingles() { + return TEST_VALUES; + } + + @ParameterizedTest + @MethodSource("testSingles") + public void testIsNonZero(int val) { + final int expected = val != 0 ? 1 : 0; + assertEquals(expected, sneaky("isNonZero", val)); + } + + @ParameterizedTest + @MethodSource("testSingles") + public void testIsZero(int val) { + final int expected = val == 0 ? 1 : 0; + assertEquals(expected, sneaky("isZero", val)); + } + + @ParameterizedTest + @MethodSource("testSingles") + public void testIsNegative(int val) { + final int expected = val < 0 ? 1 : 0; + assertEquals(expected, sneaky("isNegative", val)); + } + + @ParameterizedTest + @MethodSource("testPairs") + public void testEqual(int x, int y) { + final int expected = x == y ? 1 : 0; + assertEquals(expected, sneaky("equal", x, y)); + } + + @ParameterizedTest + @MethodSource("testPairs") + public void testGt(int x, int y) { + final int expected = x > y ? 1 : 0; + assertEquals(expected, sneaky("gt", x, y)); + } + + @ParameterizedTest + @MethodSource("testSingles") + public void testSelect(int selector) { + final int a = 10; + final int b = 11; + final int expected = selector != 0 ? a : b; + assertEquals(expected, sneaky("select", selector, a, b)); + } + + private static int sneaky(String name, int a) { + try { + return TestUtil.sneakyInvoke_int(CONSTANT_TIME_CLASS, name, a); + } catch (final Throwable t) { + throw new AssertionError(t); + } + } + + private static int sneaky(String name, int a, int b) { + try { + return TestUtil.sneakyInvoke_int(CONSTANT_TIME_CLASS, name, a, b); + } catch (final Throwable t) { + throw new AssertionError(t); + } + } + + private static int sneaky(String name, int a, int b, int c) { + try { + return TestUtil.sneakyInvoke_int(CONSTANT_TIME_CLASS, name, a, b, c); + } catch (final Throwable t) { + throw new AssertionError(t); + } + } +} From 258badc26fc2bda6561570999c5bccdf8a6f73f8 Mon Sep 17 00:00:00 2001 From: Greg Rubin Date: Wed, 9 Sep 2020 22:13:55 +0000 Subject: [PATCH 05/14] Make trimZeros more constant time --- CHANGELOG.md | 1 + .../crypto/provider/EvpKeyAgreement.java | 32 +++++++++++++++---- 2 files changed, 26 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 21226825..35566d4f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ method. ### Improvements * Stricter guarantees about which curves are used for EC key generation. [PR #127](https://github.com/corretto/amazon-corretto-crypto-provider/pull/127) +* Reduce timing signal from trimming zeros of TLSPremasterSecrets from DH KeyAgreement. [PR #129](https://github.com/corretto/amazon-corretto-crypto-provider/pull/129) ### Patches * Add version gating to some tests introduced in 1.5.0 [PR #128](https://github.com/corretto/amazon-corretto-crypto-provider/pull/128) diff --git a/src/com/amazon/corretto/crypto/provider/EvpKeyAgreement.java b/src/com/amazon/corretto/crypto/provider/EvpKeyAgreement.java index 0d89e2cd..c852cab4 100644 --- a/src/com/amazon/corretto/crypto/provider/EvpKeyAgreement.java +++ b/src/com/amazon/corretto/crypto/provider/EvpKeyAgreement.java @@ -196,17 +196,35 @@ protected void reset() { } private static byte[] trimZeros(final byte[] secret) { - // According to other implementations, we don't appear - // to need to worry about timing leaks of this data. int bytesToTrim = 0; - while (bytesToTrim < secret.length && secret[bytesToTrim] == 0) { - bytesToTrim++; + int foundNonZero = 0; + for (int x = 0; x < secret.length; x++) { + final int currByte = secret[x]; + // Have we found something that isn't a zero? + foundNonZero |= currByte; + + // foundNonZero == 0 iff we have not see any non-zero bytes + // Thus, we should update bytesToTrim iff foundNonZero == 0 + final int shouldUpdateTrim = ConstantTime.isZero(foundNonZero); + bytesToTrim = ConstantTime.select(shouldUpdateTrim, x + 1, bytesToTrim); } - if (bytesToTrim == 0) { - return secret; + // Allocating arrays of different lengths always risks non-constant time operation. + // There is no way to avoid this. + final byte[] result = new byte[secret.length - bytesToTrim]; + + // We'll always do the same number of byte copies, but the leading zeros will be overwritten by valid ones. + // While the memory access pattern won't be identical, there is no way to completely avoid this. + for (int x = 0; x < secret.length; x++) { + final int realIndex = x - bytesToTrim; + + final int notYetValid = ConstantTime.isNegative(realIndex); + + final int indexToUpdate = ConstantTime.select(notYetValid, 0, realIndex); + result[indexToUpdate] = secret[x]; } - return Arrays.copyOfRange(secret, bytesToTrim, secret.length); + + return result; } static class ECDH extends EvpKeyAgreement { From c69cfe231c946ff1d340b64e9b53d0e03ac50ea4 Mon Sep 17 00:00:00 2001 From: Greg Rubin Date: Thu, 15 Oct 2020 00:24:06 +0000 Subject: [PATCH 06/14] Reuse state in MessageDigest --- CHANGELOG.md | 1 + .../amazon/corretto/crypto/provider/InputBuffer.java | 7 ++----- .../corretto/crypto/provider/TemplateHashSpi.java | 10 +++++++++- 3 files changed, 12 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 35566d4f..57c6e781 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ method. ### Improvements * Stricter guarantees about which curves are used for EC key generation. [PR #127](https://github.com/corretto/amazon-corretto-crypto-provider/pull/127) * Reduce timing signal from trimming zeros of TLSPremasterSecrets from DH KeyAgreement. [PR #129](https://github.com/corretto/amazon-corretto-crypto-provider/pull/129) +* Reuse state in `MessageDigest` to decrease object allocation rate. [PR #131](https://github.com/corretto/amazon-corretto-crypto-provider/pull/131) ### Patches * Add version gating to some tests introduced in 1.5.0 [PR #128](https://github.com/corretto/amazon-corretto-crypto-provider/pull/128) diff --git a/src/com/amazon/corretto/crypto/provider/InputBuffer.java b/src/com/amazon/corretto/crypto/provider/InputBuffer.java index 63ab5c1f..84bf8d9e 100644 --- a/src/com/amazon/corretto/crypto/provider/InputBuffer.java +++ b/src/com/amazon/corretto/crypto/provider/InputBuffer.java @@ -165,7 +165,7 @@ public static interface StateSupplier extends Supplier { //@ spec_public private /*@ { Consumer.Local } @*/ Consumer stateResetter = (ignored) -> { }; // NOP //@ spec_public - private StateSupplier stateSupplier = () -> null; + private StateSupplier stateSupplier = () -> state; //@ spec_public private Optional> stateCloner = Optional.empty(); // If absent, delegates to arrayUpdater @@ -229,10 +229,7 @@ public static interface StateSupplier extends Supplier { public void reset() { buff.reset(); firstData = true; - if (state != null) { - stateResetter.accept(state); - } - state = stateSupplier.get(); + state = null; /*@ set bytesReceived = 0; @ set bytesProcessed = 0; @ set bufferState = ((bufferState == BufferState.Uninitialized) diff --git a/template-src/com/amazon/corretto/crypto/provider/TemplateHashSpi.java b/template-src/com/amazon/corretto/crypto/provider/TemplateHashSpi.java index 14518dd4..a2a339ab 100644 --- a/template-src/com/amazon/corretto/crypto/provider/TemplateHashSpi.java +++ b/template-src/com/amazon/corretto/crypto/provider/TemplateHashSpi.java @@ -28,6 +28,7 @@ public final class TemplateHashSpi extends MessageDigestSpi implements Cloneable private static final int HASH_SIZE; private static final byte[] INITIAL_CONTEXT; + private byte[] myContext; private byte[] oneByteArray = null; private InputBuffer buffer; @@ -108,11 +109,17 @@ private static void synchronizedFinish(byte[] context, byte[] digest, int offset } } + private byte[] resetContext() { + System.arraycopy(INITIAL_CONTEXT, 0, myContext, 0, INITIAL_CONTEXT.length); + return myContext; + } + public TemplateHashSpi() { Loader.checkNativeLibraryAvailability(); + myContext = INITIAL_CONTEXT.clone(); this.buffer = new InputBuffer(1024) - .withInitialStateSupplier(INITIAL_CONTEXT::clone) + .withInitialStateSupplier(this::resetContext) .withUpdater(TemplateHashSpi::synchronizedUpdateContextByteArray) .withUpdater(TemplateHashSpi::synchronizedUpdateNativeByteBuffer) .withDoFinal((context) -> { @@ -158,6 +165,7 @@ public Object clone() { try { TemplateHashSpi clonedObject = (TemplateHashSpi)super.clone(); + clonedObject.myContext = myContext.clone(); clonedObject.buffer = (InputBuffer) buffer.clone(); return clonedObject; From 65ac4cd64a4e3e672bc00506a7b53f5fda7bd05d Mon Sep 17 00:00:00 2001 From: SalusaSecondus Date: Wed, 2 Dec 2020 12:44:04 -0800 Subject: [PATCH 07/14] Initial commit of the development guide (#134) * Initial commit of the development guide * Spelling correction * Simplify language * Incorporate feedback from @juneb * Add old code section and fix typos * Typo fixes * Typo fixes and Cleaner note * Add JNI section * Minor fixes * Add comment about native memory * Apply suggestions from code review Co-authored-by: June Blender * Address more grammatical feedback * Small typo fixes * Add explicit note re java_ex Co-authored-by: June Blender --- DEVELOPMENT-GUIDE.md | 169 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 169 insertions(+) create mode 100644 DEVELOPMENT-GUIDE.md diff --git a/DEVELOPMENT-GUIDE.md b/DEVELOPMENT-GUIDE.md new file mode 100644 index 00000000..aeedc63c --- /dev/null +++ b/DEVELOPMENT-GUIDE.md @@ -0,0 +1,169 @@ +# ACCP Development Guidance +Correctness and code safety are paramount in the Amazon Corretto Crypto Provider. + +This guide is designed to provide a quick introduction to the most important (and ACCP-specific) components to help developers find and learn what is most important. +It doesn't provide all information needed to develop ACCP. +This is a living document and we will continue to add lessons learned and other best practice as appropriate. + +# Development Principles + +In decreasing order of importance: + +1. We must be secure. + Using ACCP must *never* cause an application to be less secure than if it weren't used. +2. Correctness. + We must _never_ do the wrong thing or return the wrong result. + Subtle failures are easy to miss and can cause problems. + When something goes wrong, we should fail obviously and force the error to be properly handled. +3. All logic flows must be designed and explicit. + This means that there must be no dependencies on undefined behavior (in Java or C++). + (There is a single exception to this rule. We are permitted to depend on implicit `NullPointerException`s in Java.) +4. Testing ensures correctness. + 1. Known answer and compatibility tests are crucial. + Because we are implementing standards, we must be compatible with other implementations. Both the default implementation from Java and BouncyCastle are considered acceptable alternatives. (They both have had historical bugs and so both are used to work around issues in the other.) + When Known Answer Tests from standards are available, we must use them. (Some older tests do not yet meet this standard but compensate with compatibility tests.) + 2. Cryptographic error cases must be checked. + (While useful, API error cases should be checked but for simple type, null, and related errors, it is acceptable to miss some.) + 3. Different call patterns must be checked. + Many cryptographic APIs can be called in different ways. These must all be checked. + ([`MessageDigest` example](https://github.com/corretto/amazon-corretto-crypto-provider/blob/develop/src/com/amazon/corretto/crypto/provider/Utils.java#L297)) +5. ACCP must be fast. + This is one of the primary purposes of ACCP. + Benchmark your code, find bottlenecks, fix them. +6. Code should be *obviously* correct. + A developer should be able to look at an implementation and *know* it is correct with minimal reasoning or justification. + By implication, this means that you shouldn't be clever. + This principle sometimes needs to be sacrificed to support higher priority tenets. When this happens, we must do the following: + 1. Isolate complexity. (So that only a few methods or a single file is hard to understand.) + 2. Test to prove correctness. (While we must always do this, it is even more critical here.) + 3. Comment to explain exactly what is intended and, when appropriate, why a particular technique was chosen. + Examples: ([ConstantTime](https://github.com/corretto/amazon-corretto-crypto-provider/blob/develop/src/com/amazon/corretto/crypto/provider/ConstantTime.java) and its [tests](https://github.com/corretto/amazon-corretto-crypto-provider/blob/develop/tst/com/amazon/corretto/crypto/provider/test/ConstantTimeTests.java), [Janitor](https://github.com/corretto/amazon-corretto-crypto-provider/blob/develop/src/com/amazon/corretto/crypto/provider/Janitor.java) and its [tests](https://github.com/corretto/amazon-corretto-crypto-provider/blob/develop/tst/com/amazon/corretto/crypto/provider/test/JanitorTest.java)) +7. Old code *must* be updated to current best practices. + As we extend and improve ACCP, we will create new tools and frameworks to make our code better (cleaner, safer, easier to read, etc.). + Historical examples of this include `java_buffer`, `InputBuffer`, `NativeResource`, `raii_env` and others. + Even though old code will continue to work, we must go back and bring old code up to current standards and techniques. + This means that just because existing code was acceptable when it was written does not mean it is acceptable now. + Doing this allows us to continually raise the bar on code quality across the project and combat [bit rot](https://en.wikipedia.org/wiki/Software_rot). + + +# Important and Unique Components +## Java +### Janitor +ACCP never uses [finalizers](https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/lang/Object.html#finalize()) due to significant performance problems. +Since we still need to support Java8 for the foreseable future, we have implemented [Janitor](https://github.com/corretto/amazon-corretto-crypto-provider/blob/develop/src/com/amazon/corretto/crypto/provider/Janitor.java) as a JDK8+ replacement for the newer (since JDK9) [Cleaner](https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/lang/ref/Cleaner.html). When JDK8 is no longer supported we will re-evaluate `Cleaner` to see if it meets our performance requirements. To avoid circular dependency issues, `Janitor` *MUST NOT* depend on any other ACCP resources (directly or indirectly). It must remain entirely self contained. +The canonical example for using `Janitor` is `NativeResource`. + +### Loader +The [Loader](https://github.com/corretto/amazon-corretto-crypto-provider/blob/develop/src/com/amazon/corretto/crypto/provider/Loader.java) is responsible for bootstrapping the provider and loading the native library. To avoid circular dependencies `Loader` *MUST NOT* depend on any other classes or logic from within ACCP (with the sole exception of `Janitor`.) + +### NativeResource +ACCP commonly needs to track pointers to C++ objects (a.k.a., "native resources"). To ensure that they are properly managed, *all* of these pointers must be wrapped in a [NativeResource](https://github.com/corretto/amazon-corretto-crypto-provider/blob/develop/src/com/amazon/corretto/crypto/provider/NativeResource.java) object and all use of the pointer *must* be via the `use` or `useVoid` methods. This provides proper synchronization and cleanup of the resources. + +### InputBuffer +Many cryptographic constructs (MACs, Hashes, AEAD decrypt, and Signatures) take in an arbitrarily long input and return a single output at the end. +The [InputBuffer](https://github.com/corretto/amazon-corretto-crypto-provider/blob/develop/src/com/amazon/corretto/crypto/provider/InputBuffer.java) generalizes this flow by letting a specific implementation plug in a few pieces of logic while handling all of the buffering and type-handling logic in a single place. +This is useful because properly joining and splitting inbound data has caused bugs in other libraries (as has proper handling of `ByteBuffers`). + +## C++ +Whenever possible, we follow the philosophy that [Resource aquisition is initialization](https://en.wikipedia.org/wiki/Resource_acquisition_is_initialization). Whenever possible we will have stack-based objects tracking the lifecycle of heap-based resources. This ensures that when we leave a code-block the destructors will properly clean up the resources, regardless of how we leave the block. This, combined with C++ exceptions, results in a relatively easy to write/read control flow while being confident that we do not leak resources. This means that only the top-level JNI native methods should ever throw Java exceptions. In all cases `goto` should be avoided and throwing a C++ exception should be used for exceptional cases. + +Please remember that (unlike Java) all memory management is C++ is manual and it is very easy to leak memory. +Even memory which hasn't been truly "leaked" (because it is being properly tracked by a `NativeResource` object and so will cleaned eventually) can create an out of memory situation. +The issue is that Java has no visibility into the size of native objects. +This means that all `NativeResource` objects appear to be quite small to Java. +If you have a large amount of native memory allocated (enough to create memory pressure on the system), Java cannot know that it needs to run the GC and clean up the (small) `NativeResource` objects to free the potentially large amount of native memory. + +### *_auto +Whenever possible, use classes that provide stack-based tracking. These classes have names with the form `*_auto` (ex: `RSA_auto`). + +### BigNumObj +`BigNumObj ` is essentially a `*_auto` object for `BN` resources, but with `BN` specific logic attached to make it easier to use. + +### java_ex +`java_ex` is a C++ exception which represents a Java exception and can be converted to one (immediately prior to returning to Java) by calling `throw_to_java(JNIEnv*)` + +### raii_env +`raii_env` wraps an instance of `JNIEnv*` and *MUST* always be used (except for calling `java_ex.throw_to_java()`) and *MUST* always be passed by reference. +It's primary purpose is to check invariants around critical regions and prevent coding mistakes by failing hard if `JNIEnv*` is used incorrectly. + +### java_buffer +Representation of any sequence of bytes passed in from Java (currently either a `DirectByteBuffer` or a `byte[]`). This does not give direct access to the underlying bytes. + +### jni_borrow +Actual representation of bytes from a `java_buffer`. Due to their interactions with the JVM and GC, they should exist for as short a time as possible. +While a `jni_borrow` exists, ACCP may be in a critial region and thus any use of `raii_env` will result in the process aborting. + +### jni_string +Represents a `String` object from Java and gives access to the UTF-8 encoded contents. + +### SecureBuffer +The `SecureBuffer` represents a fixed-length array of a type (usually `uint8_t`) which will always zero itself upon destruction. + +# About JNI (Java Native Interface) +The [Java Native Interface](https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/jniTOC.html) (JNI) is a standard way for standard Java code to interact with native code (such as that written in C or C++). +The JNI (specifically version 6.0 as supported by JDK8 and linked above) is a core component of ACCP's implementation as it allows us to connect high-performance implementations in C/C++ with callers from Java. +Unfortunately, JNI development can be tricky. If not done properly, it jeopardizes correctness, stability, and performance . +There are no shortcuts here. [Read the documentation](https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/jniTOC.html). +The most important sections of the official guide are: +* [Introduction](https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/intro.html) +* [Java Exceptions](https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/design.html#java_exceptions) +* [JNI Types and Data Structures](https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/types.html) +* [Array Operations](https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/functions.html#array_operations) + +## How ACCP uses the JNI +Just as in many other areas of development, there are a wide range of acceptable styles for JNI code. +ACCP has adopted the following guidelines and patterns to make code more efficient as well as easier to write and review. + +### Avoid native calls to Java +Crossing the JNI boundary in either direction is expensive. However, crossing it from Native to Java is far worse. +For this reason, avoid calls that interact with `JNIEnv` (or, more properly, `raii_env` in ACCP). +As a result: +* Native code should never touch a Java object +* Native code should never call a Java method +* All parameters to native methods should be either primitives or primitive arrays. + (Technically, all arrays are Java objects and thus expensive to touch; however this is an unavoidable compromise). +* Logic should use native data structures and only translate from and to Java results at the beginning and end of the top-most methods. +* Native methods should all be `static`. + While this technically isn't necessary, if they aren't allowed to touch Java objects, then there is no need for them to be passed a reference to `this`. + +One result of this design decision is that ACCP's code can be (roughly) divided into four layers. +These are not *currently* formally marked, but that may change in the future. + +Each layer from highest level (Java) down: +1. Pure (normal) Java. This layer is called by external (non-ACCP) Java code. +2. Java->Primitive Translation. This layer is written in Java and converts from Java objects to primitives and actually calls the `native` methods. +3. JNI->Native Translation. This layer is written in C++ and converts from JNI objects to native or ACCP implemented structures (such as `java_buffer`). + This layer is also responsible for translating C++ exceptions thrown by the lowest layer to Java exceptions. +4. Native. This layer is written in C++ and actually does the logic. It should rarely (if ever) directly interact with `JNIEnv`/`raii_env` and should rarely (if ever) throw a Java exception. + +### Error Handling +Once a [Java Exception](https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/design.html#java_exceptions) has been thrown, there are exceedingly few JNI operations a caller is allowed to perform. +What's more, it is easy to not notice that there is a Java exception pending. +So long as you do not interact with `JNIEnv`/`raii_env` directly, you shouldn't need to worry about Java exceptions as the ACCP objects/methods which use them already have appropriate checks which throw C++ exceptions (specifically `java_ex`) instead. +If you need to throw an exception, it should almost always be a C++ exception and thrown using the `throw_java_ex` or `throw_openssl` methods. (The former is more efficient but the latter MUST be used when the exception is due to an error state reported by OpenSSL.) +If you are throwing a C++ exception then it *must* be an instance of `java_ex`. +(This is correctly done for you by both `throw_java_ex` and `throw_openssl`.) + +Openssl has its *own* separate error handling in the form of a thread-local queue. +This has caused bugs in the past where consuming code has not noticed that errors were present on the stack and so later calls incorrectly saw old and irrelevant Openssl errors. +`throw_openssl` correctly checks *and clears* the OpenSSL error queue prior to throwing the C++ exception. +It also will use the OpenSSL provided error message if available. +If there is no OpenSSL error or applicable message, it will use the provided default message. + +Making sure that the OpenSSL error queue is empty prior to returning from native code is critical. +This is why *during coverage tests* we enable extra assertions which will terminate the process if any unhandled OpenSSL errors are present. +These assertions are only present during the coverage tests as they can be expensive (especially on multi-threaded systems). + +### Critical Regions +There are two different ways to read data from Java arrays and Strings: Copying, or Critical Regions. +* [String Critical methods](https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/functions.html#GetStringCritical_ReleaseStringCritical) +* [Array Critical methods](https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/functions.html#GetPrimitiveArrayCritical_ReleasePrimitiveArrayCritical) + +ACCP generally uses critical regions because they are (usually) faster. +When you are in a critical region the JVM (generally) pins the object being handled to a specific memory location and gives you direct access to the underlying data. +This means that the JVM is unable to do many of its memory management functions (as they require moving objects) and is operating essentially in a degraded mode. (No garbage collection, etc.) +So, when you are in a critical region there are exceedingly few operations you can safely do. +Essentially, all you can do is methods which manipulate your critical region until you get out of it. +It is **extremely important** that you do not allocate *any* Java memory (such as creating Java objects, like a Java Exception), call *any* Java methods (which you shouldn't be doing anyway), or take any actions which may block. +It is also **very important** that you don't spend too much time (more than about a millisecond) in a critical region. +If you are concerned that an operation is not reasonably time-bounded, you should probably be sure to release the critical region on a regular basis to ensure the JVM has the opportunity to do needed memory management. (This isn't always achievable when everything is happening within a single atomic cryptographic operation.) From 3ea3061ba4c5f0de0e2c4744cdee8da7d408cb16 Mon Sep 17 00:00:00 2001 From: SalusaSecondus Date: Tue, 8 Dec 2020 13:07:42 -0800 Subject: [PATCH 08/14] Move to openssl 1.1.1i (#136) * Move to openssl 1.1.1i * Clarify impact of 1.1.1i --- CHANGELOG.md | 2 ++ README.md | 2 +- build.gradle | 2 +- openssl.sha256 | 2 +- 4 files changed, 5 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 57c6e781..690b87a4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,8 @@ method. * Stricter guarantees about which curves are used for EC key generation. [PR #127](https://github.com/corretto/amazon-corretto-crypto-provider/pull/127) * Reduce timing signal from trimming zeros of TLSPremasterSecrets from DH KeyAgreement. [PR #129](https://github.com/corretto/amazon-corretto-crypto-provider/pull/129) * Reuse state in `MessageDigest` to decrease object allocation rate. [PR #131](https://github.com/corretto/amazon-corretto-crypto-provider/pull/131) +* Now uses [OpenSSL 1.1.1i](https://www.openssl.org/source/openssl-1.1.1i.tar.gz). [PR #136](https://github.com/corretto/amazon-corretto-crypto-provider/pull/136) + (ACCP is not impacted by [CVE-2020-1971](https://www.openssl.org/news/secadv/20201208.txt) as ACCP does not use or expose any of the relevant functionality.) ### Patches * Add version gating to some tests introduced in 1.5.0 [PR #128](https://github.com/corretto/amazon-corretto-crypto-provider/pull/128) diff --git a/README.md b/README.md index e99ac168..f7239119 100644 --- a/README.md +++ b/README.md @@ -1,7 +1,7 @@ # Amazon Corretto Crypto Provider The Amazon Corretto Crypto Provider (ACCP) is a collection of high-performance cryptographic implementations exposed via the standard [JCA/JCE](https://docs.oracle.com/en/java/javase/11/security/java-cryptography-architecture-jca-reference-guide.html) interfaces. This means that it can be used as a drop in replacement for many different Java applications. -Currently algorithms are primarily backed by OpenSSL's implementations (1.1.1g as of ACCP 1.5.0) but this may change in the future. +Currently algorithms are primarily backed by OpenSSL's implementations (1.1.1i as of ACCP 1.6.0) but this may change in the future. [Security issue notifications](./CONTRIBUTING.md#security-issue-notifications) diff --git a/build.gradle b/build.gradle index a87ea8b4..18bc970d 100644 --- a/build.gradle +++ b/build.gradle @@ -9,7 +9,7 @@ plugins { group = 'software.amazon.cryptools' version = '1.6.0' -def openssl_version = '1.1.1g' +def openssl_version = '1.1.1i' def opensslSrcPath = "${buildDir}/openssl/openssl-${openssl_version}" configurations { diff --git a/openssl.sha256 b/openssl.sha256 index 44803ceb..4b937300 100644 --- a/openssl.sha256 +++ b/openssl.sha256 @@ -1 +1 @@ -ddb04774f1e32f0c49751e21b67216ac87852ceb056b75209af2443400636d46 openssl-1.1.1g.tar.gz +e8be6a35fe41d10603c3cc635e93289ed00bf34b79671a3a4de64fcee00d5242 openssl-1.1.1i.tar.gz From 5c04e5dd04c276637ea6c20af683b013fd175209 Mon Sep 17 00:00:00 2001 From: Greg Rubin Date: Fri, 18 Dec 2020 00:48:26 +0000 Subject: [PATCH 09/14] Better output size estimates. Fixes #135 --- CHANGELOG.md | 1 + .../corretto/crypto/provider/AesGcmSpi.java | 9 +- .../crypto/provider/test/AesTest.java | 111 +++++++++++++++++- 3 files changed, 111 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 690b87a4..da55f041 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ method. ### Patches * Add version gating to some tests introduced in 1.5.0 [PR #128](https://github.com/corretto/amazon-corretto-crypto-provider/pull/128) +* More accurate output size estimates from `Cipher.getOutputSize()` [PR #138](https://github.com/corretto/amazon-corretto-crypto-provider/pull/138) ## 1.5.0 ### Breaking Change Warning diff --git a/src/com/amazon/corretto/crypto/provider/AesGcmSpi.java b/src/com/amazon/corretto/crypto/provider/AesGcmSpi.java index cd9b66f5..023df3c1 100644 --- a/src/com/amazon/corretto/crypto/provider/AesGcmSpi.java +++ b/src/com/amazon/corretto/crypto/provider/AesGcmSpi.java @@ -237,9 +237,7 @@ protected int engineGetKeySize(final Key key) throws InvalidKeyException { protected int engineGetOutputSize(int inputLen) { switch (opMode) { case NATIVE_MODE_ENCRYPT: - // Ensure we have room (on doFinal) for either an extra ciphertext block that was buffered in OpenSSL, - // or the final tag (whichever is bigger) - return inputLen + Math.max(tagLength, engineGetBlockSize() - 1); + return getUpdateOutputSize(inputLen) + tagLength; case NATIVE_MODE_DECRYPT: return Math.max(0, decryptInputBuf.size() + inputLen - tagLength); default: @@ -253,7 +251,7 @@ protected int engineGetOutputSize(int inputLen) { private synchronized int getUpdateOutputSize(int inputLen) { switch (opMode) { case NATIVE_MODE_ENCRYPT: - return inputLen + engineGetBlockSize() - 1; + return inputLen; case NATIVE_MODE_DECRYPT: // We do not return data from engineUpdate when decrypting - all data is returned from engineDoFinal() return 0; @@ -854,6 +852,9 @@ protected int engineDoFinal(ByteBuffer input, ByteBuffer output) throws ShortBuf } private void checkOutputBuffer(int inputLength, byte[] output, int outputOffset) throws ShortBufferException { + if (inputLength < 0 || outputOffset < 0 || outputOffset > output.length) { + throw new ArrayIndexOutOfBoundsException(); + } if (output.length - outputOffset < getUpdateOutputSize(inputLength)) { throw new ShortBufferException("Expected a buffer of at least " + engineGetOutputSize(inputLength) + " bytes; got " + (output.length - outputOffset)); } diff --git a/tst/com/amazon/corretto/crypto/provider/test/AesTest.java b/tst/com/amazon/corretto/crypto/provider/test/AesTest.java index f54c1f15..0dbe5c9a 100644 --- a/tst/com/amazon/corretto/crypto/provider/test/AesTest.java +++ b/tst/com/amazon/corretto/crypto/provider/test/AesTest.java @@ -51,6 +51,10 @@ import org.junit.jupiter.api.parallel.ExecutionMode; import org.junit.jupiter.api.parallel.ResourceAccessMode; import org.junit.jupiter.api.parallel.ResourceLock; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; + @ExtendWith(TestResultLogger.class) @Execution(ExecutionMode.SAME_THREAD) @@ -206,6 +210,106 @@ public void overlap_encrypt() throws Exception { } } + public static List estimateOutputParams() { + final int[] plaintextSizes = {0, 1, 7, 8, 9, 15, 16}; + final int[] tagLengthsInBits = {96, 112, 128}; + List result = new ArrayList<>(); + for (int tagLengthInBits: tagLengthsInBits) { + for (int first: plaintextSizes) { + for (int second: plaintextSizes) { + result.add(Arguments.of(first, second, tagLengthInBits)); + } + } + } + return result; + } + + @ParameterizedTest + @MethodSource("estimateOutputParams") + public void encryptEstimatesCorrectly(int prefixLength, int testInputLength, int tagLengthInBits) throws GeneralSecurityException { + assumeMinimumVersion("1.6.0", AmazonCorrettoCryptoProvider.INSTANCE); + amznC.init(Cipher.ENCRYPT_MODE, key, new GCMParameterSpec(tagLengthInBits, new byte[12])); + + // We sometimes encrypt a bit before the test case to catch if anything is cached + if (prefixLength != 0) { + amznC.update(new byte[prefixLength]); // Ignore output as it isn't helpful + } + + final int estimatedLength = amznC.getOutputSize(testInputLength); + assertEquals(testInputLength + tagLengthInBits / 8, estimatedLength); + + byte[] output = amznC.update(new byte[testInputLength]); + if (testInputLength == 0) { // As per the Javadoc for Cipher.update(byte[]) + assertNull(output); + } else { + assertEquals(testInputLength, output.length); + } + byte[] tag = amznC.doFinal(); + assertEquals(tagLengthInBits / 8, tag.length); + } + + @ParameterizedTest + @MethodSource("estimateOutputParams") + public void encryptEstimatesCorrectlyPlacement(int prefixLength, int testInputLength, int tagLengthInBits) throws GeneralSecurityException { + assumeMinimumVersion("1.6.0", AmazonCorrettoCryptoProvider.INSTANCE); + amznC.init(Cipher.ENCRYPT_MODE, key, new GCMParameterSpec(tagLengthInBits, new byte[12])); + + // We sometimes encrypt a bit before the test case to catch if anything is cached + if (prefixLength != 0) { + amznC.update(new byte[prefixLength]); // Ignore output as it isn't helpful + } + + final int estimatedLength = amznC.getOutputSize(testInputLength); + assertEquals(testInputLength + tagLengthInBits / 8, estimatedLength); + + final byte[] output = new byte[testInputLength]; // AES-GCM should not change the length when encrypting (until doFinal) + final int actualOutputLength = amznC.update(new byte[testInputLength], 0, testInputLength, output, 0); + + assertEquals(testInputLength, actualOutputLength); + + byte[] tag = amznC.doFinal(); + assertEquals(tagLengthInBits / 8, tag.length); + } + + @ParameterizedTest + @MethodSource("estimateOutputParams") + public void encryptEstimatesCorrectlyFinal(int prefixLength, int testInputLength, int tagLengthInBits) throws GeneralSecurityException { + assumeMinimumVersion("1.6.0", AmazonCorrettoCryptoProvider.INSTANCE); + amznC.init(Cipher.ENCRYPT_MODE, key, new GCMParameterSpec(tagLengthInBits, new byte[12])); + + // We sometimes encrypt a bit before the test case to catch if anything is cached + if (prefixLength != 0) { + amznC.update(new byte[prefixLength]); // Ignore output as it isn't helpful + } + + final int estimatedLength = amznC.getOutputSize(testInputLength); + assertEquals(testInputLength + tagLengthInBits / 8, estimatedLength); + + final byte[] output = amznC.doFinal(new byte[testInputLength]); + assertEquals(estimatedLength, output.length); + } + + @ParameterizedTest + @MethodSource("estimateOutputParams") + public void encryptEstimatesCorrectlyPlacementFinal(int prefixLength, int testInputLength, int tagLengthInBits) throws GeneralSecurityException { + assumeMinimumVersion("1.6.0", AmazonCorrettoCryptoProvider.INSTANCE); + amznC.init(Cipher.ENCRYPT_MODE, key, new GCMParameterSpec(tagLengthInBits, new byte[12])); + + // We sometimes encrypt a bit before the test case to catch if anything is cached + if (prefixLength != 0) { + amznC.update(new byte[prefixLength]); // Ignore output as it isn't helpful + } + + final int estimatedLength = amznC.getOutputSize(testInputLength); + assertEquals(testInputLength + tagLengthInBits / 8, estimatedLength); + + final byte[] output = new byte[estimatedLength]; + final int actualOutputLength = amznC.doFinal(new byte[testInputLength], 0, testInputLength, output, 0); + + assertEquals(estimatedLength, actualOutputLength); + } + + @Test public void large_overlap_encrypt() { // modes: @@ -400,11 +504,6 @@ public void test_getOutputSize_encrypt() throws Throwable { // Allows room for the final tag assertEquals(12345 + 16, sneakyInvoke_int(spi, "engineGetOutputSize", 12345)); - - // Allows room for a partial buffered block - sneakyInvoke(spi, "engineInit", Cipher.ENCRYPT_MODE, key, - new GCMParameterSpec(12 * 8, randomIV()), rnd); - assertEquals(12345 + 15, sneakyInvoke_int(spi, "engineGetOutputSize", 12345)); } @Test @@ -538,7 +637,7 @@ public void test_bufferOverflows() throws Throwable { sneakyInvoke(spi, "engineInit", Cipher.ENCRYPT_MODE, key, new GCMParameterSpec(128, randomIV()), rnd); assertThrows(ShortBufferException.class, - () -> sneakyInvoke(spi, "engineUpdate", new byte[16], 0, 16, new byte[32], 16)); + () -> sneakyInvoke(spi, "engineUpdate", new byte[16], 0, 16, new byte[32], 17)); sneakyInvoke(spi, "engineInit", Cipher.ENCRYPT_MODE, key, new GCMParameterSpec(128, randomIV()), rnd); From 20d859696d0bc762a63aa48ebde00b6aaeaf5ee7 Mon Sep 17 00:00:00 2001 From: SalusaSecondus Date: Wed, 13 Jan 2021 18:36:12 -0800 Subject: [PATCH 10/14] Add list of known differences (#137) Co-authored-by: June Blender --- DIFFERENCES.md | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++ README.md | 1 + 2 files changed, 82 insertions(+) create mode 100644 DIFFERENCES.md diff --git a/DIFFERENCES.md b/DIFFERENCES.md new file mode 100644 index 00000000..04f3aa67 --- /dev/null +++ b/DIFFERENCES.md @@ -0,0 +1,81 @@ +# Important Differences +The [JCA/JCE](https://docs.oracle.com/en/java/javase/11/security/java-cryptography-architecture-jca-reference-guide.html) specification does not completely define all behaviors of a given provider. +Although the Amazon Corretto Crypto Provider (ACCP) is fully compliant with the JCE, its behavior differs from the behavior of the default Java providers in several ways. +The following list of behavioral differences is not exhaustive, but is intended to capture the most important differences. + +Despite these differences, ACCP remains a drop-in replacement for the vast majority of Java applications and doesn't change the application behavior, other than improving its performance. + +# Behavior Changes +These differences are those most likely to be noticed by a consuming application. You might need to make minor changes to accommodate them. + +## SignatureException +The official documentation does not fully specify when the [Signature](https://docs.oracle.com/javase/8/docs/api/java/security/Signature.html) object is expected to throw a [SignatureException](https://docs.oracle.com/javase/8/docs/api/java/security/SignatureException.html). +Having multiple different ways to reject a signature (such as `signature.verify() == false` and throwing a `SignatureException`) is an anti-pattern and we should try to avoid it. +ACCP throws a `SignatureException` from `Signature.verify()` only when not throwing would introduce compatibility issues (such as with the [JCK](https://en.wikipedia.org/wiki/Technology_Compatibility_Kit#TCK_for_the_Java_platform).) +Currently, ACCP will throw a `SignatureException` only when verifying an EDSA or DSA signature that is not properly encoded. +In all other cases, ACCP will return `false` from `Signature.verify()` when given an invalid signature. +This is different from the default OpenJDK implementation, which also inspects the inner structure of RSA signatures and rejects them with a `SignatureException` if they are improperly encoded. +ACCP follows the guidance provided in [PKCS #1 section 8.2.2](https://tools.ietf.org/html/rfc8017#section-8.2.2) in that it does not parse the inner structure but, instead, does a binary comparison against the expected value. + +For this reason, regardless of whether you use ACCP or not, we recommend the following structure for signature verification: +```java + Signature signatureObject = Signature.getInstance(SIGNATURE_ALGORITHM); + signatureObject.initVerify(publicKey); + signatureObject.update(messageToVerify); + boolean signatureValid = false; + try { + signatureValid = signatureObject.verify(signature); + } catch (final SignatureException ex) { + signatureValid = false; + } +``` + +## Elliptic Curve KeyPairGeneration by curve size +Neither the JCE nor the default OpenJDK provider for Elliptic Curve Cryptography (SunEC) specify the effect of calling `KeyPairGenerator.initialize(int keysize)` with an arbitrary value. +This behavior is fully specified only for values of 192, 224, 256, 384, and 521. +This means that applications cannot depend on receiving a specific curve for any other value. Also, the application might encounter compatibility issues if SunEC ever changes its behavior or if the application changes to a different JCE provider. +In ACCP (after version 1.5.0), the `KeyPairGenerator.initialize(int keysize)` method fails with an `InvalidParameterException` for `keysize` values other than 192, 224, 256, 384, and 521. + +For this reason, even if you don't use ACCP, we recommend that you use only the [KeyPairGenerator.initialize(AlgorithmParameterSpec params)](https://docs.oracle.com/javase/8/docs/api/java/security/KeyPairGenerator.html#initialize-java.security.spec.AlgorithmParameterSpec-) method with an [ECGenParameterSpec](https://docs.oracle.com/javase/8/docs/api/java/security/spec/ECGenParameterSpec.html) to generate EC keys. +This construction is safe for all known JCE providers and is expected to remain safe even if the behavior of a provider changes. + +For more information, see the [changelog](./CHANGELOG.md) notes for version 1.5.0. + +## Cipher.getOutputSize() for AES-GCM +ACCP might overestimate the amount of space needed when encrypted with `AES/GCM/NoPadding`. +While this is compliant with the JCE (which [permits overestimation](https://docs.oracle.com/javase/8/docs/api/javax/crypto/Cipher.html#getOutputSize-int-)) it has caused confusion for some developers. +We are tracking this as [issue #135](https://github.com/corretto/amazon-corretto-crypto-provider/issues/135) and will improve this behavior. + +## SecureRandom is never deterministic +Some implementation of `SecureRandom` (such as `SHA1PRNG`, provided by the default OpenJDK cryptographic providers) can operate deterministically if `SecureRandom.setSeed(byte[])` is called prior to any other methods. +This behavior allows for insecure seeding and might make the application less secure if it requires the `SecureRandom` instance to provide secure entropy (such as for cryptographic use). +The `SecureRandom` implementation provided by ACCP automatically seeds itself upon creation and cannot be used in a deterministic manner. +This change is relevant only to systems that need deterministic behavior based on a seed, such as in some simulations. +Systems that need deterministic behavior should not use an ACCP implementation of `SecureRandom`. They should select an implementation/algorithm that specifically meets their needs. + +## SecureRandom uses thread local state internally +To avoid the costs of both RNG initialization and thread contention, ACCP maintains a single internal instance of SecureRandom for each thread. +Any time an instance of `SecureRandom` is used, ACCP routes the requests to the appropriate backing instance for the calling thread. +Because the output of calls to `SecureRandom` is computationally indistinguishable from actual random data, this implementation detail has no impact on callers other than improving performance. + +# Extensions +Applications are unlikely to directly encounter any of these changes but may choose to take advantage of them. + +## AES-GCM supports IvParameterSpec +ACCP allows use of [IvParameterSpec](https://docs.oracle.com/javase/8/docs/api/javax/crypto/spec/IvParameterSpec.html) when calling [Cipher.init()](https://docs.oracle.com/javase/8/docs/api/javax/crypto/Cipher.html#init-int-java.security.Key-java.security.spec.AlgorithmParameterSpec-). +This is equivalent to using a [GCMParameterSpec](https://docs.oracle.com/javase/8/docs/api/javax/crypto/spec/GCMParameterSpec.html) with the same IV value and a tag length of 128 bits. +By supporting the same ParameterSpec as other ciphers (such as `AES/CBC/PKCS5Padding`, which should not be used as it is no longer secure), ACCP makes it easier to migrate to the secure choice of `AES/GCM/NoPadding`. +(This behavior is identical to how [BouncyCastle](https://bouncycastle.org/java.html) treats `IvParameterSpec` when used with AES-GCM.) + +## KeyAgreement supports reuse without reinitialization +ACCP permits reuse of a [KeyAgreement](https://docs.oracle.com/javase/8/docs/api/javax/crypto/KeyAgreement.html) object without calling `.init()` more than once. +This results in better performance for Static-Ephemeral key agreement protocols. + +## AES is supported as a target key type for all KeyAgreement algorithms and supports an explicit size +[KeyAgreement.generateSecret(String)](https://docs.oracle.com/javase/8/docs/api/javax/crypto/KeyAgreement.html#generateSecret-java.lang.String-) can be called with an input of "AES" for all Key Agreement algorithms. +(The default Java implementation does not support "AES" as input with "ECDH" key agreement.) +If "AES" is passed to this method, ACCP returns the largest possible AES key corresponding to the agreed secret. +Alternatively, you can request an AES key of a particular size by appending the size (in bits) surrounded by brackets to this string. +(Ex: "AES[128]" or "AES[256]") +This returns a key of the requested strength or an `InvalidKeyException` if the agreed secret is not long enough for the requested AES key length. +(This method of specifying key size is identical to the way [BouncyCastle](https://bouncycastle.org/java.html) specifies key size for `KeyAgreement.generateSecret(String)`.) diff --git a/README.md b/README.md index f7239119..2b07f3a1 100644 --- a/README.md +++ b/README.md @@ -1,6 +1,7 @@ # Amazon Corretto Crypto Provider The Amazon Corretto Crypto Provider (ACCP) is a collection of high-performance cryptographic implementations exposed via the standard [JCA/JCE](https://docs.oracle.com/en/java/javase/11/security/java-cryptography-architecture-jca-reference-guide.html) interfaces. This means that it can be used as a drop in replacement for many different Java applications. +(Differences from the default OpenJDK implementations are [documented here](./DIFFERENCES.md).) Currently algorithms are primarily backed by OpenSSL's implementations (1.1.1i as of ACCP 1.6.0) but this may change in the future. [Security issue notifications](./CONTRIBUTING.md#security-issue-notifications) From c518c38615798df9baef5c4702fd048066dbd70e Mon Sep 17 00:00:00 2001 From: Alex Chew Date: Mon, 8 Mar 2021 16:39:43 -0800 Subject: [PATCH 11/14] Move to OpenSSL 1.1.1j (#145) * Move to OpenSSL 1.1.1j * Update PR number in CHANGELOG --- CHANGELOG.md | 5 +++-- README.md | 2 +- build.gradle | 4 ++-- openssl.sha256 | 2 +- 4 files changed, 7 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index da55f041..731e3162 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,8 +12,9 @@ method. * Stricter guarantees about which curves are used for EC key generation. [PR #127](https://github.com/corretto/amazon-corretto-crypto-provider/pull/127) * Reduce timing signal from trimming zeros of TLSPremasterSecrets from DH KeyAgreement. [PR #129](https://github.com/corretto/amazon-corretto-crypto-provider/pull/129) * Reuse state in `MessageDigest` to decrease object allocation rate. [PR #131](https://github.com/corretto/amazon-corretto-crypto-provider/pull/131) -* Now uses [OpenSSL 1.1.1i](https://www.openssl.org/source/openssl-1.1.1i.tar.gz). [PR #136](https://github.com/corretto/amazon-corretto-crypto-provider/pull/136) - (ACCP is not impacted by [CVE-2020-1971](https://www.openssl.org/news/secadv/20201208.txt) as ACCP does not use or expose any of the relevant functionality.) +* Now uses [OpenSSL 1.1.1j](https://www.openssl.org/source/openssl-1.1.1j.tar.gz). [PR #145](https://github.com/corretto/amazon-corretto-crypto-provider/pull/145) + (ACCP is not impacted by [CVE-2020-1971](https://www.openssl.org/news/secadv/20201208.txt), [CVE-2021-23841](https://www.openssl.org/news/secadv/20210216.txt), or [CVE-2021-23839](https://www.openssl.org/news/secadv/20210216.txt) as ACCP does not use or expose any of the relevant functionality. + ACCP is not impacted by [CVE-2021-23840](https://www.openssl.org/news/secadv/20210216.txt) as ACCP does not use the relevant functionality under the affected conditions.) ### Patches * Add version gating to some tests introduced in 1.5.0 [PR #128](https://github.com/corretto/amazon-corretto-crypto-provider/pull/128) diff --git a/README.md b/README.md index 2b07f3a1..59194536 100644 --- a/README.md +++ b/README.md @@ -2,7 +2,7 @@ The Amazon Corretto Crypto Provider (ACCP) is a collection of high-performance cryptographic implementations exposed via the standard [JCA/JCE](https://docs.oracle.com/en/java/javase/11/security/java-cryptography-architecture-jca-reference-guide.html) interfaces. This means that it can be used as a drop in replacement for many different Java applications. (Differences from the default OpenJDK implementations are [documented here](./DIFFERENCES.md).) -Currently algorithms are primarily backed by OpenSSL's implementations (1.1.1i as of ACCP 1.6.0) but this may change in the future. +Currently algorithms are primarily backed by OpenSSL's implementations (1.1.1j as of ACCP 1.6.0) but this may change in the future. [Security issue notifications](./CONTRIBUTING.md#security-issue-notifications) diff --git a/build.gradle b/build.gradle index 18bc970d..a82d1591 100644 --- a/build.gradle +++ b/build.gradle @@ -9,7 +9,7 @@ plugins { group = 'software.amazon.cryptools' version = '1.6.0' -def openssl_version = '1.1.1i' +def openssl_version = '1.1.1j' def opensslSrcPath = "${buildDir}/openssl/openssl-${openssl_version}" configurations { @@ -453,4 +453,4 @@ task clean(overwrite: true, type: Delete) { task deep_clean(type: Delete) { delete buildDir -} \ No newline at end of file +} diff --git a/openssl.sha256 b/openssl.sha256 index 4b937300..65e92eaa 100644 --- a/openssl.sha256 +++ b/openssl.sha256 @@ -1 +1 @@ -e8be6a35fe41d10603c3cc635e93289ed00bf34b79671a3a4de64fcee00d5242 openssl-1.1.1i.tar.gz +aaf2fcb575cdf6491b98ab4829abf78a3dec8402b8b81efc8f23c00d443981bf openssl-1.1.1j.tar.gz From 6bead185670e4177948ce9fef2deb0fa863079b6 Mon Sep 17 00:00:00 2001 From: Alex Chew Date: Fri, 12 Mar 2021 16:56:49 -0800 Subject: [PATCH 12/14] Validate that AesGcmSpi#engineInit gets non-null key (#146) * Validate that AesGcmSpi#engineInit gets non-null key * Update CHANGELOG * Only run AesTest#test_initNullKey for appropriate versions Co-authored-by: SalusaSecondus * make `key` final * Fix indent * bump to re-run CI checks Co-authored-by: SalusaSecondus --- CHANGELOG.md | 1 + .../corretto/crypto/provider/AesGcmSpi.java | 4 ++++ .../crypto/provider/test/AesTest.java | 21 +++++++++++++++++++ 3 files changed, 26 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 731e3162..7dca93d9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,7 @@ method. ### Patches * Add version gating to some tests introduced in 1.5.0 [PR #128](https://github.com/corretto/amazon-corretto-crypto-provider/pull/128) * More accurate output size estimates from `Cipher.getOutputSize()` [PR #138](https://github.com/corretto/amazon-corretto-crypto-provider/pull/138) +* Validate that `AesGcmSpi` receives a non-null key on init to prevent unncessarily late NPE [PR #146](https://github.com/corretto/amazon-corretto-crypto-provider/pull/146) ## 1.5.0 ### Breaking Change Warning diff --git a/src/com/amazon/corretto/crypto/provider/AesGcmSpi.java b/src/com/amazon/corretto/crypto/provider/AesGcmSpi.java index 023df3c1..90a9bef4 100644 --- a/src/com/amazon/corretto/crypto/provider/AesGcmSpi.java +++ b/src/com/amazon/corretto/crypto/provider/AesGcmSpi.java @@ -294,6 +294,10 @@ protected synchronized void engineInit(int opMode, Key key, SecureRandom secureR protected synchronized void engineInit( int jceOpMode, Key key, AlgorithmParameterSpec algorithmParameterSpec, SecureRandom secureRandom ) throws InvalidKeyException, InvalidAlgorithmParameterException { + if (key == null) { + throw new InvalidKeyException("Key can't be null"); + } + final GCMParameterSpec spec; if (algorithmParameterSpec instanceof GCMParameterSpec) { spec = (GCMParameterSpec) algorithmParameterSpec; diff --git a/tst/com/amazon/corretto/crypto/provider/test/AesTest.java b/tst/com/amazon/corretto/crypto/provider/test/AesTest.java index 0dbe5c9a..610c0568 100644 --- a/tst/com/amazon/corretto/crypto/provider/test/AesTest.java +++ b/tst/com/amazon/corretto/crypto/provider/test/AesTest.java @@ -23,9 +23,11 @@ import java.security.GeneralSecurityException; import java.security.InvalidAlgorithmParameterException; import java.security.InvalidKeyException; +import java.security.Key; import java.security.NoSuchAlgorithmException; import java.security.SecureRandom; import java.security.Security; +import java.security.spec.AlgorithmParameterSpec; import java.util.ArrayList; import java.util.Arrays; import java.util.List; @@ -579,6 +581,25 @@ public void test_initParameters() throws Throwable { assertArrayEquals(PLAINTEXT, decrypted); } + @SuppressWarnings("ConstantConditions") + @Test + public void test_initNullKey() throws Throwable { + assumeMinimumVersion("1.6.0", AmazonCorrettoCryptoProvider.INSTANCE); + jceC.init(Cipher.ENCRYPT_MODE, key); + + final Key key = null; + AlgorithmParameters params = jceC.getParameters(); + AlgorithmParameterSpec spec = params.getParameterSpec(GCMParameterSpec.class); + SecureRandom random = TestUtil.MISC_SECURE_RANDOM.get(); + + assertThrows(InvalidKeyException.class, () -> amznC.init(Cipher.ENCRYPT_MODE, key)); + assertThrows(InvalidKeyException.class, () -> amznC.init(Cipher.ENCRYPT_MODE, key, params)); + assertThrows(InvalidKeyException.class, () -> amznC.init(Cipher.ENCRYPT_MODE, key, params, random)); + assertThrows(InvalidKeyException.class, () -> amznC.init(Cipher.ENCRYPT_MODE, key, random)); + assertThrows(InvalidKeyException.class, () -> amznC.init(Cipher.ENCRYPT_MODE, key, spec)); + assertThrows(InvalidKeyException.class, () -> amznC.init(Cipher.ENCRYPT_MODE, key, spec, random)); + } + @Test public void test_bufferOverflows() throws Throwable { final SecureRandom rnd = TestUtil.MISC_SECURE_RANDOM.get(); From f84a1cf92d64a268fbaaab240212eaa45c6b6e65 Mon Sep 17 00:00:00 2001 From: Alex Chew Date: Fri, 12 Mar 2021 17:15:45 -0800 Subject: [PATCH 13/14] Handle RsaCipher#engineDoFinal with no input bytes (#147) * Handle RsaCipher#engineDoFinal with no input bytes * Update CHANGELOG * Use zero-byte array constant Co-authored-by: SalusaSecondus Co-authored-by: SalusaSecondus --- CHANGELOG.md | 1 + .../amazon/corretto/crypto/provider/RsaCipher.java | 8 ++++++++ .../corretto/crypto/provider/test/RsaCipherTest.java | 11 +++++++++++ 3 files changed, 20 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7dca93d9..0854c309 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,7 @@ method. * Add version gating to some tests introduced in 1.5.0 [PR #128](https://github.com/corretto/amazon-corretto-crypto-provider/pull/128) * More accurate output size estimates from `Cipher.getOutputSize()` [PR #138](https://github.com/corretto/amazon-corretto-crypto-provider/pull/138) * Validate that `AesGcmSpi` receives a non-null key on init to prevent unncessarily late NPE [PR #146](https://github.com/corretto/amazon-corretto-crypto-provider/pull/146) +* Gracefully handle calling `Cipher.doFinal()` without any input bytes in `RsaCipher` [PR #147](https://github.com/corretto/amazon-corretto-crypto-provider/pull/147) ## 1.5.0 ### Breaking Change Warning diff --git a/src/com/amazon/corretto/crypto/provider/RsaCipher.java b/src/com/amazon/corretto/crypto/provider/RsaCipher.java index 774990c5..dbff75bb 100644 --- a/src/com/amazon/corretto/crypto/provider/RsaCipher.java +++ b/src/com/amazon/corretto/crypto/provider/RsaCipher.java @@ -177,6 +177,14 @@ protected int engineDoFinal(byte[] input, int inputOffset, int inputLen, final b inputOffset = 0; inputLen = buffer_.size(); } + // One-shot, no input. Cipher only calls engineDoFinal with null input in doFinal overloads + // that don't take an input buffer, and in those cases, inputOffset and inputLen are 0. + // We set them here anyways to be safe, because the API makes no such guarantee. + else if (input == null) { + input = Utils.EMPTY_ARRAY; + inputOffset = 0; + inputLen = 0; + } if (output.length - outputOffset < engineGetOutputSize(inputLen)) { throw new ShortBufferException(); diff --git a/tst/com/amazon/corretto/crypto/provider/test/RsaCipherTest.java b/tst/com/amazon/corretto/crypto/provider/test/RsaCipherTest.java index e1039ce6..97a751b7 100644 --- a/tst/com/amazon/corretto/crypto/provider/test/RsaCipherTest.java +++ b/tst/com/amazon/corretto/crypto/provider/test/RsaCipherTest.java @@ -867,6 +867,17 @@ public void threadStorm() throws Throwable { } } + @Test + public void noInputDoFinal() throws Exception { + assumeMinimumVersion("1.6.0", AmazonCorrettoCryptoProvider.INSTANCE); + final Cipher enc = Cipher.getInstance(NO_PADDING, NATIVE_PROVIDER); + enc.init(Cipher.ENCRYPT_MODE, PAIR_1024.getPublic()); + final byte[] result = enc.doFinal(); + for (final byte b : result) { + assertEquals(b, 0); + } + } + private void testNative2Jce(final String padding, final int keySize) throws GeneralSecurityException { final Cipher jceC = Cipher.getInstance(padding); final Cipher nativeC = Cipher.getInstance(padding, NATIVE_PROVIDER); From ff85424259e4a4f2548ddbb3bafcc7314a35241a Mon Sep 17 00:00:00 2001 From: Alex Chew Date: Fri, 12 Mar 2021 17:35:59 -0800 Subject: [PATCH 14/14] Update CHANGELOG for 1.6.0 (#148) --- CHANGELOG.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0854c309..5a77b796 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,6 @@ # Changelog -## 1.6.0 (Unreleased) +## 1.6.0 ### Breaking Change In accordance with our [versioning policy](https://github.com/corretto/amazon-corretto-crypto-provider/blob/master/VERSIONING.rst), this release contains a low-risk breaking change. For details please see the [1.5.0](#150) section of this document. @@ -19,7 +19,7 @@ method. ### Patches * Add version gating to some tests introduced in 1.5.0 [PR #128](https://github.com/corretto/amazon-corretto-crypto-provider/pull/128) * More accurate output size estimates from `Cipher.getOutputSize()` [PR #138](https://github.com/corretto/amazon-corretto-crypto-provider/pull/138) -* Validate that `AesGcmSpi` receives a non-null key on init to prevent unncessarily late NPE [PR #146](https://github.com/corretto/amazon-corretto-crypto-provider/pull/146) +* Validate that `AesGcmSpi` receives a non-null key on init to prevent unnecessarily late NPE [PR #146](https://github.com/corretto/amazon-corretto-crypto-provider/pull/146) * Gracefully handle calling `Cipher.doFinal()` without any input bytes in `RsaCipher` [PR #147](https://github.com/corretto/amazon-corretto-crypto-provider/pull/147) ## 1.5.0