Added support for AES CBC Key Unwrap#871
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds explicit CKM_AES_CBC wrap/unwrap handling: reports AES-CBC as both wrap and unwrap, initializes AES CBC blocksize for wrapping, implements IV-based CBC decryption without unpadding for unwrap, accepts CKM_AES_CBC in C_UnwrapKey, and extends tests to cover AES-CBC (non-padding) paths. ChangesAES-CBC wrap/unwrap
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant SoftHSM as SoftHSM (PKCS#11)
participant Crypto as CryptoEngine
participant Store as KeyStore
Client->>SoftHSM: C_UnwrapKey(mech=CKM_AES_CBC, mechParams(IV), wrappedData)
SoftHSM->>SoftHSM: validate mechanism & IV length
SoftHSM->>Crypto: decryptInit(alg=AES-CBC, key, IV)
SoftHSM->>Crypto: decryptUpdate(wrappedData)
SoftHSM->>Crypto: decryptFinal()
Crypto-->>SoftHSM: plaintext_keydata
SoftHSM->>SoftHSM: assemble key object (no unpadding)
SoftHSM->>Store: persist key / return handle
Store-->>Client: return handle / CKR_OK
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
|
Resolves the CBC Unwrap portion of this issue: #856 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/SoftHSM.cpp (1)
7596-7609:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd wrapped blob block-alignment checks during
C_UnwrapKeymechanism validation.IV length is validated, but ciphertext length is not. For CBC unwrap, reject non-block-aligned
ulWrappedKeyLenearly (CKR_WRAPPED_KEY_LEN_RANGE) instead of failing later as generic decrypt errors.Proposed patch
case CKM_AES_CBC: case CKM_AES_CBC_PAD: - // TODO check block length if (pMechanism->pParameter == NULL_PTR || pMechanism->ulParameterLen != 16) return CKR_ARGUMENTS_BAD; + if (ulWrappedKeyLen == 0 || (ulWrappedKeyLen % 16) != 0) + return CKR_WRAPPED_KEY_LEN_RANGE; break; case CKM_DES3_CBC_PAD: - // TODO check block length 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;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/SoftHSM.cpp` around lines 7596 - 7609, In C_UnwrapKey mechanism validation for CKM_AES_CBC, CKM_AES_CBC_PAD and CKM_DES3_CBC_PAD, add a block-alignment check on ulWrappedKeyLen: ensure ulWrappedKeyLen is at least pMechanism->ulParameterLen + blockSize and that (ulWrappedKeyLen - pMechanism->ulParameterLen) % blockSize == 0 (use blockSize=16 for AES, 8 for 3DES); if not, return CKR_WRAPPED_KEY_LEN_RANGE instead of proceeding to decryption. Locate these checks near the existing IV-length checks that reference pMechanism->pParameter and pMechanism->ulParameterLen and enforce the new condition before breaking out of the switch.
🤖 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/SoftHSM.cpp`:
- Around line 7596-7609: In C_UnwrapKey mechanism validation for CKM_AES_CBC,
CKM_AES_CBC_PAD and CKM_DES3_CBC_PAD, add a block-alignment check on
ulWrappedKeyLen: ensure ulWrappedKeyLen is at least pMechanism->ulParameterLen +
blockSize and that (ulWrappedKeyLen - pMechanism->ulParameterLen) % blockSize ==
0 (use blockSize=16 for AES, 8 for 3DES); if not, return
CKR_WRAPPED_KEY_LEN_RANGE instead of proceeding to decryption. Locate these
checks near the existing IV-length checks that reference pMechanism->pParameter
and pMechanism->ulParameterLen and enforce the new condition before breaking out
of the switch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 421f8fe1-10aa-4e04-a83a-9396b68ef9fd
📒 Files selected for processing (2)
src/lib/SoftHSM.cppsrc/lib/test/SymmetricAlgorithmTests.cpp
| algo = SymAlgo::AES; | ||
| break; | ||
|
|
||
| case CKM_DES3_CBC: |
There was a problem hiding this comment.
There is PR #812 about CKM_DES3_CBC. I think it would be better to remove this change.
|
Apart from my comment about CKM_DES3_CBC blocksize, this seems good ! |
|
Looks sensible to me |
bukka
left a comment
bukka
left a comment
There was a problem hiding this comment.
I agree with @antoinelochet comment so please get rid of that bit and then it should be fine to merge.
|
Thanks all for the review! |
4 participants
Summary by CodeRabbit
Bug Fixes
Tests