Support multipart operation on hashed ECDSA#857
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughHashed ECDSA mechanisms now allow multipart signing and verification in SoftHSM. The Botan and OpenSSL backends accumulate hash state across updates, finalize the digest, and perform ECDSA operations. Tests now cover single-part and multi-part SHA-based ECDSA flows across EC key types. ChangesMultipart ECDSA support
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/crypto/OSSLECDSA.cpp (1)
427-453:⚠️ Potential issue | 🟡 MinorSame
bn_r/bn_sleak still present in single-shotverify()(and a siblingsigleak on hash failure).The "Fix potentially leaking BNs" commit applied the
BN_free(bn_r); BN_free(bn_s);guard to the newverifyFinal(lines 631-632) but the analogous code in the unchanged single-shotverify()at lines 430-436 still leaks both BIGNUMs whenBN_bin2bnsucceeds butECDSA_SIG_set0fails.In addition, the hash-prep block at lines 442-453 (entered after
sigis already allocated at line 421) returnsfalseon hash failure without callingECDSA_SIG_free(sig)— leaking theECDSA_SIGobject as well. Worth fixing in the same PR for consistency with the multipart fix, since both are identical in spirit.🩹 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; } // Pre-hash the data if necessary ByteString prepDataToSign; if (hash == HashAlgo::Unknown) { prepDataToSign = originalData; } else { HashAlgorithm* digest = CryptoFactory::i()->getHashAlgorithm(hash); if (!digest->hashInit() || !digest->hashUpdate(originalData) || !digest->hashFinal(prepDataToSign)) { delete digest; + ECDSA_SIG_free(sig); return false; } delete digest; }🤖 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 427 - 453, The verify() path leaks bn_r/bn_s when BN_bin2bn succeeds but ECDSA_SIG_set0(sig, bn_r, bn_s) fails, and it also leaks the allocated sig on hash preparation failure; update verify() to free bn_r and bn_s if ECDSA_SIG_set0 returns false (and only free them in that error path, since on success ownership is transferred), and ensure ECDSA_SIG_free(sig) is called before any early return from the hash-prep block (i.e., when digest->hashInit()/hashUpdate()/hashFinal fails) to avoid leaking sig—mirror the cleanup approach used in verifyFinal to handle these resources safely.
🧹 Nitpick comments (1)
src/lib/crypto/OSSLECDSA.cpp (1)
196-222: Consider extracting the mechanism→hash mapping into a helper.The same
switch (mechanism)mappingAsymMech::ECDSA_SHA*→HashAlgo::SHA*now appears four times in this file (lines 77-97 insign, 198-222 insignInit, 352-372 inverify, 490-514 inverifyInit). Extracting it into a small static helper (returning theHashAlgo::Typeand a success flag, orHashAlgo::Unknownon rejection) would remove ~80 lines of duplication and prevent the four copies from drifting if a new ECDSA-SHA mechanism is ever added.The OpenSSL method-set block (
ECDSA_set_method/EC_KEY_set_methodwith FIPS variants, lines 119-132 / 297-310 / 394-407 / 589-602) is similarly duplicated and could be extracted into a helper for the same reason.🤖 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 196 - 222, Extract the repeated mechanism→hash switch into a single static helper (e.g. getHashForEcdsaMechanism or mapEcdsaMechToHash) that takes AsymMech and returns HashAlgo::Type (or HashAlgo::Unknown on failure); replace the switch blocks in sign, signInit, verify, and verifyInit with calls to this helper and use its return value to handle the error path. Likewise, factor the duplicated OpenSSL method selection (the ECDSA_set_method/EC_KEY_set_method and FIPS variants) into another static helper (e.g. selectEcMethodForFips) and call it from the same four sites to centralize the logic and remove duplication.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/lib/crypto/OSSLECDSA.cpp`:
- Around line 427-453: The verify() path leaks bn_r/bn_s when BN_bin2bn succeeds
but ECDSA_SIG_set0(sig, bn_r, bn_s) fails, and it also leaks the allocated sig
on hash preparation failure; update verify() to free bn_r and bn_s if
ECDSA_SIG_set0 returns false (and only free them in that error path, since on
success ownership is transferred), and ensure ECDSA_SIG_free(sig) is called
before any early return from the hash-prep block (i.e., when
digest->hashInit()/hashUpdate()/hashFinal fails) to avoid leaking sig—mirror the
cleanup approach used in verifyFinal to handle these resources safely.
---
Nitpick comments:
In `@src/lib/crypto/OSSLECDSA.cpp`:
- Around line 196-222: Extract the repeated mechanism→hash switch into a single
static helper (e.g. getHashForEcdsaMechanism or mapEcdsaMechToHash) that takes
AsymMech and returns HashAlgo::Type (or HashAlgo::Unknown on failure); replace
the switch blocks in sign, signInit, verify, and verifyInit with calls to this
helper and use its return value to handle the error path. Likewise, factor the
duplicated OpenSSL method selection (the ECDSA_set_method/EC_KEY_set_method and
FIPS variants) into another static helper (e.g. selectEcMethodForFips) and call
it from the same four sites to centralize the logic and remove duplication.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 81b9f352-6884-4d27-aa0a-0afd1fb3dea7
📒 Files selected for processing (2)
src/lib/SoftHSM.cppsrc/lib/crypto/OSSLECDSA.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/SoftHSM.cpp
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>
3 participants
This implements missing support for multipart ECDSA hasing including OpenSSL and Botan support.
Summary by CodeRabbit
New Features
Bug Fixes
Tests