diff --git a/Cargo.lock b/Cargo.lock index eb46bdf81..4e5732a1e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3425,6 +3425,7 @@ dependencies = [ "thiserror", "tor-error", "tor-llcrypto", + "zeroize", ] [[package]] diff --git a/crates/tor-bytes/Cargo.toml b/crates/tor-bytes/Cargo.toml index 18b13442b..06bb81b6e 100644 --- a/crates/tor-bytes/Cargo.toml +++ b/crates/tor-bytes/Cargo.toml @@ -21,6 +21,7 @@ signature = "1" thiserror = "1" tor-error = { path = "../tor-error", version = "0.3.2" } tor-llcrypto = { path = "../tor-llcrypto", version = "0.3.3" } +zeroize = "1" [dev-dependencies] hex-literal = "0.3" diff --git a/crates/tor-bytes/semver.md b/crates/tor-bytes/semver.md new file mode 100644 index 000000000..7ce9421d0 --- /dev/null +++ b/crates/tor-bytes/semver.md @@ -0,0 +1,2 @@ +MODIFIED: New SecretBuf API. + diff --git a/crates/tor-bytes/src/lib.rs b/crates/tor-bytes/src/lib.rs index fa868c475..134bf69c4 100644 --- a/crates/tor-bytes/src/lib.rs +++ b/crates/tor-bytes/src/lib.rs @@ -86,10 +86,12 @@ mod err; mod impls; mod reader; +mod secretbuf; mod writer; pub use err::{EncodeError, Error}; pub use reader::Reader; +pub use secretbuf::SecretBuf; pub use writer::Writer; use arrayref::array_ref; diff --git a/crates/tor-bytes/src/secretbuf.rs b/crates/tor-bytes/src/secretbuf.rs new file mode 100644 index 000000000..02e28e0cf --- /dev/null +++ b/crates/tor-bytes/src/secretbuf.rs @@ -0,0 +1,123 @@ +//! Define a wrapper for Vec that will act as Writer, but zeroize its +//! contents on drop or reallocation. + +use crate::Writer; +use zeroize::{Zeroize, ZeroizeOnDrop}; + +/// A [`Writer`] used for accumulating secret data, which gets cleared on drop. +/// +/// Unlike `Zeroizing>`, this type makes sure that we always zeroize the +/// contents of the buffer, even if the buffer has to be reallocated in order to +/// grow. +/// +/// We use this for cases when we're building the input to a key derivation +/// function (KDF), and want to ensure that we don't expose the values we feed +/// to it. +/// +/// This struct is expected to have additional overhead beyond `Vec` only +/// when it has to grow its capacity. +#[derive(Zeroize, ZeroizeOnDrop, Debug, Clone, Eq, PartialEq)] +pub struct SecretBuf(Vec); + +/// The default size of our buffer. +/// +/// This is based on the size of a typical secret input in `tor-proto`. +const DEFAULT_CAPACITY: usize = 384; + +impl SecretBuf { + /// Construct a new empty [`SecretBuf`] + pub fn new() -> Self { + Self::with_capacity(DEFAULT_CAPACITY) + } + + /// Construct a new empty [`SecretBuf`] with a specified capacity. + /// + /// This buffer will not have to be reallocated until it uses `capacity` + /// bytes. + pub fn with_capacity(capacity: usize) -> Self { + Self(Vec::with_capacity(capacity)) + } + + /// Truncate this buffer to a given length. + pub fn truncate(&mut self, new_len: usize) { + self.0.truncate(new_len); + } + + /// Add all the bytes from `slice` to the end of this vector. + pub fn extend_from_slice(&mut self, slice: &[u8]) { + let new_len = self.0.len() + slice.len(); + if new_len >= self.0.capacity() { + // We will need to reallocate. But in doing so we might reallocate, + // which neglects to zero the previous contents. So instead, + // explicitly make a new vector and zeroize the old one. + + // Make sure we always at least double our capacity. + let new_capacity = std::cmp::max(self.0.capacity() * 2, new_len); + let mut new_vec = Vec::with_capacity(new_capacity); + new_vec.extend_from_slice(&self.0[..]); + + let mut old_vec = std::mem::replace(&mut self.0, new_vec); + old_vec.zeroize(); + } + self.0.extend_from_slice(slice); + debug_assert_eq!(self.0.len(), new_len); + } +} + +impl From> for SecretBuf { + fn from(v: Vec) -> Self { + Self(v) + } +} + +impl Default for SecretBuf { + fn default() -> Self { + Self::new() + } +} + +impl AsMut<[u8]> for SecretBuf { + fn as_mut(&mut self) -> &mut [u8] { + &mut self.0[..] + } +} + +// It's okay to implement `Deref` since all operations taking an _immutable_ +// reference are still right here. +impl std::ops::Deref for SecretBuf { + type Target = Vec; + + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +impl Writer for SecretBuf { + fn write_all(&mut self, b: &[u8]) { + self.extend_from_slice(b); + } +} + +#[cfg(test)] +mod test { + use super::*; + + #[test] + fn simple_case() -> crate::EncodeResult<()> { + // Sadly, there is no way in safe rust to test that the zeroization + // actually happened. All we can test is that the data is correct. + + let mut buf1 = SecretBuf::default(); + let mut buf2 = Vec::new(); + let xyz = b"Nine hundred pounds of sifted flax"; + + // This is enough to be sure that we'll reallocate. + for _ in 0..200 { + buf1.write(xyz)?; + buf2.write(xyz)?; + } + assert_eq!(&buf1[..], &buf2[..]); + + Ok(()) + } +}