-
Notifications
You must be signed in to change notification settings - Fork 129
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
zeroize: Implement Zeroize for CString (#650) #759
Conversation
This looks good at first glance:
As for writing a test, it seems like you should be able to use That's technically a use-after-free, since the |
Unfortunately this didn't work. In my test runs, the memory always contained random values after the zeroize. I also adapted the documentation. |
zeroize/src/lib.rs
Outdated
//! With the `std` feature enabled (which it is **not** by default), [`Zeroize`] | ||
//! is also implemented for [`CString`]. Due to its requirement of containing no | ||
//! null bytes, calling `zeroize()` on a `CString` will drop it after zeroing the | ||
//! memory. |
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 there's something a bit tricky and nuanced about the current implementation which is good to capture, but this doesn't quite describe it.
Namely it doesn't actually Drop
the CString
, but the pointer changes after calling zeroize()
and the original boxed slice is dropped. This may be surprising to CString
users.
I'm wondering if it might actually make sense to change the implementation to convert the taken Vec<u8>
back ::into_boxed_slice
after zeroization so that the original pointer remains valid for the lifetime of the CString
.
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.
Yea, I wasn't to sure about that part as well. I started with a more detailed explanation, but then wasn't sure if these implementation details should be part of the documentation.
I like the idea of putting the Vec back into the CString. I'll adapt the code to do this.
zeroize/src/lib.rs
Outdated
// DEFINITELY UB! We reconstruct the original buffer which should be deallocated after | ||
// the .zeroize() | ||
let orig_buf = std::slice::from_raw_parts(orig_ptr, orig_len); |
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.
Instead of trying to construct a slice out of the UAF'd memory, you can do pointer reads/derefs instead. That at least avoids instantiating something which violates the Rust memory model.
I'd really like to see a check for all zeroes succeed at least once. This test tells you little: the memory may be different simply because it's UAF and has been overwritten.
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.
Note: saw your other comment. Might be the other tests running in parallel in other threads causing heap allocations. Unfortunately this code is quite difficult to test.
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 used read_volatile
instead of a deref as otherwise only the first byte was checked when testing in debug mode.
When compiling with --release
apparently the whole loop is optimized away, even with read_volatile
(I must admit, I'm not a 100% clear on what exactly read_volatile
ensures).
73397a1
to
ba96cfc
Compare
Okay, I think my latest version is working and maintains validity of the pointer to the internal buffer. I've also added the Btw. is it better to amend changes and force push my branch so it remains one commit which can be easily merged, or is it better to add many small WIP commits which can then be squashed before merging? |
Ahh, that's unfortunate. I was using Edit: Oh, I misread the documentation of
Since the capacity is at least 1 and the length is 0, the capacity is sufficient and no reallocation should happen. |
You shouldn't need to use any of that. After that, use |
This implements Zeroize for std::ffi::CString. As CString is not in allocate but in std, a new optional std feature is added to depend on the std library.
@tarcieri I think this might be ready to merge now. |
Nice, looks good now. Thank you! |
I'm not quite sure if my implementation is correct and secure. In the issue linked in #650, it was mentioned that it likely is not possible to implement
Zeroize
forCString
, due to the requirement that there must be no null bytes in the buffer. One of the comments stated that this problem could maybe be solved in "hacky" way:Originally posted by @cyberia-ng in iqlusioninc/crates#509 (comment)
However, I think that my implementation of:
should be correct and secure.
std::mem::take
replaces replacesself
with aCString::default
. The original CString is then converted into aVec<u8>
(CString
itself contains aBox<u8>
), and zeroize is called on that. This should not result in a reallocation asinto_bytes
usesinto_vec
on the internalBox<u8>
buffer.I've also added a testcase similar to the one for
String
, but it doesn't actually test if the memory was zeroed, but only that the length of the internal buffer of theCString
is zero afterzeroize
which is done by themem::take
. Any ideas on how to properly test this code?If the code looks good, I'll also adapt the documentation to reflect the new impl and
std
feature :)