From c52afa459546096bdc9de12c3fe11edb723c32bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garci=CC=81a?= Date: Mon, 20 Jan 2025 16:49:56 +0100 Subject: [PATCH 01/12] Add `mlock` and `memfd_secret` implementations --- Cargo.lock | 12 + crates/bitwarden-crypto/Cargo.toml | 7 + .../custom_slice/linux_memfd_secret.rs | 111 +++ .../implementation/custom_slice/mod.rs | 747 ++++++++++++++++++ .../implementation/custom_slice/rust.rs | 111 +++ .../src/store/backend/implementation/mod.rs | 12 +- 6 files changed, 998 insertions(+), 2 deletions(-) create mode 100644 crates/bitwarden-crypto/src/store/backend/implementation/custom_slice/linux_memfd_secret.rs create mode 100644 crates/bitwarden-crypto/src/store/backend/implementation/custom_slice/mod.rs create mode 100644 crates/bitwarden-crypto/src/store/backend/implementation/custom_slice/rust.rs diff --git a/Cargo.lock b/Cargo.lock index 4c61bf967..374c88db1 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -426,6 +426,7 @@ dependencies = [ "generic-array", "hkdf", "hmac", + "memsec", "num-bigint", "num-traits", "pbkdf2", @@ -2367,6 +2368,17 @@ dependencies = [ "zeroize", ] +[[package]] +name = "memsec" +version = "0.7.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c797b9d6bb23aab2fc369c65f871be49214f5c759af65bde26ffaaa2b646b492" +dependencies = [ + "getrandom", + "libc", + "windows-sys 0.45.0", +] + [[package]] name = "mime" version = "0.3.17" diff --git a/crates/bitwarden-crypto/Cargo.toml b/crates/bitwarden-crypto/Cargo.toml index 7bf326152..bec6334eb 100644 --- a/crates/bitwarden-crypto/Cargo.toml +++ b/crates/bitwarden-crypto/Cargo.toml @@ -50,6 +50,9 @@ wasm-bindgen = { workspace = true, optional = true } zeroize = { version = ">=1.7.0, <2.0", features = ["derive", "aarch64"] } zeroizing-alloc = ">=0.1.0, <0.2" +[target.'cfg(all(not(target_arch = "wasm32"), not(windows)))'.dependencies] +memsec = { version = "0.7.0", features = ["alloc_ext"] } + [dev-dependencies] criterion = "0.5.1" rand_chacha = "0.3.1" @@ -67,3 +70,7 @@ required-features = ["no-memory-hardening"] [lints] workspace = true + +[package.metadata.cargo-udeps.ignore] +# This is unused when using --all-features, as that disables memory-hardening +normal = ["memsec"] diff --git a/crates/bitwarden-crypto/src/store/backend/implementation/custom_slice/linux_memfd_secret.rs b/crates/bitwarden-crypto/src/store/backend/implementation/custom_slice/linux_memfd_secret.rs new file mode 100644 index 000000000..9c2bd6d61 --- /dev/null +++ b/crates/bitwarden-crypto/src/store/backend/implementation/custom_slice/linux_memfd_secret.rs @@ -0,0 +1,111 @@ +use std::{mem::MaybeUninit, ptr::NonNull, sync::OnceLock}; + +use super::{KeyId, SliceBackend, SliceLike}; + +// This is an in-memory key store that is protected by memfd_secret on Linux 5.14+. +// This should be secure against memory dumps from anything except a malicious kernel driver. +// Note that not all 5.14+ systems have support for memfd_secret enabled, so +// LinuxMemfdSecretKeyStore::new returns an Option. +pub(crate) type LinuxMemfdSecretBackend = SliceBackend; + +pub(crate) struct MemfdSecretImplKeyData { + ptr: std::ptr::NonNull<[u8]>, + capacity: usize, +} + +// For Send+Sync to be safe, we need to ensure that the memory is only accessed mutably from one +// thread. To do this, we have to make sure that any funcion in `MemfdSecretImplKeyData` that +// accesses the pointer mutably is defined as &mut self, and that the pointer is never copied or +// moved outside the struct. +unsafe impl Send for MemfdSecretImplKeyData {} +unsafe impl Sync for MemfdSecretImplKeyData {} + +impl Drop for MemfdSecretImplKeyData { + fn drop(&mut self) { + unsafe { + memsec::free_memfd_secret(self.ptr); + } + } +} + +impl SliceLike for MemfdSecretImplKeyData { + fn is_available() -> bool { + static IS_SUPPORTED: OnceLock = OnceLock::new(); + + *IS_SUPPORTED.get_or_init(|| unsafe { + let Some(ptr) = memsec::memfd_secret_sized(1) else { + return false; + }; + memsec::free_memfd_secret(ptr); + true + }) + } + + fn with_capacity(capacity: usize) -> Self { + let entry_size = std::mem::size_of::>(); + + unsafe { + let ptr: NonNull<[u8]> = memsec::memfd_secret_sized(capacity * entry_size) + .expect("memfd_secret_sized failed"); + + // Initialize the array with Nones using MaybeUninit + let uninit_slice: &mut [MaybeUninit<_>] = std::slice::from_raw_parts_mut( + ptr.as_ptr() as *mut MaybeUninit>, + capacity, + ); + for elem in uninit_slice { + elem.write(None); + } + + MemfdSecretImplKeyData { ptr, capacity } + } + } + + fn get_key_data(&self) -> &[Option<(Key, Key::KeyValue)>] { + let ptr = self.ptr.as_ptr() as *const Option<(Key, Key::KeyValue)>; + // SAFETY: The pointer is valid and points to a valid slice of the correct size. + // This function is &self so it only takes a immutable *const pointer. + unsafe { std::slice::from_raw_parts(ptr, self.capacity) } + } + + fn get_key_data_mut(&mut self) -> &mut [Option<(Key, Key::KeyValue)>] { + let ptr = self.ptr.as_ptr() as *mut Option<(Key, Key::KeyValue)>; + // SAFETY: The pointer is valid and points to a valid slice of the correct size. + // This function is &mut self so it can take a mutable *mut pointer. + unsafe { std::slice::from_raw_parts_mut(ptr, self.capacity) } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::store::backend::{ + implementation::custom_slice::tests::{TestKey, TestKeyValue}, + StoreBackend as _, + }; + + #[test] + fn test_resize() { + let mut store = LinuxMemfdSecretBackend::::with_capacity(1).unwrap(); + + for (idx, key) in [ + TestKey::A, + TestKey::B(10), + TestKey::C, + TestKey::B(7), + TestKey::A, + TestKey::C, + ] + .into_iter() + .enumerate() + { + store.upsert(key, TestKeyValue::new(idx)); + } + + assert_eq!(store.get(TestKey::A), Some(&TestKeyValue::new(4))); + assert_eq!(store.get(TestKey::B(10)), Some(&TestKeyValue::new(1))); + assert_eq!(store.get(TestKey::C), Some(&TestKeyValue::new(5))); + assert_eq!(store.get(TestKey::B(7)), Some(&TestKeyValue::new(3))); + assert_eq!(store.get(TestKey::B(20)), None); + } +} diff --git a/crates/bitwarden-crypto/src/store/backend/implementation/custom_slice/mod.rs b/crates/bitwarden-crypto/src/store/backend/implementation/custom_slice/mod.rs new file mode 100644 index 000000000..f816eb481 --- /dev/null +++ b/crates/bitwarden-crypto/src/store/backend/implementation/custom_slice/mod.rs @@ -0,0 +1,747 @@ +use std::marker::PhantomData; + +use zeroize::ZeroizeOnDrop; + +use super::StoreBackend; +use crate::KeyId; + +#[cfg(all(target_os = "linux", not(feature = "no-memory-hardening")))] +pub(crate) mod linux_memfd_secret; +pub(crate) mod rust; + +/// This trait represents some data stored sequentially in memory that can be interpreted as a +/// slice. +/// Implementations of this trait should ensure that the initialized data is protected +/// as much as possible. The data is already Zeroized on Drop, so implementations should +/// only need to worry about removing any protections they've added, or releasing any resources. +#[allow(drop_bounds)] +pub(crate) trait SliceLike: Send + Sync + Sized + Drop { + /// Check if the data store is available on this platform. + fn is_available() -> bool; + + /// Initialize a new data store with the given capacity. + /// The data MUST be initialized to all None values, and + /// it's capacity must be equal or greater than the provided value. + fn with_capacity(capacity: usize) -> Self; + + /// Return an immutable slice of the data. It must return the full allocated capacity, with no + /// uninitialized values. + fn get_key_data(&self) -> &[Option<(Key, Key::KeyValue)>]; + + /// Return an mutable slice of the data. It must return the full allocated capacity, with no + /// uninitialized values. + fn get_key_data_mut(&mut self) -> &mut [Option<(Key, Key::KeyValue)>]; +} + +/// This represents a key store over an arbitrary fixed size slice. +/// This is meant to abstract over the different ways to store keys in memory, whether we're +/// using a Box<[u8]> or a NonNull, regardless of if that memory was allocated by Rust or not. +/// +/// Internally this is represented as a slice of `Option<(Key, Key::KeyValue)>` +/// and elements are sorted based on Key for performance. In essence, this is almost a homegrown +/// `HashMap`. +/// +/// The reason why we're not using a Rust collection like `Vec` or `HashMap` is that those types +/// expect their memory to be allocated by Rust, and they will deallocate/reallocate it as needed. +/// That will not work for our usecases where we want to have control over allocations/deallocations +/// and where some of our strategies rely on using system-allocated secure memory for the storage, +/// like the Linux-only `memfd_secret` API. +pub(crate) struct SliceBackend> { + // This represents the number of elements in the container, it's always less than or equal to + // the length of `data`. + length: usize, + + // This represents the maximum number of elements that can be stored in the container. + // This is always equal to the length of `data`, but we store it to avoid recomputing it. + capacity: usize, + + // This is the actual data that stores the keys, optional as we can have it not yet + // uninitialized + slice: Option, + + _key: PhantomData, +} + +impl> ZeroizeOnDrop for SliceBackend {} + +impl> Drop for SliceBackend { + fn drop(&mut self) { + self.clear(); + } +} + +impl> StoreBackend for SliceBackend { + fn upsert(&mut self, key_ref: Key, key: Key::KeyValue) { + match self.find_by_key_ref(&key_ref) { + Ok(idx) => { + // Key already exists, we just need to replace the value + let slice = self.get_key_data_mut(); + slice[idx] = Some((key_ref, key)); + } + Err(idx) => { + // Make sure that we have enough capacity, and resize if needed + self.ensure_capacity(1); + + let len = self.length; + let slice = self.get_key_data_mut(); + if idx < len { + // If we're not right at the end, we have to shift all the following elements + // one position to the right + slice[idx..=len].rotate_right(1); + } + slice[idx] = Some((key_ref, key)); + self.length += 1; + } + } + } + + fn get(&self, key_ref: Key) -> Option<&Key::KeyValue> { + self.find_by_key_ref(&key_ref) + .ok() + .and_then(|idx| self.get_key_data().get(idx)) + .and_then(|f| f.as_ref().map(|f| &f.1)) + } + + fn remove(&mut self, key_ref: Key) { + if let Ok(idx) = self.find_by_key_ref(&key_ref) { + let len = self.length; + let slice = self.get_key_data_mut(); + slice[idx] = None; + slice[idx..len].rotate_left(1); + self.length -= 1; + } + } + + fn clear(&mut self) { + let len = self.length; + self.get_key_data_mut()[0..len].fill_with(|| None); + self.length = 0; + } + + fn retain(&mut self, f: fn(Key) -> bool) { + let len = self.length; + let slice = self.get_key_data_mut(); + + let mut removed_elements = 0; + + for value in slice.iter_mut().take(len) { + let key = value + .as_ref() + .map(|e| e.0) + .expect("Values in a slice are always Some"); + + if !f(key) { + *value = None; + removed_elements += 1; + } + } + + // If we haven't removed any elements, we don't need to compact the slice + if removed_elements == 0 { + return; + } + + // Remove all the None values from the middle of the slice + + for idx in 0..len { + if slice[idx].is_none() { + slice[idx..len].rotate_left(1); + } + } + + self.length -= removed_elements; + } +} + +impl> SliceBackend { + pub(crate) fn new() -> Option { + Self::with_capacity(0) + } + + pub(crate) fn with_capacity(capacity: usize) -> Option { + if !Data::is_available() { + return None; + } + + // If the capacity is 0, we don't need to allocate any memory. + // This allows us to initialize the container lazily. + let slice = (capacity != 0).then(|| Data::with_capacity(capacity)); + Some(Self { + length: 0, + capacity, + slice, + _key: PhantomData, + }) + } + + /// Check if the container has enough capacity to store `new_elements` more elements. + /// If the result is Ok, the container has enough capacity. + /// If it's Err, the container needs to be resized. + /// The error value returns a suggested new capacity. + fn check_capacity(&self, new_elements: usize) -> Result<(), usize> { + let new_size = self.length + new_elements; + + // We still have enough capacity + if new_size <= self.capacity { + Ok(()) + + // This is the first allocation + } else if self.capacity == 0 { + const PAGE_SIZE: usize = 4096; + let entry_size = std::mem::size_of::>(); + + // We're using mlock APIs to protect the memory, which lock at the page level. + // To avoid wasting memory, we want to allocate at least a page. + let entries_per_page = PAGE_SIZE / entry_size; + Err(entries_per_page) + + // We need to resize the container + } else { + // We want to increase the capacity by a multiple to be mostly aligned with page size, + // we also need to make sure that we have enough space for the new elements, so we round + // up + let increase_factor = usize::div_ceil(new_size, self.capacity); + Err(self.capacity * increase_factor) + } + } + + fn ensure_capacity(&mut self, new_elements: usize) { + if let Err(new_capacity) = self.check_capacity(new_elements) { + // Create a new store with the correct capacity and replace self with it + let mut new_self = + Self::with_capacity(new_capacity).expect("Could not allocate new store"); + new_self.copy_from(self); + *self = new_self; + } + } + + // These two are just helper functions to avoid having to deal with the optional Data + // When Data is None we just return empty slices, which don't allow any operations + fn get_key_data(&self) -> &[Option<(Key, Key::KeyValue)>] { + self.slice.as_ref().map(|d| d.get_key_data()).unwrap_or(&[]) + } + fn get_key_data_mut(&mut self) -> &mut [Option<(Key, Key::KeyValue)>] { + self.slice + .as_mut() + .map(|d| d.get_key_data_mut()) + .unwrap_or(&mut []) + } + + fn find_by_key_ref(&self, key_ref: &Key) -> Result { + // Because we know all the None's are at the end and all the Some values are at the + // beginning, we only need to search for the key in the first `size` elements. + let slice = &self.get_key_data()[..self.length]; + + // This structure is almost always used for reads instead of writes, so we can use a binary + // search to optimize for the read case. + slice.binary_search_by(|k| { + debug_assert!( + k.is_some(), + "We should never have a None value in the middle of the slice" + ); + + match k { + Some((k, _)) => k.cmp(key_ref), + None => std::cmp::Ordering::Greater, + } + }) + } + + pub(crate) fn copy_from(&mut self, other: &mut Self) -> bool { + if other.capacity > self.capacity { + return false; + } + + // Empty the current container + self.clear(); + + let new_length = other.length; + + // Move the data from the other container + let this = self.get_key_data_mut(); + let that = other.get_key_data_mut(); + for idx in 0..new_length { + std::mem::swap(&mut this[idx], &mut that[idx]); + } + + // Update the length + self.length = new_length; + + true + } +} + +#[cfg(test)] +pub(crate) mod tests { + use zeroize::Zeroize; + + use super::{rust::RustBackend, *}; + use crate::{CryptoKey, KeyId}; + + #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] + pub enum TestKey { + A, + B(u8), + C, + } + #[derive(Debug, Clone, Hash, Eq, PartialEq, Ord, PartialOrd)] + pub struct TestKeyValue([u8; 16]); + impl zeroize::ZeroizeOnDrop for TestKeyValue {} + impl CryptoKey for TestKeyValue {} + impl TestKeyValue { + pub fn new(value: usize) -> Self { + // Just fill the array with some values + let mut key = [0; 16]; + key[0..8].copy_from_slice(&value.to_le_bytes()); + key[8..16].copy_from_slice(&value.to_be_bytes()); + Self(key) + } + } + + impl Drop for TestKeyValue { + fn drop(&mut self) { + self.0.as_mut().zeroize(); + } + } + + impl KeyId for TestKey { + type KeyValue = TestKeyValue; + + fn is_local(&self) -> bool { + false + } + } + + #[test] + fn test_slice_container_insertion() { + let mut container = RustBackend::::with_capacity(5).unwrap(); + + assert_eq!(container.get_key_data(), [None, None, None, None, None]); + + // Insert one key, which should be at the beginning + container.upsert(TestKey::B(10), TestKeyValue::new(110)); + assert_eq!( + container.get_key_data(), + [ + Some((TestKey::B(10), TestKeyValue::new(110))), + None, + None, + None, + None + ] + ); + + // Insert a key that should be right after the first one + container.upsert(TestKey::C, TestKeyValue::new(1000)); + assert_eq!( + container.get_key_data(), + [ + Some((TestKey::B(10), TestKeyValue::new(110))), + Some((TestKey::C, TestKeyValue::new(1000))), + None, + None, + None + ] + ); + + // Insert a key in the middle + container.upsert(TestKey::B(20), TestKeyValue::new(210)); + assert_eq!( + container.get_key_data(), + [ + Some((TestKey::B(10), TestKeyValue::new(110))), + Some((TestKey::B(20), TestKeyValue::new(210))), + Some((TestKey::C, TestKeyValue::new(1000))), + None, + None + ] + ); + + // Insert a key right at the start + container.upsert(TestKey::A, TestKeyValue::new(0)); + assert_eq!( + container.get_key_data(), + [ + Some((TestKey::A, TestKeyValue::new(0))), + Some((TestKey::B(10), TestKeyValue::new(110))), + Some((TestKey::B(20), TestKeyValue::new(210))), + Some((TestKey::C, TestKeyValue::new(1000))), + None + ] + ); + + // Insert a key in the middle, which fills the container + container.upsert(TestKey::B(30), TestKeyValue::new(310)); + assert_eq!( + container.get_key_data(), + [ + Some((TestKey::A, TestKeyValue::new(0))), + Some((TestKey::B(10), TestKeyValue::new(110))), + Some((TestKey::B(20), TestKeyValue::new(210))), + Some((TestKey::B(30), TestKeyValue::new(310))), + Some((TestKey::C, TestKeyValue::new(1000))), + ] + ); + + // Replacing an existing value at the start + container.upsert(TestKey::A, TestKeyValue::new(1)); + assert_eq!( + container.get_key_data(), + [ + Some((TestKey::A, TestKeyValue::new(1))), + Some((TestKey::B(10), TestKeyValue::new(110))), + Some((TestKey::B(20), TestKeyValue::new(210))), + Some((TestKey::B(30), TestKeyValue::new(310))), + Some((TestKey::C, TestKeyValue::new(1000))), + ] + ); + + // Replacing an existing value at the middle + container.upsert(TestKey::B(20), TestKeyValue::new(211)); + assert_eq!( + container.get_key_data(), + [ + Some((TestKey::A, TestKeyValue::new(1))), + Some((TestKey::B(10), TestKeyValue::new(110))), + Some((TestKey::B(20), TestKeyValue::new(211))), + Some((TestKey::B(30), TestKeyValue::new(310))), + Some((TestKey::C, TestKeyValue::new(1000))), + ] + ); + + // Replacing an existing value at the end + container.upsert(TestKey::C, TestKeyValue::new(1001)); + assert_eq!( + container.get_key_data(), + [ + Some((TestKey::A, TestKeyValue::new(1))), + Some((TestKey::B(10), TestKeyValue::new(110))), + Some((TestKey::B(20), TestKeyValue::new(211))), + Some((TestKey::B(30), TestKeyValue::new(310))), + Some((TestKey::C, TestKeyValue::new(1001))), + ] + ); + } + + #[test] + fn test_slice_container_get() { + let mut container = RustBackend::::with_capacity(5).unwrap(); + + for (key, value) in [ + (TestKey::A, TestKeyValue::new(1)), + (TestKey::B(10), TestKeyValue::new(110)), + (TestKey::C, TestKeyValue::new(1000)), + ] { + container.upsert(key, value); + } + + assert_eq!(container.get(TestKey::A), Some(&TestKeyValue::new(1))); + assert_eq!(container.get(TestKey::B(10)), Some(&TestKeyValue::new(110))); + assert_eq!(container.get(TestKey::B(20)), None); + assert_eq!(container.get(TestKey::C), Some(&TestKeyValue::new(1000))); + } + + #[test] + fn test_slice_container_clear() { + let mut container = RustBackend::::with_capacity(5).unwrap(); + + for (key, value) in [ + (TestKey::A, TestKeyValue::new(1)), + (TestKey::B(10), TestKeyValue::new(110)), + (TestKey::B(20), TestKeyValue::new(210)), + (TestKey::B(30), TestKeyValue::new(310)), + (TestKey::C, TestKeyValue::new(1000)), + ] { + container.upsert(key, value); + } + + assert_eq!( + container.get_key_data(), + [ + Some((TestKey::A, TestKeyValue::new(1))), + Some((TestKey::B(10), TestKeyValue::new(110))), + Some((TestKey::B(20), TestKeyValue::new(210))), + Some((TestKey::B(30), TestKeyValue::new(310))), + Some((TestKey::C, TestKeyValue::new(1000))), + ] + ); + + container.clear(); + + assert_eq!(container.get_key_data(), [None, None, None, None, None]); + } + + #[test] + fn test_slice_container_ensure_capacity() { + let mut container = RustBackend::::with_capacity(5).unwrap(); + + assert_eq!(container.capacity, 5); + assert_eq!(container.length, 0); + + assert_eq!(container.check_capacity(0), Ok(())); + assert_eq!(container.check_capacity(6), Err(10)); + assert_eq!(container.check_capacity(10), Err(10)); + assert_eq!(container.check_capacity(11), Err(15)); + assert_eq!(container.check_capacity(51), Err(55)); + + for (key, value) in [ + (TestKey::A, TestKeyValue::new(1)), + (TestKey::B(10), TestKeyValue::new(110)), + (TestKey::B(20), TestKeyValue::new(210)), + (TestKey::B(30), TestKeyValue::new(310)), + (TestKey::C, TestKeyValue::new(1000)), + ] { + container.upsert(key, value); + } + + assert_eq!(container.check_capacity(0), Ok(())); + assert_eq!(container.check_capacity(6), Err(15)); + assert_eq!(container.check_capacity(10), Err(15)); + assert_eq!(container.check_capacity(11), Err(20)); + assert_eq!(container.check_capacity(51), Err(60)); + } + + #[test] + fn test_slice_container_removal() { + let mut container = RustBackend::::with_capacity(5).unwrap(); + + for (key, value) in [ + (TestKey::A, TestKeyValue::new(1)), + (TestKey::B(10), TestKeyValue::new(110)), + (TestKey::B(20), TestKeyValue::new(210)), + (TestKey::B(30), TestKeyValue::new(310)), + (TestKey::C, TestKeyValue::new(1000)), + ] { + container.upsert(key, value); + } + + // Remove the last element + container.remove(TestKey::C); + assert_eq!( + container.get_key_data(), + [ + Some((TestKey::A, TestKeyValue::new(1))), + Some((TestKey::B(10), TestKeyValue::new(110))), + Some((TestKey::B(20), TestKeyValue::new(210))), + Some((TestKey::B(30), TestKeyValue::new(310))), + None, + ] + ); + + // Remove the first element + container.remove(TestKey::A); + assert_eq!( + container.get_key_data(), + [ + Some((TestKey::B(10), TestKeyValue::new(110))), + Some((TestKey::B(20), TestKeyValue::new(210))), + Some((TestKey::B(30), TestKeyValue::new(310))), + None, + None + ] + ); + + // Remove a non-existing element + container.remove(TestKey::A); + assert_eq!( + container.get_key_data(), + [ + Some((TestKey::B(10), TestKeyValue::new(110))), + Some((TestKey::B(20), TestKeyValue::new(210))), + Some((TestKey::B(30), TestKeyValue::new(310))), + None, + None + ] + ); + + // Remove an element in the middle + container.remove(TestKey::B(20)); + assert_eq!( + container.get_key_data(), + [ + Some((TestKey::B(10), TestKeyValue::new(110))), + Some((TestKey::B(30), TestKeyValue::new(310))), + None, + None, + None + ] + ); + + // Remove all the remaining elements + container.remove(TestKey::B(30)); + assert_eq!( + container.get_key_data(), + [ + Some((TestKey::B(10), TestKeyValue::new(110))), + None, + None, + None, + None + ] + ); + container.remove(TestKey::B(10)); + assert_eq!(container.get_key_data(), [None, None, None, None, None]); + + // Remove from an empty container + container.remove(TestKey::B(10)); + assert_eq!(container.get_key_data(), [None, None, None, None, None]); + } + + #[test] + fn test_slice_container_retain_removes_one() { + let mut container = RustBackend::::with_capacity(5).unwrap(); + + for (key, value) in [ + (TestKey::A, TestKeyValue::new(1)), + (TestKey::B(10), TestKeyValue::new(110)), + (TestKey::B(20), TestKeyValue::new(210)), + (TestKey::B(30), TestKeyValue::new(310)), + (TestKey::C, TestKeyValue::new(1000)), + ] { + container.upsert(key, value); + } + + // Remove the last element + container.retain(|k| k != TestKey::C); + assert_eq!( + container.get_key_data(), + [ + Some((TestKey::A, TestKeyValue::new(1))), + Some((TestKey::B(10), TestKeyValue::new(110))), + Some((TestKey::B(20), TestKeyValue::new(210))), + Some((TestKey::B(30), TestKeyValue::new(310))), + None, + ] + ); + + // Remove the first element + container.retain(|k| k != TestKey::A); + assert_eq!( + container.get_key_data(), + [ + Some((TestKey::B(10), TestKeyValue::new(110))), + Some((TestKey::B(20), TestKeyValue::new(210))), + Some((TestKey::B(30), TestKeyValue::new(310))), + None, + None + ] + ); + + // Remove a non-existing element + container.retain(|k| k != TestKey::A); + assert_eq!( + container.get_key_data(), + [ + Some((TestKey::B(10), TestKeyValue::new(110))), + Some((TestKey::B(20), TestKeyValue::new(210))), + Some((TestKey::B(30), TestKeyValue::new(310))), + None, + None + ] + ); + + // Remove an element in the middle + container.retain(|k| k != TestKey::B(20)); + assert_eq!( + container.get_key_data(), + [ + Some((TestKey::B(10), TestKeyValue::new(110))), + Some((TestKey::B(30), TestKeyValue::new(310))), + None, + None, + None + ] + ); + + // Remove all the remaining elements + container.retain(|k| k != TestKey::B(30)); + assert_eq!( + container.get_key_data(), + [ + Some((TestKey::B(10), TestKeyValue::new(110))), + None, + None, + None, + None + ] + ); + container.retain(|k| k != TestKey::B(10)); + assert_eq!(container.get_key_data(), [None, None, None, None, None]); + + // Remove from an empty container + container.retain(|k| k != TestKey::B(10)); + assert_eq!(container.get_key_data(), [None, None, None, None, None]); + } + + #[test] + fn test_slice_container_retain_removes_none() { + let mut container = RustBackend::::with_capacity(5).unwrap(); + + for (key, value) in [ + (TestKey::A, TestKeyValue::new(1)), + (TestKey::B(10), TestKeyValue::new(110)), + (TestKey::B(20), TestKeyValue::new(210)), + (TestKey::B(30), TestKeyValue::new(310)), + (TestKey::C, TestKeyValue::new(1000)), + ] { + container.upsert(key, value); + } + + container.retain(|_k| true); + assert_eq!( + container.get_key_data(), + [ + Some((TestKey::A, TestKeyValue::new(1))), + Some((TestKey::B(10), TestKeyValue::new(110))), + Some((TestKey::B(20), TestKeyValue::new(210))), + Some((TestKey::B(30), TestKeyValue::new(310))), + Some((TestKey::C, TestKeyValue::new(1000))), + ] + ); + } + + #[test] + fn test_slice_container_retain_removes_some() { + let mut container = RustBackend::::with_capacity(5).unwrap(); + + for (key, value) in [ + (TestKey::A, TestKeyValue::new(1)), + (TestKey::B(10), TestKeyValue::new(110)), + (TestKey::B(20), TestKeyValue::new(210)), + (TestKey::B(30), TestKeyValue::new(310)), + (TestKey::C, TestKeyValue::new(1000)), + ] { + container.upsert(key, value); + } + + container.retain(|k| matches!(k, TestKey::A | TestKey::B(20) | TestKey::C)); + assert_eq!( + container.get_key_data(), + [ + Some((TestKey::A, TestKeyValue::new(1))), + Some((TestKey::B(20), TestKeyValue::new(210))), + Some((TestKey::C, TestKeyValue::new(1000))), + None, + None, + ] + ); + } + + #[test] + fn test_slice_container_retain_removes_all() { + let mut container = RustBackend::::with_capacity(5).unwrap(); + + for (key, value) in [ + (TestKey::A, TestKeyValue::new(1)), + (TestKey::B(10), TestKeyValue::new(110)), + (TestKey::B(20), TestKeyValue::new(210)), + (TestKey::B(30), TestKeyValue::new(310)), + (TestKey::C, TestKeyValue::new(1000)), + ] { + container.upsert(key, value); + } + + container.retain(|_k| false); + assert_eq!(container.get_key_data(), [None, None, None, None, None]); + } +} diff --git a/crates/bitwarden-crypto/src/store/backend/implementation/custom_slice/rust.rs b/crates/bitwarden-crypto/src/store/backend/implementation/custom_slice/rust.rs new file mode 100644 index 000000000..7f4415fe5 --- /dev/null +++ b/crates/bitwarden-crypto/src/store/backend/implementation/custom_slice/rust.rs @@ -0,0 +1,111 @@ +use super::{KeyId, SliceBackend, SliceLike}; + +// This is a basic in-memory key store for the cases where we don't have a secure key store +// available. We still make use mlock to protect the memory from being swapped to disk, and we +// zeroize the values when dropped. +pub(crate) type RustBackend = SliceBackend>; + +pub(crate) struct RustBackendImpl { + #[allow(clippy::type_complexity)] + data: Box<[Option<(Key, Key::KeyValue)>]>, +} + +impl Drop for RustBackendImpl { + fn drop(&mut self) { + munlock_data(self.data.as_mut()); + } +} + +impl SliceLike for RustBackendImpl { + fn is_available() -> bool { + true + } + + fn with_capacity(capacity: usize) -> Self { + #[allow(unused_mut)] + let mut data: Box<_> = std::iter::repeat_with(|| None).take(capacity).collect(); + mlock_data(data.as_mut()); + RustBackendImpl { data } + } + + fn get_key_data(&self) -> &[Option<(Key, Key::KeyValue)>] { + self.data.as_ref() + } + + fn get_key_data_mut(&mut self) -> &mut [Option<(Key, Key::KeyValue)>] { + self.data.as_mut() + } +} + +#[allow(unused_variables)] +fn mlock_data(data: &mut [T]) { + #[cfg(all( + not(target_arch = "wasm32"), + not(windows), + not(feature = "no-memory-hardening") + ))] + { + unsafe { + memsec::mlock(data.as_mut_ptr() as *mut u8, std::mem::size_of_val(data)); + } + } +} + +#[allow(unused_variables)] +fn munlock_data(data: &mut [T]) { + #[cfg(all( + not(target_arch = "wasm32"), + not(windows), + not(feature = "no-memory-hardening") + ))] + { + use std::mem::MaybeUninit; + unsafe { + memsec::munlock(data.as_mut_ptr() as *mut u8, std::mem::size_of_val(data)); + + // Note: munlock is zeroing the memory, which leaves the data in an undefined + // state, so we set it to None/Default again to avoid UB in the Drop implementation. + let uninit_slice: &mut [MaybeUninit<_>] = std::slice::from_raw_parts_mut( + data.as_mut_ptr() as *mut MaybeUninit, + data.len(), + ); + for elem in uninit_slice { + elem.write(T::default()); + } + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::store::backend::{ + implementation::custom_slice::tests::{TestKey, TestKeyValue}, + StoreBackend as _, + }; + + #[test] + fn test_resize() { + let mut store = RustBackend::::with_capacity(1).unwrap(); + + for (idx, key) in [ + TestKey::A, + TestKey::B(10), + TestKey::C, + TestKey::B(7), + TestKey::A, + TestKey::C, + ] + .into_iter() + .enumerate() + { + store.upsert(key, TestKeyValue::new(idx)); + } + + assert_eq!(store.get(TestKey::A), Some(&TestKeyValue::new(4))); + assert_eq!(store.get(TestKey::B(10)), Some(&TestKeyValue::new(1))); + assert_eq!(store.get(TestKey::C), Some(&TestKeyValue::new(5))); + assert_eq!(store.get(TestKey::B(7)), Some(&TestKeyValue::new(3))); + assert_eq!(store.get(TestKey::B(20)), None); + } +} diff --git a/crates/bitwarden-crypto/src/store/backend/implementation/mod.rs b/crates/bitwarden-crypto/src/store/backend/implementation/mod.rs index 5ae4f8ef6..07d932278 100644 --- a/crates/bitwarden-crypto/src/store/backend/implementation/mod.rs +++ b/crates/bitwarden-crypto/src/store/backend/implementation/mod.rs @@ -1,11 +1,19 @@ use super::StoreBackend; use crate::store::KeyId; -mod basic; +mod custom_slice; /// Initializes a key store backend with the best available implementation for the current platform pub fn create_store() -> Box> { - Box::new(basic::BasicBackend::::new()) + #[cfg(all(target_os = "linux", not(feature = "no-memory-hardening")))] + if let Some(key_store) = custom_slice::linux_memfd_secret::LinuxMemfdSecretBackend::::new() + { + return Box::new(key_store); + } + + Box::new( + custom_slice::rust::RustBackend::new().expect("RustKeyStore should always be available"), + ) } #[cfg(test)] From c2e8bce073e52473f12106a3b2bd56eaf0c636a1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garci=CC=81a?= Date: Thu, 1 May 2025 20:49:28 +0200 Subject: [PATCH 02/12] Try alternative allocator-based backends --- Cargo.lock | 27 +- crates/bitwarden-crypto/Cargo.toml | 2 + .../src/store/backend/implementation/basic.rs | 7 + .../custom_alloc/linux_memfd_secret.rs | 57 ++ .../implementation/custom_alloc/malloc.rs | 75 ++ .../implementation/custom_alloc/mod.rs | 64 ++ .../custom_slice/linux_memfd_secret.rs | 111 --- .../implementation/custom_slice/mod.rs | 747 ------------------ .../implementation/custom_slice/rust.rs | 111 --- .../src/store/backend/implementation/mod.rs | 137 +++- .../bitwarden-crypto/src/store/backend/mod.rs | 5 + 11 files changed, 362 insertions(+), 981 deletions(-) create mode 100644 crates/bitwarden-crypto/src/store/backend/implementation/custom_alloc/linux_memfd_secret.rs create mode 100644 crates/bitwarden-crypto/src/store/backend/implementation/custom_alloc/malloc.rs create mode 100644 crates/bitwarden-crypto/src/store/backend/implementation/custom_alloc/mod.rs delete mode 100644 crates/bitwarden-crypto/src/store/backend/implementation/custom_slice/linux_memfd_secret.rs delete mode 100644 crates/bitwarden-crypto/src/store/backend/implementation/custom_slice/mod.rs delete mode 100644 crates/bitwarden-crypto/src/store/backend/implementation/custom_slice/rust.rs diff --git a/Cargo.lock b/Cargo.lock index fa96d2ad2..ab45a31c3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -62,6 +62,12 @@ dependencies = [ "memchr", ] +[[package]] +name = "allocator-api2" +version = "0.2.21" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "683d7910e743518b0e34f1186f92494becacb047c7b6bf616c96772180fef923" + [[package]] name = "android-tzdata" version = "0.1.1" @@ -419,6 +425,7 @@ name = "bitwarden-crypto" version = "1.0.0" dependencies = [ "aes", + "allocator-api2", "argon2", "base64", "bitwarden-error", @@ -426,6 +433,7 @@ dependencies = [ "chacha20poly1305", "criterion", "generic-array", + "hashbrown 0.15.3", "hkdf", "hmac", "memsec", @@ -1572,6 +1580,12 @@ version = "1.0.7" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "3f9eec918d3f24069decb9af1554cad7c880e2da24a9afd88aca000531ab82c1" +[[package]] +name = "foldhash" +version = "0.1.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d9c4f5dac5e15c24eb999c26181a6ca40b39fe946cbe4c263c7209467bc83af2" + [[package]] name = "form_urlencoded" version = "1.2.1" @@ -1822,9 +1836,14 @@ checksum = "e5274423e17b7c9fc20b6e7e208532f9b19825d82dfd615708b70edd83df41f1" [[package]] name = "hashbrown" -version = "0.15.2" +version = "0.15.3" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "bf151400ff0baff5465007dd2f3e717f3fe502074ca563069ce3a6629d07b289" +checksum = "84b26c544d002229e640969970a2e74021aadf6e2f96372b9c58eff97de08eb3" +dependencies = [ + "allocator-api2", + "equivalent", + "foldhash", +] [[package]] name = "heck" @@ -2181,7 +2200,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "cea70ddb795996207ad57735b50c5982d8844f38ba9ee5f1aedcfb708a2aa11e" dependencies = [ "equivalent", - "hashbrown 0.15.2", + "hashbrown 0.15.3", "serde", ] @@ -2413,7 +2432,7 @@ version = "0.7.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "c797b9d6bb23aab2fc369c65f871be49214f5c759af65bde26ffaaa2b646b492" dependencies = [ - "getrandom", + "getrandom 0.2.16", "libc", "windows-sys 0.45.0", ] diff --git a/crates/bitwarden-crypto/Cargo.toml b/crates/bitwarden-crypto/Cargo.toml index e922555d7..e7aa49c69 100644 --- a/crates/bitwarden-crypto/Cargo.toml +++ b/crates/bitwarden-crypto/Cargo.toml @@ -23,6 +23,7 @@ no-memory-hardening = [] # Disable memory hardening features [dependencies] aes = { version = ">=0.8.2, <0.9", features = ["zeroize"] } +allocator-api2 = ">=0.2.21, <0.3" argon2 = { version = ">=0.5.0, <0.6", features = [ "std", "zeroize", @@ -32,6 +33,7 @@ bitwarden-error = { workspace = true } cbc = { version = ">=0.1.2, <0.2", features = ["alloc", "zeroize"] } chacha20poly1305 = { version = "0.10.1" } generic-array = { version = ">=0.14.7, <1.0", features = ["zeroize"] } +hashbrown = ">=0.15.3, <0.16" hkdf = ">=0.12.3, <0.13" hmac = ">=0.12.1, <0.13" num-bigint = ">=0.4, <0.5" diff --git a/crates/bitwarden-crypto/src/store/backend/implementation/basic.rs b/crates/bitwarden-crypto/src/store/backend/implementation/basic.rs index 3db4b2db3..d30a5d60d 100644 --- a/crates/bitwarden-crypto/src/store/backend/implementation/basic.rs +++ b/crates/bitwarden-crypto/src/store/backend/implementation/basic.rs @@ -47,3 +47,10 @@ impl Drop for BasicBackend { self.clear(); } } + +#[cfg(test)] +impl super::super::StoreBackendDebug for BasicBackend { + fn elements(&self) -> Vec<(Key, &Key::KeyValue)> { + self.keys.iter().map(|(k, v)| (*k, v)).collect() + } +} diff --git a/crates/bitwarden-crypto/src/store/backend/implementation/custom_alloc/linux_memfd_secret.rs b/crates/bitwarden-crypto/src/store/backend/implementation/custom_alloc/linux_memfd_secret.rs new file mode 100644 index 000000000..c5b9e8f29 --- /dev/null +++ b/crates/bitwarden-crypto/src/store/backend/implementation/custom_alloc/linux_memfd_secret.rs @@ -0,0 +1,57 @@ +use std::{alloc::Layout, ptr::NonNull, sync::OnceLock}; + +use allocator_api2::alloc::{AllocError, Allocator}; + +pub(crate) struct LinuxMemfdSecretAlloc; + +impl LinuxMemfdSecretAlloc { + pub fn new() -> Option { + fn is_available() -> bool { + static IS_SUPPORTED: OnceLock = OnceLock::new(); + + *IS_SUPPORTED.get_or_init(|| unsafe { + let Some(ptr) = (unsafe { memsec::memfd_secret_sized(1) }) else { + return false; + }; + memsec::free_memfd_secret(ptr); + true + }) + } + + if !is_available() { + return None; + } + + Some(Self) + } +} + +unsafe impl Allocator for LinuxMemfdSecretAlloc { + fn allocate(&self, layout: Layout) -> Result, AllocError> { + // Note: The allocator_api2 Allocator traits requires us to handle zero-sized allocations. + // We return an invalid pointer as you cannot allocate a zero-sized slice in most + // allocators. This is what allocator_api2::Global does as well: + // https://github.com/zakarumych/allocator-api2/blob/2dde97af85f3559619689cef152e90e6d8a0cee3/src/alloc/global.rs#L24-L29 + if layout.size() == 0 { + return Ok(unsafe { + NonNull::new_unchecked(core::ptr::slice_from_raw_parts_mut( + layout.align() as *mut u8, + 0, + )) + }); + } + + let ptr: NonNull<[u8]> = unsafe { memsec::memfd_secret_sized(layout.size()) } + .expect("memfd_secret_sized failed"); + + Ok(ptr) + } + + unsafe fn deallocate(&self, ptr: NonNull, layout: Layout) { + if layout.size() == 0 { + return; + } + + memsec::free_memfd_secret(ptr); + } +} diff --git a/crates/bitwarden-crypto/src/store/backend/implementation/custom_alloc/malloc.rs b/crates/bitwarden-crypto/src/store/backend/implementation/custom_alloc/malloc.rs new file mode 100644 index 000000000..eecdd76be --- /dev/null +++ b/crates/bitwarden-crypto/src/store/backend/implementation/custom_alloc/malloc.rs @@ -0,0 +1,75 @@ +use std::{ + alloc::{GlobalAlloc, Layout}, + ptr::NonNull, +}; + +use allocator_api2::alloc::{AllocError, Allocator}; + +pub(crate) struct MlockAlloc(crate::ZeroizingAllocator); + +impl MlockAlloc { + pub fn new() -> Self { + Self(crate::ZeroizingAllocator(std::alloc::System)) + } +} + +unsafe fn ptr_to_nonnull_slice(ptr: *mut u8, len: usize) -> Result, AllocError> { + if ptr.is_null() { + return Err(AllocError); + } + + // SAFETY: The caller must ensure that `ptr` is valid for `len` elements. + Ok(unsafe { + let slice = std::slice::from_raw_parts_mut(ptr, len); + NonNull::new(slice).expect("slice is never null") + }) +} + +unsafe impl Allocator for MlockAlloc { + fn allocate(&self, layout: Layout) -> Result, AllocError> { + // Note: The allocator_api2 Allocator traits requires us to handle zero-sized allocations. + // We return an invalid pointer as you cannot allocate a zero-sized slice in most + // allocators. This is what allocator_api2::Global does as well: + // https://github.com/zakarumych/allocator-api2/blob/2dde97af85f3559619689cef152e90e6d8a0cee3/src/alloc/global.rs#L24-L29 + if layout.size() == 0 { + return Ok(unsafe { + NonNull::new_unchecked(core::ptr::slice_from_raw_parts_mut( + layout.align() as *mut u8, + 0, + )) + }); + } + + let ptr = unsafe { self.0.alloc(layout) }; + + if ptr.is_null() { + return Err(AllocError); + } + + #[cfg(all( + not(target_arch = "wasm32"), + not(windows), + not(feature = "no-memory-hardening") + ))] + unsafe { + memsec::mlock(ptr, layout.size()) + }; + + unsafe { ptr_to_nonnull_slice(ptr, layout.size()) } + } + + unsafe fn deallocate(&self, ptr: NonNull, layout: Layout) { + if layout.size() == 0 { + return; + } + + #[cfg(all( + not(target_arch = "wasm32"), + not(windows), + not(feature = "no-memory-hardening") + ))] + memsec::munlock(ptr.as_ptr(), layout.size()); + + self.0.dealloc(ptr.as_ptr(), layout); + } +} diff --git a/crates/bitwarden-crypto/src/store/backend/implementation/custom_alloc/mod.rs b/crates/bitwarden-crypto/src/store/backend/implementation/custom_alloc/mod.rs new file mode 100644 index 000000000..b462436c9 --- /dev/null +++ b/crates/bitwarden-crypto/src/store/backend/implementation/custom_alloc/mod.rs @@ -0,0 +1,64 @@ +use allocator_api2::alloc::Allocator; +use zeroize::ZeroizeOnDrop; + +use crate::{store::backend::StoreBackend, KeyId}; + +mod malloc; + +pub(super) type MlockBackend = CustomAllocBackend; +pub(super) use malloc::MlockAlloc; + +#[cfg(all(target_os = "linux", not(feature = "no-memory-hardening")))] +mod linux_memfd_secret; +#[cfg(all(target_os = "linux", not(feature = "no-memory-hardening")))] +pub(super) type LinuxMemfdSecretBackend = + CustomAllocBackend; +#[cfg(all(target_os = "linux", not(feature = "no-memory-hardening")))] +pub(super) use linux_memfd_secret::LinuxMemfdSecretAlloc; + +pub(super) struct CustomAllocBackend { + map: hashbrown::HashMap, +} + +impl CustomAllocBackend { + pub(super) fn new(alloc: Alloc) -> Self { + Self { + map: hashbrown::HashMap::new_in(alloc), + } + } +} + +impl ZeroizeOnDrop for CustomAllocBackend {} + +impl StoreBackend + for CustomAllocBackend +{ + fn upsert(&mut self, key_id: Key, key: ::KeyValue) { + self.map.insert(key_id, key); + } + + fn get(&self, key_id: Key) -> Option<&::KeyValue> { + self.map.get(&key_id) + } + + fn remove(&mut self, key_id: Key) { + self.map.remove(&key_id); + } + + fn clear(&mut self) { + self.map.clear(); + } + + fn retain(&mut self, f: fn(Key) -> bool) { + self.map.retain(|key, _| f(*key)); + } +} + +#[cfg(test)] +impl super::super::StoreBackendDebug + for CustomAllocBackend +{ + fn elements(&self) -> Vec<(Key, &Key::KeyValue)> { + self.map.iter().map(|(k, v)| (*k, v)).collect() + } +} diff --git a/crates/bitwarden-crypto/src/store/backend/implementation/custom_slice/linux_memfd_secret.rs b/crates/bitwarden-crypto/src/store/backend/implementation/custom_slice/linux_memfd_secret.rs deleted file mode 100644 index 9c2bd6d61..000000000 --- a/crates/bitwarden-crypto/src/store/backend/implementation/custom_slice/linux_memfd_secret.rs +++ /dev/null @@ -1,111 +0,0 @@ -use std::{mem::MaybeUninit, ptr::NonNull, sync::OnceLock}; - -use super::{KeyId, SliceBackend, SliceLike}; - -// This is an in-memory key store that is protected by memfd_secret on Linux 5.14+. -// This should be secure against memory dumps from anything except a malicious kernel driver. -// Note that not all 5.14+ systems have support for memfd_secret enabled, so -// LinuxMemfdSecretKeyStore::new returns an Option. -pub(crate) type LinuxMemfdSecretBackend = SliceBackend; - -pub(crate) struct MemfdSecretImplKeyData { - ptr: std::ptr::NonNull<[u8]>, - capacity: usize, -} - -// For Send+Sync to be safe, we need to ensure that the memory is only accessed mutably from one -// thread. To do this, we have to make sure that any funcion in `MemfdSecretImplKeyData` that -// accesses the pointer mutably is defined as &mut self, and that the pointer is never copied or -// moved outside the struct. -unsafe impl Send for MemfdSecretImplKeyData {} -unsafe impl Sync for MemfdSecretImplKeyData {} - -impl Drop for MemfdSecretImplKeyData { - fn drop(&mut self) { - unsafe { - memsec::free_memfd_secret(self.ptr); - } - } -} - -impl SliceLike for MemfdSecretImplKeyData { - fn is_available() -> bool { - static IS_SUPPORTED: OnceLock = OnceLock::new(); - - *IS_SUPPORTED.get_or_init(|| unsafe { - let Some(ptr) = memsec::memfd_secret_sized(1) else { - return false; - }; - memsec::free_memfd_secret(ptr); - true - }) - } - - fn with_capacity(capacity: usize) -> Self { - let entry_size = std::mem::size_of::>(); - - unsafe { - let ptr: NonNull<[u8]> = memsec::memfd_secret_sized(capacity * entry_size) - .expect("memfd_secret_sized failed"); - - // Initialize the array with Nones using MaybeUninit - let uninit_slice: &mut [MaybeUninit<_>] = std::slice::from_raw_parts_mut( - ptr.as_ptr() as *mut MaybeUninit>, - capacity, - ); - for elem in uninit_slice { - elem.write(None); - } - - MemfdSecretImplKeyData { ptr, capacity } - } - } - - fn get_key_data(&self) -> &[Option<(Key, Key::KeyValue)>] { - let ptr = self.ptr.as_ptr() as *const Option<(Key, Key::KeyValue)>; - // SAFETY: The pointer is valid and points to a valid slice of the correct size. - // This function is &self so it only takes a immutable *const pointer. - unsafe { std::slice::from_raw_parts(ptr, self.capacity) } - } - - fn get_key_data_mut(&mut self) -> &mut [Option<(Key, Key::KeyValue)>] { - let ptr = self.ptr.as_ptr() as *mut Option<(Key, Key::KeyValue)>; - // SAFETY: The pointer is valid and points to a valid slice of the correct size. - // This function is &mut self so it can take a mutable *mut pointer. - unsafe { std::slice::from_raw_parts_mut(ptr, self.capacity) } - } -} - -#[cfg(test)] -mod tests { - use super::*; - use crate::store::backend::{ - implementation::custom_slice::tests::{TestKey, TestKeyValue}, - StoreBackend as _, - }; - - #[test] - fn test_resize() { - let mut store = LinuxMemfdSecretBackend::::with_capacity(1).unwrap(); - - for (idx, key) in [ - TestKey::A, - TestKey::B(10), - TestKey::C, - TestKey::B(7), - TestKey::A, - TestKey::C, - ] - .into_iter() - .enumerate() - { - store.upsert(key, TestKeyValue::new(idx)); - } - - assert_eq!(store.get(TestKey::A), Some(&TestKeyValue::new(4))); - assert_eq!(store.get(TestKey::B(10)), Some(&TestKeyValue::new(1))); - assert_eq!(store.get(TestKey::C), Some(&TestKeyValue::new(5))); - assert_eq!(store.get(TestKey::B(7)), Some(&TestKeyValue::new(3))); - assert_eq!(store.get(TestKey::B(20)), None); - } -} diff --git a/crates/bitwarden-crypto/src/store/backend/implementation/custom_slice/mod.rs b/crates/bitwarden-crypto/src/store/backend/implementation/custom_slice/mod.rs deleted file mode 100644 index f816eb481..000000000 --- a/crates/bitwarden-crypto/src/store/backend/implementation/custom_slice/mod.rs +++ /dev/null @@ -1,747 +0,0 @@ -use std::marker::PhantomData; - -use zeroize::ZeroizeOnDrop; - -use super::StoreBackend; -use crate::KeyId; - -#[cfg(all(target_os = "linux", not(feature = "no-memory-hardening")))] -pub(crate) mod linux_memfd_secret; -pub(crate) mod rust; - -/// This trait represents some data stored sequentially in memory that can be interpreted as a -/// slice. -/// Implementations of this trait should ensure that the initialized data is protected -/// as much as possible. The data is already Zeroized on Drop, so implementations should -/// only need to worry about removing any protections they've added, or releasing any resources. -#[allow(drop_bounds)] -pub(crate) trait SliceLike: Send + Sync + Sized + Drop { - /// Check if the data store is available on this platform. - fn is_available() -> bool; - - /// Initialize a new data store with the given capacity. - /// The data MUST be initialized to all None values, and - /// it's capacity must be equal or greater than the provided value. - fn with_capacity(capacity: usize) -> Self; - - /// Return an immutable slice of the data. It must return the full allocated capacity, with no - /// uninitialized values. - fn get_key_data(&self) -> &[Option<(Key, Key::KeyValue)>]; - - /// Return an mutable slice of the data. It must return the full allocated capacity, with no - /// uninitialized values. - fn get_key_data_mut(&mut self) -> &mut [Option<(Key, Key::KeyValue)>]; -} - -/// This represents a key store over an arbitrary fixed size slice. -/// This is meant to abstract over the different ways to store keys in memory, whether we're -/// using a Box<[u8]> or a NonNull, regardless of if that memory was allocated by Rust or not. -/// -/// Internally this is represented as a slice of `Option<(Key, Key::KeyValue)>` -/// and elements are sorted based on Key for performance. In essence, this is almost a homegrown -/// `HashMap`. -/// -/// The reason why we're not using a Rust collection like `Vec` or `HashMap` is that those types -/// expect their memory to be allocated by Rust, and they will deallocate/reallocate it as needed. -/// That will not work for our usecases where we want to have control over allocations/deallocations -/// and where some of our strategies rely on using system-allocated secure memory for the storage, -/// like the Linux-only `memfd_secret` API. -pub(crate) struct SliceBackend> { - // This represents the number of elements in the container, it's always less than or equal to - // the length of `data`. - length: usize, - - // This represents the maximum number of elements that can be stored in the container. - // This is always equal to the length of `data`, but we store it to avoid recomputing it. - capacity: usize, - - // This is the actual data that stores the keys, optional as we can have it not yet - // uninitialized - slice: Option, - - _key: PhantomData, -} - -impl> ZeroizeOnDrop for SliceBackend {} - -impl> Drop for SliceBackend { - fn drop(&mut self) { - self.clear(); - } -} - -impl> StoreBackend for SliceBackend { - fn upsert(&mut self, key_ref: Key, key: Key::KeyValue) { - match self.find_by_key_ref(&key_ref) { - Ok(idx) => { - // Key already exists, we just need to replace the value - let slice = self.get_key_data_mut(); - slice[idx] = Some((key_ref, key)); - } - Err(idx) => { - // Make sure that we have enough capacity, and resize if needed - self.ensure_capacity(1); - - let len = self.length; - let slice = self.get_key_data_mut(); - if idx < len { - // If we're not right at the end, we have to shift all the following elements - // one position to the right - slice[idx..=len].rotate_right(1); - } - slice[idx] = Some((key_ref, key)); - self.length += 1; - } - } - } - - fn get(&self, key_ref: Key) -> Option<&Key::KeyValue> { - self.find_by_key_ref(&key_ref) - .ok() - .and_then(|idx| self.get_key_data().get(idx)) - .and_then(|f| f.as_ref().map(|f| &f.1)) - } - - fn remove(&mut self, key_ref: Key) { - if let Ok(idx) = self.find_by_key_ref(&key_ref) { - let len = self.length; - let slice = self.get_key_data_mut(); - slice[idx] = None; - slice[idx..len].rotate_left(1); - self.length -= 1; - } - } - - fn clear(&mut self) { - let len = self.length; - self.get_key_data_mut()[0..len].fill_with(|| None); - self.length = 0; - } - - fn retain(&mut self, f: fn(Key) -> bool) { - let len = self.length; - let slice = self.get_key_data_mut(); - - let mut removed_elements = 0; - - for value in slice.iter_mut().take(len) { - let key = value - .as_ref() - .map(|e| e.0) - .expect("Values in a slice are always Some"); - - if !f(key) { - *value = None; - removed_elements += 1; - } - } - - // If we haven't removed any elements, we don't need to compact the slice - if removed_elements == 0 { - return; - } - - // Remove all the None values from the middle of the slice - - for idx in 0..len { - if slice[idx].is_none() { - slice[idx..len].rotate_left(1); - } - } - - self.length -= removed_elements; - } -} - -impl> SliceBackend { - pub(crate) fn new() -> Option { - Self::with_capacity(0) - } - - pub(crate) fn with_capacity(capacity: usize) -> Option { - if !Data::is_available() { - return None; - } - - // If the capacity is 0, we don't need to allocate any memory. - // This allows us to initialize the container lazily. - let slice = (capacity != 0).then(|| Data::with_capacity(capacity)); - Some(Self { - length: 0, - capacity, - slice, - _key: PhantomData, - }) - } - - /// Check if the container has enough capacity to store `new_elements` more elements. - /// If the result is Ok, the container has enough capacity. - /// If it's Err, the container needs to be resized. - /// The error value returns a suggested new capacity. - fn check_capacity(&self, new_elements: usize) -> Result<(), usize> { - let new_size = self.length + new_elements; - - // We still have enough capacity - if new_size <= self.capacity { - Ok(()) - - // This is the first allocation - } else if self.capacity == 0 { - const PAGE_SIZE: usize = 4096; - let entry_size = std::mem::size_of::>(); - - // We're using mlock APIs to protect the memory, which lock at the page level. - // To avoid wasting memory, we want to allocate at least a page. - let entries_per_page = PAGE_SIZE / entry_size; - Err(entries_per_page) - - // We need to resize the container - } else { - // We want to increase the capacity by a multiple to be mostly aligned with page size, - // we also need to make sure that we have enough space for the new elements, so we round - // up - let increase_factor = usize::div_ceil(new_size, self.capacity); - Err(self.capacity * increase_factor) - } - } - - fn ensure_capacity(&mut self, new_elements: usize) { - if let Err(new_capacity) = self.check_capacity(new_elements) { - // Create a new store with the correct capacity and replace self with it - let mut new_self = - Self::with_capacity(new_capacity).expect("Could not allocate new store"); - new_self.copy_from(self); - *self = new_self; - } - } - - // These two are just helper functions to avoid having to deal with the optional Data - // When Data is None we just return empty slices, which don't allow any operations - fn get_key_data(&self) -> &[Option<(Key, Key::KeyValue)>] { - self.slice.as_ref().map(|d| d.get_key_data()).unwrap_or(&[]) - } - fn get_key_data_mut(&mut self) -> &mut [Option<(Key, Key::KeyValue)>] { - self.slice - .as_mut() - .map(|d| d.get_key_data_mut()) - .unwrap_or(&mut []) - } - - fn find_by_key_ref(&self, key_ref: &Key) -> Result { - // Because we know all the None's are at the end and all the Some values are at the - // beginning, we only need to search for the key in the first `size` elements. - let slice = &self.get_key_data()[..self.length]; - - // This structure is almost always used for reads instead of writes, so we can use a binary - // search to optimize for the read case. - slice.binary_search_by(|k| { - debug_assert!( - k.is_some(), - "We should never have a None value in the middle of the slice" - ); - - match k { - Some((k, _)) => k.cmp(key_ref), - None => std::cmp::Ordering::Greater, - } - }) - } - - pub(crate) fn copy_from(&mut self, other: &mut Self) -> bool { - if other.capacity > self.capacity { - return false; - } - - // Empty the current container - self.clear(); - - let new_length = other.length; - - // Move the data from the other container - let this = self.get_key_data_mut(); - let that = other.get_key_data_mut(); - for idx in 0..new_length { - std::mem::swap(&mut this[idx], &mut that[idx]); - } - - // Update the length - self.length = new_length; - - true - } -} - -#[cfg(test)] -pub(crate) mod tests { - use zeroize::Zeroize; - - use super::{rust::RustBackend, *}; - use crate::{CryptoKey, KeyId}; - - #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] - pub enum TestKey { - A, - B(u8), - C, - } - #[derive(Debug, Clone, Hash, Eq, PartialEq, Ord, PartialOrd)] - pub struct TestKeyValue([u8; 16]); - impl zeroize::ZeroizeOnDrop for TestKeyValue {} - impl CryptoKey for TestKeyValue {} - impl TestKeyValue { - pub fn new(value: usize) -> Self { - // Just fill the array with some values - let mut key = [0; 16]; - key[0..8].copy_from_slice(&value.to_le_bytes()); - key[8..16].copy_from_slice(&value.to_be_bytes()); - Self(key) - } - } - - impl Drop for TestKeyValue { - fn drop(&mut self) { - self.0.as_mut().zeroize(); - } - } - - impl KeyId for TestKey { - type KeyValue = TestKeyValue; - - fn is_local(&self) -> bool { - false - } - } - - #[test] - fn test_slice_container_insertion() { - let mut container = RustBackend::::with_capacity(5).unwrap(); - - assert_eq!(container.get_key_data(), [None, None, None, None, None]); - - // Insert one key, which should be at the beginning - container.upsert(TestKey::B(10), TestKeyValue::new(110)); - assert_eq!( - container.get_key_data(), - [ - Some((TestKey::B(10), TestKeyValue::new(110))), - None, - None, - None, - None - ] - ); - - // Insert a key that should be right after the first one - container.upsert(TestKey::C, TestKeyValue::new(1000)); - assert_eq!( - container.get_key_data(), - [ - Some((TestKey::B(10), TestKeyValue::new(110))), - Some((TestKey::C, TestKeyValue::new(1000))), - None, - None, - None - ] - ); - - // Insert a key in the middle - container.upsert(TestKey::B(20), TestKeyValue::new(210)); - assert_eq!( - container.get_key_data(), - [ - Some((TestKey::B(10), TestKeyValue::new(110))), - Some((TestKey::B(20), TestKeyValue::new(210))), - Some((TestKey::C, TestKeyValue::new(1000))), - None, - None - ] - ); - - // Insert a key right at the start - container.upsert(TestKey::A, TestKeyValue::new(0)); - assert_eq!( - container.get_key_data(), - [ - Some((TestKey::A, TestKeyValue::new(0))), - Some((TestKey::B(10), TestKeyValue::new(110))), - Some((TestKey::B(20), TestKeyValue::new(210))), - Some((TestKey::C, TestKeyValue::new(1000))), - None - ] - ); - - // Insert a key in the middle, which fills the container - container.upsert(TestKey::B(30), TestKeyValue::new(310)); - assert_eq!( - container.get_key_data(), - [ - Some((TestKey::A, TestKeyValue::new(0))), - Some((TestKey::B(10), TestKeyValue::new(110))), - Some((TestKey::B(20), TestKeyValue::new(210))), - Some((TestKey::B(30), TestKeyValue::new(310))), - Some((TestKey::C, TestKeyValue::new(1000))), - ] - ); - - // Replacing an existing value at the start - container.upsert(TestKey::A, TestKeyValue::new(1)); - assert_eq!( - container.get_key_data(), - [ - Some((TestKey::A, TestKeyValue::new(1))), - Some((TestKey::B(10), TestKeyValue::new(110))), - Some((TestKey::B(20), TestKeyValue::new(210))), - Some((TestKey::B(30), TestKeyValue::new(310))), - Some((TestKey::C, TestKeyValue::new(1000))), - ] - ); - - // Replacing an existing value at the middle - container.upsert(TestKey::B(20), TestKeyValue::new(211)); - assert_eq!( - container.get_key_data(), - [ - Some((TestKey::A, TestKeyValue::new(1))), - Some((TestKey::B(10), TestKeyValue::new(110))), - Some((TestKey::B(20), TestKeyValue::new(211))), - Some((TestKey::B(30), TestKeyValue::new(310))), - Some((TestKey::C, TestKeyValue::new(1000))), - ] - ); - - // Replacing an existing value at the end - container.upsert(TestKey::C, TestKeyValue::new(1001)); - assert_eq!( - container.get_key_data(), - [ - Some((TestKey::A, TestKeyValue::new(1))), - Some((TestKey::B(10), TestKeyValue::new(110))), - Some((TestKey::B(20), TestKeyValue::new(211))), - Some((TestKey::B(30), TestKeyValue::new(310))), - Some((TestKey::C, TestKeyValue::new(1001))), - ] - ); - } - - #[test] - fn test_slice_container_get() { - let mut container = RustBackend::::with_capacity(5).unwrap(); - - for (key, value) in [ - (TestKey::A, TestKeyValue::new(1)), - (TestKey::B(10), TestKeyValue::new(110)), - (TestKey::C, TestKeyValue::new(1000)), - ] { - container.upsert(key, value); - } - - assert_eq!(container.get(TestKey::A), Some(&TestKeyValue::new(1))); - assert_eq!(container.get(TestKey::B(10)), Some(&TestKeyValue::new(110))); - assert_eq!(container.get(TestKey::B(20)), None); - assert_eq!(container.get(TestKey::C), Some(&TestKeyValue::new(1000))); - } - - #[test] - fn test_slice_container_clear() { - let mut container = RustBackend::::with_capacity(5).unwrap(); - - for (key, value) in [ - (TestKey::A, TestKeyValue::new(1)), - (TestKey::B(10), TestKeyValue::new(110)), - (TestKey::B(20), TestKeyValue::new(210)), - (TestKey::B(30), TestKeyValue::new(310)), - (TestKey::C, TestKeyValue::new(1000)), - ] { - container.upsert(key, value); - } - - assert_eq!( - container.get_key_data(), - [ - Some((TestKey::A, TestKeyValue::new(1))), - Some((TestKey::B(10), TestKeyValue::new(110))), - Some((TestKey::B(20), TestKeyValue::new(210))), - Some((TestKey::B(30), TestKeyValue::new(310))), - Some((TestKey::C, TestKeyValue::new(1000))), - ] - ); - - container.clear(); - - assert_eq!(container.get_key_data(), [None, None, None, None, None]); - } - - #[test] - fn test_slice_container_ensure_capacity() { - let mut container = RustBackend::::with_capacity(5).unwrap(); - - assert_eq!(container.capacity, 5); - assert_eq!(container.length, 0); - - assert_eq!(container.check_capacity(0), Ok(())); - assert_eq!(container.check_capacity(6), Err(10)); - assert_eq!(container.check_capacity(10), Err(10)); - assert_eq!(container.check_capacity(11), Err(15)); - assert_eq!(container.check_capacity(51), Err(55)); - - for (key, value) in [ - (TestKey::A, TestKeyValue::new(1)), - (TestKey::B(10), TestKeyValue::new(110)), - (TestKey::B(20), TestKeyValue::new(210)), - (TestKey::B(30), TestKeyValue::new(310)), - (TestKey::C, TestKeyValue::new(1000)), - ] { - container.upsert(key, value); - } - - assert_eq!(container.check_capacity(0), Ok(())); - assert_eq!(container.check_capacity(6), Err(15)); - assert_eq!(container.check_capacity(10), Err(15)); - assert_eq!(container.check_capacity(11), Err(20)); - assert_eq!(container.check_capacity(51), Err(60)); - } - - #[test] - fn test_slice_container_removal() { - let mut container = RustBackend::::with_capacity(5).unwrap(); - - for (key, value) in [ - (TestKey::A, TestKeyValue::new(1)), - (TestKey::B(10), TestKeyValue::new(110)), - (TestKey::B(20), TestKeyValue::new(210)), - (TestKey::B(30), TestKeyValue::new(310)), - (TestKey::C, TestKeyValue::new(1000)), - ] { - container.upsert(key, value); - } - - // Remove the last element - container.remove(TestKey::C); - assert_eq!( - container.get_key_data(), - [ - Some((TestKey::A, TestKeyValue::new(1))), - Some((TestKey::B(10), TestKeyValue::new(110))), - Some((TestKey::B(20), TestKeyValue::new(210))), - Some((TestKey::B(30), TestKeyValue::new(310))), - None, - ] - ); - - // Remove the first element - container.remove(TestKey::A); - assert_eq!( - container.get_key_data(), - [ - Some((TestKey::B(10), TestKeyValue::new(110))), - Some((TestKey::B(20), TestKeyValue::new(210))), - Some((TestKey::B(30), TestKeyValue::new(310))), - None, - None - ] - ); - - // Remove a non-existing element - container.remove(TestKey::A); - assert_eq!( - container.get_key_data(), - [ - Some((TestKey::B(10), TestKeyValue::new(110))), - Some((TestKey::B(20), TestKeyValue::new(210))), - Some((TestKey::B(30), TestKeyValue::new(310))), - None, - None - ] - ); - - // Remove an element in the middle - container.remove(TestKey::B(20)); - assert_eq!( - container.get_key_data(), - [ - Some((TestKey::B(10), TestKeyValue::new(110))), - Some((TestKey::B(30), TestKeyValue::new(310))), - None, - None, - None - ] - ); - - // Remove all the remaining elements - container.remove(TestKey::B(30)); - assert_eq!( - container.get_key_data(), - [ - Some((TestKey::B(10), TestKeyValue::new(110))), - None, - None, - None, - None - ] - ); - container.remove(TestKey::B(10)); - assert_eq!(container.get_key_data(), [None, None, None, None, None]); - - // Remove from an empty container - container.remove(TestKey::B(10)); - assert_eq!(container.get_key_data(), [None, None, None, None, None]); - } - - #[test] - fn test_slice_container_retain_removes_one() { - let mut container = RustBackend::::with_capacity(5).unwrap(); - - for (key, value) in [ - (TestKey::A, TestKeyValue::new(1)), - (TestKey::B(10), TestKeyValue::new(110)), - (TestKey::B(20), TestKeyValue::new(210)), - (TestKey::B(30), TestKeyValue::new(310)), - (TestKey::C, TestKeyValue::new(1000)), - ] { - container.upsert(key, value); - } - - // Remove the last element - container.retain(|k| k != TestKey::C); - assert_eq!( - container.get_key_data(), - [ - Some((TestKey::A, TestKeyValue::new(1))), - Some((TestKey::B(10), TestKeyValue::new(110))), - Some((TestKey::B(20), TestKeyValue::new(210))), - Some((TestKey::B(30), TestKeyValue::new(310))), - None, - ] - ); - - // Remove the first element - container.retain(|k| k != TestKey::A); - assert_eq!( - container.get_key_data(), - [ - Some((TestKey::B(10), TestKeyValue::new(110))), - Some((TestKey::B(20), TestKeyValue::new(210))), - Some((TestKey::B(30), TestKeyValue::new(310))), - None, - None - ] - ); - - // Remove a non-existing element - container.retain(|k| k != TestKey::A); - assert_eq!( - container.get_key_data(), - [ - Some((TestKey::B(10), TestKeyValue::new(110))), - Some((TestKey::B(20), TestKeyValue::new(210))), - Some((TestKey::B(30), TestKeyValue::new(310))), - None, - None - ] - ); - - // Remove an element in the middle - container.retain(|k| k != TestKey::B(20)); - assert_eq!( - container.get_key_data(), - [ - Some((TestKey::B(10), TestKeyValue::new(110))), - Some((TestKey::B(30), TestKeyValue::new(310))), - None, - None, - None - ] - ); - - // Remove all the remaining elements - container.retain(|k| k != TestKey::B(30)); - assert_eq!( - container.get_key_data(), - [ - Some((TestKey::B(10), TestKeyValue::new(110))), - None, - None, - None, - None - ] - ); - container.retain(|k| k != TestKey::B(10)); - assert_eq!(container.get_key_data(), [None, None, None, None, None]); - - // Remove from an empty container - container.retain(|k| k != TestKey::B(10)); - assert_eq!(container.get_key_data(), [None, None, None, None, None]); - } - - #[test] - fn test_slice_container_retain_removes_none() { - let mut container = RustBackend::::with_capacity(5).unwrap(); - - for (key, value) in [ - (TestKey::A, TestKeyValue::new(1)), - (TestKey::B(10), TestKeyValue::new(110)), - (TestKey::B(20), TestKeyValue::new(210)), - (TestKey::B(30), TestKeyValue::new(310)), - (TestKey::C, TestKeyValue::new(1000)), - ] { - container.upsert(key, value); - } - - container.retain(|_k| true); - assert_eq!( - container.get_key_data(), - [ - Some((TestKey::A, TestKeyValue::new(1))), - Some((TestKey::B(10), TestKeyValue::new(110))), - Some((TestKey::B(20), TestKeyValue::new(210))), - Some((TestKey::B(30), TestKeyValue::new(310))), - Some((TestKey::C, TestKeyValue::new(1000))), - ] - ); - } - - #[test] - fn test_slice_container_retain_removes_some() { - let mut container = RustBackend::::with_capacity(5).unwrap(); - - for (key, value) in [ - (TestKey::A, TestKeyValue::new(1)), - (TestKey::B(10), TestKeyValue::new(110)), - (TestKey::B(20), TestKeyValue::new(210)), - (TestKey::B(30), TestKeyValue::new(310)), - (TestKey::C, TestKeyValue::new(1000)), - ] { - container.upsert(key, value); - } - - container.retain(|k| matches!(k, TestKey::A | TestKey::B(20) | TestKey::C)); - assert_eq!( - container.get_key_data(), - [ - Some((TestKey::A, TestKeyValue::new(1))), - Some((TestKey::B(20), TestKeyValue::new(210))), - Some((TestKey::C, TestKeyValue::new(1000))), - None, - None, - ] - ); - } - - #[test] - fn test_slice_container_retain_removes_all() { - let mut container = RustBackend::::with_capacity(5).unwrap(); - - for (key, value) in [ - (TestKey::A, TestKeyValue::new(1)), - (TestKey::B(10), TestKeyValue::new(110)), - (TestKey::B(20), TestKeyValue::new(210)), - (TestKey::B(30), TestKeyValue::new(310)), - (TestKey::C, TestKeyValue::new(1000)), - ] { - container.upsert(key, value); - } - - container.retain(|_k| false); - assert_eq!(container.get_key_data(), [None, None, None, None, None]); - } -} diff --git a/crates/bitwarden-crypto/src/store/backend/implementation/custom_slice/rust.rs b/crates/bitwarden-crypto/src/store/backend/implementation/custom_slice/rust.rs deleted file mode 100644 index 7f4415fe5..000000000 --- a/crates/bitwarden-crypto/src/store/backend/implementation/custom_slice/rust.rs +++ /dev/null @@ -1,111 +0,0 @@ -use super::{KeyId, SliceBackend, SliceLike}; - -// This is a basic in-memory key store for the cases where we don't have a secure key store -// available. We still make use mlock to protect the memory from being swapped to disk, and we -// zeroize the values when dropped. -pub(crate) type RustBackend = SliceBackend>; - -pub(crate) struct RustBackendImpl { - #[allow(clippy::type_complexity)] - data: Box<[Option<(Key, Key::KeyValue)>]>, -} - -impl Drop for RustBackendImpl { - fn drop(&mut self) { - munlock_data(self.data.as_mut()); - } -} - -impl SliceLike for RustBackendImpl { - fn is_available() -> bool { - true - } - - fn with_capacity(capacity: usize) -> Self { - #[allow(unused_mut)] - let mut data: Box<_> = std::iter::repeat_with(|| None).take(capacity).collect(); - mlock_data(data.as_mut()); - RustBackendImpl { data } - } - - fn get_key_data(&self) -> &[Option<(Key, Key::KeyValue)>] { - self.data.as_ref() - } - - fn get_key_data_mut(&mut self) -> &mut [Option<(Key, Key::KeyValue)>] { - self.data.as_mut() - } -} - -#[allow(unused_variables)] -fn mlock_data(data: &mut [T]) { - #[cfg(all( - not(target_arch = "wasm32"), - not(windows), - not(feature = "no-memory-hardening") - ))] - { - unsafe { - memsec::mlock(data.as_mut_ptr() as *mut u8, std::mem::size_of_val(data)); - } - } -} - -#[allow(unused_variables)] -fn munlock_data(data: &mut [T]) { - #[cfg(all( - not(target_arch = "wasm32"), - not(windows), - not(feature = "no-memory-hardening") - ))] - { - use std::mem::MaybeUninit; - unsafe { - memsec::munlock(data.as_mut_ptr() as *mut u8, std::mem::size_of_val(data)); - - // Note: munlock is zeroing the memory, which leaves the data in an undefined - // state, so we set it to None/Default again to avoid UB in the Drop implementation. - let uninit_slice: &mut [MaybeUninit<_>] = std::slice::from_raw_parts_mut( - data.as_mut_ptr() as *mut MaybeUninit, - data.len(), - ); - for elem in uninit_slice { - elem.write(T::default()); - } - } - } -} - -#[cfg(test)] -mod tests { - use super::*; - use crate::store::backend::{ - implementation::custom_slice::tests::{TestKey, TestKeyValue}, - StoreBackend as _, - }; - - #[test] - fn test_resize() { - let mut store = RustBackend::::with_capacity(1).unwrap(); - - for (idx, key) in [ - TestKey::A, - TestKey::B(10), - TestKey::C, - TestKey::B(7), - TestKey::A, - TestKey::C, - ] - .into_iter() - .enumerate() - { - store.upsert(key, TestKeyValue::new(idx)); - } - - assert_eq!(store.get(TestKey::A), Some(&TestKeyValue::new(4))); - assert_eq!(store.get(TestKey::B(10)), Some(&TestKeyValue::new(1))); - assert_eq!(store.get(TestKey::C), Some(&TestKeyValue::new(5))); - assert_eq!(store.get(TestKey::B(7)), Some(&TestKeyValue::new(3))); - assert_eq!(store.get(TestKey::B(20)), None); - } -} diff --git a/crates/bitwarden-crypto/src/store/backend/implementation/mod.rs b/crates/bitwarden-crypto/src/store/backend/implementation/mod.rs index 07d932278..14aa4b219 100644 --- a/crates/bitwarden-crypto/src/store/backend/implementation/mod.rs +++ b/crates/bitwarden-crypto/src/store/backend/implementation/mod.rs @@ -1,25 +1,27 @@ use super::StoreBackend; use crate::store::KeyId; -mod custom_slice; +mod basic; +mod custom_alloc; /// Initializes a key store backend with the best available implementation for the current platform pub fn create_store() -> Box> { #[cfg(all(target_os = "linux", not(feature = "no-memory-hardening")))] - if let Some(key_store) = custom_slice::linux_memfd_secret::LinuxMemfdSecretBackend::::new() - { - return Box::new(key_store); + if let Some(key_store) = custom_alloc::LinuxMemfdSecretAlloc::new() { + return Box::new(custom_alloc::LinuxMemfdSecretBackend::::new(key_store)); } - Box::new( - custom_slice::rust::RustBackend::new().expect("RustKeyStore should always be available"), - ) + Box::new(custom_alloc::MlockBackend::::new( + custom_alloc::MlockAlloc::new(), + )) } #[cfg(test)] mod tests { use super::*; - use crate::{traits::tests::TestSymmKey, SymmetricCryptoKey}; + use crate::{ + store::backend::StoreBackendDebug, traits::tests::TestSymmKey, SymmetricCryptoKey, + }; #[test] fn test_creates_a_valid_store() { @@ -33,4 +35,123 @@ mod tests { key.to_base64() ); } + + #[test] + fn validate_mlock_store() { + let basic_store: Box> = + Box::new(basic::BasicBackend::new()); + + let mlock_store: Box> = Box::new( + custom_alloc::MlockBackend::::new(custom_alloc::MlockAlloc::new()), + ); + compare_stores(100_000, basic_store, mlock_store); + } + + #[cfg(all(target_os = "linux", not(feature = "no-memory-hardening")))] + #[test] + fn validate_memfd_store() { + let basic_store: Box> = + Box::new(basic::BasicBackend::new()); + + let mlock_store: Box> = + Box::new(custom_alloc::LinuxMemfdSecretBackend::::new( + custom_alloc::LinuxMemfdSecretAlloc::new().unwrap(), + )); + compare_stores(100_000, basic_store, mlock_store); + } + + /// This function will perform a number of random operations on two stores, + /// and compare the results between them to make sure they match. + fn compare_stores( + num_iterations: usize, + mut a: Box>, + mut b: Box>, + ) { + use rand::{distributions::Standard, prelude::*}; + + #[derive(Debug)] + enum Operation { + Upsert(TestSymmKey, SymmetricCryptoKey), + Get(TestSymmKey), + Remove(TestSymmKey), + Clear, + Retain(fn(TestSymmKey) -> bool), + } + + impl Distribution for Standard { + fn sample(&self, rng: &mut R) -> Operation { + match rng.gen_range(0..=4) { + 0 => Operation::Upsert( + TestSymmKey::A(rng.gen_range(0..=10)), + SymmetricCryptoKey::generate(rng), + ), + 1 => Operation::Get(TestSymmKey::A(rng.gen_range(0..=10))), + 2 => Operation::Remove(TestSymmKey::A(rng.gen_range(0..=10))), + 3 => Operation::Clear, + // This one has to be constant as we can't capture variables + 4 => Operation::Retain(|key| match key { + TestSymmKey::A(a) => a % 9 == 0, + TestSymmKey::B((a, b)) => (a + b) % 10 == 3, + TestSymmKey::C(a) => a % 11 == 6, + }), + _ => unreachable!(), + } + } + } + + for _ in 0..num_iterations { + let operation = rand::random::(); + + match operation { + Operation::Upsert(key, value) => { + a.upsert(key, value.clone()); + b.upsert(key, value); + } + Operation::Get(key) => { + assert_eq!( + a.get(key), + b.get(key), + "Get operation for {key:?} has different results" + ); + // This doesn't modify the store, so we can skip the comparison after + continue; + } + Operation::Remove(key) => { + a.remove(key); + b.remove(key); + } + Operation::Clear => { + a.clear(); + b.clear(); + } + Operation::Retain(f) => { + a.retain(f); + b.retain(f); + } + } + + // Get all the elements from both stores and sort them the same way + let mut a_elements = a.elements(); + let mut b_elements = b.elements(); + a_elements.sort_by(|a, b| a.0.cmp(&b.0)); + b_elements.sort_by(|a, b| a.0.cmp(&b.0)); + + // Compare the two stores, as they should be the same + + assert_eq!( + a_elements.len(), + b_elements.len(), + "The two stores have different number of elements" + ); + + for ((akey, avalue), (bkey, bvalue)) in a_elements.iter().zip(b_elements.iter()) { + assert_eq!(akey, bkey, "The keys are different"); + assert_eq!( + avalue.to_base64(), + bvalue.to_base64(), + "The values are different" + ); + } + } + } } diff --git a/crates/bitwarden-crypto/src/store/backend/mod.rs b/crates/bitwarden-crypto/src/store/backend/mod.rs index 764535216..08bff35a2 100644 --- a/crates/bitwarden-crypto/src/store/backend/mod.rs +++ b/crates/bitwarden-crypto/src/store/backend/mod.rs @@ -36,3 +36,8 @@ pub trait StoreBackend: ZeroizeOnDrop + Send + Sync { /// In other words, remove all keys for which `f` returns false. fn retain(&mut self, f: fn(Key) -> bool); } + +#[cfg(test)] +trait StoreBackendDebug: StoreBackend { + fn elements(&self) -> Vec<(Key, &Key::KeyValue)>; +} From 56f1e034caebbc03593e9cb39fe0136cf4629909 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garci=CC=81a?= Date: Thu, 1 May 2025 20:55:15 +0200 Subject: [PATCH 03/12] Simplify compatibility check --- .../custom_alloc/linux_memfd_secret.rs | 29 +++++++------------ 1 file changed, 11 insertions(+), 18 deletions(-) diff --git a/crates/bitwarden-crypto/src/store/backend/implementation/custom_alloc/linux_memfd_secret.rs b/crates/bitwarden-crypto/src/store/backend/implementation/custom_alloc/linux_memfd_secret.rs index c5b9e8f29..98f7a713e 100644 --- a/crates/bitwarden-crypto/src/store/backend/implementation/custom_alloc/linux_memfd_secret.rs +++ b/crates/bitwarden-crypto/src/store/backend/implementation/custom_alloc/linux_memfd_secret.rs @@ -1,4 +1,4 @@ -use std::{alloc::Layout, ptr::NonNull, sync::OnceLock}; +use std::{alloc::Layout, ptr::NonNull, sync::LazyLock}; use allocator_api2::alloc::{AllocError, Allocator}; @@ -6,23 +6,16 @@ pub(crate) struct LinuxMemfdSecretAlloc; impl LinuxMemfdSecretAlloc { pub fn new() -> Option { - fn is_available() -> bool { - static IS_SUPPORTED: OnceLock = OnceLock::new(); - - *IS_SUPPORTED.get_or_init(|| unsafe { - let Some(ptr) = (unsafe { memsec::memfd_secret_sized(1) }) else { - return false; - }; - memsec::free_memfd_secret(ptr); - true - }) - } - - if !is_available() { - return None; - } - - Some(Self) + // To test if memfd_secret is supported, we try to allocate a 1 byte and see if that succeeds. + static IS_SUPPORTED: LazyLock = LazyLock::new(|| { + let Some(ptr) = (unsafe { memsec::memfd_secret_sized(1) }) else { + return false; + }; + memsec::free_memfd_secret(ptr); + true + }); + + (*IS_SUPPORTED).then_some(Self) } } From d1901f4879ac2d146a2848a4d583c40a54bb4863 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garci=CC=81a?= Date: Thu, 1 May 2025 20:56:31 +0200 Subject: [PATCH 04/12] Simplify conditions --- .../custom_alloc/linux_memfd_secret.rs | 2 +- .../implementation/custom_alloc/malloc.rs | 34 +++---------------- .../implementation/custom_alloc/mod.rs | 11 +++--- .../src/store/backend/implementation/mod.rs | 24 ++++++++----- 4 files changed, 25 insertions(+), 46 deletions(-) diff --git a/crates/bitwarden-crypto/src/store/backend/implementation/custom_alloc/linux_memfd_secret.rs b/crates/bitwarden-crypto/src/store/backend/implementation/custom_alloc/linux_memfd_secret.rs index 98f7a713e..19fd6eab5 100644 --- a/crates/bitwarden-crypto/src/store/backend/implementation/custom_alloc/linux_memfd_secret.rs +++ b/crates/bitwarden-crypto/src/store/backend/implementation/custom_alloc/linux_memfd_secret.rs @@ -11,7 +11,7 @@ impl LinuxMemfdSecretAlloc { let Some(ptr) = (unsafe { memsec::memfd_secret_sized(1) }) else { return false; }; - memsec::free_memfd_secret(ptr); + unsafe { memsec::free_memfd_secret(ptr) }; true }); diff --git a/crates/bitwarden-crypto/src/store/backend/implementation/custom_alloc/malloc.rs b/crates/bitwarden-crypto/src/store/backend/implementation/custom_alloc/malloc.rs index eecdd76be..5c4c7d3b5 100644 --- a/crates/bitwarden-crypto/src/store/backend/implementation/custom_alloc/malloc.rs +++ b/crates/bitwarden-crypto/src/store/backend/implementation/custom_alloc/malloc.rs @@ -13,18 +13,6 @@ impl MlockAlloc { } } -unsafe fn ptr_to_nonnull_slice(ptr: *mut u8, len: usize) -> Result, AllocError> { - if ptr.is_null() { - return Err(AllocError); - } - - // SAFETY: The caller must ensure that `ptr` is valid for `len` elements. - Ok(unsafe { - let slice = std::slice::from_raw_parts_mut(ptr, len); - NonNull::new(slice).expect("slice is never null") - }) -} - unsafe impl Allocator for MlockAlloc { fn allocate(&self, layout: Layout) -> Result, AllocError> { // Note: The allocator_api2 Allocator traits requires us to handle zero-sized allocations. @@ -45,17 +33,11 @@ unsafe impl Allocator for MlockAlloc { if ptr.is_null() { return Err(AllocError); } - - #[cfg(all( - not(target_arch = "wasm32"), - not(windows), - not(feature = "no-memory-hardening") - ))] - unsafe { - memsec::mlock(ptr, layout.size()) - }; - - unsafe { ptr_to_nonnull_slice(ptr, layout.size()) } + unsafe { memsec::mlock(ptr, layout.size()) }; + Ok(unsafe { + let slice = std::slice::from_raw_parts_mut(ptr, layout.size()); + NonNull::new(slice).expect("slice is never null") + }) } unsafe fn deallocate(&self, ptr: NonNull, layout: Layout) { @@ -63,13 +45,7 @@ unsafe impl Allocator for MlockAlloc { return; } - #[cfg(all( - not(target_arch = "wasm32"), - not(windows), - not(feature = "no-memory-hardening") - ))] memsec::munlock(ptr.as_ptr(), layout.size()); - self.0.dealloc(ptr.as_ptr(), layout); } } diff --git a/crates/bitwarden-crypto/src/store/backend/implementation/custom_alloc/mod.rs b/crates/bitwarden-crypto/src/store/backend/implementation/custom_alloc/mod.rs index b462436c9..f5e51cb9b 100644 --- a/crates/bitwarden-crypto/src/store/backend/implementation/custom_alloc/mod.rs +++ b/crates/bitwarden-crypto/src/store/backend/implementation/custom_alloc/mod.rs @@ -3,17 +3,14 @@ use zeroize::ZeroizeOnDrop; use crate::{store::backend::StoreBackend, KeyId}; +#[cfg(all(not(target_arch = "wasm32"), not(windows)))] mod malloc; - -pub(super) type MlockBackend = CustomAllocBackend; +#[cfg(all(not(target_arch = "wasm32"), not(windows)))] pub(super) use malloc::MlockAlloc; -#[cfg(all(target_os = "linux", not(feature = "no-memory-hardening")))] +#[cfg(target_os = "linux")] mod linux_memfd_secret; -#[cfg(all(target_os = "linux", not(feature = "no-memory-hardening")))] -pub(super) type LinuxMemfdSecretBackend = - CustomAllocBackend; -#[cfg(all(target_os = "linux", not(feature = "no-memory-hardening")))] +#[cfg(target_os = "linux")] pub(super) use linux_memfd_secret::LinuxMemfdSecretAlloc; pub(super) struct CustomAllocBackend { diff --git a/crates/bitwarden-crypto/src/store/backend/implementation/mod.rs b/crates/bitwarden-crypto/src/store/backend/implementation/mod.rs index 14aa4b219..7c9df1931 100644 --- a/crates/bitwarden-crypto/src/store/backend/implementation/mod.rs +++ b/crates/bitwarden-crypto/src/store/backend/implementation/mod.rs @@ -6,14 +6,19 @@ mod custom_alloc; /// Initializes a key store backend with the best available implementation for the current platform pub fn create_store() -> Box> { - #[cfg(all(target_os = "linux", not(feature = "no-memory-hardening")))] - if let Some(key_store) = custom_alloc::LinuxMemfdSecretAlloc::new() { - return Box::new(custom_alloc::LinuxMemfdSecretBackend::::new(key_store)); + if !cfg!(feature = "no-memory-hardening") { + #[cfg(target_os = "linux")] + if let Some(alloc) = custom_alloc::LinuxMemfdSecretAlloc::new() { + return Box::new(custom_alloc::CustomAllocBackend::new(alloc)); + } + + #[cfg(all(not(target_arch = "wasm32"), not(windows)))] + return Box::new(custom_alloc::CustomAllocBackend::new( + custom_alloc::MlockAlloc::new(), + )); } - Box::new(custom_alloc::MlockBackend::::new( - custom_alloc::MlockAlloc::new(), - )) + Box::new(basic::BasicBackend::new()) } #[cfg(test)] @@ -36,25 +41,26 @@ mod tests { ); } + #[cfg(all(not(target_arch = "wasm32"), not(windows)))] #[test] fn validate_mlock_store() { let basic_store: Box> = Box::new(basic::BasicBackend::new()); let mlock_store: Box> = Box::new( - custom_alloc::MlockBackend::::new(custom_alloc::MlockAlloc::new()), + custom_alloc::CustomAllocBackend::new(custom_alloc::MlockAlloc::new()), ); compare_stores(100_000, basic_store, mlock_store); } - #[cfg(all(target_os = "linux", not(feature = "no-memory-hardening")))] + #[cfg(target_os = "linux")] #[test] fn validate_memfd_store() { let basic_store: Box> = Box::new(basic::BasicBackend::new()); let mlock_store: Box> = - Box::new(custom_alloc::LinuxMemfdSecretBackend::::new( + Box::new(custom_alloc::CustomAllocBackend::new( custom_alloc::LinuxMemfdSecretAlloc::new().unwrap(), )); compare_stores(100_000, basic_store, mlock_store); From dfc2465294ef5786635f0c153f15b0858bfaca32 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garci=CC=81a?= Date: Fri, 2 May 2025 17:56:44 +0200 Subject: [PATCH 05/12] Fix memfd alignment --- .../custom_alloc/linux_memfd_secret.rs | 36 ++++++++++++++++--- .../src/store/backend/implementation/mod.rs | 10 +++--- crates/bitwarden-crypto/src/store/mod.rs | 2 +- crates/bitwarden-crypto/src/traits/mod.rs | 6 ++-- 4 files changed, 40 insertions(+), 14 deletions(-) diff --git a/crates/bitwarden-crypto/src/store/backend/implementation/custom_alloc/linux_memfd_secret.rs b/crates/bitwarden-crypto/src/store/backend/implementation/custom_alloc/linux_memfd_secret.rs index 19fd6eab5..226c5d42d 100644 --- a/crates/bitwarden-crypto/src/store/backend/implementation/custom_alloc/linux_memfd_secret.rs +++ b/crates/bitwarden-crypto/src/store/backend/implementation/custom_alloc/linux_memfd_secret.rs @@ -6,13 +6,24 @@ pub(crate) struct LinuxMemfdSecretAlloc; impl LinuxMemfdSecretAlloc { pub fn new() -> Option { - // To test if memfd_secret is supported, we try to allocate a 1 byte and see if that succeeds. + // To test if memfd_secret is supported, we try to allocate a 1 byte and see if that + // succeeds. static IS_SUPPORTED: LazyLock = LazyLock::new(|| { - let Some(ptr) = (unsafe { memsec::memfd_secret_sized(1) }) else { + let Some(ptr): Option> = (unsafe { memsec::memfd_secret_sized(1) }) + else { return false; }; + + // Check that the pointer is readable and writable + let result = unsafe { + let ptr = ptr.as_ptr() as *mut u8; + *ptr = 30; + *ptr += 107; + *ptr == 137 + }; + unsafe { memsec::free_memfd_secret(ptr) }; - true + result }); (*IS_SUPPORTED).then_some(Self) @@ -34,8 +45,23 @@ unsafe impl Allocator for LinuxMemfdSecretAlloc { }); } - let ptr: NonNull<[u8]> = unsafe { memsec::memfd_secret_sized(layout.size()) } - .expect("memfd_secret_sized failed"); + // Ensure the size we want to allocate is a multiple of the alignment, + // so that both the start and end of the allocation are aligned. + let layout = layout.pad_to_align(); + + let Some(ptr): Option> = + (unsafe { memsec::memfd_secret_sized(layout.size()) }) + else { + return Err(AllocError); + }; + + // Check that the pointer is aligned to the requested alignment. + // This should never happen, but just in case, we free the memory and return an allocation + // error. + if (ptr.as_ptr() as *mut u8).align_offset(layout.align()) != 0 { + unsafe { memsec::free_memfd_secret(ptr.as_ptr() as *mut u8) }; + return Err(AllocError); + } Ok(ptr) } diff --git a/crates/bitwarden-crypto/src/store/backend/implementation/mod.rs b/crates/bitwarden-crypto/src/store/backend/implementation/mod.rs index 7c9df1931..a0eb1fb41 100644 --- a/crates/bitwarden-crypto/src/store/backend/implementation/mod.rs +++ b/crates/bitwarden-crypto/src/store/backend/implementation/mod.rs @@ -59,11 +59,11 @@ mod tests { let basic_store: Box> = Box::new(basic::BasicBackend::new()); - let mlock_store: Box> = + let memfd_store: Box> = Box::new(custom_alloc::CustomAllocBackend::new( custom_alloc::LinuxMemfdSecretAlloc::new().unwrap(), )); - compare_stores(100_000, basic_store, mlock_store); + compare_stores(100_000, basic_store, memfd_store); } /// This function will perform a number of random operations on two stores, @@ -88,11 +88,11 @@ mod tests { fn sample(&self, rng: &mut R) -> Operation { match rng.gen_range(0..=4) { 0 => Operation::Upsert( - TestSymmKey::A(rng.gen_range(0..=10)), + TestSymmKey::A(rng.gen_range(0..=1000)), SymmetricCryptoKey::generate(rng), ), - 1 => Operation::Get(TestSymmKey::A(rng.gen_range(0..=10))), - 2 => Operation::Remove(TestSymmKey::A(rng.gen_range(0..=10))), + 1 => Operation::Get(TestSymmKey::A(rng.gen_range(0..=1000))), + 2 => Operation::Remove(TestSymmKey::A(rng.gen_range(0..=1000))), 3 => Operation::Clear, // This one has to be constant as we can't capture variables 4 => Operation::Retain(|key| match key { diff --git a/crates/bitwarden-crypto/src/store/mod.rs b/crates/bitwarden-crypto/src/store/mod.rs index 62b3650df..576a79c68 100644 --- a/crates/bitwarden-crypto/src/store/mod.rs +++ b/crates/bitwarden-crypto/src/store/mod.rs @@ -360,7 +360,7 @@ pub(crate) mod tests { // Create some test data let data: Vec<_> = (0..300usize) - .map(|n| DataView(format!("Test {}", n), TestSymmKey::A((n % 15) as u8))) + .map(|n| DataView(format!("Test {}", n), TestSymmKey::A((n % 15) as u32))) .collect(); // Encrypt the data diff --git a/crates/bitwarden-crypto/src/traits/mod.rs b/crates/bitwarden-crypto/src/traits/mod.rs index 28b811e36..e28fbe512 100644 --- a/crates/bitwarden-crypto/src/traits/mod.rs +++ b/crates/bitwarden-crypto/src/traits/mod.rs @@ -18,19 +18,19 @@ pub(crate) mod tests { key_ids! { #[symmetric] pub enum TestSymmKey { - A(u8), + A(u32), // We only support one variant value, // but that value can be a tuple B((u8, u8)), #[local] - C(u8), + C(u16), } #[asymmetric] pub enum TestAsymmKey { - A(u8), + A(u16), B, #[local] C(&'static str), From 560b6c600dfa0a813b8fe2d34179273b1a2130ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garci=CC=81a?= Date: Fri, 2 May 2025 18:01:06 +0200 Subject: [PATCH 06/12] Fix msrv and dealloc --- Cargo.toml | 2 +- .../backend/implementation/custom_alloc/linux_memfd_secret.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 88ba530c5..9040e3aaf 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -9,7 +9,7 @@ version = "1.0.0" authors = ["Bitwarden Inc"] edition = "2021" # Important: Changing rust-version should be considered a breaking change -rust-version = "1.75" +rust-version = "1.80" homepage = "https://bitwarden.com" repository = "https://github.com/bitwarden/sdk-internal" license-file = "LICENSE" diff --git a/crates/bitwarden-crypto/src/store/backend/implementation/custom_alloc/linux_memfd_secret.rs b/crates/bitwarden-crypto/src/store/backend/implementation/custom_alloc/linux_memfd_secret.rs index 226c5d42d..7eaf03312 100644 --- a/crates/bitwarden-crypto/src/store/backend/implementation/custom_alloc/linux_memfd_secret.rs +++ b/crates/bitwarden-crypto/src/store/backend/implementation/custom_alloc/linux_memfd_secret.rs @@ -59,7 +59,7 @@ unsafe impl Allocator for LinuxMemfdSecretAlloc { // This should never happen, but just in case, we free the memory and return an allocation // error. if (ptr.as_ptr() as *mut u8).align_offset(layout.align()) != 0 { - unsafe { memsec::free_memfd_secret(ptr.as_ptr() as *mut u8) }; + unsafe { memsec::free_memfd_secret(ptr) }; return Err(AllocError); } From 75870c42780b7a09051c065401d4179bf5657106 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garci=CC=81a?= Date: Fri, 2 May 2025 18:02:08 +0200 Subject: [PATCH 07/12] Use range --- crates/bitwarden-crypto/Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bitwarden-crypto/Cargo.toml b/crates/bitwarden-crypto/Cargo.toml index e7aa49c69..ce32e3ae3 100644 --- a/crates/bitwarden-crypto/Cargo.toml +++ b/crates/bitwarden-crypto/Cargo.toml @@ -57,7 +57,7 @@ zeroize = { version = ">=1.7.0, <2.0", features = ["derive", "aarch64"] } zeroizing-alloc = ">=0.1.0, <0.2" [target.'cfg(all(not(target_arch = "wasm32"), not(windows)))'.dependencies] -memsec = { version = "0.7.0", features = ["alloc_ext"] } +memsec = { version = ">=0.7.0, <0.8", features = ["alloc_ext"] } [dev-dependencies] criterion = "0.5.1" From f79bdb2ee601320cbbf660d30633f151c559fe7c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garci=CC=81a?= Date: Fri, 2 May 2025 18:05:44 +0200 Subject: [PATCH 08/12] Cfg --- crates/bitwarden-crypto/src/store/backend/implementation/mod.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/crates/bitwarden-crypto/src/store/backend/implementation/mod.rs b/crates/bitwarden-crypto/src/store/backend/implementation/mod.rs index a0eb1fb41..114451a7b 100644 --- a/crates/bitwarden-crypto/src/store/backend/implementation/mod.rs +++ b/crates/bitwarden-crypto/src/store/backend/implementation/mod.rs @@ -2,6 +2,8 @@ use super::StoreBackend; use crate::store::KeyId; mod basic; + +#[cfg(any(target_os = "linux", all(not(target_arch = "wasm32"), not(windows))))] mod custom_alloc; /// Initializes a key store backend with the best available implementation for the current platform From 3bb64d8206d27111fc104a5359017f33beb2c45c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garci=CC=81a?= Date: Fri, 2 May 2025 18:07:28 +0200 Subject: [PATCH 09/12] Cleanup --- .../src/store/backend/implementation/custom_alloc/mod.rs | 8 ++------ .../src/store/backend/implementation/mod.rs | 4 ++-- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/crates/bitwarden-crypto/src/store/backend/implementation/custom_alloc/mod.rs b/crates/bitwarden-crypto/src/store/backend/implementation/custom_alloc/mod.rs index f5e51cb9b..28f938ef4 100644 --- a/crates/bitwarden-crypto/src/store/backend/implementation/custom_alloc/mod.rs +++ b/crates/bitwarden-crypto/src/store/backend/implementation/custom_alloc/mod.rs @@ -4,14 +4,10 @@ use zeroize::ZeroizeOnDrop; use crate::{store::backend::StoreBackend, KeyId}; #[cfg(all(not(target_arch = "wasm32"), not(windows)))] -mod malloc; -#[cfg(all(not(target_arch = "wasm32"), not(windows)))] -pub(super) use malloc::MlockAlloc; +pub(super) mod malloc; #[cfg(target_os = "linux")] -mod linux_memfd_secret; -#[cfg(target_os = "linux")] -pub(super) use linux_memfd_secret::LinuxMemfdSecretAlloc; +pub(super) mod linux_memfd_secret; pub(super) struct CustomAllocBackend { map: hashbrown::HashMap, diff --git a/crates/bitwarden-crypto/src/store/backend/implementation/mod.rs b/crates/bitwarden-crypto/src/store/backend/implementation/mod.rs index 114451a7b..9585472bc 100644 --- a/crates/bitwarden-crypto/src/store/backend/implementation/mod.rs +++ b/crates/bitwarden-crypto/src/store/backend/implementation/mod.rs @@ -10,13 +10,13 @@ mod custom_alloc; pub fn create_store() -> Box> { if !cfg!(feature = "no-memory-hardening") { #[cfg(target_os = "linux")] - if let Some(alloc) = custom_alloc::LinuxMemfdSecretAlloc::new() { + if let Some(alloc) = custom_alloc::linux_memfd_secret::LinuxMemfdSecretAlloc::new() { return Box::new(custom_alloc::CustomAllocBackend::new(alloc)); } #[cfg(all(not(target_arch = "wasm32"), not(windows)))] return Box::new(custom_alloc::CustomAllocBackend::new( - custom_alloc::MlockAlloc::new(), + custom_alloc::malloc::MlockAlloc::new(), )); } From 489224749026f2d465cd7a028c32c8ba667f54c4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garci=CC=81a?= Date: Fri, 2 May 2025 18:09:18 +0200 Subject: [PATCH 10/12] Fix tests --- .../bitwarden-crypto/src/store/backend/implementation/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/bitwarden-crypto/src/store/backend/implementation/mod.rs b/crates/bitwarden-crypto/src/store/backend/implementation/mod.rs index 9585472bc..c9cd7a1f3 100644 --- a/crates/bitwarden-crypto/src/store/backend/implementation/mod.rs +++ b/crates/bitwarden-crypto/src/store/backend/implementation/mod.rs @@ -50,7 +50,7 @@ mod tests { Box::new(basic::BasicBackend::new()); let mlock_store: Box> = Box::new( - custom_alloc::CustomAllocBackend::new(custom_alloc::MlockAlloc::new()), + custom_alloc::CustomAllocBackend::new(custom_alloc::malloc::MlockAlloc::new()), ); compare_stores(100_000, basic_store, mlock_store); } @@ -63,7 +63,7 @@ mod tests { let memfd_store: Box> = Box::new(custom_alloc::CustomAllocBackend::new( - custom_alloc::LinuxMemfdSecretAlloc::new().unwrap(), + custom_alloc::linux_memfd_secret::LinuxMemfdSecretAlloc::new().unwrap(), )); compare_stores(100_000, basic_store, memfd_store); } From a3dd15968c009d8f48b9df0dfb7238c66ec23c4e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garci=CC=81a?= Date: Thu, 12 Jun 2025 18:48:13 +0200 Subject: [PATCH 11/12] Fix test --- crates/bitwarden-crypto/src/store/backend/implementation/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bitwarden-crypto/src/store/backend/implementation/mod.rs b/crates/bitwarden-crypto/src/store/backend/implementation/mod.rs index 66bee5e41..0339d29ff 100644 --- a/crates/bitwarden-crypto/src/store/backend/implementation/mod.rs +++ b/crates/bitwarden-crypto/src/store/backend/implementation/mod.rs @@ -91,7 +91,7 @@ mod tests { match rng.gen_range(0..=4) { 0 => Operation::Upsert( TestSymmKey::A(rng.gen_range(0..=1000)), - SymmetricCryptoKey::generate(rng), + SymmetricCryptoKey::make_aes256_cbc_hmac_key(), ), 1 => Operation::Get(TestSymmKey::A(rng.gen_range(0..=1000))), 2 => Operation::Remove(TestSymmKey::A(rng.gen_range(0..=1000))), From 01925f78929dde3de2194eaf9de63d51db3457bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garci=CC=81a?= Date: Mon, 23 Jun 2025 14:57:10 +0200 Subject: [PATCH 12/12] Improve comments --- Cargo.lock | 7 ++++++- .../implementation/custom_alloc/linux_memfd_secret.rs | 8 +++++--- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 89609f50a..2cdc63718 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -409,7 +409,7 @@ dependencies = [ "criterion", "ed25519-dalek", "generic-array", - "hashbrown 0.15.3", + "hashbrown 0.15.4", "hkdf", "hmac", "memsec", @@ -1868,6 +1868,11 @@ name = "hashbrown" version = "0.15.4" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "5971ac85611da7067dbfcabef3c70ebb5606018acd9e2a3903a0da507521e0d5" +dependencies = [ + "allocator-api2", + "equivalent", + "foldhash", +] [[package]] name = "heck" diff --git a/crates/bitwarden-crypto/src/store/backend/implementation/custom_alloc/linux_memfd_secret.rs b/crates/bitwarden-crypto/src/store/backend/implementation/custom_alloc/linux_memfd_secret.rs index 7eaf03312..c9207145f 100644 --- a/crates/bitwarden-crypto/src/store/backend/implementation/custom_alloc/linux_memfd_secret.rs +++ b/crates/bitwarden-crypto/src/store/backend/implementation/custom_alloc/linux_memfd_secret.rs @@ -55,9 +55,11 @@ unsafe impl Allocator for LinuxMemfdSecretAlloc { return Err(AllocError); }; - // Check that the pointer is aligned to the requested alignment. - // This should never happen, but just in case, we free the memory and return an allocation - // error. + // The pointer that we return needs to be aligned to the requested alignment. + // If that's not the case, we should free the memory and return an allocation error. + // While we check for this error condition, this should never happen in practice, as the + // pointer returned by `memfd_secret_sized` should be page-aligned (typically 4KB) + // which should be larger than any possible alignment value. if (ptr.as_ptr() as *mut u8).align_offset(layout.align()) != 0 { unsafe { memsec::free_memfd_secret(ptr) }; return Err(AllocError);