Repository navigation
fix(random): lock the shared DRBGs for every use - #86
Closed
MarkAtwood wants to merge 3 commits into
Closed
MarkAtwood wants to merge 3 commits into
MarkAtwood wants to merge 3 commits into
Conversation
priv_rng and pub_rng are process-wide, but wc_rng_lock was held only while seeding and inside wolfssl_rnd(). Key generation, ECDSA and RSA signing, RSA encryption, and RSA/ECDH/X25519 blinding passed &priv_rng to wolfCrypt with no lock. A WC_RNG is not thread safe unless wolfSSL locks it itself (5.9.4 and later, unless built with --disable-rng-autolock), so concurrent callers corrupted the Hash_DRBG state and drew the same output: duplicate private keys and reused ECDSA nonces, with no error returned. Add wolfssl_rng_lock()/wolfssl_rng_unlock() and hold the lock across every wolfCrypt call that uses either DRBG, including calls on a key that keeps a pointer to one. Nothing called under the lock re-enters the wrapper's RNG. Also: - initialize the lock with pthread_once(); the old flag check raced with wc_InitMutex() when the first two users arrived together; - release the lock when wc_InitRng() fails (both paths returned with it held, hanging the next RNG call); - clear rng_ready when the fork path frees the DRBGs, so a failed re-init is retried instead of using freed state; - take the lock in wolfssl_rnd_refresh() for rng_ready; - drop the per-keygen wc_SetSeed_Cb() calls: wolfssl_ensure_rng() has already registered the callback under the lock, and the unlocked writes raced with each other.
Eight threads generate P-256 keys and sign with them through the provider; afterwards every private scalar and ECDSA r value must be distinct and every operation must have succeeded. A shared DRBG used without its lock repeats outputs; a working one repeats with negligible probability. -fast runs 8 x 250 operations, the default 8 x 1000. Link the tests with -pthread.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The wrapper build must link its newly introduced direct pthread dependency.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Serializes access to process-wide wolfSSL DRBGs to prevent concurrent state corruption.
Changes:
- Adds thread-safe RNG initialization and locking.
- Protects all cryptographic operations using shared DRBGs.
- Adds concurrent key-generation/signing regression coverage.
| File | Description |
|---|---|
src/random.c |
Implements shared RNG locking and safe initialization. |
src/random.h |
Exposes RNG lock helpers. |
src/pk.c |
Locks DRBG-dependent operations. |
tests/Makefile |
Registers and links the threaded test. |
tests/test_pk_threads.c |
Detects duplicate keys and ECDSA nonces. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| */ | ||
| void wolfssl_rng_lock(void) | ||
| { | ||
| pthread_once(&wc_rng_lock_once, wc_rng_lock_init); |
The RNG lock is initialized with pthread_once(), a direct pthread dependency. glibc before 2.34 provides it only in libpthread, so build and link the wrapper with -pthread rather than rely on another loaded library to supply it.
Author
|
Closing in favour of #89, per Reda. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The wrapper's process-wide DRBGs (
priv_rng,pub_rng) are used without a lock.wc_rng_lockis held only while seeding and insidewolfssl_rnd(), butsrc/pk.cpasses&priv_rngto wolfCrypt unlocked in 16 places: RSA/DH/ECC/Ed25519/Ed448/X25519/X448 key generation, ECDSA and RSA signing, RSA encryption, and RSA-decrypt/ECDH/X25519 blinding.A
WC_RNGis safe to share only when wolfSSL locks it itself: 5.9.4 and later, unless built with--disable-rng-autolock. With any earlier wolfSSL, including the 5.9.1 FIPS Ready bundle, concurrent gnutls callers corrupt the Hash_DRBG and draw the same output: duplicate private keys and reused ECDSA nonces (a key that signs twice with the same r is recoverable). No error is returned. CI builds wolfSSL master, which has the per-RNG lock, so CI cannot catch this.Measured with EC P-256 keygen + ECDSA sign from concurrent threads:
--disable-rng-autolocktest_pk_threads, 6 runs--disable-rng-autolocktest_pk_threads, 6 runsThreadSanitizer, wolfSSL master
--disable-rng-autolockinstrumented, 4 threads: main reports 596 races (Hash_DRBG_Generate,array_add,wc_rng_lock_once); this PR reports 0. Single-thread controls are clean on both.The fix adds
wolfssl_rng_lock()/wolfssl_rng_unlock()and holds the lock across every wolfCrypt call that uses either DRBG, including calls on a key that keeps a pointer to one (wc_RsaSetRNG(),ecc_key.rng,wc_curve25519_set_rng()). Nothing called under the lock re-enters the wrapper's RNG.It also fixes:
wc_InitMutex()vswc_LockMutex()in two threads); nowpthread_once();wolfssl_ensure_rng()returned with the lock held whenwc_InitRng()failed, so the next RNG call hung;rng_readyset after freeing the DRBGs;wolfssl_rnd_refresh()wroterng_readyunlocked, and the per-keygenwc_SetSeed_Cb()calls raced each other (wolfssl_ensure_rng()has already registered the callback).New test
tests/test_pk_threads.c: 8 threads generate and sign, and no private scalar or r may repeat. With wolfSSL master and--disable-rng-autolockit fails on main and passes here.make testpasses 26/26 with and without the provider (debian:trixie, wolfSSL master).Left as they were (pre-existing):
fork()while another thread holds the lock leaves the child's copy locked. The window is longer now (a whole keygen). wolfSSL 5.9.4+ handles fork in its own RNG lock;wolfssl_rnd_refresh(),wolfssl_ensure_rng()re-runswc_InitRng()withoutwc_FreeRng(), which leaks the old DRBG state.