Skip to content

wp_drbg: gate wc_RNG_DRBG_Reseed on FIPS version #408

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ColtonWilley wants to merge 3 commits into
base: master
Choose a base branch
from
+229 −45
Open

wp_drbg: gate wc_RNG_DRBG_Reseed on FIPS version #408

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a05f200
wp_drbg: gate wc_RNG_DRBG_Reseed on FIPS version
ColtonWilley Jun 16, 2026
4d22a1b
wp_drbg: handle null reseed entropy, keep reseed seccomp-safe
ColtonWilley Jun 16, 2026
1f1026d
settings: gate DRBG reseed conservatively (FIPS v5.x linkage varies b…
ColtonWilley Jun 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions include/wolfprovider/settings.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,24 @@
#include <wolfssl/version.h>
#include <wolfssl/wolfcrypt/settings.h>

/* wc_RNG_DRBG_Reseed is WOLFSSL_API in non-FIPS >= 5.7.2 and in FIPS v6+.
* Across FIPS v5.x commercial bundles its linkage is inconsistent: some v5.2.4
* bundles export it and others keep it WOLFSSL_LOCAL (it depends on both the
* wolfSSL wrapper version and the FIPS cert), so it cannot be predicted from
* the version macros. Use the native in-place reseed only where the symbol is
* reliably exported and fall back to DRBG re-instantiation otherwise (which
* links on every build). Define WP_NO_DRBG_RESEED to force the fallback for a
* bundle that keeps the symbol WOLFSSL_LOCAL despite its version. */
#if defined(WP_NO_DRBG_RESEED)
/* caller forced the re-instantiation fallback */
#elif !defined(HAVE_FIPS)
#if LIBWOLFSSL_VERSION_HEX >= 0x05007002
#define WP_HAVE_DRBG_RESEED
#endif
#elif defined(HAVE_FIPS_VERSION_MAJOR) && HAVE_FIPS_VERSION_MAJOR >= 6
#define WP_HAVE_DRBG_RESEED
#endif

#define WP_HAVE_DIGEST
#if !defined(NO_MD5)
#define WP_HAVE_MD5
Expand Down
8 changes: 8 additions & 0 deletions scripts/utils-wolfssl.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,15 @@ install_wolfssl() {
# Determine configure option from tag
local fips_configure_arg=""
case "$fips_tag" in
v5.2.4|linuxv5.2.4)
# Distinct module (SP math, PATCH 4) — not the v5/cert4718 base
fips_configure_arg="v5.2.4"
;;
v5.2.3|linuxv5.2.3)
fips_configure_arg="v5.2.3"
;;
v5.2.*|v5.3.*|v5.4.*|v5.5.*|linuxv5.*)
# v5.2.1/cert4718 and other v5.x map to the base v5 module
fips_configure_arg="v5"
;;
v6.*|linuxv6.*)
Expand Down
227 changes: 182 additions & 45 deletions src/wp_drbg.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
#include <openssl/evp.h>
#include <openssl/hmac.h>

#include <wolfprovider/settings.h>
#include <wolfprovider/alg_funcs.h>
#include <wolfprovider/internal.h>

Expand Down Expand Up @@ -61,6 +62,10 @@ typedef struct wp_DrbgCtx {
OSSL_FUNC_rand_get_seed_fn* parentGetSeed;
/** Parent's clear_seed function. */
OSSL_FUNC_rand_clear_seed_fn* parentClearSeed;
#ifndef WP_HAVE_DRBG_RESEED

@aidangarske aidangarske Jun 16, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 [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.

@ColtonWilley ColtonWilley Jun 16, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/** Set when a failed reseed re-instantiation left ctx->rng de-instantiated. */
int rngError;
#endif
} wp_DrbgCtx;


Expand Down Expand Up @@ -143,6 +148,8 @@ static void wp_drbg_free(wp_DrbgCtx* ctx)
}
}

static int wp_drbg_uninstantiate(wp_DrbgCtx* ctx);

/**
* Instantiate a new DRBG.
*
Expand Down Expand Up @@ -171,6 +178,11 @@ static int wp_drbg_instantiate(wp_DrbgCtx* ctx, unsigned int strength,
ok = 0;
}

/* Free any existing DRBG before re-allocating to avoid a leak. */
if (ok && ctx->rng != NULL) {
wp_drbg_uninstantiate(ctx);
}

