From 7f86f2d0ac69fb3d952a2cbfc95e1b8fd75d302e Mon Sep 17 00:00:00 2001 From: Bernd Schoolmann Date: Thu, 23 Oct 2025 14:04:25 +0200 Subject: [PATCH] [PM-26340] Implement encrypted memory store (#16659) * Extract windows biometrics v2 changes Co-authored-by: Bernd Schoolmann * Address some code review feedback * cargo fmt * rely on zeroizing allocator * Handle TDE edge cases * Update windows default * Make windows rust code async and fix restoring focus freezes * fix formatting * cleanup native logging * Add unit test coverage * Add missing logic to edge case for PIN disable. * Address code review feedback * fix test * code review changes * fix clippy warning * Swap to unimplemented on each method * Implement encrypted memory store * Make dpapi secure key container pub(super) * Add comments on sync and send * Clean up comments * Clean up * Fix build * Add logging and update codeowners * Run cargo fmt * Clean up doc * fix unit tests * Update apps/desktop/desktop_native/core/src/secure_memory/secure_key/mod.rs Co-authored-by: Thomas Avery <43214426+Thomas-Avery@users.noreply.github.com> * Handle tampering with re-key and log * Add docs * Fix windows build * Prevent rust flycheck log from being commited to git * Undo feature flag change * Add env var override and docs * Add deps to km owership --------- Co-authored-by: Thomas Avery Co-authored-by: Thomas Avery <43214426+Thomas-Avery@users.noreply.github.com> --- .github/CODEOWNERS | 1 + .github/renovate.json5 | 11 +- .gitignore | 1 + apps/desktop/desktop_native/Cargo.lock | 93 ++++++- apps/desktop/desktop_native/Cargo.toml | 2 + apps/desktop/desktop_native/core/Cargo.toml | 2 + .../core/src/secure_memory/dpapi.rs | 2 +- .../secure_memory/encrypted_memory_store.rs | 105 ++++++++ .../core/src/secure_memory/mod.rs | 7 +- .../src/secure_memory/secure_key/crypto.rs | 96 +++++++ .../src/secure_memory/secure_key/dpapi.rs | 93 +++++++ .../src/secure_memory/secure_key/keyctl.rs | 100 ++++++++ .../secure_memory/secure_key/memfd_secret.rs | 109 ++++++++ .../src/secure_memory/secure_key/mlock.rs | 83 ++++++ .../core/src/secure_memory/secure_key/mod.rs | 242 ++++++++++++++++++ 15 files changed, 942 insertions(+), 5 deletions(-) create mode 100644 apps/desktop/desktop_native/core/src/secure_memory/encrypted_memory_store.rs create mode 100644 apps/desktop/desktop_native/core/src/secure_memory/secure_key/crypto.rs create mode 100644 apps/desktop/desktop_native/core/src/secure_memory/secure_key/dpapi.rs create mode 100644 apps/desktop/desktop_native/core/src/secure_memory/secure_key/keyctl.rs create mode 100644 apps/desktop/desktop_native/core/src/secure_memory/secure_key/memfd_secret.rs create mode 100644 apps/desktop/desktop_native/core/src/secure_memory/secure_key/mlock.rs create mode 100644 apps/desktop/desktop_native/core/src/secure_memory/secure_key/mod.rs diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index ae5d62a90c2..f03cf3ee2a8 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -8,6 +8,7 @@ apps/desktop/desktop_native @bitwarden/team-platform-dev apps/desktop/desktop_native/objc/src/native/autofill @bitwarden/team-autofill-desktop-dev apps/desktop/desktop_native/core/src/autofill @bitwarden/team-autofill-desktop-dev +apps/desktop/desktop_native/core/src/secure_memory @bitwarden/team-key-management-dev ## No ownership for Cargo.lock and Cargo.toml to allow dependency updates apps/desktop/desktop_native/Cargo.lock apps/desktop/desktop_native/Cargo.toml diff --git a/.github/renovate.json5 b/.github/renovate.json5 index 6fcffb1f875..f898df460c9 100644 --- a/.github/renovate.json5 +++ b/.github/renovate.json5 @@ -400,7 +400,16 @@ reviewers: ["team:team-vault-dev"], }, { - matchPackageNames: ["aes", "big-integer", "cbc", "rsa", "russh-cryptovec", "sha2"], + matchPackageNames: [ + "aes", + "big-integer", + "cbc", + "rsa", + "russh-cryptovec", + "sha2", + "memsec", + "linux-keyutils", + ], description: "Key Management owned dependencies", commitMessagePrefix: "[deps] KM:", reviewers: ["team:team-key-management-dev"], diff --git a/.gitignore b/.gitignore index 61a20195592..6b13d22caa7 100644 --- a/.gitignore +++ b/.gitignore @@ -28,6 +28,7 @@ npm-debug.log # Build directories dist build +target .angular/cache .flatpak .flatpak-repo diff --git a/apps/desktop/desktop_native/Cargo.lock b/apps/desktop/desktop_native/Cargo.lock index 0aa040bbcf1..3df6b41734b 100644 --- a/apps/desktop/desktop_native/Cargo.lock +++ b/apps/desktop/desktop_native/Cargo.lock @@ -927,6 +927,8 @@ dependencies = [ "interprocess", "keytar", "libc", + "linux-keyutils", + "memsec", "oo7", "pin-project", "pkcs8", @@ -1793,6 +1795,16 @@ dependencies = [ "cc", ] +[[package]] +name = "linux-keyutils" +version = "0.2.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "761e49ec5fd8a5a463f9b84e877c373d888935b71c6be78f3767fe2ae6bed18e" +dependencies = [ + "bitflags", + "libc", +] + [[package]] name = "linux-raw-sys" version = "0.4.15" @@ -1878,6 +1890,17 @@ dependencies = [ "autocfg", ] +[[package]] +name = "memsec" +version = "0.7.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c797b9d6bb23aab2fc369c65f871be49214f5c759af65bde26ffaaa2b646b492" +dependencies = [ + "getrandom 0.2.16", + "libc", + "windows-sys 0.45.0", +] + [[package]] name = "mime" version = "0.3.17" @@ -3993,6 +4016,15 @@ dependencies = [ "windows-link", ] +[[package]] +name = "windows-sys" +version = "0.45.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "75283be5efb2831d37ea142365f009c02ec203cd29a3ebecbc093d52315b66d0" +dependencies = [ + "windows-targets 0.42.2", +] + [[package]] name = "windows-sys" version = "0.52.0" @@ -4020,6 +4052,21 @@ dependencies = [ "windows-targets 0.53.3", ] +[[package]] +name = "windows-targets" +version = "0.42.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8e5180c00cd44c9b1c88adb3693291f1cd93605ded80c250a75d472756b4d071" +dependencies = [ + "windows_aarch64_gnullvm 0.42.2", + "windows_aarch64_msvc 0.42.2", + "windows_i686_gnu 0.42.2", + "windows_i686_msvc 0.42.2", + "windows_x86_64_gnu 0.42.2", + "windows_x86_64_gnullvm 0.42.2", + "windows_x86_64_msvc 0.42.2", +] + [[package]] name = "windows-targets" version = "0.48.5" @@ -4068,6 +4115,12 @@ dependencies = [ "windows_x86_64_msvc 0.53.0", ] +[[package]] +name = "windows_aarch64_gnullvm" +version = "0.42.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "597a5118570b68bc08d8d59125332c54f1ba9d9adeedeef5b99b02ba2b0698f8" + [[package]] name = "windows_aarch64_gnullvm" version = "0.48.5" @@ -4086,6 +4139,12 @@ version = "0.53.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "86b8d5f90ddd19cb4a147a5fa63ca848db3df085e25fee3cc10b39b6eebae764" +[[package]] +name = "windows_aarch64_msvc" +version = "0.42.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e08e8864a60f06ef0d0ff4ba04124db8b0fb3be5776a5cd47641e942e58c4d43" + [[package]] name = "windows_aarch64_msvc" version = "0.48.5" @@ -4104,6 +4163,12 @@ version = "0.53.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "c7651a1f62a11b8cbd5e0d42526e55f2c99886c77e007179efff86c2b137e66c" +[[package]] +name = "windows_i686_gnu" +version = "0.42.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c61d927d8da41da96a81f029489353e68739737d3beca43145c8afec9a31a84f" + [[package]] name = "windows_i686_gnu" version = "0.48.5" @@ -4134,6 +4199,12 @@ version = "0.53.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "9ce6ccbdedbf6d6354471319e781c0dfef054c81fbc7cf83f338a4296c0cae11" +[[package]] +name = "windows_i686_msvc" +version = "0.42.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "44d840b6ec649f480a41c8d80f9c65108b92d89345dd94027bfe06ac444d1060" + [[package]] name = "windows_i686_msvc" version = "0.48.5" @@ -4161,6 +4232,12 @@ dependencies = [ "windows-core 0.61.0", ] +[[package]] +name = "windows_x86_64_gnu" +version = "0.42.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8de912b8b8feb55c064867cf047dda097f92d51efad5b491dfb98f6bbb70cb36" + [[package]] name = "windows_x86_64_gnu" version = "0.48.5" @@ -4179,6 +4256,12 @@ version = "0.53.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "2e55b5ac9ea33f2fc1716d1742db15574fd6fc8dadc51caab1c16a3d3b4190ba" +[[package]] +name = "windows_x86_64_gnullvm" +version = "0.42.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "26d41b46a36d453748aedef1486d5c7a85db22e56aff34643984ea85514e94a3" + [[package]] name = "windows_x86_64_gnullvm" version = "0.48.5" @@ -4197,6 +4280,12 @@ version = "0.53.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "0a6e035dd0599267ce1ee132e51c27dd29437f63325753051e71dd9e42406c57" +[[package]] +name = "windows_x86_64_msvc" +version = "0.42.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9aec5da331524158c6d1a4ac0ab1541149c0b9505fde06423b02f5ef0106b9f0" + [[package]] name = "windows_x86_64_msvc" version = "0.48.5" @@ -4416,9 +4505,9 @@ dependencies = [ [[package]] name = "zeroize" -version = "1.8.1" +version = "1.8.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ced3678a2879b30306d323f4542626697a464a97c0a07c9aebf7ebca65cd4dde" +checksum = "b97154e67e32c85465826e8bcc1c59429aaaf107c1e4a9e53c8d8ccd5eff88d0" dependencies = [ "zeroize_derive", ] diff --git a/apps/desktop/desktop_native/Cargo.toml b/apps/desktop/desktop_native/Cargo.toml index 855b0b3aa43..edf3cb44eca 100644 --- a/apps/desktop/desktop_native/Cargo.toml +++ b/apps/desktop/desktop_native/Cargo.toml @@ -39,7 +39,9 @@ homedir = "=0.3.4" interprocess = "=2.2.1" keytar = "=0.1.6" libc = "=0.2.172" +linux-keyutils = "=0.2.4" log = "=0.4.25" +memsec = "=0.7.0" napi = "=2.16.17" napi-build = "=2.2.0" napi-derive = "=2.16.13" diff --git a/apps/desktop/desktop_native/core/Cargo.toml b/apps/desktop/desktop_native/core/Cargo.toml index b7e4c9b7a83..f6c9d669df6 100644 --- a/apps/desktop/desktop_native/core/Cargo.toml +++ b/apps/desktop/desktop_native/core/Cargo.toml @@ -32,6 +32,7 @@ ed25519 = { workspace = true, features = ["pkcs8"] } futures = { workspace = true } homedir = { workspace = true } interprocess = { workspace = true, features = ["tokio"] } +memsec = { workspace = true, features = ["alloc_ext"] } pin-project = { workspace = true } pkcs8 = { workspace = true, features = ["alloc", "encryption", "pem"] } rand = { workspace = true } @@ -87,6 +88,7 @@ desktop_objc = { path = "../objc" } [target.'cfg(target_os = "linux")'.dependencies] oo7 = { workspace = true } libc = { workspace = true } +linux-keyutils = { workspace = true } ashpd = { workspace = true } zbus = { workspace = true, optional = true } 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 ca9b6081d69..3ff8a6d3d83 100644 --- a/apps/desktop/desktop_native/core/src/secure_memory/dpapi.rs +++ b/apps/desktop/desktop_native/core/src/secure_memory/dpapi.rs @@ -54,7 +54,7 @@ impl SecureMemoryStore for DpapiSecretKVStore { self.map.insert(key, padded_data); } - fn get(&self, key: &str) -> Option> { + fn get(&mut self, key: &str) -> Option> { self.map.get(key).map(|data| { // A copy is created, that is then mutated by the DPAPI unprotect function. let mut data = data.clone(); 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 new file mode 100644 index 00000000000..a8952d8f55a --- /dev/null +++ b/apps/desktop/desktop_native/core/src/secure_memory/encrypted_memory_store.rs @@ -0,0 +1,105 @@ +use tracing::error; + +use crate::secure_memory::{ + secure_key::{EncryptedMemory, SecureMemoryEncryptionKey}, + SecureMemoryStore, +}; + +/// An encrypted memory store holds a platform protected symmetric encryption key, and uses it +/// to encrypt all items it stores. The ciphertexts for the items are not specially protected. This +/// allows circumventing length and amount limitations on platform specific secure memory APIs since +/// only a single short item needs to be protected. +/// +/// 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, + memory_encryption_key: SecureMemoryEncryptionKey, +} + +impl EncryptedMemoryStore { + #[allow(unused)] + pub(crate) fn new() -> Self { + EncryptedMemoryStore { + map: std::collections::HashMap::new(), + memory_encryption_key: SecureMemoryEncryptionKey::new(), + } + } +} + +impl SecureMemoryStore for EncryptedMemoryStore { + fn put(&mut self, key: String, 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 + } + } + } else { + None + } + } + + fn has(&self, key: &str) -> bool { + self.map.contains_key(key) + } + + fn remove(&mut self, key: &str) { + self.map.remove(key); + } + + fn clear(&mut self) { + self.map.clear(); + } +} + +impl Drop for EncryptedMemoryStore { + fn drop(&mut self) { + self.clear(); + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_secret_kv_store_various_sizes() { + let mut store = EncryptedMemoryStore::new(); + 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); + assert!(store.has(&key), "Store should have key for size {}", size); + assert_eq!( + store.get(&key), + Some(value), + "Value mismatch for size {}", + size + ); + } + } + + #[test] + fn test_crud() { + let mut store = EncryptedMemoryStore::new(); + 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)); + store.remove(&key); + assert!(!store.has(&key)); + } +} 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 0cb604e03f2..8695904758e 100644 --- a/apps/desktop/desktop_native/core/src/secure_memory/mod.rs +++ b/apps/desktop/desktop_native/core/src/secure_memory/mod.rs @@ -1,6 +1,9 @@ #[cfg(target_os = "windows")] pub(crate) mod dpapi; +mod encrypted_memory_store; +mod secure_key; + /// 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 @@ -12,7 +15,9 @@ pub(crate) trait SecureMemoryStore { /// 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. - fn get(&self, key: &str) -> Option>; + /// + /// Note: If memory was tampered with, this will re-key the store and return None. + fn get(&mut self, key: &str) -> Option>; /// Checks if a value is stored under the given key. fn has(&self, key: &str) -> bool; /// Removes the value associated with the given key from secure memory. 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 new file mode 100644 index 00000000000..1ee6c4cdf40 --- /dev/null +++ b/apps/desktop/desktop_native/core/src/secure_memory/secure_key/crypto.rs @@ -0,0 +1,96 @@ +use std::ptr::NonNull; + +use chacha20poly1305::{aead::Aead, Key, KeyInit}; +use rand::{rng, Rng}; + +pub(super) const KEY_SIZE: usize = 32; +pub(super) const NONCE_SIZE: usize = 24; + +/// The encryption performed here is xchacha-poly1305. Any tampering with the key or the ciphertexts will result +/// in a decryption failure and panic. The key's memory contents are protected from being swapped to disk +/// via mlock. +pub(super) struct MemoryEncryptionKey(NonNull<[u8]>); + +/// An encrypted memory blob that must be decrypted using the same key that it was encrypted with. +pub struct EncryptedMemory { + nonce: [u8; NONCE_SIZE], + ciphertext: Vec, +} + +impl MemoryEncryptionKey { + pub fn new() -> Self { + let mut key = [0u8; KEY_SIZE]; + rng().fill(&mut key); + MemoryEncryptionKey::from(&key) + } + + /// Encrypts the given plaintext using the key. + #[allow(unused)] + pub(super) fn encrypt(&self, plaintext: &[u8]) -> EncryptedMemory { + let cipher = chacha20poly1305::XChaCha20Poly1305::new(Key::from_slice(self.as_ref())); + let mut nonce = [0u8; NONCE_SIZE]; + rng().fill(&mut nonce); + let ciphertext = cipher + .encrypt(chacha20poly1305::XNonce::from_slice(&nonce), plaintext) + .expect("encryption should not fail"); + EncryptedMemory { nonce, ciphertext } + } + + /// Decrypts the given encrypted memory using the key. A decryption failure will panic. This is + /// okay because neither the keys nor ciphertexts should ever fail to decrypt, and doing so + /// indicates that the process memory was tampered with. + #[allow(unused)] + pub(super) fn decrypt(&self, encrypted: &EncryptedMemory) -> Result, DecryptionError> { + let cipher = chacha20poly1305::XChaCha20Poly1305::new(Key::from_slice(self.as_ref())); + cipher + .decrypt( + chacha20poly1305::XNonce::from_slice(&encrypted.nonce), + encrypted.ciphertext.as_ref(), + ) + .map_err(|_| DecryptionError::CouldNotDecrypt) + } +} + +impl Drop for MemoryEncryptionKey { + fn drop(&mut self) { + unsafe { + memsec::free(self.0); + } + } +} + +impl From<&[u8; KEY_SIZE]> for MemoryEncryptionKey { + fn from(value: &[u8; KEY_SIZE]) -> Self { + let mut ptr: NonNull<[u8]> = + unsafe { memsec::malloc_sized(KEY_SIZE).expect("malloc_sized should work") }; + unsafe { + std::ptr::copy_nonoverlapping(value.as_ptr(), ptr.as_mut().as_mut_ptr(), KEY_SIZE); + } + MemoryEncryptionKey(ptr) + } +} + +impl AsRef<[u8]> for MemoryEncryptionKey { + fn as_ref(&self) -> &[u8] { + unsafe { self.0.as_ref() } + } +} + +#[derive(Debug)] +pub(crate) enum DecryptionError { + CouldNotDecrypt, +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_memory_encryption_key() { + let key = MemoryEncryptionKey::new(); + let data = b"Hello, world!"; + let encrypted = key.encrypt(data); + let decrypted = key.decrypt(&encrypted).unwrap(); + assert_eq!(data.as_ref(), decrypted.as_slice()); + } +} diff --git a/apps/desktop/desktop_native/core/src/secure_memory/secure_key/dpapi.rs b/apps/desktop/desktop_native/core/src/secure_memory/secure_key/dpapi.rs new file mode 100644 index 00000000000..0975b542877 --- /dev/null +++ b/apps/desktop/desktop_native/core/src/secure_memory/secure_key/dpapi.rs @@ -0,0 +1,93 @@ +use super::crypto::{MemoryEncryptionKey, KEY_SIZE}; +use super::SecureKeyContainer; +use windows::Win32::Security::Cryptography::{ + CryptProtectMemory, CryptUnprotectMemory, CRYPTPROTECTMEMORY_BLOCK_SIZE, + CRYPTPROTECTMEMORY_SAME_PROCESS, +}; + +/// 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 +/// to the current process, and cannot be decrypted by other user-mode processes. +/// +/// Note: Admin processes can still decrypt this memory: +/// https://blog.slowerzs.net/posts/cryptdecryptmemory/ +pub(super) struct DpapiSecureKeyContainer { + dpapi_encrypted_key: [u8; KEY_SIZE + CRYPTPROTECTMEMORY_BLOCK_SIZE as usize], +} + +// SAFETY: The encrypted data is fully owned by this struct, and not exposed outside or cloned, +// and is disposed on drop of this struct. +unsafe impl Send for DpapiSecureKeyContainer {} +// SAFETY: The container is non-mutable and thus safe to share between threads. +unsafe impl Sync for DpapiSecureKeyContainer {} + +impl SecureKeyContainer for DpapiSecureKeyContainer { + fn as_key(&self) -> MemoryEncryptionKey { + let mut decrypted_key = self.dpapi_encrypted_key; + unsafe { + CryptUnprotectMemory( + decrypted_key.as_mut_ptr() as *mut core::ffi::c_void, + decrypted_key.len() as u32, + CRYPTPROTECTMEMORY_SAME_PROCESS, + ) + } + .expect("crypt_unprotect_memory should work"); + let mut key = [0u8; KEY_SIZE]; + key.copy_from_slice(&decrypted_key[..KEY_SIZE]); + MemoryEncryptionKey::from(&key) + } + + fn from_key(key: MemoryEncryptionKey) -> Self { + let mut padded_key = [0u8; KEY_SIZE + CRYPTPROTECTMEMORY_BLOCK_SIZE as usize]; + padded_key[..KEY_SIZE].copy_from_slice(key.as_ref()); + unsafe { + CryptProtectMemory( + padded_key.as_mut_ptr() as *mut core::ffi::c_void, + padded_key.len() as u32, + CRYPTPROTECTMEMORY_SAME_PROCESS, + ) + } + .expect("crypt_protect_memory should work"); + DpapiSecureKeyContainer { + dpapi_encrypted_key: padded_key, + } + } + + fn is_supported() -> bool { + // DPAPI is supported on all Windows versions that we support. + true + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_multiple_keys() { + let key1 = MemoryEncryptionKey::new(); + let key2 = MemoryEncryptionKey::new(); + let container1 = DpapiSecureKeyContainer::from_key(key1); + let container2 = DpapiSecureKeyContainer::from_key(key2); + + // Capture at time 1 + let data_1_1 = container1.as_key(); + let data_2_1 = container2.as_key(); + // Capture at time 2 + let data_1_2 = container1.as_key(); + let data_2_2 = container2.as_key(); + + // Same keys should be equal + assert_eq!(data_1_1.as_ref(), data_1_2.as_ref()); + assert_eq!(data_2_1.as_ref(), data_2_2.as_ref()); + + // Different keys should be different + assert_ne!(data_1_1.as_ref(), data_2_1.as_ref()); + assert_ne!(data_1_2.as_ref(), data_2_2.as_ref()); + } + + #[test] + fn test_is_supported() { + assert!(DpapiSecureKeyContainer::is_supported()); + } +} diff --git a/apps/desktop/desktop_native/core/src/secure_memory/secure_key/keyctl.rs b/apps/desktop/desktop_native/core/src/secure_memory/secure_key/keyctl.rs new file mode 100644 index 00000000000..a738d964671 --- /dev/null +++ b/apps/desktop/desktop_native/core/src/secure_memory/secure_key/keyctl.rs @@ -0,0 +1,100 @@ +use crate::secure_memory::secure_key::crypto::MemoryEncryptionKey; + +use super::crypto::KEY_SIZE; +use super::SecureKeyContainer; +use linux_keyutils::{KeyRing, KeyRingIdentifier}; + +/// The keys are bound to the process keyring. +const KEY_RING_IDENTIFIER: KeyRingIdentifier = KeyRingIdentifier::Process; +/// This is an atomic global counter used to help generate unique key IDs +static COUNTER: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(0); +/// Generates a unique ID for the key in the kernel keyring. +/// SAFETY: This function is safe to call from multiple threads because it uses an atomic counter. +fn make_id() -> String { + let counter = COUNTER.fetch_add(1, std::sync::atomic::Ordering::Relaxed); + // In case multiple processes are running, include the PID in the key ID. + let pid = std::process::id(); + format!("bitwarden_desktop_{}_{}", pid, counter) +} + +/// A secure key container that uses the Linux kernel keyctl API to store the key. +/// `https://man7.org/linux/man-pages/man1/keyctl.1.html`. The kernel enforces only +/// the correct process can read them, and they do not live in process memory space +/// and cannot be dumped. +pub(super) struct KeyctlSecureKeyContainer { + /// The kernel has an identifier for the key. This is randomly generated on construction. + id: String, +} + +// SAFETY: The key id is fully owned by this struct and not exposed or cloned, and cleaned up on drop. +// Further, since we use `KeyRingIdentifier::Process` and not `KeyRingIdentifier::Thread`, the key +// is accessible across threads within the same process bound. +unsafe impl Send for KeyctlSecureKeyContainer {} +// SAFETY: The container is non-mutable and thus safe to share between threads. +unsafe impl Sync for KeyctlSecureKeyContainer {} + +impl SecureKeyContainer for KeyctlSecureKeyContainer { + fn as_key(&self) -> MemoryEncryptionKey { + let ring = KeyRing::from_special_id(KEY_RING_IDENTIFIER, false) + .expect("should get process keyring"); + let key = ring.search(&self.id).expect("should find key"); + let mut buffer = [0u8; KEY_SIZE]; + key.read(&mut buffer).expect("should read key"); + MemoryEncryptionKey::from(&buffer) + } + + fn from_key(data: MemoryEncryptionKey) -> Self { + let ring = KeyRing::from_special_id(KEY_RING_IDENTIFIER, true) + .expect("should get process keyring"); + let id = make_id(); + ring.add_key(&id, &data).expect("should add key"); + KeyctlSecureKeyContainer { id } + } + + fn is_supported() -> bool { + KeyRing::from_special_id(KEY_RING_IDENTIFIER, true).is_ok() + } +} + +impl Drop for KeyctlSecureKeyContainer { + fn drop(&mut self) { + let ring = KeyRing::from_special_id(KEY_RING_IDENTIFIER, false) + .expect("should get process keyring"); + if let Ok(key) = ring.search(&self.id) { + let _ = key.invalidate(); + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_multiple_keys() { + let key1 = MemoryEncryptionKey::new(); + let key2 = MemoryEncryptionKey::new(); + let container1 = KeyctlSecureKeyContainer::from_key(key1); + let container2 = KeyctlSecureKeyContainer::from_key(key2); + + // Capture at time 1 + let data_1_1 = container1.as_key(); + let data_2_1 = container2.as_key(); + // Capture at time 2 + let data_1_2 = container1.as_key(); + let data_2_2 = container2.as_key(); + + // Same keys should be equal + assert_eq!(data_1_1.as_ref(), data_1_2.as_ref()); + assert_eq!(data_2_1.as_ref(), data_2_2.as_ref()); + + // Different keys should be different + assert_ne!(data_1_1.as_ref(), data_2_1.as_ref()); + assert_ne!(data_1_2.as_ref(), data_2_2.as_ref()); + } + + #[test] + fn test_is_supported() { + assert!(KeyctlSecureKeyContainer::is_supported()); + } +} diff --git a/apps/desktop/desktop_native/core/src/secure_memory/secure_key/memfd_secret.rs b/apps/desktop/desktop_native/core/src/secure_memory/secure_key/memfd_secret.rs new file mode 100644 index 00000000000..4e6a2c4d7ac --- /dev/null +++ b/apps/desktop/desktop_native/core/src/secure_memory/secure_key/memfd_secret.rs @@ -0,0 +1,109 @@ +use std::{ptr::NonNull, sync::LazyLock}; + +use super::crypto::MemoryEncryptionKey; +use super::crypto::KEY_SIZE; +use super::SecureKeyContainer; + +/// https://man.archlinux.org/man/memfd_secret.2.en +/// The memfd_secret store protects the data using the `memfd_secret` syscall. The +/// data is inaccessible to other user-mode processes, and even to root in most cases. +/// If arbitrary data can be executed in the kernel, the data can still be retrieved: +/// https://github.com/JonathonReinhart/nosecmem +pub(super) struct MemfdSecretSecureKeyContainer { + ptr: NonNull<[u8]>, +} +// SAFETY: The pointers in this struct are allocated by `memfd_secret`, and we have full ownership. +// They are never exposed outside or cloned, and are cleaned up by drop. +unsafe impl Send for MemfdSecretSecureKeyContainer {} +// SAFETY: The container is non-mutable and thus safe to share between threads. Further, memfd-secret +// is accessible across threads within the same process bound. +unsafe impl Sync for MemfdSecretSecureKeyContainer {} + +impl SecureKeyContainer for MemfdSecretSecureKeyContainer { + fn as_key(&self) -> MemoryEncryptionKey { + MemoryEncryptionKey::from( + &unsafe { self.ptr.as_ref() } + .try_into() + .expect("slice should be KEY_SIZE"), + ) + } + + fn from_key(key: MemoryEncryptionKey) -> Self { + let mut ptr: NonNull<[u8]> = unsafe { + memsec::memfd_secret_sized(KEY_SIZE).expect("memfd_secret_sized should work") + }; + unsafe { + std::ptr::copy_nonoverlapping( + key.as_ref().as_ptr(), + ptr.as_mut().as_mut_ptr(), + KEY_SIZE, + ); + } + MemfdSecretSecureKeyContainer { ptr } + } + + /// Note, `memfd_secret` is only available since Linux 6.5, so fallbacks are needed. + fn is_supported() -> bool { + // 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): 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) }; + result + }); + *IS_SUPPORTED + } +} + +impl Drop for MemfdSecretSecureKeyContainer { + fn drop(&mut self) { + unsafe { + memsec::free_memfd_secret(self.ptr); + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_multiple_keys() { + let key1 = MemoryEncryptionKey::new(); + let key2 = MemoryEncryptionKey::new(); + let container1 = MemfdSecretSecureKeyContainer::from_key(key1); + let container2 = MemfdSecretSecureKeyContainer::from_key(key2); + + // Capture at time 1 + let data_1_1 = container1.as_key(); + let data_2_1 = container2.as_key(); + // Capture at time 2 + let data_1_2 = container1.as_key(); + let data_2_2 = container2.as_key(); + + // Same keys should be equal + assert_eq!(data_1_1.as_ref(), data_1_2.as_ref()); + assert_eq!(data_2_1.as_ref(), data_2_2.as_ref()); + + // Different keys should be different + assert_ne!(data_1_1.as_ref(), data_2_1.as_ref()); + assert_ne!(data_1_2.as_ref(), data_2_2.as_ref()); + } + + #[test] + fn test_is_supported() { + assert!(MemfdSecretSecureKeyContainer::is_supported()); + } +} diff --git a/apps/desktop/desktop_native/core/src/secure_memory/secure_key/mlock.rs b/apps/desktop/desktop_native/core/src/secure_memory/secure_key/mlock.rs new file mode 100644 index 00000000000..db21cd7fedc --- /dev/null +++ b/apps/desktop/desktop_native/core/src/secure_memory/secure_key/mlock.rs @@ -0,0 +1,83 @@ +use std::ptr::NonNull; + +use super::crypto::MemoryEncryptionKey; +use super::crypto::KEY_SIZE; +use super::SecureKeyContainer; + +/// A SecureKeyContainer that uses mlock to prevent the memory from being swapped to disk. +/// This does not provide as strong protections as other methods, but is always supported. +pub(super) struct MlockSecureKeyContainer { + ptr: NonNull<[u8]>, +} +// SAFETY: The pointers in this struct are allocated by `malloc_sized`, and we have full ownership. +// They are never exposed outside or cloned, and are cleaned up by drop. +unsafe impl Send for MlockSecureKeyContainer {} +// SAFETY: The container is non-mutable and thus safe to share between threads. +unsafe impl Sync for MlockSecureKeyContainer {} + +impl SecureKeyContainer for MlockSecureKeyContainer { + fn as_key(&self) -> MemoryEncryptionKey { + MemoryEncryptionKey::from( + &unsafe { self.ptr.as_ref() } + .try_into() + .expect("slice should be KEY_SIZE"), + ) + } + fn from_key(key: MemoryEncryptionKey) -> Self { + let mut ptr: NonNull<[u8]> = + unsafe { memsec::malloc_sized(KEY_SIZE).expect("malloc_sized should work") }; + unsafe { + std::ptr::copy_nonoverlapping( + key.as_ref().as_ptr(), + ptr.as_mut().as_mut_ptr(), + KEY_SIZE, + ); + } + MlockSecureKeyContainer { ptr } + } + + fn is_supported() -> bool { + true + } +} + +impl Drop for MlockSecureKeyContainer { + fn drop(&mut self) { + unsafe { + memsec::free(self.ptr); + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_multiple_keys() { + let key1 = MemoryEncryptionKey::new(); + let key2 = MemoryEncryptionKey::new(); + let container1 = MlockSecureKeyContainer::from_key(key1); + let container2 = MlockSecureKeyContainer::from_key(key2); + + // Capture at time 1 + let data_1_1 = container1.as_key(); + let data_2_1 = container2.as_key(); + // Capture at time 2 + let data_1_2 = container1.as_key(); + let data_2_2 = container2.as_key(); + + // Same keys should be equal + assert_eq!(data_1_1.as_ref(), data_1_2.as_ref()); + assert_eq!(data_2_1.as_ref(), data_2_2.as_ref()); + + // Different keys should be different + assert_ne!(data_1_1.as_ref(), data_2_1.as_ref()); + assert_ne!(data_1_2.as_ref(), data_2_2.as_ref()); + } + + #[test] + fn test_is_supported() { + assert!(MlockSecureKeyContainer::is_supported()); + } +} 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 new file mode 100644 index 00000000000..6c3b53117a5 --- /dev/null +++ b/apps/desktop/desktop_native/core/src/secure_memory/secure_key/mod.rs @@ -0,0 +1,242 @@ +//! This module provides hardened storage for single cryptographic keys. These are meant for encrypting large amounts of memory. +//! Some platforms restrict how many keys can be protected by their APIs, which necessitates this layer of indirection. This significantly +//! reduces the complexity of each platform specific implementation, since all that's needed is implementing protecting a single fixed sized key +//! instead of protecting many arbitrarily sized secrets. This significantly lowers the effort to maintain each implementation. +//! +//! The implementations include DPAPI on Windows, `keyctl` on Linux, and `memfd_secret` on Linux, and a fallback implementation using mlock. + +use tracing::info; + +mod crypto; +#[cfg(target_os = "windows")] +mod dpapi; +#[cfg(target_os = "linux")] +mod keyctl; +#[cfg(target_os = "linux")] +mod memfd_secret; +mod mlock; + +pub use crypto::EncryptedMemory; + +use crate::secure_memory::secure_key::crypto::DecryptionError; + +/// 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, persistent data cannot be encrypted with this key. +/// On Linux and Windows, in most cases the protection mechanisms prevent memory dumps/debuggers from reading the key. +/// +/// Note: This can be circumvented if code can be injected into the process and is only effective in combination with the +/// memory isolation provided in `process_isolation`. +/// - https://github.com/zer1t0/keydump +#[allow(unused)] +pub(crate) struct SecureMemoryEncryptionKey(CrossPlatformSecureKeyContainer); + +impl SecureMemoryEncryptionKey { + pub fn new() -> Self { + SecureMemoryEncryptionKey(CrossPlatformSecureKeyContainer::from_key( + crypto::MemoryEncryptionKey::new(), + )) + } + + /// Encrypts the provided plaintext using the contained key, returning an EncryptedMemory blob. + #[allow(unused)] + pub fn encrypt(&self, plaintext: &[u8]) -> crypto::EncryptedMemory { + self.0.as_key().encrypt(plaintext) + } + + /// Decrypts the provided EncryptedMemory blob using the contained key, returning the plaintext. + /// If the decryption fails, that means the memory was tampered with, and the function panics. + #[allow(unused)] + pub fn decrypt(&self, encrypted: &crypto::EncryptedMemory) -> Result, DecryptionError> { + self.0.as_key().decrypt(encrypted) + } +} + +/// A platform specific implementation of a key container that protects a single encryption key +/// from memory attacks. +#[allow(unused)] +trait SecureKeyContainer: Sync + Send { + /// Returns the key as a byte slice. This slice does not have additional memory protections applied. + fn as_key(&self) -> crypto::MemoryEncryptionKey; + /// Creates a new SecureKeyContainer from the provided key. + fn from_key(key: crypto::MemoryEncryptionKey) -> Self; + /// Returns true if this platform supports this secure key container implementation. + fn is_supported() -> bool; +} + +#[allow(unused)] +enum CrossPlatformSecureKeyContainer { + #[cfg(target_os = "windows")] + Dpapi(dpapi::DpapiSecureKeyContainer), + #[cfg(target_os = "linux")] + Keyctl(keyctl::KeyctlSecureKeyContainer), + #[cfg(target_os = "linux")] + MemfdSecret(memfd_secret::MemfdSecretSecureKeyContainer), + Mlock(mlock::MlockSecureKeyContainer), +} + +impl SecureKeyContainer for CrossPlatformSecureKeyContainer { + fn as_key(&self) -> crypto::MemoryEncryptionKey { + match self { + #[cfg(target_os = "windows")] + CrossPlatformSecureKeyContainer::Dpapi(c) => c.as_key(), + #[cfg(target_os = "linux")] + CrossPlatformSecureKeyContainer::Keyctl(c) => c.as_key(), + #[cfg(target_os = "linux")] + CrossPlatformSecureKeyContainer::MemfdSecret(c) => c.as_key(), + CrossPlatformSecureKeyContainer::Mlock(c) => c.as_key(), + } + } + + fn from_key(key: crypto::MemoryEncryptionKey) -> Self { + if let Some(container) = get_env_forced_container() { + return container; + } + + #[cfg(target_os = "windows")] + { + if dpapi::DpapiSecureKeyContainer::is_supported() { + info!("Using DPAPI for secure key storage"); + return CrossPlatformSecureKeyContainer::Dpapi( + dpapi::DpapiSecureKeyContainer::from_key(key), + ); + } + } + #[cfg(target_os = "linux")] + { + // Memfd_secret is slightly better in some cases of the kernel being compromised. + // Note that keyctl may sometimes not be available in e.g. snap. Memfd_secret is + // not available on kernels older than 6.5 while keyctl is supported since 2.6. + // + // Note: This may prevent the system from hibernating but not sleeping. Hibernate + // would write the memory to disk, exposing the keys. If this is an issue, + // the environment variable `SECURE_KEY_CONTAINER_BACKEND` can be used + // to force the use of keyctl or mlock. + if memfd_secret::MemfdSecretSecureKeyContainer::is_supported() { + info!("Using memfd_secret for secure key storage"); + return CrossPlatformSecureKeyContainer::MemfdSecret( + memfd_secret::MemfdSecretSecureKeyContainer::from_key(key), + ); + } + if keyctl::KeyctlSecureKeyContainer::is_supported() { + info!("Using keyctl for secure key storage"); + return CrossPlatformSecureKeyContainer::Keyctl( + keyctl::KeyctlSecureKeyContainer::from_key(key), + ); + } + } + + // Falling back to mlock means that the key is accessible via memory dumping. + info!("Falling back to mlock for secure key storage"); + CrossPlatformSecureKeyContainer::Mlock(mlock::MlockSecureKeyContainer::from_key(key)) + } + + fn is_supported() -> bool { + // Mlock is always supported as a fallback. + true + } +} + +fn get_env_forced_container() -> Option { + let env_var = std::env::var("SECURE_KEY_CONTAINER_BACKEND"); + match env_var.as_deref() { + #[cfg(target_os = "windows")] + Ok("dpapi") => { + info!("Forcing DPAPI secure key container via environment variable"); + Some(CrossPlatformSecureKeyContainer::Dpapi( + dpapi::DpapiSecureKeyContainer::from_key(crypto::MemoryEncryptionKey::new()), + )) + } + #[cfg(target_os = "linux")] + Ok("memfd_secret") => { + info!("Forcing memfd_secret secure key container via environment variable"); + Some(CrossPlatformSecureKeyContainer::MemfdSecret( + memfd_secret::MemfdSecretSecureKeyContainer::from_key( + crypto::MemoryEncryptionKey::new(), + ), + )) + } + #[cfg(target_os = "linux")] + Ok("keyctl") => { + info!("Forcing keyctl secure key container via environment variable"); + Some(CrossPlatformSecureKeyContainer::Keyctl( + keyctl::KeyctlSecureKeyContainer::from_key(crypto::MemoryEncryptionKey::new()), + )) + } + Ok("mlock") => { + info!("Forcing mlock secure key container via environment variable"); + Some(CrossPlatformSecureKeyContainer::Mlock( + mlock::MlockSecureKeyContainer::from_key(crypto::MemoryEncryptionKey::new()), + )) + } + _ => { + info!( + "{} is not a valid secure key container backend, using automatic selection", + env_var.unwrap_or_default() + ); + None + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_multiple_keys() { + // Create 20 different keys + let original_keys: Vec = (0..20) + .map(|_| crypto::MemoryEncryptionKey::new()) + .collect(); + + // Store them in secure containers + let containers: Vec = original_keys + .iter() + .map(|key| { + let key_bytes: &[u8; crypto::KEY_SIZE] = key.as_ref().try_into().unwrap(); + CrossPlatformSecureKeyContainer::from_key(crypto::MemoryEncryptionKey::from( + key_bytes, + )) + }) + .collect(); + + // Read all keys back and validate they match the originals + for (i, (original_key, container)) in + original_keys.iter().zip(containers.iter()).enumerate() + { + let retrieved_key = container.as_key(); + assert_eq!( + original_key.as_ref(), + retrieved_key.as_ref(), + "Key {} should match after storage and retrieval", + i + ); + } + + // Verify all keys are different from each other + for i in 0..original_keys.len() { + for j in (i + 1)..original_keys.len() { + assert_ne!( + original_keys[i].as_ref(), + original_keys[j].as_ref(), + "Keys {} and {} should be different", + i, + j + ); + } + } + + // Read keys back a second time to ensure consistency + for (i, (original_key, container)) in + original_keys.iter().zip(containers.iter()).enumerate() + { + let retrieved_key_again = container.as_key(); + assert_eq!( + original_key.as_ref(), + retrieved_key_again.as_ref(), + "Key {} should still match on second retrieval", + i + ); + } + } +}