Closed drok closed 1 year ago
Hello @drok , thanks for the PR.
I've checked invalid cyphertext lengths with various GnuTLS versions.
gnutls_cipher_decrypt2
doesn't seem to check the cyphertext length, and side effects can appear inside gnutls_cipher_decrypt2
gnutls_cipher_decrypt2
returns GNUTLS_E_INVALID_REQUEST
The problem with your patch is that lots of other test fail, because sometimes cyphertext length seems to be a modulo of block size with a margin of 1, like 63 or 65 instead of 64, and sometimes (cookbook tests), it's even more different.
If I change your check from:
if (cipher_block_size > 1 && ciphertext_decoded_len % cipher_block_size) {
to something like:
if (cipher_block_size > 1 && (ciphertext_decoded_len % cipher_block_size) != 0 &&
(ciphertext_decoded_len % cipher_block_size) != cipher_block_size-1 &&
(ciphertext_decoded_len % cipher_block_size) != 1) {
The most of the tests will succeed, except for the cookbook test, which fails at Key Wrap using AES-KeyWrap with AES-GCM. In that test, ciphertext length is 170 and block size is 16, therefore the modulo is 10.
Maybe the good solution is to backport in your fork the cipher text length check made in a recent GnuTLS version?
The problem with your patch is that lots of other test fail, because sometimes cyphertext length seems to be a modulo of block size with a margin of 1, like 63 or 65 instead of 64, and sometimes (cookbook tests), it's even more different.
I appears that my assumption that the cipher_block_size > 1
condition is equivalent to an assertion that the cipher given cipher is operating in block mode was incorrect. (eg., AES GCM has a block size of 16 or 32 depending on key size, but it is a streaming mode cipher, and is not subject to the constraint that the buffer size must be a multiple of block size)
Maybe the good solution is to backport in your fork the cipher text length check made in a recent GnuTLS version?
This is a great suggestion. The GnuTLS checks are not nicely contained in one neat package, so I implemented a lookup function, int gnutls_is_block_cipher(gnutls_cipher_algorithm_t alg)
which returns non-zero for ciphers in block mode, and zero otherwise. The updated branch uses this lookup to replace the cipher_block_size > 1
condition, so the buffer length constraint is applied only for block mode ciphers, which is what I believe the GnuTLS API documentation intended.
I have run the test suite on Ubuntu 22.04 and all tests pass. I apologize for not doing this initially.
AES GCM has a block size of 16 or 32 depending on key size, but it is a streaming mode cipher, and is not subject to the constraint that the buffer size must be a multiple of block size
Thanks for deciphering that!
Please don't force push your changes, just add commits on top of these ones
When the ciphertext in a JWE is modified so its length is not a multiple of the cipher block size,
r_jwe_decrypt_payload()
did not allocate enough memory, and calledgnutls_cipher_decrypt2()
with invalid arguments (ciphertext length non-multiple of block length).One would expect GnuTLS to error out in this case, but at least one version, "3.4.10-4ubuntu1.9", overruns the provided plaintext buffer. The provided plaintext buffer len is equal to the (invalid, non-multiple of block length) ciphertext length, as required by the
gnutls_cipher_decrypt2
APIThis branch adds a check to
r_jwe_decrypt_payload
to check if the provided ciphertext length is a multiple of the block size. If it is not, the JWE is rejected as invalid, without decoding the IV, ciphertext or tag. This avoids triggering the buffer overrun in affected GnuTLS libraries, and additionally avoids some mallocs and base64 decodes, making the "malicious JWE ciphertext" attack vector cheaper to handle.The af03aef patch is applied directly on top of the initial bug at d92ae6a, and is refit to the following refactorings in the upstream: