rsa: hold the key mutex on every shared-key wolfCrypt call - #487
rsa: hold the key mutex on every shared-key wolfCrypt call#487yosuke-wolfssl wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new concurrency test’s key-size guard should enforce an exact modulus-size match to avoid passing the guard while later failing in RSA_NO_PADDING operations.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR hardens RSA operations against shared-key concurrency races by consistently holding the per-key mutex around wolfCrypt calls that mutate RsaKey state, and adds a multi-threaded regression test to exercise the contended paths under ThreadSanitizer.
Changes:
- Wrap RSA verify paths (PKCS1, PSS, no-pad, X9.31, verify-recover) with
wp_rsa_get_mutex()locking. - Extend the same locking discipline to RSA encrypt/decrypt, RSASVE KEM encapsulate/decapsulate, and
wc_CheckRsaKey()validation. - Add
test_rsa_concurrent_opsto drive one sharedEVP_PKEYfrom multiple threads across the affected operations.
File summaries
| File | Description |
|---|---|
| test/unit.h | Registers the new RSA concurrency test prototype. |
| test/unit.c | Adds the RSA concurrency test to the unit test list. |
| test/test_rsa.c | Implements a pthread-based concurrent RSA sign/verify/enc/dec/KEM test over a shared key. |
| src/wp_rsa_sig.c | Adds per-key mutex coverage for RSA verification-related wolfCrypt calls. |
| src/wp_rsa_asym.c | Ensures encrypt/decrypt operations hold the per-key mutex (including non-blinding builds). |
| src/wp_rsa_kem.c | Serializes RSASVE KEM wolfCrypt calls with the per-key mutex. |
| src/wp_rsa_kmgmt.c | Documents expanded mutex intent and locks wc_CheckRsaKey() under the per-key mutex. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- The five verify helpers, wp_rsaa_encrypt(), wp_rsasve_generate(), wp_rsasve_recover() and wp_rsa_validate() take the per-key mutex around their wc_* calls; wp_rsaa_decrypt() takes it regardless of WC_RSA_BLINDING, which now guards only wc_RsaSetRNG() and its NULL reset. - wp_rsaa_encrypt() and wp_rsaa_decrypt() skip their padding ladders when the lock is not held; wp_rsa_verify_pss() releases on a locked flag; wp_rsasve_generate() cleanses the secret on lock failure. - rc in the verify and RSASVE helpers and in wp_rsa_validate(), and saltLen in wp_rsa_verify_pss(), are initialized at declaration. - The wp_Rsa mutex comment says it is held while refCnt changes and while a wolfCrypt call uses key. - test_rsa_concurrent_ops() runs four threads over one shared EVP_PKEY doing PKCS#1 sign/verify/verify-recover, PSS digest sign/verify, PKCS#1 and OAEP encrypt/decrypt, RSASVE encapsulate/decapsulate, X9.31 sign/verify and a pairwise check. It is built only with HAVE_PTHREAD and without WP_SINGLE_THREADED. Issue: F-8702
f076aba to
8040cef
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #487
Scan targets checked: wolfprovider-bugs, wolfprovider-src
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
Problem
wp_Rsaobjects are reference-shared between operation contexts viawp_rsa_up_ref(), but only 4 of 13 wolfCrypt call sites took the per-key mutex.RsaKeycarriesstate,data,dataLenanddataIsAlloc, which every RSA call mutates — this is not gated on async.wc_RsaSSL_Verify()andwc_RsaPSS_Verify_ex()route throughRsaPrivateDecryptEx(), whichXMALLOCs intokey->dataand letswc_RsaCleanup()free it. The locked sign path (RsaPublicEncryptEx()) callswc_RsaCleanup()as well.So the existing mutex only serialized sign against sign. A concurrent verify, encrypt or KEM operation on the same key raced that malloc/free pair — use-after-free and double-free, plus spurious
BAD_STATE_Everification failures. Locking the public side is what makes the private-side lock mean anything.Closes f-8702.
Fix (
src/wp_rsa_sig.c)Ten call sites now hold
wp_rsa_get_mutex()across theirwc_*call:wp_rsa_sig.cverify_pkcs1,verify_pss,verify_no_pad,verify_x931,verify_recoverwp_rsa_asym.cwp_rsaa_encrypt(fail-closed ladder);wp_rsaa_decryptno longer conditional onWC_RSA_BLINDINGwp_rsa_kem.cwp_rsasve_generate,wp_rsasve_recoverwp_rsa_kmgmt.cwp_rsa_validatearoundwc_CheckRsaKeySize queries (
wc_RsaEncryptSize) andkey->nreads stay outside the lock — nothing writesnduring an operation, and TSan confirms they are not racy.Tests (
test/test_rsa.c)test_rsa_concurrent_opsruns four threads over one sharedEVP_PKEY. All ten locked sites are exercised under contention, verified by instrumented call counts.Verification
-Werrorset.Not in this PR
wp_ecdsa_sig.chas the same asymmetry (sign locks, verify does not);wp_ecx_sig.cand the ML-DSA paths warrant the same audit.