Update pkcs.h to 3.1.0 version and fix CK_RSA_AES_KEY_WRAP_PARAMS #815
Conversation
This updates pkcs11.h header with the latest version from p11-kit. The CK_RSA_AES_KEY_WRAP_PARAMS were incorrectly added without param defines so this is also fixed and the usage renamed. Fixes softhsm#604
WalkthroughReplaced RSAAES key-wrap parameter fields to use ulAESKeyBits and pOAEPParams throughout SoftHSM.cpp, updating key length computation, OAEP mechanism construction, and validation paths in wrap/unwrap flows. Control flow is unchanged and no exported/public API signatures were modified. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Caller
participant H as SoftHSM.cpp
participant T as Token/Mechanisms
Note over C,H: RSA-AES Key Wrap (Updated param usage)
C->>H: C_WrapKey(mech: CKM_RSA_AES_KEY_WRAP, pOAEPParams, ulAESKeyBits, ...)
H->>H: Validate pOAEPParams, ulAESKeyBits
H->>T: Construct OAEP mech from pOAEPParams
T-->>H: OAEP mech ready
H->>T: Perform RSA+AES-KW (key length from ulAESKeyBits)
T-->>H: Wrapped key
H-->>C: Return wrapped key
Note over C,H: RSA-AES Key Unwrap (Updated param usage)
C->>H: C_UnwrapKey(mech: CKM_RSA_AES_KEY_WRAP, pOAEPParams, ...)
H->>H: Validate pOAEPParams
H->>T: Construct OAEP mech from pOAEPParams
T-->>H: OAEP mech ready
H->>T: Perform RSA+AES-KW unwrap
T-->>H: Unwrapped key
H-->>C: Return new key handle
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (4 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing touches
🧪 Generate unit tests
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lib/SoftHSM.cpp (2)
7054-7066: Missing length guard before splitting wrapped blob can cause OOB/underflow.You compute wrappedLen1 from modulus.size() and then do ulWrappedKeyLen - wrappedLen1 without checking ulWrappedKeyLen >= wrappedLen1. Malformed input can underflow and lead to out-of-bounds reads.
Apply this guard:
@@ -7054,6 +7059,13 @@ CK_RV SoftHSM::UnwrapMechRsaAesKw(...) CK_ULONG ulWrappedKeyLen = wrapped.size(); CK_ULONG wrappedLen1 = modulus.size(); - CK_ULONG wrappedLen2 = ulWrappedKeyLen - wrappedLen1; + if (ulWrappedKeyLen < wrappedLen1) + { + ERROR_MSG("Wrapped key blob shorter than RSA ciphertext."); + return CKR_WRAPPED_KEY_LEN_RANGE; + } + CK_ULONG wrappedLen2 = ulWrappedKeyLen - wrappedLen1;
13162-13194: OAEP param validation is incomplete/inconsistent with the rest of the module.
- You allow mgf in [1..5] but don’t validate hashAlg at all.
- Elsewhere (e.g., MechParamCheckRSAPKCSOAEP) SoftHSM restricts OAEP to SHA-1/MGF1-SHA1.
- As WrapKeyAsym/UnwrapKeyAsym ignore CK_MECHANISM::pParameter, accepting non-SHA1 here misrepresents supported features.
Lock this down to SHA-1/MGF1-SHA1 now, or wire the params through.
Use the diff proposed above in the 6543-6546 comment to align checks. Optionally, delegate to the existing checker:
@@ -13161,6 +13195,16 @@ CK_RV SoftHSM::MechParamCheckRSAAESKEYWRAP(CK_MECHANISM_PTR pMechanism) - if (params->pOAEPParams == NULL_PTR) + if (params->pOAEPParams == NULL_PTR) ... + /* Reuse OAEP validator for consistent checks */ + CK_MECHANISM oaep = { CKM_RSA_PKCS_OAEP, params->pOAEPParams, sizeof(CK_RSA_PKCS_OAEP_PARAMS) }; + CK_RV rv = MechParamCheckRSAPKCSOAEP(&oaep); + if (rv != CKR_OK) return rv;
🧹 Nitpick comments (1)
src/lib/SoftHSM.cpp (1)
6481-6495: Optional: rename “emph” to “ephem” for clarity.Pure readability nit; no functional impact.
Also applies to: 6543-6546, 7054-7066, 13162-13194
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/lib/SoftHSM.cpp(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: antoinelochet
PR: softhsm/SoftHSMv2#809
File: src/lib/SoftHSM.cpp:4193-4196
Timestamp: 2025-08-19T09:23:25.434Z
Learning: MLDSA parameter set inference: In src/lib/crypto/MLDSAPrivateKey.{h,cpp} and MLDSAPublicKey.{h,cpp}, getParameterSet() maps key material length to CKP_ML_DSA_44/65/87, and there is no setParameterSet(). Runtime MLDSA keys used by SoftHSM (e.g., in C_SignInit/C_VerifyInit via MLDSAUtil) rely on this inference; no explicit parameter-set assignment is required.
📚 Learning: 2025-08-18T19:46:46.659Z
Learnt from: antoinelochet
PR: softhsm/SoftHSMv2#809
File: src/lib/SoftHSM.cpp:10044-10055
Timestamp: 2025-08-18T19:46:46.659Z
Learning: In SoftHSM::generateMLDSA (src/lib/SoftHSM.cpp), the project prefers using CKP_ML_DSA_* constants for parameter set checks and logs "Unsupported parameter set: %lu". The code currently returns CKR_PARAMETER_SET_NOT_SUPPORTED for unsupported sets.
Applied to files:
src/lib/SoftHSM.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Windows (x86, openssl)
- GitHub Check: Linux with OpenSSL 3.0
- GitHub Check: Windows (x64, botan)
- GitHub Check: Linux with OpenSSL 1.1.1
- GitHub Check: Windows (x64, openssl)
- GitHub Check: macOS (openssl)
- GitHub Check: macOS (botan)
- GitHub Check: Linux with Botan
🔇 Additional comments (1)
src/lib/SoftHSM.cpp (1)
6481-6495: Correct field migration to ulAESKeyBits (LGTM).Deriving emphKeyLen from params->ulAESKeyBits keeps the implementation aligned with the new PKCS #11 3.1.0 header. No functional concern here.
| CK_MECHANISM oaepMech = {CKM_RSA_PKCS_OAEP, params->pOAEPParams, sizeof(CK_RSA_PKCS_OAEP_PARAMS)}; | ||
| // Wraps the AES emph key with the wrapping RSA key using CKM_RSA_PKCS_OAEP with parameters of OAEPParams. | ||
| rv = SoftHSM::WrapKeyAsym(&oaepMech, token, wrapKey, emphkeydata, wrapped_1); | ||
|
|
There was a problem hiding this comment.
OAEP params are passed, but not actually used by WrapKeyAsym/UnwrapKeyAsym.
WrapKeyAsym/UnwrapKeyAsym ignore CK_MECHANISM::pParameter and always perform OAEP with SHA-1 (see the hard-coded size check and lack of param pluming). With the new API you accept non-SHA1 MGF values, which can mislead callers and cause interop failures when they expect OAEP(SHA-256, …). Either (a) restrict OAEPParams to SHA-1/MGF1-SHA1 here, or (b) plumb OAEP params down to the crypto backend and enforce consistency checks.
Apply this minimal guard now (option a):
--- a/src/lib/SoftHSM.cpp
+++ b/src/lib/SoftHSM.cpp
@@ -13162,6 +13192,24 @@ CK_RV SoftHSM::MechParamCheckRSAAESKEYWRAP(CK_MECHANISM_PTR pMechanism)
- if (params->pOAEPParams->mgf < 1UL || params->pOAEPParams->mgf > 5UL)
- {
- ERROR_MSG("mgf not supported");
- return CKR_ARGUMENTS_BAD;
- }
+ /* SoftHSM currently supports OAEP with SHA-1 only. Keep checks consistent
+ * with MechParamCheckRSAPKCSOAEP until OAEP params are fully plumbed. */
+ if (params->pOAEPParams->hashAlg != CKM_SHA_1)
+ {
+ ERROR_MSG("hashAlg must be CKM_SHA_1");
+ return CKR_ARGUMENTS_BAD;
+ }
+ if (params->pOAEPParams->mgf != CKG_MGF1_SHA1)
+ {
+ ERROR_MSG("mgf must be CKG_MGF1_SHA1");
+ return CKR_ARGUMENTS_BAD;
+ }Option (b) is preferable long-term: extend WrapKeyAsym/UnwrapKeyAsym and the RSA backend to accept OAEP params and validate hashAlg/mgf consistency.
📝 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.
| CK_MECHANISM oaepMech = {CKM_RSA_PKCS_OAEP, params->pOAEPParams, sizeof(CK_RSA_PKCS_OAEP_PARAMS)}; | |
| // Wraps the AES emph key with the wrapping RSA key using CKM_RSA_PKCS_OAEP with parameters of OAEPParams. | |
| rv = SoftHSM::WrapKeyAsym(&oaepMech, token, wrapKey, emphkeydata, wrapped_1); | |
| CK_RV SoftHSM::MechParamCheckRSAAESKEYWRAP(CK_MECHANISM_PTR pMechanism) | |
| { | |
| CK_RSA_AES_KEY_WRAP_PARAMS_PTR params = static_cast<CK_RSA_AES_KEY_WRAP_PARAMS_PTR>(pMechanism->pParameter); | |
| /* SoftHSM currently supports OAEP with SHA-1 only. Keep checks consistent | |
| * with MechParamCheckRSAPKCSOAEP until OAEP params are fully plumbed. */ | |
| if (params->pOAEPParams->hashAlg != CKM_SHA_1) | |
| { | |
| ERROR_MSG("hashAlg must be CKM_SHA_1"); | |
| return CKR_ARGUMENTS_BAD; | |
| } | |
| if (params->pOAEPParams->mgf != CKG_MGF1_SHA1) | |
| { | |
| ERROR_MSG("mgf must be CKG_MGF1_SHA1"); | |
| return CKR_ARGUMENTS_BAD; | |
| } | |
| // ...remaining checks ... | |
| } |
🤖 Prompt for AI Agents
In src/lib/SoftHSM.cpp around lines 6543-6546, the code passes OAEP params to
WrapKeyAsym but that function ignores pParameter and always uses SHA-1; to apply
the minimal guard, validate params->pOAEPParams before calling WrapKeyAsym: if
pOAEPParams is non-null, cast to CK_RSA_PKCS_OAEP_PARAMS and verify hashAlg ==
CKM_SHA_1 and mgf == CKG_MGF1_SHA1 (and pSource == CKZ_DATA or other allowed
source if applicable); if the check fails, return CKR_MECHANISM_PARAM_INVALID
(or an appropriate CKR) and avoid calling WrapKeyAsym; apply the same validation
for UnwrapKeyAsym call sites to ensure callers cannot request non-SHA1 OAEP
until full plumbing is implemented.
💡 Verification agent
🧩 Analysis chain
Confirm backend support before relaxing OAEP constraints.
If you plan to support OAEP with SHA-2, verify the RSA backend (OSSLCryptoFactory/BotanCryptoFactory) and AsymmetricAlgorithm::wrapKey/unwrapKey accept OAEP params and set the hash/MGF accordingly.
Run:
Also applies to: 7060-7063
🏁 Script executed:
#!/bin/bash
# Inspect whether OAEP params are used anywhere in RSA wrap/unwrap paths.
rg -nP -C3 'wrapKey\s*\(\s*publicKey\s*,\s*\w+\s*,\s*\w+\s*,\s*AsymMech::RSA_PKCS_OAEP' src
rg -nP -C3 'unwrapKey\s*\(\s*unwrappingkey\s*,\s*\w+\s*,\s*\w+\s*,\s*AsymMech::RSA_PKCS_OAEP' src
rg -nP -C3 'CK_RSA_PKCS_OAEP_PARAMS|hashAlg|mgf' srcLength of output: 41122
Do not relax OAEP constraints until backend OAEP (SHA‑2/MGF1) support is confirmed
SoftHSM currently forces CKM_SHA_1 / CKG_MGF1_SHA1 in OAEP paths (src/lib/SoftHSM.cpp:3207–3216 and 13117–13127) while Wrap/Unwrap pass params->pOAEPParams (src/lib/SoftHSM.cpp:6543, 7059). Verify that AsymmetricAlgorithm::wrapKey/unwrapKey and the RSA backends map CK_RSA_PKCS_OAEP_PARAMS → backend hash/MGF options (check src/lib/crypto/AsymmetricAlgorithm.{h,cpp}, src/lib/crypto/OSSLRSA.cpp, src/lib/crypto/BotanRSA.cpp). If backends do not accept OAEP params or SHA‑2 MGF1, either implement that mapping in the backends or keep the OAEP restriction to SHA‑1 and return CKR_ARGUMENTS_BAD.
🤖 Prompt for AI Agents
In src/lib/SoftHSM.cpp around lines 6543–6546, the code passes
params->pOAEPParams to WrapKeyAsym but SoftHSM currently forces OAEP to
SHA‑1/MGF1 elsewhere; do not relax this constraint until the RSA backends
actually support OAEP params and SHA‑2/MGF1. Verify
AsymmetricAlgorithm::wrapKey/unwrapKey and the RSA backend mappings in
src/lib/crypto/AsymmetricAlgorithm.{h,cpp}, src/lib/crypto/OSSLRSA.cpp, and
src/lib/crypto/BotanRSA.cpp: if those backends already map
CK_RSA_PKCS_OAEP_PARAMS to the correct hash/MGF options, ensure Wrap/Unwrap use
that mapping; if they do not, either implement the mapping in the backends to
accept pOAEPParams (supporting SHA‑2 and MGF1 variants) or enforce the existing
restriction by rejecting non‑SHA1 OAEP parameters and returning
CKR_ARGUMENTS_BAD.
There was a problem hiding this comment.
This PR just changes the parameter name so this seems a bit out of scope but possibly a good suggestion for the future.
There was a problem hiding this comment.
@bukka Agreed—let’s keep this PR focused on the header update/param rename. I’ll defer the OAEP behavior checks.
Would you like me to open a follow‑up issue to track: “Plumb CK_RSA_PKCS_OAEP params (hash/MGF) through RSA wrap/unwrap or restrict to SHA‑1 until supported”? Draft:
- Problem: WrapKeyAsym/UnwrapKeyAsym ignore CK_RSA_PKCS_OAEP_PARAMS; RSAAES check now allows non‑SHA1 MGF, which can mislead callers.
- Short‑term options: (a) reject non‑SHA1 OAEP in RSAAES guard; (b) wire OAEP params to the RSA backend and add tests.
- Deliverables: decide approach, implement, and add interop tests for OAEP(SHA‑1/256/384/512) if (b).
If you confirm, I’ll open it and link back to this PR/comment.
✏️ Learnings added
Learnt from: bukka
PR: softhsm/SoftHSMv2#815
File: src/lib/SoftHSM.cpp:6543-6546
Timestamp: 2025-09-11T16:54:00.347Z
Learning: SoftHSM PR #815 (pkcs11.h 3.1.0 update, CK_RSA_AES_KEY_WRAP_PARAMS fix): Maintainer bukka prefers to keep scope minimal (header sync and param rename only); broader OAEP parameter enforcement or backend plumbing is out of scope for this PR and should be handled in a follow-up issue.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: antoinelochet
PR: softhsm/SoftHSMv2#809
File: src/lib/SoftHSM.cpp:10044-10055
Timestamp: 2025-08-18T19:46:46.659Z
Learning: In SoftHSM::generateMLDSA (src/lib/SoftHSM.cpp), the project prefers using CKP_ML_DSA_* constants for parameter set checks and logs "Unsupported parameter set: %lu". The code currently returns CKR_PARAMETER_SET_NOT_SUPPORTED for unsupported sets.
|
This needs some fixing on Windows. Will take a proper look and test it on Windows when I'm back on this. |
|
Closing in favour of #827 |
1 participant
This updates pkcs11.h header with the latest version from p11-kit.
The CK_RSA_AES_KEY_WRAP_PARAMS were incorrectly added without param defines so this is also fixed and the usage renamed.
Fixes #604
Summary by CodeRabbit
Bug Fixes
Refactor