wc_pkcs11: add ML-KEM PKCS#11 integration#1
Conversation
Frauschi
left a comment
There was a problem hiding this comment.
One additional note: When ML-KEM and PKCS#11 are enabled, then the option WOLFSSL_TLSX_PQC_MLKEM_STORE_OBJ is required to ensure that a generated ephemeral ML-KEM private key on the TLS client side is kept alive until the decapsulation is done. Otherwise, the key on the token would be deleted after the key generation.
| #ifdef WOLF_CRYPTO_CB | ||
| /* Cache device id - not used in this algorithm yet. */ | ||
| key->devCtx = NULL; | ||
| /* Cache device id. */ |
There was a problem hiding this comment.
remove the comment, unnecessary
| #endif /* WOLFSSL_HAVE_MLKEM */ | ||
|
|
||
| #endif /* WOLF_CRYPT_MLKEM_H */ | ||
|
|
There was a problem hiding this comment.
Leave the last empty line as is
| #ifndef MLKEM_MAX_LABEL_LEN | ||
| #define MLKEM_MAX_LABEL_LEN 32 | ||
| #endif | ||
| #endif |
There was a problem hiding this comment.
These are also defined unconditionally in mlkem.h. I like the check whether the user overwrote them. But do this only once.
| PKCS11_KEY_TYPE_HMAC, | ||
| PKCS11_KEY_TYPE_RSA, | ||
| PKCS11_KEY_TYPE_EC, | ||
| PKCS11_KEY_TYPE_MLKEM, |
There was a problem hiding this comment.
add new entry at the end, not in the middle of the enum
| #ifndef WOLFSSL_NO_ML_KEM | ||
| case WC_ML_KEM_512: | ||
| #endif | ||
| #ifdef WOLFSSL_MLKEM_KYBER |
There was a problem hiding this comment.
We don't need support for the pre-FIPS203 Kyber version in the PKCS#11 implementation. Remove all the WOLFSSL_MLKEM_KYBER additions.
| return ret; | ||
| } | ||
|
|
||
| static int Pkcs11MlKemPubSet(const MlKemKey* key) |
There was a problem hiding this comment.
Please remove these small helper methods and instead add the relevant checks inline directly. You can also remove the support for ext_mlkem then overall and only stick to the wolfssl internal mlkem implementation.
| { CKA_CLASS, &privKeyClass, sizeof(privKeyClass) }, | ||
| { CKA_DECAPSULATE, &ckTrue, sizeof(ckTrue) }, | ||
| { CKA_KEY_TYPE, &mlkemKeyType, sizeof(mlkemKeyType) }, | ||
| { CKA_PARAMETER_SET, ¶m_set, sizeof(param_set) }, |
There was a problem hiding this comment.
As per chapter 6.68.3 "ML-KEM private key objects" of the PKCS#11 version 3.2 standard, the ML-KEM private key template does not contain the CKA_PARAMETER_SET attribute for key pair generation, as this information is already in the public key template. so please remove it here.
| rv = functionList->C_EncapsulateKey(session->handle, &mech, publicKey, | ||
| sharedKeyTempl, sharedKeyTmplCnt, | ||
| &sharedKey, | ||
| (CK_BYTE_PTR)info->pk.pqc_encaps.ciphertext, |
There was a problem hiding this comment.
this looks ugly, please format differently
| mech.pParameter = NULL; | ||
|
|
||
| rv = functionList->C_DecapsulateKey(session->handle, &mech, privateKey, | ||
| (CK_BYTE_PTR)info->pk.pqc_decaps.ciphertext, |
There was a problem hiding this comment.
also ugly, format differently
Add PKCS#11 integration for ML-KEM with key generation, encapsulation and decapsulation support through the crypto callback path.\n\nIncludes ML-KEM PKCS#11 constants/types, key store handling, token object lifecycle management, and ML-KEM key init helpers for private-key ID/label workflows.\n\nAlign implementation details with current upstream conventions and review feedback:\n- internal wolfCrypt ML-KEM path only for PKCS#11\n- inline ML-KEM key-type/flag checks in PKCS#11 code\n- proper key template formatting and enum placement\n- remove CKA_PARAMETER_SET from ML-KEM private key template in key generation\n- ensure TLS ML-KEM object storage behavior is compatible with PKCS#11 ephemeral-key decapsulation flow
|
Closing this fork-local PR after squashing into one commit. I will open a new PR upstream later. |
- Fix small-variant signing regression (review finding bonus): reverting word16 sum -> unsigned int in wc_lmots_q_expand broke the small-variant checksum loop, which relies on arithmetic wrapping at 2^16. Restore word16 sum with explicit (word16) casts at every assignment so the -Wconversion warning is silenced without changing semantics. Caught by the new runtime test job (also added). - Fix 16-bit shift UB (review #1): the matrix doesn't cover 16-bit, but on word32 = unsigned long targets `1U << params->height` is undefined for height >= 16. Restore (word32)1U on every shift whose exponent can reach height (LMS_Q_AT_LEVEL macro and four open-coded sites). - Add msgSz <= 0 check to wc_LmsKey_Verify (review #2): wc_LmsKey_Sign already rejects non-positive msgSz; the public verify entry point did not. Without it, the (word32)msgSz cast forwarded a huge value to wc_hss_verify and caused buffer over-read. - Add a test_lms_runtime CI job (review #11): the existing matrix only builds; it cannot catch a semantic regression like the q_expand one above. The new job builds --enable-lms and --enable-lms=yes,small and runs testwolfcrypt for both. - Make wc_hss_make_key loop index word32 (review #10) so the (int)cast on HSS_MAX_LEVELS in the second-loop bound is no longer needed. - Add LMOTS_Y_LEN as the canonical name for the LM-OTS y[] length and alias LMS_PRIV_Y_TREE_LEN to it (review #6); switch the non-cache call sites in wc_hss_sign / wc_lms_sig_copy / wc_hss_sign_build_sig to LMOTS_Y_LEN to match intent. Verified: 15 matrix rows still build clean under the conversion flags; testwolfcrypt LMS Vfy / LMS pass for --enable-lms, --enable-lms=yes,small, and --enable-lms=yes,small,sha256-192,shake256; bench_lms shows no regression. https://claude.ai/code/session_01EJmy1bKDgHseTwZ5Qqpu1g
Negative findings from review of 3b2d711: - Drop redundant `(word16)` inner cast in `wc_xmss_impl.c` (#1): `(word16)((word16)hs * n)` -> `(word16)(hs * n)`. The inner cast added nothing; word8 promotes to int regardless. - Normalize `(word32)1` to `(word32)1U` across the file (#5) so the pre-existing call sites match the style of the new shifts. - Defensive guard in `wc_xmss_hash_message` (#2): if `idx_len > params->n` ever holds, the explicit `(word32)(params->n - idx_len)` cast that silenced the warning would otherwise produce a ~4 GB XMEMSET. Set state->ret = WC_FAILURE and bail; the invariant is structural for valid parameter sets. - Defensive guard in `wc_idx_copy` (#3): if `dl < sl` is ever passed, the word32 subtraction wraps and the XMEMSET corrupts memory. Same structural invariant; early return rather than crash. - Extend `test_xmss_runtime` (#7, #8) from 2 to 4 configurations: 1. --enable-xmss (default) 2. --enable-xmss=yes,small 3. --enable-xmss=yes,verify-only (NEW: RFC 8391 test vectors) 4. --enable-xmss --enable-32bit -m32 (NEW: catches 32-bit width-dependent bugs in tree-index arithmetic; XmssIdx narrows to word32 there) The 32-bit row needs gcc-multilib so the job now installs it. Verified locally: - All 13 build_library matrix rows compile clean under the conversion flags. - testwolfcrypt's "XMSS Vfy" / "XMSS" pass for --enable-xmss, --enable-xmss=yes,small, --enable-xmss=yes,verify-only, and --enable-xmss --enable-32bit (4/4). - bench_xmss_xmssmt re-run with `-DBENCH_MIN_RUNTIME_SEC=5.0F` for longer averaging. Sign/verify deltas range -10% to +15% with no coherent regression pattern across parameter sets (the largest moves in either direction are on neighbouring rows of the same hash family), consistent with shared-system run-to-run noise rather than a real perf change. Single-sample keygens (1 op per measurement) carry expectedly high variance (-7% to +57%); sign/verify with hundreds to thousands of ops per measurement are the meaningful signal. https://claude.ai/code/session_01EJmy1bKDgHseTwZ5Qqpu1g
1. BasicConstraints pathLenConstraint absent vs. 0 — get_ext_d2i/set_ext/V3_EXT_d2i now distinguish "no constraint" from 0 per RFC 5280 §4.2.1.9, using the existing basicConstPlSet flag. 2. GENERAL_NAME_print GEN_DIRNAME — added missing return-value normalization so the directory name is actually printed (was emitting only DirName:). 3. GENERAL_NAME_print GEN_DNS — use ASN1_STRING_print like the EMAIL/URI cases, avoiding NULL-strData deref and NUL-truncation. 4. X509_print BasicConstraints — print , pathlen:N to match OpenSSL. 5. X509_print Extended Key Usage — print Any Extended Key Usage (was omitted). 6. get_ext_d2i CRL_DIST_OID double-free — null gn immediately after ownership transfers to dp, so an error from the next push doesn't free it twice. 7. X509V3_EXT_print SAN truncation/failure — match XSNPRINTF size cap to the allocation; was truncating at indent==1 and failing at indent>=2. 8. X509V3_EXT_print AUTH_KEY/SUBJ_KEY NULL deref — NULL-check i2s_ASN1_STRING return before passing to %s. 9. X509_add_ext SAN type confusion — reject DIRNAME/RID/X400/EDIPARTY; only the ASN1_STRING*-backed types are read via gn->d.ia5. Was performing a wild-pointer XMEMCPY in add_altname_ex. Also: extracted the SAN and WOLFSSL_CUSTOM_OID arms of X509_add_ext into static helpers (behavior-preserving). Regression tests added for #1–5 and #9; existing GENERAL_NAME_print test hardened (gives GEN_DIRNAME a real directoryName, eliminating an OOB read that the print fix would otherwise expose).
Description
Add PKCS#11 support for ML-KEM key encapsulation/decapsulation in wolfSSL, aligned with the upstream ML-DSA integration approach.
What this includes
CKK_ML_KEM,CKM_ML_KEM*,CKA_ENCAPSULATE,CKA_DECAPSULATE, parameter sets, mechanism flags).PKCS11_KEY_TYPE_MLKEMhandling inwc_Pkcs11StoreKey.WC_PK_TYPE_PQC_KEM_KEYGEN)WC_PK_TYPE_PQC_KEM_ENCAPS)WC_PK_TYPE_PQC_KEM_DECAPS)WC_ALGO_TYPE_FREEcallback path.wc_MlKemKey_Init_Id/wc_MlKemKey_Init_LabelAPIs and Kyber aliases for private-key-ID workflows.NO_PKCS11_MLKEM.Testing
make -j$(nproc)make check -j$(nproc)