Merge pull request #159 from corretto/develop

Merge 1.6.1 changes to master

.gitignore

@@ -1,6 +1,5 @@
 /build
 /.gradle
-/.project
 /.settings/
 /.vscode/
 /.idea/
@@ -10,3 +9,4 @@
 *.iws
 *.iml
 *.orig
+/.classpath

.project

@@ -0,0 +1,28 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<projectDescription>
+    <name>AmazonCorrettoCryptoProvider</name>
+    <comment>Project amazon-corretto-crypto-provider file has been manually crafted.</comment>
+    <projects>
+    </projects>
+    <buildSpec>
+        <buildCommand>
+            <name>org.eclipse.jdt.core.javabuilder</name>
+            <arguments>
+            </arguments>
+        </buildCommand>
+    </buildSpec>
+    <natures>
+        <nature>org.eclipse.jdt.core.javanature</nature>
+    </natures>
+    <filteredResources>
+        <filter>
+            <id>0</id>
+            <name></name>
+            <type>30</type>
+            <matcher>
+                <id>org.eclipse.core.resources.regexFilterMatcher</id>
+                <arguments>node_modules|.git|__CREATED_BY_JAVA_LANGUAGE_SERVER__</arguments>
+            </matcher>
+        </filter>
+    </filteredResources>
+</projectDescription>

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),

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 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.
-    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).
 
 

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.

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 |
 | ---------- | ------------- | -------------- |
@@ -96,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.
@@ -162,6 +169,9 @@ 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.)
+* 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.
@@ -180,13 +190,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.

build.gradle

@@ -1,13 +1,15 @@
 // 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'
 }
 
 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}"
@@ -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

src/com/amazon/corretto/crypto/provider/InputBuffer.java

@@ -138,12 +138,12 @@ public static interface ByteBufferBiConsumer<S> extends BiConsumer<S, ByteBuffer
     // provided for specification purposes
     //@ non_null_by_default
     @FunctionalInterface
-    public static interface StateSupplier<S> extends Supplier<S> {
+    public static interface StateSupplier<S> extends Function<S, S> {
         //@ 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<S> extends Supplier<S> {
     //@ spec_public
     private /*@ nullable @*/ FinalHandlerFunction<S, T> finalHandler;
     //@ spec_public
-    private /*@ { Consumer.Local<S> } @*/ Consumer<S> stateResetter = (ignored) -> { }; // NOP
-    //@ spec_public
-    private StateSupplier<S> stateSupplier = () -> state;
+    private StateSupplier<S> stateSupplier = (oldState) -> oldState;
     //@ spec_public
     private Optional<Function<S, S>> stateCloner = Optional.empty();
     // If absent, delegates to arrayUpdater
@@ -229,7 +227,6 @@ public static interface StateSupplier<S> extends Supplier<S> {
     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<T, S> withStateCloner(final /*@ nullable @*/ Function<S, S> c
         return this;
     }
 
-    //@ normal_behavior
-    //@     requires true;
-    //@     assignable stateResetter;
-    //@     ensures \result == this && stateResetter == resetter;
-    public InputBuffer<T, S> withStateResetter(final /*@ { Consumer.Local<S> } @*/ Consumer<S> 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 {

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 <em>all</em> 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:
+ * <ol>
+ * <li>Read library version from embedded properties file
+ * <li>Use some entropy from {@code /dev/urandom} to create a random temporary filename
+ * <li>Copy the shared object from within our own JAR to the temporary file
+ * <li>Load the shared object as a library from the temporary file
+ * <li>Delete the temporary file
+ * <li>If loading the library from above fails, try to load the library from our standard library path
+ * <li>If we have successfully loaded a library, ask it for the version it was compiled with
+ * <li>If the versions match, we mark that we loaded successful, else, we record the error.
+ * </ol>
+ */
 final class Loader {
     private static final String PROPERTY_BASE = "com.amazon.corretto.crypto.provider.";
     private static final String LIBRARY_NAME = "amazonCorrettoCryptoProvider";

template-src/com/amazon/corretto/crypto/provider/TemplateHashSpi.java

@@ -28,8 +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 byte[] oneByteArray = null;
     private InputBuffer<byte[], byte[]> buffer;
 
     static {
@@ -109,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<byte[], byte[]>(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());
     }
 
@@ -165,7 +170,6 @@ public Object clone() {
         try {
             TemplateHashSpi clonedObject = (TemplateHashSpi)super.clone();
 
-            clonedObject.myContext = myContext.clone();
             clonedObject.buffer = (InputBuffer<byte[], byte[]>) buffer.clone();
 
             return clonedObject;