From 0679ecadf2a79f2acc3c83ce89d9a9af4566aa23 Mon Sep 17 00:00:00 2001 From: Robin Hundt <24554122+robinhundt@users.noreply.github.com> Date: Mon, 4 Apr 2022 19:21:00 +0200 Subject: [PATCH] Implement Zeroize for std::ffi::CString 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. --- .github/workflows/zeroize.yml | 2 +- zeroize/Cargo.toml | 1 + zeroize/src/lib.rs | 67 ++++++++++++++++++++++++++++++++--- 3 files changed, 64 insertions(+), 6 deletions(-) diff --git a/.github/workflows/zeroize.yml b/.github/workflows/zeroize.yml index d7126922..c67bf1e7 100644 --- a/.github/workflows/zeroize.yml +++ b/.github/workflows/zeroize.yml @@ -69,7 +69,7 @@ jobs: # Isolate this crate from workspace which is otherwise MSRV 1.56 due to 2021 edition crates - run: rm ../Cargo.toml - run: cargo test - - run: cargo test --features alloc,derive + - run: cargo test --features alloc,derive,std # Feature-gated ARM64 SIMD register support (nightly-only) aarch64: diff --git a/zeroize/Cargo.toml b/zeroize/Cargo.toml index 29d6cd99..4ffe23ee 100644 --- a/zeroize/Cargo.toml +++ b/zeroize/Cargo.toml @@ -24,6 +24,7 @@ default = ["alloc"] aarch64 = [] alloc = [] derive = ["zeroize_derive"] +std = [] [package.metadata.docs.rs] all-features = true diff --git a/zeroize/src/lib.rs b/zeroize/src/lib.rs index ceca6068..a8b34b8b 100644 --- a/zeroize/src/lib.rs +++ b/zeroize/src/lib.rs @@ -61,7 +61,13 @@ //! impl'd for `Vec` for the above types as well as `String`, where it provides //! [`Vec::clear`] / [`String::clear`]-like behavior (truncating to zero-length) //! but ensures the backing memory is securely zeroed with some caveats. -//! (NOTE: see "Stack/Heap Zeroing Notes" for important `Vec`/`String` details) +//! +//! With the `std` feature enabled (which it is **not** by default), [`Zeroize`] +//! is also implemented for [`CString`]. After calling `zeroize()` on a `CString`, +//! it will its internal buffer will contain exactly one nul byte. The backing +//! memory is zeroed by converting it to a `Vec` and back into a `CString`. +//! (NOTE: see "Stack/Heap Zeroing Notes" for important `Vec`/`String`/`CString` details) +//! //! //! The [`DefaultIsZeroes`] marker trait can be impl'd on types which also //! impl [`Default`], which implements [`Zeroize`] by overwriting a value with @@ -191,10 +197,10 @@ //! [`Pin`][`core::pin::Pin`] can be leveraged in conjunction with this crate //! to ensure data kept on the stack isn't moved. //! -//! The `Zeroize` impls for `Vec` and `String` zeroize the entire capacity of -//! their backing buffer, but cannot guarantee copies of the data were not -//! previously made by buffer reallocation. It's therefore important when -//! attempting to zeroize such buffers to initialize them to the correct +//! The `Zeroize` impls for `Vec`, `String` and `CString` zeroize the entire +//! capacity of their backing buffer, but cannot guarantee copies of the data +//! were not previously made by buffer reallocation. It's therefore important +//! when attempting to zeroize such buffers to initialize them to the correct //! capacity, and take care to prevent subsequent reallocation. //! //! The `secrecy` crate provides higher-level abstractions for eliminating @@ -233,6 +239,9 @@ #[cfg_attr(test, macro_use)] extern crate alloc; +#[cfg(feature = "std")] +extern crate std; + #[cfg(feature = "zeroize_derive")] #[cfg_attr(docsrs, doc(cfg(feature = "zeroize_derive")))] pub use zeroize_derive::{Zeroize, ZeroizeOnDrop}; @@ -253,6 +262,9 @@ use core::{ops, ptr, slice::IterMut, sync::atomic}; #[cfg(feature = "alloc")] use alloc::{boxed::Box, string::String, vec::Vec}; +#[cfg(feature = "std")] +use std::ffi::CString; + /// Trait for securely erasing types from memory pub trait Zeroize { /// Zero out this object from memory using Rust intrinsics which ensure the @@ -511,6 +523,27 @@ impl Zeroize for String { } } +#[cfg(feature = "std")] +#[cfg_attr(docsrs, doc(cfg(feature = "std")))] +impl Zeroize for CString { + fn zeroize(&mut self) { + // mem::take uses replace internally to swap the pointer + // Unfortunately this results in an allocation for a Box::new(&[0]) as CString must + // contain a trailing zero byte + let this = std::mem::take(self); + // - CString::into_bytes calls ::into_vec which takes ownership of the heap pointer + // as a Vec + // - Calling .zeroize() on the resulting vector clears out the bytes + // From: https://github.com/RustCrypto/utils/pull/759#issuecomment-1087976570 + let mut buf = this.into_bytes(); + buf.zeroize(); + // expect() should never fail, because zeroize() truncates the Vec + let zeroed = CString::new(buf).expect("buf not truncated"); + // Replace self by the zeroed CString to maintain the original ptr of the buffer + let _ = std::mem::replace(self, zeroed); + } +} + /// Fallible trait for representing cases where zeroization may or may not be /// possible. /// @@ -723,6 +756,9 @@ mod tests { #[cfg(feature = "alloc")] use alloc::vec::Vec; + #[cfg(feature = "std")] + use std::ffi::CString; + #[derive(Clone, Debug, PartialEq)] struct ZeroizedOnDrop(u64); @@ -881,6 +917,27 @@ mod tests { assert!(as_vec.iter().all(|byte| *byte == 0)); } + #[cfg(feature = "std")] + #[test] + fn zeroize_c_string() { + let mut cstring = CString::new("Hello, world!").expect("CString::new failed"); + let orig_len = cstring.as_bytes().len(); + let orig_ptr = cstring.as_bytes().as_ptr(); + cstring.zeroize(); + // This doesn't quite test that the original memory has been cleared, but only that + // cstring now owns an empty buffer + assert!(cstring.as_bytes().is_empty()); + for i in 0..orig_len { + unsafe { + // Using a simple deref, only one iteration of the loop is performed + // presumably because after zeroize, the internal buffer has a length of one/ + // `read_volatile` seems to "fix" this + // Note that this is very likely UB + assert_eq!(orig_ptr.add(i).read_volatile(), 0); + } + } + } + #[cfg(feature = "alloc")] #[test] fn zeroize_box() {