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

Jump to bottom

Fix input wiping on decryption failure #337

Closed
wants to merge 1 commit into from
Closed

Fix input wiping on decryption failure #337

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
15 changes: 6 additions & 9 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 @@ -707,8 +707,8 @@ private int engineDecryptFinal(
workingInputArray,
workingInputOffset,
workingInputLength,
output,
outputOffset,
workingInputArray, // Output
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In cases where the output buffer is distinct from the input, like here the copying back can be avoided by directly using the output buffer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am willing to change this, however:

  1. The current design never leaks unauthenticated plaintext outside the ACCP boundary.
  2. The proposed change is more complicated and we're already seeing bugs creep in which suggest that we're nearing maximum complexity to safely work here.

So, if left to me, I would leave this as written, but I am willing to change it. (Or, to read it another way. I have one approval and one request for changes. Mind reaching consensus?)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We spoke offline and I'm more hesitant now. My interpretation of the contract is that if the caller gives us an output buffer and a length we have no obligation to preserve the contents of that buffer even if the tag check fails. If we can preserve it for free, awesome, but if it adds O(n) memory I/O to all AES-GCM operations I'm less inclined. Even if we add the risky logic for overlap detection, what we'll find is that the perfect overlap case is the common case and it won't buy us much.

The remaining point of clarity for us is to look at how other libraries handle this case to determine if this is considered a bug as that's still a bit subjective to me. AFAICT, OpenSSL/AWS-LC will scribble over the output buffer and simply return an error code if the tag fails. I'd be hesitant to add new semantics at the ACCP layer which don't line up with the native library.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My concern is us overwriting the input buffer (which happens to be the same).

I have personally seen decryption logic which attempts repeated in-place decryption using multiple keys. Unfortunately, I no longer have access to said code. However, wiping the in-place buffer on failure would absolutely break that code.

More interesting than how OpenSSL/AWS-LC behave is how other JCA providers behave as that's what ACCP is replacing.

workingInputOffset, // Output Offset
tagLength,
key,
iv,
Expand All @@ -730,8 +730,8 @@ private int engineDecryptFinal(
workingInputArray,
workingInputOffset,
workingInputLength,
output,
outputOffset,
workingInputArray, // Output
workingInputOffset, // Output Offset
tagLength,
key,
iv,
Expand All @@ -747,12 +747,9 @@ private int engineDecryptFinal(
}
}
// Decryption completed successfully.
// Copy from working buffer into actual output
System.arraycopy(workingInputArray, workingInputOffset, output, outputOffset, outLen);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can avoid this copy in case the output buffer doesn't overlap input. Still in the case of failure the output buffer would potentially be modified, but it's output buffer after all.

return outLen;
} catch (AEADBadTagException e) {
final int maxFillSize = output.length - outputOffset;
Arrays.fill(
output, outputOffset, Math.min(maxFillSize, engineGetOutputSize(inputLen)), (byte) 0);
throw e;
} finally {
stateReset();
}
Expand Down
20 changes: 20 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 @@ -1199,6 +1199,26 @@ public void safeReuse() throws Throwable {
assertArrayEquals(plaintext2, c.doFinal(ciphertext2));
}

// When decrypting a ciphertext in-place (which is allowed), we should not modify the input buffer
// when the encryption fails.
@Test
public void inplaceShouldNotWipeInput() throws Exception {
final GCMParameterSpec spec = new GCMParameterSpec(128, randomIV());
final Cipher c = Cipher.getInstance(ALGO_NAME, NATIVE_PROVIDER);
c.init(Cipher.ENCRYPT_MODE, key, spec);

final byte[] properCiphertext = c.doFinal(PLAINTEXT);
final byte[] corruptCiphertext = properCiphertext.clone();
corruptCiphertext[0] ^= 0x01;
final byte[] inputCiphertext = corruptCiphertext.clone();
c.init(Cipher.DECRYPT_MODE, key, spec);
assertThrows(
AEADBadTagException.class,
"Tag mismatch!",
() -> c.doFinal(inputCiphertext, 0, inputCiphertext.length, inputCiphertext, 0));
assertArrayEquals(corruptCiphertext, inputCiphertext);
}

private byte[] randomIV() {
return TestUtil.getRandomBytes(16);
}
Expand Down
Loading