Lock globalRNGMutex in BN_rand, ECDH, EC25519 and AddSession - #11048
Open
yosuke-wolfssl wants to merge 1 commit into
Open
Lock globalRNGMutex in BN_rand, ECDH, EC25519 and AddSession#11048yosuke-wolfssl wants to merge 1 commit into
yosuke-wolfssl wants to merge 1 commit into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses a thread-safety issue where multiple OpenSSL-compat entry points use the process-wide globalRNG without taking its designated lock (globalRNGMutex), risking concurrent DRBG state corruption and unsafe key material generation.
Changes:
- Add
globalRNGMutexlocking aroundwc_RNG_GenerateBlock()inwolfSSL_BN_rand()andAddSession(). - Hold
globalRNGMutexacrossEC_KEY->rngmutation andwc_ecc_shared_secret_ex()inwolfSSL_ECDH_compute_key(), including a missing NULL check onwolfssl_make_global_rng(). - Lock
globalRNGMutexaround Curve25519 blinding usage duringwolfSSL_EC25519_shared_key().
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/ssl_sess.c | Locks globalRNGMutex while generating altSessionID via the global RNG. |
| src/ssl_bn.c | Locks globalRNGMutex around global RNG byte generation in wolfSSL_BN_rand(). |
| src/pk.c | Locks globalRNGMutex during Curve25519 shared-secret generation when blinding is enabled. |
| src/pk_ec.c | Locks globalRNGMutex across key->rng set/clear and ECDH shared-secret computation, adding a NULL check for the global RNG. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
yosuke-wolfssl
force-pushed
the
fix/f_7543
branch
from
August 4, 2026 06:06
ad459fc to
c2e39c8
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11048
Scan targets checked: wolfssl-bugs, wolfssl-src
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
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.
Problem
globalRNG(src/ssl.c) is a single process-wideWC_RNGwhose designated lock isglobalRNGMutex. TheRAND_*family insrc/ssl_crypto.ctakes that mutex at all eleven of its call sites. Four OpenSSL-compat entry points that useglobalRNGas their primary RNG take no lock at all.WC_RNGhas no internal per-instance locking: the only mutex inwolfcrypt/src/random.cisdrbgStateMutex, which guards capability flags, not the DRBG V/C working state. Two threads callingBN_rand()concurrently, or one callingBN_rand()while another calls the correctly lockedRAND_bytes(), interleave DRBG generation on the same state and can produce torn or duplicate output used directly as private key material.Closes F-7543.
Fix (
src/ssl_bn.c)Each site now holds
globalRNGMutexacross its use of the global RNG, open-coded in the same style as the existing lock sites (wolfSSL_SMIME_write_PKCS7is the closest template). No new helper is introduced, so there remains one locking convention in the file.wolfSSL_BN_rand(ssl_bn.c)wc_RNG_GenerateBlockwolfSSL_ECDH_compute_key(pk_ec.c)key->rngset,wc_ecc_shared_secret_ex,key->rngclearwolfSSL_EC25519_shared_key(pk.c)wc_curve25519_shared_secret_ex, underWOLFSSL_CURVE25519_BLINDINGAddSession(ssl_sess.c)wc_RNG_GenerateBlockBoth ECDH sites also gain the missing NULL check on
wolfssl_make_global_rng(), so the mutex is not taken on a path that cannot use the RNG. InwolfSSL_ECDH_compute_keythe shared-secret call is now guarded on!err, and the lock spans thekey->rngset and clear since those mutate theEC_KEY.Review follow-ups
TOCTOU read of
key->rnginwolfSSL_ECDH_compute_key(f-bot) - acknowledged, not fixed here. The observation is accurate:key->rng == NULLis read outside the lock, so a second thread sharing the sameEC_KEYcould observe it already set and skip locking. It is not addressed in this PR because:globalRNGMutexon every ECDH call, including keys carrying their own RNG that never touch the global, serializing all ECDH process-wide.key->rng != NULLunder the lock and either release it, or hold it for every operation regardless of RNG provenance. Even holding it, the first thread's cleanup setskey->rng = NULLmid-operation, yieldingMISSING_RNG_E(ecc.c).EC_KEYis already unsound here independent of the RNG.wc_ecc_shared_secret_exwritesprivate_key->statethree times and dispatches on it withdefault: err = BAD_STATE_E, so two concurrent callers on one key corrupt that state machine and can spuriously fail. That needs a per-key lock, which neither wolfSSL nor OpenSSL provides forEC_KEY.The unlocked read is pre-existing and unchanged by this PR, so there is no regression, and the case that matters in practice, each thread holding its own key and falling back to the global RNG, is fully fixed.
Intentionally not in this PR
wolfssl_make_rng()'s fallback, afterwc_InitRng()on a local RNG has already failed. Same defect, far lower reachability, deferred to keep this change reviewable.EC_KEYsafe, per the review follow-up above. That is a per-key locking problem, not an RNG one.wolfssl_make_rng()sets*localonly on success, andpk.cdeclaresint localRng;uninitialized at one caller, so the fallback path can reachwc_FreeRng()on&globalRNG. Worth its own finding.Verification
opensslall+keygen,opensslextra+all,dh+ecc(noOPENSSL_EXTRA),singlethreaded, andwpas, plus a-DWOLFSSL_CURVE25519_BLINDINGbuild for the Curve25519 path.make check: 17/17 pass on--enable-all, 6/6 onopensslall, 0 failures.BN_rand,ECDH_compute_key,EC25519_shared_key, andRAND_bytesconcurrently, 0 races.BN_randlock reports races inHash512_DRBG_Generate, including a thread holdingglobalRNGMutexracing an unlocked one. Reverting the Curve25519 lock reports 162 races viacurve25519_smul_blind, confirming that lock is on a live path.No regression test is added: a threaded test is nondeterministic and there is no ThreadSanitizer job in CI, so it would add flake without signal.