if (ok && ctx->parentGetSeed != NULL) {
/* Get entropy from parent DRBG (no file I/O needed) */
WOLFPROV_MSG_DEBUG(WP_LOG_COMP_RNG,
Expand All @@ -189,22 +201,29 @@ static int wp_drbg_instantiate(wp_DrbgCtx* ctx, unsigned int strength,
}

if (ok) {
/* Initialize wolfCrypt RNG with parent-provided seed */
ctx->rng = OPENSSL_zalloc(sizeof(*ctx->rng));
/* Use wc_rng_new (>= 5.0) so alloc and free use the same allocator,
* 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) {

@aidangarske aidangarske Jun 16, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 [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.

@ColtonWilley ColtonWilley Jun 16, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, fix made

ok = 0;
}
}

if (ok) {
int rc = wc_InitRngNonce(ctx->rng, seed, (word32)seedLen);
if (rc != 0) {
WOLFPROV_MSG_DEBUG_RETCODE(WP_LOG_COMP_RNG,
"wc_InitRngNonce", rc);
OPENSSL_free(ctx->rng);
ctx->rng = NULL;
#else
ctx->rng = OPENSSL_zalloc(sizeof(*ctx->rng));
if (ctx->rng == NULL) {
ok = 0;
}
if (ok) {
int rc = wc_InitRngNonce(ctx->rng, seed, (word32)seedLen);
if (rc != 0) {
WOLFPROV_MSG_DEBUG_RETCODE(WP_LOG_COMP_RNG,
"wc_InitRngNonce", rc);
OPENSSL_clear_free(ctx->rng, sizeof(*ctx->rng));
ctx->rng = NULL;
ok = 0;
}
}
#endif
}

/* Clear the seed from parent */
Expand Down Expand Up @@ -242,6 +261,13 @@ static int wp_drbg_instantiate(wp_DrbgCtx* ctx, unsigned int strength,
#endif
}

#ifndef WP_HAVE_DRBG_RESEED
if (ok) {
/* Clear any prior reseed error state. */
ctx->rngError = 0;
}
#endif

WOLFPROV_LEAVE(WP_LOG_COMP_RNG, __FILE__ ":" WOLFPROV_STRINGIZE(__LINE__), ok);
return ok;
}
Expand All @@ -262,6 +288,9 @@ static int wp_drbg_uninstantiate(wp_DrbgCtx* ctx)
OPENSSL_clear_free(ctx->rng, sizeof(*ctx->rng));
#endif
ctx->rng = NULL;
#ifndef WP_HAVE_DRBG_RESEED
ctx->rngError = 0;
#endif
WOLFPROV_LEAVE(WP_LOG_COMP_RNG, __FILE__ ":" WOLFPROV_STRINGIZE(__LINE__), 1);
return 1;
}
Expand Down Expand Up @@ -292,6 +321,16 @@ static int wp_drbg_generate(wp_DrbgCtx* ctx, unsigned char* out,
if (strength > WP_DRBG_STRENGTH) {
ok = 0;
}
if (ok && ctx->rng == NULL) {
WOLFPROV_MSG_DEBUG(WP_LOG_COMP_RNG, "DRBG not instantiated");
ok = 0;
}
#ifndef WP_HAVE_DRBG_RESEED
if (ok && ctx->rngError) {
WOLFPROV_MSG_DEBUG(WP_LOG_COMP_RNG, "DRBG in error state");
ok = 0;
}
#endif
#if 0
if (ok && (addInLen > 0)) {
rc = wc_RNG_DRBG_Reseed(ctx->rng, addIn, addInLen);
Expand Down Expand Up @@ -322,6 +361,10 @@ static int wp_drbg_generate(wp_DrbgCtx* ctx, unsigned char* out,
/**
* Reseed DRBG.
*
* When WP_HAVE_DRBG_RESEED is undefined (FIPS modules without an exported
* wc_RNG_DRBG_Reseed), this re-instantiates rather than reseeding: @p entropy /
* @p addIn are used as the nonce, not as DRBG entropy_input.
*
* @param [in, out] ctx DRBG context object.
* @param [in] predResist Prediction resistance required.
* @param [in] entropy Entropy data to reseed with.
Expand All @@ -338,52 +381,130 @@ static int wp_drbg_reseed(wp_DrbgCtx* ctx, int predResist,
{
int ok = 1;
int rc;
unsigned char *seed = NULL;
size_t seedLen = 0;

WOLFPROV_ENTER(WP_LOG_COMP_RNG, "wp_drbg_reseed");

/* If no entropy provided, get fresh entropy from the OS source. */
if (entropy == NULL || entropyLen == 0) {
seedLen = 48;
seed = OPENSSL_malloc(seedLen);
if (seed == NULL) {
ok = 0;
}
if (ok) {
OS_Seed osSeed;
rc = wc_GenerateSeed(&osSeed, seed, (word32)seedLen);
if (rc != 0) {
/* Reseed requires an instantiated DRBG. */
if (ctx->rng == NULL) {
ok = 0;
}

/* wolfCrypt RNG APIs take word32 lengths; reject oversized inputs. */
if (ok && entropy != NULL && entropyLen > 0xFFFFFFFFU) {
ok = 0;
}
if (ok && addIn != NULL && addInLen > 0xFFFFFFFFU) {
ok = 0;
}

#ifdef WP_HAVE_DRBG_RESEED
{
unsigned char* seed = NULL;
size_t seedLen = 0;

/* An in-place reseed needs explicit entropy. If the caller supplied
* none, draw fresh entropy from the cached /dev/urandom fd (SEED-SRC),
* which stays open across a seccomp sandbox, rather than
* wc_GenerateSeed() which opens /dev/urandom directly and would be
* blocked under the sandbox. */
if (ok && (entropy == NULL || entropyLen == 0)) {
seedLen = 48;
seed = OPENSSL_malloc(seedLen);
if (seed == NULL) {
ok = 0;
}
else {
if (ok) {
#if defined(WP_HAVE_SEED_SRC) && defined(WP_HAVE_RANDOM)
if (wp_urandom_read(seed, seedLen) != (int)seedLen) {
ok = 0;
}
#else
OS_Seed osSeed;
if (wc_GenerateSeed(&osSeed, seed, (word32)seedLen) != 0) {
ok = 0;
}
#endif
}
if (ok) {
entropy = seed;
entropyLen = seedLen;
}
}
}

if (ok && entropy != NULL && entropyLen > 0) {
rc = wc_RNG_DRBG_Reseed(ctx->rng, entropy, (word32)entropyLen);
if (rc != 0) {
WOLFPROV_MSG_DEBUG_RETCODE(WP_LOG_COMP_RNG,
"wc_RNG_DRBG_Reseed", rc);
ok = 0;
/* In-place SP 800-90A reseed via wolfCrypt's public DRBG API. */
if (ok && entropy != NULL && entropyLen > 0) {
rc = wc_RNG_DRBG_Reseed(ctx->rng, entropy, (word32)entropyLen);
if (rc != 0) {
WOLFPROV_MSG_DEBUG_RETCODE(WP_LOG_COMP_RNG,
"wc_RNG_DRBG_Reseed", rc);
ok = 0;
}
}
}
if (ok && (addInLen > 0) && (addIn != NULL)) {
rc = wc_RNG_DRBG_Reseed(ctx->rng, addIn, (word32)addInLen);
if (rc != 0) {
WOLFPROV_MSG_DEBUG_RETCODE(WP_LOG_COMP_RNG,
"wc_RNG_DRBG_Reseed", rc);
ok = 0;
if (ok && (addInLen > 0) && (addIn != NULL)) {
rc = wc_RNG_DRBG_Reseed(ctx->rng, addIn, (word32)addInLen);
if (rc != 0) {
WOLFPROV_MSG_DEBUG_RETCODE(WP_LOG_COMP_RNG,
"wc_RNG_DRBG_Reseed", rc);
ok = 0;
}
}

if (seed != NULL) {
OPENSSL_clear_free(seed, seedLen);
}
}
#else

@aidangarske aidangarske Jun 16, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 [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.

@ColtonWilley ColtonWilley Jun 16, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added a test for this.

/* FIPS modules without an exported wc_RNG_DRBG_Reseed (e.g. cert4718):
* re-instantiate in place via wc_FreeRng() + wc_InitRngNonce() on the same
* WC_RNG struct. wc_InitRngNonce self-seeds fresh entropy_input; with
* SEED-SRC that entropy comes from the cached /dev/urandom fd via the seed
* callback (so it stays sandbox-safe), otherwise from wc_GenerateSeed().
* Caller entropy/addIn are folded in only as the nonce (which may be empty).
* A failed re-init leaves the DRBG de-instantiated and sets rngError. */
if (ok) {
unsigned char* nonce = NULL;
word32 nonceLen = 0;
word32 eLen = (entropy != NULL) ? (word32)entropyLen : 0;
word32 aLen = (addIn != NULL) ? (word32)addInLen : 0;

/* Securely clear and free locally allocated seed buffer. */
if (seed != NULL) {
OPENSSL_clear_free(seed, seedLen);
/* Build nonce = entropy || addIn (either may be absent). */
if (aLen > (0xFFFFFFFFU - eLen)) {
ok = 0;
}
if (ok && (eLen + aLen) > 0) {
nonceLen = eLen + aLen;
nonce = OPENSSL_malloc(nonceLen);
if (nonce == NULL) {
ok = 0;
}
else {
if (eLen > 0) {
XMEMCPY(nonce, entropy, eLen);
}
if (aLen > 0) {
XMEMCPY(nonce + eLen, addIn, aLen);
}
}
}
if (ok) {
wc_FreeRng(ctx->rng);

@aidangarske aidangarske Jun 16, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 [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.

@ColtonWilley ColtonWilley Jun 16, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

leaving as is for consistency

rc = wc_InitRngNonce(ctx->rng, nonce, nonceLen);
if (rc != 0) {
WOLFPROV_MSG_DEBUG_RETCODE(WP_LOG_COMP_RNG,
"wc_InitRngNonce", rc);
ctx->rngError = 1;
ok = 0;
}
else {
/* Recovered: clear any prior reseed error. */
ctx->rngError = 0;
}
}
if (nonce != NULL) {
OPENSSL_clear_free(nonce, nonceLen);
}
}
#endif /* WP_HAVE_DRBG_RESEED */

(void)predResist;

Expand Down Expand Up @@ -512,15 +633,25 @@ static int wp_drbg_get_ctx_params(wp_DrbgCtx* ctx, OSSL_PARAM params[])

WOLFPROV_ENTER(WP_LOG_COMP_RNG, "wp_drbg_get_ctx_params");

(void)ctx;

p = OSSL_PARAM_locate(params, OSSL_RAND_PARAM_MAX_REQUEST);
if ((p != NULL) && (!OSSL_PARAM_set_size_t(p, WP_DRBG_MAX_REQUESTS))) {
ok = 0;
}
if (ok) {
int state = EVP_RAND_STATE_READY;

if (ctx->rng == NULL) {
state = EVP_RAND_STATE_UNINITIALISED;
}
#ifndef WP_HAVE_DRBG_RESEED
/* Failed reseed re-instantiation left the DRBG de-instantiated. */
else if (ctx->rngError) {
state = EVP_RAND_STATE_ERROR;
}
#endif

p = OSSL_PARAM_locate(params, OSSL_RAND_PARAM_STATE);
if ((p != NULL) && (!OSSL_PARAM_set_int(p, EVP_RAND_STATE_READY))) {
if ((p != NULL) && (!OSSL_PARAM_set_int(p, state))) {
ok = 0;
}
}
Expand Down Expand Up @@ -622,6 +753,12 @@ static size_t wp_drbg_get_seed(wp_DrbgCtx* ctx, unsigned char** pSeed,
"DRBG not instantiated");
goto end;
}
#ifndef WP_HAVE_DRBG_RESEED
if (ctx->rngError) {
WOLFPROV_MSG_DEBUG(WP_LOG_COMP_RNG, "DRBG in error state");
goto end;
}
#endif

buffer = OPENSSL_secure_malloc(minLen);
if (buffer == NULL) {
Expand Down
Loading
Loading