Support encryption for END_DATE and START_DATE attributes in private keys#665
Open
antoinelochet wants to merge 1 commit into
Open
Support encryption for END_DATE and START_DATE attributes in private keys#665antoinelochet wants to merge 1 commit into
antoinelochet wants to merge 1 commit into
Conversation
antoinelochet
force-pushed
the
fix_issue_655
branch
from
fcc2892 to
a00d998
Compare
antoinelochet
force-pushed
the
fix_issue_655
branch
from
4bb499c to
0ce6170
Compare
jschlyter
changed the title
Contributor
|
Please rebase on develop and mark as ready when ready. |
antoinelochet
force-pushed
the
fix_issue_655
branch
from
0ce6170 to
cba50f4
Compare
antoinelochet
force-pushed
the
fix_issue_655
branch
from
cba50f4 to
4d19f78
Compare
antoinelochet
force-pushed
the
fix_issue_655
branch
from
4d19f78 to
bdefa26
Compare
Contributor
Author
|
Hello @jschlyter |
antoinelochet
force-pushed
the
fix_issue_655
branch
from
bdefa26 to
0316da5
Compare
Contributor
Author
|
As requested by @jschlyter, this PR has been rebased to the latest develop |
Contributor
Author
|
This could be closed without merging when #805 is merged |
antoinelochet
force-pushed
the
fix_issue_655
branch
from
0316da5 to
8b69f59
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughEncrypts CKA_START_DATE/CKA_END_DATE for private objects, writes back OSAttribute on creation when present, adds per-attribute debug/error logs in template handling and decryption, introduces RSA creation tests with date attributes, and updates CI checkout/tool versions. ChangesPrivate Date Attribute Encryption and Logging
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
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)
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 (1)
src/lib/data_mgr/SecureDataManager.cpp (1)
400-453:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRemove raw ciphertext/IV/plaintext dumping from decrypt debug logs
%swithByteString::const_byte_str()is unsafe:const_byte_str()returns raw bytes without NUL termination (sentinel is uninitialized for the empty case), whilesoftHSMLogusesvsnprintfwhich treats%sas a C-string; this can cause out-of-bounds reads (UB) and also leaks encrypted/IV/plaintext material to DEBUG logs (enabled by default).- Replace those
%sbuffer dumps with length-only (or hex) logging.🛠️ Suggested patch
- DEBUG_MSG("encrypted %s", encrypted.const_byte_str()); + DEBUG_MSG("decrypt input length=%lu", (unsigned long)encrypted.size()); - DEBUG_MSG("AES block size %d", aes->getBlockSize()); + DEBUG_MSG("AES block size=%lu", (unsigned long)aes->getBlockSize()); - DEBUG_MSG("IV %s", IV.const_byte_str()); + DEBUG_MSG("IV length=%lu", (unsigned long)IV.size()); - DEBUG_MSG("plaintext %s", plaintext.const_byte_str()); + DEBUG_MSG("decrypt output length=%lu", (unsigned long)plaintext.size());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/data_mgr/SecureDataManager.cpp` around lines 400 - 453, The debug logs in SecureDataManager.cpp leak raw ciphertext/IV/plaintext and misuse %s with ByteString::const_byte_str(), causing potential UB and sensitive-data exposure; update the DEBUG_MSG calls that reference encrypted.const_byte_str(), IV.const_byte_str(), and plaintext.const_byte_str() (and any similar uses around the decrypt flow in the decrypt routine using aes/decryptInit/decryptUpdate/decryptFinal) to avoid printing raw bytes — log only lengths or hex-encoded safe representations (or remove the logs), and ensure you do not pass non-NUL-terminated buffers to %s (use a length-based formatter or omit const_byte_str()).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/P11Objects.cpp`:
- Around line 172-173: Logging in loadTemplate and saveTemplate uses wrong
printf specifiers: change the pTemplate[i].pValue specifier from (%x) to (%p)
and pass it as (void*)pTemplate[i].pValue, and change the CK_ULONG index
specifier from @ %d to @ %lu or cast the index to (unsigned long) when using
%lu; update the ERROR_MSG calls in loadTemplate and saveTemplate to use these
corrected format specifiers and casts to match types.
In `@src/lib/test/ObjectTests.cpp`:
- Line 1818: The calls to checkCommonKeyAttributes pass pointer-size lengths for
CK_DATE fields; replace sizeof(&startDate) and sizeof(&endDate) with the actual
CK_DATE byte sizes by using sizeof(startDate) and sizeof(endDate) in the calls
(references: checkCommonKeyAttributes, startDate, endDate) so the
ulStartDateLen/ulEndDateLen reflect the CK_DATE struct size rather than the
pointer size.
---
Outside diff comments:
In `@src/lib/data_mgr/SecureDataManager.cpp`:
- Around line 400-453: The debug logs in SecureDataManager.cpp leak raw
ciphertext/IV/plaintext and misuse %s with ByteString::const_byte_str(), causing
potential UB and sensitive-data exposure; update the DEBUG_MSG calls that
reference encrypted.const_byte_str(), IV.const_byte_str(), and
plaintext.const_byte_str() (and any similar uses around the decrypt flow in the
decrypt routine using aes/decryptInit/decryptUpdate/decryptFinal) to avoid
printing raw bytes — log only lengths or hex-encoded safe representations (or
remove the logs), and ensure you do not pass non-NUL-terminated buffers to %s
(use a length-based formatter or omit const_byte_str()).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bfb22f6a-0f22-4ada-bc61-fa038f08c28b
📒 Files selected for processing (5)
src/lib/P11Attributes.cppsrc/lib/P11Objects.cppsrc/lib/data_mgr/SecureDataManager.cppsrc/lib/test/ObjectTests.cppsrc/lib/test/ObjectTests.h
antoinelochet
force-pushed
the
fix_issue_655
branch
from
8b69f59 to
fc8bd90
Compare
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 (1)
src/lib/data_mgr/SecureDataManager.cpp (1)
400-452:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRemove sensitive decrypt-path logging that prints raw
ByteStringbytes as%s
DEBUG_MSG(... "%s", <ByteString>::const_byte_str())treats raw, non-null-terminated bytes as a C-string (can truncate on NULs and may read past the buffer); these statements logencrypted,IV, andplaintextdirectly inSecureDataManager::decrypt.🛠️ Minimal fix
bool SecureDataManager::decrypt(const ByteString& encrypted, ByteString& plaintext) { - DEBUG_MSG("encrypted %s", encrypted.const_byte_str()); // Check the object logged in state if ((!userLoggedIn && !soLoggedIn) || (maskedKey.size() != 32)) { return false; @@ - DEBUG_MSG("AES block size %d", aes->getBlockSize()); ByteString IV = encrypted.substr(0, aes->getBlockSize()); @@ - DEBUG_MSG("IV %s", IV.const_byte_str()); - ByteString finalBlock; @@ plaintext += finalBlock; - - DEBUG_MSG("plaintext %s", plaintext.const_byte_str()); return true; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/data_mgr/SecureDataManager.cpp` around lines 400 - 452, In SecureDataManager::decrypt, remove or replace unsafe DEBUG_MSG(...) calls that pass ByteString::const_byte_str() (currently logging variables encrypted, IV, and plaintext) because they treat raw bytes as C-strings and leak sensitive plaintext; instead log non-sensitive metadata (e.g., encrypted.size(), IV.size(), plaintext.size()) or a safe encoded representation (e.g., a hex/BASE64 of a limited prefix) using ByteString’s safe conversion helper rather than const_byte_str(); update the DEBUG_MSG invocations so they reference encrypted, IV, and plaintext only via safe methods and avoid printing raw decrypted data.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Line 14: Update all usages of the actions/checkout step (e.g., the occurrence
using "actions/checkout@v6") to pin to a commit SHA instead of a tag (leave the
tag as a trailing comment for human readability, e.g., "# v6") and add the input
"persist-credentials: false" to each checkout step (unless later steps
explicitly require git credentials); ensure you change every checkout occurrence
reported (the ones using actions/checkout) so they consistently use SHA pinning
and persistent credentials are disabled.
- Line 14: The workflow uses actions/checkout@v6 (and other actions like
ilammy/msvc-dev-cmd@v1.13.0 and johnwason/vcpkg-action@v8); ensure those
versions/tags exist and then harden each checkout step by adding with:
persist-credentials: false to the actions/checkout@v6 invocations so credentials
are not left in the workspace when not needed, and if your security policy
requires immutable pins, replace tags like actions/checkout@v6 with their
corresponding commit SHAs to fully pin the action.
---
Outside diff comments:
In `@src/lib/data_mgr/SecureDataManager.cpp`:
- Around line 400-452: In SecureDataManager::decrypt, remove or replace unsafe
DEBUG_MSG(...) calls that pass ByteString::const_byte_str() (currently logging
variables encrypted, IV, and plaintext) because they treat raw bytes as
C-strings and leak sensitive plaintext; instead log non-sensitive metadata
(e.g., encrypted.size(), IV.size(), plaintext.size()) or a safe encoded
representation (e.g., a hex/BASE64 of a limited prefix) using ByteString’s safe
conversion helper rather than const_byte_str(); update the DEBUG_MSG invocations
so they reference encrypted, IV, and plaintext only via safe methods and avoid
printing raw decrypted data.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3190ae94-c06c-4b15-ba15-ccd1b9625be4
📒 Files selected for processing (6)
.github/workflows/ci.ymlsrc/lib/P11Attributes.cppsrc/lib/P11Objects.cppsrc/lib/data_mgr/SecureDataManager.cppsrc/lib/test/ObjectTests.cppsrc/lib/test/ObjectTests.h
antoinelochet
force-pushed
the
fix_issue_655
branch
3 times, most recently
from
c7b9e02 to
5250f19
Compare
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/data_mgr/SecureDataManager.cpp (1)
400-452:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRemove raw ciphertext/IV/plaintext logging from SecureDataManager::decrypt
softHSMLogformats withvsnprintf, so the existingDEBUG_MSG("%s", <ByteString>.const_byte_str())uses%son non-NUL-terminated binary buffers (viaByteString::const_byte_str()), which is unsafe; it also leaks sensitive encrypted/IV/plaintext material fromSecureDataManager::decrypt(lines ~400-452). Replace these logs with non-sensitive metadata only (e.g., lengths / success flag), or use safe hex/encoded representations that don’t treat raw bytes as C strings.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/data_mgr/SecureDataManager.cpp` around lines 400 - 452, SecureDataManager::decrypt currently logs raw binary buffers (encrypted, IV, plaintext) using DEBUG_MSG("%s", ByteString::const_byte_str()), which is unsafe and leaks secrets; remove these raw %s logs and instead log only non-sensitive metadata or safe encodings: replace DEBUG_MSG/ERROR_MSG calls that print encrypted, IV or plaintext with statements that log lengths/status (e.g., encrypted.size(), IV.size(), plaintext.size()) or use a safe hex/base64 helper (not using %s on const_byte_str()) to produce NUL-safe representations; ensure references to symbols encrypted, IV, plaintext, DEBUG_MSG, ERROR_MSG, and ByteString::const_byte_str() are updated accordingly and do not print raw binary data.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/lib/data_mgr/SecureDataManager.cpp`:
- Around line 400-452: SecureDataManager::decrypt currently logs raw binary
buffers (encrypted, IV, plaintext) using DEBUG_MSG("%s",
ByteString::const_byte_str()), which is unsafe and leaks secrets; remove these
raw %s logs and instead log only non-sensitive metadata or safe encodings:
replace DEBUG_MSG/ERROR_MSG calls that print encrypted, IV or plaintext with
statements that log lengths/status (e.g., encrypted.size(), IV.size(),
plaintext.size()) or use a safe hex/base64 helper (not using %s on
const_byte_str()) to produce NUL-safe representations; ensure references to
symbols encrypted, IV, plaintext, DEBUG_MSG, ERROR_MSG, and
ByteString::const_byte_str() are updated accordingly and do not print raw binary
data.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2508535b-9eaa-4ab0-a3d5-afd619bb00fc
📒 Files selected for processing (6)
.github/workflows/ci.ymlsrc/lib/P11Attributes.cppsrc/lib/P11Objects.cppsrc/lib/data_mgr/SecureDataManager.cppsrc/lib/test/ObjectTests.cppsrc/lib/test/ObjectTests.h
✅ Files skipped from review due to trivial changes (1)
- src/lib/P11Objects.cpp
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/data_mgr/SecureDataManager.cpp (1)
400-452:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not log decrypted plaintext or crypto material in
decrypt().Lines 400/438/452 expose encrypted payloads, IV, and plaintext in logs; this can leak sensitive key material.
🔧 Suggested fix
- DEBUG_MSG("encrypted %s", encrypted.hex_str().c_str()); @@ - DEBUG_MSG("AES block size %d", aes->getBlockSize()); @@ - DEBUG_MSG("IV %s", IV.hex_str().c_str()); @@ - DEBUG_MSG("plaintext %s", plaintext.hex_str().c_str()); + // Avoid logging sensitive cryptographic material🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/data_mgr/SecureDataManager.cpp` around lines 400 - 452, The decrypt() implementation currently logs sensitive crypto material (encrypted, IV, plaintext and unmasked key) via DEBUG_MSG/ERROR_MSG; remove or redact those log calls and ensure unmaskedKey is never logged or persisted. Specifically, in SecureDataManager::decrypt (references: variables encrypted, IV, plaintext, unmaskedKey, AESKey theKey and calls unmask()/remask()) delete or replace DEBUG_MSG("encrypted ..."), DEBUG_MSG("IV ..."), and DEBUG_MSG("plaintext ...") with non-sensitive, high-level messages (e.g. "decrypt started" / "decrypt completed" or nothing), and ensure errors reported by ERROR_MSG do not include IV/plaintext/unmaskedKey contents; keep unmask()/remask() usage but never log their results.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/lib/data_mgr/SecureDataManager.cpp`:
- Around line 400-452: The decrypt() implementation currently logs sensitive
crypto material (encrypted, IV, plaintext and unmasked key) via
DEBUG_MSG/ERROR_MSG; remove or redact those log calls and ensure unmaskedKey is
never logged or persisted. Specifically, in SecureDataManager::decrypt
(references: variables encrypted, IV, plaintext, unmaskedKey, AESKey theKey and
calls unmask()/remask()) delete or replace DEBUG_MSG("encrypted ..."),
DEBUG_MSG("IV ..."), and DEBUG_MSG("plaintext ...") with non-sensitive,
high-level messages (e.g. "decrypt started" / "decrypt completed" or nothing),
and ensure errors reported by ERROR_MSG do not include IV/plaintext/unmaskedKey
contents; keep unmask()/remask() usage but never log their results.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 423ae12f-7b2e-46fc-90e0-2eff4001ef19
📒 Files selected for processing (6)
.github/workflows/ci.ymlsrc/lib/P11Attributes.cppsrc/lib/P11Objects.cppsrc/lib/data_mgr/SecureDataManager.cppsrc/lib/test/ObjectTests.cppsrc/lib/test/ObjectTests.h
antoinelochet
force-pushed
the
fix_issue_655
branch
from
5250f19 to
f1fa755
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/lib/test/ObjectTests.cpp (1)
1823-1884:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRemove duplicated test function definitions (merge artifact) and keep only one corrected copy per test.
testDefaultRSAPubAttributesWithDatesandtestDefaultRSAPrivAttributesWithDatesare each defined twice, which is a hard compile failure in C++. The second copies are also stale and still pass pointer sizes (sizeof(&startDate)/sizeof(&endDate)) intocheckCommonKeyAttributes.Suggested minimal fix
-void ObjectTests::testDefaultRSAPubAttributesWithDates() -{ - ... - checkCommonKeyAttributes(hSession, hObject, objType, NULL_PTR, 0, startDate, sizeof(&startDate), endDate, sizeof(&endDate), CK_FALSE, CK_FALSE, CK_UNAVAILABLE_INFORMATION, NULL_PTR, 0); - ... -} - ... -void ObjectTests::testDefaultRSAPrivAttributesWithDates() -{ - ... - checkCommonKeyAttributes(hSession, hObject, objType, NULL_PTR, 0, startDate, sizeof(&startDate), endDate, sizeof(&endDate), CK_FALSE, CK_FALSE, CK_UNAVAILABLE_INFORMATION, NULL_PTR, 0); - ... -}Also applies to: 2025-2098
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/test/ObjectTests.cpp` around lines 1823 - 1884, Remove the duplicate test function definitions and keep only one corrected copy of each test (testDefaultRSAPubAttributesWithDates and testDefaultRSAPrivAttributesWithDates); in the kept copies fix the incorrect size arguments passed to checkCommonKeyAttributes by replacing sizeof(&startDate)/sizeof(&endDate) with sizeof(startDate)/sizeof(endDate), and ensure all calls to checkCommonRSAPublicKeyAttributes/checkCommonRSAPrivateKeyAttributes and related checks remain in the single canonical test definitions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/lib/test/ObjectTests.cpp`:
- Around line 1823-1884: Remove the duplicate test function definitions and keep
only one corrected copy of each test (testDefaultRSAPubAttributesWithDates and
testDefaultRSAPrivAttributesWithDates); in the kept copies fix the incorrect
size arguments passed to checkCommonKeyAttributes by replacing
sizeof(&startDate)/sizeof(&endDate) with sizeof(startDate)/sizeof(endDate), and
ensure all calls to
checkCommonRSAPublicKeyAttributes/checkCommonRSAPrivateKeyAttributes and related
checks remain in the single canonical test definitions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5d3d32a4-e3f9-4519-bc29-7ef166ea853c
📒 Files selected for processing (6)
.github/workflows/ci.ymlsrc/lib/P11Attributes.cppsrc/lib/P11Objects.cppsrc/lib/data_mgr/SecureDataManager.cppsrc/lib/test/ObjectTests.cppsrc/lib/test/ObjectTests.h
🚧 Files skipped from review as they are similar to previous changes (4)
- src/lib/data_mgr/SecureDataManager.cpp
- src/lib/test/ObjectTests.h
- src/lib/P11Objects.cpp
- src/lib/P11Attributes.cpp
antoinelochet
force-pushed
the
fix_issue_655
branch
2 times, most recently
from
126c3a8 to
ce39f86
Compare
antoinelochet
force-pushed
the
fix_issue_655
branch
from
ce39f86 to
0a842d4
Compare
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.
Addresses the issue in #655
Summary by CodeRabbit
Bug Fixes
Tests
Chores