Added PKCS#11 3.2 ML-DSA multiple-part support in lib#867
Open
antoinelochet wants to merge 1 commit intosofthsm:mainfrom
Open
Added PKCS#11 3.2 ML-DSA multiple-part support in lib#867antoinelochet wants to merge 1 commit intosofthsm:mainfrom
antoinelochet wants to merge 1 commit intosofthsm:mainfrom
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:
📝 WalkthroughWalkthroughExtended signInit/verifyInit APIs to accept an optional Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant SoftHSM
participant OSSL as OSSLMLDSA
participant Asym as AsymmetricAlgorithm
App->>SoftHSM: C_SignInit (ML‑DSA, mechanismParam)
SoftHSM->>OSSL: signInit(privateKey, mech, param, paramLen, mechanismParam)
OSSL->>Asym: signInit(..., mechanismParam)
Asym-->>OSSL: ok
OSSL->>OSSL: clone mechanismParam\ninit message buffer
OSSL-->>SoftHSM: ok
SoftHSM-->>App: ok
App->>SoftHSM: C_SignUpdate(chunk1)
SoftHSM->>OSSL: signUpdate(chunk1)
OSSL->>Asym: signUpdate(chunk1)
Asym-->>OSSL: ok
OSSL->>OSSL: append chunk1
OSSL-->>SoftHSM: ok
App->>SoftHSM: C_SignUpdate(chunk2)
SoftHSM->>OSSL: signUpdate(chunk2)
OSSL->>Asym: signUpdate(chunk2)
Asym-->>OSSL: ok
OSSL->>OSSL: append chunk2
OSSL-->>SoftHSM: ok
App->>SoftHSM: C_SignFinal
SoftHSM->>OSSL: signFinal()
OSSL->>OSSL: call one‑shot sign() on accumulated message
OSSL->>Asym: signFinal()
Asym-->>OSSL: signature
OSSL->>OSSL: free mechanismParam\nclear buffer
OSSL-->>SoftHSM: signature
SoftHSM-->>App: signature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
antoinelochet
force-pushed
the
feature/ml-dsa-multi
branch
from
9e04028 to
3740390
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lib/SoftHSM.cpp (1)
5587-5590:⚠️ Potential issue | 🔴 CriticalEnable multipart verify for
CKM_ML_DSA.Line 5589 still disables multipart mode. That makes
C_VerifyUpdate/C_VerifyFinalunusable for ML-DSA, and Line 5780 never callsverifyInitto prepare backend state. The PR’s verify-side multipart path stays effectively off.[suggested fix]
Patch
case CKM_ML_DSA: mechanism = AsymMech::MLDSA; - bAllowMultiPartOp = false; + bAllowMultiPartOp = true; isMLDSA = true;Also applies to: 5780-5780
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/SoftHSM.cpp` around lines 5587 - 5590, The ML-DSA branch currently disables multipart operations (setting bAllowMultiPartOp = false and isMLDSA = true) which prevents C_VerifyUpdate/C_VerifyFinal and never triggers verifyInit; change the CKM_ML_DSA handling so multipart is allowed (do not set bAllowMultiPartOp = false — set it true or omit the false override) while keeping AsymMech::MLDSA and isMLDSA handling, and ensure the verify-side flow calls verifyInit when isMLDSA is set so the backend verify state is prepared (verifyInit must be invoked on the verify path where other mechanisms initialize multipart verify).src/lib/crypto/OSSLMLDSA.cpp (1)
55-58:⚠️ Potential issue | 🟠 MajorUninitialized
mechanismParametersmember risks undefined behavior.Looking at OSSLMLDSA.h (lines 55-58), the
mechanismParametersmember is not initialized. IfsignFinal/verifyFinalis called without a priorsignInit/verifyInit, or ifsignInit/verifyInitis called withmechanismParam == NULL, the member remains uninitialized (or stays at its prior value). InsignFinal/verifyFinal,delete mechanismParameterswill then cause undefined behavior.Initialize
mechanismParameterstonullptrin a constructor or add initialization insignInit/verifyInitbefore conditionally cloning.Proposed fix in OSSLMLDSA.h
Add a constructor to initialize members:
class OSSLMLDSA : public AsymmetricAlgorithm { public: // Constructor OSSLMLDSA() : parameters(nullptr), paramLength(0), mechanismParameters(nullptr) {} // Destructor virtual ~OSSLMLDSA() { } // ...Or initialize
mechanismParameters = nullptrat the start ofsignInit/verifyInitbefore the conditional clone:bool OSSLMLDSA::signInit(PrivateKey * privateKey, const AsymMech::Type mechanism, const void * param /* = NULL */ , const size_t paramLen /* = 0 */, const MechanismParam* mechanismParam /* = NULL */) { + mechanismParameters = nullptr; + if (!AsymmetricAlgorithm::signInit(privateKey, mechanism, param, paramLen))🤖 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 55 - 58, OSSLMLDSA's mechanismParameters member is not initialized which can lead to undefined behavior when delete is called in signFinal/verifyFinal; fix by initializing mechanismParameters to nullptr (either in a new/default constructor for class OSSLMLDSA that sets parameters=nullptr, paramLength=0, mechanismParameters=nullptr, or by explicitly setting mechanismParameters = nullptr at the start of signInit and verifyInit before any conditional cloning) so delete is only called on a valid pointer or nullptr.
🧹 Nitpick comments (8)
src/lib/crypto/test/MLDSATests.cpp (1)
455-455: Multipart verify path is not exercised by these new multipart tests.Both tests currently do multipart sign + one-shot verify. Since this PR adds multipart ML-DSA support, please also cover
verifyInit/verifyUpdate/verifyFinal.🧪 Suggested coverage improvement
- CPPUNIT_ASSERT(mldsa->verify(kp->getPublicKey(), all, sigMultiPart, AsymMech::MLDSA)); + CPPUNIT_ASSERT(mldsa->verifyInit(kp->getPublicKey(), AsymMech::MLDSA)); + CPPUNIT_ASSERT(mldsa->verifyUpdate(dataToSign)); + CPPUNIT_ASSERT(mldsa->verifyUpdate(dataToSignMultipart)); + CPPUNIT_ASSERT(mldsa->verifyFinal(sigMultiPart));For the context test, mirror the same pattern and pass
&contextas the 5th argument toverifyInit.Also applies to: 498-498
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/crypto/test/MLDSATests.cpp` at line 455, The tests currently perform multipart signing but only one-shot verification; update both occurrences (the existing CPPUNIT_ASSERT(mldsa->verify(..., AsymMech::MLDSA)) checks) to exercise the multipart verify path by calling mldsa->verifyInit(..., AsymMech::MLDSA, &context) (pass &context as the 5th argument for the context test), then feed the same data chunks via mldsa->verifyUpdate(&context, chunk) calls, and finally call mldsa->verifyFinal(&context, signature) and assert the result is true; do this for both the non-context and context variants (mirror the sign multipart pattern) so verifyInit/verifyUpdate/verifyFinal are covered.src/lib/crypto/BotanDSA.cpp (1)
136-140: ForwardmechanismParaminto basesignInit/verifyInit.Both methods now take
mechanismParam, but delegation still uses the old call shape. Passing it through avoids silent loss of caller-supplied mechanism context.♻️ Proposed minimal refactor
-bool BotanDSA::signInit(PrivateKey* privateKey, const AsymMech::Type mechanism, - const void* param /* = NULL */, const size_t paramLen /* = 0 */, - const MechanismParam* /* mechanismParam */) +bool BotanDSA::signInit(PrivateKey* privateKey, const AsymMech::Type mechanism, + const void* param /* = NULL */, const size_t paramLen /* = 0 */, + const MechanismParam* mechanismParam) { - if (!AsymmetricAlgorithm::signInit(privateKey, mechanism, param, paramLen)) + if (!AsymmetricAlgorithm::signInit(privateKey, mechanism, param, paramLen, mechanismParam)) { return false; } @@ -bool BotanDSA::verifyInit(PublicKey* publicKey, const AsymMech::Type mechanism, - const void* param /* = NULL */, const size_t paramLen /* = 0 */, - const MechanismParam* /* mechanismParam */) +bool BotanDSA::verifyInit(PublicKey* publicKey, const AsymMech::Type mechanism, + const void* param /* = NULL */, const size_t paramLen /* = 0 */, + const MechanismParam* mechanismParam) { - if (!AsymmetricAlgorithm::verifyInit(publicKey, mechanism, param, paramLen)) + if (!AsymmetricAlgorithm::verifyInit(publicKey, mechanism, param, paramLen, mechanismParam)) { return false; }Also applies to: 352-356
🤖 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 136 - 140, The delegation to the base AsymmetricAlgorithm in BotanDSA::signInit (and the corresponding BotanDSA::verifyInit) currently omits the new mechanismParam argument, causing caller-supplied mechanism context to be dropped; update the calls to AsymmetricAlgorithm::signInit and AsymmetricAlgorithm::verifyInit to pass the mechanismParam (and paramLen if applicable) through so the base methods receive the new parameter—search for BotanDSA::signInit and BotanDSA::verifyInit and forward the mechanismParam into their AsymmetricAlgorithm::*init calls.src/lib/crypto/OSSLGOST.cpp (1)
129-133: ForwardmechanismParamto base init to avoid silent parameter drop.Both init methods accept
mechanismParambut currently ignore it when delegating toAsymmetricAlgorithm. Forwarding it keeps the API contract consistent with no behavior change for current GOST flows.♻️ Proposed minimal refactor
-bool OSSLGOST::signInit(PrivateKey* privateKey, const AsymMech::Type mechanism, - const void* param /* = NULL */, const size_t paramLen /* = 0 */, - const MechanismParam* /* mechanismParam */) +bool OSSLGOST::signInit(PrivateKey* privateKey, const AsymMech::Type mechanism, + const void* param /* = NULL */, const size_t paramLen /* = 0 */, + const MechanismParam* mechanismParam) { - if (!AsymmetricAlgorithm::signInit(privateKey, mechanism, param, paramLen)) + if (!AsymmetricAlgorithm::signInit(privateKey, mechanism, param, paramLen, mechanismParam)) { return false; } @@ -bool OSSLGOST::verifyInit(PublicKey* publicKey, const AsymMech::Type mechanism, - const void* param /* = NULL */, const size_t paramLen /* = 0 */, - const MechanismParam* /* mechanismParam */) +bool OSSLGOST::verifyInit(PublicKey* publicKey, const AsymMech::Type mechanism, + const void* param /* = NULL */, const size_t paramLen /* = 0 */, + const MechanismParam* mechanismParam) { - if (!AsymmetricAlgorithm::verifyInit(publicKey, mechanism, param, paramLen)) + if (!AsymmetricAlgorithm::verifyInit(publicKey, mechanism, param, paramLen, mechanismParam)) { return false; }Also applies to: 315-320
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/crypto/OSSLGOST.cpp` around lines 129 - 133, The OSSLGOST::signInit currently calls AsymmetricAlgorithm::signInit without forwarding the MechanismParam*, causing silent parameter drop; update OSSLGOST::signInit to pass the mechanismParam through to AsymmetricAlgorithm::signInit (i.e., include the mechanismParam argument in the call) and make the same change for the corresponding init method (the verify/init variant around lines 315-320) so both OSSLGOST::signInit and the verify/init method preserve and forward the mechanismParam to the base class.src/lib/crypto/BotanGOST.cpp (1)
64-68: Please forwardmechanismParamin both init delegations.The new argument is accepted but discarded when calling
AsymmetricAlgorithm. Forwarding keeps behavior consistent with the widened API surface.♻️ Proposed minimal refactor
-bool BotanGOST::signInit(PrivateKey* privateKey, const AsymMech::Type mechanism, - const void* param /* = NULL */, const size_t paramLen /* = 0 */, - const MechanismParam* /* mechanismParam */) +bool BotanGOST::signInit(PrivateKey* privateKey, const AsymMech::Type mechanism, + const void* param /* = NULL */, const size_t paramLen /* = 0 */, + const MechanismParam* mechanismParam) { - if (!AsymmetricAlgorithm::signInit(privateKey, mechanism, param, paramLen)) + if (!AsymmetricAlgorithm::signInit(privateKey, mechanism, param, paramLen, mechanismParam)) { return false; } @@ -bool BotanGOST::verifyInit(PublicKey* publicKey, const AsymMech::Type mechanism, - const void* param /* = NULL */, const size_t paramLen /* = 0 */, - const MechanismParam* /* mechanismParam */) +bool BotanGOST::verifyInit(PublicKey* publicKey, const AsymMech::Type mechanism, + const void* param /* = NULL */, const size_t paramLen /* = 0 */, + const MechanismParam* mechanismParam) { - if (!AsymmetricAlgorithm::verifyInit(publicKey, mechanism, param, paramLen)) + if (!AsymmetricAlgorithm::verifyInit(publicKey, mechanism, param, paramLen, mechanismParam)) { return false; }Also applies to: 200-205
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/crypto/BotanGOST.cpp` around lines 64 - 68, The new mechanismParam argument is accepted but not being forwarded to the base init calls; update BotanGOST::signInit to pass mechanismParam through to AsymmetricAlgorithm::signInit (and likewise update the corresponding verify/init delegation around the other block referenced at lines 200-205) so the AsymmetricAlgorithm::signInit/verifyInit calls receive the MechanismParam pointer instead of being discarded; locate the calls to AsymmetricAlgorithm::signInit and the matching verify/init wrapper in this file and add mechanismParam as the final argument in those delegations.src/lib/crypto/OSSLMLDSA.cpp (3)
179-183: Consider validatingmechanismParamtype before cloning.In
signInit, the code clonesmechanismParamwithout validating its type. However, the one-shotsign()method (lines 65-70) validates thatmechanismParamis of typeMLDSAMechanismParambefore use. If an invalid type is passed tosignInit, it will be cloned and stored, butsignFinalwill fail later whensign()rejects it.For consistency with the one-shot path and to fail fast, consider validating the type in
signInit:Suggested validation
DEBUG_MSG("signInit mechanismParam != NULL: %d", mechanismParam != NULL); if (mechanismParam != NULL) { + if (!mechanismParam->isOfType(MLDSAMechanismParam::type)) + { + ERROR_MSG("Invalid mechanism parameter type supplied"); + ByteString dummy; + AsymmetricAlgorithm::signFinal(dummy); + return false; + } mechanismParameters = mechanismParam->clone(); }🤖 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 179 - 183, In signInit, validate the type of mechanismParam before cloning: if mechanismParam is non-NULL ensure it is an instance of MLDSAMechanismParam (same check used in the one-shot sign() path) and reject/return an error if it is not; only then assign mechanismParameters = mechanismParam->clone(); update signInit to mirror the one-shot validation so invalid types fail fast instead of being stored for later failure in signFinal/sign().
373-375: Same type validation issue inverifyInitas insignInit.The same concern applies here -
mechanismParamis cloned without type validation. Consider adding the same validation as suggested forsignInit.🤖 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 373 - 375, verifyInit clones mechanismParam into mechanismParameters without validating its type, mirroring the same bug from signInit; update verifyInit to perform the same type-check/validation used in signInit before calling mechanismParam->clone() (reject or throw if the mechanismParam type is unexpected) so only validated mechanismParam objects are cloned and assigned to mechanismParameters.
33-33: Debug logging of potentially sensitive data.The debug messages log the hex representation of data being signed/verified (lines 33, 195, 199, 227-228, 387, 389, 391). While useful for debugging, this could expose sensitive message content in debug logs. Consider whether this level of detail is appropriate for production builds.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/crypto/OSSLMLDSA.cpp` at line 33, The current DEBUG_MSG calls output full hex of sensitive buffers (e.g., DEBUG_MSG with dataToSign and related debug lines); remove or guard these prints by either omitting the hex payload in production (log only length or a hash) or wrapping them behind a strict sensitive-logging flag/compile-time check; specifically update usages of DEBUG_MSG in OSSLMLDSA.cpp that print dataToSign, message bytes, or signature bytes to instead emit non-sensitive metadata (e.g., data length or SHA256 digest) or be conditional on an explicit SENSITIVE_DEBUG enabled flag.src/lib/crypto/OSSLMLDSA.h (1)
55-58:parametersandparamLengthmembers appear unused.The
parameters(void*) andparamLength(size_t) members declared here are not used anywhere in the OSSLMLDSA.cpp implementation. The multipart flow usesmechanismParametersinstead. Consider removing these if they are not needed.Proposed cleanup
private: static int OSSL_RANDOM; static int OSSL_DETERMINISTIC; ByteString message; - void* parameters; - size_t paramLength; const MechanismParam* mechanismParameters; };🤖 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 55 - 58, Remove the unused members parameters (void*) and paramLength (size_t) from the OSSLMLDSA class/struct declaration and strip any related handling in the implementation: remove any constructor/destructor/initializer-list references, allocations, frees, copies, or assignments for parameters/paramLength in OSSLMLDSA.cpp, and ensure multipart flow continues to use mechanismParameters (const MechanismParam*) only; rebuild to confirm no remaining references to these two symbols.
🤖 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/OSSLMLDSA.cpp`:
- Around line 387-391: The DEBUG_MSG calls inside the verifyUpdate
implementation currently log "signUpdate"; update those two debug strings to
correctly read "verifyUpdate" so the logs reflect the actual function (locate
the DEBUG_MSG calls that reference originalData and message inside verifyUpdate
and replace "signUpdate" with "verifyUpdate").
In `@src/lib/crypto/test/MLDSATests.cpp`:
- Line 493: The multipart init/verify calls pass the context in the wrong slot:
change the signInit and verifyInit calls (e.g., mldsa->signInit(...) and
mldsa->verifyInit(...)) so the third argument (param) is nullptr and the fourth
argument is the mechanismParam (pass &context there instead of as param),
ensuring AsymMech::MLDSA is invoked with mechanismParam set to &context for the
"with context" tests.
- Around line 488-490: The test leaks a ByteString by allocating it on the heap;
instead create a stack ByteString and pass it to MLDSAMechanismParam to avoid
mallocs and frees (e.g., replace "ByteString* contextBS = new ByteString(...);
MLDSAMechanismParam context = MLDSAMechanismParam(..., *contextBS);" with a
local ByteString variable and construct MLDSAMechanismParam from that local),
ensuring no manual delete is needed and the temporary context bytes are copied
or owned correctly by MLDSAMechanismParam.
In `@src/lib/SoftHSM.cpp`:
- Around line 4522-4523: The DEBUG_MSG is logging arbitrary binary data via %s
(mldsaParam.additionalContext / ckSignAdditionalContext->pContext), which can
read past buffers and leak data; change the log to not print raw bytes—use the
context length (ckSignAdditionalContext->ulContextLen or
mldsaParam.additionalContext.size()) or produce a bounded hex/snippet of the
first N bytes when needed, and update both occurrences (the DEBUG_MSG near
mldsaParam.additionalContext and the other occurrence around lines 5621-5622) to
use the safe form (e.g., "additionalContext.len=%zu" or "additionalContext=%s"
with a prior safe hex-encoding of up to N bytes).
---
Outside diff comments:
In `@src/lib/crypto/OSSLMLDSA.cpp`:
- Around line 55-58: OSSLMLDSA's mechanismParameters member is not initialized
which can lead to undefined behavior when delete is called in
signFinal/verifyFinal; fix by initializing mechanismParameters to nullptr
(either in a new/default constructor for class OSSLMLDSA that sets
parameters=nullptr, paramLength=0, mechanismParameters=nullptr, or by explicitly
setting mechanismParameters = nullptr at the start of signInit and verifyInit
before any conditional cloning) so delete is only called on a valid pointer or
nullptr.
In `@src/lib/SoftHSM.cpp`:
- Around line 5587-5590: The ML-DSA branch currently disables multipart
operations (setting bAllowMultiPartOp = false and isMLDSA = true) which prevents
C_VerifyUpdate/C_VerifyFinal and never triggers verifyInit; change the
CKM_ML_DSA handling so multipart is allowed (do not set bAllowMultiPartOp =
false — set it true or omit the false override) while keeping AsymMech::MLDSA
and isMLDSA handling, and ensure the verify-side flow calls verifyInit when
isMLDSA is set so the backend verify state is prepared (verifyInit must be
invoked on the verify path where other mechanisms initialize multipart verify).
---
Nitpick comments:
In `@src/lib/crypto/BotanDSA.cpp`:
- Around line 136-140: The delegation to the base AsymmetricAlgorithm in
BotanDSA::signInit (and the corresponding BotanDSA::verifyInit) currently omits
the new mechanismParam argument, causing caller-supplied mechanism context to be
dropped; update the calls to AsymmetricAlgorithm::signInit and
AsymmetricAlgorithm::verifyInit to pass the mechanismParam (and paramLen if
applicable) through so the base methods receive the new parameter—search for
BotanDSA::signInit and BotanDSA::verifyInit and forward the mechanismParam into
their AsymmetricAlgorithm::*init calls.
In `@src/lib/crypto/BotanGOST.cpp`:
- Around line 64-68: The new mechanismParam argument is accepted but not being
forwarded to the base init calls; update BotanGOST::signInit to pass
mechanismParam through to AsymmetricAlgorithm::signInit (and likewise update the
corresponding verify/init delegation around the other block referenced at lines
200-205) so the AsymmetricAlgorithm::signInit/verifyInit calls receive the
MechanismParam pointer instead of being discarded; locate the calls to
AsymmetricAlgorithm::signInit and the matching verify/init wrapper in this file
and add mechanismParam as the final argument in those delegations.
In `@src/lib/crypto/OSSLGOST.cpp`:
- Around line 129-133: The OSSLGOST::signInit currently calls
AsymmetricAlgorithm::signInit without forwarding the MechanismParam*, causing
silent parameter drop; update OSSLGOST::signInit to pass the mechanismParam
through to AsymmetricAlgorithm::signInit (i.e., include the mechanismParam
argument in the call) and make the same change for the corresponding init method
(the verify/init variant around lines 315-320) so both OSSLGOST::signInit and
the verify/init method preserve and forward the mechanismParam to the base
class.
In `@src/lib/crypto/OSSLMLDSA.cpp`:
- Around line 179-183: In signInit, validate the type of mechanismParam before
cloning: if mechanismParam is non-NULL ensure it is an instance of
MLDSAMechanismParam (same check used in the one-shot sign() path) and
reject/return an error if it is not; only then assign mechanismParameters =
mechanismParam->clone(); update signInit to mirror the one-shot validation so
invalid types fail fast instead of being stored for later failure in
signFinal/sign().
- Around line 373-375: verifyInit clones mechanismParam into mechanismParameters
without validating its type, mirroring the same bug from signInit; update
verifyInit to perform the same type-check/validation used in signInit before
calling mechanismParam->clone() (reject or throw if the mechanismParam type is
unexpected) so only validated mechanismParam objects are cloned and assigned to
mechanismParameters.
- Line 33: The current DEBUG_MSG calls output full hex of sensitive buffers
(e.g., DEBUG_MSG with dataToSign and related debug lines); remove or guard these
prints by either omitting the hex payload in production (log only length or a
hash) or wrapping them behind a strict sensitive-logging flag/compile-time
check; specifically update usages of DEBUG_MSG in OSSLMLDSA.cpp that print
dataToSign, message bytes, or signature bytes to instead emit non-sensitive
metadata (e.g., data length or SHA256 digest) or be conditional on an explicit
SENSITIVE_DEBUG enabled flag.
In `@src/lib/crypto/OSSLMLDSA.h`:
- Around line 55-58: Remove the unused members parameters (void*) and
paramLength (size_t) from the OSSLMLDSA class/struct declaration and strip any
related handling in the implementation: remove any
constructor/destructor/initializer-list references, allocations, frees, copies,
or assignments for parameters/paramLength in OSSLMLDSA.cpp, and ensure multipart
flow continues to use mechanismParameters (const MechanismParam*) only; rebuild
to confirm no remaining references to these two symbols.
In `@src/lib/crypto/test/MLDSATests.cpp`:
- Line 455: The tests currently perform multipart signing but only one-shot
verification; update both occurrences (the existing
CPPUNIT_ASSERT(mldsa->verify(..., AsymMech::MLDSA)) checks) to exercise the
multipart verify path by calling mldsa->verifyInit(..., AsymMech::MLDSA,
&context) (pass &context as the 5th argument for the context test), then feed
the same data chunks via mldsa->verifyUpdate(&context, chunk) calls, and finally
call mldsa->verifyFinal(&context, signature) and assert the result is true; do
this for both the non-context and context variants (mirror the sign multipart
pattern) so verifyInit/verifyUpdate/verifyFinal are covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f678e0b4-76d4-4396-9726-332d611c1bf0
📒 Files selected for processing (35)
src/lib/SoftHSM.cppsrc/lib/crypto/AsymmetricAlgorithm.cppsrc/lib/crypto/AsymmetricAlgorithm.hsrc/lib/crypto/BotanDH.cppsrc/lib/crypto/BotanDH.hsrc/lib/crypto/BotanDSA.cppsrc/lib/crypto/BotanDSA.hsrc/lib/crypto/BotanECDH.cppsrc/lib/crypto/BotanECDH.hsrc/lib/crypto/BotanECDSA.cppsrc/lib/crypto/BotanECDSA.hsrc/lib/crypto/BotanEDDSA.cppsrc/lib/crypto/BotanEDDSA.hsrc/lib/crypto/BotanGOST.cppsrc/lib/crypto/BotanGOST.hsrc/lib/crypto/BotanRSA.cppsrc/lib/crypto/BotanRSA.hsrc/lib/crypto/OSSLDH.cppsrc/lib/crypto/OSSLDH.hsrc/lib/crypto/OSSLDSA.cppsrc/lib/crypto/OSSLDSA.hsrc/lib/crypto/OSSLECDH.cppsrc/lib/crypto/OSSLECDH.hsrc/lib/crypto/OSSLECDSA.cppsrc/lib/crypto/OSSLECDSA.hsrc/lib/crypto/OSSLEDDSA.cppsrc/lib/crypto/OSSLEDDSA.hsrc/lib/crypto/OSSLGOST.cppsrc/lib/crypto/OSSLGOST.hsrc/lib/crypto/OSSLMLDSA.cppsrc/lib/crypto/OSSLMLDSA.hsrc/lib/crypto/OSSLRSA.cppsrc/lib/crypto/OSSLRSA.hsrc/lib/crypto/test/MLDSATests.cppsrc/lib/crypto/test/MLDSATests.h
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)
5587-5590:⚠️ Potential issue | 🔴 CriticalEnable multipart for
CKM_ML_DSAverify too.Line 5589 still leaves
bAllowMultiPartOpdisabled, so Line 5780 never callsverifyInit(...). As a result,C_VerifyUpdate/C_VerifyFinalstill cannot run for ML-DSA, which leaves half of this PR’s feature unavailable.Suggested fix
case CKM_ML_DSA: mechanism = AsymMech::MLDSA; - bAllowMultiPartOp = false; + bAllowMultiPartOp = true; isMLDSA = true;Also applies to: 5780-5780
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/SoftHSM.cpp` around lines 5587 - 5590, The CKM_ML_DSA branch currently sets isMLDSA = true but leaves bAllowMultiPartOp false, preventing verifyInit(...) from being called and disabling C_VerifyUpdate/C_VerifyFinal for ML-DSA; update the CKM_ML_DSA case in the mechanism switch (where CKM_ML_DSA, mechanism = AsymMech::MLDSA, isMLDSA are set) to set bAllowMultiPartOp = true (or otherwise enable multipart ops for ML-DSA) so that the later verifyInit(...) call path is exercised and C_VerifyUpdate/C_VerifyFinal work for ML-DSA.
♻️ Duplicate comments (2)
src/lib/crypto/OSSLMLDSA.cpp (1)
387-391:⚠️ Potential issue | 🟡 MinorDebug messages incorrectly say "signUpdate" in
verifyUpdate.The debug messages reference "signUpdate" but this is the
verifyUpdatemethod - a copy-paste error.Proposed fix
- DEBUG_MSG("signUpdate originalData %s", originalData.hex_str().c_str()); + DEBUG_MSG("verifyUpdate originalData %s", originalData.hex_str().c_str()); message += originalData; - DEBUG_MSG("signUpdate message %s", message.hex_str().c_str()); + DEBUG_MSG("verifyUpdate message %s", message.hex_str().c_str());,
🤖 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 387 - 391, The debug lines in verifyUpdate mistakenly print "signUpdate"; update the DEBUG_MSG calls inside the verifyUpdate method to reference "verifyUpdate" instead of "signUpdate" so logs correctly identify the method (look for the DEBUG_MSG calls that log originalData.hex_str() and message.hex_str() near the verifyUpdate implementation and update those format strings).src/lib/crypto/test/MLDSATests.cpp (1)
492-503:⚠️ Potential issue | 🟠 MajorMemory leak and incorrect mechanismParam argument slot.
Two issues remain from previous review:
Memory leak (Line 493):
contextBSis allocated on the heap but never freed.Context passed to wrong argument (Lines 498, 503):
&contextis passed as theparamargument (3rd position), but themechanismParamargument (5th position) remains NULL. This test does not actually validate the mechanism context plumbing for multipart operations.Proposed fix
- std::string contextStr = std::string("HEDGE_PREFERRED"); - ByteString* contextBS = new ByteString((const unsigned char*)contextStr.c_str(), contextStr.size()); - - MLDSAMechanismParam context = MLDSAMechanismParam(Hedge::Type::HEDGE_PREFERRED, *contextBS); + const std::string contextStr("HEDGE_PREFERRED"); + const ByteString contextBS((const unsigned char*)contextStr.data(), contextStr.size()); + MLDSAMechanismParam context(Hedge::Type::HEDGE_PREFERRED, contextBS); ByteString sigMultiPart; - CPPUNIT_ASSERT(mldsa->signInit(kp->getPrivateKey(), AsymMech::MLDSA, &context, sizeof(context))); + CPPUNIT_ASSERT(mldsa->signInit(kp->getPrivateKey(), AsymMech::MLDSA, NULL, 0UL, &context)); CPPUNIT_ASSERT(mldsa->signUpdate(dataToSign)); CPPUNIT_ASSERT(mldsa->signUpdate(dataToSignMultipart)); CPPUNIT_ASSERT(mldsa->signFinal(sigMultiPart)); - CPPUNIT_ASSERT(mldsa->verify(kp->getPublicKey(), all, sigMultiPart, AsymMech::MLDSA, &context, sizeof(context))); + CPPUNIT_ASSERT(mldsa->verify(kp->getPublicKey(), all, sigMultiPart, AsymMech::MLDSA, NULL, 0UL, &context));Based on learnings: SoftHSM ML‑DSA additional context: avoid transient malloc in SoftHSM::{AsymSignInit, AsymVerifyInit}.
,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/crypto/test/MLDSATests.cpp` around lines 492 - 503, Free the heap allocation for contextBS (delete contextBS) after the test finishes to avoid the memory leak, and pass the MLDSAMechanismParam as the mechanismParam (5th argument) rather than the param (3rd argument): call mldsa->signInit(kp->getPrivateKey(), AsymMech::MLDSA, nullptr, 0, &context, sizeof(context)) (or equivalent API where the 3rd/4th args are param/paramLen and 5th/6th are mechanismParam/mechanismParamLen), and likewise pass nullptr for the param and &context for the mechanismParam in mldsa->verify; keep the MLDSAMechanismParam construction (MLDSAMechanismParam context = ...) and ensure delete contextBS is executed after sign/verify.
🧹 Nitpick comments (4)
src/lib/crypto/OSSLMLDSA.cpp (4)
396-411: SetmechanismParameterstonullptrafter deletion inverifyFinal.Same concern as
signFinal- the pointer should be nullified after deletion.Suggested fix
delete mechanismParameters; + mechanismParameters = NULL; message.resize(0);🤖 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 396 - 411, In OSSLMLDSA::verifyFinal, after deleting the mechanismParameters pointer do not leave a dangling pointer—set mechanismParameters to nullptr (same fix as in signFinal): locate OSSLMLDSA::verifyFinal and after the delete mechanismParameters statement assign mechanismParameters = nullptr so subsequent checks (e.g., DEBUG_MSG or later logic) won't use a freed pointer.
159-186: Consider resettingmechanismParametersbefore cloning to prevent leaks.If
signInitis called twice without an interveningsignFinal(e.g., in error recovery paths), the previousmechanismParameterswould leak.Suggested defensive fix
DEBUG_MSG("signInit mechanismParam != NULL: %d", mechanismParam != NULL); if (mechanismParam != NULL) { + delete mechanismParameters; // safe even if nullptr mechanismParameters = mechanismParam->clone(); + } else { + delete mechanismParameters; + mechanismParameters = NULL; }🤖 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 159 - 186, OSSLMLDSA::signInit can leak the previous mechanismParameters when called a second time because it clones mechanismParam without clearing the existing member; before cloning, safely release/reset the existing mechanismParameters (e.g., delete or reset the smart pointer and set to nullptr) so the old object is freed, then clone mechanismParam into mechanismParameters; keep the existing early-return/error paths (like the invalid key branch that calls AsymmetricAlgorithm::signFinal) unchanged.
204-219: SetmechanismParameterstonullptrafter deletion.After
delete mechanismParameters, the pointer is not reset. While unlikely in normal flow, if the object is reused or if there's an error path that leads to anothersignFinalcall, this could cause undefined behavior.Suggested fix
delete mechanismParameters; + mechanismParameters = NULL; message.resize(0);🤖 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 204 - 219, In OSSLMLDSA::signFinal the raw pointer mechanismParameters is deleted but not reset, risking use-after-free; after the delete mechanismParameters call set mechanismParameters = nullptr to avoid dangling pointer use (locate in function OSSLMLDSA::signFinal immediately after the delete), and ensure any subsequent paths or early returns use the nullptr-checked logic already present.
351-378: Same potential leak concern assignInit.Similar to
signInit, ifverifyInitis called twice without an interveningverifyFinal, the previousmechanismParameterswould leak.Suggested defensive fix
DEBUG_MSG("verifyInit mechanismParam != NULL: %d", mechanismParam != NULL); if (mechanismParam != NULL) { + delete mechanismParameters; mechanismParameters = mechanismParam->clone(); + } else { + delete mechanismParameters; + mechanismParameters = NULL; }🤖 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 351 - 378, The verifyInit method can leak the previous mechanismParameters if verifyInit is called twice; before assigning mechanismParameters = mechanismParam->clone() check if mechanismParameters is non-NULL and free it (e.g. delete mechanismParameters; mechanismParameters = NULL) or call the appropriate cleanup method, then assign the cloned value; also ensure the same defensive cleanup is applied in signInit and any other init methods that clone mechanismParameters so repeated inits don't leak.
🤖 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/OSSLMLDSA.h`:
- Around line 55-58: The mechanismParameters member is left uninitialized and
later deleted unconditionally, causing potential double-free/use-after-free; add
an explicit constructor for the OSSLMLDSA class that initializes
mechanismParameters to NULL, ensure signInit and verifyInit set
mechanismParameters only when mechanismParam is non-NULL, and after deleting
mechanismParameters in signFinal and verifyFinal assign mechanismParameters =
NULL to avoid dangling pointers and allow safe reuse of the instance.
---
Outside diff comments:
In `@src/lib/SoftHSM.cpp`:
- Around line 5587-5590: The CKM_ML_DSA branch currently sets isMLDSA = true but
leaves bAllowMultiPartOp false, preventing verifyInit(...) from being called and
disabling C_VerifyUpdate/C_VerifyFinal for ML-DSA; update the CKM_ML_DSA case in
the mechanism switch (where CKM_ML_DSA, mechanism = AsymMech::MLDSA, isMLDSA are
set) to set bAllowMultiPartOp = true (or otherwise enable multipart ops for
ML-DSA) so that the later verifyInit(...) call path is exercised and
C_VerifyUpdate/C_VerifyFinal work for ML-DSA.
---
Duplicate comments:
In `@src/lib/crypto/OSSLMLDSA.cpp`:
- Around line 387-391: The debug lines in verifyUpdate mistakenly print
"signUpdate"; update the DEBUG_MSG calls inside the verifyUpdate method to
reference "verifyUpdate" instead of "signUpdate" so logs correctly identify the
method (look for the DEBUG_MSG calls that log originalData.hex_str() and
message.hex_str() near the verifyUpdate implementation and update those format
strings).
In `@src/lib/crypto/test/MLDSATests.cpp`:
- Around line 492-503: Free the heap allocation for contextBS (delete contextBS)
after the test finishes to avoid the memory leak, and pass the
MLDSAMechanismParam as the mechanismParam (5th argument) rather than the param
(3rd argument): call mldsa->signInit(kp->getPrivateKey(), AsymMech::MLDSA,
nullptr, 0, &context, sizeof(context)) (or equivalent API where the 3rd/4th args
are param/paramLen and 5th/6th are mechanismParam/mechanismParamLen), and
likewise pass nullptr for the param and &context for the mechanismParam in
mldsa->verify; keep the MLDSAMechanismParam construction (MLDSAMechanismParam
context = ...) and ensure delete contextBS is executed after sign/verify.
---
Nitpick comments:
In `@src/lib/crypto/OSSLMLDSA.cpp`:
- Around line 396-411: In OSSLMLDSA::verifyFinal, after deleting the
mechanismParameters pointer do not leave a dangling pointer—set
mechanismParameters to nullptr (same fix as in signFinal): locate
OSSLMLDSA::verifyFinal and after the delete mechanismParameters statement assign
mechanismParameters = nullptr so subsequent checks (e.g., DEBUG_MSG or later
logic) won't use a freed pointer.
- Around line 159-186: OSSLMLDSA::signInit can leak the previous
mechanismParameters when called a second time because it clones mechanismParam
without clearing the existing member; before cloning, safely release/reset the
existing mechanismParameters (e.g., delete or reset the smart pointer and set to
nullptr) so the old object is freed, then clone mechanismParam into
mechanismParameters; keep the existing early-return/error paths (like the
invalid key branch that calls AsymmetricAlgorithm::signFinal) unchanged.
- Around line 204-219: In OSSLMLDSA::signFinal the raw pointer
mechanismParameters is deleted but not reset, risking use-after-free; after the
delete mechanismParameters call set mechanismParameters = nullptr to avoid
dangling pointer use (locate in function OSSLMLDSA::signFinal immediately after
the delete), and ensure any subsequent paths or early returns use the
nullptr-checked logic already present.
- Around line 351-378: The verifyInit method can leak the previous
mechanismParameters if verifyInit is called twice; before assigning
mechanismParameters = mechanismParam->clone() check if mechanismParameters is
non-NULL and free it (e.g. delete mechanismParameters; mechanismParameters =
NULL) or call the appropriate cleanup method, then assign the cloned value; also
ensure the same defensive cleanup is applied in signInit and any other init
methods that clone mechanismParameters so repeated inits don't leak.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2fde95d1-ae0d-4517-a3d7-50917fe99f8b
📒 Files selected for processing (35)
src/lib/SoftHSM.cppsrc/lib/crypto/AsymmetricAlgorithm.cppsrc/lib/crypto/AsymmetricAlgorithm.hsrc/lib/crypto/BotanDH.cppsrc/lib/crypto/BotanDH.hsrc/lib/crypto/BotanDSA.cppsrc/lib/crypto/BotanDSA.hsrc/lib/crypto/BotanECDH.cppsrc/lib/crypto/BotanECDH.hsrc/lib/crypto/BotanECDSA.cppsrc/lib/crypto/BotanECDSA.hsrc/lib/crypto/BotanEDDSA.cppsrc/lib/crypto/BotanEDDSA.hsrc/lib/crypto/BotanGOST.cppsrc/lib/crypto/BotanGOST.hsrc/lib/crypto/BotanRSA.cppsrc/lib/crypto/BotanRSA.hsrc/lib/crypto/OSSLDH.cppsrc/lib/crypto/OSSLDH.hsrc/lib/crypto/OSSLDSA.cppsrc/lib/crypto/OSSLDSA.hsrc/lib/crypto/OSSLECDH.cppsrc/lib/crypto/OSSLECDH.hsrc/lib/crypto/OSSLECDSA.cppsrc/lib/crypto/OSSLECDSA.hsrc/lib/crypto/OSSLEDDSA.cppsrc/lib/crypto/OSSLEDDSA.hsrc/lib/crypto/OSSLGOST.cppsrc/lib/crypto/OSSLGOST.hsrc/lib/crypto/OSSLMLDSA.cppsrc/lib/crypto/OSSLMLDSA.hsrc/lib/crypto/OSSLRSA.cppsrc/lib/crypto/OSSLRSA.hsrc/lib/crypto/test/MLDSATests.cppsrc/lib/crypto/test/MLDSATests.h
✅ Files skipped from review due to trivial changes (6)
- src/lib/crypto/test/MLDSATests.h
- src/lib/crypto/OSSLRSA.h
- src/lib/crypto/AsymmetricAlgorithm.cpp
- src/lib/crypto/BotanGOST.h
- src/lib/crypto/OSSLGOST.cpp
- src/lib/crypto/BotanDSA.h
🚧 Files skipped from review as they are similar to previous changes (10)
- src/lib/crypto/OSSLDSA.cpp
- src/lib/crypto/BotanECDSA.cpp
- src/lib/crypto/BotanRSA.h
- src/lib/crypto/BotanECDSA.h
- src/lib/crypto/BotanDH.cpp
- src/lib/crypto/OSSLDSA.h
- src/lib/crypto/BotanGOST.cpp
- src/lib/crypto/BotanEDDSA.h
- src/lib/crypto/OSSLDH.h
- src/lib/crypto/BotanEDDSA.cpp
antoinelochet
force-pushed
the
feature/ml-dsa-multi
branch
2 times, most recently
from
05770ac to
759b2b5
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/lib/crypto/OSSLMLDSA.h (1)
55-58:⚠️ Potential issue | 🔴 CriticalInitialize and clear
mechanismParametersto prevent invalid delete/use-after-free.Line [58] introduces raw pointer state with no visible constructor initialization here. With conditional assignment in init paths, finalizers can delete an indeterminate/stale pointer unless lifecycle is explicitly initialized and reset.
🔧 Proposed fix
class OSSLMLDSA : public AsymmetricAlgorithm { public: + OSSLMLDSA() : parameters(NULL), paramLength(0), mechanismParameters(NULL) { } // Destructor virtual ~OSSLMLDSA() { }bool OSSLMLDSA::signFinal(ByteString & signature) { ... delete mechanismParameters; + mechanismParameters = NULL; message.resize(0); ... } bool OSSLMLDSA::verifyFinal(const ByteString & signature) { ... delete mechanismParameters; + mechanismParameters = NULL; message.resize(0); ... }Use this read-only check to confirm constructor/init/reset paths:
#!/bin/bash set -euo pipefail echo "=== OSSLMLDSA class declaration ===" sed -n '/class OSSLMLDSA/,/^};/p' src/lib/crypto/OSSLMLDSA.h echo echo "=== Constructor and pointer lifecycle usage ===" rg -n 'OSSLMLDSA::OSSLMLDSA|mechanismParameters|delete mechanismParameters|signInit|verifyInit|signFinal|verifyFinal' src/lib/crypto/OSSLMLDSA.cpp src/lib/crypto/OSSLMLDSA.h -C2🤖 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 55 - 58, The member pointer mechanismParameters in class OSSLMLDSA is not initialized or cleared, risking use-after-free in destructor/reset paths; update the OSSLMLDSA constructor to explicitly initialize mechanismParameters to nullptr, ensure any reset/cleanup routine (and the destructor) checks for non-null before deleting and then sets mechanismParameters back to nullptr, and audit all users (signInit, verifyInit, signFinal, verifyFinal) to assign or copy into mechanismParameters only via safe allocation paths so there is a single clear ownership and lifecycle for mechanismParameters.
🤖 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/OSSLMLDSA.cpp`:
- Line 33: The DEBUG_MSG calls in OSSLMLDSA.cpp are printing full message
payloads (e.g., DEBUG_MSG("sign dataToSign: %s", dataToSign.hex_str().c_str()))
which leaks data and is expensive; replace these full hex-dumps with safe
diagnostics such as logging only the data length and a short deterministic
fingerprint (e.g., SHA-256 or truncated hash) or a redaction marker, and remove
any similar uses that call .hex_str() on message, signature, or multipart
buffers (search for DEBUG_MSG and dataToSign.hex_str(), signature.hex_str(), or
multipart accumulation calls in OSSLMLDSA.cpp). Ensure the log message includes
context (what field it refers to) and the computed fingerprint/length instead of
the raw contents.
- Around line 181-183: The code currently clones mechanismParam into
mechanismParameters but never resets mechanismParameters on init/final, leaving
dangling pointers after delete; update all init paths (where mechanismParam is
handled) to first delete and set mechanismParameters = NULL, then if
mechanismParam != NULL set mechanismParameters = mechanismParam->clone(); and in
all final/cleanup paths ensure you delete mechanismParameters and set it to
NULL; additionally audit signFinal and verifyFinal to treat mechanismParameters
as possibly NULL before use so multipart ops started without a mechanismParam
won’t reuse freed memory.
In `@src/lib/crypto/test/MLDSATests.cpp`:
- Around line 429-465: The test MLDSATests::testSigningMultiPartVerifying signs
data in multipart but only exercises the one-shot verify(kp->getPublicKey(),
all, sigMultiPart, AsymMech::MLDSA) path; add calls to
mldsa->verifyInit(kp->getPublicKey(), AsymMech::MLDSA), mldsa->verifyUpdate(...)
for each chunk (dataToSign and dataToSignMultipart) and
mldsa->verifyFinal(sigMultiPart) and assert the final verify result to cover the
multipart verify code path; locate these changes around the end of
testSigningMultiPartVerifying where sigMultiPart, all and kp are available and
replace or augment the existing one-shot verify assertion with the multipart
verifyInit/verifyUpdate/verifyFinal sequence.
---
Duplicate comments:
In `@src/lib/crypto/OSSLMLDSA.h`:
- Around line 55-58: The member pointer mechanismParameters in class OSSLMLDSA
is not initialized or cleared, risking use-after-free in destructor/reset paths;
update the OSSLMLDSA constructor to explicitly initialize mechanismParameters to
nullptr, ensure any reset/cleanup routine (and the destructor) checks for
non-null before deleting and then sets mechanismParameters back to nullptr, and
audit all users (signInit, verifyInit, signFinal, verifyFinal) to assign or copy
into mechanismParameters only via safe allocation paths so there is a single
clear ownership and lifecycle for mechanismParameters.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 04c675c6-6346-42ff-b1e4-5233ab0864cd
📒 Files selected for processing (35)
src/lib/SoftHSM.cppsrc/lib/crypto/AsymmetricAlgorithm.cppsrc/lib/crypto/AsymmetricAlgorithm.hsrc/lib/crypto/BotanDH.cppsrc/lib/crypto/BotanDH.hsrc/lib/crypto/BotanDSA.cppsrc/lib/crypto/BotanDSA.hsrc/lib/crypto/BotanECDH.cppsrc/lib/crypto/BotanECDH.hsrc/lib/crypto/BotanECDSA.cppsrc/lib/crypto/BotanECDSA.hsrc/lib/crypto/BotanEDDSA.cppsrc/lib/crypto/BotanEDDSA.hsrc/lib/crypto/BotanGOST.cppsrc/lib/crypto/BotanGOST.hsrc/lib/crypto/BotanRSA.cppsrc/lib/crypto/BotanRSA.hsrc/lib/crypto/OSSLDH.cppsrc/lib/crypto/OSSLDH.hsrc/lib/crypto/OSSLDSA.cppsrc/lib/crypto/OSSLDSA.hsrc/lib/crypto/OSSLECDH.cppsrc/lib/crypto/OSSLECDH.hsrc/lib/crypto/OSSLECDSA.cppsrc/lib/crypto/OSSLECDSA.hsrc/lib/crypto/OSSLEDDSA.cppsrc/lib/crypto/OSSLEDDSA.hsrc/lib/crypto/OSSLGOST.cppsrc/lib/crypto/OSSLGOST.hsrc/lib/crypto/OSSLMLDSA.cppsrc/lib/crypto/OSSLMLDSA.hsrc/lib/crypto/OSSLRSA.cppsrc/lib/crypto/OSSLRSA.hsrc/lib/crypto/test/MLDSATests.cppsrc/lib/crypto/test/MLDSATests.h
✅ Files skipped from review due to trivial changes (8)
- src/lib/crypto/OSSLECDH.h
- src/lib/crypto/BotanECDH.h
- src/lib/crypto/BotanGOST.cpp
- src/lib/crypto/OSSLDH.h
- src/lib/crypto/AsymmetricAlgorithm.h
- src/lib/crypto/OSSLGOST.h
- src/lib/crypto/BotanDH.cpp
- src/lib/crypto/BotanDH.h
🚧 Files skipped from review as they are similar to previous changes (10)
- src/lib/crypto/test/MLDSATests.h
- src/lib/crypto/BotanECDH.cpp
- src/lib/crypto/OSSLEDDSA.h
- src/lib/crypto/OSSLRSA.h
- src/lib/crypto/BotanDSA.cpp
- src/lib/crypto/BotanDSA.h
- src/lib/crypto/OSSLRSA.cpp
- src/lib/crypto/OSSLDSA.h
- src/lib/crypto/OSSLDH.cpp
- src/lib/SoftHSM.cpp
antoinelochet
force-pushed
the
feature/ml-dsa-multi
branch
from
759b2b5 to
63e5433
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/SoftHSM.cpp (1)
5587-5590:⚠️ Potential issue | 🔴 CriticalEnable multipart verify for
CKM_ML_DSA.Line 5589 still sets
bAllowMultiPartOptofalse, so Line 5780 never callsverifyInit(...)and the session is marked single-part only.C_VerifyUpdate/C_VerifyFinalwill therefore reject ML-DSA, which breaks the feature this PR is adding.🔧 Minimal fix
case CKM_ML_DSA: mechanism = AsymMech::MLDSA; - bAllowMultiPartOp = false; + bAllowMultiPartOp = true; isMLDSA = true;Also applies to: 5779-5794
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/SoftHSM.cpp` around lines 5587 - 5590, In the CKM_ML_DSA case in the switch (where mechanism is set to AsymMech::MLDSA and isMLDSA is set), change the multipart allowance so bAllowMultiPartOp is true (instead of false) so the code path will call verifyInit(...) later and the session will be treated as multipart; this ensures C_VerifyUpdate/C_VerifyFinal accept ML-DSA. Verify the same multipart flag logic around the verifyInit(...) call (lines that reference verifyInit, C_VerifyUpdate, C_VerifyFinal and isMLDSA) is consistent so ML-DSA sessions are not forced single-part.
♻️ Duplicate comments (1)
src/lib/crypto/test/MLDSATests.cpp (1)
460-460:⚠️ Potential issue | 🟠 MajorUse multipart verify API in these multipart tests.
Line [460] and Line [503] still verify with one-shot
verify(...), so the new multipart verify flow is not exercised by these tests.✅ Proposed test update
- CPPUNIT_ASSERT(mldsa->verify(kp->getPublicKey(), all, sigMultiPart, AsymMech::MLDSA)); + CPPUNIT_ASSERT(mldsa->verifyInit(kp->getPublicKey(), AsymMech::MLDSA)); + CPPUNIT_ASSERT(mldsa->verifyUpdate(dataToSign)); + CPPUNIT_ASSERT(mldsa->verifyUpdate(dataToSignMultipart)); + CPPUNIT_ASSERT(mldsa->verifyFinal(sigMultiPart));- CPPUNIT_ASSERT(mldsa->verify(kp->getPublicKey(), all, sigMultiPart, AsymMech::MLDSA, NULL, 0, &context)); + CPPUNIT_ASSERT(mldsa->verifyInit(kp->getPublicKey(), AsymMech::MLDSA, NULL, 0, &context)); + CPPUNIT_ASSERT(mldsa->verifyUpdate(dataToSign)); + CPPUNIT_ASSERT(mldsa->verifyUpdate(dataToSignMultipart)); + CPPUNIT_ASSERT(mldsa->verifyFinal(sigMultiPart));Also applies to: 503-503
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/crypto/test/MLDSATests.cpp` at line 460, Tests still call the one-shot verify(...) in multipart tests; replace those calls with the multipart verify flow: call mldsa->verifyInit(kp->getPublicKey(), AsymMech::MLDSA) (or the library's multipart-start API), feed each message chunk used to build sigMultiPart via mldsa->verifyUpdate(...) in the same order (e.g. the pieces that produced sigMultiPart instead of "all"), then finish with mldsa->verifyFinal(sigMultiPart) (or verifyFinish/verifyComplete) and assert the final result. Update the assertions that currently call mldsa->verify(kp->getPublicKey(), all, sigMultiPart, AsymMech::MLDSA) to use the init/update/final sequence so the multipart verify path is exercised (refer to mldsa, kp->getPublicKey(), sigMultiPart, and the verifyInit/verifyUpdate/verifyFinal methods).
🧹 Nitpick comments (1)
src/lib/crypto/OSSLMLDSA.h (1)
18-19: Initialize all newly added state members in the constructor.
parametersandparamLengthare declared as persistent state but not explicitly initialized. Even if not used yet, initializing them avoids accidental UB in future paths.Based on learnings: In C++11-compatible code, prefer explicit constructor/value initialization over relying on implicit defaults for class members.💡 Suggested tweak
- OSSLMLDSA() : mechanismParameters(NULL) { } + OSSLMLDSA() : parameters(NULL), paramLength(0), mechanismParameters(NULL) { }Also applies to: 57-60
🤖 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 18 - 19, The constructor OSSLMLDSA() currently only initializes mechanismParameters; explicitly initialize the newly added persistent members parameters and paramLength in that constructor (and any other constructors or initializers in the class) to safe defaults (e.g., parameters = nullptr and paramLength = 0) to avoid undefined behavior; update OSSLMLDSA() and any other ctor or member initializer blocks that touch parameters/paramLength to set these members alongside mechanismParameters.
🤖 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/OSSLMLDSA.cpp`:
- Around line 181-186: When detecting an invalid mechanism parameter type (the
check against MLDSAMechanismParam::type for variable mechanismParam), call the
asymmetric-operation finalizer to clear the operation state before returning
false — e.g. invoke AsymmetricAlgorithm::Final(...) (pass the current operation
handle/context member used by the class and ignore its return) so the init
failure does not leave the operation active; apply the same change to both
places where this check returns false (the mechanismParam checks around the two
reported blocks).
---
Outside diff comments:
In `@src/lib/SoftHSM.cpp`:
- Around line 5587-5590: In the CKM_ML_DSA case in the switch (where mechanism
is set to AsymMech::MLDSA and isMLDSA is set), change the multipart allowance so
bAllowMultiPartOp is true (instead of false) so the code path will call
verifyInit(...) later and the session will be treated as multipart; this ensures
C_VerifyUpdate/C_VerifyFinal accept ML-DSA. Verify the same multipart flag logic
around the verifyInit(...) call (lines that reference verifyInit,
C_VerifyUpdate, C_VerifyFinal and isMLDSA) is consistent so ML-DSA sessions are
not forced single-part.
---
Duplicate comments:
In `@src/lib/crypto/test/MLDSATests.cpp`:
- Line 460: Tests still call the one-shot verify(...) in multipart tests;
replace those calls with the multipart verify flow: call
mldsa->verifyInit(kp->getPublicKey(), AsymMech::MLDSA) (or the library's
multipart-start API), feed each message chunk used to build sigMultiPart via
mldsa->verifyUpdate(...) in the same order (e.g. the pieces that produced
sigMultiPart instead of "all"), then finish with
mldsa->verifyFinal(sigMultiPart) (or verifyFinish/verifyComplete) and assert the
final result. Update the assertions that currently call
mldsa->verify(kp->getPublicKey(), all, sigMultiPart, AsymMech::MLDSA) to use the
init/update/final sequence so the multipart verify path is exercised (refer to
mldsa, kp->getPublicKey(), sigMultiPart, and the
verifyInit/verifyUpdate/verifyFinal methods).
---
Nitpick comments:
In `@src/lib/crypto/OSSLMLDSA.h`:
- Around line 18-19: The constructor OSSLMLDSA() currently only initializes
mechanismParameters; explicitly initialize the newly added persistent members
parameters and paramLength in that constructor (and any other constructors or
initializers in the class) to safe defaults (e.g., parameters = nullptr and
paramLength = 0) to avoid undefined behavior; update OSSLMLDSA() and any other
ctor or member initializer blocks that touch parameters/paramLength to set these
members alongside mechanismParameters.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 095bc2f8-f4d3-4c99-9546-5bc71f87bebb
📒 Files selected for processing (35)
src/lib/SoftHSM.cppsrc/lib/crypto/AsymmetricAlgorithm.cppsrc/lib/crypto/AsymmetricAlgorithm.hsrc/lib/crypto/BotanDH.cppsrc/lib/crypto/BotanDH.hsrc/lib/crypto/BotanDSA.cppsrc/lib/crypto/BotanDSA.hsrc/lib/crypto/BotanECDH.cppsrc/lib/crypto/BotanECDH.hsrc/lib/crypto/BotanECDSA.cppsrc/lib/crypto/BotanECDSA.hsrc/lib/crypto/BotanEDDSA.cppsrc/lib/crypto/BotanEDDSA.hsrc/lib/crypto/BotanGOST.cppsrc/lib/crypto/BotanGOST.hsrc/lib/crypto/BotanRSA.cppsrc/lib/crypto/BotanRSA.hsrc/lib/crypto/OSSLDH.cppsrc/lib/crypto/OSSLDH.hsrc/lib/crypto/OSSLDSA.cppsrc/lib/crypto/OSSLDSA.hsrc/lib/crypto/OSSLECDH.cppsrc/lib/crypto/OSSLECDH.hsrc/lib/crypto/OSSLECDSA.cppsrc/lib/crypto/OSSLECDSA.hsrc/lib/crypto/OSSLEDDSA.cppsrc/lib/crypto/OSSLEDDSA.hsrc/lib/crypto/OSSLGOST.cppsrc/lib/crypto/OSSLGOST.hsrc/lib/crypto/OSSLMLDSA.cppsrc/lib/crypto/OSSLMLDSA.hsrc/lib/crypto/OSSLRSA.cppsrc/lib/crypto/OSSLRSA.hsrc/lib/crypto/test/MLDSATests.cppsrc/lib/crypto/test/MLDSATests.h
✅ Files skipped from review due to trivial changes (11)
- src/lib/crypto/BotanECDSA.h
- src/lib/crypto/BotanDH.cpp
- src/lib/crypto/BotanRSA.h
- src/lib/crypto/BotanGOST.cpp
- src/lib/crypto/OSSLECDSA.cpp
- src/lib/crypto/OSSLECDH.h
- src/lib/crypto/BotanRSA.cpp
- src/lib/crypto/BotanECDH.cpp
- src/lib/crypto/BotanDH.h
- src/lib/crypto/BotanECDH.h
- src/lib/crypto/BotanDSA.h
🚧 Files skipped from review as they are similar to previous changes (9)
- src/lib/crypto/test/MLDSATests.h
- src/lib/crypto/OSSLECDH.cpp
- src/lib/crypto/AsymmetricAlgorithm.cpp
- src/lib/crypto/BotanGOST.h
- src/lib/crypto/BotanEDDSA.h
- src/lib/crypto/OSSLRSA.cpp
- src/lib/crypto/OSSLDH.h
- src/lib/crypto/OSSLRSA.h
- src/lib/crypto/OSSLDSA.h
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)
5587-5625:⚠️ Potential issue | 🔴 CriticalEnable multipart verification for
CKM_ML_DSA.
bAllowMultiPartOpis stillfalsehere. That leavesC_VerifyUpdate/C_VerifyFinalunavailable for ML-DSA and also skips the newverifyInit(..., mechanismParam)path at Line 5780, so the PR only enables multipart signing.Suggested fix
case CKM_ML_DSA: mechanism = AsymMech::MLDSA; - bAllowMultiPartOp = false; + bAllowMultiPartOp = true; isMLDSA = true;Also applies to: 5779-5780
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/SoftHSM.cpp` around lines 5587 - 5625, The CKM_ML_DSA case currently sets bAllowMultiPartOp = false which prevents C_VerifyUpdate/C_VerifyFinal and skips the verifyInit(..., mechanismParam) multipart verification path; change the logic in the CKM_ML_DSA branch (where isMLDSA is set and mechanismParam = &mldsaParam) to set bAllowMultiPartOp = true so multipart verification is allowed for ML-DSA, ensuring the same mechanismParam (mldsaParam) is passed into the verifyInit(...) path (the new verifyInit call referenced around verifyInit(..., mechanismParam)); keep existing parameter validation and mldsaParam population unchanged.
♻️ Duplicate comments (1)
src/lib/crypto/test/MLDSATests.cpp (1)
455-460:⚠️ Potential issue | 🟠 MajorExercise multipart verify here as well.
Both new tests still finish with one-shot
verify(...), soverifyInit/verifyUpdate/verifyFinalremains untested. Please switch the final assertions to the streaming verify flow.🔧 Suggested update
- CPPUNIT_ASSERT(mldsa->verify(kp->getPublicKey(), all, sigMultiPart, AsymMech::MLDSA)); + CPPUNIT_ASSERT(mldsa->verifyInit(kp->getPublicKey(), AsymMech::MLDSA)); + CPPUNIT_ASSERT(mldsa->verifyUpdate(dataToSign)); + CPPUNIT_ASSERT(mldsa->verifyUpdate(dataToSignMultipart)); + CPPUNIT_ASSERT(mldsa->verifyFinal(sigMultiPart));- CPPUNIT_ASSERT(mldsa->verify(kp->getPublicKey(), all, sigMultiPart, AsymMech::MLDSA, NULL, 0, &context)); + CPPUNIT_ASSERT(mldsa->verifyInit(kp->getPublicKey(), AsymMech::MLDSA, NULL, 0, &context)); + CPPUNIT_ASSERT(mldsa->verifyUpdate(dataToSign)); + CPPUNIT_ASSERT(mldsa->verifyUpdate(dataToSignMultipart)); + CPPUNIT_ASSERT(mldsa->verifyFinal(sigMultiPart));Also applies to: 498-503
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/crypto/test/MLDSATests.cpp` around lines 455 - 460, The test currently exercises multipart signing via mldsa->signInit/signUpdate/signFinal but still calls the one-shot mldsa->verify(...) at the end; replace that final one-shot verify with the streaming verify flow by calling mldsa->verifyInit(kp->getPublicKey(), AsymMech::MLDSA), then feed the same chunks with mldsa->verifyUpdate(dataToSign) and mldsa->verifyUpdate(dataToSignMultipart) (or their equivalents), and finish with mldsa->verifyFinal(all, sigMultiPart) to assert the multipart verification succeeds; do the same replacement in the other occurrence around lines 498-503.
🤖 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/OSSLMLDSA.h`:
- Line 18: The OSSLMLDSA() constructor currently only initializes
mechanismParameters leaving members parameters and paramLength uninitialized;
update the OSSLMLDSA() constructor in class OSSLMLDSA to explicitly initialize
message (if present), set parameters to NULL (or nullptr) and paramLength to 0
alongside mechanismParameters to ensure defined state (e.g. initialize
message(), parameters(NULL), paramLength(0), mechanismParameters(NULL)).
---
Outside diff comments:
In `@src/lib/SoftHSM.cpp`:
- Around line 5587-5625: The CKM_ML_DSA case currently sets bAllowMultiPartOp =
false which prevents C_VerifyUpdate/C_VerifyFinal and skips the verifyInit(...,
mechanismParam) multipart verification path; change the logic in the CKM_ML_DSA
branch (where isMLDSA is set and mechanismParam = &mldsaParam) to set
bAllowMultiPartOp = true so multipart verification is allowed for ML-DSA,
ensuring the same mechanismParam (mldsaParam) is passed into the verifyInit(...)
path (the new verifyInit call referenced around verifyInit(...,
mechanismParam)); keep existing parameter validation and mldsaParam population
unchanged.
---
Duplicate comments:
In `@src/lib/crypto/test/MLDSATests.cpp`:
- Around line 455-460: The test currently exercises multipart signing via
mldsa->signInit/signUpdate/signFinal but still calls the one-shot
mldsa->verify(...) at the end; replace that final one-shot verify with the
streaming verify flow by calling mldsa->verifyInit(kp->getPublicKey(),
AsymMech::MLDSA), then feed the same chunks with mldsa->verifyUpdate(dataToSign)
and mldsa->verifyUpdate(dataToSignMultipart) (or their equivalents), and finish
with mldsa->verifyFinal(all, sigMultiPart) to assert the multipart verification
succeeds; do the same replacement in the other occurrence around lines 498-503.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9b93a7e2-7a90-43dd-b23e-c0b2d3e2c0ca
📒 Files selected for processing (35)
src/lib/SoftHSM.cppsrc/lib/crypto/AsymmetricAlgorithm.cppsrc/lib/crypto/AsymmetricAlgorithm.hsrc/lib/crypto/BotanDH.cppsrc/lib/crypto/BotanDH.hsrc/lib/crypto/BotanDSA.cppsrc/lib/crypto/BotanDSA.hsrc/lib/crypto/BotanECDH.cppsrc/lib/crypto/BotanECDH.hsrc/lib/crypto/BotanECDSA.cppsrc/lib/crypto/BotanECDSA.hsrc/lib/crypto/BotanEDDSA.cppsrc/lib/crypto/BotanEDDSA.hsrc/lib/crypto/BotanGOST.cppsrc/lib/crypto/BotanGOST.hsrc/lib/crypto/BotanRSA.cppsrc/lib/crypto/BotanRSA.hsrc/lib/crypto/OSSLDH.cppsrc/lib/crypto/OSSLDH.hsrc/lib/crypto/OSSLDSA.cppsrc/lib/crypto/OSSLDSA.hsrc/lib/crypto/OSSLECDH.cppsrc/lib/crypto/OSSLECDH.hsrc/lib/crypto/OSSLECDSA.cppsrc/lib/crypto/OSSLECDSA.hsrc/lib/crypto/OSSLEDDSA.cppsrc/lib/crypto/OSSLEDDSA.hsrc/lib/crypto/OSSLGOST.cppsrc/lib/crypto/OSSLGOST.hsrc/lib/crypto/OSSLMLDSA.cppsrc/lib/crypto/OSSLMLDSA.hsrc/lib/crypto/OSSLRSA.cppsrc/lib/crypto/OSSLRSA.hsrc/lib/crypto/test/MLDSATests.cppsrc/lib/crypto/test/MLDSATests.h
✅ Files skipped from review due to trivial changes (5)
- src/lib/crypto/BotanGOST.h
- src/lib/crypto/BotanRSA.h
- src/lib/crypto/BotanECDH.h
- src/lib/crypto/OSSLRSA.cpp
- src/lib/crypto/BotanDSA.cpp
🚧 Files skipped from review as they are similar to previous changes (8)
- src/lib/crypto/OSSLRSA.h
- src/lib/crypto/OSSLDH.h
- src/lib/crypto/OSSLDH.cpp
- src/lib/crypto/AsymmetricAlgorithm.h
- src/lib/crypto/OSSLEDDSA.cpp
- src/lib/crypto/OSSLDSA.h
- src/lib/crypto/OSSLMLDSA.cpp
- src/lib/crypto/BotanEDDSA.h
antoinelochet
force-pushed
the
feature/ml-dsa-multi
branch
from
63e5433 to
93dec04
Compare
antoinelochet
force-pushed
the
feature/ml-dsa-multi
branch
from
93dec04 to
ba6d57a
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds the multiple-part support for ML-DSA.
Follow-up of #809
Summary by CodeRabbit
New Features
Tests