Crypto layer: Add missing input validation - #10819
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens wolfCrypt/wolfSSL by adding additional input validation and tightening default Diffie–Hellman parameter minimums to align with modern security guidance (2048-bit minimum by default, overridable for legacy use).
Changes:
- Introduces
DH_MIN_SIZE(default 2048 bits) and aligns TLS-layer DH minimums with the DH primitive’s minimum. - Adds/adjusts input validation to prevent overflow/wraparound and invalid arguments in KDF/PRF, ECC key import, RSA key generation, and DH operations.
- Updates an existing DH test to be conditionally compiled based on
DH_MIN_SIZE.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| wolfssl/wolfcrypt/settings.h | Defines DH_MIN_SIZE with a secure default and legacy override mapping. |
| wolfssl/version.h | Updates library version macros (currently inconsistent with rest of repo). |
| wolfssl/internal.h | Aligns WOLFSSL_MIN_DHKEY_BITS default with DH_MIN_SIZE and enforces consistency. |
| wolfcrypt/src/rsa.c | Fixes heap usage in an OAEP error path and tightens RSA exponent validation under FIPS. |
| wolfcrypt/src/kdf.c | Adds null/length argument validation and prevents word32 wraparound in length checks. |
| wolfcrypt/src/ecc.c | Prevents potential word32 overflow when expanding compressed ECC point lengths. |
| wolfcrypt/src/dh.c | Rejects DH primes smaller than DH_MIN_SIZE and adds a null check in wc_DhCheckPubValue. |
| tests/api/test_dh.c | Gates a subgroup-check test on DH_MIN_SIZE (affects coverage in default builds). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
c66cde1 to
8961b60
Compare
|
Jenkins Retest this please |
cb5d209 to
69af8ed
Compare
|
@lealem47 PRB-master-job failures are legit. |
b465f15 to
7a1e0ca
Compare
|
Jenkins retest this please |
Frauschi
left a comment
Frauschi
left a comment
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: APPROVE
Findings: 2 total — 2 posted, 0 skipped
Posted findings
- [Low] FIPS RSA exponent floor reuses WC_RSA_EXPONENT default macro —
wolfcrypt/src/rsa.c:5415-5419 - [Info] Non-conforming indentation in wc_PRF NULL-argument check —
wolfcrypt/src/kdf.c:92-94
Review generated by Skoll via Claude/Codex
7a1e0ca to
6c96a72
Compare
|
Jenkins retest this please. |
Frauschi
left a comment
Frauschi
left a comment
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: APPROVE
Findings: 3 total — 2 posted, 2 skipped
Posted findings
- [Medium] WOLFSSL_MIN_AUTH_TAG_SZ clamp moved from kernel-only to all builds silently overrides explicit user values —
wolfssl/wolfcrypt/settings.h:3895-3919 (removed from 4275-4424 WOLFSSL_LINUXKM block) - [Info] Indentation of new wc_PRF NULL check does not match wolfSSL 4-space style —
wolfcrypt/src/kdf.c:92-94
Skipped findings
- [Low] CMAC minimum tag size raised 4->8 changes accepted tag range for existing callers
- [Medium] WOLFSSL_MIN_AUTH_TAG_SZ clamp moved from kernel-only to all builds silently overrides explicit user values
Review generated by Skoll via Claude/Codex
362a269 to
48794a1
Compare
48794a1 to
1a7a536
Compare
|
Jenkins Retest this please. |
1a7a536 to
8a51b31
Compare
|
Jenkins retest this please |
|
Jenkins Retest this please . |
Frauschi
left a comment
Frauschi
left a comment
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: REQUEST_CHANGES
Findings: 17 total — 6 posted, 11 skipped
Posted findings
- [High] FIPS gate narrowed from <7.0.0 to ==5.2.4 raises min GCM tag to 12 for kernel FIPS 6.x, breaking RFC 4106 8-byte ICV —
wolfssl/wolfcrypt/settings.h:3902-3913 - [Medium] FIPS 5.2.4 branch force-lowers WOLFSSL_MIN_AUTH_TAG_SZ to 8 for user-space builds and overrides an explicit user setting —
wolfssl/wolfcrypt/settings.h:3902-3906 - [Medium] WC_CMAC_TAG_MAX_SZ made user-overridable without bounding it to the 16-byte digest buffer —
wolfssl/wolfcrypt/cmac.h:101-104 - [Medium] AES-GCM/GMAC negative tests hardcode
- 10instead of deriving from WOLFSSL_MIN_AUTH_TAG_SZ —tests/api/test_aes.c:3384 - [Low] WC_CMAC_TAG_MIN_SZ redefined without #undef in the FIPS >= 7 branch —
wolfssl/wolfcrypt/cmac.h:105-110 - [Low] Redundant/dead FIPS >= 7 arm and kernel-only CONFIG_CRYPTO_ branches now applied to user-space builds* —
wolfssl/wolfcrypt/settings.h:3895-3919
Skipped findings
- [High] LINUXKM FIPS builds other than 5.2.4 lose 64-bit AES-GCM tag support, breaking RFC 4106 IPsec ESP
- [Medium] New validation paths in kdf.c/dh.c/ecc.c have no test coverage, and the MC/DC guard test no longer matches the guard
- [Medium] FIPS 5.2.4 arm unconditionally overrides an explicitly configured WOLFSSL_MIN_AUTH_TAG_SZ downward
- [Medium] WC_CMAC_TAG_MAX_SZ is now user-overridable but unvalidated, enabling a 16-byte over-read of cmac->digest
- [Low] wc_PRF NULL check indentation and missing braces are inconsistent with the sibling checks added in the same commit
- [Low] RsaUnPad_OAEP error path uses raw XFREE while the neighbouring error path uses WC_FREE_VAR_EX
- [Low] WC_CMAC_TAG_MIN_SZ redefined without #undef/#ifndef in the FIPS >= 7 arm
- [Low] WC_RSA_EXPONENT (a user-overridable default) misused as the FIPS minimum public exponent
- [Low] AES-GCM/GMAC negative tests hardcode a tag length instead of deriving it from WOLFSSL_MIN_AUTH_TAG_SZ
- [Info] Compressed x963 import of an over-large curve now returns ECC_BAD_ARG_E instead of ECC_CURVE_OID_E
- [Info] Two overlapping SP 800-38D clamps for WOLFSSL_MIN_AUTH_TAG_SZ with different failure behaviour
Review generated by Skoll via Claude/Codex
| #endif | ||
| #endif | ||
|
|
||
| #if defined(HAVE_FIPS) |
There was a problem hiding this comment.
🟠 [High] FIPS gate narrowed from <7.0.0 to ==5.2.4 raises min GCM tag to 12 for kernel FIPS 6.x, breaking RFC 4106 8-byte ICV
🚫 BLOCK bug
The block moved out of the WOLFSSL_LINUXKM section did not move verbatim: the FIPS condition changed from FIPS_VERSION3_LT(7, 0, 0) to FIPS_VERSION3_EQ(5, 2, 4). For a LINUXKM build with wolfCrypt FIPS v6.x (a supported kernel configuration - see linuxkm/module_hooks.c:115,639,1216 and linuxkm/linuxkm_wc_port.h:874 which all branch on FIPS_VERSION3_GE(6,0,0)), WOLFSSL_MIN_AUTH_TAG_SZ goes from 8 to 12. The kernel glue still advertises and accepts an 8-byte ICV for RFC 4106: km_AesGcmSetAuthsize_Rfc4106() (linuxkm/lkcapi_aes_glue.c:1092-1100) returns 0 for authsize == 8, and rfc4106(gcm(aes)) registration is not FIPS-gated (linuxkm/lkcapi_aes_glue.c:162-168). The tfm therefore accepts setauthsize(8), but every subsequent wc_AesGcmEncrypt/wc_AesGcmDecrypt/*Final call fails with BAD_FUNC_ARG at wolfcrypt/src/aes.c:20086 and :20389. IPsec ESP with a 64-bit ICV silently stops working on FIPS v6 kernel modules. The module self-test (linuxkm_test_aesgcm_rfc4106) uses a 16-byte tag, so nothing catches this at load time. The commit message says only "Remove MIN_AUTH_TAG_SZ logic from kernel section to synchronize with user-space build" - the version-predicate change is not mentioned and looks unintended.
Suggestion:
| #if defined(HAVE_FIPS) | |
| #if defined(HAVE_FIPS) | |
| #if FIPS_VERSION3_LT(7, 0, 0) | |
| /* support RFC 4106 IPsec ESP 64 bit tags */ | |
| #if WOLFSSL_MIN_AUTH_TAG_SZ > 8 | |
| #undef WOLFSSL_MIN_AUTH_TAG_SZ | |
| #define WOLFSSL_MIN_AUTH_TAG_SZ 8 | |
| #endif | |
| #else | |
| /* No short (<96 bit) tags per SP 800-38D 2026 revision in process. */ | |
| #if WOLFSSL_MIN_AUTH_TAG_SZ < 12 | |
| #undef WOLFSSL_MIN_AUTH_TAG_SZ | |
| #define WOLFSSL_MIN_AUTH_TAG_SZ 12 | |
| #endif | |
| #endif |
| #endif | ||
| #endif | ||
|
|
||
| #if defined(HAVE_FIPS) |
There was a problem hiding this comment.
🟡 [Medium] FIPS 5.2.4 branch force-lowers WOLFSSL_MIN_AUTH_TAG_SZ to 8 for user-space builds and overrides an explicit user setting
💡 SUGGEST api
Before this PR the whole adjustment block lived inside the #ifdef WOLFSSL_LINUXKM section, so a user-space FIPS build used the default (or user-supplied) WOLFSSL_MIN_AUTH_TAG_SZ from line 3889 - normally 12. Now every FIPS 5.2.4 build, kernel or user-space, gets 8. Two consequences: (1) user-space FIPS 5.2.4 applications silently start accepting 64-bit GCM tags where they previously required 96-bit; (2) unlike the two sibling branches at 3914-3925, this one is an unconditional #undef/#define rather than a clamp, so a deliberate -DWOLFSSL_MIN_AUTH_TAG_SZ=16 is silently downgraded to 8 - a "minimum" macro behaving as a hard ceiling. Relaxing an authentication-tag minimum for every consumer of one FIPS version is a security-relevant default change that deserves to be explicit in the PR description.
Suggestion:
| #if defined(HAVE_FIPS) | |
| #if FIPS_VERSION3_EQ(5, 2, 4) | |
| /* support RFC 4106 IPsec ESP 64 bit tags */ | |
| #if WOLFSSL_MIN_AUTH_TAG_SZ > 8 | |
| #undef WOLFSSL_MIN_AUTH_TAG_SZ | |
| #define WOLFSSL_MIN_AUTH_TAG_SZ 8 | |
| #endif | |
| #else |
| #define WC_CMAC_TAG_MAX_SZ WC_AES_BLOCK_SIZE | ||
| #define WC_CMAC_TAG_MIN_SZ (WC_AES_BLOCK_SIZE/4) | ||
| #else | ||
| /* Reasonable defaults */ |
There was a problem hiding this comment.
🟡 [Medium] WC_CMAC_TAG_MAX_SZ made user-overridable without bounding it to the 16-byte digest buffer
💡 SUGGEST api
The macro was previously hard-tied to the AES block size (#define WC_CMAC_TAG_MAX_SZ WC_AES_BLOCK_SIZE) and could not be overridden. Wrapping it in #ifndef makes it a user-tunable knob, but the only consumer is the upper bound in wc_CmacFinalNoFree() (wolfcrypt/src/cmac.c:396), which is immediately followed by XMEMCPY(out, cmac->digest, *outSz) (wolfcrypt/src/cmac.c:452, :456) where cmac->digest is a fixed byte digest[WC_AES_BLOCK_SIZE] (cmac.h:63). Any build defining WC_CMAC_TAG_MAX_SZ above 16 therefore turns a legal API call into an out-of-bounds read of the Cmac struct and leaks adjacent struct bytes (including k1/k2 subkey material) into the caller's output buffer. The header change alone creates the footgun; nothing in the tree enforces the upper bound.
Suggestion:
| /* Reasonable defaults */ | |
| /* Reasonable defaults */ | |
| #ifndef WC_CMAC_TAG_MAX_SZ | |
| #define WC_CMAC_TAG_MAX_SZ 16 | |
| #endif | |
| #if !defined(NO_AES) && (WC_CMAC_TAG_MAX_SZ > WC_AES_BLOCK_SIZE) | |
| #error WC_CMAC_TAG_MAX_SZ cannot exceed WC_AES_BLOCK_SIZE | |
| #endif |
| WC_NO_ERR_TRACE(BAD_FUNC_ARG)); | ||
| ExpectIntEQ(wc_AesGcmEncrypt(&aes, enc, vector, sizeof(vector), iv, | ||
| sizeof(iv)/sizeof(byte), resultT, sizeof(resultT) - 5, a, sizeof(a)), | ||
| sizeof(iv)/sizeof(byte), resultT, sizeof(resultT) - 10, a, sizeof(a)), |
There was a problem hiding this comment.
🟡 [Medium] AES-GCM/GMAC negative tests hardcode - 10 instead of deriving from WOLFSSL_MIN_AUTH_TAG_SZ
💡 SUGGEST test
These assertions exist to prove a too-short auth tag is rejected, but the tag length is expressed as a literal offset from a 16-byte buffer (sizeof(resultT) - 10 = 6, sizeof(tag3) - 10 = 6). The magic number had to be re-tuned in this PR precisely because WOLFSSL_MIN_AUTH_TAG_SZ moved (11 is no longer below the minimum for FIPS 5.2.4 builds), and it will silently invert again for any configuration where the minimum drops to <= 6 - e.g. the CONFIG_CRYPTO_MANAGER_EXTRA_TESTS / CONFIG_CRYPTO_SELFTESTS_FULL path introduced at settings.h:3914-3919, which clamps to 4. When that happens wc_AesGcmEncrypt returns 0 and the test fails with a confusing message rather than being skipped. The same file already has the robust pattern at test_aes.c:8017-8022 (WOLFSSL_MIN_AUTH_TAG_SZ - 1).
Suggestion:
| sizeof(iv)/sizeof(byte), resultT, sizeof(resultT) - 10, a, sizeof(a)), | |
| ExpectIntEQ(wc_AesGcmEncrypt(&aes, enc, vector, sizeof(vector), iv, | |
| sizeof(iv)/sizeof(byte), resultT, WOLFSSL_MIN_AUTH_TAG_SZ - 1, | |
| a, sizeof(a)), | |
| WC_NO_ERR_TRACE(BAD_FUNC_ARG)); |
| #ifndef WC_CMAC_TAG_MAX_SZ | ||
| #define WC_CMAC_TAG_MAX_SZ 16 | ||
| #endif | ||
| /* SP800-38b recommends a minimum tag length of 64-bits */ |
There was a problem hiding this comment.
🔵 [Low] WC_CMAC_TAG_MIN_SZ redefined without #undef in the FIPS >= 7 branch
💡 SUGGEST convention
The #elif !defined(WC_CMAC_TAG_MIN_SZ) arm newly advertises WC_CMAC_TAG_MIN_SZ as a user-overridable macro, but the FIPS >= 7.0.0 arm above it does a bare #define with no #undef. A FIPS v7 build that also passes -DWC_CMAC_TAG_MIN_SZ=4 (or sets it in user_settings.h) gets a macro redefinition with a different value - a diagnostic that becomes a hard error under the -Werror used in several CI configs. The settings.h changes in this same PR consistently use #undef before #define for exactly this reason.
Suggestion:
| /* SP800-38b recommends a minimum tag length of 64-bits */ | |
| /* SP800-38b recommends a minimum tag length of 64-bits */ | |
| #if FIPS_VERSION3_GE(7,0,0) | |
| #undef WC_CMAC_TAG_MIN_SZ | |
| #define WC_CMAC_TAG_MIN_SZ 8 | |
| #elif !defined(WC_CMAC_TAG_MIN_SZ) | |
| #define WC_CMAC_TAG_MIN_SZ 4 | |
| #endif |
| @@ -3899,6 +3899,32 @@ | |||
| #endif | |||
There was a problem hiding this comment.
🔵 [Low] Redundant/dead FIPS >= 7 arm and kernel-only CONFIG_CRYPTO_ branches now applied to user-space builds*
🔧 NIT style
Two small cleanups on the relocated block. (1) The #else arm at 3907-3913 silently bumps a sub-12 value to 12, but for FIPS >= 7.0.0 the pre-existing #error at 3895-3900 already rejects that same case, so the silent bump only ever fires for FIPS < 7 - the two policies now overlap and read as if they contradict each other. (2) The CONFIG_CRYPTO_MANAGER_EXTRA_TESTS / CONFIG_CRYPTO_SELFTESTS_FULL arm and its comment ("The Linux kernel native crypto fuzzer...") used to be reachable only inside #ifdef WOLFSSL_LINUXKM. At file scope it now also applies to any user-space build that happens to have those kernel Kconfig symbols defined, silently dropping the minimum tag size to 4 outside the kernel.
Suggestion:
| #endif | |
| #elif defined(WOLFSSL_LINUXKM) && \ | |
| (defined(CONFIG_CRYPTO_MANAGER_EXTRA_TESTS) || \ | |
| defined(CONFIG_CRYPTO_SELFTESTS_FULL)) | |
| /* The Linux kernel native crypto fuzzer expects small AES-GCM tag sizes to succeed. */ | |
| #if WOLFSSL_MIN_AUTH_TAG_SZ > 4 |
c92b673 to
2d69802
Compare
2d69802 to
a39ac0e
Compare
douzzer
left a comment
douzzer
left a comment
There was a problem hiding this comment.
Don't merge until I've done full testing of this on all customer kernels/configs -- it may be fine, I just haven't had time to review+test it in detail yet.
Description
WOLFSSL_MIN_AUTH_TAG_SZTesting
./configure --enable-all && make check
Checklist