Java: Reuse Cipher in AesGcmJce

[kelunik]

Currently, AesGcmJce calls instance() for every cryptographic operation, which in turn calls EngineFactory.CIPHER.getInstance(), which is rather slow.

According to the Java documentation, Cipher objects are reusable, as all state is dropped when they're re-initialized. As far as I can tell, these Cipher instances could be stored in a ThreadLocal allowing them to be reused from the same thread at least, which seems to be rather common, e.g. decoding multiple database rows sequentially with a fixed application key.

[thaidn]

Thanks for the suggestion.

@bleichen found that one of the dangerous features in JCA is that the default behaviour of Cipher.doFinal is to reinitialize the cipher with the same parameters as the last encryption. For AES-GCM this behaviour would imply that the authentication key is leaked, if the Cipher object is reused in two encryptions without an explicit initialization of the IV.

Our implementation always explicitly initializes the IV though, so I think what your suggestion should work.

@bleichen what do you think?

[bleichen]

On Tue, Jul 23, 2019 at 10:26 PM Thai Duong ***@***.***> wrote: Thanks for the suggestion. @bleichen <https://github.com/bleichen> found that one of the dangerous features in JCA is that the default behaviour of Cipher.doFinal is to reinitialize the cipher with the same parameters as the last encryption. For AES-GCM this behaviour would imply that the authentication key is leaked, if the Cipher object is reused in two encryptions without an explicit initialization of the IV.

jdk, conscrypt and BouncyCastle all changed their implementations so that calling .updated directly after .doFinal throws an exception, because this would reuse the same IV otherwise. Hence this forces the caller to use .init() after a .doFinal(). There are no CVEs for these changes, since the maintainers of the JCA providers typically point out that it would be the users fault to use Cipher incorrectly.

Our implementation always explicitly initializes the IV though, so I think what your suggestion should work.

Sure. The providers would throw exceptions if that were not done.

…

@bleichen <https://github.com/bleichen> what do you think? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#208>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGGH7XSI7NNIGV55ZN4W5YTQA5SNRANCNFSM4HNGXCEQ> .

[thaidn]

This is fixed, and will be included in 1.3.0.

Thanks so much for the suggestion! It was indeed much faster.

[kelunik]

@thaidn: That's very good news! Thank you for your work on this.