Skip to content

fix: restore alloc-size >= ivlen invariant in legacy AES-GCM SET_IVLEN - #3466

Open
dougch wants to merge 1 commit into
aws:mainfrom
dougch:port/boringssl-aes-gcm-set-ivlen
Open

fix: restore alloc-size >= ivlen invariant in legacy AES-GCM SET_IVLEN#3466
dougch wants to merge 1 commit into
aws:mainfrom
dougch:port/boringssl-aes-gcm-set-ivlen

Conversation

@dougch

@dougch dougch commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Context and motivation

EVP_CTRL_AEAD_SET_IVLEN did not maintain the invariant that
alloc-size(gctx->iv) >= gctx->ivlen. Its old guard
arg > EVP_MAX_IV_LENGTH && arg > gctx->ivlen skipped reallocation
when shrinking (leaving gctx->iv pointing at a stale heap buffer
while callers assumed the built-in c->iv was in use) and, after an
intervening EVP_CIPHER_CTX_copy, allowed a subsequent grow within
the "no reallocation" range to raise gctx->ivlen above the actual
buffer size. This bypasses the ivlen >= 8 guard on
EVP_CTRL_GCM_IV_GEN and lets its 8-byte counter store land
out-of-bounds.

Concrete bypass sequence, all through public EVP APIs:

  • SET_IVLEN(large) allocates a heap buffer
  • SET_IVLEN(4) leaves gctx->iv pointing at that heap buffer
    but sets ivlen = 4
  • EVP_CIPHER_CTX_copy triggers EVP_CTRL_COPY, which does
    memdup(gctx->iv, gctx->ivlen) = a 4-byte heap allocation on
    the destination
  • SET_IVLEN(8) on the destination fails the old guard
    (8 > 16 && 8 > 4 → false), so no reallocation, and now
    ivlen = 8 sits on a 4-byte buffer
  • EVP_CTRL_GCM_IV_GEN passes its ivlen < 8 check, then writes
    8 bytes at iv + ivlen - 8 = iv + 0, four of which are OOB

Description of changes

Ports BoringSSL commit
04aa32f9.
EVP_CTRL_AEAD_SET_IVLEN is rewritten into two explicit branches:

  • arg <= EVP_MAX_IV_LENGTH — if gctx->iv currently points at a
    heap buffer, free it and reset gctx->iv = c->iv. This makes the
    small-IV state always live in the built-in buffer.
  • arg > gctx->ivlen — allocate the new buffer first, then free the
    old one, so an OOM leaves the context intact.
  • Otherwise (existing heap buffer is already large enough) — reuse
    in place.

With the invariant restored, the existing EVP_CTRL_COPY handler's
memdup(gctx->iv, gctx->ivlen) reads a validly-sized buffer in every
reachable state, so no separate change to the copy handler is needed.

Testing

Adds TEST(CipherTest, GCMRepeatedlyChangeIVLength), mirroring the
shape of upstream's regression test. Six length sequences (monotonic
grow and shrink in both the heap and the built-in-buffer regime, plus
two zig-zag sequences crossing EVP_MAX_IV_LENGTH) run in both
no-copy and copy-interleaved variants, comparing the resulting
ciphertext and tag against a straight-line reference encryption.

The regression manifests as an OOB heap access, not as a ciphertext
mismatch.
aes_gcm_init_key computes GCM state from the caller's
iv pointer, not from gctx->iv, so the specific EncryptUpdate /
GET_TAG path this test drives produces the correct ciphertext even
when gctx->iv is undersized — the OOB memcpy into gctx->iv
smashes adjacent heap silently. This test therefore only fails
pre-fix under an AddressSanitizer build. Please pay particular
attention to the Linux ASan CI job; that is the signal that
distinguishes fixed from broken.

Review considerations

  • FIPS boundary. crypto/fipsmodule/cipher/e_aes.c is inside the
    FIPS module. This change is internal buffer-management bookkeeping
    only. No public API, no ABI change, no algorithm behavior change
    visible outside the module, no new entry points. The bypass path
    was reachable before this change; it is not any more.
  • Prior work. A related fix rejecting ivlen < 8 in
    EVP_CTRL_GCM_IV_GEN shipped earlier (tracked in an internal SIM).
    That guard is only sound against the bypass this PR closes;
    together the two commits close the underflow class end-to-end.
  • No CHANGES file to update in this repo. clang-format was not run
    locally; CI will flag drift if any.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.

@github-actions

Copy link
Copy Markdown
Contributor

🔒 Security ReviewView Report

Please review before merging.

@codecov-commenter

codecov-commenter commented Aug 31, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.14%. Comparing base (ac587fb) to head (d86460a).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3466      +/-   ##
==========================================
+ Coverage   78.11%   78.14%   +0.02%     
==========================================
  Files         700      700              
  Lines      125599   125628      +29     
  Branches    17372    17378       +6     
==========================================
+ Hits        98117    98177      +60     
+ Misses      26611    26580      -31     
  Partials      871      871              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@dougch
dougch marked this pull request as ready for review September 1, 2026 17:57
@dougch
dougch requested a review from a team as a code owner September 1, 2026 17:57
Port BoringSSL 04aa32f96. EVP_CTRL_AEAD_SET_IVLEN's old guard
(arg > EVP_MAX_IV_LENGTH && arg > gctx->ivlen) let gctx->iv keep
pointing at an undersized heap buffer after a COPY-then-grow
sequence, bypassing the ivlen >= 8 guard added for AWS-LC-1298.

Adds TEST(CipherTest, GCMRepeatedlyChangeIVLength) mirroring
upstream's coverage. Regression manifests as OOB heap access;
Linux ASan CI catches it.
@justsmth
justsmth force-pushed the port/boringssl-aes-gcm-set-ivlen branch from a0ee5b5 to d86460a Compare September 8, 2026 21:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants