20260616-aes-fixes by douzzer · Pull Request #10709 · wolfSSL/wolfssl

douzzer · 2026-06-16T23:11:22Z

wolfcrypt/src/aes.c: catch and error on total length overflow in wc_AesGcmEncryptUpdate(), wc_AesGcmDecryptUpdate(), wc_AesCcmEncrypt(), and wc_AesCcmEncrypt().

tested with

wolfssl-multi-test.sh ...
super-quick-check

wolfSSL-Fenrir-bot

Fenrir Automated Review — PR #10709

Scan targets checked: wolfcrypt-bugs, wolfcrypt-src

Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Findings are non-blocking.

github-actions · 2026-06-16T23:22:16Z

MemBrowse Memory Report

gcc-arm-cortex-m4-openssl-compat

FLASH: .text +64 B (+0.0%, 767,796 B / 1,048,576 B, total: 73% used)

gcc-arm-cortex-m4-pkcs7

FLASH: .text +64 B (+0.0%, 211,377 B / 262,144 B, total: 81% used)
No memory changes detected for:
gcc-arm-cortex-m0plus
gcc-arm-cortex-m3
gcc-arm-cortex-m4
gcc-arm-cortex-m4-baremetal
gcc-arm-cortex-m4-crypto-only
gcc-arm-cortex-m4-dtls13
gcc-arm-cortex-m4-min-ecc
gcc-arm-cortex-m4-pq
gcc-arm-cortex-m4-rsa-only
gcc-arm-cortex-m4-sp-math
gcc-arm-cortex-m4-tls12
gcc-arm-cortex-m4-tls13
gcc-arm-cortex-m7
gcc-arm-cortex-m7-pq
gcc-arm-cortex-m7-tls13
linuxkm-pie
linuxkm-standard
stm32-sim-stm32h753

Frauschi

Should we add test coverage for the OVERFLOW_E cases?

dgarske

Skoll Code Review

Scan type: reviewOverall recommendation: COMMENT
Findings: 5 total — 4 posted, 1 skipped
4 finding(s) posted as inline comments (see file-level comments below)

Posted findings

[Low] Comment typo: 'cunulative' should be 'cumulative' — wolfcrypt/src/aes.c:12813,12967
[Low] Brace-on-own-line and redundant double parentheses deviate from file style — wolfcrypt/src/aes.c:12816-12821,12970-12975
[Low] Bare 0xffffffff literal; prefer suffixed/named constant — wolfcrypt/src/aes.c:12817-12818,12971-12972
[Low] Reused AES_CCM_OVERFLOW_E error string is about 'invocation counter', not input length — wolfcrypt/src/aes.c:13621,13785

Skipped findings

[Medium] ~~No tests added for new AES-GCM/CCM overflow error paths~~

Review generated by Skoll

dgarske · 2026-06-17T15:13:31Z

