diff --git a/src/secure_types/array.rs b/src/secure_types/array.rs index 6bb2e34..ff3737d 100644 --- a/src/secure_types/array.rs +++ b/src/secure_types/array.rs @@ -5,7 +5,7 @@ use std::str::FromStr; use subtle::ConstantTimeEq; use zeroize::Zeroize; -use crate::secure_utils::memlock; +use crate::SecureBox; /// A data type suitable for storing sensitive information such as passwords and /// private keys in memory, that implements: @@ -19,28 +19,36 @@ use crate::secure_utils::memlock; /// - Automatic `mlock` to protect against leaking into swap (any unix) /// - Automatic `madvise(MADV_NOCORE/MADV_DONTDUMP)` to protect against leaking /// into core dumps (FreeBSD, DragonflyBSD, Linux) +/// +/// The contents are stored on the heap (via [`SecureBox`]) so the locked memory +/// region has a stable address: moving the `SecureArray` only moves the +/// pointer, keeping the `mlock` valid. pub struct SecureArray where T: Copy + Zeroize, { - pub(crate) content: [T; LENGTH], + inner: SecureBox<[T; LENGTH]>, } impl SecureArray where T: Copy + Zeroize, { - pub fn new(mut content: [T; LENGTH]) -> Self { - memlock::mlock(content.as_mut_ptr(), content.len()); - Self { content } + #[must_use] + pub fn new(content: [T; LENGTH]) -> Self { + Self { + inner: SecureBox::new(Box::new(content)), + } } /// Borrow the contents of the string. + #[must_use] pub fn unsecure(&self) -> &[T] { self.borrow() } /// Mutably borrow the contents of the string. + #[must_use] pub fn unsecure_mut(&mut self) -> &mut [T] { self.borrow_mut() } @@ -48,13 +56,15 @@ where /// Overwrite the string with zeros. This is automatically called in the /// destructor. pub fn zero_out(&mut self) { - self.content.zeroize(); + self.inner.unsecure_mut().zeroize(); } } impl Clone for SecureArray { fn clone(&self) -> Self { - Self::new(self.content) + Self { + inner: self.inner.clone(), + } } } @@ -62,7 +72,7 @@ impl ConstantTimeEq for SecureArray { fn ct_eq(&self, other: &Self) -> subtle::Choice { - self.content.as_slice().ct_eq(other.content.as_slice()) + self.unsecure().ct_eq(other.unsecure()) } } @@ -117,7 +127,7 @@ where type Output = <[T; LENGTH] as std::ops::Index>::Output; fn index(&self, index: U) -> &Self::Output { - std::ops::Index::index(&self.content, index) + std::ops::Index::index(self.inner.unsecure(), index) } } @@ -127,7 +137,7 @@ where T: Copy + Zeroize, { fn borrow(&self) -> &[T] { - self.content.borrow() + self.inner.unsecure().as_slice() } } @@ -136,18 +146,7 @@ where T: Copy + Zeroize, { fn borrow_mut(&mut self) -> &mut [T] { - self.content.borrow_mut() - } -} - -// Overwrite memory with zeros when we're done -impl Drop for SecureArray -where - T: Copy + Zeroize, -{ - fn drop(&mut self) { - self.zero_out(); - memlock::munlock(self.content.as_mut_ptr(), self.content.len()); + self.inner.unsecure_mut().as_mut_slice() } } @@ -221,6 +220,18 @@ mod tests { ); } + #[test] + fn test_move_keeps_contents() { + // Regression guard for the move-after-mlock bug: moving the value must + // preserve the contents (data lives on the heap behind a SecureBox). + fn make() -> SecureArray { + SecureArray::from_str("hello").unwrap() + } + let moved = make(); + let v = [moved]; + assert_eq!(v[0].unsecure(), b"hello"); + } + #[test] fn test_comparison_zero_out_multibyte() { let data1 = SecureArray::from([ diff --git a/src/secure_types/boxed.rs b/src/secure_types/boxed.rs index 2be9c2b..385c064 100644 --- a/src/secure_types/boxed.rs +++ b/src/secure_types/boxed.rs @@ -26,6 +26,9 @@ where // This is an `Option` to avoid UB in the destructor, outside the destructor, it is always // `Some(_)` content: Option>, + /// Whether `content` is currently `mlock`ed. If `mlock` failed, `munlock` + /// must be skipped. + is_locked: bool, } impl SecureBox @@ -34,9 +37,10 @@ where { #[must_use] pub fn new(mut cont: Box) -> Self { - memlock::mlock(&raw mut *cont, 1); + let is_locked = memlock::mlock(&raw mut *cont, 1); SecureBox { content: Some(cont), + is_locked, } } @@ -57,6 +61,7 @@ where /// # Panics /// /// Panics if the content has already been dropped. + #[must_use] pub fn unsecure_mut(&mut self) -> &mut T { self.content .as_deref_mut() @@ -141,7 +146,9 @@ where .zeroize(); } - memlock::munlock(ptr, 1); + if self.is_locked { + memlock::munlock(ptr, 1); + } // Deallocate only non-zero-sized types, because otherwise it's UB if std::mem::size_of::() != 0 { diff --git a/src/secure_types/string.rs b/src/secure_types/string.rs index 395fa6e..92803a3 100644 --- a/src/secure_types/string.rs +++ b/src/secure_types/string.rs @@ -19,6 +19,7 @@ impl SecureString { } /// Mutably borrow the contents of the string. + #[must_use] pub fn unsecure_mut(&mut self) -> &mut str { // SAFETY: Same as `unsecure` - contents are always valid UTF-8. unsafe { std::str::from_utf8_unchecked_mut(self.0.unsecure_mut()) } @@ -27,7 +28,9 @@ impl SecureString { /// Turn the string into a regular `String` again. #[must_use] pub fn into_unsecure(mut self) -> String { - memlock::munlock(self.0.content.as_mut_ptr(), self.0.content.capacity()); + if self.0.is_locked { + memlock::munlock(self.0.content.as_mut_ptr(), self.0.content.capacity()); + } let content = std::mem::take(&mut self.0.content); std::mem::forget(self); // SAFETY: Same as `unsecure` - contents are always valid UTF-8. @@ -98,7 +101,7 @@ impl<'de> serde::Deserialize<'de> for SecureString { D: serde::Deserializer<'de>, { struct SecureStringVisitor; - impl<'de> serde::de::Visitor<'de> for SecureStringVisitor { + impl serde::de::Visitor<'_> for SecureStringVisitor { type Value = SecureString; fn expecting(&self, formatter: &mut ::std::fmt::Formatter) -> ::std::fmt::Result { write!(formatter, "an utf-8 encoded string") diff --git a/src/secure_types/vec.rs b/src/secure_types/vec.rs index 9e7e869..cefbc6b 100644 --- a/src/secure_types/vec.rs +++ b/src/secure_types/vec.rs @@ -31,6 +31,9 @@ where T: Copy + Zeroize, { pub(crate) content: Vec, + /// Whether `content` is currently `mlock`ed. If `mlock` failed, `munlock` + /// must be skipped. + pub(crate) is_locked: bool, } /// Type alias for a vector that stores just bytes @@ -42,8 +45,11 @@ where { #[must_use] pub fn new(mut cont: Vec) -> Self { - memlock::mlock(cont.as_mut_ptr(), cont.capacity()); - SecureVec { content: cont } + let is_locked = memlock::mlock(cont.as_mut_ptr(), cont.capacity()); + SecureVec { + content: cont, + is_locked, + } } /// Borrow the contents of the string. @@ -53,6 +59,7 @@ where } /// Mutably borrow the contents of the string. + #[must_use] pub fn unsecure_mut(&mut self) -> &mut [T] { self.borrow_mut() } @@ -76,13 +83,16 @@ where // Allocate new vector, copy old data into it let mut new_vec = vec![value; new_len]; - memlock::mlock(new_vec.as_mut_ptr(), new_vec.capacity()); + let new_is_locked = memlock::mlock(new_vec.as_mut_ptr(), new_vec.capacity()); new_vec[0..self.content.len()].copy_from_slice(&self.content); // Securely clear old vector, replace with new vector self.zero_out(); - memlock::munlock(self.content.as_mut_ptr(), self.content.capacity()); + if self.is_locked { + memlock::munlock(self.content.as_mut_ptr(), self.content.capacity()); + } self.content = new_vec; + self.is_locked = new_is_locked; } /// Overwrite the string with zeros. This is automatically called in the @@ -172,7 +182,9 @@ where { fn drop(&mut self) { self.zero_out(); - memlock::munlock(self.content.as_mut_ptr(), self.content.capacity()); + if self.is_locked { + memlock::munlock(self.content.as_mut_ptr(), self.content.capacity()); + } } } diff --git a/src/secure_utils.rs b/src/secure_utils.rs index c17c0ce..556cebd 100644 --- a/src/secure_utils.rs +++ b/src/secure_utils.rs @@ -2,8 +2,14 @@ pub mod memlock { extern crate libc; + /// Locks the given memory region into RAM. + /// + /// Returns `true` if the region was successfully locked. When `mlock` fails + /// the region is left unlocked and the matching [`munlock`] call must be + /// skipped, so callers should store the returned flag. #[allow(unused_variables)] - pub fn mlock(data: *mut T, count: usize) { + #[must_use] + pub fn mlock(data: *mut T, count: usize) -> bool { let byte_num = count * std::mem::size_of::(); // SAFETY: `cont` points to a valid allocation of at least `count * // size_of::()` bytes (guaranteed by callers passing pointers from @@ -12,12 +18,15 @@ pub mod memlock { #[cfg(not(miri))] // unsupported operation: can't call foreign function `mlock` on OS `linux unsafe { let ptr = data.cast::(); - libc::mlock(ptr, byte_num); + if libc::mlock(ptr, byte_num) != 0 { + return false; + } #[cfg(any(target_os = "freebsd", target_os = "dragonfly"))] libc::madvise(ptr, byte_num, libc::MADV_NOCORE); #[cfg(target_os = "linux")] libc::madvise(ptr, byte_num, libc::MADV_DONTDUMP); } + true } #[allow(unused_variables)] @@ -40,7 +49,10 @@ pub mod memlock { #[cfg(not(unix))] pub mod memlock { - pub fn mlock(_cont: *mut T, _count: usize) {} + #[must_use] + pub fn mlock(_cont: *mut T, _count: usize) -> bool { + false + } pub fn munlock(_cont: *mut T, _count: usize) {} } diff --git a/src/serde.rs b/src/serde.rs index 7eb7f05..5377873 100644 --- a/src/serde.rs +++ b/src/serde.rs @@ -14,7 +14,7 @@ struct BytesVisitor { impl Default for BytesVisitor { fn default() -> Self { Self { - phandom_data: Default::default(), + phandom_data: PhantomData, } } } @@ -33,10 +33,10 @@ where where E: de::Error, { - Self::Value::try_from(value.to_vec()).map_err(|error| { - serde::de::Error::custom(format!( - "cannot construct secure value from byte slice: {error}" - )) + Self::Value::try_from(value.to_vec()).map_err(|_| { + // Do not include the underlying error: some deserializers embed the + // (secret) input in their error messages. + serde::de::Error::custom("cannot construct secure value from byte slice") }) } @@ -44,10 +44,10 @@ where where E: de::Error, { - Self::Value::try_from(value).map_err(|error| { - serde::de::Error::custom(format!( - "cannot construct secure value from byte vector: {error}" - )) + Self::Value::try_from(value).map_err(|_| { + // Do not include the underlying error: some deserializers embed the + // (secret) input in their error messages. + serde::de::Error::custom("cannot construct secure value from byte vector") }) } @@ -61,10 +61,10 @@ where value.push(element); } - Self::Value::try_from(value).map_err(|error| { - serde::de::Error::custom(format!( - "cannot construct secure value from byte sequence: {error}" - )) + Self::Value::try_from(value).map_err(|_| { + // Do not include the underlying error: some deserializers embed the + // (secret) input in their error messages. + serde::de::Error::custom("cannot construct secure value from byte sequence") }) } } @@ -101,7 +101,7 @@ impl Serialize for SecureArray { where S: Serializer, { - serializer.serialize_bytes(self.content.borrow()) + serializer.serialize_bytes(self.unsecure()) } }