20260727-fips-dev-no-post - #11031
Conversation
|
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
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
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.
547791b to
e42afa6
Compare
|
retest this please |
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
5fec377 to
2468fe7
Compare
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-src
Failed targets: wolfssl-bugs
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.
2468fe7 to
a4ae192
Compare
…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().
.github/workflows/fips-dev-no-post.yml: add reporting of fuzzing seed. wolfcrypt/src/aes.c, tests/api/test_aes.c, .wolfssl_known_macro_extras: * change FIPS AES-GCM nonce size restrictions from from opt-out (WC_FIPS_AESGCM_ALLOW_SHORT_NONCES) to opt-in (WC_FIPS_AESGCM_NO_SHORT_NONCES). * apply restrictions only on encryption operations, never on decryption. configure.ac: fix HAVE_FIPS_VERSION of fips-ready; fix enable_dh setup in KERNEL_MODE_DEFAULTS setup; fix help message for --enable-dh. linuxkm/x86_vector_register_glue.c: add dump_stack() on each BUG/WARNING message that didn't already have it. src/tls.c: fix a couple leaks in TLSX_KeyShare_GenDhKey(). tests/swdev/swdev.c: gate src->sha_method access in swdev_sha256_copy_state() and swdev_sha512_copy_state() appropriately. tests/unit.c: conditionally include dh.h, to assure wc_dh_enable() is available. wolfcrypt/src/dh.c: * in wc_InitDhKey_ex(), zero the key at entry unless null, remove duplicate key->trustedGroup = 0, and call wc_FreeDhKey() on error at end. * add missing wc_dh_enabled checks in wc_DhGeneratePublic() and wc_DhGenerateParams(). wolfcrypt/src/error.c: fix missing space in FIPS_UNAPPROVED_E string. b/wolfssl/wolfcrypt/settings.h: sense __SIZEOF_INT128__ and if defined, but HAVE___INT128_T and/or HAVE___UINT128_T are undefined, define them. wolfcrypt/src/falcon.c: tweak the gate on __uint128_t availability to lean solely on HAVE___UINT128_T. wolfcrypt/src/random.c: fix a couple missed WC_NO_ERR_TRACE() wrappers. wolfcrypt/src/rng_bank.c: properly tolerate WC_ACCEL_INHIBIT_E as a retval from bank->affinity_lock_cb(). wolfcrypt/src/sha256.c, wolfcrypt/src/sha512.c: move #undef WC_C_DYNAMIC_FALLBACK for WOLFSSL_AESNI without USE_INTEL_SPEEDUP to follow all includes, assuring no struct layout conflict. wolfcrypt/src/sha512.c: fix wrong call in intelasm Transform_Sha512() !WC_C_DYNAMIC_FALLBACK SHA512_C path. wolfcrypt/test/test.c: fix double-WC_TEST_RET_ENC_EC() in mldsa_param_*_vfy_test().
…ating around WC_C_DYNAMIC_FALLBACK: use WC_SHA{256,512}_RAW_BE_BUFFER to indicate big-endian buffer.
tests/swdev/swdev.c: fix typos (FIPS_VERSION3_LT, not FIPS_VERSION_LT).
…_UINT128_T to avoid inadvertent configuration shifts -- HAVE___UINT128_T is a backend selector (SP_WORD_SIZE, CURVED25519_128BIT, CURVED448_128BIT), not merely a type-availability macro; wolfcrypt/src/falcon.c: accept either defined(__SIZEOF_INT128__) or defined(HAVE___UINT128_T) in FALCON_MULHI() implementation selector. wolfssl/wolfcrypt/sp_int.h: fix size of struct sp_ecc_ctx when SP_WORD_SIZE == 64 (as when HAVE___UINT128_T is defined), fixing assert failure in sp_c64.c sp_ecc_verify_256_nb(). The P-256-only bucket was sized against the C32 layout (verify ctx 2376 <= 2560); the C64 ctx is 2640. The 384 and 521 buckets are equally word-size-blind but currently pass at 64 bits on margin (3600 <= 3840, 4560 <= 5280).
linuxkm/x86_vector_register_glue.c: in wc_save_vector_registers_x86(), properly squelch the hard-IRQ call warning if flags & WC_SVR_FLAG_MAYBE_INHIBIT; src/internal.c: in AllocKey(), properly set key_inited = 1 if wc_SlhDsaKey_Init() succeeds; .github/workflows/fips-dev-no-post.yml: test -DWC_FIPS_AESGCM_NO_SHORT_NONCES.
…xpansion: wolfcrypt/src/wc_mldsa.c: WC_C_DYNAMIC_FALLBACK fixes for AVX512. tests/api/test_frodokem.c, wolfcrypt/test/test.c: fixes for WC_DEBUG_CIPHER_LIFECYCLE. wolfcrypt/src/fe_x25519_asm.S, wolfcrypt/src/port/arm/armv8-32-aes-asm.S, wolfcrypt/src/port/arm/armv8-aes-asm.S, wolfcrypt/src/port/arm/armv8-aes-asm_c.c, wolfcrypt/src/port/arm/thumb2-aes-asm.S, wolfcrypt/src/sha3_asm.S, wolfcrypt/src/wc_mldsa_asm.S: regenerate from scripts#647 .github/workflows/fips-dev-no-post.yml: * update "minutes" for tests using empirical data; * add --enable-experimental --enable-all-quantum-crypto to kernel-settings-all-asm scenario and rename it kernel-settings-all-pqc-asm; add all-pqc-asm-fallback-fuzzer scenario. wolfcrypt/src/wolfentropy.c: remove WC_FIPS_LL_CRYPTO (it is not a FIPS file except in FIPS v5.2.4). wolfcrypt/src/sha256.c: fix for rebase error (stray #endif).
b65d38a to
67df492
Compare
lealem47
left a comment
There was a problem hiding this comment.
It's worth considering that while Autotools users have to explicitly opt-in to DH with v7, user_settings.h users compile it in by default unless they define NO_DH. Can we guard against that?
|
|
||
| # Enable WOLFSSL_DH_EXTRA and DH const table speedups (eliminates `-lm` math lib dependency) | ||
| # No effect if DH is disabled. | ||
| AM_CFLAGS="$AM_CFLAGS -DWOLFSSL_DH_EXTRA -DHAVE_FFDHE_2048 -DHAVE_FFDHE_3072" |
There was a problem hiding this comment.
These should be avoided if DH is disabled, otherwise the options become confusing
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
2 finding(s) posted as inline comments (see file-level comments below)
Medium (1)
test.c reads non-existent sha256.sha_method / sha512.sha_method fields
File: wolfcrypt/test/test.c:25672
Function: random_bank_test
Category: Logic errors
((struct DRBG_internal *)...)->sha256.sha_method and the SHA-512 analogue at test.c:25666 access a sha_method field on wc_Sha256/wc_Sha512, but neither struct declares such a member (it is a static file-scope variable in sha256.c/sha512.c). Breaks compilation under the guarded macro combination (linuxkm + WC_C_DYNAMIC_FALLBACK + old FIPS).
Recommendation: Drop this check or expose the actual dispatch state via a real accessor/field instead of a nonexistent struct member.
Referenced code: wolfcrypt/test/test.c:25672-25673 (2 lines)
This review was generated automatically by Fenrir. Findings are non-blocking.
|
|
||
| */ | ||
|
|
||
| #define WC_FIPS_LL_CRYPTO |
There was a problem hiding this comment.
Thanks @lealem47 ! Fixing.
It was listed in fips-check-PILOT.sh -- that's how it wound up on the list.
Fixing it now.
|
|
||
| # Enable WOLFSSL_DH_EXTRA and DH const table speedups (eliminates `-lm` math lib dependency) | ||
| # No effect if DH is disabled. | ||
| AM_CFLAGS="$AM_CFLAGS -DWOLFSSL_DH_EXTRA -DHAVE_FFDHE_2048 -DHAVE_FFDHE_3072" |
…ble-all-crypto default sensing, and fixes -ffile-prefix-map and --build-id=sha1 setup for reproducible-build,
…n DH for FIPS v7+.
9473cd1 to
e899972
Compare
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.
| #endif | ||
| #include <wolfssl/wolfcrypt/fips_test.h> | ||
| #if defined(WOLFSSL_FIPS_DEV_NO_POST) && !defined(WC_USE_PIE_FENCEPOSTS_FOR_FIPS) | ||
| #define WC_USE_PIE_FENCEPOSTS_FOR_FIPS |
There was a problem hiding this comment.
🟠 [Medium] WC_USE_PIE_FENCEPOSTS_FOR_FIPS forced unconditionally, breaks non-PIE FIPS dev-no-post builds · Logic errors
WC_USE_PIE_FENCEPOSTS_FOR_FIPS is force-defined whenever WOLFSSL_FIPS_DEV_NO_POST is set, without checking WC_SYM_RELOC_TABLES. The non-PIE seg_map at line 1277 then references __wc_text_start/__wc_rodata_start, which are declared only under #ifdef WC_SYM_RELOC_TABLES in linuxkm_wc_port.h, breaking compilation for --enable-fips=dev-no-post builds with linuxkm PIE disabled.
Fix: Only force WC_USE_PIE_FENCEPOSTS_FOR_FIPS when WC_SYM_RELOC_TABLES is defined; otherwise #error for the unsupported dev-no-post + non-PIE combination.
tested with