bitwarden/browser
1
0
Fork 0
mirror of https://github.com/bitwarden/browser synced 2026-02-05 19:23:19 +00:00
Code Issues Releases Wiki Activity

Enhancements to EncryptedMemoryStore (#18484)

Browse Source
This commit is contained in:
neuronull
2026-02-03 14:04:18 -08:00
committed by GitHub
parent 3f5ca7155b
commit e5c9f9398d
10 changed files with 225 additions and 90 deletions
Download Patch File Download Diff File Expand all files Collapse all files

16
apps/desktop/desktop_native/core/src/biometric_v2/linux.rs
View File

@@ -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<Mutex<crate::secure_memory::encrypted_memory_store::EncryptedMemoryStore>>,
secure_memory: Arc<Mutex<EncryptedMemoryStore<String>>>,
}
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<u8>) -> Result<Vec<u8>> {
async fn unlock(&self, user_id: &String, _hwnd: Vec<u8>) -> Result<Vec<u8>> {
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<bool> {
async fn unlock_available(&self, user_id: &String) -> Result<bool> {
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(())
}

9
apps/desktop/desktop_native/core/src/biometric_v2/mod.rs
View File

@@ -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<bool>;
/// 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<u8>) -> Result<Vec<u8>>;
#[allow(clippy::ptr_arg)] // to allow using user_id as map key type
async fn unlock(&self, user_id: &String, hwnd: Vec<u8>) -> Result<Vec<u8>>;
/// 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<bool>;
#[allow(clippy::ptr_arg)] // to allow using user_id as map key type
async fn unlock_available(&self, user_id: &String) -> Result<bool>;
}

6
apps/desktop/desktop_native/core/src/biometric_v2/unimplemented.rs
View File

@@ -29,11 +29,11 @@ impl super::BiometricTrait for BiometricLockSystem {
unimplemented!()
}
async fn unlock(&self, _user_id: &str, _hwnd: Vec<u8>) -> Result<Vec<u8>, anyhow::Error> {
async fn unlock(&self, _user_id: &String, _hwnd: Vec<u8>) -> Result<Vec<u8>, anyhow::Error> {
unimplemented!()
}
async fn unlock_available(&self, _user_id: &str) -> Result<bool, anyhow::Error> {
async fn unlock_available(&self, _user_id: &String) -> Result<bool, anyhow::Error> {
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!()
}
}

37
apps/desktop/desktop_native/core/src/biometric_v2/windows.rs
View File

@@ -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<u8>) -> Result<Vec<u8>> {
async fn unlock(&self, user_id: &String, _hwnd: Vec<u8>) -> Result<Vec<u8>> {
// 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<bool> {
async fn unlock_available(&self, user_id: &String) -> Result<bool> {
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());
}

2
apps/desktop/desktop_native/core/src/lib.rs
View File

@@ -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;

26
apps/desktop/desktop_native/core/src/secure_memory/dpapi.rs
View File

@@ -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::<usize>();
// 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<Vec<u8>> {
self.map.get(key).map(|data| {
fn get(&mut self, key: &Self::KeyType) -> Result<Option<Vec<u8>>, 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));
}

178
apps/desktop/desktop_native/core/src/secure_memory/encrypted_memory_store.rs
View File

@@ -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<String, EncryptedMemory>,
/// # Type Parameters
///
/// * `K` - The type of the key.
pub struct EncryptedMemoryStore<K>
where
K: std::cmp::Ord + std::fmt::Display + std::clone::Clone,
{
map: BTreeMap<K, EncryptedMemory>,
memory_encryption_key: SecureMemoryEncryptionKey,
}
impl EncryptedMemoryStore {
#[allow(unused)]
pub(crate) fn new() -> Self {
impl<K> EncryptedMemoryStore<K>
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<Vec<Vec<u8>>, 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<K> Default for EncryptedMemoryStore<K>
where
K: std::cmp::Ord + std::fmt::Display + std::clone::Clone,
{
fn default() -> Self {
Self::new()
}
}
impl<K> SecureMemoryStore for EncryptedMemoryStore<K>
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<Vec<u8>> {
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<Option<Vec<u8>>, 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<K> Drop for EncryptedMemoryStore<K>
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<u8> = (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<u8> = (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]]);
}
}

25
apps/desktop/desktop_native/core/src/secure_memory/mod.rs
View File

@@ -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<Vec<u8>>;
/// # Errors
///
/// `DecryptionError` if memory is tampered with. This also re-keys the memory store.
fn get(&mut self, key: &Self::KeyType) -> Result<Option<Vec<u8>>, 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);
}

12
apps/desktop/desktop_native/core/src/secure_memory/secure_key/crypto.rs
View File

@@ -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::*;

4
apps/desktop/desktop_native/core/src/secure_memory/secure_key/mod.rs
View File

@@ -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,