-
Notifications
You must be signed in to change notification settings - Fork 49
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve zeroization of key buffer #1069
Conversation
66f20bd
to
40b3b4a
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1069 +/- ##
==========================================
- Coverage 58.12% 58.11% -0.01%
==========================================
Files 197 197
Lines 13532 13531 -1
==========================================
- Hits 7865 7864 -1
Misses 5667 5667 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @AaronFeickert,
This change looks reasonable, I think the only disadvantage is that it's slightly less obvious that res
is mutated.
Since we wrote this code we've also implement our own Allocator which zeros on dealloc which should act as an additional guard against sensitive memory being leaked.
Can you please run cargo +nightly fmt
to make the formatted happy?
40b3b4a
to
6b5cb25
Compare
@Hinton sorry about that! I had run the formatter locally, but must have forgotten to push it. It's now passing formatting and has been rebased. |
6b5cb25
to
a779aed
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the improvement.
🎟️ Tracking
None.
📔 Objective
Currently, the temporary buffer used for deriving shareable keys is manually zeroized. While documentation indicates that preceding
expect
calls cannot fail, this still seems brittle to future changes.This PR places the buffer into a
Zeroizing
wrapper. It is still the case that zeroization may not occur on a panic, but this was already the case with the existing implementation, which would never zeroize in such a case.⏰ Reminders before review
team
🦮 Reviewer guidelines
:+1:
) or similar for great changes:memo:
) or ℹ️ (:information_source:
) for notes or general info:question:
) for questions:thinking:
) or 💭 (:thought_balloon:
) for more open inquiry that's not quite a confirmedissue and could potentially benefit from discussion
:art:
) for suggestions / improvements:x:
) or:warning:
) for more significant problems or concerns needing attention:seedling:
) or ♻️ (:recycle:
) for future improvements or indications of technical debt:pick:
) for minor or nitpick changes