Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not modify output when Cipher.doFinal fails #123

Merged
merged 5 commits into from
Sep 1, 2020
Merged

Do not modify output when Cipher.doFinal fails #123

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e1aa886
Add tests for not releasing partial decryptions
SalusaSecondus Aug 24, 2020
c1b03e8
Ensure plaintext is not exposed when AES-GCM tag is wrong
SalusaSecondus Aug 24, 2020
c751f9f
Ignore a few more files
SalusaSecondus Aug 24, 2020
6fc66d1
Merge branch 'develop' into doFinal3
SalusaSecondus Aug 29, 2020
c46d5db
Merge branch 'develop' into doFinal3
SalusaSecondus Aug 31, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .gitignore
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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