You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
gdv_fn_aes_encrypt sizes its output buffer with RoundUpToPowerOf2(data_len, key_data_len), which treats the key length as the AES block size. AES works on 16 byte blocks for every supported key length, so the rounding factor is wrong, and a 24 byte key asks RoundUpToPowerOf2 to round to a factor that is not a power of two, which its contract requires.
On top of that, PKCS#7 padding always appends a whole block, so a block aligned input produces a ciphertext one block longer than the input and the buffer comes up short. Allocated size against the ciphertext OpenSSL actually writes:
key
data_len
allocated
actual ciphertext
16
16
16
32
16
8192
8192
8208
24
8
8
16
32
32
32
48
aes_encrypt(col, key) over any 16/32/48/... byte value lets EVP_EncryptFinal_ex write 16 bytes past the arena buffer, and a 24 byte key with an 8 byte value writes 8 past. The overrun lands on whatever the arena hands out next, so the neighbouring row's output is corrupted; once the value exceeds the arena chunk size the chunk is allocated exactly and the write runs off the end of it.
gdv_fn_aes_decrypt sizes its buffer with the same expression. Its plaintext is never longer than the ciphertext so it does not overrun today, but the size it computes can equal data_len while OpenSSL documents inl + block_size as the output space EVP_DecryptUpdate may use, and the trailing ret[*out_len] = '\0' needs the room as well.
What changes are included in this PR?
Both stubs now size the output from the fixed 16 byte AES block plus one block for padding, which covers the exact PKCS#7 ciphertext length and the output space OpenSSL asks for. The key length keeps its 16/24/32 validation and only picks the cipher.
Are these changes tested?
Yes. TestGdvFnStubs.TestAesEncryptBlockAlignedInput encrypts two block aligned values from one context and decrypts the first. The undersized buffer makes the second ciphertext land on top of the first, so the decrypt fails on the current code and round-trips after the fix. It needs no sanitizer, which the existing Gandiva CI jobs do not enable. Full gandiva-internals-test, gandiva-precompiled-test and gandiva-projector-test pass.
Are there any user-facing changes?
No.
This PR contains a "Critical Fix". It fixes a heap buffer overflow in aes_encrypt that corrupts neighbouring output (or writes off the end of the arena chunk) for any input whose length is a multiple of the AES block size.
Was AI used for this PR?
In accordance to the AI generation guidelines, please disclose below whether and how AI was used in this PR.
PR code and description written by:
Human
AI
AI tooling was used to find the bug and to write the fix, the regression test and this description. The regression test was run against the unpatched code, where it fails, and against the patched code before submission.
Rebased onto main to get a fresh CI run. The only red before was the Windows MSVC GLib job, which died on an SSL error pulling CMake during vcpkg install (that job builds with Gandiva off, so unrelated to this change). New run is going now.
Issue says a 24-byte key can round to a value "below data_len". Could you confirm if it does?
It appears to be rounding below the needed ciphertext length, which is a different statement.
I'd say rebase, get a fresh build and ping me for review.
You're right, and the issue wording is off. With a 24-byte key RoundUpToPowerOf2(data_len, 24) never lands below data_len. 24 isn't a power of two so the mask isn't clean, but it still rounds to >= data_len for every length I checked. The shortfall is against the ciphertext length instead: data_len=8 rounds to 8 while the ciphertext is 16, and data_len=32 rounds to 32 while the ciphertext is 48. So it comes up short of the needed output, not below data_len. The real cause either way is the missing padding block, which is what the fix sizes in.
Rebased onto main and force-pushed, so fresh CI is running. Ready for your review.
Updated the issue. It now says the 24-byte key rounding never goes below data_len and that the shortfall is against the ciphertext length (data_len 8 rounds to 8, ciphertext is 16).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rationale for this change
gdv_fn_aes_encryptsizes its output buffer withRoundUpToPowerOf2(data_len, key_data_len), which treats the key length as the AES block size. AES works on 16 byte blocks for every supported key length, so the rounding factor is wrong, and a 24 byte key asksRoundUpToPowerOf2to round to a factor that is not a power of two, which its contract requires.On top of that, PKCS#7 padding always appends a whole block, so a block aligned input produces a ciphertext one block longer than the input and the buffer comes up short. Allocated size against the ciphertext OpenSSL actually writes:
aes_encrypt(col, key)over any 16/32/48/... byte value letsEVP_EncryptFinal_exwrite 16 bytes past the arena buffer, and a 24 byte key with an 8 byte value writes 8 past. The overrun lands on whatever the arena hands out next, so the neighbouring row's output is corrupted; once the value exceeds the arena chunk size the chunk is allocated exactly and the write runs off the end of it.gdv_fn_aes_decryptsizes its buffer with the same expression. Its plaintext is never longer than the ciphertext so it does not overrun today, but the size it computes can equaldata_lenwhile OpenSSL documentsinl + block_sizeas the output spaceEVP_DecryptUpdatemay use, and the trailingret[*out_len] = '\0'needs the room as well.What changes are included in this PR?
Both stubs now size the output from the fixed 16 byte AES block plus one block for padding, which covers the exact PKCS#7 ciphertext length and the output space OpenSSL asks for. The key length keeps its 16/24/32 validation and only picks the cipher.
Are these changes tested?
Yes.
TestGdvFnStubs.TestAesEncryptBlockAlignedInputencrypts two block aligned values from one context and decrypts the first. The undersized buffer makes the second ciphertext land on top of the first, so the decrypt fails on the current code and round-trips after the fix. It needs no sanitizer, which the existing Gandiva CI jobs do not enable. Fullgandiva-internals-test,gandiva-precompiled-testandgandiva-projector-testpass.Are there any user-facing changes?
No.
This PR contains a "Critical Fix". It fixes a heap buffer overflow in
aes_encryptthat corrupts neighbouring output (or writes off the end of the arena chunk) for any input whose length is a multiple of the AES block size.Was AI used for this PR?
In accordance to the AI generation guidelines, please disclose below whether and how AI was used in this PR.
PR code and description written by:
AI tooling was used to find the bug and to write the fix, the regression test and this description. The regression test was run against the unpatched code, where it fails, and against the patched code before submission.
Reviewed before submission by: