fix: check key type in C_SignInit/C_VerifyInit to prevent crash#854
Open
mingulov wants to merge 1 commit intosofthsm:mainfrom
Open
fix: check key type in C_SignInit/C_VerifyInit to prevent crash#854mingulov wants to merge 1 commit intosofthsm:mainfrom
mingulov wants to merge 1 commit intosofthsm:mainfrom
Conversation
📝 WalkthroughWalkthroughThis PR updates asymmetric sign/verify initialization to fetch key type up-front, enforce per-mechanism key-type consistency (including explicit GOST handling), and adds tests asserting Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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.
🧹 Nitpick comments (1)
src/lib/test/SignVerifyTests.cpp (1)
1290-1439: Optional: extract shared “wrong key type” assertion helper to reduce duplication.Both new tests repeat the same mechanism-loop/assert logic; a small helper would keep future mechanism list updates in one place.
Refactor sketch
+static void assertInitWrongKeyType( + CK_SESSION_HANDLE hSession, + CK_OBJECT_HANDLE hKey, + const CK_MECHANISM_TYPE* mechs, + size_t mechCount, + CK_RV (*initFn)(CK_SESSION_HANDLE, CK_MECHANISM_PTR, CK_OBJECT_HANDLE)) +{ + for (size_t i = 0; i < mechCount; i++) + { + CK_MECHANISM mechanism = { mechs[i], NULL_PTR, 0 }; + CK_RV rv = initFn(hSession, &mechanism, hKey); + CPPUNIT_ASSERT(rv == CKR_KEY_TYPE_INCONSISTENT); + } +}🤖 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 1290 - 1439, Both tests testSignInitWrongKeyType and testVerifyInitWrongKeyType duplicate the same mechanism-loop/assert pattern; extract a small helper (e.g., assertInitReportsWrongKeyType) that takes a session handle, a mechanism type array + count, a key handle and a pointer/enum to indicate whether to call C_SignInit or C_VerifyInit, iterates the mechanisms, builds CK_MECHANISM, calls the appropriate PKCS#11 init function (C_SignInit or C_VerifyInit) and asserts CKR_KEY_TYPE_INCONSISTENT; move the mech arrays (rsaMechs, dsaMechs, ecdsaMechs) to local/shared constants and replace the duplicated loops in testSignInitWrongKeyType and testVerifyInitWrongKeyType with calls to this helper, keeping calls to generateEC and generateRSA and existing key handles (hEcPub/hEcPriv, hRsaPub/hRsaPriv) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/lib/test/SignVerifyTests.cpp`:
- Around line 1290-1439: Both tests testSignInitWrongKeyType and
testVerifyInitWrongKeyType duplicate the same mechanism-loop/assert pattern;
extract a small helper (e.g., assertInitReportsWrongKeyType) that takes a
session handle, a mechanism type array + count, a key handle and a pointer/enum
to indicate whether to call C_SignInit or C_VerifyInit, iterates the mechanisms,
builds CK_MECHANISM, calls the appropriate PKCS#11 init function (C_SignInit or
C_VerifyInit) and asserts CKR_KEY_TYPE_INCONSISTENT; move the mech arrays
(rsaMechs, dsaMechs, ecdsaMechs) to local/shared constants and replace the
duplicated loops in testSignInitWrongKeyType and testVerifyInitWrongKeyType with
calls to this helper, keeping calls to generateEC and generateRSA and existing
key handles (hEcPub/hEcPriv, hRsaPub/hRsaPriv) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 47c75ed5-e53c-4a82-ad36-51d68e185e45
📒 Files selected for processing (3)
src/lib/SoftHSM.cppsrc/lib/test/SignVerifyTests.cppsrc/lib/test/SignVerifyTests.h
bukka
reviewed
Mar 28, 2026
Member
bukka
left a comment
bukka
left a comment
There was a problem hiding this comment.
It looks good. I would just move it to the blocks below to save few comparisons...
Passing a wrong asymmetric key type (e.g. EC key) to a mismatched mechanism (e.g. CKM_RSA_PKCS) caused a crash (segfault or abort) because the code tried to read RSA attributes from an EC key object, then passed invalid data to OpenSSL. Add key type validation after the mechanism switch in both functions, returning CKR_KEY_TYPE_INCONSISTENT on mismatch. Also convert the GOST catch-all else branch to an explicit else-if(isGOST) check. Move MLDSA check also - to be consistent with other checks. Regression tests use cross-matched asymmetric keys (EC key with RSA mechanisms, RSA key with ECDSA/EdDSA mechanisms) to verify the fix. Without the fix, C_SignInit accepts the wrong key type and the subsequent C_Sign crashes.
mingulov
force-pushed
the
fix/sign-init-key-type-crash
branch
from
ce64feb to
d7dd4f5
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/SoftHSM.cpp (1)
4533-4676: Consider centralizing key-type validation mapping to reduce drift risk.
AsymSignInitandAsymVerifyInitnow duplicate the same family checks (RSA/DSA/ECDSA/EDDSA/MLDSA/GOST). A small shared helper for expected key type per family would reduce maintenance risk in future mechanism additions.Also applies to: 5631-5774
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/SoftHSM.cpp` around lines 4533 - 4676, Introduce a single helper that maps an asymmetric family to its expected CK key type (e.g. CKK_RSA, CKK_DSA, CKK_EC, CKK_EC_EDWARDS, CKK_ML_DSA, CKK_GOST) — e.g. CK_ULONG expectedKeyTypeForAsymAlgo(AsymAlgo) — and call it from AsymSignInit and AsymVerifyInit before creating the algorithm instance; replace each inline check like "if (keyType != CKK_RSA)" / "if (keyType != CKK_DSA)" / "if (keyType != CKK_EC)" / "if (keyType != CKK_EC_EDWARDS)" / "if (keyType != CKK_ML_DSA)" / "if (keyType != CKK_GOST)" with a single comparison against expectedKeyTypeForAsymAlgo(...) (or return CKR_KEY_TYPE_INCONSISTENT when mismatched), so the key-type validation logic is centralized and reused in both AsymSignInit and AsymVerifyInit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/lib/SoftHSM.cpp`:
- Around line 4533-4676: Introduce a single helper that maps an asymmetric
family to its expected CK key type (e.g. CKK_RSA, CKK_DSA, CKK_EC,
CKK_EC_EDWARDS, CKK_ML_DSA, CKK_GOST) — e.g. CK_ULONG
expectedKeyTypeForAsymAlgo(AsymAlgo) — and call it from AsymSignInit and
AsymVerifyInit before creating the algorithm instance; replace each inline check
like "if (keyType != CKK_RSA)" / "if (keyType != CKK_DSA)" / "if (keyType !=
CKK_EC)" / "if (keyType != CKK_EC_EDWARDS)" / "if (keyType != CKK_ML_DSA)" / "if
(keyType != CKK_GOST)" with a single comparison against
expectedKeyTypeForAsymAlgo(...) (or return CKR_KEY_TYPE_INCONSISTENT when
mismatched), so the key-type validation logic is centralized and reused in both
AsymSignInit and AsymVerifyInit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0d500499-19c2-48d1-83de-ad889c7b5ea3
📒 Files selected for processing (3)
src/lib/SoftHSM.cppsrc/lib/test/SignVerifyTests.cppsrc/lib/test/SignVerifyTests.h
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/test/SignVerifyTests.cpp
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
2 participants
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.
Passing a wrong asymmetric key type (e.g. EC key) to a mismatched mechanism (for example RSA related) caused a crash (segfault or abort) because the code tried to read RSA attributes from an EC key object and passed invalid data to OpenSSL.
Add key type validation after the mechanism switch in both functions, returning CKR_KEY_TYPE_INCONSISTENT on mismatch. Also convert the GOST catch-all else branch to an explicit else-if(isGOST) check.
Regression tests use cross-matched asymmetric keys (EC key with RSA mechanisms, RSA key with ECDSA/EdDSA mechanisms) to verify the fix. Without the fix, C_SignInit accepts the wrong key type and the subsequent C_Sign crashes.
Summary by CodeRabbit
Bug Fixes
Tests