20260727-fips-dev-no-post - #11031
Conversation
|
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11031
Scan targets checked: linuxkm-bugs, linuxkm-src, wolfcrypt-bugs, wolfcrypt-port-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
Findings: 5
4 finding(s) posted as inline comments (see file-level comments below)
Medium (1)
Unguarded verifyCore reference in non-PIE FIPS seg_map under WOLFSSL_FIPS_DEV_NO_POST
File: linuxkm/module_hooks.c:1291
Function: seg_map (file-scope, #elif defined(HAVE_FIPS) branch)
Category: Incorrect error handling
The extern declaration of verifyCore (line 119) is now gated with !defined(WOLFSSL_FIPS_DEV_NO_POST), and the primary seg_map initializer (line 1223) got the matching && !defined(WOLFSSL_FIPS_DEV_NO_POST) guard, but the parallel seg_map used when WC_SYM_RELOC_TABLES is undefined (line 1290) was not updated to match, so it references the undeclared verifyCore symbol when building with --enable-fips=dev-no-post and PIE reloc tables disabled.
Recommendation: Add && !defined(WOLFSSL_FIPS_DEV_NO_POST) to the #if at line 1290, matching line 1223.
Referenced code: linuxkm/module_hooks.c:1291-1294 (4 lines)
This review was generated automatically by Fenrir. Findings are non-blocking.
c212bc1 to
107fc6d
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11031
Scan targets checked: linuxkm-bugs, linuxkm-src, wolfcrypt-bugs, wolfcrypt-port-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
Findings: 3
3 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
107fc6d to
976257d
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11031
Scan targets checked: linuxkm-bugs, linuxkm-src, wolfcrypt-bugs, wolfcrypt-port-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
Findings: 2
2 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
…avx and -mavx2 AX_APPEND_COMPILE_FLAGS. gcc emits AVX instructions unbidden under those flags (e.g. for 128-bit types) in code paths that cannot be runtime-dispatched on cpuid, leading to invalid-opcode crashes on CPUs lacking AVX (e.g. Westmere).
…VALUE_E "Supplied value was rejected by FIPS policy" and FIPS_UNAPPROVED_E "Requested operation succeeded, but supplied parameters are unapproved for FIPS". The first is a new fatal error, the second is a new nonfatal error to which WC_FIPS_NOT_APPROVED will be bound.
…SM when not already set, so settings.h can distinguish assembly translation units. wolfssl/wolfcrypt/settings.h: in the config-source selection, when BUILDING_WOLFSSL_ASM && WOLFSSL_USER_SETTINGS_ASM, include user_settings_asm.h (the assembly-safe, directives-only header produced by user_settings_asm.sh) instead of user_settings.h, which may contain C that breaks the assembler. wolfcrypt/src/aes_asm.S: drop the file's bespoke copy of the user_settings_asm.h selection block and route through libwolfssl_sources_asm.h, so the choice lives in one place. (aes_asm.S is the only .S that open-coded this.) Also add `#define WC_FIPS_LL_CRYPTO` immediately above the new include.
…s before including libwolfssl_sources.h, replacing the previous per-file `#define FIPS_NO_WRAPPERS` that was placed *after* the include and therefore never took effect. Low-level crypto TUs stop carrying a per-file opinion about wrapper generation; settings.h now derives FIPS_NO_WRAPPERS centrally. des3.c and wolfentropy.c newly acquire FIPS_NO_WRAPPERS (they had none). port/st/stm32.c converts to the same idiom (drops its HAVE_CONFIG_H/config.h + redundant types.h block). Note on scope: FIPS_NO_WRAPPERS governs how the *including* TU resolves its own outbound calls -- it does not change what the file makes available to others, beyond defining the FIPS-supported APIs without the `_fips()` extension the wrappers arrange. The pre-existing effect of the misplacement was that boundary-internal calls resolved to the wrapped forms, i.e. took an unintended round-trip back out through the wrappers. wolfssl/wolfcrypt/settings.h: derive FIPS_NO_WRAPPERS from (WC_FIPS_LL_CRYPTO || WOLFSSL_FIPS_DEV_NO_POST) under HAVE_FIPS, positioned after the config-source selection so it sees HAVE_FIPS regardless of whether it arrived via command line or user_settings.h.
…, MAJOR=8) and AM_CONDITIONAL BUILD_FIPS_NO_POST. Refactor FIPS dev/ready version setup: hoist ENABLED_FIPS_DEV / ENABLED_FIPS_READY to set -DWOLFSSL_FIPS_DEV / -DWOLFSSL_FIPS_READY centrally, and switch the FIPS AS_CASE arms from `test "$FIPS_VERSION" != "dev"` to `test "$ENABLED_FIPS_DEV" != "yes"` so the dev semantics extend to v5-dev/v6-dev/lean-aesgcm-dev. src/include.am: under !BUILD_FIPS_NO_POST, drop fips.c / fips_test.c / wolfcrypt_first.c / wolfcrypt_last.c from the build (dev-no-post uses no fips repo content). wolfssl/wolfcrypt/settings.h, wolfssl/wolfcrypt/wc_compat.h: under WOLFSSL_FIPS_DEV_NO_POST, squat WOLF_CRYPT_FIPS_H to inhibit fips.h, and change the FIPS_READY/DEV version block guard to `!defined(HAVE_FIPS_VERSION)` (required so an externally supplied version is not clobbered). wolfssl/wolfcrypt/fips_test.h: add WOLFSSL_FIPS_DEV_NO_POST stub block (fipsCastStatus_get, the PRIVATE_KEY macros) so master builds without the fips repo. linuxkm/linuxkm_wc_port.h, linuxkm/module_hooks.c: accommodate WOLFSSL_FIPS_DEV_NO_POST (guard verifyCore / CAST / fencepost paths that the fips repo would otherwise provide; force WC_USE_PIE_FENCEPOSTS_FOR_FIPS). wolfcrypt/test/test.c: in hmac_sha256_test(), don't expect HMAC_KAT_FIPS_E in WOLFSSL_FIPS_DEV_NO_POST builds. wolfssl/wolfcrypt/types.h: add stub macro for wolfCrypt_SetPrivateKeyReadEnable_fips() when WOLFSSL_FIPS_DEV_NO_POST.
…FIPS_DEV_NO_POST stubs.
…ure unconditional matching of PRIVATE_KEY_UNLOCK() regardless of intervening error code collection.
is_enabled, WC_DH_INITIAL_RUNTIME_ENABLEMENT, WC_DH_HAVE_RUNTIME_ENABLEMENT. Place the enablement check AFTER key->heap/trustedGroup init in the five entry points (wc_InitDhKey_ex, wc_DhGenerateKeyPair, wc_DhAgree, wc_DhAgree_ct, _DhSetKey) so a disabled-DH early return never leaves a half-initialized key for wc_FreeDhKey to mp_clear on garbage. configure.ac: add --enable-dh=conditional; when DH is enabled (directly or via all-crypto) set it initially usable under FIPS v7 with -DWC_DH_INITIAL_RUNTIME_ENABLEMENT=1; remove the FIPS-v7 DH force-off (in FIPS v7+, disable build by default, unless building in kernel mode with DH registration enabled). linuxkm/lkcapi_glue.c: bracket LKCAPI registration with `need_dh_disable = (wc_dh_enable() == 0)` ... `if (need_dh_disable) wc_dh_disable();`, so DH is disabled on every exit path, and only by the caller that actually enabled it (wc_dh_enable returns ALREADY_E if DH was already on, so this never disables a DH some other context legitimately enabled). tests/unit.c, wolfcrypt/test/test.c: bracket the DH tests with enable/disable so they succeed regardless of runtime initial default enablement.
…GcmEncrypt() remapping if needed.
…12) with
FIPS_BAD_VALUE_E in the GCM IV-construction paths under HAVE_FIPS, overridable
by WC_FIPS_AESGCM_ALLOW_SHORT_NONCES. Decrypt accepts any supported length per
SP 800-38D (IV construction requirements bind encryption only).
The floor takes two shapes, both correct by construction:
- wc_AesGcmInit_local (reached from wc_AesGcmInit and
wc_AesGcmEncryptInit_ex, which pass a decrypt_p flag):
(ret == 0) && (! decrypt_p) && (ivSz > 0) && (ivSz < GCM_NONCE_MID_SZ)
The `ivSz > 0` clause is load-bearing -- iv is an optional argument there,
and the key-only re-init form (iv == NULL, ivSz == 0) used for
module-generated-IV streaming must still pass.
- wc_AesGcmSetIV / wc_AesGcmSetExtIV: bare `ivSz < GCM_NONCE_MID_SZ`, no
ivSz>0 clause needed -- CheckAesGcmIvSize() already admits only {8,12,16},
so ivSz == 0 cannot reach the floor.
tests/api/test_aes.c: update tests for new FIPS nonce size restrictions.
.wolfssl_known_macro_extras: add WC_FIPS_AESGCM_ALLOW_SHORT_NONCES.
wolfcrypt/src/cmac.c: on _InitCmac_common failure free the Aes and set cmac->type = WC_CMAC_NONE, so a contract-violating wc_CmacFree on a never-initialized object hits a no-op arm instead of re-entering wc_AesFree; add explicit case WC_CMAC_NONE arms to the three type switches (wc_CmacUpdate, wc_CmacFree, wc_CmacFinalNoFree).
(unversioned defined(HAVE_FIPS), not a version arm -- hmac.c is in-boundary and master's copy only compiles at v7+/MAJOR=8). This structurally closes the old-TLS MD5 PRF: wc_PRF_TLSv1 -> wc_PRF(md5_mac) -> wc_HmacSetKey(WC_MD5) -> BAD_FUNC_ARG. Separately, in wc_HKDF_Expand_ex, add `else if (ret == 0) return BAD_FUNC_ARG;` after the wc_HmacSizeByType call: the existing code guarded ret < 0 but not ret == 0, and hashSz is the divisor in the `outSz/hashSz + ((outSz % hashSz) != 0) > 255` check three lines below. wolfcrypt/src/kdf.c: delete the two WC_HASH_TYPE_MD5_SHA guards in wc_PRF / wc_PRF_TLS -- they were a domain error (that arg is wc_MACAlgorithm, where WC_HASH_TYPE_MD5_SHA == 9 == sm3_mac, so the guard blocked SM3, not MD5-SHA), and the hmac.c reject is the correct layer. wolfcrypt/src/evp.c: drop the MD5 EVP mapping at FIPS >= 5 (evp.c is out of boundary, so the version arm is live and correct here). tests/api/test_kdf.c: derive secLen from MAX_PRF_HALF rather than hardcoding 521/261 -- MAX_PRF_HALF is config-dependent (516 under HAVE_FFDHE_8192, 388 under FFDHE_6144, else 260), so the hardcoded value made the BUFFER_E expectation config-dependent.
* in AllocKey capture and propagate the return of the per-type wc_*_init_ex calls (ed25519, ed448, falcon, ML-DSA, ML-KEM, ...) instead of discarding it, so an init failure surfaces rather than leaving a partially-constructed key for later use. * The `default:` arm of the type switch now sets `ret = BAD_FUNC_ARG` and breaks, instead of returning directly, so it reaches the common cleanup. * The failure cleanup distinguishes the two states: if the key was initialized, FreeKey(); otherwise XFREE(*pKey) and NULL the caller's pointer -- previously an allocation that failed before init leaked. * Two mis-copied #endif comments corrected: HAVE_CURVE25519 -> HAVE_ED25519 and HAVE_CURVE448 -> HAVE_ED448.
…EN(name) ->
`.hidden name` macro under __ELF__, currently empty under !__ELF__.
wolfcrypt/src/*.S, wolfcrypt/src/port/{arm,ppc32,ppc64,riscv64}/*.S: emit
WC_ASM_ATT_HIDDEN for internal ATT-syntax symbols so they don't become
dynamic-table entries or otherwise pollute symbol namespace beyond the
library/module.
…R_SPEEDUPS definition (any asm speedup present, minus WOLFSSL_NO_ASM) from types.h to settings.h, and add `#error WC_C_DYNAMIC_FALLBACK requires WC_HAVE_VECTOR_SPEEDUPS` so a fallback-to-C build with no asm to fall back from is rejected at configure time rather than mis-building.
wolfcrypt/src/rsa.c: wrap wc_hash2mgf in a targeted -Wswitch-enum pragma configure.ac: rename $ENABLE_ORIGINAL -> $ENABLE_ORIGINAL_KYBER to disambiguate. wolfssl/wolfcrypt/types.h: tighten a braced-group guard with __STRICT_ANSI__ (pedantic-mode correctness). wolfssl/internal.h: add the WOLFSSL_API_PREFIX_MAP mapping for TLSX_CKS_Parse.
Refactor WC_C_DYNAMIC_FALLBACK architecture to allow per-call alternation between asm and C: * Under WC_C_DYNAMIC_FALLBACK keep the block buffer as the raw big-endian stream and byte-reverse just-in-time inside the C transform, so a given wc_Sha256 / wc_Sha512 instance may switch between the vectorized and pure-C transforms per call without producing a wrong digest. * Add Transform_*_C_from_raw / Transform_*_Len_C_from_raw JIT-reversing adapters; the dispatchers early-out through them on (method == C) || SAVE_VECTOR_REGISTERS2 failure; caller-side method-keyed ByteReverse sites are compiled out under the raw-buffer convention and the final block's length words are written unconditionally big-endian. * Remove the init-time CAN_SAVE_VECTOR_REGISTERS pin from Sha*_SetTransform so the recorded method reflects pure CPU capability (enabling fall-forward, not only fallback). Update the bulk paths to check the transform return and not advance on failure. * The raw-buffer convention is scoped to WC_C_DYNAMIC_FALLBACK specifically -- not to WC_NO_INTERNAL_FUNCTION_POINTERS -- because only the fallback build can change transform mid-object; a plain no-function-pointers build picks one method and keeps it, so it retains the conventional host-endian buffer (no change from incumbent code). * Drop the per-object `.sha_method` member. Method selection is a property of the CPU, not of the hash object, so it becomes a file-scope static in each .c, set once (Sha*_SetTransform() early- returns when already set) and read by every instance. Shrinks both structs. * When WOLFSSL_AESNI is enabled without the rest of USE_INTEL_SPEEDUP, `#undef WC_C_DYNAMIC_FALLBACK` -- AES-NI alone satisfies WC_HAVE_VECTOR_SPEEDUPS but leaves SHA with no vectorized transform to fall back from, and the caller-side gating would otherwise suppress a byte-reversal that is still required.
WOLFSSL_DEBUG_TRACE_ERROR_CODES support for internal DRBG errors. Converts the DRBG internal status #defines (DRBG_SUCCESS/DRBG_FAILURE, WC_DRBG_*) to enums, that are wrapped in WC_ERR_TRACE() when WOLFSSL_DEBUG_TRACE_ERROR_CODES. Deploys well-known error codes and WC_NO_ERR_TRACE() as needed throughout.
…i_rsa_glue.c, linuxkm/lkcapi_sha_glue.c, wolfcrypt/test/test.c: * Use defined(WC_HAVE_RNG_BANKREF), not defined(WC_RNG_BANK_SUPPORT), as the feature sensor for RNG bankrefs. * Add DRBG_KAT_FIPS_E and DRBG_CONT_FIPS_E to the list of immediate-failure errors in wc_rng_bank_init().
…sm}:
The AVX2 constant-time table-lookup routines seed a broadcast vector with a
legacy-SSE GPR->XMM move (movd/movq), which writes bits [127:0] and leaves
[255:128] UNMODIFIED, then read the register at full YMM width via
vpermd %ymm,%ymm(zeroed),%ymm (a lane-0 broadcast across all 256 bits). If a
prior vector op left the upper lane non-zero, the broadcast is corrupt and the
constant-time selection returns the wrong table entry -- a wrong ECC point/entry
in sp_{256,384,521}_get_{point_33,entry_64,entry_65}_avx2 and
sp_{2048,3072,4096}_get_from_table_avx2, or a wrong X25519 public key from
fe_cmov_table_avx2. Deterministic given register history; surfaces as
intermittent failures because it depends on the upper lane being dirty on entry.
Under kernel_fpu_begin (which does not zero YMM) a dirty upper lane is ambient,
which is why ED25519 asm was kept disabled in kernel mode. Fix: emit the VEX
form (vmovd for 32-bit source, vmovq for 64-bit), which zeroes [255:128].
…, wolfcrypt/test/test.h, wolfcrypt/test/test.c: Falcon fixes and cleanups: wolfcrypt/src/falcon.c: * define _WC_BUILDING_FALCON_C; * add #error arch guards -- WOLFSSL_FALCON_FPR_ASM requires x86-64 (wc_falcon_fpr_x86_64_asm.S), WOLFSSL_FALCON_FFT_AVX2 requires an x86 target, WOLFSSL_FALCON_FFT_NEON requires AArch64 (ARM32 NEON lacks the double-precision lanes used by float64x2_t); * drop the unused FALCON_PRIMES forward declaration; * fix u->U literals and an (sword64)z0*(sword64)z0 overflow; * tightened __uint128_t guard. wolfcrypt/benchmark/benchmark.c: * falcon message buffer -> WC_ALLOC_VAR; * bench wiring for the experimental algs. wolfcrypt/test/test.h: add frodokem_test / falcon_test externs. wolfcrypt/test/test.c: mldsa/falcon WC_ALLOC_VAR conversions. configure.ac: update all-quantum-crypto with the experimental algorithms.
…_func() /
err_sys_with_errno_func() taking __FILE__ and __LINE__, with function-like
macros preserving every existing call site; failure messages now carry file and
line ("wolfSSL error, %s L %d: %s").
tests/suites.c: client_test / server_test failure reports go to stderr
(printf -> fprintf(stderr, ...)).
…tions: Under WC_C_DYNAMIC_FALLBACK, SAVE_VECTOR_REGISTERS2() can fail on any call, so two calls on the same object can dispatch differently. Each of these algorithms had state that silently assumed a single dispatch for its lifetime. wolfcrypt/src/wc_mldsa.c: add MLDSA_NTT_AVX2()/MLDSA_INVNTT_AVX2() selecting the "full" AVX2 NTT/invNTT under WC_C_DYNAMIC_FALLBACK. The non-full variants leave NTT-domain coefficients in a permuted, lane-interleaved order that only their matching consumers understand, whereas the full variants and the C implementations use standard order. NTT-domain data at rest (cached s1/s2/t0 vectors, the challenge polynomial) can be produced and consumed by differently-dispatched calls, so its representation must be dispatch-invariant. Without fallback, dispatch is invariant and the ~2%/~4% faster permuted-order variants are kept. Both pipelines are bit-identical end to end. wolfcrypt/src/wc_mlkem_poly.c: in mlkem_derive_secret(), re-initialize the shared SHAKE-256 object under WC_C_DYNAMIC_FALLBACK. The buffer-stuffing shortcut assumes a freshly initialized (zeroed) sponge, which no longer holds once the C fallback legs of mlkem_gen_matrix()/mlkem_get_noise() drive the XOF on that object and leave it mid-squeeze. wolfcrypt/src/wc_slhdsa.c: in slhdsakey_fors_sign(), replace the CAN_SAVE_VECTOR_REGISTERS() test with an actual SAVE_VECTOR_REGISTERS2() == 0 acquisition and a matching RESTORE_VECTOR_REGISTERS(), so the region is held rather than merely predicted to be available. wolfcrypt/src/wc_frodokem_mat.c: in the AES row kernels of frodokem_mul_add_as_plus_e_aes() and frodokem_mul_add_sa_plus_e_aes(), re-key with wc_AesSetKeyDirect() when IS_INTEL_AESNI() but !aes->use_aesni. The kernels consume aes->key directly, which holds an AES-NI-layout schedule only if SetKey ran with vector registers available; under fallback a failed SAVE_VECTOR_REGISTERS2() inside SetKey returns success having keyed only the C-fallback schedule. Re-keying happens inside the held region, where the nested save always succeeds. Loop conditions gain (ret == 0) so a re-key failure stops the run. wolfssl/wolfcrypt/settings.h: with the above, ML-KEM, ML-DSA, SLH-DSA and FrodoKEM are fuzzer-clean, so the DEBUG_VECTOR_REGISTER_ACCESS_FUZZING exclusion narrows from the _WC_BUILDING_WC_MLKEM_POLY_C / _WC_BUILDING_WC_MLDSA_C / _WC_BUILDING_WC_SLHDSA_C set to _WC_BUILDING_FALCON_C alone. Falcon stays excluded because it uses FP or vector registers in all of its asm implementations and there is no option yet to build the C-no-FP implementation alongside them. tests/api/test_mldsa.c: in test_mldsa_encode_w1_large_values(), pin dispatch to the C path with WC_DEBUG_SET_VECTOR_REGISTERS_RETVAL() for the duration of the test and restore it afterward. The two calls being compared are only specified -- and only equal -- on the valid input domain, so letting the fuzzer send them down different (AVX2 vs C) implementations is not a meaningful comparison.
MAYBE_INHIBIT vector-register save mode for the convenience of the DRBG glue
logic.
Add WC_SVR_FLAG_MAYBE_INHIBIT = 2 (WC_SVR_FLAG_FUZZ becomes 4), and add
SAVE_VECTOR_REGISTERS_MAYBE_INHIBIT() / RESTORE_VECTOR_REGISTERS_MAYBE_INHIBITED()
macros, routing through wc_{save,restore}_vector_registers_x86(WC_SVR_FLAG_MAYBE_INHIBIT).
linuxkm/x86_vector_register_glue.c: implement the mode. A _MAYBE_INHIBIT call
must be outermost (BAD_STATE_E otherwise, and a warning on the matching restore
at non-outermost depth). The fuzzer check moves ahead of the inhibit decision
so that a fuzzed failure converts into an inhibited (rather than failed)
acquisition when _MAYBE_INHIBIT is set; a real inability to use SIMD
(preempt_count() != 0 && !may_use_simd()) does the same. The restore path
treats _MAYBE_INHIBIT like _INHIBIT when deciding whether registers were
actually taken.
Note, under fuzzer vector disablement,
wc_save_vector_registers_x86(WC_SVR_FLAG_MAYBE_INHIBIT) returns 0 with affinity
locked, as though explicit WC_SVR_FLAG_INHIBIT were passed. Thus on 0 return,
the caller must use `CAN_SAVE_VECTOR_REGISTERS()` to discover whether vector
registers are actually usable.
… SAVE_VECTOR_REGISTERS_MAYBE_INHIBIT() and RESTORE_VECTOR_REGISTERS_MAYBE_INHIBITED(), and in wc_linuxkm_rng_bank_init(), disable vector ops only if FIPS <v7.
…tement-expr definitions with __extension__ at both arms (the fuzzing arm and the plain arm) so -pedantic builds (which the kernel-settings CI configs use) don't drown in "ISO C forbids braced-groups within expressions" across the hundreds of expansion sites.
…nable-fips=dev-no-post. Notes: * EXTRA_CPPFLAGS carries -Werror in the base configure line, because the autotools -Werror is gated on a VCS checkout being detected in the configure CWD and these are VPATH builds, which would otherwise silently get no -Werror at all. * The fuzzing cell's WC_DEBUG_VECTOR_REGISTERS_FUZZING_SEED is $$, which changes from run to run. * The two out-of-tree kernel-module cells (linuxkm, linuxkm-all-asm) are commented out, with a note that out-of-tree module builds don't work yet.
…PTO_FIPS) != defined(HAVE_FIPS) more informative.
…iables exported to the linuxkm sub-make. linuxkm/Kbuild: * Feature-detect $(intcmp) (GNU make >= 4.4) into $(HAVE_INTCMP). * Derive FIPS_OPTEST_NO_DH from $(HAVE_FIPS_VERSION_MAJOR) via $(intcmp) when available; otherwise $(error) with instructions to supply it on the make command line. When set, build the optest wrapper with -DNO_DH -- DH is not optested at FIPS v7+ even when the module has DH, because its APIs have no FIPS wrappers. * Same $(intcmp)/$(error) treatment for NO_PIE_FLAG (target kernel < 5.11), and change its test from `ifdef NO_PIE_FLAG` to an explicit `ifeq "$(NO_PIE_FLAG)" "0"` so an explicitly-zero value means what it says. * Add $(CFLAGS_AUTO_VECTORIZE_DISABLE) to benchmark.o ccflags-y and drop its unused asflags-y line.
…FIPS || FIPS >= v7.
…IPS_DEV from the gate for implicit defining of WC_TEST_NO_ECC_SIGN_VERIFY_ZERO_DIGEST.
…SL_DEBUG_TRACE_ERROR_CODES_SUPPORT, facilitating opt-in per-file error tracing in debug campaigns.
…() with xorshift64 (Marsaglia 2003).
…REGISTER_ACCESS configurations.
…a_param_*_vfy_test(), test_mldsa_decode_level(), and mldsa_test().
…pt/src/sha3_asm.asm, wolfcrypt/src/wc_mlkem_asm.asm: synchronize with current generator scripts.
547791b to
e42afa6
Compare
|
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: 33 total — 24 posted, 10 skipped
Posted findings
- [Critical] Infinite recursion in Transform_Sha512() C-fallback path — self-call replaced _Transform_Sha512() —
wolfcrypt/src/sha512.c:1287-1288 - [High] WC_C_DYNAMIC_FALLBACK raw-buffer gating in sha512.c is not scoped to the x86-64/AVX backend, producing wrong digests —
wolfcrypt/src/sha512.c:2013,2248,2304,2610 - [High] Removing the per-context sha_method field leaves two dangling references in tests/swdev/swdev.c —
tests/swdev/swdev.c:352,448 - [High] wc_AesGcmInit() hardcodes encrypt mode, so the FIPS short-nonce rejection breaks AES-GCM decryption via the public streaming API and the EVP compat layer —
wolfcrypt/src/aes.c:14078-14082, 13992-14002 - [High] wc_InitDhKey_ex() returns FIPS_NOT_ALLOWED_E leaving the mp_ints uninitialized, and src/tls.c then frees them —
wolfcrypt/src/dh.c:966-973 - [High] --enable-fips=v5-kcapi silently loses every "-dev" feature override —
configure.ac:657-664 - [High] FIPS_DEVREADY_MAJOR=7 contradicts commit 606415c's stated MAJOR=8, and makes ready/dev/dev-no-post indistinguishable from v7 —
configure.ac:695-704 - [Medium] kernel-settings "-dh" reset leaves enable_dh set-but-empty, defeating AC_ARG_ENABLE's default and building DH when it should be excluded —
configure.ac:1886-1891 - [Medium] WC_DH_INITIAL_RUNTIME_ENABLEMENT=1 only triggers on an exact enable_dh=yes, so --enable-dh=nonblock and --enable-openssh build permanently-disabled DH —
configure.ac:6362-6369, 1744 - [Medium] bench_falconKeySign() never checks WC_ALLOC_VAR results, NULL-dereferencing msg on allocation failure —
wolfcrypt/benchmark/benchmark.c:15806-15847 - [Medium] mldsa_param_{44,65,87}_vfy_test() double-encode an already WC_TEST_RET_ENC-encoded return value —
wolfcrypt/test/test.c:56147-56160, 57149-57162, 58510-58523 - [Medium] random.c: two DRBG_FAILURE comparisons were not converted to WC_NO_ERR_TRACE —
wolfcrypt/src/random.c:816, 1014 - [Medium] wc_dh_enable()/wc_dh_disable() are a non-atomic read-modify-write on a plain volatile int —
wolfcrypt/src/dh.c:60-81 - [Medium] wc_DhGeneratePublic() and wc_DhGenerateParams() are not covered by the DH enablement check —
wolfcrypt/src/dh.c - [Medium] linuxkm_affinity_lock can now fail with INTERRUPTED_E / WC_ACCEL_INHIBIT_E, which wc_rng_bank_checkout treats as fatal —
linuxkm/lkcapi_sha_glue.c:2068-2094 - [Low] FIPS_UNAPPROVED_E error string is missing a space at the concatenation boundary —
wolfcrypt/src/error.c:746-748 - [Low] wc_frodokem_mat.c re-key blocks read aes->use_aesni, which only exists under WOLFSSL_AESNI —
wolfcrypt/src/wc_frodokem_mat.c:1732-1742, 2054-2064 - [Low] falcon.c: wc_MemZero_Add registers sizeof(pointer) instead of sizeof(falcon_sampler_ctx) —
wolfcrypt/src/falcon.c:8313-8314 - [Low] configure --help prints the literal string ${ENABLED_DH_DEFAULT} for --enable-dh —
configure.ac:6379 - [Low] fips-dev-no-post.yml: the fuzzing seed is the shell PID and is never echoed, and the adjacent comment about '$' in the heredoc is now wrong —
.github/workflows/fips-dev-no-post.yml:87-89,116 - [Low] sha256.c: #undef WC_C_DYNAMIC_FALLBACK precedes the header includes, so this TU can see a different struct layout —
wolfcrypt/src/sha256.c:46-53, wolfcrypt/src/sha512.c:68-76 - [Low] tests/api/test_cmac.c drops KCAPI coverage without explanation —
tests/api/test_cmac.c:374-378, 421-425 - [Low] tests/unit.c uses wc_dh_enable()/WC_DH_HAVE_RUNTIME_ENABLEMENT without including dh.h —
tests/unit.c:82-84 - [Info] falcon.c: FALCON_MULHI now additionally requires HAVE___UINT128_T, silently dropping to the 4-multiply fallback —
wolfcrypt/src/falcon.c:1152-1156
Skipped findings
- [High] --enable-fips=v7 no longer defines WOLFSSL_FIPS_READY, flipping the build from FIPS 186-4 to 186-5
- [High] AES-GCM short-nonce rejection is gated on HAVE_FIPS with no version qualifier, changing behavior of shipped FIPS v5/v6 modules and breaking their tests
- [High] Out of scope: 24 cicd findings on CI files this PR does not touch
- [Medium] Already answered by the commit messages -- intentional, with stated rationale
- [Medium] test_wc_AesGcmDecisionCoverage / test_wc_AesGcmArgMcdc expectations do not match the implementation's FIPS gating
- [Medium] FIPS optest -DNO_DH rule names a target kbuild never builds, so the flag is silently dropped
- [Low] settings.h dropped the #undefs before defining HAVE_FIPS_VERSION_MAJOR/_MINOR/_PATCH
- [Low] x86_vector_register_glue.c: new fuzzing code has a misindented unbraced warning and assigns an out-of-enumeration value to the enum-typed flags parameter
- [Info] falcon.c: WC_DECLARE_VAR heap argument reads as a dereference before the NULL check
- [Medium] wc_DhGeneratePublic() and wc_DhGenerateParams() are not covered by the DH enablement check
Review generated by Skoll via Claude/Codex
| int ret; | ||
| if (SHA_METHOD == SHA512_C) | ||
| return _Transform_Sha512(sha512); | ||
| if (sha_method == SHA512_C) |
There was a problem hiding this comment.
🔴 [Critical] Infinite recursion in Transform_Sha512() C-fallback path — self-call replaced _Transform_Sha512()
🚫 BLOCK bug
The refactor that removed the SHA_METHOD macro rewrote the non-WC_C_DYNAMIC_FALLBACK early-exit in Transform_Sha512() and changed the callee from _Transform_Sha512(sha512) (the C implementation) to Transform_Sha512(sha512) (the enclosing function itself). sha_method is a file-static that is unchanged on re-entry, so the branch is taken again immediately: unbounded self-recursion. Verified against the merge-base — git show 5b22fa901e81:wolfcrypt/src/sha512.c shows return _Transform_Sha512(sha512); at the corresponding line. Because it is a tail call, at -O2 GCC/clang turn it into an infinite loop (hang, no crash); at -O0 it is a stack overflow. Reachable whenever WC_NO_INTERNAL_FUNCTION_POINTERS is set WITHOUT WC_C_DYNAMIC_FALLBACK (settings.h auto-defines the former under WC_SYM_RELOC_TABLES) on x86-64 with USE_INTEL_SPEEDUP, running on a CPU or VM where neither AVX1 nor AVX2 is present — Sha256_SetTransform's SHA-512 twin then selects SHA512_C and every SHA-512/384 call hangs. The equivalent conversion in sha256.c (inline_XTRANSFORM, line 573-574) was done correctly, which confirms this is an isolated slip rather than intent.
The rework lost the leading underscore on the C-transform call. The #else (non-WC_C_DYNAMIC_FALLBACK) arm of Transform_Sha512() now calls Transform_Sha512() — itself — instead of _Transform_Sha512(). The diff hunk shows the regression directly: - return _Transform_Sha512(sha512); / + return Transform_Sha512(sha512);. This is reached whenever WC_NO_INTERNAL_FUNCTION_POINTERS is defined without WC_C_DYNAMIC_FALLBACK (settings.h:4381 sets it from WC_SYM_RELOC_TABLES, i.e. the Linux kernel-module PIE build; users can also set it directly), the x86-64 + USE_INTEL_SPEEDUP + AVX dispatcher is compiled, and the runtime CPU has neither AVX1 nor AVX2 so Sha512_SetTransform() selects SHA512_C. Because the call is in tail position, -O2 will convert it to a jump and produce an unbounded loop; at lower optimization it is a real recursive call and exhausts the stack. In kernel context either outcome is a soft lockup or panic on the first SHA-512/384 operation.
Commit-message cross-check. 58815c9 documents the whole WC_C_DYNAMIC_FALLBACK refactor in detail and never mentions changing this call. The diff rewrites return _Transform_Sha512(sha512); into return Transform_Sha512(sha512); -- an unintended edit, not a design choice.
Suggestion:
| if (sha_method == SHA512_C) | |
| #else | |
| if (sha_method == SHA512_C) | |
| return _Transform_Sha512(sha512); | |
| SAVE_VECTOR_REGISTERS(return _svr_ret;); | |
| #endif |
Recommendation: Restore the _Transform_Sha512() call. Add a CI config that exercises WC_NO_INTERNAL_FUNCTION_POINTERS on a non-AVX host (or force sha_method = SHA512_C) so this path is covered — no existing job hits it, which is why the hang went unnoticed.
[wolfcrypt/src/sha512.c:1278-1290] Restore the underscore: return _Transform_Sha512(sha512);. Consider adding a pre-AVX x86-64 CI leg (or a WC_NO_INTERNAL_FUNCTION_POINTERS build with AVX masked off) so the SHA512_C dispatch arm is actually exercised.
|
|
||
| if (sha512->buffLen == WC_SHA512_BLOCK_SIZE) { | ||
| #if defined(LITTLE_ENDIAN_ORDER) | ||
| #if defined(LITTLE_ENDIAN_ORDER) && !defined(WC_C_DYNAMIC_FALLBACK) && \ |
There was a problem hiding this comment.
🟠 [High] WC_C_DYNAMIC_FALLBACK raw-buffer gating in sha512.c is not scoped to the x86-64/AVX backend, producing wrong digests
🚫 BLOCK bug
The new raw-big-endian buffer convention requires that, when WC_C_DYNAMIC_FALLBACK is set, the unconditional ByteReverseWords64() calls in Sha512Update(), Sha512Final() (padding and length words) and wc_Sha512Transform() be suppressed, because the compensating just-in-time reversal now lives in Transform_Sha512_C_from_raw(). The diff added && !defined(WC_C_DYNAMIC_FALLBACK) at the OUTER guard of all four sites — but Transform_Sha512_C_from_raw() is only compiled inside the #if defined(WOLFSSL_X86_64_BUILD) && defined(USE_INTEL_SPEEDUP) && (HAVE_INTEL_AVX1||HAVE_INTEL_AVX2) section. In any configuration where WC_C_DYNAMIC_FALLBACK is defined but that section is NOT compiled, Transform_Sha512() collapses to plain _Transform_Sha512(), which reads host-endian words, while the buffer now holds raw big-endian bytes that are never reversed. Result: silently incorrect SHA-512/SHA-384 digests with no error returned. Two concrete reachable configs: (1) a user_settings.h that sets WC_C_DYNAMIC_FALLBACK together with WOLFSSL_SP_ASM only — the new auto-#undef added at sha512.c:68-76 only tests WOLFSSL_AESNI, so it does not fire; (2) 32-bit x86 --enable-intelasm, where WOLFSSL_X86_64_BUILD is false. sha256.c does NOT have this defect: its SHA256_UPDATE_REV_BYTES switch is correctly nested inside the x86/AVX #if, so the asymmetry between the two files is itself the tell.
The rework adopts the "sha512->buffer always holds the raw big-endian stream" convention by adding && !defined(WC_C_DYNAMIC_FALLBACK) to four ByteReverseWords64() guards (sha512.c:2013 partial-block completion, :2248 Final extra padding block, :2304 Final pre-length-store, :2610 wc_Sha512Transform). The just-in-time replacement Transform_Sha512_C_from_raw() is defined only at sha512.c:1191-1229, inside #if (WOLFSSL_X86_64_BUILD && USE_INTEL_SPEEDUP && (AVX1||AVX2)) || WOLFSSL_ARMASM. But WC_C_DYNAMIC_FALLBACK is a global setting whose only prerequisite is the new WC_HAVE_VECTOR_SPEEDUPS (settings.h:5941-5951), satisfied by USE_INTEL_SPEEDUP, USE_INTEL_SPEEDUP_FOR_AES, WOLFSSL_AESNI, WOLFSSL_ARMASM or WOLFSSL_SP_ASM. When WC_C_DYNAMIC_FALLBACK is set but that dispatcher is absent, Transform_Sha512 degenerates to _Transform_Sha512 (which reads host-endian word64s) while the reversal has been deleted — wrong digests, silently. The inconsistency is visible within a single build: the bulk-block loop's #else arm (sha512.c:2120-2135) still reverses unconditionally, so only the final/partial block is corrupted. Concrete reachable path: configure.ac:1206-1211 defines WOLFSSL_X86_64_BUILD only when (host_cpu = x86_64|amd64) && ENABLED_32BIT != yes, while configure.ac:4724-4730 defines WC_C_DYNAMIC_FALLBACK for any ENABLED_AESNI|ENABLED_INTELASM + KERNEL_MODE_DEFAULTS — so --enable-kernel-settings --enable-intelasm --enable-32bit (or an i686 host) yields WC_C_DYNAMIC_FALLBACK + USE_INTEL_SPEEDUP with no WOLFSSL_X86_64_BUILD, and the new #undef at sha512.c:68-75 does not fire because it requires !defined(USE_INTEL_SPEEDUP). A user_settings.h pairing WC_C_DYNAMIC_FALLBACK with WOLFSSL_SP_ASM hits the same hole. Note sha256.c got this right: its equivalent switch (SHA256_UPDATE_REV_BYTES, sha256.c:242-260) is nested inside the x86-64/AVX gate so other backends fall through to the correct arm; sha512.c has no such scoping.
Commit-message cross-check. 58815c9 identifies exactly this hazard and carves out one case: "When WOLFSSL_AESNI is enabled without the rest of USE_INTEL_SPEEDUP, #undef WC_C_DYNAMIC_FALLBACK -- AES-NI alone satisfies WC_HAVE_VECTOR_SPEEDUPS but leaves SHA with no vectorized transform to fall back from." But settings.h:5941 sets WC_HAVE_VECTOR_SPEEDUPS from four macros -- USE_INTEL_SPEEDUP, USE_INTEL_SPEEDUP_FOR_AES, WOLFSSL_AESNI, WOLFSSL_ARMASM (plus WOLFSSL_SP_ASM) -- and the sha256.c/sha512.c carve-out names only WOLFSSL_AESNI. A WOLFSSL_SP_ASM-only or USE_INTEL_SPEEDUP_FOR_AES-only build passes the new #error WC_C_DYNAMIC_FALLBACK requires WC_HAVE_VECTOR_SPEEDUPS check, keeps WC_C_DYNAMIC_FALLBACK, suppresses the byte-reversal, and has no from_raw adapter. The reasoning is right; the carve-out is one macro wide.
Suggestion: Introduce a single backend-scoped macro, mirroring sha256.c's SHA256_UPDATE_REV_BYTES, and gate on it instead of on WC_C_DYNAMIC_FALLBACK directly:
#if defined(WOLFSSL_X86_64_BUILD) && defined(USE_INTEL_SPEEDUP) &&
(defined(HAVE_INTEL_AVX1) || defined(HAVE_INTEL_AVX2)) &&
defined(WC_C_DYNAMIC_FALLBACK)
#define SHA512_RAW_BUFFER_CONVENTION
#endif
/* then use !defined(SHA512_RAW_BUFFER_CONVENTION) at the four sites */
Recommendation: Scope the four !defined(WC_C_DYNAMIC_FALLBACK) conditions to the same #if that compiles Transform_Sha512_C_from_raw(), exactly as sha256.c does. Add a KAT run under WC_C_DYNAMIC_FALLBACK without USE_INTEL_SPEEDUP — no current CI job covers that combination.
[wolfcrypt/src/sha512.c:2013-2027] Gate the raw-buffer convention on the backend that implements it. Define one local switch next to the Intel dispatcher — #if defined(WC_C_DYNAMIC_FALLBACK) && defined(WOLFSSL_X86_64_BUILD) && defined(USE_INTEL_SPEEDUP) && (defined(HAVE_INTEL_AVX1) || defined(HAVE_INTEL_AVX2)) -> #define SHA512_RAW_BUFFER_CONVENTION — and use !defined(SHA512_RAW_BUFFER_CONVENTION) at lines 2013, 2248, 2304 and 2610. That also makes the sha512.c:68-75 #undef unnecessary.
| #ifdef WC_C_DYNAMIC_FALLBACK | ||
| int sha_method; | ||
| #endif | ||
| #ifdef WOLFSSL_ASYNC_CRYPT |
There was a problem hiding this comment.
🟠 [High] Removing the per-context sha_method field leaves two dangling references in tests/swdev/swdev.c
🚫 BLOCK bug
58815c9e7 deletes the #ifdef WC_C_DYNAMIC_FALLBACK int sha_method; member from struct wc_Sha256 (wolfssl/wolfcrypt/sha256.h) and struct wc_Sha512 (wolfssl/wolfcrypt/sha512.h) -- deliberately, per the commit message: "Method selection is a property of the CPU, not of the hash object." But tests/swdev/swdev.c still copies the field at lines 352 and 448, under exactly the WC_C_DYNAMIC_FALLBACK guard the member used to live under. Both sites now fail with "no member named 'sha_method'".
This is not a stray file: tests/include.am:29 links tests/swdev/build/swdev.o into unit.test, and tests/swdev/user_settings.h pulls in wolfssl/options.h, so swdev.c compiles against the tree's own headers. Any --enable-swdev build that also carries WC_C_DYNAMIC_FALLBACK fails to compile.
Scope note. wolfcrypt/test/test.c:25402,25408 reads the same field, but this diff gated that block behind defined(HAVE_FIPS) && FIPS_VERSION3_LT(7,0,0), and FIPS v5/v6 builds overlay the certified wolfCrypt sources and their headers, which still carry sha_method. That site is fine and needs no change -- only swdev.c does.
One knock-on worth a thought: that test.c block was the only in-tree assertion that WC_RNG_BANK_FLAG_NO_VECTOR_OPS really pinned the DRBG's SHA contexts to the C transform. Now that it is old-FIPS-only and sha_method is a file-scope static, the property is unobservable from a context in v7+/non-FIPS builds, so it goes untested there.
Suggestion: Delete both #ifdef WC_C_DYNAMIC_FALLBACK dst->sha_method = src->sha_method; #endif blocks from tests/swdev/swdev.c — with the file-static sha_method there is no per-context state left to copy.
Recommendation: Delete the two #ifdef WC_C_DYNAMIC_FALLBACK sha_method copy blocks in tests/swdev/swdev.c (there is no per-context state left to copy). Optionally re-express the NO_VECTOR_OPS assertion against something still observable so it keeps coverage outside old-FIPS builds.
| return ret; | ||
| } | ||
|
|
||
| int wc_AesGcmInit(Aes* aes, const byte* key, word32 len, const byte* iv, |
There was a problem hiding this comment.
🟠 [High] wc_AesGcmInit() hardcodes encrypt mode, so the FIPS short-nonce rejection breaks AES-GCM decryption via the public streaming API and the EVP compat layer
🚫 BLOCK bug
The new decrypt_p parameter exists precisely to exempt decryption from the short-nonce rule — the in-code comment says "there is no FIPS check on ivSz in decrypt mode — SP 800-38D IV construction requirements bind encryption only; decryption must accept externally generated IVs of any supported length." But the public wc_AesGcmInit() wrapper passes 0 (encrypt) unconditionally, and that entry point is documented and used for BOTH directions. doc/dox_comments/header_files/aes.h:2554 describes wc_AesGcmDecryptInit as "a convenience wrapper around wc_AesGcmInit for decryption", and wolfSSL's own OpenSSL-compat layer calls wc_AesGcmInit() direction-agnostically at wolfcrypt/src/evp.c:1388, :7124 and :7225. Since an application can set an 8-byte IV through EVP_CTRL_GCM_SET_IVLEN, a FIPS build now fails AES-GCM decryption with FIPS_BAD_VALUE_E — exactly the case the carve-out was written to permit. evp.c:7113 compounds this by calling wc_AesGcmSetExtIV() unconditionally for both directions, and that function's new check has no direction parameter at all. Net effect: FIPS builds can no longer decrypt AES-GCM data produced by a peer using a short nonce, which is a data-availability regression, not just a policy tightening.
The new short-nonce gate in wc_AesGcmInit_local() is deliberately suppressed for decryption ((! decrypt_p)), with the comment stating "decryption must accept externally generated IVs of any supported length". wc_AesGcmDecryptInit() correctly passes decrypt_p=1. But the public generic wc_AesGcmInit(), whose own doc comment says "Initialize an AES GCM cipher for encryption or decryption", passes 0. Any caller that uses the generic init to set up a decryption with an 8-byte nonce now gets FIPS_BAD_VALUE_E under HAVE_FIPS, while the identical operation succeeds via wc_AesGcmDecryptInit() — so the restriction is applied inconsistently and contradicts the stated SP 800-38D rationale. This is not hypothetical: bsdkm/wolfkmod.c:916-925 selects encrypt/decrypt into is_encrypt and then calls wc_AesGcmInit() for both directions with csp->csp_ivlen (a session-supplied IV length, not pinned to 12), and linuxkm/lkcapi_aes_glue.c AesGcmCrypt_1() likewise uses wc_AesGcmInit() for both directions — only its hard-coded GCM_NONCE_MID_SZ keeps that path working.
Commit-message cross-check. 49bcd1f asserts "The floor takes two shapes, both correct by construction" and describes wc_AesGcmInit_local as "reached from wc_AesGcmInit and wc_AesGcmEncryptInit_ex, which pass a decrypt_p flag". wc_AesGcmInit does not pass a meaningful flag -- it hardcodes 0 /* decrypt_p */. The same commit states "Decrypt accepts any supported length per SP 800-38D", which is precisely what this call site prevents: wolfSSL_EVP_CipherInit (evp.c:7124) routes both directions through wc_AesGcmInit.
Suggestion: Either keep wc_AesGcmInit() direction-agnostic (skip the check, and let wc_AesGcmEncryptInit/wc_AesGcmEncryptFinal enforce it), or add an explicit direction to the public API and route evp.c through wc_AesGcmDecryptInit() on the decrypt path. Also give wc_AesGcmSetExtIV() a direction-aware variant, since evp.c:7113 calls it for both directions.
Recommendation: Decide the contract for wc_AesGcmInit() and make evp.c consistent with it. Add a regression test that decrypts a known AES-GCM vector with an 8-byte IV under FIPS and expects success.
[wolfcrypt/src/aes.c:14078-14082] Either treat the generic init as "direction unknown" and skip the check (deferring enforcement to wc_AesGcmEncryptUpdate/Final), or document wc_AesGcmInit() as encrypt-only under FIPS and convert the in-tree dual-direction callers (bsdkm/wolfkmod.c, linuxkm AesGcmCrypt_1) to select wc_AesGcmEncryptInit()/wc_AesGcmDecryptInit() explicitly.
| @@ -945,6 +966,11 @@ int wc_InitDhKey_ex(DhKey* key, void* heap, int devId) | |||
| key->heap = heap; /* for XMALLOC/XFREE in future */ | |||
There was a problem hiding this comment.
🟠 [High] wc_InitDhKey_ex() returns FIPS_NOT_ALLOWED_E leaving the mp_ints uninitialized, and src/tls.c then frees them
🚫 BLOCK bug
The new enablement check is placed after key->heap / key->trustedGroup are assigned but BEFORE mp_init_multi(). The commit rationale (set the admin fields first) does not achieve a safe partial state: p, g, q, pub, priv, nb and handle are all still raw XMALLOC memory. src/tls.c:8214-8221 allocates kse->key with plain XMALLOC (no memset) and ignores the return of wc_InitDhKey_ex(); src/tls.c:9311 then unconditionally calls wc_FreeDhKey() on it, which runs mp_clear/mp_forcezero over garbage pointers, and under WC_DH_NONBLOCK also does XFREE(key->nb) on a wild pointer. This is reachable in any FIPS v7 build (dh.h:164-167 defaults WC_DH_INITIAL_RUNTIME_ENABLEMENT to 0) and in any --enable-dh=conditional kernel build, whenever the application has not called wc_dh_enable(). Note this same PR fixed the identical pattern in src/internal.c AllocKey() by adding the key_inited flag — tls.c was left behind, so the two call sites now disagree.
The new if (! wc_dh_enabled) return FIPS_NOT_ALLOWED_E; is inserted after key->heap / key->trustedGroup are set but before mp_init_multi(), so key->p/g/q (and pub/priv under WOLFSSL_DH_EXTRA) are left holding whatever the caller's buffer contained. wc_FreeDhKey() (dh.c:1025-1034) only NULL-checks the key pointer and then unconditionally mp_clear()s all of them. Under FIPS v7 this early return is the default path: dh.h:164-168 force-defines WC_DH_INITIAL_RUNTIME_ENABLEMENT to 0, so every wc_InitDhKey*() fails until wc_dh_enable() is called. src/tls.c:8213-8221 and :9556-9560 assign the XMALLOC'd DhKey to kse->key / keyShareEntry->key before calling wc_InitDhKey_ex(), and keep it on the key-share entry when init fails; TLSX_KeyShare_FreeAll() then calls wc_FreeDhKey((DhKey*)current->key) unconditionally at src/tls.c:9311. With sp-math/fastmath (fixed-size mp_int) this merely scribbles zeros, but with heap integer math (--enable-heapmath) mp_clear() does XFREE(a->dp) on an indeterminate pointer. Note src/internal.c AllocKey() was hardened against exactly this in the same diff (the new key_inited flag), which shows the hazard was recognised at one call site but not swept.
Commit-message cross-check. 21305c9 states the intended invariant verbatim: "Place the enablement check AFTER key->heap/trustedGroup init in the five entry points (wc_InitDhKey_ex, ...) so a disabled-DH early return never leaves a half-initialized key for wc_FreeDhKey to mp_clear on garbage." In wc_InitDhKey_ex the check sits after key->heap/key->trustedGroup but before mp_init_multi(), so the exact hazard the commit message claims to have avoided is still present -- tls.c:8214 XMALLOCs kse->key (uninitialized), and TLSX_KeyShare_FreeAll unconditionally wc_FreeDhKey()s it. The fix is to move the check above the heap/trustedGroup stores and return before anything is stored, or to mp_init_multi() first.
Suggestion: Move the enablement check so the key is fully initialized before returning the error, so that a caller which frees regardless is safe:
if (mp_init_multi(...) != MP_OKAY)
return MEMORY_E;
...
#ifdef WC_DH_INITIAL_RUNTIME_ENABLEMENT
if (! wc_dh_enabled) {
wc_FreeDhKey(key);
return FIPS_NOT_ALLOWED_E;
}
#endif
Recommendation: Either fully initialize before failing, or fix src/tls.c:8214-8221 to check the return of wc_InitDhKey_ex() and skip the later wc_FreeDhKey() — mirroring the key_inited fix already applied to AllocKey() in this PR. Add a negative test that builds a DH key share with DH runtime-disabled.
[wolfcrypt/src/dh.c:966-975] Move the enablement check to after mp_init_multi() (freeing the mp_ints before returning), or zero the mp_int members before the early return so wc_FreeDhKey() is safe. Also fix src/tls.c to clear kse->key / keyShareEntry->key when wc_InitDhKey_ex() fails.
| # is prepended to each config's own. KERNEL_EXTRA_CFLAGS_REMOVE and | ||
| # FORCE_NO_MODULE_SIG go in the environment rather than the JSON because | ||
| # the pool builds with a bare "make" (no way to add variables to that | ||
| # command line); make picks them up from the environment, and they are |
There was a problem hiding this comment.
🔵 [Low] fips-dev-no-post.yml: the fuzzing seed is the shell PID and is never echoed, and the adjacent comment about '$' in the heredoc is now wrong
🔧 NIT test
Sites: .github/workflows/fips-dev-no-post.yml:87-89,116; .github/workflows/fips-dev-no-post.yml:88-89,116
The comment at lines 87-89 states "the heredoc is unquoted so $(uname -r) expands -- there is no other $ or backtick in the JSON." That is inaccurate: line 116 contains -DWC_DEBUG_VECTOR_REGISTERS_FUZZING_SEED=$$, and in an unquoted bash heredoc $$ expands to the shell PID. Assuming the expansion is intentional (a literal $$ would not be valid C), the seed is a per-run value that is never printed, so a DEBUG_VECTOR_REGISTER_ACCESS_FUZZING failure in the kernel-settings-all-asm config cannot be reproduced from the log. The existing .github/workflows/intelasm-c-fallback.yml:39-41 draws its seed from /dev/urandom and echoes it precisely so failures are reproducible.
Also, at .github/workflows/fips-dev-no-post.yml:88-89,116 (Low, portability scan) — fips-dev-no-post.yml comment claims the JSON heredoc has no other '$', but it now contains $$:
The step comment reads "Note the heredoc is unquoted so $(uname -r) expands -- there is no other $ or backtick in the JSON", but line 116 contains -DWC_DEBUG_VECTOR_REGISTERS_FUZZING_SEED=$$, which the unquoted heredoc expands to the shell's PID. The expansion itself works (wolfcrypt/src/memory.c:1816-1821 takes an integer constant and a PID fits), so the seed varies per run — presumably intended. But the comment is now false and is a trap: anyone acting on it and quoting the heredoc would emit the literal token $$ into CPPFLAGS and break the static word64 prn = ... initializer. A PID-derived seed also makes a fuzzing failure non-reproducible from the run log alone (recoverable only from the uploaded config.log).
Suggestion:
| # command line); make picks them up from the environment, and they are | |
| SEED=$$ | |
| echo "fuzzing seed=$SEED" | |
| cat > "$RUNNER_TEMP/fips-dev-no-post-configs.json" <<EOF | |
| ... -DWC_DEBUG_VECTOR_REGISTERS_FUZZING_SEED=$SEED ... |
Recommendation: Echo the seed before the heredoc and correct the comment, matching the intelasm-c-fallback precedent.
[.github/workflows/fips-dev-no-post.yml:88-89,116] Update the comment to name $$ explicitly, and echo the resolved seed into the step log (or use ${GITHUB_RUN_ID}) so a fuzzing failure is reproducible from the log.
|
|
||
| #include <wolfssl/wolfcrypt/libwolfssl_sources.h> | ||
|
|
||
| #if defined(WC_C_DYNAMIC_FALLBACK) && \ |
There was a problem hiding this comment.
🔵 [Low] sha256.c: #undef WC_C_DYNAMIC_FALLBACK precedes the header includes, so this TU can see a different struct layout
🔧 NIT bug
The new #undef WC_C_DYNAMIC_FALLBACK blocks execute before hash.h and friends are pulled in. In an --enable-aesni kernel-mode build these two translation units would therefore see struct wc_Sha3 (sha3.h:166) 16 bytes smaller than every other TU in the library, since that struct has a WC_C_DYNAMIC_FALLBACK-conditional member. Latent today — neither file names wc_Sha3 or wc_HashAlg — but it is a genuine ODR/layout trap for anyone who later adds such a use.
Both files now #include <wolfssl/wolfcrypt/libwolfssl_sources.h> and then #undef WC_C_DYNAMIC_FALLBACK (sha256.c:52, sha512.c:74) before including hash.h (sha256.c:84) and cryptocb.h (sha256.c:87). libwolfssl_sources.h brings in only types.h/error-crypt.h/logging.h, so every struct reached through hash.h and cryptocb.h is parsed with the macro already undefined. Three change size under WC_C_DYNAMIC_FALLBACK: wolfssl/wolfcrypt/sha3.h:166-171 (struct wc_Sha3 gains sha3_block/sha3_block_n, 16 bytes on LP64, which propagates into the wc_HashAlg union at hash.h:84-104), wolfssl/wolfcrypt/aes.h:292-294 (struct Aes gains word32 key_C_fallback[60], 240 bytes, reached via cryptocb.h:45), and wolfssl/wolfcrypt/cmac.h:62 (struct Cmac embeds Aes by value). The triggering configuration is exactly the one the #undef was written for: --enable-aesni with kernel-mode defaults (configure.ac:4724-4729 emits WOLFSSL_AESNI + WC_C_DYNAMIC_FALLBACK without USE_INTEL_SPEEDUP). Latent today — neither file names any of those types — but a silent ODR/ABI trap for the next edit. Note that in sha256.c the #undef has no functional effect at all, since every WC_C_DYNAMIC_FALLBACK use there is already nested inside the x86-64/AVX gate.
Commit-message cross-check. Direct consequence of the carve-out discussed in the WC_C_DYNAMIC_FALLBACK gating finding. aes.h:292 and sha3.h:166 still key struct members on WC_C_DYNAMIC_FALLBACK, and sha256.c #undefs it at line 52 then includes hash.h at line 84 -- so in the AESNI-without-USE_INTEL_SPEEDUP config this TU sees a smaller wc_Sha3 (and wc_HashAlg) than the rest of the library.
Suggestion: Use a file-local macro (e.g. SHA256_NO_DYNAMIC_FALLBACK) for the internal dispatch decisions instead of #undef-ing a library-wide, layout-affecting macro.
Recommendation: Replace the #undef with a local macro, or move it after the includes and add a comment noting that no struct visible here depends on the macro.
[wolfcrypt/src/sha256.c:41-53] Derive a file-local switch instead of undefining the global one — keep WC_C_DYNAMIC_FALLBACK intact and define e.g. SHA512_RAW_BUFFER_CONVENTION from it (needed anyway for the sha512.c gating finding), using only the local name inside the .c file.
| @@ -374,7 +374,7 @@ int test_wc_InitCmac_Id(void) | |||
| { | |||
There was a problem hiding this comment.
🔵 [Low] tests/api/test_cmac.c drops KCAPI coverage without explanation
🔧 NIT test
Both tests gained && !defined(WOLFSSL_KCAPI) with no comment explaining why. Silently disabling coverage for a whole backend is the kind of change that becomes permanent because nobody remembers the reason. Since this PR also changes _InitCmac_common's error path (adding wc_AesFree + cmac->type = WC_CMAC_NONE), it would be good to record whether the KCAPI exclusion is a pre-existing gap or a consequence of that change.
Commit-message cross-check. 0c580b4 is a one-line commit -- "add && !defined(WOLFSSL_KCAPI) to two build guards" -- with no rationale, so the question stands on the commit message too.
Suggestion: Add a one-line comment above each guard, e.g.:
/* KCAPI's CMAC backend does not implement key-by-id/label. */
Recommendation: Add a brief comment stating why KCAPI is excluded, per the repo convention of documenting non-obvious build guards.
| { | ||
| int ret = 0; | ||
|
|
||
| #ifdef WC_DH_HAVE_RUNTIME_ENABLEMENT |
There was a problem hiding this comment.
🔵 [Low] tests/unit.c uses wc_dh_enable()/WC_DH_HAVE_RUNTIME_ENABLEMENT without including dh.h
🔧 NIT
unit.c includes only tests/unit.h, types.h, ecc.h and fips_test.h. Neither unit.h nor unit.c includes wolfssl/wolfcrypt/dh.h directly; it is reached only indirectly (wolfssl/test.h:68 includes it under #ifdef HAVE_PK_CALLBACKS, and wolfssl/test.h:54 -> asn.h:51 under #ifndef NO_DH, with asn.h's whole body under #if !defined(NO_ASN) || !defined(NO_PWDBASED)). If that chain breaks in any configuration, WC_DH_HAVE_RUNTIME_ENABLEMENT is simply undefined and the new enable/disable pair compiles out with no diagnostic. Under FIPS v7 WC_DH_INITIAL_RUNTIME_ENABLEMENT defaults to 0 (dh.h:164-168), so every wc_InitDhKey_ex then returns FIPS_NOT_ALLOWED_E and the whole DH portion of unit.test fails for a reason that looks nothing like a missing include. The C89 declaration placement, the enum/prototype guards and the goto-exit paths were all verified correct.
Recommendation: Add an explicit #include <wolfssl/wolfcrypt/dh.h> to tests/unit.c.
| * (one MUL becomes four) is kept for platforms without a 128-bit integer type | ||
| * (e.g. Cortex-M). Both paths are constant-time and bit-identical. */ | ||
| #if defined(__SIZEOF_INT128__) | ||
| #if defined(HAVE___UINT128_T) && !defined(NO_INT128) && \ |
There was a problem hiding this comment.
⚪ [Info] falcon.c: FALCON_MULHI now additionally requires HAVE___UINT128_T, silently dropping to the 4-multiply fallback
🔧 NIT question
The gate changed from #if defined(__SIZEOF_INT128__) to #if defined(HAVE___UINT128_T) && !defined(NO_INT128) && defined(__SIZEOF_INT128__). HAVE___UINT128_T is supplied by the autotools/CMake configure probe; a hand-rolled user_settings.h build on a compiler that does define __SIZEOF_INT128__ will not define it, and will silently take the portable 32x32 path (one multiply becomes four) in fpr_expm_p63, which is on the signing hot path. Functionally correct — both paths are documented as bit-identical — but a measurable performance regression for out-of-tree builds, with no diagnostic.
Suggestion: Treat SIZEOF_INT128 as sufficient evidence on its own when HAVE___UINT128_T is absent, e.g. #if !defined(NO_INT128) && (defined(HAVE___UINT128_T) || defined(__SIZEOF_INT128__)).
Recommendation: Confirm the extra conjuncts are intended; if the goal was only to honour NO_INT128, drop the HAVE___UINT128_T requirement so user_settings.h builds keep the fast path.
4 participants
tested with