Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds ENABLE_MLDSA/WITH_ML_DSA build flags and implements end-to-end ML‑DSA: configure probes, build integration, core types and OpenSSL backend, PKCS#11 object/attribute support, mechanism‑param/session plumbing, tests, and a CI job for Ubuntu + OpenSSL 3.5.5. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,200,255,0.5)
participant Client as Client
participant SoftHSM as SoftHSM
participant CryptoFactory as CryptoFactory
participant OSSLMLDSA as OSSLMLDSA
participant OpenSSL as OpenSSL
end
Client->>SoftHSM: C_GenerateKeyPair(CKM_ML_DSA_KEY_PAIR_GEN, params)
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: return keypair wrapper
SoftHSM->>SoftHSM: create P11 objects (CKA_PARAMETER_SET, CKA_VALUE, CKA_SEED)
SoftHSM-->>Client: return object handles
sequenceDiagram
rect rgba(200,255,200,0.5)
participant Client as Client
participant SoftHSM as SoftHSM
participant Token as Token
participant OSSLMLDSA as OSSLMLDSA
participant OpenSSL as OpenSSL
end
Client->>SoftHSM: C_SignInit(hKey, CKM_ML_DSA, pParameter)
SoftHSM->>SoftHSM: Session::setMechanismParam(hedge/context)
SoftHSM->>Token: retrieve/decrypt CKA_VALUE and CKA_SEED if private
Token-->>SoftHSM: key bytes
SoftHSM->>OSSLMLDSA: sign(privateKey, data, mechanismParam)
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
✨ Finishing Touches🧪 Generate unit tests (beta)
|
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. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/lib/crypto/OSSLMLDSAPrivateKey.cpp (1)
62-87:⚠️ Potential issue | 🟠 MajorMake PKCS#8 decode fail atomically and free
localPKeyon the failure path.
setFromOSSL()still mutates the object before it knows the private value was extracted successfully, andPKCS8Decode()treatsvalue.size()as the success signal. On a reused instance, a failed extract can therefore return success with stale key material. Also, thereturn falseat Line 149 leakslocalPKey.Have
setFromOSSL()returnbool, populate locals first, commitsetSeed()/setValue()only on success, and freelocalPKeybefore every failure return inPKCS8Decode().Suggested direction
-void OSSLMLDSAPrivateKey::setFromOSSL(const EVP_PKEY* inMLDSAKEY) +bool OSSLMLDSAPrivateKey::setFromOSSL(const EVP_PKEY* inMLDSAKEY) { + ByteString newSeed; ... - if(rv && seed_len == 32) { - ByteString seedBS = ByteString(seed, seed_len); - setSeed(seedBS); - } + if (rv && seed_len == 32) + newSeed = ByteString(seed, seed_len); ... if(!rv) { ERROR_MSG("Could not get private key, rv: %d", rv); - return; + return false; } - ByteString valueBS = ByteString(priv, priv_len); - - setValue(valueBS); + if (newSeed.size() != 0) setSeed(newSeed); + setValue(ByteString(priv, priv_len)); + return true; } bool OSSLMLDSAPrivateKey::PKCS8Decode(const ByteString& ber) { ... - setFromOSSL(localPKey); - if (value.size() == 0) return false; + if (!setFromOSSL(localPKey)) + { + EVP_PKEY_free(localPKey); + return false; + } EVP_PKEY_free(localPKey); return true; }Also applies to: 138-150
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/crypto/OSSLMLDSAPrivateKey.cpp` around lines 62 - 87, Change setFromOSSL to return bool and stop mutating the object until the private material is fully extracted: extract seed and priv into local buffers/ByteStrings first, validate both extractions succeeded, then call setSeed() and setValue() only on success; update all callers as needed. In PKCS8Decode, ensure localPKey is freed on every failure path before returning (including the current return false at the end) and avoid relying on value.size() as the sole success signal; use the boolean result from setFromOSSL or explicit checks of the local ByteStrings to decide success and only commit state after successful decode.src/lib/SoftHSM.cpp (1)
4487-4499:⚠️ Potential issue | 🔴 CriticalRestore full
CK_SIGN_ADDITIONAL_CONTEXTvalidation.The old ML-DSA path rejected contexts longer than 255 bytes; that check is gone in both init branches. The verify branch at Line 5559 also dropped the
pContext != NULL_PTRguard before constructingByteString, soulContextLen > 0can still dereference a null pointer.🩹 Minimal fix
- if (ckSignAdditionalContext->ulContextLen > 0) { + if (ckSignAdditionalContext->ulContextLen > 255) { + ERROR_MSG("Invalid parameters"); + return CKR_ARGUMENTS_BAD; + } + if (ckSignAdditionalContext->ulContextLen > 0) { + if (ckSignAdditionalContext->pContext == NULL_PTR) { + ERROR_MSG("Invalid parameters"); + return CKR_ARGUMENTS_BAD; + } mldsaParam.additionalContext = ByteString(ckSignAdditionalContext->pContext, ckSignAdditionalContext->ulContextLen); }Apply the same
ulContextLen > 255guard inAsymSignInitas well.Based on learnings: SoftHSM ML‑DSA additional context handling should reject
ulContextLen > 255and validatepContextbefore forwarding the context.Also applies to: 5553-5560
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/SoftHSM.cpp` around lines 4487 - 4499, The CK_SIGN_ADDITIONAL_CONTEXT handling currently misses the original validations: add a guard rejecting ulContextLen > 255 and ensure pContext is non-NULL before constructing ByteString; specifically update the AsymSignInit path where CK_SIGN_ADDITIONAL_CONTEXT* ckSignAdditionalContext is used (after calling MLDSAUtil::setHedge and before assigning mldsaParam.additionalContext) to return CKR_ARGUMENTS_BAD if ulContextLen > 255 or if ulContextLen > 0 && pContext == NULL_PTR, and apply the same checks to the corresponding verify/init branch that constructs ByteString (the branch referenced around the verify path where ByteString is created) so neither path can dereference a null pContext or accept >255 bytes.
🧹 Nitpick comments (3)
src/lib/crypto/MLDSAParameters.h (1)
13-13: Unnecessary include ofMechanismParam.h.
MLDSAParametersextendsAsymmetricParameters, notMechanismParam. This include appears unused and can be removed to reduce compilation dependencies.♻️ Proposed fix
`#include` "ByteString.h" -#include "MechanismParam.h" `#include` "AsymmetricParameters.h"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/crypto/MLDSAParameters.h` at line 13, The header unnecessarily includes "MechanismParam.h" even though the class MLDSAParameters derives from AsymmetricParameters; remove the stray include to reduce compilation dependencies by deleting the `#include` "MechanismParam.h" line from the MLDSAParameters.h header so the file only relies on the correct AsymmetricParameters declaration.src/lib/crypto/MLDSAMechanismParam.cpp (1)
35-38: Consider adding<cstring>include forstrcmp.The
isOfType()function usesstrcmpbut no explicit<cstring>include is present. While it may be transitively included via other headers, adding the explicit include improves portability and makes the dependency clear.Suggested fix
`#include` "config.h" `#ifdef` WITH_ML_DSA `#include` "ByteString.h" +#include <cstring> `#include` "MechanismParam.h" `#include` "MLDSAMechanismParam.h"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/crypto/MLDSAMechanismParam.cpp` around lines 35 - 38, The isOfType method in MLDSAMechanismParam uses strcmp but the file lacks an explicit `#include` <cstring>; add the explicit include at the top of src/lib/crypto/MLDSAMechanismParam.cpp so MLDSAMechanismParam::isOfType can reliably call strcmp across platforms and compilers, keeping the existing implementation (return !strcmp(type, inType)).src/lib/crypto/OSSLMLDSA.cpp (1)
89-97: Consider adding explicitHEDGE_PREFERREDcase for consistency withverify().The
verify()function (lines 250-258) explicitly listsHEDGE_PREFERREDas a case label beforeHEDGE_REQUIRED, whilesign()only hasHEDGE_REQUIREDwithdefault. Both produce the same result (OSSL_RANDOM), but matching the pattern would improve code clarity and maintainability.♻️ Suggested alignment with verify()
switch (type) { case Hedge::Type::DETERMINISTIC_REQUIRED: *p++ = OSSL_PARAM_construct_int(OSSL_SIGNATURE_PARAM_DETERMINISTIC, &OSSL_DETERMINISTIC); break; + case Hedge::Type::HEDGE_PREFERRED: case Hedge::Type::HEDGE_REQUIRED: default: *p++ = OSSL_PARAM_construct_int(OSSL_SIGNATURE_PARAM_DETERMINISTIC, &OSSL_RANDOM); break; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/crypto/OSSLMLDSA.cpp` around lines 89 - 97, The switch in sign() that handles Hedge::Type currently only has a HEDGE_REQUIRED case and a default; to mirror verify() and improve clarity add an explicit case for Hedge::Type::HEDGE_PREFERRED before the HEDGE_REQUIRED case (falling through to set OSSL_SIGNATURE_PARAM_DETERMINISTIC to OSSL_RANDOM). Update the switch in the function that constructs OSSL_PARAMs so that Hedge::Type::HEDGE_PREFERRED and Hedge::Type::HEDGE_REQUIRED both assign the OSSL_RANDOM value (using the same OSSL_PARAM_construct_int call), keeping OSSL_SIGNATURE_PARAM_DETERMINISTIC, OSSL_RANDOM and OSSL_DETERMINISTIC usage consistent with verify().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/session_mgr/Session.cpp`:
- Around line 368-372: The code in setParameters() is freeing mechanismParam
with free(), which mismatches its allocation (allocated with new) and can cause
undefined behavior; change the deallocation to use delete (or delete[] if
mechanismParam was allocated with new[]) for the mechanismParam member in
Session::setParameters(), mirroring the fix applied to resetOp(), and ensure
mechanismParam is set to NULL after deletion.
- Around line 196-199: The code in resetOp() is freeing mechanismParam with
free() even though mechanismParam is created via MLDSAMechanismParam::clone()
(which uses new); replace the free() call with the matching delete and null the
pointer: use delete mechanismParam; mechanismParam = NULL; to avoid mixing
malloc/free with new/delete and causing undefined behavior.
---
Duplicate comments:
In `@src/lib/crypto/OSSLMLDSAPrivateKey.cpp`:
- Around line 62-87: Change setFromOSSL to return bool and stop mutating the
object until the private material is fully extracted: extract seed and priv into
local buffers/ByteStrings first, validate both extractions succeeded, then call
setSeed() and setValue() only on success; update all callers as needed. In
PKCS8Decode, ensure localPKey is freed on every failure path before returning
(including the current return false at the end) and avoid relying on
value.size() as the sole success signal; use the boolean result from setFromOSSL
or explicit checks of the local ByteStrings to decide success and only commit
state after successful decode.
In `@src/lib/SoftHSM.cpp`:
- Around line 4487-4499: The CK_SIGN_ADDITIONAL_CONTEXT handling currently
misses the original validations: add a guard rejecting ulContextLen > 255 and
ensure pContext is non-NULL before constructing ByteString; specifically update
the AsymSignInit path where CK_SIGN_ADDITIONAL_CONTEXT* ckSignAdditionalContext
is used (after calling MLDSAUtil::setHedge and before assigning
mldsaParam.additionalContext) to return CKR_ARGUMENTS_BAD if ulContextLen > 255
or if ulContextLen > 0 && pContext == NULL_PTR, and apply the same checks to the
corresponding verify/init branch that constructs ByteString (the branch
referenced around the verify path where ByteString is created) so neither path
can dereference a null pContext or accept >255 bytes.
---
Nitpick comments:
In `@src/lib/crypto/MLDSAMechanismParam.cpp`:
- Around line 35-38: The isOfType method in MLDSAMechanismParam uses strcmp but
the file lacks an explicit `#include` <cstring>; add the explicit include at the
top of src/lib/crypto/MLDSAMechanismParam.cpp so MLDSAMechanismParam::isOfType
can reliably call strcmp across platforms and compilers, keeping the existing
implementation (return !strcmp(type, inType)).
In `@src/lib/crypto/MLDSAParameters.h`:
- Line 13: The header unnecessarily includes "MechanismParam.h" even though the
class MLDSAParameters derives from AsymmetricParameters; remove the stray
include to reduce compilation dependencies by deleting the `#include`
"MechanismParam.h" line from the MLDSAParameters.h header so the file only
relies on the correct AsymmetricParameters declaration.
In `@src/lib/crypto/OSSLMLDSA.cpp`:
- Around line 89-97: The switch in sign() that handles Hedge::Type currently
only has a HEDGE_REQUIRED case and a default; to mirror verify() and improve
clarity add an explicit case for Hedge::Type::HEDGE_PREFERRED before the
HEDGE_REQUIRED case (falling through to set OSSL_SIGNATURE_PARAM_DETERMINISTIC
to OSSL_RANDOM). Update the switch in the function that constructs OSSL_PARAMs
so that Hedge::Type::HEDGE_PREFERRED and Hedge::Type::HEDGE_REQUIRED both assign
the OSSL_RANDOM value (using the same OSSL_PARAM_construct_int call), keeping
OSSL_SIGNATURE_PARAM_DETERMINISTIC, OSSL_RANDOM and OSSL_DETERMINISTIC usage
consistent with verify().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 88accdff-d967-4c9f-ab4a-f2cbfc97b44d
📒 Files selected for processing (34)
src/lib/SoftHSM.cppsrc/lib/crypto/AsymmetricAlgorithm.cppsrc/lib/crypto/AsymmetricAlgorithm.hsrc/lib/crypto/BotanDSA.cppsrc/lib/crypto/BotanDSA.hsrc/lib/crypto/BotanECDSA.cppsrc/lib/crypto/BotanECDSA.hsrc/lib/crypto/BotanEDDSA.cppsrc/lib/crypto/BotanEDDSA.hsrc/lib/crypto/BotanRSA.cppsrc/lib/crypto/BotanRSA.hsrc/lib/crypto/CMakeLists.txtsrc/lib/crypto/MLDSAMechanismParam.cppsrc/lib/crypto/MLDSAMechanismParam.hsrc/lib/crypto/MLDSAParameters.cppsrc/lib/crypto/MLDSAParameters.hsrc/lib/crypto/MLDSAUtil.cppsrc/lib/crypto/MLDSAUtil.hsrc/lib/crypto/Makefile.amsrc/lib/crypto/MechanismParam.hsrc/lib/crypto/OSSLDSA.cppsrc/lib/crypto/OSSLDSA.hsrc/lib/crypto/OSSLECDSA.cppsrc/lib/crypto/OSSLECDSA.hsrc/lib/crypto/OSSLEDDSA.cppsrc/lib/crypto/OSSLEDDSA.hsrc/lib/crypto/OSSLMLDSA.cppsrc/lib/crypto/OSSLMLDSA.hsrc/lib/crypto/OSSLMLDSAPrivateKey.cppsrc/lib/crypto/OSSLRSA.cppsrc/lib/crypto/OSSLRSA.hsrc/lib/crypto/test/MLDSATests.cppsrc/lib/session_mgr/Session.cppsrc/lib/session_mgr/Session.h
🚧 Files skipped from review as they are similar to previous changes (7)
- src/lib/crypto/OSSLECDSA.h
- src/lib/crypto/BotanEDDSA.h
- src/lib/session_mgr/Session.h
- src/lib/crypto/Makefile.am
- src/lib/crypto/OSSLRSA.cpp
- src/lib/crypto/BotanDSA.cpp
- src/lib/crypto/BotanECDSA.cpp
antoinelochet
force-pushed
the
ml-dsa-pr
branch
6 times, most recently
from
84194cd to
a6161d4
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/lib/crypto/OSSLMLDSAPrivateKey.cpp (1)
62-87:⚠️ Potential issue | 🟡 MinorNon-atomic state update in
setFromOSSLcan leave object inconsistent.If private key extraction fails at line 79-81,
setSeed()may have already been called at line 71, leaving the object in an inconsistent state (seed updated, value stale/empty). This can cause issues since runtime ML-DSA keys infer parameter sets from key material length.Consider extracting to local variables first, then committing all changes only on full success:
🛠️ Proposed fix for atomic update
void OSSLMLDSAPrivateKey::setFromOSSL(const EVP_PKEY* inMLDSAKEY) { + ByteString localSeed; uint8_t seed[32]; size_t seed_len; int rv = EVP_PKEY_get_octet_string_param(inMLDSAKEY, OSSL_PKEY_PARAM_ML_DSA_SEED, seed, sizeof(seed), &seed_len); if(rv && seed_len == 32) { - // seed is not mandatory for OSSL key reconstruction - ByteString seedBS = ByteString(seed, seed_len); - setSeed(seedBS); + localSeed = ByteString(seed, seed_len); } // let's use max priv length uint8_t priv[MLDSAParameters::ML_DSA_87_PRIV_LENGTH]; size_t priv_len; rv = EVP_PKEY_get_octet_string_param(inMLDSAKEY, OSSL_PKEY_PARAM_PRIV_KEY, priv, sizeof(priv), &priv_len); if(!rv) { ERROR_MSG("Could not get private key, rv: %d", rv); return; } - ByteString valueBS = ByteString(priv, priv_len); - - setValue(valueBS); + // Commit state atomically after successful extraction + if (localSeed.size() > 0) { + setSeed(localSeed); + } + setValue(ByteString(priv, priv_len)); }Based on learnings: runtime ML-DSA keys infer the parameter set from key material length, so inconsistent state from partial updates could cause incorrect parameter set inference.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/crypto/OSSLMLDSAPrivateKey.cpp` around lines 62 - 87, In setFromOSSL, avoid partially mutating object state by first reading both octet params into local buffers/variables (e.g., local seed buffer/seed_len and priv buffer/priv_len) and only call setSeed(...) and setValue(...) after both EVP_PKEY_get_octet_string_param calls succeed and lengths are validated (seed_len==32 and priv_len matches MLDSAParameters::ML_DSA_87_PRIV_LENGTH or other valid length); on any failure, log/return without mutating the object's seed/value so the ML-DSA parameter inference (setFromOSSL / setSeed / setValue) remains atomic and consistent.src/lib/SoftHSM.cpp (1)
5553-5561:⚠️ Potential issue | 🔴 CriticalValidate
pContextbefore building the ML-DSA verify context.
AsymVerifyInitstill constructsByteString(ckSignAdditionalContext->pContext, ...)without the null check already present in the sign path. A malformed caller can passulContextLen > 0withpContext == NULL_PTRand crash here.Proposed fix
- if (ckSignAdditionalContext->ulContextLen > 0) { - mldsaParam.additionalContext = ByteString(ckSignAdditionalContext->pContext, ckSignAdditionalContext->ulContextLen); + if (ckSignAdditionalContext->ulContextLen > 0) { + if (ckSignAdditionalContext->pContext == NULL_PTR) { + ERROR_MSG("Invalid parameters"); + return CKR_ARGUMENTS_BAD; + } + mldsaParam.additionalContext = ByteString( + ckSignAdditionalContext->pContext, + ckSignAdditionalContext->ulContextLen); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/SoftHSM.cpp` around lines 5553 - 5561, AsymVerifyInit builds mldsaParam.additionalContext from CK_SIGN_ADDITIONAL_CONTEXT without checking pContext; add a null check before constructing ByteString: if ckSignAdditionalContext->ulContextLen > 0 ensure ckSignAdditionalContext->pContext != NULL_PTR and return CKR_ARGUMENTS_BAD (or set an error) if it's null; update the block in AsymVerifyInit handling CK_SIGN_ADDITIONAL_CONTEXT so it mirrors the sign path validation and only constructs ByteString(ckSignAdditionalContext->pContext, ...) when pContext is non-null.
🧹 Nitpick comments (7)
src/lib/crypto/Makefile.am (1)
33-37: Fix inconsistent indentation.Lines 35-37 use spaces for indentation while the rest of the file uses tabs. This inconsistency should be corrected for maintainability.
🔧 Proposed fix
libsofthsm_crypto_la_SOURCES = AESKey.cpp \ ... MLDSAMechanismParam.cpp \ MLDSAParameters.cpp \ - MLDSAPrivateKey.cpp \ - MLDSAPublicKey.cpp \ - MLDSAUtil.cpp \ + MLDSAPrivateKey.cpp \ + MLDSAPublicKey.cpp \ + MLDSAUtil.cpp \ RSAParameters.cpp \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/crypto/Makefile.am` around lines 33 - 37, Indentation is inconsistent in the src/lib/crypto/Makefile.am entries for MLDSAPrivateKey.cpp, MLDSAPublicKey.cpp, and MLDSAUtil.cpp (these lines use spaces while others use tabs); update those lines to use tabs to match the existing tab-based indentation used by MLDSAMechanismParam.cpp and MLDSAParameters.cpp so the file is uniformly tab-indented.src/lib/crypto/OSSLMLDSAPrivateKey.cpp (1)
181-183: Inconsistent indentation.Lines 182-183 use spaces instead of tabs, inconsistent with the rest of the file.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/crypto/OSSLMLDSAPrivateKey.cpp` around lines 181 - 183, Fix inconsistent indentation in OSSLMLDSAPrivateKey.cpp by replacing the space-indented lines that declare the variables (int selection = 0;, EVP_PKEY_CTX *ctx = NULL;, OSSL_PARAM params[3], *p = params;) so they use the same tab-based indentation as the surrounding file; locate the block where these symbols (selection, ctx, params, p) are declared and convert the leading spaces to tabs to match the file's existing style.src/lib/crypto/MLDSAMechanismParam.cpp (1)
29-32: Simplify clone() implementation.The explicit cast in
clone()is redundant since*thisis already of typeMLDSAMechanismParam.♻️ Proposed simplification
MLDSAMechanismParam* MLDSAMechanismParam::clone() const { - return new MLDSAMechanismParam(static_cast<const MLDSAMechanismParam&>(*this)); // call the copy ctor. + return new MLDSAMechanismParam(*this); // call the copy ctor. }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/crypto/MLDSAMechanismParam.cpp` around lines 29 - 32, The clone() implementation uses an unnecessary static_cast of *this; update MLDSAMechanismParam::clone to directly pass *this to the copy constructor (i.e., construct a new MLDSAMechanismParam from *this) instead of using static_cast<const MLDSAMechanismParam&>(*this), keeping the copy-constructor call semantics intact.src/lib/crypto/MLDSAParameters.cpp (1)
50-56: Use the already-extractedparametervariable instead of callinglong_val()again.Line 50 extracts the value into
parameter, validates it on line 51-54, but line 56 callsserialised.long_val()again instead of reusingparameter. This is redundant and could be inconsistent iflong_val()has any side effects.♻️ Proposed fix
unsigned long parameter = serialised.long_val(); const char* name = OSSL::mldsaParameterSet2Name(parameter); if (name == NULL) { return false; } - setParameterSet(serialised.long_val()); + setParameterSet(parameter); return true;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/crypto/MLDSAParameters.cpp` around lines 50 - 56, Replace the redundant call to serialised.long_val() with the already-extracted variable: after validating `parameter` and checking `OSSL::mldsaParameterSet2Name(parameter)` in MLDSAParameters.cpp, pass `parameter` into `setParameterSet` instead of calling `serialised.long_val()` again so the code uses the validated value and avoids potential side effects.src/lib/crypto/MLDSAParameters.h (1)
12-12: Consider removing unused include.
MechanismParam.his included but doesn't appear to be used byMLDSAParameters. This class derives fromAsymmetricParametersand only handles parameter sets and length constants.♻️ Proposed fix
`#include` "config.h" `#include` "ByteString.h" -#include "MechanismParam.h" `#include` "AsymmetricParameters.h"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/crypto/MLDSAParameters.h` at line 12, Remove the unused include of "MechanismParam.h" from MLDSAParameters.h: delete the `#include` "MechanismParam.h" line in the MLDSAParameters header (the class MLDSAParameters derives from AsymmetricParameters and does not reference MechanismParam), then rebuild to ensure no symbols from MechanismParam are required; if the build fails, instead forward-declare only the required types or re-add the include where the symbols are actually used.src/lib/crypto/OSSLMLDSA.h (1)
20-24: Minor indentation inconsistency.Lines 21-22 use 4-space indentation while lines 23-24 (and the rest of the file) use tabs. Consider aligning to tabs for consistency with the rest of the codebase.
♻️ Suggested 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, const MechanismParam* mechanismParam = NULL); - 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, const MechanismParam* mechanismParam = NULL); + 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);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/crypto/OSSLMLDSA.h` around lines 20 - 24, The sign and signInit declaration lines use spaces instead of tabs; update the indentation of the methods sign(PrivateKey*, const ByteString&, ByteString&, const AsymMech::Type, const void*, size_t, const MechanismParam*), signInit(PrivateKey*, const AsymMech::Type, const void*, size_t) to use tabs to match the rest of the file (and the indent style used by signUpdate and signFinal) so all four signing function declarations use consistent tab indentation.src/lib/crypto/OSSLMLDSA.cpp (1)
250-261: Consider removing the deterministic parameter construction from verify().The
OSSL_SIGNATURE_PARAM_DETERMINISTICparameter controls whether signing uses hedged (randomized) or deterministic mode. This setting is irrelevant during verification since verification doesn't involve randomness—it only checks if a signature matches the message and public key.While setting this parameter is harmless, removing it would clarify the code's intent and eliminate unnecessary parameter handling in the verification path.
♻️ Proposed simplification
if (signAdditionalContext != NULL) { - Hedge::Type type = signAdditionalContext->hedgeType; if (signAdditionalContext->additionalContext.size() > 0) { context = signAdditionalContext->additionalContext; size_t contextSize = context.size(); if (contextSize > 255) { ERROR_MSG("Invalid parameters, context length > 255"); return false; } *p++ = OSSL_PARAM_construct_octet_string(OSSL_SIGNATURE_PARAM_CONTEXT_STRING, context.byte_str(), contextSize); } - switch (type) { - case Hedge::Type::DETERMINISTIC_REQUIRED: - *p++ = OSSL_PARAM_construct_int(OSSL_SIGNATURE_PARAM_DETERMINISTIC, &OSSL_DETERMINISTIC); - break; - case Hedge::Type::HEDGE_PREFERRED: - case Hedge::Type::HEDGE_REQUIRED: - default: - *p++ = OSSL_PARAM_construct_int(OSSL_SIGNATURE_PARAM_DETERMINISTIC, &OSSL_RANDOM); - break; - } *p = OSSL_PARAM_construct_end(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/crypto/OSSLMLDSA.cpp` around lines 250 - 261, The verify() path is constructing OSSL_SIGNATURE_PARAM_DETERMINISTIC based on Hedge::Type (using OSSL_DETERMINISTIC/OSSL_RANDOM) even though verification doesn't need a deterministic/random setting; remove the switch that writes OSSL_SIGNATURE_PARAM_DETERMINISTIC in verify(), stop adding that param to the OSSL_PARAM array, and ensure you still terminate the params array with OSSL_PARAM_construct_end() (keep the surrounding params logic intact). Locate the code in verify() that references OSSL_SIGNATURE_PARAM_DETERMINISTIC and the Hedge::Type switch and delete that block so verification only sets parameters relevant to verification.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/crypto/MLDSAMechanismParam.h`:
- Around line 15-45: The default constructor for MLDSAMechanismParam currently
sets hedgeType to Hedge::HEDGE_REQUIRED but the PKCS#11 spec requires the
default be HEDGE_PREFERRED; update the MLDSAMechanismParam() implementation to
initialize hedgeType to Hedge::HEDGE_PREFERRED (ensure any constructor overloads
remain unchanged and only the no-argument constructor is modified so
MLDSAMechanismParam::hedgeType defaults to Hedge::HEDGE_PREFERRED).
---
Duplicate comments:
In `@src/lib/crypto/OSSLMLDSAPrivateKey.cpp`:
- Around line 62-87: In setFromOSSL, avoid partially mutating object state by
first reading both octet params into local buffers/variables (e.g., local seed
buffer/seed_len and priv buffer/priv_len) and only call setSeed(...) and
setValue(...) after both EVP_PKEY_get_octet_string_param calls succeed and
lengths are validated (seed_len==32 and priv_len matches
MLDSAParameters::ML_DSA_87_PRIV_LENGTH or other valid length); on any failure,
log/return without mutating the object's seed/value so the ML-DSA parameter
inference (setFromOSSL / setSeed / setValue) remains atomic and consistent.
In `@src/lib/SoftHSM.cpp`:
- Around line 5553-5561: AsymVerifyInit builds mldsaParam.additionalContext from
CK_SIGN_ADDITIONAL_CONTEXT without checking pContext; add a null check before
constructing ByteString: if ckSignAdditionalContext->ulContextLen > 0 ensure
ckSignAdditionalContext->pContext != NULL_PTR and return CKR_ARGUMENTS_BAD (or
set an error) if it's null; update the block in AsymVerifyInit handling
CK_SIGN_ADDITIONAL_CONTEXT so it mirrors the sign path validation and only
constructs ByteString(ckSignAdditionalContext->pContext, ...) when pContext is
non-null.
---
Nitpick comments:
In `@src/lib/crypto/Makefile.am`:
- Around line 33-37: Indentation is inconsistent in the
src/lib/crypto/Makefile.am entries for MLDSAPrivateKey.cpp, MLDSAPublicKey.cpp,
and MLDSAUtil.cpp (these lines use spaces while others use tabs); update those
lines to use tabs to match the existing tab-based indentation used by
MLDSAMechanismParam.cpp and MLDSAParameters.cpp so the file is uniformly
tab-indented.
In `@src/lib/crypto/MLDSAMechanismParam.cpp`:
- Around line 29-32: The clone() implementation uses an unnecessary static_cast
of *this; update MLDSAMechanismParam::clone to directly pass *this to the copy
constructor (i.e., construct a new MLDSAMechanismParam from *this) instead of
using static_cast<const MLDSAMechanismParam&>(*this), keeping the
copy-constructor call semantics intact.
In `@src/lib/crypto/MLDSAParameters.cpp`:
- Around line 50-56: Replace the redundant call to serialised.long_val() with
the already-extracted variable: after validating `parameter` and checking
`OSSL::mldsaParameterSet2Name(parameter)` in MLDSAParameters.cpp, pass
`parameter` into `setParameterSet` instead of calling `serialised.long_val()`
again so the code uses the validated value and avoids potential side effects.
In `@src/lib/crypto/MLDSAParameters.h`:
- Line 12: Remove the unused include of "MechanismParam.h" from
MLDSAParameters.h: delete the `#include` "MechanismParam.h" line in the
MLDSAParameters header (the class MLDSAParameters derives from
AsymmetricParameters and does not reference MechanismParam), then rebuild to
ensure no symbols from MechanismParam are required; if the build fails, instead
forward-declare only the required types or re-add the include where the symbols
are actually used.
In `@src/lib/crypto/OSSLMLDSA.cpp`:
- Around line 250-261: The verify() path is constructing
OSSL_SIGNATURE_PARAM_DETERMINISTIC based on Hedge::Type (using
OSSL_DETERMINISTIC/OSSL_RANDOM) even though verification doesn't need a
deterministic/random setting; remove the switch that writes
OSSL_SIGNATURE_PARAM_DETERMINISTIC in verify(), stop adding that param to the
OSSL_PARAM array, and ensure you still terminate the params array with
OSSL_PARAM_construct_end() (keep the surrounding params logic intact). Locate
the code in verify() that references OSSL_SIGNATURE_PARAM_DETERMINISTIC and the
Hedge::Type switch and delete that block so verification only sets parameters
relevant to verification.
In `@src/lib/crypto/OSSLMLDSA.h`:
- Around line 20-24: The sign and signInit declaration lines use spaces instead
of tabs; update the indentation of the methods sign(PrivateKey*, const
ByteString&, ByteString&, const AsymMech::Type, const void*, size_t, const
MechanismParam*), signInit(PrivateKey*, const AsymMech::Type, const void*,
size_t) to use tabs to match the rest of the file (and the indent style used by
signUpdate and signFinal) so all four signing function declarations use
consistent tab indentation.
In `@src/lib/crypto/OSSLMLDSAPrivateKey.cpp`:
- Around line 181-183: Fix inconsistent indentation in OSSLMLDSAPrivateKey.cpp
by replacing the space-indented lines that declare the variables (int selection
= 0;, EVP_PKEY_CTX *ctx = NULL;, OSSL_PARAM params[3], *p = params;) so they use
the same tab-based indentation as the surrounding file; locate the block where
these symbols (selection, ctx, params, p) are declared and convert the leading
spaces to tabs to match the file's existing style.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4db64896-4d51-48e8-88d4-cbc7b7006b7d
📒 Files selected for processing (34)
src/lib/SoftHSM.cppsrc/lib/crypto/AsymmetricAlgorithm.cppsrc/lib/crypto/AsymmetricAlgorithm.hsrc/lib/crypto/BotanDSA.cppsrc/lib/crypto/BotanDSA.hsrc/lib/crypto/BotanECDSA.cppsrc/lib/crypto/BotanECDSA.hsrc/lib/crypto/BotanEDDSA.cppsrc/lib/crypto/BotanEDDSA.hsrc/lib/crypto/BotanRSA.cppsrc/lib/crypto/BotanRSA.hsrc/lib/crypto/CMakeLists.txtsrc/lib/crypto/MLDSAMechanismParam.cppsrc/lib/crypto/MLDSAMechanismParam.hsrc/lib/crypto/MLDSAParameters.cppsrc/lib/crypto/MLDSAParameters.hsrc/lib/crypto/MLDSAUtil.cppsrc/lib/crypto/MLDSAUtil.hsrc/lib/crypto/Makefile.amsrc/lib/crypto/MechanismParam.hsrc/lib/crypto/OSSLDSA.cppsrc/lib/crypto/OSSLDSA.hsrc/lib/crypto/OSSLECDSA.cppsrc/lib/crypto/OSSLECDSA.hsrc/lib/crypto/OSSLEDDSA.cppsrc/lib/crypto/OSSLEDDSA.hsrc/lib/crypto/OSSLMLDSA.cppsrc/lib/crypto/OSSLMLDSA.hsrc/lib/crypto/OSSLMLDSAPrivateKey.cppsrc/lib/crypto/OSSLRSA.cppsrc/lib/crypto/OSSLRSA.hsrc/lib/crypto/test/MLDSATests.cppsrc/lib/session_mgr/Session.cppsrc/lib/session_mgr/Session.h
🚧 Files skipped from review as they are similar to previous changes (13)
- src/lib/crypto/OSSLDSA.cpp
- src/lib/crypto/BotanRSA.cpp
- src/lib/crypto/CMakeLists.txt
- src/lib/crypto/BotanEDDSA.h
- src/lib/crypto/BotanECDSA.cpp
- src/lib/crypto/BotanDSA.h
- src/lib/crypto/BotanDSA.cpp
- src/lib/crypto/BotanECDSA.h
- src/lib/crypto/OSSLEDDSA.h
- src/lib/crypto/OSSLRSA.cpp
- src/lib/crypto/BotanEDDSA.cpp
- src/lib/crypto/OSSLECDSA.h
- src/lib/crypto/AsymmetricAlgorithm.cpp
antoinelochet
force-pushed
the
ml-dsa-pr
branch
3 times, most recently
from
86fea71 to
a8cda23
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/lib/crypto/OSSLMLDSAPrivateKey.cpp (1)
146-151:⚠️ Potential issue | 🟠 MajorMake PKCS#8 decode succeed or fail atomically.
This still has the old partial-failure problem: on a reused object, Line 149 can fail silently and Line 150 may still pass because it reads the pre-existing
value. The earlyreturn falsealso skipsEVP_PKEY_free(localPKey).Proposed fix
-bool OSSLMLDSAPrivateKey::PKCS8Decode(const ByteString& ber) +bool OSSLMLDSAPrivateKey::PKCS8Decode(const ByteString& ber) { int len = ber.size(); if (len <= 0) return false; const unsigned char* priv = ber.const_byte_str(); PKCS8_PRIV_KEY_INFO* p8 = d2i_PKCS8_PRIV_KEY_INFO(NULL, &priv, len); if (p8 == NULL) return false; EVP_PKEY* localPKey = EVP_PKCS82PKEY(p8); PKCS8_PRIV_KEY_INFO_free(p8); if (localPKey == NULL) return false; - setFromOSSL(localPKey); - if (value.size() == 0) return false; + const bool ok = setFromOSSL(localPKey); EVP_PKEY_free(localPKey); - return true; + return ok; }-void OSSLMLDSAPrivateKey::setFromOSSL(const EVP_PKEY* inMLDSAKEY) +bool OSSLMLDSAPrivateKey::setFromOSSL(const EVP_PKEY* inMLDSAKEY) { ... if(!rv) { ERROR_MSG("Could not get private key, rv: %d", rv); - return; + return false; } ... setValue(ByteString(priv, priv_len)); + return true; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/crypto/OSSLMLDSAPrivateKey.cpp` around lines 146 - 151, The PKCS#8 decode must be atomic: do not mutate the object unless the decode fully succeeds and always free the temporary EVP_PKEY. Change the flow around EVP_PKCS82PKEY/PKCS8_PRIV_KEY_INFO_free/localPKey so you first obtain localPKey (via EVP_PKCS82PKEY(p8)), validate it and that the derived value would be non-empty, then call setFromOSSL(localPKey) only after those checks pass; ensure EVP_PKEY_free(localPKey) is called on every exit path (including failure paths) and remove early returns that skip freeing or that let a pre-existing value remain when decode fails.
🧹 Nitpick comments (3)
src/lib/session_mgr/Session.h (1)
110-111: Make the session mechanism-param API const-correct.
Session::setMechanismParam()clones the input, and the stored value is only forwarded to crypto backends asconst MechanismParam*. Exposing it here as mutable makes the session-owned copy writable from the outside for no benefit. Preferconst MechanismParam*on both the setter input and getter return type.Also applies to: 169-169
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/session_mgr/Session.h` around lines 110 - 111, Change the session mechanism-param API to be const-correct by updating the declarations and definitions of setMechanismParam and getMechanismParam to use const MechanismParam* (i.e., setMechanismParam(const MechanismParam*) and MechanismParam const* getMechanismParam()), and do the same for the second pair referenced around the other occurrence; ensure the setter still clones the input into the session-owned copy and that the getter returns a pointer-to-const to that internal copy so callers cannot mutate the stored object.src/lib/crypto/MLDSAParameters.cpp (1)
42-53: KeepMLDSAParametersvalidation backend-neutral.This deserializer only needs to accept or reject the three
CKP_ML_DSA_*values, but it currently reaches intoOSSLUtilto do that. A local switch over the PKCS#11 constants would keep this parameter class independent from the OpenSSL helper layer and avoid the hiddenWITH_ML_DSA⇒WITH_OPENSSLcoupling here.Based on learnings: 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.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/crypto/MLDSAParameters.cpp` around lines 42 - 53, The deserialiser currently calls OSSL::mldsaParameterSet2Name (OSSL::mldsaParameterSet2Name) which ties MLDSAParameters::deserialise to the OpenSSL helper; instead remove that dependency and validate the value returned by serialised.long_val() with a local switch over the PKCS#11 constants (e.g. the CKP_ML_DSA_* constants) inside MLDSAParameters::deserialise: accept and return true for the three allowed CKP_ML_DSA_* values and return false for anything else, thereby avoiding use of OSSL::mldsaParameterSet2Name and keeping the class backend-neutral.src/lib/crypto/OSSLRSA.cpp (1)
68-71: ForwardmechanismParamthrough the generic fallback paths.The new parameter is accepted here, but the fallback calls at Line 283 and Line 841 still drop it. Passing it through keeps this override aligned with the base API.
Proposed fix
- return AsymmetricAlgorithm::sign(privateKey, dataToSign, signature, mechanism, param, paramLen); + return AsymmetricAlgorithm::sign(privateKey, dataToSign, signature, mechanism, param, paramLen, mechanismParam); ... - return AsymmetricAlgorithm::verify(publicKey, originalData, signature, mechanism, param, paramLen); + return AsymmetricAlgorithm::verify(publicKey, originalData, signature, mechanism, param, paramLen, mechanismParam);Also applies to: 281-284, 677-680, 839-842
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/crypto/OSSLRSA.cpp` around lines 68 - 71, The OSSLRSA::sign implementation accepts the new mechanismParam but fails to pass it into the generic fallback sign calls; update the fallback invocations inside OSSLRSA::sign (the calls at the two fallback paths currently omitting mechanismParam) to include the mechanismParam argument so the override forwards the parameter through the base API; apply the same change to the other similar sign call sites referenced (the other occurrences in the file where the fallback sign is invoked) so every fallback call passes mechanismParam along with the existing arguments.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/crypto/OSSLMLDSAPrivateKey.cpp`:
- Around line 197-205: Update the failure checks for EVP_PKEY_fromdata_init and
EVP_PKEY_fromdata to detect negative return codes: change the conditionals that
currently read "if (!rv)" to "if (rv <= 0)" for both EVP_PKEY_fromdata_init(ctx)
and EVP_PKEY_fromdata(ctx, &pkey, selection, params) in OSSLMLDSAPrivateKey.cpp
so negative errors (e.g. -2) are treated as failures; keep the existing error
logging (ERROR_MSG) and cleanup (EVP_PKEY_CTX_free(ctx) and early return) paths
intact and ensure pkey is only used when rv > 0.
In `@src/lib/SoftHSM.cpp`:
- Around line 4468-4501: The ML-DSA mechanism handler currently accepts
CK_SIGN_ADDITIONAL_CONTEXT.ulContextLen without an upper bound; add a guard in
the CKM_ML_DSA case to reject ulContextLen > 255 by logging ERROR_MSG("Invalid
parameters") and returning CKR_ARGUMENTS_BAD (retain the existing null-pointer
check for pContext when ulContextLen > 0), and apply the same 255-byte
validation to the other ML-DSA handling block (the second CKM_ML_DSA branch
where mldsaParam.additionalContext is set) so oversized contexts are rejected at
the PKCS#11 boundary instead of being forwarded to the backend.
---
Duplicate comments:
In `@src/lib/crypto/OSSLMLDSAPrivateKey.cpp`:
- Around line 146-151: The PKCS#8 decode must be atomic: do not mutate the
object unless the decode fully succeeds and always free the temporary EVP_PKEY.
Change the flow around EVP_PKCS82PKEY/PKCS8_PRIV_KEY_INFO_free/localPKey so you
first obtain localPKey (via EVP_PKCS82PKEY(p8)), validate it and that the
derived value would be non-empty, then call setFromOSSL(localPKey) only after
those checks pass; ensure EVP_PKEY_free(localPKey) is called on every exit path
(including failure paths) and remove early returns that skip freeing or that let
a pre-existing value remain when decode fails.
---
Nitpick comments:
In `@src/lib/crypto/MLDSAParameters.cpp`:
- Around line 42-53: The deserialiser currently calls
OSSL::mldsaParameterSet2Name (OSSL::mldsaParameterSet2Name) which ties
MLDSAParameters::deserialise to the OpenSSL helper; instead remove that
dependency and validate the value returned by serialised.long_val() with a local
switch over the PKCS#11 constants (e.g. the CKP_ML_DSA_* constants) inside
MLDSAParameters::deserialise: accept and return true for the three allowed
CKP_ML_DSA_* values and return false for anything else, thereby avoiding use of
OSSL::mldsaParameterSet2Name and keeping the class backend-neutral.
In `@src/lib/crypto/OSSLRSA.cpp`:
- Around line 68-71: The OSSLRSA::sign implementation accepts the new
mechanismParam but fails to pass it into the generic fallback sign calls; update
the fallback invocations inside OSSLRSA::sign (the calls at the two fallback
paths currently omitting mechanismParam) to include the mechanismParam argument
so the override forwards the parameter through the base API; apply the same
change to the other similar sign call sites referenced (the other occurrences in
the file where the fallback sign is invoked) so every fallback call passes
mechanismParam along with the existing arguments.
In `@src/lib/session_mgr/Session.h`:
- Around line 110-111: Change the session mechanism-param API to be
const-correct by updating the declarations and definitions of setMechanismParam
and getMechanismParam to use const MechanismParam* (i.e.,
setMechanismParam(const MechanismParam*) and MechanismParam const*
getMechanismParam()), and do the same for the second pair referenced around the
other occurrence; ensure the setter still clones the input into the
session-owned copy and that the getter returns a pointer-to-const to that
internal copy so callers cannot mutate the stored object.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6ca4ebbb-2ed6-4e6f-9a0c-5955e62e3b32
📒 Files selected for processing (33)
src/lib/SoftHSM.cppsrc/lib/crypto/AsymmetricAlgorithm.cppsrc/lib/crypto/AsymmetricAlgorithm.hsrc/lib/crypto/BotanDSA.cppsrc/lib/crypto/BotanDSA.hsrc/lib/crypto/BotanECDSA.cppsrc/lib/crypto/BotanECDSA.hsrc/lib/crypto/BotanEDDSA.cppsrc/lib/crypto/BotanEDDSA.hsrc/lib/crypto/BotanRSA.cppsrc/lib/crypto/BotanRSA.hsrc/lib/crypto/CMakeLists.txtsrc/lib/crypto/MLDSAMechanismParam.cppsrc/lib/crypto/MLDSAMechanismParam.hsrc/lib/crypto/MLDSAParameters.cppsrc/lib/crypto/MLDSAUtil.cppsrc/lib/crypto/MLDSAUtil.hsrc/lib/crypto/Makefile.amsrc/lib/crypto/MechanismParam.hsrc/lib/crypto/OSSLDSA.cppsrc/lib/crypto/OSSLDSA.hsrc/lib/crypto/OSSLECDSA.cppsrc/lib/crypto/OSSLECDSA.hsrc/lib/crypto/OSSLEDDSA.cppsrc/lib/crypto/OSSLEDDSA.hsrc/lib/crypto/OSSLMLDSA.cppsrc/lib/crypto/OSSLMLDSA.hsrc/lib/crypto/OSSLMLDSAPrivateKey.cppsrc/lib/crypto/OSSLRSA.cppsrc/lib/crypto/OSSLRSA.hsrc/lib/crypto/test/MLDSATests.cppsrc/lib/session_mgr/Session.cppsrc/lib/session_mgr/Session.h
🚧 Files skipped from review as they are similar to previous changes (7)
- src/lib/crypto/BotanEDDSA.h
- src/lib/crypto/AsymmetricAlgorithm.cpp
- src/lib/crypto/BotanRSA.cpp
- src/lib/crypto/OSSLECDSA.h
- src/lib/crypto/BotanDSA.cpp
- src/lib/crypto/BotanECDSA.h
- src/lib/crypto/BotanECDSA.cpp
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lib/crypto/BotanDSA.cpp (2)
282-294:⚠️ Potential issue | 🟠 MajorForward
mechanismParamin the delegated verify path.Line 294 has the same drop-on-delegate problem as
sign(): the new mechanism-specific parameter is not passed through toAsymmetricAlgorithm::verify().Proposed fix
- return AsymmetricAlgorithm::verify(publicKey, originalData, signature, mechanism, param, paramLen); + return AsymmetricAlgorithm::verify(publicKey, originalData, signature, mechanism, param, paramLen, mechanismParam);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/crypto/BotanDSA.cpp` around lines 282 - 294, The delegated verify path in BotanDSA::verify drops the mechanism-specific pointer; update the call to AsymmetricAlgorithm::verify(publicKey, originalData, signature, mechanism, param, paramLen) to also forward the mechanismParam argument (the same mechanismParam parameter declared in BotanDSA::verify), mirroring the fix applied in sign(), so the delegated call receives the mechanism-specific parameter.
64-76:⚠️ Potential issue | 🟠 MajorForward
mechanismParamin the delegated sign path.The fallback on Line 76 drops the new parameter and always calls the generic signer with
NULL. That breaks the new mechanism-parameter plumbing for any path that relies onAsymmetricAlgorithm::sign().Proposed fix
- return AsymmetricAlgorithm::sign(privateKey, dataToSign, signature, mechanism, param, paramLen); + return AsymmetricAlgorithm::sign(privateKey, dataToSign, signature, mechanism, param, paramLen, mechanismParam);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/crypto/BotanDSA.cpp` around lines 64 - 76, The delegated fallback in BotanDSA::sign currently omits the mechanismParam and calls AsymmetricAlgorithm::sign with NULL; update the fallback call to forward the mechanismParam argument instead so the delegated path receives the mechanism-specific data (i.e., call AsymmetricAlgorithm::sign(privateKey, dataToSign, signature, mechanism, param, paramLen, mechanismParam) or the appropriate overload that accepts mechanismParam), ensuring the same mechanismParam identifier from the BotanDSA::sign signature is passed through.
♻️ Duplicate comments (2)
src/lib/crypto/OSSLMLDSAPrivateKey.cpp (1)
62-88:⚠️ Potential issue | 🟠 MajorReject unsupported private-key lengths before accepting the decoded key.
setFromOSSL()now fails atomically on extraction errors, but it still accepts any blob returned throughOSSL_PKEY_PARAM_PRIV_KEY. That meansPKCS8Decode()can succeed without proving the decoded key is actually one of the supported ML-DSA parameter sets. Please rejectpriv_lenvalues that do not match the supported ML-DSA private-key sizes before callingsetValue().Based on learnings, MLDSAPrivateKey infers the parameter set from key-material length (CKP_ML_DSA_44/65/87), so unsupported lengths should be rejected here.Proposed fix
rv = EVP_PKEY_get_octet_string_param(inMLDSAKEY, OSSL_PKEY_PARAM_PRIV_KEY, priv, sizeof(priv), &priv_len); if(!rv) { ERROR_MSG("Could not get private key, rv: %d", rv); return false; } + if (priv_len != MLDSAParameters::ML_DSA_44_PRIV_LENGTH && + priv_len != MLDSAParameters::ML_DSA_65_PRIV_LENGTH && + priv_len != MLDSAParameters::ML_DSA_87_PRIV_LENGTH) + { + ERROR_MSG("Unsupported ML-DSA private key length: %zu", priv_len); + return false; + } // Commit state atomically after successful extraction🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/crypto/OSSLMLDSAPrivateKey.cpp` around lines 62 - 88, setFromOSSL currently accepts any priv blob length; before calling setValue() reject unsupported private-key lengths by validating priv_len against the supported ML-DSA sizes (e.g. MLDSAParameters::ML_DSA_44_PRIV_LENGTH, MLDSAParameters::ML_DSA_65_PRIV_LENGTH, MLDSAParameters::ML_DSA_87_PRIV_LENGTH). If priv_len does not match one of those constants, log an error (ERROR_MSG) and return false; only after passing this length check (and optionally after setting seed with setSeed(localSeed)) call setValue(ByteString(priv, priv_len)) and return true.src/lib/SoftHSM.cpp (1)
5538-5570:⚠️ Potential issue | 🟠 MajorMirror the 255-byte ML-DSA context limit in verify-init.
AsymVerifyInitacceptsulContextLen > 255and forwards it intoByteString, whileAsymSignInitrejects the same input. That makes the PKCS#11 boundary inconsistent and lets invalid verify parameters escape into the backend.Based on learnings SoftHSM ML‑DSA additional context handling validates PKCS#11 `CK_SIGN_ADDITIONAL_CONTEXT`, and PKCS#11 ML‑DSA context is limited to 255 bytes.🛠️ Minimal fix
if (ckSignAdditionalContext->ulContextLen > 0) { if (ckSignAdditionalContext->pContext == NULL_PTR) { ERROR_MSG("Invalid parameters"); return CKR_ARGUMENTS_BAD; } + if (ckSignAdditionalContext->ulContextLen > 255) { + ERROR_MSG("Invalid parameters"); + return CKR_ARGUMENTS_BAD; + } mldsaParam.additionalContext = ByteString(ckSignAdditionalContext->pContext, ckSignAdditionalContext->ulContextLen); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/SoftHSM.cpp` around lines 5538 - 5570, AsymVerifyInit currently allows CK_SIGN_ADDITIONAL_CONTEXT.ulContextLen > 255 while AsymSignInit rejects it; replicate the ML‑DSA 255‑byte context limit in AsymVerifyInit by validating CK_SIGN_ADDITIONAL_CONTEXT (the same checks done in AsymSignInit): if ulContextLen > 255 return CKR_ARGUMENTS_BAD, ensure pContext is non‑NULL when ulContextLen > 0, call MLDSAUtil::setHedge as done in the sign path, and populate mldsaParam.additionalContext via ByteString only after these checks (symbols to locate: AsymVerifyInit, CK_SIGN_ADDITIONAL_CONTEXT, mldsaParam, MLDSAUtil::setHedge, ByteString).
🧹 Nitpick comments (3)
src/lib/crypto/MLDSAParameters.cpp (1)
39-57: Inconsistent indentation indeserialisefunction body.The function body uses double indentation (lines 41-56) while other functions in this file use single indentation. This should be aligned for consistency.
🧹 Suggested fix for indentation
bool MLDSAParameters::deserialise(ByteString& serialised) { - if (serialised.size() == 0) - { - return false; - } + if (serialised.size() == 0) + { + return false; + } - unsigned long parameter = serialised.long_val(); - if (parameter != ML_DSA_44_PARAMETER_SET && - parameter != ML_DSA_65_PARAMETER_SET && - parameter != ML_DSA_87_PARAMETER_SET) - { - return false; - } + unsigned long parameter = serialised.long_val(); + if (parameter != ML_DSA_44_PARAMETER_SET && + parameter != ML_DSA_65_PARAMETER_SET && + parameter != ML_DSA_87_PARAMETER_SET) + { + return false; + } - setParameterSet(parameter); + setParameterSet(parameter); - return true; + return true; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/crypto/MLDSAParameters.cpp` around lines 39 - 57, The MLDSAParameters::deserialise function body is indented with an extra level compared to the rest of the file; reformat the function so its internal statements use the same single indentation style as other functions in this file (adjust braces, the if blocks, declarations like "unsigned long parameter = serialised.long_val();" and the return statements) to restore consistent indentation and alignment.src/lib/crypto/MechanismParam.h (1)
12-23: Mixed indentation (spaces and tabs) in class definition.Lines 15-16 use spaces while lines 18-22 use tabs. Consider standardizing to match the project's conventions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/crypto/MechanismParam.h` around lines 12 - 23, The class definition for MechanismParam has mixed indentation; standardize the file to the project's indentation style (use either tabs or spaces consistently) by adjusting the whitespace for the class declaration and its members (type, isOfType, clone, destructor) so all lines use the same indentation convention; update the lines declaring "static const char* type;", "virtual bool isOfType(...)", "virtual MechanismParam* clone() const", and the destructor to match the project's preferred indentation.src/lib/crypto/MLDSAUtil.h (1)
21-31: Consider makingMLDSAUtilnon-instantiable.Since
MLDSAUtilis a utility class with only static methods, consider preventing instantiation by deleting the constructor and destructor, or making the constructor private.🧹 Suggested improvement
class MLDSAUtil { public: + MLDSAUtil() = delete; + static CK_RV getMLDSAPrivateKey(MLDSAPrivateKey* privateKey, Token* token, OSObject* key); static CK_RV getMLDSAPublicKey(MLDSAPublicKey* publicKey, Token* token, OSObject* key); static bool setMLDSAPrivateKey(OSObject* key, const ByteString &ber, Token* token, bool isPrivate); static CK_RV setHedge(CK_HEDGE_TYPE inHedgeType, Hedge::Type* outHedgeType); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/crypto/MLDSAUtil.h` around lines 21 - 31, MLDSAUtil is a pure-static utility class and should be made non-instantiable: add a deleted default constructor and deleted destructor (or alternatively declare a private default constructor and destructor) to the MLDSAUtil class so callers cannot create instances; update the class declaration near the existing static methods (getMLDSAPrivateKey, getMLDSAPublicKey, setMLDSAPrivateKey, setHedge) to include the deleted/private ctor/dtor declarations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/crypto/MechanismParam.h`:
- Around line 15-16: The base class MechanismParam declares a static member
const char* type that is never defined or used; remove this misleading
declaration from MechanismParam (or alternatively add clear
documentation/abstract contract if you intend subclasses like
MLDSAMechanismParam to provide their own static type) so that only subclasses
declare/define their own type symbols; update any comments/docs to reflect the
chosen approach.
In `@src/lib/crypto/OSSLMLDSAPrivateKey.cpp`:
- Around line 64-87: The temporary stack buffers seed and priv contain secret
ML-DSA material and must be zeroized on every exit path: after copying into
ByteString (localSeed and setValue) and before any early return (e.g., when
EVP_PKEY_get_octet_string_param for OSSL_PKEY_PARAM_PRIV_KEY fails). Update the
code around the EVP_PKEY_get_octet_string_param calls to securely cleanse seed
and priv (use a crypto-safe wipe like OPENSSL_cleanse or
explicit_bzero/memset_s) immediately after the data is consumed and also right
before each return; ensure you still call setSeed(localSeed) and
setValue(ByteString(priv, priv_len)) before wiping the corresponding buffer so
the copy is preserved.
---
Outside diff comments:
In `@src/lib/crypto/BotanDSA.cpp`:
- Around line 282-294: The delegated verify path in BotanDSA::verify drops the
mechanism-specific pointer; update the call to
AsymmetricAlgorithm::verify(publicKey, originalData, signature, mechanism,
param, paramLen) to also forward the mechanismParam argument (the same
mechanismParam parameter declared in BotanDSA::verify), mirroring the fix
applied in sign(), so the delegated call receives the mechanism-specific
parameter.
- Around line 64-76: The delegated fallback in BotanDSA::sign currently omits
the mechanismParam and calls AsymmetricAlgorithm::sign with NULL; update the
fallback call to forward the mechanismParam argument instead so the delegated
path receives the mechanism-specific data (i.e., call
AsymmetricAlgorithm::sign(privateKey, dataToSign, signature, mechanism, param,
paramLen, mechanismParam) or the appropriate overload that accepts
mechanismParam), ensuring the same mechanismParam identifier from the
BotanDSA::sign signature is passed through.
---
Duplicate comments:
In `@src/lib/crypto/OSSLMLDSAPrivateKey.cpp`:
- Around line 62-88: setFromOSSL currently accepts any priv blob length; before
calling setValue() reject unsupported private-key lengths by validating priv_len
against the supported ML-DSA sizes (e.g. MLDSAParameters::ML_DSA_44_PRIV_LENGTH,
MLDSAParameters::ML_DSA_65_PRIV_LENGTH, MLDSAParameters::ML_DSA_87_PRIV_LENGTH).
If priv_len does not match one of those constants, log an error (ERROR_MSG) and
return false; only after passing this length check (and optionally after setting
seed with setSeed(localSeed)) call setValue(ByteString(priv, priv_len)) and
return true.
In `@src/lib/SoftHSM.cpp`:
- Around line 5538-5570: AsymVerifyInit currently allows
CK_SIGN_ADDITIONAL_CONTEXT.ulContextLen > 255 while AsymSignInit rejects it;
replicate the ML‑DSA 255‑byte context limit in AsymVerifyInit by validating
CK_SIGN_ADDITIONAL_CONTEXT (the same checks done in AsymSignInit): if
ulContextLen > 255 return CKR_ARGUMENTS_BAD, ensure pContext is non‑NULL when
ulContextLen > 0, call MLDSAUtil::setHedge as done in the sign path, and
populate mldsaParam.additionalContext via ByteString only after these checks
(symbols to locate: AsymVerifyInit, CK_SIGN_ADDITIONAL_CONTEXT, mldsaParam,
MLDSAUtil::setHedge, ByteString).
---
Nitpick comments:
In `@src/lib/crypto/MechanismParam.h`:
- Around line 12-23: The class definition for MechanismParam has mixed
indentation; standardize the file to the project's indentation style (use either
tabs or spaces consistently) by adjusting the whitespace for the class
declaration and its members (type, isOfType, clone, destructor) so all lines use
the same indentation convention; update the lines declaring "static const char*
type;", "virtual bool isOfType(...)", "virtual MechanismParam* clone() const",
and the destructor to match the project's preferred indentation.
In `@src/lib/crypto/MLDSAParameters.cpp`:
- Around line 39-57: The MLDSAParameters::deserialise function body is indented
with an extra level compared to the rest of the file; reformat the function so
its internal statements use the same single indentation style as other functions
in this file (adjust braces, the if blocks, declarations like "unsigned long
parameter = serialised.long_val();" and the return statements) to restore
consistent indentation and alignment.
In `@src/lib/crypto/MLDSAUtil.h`:
- Around line 21-31: MLDSAUtil is a pure-static utility class and should be made
non-instantiable: add a deleted default constructor and deleted destructor (or
alternatively declare a private default constructor and destructor) to the
MLDSAUtil class so callers cannot create instances; update the class declaration
near the existing static methods (getMLDSAPrivateKey, getMLDSAPublicKey,
setMLDSAPrivateKey, setHedge) to include the deleted/private ctor/dtor
declarations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 49a06724-469d-4f64-a725-6d7cf5faee0d
📒 Files selected for processing (34)
src/lib/SoftHSM.cppsrc/lib/crypto/AsymmetricAlgorithm.cppsrc/lib/crypto/AsymmetricAlgorithm.hsrc/lib/crypto/BotanDSA.cppsrc/lib/crypto/BotanDSA.hsrc/lib/crypto/BotanECDSA.cppsrc/lib/crypto/BotanECDSA.hsrc/lib/crypto/BotanEDDSA.cppsrc/lib/crypto/BotanEDDSA.hsrc/lib/crypto/BotanRSA.cppsrc/lib/crypto/BotanRSA.hsrc/lib/crypto/CMakeLists.txtsrc/lib/crypto/MLDSAMechanismParam.cppsrc/lib/crypto/MLDSAMechanismParam.hsrc/lib/crypto/MLDSAParameters.cppsrc/lib/crypto/MLDSAUtil.cppsrc/lib/crypto/MLDSAUtil.hsrc/lib/crypto/Makefile.amsrc/lib/crypto/MechanismParam.hsrc/lib/crypto/OSSLDSA.cppsrc/lib/crypto/OSSLDSA.hsrc/lib/crypto/OSSLECDSA.cppsrc/lib/crypto/OSSLECDSA.hsrc/lib/crypto/OSSLEDDSA.cppsrc/lib/crypto/OSSLEDDSA.hsrc/lib/crypto/OSSLMLDSA.cppsrc/lib/crypto/OSSLMLDSA.hsrc/lib/crypto/OSSLMLDSAPrivateKey.cppsrc/lib/crypto/OSSLMLDSAPrivateKey.hsrc/lib/crypto/OSSLRSA.cppsrc/lib/crypto/OSSLRSA.hsrc/lib/crypto/test/MLDSATests.cppsrc/lib/session_mgr/Session.cppsrc/lib/session_mgr/Session.h
🚧 Files skipped from review as they are similar to previous changes (10)
- src/lib/crypto/OSSLDSA.h
- src/lib/crypto/BotanECDSA.cpp
- src/lib/session_mgr/Session.cpp
- src/lib/crypto/Makefile.am
- src/lib/crypto/BotanEDDSA.h
- src/lib/crypto/BotanDSA.h
- src/lib/crypto/CMakeLists.txt
- src/lib/crypto/OSSLEDDSA.cpp
- src/lib/crypto/OSSLDSA.cpp
- src/lib/crypto/OSSLECDSA.cpp
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/lib/SoftHSM.cpp (1)
6550-6567:⚠️ Potential issue | 🔴 Critical
CKM_AES_CBCwrapping still builds an empty IV.
blocksizestays0in the non-PAD CBC cases, but the shared CBC path always sizes and copies the IV usingblocksize. SinceC_WrapKeyalready acceptsCKM_AES_CBC, this still breaks non-PAD CBC wrapping at runtime.🐛 Minimal fix
case CKM_AES_CBC: algo = SymAlgo::AES; + blocksize = 16; break; @@ case CKM_DES3_CBC: algo = SymAlgo::DES3; + blocksize = 8; break;Also applies to: 6593-6600
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/SoftHSM.cpp` around lines 6550 - 6567, In the CKM_AES_CBC and CKM_DES3_CBC switch branches in SoftHSM.cpp the blocksize remains 0 causing the shared CBC path to allocate/copy an empty IV; set blocksize = 16 for CKM_AES_CBC and blocksize = 8 for CKM_DES3_CBC (leave CKM_AES_CBC_PAD/CKM_DES3_CBC_PAD using RFC5652Pad to set wrappedlen) so the later IV sizing/copy uses the correct block size; locate the switch on mechanism types (CKM_AES_CBC, CKM_AES_CBC_PAD, CKM_DES3_CBC, CKM_DES3_CBC_PAD), and update those two branches accordingly.
🧹 Nitpick comments (3)
src/lib/crypto/MLDSAMechanismParam.h (1)
39-41: PassadditionalContextby const reference.This constructor currently copies
ByteStringinto the parameter and then into the member. Acceptingconst ByteString&avoids an extra copy on every construction path with no semantic change.Suggested change
- MLDSAMechanismParam(Hedge::Type hedgeType, ByteString additionalContext); + MLDSAMechanismParam(Hedge::Type hedgeType, const ByteString& additionalContext);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/crypto/MLDSAMechanismParam.h` around lines 39 - 41, Change the MLDSAMechanismParam constructor signature to take the additionalContext by const reference (const ByteString& additionalContext) instead of by value to avoid an extra copy; update the MLDSAMechanismParam(Hedge::Type hedgeType, ByteString additionalContext) declaration to MLDSAMechanismParam(Hedge::Type hedgeType, const ByteString& additionalContext) and make the same change in the corresponding definition/implementation so the member assignment still copies into the member as before and the MLDSAMechanismParam::clone() behavior remains unchanged.src/lib/session_mgr/Session.h (1)
110-111: Make the mechanism-param API const-correct.
Session::setMechanismParam()clones the input, and downstreamsign()/verify()only consumeconst MechanismParam*. Accepting and returning a mutable pointer here makes it too easy to modify session-owned state accidentally.Suggested change
- void setMechanismParam(MechanismParam* mechanismParam); - MechanismParam* getMechanismParam(); + void setMechanismParam(const MechanismParam* mechanismParam); + const MechanismParam* getMechanismParam() const;Also applies to: 169-169
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/session_mgr/Session.h` around lines 110 - 111, Change the mechanism-param API to be const-correct: have Session::setMechanismParam accept a const MechanismParam* (since it clones the input) and have Session::getMechanismParam return a const MechanismParam* so callers cannot mutate session-owned state; update any other declarations/uses of setMechanismParam/getMechanismParam (including the second occurrence referenced) to match the new signatures and adjust callers to remove any mutable expectations.src/lib/crypto/OSSLMLDSA.cpp (1)
24-25: Consider making static membersconstexpr.Since
OSSL_RANDOMandOSSL_DETERMINISTICare never modified after initialization, declaring themstatic constexpr intwould express this intent and allow compile-time optimization.♻️ Proposed change
In
OSSLMLDSA.h(lines 52-53):- static int OSSL_RANDOM; - static int OSSL_DETERMINISTIC; + static constexpr int OSSL_RANDOM = 0; + static constexpr int OSSL_DETERMINISTIC = 1;Then remove the definitions from
OSSLMLDSA.cpp(lines 24-25).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/crypto/OSSLMLDSA.cpp` around lines 24 - 25, Make the two mode constants true compile-time constants by declaring them as static constexpr int members in class OSSLMLDSA (replace the current declarations for OSSL_RANDOM and OSSL_DETERMINISTIC in OSSLMLDSA.h with static constexpr int OSSL_RANDOM = 0; and static constexpr int OSSL_DETERMINISTIC = 1;) and remove the duplicate out-of-class definitions for OSSLMLDSA::OSSL_RANDOM and OSSLMLDSA::OSSL_DETERMINISTIC from OSSLMLDSA.cpp so they are no longer defined at namespace scope.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/crypto/MLDSAParameters.cpp`:
- Around line 39-56: The deserialiser currently only checks for empty input and
then calls ByteString::long_val(), which accepts truncated or overlong blobs;
update MLDSAParameters::deserialise to require the canonical 8-byte encoding
emitted by serialise() by validating that serialised.size() == 8 (reject
anything not exactly 8 bytes) before calling serialised.long_val(), then proceed
with the existing parameter value checks and setParameterSet(parameter).
In `@src/lib/crypto/OSSLMLDSAPrivateKey.cpp`:
- Around line 95-99: The object leaves a stale seed when importing a key that
lacks OSSL_PKEY_PARAM_ML_DSA_SEED; ensure you always reset the seed atomically
with the value update by calling setSeed with an empty ByteString (or equivalent
clear) when localSeed is empty, then call setValue(ByteString(priv, priv_len))
so setSeed(localSeed) is used when present and cleared when not (refer to
localSeed, setSeed, setValue, ByteString).
- Around line 7-15: This translation unit uses std::move, memset and strcmp but
doesn't include the standard headers; update OSSLMLDSAPrivateKey.cpp to directly
include <utility> for std::move and <cstring> for memset/strcmp (ensuring
symbols used in functions/methods within OSSLMLDSAPrivateKey.cpp such as the
constructors/assignments that call std::move and any cleanup/compare code that
calls memset/strcmp are resolved), remove reliance on transitive includes so the
file builds reliably across toolchains.
---
Duplicate comments:
In `@src/lib/SoftHSM.cpp`:
- Around line 6550-6567: In the CKM_AES_CBC and CKM_DES3_CBC switch branches in
SoftHSM.cpp the blocksize remains 0 causing the shared CBC path to allocate/copy
an empty IV; set blocksize = 16 for CKM_AES_CBC and blocksize = 8 for
CKM_DES3_CBC (leave CKM_AES_CBC_PAD/CKM_DES3_CBC_PAD using RFC5652Pad to set
wrappedlen) so the later IV sizing/copy uses the correct block size; locate the
switch on mechanism types (CKM_AES_CBC, CKM_AES_CBC_PAD, CKM_DES3_CBC,
CKM_DES3_CBC_PAD), and update those two branches accordingly.
---
Nitpick comments:
In `@src/lib/crypto/MLDSAMechanismParam.h`:
- Around line 39-41: Change the MLDSAMechanismParam constructor signature to
take the additionalContext by const reference (const ByteString&
additionalContext) instead of by value to avoid an extra copy; update the
MLDSAMechanismParam(Hedge::Type hedgeType, ByteString additionalContext)
declaration to MLDSAMechanismParam(Hedge::Type hedgeType, const ByteString&
additionalContext) and make the same change in the corresponding
definition/implementation so the member assignment still copies into the member
as before and the MLDSAMechanismParam::clone() behavior remains unchanged.
In `@src/lib/crypto/OSSLMLDSA.cpp`:
- Around line 24-25: Make the two mode constants true compile-time constants by
declaring them as static constexpr int members in class OSSLMLDSA (replace the
current declarations for OSSL_RANDOM and OSSL_DETERMINISTIC in OSSLMLDSA.h with
static constexpr int OSSL_RANDOM = 0; and static constexpr int
OSSL_DETERMINISTIC = 1;) and remove the duplicate out-of-class definitions for
OSSLMLDSA::OSSL_RANDOM and OSSLMLDSA::OSSL_DETERMINISTIC from OSSLMLDSA.cpp so
they are no longer defined at namespace scope.
In `@src/lib/session_mgr/Session.h`:
- Around line 110-111: Change the mechanism-param API to be const-correct: have
Session::setMechanismParam accept a const MechanismParam* (since it clones the
input) and have Session::getMechanismParam return a const MechanismParam* so
callers cannot mutate session-owned state; update any other declarations/uses of
setMechanismParam/getMechanismParam (including the second occurrence referenced)
to match the new signatures and adjust callers to remove any mutable
expectations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5f1b7bad-a036-4528-9352-dc16b17f59f7
📒 Files selected for processing (34)
src/lib/SoftHSM.cppsrc/lib/crypto/AsymmetricAlgorithm.cppsrc/lib/crypto/AsymmetricAlgorithm.hsrc/lib/crypto/BotanDSA.cppsrc/lib/crypto/BotanDSA.hsrc/lib/crypto/BotanECDSA.cppsrc/lib/crypto/BotanECDSA.hsrc/lib/crypto/BotanEDDSA.cppsrc/lib/crypto/BotanEDDSA.hsrc/lib/crypto/BotanRSA.cppsrc/lib/crypto/BotanRSA.hsrc/lib/crypto/CMakeLists.txtsrc/lib/crypto/MLDSAMechanismParam.cppsrc/lib/crypto/MLDSAMechanismParam.hsrc/lib/crypto/MLDSAParameters.cppsrc/lib/crypto/MLDSAUtil.cppsrc/lib/crypto/MLDSAUtil.hsrc/lib/crypto/Makefile.amsrc/lib/crypto/MechanismParam.hsrc/lib/crypto/OSSLDSA.cppsrc/lib/crypto/OSSLDSA.hsrc/lib/crypto/OSSLECDSA.cppsrc/lib/crypto/OSSLECDSA.hsrc/lib/crypto/OSSLEDDSA.cppsrc/lib/crypto/OSSLEDDSA.hsrc/lib/crypto/OSSLMLDSA.cppsrc/lib/crypto/OSSLMLDSA.hsrc/lib/crypto/OSSLMLDSAPrivateKey.cppsrc/lib/crypto/OSSLMLDSAPrivateKey.hsrc/lib/crypto/OSSLRSA.cppsrc/lib/crypto/OSSLRSA.hsrc/lib/crypto/test/MLDSATests.cppsrc/lib/session_mgr/Session.cppsrc/lib/session_mgr/Session.h
🚧 Files skipped from review as they are similar to previous changes (10)
- src/lib/crypto/OSSLDSA.h
- src/lib/crypto/OSSLECDSA.cpp
- src/lib/crypto/AsymmetricAlgorithm.cpp
- src/lib/crypto/BotanEDDSA.cpp
- src/lib/session_mgr/Session.cpp
- src/lib/crypto/Makefile.am
- src/lib/crypto/BotanECDSA.cpp
- src/lib/crypto/OSSLRSA.cpp
- src/lib/crypto/OSSLDSA.cpp
- src/lib/crypto/BotanEDDSA.h
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
src/lib/crypto/MLDSAMechanismParam.cpp (1)
7-11: Consider adding explicit<cstring>include forstrcmp.Line 37 uses
strcmpbut the file doesn't explicitly include<cstring>or<string.h>. While it may work via transitive includes, explicit inclusion is safer and clearer.♻️ Suggested fix
`#include` "config.h" `#ifdef` WITH_ML_DSA `#include` "ByteString.h" `#include` "MechanismParam.h" `#include` "MLDSAMechanismParam.h" +#include <cstring>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/crypto/MLDSAMechanismParam.cpp` around lines 7 - 11, Add an explicit C string header to MLDSAMechanismParam.cpp so strcmp is properly declared: include <cstring> (or <string.h>) in the top include block alongside the existing includes so the call to strcmp in this file is not relying on transitive includes.src/lib/crypto/OSSLRSA.cpp (2)
280-284: Consider forwardingmechanismParamto base class call.Line 283 calls
AsymmetricAlgorithm::sign()without forwarding themechanismParamparameter. While RSA doesn't use it and the base class ignores it anyway, forwarding would be more consistent:♻️ Suggested fix
- return AsymmetricAlgorithm::sign(privateKey, dataToSign, signature, mechanism, param, paramLen); + return AsymmetricAlgorithm::sign(privateKey, dataToSign, signature, mechanism, param, paramLen, NULL);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/crypto/OSSLRSA.cpp` around lines 280 - 284, The else branch in OSSLRSA::sign returns AsymmetricAlgorithm::sign(...) but omits forwarding the mechanismParam argument; update the call in OSSLRSA::sign to pass the mechanismParam (and paramLen if applicable) through to AsymmetricAlgorithm::sign so the base-class signature receives the same mechanism parameter you were given, keeping behavior consistent with the function signature.
838-842: Consider forwardingmechanismParamto base class call.Same as the
sign()case - line 841 could forwardmechanismParamfor consistency, though it's functionally correct as-is.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/crypto/OSSLRSA.cpp` around lines 838 - 842, The verify path in OSSLRSA.cpp currently calls AsymmetricAlgorithm::verify(publicKey, originalData, signature, mechanism, param, paramLen) while the local variable is named mechanismParam; update the call in the else branch of OSSLRSA::verify to forward mechanismParam (i.e., use mechanismParam in place of param) so it matches the sign() case and maintains consistency with the surrounding code and intent.src/lib/crypto/MLDSAParameters.cpp (1)
7-10: Consider adding explicit<cstring>include.Similar to MLDSAMechanismParam.cpp, line 30 uses
strcmpwithout an explicit<cstring>include. While it likely works via transitive includes, explicit inclusion is cleaner.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/crypto/MLDSAParameters.cpp` around lines 7 - 10, The source uses strcmp in MLDSAParameters.cpp but currently includes the C header <string.h>; add an explicit C++ header by inserting `#include` <cstring> (and optionally replace <string.h> with <cstring>) at the top of MLDSAParameters.cpp so strcmp is provided via the proper C++ header, matching the approach used in MLDSAMechanismParam.cpp and avoiding reliance on transitive includes.src/lib/crypto/CMakeLists.txt (1)
77-80: Same recommendation: gate OSSL ML-DSA sources withWITH_ML_DSA.For consistency with the Automake build, consider wrapping these OpenSSL ML-DSA sources in a
if(WITH_ML_DSA)block to avoid compiling them when ML-DSA is disabled or OpenSSL < 3.5 is in use.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/crypto/CMakeLists.txt` around lines 77 - 80, Wrap the ML-DSA OpenSSL source files (OSSLMLDSA.cpp, OSSLMLDSAKeyPair.cpp, OSSLMLDSAPrivateKey.cpp, OSSLMLDSAPublicKey.cpp) in a CMake conditional using the WITH_ML_DSA option so they are only added when ML-DSA is enabled; modify the CMakeLists.txt to enclose the list of these source filenames in an if(WITH_ML_DSA) ... endif() block (and ensure any target_sources or add_library calls that reference them are similarly guarded) so builds without WITH_ML_DSA or with older OpenSSL skip compiling these files.src/lib/crypto/OSSLDSA.cpp (1)
99-103: Consider forwardingmechanismParamto the base class for interface consistency.The
mechanismParamparameter is accepted but not forwarded when falling through toAsymmetricAlgorithm::sign(). While DSA doesn't use this parameter and the base class defaults toNULL, forwarding it maintains consistency with the interface contract and mirrors the pattern inOSSLMLDSA::sign().Same applies to
verify()at line 291.♻️ Proposed fix
else { // Call default implementation - return AsymmetricAlgorithm::sign(privateKey, dataToSign, signature, mechanism, param, paramLen); + return AsymmetricAlgorithm::sign(privateKey, dataToSign, signature, mechanism, param, paramLen, NULL); }And similarly for
verify():else { // Call the generic function - return AsymmetricAlgorithm::verify(publicKey, originalData, signature, mechanism, param, paramLen); + return AsymmetricAlgorithm::verify(publicKey, originalData, signature, mechanism, param, paramLen, NULL); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/crypto/OSSLDSA.cpp` around lines 99 - 103, In OSSLDSA::sign() and OSSLDSA::verify() forward the mechanismParam argument to the base implementation instead of omitting it: when falling back to AsymmetricAlgorithm::sign(...) and AsymmetricAlgorithm::verify(...), include the mechanismParam parameter so the call signature matches the interface (i.e., pass mechanismParam through from OSSLDSA::sign/verify to AsymmetricAlgorithm::sign/verify).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/crypto/AsymmetricAlgorithm.h`:
- Around line 135-141: OSSLGOST's sign() and verify() declarations do not match
the base class signature so they are not overriding; update OSSLGOST to declare
bool sign(PrivateKey* privateKey, const ByteString& dataToSign, ByteString&
signature, const AsymMech::Type mechanism, const void* param = NULL, const
size_t paramLen = 0, const MechanismParam* mechanismParam = NULL) and bool
verify(PublicKey* publicKey, const ByteString& originalData, const ByteString&
signature, const AsymMech::Type mechanism, const void* param = NULL, const
size_t paramLen = 0, const MechanismParam* mechanismParam = NULL) so signatures
exactly match AsymmetricAlgorithm; then update the OSSLGOST.cpp definitions
(method implementations) to the same parameter list and default for
mechanismParam to ensure proper overrides and correct linkage.
In `@src/lib/crypto/Makefile.am`:
- Around line 86-89: Header guard inconsistency: OSSLMLDSA.h is missing the
WITH_ML_DSA conditional present in OSSLMLDSAKeyPair.h, OSSLMLDSAPrivateKey.h,
and OSSLMLDSAPublicKey.h; update OSSLMLDSA.h to wrap its entire contents with
the same `#ifdef` WITH_ML_DSA / `#endif` guard (using the exact macro name
WITH_ML_DSA) so all OSSL ML-DSA headers are consistently conditional, or
alternatively remove the guards from the other ML-DSA headers if you prefer
unconditional exposure—ensure the chosen approach is applied consistently across
OSSLMLDSA.h, OSSLMLDSAKeyPair.h, OSSLMLDSAPrivateKey.h, and OSSLMLDSAPublicKey.h
and that the macro matches the one used in m4/acx_crypto_backend.m4.
In `@src/lib/crypto/OSSLMLDSA.cpp`:
- Around line 348-355: The min/max key-size accessors currently return
private-key lengths; change OSSLMLDSA::getMinKeySize() to return
MLDSAParameters::ML_DSA_44_PUB_LENGTH and OSSLMLDSA::getMaxKeySize() to return
MLDSAParameters::ML_DSA_87_PUB_LENGTH so the mechanism reports public-key sizes
per PKCS#11 (use ML_DSA_44_PUB_LENGTH, ML_DSA_65_PUB_LENGTH,
ML_DSA_87_PUB_LENGTH names from MLDSAParameters where relevant).
In `@src/lib/SoftHSM.cpp`:
- Around line 4468-4505: In the CKM_ML_DSA initialization branch (case
CKM_ML_DSA) add an explicit key-type check and reject non-ML-DSA keys by
returning CKR_KEY_TYPE_INCONSISTENT; specifically verify that the operation's
key type (the same variable used elsewhere as keyType) equals CKK_ML_DSA before
proceeding with parameter parsing and setting mechanismParam, and mirror the
same check in the other ML-DSA init branch around lines 5538-5575 so
MLDSAUtil::getMLDSAPrivateKey()/getMLDSAPublicKey() are never called for a
mismatched key.
---
Nitpick comments:
In `@src/lib/crypto/CMakeLists.txt`:
- Around line 77-80: Wrap the ML-DSA OpenSSL source files (OSSLMLDSA.cpp,
OSSLMLDSAKeyPair.cpp, OSSLMLDSAPrivateKey.cpp, OSSLMLDSAPublicKey.cpp) in a
CMake conditional using the WITH_ML_DSA option so they are only added when
ML-DSA is enabled; modify the CMakeLists.txt to enclose the list of these source
filenames in an if(WITH_ML_DSA) ... endif() block (and ensure any target_sources
or add_library calls that reference them are similarly guarded) so builds
without WITH_ML_DSA or with older OpenSSL skip compiling these files.
In `@src/lib/crypto/MLDSAMechanismParam.cpp`:
- Around line 7-11: Add an explicit C string header to MLDSAMechanismParam.cpp
so strcmp is properly declared: include <cstring> (or <string.h>) in the top
include block alongside the existing includes so the call to strcmp in this file
is not relying on transitive includes.
In `@src/lib/crypto/MLDSAParameters.cpp`:
- Around line 7-10: The source uses strcmp in MLDSAParameters.cpp but currently
includes the C header <string.h>; add an explicit C++ header by inserting
`#include` <cstring> (and optionally replace <string.h> with <cstring>) at the top
of MLDSAParameters.cpp so strcmp is provided via the proper C++ header, matching
the approach used in MLDSAMechanismParam.cpp and avoiding reliance on transitive
includes.
In `@src/lib/crypto/OSSLDSA.cpp`:
- Around line 99-103: In OSSLDSA::sign() and OSSLDSA::verify() forward the
mechanismParam argument to the base implementation instead of omitting it: when
falling back to AsymmetricAlgorithm::sign(...) and
AsymmetricAlgorithm::verify(...), include the mechanismParam parameter so the
call signature matches the interface (i.e., pass mechanismParam through from
OSSLDSA::sign/verify to AsymmetricAlgorithm::sign/verify).
In `@src/lib/crypto/OSSLRSA.cpp`:
- Around line 280-284: The else branch in OSSLRSA::sign returns
AsymmetricAlgorithm::sign(...) but omits forwarding the mechanismParam argument;
update the call in OSSLRSA::sign to pass the mechanismParam (and paramLen if
applicable) through to AsymmetricAlgorithm::sign so the base-class signature
receives the same mechanism parameter you were given, keeping behavior
consistent with the function signature.
- Around line 838-842: The verify path in OSSLRSA.cpp currently calls
AsymmetricAlgorithm::verify(publicKey, originalData, signature, mechanism,
param, paramLen) while the local variable is named mechanismParam; update the
call in the else branch of OSSLRSA::verify to forward mechanismParam (i.e., use
mechanismParam in place of param) so it matches the sign() case and maintains
consistency with the surrounding code and intent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 082f7288-4acd-4598-8a57-ebcdf457461a
📒 Files selected for processing (34)
src/lib/SoftHSM.cppsrc/lib/crypto/AsymmetricAlgorithm.cppsrc/lib/crypto/AsymmetricAlgorithm.hsrc/lib/crypto/BotanDSA.cppsrc/lib/crypto/BotanDSA.hsrc/lib/crypto/BotanECDSA.cppsrc/lib/crypto/BotanECDSA.hsrc/lib/crypto/BotanEDDSA.cppsrc/lib/crypto/BotanEDDSA.hsrc/lib/crypto/BotanRSA.cppsrc/lib/crypto/BotanRSA.hsrc/lib/crypto/CMakeLists.txtsrc/lib/crypto/MLDSAMechanismParam.cppsrc/lib/crypto/MLDSAMechanismParam.hsrc/lib/crypto/MLDSAParameters.cppsrc/lib/crypto/MLDSAUtil.cppsrc/lib/crypto/MLDSAUtil.hsrc/lib/crypto/Makefile.amsrc/lib/crypto/MechanismParam.hsrc/lib/crypto/OSSLDSA.cppsrc/lib/crypto/OSSLDSA.hsrc/lib/crypto/OSSLECDSA.cppsrc/lib/crypto/OSSLECDSA.hsrc/lib/crypto/OSSLEDDSA.cppsrc/lib/crypto/OSSLEDDSA.hsrc/lib/crypto/OSSLMLDSA.cppsrc/lib/crypto/OSSLMLDSA.hsrc/lib/crypto/OSSLMLDSAPrivateKey.cppsrc/lib/crypto/OSSLMLDSAPrivateKey.hsrc/lib/crypto/OSSLRSA.cppsrc/lib/crypto/OSSLRSA.hsrc/lib/crypto/test/MLDSATests.cppsrc/lib/session_mgr/Session.cppsrc/lib/session_mgr/Session.h
🚧 Files skipped from review as they are similar to previous changes (12)
- src/lib/crypto/AsymmetricAlgorithm.cpp
- src/lib/crypto/BotanECDSA.h
- src/lib/crypto/OSSLECDSA.cpp
- src/lib/crypto/BotanDSA.h
- src/lib/crypto/BotanDSA.cpp
- src/lib/crypto/BotanEDDSA.cpp
- src/lib/crypto/BotanEDDSA.h
- src/lib/crypto/OSSLEDDSA.cpp
- src/lib/crypto/OSSLECDSA.h
- src/lib/crypto/OSSLEDDSA.h
- src/lib/crypto/BotanRSA.cpp
- src/lib/crypto/BotanRSA.h
Can you check my latest commit ? |
bukka
left a comment
bukka
left a comment
There was a problem hiding this comment.
It looks much better and something that we could go for (and possibily later improve / extend).
|
|
||
| // 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 sign(PrivateKey* privateKey, const ByteString& dataToSign, ByteString& signature, const AsymMech::Type mechanism, const void* param = NULL, const size_t paramLen = 0, const MechanismParam* mechanismParam = NULL); |
There was a problem hiding this comment.
Have you thought about incorporating param and paramLen into this new class so signature gets simlpler. I guess we can do it later but just wondering if it would make sense?
There was a problem hiding this comment.
We could but I think that it should left for another PR and make param and paramLen disappear altogether from the sign/verify methods signatures.
This PR has lingered and went through too much already :D
7 participants
Adds ML-DSA support with OpenSSL >= 3.5
Summary by CodeRabbit
New Features
Build & Configuration
CI
Tests
Documentation