AES-GCM: constant-time output clear on decrypt auth failure#10895
Merged
Conversation
AES_GCM_decrypt_C cleared the output on a tag mismatch with 'if (ret != 0) ForceZero(out, sz)'. That is a conditional branch on the secret-dependent authentication result, which is not constant time and is flagged by the ct-valgrind constant-time test (Conditional jump depends on uninitialised value in AES_GCM_decrypt_C). Mask the output with 'res' (already computed as all-ones on tag mismatch, zero on match) instead of branching, matching the constant-time idiom used for the tag comparison itself. C path only; the AES-NI/ASM paths are unaffected.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the AES-GCM C decryption path to clear (mask) decrypted output on authentication-tag mismatch without a secret-dependent conditional branch, addressing constant-time analysis findings from ct-valgrind in AES_GCM_decrypt_C.
Changes:
- Replace
if (ret != 0) ForceZero(out, sz)with a constant-time masking loop driven by the tag-compare result mask (res). - Add local variables (
mask,i) to support branchless output clearing.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #10895
Scan targets checked: wolfcrypt-bugs, wolfcrypt-src
No new issues found in the changed files. ✅
Frauschi
reviewed
Jul 14, 2026
Frauschi
left a comment
Frauschi
left a comment
Contributor
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: APPROVE
Findings: 2 total — 2 posted, 0 skipped
Posted findings
- [Low] Output masking now runs a full pass over out[] on every successful decrypt —
wolfcrypt/src/aes.c:11446-11449 - [Low] No test asserts output buffer is zeroed on tag mismatch for the C path —
wolfcrypt/src/aes.c:11444-11449
Review generated by Skoll via Claude/Codex
Review follow-ups for the constant-time AES-GCM decrypt output clear: - Guard the output-masking pass with #ifndef WC_AES_GCM_DEC_AUTH_EARLY. In that configuration the tag is verified before decryption and a mismatch returns before any output is written, so the masking pass is a guaranteed no-op; skipping it avoids a wasted O(sz) pass. - Add a test in aesgcm_test: decrypt with a corrupted tag into a pre-filled buffer and assert wc_AesGcmDecrypt returns AES_GCM_AUTH_E and, on the software C path, that the output buffer is cleared to zero. The AES-NI/asm decrypt paths and the FIPS module do not clear the output on auth failure, so the zero check forces the C path (use_aesni = 0) and is limited to it (and skipped under HAVE_FIPS). The AES_GCM_AUTH_E comparison uses WC_NO_ERR_TRACE(). Verified (gcc 15.2): make check passes on the default (C path) build; testwolfcrypt AES-GCM passes with --enable-aesni and with -DWC_AES_GCM_DEC_AUTH_EARLY; ct-valgrind aes_gcm reports 0 errors.
check-source-text reports these as unneeded because the macros are now defined in the checked build config, so their known-extra whitelist entries are redundant: WOLFSSL_ASN_TEMPLATE_NEED_SET_INT32 WOLFSSL_ASYNC_CERT_YIELD WOLFSSL_MLKEM_DYNAMIC_KEYS
danielinux
force-pushed
the
aes-gcm-fix-ct-compare
branch
from
July 14, 2026 15:19
2847e99 to
e7938b3
Compare
Frauschi
requested changes
Jul 14, 2026
Frauschi
left a comment
Frauschi
left a comment
Contributor
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: REQUEST_CHANGES
Findings: 4 total — 2 posted, 2 skipped
Posted findings
- [High] Zero-check test fails on WC_AES_GCM_DEC_AUTH_EARLY builds —
wolfcrypt/test/test.c:19197-19214 - [Medium] Zero-check denylist may fail under WOLFSSL_ASYNC_CRYPT with a real device —
wolfcrypt/test/test.c:19180-19214
Skipped findings
- [Low] Unrelated macro-list deletions bundled into AES-GCM fix
- [Low] Unconditional output pass runs even on auth success
Review generated by Skoll via Claude/Codex
Skoll review of the auth-fail zero-check test in aesgcm_test: - The guard listed WOLFSSL_ARMASM_NO_HW_CRYPTO and __aarch64__, which are defined on default x86-64 builds, so the zero-check block was compiled out and the assertion never actually ran there. They are subsumed by WOLFSSL_ARMASM (the condition under which AES_GCM_decrypt_C is not the decrypt path), so use that instead and the check runs on the C path. - Exclude WC_AES_GCM_DEC_AUTH_EARLY (out is not written on an early-auth failure) and WOLFSSL_ASYNC_CRYPT (a real async device may offload the decrypt and not clear the output). Verified: default make check passes with the zero-check now executing; testwolfcrypt AES-GCM passes with --enable-aesni and with -DWC_AES_GCM_DEC_AUTH_EARLY.
The RISC-V ASM build provides its own AES-GCM implementation (wolfcrypt/src/port/riscv/riscv-64-aes.c) rather than AES_GCM_decrypt_C, so it does not clear the output buffer on authentication failure. Exclude it from the zero-check, matching the other non-C decrypt paths. Fixes the riscv64 multi-arch testwolfcrypt failure.
Member
Author
|
Jenkins retest this please |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
AES_GCM_decrypt_C cleared the output on a tag mismatch with 'if (ret != 0) ForceZero(out, sz)'. That is a conditional branch on the secret-dependent authentication result, which is not constant time and is flagged by the ct-valgrind constant-time test (Conditional jump depends on uninitialised value in AES_GCM_decrypt_C).
Mask the output with 'res' (already computed as all-ones on tag mismatch, zero on match) instead of branching, matching the constant-time idiom used for the tag comparison itself. C path only; the AES-NI/ASM paths are unaffected.
Testing
Found while troubleshooting a recurring failing nightly test
nightly-ConstantTimeTest-v2