Fix unsynchronized globalRNG use in BN_rand, AddSession, ECDH and X25519 - #11048
Conversation
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.
|
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.
c2e39c8 to
c96f89d
Compare
c96f89d to
266a9ea
Compare
|
Hello @philljj , |
dgarske
left a comment
There was a problem hiding this comment.
Previously (before this PR) ECDH and X25519 shared-secret paths used the global RNG, but now always instantiate and seed a fresh DRBG on every call. This could impact performance for some customers. I think originally this PR grabbed the global and locked it during the entire ECDH/X25519 which caused a different issue. I am not sure what the best solution is and it requires a larger refactor, which @philljj has agreed to take on. For now we'll merge this PR, but it does need some improvement soon (before next release). Its still better than what was in master. Also the PR title and description are wrong, so please correct those. Thanks
Problem
globalRNG(src/ssl.c) is a single process-wideWC_RNGwith no internalper-instance locking; its designated lock is
globalRNGMutex. TheRAND_*family in
src/ssl_crypto.ctakes that mutex at all eleven of its call sites.Four OpenSSL-compat entry points using
globalRNGas their primary RNG took nolock at all, so two threads could interleave DRBG generation on the same V/C
state and produce torn or duplicate output used directly as key material.
Closes f-7543.
Fix
Two remedies, depending on whether the site can cheaply own its RNG:
wolfSSL_BN_randsrc/ssl_bn.cglobalRNGMutexacrosswc_RNG_GenerateBlockAddSessionsrc/ssl_sess.cglobalRNGMutexacrosswc_RNG_GenerateBlock, global path onlywolfSSL_ECDH_compute_keysrc/pk_ec.cwolfssl_make_rng()in place ofwolfssl_make_global_rng()wolfSSL_EC25519_shared_keysrc/pk.cWOLFSSL_CURVE25519_BLINDINGRNGThe two lock sites are open-coded in the style already used in those files, so
no new locking convention is introduced.
The two shared-secret paths instead take a per-call RNG via
wolfssl_make_rng()— the helper the rest ofpk.c/pk_ec.calready uses —removing the sharing rather than serializing on it. Holding
globalRNGMutexacross the whole shared-secret operation (the first version of this PR) would
have serialized all ECDH process-wide, including keys that carry their own RNG.
Both sites now NULL-check the RNG;
wolfSSL_ECDH_compute_keyadditionallyguards the shared-secret call on
!errand clearskey->rngbefore the localRNG is freed, so no dangling reference is left behind.
Known trade-off
ECDH and X25519 now init and seed a fresh DRBG per call instead of reusing the
global one, which costs performance on those paths. Accepted as an interim
state — still better than the unsynchronized global.
Intentionally not in this PR
wolfssl_make_rng()'s fallback, after a localwc_InitRng()has alreadyfailed. Same defect, far lower reachability.
EC_KEYis unsafe independent of the RNG(
wc_ecc_shared_secret_exwritesprivate_key->stateand dispatches on it).That needs per-key locking, which neither wolfSSL nor OpenSSL provides.
Verification
opensslall+keygen,opensslextra+all,dh+ecc(noOPENSSL_EXTRA),singlethreadedandwpas, plus a-DWOLFSSL_CURVE25519_BLINDINGbuild for the Curve25519 path.make check: 17/17 on--enable-all, 6/6 onopensslall, 0 failures.BN_rand,ECDH_compute_key,EC25519_shared_keyandRAND_bytes, 0 races.Negative control: reverting the
BN_randlock reports races inHash512_DRBG_Generate.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.