wp_drbg: gate wc_RNG_DRBG_Reseed on FIPS version by ColtonWilley · Pull Request #408 · wolfSSL/wolfProvider

ColtonWilley · 2026-06-16T21:23:44Z

wp_drbg_reseed references wc_RNG_DRBG_Reseed, but that is not always an exported symbol. It is WOLFSSL_API in non-FIPS >= 5.7.2 and in FIPS v6+, while FIPS v5.x commercial bundles are inconsistent -- the same v5.2.4 cert is WOLFSSL_LOCAL on a 5.8.4 wrapper but WOLFSSL_API on a 5.9.1 wrapper. Linking it fails (undefined reference to wc_RNG_DRBG_Reseed) on the bundles that keep it LOCAL, which the previous version-only gate did not account for.

settings.h: gate WP_HAVE_DRBG_RESEED conservatively. Use the native in-place reseed only where the symbol is reliably exported (non-FIPS >= 5.7.2, FIPS v6+) and fall back to DRBG re-instantiation for all FIPS v5.x (the fallback links on every build). WP_NO_DRBG_RESEED forces the fallback for any other bundle that keeps the symbol WOLFSSL_LOCAL despite its version.
wp_drbg.c: native in-place reseed when available, else re-instantiate the DRBG in place via wc_FreeRng()+wc_InitRngNonce() (with error-state reporting). Handle reseed with no caller entropy on both paths, and keep reseed seccomp-safe (native draws from the cached /dev/urandom fd via wp_urandom_read; fallback self-seeds via wc_InitRngNonce).
utils-wolfssl.sh: map --fips-check=v5.2.4 to --enable-fips=v5.2.4 (was collapsing to v5, which mismatched the SP-math FIPS module).

Validated: build + full unit suite pass (136/0) against the 5.8.4 and 5.9.1 wolfSSL wrappers x v5.2.1 and v5.2.4 commercial FIPS bundles, plus non-FIPS.

Note on the FIPS v5.x fallback path: caller-supplied entropy/addIn is folded into wc_InitRngNonce as the nonce, not mixed as DRBG entropy_input, and predResist is not honored -- wolfCrypt re-seeds internally via its seed source. This is the available behavior where the module does not export wc_RNG_DRBG_Reseed, and differs from the native path where caller entropy is reseed input.

wc_RNG_DRBG_Reseed is WOLFSSL_LOCAL in FIPS v5.2.1 (cert4718), causing an undefined-reference link failure; it is public in non-FIPS >= 5.7.2 and FIPS v5.2.4/v6/v7. Gate WP_HAVE_DRBG_RESEED on the FIPS version in settings.h and reseed in place when public, else re-instantiate. Also map --fips-check=v5.2.4 to --enable-fips=v5.2.4 instead of collapsing to v5.

aidangarske

Skoll Multi-Scan Review

Modes: review + review-security + bugsOverall recommendation: COMMENT
Findings: 5 total — 4 posted, 1 skipped
4 finding(s) posted as inline comments (see file-level comments below)

Posted findings

[Medium] [review+review-security] FIPS fallback reseed re-instantiates from OS entropy, bypassing the parent-seed/seccomp design and treating caller entropy as nonce — src/wp_drbg.c:440-485
[Medium] [review] New error-state and explicit-entropy reseed paths have no test coverage — src/wp_drbg.c:328-333,440-485,612-644
[Low] [review-security+bugs] wp_drbg_get_seed does not check the new rngError state, operating on a de-instantiated RNG after a failed fallback reseed — src/wp_drbg.c:734-756
[Low] [review] wc_FreeRng() return code ignored in fallback reseed — src/wp_drbg.c:468

Skipped findings

[Low] ~~utils-wolfssl.sh adds untested v5.2.3 - --enable-fips=v5.2.3 mapping that changes prior behavior~~

Review generated by Skoll

aidangarske · 2026-06-16T21:36:26Z

            ok = 0;
        }
    }
+#else


🟠 [Medium] FIPS fallback reseed re-instantiates from OS entropy, bypassing the parent-seed/seccomp design and treating caller… · Security

