Add SLH-DSA Support - #455
Conversation
0487400 to
02cbe8b
Compare
8bf2ba2 to
ba88c76
Compare
Frauschi
left a comment
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: REQUEST_CHANGES
Findings: 26 total — 15 posted, 11 skipped
Posted findings
- [High] Examples are check_PROGRAMS so every build configuration runs them via
make check, and CI's example steps silently depend on that —examples/include.am:22-32 - [High] pqc_openssl_example hard-codes SLH-DSA-SHA2-128f, which is absent whenever WOLFPROV_HAVE_SLHDSA is set but SHA2 sets are not built —
examples/pqc_openssl_example.c:210-218 - [Medium] wp_encrypt_key_pkcs8: reports the output buffer size as the encrypted length, and ignores the RNG lock result its sibling now checks —
src/wp_internal.c:1110-1133 - [Medium] Deterministic seed handling: seedLen survives gen_set_params failure paths, and validator and consumer compute the expected length differently —
src/wp_slhdsa_kmgmt.c:1159-1174 - [Medium] wp_slhdsa_import flag handling: asserts hasPub instead of verifying it, and wipes hasPub/hasPriv on failure —
src/wp_slhdsa_kmgmt.c:690-702 - [Medium] Signature context state (context string, deterministic, entropy, encoding) persists across re-init —
src/wp_slhdsa_sig.c:284-291 - [Medium] Uninitialized locals may break the build under the project's own -Werror hardening —
src/wp_slhdsa_sig.c:338 - [Medium] SLH-DSA key mutex covers only sign/verify and the refcount: keymgmt readers, import writers, and lock scope are all unguarded —
src/wp_slhdsa_kmgmt.c:432 - [Medium] wp_dh_sync_priv_to_key mutates the shared inner DhKey unlocked, and wp_dh_encode_epki depends on the size function having run first —
src/wp_dh_kmgmt.c:2789-2837 - [Low] OSSL_SIGNATURE_PARAM_DETERMINISTIC is sent as unsigned int but advertised and read as int —
test/test_slhdsa.c:1139 - [Low] wp_slhdsa_free refcount handling: unsynchronized read, permanent leak when wc_LockMutex fails, and up_ref/free test the mutex result differently —
src/wp_slhdsa_kmgmt.c:363-391 - [Low] SLH-DSA export and get_params report the wrong outcome when a key component is absent —
src/wp_slhdsa_kmgmt.c:805-875 - [Low] wp_slhdsa_gen_init_base calls wc_FreeRng on an RNG that wc_InitRng may never have initialized —
src/wp_slhdsa_kmgmt.c:1067-1091 - [Low] Encoder dispatch binds OSSL_FUNC_ENCODER_IMPORT_OBJECT to a function with a mismatched signature —
src/wp_slhdsa_kmgmt.c:1978 - [Info] Preprocessor hygiene in the new SLH-DSA files: dead version guards, a redundant #ifndef, a dropped WP_HAVE_PKCS8_ENC guard, and a stale #endif —
src/wp_slhdsa_kmgmt.c:1985-1996
Skipped findings
- [Medium] Encrypted-PKCS#8 PEM decode without WOLFSSL_ENCRYPTED_KEYS is unverified in the FIPS config it targets
- [Medium] SLH-DSA test gating: verify-only builds, the decoder parameter-set mismatch, and the per-parameter-set macros are all untested
- [Medium] New encrypted-PKCS#8 tests are guarded out of the exact configurations they were written for
- [Medium] libacvp-pqc workflow pinned to a personal OSP fork branch
- [Low] test_pki_cipher_encrypts stub comment contradicts the new fail-closed behaviour
- [Low] test_slhdsa.c helper robustness: slhdsa_get_raw reads *out on its failure path, and pubonly_sign_fails passes silently on allocation failure
- [Low] wp_cipher_from_params keeps a previously set cipher when the param has the wrong data type
- [Low] wp_dh_fix_pki_len: doc block misattached and no bounds check
- [Low] SLH-DSA key state is read from wolfSSL struct internals, with no has_private accessor of the kind ML-DSA exposes
- [Info] PEM buffer is cleansed with the written length rather than the allocated length
- [Info] openssl_example prints "self-test passed" although the provider registers no self-test callback
Review generated by Skoll via Claude/Codex
Frauschi
left a comment
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: APPROVE
Findings: 24 total — 20 posted, 4 skipped
Posted findings
- [Medium] wp_slhdsa_export_object treats the decoder reference as the key pointer instead of dereferencing it —
src/wp_slhdsa_kmgmt.c:1917-1922 - [Medium] wp_slhdsa_export invokes the caller-supplied paramCb while still holding keyMutex —
src/wp_slhdsa_kmgmt.c:898-957 - [Medium] hasPub/hasPriv bookkeeping is inconsistent: wp_slhdsa_dup never derives them from the key flags, and wp_slhdsa_import clears them on failures that never touched the key —
src/wp_slhdsa_kmgmt.c:483-491 - [Medium] SLH-DSA test coverage gaps: the OpenSSL 3.5+ message API, the SIGNATURE param branch, verify-only builds, and the pure/raw rejection path are all untested —
test/test_slhdsa.c:29-30 - [Medium] One-shot sign/verify rejects messages over 64 MiB using the streaming-buffer cap —
src/wp_slhdsa_sig.c:378-383 - [Medium] wp_dh_fix_pki_len only corrects the 0x81 short form, and indexes keyData[1]/[2] with no minimum-length guard —
src/wp_dh_kmgmt.c:2690-2695 - [Medium] New refuse-cipher-on-traditional guard in ECC/RSA is untested and turns a common CLI invocation into a silent failure —
src/wp_rsa_kmgmt.c:3558-3564 - [Medium] PQC example still hardcodes relative ".libs" while openssl_example.c was fixed to resolve from argv[0] —
examples/pqc_openssl_example.c:200-217 - [Medium] The SHAKE/SHA2 parameter-set split is not respected outside the example: the interop algorithm list is ungated and the README still recommends plain --enable-slhdsa —
test/standalone/tests/pqc_interop/test_pqc_interop.c:1011-1018 - [Medium] The new DH key mutex is one-sided: it covers the import but none of the readers, and one encode path skips the sync helper entirely —
src/wp_dh_kmgmt.c:2584-2590, 2656-2687 - [Low] wp_slhdsa_encode locks before any NULL check on slhdsa, and ends with an unreachable second unlock —
src/wp_slhdsa_kmgmt.c:1778-1789 - [Low] wp_slhdsa_get_params now reports success with return_size 0 when the component is absent, diverging from wp_mldsa_get_params —
src/wp_slhdsa_kmgmt.c:1042-1044 - [Low] SLH-DSA keymgmt provides no OSSL_FUNC_KEYMGMT_VALIDATE despite wc_SlhDsaKey_CheckKey being available —
src/wp_slhdsa_kmgmt.c:1357-1383 - [Low] wp_cipher_from_params does not clear cipher/cipherName on the non-UTF8 param path, contradicting its own new comment —
src/wp_internal.c:925-927 - [Low] wp_encrypt_key_pkcs8 assigns the wolfSSL-returned length without checking it fits the caller's buffer —
src/wp_internal.c:1142 - [Low] verifySig is cleared in verify_message_init but not in the shared wp_slhdsa_init, which the re-init fix otherwise now resets —
src/wp_slhdsa_sig.c:631-640 - [Low] PKCS#8 helper hygiene: raw HAVE_PKCS8/NO_PWDBASED test instead of the new WP_HAVE_PKCS8_ENC macro, and an RNG unlock coupled to the shared ok flag —
src/wp_internal.c:994 - [Low] Doc comment on wp_mlkem_kem_encapsulate contradicts the new NULL-output behaviour —
src/wp_mlkem_kem.c:186 - [Low] Rejected SLH-DSA signing randomizer left in the signature context without cleansing —
src/wp_slhdsa_sig.c:778-798 - [Low] wp_slhdsa_free permanently abandons the key object when the refcount mutex cannot be acquired —
src/wp_slhdsa_kmgmt.c:373-402
Skipped findings
- [Medium] Three tests pass without exercising what they claim: allocation failures read as success, a decapsulate case never reaches the provider, and the ECX EPKI assertions are compiled out everywhere
- [Low] CI and script hygiene: redundant apt installs, example steps ahead of the ORAS cache push, a hardcoded EXPECTED_TESTS count, and a stale .dockerignore entry
- [Low] The -Wmaybe-uninitialized locals flagged in the previous review are still uninitialized
- [Info] Two comment lines in wp_slhdsa_dup exceed the 80-column guideline
Review generated by Skoll via Claude/Codex
| * @return 1 on success. | ||
| * @return 0 on failure. | ||
| */ | ||
| static int wp_slhdsa_export_object(wp_SlhDsaEncDecCtx* ctx, wp_SlhDsa* slhdsa, |
There was a problem hiding this comment.
🟡 [Medium] wp_slhdsa_export_object treats the decoder reference as the key pointer instead of dereferencing it
💡 SUGGEST api
OSSL_FUNC_decoder_export_object_fn is int (*)(void *ctx, const void *objref, size_t objref_sz, OSSL_CALLBACK *export_cb, void *export_cbarg). OpenSSL passes objref = the data of the OSSL_OBJECT_PARAM_REFERENCE octet string, whose contents are the key pointer, and objref_sz = sizeof(void*). OpenSSL's own der2key_export_object does keydata = *(void **)reference; after checking reference_sz == sizeof(keydata). This function binds the second argument as wp_SlhDsa* slhdsa and passes it straight to wp_slhdsa_export, so it operates on the address of the stored pointer, not the key. The file demonstrates the correct convention one function away: wp_slhdsa_load (lines 520-526) takes const wp_SlhDsa** pSlhDsa and dereferences it. IMPORTANT SCOPE NOTE: I verified that wp_ecc_export_object (src/wp_ecc_kmgmt.c:3121), wp_mldsa_export_object (src/wp_mldsa_kmgmt.c:1664), wp_ecx_export_object (src/wp_ecx_kmgmt.c:2342) and wp_rsa_export_object (src/wp_rsa_kmgmt.c:3714) all use the identical pattern on master. This is therefore established repo-wide convention, not a regression introduced by this PR, which is why it is filed as SUGGEST rather than BLOCK. Reachability is limited to the cross-provider decode path (decoder from wolfProvider, keymgmt fetched from another provider — realistic since OpenSSL 3.6 also ships SLH-DSA). Secondary point: OpenSSL substitutes OSSL_KEYMGMT_SELECT_ALL when the ctx selection is 0; here selection 0 falls through wp_slhdsa_export to paramCb with an all-zero OSSL_PARAM array, a silent success producing an empty key.
Suggestion:
| static int wp_slhdsa_export_object(wp_SlhDsaEncDecCtx* ctx, wp_SlhDsa* slhdsa, | |
| static int wp_slhdsa_export_object(wp_SlhDsaEncDecCtx* ctx, | |
| const void* reference, size_t size, OSSL_CALLBACK* exportCb, | |
| void* exportCbArg) | |
| { | |
| wp_SlhDsa* slhdsa; | |
| int selection = ctx->selection; | |
| if (size != sizeof(slhdsa)) { | |
| return 0; | |
| } | |
| /* The contents of the reference is the address of the key object. */ | |
| slhdsa = *(wp_SlhDsa* const*)reference; | |
| if (selection == 0) { | |
| selection = OSSL_KEYMGMT_SELECT_ALL; | |
| } | |
| return wp_slhdsa_export(slhdsa, selection, exportCb, exportCbArg); | |
| } |
Recommendation: Since this is a repo-wide pattern, fix it once across all five kmgmt files in a separate change rather than blocking this PR. If it is deliberate (e.g. the repo relies on the reference never being used cross-provider), add a comment saying so, because the new file reads as if it independently chose the wrong convention.
| if (!wolfssl_prov_is_running() || (slhdsa == NULL)) { | ||
| ok = 0; | ||
| } | ||
| if (ok && (wp_lock(wp_slhdsa_get_mutex(slhdsa)) != 1)) { |
There was a problem hiding this comment.
🟡 [Medium] wp_slhdsa_export invokes the caller-supplied paramCb while still holding keyMutex
💡 SUGGEST bug
The key mutex is taken at line 898 and only released at line 956, so the arbitrary caller callback at line 950 runs under the lock. wolfSSL mutexes are not recursive, and every other SLH-DSA keymgmt entry point takes the same keyMutex: wp_slhdsa_has (548), wp_slhdsa_import (741), wp_slhdsa_get_params (1005), wp_slhdsa_match (606/613), wp_slhdsa_encode (1781), wp_slhdsa_dup (428). Any callback that touches the same EVP_PKEY (EVP_PKEY_get_octet_string_param, EVP_PKEY_eq, a nested encode) self-deadlocks with no diagnostic. Nothing needs the lock at that point: pubBuf and privBuf are private heap copies made under the lock, so the key can be unlocked before the callback runs. This is new exposure relative to wp_mldsa_kmgmt.c and wp_ecx_kmgmt.c, which take no key lock at all.
Suggestion:
| if (ok && (wp_lock(wp_slhdsa_get_mutex(slhdsa)) != 1)) { | |
| if (ok && (paramsSz == 0) && (expPub || expPriv)) { | |
| ok = 0; | |
| } | |
| /* Buffers are private copies; drop the lock before running caller code. */ | |
| if (locked) { | |
| wp_unlock(wp_slhdsa_get_mutex(slhdsa)); | |
| locked = 0; | |
| } | |
| if (ok) { | |
| ok = paramCb(params, cbArg); | |
| } | |
| OPENSSL_free(pubBuf); | |
| OPENSSL_clear_free(privBuf, privAllocLen); |
Recommendation: Release keyMutex after the key material has been copied out and before calling paramCb.
| ok = 0; | ||
| } | ||
| } | ||
| if (ok) { |
There was a problem hiding this comment.
🟡 [Medium] hasPub/hasPriv bookkeeping is inconsistent: wp_slhdsa_dup never derives them from the key flags, and wp_slhdsa_import clears them on failures that never touched the key
💡 SUGGEST bug
Sites: src/wp_slhdsa_kmgmt.c:483-491; src/wp_slhdsa_kmgmt.c:808-812
After wc_SlhDsaKey_ImportPrivate succeeds, dup only sets dst->hasPriv. Everywhere else in the file the code recognises that a FIPS 205 4n private key carries the public half and derives hasPub from the key's own flags: wp_slhdsa_import lines 766-769 (slhdsa->hasPub = ((slhdsa->key.flags & WC_SLHDSA_FLAG_PUBLIC) != 0) ? 1 : 0;) and wp_slhdsa_decode lines 1706-1709 with the same comment. Consequence: a dup driven with selection == OSSL_KEYMGMT_SELECT_PRIVATE_KEY, or of a source whose hasPub happens to be 0, returns a key that signs correctly but reports hasPub == 0, so wp_slhdsa_has(PUBLIC_KEY) fails, wp_slhdsa_export skips the public (line 906) and wp_slhdsa_get_params reports return_size 0 for OSSL_PKEY_PARAM_PUB_KEY (line 1042). test_slhdsa_dup only uses EVP_PKEY_dup, which passes OSSL_KEYMGMT_SELECT_ALL, so this path is untested.
Also, at src/wp_slhdsa_kmgmt.c:808-812 (Low, review scan) — wp_slhdsa_import clears hasPub and hasPriv on failures that never touched the key:
The failure cleanup runs for every ok == 0 path, including ones that never modified the key: wolfssl_prov_is_running() false (735), an unsupported selection (738), a failed wp_lock (741), and the 'neither priv nor pub supplied' rejection (795). A keymgmt object that already held a valid key is therefore downgraded to hasPub = hasPriv = 0 by an import call that was rejected before doing anything. In the common OpenSSL flow import runs on a freshly-new'd object so this is benign, but the object is not documented as single-use and wp_slhdsa_enc_import_object is the only in-tree caller that guarantees it. wp_mldsa_import has the same shape, so this is a shared convention question rather than a regression.
Suggestion:
| if (ok) { | |
| if (ok) { | |
| dst->hasPriv = 1; | |
| /* The 4n private key carries the public half. */ | |
| if ((dst->key.flags & WC_SLHDSA_FLAG_PUBLIC) != 0) { | |
| dst->hasPub = 1; | |
| } | |
| } |
Recommendation: Set hasPub from WC_SLHDSA_FLAG_PUBLIC after ImportPrivate, matching wp_slhdsa_import and wp_slhdsa_decode, and add a dup test that selects the private key only.
[src/wp_slhdsa_kmgmt.c:808-812] Only clear the flags when a wc_SlhDsaKey_Import* call actually ran, so an early-rejected import leaves prior state intact.
| #include <openssl/decoder.h> | ||
| #include <openssl/x509.h> | ||
|
|
||
| #if defined(WP_HAVE_SLHDSA) && defined(WP_HAVE_SLHDSA_PRIVATE) && \ |
There was a problem hiding this comment.
🟡 [Medium] SLH-DSA test coverage gaps: the OpenSSL 3.5+ message API, the SIGNATURE param branch, verify-only builds, and the pure/raw rejection path are all untested
💡 SUGGEST test
Sites: test/test_slhdsa.c:29-30; src/wp_slhdsa_sig.c:611-649; src/wp_slhdsa_sig.c:527-530
include/wolfprovider/settings.h explicitly supports a verify-only wolfSSL ('A verify-only wolfSSL compiles out keygen, sign and every private-key entry point, so the provider offers verify only against it' — WP_HAVE_SLHDSA_PRIVATE is left undefined when WOLFSSL_SLHDSA_VERIFY_ONLY is set). The entire new 1511-line test file, the unit.c registrations and the pqc_interop coverage are all gated on WP_HAVE_SLHDSA_PRIVATE, so that supported configuration ships with zero automated coverage. Specifically unexercised: wp_slhdsa_key_types with a private-only selection silently returns the 'none' entry (idx stays 0) in that build; the SPKI-only decode/encode path; public-key import; EVP_DigestVerify; and get_params. src/wp_slhdsa_kmgmt.c gates a large amount of code on this macro — the key_types index arithmetic (848-854), the private half of import (747-775), export (923-945) and match (662-697), the entire keygen block (1139-1315), the GEN_ dispatch entries (1324-1344), the PKI decoder (2075-2093) and all four private encoder tables (2132-2144). No CI row builds a verify-only wolfSSL either. Note: the wp_wolfprov.c registration gates were checked and DO match the kmgmt definitions exactly for both configurations, so this is a coverage gap rather than a link break.
Also, at src/wp_slhdsa_sig.c:611-649 (Medium, review scan) — OpenSSL 3.5+ signature message API and the SIGNATURE param branch are entirely untested:
The dispatch table registers SIGN_MESSAGE_INIT/UPDATE/FINAL and VERIFY_MESSAGE_INIT/UPDATE/FINAL (lines 889-905), and wp_slhdsa_set_ctx_params:832-849 implements OSSL_SIGNATURE_PARAM_SIGNATURE, which is the only way verify_message_final can ever get a signature. Grepping test/test_slhdsa.c for sign_message_init, verify_message_init, EVP_PKEY_sign_message, EVP_PKEY_verify_message and SIGNATURE_PARAM_SIGNATURE returns nothing. So wp_slhdsa_verify_message_init, wp_slhdsa_verify_message_final, wp_slhdsa_sign_message_final and the whole verifySig lifecycle (allocation via OSSL_PARAM_get_octet_string with max=0, the p->data_size == sig_size guard, the free-then-replace, the memdup in dupctx:236-244, the free in freectx:194) are never exercised by CI. This is the one part of the file with non-trivial heap ownership and it has no coverage. The code itself is correct here (wp_slhdsa_verify_message_final takes only ctx, matching OSSL_FUNC_signature_verify_message_final, unlike wp_mldsa_verify_message_final which takes three args), which makes the absence of a test that would catch that class of mistake more notable.
Also, at src/wp_slhdsa_sig.c:527-530 (Low, review scan) — wp_slhdsa_check_pure rejection path has no negative test:
wp_slhdsa_check_pure gates both digest_sign_init (line 539) and digest_verify_init (line 551) on the caller not naming a pre-hash digest, since FIPS 205 HashSLH-DSA is a separate algorithm. The accept side is covered: test_slhdsa_x509_sign_verify calls X509_sign(cert, key, NULL) (test/test_slhdsa.c:1447), which reaches digest_sign_init with mdName NULL, and every other test uses EVP_DigestSignInit_ex with a NULL EVP_MD. Nothing in test_slhdsa.c ever passes a non-NULL digest name (no EVP_sha256 or 'SHA256' string appears in the file), so the reject path is unverified. A regression that silently ignored mdName would produce signatures labelled pure SLH-DSA over pre-hashed input and no test would fail.
Suggestion: Split test_slhdsa.c so the public-only tests (SPKI encode/decode, verify with an imported public key, get_params, match, has) build under #if defined(WP_HAVE_SLHDSA) and only the keygen/sign/PKI/EPKI tests require WP_HAVE_SLHDSA_PRIVATE, seeding the public-only tests from a hard-coded SPKI DER vector plus a known-good signature.
Recommendation: Add coverage for the verify-only configuration and a CI row that builds it; today the second half of every #ifdef in the new kmgmt file is compiled but never run.
[src/wp_slhdsa_sig.c:611-649] Cover the 3.5+ message API and the SIGNATURE param branch before merge; they are dispatched to applications but have zero test coverage.
[src/wp_slhdsa_sig.c:527-530] Add a negative test for a named digest on both the sign and verify init paths.
| if (sigSize < sigSz) { | ||
| ok = 0; | ||
| } | ||
| /* wolfSSL's SLH-DSA API takes a 32-bit message length. Reject >4 GiB |
There was a problem hiding this comment.
🟡 [Medium] One-shot sign/verify rejects messages over 64 MiB using the streaming-buffer cap
💡 SUGGEST api
WP_SLHDSA_BUF_MAX is introduced at line 41 to bound the accumulated message buffer: 'wolfSSL has no SLH-DSA streaming API, so bound the buffered message.' But the same cap is also applied to msgLen on the one-shot paths, where nothing is buffered and the message lives entirely in the caller's own allocation. wp_slhdsa_sign:380-383 rejects msgLen > WP_SLHDSA_BUF_MAX, and wp_slhdsa_verify:487-491 does the same. The equivalent sibling wp_mldsa_sig.c applies WP_MLDSA_BUF_MAX only inside wp_mldsa_buf_append; wp_mldsa_sign:445 and wp_mldsa_verify:533 check only WP_FITS_WORD32. The underlying wolfSSL API takes a word32 length, so the true limit is 4 GiB. Net effect: EVP_PKEY_sign / EVP_PKEY_verify on a 100 MiB caller-owned buffer fails with no diagnostic, and only for SLH-DSA.
Suggestion: Drop (msgLen > WP_SLHDSA_BUF_MAX) from wp_slhdsa_sign:380-383 and wp_slhdsa_verify:487-491, keeping the cap where it belongs: inside wp_slhdsa_buf_append (line 97). The comment above the check already only claims to be guarding the word32 conversion.
Recommendation: Limit WP_SLHDSA_BUF_MAX to the buffered/streaming path, matching wp_mldsa_sig.c, or make the policy explicit and consistent across both PQC signature files.
| } | ||
| #endif | ||
|
|
||
| static int wp_slhdsa_verify_message_init(wp_SlhDsaSigCtx* ctx, |
There was a problem hiding this comment.
🔵 [Low] verifySig is cleared in verify_message_init but not in the shared wp_slhdsa_init, which the re-init fix otherwise now resets
🔧 NIT convention
wp_slhdsa_init (lines 284-290) resets every other piece of per-operation state on re-init: the message buffer, context, testEntropy, deterministic and rawMsg. It does not touch verifySig. Only wp_slhdsa_verify_message_init does, via a wrapper that frees it before delegating to wp_slhdsa_message_init. So a context that ran the 3.5+ verify-message flow and is then re-inited through wp_slhdsa_verify_init or wp_slhdsa_digest_verify_init keeps the previous signature buffer alive until freectx. This is not a leak (freectx:194 frees it) and not a correctness bug, because OpenSSL only routes verify_message_final to a context whose operation is EVP_PKEY_OP_VERIFYMSG, which requires verify_message_init and therefore the clear. It is still an inconsistency: the state reset lives in two places and the special-case wrapper exists solely to patch the gap.
Suggestion: Move the three verifySig lines into wp_slhdsa_init alongside the other resets at lines 284-290 (before the wp_slhdsa_set_ctx_params call, so an init-time OSSL_SIGNATURE_PARAM_SIGNATURE still lands), then point OSSL_FUNC_SIGNATURE_VERIFY_MESSAGE_INIT straight at wp_slhdsa_message_init and delete the wrapper.
Recommendation: Consolidate the verifySig reset into wp_slhdsa_init so every init path clears the same state.
| @@ -987,6 +1004,13 @@ int wp_encrypt_key_pkcs8_size(WOLFPROV_CTX* provCtx, int cipher, | |||
| if (cipher == 0) { | |||
There was a problem hiding this comment.
🔵 [Low] PKCS#8 helper hygiene: raw HAVE_PKCS8/NO_PWDBASED test instead of the new WP_HAVE_PKCS8_ENC macro, and an RNG unlock coupled to the shared ok flag
🔧 NIT convention
Sites: src/wp_internal.c:994; src/wp_internal.c:1007-1030, 1110-1135
The PR introduces WP_HAVE_PKCS8_ENC (include/wolfprovider/settings.h:47-50) as defined(HAVE_PKCS8) && !defined(NO_PWDBASED) and converts every kmgmt call site from WOLFSSL_ENCRYPTED_KEYS to it. The four bodies in wp_internal.c that the macro exists for (lines 994, 1074, 1172, 1214) still spell the condition out literally, and wp_cipher_from_params two functions above already uses the macro. The macro definition itself checks out: I confirmed wc_EncryptPKCS8Key_ex, wc_EncryptPKCS8Key and wc_DecryptPKCS8Key in wolfSSL master sit inside #ifndef NO_PWDBASED (asn.c:10012) nested in #ifdef HAVE_PKCS8 (asn.c:10630), with no WOLFSSL_ENCRYPTED_KEYS dependency, and PKCS8_ENC_PRIVATEKEY_TYPE in wc_PemGetHeaderFooter (asn.c:25567) is unguarded, so the swap in wp_dec_pem2der.c is safe.
Also, at src/wp_internal.c:1007-1030, 1110-1135 (Info, concurrency scan) — RNG unlock in the PKCS#8 encrypt helpers is coupled to the shared ok flag instead of an explicit locked flag:
This PR added an RNG lock acquisition to wp_encrypt_key_pkcs8_size and converted the one in wp_encrypt_key_pkcs8 to a checked acquisition. Both were traced and are currently correct — there is no lock leak. Specifically, the suspected #if/#else asymmetry does not exist: the wp_provctx_unlock_rng call sits after the #endif closing the wolfSSL-version #if, so it compiles into both the wc_EncryptPKCS8Key_ex and wc_EncryptPKCS8Key branches, and the if (rc != LENGTH_ONLY_E) error check comes after the unlock. There are no early returns between lock and unlock in either function.
The fragility is structural: both functions rely on the invariant "ok is still 1 at the if (ok) block, therefore the lock is held, therefore the unlock inside runs." Any future validation inserted between the lock acquisition and that block, or any early return added inside it, will silently leak the provider-global, non-recursive RNG mutex (wc_InitMutex, wp_wolfprov.c:236) and permanently deadlock every RNG consumer in the process — including wc_AesGcmSetIV on the TLS path (wp_aes_aead.c) and the AES-CBC IV draw (wp_aes_block.c). Every other lock site in this PR uses the explicit-flag pattern with an unconditional cleanup unlock (wp_slhdsa_kmgmt.c:1781-1786/1898-1900, wp_dh_kmgmt.c:2660-2683); these two do not.
Suggestion: Replace the four literal tests in wp_internal.c with #ifdef WP_HAVE_PKCS8_ENC so there is a single definition of the condition.
Recommendation: Use the new macro consistently in wp_internal.c.
[src/wp_internal.c:1007-1030, 1110-1135] Adopt the same explicit-flag discipline used elsewhere in this PR: track int locked = 0; set immediately after a successful wp_provctx_lock_rng, and place an unconditional if (locked) { wp_provctx_unlock_rng(provCtx); } in a cleanup tail at the end of each function, so the unlock cannot be bypassed by a future early exit.
| @@ -211,9 +211,9 @@ static int wp_mlkem_kem_encapsulate(wp_MlKemCtx* ctx, unsigned char* out, | |||
| ctSize = wp_mlkem_data_ct_size(data); | |||
There was a problem hiding this comment.
🔵 [Low] Doc comment on wp_mlkem_kem_encapsulate contradicts the new NULL-output behaviour
🔧 NIT style
The function header still says 'If out or secret is NULL, just report the output sizes.' After this PR only out == NULL is a size query (line 216); secret == NULL with out != NULL is now an explicit failure (line 226). The inline comment at 214-215 was updated but the Doxygen block above it was not, so the two comments in the same function now disagree. wp_mlx_kem.c does not carry the same stale line.
Suggestion:
| ctSize = wp_mlkem_data_ct_size(data); | |
| * If out is NULL, report the output sizes and return success. |
Recommendation: Update the header comment to match the implemented contract.
| ctx->contextLen = len; | ||
| } | ||
| } | ||
| if (ok) { |
There was a problem hiding this comment.
🔵 [Low] Rejected SLH-DSA signing randomizer left in the signature context without cleansing
🔧 NIT
When handling OSSL_SIGNATURE_PARAM_TEST_ENTROPY, vp is pointed directly at the context-resident buffer ctx->testEntropy, so OSSL_PARAM_get_octet_string() copies the caller-supplied FIPS 205 addrnd randomizer into ctx->testEntropy before the length is validated. If the subsequent check len != wp_slhdsa_get_n(ctx->slhdsa) fails, the function sets ok = 0 and resets ctx->testEntropyLen = 0, but never wipes the bytes already copied into ctx->testEntropy. The randomizer therefore persists in the heap-allocated signature context after the operation was explicitly rejected.
The path is reachable and exercised: the PR's own new unit test test_slhdsa_sig_params (test/test_slhdsa.c) drives it with a deliberately undersized n - 1 byte randomizer to assert the rejection.
The sibling handler introduced in the same PR — wp_slhdsa_gen_set_params() in src/wp_slhdsa_kmgmt.c:1265-1277 — handles the structurally identical case for OSSL_PKEY_PARAM_SLH_DSA_SEED correctly, calling OPENSSL_cleanse(ctx->seed, sizeof(ctx->seed)) on the rejection branch. That contrast is the second independent signal that the author regards this class of value as requiring a wipe on rejection, and that the signature-side handler simply omits it.
Exposure is bounded: the stale bytes are overwritten by wc_ForceZero(ctx->testEntropy, sizeof(ctx->testEntropy)) on the next wp_slhdsa_init(), and the whole context is OPENSSL_clear_free'd in wp_slhdsa_freectx(). Combined with the fact that TEST_ENTROPY is a test-only hedging randomizer rather than long-term key material, real-world impact is minimal — extraction would require local access and heap-forensics tooling within the window before re-init or free.
Recommendation: Mirror the seed handler in wp_slhdsa_kmgmt.c: add wc_ForceZero(ctx->testEntropy, sizeof(ctx->testEntropy)); (or OPENSSL_cleanse) on the rejection branch, so any partially-copied randomizer is wiped before returning failure. Concretely, replace the trailing if (ok) { ctx->testEntropyLen = len; } with an if/else that cleanses the full sizeof(ctx->testEntropy) when ok == 0. Wipe the full buffer size rather than len, since OSSL_PARAM_get_octet_string may have written up to sizeof(ctx->testEntropy) bytes before the length check ran.
| * | ||
| * @param [in, out] slhdsa SLH-DSA key object. May be NULL. | ||
| */ | ||
| void wp_slhdsa_free(wp_SlhDsa* slhdsa) |
There was a problem hiding this comment.
🔵 [Low] wp_slhdsa_free permanently abandons the key object when the refcount mutex cannot be acquired
🔧 NIT
When wc_LockMutex(&slhdsa->refMutex) fails, the code deliberately sets cnt = 1 so the refcount is not decremented and the object is not freed. The comment states the intent ("Cannot safely decrement without the lock; keep the object"), but the consequence is that the wp_SlhDsa — including the SlhDsaKey holding the FIPS 205 private key, which for the 256-bit parameter sets is a multi-kilobyte allocation — is leaked permanently, with no retry path and no error reported to the caller. wp_slhdsa_free returns void, so nothing upstream can observe or recover from this.
A mutex lock failure is rare, but it is exactly the kind of degraded condition (EDEADLK, EINVAL after a wc_FreeMutex race, or resource exhaustion) that repeats, so a process hitting it once under load will typically hit it repeatedly and accumulate leaked private key material. Note the asymmetry: wp_slhdsa_up_ref correctly propagates lock failure as a return of 0, so the increment path fails loudly while the decrement path fails silently.
Recommendation: Log the lock failure at minimum so the leak is diagnosable, and consider an atomic refcount (or OpenSSL's CRYPTO_REF_COUNT / CRYPTO_DOWN_REF) so the decrement cannot fail. If the mutex is retained, treat a lock failure on the final release as a fatal provider condition rather than a silent abandonment of key material.
Uh oh!
There was an error while loading. Please reload this page.