Skip to content

Commit

Permalink
Fix issue with hashing (#157)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@farleyb-amazon
farleyb-amazon authored Apr 21, 2021
1 parent fd967b0 commit 1503e70
Show file tree
Hide file tree
Showing 6 changed files with 212 additions and 46 deletions.
24 changes: 6 additions & 18 deletions src/com/amazon/corretto/crypto/provider/InputBuffer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down
47 changes: 26 additions & 21 deletions template-src/com/amazon/corretto/crypto/provider/TemplateHashSpi.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<byte[], byte[]> buffer;

static {
Expand Down Expand Up @@ -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<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());
}

Expand Down Expand Up @@ -164,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;
Expand Down
73 changes: 73 additions & 0 deletions tst/com/amazon/corretto/crypto/provider/test/HashFunctionTester.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -222,6 +223,8 @@ public void testAPI() throws Exception {
testBoundsChecks();
testByteBufferReflectionFallback();
testClone();
testCloneLarge();
testDraggedState();
testDirectBufferSlices();
testLargeArray();
testLargeDirectBuffer();
Expand Down Expand Up @@ -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();

Expand Down
91 changes: 91 additions & 0 deletions tst/com/amazon/corretto/crypto/provider/test/HmacTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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());
Expand Down
Loading

0 comments on commit 1503e70

Please sign in to comment.