Fix EdDSA lazy public-key derivation race on shared private-only keys - #478
Fix EdDSA lazy public-key derivation race on shared private-only keys#478sameehj wants to merge 2 commits into
Conversation
caaafa7 to
c95f40d
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #478
Scan targets checked: wolfprovider-bugs, wolfprovider-src
Findings: 2
2 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
af3b531 to
531273c
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #478
Scan targets checked: wolfprovider-bugs, wolfprovider-src
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
Fenrir's latest completed scan found no issues; clearing the prior automated change request.
aidangarske
left a comment
There was a problem hiding this comment.
Skoll Code Review
Scan type: review
Overall recommendation: COMMENT
Findings: 2 total — 2 posted, 0 skipped
2 finding(s) posted as inline comments (see file-level comments below)
Posted findings
- [Medium] Start gate does not wait for every worker —
test/test_ecx.c:1341-1345 - [Medium] Concurrent wolfProvider verification is not exercised —
test/test_ecx.c:1231-1237
Review generated by Skoll
Load one seed-only EdDSA key and use it from several threads at once. A readiness barrier parks every worker before the parent releases them, so they hit the first use together. Wave one signs and exports, which runs the lazy public-key derivation under contention. Wave two verifies a precomputed signature through a shared wolfProvider public key, which runs the verify lock under contention. Buffers are sized for whichever EdDSA types are built so an Ed25519-only build compiles. Fenrir 11559.
A private-only EdDSA key derives its public half lazily on first use. The derivation wrote into the shared key object without the key mutex, so concurrent first use could sign or export with a partly written public key. Derive once under the mutex through wp_ecx_ensure_pub and call it from every first-use site; the export and DER helpers become plain export/encode. Fenrir 11559.
531273c to
1843380
Compare
padelsbach
left a comment
There was a problem hiding this comment.
Looks good, couple small questions/concerns
|
|
||
| /* Copy the full key union to preserve internal wolfSSL state. | ||
| * Private material is zeroized below if not selected. */ | ||
| XMEMCPY(&dst->key, &src->key, sizeof(src->key)); |
There was a problem hiding this comment.
We probably want to lock the mutex while doing this memcpy and exportPub. This is a pre-existing issue
| if ((ecx != NULL) && (ecx->data->derivePub != NULL)) { | ||
| int rc; | ||
|
|
||
| #ifndef WP_SINGLE_THREADED |
There was a problem hiding this comment.
Nit: the check of WP_SINGLE_THREADED is built into wp_lock. Other callers in WP of wp_lock don't do this check
| ok = 0; | ||
| } | ||
| else { | ||
| /* Signing mutates the key (persistent SHA), so hold the key mutex |
There was a problem hiding this comment.
What is the "persistent SHA" referring to in this case (and a couple similar comments)? Not sure a comment is even needed here
|
|
||
| if (!key->pubKeySet) { | ||
| ret = wc_ed25519_make_public(key, (byte*)out, *outLen); | ||
| ret = wc_ed25519_make_public(key, pub, sizeof(pub)); |
There was a problem hiding this comment.
AI says it may be more efficient to use wc_ed25519_import_public_ex with the trusted param set. Can you investigate?
Summary
A private-only EdDSA key (a seed-only PKCS#8 Ed25519 or Ed448 key) has no
public half after import. wolfProvider derives the public half later, on
first use. The derivation writes into the shared wolfSSL key object without
holding the key mutex. When two or more threads first use the same
EVP_PKEYat the same time, they sign or export with a partly writtenpublic key. This produces failed or invalid signatures.
Reported by: Fenrir finding 11559.
Root cause
wc_ed25519_make_public/wc_ed448_make_publicsetpubKeySetwhen theywrite to the output buffer. They do not always fill
key->p. Onlywc_ed*_import_publicstores the value intokey->p. The old code derivedthe public half in three places without the mutex:
wp_ed25519_export_public/wp_ed448_export_publicwp_ed25519_digest_sign/wp_ed448_digest_sign(derived before the lock;only
wc_ed*_sign_msgwas locked)wp_Ed25519PublicKeyToDer/wp_Ed448PublicKeyToDerA concurrent first use could read
key->pwhile another thread wrote it.Fix
Derive the public half in one place, under the key mutex.
derivePubcallback towp_EcxData(set for Ed25519 and Ed448,NULLfor X25519/X448).wp_ecx_ensure_pub(). It takes the key mutex, then calls the derivehelper. The helper calls
make_publicinto a local buffer, thenimport_public, which is the only call that setskey->pandpubKeySet.now plain export/encode.
wp_ecx_ensure_pub()from every first-use site: the sign paths,wp_ecx_get_params_enc_pub_key,wp_ecx_match_pub_key,wp_ecx_export_keypair,wp_ecx_dup, and the SPKI branch ofwp_ecx_encode.The SPKI-only guard in
wp_ecx_encodekeeps the public key out of a privatekey encoding. In
wp_ecx_dup, the derive runs before the key copy, so thecopy also gets a happens-before edge against a concurrent first use.
Scope: other algorithms
I reviewed the other key types for the same pattern.
import/decode time, while the object has one owner. No race.
hasPub = 0for aprivate-only key and fails cleanly.
EdDSA was the only affected type.
Test
test_ecx_shared_key_first_use(test 195) loads one seed-only key, then usesit from 4 threads at the same time. Half the threads sign, half export. The
signers verify against a separate public-only key, so a bad public half
cannot hide a bad signature. The workload is fixed and small, so any failure
is a defect, not timing.
ThreadSanitizer (the
tsanjob) is the reliable detector for the data raceitself. The test is the deterministic correctness guard.
Severity
Medium. The window is first use of a shared private-only key. The impact is
failed or invalid signatures, not key disclosure or forgery. A TLS load path
that warms the public key via
X509_check_private_keyis not affected. Asign-only service with a raw private import and a thread pool is the
realistic case.
Commits
test:adds the failing test.fix:adds the fix.The first commit fails on its own by design (red), the second makes it pass
(green). Please merge as a unit; do not run per-commit CI or bisect across
the pair.