20260907-RNG-extras - #11435
Conversation
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11435
Scan targets checked: none
Failed targets: linuxkm-src, linuxkm-bugs, wolfcrypt-src, wolfcrypt-bugs, wolfssl-src, wolfssl-bugs
|
Note the kernel module build failures are expected, and will resolve once #11382 merges. |
|
| WC_RNG_LOCK_ENTROPY_INVALIDATED) || | ||
| ((rng->RBGCStratum > 0) && (banked_stratum == 0))) | ||
| { | ||
| (void)wc_RNG_DRBG_NextSeedNow(rng); |
There was a problem hiding this comment.
We end up ignoring a return code here (silently casting away to (void)) but all status returns need to be checked and returned if failures occur.
I had claude confirm:
Confirmed non-compliance (NC-1) · random.c:4615 · Hash_DRBG generate · output is returned after a banked-seed reseed fails in the same call. NextSeedNow sets DRBG_FAILED or DRBG_CONT_FAILED (4424, 4428), but status was checked only back at 4580, so Hash_DRBG_Generate at line 4652 below is still writing output it shouldn't. The stir at 4646 discards any earlier reseed failure and never sets status (4496). Code: (void)wc_RNG_DRBG_NextSeedNow(rng); Req: SP 800-90A Rev.1 §11.4.2 (Errors Encountered During Health Testing): "pseudorandom bits shall not be output when an error state exists". §11.4.1 (Errors Encountered During Normal Operation): catastrophic errors "shall be treated in the same manner as an error detected during health testing". §9.3.1 (The Generate Function, Output to a consuming application): on any non-SUCCESS status "a Null string shall be returned". Fix: re-check rng->status after 4617 and test the stir return at 4646; return RNG_FAILURE_E; map a stir reseed failure to DRBG_FAILED. Cfg: NEXT_SEED + LOCK + RBGC (all default on); FIPS v7+ and non-FIPS.
There was a problem hiding this comment.
Fixed. wc_RNG_DRBG_NextSeedNow() can only fail there if the RNG state is corrupt -- I promoted the error to customary
{
if (wc_RNG_DRBG_NextSeedNow(rng) != 0) {
rng->status = DRBG_FAILED;
return RNG_FAILURE_E;
}
}
There was a problem hiding this comment.
The snippet you posted is what I had in mind, but I don't think it made it into the latest commits yet; line 4698 still reads (void)wc_RNG_DRBG_NextSeedNow(rng). There's also no rng->status re-check before the generate at 4744, so if the recover arm fails and sets DRBG_FAILED we still hand out output. Could you double-check whether it got lost in the rebase? A quick status check just before 4742 would cover both paths.
SP 800-90A r1 §11.4.2, pseudorandom bits shall not be output when an error state exists.
| ret = Hash_DRBG_Reseed(rng, seed, seedSz, nonce, nonceSz, 1 /* credited */); | ||
| #ifdef WC_RNG_HAVE_RBGC | ||
| if (ret == 0) | ||
| rng->RBGCStratum = 0; |
There was a problem hiding this comment.
Setting rng->RBGCStratum to 0 here effectively converts any potential RNG to the root (no real control over the root) and clears ENTROPY_INVALIDATED. I had claude double check me on this one:
Confirmed Risk (R-2): · random.c:1003 · Hash_DRBG reseed · a credited reseed from a caller-supplied seed now sets RBGCStratum 0 (making the instance an eligible RBGC root) and clears ENTROPY_INVALIDATED (set at line 972). That path has no minimum seedSz and no wc_RNG_TestSeed. Crediting caller seeds predates this PR; the stratum and quarantine effects are new. Req: SP 800-90A Rev.1 §10.1 (DRBG Mechanisms Based on Hash Functions) Table 2: minimum entropy input length is security_strength. SP 800-90C §5.3 item 3: validated entropy sources "shall be used to instantiate and reseed the DRBG"; §7.3.2 (Additional Requirements for the Root RBGC Construction) item 4: the initial randomness source "must be a validated RBG3(XOR), RBG3(RS), RBG2(P), or RBG2(NP) construction or a full-entropy source".
Proposed Fix: do not set stratum 0 or clear quarantine for caller seeds; reject seedSz < SEED_SZ.
There was a problem hiding this comment.
That's preexisting logic, just extended for the new extras.
I don't like it either.
We'll discuss tomorrow.
There was a problem hiding this comment.
TLDR: This is an inherently problematic public API. In its current form, it accepts user-supplied data of unvetted provenance, treats it as entropy of sterling provenance, and resets the reseedCtr. Clearing ENTROPY_INVALIDATED is clearly correct if resetting reseedCtr is correct, though I came up with something we can do that won't break existing callers:
#ifdef WC_RNG_HAVE_LOCK
if ((cur_lock & WC_RNG_LOCK_ENTROPY_INVALIDATED) &&
(seedSz >= WC_DRBG_SEED_SZ) &&
(ret == 0))
{
The difference being that we won't clear the invalidated bit unless the seed met the WC_DRBG_SEED_SZ criterion.
As for what to do with RBGCStratum, I've squared this circle by adding a WC_RNG_RBGC_USER_SEED_STRATUM, default value 65536 subject to user override, with the following policy enforcement provision:
ret = Hash_DRBG_Reseed(rng, seed, seedSz, nonce, nonceSz);
#ifdef WC_RNG_HAVE_RBGC
if (ret == 0) {
/* User-supplied entropy is of formally unknown provenance. In RBGC
* builds, represent that fact using WC_RNG_RBGC_USER_SEED_STRATUM. */
rng->RBGCStratum = WC_RNG_RBGC_USER_SEED_STRATUM;
}
#endif
Plus wrap prevention to strictly separate user-seed from RBGC-seed values from each other.
We have thus found a roundabout way to represent the unknown provenance of the user-supplied seed, and to prevent that RNG from polluting other properly seeded RNGs.
There was a problem hiding this comment.
I like the seedSz >= WC_DRBG_SEED_SZ idea, but I don't see it in this push yet; the clear at 960 still fires whenever ret == 0. When it lands, would you also be up for skipping the RBGCStratum = 0 here for short caller seeds? That way a caller-supplied seed can't promote an instance to root or lift quarantine unless it meets the minimum.
SP 800-90A r1 §10.1 Table 2, minimum entropy input equals the security strength. SP 800-90C §7.3.2 item 4, a root needs a validated randomness source.
| * invalidation semantics on a lock-free consumer; if a lock is held, | ||
| * the holder learns at unlock time. */ | ||
| if (! (cur_lock & WC_RNG_LOCK_HELD)) | ||
| (void)wc_RNG_DRBG_ScheduleReseed(WC_RNG_BANK_INST_TO_RNG(inst)); |
There was a problem hiding this comment.
A second pre-v7 compatibility item:
- the bank-side invalidate writes reseedCtr without the lease when the instance is not HELD at CAS time.
- A claimant can take the lease in between, and the write can land between the frozen v6 module's reseedCtr == RESEED_INTERVAL check and its reseedCtr++, leaving INTERVAL+1. The frozen == test then never matches until the word32 counter wraps, about 2^32 more requests
| /* defer to wc_RNG_DRBG_Reseed()'s RDRAND-config handling. */ | ||
| return wc_RNG_DRBG_Reseed(rng, seed, seedSz); | ||
| } | ||
| saved_ctr = (wc_drbg_reseed_ctr_t)WC_RNG_BANK_RESEED_CTR(rng); |
There was a problem hiding this comment.
v6 compatibility item:
code outside the boundary writes the v6 frozen module's reseedCtr (803, 849). Legacy Reseed_Uncredited calls the module's credited wc_RNG_DRBG_Reseed with caller or kernel data as entropy_input, then restores the counter from outside, so the frozen module sees a credited reseed with consumer-supplied entropy_input.
SP 800-90A Rev.1
§9.2 (entropy_input not provided by the consuming application)
§10.1.1.3 process step 5 (reseed_counter = 1 is part of the reseed algorithm).
SP 800-90C
§2.6 item 2. Fix: on pre-v7, drop the caller and kernel mix-in, or document it as a non-approved service (SP 800-140B Rev.1 §4.4). (but again only if the v6 module is doing kernel OE's)
There was a problem hiding this comment.
v6 kernel randomness == "fake FIPS"
| return 0; | ||
| } | ||
|
|
||
| ret = wc_RNG_GenerateBlock(root, seed, SEED_SZ); |
There was a problem hiding this comment.
I believe the problem trying to be solved here is:
A random number generator is a small block of secret memory (V and C) that produces new random output every time you ask. If a VM is snapshotted and restored twice, or cloned, both copies start with the exact same secret memory. Both then produce the exact same "random" numbers, so two machines can end up with identical keys or nonces.
The defense is "invalidation." When the kernel reports a VM clone or resume, the code does two things. It puts every RNG in quarantine by setting the INVALIDATED flag. And it throws away any random bytes that were made ahead of time. A quarantined RNG is supposed to be reseeded with fresh entropy from the new machine before anyone needs or depends on it.
The gaps I am seeing are:
- A quarantined parent can still create or reseed children (random.c:3726, and the spawn and banking paths).
- The pool (which is generate now, use later which is already a separate issue) can accept stale bytes after it was emptied (random.c:3444, 3475, 871).
- A second clone event can be erased (random.c:972).
- The bank hands out a still-quarantined RNG as healthy (rng_bank.c:776, 833).
- The kernel quarantines things in the wrong order (lkcapi_sha_glue.c:2310, 2316).
SP 800-90C §7.3.1 item 11 is a testable requirement: the DRBG internal state "shall not be removed from the computing platform on which it was created, including for storage". A VM snapshot or clone moves that state to another platform, and invalidation is our way of honoring the requirement, that's good!
Each gap above is a place that the protection can leaks.
The proposed fix:
- Numbered events instead of a yes/no flag. Keep a counter that goes up on every clone or resume event, and stamp everything made ahead of time with the counter value it was made under. At the moment of use or publish, anything stamped with an older number gets thrown away. This fixes gaps 2 and 3.
- Quarantined parents can't make or reseed children. Reseed the parent first, and have children inherit the quarantine. This fixes gap 1.
- A failed cure means no handoff. Release the instance and move on. This fixes gap 4.
- Quarantine the root first, then the instances it feeds. This fixes gap 5.
There was a problem hiding this comment.
Nice progress here: spawn, ReseedRBGC and the banked RBGC path all go through wc_RNG_GenerateBlock now, so the root's quarantine is honored. A few gaps remain: the pool publish CAS (3508, 3539) can succeed against a word a purge just reset to 0; the clear at 960 can absorb a second invalidation that arrives mid-reseed; rng_bank.c:833 can still hand out a quarantined lease; and lkcapi_sha_glue.c:2310 invalidates the bank instances before the daemon root. An invalidation epoch that publish and consume both re-check might cover most of these in one go. What do you think?
SP 800-90C §7.3.1 item 11, internal state must not leave the platform it was created on.
| /* kernel must-succeed call used from hard IRQ contexts etc. -- the | ||
| * callback dispatch point will always fall through to native DRBG, but | ||
| * we log the condition loudly from here. */ | ||
| return -ECANCELED; |
There was a problem hiding this comment.
This isn't what I would consider a strict compliance issue but if we're claiming to be replacing kernel services with FIPS compliant services this can fallback to non-compliant ChaCha20 DRBG.
under HAVE_FIPS the new WOLFSSL_LINUXKM_GET_RANDOM_NO_FALLTHROUGH (3877) only raises the log level (3891-3897, 3909-3917); wc__get_random_bytes still returns an error.
The new 7.3 kernel patch then serves native ChaCha output with only a WARN_ONCE (patch 7v3:352-364), and now routes every get_random_uXX call through that path (7v3:397).
The fallback itself first appeared in the linux 7.0 patch as far as I can tell.
To suport linux next with "only FIPS or fail":
FIPS 140-3 IG 2.4.C (Approved Security Service Indicator; AS02.24, ISO/IEC 19790:2012 §7.2.4.2): "All services shall provide an indicator when the service utilizes an approved cryptographic algorithm, security function or process in an approved manner" (only applies if the Security Policy claims get_random_bytes as a module provided service).
SP 800-140B Rev.1
§4.4 (Non-Approved Services)
§10.4 (Error States): "Describe all error states and status indicators".
Proposed Fix: fail closed at the dispatch point under HAVE_FIPS, or latch a visible non-approved flag (for example a kernel taint) on the first fallback and then I can document it in the Security Policy.
There was a problem hiding this comment.
The new 7.3 kernel patch then serves native ChaCha output with only a WARN_ONCE (patch 7v3:352-364), and now routes every get_random_uXX call through that path (7v3:397).
This is a misunderstanding -- there is a pre_emerg_ratelimited() for every fallthrough event. It's the best we can do on a kernel native void must-succeed call.
| * Subsequent operations that require the key's RNG (blinding, pairwise | ||
| * consistency) then either fail with MISSING_RNG_E or fall back to a | ||
| * locally instantiated RNG, per operation, until a new RNG is set. */ | ||
| int wc_RsaClearRNG(RsaKey* key) |
There was a problem hiding this comment.
Just a note to self to remember to add this to fips.c / fips.h as a wrapped service! :)
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11435
Scan targets checked: linuxkm-src, linuxkm-bugs, wolfcrypt-src, wolfcrypt-bugs, wolfssl-src, wolfssl-bugs
Findings: 17
17 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
| #endif | ||
|
|
||
| static ssize_t install_algs_handler(struct kobject *kobj, struct kobj_attribute *attr, | ||
| static ssize_t install_algs_handler(WC_MODULE_ATTR_CONST struct module_attribute *mattr, struct module_kobject *mk, |
There was a problem hiding this comment.
Sysfs conversion references missing compatibility APIs · API contract violations
The sysfs conversion references WC_MODULE_ATTR_CONST and linuxkm_sysfs_install_attr()/_deinstall_attr(), but none is declared or defined in the checkout. Every LKCAPI build fails compilation.
Suggested fix: Add the version-compatible module-attribute macro and sysfs install/deinstall wrappers before switching these call sites.
Basis: ISO C11 §6.2.1 and §6.5.2.2 require identifiers and called functions to have declarations in scope.
|
|
||
| #define WC_LINUXKM_ENTROPY_DAEMON_MAGIC 0x6f77666c | ||
|
|
||
| #if !defined(HAVE_FIPS) || FIPS_VERSION3_GE(7,0,0) |
There was a problem hiding this comment.
RNG registry ignores disabled prerequisite features · API contract violations
WC_RNG_NO_LOCK and WC_RNG_NO_FREE_HOOK remove APIs called unconditionally by the new registry, including wc_RNG_invalidate_entropy() and wc_RNG_register_free_hook(). Either configuration breaks the Linux hash-DRBG build.
Related known finding #10961 (similar but distinct): Both affect Linux DRBG registry/startup handling, but #10961 races default-bank selection via a transient flag, while this unconditionally references APIs removed by disabled features; configuration gating rather than synchronization is required.
Suggested fix: Gate the registry on its required features or explicitly reject these configurations with a targeted preprocessor error.
Basis: ISO C11 §6.5.2.2 requires called functions to have compatible declarations.
| static void wc_linuxkm_rng_registry_add_bank(struct wc_rng_bank *bank) | ||
| { | ||
| struct linuxkm_rng_object *obj = kmalloc(sizeof(*obj), GFP_KERNEL); | ||
| if (obj == NULL) |
There was a problem hiding this comment.
Registry allocation failure disables VM-state invalidation · Cryptographic correctness
Both registry-add helpers silently return when kmalloc() fails while initialization succeeds. The untracked DRBG is skipped by VM-fork and hibernation invalidation, allowing continued pre-event generator state.
Related known finding #7893 (similar but distinct): Both leave initialized DRBG-related state incompletely managed after an error, but #7893 omits staged startup rollback whereas this silently accepts registry-node allocation failure and loses invalidation tracking; the faulting operations and cleanup patches differ.
Suggested fix: Return registration errors and unwind RNG or bank initialization whenever the object cannot be added to the invalidation registry.
Basis: Linux Kernel Memory Management APIs, kmalloc(): the allocation returns NULL on failure.
| break; | ||
|
|
||
| #ifdef WC_LINUXKM_VMGENID_POLL | ||
| wc_linuxkm_vmgenid_poll(&vmgenid_poll_state, local_root); |
There was a problem hiding this comment.
VM-generation poll uses conditionally undeclared root · Logic errors
local_root is declared only when pool or next-seed support is enabled, but the VM-generation poll uses it whenever CONFIG_ACPI && !CONFIG_VMGENID. Disabling both extras produces an undeclared identifier.
Suggested fix: Declare local_root whenever VM-generation polling uses it, initializing it to NULL when both producer features are disabled.
Basis: ISO C11 §6.2.1 requires an identifier's declaration to be visible at each use.
| } | ||
|
|
||
| #ifdef WC_LINUXKM_HAVE_RNG_STATE_INVALIDATE_HANDLER | ||
| ret = linuxkm_sysfs_install_attr(&wc_linuxkm_rng_state_invalidate_attr.attr, |
There was a problem hiding this comment.
RNG sysfs handlers race DRBG teardown · Race conditions
rng_stats and rng_state_invalidate remain callable while deinstall_algs tears down the DRBG. Concurrent writes can dereference freed bank or daemon-root state; unlike known #10962, this is a cross-handler lifetime race.
Suggested fix: Drain both RNG attributes before DRBG teardown, or serialize their handlers with teardown while holding referenced objects alive.
| * always harmless. */ | ||
| if (n > nextSeedSz) | ||
| n = nextSeedSz; | ||
| xorbuf(seed, nonce, n); |
There was a problem hiding this comment.
Concurrent uncredited-seed stores race on shared bytes · Race conditions
Uncredited-seed stores write or XOR the buffer without reserving bytes. Concurrent stores target the same range, while stores during CONSUMING race with reseed reads, losing or corrupting additional input.
Suggested fix: Reserve each write range atomically and reject or defer stores while the buffer is in the CONSUMING state.
Basis: ISO/IEC 9899:2011 §5.1.2.4p25 defines conflicting non-atomic accesses without happens-before ordering as a data race.
There was a problem hiding this comment.
False positive, wrong fix. This is explained in existing comnents -- uncredited data is deliberately raced. It has no provenance to preserve.
| ++rng->_stats_n_nextuncreditedseed_redeemed; | ||
| #endif | ||
|
|
||
| WOLFSSL_ATOMIC_STORE(*lenp, WC_DRBG_NEXT_SEED_EMPTY); |
There was a problem hiding this comment.
Consumed uncredited seed remains in DRBG memory · Missing ForceZero
The function marks the seed buffer empty without wiping caller-supplied seed material. Unlike known #7135's generated-output scratch, this retains stored additional seed data for the RNG lifetime.
Suggested fix: Zeroize the consumed uncredited-seed buffer before publishing its state as EMPTY, including reseed failure paths.
| if (*leaf == NULL) | ||
| return MEMORY_E; | ||
| else | ||
| return wc_InitRngNonceRBGC(*leaf, root, NULL, 0, flags); |
There was a problem hiding this comment.
FIPS RBGC constructors leak allocations on initialization failure · Resource leaks on error paths
The FIPS compatibility constructors return initialization errors without freeing or clearing allocated *leaf. This differs from known #11135's internal async-context leak.
Related known finding #11135 (similar but distinct): Both leak resources after RNG initialization failure, but #11135 is _InitRng's async-device context leak, whereas this is allocation ownership in FIPS RBGC constructors; the functions, resources, and rollback patches differ.
Suggested fix: On initialization failure, release partial RNG state, free *leaf with the root heap, and set the output pointer to NULL.
| api_ret = wc_RNG_DRBG_GetReseedCtr(root, &ctr_after); | ||
| if (api_ret != 0) | ||
| ERROR_OUT(WC_TEST_RET_ENC_EC(api_ret), out); | ||
| if (ctr_after < ctr_before) |
There was a problem hiding this comment.
Uncredited-reseed test accepts an advanced counter · Weak or missing assertions
The test rejects only a decreased reseed counter, so an implementation that increments it still passes even though uncredited reseeding must preserve the counter.
Suggested fix: Require ctr_after == ctr_before after wc_RNG_DRBG_NextUncreditedSeedNow().
| if (api_ret != 0) | ||
| ERROR_OUT(WC_TEST_RET_ENC_EC(api_ret), out); | ||
| api_ret = wc_RNG_invalidate_entropy(&leaf); | ||
| if (api_ret != 0) |
There was a problem hiding this comment.
Next-seed test leaks leaf RNG on assertion failures · Resource leaks in fixtures and teardown
After leaf initializes, every subsequent ERROR_OUT jumps past wc_FreeRng(&leaf). Failed assertions therefore leak its DRBG allocations and other initialized resources.
Suggested fix: Track whether leaf initialized and free it from the shared out teardown path.
71f1f6b to
97bdcee
Compare
| return ret; | ||
| } | ||
|
|
||
| int wc_RNG_DRBG_Reseed_Nonce(WC_RNG* rng, const byte* seed, word32 seedSz, |
There was a problem hiding this comment.
Thanks for adding the gate on the stir path (2091). If we put the same rng->status check at the top of wc_RNG_DRBG_Reseed_Nonce, right alongside the check that rng and seed aren't NULL, we'd keep a failed instance from accepting a credited reseed that resets the counter, purges the pool and clears quarantine.
SP 800-90A r1 §11.4.2, no reseed operations in the error state.
| #ifdef WC_RNG_HAVE_LOCK_FULL_MUTEX | ||
| /* deliberately the last init step: no failure path can strand an | ||
| * initialized mutex. */ | ||
| if (wc_InitMutex(&rng->mutex) != 0) |
There was a problem hiding this comment.
R-4, one remaining gap: Both NOT_COMPILED_IN returns were hoisted ahead of instantiation (2402-2409) and the catch-all cleanup at 2867-2915 now uninstantiates before freeing. This one path still escapes: the mutex-init failure here at 2856 is a bare return that jumps over 2867 with drbg_instantiated == 1, leaving live V and C the caller never frees.
Fix: set ret = BAD_MUTEX_E and fall through to the cleanup instead of returning.
There was a problem hiding this comment.
Acknowledge this has been partially fixed but one path still slips past the new cleanup: if wc_InitMutex fails here we return BAD_MUTEX_E directly, which skips the block at 2867 even though the DRBG is already instantiated, so V and C are left behind. Setting ret = BAD_MUTEX_E and falling through to the cleanup would close it.
SP 800-90A r1 §9.4 step 2, erase the internal state. FIPS 140-3 IG D.L, V and C are CSPs.
| * plain-store purge: | ||
| * stirs carry no claims, so resurrection there is benign by the | ||
| * three-no-ops doctrine.) */ | ||
| static void NextSeedPurge(wolfSSL_Atomic_Int *lenp) |
There was a problem hiding this comment.
Totally understand the view that these bytes were never used. The catch is that the lab tends to read IG D.L as covering entropy input and seed material whether or not it ends up used, so leftover bytes in the buffer after a purge or a failed health test are likely to get flagged. Would it be possible to wipe the buffer at those two points while the producer still owns it? That would also take some pressure off wc_FreeRng, since right now the whole-struct wipe at uninstantiate is the only thing that clears it.
FIPS 140-3 IG D.L, entropy input and seed are CSPs. SP 800-90C §2.7 item 1, destroy intermediate values.
| WC_ATOMIC_INT_ARG want; | ||
| if (cur == WC_DRBG_NEXT_SEED_PURGED) | ||
| return; /* already handed off to a producer's unwind. */ | ||
| want = (cur == WC_DRBG_NEXT_SEED_PRODUCING) ? |
There was a problem hiding this comment.
I think there's a narrow race here. The purge marks an in-flight producer as PURGED, but a consumer that's in the middle of using the buffer (CONSUMING) gets flipped straight to EMPTY. That lets a producer grab the buffer and start writing while the consumer is still reseeding from it, which can mix bytes, pick up the wrong stratum at 4471, and zeroize a buffer the consumer no longer owns. The unconditional EMPTY store at 4479 can also wipe out that producer's claim. Would it work to leave CONSUMING alone in the purge, and read the stratum right after the consumer's CAS?
SP 800-90A r1 §9.2 step 4, reseed entropy comes from the entropy source. SP 800-90C §7.3.2 item 4, root eligibility depends on where the seed came from.
| /* Purge the pool on credited reseeds. A credited reseed is an epoch | ||
| * boundary -- the pool must not serve output of a retired state | ||
| * (particularly pre-invalidation state). */ | ||
| WOLFSSL_ATOMIC_STORE(rng->poolState, 0); |
There was a problem hiding this comment.
When we purge the pool here (and at 3382 and 4661), poolState goes back to 0 but the bytes themselves stay in the ring, up to 65535 of them from before the event. The other two teardown spots (3578 and 4884) do a ForceZero first, and the comment at 3437 says the pool is zeroized under CSP discipline, so adding ForceZero(rng->pool, rng->poolSize) before each of these purges would bring them in line (if we still want to keep the pool, obviously my preference would be to not have a pool to avoid triggering an RFG if the CSTL questions it's presence against the store now, use later statement).
SP 800-90C §2.7 item 1, destroy intermediate values. FIPS 140-3 IG D.L.
| return ret; | ||
| return NOT_READY_E; | ||
| } | ||
| else if ((ret == WC_NO_ERR_TRACE(ENTROPY_RT_E)) || |
There was a problem hiding this comment.
That's fair, and I agree the destination RNG shouldn't be marked failed since it never saw the data. The part I'm still worried about is the entropy source itself: an RCT or APT failure here gets dropped without being counted, and the daemon only logs it under WC_VERBOSE_RNG. Would a simple consecutive-failure counter that latches past a threshold work? That covers the requirement without touching the destination's status.
SP 800-90C §8.1.2.1, stop the RBG when credited entropy sources keep failing. SP 800-140B r1 §10.4, document error states.
| } | ||
| #endif | ||
| if (stir_ready) | ||
| (void)wc_RNG_DRBG_NextStirNow(rng); |
There was a problem hiding this comment.
New since first review: If a hash call fails partway through a stir, Hash_DRBG_StirGenerate returns the error but doesn't set rng->status, and this call site drops it. By then step 2 has already advanced V but the counter increment was skipped, so the instance keeps serving in a half-updated state with nothing flagging it. Could we treat a DRBG_FAILURE from the stir like any other generate failure (set DRBG_FAILED), while still letting the harmless NOT_READY_E refusals through, and check the result here?
SP 800-90A r1 §11.4.1, catastrophic errors during normal operation are handled like health-test failures. SP 800-90A r1 §11.4.2, no output while in the error state.
| #endif /* WC_RNG_HAVE_NEXT_SEED */ | ||
|
|
||
| #ifdef WC_RNG_HAVE_LOCK | ||
| if (WOLFSSL_ATOMIC_LOAD(rng->lock) & WC_RNG_LOCK_ENTROPY_INVALIDATED) { |
There was a problem hiding this comment.
New since first review: This recovery is a good addition. One side effect: PollAndReSeed runs the full KAT and a blocking entropy gather, so every generate on a quarantined instance will now block, which works against the atomic-context contract described around 3984. Where blocking isn't allowed, could we return an error here instead and let the recovery happen in a context that can block, like the daemon? That keeps the 3984 contract and still never serves output from a quarantined instance. I'd avoid reseeding from another DRBG here, since after a VM clone every copy shares that DRBG's state. This probably ties into the pool discussion too.
SP 800-90A r1 §9.2, when entropy is unavailable the reseed returns an error to the caller.
SP 800-90C §2.4.2, reseeding from another DRBG does not recover if both are compromised.
| @@ -2708,13 +4761,41 @@ int wc_FreeRng(WC_RNG* rng) | |||
| { | |||
| int ret = 0; | |||
|
|
|||
| if (rng != NULL) { | |||
There was a problem hiding this comment.
Gentle reminder on this one. wc_FreeRng still returns OBJECT_NOT_LOCKED_E before wiping anything, and wc_rng_free at 2973 still ignores that return, so V and C stay in memory when someone frees without holding the lock. Would you be open to uninstantiating regardless of the lock, or having wc_rng_free take the lock first as long as it doesn't end up in undefined behavior?
SP 800-90A r1 §9.4 step 2, erase the internal state. FIPS 140-3 IG D.L, V and C are CSPs. SP 800-140B r1 §9.3, zeroization methods.
|
|
||
| static ssize_t wc_linuxkm_rng_state_invalidate_handler( |
There was a problem hiding this comment.
Not a FIPS item, just a hygiene thought: if this sysfs node is only for testing, could we gate it out of production builds? It lets root force quarantine and reseeds, which now also triggers the blocking recovery path.
(Same thought for what seems like might be testing or debugging only things added here, if they aren't meant to be in live production builds should we gate them behind a testing/debugging flag of some kind?)
There was a problem hiding this comment.
Actually it's pretty cool that there's a knob available to root to invalidate the kernel randomness -- I think we should keep it. Never know when it might come in handy.
* add NO_DEFAULT_FOUND_E "No default object registered for request type". * add missing #include <wolfssl/wolfcrypt/logging.h> in WOLFSSL_DEBUG_TRACE_ERROR_CODES path. * add __func__ to __GNUC__ WC_ERR_TRACE(). wolfcrypt/src/rng_bank.c, wolfssl/wolfcrypt/rng_bank.h, wolfcrypt/test/test.c: * add wc_rng_bank_inst.bank for parent access from the children. * add wc_rng_bank_inst_checkin(). * improve thread safety, error checking, and default bank support in wc_rng_bank_checkout(), wc_local_rng_bank_checkout_for_bankref(), wc_rng_bank_inst_reinit(), wc_rng_bank_seed(), wc_rng_bank_reseed(), and wc_InitRng_BankRef_local(). * orthogonalize common code in wc_InitRng_BankRef() and wc_rng_new_bankref() into wc_InitRng_BankRef_local(). wolfcrypt/test/test.c: * fix typo in PRINT_HEAP_CHECKPOINT(). * add wc_rng_bank_inst_checkin() tests to random_bank_test(), update expected failure codes, and remove obsolete test clauses. * add missing !HAVE_FIPS gates in cryptocb_test(). wolfcrypt/src/random.c: * in WC_RNG_BANK_SUPPORT variant of wc_RNG_GenerateBlock(), use the new wc_rng_bank_inst_checkin(), not wc_rng_bank_checkin(). * tweaks to WOLFSSL_DEBUG_TRACE_ERROR_CODES code wrappers to mollify clang-tidy and -Wconversion. * fix a spelling error in _InitRng(). linuxkm/lkcapi_sha_glue.c: * in wc_linuxkm_drbg_generate(), opportunistically reseed once half way to WC_RESEED_INTERVAL and wc_linuxkm_can_block(). Also properly inhibit the recovery call to wc_rng_bank_inst_reinit() if ! wc_linuxkm_can_block(). * refactor wc_mix_pool_bytes() to use wc_RNG_DRBG_Reseed(), and only on the CPU-local RNG. wolfcrypt/src/asn.c, wolfcrypt/src/curve25519.c, wolfcrypt/src/evp.c, wolfcrypt/src/pkcs7.c, wolfcrypt/src/pkcs12.c, wolfcrypt/src/srp.c: at each existing wc_InitRng(), attempt wc_InitRng_BankRef() if WC_RNG_BANK_DEFAULT_SUPPORT && WC_HAVE_RNG_BANKREF.
…mproved dynamics.
… wolfcrypt/src/pkcs7.c, wolfcrypt/src/pkcs12.c, wolfcrypt/src/srp.c: revert wc_InitRng_BankRef() airdrops.
* supply WC_DRBG_OK for HAVE_FIPS with FIPS_VERSION3 < 5.2.4 or == 6.0.0, for the DRBG health tests in wc_rng_bank_checkout(), wc_rng_bank_seed(), and wc_rng_bank_reseed(); * in rng_inst_matches_bank(), compare integer addresses (wc_ptr_t), and compute the mid-instance alignment check on the integer difference -- the negative tests deliberately supply fabricated pointers, for which pointer relationals and subtraction are undefined (C11 6.5.8p5 / 6.5.6p9), while integer comparisons are defined for any value.
wc_rng_bank checkin and seed edge cases from the 20260820 review batch: duplicate (stale-copy) checkin through both wc_rng_bank_checkin() and wc_rng_bank_inst_checkin(), asserting BAD_STATE_E without bank mutation; seedSz == 0 no-op success for an explicit inited bank and for the default form while a default is set; and seedSz == 0 with no default bank set, asserting NO_DEFAULT_FOUND_E.
blockability into can_wait before the DRBG checkout, and gate the opportunistic reseed and the RNG_FAILURE_E reinit recovery on it -- wc_linuxkm_can_block() is false whenever the affinity lock is held (the lock callback is SAVE_VECTOR_REGISTERS_MAYBE_INHIBIT()), so the previous live-sampled guards were unsatisfiable in every reachable state and both features were dead. For each of the two blocking operations, bracket it to restore preemptibility while keeping the instance checked out: migrate_disable() (CONFIG_SMP && >= 5.11.0 -- on earlier kernels migrate_disable() is absent or maps to preempt_disable(), so migration during the operation is simply tolerated there), then RESTORE_VECTOR_REGISTERS_MAYBE_INHIBITED() keyed on the lock's _AFFINITY_LOCKED/_VEC_OPS_INH bits, then the blocking operation preemptibly (wc_rng_bank_inst_reinit()'s WC_RELAX_LONG_LOOP now sees can_block() true and actually yields), then re-establish the vector state flavor-matched (SAVE_VECTOR_REGISTERS2() for the affinity hold, DISABLE_VECTOR_REGISTERS() for the inhibit hold), then migrate_enable(). If re-establishment fails, clear the corresponding lock bits so checkin doesn't double-restore; the cleared bits also make a failed bracket a valid entry state for the subsequent one. Plain (non-atomic) accesses to drbg->lock are sound across the span: WC_RNG_BANK_INST_LOCK_HELD is invariantly held, and it is the only bit considered by contending threads. (root defect identified in 20260820 review batch; see PR discussion for why the review's proposed fix -- pre-captured blockability alone, without releasing the holds -- would have converted the dead code into sleep-in-atomic / bh-off stalls)
…r message on failed wc_rng_bank_default_checkout(), and rework the error message when wc_linuxkm_drbg_generate() fails.
…checked out across the stale check-in probes. With the bank refcount at 1, rng_inst_matches_bank() rejected the probes with BAD_STATE_E before the WC_RNG_BANK_INST_LOCK_HELD guard in wc_rng_bank_checkin() -- the guard under test -- was reached, leaving it uncovered. With refcount >= 2 the HELD guard is the rejecting path.
accessor and reseed scheduling services Give outside-the-boundary consumers defined service interfaces for previously missing semantics, avoiding any need for reaching into WC_RNG/DRBG internals for. * wc_RNG_GetStatus(): read-only health-status accessor. * wc_RNG_DRBG_Present(): discriminates the HAVE_INTEL_RDRAND no-DRBG instantiation shape. * wc_RNG_DRBG_GetReseedCtr(): read-only reseed counter accessor; no DRBG reports 0 (never due), so callers need no special-casing. Adds sentinel-guarded wc_drbg_reseed_ctr_t (word64, word32 when !WORD64_AVAILABLE) tracking struct DRBG_internal. * wc_RNG_DRBG_ScheduleReseed(): drive reseedCtr to WC_RESEED_INTERVAL, forcing reseed on the next generate. Deliberately set-to-threshold only -- a general setter would be a mandatory-reseed-bypass primitive. Compile-time assert that WC_RESEED_INTERVAL fits a 32-bit counter when !WORD64_AVAILABLE. * wc_RNG_DRBG_Reseed_Uncredited(): wc_RNG_DRBG_Reseed() semantics with reseedCtr preserved (save/restore in-boundary) -- the SP 800-90A additional-input concept via the reseed derivation, for mixing caller material without entropy credit. * wc_RNG_DRBG_Reseed_Now() (!CUSTOM_RAND_GENERATE_BLOCK): immediate reseed from the module's own seed source, identical gather/health-test/apply/counter/status semantics to the WC_RESEED_INTERVAL backstop reseed; optional nonce rides the same 10.1.1.3 transition as uncredited additional input, via new (additional, additionalSz) pass-through in PollAndReSeed().
state exclusively through the new random.c services; relocate the legacy direct-access mechanism to a pre-v7-FIPS-only compat block. wc_rng_bank_checkout() reseed-due divert and out-of-service gates, wc_rng_bank_seed(), and wc_rng_bank_reseed() now use wc_RNG_GetStatus() / wc_RNG_DRBG_Present() / wc_RNG_DRBG_GetReseedCtr(); the reseed walk uses wc_RNG_DRBG_Reseed_Now() when waiting is allowed (no more scratch-generate counter forcing) and wc_RNG_DRBG_ScheduleReseed() for the non-waiting arm and to preserve the pending-reseed contract after a failed forced reseed. wc_rng_bank_inst_reinit() failure now marks the instance out of service via wc_FreeRng() (deterministic WC_DRBG_NOT_INIT) instead of writing rng->status directly; _InitRng() has platform failure arms (async devctx, Versal TRNG) that return nonzero with status WC_DRBG_OK, so the explicit marking must stay, but it can be service-mediated. No consumer distinguishes WC_DRBG_FAILED from other non-OK statuses. New WC_RNG_BANK_FLAG_SEED_UNCREDITED (1<<6): wc_rng_bank_seed() mixes via wc_RNG_DRBG_Reseed_Uncredited() per instance (and masks the flag from its checkout calls); wc_rng_bank_reseed() rejects it (bank reseeds are always from the module's own source, always credited). rng_bank.h gains a HAVE_FIPS && FIPS_VERSION3_LT(7,0,0) compat block: the historic three-combination reseedCtr/DRBG-null macros relocated verbatim, plus same-named static WC_INLINE fallbacks for the six services, so call sites are uniform across boundary versions and direct state access is confined to one labeled block that exists only where frozen boundaries cannot supply services. Legacy Reseed_Now is schedule + 4-byte generate (counter lands 2 on success; nonce is a separate follow-on uncredited transition; still-at-threshold-guarded counter restore on failure).
kernel-supplied seed material as uncredited, per mainline crypto/drbg.c Delete the local copy of the reseedCtr/DRBG-null macros (now in rng_bank.h's pre-v7 compat block); zero direct DRBG state access remains on the v7 path. wc_linuxkm_drbg_generate(): generate-op src now mixes via wc_RNG_DRBG_Reseed_Uncredited() (crypto/drbg.c passes the same input as SP 800-90A additional input, never crediting it); the opportunistic early reseed observes wc_RNG_DRBG_GetReseedCtr() and calls wc_RNG_DRBG_Reseed_Now() inside the existing SVR release/reestablish bracket -- counter save/poke/restore and the scratch generate are gone, since Reseed_Now resets the counter iff the reseed succeeds. wc_linuxkm_drbg_seed(): pass WC_RNG_BANK_FLAG_SEED_UNCREDITED, so crypto_rng_reset() with caller data no longer resets reseed schedules. wc_mix_pool_bytes(): wc_RNG_DRBG_Present() + wc_RNG_DRBG_Reseed_Uncredited(), dropping the counter save/restore.
log panic-adjacent, when the FIPS RNG cannot serve (WOLFSSL_LINUXKM_GET_RANDOM_NO_FALLTHROUGH) HAVE_FIPS now implies WOLFSSL_LINUXKM_GET_RANDOM_NO_FALLTHROUGH (no opt-out). Under it, wc_get_random_bytes_user() and wc_extract_crng_user() return -EIO -- final, no fallthrough to native randomness -- on checkout or generate failure, with pr_emerg_ratelimited records. In a FIPS setting an RNG failure in a must-succeed RNG call is a system-unusable condition; emerg-over- panic() is deliberate, so the operator can restart in a controlled fashion (glibc getrandom()/arc4random consumers go fatal on -EIO, which is the intended blast radius). wc__get_random_bytes() remains must-succeed in every flavor -- the kernel patch's dispatch point serves the request from the native DRBG on any nonzero return, honoring the get_random_bytes() unconditional-success contract -- but the event is now recorded loudly (emerg under the knob, err otherwise, never silent). Checkout-failure logging on the user paths is un-gated from WC_VERBOSE_RNG; in-loop generate failures become ratelimited (the paths are unprivileged-hammerable). wc_linuxkm_drbg_generate()'s tail conversion becomes -EIO (RNG- failure class) instead of -EINVAL, keeping the slen>0 arm's argument-class -EINVAL distinct; no wolfCrypt-domain code reaches the kernel interface. wc_extract_crng_user()'s tail is reworked to convert live generate failures (previously the raw wolfCrypt-mapped code leaked out, and only an unreachable ret==0 arm was handled), excluding -EFAULT so first-block copy_to_user() faults keep their honest code; partial-copy-then-failure returns final -EIO in both flavors, required because the -ECANCELED fallthrough contract promises an untouched iterator.
wolfssl/wolfcrypt/{rsa,ecc,curve25519}.h: add wc_RsaClearRNG(),
wc_ecc_clear_rng(), wc_curve25519_clear_rng().
Companions to the three existing RNG setters (the complete set in
wolfcrypt), so a caller can detach a key's RNG association through a
dedicated API; the setters are unchanged, in particular
wc_RsaSetRNG() retains BAD_FUNC_ARG on a NULL rng. Each companion
is declared beside its setter under the same gate and mirrors its
shape (wc_ecc_clear_rng() keeps the success-no-op stub arm when
!ECC_TIMING_RESISTANT). A cleared key fails RNG-requiring
operations safely: MISSING_RNG_E on RSA blinding and ECC
timing-resistant paths, BAD_FUNC_ARG from the RNG service on
curve25519 blinding; the RSA pairwise consistency test falls back to
a locally instantiated RNG. For each key type the member written is
the sole storage site -- no subordinate object holds a copy -- so
clearing the top level detaches everything.
Enables checkout / set / operate / clear / checkin usage of
wc_rng_bank instances against per-object RNG consumers. FIPS
wrapper plumbing for the three new symbols to follow with the DRBG
service wrappers.
wolfcrypt/src/ecc.c: in wc_ecc_encrypt_ex() and wc_ecc_decrypt(),
restore privKey->rng after lending ctx->rng for the operation
Both functions implanted ctx->rng into privKey->rng when the ctx had
an RNG and the key did not (ECC_TIMING_RESISTANT needs it in the
shared-secret point multiply), and never removed it -- the caller's
key left the call holding a borrowed pointer into the ecEncCtx's
RNG, dangling once the ctx or its RNG is freed, and silently
re-attaching an RNG behind the back of any caller practicing
set/operate/clear hygiene. Track the lend (lentRng) and restore
privKey->rng = NULL at every post-implant exit, including both
WOLFSSL_SMALL_STACK allocation-failure arms and both tails. Callers
that pre-set the key's RNG are unaffected.
reinit brackets, release and re-establish the scheduling locks per-level -- an affinity-locked checkout with WC_RNG_BANK_FLAG_NO_VECTOR_OPS (from the caller's flags or bank-wide bank->flags) holds BOTH the affinity save and the vector-ops inhibit, and the single flavor-sniffing release only popped the innermost level, while the re-establishment failure arm cleared both lock bits including the never-released affinity hold, leaving checkin unable to unwind it (leaked bh-off). Release now mirrors wc_rng_bank_inst_checkin() (innermost first), re-establishment mirrors wc_rng_bank_checkout() (acquisition order), and each level's failure clears only its own bit. (identified in 20260824 review batch)
Hash256_DRBG_Reseed() carried two diagnostics naming Hash_DRBG_Reseed(), its caller, which sends anyone reading a log or a wc_MemZero_Add() report to the wrong function. Also reflows two declarations whose continuation lines were indented to a column that no longer matches their return type, and completes a sentence in the wc_RNG_DRBG_NextStirStore() comment.
wc_RNG_GenerateBlock() discarded the return value of the banked-next-seed consumption path. A hard failure there leaves the instance's state in an indeterminate condition, which is precisely the situation the DRBG_FAILED status exists to represent, and callers were given output anyway. Set the status and return RNG_FAILURE_E so that the instance is taken out of service and the caller learns of it, matching how every other reseed-path failure in this file is handled.
The RBGC test entry points were gated on WC_RNG_HAVE_RBGC alone, and the pre-v7 copy of rng_drbg_rbgc_test() pulled rng_bank.h in with WC_RNG_BANK_SUPPORT defined locally and then undefined again, so that the RNG-level compat shims were visible only inside that one translation region. Gate the tests on the boundary version or on WC_RNG_BANK_SUPPORT actually being configured, and drop the local define/undef dance in favour of the real setting. The shims are needed by more than that one test.
Several problems in one function, all of them about failing silently:
- No argument validation. A buffer/length pair with a NULL pointer and
a nonzero length reached the callees, where the STIR and reseed arms
disagreed about what happens -- one returned BAD_FUNC_ARG after a
checkout, the other dropped the input without an error.
- An instance with no DRBG (the HAVE_INTEL_RDRAND bypass in _InitRng())
was "seeded" successfully, having absorbed nothing. Report
NOT_COMPILED_IN instead: we have no way to seed it, so don't pretend
we can.
- The checkin diagnostic printed the reseed's error code rather than
the checkin's, and was the only message in the loop not gated on
WC_RNG_BANK_FLAG_QUIET.
- Messages named wc_rng_bank_seed() from inside the _range() function.
The no-DRBG case is a behaviour change: callers that previously got 0
now get NOT_COMPILED_IN. That is the point -- the previous answer was
not true.
RBGCStratum was reset to 0 by any successful reseed, including one carrying a caller-supplied seed. Stratum 0 means "primary" -- an RNG seeded from the entropy source -- and it is what wc_RNG_DRBG_NextSeedGenerate_local() requires of a NextSeed root. So a single wc_RNG_DRBG_Reseed() call promoted an instance to a provenance it had not earned. Reserve a disjoint region of the stratum space for chains rooted in user-supplied entropy. A user-seeded reseed now sets WC_RNG_RBGC_USER_SEED_STRATUM rather than 0, and children descend from there, so unknown provenance is carried rather than erased. Source- gathered reseeds still promote to 0. Guards in _InitRng(), wc_RNG_DRBG_ReseedRBGC_local() and wc_RNG_DRBG_NextSeedGenerate_local() keep a natural chain from growing into the reserved region, and a static assert keeps the two regions from overlapping if the constant is overridden. Consequence worth stating: while an instance carries a user-seed stratum it cannot serve as a NextSeed root. That is state, not a brand -- a credited primary reseed promotes it back to 0, and an RBGC reseed places it one below its root. random_bank_test() covers both directions: a spawned child at USER_SEED_STRATUM + 1, and a prediction-resistance spawn whose parent is re-promoted to 0 by a fresh credited primary reseed immediately before the child's draw.
Hash_DRBG_Reseed() cleared WC_RNG_LOCK_ENTROPY_INVALIDATED whenever the bit had been observed set on entry and the reseed then succeeded. If a second invalidation event landed while the reseed was in flight, that event was swallowed: the clearing CAS cleared whatever it observed, so an instance whose seed predated the new event was presented as recovered. One bit cannot distinguish "the assertion I observed" from "a newer assertion", so add a second: WC_RNG_LOCK_ENTROPY_RECOVERING, asserted by the reseed path on entry and cleared by wc_RNG_invalidate_entropy(). It functions as a mutex and as an epoch witness -- finding it gone at exit means we were re-invalidated, so _INVALIDATED stands and the caller gets NEEDS_RECOVERY_E. Also refuses an undersized seed up front rather than reseeding and then silently declining to clear the latch, and returns BUSY_E rather than interleaving two recoveries. Both short-circuit before the mutex is taken. Every path below the acquire must reach the release, or the instance is stuck at BUSY_E. The pre-lock boundary keeps the latch bank-side and cannot see any of this, so the same protocol is reproduced around the four bank-side recovery paths: wc_rng_bank_inst_reseed_now(), _reseed_rbgc(), the wc_rng_bank_reseed_range() retry loop, and NextSeed consumption during checkout. WC_RNG_LOCK_EXTRA_SHIFT moves from 3 to 4 accordingly, in both copies of the definitions.
Instantiation already runs wc_RNG_TestSeed() over caller-supplied seed material, and both the PollAndReSeed() and NextSeed paths run it over source-gathered material. The user-supplied reseed path was the only one that did not. Apply it when recovering an invalidated instance. The legacy wc_RNG_DRBG_Reseed() API conflates seed material of every provenance into one argument and credits it entirely on caller attestation, so this is the only structural check available there. Scoped to the invalidated case, so no incumbent path changes behaviour. On a full-width seed the RCT and APT cutoffs cannot be reached by chance, so only degenerate material -- long runs, constants -- is rejected.
Two related gaps in what callers could supply.
The SP 800-90A personalization string was reachable only as the hardcoded
NULL that _InitRng() passed to Hash_DRBG_Init(). Hash_df() already had
the input; nothing could fill it. Thread it through the instantiate
family -- wc_InitRngNonce_ex2(), wc_InitRngNonceRBGC{,_New}(),
SpawnRngRBGC(), wc_rng_bank_init_nonce() and wc_rng_bank_spawn{,_new}().
Personalization is an instantiate-time input in SP 800-90A, so the reseed
and stir APIs deliberately do not gain it; material of that kind belongs
in their existing additional-input argument.
Separately, the bank-level seed and reseed APIs had no way to pass a nonce
to the per-instance reseed underneath them, so wc_rng_bank_seed{,_range}()
and wc_rng_bank_reseed{,_range}() gain one. The index arguments keep
their positions and the material sits immediately before timeout_secs.
linuxkm uses it to carry a post-event timestamp into the recovery reseeds
issued after a state invalidation, and zeroizes it on every path, matching
the existing uncredited-nonce sites in that file.
The pre-v7 boundary shims had grown to roughly six hundred lines of static inline code in a public header, instantiated into every translation unit that included it whether or not any of it was reachable. Move the implementations into rng_bank.c and leave declarations behind. Pure relocation -- review with --color-moved; the only edits are the conversion from static inline to external linkage and the reindentation that follows from leaving the nested conditional blocks.
The banking loop assigned wc_RNG_DRBG_NextSeedGenerate()'s return value to api_ret and then immediately overwrote it with wc_RNG_DRBG_NextSeedCurrent()'s, so every check below -- the error classification, the ALREADY_E break, and the guard on the monotonic-progress assertion -- was reading the query's result, not the generation's. A hard failure from the generate could not fail the test. Keep the two results in separate variables and classify the generate's own return value. Reported by Fenrir.
The assertion rejected only a decreased reseed counter, which no implementation would produce, so it could not distinguish a stir from a credited reseed or from a stir that never ran. A stir is one SP 800-90A Rev.1 10.1.1.4 generate with the stir material as additional input, so step 7 advances the counter by exactly one. Require that: a reset to 1 would mean the stir had masqueraded as a credited reseed, and no change at all would mean it never executed. Reported by Fenrir, whose suggested assertion -- that the counter be unchanged -- does not hold: it fails against the current implementation, because incrementing is what the generate is required to do.
leaf was declared inside the inner block that initializes it, so the ERROR_OUT() calls between its wc_InitRngNonceRBGC() and its wc_FreeRng() jumped to a teardown that could neither see it nor free it. Any failed assertion in that window leaked the DRBG allocation and whatever else the instance held. Hoist the declaration to function scope under the same guard, track initialization, and free it from out: alongside root. Reported by Fenrir.
wc_RNG_DRBG_Reseed_Nonce() accepted nonce == NULL with nonceSz > 0. Hash_df() skips an input whose pointer is NULL regardless of its stated length, so the caller's additional input was discarded and the reseed reported success -- the caller had no way to learn that the material it supplied never entered the derivation. Return BAD_FUNC_ARG, matching the check _InitRng(), SpawnRngRBGC() and wc_RNG_DRBG_ReseedRBGC_local() already make on the same pair. Reported by Fenrir.
The WC_RNG_INIT_FLAGS_USE_FULL_MUTEX arm returned BAD_MUTEX_E directly. By that point the DRBG is instantiated, and the common cleanup that uninstantiates it and frees the allocation sits below the return, so V and C survived a failed _InitRng() -- in memory owned by a WC_RNG the caller has been told is not initialized, and will therefore not pass to wc_FreeRng(). Set ret and fall through to that cleanup instead. SP 800-90A Rev.1 9.4 process step 2 requires the internal state to be erased; FIPS 140-3 IG D.L makes V and C CSPs. Reported by Fenrir; the sibling flag-validation failures were hoisted ahead of instantiation earlier, but this path was missed.
n_rngs was checked only for positivity before being multiplied by sizeof(struct wc_rng_bank_inst) for the instance-array allocation. Where size_t is narrow enough for that product to wrap, the allocation succeeds undersized and the initialization loop immediately runs off the end of it. Reject n_rngs above SIZE_MAX / sizeof(*ctx->rngs) with BAD_LENGTH_E, which is what the static-bank arm already returns for an oversized request. Reported by Fenrir.
wc_InitRngRBGC_New() and wc_InitRngNonceRBGC_New() allocated *leaf and then returned wc_InitRngNonceRBGC()'s status directly. On failure the allocation was neither freed nor cleared, and the caller -- holding a non-zero return -- has no contract entitling it to free the pointer. Release it and NULL the output, as SpawnRngRBGC() already does for the mainline constructors. Reported by Fenrir. These are the pre-v7 boundary's compatibility constructors, so this only affects HAVE_FIPS && FIPS_VERSION3_LT(7,0,0) builds.
The invalidation registry was gated on the FIPS boundary version alone, but it calls wc_RNG_register_free_hook() and wc_RNG_invalidate_entropy() unconditionally. WC_RNG_NO_FREE_HOOK and WC_RNG_NO_LOCK remove those respectively, so either setting broke the build rather than dropping the feature. Require WC_RNG_HAVE_FREE_HOOK and WC_RNG_HAVE_LOCK as well, and say so in the comment that already explains the old-FIPS exclusion. Reported by Fenrir.
* refactor wc_mix_pool_bytes(), when called with small inputs, to use lockless wc_RNG_DRBG_NextStirStore() to a round-robin-rotated target RNG counted using DEFINE_PER_CPU() and friends. * fix config-dependent undeclared-variable access (local_root) in wc_linuxkm_entropy_daemon. * add WC_RNG_BANK_OFFSET_TO_RNG().
…EADY_E return if reseed is due.
…edGenerate_local() re seed test failures.
… wc_FreeRng(); wolfcrypt/src/rng_bank.c: in wc_rng_bank_fini(), fix comment around RNG teardown lease, and capture and report failure code from wc_FreeRng(), percolating to the caller as RNG_FAILURE_E (signaling that the bank was freed, but wc_FreeRng() had complaints along the way).
…in wc_RNG_Pool_Extract(), wc_RNG_DRBG_NextSeedNow_Nonce(), and wc_RNG_GenerateBlock(), to guard against duplicate internal state.
…perture after absorbing it.
…ratum generators.
* in struct wc_rng_bank, rename daemon_root to root_rng, and change its gate from WC_RNG_BANK_HAVE_DAEMON_SUPPORT to WC_RNG_HAVE_RBGC || WC_RNG_HAVE_NEXT_SEED || WC_RNG_HAVE_POOL. * add devId slot to struct wc_rng_bank. * rename WC_RNG_BANK_FLAG_INIT_RBGC to WC_RNG_BANK_FLAG_RBGC, and support it in wc_rng_bank_reseed_range(). * remove root argument from wc_rng_bank_next_seed_generate_rbgc() (bank->root_rng is now implicit). * remove wc_rng_bank_daemon_root_set() and wc_rng_bank_daemon_root_get(), and add wc_rng_bank_root_rng_init() and wc_rng_bank_root_rng_get(). * revert wc_rng_bank_inst_lock_get() et al from macros back to inlines, to assure type enforcement, and remove dangerous cast from WC_RNG_BANK_INST_TO_RNG(). * in wc_rng_bank_init_nonce(), use the persistent ctx->root_rng, removing the ephemeral-on-stack "WC_RNG root". * in wc_rng_bank_fini(), clean up ctx->root_rng. * in wc_rng_bank_reseed_range(), implement support for WC_RNG_BANK_FLAG_RBGC. * in wc_rng_bank_invalidate_entropy(), add invalidation of bank->root_rng. linuxkm/lkcapi_sha_glue.c: * in wc_linuxkm_rng_state_invalidate(), remove now-obsolete special-case "daemon_root" code, and pass WC_RNG_BANK_FLAG_RBGC to the inline (non-daemon) wc_rng_bank_reseed_range(). * in wc_linuxkm_entropy_daemon(), rename local_root to root_rng, assign it at entry from wc_rng_bank_root_rng_get(bank), and remove the in-daemon cleanup of local_root; move the periodic explicit reseed of the root_rng to precede the pooling pass.
* add error percolation to wc_linuxkm_rng_registry_add_rng() and wc_linuxkm_rng_registry_add_bank(), and treat failures as fatal in wc_linuxkm_rng_bank_init() and linuxkm_InitRng_DefaultRBGC(). * bikeshed wc_mix_pool_bytes() with staggered-startup-offset. wolfcrypt/src/rng_bank.c: * add missing backward-compat WC_DRBG_NOT_INIT and WC_DRBG_OK definitions. * add missing old-old-FIPS wc_RNG_DRBG_Reseed_Now() prototype and wc_RNG_DRBG_Stir() implementation. wolfssl/wolfcrypt/rng_bank.h: fix sign-clash from WC_RNG_BANK_OFFSET_TO_RNG().
…a_glue.c: refactor WC_RNG_HAVE_POOL facility as a CAS-synchronized-writer FIFO, preserving lock-free single-reader, while assiduously enforcing entropy invalidation locally for both readers and writers.
97bdcee to
0c4cec3
Compare
4 participants
Expansion of core RNG facility to add kernel-facing semantics.
The SP 800-90 compliance report is the best introduction to the expansion:
rng-extras-compliance-report (2).pdf
Also expands rng_bank facility with corresponding functionalities, and refactors corresponding portions of
linuxkm/lkcapi_sha_glue.cto follow suit.Tested with full run
wolfssl-multi-test.sh(530 scenarios).Relevant new scenarios:
Note, tested atop #11382, and must merge after #11382 merges.