From 07a424e2aba1b892f9806b039f9fb08979370130 Mon Sep 17 00:00:00 2001 From: SalusaSecondus Date: Thu, 18 Mar 2021 14:56:36 -0700 Subject: [PATCH 1/7] Improve documentation (#151) * Improve documentation * Language tweaks --- DEVELOPMENT-GUIDE.md | 6 +++--- DIFFERENCES.md | 4 ++-- README.md | 7 ++++++- .../amazon/corretto/crypto/provider/Loader.java | 16 ++++++++++++++++ 4 files changed, 27 insertions(+), 6 deletions(-) diff --git a/DEVELOPMENT-GUIDE.md b/DEVELOPMENT-GUIDE.md index aeedc63c..074dbf3d 100644 --- a/DEVELOPMENT-GUIDE.md +++ b/DEVELOPMENT-GUIDE.md @@ -38,11 +38,11 @@ In decreasing order of importance: 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. +7. New best practices *must* be applied uniformly to the codebase. As we extend and improve ACCP, we will create new tools and frameworks to make our code better (cleaner, safer, easier to read, etc.). + When we do this, we must go back through the rest of the ACCP codebase to make the same improvements everywhere. 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. + 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). diff --git a/DIFFERENCES.md b/DIFFERENCES.md index 04f3aa67..87fdba9f 100644 --- a/DIFFERENCES.md +++ b/DIFFERENCES.md @@ -42,9 +42,9 @@ This construction is safe for all known JCE providers and is expected to remain 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`. +ACCP might overestimate the amount of space needed when encrypted with `AES/GCM/NoPadding` on versions prior to 1.6.0. 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. diff --git a/README.md b/README.md index 59194536..116bd935 100644 --- a/README.md +++ b/README.md @@ -8,7 +8,12 @@ Currently algorithms are primarily backed by OpenSSL's implementations (1.1.1j a ## Build Status -Please be aware that "Overkill" tests are known to be flakey +Please be aware that both "Overkill" and "Dieharder" tests are known to be flakey. +Both of these tests are flakey because they include entropy generation tests +(specificaly, the [Dieharder tests](http://webhome.phy.duke.edu/~rgb/General/dieharder.php)). +Entropy tests are unavoidably flakey because there is always a possibility that a high-quality +random number generator will output data which looks non-random. +(For example, even a fair coin will come up heads ten times in a row about one in a thousand trials.) | Build Name | master branch | develop branch | | ---------- | ------------- | -------------- | diff --git a/src/com/amazon/corretto/crypto/provider/Loader.java b/src/com/amazon/corretto/crypto/provider/Loader.java index b11000a8..ecbbac88 100644 --- a/src/com/amazon/corretto/crypto/provider/Loader.java +++ b/src/com/amazon/corretto/crypto/provider/Loader.java @@ -27,6 +27,22 @@ import java.util.regex.Matcher; import java.util.regex.Pattern; +/** + * This class owns all logic necessary for loading the native library. + * For this reason it must not depend on any other classes from this project (other than Janitor). + * + * The rough logic flow is as follows: + *
    + *
  1. Read library version from embedded properties file + *
  2. Use some entropy from {@code /dev/urandom} to create a random temporary filename + *
  3. Copy the shared object from within our own JAR to the temporary file + *
  4. Load the shared object as a library from the temporary file + *
  5. Delete the temporary file + *
  6. If loading the library from above fails, try to load the library from our standard library path + *
  7. If we have successfully loaded a library, ask it for the version it was compiled with + *
  8. If the versions match, we mark that we loaded successful, else, we record the error. + *
+ */ final class Loader { private static final String PROPERTY_BASE = "com.amazon.corretto.crypto.provider."; private static final String LIBRARY_NAME = "amazonCorrettoCryptoProvider"; From 059448ebccd42eebeeafa57151c71d8937663256 Mon Sep 17 00:00:00 2001 From: Greg Rubin Date: Thu, 18 Mar 2021 22:04:13 +0000 Subject: [PATCH 2/7] Clarify best practices are local to ACCP --- DEVELOPMENT-GUIDE.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/DEVELOPMENT-GUIDE.md b/DEVELOPMENT-GUIDE.md index 074dbf3d..bf7f6fe0 100644 --- a/DEVELOPMENT-GUIDE.md +++ b/DEVELOPMENT-GUIDE.md @@ -38,7 +38,7 @@ In decreasing order of importance: 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. New best practices *must* be applied uniformly to the codebase. +7. New best practices for this project *must* be applied uniformly to the codebase. As we extend and improve ACCP, we will create new tools and frameworks to make our code better (cleaner, safer, easier to read, etc.). When we do this, we must go back through the rest of the ACCP codebase to make the same improvements everywhere. Historical examples of this include `java_buffer`, `InputBuffer`, `NativeResource`, `raii_env` and others. From 64232730e908ed3cd364fffa3103d4fccf5c14f3 Mon Sep 17 00:00:00 2001 From: Greg Rubin Date: Fri, 26 Mar 2021 16:58:28 +0000 Subject: [PATCH 3/7] Add logic to generate a classpath file --- .gitignore | 2 +- .project | 28 ++++++++++++++++++++++++++++ README.md | 7 +++++-- build.gradle | 44 ++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 78 insertions(+), 3 deletions(-) create mode 100644 .project diff --git a/.gitignore b/.gitignore index ef64a2e6..c12bd894 100644 --- a/.gitignore +++ b/.gitignore @@ -1,6 +1,5 @@ /build /.gradle -/.project /.settings/ /.vscode/ /.idea/ @@ -10,3 +9,4 @@ *.iws *.iml *.orig +/.classpath diff --git a/.project b/.project new file mode 100644 index 00000000..02fde04f --- /dev/null +++ b/.project @@ -0,0 +1,28 @@ + + + AmazonCorrettoCryptoProvider + Project amazon-corretto-crypto-provider file has been manually crafted. + + + + + org.eclipse.jdt.core.javabuilder + + + + + + org.eclipse.jdt.core.javanature + + + + 0 + + 30 + + org.eclipse.core.resources.regexFilterMatcher + node_modules|.git|__CREATED_BY_JAVA_LANGUAGE_SERVER__ + + + + diff --git a/README.md b/README.md index 116bd935..259c5dd2 100644 --- a/README.md +++ b/README.md @@ -101,6 +101,8 @@ Whether you're using Maven, Gradle, or some other build system that also pulls packages from Maven Central, it's important to specify `linux-x86_64` as the classifier. You'll get an empty package otherwise. +Regardless of how you acquire ACCP (Maven, manual build, etc.) you will still need to follow the guidance in the [Configuration section][#configuration] to enable ACCP in your application. + ### Maven Add the following to your `pom.xml` or wherever you configure your Maven dependencies. This will instruct it to use the most recent 1.x version of ACCP. @@ -167,6 +169,7 @@ Building this provider requires a 64 bit Linux build system with the following p * coverage: Run target `test` and collect both Java and C++ coverage metrics (saved in `build/reports`) * release: **Default target** depends on build, test, and coverage * overkill: Run **all** tests (no coverage) +* generateEclipseClasspath: Generates a `.classpath` file which is understandable by Eclipse and VS Code to make development easier. (This should ideally be run prior to opening ACCP in your IDE.) ## Configuration There are several ways to configure the ACCP as the highest priority provider in Java. @@ -185,13 +188,13 @@ If you want to check to verify that ACCP is properly working on your system, you 1. Verify that the highest priority provider actually is ACCP: ```java if (Cipher.getInstance("AES/GCM/NoPadding").getProvider().getName().equals(AmazonCorrettoCryptoProvider.PROVIDER_NAME)) { - // Successfully installed + // Successfully installed } ``` 2. Ask ACCP about its health ```java if (AmazonCorrettoCryptoProvider.INSTANCE.getLoadingError() == null && AmazonCorrettoCryptoProvider.INSTANCE.runSelfTests().equals(SelfTestStatus.PASSED)) { - // Successfully installed + // Successfully installed } ``` 3. Assert that ACCP is healthy and throw a `RuntimeCryptoException` if it isn't. diff --git a/build.gradle b/build.gradle index a82d1591..1d3f18fe 100644 --- a/build.gradle +++ b/build.gradle @@ -1,6 +1,8 @@ // Copyright Amazon.com Inc. or its affiliates. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 +import groovy.xml.* + plugins { id 'maven-publish' id 'signing' @@ -454,3 +456,45 @@ task clean(overwrite: true, type: Delete) { task deep_clean(type: Delete) { delete buildDir } + +task generateEclipseClasspath { + doLast { + file(".classpath").withWriter { writer -> + // Create MarkupBuilder with 4 space indent + def xml = new MarkupBuilder(new IndentPrinter(writer, " ", true)) + + xml.doubleQuotes = true + xml.mkp.xmlDeclaration(version: '1.0', encoding: 'utf-8') + xml.classpath { + classpathentry('kind': 'con', 'path': 'org.eclipse.jdt.launching.JRE_CONTAINER') { + attributes { + attribute('name': 'module', 'value': 'true') + } + } + classpathentry('kind': 'src', 'output': 'build/eclipse/src', 'path': 'src') + classpathentry('kind': 'src', 'output': 'build/eclipse/template-src', 'path': 'template-src') + classpathentry('kind': 'src', 'output': 'build/eclipse/tst', 'path': 'tst') { + attributes { + attribute('name': 'test', 'value': 'true') + } + } + classpathentry('kind': 'src', 'output': 'build/eclipse/src', 'path': 'build/cmake/generated-java') { + attributes { + attribute('name': 'optional', 'value': 'true') + } + } + classpathentry('kind': 'src', 'output': 'build/eclipse/src', 'path': 'build/coverage-cmake/generated-java') { + attributes { + attribute('name': 'optional', 'value': 'true') + } + } + + configurations.testDep.files.each{f -> + classpathentry('kind': 'lib', 'path': f) { + attribute('name': 'test', 'value': 'true') + } + } + } // xml.classpath + } // file.withWriter + } // doLast +} // generateEclipseClasspath From 93da63c215d28e8e682f66c9b15cf52bfc630b9a Mon Sep 17 00:00:00 2001 From: Greg Rubin Date: Thu, 8 Apr 2021 23:32:38 +0000 Subject: [PATCH 4/7] Remove dead code and bump patch version --- build.gradle | 2 +- .../com/amazon/corretto/crypto/provider/TemplateHashSpi.java | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/build.gradle b/build.gradle index 1d3f18fe..f24385de 100644 --- a/build.gradle +++ b/build.gradle @@ -9,7 +9,7 @@ plugins { } group = 'software.amazon.cryptools' -version = '1.6.0' +version = '1.6.1' def openssl_version = '1.1.1j' def opensslSrcPath = "${buildDir}/openssl/openssl-${openssl_version}" diff --git a/template-src/com/amazon/corretto/crypto/provider/TemplateHashSpi.java b/template-src/com/amazon/corretto/crypto/provider/TemplateHashSpi.java index a2a339ab..c8f6c02a 100644 --- a/template-src/com/amazon/corretto/crypto/provider/TemplateHashSpi.java +++ b/template-src/com/amazon/corretto/crypto/provider/TemplateHashSpi.java @@ -29,7 +29,6 @@ public final class TemplateHashSpi extends MessageDigestSpi implements Cloneable private static final byte[] INITIAL_CONTEXT; private byte[] myContext; - private byte[] oneByteArray = null; private InputBuffer buffer; static { From fd967b00839fad0651d4cae77625cb5ff85d9896 Mon Sep 17 00:00:00 2001 From: Greg Rubin Date: Fri, 16 Apr 2021 10:14:12 -0700 Subject: [PATCH 5/7] Improve the README (#155) --- README.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 259c5dd2..fcb399f1 100644 --- a/README.md +++ b/README.md @@ -101,7 +101,7 @@ Whether you're using Maven, Gradle, or some other build system that also pulls packages from Maven Central, it's important to specify `linux-x86_64` as the classifier. You'll get an empty package otherwise. -Regardless of how you acquire ACCP (Maven, manual build, etc.) you will still need to follow the guidance in the [Configuration section][#configuration] to enable ACCP in your application. +Regardless of how you acquire ACCP (Maven, manual build, etc.) you will still need to follow the guidance in the [Configuration section](#configuration) to enable ACCP in your application. ### Maven Add the following to your `pom.xml` or wherever you configure your Maven dependencies. @@ -170,6 +170,8 @@ Building this provider requires a 64 bit Linux build system with the following p * release: **Default target** depends on build, test, and coverage * overkill: Run **all** tests (no coverage) * generateEclipseClasspath: Generates a `.classpath` file which is understandable by Eclipse and VS Code to make development easier. (This should ideally be run prior to opening ACCP in your IDE.) +* single_test: Runs a single unit test. The test is selected with the Java system property `SINGLE_TEST`. For example: `./gradlew single_test -DSINGLE_TEST=com.amazon.corretto.crypto.provider.test.EcGenTest` + (You may need to do a clean build when switching between selected tests.) ## Configuration There are several ways to configure the ACCP as the highest priority provider in Java. From 1503e7050fbe9dfee42977fdd68f9135f605cb4b Mon Sep 17 00:00:00 2001 From: Ben Farley <47006790+farleyb-amazon@users.noreply.github.com> Date: Wed, 21 Apr 2021 11:35:10 -0600 Subject: [PATCH 6/7] Fix issue with hashing (#157) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A race condition can cause ACCP’s MessageDigest hashing algorithms to return the same value for different inputs. This patch fixes the issue, and adds new unit tests for both the hash and hmac code to prevent regression. --- .../corretto/crypto/provider/InputBuffer.java | 24 ++--- .../crypto/provider/TemplateHashSpi.java | 47 +++++----- .../provider/test/HashFunctionTester.java | 73 +++++++++++++++ .../crypto/provider/test/HmacTest.java | 91 +++++++++++++++++++ .../crypto/provider/test/InputBufferTest.java | 12 +-- .../crypto/provider/test/TestUtil.java | 11 ++- 6 files changed, 212 insertions(+), 46 deletions(-) diff --git a/src/com/amazon/corretto/crypto/provider/InputBuffer.java b/src/com/amazon/corretto/crypto/provider/InputBuffer.java index 84bf8d9e..38156af1 100644 --- a/src/com/amazon/corretto/crypto/provider/InputBuffer.java +++ b/src/com/amazon/corretto/crypto/provider/InputBuffer.java @@ -138,12 +138,12 @@ public static interface ByteBufferBiConsumer extends BiConsumer extends Supplier { + public static interface StateSupplier extends Function { //@ also //@ public normal_behavior //@ ensures \result != null ==> \fresh(\result); //@ pure - public /*@ nullable @*/ S get(); + public /*@ nullable @*/ S apply(S state); } //@ private invariant 0 <= buffSize; @@ -163,9 +163,7 @@ public static interface StateSupplier extends Supplier { //@ spec_public private /*@ nullable @*/ FinalHandlerFunction finalHandler; //@ spec_public - private /*@ { Consumer.Local } @*/ Consumer stateResetter = (ignored) -> { }; // NOP - //@ spec_public - private StateSupplier stateSupplier = () -> state; + private StateSupplier stateSupplier = (oldState) -> oldState; //@ spec_public private Optional> stateCloner = Optional.empty(); // If absent, delegates to arrayUpdater @@ -229,7 +227,6 @@ public static interface StateSupplier extends Supplier { public void reset() { buff.reset(); firstData = true; - state = null; /*@ set bytesReceived = 0; @ set bytesProcessed = 0; @ set bufferState = ((bufferState == BufferState.Uninitialized) @@ -311,15 +308,6 @@ public InputBuffer withStateCloner(final /*@ nullable @*/ Function c return this; } - //@ normal_behavior - //@ requires true; - //@ assignable stateResetter; - //@ ensures \result == this && stateResetter == resetter; - public InputBuffer withStateResetter(final /*@ { Consumer.Local } @*/ Consumer resetter) { - stateResetter = resetter; - return this; - } - /*@ normal_behavior @ requires canSetHandler(bufferState); @ assignable stateSupplier; @@ -469,7 +457,7 @@ private void processBuffer(boolean forceInit) { buff.reset(); //@ set bytesProcessed = bytesProcessed + oldSize; } else { - state = stateSupplier.get(); + state = stateSupplier.apply(state); } //@ set bufferState = BufferState.HandlerCalled; firstData = false; @@ -522,7 +510,7 @@ public void update(final ByteBuffer src) { if (initialBufferUpdater.isPresent()) { state = initialBufferUpdater.get().apply(src.slice()); } else { - state = stateSupplier.get(); + state = stateSupplier.apply(state); bufferUpdater.get().accept(state, src.slice()); } } else { @@ -569,7 +557,7 @@ public void update(final byte[] src, final int offset, final int length) { if (initialArrayUpdater.isPresent()) { state = initialArrayUpdater.get().apply(src, offset, length); } else { - state = stateSupplier.get(); + state = stateSupplier.apply(state); arrayUpdater.accept(state, src, offset, length); } } else { diff --git a/template-src/com/amazon/corretto/crypto/provider/TemplateHashSpi.java b/template-src/com/amazon/corretto/crypto/provider/TemplateHashSpi.java index c8f6c02a..93f80462 100644 --- a/template-src/com/amazon/corretto/crypto/provider/TemplateHashSpi.java +++ b/template-src/com/amazon/corretto/crypto/provider/TemplateHashSpi.java @@ -28,7 +28,6 @@ public final class TemplateHashSpi extends MessageDigestSpi implements Cloneable private static final int HASH_SIZE; private static final byte[] INITIAL_CONTEXT; - private byte[] myContext; private InputBuffer buffer; static { @@ -108,33 +107,40 @@ 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; + private static byte[] resetContext(byte[] context) { + if (context == null) { + context = INITIAL_CONTEXT.clone(); + } else { + System.arraycopy(INITIAL_CONTEXT, 0, context, 0, INITIAL_CONTEXT.length); + } + return context; + } + + private static byte[] doFinal(byte[] context) { + final byte[] result = new byte[HASH_SIZE]; + synchronizedFinish(context, result, 0); + return result; + } + + private static byte[] singlePass(byte[] src, int offset, int length) { + if (offset != 0 || length != src.length) { + src = Arrays.copyOf(src, length); + offset = 0; + } + final byte[] result = new byte[HASH_SIZE]; + fastDigest(result, src, src.length); + return result; } public TemplateHashSpi() { Loader.checkNativeLibraryAvailability(); - myContext = INITIAL_CONTEXT.clone(); this.buffer = new InputBuffer(1024) - .withInitialStateSupplier(this::resetContext) + .withInitialStateSupplier(TemplateHashSpi::resetContext) .withUpdater(TemplateHashSpi::synchronizedUpdateContextByteArray) .withUpdater(TemplateHashSpi::synchronizedUpdateNativeByteBuffer) - .withDoFinal((context) -> { - final byte[] result = new byte[HASH_SIZE]; - synchronizedFinish(context, result, 0); - return result; - }) - .withSinglePass((src, offset, length) -> { - if (offset != 0 || length != src.length) { - src = Arrays.copyOf(src, length); - offset = 0; - } - final byte[] result = new byte[HASH_SIZE]; - fastDigest(result, src, src.length); - return result; - }) + .withDoFinal(TemplateHashSpi::doFinal) + .withSinglePass(TemplateHashSpi::singlePass) .withStateCloner((context) -> context.clone()); } @@ -164,7 +170,6 @@ public Object clone() { try { TemplateHashSpi clonedObject = (TemplateHashSpi)super.clone(); - clonedObject.myContext = myContext.clone(); clonedObject.buffer = (InputBuffer) buffer.clone(); return clonedObject; diff --git a/tst/com/amazon/corretto/crypto/provider/test/HashFunctionTester.java b/tst/com/amazon/corretto/crypto/provider/test/HashFunctionTester.java index 58374f37..342b64d8 100644 --- a/tst/com/amazon/corretto/crypto/provider/test/HashFunctionTester.java +++ b/tst/com/amazon/corretto/crypto/provider/test/HashFunctionTester.java @@ -3,6 +3,7 @@ package com.amazon.corretto.crypto.provider.test; +import static com.amazon.corretto.crypto.provider.test.TestUtil.assertArraysHexEquals;; import static com.amazon.corretto.crypto.provider.test.TestUtil.assertThrows; import static com.amazon.corretto.crypto.provider.test.TestUtil.sneakyInvoke; import static org.junit.jupiter.api.Assertions.assertArrayEquals; @@ -222,6 +223,8 @@ public void testAPI() throws Exception { testBoundsChecks(); testByteBufferReflectionFallback(); testClone(); + testCloneLarge(); + testDraggedState(); testDirectBufferSlices(); testLargeArray(); testLargeDirectBuffer(); @@ -266,6 +269,76 @@ private void testDirectBufferSlices() { assertArrayEquals(expected.digest(), md.digest()); } + private void testDraggedState() throws CloneNotSupportedException { + final byte[] base = new byte[4096]; + final byte[] suffix1 = new byte[4096]; + final byte[] suffix2 = new byte[4096]; + for (int x = 0; x < base.length; x++) { + base[x] = (byte) x; + suffix1[x] = (byte) (x + 1); + suffix2[x] = (byte) (x + 2); + } + MessageDigest defaultInstance = getDefaultInstance(); + defaultInstance.update(base); + final byte[] expected1 = defaultInstance.digest(suffix1); + + defaultInstance.update(base); + final byte[] expected2 = defaultInstance.digest(suffix2); + + final MessageDigest original = getAmazonInstance(); + final MessageDigest duplicate = (MessageDigest) original.clone(); + + // First use uses the explicitly cloned state + original.update(base); + duplicate.update(base); + + assertArraysHexEquals(expected1, original.digest(suffix1)); + assertArraysHexEquals(expected2, duplicate.digest(suffix2)); + + // State has been reset and thus we might no longer be on the explicitly cloned state + original.update(base); + duplicate.update(base); + + assertArraysHexEquals(expected1, original.digest(suffix1)); + assertArraysHexEquals(expected2, duplicate.digest(suffix2)); + } + + private void testCloneLarge() throws CloneNotSupportedException { + MessageDigest md = getAmazonInstance(); + + final byte[] base = new byte[4096]; + final byte[] suffix1 = new byte[4096]; + final byte[] suffix2 = new byte[4096]; + for (int x = 0; x < base.length; x++) { + base[x] = (byte) x; + suffix1[x] = (byte) (x + 1); + suffix2[x] = (byte) (x + 2); + } + + md.update(base); + + MessageDigest md2 = (MessageDigest) md.clone(); + + md2.update(suffix1); + md.update(suffix2); + + MessageDigest defaultInstance = getDefaultInstance(); + defaultInstance.update(base); + final byte[] expected1 = defaultInstance.digest(suffix1); + + defaultInstance.update(base); + final byte[] expected2 = defaultInstance.digest(suffix2); + + assertArraysHexEquals( + expected1, + md2.digest() + ); + assertArraysHexEquals( + expected2, + md.digest() + ); + } + private void testClone() throws CloneNotSupportedException { MessageDigest md = getAmazonInstance(); diff --git a/tst/com/amazon/corretto/crypto/provider/test/HmacTest.java b/tst/com/amazon/corretto/crypto/provider/test/HmacTest.java index 8cbaa577..cb7de3f0 100644 --- a/tst/com/amazon/corretto/crypto/provider/test/HmacTest.java +++ b/tst/com/amazon/corretto/crypto/provider/test/HmacTest.java @@ -3,6 +3,7 @@ package com.amazon.corretto.crypto.provider.test; +import static com.amazon.corretto.crypto.provider.test.TestUtil.assertArraysHexEquals;; import static com.amazon.corretto.crypto.provider.test.TestUtil.NATIVE_PROVIDER; import static com.amazon.corretto.crypto.provider.test.TestUtil.assertThrows; import static com.amazon.corretto.crypto.provider.test.TestUtil.sneakyInvoke; @@ -427,6 +428,96 @@ public void supportsCloneable() throws Exception { } } + @Test + public void supportsCloneableLarge() throws Exception { + TestUtil.assumeMinimumVersion("1.3.0", NATIVE_PROVIDER); + final byte[] prefix = new byte[4096]; + final byte[] suffix1 = new byte[4096]; + final byte[] suffix2 = new byte[4096]; + + for (int x = 0; x < prefix.length; x++) { + prefix[x] = (byte) x; + suffix1[x] = (byte) (x + 1); + suffix2[x] = (byte) (x + 2); + } + + final SecretKeySpec key = new SecretKeySpec(new byte[4096], "Generic"); + for (final String algorithm : SUPPORTED_HMACS) { + final Mac defaultInstance = Mac.getInstance(algorithm, "SunJCE"); + defaultInstance.init(key); + defaultInstance.update(prefix); + + final byte[] expected1 = defaultInstance.doFinal(suffix1); + + defaultInstance.update(prefix); + final byte[] expected2 = defaultInstance.doFinal(suffix2); + + + final Mac original = Mac.getInstance(algorithm, NATIVE_PROVIDER); + original.init(key); + original.update(prefix); + + final Mac duplicate = (Mac) original.clone(); + + original.update(suffix1); + duplicate.update(suffix2); + + assertArraysHexEquals( + expected1, + original.doFinal() + ); + assertArraysHexEquals( + expected2, + duplicate.doFinal() + ); + } + } + + + @Test + public void testDraggedState() throws Exception { + TestUtil.assumeMinimumVersion("1.3.0", NATIVE_PROVIDER); + final byte[] prefix = new byte[4096]; + final byte[] suffix1 = new byte[4096]; + final byte[] suffix2 = new byte[4096]; + + for (int x = 0; x < prefix.length; x++) { + prefix[x] = (byte) x; + suffix1[x] = (byte) (x + 1); + suffix2[x] = (byte) (x + 2); + } + + final SecretKeySpec key = new SecretKeySpec(new byte[4096], "Generic"); + for (final String algorithm : SUPPORTED_HMACS) { + final Mac defaultInstance = Mac.getInstance(algorithm, "SunJCE"); + defaultInstance.init(key); + defaultInstance.update(prefix); + final byte[] expected1 = defaultInstance.doFinal(suffix1); + + defaultInstance.update(prefix); + final byte[] expected2 = defaultInstance.doFinal(suffix2); + + final Mac original = Mac.getInstance(algorithm, NATIVE_PROVIDER); + final Mac duplicate = (Mac) original.clone(); + original.init(key); + duplicate.init(key); + + // First use uses the explicitly cloned state + original.update(prefix); + duplicate.update(prefix); + + assertArraysHexEquals(expected1, original.doFinal(suffix1)); + assertArraysHexEquals(expected2, duplicate.doFinal(suffix2)); + + // State has been reset and thus we might no longer be on the explicitly cloned state + original.update(prefix); + duplicate.update(prefix); + + assertArraysHexEquals(expected1, original.doFinal(suffix1)); + assertArraysHexEquals(expected2, duplicate.doFinal(suffix2)); + } + } + @Test public void selfTest() { assertEquals(SelfTestStatus.PASSED, HmacSHA512Spi.runSelfTest().getStatus()); diff --git a/tst/com/amazon/corretto/crypto/provider/test/InputBufferTest.java b/tst/com/amazon/corretto/crypto/provider/test/InputBufferTest.java index 7b8a2818..a6bf3aad 100644 --- a/tst/com/amazon/corretto/crypto/provider/test/InputBufferTest.java +++ b/tst/com/amazon/corretto/crypto/provider/test/InputBufferTest.java @@ -54,7 +54,7 @@ public void minimalCase() { final ByteBuffer result = ByteBuffer.allocate(17); final InputBuffer buffer = getBuffer(4); - buffer.withInitialStateSupplier(() -> { return result; }) + buffer.withInitialStateSupplier((s) -> { return result; }) .withUpdater((ctx, src, offset, length) -> ctx.put(src, offset, length)) .withDoFinal(ByteBuffer::array); @@ -93,7 +93,7 @@ public void singleByteUpdates() { // In all cases, the byte being processed should be exactly one byte and one byte behind. final InputBuffer buffer = getBuffer(1); - buffer.withInitialStateSupplier(() -> { return result; }) + buffer.withInitialStateSupplier((s) -> { return result; }) .withUpdater((ctx, src, offset, length) -> ctx.put(src, offset, length)) .withDoFinal(ByteBuffer::array); @@ -150,7 +150,7 @@ public void prefersBufferHandlers() { // By leaving other handlers null, I'll force an exception if they are used final InputBuffer buffer = getBuffer(1); - buffer.withInitialStateSupplier(() -> { return result;} ) + buffer.withInitialStateSupplier((s) -> { return result;} ) .withUpdater((ctx, src) -> ctx.put(src)) .withDoFinal(ByteBuffer::array); @@ -174,7 +174,7 @@ public void prefersBufferHandlers() { public void cloneDuplicatesBufferAndState() throws Throwable { byte[] expected = new byte[]{0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15}; final InputBuffer buffer1 = getBuffer(16); - buffer1.withInitialStateSupplier(ByteArrayOutputStream::new) + buffer1.withInitialStateSupplier((s) -> new ByteArrayOutputStream()) .withUpdater((state, src, offset, length) -> { state.write(src, offset, length); }) .withDoFinal(ByteArrayOutputStream::toByteArray) .withStateCloner((state) -> { @@ -216,7 +216,7 @@ public void cloneDuplicatesBufferAndState() throws Throwable { @Test public void cantCloneUncloneable() throws Throwable { final InputBuffer buffer = getBuffer(8); - buffer.withInitialStateSupplier(() -> { return new byte[128]; } ) + buffer.withInitialStateSupplier((s) -> { return new byte[128]; } ) .withUpdater((state, src, offset, length) -> { System.arraycopy(src, offset, state, 0, length); }) .withDoFinal((state) -> state.clone()); @@ -228,7 +228,7 @@ public void cantCloneUncloneable() throws Throwable { @Test public void nullStateProperlyHandled() throws Throwable { InputBuffer buffer = getBuffer(4); - buffer.withInitialStateSupplier(() -> { + buffer.withInitialStateSupplier((s) -> { return new byte[4]; }).withUpdater((state, src, offset, length) -> { System.arraycopy(src, offset, state, 0, length); diff --git a/tst/com/amazon/corretto/crypto/provider/test/TestUtil.java b/tst/com/amazon/corretto/crypto/provider/test/TestUtil.java index f30cfc39..a66d8d72 100644 --- a/tst/com/amazon/corretto/crypto/provider/test/TestUtil.java +++ b/tst/com/amazon/corretto/crypto/provider/test/TestUtil.java @@ -4,9 +4,12 @@ package com.amazon.corretto.crypto.provider.test; import com.amazon.corretto.crypto.provider.AmazonCorrettoCryptoProvider; + +import org.apache.commons.codec.binary.Hex; import org.bouncycastle.jce.provider.BouncyCastleProvider; import org.junit.jupiter.api.Assumptions; +import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.fail; import java.io.File; @@ -58,7 +61,13 @@ public static byte[] getRandomBytes(int length) { MISC_SECURE_RANDOM.get().nextBytes(result); return result; } - + + public static void assertArraysHexEquals(byte[] expected, byte[] actual) { + final String expectedHex = Hex.encodeHexString(expected); + final String actualHex = Hex.encodeHexString(actual); + assertEquals(expectedHex, actualHex); + } + public static void assertThrows(Class expected, ThrowingRunnable callable) { try { callable.run(); From 5e17e9d1f7832a7297d2cf3a6472512639bace49 Mon Sep 17 00:00:00 2001 From: Ben Farley <47006790+farleyb-amazon@users.noreply.github.com> Date: Wed, 21 Apr 2021 11:55:26 -0600 Subject: [PATCH 7/7] Update CHANGELOG for 1.6.1 (#158) --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5a77b796..1334dc0f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ # Changelog +## 1.6.1 +### Patches +* Fix an issue where a race condition can cause ACCP's MessageDigest hashing algorithms to return the same value for different inputs [PR #157](https://github.com/corretto/amazon-corretto-crypto-provider/pull/157) + ## 1.6.0 ### Breaking Change In accordance with our [versioning policy](https://github.com/corretto/amazon-corretto-crypto-provider/blob/master/VERSIONING.rst),