+     * can't currently support the maximum allowed.
+     */
+    if ((ret == 0) &&
+        (((aes->cSz > 0xffffffff - sz)) ||


🔵 [Low] Bare 0xffffffff literal; prefer suffixed/named constant

The overflow bound is written as the bare literal 0xffffffff. It is functionally correct (an unsigned hex literal that fits word32 on all supported platforms, including 16-bit-int targets where it promotes to unsigned long), but the wolfSSL convention favors an explicit unsigned suffix (0xFFFFFFFFU) or a named maximum to make the word32-max intent obvious and avoid any signedness ambiguity in the subtraction.

Fix: Add a U suffix (or use a named WORD32_MAX-style constant) for clarity and consistency.

fixed. used WOLFSSL_MAX_32BIT, a macro I wasn't previously familiar with.

dgarske · 2026-06-17T15:13:32Z

+    /* With a large nonce, B[] runs out of room to represent inSz, and beyond
+     * that, the counter itself can wrap.
+     */
+    if ((lenSz < sizeof(inSz)) &&


🔵 [Low] Reused AES_CCM_OVERFLOW_E error string is about 'invocation counter', not input length

The new CCM checks return AES_CCM_OVERFLOW_E, whose human-readable string (error.c:503-504) and header comment (error-crypt.h:255) describe it as 'AES-CCM invocation counter overflow'. The new condition is actually an input-length-vs-nonce-size limitation (the message length doesn't fit the CCM L field), not an invocation-counter overflow. Reusing the closest existing code is reasonable and avoids adding a new error number, but the diagnostic string may mislead someone debugging the failure. (The GCM reuse of AES_GCM_OVERFLOW_E for cumulative-size overflow is a closer semantic fit.)

Fix: Consider broadening the error string (e.g. 'AES-CCM length/counter overflow') so it covers the input-size case, or document the dual meaning. Not blocking.

The error string does need updating. Have made a note and will fix in a follow-up PR, or if this one needs another spin for a substantive reason, will fix it here.

…esGcmEncryptUpdate(), wc_AesGcmDecryptUpdate(), wc_AesCcmEncrypt(), and wc_AesCcmEncrypt().

…"catch and error on total length overflow".

dgarske

Skoll Multi-Scan Review

Modes: reviewOverall recommendation: COMMENT
Findings: 3 total — 3 posted, 0 skipped
3 finding(s) posted as inline comments (see file-level comments below)

One or more scans did not complete — the results below are partial.

Posted findings

[Low] [review] GCM aSz (AAD) overflow branch is not exercised by tests — wolfcrypt/test/test.c:19152-19161,19179-19188
[Low] [review] Raw sizeof in mixed-signedness comparison deviates from function's wordSz convention — wolfcrypt/src/aes.c:13618,13782
[Info] [review] Test code pointer style and line length deviate from wolfSSL conventions — wolfcrypt/test/test.c:21128-21130

Review generated by Skoll

dgarske · 2026-06-17T18:01:08Z

        ERROR_OUT(WC_TEST_RET_ENC_EC(ret), out);
+
+#if !defined(HAVE_FIPS) || FIPS_VERSION3_GE(7, 0, 0)
+    ret = wc_AesGcmEncryptUpdate(enc, resultC, p, 0xffffffff,


🔵 [Low] GCM aSz (AAD) overflow branch is not exercised by tests · test

The new overflow guard in wc_AesGcmEncryptUpdate/wc_AesGcmDecryptUpdate has two independent branches: aes->cSz > 0xffffffff - sz (ciphertext) and aes->aSz > 0xffffffff - authInSz (AAD). The new tests only drive the ciphertext branch by passing sz = 0xffffffff with authInSz = 0. The AAD-accumulation branch (aes->aSz overflow) is never tested, so a regression in that condition would go undetected.

Fix: Add a test that triggers the aSz overflow branch so both conditions of the new guard are covered. Add a symmetric case that passes a large authInSz (e.g. authIn buffer with authInSz = 0xffffffff and sz = 0) after a prior update that left aes->aSz > 0, asserting AES_GCM_OVERFLOW_E.

dgarske · 2026-06-17T18:01:08Z

+    /* With a large nonce, B[] runs out of room to represent inSz, and beyond
+     * that, the counter itself can wrap.
+     */
+    if ((lenSz < sizeof(inSz)) &&


🔵 [Low] Raw sizeof in mixed-signedness comparison deviates from function's wordSz convention · convention

The new check (lenSz < sizeof(inSz)) compares lenSz (a byte, integer-promoted to signed int) against sizeof(inSz) (size_t, unsigned). Both functions already define const word32 wordSz = (word32)sizeof(word32); precisely to avoid comparing against a raw sizeof (used in the loop a few lines below). Using the raw sizeof here is inconsistent with that local convention; depending on warning flags it can also surface as a signed/unsigned comparison note. The logic itself is correct (lenSz is small and non-negative).

Fix: Use the existing wordSz (with an explicit cast on lenSz) to keep the comparison consistent with the rest of the function and avoid mixed-signedness: if (((word32)lenSz < wordSz) && (inSz >= ((word32)1 << (lenSz * 8)))) { return AES_CCM_OVERFLOW_E; }

Not something to fix. The same new code pattern is used at 10 different code points, with widely varying local context. wordSz is only available at 2 of them. It is more useful to keep the checks identical, than to fork 2 of the 10 to leverage the (pointless) wordSz stack allocation.

dgarske · 2026-06-17T18:01:08Z

+        wc_static_assert(sizeof(iv) >= 13);
+        word32 clear_size = ((word32)1 << 16);
+        word32 cipher_size = ((word32)1 << 16);
+        byte *large_alloc = (byte *)XMALLOC(clear_size + cipher_size, HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER);


⚪ [Info] Test code pointer style and line length deviate from wolfSSL conventions · style

The new allocation in the CCM large-buffer test uses byte *large_alloc / byte *clear / byte *cipher and (byte *) casts rather than the wolfSSL byte* x placement, and line 21128 (byte *large_alloc = (byte *)XMALLOC(...)) exceeds the project's ~80-column width. The scoped { ... } block is justified (it bounds the malloc lifetime and frees on every path before goto out), so only the pointer style and line width are at issue. New-code-only nit; pre-existing mixed styles in test.c are out of scope.

Fix: Adopt byte* pointer placement and wrap the long XMALLOC line to stay within the column limit for new code.

Fixed the line length.

The incumbent code in aesccm_128_badarg_test() uses the form I'm using here (e.g. casts by (const byte *)).

dgarske

#10709 (review)

JacobBarthelmeh · 2026-06-17T18:14:54Z

+     */
+    if ((ret == 0) &&
+        ((aes->cSz > 0xffffffff - sz) ||
+         (aes->aSz > 0xffffffff - authInSz)))


It this is for WOLFSSL_MAX_32BIT values I think we should use the macro available.

Definitely the right idea! Fixing. This could protect us from build failures on weird targets in the future.

…c/port/riscv/riscv-64-aes.c, wolfcrypt/src/port/silabs/silabs_aes.c, wolfcrypt/src/port/ti/ti-aes.c: implement AES-CCM counter overflow checks for ports; wolfcrypt/test/test.c: add missing !HAVE_SELFTEST gate around AES-CCM counter overflow test in aesccm_128_badarg_test(); wolfcrypt/src/error.c and wolfssl/wolfcrypt/error-crypt.h: update messages for AES_{GCM,CCM}_OVERFLOW_E.

…her than magic 0xffffffff; wolfcrypt/test/test.c: in aesgcm_stream_test(), implement tests for sSz overflow, and in aesccm_128_badarg_test(), fix line length.

douzzer requested review from SparkiDev, wolfSSL-Bot and wolfSSL-Fenrir-bot June 16, 2026 23:11

douzzer assigned wolfSSL-Bot Jun 16, 2026

douzzer added the For This Release Release version 5.9.2 label Jun 16, 2026

wolfSSL-Fenrir-bot reviewed Jun 16, 2026

View reviewed changes

Comment thread wolfcrypt/src/aes.c Outdated

douzzer force-pushed the 20260616-aes-fixes branch 2 times, most recently from a9fefc4 to f1b8d03 Compare June 17, 2026 03:11

douzzer requested a review from Frauschi June 17, 2026 14:29

Frauschi requested changes Jun 17, 2026

View reviewed changes

Comment thread wolfcrypt/src/aes.c Outdated

Comment thread wolfcrypt/src/aes.c Outdated

dgarske reviewed Jun 17, 2026

View reviewed changes

dgarske assigned douzzer Jun 17, 2026

douzzer added 2 commits June 17, 2026 12:01

wolfcrypt/src/aes.c: catch and error on total length overflow in wc_A…

5def276

…esGcmEncryptUpdate(), wc_AesGcmDecryptUpdate(), wc_AesCcmEncrypt(), and wc_AesCcmEncrypt().

wolfcrypt/src/aes.c and wolfcrypt/test/test.c: fixes from review, re …

1070384

…"catch and error on total length overflow".

douzzer force-pushed the 20260616-aes-fixes branch from f1b8d03 to 1070384 Compare June 17, 2026 17:01

douzzer requested review from Frauschi and dgarske June 17, 2026 17:02

dgarske reviewed Jun 17, 2026

View reviewed changes

dgarske requested changes Jun 17, 2026

View reviewed changes

JacobBarthelmeh reviewed Jun 17, 2026

View reviewed changes

douzzer added 2 commits June 17, 2026 13:18

wolfcrypt/src/aes.c, wolfcrypt/test/test.c: use WOLFSSL_MAX_32BIT rat…

881fe76

…her than magic 0xffffffff; wolfcrypt/test/test.c: in aesgcm_stream_test(), implement tests for sSz overflow, and in aesccm_128_badarg_test(), fix line length.

douzzer requested review from JacobBarthelmeh, dgarske and wolfSSL-Fenrir-bot June 17, 2026 18:55

Conversation

douzzer commented Jun 16, 2026

Uh oh!

wolfSSL-Fenrir-bot left a comment

Choose a reason for hiding this comment

Fenrir Automated Review — PR #10709

Uh oh!

Uh oh!

github-actions Bot commented Jun 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

MemBrowse Memory Report

Uh oh!

Frauschi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dgarske left a comment

Choose a reason for hiding this comment

Skoll Code Review

Posted findings

Skipped findings

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

douzzer Jun 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dgarske left a comment

Choose a reason for hiding this comment

Skoll Multi-Scan Review

Posted findings

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dgarske left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

github-actions Bot commented Jun 16, 2026 •

edited

Loading

douzzer Jun 17, 2026 •

edited

Loading