fix(security): handle few security issues

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,50 +19,60 @@ 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<T, const LENGTH: usize>
 where
     T: Copy + Zeroize,
 {
-    pub(crate) content: [T; LENGTH],
+    inner: SecureBox<[T; LENGTH]>,
 }
 
 impl<T, const LENGTH: usize> SecureArray<T, LENGTH>
 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()
     }
 
     /// 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<T: Copy + Zeroize, const LENGTH: usize> Clone for SecureArray<T, LENGTH> {
     fn clone(&self) -> Self {
-        Self::new(self.content)
+        Self {
+            inner: self.inner.clone(),
+        }
     }
 }
 
 impl<T: Copy + Zeroize + ConstantTimeEq, const LENGTH: usize> ConstantTimeEq
     for SecureArray<T, LENGTH>
 {
     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<U>>::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<T, const LENGTH: usize> Drop for SecureArray<T, LENGTH>
-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<u8, 5> {
+            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([

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<Box<T>>,
+    /// Whether `content` is currently `mlock`ed. If `mlock` failed, `munlock`
+    /// must be skipped.
+    is_locked: bool,
 }
 
 impl<T> SecureBox<T>
@@ -34,9 +37,10 @@ where
 {
     #[must_use]
     pub fn new(mut cont: Box<T>) -> 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::<T>() != 0 {

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")

src/secure_types/vec.rs

@@ -31,6 +31,9 @@ where
     T: Copy + Zeroize,
 {
     pub(crate) content: Vec<T>,
+    /// 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<T>) -> 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());
+        }
     }
 }
 

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<T>(data: *mut T, count: usize) {
+    #[must_use]
+    pub fn mlock<T>(data: *mut T, count: usize) -> bool {
         let byte_num = count * std::mem::size_of::<T>();
         // SAFETY: `cont` points to a valid allocation of at least `count *
         // size_of::<T>()` 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::c_void>();
-            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<T>(_cont: *mut T, _count: usize) {}
+    #[must_use]
+    pub fn mlock<T>(_cont: *mut T, _count: usize) -> bool {
+        false
+    }
 
     pub fn munlock<T>(_cont: *mut T, _count: usize) {}
 }

src/serde.rs

@@ -14,7 +14,7 @@ struct BytesVisitor<Value> {
 impl<Value> Default for BytesVisitor<Value> {
     fn default() -> Self {
         Self {
-            phandom_data: Default::default(),
+            phandom_data: PhantomData,
         }
     }
 }
@@ -33,21 +33,21 @@ 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")
         })
     }
 
     fn visit_byte_buf<E>(self, value: Vec<u8>) -> Result<Self::Value, E>
     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<const LENGTH: usize> Serialize for SecureArray<u8, LENGTH> {
     where
         S: Serializer,
     {
-        serializer.serialize_bytes(self.content.borrow())
+        serializer.serialize_bytes(self.unsecure())
     }
 }