Added ML-DSA support #809
Added ML-DSA support #809
Conversation
📝 WalkthroughWalkthroughAdds ML‑DSA support across build systems and Autotools detection, new ML‑DSA crypto types and OpenSSL backend, PKCS#11 attributes/objects and SoftHSM routing, tests, build list updates, and a CI job for OpenSSL 3.5.4. (No public API removals.) Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SoftHSM
participant CryptoFactory
participant OSSLMLDSA
participant OpenSSL
Client->>SoftHSM: C_GenerateKeyPair(CKM_ML_DSA_KEY_PAIR_GEN, params)
SoftHSM->>SoftHSM: generateMLDSA(...)
SoftHSM->>CryptoFactory: getAsymmetricAlgorithm(AsymAlgo::MLDSA)
CryptoFactory->>OSSLMLDSA: instantiate OSSLMLDSA
SoftHSM->>OSSLMLDSA: generateKeyPair(parameters)
OSSLMLDSA->>OpenSSL: EVP_PKEY_CTX_new_from_name("ML-DSA-xx") / keygen
OpenSSL-->>OSSLMLDSA: EVP_PKEY (key pair)
OSSLMLDSA-->>SoftHSM: MLDSA key pair wrapper
SoftHSM->>SoftHSM: create P11 MLDSA objects (CKA_PARAMETER_SET, CKA_VALUE, CKA_SEED)
SoftHSM-->>Client: return object handles
sequenceDiagram
participant Client
participant SoftHSM
participant Token
participant OSSLMLDSA
participant OpenSSL
Client->>SoftHSM: C_SignInit(hKey, CKM_ML_DSA, pParameter)
SoftHSM->>SoftHSM: copyMLDSAContext / AsymSignInit
SoftHSM->>Token: retrieve/decrypt CKA_VALUE, CKA_SEED (if private)
Token-->>SoftHSM: key bytes
SoftHSM->>OSSLMLDSA: sign(privateKey, data, SIGN_ADDITIONAL_CONTEXT)
OSSLMLDSA->>OpenSSL: EVP_PKEY_sign (with hedge/deterministic params)
OpenSSL-->>OSSLMLDSA: signature bytes
OSSLMLDSA-->>SoftHSM: signature
SoftHSM-->>Client: return signature
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
kalvdans
left a comment
kalvdans
left a comment
There was a problem hiding this comment.
The code is marked with "Copyright (c) 2010 SURFnet bv". Since you wrote it yourself, you own the copyright to the code, so remove the incorrect copyright statement.
More comments when you undraft :)
|
Copyright banners on new code have been removed. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 34
🔭 Outside diff range comments (3)
src/lib/SoftHSM.cpp (3)
6731-6736: Wrap/unwarp mechanism parity + DES3 key type check is always true
- Parity: C_WrapKey allows AES_CBC/_PAD but C_UnwrapKey doesn’t allow AES_CBC at all. Either remove AES_CBC wrapping or add AES_CBC unwrapping. Same for DES3_CBC: WrapKeySym supports it but C_WrapKey rejects it due to a logic bug (see below) and C_UnwrapKey lacks a corresponding case.
- Logic bug: The DES3 key-type check uses “!= DES2 || != DES3” which is tautologically true; it should be “&&”.
case CKM_AES_CBC: case CKM_AES_CBC_PAD: if (pMechanism->pParameter == NULL_PTR || pMechanism->ulParameterLen != 16) return CKR_ARGUMENTS_BAD; break; + case CKM_DES3_CBC: + case CKM_DES3_CBC_PAD: + if (pMechanism->pParameter == NULL_PTR || + pMechanism->ulParameterLen != 8) + return CKR_ARGUMENTS_BAD; + break; @@ - if (pMechanism->mechanism == CKM_DES3_CBC && (wrapKey->getUnsignedLongValue(CKA_KEY_TYPE, CKK_VENDOR_DEFINED) != CKK_DES2 || - wrapKey->getUnsignedLongValue(CKA_KEY_TYPE, CKK_VENDOR_DEFINED) != CKK_DES3)) + if (pMechanism->mechanism == CKM_DES3_CBC && + (wrapKey->getUnsignedLongValue(CKA_KEY_TYPE, CKK_VENDOR_DEFINED) != CKK_DES2 && + wrapKey->getUnsignedLongValue(CKA_KEY_TYPE, CKK_VENDOR_DEFINED) != CKK_DES3)) return CKR_WRAPPING_KEY_TYPE_INCONSISTENT;Follow-up: see UnwrapKeySym and C_UnwrapKey changes below to restore symmetry.
Also applies to: 6776-6779
7015-7024: Add AES_CBC/DES3_CBC support in UnwrapKeySym to match wrappingCurrently only PAD variants are supported. Either drop non-PAD wrapping or add non-PAD unwrapping. Here is the latter:
case CKM_AES_CBC_PAD: algo = SymAlgo::AES; blocksize = 16; break; + case CKM_AES_CBC: + algo = SymAlgo::AES; + blocksize = 16; + break; @@ case CKM_DES3_CBC_PAD: algo = SymAlgo::DES3; blocksize = 8; break; + case CKM_DES3_CBC: + algo = SymAlgo::DES3; + blocksize = 8; + break; @@ - case CKM_AES_CBC_PAD: - case CKM_DES3_CBC_PAD: + case CKM_AES_CBC_PAD: + case CKM_DES3_CBC_PAD: { iv.resize(blocksize); memcpy(&iv[0], pMechanism->pParameter, blocksize); @@ keydata += decryptedFinal; if(!RFC5652Unpad(keydata,blocksize)) { cipher->recycleKey(unwrappingkey); CryptoFactory::i()->recycleSymmetricAlgorithm(cipher); return CKR_GENERAL_ERROR; // TODO should be another error } break; + } + case CKM_AES_CBC: + case CKM_DES3_CBC: + { + iv.resize(blocksize); + memcpy(&iv[0], pMechanism->pParameter, blocksize); + + if (!cipher->decryptInit(unwrappingkey, SymMode::CBC, iv, false)) + { + cipher->recycleKey(unwrappingkey); + CryptoFactory::i()->recycleSymmetricAlgorithm(cipher); + return CKR_MECHANISM_INVALID; + } + if (!cipher->decryptUpdate(wrapped, keydata)) + { + cipher->recycleKey(unwrappingkey); + CryptoFactory::i()->recycleSymmetricAlgorithm(cipher); + return CKR_GENERAL_ERROR; + } + // Finalize decryption + if (!cipher->decryptFinal(decryptedFinal)) + { + cipher->recycleKey(unwrappingkey); + CryptoFactory::i()->recycleSymmetricAlgorithm(cipher); + return CKR_GENERAL_ERROR; + } + keydata += decryptedFinal; + break; }Also applies to: 7051-7083
7361-7374: C_UnwrapKey parity with wrapping + fix DES3 key-type checkAllow non-PAD AES_CBC/DES3_CBC with proper IV length checks and fix the DES3 key-type check.
case CKM_RSA_AES_KEY_WRAP: rv = MechParamCheckRSAAESKEYWRAP(pMechanism); if (rv != CKR_OK) return rv; break; case CKM_AES_CBC: + if (pMechanism->pParameter == NULL_PTR || + pMechanism->ulParameterLen != 16) + return CKR_ARGUMENTS_BAD; + break; + case CKM_DES3_CBC: + if (pMechanism->pParameter == NULL_PTR || + pMechanism->ulParameterLen != 8) + return CKR_ARGUMENTS_BAD; + break; case CKM_AES_CBC_PAD: // TODO check block length if (pMechanism->pParameter == NULL_PTR || pMechanism->ulParameterLen != 16) return CKR_ARGUMENTS_BAD; break; @@ - if (pMechanism->mechanism == CKM_DES3_CBC && (unwrapKey->getUnsignedLongValue(CKA_KEY_TYPE, CKK_VENDOR_DEFINED) != CKK_DES2 || - unwrapKey->getUnsignedLongValue(CKA_KEY_TYPE, CKK_VENDOR_DEFINED) != CKK_DES3)) + if (pMechanism->mechanism == CKM_DES3_CBC && + (unwrapKey->getUnsignedLongValue(CKA_KEY_TYPE, CKK_VENDOR_DEFINED) != CKK_DES2 && + unwrapKey->getUnsignedLongValue(CKA_KEY_TYPE, CKK_VENDOR_DEFINED) != CKK_DES3)) return CKR_WRAPPING_KEY_TYPE_INCONSISTENT;Also applies to: 7415-7417
♻️ Duplicate comments (1)
src/lib/SoftHSM.cpp (1)
13004-13005: Re: make helper methods static or move them out of the class (still applicable)The previous remark about marking these helpers static or moving them out applies here as well; the implicit ‘this’ is not used.
If you want, I can prepare a follow-up patch to make all get/set*Key helpers static free functions for consistency.
🧹 Nitpick comments (60)
src/lib/crypto/test/CMakeLists.txt (1)
25-25: Guard MLDSATests.cpp behind ENABLE_MLDSAML-DSA tests are currently always compiled, even when the feature is disabled; wrapping them in the existing
ENABLE_MLDSAoption prevents needless builds and keeps backend-only configurations working.• File:
src/lib/crypto/test/CMakeLists.txt(around line 25)Apply this diff to remove the unconditional entry and append it only when
ENABLE_MLDSAis on:--- a/src/lib/crypto/test/CMakeLists.txt +++ b/src/lib/crypto/test/CMakeLists.txt @@ –22,7 +22,10 @@ set(SOURCES PKCS11ModuleTests.cpp PKCS11SessionTests.cpp PKCS11ObjectTests.cpp - MLDSATests.cpp ) +# Only build ML-DSA tests when the feature is enabled +if(ENABLE_MLDSA) + list(APPEND SOURCES MLDSATests.cpp) +endif()README.md (2)
85-85: Replace hard tab with spaces (MD010) on the new option linemarkdownlint flagged a hard tab on this newly added line. Use spaces for indentation to satisfy MD010.
Apply this diff:
- --enable-mldsa Enable support for ML-DSA (default detect) + --enable-mldsa Enable support for ML-DSA (default detect)
85-85: Clarify minimum OpenSSL version required for ML-DSASince ML-DSA support is only available with OpenSSL >= 3.5 (per the PR description), it would help users if we document this requirement alongside the flag or in the Dependencies section.
Proposed wording to add under “Dependencies” or as a parenthetical on the option line:
- “ML-DSA requires OpenSSL 3.5 or newer when using the OpenSSL backend.”
src/lib/crypto/CMakeLists.txt (2)
75-78: Gate ML-DSA sources on ENABLE_MLDSA in CMakeTo avoid compiling empty or unsupported ML-DSA wrappers when the feature is disabled (or OpenSSL lacks ML-DSA), wrap the four OSSLMLDSA source files in the existing
WITH_OPENSSLblock under anENABLE_MLDSAguard, and defineWITH_ML_DSAwhen enabled.• File:
src/lib/crypto/CMakeLists.txt(around lines 75–78)if(WITH_OPENSSL) list(APPEND SOURCES OSSLCryptoFactory.cpp OSSLEDDSA.cpp - OSSLMLDSA.cpp - OSSLMLDSAKeyPair.cpp - OSSLMLDSAPrivateKey.cpp - OSSLMLDSAPublicKey.cpp ) + + if(ENABLE_MLDSA) + list(APPEND SOURCES + OSSLMLDSA.cpp + OSSLMLDSAKeyPair.cpp + OSSLMLDSAPrivateKey.cpp + OSSLMLDSAPublicKey.cpp + ) + target_compile_definitions(${PROJECT_NAME} PRIVATE WITH_ML_DSA) + endif() endif()• This matches the existing
#ifdef WITH_ML_DSAusage throughout the code (e.g., inOSSLMLDSA*.cppandSoftHSM.cpp), ensuring ML-DSA files are only built—and the macro only defined—when the feature is explicitly enabled.
31-33: Only compile ML-DSA sources when ENABLE_MLDSA is ONCurrently, the three ML-DSA files are unconditionally appended to the SOURCES list in
src/lib/crypto/CMakeLists.txt, which defeats the purpose of theENABLE_MLDSAoption defined at the project root (CMakeLists.txt:11). Wrapping them in anif(ENABLE_MLDSA)both avoids unnecessary compile costs and keeps the build graph consistent with other algorithm flags.Location
• src/lib/crypto/CMakeLists.txt: lines 31–33Suggested diff
- MLDSAParameters.cpp - MLDSAPrivateKey.cpp - MLDSAPublicKey.cppInsert after the main
list(APPEND SOURCES …)block:# ML-DSA core sources if(ENABLE_MLDSA) list(APPEND SOURCES MLDSAParameters.cpp MLDSAPrivateKey.cpp MLDSAPublicKey.cpp ) endif()Verify that
ENABLE_MLDSAis declared (found in root CMakeLists.txt at line 11)..github/workflows/ci.yml (3)
90-101: Make OpenSSL 3.5.2 discoverable at runtime; avoid brittle symlink hacks.Relying on a lib64→lib symlink is fragile. Prefer registering the library path with ldconfig, or export LD_LIBRARY_PATH for the build/test steps.
For a robust setup, append after “sudo make install”:
sudo make install > install.log - cd ${{ env.OPENSSL_INSTALL_DIR }} - sudo ln -s lib64 lib + echo "${{ env.OPENSSL_INSTALL_DIR }}/lib64" | sudo tee /etc/ld.so.conf.d/openssl-3.5.conf + sudo ldconfigAlternatively (if avoiding system config changes), export LD_LIBRARY_PATH in Build/Test steps:
- name: Build env: # Once all OpenSSL deprecations fixed, uncomment this # CXXFLAGS: -Werror OPENSSL_INSTALL_DIR: /usr/local/openssl-3.5 - LDFLAGS: "-Wl,-R/usr/local/openssl-3.5/lib64 -L/usr/local/openssl-3.5/lib64" + LDFLAGS: "-Wl,-rpath,/usr/local/openssl-3.5/lib64 -L/usr/local/openssl-3.5/lib64" + LD_LIBRARY_PATH: "/usr/local/openssl-3.5/lib64:${LD_LIBRARY_PATH}"
97-99: Surface build/install logs on failure for faster debugging.Currently build/install logs are redirected to files and lost on failure.
Emit the tail of the logs on failure:
- make -j$(nproc) > build.log - sudo make install > install.log + set -e + make -j"$(nproc)" > build.log || { echo '--- build.log (tail) ---'; tail -n +1 build.log; exit 1; } + sudo make install > install.log || { echo '--- install.log (tail) ---'; tail -n +1 install.log; exit 1; }
91-91: Trim unnecessary packages to speed up CI.checkinstall and sudo are not needed on GitHub-hosted Ubuntu runners for this build. Consider dropping them.
- sudo apt-get install -y libcppunit-dev p11-kit build-essential checkinstall zlib1g-dev sudo autoconf libtool git + sudo apt-get install -y libcppunit-dev p11-kit zlib1g-dev autoconf libtool gitsrc/lib/pkcs11/pkcs11.h (1)
1706-1751: PQ context structs and parameter-set enums: align comment and ensure ABI expectations.
- Minor: Comments say “CKA_PARAMETER_SETS” while the attribute is singular CKA_PARAMETER_SET.
- Struct field naming matches PKCS #11 style. Ensure no packing/alignment directives are needed beyond the existing header guarantees.
Apply this cosmetic comment fix:
-/* ML-DSA values for CKA_PARAMETER_SETS */ +/* ML-DSA values for CKA_PARAMETER_SET */ ... -/* SLH-DSA values for CKA_PARAMETER_SETS */ +/* SLH-DSA values for CKA_PARAMETER_SET */ ... -/* ML-KEM values for CKA_PARAMETER_SETS */ +/* ML-KEM values for CKA_PARAMETER_SET */src/bin/dump/tables.h (2)
483-485: Add mappings for ML-DSA mechanisms: good start; consider extending to hash variants.CKM_ML_DSA[_KEY_PAIR_GEN] are mapped. For completeness, you may also add the CKM_HASH_ML_DSA* entries so dumps display those by name when encountered.
I can prepare a patch adding the CKM_HASH_ML_DSA* identifiers if you want to include them now.
551-551: CKK_ML_DSA mapping is fine; optionally add CKK_ML_KEM while you're here.Even if KEM isn’t enabled yet, mapping the constant improves dump readability.
For example:
t[CKK_EC_EDWARDS] = "CKK_EC_EDWARDS"; + t[CKK_ML_KEM] = "CKK_ML_KEM"; t[CKK_ML_DSA] = "CKK_ML_DSA";CMAKE-WIN-NOTES.md (1)
55-55: Replace hard tab with spaces to satisfy markdownlint (MD010).The linter flags a hard tab on this line.
- -DENABLE_MLDSA=ON Enable support for ML-DSA + -DENABLE_MLDSA=ON Enable support for ML-DSAsrc/lib/crypto/test/Makefile.am (1)
27-27: Ensure MLDSATests.cpp is gated when ML-DSA is disabled/backends lack support.If MLDSATests.cpp compiles conditionally with #if defined(WITH_ML_DSA), you’re fine. If not, the OpenSSL 1.1.1/Botan jobs may fail to build tests.
Option A (preferred, if configure.ac defines AM_CONDITIONAL WITH_ML_DSA):
- MLDSATests.cpp \ +if WITH_ML_DSA + MLDSATests.cpp \ +endifOption B: Keep file listed but guard its contents with preprocessor in MLDSATests.cpp:
#if defined(WITH_ML_DSA) // existing tests #else // compile an empty test suite or nothing #endifWould you like me to prepare the corresponding configure.ac change to define WITH_ML_DSA for Automake?
src/lib/crypto/Makefile.am (1)
31-33: Gate ML-DSA sources behind WITH_ML_DSAThe Autotools setup already provides
AC_ARG_ENABLE(mldsa)andAM_CONDITIONAL([WITH_ML_DSA], …), so you can mirror the CMakeENABLE_MLDSAflag and avoid unconditionally compiling the ML-DSA .cpp files.Locations to update:
- src/lib/crypto/Makefile.am (around lines 31–33)
Suggested diff:
--- a/src/lib/crypto/Makefile.am +++ b/src/lib/crypto/Makefile.am @@ -28,10 +28,6 @@ libsofthsm_crypto_la_SOURCES = \ <other sources> \ - MLDSAParameters.cpp \ - MLDSAPrivateKey.cpp \ - MLDSAPublicKey.cpp - -# (previous unconditional inclusion) +# ML-DSA core sources are added only when enabled if WITH_ML_DSA libsofthsm_crypto_la_SOURCES += \ MLDSAParameters.cpp \ MLDSAPrivateKey.cpp \ MLDSAPublicKey.cpp endifCMAKE-NOTES.md (1)
14-14: Fix markdownlint: replace hard tab with spaces; optionally note OpenSSL ≥ 3.5 for ML-DSA.The linter flags a hard tab on this newly added line. Replace with spaces. Consider documenting the OpenSSL ≥ 3.5 requirement for ML-DSA builds.
Apply this diff:
- -DENABLE_MLDSA=ON Enable support for ML-DSA + -DENABLE_MLDSA=ON Enable support for ML-DSAOptionally append a note under Configure:
Note: ML-DSA requires OpenSSL 3.5 or newer when building with the OpenSSL backend.src/lib/crypto/OSSLUtil.cpp (2)
42-49: Make the parameter-set mapping const and file-local.The mapping is immutable; marking it const avoids accidental mutation and enables better optimization.
Apply this diff:
-#ifdef WITH_ML_DSA -static std::map<unsigned long, const char*> mldsaAlgNameFromParameterSet { +#ifdef WITH_ML_DSA +static const std::map<unsigned long, const char*> mldsaAlgNameFromParameterSet{ {MLDSAParameters::ML_DSA_44_PARAMETER_SET, "ML-DSA-44"}, {MLDSAParameters::ML_DSA_65_PARAMETER_SET, "ML-DSA-65"}, {MLDSAParameters::ML_DSA_87_PARAMETER_SET, "ML-DSA-87"} }; #endif
223-231: Useconst auto(orconst_iterator) for the map lookupNo mutation occurs; tightening the iterator type improves safety if
mldsaAlgNameFromParameterSetever becomesconst.
- Verified that
mldsaParameterSet2Nameis declared under#ifdef WITH_ML_DSAinOSSLUtil.h, matching the definition.Apply this diff:
-const char* OSSL::mldsaParameterSet2Name(unsigned long parameterSet) { - - std::map<unsigned long, const char*>::iterator it = mldsaAlgNameFromParameterSet.find(parameterSet); - - if (it != mldsaAlgNameFromParameterSet.end()) { - return it->second; - } +const char* OSSL::mldsaParameterSet2Name(unsigned long parameterSet) +{ + const auto it = mldsaAlgNameFromParameterSet.find(parameterSet); + if (it != mldsaAlgNameFromParameterSet.end()) + { + return it->second; + } return NULL; }src/lib/crypto/MLDSAPrivateKey.cpp (3)
27-29: Fix misleading comment.The comment says “Get the bit length” but this function returns the parameter set.
Apply this diff:
-// Get the bit length +// Get the ML-DSA parameter set from the private key length
41-42: Tighten wording in comment.Minor nit on phrasing.
Apply this diff:
-// Get the signatureLength length +// Get the signature length
82-97: Avoid shadowing and optionally validate the private key size on deserialisation.
- Local variables ‘seed’ and ‘value’ shadow members; rename for clarity.
- Optionally, reject deserialised keys with unknown private-key length to prevent invalid state early.
Apply this diff:
-bool MLDSAPrivateKey::deserialise(ByteString& serialised) -{ - ByteString seed = ByteString::chainDeserialise(serialised); - ByteString value = ByteString::chainDeserialise(serialised); - - if ((seed.size() == 0) || - (value.size() == 0)) - { - return false; - } - - setSeed(seed); - setValue(value); - - return true; -} +bool MLDSAPrivateKey::deserialise(ByteString& serialised) +{ + ByteString seedBS = ByteString::chainDeserialise(serialised); + ByteString valueBS = ByteString::chainDeserialise(serialised); + + if (seedBS.size() == 0 || valueBS.size() == 0) + { + return false; + } + + // Optional: ensure private key length matches a known ML-DSA parameter set + switch (valueBS.size()) + { + case MLDSAParameters::ML_DSA_44_PRIV_LENGTH: + case MLDSAParameters::ML_DSA_65_PRIV_LENGTH: + case MLDSAParameters::ML_DSA_87_PRIV_LENGTH: + break; + default: + return false; + } + + setSeed(seedBS); + setValue(valueBS); + return true; +}src/lib/crypto/test/MLDSATests.h (3)
7-12: Consider guarding the test with WITH_ML_DSA to avoid build issues when disabled.If the test sources are compiled unconditionally, include guards prevent build failures when ML-DSA is not enabled.
Apply this diff:
+#ifdef WITH_ML_DSA #include <cppunit/extensions/HelperMacros.h> #include "AsymmetricAlgorithm.h" +// Forward declarations if desired: +// class AsymmetricAlgorithm;And add at file end (see separate comment) a matching #endif.
--- `31-34`: **Member layout is fine; prefer forward-declare AsymmetricAlgorithm in headers to reduce coupling.** This avoids pulling extra headers into test compilations. Example: ```diff -#include "AsymmetricAlgorithm.h" +class AsymmetricAlgorithm;--- `36-36`: **Fix incorrect header-guard end comment and optionally close the WITH_ML_DSA.** The comment says EDDSATESTS while this is MLDSATests. Also close the optional WITH_ML_DSA guard. ```diff -#endif // !_SOFTHSM_V2_EDDSATESTS_H +#endif // !_SOFTHSM_V2_MLDSATESTS_H +#ifdef WITH_ML_DSA +#endifIf you adopt the earlier WITH_ML_DSA guard, ensure exactly one #endif closes it here.
</blockquote></details> <details> <summary>src/lib/P11Attributes.h (1)</summary><blockquote> `1151-1163`: **P11AttrParameterSet checks: consider requiring or allowing during GenerateKeyPair.** For key generation, parameter set typically must be provided or derived from templates/params. Using only ck1 (CreateObject) may be too permissive/ambiguous for GenerateKeyPair. Consider including ck3 if you want to enforce it at generation time, or ensure setDefault() derives it reliably. Would you like me to align this with existing parameter attributes (e.g., EC params) and update setDefault()/updateAttr() in P11Attributes.cpp accordingly? </blockquote></details> <details> <summary>m4/acx_crypto_backend.m4 (2)</summary><blockquote> `113-121`: **OpenSSL backend check wires correctly; ensure macro is available in all configure paths.** The flow matches ECC/EDDSA. Make sure m4/acx_openssl_mldsa.m4 is included by aclocal/ACLOCAL_AMFLAGS so the macro is found at autoconf time. If not already, add it to EXTRA_DIST or ensure m4/ is scanned. --- `261-273`: **Consider disallowing ML-DSA in FIPS mode (policy alignment with GOST).** You already block GOST when FIPS=yes. Given ML-DSA is not FIPS-approved, you may want to error when enable_fips=yes and enable_mldsa=yes to avoid misconfigured builds. Example: ```diff AC_MSG_CHECKING(for ML-DSA support) if test "x${enable_mldsa}" = "xyes"; then + if test "x${enable_fips}" = "xyes"; then + AC_MSG_ERROR([ML-DSA is not FIPS approved]) + fi AC_MSG_RESULT(yes) AC_DEFINE_UNQUOTED( [WITH_ML_DSA], [], [Compile with ML-DSA support] )src/lib/crypto/OSSLMLDSAPublicKey.h (3)
15-23: Constructors/destructor are fine; ensure pkey lifetime semantics are documented.No changes needed; destructor frees pkey. If getOSSLKey hands out the internal pointer for use by OpenSSL calls, document that callers must not free it.
Add a short note in the header comment for getOSSLKey().
37-39: getOSSLKey() returns non-const EVP_PKEY; consider const-correctness if mutation isn’t intended.*If callers should not modify pkey, return const EVP_PKEY* (and add a non-const overload only where needed).
-EVP_PKEY* getOSSLKey(); +const EVP_PKEY* getOSSLKey() const;And adjust the .cpp accordingly.
48-48: Fix the trailing header-guard comment.The guard is _SOFTHSM_V2_OSSLMLDSAPUBLICKEY_H but the comment says OSSLDSAPUBLICKEY.
-#endif // !_SOFTHSM_V2_OSSLDSAPUBLICKEY_H +#endif // !_SOFTHSM_V2_OSSLMLDSAPUBLICKEY_Hsrc/lib/crypto/test/MLDSATests.cpp (3)
1-6: Fix copy/paste header: EDDSA → MLDSAThe file banner still references EDDSA and EDDSATests.cpp. It should reflect MLDSA to avoid confusion.
Apply:
/***************************************************************************** - EDDSATests.cpp + MLDSATests.cpp - Contains test cases to test the EDDSA class + Contains test cases to test the MLDSA class *****************************************************************************/
33-35: Comment nit: “EDDSA object” → “MLDSA object”Align the inline comment with the class under test.
- // Check the EDDSA object + // Check the MLDSA object
164-196: Signing/verification test is effective; consider adding a negative checkEnd-to-end sign/verify with random data is covered. Consider an additional negative test (e.g., verify with a different public key or modified data) to ensure failure paths behave correctly.
src/lib/crypto/MLDSAParameters.cpp (3)
28-31: Minor: areOfType could be const (if interface allows)If AsymmetricParameters::areOfType permits const, marking this method const improves const-correctness. Skip if the base interface is non-const.
39-50: Optional validation: reject unsupported parameter sets on deserialiseCurrently any unsigned long is accepted. If only {1,2,3} are supported, reject others early to avoid propagating invalid state.
bool MLDSAParameters::deserialise(ByteString& serialised) { - if (serialised.size() == 0) + if (serialised.size() == 0) { return false; } - setParameterSet(serialised.long_val()); + const unsigned long ps = serialised.long_val(); + switch (ps) + { + case 1UL: + case 2UL: + case 3UL: + break; + default: + return false; + } + setParameterSet(ps); return true; }
7-11: Nit: unused includelog.h appears unused in this TU. Consider dropping it to keep includes minimal.
src/lib/crypto/OSSLMLDSAKeyPair.h (1)
24-31: Optional: mark overrides explicitlyAdding override clarifies intent and helps catch signature mismatches.
- virtual PublicKey* getPublicKey(); - virtual const PublicKey* getConstPublicKey() const; + virtual PublicKey* getPublicKey() override; + virtual const PublicKey* getConstPublicKey() const override; @@ - virtual PrivateKey* getPrivateKey(); - virtual const PrivateKey* getConstPrivateKey() const; + virtual PrivateKey* getPrivateKey() override; + virtual const PrivateKey* getConstPrivateKey() const override;src/lib/crypto/OSSLMLDSAKeyPair.cpp (1)
9-11: Remove unused includelog.h is not used here.
Apply this diff:
-#include "log.h" #include "OSSLMLDSAKeyPair.h"src/lib/crypto/OSSLMLDSAPrivateKey.h (3)
13-14: Drop unnecessary header from a public header to reduce coupling<openssl/bn.h> is not used in this header. Remove it to keep the header lean.
Apply this diff:
-#include <openssl/bn.h> #include <openssl/evp.h>
58-59: Fix header guard commentTrailing comment references a different class name.
Apply this diff:
-#endif // !_SOFTHSM_V2_OSSLECPRIVATEKEY_H +#endif // !_SOFTHSM_V2_OSSLMLDSAPRIVATEKEY_H
20-25: Consider defining safe copy semantics (optional)Given this class owns an EVP_PKEY*, the default copy/assignment will shallow-copy the pointer and double-free on destruction. Either delete copy/assignment or implement them to duplicate/reset pkey based on ByteString value. Not strictly required if all call sites avoid copying, but it’s safer.
I can provide a patch to delete copy/assignment or to implement them by rebuilding pkey from getValue().
src/lib/crypto/MLDSAPublicKey.cpp (3)
8-12: Remove unused includelog.h isn’t used in this unit.
Apply this diff:
-#include "log.h" #include "MLDSAParameters.h" #include "MLDSAPublicKey.h"
27-39: Comment mismatchThe block header says “Get the bit length” but this method returns the parameter set. Update the comment for clarity.
Apply this diff:
-// Get the bit length +// Get the parameter set
41-53: Typo in comment“Get the signatureLength length” → “Get the signature length”.
Apply this diff:
-// Get the signatureLength length +// Get the signature lengthsrc/lib/crypto/MLDSAPublicKey.h (1)
22-27: Fix comment labelsThese comment headers are misleading. Adjust to match the methods’ purpose.
Apply this diff:
-// Get the bit length +// Get the parameter set virtual unsigned long getParameterSet() const; -// Get the signature length +// Get the signature length virtual unsigned long getOutputLength() const;src/lib/P11Attributes.cpp (1)
2564-2597: Optional: validate and protect CKA_SEED more strictly
- Consider enforcing expected lengths when applicable (e.g., ML-DSA seeds are typically 32 bytes).
- Ensure P11AttrSeed has ck7 in its checks to block retrieval when sensitive/unextractable, aligning with base retrieve() semantics.
Can you confirm P11AttrSeed’s checks include ck7 in P11Attributes.h so that retrieval is blocked when appropriate?
src/lib/crypto/MLDSAParameters.h (2)
69-69: Initialize parameterSet to a safe defaultAvoid using an uninitialized value if getParameterSet() is called before setParameterSet(). Initialize to 0.
- unsigned long parameterSet; + unsigned long parameterSet = 0UL;
10-11: Remove unused includesand are not used in this header; dropping them reduces compile time and dependencies.
-#include <tuple> -#include <stdexcept>src/lib/crypto/MLDSAPrivateKey.h (1)
15-21: Add a virtual destructorPrivateKey already has a virtual destructor, but explicitly providing one here improves clarity and helps prevent potential slicing/warnings when extending further.
public: // The type static const char* type; + // Destructor + virtual ~MLDSAPrivateKey() { } + // Check if the key is of the given type virtual bool isOfType(const char* inType);src/lib/crypto/OSSLMLDSAPrivateKey.cpp (3)
151-167: Guard against unsupported/unknown parameter set namesIf mldsaParameterSet2Name() returns NULL (e.g., unexpected key length), creating the context will fail. Add an explicit check with a clear error.
const char* name = OSSL::mldsaParameterSet2Name(getParameterSet()); int selection = 0; EVP_PKEY_CTX *ctx = NULL; OSSL_PARAM params[3], *p = params; + if (name == NULL) { + ERROR_MSG("Unsupported ML-DSA parameter set (value: %lu)", getParameterSet()); + return; + }
13-15: Remove unused include and add cleansing support (optional)<openssl/bn.h> isn’t used here. If you choose to cleanse stack buffers, include <openssl/crypto.h>.
-#include <openssl/bn.h> #include <openssl/core_names.h> #include <openssl/x509.h> +#include <openssl/crypto.h>
46-69: Optional: cleanse sensitive stack buffersSeed and private key material temporarily reside on the stack. Cleanse after use to reduce exposure.
if(rv && seed_len == 32) { // seed is not mandatory for OSSL key reconstruction ByteString seedBS = ByteString(seed, seed_len); setSeed(seedBS); } + OPENSSL_cleanse(seed, sizeof(seed)); @@ ByteString valueBS = ByteString(priv, priv_len); setValue(valueBS); + OPENSSL_cleanse(priv, sizeof(priv));src/lib/crypto/OSSLMLDSAPublicKey.cpp (4)
52-54: Fix log message: it’s a public keyThe error message incorrectly mentions “private key”. Make it clear this is extracting the public key.
- ERROR_MSG("Could not get private key private, rv: %d", rv); + ERROR_MSG("Could not get public key data, rv: %d", rv);
95-111: Guard against unknown parameter set nameAdd an explicit check for NULL algorithm name with a clear error log.
const char* name = OSSL::mldsaParameterSet2Name(getParameterSet()); int selection = 0; EVP_PKEY_CTX *ctx = NULL; OSSL_PARAM params[3], *p = params; + if (name == NULL) { + ERROR_MSG("Unsupported ML-DSA parameter set (value: %lu)", getParameterSet()); + return; + }
13-16: Optional: add cleansing support for pubkey bufferIf you prefer to cleanse stack buffers, include <openssl/crypto.h>.
#include <openssl/evp.h> #include <openssl/core_names.h> #include <string.h> +#include <openssl/crypto.h>
57-60: Optional: cleanse temporary pubkey bufferThe public key is not secret in most contexts, but if you want consistent hygiene:
ByteString pubBS = ByteString(localPub, pub_len); setValue(pubBS); - + OPENSSL_cleanse(localPub, sizeof(localPub));src/lib/crypto/OSSLMLDSA.h (1)
10-50: Consider guarding the header with WITH_ML_DSAImplementations are compiled under WITH_ML_DSA; exposing this header unconditionally can lead to accidental references when ML-DSA is disabled. Guarding the declarations as well reduces accidental link/ODR issues.
src/lib/SoftHSM.cpp (1)
13539-13556: Remove unused variable‘parameterSet’ local ByteString is declared but never used in setMLDSAPrivateKey.
- ByteString parameterSet; ByteString seed; ByteString value;src/lib/crypto/OSSLMLDSA.cpp (3)
67-69: Right-size the signature buffer after signingEVP_DigestSign updates the out length. If it’s ever shorter than the pre-sized buffer, the trailing bytes remain zeroed. Resize to the actual produced length.
Apply this diff:
- signature.resize(len); - memset(&signature[0], 0, len); + signature.resize(len); + memset(&signature[0], 0, len); @@ - if (!EVP_DigestSign(ctx, &signature[0], &len, dataToSign.const_byte_str(), dataToSign.size())) + if (!EVP_DigestSign(ctx, &signature[0], &len, dataToSign.const_byte_str(), dataToSign.size())) { ERROR_MSG("ML-DSA sign failed (0x%08X)", ERR_get_error()); EVP_MD_CTX_free(ctx); return false; } + // Trim to the actual signature size (defensive) + signature.resize(len);Also applies to: 77-84
138-141: Fix comment typo and duplicationThe block has duplicate/mismatched comments (“Perform the verify operation” and “Perform the signature operation”). Clean this up for clarity.
Apply this diff:
- // Perform the verify operation - // Perform the signature operation + // Perform the verify operation
73-74: Use correct printf specifier for ERR_get_error() (unsigned long)ERR_get_error() returns unsigned long. Using %08X may truncate on LP64 platforms. Prefer %08lX (or cast to unsigned int with care).
Example fixes:
- ERROR_MSG("ML-DSA sign init failed (0x%08X)", ERR_get_error()); + ERROR_MSG("ML-DSA sign init failed (0x%08lX)", ERR_get_error());Apply similarly to other ERR_get_error() logs in this file.
Also applies to: 79-81, 156-157, 164-166, 171-175, 181-183, 272-279, 284-286
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (2)
src/lib/SoftHSM.cpp (2)
7001-7024: UnwrapKeySym lacks AES_CBC/DES3_CBC (non-PAD) support → cannot unwrap what WrapKeySym can wrapWrapKeySym supports CKM_AES_CBC and CKM_DES3_CBC, but UnwrapKeySym only supports the PAD variants. This asymmetry prevents unwrapping CBC-wrapped blobs (non-PAD).
Apply:
switch(pMechanism->mechanism) { @@ +#ifdef HAVE_AES_KEY_WRAP case CKM_AES_KEY_WRAP: @@ #endif #ifdef HAVE_AES_KEY_WRAP_PAD case CKM_AES_KEY_WRAP_PAD: @@ #endif + case CKM_AES_CBC: + algo = SymAlgo::AES; + blocksize = 16; + break; case CKM_AES_CBC_PAD: algo = SymAlgo::AES; blocksize = 16; break; + case CKM_DES3_CBC: + algo = SymAlgo::DES3; + blocksize = 8; + break; case CKM_DES3_CBC_PAD: algo = SymAlgo::DES3; blocksize = 8; break; @@ - case CKM_AES_CBC_PAD: - case CKM_DES3_CBC_PAD: + case CKM_AES_CBC: + case CKM_AES_CBC_PAD: + case CKM_DES3_CBC: + case CKM_DES3_CBC_PAD: iv.resize(blocksize); memcpy(&iv[0], pMechanism->pParameter, blocksize);Also ensure the C_UnwrapKey mechanism validation accepts CKM_AES_CBC (IV len 16) and CKM_DES3_CBC (IV len 8); see next comment.
Also applies to: 7047-7055
7361-7374: Accept non-PAD CBC in C_UnwrapKey and fix IV length checksCurrently only CKM_AES_CBC_PAD is accepted; add CKM_AES_CBC and CKM_DES3_CBC to keep the unwrap path aligned with WrapKeySym/UnwrapKeySym.
Apply:
- case CKM_AES_CBC_PAD: - // TODO check block length - if (pMechanism->pParameter == NULL_PTR || - pMechanism->ulParameterLen != 16) - return CKR_ARGUMENTS_BAD; - break; + case CKM_AES_CBC: + case CKM_AES_CBC_PAD: + if (pMechanism->pParameter == NULL_PTR || + pMechanism->ulParameterLen != 16) + return CKR_ARGUMENTS_BAD; + break; @@ - case CKM_DES3_CBC_PAD: - // TODO check block length - if (pMechanism->pParameter == NULL_PTR || - pMechanism->ulParameterLen != 8) - return CKR_ARGUMENTS_BAD; - break; + case CKM_DES3_CBC: + case CKM_DES3_CBC_PAD: + if (pMechanism->pParameter == NULL_PTR || + pMechanism->ulParameterLen != 8) + return CKR_ARGUMENTS_BAD; + break;Additionally, in the “Check unwrapping key class and type” section just below (Lines 7413–7417), the error code uses CKR_WRAPPING_KEY_TYPE_INCONSISTENT for unwrap — change to CKR_UNWRAPPING_KEY_TYPE_INCONSISTENT. Same applies to the DES3 clause. Example:
- return CKR_WRAPPING_KEY_TYPE_INCONSISTENT → return CKR_UNWRAPPING_KEY_TYPE_INCONSISTENT
♻️ Duplicate comments (10)
src/lib/crypto/MLDSAPublicKey.h (1)
10-12: Resolved: explicit ByteString dependency includedIncluding ByteString.h directly removes reliance on transitive includes and avoids brittle builds. Good fix.
src/lib/P11Attributes.cpp (1)
2230-2260: CKA_PARAMETER_SET default type and update path look correct
- Default is now CK_ULONG (0) instead of ByteString. Matches the update path and OSAttribute usage.
- Update restricted to GENERATE/CREATE and enforces sizeof(CK_ULONG). Solid.
Optional: consider validating values against the allowed PKCS#11 enumerants (e.g., CKP_ML_DSA_44/65/87), rejecting unknown non-zero sets. This catches template errors early.
CMakeLists.txt (1)
11-11: Wire ENABLE_MLDSA → WITH_ML_DSA and enforce OpenSSL ≥ 3.5The
ENABLE_MLDSAoption is currently inert: it neither defines theWITH_ML_DSAmacro nor checks your OpenSSL version, so all#ifdef WITH_ML_DSAblocks remain disabled. To activate ML-DSA support only when OpenSSL ≥ 3.5:• In CMakeLists.txt (around the existing
option(ENABLE_MLDSA…)on line 11):
- Add a
find_package(OpenSSL REQUIRED)- Gate on
OpenSSL_VERSION_MAJOR/MINORto setWITH_ML_DSAfromENABLE_MLDSAor force it OFF with a warning- Propagate
WITH_ML_DSAviaadd_compile_definitions• In your CMake config-header template (e.g.
config.h.in.cmakeor equivalent):
- Add
#cmakedefine WITH_ML_DSA @WITH_ML_DSA@Suggested patch:
option(ENABLE_EDDSA "Enable support for EDDSA" ON) option(ENABLE_MLDSA "Enable support for ML-DSA" OFF) option(ENABLE_GOST "Enable support for GOST" OFF) +# Locate OpenSSL and enforce version gate for ML-DSA +find_package(OpenSSL REQUIRED) +if(OPENSSL_VERSION_MAJOR GREATER 3 + OR (OPENSSL_VERSION_MAJOR EQUAL 3 AND OPENSSL_VERSION_MINOR GREATER_EQUAL 5)) + set(WITH_ML_DSA ${ENABLE_MLDSA} + CACHE BOOL "Enable ML-DSA support (OpenSSL ≥ 3.5)" FORCE) +else() + set(WITH_ML_DSA OFF + CACHE BOOL "ML-DSA disabled: requires OpenSSL ≥ 3.5" FORCE) + if(ENABLE_MLDSA) + message(WARNING + "ENABLE_MLDSA requested but OpenSSL ${OPENSSL_VERSION} < 3.5; ML-DSA disabled") + endif() +endif() +# Define macro for sources +if(WITH_ML_DSA) + add_compile_definitions(WITH_ML_DSA) +endif()And in your config header template:
#cmakedefine WITH_ML_DSA @WITH_ML_DSA@src/lib/crypto/OSSLUtil.cpp (1)
37-39: Resolved: added missing includesIncluding MLDSAParameters.h and fixes the prior compile issue on stricter toolchains when using std::map.
src/lib/crypto/OSSLMLDSAPrivateKey.cpp (1)
120-133: Resolved: EVP_PKEY leak fixed in PKCS8Decode()Freeing localPKey after setFromOSSL avoids a leak. Thanks for addressing this.
src/lib/crypto/OSSLMLDSA.cpp (1)
48-56: Remove redundant NULL-check on pk after a non-NULL check of privateKey
pkis a direct cast ofprivateKey, which you already validated as non-NULL (Line 34). Thepk == NULLbranch is dead code; drop it.OSSLMLDSAPrivateKey *pk = (OSSLMLDSAPrivateKey *)privateKey; - - if (pk == NULL) - { - ERROR_MSG("Could not get the SoftHSM private key"); - - return false; - }src/lib/SoftHSM.cpp (4)
13001-13006: These helpers don’t use ‘this’ — consider making them static methods or free functionsgetMLDSAPrivateKey, getMLDSAPublicKey, and setMLDSAPrivateKey can be marked static (or moved out of the class) to reduce coupling, consistent with prior review discussion.
Also applies to: 13033-13038, 13518-13522
6414-6422: AES_CBC/DES3_CBC blocksize remains 0 in WrapKeySym (IV becomes empty) — set blocksize for non-PAD modesFor non-PAD CBC modes, blocksize is left as 0, so iv.resize(0) and memcpy with 0 bytes occurs; encryptInit then runs with an empty IV. This is a functional bug that breaks wrapping and is inconsistent with PAD cases.
Apply:
case CKM_AES_CBC: - algo = SymAlgo::AES; + algo = SymAlgo::AES; + blocksize = 16; break; @@ case CKM_DES3_CBC: - algo = SymAlgo::DES3; + algo = SymAlgo::DES3; + blocksize = 8; break; @@ - iv.resize(blocksize); - memcpy(&iv[0], pMechanism->pParameter, blocksize); + iv.resize(blocksize); + memcpy(&iv[0], pMechanism->pParameter, blocksize);Optional hardening (recommended): also validate pMechanism->ulParameterLen == blocksize before memcpy to fail fast on malformed IV.
Also applies to: 6423-6431, 6460-6463
13027-13031: Populate parameter set on ML‑DSA private key (required for backend context)getMLDSAPrivateKey sets value/seed but not the parameter set; the backend may need it to sign correctly.
Apply:
privateKey->setValue(value); privateKey->setSeed(seed); + // Ensure parameter set is conveyed to the runtime key + unsigned long parameterSet = key->getUnsignedLongValue(CKA_PARAMETER_SET, 0); + privateKey->setParameterSet(parameterSet);
13056-13058: Populate parameter set on ML‑DSA public keySame rationale as private: set the parameter set on the runtime object.
Apply:
publicKey->setValue(value); + unsigned long parameterSet = key->getUnsignedLongValue(CKA_PARAMETER_SET, 0); + publicKey->setParameterSet(parameterSet);
🧹 Nitpick comments (15)
m4/acx_botan_mldsa.m4 (2)
2-2: Drop AC_MSG_CHECKING when using AC_CACHE_CHECKAC_CACHE_CHECK emits both the checking message and the final result. Keeping AC_MSG_CHECKING here will duplicate or misalign output.
- AC_MSG_CHECKING(for Botan ML-DSA support) + # AC_CACHE_CHECK will print the check and result
5-8: Nit: LIBS mangling is unnecessary for a compile-only probeAC_COMPILE_IFELSE doesn’t link, so LIBS changes are unused. Dropping them reduces side effects in configure logs.
- tmp_LIBS=$LIBS ... - LIBS="$CRYPTO_LIBS $LIBS" ... - LIBS=$tmp_LIBSAlso applies to: 30-31
src/lib/crypto/MLDSAPublicKey.h (1)
23-31: Fix mislabeled comment for getParameterSet()The comment above getParameterSet() says "Get the bit length". It should describe the parameter set. Minor, but prevents confusion when skimming headers.
Apply:
- // Get the bit length + // Get the parameter set virtual unsigned long getParameterSet() const; - // Get the signature length + // Get the signature length virtual unsigned long getOutputLength() const; // Get the bit length virtual unsigned long getBitLength() const;src/lib/crypto/OSSLUtil.cpp (1)
43-49: Prefer a simple switch over a global std::map for 3 constantsA switch avoids static initialization, reduces code size, and removes an otherwise unnecessary dependency. Functionally identical here.
Apply:
-#ifdef WITH_ML_DSA -static std::map<unsigned long, const char*> mldsaAlgNameFromParameterSet { - {MLDSAParameters::ML_DSA_44_PARAMETER_SET, "ML-DSA-44"}, - {MLDSAParameters::ML_DSA_65_PARAMETER_SET, "ML-DSA-65"}, - {MLDSAParameters::ML_DSA_87_PARAMETER_SET, "ML-DSA-87"} -}; -#endif +#ifdef WITH_ML_DSA +#endifAnd rewrite the function:
#ifdef WITH_ML_DSA const char* OSSL::mldsaParameterSet2Name(unsigned long parameterSet) { - - std::map<unsigned long, const char*>::iterator it = mldsaAlgNameFromParameterSet.find(parameterSet); - - if (it != mldsaAlgNameFromParameterSet.end()) { - return it->second; - } - - return NULL; + switch (parameterSet) { + case MLDSAParameters::ML_DSA_44_PARAMETER_SET: return "ML-DSA-44"; + case MLDSAParameters::ML_DSA_65_PARAMETER_SET: return "ML-DSA-65"; + case MLDSAParameters::ML_DSA_87_PARAMETER_SET: return "ML-DSA-87"; + default: return NULL; + } } #endifIf you take this, you can also drop the include.
Also applies to: 223-234
src/lib/crypto/OSSLMLDSAPrivateKey.cpp (2)
7-16: Include <stdint.h> for uint8_tuint8_t is used; include <stdint.h>/ explicitly to avoid relying on transitive includes.
#include "OSSLUtil.h" +#include <stdint.h> #include <openssl/bn.h> #include <openssl/core_names.h> #include <openssl/x509.h>
60-63: Minor: clarify error messageTypo/redundant wording in the log.
- ERROR_MSG("Could not get private key private, rv: %d", rv); + ERROR_MSG("Could not get private key value, rv: %d", rv);src/lib/crypto/OSSLMLDSA.h (4)
10-13: Avoid exposing OpenSSL in the public header; include cstddef for size_tThis header doesn’t use any OpenSSL types/macros directly; only the .cpp needs <openssl/evp.h>. Also, this header uses size_t in method signatures; relying on transitive includes for size_t can be brittle.
- Remove <openssl/evp.h> from the header to reduce compile dependencies.
- Add so size_t is always defined here.
Apply:
#include "config.h" #include "AsymmetricAlgorithm.h" -#include <openssl/evp.h> +#include <cstddef>And ensure the implementation file includes OpenSSL:
// In src/lib/crypto/OSSLMLDSA.cpp #include <openssl/evp.h>
16-50: Add override specifiers to catch signature drift; modernize destructorMarking overrides explicitly improves safety (compiler checks exact signature matches) and readability. It would also immediately flag any mismatch with the base AsymmetricAlgorithm interface (e.g., missing const qualifiers or differing parameter lists).
Apply:
// Destructor - virtual ~OSSLMLDSA() { } + virtual ~OSSLMLDSA() override = default; // Signing functions - virtual bool sign(PrivateKey* privateKey, const ByteString& dataToSign, ByteString& signature, const AsymMech::Type mechanism, const void* param = NULL, const size_t paramLen = 0); - virtual bool signInit(PrivateKey* privateKey, const AsymMech::Type mechanism, const void* param = NULL, const size_t paramLen = 0); - virtual bool signUpdate(const ByteString& dataToSign); - virtual bool signFinal(ByteString& signature); + virtual bool sign(PrivateKey* privateKey, const ByteString& dataToSign, ByteString& signature, const AsymMech::Type mechanism, const void* param = NULL, const size_t paramLen = 0) override; + virtual bool signInit(PrivateKey* privateKey, const AsymMech::Type mechanism, const void* param = NULL, const size_t paramLen = 0) override; + virtual bool signUpdate(const ByteString& dataToSign) override; + virtual bool signFinal(ByteString& signature) override; // Verification functions - virtual bool verify(PublicKey* publicKey, const ByteString& originalData, const ByteString& signature, const AsymMech::Type mechanism, const void* param = NULL, const size_t paramLen = 0); - virtual bool verifyInit(PublicKey* publicKey, const AsymMech::Type mechanism, const void* param = NULL, const size_t paramLen = 0); - virtual bool verifyUpdate(const ByteString& originalData); - virtual bool verifyFinal(const ByteString& signature); + virtual bool verify(PublicKey* publicKey, const ByteString& originalData, const ByteString& signature, const AsymMech::Type mechanism, const void* param = NULL, const size_t paramLen = 0) override; + virtual bool verifyInit(PublicKey* publicKey, const AsymMech::Type mechanism, const void* param = NULL, const size_t paramLen = 0) override; + virtual bool verifyUpdate(const ByteString& originalData) override; + virtual bool verifyFinal(const ByteString& signature) override; // Encryption functions - virtual bool encrypt(PublicKey* publicKey, const ByteString& data, ByteString& encryptedData, const AsymMech::Type padding); + virtual bool encrypt(PublicKey* publicKey, const ByteString& data, ByteString& encryptedData, const AsymMech::Type padding) override; // Decryption functions - virtual bool checkEncryptedDataSize(PrivateKey* privateKey, const ByteString& encryptedData, int* errorCode); - virtual bool decrypt(PrivateKey* privateKey, const ByteString& encryptedData, ByteString& data, const AsymMech::Type padding); - virtual unsigned long getMinKeySize(); - virtual unsigned long getMaxKeySize(); + virtual bool checkEncryptedDataSize(PrivateKey* privateKey, const ByteString& encryptedData, int* errorCode) override; + virtual bool decrypt(PrivateKey* privateKey, const ByteString& encryptedData, ByteString& data, const AsymMech::Type padding) override; + virtual unsigned long getMinKeySize() override; + virtual unsigned long getMaxKeySize() override; // Key factory - virtual bool generateKeyPair(AsymmetricKeyPair** ppKeyPair, AsymmetricParameters* parameters, RNG* rng = NULL); - virtual bool reconstructKeyPair(AsymmetricKeyPair** ppKeyPair, ByteString& serialisedData); - virtual bool reconstructPublicKey(PublicKey** ppPublicKey, ByteString& serialisedData); - virtual bool reconstructPrivateKey(PrivateKey** ppPrivateKey, ByteString& serialisedData); - virtual bool reconstructParameters(AsymmetricParameters** ppParams, ByteString& serialisedData); - virtual PublicKey* newPublicKey(); - virtual PrivateKey* newPrivateKey(); - virtual AsymmetricParameters* newParameters(); + virtual bool generateKeyPair(AsymmetricKeyPair** ppKeyPair, AsymmetricParameters* parameters, RNG* rng = NULL) override; + virtual bool reconstructKeyPair(AsymmetricKeyPair** ppKeyPair, ByteString& serialisedData) override; + virtual bool reconstructPublicKey(PublicKey** ppPublicKey, ByteString& serialisedData) override; + virtual bool reconstructPrivateKey(PrivateKey** ppPrivateKey, ByteString& serialisedData) override; + virtual bool reconstructParameters(AsymmetricParameters** ppParams, ByteString& serialisedData) override; + virtual PublicKey* newPublicKey() override; + virtual PrivateKey* newPrivateKey() override; + virtual AsymmetricParameters* newParameters() override;Note: If the base class marks any of these as const (e.g., getMinKeySize() const), the compiler will help you align signatures once override is added.
32-37: Clarify that ML-DSA does not support encryption/decryptionThese methods purposely return false in the implementation. A short note in the header helps prevent confusion for future readers.
Apply:
- // Encryption functions + // Encryption functions (ML-DSA does not support encryption; methods return false) virtual bool encrypt(PublicKey* publicKey, const ByteString& data, ByteString& encryptedData, const AsymMech::Type padding); - // Decryption functions + // Decryption functions (ML-DSA does not support decryption; methods return false) virtual bool checkEncryptedDataSize(PrivateKey* privateKey, const ByteString& encryptedData, int* errorCode); virtual bool decrypt(PrivateKey* privateKey, const ByteString& encryptedData, ByteString& data, const AsymMech::Type padding);
51-52: Optional: remove empty private sectionThere are no private members. Consider removing the empty private: block to reduce noise, unless you plan to add members imminently.
src/lib/crypto/OSSLMLDSA.cpp (2)
138-141: Fix comment: it’s a public key type check, not privateSmall clarity nit.
- // Check if the private key is the right type + // Check if the public key is the right type
164-168: Improve error message with expected vs actual signature lengthsThis aids operators and test triage.
- ERROR_MSG("Invalid buffer length"); + ERROR_MSG("Invalid signature length: got %zu, expected %zu", signature.size(), len);src/lib/SoftHSM.cpp (3)
10064-10067: Prefer standard error code and fix format specifier for paramSet diagnostics
- Use CKR_MECHANISM_PARAM_INVALID instead of CKR_PARAMETER_SET_NOT_SUPPORTED (unless you explicitly want to expose that code).
- paramSet is CK_ULONG; use %lu and cast to unsigned long for portability.
Apply:
- if (paramSet != 1UL && paramSet != 2UL && paramSet != 3UL) { - INFO_MSG("Wrong parameterSet: %ld", paramSet); - return CKR_PARAMETER_SET_NOT_SUPPORTED; + if (paramSet != 1UL && paramSet != 2UL && paramSet != 3UL) { + INFO_MSG("Unsupported parameter set: %lu", (unsigned long)paramSet); + return CKR_MECHANISM_PARAM_INVALID; }Nit: if your PKCS#11 headers define CKP_ML_DSA_* constants for parameter sets, prefer those symbolic constants over raw numerics to align with project conventions.
10215-10217: Remove unused variable ‘parameterSet’Local ‘parameterSet’ is never used.
Apply:
- // MLDSA Private Key Attributes - ByteString parameterSet; - ByteString value; + // MLDSA Private Key Attributes + ByteString value; ByteString seed;
13536-13539: Remove unused ‘parameterSet’ variable in setMLDSAPrivateKeyIt’s not used; the attribute is set directly from priv->getParameterSet().
Apply:
- // ML-DSA Private Key Attributes - ByteString parameterSet; + // ML-DSA Private Key Attributes ByteString seed; ByteString value;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (14)
.github/workflows/ci.yml(1 hunks)CMakeLists.txt(1 hunks)m4/acx_botan_mldsa.m4(1 hunks)src/lib/P11Attributes.cpp(2 hunks)src/lib/SoftHSM.cpp(35 hunks)src/lib/SoftHSM.h(4 hunks)src/lib/crypto/MLDSAPublicKey.h(1 hunks)src/lib/crypto/OSSLMLDSA.cpp(1 hunks)src/lib/crypto/OSSLMLDSA.h(1 hunks)src/lib/crypto/OSSLMLDSAKeyPair.cpp(1 hunks)src/lib/crypto/OSSLMLDSAKeyPair.h(1 hunks)src/lib/crypto/OSSLMLDSAPrivateKey.cpp(1 hunks)src/lib/crypto/OSSLMLDSAPublicKey.cpp(1 hunks)src/lib/crypto/OSSLUtil.cpp(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/lib/crypto/OSSLMLDSAPublicKey.cpp
- src/lib/crypto/OSSLMLDSAKeyPair.cpp
- src/lib/crypto/OSSLMLDSAKeyPair.h
- src/lib/SoftHSM.h
- .github/workflows/ci.yml
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-18T17:54:37.601Z
Learnt from: antoinelochet
PR: softhsm/SoftHSMv2#809
File: m4/acx_openssl_mldsa.m4:10-35
Timestamp: 2025-08-18T17:54:37.601Z
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_botan_mldsa.m4
📚 Learning: 2025-08-18T17:59:30.642Z
Learnt from: antoinelochet
PR: softhsm/SoftHSMv2#809
File: src/lib/crypto/MLDSAParameters.h:42-44
Timestamp: 2025-08-18T17:59:30.642Z
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/OSSLMLDSA.hsrc/lib/crypto/OSSLMLDSA.cppsrc/lib/crypto/OSSLUtil.cppsrc/lib/SoftHSM.cppsrc/lib/crypto/OSSLMLDSAPrivateKey.cppsrc/lib/crypto/MLDSAPublicKey.h
🧬 Code Graph Analysis (4)
src/lib/crypto/OSSLMLDSA.h (6)
src/lib/crypto/AsymmetricAlgorithm.h (2)
AsymmetricAlgorithm(116-185)AsymmetricAlgorithm(123-123)src/lib/crypto/OSSLMLDSA.cpp (42)
sign(24-97)sign(24-26)signInit(99-105)signInit(99-100)signUpdate(107-112)signUpdate(107-107)signFinal(114-119)signFinal(114-114)verify(122-222)verify(122-124)verifyInit(224-230)verifyInit(224-225)verifyUpdate(232-237)verifyUpdate(232-232)verifyFinal(239-244)verifyFinal(239-239)encrypt(247-253)encrypt(247-248)checkEncryptedDataSize(274-279)checkEncryptedDataSize(274-274)decrypt(256-262)decrypt(256-257)getMinKeySize(264-267)getMinKeySize(264-264)getMaxKeySize(269-272)getMaxKeySize(269-269)generateKeyPair(282-345)generateKeyPair(282-282)reconstructKeyPair(347-383)reconstructKeyPair(347-347)reconstructPublicKey(385-406)reconstructPublicKey(385-385)reconstructPrivateKey(408-429)reconstructPrivateKey(408-408)reconstructParameters(446-466)reconstructParameters(446-446)newPublicKey(431-434)newPublicKey(431-431)newPrivateKey(436-439)newPrivateKey(436-436)newParameters(441-444)newParameters(441-441)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/OSSLMLDSA.cpp (1)
src/lib/crypto/OSSLUtil.cpp (2)
mldsaParameterSet2Name(224-233)mldsaParameterSet2Name(224-224)
src/lib/crypto/OSSLMLDSAPrivateKey.cpp (4)
src/lib/crypto/OSSLMLDSAPrivateKey.h (1)
OSSLMLDSAPrivateKey(16-56)src/lib/crypto/MLDSAPrivateKey.cpp (10)
setSeed(65-68)setSeed(65-65)setValue(55-58)setValue(55-55)isOfType(17-20)isOfType(17-17)getValue(60-63)getValue(60-60)getParameterSet(28-39)getParameterSet(28-28)src/lib/crypto/OSSLUtil.cpp (2)
mldsaParameterSet2Name(224-233)mldsaParameterSet2Name(224-224)src/lib/crypto/MLDSAParameters.cpp (2)
getParameterSet(22-25)getParameterSet(22-22)
src/lib/crypto/MLDSAPublicKey.h (4)
src/lib/crypto/PublicKey.h (1)
PublicKey(40-54)src/lib/crypto/OSSLMLDSAPublicKey.cpp (4)
isOfType(63-66)isOfType(63-63)setValue(68-76)setValue(68-68)src/lib/crypto/MLDSAPublicKey.cpp (16)
isOfType(17-20)isOfType(17-17)getParameterSet(28-39)getParameterSet(28-28)getOutputLength(42-53)getOutputLength(42-42)getBitLength(22-25)getBitLength(22-22)setValue(60-63)setValue(60-60)getValue(55-58)getValue(55-55)serialise(66-69)serialise(66-66)deserialise(71-83)deserialise(71-71)src/lib/crypto/MLDSAParameters.cpp (6)
getParameterSet(22-25)getParameterSet(22-22)serialise(34-37)serialise(34-34)deserialise(39-50)deserialise(39-39)
🔇 Additional comments (16)
m4/acx_botan_mldsa.m4 (2)
4-11: Good hygiene: language push/pop and CPPFLAGS save/restoreThe macro correctly switches to C++ and restores CPPFLAGS/LIBS afterward, and exposes have_lib_botan_mldsa_support for downstream logic.
Also applies to: 28-33
32-33: ML-DSA detection is properly propagatedThe
have_lib_botan_mldsa_supportvariable is indeed consumed downstream in m4/acx_crypto_backend.m4, influencing the build via the case dispatch:
- m4/acx_crypto_backend.m4:191-194 –
case "${enable_mldsa}-${have_lib_botan_mldsa_support}" in yes-no) AC_MSG_ERROR([Botan library has no ML-DSA support]);; detect-*) enable_mldsa="${have_lib_botan_mldsa_support}";; esacNo further AC_SUBST or AM_CONDITIONAL is required, as the m4 dispatch already covers propagation.
src/lib/crypto/MLDSAPublicKey.h (1)
18-21: Static ‘type’ definition confirmed
- Definition found once in
src/lib/crypto/MLDSAPublicKey.cpp:14:/*static*/ const char* MLDSAPublicKey::type = "Abstract ML-DSA public key";No further action required.
src/lib/P11Attributes.cpp (1)
2558-2596: CKA_SEED handling is secure and consistent
- No plaintext logging; encryption pathways mirror existing sensitive attributes.
- Accepts 0-length with NULL pointer via upstream checks. Stores as-is for public objects, encrypted for private ones.
Looks good.
src/lib/crypto/OSSLMLDSA.h (2)
7-9: Include guard mismatch is resolved — LGTMThe opening and closing guards consistently use _SOFTHSM_V2_OSSLMLDSA_H now. Thanks for fixing the earlier mismatch.
Also applies to: 54-54
21-40: All OSSLMLDSA method signatures match the AsymmetricAlgorithm base class – you can safely addoverride.I’ve confirmed that for each of these methods in
src/lib/crypto/OSSLMLDSA.h, the parameter lists and return types are identical to the declarations insrc/lib/crypto/AsymmetricAlgorithm.h:
- sign, signInit, signUpdate, signFinal
- verify, verifyInit, verifyUpdate, verifyFinal
- encrypt, decrypt
- checkEncryptedDataSize
- getMinKeySize, getMaxKeySize
Feel free to annotate each with
overrideto prevent any silent non-override errors in the future.src/lib/crypto/OSSLMLDSA.cpp (7)
76-82: Good: EVP_MD_CTX_new() NULL-checkedPrevents a potential crash on allocation failure. Nicely handled.
181-186: Good: explicit guard for unknown/unsupported parameter setsFail-fast with a clear diagnostic and clean-up. This improves debuggability significantly.
205-217: Good: distinguish invalid signatures from OpenSSL errorsCorrect handling: 0 => invalid signature; <0 => provider error via ERR_get_error(). Clean and accurate.
302-307: Good: keygen explicitly rejects unknown parameter setsNice early error with a clear message before context creation.
331-342: Good: proper EVP_PKEY refcounting to avoid UAF/double-freeUsing EVP_PKEY_up_ref before handing the same pkey to both wrappers and freeing the original reference is the correct ownership model.
246-279: Clear unsupported-operation messaging (encrypt/decrypt/checkEncryptedDataSize)Explicit “not supported” errors are fine and consistent with signature-only algorithms.
264-272: Units confirmed as bits—no change neededThe constants ML_DSA_44_PRIV_LENGTH (2560) and ML_DSA_87_PRIV_LENGTH (4896) in MLDSAParameters.h represent key lengths in bits, matching the bit‐based values returned by all other
getMinKeySize/getMaxKeySizeimplementations (e.g., RSA: 1024–4096, ECDSA: 112–521). Your use of these named constants is correct and requires no adjustment.src/lib/SoftHSM.cpp (3)
146-148: ML‑DSA P11 object wiring (public key) looks correctnewP11Object correctly recognizes CKK_ML_DSA and instantiates P11MLDSAPublicKeyObj.
165-167: ML‑DSA P11 object wiring (private key) looks correctnewP11Object correctly recognizes CKK_ML_DSA and instantiates P11MLDSAPrivateKeyObj.
830-833: Mechanism table and info for ML‑DSA: LGTM
- CKM_ML_DSA[_KEY_PAIR_GEN] added under WITH_ML_DSA.
- Min/max sizes obtained from CryptoFactory and exposed in C_GetMechanismInfo.
- Flags CKF_GENERATE_KEY_PAIR and CKF_SIGN|CKF_VERIFY are consistent.
No issues spotted.
Also applies to: 938-941, 1038-1049, 1336-1346
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (2)
src/lib/SoftHSM.cpp (2)
7005-7026: Unwrap path does not support non-PAD CBC and has two critical bugs
- Unwrap only handles CKM_AES_CBC_PAD/CKM_DES3_CBC_PAD; the wrap path now accepts non-PAD CKM_AES_CBC/CKM_DES3_CBC (once fixed above). Add symmetric support here.
- Wrong return code and logic in key-type checks:
- Returning CKR_WRAPPING_KEY_TYPE_INCONSISTENT in Unwrap; should be CKR_UNWRAPPING_KEY_TYPE_INCONSISTENT.
- The DES3 check uses “!= DES2 || != DES3” which is always true. Use &&.
Apply to UnwrapKeySym to accept non-PAD CBC:
switch(pMechanism->mechanism) { @@ - case CKM_AES_CBC_PAD: + case CKM_AES_CBC: + algo = SymAlgo::AES; + blocksize = 16; + break; + case CKM_AES_CBC_PAD: algo = SymAlgo::AES; blocksize = 16; break; @@ - case CKM_DES3_CBC_PAD: + case CKM_DES3_CBC: + algo = SymAlgo::DES3; + blocksize = 8; + break; + case CKM_DES3_CBC_PAD: algo = SymAlgo::DES3; blocksize = 8; break;Apply to C_UnwrapKey mechanism validation to accept non-PAD CBC with IV length checks:
- case CKM_AES_CBC_PAD: + case CKM_AES_CBC: + if (pMechanism->pParameter == NULL_PTR || + pMechanism->ulParameterLen != 16) + return CKR_ARGUMENTS_BAD; + break; + case CKM_AES_CBC_PAD: // TODO check block length if (pMechanism->pParameter == NULL_PTR || pMechanism->ulParameterLen != 16) return CKR_ARGUMENTS_BAD; break; @@ - case CKM_DES3_CBC_PAD: + case CKM_DES3_CBC: + if (pMechanism->pParameter == NULL_PTR || + pMechanism->ulParameterLen != 8) + return CKR_ARGUMENTS_BAD; + break; + case CKM_DES3_CBC_PAD: // TODO check block length if (pMechanism->pParameter == NULL_PTR || pMechanism->ulParameterLen != 8) return CKR_ARGUMENTS_BAD; break;Fix the unwrapping key-type checks:
- if ((pMechanism->mechanism == CKM_AES_CBC || pMechanism->mechanism == CKM_AES_CBC_PAD) && unwrapKey->getUnsignedLongValue(CKA_KEY_TYPE, CKK_VENDOR_DEFINED) != CKK_AES) - return CKR_WRAPPING_KEY_TYPE_INCONSISTENT; + if ((pMechanism->mechanism == CKM_AES_CBC || pMechanism->mechanism == CKM_AES_CBC_PAD) && + unwrapKey->getUnsignedLongValue(CKA_KEY_TYPE, CKK_VENDOR_DEFINED) != CKK_AES) + return CKR_UNWRAPPING_KEY_TYPE_INCONSISTENT; @@ - if (pMechanism->mechanism == CKM_DES3_CBC && (unwrapKey->getUnsignedLongValue(CKA_KEY_TYPE, CKK_VENDOR_DEFINED) != CKK_DES2 || - unwrapKey->getUnsignedLongValue(CKA_KEY_TYPE, CKK_VENDOR_DEFINED) != CKK_DES3)) - return CKR_WRAPPING_KEY_TYPE_INCONSISTENT; + if (pMechanism->mechanism == CKM_DES3_CBC && + (unwrapKey->getUnsignedLongValue(CKA_KEY_TYPE, CKK_VENDOR_DEFINED) != CKK_DES2 && + unwrapKey->getUnsignedLongValue(CKA_KEY_TYPE, CKK_VENDOR_DEFINED) != CKK_DES3)) + return CKR_UNWRAPPING_KEY_TYPE_INCONSISTENT;Note: Consider making the analogous DES3 check fix in C_WrapKey too (same ‘||’ issue) for consistency.
Also applies to: 7051-7086, 7364-7377, 7416-7421
7416-7421: Fix two typos in Unwrap key-type check (return code and logic)
- Use UNWRAPPING in the error code for Unwrap path.
- Correct the boolean logic so the check does not always fail.
Apply:
- if ((pMechanism->mechanism == CKM_AES_CBC || pMechanism->mechanism == CKM_AES_CBC_PAD) && unwrapKey->getUnsignedLongValue(CKA_KEY_TYPE, CKK_VENDOR_DEFINED) != CKK_AES) - return CKR_WRAPPING_KEY_TYPE_INCONSISTENT; + if ((pMechanism->mechanism == CKM_AES_CBC || pMechanism->mechanism == CKM_AES_CBC_PAD) && + unwrapKey->getUnsignedLongValue(CKA_KEY_TYPE, CKK_VENDOR_DEFINED) != CKK_AES) + return CKR_UNWRAPPING_KEY_TYPE_INCONSISTENT; @@ - if (pMechanism->mechanism == CKM_DES3_CBC && (unwrapKey->getUnsignedLongValue(CKA_KEY_TYPE, CKK_VENDOR_DEFINED) != CKK_DES2 || - unwrapKey->getUnsignedLongValue(CKA_KEY_TYPE, CKK_VENDOR_DEFINED) != CKK_DES3)) - return CKR_WRAPPING_KEY_TYPE_INCONSISTENT; + if (pMechanism->mechanism == CKM_DES3_CBC && + (unwrapKey->getUnsignedLongValue(CKA_KEY_TYPE, CKK_VENDOR_DEFINED) != CKK_DES2 && + unwrapKey->getUnsignedLongValue(CKA_KEY_TYPE, CKK_VENDOR_DEFINED) != CKK_DES3)) + return CKR_UNWRAPPING_KEY_TYPE_INCONSISTENT;
♻️ Duplicate comments (1)
src/lib/SoftHSM.cpp (1)
4190-4191: Populate ML-DSA parameter set on runtime key objectsThe runtime MLDSA{Private,Public}Key instances created via MLDSAUtil currently only set value/seed. Parameter set is required for the backend to know which ML-DSA variant to use.
Please extend MLDSAUtil::{getMLDSAPrivateKey,getMLDSAPublicKey} to set the parameter set read from the OSObject:
Outside this file (src/lib/crypto/MLDSAUtil.cpp), apply:
--- a/src/lib/crypto/MLDSAUtil.cpp +++ b/src/lib/crypto/MLDSAUtil.cpp @@ CK_RV MLDSAUtil::getMLDSAPrivateKey(MLDSAPrivateKey* privateKey, Token* token, OSObject* key) @@ - privateKey->setValue(value); - privateKey->setSeed(seed); + privateKey->setValue(value); + privateKey->setSeed(seed); + // Parameter set is required + CK_ULONG paramSet = key->getUnsignedLongValue(CKA_PARAMETER_SET, 0); + privateKey->setParameterSet(paramSet); @@ CK_RV MLDSAUtil::getMLDSAPublicKey(MLDSAPublicKey* publicKey, Token* token, OSObject* key) @@ - publicKey->setValue(value); + publicKey->setValue(value); + CK_ULONG paramSet = key->getUnsignedLongValue(CKA_PARAMETER_SET, 0); + publicKey->setParameterSet(paramSet);Would you like me to open a follow-up PR for this?
Also applies to: 4434-4439, 4527-4546, 5199-5200, 5441-5446, 5534-5553
🧹 Nitpick comments (6)
src/lib/crypto/MLDSAUtil.h (2)
29-30: Fix mismatched/end-if comments for clarityThe nesting is correct (#ifdef WITH_ML_DSA closes before the include guard), but the comments are swapped and confusing. Align comments with the blocks they close.
-#endif // !_SOFTHSM_V2_MLDSAUTIL_H -#endif +#endif // WITH_ML_DSA +#endif // _SOFTHSM_V2_MLDSAUTIL_H
14-15: Trim unused includes from the header to reduce transitive dependenciesAsymmetricAlgorithm.h and CryptoFactory.h are not needed for the declarations in this header (they’re only used in the .cpp). Removing them will reduce rebuilds and header coupling.
-#include "AsymmetricAlgorithm.h" -#include "CryptoFactory.h"Add them in the implementation file instead (see suggested change in MLDSAUtil.cpp).
src/lib/crypto/MLDSAUtil.cpp (3)
52-54: Fix copy/paste comment (EC → ML-DSA)Minor correctness/clarity tweak.
- // EC Public Key Attributes + // ML-DSA Public Key Attributes
89-89: Remove unused variableparameterSet is declared but never used; drop it to avoid warnings.
- ByteString parameterSet;
7-10: Move algorithm/factory includes from header into the implementationTo support the header cleanup, include what you use here.
#include "config.h" #ifdef WITH_ML_DSA #include "MLDSAUtil.h" +#include "AsymmetricAlgorithm.h" +#include "CryptoFactory.h"src/lib/SoftHSM.cpp (1)
6734-6739: Symmetry: consider allowing DES3_CBC in C_WrapKeyYou support CKM_DES3_CBC in WrapKeySym but the top-level C_WrapKey switch does not accept CKM_DES3_CBC. Add a branch with IV length 8 for consistency with the unwrap side.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
src/lib/SoftHSM.cpp(34 hunks)src/lib/SoftHSM.h(2 hunks)src/lib/crypto/CMakeLists.txt(2 hunks)src/lib/crypto/MLDSAUtil.cpp(1 hunks)src/lib/crypto/MLDSAUtil.h(1 hunks)src/lib/crypto/Makefile.am(3 hunks)src/lib/crypto/OSSLComp.cpp(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/lib/crypto/OSSLComp.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/crypto/CMakeLists.txt
- src/lib/crypto/Makefile.am
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-18T17:59:30.642Z
Learnt from: antoinelochet
PR: softhsm/SoftHSMv2#809
File: src/lib/crypto/MLDSAParameters.h:42-44
Timestamp: 2025-08-18T17:59:30.642Z
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/MLDSAUtil.hsrc/lib/crypto/MLDSAUtil.cppsrc/lib/SoftHSM.cppsrc/lib/SoftHSM.h
🧬 Code Graph Analysis (2)
src/lib/crypto/MLDSAUtil.h (1)
src/lib/crypto/MLDSAUtil.cpp (6)
getMLDSAPrivateKey(11-41)getMLDSAPrivateKey(11-11)getMLDSAPublicKey(43-69)getMLDSAPublicKey(43-43)setMLDSAPrivateKey(71-111)setMLDSAPrivateKey(71-71)
src/lib/SoftHSM.cpp (2)
src/lib/crypto/MLDSAUtil.cpp (6)
getMLDSAPrivateKey(11-41)getMLDSAPrivateKey(11-11)getMLDSAPublicKey(43-69)getMLDSAPublicKey(43-43)setMLDSAPrivateKey(71-111)setMLDSAPrivateKey(71-71)src/lib/P11Objects.cpp (2)
isPrivate(303-311)isPrivate(303-303)
🔇 Additional comments (11)
src/lib/crypto/MLDSAUtil.h (1)
23-27: API shape looks good and consistentThe utility provides a minimal and focused surface for ML-DSA key get/set, taking Token and OSObject as inputs and respecting privacy. Signature choices (CK_RV for getters, bool for setter) match existing patterns.
src/lib/SoftHSM.h (2)
55-58: Good: MLDSA includes correctly gatedWrapping MLDSA key includes with WITH_ML_DSA prevents builds from breaking when ML-DSA is disabled or OpenSSL < 3.5.
378-393: Good: MLDSA generator declaration properly feature-gatedThe generateMLDSA() declaration matches the conditional compilation used elsewhere, avoiding undefined references when ML-DSA is not enabled.
src/lib/crypto/MLDSAUtil.cpp (2)
94-105: I’ve confirmed that Token::encrypt returns a bool and that the MLDSAUtil method itself returns a bool status. Integrating encryption failures into the overall success flag and switching to static_cast is both safe and consistent with existing patterns in P11Attributes.cpp and SoftHSM.cpp.Refactor MLDSAUtil to check encrypt() results and use static_cast
• In
src/lib/crypto/MLDSAUtil.cpp, lines 93–105:
– Wrap the twotoken->encrypt(...)calls in a boolean check (e.g.encOK)
– On failure, recycle the private key and algorithm, then returnfalseimmediately
– Replace all C–style casts onprivwithstatic_cast<MLDSAPrivateKey*>• Downstream code already combines
bOKfromsetAttribute; no change needed there beyond using the safer cast forgetParameterSet().Revised snippet:
- if (isPrivate) - { - token->encrypt(((MLDSAPrivateKey*)priv)->getSeed(), seed); - token->encrypt(((MLDSAPrivateKey*)priv)->getValue(), value); - } + if (isPrivate) + { + bool encOK = true; + auto mlpriv = static_cast<MLDSAPrivateKey*>(priv); + encOK = encOK && token->encrypt(mlpriv->getSeed(), seed); + encOK = encOK && token->encrypt(mlpriv->getValue(), value); + if (!encOK) + { + mldsa->recyclePrivateKey(priv); + CryptoFactory::i()->recycleAsymmetricAlgorithm(mldsa); + return false; + } + } else { - seed = ((MLDSAPrivateKey*)priv)->getSeed(); - value = ((MLDSAPrivateKey*)priv)->getValue(); + auto mlpriv = static_cast<MLDSAPrivateKey*>(priv); + seed = mlpriv->getSeed(); + value = mlpriv->getValue(); } bool bOK = true; - bOK = bOK && key->setAttribute(CKA_PARAMETER_SET, ((MLDSAPrivateKey*)priv)->getParameterSet()); + bOK = bOK && key->setAttribute(CKA_PARAMETER_SET, + static_cast<MLDSAPrivateKey*>(priv)->getParameterSet()); bOK = bOK && key->setAttribute(CKA_SEED, seed); bOK = bOK && key->setAttribute(CKA_VALUE, value);This aligns MLDSAUtil with existing encrypt()–error–handling patterns and uses modern C++ casts.
71-111: Ignore the parameter-set setter suggestionAfter reviewing the code:
- MLDSAUtil::setMLDSAPrivateKey already writes CKA_PARAMETER_SET to the token object.
- MLDSAPrivateKey and MLDSAPublicKey only expose getParameterSet(), which computes the domain parameters from the key’s value size—there is no setParameterSet() on those classes.
- A manual setter on the in-memory key isn’t available or needed.
No changes required.
Likely an incorrect or invalid review comment.
src/lib/SoftHSM.cpp (6)
65-70: ML-DSA headers are correctly gatedIncludes for ML-DSA are properly hidden behind WITH_ML_DSA. No concerns here.
832-836: Mechanism table extended for ML-DSACKM_ML_DSA_KEY_PAIR_GEN and CKM_ML_DSA added and gated. Looks good.
941-944: Mechanism info for ML-DSA is consistent and gated
- Min/max sizes are queried via CryptoFactory behind WITH_ML_DSA.
- CKM_ML_DSA and KEY_PAIR_GEN expose sensible flags (SIGN/VERIFY and GENERATE_KEY_PAIR).
LGTM.
Also applies to: 1041-1052, 1339-1349
6167-6170: Key-pair generation flow and ML-DSA routingC_GenerateKeyPair integration (mechanism dispatch and routing to generateMLDSA) looks correct and gated via WITH_ML_DSA. No issues.
Also applies to: 6198-6200, 6224-6225, 6298-6305
5672-5673: Minor: Debug output is fineExtra DEBUG_MSG with sizes only (no sensitive data) is acceptable.
Also applies to: 5856-5856
149-151: Wrap ML-DSA support in WITH_ML_DSA guardsThe ML-DSA key-object classes and their use in SoftHSM.cpp aren’t currently wrapped in
#ifdef WITH_ML_DSA, so they’ll always be compiled—even when ML-DSA support is disabled. We need to guard both the class definitions and theCKK_ML_DSAbranches.Locations to fix:
- src/lib/P11Objects.h
- Line 201:
class P11MLDSAPublicKeyObj- Line 303:
class P11MLDSAPrivateKeyObj
Wrap each definition in:#ifdef WITH_ML_DSA class P11MLDSAPublicKeyObj : public P11PublicKeyObj { … }; // … class P11MLDSAPrivateKeyObj : public P11PrivateKeyObj { … }; #endif // WITH_ML_DSA- src/lib/SoftHSM.cpp
- Lines 149–151 and 169–170: the
else if (keyType == CKK_ML_DSA)branches
Wrap these cases in the same guard, for example:#ifdef WITH_ML_DSA- else if (keyType == CKK_ML_DSA)
*p11object = new P11MLDSAPublicKeyObj();
- else if (keyType == CKK_ML_DSA) {
*p11object = new P11MLDSAPublicKeyObj();- }
#endif // WITH_ML_DSA<details> <summary>⛔ Skipped due to learnings</summary>Learnt from: antoinelochet
PR: #809
File: src/lib/crypto/MLDSAParameters.h:42-44
Timestamp: 2025-08-18T17:59:30.642Z
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.</details> </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
antoinelochet
force-pushed
the
ml-dsa-pr
branch
2 times, most recently
from
3bf81b0 to
54e8ee0
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/lib/SoftHSM.cpp (1)
5445-5556: ML-DSA verify path: propagate parameter set to MLDSAPublicKeyAfter calling
MLDSAUtil::getMLDSAPublicKey((MLDSAPublicKey*)publicKey, token, key)in the CKM_ML_DSA branch (src/lib/SoftHSM.cpp around lines 5445–5556), you must also read the CKA_PARAMETER_SET from the OSObject and apply it to the in-memory public key so the backend has the full context for verification. For example:
Suggested diff (around line 5550):
if (MLDSAUtil::getMLDSAPublicKey((MLDSAPublicKey*)publicKey, token, key) != CKR_OK) { asymCrypto->recyclePublicKey(publicKey); CryptoFactory::i()->recycleAsymmetricAlgorithm(asymCrypto); return CKR_GENERAL_ERROR; } + // Apply parameter set for ML-DSA verify context + { + CK_ULONG paramSet = key->getUnsignedLongValue(CKA_PARAMETER_SET); + ((MLDSAPublicKey*)publicKey)->setParameterSet(paramSet); + }This mirrors the sign path and ensures the verifier knows which ML-DSA parameters to use.
♻️ Duplicate comments (2)
src/lib/SoftHSM.cpp (2)
10020-10277: generateMLDSA: overall solid, but remove an unused variable and consider return code convention
The flow for extracting CKA_PARAMETER_SET, parameter validation, keypair generation, and attribute population is consistent with the project style (using CKP_ML_DSA_* and returning CKR_PARAMETER_SET_NOT_SUPPORTED). Keeping that is fine given the current repository convention.
There’s an unused variable in the private key block:
- ByteString parameterSet; is declared but never used.
Apply this small cleanup:
@@ - // MLDSA Private Key Attributes - ByteString parameterSet; - ByteString value; - ByteString seed; + // MLDSA Private Key Attributes + ByteString value; + ByteString seed;
6421-6439: CBC wrap bug: empty IV used for non-PAD modes (AES_CBC/DES3_CBC) and missing input size checksFor CKM_AES_CBC and CKM_DES3_CBC you never set blocksize. Consequently:
- iv.resize(blocksize) yields a 0-length IV
- memcpy copies 0 bytes
- encryptInit gets an empty IV for CBC, which will fail or be unsafe
Additionally, for non-PAD modes the input must be a multiple of the block size.
Fix by setting blocksize, validating IV length, and (recommended) enforcing block-aligned input when no padding:
@@ - case CKM_AES_CBC: - algo = SymAlgo::AES; - break; + case CKM_AES_CBC: + algo = SymAlgo::AES; + blocksize = 16; + if (wrappedlen % blocksize != 0) return CKR_KEY_SIZE_RANGE; + break; @@ - case CKM_DES3_CBC: - algo = SymAlgo::DES3; - break; + case CKM_DES3_CBC: + algo = SymAlgo::DES3; + blocksize = 8; + if (wrappedlen % blocksize != 0) return CKR_KEY_SIZE_RANGE; + break; @@ - case CKM_AES_CBC: - case CKM_AES_CBC_PAD: - case CKM_DES3_CBC: - case CKM_DES3_CBC_PAD: - iv.resize(blocksize); - memcpy(&iv[0], pMechanism->pParameter, blocksize); + case CKM_AES_CBC: + case CKM_AES_CBC_PAD: + case CKM_DES3_CBC: + case CKM_DES3_CBC_PAD: + if (pMechanism->pParameter == NULL_PTR || + pMechanism->ulParameterLen != blocksize) { + cipher->recycleKey(wrappingkey); + CryptoFactory::i()->recycleSymmetricAlgorithm(cipher); + return CKR_ARGUMENTS_BAD; + } + iv.resize(blocksize); + memcpy(&iv[0], pMechanism->pParameter, blocksize);This mirrors the already-correct PAD paths and prevents empty-IV CBC usage.
Also applies to: 6463-6470
🧹 Nitpick comments (5)
src/lib/crypto/OSSLMLDSAPrivateKey.cpp (5)
61-63: Fix typo in error message"Could not get private key private" → "Could not get private key".
Apply this diff:
- ERROR_MSG("Could not get private key private, rv: %d", rv); + ERROR_MSG("Could not get private key, rv: %d", rv);
127-132: Harden PKCS#8 decode by checking the key algorithm is ML-DSAGuard against mis-decoding a different algorithm into this class.
Apply this diff:
EVP_PKEY* localPKey = EVP_PKCS82PKEY(p8); PKCS8_PRIV_KEY_INFO_free(p8); if (localPKey == NULL) return false; + // Ensure the decoded key is ML-DSA + if (!EVP_PKEY_is_a(localPKey, "ML-DSA")) { + EVP_PKEY_free(localPKey); + return false; + } setFromOSSL(localPKey); EVP_PKEY_free(localPKey); return true;
122-126: Avoid potential size truncation when converting BER lengthber.size() is size_t; storing into int risks truncation for very large inputs.
Apply this diff:
- int len = ber.size(); - if (len <= 0) return false; + if (ber.size() == 0) return false; + if (ber.size() > LONG_MAX) return false; + long len = static_cast<long>(ber.size());Additionally, ensure LONG_MAX is available by including climits at the top of the file:
// add near other includes #include <climits>
13-15: Remove unused include to keep build surface minimalopenssl/bn.h isn't used here.
Apply this diff:
-#include <openssl/bn.h>
44-69: Zeroize stack buffers holding sensitive material before returnSeed and private-key buffers remain on the stack; zeroizing is a small hardening win. Consider wiping in both success and error paths.
Apply this diff to wipe on the success path:
ByteString valueBS = ByteString(priv, priv_len); setValue(valueBS); +OPENSSL_cleanse(priv, sizeof(priv)); +OPENSSL_cleanse(seed, sizeof(seed));And in the error branch before returning:
- if(!rv) { - ERROR_MSG("Could not get private key, rv: %d", rv); - return; - } + if(!rv) { + ERROR_MSG("Could not get private key, rv: %d", rv); + OPENSSL_cleanse(priv, sizeof(priv)); + OPENSSL_cleanse(seed, sizeof(seed)); + return; + }Add the necessary include:
// near other includes #include <openssl/crypto.h> // for OPENSSL_cleanse
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/lib/SoftHSM.cpp(34 hunks)src/lib/crypto/OSSLMLDSAPrivateKey.cpp(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-18T17:59:30.642Z
Learnt from: antoinelochet
PR: softhsm/SoftHSMv2#809
File: src/lib/crypto/MLDSAParameters.h:42-44
Timestamp: 2025-08-18T17:59:30.642Z
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.cppsrc/lib/crypto/OSSLMLDSAPrivateKey.cpp
📚 Learning: 2025-08-18T19:46:46.645Z
Learnt from: antoinelochet
PR: softhsm/SoftHSMv2#809
File: src/lib/SoftHSM.cpp:10044-10055
Timestamp: 2025-08-18T19:46:46.645Z
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/SoftHSM.cppsrc/lib/crypto/OSSLMLDSAPrivateKey.cpp
🧬 Code Graph Analysis (2)
src/lib/SoftHSM.cpp (1)
src/lib/crypto/MLDSAUtil.cpp (6)
getMLDSAPrivateKey(11-41)getMLDSAPrivateKey(11-11)getMLDSAPublicKey(43-69)getMLDSAPublicKey(43-43)setMLDSAPrivateKey(71-111)setMLDSAPrivateKey(71-71)
src/lib/crypto/OSSLMLDSAPrivateKey.cpp (3)
src/lib/crypto/OSSLMLDSAPublicKey.cpp (10)
setFromOSSL(44-60)setFromOSSL(44-44)setValue(68-76)setValue(68-68)isOfType(63-66)isOfType(63-63)getOSSLKey(79-84)getOSSLKey(79-79)createOSSLKey(87-125)createOSSLKey(87-87)src/lib/crypto/MLDSAPrivateKey.cpp (10)
setSeed(65-68)setSeed(65-65)setValue(55-58)setValue(55-55)isOfType(17-20)isOfType(17-17)getValue(60-63)getValue(60-60)getParameterSet(28-39)getParameterSet(28-28)src/lib/crypto/OSSLUtil.cpp (2)
mldsaParameterSet2Name(224-233)mldsaParameterSet2Name(224-224)
🔇 Additional comments (12)
src/lib/crypto/OSSLMLDSAPrivateKey.cpp (6)
31-38: Proper pkey lifecycle management in destructor — LGTMDestructor correctly frees pkey and nulls it, preventing leaks/double-frees.
44-55: Seed retrieval handled correctly — LGTMOptional seed is read and applied only when present and of expected length.
98-117: PKCS#8 encode path frees resources on all branches — LGTMHappy path and error paths release PKCS8_PRIV_KEY_INFO correctly; der consistency check is good.
120-133: Memory leak from earlier revision is resolved — LGTMEVP_PKEY from PKCS#8 gets freed after setFromOSSL(localPKey); good catch and fix.
150-160: Good defensive checks for unknown parameter set and empty keyFail-fast with clear logs prevents undefined behavior when building EVP_PKEY.
172-191: Provider-based key creation path looks correct — LGTMNo pre-allocation of pkey before EVP_PKEY_fromdata, proper ctx cleanup on all paths.
src/lib/SoftHSM.cpp (6)
65-70: Good encapsulation: ML-DSA headers are properly compiled behind WITH_ML_DSAThe conditional includes keep non-ML builds unaffected. No issues.
149-152: P11 ML-DSA objects are correctly gated with WITH_ML_DSAAdding P11MLDSAPublicKeyObj and P11MLDSAPrivateKeyObj under the compile guard prevents build breaks on non-ML configurations. Looks good.
Also applies to: 171-173
837-840: Mechanism registry updated for ML-DSACKM_ML_DSA_KEY_PAIR_GEN and CKM_ML_DSA are registered under WITH_ML_DSA. This aligns with the overall gating strategy.
945-947: Mechanism info: ML-DSA min/max sizes wired through CryptoFactoryRetrieval and propagation of key sizes for CKM_ML_DSA(_KEY_PAIR_GEN) are consistent with the rest of the codebase and guarded behind WITH_ML_DSA. Flags are appropriate (generate/sign/verify).
Also applies to: 1045-1056, 1343-1353
6171-6174: Keygen dispatch and template validation for ML-DSA
- CKM_ML_DSA_KEY_PAIR_GEN is correctly integrated into dispatch.
- Template consistency checks mirror other mechanisms.
- Dispatch to generateMLDSA is straightforward.
All good.
Also applies to: 6202-6204, 6227-6229, 6302-6309
6899-6902: Wrap/unwrap integration for ML-DSA private keys looks correct
- Wrap path maps CKK_ML_DSA to AsymAlgo::MLDSA and uses MLDSAUtil::getMLDSAPrivateKey before PKCS8 encoding; good.
- Unwrap path stores ML-DSA private key with MLDSAUtil::setMLDSAPrivateKey; good.
Assuming MLDSAUtil is updated to also carry the parameter set on runtime key objects (see other comments), this integration is sound.
Also applies to: 6943-6946, 7624-7627
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/SoftHSM.cpp (1)
12423-12454: Bug: CKA_NEVER_EXTRACTABLE assignment uses CKA_ALWAYS_SENSITIVEIn the CKM_CONCATENATE_BASE_AND_KEY derivation path, the computed bNeverExtractable is assigned to CKA_ALWAYS_SENSITIVE, which is incorrect. It should set CKA_NEVER_EXTRACTABLE.
Apply:
- bool bNeverExtractable = baseKey->getBooleanValue(CKA_NEVER_EXTRACTABLE, false) && - otherKey->getBooleanValue(CKA_NEVER_EXTRACTABLE, false); - bOK = bOK && osobject->setAttribute(CKA_ALWAYS_SENSITIVE, bNeverExtractable); + bool bNeverExtractable = baseKey->getBooleanValue(CKA_NEVER_EXTRACTABLE, false) && + otherKey->getBooleanValue(CKA_NEVER_EXTRACTABLE, false); + bOK = bOK && osobject->setAttribute(CKA_NEVER_EXTRACTABLE, bNeverExtractable);
🧹 Nitpick comments (1)
src/lib/SoftHSM.cpp (1)
10223-10241: Remove unused local ‘parameterSet’ in ML-DSA private-key attributes‘ByteString parameterSet;’ is unused in this block.
Apply:
- // MLDSA Private Key Attributes - ByteString parameterSet; - ByteString value; - ByteString seed; + // MLDSA Private Key Attributes + ByteString value; + ByteString seed;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/lib/SoftHSM.cpp(34 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: antoinelochet
PR: softhsm/SoftHSMv2#809
File: src/lib/SoftHSM.cpp:4193-4196
Timestamp: 2025-08-19T09:23:25.386Z
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-19T09:23:25.386Z
Learnt from: antoinelochet
PR: softhsm/SoftHSMv2#809
File: src/lib/SoftHSM.cpp:4193-4196
Timestamp: 2025-08-19T09:23:25.386Z
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-18T19:46:46.645Z
Learnt from: antoinelochet
PR: softhsm/SoftHSMv2#809
File: src/lib/SoftHSM.cpp:10044-10055
Timestamp: 2025-08-18T19:46:46.645Z
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/SoftHSM.cpp
📚 Learning: 2025-08-18T17:59:30.642Z
Learnt from: antoinelochet
PR: softhsm/SoftHSMv2#809
File: src/lib/crypto/MLDSAParameters.h:42-44
Timestamp: 2025-08-18T17:59:30.642Z
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
🧬 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/crypto/MLDSAUtil.cpp (6)
getMLDSAPrivateKey(11-41)getMLDSAPrivateKey(11-11)getMLDSAPublicKey(43-69)getMLDSAPublicKey(43-43)setMLDSAPrivateKey(71-111)setMLDSAPrivateKey(71-71)
🔇 Additional comments (11)
src/lib/SoftHSM.cpp (11)
65-70: LGTM: ML-DSA headers are correctly feature-gatedIncluding ML-DSA headers under WITH_ML_DSA keeps non-ML builds clean.
149-152: LGTM: P11MLDSA object creation is behind WITH_ML_DSA*The new P11 object branches for CKK_ML_DSA are correctly wrapped with the feature guard.
Also applies to: 171-173
836-840: LGTM: Mechanism map extended with ML-DSACKM_ML_DSA_KEY_PAIR_GEN and CKM_ML_DSA added consistently with existing style.
945-948: LGTM: Mechanism info for ML-DSA added
- Min/max key sizes are fetched via CryptoFactory and recycled properly.
- Mechanism flags for keygen and sign/verify look correct.
Also applies to: 1045-1056, 1343-1353
1147-1147: Confirm reduced CKM_GENERIC_SECRET_KEY_GEN max size (0x8000000)You’ve reduced ulMaxKeySize from 0x80000000 to 0x8000000. This matches the check in generateGeneric (CKA_VALUE_LEN upper bound). Please confirm this change is intentional and won’t break existing clients depending on larger generic secret sizes.
4194-4443: LGTM: ML-DSA sign path integration
- Proper mechanism flagging, bAllowMultiPartOp=false, and use of MLDSAUtil for runtime key hydration.
- Parameter-set inference via key material length (no explicit setter) is consistent with the ML-DSA key types design.
Also applies to: 4531-4550
5203-5450: LGTM: ML-DSA verify path integration
- Mechanism mapping and object instantiation are consistent with the sign path.
- Uses MLDSAUtil for public key material.
Also applies to: 5538-5556
6739-6743: Wrap supports AES_CBC, but unwrap only supports AES_CBC_PAD — clarify intentC_WrapKey permits CKM_AES_CBC while UnwrapKeySym only implements PAD modes (CKM_AES_CBC_PAD/CKM_DES3_CBC_PAD). This asymmetry may surprise users.
- Either disallow CKM_AES_CBC in C_WrapKey, or add the corresponding non-PAD CBC path in UnwrapKeySym (with strict block/IV validation).
Also applies to: 7009-7035
10049-10076: LGTM: CKA_PARAMETER_SET parsing and validation
- Length check against sizeof(CK_ULONG) is correct.
- Using CKP_ML_DSA_44/65/87 constants improves clarity.
- Return code on unsupported set is CKR_PARAMETER_SET_NOT_SUPPORTED (assumed intentionally defined).
6890-6903: LGTM: ML-DSA integrated in wrap/unwrap flows
- Correct keyType routing to MLDSA for wrapping/unwrapping private keys.
- Uses MLDSAUtil for extracting/setting key material.
Also applies to: 6944-6947, 7625-7628
10099-10109: LGTM: ML-DSA key objects persist parameter set + seed/value
- Public key stores CKA_PARAMETER_SET and CKA_VALUE (optionally encrypted).
- Private key stores CKA_PARAMETER_SET, CKA_VALUE, and CKA_SEED (optionally encrypted).
No secret material is logged — good.Also applies to: 10156-10166, 10200-10210, 10238-10241
antoinelochet
force-pushed
the
ml-dsa-pr
branch
from
5fac462 to
c0ed069
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/lib/SoftHSM.cpp (1)
6421-6440: CBC wrap uses zero-length IV for non‑PAD modes — functional bug and potential UBFor CKM_AES_CBC and CKM_DES3_CBC, blocksize isn’t set, but you later do iv.resize(blocksize) and memcpy(..., blocksize). This yields a zero-length IV and memcpy with 0 bytes; accessing &iv[0] when size() == 0 is undefined behavior. Result: encryption init may use an empty IV or crash.
Fix by:
- Setting blocksize for non‑PAD modes (16 for AES, 8 for 3DES).
- Validating pParameter is non‑NULL and ulParameterLen equals blocksize for all CBC cases before memcpy/encryptInit.
Apply:
@@ - case CKM_AES_CBC: - algo = SymAlgo::AES; - break; + case CKM_AES_CBC: + algo = SymAlgo::AES; + blocksize = 16; + break; @@ - case CKM_DES3_CBC: - algo = SymAlgo::DES3; - break; + case CKM_DES3_CBC: + algo = SymAlgo::DES3; + blocksize = 8; + break; @@ - case CKM_AES_CBC: - case CKM_AES_CBC_PAD: - case CKM_DES3_CBC: - case CKM_DES3_CBC_PAD: - iv.resize(blocksize); - memcpy(&iv[0], pMechanism->pParameter, blocksize); + case CKM_AES_CBC: + case CKM_AES_CBC_PAD: + case CKM_DES3_CBC: + case CKM_DES3_CBC_PAD: + if (pMechanism->pParameter == NULL_PTR || + pMechanism->ulParameterLen != blocksize) { + cipher->recycleKey(wrappingkey); + CryptoFactory::i()->recycleSymmetricAlgorithm(cipher); + return CKR_ARGUMENTS_BAD; + } + iv.resize(blocksize); + memcpy(&iv[0], pMechanism->pParameter, blocksize);Optional (recommended): for non‑PAD modes also check keydata.size() is a multiple of blocksize before encrypting.
This is a correctness issue that can break CBC wrapping at runtime.
Also applies to: 6464-6470
🧹 Nitpick comments (2)
src/lib/SoftHSM.cpp (2)
10021-10279: Consider adding targeted tests for ML‑DSA generation/sign/verifyNew surface area merits basic tests to catch regressions:
- C_GenerateKeyPair with CKA_PARAMETER_SET = CKP_ML_DSA_44/65/87
- C_SignInit/C_Sign and C_VerifyInit/C_Verify with CKM_ML_DSA
- Attribute persistence: CKA_PARAMETER_SET, CKA_SEED, CKA_VALUE (private), CKA_VALUE (public)
I can draft unit/integration tests for these flows if helpful.
10223-10227: Remove unused local variable ‘parameterSet’‘ByteString parameterSet;’ is declared but never used in generateMLDSA’s private-key block. Clean it up to avoid warnings.
- // MLDSA Private Key Attributes - ByteString parameterSet; - ByteString value; - ByteString seed; + // MLDSA Private Key Attributes + ByteString value; + ByteString seed;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/lib/SoftHSM.cpp(34 hunks)src/lib/crypto/OSSLUtil.cpp(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/crypto/OSSLUtil.cpp
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: antoinelochet
PR: softhsm/SoftHSMv2#809
File: src/lib/SoftHSM.cpp:4193-4196
Timestamp: 2025-08-19T09:23:25.386Z
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-19T09:23:25.386Z
Learnt from: antoinelochet
PR: softhsm/SoftHSMv2#809
File: src/lib/SoftHSM.cpp:4193-4196
Timestamp: 2025-08-19T09:23:25.386Z
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.642Z
Learnt from: antoinelochet
PR: softhsm/SoftHSMv2#809
File: src/lib/crypto/MLDSAParameters.h:42-44
Timestamp: 2025-08-18T17:59:30.642Z
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-18T19:46:46.645Z
Learnt from: antoinelochet
PR: softhsm/SoftHSMv2#809
File: src/lib/SoftHSM.cpp:10044-10055
Timestamp: 2025-08-18T19:46:46.645Z
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/SoftHSM.cpp
🧬 Code Graph Analysis (1)
src/lib/SoftHSM.cpp (1)
src/lib/crypto/MLDSAUtil.cpp (6)
getMLDSAPrivateKey(11-41)getMLDSAPrivateKey(11-11)getMLDSAPublicKey(43-69)getMLDSAPublicKey(43-43)setMLDSAPrivateKey(71-111)setMLDSAPrivateKey(71-71)
🔇 Additional comments (8)
src/lib/SoftHSM.cpp (8)
65-70: Correctly gated ML‑DSA headersIncluding ML‑DSA headers behind WITH_ML_DSA is consistent with the project’s feature-gating approach. LGTM.
149-152: P11 ML‑DSA object creation properly feature‑gatedThe new CKK_ML_DSA branches for P11 public/private objects are correctly wrapped in WITH_ML_DSA. Prevents non-ML builds from referencing unknown symbols. LGTM.
Also applies to: 171-173
837-840: Mechanism registration for ML‑DSA addedCKM_ML_DSA_KEY_PAIR_GEN and CKM_ML_DSA are registered under WITH_ML_DSA. Good integration with supportedMechanisms and config filtering.
945-947: Mechanism info wiring for ML‑DSA is consistent
- Min/max key sizes fetched via CryptoFactory (mirrors other algos).
- Flags for keypair gen and sign/verify are set appropriately.
LGTM.Also applies to: 1044-1056, 1343-1353
4438-4443: Sign path: ML‑DSA integration via MLDSAUtilCKM_ML_DSA routed to AsymMech::MLDSA; key material loaded using MLDSAUtil. Parameter set is inferred from key length in MLDSA key classes, so no explicit setParameterSet is needed. Looks correct.
Also applies to: 4531-4550
5445-5450: Verify path: ML‑DSA integration via MLDSAUtilCKM_ML_DSA routed to AsymMech::MLDSA; MLDSAPublicKey populated via MLDSAUtil. Parameter set inference by key size keeps this path simple. LGTM.
Also applies to: 5538-5556
6171-6174: Key-pair generation: ML‑DSA routing and template checks
- CKM_ML_DSA_KEY_PAIR_GEN mapped to CKK_ML_DSA in both public/private template checks.
- generateMLDSA() invoked under WITH_ML_DSA.
Template consistency handling is aligned with other key types. LGTM.Also applies to: 6202-6204, 6228-6229, 6303-6311
10049-10076: CKA_PARAMETER_SET handling: length check and constants are correct
- Validates ulValueLen == sizeof(CK_ULONG) before reading.
- Uses CKP_ML_DSA_44/65/87 constants (consistent with project preference).
- Returns CKR_PARAMETER_SET_NOT_SUPPORTED for unsupported sets (as per current codebase semantics).
LGTM.
bukka
left a comment
bukka
left a comment
There was a problem hiding this comment.
It looks pretty good from the initial look. Just few things but I haven't checked to logic too deeply and haven't tested it either.
@jschlyter It would be great if the workflow could be enabled so pipeline can run.
antoinelochet
force-pushed
the
ml-dsa-pr
branch
from
c0ed069 to
71b03bc
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lib/SoftHSM.cpp (2)
7070-7092: If keeping non-PAD CBC wrap, add the matching unwrap pathCurrently only PAD cases are implemented. Add CKM_AES_CBC and CKM_DES3_CBC here with blocksize set and padding=false, plus IV length validation, to make wrap/unwrap symmetric.
Happy to provide a full patch if you choose Option B (support non-PAD).
Also applies to: 7116-7151
6799-6804: Align AES‑CBC wrap/unwrap: disallow CKM_AES_CBC (non‑PAD) or implement non‑PAD unwrapC_WrapKey currently accepts CKM_AES_CBC but UnwrapKeySym only implements CKM_AES_CBC_PAD — pick one approach:
- Option A (restrict wrap to PAD): change C_WrapKey to accept only CKM_AES_CBC_PAD (src/lib/SoftHSM.cpp around lines 6753–6804).
- Option B (implement non‑PAD unwrap): add CKM_AES_CBC (no‑pad) handling inside UnwrapKeySym (implementation starts ~7054; current cases at 7083/7118/7429).
Option A (recommended small-scope fix):
- case CKM_AES_CBC: - case CKM_AES_CBC_PAD: + case CKM_AES_CBC_PAD: if (pMechanism->pParameter == NULL_PTR || pMechanism->ulParameterLen != 16) return CKR_ARGUMENTS_BAD; break;
♻️ Duplicate comments (4)
src/lib/crypto/OSSLMLDSAPublicKey.h (1)
29-31: Polymorphic type check LGTM.The .cpp now returns true for both this class’s type and MLDSAPublicKey::type. This resolves the earlier concern.
src/lib/crypto/OSSLMLDSAPrivateKey.cpp (1)
119-133: PKCS#8 decode leak fixedEVP_PKEY is now freed after setFromOSSL(); good catch.
src/lib/crypto/OSSLMLDSA.h (1)
7-9: Include guard consistency: fixedGuard now matches; all good.
Also applies to: 54-54
src/lib/SoftHSM.cpp (1)
6481-6500: CBC wrap bug: zero-length IV used for AES_CBC/DES3_CBC (non-PAD)blocksize remains 0 for CKM_AES_CBC and CKM_DES3_CBC; you then copy a 0-byte IV and call encryptInit with an empty IV. This will fail or be incorrect. Please either set blocksize and validate IV length, or reject non-PAD modes.
Minimal fix:
- case CKM_AES_CBC: - algo = SymAlgo::AES; - break; + case CKM_AES_CBC: + algo = SymAlgo::AES; + blocksize = 16; + break; @@ - case CKM_DES3_CBC: - algo = SymAlgo::DES3; - break; + case CKM_DES3_CBC: + algo = SymAlgo::DES3; + blocksize = 8; + break; @@ - case CKM_AES_CBC: - case CKM_AES_CBC_PAD: - case CKM_DES3_CBC: - case CKM_DES3_CBC_PAD: - iv.resize(blocksize); - memcpy(&iv[0], pMechanism->pParameter, blocksize); + case CKM_AES_CBC: + case CKM_AES_CBC_PAD: + case CKM_DES3_CBC: + case CKM_DES3_CBC_PAD: + if (pMechanism->pParameter == NULL_PTR || + pMechanism->ulParameterLen != blocksize) { + cipher->recycleKey(wrappingkey); + CryptoFactory::i()->recycleSymmetricAlgorithm(cipher); + return CKR_ARGUMENTS_BAD; + } + iv.resize(blocksize); + memcpy(&iv[0], pMechanism->pParameter, blocksize);Optional: enforce wrappedlen % blocksize == 0 for non-PAD modes before encrypting.
Also applies to: 6524-6531
🧹 Nitpick comments (15)
src/lib/crypto/OSSLMLDSAPublicKey.h (5)
21-21: Parameter name consistency (+make ctor explicit).Align with the .cpp and avoid implicit conversions.
- OSSLMLDSAPublicKey(const EVP_PKEY* inMLDSAKEY); + explicit OSSLMLDSAPublicKey(const EVP_PKEY* inEVPPKEY);Mirror the rename in OSSLMLDSAPublicKey.cpp constructor signature. Also consider renaming the setFromOSSL parameter for consistency (see below).
29-33: Add override specifiers for safer vtable checks.Marking overrides helps catch signature drift at compile time.
- virtual bool isOfType(const char* inType); + virtual bool isOfType(const char* inType) override; - virtual void setValue(const ByteString& value); + virtual void setValue(const ByteString& value) override;
34-36: Rename param to match OpenSSL type; fix error message in .cpp.Minor consistency: use inEVPPKEY here, matching the type and the ctor.
Also, in OSSLMLDSAPublicKey.cpp inside setFromOSSL(), the error string says “private key private” while fetching OSSL_PKEY_PARAM_PUB_KEY; change to “public key”.
Header tweak:
- virtual void setFromOSSL(const EVP_PKEY* inMLDSAKEY); + virtual void setFromOSSL(const EVP_PKEY* inEVPPKEY);CPP tweak (not in this hunk):
- ERROR_MSG("Could not get private key private, rv: %d", rv); + ERROR_MSG("Could not get public key, rv: %d", rv);
13-13: Reduce header coupling: forward-declare EVP_PKEY.To limit transitive includes from a widely included header, forward-declare EVP_PKEY here and include <openssl/evp.h> in the .cpp.
-#include <openssl/evp.h> +// Forward declaration to decouple header from OpenSSL headers +typedef struct evp_pkey_st EVP_PKEY;Ensure OSSLMLDSAPublicKey.cpp retains
#include <openssl/evp.h>.
48-49: Fix include-guard trailing comment typo.The guard name includes “ML”. Update the trailing comment to match.
-#endif // !_SOFTHSM_V2_OSSLDSAPUBLICKEY_H +#endif // !_SOFTHSM_V2_OSSLMLDSAPUBLICKEY_HREADME.md (1)
85-85: Documented option looks good; fix hard tab to satisfy markdownlint (MD010).Replace the tab(s) on this new line with spaces, or fence the whole options block to silence the linter.
- --enable-mldsa Enable support for ML-DSA (default detect) + --enable-mldsa Enable support for ML-DSA (default detect)src/lib/crypto/OSSLMLDSAPrivateKey.h (2)
13-15: Trim header includes; forward-declare EVP_PKEY to reduce compile surface.-#include <openssl/bn.h> -#include <openssl/evp.h> +// Forward declaration; include <openssl/evp.h> in the .cpp +typedef struct evp_pkey_st EVP_PKEY;
58-58: Fix end-guard comment typo.-#endif // !_SOFTHSM_V2_OSSLECPRIVATEKEY_H +#endif // !_SOFTHSM_V2_OSSLMLDSAPRIVATEKEY_Hsrc/lib/crypto/OSSLUtil.cpp (1)
43-49: Optional: replace the map with a switch to drop and constexpr-initialize.-#ifdef WITH_ML_DSA -static const std::map<unsigned long, const char*> mldsaAlgNameFromParameterSet { - {MLDSAParameters::ML_DSA_44_PARAMETER_SET, "ML-DSA-44"}, - {MLDSAParameters::ML_DSA_65_PARAMETER_SET, "ML-DSA-65"}, - {MLDSAParameters::ML_DSA_87_PARAMETER_SET, "ML-DSA-87"} -}; -#endif +#ifdef WITH_ML_DSA +#include <openssl/core_names.h> +#endifAnd:
-const char* OSSL::mldsaParameterSet2Name(unsigned long parameterSet) { - - std::map<unsigned long, const char*>::const_iterator it = mldsaAlgNameFromParameterSet.find(parameterSet); - - if (it != mldsaAlgNameFromParameterSet.end()) { - return it->second; - } - - return NULL; -} +const char* OSSL::mldsaParameterSet2Name(unsigned long parameterSet) { + switch (parameterSet) { + case MLDSAParameters::ML_DSA_44_PARAMETER_SET: return "ML-DSA-44"; + case MLDSAParameters::ML_DSA_65_PARAMETER_SET: return "ML-DSA-65"; + case MLDSAParameters::ML_DSA_87_PARAMETER_SET: return "ML-DSA-87"; + default: return NULL; + } +}m4/acx_crypto_backend.m4 (1)
188-191: Handle --enable-mldsa=detect on Botan path explicitlyFor symmetry with ECC/EDDSA handling, set detect→no on Botan to make logs deterministic and avoid leaving "detect" dangling.
Apply:
- if test "x${enable_mldsa}" = "xyes"; then - AC_MSG_ERROR([Botan does not support ML-DSA]) - fi + case "${enable_mldsa}" in + yes) AC_MSG_ERROR([Botan does not support ML-DSA]);; + detect) enable_mldsa="no";; + esacsrc/lib/crypto/OSSLMLDSAPrivateKey.cpp (3)
23-29: Guard against NULL EVP_PKEY in ctor-from-OSSLDefensive check avoids accidental nullptr deref if misused.
OSSLMLDSAPrivateKey::OSSLMLDSAPrivateKey(const EVP_PKEY* inMLDSAKEY) { pkey = NULL; - setFromOSSL(inMLDSAKEY); + if (inMLDSAKEY == NULL) { + ERROR_MSG("NULL EVP_PKEY passed to OSSLMLDSAPrivateKey ctor"); + return; + } + setFromOSSL(inMLDSAKEY); }
46-54: Zeroise temporary key buffers after useWipe seed/priv stack buffers after copying to ByteString to reduce leakage surface.
#include "OSSLUtil.h" #include <openssl/bn.h> #include <openssl/core_names.h> +#include <openssl/crypto.h> #include <openssl/x509.h> @@ if(rv && seed_len == 32) { // seed is not mandatory for OSSL key reconstruction ByteString seedBS = ByteString(seed, seed_len); setSeed(seedBS); } + OPENSSL_cleanse(seed, sizeof(seed)); @@ ByteString valueBS = ByteString(priv, priv_len); setValue(valueBS); + OPENSSL_cleanse(priv, sizeof(priv));Also applies to: 56-69
59-65: Fix log message typo“private key private” → “private key value”.
- if(!rv) { - ERROR_MSG("Could not get private key private, rv: %d", rv); + if(!rv) { + ERROR_MSG("Could not get private key value, rv: %d", rv); return; }src/lib/crypto/OSSLMLDSA.h (1)
10-13: Gate header contents under WITH_ML_DSA to avoid OpenSSL header requirements in non-ML-DSA buildsPrevents accidental inclusion on Botan-only builds.
#ifndef _SOFTHSM_V2_OSSLMLDSA_H #define _SOFTHSM_V2_OSSLMLDSA_H #include "config.h" -#include "AsymmetricAlgorithm.h" -#include <openssl/evp.h> +#ifdef WITH_ML_DSA +#include "AsymmetricAlgorithm.h" +#include <openssl/evp.h> @@ private: }; -#endif // !_SOFTHSM_V2_OSSLMLDSA_H +#endif // WITH_ML_DSA +#endif // !_SOFTHSM_V2_OSSLMLDSA_HAlso applies to: 14-52
src/lib/SoftHSM.cpp (1)
10284-10286: Remove unused local variable in ML-DSA private-key blockByteString parameterSet is unused; trim to avoid warnings.
- // MLDSA Private Key Attributes - ByteString parameterSet; - ByteString value; - ByteString seed; + // MLDSA Private Key Attributes + ByteString value; + ByteString seed;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (44)
.github/workflows/ci.yml(1 hunks)CMAKE-NOTES.md(1 hunks)CMAKE-WIN-NOTES.md(1 hunks)CMakeLists.txt(1 hunks)README.md(1 hunks)config.h.in.cmake(1 hunks)m4/acx_crypto_backend.m4(4 hunks)m4/acx_openssl_mldsa.m4(1 hunks)src/bin/dump/tables.h(3 hunks)src/lib/P11Attributes.cpp(2 hunks)src/lib/P11Attributes.h(2 hunks)src/lib/P11Objects.cpp(2 hunks)src/lib/P11Objects.h(2 hunks)src/lib/SoftHSM.cpp(34 hunks)src/lib/SoftHSM.h(2 hunks)src/lib/crypto/AsymmetricAlgorithm.h(2 hunks)src/lib/crypto/CMakeLists.txt(2 hunks)src/lib/crypto/MLDSAParameters.cpp(1 hunks)src/lib/crypto/MLDSAParameters.h(1 hunks)src/lib/crypto/MLDSAPrivateKey.cpp(1 hunks)src/lib/crypto/MLDSAPrivateKey.h(1 hunks)src/lib/crypto/MLDSAPublicKey.cpp(1 hunks)src/lib/crypto/MLDSAPublicKey.h(1 hunks)src/lib/crypto/MLDSAUtil.cpp(1 hunks)src/lib/crypto/MLDSAUtil.h(1 hunks)src/lib/crypto/Makefile.am(3 hunks)src/lib/crypto/OSSLComp.cpp(1 hunks)src/lib/crypto/OSSLCryptoFactory.cpp(2 hunks)src/lib/crypto/OSSLMLDSA.cpp(1 hunks)src/lib/crypto/OSSLMLDSA.h(1 hunks)src/lib/crypto/OSSLMLDSAKeyPair.cpp(1 hunks)src/lib/crypto/OSSLMLDSAKeyPair.h(1 hunks)src/lib/crypto/OSSLMLDSAPrivateKey.cpp(1 hunks)src/lib/crypto/OSSLMLDSAPrivateKey.h(1 hunks)src/lib/crypto/OSSLMLDSAPublicKey.cpp(1 hunks)src/lib/crypto/OSSLMLDSAPublicKey.h(1 hunks)src/lib/crypto/OSSLUtil.cpp(2 hunks)src/lib/crypto/OSSLUtil.h(1 hunks)src/lib/crypto/test/CMakeLists.txt(1 hunks)src/lib/crypto/test/MLDSATests.cpp(1 hunks)src/lib/crypto/test/MLDSATests.h(1 hunks)src/lib/crypto/test/Makefile.am(1 hunks)src/lib/object_store/DBObject.cpp(1 hunks)src/lib/pkcs11/pkcs11.h(5 hunks)
✅ Files skipped from review due to trivial changes (3)
- CMAKE-NOTES.md
- CMAKE-WIN-NOTES.md
- src/lib/crypto/OSSLComp.cpp
🚧 Files skipped from review as they are similar to previous changes (31)
- src/lib/crypto/OSSLUtil.h
- src/lib/crypto/test/CMakeLists.txt
- CMakeLists.txt
- src/lib/crypto/MLDSAPrivateKey.h
- m4/acx_openssl_mldsa.m4
- src/lib/crypto/MLDSAPublicKey.h
- src/lib/crypto/test/MLDSATests.cpp
- src/lib/crypto/test/MLDSATests.h
- src/lib/crypto/test/Makefile.am
- src/lib/crypto/MLDSAParameters.h
- src/lib/crypto/MLDSAParameters.cpp
- src/lib/crypto/MLDSAPrivateKey.cpp
- src/lib/crypto/OSSLCryptoFactory.cpp
- src/bin/dump/tables.h
- src/lib/crypto/CMakeLists.txt
- src/lib/crypto/AsymmetricAlgorithm.h
- src/lib/P11Attributes.h
- src/lib/crypto/MLDSAPublicKey.cpp
- src/lib/P11Objects.h
- src/lib/crypto/Makefile.am
- src/lib/P11Objects.cpp
- src/lib/crypto/OSSLMLDSA.cpp
- src/lib/crypto/OSSLMLDSAKeyPair.h
- src/lib/P11Attributes.cpp
- src/lib/crypto/OSSLMLDSAKeyPair.cpp
- config.h.in.cmake
- src/lib/crypto/OSSLMLDSAPublicKey.cpp
- src/lib/crypto/MLDSAUtil.h
- .github/workflows/ci.yml
- src/lib/crypto/MLDSAUtil.cpp
- src/lib/pkcs11/pkcs11.h
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: bukka
PR: softhsm/SoftHSMv2#815
File: src/lib/SoftHSM.cpp:6543-6546
Timestamp: 2025-09-11T16:54:00.347Z
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
PR: softhsm/SoftHSMv2#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
PR: softhsm/SoftHSMv2#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-08-19T09:23:25.434Z
Learnt from: antoinelochet
PR: softhsm/SoftHSMv2#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/OSSLMLDSAPublicKey.hsrc/lib/crypto/OSSLMLDSAPrivateKey.cppsrc/lib/crypto/OSSLMLDSA.hsrc/lib/crypto/OSSLMLDSAPrivateKey.hsrc/lib/crypto/OSSLUtil.cppsrc/lib/SoftHSM.cppsrc/lib/SoftHSM.h
📚 Learning: 2025-08-18T17:59:30.693Z
Learnt from: antoinelochet
PR: softhsm/SoftHSMv2#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/OSSLMLDSAPublicKey.hsrc/lib/crypto/OSSLMLDSAPrivateKey.cppsrc/lib/crypto/OSSLMLDSA.hsrc/lib/crypto/OSSLMLDSAPrivateKey.hsrc/lib/crypto/OSSLUtil.cppsrc/lib/SoftHSM.cppsrc/lib/SoftHSM.h
📚 Learning: 2025-08-18T19:46:46.659Z
Learnt from: antoinelochet
PR: softhsm/SoftHSMv2#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/OSSLMLDSAPrivateKey.cppsrc/lib/crypto/OSSLUtil.cppsrc/lib/SoftHSM.cppsrc/lib/SoftHSM.h
📚 Learning: 2025-09-11T16:54:00.347Z
Learnt from: bukka
PR: softhsm/SoftHSMv2#815
File: src/lib/SoftHSM.cpp:6543-6546
Timestamp: 2025-09-11T16:54:00.347Z
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/OSSLMLDSAPrivateKey.cppsrc/lib/SoftHSM.cppsrc/lib/SoftHSM.h
📚 Learning: 2025-08-18T17:54:37.645Z
Learnt from: antoinelochet
PR: softhsm/SoftHSMv2#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
🧬 Code graph analysis (6)
src/lib/crypto/OSSLMLDSAPublicKey.h (3)
src/lib/crypto/OSSLMLDSAPublicKey.cpp (13)
OSSLMLDSAPublicKey(18-21)OSSLMLDSAPublicKey(23-28)OSSLMLDSAPublicKey(31-38)isOfType(63-66)isOfType(63-63)setValue(68-76)setValue(68-68)setFromOSSL(44-60)setFromOSSL(44-44)getOSSLKey(79-84)getOSSLKey(79-79)createOSSLKey(87-125)createOSSLKey(87-87)src/lib/crypto/MLDSAPublicKey.h (1)
MLDSAPublicKey(14-45)src/lib/crypto/MLDSAPublicKey.cpp (4)
isOfType(17-20)isOfType(17-17)setValue(60-63)setValue(60-60)
src/lib/crypto/OSSLMLDSAPrivateKey.cpp (4)
src/lib/crypto/OSSLMLDSAPrivateKey.h (1)
OSSLMLDSAPrivateKey(16-56)src/lib/crypto/OSSLMLDSAPublicKey.cpp (10)
setFromOSSL(44-60)setFromOSSL(44-44)setValue(68-76)setValue(68-68)isOfType(63-66)isOfType(63-63)getOSSLKey(79-84)getOSSLKey(79-79)createOSSLKey(87-125)createOSSLKey(87-87)src/lib/crypto/MLDSAPrivateKey.cpp (10)
setSeed(65-68)setSeed(65-65)setValue(55-58)setValue(55-55)isOfType(17-20)isOfType(17-17)getValue(60-63)getValue(60-60)getParameterSet(28-39)getParameterSet(28-28)src/lib/crypto/OSSLUtil.cpp (2)
mldsaParameterSet2Name(224-233)mldsaParameterSet2Name(224-224)
src/lib/crypto/OSSLMLDSA.h (2)
src/lib/crypto/AsymmetricAlgorithm.h (2)
AsymmetricAlgorithm(121-190)AsymmetricAlgorithm(128-128)src/lib/crypto/OSSLMLDSA.cpp (40)
sign(24-97)sign(24-26)signInit(99-105)signInit(99-100)signUpdate(107-112)signUpdate(107-107)signFinal(114-119)signFinal(114-114)verify(122-222)verify(122-124)verifyInit(224-230)verifyInit(224-225)verifyUpdate(232-237)verifyUpdate(232-232)verifyFinal(239-244)verifyFinal(239-239)encrypt(247-253)encrypt(247-248)decrypt(256-262)decrypt(256-257)getMinKeySize(264-267)getMinKeySize(264-264)getMaxKeySize(269-272)getMaxKeySize(269-269)generateKeyPair(282-345)generateKeyPair(282-282)reconstructKeyPair(347-383)reconstructKeyPair(347-347)reconstructPublicKey(385-406)reconstructPublicKey(385-385)reconstructPrivateKey(408-429)reconstructPrivateKey(408-408)reconstructParameters(446-466)reconstructParameters(446-446)newPublicKey(431-434)newPublicKey(431-431)newPrivateKey(436-439)newPrivateKey(436-436)newParameters(441-444)newParameters(441-441)
src/lib/crypto/OSSLMLDSAPrivateKey.h (3)
src/lib/crypto/OSSLMLDSAPrivateKey.cpp (19)
OSSLMLDSAPrivateKey(18-21)OSSLMLDSAPrivateKey(23-28)OSSLMLDSAPrivateKey(31-38)isOfType(72-75)isOfType(72-72)setValue(77-85)setValue(77-77)setSeed(87-95)setSeed(87-87)PKCS8Encode(98-117)PKCS8Encode(98-98)PKCS8Decode(120-133)PKCS8Decode(120-120)setFromOSSL(44-69)setFromOSSL(44-44)getOSSLKey(136-141)getOSSLKey(136-136)createOSSLKey(144-192)createOSSLKey(144-144)src/lib/crypto/MLDSAPrivateKey.h (1)
MLDSAPrivateKey(13-47)src/lib/crypto/MLDSAPrivateKey.cpp (6)
isOfType(17-20)isOfType(17-17)setValue(55-58)setValue(55-55)setSeed(65-68)setSeed(65-65)
src/lib/SoftHSM.cpp (1)
src/lib/crypto/MLDSAUtil.cpp (6)
getMLDSAPrivateKey(11-41)getMLDSAPrivateKey(11-11)getMLDSAPublicKey(43-69)getMLDSAPublicKey(43-43)setMLDSAPrivateKey(71-111)setMLDSAPrivateKey(71-71)
src/lib/SoftHSM.h (1)
src/lib/SoftHSM.cpp (2)
generateMLDSA(10083-10339)generateMLDSA(10083-10094)
🪛 markdownlint-cli2 (0.17.2)
README.md
85-85: Hard tabs
Column: 1
(MD010, no-hard-tabs)
85-85: Hard tabs
Column: 16
(MD010, no-hard-tabs)
🔇 Additional comments (17)
src/lib/SoftHSM.h (2)
55-58: Good: ML-DSA headers are correctly feature-gated.
378-393: Good: generateMLDSA() is declared under WITH_ML_DSA, matching the implementation.src/lib/crypto/OSSLUtil.cpp (1)
223-234: Declaration exists and is correctly guarded.
Header src/lib/crypto/OSSLUtil.h already declaresconst char* mldsaParameterSet2Name(unsigned long)inside#ifdef WITH_ML_DSA(lines ~76–78).m4/acx_crypto_backend.m4 (3)
31-39: ML-DSA configure flag: LGTMOption wiring and help text look consistent with ECC/EDDSA.
257-269: WITH_ML_DSA define/conditional: LGTMConsistent with existing feature toggles; no issues.
113-121: Resolve: ACX_OPENSSL_MLDSA uses capability-based detectionConfirmed — the macro performs a functional check calling EVP_PKEY_CTX_new_from_name("ML-DSA-44") in m4/acx_openssl_mldsa.m4; no version-gate change required.
src/lib/crypto/OSSLMLDSAPrivateKey.cpp (1)
150-160: Early guards on param-set/name and empty key: LGTMPrevents undefined provider behavior on bad inputs.
src/lib/crypto/OSSLMLDSA.h (1)
32-39: Resolved — ML-DSA encrypt/decrypt/checkEncryptedDataSize are already stubbedImplementations in src/lib/crypto/OSSLMLDSA.cpp (≈ lines 247–276) log "ML-DSA does not support encryption" and act as stubs — no change required.
src/lib/SoftHSM.cpp (9)
65-70: ML-DSA includes and guard look goodHeaders are properly gated under WITH_ML_DSA.
149-152: Correct feature-gating for ML-DSA P11 objectsnewP11Object branches are properly wrapped; avoids non-ML builds breaking.
Also applies to: 171-173
842-846: Mechanism registration: OKCKM_ML_DSA and KEY_PAIR_GEN added as expected.
950-953: C_GetMechanismInfo wiring for ML-DSA: OKMin/max are pulled from CryptoFactory and flags are set appropriately.
Also applies to: 1049-1061, 1353-1363
4204-4205: Sign init path for ML-DSA: OKUsing MLDSAUtil and relying on parameter-set inference is consistent with project preference.
Also applies to: 4473-4478, 4566-4584
5238-5239: Verify init path for ML-DSA: OKMirrors sign path; no explicit setParameterSet required.
Also applies to: 5505-5510, 5598-5616
6231-6234: Key-pair gen plumbing for ML-DSA: OKMechanism switch, template checks, and dispatch to generateMLDSA are sound.
Also applies to: 6262-6264, 6288-6289, 6362-6371
10099-10121: generateMLDSA: robust template parsing and validationCKA_PARAMETER_SET length checked, CKP_ML_DSA_* constants used, and “unsupported” path logged consistently.
Also applies to: 10133-10136
1152-1152: Confirm CKM_GENERIC_SECRET_KEY_GEN max size changeulMaxKeySize = 0x08000000 (134,217,728) — intentional? It differs from other “unbounded” caps used elsewhere.
|
Very cool. Did some manual test. I checked out this branch and tested with our p11ng-cli from EJBCA (Java code using Bouncy Castle). Generating keys, listing keys, signing (and verifying the signatures with Bouncy Castle code). I have PKCS#11v3.2 draft implemented and it worked out-of-the-box. Works well with all variants ML-DSA-44, ML-DSA-65 and ML-DSA-87. |
Thank you very much ! |
|
Nice work! I'll ask @bjosv to review before merge. |
jschlyter
left a comment
jschlyter
left a comment
There was a problem hiding this comment.
Please address and resolve all comments. If you believe the rabbit is out of line, do tell it.
|
Not sure if it's helpful, but have an implementation of it working here: |
All resolved :) |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/lib/crypto/OSSLMLDSAPublicKey.cpp(1 hunks)src/lib/crypto/OSSLMLDSAPublicKey.h(1 hunks)src/lib/crypto/OSSLUtil.cpp(2 hunks)src/lib/object_store/DBObject.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/object_store/DBObject.cpp
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-19T09:23:25.434Z
Learnt from: antoinelochet
PR: softhsm/SoftHSMv2#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/OSSLUtil.cppsrc/lib/crypto/OSSLMLDSAPublicKey.hsrc/lib/crypto/OSSLMLDSAPublicKey.cpp
📚 Learning: 2025-08-18T17:59:30.693Z
Learnt from: antoinelochet
PR: softhsm/SoftHSMv2#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.cpp
📚 Learning: 2025-08-18T19:46:46.659Z
Learnt from: antoinelochet
PR: softhsm/SoftHSMv2#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/OSSLUtil.cpp
🧬 Code graph analysis (2)
src/lib/crypto/OSSLMLDSAPublicKey.h (3)
src/lib/crypto/OSSLMLDSAPublicKey.cpp (14)
OSSLMLDSAPublicKey(18-21)OSSLMLDSAPublicKey(23-28)OSSLMLDSAPublicKey(31-38)OSSLMLDSAPublicKey(40-44)isOfType(84-87)isOfType(84-84)setValue(89-97)setValue(89-89)setFromOSSL(65-81)setFromOSSL(65-65)getOSSLKey(100-105)getOSSLKey(100-100)createOSSLKey(108-146)createOSSLKey(108-108)src/lib/crypto/MLDSAPublicKey.h (1)
MLDSAPublicKey(14-45)src/lib/crypto/MLDSAPublicKey.cpp (4)
isOfType(17-20)isOfType(17-17)setValue(60-63)setValue(60-60)
src/lib/crypto/OSSLMLDSAPublicKey.cpp (4)
src/lib/crypto/OSSLMLDSAPublicKey.h (1)
OSSLMLDSAPublicKey(15-54)src/lib/crypto/OSSLMLDSAPrivateKey.cpp (10)
setFromOSSL(44-69)setFromOSSL(44-44)setValue(77-85)setValue(77-77)isOfType(72-75)isOfType(72-72)getOSSLKey(136-141)getOSSLKey(136-136)createOSSLKey(144-192)createOSSLKey(144-144)src/lib/crypto/MLDSAPublicKey.cpp (8)
setValue(60-63)setValue(60-60)isOfType(17-20)isOfType(17-17)getValue(55-58)getValue(55-55)getParameterSet(28-39)getParameterSet(28-28)src/lib/crypto/OSSLUtil.cpp (2)
mldsaParameterSet2Name(226-235)mldsaParameterSet2Name(226-226)
|
I found an issue. It does not handle signing with a provided ML-DSA context. I.e. PKCS11v3.2 section 6.67.5.
Referring to section 6.67.1: This is needed for Composite certificates as a domain separator is used as specified in composite draft 12 section 7. |
I think there is (was?) an issue with OpenSSL. I will have to check. |
antoinelochet
force-pushed
the
ml-dsa-pr
branch
from
e8e987f to
cdac7b0
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lib/SoftHSM.cpp (2)
6519-6571: Fix UB + missing IV handling in WrapKeySym() for CKM_AES_CBC / CKM_DES3_CBC.
blocksizeis only set for*_CBC_PADcases, but the CBC encrypt path unconditionally does:
iv.resize(blocksize);memcpy(&iv[0], ..., blocksize);If
blocksize == 0(e.g.,CKM_AES_CBCorCKM_DES3_CBC),&iv[0]is undefined behavior, and the IV won’t be provided to the cipher.Proposed fix
switch(pMechanism->mechanism) { #ifdef HAVE_AES_KEY_WRAP case CKM_AES_KEY_WRAP: wrappedlen = RFC3394Pad(keydata); if ((wrappedlen < 16) || ((wrappedlen % 8) != 0)) return CKR_KEY_SIZE_RANGE; algo = SymAlgo::AES; mode = SymWrap::AES_KEYWRAP; break; #endif #ifdef HAVE_AES_KEY_WRAP_PAD case CKM_AES_KEY_WRAP_PAD: algo = SymAlgo::AES; mode = SymWrap::AES_KEYWRAP_PAD; break; #endif case CKM_AES_CBC: algo = SymAlgo::AES; + blocksize = 16; break; case CKM_AES_CBC_PAD: blocksize = 16; wrappedlen = RFC5652Pad(keydata, blocksize); algo = SymAlgo::AES; break; case CKM_DES3_CBC: algo = SymAlgo::DES3; + blocksize = 8; break; case CKM_DES3_CBC_PAD: blocksize = 8; wrappedlen = RFC5652Pad(keydata, blocksize); algo = SymAlgo::DES3; break; default: return CKR_MECHANISM_INVALID; } @@ switch(pMechanism->mechanism) { case CKM_AES_CBC: case CKM_AES_CBC_PAD: case CKM_DES3_CBC: case CKM_DES3_CBC_PAD: + if (pMechanism->pParameter == NULL_PTR || pMechanism->ulParameterLen != blocksize) + return CKR_ARGUMENTS_BAD; iv.resize(blocksize); memcpy(&iv[0], pMechanism->pParameter, blocksize); if (!cipher->encryptInit(wrappingkey, SymMode::CBC, iv, false))Also, if
CKM_DES3_CBC(_PAD)wrapping is intended to be supported,C_WrapKey’s mechanism allowlist should include it (currently only AES_CBC(_PAD) is accepted).
949-1059: Add comment clarifying ML-DSA key sizes are in bytes, not bits.The implementation correctly returns ML-DSA key sizes in bytes (2560, 4032, 4896 for ML-DSA-44/65/87) as required by PKCS#11 v3.2 §6.67.4, as documented in MLDSAParameters.h. However, lines 949–1059 in SoftHSM.cpp lack an explicit comment distinguishing this from other algorithms (RSA, DSA, ECDSA, ECDH, EdDSA) which use bits. Add a brief comment above the ML-DSA section to document that these values are bytes per PKCS#11 v3.2, preventing accidental regressions.
This also applies to lines 1351–1361.
🤖 Fix all issues with AI agents
In @src/lib/crypto/MLDSAUtil.cpp:
- Around line 173-188: The code allocates a single contiguous blob into mem but
then separately mallocs sac->contextData and sets ownsContextMemory=false,
leaking the second allocation; instead, set sac->contextData to point into the
contiguous blob (offset after the SIGN_ADDITIONAL_CONTEXT structure) and copy
contextAsChar there, remove the extra malloc so ownsContextMemory remains false
and only the outer mem (sac) must be freed; update the allocation to use total =
sizeof(SIGN_ADDITIONAL_CONTEXT) + contextLength, assign sac =
reinterpret_cast<SIGN_ADDITIONAL_CONTEXT*>(mem), set sac->contextData = mem +
sizeof(SIGN_ADDITIONAL_CONTEXT) when contextLength>0, then memcpy from
contextAsChar into sac->contextData and do not call malloc for contextData.
In @src/lib/SoftHSM.cpp:
- Around line 5252-5258: In AsymVerifyInit, add a strict validation for the
CKM_ML_DSA branch to ensure pMechanism->pParameter and
pMechanism->ulParameterLen are consistent: require (pMechanism->pParameter ==
NULL_PTR) if and only if (pMechanism->ulParameterLen == 0); if this invariant
fails, return CKR_ARGUMENTS_BAD. Locate the CKM_ML_DSA handling in
AsymVerifyInit and perform this check before any use of the parameter (and
before constructing or calling MLDSAAdditionalContextGuard/setParameters()),
keeping the existing path for non-NULL parameters unchanged.
🧹 Nitpick comments (5)
src/lib/crypto/AsymmetricAlgorithm.h (1)
133-162: Conditional allocation in constructor silently degrades on OOM.When
std::mallocfails in the convenience constructor (Line 148-150),contextLenis set to 0 andownsContextMemoryto false, silently discarding the requested context. The caller has no way to detect this failure.Consider throwing
std::bad_allocor providing a factory method that returns an error code, consistent with howcopyMLDSAContextreturnsCKR_HOST_MEMORY.src/lib/crypto/MLDSAUtil.cpp (1)
141-156: Consider validatingulContextLen == 0with non-nullpContext.The code only validates that
pContext != NULLwhenulContextLen > 0. While not strictly an error, it may be worth logging a debug message if a caller passespContext != NULLwithulContextLen == 0, as this could indicate a caller bug.src/lib/SoftHSM.cpp (2)
5770-5774: Extra debug on signature length mismatch is OK, but ensure it’s not too noisy in typical failure cases.Logging
sizeandulSignatureLenbehindDEBUG_MSGis generally fine; just worth confirming it doesn’t spam logs in common negative-test flows.Also applies to: 5954-5958
10116-10374: generateMLDSA(): avoid usingparamSet==0as “missing” sentinel; track presence explicitly.Relying on
paramSet == 0to mean “not provided” is brittle (even if today’s constants are non-zero). Prefer abool hasParamSetgate and keepparamSetpurely as a value.Proposed refactor
- CK_ULONG paramSet = 0; + CK_ULONG paramSet = 0; + bool hasParamSet = false; for (CK_ULONG i = 0; i < ulPublicKeyAttributeCount; i++) { switch (pPublicKeyTemplate[i].type) { case CKA_PARAMETER_SET: if (pPublicKeyTemplate[i].ulValueLen != sizeof(CK_ULONG)) { INFO_MSG("CKA_PARAMETER_SET must be sizeof(CK_ULONG)"); return CKR_ATTRIBUTE_VALUE_INVALID; } paramSet = *(CK_ULONG*)pPublicKeyTemplate[i].pValue; + hasParamSet = true; break; default: break; } } // The parameters must be specified to be able to generate a key pair. - if (paramSet == 0) { + if (!hasParamSet) { INFO_MSG("Missing parameter(s) CKA_PARAMETER_SET in pPublicKeyTemplate"); return CKR_TEMPLATE_INCOMPLETE; }Based on learnings, using
CKP_ML_DSA_*constants is preferred (and you already do).src/lib/crypto/OSSLMLDSA.cpp (1)
83-83: Consider removing or guarding sensitive context logging.This
INFO_MSGlogs the ML-DSA context bytes (which may include domain separators for composite certificates). While INFO-level logs are typically debug-only, logging raw context data could expose application-specific sensitive information in debug scenarios.Consider removing this log or using a lower log level (DEBUG_MSG) if the project has one.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/lib/SoftHSM.cppsrc/lib/crypto/AsymmetricAlgorithm.hsrc/lib/crypto/MLDSAPrivateKey.cppsrc/lib/crypto/MLDSAUtil.cppsrc/lib/crypto/MLDSAUtil.hsrc/lib/crypto/OSSLMLDSA.cpp
🧰 Additional context used
🧠 Learnings (13)
📓 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: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: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/crypto/OSSLMLDSA.cpp:373-381
Timestamp: 2026-01-11T16:15:14.696Z
Learning: In PKCS#11 v3.2, ML-DSA key sizes (returned by getMinKeySize() and getMaxKeySize()) are specified in bytes, not bits. This is different from other algorithms like RSA, DSA, ECDSA, ECDH, and EdDSA which use bits. This is defined in chapter 6.67.4 of the PKCS#11 3.2 specification.
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/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-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/AsymmetricAlgorithm.hsrc/lib/crypto/MLDSAUtil.cppsrc/lib/SoftHSM.cppsrc/lib/crypto/OSSLMLDSA.cppsrc/lib/crypto/MLDSAUtil.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/AsymmetricAlgorithm.hsrc/lib/crypto/MLDSAUtil.cppsrc/lib/crypto/MLDSAPrivateKey.cppsrc/lib/SoftHSM.cppsrc/lib/crypto/OSSLMLDSA.cppsrc/lib/crypto/MLDSAUtil.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/crypto/AsymmetricAlgorithm.hsrc/lib/crypto/MLDSAUtil.cppsrc/lib/crypto/MLDSAPrivateKey.cppsrc/lib/SoftHSM.cppsrc/lib/crypto/OSSLMLDSA.cppsrc/lib/crypto/MLDSAUtil.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/AsymmetricAlgorithm.hsrc/lib/crypto/MLDSAUtil.cppsrc/lib/SoftHSM.cppsrc/lib/crypto/OSSLMLDSA.cppsrc/lib/crypto/MLDSAUtil.h
📚 Learning: 2025-11-11T19:42:46.886Z
Learnt from: JoaoHenrique12
Repo: softhsm/SoftHSMv2 PR: 823
File: src/lib/SoftHSM.cpp:58-58
Timestamp: 2025-11-11T19:42:46.886Z
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/AsymmetricAlgorithm.hsrc/lib/SoftHSM.cppsrc/lib/crypto/OSSLMLDSA.cppsrc/lib/crypto/MLDSAUtil.h
📚 Learning: 2026-01-11T16:15:14.696Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/crypto/OSSLMLDSA.cpp:373-381
Timestamp: 2026-01-11T16:15:14.696Z
Learning: In PKCS#11 v3.2, ML-DSA key sizes (returned by getMinKeySize() and getMaxKeySize()) are specified in bytes, not bits. This is different from other algorithms like RSA, DSA, ECDSA, ECDH, and EdDSA which use bits. This is defined in chapter 6.67.4 of the PKCS#11 3.2 specification.
Applied to files:
src/lib/crypto/AsymmetricAlgorithm.hsrc/lib/crypto/MLDSAUtil.cppsrc/lib/crypto/MLDSAPrivateKey.cppsrc/lib/SoftHSM.cppsrc/lib/crypto/OSSLMLDSA.cppsrc/lib/crypto/MLDSAUtil.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:
src/lib/crypto/AsymmetricAlgorithm.hsrc/lib/crypto/MLDSAUtil.cppsrc/lib/crypto/MLDSAPrivateKey.cppsrc/lib/SoftHSM.cppsrc/lib/crypto/OSSLMLDSA.cppsrc/lib/crypto/MLDSAUtil.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/MLDSAUtil.cppsrc/lib/SoftHSM.cppsrc/lib/crypto/OSSLMLDSA.cppsrc/lib/crypto/MLDSAUtil.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/SoftHSM.cpp
📚 Learning: 2026-01-10T15:07:10.701Z
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 837
File: src/lib/common/log.cpp:192-194
Timestamp: 2026-01-10T15:07:10.701Z
Learning: In SoftHSM, setLogFile and closeLogFile are designed to be called only during C_Initialize and C_Finalize respectively. Per PKCS#11 spec, C_Finalize behavior is undefined if called while other threads are making Cryptoki calls, so concurrent access between closeLogFile and softHSMLog would be API misuse. The implementation relies on these PKCS#11 threading guarantees rather than defensive locking in open/close operations.
Applied to files:
src/lib/SoftHSM.cpp
📚 Learning: 2025-08-18T19:35:33.938Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/crypto/OSSLMLDSA.cpp:195-204
Timestamp: 2025-08-18T19:35:33.938Z
Learning: In OpenSSL, EVP_PKEY_verify_message does not exist. EVP_PKEY_verify_message_init is designed for streaming verification and should be paired with EVP_PKEY_CTX_set_signature, EVP_PKEY_verify_message_update, and EVP_PKEY_verify_message_final - not with EVP_PKEY_verify.
Applied to files:
src/lib/crypto/OSSLMLDSA.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/OSSLMLDSA.cpp
🧬 Code graph analysis (6)
src/lib/crypto/AsymmetricAlgorithm.h (1)
src/lib/test/TestsNoPINInitBase.cpp (1)
ptr(60-60)
src/lib/crypto/MLDSAUtil.cpp (1)
src/lib/SoftHSM.cpp (2)
i(395-416)i(395-395)
src/lib/crypto/MLDSAPrivateKey.cpp (3)
src/lib/crypto/MLDSAPublicKey.cpp (12)
isOfType(17-20)isOfType(17-17)getParameterSet(28-39)getParameterSet(28-28)getOutputLength(42-53)getOutputLength(42-42)setValue(60-63)setValue(60-60)serialise(66-69)serialise(66-66)deserialise(71-83)deserialise(71-71)src/lib/crypto/OSSLMLDSAPrivateKey.cpp (6)
isOfType(90-93)isOfType(90-90)setValue(95-103)setValue(95-95)setSeed(105-113)setSeed(105-105)src/lib/crypto/MLDSAParameters.cpp (6)
getParameterSet(22-25)getParameterSet(22-22)serialise(34-37)serialise(34-34)deserialise(39-50)deserialise(39-39)
src/lib/SoftHSM.cpp (1)
src/lib/crypto/MLDSAUtil.cpp (8)
copyMLDSAContext(130-191)copyMLDSAContext(130-130)getMLDSAPrivateKey(11-41)getMLDSAPrivateKey(11-11)getMLDSAPublicKey(43-69)getMLDSAPublicKey(43-43)setMLDSAPrivateKey(71-128)setMLDSAPrivateKey(71-71)
src/lib/crypto/OSSLMLDSA.cpp (1)
src/lib/crypto/OSSLUtil.cpp (2)
mldsaParameterSet2Name(226-235)mldsaParameterSet2Name(226-226)
src/lib/crypto/MLDSAUtil.h (1)
src/lib/crypto/MLDSAUtil.cpp (8)
getMLDSAPrivateKey(11-41)getMLDSAPrivateKey(11-11)getMLDSAPublicKey(43-69)getMLDSAPublicKey(43-43)setMLDSAPrivateKey(71-128)setMLDSAPrivateKey(71-71)copyMLDSAContext(130-191)copyMLDSAContext(130-130)
🪛 Clang (14.0.6)
src/lib/crypto/AsymmetricAlgorithm.h
[error] 36-36: 'cstdlib' file not found
(clang-diagnostic-error)
src/lib/crypto/MLDSAUtil.cpp
[error] 7-7: 'config.h' file not found
(clang-diagnostic-error)
src/lib/crypto/MLDSAPrivateKey.cpp
[error] 7-7: 'config.h' file not found
(clang-diagnostic-error)
src/lib/crypto/OSSLMLDSA.cpp
[error] 7-7: 'config.h' file not found
(clang-diagnostic-error)
src/lib/crypto/MLDSAUtil.h
[error] 10-10: 'config.h' file not found
(clang-diagnostic-error)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Linux with OpenSSL 1.1.1
- GitHub Check: Windows (x64, botan)
- GitHub Check: Linux with OpenSSL 3.5.4
- GitHub Check: macOS (botan)
- GitHub Check: Windows (x86, openssl)
- GitHub Check: macOS (openssl)
- GitHub Check: Windows (x64, openssl)
- GitHub Check: Linux with OpenSSL 3.0
- GitHub Check: Linux with Botan
🔇 Additional comments (27)
src/lib/crypto/AsymmetricAlgorithm.h (3)
36-37: LGTM!Standard C library headers included for
malloc/free/memcpyused by the new memory-managed context structures.
58-59: LGTM!MLDSA type additions to
AsymAlgoandAsymMechenums correctly extend the existing algorithm enumeration pattern for ML-DSA support.Also applies to: 98-99
123-131: LGTM!The
Hedgeenum correctly maps to the PKCS#11 v3.2CK_HEDGE_TYPEvariants (CKH_HEDGE_PREFERRED,CKH_HEDGE_REQUIRED,CKH_DETERMINISTIC_REQUIRED).src/lib/crypto/MLDSAUtil.h (1)
1-32: LGTM!Well-structured utility class declaration with proper include guards and
WITH_ML_DSAfeature gating. The static methods provide a clean API for ML-DSA key material extraction and context copying, consistent with similar utility patterns in the codebase (e.g., EdDSA utilities).src/lib/crypto/MLDSAUtil.cpp (3)
11-41: LGTM!Proper null-pointer validation, conditional decryption based on
CKA_PRIVATEattribute, and consistent error handling pattern matching other key utility implementations.
43-69: LGTM!Implementation mirrors
getMLDSAPrivateKeyappropriately for public key retrieval with proper token decryption handling.
71-128: LGTM!Proper resource cleanup on all error paths via
recyclePrivateKeyandrecycleAsymmetricAlgorithm. The PKCS#8 decode, encryption, and attribute setting flow is consistent with other key utility patterns.src/lib/crypto/MLDSAPrivateKey.cpp (4)
13-24: LGTM!Standard type identification pattern consistent with other key classes in the codebase.
26-57: LGTM!Parameter set and output length inference from key material size follows the established design pattern. Based on learnings, this inference approach (
getParameterSet()mapping key length toCKP_ML_DSA_44/65/87) is intentional, and runtime keys rely on this without explicit parameter-set assignment.
59-77: LGTM!Standard accessor/mutator implementations for
valueandseedmembers.
79-100: LGTM!Serialization concatenates
seedthenvalue; deserialization correctly useschainDeserialisein the same order and validates both fields are non-empty before assignment. This mirrors the pattern used inMLDSAPublicKey.src/lib/SoftHSM.cpp (7)
65-68: ML-DSA includes look consistent with the project’s include policy.Not wrapping algorithm headers in
WITH_*guards matches prior guidance (avoids include-level feature gating). Based on learnings, keep an eye on ensuring these headers are always present in non-ML-DSA builds (so non-WITH_ML_DSAbuilds still compile cleanly).
148-172: Correct object instantiation for CKK_ML_DSA under WITH_ML_DSA.The
newP11Object(...)routing forP11MLDSAPublicKeyObj/P11MLDSAPrivateKeyObjis straightforward and properly gated.
840-845: Mechanism exposure for CKM_ML_DSA is OK; exception handling change is fine.*Adding
CKM_ML_DSA_KEY_PAIR_GENandCKM_ML_DSAto the supported-mechanism map underWITH_ML_DSAlooks correct; thestd::out_of_rangecatch style change is also fine.Also applies to: 886-889
1150-1152: Generic secret key max size change should be kept consistent with generation limits.
0x8000000matches the generation-side upper bound later ingenerateGeneric(), so the mechanism-info change is coherent.
6265-6269: ML-DSA keypair-gen routing is correctly wired into C_GenerateKeyPair.Mechanism dispatch to
generateMLDSA(...)and the template consistency checks look consistent with the rest of the function’s pattern.Also applies to: 6297-6299, 6322-6324, 6397-6406
7118-7127: UnwrapKeySym(): CBC_PAD IV handling looks consistent, but keep parameter checks aligned with WrapKeySym().The
blocksizeis set before IV extraction in the*_CBC_PADcases, so thememcpy(&iv[0], ...)usage is safe here assumingpMechanism->ulParameterLenis enforced upstream (it is inC_UnwrapKey).Also applies to: 7153-7160
6994-6998: Wrap/unwrap support for CKK_ML_DSA keys looks consistent.Routing
CKK_ML_DSAtoAsymAlgo::MLDSAand usingMLDSAUtil::{getMLDSAPrivateKey,setMLDSAPrivateKey}matches the existing patterns for other asymmetric types.Also applies to: 7039-7042, 7720-7724
src/lib/crypto/OSSLMLDSA.cpp (9)
1-24: LGTM!Header structure, includes, and static constant definitions look correct. The
config.hinclude is standard for autotools projects (generated at build time).
176-311: LGTM!The verification implementation is well-structured with proper validation of signature length before verification and correct handling of OpenSSL's verification return values (distinguishing failure from error). Resource cleanup is handled correctly on all paths.
154-174: LGTM!Multi-part signing stubs correctly reject operations per PKCS#11 3.2 specification (ML-DSA is single-part only).
335-351: LGTM!Correctly rejects encryption/decryption operations since ML-DSA is a signature-only algorithm.
353-368: LGTM!Key size functions correctly return ML-DSA private key lengths in bytes, which aligns with PKCS#11 v3.2 §6.67.4 specification (ML-DSA uses bytes unlike RSA/DSA/ECDSA which use bits). Based on learnings, this is the expected behavior.
433-469: LGTM!Key pair reconstruction correctly uses chain deserialization and properly cleans up the allocated key pair on failure.
471-515: LGTM!Public and private key reconstruction functions follow the standard project pattern with proper cleanup on deserialization failure.
517-552: LGTM!Factory methods follow the standard project pattern for creating new key/parameter objects and reconstructing from serialized data.
418-428: No issues found. The code correctly handles thepkeyreference. BothsetFromOSSLmethods extract key material bytes usingEVP_PKEY_get_octet_string_paramand store only the extracted data inByteStringmembers—they do not retain a reference to the inputEVP_PKEYpointer. Each key object manages its own separateEVP_PKEYinstance created on-demand viacreateOSSLKey(). Freeing the temporarypkeyat line 428 is safe and correct.
antoinelochet
force-pushed
the
ml-dsa-pr
branch
from
cdac7b0 to
07d0768
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/SoftHSM.cpp (1)
6533-6602: Fix CBC wrapping/unwrapping:blocksize/DES3 bitlen are wrong for some paths (can break CKM_AES_CBC / CKM_DES3_CBC).*
WrapKeySym()usesiv.resize(blocksize)/memcpy(..., blocksize)for CBC modes, butblocksizeis never set forCKM_AES_CBCandCKM_DES3_CBC. Also, DES3 should likely usebb = 7(consistent with other DES(3) codepaths) beforesetBitLen().Proposed fix (ensure blocksize + DES3 bb are set, and enforce alignment for non-PAD CBC)
--- a/src/lib/SoftHSM.cpp +++ b/src/lib/SoftHSM.cpp @@ // Get the symmetric algorithm matching the mechanism SymAlgo::Type algo = SymAlgo::Unknown; SymWrap::Type mode = SymWrap::Unknown; size_t bb = 8; size_t blocksize = 0; auto wrappedlen = keydata.size(); @@ case CKM_AES_CBC: + blocksize = 16; algo = SymAlgo::AES; + if ((wrappedlen % blocksize) != 0) + return CKR_KEY_SIZE_RANGE; break; case CKM_AES_CBC_PAD: blocksize = 16; wrappedlen = RFC5652Pad(keydata, blocksize); algo = SymAlgo::AES; break; @@ case CKM_DES3_CBC: + blocksize = 8; + bb = 7; algo = SymAlgo::DES3; + if ((wrappedlen % blocksize) != 0) + return CKR_KEY_SIZE_RANGE; break; case CKM_DES3_CBC_PAD: blocksize = 8; + bb = 7; wrappedlen = RFC5652Pad(keydata, blocksize); algo = SymAlgo::DES3; break; @@ // Internal: Unwrap blob using symmetric key @@ // Get the symmetric algorithm matching the mechanism SymAlgo::Type algo = SymAlgo::Unknown; SymWrap::Type mode = SymWrap::Unknown; size_t bb = 8; size_t blocksize = 0; @@ case CKM_DES3_CBC_PAD: algo = SymAlgo::DES3; blocksize = 8; + bb = 7; break;Also applies to: 7134-7201
🧹 Nitpick comments (4)
src/lib/crypto/AsymmetricAlgorithm.h (1)
144-152: Silent allocation failure in convenience constructor.When
std::mallocfails (returnsNULL), the constructor silently setscontextLen = 0andownsContextMemory = false. This is a valid fallback, but callers have no way to detect the allocation failure. Consider whether signaling this failure is important for unit test scenarios where this constructor is used.src/lib/crypto/OSSLMLDSA.cpp (1)
236-236: Remove debug INFO_MSG.This
INFO_MSG("contextSize=%d", contextSize)appears to be leftover debug logging. Consider removing it before merging.Proposed fix
if (contextSize > 0) { - INFO_MSG("contextSize=%d", contextSize); if (contextSize > 255) {src/lib/SoftHSM.cpp (2)
949-951: Add a clarifying comment: ML‑DSA min/max sizes are bytes (PKCS#11 v3.2).
Since most other mechanisms treatulMinKeySize/ulMaxKeySizeas “bits”, a short comment here would prevent future accidental “*8” conversions.Also applies to: 1051-1059, 1351-1361
10132-10187: ML‑DSA keygen path looks solid; consider checking conflicting CKA_PARAMETER_SET in templates.
You correctly enforceCKP_ML_DSA_{44,65,87}and persistCKA_PARAMETER_SET/CKA_VALUE(/CKA_SEED)transactionally. Optional hardening: ifCKA_PARAMETER_SETis present in the private template too, reject mismatches early (instead of silently overriding during persistence).Also applies to: 10189-10206, 10257-10269, 10334-10351
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/lib/SoftHSM.cppsrc/lib/crypto/AsymmetricAlgorithm.hsrc/lib/crypto/MLDSAPrivateKey.cppsrc/lib/crypto/MLDSAUtil.cppsrc/lib/crypto/MLDSAUtil.hsrc/lib/crypto/OSSLMLDSA.cppsrc/lib/session_mgr/Session.cpp
🧰 Additional context used
🧠 Learnings (13)
📓 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: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: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/crypto/OSSLMLDSA.cpp:373-381
Timestamp: 2026-01-11T16:15:14.696Z
Learning: In PKCS#11 v3.2, ML-DSA key sizes (returned by getMinKeySize() and getMaxKeySize()) are specified in bytes, not bits. This is different from other algorithms like RSA, DSA, ECDSA, ECDH, and EdDSA which use bits. This is defined in chapter 6.67.4 of the PKCS#11 3.2 specification.
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/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: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/MLDSAUtil.hsrc/lib/session_mgr/Session.cppsrc/lib/crypto/MLDSAPrivateKey.cppsrc/lib/SoftHSM.cppsrc/lib/crypto/MLDSAUtil.cppsrc/lib/crypto/OSSLMLDSA.cppsrc/lib/crypto/AsymmetricAlgorithm.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/crypto/MLDSAUtil.hsrc/lib/session_mgr/Session.cppsrc/lib/crypto/MLDSAPrivateKey.cppsrc/lib/SoftHSM.cppsrc/lib/crypto/MLDSAUtil.cppsrc/lib/crypto/OSSLMLDSA.cppsrc/lib/crypto/AsymmetricAlgorithm.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/MLDSAUtil.hsrc/lib/session_mgr/Session.cppsrc/lib/SoftHSM.cppsrc/lib/crypto/MLDSAUtil.cppsrc/lib/crypto/OSSLMLDSA.cppsrc/lib/crypto/AsymmetricAlgorithm.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:
src/lib/crypto/MLDSAUtil.hsrc/lib/session_mgr/Session.cppsrc/lib/crypto/MLDSAPrivateKey.cppsrc/lib/SoftHSM.cppsrc/lib/crypto/MLDSAUtil.cppsrc/lib/crypto/OSSLMLDSA.cppsrc/lib/crypto/AsymmetricAlgorithm.h
📚 Learning: 2026-01-11T16:15:14.696Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/crypto/OSSLMLDSA.cpp:373-381
Timestamp: 2026-01-11T16:15:14.696Z
Learning: In PKCS#11 v3.2, ML-DSA key sizes (returned by getMinKeySize() and getMaxKeySize()) are specified in bytes, not bits. This is different from other algorithms like RSA, DSA, ECDSA, ECDH, and EdDSA which use bits. This is defined in chapter 6.67.4 of the PKCS#11 3.2 specification.
Applied to files:
src/lib/crypto/MLDSAUtil.hsrc/lib/session_mgr/Session.cppsrc/lib/crypto/MLDSAPrivateKey.cppsrc/lib/SoftHSM.cppsrc/lib/crypto/MLDSAUtil.cppsrc/lib/crypto/OSSLMLDSA.cppsrc/lib/crypto/AsymmetricAlgorithm.h
📚 Learning: 2025-11-11T19:42:46.886Z
Learnt from: JoaoHenrique12
Repo: softhsm/SoftHSMv2 PR: 823
File: src/lib/SoftHSM.cpp:58-58
Timestamp: 2025-11-11T19:42:46.886Z
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/MLDSAUtil.hsrc/lib/SoftHSM.cppsrc/lib/crypto/MLDSAUtil.cppsrc/lib/crypto/OSSLMLDSA.cppsrc/lib/crypto/AsymmetricAlgorithm.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/MLDSAUtil.hsrc/lib/session_mgr/Session.cppsrc/lib/SoftHSM.cppsrc/lib/crypto/MLDSAUtil.cppsrc/lib/crypto/OSSLMLDSA.cppsrc/lib/crypto/AsymmetricAlgorithm.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/MLDSAUtil.hsrc/lib/session_mgr/Session.cppsrc/lib/SoftHSM.cppsrc/lib/crypto/MLDSAUtil.cppsrc/lib/crypto/OSSLMLDSA.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
📚 Learning: 2026-01-10T15:07:10.701Z
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 837
File: src/lib/common/log.cpp:192-194
Timestamp: 2026-01-10T15:07:10.701Z
Learning: In SoftHSM, setLogFile and closeLogFile are designed to be called only during C_Initialize and C_Finalize respectively. Per PKCS#11 spec, C_Finalize behavior is undefined if called while other threads are making Cryptoki calls, so concurrent access between closeLogFile and softHSMLog would be API misuse. The implementation relies on these PKCS#11 threading guarantees rather than defensive locking in open/close operations.
Applied to files:
src/lib/SoftHSM.cpp
📚 Learning: 2025-08-18T19:35:33.938Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/crypto/OSSLMLDSA.cpp:195-204
Timestamp: 2025-08-18T19:35:33.938Z
Learning: In OpenSSL, EVP_PKEY_verify_message does not exist. EVP_PKEY_verify_message_init is designed for streaming verification and should be paired with EVP_PKEY_CTX_set_signature, EVP_PKEY_verify_message_update, and EVP_PKEY_verify_message_final - not with EVP_PKEY_verify.
Applied to files:
src/lib/crypto/OSSLMLDSA.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/OSSLMLDSA.cpp
🧬 Code graph analysis (4)
src/lib/crypto/MLDSAUtil.h (1)
src/lib/crypto/MLDSAUtil.cpp (8)
getMLDSAPrivateKey(11-41)getMLDSAPrivateKey(11-11)getMLDSAPublicKey(43-69)getMLDSAPublicKey(43-43)setMLDSAPrivateKey(71-128)setMLDSAPrivateKey(71-71)copyMLDSAContext(130-191)copyMLDSAContext(130-130)
src/lib/SoftHSM.cpp (1)
src/lib/crypto/MLDSAUtil.cpp (8)
copyMLDSAContext(130-191)copyMLDSAContext(130-130)getMLDSAPrivateKey(11-41)getMLDSAPrivateKey(11-11)getMLDSAPublicKey(43-69)getMLDSAPublicKey(43-43)setMLDSAPrivateKey(71-128)setMLDSAPrivateKey(71-71)
src/lib/crypto/OSSLMLDSA.cpp (1)
src/lib/crypto/OSSLUtil.cpp (2)
mldsaParameterSet2Name(226-235)mldsaParameterSet2Name(226-226)
src/lib/crypto/AsymmetricAlgorithm.h (1)
src/lib/test/TestsNoPINInitBase.cpp (1)
ptr(60-60)
🪛 Clang (14.0.6)
src/lib/crypto/MLDSAUtil.h
[error] 10-10: 'config.h' file not found
(clang-diagnostic-error)
src/lib/crypto/MLDSAPrivateKey.cpp
[error] 7-7: 'config.h' file not found
(clang-diagnostic-error)
src/lib/crypto/MLDSAUtil.cpp
[error] 7-7: 'config.h' file not found
(clang-diagnostic-error)
src/lib/crypto/OSSLMLDSA.cpp
[error] 7-7: 'config.h' file not found
(clang-diagnostic-error)
src/lib/crypto/AsymmetricAlgorithm.h
[error] 36-36: 'cstdlib' file not found
(clang-diagnostic-error)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Windows (x64, openssl)
- GitHub Check: macOS (openssl)
- GitHub Check: Linux with OpenSSL 1.1.1
- GitHub Check: Windows (x64, botan)
- GitHub Check: macOS (botan)
- GitHub Check: Linux with Botan
- GitHub Check: Windows (x86, openssl)
- GitHub Check: Linux with OpenSSL 3.5.4
- GitHub Check: Linux with OpenSSL 3.0
🔇 Additional comments (26)
src/lib/session_mgr/Session.cpp (1)
360-371: LGTM! The pointer fixup logic is correct.The deep-copy approach with pointer adjustment follows the agreed design. The calculation
reinterpret_cast<unsigned char*>(sac + 1)correctly points to the context bytes immediately following the struct in the newly copied buffer. Aftermemcpy, thecontextDatapointer becomes stale and requires adjustment to point into the new allocation—the fixup properly handles this.The condition
inParamLen > sizeof(SIGN_ADDITIONAL_CONTEXT)ensures context bytes exist, and the inner check guards against fixing up when there's no actual context data. The invariant thatcontextLen <= inParamLen - sizeof(SIGN_ADDITIONAL_CONTEXT)is guaranteed by the caller (SoftHSM.cpp), which constructs the parameter assizeof(struct) + contextLenand validatescontextLen <= 255, so a defensive check is not required.src/lib/crypto/AsymmetricAlgorithm.h (5)
36-37: LGTM on includes.The
<cstdlib>and<cstring>includes are correctly added to supportstd::malloc,std::free, andstd::memcpyused in the new structs below. The static analysis hint aboutcstdlibnot found is a false positive—this is a standard C++ header that will be available during actual compilation.
58-59: LGTM on enum additions.The
MLDSAentries are correctly added to bothAsymAlgo::TypeandAsymMech::Typeenums, following the existing pattern for other algorithms.Also applies to: 98-99
123-131: LGTM on Hedge struct.The
Hedge::Typeenum correctly models the three hedge variants from PKCS#11 v3.2 (CKH_HEDGE_PREFERRED,CKH_HEDGE_REQUIRED,CKH_DETERMINISTIC_REQUIRED).
157-162: LGTM on deleted copy/move semantics.Properly prevents accidental double-free by deleting copy and move constructors/assignments.
164-175: Guard destructor correctly handles placement-new'd objects.The explicit destructor call followed by
std::freeis necessary becausecopyMLDSAContextusesreinterpret_caston malloc'd memory rather thannew. SinceownsContextMemoryisfalsefor these cases, the~SIGN_ADDITIONAL_CONTEXT()destructor won't double-free the context data.src/lib/crypto/MLDSAPrivateKey.cpp (4)
17-24: LGTM on isOfType implementation.Correctly handles NULL input and uses
strcmpconsistently with the pattern inMLDSAPublicKey::isOfType.
32-43: LGTM on parameter set and output length inference.The switch statements correctly use
ML_DSA_*_PRIV_LENGTHconstants to infer parameter sets and signature lengths from private key material size. This aligns with the project's design where parameter sets are inferred from key length rather than explicitly stored. Based on learnings, this is the intended approach.Also applies to: 46-57
80-84: LGTM on serialization.Serialization correctly concatenates
seed.serialise()followed byvalue.serialise(), matching the expected deserialization order.
86-100: LGTM on deserialization.The
chainDeserialisepattern correctly reads seed then value in order, and properly validates that both components are non-empty before accepting the key.src/lib/crypto/OSSLMLDSA.cpp (5)
27-151: LGTM on sign() implementation.The signing flow correctly:
- Validates mechanism and key types
- Handles
SIGN_ADDITIONAL_CONTEXTwith proper bounds checking (context length ≤ 255)- Constructs
OSSL_PARAMarray for context string and deterministic mode- Uses
EVP_PKEY_sign_message_init+EVP_PKEY_signworkflow- Properly frees
EVP_SIGNATUREandEVP_PKEY_CTXon all paths
153-173: LGTM on multi-part operation stubs.Correctly reject multi-part signing/verification with appropriate error messages. Per PKCS#11 v3.2 and PR comments, ML-DSA is single-part only.
Also applies to: 311-331
351-359: LGTM on key size methods.Returning private key lengths in bytes aligns with PKCS#11 v3.2 §6.67.4 which specifies ML-DSA key sizes in bytes (unlike RSA/DSA/EC which use bits). Based on learnings, this is the correct approach.
431-467: LGTM on reconstructKeyPair.Correctly deserializes public and private keys from serialized data, with proper cleanup on failure.
415-428: No reference counting issue detected.The
setFromOSSLmethods in bothOSSLMLDSAPrivateKeyandOSSLMLDSAPublicKeyextract key material usingEVP_PKEY_get_octet_string_paramand store the data asByteStringvalues; they do not store or increment references to theEVP_PKEYpointer. Therefore, callingEVP_PKEY_free(pkey)after bothsetFromOSSLcalls is correct and required.src/lib/crypto/MLDSAUtil.h (1)
20-29: LGTM on MLDSAUtil class declaration.Clean utility class with static methods for:
- Key material extraction (
getMLDSAPrivateKey,getMLDSAPublicKey)- Key attribute setting (
setMLDSAPrivateKey)- Context copying (
copyMLDSAContext)The API is consistent with similar utility classes in the codebase.
src/lib/crypto/MLDSAUtil.cpp (4)
11-41: LGTM on getMLDSAPrivateKey.Correctly:
- Validates all input pointers
- Checks
CKA_PRIVATEto determine if decryption is needed- Reads both
CKA_VALUEandCKA_SEEDattributes- Returns appropriate error codes (
CKR_ARGUMENTS_BAD,CKR_GENERAL_ERROR)
43-69: LGTM on getMLDSAPublicKey.Follows the same pattern as
getMLDSAPrivateKey, appropriately handling onlyCKA_VALUEfor public keys.
71-128: LGTM on setMLDSAPrivateKey.The implementation correctly:
- Acquires and releases the asymmetric algorithm via
CryptoFactory- Decodes PKCS#8 data using
PKCS8Decode- Encrypts seed and value when
isPrivateis true- Sets all three attributes (
CKA_PARAMETER_SET,CKA_SEED,CKA_VALUE)- Cleans up resources on all exit paths
130-191: LGTM on copyMLDSAContext.The implementation correctly:
- Validates inputs including context length bounds (≤ 255)
- Maps
CKH_*hedge variants to internalHedge::Type- Allocates a single contiguous blob for struct + context bytes
- Sets
ownsContextMemory = falsesince the entire blob is freed togetherPer learnings, this design avoids transient malloc in SoftHSM and lets
Session::resetOpfree the entirem_pParametersblob.src/lib/SoftHSM.cpp (6)
65-69: Unconditional ML‑DSA includes match project include policy.
Good to see these not gated by WITH_* macros; feature gating should happen in mechanism tables / CryptoFactory. As per coding guidelines, keep it this way.
148-172: ML‑DSA P11 object instantiation is correctly gated.
P11MLDSAPublicKeyObj / P11MLDSAPrivateKeyObj wiring under#ifdef WITH_ML_DSAlooks consistent with other key types.
841-844: Mechanism exposure for CKM_ML_DSA(_KEY_PAIR_GEN) looks correct.
Matches the expected pattern inprepareSupportedMechanisms().
5787-5788: Extra debug logging on signature-length mismatch is useful and low risk.
This should help diagnose interop issues without changing functional behavior.Also applies to: 5971-5972
6281-6285: CKM_ML_DSA_KEY_PAIR_GEN routing into generateMLDSA is consistent.
Template consistency checks + gated dispatch look aligned with existing RSA/EC/EDDSA patterns.Also applies to: 6313-6315, 6338-6340, 6413-6422
4200-4205: The implementation is correct and does not require changes.Session::setParameters()properly deep-copies the transient SIGN_ADDITIONAL_CONTEXT blob (lines 355–358 in Session.cpp), includes the appended context bytes in the copy, and reconstructs the internal pointer after the copy (line 368). The copied buffer is owned by Session and freed bySession::resetOp(). TheMLDSAAdditionalContextGuardRAII object is safely destroyed at the end of the function scope after passing ownership to Session viasetParameters(). The inline comment is slightly imprecise but not incorrect—it conveys that the guard becomes irrelevant aftersetParameters()transfers ownership to Session.
antoinelochet
force-pushed
the
ml-dsa-pr
branch
4 times, most recently
from
107776b to
0009185
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/SoftHSM.cpp (1)
6537-6614: Fix blocksize initialization and DES3 mechanism support in wrap/unwrap functions.Three correctness bugs exist:
Blocksize uninitialized for CBC variants:
CKM_AES_CBCandCKM_DES3_CBCcases inWrapKeySymdo not setblocksize, leaving it at 0. Later code uses this to size and copy the IV, resulting in a zero-length IV.DES3 key-type check always fails: Both
C_WrapKey(line ~6894) andC_UnwrapKey(line ~7536) use(keytype != CKK_DES2 || keytype != CKK_DES3)with OR operator. This always evaluates to true for any keytype, unconditionally rejecting all DES3 wrapping operations.Mechanism whitelist missing DES3 support:
C_WrapKeyandC_UnwrapKeyonly whitelistCKM_AES_CBC(_PAD), notCKM_DES3_CBC(_PAD). YetWrapKeySymsupports all four mechanisms, creating dead/unreachable code.🛠️ Proposed fix
--- a/src/lib/SoftHSM.cpp +++ b/src/lib/SoftHSM.cpp @@ -6539,6 @@ CK_RV SoftHSM::WrapKeySym case CKM_AES_CBC: + blocksize = 16; algo = SymAlgo::AES; break; @@ -6549,6 +6550,7 @@ CK_RV SoftHSM::WrapKeySym case CKM_DES3_CBC: + blocksize = 8; algo = SymAlgo::DES3; break; @@ -6583,6 +6585,12 @@ CK_RV SoftHSM::WrapKeySym case CKM_AES_CBC_PAD: case CKM_DES3_CBC: case CKM_DES3_CBC_PAD: + // Defensive check: blocksize must be set for CBC variants. + if (blocksize == 0) { + cipher->recycleKey(wrappingkey); + CryptoFactory::i()->recycleSymmetricAlgorithm(cipher); + return CKR_MECHANISM_INVALID; + } iv.resize(blocksize); memcpy(&iv[0], pMechanism->pParameter, blocksize); @@ -6843,8 +6851,9 @@ CK_RV SoftHSM::C_WrapKey case CKM_AES_CBC: case CKM_AES_CBC_PAD: + case CKM_DES3_CBC: + case CKM_DES3_CBC_PAD: - if (pMechanism->pParameter == NULL_PTR || - pMechanism->ulParameterLen != 16) + if (pMechanism->pParameter == NULL_PTR) return CKR_ARGUMENTS_BAD; break; default: @@ -6894,8 +6903,9 @@ CK_RV SoftHSM::C_WrapKey if ((pMechanism->mechanism == CKM_AES_CBC || pMechanism->mechanism == CKM_AES_CBC_PAD) && wrapKey->getUnsignedLongValue(CKA_KEY_TYPE, CKK_VENDOR_DEFINED) != CKK_AES) return CKR_WRAPPING_KEY_TYPE_INCONSISTENT; - if (pMechanism->mechanism == CKM_DES3_CBC && (wrapKey->getUnsignedLongValue(CKA_KEY_TYPE, CKK_VENDOR_DEFINED) != CKK_DES2 || - wrapKey->getUnsignedLongValue(CKA_KEY_TYPE, CKK_VENDOR_DEFINED) != CKK_DES3)) + if ((pMechanism->mechanism == CKM_DES3_CBC || pMechanism->mechanism == CKM_DES3_CBC_PAD) && + (wrapKey->getUnsignedLongValue(CKA_KEY_TYPE, CKK_VENDOR_DEFINED) != CKK_DES2 && + wrapKey->getUnsignedLongValue(CKA_KEY_TYPE, CKK_VENDOR_DEFINED) != CKK_DES3)) return CKR_WRAPPING_KEY_TYPE_INCONSISTENT; @@ -7536,8 +7545,9 @@ CK_RV SoftHSM::C_UnwrapKey if ((pMechanism->mechanism == CKM_AES_CBC || pMechanism->mechanism == CKM_AES_CBC_PAD) && unwrapKey->getUnsignedLongValue(CKA_KEY_TYPE, CKK_VENDOR_DEFINED) != CKK_AES) return CKR_WRAPPING_KEY_TYPE_INCONSISTENT; - if (pMechanism->mechanism == CKM_DES3_CBC && (unwrapKey->getUnsignedLongValue(CKA_KEY_TYPE, CKK_VENDOR_DEFINED) != CKK_DES2 || - unwrapKey->getUnsignedLongValue(CKA_KEY_TYPE, CKK_VENDOR_DEFINED) != CKK_DES3)) + if ((pMechanism->mechanism == CKM_DES3_CBC || pMechanism->mechanism == CKM_DES3_CBC_PAD) && + (unwrapKey->getUnsignedLongValue(CKA_KEY_TYPE, CKK_VENDOR_DEFINED) != CKK_DES2 && + unwrapKey->getUnsignedLongValue(CKA_KEY_TYPE, CKK_VENDOR_DEFINED) != CKK_DES3)) return CKR_WRAPPING_KEY_TYPE_INCONSISTENT;
🧹 Nitpick comments (2)
src/lib/crypto/MLDSAPrivateKey.cpp (1)
32-43: Consider logging unknown parameter set cases.The function returns
0ULfor unrecognized key sizes, which is correct. However, callers should be aware that an unknown size will silently return 0. Consider adding a debug log for unmatched sizes to aid troubleshooting invalid key material.💡 Optional: Add debug logging for unknown sizes
unsigned long MLDSAPrivateKey::getParameterSet() const { switch(value.size()) { case MLDSAParameters::ML_DSA_44_PRIV_LENGTH: return MLDSAParameters::ML_DSA_44_PARAMETER_SET; case MLDSAParameters::ML_DSA_65_PRIV_LENGTH: return MLDSAParameters::ML_DSA_65_PARAMETER_SET; case MLDSAParameters::ML_DSA_87_PRIV_LENGTH: return MLDSAParameters::ML_DSA_87_PARAMETER_SET; + default: + DEBUG_MSG("Unknown ML-DSA private key size: %zu", value.size()); + break; } return 0UL; }src/lib/crypto/MLDSAUtil.cpp (1)
71-128: Consider extracting the cast to a local variable.The repeated
((MLDSAPrivateKey*)priv)casts work correctly but add visual noise. Consider storing the cast result in a local variable for clarity.♻️ Optional: Extract cast to local variable
if (!priv->PKCS8Decode(ber)) { mldsa->recyclePrivateKey(priv); CryptoFactory::i()->recycleAsymmetricAlgorithm(mldsa); return false; } + MLDSAPrivateKey* mldsaPriv = static_cast<MLDSAPrivateKey*>(priv); // ML-DSA Private Key Attributes ByteString seed; ByteString value; if (isPrivate) { if (token == NULL) { mldsa->recyclePrivateKey(priv); CryptoFactory::i()->recycleAsymmetricAlgorithm(mldsa); return false; } - if (!token->encrypt(((MLDSAPrivateKey*)priv)->getSeed(), seed)) + if (!token->encrypt(mldsaPriv->getSeed(), seed)) { mldsa->recyclePrivateKey(priv); CryptoFactory::i()->recycleAsymmetricAlgorithm(mldsa); return false; } - if (!token->encrypt(((MLDSAPrivateKey*)priv)->getValue(), value)) + if (!token->encrypt(mldsaPriv->getValue(), value)) { mldsa->recyclePrivateKey(priv); CryptoFactory::i()->recycleAsymmetricAlgorithm(mldsa); return false; } } else { - seed = ((MLDSAPrivateKey*)priv)->getSeed(); - value = ((MLDSAPrivateKey*)priv)->getValue(); + seed = mldsaPriv->getSeed(); + value = mldsaPriv->getValue(); } bool bOK = true; - bOK = bOK && key->setAttribute(CKA_PARAMETER_SET, ((MLDSAPrivateKey*)priv)->getParameterSet()); + bOK = bOK && key->setAttribute(CKA_PARAMETER_SET, mldsaPriv->getParameterSet()); bOK = bOK && key->setAttribute(CKA_SEED, seed); bOK = bOK && key->setAttribute(CKA_VALUE, value);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/lib/SoftHSM.cppsrc/lib/crypto/AsymmetricAlgorithm.hsrc/lib/crypto/MLDSAPrivateKey.cppsrc/lib/crypto/MLDSAUtil.cppsrc/lib/crypto/MLDSAUtil.hsrc/lib/crypto/OSSLMLDSA.cppsrc/lib/session_mgr/Session.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/session_mgr/Session.cpp
🧰 Additional context used
🧠 Learnings (13)
📓 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: 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: 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.
📚 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/AsymmetricAlgorithm.hsrc/lib/crypto/MLDSAUtil.cppsrc/lib/SoftHSM.cppsrc/lib/crypto/OSSLMLDSA.cppsrc/lib/crypto/MLDSAPrivateKey.cppsrc/lib/crypto/MLDSAUtil.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/AsymmetricAlgorithm.hsrc/lib/crypto/MLDSAUtil.cppsrc/lib/SoftHSM.cppsrc/lib/crypto/OSSLMLDSA.cppsrc/lib/crypto/MLDSAPrivateKey.cppsrc/lib/crypto/MLDSAUtil.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/crypto/AsymmetricAlgorithm.hsrc/lib/crypto/MLDSAUtil.cppsrc/lib/SoftHSM.cppsrc/lib/crypto/OSSLMLDSA.cppsrc/lib/crypto/MLDSAPrivateKey.cppsrc/lib/crypto/MLDSAUtil.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/AsymmetricAlgorithm.hsrc/lib/crypto/MLDSAUtil.cppsrc/lib/SoftHSM.cppsrc/lib/crypto/OSSLMLDSA.cppsrc/lib/crypto/MLDSAUtil.h
📚 Learning: 2025-11-11T19:42:46.886Z
Learnt from: JoaoHenrique12
Repo: softhsm/SoftHSMv2 PR: 823
File: src/lib/SoftHSM.cpp:58-58
Timestamp: 2025-11-11T19:42:46.886Z
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/AsymmetricAlgorithm.hsrc/lib/crypto/MLDSAUtil.cppsrc/lib/SoftHSM.cppsrc/lib/crypto/OSSLMLDSA.cppsrc/lib/crypto/MLDSAUtil.h
📚 Learning: 2026-01-11T16:15:14.696Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/crypto/OSSLMLDSA.cpp:373-381
Timestamp: 2026-01-11T16:15:14.696Z
Learning: In PKCS#11 v3.2, ML-DSA key sizes (returned by getMinKeySize() and getMaxKeySize()) are specified in bytes, not bits. This is different from other algorithms like RSA, DSA, ECDSA, ECDH, and EdDSA which use bits. This is defined in chapter 6.67.4 of the PKCS#11 3.2 specification.
Applied to files:
src/lib/crypto/AsymmetricAlgorithm.hsrc/lib/crypto/MLDSAUtil.cppsrc/lib/SoftHSM.cppsrc/lib/crypto/OSSLMLDSA.cppsrc/lib/crypto/MLDSAPrivateKey.cppsrc/lib/crypto/MLDSAUtil.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:
src/lib/crypto/AsymmetricAlgorithm.hsrc/lib/crypto/MLDSAUtil.cppsrc/lib/SoftHSM.cppsrc/lib/crypto/OSSLMLDSA.cppsrc/lib/crypto/MLDSAPrivateKey.cppsrc/lib/crypto/MLDSAUtil.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/MLDSAUtil.cppsrc/lib/SoftHSM.cppsrc/lib/crypto/OSSLMLDSA.cppsrc/lib/crypto/MLDSAUtil.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/SoftHSM.cpp
📚 Learning: 2026-01-10T15:07:10.701Z
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 837
File: src/lib/common/log.cpp:192-194
Timestamp: 2026-01-10T15:07:10.701Z
Learning: In SoftHSM, setLogFile and closeLogFile are designed to be called only during C_Initialize and C_Finalize respectively. Per PKCS#11 spec, C_Finalize behavior is undefined if called while other threads are making Cryptoki calls, so concurrent access between closeLogFile and softHSMLog would be API misuse. The implementation relies on these PKCS#11 threading guarantees rather than defensive locking in open/close operations.
Applied to files:
src/lib/SoftHSM.cpp
📚 Learning: 2025-08-18T19:35:33.938Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/crypto/OSSLMLDSA.cpp:195-204
Timestamp: 2025-08-18T19:35:33.938Z
Learning: In OpenSSL, EVP_PKEY_verify_message does not exist. EVP_PKEY_verify_message_init is designed for streaming verification and should be paired with EVP_PKEY_CTX_set_signature, EVP_PKEY_verify_message_update, and EVP_PKEY_verify_message_final - not with EVP_PKEY_verify.
Applied to files:
src/lib/crypto/OSSLMLDSA.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/OSSLMLDSA.cpp
🧬 Code graph analysis (5)
src/lib/crypto/AsymmetricAlgorithm.h (1)
src/lib/test/TestsNoPINInitBase.cpp (1)
ptr(60-60)
src/lib/SoftHSM.cpp (1)
src/lib/crypto/MLDSAUtil.cpp (8)
copyMLDSAContext(130-191)copyMLDSAContext(130-130)getMLDSAPrivateKey(11-41)getMLDSAPrivateKey(11-11)getMLDSAPublicKey(43-69)getMLDSAPublicKey(43-43)setMLDSAPrivateKey(71-128)setMLDSAPrivateKey(71-71)
src/lib/crypto/OSSLMLDSA.cpp (1)
src/lib/crypto/OSSLUtil.cpp (2)
mldsaParameterSet2Name(226-235)mldsaParameterSet2Name(226-226)
src/lib/crypto/MLDSAPrivateKey.cpp (1)
src/lib/crypto/MLDSAParameters.cpp (6)
getParameterSet(22-25)getParameterSet(22-22)serialise(34-37)serialise(34-34)deserialise(39-50)deserialise(39-39)
src/lib/crypto/MLDSAUtil.h (1)
src/lib/crypto/MLDSAUtil.cpp (8)
getMLDSAPrivateKey(11-41)getMLDSAPrivateKey(11-11)getMLDSAPublicKey(43-69)getMLDSAPublicKey(43-43)setMLDSAPrivateKey(71-128)setMLDSAPrivateKey(71-71)copyMLDSAContext(130-191)copyMLDSAContext(130-130)
🪛 Clang (14.0.6)
src/lib/crypto/AsymmetricAlgorithm.h
[error] 36-36: 'cstdlib' file not found
(clang-diagnostic-error)
src/lib/crypto/MLDSAUtil.cpp
[error] 7-7: 'config.h' file not found
(clang-diagnostic-error)
src/lib/crypto/OSSLMLDSA.cpp
[error] 7-7: 'config.h' file not found
(clang-diagnostic-error)
src/lib/crypto/MLDSAPrivateKey.cpp
[error] 7-7: 'config.h' file not found
(clang-diagnostic-error)
src/lib/crypto/MLDSAUtil.h
[error] 10-10: 'config.h' file not found
(clang-diagnostic-error)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: macOS (openssl)
- GitHub Check: Linux with OpenSSL 3.5.4
- GitHub Check: macOS (botan)
- GitHub Check: Windows (x86, openssl)
- GitHub Check: Linux with OpenSSL 1.1.1
- GitHub Check: Windows (x64, botan)
- GitHub Check: Windows (x64, openssl)
- GitHub Check: Linux with OpenSSL 3.0
- GitHub Check: Linux with Botan
🔇 Additional comments (28)
src/lib/crypto/MLDSAPrivateKey.cpp (3)
17-24: LGTM!The type checking implementation correctly handles NULL input and uses
strcmpfor string comparison.
46-57: LGTM!The signature length mapping follows the same established pattern as
getParameterSet()and correctly maps private key sizes to their corresponding signature lengths.
86-100: LGTM with minor observation.The deserialization correctly validates both fields are non-empty before assignment. The implementation follows the expected serialization order (seed then value) established in
serialise().src/lib/crypto/MLDSAUtil.h (1)
20-29: LGTM!The utility class provides a clean static interface for ML-DSA key operations and context handling. The function signatures appropriately use the internal
SIGN_ADDITIONAL_CONTEXTtype for the copied context output, aligning with the deep-copy strategy described in the learnings.src/lib/crypto/MLDSAUtil.cpp (3)
11-41: LGTM!The function correctly handles both encrypted (CKA_PRIVATE=true) and unencrypted key material, with appropriate error handling for decryption failures.
43-69: LGTM!Follows the same secure pattern as
getMLDSAPrivateKey, appropriately handling only the value attribute for public keys.
130-191: LGTM - well-designed single-allocation pattern.The function correctly implements the single-blob allocation strategy for
SIGN_ADDITIONAL_CONTEXTplus context bytes, as discussed in the learnings. TheownsContextMemory = falseflag is intentional since the entire blob is freed viafree(sac)rather than separately freeingcontextData.The context length validation (max 255) aligns with the ML-DSA specification.
src/lib/crypto/AsymmetricAlgorithm.h (4)
36-37: LGTM!The
<cstdlib>and<cstring>includes are necessary forstd::malloc,std::free, andstd::memcpyused in the new structures below.
58-59: LGTM!The
MLDSAentries extend the existing algorithm and mechanism enums cleanly, following the established pattern.Also applies to: 98-99
123-131: LGTM!The
Hedge::Typeenum provides an internal representation for the PKCS#11CK_HEDGE_TYPEvalues, cleanly abstracting the PKCS#11 constants.
164-175: LGTM with usage caveat.The guard correctly handles the single-blob allocation pattern from
copyMLDSAContext()by calling the destructor (which is a no-op whenownsContextMemory=false) thenfree().Note: This guard is designed specifically for
malloc-allocatedSIGN_ADDITIONAL_CONTEXTpointers. Objects created vianewor on the stack should not use this guard.src/lib/SoftHSM.cpp (8)
65-69: Unconditional ML-DSA includes look consistent with SoftHSM policy.
This matches the project’s “include headers unconditionally; gate via mechanisms/CryptoFactory” approach. Based on learnings, this is preferred.
148-172: ML-DSA object routing via WITH_ML_DSA looks correct.
The CKK_ML_DSA → P11MLDSA{Public,Private}KeyObj wiring is appropriately guarded and mirrors existing key-type routing.
841-845: Mechanism exposure for ML-DSA is wired correctly.
Adding CKM_ML_DSA_KEY_PAIR_GEN and CKM_ML_DSA behind WITH_ML_DSA is the right place to surface them for configuration and mechanism lists.Also applies to: 886-889
949-952: ML-DSA mechanism info: confirm “key sizes in bytes” semantics + clean up minor nits.
The comment/handling indicates ML-DSA min/max are bytes (unlike most others). Please ensure CryptoFactory/AsymAlgo::MLDSA returns min/max in bytes as expected, and consider fixing the spelling/indentation for readability. Based on learnings, bytes-vs-bits is intentional for ML-DSA.Also applies to: 1051-1061, 1352-1363
1151-1154: CKM_GENERIC_SECRET_KEY_GEN max size change aligns with generateGeneric checks.
Setting ulMaxKeySize to 0x8000000 matches later enforcement (keyLen > 0x8000000), so the API surface and implementation stay consistent.
5786-5792: Debug logging on signature length mismatch is fine; keep it non-sensitive.
Logging only lengths is typically OK; just ensure DEBUG logs won’t be enabled in sensitive deployments and the format types match CK_ULONG on all supported platforms.Also applies to: 5969-5976
6283-6424: ML-DSA keypair generation path looks coherent; parameter-set validation is solid.
Template consistency checks +generateMLDSAenforcingCKP_ML_DSA_{44,65,87}and returningCKR_PARAMETER_SET_NOT_SUPPORTEDlooks correct, and the object attribute persistence (CKA_PARAMETER_SET/CKA_VALUE/CKA_SEED) is consistent.Also applies to: 6315-6317, 6340-6342, 10134-10392
4202-4505: The ML-DSA additional context ownership is handled correctly. The code allocates a temporary context viaMLDSAUtil::copyMLDSAContext(), whichMLDSAAdditionalContextGuard(RAII) holds safely.Session::setParameters()deep-copies this into session-owned storage viamalloc()andmemcpy(), then fixes the internalcontextDatapointer to reference the new copied buffer. When the guard scope ends, the temporary is freed via the destructor; when the session resets,Session::resetOp()frees the session-owned copy. No UAF or double-free risk.src/lib/crypto/OSSLMLDSA.cpp (9)
1-25: LGTM!The includes are appropriate for the OpenSSL EVP-based ML-DSA implementation. The static
OSSL_RANDOMandOSSL_DETERMINISTICmembers are correctly defined asintvariables (not constants) sinceOSSL_PARAM_construct_intrequires a pointer to a mutableint.Note: The static analysis warning about
config.hnot found is a false positive—this file is generated by the autoconf build system.
26-151: LGTM!The signing implementation correctly:
- Validates the mechanism and key types
- Handles SIGN_ADDITIONAL_CONTEXT parameters for hedged/deterministic variants
- Properly terminates the OSSL_PARAM array in all code paths
- Cleans up OpenSSL resources (EVP_PKEY_CTX, EVP_SIGNATURE) on all error paths
153-173: LGTM!Multi-part signing is correctly unsupported for ML-DSA per PKCS#11 3.2 specification. The stubs appropriately return
falsewith descriptive error messages.
175-308: LGTM!The verification implementation correctly:
- Validates signature length against expected output before calling OpenSSL
- Handles SIGN_ADDITIONAL_CONTEXT parameters consistently with the sign function
- Properly distinguishes between verification failure (
verifyRV == 0) and actual errors (verifyRV < 0), only logging errors for the latter
310-330: LGTM!Multi-part verification stubs correctly reject unsupported operations, consistent with ML-DSA single-part requirement.
332-365: LGTM!
- Encryption/decryption stubs correctly reject operations since ML-DSA is a signature-only algorithm
- Key size functions return byte values (not bits), which aligns with PKCS#11 3.2 §6.67.4 for ML-DSA
430-512: LGTM!The key reconstruction functions follow the established SoftHSM pattern:
- Validate input parameters
- Create new key object
- Attempt deserialization
- Clean up on failure
The
reconstructKeyPairfunction could theoretically short-circuit after the first deserialization failure, but the current approach is correct and matches patterns used elsewhere in the codebase.
514-549: LGTM!The factory methods and
reconstructParametersfollow the established patterns in the codebase. Clean and straightforward implementations.
414-425: No action required — the code is safe.
setFromOSSLextracts and deep-copies the key material usingEVP_PKEY_get_octet_string_param(), storing only the extracted seed/private/public key bytes. TheEVP_PKEYpointer itself is not retained, making theEVP_PKEY_free(pkey)call at line 425 safe.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/lib/crypto/AsymmetricAlgorithm.h (1)
144-152: Silent allocation failure could mask errors in tests.When
std::mallocfails on line 148, the constructor silently setscontextLen = 0andownsContextMemory = false, making the object appear valid but with no context data. Since this constructor is noted as being "used in unit tests," consider adding anisValid()method or throwing an exception to help detect allocation failures during testing.💡 Optional: Add validity check
+ bool isValid() const { + // Valid if no context was requested, or if context was successfully allocated + return (contextLen == 0 && contextData == NULL) || + (contextLen > 0 && contextData != NULL); + }src/lib/SoftHSM.cpp (2)
840-844: Mechanism exposure for ML-DSA is straightforward; consider config token UX.Adding
CKM_ML_DSA_KEY_PAIR_GENandCKM_ML_DSAtoprepareSupportedMechanisms()is fine. One small UX thought: ifslots.mechanismsis configured, users must now explicitly allow/deny these tokens—worth ensuring the docs/tests cover this new config surface.Also applies to: 886-889
10135-10393:generateMLDSA: solid parameter-set validation; consider template consistency checks.The
CKA_PARAMETER_SETvalidation andCKR_PARAMETER_SET_NOT_SUPPORTEDbehavior match project conventions (usingCKP_ML_DSA_*). Based on learnings, keepingCKP_ML_DSA_*constants is preferred.One follow-up that may prevent confusing callers: if
CKA_PARAMETER_SETis also provided in the private template (or both templates), consider rejecting mismatches withCKR_TEMPLATE_INCONSISTENTrather than ignoring one side.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/lib/SoftHSM.cppsrc/lib/crypto/AsymmetricAlgorithm.hsrc/lib/crypto/MLDSAPrivateKey.cppsrc/lib/crypto/MLDSAUtil.cppsrc/lib/crypto/MLDSAUtil.hsrc/lib/crypto/OSSLMLDSA.cppsrc/lib/session_mgr/Session.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/session_mgr/Session.cpp
🧰 Additional context used
🧠 Learnings (13)
📓 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: 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: 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.
📚 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/AsymmetricAlgorithm.hsrc/lib/crypto/MLDSAPrivateKey.cppsrc/lib/crypto/MLDSAUtil.cppsrc/lib/crypto/OSSLMLDSA.cppsrc/lib/SoftHSM.cppsrc/lib/crypto/MLDSAUtil.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/AsymmetricAlgorithm.hsrc/lib/crypto/MLDSAPrivateKey.cppsrc/lib/crypto/MLDSAUtil.cppsrc/lib/crypto/OSSLMLDSA.cppsrc/lib/SoftHSM.cppsrc/lib/crypto/MLDSAUtil.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/crypto/AsymmetricAlgorithm.hsrc/lib/crypto/MLDSAPrivateKey.cppsrc/lib/crypto/MLDSAUtil.cppsrc/lib/crypto/OSSLMLDSA.cppsrc/lib/SoftHSM.cppsrc/lib/crypto/MLDSAUtil.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/AsymmetricAlgorithm.hsrc/lib/crypto/MLDSAUtil.cppsrc/lib/crypto/OSSLMLDSA.cppsrc/lib/SoftHSM.cppsrc/lib/crypto/MLDSAUtil.h
📚 Learning: 2025-11-11T19:42:46.886Z
Learnt from: JoaoHenrique12
Repo: softhsm/SoftHSMv2 PR: 823
File: src/lib/SoftHSM.cpp:58-58
Timestamp: 2025-11-11T19:42:46.886Z
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/AsymmetricAlgorithm.hsrc/lib/crypto/MLDSAUtil.cppsrc/lib/crypto/OSSLMLDSA.cppsrc/lib/SoftHSM.cppsrc/lib/crypto/MLDSAUtil.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:
src/lib/crypto/AsymmetricAlgorithm.hsrc/lib/crypto/MLDSAPrivateKey.cppsrc/lib/crypto/MLDSAUtil.cppsrc/lib/crypto/OSSLMLDSA.cppsrc/lib/SoftHSM.cppsrc/lib/crypto/MLDSAUtil.h
📚 Learning: 2026-01-11T16:15:14.696Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/crypto/OSSLMLDSA.cpp:373-381
Timestamp: 2026-01-11T16:15:14.696Z
Learning: In PKCS#11 v3.2, ML-DSA key sizes (returned by getMinKeySize() and getMaxKeySize()) are specified in bytes, not bits. This is different from other algorithms like RSA, DSA, ECDSA, ECDH, and EdDSA which use bits. This is defined in chapter 6.67.4 of the PKCS#11 3.2 specification.
Applied to files:
src/lib/crypto/AsymmetricAlgorithm.hsrc/lib/crypto/MLDSAPrivateKey.cppsrc/lib/crypto/MLDSAUtil.cppsrc/lib/crypto/OSSLMLDSA.cppsrc/lib/SoftHSM.cppsrc/lib/crypto/MLDSAUtil.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/MLDSAUtil.cppsrc/lib/crypto/OSSLMLDSA.cppsrc/lib/SoftHSM.cppsrc/lib/crypto/MLDSAUtil.h
📚 Learning: 2025-08-18T19:35:33.938Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/crypto/OSSLMLDSA.cpp:195-204
Timestamp: 2025-08-18T19:35:33.938Z
Learning: In OpenSSL, EVP_PKEY_verify_message does not exist. EVP_PKEY_verify_message_init is designed for streaming verification and should be paired with EVP_PKEY_CTX_set_signature, EVP_PKEY_verify_message_update, and EVP_PKEY_verify_message_final - not with EVP_PKEY_verify.
Applied to files:
src/lib/crypto/OSSLMLDSA.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/OSSLMLDSA.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
📚 Learning: 2026-01-10T15:07:10.701Z
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 837
File: src/lib/common/log.cpp:192-194
Timestamp: 2026-01-10T15:07:10.701Z
Learning: In SoftHSM, setLogFile and closeLogFile are designed to be called only during C_Initialize and C_Finalize respectively. Per PKCS#11 spec, C_Finalize behavior is undefined if called while other threads are making Cryptoki calls, so concurrent access between closeLogFile and softHSMLog would be API misuse. The implementation relies on these PKCS#11 threading guarantees rather than defensive locking in open/close operations.
Applied to files:
src/lib/SoftHSM.cpp
🧬 Code graph analysis (4)
src/lib/crypto/AsymmetricAlgorithm.h (1)
src/lib/test/TestsNoPINInitBase.cpp (1)
ptr(60-60)
src/lib/crypto/MLDSAPrivateKey.cpp (3)
src/lib/crypto/MLDSAPublicKey.cpp (12)
isOfType(17-20)isOfType(17-17)getParameterSet(28-39)getParameterSet(28-28)getOutputLength(42-53)getOutputLength(42-42)setValue(60-63)setValue(60-60)serialise(66-69)serialise(66-66)deserialise(71-83)deserialise(71-71)src/lib/crypto/OSSLMLDSAPrivateKey.cpp (4)
isOfType(90-93)isOfType(90-90)setValue(95-103)setValue(95-95)src/lib/crypto/MLDSAParameters.cpp (6)
getParameterSet(22-25)getParameterSet(22-22)serialise(34-37)serialise(34-34)deserialise(39-50)deserialise(39-39)
src/lib/SoftHSM.cpp (1)
src/lib/crypto/MLDSAUtil.cpp (6)
getMLDSAPrivateKey(11-41)getMLDSAPrivateKey(11-11)getMLDSAPublicKey(43-69)getMLDSAPublicKey(43-43)setMLDSAPrivateKey(71-128)setMLDSAPrivateKey(71-71)
src/lib/crypto/MLDSAUtil.h (1)
src/lib/crypto/MLDSAUtil.cpp (8)
getMLDSAPrivateKey(11-41)getMLDSAPrivateKey(11-11)getMLDSAPublicKey(43-69)getMLDSAPublicKey(43-43)setMLDSAPrivateKey(71-128)setMLDSAPrivateKey(71-71)copyMLDSAContext(130-191)copyMLDSAContext(130-130)
🪛 Clang (14.0.6)
src/lib/crypto/AsymmetricAlgorithm.h
[error] 36-36: 'cstdlib' file not found
(clang-diagnostic-error)
src/lib/crypto/MLDSAPrivateKey.cpp
[error] 7-7: 'config.h' file not found
(clang-diagnostic-error)
src/lib/crypto/MLDSAUtil.cpp
[error] 7-7: 'config.h' file not found
(clang-diagnostic-error)
src/lib/crypto/OSSLMLDSA.cpp
[error] 7-7: 'config.h' file not found
(clang-diagnostic-error)
src/lib/crypto/MLDSAUtil.h
[error] 10-10: 'config.h' file not found
(clang-diagnostic-error)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Windows (x64, botan)
- GitHub Check: Linux with OpenSSL 3.5.4
- GitHub Check: Windows (x86, openssl)
- GitHub Check: Windows (x64, openssl)
🔇 Additional comments (26)
src/lib/crypto/AsymmetricAlgorithm.h (4)
36-37: LGTM!The inclusion of
<cstdlib>and<cstring>is appropriate for thestd::malloc,std::free, andstd::memcpyusage in the new structs below. The static analysis warning about 'cstdlib' not found is a false positive due to project-specific include paths.
58-59: LGTM!The
MLDSAenum values are correctly added to bothAsymAlgo::TypeandAsymMech::Type, following the existing pattern established by other algorithms likeEDDSA.Also applies to: 98-99
123-131: LGTM!The
Hedgestruct cleanly encapsulates the PKCS#11 3.2 hedge variant options for ML-DSA signing operations.
164-175: LGTM!The
MLDSAAdditionalContextGuardcorrectly implements RAII cleanup for the contiguous blob allocation pattern. The explicit destructor call beforestd::free()is appropriate sinceSIGN_ADDITIONAL_CONTEXThas a non-trivial destructor, and theownsContextMemory = falseflag in blob allocations prevents double-free of the context data. Based on learnings, this aligns with the Session::setParameters deep-copy pattern.src/lib/crypto/MLDSAUtil.h (1)
1-32: LGTM!The header file cleanly declares the
MLDSAUtilutility class with appropriate static methods for ML-DSA key handling and context copying. TheWITH_ML_DSAguard ensures the code is only compiled when ML-DSA support is enabled. The static analysis warning about 'config.h' not found is a false positive.src/lib/crypto/MLDSAUtil.cpp (4)
11-41: LGTM!The
getMLDSAPrivateKeyfunction correctly handles both private and non-private keys, with proper input validation and error handling for decryption failures.
43-69: LGTM!The
getMLDSAPublicKeyfunction correctly handles public key extraction with the same validation and decryption pattern as the private key counterpart.
71-128: LGTM!The
setMLDSAPrivateKeyfunction correctly handles PKCS#8 decoding, optional encryption for private keys, and attribute setting. The cleanup pattern, while repetitive, is consistent with the existing codebase style. Based on learnings, the parameter set is correctly inferred from key material length viagetParameterSet().
130-191: LGTM!The
copyMLDSAContextfunction correctly implements the contiguous blob allocation pattern per the retrieved learnings. It properly validates the context length (max 255 bytes per PKCS#11 3.2 §6.67.5), maps all hedge variants including the default case, and handles memory allocation failures withCKR_HOST_MEMORY. TheownsContextMemory = falseflag ensures the blob is freed as a unit viafree(sac).src/lib/crypto/MLDSAPrivateKey.cpp (3)
13-24: LGTM!The
isOfTypeimplementation correctly handles NULL input and follows the established pattern in the codebase.
31-57: LGTM!The
getParameterSetandgetOutputLengthmethods correctly infer the ML-DSA parameter set from the private key material length, using the appropriateML_DSA_*_PRIV_LENGTHconstants. This aligns with the established pattern inMLDSAPublicKeyand the retrieved learnings about parameter set inference.
79-100: LGTM!The serialization correctly concatenates seed and value, while deserialization properly validates both components are non-empty before populating the key. The use of
chainDeserialiseensures correct parsing of the concatenated data.src/lib/crypto/OSSLMLDSA.cpp (8)
96-151: LGTM!The signing implementation correctly handles all error paths with proper resource cleanup for both
EVP_PKEY_CTXandEVP_SIGNATURE. The two-phase signing (size query followed by actual sign) follows OpenSSL best practices.
176-308: LGTM!The verification implementation correctly uses
EVP_PKEY_verify_message_initpaired withEVP_PKEY_verifyper the retrieved learnings. The signature length validation, parameter handling, and error/resource cleanup mirror the signing flow appropriately.
153-173: LGTM!The stub functions correctly reject unsupported operations (multi-part signing/verification and encryption/decryption) with appropriate error messages. This aligns with PKCS#11 3.2 which specifies ML-DSA as single-part only.
Also applies to: 310-365
350-358: LGTM!The key size functions correctly return ML-DSA private key lengths in bytes, which aligns with PKCS#11 v3.2 chapter 6.67.4 (per retrieved learnings, ML-DSA key sizes are specified in bytes unlike other algorithms that use bits).
430-512: LGTM!The key reconstruction functions follow the established pattern in the codebase, with proper deserialization, validation, and cleanup on failure.
514-549: LGTM!The factory methods correctly instantiate the appropriate OpenSSL-backed key and parameter types.
414-428: No issue withEVP_PKEY_freecall on line 425.
setFromOSSLextracts and copies key material into the key objects usingEVP_PKEY_get_octet_string_param(), not storing the incomingpkeypointer. The EVP_PKEY is safely freed after bothsetFromOSSLcalls.
72-94: No issue. The const-cast on line 83 is safe and necessary for OpenSSL API compatibility.OSSL_PARAM_construct_octet_stringdoes not modify the input data—it only stores a pointer to it. The data lifetime is correctly maintained (thecontextDatapointer from theadditionalContextparameter remains valid through the signing operation), and the non-const signature is a quirk of OpenSSL's API design.src/lib/SoftHSM.cpp (6)
65-69: Unconditional ML-DSA includes align with SoftHSM include policy.Keeping
MLDSA*headers unconditional here is consistent with the project approach (feature gating via mechanism tables /WITH_*, not includes). Based on learnings, the include policy is intentional.
148-152: ML-DSA object instantiation wiring looks correct.
CKK_ML_DSA→P11MLDSAPublicKeyObj/P11MLDSAPrivateKeyObjunderWITH_ML_DSAmatches the existing pattern for other key types.Also applies to: 169-172
949-952: ML-DSA mechanism info: bytes-vs-bits handling is correct; keep it consistent end-to-end.The comment + plumbing for ML-DSA min/max sizes being bytes (not bits) is correct for PKCS#11 v3.2 ML-DSA. Based on learnings,
OSSLMLDSA::{getMinKeySize,getMaxKeySize}are in bytes, so this aligns.Also applies to: 1051-1061, 1353-1364
5790-5791: Debug logging is safe and useful here.Logging only
(expected size, provided size)on signature-length mismatch helps diagnose interop issues without leaking sensitive material.Also applies to: 5974-5975
6284-6288: Keypair-gen routing for ML-DSA looks consistent.
CKM_ML_DSA_KEY_PAIR_GENrouting togenerateMLDSA()and the template mismatch checks follow existing patterns.Also applies to: 6316-6318, 6341-6343, 6416-6425
4204-4208: TheCK_SIGN_ADDITIONAL_CONTEXTownership semantics are correct; no UAF/double-free risk.The code is safe as implemented:
MLDSAUtil::copyMLDSAContext()allocates a buffer (buffer A) and stores it inmldsaCtxGuard.ptrsession->setParameters(param, paramLen)allocates its own independent buffer (buffer B) and deep-copies the data from buffer A into it, then fixes up internal pointers in buffer B- The guard's destructor frees buffer A when scope exits
session->resetOp()later frees buffer BNo transfer of ownership is needed, and the current approach avoids the complexity of transient buffer passing. The design is sound.
antoinelochet
force-pushed
the
ml-dsa-pr
branch
from
0009185 to
52c360c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/SoftHSM.cpp (1)
6502-6619: Bug:WrapKeySymusesblocksizefor CBC IV but never sets it forCKM_AES_CBC/CKM_DES3_CBC. This makesiv.resize(blocksize)zero-length and will likely break AES-CBC wrapping (and DES3-CBC if reachable).Proposed fix
--- a/src/lib/SoftHSM.cpp +++ b/src/lib/SoftHSM.cpp @@ switch(pMechanism->mechanism) { @@ case CKM_AES_CBC: algo = SymAlgo::AES; + blocksize = 16; break; case CKM_AES_CBC_PAD: blocksize = 16; wrappedlen = RFC5652Pad(keydata, blocksize); algo = SymAlgo::AES; break; @@ case CKM_DES3_CBC: algo = SymAlgo::DES3; + blocksize = 8; break; case CKM_DES3_CBC_PAD: blocksize = 8; wrappedlen = RFC5652Pad(keydata, blocksize); algo = SymAlgo::DES3; break;
🧹 Nitpick comments (2)
src/lib/SoftHSM.cpp (2)
4204-4208: ML-DSACK_SIGN_ADDITIONAL_CONTEXThandling + deep-copy approach looks solid. TheMLDSAUtil::copyMLDSAContext()usage + passing a contiguousSIGN_ADDITIONAL_CONTEXTblob intoSession::setParameters()aligns with the “Session owns a deep copy” approach (based on learnings).Also applies to: 4475-4505, 4593-4612, 5266-5269, 5535-5564, 5651-5670
7137-7146: CBC_PAD unwrap path looks coherent (manual unpad after decrypt). Consider using a more specificCKR_*thanCKR_GENERAL_ERRORfor padding failures when you have time.Also applies to: 7176-7204
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/lib/SoftHSM.cppsrc/lib/crypto/AsymmetricAlgorithm.hsrc/lib/crypto/MLDSAPrivateKey.cppsrc/lib/crypto/MLDSAUtil.cppsrc/lib/crypto/MLDSAUtil.hsrc/lib/crypto/OSSLMLDSA.cppsrc/lib/session_mgr/Session.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/session_mgr/Session.cpp
🧰 Additional context used
🧠 Learnings (13)
📓 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: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: 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: 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.
📚 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/AsymmetricAlgorithm.hsrc/lib/crypto/MLDSAUtil.cppsrc/lib/crypto/MLDSAPrivateKey.cppsrc/lib/SoftHSM.cppsrc/lib/crypto/OSSLMLDSA.cppsrc/lib/crypto/MLDSAUtil.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/AsymmetricAlgorithm.hsrc/lib/crypto/MLDSAUtil.cppsrc/lib/crypto/MLDSAPrivateKey.cppsrc/lib/SoftHSM.cppsrc/lib/crypto/OSSLMLDSA.cppsrc/lib/crypto/MLDSAUtil.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/crypto/AsymmetricAlgorithm.hsrc/lib/crypto/MLDSAUtil.cppsrc/lib/crypto/MLDSAPrivateKey.cppsrc/lib/SoftHSM.cppsrc/lib/crypto/OSSLMLDSA.cppsrc/lib/crypto/MLDSAUtil.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/AsymmetricAlgorithm.hsrc/lib/crypto/MLDSAUtil.cppsrc/lib/SoftHSM.cppsrc/lib/crypto/OSSLMLDSA.cppsrc/lib/crypto/MLDSAUtil.h
📚 Learning: 2025-11-11T19:42:46.886Z
Learnt from: JoaoHenrique12
Repo: softhsm/SoftHSMv2 PR: 823
File: src/lib/SoftHSM.cpp:58-58
Timestamp: 2025-11-11T19:42:46.886Z
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/AsymmetricAlgorithm.hsrc/lib/crypto/MLDSAUtil.cppsrc/lib/SoftHSM.cppsrc/lib/crypto/OSSLMLDSA.cppsrc/lib/crypto/MLDSAUtil.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:
src/lib/crypto/AsymmetricAlgorithm.hsrc/lib/crypto/MLDSAUtil.cppsrc/lib/crypto/MLDSAPrivateKey.cppsrc/lib/SoftHSM.cppsrc/lib/crypto/OSSLMLDSA.cppsrc/lib/crypto/MLDSAUtil.h
📚 Learning: 2026-01-11T16:15:14.696Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/crypto/OSSLMLDSA.cpp:373-381
Timestamp: 2026-01-11T16:15:14.696Z
Learning: In PKCS#11 v3.2, ML-DSA key sizes (returned by getMinKeySize() and getMaxKeySize()) are specified in bytes, not bits. This is different from other algorithms like RSA, DSA, ECDSA, ECDH, and EdDSA which use bits. This is defined in chapter 6.67.4 of the PKCS#11 3.2 specification.
Applied to files:
src/lib/crypto/AsymmetricAlgorithm.hsrc/lib/crypto/MLDSAUtil.cppsrc/lib/crypto/MLDSAPrivateKey.cppsrc/lib/SoftHSM.cppsrc/lib/crypto/OSSLMLDSA.cppsrc/lib/crypto/MLDSAUtil.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/MLDSAUtil.cppsrc/lib/SoftHSM.cppsrc/lib/crypto/OSSLMLDSA.cppsrc/lib/crypto/MLDSAUtil.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/SoftHSM.cpp
📚 Learning: 2026-01-10T15:07:10.701Z
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 837
File: src/lib/common/log.cpp:192-194
Timestamp: 2026-01-10T15:07:10.701Z
Learning: In SoftHSM, setLogFile and closeLogFile are designed to be called only during C_Initialize and C_Finalize respectively. Per PKCS#11 spec, C_Finalize behavior is undefined if called while other threads are making Cryptoki calls, so concurrent access between closeLogFile and softHSMLog would be API misuse. The implementation relies on these PKCS#11 threading guarantees rather than defensive locking in open/close operations.
Applied to files:
src/lib/SoftHSM.cpp
📚 Learning: 2025-08-18T19:35:33.938Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/crypto/OSSLMLDSA.cpp:195-204
Timestamp: 2025-08-18T19:35:33.938Z
Learning: In OpenSSL, EVP_PKEY_verify_message does not exist. EVP_PKEY_verify_message_init is designed for streaming verification and should be paired with EVP_PKEY_CTX_set_signature, EVP_PKEY_verify_message_update, and EVP_PKEY_verify_message_final - not with EVP_PKEY_verify.
Applied to files:
src/lib/crypto/OSSLMLDSA.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/OSSLMLDSA.cpp
🧬 Code graph analysis (4)
src/lib/crypto/MLDSAUtil.cpp (1)
src/lib/crypto/AsymmetricAlgorithm.h (1)
SIGN_ADDITIONAL_CONTEXT(133-157)
src/lib/crypto/MLDSAPrivateKey.cpp (2)
src/lib/crypto/MLDSAPublicKey.cpp (10)
getParameterSet(28-39)getParameterSet(28-28)getOutputLength(42-53)getOutputLength(42-42)setValue(60-63)setValue(60-60)serialise(66-69)serialise(66-66)deserialise(71-83)deserialise(71-71)src/lib/crypto/MLDSAParameters.cpp (6)
getParameterSet(22-25)getParameterSet(22-22)serialise(34-37)serialise(34-34)deserialise(39-50)deserialise(39-39)
src/lib/SoftHSM.cpp (1)
src/lib/crypto/MLDSAUtil.cpp (8)
copyMLDSAContext(130-191)copyMLDSAContext(130-130)getMLDSAPrivateKey(11-41)getMLDSAPrivateKey(11-11)getMLDSAPublicKey(43-69)getMLDSAPublicKey(43-43)setMLDSAPrivateKey(71-128)setMLDSAPrivateKey(71-71)
src/lib/crypto/OSSLMLDSA.cpp (2)
src/lib/crypto/AsymmetricAlgorithm.h (1)
SIGN_ADDITIONAL_CONTEXT(133-157)src/lib/crypto/OSSLUtil.cpp (2)
mldsaParameterSet2Name(226-235)mldsaParameterSet2Name(226-226)
🪛 Clang (14.0.6)
src/lib/crypto/AsymmetricAlgorithm.h
[error] 36-36: 'cstdlib' file not found
(clang-diagnostic-error)
src/lib/crypto/MLDSAUtil.cpp
[error] 7-7: 'config.h' file not found
(clang-diagnostic-error)
src/lib/crypto/MLDSAPrivateKey.cpp
[error] 7-7: 'config.h' file not found
(clang-diagnostic-error)
src/lib/crypto/OSSLMLDSA.cpp
[error] 7-7: 'config.h' file not found
(clang-diagnostic-error)
src/lib/crypto/MLDSAUtil.h
[error] 10-10: 'config.h' file not found
(clang-diagnostic-error)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Windows (x64, openssl)
- GitHub Check: Linux with OpenSSL 1.1.1
- GitHub Check: Linux with OpenSSL 3.5.4
- GitHub Check: Linux with Botan
- GitHub Check: macOS (openssl)
- GitHub Check: Windows (x86, openssl)
- GitHub Check: Windows (x64, botan)
- GitHub Check: Linux with OpenSSL 3.0
- GitHub Check: macOS (botan)
🔇 Additional comments (25)
src/lib/crypto/AsymmetricAlgorithm.h (3)
36-37: LGTM - Standard library includes for memory management.The
<cstdlib>and<cstring>includes are appropriate for thestd::malloc,std::free, andstd::memcpyoperations used in the newSIGN_ADDITIONAL_CONTEXTstruct.Note: The static analysis hint about
'cstdlib' file not foundis a false positive - this is a standard C++ header and builds correctly per the CI reports.
133-169: Well-designed SIGN_ADDITIONAL_CONTEXT with proper ownership semantics.The struct correctly implements:
- Ownership tracking via
ownsContextMemoryflag- Safe destructor that only frees when the struct owns the memory
- Non-copyable/non-movable to prevent double-free issues
- Exception safety by throwing
std::bad_allocon allocation failureBased on learnings, the
ownsContextMemory=truepath is for unit test convenience, whileSession::setParametersuses the single-blob allocation pattern withownsContextMemory=false.
171-182: Correct RAII guard for contiguous blob allocation.The destructor pattern of calling
ptr->~SIGN_ADDITIONAL_CONTEXT()followed bystd::free(ptr)is correct for the single-blob allocation used incopyMLDSAContext. SinceownsContextMemory=falsefor blob-allocated contexts, the destructor won't double-free the context data.src/lib/crypto/MLDSAPrivateKey.cpp (3)
17-29: LGTM - Type checking and bit length calculation.The
isOfTypecorrectly handles NULL input andgetBitLengthproperly delegates to theByteString::bits()method.
31-57: LGTM - Parameter set inference from key length.These methods correctly infer the ML-DSA parameter set from the private key length, consistent with the
MLDSAPublicKeypattern. Based on learnings, this inference approach is intentional - there's nosetParameterSet()as the parameter set is determined from key material length.
79-100: LGTM - Serialization with proper ordering.The
serialiseanddeserialisemethods maintain consistent ordering (seed then value) and correctly require both components to be present for a valid private key. The use ofchainDeserialisefor sequential extraction matches the pattern inMLDSAPublicKey.src/lib/crypto/OSSLMLDSA.cpp (6)
72-94: OSSL_PARAM construction looks correct, but the const cast is unavoidable here.The
OSSL_PARAM_construct_octet_stringAPI requires a non-const pointer even though it shouldn't modify the data. The cast on line 83 is a necessary workaround for the OpenSSL API design.The handling of
Hedge::Typecorrectly mapsDETERMINISTIC_REQUIREDtoOSSL_DETERMINISTICand all other cases (includingHEDGE_PREFERREDandHEDGE_REQUIRED) toOSSL_RANDOM, which aligns with the PKCS#11 v3.2 specification where hedging means randomized signatures.
289-308: Correct verification flow with proper error handling.The verification correctly distinguishes between signature mismatch (
verifyRV == 0, returns false silently) and actual errors (verifyRV != 0 && verifyRV != 1, logs error then returns false). Based on learnings, theEVP_PKEY_verify_message_init+EVP_PKEY_verifypairing is the correct OpenSSL pattern for ML-DSA.
153-173: Correct stubs for unsupported multi-part operations.Per PKCS#11 v3.2, ML-DSA is single-part only. These stubs correctly reject multi-part signing operations with clear error messages.
350-358: LGTM - Key sizes in bytes per PKCS#11 v3.2.Based on learnings, ML-DSA key sizes are specified in bytes (not bits like RSA/DSA/ECDSA), as defined in PKCS#11 3.2 chapter 6.67.4. The use of
ML_DSA_*_PRIV_LENGTHconstants is correct.
430-466: LGTM - Proper cleanup on deserialisation failure.The
reconstructKeyPaircorrectly handles the case where either public or private key fails to deserialise by deleting the allocated key pair before returning false.
414-428: No changes needed. The reference counting is correct.
setFromOSSLdoes not store a reference topkey. Instead, it extracts the raw key material (seed, private key, or public key) usingEVP_PKEY_get_octet_string_paramand stores only the extractedByteStringdata. Therefore, freeingpkeyimmediately after callingsetFromOSSLis safe and does not requireEVP_PKEY_up_ref.Likely an incorrect or invalid review comment.
src/lib/crypto/MLDSAUtil.h (1)
20-29: LGTM - Clean utility class declaration.The
MLDSAUtilclass provides a well-organized set of static utility functions for ML-DSA key and context management. The method signatures are appropriate for their purposes.src/lib/crypto/MLDSAUtil.cpp (3)
11-69: LGTM - Key retrieval with proper encryption handling.Both functions correctly handle the
CKA_PRIVATEattribute to determine whether decryption is needed. The private key function appropriately retrieves bothCKA_VALUEandCKA_SEED, while the public key function only needsCKA_VALUE.
71-128: LGTM - Proper PKCS#8 import with encryption handling.The function correctly:
- Validates that a token is available when the key is private
- Encrypts both seed and value before storing for private keys
- Uses
getParameterSet()which infers the parameter set from key material length (per learnings)- Cleans up resources on all code paths
173-191: Well-implemented single-blob allocation pattern.The contiguous allocation
[SIGN_ADDITIONAL_CONTEXT][context bytes]withownsContextMemory=falsecorrectly follows the pattern described in learnings, whereSession::resetOpfreesm_pParametersto release the entire blob.The pointer arithmetic
sac + 1correctly points past the struct header to the context data area, and standard struct alignment should ensure proper alignment for theunsigned chararray that follows.src/lib/SoftHSM.cpp (9)
65-68: Unconditional ML-DSA header includes look correct (matches SoftHSM include policy). As per learnings, keep feature gating out of includes and in mechanism tables/CryptoFactory.
148-172: ML-DSA object instantiation vianewP11Object(...)is wired correctly underWITH_ML_DSA.
840-844: Mechanism string registration forCKM_ML_DSA(_KEY_PAIR_GEN)looks good. (Thestd::out_of_rangecatch tweak is fine too.)Also applies to: 886-889
949-952:C_GetMechanismInfo: ML-DSA min/max sizing + flags look consistent with “sizes are bytes” intent. Please double-check theAsymAlgo::MLDSAbackend returns min/max in bytes as expected.Also applies to: 1052-1061, 1352-1364
1151-1154: Generic secret key max size change is consistent withgenerateGeneric()validation.
5789-5793: Extra debug logging on signature length mismatch is fine, but consider verbosity. This will be helpful for ML-DSA bring-up; just ensure debug logs don’t become too noisy in common failure paths.Also applies to: 5973-5977
6284-6288:C_GenerateKeyPairML-DSA routing looks correct (type checks +generateMLDSA).Also applies to: 6316-6318, 6341-6343, 6416-6425
7013-7017: ML-DSA wrap/unwrap integration viaMLDSAUtil::{get,set}*Keylooks consistent. Please verify the PKCS#8 encode/decode roundtrip for ML-DSA works across the supported OpenSSL provider setup.Also applies to: 7058-7061, 7738-7743
10135-10393:generateMLDSA(...)implementation looks consistent with existing keygen patterns (param-set validation, transactional writes, cleanup). Nice use ofCKP_ML_DSA_*constants andCKR_PARAMETER_SET_NOT_SUPPORTED(matches project preference per learnings).
Co-authored-by: kalvdans <github@kalvdans.no-ip.org>
Fixed compilation for mldsaAlgNameFromParameterSet as a constant
antoinelochet
force-pushed
the
ml-dsa-pr
branch
from
52c360c to
c62ce20
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@CMAKE-NOTES.md`:
- Around line 11-15: The MD010 lint failure is caused by hard tabs in the
CMAKE-NOTES.md lines listing cmake flags (e.g., the lines starting with
-DBUILD_TESTS=ON, -DDISABLE_NON_PAGED_MEMORY=ON, -DENABLE_EDDSA=ON,
-DENABLE_MLDSA=ON, -DWITH_MIGRATE=ON); replace each hard tab character with
spaces (use consistent spacing, e.g., one space between flag and description or
align columns with spaces) so the file no longer contains tabs and satisfies
markdownlint MD010.
In `@CMAKE-WIN-NOTES.md`:
- Line 55: The markdown line containing the CMake option "-DENABLE_MLDSA=ON
Enable support for ML-DSA" uses a hard tab that triggers MD010; replace the hard
tab with spaces so the option and its description are separated by spaces (e.g.,
align with spaces) to satisfy markdownlint, updating the line that references
ENABLE_MLDSA accordingly.
In `@README.md`:
- Line 85: Replace the hard tab characters in the README line containing the CLI
option "--enable-mldsa" so it uses spaces instead (e.g., align the description
with spaces instead of a tab) to satisfy MD010; update the line that currently
reads "--enable-mldsa\t\tEnable support for ML-DSA (default detect)" to use
space characters between the flag and its description.
In `@src/lib/crypto/MLDSAParameters.h`:
- Around line 68-69: The private member `parameterSet` in class MLDSAParameters
is uninitialized and may be read by getParameterSet() before setParameterSet()
is called; initialize `parameterSet` to a sensible default (e.g., 0) in the
MLDSAParameters constructor(s) so all constructors set this field, or initialize
it inline where declared, and ensure getParameterSet() then returns a
well-defined value.
In `@src/lib/session_mgr/Session.cpp`:
- Around line 360-369: When handling MLDSA in Session::… (mechanism ==
AsymMech::Type::MLDSA) validate the embedded SIGN_ADDITIONAL_CONTEXT before
adjusting pointers: after casting to SIGN_ADDITIONAL_CONTEXT* sac, ensure
sac->contextLen is non-negative and that sizeof(SIGN_ADDITIONAL_CONTEXT) +
sac->contextLen <= inParamLen; only then set sac->contextData =
reinterpret_cast<unsigned char*>(sac + 1). If the check fails treat the input as
invalid (return an error/abort processing) instead of repointing to avoid
out-of-bounds reads from param.
♻️ Duplicate comments (4)
.github/workflows/ci.yml (1)
97-98: Avoid hiding build/install errors in CI logs (Line 97-98).Redirecting output to files only makes failures harder to diagnose in Actions logs.
src/lib/crypto/OSSLMLDSAPrivateKey.h (1)
67-68: Fix include-guard closing comment typo.The closing comment still references the wrong guard name.
🛠️ Proposed fix
-#endif // !_SOFTHSM_V2_OSSLECPRIVATEKEY_H +#endif // !_SOFTHSM_V2_OSSLMLDSAPRIVATEKEY_Hsrc/lib/crypto/OSSLMLDSA.cpp (2)
118-141: Fix OpenSSL sign/verify API pairing (message vs one‑shot).
EVP_PKEY_sign_message_init/EVP_PKEY_verify_message_initare streaming APIs but are paired here withEVP_PKEY_sign/EVP_PKEY_verify. That mix is undefined and can ignore parameters or fail unexpectedly. Either switch to the streaming update/final sequence or use the one‑shot init APIs (*_init_ex) that matchEVP_PKEY_sign/verify. Based on learnings, ...🔧 Minimal one‑shot pairing fix (use *_init_ex)
- initRv = EVP_PKEY_sign_message_init(sctx, sig_alg, params); + initRv = EVP_PKEY_sign_init_ex(sctx, sig_alg, params); ... - initRv = EVP_PKEY_sign_message_init(sctx, sig_alg, NULL); + initRv = EVP_PKEY_sign_init_ex(sctx, sig_alg, NULL);- initRv = EVP_PKEY_verify_message_init(vctx, sig_alg, params); + initRv = EVP_PKEY_verify_init_ex(vctx, sig_alg, params); ... - initRv = EVP_PKEY_verify_message_init(vctx, sig_alg, NULL); + initRv = EVP_PKEY_verify_init_ex(vctx, sig_alg, NULL);Also applies to: 280-296
74-83: GuardcontextDatawhencontextLen > 0.If
contextLenis non‑zero butcontextDatais NULL,OSSL_PARAM_construct_octet_stringwill dereference a NULL pointer. Add an explicit NULL check in both sign and verify.🩹 Suggested guard (apply in both blocks)
- if (contextSize > 0) { + if (contextSize > 0) { + if (additionalContext->contextData == NULL) { + ERROR_MSG("Invalid parameters, context pointer is NULL"); + return false; + } if (contextSize > 255) { ERROR_MSG("Invalid parameters, context length > 255"); return false; }Also applies to: 232-241
🧹 Nitpick comments (5)
.github/workflows/ci.yml (1)
96-100: Verify OpenSSL libdir matches the hardcoded lib64 paths.
./configdoesn’t explicitly set--libdir, yetLDFLAGS/PKG_CONFIG_PATHtargetlib64. If OpenSSL installs intolibby default,ln -sf lib64 libwon’t create/usr/local/openssl-3.5/lib64, and the build may fail to locate libs. Please confirm the actual install layout on ubuntu-24.04 and align it explicitly (e.g., set--libdir=lib64, or switch paths toliband linklib64 -> lib).🔧 Example alignment (explicit lib64)
- ./config shared zlib no-ssl3 no-weak-ssl-ciphers --prefix=${{ env.OPENSSL_INSTALL_DIR }} --openssldir=${{ env.OPENSSL_INSTALL_DIR }} + ./config shared zlib no-ssl3 no-weak-ssl-ciphers --prefix=${{ env.OPENSSL_INSTALL_DIR }} --openssldir=${{ env.OPENSSL_INSTALL_DIR }} --libdir=lib64src/lib/crypto/OSSLMLDSAKeyPair.cpp (1)
9-9: Consider removing unused include.
log.his included but no logging macros (ERROR_MSG,DEBUG_MSG, etc.) are used in this file.Suggested fix
`#include` "config.h" `#ifdef` WITH_ML_DSA -#include "log.h" `#include` "OSSLMLDSAKeyPair.h"src/lib/crypto/OSSLMLDSAPrivateKey.cpp (1)
180-182: Minor: Inconsistent indentation.Lines 181-182 use spaces while surrounding code uses tabs. Consider aligning with project style.
Suggested fix
int selection = 0; - EVP_PKEY_CTX *ctx = NULL; - OSSL_PARAM params[3], *p = params; + EVP_PKEY_CTX *ctx = NULL; + OSSL_PARAM params[3], *p = params;src/lib/crypto/MLDSAParameters.h (1)
10-11: Consider removing unused includes.
<tuple>and<stdexcept>appear unused in this header. If they're needed in the implementation file, include them there instead.src/lib/crypto/OSSLMLDSA.h (1)
20-24: Minor indentation inconsistency.Lines 21-22 use spaces while other lines use tabs. Consider aligning for consistency.
Proposed fix
// Signing functions - virtual bool sign(PrivateKey *privateKey, const ByteString &dataToSign, ByteString &signature, const AsymMech::Type mechanism, const void *param = NULL, const size_t paramLen = 0); - virtual bool signInit(PrivateKey* privateKey, const AsymMech::Type mechanism, const void* param = NULL, const size_t paramLen = 0); + virtual bool sign(PrivateKey *privateKey, const ByteString &dataToSign, ByteString &signature, const AsymMech::Type mechanism, const void *param = NULL, const size_t paramLen = 0); + virtual bool signInit(PrivateKey* privateKey, const AsymMech::Type mechanism, const void* param = NULL, const size_t paramLen = 0); virtual bool signUpdate(const ByteString& dataToSign);
| -DBUILD_TESTS=ON Compile tests along with libraries | ||
| -DDISABLE_NON_PAGED_MEMORY=ON Disable non-paged memory for secure storage | ||
| -DENABLE_EDDSA=ON Enable support for EDDSA | ||
| -DENABLE_MLDSA=ON Enable support for ML-DSA | ||
| -DWITH_MIGRATE=ON Build migration tool |
There was a problem hiding this comment.
Replace hard tabs to satisfy MD010.
Static analysis flags hard tabs on the new line. Please switch to spaces to keep markdownlint clean.
🧹 Suggested fix
- -DENABLE_MLDSA=ON Enable support for ML-DSA
+ -DENABLE_MLDSA=ON Enable support for ML-DSA🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
11-11: Hard tabs
Column: 1
(MD010, no-hard-tabs)
11-11: Hard tabs
Column: 18
(MD010, no-hard-tabs)
12-12: Hard tabs
Column: 1
(MD010, no-hard-tabs)
12-12: Hard tabs
Column: 31
(MD010, no-hard-tabs)
13-13: Hard tabs
Column: 1
(MD010, no-hard-tabs)
13-13: Hard tabs
Column: 19
(MD010, no-hard-tabs)
14-14: Hard tabs
Column: 1
(MD010, no-hard-tabs)
14-14: Hard tabs
Column: 19
(MD010, no-hard-tabs)
15-15: Hard tabs
Column: 1
(MD010, no-hard-tabs)
15-15: Hard tabs
Column: 19
(MD010, no-hard-tabs)
🤖 Prompt for AI Agents
In `@CMAKE-NOTES.md` around lines 11 - 15, The MD010 lint failure is caused by
hard tabs in the CMAKE-NOTES.md lines listing cmake flags (e.g., the lines
starting with -DBUILD_TESTS=ON, -DDISABLE_NON_PAGED_MEMORY=ON,
-DENABLE_EDDSA=ON, -DENABLE_MLDSA=ON, -DWITH_MIGRATE=ON); replace each hard tab
character with spaces (use consistent spacing, e.g., one space between flag and
description or align columns with spaces) so the file no longer contains tabs
and satisfies markdownlint MD010.
| #ifdef WITH_ML_DSA | ||
| // Fix up SIGN_ADDITIONAL_CONTEXT pointer after deep copy | ||
| if (mechanism == AsymMech::Type::MLDSA && inParamLen > sizeof(SIGN_ADDITIONAL_CONTEXT)) | ||
| { | ||
| SIGN_ADDITIONAL_CONTEXT* sac = reinterpret_cast<SIGN_ADDITIONAL_CONTEXT*>(param); | ||
| if (sac->contextLen > 0 && sac->contextData != NULL) | ||
| { | ||
| // Adjust contextData to point into the new copied blob | ||
| sac->contextData = reinterpret_cast<unsigned char*>(sac + 1); | ||
| } |
There was a problem hiding this comment.
Validate contextLen against inParamLen before repointing.
If contextLen exceeds the copied blob size, downstream reads can run past param. Add a bounds check and treat mismatched lengths as invalid input.
🔧 Proposed fix
`#ifdef` WITH_ML_DSA
// Fix up SIGN_ADDITIONAL_CONTEXT pointer after deep copy
- if (mechanism == AsymMech::Type::MLDSA && inParamLen > sizeof(SIGN_ADDITIONAL_CONTEXT))
+ if (mechanism == AsymMech::Type::MLDSA && inParamLen >= sizeof(SIGN_ADDITIONAL_CONTEXT))
{
SIGN_ADDITIONAL_CONTEXT* sac = reinterpret_cast<SIGN_ADDITIONAL_CONTEXT*>(param);
- if (sac->contextLen > 0 && sac->contextData != NULL)
+ if (sac->contextLen > 0)
{
+ const size_t expected = sizeof(SIGN_ADDITIONAL_CONTEXT) + sac->contextLen;
+ if (inParamLen < expected) {
+ free(param);
+ param = NULL;
+ paramLen = 0;
+ return;
+ }
// Adjust contextData to point into the new copied blob
sac->contextData = reinterpret_cast<unsigned char*>(sac + 1);
+ }
+ else
+ {
+ sac->contextData = NULL;
}
}
`#endif`🤖 Prompt for AI Agents
In `@src/lib/session_mgr/Session.cpp` around lines 360 - 369, When handling MLDSA
in Session::… (mechanism == AsymMech::Type::MLDSA) validate the embedded
SIGN_ADDITIONAL_CONTEXT before adjusting pointers: after casting to
SIGN_ADDITIONAL_CONTEXT* sac, ensure sac->contextLen is non-negative and that
sizeof(SIGN_ADDITIONAL_CONTEXT) + sac->contextLen <= inParamLen; only then set
sac->contextData = reinterpret_cast<unsigned char*>(sac + 1). If the check fails
treat the input as invalid (return an error/abort processing) instead of
repointing to avoid out-of-bounds reads from param.
Applied RabbitAI suggested fixes
antoinelochet
force-pushed
the
ml-dsa-pr
branch
from
c62ce20 to
5fe2207
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/lib/crypto/AsymmetricAlgorithm.h`:
- Around line 144-157: The constructor SIGN_ADDITIONAL_CONTEXT throws
std::bad_alloc but the header doesn't include the declaration; add an `#include`
<new> to the top of the AsymmetricAlgorithm.h header so std::bad_alloc is
declared (ensure the include is added alongside the other standard includes
before the SIGN_ADDITIONAL_CONTEXT definition).
♻️ Duplicate comments (7)
README.md (1)
85-85: Hard tabs still present (duplicate issue).This line was previously flagged for MD010 violations (hard tabs). The issue remains unresolved. Please replace the hard tabs with spaces as suggested in the earlier review.
src/lib/session_mgr/Session.cpp (1)
345-369: Validate ML‑DSA context length against copied buffer size.
Without a bounds check, a craftedcontextLencan cause downstream reads pastparam. Based on learnings, add a length guard before repointing.🔧 Suggested fix
`#ifdef` WITH_ML_DSA // Fix up SIGN_ADDITIONAL_CONTEXT pointer after deep copy - if (mechanism == AsymMech::Type::MLDSA && inParamLen > sizeof(SIGN_ADDITIONAL_CONTEXT)) + if (mechanism == AsymMech::Type::MLDSA && inParamLen >= sizeof(SIGN_ADDITIONAL_CONTEXT)) { SIGN_ADDITIONAL_CONTEXT* sac = reinterpret_cast<SIGN_ADDITIONAL_CONTEXT*>(param); - if (sac->contextLen > 0 && sac->contextData != NULL) + if (sac->contextLen > 0) { + const size_t expected = sizeof(SIGN_ADDITIONAL_CONTEXT) + sac->contextLen; + if (inParamLen < expected) { + free(param); + param = NULL; + paramLen = 0; + return; + } // Adjust contextData to point into the new copied blob sac->contextData = reinterpret_cast<unsigned char*>(sac + 1); + } + else + { + sac->contextData = NULL; } } `#endif`src/lib/crypto/OSSLMLDSA.cpp (2)
72-84: Guard against NULL contextData when contextLen > 0.If
contextLen > 0butcontextDatais NULL,OSSL_PARAM_construct_octet_stringwill dereference a NULL pointer. Add a guard in both sign and verify paths.🐛 Suggested fix
if (additionalContext != NULL) { Hedge::Type type = additionalContext->hedgeType; size_t contextSize = additionalContext->contextLen; if (contextSize > 0) { + if (additionalContext->contextData == NULL) { + ERROR_MSG("Invalid parameters, context pointer is NULL"); + return false; + } if (contextSize > 255) { ERROR_MSG("Invalid parameters, context length > 255"); return false; } const unsigned char* contextAsChars = additionalContext->contextData; *p++ = OSSL_PARAM_construct_octet_string(OSSL_SIGNATURE_PARAM_CONTEXT_STRING, (unsigned char*)contextAsChars, contextSize); }if (additionalContext != NULL) { Hedge::Type type = additionalContext->hedgeType; size_t contextSize = additionalContext->contextLen; if (contextSize > 0) { + if (additionalContext->contextData == NULL) { + ERROR_MSG("Invalid parameters, context pointer is NULL"); + return false; + } if (contextSize > 255) { ERROR_MSG("Invalid parameters, context length > 255"); return false; } const unsigned char* contextAsChars = additionalContext->contextData; *p++ = OSSL_PARAM_construct_octet_string(OSSL_SIGNATURE_PARAM_CONTEXT_STRING, (unsigned char*)contextAsChars, contextSize); }Also applies to: 230-242
118-141: Don’t mix message-init with one‑shot sign/verify APIs.
EVP_PKEY_sign_message_init/EVP_PKEY_verify_message_initmust be paired with the message update/final API. UsingEVP_PKEY_sign/EVP_PKEY_verifyafter message-init is undefined and can ignore the ML‑DSA context parameters.🛠️ Suggested fix (message update/final flow)
- /* Calculate the required size for the signature by passing a NULL buffer. */ - if (EVP_PKEY_sign(sctx, NULL, &len, dataToSign.const_byte_str(), dataToSign.size()) <= 0) { - ERROR_MSG("ML-DSA sign size query failed (0x%08lX)", ERR_get_error()); - EVP_SIGNATURE_free(sig_alg); - EVP_PKEY_CTX_free(sctx); - return false; - } - signature.resize(len); - if (EVP_PKEY_sign(sctx, &signature[0], &len, dataToSign.const_byte_str(), dataToSign.size()) <= 0) { + if (EVP_PKEY_sign_message_update(sctx, dataToSign.const_byte_str(), dataToSign.size()) <= 0) { + ERROR_MSG("ML-DSA sign update failed (0x%08lX)", ERR_get_error()); + EVP_SIGNATURE_free(sig_alg); + EVP_PKEY_CTX_free(sctx); + return false; + } + if (EVP_PKEY_sign_message_final(sctx, NULL, &len) <= 0) { + ERROR_MSG("ML-DSA sign final size query failed (0x%08lX)", ERR_get_error()); + EVP_SIGNATURE_free(sig_alg); + EVP_PKEY_CTX_free(sctx); + return false; + } + signature.resize(len); + if (EVP_PKEY_sign_message_final(sctx, &signature[0], &len) <= 0) { ERROR_MSG("ML-DSA sign failed (0x%08lX)", ERR_get_error()); EVP_SIGNATURE_free(sig_alg); EVP_PKEY_CTX_free(sctx); return false; } + signature.resize(len);- int verifyRV = EVP_PKEY_verify(vctx, signature.const_byte_str(), signature.size(), - originalData.const_byte_str(), originalData.size()); + if (EVP_PKEY_CTX_set_signature(vctx, signature.const_byte_str(), signature.size()) <= 0) { + ERROR_MSG("ML-DSA set_signature failed (0x%08lX)", ERR_get_error()); + EVP_PKEY_CTX_free(vctx); + EVP_SIGNATURE_free(sig_alg); + return false; + } + if (EVP_PKEY_verify_message_update(vctx, originalData.const_byte_str(), originalData.size()) <= 0) { + ERROR_MSG("ML-DSA verify update failed (0x%08lX)", ERR_get_error()); + EVP_PKEY_CTX_free(vctx); + EVP_SIGNATURE_free(sig_alg); + return false; + } + int verifyRV = EVP_PKEY_verify_message_final(vctx); EVP_PKEY_CTX_free(vctx); EVP_SIGNATURE_free(sig_alg);OpenSSL 3.5 EVP_PKEY_sign_message_init usage with EVP_PKEY_sign_message_update and EVP_PKEY_sign_message_finalBased on learnings, the message-init APIs must be paired with update/final, not the one‑shot sign/verify calls.
Also applies to: 280-296
src/lib/SoftHSM.cpp (3)
5524-5553: Same paramLen/Session::setParameters coupling as sign‑init.
See the sign‑init comment about verifying pointer rebasing for ML‑DSA params.
6526-6575: CBC wrap still usesblocksize == 0for CKM_AES_CBC / CKM_DES3_CBC.
Non‑PAD CBC cases leaveblocksizeunset, so IV copy uses a 0‑byte IV and CBC init is incorrect. This was previously flagged.🐛 Suggested fix (same as prior)
case CKM_AES_CBC: - algo = SymAlgo::AES; + algo = SymAlgo::AES; + blocksize = 16; break; ... case CKM_DES3_CBC: - algo = SymAlgo::DES3; + algo = SymAlgo::DES3; + blocksize = 8; break;
10154-10383: GuardCKA_PARAMETER_SETpointer before dereference.
pPublicKeyTemplate[i].pValueis dereferenced without a NULL check. A malformed template can still hit this path even ifulValueLenmatches, causing a NULL deref. This has been raised previously.🛠️ Proposed fix
case CKA_PARAMETER_SET: - if (pPublicKeyTemplate[i].ulValueLen != sizeof(CK_ULONG)) { + if (pPublicKeyTemplate[i].pValue == NULL_PTR || + pPublicKeyTemplate[i].ulValueLen != sizeof(CK_ULONG)) { INFO_MSG("CKA_PARAMETER_SET must be sizeof(CK_ULONG)"); return CKR_ATTRIBUTE_VALUE_INVALID; } paramSet = *(CK_ULONG*)pPublicKeyTemplate[i].pValue; break;
| SIGN_ADDITIONAL_CONTEXT(Hedge::Type ht, const unsigned char* buf, size_t len) | ||
| : hedgeType(ht), contextLen(0), contextData(NULL), ownsContextMemory(false) | ||
| { | ||
| if (len > 0) { | ||
| contextData = static_cast<unsigned char*>(std::malloc(len)); | ||
| if (contextData) { | ||
| std::memcpy(contextData, buf, len); | ||
| contextLen = len; | ||
| ownsContextMemory = true; | ||
| } | ||
| else | ||
| { | ||
| throw std::bad_alloc(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the file to see current includes and the code in question
cat -n src/lib/crypto/AsymmetricAlgorithm.h | head -160Repository: softhsm/SoftHSMv2
Length of output: 5151
Add <new> to declare std::bad_alloc.
This header throws std::bad_alloc (line 156) but doesn't include <new>. Relying on transitive includes can break builds.
🔧 Suggested fix
`#include` <cstdlib>
`#include` <cstring>
+#include <new>🤖 Prompt for AI Agents
In `@src/lib/crypto/AsymmetricAlgorithm.h` around lines 144 - 157, The constructor
SIGN_ADDITIONAL_CONTEXT throws std::bad_alloc but the header doesn't include the
declaration; add an `#include` <new> to the top of the AsymmetricAlgorithm.h
header so std::bad_alloc is declared (ensure the include is added alongside the
other standard includes before the SIGN_ADDITIONAL_CONTEXT definition).
7 participants
Adds ML-DSA support with OpenSSL >= 3.5
Summary by CodeRabbit
New Features
Build & Configuration
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.