From 96179806d2ed5e8818fe2cb9eba551fdbb92f32e Mon Sep 17 00:00:00 2001 From: Marek 'seqre' Grzelak Date: Sun, 24 May 2026 23:52:55 +0200 Subject: [PATCH 1/6] fix(serde): do not leak underlying deserialization error Some deserializing libraries embed the (secret) input in their error messages. Drop the `{error}` interpolation from the three byte deserialization paths so password fragments cannot leak via serde errors. Co-Authored-By: Claude Opus 4.7 --- src/serde.rs | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/src/serde.rs b/src/serde.rs index 7eb7f05..58b6c29 100644 --- a/src/serde.rs +++ b/src/serde.rs @@ -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,11 +44,8 @@ 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(|_| serde::de::Error::custom("cannot construct secure value from byte vector")) } fn visit_seq(self, mut seq: A) -> Result @@ -61,10 +58,8 @@ 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(|_| { + serde::de::Error::custom("cannot construct secure value from byte sequence") }) } } From 18922134c3c08faf1422df03c78a61f33838c858 Mon Sep 17 00:00:00 2001 From: Marek 'seqre' Grzelak Date: Sun, 24 May 2026 23:54:28 +0200 Subject: [PATCH 2/6] fix(memlock): skip munlock when mlock failed `mlock` now returns whether the region was actually locked (it can fail, e.g. on RLIMIT_MEMLOCK). Each secure type stores an `is_locked` flag and only calls the paired `munlock` when locking succeeded, avoiding munlock on a region this type never locked. Co-Authored-By: Claude Opus 4.7 --- src/secure_types/array.rs | 9 ++++++--- src/secure_types/boxed.rs | 10 ++++++++-- src/secure_types/string.rs | 4 +++- src/secure_types/vec.rs | 21 ++++++++++++++++----- src/secure_utils.rs | 18 +++++++++++++++--- 5 files changed, 48 insertions(+), 14 deletions(-) diff --git a/src/secure_types/array.rs b/src/secure_types/array.rs index 6bb2e34..973cef3 100644 --- a/src/secure_types/array.rs +++ b/src/secure_types/array.rs @@ -24,6 +24,7 @@ where T: Copy + Zeroize, { pub(crate) content: [T; LENGTH], + is_locked: bool, } impl SecureArray @@ -31,8 +32,8 @@ where T: Copy + Zeroize, { pub fn new(mut content: [T; LENGTH]) -> Self { - memlock::mlock(content.as_mut_ptr(), content.len()); - Self { content } + let is_locked = memlock::mlock(content.as_mut_ptr(), content.len()); + Self { content, is_locked } } /// Borrow the contents of the string. @@ -147,7 +148,9 @@ where { fn drop(&mut self) { self.zero_out(); - memlock::munlock(self.content.as_mut_ptr(), self.content.len()); + if self.is_locked { + memlock::munlock(self.content.as_mut_ptr(), self.content.len()); + } } } diff --git a/src/secure_types/boxed.rs b/src/secure_types/boxed.rs index 2be9c2b..f1e794c 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, } } @@ -141,7 +145,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..11acb7b 100644 --- a/src/secure_types/string.rs +++ b/src/secure_types/string.rs @@ -27,7 +27,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. diff --git a/src/secure_types/vec.rs b/src/secure_types/vec.rs index 9e7e869..bda1e15 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. @@ -76,13 +82,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 +181,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) {} } From bc81589df5505d50877dc58d5b36d28b3f5c1fd4 Mon Sep 17 00:00:00 2001 From: Marek 'seqre' Grzelak Date: Sun, 24 May 2026 23:56:06 +0200 Subject: [PATCH 3/6] fix(array): store contents on the heap via SecureBox The previous SecureArray stored `[T; LENGTH]` inline and mlocked the local before moving it into the returned value, so the lock applied to a stale address; any later move of the SecureArray relocated the bytes and left the lock behind. Wrap a `SecureBox<[T; LENGTH]>` instead so the data has a stable heap address and the mlock stays valid across moves. Co-Authored-By: Claude Opus 4.7 --- src/secure_types/array.rs | 58 ++++++++++++++++++++++----------------- src/serde.rs | 2 +- 2 files changed, 34 insertions(+), 26 deletions(-) diff --git a/src/secure_types/array.rs b/src/secure_types/array.rs index 973cef3..173162b 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,29 +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], - is_locked: bool, + inner: SecureBox<[T; LENGTH]>, } impl SecureArray where T: Copy + Zeroize, { - pub fn new(mut content: [T; LENGTH]) -> Self { - let is_locked = memlock::mlock(content.as_mut_ptr(), content.len()); - Self { content, is_locked } + #[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() } @@ -49,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(), + } } } @@ -63,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()) } } @@ -118,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) } } @@ -128,7 +137,7 @@ where T: Copy + Zeroize, { fn borrow(&self) -> &[T] { - self.content.borrow() + self.inner.unsecure().as_slice() } } @@ -137,20 +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(); - if self.is_locked { - memlock::munlock(self.content.as_mut_ptr(), self.content.len()); - } + self.inner.unsecure_mut().as_mut_slice() } } @@ -224,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 = vec![moved]; + assert_eq!(v[0].unsecure(), b"hello"); + } + #[test] fn test_comparison_zero_out_multibyte() { let data1 = SecureArray::from([ diff --git a/src/serde.rs b/src/serde.rs index 58b6c29..cd4b996 100644 --- a/src/serde.rs +++ b/src/serde.rs @@ -96,7 +96,7 @@ impl Serialize for SecureArray { where S: Serializer, { - serializer.serialize_bytes(self.content.borrow()) + serializer.serialize_bytes(self.unsecure()) } } From c73d4d2725d134352d2c45dc9eabe52e7729d917 Mon Sep 17 00:00:00 2001 From: Marek 'seqre' Grzelak Date: Sun, 24 May 2026 23:56:30 +0200 Subject: [PATCH 4/6] fix: add missing #[must_use] on unsecure_mut `unsecure_mut` returns a borrow of the secret; ignoring it is a no-op. Annotate it across SecureString, SecureVec, and SecureBox for parity with the already-annotated `unsecure`/`into_unsecure`. Co-Authored-By: Claude Opus 4.7 --- src/secure_types/boxed.rs | 1 + src/secure_types/string.rs | 1 + src/secure_types/vec.rs | 1 + 3 files changed, 3 insertions(+) diff --git a/src/secure_types/boxed.rs b/src/secure_types/boxed.rs index f1e794c..385c064 100644 --- a/src/secure_types/boxed.rs +++ b/src/secure_types/boxed.rs @@ -61,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() diff --git a/src/secure_types/string.rs b/src/secure_types/string.rs index 11acb7b..1da1e7c 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()) } diff --git a/src/secure_types/vec.rs b/src/secure_types/vec.rs index bda1e15..cefbc6b 100644 --- a/src/secure_types/vec.rs +++ b/src/secure_types/vec.rs @@ -59,6 +59,7 @@ where } /// Mutably borrow the contents of the string. + #[must_use] pub fn unsecure_mut(&mut self) -> &mut [T] { self.borrow_mut() } From 8b440f20389dbf6511c8d9395f9ded0dea3570c5 Mon Sep 17 00:00:00 2001 From: Marek 'seqre' Grzelak Date: Mon, 25 May 2026 18:53:07 +0200 Subject: [PATCH 5/6] style: resolve clippy pedantic warnings Use `PhantomData` directly instead of `Default::default()` (default_trait_access) and elide the `'de` lifetime on the SecureStringVisitor Visitor impl (elidable_lifetime_names). Co-Authored-By: Claude Opus 4.7 --- src/secure_types/array.rs | 2 +- src/secure_types/string.rs | 2 +- src/serde.rs | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/secure_types/array.rs b/src/secure_types/array.rs index 173162b..ff3737d 100644 --- a/src/secure_types/array.rs +++ b/src/secure_types/array.rs @@ -228,7 +228,7 @@ mod tests { SecureArray::from_str("hello").unwrap() } let moved = make(); - let v = vec![moved]; + let v = [moved]; assert_eq!(v[0].unsecure(), b"hello"); } diff --git a/src/secure_types/string.rs b/src/secure_types/string.rs index 1da1e7c..92803a3 100644 --- a/src/secure_types/string.rs +++ b/src/secure_types/string.rs @@ -101,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/serde.rs b/src/serde.rs index cd4b996..dc21408 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, } } } From 0dc5ad2819c9ec865918935fd7ed2fd4c907677a Mon Sep 17 00:00:00 2001 From: Marek 'seqre' Grzelak Date: Mon, 25 May 2026 19:27:28 +0200 Subject: [PATCH 6/6] docs(serde): note the no-leak rationale on each visitor method Co-Authored-By: Claude Opus 4.7 --- src/serde.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/serde.rs b/src/serde.rs index dc21408..5377873 100644 --- a/src/serde.rs +++ b/src/serde.rs @@ -44,8 +44,11 @@ where where E: de::Error, { - Self::Value::try_from(value) - .map_err(|_| serde::de::Error::custom("cannot construct secure value from byte vector")) + 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") + }) } fn visit_seq(self, mut seq: A) -> Result @@ -59,6 +62,8 @@ where } 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") }) }