-
Notifications
You must be signed in to change notification settings - Fork 227
Add zeroization support for heapless data structures #614
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
Conversation
Thanks so much. I think it's definitely a good change. Before we do a proper review, could you please squash the commits and force push (please do not create a new PR). |
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.
That's a good feature to have, thank you.
A couple of changes are needed but this looks good!
src/vec/mod.rs
Outdated
#[cfg_attr( | ||
feature = "zeroize", | ||
derive(Zeroize), | ||
zeroize(bound = "S: Zeroize, LenT: Zeroize") |
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.
I think that in the case the Zeroize
feature is enabled, the LenType
sealed trait can be a supertrait of Zeroize
so that the bounds are simpler.
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.
Resolved!
23b933e
to
e08f29b
Compare
@vishy11 thanks! I hate to ask but could please rebase (rather than merge) on master, squash the remaining 2 commits and use the PR description as the commit message? Then we'll have a clean history. 🙏 |
7c07198
to
ed337d8
Compare
Apologies, was having a lot of the commits mess with each other - they should be good now. Let me know if you need any other changes and I'll do my best to get them done! |
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. 👍
No worries. It looks all good now. For future reference, I'd suggest first checking the history locally to see everything is in order before submitting the PR and each time before pushing changes to the PR. It's just easier for reviewers this way. 👍 |
src/string/mod.rs
Outdated
#[cfg_attr( | ||
feature = "zeroize", | ||
derive(Zeroize), | ||
zeroize(bound = "S: Zeroize, LenT: Zeroize") |
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.
zeroize(bound = "S: Zeroize, LenT: Zeroize") | |
zeroize(bound = "S: Zeroize") |
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.
Resolved!
src/c_string.rs
Outdated
#[cfg(feature = "zeroize")] | ||
impl<const N: usize, LenT: LenType> Zeroize for CString<N, LenT> | ||
where | ||
LenT: Zeroize, |
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.
#[cfg(feature = "zeroize")] | |
impl<const N: usize, LenT: LenType> Zeroize for CString<N, LenT> | |
where | |
LenT: Zeroize, | |
#[cfg(feature = "zeroize")] | |
impl<const N: usize, LenT: LenType> Zeroize for CString<N, LenT> |
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.
Resolved!
… securely clear sensitive data from memory: - When the zeroize feature is enabled, the LenType sealed trait now has Zeroize as a supertrait - This simplifies the bound for deriving Zeroize for VecInner and other types - Added tests to verify VecView also implements Zeroize correctly This feature is essential for security-sensitive applications needing to prevent data leaks from memory dumps. Note: Zeroize initially worked on Vector purely via derivation, however was not complete without proper bound checks. Without these checks, the deref implementation of Zeroize was used instead, which led to incomplete zeroization of the Vector's contents.
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 contribution!
Implements zeroization support across all heapless data structures to securely clear sensitive data from memory:
Vector
,CString
,String
,IndexMap
,IndexSet
, and other structuresThis feature is essential for security-sensitive applications needing to prevent data leaks from memory dumps.
Note: Zeroize initially worked on
Vector
purely via derivation, however was not complete without proper bound checks. Without these checks, thederef
implementation of Zeroize was used instead, which led to incomplete zeroization of the Vector's contents.