Support multipart operation on hashed ECDSA#857
Conversation
Signed-off-by: Jakub Jelen <jjelen@redhat.com>
This does these things: * runs mutlipart tests on hashed ECDSA mechanisms * reorders tests to prevent generating dozens of keys Signed-off-by: Jakub Jelen <jjelen@redhat.com>
📝 WalkthroughWalkthroughThis PR enables multipart ECDSA signing and verification for ECDSA-with-SHA mechanisms (CKM_ECDSA_SHA1/224/256/384/512) by implementing working multipart flows in Botan and OpenSSL backends, updating SoftHSM framework configuration, and expanding test coverage. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant SoftHSM as SoftHSM::AsymSignInit
participant Backend as Backend (Botan/OSSLECDSA)
participant Hash as HashAlgorithm
Client->>SoftHSM: signInit(privateKey, CKM_ECDSA_SHA256, ...)
SoftHSM->>Backend: signInit(privateKey, mechanism)
Backend->>Backend: Validate private key type
Backend->>Backend: Map CKM_ECDSA_SHA256 to hash algo
Backend->>Hash: allocate & initialize pCurrentHash
alt Hash initialization fails
Backend->>Backend: cleanup, call signFinal(dummy)
Backend-->>SoftHSM: return false
else Success
Backend-->>SoftHSM: return true
end
Client->>SoftHSM: signUpdate(dataChunk1)
SoftHSM->>Backend: signUpdate(dataChunk1)
Backend->>Hash: hashUpdate(dataChunk1)
alt Hash update fails
Backend->>Backend: cleanup pCurrentHash, call signFinal(dummy)
Backend-->>SoftHSM: return false
else Success
Backend-->>SoftHSM: return true
end
Client->>SoftHSM: signUpdate(dataChunk2)
SoftHSM->>Backend: signUpdate(dataChunk2)
Backend->>Hash: hashUpdate(dataChunk2)
Backend-->>SoftHSM: return true
Client->>SoftHSM: signFinal(signature)
SoftHSM->>Backend: signFinal(signature)
Backend->>Hash: hashFinal() → finalized hash
Backend->>Backend: delete pCurrentHash
Backend->>Backend: Create ECDSA Signer/Verifier
Backend->>Backend: sign_message(finalized hash) → signature bytes
Backend->>Backend: Serialize to signature buffer
Backend-->>SoftHSM: return true
SoftHSM-->>Client: signature
sequenceDiagram
participant Client as Client
participant SoftHSM as SoftHSM::AsymVerifyInit
participant Backend as Backend (Botan/OSSLECDSA)
participant Hash as HashAlgorithm
Client->>SoftHSM: verifyInit(publicKey, CKM_ECDSA_SHA256, ...)
SoftHSM->>Backend: verifyInit(publicKey, mechanism)
Backend->>Backend: Validate public key type
Backend->>Backend: Map CKM_ECDSA_SHA256 to hash algo
Backend->>Hash: allocate & initialize pCurrentHash
alt Hash initialization fails
Backend->>Backend: cleanup, call verifyFinal(dummy)
Backend-->>SoftHSM: return false
else Success
Backend-->>SoftHSM: return true
end
Client->>SoftHSM: verifyUpdate(dataChunk1)
SoftHSM->>Backend: verifyUpdate(dataChunk1)
Backend->>Hash: hashUpdate(dataChunk1)
Backend-->>SoftHSM: return true
Client->>SoftHSM: verifyUpdate(dataChunk2)
SoftHSM->>Backend: verifyUpdate(dataChunk2)
Backend->>Hash: hashUpdate(dataChunk2)
Backend-->>SoftHSM: return true
Client->>SoftHSM: verifyFinal(signature)
SoftHSM->>Backend: verifyFinal(signature)
Backend->>Hash: hashFinal() → finalized hash
Backend->>Backend: delete pCurrentHash
Backend->>Backend: Parse signature buffer (r||s format)
Backend->>Backend: Create ECDSA Verifier
Backend->>Backend: verify_message(finalized hash, signature)
alt Verification succeeds
Backend-->>SoftHSM: return true
else Verification fails
Backend-->>SoftHSM: return false
end
SoftHSM-->>Client: verification result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/lib/crypto/BotanECDSA.cpp (1)
197-223: Factor the mechanism-to-hash mapping into one helper.These new switch blocks repeat logic that already exists in the one-shot
sign()andverify()paths, so single-part and multipart support can drift the next time an ECDSA hash mechanism is added or adjusted.♻️ Helper sketch
+static bool mapEcdsaHashMechanism(AsymMech::Type mechanism, HashAlgo::Type& hash) +{ + switch (mechanism) + { + case AsymMech::ECDSA_SHA1: hash = HashAlgo::SHA1; return true; + case AsymMech::ECDSA_SHA224: hash = HashAlgo::SHA224; return true; + case AsymMech::ECDSA_SHA256: hash = HashAlgo::SHA256; return true; + case AsymMech::ECDSA_SHA384: hash = HashAlgo::SHA384; return true; + case AsymMech::ECDSA_SHA512: hash = HashAlgo::SHA512; return true; + default: return false; + } +}Also applies to: 463-487
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/crypto/BotanECDSA.cpp` around lines 197 - 223, Extract the repeated mechanism-to-hash switch into a single helper (e.g. a static/private method like BotanECDSA::mechanismToHash or mapMechanismToHash) that takes AsymMech mechanism and returns HashAlgo::Type (or throws/returns an optional/error); replace the switch in the sign(), verify() and signFinal()-path (the block referencing HashAlgo::Type hash = HashAlgo::Unknown and the one at lines ~463-487) with a call to this helper, and ensure the helper performs the same default/error handling (calls ERROR_MSG and invokes AsymmetricAlgorithm::signFinal(dummy)/returns false or propagates failure) so behavior remains unchanged.src/lib/test/SignVerifyTests.cpp (1)
553-557: Exercise more than one update call in the multipart path.These new
signVerifyMulti(...)calls still go through a helper that feeds the whole message in a singleC_SignUpdate/C_VerifyUpdate. That means the added coverage does not actually validate chunk accumulation, which is the main regression surface of this PR. Please split the payload across at least two updates here or in a dedicated helper for the hashed ECDSA cases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/test/SignVerifyTests.cpp` around lines 553 - 557, The tests call signVerifyMulti(...) for various CKM_ECDSA_* mechanisms but feed the entire message in a single C_SignUpdate/C_VerifyUpdate, so multipart chunking isn't exercised; change signVerifyMulti or add a dedicated helper (e.g., signVerifyMultiChunked or adjust signVerifyMulti) to split the payload into at least two C_SignUpdate calls and corresponding C_VerifyUpdate calls for the hashed ECDSA paths (CKM_ECDSA_SHA1, SHA224, SHA256, SHA384, SHA512), ensuring both the sign and verify flows accumulate chunks before finishing the operation.
🤖 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/OSSLECDSA.cpp`:
- Around line 624-632: The code leaks BIGNUMs when BN_bin2bn succeeds but
ECDSA_SIG_set0(sig, bn_r, bn_s) fails; update the error path in the block that
checks (bn_r == NULL || bn_s == NULL || !ECDSA_SIG_set0(...)) to free any
non-NULL bn_r and bn_s before freeing sig and returning false (i.e., if bn_r !=
NULL call BN_free(bn_r); if bn_s != NULL call BN_free(bn_s)); keep the current
behavior on success since ownership is transferred by ECDSA_SIG_set0.
In `@src/lib/SoftHSM.cpp`:
- Line 4443: AsymVerifyInit currently leaves multipart disabled for hashed ECDSA
mechanisms; update the CKM_ECDSA_SHA1 / CKM_ECDSA_SHA224 / CKM_ECDSA_SHA256 /
CKM_ECDSA_SHA384 / CKM_ECDSA_SHA512 branches in SoftHSM::AsymVerifyInit to set
bAllowMultiPartOp = true (same as the signing side), so
C_VerifyUpdate/C_VerifyFinal will be allowed for these mechanisms; modify the
branches handling those mechanism constants to enable multipart by assigning
bAllowMultiPartOp = true.
---
Nitpick comments:
In `@src/lib/crypto/BotanECDSA.cpp`:
- Around line 197-223: Extract the repeated mechanism-to-hash switch into a
single helper (e.g. a static/private method like BotanECDSA::mechanismToHash or
mapMechanismToHash) that takes AsymMech mechanism and returns HashAlgo::Type (or
throws/returns an optional/error); replace the switch in the sign(), verify()
and signFinal()-path (the block referencing HashAlgo::Type hash =
HashAlgo::Unknown and the one at lines ~463-487) with a call to this helper, and
ensure the helper performs the same default/error handling (calls ERROR_MSG and
invokes AsymmetricAlgorithm::signFinal(dummy)/returns false or propagates
failure) so behavior remains unchanged.
In `@src/lib/test/SignVerifyTests.cpp`:
- Around line 553-557: The tests call signVerifyMulti(...) for various
CKM_ECDSA_* mechanisms but feed the entire message in a single
C_SignUpdate/C_VerifyUpdate, so multipart chunking isn't exercised; change
signVerifyMulti or add a dedicated helper (e.g., signVerifyMultiChunked or
adjust signVerifyMulti) to split the payload into at least two C_SignUpdate
calls and corresponding C_VerifyUpdate calls for the hashed ECDSA paths
(CKM_ECDSA_SHA1, SHA224, SHA256, SHA384, SHA512), ensuring both the sign and
verify flows accumulate chunks before finishing the operation.
🪄 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: dab0a6df-5a9f-40be-bba7-1f4c19e2c993
📒 Files selected for processing (6)
src/lib/SoftHSM.cppsrc/lib/crypto/BotanECDSA.cppsrc/lib/crypto/BotanECDSA.hsrc/lib/crypto/OSSLECDSA.cppsrc/lib/crypto/OSSLECDSA.hsrc/lib/test/SignVerifyTests.cpp
| const unsigned char *s = signature.const_byte_str(); | ||
| BIGNUM* bn_r = BN_bin2bn(s, len, NULL); | ||
| BIGNUM* bn_s = BN_bin2bn(s + len, len, NULL); | ||
| if (bn_r == NULL || bn_s == NULL || | ||
| !ECDSA_SIG_set0(sig, bn_r, bn_s)) | ||
| { | ||
| ERROR_MSG("Could not add data to the ECDSA_SIG object"); | ||
| ECDSA_SIG_free(sig); | ||
| return false; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
OpenSSL documentation for ECDSA_SIG_set0: does the function take ownership of BIGNUM* randBIGNUM* sonly on success, and must callers free them ifECDSA_SIG_set0 fails?
💡 Result:
Yes, the OpenSSL documentation for ECDSA_SIG_set0 states that the function takes ownership of the provided BIGNUM* r and BIGNUM* s only on success. Callers must free them if the function fails (returns 0). The official documentation explicitly shows this behavior in the example code: after a successful call to ECDSA_SIG_set0(obj, rbn, sbn), the code sets rbn = sbn = NULL to indicate they are now owned by obj and should not be freed by the caller. Conversely, if !ECDSA_SIG_set0(...) /* error */, the code does not set them to NULL, implying the caller retains ownership and must free them. The description confirms: "Calling this function transfers the memory management of the values to the ECDSA_SIG object, and therefore the values that have been passed in should not be freed by the caller." Combined with the return value (1 on success, 0 on failure), ownership transfer occurs only on success. This pattern matches OpenSSL's standard "set0" semantics (vs "set1" which duplicates), where ownership is stolen only if the operation succeeds.
Citations:
- 1: https://openssl.org/docs/man3.0/man3/ECDSA_SIG_set0.html
- 2: https://manpages.opensuse.org/Tumbleweed/openssl-3-doc/ECDSA_SIG_set0.33ssl.en.html
- 3: https://man.archlinux.org/man/ECDSA_SIG_new.3.en
- 4: https://docs.openssl.org/3.4/man3/ECDSA_SIG_new/
Free bn_r and bn_s if ECDSA_SIG_set0() fails.
OpenSSL's ECDSA_SIG_set0() transfers ownership of the BIGNUM pointers only on success. If one or both BN_bin2bn() calls succeed but ECDSA_SIG_set0() then fails, the allocated numbers leak because the error path only frees sig, not the orphaned bn_r and bn_s.
🩹 Suggested fix
const unsigned char *s = signature.const_byte_str();
BIGNUM* bn_r = BN_bin2bn(s, len, NULL);
BIGNUM* bn_s = BN_bin2bn(s + len, len, NULL);
if (bn_r == NULL || bn_s == NULL ||
!ECDSA_SIG_set0(sig, bn_r, bn_s))
{
ERROR_MSG("Could not add data to the ECDSA_SIG object");
+ BN_free(bn_r);
+ BN_free(bn_s);
ECDSA_SIG_free(sig);
return false;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const unsigned char *s = signature.const_byte_str(); | |
| BIGNUM* bn_r = BN_bin2bn(s, len, NULL); | |
| BIGNUM* bn_s = BN_bin2bn(s + len, len, NULL); | |
| if (bn_r == NULL || bn_s == NULL || | |
| !ECDSA_SIG_set0(sig, bn_r, bn_s)) | |
| { | |
| ERROR_MSG("Could not add data to the ECDSA_SIG object"); | |
| ECDSA_SIG_free(sig); | |
| return false; | |
| const unsigned char *s = signature.const_byte_str(); | |
| BIGNUM* bn_r = BN_bin2bn(s, len, NULL); | |
| BIGNUM* bn_s = BN_bin2bn(s + len, len, NULL); | |
| if (bn_r == NULL || bn_s == NULL || | |
| !ECDSA_SIG_set0(sig, bn_r, bn_s)) | |
| { | |
| ERROR_MSG("Could not add data to the ECDSA_SIG object"); | |
| BN_free(bn_r); | |
| BN_free(bn_s); | |
| ECDSA_SIG_free(sig); | |
| return false; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/crypto/OSSLECDSA.cpp` around lines 624 - 632, The code leaks BIGNUMs
when BN_bin2bn succeeds but ECDSA_SIG_set0(sig, bn_r, bn_s) fails; update the
error path in the block that checks (bn_r == NULL || bn_s == NULL ||
!ECDSA_SIG_set0(...)) to free any non-NULL bn_r and bn_s before freeing sig and
returning false (i.e., if bn_r != NULL call BN_free(bn_r); if bn_s != NULL call
BN_free(bn_s)); keep the current behavior on success since ownership is
transferred by ECDSA_SIG_set0.
| case CKM_ECDSA_SHA1: | ||
| mechanism = AsymMech::ECDSA_SHA1; | ||
| bAllowMultiPartOp = false; | ||
| bAllowMultiPartOp = true; |
There was a problem hiding this comment.
Enable multipart verify for the same hashed ECDSA mechanisms.
This only turns multipart on for signing. C_VerifyUpdate/C_VerifyFinal will still fail for CKM_ECDSA_SHA1/224/256/384/512 because SoftHSM::AsymVerifyInit still leaves bAllowMultiPartOp = false (see Line 5541 and the matching SHA224/256/384/512 cases below).
Suggested fix
case CKM_ECDSA_SHA1:
mechanism = AsymMech::ECDSA_SHA1;
- bAllowMultiPartOp = false;
+ bAllowMultiPartOp = true;
isECDSA = true;
break;
case CKM_ECDSA_SHA224:
mechanism = AsymMech::ECDSA_SHA224;
- bAllowMultiPartOp = false;
+ bAllowMultiPartOp = true;
isECDSA = true;
break;
case CKM_ECDSA_SHA256:
mechanism = AsymMech::ECDSA_SHA256;
- bAllowMultiPartOp = false;
+ bAllowMultiPartOp = true;
isECDSA = true;
break;
case CKM_ECDSA_SHA384:
mechanism = AsymMech::ECDSA_SHA384;
- bAllowMultiPartOp = false;
+ bAllowMultiPartOp = true;
isECDSA = true;
break;
case CKM_ECDSA_SHA512:
mechanism = AsymMech::ECDSA_SHA512;
- bAllowMultiPartOp = false;
+ bAllowMultiPartOp = true;
isECDSA = true;
break;Also applies to: 4448-4448, 4453-4453, 4458-4458, 4463-4463
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/SoftHSM.cpp` at line 4443, AsymVerifyInit currently leaves multipart
disabled for hashed ECDSA mechanisms; update the CKM_ECDSA_SHA1 /
CKM_ECDSA_SHA224 / CKM_ECDSA_SHA256 / CKM_ECDSA_SHA384 / CKM_ECDSA_SHA512
branches in SoftHSM::AsymVerifyInit to set bAllowMultiPartOp = true (same as the
signing side), so C_VerifyUpdate/C_VerifyFinal will be allowed for these
mechanisms; modify the branches handling those mechanism constants to enable
multipart by assigning bAllowMultiPartOp = true.
2 participants
This implements missing support for multipart ECDSA hasing including OpenSSL and Botan support.
Summary by CodeRabbit
New Features
Bug Fixes
Tests