In the non-WP_HAVE_DRBG_RESEED fallback (FIPS modules without an exported wc_RNG_DRBG_Reseed, e.g. cert4718), reseed is implemented as wc_FreeRng(ctx->rng) + wc_InitRngNonce(ctx->rng, nonce, nonceLen), passing the caller-supplied entropy/addIn as the SP 800-90A NONCE rather than as entropy_input. This diverges from the native wc_RNG_DRBG_Reseed path in two ways: (1) wc_InitRngNonce always pulls fresh entropy from the OS seed source (wc_GenerateSeed -> getrandom()/dev/urandom) regardless of caller-supplied entropy, whereas the native path reseeds from caller entropy with no OS access; (2) it is a re-instantiation, not a true reseed, so the DRBG reseed counter/internal state semantics differ. The DRBG is not weakened (it is freshly seeded), and the divergence is documented in the function header. The notable risk worth confirming: wp_drbg_new documents that child DRBGs deliberately route entropy through the parent DRBG (parentGetSeed) so they avoid direct /dev/urandom access for seccomp-sandbox compatibility (see test/test_seccomp_sandbox.c). The in-place wc_InitRngNonce bypasses parentGetSeed entirely, so a reseed under a seccomp sandbox (reachable via RAND_seed/RAND_add, exercised by test_rand.c) could hit /dev/urandom and be blocked/killed on exactly the cert4718 FIPS builds this PR targets. Severity views: review rated Medium/SUGGEST (sandbox interaction worth confirming); review-security rated Info (documented, fails safe). Kept the stricter Medium.

Fix: Confirm (ideally with a sandbox test on a fallback/cert4718 build that triggers a reseed) that the fallback wc_InitRngNonce path does not break the seccomp-sandbox guarantee the parent-seed design provides. If it can, route the fallback re-instantiation seed through ctx->parentGetSeed when available, mirroring wp_drbg_instantiate, to preserve the no-direct-/dev/urandom invariant.

added a test for this.

aidangarske · 2026-06-16T21:36:26Z

    OSSL_FUNC_rand_get_seed_fn* parentGetSeed;
    /** Parent's clear_seed function. */
    OSSL_FUNC_rand_clear_seed_fn* parentClearSeed;
+#ifndef WP_HAVE_DRBG_RESEED


🟠 [Medium] New error-state and explicit-entropy reseed paths have no test coverage · Test Coverage

The PR adds new state logic only exercised on FIPS-fallback builds: ctx->rngError being set on a failed re-instantiation, wp_drbg_generate refusing to run in the error state, and wp_drbg_get_ctx_params reporting EVP_RAND_STATE_ERROR/EVP_RAND_STATE_UNINITIALISED. The existing test/test_rand.c only calls RAND_seed/RAND_add/RAND_bytes on a healthy DRBG and never asserts on the reported OSSL_RAND_PARAM_STATE or drives a reseed failure, so none of the new error-handling branches are validated. make test passing on cert4718 only confirms the happy-path fallback works.

Fix: Add a test that (a) reads OSSL_RAND_PARAM_STATE via EVP_RAND_CTX_get_params and asserts EVP_RAND_STATE_READY after instantiate, and (b) forces a reseed failure to confirm the state transitions to EVP_RAND_STATE_ERROR and that subsequent generate calls fail, plus recovery via uninstantiate+instantiate.

This is basically legacy path at this point, seems like test bloat rather than high ROI testing to me.

aidangarske · 2026-06-16T21:36:26Z

