feat(signature): implement SLH-DSA using OpenSSL #823
Conversation
|
Warning Rate limit exceeded@JoaoHenrique12 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 32 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughAdds SLHDSA (SLH-DSA) support across build, PKCS#11, crypto abstractions and OpenSSL backend: new Autoconf checks, PKCS#11 constants/attributes, SLH public/private key classes, OpenSSL implementations, import/export helpers, and CPPUnit tests; feature gated by a new enable_slhdsa/WITH_SLHDSA build-time switch. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as PKCS#11 Client
participant SoftHSM as SoftHSM
participant Factory as CryptoFactory
participant OSSLSLH as OSSLSLHDSA
participant OpenSSL as OpenSSL EVP
Client->>SoftHSM: C_GenerateKeyPair(CKM_SLH_KEY_PAIR_GEN, templates)
SoftHSM->>SoftHSM: generateSLH()
SoftHSM->>Factory: getAsymmetricAlgorithm(SLHDSA)
Factory->>OSSLSLH: instantiate
SoftHSM->>OSSLSLH: generateKeyPair(params)
OSSLSLH->>OpenSSL: EVP_PKEY_keygen(SLH-DSA-SHA2-...)
OpenSSL-->>OSSLSLH: EVP_PKEY*
OSSLSLH-->>SoftHSM: AsymmetricKeyPair (DERs, params)
SoftHSM->>SoftHSM: create P11SLH objects, store CKA_SLHDSA_PARAMS
SoftHSM-->>Client: object handles
sequenceDiagram
participant Client as PKCS#11 Client
participant SoftHSM as SoftHSM
participant OSSLSLH as OSSLSLHDSA
participant OpenSSL as OpenSSL EVP
Client->>SoftHSM: C_SignInit(CKM_SLHDSA, privHandle)
SoftHSM->>SoftHSM: getSLHPrivateKey()
SoftHSM->>OSSLSLH: signInit(privateKey, mech)
Client->>SoftHSM: C_Sign(data)
SoftHSM->>OSSLSLH: sign(privateKey, data)
OSSLSLH->>OpenSSL: EVP_DigestSign
OpenSSL-->>OSSLSLH: signature
OSSLSLH-->>SoftHSM: signature
SoftHSM-->>Client: signature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Areas requiring extra attention:
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (10)
src/lib/crypto/SLHPrivateKey.h (1)
68-71: Consider adding variant name inference.The TODO correctly identifies that storing the entire key as DER-encoded data is a temporary limitation. Given that OSSLSLHPrivateKey::getOrderLength (seen in snippets) derives signature size from the variant name, consider adding a getVariantName() method to the base class to enable parameter inference from key material, similar to how MLDSA infers parameter sets from key length.
This would allow runtime code to determine the SLH variant without relying on external parameter tracking.
src/lib/crypto/SLHParameters.cpp (1)
61-73: Consider validating parameter names.The deserialise method accepts any non-empty ByteString as a valid name. Consider adding validation to ensure the name matches a known SLH-DSA variant (e.g., "SLH-DSA-SHA2-128s", "SLH-DSA-SHAKE-256f", etc.) to catch configuration errors early.
However, this can be deferred if validation occurs at the OpenSSL layer during key generation.
src/lib/crypto/OSSLSLHPublicKey.h (1)
40-76: Consider defining copy semantics.The class manages an OpenSSL EVP_PKEY* resource but doesn't explicitly define copy constructor or copy assignment operator. While the base class pattern may prevent copying, consider adding
= deletedeclarations for clarity and safety to prevent accidental double-free issues.Example:
// Prevent copying OSSLSLHPublicKey(const OSSLSLHPublicKey&) = delete; OSSLSLHPublicKey& operator=(const OSSLSLHPublicKey&) = delete;This is a general concern for RAII classes managing OpenSSL resources. Check if similar classes (like OSSLEDPublicKey) follow a similar pattern.
src/lib/crypto/OSSLSLHPrivateKey.h (2)
38-38: Verify openssl/bn.h inclusion is necessary.The header includes
<openssl/bn.h>but doesn't appear to use BIGNUM types. If this include is unused, consider removing it to minimize dependencies.Verify if BIGNUM is actually needed in the implementation file or if this was carried over from another key class template.
41-83: Consider defining copy semantics.Similar to OSSLSLHPublicKey, this class manages OpenSSL EVP_PKEY* resources but doesn't explicitly define copy operations. Consider adding
= deletedeclarations for copy constructor and assignment operator to prevent accidental resource management issues.Example:
// Prevent copying OSSLSLHPrivateKey(const OSSLSLHPrivateKey&) = delete; OSSLSLHPrivateKey& operator=(const OSSLSLHPrivateKey&) = delete;src/lib/crypto/SLHPublicKey.h (1)
69-71: TODO comment flags a known limitation.The comment indicates that the DER-encoded key representation should eventually be replaced with separate attributes (PK.seed, PK.root). This aligns with the PR objectives mentioning that "private/public key values are stored DER-encoded without separating key attributes."
Is this limitation intended to be addressed in a follow-up, or would you like me to open an issue to track this refactoring? The separate-attribute approach would provide finer-grained PKCS#11 attribute access and align better with the FIPS 205 specification structure.
src/lib/crypto/OSSLSLHPublicKey.cpp (3)
79-90: Magic numbers lack documentation.The hardcoded signature sizes (7856, 17088, 16224, etc.) are not documented. Consider adding a comment referencing the FIPS 205 specification or OpenSSL documentation that defines these values for each SLHDSA parameter set.
Add a brief comment explaining the source of these values:
+ // Signature sizes per FIPS 205 / OpenSSL 3.5+ SLHDSA variants if (strncmp(&name[name_len - 4], "128s", 4) == 0) { signature_size = 7856;
129-141: Inefficient pattern: creating key only to extract name.Lines 133-140 create an OpenSSL key via
getOSSLKey(), extract the algorithm name, then immediately free the key. This is wasteful—the key is created, used briefly for metadata, then discarded.Consider one of these approaches:
- Cache the key: Remove the immediate
EVP_PKEY_free()and let the destructor handle cleanup.- Extract name during deserialization: Parse the DER structure to extract the algorithm OID without fully materializing the EVP_PKEY.
- Align with setFromOSSL: The
setFromOSSLpattern (lines 98-120) already setsnameand stores DER; ensuresetDerPublicKeydoesn't need to recreate the key.Apply this diff if caching is preferred:
void OSSLSLHPublicKey::setDerPublicKey(const ByteString& inPk) { SLHPublicKey::setDerPublicKey(inPk); getOSSLKey(); if (EVP_PKEY_get0_type_name(pkey) == NULL) { ERROR_MSG("Could not determine algorithm name from EVP_PKEY"); return; } name = EVP_PKEY_get0_type_name(pkey); - - if (pkey) - { EVP_PKEY_free(pkey); pkey = NULL; } + // pkey is now cached and will be freed in the destructor }
76-76: Hardcoded buffer size in strnlen.The
strnlen(name, 100)uses a hardcoded buffer limit of 100. While OpenSSL algorithm names are typically short, this assumption is undocumented and could be fragile if OpenSSL introduces longer names in future versions.Consider using
strlenifnameis guaranteed to be null-terminated (which EVP_PKEY_get0_type_name should ensure), or document the 100-byte assumption:- size_t name_len = strnlen(name, 100); + size_t name_len = strlen(name); // name is null-terminated per EVP_PKEY_get0_type_namesrc/lib/crypto/OSSLSLHDSA.cpp (1)
93-105: Redundant error retrieval may capture inconsistent errors.The error handling calls
ERR_get_error()multiple times (implicitly at line 95 via ERROR_MSG, then explicitly at line 97). Since ERR_get_error() removes errors from the OpenSSL error queue, subsequent calls may return different errors or zero. Capture the error code once and use it for all logging.Apply this diff to fix the error handling:
if (!EVP_DigestSign(ctx, &signature[0], &len, dataToSign.const_byte_str(), dataToSign.size())) { - ERROR_MSG("SLHDSA sign failed (0x%08X)", ERR_get_error()); - - unsigned long err = ERR_get_error(); + unsigned long err = ERR_get_error(); + ERROR_MSG("SLHDSA sign failed (0x%08X)", err); char buf[256]; ERR_error_string_n(err, buf, sizeof(buf)); ERROR_MSG("SLHDSA sign failed: %s\n", buf);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (36)
data.txt(1 hunks)m4/acx_crypto_backend.m4(4 hunks)m4/acx_openssl_slhdsa.m4(1 hunks)src/bin/dump/tables.h(3 hunks)src/bin/util/softhsm2-util-ossl.cpp(2 hunks)src/bin/util/softhsm2-util-ossl.h(3 hunks)src/lib/P11Attributes.cpp(1 hunks)src/lib/P11Attributes.h(1 hunks)src/lib/P11Objects.cpp(4 hunks)src/lib/P11Objects.h(2 hunks)src/lib/SoftHSM.cpp(22 hunks)src/lib/SoftHSM.h(3 hunks)src/lib/crypto/AsymmetricAlgorithm.h(2 hunks)src/lib/crypto/CMakeLists.txt(2 hunks)src/lib/crypto/Makefile.am(2 hunks)src/lib/crypto/OSSLCryptoFactory.cpp(2 hunks)src/lib/crypto/OSSLSLHDSA.cpp(1 hunks)src/lib/crypto/OSSLSLHDSA.h(1 hunks)src/lib/crypto/OSSLSLHKeyPair.cpp(1 hunks)src/lib/crypto/OSSLSLHKeyPair.h(1 hunks)src/lib/crypto/OSSLSLHPrivateKey.cpp(1 hunks)src/lib/crypto/OSSLSLHPrivateKey.h(1 hunks)src/lib/crypto/OSSLSLHPublicKey.cpp(1 hunks)src/lib/crypto/OSSLSLHPublicKey.h(1 hunks)src/lib/crypto/OSSLUtil.cpp(1 hunks)src/lib/crypto/OSSLUtil.h(2 hunks)src/lib/crypto/SLHParameters.cpp(1 hunks)src/lib/crypto/SLHParameters.h(1 hunks)src/lib/crypto/SLHPrivateKey.cpp(1 hunks)src/lib/crypto/SLHPrivateKey.h(1 hunks)src/lib/crypto/SLHPublicKey.cpp(1 hunks)src/lib/crypto/SLHPublicKey.h(1 hunks)src/lib/crypto/test/Makefile.am(1 hunks)src/lib/crypto/test/SLHDSATests.cpp(1 hunks)src/lib/crypto/test/SLHDSATests.h(1 hunks)src/lib/pkcs11/pkcs11.h(3 hunks)
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 815
File: src/lib/SoftHSM.cpp:6543-6546
Timestamp: 2025-09-11T16:54:00.370Z
Learning: SoftHSM PR #815 (pkcs11.h 3.1.0 update, CK_RSA_AES_KEY_WRAP_PARAMS fix): Maintainer bukka prefers to keep scope minimal (header sync and param rename only); broader OAEP parameter enforcement or backend plumbing is out of scope for this PR and should be handled in a follow-up issue.
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:5550-5596
Timestamp: 2025-10-27T16:44:57.846Z
Learning: SoftHSM ML‑DSA additional context: avoid transient malloc in SoftHSM::{AsymSignInit, AsymVerifyInit}; let Session::setParameters deep‑copy SIGN_ADDITIONAL_CONTEXT by allocating one buffer for struct + context bytes and fixing the stored pointer; Session::resetOp frees m_pParameters so both struct and context are released.
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 818
File: src/lib/SoftHSM.cpp:13265-13282
Timestamp: 2025-10-13T09:43:45.885Z
Learning: SoftHSM OAEP policy: In SoftHSM::MechParamCheckRSAPKCSOAEP (src/lib/SoftHSM.cpp), enforce that the MGF1 variant must correspond to the selected hashAlg (e.g., SHA-256 -> CKG_MGF1_SHA256). Mismatches should return CKR_ARGUMENTS_BAD with "Hash and MGF don't match". This mirrors the existing PSS enforcement.
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 818
File: src/lib/SoftHSM.cpp:3238-3266
Timestamp: 2025-10-13T09:42:31.270Z
Learning: In src/lib/SoftHSM.cpp, SoftHSM::AsymDecryptInit should use MechParamCheckRSAPKCSOAEP for CKM_RSA_PKCS_OAEP to keep OAEP parameter validation identical to AsymEncryptInit, and avoid redundant manual checks for pMechanism->pParameter and MGF allowlists.
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:10044-10055
Timestamp: 2025-08-18T19:46:46.659Z
Learning: In SoftHSM::generateMLDSA (src/lib/SoftHSM.cpp), the project prefers using CKP_ML_DSA_* constants for parameter set checks and logs "Unsupported parameter set: %lu". The code currently returns CKR_PARAMETER_SET_NOT_SUPPORTED for unsupported sets.
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:4193-4196
Timestamp: 2025-08-19T09:23:25.434Z
Learning: MLDSA parameter set inference: In src/lib/crypto/MLDSAPrivateKey.{h,cpp} and MLDSAPublicKey.{h,cpp}, getParameterSet() maps key material length to CKP_ML_DSA_44/65/87, and there is no setParameterSet(). Runtime MLDSA keys used by SoftHSM (e.g., in C_SignInit/C_VerifyInit via MLDSAUtil) rely on this inference; no explicit parameter-set assignment is required.
📚 Learning: 2025-08-18T17:59:30.693Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/crypto/MLDSAParameters.h:42-44
Timestamp: 2025-08-18T17:59:30.693Z
Learning: In MLDSAParameters.h, the author prefers to keep PKCS#11 constant references (CKP_ML_DSA_*) rather than explicit numeric values for better code clarity and maintainability.
Applied to files:
src/bin/dump/tables.hsrc/lib/crypto/SLHParameters.hsrc/lib/crypto/OSSLUtil.hsrc/lib/crypto/CMakeLists.txtsrc/lib/P11Attributes.hsrc/lib/crypto/AsymmetricAlgorithm.hsrc/lib/pkcs11/pkcs11.hsrc/lib/P11Attributes.cppsrc/bin/util/softhsm2-util-ossl.hsrc/lib/crypto/OSSLSLHDSA.hsrc/bin/util/softhsm2-util-ossl.cppsrc/lib/SoftHSM.hsrc/lib/crypto/OSSLCryptoFactory.cppsrc/lib/SoftHSM.cpp
📚 Learning: 2025-09-11T16:54:00.370Z
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 815
File: src/lib/SoftHSM.cpp:6543-6546
Timestamp: 2025-09-11T16:54:00.370Z
Learning: SoftHSM PR #815 (pkcs11.h 3.1.0 update, CK_RSA_AES_KEY_WRAP_PARAMS fix): Maintainer bukka prefers to keep scope minimal (header sync and param rename only); broader OAEP parameter enforcement or backend plumbing is out of scope for this PR and should be handled in a follow-up issue.
Applied to files:
src/bin/dump/tables.hsrc/lib/crypto/Makefile.amsrc/lib/crypto/CMakeLists.txtsrc/lib/P11Attributes.hsrc/lib/pkcs11/pkcs11.hm4/acx_crypto_backend.m4src/bin/util/softhsm2-util-ossl.hsrc/lib/crypto/OSSLSLHPrivateKey.cppsrc/lib/crypto/OSSLSLHPrivateKey.hsrc/lib/P11Objects.hsrc/bin/util/softhsm2-util-ossl.cppsrc/lib/SoftHSM.hsrc/lib/crypto/OSSLSLHDSA.cppsrc/lib/P11Objects.cppsrc/lib/SoftHSM.cpp
📚 Learning: 2025-08-18T19:46:46.659Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:10044-10055
Timestamp: 2025-08-18T19:46:46.659Z
Learning: In SoftHSM::generateMLDSA (src/lib/SoftHSM.cpp), the project prefers using CKP_ML_DSA_* constants for parameter set checks and logs "Unsupported parameter set: %lu". The code currently returns CKR_PARAMETER_SET_NOT_SUPPORTED for unsupported sets.
Applied to files:
src/bin/dump/tables.hsrc/lib/crypto/SLHParameters.hsrc/lib/crypto/CMakeLists.txtsrc/lib/P11Attributes.hsrc/lib/crypto/AsymmetricAlgorithm.hsrc/lib/pkcs11/pkcs11.hsrc/lib/P11Attributes.cppsrc/bin/util/softhsm2-util-ossl.hsrc/bin/util/softhsm2-util-ossl.cppsrc/lib/SoftHSM.hsrc/lib/crypto/OSSLSLHDSA.cppsrc/lib/SoftHSM.cpp
📚 Learning: 2025-10-27T16:44:57.846Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:5550-5596
Timestamp: 2025-10-27T16:44:57.846Z
Learning: SoftHSM ML‑DSA additional context: avoid transient malloc in SoftHSM::{AsymSignInit, AsymVerifyInit}; let Session::setParameters deep‑copy SIGN_ADDITIONAL_CONTEXT by allocating one buffer for struct + context bytes and fixing the stored pointer; Session::resetOp frees m_pParameters so both struct and context are released.
Applied to files:
src/bin/dump/tables.hsrc/lib/crypto/SLHParameters.hsrc/lib/crypto/Makefile.amsrc/lib/P11Attributes.hsrc/lib/crypto/AsymmetricAlgorithm.hsrc/lib/pkcs11/pkcs11.hsrc/lib/P11Attributes.cppsrc/bin/util/softhsm2-util-ossl.hsrc/lib/crypto/OSSLSLHDSA.hsrc/bin/util/softhsm2-util-ossl.cppsrc/lib/SoftHSM.hsrc/lib/crypto/OSSLSLHDSA.cppsrc/lib/crypto/OSSLCryptoFactory.cppsrc/lib/SoftHSM.cpp
📚 Learning: 2025-08-19T09:23:25.434Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:4193-4196
Timestamp: 2025-08-19T09:23:25.434Z
Learning: MLDSA parameter set inference: In src/lib/crypto/MLDSAPrivateKey.{h,cpp} and MLDSAPublicKey.{h,cpp}, getParameterSet() maps key material length to CKP_ML_DSA_44/65/87, and there is no setParameterSet(). Runtime MLDSA keys used by SoftHSM (e.g., in C_SignInit/C_VerifyInit via MLDSAUtil) rely on this inference; no explicit parameter-set assignment is required.
Applied to files:
src/lib/crypto/SLHParameters.hsrc/lib/crypto/Makefile.amsrc/lib/crypto/CMakeLists.txtsrc/lib/P11Attributes.hsrc/lib/crypto/AsymmetricAlgorithm.hsrc/lib/pkcs11/pkcs11.hsrc/lib/P11Attributes.cppsrc/lib/crypto/SLHPublicKey.hsrc/lib/crypto/OSSLSLHKeyPair.cppsrc/bin/util/softhsm2-util-ossl.hsrc/lib/crypto/OSSLSLHPrivateKey.cppsrc/lib/crypto/SLHPublicKey.cppsrc/lib/crypto/OSSLSLHDSA.hsrc/lib/crypto/OSSLSLHPublicKey.cppsrc/lib/crypto/SLHPrivateKey.hsrc/bin/util/softhsm2-util-ossl.cppsrc/lib/SoftHSM.hsrc/lib/crypto/SLHPrivateKey.cppsrc/lib/crypto/OSSLSLHDSA.cppsrc/lib/crypto/OSSLCryptoFactory.cppsrc/lib/SoftHSM.cpp
📚 Learning: 2025-08-18T17:54:37.645Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: m4/acx_openssl_mldsa.m4:10-35
Timestamp: 2025-08-18T17:54:37.645Z
Learning: For OpenSSL feature detection in autoconf macros, prefer functional tests (like EVP_PKEY_CTX_new_from_name) over version number checks, as the functional approach directly tests the actual capability and is more reliable across different OpenSSL builds where features may be backported or disabled.
Applied to files:
m4/acx_openssl_slhdsa.m4src/lib/crypto/OSSLUtil.hm4/acx_crypto_backend.m4src/bin/util/softhsm2-util-ossl.h
📚 Learning: 2025-10-13T09:42:31.270Z
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 818
File: src/lib/SoftHSM.cpp:3238-3266
Timestamp: 2025-10-13T09:42:31.270Z
Learning: In src/lib/SoftHSM.cpp, SoftHSM::AsymDecryptInit should use MechParamCheckRSAPKCSOAEP for CKM_RSA_PKCS_OAEP to keep OAEP parameter validation identical to AsymEncryptInit, and avoid redundant manual checks for pMechanism->pParameter and MGF allowlists.
Applied to files:
src/lib/crypto/Makefile.amsrc/lib/crypto/AsymmetricAlgorithm.hsrc/lib/pkcs11/pkcs11.hsrc/bin/util/softhsm2-util-ossl.hsrc/lib/crypto/OSSLSLHDSA.hsrc/bin/util/softhsm2-util-ossl.cppsrc/lib/SoftHSM.hsrc/lib/crypto/OSSLSLHDSA.cppsrc/lib/crypto/OSSLCryptoFactory.cppsrc/lib/SoftHSM.cpp
📚 Learning: 2025-10-13T09:43:45.885Z
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 818
File: src/lib/SoftHSM.cpp:13265-13282
Timestamp: 2025-10-13T09:43:45.885Z
Learning: SoftHSM OAEP policy: In SoftHSM::MechParamCheckRSAPKCSOAEP (src/lib/SoftHSM.cpp), enforce that the MGF1 variant must correspond to the selected hashAlg (e.g., SHA-256 -> CKG_MGF1_SHA256). Mismatches should return CKR_ARGUMENTS_BAD with "Hash and MGF don't match". This mirrors the existing PSS enforcement.
Applied to files:
src/lib/P11Attributes.hsrc/lib/pkcs11/pkcs11.hsrc/bin/util/softhsm2-util-ossl.hsrc/bin/util/softhsm2-util-ossl.cppsrc/lib/SoftHSM.hsrc/lib/crypto/OSSLCryptoFactory.cppsrc/lib/SoftHSM.cpp
📚 Learning: 2025-08-11T16:42:23.012Z
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 806
File: src/lib/test/p11test.cpp:81-84
Timestamp: 2025-08-11T16:42:23.012Z
Learning: In SoftHSM test files (like p11test.cpp and cryptotest.cpp), error handling for OSSL_PROVIDER_load() calls for "legacy" and "default" providers is intentionally omitted because these providers won't fail in practice and the test code is kept deliberately simple.
Applied to files:
src/bin/util/softhsm2-util-ossl.hsrc/lib/crypto/test/SLHDSATests.hsrc/bin/util/softhsm2-util-ossl.cppsrc/lib/crypto/test/SLHDSATests.cppsrc/lib/SoftHSM.cpp
🧬 Code graph analysis (21)
src/lib/crypto/SLHParameters.h (2)
src/lib/crypto/AsymmetricParameters.h (1)
AsymmetricParameters(40-45)src/lib/crypto/SLHParameters.cpp (10)
setName(40-43)setName(40-40)getName(45-48)getName(45-45)areOfType(50-53)areOfType(50-50)serialise(56-59)serialise(56-56)deserialise(61-73)deserialise(61-61)
src/lib/P11Attributes.h (2)
src/lib/P11Attributes.cpp (18)
P11Attribute(43-49)P11Attribute(52-54)setDefault(503-507)setDefault(503-503)setDefault(532-536)setDefault(532-532)setDefault(563-567)setDefault(563-563)setDefault(592-596)setDefault(592-592)setDefault(630-634)setDefault(630-630)setDefault(668-672)setDefault(668-668)setDefault(706-710)setDefault(706-706)setDefault(717-721)setDefault(717-717)src/lib/object_store/OSObject.h (1)
OSObject(42-92)
src/lib/P11Attributes.cpp (1)
src/lib/data_mgr/ByteString.cpp (5)
ByteString(49-51)ByteString(53-59)ByteString(61-80)ByteString(82-107)ByteString(109-112)
src/lib/crypto/SLHPublicKey.h (4)
src/lib/crypto/PublicKey.h (1)
PublicKey(40-54)src/lib/crypto/OSSLSLHPublicKey.cpp (6)
isOfType(123-126)isOfType(123-123)getOrderLength(70-95)getOrderLength(70-70)setDerPublicKey(129-141)setDerPublicKey(129-129)src/lib/crypto/SLHPrivateKey.cpp (10)
isOfType(42-45)isOfType(42-42)getBitLength(48-51)getBitLength(48-48)getOutputLength(54-57)getOutputLength(54-54)serialise(70-73)serialise(70-70)deserialise(75-86)deserialise(75-75)src/lib/crypto/SLHPublicKey.cpp (14)
isOfType(42-45)isOfType(42-42)getBitLength(48-51)getBitLength(48-48)getOutputLength(54-57)getOutputLength(54-54)setDerPublicKey(60-63)setDerPublicKey(60-60)getDerPublicKey(66-69)getDerPublicKey(66-66)serialise(72-75)serialise(72-72)deserialise(77-89)deserialise(77-77)
src/lib/crypto/SLHParameters.cpp (2)
src/lib/crypto/SLHPrivateKey.cpp (4)
serialise(70-73)serialise(70-70)deserialise(75-86)deserialise(75-75)src/lib/crypto/SLHPublicKey.cpp (4)
serialise(72-75)serialise(72-72)deserialise(77-89)deserialise(77-77)
src/bin/util/softhsm2-util-ossl.h (1)
src/bin/util/softhsm2-util-ossl.cpp (6)
crypto_save_slhdsa(1159-1232)crypto_save_slhdsa(1159-1167)crypto_malloc_slhdsa(1236-1289)crypto_malloc_slhdsa(1236-1236)crypto_free_slhdsa(1292-1304)crypto_free_slhdsa(1292-1292)
src/lib/crypto/test/SLHDSATests.h (1)
src/lib/crypto/test/SLHDSATests.cpp (12)
testKeyGeneration(66-113)testKeyGeneration(66-66)testSerialisation(115-162)testSerialisation(115-115)testPKCS8(164-192)testPKCS8(164-164)testSigningVerifying(194-240)testSigningVerifying(194-194)setUp(49-56)setUp(49-49)tearDown(58-64)tearDown(58-58)
src/lib/crypto/OSSLSLHPrivateKey.cpp (3)
src/lib/crypto/OSSLSLHPrivateKey.h (1)
OSSLSLHPrivateKey(41-83)src/lib/crypto/OSSLSLHPublicKey.cpp (10)
setFromOSSL(98-120)setFromOSSL(98-98)getOrderLength(70-95)getOrderLength(70-70)isOfType(123-126)isOfType(123-123)getOSSLKey(144-149)getOSSLKey(144-144)createOSSLKey(152-165)createOSSLKey(152-152)src/lib/crypto/SLHPrivateKey.cpp (4)
setDerPrivateKey(59-62)setDerPrivateKey(59-59)isOfType(42-45)isOfType(42-42)
src/lib/crypto/SLHPublicKey.cpp (3)
src/lib/crypto/OSSLSLHPublicKey.cpp (6)
isOfType(123-126)isOfType(123-123)getOrderLength(70-95)getOrderLength(70-70)setDerPublicKey(129-141)setDerPublicKey(129-129)src/lib/crypto/SLHPrivateKey.cpp (10)
isOfType(42-45)isOfType(42-42)getBitLength(48-51)getBitLength(48-48)getOutputLength(54-57)getOutputLength(54-54)serialise(70-73)serialise(70-70)deserialise(75-86)deserialise(75-75)src/lib/crypto/SLHParameters.cpp (4)
serialise(56-59)serialise(56-56)deserialise(61-73)deserialise(61-61)
src/lib/crypto/OSSLSLHPublicKey.h (3)
src/lib/crypto/OSSLSLHPublicKey.cpp (15)
OSSLSLHPublicKey(43-47)OSSLSLHPublicKey(49-55)OSSLSLHPublicKey(58-65)isOfType(123-126)isOfType(123-123)getOrderLength(70-95)getOrderLength(70-70)setDerPublicKey(129-141)setDerPublicKey(129-129)setFromOSSL(98-120)setFromOSSL(98-98)getOSSLKey(144-149)getOSSLKey(144-144)createOSSLKey(152-165)createOSSLKey(152-152)src/lib/crypto/SLHPublicKey.h (1)
SLHPublicKey(39-73)src/lib/crypto/SLHPublicKey.cpp (4)
isOfType(42-45)isOfType(42-42)setDerPublicKey(60-63)setDerPublicKey(60-60)
src/lib/crypto/OSSLSLHPrivateKey.h (3)
src/lib/crypto/OSSLSLHPrivateKey.cpp (19)
OSSLSLHPrivateKey(43-47)OSSLSLHPrivateKey(49-55)OSSLSLHPrivateKey(58-65)isOfType(132-135)isOfType(132-132)getOrderLength(70-96)getOrderLength(70-70)setDerPrivateKey(137-149)setDerPrivateKey(137-137)PKCS8Encode(152-170)PKCS8Encode(152-152)PKCS8Decode(173-186)PKCS8Decode(173-173)setFromOSSL(99-129)setFromOSSL(99-99)getOSSLKey(189-194)getOSSLKey(189-189)createOSSLKey(197-215)createOSSLKey(197-197)src/lib/crypto/SLHPrivateKey.h (1)
SLHPrivateKey(39-72)src/lib/crypto/SLHPrivateKey.cpp (4)
isOfType(42-45)isOfType(42-42)setDerPrivateKey(59-62)setDerPrivateKey(59-59)
src/lib/crypto/OSSLSLHDSA.h (2)
src/lib/crypto/AsymmetricAlgorithm.h (2)
AsymmetricAlgorithm(121-190)AsymmetricAlgorithm(128-128)src/lib/crypto/OSSLSLHDSA.cpp (42)
sign(49-108)sign(49-51)signInit(110-116)signInit(110-111)signUpdate(118-123)signUpdate(118-118)signFinal(125-130)signFinal(125-125)verify(133-190)verify(133-135)verifyInit(192-198)verifyInit(192-193)verifyUpdate(200-205)verifyUpdate(200-200)verifyFinal(207-212)verifyFinal(207-207)encrypt(215-221)encrypt(215-216)decrypt(224-230)decrypt(224-225)generateKeyPair(233-289)generateKeyPair(233-233)getMinKeySize(368-372)getMinKeySize(368-368)getMaxKeySize(374-378)getMaxKeySize(374-374)deriveKey(291-366)deriveKey(291-291)reconstructKeyPair(380-416)reconstructKeyPair(380-380)reconstructPublicKey(418-439)reconstructPublicKey(418-418)reconstructPrivateKey(441-462)reconstructPrivateKey(441-441)reconstructParameters(479-499)reconstructParameters(479-479)newPublicKey(464-467)newPublicKey(464-464)newPrivateKey(469-472)newPrivateKey(469-469)newParameters(474-477)newParameters(474-474)
src/lib/crypto/OSSLSLHPublicKey.cpp (3)
src/lib/crypto/OSSLSLHPublicKey.h (1)
OSSLSLHPublicKey(40-76)src/lib/crypto/OSSLSLHPrivateKey.cpp (10)
setFromOSSL(99-129)setFromOSSL(99-99)getOrderLength(70-96)getOrderLength(70-70)isOfType(132-135)isOfType(132-132)getOSSLKey(189-194)getOSSLKey(189-189)createOSSLKey(197-215)createOSSLKey(197-197)src/lib/crypto/SLHPublicKey.cpp (4)
setDerPublicKey(60-63)setDerPublicKey(60-60)isOfType(42-45)isOfType(42-42)
src/lib/P11Objects.h (1)
src/lib/P11Objects.cpp (20)
P11SLHPublicKeyObj(935-938)P11PublicKeyObj(665-668)init(64-114)init(64-64)init(339-384)init(339-339)init(393-460)init(393-393)init(469-537)init(469-469)init(546-598)init(546-546)init(607-662)init(607-607)init(671-740)init(671-671)init(749-789)init(749-749)P11SLHPrivateKeyObj(1385-1388)P11PrivateKeyObj(1082-1085)
src/lib/crypto/SLHPrivateKey.h (2)
src/lib/crypto/PrivateKey.h (1)
PrivateKey(41-58)src/lib/crypto/SLHPrivateKey.cpp (14)
isOfType(42-45)isOfType(42-42)getBitLength(48-51)getBitLength(48-48)getOutputLength(54-57)getOutputLength(54-54)setDerPrivateKey(59-62)setDerPrivateKey(59-59)getDerPrivateKey(64-67)getDerPrivateKey(64-64)serialise(70-73)serialise(70-70)deserialise(75-86)deserialise(75-75)
src/lib/crypto/test/SLHDSATests.cpp (1)
src/lib/crypto/test/SLHDSATests.h (1)
SLHDSATests(39-59)
src/lib/SoftHSM.h (1)
src/lib/SoftHSM.cpp (6)
generateSLH(10061-10304)generateSLH(10061-10072)getSLHPrivateKey(13029-13055)getSLHPrivateKey(13029-13029)getSLHPublicKey(13057-13082)getSLHPublicKey(13057-13057)
src/lib/crypto/SLHPrivateKey.cpp (3)
src/lib/crypto/OSSLSLHPrivateKey.cpp (6)
isOfType(132-135)isOfType(132-132)getOrderLength(70-96)getOrderLength(70-70)setDerPrivateKey(137-149)setDerPrivateKey(137-137)src/lib/crypto/SLHPublicKey.cpp (10)
isOfType(42-45)isOfType(42-42)getBitLength(48-51)getBitLength(48-48)getOutputLength(54-57)getOutputLength(54-54)serialise(72-75)serialise(72-72)deserialise(77-89)deserialise(77-77)src/lib/crypto/SLHParameters.cpp (4)
serialise(56-59)serialise(56-56)deserialise(61-73)deserialise(61-61)
src/lib/crypto/OSSLSLHKeyPair.h (4)
src/lib/crypto/AsymmetricKeyPair.h (1)
AsymmetricKeyPair(42-55)src/lib/crypto/OSSLSLHKeyPair.cpp (12)
setPublicKey(39-42)setPublicKey(39-39)setPrivateKey(45-48)setPrivateKey(45-45)getPublicKey(51-54)getPublicKey(51-51)getConstPublicKey(56-59)getConstPublicKey(56-56)getPrivateKey(62-65)getPrivateKey(62-62)getConstPrivateKey(67-70)getConstPrivateKey(67-67)src/lib/crypto/PublicKey.h (1)
PublicKey(40-54)src/lib/crypto/PrivateKey.h (1)
PrivateKey(41-58)
src/lib/P11Objects.cpp (2)
src/lib/P11Objects.h (2)
P11SLHPublicKeyObj(201-212)P11SLHPrivateKeyObj(303-314)src/lib/P11Attributes.cpp (2)
init(109-120)init(109-109)
src/lib/SoftHSM.cpp (2)
src/lib/crypto/CryptoFactory.cpp (4)
i(43-46)i(43-43)i(59-62)i(59-59)src/lib/data_mgr/ByteString.cpp (7)
size(310-313)size(310-310)ByteString(49-51)ByteString(53-59)ByteString(61-80)ByteString(82-107)ByteString(109-112)
🪛 Clang (14.0.6)
src/lib/crypto/SLHParameters.h
[error] 36-36: 'config.h' file not found
(clang-diagnostic-error)
src/lib/crypto/SLHPublicKey.h
[error] 36-36: 'config.h' file not found
(clang-diagnostic-error)
src/lib/crypto/SLHParameters.cpp
[error] 33-33: 'config.h' file not found
(clang-diagnostic-error)
src/lib/crypto/OSSLSLHKeyPair.cpp
[error] 33-33: 'config.h' file not found
(clang-diagnostic-error)
src/lib/crypto/test/SLHDSATests.h
[error] 36-36: 'AsymmetricAlgorithm.h' file not found
(clang-diagnostic-error)
src/lib/crypto/OSSLSLHPrivateKey.cpp
[error] 33-33: 'config.h' file not found
(clang-diagnostic-error)
src/lib/crypto/SLHPublicKey.cpp
[error] 33-33: 'config.h' file not found
(clang-diagnostic-error)
src/lib/crypto/OSSLSLHPublicKey.h
[error] 36-36: 'config.h' file not found
(clang-diagnostic-error)
src/lib/crypto/OSSLSLHPrivateKey.h
[error] 36-36: 'config.h' file not found
(clang-diagnostic-error)
src/lib/crypto/OSSLSLHDSA.h
[error] 36-36: 'config.h' file not found
(clang-diagnostic-error)
src/lib/crypto/OSSLSLHPublicKey.cpp
[error] 33-33: 'config.h' file not found
(clang-diagnostic-error)
src/lib/crypto/SLHPrivateKey.h
[error] 36-36: 'config.h' file not found
(clang-diagnostic-error)
src/lib/crypto/SLHPrivateKey.cpp
[error] 33-33: 'config.h' file not found
(clang-diagnostic-error)
src/lib/crypto/OSSLSLHDSA.cpp
[error] 33-33: 'config.h' file not found
(clang-diagnostic-error)
src/lib/crypto/OSSLSLHKeyPair.h
[error] 36-36: 'config.h' file not found
(clang-diagnostic-error)
🔇 Additional comments (37)
src/lib/crypto/OSSLUtil.h (1)
68-74: Verification confirms the code is correct as-is.SLHDSA implementation does not use
oid2ByteStringorbyteString2oidfunctions. The SLHDSA implementation (OSSLSLHDSA.cpp/h) usesEVP_DigestSign/EVP_DigestSignInitdirectly and does not call these OID utilities. Keeping these functions guarded with#if defined(WITH_EDDSA)only is appropriate. Theobjects.hinclusion for SLHDSA is unrelated to these utilities.Minor note: Line 39 uses
#ifdef WITH_ECCwhile line 68 uses#if defined(WITH_EDDSA)(style inconsistency), but this does not affect functionality.Likely an incorrect or invalid review comment.
data.txt (1)
1-1: LGTM!Simple test data file for validation purposes.
src/lib/crypto/OSSLUtil.cpp (1)
121-121: LGTM!Style improvement using
#if defined()for consistency with broader SLHDSA integration patterns.src/bin/dump/tables.h (3)
103-103: LGTM!Adds CKA_SLHDSA_PARAMS attribute mapping following the existing table pattern.
482-483: LGTM!Adds SLHDSA mechanism mappings (CKM_SLH_KEY_PAIR_GEN, CKM_SLHDSA) following the existing pattern.
550-550: LGTM!Adds CKK_SLHDSA key type mapping following the existing table pattern.
src/lib/crypto/OSSLCryptoFactory.cpp (2)
61-63: LGTM!Conditional include for OSSLSLHDSA follows the same pattern as EDDSA.
354-357: LGTM!SLHDSA algorithm factory case follows the existing pattern for asymmetric algorithms.
src/lib/P11Attributes.cpp (1)
2230-2239: LGTM!P11AttrSLHDSAParams::setDefault implementation follows the same pattern as CKA_EC_POINT attribute defaulting.
src/lib/crypto/CMakeLists.txt (2)
23-23: LGTM!Adds SLH-related generic sources to the build, following the existing pattern for other cryptographic algorithms.
Also applies to: 28-29
64-67: LGTM!Adds OpenSSL-specific SLHDSA sources to the WITH_OPENSSL block, consistent with other OpenSSL algorithm implementations.
src/lib/pkcs11/pkcs11.h (3)
411-411: LGTM!Adds CKK_SLHDSA key type using vendor-defined range (0x80010037UL), following PKCS#11 conventions.
489-489: LGTM!Adds CKA_SLHDSA_PARAMS attribute using vendor-defined range (0x80000030UL), following PKCS#11 conventions.
897-898: LGTM!Adds SLHDSA mechanism constants (CKM_SLH_KEY_PAIR_GEN, CKM_SLHDSA) using vendor-defined range, following PKCS#11 conventions and placed appropriately near CKM_EDDSA.
src/lib/crypto/Makefile.am (2)
23-23: LGTM!Adds SLH-related generic sources to the Automake build, mirroring the CMakeLists.txt changes and following the existing pattern.
Also applies to: 28-29
70-73: LGTM!Adds OpenSSL-specific SLHDSA sources to the WITH_OPENSSL block, consistent with the CMake configuration and other OpenSSL algorithm implementations.
src/lib/crypto/test/SLHDSATests.h (1)
33-61: LGTM - Well-structured test fixture.The test header follows established CPPUnit patterns in the codebase and covers the essential test categories (key generation, serialization, PKCS#8, and signing/verification). The structure aligns with similar test fixtures for other asymmetric algorithms.
src/lib/crypto/SLHParameters.h (1)
40-58: LGTM - Clean parameter class design.The SLHParameters class follows the established pattern for asymmetric algorithm parameters in the codebase. The name-based approach (storing variant names like "SLH-DSA-SHA2-128s") is appropriate for SLHDSA, which has multiple named variants rather than numeric parameter sets.
src/lib/SoftHSM.h (3)
51-52: LGTM - Consistent header additions.The SLH key type includes follow the established pattern for other asymmetric key types (ED, EC, RSA, DSA, etc.).
318-331: LGTM - Standard key generation signature.The generateSLH method signature is consistent with other asymmetric key pair generation methods (generateED, generateEC, etc.), maintaining uniformity across the codebase.
447-448: LGTM - Consistent key accessor pattern.The getSLHPrivateKey and getSLHPublicKey declarations follow the established naming and signature pattern for key retrieval methods.
src/lib/P11Objects.h (2)
201-212: LGTM - Standard P11 object class structure.P11SLHPublicKeyObj follows the established pattern for PKCS#11 public key objects, consistent with P11EDPublicKeyObj, P11ECPublicKeyObj, and other key types.
303-314: LGTM - Standard P11 object class structure.P11SLHPrivateKeyObj follows the established pattern for PKCS#11 private key objects, maintaining consistency with the codebase's object model.
src/lib/crypto/OSSLSLHKeyPair.h (1)
41-64: LGTM!The OSSLSLHKeyPair class correctly implements the AsymmetricKeyPair interface and follows the established pattern used by other key pair classes (e.g., EdDSA). The implementation provides appropriate setters and getters for managing SLH public and private keys.
src/lib/crypto/SLHPublicKey.cpp (1)
42-89: LGTM!The SLHPublicKey implementation correctly provides type checking, bit length calculation, DER key accessors, and serialization support. The deserialization properly validates that the key is non-empty before storing it.
src/lib/crypto/test/SLHDSATests.cpp (1)
115-162: LGTM!The serialization test correctly validates parameter serialization, key pair generation, and round-trip serialization/deserialization of SLHDSA keys.
src/bin/util/softhsm2-util-ossl.cpp (2)
162-169: LGTM!The SLHDSA key detection and early routing correctly identifies SLH-DSA keys by algorithm name and properly manages the EVP_PKEY reference count.
1236-1304: LGTM!The memory allocation and deallocation functions for SLHDSA key material correctly handle DER encoding of both public and private keys, with proper error handling and cleanup paths. The use of OPENSSL_malloc/free for OpenSSL buffers is appropriate.
src/lib/P11Objects.cpp (3)
934-977: LGTM!The P11SLHPublicKeyObj implementation correctly follows the established pattern for public key objects. It properly sets the key type, creates the necessary attributes (CKA_SLHDSA_PARAMS and CKA_VALUE), and manages initialization state.
1384-1427: LGTM!The P11SLHPrivateKeyObj implementation correctly mirrors the public key object pattern and properly integrates with the private key object hierarchy.
229-229: Missing semicolon causes compilation error.The ERROR_MSG statement is missing a terminating semicolon.
Apply this diff:
- ERROR_MSG("ATTR is NULL for attribute type 0x%08X, abortTransaction", (unsigned int)pTemplate[i].type); + ERROR_MSG("ATTR is NULL for attribute type 0x%08X, abortTransaction", (unsigned int)pTemplate[i].type);Likely an incorrect or invalid review comment.
src/lib/crypto/OSSLSLHDSA.cpp (6)
110-130: LGTM: Multi-part operations correctly rejected.The signInit/signUpdate/signFinal and verifyInit/verifyUpdate/verifyFinal stubs correctly return false with error messages, as SLH-DSA uses single-shot signing/verification operations and does not support multi-part flows.
Also applies to: 192-212
214-230: LGTM: Encryption/decryption correctly rejected.The encrypt() and decrypt() stubs correctly return false, as SLH-DSA is a signature-only algorithm and does not support encryption or decryption operations.
232-289: LGTM: Key generation implementation is sound.The generateKeyPair() implementation correctly:
- Validates parameters and checks for SLHParameters type
- Uses EVP_PKEY_CTX_new_from_name with the algorithm name from parameters
- Properly handles errors and cleans up resources on failure paths
- Transfers the generated key to the OSSLSLHKeyPair wrapper
380-462: LGTM: Key reconstruction methods are correctly implemented.The reconstructKeyPair(), reconstructPublicKey(), and reconstructPrivateKey() methods properly:
- Validate input parameters
- Use ByteString::chainDeserialise for extracting serialised key data
- Call deserialise() on the appropriate key classes
- Clean up allocated objects on failure
- Follow the established pattern for key reconstruction in the codebase
464-477: LGTM: Factory methods correctly instantiate SLHDSA types.The newPublicKey(), newPrivateKey(), and newParameters() factory methods correctly instantiate and return the appropriate SLH-DSA-specific types (OSSLSLHPublicKey, OSSLSLHPrivateKey, SLHParameters).
479-499: LGTM: reconstructParameters follows the correct pattern.The reconstructParameters() method properly validates inputs, instantiates a new SLHParameters object, deserialises the data, and cleans up on failure. This follows the established pattern for parameter reconstruction in the codebase.
| CK_ATTRIBUTE pubTemplate[] = { | ||
| { CKA_CLASS, &pubClass, sizeof(pubClass) }, | ||
| { CKA_KEY_TYPE, &keyType, sizeof(keyType) }, | ||
| { CKA_LABEL, label, strlen(label) }, | ||
| { CKA_ID, objID, objIDLen }, | ||
| { CKA_TOKEN, &ckToken, sizeof(ckToken) }, | ||
| { CKA_VERIFY, &ckTrue, sizeof(ckTrue) }, | ||
| { CKA_ENCRYPT, &ckFalse, sizeof(ckFalse) }, | ||
| { CKA_WRAP, &ckFalse, sizeof(ckFalse) }, | ||
| { CKA_SLHDSA_PARAMS, keyMat->derPublicKey, keyMat->sizePublicKey }, | ||
| }; | ||
| CK_ATTRIBUTE privTemplate[] = { | ||
| { CKA_CLASS, &privClass, sizeof(privClass) }, | ||
| { CKA_KEY_TYPE, &keyType, sizeof(keyType) }, | ||
| { CKA_LABEL, label, strlen(label) }, | ||
| { CKA_ID, objID, objIDLen }, | ||
| { CKA_SIGN, &ckTrue, sizeof(ckTrue) }, | ||
| { CKA_DECRYPT, &ckFalse, sizeof(ckFalse) }, | ||
| { CKA_UNWRAP, &ckFalse, sizeof(ckFalse) }, | ||
| { CKA_SENSITIVE, &ckTrue, sizeof(ckTrue) }, | ||
| { CKA_TOKEN, &ckTrue, sizeof(ckTrue) }, | ||
| { CKA_PRIVATE, &ckTrue, sizeof(ckTrue) }, | ||
| { CKA_EXTRACTABLE, &ckFalse, sizeof(ckFalse) }, | ||
| { CKA_SLHDSA_PARAMS, keyMat->derPrivateKey, keyMat->sizePrivateKey }, | ||
| }; |
There was a problem hiding this comment.
Critical: Incorrect attribute usage for SLHDSA keys.
The CKA_SLHDSA_PARAMS attribute is being used to store the entire DER-encoded public/private key, but it should only contain the algorithm parameters (e.g., the variant name like "SLH-DSA-SHA2-128s"). The actual key material should be in CKA_VALUE or a similar attribute.
Compare with EdDSA (lines 1000-1026):
- CKA_EC_PARAMS stores the algorithm OID (parameters)
- CKA_EC_POINT stores the public key
- CKA_VALUE stores the private key
For SLHDSA, you should:
- Extract the algorithm variant/parameters from the EVP_PKEY
- Store only those parameters in CKA_SLHDSA_PARAMS
- Store the public key material in an appropriate attribute (possibly CKA_VALUE for public keys)
- Store the private key material in CKA_VALUE for private keys
This aligns with the PR description noting that "private/public key values are stored DER-encoded without separating key attributes" as a limitation to be addressed.
🤖 Prompt for AI Agents
In src/bin/util/softhsm2-util-ossl.cpp around lines 1183-1207, the code
currently places entire DER-encoded public/private keys into CKA_SLHDSA_PARAMS;
update this to store only the SLH-DSA algorithm parameters (variant name/OID) in
CKA_SLHDSA_PARAMS and move the actual key blobs into the appropriate CKA_VALUE
attributes (public key into the public object's CKA_VALUE-like attribute and
private key into the private object's CKA_VALUE). Specifically: extract the
SLHDSA algorithm variant/parameters from the EVP_PKEY (or related params) and
set that buffer/length for CKA_SLHDSA_PARAMS; replace
keyMat->derPublicKey/sizePublicKey and keyMat->derPrivateKey/sizePrivateKey in
the SLHDSA params entries with the parameter blob; add CKA_VALUE entries to the
pubTemplate and privTemplate that hold the DER public and DER private key bytes
respectively (with correct lengths), and remove or adjust the incorrect
CKA_SLHDSA_PARAMS usages so attribute types and sizes match PKCS#11
expectations.
| ByteString value; | ||
| if (isPublicKeyPrivate) | ||
| { | ||
| token->encrypt(pub->getDerPublicKey(), value); | ||
| } | ||
| else | ||
| { | ||
| value = pub->getDerPublicKey(); | ||
| } | ||
| bOK = bOK && osobject->setAttribute(CKA_SLHDSA_PARAMS, value); | ||
|
|
||
| if (bOK) | ||
| bOK = osobject->commitTransaction(); | ||
| else | ||
| osobject->abortTransaction(); | ||
|
|
||
| if (!bOK) | ||
| rv = CKR_FUNCTION_FAILED; | ||
| } else | ||
| rv = CKR_FUNCTION_FAILED; | ||
| } | ||
| } | ||
|
|
||
| // Create a private key using C_CreateObject | ||
| if (rv == CKR_OK) | ||
| { | ||
| const CK_ULONG maxAttribs = 32; | ||
| CK_OBJECT_CLASS privateKeyClass = CKO_PRIVATE_KEY; |
There was a problem hiding this comment.
Don’t overwrite CKA_SLHDSA_PARAMS with the DER key blob
In generateSLH the call to saveTemplate() persists the caller-supplied parameter set under CKA_SLHDSA_PARAMS. Immediately afterwards we replace that attribute with the DER-encoded public/private key material. As a result the stored object no longer exposes its parameter set, C_GetAttributeValue on CKA_SLHDSA_PARAMS returns raw key bytes, and any consumer that relies on that attribute to identify the SLH variant now breaks. Please keep CKA_SLHDSA_PARAMS as the parameter selector and store the generated key data under a dedicated value attribute (e.g., CKA_VALUE/vendor value attribute), updating the corresponding getSLH*Key helpers accordingly.
🤖 Prompt for AI Agents
In src/lib/SoftHSM.cpp around lines 10178 to 10205, the code currently
overwrites CKA_SLHDSA_PARAMS with the DER-encoded public key blob; preserve the
original CKA_SLHDSA_PARAMS attribute and instead store the generated key bytes
in a dedicated value attribute (e.g., CKA_VALUE or a vendor-specific value
attribute) when saving the public/private material, update the
osobject->setAttribute call to use that value attribute and leave
CKA_SLHDSA_PARAMS untouched, and then update the getSLH*Key helper functions to
read the key material from the new value attribute rather than from
CKA_SLHDSA_PARAMS.
|
I guess this is also related to #809 ? |
feat: temporary commit with cache feat: remove cache
removed logs used to learn about how softhsm handle crypto algorithms
* feat: duplicate m4 files, still using NID from EDDSA Update dockerfile to use --with-slhdsa * feat: duplicate SLH(Public|Private)Key.(cpp|h) * feat: duplicate OSSLSLH(Public|Private)Key.(cpp|h) * feat: duplicate OSSLSLHKeypair.(cpp|h) * feat: add SLHDSA mech and algo to AsymmetricAlgorithm.h * feat: duplicate OSSLSLHDSA.(cpp|h) * feat: duplicate on OSSLCryptoFactory.cpp * feat: OSSLUtil.(cpp|h), duplication by definition * feat: add duplicated files to crypto/CMakeLists.txt * feat: duplicate P11ED(Public|Private)KeyObj for slhdsa PTAL on EC params, update it later, CKK_EC_EDWARDS * feat: duplicate softhsm2-util-ossl.(cpp|h) * feat: duplicate SoftHSM.(cpp|h) * fix: force disable eddsa to avoid conflict with cases and tables * fix: compilation error by add crypto/Makefile.am cpp new files grep -r OSSLEDDSA * feat: add syslog to container * fix: key gen on SLH-DSA * fix: add default log level info * refactor: use separated pkcs constants (with same value) for slh-dsa * feat(pkcs11): add ibm implementation to access cryptographic tokens
* feat: update slh-dsa pkcs11 params, use different numbers * docs: add links about oid ed25519 * refactor: replace ec to slh on generateSLH * chore(logs): added to C_GenerateKeyPair flow structure found where problems in key generation is: EC_PARAMS must be duplicated to SLH_PARAMS PKCS11 constant is raising error on P11Object::saveTemplate * feat(pkcs11): update input params for public key to SLH_DSA_PARAMS * feat: duplication ECParameters to SLHParameters * feat: update SLH(Public|Priavte)Key, remove EC and use der for attribs * feat: update OSSLSLH(Public|Priavte)Key classes * feat: update softhsm2-util-ossl.(cpp|h) * feat: update P11Attributes.(cpp|h) + P11Objects.cpp * feat: make changes on OSSLSLHDSA and SoftHSM to store keys add logs either * fix: getOrderLength and setDer(Public|Private)Key setup name when setDer(Public|Private)Key to get the signature length on OSSLSLH(Private|Public )Key.cpp * fix: getOrderLength for private and public key not multiply by 2 * fix: OSSLSLHPublicKey, remove ED key len variables * fix: OSSLSLHPublicKey use inPKEY instead pkey * fix: pkcs11-slhdsa.c signature size to 7856 * fix: OSSLSLHPublicKey verification of pkey * chore: add some debug logs on OSSLSLHDSA and SoftHSM files * test: add pkcs11-slhdsa.c test cases for all signatures with slh-dsa add as well data.txt * fix: file OSSLSLHDSA.cpp get back macro with_slhdsa replace printf and use ERROR_MSG * chore: dockerfile enable eddsa either * fix: remove macro slh-dsa for nid functions * fix: update m4 files to disable slh-dsa for older versions of OSSL disable SLH-DSA for Botan either * fix: warning of unused variable get slhdsaMin/Maxsize oly if slh-dsa is enabled
key gen, serialization, deserialization, pkcs8, sign and verify
JoaoHenrique12
force-pushed
the
jp-implement-slh-dsa
branch
from
7c74ffe to
21a981f
Compare
these logs was used on exploratory part, do not make sense for now
slh-dsa do not use it, renmant of duplication of eddsa
it was 20 and should be 19
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (12)
src/lib/crypto/OSSLUtil.h (1)
42-44: Duplicate: SLHDSA doesn't require objects.h.As noted in previous review, the include guard was expanded to SLHDSA but SLHDSA code has no uses of
objects.hsymbols or OID conversion utilities. Either revert the guard to EDDSA-only or add corresponding OID support for SLHDSA.src/lib/crypto/test/SLHDSATests.cpp (2)
69-94: Fix incorrect ByteString lengths for SHAKE variants.All SHAKE variant algorithm names are 18 characters long ("SLH-DSA-SHAKE-128s", etc.), so the ByteString length should be 19 (including null terminator), not 20.
Apply this diff:
slhdsa_names.push_back( ByteString((const unsigned char *)"SLH-DSA-SHA2-128s", 18)); slhdsa_names.push_back( - ByteString((const unsigned char *)"SLH-DSA-SHAKE-128s", 20)); + ByteString((const unsigned char *)"SLH-DSA-SHAKE-128s", 19)); slhdsa_names.push_back( ByteString((const unsigned char *)"SLH-DSA-SHA2-128f", 18)); slhdsa_names.push_back( - ByteString((const unsigned char *)"SLH-DSA-SHAKE-128f", 20)); + ByteString((const unsigned char *)"SLH-DSA-SHAKE-128f", 19)); slhdsa_names.push_back( ByteString((const unsigned char *)"SLH-DSA-SHA2-192s", 18)); slhdsa_names.push_back( - ByteString((const unsigned char *)"SLH-DSA-SHAKE-192s", 20)); + ByteString((const unsigned char *)"SLH-DSA-SHAKE-192s", 19)); slhdsa_names.push_back( ByteString((const unsigned char *)"SLH-DSA-SHA2-192f", 18)); slhdsa_names.push_back( - ByteString((const unsigned char *)"SLH-DSA-SHAKE-192f", 20)); + ByteString((const unsigned char *)"SLH-DSA-SHAKE-192f", 19)); slhdsa_names.push_back( ByteString((const unsigned char *)"SLH-DSA-SHA2-256s", 18)); slhdsa_names.push_back( - ByteString((const unsigned char *)"SLH-DSA-SHAKE-256s", 20)); + ByteString((const unsigned char *)"SLH-DSA-SHAKE-256s", 19)); slhdsa_names.push_back( ByteString((const unsigned char *)"SLH-DSA-SHA2-256f", 18)); slhdsa_names.push_back( - ByteString((const unsigned char *)"SLH-DSA-SHAKE-256f", 20)); + ByteString((const unsigned char *)"SLH-DSA-SHAKE-256f", 19));
194-211: Same ByteString length issue in testSigningVerifying.Identical incorrect length issue for all SHAKE variants (should be 19, not 20).
Apply the same fix as in testKeyGeneration to correct the SHAKE variant lengths from 20 to 19.
src/lib/crypto/OSSLSLHPublicKey.h (1)
78-78: Fix include guard comment typo.The include guard comment references "OSSLDSAPUBLICKEY" instead of "OSSLSLHPUBLICKEY".
Apply this fix:
-#endif // !_SOFTHSM_V2_OSSLDSAPUBLICKEY_H +#endif // !_SOFTHSM_V2_OSSLSLHPUBLICKEY_Hsrc/bin/util/softhsm2-util-ossl.cpp (1)
1183-1207: Critical: Incorrect attribute usage for SLHDSA keys.CKA_SLHDSA_PARAMS is being used to store the entire DER-encoded public/private key (lines 1192, 1206), but per PKCS#11 conventions it should only contain the algorithm parameters (the variant name like "SLH-DSA-SHA2-128s"). The actual key material should be in CKA_VALUE.
Compare with EdDSA (lines 1000-1026):
- CKA_EC_PARAMS stores the algorithm OID (parameters)
- CKA_EC_POINT stores the public key
- CKA_VALUE stores the private key
For SLHDSA, the templates should:
- Extract the algorithm variant/parameters from the EVP_PKEY
- Store only those parameters in CKA_SLHDSA_PARAMS
- Store the public key material in CKA_VALUE (or appropriate attribute) for public keys
- Store the private key material in CKA_VALUE for private keys
This aligns with the PR description noting "private/public key values are stored DER-encoded without separating key attributes" as a known limitation.
m4/acx_crypto_backend.m4 (1)
33-39: Default contradicts help text and disables auto-detection.Help says “default detect” yet the default assignment forces
enable_slhdsa="no", so we never probe OpenSSL unless the user opts in, unlike ECC/EDDSA. Please switch the default to"detect"to match the other toggles and the documented behavior.src/lib/P11Objects.cpp (1)
238-238: Terminate the ERROR_MSG call with a semicolon.
ERROR_MSG("ATTR update error, abortTransaction")still lacks the trailing;, so this file will not compile. Please add the semicolon.- ERROR_MSG("ATTR update error, abortTransaction") + ERROR_MSG("ATTR update error, abortTransaction");src/lib/SoftHSM.cpp (1)
10177-10195: Don’t overwrite CKA_SLHDSA_PARAMS with DER key bytes; store DER in CKA_VALUECKA_SLHDSA_PARAMS must remain the parameter selector. Persist DER-encoded key material under CKA_VALUE instead. This fixes attribute semantics and C_GetAttributeValue behavior.
Apply this diff:
- bOK = bOK && osobject->setAttribute(CKA_SLHDSA_PARAMS, value); + bOK = bOK && osobject->setAttribute(CKA_VALUE, value); ... - bOK = bOK && osobject->setAttribute(CKA_SLHDSA_PARAMS, value); + bOK = bOK && osobject->setAttribute(CKA_VALUE, value);#!/bin/bash # Find remaining places where CKA_SLHDSA_PARAMS might be used as value storage rg -n --no-heading "CKA_SLHDSA_PARAMS" -S -g '!**/build/**' -g '!**/third_party/**'Also applies to: 10256-10270
src/lib/crypto/OSSLSLHPrivateKey.cpp (1)
137-149: Critical: Dangling pointer and broken cache after freeing pkey.After extracting the algorithm name from
pkeyat line 145, the code immediately freespkeyat line 148. This creates two critical issues:
Use-after-free: The
namepointer (line 145) points into memory owned bypkey. Once freed,namebecomes dangling, and subsequent calls togetOrderLength()(line 72) will dereference freed memory.Broken cache: The method is supposed to refresh the cached
pkey, but by freeing it immediately, subsequentgetOSSLKey()calls must recreate it. Additionally, ifsetDerPrivateKey()is called twice, the oldpkeyis never released before updating.The suggested fix in the past review comment correctly addresses both issues by freeing any existing
pkeybefore the update, keeping the newpkeyalive, and storing the algorithm name safely.src/lib/crypto/OSSLSLHDSA.h (1)
68-68: Remove or simplify deriveKey() — SLH-DSA doesn't support key derivation.SLH-DSA is a signature-only algorithm and does not support key derivation operations in OpenSSL. The current implementation (in OSSLSLHDSA.cpp, lines 291–366) attempts ECDH-style derivation which is incompatible with SLH-DSA keys and will fail at runtime.
Recommended approach: Remove the
deriveKey()declaration here and inherit the baseAsymmetricAlgorithm::deriveKey()which returnsfalseby default. Alternatively, override to returnfalsewith a clear error message as done forencrypt()anddecrypt().src/lib/crypto/OSSLSLHDSA.cpp (2)
291-366: Critical: deriveKey() uses incompatible ECDH operations for SLH-DSA.SLH-DSA is a signature-only algorithm and does not support key derivation in OpenSSL. This implementation uses ECDH key agreement operations (
EVP_PKEY_derive_init,EVP_PKEY_derive_set_peer,EVP_PKEY_derive) which are incompatible with SLH-DSA keys and will fail at runtime. Additionally, all error messages reference "ECDH" (lines 306, 315, 328, 338, 347), indicating this was copied from an ECDH/EdDSA implementation.Replace the entire body with an immediate rejection as suggested in the past review comment.
368-378: Critical: EdDSA key sizes used instead of SLH-DSA bounds.The
getMinKeySize()andgetMaxKeySize()functions return EdDSA values (255 for Ed25519, 448 for Ed448) with EdDSA comments. SLH-DSA parameter sets have different key size ranges: public keys are 32–64 bytes (256–512 bits) and private keys are 64–128 bytes (512–1024 bits).Update these functions to reflect correct SLH-DSA bounds as suggested in the past review comment.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (36)
data.txt(1 hunks)m4/acx_crypto_backend.m4(4 hunks)m4/acx_openssl_slhdsa.m4(1 hunks)src/bin/dump/tables.h(3 hunks)src/bin/util/softhsm2-util-ossl.cpp(2 hunks)src/bin/util/softhsm2-util-ossl.h(3 hunks)src/lib/P11Attributes.cpp(1 hunks)src/lib/P11Attributes.h(1 hunks)src/lib/P11Objects.cpp(4 hunks)src/lib/P11Objects.h(2 hunks)src/lib/SoftHSM.cpp(22 hunks)src/lib/SoftHSM.h(3 hunks)src/lib/crypto/AsymmetricAlgorithm.h(2 hunks)src/lib/crypto/CMakeLists.txt(2 hunks)src/lib/crypto/Makefile.am(2 hunks)src/lib/crypto/OSSLCryptoFactory.cpp(2 hunks)src/lib/crypto/OSSLSLHDSA.cpp(1 hunks)src/lib/crypto/OSSLSLHDSA.h(1 hunks)src/lib/crypto/OSSLSLHKeyPair.cpp(1 hunks)src/lib/crypto/OSSLSLHKeyPair.h(1 hunks)src/lib/crypto/OSSLSLHPrivateKey.cpp(1 hunks)src/lib/crypto/OSSLSLHPrivateKey.h(1 hunks)src/lib/crypto/OSSLSLHPublicKey.cpp(1 hunks)src/lib/crypto/OSSLSLHPublicKey.h(1 hunks)src/lib/crypto/OSSLUtil.cpp(1 hunks)src/lib/crypto/OSSLUtil.h(2 hunks)src/lib/crypto/SLHParameters.cpp(1 hunks)src/lib/crypto/SLHParameters.h(1 hunks)src/lib/crypto/SLHPrivateKey.cpp(1 hunks)src/lib/crypto/SLHPrivateKey.h(1 hunks)src/lib/crypto/SLHPublicKey.cpp(1 hunks)src/lib/crypto/SLHPublicKey.h(1 hunks)src/lib/crypto/test/Makefile.am(1 hunks)src/lib/crypto/test/SLHDSATests.cpp(1 hunks)src/lib/crypto/test/SLHDSATests.h(1 hunks)src/lib/pkcs11/pkcs11.h(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- src/lib/P11Attributes.cpp
- src/lib/crypto/AsymmetricAlgorithm.h
- m4/acx_openssl_slhdsa.m4
- src/lib/crypto/Makefile.am
- src/bin/dump/tables.h
- src/lib/P11Objects.h
- src/lib/crypto/test/Makefile.am
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 815
File: src/lib/SoftHSM.cpp:6543-6546
Timestamp: 2025-09-11T16:54:00.370Z
Learning: SoftHSM PR #815 (pkcs11.h 3.1.0 update, CK_RSA_AES_KEY_WRAP_PARAMS fix): Maintainer bukka prefers to keep scope minimal (header sync and param rename only); broader OAEP parameter enforcement or backend plumbing is out of scope for this PR and should be handled in a follow-up issue.
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:5550-5596
Timestamp: 2025-10-27T16:44:57.865Z
Learning: SoftHSM ML‑DSA additional context: avoid transient malloc in SoftHSM::{AsymSignInit, AsymVerifyInit}; let Session::setParameters deep‑copy SIGN_ADDITIONAL_CONTEXT by allocating one buffer for struct + context bytes and fixing the stored pointer; Session::resetOp frees m_pParameters so both struct and context are released.
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:4193-4196
Timestamp: 2025-08-19T09:23:25.434Z
Learning: MLDSA parameter set inference: In src/lib/crypto/MLDSAPrivateKey.{h,cpp} and MLDSAPublicKey.{h,cpp}, getParameterSet() maps key material length to CKP_ML_DSA_44/65/87, and there is no setParameterSet(). Runtime MLDSA keys used by SoftHSM (e.g., in C_SignInit/C_VerifyInit via MLDSAUtil) rely on this inference; no explicit parameter-set assignment is required.
📚 Learning: 2025-09-11T16:54:00.370Z
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 815
File: src/lib/SoftHSM.cpp:6543-6546
Timestamp: 2025-09-11T16:54:00.370Z
Learning: SoftHSM PR #815 (pkcs11.h 3.1.0 update, CK_RSA_AES_KEY_WRAP_PARAMS fix): Maintainer bukka prefers to keep scope minimal (header sync and param rename only); broader OAEP parameter enforcement or backend plumbing is out of scope for this PR and should be handled in a follow-up issue.
Applied to files:
src/lib/crypto/OSSLSLHPrivateKey.hm4/acx_crypto_backend.m4src/lib/crypto/OSSLUtil.hsrc/lib/P11Attributes.hsrc/lib/crypto/CMakeLists.txtsrc/lib/pkcs11/pkcs11.hsrc/lib/crypto/OSSLSLHPublicKey.cppsrc/lib/P11Objects.cppsrc/bin/util/softhsm2-util-ossl.hsrc/lib/SoftHSM.cppsrc/bin/util/softhsm2-util-ossl.cppsrc/lib/crypto/OSSLSLHDSA.cppsrc/lib/crypto/OSSLSLHDSA.hsrc/lib/crypto/OSSLSLHPrivateKey.cppsrc/lib/SoftHSM.hsrc/lib/crypto/OSSLSLHPublicKey.h
📚 Learning: 2025-08-18T17:54:37.645Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: m4/acx_openssl_mldsa.m4:10-35
Timestamp: 2025-08-18T17:54:37.645Z
Learning: For OpenSSL feature detection in autoconf macros, prefer functional tests (like EVP_PKEY_CTX_new_from_name) over version number checks, as the functional approach directly tests the actual capability and is more reliable across different OpenSSL builds where features may be backported or disabled.
Applied to files:
m4/acx_crypto_backend.m4src/lib/crypto/OSSLUtil.hsrc/bin/util/softhsm2-util-ossl.h
📚 Learning: 2025-10-13T09:42:31.270Z
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 818
File: src/lib/SoftHSM.cpp:3238-3266
Timestamp: 2025-10-13T09:42:31.270Z
Learning: In src/lib/SoftHSM.cpp, SoftHSM::AsymDecryptInit should use MechParamCheckRSAPKCSOAEP for CKM_RSA_PKCS_OAEP to keep OAEP parameter validation identical to AsymEncryptInit, and avoid redundant manual checks for pMechanism->pParameter and MGF allowlists.
Applied to files:
m4/acx_crypto_backend.m4src/lib/crypto/OSSLUtil.hsrc/lib/crypto/test/SLHDSATests.cppsrc/lib/crypto/OSSLSLHPublicKey.cppsrc/bin/util/softhsm2-util-ossl.hsrc/lib/SoftHSM.cppsrc/lib/crypto/OSSLCryptoFactory.cppsrc/bin/util/softhsm2-util-ossl.cppsrc/lib/crypto/OSSLUtil.cppsrc/lib/crypto/OSSLSLHDSA.cppsrc/lib/crypto/OSSLSLHDSA.hsrc/lib/crypto/OSSLSLHPrivateKey.cppsrc/lib/SoftHSM.hsrc/lib/crypto/OSSLSLHPublicKey.h
📚 Learning: 2025-10-13T09:43:45.885Z
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 818
File: src/lib/SoftHSM.cpp:13265-13282
Timestamp: 2025-10-13T09:43:45.885Z
Learning: SoftHSM OAEP policy: In SoftHSM::MechParamCheckRSAPKCSOAEP (src/lib/SoftHSM.cpp), enforce that the MGF1 variant must correspond to the selected hashAlg (e.g., SHA-256 -> CKG_MGF1_SHA256). Mismatches should return CKR_ARGUMENTS_BAD with "Hash and MGF don't match". This mirrors the existing PSS enforcement.
Applied to files:
m4/acx_crypto_backend.m4src/lib/P11Attributes.hsrc/lib/crypto/test/SLHDSATests.cppsrc/lib/pkcs11/pkcs11.hsrc/bin/util/softhsm2-util-ossl.hsrc/lib/SoftHSM.cppsrc/lib/crypto/OSSLCryptoFactory.cppsrc/bin/util/softhsm2-util-ossl.cppsrc/lib/crypto/OSSLSLHDSA.cppsrc/lib/SoftHSM.hsrc/lib/crypto/OSSLSLHPublicKey.h
📚 Learning: 2025-08-18T19:46:46.659Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:10044-10055
Timestamp: 2025-08-18T19:46:46.659Z
Learning: In SoftHSM::generateMLDSA (src/lib/SoftHSM.cpp), the project prefers using CKP_ML_DSA_* constants for parameter set checks and logs "Unsupported parameter set: %lu". The code currently returns CKR_PARAMETER_SET_NOT_SUPPORTED for unsupported sets.
Applied to files:
m4/acx_crypto_backend.m4src/lib/P11Attributes.hsrc/lib/crypto/CMakeLists.txtsrc/lib/crypto/SLHParameters.hsrc/lib/pkcs11/pkcs11.hsrc/lib/crypto/OSSLSLHPublicKey.cppsrc/bin/util/softhsm2-util-ossl.hsrc/lib/SoftHSM.cppsrc/bin/util/softhsm2-util-ossl.cppsrc/lib/crypto/OSSLSLHDSA.cppsrc/lib/crypto/OSSLSLHDSA.hsrc/lib/SoftHSM.hsrc/lib/crypto/OSSLSLHPublicKey.h
📚 Learning: 2025-10-27T16:44:57.865Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:5550-5596
Timestamp: 2025-10-27T16:44:57.865Z
Learning: SoftHSM ML‑DSA additional context: avoid transient malloc in SoftHSM::{AsymSignInit, AsymVerifyInit}; let Session::setParameters deep‑copy SIGN_ADDITIONAL_CONTEXT by allocating one buffer for struct + context bytes and fixing the stored pointer; Session::resetOp frees m_pParameters so both struct and context are released.
Applied to files:
src/lib/crypto/OSSLUtil.hsrc/lib/crypto/test/SLHDSATests.cppsrc/lib/crypto/SLHParameters.hsrc/lib/pkcs11/pkcs11.hsrc/lib/crypto/OSSLSLHPublicKey.cppsrc/bin/util/softhsm2-util-ossl.hsrc/lib/SoftHSM.cppsrc/lib/crypto/OSSLCryptoFactory.cppsrc/bin/util/softhsm2-util-ossl.cppsrc/lib/crypto/OSSLSLHDSA.cppsrc/lib/crypto/OSSLSLHDSA.hsrc/lib/crypto/OSSLSLHPrivateKey.cppsrc/lib/SoftHSM.h
📚 Learning: 2025-08-18T17:59:30.693Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/crypto/MLDSAParameters.h:42-44
Timestamp: 2025-08-18T17:59:30.693Z
Learning: In MLDSAParameters.h, the author prefers to keep PKCS#11 constant references (CKP_ML_DSA_*) rather than explicit numeric values for better code clarity and maintainability.
Applied to files:
src/lib/crypto/OSSLUtil.hsrc/lib/P11Attributes.hsrc/lib/crypto/CMakeLists.txtsrc/lib/crypto/SLHParameters.hsrc/lib/pkcs11/pkcs11.hsrc/bin/util/softhsm2-util-ossl.hsrc/lib/SoftHSM.cppsrc/lib/crypto/OSSLCryptoFactory.cppsrc/bin/util/softhsm2-util-ossl.cppsrc/lib/crypto/OSSLSLHDSA.cppsrc/lib/crypto/OSSLSLHDSA.hsrc/lib/SoftHSM.h
📚 Learning: 2025-08-19T09:23:25.434Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:4193-4196
Timestamp: 2025-08-19T09:23:25.434Z
Learning: MLDSA parameter set inference: In src/lib/crypto/MLDSAPrivateKey.{h,cpp} and MLDSAPublicKey.{h,cpp}, getParameterSet() maps key material length to CKP_ML_DSA_44/65/87, and there is no setParameterSet(). Runtime MLDSA keys used by SoftHSM (e.g., in C_SignInit/C_VerifyInit via MLDSAUtil) rely on this inference; no explicit parameter-set assignment is required.
Applied to files:
src/lib/P11Attributes.hsrc/lib/crypto/CMakeLists.txtsrc/lib/crypto/SLHParameters.hsrc/lib/pkcs11/pkcs11.hsrc/lib/crypto/OSSLSLHPublicKey.cppsrc/bin/util/softhsm2-util-ossl.hsrc/lib/SoftHSM.cppsrc/lib/crypto/SLHPublicKey.cppsrc/lib/crypto/OSSLSLHKeyPair.cppsrc/lib/crypto/OSSLCryptoFactory.cppsrc/lib/crypto/SLHPrivateKey.cppsrc/bin/util/softhsm2-util-ossl.cppsrc/lib/crypto/OSSLSLHDSA.cppsrc/lib/crypto/OSSLSLHDSA.hsrc/lib/crypto/SLHPublicKey.hsrc/lib/crypto/OSSLSLHPrivateKey.cppsrc/lib/SoftHSM.hsrc/lib/crypto/SLHPrivateKey.h
📚 Learning: 2025-08-11T16:42:23.012Z
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 806
File: src/lib/test/p11test.cpp:81-84
Timestamp: 2025-08-11T16:42:23.012Z
Learning: In SoftHSM test files (like p11test.cpp and cryptotest.cpp), error handling for OSSL_PROVIDER_load() calls for "legacy" and "default" providers is intentionally omitted because these providers won't fail in practice and the test code is kept deliberately simple.
Applied to files:
src/lib/crypto/test/SLHDSATests.cppsrc/bin/util/softhsm2-util-ossl.hsrc/lib/SoftHSM.cppsrc/bin/util/softhsm2-util-ossl.cppsrc/lib/crypto/test/SLHDSATests.h
🧬 Code graph analysis (19)
src/lib/crypto/OSSLSLHPrivateKey.h (3)
src/lib/crypto/OSSLSLHPrivateKey.cpp (19)
OSSLSLHPrivateKey(43-47)OSSLSLHPrivateKey(49-55)OSSLSLHPrivateKey(58-65)isOfType(132-135)isOfType(132-132)getOrderLength(70-96)getOrderLength(70-70)setDerPrivateKey(137-149)setDerPrivateKey(137-137)PKCS8Encode(152-170)PKCS8Encode(152-152)PKCS8Decode(173-186)PKCS8Decode(173-173)setFromOSSL(99-129)setFromOSSL(99-99)getOSSLKey(189-194)getOSSLKey(189-189)createOSSLKey(197-215)createOSSLKey(197-197)src/lib/crypto/SLHPrivateKey.h (1)
SLHPrivateKey(39-72)src/lib/crypto/SLHPrivateKey.cpp (4)
isOfType(42-45)isOfType(42-42)setDerPrivateKey(59-62)setDerPrivateKey(59-59)
src/lib/P11Attributes.h (2)
src/lib/P11Attributes.cpp (18)
P11Attribute(43-49)P11Attribute(52-54)setDefault(503-507)setDefault(503-503)setDefault(532-536)setDefault(532-532)setDefault(563-567)setDefault(563-563)setDefault(592-596)setDefault(592-592)setDefault(630-634)setDefault(630-630)setDefault(668-672)setDefault(668-668)setDefault(706-710)setDefault(706-706)setDefault(717-721)setDefault(717-717)src/lib/object_store/OSObject.h (1)
OSObject(42-92)
src/lib/crypto/test/SLHDSATests.cpp (1)
src/lib/crypto/test/SLHDSATests.h (1)
SLHDSATests(39-59)
src/lib/crypto/SLHParameters.h (2)
src/lib/crypto/AsymmetricParameters.h (1)
AsymmetricParameters(40-45)src/lib/crypto/SLHParameters.cpp (10)
setName(40-43)setName(40-40)getName(45-48)getName(45-45)areOfType(50-53)areOfType(50-50)serialise(56-59)serialise(56-56)deserialise(61-73)deserialise(61-61)
src/lib/crypto/OSSLSLHPublicKey.cpp (3)
src/lib/crypto/OSSLSLHPublicKey.h (1)
OSSLSLHPublicKey(40-76)src/lib/crypto/OSSLSLHPrivateKey.cpp (10)
setFromOSSL(99-129)setFromOSSL(99-99)getOrderLength(70-96)getOrderLength(70-70)isOfType(132-135)isOfType(132-132)getOSSLKey(189-194)getOSSLKey(189-189)createOSSLKey(197-215)createOSSLKey(197-197)src/lib/crypto/SLHPublicKey.cpp (4)
setDerPublicKey(60-63)setDerPublicKey(60-60)isOfType(42-45)isOfType(42-42)
src/lib/P11Objects.cpp (2)
src/lib/P11Objects.h (2)
P11SLHPublicKeyObj(201-212)P11SLHPrivateKeyObj(303-314)src/lib/P11Attributes.cpp (2)
init(109-120)init(109-109)
src/bin/util/softhsm2-util-ossl.h (1)
src/bin/util/softhsm2-util-ossl.cpp (6)
crypto_save_slhdsa(1159-1232)crypto_save_slhdsa(1159-1167)crypto_malloc_slhdsa(1236-1289)crypto_malloc_slhdsa(1236-1236)crypto_free_slhdsa(1292-1304)crypto_free_slhdsa(1292-1292)
src/lib/SoftHSM.cpp (2)
src/lib/crypto/CryptoFactory.cpp (4)
i(43-46)i(43-43)i(59-62)i(59-59)src/lib/data_mgr/ByteString.cpp (7)
size(310-313)size(310-310)ByteString(49-51)ByteString(53-59)ByteString(61-80)ByteString(82-107)ByteString(109-112)
src/lib/crypto/SLHPublicKey.cpp (2)
src/lib/crypto/OSSLSLHPublicKey.cpp (4)
getOrderLength(70-95)getOrderLength(70-70)setDerPublicKey(129-141)setDerPublicKey(129-129)src/lib/crypto/SLHPrivateKey.cpp (8)
getBitLength(48-51)getBitLength(48-48)getOutputLength(54-57)getOutputLength(54-54)serialise(70-73)serialise(70-70)deserialise(75-86)deserialise(75-75)
src/lib/crypto/SLHPrivateKey.cpp (2)
src/lib/crypto/OSSLSLHPrivateKey.cpp (6)
isOfType(132-135)isOfType(132-132)getOrderLength(70-96)getOrderLength(70-70)setDerPrivateKey(137-149)setDerPrivateKey(137-137)src/lib/crypto/SLHPublicKey.cpp (10)
isOfType(42-45)isOfType(42-42)getBitLength(48-51)getBitLength(48-48)getOutputLength(54-57)getOutputLength(54-54)serialise(72-75)serialise(72-72)deserialise(77-89)deserialise(77-77)
src/lib/crypto/OSSLSLHDSA.h (5)
src/lib/crypto/AsymmetricAlgorithm.h (2)
AsymmetricAlgorithm(121-190)AsymmetricAlgorithm(128-128)src/lib/crypto/AsymmetricKeyPair.h (1)
AsymmetricKeyPair(42-55)src/lib/crypto/AsymmetricParameters.h (1)
AsymmetricParameters(40-45)src/lib/crypto/PublicKey.h (1)
PublicKey(40-54)src/lib/crypto/PrivateKey.h (1)
PrivateKey(41-58)
src/lib/crypto/SLHPublicKey.h (3)
src/lib/crypto/PublicKey.h (1)
PublicKey(40-54)src/lib/crypto/OSSLSLHPublicKey.cpp (6)
isOfType(123-126)isOfType(123-123)getOrderLength(70-95)getOrderLength(70-70)setDerPublicKey(129-141)setDerPublicKey(129-129)src/lib/crypto/SLHPublicKey.cpp (14)
isOfType(42-45)isOfType(42-42)getBitLength(48-51)getBitLength(48-48)getOutputLength(54-57)getOutputLength(54-54)setDerPublicKey(60-63)setDerPublicKey(60-60)getDerPublicKey(66-69)getDerPublicKey(66-66)serialise(72-75)serialise(72-72)deserialise(77-89)deserialise(77-77)
src/lib/crypto/OSSLSLHPrivateKey.cpp (3)
src/lib/crypto/OSSLSLHPrivateKey.h (1)
OSSLSLHPrivateKey(41-83)src/lib/crypto/OSSLSLHPublicKey.cpp (10)
setFromOSSL(98-120)setFromOSSL(98-98)getOrderLength(70-95)getOrderLength(70-70)isOfType(123-126)isOfType(123-123)getOSSLKey(144-149)getOSSLKey(144-144)createOSSLKey(152-165)createOSSLKey(152-152)src/lib/crypto/SLHPrivateKey.cpp (4)
setDerPrivateKey(59-62)setDerPrivateKey(59-59)isOfType(42-45)isOfType(42-42)
src/lib/SoftHSM.h (1)
src/lib/SoftHSM.cpp (6)
generateSLH(10061-10304)generateSLH(10061-10072)getSLHPrivateKey(13029-13055)getSLHPrivateKey(13029-13029)getSLHPublicKey(13057-13082)getSLHPublicKey(13057-13057)
src/lib/crypto/OSSLSLHPublicKey.h (2)
src/lib/crypto/OSSLSLHPublicKey.cpp (15)
OSSLSLHPublicKey(43-47)OSSLSLHPublicKey(49-55)OSSLSLHPublicKey(58-65)isOfType(123-126)isOfType(123-123)getOrderLength(70-95)getOrderLength(70-70)setDerPublicKey(129-141)setDerPublicKey(129-129)setFromOSSL(98-120)setFromOSSL(98-98)getOSSLKey(144-149)getOSSLKey(144-144)createOSSLKey(152-165)createOSSLKey(152-152)src/lib/crypto/SLHPublicKey.h (1)
SLHPublicKey(39-73)
src/lib/crypto/OSSLSLHKeyPair.h (4)
src/lib/crypto/AsymmetricKeyPair.h (1)
AsymmetricKeyPair(42-55)src/lib/crypto/OSSLSLHKeyPair.cpp (12)
setPublicKey(39-42)setPublicKey(39-39)setPrivateKey(45-48)setPrivateKey(45-45)getPublicKey(51-54)getPublicKey(51-51)getConstPublicKey(56-59)getConstPublicKey(56-56)getPrivateKey(62-65)getPrivateKey(62-62)getConstPrivateKey(67-70)getConstPrivateKey(67-67)src/lib/crypto/PublicKey.h (1)
PublicKey(40-54)src/lib/crypto/PrivateKey.h (1)
PrivateKey(41-58)
src/lib/crypto/SLHParameters.cpp (2)
src/lib/crypto/SLHPrivateKey.cpp (4)
serialise(70-73)serialise(70-70)deserialise(75-86)deserialise(75-75)src/lib/crypto/SLHPublicKey.cpp (4)
serialise(72-75)serialise(72-72)deserialise(77-89)deserialise(77-77)
src/lib/crypto/SLHPrivateKey.h (3)
src/lib/crypto/PrivateKey.h (1)
PrivateKey(41-58)src/lib/crypto/OSSLSLHPrivateKey.cpp (6)
isOfType(132-135)isOfType(132-132)getOrderLength(70-96)getOrderLength(70-70)setDerPrivateKey(137-149)setDerPrivateKey(137-137)src/lib/crypto/SLHPrivateKey.cpp (14)
isOfType(42-45)isOfType(42-42)getBitLength(48-51)getBitLength(48-48)getOutputLength(54-57)getOutputLength(54-54)setDerPrivateKey(59-62)setDerPrivateKey(59-59)getDerPrivateKey(64-67)getDerPrivateKey(64-64)serialise(70-73)serialise(70-70)deserialise(75-86)deserialise(75-75)
src/lib/crypto/test/SLHDSATests.h (1)
src/lib/crypto/test/SLHDSATests.cpp (12)
testKeyGeneration(66-113)testKeyGeneration(66-66)testSerialisation(115-162)testSerialisation(115-115)testPKCS8(164-192)testPKCS8(164-164)testSigningVerifying(194-240)testSigningVerifying(194-194)setUp(49-56)setUp(49-49)tearDown(58-64)tearDown(58-58)
🪛 Clang (14.0.6)
src/lib/crypto/OSSLSLHPrivateKey.h
[error] 36-36: 'config.h' file not found
(clang-diagnostic-error)
src/lib/crypto/SLHParameters.h
[error] 36-36: 'config.h' file not found
(clang-diagnostic-error)
src/lib/crypto/OSSLSLHPublicKey.cpp
[error] 33-33: 'config.h' file not found
(clang-diagnostic-error)
src/lib/crypto/SLHPublicKey.cpp
[error] 33-33: 'config.h' file not found
(clang-diagnostic-error)
src/lib/crypto/OSSLSLHKeyPair.cpp
[error] 33-33: 'config.h' file not found
(clang-diagnostic-error)
src/lib/crypto/SLHPrivateKey.cpp
[error] 33-33: 'config.h' file not found
(clang-diagnostic-error)
src/lib/crypto/OSSLSLHDSA.cpp
[error] 33-33: 'config.h' file not found
(clang-diagnostic-error)
src/lib/crypto/OSSLSLHDSA.h
[error] 36-36: 'config.h' file not found
(clang-diagnostic-error)
src/lib/crypto/SLHPublicKey.h
[error] 36-36: 'config.h' file not found
(clang-diagnostic-error)
src/lib/crypto/OSSLSLHPrivateKey.cpp
[error] 33-33: 'config.h' file not found
(clang-diagnostic-error)
src/lib/crypto/OSSLSLHPublicKey.h
[error] 36-36: 'config.h' file not found
(clang-diagnostic-error)
src/lib/crypto/OSSLSLHKeyPair.h
[error] 36-36: 'config.h' file not found
(clang-diagnostic-error)
src/lib/crypto/SLHParameters.cpp
[error] 33-33: 'config.h' file not found
(clang-diagnostic-error)
src/lib/crypto/SLHPrivateKey.h
[error] 36-36: 'config.h' file not found
(clang-diagnostic-error)
src/lib/crypto/test/SLHDSATests.h
[error] 36-36: 'AsymmetricAlgorithm.h' file not found
(clang-diagnostic-error)
🔇 Additional comments (21)
src/lib/crypto/OSSLUtil.cpp (1)
121-121: LGTM!Style consistency improvement using
#if defined()instead of#ifdef.data.txt (1)
1-1: Clarify the purpose of this test data file.This appears to be validation test data based on the PR description. Should this file be in the main source tree, or should it be moved to a test directory or documentation folder? If it's essential for reproduction/validation, consider adding a comment or README explaining its purpose.
src/lib/crypto/OSSLCryptoFactory.cpp (2)
61-63: LGTM!SLHDSA header inclusion follows the established pattern for conditional algorithm support.
354-356: LGTM!Factory method instantiation follows the established pattern and is correctly guarded by
WITH_SLHDSA.src/lib/crypto/CMakeLists.txt (2)
23-29: LGTM!Core SLH parameter and key sources properly added to the build.
64-67: LGTM!OpenSSL-specific SLH implementation sources correctly added to the WITH_OPENSSL block.
src/lib/P11Attributes.h (1)
1147-1160: LGTM!The P11AttrSLHDSAParams class follows the established pattern for parameter attributes (e.g., P11AttrEcParams at lines 1121-1130), using the correct check flags (ck1) and attribute type (CKA_SLHDSA_PARAMS).
src/lib/pkcs11/pkcs11.h (1)
411-411: LGTM!SLHDSA PKCS#11 constants properly defined in the vendor-specific namespace (0x8000xxxx/0x8001xxxx), consistent with other vendor extensions in this codebase.
Also applies to: 489-489, 897-898
src/lib/crypto/test/SLHDSATests.h (1)
33-61: LGTM!Test fixture follows the established CppUnit pattern used for other algorithm tests (e.g., EDDSA). The static analysis error about the missing AsymmetricAlgorithm.h header is a false positive—this is a build-time path issue, not a code defect.
src/lib/crypto/SLHParameters.h (1)
40-58: LGTM!The SLHParameters class follows the established pattern for asymmetric parameter containers in the codebase. The interface is clean with proper type identification, serialization support, and a simple name-based parameter storage matching the SLH-DSA variant naming scheme.
src/lib/crypto/SLHParameters.cpp (1)
38-73: LGTM!The implementation is clean and follows established patterns in the codebase. Error handling in
deserialiseproperly checks for empty ByteString, and the serialization simply delegates to the name's serialization which is appropriate for this parameter class.src/lib/crypto/OSSLSLHPrivateKey.h (1)
41-83: LGTM!The header defines a complete OpenSSL-backed SLH private key wrapper. The interface provides proper PKCS#8 encoding/decoding, DER handling, and OpenSSL interop methods consistent with other OpenSSL key wrappers in the codebase.
src/lib/crypto/OSSLSLHPublicKey.h (1)
40-76: LGTM!The header defines a complete OpenSSL-backed SLH public key wrapper with proper DER handling and OpenSSL interop, consistent with the private key counterpart.
src/lib/crypto/OSSLSLHKeyPair.h (1)
41-64: LGTM!The key pair wrapper correctly follows the AsymmetricKeyPair pattern, providing typed setters for OSSL-specific keys and polymorphic getters returning abstract base pointers for broader compatibility.
src/lib/crypto/SLHPrivateKey.h (1)
39-72: LGTM!The abstract base class provides a clean interface for SLH private keys. The TODO comment at lines 68-70 documents a known limitation (DER-encoded storage without separated attributes) that's also mentioned in the PR description, indicating awareness of this design choice.
src/bin/util/softhsm2-util-ossl.cpp (2)
159-170: LGTM!The early SLHDSA key detection logic correctly identifies SLH-DSA keys by name prefix, up-refs the key, routes to the SLHDSA-specific save function, and cleans up properly. This approach is clean and prevents the key from falling through to the generic switch statement below.
1236-1304: LGTM!The DER encoding logic for both public and private keys is correct, with proper error handling at each step. Memory management in
crypto_free_slhdsacorrectly uses OPENSSL_free for OpenSSL-allocated buffers and standard free for the struct itself.src/lib/SoftHSM.cpp (4)
833-836: Mechanism exposure looks goodNew SLH mechanisms gated behind WITH_SLHDSA are added consistently.
1042-1054: Mechanism info acquisition OKFetching min/max key sizes for SLHDSA mirrors existing patterns. No issues.
4467-4472: SignInit: SLHDSA path is consistentMechanism switch, crypto factory usage, and key extraction align with existing algorithms.
Also applies to: 4560-4579
5499-5505: VerifyInit: SLHDSA path is consistentPublic key allocation/extraction and session setup match established patterns.
Also applies to: 5593-5612
| P11Attribute* attrSLHDSAParams = new P11AttrSLHDSAParams(osobject,P11Attribute::ck3); | ||
| P11Attribute* attrValue = new P11AttrValue(osobject,P11Attribute::ck1|P11Attribute::ck4|P11Attribute::ck6|P11Attribute::ck7); | ||
|
|
||
| // Initialize the attributes | ||
| if | ||
| ( | ||
| !attrSLHDSAParams->init() || | ||
| !attrValue->init() | ||
| ) | ||
| { | ||
| ERROR_MSG("Could not initialize the attribute"); | ||
| delete attrSLHDSAParams; | ||
| delete attrValue; | ||
| return false; | ||
| } | ||
|
|
||
| // Add them to the map | ||
| attributes[attrSLHDSAParams->getType()] = attrSLHDSAParams; | ||
| attributes[attrValue->getType()] = attrValue; | ||
|
|
There was a problem hiding this comment.
CKA_SLHDSA_PARAMS is overwritten with key material.
P11SLHPublicKeyObj expects CKA_SLHDSA_PARAMS to hold the parameter set while CKA_VALUE carries the DER key, yet SoftHSM::generateSLH/getSLHPublicKey currently stuff the DER blob into CKA_SLHDSA_PARAMS. That nukes the caller-supplied params and breaks attribute queries. Please keep CKA_SLHDSA_PARAMS for the parameter-set value and move the DER bits into CKA_VALUE (mirroring other key types).
| else if (keyType == CKK_SLHDSA) | ||
| *p11object = new P11SLHPublicKeyObj(); | ||
| else |
There was a problem hiding this comment.
Wrap SLH P11 object creation with WITH_SLHDSA
Avoid unresolved symbols when SLH is disabled.
- else if (keyType == CKK_SLHDSA)
- *p11object = new P11SLHPublicKeyObj();
+ #ifdef WITH_SLHDSA
+ else if (keyType == CKK_SLHDSA)
+ *p11object = new P11SLHPublicKeyObj();
+ #endif
...
- else if (keyType == CKK_SLHDSA)
- *p11object = new P11SLHPrivateKeyObj();
+ #ifdef WITH_SLHDSA
+ else if (keyType == CKK_SLHDSA)
+ *p11object = new P11SLHPrivateKeyObj();
+ #endifAlso applies to: 163-165
🤖 Prompt for AI Agents
In src/lib/SoftHSM.cpp around lines 144-146 (and similarly 163-165), creation of
P11SLHPublicKeyObj and the SLH-related object is unconditional, causing
unresolved symbol errors when SLH support is disabled; wrap the SLH object
constructions with the existing WITH_SLHDSA compile-time guard (e.g., #ifdef
WITH_SLHDSA / #endif) so those new P11SLH* allocations only compile when
WITH_SLHDSA is defined, keeping non-SLH builds free of references to SLH
symbols.
There was a problem hiding this comment.
I think this comment do not make sense, since EDDSA do not have this guard.
There was a problem hiding this comment.
@JoaoHenrique12 You’re right to push back here.
- In SoftHSM, the P11* object types (P11EDPublicKeyObj/P11EDPrivateKeyObj) are created unconditionally in newP11Object(), and they don’t pull in any crypto backend symbols. They’re attribute containers used by the object store.
- The newly added P11SLHPublicKeyObj/P11SLHPrivateKeyObj follow that same pattern. As long as those classes (and the SLH attribute type P11AttrSLHDSAParams) are compiled unconditionally, referencing them here without a WITH_SLHDSA guard won’t cause unresolved symbols when SLH is disabled. The backend/algorithm code paths themselves are already guarded where needed (mechanism tables, CryptoFactory, OpenSSL impl).
For consistency with EDDSA and to avoid sprinkling preprocessor guards into the object factory, I recommend we keep the SLH branches unguarded here. If we ever decide to guard these, we should do it consistently for both EDDSA and SLH across P11 object creation, not just for SLH.
Unless there’s a concrete build configuration where this causes a failure, I suggest resolving this as “won’t change.”
🧠 Learnings used
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 806
File: src/lib/test/p11test.cpp:81-84
Timestamp: 2025-08-11T16:42:23.012Z
Learning: In SoftHSM test files (like p11test.cpp and cryptotest.cpp), error handling for OSSL_PROVIDER_load() calls for "legacy" and "default" providers is intentionally omitted because these providers won't fail in practice and the test code is kept deliberately simple.
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:10044-10055
Timestamp: 2025-08-18T19:46:46.659Z
Learning: In SoftHSM::generateMLDSA (src/lib/SoftHSM.cpp), the project prefers using CKP_ML_DSA_* constants for parameter set checks and logs "Unsupported parameter set: %lu". The code currently returns CKR_PARAMETER_SET_NOT_SUPPORTED for unsupported sets.
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:4193-4196
Timestamp: 2025-08-19T09:23:25.434Z
Learning: MLDSA parameter set inference: In src/lib/crypto/MLDSAPrivateKey.{h,cpp} and MLDSAPublicKey.{h,cpp}, getParameterSet() maps key material length to CKP_ML_DSA_44/65/87, and there is no setParameterSet(). Runtime MLDSA keys used by SoftHSM (e.g., in C_SignInit/C_VerifyInit via MLDSAUtil) rely on this inference; no explicit parameter-set assignment is required.
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 818
File: src/lib/SoftHSM.cpp:13265-13282
Timestamp: 2025-10-13T09:43:45.885Z
Learning: SoftHSM OAEP policy: In SoftHSM::MechParamCheckRSAPKCSOAEP (src/lib/SoftHSM.cpp), enforce that the MGF1 variant must correspond to the selected hashAlg (e.g., SHA-256 -> CKG_MGF1_SHA256). Mismatches should return CKR_ARGUMENTS_BAD with "Hash and MGF don't match". This mirrors the existing PSS enforcement.
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 815
File: src/lib/SoftHSM.cpp:6543-6546
Timestamp: 2025-09-11T16:54:00.370Z
Learning: SoftHSM PR #815 (pkcs11.h 3.1.0 update, CK_RSA_AES_KEY_WRAP_PARAMS fix): Maintainer bukka prefers to keep scope minimal (header sync and param rename only); broader OAEP parameter enforcement or backend plumbing is out of scope for this PR and should be handled in a follow-up issue.
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/crypto/MLDSAParameters.h:42-44
Timestamp: 2025-08-18T17:59:30.693Z
Learning: In MLDSAParameters.h, the author prefers to keep PKCS#11 constant references (CKP_ML_DSA_*) rather than explicit numeric values for better code clarity and maintainability.
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 818
File: src/lib/SoftHSM.cpp:3238-3266
Timestamp: 2025-10-13T09:42:31.270Z
Learning: In src/lib/SoftHSM.cpp, SoftHSM::AsymDecryptInit should use MechParamCheckRSAPKCSOAEP for CKM_RSA_PKCS_OAEP to keep OAEP parameter validation identical to AsymEncryptInit, and avoid redundant manual checks for pMechanism->pParameter and MGF allowlists.
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:5550-5596
Timestamp: 2025-10-27T16:44:57.865Z
Learning: SoftHSM ML‑DSA additional context: avoid transient malloc in SoftHSM::{AsymSignInit, AsymVerifyInit}; let Session::setParameters deep‑copy SIGN_ADDITIONAL_CONTEXT by allocating one buffer for struct + context bytes and fixing the stored pointer; Session::resetOp frees m_pParameters so both struct and context are released.
src/lib/SoftHSM.cpp
Outdated
| ERROR_MSG("signatureLen=%d, size=%d", *pulSignatureLen, size); | ||
| *pulSignatureLen = size; | ||
| return CKR_BUFFER_TOO_SMALL; |
There was a problem hiding this comment.
Fix printf format specifiers for size types in log messages
Use %lu (cast) or %zu to avoid UB and incorrect logs.
- ERROR_MSG("signatureLen=%d, size=%d", *pulSignatureLen, size);
+ ERROR_MSG("signatureLen=%lu, size=%lu", (unsigned long)*pulSignatureLen, (unsigned long)size);
...
- ERROR_MSG("The size of the signature differs from the size of the mechanism %d, %d", signature.size(), size);
+ ERROR_MSG("The size of the signature differs from the size of the mechanism %lu, %lu",
+ (unsigned long)signature.size(), (unsigned long)size);Also applies to: 4760-4764
🤖 Prompt for AI Agents
In src/lib/SoftHSM.cpp around lines 4727-4729 (and also 4760-4764), the log
calls use %d for variables representing sizes which can cause UB and incorrect
output; update the format specifiers to use the proper size type (e.g., %zu for
size_t or %lu with an explicit cast to unsigned long) and cast the variables
accordingly when calling ERROR_MSG so the printed types match the format
specifier.
There was a problem hiding this comment.
removed exploratory log.
There was a problem hiding this comment.
@JoaoHenrique12 thanks — acknowledged. If all that was left was the exploratory line you mentioned, we’re good here.
Minor follow‑up: there’s also an ERROR_MSG a few lines below that compares signature.size() to size. If it’s still present in your latest push, consider switching its %d specifiers to an unsigned type, e.g.:
- Use %lu with casts to (unsigned long), or
- Use %zu for size_t and %lu for CK_ULONG.
Example:
ERROR_MSG("The size of the signature differs from the size of the mechanism %lu, %lu",
(unsigned long)signature.size(), (unsigned long)size);
If you already removed or fixed that one too, then nothing else to do.
🧠 Learnings used
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:5550-5596
Timestamp: 2025-10-27T16:44:57.865Z
Learning: SoftHSM ML‑DSA additional context: avoid transient malloc in SoftHSM::{AsymSignInit, AsymVerifyInit}; let Session::setParameters deep‑copy SIGN_ADDITIONAL_CONTEXT by allocating one buffer for struct + context bytes and fixing the stored pointer; Session::resetOp frees m_pParameters so both struct and context are released.
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 818
File: src/lib/SoftHSM.cpp:3238-3266
Timestamp: 2025-10-13T09:42:31.270Z
Learning: In src/lib/SoftHSM.cpp, SoftHSM::AsymDecryptInit should use MechParamCheckRSAPKCSOAEP for CKM_RSA_PKCS_OAEP to keep OAEP parameter validation identical to AsymEncryptInit, and avoid redundant manual checks for pMechanism->pParameter and MGF allowlists.
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 818
File: src/lib/SoftHSM.cpp:13265-13282
Timestamp: 2025-10-13T09:43:45.885Z
Learning: SoftHSM OAEP policy: In SoftHSM::MechParamCheckRSAPKCSOAEP (src/lib/SoftHSM.cpp), enforce that the MGF1 variant must correspond to the selected hashAlg (e.g., SHA-256 -> CKG_MGF1_SHA256). Mismatches should return CKR_ARGUMENTS_BAD with "Hash and MGF don't match". This mirrors the existing PSS enforcement.
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:10044-10055
Timestamp: 2025-08-18T19:46:46.659Z
Learning: In SoftHSM::generateMLDSA (src/lib/SoftHSM.cpp), the project prefers using CKP_ML_DSA_* constants for parameter set checks and logs "Unsupported parameter set: %lu". The code currently returns CKR_PARAMETER_SET_NOT_SUPPORTED for unsupported sets.
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 815
File: src/lib/SoftHSM.cpp:6543-6546
Timestamp: 2025-09-11T16:54:00.370Z
Learning: SoftHSM PR #815 (pkcs11.h 3.1.0 update, CK_RSA_AES_KEY_WRAP_PARAMS fix): Maintainer bukka prefers to keep scope minimal (header sync and param rename only); broader OAEP parameter enforcement or backend plumbing is out of scope for this PR and should be handled in a follow-up issue.
Learnt from: JoaoHenrique12
Repo: softhsm/SoftHSMv2 PR: 823
File: src/lib/SoftHSM.cpp:58-58
Timestamp: 2025-11-11T19:42:46.859Z
Learning: SoftHSM include policy: Algorithm key/parameter headers (e.g., SLHParameters, SLH*Key) are included unconditionally in SoftHSM.cpp/SoftHSM.h; feature gating is handled in mechanism tables and CryptoFactory (WITH_*). Avoid wrapping such includes in WITH_* guards.
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:4193-4196
Timestamp: 2025-08-19T09:23:25.434Z
Learning: MLDSA parameter set inference: In src/lib/crypto/MLDSAPrivateKey.{h,cpp} and MLDSAPublicKey.{h,cpp}, getParameterSet() maps key material length to CKP_ML_DSA_44/65/87, and there is no setParameterSet(). Runtime MLDSA keys used by SoftHSM (e.g., in C_SignInit/C_VerifyInit via MLDSAUtil) rely on this inference; no explicit parameter-set assignment is required.
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/crypto/MLDSAParameters.h:42-44
Timestamp: 2025-08-18T17:59:30.693Z
Learning: In MLDSAParameters.h, the author prefers to keep PKCS#11 constant references (CKP_ML_DSA_*) rather than explicit numeric values for better code clarity and maintainability.
| CK_RV SoftHSM::getSLHPrivateKey(SLHPrivateKey* privateKey, Token* token, OSObject* key) | ||
| { | ||
| if (privateKey == NULL) return CKR_ARGUMENTS_BAD; | ||
| if (token == NULL) return CKR_ARGUMENTS_BAD; | ||
| if (key == NULL) return CKR_ARGUMENTS_BAD; | ||
|
|
||
| // Get the CKA_PRIVATE attribute, when the attribute is not present use default false | ||
| bool isKeyPrivate = key->getBooleanValue(CKA_PRIVATE, false); | ||
|
|
||
| // SLHDSA Private Key Attributes | ||
| ByteString value; | ||
| if (isKeyPrivate) | ||
| { | ||
| bool bOK = true; | ||
| bOK = bOK && token->decrypt(key->getByteStringValue(CKA_SLHDSA_PARAMS), value); | ||
| if (!bOK) | ||
| return CKR_GENERAL_ERROR; | ||
| } | ||
| else | ||
| { | ||
| value = key->getByteStringValue(CKA_SLHDSA_PARAMS); | ||
| } | ||
|
|
||
| privateKey->setDerPrivateKey(value); | ||
|
|
||
| return CKR_OK; | ||
| } |
There was a problem hiding this comment.
getSLHPrivateKey should read DER from CKA_VALUE, not CKA_SLHDSA_PARAMS
Maintain params and value separation; decrypt CKA_VALUE when private.
- ByteString value;
+ ByteString value;
if (isKeyPrivate)
{
bool bOK = true;
- bOK = bOK && token->decrypt(key->getByteStringValue(CKA_SLHDSA_PARAMS), value);
+ bOK = bOK && token->decrypt(key->getByteStringValue(CKA_VALUE), value);
if (!bOK)
return CKR_GENERAL_ERROR;
}
else
{
- value = key->getByteStringValue(CKA_SLHDSA_PARAMS);
+ value = key->getByteStringValue(CKA_VALUE);
}🤖 Prompt for AI Agents
In src/lib/SoftHSM.cpp around lines 13029 to 13055, change the logic so the SLH
params remain read from CKA_SLHDSA_PARAMS while the DER-encoded private key is
read from CKA_VALUE; when CKA_PRIVATE is true call token->decrypt on the
CKA_VALUE into the ByteString value (and return CKR_GENERAL_ERROR on failure),
otherwise assign value directly from CKA_VALUE, then call
privateKey->setDerPrivateKey(value); do not decrypt or treat CKA_SLHDSA_PARAMS
as the DER private value.
| CK_RV SoftHSM::getSLHPublicKey(SLHPublicKey* publicKey, Token* token, OSObject* key) | ||
| { | ||
| if (publicKey == NULL) return CKR_ARGUMENTS_BAD; | ||
| if (token == NULL) return CKR_ARGUMENTS_BAD; | ||
| if (key == NULL) return CKR_ARGUMENTS_BAD; | ||
|
|
||
| // Get the CKA_PRIVATE attribute, when the attribute is not present use default false | ||
| bool isKeyPrivate = key->getBooleanValue(CKA_PRIVATE, false); | ||
|
|
||
| // SLHDSA Public Key Attributes | ||
| ByteString value; | ||
| if (isKeyPrivate) | ||
| { | ||
| bool bOK = true; | ||
| bOK = bOK && token->decrypt(key->getByteStringValue(CKA_SLHDSA_PARAMS), value); | ||
| if (!bOK) | ||
| return CKR_GENERAL_ERROR; | ||
| } | ||
| else | ||
| { | ||
| value = key->getByteStringValue(CKA_SLHDSA_PARAMS); | ||
| } | ||
|
|
||
| publicKey->setDerPublicKey(value); | ||
| return CKR_OK; | ||
| } |
There was a problem hiding this comment.
getSLHPublicKey should read DER from CKA_VALUE, not CKA_SLHDSA_PARAMS
Same separation on public keys.
- ByteString value;
+ ByteString value;
if (isKeyPrivate)
{
bool bOK = true;
- bOK = bOK && token->decrypt(key->getByteStringValue(CKA_SLHDSA_PARAMS), value);
+ bOK = bOK && token->decrypt(key->getByteStringValue(CKA_VALUE), value);
if (!bOK)
return CKR_GENERAL_ERROR;
}
else
{
- value = key->getByteStringValue(CKA_SLHDSA_PARAMS);
+ value = key->getByteStringValue(CKA_VALUE);
}🤖 Prompt for AI Agents
In src/lib/SoftHSM.cpp around lines 13057 to 13082, the function getSLHPublicKey
incorrectly reads the DER-encoded public key from CKA_SLHDSA_PARAMS; change it
to read from CKA_VALUE instead. Specifically, when isKeyPrivate is true call
token->decrypt(key->getByteStringValue(CKA_VALUE), value) and when false assign
value = key->getByteStringValue(CKA_VALUE), then call
publicKey->setDerPublicKey(value). Keep the same null checks and return codes.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/lib/crypto/OSSLSLHDSA.cpp (1)
297-305: Incorrect key size values.getMinKeySize() returns 7856 and getMaxKeySize() returns 49856, which don't correspond to any SLHDSA parameter set sizes. SLHDSA keys range from 32 bytes (256 bits, smallest public key in 128s/f) to 128 bytes (1024 bits, largest private key in 256s/f).
Apply this diff to use correct SLHDSA key size bounds:
unsigned long OSSLSLHDSA::getMinKeySize() { - return 7856; + // Minimum SLHDSA public key: 32 bytes (SLH-DSA-*-128s/f) + return 256; } unsigned long OSSLSLHDSA::getMaxKeySize() { - return 49856; + // Maximum SLHDSA private key: 128 bytes (SLH-DSA-*-256s/f) + return 1024; }
🧹 Nitpick comments (2)
src/lib/crypto/OSSLUtil.h (2)
42-44: Consider maintaining consistent preprocessor style.The change from
#ifdefto#if defined()is functionally equivalent but introduces inconsistency with line 39 (#ifdef WITH_ECC). Unless this is part of a broader style update, consider keeping the style uniform throughout the file.-#if defined(WITH_EDDSA) +#ifdef WITH_EDDSA #include <openssl/objects.h> #endif
68-74: LGTM: Guard remains correctly EDDSA-only.The guard correctly remains EDDSA-specific, which addresses the past review concern. Based on previous verification, SLHDSA doesn't use these OID conversion utilities, so keeping them EDDSA-only is appropriate.
However, the same stylistic inconsistency noted for line 42 applies here as well.
-#if defined(WITH_EDDSA) +#ifdef WITH_EDDSA // Convert an OpenSSL NID to a ByteString ByteString oid2ByteString(int nid); // Convert a ByteString to an OpenSSL NID int byteString2oid(const ByteString& byteString); #endif
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
m4/acx_crypto_backend.m4(4 hunks)src/lib/P11Objects.cpp(2 hunks)src/lib/crypto/OSSLSLHDSA.cpp(1 hunks)src/lib/crypto/OSSLUtil.h(2 hunks)src/lib/crypto/test/SLHDSATests.cpp(1 hunks)
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 815
File: src/lib/SoftHSM.cpp:6543-6546
Timestamp: 2025-09-11T16:54:00.370Z
Learning: SoftHSM PR #815 (pkcs11.h 3.1.0 update, CK_RSA_AES_KEY_WRAP_PARAMS fix): Maintainer bukka prefers to keep scope minimal (header sync and param rename only); broader OAEP parameter enforcement or backend plumbing is out of scope for this PR and should be handled in a follow-up issue.
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:5550-5596
Timestamp: 2025-10-27T16:44:57.865Z
Learning: SoftHSM ML‑DSA additional context: avoid transient malloc in SoftHSM::{AsymSignInit, AsymVerifyInit}; let Session::setParameters deep‑copy SIGN_ADDITIONAL_CONTEXT by allocating one buffer for struct + context bytes and fixing the stored pointer; Session::resetOp frees m_pParameters so both struct and context are released.
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:4193-4196
Timestamp: 2025-08-19T09:23:25.434Z
Learning: MLDSA parameter set inference: In src/lib/crypto/MLDSAPrivateKey.{h,cpp} and MLDSAPublicKey.{h,cpp}, getParameterSet() maps key material length to CKP_ML_DSA_44/65/87, and there is no setParameterSet(). Runtime MLDSA keys used by SoftHSM (e.g., in C_SignInit/C_VerifyInit via MLDSAUtil) rely on this inference; no explicit parameter-set assignment is required.
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:10044-10055
Timestamp: 2025-08-18T19:46:46.659Z
Learning: In SoftHSM::generateMLDSA (src/lib/SoftHSM.cpp), the project prefers using CKP_ML_DSA_* constants for parameter set checks and logs "Unsupported parameter set: %lu". The code currently returns CKR_PARAMETER_SET_NOT_SUPPORTED for unsupported sets.
📚 Learning: 2025-09-11T16:54:00.370Z
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 815
File: src/lib/SoftHSM.cpp:6543-6546
Timestamp: 2025-09-11T16:54:00.370Z
Learning: SoftHSM PR #815 (pkcs11.h 3.1.0 update, CK_RSA_AES_KEY_WRAP_PARAMS fix): Maintainer bukka prefers to keep scope minimal (header sync and param rename only); broader OAEP parameter enforcement or backend plumbing is out of scope for this PR and should be handled in a follow-up issue.
Applied to files:
src/lib/P11Objects.cppm4/acx_crypto_backend.m4src/lib/crypto/OSSLSLHDSA.cppsrc/lib/crypto/OSSLUtil.h
📚 Learning: 2025-10-27T16:44:57.865Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:5550-5596
Timestamp: 2025-10-27T16:44:57.865Z
Learning: SoftHSM ML‑DSA additional context: avoid transient malloc in SoftHSM::{AsymSignInit, AsymVerifyInit}; let Session::setParameters deep‑copy SIGN_ADDITIONAL_CONTEXT by allocating one buffer for struct + context bytes and fixing the stored pointer; Session::resetOp frees m_pParameters so both struct and context are released.
Applied to files:
src/lib/P11Objects.cppsrc/lib/crypto/OSSLSLHDSA.cppsrc/lib/crypto/test/SLHDSATests.cppsrc/lib/crypto/OSSLUtil.h
📚 Learning: 2025-08-18T17:59:30.693Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/crypto/MLDSAParameters.h:42-44
Timestamp: 2025-08-18T17:59:30.693Z
Learning: In MLDSAParameters.h, the author prefers to keep PKCS#11 constant references (CKP_ML_DSA_*) rather than explicit numeric values for better code clarity and maintainability.
Applied to files:
src/lib/P11Objects.cppsrc/lib/crypto/OSSLSLHDSA.cpp
📚 Learning: 2025-08-18T19:46:46.659Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:10044-10055
Timestamp: 2025-08-18T19:46:46.659Z
Learning: In SoftHSM::generateMLDSA (src/lib/SoftHSM.cpp), the project prefers using CKP_ML_DSA_* constants for parameter set checks and logs "Unsupported parameter set: %lu". The code currently returns CKR_PARAMETER_SET_NOT_SUPPORTED for unsupported sets.
Applied to files:
src/lib/P11Objects.cppm4/acx_crypto_backend.m4src/lib/crypto/OSSLSLHDSA.cpp
📚 Learning: 2025-10-13T09:42:31.270Z
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 818
File: src/lib/SoftHSM.cpp:3238-3266
Timestamp: 2025-10-13T09:42:31.270Z
Learning: In src/lib/SoftHSM.cpp, SoftHSM::AsymDecryptInit should use MechParamCheckRSAPKCSOAEP for CKM_RSA_PKCS_OAEP to keep OAEP parameter validation identical to AsymEncryptInit, and avoid redundant manual checks for pMechanism->pParameter and MGF allowlists.
Applied to files:
src/lib/P11Objects.cppm4/acx_crypto_backend.m4src/lib/crypto/OSSLSLHDSA.cppsrc/lib/crypto/test/SLHDSATests.cppsrc/lib/crypto/OSSLUtil.h
📚 Learning: 2025-08-19T09:23:25.434Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:4193-4196
Timestamp: 2025-08-19T09:23:25.434Z
Learning: MLDSA parameter set inference: In src/lib/crypto/MLDSAPrivateKey.{h,cpp} and MLDSAPublicKey.{h,cpp}, getParameterSet() maps key material length to CKP_ML_DSA_44/65/87, and there is no setParameterSet(). Runtime MLDSA keys used by SoftHSM (e.g., in C_SignInit/C_VerifyInit via MLDSAUtil) rely on this inference; no explicit parameter-set assignment is required.
Applied to files:
src/lib/P11Objects.cppsrc/lib/crypto/OSSLSLHDSA.cpp
📚 Learning: 2025-10-13T09:43:45.885Z
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 818
File: src/lib/SoftHSM.cpp:13265-13282
Timestamp: 2025-10-13T09:43:45.885Z
Learning: SoftHSM OAEP policy: In SoftHSM::MechParamCheckRSAPKCSOAEP (src/lib/SoftHSM.cpp), enforce that the MGF1 variant must correspond to the selected hashAlg (e.g., SHA-256 -> CKG_MGF1_SHA256). Mismatches should return CKR_ARGUMENTS_BAD with "Hash and MGF don't match". This mirrors the existing PSS enforcement.
Applied to files:
src/lib/P11Objects.cppm4/acx_crypto_backend.m4src/lib/crypto/OSSLSLHDSA.cppsrc/lib/crypto/test/SLHDSATests.cpp
📚 Learning: 2025-08-18T17:54:37.645Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: m4/acx_openssl_mldsa.m4:10-35
Timestamp: 2025-08-18T17:54:37.645Z
Learning: For OpenSSL feature detection in autoconf macros, prefer functional tests (like EVP_PKEY_CTX_new_from_name) over version number checks, as the functional approach directly tests the actual capability and is more reliable across different OpenSSL builds where features may be backported or disabled.
Applied to files:
m4/acx_crypto_backend.m4src/lib/crypto/OSSLUtil.h
📚 Learning: 2025-08-11T16:42:23.012Z
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 806
File: src/lib/test/p11test.cpp:81-84
Timestamp: 2025-08-11T16:42:23.012Z
Learning: In SoftHSM test files (like p11test.cpp and cryptotest.cpp), error handling for OSSL_PROVIDER_load() calls for "legacy" and "default" providers is intentionally omitted because these providers won't fail in practice and the test code is kept deliberately simple.
Applied to files:
src/lib/crypto/test/SLHDSATests.cpp
🧬 Code graph analysis (2)
src/lib/P11Objects.cpp (2)
src/lib/P11Objects.h (2)
P11SLHPublicKeyObj(201-212)P11SLHPrivateKeyObj(303-314)src/lib/P11Attributes.cpp (2)
init(109-120)init(109-109)
src/lib/crypto/test/SLHDSATests.cpp (1)
src/lib/crypto/test/SLHDSATests.h (1)
SLHDSATests(39-59)
🪛 Clang (14.0.6)
src/lib/crypto/OSSLSLHDSA.cpp
[error] 33-33: 'config.h' file not found
(clang-diagnostic-error)
🔇 Additional comments (13)
src/lib/crypto/test/SLHDSATests.cpp (2)
66-113: LGTM! ByteString lengths corrected.The ByteString length issue for SHAKE variants flagged in previous reviews has been fixed (all SHAKE variants now use 19 bytes, matching the 18-character string length plus null terminator). The test comprehensively covers all 12 SLHDSA parameter sets and validates key generation.
194-240: LGTM! Sign/verify test complete.The signing and verification test correctly covers all 12 SLHDSA variants with properly sized ByteStrings (SHAKE variants use 19, SHA2 variants use 18). The test validates end-to-end sign/verify operations with RNG-generated data.
src/lib/P11Objects.cpp (2)
932-975: LGTM! P11SLHPublicKeyObj follows established patterns.The implementation correctly mirrors the structure of other public key types (e.g., P11EDPublicKeyObj). The attribute setup with CKA_SLHDSA_PARAMS and CKA_VALUE is appropriate, with the ck3 flag indicating params must be specified during generation.
1382-1425: LGTM! P11SLHPrivateKeyObj follows established patterns.The implementation correctly mirrors the structure of other private key types (e.g., P11EDPrivateKeyObj). The attribute flags (ck4|ck6 for params, ck1|ck4|ck6|ck7 for value) match the patterns used by comparable key types.
m4/acx_crypto_backend.m4 (3)
31-39: LGTM! Default value corrected.The default for
enable_slhdsahas been corrected to "detect" (line 38), matching the help string and aligning with the pattern used by other optional features (ECC, EDDSA, GOST).
113-119: LGTM! OpenSSL SLHDSA detection properly integrated.The detection logic follows the established pattern: calls ACX_OPENSSL_SLHDSA on "yes|detect", then validates the result and either errors (explicit "yes" with no support) or syncs enable_slhdsa with detection outcome.
199-202: LGTM! Botan handling correct.Appropriately warns users and disables SLHDSA when Botan backend is selected with explicit SLHDSA request, since Botan does not support SLHDSA.
src/lib/crypto/OSSLSLHDSA.cpp (6)
48-108: LGTM! Sign function properly implemented.The signing implementation correctly uses EVP_DigestSignInit/EVP_DigestSign, validates mechanism and key type, handles errors with descriptive messages, and properly manages OpenSSL context lifecycle.
110-130: LGTM! Multi-part signing correctly rejected.SLHDSA does not support multi-part signing operations, and these functions correctly return errors with clear messages.
132-190: LGTM! Verify function properly implemented.The verification implementation correctly uses EVP_DigestVerifyInit/EVP_DigestVerify, validates mechanism, key type, and signature length, and properly manages the OpenSSL context lifecycle.
214-230: LGTM! Encryption/decryption correctly rejected.SLHDSA is a signature-only algorithm and these functions correctly return errors.
232-289: LGTM! Key generation properly implemented.The key generation correctly uses EVP_PKEY_keygen with the parameter set name, validates inputs, handles errors, and properly manages OpenSSL context lifecycle.
291-295: LGTM! deriveKey issue from previous review resolved.The deriveKey function now correctly returns an error immediately (line 293-294) instead of attempting ECDH operations, as flagged in the previous review.
Well, both are PQC added on OpenSSL 3.5.0, and the implementation in the mentioned PR seems similar to mine (since both had used der encoding for represent public/private key). I should contribute to that issue trying run it's implementation using hpcs-pkcs11. I'll take a deep look there and will see if there is some comment that should be applied here either. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
m4/acx_crypto_backend.m4 (1)
31-40: SLHDSA defaults to "no" while other features use "detect".The SLHDSA option defaults to
"no"and requires explicit enabling, whereas ECC, EDDSA, and GOST all default to"detect"and enable automatically when the backend supports them. This makes SLHDSA opt-in rather than auto-detected.If this is intentional (e.g., because SLHDSA is experimental or requires OpenSSL ≥ 3.5.0), the current implementation is correct. Otherwise, consider aligning with the existing pattern for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
m4/acx_crypto_backend.m4(4 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 815
File: src/lib/SoftHSM.cpp:6543-6546
Timestamp: 2025-09-11T16:54:00.370Z
Learning: SoftHSM PR #815 (pkcs11.h 3.1.0 update, CK_RSA_AES_KEY_WRAP_PARAMS fix): Maintainer bukka prefers to keep scope minimal (header sync and param rename only); broader OAEP parameter enforcement or backend plumbing is out of scope for this PR and should be handled in a follow-up issue.
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:5550-5596
Timestamp: 2025-10-27T16:44:57.865Z
Learning: SoftHSM ML‑DSA additional context: avoid transient malloc in SoftHSM::{AsymSignInit, AsymVerifyInit}; let Session::setParameters deep‑copy SIGN_ADDITIONAL_CONTEXT by allocating one buffer for struct + context bytes and fixing the stored pointer; Session::resetOp frees m_pParameters so both struct and context are released.
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:4193-4196
Timestamp: 2025-08-19T09:23:25.434Z
Learning: MLDSA parameter set inference: In src/lib/crypto/MLDSAPrivateKey.{h,cpp} and MLDSAPublicKey.{h,cpp}, getParameterSet() maps key material length to CKP_ML_DSA_44/65/87, and there is no setParameterSet(). Runtime MLDSA keys used by SoftHSM (e.g., in C_SignInit/C_VerifyInit via MLDSAUtil) rely on this inference; no explicit parameter-set assignment is required.
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:10044-10055
Timestamp: 2025-08-18T19:46:46.659Z
Learning: In SoftHSM::generateMLDSA (src/lib/SoftHSM.cpp), the project prefers using CKP_ML_DSA_* constants for parameter set checks and logs "Unsupported parameter set: %lu". The code currently returns CKR_PARAMETER_SET_NOT_SUPPORTED for unsupported sets.
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 818
File: src/lib/SoftHSM.cpp:13265-13282
Timestamp: 2025-10-13T09:43:45.885Z
Learning: SoftHSM OAEP policy: In SoftHSM::MechParamCheckRSAPKCSOAEP (src/lib/SoftHSM.cpp), enforce that the MGF1 variant must correspond to the selected hashAlg (e.g., SHA-256 -> CKG_MGF1_SHA256). Mismatches should return CKR_ARGUMENTS_BAD with "Hash and MGF don't match". This mirrors the existing PSS enforcement.
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 818
File: src/lib/SoftHSM.cpp:3238-3266
Timestamp: 2025-10-13T09:42:31.270Z
Learning: In src/lib/SoftHSM.cpp, SoftHSM::AsymDecryptInit should use MechParamCheckRSAPKCSOAEP for CKM_RSA_PKCS_OAEP to keep OAEP parameter validation identical to AsymEncryptInit, and avoid redundant manual checks for pMechanism->pParameter and MGF allowlists.
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/crypto/MLDSAParameters.h:42-44
Timestamp: 2025-08-18T17:59:30.693Z
Learning: In MLDSAParameters.h, the author prefers to keep PKCS#11 constant references (CKP_ML_DSA_*) rather than explicit numeric values for better code clarity and maintainability.
📚 Learning: 2025-08-18T17:54:37.645Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: m4/acx_openssl_mldsa.m4:10-35
Timestamp: 2025-08-18T17:54:37.645Z
Learning: For OpenSSL feature detection in autoconf macros, prefer functional tests (like EVP_PKEY_CTX_new_from_name) over version number checks, as the functional approach directly tests the actual capability and is more reliable across different OpenSSL builds where features may be backported or disabled.
Applied to files:
m4/acx_crypto_backend.m4
📚 Learning: 2025-09-11T16:54:00.370Z
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 815
File: src/lib/SoftHSM.cpp:6543-6546
Timestamp: 2025-09-11T16:54:00.370Z
Learning: SoftHSM PR #815 (pkcs11.h 3.1.0 update, CK_RSA_AES_KEY_WRAP_PARAMS fix): Maintainer bukka prefers to keep scope minimal (header sync and param rename only); broader OAEP parameter enforcement or backend plumbing is out of scope for this PR and should be handled in a follow-up issue.
Applied to files:
m4/acx_crypto_backend.m4
📚 Learning: 2025-10-13T09:42:31.270Z
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 818
File: src/lib/SoftHSM.cpp:3238-3266
Timestamp: 2025-10-13T09:42:31.270Z
Learning: In src/lib/SoftHSM.cpp, SoftHSM::AsymDecryptInit should use MechParamCheckRSAPKCSOAEP for CKM_RSA_PKCS_OAEP to keep OAEP parameter validation identical to AsymEncryptInit, and avoid redundant manual checks for pMechanism->pParameter and MGF allowlists.
Applied to files:
m4/acx_crypto_backend.m4
📚 Learning: 2025-10-13T09:43:45.885Z
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 818
File: src/lib/SoftHSM.cpp:13265-13282
Timestamp: 2025-10-13T09:43:45.885Z
Learning: SoftHSM OAEP policy: In SoftHSM::MechParamCheckRSAPKCSOAEP (src/lib/SoftHSM.cpp), enforce that the MGF1 variant must correspond to the selected hashAlg (e.g., SHA-256 -> CKG_MGF1_SHA256). Mismatches should return CKR_ARGUMENTS_BAD with "Hash and MGF don't match". This mirrors the existing PSS enforcement.
Applied to files:
m4/acx_crypto_backend.m4
📚 Learning: 2025-08-18T19:46:46.659Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:10044-10055
Timestamp: 2025-08-18T19:46:46.659Z
Learning: In SoftHSM::generateMLDSA (src/lib/SoftHSM.cpp), the project prefers using CKP_ML_DSA_* constants for parameter set checks and logs "Unsupported parameter set: %lu". The code currently returns CKR_PARAMETER_SET_NOT_SUPPORTED for unsupported sets.
Applied to files:
m4/acx_crypto_backend.m4
📚 Learning: 2025-10-27T16:44:57.865Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:5550-5596
Timestamp: 2025-10-27T16:44:57.865Z
Learning: SoftHSM ML‑DSA additional context: avoid transient malloc in SoftHSM::{AsymSignInit, AsymVerifyInit}; let Session::setParameters deep‑copy SIGN_ADDITIONAL_CONTEXT by allocating one buffer for struct + context bytes and fixing the stored pointer; Session::resetOp frees m_pParameters so both struct and context are released.
Applied to files:
m4/acx_crypto_backend.m4
🔇 Additional comments (2)
m4/acx_crypto_backend.m4 (2)
113-120: LGTM!The OpenSSL SLHDSA detection logic correctly follows the established pattern for ECC and EDDSA feature detection, with proper error handling and auto-detection support.
257-268: LGTM!The SLHDSA support declaration and conditional setup correctly follow the established pattern for ECC, GOST, and EDDSA features.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
m4/acx_crypto_backend.m4 (1)
199-202: Fix indentation to use tabs.Lines 200–201 use spaces for indentation, but the rest of the file uses tabs. This inconsistency was flagged in a previous review but remains unfixed.
Apply this diff to fix the indentation:
if test "x${enable_slhdsa}" = "xyes"; then - AC_MSG_WARN([SLHDSA is not supported with Botan. Disabling.]) - enable_slhdsa="no" + AC_MSG_WARN([SLHDSA is not supported with Botan. Disabling.]) + enable_slhdsa="no" fisrc/lib/SoftHSM.cpp (4)
10059-10303: Don’t overwrite CKA_SLHDSA_PARAMS; store DER in CKA_VALUE.Currently, DER key bytes replace CKA_SLHDSA_PARAMS on both pub/priv objects, erasing the parameter set. Keep params in CKA_SLHDSA_PARAMS and put DER into CKA_VALUE (encrypted when private), as done for other algs.
Apply these targeted diffs:
@@ - // SLHDSA Public Key Attributes - ByteString value; - if (isPublicKeyPrivate) - { - token->encrypt(pub->getDerPublicKey(), value); - } - else - { - value = pub->getDerPublicKey(); - } - bOK = bOK && osobject->setAttribute(CKA_SLHDSA_PARAMS, value); + // SLHDSA Public Key Attributes + ByteString derPub = pub->getDerPublicKey(); + ByteString value; + if (isPublicKeyPrivate) + { + token->encrypt(derPub, value); + } + else + { + value = derPub; + } + // Store DER under CKA_VALUE, keep params under CKA_SLHDSA_PARAMS + bOK = bOK && osobject->setAttribute(CKA_VALUE, value); + bOK = bOK && osobject->setAttribute(CKA_SLHDSA_PARAMS, params); @@ - // SLHDSA Private Key Attributes - ByteString value; - if (isPrivateKeyPrivate) - { - token->encrypt(priv->getDerPrivateKey(), value); - } - else - { - value = priv->getDerPrivateKey(); - } - bOK = bOK && osobject->setAttribute(CKA_SLHDSA_PARAMS, value); + // SLHDSA Private Key Attributes + ByteString derPriv = priv->getDerPrivateKey(); + ByteString value; + if (isPrivateKeyPrivate) + { + token->encrypt(derPriv, value); + } + else + { + value = derPriv; + } + // Store DER under CKA_VALUE, keep params under CKA_SLHDSA_PARAMS + bOK = bOK && osobject->setAttribute(CKA_VALUE, value); + bOK = bOK && osobject->setAttribute(CKA_SLHDSA_PARAMS, params);#!/bin/bash # Find remaining writes of DER into CKA_SLHDSA_PARAMS rg -nP -C2 'setAttribute\s*\(\s*CKA_SLHDSA_PARAMS\s*,\s*value\s*\)' src | sed -n '1,120p' # Check calls that read/write SLH attrs rg -n 'CKA_SLHDSA_PARAMS|CKA_VALUE' src/lib/SoftHSM.cpp -n -C2
13027-13053: getSLHPrivateKey must read DER from CKA_VALUE (decrypt when private).Reading from CKA_SLHDSA_PARAMS conflates parameters and value. Switch to CKA_VALUE.
Apply this diff:
@@ - // SLHDSA Private Key Attributes + // SLHDSA Private Key Attributes ByteString value; if (isKeyPrivate) { bool bOK = true; - bOK = bOK && token->decrypt(key->getByteStringValue(CKA_SLHDSA_PARAMS), value); + bOK = bOK && token->decrypt(key->getByteStringValue(CKA_VALUE), value); if (!bOK) return CKR_GENERAL_ERROR; } else { - value = key->getByteStringValue(CKA_SLHDSA_PARAMS); + value = key->getByteStringValue(CKA_VALUE); } privateKey->setDerPrivateKey(value);
13055-13080: getSLHPublicKey must read DER from CKA_VALUE (decrypt when private).Same separation for public key.
Apply this diff:
@@ - // SLHDSA Public Key Attributes + // SLHDSA Public Key Attributes ByteString value; if (isKeyPrivate) { bool bOK = true; - bOK = bOK && token->decrypt(key->getByteStringValue(CKA_SLHDSA_PARAMS), value); + bOK = bOK && token->decrypt(key->getByteStringValue(CKA_VALUE), value); if (!bOK) return CKR_GENERAL_ERROR; } else { - value = key->getByteStringValue(CKA_SLHDSA_PARAMS); + value = key->getByteStringValue(CKA_VALUE); } publicKey->setDerPublicKey(value);
4761-4764: Fix printf specifiers for size types.Use %lu with casts (or %zu if supported) to avoid UB.
Apply this diff:
- ERROR_MSG("The size of the signature differs from the size of the mechanism %d, %d", signature.size(), size); + ERROR_MSG("The size of the signature differs from the size of the mechanism %lu, %lu", + (unsigned long)signature.size(), (unsigned long)size);
🧹 Nitpick comments (2)
m4/acx_crypto_backend.m4 (1)
33-39: Consider aligning default with other crypto features.SLHDSA currently defaults to
"no"while ECC (line 10), GOST (line 18), and EDDSA (line 28) all default to"detect". This inconsistency breaks the established pattern for optional crypto features. The backend-specific nature ("OpenSSL only") is already handled by the detection logic at lines 113–119 (which will setenable_slhdsa="no"automatically when Botan is selected), so defaulting to"detect"would maintain consistency without requiring users to explicitly enable the feature when OpenSSL provides it.Apply this diff to align with the project's pattern:
AC_ARG_ENABLE(slhdsa, AS_HELP_STRING([--enable-slhdsa], - [Enable support for SLHDSA (default disabled, OpenSSL only)] + [Enable support for SLHDSA (default detect, OpenSSL only)] ), [enable_slhdsa="${enableval}"], - [enable_slhdsa="no"] + [enable_slhdsa="detect"] )src/lib/SoftHSM.cpp (1)
937-945: Unnecessary SLH tie-in for ECDH/EDDSA size vars.Including WITH_SLHDSA here can leave ecdh/eddsa size vars declared without their corresponding sections, inviting unused-var warnings when only SLH is enabled. Suggest reverting to the original condition.
Apply this diff:
-#if defined(WITH_ECC) || defined(WITH_EDDSA) || defined(WITH_SLHDSA) +#if defined(WITH_ECC) || defined(WITH_EDDSA) unsigned long ecdhMinSize = 0, ecdhMaxSize = 0; unsigned long eddsaMinSize = 0, eddsaMaxSize = 0; #endif
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
m4/acx_crypto_backend.m4(4 hunks)src/lib/SoftHSM.cpp(21 hunks)
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 815
File: src/lib/SoftHSM.cpp:6543-6546
Timestamp: 2025-09-11T16:54:00.370Z
Learning: SoftHSM PR #815 (pkcs11.h 3.1.0 update, CK_RSA_AES_KEY_WRAP_PARAMS fix): Maintainer bukka prefers to keep scope minimal (header sync and param rename only); broader OAEP parameter enforcement or backend plumbing is out of scope for this PR and should be handled in a follow-up issue.
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:5550-5596
Timestamp: 2025-10-27T16:44:57.865Z
Learning: SoftHSM ML‑DSA additional context: avoid transient malloc in SoftHSM::{AsymSignInit, AsymVerifyInit}; let Session::setParameters deep‑copy SIGN_ADDITIONAL_CONTEXT by allocating one buffer for struct + context bytes and fixing the stored pointer; Session::resetOp frees m_pParameters so both struct and context are released.
Learnt from: JoaoHenrique12
Repo: softhsm/SoftHSMv2 PR: 823
File: src/lib/SoftHSM.cpp:58-58
Timestamp: 2025-11-11T19:42:46.859Z
Learning: SoftHSM include policy: Algorithm key/parameter headers (e.g., SLHParameters, SLH*Key) are included unconditionally in SoftHSM.cpp/SoftHSM.h; feature gating is handled in mechanism tables and CryptoFactory (WITH_*). Avoid wrapping such includes in WITH_* guards.
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:4193-4196
Timestamp: 2025-08-19T09:23:25.434Z
Learning: MLDSA parameter set inference: In src/lib/crypto/MLDSAPrivateKey.{h,cpp} and MLDSAPublicKey.{h,cpp}, getParameterSet() maps key material length to CKP_ML_DSA_44/65/87, and there is no setParameterSet(). Runtime MLDSA keys used by SoftHSM (e.g., in C_SignInit/C_VerifyInit via MLDSAUtil) rely on this inference; no explicit parameter-set assignment is required.
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:10044-10055
Timestamp: 2025-08-18T19:46:46.659Z
Learning: In SoftHSM::generateMLDSA (src/lib/SoftHSM.cpp), the project prefers using CKP_ML_DSA_* constants for parameter set checks and logs "Unsupported parameter set: %lu". The code currently returns CKR_PARAMETER_SET_NOT_SUPPORTED for unsupported sets.
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 818
File: src/lib/SoftHSM.cpp:13265-13282
Timestamp: 2025-10-13T09:43:45.885Z
Learning: SoftHSM OAEP policy: In SoftHSM::MechParamCheckRSAPKCSOAEP (src/lib/SoftHSM.cpp), enforce that the MGF1 variant must correspond to the selected hashAlg (e.g., SHA-256 -> CKG_MGF1_SHA256). Mismatches should return CKR_ARGUMENTS_BAD with "Hash and MGF don't match". This mirrors the existing PSS enforcement.
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 818
File: src/lib/SoftHSM.cpp:3238-3266
Timestamp: 2025-10-13T09:42:31.270Z
Learning: In src/lib/SoftHSM.cpp, SoftHSM::AsymDecryptInit should use MechParamCheckRSAPKCSOAEP for CKM_RSA_PKCS_OAEP to keep OAEP parameter validation identical to AsymEncryptInit, and avoid redundant manual checks for pMechanism->pParameter and MGF allowlists.
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/crypto/MLDSAParameters.h:42-44
Timestamp: 2025-08-18T17:59:30.693Z
Learning: In MLDSAParameters.h, the author prefers to keep PKCS#11 constant references (CKP_ML_DSA_*) rather than explicit numeric values for better code clarity and maintainability.
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 806
File: src/lib/test/p11test.cpp:81-84
Timestamp: 2025-08-11T16:42:23.012Z
Learning: In SoftHSM test files (like p11test.cpp and cryptotest.cpp), error handling for OSSL_PROVIDER_load() calls for "legacy" and "default" providers is intentionally omitted because these providers won't fail in practice and the test code is kept deliberately simple.
📚 Learning: 2025-08-18T17:54:37.645Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: m4/acx_openssl_mldsa.m4:10-35
Timestamp: 2025-08-18T17:54:37.645Z
Learning: For OpenSSL feature detection in autoconf macros, prefer functional tests (like EVP_PKEY_CTX_new_from_name) over version number checks, as the functional approach directly tests the actual capability and is more reliable across different OpenSSL builds where features may be backported or disabled.
Applied to files:
m4/acx_crypto_backend.m4
📚 Learning: 2025-11-11T19:42:46.859Z
Learnt from: JoaoHenrique12
Repo: softhsm/SoftHSMv2 PR: 823
File: src/lib/SoftHSM.cpp:58-58
Timestamp: 2025-11-11T19:42:46.859Z
Learning: SoftHSM include policy: Algorithm key/parameter headers (e.g., SLHParameters, SLH*Key) are included unconditionally in SoftHSM.cpp/SoftHSM.h; feature gating is handled in mechanism tables and CryptoFactory (WITH_*). Avoid wrapping such includes in WITH_* guards.
Applied to files:
m4/acx_crypto_backend.m4src/lib/SoftHSM.cpp
📚 Learning: 2025-10-13T09:42:31.270Z
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 818
File: src/lib/SoftHSM.cpp:3238-3266
Timestamp: 2025-10-13T09:42:31.270Z
Learning: In src/lib/SoftHSM.cpp, SoftHSM::AsymDecryptInit should use MechParamCheckRSAPKCSOAEP for CKM_RSA_PKCS_OAEP to keep OAEP parameter validation identical to AsymEncryptInit, and avoid redundant manual checks for pMechanism->pParameter and MGF allowlists.
Applied to files:
m4/acx_crypto_backend.m4src/lib/SoftHSM.cpp
📚 Learning: 2025-10-13T09:43:45.885Z
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 818
File: src/lib/SoftHSM.cpp:13265-13282
Timestamp: 2025-10-13T09:43:45.885Z
Learning: SoftHSM OAEP policy: In SoftHSM::MechParamCheckRSAPKCSOAEP (src/lib/SoftHSM.cpp), enforce that the MGF1 variant must correspond to the selected hashAlg (e.g., SHA-256 -> CKG_MGF1_SHA256). Mismatches should return CKR_ARGUMENTS_BAD with "Hash and MGF don't match". This mirrors the existing PSS enforcement.
Applied to files:
m4/acx_crypto_backend.m4src/lib/SoftHSM.cpp
📚 Learning: 2025-08-18T19:46:46.659Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:10044-10055
Timestamp: 2025-08-18T19:46:46.659Z
Learning: In SoftHSM::generateMLDSA (src/lib/SoftHSM.cpp), the project prefers using CKP_ML_DSA_* constants for parameter set checks and logs "Unsupported parameter set: %lu". The code currently returns CKR_PARAMETER_SET_NOT_SUPPORTED for unsupported sets.
Applied to files:
m4/acx_crypto_backend.m4src/lib/SoftHSM.cpp
📚 Learning: 2025-10-27T16:44:57.865Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:5550-5596
Timestamp: 2025-10-27T16:44:57.865Z
Learning: SoftHSM ML‑DSA additional context: avoid transient malloc in SoftHSM::{AsymSignInit, AsymVerifyInit}; let Session::setParameters deep‑copy SIGN_ADDITIONAL_CONTEXT by allocating one buffer for struct + context bytes and fixing the stored pointer; Session::resetOp frees m_pParameters so both struct and context are released.
Applied to files:
m4/acx_crypto_backend.m4src/lib/SoftHSM.cpp
📚 Learning: 2025-09-11T16:54:00.370Z
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 815
File: src/lib/SoftHSM.cpp:6543-6546
Timestamp: 2025-09-11T16:54:00.370Z
Learning: SoftHSM PR #815 (pkcs11.h 3.1.0 update, CK_RSA_AES_KEY_WRAP_PARAMS fix): Maintainer bukka prefers to keep scope minimal (header sync and param rename only); broader OAEP parameter enforcement or backend plumbing is out of scope for this PR and should be handled in a follow-up issue.
Applied to files:
m4/acx_crypto_backend.m4src/lib/SoftHSM.cpp
📚 Learning: 2025-08-19T09:23:25.434Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:4193-4196
Timestamp: 2025-08-19T09:23:25.434Z
Learning: MLDSA parameter set inference: In src/lib/crypto/MLDSAPrivateKey.{h,cpp} and MLDSAPublicKey.{h,cpp}, getParameterSet() maps key material length to CKP_ML_DSA_44/65/87, and there is no setParameterSet(). Runtime MLDSA keys used by SoftHSM (e.g., in C_SignInit/C_VerifyInit via MLDSAUtil) rely on this inference; no explicit parameter-set assignment is required.
Applied to files:
src/lib/SoftHSM.cpp
📚 Learning: 2025-08-18T17:59:30.693Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/crypto/MLDSAParameters.h:42-44
Timestamp: 2025-08-18T17:59:30.693Z
Learning: In MLDSAParameters.h, the author prefers to keep PKCS#11 constant references (CKP_ML_DSA_*) rather than explicit numeric values for better code clarity and maintainability.
Applied to files:
src/lib/SoftHSM.cpp
📚 Learning: 2025-08-11T16:42:23.012Z
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 806
File: src/lib/test/p11test.cpp:81-84
Timestamp: 2025-08-11T16:42:23.012Z
Learning: In SoftHSM test files (like p11test.cpp and cryptotest.cpp), error handling for OSSL_PROVIDER_load() calls for "legacy" and "default" providers is intentionally omitted because these providers won't fail in practice and the test code is kept deliberately simple.
Applied to files:
src/lib/SoftHSM.cpp
🧬 Code graph analysis (1)
src/lib/SoftHSM.cpp (2)
src/lib/crypto/CryptoFactory.cpp (4)
i(43-46)i(43-43)i(59-62)i(59-59)src/lib/data_mgr/ByteString.cpp (7)
size(310-313)size(310-310)ByteString(49-51)ByteString(53-59)ByteString(61-80)ByteString(82-107)ByteString(109-112)
🔇 Additional comments (10)
m4/acx_crypto_backend.m4 (2)
113-119: LGTM!The OpenSSL backend integration correctly follows the established pattern used by EDDSA (lines 104–111). The case logic properly invokes the detection macro, errors on missing support when explicitly enabled, and auto-detects when in detect mode.
257-268: LGTM!The conditional definition for
WITH_SLHDSAcorrectly follows the established pattern used by ECC (lines 218–229), GOST (lines 231–242), and EDDSA (lines 244–255). The AC_DEFINE and AM_CONDITIONAL setup is consistent and correct.src/lib/SoftHSM.cpp (8)
58-58: Include policy alignment (no guard needed).Unconditional include of SLHParameters matches the project’s pattern; availability is gated elsewhere. LGTM. Based on learnings.
144-145: P11 SLH object creation consistency.Unconditional P11SLH* creation mirrors EDDSA and won’t pull backend symbols. LGTM. Based on learnings.
Also applies to: 163-164
832-836: Mechanism table: SLH entries behind WITH_SLHDSA.Correct gating and naming. LGTM.
1042-1055: SLHDSA min/max key size retrieval.Looks correct and matches other algs. LGTM.
1346-1357: C_GetMechanismInfo: SLHDSA cases.Flags and min/max set as expected. LGTM.
6223-6226: C_GenerateKeyPair: SLHDSA integration and routing.Template checks and dispatch to generateSLH() look good. LGTM.
Also applies to: 6254-6256, 6280-6282, 6346-6353
4197-4200: SignInit: Plumbing for SLHDSA is fine, depends on getters.Flow mirrors other algs. Ensure getSLHPrivateKey reads DER from CKA_VALUE (see separate comment) or signing will fail for objects saved with correct attribute separation.
Also applies to: 4467-4472, 4560-4578
5231-5234: VerifyInit: SLHDSA path added.Looks consistent. As above, correctness hinges on getSLHPublicKey reading DER from CKA_VALUE.
Also applies to: 5499-5504, 5592-5611
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/lib/crypto/OSSLSLHPrivateKey.cpp (1)
115-127: Dangling pointer:namebecomes invalid after freeingpkey.This is the same critical issue flagged in previous reviews. Lines 119-123 obtain the
EVP_PKEYviagetOSSLKey(), capturenamefromEVP_PKEY_get0_type_name(pkey), then immediately free the key (lines 125-126). SinceEVP_PKEY_get0_type_namereturns a pointer to the internal string within theEVP_PKEYstructure, freeingpkeyinvalidatesname. Subsequent calls togetOrderLength()(line 73) will dereference freed memory.Apply the fix from the previous review: free any existing
pkeybefore updating the DER buffer, keep the newEVP_PKEYalive, and capturenameonly after verifying the key is valid:void OSSLSLHPrivateKey::setDerPrivateKey(const ByteString& inSk) { + if (pkey != NULL) + { + EVP_PKEY_free(pkey); + pkey = NULL; + name = NULL; + } + SLHPrivateKey::setDerPrivateKey(inSk); - getOSSLKey(); - if (EVP_PKEY_get0_type_name(pkey) == NULL) - { ERROR_MSG("Could not determine algorithm name from EVP_PKEY"); return; } - - name = EVP_PKEY_get0_type_name(pkey); - - if (pkey) - { EVP_PKEY_free(pkey); pkey = NULL; } + if (derPrivateKey.size() == 0) return; + + pkey = getOSSLKey(); + if (pkey == NULL) + { + ERROR_MSG("Could not materialize EVP_PKEY from DER"); + return; + } + + name = EVP_PKEY_get0_type_name(pkey); + if (name == NULL) + { + ERROR_MSG("Could not determine algorithm name from EVP_PKEY"); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/lib/crypto/Makefile.am(2 hunks)src/lib/crypto/OSSLSLHPrivateKey.cpp(1 hunks)src/lib/crypto/OSSLSLHPublicKey.cpp(1 hunks)src/lib/crypto/OSSLSLHUtil.cpp(1 hunks)src/lib/crypto/OSSLSLHUtil.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/crypto/Makefile.am
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 815
File: src/lib/SoftHSM.cpp:6543-6546
Timestamp: 2025-09-11T16:54:00.370Z
Learning: SoftHSM PR #815 (pkcs11.h 3.1.0 update, CK_RSA_AES_KEY_WRAP_PARAMS fix): Maintainer bukka prefers to keep scope minimal (header sync and param rename only); broader OAEP parameter enforcement or backend plumbing is out of scope for this PR and should be handled in a follow-up issue.
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:5550-5596
Timestamp: 2025-10-27T16:44:57.865Z
Learning: SoftHSM ML‑DSA additional context: avoid transient malloc in SoftHSM::{AsymSignInit, AsymVerifyInit}; let Session::setParameters deep‑copy SIGN_ADDITIONAL_CONTEXT by allocating one buffer for struct + context bytes and fixing the stored pointer; Session::resetOp frees m_pParameters so both struct and context are released.
Learnt from: JoaoHenrique12
Repo: softhsm/SoftHSMv2 PR: 823
File: src/lib/SoftHSM.cpp:58-58
Timestamp: 2025-11-11T19:42:46.859Z
Learning: SoftHSM include policy: Algorithm key/parameter headers (e.g., SLHParameters, SLH*Key) are included unconditionally in SoftHSM.cpp/SoftHSM.h; feature gating is handled in mechanism tables and CryptoFactory (WITH_*). Avoid wrapping such includes in WITH_* guards.
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 818
File: src/lib/SoftHSM.cpp:3238-3266
Timestamp: 2025-10-13T09:42:31.270Z
Learning: In src/lib/SoftHSM.cpp, SoftHSM::AsymDecryptInit should use MechParamCheckRSAPKCSOAEP for CKM_RSA_PKCS_OAEP to keep OAEP parameter validation identical to AsymEncryptInit, and avoid redundant manual checks for pMechanism->pParameter and MGF allowlists.
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:4193-4196
Timestamp: 2025-08-19T09:23:25.434Z
Learning: MLDSA parameter set inference: In src/lib/crypto/MLDSAPrivateKey.{h,cpp} and MLDSAPublicKey.{h,cpp}, getParameterSet() maps key material length to CKP_ML_DSA_44/65/87, and there is no setParameterSet(). Runtime MLDSA keys used by SoftHSM (e.g., in C_SignInit/C_VerifyInit via MLDSAUtil) rely on this inference; no explicit parameter-set assignment is required.
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:10044-10055
Timestamp: 2025-08-18T19:46:46.659Z
Learning: In SoftHSM::generateMLDSA (src/lib/SoftHSM.cpp), the project prefers using CKP_ML_DSA_* constants for parameter set checks and logs "Unsupported parameter set: %lu". The code currently returns CKR_PARAMETER_SET_NOT_SUPPORTED for unsupported sets.
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 818
File: src/lib/SoftHSM.cpp:13265-13282
Timestamp: 2025-10-13T09:43:45.885Z
Learning: SoftHSM OAEP policy: In SoftHSM::MechParamCheckRSAPKCSOAEP (src/lib/SoftHSM.cpp), enforce that the MGF1 variant must correspond to the selected hashAlg (e.g., SHA-256 -> CKG_MGF1_SHA256). Mismatches should return CKR_ARGUMENTS_BAD with "Hash and MGF don't match". This mirrors the existing PSS enforcement.
📚 Learning: 2025-11-11T19:42:46.859Z
Learnt from: JoaoHenrique12
Repo: softhsm/SoftHSMv2 PR: 823
File: src/lib/SoftHSM.cpp:58-58
Timestamp: 2025-11-11T19:42:46.859Z
Learning: SoftHSM include policy: Algorithm key/parameter headers (e.g., SLHParameters, SLH*Key) are included unconditionally in SoftHSM.cpp/SoftHSM.h; feature gating is handled in mechanism tables and CryptoFactory (WITH_*). Avoid wrapping such includes in WITH_* guards.
Applied to files:
src/lib/crypto/OSSLSLHUtil.hsrc/lib/crypto/OSSLSLHPublicKey.cppsrc/lib/crypto/OSSLSLHPrivateKey.cpp
📚 Learning: 2025-08-19T09:23:25.434Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:4193-4196
Timestamp: 2025-08-19T09:23:25.434Z
Learning: MLDSA parameter set inference: In src/lib/crypto/MLDSAPrivateKey.{h,cpp} and MLDSAPublicKey.{h,cpp}, getParameterSet() maps key material length to CKP_ML_DSA_44/65/87, and there is no setParameterSet(). Runtime MLDSA keys used by SoftHSM (e.g., in C_SignInit/C_VerifyInit via MLDSAUtil) rely on this inference; no explicit parameter-set assignment is required.
Applied to files:
src/lib/crypto/OSSLSLHPublicKey.cppsrc/lib/crypto/OSSLSLHPrivateKey.cpp
📚 Learning: 2025-10-27T16:44:57.865Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:5550-5596
Timestamp: 2025-10-27T16:44:57.865Z
Learning: SoftHSM ML‑DSA additional context: avoid transient malloc in SoftHSM::{AsymSignInit, AsymVerifyInit}; let Session::setParameters deep‑copy SIGN_ADDITIONAL_CONTEXT by allocating one buffer for struct + context bytes and fixing the stored pointer; Session::resetOp frees m_pParameters so both struct and context are released.
Applied to files:
src/lib/crypto/OSSLSLHPublicKey.cppsrc/lib/crypto/OSSLSLHPrivateKey.cpp
📚 Learning: 2025-09-11T16:54:00.370Z
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 815
File: src/lib/SoftHSM.cpp:6543-6546
Timestamp: 2025-09-11T16:54:00.370Z
Learning: SoftHSM PR #815 (pkcs11.h 3.1.0 update, CK_RSA_AES_KEY_WRAP_PARAMS fix): Maintainer bukka prefers to keep scope minimal (header sync and param rename only); broader OAEP parameter enforcement or backend plumbing is out of scope for this PR and should be handled in a follow-up issue.
Applied to files:
src/lib/crypto/OSSLSLHPublicKey.cppsrc/lib/crypto/OSSLSLHPrivateKey.cpp
📚 Learning: 2025-10-13T09:42:31.270Z
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 818
File: src/lib/SoftHSM.cpp:3238-3266
Timestamp: 2025-10-13T09:42:31.270Z
Learning: In src/lib/SoftHSM.cpp, SoftHSM::AsymDecryptInit should use MechParamCheckRSAPKCSOAEP for CKM_RSA_PKCS_OAEP to keep OAEP parameter validation identical to AsymEncryptInit, and avoid redundant manual checks for pMechanism->pParameter and MGF allowlists.
Applied to files:
src/lib/crypto/OSSLSLHPublicKey.cppsrc/lib/crypto/OSSLSLHPrivateKey.cpp
📚 Learning: 2025-08-18T19:46:46.659Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:10044-10055
Timestamp: 2025-08-18T19:46:46.659Z
Learning: In SoftHSM::generateMLDSA (src/lib/SoftHSM.cpp), the project prefers using CKP_ML_DSA_* constants for parameter set checks and logs "Unsupported parameter set: %lu". The code currently returns CKR_PARAMETER_SET_NOT_SUPPORTED for unsupported sets.
Applied to files:
src/lib/crypto/OSSLSLHPublicKey.cpp
📚 Learning: 2025-10-13T09:43:45.885Z
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 818
File: src/lib/SoftHSM.cpp:13265-13282
Timestamp: 2025-10-13T09:43:45.885Z
Learning: SoftHSM OAEP policy: In SoftHSM::MechParamCheckRSAPKCSOAEP (src/lib/SoftHSM.cpp), enforce that the MGF1 variant must correspond to the selected hashAlg (e.g., SHA-256 -> CKG_MGF1_SHA256). Mismatches should return CKR_ARGUMENTS_BAD with "Hash and MGF don't match". This mirrors the existing PSS enforcement.
Applied to files:
src/lib/crypto/OSSLSLHPublicKey.cpp
📚 Learning: 2025-08-18T17:54:37.645Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: m4/acx_openssl_mldsa.m4:10-35
Timestamp: 2025-08-18T17:54:37.645Z
Learning: For OpenSSL feature detection in autoconf macros, prefer functional tests (like EVP_PKEY_CTX_new_from_name) over version number checks, as the functional approach directly tests the actual capability and is more reliable across different OpenSSL builds where features may be backported or disabled.
Applied to files:
src/lib/crypto/OSSLSLHPublicKey.cpp
🧬 Code graph analysis (3)
src/lib/crypto/OSSLSLHUtil.h (1)
src/lib/crypto/OSSLSLHUtil.cpp (2)
getSignatureSizeFromName(39-70)getSignatureSizeFromName(39-39)
src/lib/crypto/OSSLSLHPublicKey.cpp (4)
src/lib/crypto/OSSLSLHPublicKey.h (1)
OSSLSLHPublicKey(40-76)src/lib/crypto/OSSLSLHPrivateKey.cpp (10)
setFromOSSL(77-107)setFromOSSL(77-77)getOrderLength(71-74)getOrderLength(71-71)isOfType(110-113)isOfType(110-110)getOSSLKey(167-172)getOSSLKey(167-167)createOSSLKey(175-193)createOSSLKey(175-175)src/lib/crypto/OSSLSLHUtil.cpp (2)
getSignatureSizeFromName(39-70)getSignatureSizeFromName(39-39)src/lib/crypto/SLHPublicKey.cpp (4)
setDerPublicKey(60-63)setDerPublicKey(60-60)isOfType(42-45)isOfType(42-42)
src/lib/crypto/OSSLSLHPrivateKey.cpp (4)
src/lib/crypto/OSSLSLHPrivateKey.h (1)
OSSLSLHPrivateKey(41-83)src/lib/crypto/OSSLSLHPublicKey.cpp (10)
setFromOSSL(77-99)setFromOSSL(77-77)getOrderLength(71-74)getOrderLength(71-71)isOfType(102-105)isOfType(102-102)getOSSLKey(123-128)getOSSLKey(123-123)createOSSLKey(131-144)createOSSLKey(131-131)src/lib/crypto/OSSLSLHUtil.cpp (2)
getSignatureSizeFromName(39-70)getSignatureSizeFromName(39-39)src/lib/crypto/SLHPrivateKey.cpp (4)
setDerPrivateKey(59-62)setDerPrivateKey(59-59)isOfType(42-45)isOfType(42-42)
🪛 Clang (14.0.6)
src/lib/crypto/OSSLSLHUtil.cpp
[error] 34-34: 'log.h' file not found
(clang-diagnostic-error)
src/lib/crypto/OSSLSLHUtil.h
[error] 36-36: unknown type name 'namespace'
(clang-diagnostic-error)
[error] 36-36: expected ';' after top level declarator
(clang-diagnostic-error)
src/lib/crypto/OSSLSLHPublicKey.cpp
[error] 33-33: 'config.h' file not found
(clang-diagnostic-error)
src/lib/crypto/OSSLSLHPrivateKey.cpp
[error] 33-33: 'config.h' file not found
(clang-diagnostic-error)
🔇 Additional comments (2)
src/lib/crypto/OSSLSLHUtil.cpp (1)
39-70: LGTM: Proper input validation and error handling.The implementation correctly validates inputs (NULL check and minimum length requirement) before processing the algorithm name suffix. The signature size mappings align with the SLH-DSA specification variants documented in the PR description.
src/lib/crypto/OSSLSLHUtil.h (1)
33-40: LGTM: Clean header design.The header properly declares the utility function within the OSSLSLH namespace with appropriate include guards.
| void OSSLSLHPublicKey::setDerPublicKey(const ByteString& inPk) | ||
| { | ||
| SLHPublicKey::setDerPublicKey(inPk); | ||
|
|
||
| getOSSLKey(); | ||
| if (EVP_PKEY_get0_type_name(pkey) == NULL) | ||
| { ERROR_MSG("Could not determine algorithm name from EVP_PKEY"); return; } | ||
|
|
||
| name = EVP_PKEY_get0_type_name(pkey); | ||
|
|
||
| if (pkey) | ||
| { EVP_PKEY_free(pkey); pkey = NULL; } | ||
| } |
There was a problem hiding this comment.
Dangling pointer: name becomes invalid after freeing pkey.
Lines 116 and 118-119 capture name from EVP_PKEY_get0_type_name(pkey) and then immediately free the pkey. Since EVP_PKEY_get0_type_name returns a pointer to OpenSSL's internal string within the EVP_PKEY structure, freeing the key invalidates the pointer. Subsequent calls to getOrderLength() (line 73) will dereference freed memory, causing undefined behavior and potential crashes.
The correct approach is to keep pkey alive after setting the DER key. Before updating, free any existing pkey, then create and retain the new one:
void OSSLSLHPublicKey::setDerPublicKey(const ByteString& inPk)
{
+ if (pkey != NULL)
+ {
+ EVP_PKEY_free(pkey);
+ pkey = NULL;
+ name = NULL;
+ }
+
SLHPublicKey::setDerPublicKey(inPk);
- getOSSLKey();
- if (EVP_PKEY_get0_type_name(pkey) == NULL)
- { ERROR_MSG("Could not determine algorithm name from EVP_PKEY"); return; }
-
- name = EVP_PKEY_get0_type_name(pkey);
-
- if (pkey)
- { EVP_PKEY_free(pkey); pkey = NULL; }
+ if (derPublicKey.size() == 0) return;
+
+ pkey = getOSSLKey();
+ if (pkey == NULL)
+ {
+ ERROR_MSG("Could not materialize EVP_PKEY from DER");
+ return;
+ }
+
+ name = EVP_PKEY_get0_type_name(pkey);
+ if (name == NULL)
+ {
+ ERROR_MSG("Could not determine algorithm name from EVP_PKEY");
+ }
}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/lib/crypto/OSSLSLHPrivateKey.h (2)
38-38: Consider removing the unused openssl/bn.h include.The header and implementation don't use BIGNUM types or BN_* functions. This include appears unnecessary and was likely copied from another key type.
Apply this diff to remove the unused include:
#include "config.h" #include "SLHPrivateKey.h" -#include <openssl/bn.h> #include <openssl/evp.h>
50-62: Consider addingoverridekeyword to overridden virtual methods.Methods
isOfType,getOrderLength, andsetDerPrivateKeyoverride base class virtuals. Adding theoverridekeyword improves clarity and catches signature mismatches at compile time.Apply this diff:
// Destructor - virtual ~OSSLSLHPrivateKey(); + virtual ~OSSLSLHPrivateKey() override; // The type static const char* type; // Check if the key is of the given type - virtual bool isOfType(const char* inType); + virtual bool isOfType(const char* inType) override; // Get the base point order length - virtual unsigned long getOrderLength() const; + virtual unsigned long getOrderLength() const override; // Setters for the SLHDSA private key components - virtual void setDerPrivateKey(const ByteString& inSk); + virtual void setDerPrivateKey(const ByteString& inSk) override;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/lib/crypto/OSSLSLHPrivateKey.h(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 815
File: src/lib/SoftHSM.cpp:6543-6546
Timestamp: 2025-09-11T16:54:00.370Z
Learning: SoftHSM PR #815 (pkcs11.h 3.1.0 update, CK_RSA_AES_KEY_WRAP_PARAMS fix): Maintainer bukka prefers to keep scope minimal (header sync and param rename only); broader OAEP parameter enforcement or backend plumbing is out of scope for this PR and should be handled in a follow-up issue.
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:5550-5596
Timestamp: 2025-10-27T16:44:57.865Z
Learning: SoftHSM ML‑DSA additional context: avoid transient malloc in SoftHSM::{AsymSignInit, AsymVerifyInit}; let Session::setParameters deep‑copy SIGN_ADDITIONAL_CONTEXT by allocating one buffer for struct + context bytes and fixing the stored pointer; Session::resetOp frees m_pParameters so both struct and context are released.
Learnt from: JoaoHenrique12
Repo: softhsm/SoftHSMv2 PR: 823
File: src/lib/SoftHSM.cpp:58-58
Timestamp: 2025-11-11T19:42:46.859Z
Learning: SoftHSM include policy: Algorithm key/parameter headers (e.g., SLHParameters, SLH*Key) are included unconditionally in SoftHSM.cpp/SoftHSM.h; feature gating is handled in mechanism tables and CryptoFactory (WITH_*). Avoid wrapping such includes in WITH_* guards.
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:4193-4196
Timestamp: 2025-08-19T09:23:25.434Z
Learning: MLDSA parameter set inference: In src/lib/crypto/MLDSAPrivateKey.{h,cpp} and MLDSAPublicKey.{h,cpp}, getParameterSet() maps key material length to CKP_ML_DSA_44/65/87, and there is no setParameterSet(). Runtime MLDSA keys used by SoftHSM (e.g., in C_SignInit/C_VerifyInit via MLDSAUtil) rely on this inference; no explicit parameter-set assignment is required.
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: m4/acx_openssl_mldsa.m4:10-35
Timestamp: 2025-08-18T17:54:37.645Z
Learning: For OpenSSL feature detection in autoconf macros, prefer functional tests (like EVP_PKEY_CTX_new_from_name) over version number checks, as the functional approach directly tests the actual capability and is more reliable across different OpenSSL builds where features may be backported or disabled.
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:10044-10055
Timestamp: 2025-08-18T19:46:46.659Z
Learning: In SoftHSM::generateMLDSA (src/lib/SoftHSM.cpp), the project prefers using CKP_ML_DSA_* constants for parameter set checks and logs "Unsupported parameter set: %lu". The code currently returns CKR_PARAMETER_SET_NOT_SUPPORTED for unsupported sets.
📚 Learning: 2025-11-11T19:42:46.859Z
Learnt from: JoaoHenrique12
Repo: softhsm/SoftHSMv2 PR: 823
File: src/lib/SoftHSM.cpp:58-58
Timestamp: 2025-11-11T19:42:46.859Z
Learning: SoftHSM include policy: Algorithm key/parameter headers (e.g., SLHParameters, SLH*Key) are included unconditionally in SoftHSM.cpp/SoftHSM.h; feature gating is handled in mechanism tables and CryptoFactory (WITH_*). Avoid wrapping such includes in WITH_* guards.
Applied to files:
src/lib/crypto/OSSLSLHPrivateKey.h
📚 Learning: 2025-09-11T16:54:00.370Z
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 815
File: src/lib/SoftHSM.cpp:6543-6546
Timestamp: 2025-09-11T16:54:00.370Z
Learning: SoftHSM PR #815 (pkcs11.h 3.1.0 update, CK_RSA_AES_KEY_WRAP_PARAMS fix): Maintainer bukka prefers to keep scope minimal (header sync and param rename only); broader OAEP parameter enforcement or backend plumbing is out of scope for this PR and should be handled in a follow-up issue.
Applied to files:
src/lib/crypto/OSSLSLHPrivateKey.h
🧬 Code graph analysis (1)
src/lib/crypto/OSSLSLHPrivateKey.h (2)
src/lib/crypto/OSSLSLHPrivateKey.cpp (19)
OSSLSLHPrivateKey(44-48)OSSLSLHPrivateKey(50-56)OSSLSLHPrivateKey(59-66)isOfType(110-113)isOfType(110-110)getOrderLength(71-74)getOrderLength(71-71)setDerPrivateKey(115-127)setDerPrivateKey(115-115)PKCS8Encode(130-148)PKCS8Encode(130-130)PKCS8Decode(151-164)PKCS8Decode(151-151)setFromOSSL(77-107)setFromOSSL(77-77)getOSSLKey(167-172)getOSSLKey(167-167)createOSSLKey(175-193)createOSSLKey(175-175)src/lib/crypto/SLHPrivateKey.h (1)
SLHPrivateKey(39-72)
🪛 Clang (14.0.6)
src/lib/crypto/OSSLSLHPrivateKey.h
[error] 36-36: 'config.h' file not found
(clang-diagnostic-error)
| class OSSLSLHPrivateKey : public SLHPrivateKey | ||
| { | ||
| public: | ||
| // Constructors | ||
| OSSLSLHPrivateKey(); | ||
|
|
||
| OSSLSLHPrivateKey(const EVP_PKEY* inPKEY); | ||
|
|
||
| // Destructor | ||
| virtual ~OSSLSLHPrivateKey(); | ||
|
|
||
| // The type | ||
| static const char* type; | ||
|
|
||
| // Check if the key is of the given type | ||
| virtual bool isOfType(const char* inType); | ||
|
|
||
| // Get the base point order length | ||
| virtual unsigned long getOrderLength() const; | ||
|
|
||
| // Setters for the SLHDSA private key components | ||
| virtual void setDerPrivateKey(const ByteString& inSk); | ||
|
|
||
| // Encode into PKCS#8 DER | ||
| virtual ByteString PKCS8Encode(); | ||
|
|
||
| // Decode from PKCS#8 BER | ||
| virtual bool PKCS8Decode(const ByteString& ber); | ||
|
|
||
| // Set from OpenSSL representation | ||
| virtual void setFromOSSL(const EVP_PKEY* inPKEY); | ||
|
|
||
| // Retrieve the OpenSSL representation of the key | ||
| EVP_PKEY* getOSSLKey(); | ||
|
|
||
| private: | ||
| // The internal OpenSSL representation | ||
| const char* name; | ||
| EVP_PKEY* pkey; | ||
|
|
||
| // Create the OpenSSL representation of the key | ||
| void createOSSLKey(); | ||
| }; |
There was a problem hiding this comment.
Implement or delete copy and move operations to prevent double‑free.
The class manages an EVP_PKEY* resource with a destructor that calls EVP_PKEY_free, but lacks copy constructor, copy assignment, move constructor, and move assignment operators. If an instance is copied, both the original and the copy will attempt to free the same pointer, causing a double‑free.
Apply this diff to explicitly delete copy and move operations:
public:
// Constructors
OSSLSLHPrivateKey();
OSSLSLHPrivateKey(const EVP_PKEY* inPKEY);
+ // Prevent copying and moving
+ OSSLSLHPrivateKey(const OSSLSLHPrivateKey&) = delete;
+ OSSLSLHPrivateKey& operator=(const OSSLSLHPrivateKey&) = delete;
+ OSSLSLHPrivateKey(OSSLSLHPrivateKey&&) = delete;
+ OSSLSLHPrivateKey& operator=(OSSLSLHPrivateKey&&) = delete;
+
// Destructor
virtual ~OSSLSLHPrivateKey();🤖 Prompt for AI Agents
In src/lib/crypto/OSSLSLHPrivateKey.h around lines 41 to 83, the class owns an
EVP_PKEY* and lacks copy/move special member handling which can cause
double-free; explicitly delete the copy constructor, copy assignment, move
constructor, and move assignment operators to prevent accidental copying or
moving, e.g. declare OSSLSLHPrivateKey(const OSSLSLHPrivateKey&) = delete;
OSSLSLHPrivateKey& operator=(const OSSLSLHPrivateKey&) = delete;
OSSLSLHPrivateKey(OSSLSLHPrivateKey&&) = delete; OSSLSLHPrivateKey&
operator=(OSSLSLHPrivateKey&&) = delete; so ownership cannot be duplicated.
| const char* name; | ||
| EVP_PKEY* pkey; |
There was a problem hiding this comment.
Critical lifetime bug: name becomes a dangling pointer in setDerPrivateKey.
The name member stores a borrowed pointer from EVP_PKEY_get0_type_name(pkey), whose lifetime is tied to pkey. However, in OSSLSLHPrivateKey::setDerPrivateKey (OSSLSLHPrivateKey.cpp:114-126), the code stores name from pkey and then immediately frees pkey, leaving name dangling. Subsequent calls to getOrderLength() (which reads name) will access freed memory.
In OSSLSLHPrivateKey.cpp, apply this diff to fix setDerPrivateKey:
void OSSLSLHPrivateKey::setDerPrivateKey(const ByteString& inSk)
{
SLHPrivateKey::setDerPrivateKey(inSk);
+ if (pkey)
+ {
+ EVP_PKEY_free(pkey);
+ pkey = NULL;
+ name = NULL;
+ }
+
getOSSLKey();
- if (EVP_PKEY_get0_type_name(pkey) == NULL)
- { ERROR_MSG("Could not determine algorithm name from EVP_PKEY"); return; }
-
- name = EVP_PKEY_get0_type_name(pkey);
-
- if (pkey)
- { EVP_PKEY_free(pkey); pkey = NULL; }
+ if (EVP_PKEY_get0_type_name(pkey) == NULL)
+ {
+ ERROR_MSG("Could not determine algorithm name from EVP_PKEY");
+ return;
+ }
+ name = EVP_PKEY_get0_type_name(pkey);
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/lib/crypto/OSSLSLHPrivateKey.h around lines 78-79, the name member is
currently a borrowed const char* that becomes a dangling pointer when
setDerPrivateKey frees its EVP_PKEY; update setDerPrivateKey to copy the type
name into owned memory (e.g., strdup or std::string) before freeing or replacing
the EVP_PKEY, free or release any previous owned name to avoid leaks, and adjust
the class (or destructor) to free the owned name on destruction; ensure
getOrderLength reads from the owned copy rather than the freed pointer.
bcfd6c1 to
3db0676
Compare
2 participants
SLH-DSA
Intro
Hello everyone, this MR contains the implementation of SLH-DSA (which is an exclusive post-quantum digital signature algorithm). Currently, this algorithm is available starting from OpenSSL 3.5.0, so there is no possibility of using it with the botan library. Furthermore, I was inspired by EDDSA to make this implementation.
To validate the implementation, I used hpcs-pkcs11 as a base, as I had to define
'vendor defined' constants for the key pair generation, signature, and validation processes.
How to validate it
To validate the project, check out commit ed71271, which has a folder with hpcs-pkcs11, a Dockerfile that configures OpenSSL to version 3.5.1, and some logs I used in the coding process.
Build and run the Dockerfile with:
If you want to activate the logs, execute the command:
syslogd -n -O /var/log/syslog &Create the file containing the text to be signed:
Compile the pkcs11 client, and use it to generate the keys, sign, and validate the message.
By default, the SLH-DSA version used by pkcs11-slhdsa.c is SLH-DSA-SHA2-128s, but it is possible to change the file at the following points to change the algorithm used:
To validate a test that fails, just replace the line 318
Implementation Limitations
Among the limitations of this MR, the following stand out:
PK = (PK.seed, PK.root),SK = (SK.seed, SK.prf, PK)Conclusions
Finally, if there is a possibility for this MR to become part of the main project, I would be very happy to work on making the necessary adjustments so that it is fit to enter the main branch.
Summary by CodeRabbit
New Features
Tests