bcgit / bc-java

Bouncy Castle Java Distribution (Mirror)
https://www.bouncycastle.org/java.html
MIT License
2.31k stars 1.14k forks source link

Slight deviation from Cipher.update Javadoc when using AES/GCM/NoPadding #838

Closed srdo closed 3 years ago

srdo commented 3 years ago

This is a minor issue with an easy workaround, but I thought I'd report it.

The Javadoc for javax.crypto.Cipher.update (https://docs.oracle.com/en/java/javase/11/docs/api/java.base/javax/crypto/Cipher.html#update(byte%5B%5D)) says it returns

the new buffer with the result, or null if the underlying cipher is a block cipher and the input data is too short to result in a new block.

When using Bouncycastle's implementation of AES/GCM/NoPadding, the update method behaves as if the underlying cipher is a block cipher, returning null from Cipher.update if the input is too short. Isn't AES/GCM a stream cipher, and so it should not be returning null from update?

dghgit commented 3 years ago

I probably wouldn't call this a feature, but this has never been taken seriously. In the "good old days" a HSM implementing RC4 might return chunks of 4K as otherwise the overheads of passing the text around in smaller chunks would start to dominate the operation. RC4 is obviously a stream cipher, on the other hand 4K is a block... So it's not actually a bug either (well in the implementation, as you'll see I think the JavaDoc should be fixed).

You should always assume with calls to update that the underlying provider may choose not to return output if it decides it doesn't have enough - but even with a "stream cipher", if a tag is at the end of the stream for authentication purposes and AEAD modes were not considered when Cipher was originally defined, the provider will always need to hold onto some data on decryption as it won't know till doFinal() is called that it's actually hit the tag. Looked at that way, the promise of the JavaDoc is not really one that can actually be met.

In these days of Crypto Agility, it's probably better to read the JavaDoc as a warning. This is not something we'll change but I would stick with your work around even if we did. In some, perhaps not so distant, future, another developer will probably praise you for it.

srdo commented 3 years ago

Thanks for explaining.