+             * matching the root path and wc_rng_free() in teardown. */
+        #if LIBWOLFSSL_VERSION_HEX >= 0x05000000
+            ctx->rng = wc_rng_new(seed, (word32)seedLen, NULL);
            if (ctx->rng == NULL) {


🔵 [Low] wp_drbg_get_seed does not check the new rngError state, operating on a de-instantiated RNG after a failed fallback… · State Management

This PR introduces a new failure state in the non-WP_HAVE_DRBG_RESEED fallback of wp_drbg_reseed: if wc_InitRngNonce fails after wc_FreeRng(ctx->rng), ctx->rng is left non-NULL but de-instantiated (internal DRBG freed) and ctx->rngError = 1 is set. The PR correctly teaches wp_drbg_generate (line 328-333) and wp_drbg_get_ctx_params (line 629-634) to honor ctx->rngError, but wp_drbg_get_seed was NOT updated. It still gates only on if (ctx->rng == NULL) (line 734), so on a parent DRBG whose reseed re-instantiation failed, a child's GET_SEED request (OSSL_FUNC_RAND_GET_SEED) falls through to wc_RNG_GenerateBlock(ctx->rng, buffer, ...) (line 750) on a de-instantiated WC_RNG. Before this PR the reseed path used wc_RNG_DRBG_Reseed, which never de-instantiates, so ctx->rng != NULL always implied a usable RNG and the single NULL check was sufficient. wolfCrypt's wc_RNG_GenerateBlock guards a de-instantiated RNG (returns an error rather than dereferencing freed memory), so this fails safe with no crash, use-after-free, or weak-randomness leak — hence Low/Info — but the error-state handling is inconsistent with the other two consumers. Severity views: review-security rated Low (CWE-665, defense-in-depth); bugs rated Info (narrow trigger). Kept the stricter Low.

Fix: Mirror the guard used in wp_drbg_generate: immediately after the existing ctx->rng == NULL test in wp_drbg_get_seed, add an #ifndef WP_HAVE_DRBG_RESEED block: if (ctx->rngError) { WOLFPROV_MSG_DEBUG(WP_LOG_COMP_RNG, "DRBG in error state"); goto end; } so a failed-reseed parent reports failure to children consistently rather than relying on wolfCrypt's internal status guard.

Agreed, fix made

aidangarske · 2026-06-16T21:36:26Z

+            }
+        }
+        if (ok) {
+            wc_FreeRng(ctx->rng);


🔵 [Low] wc_FreeRng() return code ignored in fallback reseed · Code Convention

wc_FreeRng(ctx->rng) in the fallback branch discards its return value. The wolfSSL C coding standard calls for checking every return code. This matches the existing style in this file (wp_drbg_free/wp_drbg_uninstantiate also ignore wc_FreeRng's return in the <5.0 path), so it is consistent rather than a regression — noted only for completeness.

Fix: Optional: capture and debug-log the wc_FreeRng return code for consistency with the coding standard, or leave as-is to match the file's existing convention.

leaving as is for consistency

Draw fresh entropy via wp_urandom_read (cached /dev/urandom fd, survives a seccomp sandbox) on the native path when the caller supplies none; the cert4718 fallback self-seeds via wc_InitRngNonce. Neither path opens a file during reseed. Fixes null-entropy handling the fallback previously skipped, and guards wp_drbg_get_seed while the DRBG is in an error state.

…y bundle) wc_RNG_DRBG_Reseed visibility is not predictable from the FIPS version: across frozen commercial bundles the same v5.2.4 cert is WOLFSSL_LOCAL on a 5.8.4 wrapper but WOLFSSL_API on a 5.9.1 wrapper, so the prior "v5.2.4+ -> native" gate hit `undefined reference to wc_RNG_DRBG_Reseed` on 5.8.4+v5.2.4. Use the native in-place reseed only where the symbol is reliably exported (non-FIPS >= 5.7.2, FIPS v6+) and fall back to DRBG re-instantiation for all FIPS v5.x. Add WP_NO_DRBG_RESEED to force the fallback for any bundle that keeps the symbol WOLFSSL_LOCAL despite its version. Verified: builds + full unit suite pass on 5.8.4/5.9.1 wrappers x v5.2.1/v5.2.4 commercial FIPS bundles (5/5, 136/0).

aidangarske self-requested a review June 16, 2026 21:28

aidangarske reviewed Jun 16, 2026

View reviewed changes

aidangarske assigned ColtonWilley and aidangarske Jun 16, 2026

ColtonWilley added 2 commits June 16, 2026 16:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wp_drbg: gate wc_RNG_DRBG_Reseed on FIPS version#408

wp_drbg: gate wc_RNG_DRBG_Reseed on FIPS version#408
ColtonWilley wants to merge 3 commits into
wolfSSL:masterfrom
ColtonWilley:fix-drbg-reseed-fips-version-gate

ColtonWilley commented Jun 16, 2026 •

edited

Loading

Uh oh!

aidangarske left a comment

Uh oh!

aidangarske Jun 16, 2026

Uh oh!

ColtonWilley Jun 16, 2026

Uh oh!

aidangarske Jun 16, 2026

Uh oh!

ColtonWilley Jun 16, 2026

Uh oh!

aidangarske Jun 16, 2026

Uh oh!

ColtonWilley Jun 16, 2026

Uh oh!

aidangarske Jun 16, 2026

Uh oh!

ColtonWilley Jun 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

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

Uh oh!

Uh oh!

aidangarske left a comment

Choose a reason for hiding this comment

Skoll Multi-Scan Review

Posted findings

Skipped 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!

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!

2 participants

ColtonWilley commented Jun 16, 2026 •

edited

Loading