diff --git a/apps/desktop/desktop_native/core/src/biometric_v2/linux.rs b/apps/desktop/desktop_native/core/src/biometric_v2/linux.rs index ff2abc0686b..ef6527e7b26 100644 --- a/apps/desktop/desktop_native/core/src/biometric_v2/linux.rs +++ b/apps/desktop/desktop_native/core/src/biometric_v2/linux.rs @@ -19,20 +19,18 @@ use tracing::{debug, warn}; use zbus::Connection; use zbus_polkit::policykit1::{AuthorityProxy, CheckAuthorizationFlags, Subject}; -use crate::secure_memory::*; +use crate::secure_memory::{encrypted_memory_store::EncryptedMemoryStore, SecureMemoryStore as _}; pub struct BiometricLockSystem { // The userkeys that are held in memory MUST be protected from memory dumping attacks, to // ensure locked vaults cannot be unlocked - secure_memory: Arc>, + secure_memory: Arc>>, } impl BiometricLockSystem { pub fn new() -> Self { Self { - secure_memory: Arc::new(Mutex::new( - crate::secure_memory::encrypted_memory_store::EncryptedMemoryStore::new(), - )), + secure_memory: Arc::new(Mutex::new(EncryptedMemoryStore::default())), } } } @@ -64,7 +62,7 @@ impl super::BiometricTrait for BiometricLockSystem { .put(user_id.to_string(), key); } - async fn unlock(&self, user_id: &str, _hwnd: Vec) -> Result> { + async fn unlock(&self, user_id: &String, _hwnd: Vec) -> Result> { if !polkit_authenticate_bitwarden_policy().await? { return Err(anyhow!("Authentication failed")); } @@ -72,11 +70,11 @@ impl super::BiometricTrait for BiometricLockSystem { self.secure_memory .lock() .await - .get(user_id) + .get(user_id)? .ok_or(anyhow!("No key found")) } - async fn unlock_available(&self, user_id: &str) -> Result { + async fn unlock_available(&self, user_id: &String) -> Result { Ok(self.secure_memory.lock().await.has(user_id)) } @@ -84,7 +82,7 @@ impl super::BiometricTrait for BiometricLockSystem { Ok(false) } - async fn unenroll(&self, user_id: &str) -> Result<(), anyhow::Error> { + async fn unenroll(&self, user_id: &String) -> Result<(), anyhow::Error> { self.secure_memory.lock().await.remove(user_id); Ok(()) } diff --git a/apps/desktop/desktop_native/core/src/biometric_v2/mod.rs b/apps/desktop/desktop_native/core/src/biometric_v2/mod.rs index 55aee27dd33..d577a2a0c0b 100644 --- a/apps/desktop/desktop_native/core/src/biometric_v2/mod.rs +++ b/apps/desktop/desktop_native/core/src/biometric_v2/mod.rs @@ -21,14 +21,17 @@ pub trait BiometricTrait: Send + Sync { /// enrollment, this function should do nothing. async fn enroll_persistent(&self, user_id: &str, key: &[u8]) -> Result<()>; /// Clear the persistent and ephemeral keys - async fn unenroll(&self, user_id: &str) -> Result<()>; + #[allow(clippy::ptr_arg)] // to allow using user_id as map key type + async fn unenroll(&self, user_id: &String) -> Result<()>; /// Check if a persistent (survives app restarts and reboots) key is set for a user async fn has_persistent(&self, user_id: &str) -> Result; /// Provide a key to be ephemerally held. This should be called on every unlock. async fn provide_key(&self, user_id: &str, key: &[u8]); /// Perform biometric unlock and return the key - async fn unlock(&self, user_id: &str, hwnd: Vec) -> Result>; + #[allow(clippy::ptr_arg)] // to allow using user_id as map key type + async fn unlock(&self, user_id: &String, hwnd: Vec) -> Result>; /// Check if biometric unlock is available based on whether a key is present and whether /// authentication is possible - async fn unlock_available(&self, user_id: &str) -> Result; + #[allow(clippy::ptr_arg)] // to allow using user_id as map key type + async fn unlock_available(&self, user_id: &String) -> Result; } diff --git a/apps/desktop/desktop_native/core/src/biometric_v2/unimplemented.rs b/apps/desktop/desktop_native/core/src/biometric_v2/unimplemented.rs index 1503cfea89c..02ac435da3c 100644 --- a/apps/desktop/desktop_native/core/src/biometric_v2/unimplemented.rs +++ b/apps/desktop/desktop_native/core/src/biometric_v2/unimplemented.rs @@ -29,11 +29,11 @@ impl super::BiometricTrait for BiometricLockSystem { unimplemented!() } - async fn unlock(&self, _user_id: &str, _hwnd: Vec) -> Result, anyhow::Error> { + async fn unlock(&self, _user_id: &String, _hwnd: Vec) -> Result, anyhow::Error> { unimplemented!() } - async fn unlock_available(&self, _user_id: &str) -> Result { + async fn unlock_available(&self, _user_id: &String) -> Result { unimplemented!() } @@ -41,7 +41,7 @@ impl super::BiometricTrait for BiometricLockSystem { unimplemented!() } - async fn unenroll(&self, _user_id: &str) -> Result<(), anyhow::Error> { + async fn unenroll(&self, _user_id: &String) -> Result<(), anyhow::Error> { unimplemented!() } } diff --git a/apps/desktop/desktop_native/core/src/biometric_v2/windows.rs b/apps/desktop/desktop_native/core/src/biometric_v2/windows.rs index 669dd757c40..914bf50c20e 100644 --- a/apps/desktop/desktop_native/core/src/biometric_v2/windows.rs +++ b/apps/desktop/desktop_native/core/src/biometric_v2/windows.rs @@ -110,7 +110,7 @@ impl super::BiometricTrait for BiometricLockSystem { } } - async fn unenroll(&self, user_id: &str) -> Result<()> { + async fn unenroll(&self, user_id: &String) -> Result<()> { self.secure_memory.lock().await.remove(user_id); delete_keychain_entry(user_id).await } @@ -148,7 +148,7 @@ impl super::BiometricTrait for BiometricLockSystem { .put(user_id.to_string(), key); } - async fn unlock(&self, user_id: &str, _hwnd: Vec) -> Result> { + async fn unlock(&self, user_id: &String, _hwnd: Vec) -> Result> { // Allow restoring focus to the previous window (browser) let previous_active_window = super::windows_focus::get_active_window(); let _focus_scopeguard = scopeguard::guard((), |_| { @@ -164,8 +164,7 @@ impl super::BiometricTrait for BiometricLockSystem { if secure_memory.has(user_id) { if windows_hello_authenticate("Unlock your vault".to_string()).await? { secure_memory - .get(user_id) - .clone() + .get(user_id)? .ok_or_else(|| anyhow!("No key found for user")) } else { Err(anyhow!("Authentication failed")) @@ -186,7 +185,7 @@ impl super::BiometricTrait for BiometricLockSystem { } } - async fn unlock_available(&self, user_id: &str) -> Result { + async fn unlock_available(&self, user_id: &String) -> Result { let secure_memory = self.secure_memory.lock().await; let has_key = secure_memory.has(user_id) || has_keychain_entry(user_id).await.unwrap_or(false); @@ -435,7 +434,7 @@ mod tests { #[tokio::test] #[ignore] async fn test_double_unenroll() { - let user_id = "test_user"; + let user_id = String::from("test_user"); let mut key = [0u8; XCHACHA20POLY1305_KEY_LENGTH]; rand::fill(&mut key); @@ -443,34 +442,34 @@ mod tests { println!("Enrolling user"); windows_hello_lock_system - .enroll_persistent(user_id, &key) + .enroll_persistent(&user_id, &key) .await .unwrap(); assert!(windows_hello_lock_system - .has_persistent(user_id) + .has_persistent(&user_id) .await .unwrap()); println!("Unlocking user"); let key_after_unlock = windows_hello_lock_system - .unlock(user_id, Vec::new()) + .unlock(&user_id, Vec::new()) .await .unwrap(); assert_eq!(key_after_unlock, key); println!("Unenrolling user"); - windows_hello_lock_system.unenroll(user_id).await.unwrap(); + windows_hello_lock_system.unenroll(&user_id).await.unwrap(); assert!(!windows_hello_lock_system - .has_persistent(user_id) + .has_persistent(&user_id) .await .unwrap()); println!("Unenrolling user again"); // This throws PASSWORD_NOT_FOUND but our code should handle that and not throw. - windows_hello_lock_system.unenroll(user_id).await.unwrap(); + windows_hello_lock_system.unenroll(&user_id).await.unwrap(); assert!(!windows_hello_lock_system - .has_persistent(user_id) + .has_persistent(&user_id) .await .unwrap()); } @@ -478,7 +477,7 @@ mod tests { #[tokio::test] #[ignore] async fn test_enroll_unlock_unenroll() { - let user_id = "test_user"; + let user_id = String::from("test_user"); let mut key = [0u8; XCHACHA20POLY1305_KEY_LENGTH]; rand::fill(&mut key); @@ -486,25 +485,25 @@ mod tests { println!("Enrolling user"); windows_hello_lock_system - .enroll_persistent(user_id, &key) + .enroll_persistent(&user_id, &key) .await .unwrap(); assert!(windows_hello_lock_system - .has_persistent(user_id) + .has_persistent(&user_id) .await .unwrap()); println!("Unlocking user"); let key_after_unlock = windows_hello_lock_system - .unlock(user_id, Vec::new()) + .unlock(&user_id, Vec::new()) .await .unwrap(); assert_eq!(key_after_unlock, key); println!("Unenrolling user"); - windows_hello_lock_system.unenroll(user_id).await.unwrap(); + windows_hello_lock_system.unenroll(&user_id).await.unwrap(); assert!(!windows_hello_lock_system - .has_persistent(user_id) + .has_persistent(&user_id) .await .unwrap()); } diff --git a/apps/desktop/desktop_native/core/src/lib.rs b/apps/desktop/desktop_native/core/src/lib.rs index 668badb95ed..b6a558b7152 100644 --- a/apps/desktop/desktop_native/core/src/lib.rs +++ b/apps/desktop/desktop_native/core/src/lib.rs @@ -9,7 +9,7 @@ pub mod ipc; pub mod password; pub mod powermonitor; pub mod process_isolation; -pub(crate) mod secure_memory; +pub mod secure_memory; pub mod ssh_agent; use zeroizing_alloc::ZeroAlloc; diff --git a/apps/desktop/desktop_native/core/src/secure_memory/dpapi.rs b/apps/desktop/desktop_native/core/src/secure_memory/dpapi.rs index 8d8e10d92c4..45b8936a17f 100644 --- a/apps/desktop/desktop_native/core/src/secure_memory/dpapi.rs +++ b/apps/desktop/desktop_native/core/src/secure_memory/dpapi.rs @@ -5,7 +5,7 @@ use windows::Win32::Security::Cryptography::{ CRYPTPROTECTMEMORY_SAME_PROCESS, }; -use crate::secure_memory::SecureMemoryStore; +use crate::secure_memory::{DecryptionError, SecureMemoryStore}; /// https://learn.microsoft.com/en-us/windows/win32/api/dpapi/nf-dpapi-cryptprotectdata /// The DPAPI store encrypts data using the Windows Data Protection API (DPAPI). The key is bound @@ -26,7 +26,9 @@ impl DpapiSecretKVStore { } impl SecureMemoryStore for DpapiSecretKVStore { - fn put(&mut self, key: String, value: &[u8]) { + type KeyType = String; + + fn put(&mut self, key: Self::KeyType, value: &[u8]) { let length_header_len = std::mem::size_of::(); // The allocated data has to be a multiple of CRYPTPROTECTMEMORY_BLOCK_SIZE, so we pad it @@ -55,8 +57,8 @@ impl SecureMemoryStore for DpapiSecretKVStore { self.map.insert(key, padded_data); } - fn get(&mut self, key: &str) -> Option> { - self.map.get(key).map(|data| { + fn get(&mut self, key: &Self::KeyType) -> Result>, DecryptionError> { + if let Some(data) = self.map.get(key) { // A copy is created, that is then mutated by the DPAPI unprotect function. let mut data = data.clone(); unsafe { @@ -77,15 +79,19 @@ impl SecureMemoryStore for DpapiSecretKVStore { .expect("length header should be usize"), ); - data[length_header_size..length_header_size + data_length].to_vec() - }) + Ok(Some( + data[length_header_size..length_header_size + data_length].to_vec(), + )) + } else { + Ok(None) + } } - fn has(&self, key: &str) -> bool { + fn has(&self, key: &Self::KeyType) -> bool { self.map.contains_key(key) } - fn remove(&mut self, key: &str) { + fn remove(&mut self, key: &Self::KeyType) { self.map.remove(key); } @@ -113,7 +119,7 @@ mod tests { store.put(key.clone(), &value); assert!(store.has(&key), "Store should have key for size {}", size); assert_eq!( - store.get(&key), + store.get(&key).expect("entry in map for key"), Some(value), "Value mismatch for size {}", size @@ -128,7 +134,7 @@ mod tests { let value = vec![1, 2, 3, 4, 5]; store.put(key.clone(), &value); assert!(store.has(&key)); - assert_eq!(store.get(&key), Some(value)); + assert_eq!(store.get(&key).expect("entry in map for key"), Some(value)); store.remove(&key); assert!(!store.has(&key)); } diff --git a/apps/desktop/desktop_native/core/src/secure_memory/encrypted_memory_store.rs b/apps/desktop/desktop_native/core/src/secure_memory/encrypted_memory_store.rs index d116e564bc8..8961b63ccee 100644 --- a/apps/desktop/desktop_native/core/src/secure_memory/encrypted_memory_store.rs +++ b/apps/desktop/desktop_native/core/src/secure_memory/encrypted_memory_store.rs @@ -1,7 +1,9 @@ +use std::collections::BTreeMap; + use tracing::error; use crate::secure_memory::{ - secure_key::{EncryptedMemory, SecureMemoryEncryptionKey}, + secure_key::{DecryptionError, EncryptedMemory, SecureMemoryEncryptionKey}, SecureMemoryStore, }; @@ -12,50 +14,87 @@ use crate::secure_memory::{ /// /// The key is briefly in process memory during encryption and decryption, in memory that is /// protected from swapping to disk via mlock, and then zeroed out immediately after use. -#[allow(unused)] -pub(crate) struct EncryptedMemoryStore { - map: std::collections::HashMap, +/// # Type Parameters +/// +/// * `K` - The type of the key. +pub struct EncryptedMemoryStore +where + K: std::cmp::Ord + std::fmt::Display + std::clone::Clone, +{ + map: BTreeMap, memory_encryption_key: SecureMemoryEncryptionKey, } -impl EncryptedMemoryStore { - #[allow(unused)] - pub(crate) fn new() -> Self { +impl EncryptedMemoryStore +where + K: std::cmp::Ord + std::fmt::Display + std::clone::Clone, +{ + #[must_use] + pub fn new() -> Self { EncryptedMemoryStore { - map: std::collections::HashMap::new(), + map: BTreeMap::new(), memory_encryption_key: SecureMemoryEncryptionKey::new(), } } + + /// # Returns + /// + /// An array of all decrypted values. + /// Due to the usage of `BtreeMap`, the order is deterministic. + /// + /// # Errors + /// + /// `DecryptionError` if an error occured during decryption + pub fn to_vec(&mut self) -> Result>, DecryptionError> { + let mut result = vec![]; + let keys: Vec<_> = self.map.keys().cloned().collect(); + + for key in &keys { + let bytes = self.get(key)?.expect("All keys to still be in map."); + result.push(bytes); + } + Ok(result) + } } -impl SecureMemoryStore for EncryptedMemoryStore { - fn put(&mut self, key: String, value: &[u8]) { +impl Default for EncryptedMemoryStore +where + K: std::cmp::Ord + std::fmt::Display + std::clone::Clone, +{ + fn default() -> Self { + Self::new() + } +} + +impl SecureMemoryStore for EncryptedMemoryStore +where + K: std::cmp::Ord + std::fmt::Display + std::clone::Clone, +{ + type KeyType = K; + + fn put(&mut self, key: Self::KeyType, value: &[u8]) { let encrypted_value = self.memory_encryption_key.encrypt(value); self.map.insert(key, encrypted_value); } - fn get(&mut self, key: &str) -> Option> { - let encrypted_memory = self.map.get(key); - if let Some(encrypted_memory) = encrypted_memory { - match self.memory_encryption_key.decrypt(encrypted_memory) { - Ok(plaintext) => Some(plaintext), - Err(_) => { - error!("In memory store, decryption failed for key {}. The memory may have been tampered with. re-keying.", key); - self.memory_encryption_key = SecureMemoryEncryptionKey::new(); - self.clear(); - None - } - } + fn get(&mut self, key: &Self::KeyType) -> Result>, DecryptionError> { + if let Some(encrypted) = self.map.get(key) { + self.memory_encryption_key.decrypt(encrypted).map_err(|error| { + error!(?error, %key, "In memory store, decryption failed. The memory may have been tampered with. Re-keying."); + self.memory_encryption_key = SecureMemoryEncryptionKey::new(); + self.clear(); + error + }).map(Some) } else { - None + Ok(None) } } - fn has(&self, key: &str) -> bool { + fn has(&self, key: &Self::KeyType) -> bool { self.map.contains_key(key) } - fn remove(&mut self, key: &str) { + fn remove(&mut self, key: &Self::KeyType) { self.map.remove(key); } @@ -64,7 +103,10 @@ impl SecureMemoryStore for EncryptedMemoryStore { } } -impl Drop for EncryptedMemoryStore { +impl Drop for EncryptedMemoryStore +where + K: std::cmp::Ord + std::fmt::Display + std::clone::Clone, +{ fn drop(&mut self) { self.clear(); } @@ -76,30 +118,98 @@ mod tests { #[test] fn test_secret_kv_store_various_sizes() { - let mut store = EncryptedMemoryStore::new(); + let mut store = EncryptedMemoryStore::default(); for size in 0..=2048 { - let key = format!("test_key_{}", size); + let key = format!("test_key_{size}"); let value: Vec = (0..size).map(|i| (i % 256) as u8).collect(); store.put(key.clone(), &value); - assert!(store.has(&key), "Store should have key for size {}", size); + assert!(store.has(&key), "Store should have key for size {size}"); assert_eq!( - store.get(&key), + store.get(&key).expect("entry in map for key"), Some(value), - "Value mismatch for size {}", - size + "Value mismatch for size {size}", ); } } #[test] fn test_crud() { - let mut store = EncryptedMemoryStore::new(); + let mut store = EncryptedMemoryStore::default(); let key = "test_key".to_string(); let value = vec![1, 2, 3, 4, 5]; store.put(key.clone(), &value); assert!(store.has(&key)); - assert_eq!(store.get(&key), Some(value)); + assert_eq!(store.get(&key).expect("entry in map for key"), Some(value)); store.remove(&key); assert!(!store.has(&key)); } + + #[test] + fn test_to_vec_contains_all() { + let mut store = EncryptedMemoryStore::default(); + + for size in 0..=2048 { + let key = format!("test_key_{size}"); + let value: Vec = (0..size).map(|i| (i % 256) as u8).collect(); + store.put(key.clone(), &value); + } + let vec_values = store.to_vec().expect("decryption to not fail"); + + // to_vec() should contain same number of values as inserted + assert_eq!(vec_values.len(), 2049); + + // the value from the store should match the value in the vec + let keys: Vec<_> = store.map.keys().cloned().collect(); + for (store_key, vec_value) in keys.iter().zip(vec_values.iter()) { + let store_value = store.get(store_key).expect("entry in map for key").unwrap(); + assert_eq!(&store_value, vec_value); + store.remove(store_key); + } + + // all values were present + assert!(store.map.is_empty()); + } + + #[test] + fn test_to_vec_preserves_sorted_key_order() { + let mut store = EncryptedMemoryStore::new(); + + // insert in non-sorted order + store.put("morpheus", &[4, 5, 6]); + store.put("trinity", &[1, 2, 3]); + store.put("dozer", &[7, 8, 9]); + store.put("neo", &[10, 11, 12]); + + let vec = store.to_vec().expect("decryption to not fail"); + + assert_eq!( + vec, + vec![ + vec![7, 8, 9], // dozer + vec![4, 5, 6], // morpheus + vec![10, 11, 12], // neo + vec![1, 2, 3], // trinity + ] + ); + } + + #[test] + fn test_to_vec_order_after_remove() { + let mut store = EncryptedMemoryStore::new(); + + // insert in non-sorted order + store.put("trinity", &[3]); + store.put("morpheus", &[1]); + store.put("neo", &[2]); + + let vec = store.to_vec().expect("decryption to not fail"); + + assert_eq!(vec, vec![vec![1], vec![2], vec![3]]); + + store.remove(&"neo"); + + let vec = store.to_vec().expect("decryption to not fail"); + + assert_eq!(vec, vec![vec![1], vec![3]]); + } } diff --git a/apps/desktop/desktop_native/core/src/secure_memory/mod.rs b/apps/desktop/desktop_native/core/src/secure_memory/mod.rs index d4323ce40dd..b5c3bcdccd9 100644 --- a/apps/desktop/desktop_native/core/src/secure_memory/mod.rs +++ b/apps/desktop/desktop_native/core/src/secure_memory/mod.rs @@ -4,24 +4,35 @@ pub(crate) mod dpapi; pub(crate) mod encrypted_memory_store; mod secure_key; +pub use encrypted_memory_store::EncryptedMemoryStore; + +use crate::secure_memory::secure_key::DecryptionError; + /// The secure memory store provides an ephemeral key-value store for sensitive data. /// Data stored in this store is prevented from being swapped to disk and zeroed out. Additionally, /// platform-specific protections are applied to prevent memory dumps or debugger access from /// reading the stored values. -#[allow(unused)] -pub(crate) trait SecureMemoryStore { +pub trait SecureMemoryStore { + type KeyType; + /// Stores a copy of the provided value in secure memory. - fn put(&mut self, key: String, value: &[u8]); + fn put(&mut self, key: Self::KeyType, value: &[u8]); + /// Retrieves a copy of the value associated with the given key from secure memory. /// This copy does not have additional memory protections applied, and should be zeroed when no /// longer needed. /// - /// Note: If memory was tampered with, this will re-key the store and return None. - fn get(&mut self, key: &str) -> Option>; + /// # Errors + /// + /// `DecryptionError` if memory is tampered with. This also re-keys the memory store. + fn get(&mut self, key: &Self::KeyType) -> Result>, DecryptionError>; + /// Checks if a value is stored under the given key. - fn has(&self, key: &str) -> bool; + fn has(&self, key: &Self::KeyType) -> bool; + /// Removes the value associated with the given key from secure memory. - fn remove(&mut self, key: &str); + fn remove(&mut self, key: &Self::KeyType); + /// Clears all values stored in secure memory. fn clear(&mut self); } diff --git a/apps/desktop/desktop_native/core/src/secure_memory/secure_key/crypto.rs b/apps/desktop/desktop_native/core/src/secure_memory/secure_key/crypto.rs index 7e2917ade6d..7fb3bcc299a 100644 --- a/apps/desktop/desktop_native/core/src/secure_memory/secure_key/crypto.rs +++ b/apps/desktop/desktop_native/core/src/secure_memory/secure_key/crypto.rs @@ -77,10 +77,20 @@ impl AsRef<[u8]> for MemoryEncryptionKey { } #[derive(Debug)] -pub(crate) enum DecryptionError { +pub enum DecryptionError { CouldNotDecrypt, } +impl std::error::Error for DecryptionError {} + +impl std::fmt::Display for DecryptionError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + DecryptionError::CouldNotDecrypt => write!(f, "Could not decrypt"), + } + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/apps/desktop/desktop_native/core/src/secure_memory/secure_key/mod.rs b/apps/desktop/desktop_native/core/src/secure_memory/secure_key/mod.rs index 26e72f7d581..8050157a5f3 100644 --- a/apps/desktop/desktop_native/core/src/secure_memory/secure_key/mod.rs +++ b/apps/desktop/desktop_native/core/src/secure_memory/secure_key/mod.rs @@ -19,9 +19,7 @@ mod keyctl; mod memfd_secret; mod mlock; -pub use crypto::EncryptedMemory; - -use crate::secure_memory::secure_key::crypto::DecryptionError; +pub use crypto::{DecryptionError, EncryptedMemory}; /// An ephemeral key that is protected using a platform mechanism. It is generated on construction /// freshly, and can be used to encrypt and decrypt segments of memory. Since the key is ephemeral,