Allow OBJECT_OP_UNWRAP to modify attributes #728
Conversation
antoinelochet
force-pushed
the
fix_issue_707
branch
from
42d04e1 to
828e2c8
Compare
jschlyter
changed the title
|
Please rebase on develop and mark as ready when ready. |
antoinelochet
force-pushed
the
fix_issue_707
branch
from
828e2c8 to
42006eb
Compare
antoinelochet
force-pushed
the
fix_issue_707
branch
from
42006eb to
84d1af0
Compare
antoinelochet
force-pushed
the
fix_issue_707
branch
from
84d1af0 to
14749ef
Compare
|
Hello @jschlyter |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Warning Rate limit exceeded@antoinelochet has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 45 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughP11Attribute::update now treats OBJECT_OP_UNWRAP like OBJECT_OP_GENERATE and OBJECT_OP_CREATE for attribute modification checks. New tests added: SymmetricAlgorithmTests gains aesWrapUnwrapNonModifiableGeneric and calls it for CKM_AES_KEY_WRAP and CKM_AES_CBC_PAD to verify unwrap into a non-modifiable key and buffer behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as AES Test
participant PKCS11 as PKCS#11 Layer
participant Attr as P11Attribute::update
Test->>PKCS11: Unwrap(wrappedKey, mechanism, unwrapAttrs)
PKCS11->>Attr: update(unwrapAttrs, OBJECT_OP_UNWRAP)
Note right of Attr: treat OBJECT_OP_UNWRAP like\nOBJECT_OP_GENERATE / OBJECT_OP_CREATE\nfor modifiability/trusted checks
Alt attributes accepted
Attr-->>PKCS11: accept attributes
PKCS11-->>Test: return new key handle (unwrap succeeds)
Else modification denied
Attr-->>PKCS11: reject modification
PKCS11-->>Test: unwrap error (e.g., CKR_ATTRIBUTE_READ_ONLY)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
✨ 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 (
|
bjosv
left a comment
bjosv
left a comment
There was a problem hiding this comment.
I would be nice with a test in SymmetricAlgorithmTests.cpp similar to its testNonModifiableDesKeyGeneration.
antoinelochet
force-pushed
the
fix_issue_707
branch
from
14749ef to
9e67f7e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/lib/test/SymmetricAlgorithmTests.h (1)
44-58: Add the new test to the test suite.The new method
aesWrapUnwrapNonModifiableGenericis not included in the test suite. Consider adding it to ensure the new functionality is tested.Apply this diff to add the test:
CPPUNIT_TEST_SUITE(SymmetricAlgorithmTests); CPPUNIT_TEST(testAesEncryptDecrypt); CPPUNIT_TEST(testDesEncryptDecrypt); #ifdef HAVE_AES_KEY_WRAP CPPUNIT_TEST(testAesWrapUnwrap); +CPPUNIT_TEST(testAesWrapUnwrapNonModifiable); #endif CPPUNIT_TEST(testNullTemplate);You'll also need to add the corresponding public test method. Let me know if you'd like me to help with that implementation.
🧹 Nitpick comments (2)
src/lib/test/SymmetricAlgorithmTests.cpp (2)
1189-1190: Add documentation for the test method.Please add comments explaining that this test verifies unwrapping behavior when CKA_MODIFIABLE is set to false.
+// Test that unwrapping works correctly when CKA_MODIFIABLE is set to false. +// This verifies the fix for issue #707 which allows OBJECT_OP_UNWRAP to modify attributes +// even when CKA_MODIFIABLE is false. void SymmetricAlgorithmTests::aesWrapUnwrapNonModifiableGeneric(CK_MECHANISM_TYPE mechanismType, CK_SESSION_HANDLE hSession, CK_OBJECT_HANDLE hKey)
1189-1288: Consider refactoring to reduce code duplication.This method shares a lot of code with
aesWrapUnwrapGeneric. Consider extracting the common functionality into helper methods to improve maintainability.For example:
- Extract key generation and wrapping into a helper method
- Extract unwrapping and verification into another helper method
- Pass CKA_MODIFIABLE value as a parameter
+// Helper method for key generation and wrapping +void wrapKey(CK_SESSION_HANDLE hSession, CK_MECHANISM_TYPE mechanismType, + CK_OBJECT_HANDLE hKey, CK_BBOOL bExtractable, + CK_BYTE_PTR& wrappedPtr, CK_ULONG& wrappedLen) { + // Common key generation and wrapping code +} + +// Helper method for unwrapping and verification +void unwrapAndVerifyKey(CK_SESSION_HANDLE hSession, CK_MECHANISM_TYPE mechanismType, + CK_OBJECT_HANDLE hKey, CK_BBOOL bModifiable, + CK_BYTE_PTR wrappedPtr, CK_ULONG wrappedLen) { + // Common unwrapping and verification code +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/lib/P11Attributes.cpp(1 hunks)src/lib/test/SymmetricAlgorithmTests.cpp(1 hunks)src/lib/test/SymmetricAlgorithmTests.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/P11Attributes.cpp
🔇 Additional comments (1)
src/lib/test/SymmetricAlgorithmTests.h (1)
91-91: LGTM! Method signature follows existing patterns.The new method
aesWrapUnwrapNonModifiableGenericfollows the established pattern of similar methods in the class, maintaining consistency in the codebase.
antoinelochet
force-pushed
the
fix_issue_707
branch
from
9e67f7e to
1c98eb5
Compare
I have added a |
antoinelochet
force-pushed
the
fix_issue_707
branch
from
1c98eb5 to
9887274
Compare
|
As requested by @jschlyter, this PR has been rebased to the latest develop |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/lib/test/SymmetricAlgorithmTests.cpp (1)
1747-1748: Good integration: the new non-modifiable unwrap test is now exercisedThese calls address the earlier request to invoke aesWrapUnwrapNonModifiableGeneric from the test suite. Consider also covering the PAD variant when available:
Additional suggestion (outside this hunk, under HAVE_AES_KEY_WRAP_PAD):
#ifdef HAVE_AES_KEY_WRAP_PAD aesWrapUnwrapNonModifiableGeneric(CKM_AES_KEY_WRAP_PAD, hSession, hKey); #endif
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/lib/P11Attributes.cpp(1 hunks)src/lib/test/SymmetricAlgorithmTests.cpp(2 hunks)src/lib/test/SymmetricAlgorithmTests.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/test/SymmetricAlgorithmTests.h
- src/lib/P11Attributes.cpp
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/lib/test/SymmetricAlgorithmTests.cpp (1)
src/lib/SoftHSM.cpp (2)
C_WrapKey(4792-5063)C_UnwrapKey(5066-5361)
⏰ 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). (6)
- GitHub Check: macOS (botan)
- GitHub Check: Linux (openssl)
- GitHub Check: Windows (x64, openssl)
- GitHub Check: Linux (botan)
- GitHub Check: Windows (x64, botan)
- GitHub Check: Windows (x86, openssl)
antoinelochet
force-pushed
the
fix_issue_707
branch
from
9887274 to
ee7749e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/lib/test/SymmetricAlgorithmTests.cpp (2)
1189-1301: Good coverage of unwrap with CKA_MODIFIABLE=FALSE and solid cleanup
- Asserts CKR_KEY_UNEXTRACTABLE first, then wraps after making the key extractable.
- Validates wrap length including mechanism-dependent overhead.
- Unwraps with CKA_MODIFIABLE=CK_FALSE, verifies via C_GetAttributeValue, and destroys both hSecret and hNew to avoid leaks.
Addresses earlier feedback to verify CKA_MODIFIABLE and clean up objects.
1759-1760: Integration looks good; consider adding KEY_WRAP_PAD variant tooThe new helper is now invoked for CKM_AES_KEY_WRAP and CKM_AES_CBC_PAD — this addresses earlier integration feedback.
Optionally, mirror coverage for CKM_AES_KEY_WRAP_PAD inside the existing HAVE_AES_KEY_WRAP_PAD block, similar to aesWrapUnwrapGeneric/aesWrapUnwrapRsa.
Add inside the HAVE_AES_KEY_WRAP_PAD block:
// Also validate non-modifiable unwrap path for RFC5649 aesWrapUnwrapNonModifiableGeneric(CKM_AES_KEY_WRAP_PAD, hSession, hKey);
🧹 Nitpick comments (1)
src/lib/test/SymmetricAlgorithmTests.cpp (1)
1284-1293: Nit: read CKA_MODIFIABLE into a dedicated local variable for clarityUse a separate buffer instead of reusing &bFalse as the output buffer. It makes the intent explicit and avoids confusion with the input template value.
- CK_ATTRIBUTE modifiableAttribs[] = { - { CKA_MODIFIABLE, &bFalse, sizeof(bFalse) } - }; - - rv = CRYPTOKI_F_PTR( C_GetAttributeValue(hSession, hNew, modifiableAttribs, sizeof(modifiableAttribs)/sizeof(CK_ATTRIBUTE)) ); - CPPUNIT_ASSERT(rv == CKR_OK); - - CPPUNIT_ASSERT(modifiableAttribs[0].ulValueLen == sizeof(bFalse)); - CPPUNIT_ASSERT(*(CK_BBOOL*)modifiableAttribs[0].pValue == bFalse); + CK_BBOOL modifiableReadback = CK_TRUE; // sentinel to ensure we read back from token + CK_ATTRIBUTE getAttrs[] = { + { CKA_MODIFIABLE, &modifiableReadback, sizeof(modifiableReadback) } + }; + + rv = CRYPTOKI_F_PTR( C_GetAttributeValue(hSession, hNew, getAttrs, 1) ); + CPPUNIT_ASSERT(rv == CKR_OK); + + CPPUNIT_ASSERT(getAttrs[0].ulValueLen == sizeof(modifiableReadback)); + CPPUNIT_ASSERT(modifiableReadback == CK_FALSE);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/lib/P11Attributes.cpp(1 hunks)src/lib/test/SymmetricAlgorithmTests.cpp(2 hunks)src/lib/test/SymmetricAlgorithmTests.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/P11Attributes.cpp
- src/lib/test/SymmetricAlgorithmTests.h
antoinelochet
force-pushed
the
fix_issue_707
branch
from
ee7749e to
ccc1964
Compare
…ify attributes when CKA_MODIFIABLE is true
antoinelochet
force-pushed
the
fix_issue_707
branch
from
ccc1964 to
26ec19a
Compare
bukka
left a comment
bukka
left a comment
There was a problem hiding this comment.
The logic makes sense to me. It looks really just like an omission as unwrap is the same as create in this context. The test also looks fine.
@jschlyter please can you approve the workflow so CI runs (it's rebased so it should get properly tested)
4 participants
Allow attributes update when CKA_MODIFIABLE is false and the operation is UNWRAP.
Here is my proposed fix for #707
Summary by CodeRabbit
New Features
Bug Fixes