Skip to content

GH-50522: [C++][Gandiva] fix heap overflow in aes_encrypt output sizing - #50523

Open
Arawoof06 wants to merge 1 commit into
apache:mainfrom
Arawoof06:aes-encrypt-block-size
Open

Arawoof06 wants to merge 1 commit into
apache:mainfrom
Arawoof06:aes-encrypt-block-size

Conversation

@Arawoof06

@Arawoof06 Arawoof06 commented Jul 16, 2026 •

Copy link
Copy Markdown
Contributor

Rationale for this change

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.

Reviewed before submission by:

  • Human
  • AI
  • Not reviewed

@github-actions

Copy link
Copy Markdown

⚠️ GitHub issue #50522 has been automatically assigned in GitHub to PR creator.

@github-actions

Copy link
Copy Markdown

⚠️ GitHub issue #50522 has no components, please add labels for components.

@Arawoof06

Copy link
Copy Markdown
Contributor Author

any update here?

@Arawoof06
Arawoof06 force-pushed the aes-encrypt-block-size branch from 4befacf to 07ae81e Compare August 19, 2026 14:23
@Arawoof06

Copy link
Copy Markdown
Contributor Author

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.

@github-actions

github-actions Bot commented Oct 5, 2026

Copy link
Copy Markdown

⚠️ GitHub issue #50522 has no components, please add labels for components.

@dmitry-chirkov-dremio

Copy link
Copy Markdown
Contributor

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.

@Arawoof06
Arawoof06 force-pushed the aes-encrypt-block-size branch from 07ae81e to b74f571 Compare October 15, 2026 06:33
@Arawoof06

Copy link
Copy Markdown
Contributor Author

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.

@dmitry-chirkov-dremio

dmitry-chirkov-dremio commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

You're right, and the issue wording is off.

Could you update the issue's text to reflect that, please?

@github-actions github-actions Bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Oct 15, 2026
@Arawoof06

Copy link
Copy Markdown
Contributor Author

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants