Explore new sensitive string implementation #754
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
New Issues
Fixed Issues
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #754 +/- ##
==========================================
+ Coverage 58.72% 59.05% +0.32%
==========================================
Files 177 179 +2
Lines 11501 11620 +119
==========================================
+ Hits 6754 6862 +108
- Misses 4747 4758 +11 ☔ View full report in Codecov by Sentry. |
| /// Extend the size of the string by `size`. | ||
| /// | ||
| /// Internally creates a new string with the new size and copies the old string into it. |
There was a problem hiding this comment.
| /// Extend the size of the string by `size`. | |
| /// | |
| /// Internally creates a new string with the new size and copies the old string into it. | |
| /// Extend the size of the string by `size`. | |
| /// | |
| /// Internally creates a new string with the new size and copies the old string into it, zeroing the original. |
| } | ||
|
|
||
| /// Extends the capacity of the string to `size` if it is larger than the current capacity. | ||
| pub fn extend_spec(&mut self, size: usize) { |
| /// Extend the size of the string by `size`. | ||
| /// | ||
| /// Internally creates a new string with the new size and copies the old string into it. | ||
| pub fn extend_size(&mut self, size: usize) { |
There was a problem hiding this comment.
What happens if size < self.inner.len()?
There was a problem hiding this comment.
It should have been inner.len() + size) 🤦
There was a problem hiding this comment.
A bunch of references to BitString, but I think the newer name of SensitiveString is better
| pub fn as_str(&self) -> &str { | ||
| &self.inner | ||
| } | ||
|
|
||
| /// Expose the inner value as a byte slice. | ||
| /// | ||
| /// Warning: Taking ownership of the inner value will create a copy of the string. | ||
| pub fn as_bytes(&self) -> &[u8] { |
There was a problem hiding this comment.
I think as long as these methods are public, it'll be really hard to identify potential leaks. I realize there are operations we need to do, but the goal should be containing those as much as possible.
The only thing I can think of right now that might be clearer is rather than allowing a method to expose the zeroized data, using something like a visitor pattern. We could more easily identify through linting rules implementers of an ExposeSensitiveData trait and develop means of understanding the level of exposure through macros that write fuzzing tests for us.
There was a problem hiding this comment.
Yes, unfortunately we do need direct access to the str and bytes in many scenarios. But as you said we need to be careful with how they are used since taking ownership of them means you need to care about zeroing it out.
I was leaning towards moving them into a separate trait so that we have more fine grained control of the usage, but I think ultimately we will have to rely on reviewers to be vigilant when using that trait.
# Conflicts: # crates/memory-testing/src/lib.rs # crates/memory-testing/src/main.rs
| @@ -310,7 +310,7 @@ | |||
| name: require!(send.name).parse()?, | |||
| notes: EncString::try_from_optional(send.notes)?, | |||
| key: require!(send.key).parse()?, | |||
| password: send.password.map(|p| SensitiveString::new(Box::new(p))), | |||
| password: send.password.map(|p| SensitiveString::new(p)), | |||
Check failure
Code scanning / clippy
redundant closure Error
|
Abandoned since we decided to use an allocator instead. |
Type of change
Objective
Code changes
Before you submit