Add support for DES3 wrap/unwrap #812
Conversation
softhsm#405 (cherry picked from commit 40856c7)
Enabled the use of CKA_MODIFIABLE and CKA_VALUE_LEN in the unwrap template. Based on PR softhsm#657 softhsm#657
WalkthroughExtends attribute modifiability to permit updates during UNWRAP (treating UNWRAP like CREATE/GENERATE); adds per-mechanism CBC/ECB selection and IV handling for wrap/unwrap (including CKM_DES3_ECB), CBC_PAD unpadding, CKA_VALUE_LEN enforcement on unwrap; and accepts parity-inclusive DES/3DES key lengths in Botan and OpenSSL backends. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant HSM as SoftHSM
participant Mech as Mechanism Resolver
participant Cipher as Symmetric Cipher
participant Pad as RFC5652 Unpad
rect rgba(230,245,255,0.6)
App->>HSM: C_WrapKey(mech, tmpl, key)
HSM->>Mech: Resolve SymMode (CBC/ECB) and IV rules
Mech-->>HSM: SymMode + IV validation
HSM->>Cipher: encryptInit(mode, IV?)
HSM->>Cipher: Encrypt(keydata)
Cipher-->>HSM: wrappedData
HSM-->>App: CKR_OK + wrappedData
end
rect rgba(240,255,230,0.6)
App->>HSM: C_UnwrapKey(mech, tmpl{... CKA_VALUE_LEN?}, wrapped)
HSM->>Mech: Resolve SymMode (CBC/ECB) and IV rules
Mech-->>HSM: SymMode + IV validation
HSM->>Cipher: decryptInit(mode, IV?)
HSM->>Cipher: Decrypt(wrapped)
Cipher-->>HSM: plaintext
alt CBC_PAD
HSM->>Pad: Unpad(plaintext)
Pad-->>HSM: keydata or error
HSM-->>App: CKR_WRAPPED_KEY_INVALID on unpad failure
end
HSM->>HSM: Enforce CKA_VALUE_LEN if provided (secret keys)
HSM-->>App: CKR_OK + new key handle or CKR_TEMPLATE_INCONSISTENT
end
sequenceDiagram
autonumber
actor App
participant Attr as P11Attribute::update
App->>Attr: Attempt attribute update during UNWRAP
alt Operation ∈ {CREATE, GENERATE, UNWRAP}
Attr-->>App: Allow update despite CKA_MODIFIABLE=false
else Other operations
Attr-->>App: CKR_ATTRIBUTE_READ_ONLY
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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/crypto/OSSLDES.cpp (1)
56-66: Add a FIPS‐only DES key-length test for getCipher()I wasn’t able to find any existing unit tests that exercise the
#ifdef WITH_FIPSguard inOSSLDES.cpp, so we need to add one to prevent regressions:• No tests covering 56- or 64-bit DES key rejection under FIPS were found (a search for “WITH_FIPS” in any test files returned nothing).
• Please add a new test case in the crypto test suite (e.g.tests/crypto/OSSLDESTest.cpp):
– DefineWITH_FIPSat compile time for the test
– CallOSSLDES::getCipher()(or the public wrapper) with a 56-bit key and assert it fails
– Repeat for a 64-bit (8-byte) key length
• Verify that in non-FIPS builds those same key lengths still succeed.This will ensure single‐DES key lengths are correctly rejected only when
WITH_FIPSis enabled.src/lib/SoftHSM.cpp (1)
6327-6341: DES/3DES bit-length adjustment missing in wrap/unwrap pathsElsewhere you use 7 bits/byte for DES-family keys (parity ignored). Wrap/unwrap still multiplies by 8, which is inconsistent and may trip provider-side validation.
// after the switch that sets `algo` + if (algo == SymAlgo::DES || algo == SymAlgo::DES3) { + bb = 7; + } SymmetricAlgorithm* cipher = CryptoFactory::i()->getSymmetricAlgorithm(algo);Apply the same in UnwrapKeySym before
unwrappingkey->setBitLen(...):// after the switch that sets `algo` + if (algo == SymAlgo::DES || algo == SymAlgo::DES3) { + bb = 7; + } SymmetricAlgorithm* cipher = CryptoFactory::i()->getSymmetricAlgorithm(algo);Also applies to: 6930-6946
🧹 Nitpick comments (8)
src/lib/P11Attributes.cpp (1)
413-417: Allowing attribute updates during C_UnwrapKey when CKA_MODIFIABLE=false fixes self-lock and aligns with PKCS#11 object creation semanticsGood change. During unwrap, the object is being created; evaluating CKA_MODIFIABLE before all template attributes are applied could prematurely lock further updates in the same operation. Exempting OBJECT_OP_UNWRAP from the read-only gate matches the existing exemptions for CREATE/GENERATE.
Two small follow-ups:
- Add a short code comment explaining why UNWRAP is exempted (future maintainers will thank you).
- Grammar nit in the error string.
Apply this diff to document intent and fix the message:
- // Attributes cannot be changed if CKA_MODIFIABLE is set to false - if (!isModifiable() && op != OBJECT_OP_GENERATE && op != OBJECT_OP_CREATE && op != OBJECT_OP_UNWRAP) { - ERROR_MSG("An object is with CKA_MODIFIABLE set to false is not modifiable"); + // Attributes cannot be changed if CKA_MODIFIABLE is false. + // Note: During CREATE/GENERATE/UNWRAP the object is in creation; allow setting attributes in the same op. + if (!isModifiable() && op != OBJECT_OP_GENERATE && op != OBJECT_OP_CREATE && op != OBJECT_OP_UNWRAP) { + ERROR_MSG("An object with CKA_MODIFIABLE set to false is not modifiable"); return CKR_ATTRIBUTE_READ_ONLY; }src/lib/crypto/OSSLDES.cpp (1)
56-76: Accept both “effective” and “parity-included” DES/3DES key lengths; improve warning textAllowing 56/64, 112/128, and 168/192 bit lengths tolerates both representations (effective vs. with parity). That’s a pragmatic interoperability improvement. The warning currently says “56-bit” even when the bit length is 64 (single-DES with parity). Suggest rewording for clarity.
Apply this diff to clarify the comment and warning:
- // Check currentKey bit length; 3DES only supports 56-bit, 112-bit or 168-bit keys + // Accept both effective (without parity) and parity-included lengths: + // DES: 56 or 64 bits (64 includes parity) + // 2-key 3DES: 112 or 128 bits (128 includes parity) + // 3-key 3DES: 168 or 192 bits (192 includes parity) @@ - // People shouldn't really be using 56-bit DES keys, generate a warning - if (currentKey->getBitLen() == 56 || currentKey->getBitLen() == 64) + // Single-DES (effective 56-bit) is weak; warn irrespective of representation + if (currentKey->getBitLen() == 56 || currentKey->getBitLen() == 64) { - DEBUG_MSG("CAUTION: use of 56-bit DES keys is not recommended!"); + DEBUG_MSG("CAUTION: use of single-DES keys (effective 56-bit) is not recommended!"); }src/lib/crypto/BotanDES.cpp (1)
60-79: Parity-inclusive key lengths accepted; align the warning message with OSSLAccepting 64/128/192 in addition to 56/112/168 improves interoperability. Please match the improved single-DES warning text for consistency with the OpenSSL backend.
Apply this diff:
- case 56: - case 64: - // People shouldn't really be using 56-bit DES keys, generate a warning - DEBUG_MSG("CAUTION: use of 56-bit DES keys is not recommended!"); + case 56: + case 64: + // Single-DES (effective 56-bit) is weak; warn irrespective of representation + DEBUG_MSG("CAUTION: use of single-DES keys (effective 56-bit) is not recommended!"); algo = "DES"; break;src/lib/SoftHSM.cpp (5)
7472-7477: CKA_VALUE_LEN check should apply only to secret keys to avoid false rejectionsAsymmetric keys (PKCS#8 blobs) have no meaningful CKA_VALUE_LEN; current code would fail if callers add it by mistake.
- // If the caller supplied CKA_VALUE_LEN, ensure it matches the unwrapped key length - if (haveRequestedValueLen && keydata.size() != requestedValueLen) + // If provided, enforce CKA_VALUE_LEN only for secret keys + if (objClass == CKO_SECRET_KEY && haveRequestedValueLen && keydata.size() != requestedValueLen) { return CKR_TEMPLATE_INCONSISTENT; }Optionally, consider returning CKR_WRAPPED_KEY_LEN_RANGE instead for a stricter PKCS #11 mapping when the length of the unwrapped secret doesn't match the requested length.
6318-6322: ECB IV semantics: clarify and document blocksize=0Setting blocksize=0 for ECB is fine, but it indirectly relies on downstream code not touching
&iv[0]. After guardingmemcpyas proposed above, add a short comment to document why blocksize is 0 for ECB to prevent future regressions.- case CKM_DES3_ECB: - blocksize = 0; + case CKM_DES3_ECB: + // ECB mode has no IV; keep blocksize=0 so iv remains empty + blocksize = 0;
6976-6983: Return a wrap-specific error on unpadding failure (not a generic one)Returning CKR_WRAPPED_KEY_INVALID improves diagnosability and conforms better to caller expectations on unwrap failures.
See the earlier diff in this review that switches to CKR_WRAPPED_KEY_INVALID.
6629-6634: Add tests for new DES3 wrap/unwrap modes and edge cases
- CKM_DES3_ECB: parameter length 0 (both NULL and non-NULL pointer), invalid non-zero length.
- CKM_DES3_CBC/CKM_DES3_CBC_PAD: missing IV, wrong IV length.
- Round-trip wrap/unwrap parity with 2-key (DES2) vs 3-key (DES3).
- Bad padding on CBC_PAD unwrap returns CKR_WRAPPED_KEY_INVALID.
I can draft unit/integration tests for these cases against the SoftHSM PKCS #11 module. Want me to open a follow-up PR?
Also applies to: 7269-7278
7317-7324: Optional: tighten mechanism/type checks to reduce branchesYou can avoid repeated mechanism membership checks by precomputing booleans (isAESWrap, isDES3CBC, isDES3ECB) and using them in both the top-level switch and the type checks. This reduces drift between validation and execution.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
src/lib/P11Attributes.cpp(1 hunks)src/lib/SoftHSM.cpp(13 hunks)src/lib/crypto/BotanDES.cpp(1 hunks)src/lib/crypto/OSSLDES.cpp(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/lib/P11Attributes.cpp (2)
src/lib/P11Objects.cpp (2)
isModifiable(322-329)isModifiable(322-322)src/lib/object_store/DBObject.cpp (2)
isModifiable(365-394)isModifiable(365-365)
src/lib/SoftHSM.cpp (4)
src/lib/crypto/BotanDES.cpp (4)
wrapKey(38-43)wrapKey(38-38)unwrapKey(45-50)unwrapKey(45-45)src/lib/crypto/OSSLDES.cpp (4)
wrapKey(38-43)wrapKey(38-38)unwrapKey(45-50)unwrapKey(45-45)src/lib/crypto/BotanAES.cpp (4)
wrapKey(45-120)wrapKey(45-45)unwrapKey(123-211)unwrapKey(123-123)src/lib/crypto/OSSLAES.cpp (8)
wrapKey(41-48)wrapKey(41-41)wrapKey(50-53)wrapKey(50-50)unwrapKey(57-65)unwrapKey(57-57)unwrapKey(67-70)unwrapKey(67-67)
🔇 Additional comments (8)
src/lib/P11Attributes.cpp (1)
969-985: No action needed: unwrap correctly validates and sets CKA_VALUE_LEN on create
- In SoftHSM::C_UnwrapKey (src/lib/SoftHSM.cpp around lines 7374–7478), the code captures an optional CKA_VALUE_LEN from the unwrap template and, after decryption, only checks
without ever calling updateAttr on CKA_VALUE_LEN during unwrap.if (haveRequestedValueLen && keydata.size() != requestedValueLen) { return CKR_TEMPLATE_INCONSISTENT; }- In P11Attributes.cpp (src/lib/P11Attributes.cpp lines 969–985), OBJECT_OP_UNWRAP is treated like a create operation and uses osobject->setAttribute(…) to initialize CKA_VALUE_LEN on the newly created object, rather than invoking updateAttr.
Both checks confirm that the unwrap flow enforces a mismatched CKA_VALUE_LEN as a rejection and only sets the attribute on object creation, respecting the READ_ONLY design.
src/lib/crypto/OSSLDES.cpp (3)
79-93: CBC: mappings for 64/128/192 mirror 56/112/168 — LGTMMode-specific cipher selection looks correct for CBC. No functional concerns.
94-108: ECB: ensure CI includes CKM_DES3_ECB unwrap testsThe code correctly maps 64/128/192-bit keys to the appropriate
EVP_des_*_ecb()calls, and the PR introduces theCKM_DES3_ECBunwrap functionality. However, I was unable to find any existing tests (e.g. notestortestsdirectory) covering ECB unwrap for 2-key (112/128 bit) or 3-key (168/192 bit) 3DES. Please verify that your CI pipeline includes end-to-end tests for both cases:
- A 2-key (112/128 bit) 3DES object unwrapped with
CKM_DES3_ECBin ECB mode- A 3-key (168/192 bit) 3DES object unwrapped with
CKM_DES3_ECBin ECB modeIf these tests don’t already exist in your test suite, please add them to ensure full coverage of the new unwrap functionality.
109-138: It looks like the sandbox environment didn’t return any matches forEVP_des_ofborEVP_des_cfb, which usually means the OpenSSL headers aren’t present (or live in a non-standard include path). Please manually verify in your target build environment that those aliases exist. For example:# 1) Locate your OpenSSL include directory: dir=$(openssl version -d | awk -F'"' '{print $2}')/include/openssl echo "Searching in $dir" grep -R "EVP_des_ofb" "$dir" || true grep -R "EVP_des_cfb" "$dir" || true # 2) Or via pkg-config: inc=$(pkg-config --cflags openssl | sed 's/-I//g') echo "Searching in $inc/openssl" grep -R "EVP_des_ofb" "$inc/openssl" || true grep -R "EVP_des_cfb" "$inc/openssl" || trueIf you find that some distributions only provide
EVP_des_ofb64/EVP_des_cfb64, you may need to add configure-time detection (e.g. with functional tests likeEVP_PKEY_CTX_new_from_name) and alias appropriately. Let me know what you find!src/lib/crypto/BotanDES.cpp (1)
52-118: Confirmed test coverage for both two-key (112/128 bit) and three-key (168/192 bit) TripleDES scenarios– In src/lib/crypto/test/DESTests.cpp, the suite instantiates DESKey with 56, 112 and 168 effective-bit lengths under CBC/ECB/CFB/OFB modes, exercising both 112-bit and 168-bit TripleDES cases.
– In the PKCS#11 integration tests (SymmetricAlgorithmTests.cpp), importKey is called with CKK_DES2 (16 bytes ⇒ 128 bits) and CKK_DES3 (24 bytes ⇒ 192 bits), followed by encrypt/decrypt via CKM_DES3_CBC_PAD and CKM_DES3_CBC, covering the parity-inclusive 128 / 192 bit lengths.Algorithm naming “DES” and “TripleDES” with mode and PKCS7/NoPadding remains stable across Botan versions; no changes needed.
src/lib/SoftHSM.cpp (3)
7317-7320: Mechanism/type check includes CKM_AES_CBC but mechanism switch does notYou validate unwrapKey type for CKM_AES_CBC here, but the top-level switch does not accept CKM_AES_CBC (only AES_CBC_PAD). Either add CKM_AES_CBC to the switch with parameter length checks or drop it from this type check for consistency.
Would you like me to add CKM_AES_CBC to C_UnwrapKey with
pParameterlength 16, mirroring AES_CBC in the wrap path?
6272-6326: Nice addition: per-mechanism symmetric mode selection for DES3, incl. ECBThe
sym_modeintroduction and DES3 switch cases look good and align with CBC/PAD vs ECB semantics. Once the small issues noted are fixed, this should be robust.
7263-7266: AES_CBC_PAD parameter validation added — LGTMThe explicit check for a 16-byte IV for CKM_AES_CBC_PAD is correct and matches the AES block size.
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)
1130-1149: MechanismInfo flags don’t reflect newly added DES3 wrap/unwrap support.C_GetMechanismInfo currently advertises:
- DES3_CBC_PAD: CKF_WRAP | CKF_UNWRAP (OK)
- DES3_CBC: CKF_WRAP only (but C_UnwrapKey supports unwrapping with DES3_CBC)
- DES3_ECB: only CKF_ENCRYPT | CKF_DECRYPT (but wrap/unwrap are implemented)
This will confuse clients that query capabilities before use. Advertise CKF_WRAP | CKF_UNWRAP for DES3_CBC and DES3_ECB as implemented.
Apply:
- case CKM_DES3_CBC: - pInfo->flags |= CKF_WRAP; + case CKM_DES3_CBC: + pInfo->flags |= CKF_WRAP | CKF_UNWRAP; /* FALLTHROUGH */ case CKM_DES3_ECB: + pInfo->flags |= CKF_WRAP | CKF_UNWRAP; // Key size is not in use pInfo->ulMinKeySize = 0; pInfo->ulMaxKeySize = 0; pInfo->flags |= CKF_ENCRYPT | CKF_DECRYPT; break;
♻️ Duplicate comments (2)
src/lib/SoftHSM.cpp (2)
6352-6364: ECB IV handling guard and consistent mode usage look good.The memcpy guard for blocksize > 0 avoids taking &iv[0] on an empty buffer, and encryptInit now uses the selected sym_mode consistently. This addresses the earlier IV/ECB hazards.
6901-6937: Unwrap: DES3 mode selection and ECB IV handling guard look correct.Using sym_mode for DES3_* and copying the IV only when blocksize > 0 prevents the earlier ECB/IV misuse. Nice.
🧹 Nitpick comments (1)
src/lib/SoftHSM.cpp (1)
6986-7000: Return a key-specific error on padding/decrypt failures; fix comment typo.For CBC_PAD unwraps, decryptFinal/padding failures should map to CKR_WRAPPED_KEY_INVALID (callers rely on this), not CKR_GENERAL_ERROR. Also, the comment says “Finalize encryption” in the decrypt path.
Apply:
- // Finalize encryption - if (!cipher->decryptFinal(decryptedFinal)) { + // Finalize decryption + if (!cipher->decryptFinal(decryptedFinal)) { cipher->recycleKey(unwrappingkey); CryptoFactory::i()->recycleSymmetricAlgorithm(cipher); - return CKR_GENERAL_ERROR; + return (pMechanism->mechanism == CKM_AES_CBC_PAD || pMechanism->mechanism == CKM_DES3_CBC_PAD) + ? CKR_WRAPPED_KEY_INVALID + : CKR_GENERAL_ERROR; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
src/lib/P11Attributes.cpp(1 hunks)src/lib/SoftHSM.cpp(13 hunks)src/lib/crypto/BotanDES.cpp(1 hunks)src/lib/crypto/OSSLDES.cpp(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib/P11Attributes.cpp
- src/lib/crypto/OSSLDES.cpp
- src/lib/crypto/BotanDES.cpp
🧰 Additional context used
🧬 Code graph analysis (1)
src/lib/SoftHSM.cpp (2)
src/lib/crypto/BotanDES.cpp (4)
wrapKey(38-43)wrapKey(38-38)unwrapKey(45-50)unwrapKey(45-45)src/lib/crypto/OSSLDES.cpp (4)
wrapKey(38-43)wrapKey(38-38)unwrapKey(45-50)unwrapKey(45-45)
🔇 Additional comments (5)
src/lib/SoftHSM.cpp (5)
6636-6644: Good validation: DES3 ECB/CBC(CK_PARAM) checks added in C_WrapKey.
- CKM_DES3_ECB: ulParameterLen must be 0 (no IV).
- CKM_DES3_CBC / CKM_DES3_CBC_PAD: IV must be present and exactly 8 bytes.
This matches PKCS #11 semantics and prevents NULL IV dereferences downstream.
6685-6689: Key type consistency checks for DES2/DES3 wrapping are correct.Allowing CKK_DES2 and CKK_DES3 (and rejecting others) for DES3* mechanisms is the right gate.
7280-7300: Good parity checks in C_UnwrapKey for AES/DES3 parameters.
- AES-CBC-PAD requires 16-byte IV.
- DES3-CBC / DES3-CBC-PAD require 8-byte IV.
- DES3-ECB requires ulParameterLen == 0.
These align with the wrap-side checks and PKCS #11 expectations.
7340-7346: Unwrap key type gating for DES3 mechanisms is correct.*Rejecting unwrapping keys that aren’t CKK_DES2/CKK_DES3 prevents invalid key/mech combinations.
7397-7419: CKA_VALUE_LEN support on unwrap is implemented cleanly.
- You accept CKA_VALUE_LEN in the unwrap template (secret keys only), validate its type, and enforce it after decrypting.
- Returning CKR_TEMPLATE_INCONSISTENT on mismatch is appropriate.
Also applies to: 7494-7499
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)
12116-12128: Bug: CKA_NEVER_EXTRACTABLE is written to CKA_ALWAYS_SENSITIVEIn the CKM_CONCATENATE_BASE_AND_KEY derive path, the last assignment stores bNeverExtractable into CKA_ALWAYS_SENSITIVE instead of CKA_NEVER_EXTRACTABLE.
Apply:
- bool bNeverExtractable = baseKey->getBooleanValue(CKA_NEVER_EXTRACTABLE, false) && - otherKey->getBooleanValue(CKA_NEVER_EXTRACTABLE, false); - bOK = bOK && osobject->setAttribute(CKA_ALWAYS_SENSITIVE, bNeverExtractable); + bool bNeverExtractable = baseKey->getBooleanValue(CKA_NEVER_EXTRACTABLE, false) && + otherKey->getBooleanValue(CKA_NEVER_EXTRACTABLE, false); + bOK = bOK && osobject->setAttribute(CKA_NEVER_EXTRACTABLE, bNeverExtractable);This is a correctness issue with security semantics (derive of a non‑extractable input must remain non‑extractable).
♻️ Duplicate comments (2)
src/lib/SoftHSM.cpp (2)
6351-6364: Guarded IV memcpy for ECB (no IV) and consistent mode usage: addressedThe undefined behavior on iv[0] for blocksize==0 is fixed, and encryptInit now receives the computed sym_mode. This resolves the previously reported issue.
6293-6296: AES-CBC wrap IV handling fixed (blocksize=16 and correct mode): addressedSetting blocksize=16 and using sym_mode=CBC ensures the IV is copied and used. This unblocks AES-CBC wrapping which previously initialized CBC with an empty IV.
Also applies to: 6301-6303
🧹 Nitpick comments (3)
src/lib/SoftHSM.cpp (3)
7282-7286: Outdated TODO left behindThe 16‑byte IV length check for CKM_AES_CBC_PAD is now implemented; the “TODO check block length” comment is stale.
Apply:
- case CKM_AES_CBC_PAD: - // TODO check block length + case CKM_AES_CBC_PAD:
6274-6284: Nit: wrappedlen and magic numbers
- wrappedlen is computed in a few branches but not subsequently used; consider removing it where it’s redundant to reduce cognitive load.
- Consider introducing named constants for block sizes (e.g., kBlockAES=16, kBlockDES=8) for readability.
Also applies to: 6298-6312, 6320-6326
6964-7004: Optional: unify unwrap error semantics for symmetric modesToday, decrypt failures map to WRAPPED_KEY_INVALID only for *_CBC_PAD and to GENERAL_ERROR for CBC/ECB. Many PKCS#11 clients expect CKR_WRAPPED_KEY_INVALID for any malformed wrapped input. Consider returning CKR_WRAPPED_KEY_INVALID for all symmetric unwrap failures for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/lib/SoftHSM.cpp(14 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/lib/SoftHSM.cpp (2)
src/lib/crypto/BotanDES.cpp (4)
wrapKey(38-43)wrapKey(38-38)unwrapKey(45-50)unwrapKey(45-45)src/lib/crypto/OSSLDES.cpp (4)
wrapKey(38-43)wrapKey(38-38)unwrapKey(45-50)unwrapKey(45-45)
🔇 Additional comments (7)
src/lib/SoftHSM.cpp (7)
1140-1147: Mechanism flags correctly advertise 3DES wrap/unwrap supportAdding CKF_WRAP | CKF_UNWRAP for CKM_DES3_{CBC,ECB} is aligned with the new wrap/unwrap paths. Please ensure the module’s mechanism tests cover these flags to avoid regressions in consumers that feature‑detect via C_GetMechanismInfo.
Would you like a quick test snippet exercising C_GetMechanismInfo for these mechs?
6269-6330: Centralizing sym_mode selection and explicit DES3 branches: good improvementIntroducing sym_mode and adding explicit branches for CKM_DES3_{ECB,CBC,CBC_PAD} removes the earlier CBC hard‑coding and makes IV handling clear per mode. This also prepares the ground for future symmetric modes.
6900-6937: Symmetric unwrap: per‑mechanism mode and blocksize selection looks correctUnwrapKeySym now selects CBC vs ECB and blocksize per mechanism; DES3 uses 8‑byte blocks and ECB has no IV. This matches the wrap path and PKCS#11 expectations.
6636-6645: Argument validation for DES3 wrap: solid
- CKM_DES3_ECB enforcing ulParameterLen==0 is correct (ECB has no IV).
- CKM_DES3_{CBC,CBC_PAD} requiring an 8‑byte IV prevents null derefs and bad IV sizes.
7342-7349: Unwrap key type checks extended to DES3: good parity with wrapValidating that unwrapping keys for CKM_DES3_* are DES2/DES3 avoids inconsistent key/mechanism pairs.
7399-7423: CKA_VALUE_LEN capture in unwrap template: type‑checked and non‑intrusiveCapturing (not setting) CKA_VALUE_LEN and verifying post‑unwrap is a reasonable way to support caller expectations without polluting CreateObject inputs. Consider documenting that this applies only to CKO_SECRET_KEY in our developer docs.
Do you want a unit test that unwraps a 24‑byte 3DES key with CKA_VALUE_LEN=24 and asserts CKR_TEMPLATE_INCONSISTENT for mismatches?
7498-7502: Enforcing CKA_VALUE_LEN only for secret keys: correct scopeReturning CKR_TEMPLATE_INCONSISTENT on length mismatch for CKO_SECRET_KEY is consistent with how CreateObject validates lengths elsewhere in this file.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/lib/SoftHSM.cpp (1)
6986-6992: Map decryptFinal failures to CKR_WRAPPED_KEY_INVALID for non-PAD as wellFor invalid ciphertext, callers typically expect CKR_WRAPPED_KEY_INVALID rather than a generic error, even for non-PAD CBC/ECB. Unifying this improves API ergonomics and debuggability.
Apply:
- if (!cipher->decryptFinal(decryptedFinal)) { + if (!cipher->decryptFinal(decryptedFinal)) { cipher->recycleKey(unwrappingkey); CryptoFactory::i()->recycleSymmetricAlgorithm(cipher); - return (pMechanism->mechanism == CKM_AES_CBC_PAD || pMechanism->mechanism == CKM_DES3_CBC_PAD) - ? CKR_WRAPPED_KEY_INVALID - : CKR_GENERAL_ERROR; + return CKR_WRAPPED_KEY_INVALID; }
🧹 Nitpick comments (4)
src/lib/SoftHSM.cpp (4)
6269-6331: Preflight block-size alignment for non-PAD modes to return precise errorsFor non-PAD block modes (e.g., DES3_ECB, DES3_CBC, AES_CBC), wrapping fails later inside crypto with a generic error if the input length isn’t a multiple of the block size. Add an explicit alignment check to return CKR_KEY_SIZE_RANGE early.
Apply:
@@ switch(pMechanism->mechanism) { @@ case CKM_DES3_ECB: @@ bb = 7; break; } + // For non-PAD modes, enforce block alignment up-front for clearer error reporting + const bool isPadMode = + (pMechanism->mechanism == CKM_AES_CBC_PAD) || + (pMechanism->mechanism == CKM_DES3_CBC_PAD); + const size_t algoBlock = + (algo == SymAlgo::AES) ? 16 : + ((algo == SymAlgo::DES || algo == SymAlgo::DES3) ? 8 : 0); + const size_t effBlock = (sym_mode == SymMode::ECB) ? algoBlock : blocksize; + if (!isPadMode && effBlock != 0 && (wrappedlen % effBlock) != 0) + return CKR_KEY_SIZE_RANGE;
7281-7300: Mechanism param checks for DES3/AES unwrap — LGTM with one enhancementIV/param validation for AES_CBC_PAD and DES3 variants looks good. Consider also checking wrapped length alignment to fail early on impossible inputs.
Apply:
case CKM_DES3_CBC_PAD: - if (pMechanism->pParameter == NULL_PTR || pMechanism->ulParameterLen != 8) + if (pMechanism->pParameter == NULL_PTR || pMechanism->ulParameterLen != 8) return CKR_ARGUMENTS_BAD; + if (ulWrappedKeyLen == 0 || (ulWrappedKeyLen % 8) != 0) + return CKR_WRAPPED_KEY_LEN_RANGE; break; case CKM_DES3_CBC: - if (pMechanism->pParameter == NULL_PTR || pMechanism->ulParameterLen != 8) + if (pMechanism->pParameter == NULL_PTR || pMechanism->ulParameterLen != 8) return CKR_ARGUMENTS_BAD; + if ((ulWrappedKeyLen % 8) != 0) + return CKR_WRAPPED_KEY_LEN_RANGE; break; case CKM_DES3_ECB: if (pMechanism->ulParameterLen != 0) return CKR_ARGUMENTS_BAD; // ECB takes no IV; allow NULL or non-NULL pointer when length==0 + if ((ulWrappedKeyLen % 8) != 0) + return CKR_WRAPPED_KEY_LEN_RANGE; break;
7396-7420: Capture of CKA_VALUE_LEN in unwrap template — LGTM with a small guardNice addition. To catch bad lengths sooner, validate requested CKA_VALUE_LEN against keyType-specific rules before unwrapping.
Apply:
@@ - case CKA_VALUE_LEN: + case CKA_VALUE_LEN: // Accept a caller-provided length for compatibility, but do not set it here; we will verify after unwrap if (pTemplate[i].ulValueLen != sizeof(CK_ULONG)) { return CKR_TEMPLATE_INCONSISTENT; } requestedValueLen = *(CK_ULONG *) pTemplate[i].pValue; haveRequestedValueLen = true; + // Optional early validation against the target key type + if (objClass == CKO_SECRET_KEY) { + CK_RV lenRv = checkKeyLength(keyType, requestedValueLen); + if (lenRv != CKR_OK) return lenRv; + } continue;
7281-7284: Optional: parity between AES_CBC wrap and unwrapYou currently allow CKM_AES_CBC for wrapping but not for unwrapping. If intentional, consider documenting; if not, adding AES_CBC to C_UnwrapKey with IV-length check would restore symmetry.
Also applies to: 7339-7346
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/lib/SoftHSM.cpp(15 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/lib/SoftHSM.cpp (4)
src/lib/crypto/OSSLDES.cpp (4)
wrapKey(38-43)wrapKey(38-38)unwrapKey(45-50)unwrapKey(45-45)src/lib/crypto/BotanDES.cpp (4)
wrapKey(38-43)wrapKey(38-38)unwrapKey(45-50)unwrapKey(45-45)src/lib/crypto/BotanAES.cpp (4)
wrapKey(45-120)wrapKey(45-45)unwrapKey(123-211)unwrapKey(123-123)src/lib/crypto/OSSLAES.cpp (8)
wrapKey(41-48)wrapKey(41-41)wrapKey(50-53)wrapKey(50-50)unwrapKey(57-65)unwrapKey(57-57)unwrapKey(67-70)unwrapKey(67-67)
🔇 Additional comments (9)
src/lib/SoftHSM.cpp (9)
1140-1147: Mechanism info flags for DES3 ECB/CBC — LGTMExposing CKF_WRAP/CKF_UNWRAP for CKM_DES3_ECB/CBC aligns with the new wrap/unwrap support and keeps fallthrough semantics correct. No issues spotted.
6269-6331: Clean per-mech mode selection and DES3_ECB support — LGTMThe introduction of
sym_modeand explicit branches for CKM_DES3_{ECB,CBC,CBC_PAD} are clear and correct. Thebb=7adjustment for DES family is consistently applied.
6351-6364: Guarded IV handling for ECB — LGTMThe conditional IV copy avoids taking &iv[0] on an empty buffer and correctly wipes IV for ECB. Good fix.
6636-6644: Mechanism param checks for DES3 wrap — LGTM
- CKM_DES3_ECB: enforces zero-length parameter.
- CKM_DES3_CBC/PAD: require 8-byte IV.
These mirror CBC requirements and prevent NULL derefs down the path.
6685-6689: Key type gating for DES3 wrap — LGTMRequiring the wrapping key to be CKK_DES2 or CKK_DES3 for DES3 mechanisms is correct and prevents type misuse.
6901-6941: ECB/CBC mode selection and IV handling on unwrap — LGTM
sym_modeis propagated to decryptInit; ECB keeps IV empty; CBC variants copy the 8-byte IV. This fixes the earlier mode/IV issues.
7494-7499: Enforce CKA_VALUE_LEN after unwrap — LGTMPost-unwrap check and CKR_TEMPLATE_INCONSISTENT mapping look correct.
12114-12125: CKM_CONCATENATE_BASE_AND_KEY propagation of sensitivity/extractability — LGTMConjunction logic for ALWAYS_SENSITIVE/NEVER_EXTRACTABLE follows PKCS #11 guidance. Good attention to attribute semantics.
6293-6296: AES-CBC IV application fix — LGTMSetting
blocksize = 16for CKM_AES_CBC ensures the IV is actually copied and used; passingSymMode::CBCis consistent across wrap/unwrap paths.Also applies to: 2311-2325
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/lib/SoftHSM.cpp (2)
6271-6271: Minor: clarify naming to avoid mode vs. mode confusionYou now have two “mode” concepts in scope: SymWrap::Type mode and SymMode::Type sym_mode. Consider renaming sym_mode to cipher_mode to improve readability.
- SymMode::Type sym_mode = SymMode::Unknown; + SymMode::Type cipher_mode = SymMode::Unknown;
7405-7427: CKA_VALUE_LEN capture and enforcement after unwrap: good, confirm return code choiceAccepting CKA_VALUE_LEN in the template and enforcing it post-decrypt only for secret keys is sensible. Returning CKR_TEMPLATE_INCONSISTENT on mismatch is defensible; some stacks use CKR_WRAPPED_KEY_LEN_RANGE for length mismatches. If interoperability becomes an issue, consider switching to CKR_WRAPPED_KEY_LEN_RANGE.
Would you like me to add a small compatibility knob to choose the return code based on a config flag?
Also applies to: 7503-7506
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/lib/SoftHSM.cpp(15 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/lib/SoftHSM.cpp (4)
src/lib/crypto/BotanDES.cpp (4)
wrapKey(38-43)wrapKey(38-38)unwrapKey(45-50)unwrapKey(45-45)src/lib/crypto/OSSLDES.cpp (4)
wrapKey(38-43)wrapKey(38-38)unwrapKey(45-50)unwrapKey(45-45)src/lib/crypto/BotanAES.cpp (4)
wrapKey(45-120)wrapKey(45-45)unwrapKey(123-211)unwrapKey(123-123)src/lib/crypto/OSSLAES.cpp (8)
wrapKey(41-48)wrapKey(41-41)wrapKey(50-53)wrapKey(50-50)unwrapKey(57-65)unwrapKey(57-57)unwrapKey(67-70)unwrapKey(67-67)
🔇 Additional comments (7)
src/lib/SoftHSM.cpp (7)
1146-1146: Mechanism info: DES3 flags expanded correctlyAdding CKF_ENCRYPT | CKF_DECRYPT | CKF_WRAP | CKF_UNWRAP via |= is consistent with the fallthrough pattern used for AES and matches the wrap/unwrap capability you’ve implemented for DES3 ECB/CBC/PAD.
6360-6367: IV handling for ECB vs CBC is correct and safeGuarding memcpy with blocksize > 0 avoids taking &iv[0] on empty buffers, and passing the selected cipher mode into encryptInit fixes the earlier ECB/CBC mismatch.
6904-6941: Unwrap DES3/AES CBC(PAD): mode selection, IV handling, and error mapping look right
- Properly selects ECB/CBC per mechanism and keeps IV empty for ECB.
- Uses CKR_WRAPPED_KEY_INVALID for padding/decrypt failures (better for callers).
Also applies to: 6968-7004
6641-6648: Wrap parameter validation for DES3 ECB/CBC/PAD is sound
- ECB enforces zero-length parameters.
- CBC/CBC_PAD enforce 8-byte IVs.
This aligns with the unwrap-side checks you added.
6690-6693: Key type consistency checks for DES3 wrap are correctAllowing CKK_DES2 and CKK_DES3 for DES3 modes (and rejecting others) is appropriate.
7289-7307: DES3 unwrap validation looks completeCBC/CBC_PAD properly check 8-byte IVs and ciphertext alignment; ECB requires zero-length parameters and block-size alignment.
Also applies to: 7303-7307
12132-12132: Bug fix: NEVER_EXTRACTABLE propagation during concatenate-keys deriveSetting CKA_NEVER_EXTRACTABLE based on both inputs (AND) matches PKCS#11 2.40 §2.31.3. Good catch.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/lib/SoftHSM.cpp (4)
6357-6370: Guarded IV copy and correct mode passed to cipher — resolved correctlyThe “memcpy into iv[0] when size==0” hazard is eliminated and the selected
sym_modeis used consistently. Nice.
6643-6650: C_WrapKey: parameter validation for DES3 ECB/CBC/CBC_PAD
- ECB: requires ulParameterLen==0 (no IV)
- CBC/CBC_PAD: require 8-byte IV
These checks match the wrap implementation and prevent NULL deref in the IV copy.
6970-7006: UnwrapKeySym: guarded IV copy + error mapping to CKR_WRAPPED_KEY_INVALID
- IV copy is gated on blocksize>0 (safe for ECB).
- decryptInit uses correct
sym_mode.- Padding errors and decryptFinal failures return CKR_WRAPPED_KEY_INVALID instead of a generic error.
This aligns with PKCS #11 expectations.
7256-7312: C_UnwrapKey: robust size/IV validation for AES-CBC-PAD and 3DES (ECB/CBC/PAD)
- AES-CBC-PAD: IV length 16, wrapped length >0 and multiple of 16.
- DES3-CBC(PAD): IV length 8, length multiple of 8 (and >0 for PAD).
- DES3-ECB: parameter len 0 and multiple-of-8 data.
These checks fail early and match the decrypt path assumptions.
🧹 Nitpick comments (2)
src/lib/SoftHSM.cpp (2)
6293-6299: AES-CBC wrap: IV is used and input alignment is enforcedSetting
sym_mode=CBC,blocksize=16, and rejecting non-16-aligned inputs fixes the historical “IV never applied” issue and prevents silent misuse.If you want errors to better reflect “data length,” consider returning CKR_DATA_LEN_RANGE instead of CKR_KEY_SIZE_RANGE here for parity with encrypt paths. No strong preference; current code is consistent with DES3 branches.
7506-7511: Enforce CKA_VALUE_LEN for secret keys only — correct scopeReturning CKR_TEMPLATE_INCONSISTENT on mismatch makes failures explicit without affecting private-key unwrap flows.
Consider logging the expected vs actual size at DEBUG level to aid troubleshooting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/lib/SoftHSM.cpp(15 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/lib/SoftHSM.cpp (5)
src/lib/crypto/BotanDES.cpp (4)
wrapKey(38-43)wrapKey(38-38)unwrapKey(45-50)unwrapKey(45-45)src/lib/crypto/OSSLDES.cpp (4)
wrapKey(38-43)wrapKey(38-38)unwrapKey(45-50)unwrapKey(45-45)src/lib/crypto/AsymmetricAlgorithm.cpp (4)
wrapKey(159-165)wrapKey(159-159)unwrapKey(167-173)unwrapKey(167-167)src/lib/crypto/BotanAES.cpp (4)
wrapKey(45-120)wrapKey(45-45)unwrapKey(123-211)unwrapKey(123-123)src/lib/crypto/OSSLAES.cpp (8)
wrapKey(41-48)wrapKey(41-41)wrapKey(50-53)wrapKey(50-50)unwrapKey(57-65)unwrapKey(57-57)unwrapKey(67-70)unwrapKey(67-67)
🔇 Additional comments (8)
src/lib/SoftHSM.cpp (8)
1144-1147: Expose DES3 ECB/CBC as wrap/unwrap capable in mechanism info — good changeFlags now reflect the newly supported wrap/unwrap paths for 3DES ECB/CBC. Matches the implementation below in C_WrapKey/C_UnwrapKey.
6271-6274: Separate SymWrap vs SymMode for clarityAdding a dedicated
sym_modeavoids confusion withSymWrap::Type modeused for AES Key Wrap. Clear separation improves readability in the switch below.
6301-6305: 3DES wrap paths: PAD/non-PAD for CBC and new ECB support look correct
- CBC_PAD: PKCS#7 pre-padding is applied; padding=false at init is appropriate.
- CBC: enforces 8-byte alignment.
- ECB: treats IV as empty (blocksize=0), with length check on wrapped data.
All three set
bb=7to account for parity-inclusive key bytes.Also applies to: 6307-6314, 6315-6323, 6324-6332
6691-6695: C_WrapKey: 3DES wrapping key type checkAccepts CKK_DES2 and CKK_DES3; rejects others. This matches the supported modes and avoids misuse.
7351-7358: C_UnwrapKey: unwrapping key type consistency
- AES_CBC(_PAD) requires CKK_AES secret key.
- 3DES ECB/CBC(PAD) requires CKK_DES2 or CKK_DES3 secret key.
Prevents invalid key/mechanism pairings.
7409-7431: C_UnwrapKey: accept and capture CKA_VALUE_LEN in unwrap templateStoring (but not setting) CKA_VALUE_LEN for later enforcement preserves compatibility and avoids premature writes during template assembly.
Do we also want to accept (but ignore) CKA_VALUE for secret keys in unwrap templates and fail with CKR_ATTRIBUTE_READ_ONLY? If that’s a requirement in your environment, I can add the guard where you parse the template.
12126-12137: deriveSymmetric (CONCATENATE_BASE_AND_KEY): fix propagation of NEVER_EXTRACTABLE/ALWAYS_SENSITIVEThe boolean propagation now mirrors PKCS #11 rules (“true iff both originals are true” for ALWAYS_SENSITIVE, NEVER_EXTRACTABLE). This addresses the earlier inversion pitfall.
6923-6944: Confirmed: AES-CBC unwrap is intentionally unsupportedBoth the mechanism info and unwrap implementation only support AES-CBC-PAD for unwrapping:
- In src/lib/SoftHSM.cpp (lines 1159–1164), CKM_AES_CBC is only ever OR’d with CKF_WRAP (no CKF_UNWRAP is ever set)
- In the unwrap switch (around line 6970), only CKM_AES_CBC_PAD appears (plain CKM_AES_CBC is not handled)
Since CKM_AES_CBC correctly advertises wrap-only and there is no non-PAD unwrap path by design, no changes are required.
| algo = SymAlgo::AES; | ||
| sym_mode = SymMode::CBC; | ||
| blocksize = 16; | ||
| if ((wrappedlen % 16) != 0) | ||
| return CKR_KEY_SIZE_RANGE; | ||
| break; | ||
|
|
||
| case CKM_AES_CBC_PAD: | ||
| blocksize = 16; | ||
| wrappedlen = RFC5652Pad(keydata, blocksize); | ||
| algo = SymAlgo::AES; | ||
| break; | ||
|
|
||
| case CKM_DES3_CBC: | ||
| algo = SymAlgo::DES3; | ||
| sym_mode = SymMode::CBC; | ||
| break; | ||
|
|
||
| case CKM_DES3_CBC_PAD: | ||
| blocksize = 8; | ||
| blocksize = 8; | ||
| wrappedlen = RFC5652Pad(keydata, blocksize); | ||
| algo = SymAlgo::DES3; | ||
| sym_mode = SymMode::CBC; | ||
| bb = 7; | ||
| break; | ||
|
|
||
| case CKM_DES3_CBC: | ||
| if ((wrappedlen % 8) != 0) | ||
| return CKR_KEY_SIZE_RANGE; | ||
| blocksize = 8; | ||
| algo = SymAlgo::DES3; | ||
| sym_mode = SymMode::CBC; | ||
| bb = 7; | ||
| break; | ||
|
|
||
| case CKM_DES3_ECB: | ||
| // ECB mode has no IV; keep blocksize=0 so iv remains empty | ||
| if ((wrappedlen % 8) != 0) | ||
| return CKR_KEY_SIZE_RANGE; | ||
| blocksize = 0; | ||
| algo = SymAlgo::DES3; | ||
| sym_mode = SymMode::ECB; | ||
| bb = 7; | ||
| break; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add targeted tests for new 3DES ECB wrap/unwrap and template enforcement
- Wrap/unwrap CKM_DES3_ECB with:
- correct IV handling (none), correct multiples-of-8
- wrong ciphertext length → CKR_WRAPPED_KEY_LEN_RANGE
- corrupt last block/padding (CBC_PAD) → CKR_WRAPPED_KEY_INVALID
- Unwrap secret key with CKA_VALUE_LEN present:
- matching length → OK
- mismatching length → CKR_TEMPLATE_INCONSISTENT
I can draft unit tests exercising these cases if helpful.
Also applies to: 6923-6944, 7256-7312
🏁 Script executed:
#!/bin/bash
# Sanity: ensure the new paths are compiled-in and referenced
rg -n 'CKM_DES3_ECB' -C3 src/lib/SoftHSM.cpp
rg -n 'CKA_VALUE_LEN' -C2 src/lib/SoftHSM.cppLength of output: 13141
Add tests for 3DES ECB wrap/unwrap and CKA_VALUE_LEN enforcement
Please add unit tests covering the new DES3 ECB code paths and the template‐length checks:
• Wrap (SoftHSM.cpp ~6323–6332)
– CKM_DES3_ECB: ulParameterLen must be 0 (no IV)
– ciphertext lengths that are multiples of 8 → success
– non-8-aligned lengths → CKR_WRAPPED_KEY_LEN_RANGE
• Unwrap (SoftHSM.cpp ~7306–7312)
– CKM_DES3_ECB: ulParameterLen must be 0
– ulWrappedKeyLen multiple of 8 → success
– non-aligned lengths → CKR_WRAPPED_KEY_LEN_RANGE
– for CBC_PAD variants, corrupt padding → CKR_WRAPPED_KEY_INVALID
• Secret-key template enforcement (SoftHSM.cpp ~7408–7510)
– haveRequestedValueLen && keydata.size() == requestedValueLen → OK
– mismatched CKA_VALUE_LEN → CKR_TEMPLATE_INCONSISTENT
Let me know if you’d like me to draft those unit tests.
There was a problem hiding this comment.
I think the test should be part of this PR rather than a different PR generated by Code Rabbit. Those tests should be also cleaned up and verified. The reason is that this should be tested in CI before merging.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
✅ UTG Post-Process Complete No new issues were detected in the generated code and all check runs have completed. The unit test generation process has completed successfully. |
|
Unit Test Generation process completed. You can view the generated tests in this PR: View PR |
✅ Actions performedUnit Test Generation started... |
| bool bNeverExtractable = baseKey->getBooleanValue(CKA_NEVER_EXTRACTABLE, false) && | ||
| otherKey->getBooleanValue(CKA_NEVER_EXTRACTABLE, false); | ||
| bOK = bOK && osobject->setAttribute(CKA_ALWAYS_SENSITIVE, bNeverExtractable); | ||
| bOK = bOK && osobject->setAttribute(CKA_NEVER_EXTRACTABLE, bNeverExtractable); |
There was a problem hiding this comment.
This is an unrelated change which should be in its own PR with a test.
| ERROR_MSG("An object is with CKA_MODIFIABLE set to false is not modifiable"); | ||
| // Attributes cannot be changed if CKA_MODIFIABLE is false. | ||
| // Note: During CREATE/GENERATE/UNWRAP the object is in creation; allow setting attributes in the same op. | ||
| if (!isModifiable() && op != OBJECT_OP_GENERATE && op != OBJECT_OP_CREATE && op != OBJECT_OP_UNWRAP) { |
| algo = SymAlgo::AES; | ||
| sym_mode = SymMode::CBC; | ||
| blocksize = 16; | ||
| if ((wrappedlen % 16) != 0) | ||
| return CKR_KEY_SIZE_RANGE; | ||
| break; | ||
|
|
||
| case CKM_AES_CBC_PAD: | ||
| blocksize = 16; | ||
| wrappedlen = RFC5652Pad(keydata, blocksize); | ||
| algo = SymAlgo::AES; | ||
| break; | ||
|
|
||
| case CKM_DES3_CBC: | ||
| algo = SymAlgo::DES3; | ||
| sym_mode = SymMode::CBC; | ||
| break; | ||
|
|
||
| case CKM_DES3_CBC_PAD: | ||
| blocksize = 8; | ||
| blocksize = 8; | ||
| wrappedlen = RFC5652Pad(keydata, blocksize); | ||
| algo = SymAlgo::DES3; | ||
| sym_mode = SymMode::CBC; | ||
| bb = 7; | ||
| break; | ||
|
|
||
| case CKM_DES3_CBC: | ||
| if ((wrappedlen % 8) != 0) | ||
| return CKR_KEY_SIZE_RANGE; | ||
| blocksize = 8; | ||
| algo = SymAlgo::DES3; | ||
| sym_mode = SymMode::CBC; | ||
| bb = 7; | ||
| break; | ||
|
|
||
| case CKM_DES3_ECB: | ||
| // ECB mode has no IV; keep blocksize=0 so iv remains empty | ||
| if ((wrappedlen % 8) != 0) | ||
| return CKR_KEY_SIZE_RANGE; | ||
| blocksize = 0; | ||
| algo = SymAlgo::DES3; | ||
| sym_mode = SymMode::ECB; | ||
| bb = 7; | ||
| break; |
There was a problem hiding this comment.
I think the test should be part of this PR rather than a different PR generated by Code Rabbit. Those tests should be also cleaned up and verified. The reason is that this should be tested in CI before merging.
3 participants
Added support for CKM_DES3_ECB wrap/unwrap functionality.
Enabled the use of CKA_MODIFIABLE and CKA_VALUE_LEN in the unwrap template.
Based on pull request #657
Summary by CodeRabbit
New Features
Improvements
Bug Fixes