Skip to content

Commit

Permalink
Merge pull request #123 from SalusaSecondus/doFinal3
Browse files Browse the repository at this point in the history 
Do not modify output when Cipher.doFinal fails
  • Loading branch information
@SalusaSecondus
SalusaSecondus authored Sep 1, 2020
2 parents 35d2ae1 + c46d5db commit 07b6e86
Show file tree
Hide file tree
Showing 5 changed files with 189 additions and 121 deletions.
3 changes: 3 additions & 0 deletions .gitignore
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@
/.project
/.settings/
/.vscode/
/.idea/
*~
\#*#
*.ipr
*.iws
*.iml
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ For other sizes, there are no documented guarantees of the SunEC behavior.
You may need to do a clean build when changing tests.

### Patches
* Ensure unauthenticated plaintext is not released through either [Cipher.doFinal(byte[], int, int, byte[], int)](https://docs.oracle.com/javase/9/docs/api/javax/crypto/Cipher.html#doFinal-byte:A-int-int-byte:A-int-) or [Cipher.doFinal(ByteBuffer, ByteBuffer)](https://docs.oracle.com/javase/9/docs/api/javax/crypto/Cipher.html#doFinal-java.nio.ByteBuffer-java.nio.ByteBuffer-). [PR #123](https://github.com/corretto/amazon-corretto-crypto-provider/pull/123)
* Better handle HMAC keys with a `null` format. [PR #124](https://github.com/corretto/amazon-corretto-crypto-provider/pull/124)
* Throw `IllegalBlockSizeException` when attempting RSA encryption/decryption on data larger than the keysize. [PR #122](https://github.com/corretto/amazon-corretto-crypto-provider/pull/122)

Expand Down
240 changes: 119 additions & 121 deletions src/com/amazon/corretto/crypto/provider/AesGcmSpi.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -516,70 +516,90 @@ protected byte[] engineDoFinal(byte[] bytes, int offset, int length) throws Ille
protected synchronized int engineDoFinal(byte[] bytes, int offset, int length, byte[] output, int outputOffset)
throws ShortBufferException, IllegalBlockSizeException, BadPaddingException
{
if (bytes == null) bytes = EMPTY_ARRAY;
try {
if (bytes == null) bytes = EMPTY_ARRAY;

boolean overlaps = Utils.arraysOverlap(
bytes, offset, output, outputOffset, Math.max(length , engineGetOutputSize(length))
);
boolean overlaps = Utils.arraysOverlap(
bytes, offset, output, outputOffset, Math.max(length , engineGetOutputSize(length))
);

if (opMode != NATIVE_MODE_ENCRYPT && opMode != NATIVE_MODE_DECRYPT) {
throw new IllegalStateException("Cipher not initialized");
}

if (outputOffset < 0) {
throw new ArrayIndexOutOfBoundsException("Negative output offset");
}
if (opMode != NATIVE_MODE_ENCRYPT && opMode != NATIVE_MODE_DECRYPT) {
throw new IllegalStateException("Cipher not initialized");
}

checkOutputBuffer(length, output, outputOffset);
checkArrayLimits(bytes, offset, length);
if (outputOffset < 0) {
throw new ArrayIndexOutOfBoundsException("Negative output offset");
}

int resultLength = 0;
checkOutputBuffer(length, output, outputOffset);
checkArrayLimits(bytes, offset, length);

if (overlaps) {
// The input and output potentially overlap. We'll need to make sure we copy the input somewhere safe before
// proceeding too much further.
int resultLength = 0;

// Since we need to take care of this on engineUpdate as well, we can just delegate to engineUpdate, which
// will make sure to copy the buffer - on encrypt this is an explicit check, while on decrypt engineUpdate
// unconditionally copies to a temporary buffer.
if (overlaps) {
// The input and output potentially overlap. We'll need to make sure we copy the input somewhere safe before
// proceeding too much further.

resultLength = engineUpdate(bytes, offset, length, output, outputOffset);
outputOffset += resultLength;
// Since we need to take care of this on engineUpdate as well, we can just delegate to engineUpdate, which
// will make sure to copy the buffer - on encrypt this is an explicit check, while on decrypt engineUpdate
// unconditionally copies to a temporary buffer.

// We processed all of the input in engineUpdate. So there's no longer an overlap to deal with.
length = 0;
}
resultLength = engineUpdate(bytes, offset, length, output, outputOffset);
outputOffset += resultLength;

if (opMode == NATIVE_MODE_DECRYPT) {
// If we have some data already, merge them into our temporary buffer.
if (decryptInputBuf.size() != 0) {
engineUpdate(bytes, offset, length);
bytes = decryptInputBuf.getDataBuffer();
offset = 0;
length = decryptInputBuf.size();
// We processed all of the input in engineUpdate. So there's no longer an overlap to deal with.
length = 0;
}

if (length < tagLength) {
throw new AEADBadTagException("Input too short - need tag");
}
if (opMode == NATIVE_MODE_DECRYPT) {
// We use this temporary buffer both for our input ciphertext and our output (possibly unauthenticated) plaintext.
// We'll copy it to the actual output location iff decryption completes successfully.
engineUpdate(bytes, offset, length); // Decrypt mode never generates output for updates
final byte[] tempBuffer = decryptInputBuf.getDataBuffer();
final int tempBufferLength = decryptInputBuf.size();

final byte[] finalBytes = bytes;
final int finalOffset = offset;
final int finalOutputOffset = outputOffset;
final int finalLength = length;
keyUsageCount++;
if (context != null) {
// We already have a context, so let's reuse it.
return context.use(ptr -> {
return oneShotDecrypt(
ptr,
null,
finalBytes,
finalOffset,
finalLength,
if (tempBufferLength < tagLength) {
throw new AEADBadTagException("Input too short - need tag");
}

output,
finalOutputOffset,
keyUsageCount++;
final int outLen;
if (context != null) {
// We already have a context, so let's reuse it.
outLen = context.use(ptr -> {
return oneShotDecrypt(
ptr,
null,
tempBuffer,
0,
tempBufferLength,

tempBuffer,
0,

tagLength,
key,
iv,

// The cost of calling decryptAADBuf.getDataBuffer() when its buffer is empty is significant for 16-byte
// decrypt operations (approximately a 7% performance hit). To avoid this, we reuse the same empty array
// instead in this common-case path.
decryptAADBuf.size() != 0 ? decryptAADBuf.getDataBuffer() : EMPTY_ARRAY,
decryptAADBuf.size()
);
});
} else {
// We don't have an existing context, however we might want to save one
final long[] ptrOut = keyUsageCount > KEY_REUSE_THRESHOLD ? new long[1] : null;
outLen = oneShotDecrypt(
0,
ptrOut,
tempBuffer,
0,
tempBufferLength,

tempBuffer,
0,

tagLength,
key,
Expand All @@ -591,56 +611,51 @@ protected synchronized int engineDoFinal(byte[] bytes, int offset, int length, b
decryptAADBuf.size() != 0 ? decryptAADBuf.getDataBuffer() : EMPTY_ARRAY,
decryptAADBuf.size()
);
});
} else {
// We don't have an existing context, however we might want to save one
final long[] ptrOut = keyUsageCount > KEY_REUSE_THRESHOLD ? new long[1] : null;
final int outLen = oneShotDecrypt(
0,
ptrOut,
bytes,
offset,
length,

output,
outputOffset,

tagLength,
key,
iv,

// The cost of calling decryptAADBuf.getDataBuffer() when its buffer is empty is significant for 16-byte
// decrypt operations (approximately a 7% performance hit). To avoid this, we reuse the same empty array
// instead in this common-case path.
decryptAADBuf.size() != 0 ? decryptAADBuf.getDataBuffer() : EMPTY_ARRAY,
decryptAADBuf.size()
);

if (ptrOut != null) {
context = new NativeContext(ptrOut[0]);

if (ptrOut != null) {
context = new NativeContext(ptrOut[0]);
}
}
// Decryption completed successfully. Copy it to the output location
System.arraycopy(tempBuffer, 0, output, outputOffset, outLen);
return outLen;
}
}
} // End of Decrypt mode

checkNeedReset();
// Start of Encrypt mode
checkNeedReset();

this.needReset = true;
final byte[] finalBytes = bytes;
final int finalOffset = offset;
final int finalOutputOffset = outputOffset;
final int finalLength = length;
if (!contextInitialized) {
// Context has not been initialized, meaning the user called doFinal immediately after init(). In this case
// we make a single native call to perform the encryption operation in one go.

keyUsageCount++;
if (context != null) {
// Our key, but not our IV has been initialized
return context.use(ptr -> {
return oneShotEncrypt(
ptr,
null,
this.needReset = true;
final byte[] finalBytes = bytes;
final int finalOffset = offset;
final int finalOutputOffset = outputOffset;
final int finalLength = length;
if (!contextInitialized) {
// Context has not been initialized, meaning the user called doFinal immediately after init(). In this case
// we make a single native call to perform the encryption operation in one go.

keyUsageCount++;
if (context != null) {
// Our key, but not our IV has been initialized
return context.use(ptr -> {
return oneShotEncrypt(
ptr,
null,
finalBytes,
finalOffset,
finalLength,
output,
finalOutputOffset,
tagLength,
key,
iv
);
});
} else {
// We don't have an existing context, however we might want to save one
final long[] ptrOut = keyUsageCount > KEY_REUSE_THRESHOLD ? new long[1] : null;
final int outLen = oneShotEncrypt(
0,
ptrOut,
finalBytes,
finalOffset,
finalLength,
Expand All @@ -650,29 +665,12 @@ protected synchronized int engineDoFinal(byte[] bytes, int offset, int length, b
key,
iv
);
});
} else {
// We don't have an existing context, however we might want to save one
final long[] ptrOut = keyUsageCount > KEY_REUSE_THRESHOLD ? new long[1] : null;
final int outLen = oneShotEncrypt(
0,
ptrOut,
finalBytes,
finalOffset,
finalLength,
output,
finalOutputOffset,
tagLength,
key,
iv
);
if (ptrOut != null) {
context = new NativeContext(ptrOut[0]);
if (ptrOut != null) {
context = new NativeContext(ptrOut[0]);
}
return outLen;
}
return outLen;
}
} else {
try {
} else {
// We need to make sure to add resultLength here; engineUpdate in encrypt mode produces incremental
// output (unlike in decrypt mode) and so we need to carry forward whatever amount of data it produced
// in our return value.
Expand Down Expand Up @@ -712,9 +710,9 @@ protected synchronized int engineDoFinal(byte[] bytes, int offset, int length, b
context = null;
}
return resultLength + finalOutputLen;
} finally {
stateReset();
}
} finally {
stateReset();
}
}

Expand Down
50 changes: 50 additions & 0 deletions tst/com/amazon/corretto/crypto/provider/test/AesTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -638,6 +638,56 @@ public void testBadAEADTagException() throws Throwable {
}
}

@Test
public void testBadAEADTagException_noRelease() throws Throwable {
final SecureRandom rnd = TestUtil.MISC_SECURE_RANDOM.get();

Cipher c = Cipher.getInstance(ALGO_NAME, PROVIDER_SUN);
GCMParameterSpec algorithmParameterSpec = new GCMParameterSpec(128, randomIV());
c.init(Cipher.ENCRYPT_MODE, key, algorithmParameterSpec, rnd);

byte[] plaintext = randomIV();
byte[] data = c.doFinal(plaintext);
byte[] output = new byte[plaintext.length];

for (int bit = 0; bit < data.length * 8; bit++) {
byte[] corruptData = data.clone();
corruptData[bit / 8] ^= (1 << (bit % 8));

Cipher check = Cipher.getInstance(ALGO_NAME, PROVIDER_AMAZON);
check.init(Cipher.DECRYPT_MODE, key, algorithmParameterSpec, rnd);

assertThrows(AEADBadTagException.class, () -> check.doFinal(corruptData, 0, corruptData.length, output, 0));
assertArrayEquals(new byte[plaintext.length], output);
}
}

@Test
public void testBadAEADTagException_noReleaseByteBuffer() throws Throwable {
final SecureRandom rnd = TestUtil.MISC_SECURE_RANDOM.get();

Cipher c = Cipher.getInstance(ALGO_NAME, PROVIDER_SUN);
GCMParameterSpec algorithmParameterSpec = new GCMParameterSpec(128, randomIV());
c.init(Cipher.ENCRYPT_MODE, key, algorithmParameterSpec, rnd);

byte[] plaintext = randomIV();
byte[] data = c.doFinal(plaintext);
byte[] output = new byte[plaintext.length];

for (int bit = 0; bit < data.length * 8; bit++) {
byte[] corruptData = data.clone();
corruptData[bit / 8] ^= (1 << (bit % 8));
ByteBuffer corruptBuff = ByteBuffer.wrap(corruptData);
ByteBuffer outputBuff = ByteBuffer.wrap(output);

Cipher check = Cipher.getInstance(ALGO_NAME, PROVIDER_AMAZON);
check.init(Cipher.DECRYPT_MODE, key, algorithmParameterSpec, rnd);

assertThrows(AEADBadTagException.class, () -> check.doFinal(corruptBuff, outputBuff));
assertArrayEquals(new byte[plaintext.length], output);
}
}

@Test
public void whenCipherReusedWithoutReinit_throwsIVReuseException() throws Throwable {
final SecureRandom rnd = TestUtil.MISC_SECURE_RANDOM.get();
Expand Down
16 changes: 16 additions & 0 deletions tst/com/amazon/corretto/crypto/provider/test/RsaCipherTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import javax.crypto.spec.SecretKeySpec;

import java.math.BigInteger;
import java.nio.ByteBuffer;
import java.security.AlgorithmParameters;
import java.security.GeneralSecurityException;
import java.security.InvalidKeyException;
Expand Down Expand Up @@ -910,6 +911,21 @@ private void testEncryptDecryptCycle(final Cipher encrypt, final Cipher decrypt,
final byte[] ciphertext = encrypt.doFinal(plaintext);
final byte[] decrypted = decrypt.doFinal(ciphertext);
assertArrayEquals(plaintext, decrypted);

// Verify no release of data even on bad padding
if (!NO_PADDING.equals(padding)) {
final byte[] result = new byte[ciphertext.length]; // Full size
ciphertext[3] ^= 0x13; // Just twiddle some bits
assertThrows(BadPaddingException.class, () -> decrypt.doFinal(ciphertext, 0, ciphertext.length, result, 0));
assertArrayEquals(new byte[ciphertext.length], result);

Arrays.fill(result, (byte) 0);
ByteBuffer ciphertextBuff = ByteBuffer.wrap(ciphertext);
ByteBuffer resultBuff = ByteBuffer.wrap(result);
assertThrows(BadPaddingException.class, () -> decrypt.doFinal(ciphertextBuff, resultBuff));
assertArrayEquals(new byte[ciphertext.length], result);
}

}

private void wrapUnwrap(final Cipher wrap, final Cipher unwrap) throws InvalidKeyException,
Expand Down

0 comments on commit 07b6e86

Please sign in to comment.