From d7c7c3e1df07cf4efb8e56ebe600459b47205ec3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garci=CC=81a?= Date: Fri, 26 Jul 2024 20:02:59 +0200 Subject: [PATCH 01/34] Initial CryptoService impl --- Cargo.lock | 78 ++ crates/bitwarden-core/src/lib.rs | 116 +++ crates/bitwarden-crypto/Cargo.toml | 1 + crates/bitwarden-crypto/src/error.rs | 2 + crates/bitwarden-crypto/src/lib.rs | 1 + .../src/service/crypto_engine/mod.rs | 102 +++ .../src/service/crypto_engine/rust_impl.rs | 250 ++++++ .../src/service/encryptable.rs | 130 ++++ .../bitwarden-crypto/src/service/key_ref.rs | 86 +++ .../key_store/linux_memfd_secret_impl.rs | 122 +++ .../src/service/key_store/mod.rs | 27 + .../src/service/key_store/rust_impl.rs | 129 ++++ .../src/service/key_store/util.rs | 719 ++++++++++++++++++ crates/bitwarden-crypto/src/service/mod.rs | 82 ++ 14 files changed, 1845 insertions(+) create mode 100644 crates/bitwarden-crypto/src/service/crypto_engine/mod.rs create mode 100644 crates/bitwarden-crypto/src/service/crypto_engine/rust_impl.rs create mode 100644 crates/bitwarden-crypto/src/service/encryptable.rs create mode 100644 crates/bitwarden-crypto/src/service/key_ref.rs create mode 100644 crates/bitwarden-crypto/src/service/key_store/linux_memfd_secret_impl.rs create mode 100644 crates/bitwarden-crypto/src/service/key_store/mod.rs create mode 100644 crates/bitwarden-crypto/src/service/key_store/rust_impl.rs create mode 100644 crates/bitwarden-crypto/src/service/key_store/util.rs create mode 100644 crates/bitwarden-crypto/src/service/mod.rs diff --git a/Cargo.lock b/Cargo.lock index e9ed72fee..c201b243c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -470,6 +470,7 @@ dependencies = [ "generic-array", "hkdf", "hmac", + "memsec", "num-bigint", "num-traits", "pbkdf2", @@ -2282,6 +2283,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" @@ -4616,6 +4628,15 @@ dependencies = [ "windows-targets 0.52.6", ] +[[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.48.0" @@ -4643,6 +4664,21 @@ dependencies = [ "windows-targets 0.52.6", ] +[[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" @@ -4674,6 +4710,12 @@ dependencies = [ "windows_x86_64_msvc 0.52.6", ] +[[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" @@ -4686,6 +4728,12 @@ version = "0.52.6" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "32a4622180e7a0ec044bb555404c800bc9fd9ec262ec147edd5989ccd0c02cd3" +[[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" @@ -4698,6 +4746,12 @@ version = "0.52.6" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "09ec2a7bb152e2252b53fa7803150007879548bc709c039df7627cabbd05d469" +[[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" @@ -4716,6 +4770,12 @@ version = "0.52.6" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "0eee52d38c090b3caa76c563b86c3a4bd71ef1a819287c19d586d7334ae8ed66" +[[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" @@ -4728,6 +4788,12 @@ version = "0.52.6" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "240948bc05c5e7c6dabba28bf89d89ffce3e303022809e73deaefe4f6ec56c66" +[[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" @@ -4740,6 +4806,12 @@ version = "0.52.6" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "147a5c80aabfbf0c7d901cb5895d1de30ef2907eb21fbbab29ca94c5b08b1a78" +[[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" @@ -4752,6 +4824,12 @@ version = "0.52.6" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "24d5b23dc417412679681396f2b49f3de8c1473deb516bd34410872eff51ed0d" +[[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" diff --git a/crates/bitwarden-core/src/lib.rs b/crates/bitwarden-core/src/lib.rs index 12b0df3c3..5f900a468 100644 --- a/crates/bitwarden-core/src/lib.rs +++ b/crates/bitwarden-core/src/lib.rs @@ -19,3 +19,119 @@ mod util; pub use bitwarden_crypto::ZeroizingAllocator; pub use client::{Client, ClientSettings, DeviceType}; + +#[allow(warnings)] +#[cfg(test)] +mod testcrypto { + + // TEST IMPL OF THE NEW ENCRYPTABLE/DECRYPTABLE TRAITS + // Note that these never touch the keys at all, they just use the context and key references to + // encrypt/decrypt + + use bitwarden_crypto::{ + key_refs, service::*, AsymmetricCryptoKey, CryptoError, EncString, KeyEncryptable, + SymmetricCryptoKey, + }; + + key_refs! { + #[symmetric] + pub enum MySymmKeyRef { + User, + Organization(uuid::Uuid), + #[local] + Local(&'static str), + } + + #[asymmetric] + pub enum MyAsymmKeyRef { + UserPrivateKey, + #[local] + Local(&'static str), + } + } + + #[derive(Clone)] + struct Cipher { + key: Option, + name: EncString, + } + + #[derive(Clone)] + struct CipherView { + key: Option, + name: String, + } + + const CIPHER_KEY: MySymmKeyRef = MySymmKeyRef::Local("cipher_key"); + + impl Encryptable for CipherView { + fn encrypt( + &self, + ctx: &mut CryptoServiceContext, + key: MySymmKeyRef, + ) -> Result { + let cipher_key = match &self.key { + Some(cipher_key) => { + ctx.decrypt_and_store_symmetric_key(key, CIPHER_KEY, cipher_key)? + } + None => key, + }; + + Ok(Cipher { + key: self.key.clone(), + name: self.name.encrypt(ctx, cipher_key)?, + }) + } + } + + impl Decryptable for Cipher { + fn decrypt( + &self, + ctx: &mut CryptoServiceContext, + key: MySymmKeyRef, + ) -> Result { + let cipher_key = match &self.key { + Some(cipher_key) => { + ctx.decrypt_and_store_symmetric_key(key, CIPHER_KEY, cipher_key)? + } + None => key, + }; + + Ok(CipherView { + key: self.key.clone(), + name: self.name.decrypt(ctx, cipher_key)?, + }) + } + } + + #[test] + fn test_cipher() { + let user_key = SymmetricCryptoKey::generate(rand::thread_rng()); + + let org_id = uuid::Uuid::parse_str("91b000b6-81ce-47f4-9802-3390e0b895ed").unwrap(); + let org_key = SymmetricCryptoKey::generate(rand::thread_rng()); + + let cipher_key = SymmetricCryptoKey::generate(rand::thread_rng()); + let cipher_key_user_enc = cipher_key.to_vec().encrypt_with_key(&user_key).unwrap(); + let cipher_view = CipherView { + key: Some(cipher_key_user_enc.clone()), + name: "test".to_string(), + }; + + let service: CryptoService = CryptoService::new(); + // Ideally we'd decrypt the keys directly into the service, but that's not implemented yet + #[allow(deprecated)] + { + service.insert_symmetric_key(MySymmKeyRef::User, user_key); + service.insert_symmetric_key(MySymmKeyRef::Organization(org_id), org_key); + } + + let cipher_enc2 = service + .encrypt(MySymmKeyRef::User, cipher_view.clone()) + .unwrap(); + + let cipher_view2 = service.decrypt(MySymmKeyRef::User, &cipher_enc2).unwrap(); + + assert_eq!(cipher_view.name, cipher_view2.name); + } +} diff --git a/crates/bitwarden-crypto/Cargo.toml b/crates/bitwarden-crypto/Cargo.toml index f2bbc6f7e..aa683dc7f 100644 --- a/crates/bitwarden-crypto/Cargo.toml +++ b/crates/bitwarden-crypto/Cargo.toml @@ -30,6 +30,7 @@ cbc = { version = ">=0.1.2, <0.2", features = ["alloc", "zeroize"] } generic-array = { version = ">=0.14.7, <1.0", features = ["zeroize"] } hkdf = ">=0.12.3, <0.13" hmac = ">=0.12.1, <0.13" +memsec = { version = "0.7.0", features = ["alloc_ext"] } num-bigint = ">=0.4, <0.5" num-traits = ">=0.2.15, <0.3" pbkdf2 = { version = ">=0.12.1, <0.13", default-features = false } diff --git a/crates/bitwarden-crypto/src/error.rs b/crates/bitwarden-crypto/src/error.rs index 2fc4d2591..f2ac92e19 100644 --- a/crates/bitwarden-crypto/src/error.rs +++ b/crates/bitwarden-crypto/src/error.rs @@ -21,6 +21,8 @@ pub enum CryptoError { MissingKey(Uuid), #[error("The item was missing a required field: {0}")] MissingField(&'static str), + #[error("Missing Key for Ref. {0}")] + MissingKey2(String), #[error("EncString error, {0}")] EncString(#[from] EncStringParseError), diff --git a/crates/bitwarden-crypto/src/lib.rs b/crates/bitwarden-crypto/src/lib.rs index 4c71b0029..04c441e47 100644 --- a/crates/bitwarden-crypto/src/lib.rs +++ b/crates/bitwarden-crypto/src/lib.rs @@ -81,6 +81,7 @@ mod wordlist; pub use wordlist::EFF_LONG_WORD_LIST; mod allocator; pub use allocator::ZeroizingAllocator; +pub mod service; #[cfg(feature = "uniffi")] uniffi::setup_scaffolding!(); diff --git a/crates/bitwarden-crypto/src/service/crypto_engine/mod.rs b/crates/bitwarden-crypto/src/service/crypto_engine/mod.rs new file mode 100644 index 000000000..404ad201d --- /dev/null +++ b/crates/bitwarden-crypto/src/service/crypto_engine/mod.rs @@ -0,0 +1,102 @@ +use crate::{ + service::{AsymmetricKeyRef, SymmetricKeyRef}, + AsymmetricEncString, EncString, SymmetricCryptoKey, +}; + +mod rust_impl; + +pub(crate) use rust_impl::RustCryptoEngine; + +// This trait represents a service that can store cryptographic keys and perform operations with +// them, ideally within a Secure Enclave or HSM. Users of this trait will not handle the keys +// directly but will use references to them. +// +// For the cases where a secure element capable of doing cryptographic operations is not available, +// but there is a secure way to store keys, `KeyStore` can be implemented and then used in +// conjunction with `RustCryptoEngine`. +pub(crate) trait CryptoEngine { + /// Create a new context for this service. This allows the user to perform cryptographic + // operations with keys that are only relevant to the current context. + /// + /// NOTE: This is an advanced API, and should be used with care. Particularly, it's important + /// to ensure the context is dropped when it's no longer needed, to avoid holding a reference to + /// the RwLock for too long. It's also important to ensure that the context is cleared of + /// keys after every use if it's being reused, to avoid it growing indefinitely. + fn context(&'_ self) -> Box + '_>; + + #[deprecated(note = "We should be generating/decrypting the keys into the service directly")] + fn insert_symmetric_key(&self, key_ref: SymmKeyRef, key: SymmetricCryptoKey); + + fn clear(&self); +} + +// This trait represents a context for a `CryptoEngine`. It allows the user to perform cryptographic +// operations with keys that are only relevant to the current context. +#[allow(dead_code)] +pub(crate) trait CryptoEngineContext<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> +{ + fn clear(&mut self); + + // Symmetric key operations + + fn decrypt_data_with_symmetric_key( + &self, + key: SymmKeyRef, + data: &EncString, + ) -> Result, crate::CryptoError>; + + fn encrypt_data_with_symmetric_key( + &self, + key: SymmKeyRef, + data: &[u8], + ) -> Result; + + fn decrypt_and_store_symmetric_key( + &mut self, + encryption_key: SymmKeyRef, + new_key_ref: SymmKeyRef, + encrypted_key: &EncString, + ) -> Result; + + fn encrypt_symmetric_key( + &self, + encryption_key: SymmKeyRef, + key_to_encrypt: SymmKeyRef, + ) -> Result; + + // Asymmetric key operations + + fn decrypt_data_with_asymmetric_key( + &self, + key: AsymmKeyRef, + data: &AsymmetricEncString, + ) -> Result, crate::CryptoError>; + + fn encrypt_data_with_asymmetric_key( + &self, + key: AsymmKeyRef, + data: &[u8], + ) -> Result; + + fn decrypt_and_store_asymmetric_key( + &mut self, + encryption_key: AsymmKeyRef, + new_key_ref: AsymmKeyRef, + encrypted_key: &AsymmetricEncString, + ) -> Result; + + fn encrypt_asymmetric_key( + &self, + encryption_key: AsymmKeyRef, + key_to_encrypt: AsymmKeyRef, + ) -> Result; +} + +fn _ensure_that_traits_are_object_safe< + SymmKeyRef: SymmetricKeyRef, + AsymmKeyRef: AsymmetricKeyRef, +>( + _: Box>, + _: Box>, +) { +} diff --git a/crates/bitwarden-crypto/src/service/crypto_engine/rust_impl.rs b/crates/bitwarden-crypto/src/service/crypto_engine/rust_impl.rs new file mode 100644 index 000000000..69164b361 --- /dev/null +++ b/crates/bitwarden-crypto/src/service/crypto_engine/rust_impl.rs @@ -0,0 +1,250 @@ +use std::sync::RwLock; + +use rsa::Oaep; + +use crate::{ + service::{ + crypto_engine::{CryptoEngine, CryptoEngineContext}, + key_ref::KeyRef, + key_store::KeyStore, + AsymmetricKeyRef, SymmetricKeyRef, + }, + AsymmetricCryptoKey, AsymmetricEncString, CryptoError, EncString, SymmetricCryptoKey, +}; + +fn create_key_store() -> Box> { + #[cfg(target_os = "linux")] + if let Some(key_store) = crate::service::key_store::LinuxMemfdSecretKeyStore::::new() { + return Box::new(key_store); + } + + Box::new(crate::service::key_store::RustKeyStore::new()) +} + +pub(crate) struct RustCryptoEngine { + key_stores: RwLock>, +} + +// This is just a wrapper around the keys so we only deal with one RwLock +struct RustCryptoEngineKeys { + symmetric_keys: Box>, + asymmetric_keys: Box>, +} + +impl + RustCryptoEngine +{ + pub(crate) fn new() -> Self { + Self { + key_stores: RwLock::new(RustCryptoEngineKeys { + symmetric_keys: create_key_store(), + asymmetric_keys: create_key_store(), + }), + } + } +} + +impl + CryptoEngine for RustCryptoEngine +{ + fn context(&'_ self) -> Box + '_> { + // TODO: Cache these?, or maybe initialize them lazily? or both? + Box::new(RustCryptoEngineContext { + global_keys: self.key_stores.read().expect("RwLock is poisoned"), + local_symmetric_keys: create_key_store(), + local_asymmetric_keys: create_key_store(), + }) + } + + fn insert_symmetric_key(&self, key_ref: SymmKeyRef, key: SymmetricCryptoKey) { + self.key_stores + .write() + .expect("RwLock is poisoned") + .symmetric_keys + .insert(key_ref, key); + } + + fn clear(&self) { + let mut keys = self.key_stores.write().expect("RwLock is poisoned"); + keys.symmetric_keys.clear(); + keys.asymmetric_keys.clear(); + } +} + +pub(crate) struct RustCryptoEngineContext< + 'a, + SymmKeyRef: SymmetricKeyRef, + AsymmKeyRef: AsymmetricKeyRef, +> { + // We hold a RwLock read guard to avoid having any nested + //calls locking it again and potentially causing a deadlock + global_keys: std::sync::RwLockReadGuard<'a, RustCryptoEngineKeys>, + + local_symmetric_keys: Box>, + local_asymmetric_keys: Box>, +} + +impl<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> + RustCryptoEngineContext<'a, SymmKeyRef, AsymmKeyRef> +{ + fn get_symmetric_key( + &self, + key_ref: SymmKeyRef, + ) -> Result<&SymmetricCryptoKey, crate::CryptoError> { + if key_ref.is_local() { + self.local_symmetric_keys.get(key_ref) + } else { + self.global_keys.symmetric_keys.get(key_ref) + } + .ok_or_else(|| crate::CryptoError::MissingKey2(format!("{key_ref:?}"))) + } + + fn get_asymmetric_key( + &self, + key_ref: AsymmKeyRef, + ) -> Result<&AsymmetricCryptoKey, crate::CryptoError> { + if key_ref.is_local() { + self.local_asymmetric_keys.get(key_ref) + } else { + self.global_keys.asymmetric_keys.get(key_ref) + } + .ok_or_else(|| crate::CryptoError::MissingKey2(format!("{key_ref:?}"))) + } +} + +impl<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> + CryptoEngineContext<'a, SymmKeyRef, AsymmKeyRef> + for RustCryptoEngineContext<'a, SymmKeyRef, AsymmKeyRef> +{ + fn clear(&mut self) { + self.local_symmetric_keys.clear(); + self.local_asymmetric_keys.clear(); + } + + fn decrypt_data_with_symmetric_key( + &self, + key: SymmKeyRef, + data: &EncString, + ) -> Result, crate::CryptoError> { + let key = self.get_symmetric_key(key)?; + + match data { + EncString::AesCbc256_B64 { iv, data } => { + let dec = crate::aes::decrypt_aes256(iv, data.clone(), &key.key)?; + Ok(dec) + } + EncString::AesCbc128_HmacSha256_B64 { iv, mac, data } => { + // TODO: SymmetricCryptoKey is designed to handle 32 byte keys only, but this + // variant uses a 16 byte key This means the key+mac are going to be + // parsed as a single 32 byte key, at the moment we split it manually + // When refactoring the key handling, this should be fixed. + let enc_key = (&key.key[0..16]).into(); + let mac_key = (&key.key[16..32]).into(); + let dec = crate::aes::decrypt_aes128_hmac(iv, mac, data.clone(), mac_key, enc_key)?; + Ok(dec) + } + EncString::AesCbc256_HmacSha256_B64 { iv, mac, data } => { + let mac_key = key.mac_key.as_ref().ok_or(CryptoError::InvalidMac)?; + let dec = + crate::aes::decrypt_aes256_hmac(iv, mac, data.clone(), mac_key, &key.key)?; + Ok(dec) + } + } + } + + fn decrypt_and_store_symmetric_key( + &mut self, + encryption_key: SymmKeyRef, + new_key_ref: SymmKeyRef, + encrypted_key: &EncString, + ) -> Result { + let mut new_key_material = + self.decrypt_data_with_symmetric_key(encryption_key, encrypted_key)?; + + let new_key = SymmetricCryptoKey::try_from(new_key_material.as_mut_slice())?; + self.local_symmetric_keys.insert(new_key_ref, new_key); + Ok(new_key_ref) + } + + fn encrypt_data_with_symmetric_key( + &self, + key: SymmKeyRef, + data: &[u8], + ) -> Result { + let key = self.get_symmetric_key(key)?; + EncString::encrypt_aes256_hmac( + data, + key.mac_key.as_ref().ok_or(CryptoError::InvalidMac)?, + &key.key, + ) + } + + fn encrypt_symmetric_key( + &self, + encryption_key: SymmKeyRef, + key_to_encrypt: SymmKeyRef, + ) -> Result { + let key_to_encrypt = self.get_symmetric_key(key_to_encrypt)?; + self.encrypt_data_with_symmetric_key(encryption_key, &key_to_encrypt.to_vec()) + } + + fn decrypt_data_with_asymmetric_key( + &self, + key: AsymmKeyRef, + data: &AsymmetricEncString, + ) -> Result, crate::CryptoError> { + let key = self.get_asymmetric_key(key)?; + + use AsymmetricEncString::*; + match data { + Rsa2048_OaepSha256_B64 { data } => key.key.decrypt(Oaep::new::(), data), + Rsa2048_OaepSha1_B64 { data } => key.key.decrypt(Oaep::new::(), data), + #[allow(deprecated)] + Rsa2048_OaepSha256_HmacSha256_B64 { data, .. } => { + key.key.decrypt(Oaep::new::(), data) + } + #[allow(deprecated)] + Rsa2048_OaepSha1_HmacSha256_B64 { data, .. } => { + key.key.decrypt(Oaep::new::(), data) + } + } + .map_err(|_| CryptoError::KeyDecrypt) + } + + fn decrypt_and_store_asymmetric_key( + &mut self, + encryption_key: AsymmKeyRef, + new_key_ref: AsymmKeyRef, + encrypted_key: &AsymmetricEncString, + ) -> Result { + let new_key_material = + self.decrypt_data_with_asymmetric_key(encryption_key, encrypted_key)?; + + let new_key = AsymmetricCryptoKey::from_der(&new_key_material)?; + self.local_asymmetric_keys.insert(new_key_ref, new_key); + Ok(new_key_ref) + } + + fn encrypt_data_with_asymmetric_key( + &self, + key: AsymmKeyRef, + data: &[u8], + ) -> Result { + let key = self.get_asymmetric_key(key)?; + AsymmetricEncString::encrypt_rsa2048_oaep_sha1(data, key) + } + + fn encrypt_asymmetric_key( + &self, + encryption_key: AsymmKeyRef, + key_to_encrypt: AsymmKeyRef, + ) -> Result { + let encryption_key = self.get_asymmetric_key(encryption_key)?; + let key_to_encrypt = self.get_asymmetric_key(key_to_encrypt)?; + + AsymmetricEncString::encrypt_rsa2048_oaep_sha1( + key_to_encrypt.to_der()?.as_slice(), + encryption_key, + ) + } +} diff --git a/crates/bitwarden-crypto/src/service/encryptable.rs b/crates/bitwarden-crypto/src/service/encryptable.rs new file mode 100644 index 000000000..98fc24500 --- /dev/null +++ b/crates/bitwarden-crypto/src/service/encryptable.rs @@ -0,0 +1,130 @@ +use super::{ + key_ref::{AsymmetricKeyRef, KeyRef, SymmetricKeyRef}, + CryptoServiceContext, +}; + +pub trait Encryptable< + SymmKeyRef: SymmetricKeyRef, + AsymmKeyRef: AsymmetricKeyRef, + Key: KeyRef, + Output, +> +{ + fn encrypt( + &self, + ctx: &mut CryptoServiceContext, + key: Key, + ) -> Result; +} + +pub trait Decryptable< + SymmKeyRef: SymmetricKeyRef, + AsymmKeyRef: AsymmetricKeyRef, + Key: KeyRef, + Output, +> +{ + fn decrypt( + &self, + ctx: &mut CryptoServiceContext, + key: Key, + ) -> Result; +} + +impl + Decryptable> for EncString +{ + fn decrypt( + &self, + ctx: &mut CryptoServiceContext, + key: SymmKeyRef, + ) -> Result, crate::CryptoError> { + ctx.engine.decrypt_data_with_symmetric_key(key, self) + } +} + +impl + Decryptable> for AsymmetricEncString +{ + fn decrypt( + &self, + ctx: &mut CryptoServiceContext, + key: AsymmKeyRef, + ) -> Result, crate::CryptoError> { + ctx.engine.decrypt_data_with_asymmetric_key(key, self) + } +} + +impl + Encryptable for [u8] +{ + fn encrypt( + &self, + ctx: &mut CryptoServiceContext, + key: SymmKeyRef, + ) -> Result { + ctx.engine.encrypt_data_with_symmetric_key(key, self) + } +} + +impl + Encryptable for [u8] +{ + fn encrypt( + &self, + ctx: &mut CryptoServiceContext, + key: AsymmKeyRef, + ) -> Result { + ctx.engine.encrypt_data_with_asymmetric_key(key, self) + } +} + +impl + Decryptable for EncString +{ + fn decrypt( + &self, + ctx: &mut CryptoServiceContext, + key: SymmKeyRef, + ) -> Result { + let bytes: Vec = self.decrypt(ctx, key)?; + String::from_utf8(bytes).map_err(|_| CryptoError::InvalidUtf8String) + } +} + +impl + Decryptable for AsymmetricEncString +{ + fn decrypt( + &self, + ctx: &mut CryptoServiceContext, + key: AsymmKeyRef, + ) -> Result { + let bytes: Vec = self.decrypt(ctx, key)?; + String::from_utf8(bytes).map_err(|_| CryptoError::InvalidUtf8String) + } +} + +impl + Encryptable for str +{ + fn encrypt( + &self, + ctx: &mut CryptoServiceContext, + key: SymmKeyRef, + ) -> Result { + self.as_bytes().encrypt(ctx, key) + } +} + +impl + Encryptable for str +{ + fn encrypt( + &self, + ctx: &mut CryptoServiceContext, + key: AsymmKeyRef, + ) -> Result { + self.as_bytes().encrypt(ctx, key) + } +} diff --git a/crates/bitwarden-crypto/src/service/key_ref.rs b/crates/bitwarden-crypto/src/service/key_ref.rs new file mode 100644 index 000000000..f70f25fad --- /dev/null +++ b/crates/bitwarden-crypto/src/service/key_ref.rs @@ -0,0 +1,86 @@ +use std::{fmt::Debug, hash::Hash}; + +use zeroize::ZeroizeOnDrop; + +use crate::{AsymmetricCryptoKey, CryptoKey, SymmetricCryptoKey}; + +/// This trait represents a key reference that can be used to identify cryptographic keys in the key +/// store. It is used to abstract over the different types of keys that can be used in the system, +/// an end user would not implement this trait directly, and would instead use the `SymmetricKeyRef` +/// and `AsymmetricKeyRef` traits. +pub trait KeyRef: + Debug + Clone + Copy + Hash + Eq + PartialEq + Ord + PartialOrd + 'static +{ + type KeyValue: Debug + CryptoKey + ZeroizeOnDrop; + + /// Returns whether the key is local to the current context or shared globally by the service. + fn is_local(&self) -> bool; +} + +// These traits below are just basic aliases of the `KeyRef` trait, but they allow us to have two +// separate trait bounds + +pub trait SymmetricKeyRef: KeyRef {} +pub trait AsymmetricKeyRef: KeyRef {} + +// Hide the `KeyRef` trait from the public API, to avoid confusion +#[doc(hidden)] +pub mod __macro_internal { + pub use super::KeyRef; +} + +// Just a test of a derive_like macro that can be used to generate the key reference enums. +// Example usage: +// ```rust +// key_refs! { +// #[symmetric] +// pub enum KeyRef { +// User, +// Org(Uuid), +// #[local] +// Local(String), +// } +// } +#[macro_export] +macro_rules! key_refs { + ( $( + #[$meta_type:tt] + $(pub)? enum $name:ident { + $( + $( #[$variant_tag:tt] )? + $variant:ident $( ( $inner:ty ) )? + ,)+ + } + )+ ) => { $( + #[derive(std::fmt::Debug, Clone, Copy, std::hash::Hash, Eq, PartialEq, Ord, PartialOrd)] + pub enum $name { $( + $variant $( ($inner) )? + ,)+ } + + impl $crate::service::key_ref::__macro_internal::KeyRef for $name { + type KeyValue = key_refs!(@key_type $meta_type); + + fn is_local(&self) -> bool { + use $name::*; + match self { $( + key_refs!(@variant_match $variant $( ( $inner ) )?) => + key_refs!(@variant_tag $( $variant_tag )? ), + )+ } + } + } + + key_refs!(@key_trait $meta_type $name); + )+ }; + + ( @key_type symmetric ) => { $crate::SymmetricCryptoKey }; + ( @key_type asymmetric ) => { $crate::AsymmetricCryptoKey }; + + ( @key_trait symmetric $name:ident ) => { impl $crate::service::key_ref::SymmetricKeyRef for $name {} }; + ( @key_trait asymmetric $name:ident ) => { impl $crate::service::key_ref::AsymmetricKeyRef for $name {} }; + + ( @variant_match $variant:ident ( $inner:ty ) ) => { $variant (_) }; + ( @variant_match $variant:ident ) => { $variant }; + + ( @variant_tag local ) => { true }; + ( @variant_tag ) => { false }; +} diff --git a/crates/bitwarden-crypto/src/service/key_store/linux_memfd_secret_impl.rs b/crates/bitwarden-crypto/src/service/key_store/linux_memfd_secret_impl.rs new file mode 100644 index 000000000..23f335f0c --- /dev/null +++ b/crates/bitwarden-crypto/src/service/key_store/linux_memfd_secret_impl.rs @@ -0,0 +1,122 @@ +use std::{mem::MaybeUninit, ptr::NonNull}; + +use zeroize::{Zeroize, ZeroizeOnDrop}; + +use super::{ + util::{MemPtr, SliceKeyContainer}, + KeyRef, KeyStore, +}; + +// 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) struct LinuxMemfdSecretKeyStore { + container: SliceKeyContainer, + + _key: std::marker::PhantomData, +} + +impl LinuxMemfdSecretKeyStore { + pub(crate) fn new() -> Option { + // This might not be exactly correct in all platforms, but it's a good enough approximation + const PAGE_SIZE: usize = 4096; + let entry_size = std::mem::size_of::>(); + let elements_per_page = PAGE_SIZE / entry_size; + + // We're using mlock APIs to protect the memory, so allocating less than a page is a waste + let capacity = std::cmp::max(32, elements_per_page); + + Self::with_capacity(capacity) + } + + pub(crate) fn with_capacity(capacity: usize) -> Option { + let entry_size = std::mem::size_of::>(); + + let memory = unsafe { + let ptr: NonNull<[u8]> = memsec::memfd_secret_sized(capacity * entry_size)?; + MemPtr::new(ptr, capacity) + }; + + let container = SliceKeyContainer::new(memory); + + // Validate that the entry size is correct + debug_assert_eq!(container.entry_size(), entry_size); + + Some(Self { + container, + _key: std::marker::PhantomData, + }) + } +} + +impl ZeroizeOnDrop for LinuxMemfdSecretKeyStore {} + +impl Drop for LinuxMemfdSecretKeyStore { + fn drop(&mut self) { + // Freeing the memory should clear all the secrets, but to ensure all the Drop impls + // are called correctly we clear the container first + self.container.clear(); + unsafe { + memsec::free_memfd_secret(self.container.inner_mut().as_ptr()); + } + } +} + +impl KeyStore for LinuxMemfdSecretKeyStore { + fn insert(&mut self, key_ref: Key, key: Key::KeyValue) { + if let Err(new_capacity) = self.container.ensure_capacity(1) { + // Create a new store with the correct capacity and replace self with it + let mut new_self = + Self::with_capacity(new_capacity).expect("Failed to allocate new memfd_secret"); + new_self.container.copy_from(&mut self.container); + *self = new_self; + }; + + let ok = self.container.insert(key_ref, key); + debug_assert!(ok, "insert failed"); + } + + fn get(&self, key_ref: Key) -> Option<&Key::KeyValue> { + self.container.get(key_ref) + } + fn remove(&mut self, key_ref: Key) { + self.container.remove(key_ref) + } + fn clear(&mut self) { + self.container.clear() + } + fn retain(&mut self, f: fn(Key) -> bool) { + self.container.retain(f); + } +} + +#[cfg(test)] +mod tests { + use super::{super::util::tests::*, *}; + + #[test] + fn test_resize() { + let mut store = super::LinuxMemfdSecretKeyStore::::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.insert(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/service/key_store/mod.rs b/crates/bitwarden-crypto/src/service/key_store/mod.rs new file mode 100644 index 000000000..9a4d75f46 --- /dev/null +++ b/crates/bitwarden-crypto/src/service/key_store/mod.rs @@ -0,0 +1,27 @@ +use zeroize::ZeroizeOnDrop; + +use crate::service::KeyRef; + +#[cfg(target_os = "linux")] +mod linux_memfd_secret_impl; +mod rust_impl; +mod util; + +#[cfg(target_os = "linux")] +pub(crate) use linux_memfd_secret_impl::LinuxMemfdSecretKeyStore; +pub(crate) use rust_impl::RustKeyStore; + +/// This trait represents a platform that can securely store and return keys. The `RustKeyStore` +/// implementation is a simple in-memory store without any security guarantees. Other +/// implementations could use secure enclaves or HSMs, or OS provided keychains. +#[allow(dead_code)] +pub(crate) trait KeyStore: ZeroizeOnDrop { + fn insert(&mut self, key_ref: Key, key: Key::KeyValue); + fn get(&self, key_ref: Key) -> Option<&Key::KeyValue>; + fn remove(&mut self, key_ref: Key); + fn clear(&mut self); + + fn retain(&mut self, f: fn(Key) -> bool); +} + +fn _ensure_that_trait_is_object_safe(_: Box>) {} diff --git a/crates/bitwarden-crypto/src/service/key_store/rust_impl.rs b/crates/bitwarden-crypto/src/service/key_store/rust_impl.rs new file mode 100644 index 000000000..1944a58cd --- /dev/null +++ b/crates/bitwarden-crypto/src/service/key_store/rust_impl.rs @@ -0,0 +1,129 @@ +use zeroize::ZeroizeOnDrop; + +use super::{util::SliceKeyContainer, KeyRef, KeyStore}; + +// 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) struct RustKeyStore { + #[allow(clippy::type_complexity)] + container: SliceKeyContainer]>>, +} + +const ENABLE_MLOCK: bool = true; + +impl RustKeyStore { + pub(crate) fn new() -> Self { + // This might not be exactly correct in all platforms, but it's a good enough approximation + const PAGE_SIZE: usize = 4096; + let entry_size = std::mem::size_of::>(); + + let entries_per_page = PAGE_SIZE / entry_size; + + // We're using mlock APIs to protect the memory, so allocating less than a page is a waste + let capacity = std::cmp::max(32, entries_per_page); + + Self::with_capacity(capacity) + } + + pub(crate) fn with_capacity(capacity: usize) -> Self { + let entry_size = std::mem::size_of::>(); + + // This is a bit awkward, but we need to fill the entire slice with None, and we can't just + // use vec![None; capacity] because that requires adding a Clone bound to the key + // value + let mut keys: Box<_> = std::iter::repeat_with(|| None).take(capacity).collect(); + + if ENABLE_MLOCK { + unsafe { + memsec::mlock(keys.as_mut_ptr() as *mut u8, capacity * entry_size); + } + } + + let container = SliceKeyContainer::new(keys); + + // Validate that the entry size is correct + debug_assert_eq!(container.entry_size(), entry_size); + + Self { container } + } +} + +impl ZeroizeOnDrop for RustKeyStore {} + +impl Drop for RustKeyStore { + fn drop(&mut self) { + if ENABLE_MLOCK { + // We need to ensure the values get dropped and zeroized _before_ + // the mlock gets removed, to avoid any last minute swaps to disk + self.container.clear(); + + unsafe { + memsec::munlock( + self.container.inner_mut().as_mut_ptr() as *mut u8, + self.container.byte_len(), + ); + } + } + } +} + +impl KeyStore for RustKeyStore { + fn insert(&mut self, key_ref: Key, key: Key::KeyValue) { + if let Err(new_capacity) = self.container.ensure_capacity(1) { + // Create a new store with the correct capacity and replace self with it + let mut new_self = Self::with_capacity(new_capacity); + new_self.container.copy_from(&mut self.container); + *self = new_self; + }; + + let ok = self.container.insert(key_ref, key); + debug_assert!(ok, "insert failed"); + } + + fn get(&self, key_ref: Key) -> Option<&Key::KeyValue> { + self.container.get(key_ref) + } + + fn remove(&mut self, key_ref: Key) { + self.container.remove(key_ref); + } + + fn clear(&mut self) { + self.container.clear(); + } + + fn retain(&mut self, f: fn(Key) -> bool) { + self.container.retain(f); + } +} + +#[cfg(test)] +mod tests { + use super::{super::util::tests::*, *}; + + #[test] + fn test_resize() { + let mut store = super::RustKeyStore::::with_capacity(1); + + for (idx, key) in [ + TestKey::A, + TestKey::B(10), + TestKey::C, + TestKey::B(7), + TestKey::A, + TestKey::C, + ] + .into_iter() + .enumerate() + { + store.insert(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/service/key_store/util.rs b/crates/bitwarden-crypto/src/service/key_store/util.rs new file mode 100644 index 000000000..2b527d14a --- /dev/null +++ b/crates/bitwarden-crypto/src/service/key_store/util.rs @@ -0,0 +1,719 @@ +use crate::service::key_ref::KeyRef; + +pub(crate) trait AsSlice { + fn as_slice(&self) -> &[Option<(Key, Key::KeyValue)>]; + fn as_mut_slice(&mut self) -> &mut [Option<(Key, Key::KeyValue)>]; +} + +impl AsSlice for Box<[Option<(Key, Key::KeyValue)>]> { + fn as_slice(&self) -> &[Option<(Key, Key::KeyValue)>] { + self.as_ref() + } + + fn as_mut_slice(&mut self) -> &mut [Option<(Key, Key::KeyValue)>] { + self.as_mut() + } +} + +#[cfg(target_os = "linux")] +pub(crate) struct MemPtr { + ptr: std::ptr::NonNull<[u8]>, + capacity: usize, +} + +#[cfg(target_os = "linux")] +impl MemPtr { + /// SAFETY: The caller must ensure that the pointer is valid, correctly aligned + pub unsafe fn new(ptr: std::ptr::NonNull<[u8]>, capacity: usize) -> MemPtr { + MemPtr { ptr, capacity } + } + + pub unsafe fn as_ptr(&self) -> std::ptr::NonNull<[u8]> { + self.ptr + } +} + +#[cfg(target_os = "linux")] +impl AsSlice for MemPtr { + fn as_slice(&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. + unsafe { std::slice::from_raw_parts(ptr, self.capacity) } + } + + fn as_mut_slice(&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. + unsafe { std::slice::from_raw_parts_mut(ptr, self.capacity) } + } +} + +/// This represents a container 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 Vec, a Box<[u8]> or a NonNull. +pub(crate) struct SliceKeyContainer> { + data: Data, + + // 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. + capacity: usize, + + _key: std::marker::PhantomData, +} + +#[allow(dead_code)] +impl> SliceKeyContainer { + pub(crate) fn new(data: S) -> Self { + let capacity = data.as_slice().len(); + + debug_assert!( + capacity > 0, + "The container should have a capacity of at least 1" + ); + + let mut container = Self { + data, + length: 0, + capacity, + _key: std::marker::PhantomData, + }; + + // Ensure the container is properly initialized + container.clear(); + + container + } + + pub(crate) const fn entry_size(&self) -> usize { + std::mem::size_of::>() + } + + pub(crate) unsafe fn inner_mut(&mut self) -> &mut S { + &mut self.data + } + + pub(crate) fn len(&self) -> usize { + self.length + } + + pub(crate) fn byte_len(&self) -> usize { + self.length * self.entry_size() + } + + /// 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. + pub(crate) fn ensure_capacity(&self, new_elements: usize) -> Result<(), usize> { + let new_size = self.length + new_elements; + + if new_size > self.capacity { + // 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) + } else { + Ok(()) + } + } + + 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.data.as_slice()[..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 clear(&mut self) { + self.data.as_mut_slice().fill_with(|| None); + self.length = 0; + } + + pub(crate) fn remove(&mut self, key_ref: Key) { + if let Ok(idx) = self.find_by_key_ref(&key_ref) { + let slice = self.data.as_mut_slice(); + slice[idx] = None; + slice[idx..self.length].rotate_left(1); + self.length -= 1; + } + } + + pub(crate) fn insert(&mut self, key_ref: Key, key: ::KeyValue) -> bool { + match self.find_by_key_ref(&key_ref) { + Ok(idx) => { + // Key already exists, we just need to replace the value + let slice = self.data.as_mut_slice(); + slice[idx] = Some((key_ref, key)); + } + Err(idx) => { + // We need to insert the key, check if we have enough space + if self.length >= self.capacity { + return false; + } + + let slice = self.data.as_mut_slice(); + if idx < self.length { + // If we're not right at the end, we have to shift all the following elements + // one position to the right + slice[idx..=self.length].rotate_right(1); + } + slice[idx] = Some((key_ref, key)); + self.length += 1; + } + } + + true + } + + pub(crate) fn get(&self, key_ref: Key) -> Option<&::KeyValue> { + self.find_by_key_ref(&key_ref) + .ok() + .and_then(|idx| self.data.as_slice().get(idx)) + .and_then(|f| f.as_ref().map(|f| &f.1)) + } + + pub(crate) fn retain(&mut self, f: fn(Key) -> bool) { + let slice = self.data.as_mut_slice(); + + let mut removed_elements = 0; + + for value in slice.iter_mut().take(self.length) { + 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..self.length { + if slice[idx].is_none() { + slice[idx..self.length].rotate_left(1); + } + } + + self.length -= removed_elements; + } + + pub(crate) fn copy_from(&mut self, other: &mut Self) -> bool { + if other.capacity > self.capacity { + return false; + } + + // Empty the current container + self.clear(); + + // Move the data from the other container + let this = self.data.as_mut_slice(); + let that = other.data.as_mut_slice(); + for idx in 0..other.length { + std::mem::swap(&mut this[idx], &mut that[idx]); + } + + // Update the length + self.length = other.length; + + true + } +} + +#[cfg(test)] +pub(crate) mod tests { + use zeroize::Zeroize; + + use super::*; + use crate::{service::key_ref::KeyRef, CryptoKey}; + + #[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 KeyRef for TestKey { + type KeyValue = TestKeyValue; + + fn is_local(&self) -> bool { + false + } + } + + #[test] + fn test_slice_container_insertion() { + let mut container = SliceKeyContainer::::new(vec![None; 5].into_boxed_slice()); + + assert_eq!(container.data.as_slice(), [None, None, None, None, None]); + + // Insert one key, which should be at the beginning + assert!(container.insert(TestKey::B(10), TestKeyValue::new(110))); + assert_eq!( + container.data.as_slice(), + [ + Some((TestKey::B(10), TestKeyValue::new(110))), + None, + None, + None, + None + ] + ); + + // Insert a key that should be right after the first one + assert!(container.insert(TestKey::C, TestKeyValue::new(1000))); + assert_eq!( + container.data.as_slice(), + [ + Some((TestKey::B(10), TestKeyValue::new(110))), + Some((TestKey::C, TestKeyValue::new(1000))), + None, + None, + None + ] + ); + + // Insert a key in the middle + assert!(container.insert(TestKey::B(20), TestKeyValue::new(210))); + assert_eq!( + container.data.as_slice(), + [ + 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 + assert!(container.insert(TestKey::A, TestKeyValue::new(0))); + assert_eq!( + container.data.as_slice(), + [ + 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 + assert!(container.insert(TestKey::B(30), TestKeyValue::new(310))); + assert_eq!( + container.data.as_slice(), + [ + 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 + assert!(container.insert(TestKey::A, TestKeyValue::new(1))); + assert_eq!( + container.data.as_slice(), + [ + 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 + assert!(container.insert(TestKey::B(20), TestKeyValue::new(211))); + assert_eq!( + container.data.as_slice(), + [ + 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 + assert!(container.insert(TestKey::C, TestKeyValue::new(1001))); + assert_eq!( + container.data.as_slice(), + [ + 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 = SliceKeyContainer::::new(vec![None; 5].into_boxed_slice()); + + for (key, value) in [ + (TestKey::A, TestKeyValue::new(1)), + (TestKey::B(10), TestKeyValue::new(110)), + (TestKey::C, TestKeyValue::new(1000)), + ] { + assert!(container.insert(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 = SliceKeyContainer::::new(vec![None; 5].into_boxed_slice()); + + 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)), + ] { + assert!(container.insert(key, value)); + } + + assert_eq!( + container.data.as_slice(), + [ + 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.data.as_slice(), [None, None, None, None, None]); + } + + #[test] + fn test_slice_container_ensure_capacity() { + let mut container = SliceKeyContainer::::new(vec![None; 5].into_boxed_slice()); + assert_eq!(container.capacity, 5); + assert_eq!(container.length, 0); + + assert_eq!(container.ensure_capacity(0), Ok(())); + assert_eq!(container.ensure_capacity(6), Err(10)); + assert_eq!(container.ensure_capacity(10), Err(10)); + assert_eq!(container.ensure_capacity(11), Err(15)); + assert_eq!(container.ensure_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)), + ] { + assert!(container.insert(key, value)); + } + + assert_eq!(container.ensure_capacity(0), Ok(())); + assert_eq!(container.ensure_capacity(6), Err(15)); + assert_eq!(container.ensure_capacity(10), Err(15)); + assert_eq!(container.ensure_capacity(11), Err(20)); + assert_eq!(container.ensure_capacity(51), Err(60)); + } + + #[test] + fn test_slice_container_removal() { + let mut container = SliceKeyContainer::::new(vec![None; 5].into_boxed_slice()); + + 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)), + ] { + assert!(container.insert(key, value)); + } + + // Remove the last element + container.remove(TestKey::C); + assert_eq!( + container.data.as_slice(), + [ + 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.data.as_slice(), + [ + 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.data.as_slice(), + [ + 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.data.as_slice(), + [ + 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.data.as_slice(), + [ + Some((TestKey::B(10), TestKeyValue::new(110))), + None, + None, + None, + None + ] + ); + container.remove(TestKey::B(10)); + assert_eq!(container.data.as_slice(), [None, None, None, None, None]); + + // Remove from an empty container + container.remove(TestKey::B(10)); + assert_eq!(container.data.as_slice(), [None, None, None, None, None]); + } + + #[test] + fn test_slice_container_retain_removes_one() { + let mut container = SliceKeyContainer::::new(vec![None; 5].into_boxed_slice()); + + 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)), + ] { + assert!(container.insert(key, value)); + } + + // Remove the last element + container.retain(|k| k != TestKey::C); + assert_eq!( + container.data.as_slice(), + [ + 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.data.as_slice(), + [ + 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.data.as_slice(), + [ + 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.data.as_slice(), + [ + 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.data.as_slice(), + [ + Some((TestKey::B(10), TestKeyValue::new(110))), + None, + None, + None, + None + ] + ); + container.retain(|k| k != TestKey::B(10)); + assert_eq!(container.data.as_slice(), [None, None, None, None, None]); + + // Remove from an empty container + container.retain(|k| k != TestKey::B(10)); + assert_eq!(container.data.as_slice(), [None, None, None, None, None]); + } + + #[test] + fn test_slice_container_retain_removes_none() { + let mut container = SliceKeyContainer::::new(vec![None; 5].into_boxed_slice()); + + 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)), + ] { + assert!(container.insert(key, value)); + } + + container.retain(|_k| true); + assert_eq!( + container.data.as_slice(), + [ + 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 = SliceKeyContainer::::new(vec![None; 5].into_boxed_slice()); + + 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)), + ] { + assert!(container.insert(key, value)); + } + + container.retain(|k| matches!(k, TestKey::A | TestKey::B(20) | TestKey::C)); + assert_eq!( + container.data.as_slice(), + [ + 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 = SliceKeyContainer::::new(vec![None; 5].into_boxed_slice()); + + 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)), + ] { + assert!(container.insert(key, value)); + } + + container.retain(|_k| false); + assert_eq!(container.data.as_slice(), [None, None, None, None, None]); + } +} diff --git a/crates/bitwarden-crypto/src/service/mod.rs b/crates/bitwarden-crypto/src/service/mod.rs new file mode 100644 index 000000000..a1416e776 --- /dev/null +++ b/crates/bitwarden-crypto/src/service/mod.rs @@ -0,0 +1,82 @@ +use std::sync::Arc; + +use crate::{EncString, SymmetricCryptoKey}; + +mod crypto_engine; +mod encryptable; +pub mod key_ref; +mod key_store; + +use crypto_engine::{CryptoEngine, CryptoEngineContext, RustCryptoEngine}; +pub use encryptable::{Decryptable, Encryptable}; +use key_ref::{AsymmetricKeyRef, KeyRef, SymmetricKeyRef}; + +#[derive(Clone)] +pub struct CryptoService { + // We use an Arc<> to make it easier to pass this service around, as we can + // clone it instead of passing references + engine: Arc>, +} + +impl + CryptoService +{ + #[allow(clippy::new_without_default)] + pub fn new() -> Self { + Self { + engine: Arc::new(RustCryptoEngine::new()), + } + } + + pub fn clear(&self) { + self.engine.clear(); + } + + #[deprecated(note = "We should be generating/decrypting the keys into the service directly")] + pub fn insert_symmetric_key(&self, key_ref: SymmKeyRef, key: SymmetricCryptoKey) { + #[allow(deprecated)] + self.engine.insert_symmetric_key(key_ref, key); + } + + pub fn context(&'_ self) -> CryptoServiceContext<'_, SymmKeyRef, AsymmKeyRef> { + CryptoServiceContext { + engine: self.engine.context(), + } + } + + // These are just convenience methods to avoid having to call `context` every time + pub fn decrypt, Output>( + &self, + key: Key, + data: &Data, + ) -> Result { + data.decrypt(&mut self.context(), key) + } + + pub fn encrypt, Output>( + &self, + key: Key, + data: Data, + ) -> Result { + data.encrypt(&mut self.context(), key) + } +} + +pub struct CryptoServiceContext<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> { + engine: Box + 'a>, +} + +impl<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> + CryptoServiceContext<'a, SymmKeyRef, AsymmKeyRef> +{ + /// Decrypt a key and store it in the local key store + pub fn decrypt_and_store_symmetric_key( + &mut self, + encryption_key: SymmKeyRef, + new_key_ref: SymmKeyRef, + encrypted_key: &EncString, + ) -> Result { + self.engine + .decrypt_and_store_symmetric_key(encryption_key, new_key_ref, encrypted_key) + } +} From 6068e8489920b50d58b017210f97d959376191f5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garci=CC=81a?= Date: Fri, 23 Aug 2024 15:41:02 +0200 Subject: [PATCH 02/34] More work --- .../src/service/crypto_engine/mod.rs | 8 ++++++-- .../src/service/crypto_engine/rust_impl.rs | 16 ++++++++++++---- crates/bitwarden-crypto/src/service/mod.rs | 3 ++- 3 files changed, 20 insertions(+), 7 deletions(-) diff --git a/crates/bitwarden-crypto/src/service/crypto_engine/mod.rs b/crates/bitwarden-crypto/src/service/crypto_engine/mod.rs index 404ad201d..03a9f0d91 100644 --- a/crates/bitwarden-crypto/src/service/crypto_engine/mod.rs +++ b/crates/bitwarden-crypto/src/service/crypto_engine/mod.rs @@ -39,6 +39,8 @@ pub(crate) trait CryptoEngineContext<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRe // Symmetric key operations + fn remove_symmetric_key(&mut self, key_ref: SymmKeyRef); + fn decrypt_data_with_symmetric_key( &self, key: SymmKeyRef, @@ -56,7 +58,7 @@ pub(crate) trait CryptoEngineContext<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRe encryption_key: SymmKeyRef, new_key_ref: SymmKeyRef, encrypted_key: &EncString, - ) -> Result; + ) -> Result<(), crate::CryptoError>; fn encrypt_symmetric_key( &self, @@ -66,6 +68,8 @@ pub(crate) trait CryptoEngineContext<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRe // Asymmetric key operations + fn remove_asymmetric_key(&mut self, key_ref: AsymmKeyRef); + fn decrypt_data_with_asymmetric_key( &self, key: AsymmKeyRef, @@ -83,7 +87,7 @@ pub(crate) trait CryptoEngineContext<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRe encryption_key: AsymmKeyRef, new_key_ref: AsymmKeyRef, encrypted_key: &AsymmetricEncString, - ) -> Result; + ) -> Result<(), crate::CryptoError>; fn encrypt_asymmetric_key( &self, diff --git a/crates/bitwarden-crypto/src/service/crypto_engine/rust_impl.rs b/crates/bitwarden-crypto/src/service/crypto_engine/rust_impl.rs index 69164b361..d5f506cca 100644 --- a/crates/bitwarden-crypto/src/service/crypto_engine/rust_impl.rs +++ b/crates/bitwarden-crypto/src/service/crypto_engine/rust_impl.rs @@ -121,6 +121,10 @@ impl<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> self.local_asymmetric_keys.clear(); } + fn remove_symmetric_key(&mut self, key_ref: SymmKeyRef) { + self.local_symmetric_keys.remove(key_ref); + } + fn decrypt_data_with_symmetric_key( &self, key: SymmKeyRef, @@ -157,13 +161,13 @@ impl<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> encryption_key: SymmKeyRef, new_key_ref: SymmKeyRef, encrypted_key: &EncString, - ) -> Result { + ) -> Result<(), crate::CryptoError> { let mut new_key_material = self.decrypt_data_with_symmetric_key(encryption_key, encrypted_key)?; let new_key = SymmetricCryptoKey::try_from(new_key_material.as_mut_slice())?; self.local_symmetric_keys.insert(new_key_ref, new_key); - Ok(new_key_ref) + Ok(()) } fn encrypt_data_with_symmetric_key( @@ -188,6 +192,10 @@ impl<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> self.encrypt_data_with_symmetric_key(encryption_key, &key_to_encrypt.to_vec()) } + fn remove_asymmetric_key(&mut self, key_ref: AsymmKeyRef) { + self.local_asymmetric_keys.remove(key_ref); + } + fn decrypt_data_with_asymmetric_key( &self, key: AsymmKeyRef, @@ -216,13 +224,13 @@ impl<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> encryption_key: AsymmKeyRef, new_key_ref: AsymmKeyRef, encrypted_key: &AsymmetricEncString, - ) -> Result { + ) -> Result<(), crate::CryptoError> { let new_key_material = self.decrypt_data_with_asymmetric_key(encryption_key, encrypted_key)?; let new_key = AsymmetricCryptoKey::from_der(&new_key_material)?; self.local_asymmetric_keys.insert(new_key_ref, new_key); - Ok(new_key_ref) + Ok(()) } fn encrypt_data_with_asymmetric_key( diff --git a/crates/bitwarden-crypto/src/service/mod.rs b/crates/bitwarden-crypto/src/service/mod.rs index a1416e776..1a003be5c 100644 --- a/crates/bitwarden-crypto/src/service/mod.rs +++ b/crates/bitwarden-crypto/src/service/mod.rs @@ -38,7 +38,8 @@ impl self.engine.insert_symmetric_key(key_ref, key); } - pub fn context(&'_ self) -> CryptoServiceContext<'_, SymmKeyRef, AsymmKeyRef> { + // TODO: Do we want this to be public? + pub(crate) fn context(&'_ self) -> CryptoServiceContext<'_, SymmKeyRef, AsymmKeyRef> { CryptoServiceContext { engine: self.engine.context(), } From 50dc1b42f98d244e8bd392e1d1024baa51efe42b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garci=CC=81a?= Date: Fri, 23 Aug 2024 19:10:58 +0200 Subject: [PATCH 03/34] Refactor keystore to also handle resizes --- .../key_store/linux_memfd_secret_impl.rs | 105 +++--- .../src/service/key_store/rust_impl.rs | 101 ++--- .../src/service/key_store/util.rs | 345 +++++++++--------- 3 files changed, 295 insertions(+), 256 deletions(-) diff --git a/crates/bitwarden-crypto/src/service/key_store/linux_memfd_secret_impl.rs b/crates/bitwarden-crypto/src/service/key_store/linux_memfd_secret_impl.rs index 23f335f0c..c8fbfb3a7 100644 --- a/crates/bitwarden-crypto/src/service/key_store/linux_memfd_secret_impl.rs +++ b/crates/bitwarden-crypto/src/service/key_store/linux_memfd_secret_impl.rs @@ -1,12 +1,61 @@ -use std::{mem::MaybeUninit, ptr::NonNull}; +use std::{ptr::NonNull, sync::OnceLock}; -use zeroize::{Zeroize, ZeroizeOnDrop}; +use zeroize::ZeroizeOnDrop; use super::{ - util::{MemPtr, SliceKeyContainer}, + util::{KeyData, SliceKeyContainer}, KeyRef, KeyStore, }; +pub(crate) fn is_memfd_supported() -> 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 + }) +} + +pub(crate) struct MemPtr { + ptr: std::ptr::NonNull<[u8]>, + capacity: usize, +} + +impl Drop for MemPtr { + fn drop(&mut self) { + unsafe { + memsec::free_memfd_secret(self.ptr); + } + } +} + +impl KeyData for MemPtr { + fn new_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("Supported operation"); + MemPtr { 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. + 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. + unsafe { std::slice::from_raw_parts_mut(ptr, self.capacity) } + } +} + // 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 @@ -19,64 +68,28 @@ pub(crate) struct LinuxMemfdSecretKeyStore { impl LinuxMemfdSecretKeyStore { pub(crate) fn new() -> Option { - // This might not be exactly correct in all platforms, but it's a good enough approximation - const PAGE_SIZE: usize = 4096; - let entry_size = std::mem::size_of::>(); - let elements_per_page = PAGE_SIZE / entry_size; - - // We're using mlock APIs to protect the memory, so allocating less than a page is a waste - let capacity = std::cmp::max(32, elements_per_page); - - Self::with_capacity(capacity) + Self::with_capacity(0) } pub(crate) fn with_capacity(capacity: usize) -> Option { - let entry_size = std::mem::size_of::>(); - - let memory = unsafe { - let ptr: NonNull<[u8]> = memsec::memfd_secret_sized(capacity * entry_size)?; - MemPtr::new(ptr, capacity) - }; - - let container = SliceKeyContainer::new(memory); - - // Validate that the entry size is correct - debug_assert_eq!(container.entry_size(), entry_size); + if !is_memfd_supported() { + return None; + } Some(Self { - container, + container: SliceKeyContainer::new_with_capacity(capacity), _key: std::marker::PhantomData, }) } } +// Zeroize is done by the Drop impl of SliceKeyContainer impl ZeroizeOnDrop for LinuxMemfdSecretKeyStore {} -impl Drop for LinuxMemfdSecretKeyStore { - fn drop(&mut self) { - // Freeing the memory should clear all the secrets, but to ensure all the Drop impls - // are called correctly we clear the container first - self.container.clear(); - unsafe { - memsec::free_memfd_secret(self.container.inner_mut().as_ptr()); - } - } -} - impl KeyStore for LinuxMemfdSecretKeyStore { fn insert(&mut self, key_ref: Key, key: Key::KeyValue) { - if let Err(new_capacity) = self.container.ensure_capacity(1) { - // Create a new store with the correct capacity and replace self with it - let mut new_self = - Self::with_capacity(new_capacity).expect("Failed to allocate new memfd_secret"); - new_self.container.copy_from(&mut self.container); - *self = new_self; - }; - - let ok = self.container.insert(key_ref, key); - debug_assert!(ok, "insert failed"); + self.container.insert(key_ref, key); } - fn get(&self, key_ref: Key) -> Option<&Key::KeyValue> { self.container.get(key_ref) } diff --git a/crates/bitwarden-crypto/src/service/key_store/rust_impl.rs b/crates/bitwarden-crypto/src/service/key_store/rust_impl.rs index 1944a58cd..486f6c00f 100644 --- a/crates/bitwarden-crypto/src/service/key_store/rust_impl.rs +++ b/crates/bitwarden-crypto/src/service/key_store/rust_impl.rs @@ -1,76 +1,79 @@ use zeroize::ZeroizeOnDrop; -use super::{util::SliceKeyContainer, KeyRef, KeyStore}; +use super::{ + util::{KeyData, SliceKeyContainer}, + KeyRef, KeyStore, +}; +const ENABLE_MLOCK: bool = true; -// 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) struct RustKeyStore { +struct Mem { #[allow(clippy::type_complexity)] - container: SliceKeyContainer]>>, + data: Box<[Option<(Key, Key::KeyValue)>]>, } -const ENABLE_MLOCK: bool = true; - -impl RustKeyStore { - pub(crate) fn new() -> Self { - // This might not be exactly correct in all platforms, but it's a good enough approximation - const PAGE_SIZE: usize = 4096; - let entry_size = std::mem::size_of::>(); - - let entries_per_page = PAGE_SIZE / entry_size; - - // We're using mlock APIs to protect the memory, so allocating less than a page is a waste - let capacity = std::cmp::max(32, entries_per_page); - - Self::with_capacity(capacity) +impl Drop for Mem { + fn drop(&mut self) { + if ENABLE_MLOCK { + let entry_size = std::mem::size_of::>(); + unsafe { + memsec::munlock( + self.data.as_mut_ptr() as *mut u8, + self.data.len() * entry_size, + ); + } + } } +} - pub(crate) fn with_capacity(capacity: usize) -> Self { - let entry_size = std::mem::size_of::>(); - - // This is a bit awkward, but we need to fill the entire slice with None, and we can't just - // use vec![None; capacity] because that requires adding a Clone bound to the key - // value - let mut keys: Box<_> = std::iter::repeat_with(|| None).take(capacity).collect(); +impl KeyData for Mem { + fn new_with_capacity(capacity: usize) -> Self { + let mut data: Box<_> = std::iter::repeat_with(|| None).take(capacity).collect(); if ENABLE_MLOCK { + let entry_size = std::mem::size_of::>(); unsafe { - memsec::mlock(keys.as_mut_ptr() as *mut u8, capacity * entry_size); + memsec::mlock(data.as_mut_ptr() as *mut u8, capacity * entry_size); } } + Mem { data } + } - let container = SliceKeyContainer::new(keys); - - // Validate that the entry size is correct - debug_assert_eq!(container.entry_size(), entry_size); + fn get_key_data(&self) -> &[Option<(Key, Key::KeyValue)>] { + self.data.as_ref() + } - Self { container } + fn get_key_data_mut(&mut self) -> &mut [Option<(Key, Key::KeyValue)>] { + self.data.as_mut() } } -impl ZeroizeOnDrop for RustKeyStore {} +// 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) struct RustKeyStore { + #[allow(clippy::type_complexity)] + container: SliceKeyContainer>, +} -impl Drop for RustKeyStore { - fn drop(&mut self) { - if ENABLE_MLOCK { - // We need to ensure the values get dropped and zeroized _before_ - // the mlock gets removed, to avoid any last minute swaps to disk - self.container.clear(); +impl RustKeyStore { + pub(crate) fn new() -> Self { + Self::with_capacity(0) + } - unsafe { - memsec::munlock( - self.container.inner_mut().as_mut_ptr() as *mut u8, - self.container.byte_len(), - ); - } + pub(crate) fn with_capacity(capacity: usize) -> Self { + Self { + container: SliceKeyContainer::new_with_capacity(capacity), } } } +// Zeroize is done by the Drop impl of SliceKeyContainer +impl ZeroizeOnDrop for RustKeyStore {} + impl KeyStore for RustKeyStore { fn insert(&mut self, key_ref: Key, key: Key::KeyValue) { - if let Err(new_capacity) = self.container.ensure_capacity(1) { + /* + if let Err(new_capacity) = self.container.ensure_capacity(1) { // Create a new store with the correct capacity and replace self with it let mut new_self = Self::with_capacity(new_capacity); new_self.container.copy_from(&mut self.container); @@ -79,6 +82,10 @@ impl KeyStore for RustKeyStore { let ok = self.container.insert(key_ref, key); debug_assert!(ok, "insert failed"); + + */ + + self.container.insert(key_ref, key) } fn get(&self, key_ref: Key) -> Option<&Key::KeyValue> { diff --git a/crates/bitwarden-crypto/src/service/key_store/util.rs b/crates/bitwarden-crypto/src/service/key_store/util.rs index 2b527d14a..818591bd3 100644 --- a/crates/bitwarden-crypto/src/service/key_store/util.rs +++ b/crates/bitwarden-crypto/src/service/key_store/util.rs @@ -1,83 +1,59 @@ use crate::service::key_ref::KeyRef; -pub(crate) trait AsSlice { - fn as_slice(&self) -> &[Option<(Key, Key::KeyValue)>]; - fn as_mut_slice(&mut self) -> &mut [Option<(Key, Key::KeyValue)>]; -} - -impl AsSlice for Box<[Option<(Key, Key::KeyValue)>]> { - fn as_slice(&self) -> &[Option<(Key, Key::KeyValue)>] { - self.as_ref() - } - - fn as_mut_slice(&mut self) -> &mut [Option<(Key, Key::KeyValue)>] { - self.as_mut() - } -} - -#[cfg(target_os = "linux")] -pub(crate) struct MemPtr { - ptr: std::ptr::NonNull<[u8]>, - capacity: usize, -} - -#[cfg(target_os = "linux")] -impl MemPtr { - /// SAFETY: The caller must ensure that the pointer is valid, correctly aligned - pub unsafe fn new(ptr: std::ptr::NonNull<[u8]>, capacity: usize) -> MemPtr { - MemPtr { ptr, capacity } - } - - pub unsafe fn as_ptr(&self) -> std::ptr::NonNull<[u8]> { - self.ptr - } -} - -#[cfg(target_os = "linux")] -impl AsSlice for MemPtr { - fn as_slice(&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. - unsafe { std::slice::from_raw_parts(ptr, self.capacity) } - } - - fn as_mut_slice(&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. - unsafe { std::slice::from_raw_parts_mut(ptr, self.capacity) } - } +// This trait represents some data stored sequentially in memory, with a fixed size. +// We use this to abstract the implementation over Vec/Box<[u8]/NonNull<[u8]>. +#[allow(drop_bounds)] +pub(crate) trait KeyData: Drop { + fn new_with_capacity(capacity: usize) -> Self; + + fn get_key_data(&self) -> &[Option<(Key, Key::KeyValue)>]; + fn get_key_data_mut(&mut self) -> &mut [Option<(Key, Key::KeyValue)>]; } /// This represents a container 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 Vec, a Box<[u8]> or a NonNull. -pub(crate) struct SliceKeyContainer> { - data: Data, - +pub(crate) struct SliceKeyContainer> { // 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 + data: Option, + _key: std::marker::PhantomData, } -#[allow(dead_code)] -impl> SliceKeyContainer { - pub(crate) fn new(data: S) -> Self { - let capacity = data.as_slice().len(); +impl> Drop for SliceKeyContainer { + fn drop(&mut self) { + self.clear(); - debug_assert!( - capacity > 0, - "The container should have a capacity of at least 1" - ); + // Ensure the container gets dropped after the data is cleared + let _ = self.data.take(); + } +} + +#[allow(dead_code)] +impl> SliceKeyContainer { + pub(crate) fn new_with_capacity(capacity: usize) -> Self { + if capacity == 0 { + return Self { + length: 0, + capacity: 0, + data: None, + _key: std::marker::PhantomData, + }; + } let mut container = Self { - data, length: 0, capacity, + data: Some(Data::new_with_capacity(capacity)), _key: std::marker::PhantomData, }; @@ -87,44 +63,62 @@ impl> SliceKeyContainer { container } - pub(crate) const fn entry_size(&self) -> usize { - std::mem::size_of::>() - } - - pub(crate) unsafe fn inner_mut(&mut self) -> &mut S { - &mut self.data - } - - pub(crate) fn len(&self) -> usize { - self.length - } - - pub(crate) fn byte_len(&self) -> usize { - self.length * self.entry_size() - } - /// 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. - pub(crate) fn ensure_capacity(&self, new_elements: usize) -> Result<(), usize> { + fn check_capacity(&self, new_elements: usize) -> Result<(), usize> { let new_size = self.length + new_elements; - if new_size > self.capacity { + // This is the first allocation + 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 if new_size > self.capacity { // 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) + + // We still have enough capacity } else { Ok(()) } } + 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::new_with_capacity(new_capacity); + 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.data.as_ref().map(|d| d.get_key_data()).unwrap_or(&[]) + } + fn get_key_data_mut(&mut self) -> &mut [Option<(Key, Key::KeyValue)>] { + self.data + .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.data.as_slice()[..self.length]; + 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. @@ -142,59 +136,58 @@ impl> SliceKeyContainer { } pub(crate) fn clear(&mut self) { - self.data.as_mut_slice().fill_with(|| None); + self.get_key_data_mut().fill_with(|| None); self.length = 0; } pub(crate) fn remove(&mut self, key_ref: Key) { if let Ok(idx) = self.find_by_key_ref(&key_ref) { - let slice = self.data.as_mut_slice(); + let len = self.length; + let slice = self.get_key_data_mut(); slice[idx] = None; - slice[idx..self.length].rotate_left(1); + slice[idx..len].rotate_left(1); self.length -= 1; } } - pub(crate) fn insert(&mut self, key_ref: Key, key: ::KeyValue) -> bool { + pub(crate) fn insert(&mut self, key_ref: 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.data.as_mut_slice(); + let slice = self.get_key_data_mut(); slice[idx] = Some((key_ref, key)); } Err(idx) => { - // We need to insert the key, check if we have enough space - if self.length >= self.capacity { - return false; - } + // Make sure that we have enough capacity, and resize if needed + self.ensure_capacity(1); - let slice = self.data.as_mut_slice(); - if idx < self.length { + 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..=self.length].rotate_right(1); + slice[idx..=len].rotate_right(1); } slice[idx] = Some((key_ref, key)); self.length += 1; } } - - true } pub(crate) fn get(&self, key_ref: Key) -> Option<&::KeyValue> { self.find_by_key_ref(&key_ref) .ok() - .and_then(|idx| self.data.as_slice().get(idx)) + .and_then(|idx| self.get_key_data().get(idx)) .and_then(|f| f.as_ref().map(|f| &f.1)) } pub(crate) fn retain(&mut self, f: fn(Key) -> bool) { - let slice = self.data.as_mut_slice(); + let len = self.length; + let slice = self.get_key_data_mut(); let mut removed_elements = 0; - for value in slice.iter_mut().take(self.length) { + for value in slice.iter_mut().take(len) { let key = value .as_ref() .map(|e| e.0) @@ -213,9 +206,9 @@ impl> SliceKeyContainer { // Remove all the None values from the middle of the slice - for idx in 0..self.length { + for idx in 0..len { if slice[idx].is_none() { - slice[idx..self.length].rotate_left(1); + slice[idx..len].rotate_left(1); } } @@ -230,15 +223,17 @@ impl> SliceKeyContainer { // Empty the current container self.clear(); + let new_length = other.length; + // Move the data from the other container - let this = self.data.as_mut_slice(); - let that = other.data.as_mut_slice(); - for idx in 0..other.length { + 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 = other.length; + self.length = new_length; true } @@ -285,16 +280,39 @@ pub(crate) mod tests { } } + pub struct TestKeyContainer { + keys: Vec>, + } + impl Drop for TestKeyContainer { + fn drop(&mut self) {} + } + + impl KeyData for TestKeyContainer { + fn new_with_capacity(capacity: usize) -> Self { + Self { + keys: std::iter::repeat_with(|| None).take(capacity).collect(), + } + } + + fn get_key_data(&self) -> &[Option<(Key, Key::KeyValue)>] { + self.keys.as_slice() + } + + fn get_key_data_mut(&mut self) -> &mut [Option<(Key, Key::KeyValue)>] { + self.keys.as_mut_slice() + } + } + #[test] fn test_slice_container_insertion() { - let mut container = SliceKeyContainer::::new(vec![None; 5].into_boxed_slice()); + let mut container = SliceKeyContainer::>::new_with_capacity(5); - assert_eq!(container.data.as_slice(), [None, None, None, None, None]); + assert_eq!(container.get_key_data(), [None, None, None, None, None]); // Insert one key, which should be at the beginning - assert!(container.insert(TestKey::B(10), TestKeyValue::new(110))); + container.insert(TestKey::B(10), TestKeyValue::new(110)); assert_eq!( - container.data.as_slice(), + container.get_key_data(), [ Some((TestKey::B(10), TestKeyValue::new(110))), None, @@ -305,9 +323,9 @@ pub(crate) mod tests { ); // Insert a key that should be right after the first one - assert!(container.insert(TestKey::C, TestKeyValue::new(1000))); + container.insert(TestKey::C, TestKeyValue::new(1000)); assert_eq!( - container.data.as_slice(), + container.get_key_data(), [ Some((TestKey::B(10), TestKeyValue::new(110))), Some((TestKey::C, TestKeyValue::new(1000))), @@ -318,9 +336,9 @@ pub(crate) mod tests { ); // Insert a key in the middle - assert!(container.insert(TestKey::B(20), TestKeyValue::new(210))); + container.insert(TestKey::B(20), TestKeyValue::new(210)); assert_eq!( - container.data.as_slice(), + container.get_key_data(), [ Some((TestKey::B(10), TestKeyValue::new(110))), Some((TestKey::B(20), TestKeyValue::new(210))), @@ -331,9 +349,9 @@ pub(crate) mod tests { ); // Insert a key right at the start - assert!(container.insert(TestKey::A, TestKeyValue::new(0))); + container.insert(TestKey::A, TestKeyValue::new(0)); assert_eq!( - container.data.as_slice(), + container.get_key_data(), [ Some((TestKey::A, TestKeyValue::new(0))), Some((TestKey::B(10), TestKeyValue::new(110))), @@ -344,9 +362,9 @@ pub(crate) mod tests { ); // Insert a key in the middle, which fills the container - assert!(container.insert(TestKey::B(30), TestKeyValue::new(310))); + container.insert(TestKey::B(30), TestKeyValue::new(310)); assert_eq!( - container.data.as_slice(), + container.get_key_data(), [ Some((TestKey::A, TestKeyValue::new(0))), Some((TestKey::B(10), TestKeyValue::new(110))), @@ -357,9 +375,9 @@ pub(crate) mod tests { ); // Replacing an existing value at the start - assert!(container.insert(TestKey::A, TestKeyValue::new(1))); + container.insert(TestKey::A, TestKeyValue::new(1)); assert_eq!( - container.data.as_slice(), + container.get_key_data(), [ Some((TestKey::A, TestKeyValue::new(1))), Some((TestKey::B(10), TestKeyValue::new(110))), @@ -370,9 +388,9 @@ pub(crate) mod tests { ); // Replacing an existing value at the middle - assert!(container.insert(TestKey::B(20), TestKeyValue::new(211))); + container.insert(TestKey::B(20), TestKeyValue::new(211)); assert_eq!( - container.data.as_slice(), + container.get_key_data(), [ Some((TestKey::A, TestKeyValue::new(1))), Some((TestKey::B(10), TestKeyValue::new(110))), @@ -383,9 +401,9 @@ pub(crate) mod tests { ); // Replacing an existing value at the end - assert!(container.insert(TestKey::C, TestKeyValue::new(1001))); + container.insert(TestKey::C, TestKeyValue::new(1001)); assert_eq!( - container.data.as_slice(), + container.get_key_data(), [ Some((TestKey::A, TestKeyValue::new(1))), Some((TestKey::B(10), TestKeyValue::new(110))), @@ -398,14 +416,14 @@ pub(crate) mod tests { #[test] fn test_slice_container_get() { - let mut container = SliceKeyContainer::::new(vec![None; 5].into_boxed_slice()); + let mut container = SliceKeyContainer::>::new_with_capacity(5); for (key, value) in [ (TestKey::A, TestKeyValue::new(1)), (TestKey::B(10), TestKeyValue::new(110)), (TestKey::C, TestKeyValue::new(1000)), ] { - assert!(container.insert(key, value)); + container.insert(key, value); } assert_eq!(container.get(TestKey::A), Some(&TestKeyValue::new(1))); @@ -416,7 +434,7 @@ pub(crate) mod tests { #[test] fn test_slice_container_clear() { - let mut container = SliceKeyContainer::::new(vec![None; 5].into_boxed_slice()); + let mut container = SliceKeyContainer::>::new_with_capacity(5); for (key, value) in [ (TestKey::A, TestKeyValue::new(1)), @@ -425,11 +443,11 @@ pub(crate) mod tests { (TestKey::B(30), TestKeyValue::new(310)), (TestKey::C, TestKeyValue::new(1000)), ] { - assert!(container.insert(key, value)); + container.insert(key, value); } assert_eq!( - container.data.as_slice(), + container.get_key_data(), [ Some((TestKey::A, TestKeyValue::new(1))), Some((TestKey::B(10), TestKeyValue::new(110))), @@ -441,20 +459,21 @@ pub(crate) mod tests { container.clear(); - assert_eq!(container.data.as_slice(), [None, None, None, None, None]); + assert_eq!(container.get_key_data(), [None, None, None, None, None]); } #[test] fn test_slice_container_ensure_capacity() { - let mut container = SliceKeyContainer::::new(vec![None; 5].into_boxed_slice()); + let mut container = SliceKeyContainer::>::new_with_capacity(5); + assert_eq!(container.capacity, 5); assert_eq!(container.length, 0); - assert_eq!(container.ensure_capacity(0), Ok(())); - assert_eq!(container.ensure_capacity(6), Err(10)); - assert_eq!(container.ensure_capacity(10), Err(10)); - assert_eq!(container.ensure_capacity(11), Err(15)); - assert_eq!(container.ensure_capacity(51), Err(55)); + 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)), @@ -463,19 +482,19 @@ pub(crate) mod tests { (TestKey::B(30), TestKeyValue::new(310)), (TestKey::C, TestKeyValue::new(1000)), ] { - assert!(container.insert(key, value)); + container.insert(key, value); } - assert_eq!(container.ensure_capacity(0), Ok(())); - assert_eq!(container.ensure_capacity(6), Err(15)); - assert_eq!(container.ensure_capacity(10), Err(15)); - assert_eq!(container.ensure_capacity(11), Err(20)); - assert_eq!(container.ensure_capacity(51), Err(60)); + 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 = SliceKeyContainer::::new(vec![None; 5].into_boxed_slice()); + let mut container = SliceKeyContainer::>::new_with_capacity(5); for (key, value) in [ (TestKey::A, TestKeyValue::new(1)), @@ -484,13 +503,13 @@ pub(crate) mod tests { (TestKey::B(30), TestKeyValue::new(310)), (TestKey::C, TestKeyValue::new(1000)), ] { - assert!(container.insert(key, value)); + container.insert(key, value); } // Remove the last element container.remove(TestKey::C); assert_eq!( - container.data.as_slice(), + container.get_key_data(), [ Some((TestKey::A, TestKeyValue::new(1))), Some((TestKey::B(10), TestKeyValue::new(110))), @@ -503,7 +522,7 @@ pub(crate) mod tests { // Remove the first element container.remove(TestKey::A); assert_eq!( - container.data.as_slice(), + container.get_key_data(), [ Some((TestKey::B(10), TestKeyValue::new(110))), Some((TestKey::B(20), TestKeyValue::new(210))), @@ -516,7 +535,7 @@ pub(crate) mod tests { // Remove a non-existing element container.remove(TestKey::A); assert_eq!( - container.data.as_slice(), + container.get_key_data(), [ Some((TestKey::B(10), TestKeyValue::new(110))), Some((TestKey::B(20), TestKeyValue::new(210))), @@ -529,7 +548,7 @@ pub(crate) mod tests { // Remove an element in the middle container.remove(TestKey::B(20)); assert_eq!( - container.data.as_slice(), + container.get_key_data(), [ Some((TestKey::B(10), TestKeyValue::new(110))), Some((TestKey::B(30), TestKeyValue::new(310))), @@ -542,7 +561,7 @@ pub(crate) mod tests { // Remove all the remaining elements container.remove(TestKey::B(30)); assert_eq!( - container.data.as_slice(), + container.get_key_data(), [ Some((TestKey::B(10), TestKeyValue::new(110))), None, @@ -552,16 +571,16 @@ pub(crate) mod tests { ] ); container.remove(TestKey::B(10)); - assert_eq!(container.data.as_slice(), [None, None, None, None, None]); + assert_eq!(container.get_key_data(), [None, None, None, None, None]); // Remove from an empty container container.remove(TestKey::B(10)); - assert_eq!(container.data.as_slice(), [None, None, None, None, None]); + assert_eq!(container.get_key_data(), [None, None, None, None, None]); } #[test] fn test_slice_container_retain_removes_one() { - let mut container = SliceKeyContainer::::new(vec![None; 5].into_boxed_slice()); + let mut container = SliceKeyContainer::>::new_with_capacity(5); for (key, value) in [ (TestKey::A, TestKeyValue::new(1)), @@ -570,13 +589,13 @@ pub(crate) mod tests { (TestKey::B(30), TestKeyValue::new(310)), (TestKey::C, TestKeyValue::new(1000)), ] { - assert!(container.insert(key, value)); + container.insert(key, value); } // Remove the last element container.retain(|k| k != TestKey::C); assert_eq!( - container.data.as_slice(), + container.get_key_data(), [ Some((TestKey::A, TestKeyValue::new(1))), Some((TestKey::B(10), TestKeyValue::new(110))), @@ -589,7 +608,7 @@ pub(crate) mod tests { // Remove the first element container.retain(|k| k != TestKey::A); assert_eq!( - container.data.as_slice(), + container.get_key_data(), [ Some((TestKey::B(10), TestKeyValue::new(110))), Some((TestKey::B(20), TestKeyValue::new(210))), @@ -602,7 +621,7 @@ pub(crate) mod tests { // Remove a non-existing element container.retain(|k| k != TestKey::A); assert_eq!( - container.data.as_slice(), + container.get_key_data(), [ Some((TestKey::B(10), TestKeyValue::new(110))), Some((TestKey::B(20), TestKeyValue::new(210))), @@ -615,7 +634,7 @@ pub(crate) mod tests { // Remove an element in the middle container.retain(|k| k != TestKey::B(20)); assert_eq!( - container.data.as_slice(), + container.get_key_data(), [ Some((TestKey::B(10), TestKeyValue::new(110))), Some((TestKey::B(30), TestKeyValue::new(310))), @@ -628,7 +647,7 @@ pub(crate) mod tests { // Remove all the remaining elements container.retain(|k| k != TestKey::B(30)); assert_eq!( - container.data.as_slice(), + container.get_key_data(), [ Some((TestKey::B(10), TestKeyValue::new(110))), None, @@ -638,16 +657,16 @@ pub(crate) mod tests { ] ); container.retain(|k| k != TestKey::B(10)); - assert_eq!(container.data.as_slice(), [None, None, None, None, None]); + 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.data.as_slice(), [None, None, None, None, None]); + assert_eq!(container.get_key_data(), [None, None, None, None, None]); } #[test] fn test_slice_container_retain_removes_none() { - let mut container = SliceKeyContainer::::new(vec![None; 5].into_boxed_slice()); + let mut container = SliceKeyContainer::>::new_with_capacity(5); for (key, value) in [ (TestKey::A, TestKeyValue::new(1)), @@ -656,12 +675,12 @@ pub(crate) mod tests { (TestKey::B(30), TestKeyValue::new(310)), (TestKey::C, TestKeyValue::new(1000)), ] { - assert!(container.insert(key, value)); + container.insert(key, value); } container.retain(|_k| true); assert_eq!( - container.data.as_slice(), + container.get_key_data(), [ Some((TestKey::A, TestKeyValue::new(1))), Some((TestKey::B(10), TestKeyValue::new(110))), @@ -674,7 +693,7 @@ pub(crate) mod tests { #[test] fn test_slice_container_retain_removes_some() { - let mut container = SliceKeyContainer::::new(vec![None; 5].into_boxed_slice()); + let mut container = SliceKeyContainer::>::new_with_capacity(5); for (key, value) in [ (TestKey::A, TestKeyValue::new(1)), @@ -683,12 +702,12 @@ pub(crate) mod tests { (TestKey::B(30), TestKeyValue::new(310)), (TestKey::C, TestKeyValue::new(1000)), ] { - assert!(container.insert(key, value)); + container.insert(key, value); } container.retain(|k| matches!(k, TestKey::A | TestKey::B(20) | TestKey::C)); assert_eq!( - container.data.as_slice(), + container.get_key_data(), [ Some((TestKey::A, TestKeyValue::new(1))), Some((TestKey::B(20), TestKeyValue::new(210))), @@ -701,7 +720,7 @@ pub(crate) mod tests { #[test] fn test_slice_container_retain_removes_all() { - let mut container = SliceKeyContainer::::new(vec![None; 5].into_boxed_slice()); + let mut container = SliceKeyContainer::>::new_with_capacity(5); for (key, value) in [ (TestKey::A, TestKeyValue::new(1)), @@ -710,10 +729,10 @@ pub(crate) mod tests { (TestKey::B(30), TestKeyValue::new(310)), (TestKey::C, TestKeyValue::new(1000)), ] { - assert!(container.insert(key, value)); + container.insert(key, value); } container.retain(|_k| false); - assert_eq!(container.data.as_slice(), [None, None, None, None, None]); + assert_eq!(container.get_key_data(), [None, None, None, None, None]); } } From 4b846ed3a1fa8b47f795510c8d85909e4bfc14a7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garci=CC=81a?= Date: Fri, 23 Aug 2024 19:11:21 +0200 Subject: [PATCH 04/34] Fix --- crates/bitwarden-crypto/src/service/mod.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/crates/bitwarden-crypto/src/service/mod.rs b/crates/bitwarden-crypto/src/service/mod.rs index 1a003be5c..a541823be 100644 --- a/crates/bitwarden-crypto/src/service/mod.rs +++ b/crates/bitwarden-crypto/src/service/mod.rs @@ -78,6 +78,8 @@ impl<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> encrypted_key: &EncString, ) -> Result { self.engine - .decrypt_and_store_symmetric_key(encryption_key, new_key_ref, encrypted_key) + .decrypt_and_store_symmetric_key(encryption_key, new_key_ref, encrypted_key)?; + // This is returned for convenience + Ok(new_key_ref) } } From 7a01168f6936010ae6dc00983137f8f6feba30fc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garci=CC=81a?= Date: Fri, 23 Aug 2024 23:11:57 +0200 Subject: [PATCH 05/34] Paralelization plus alternative to locatekey --- crates/bitwarden-core/src/lib.rs | 43 ++++++++++-- .../src/service/crypto_engine/mod.rs | 4 +- .../src/service/encryptable.rs | 69 +++++++++++++++++-- .../bitwarden-crypto/src/service/key_ref.rs | 4 +- .../key_store/linux_memfd_secret_impl.rs | 8 ++- .../src/service/key_store/mod.rs | 2 +- crates/bitwarden-crypto/src/service/mod.rs | 59 ++++++++++++++-- 7 files changed, 169 insertions(+), 20 deletions(-) diff --git a/crates/bitwarden-core/src/lib.rs b/crates/bitwarden-core/src/lib.rs index 5f900a468..9126eef95 100644 --- a/crates/bitwarden-core/src/lib.rs +++ b/crates/bitwarden-core/src/lib.rs @@ -62,6 +62,17 @@ mod testcrypto { name: String, } + impl UsesKey for Cipher { + fn uses_key(&self) -> MySymmKeyRef { + MySymmKeyRef::User + } + } + impl UsesKey for CipherView { + fn uses_key(&self) -> MySymmKeyRef { + MySymmKeyRef::User + } + } + const CIPHER_KEY: MySymmKeyRef = MySymmKeyRef::Local("cipher_key"); impl Encryptable for CipherView { @@ -79,7 +90,7 @@ mod testcrypto { Ok(Cipher { key: self.key.clone(), - name: self.name.encrypt(ctx, cipher_key)?, + name: self.name.as_str().encrypt(ctx, cipher_key)?, }) } } @@ -126,12 +137,34 @@ mod testcrypto { service.insert_symmetric_key(MySymmKeyRef::Organization(org_id), org_key); } - let cipher_enc2 = service - .encrypt(MySymmKeyRef::User, cipher_view.clone()) - .unwrap(); + let cipher_enc2 = service.encrypt(cipher_view.clone()).unwrap(); - let cipher_view2 = service.decrypt(MySymmKeyRef::User, &cipher_enc2).unwrap(); + let cipher_view2 = service.decrypt(&cipher_enc2).unwrap(); assert_eq!(cipher_view.name, cipher_view2.name); + + // We can also decrypt a value by tagging it with the key + let text = String::from("test!"); + + let text_enc = service + .encrypt(text.as_str().using_key(MySymmKeyRef::User)) + .unwrap(); + + // And decrypt values in parallel + let mut data = Vec::with_capacity(10_000_000); + for _ in 0..data.capacity() { + data.push("hello world, this is an encryption test!".using_key(MySymmKeyRef::User)); + } + let now = std::time::Instant::now(); + let _ = service.encrypt_list(&data).unwrap(); + println!("Batch encrypting took {:?}", now.elapsed()); + + let now = std::time::Instant::now(); + for d in data { + let _ = service.encrypt(d).unwrap(); + } + println!("Individual encrypting took {:?}", now.elapsed()); + + panic!("DONE") } } diff --git a/crates/bitwarden-crypto/src/service/crypto_engine/mod.rs b/crates/bitwarden-crypto/src/service/crypto_engine/mod.rs index 03a9f0d91..7291cd0d2 100644 --- a/crates/bitwarden-crypto/src/service/crypto_engine/mod.rs +++ b/crates/bitwarden-crypto/src/service/crypto_engine/mod.rs @@ -14,7 +14,9 @@ pub(crate) use rust_impl::RustCryptoEngine; // For the cases where a secure element capable of doing cryptographic operations is not available, // but there is a secure way to store keys, `KeyStore` can be implemented and then used in // conjunction with `RustCryptoEngine`. -pub(crate) trait CryptoEngine { +pub(crate) trait CryptoEngine: + Send + Sync +{ /// Create a new context for this service. This allows the user to perform cryptographic // operations with keys that are only relevant to the current context. /// diff --git a/crates/bitwarden-crypto/src/service/encryptable.rs b/crates/bitwarden-crypto/src/service/encryptable.rs index 98fc24500..8bd36d0d4 100644 --- a/crates/bitwarden-crypto/src/service/encryptable.rs +++ b/crates/bitwarden-crypto/src/service/encryptable.rs @@ -2,6 +2,67 @@ use super::{ key_ref::{AsymmetricKeyRef, KeyRef, SymmetricKeyRef}, CryptoServiceContext, }; +use crate::{AsymmetricEncString, CryptoError, EncString}; + +/////////////////////// + +// Just like LocateKey but this time we're not locating anything, just returning a ref + +pub trait UsesKey { + fn uses_key(&self) -> Key; +} + +// This extension trait allows any type to be wrapped with `KeyProvided` +// to make it easy to encrypt/decrypt it with the desired key +pub trait KeyProvidedExt: Sized { + fn using_key(self, key: Key) -> KeyProvided { + KeyProvided { key, value: self } + } +} +impl KeyProvidedExt for T {} +pub struct KeyProvided { + key: Key, + value: T, +} +impl UsesKey for KeyProvided { + fn uses_key(&self) -> Key { + self.key + } +} +impl< + SymmKeyRef: SymmetricKeyRef, + AsymmKeyRef: AsymmetricKeyRef, + Key: KeyRef, + T: Encryptable, + Output, + > Encryptable for KeyProvided +{ + fn encrypt( + &self, + ctx: &mut CryptoServiceContext, + _key: Key, + ) -> Result { + self.value.encrypt(ctx, self.key) + } +} +impl< + SymmKeyRef: SymmetricKeyRef, + AsymmKeyRef: AsymmetricKeyRef, + Key: KeyRef, + T: Decryptable, + Output, + > Decryptable for KeyProvided +{ + fn decrypt( + &self, + ctx: &mut CryptoServiceContext, + _key: Key, + ) -> Result { + self.value.decrypt(ctx, self.key) + } +} + +///////////////////// pub trait Encryptable< SymmKeyRef: SymmetricKeyRef, @@ -56,7 +117,7 @@ impl } impl - Encryptable for [u8] + Encryptable for &[u8] { fn encrypt( &self, @@ -68,7 +129,7 @@ impl } impl - Encryptable for [u8] + Encryptable for &[u8] { fn encrypt( &self, @@ -106,7 +167,7 @@ impl } impl - Encryptable for str + Encryptable for &str { fn encrypt( &self, @@ -118,7 +179,7 @@ impl } impl - Encryptable for str + Encryptable for &str { fn encrypt( &self, diff --git a/crates/bitwarden-crypto/src/service/key_ref.rs b/crates/bitwarden-crypto/src/service/key_ref.rs index f70f25fad..2b08f40a0 100644 --- a/crates/bitwarden-crypto/src/service/key_ref.rs +++ b/crates/bitwarden-crypto/src/service/key_ref.rs @@ -9,9 +9,9 @@ use crate::{AsymmetricCryptoKey, CryptoKey, SymmetricCryptoKey}; /// an end user would not implement this trait directly, and would instead use the `SymmetricKeyRef` /// and `AsymmetricKeyRef` traits. pub trait KeyRef: - Debug + Clone + Copy + Hash + Eq + PartialEq + Ord + PartialOrd + 'static + Debug + Clone + Copy + Hash + Eq + PartialEq + Ord + PartialOrd + Send + Sync + 'static { - type KeyValue: Debug + CryptoKey + ZeroizeOnDrop; + type KeyValue: Debug + CryptoKey + Send + Sync + ZeroizeOnDrop; /// Returns whether the key is local to the current context or shared globally by the service. fn is_local(&self) -> bool; diff --git a/crates/bitwarden-crypto/src/service/key_store/linux_memfd_secret_impl.rs b/crates/bitwarden-crypto/src/service/key_store/linux_memfd_secret_impl.rs index c8fbfb3a7..2192cc681 100644 --- a/crates/bitwarden-crypto/src/service/key_store/linux_memfd_secret_impl.rs +++ b/crates/bitwarden-crypto/src/service/key_store/linux_memfd_secret_impl.rs @@ -7,7 +7,7 @@ use super::{ KeyRef, KeyStore, }; -pub(crate) fn is_memfd_supported() -> bool { +fn is_memfd_supported() -> bool { static IS_SUPPORTED: OnceLock = OnceLock::new(); *IS_SUPPORTED.get_or_init(|| unsafe { @@ -19,11 +19,15 @@ pub(crate) fn is_memfd_supported() -> bool { }) } -pub(crate) struct MemPtr { +struct MemPtr { ptr: std::ptr::NonNull<[u8]>, capacity: usize, } +// TODO: Is this safe? +unsafe impl Send for MemPtr {} +unsafe impl Sync for MemPtr {} + impl Drop for MemPtr { fn drop(&mut self) { unsafe { diff --git a/crates/bitwarden-crypto/src/service/key_store/mod.rs b/crates/bitwarden-crypto/src/service/key_store/mod.rs index 9a4d75f46..2b4bd0476 100644 --- a/crates/bitwarden-crypto/src/service/key_store/mod.rs +++ b/crates/bitwarden-crypto/src/service/key_store/mod.rs @@ -15,7 +15,7 @@ pub(crate) use rust_impl::RustKeyStore; /// implementation is a simple in-memory store without any security guarantees. Other /// implementations could use secure enclaves or HSMs, or OS provided keychains. #[allow(dead_code)] -pub(crate) trait KeyStore: ZeroizeOnDrop { +pub(crate) trait KeyStore: ZeroizeOnDrop + Send + Sync { fn insert(&mut self, key_ref: Key, key: Key::KeyValue); fn get(&self, key_ref: Key) -> Option<&Key::KeyValue>; fn remove(&mut self, key_ref: Key); diff --git a/crates/bitwarden-crypto/src/service/mod.rs b/crates/bitwarden-crypto/src/service/mod.rs index a541823be..dcf8ef8cf 100644 --- a/crates/bitwarden-crypto/src/service/mod.rs +++ b/crates/bitwarden-crypto/src/service/mod.rs @@ -8,7 +8,7 @@ pub mod key_ref; mod key_store; use crypto_engine::{CryptoEngine, CryptoEngineContext, RustCryptoEngine}; -pub use encryptable::{Decryptable, Encryptable}; +pub use encryptable::{Decryptable, Encryptable, KeyProvided, KeyProvidedExt, UsesKey}; use key_ref::{AsymmetricKeyRef, KeyRef, SymmetricKeyRef}; #[derive(Clone)] @@ -46,21 +46,66 @@ impl } // These are just convenience methods to avoid having to call `context` every time - pub fn decrypt, Output>( + pub fn decrypt< + Key: KeyRef, + Data: Decryptable + UsesKey, + Output, + >( &self, - key: Key, data: &Data, ) -> Result { + let key = data.uses_key(); data.decrypt(&mut self.context(), key) } - pub fn encrypt, Output>( + pub fn encrypt< + Key: KeyRef, + Data: Encryptable + UsesKey, + Output, + >( &self, - key: Key, data: Data, ) -> Result { + let key = data.uses_key(); data.encrypt(&mut self.context(), key) } + + pub fn encrypt_list< + Key: KeyRef, + Data: Encryptable + UsesKey + Send + Sync, + Output: Send + Sync, + >( + &self, + data: &[Data], + ) -> Result, crate::CryptoError> { + use rayon::prelude::*; + + // We want to split all the data between available threads, but at the + // same time we don't want to split it too much if the amount of data is small. + + // In this case, the minimum chunk size is 50. + let chunk_size = usize::max(1 + data.len() / rayon::current_num_threads(), 50); + + let res: Result, _> = data + .par_chunks(chunk_size) + .map(|chunk| { + let mut context = self.context(); + + let mut result = Vec::with_capacity(chunk.len()); + + for item in chunk { + let key = item.uses_key(); + result.push(item.encrypt(&mut context, key)); + context.clear(); + } + + result + }) + .flatten() + .collect(); + + res + } } pub struct CryptoServiceContext<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> { @@ -82,4 +127,8 @@ impl<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> // This is returned for convenience Ok(new_key_ref) } + + pub fn clear(&mut self) { + self.engine.clear(); + } } From c649bf060c7ecfc2203fb7b55be81dc84d0a041e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garci=CC=81a?= Date: Fri, 23 Aug 2024 23:17:49 +0200 Subject: [PATCH 06/34] WASM support --- crates/bitwarden-crypto/Cargo.toml | 4 +++- crates/bitwarden-crypto/src/service/key_store/rust_impl.rs | 2 ++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/crates/bitwarden-crypto/Cargo.toml b/crates/bitwarden-crypto/Cargo.toml index d530e18bf..4109b659e 100644 --- a/crates/bitwarden-crypto/Cargo.toml +++ b/crates/bitwarden-crypto/Cargo.toml @@ -30,7 +30,6 @@ cbc = { version = ">=0.1.2, <0.2", features = ["alloc", "zeroize"] } generic-array = { version = ">=0.14.7, <1.0", features = ["zeroize"] } hkdf = ">=0.12.3, <0.13" hmac = ">=0.12.1, <0.13" -memsec = { version = "0.7.0", features = ["alloc_ext"] } num-bigint = ">=0.4, <0.5" num-traits = ">=0.2.15, <0.3" pbkdf2 = { version = ">=0.12.1, <0.13", default-features = false } @@ -47,6 +46,9 @@ uniffi = { version = "=0.28.1", optional = true } uuid = { version = ">=1.3.3, <2.0", features = ["serde"] } zeroize = { version = ">=1.7.0, <2.0", features = ["derive", "aarch64"] } +[target.'cfg(not(target_arch="wasm32"))'.dependencies] +memsec = { version = "0.7.0", features = ["alloc_ext"] } + [dev-dependencies] criterion = "0.5.1" rand_chacha = "0.3.1" diff --git a/crates/bitwarden-crypto/src/service/key_store/rust_impl.rs b/crates/bitwarden-crypto/src/service/key_store/rust_impl.rs index 486f6c00f..877e876d1 100644 --- a/crates/bitwarden-crypto/src/service/key_store/rust_impl.rs +++ b/crates/bitwarden-crypto/src/service/key_store/rust_impl.rs @@ -13,6 +13,7 @@ struct Mem { impl Drop for Mem { fn drop(&mut self) { + #[cfg(not(target_arch = "wasm32"))] if ENABLE_MLOCK { let entry_size = std::mem::size_of::>(); unsafe { @@ -29,6 +30,7 @@ impl KeyData for Mem { fn new_with_capacity(capacity: usize) -> Self { let mut data: Box<_> = std::iter::repeat_with(|| None).take(capacity).collect(); + #[cfg(not(target_arch = "wasm32"))] if ENABLE_MLOCK { let entry_size = std::mem::size_of::>(); unsafe { From e2129ad5997fe778830586da520b05f88a5b70b0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garci=CC=81a?= Date: Mon, 23 Sep 2024 16:05:13 +0200 Subject: [PATCH 07/34] Remove unnecessary trait --- .../{rust_impl.rs => context.rs} | 103 ++---------- .../src/service/crypto_engine/mod.rs | 146 ++++++------------ .../src/service/key_store/mod.rs | 9 ++ crates/bitwarden-crypto/src/service/mod.rs | 6 +- 4 files changed, 78 insertions(+), 186 deletions(-) rename crates/bitwarden-crypto/src/service/crypto_engine/{rust_impl.rs => context.rs} (65%) diff --git a/crates/bitwarden-crypto/src/service/crypto_engine/rust_impl.rs b/crates/bitwarden-crypto/src/service/crypto_engine/context.rs similarity index 65% rename from crates/bitwarden-crypto/src/service/crypto_engine/rust_impl.rs rename to crates/bitwarden-crypto/src/service/crypto_engine/context.rs index d5f506cca..8b5e0e99c 100644 --- a/crates/bitwarden-crypto/src/service/crypto_engine/rust_impl.rs +++ b/crates/bitwarden-crypto/src/service/crypto_engine/context.rs @@ -1,75 +1,11 @@ -use std::sync::RwLock; - use rsa::Oaep; use crate::{ - service::{ - crypto_engine::{CryptoEngine, CryptoEngineContext}, - key_ref::KeyRef, - key_store::KeyStore, - AsymmetricKeyRef, SymmetricKeyRef, - }, + service::{key_store::KeyStore, AsymmetricKeyRef, SymmetricKeyRef}, AsymmetricCryptoKey, AsymmetricEncString, CryptoError, EncString, SymmetricCryptoKey, }; -fn create_key_store() -> Box> { - #[cfg(target_os = "linux")] - if let Some(key_store) = crate::service::key_store::LinuxMemfdSecretKeyStore::::new() { - return Box::new(key_store); - } - - Box::new(crate::service::key_store::RustKeyStore::new()) -} - -pub(crate) struct RustCryptoEngine { - key_stores: RwLock>, -} - -// This is just a wrapper around the keys so we only deal with one RwLock -struct RustCryptoEngineKeys { - symmetric_keys: Box>, - asymmetric_keys: Box>, -} - -impl - RustCryptoEngine -{ - pub(crate) fn new() -> Self { - Self { - key_stores: RwLock::new(RustCryptoEngineKeys { - symmetric_keys: create_key_store(), - asymmetric_keys: create_key_store(), - }), - } - } -} - -impl - CryptoEngine for RustCryptoEngine -{ - fn context(&'_ self) -> Box + '_> { - // TODO: Cache these?, or maybe initialize them lazily? or both? - Box::new(RustCryptoEngineContext { - global_keys: self.key_stores.read().expect("RwLock is poisoned"), - local_symmetric_keys: create_key_store(), - local_asymmetric_keys: create_key_store(), - }) - } - - fn insert_symmetric_key(&self, key_ref: SymmKeyRef, key: SymmetricCryptoKey) { - self.key_stores - .write() - .expect("RwLock is poisoned") - .symmetric_keys - .insert(key_ref, key); - } - - fn clear(&self) { - let mut keys = self.key_stores.write().expect("RwLock is poisoned"); - keys.symmetric_keys.clear(); - keys.asymmetric_keys.clear(); - } -} +use super::RustCryptoEngineKeys; pub(crate) struct RustCryptoEngineContext< 'a, @@ -78,10 +14,11 @@ pub(crate) struct RustCryptoEngineContext< > { // We hold a RwLock read guard to avoid having any nested //calls locking it again and potentially causing a deadlock - global_keys: std::sync::RwLockReadGuard<'a, RustCryptoEngineKeys>, + pub(super) global_keys: + std::sync::RwLockReadGuard<'a, RustCryptoEngineKeys>, - local_symmetric_keys: Box>, - local_asymmetric_keys: Box>, + pub(super) local_symmetric_keys: Box>, + pub(super) local_asymmetric_keys: Box>, } impl<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> @@ -110,22 +47,17 @@ impl<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> } .ok_or_else(|| crate::CryptoError::MissingKey2(format!("{key_ref:?}"))) } -} -impl<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> - CryptoEngineContext<'a, SymmKeyRef, AsymmKeyRef> - for RustCryptoEngineContext<'a, SymmKeyRef, AsymmKeyRef> -{ - fn clear(&mut self) { + pub fn clear(&mut self) { self.local_symmetric_keys.clear(); self.local_asymmetric_keys.clear(); } - fn remove_symmetric_key(&mut self, key_ref: SymmKeyRef) { + pub fn remove_symmetric_key(&mut self, key_ref: SymmKeyRef) { self.local_symmetric_keys.remove(key_ref); } - fn decrypt_data_with_symmetric_key( + pub fn decrypt_data_with_symmetric_key( &self, key: SymmKeyRef, data: &EncString, @@ -156,7 +88,7 @@ impl<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> } } - fn decrypt_and_store_symmetric_key( + pub fn decrypt_and_store_symmetric_key( &mut self, encryption_key: SymmKeyRef, new_key_ref: SymmKeyRef, @@ -170,7 +102,7 @@ impl<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> Ok(()) } - fn encrypt_data_with_symmetric_key( + pub fn encrypt_data_with_symmetric_key( &self, key: SymmKeyRef, data: &[u8], @@ -183,7 +115,7 @@ impl<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> ) } - fn encrypt_symmetric_key( + pub fn encrypt_symmetric_key( &self, encryption_key: SymmKeyRef, key_to_encrypt: SymmKeyRef, @@ -192,11 +124,11 @@ impl<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> self.encrypt_data_with_symmetric_key(encryption_key, &key_to_encrypt.to_vec()) } - fn remove_asymmetric_key(&mut self, key_ref: AsymmKeyRef) { + pub fn remove_asymmetric_key(&mut self, key_ref: AsymmKeyRef) { self.local_asymmetric_keys.remove(key_ref); } - fn decrypt_data_with_asymmetric_key( + pub fn decrypt_data_with_asymmetric_key( &self, key: AsymmKeyRef, data: &AsymmetricEncString, @@ -219,7 +151,7 @@ impl<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> .map_err(|_| CryptoError::KeyDecrypt) } - fn decrypt_and_store_asymmetric_key( + pub fn decrypt_and_store_asymmetric_key( &mut self, encryption_key: AsymmKeyRef, new_key_ref: AsymmKeyRef, @@ -232,8 +164,7 @@ impl<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> self.local_asymmetric_keys.insert(new_key_ref, new_key); Ok(()) } - - fn encrypt_data_with_asymmetric_key( + pub fn encrypt_data_with_asymmetric_key( &self, key: AsymmKeyRef, data: &[u8], @@ -242,7 +173,7 @@ impl<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> AsymmetricEncString::encrypt_rsa2048_oaep_sha1(data, key) } - fn encrypt_asymmetric_key( + pub fn encrypt_asymmetric_key( &self, encryption_key: AsymmKeyRef, key_to_encrypt: AsymmKeyRef, diff --git a/crates/bitwarden-crypto/src/service/crypto_engine/mod.rs b/crates/bitwarden-crypto/src/service/crypto_engine/mod.rs index 7291cd0d2..1d75f994c 100644 --- a/crates/bitwarden-crypto/src/service/crypto_engine/mod.rs +++ b/crates/bitwarden-crypto/src/service/crypto_engine/mod.rs @@ -1,108 +1,60 @@ +use std::sync::RwLock; + +use super::key_store::create_key_store; use crate::{ - service::{AsymmetricKeyRef, SymmetricKeyRef}, - AsymmetricEncString, EncString, SymmetricCryptoKey, + service::{key_store::KeyStore, AsymmetricKeyRef, SymmetricKeyRef}, + SymmetricCryptoKey, }; -mod rust_impl; - -pub(crate) use rust_impl::RustCryptoEngine; +mod context; +pub(crate) use context::RustCryptoEngineContext; -// This trait represents a service that can store cryptographic keys and perform operations with -// them, ideally within a Secure Enclave or HSM. Users of this trait will not handle the keys -// directly but will use references to them. -// -// For the cases where a secure element capable of doing cryptographic operations is not available, -// but there is a secure way to store keys, `KeyStore` can be implemented and then used in -// conjunction with `RustCryptoEngine`. -pub(crate) trait CryptoEngine: - Send + Sync -{ - /// Create a new context for this service. This allows the user to perform cryptographic - // operations with keys that are only relevant to the current context. - /// - /// NOTE: This is an advanced API, and should be used with care. Particularly, it's important - /// to ensure the context is dropped when it's no longer needed, to avoid holding a reference to - /// the RwLock for too long. It's also important to ensure that the context is cleared of - /// keys after every use if it's being reused, to avoid it growing indefinitely. - fn context(&'_ self) -> Box + '_>; - - #[deprecated(note = "We should be generating/decrypting the keys into the service directly")] - fn insert_symmetric_key(&self, key_ref: SymmKeyRef, key: SymmetricCryptoKey); +pub(crate) struct RustCryptoEngine { + key_stores: RwLock>, +} - fn clear(&self); +// This is just a wrapper around the keys so we only deal with one RwLock +struct RustCryptoEngineKeys { + symmetric_keys: Box>, + asymmetric_keys: Box>, } -// This trait represents a context for a `CryptoEngine`. It allows the user to perform cryptographic -// operations with keys that are only relevant to the current context. -#[allow(dead_code)] -pub(crate) trait CryptoEngineContext<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> +impl + RustCryptoEngine { - fn clear(&mut self); - - // Symmetric key operations - - fn remove_symmetric_key(&mut self, key_ref: SymmKeyRef); - - fn decrypt_data_with_symmetric_key( - &self, - key: SymmKeyRef, - data: &EncString, - ) -> Result, crate::CryptoError>; - - fn encrypt_data_with_symmetric_key( - &self, - key: SymmKeyRef, - data: &[u8], - ) -> Result; - - fn decrypt_and_store_symmetric_key( - &mut self, - encryption_key: SymmKeyRef, - new_key_ref: SymmKeyRef, - encrypted_key: &EncString, - ) -> Result<(), crate::CryptoError>; - - fn encrypt_symmetric_key( - &self, - encryption_key: SymmKeyRef, - key_to_encrypt: SymmKeyRef, - ) -> Result; - - // Asymmetric key operations - - fn remove_asymmetric_key(&mut self, key_ref: AsymmKeyRef); - - fn decrypt_data_with_asymmetric_key( - &self, - key: AsymmKeyRef, - data: &AsymmetricEncString, - ) -> Result, crate::CryptoError>; - - fn encrypt_data_with_asymmetric_key( - &self, - key: AsymmKeyRef, - data: &[u8], - ) -> Result; - - fn decrypt_and_store_asymmetric_key( - &mut self, - encryption_key: AsymmKeyRef, - new_key_ref: AsymmKeyRef, - encrypted_key: &AsymmetricEncString, - ) -> Result<(), crate::CryptoError>; - - fn encrypt_asymmetric_key( - &self, - encryption_key: AsymmKeyRef, - key_to_encrypt: AsymmKeyRef, - ) -> Result; + pub(crate) fn new() -> Self { + Self { + key_stores: RwLock::new(RustCryptoEngineKeys { + symmetric_keys: create_key_store(), + asymmetric_keys: create_key_store(), + }), + } + } } -fn _ensure_that_traits_are_object_safe< - SymmKeyRef: SymmetricKeyRef, - AsymmKeyRef: AsymmetricKeyRef, ->( - _: Box>, - _: Box>, -) { +impl + RustCryptoEngine +{ + pub(crate) fn context(&'_ self) -> RustCryptoEngineContext<'_, SymmKeyRef, AsymmKeyRef> { + // TODO: Cache these?, or maybe initialize them lazily? or both? + RustCryptoEngineContext { + global_keys: self.key_stores.read().expect("RwLock is poisoned"), + local_symmetric_keys: create_key_store(), + local_asymmetric_keys: create_key_store(), + } + } + + pub(crate) fn insert_symmetric_key(&self, key_ref: SymmKeyRef, key: SymmetricCryptoKey) { + self.key_stores + .write() + .expect("RwLock is poisoned") + .symmetric_keys + .insert(key_ref, key); + } + + pub(crate) fn clear(&self) { + let mut keys = self.key_stores.write().expect("RwLock is poisoned"); + keys.symmetric_keys.clear(); + keys.asymmetric_keys.clear(); + } } diff --git a/crates/bitwarden-crypto/src/service/key_store/mod.rs b/crates/bitwarden-crypto/src/service/key_store/mod.rs index 2b4bd0476..4deec3317 100644 --- a/crates/bitwarden-crypto/src/service/key_store/mod.rs +++ b/crates/bitwarden-crypto/src/service/key_store/mod.rs @@ -11,6 +11,15 @@ mod util; pub(crate) use linux_memfd_secret_impl::LinuxMemfdSecretKeyStore; pub(crate) use rust_impl::RustKeyStore; +pub(crate) fn create_key_store() -> Box> { + #[cfg(target_os = "linux")] + if let Some(key_store) = LinuxMemfdSecretKeyStore::::new() { + return Box::new(key_store); + } + + Box::new(RustKeyStore::new()) +} + /// This trait represents a platform that can securely store and return keys. The `RustKeyStore` /// implementation is a simple in-memory store without any security guarantees. Other /// implementations could use secure enclaves or HSMs, or OS provided keychains. diff --git a/crates/bitwarden-crypto/src/service/mod.rs b/crates/bitwarden-crypto/src/service/mod.rs index dcf8ef8cf..f0437cbdc 100644 --- a/crates/bitwarden-crypto/src/service/mod.rs +++ b/crates/bitwarden-crypto/src/service/mod.rs @@ -7,7 +7,7 @@ mod encryptable; pub mod key_ref; mod key_store; -use crypto_engine::{CryptoEngine, CryptoEngineContext, RustCryptoEngine}; +use crypto_engine::{RustCryptoEngine, RustCryptoEngineContext}; pub use encryptable::{Decryptable, Encryptable, KeyProvided, KeyProvidedExt, UsesKey}; use key_ref::{AsymmetricKeyRef, KeyRef, SymmetricKeyRef}; @@ -15,7 +15,7 @@ use key_ref::{AsymmetricKeyRef, KeyRef, SymmetricKeyRef}; pub struct CryptoService { // We use an Arc<> to make it easier to pass this service around, as we can // clone it instead of passing references - engine: Arc>, + engine: Arc>, } impl @@ -109,7 +109,7 @@ impl } pub struct CryptoServiceContext<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> { - engine: Box + 'a>, + engine: RustCryptoEngineContext<'a, SymmKeyRef, AsymmKeyRef>, } impl<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> From f708fcc7011e01a03e5b2843922259b77b0dda59 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garci=CC=81a?= Date: Mon, 23 Sep 2024 16:09:47 +0200 Subject: [PATCH 08/34] Inline cryptoengine --- .../service/{crypto_engine => }/context.rs | 14 ++--- .../src/service/crypto_engine/mod.rs | 60 ------------------- crates/bitwarden-crypto/src/service/mod.rs | 40 +++++++++---- 3 files changed, 37 insertions(+), 77 deletions(-) rename crates/bitwarden-crypto/src/service/{crypto_engine => }/context.rs (94%) delete mode 100644 crates/bitwarden-crypto/src/service/crypto_engine/mod.rs diff --git a/crates/bitwarden-crypto/src/service/crypto_engine/context.rs b/crates/bitwarden-crypto/src/service/context.rs similarity index 94% rename from crates/bitwarden-crypto/src/service/crypto_engine/context.rs rename to crates/bitwarden-crypto/src/service/context.rs index 8b5e0e99c..6334b5794 100644 --- a/crates/bitwarden-crypto/src/service/crypto_engine/context.rs +++ b/crates/bitwarden-crypto/src/service/context.rs @@ -5,24 +5,24 @@ use crate::{ AsymmetricCryptoKey, AsymmetricEncString, CryptoError, EncString, SymmetricCryptoKey, }; -use super::RustCryptoEngineKeys; +use super::RustCryptoServiceKeys; -pub(crate) struct RustCryptoEngineContext< +pub(crate) struct RustCryptoServiceContext< 'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef, > { // We hold a RwLock read guard to avoid having any nested //calls locking it again and potentially causing a deadlock - pub(super) global_keys: - std::sync::RwLockReadGuard<'a, RustCryptoEngineKeys>, + pub(crate) global_keys: + std::sync::RwLockReadGuard<'a, RustCryptoServiceKeys>, - pub(super) local_symmetric_keys: Box>, - pub(super) local_asymmetric_keys: Box>, + pub(crate) local_symmetric_keys: Box>, + pub(crate) local_asymmetric_keys: Box>, } impl<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> - RustCryptoEngineContext<'a, SymmKeyRef, AsymmKeyRef> + RustCryptoServiceContext<'a, SymmKeyRef, AsymmKeyRef> { fn get_symmetric_key( &self, diff --git a/crates/bitwarden-crypto/src/service/crypto_engine/mod.rs b/crates/bitwarden-crypto/src/service/crypto_engine/mod.rs deleted file mode 100644 index 1d75f994c..000000000 --- a/crates/bitwarden-crypto/src/service/crypto_engine/mod.rs +++ /dev/null @@ -1,60 +0,0 @@ -use std::sync::RwLock; - -use super::key_store::create_key_store; -use crate::{ - service::{key_store::KeyStore, AsymmetricKeyRef, SymmetricKeyRef}, - SymmetricCryptoKey, -}; - -mod context; -pub(crate) use context::RustCryptoEngineContext; - -pub(crate) struct RustCryptoEngine { - key_stores: RwLock>, -} - -// This is just a wrapper around the keys so we only deal with one RwLock -struct RustCryptoEngineKeys { - symmetric_keys: Box>, - asymmetric_keys: Box>, -} - -impl - RustCryptoEngine -{ - pub(crate) fn new() -> Self { - Self { - key_stores: RwLock::new(RustCryptoEngineKeys { - symmetric_keys: create_key_store(), - asymmetric_keys: create_key_store(), - }), - } - } -} - -impl - RustCryptoEngine -{ - pub(crate) fn context(&'_ self) -> RustCryptoEngineContext<'_, SymmKeyRef, AsymmKeyRef> { - // TODO: Cache these?, or maybe initialize them lazily? or both? - RustCryptoEngineContext { - global_keys: self.key_stores.read().expect("RwLock is poisoned"), - local_symmetric_keys: create_key_store(), - local_asymmetric_keys: create_key_store(), - } - } - - pub(crate) fn insert_symmetric_key(&self, key_ref: SymmKeyRef, key: SymmetricCryptoKey) { - self.key_stores - .write() - .expect("RwLock is poisoned") - .symmetric_keys - .insert(key_ref, key); - } - - pub(crate) fn clear(&self) { - let mut keys = self.key_stores.write().expect("RwLock is poisoned"); - keys.symmetric_keys.clear(); - keys.asymmetric_keys.clear(); - } -} diff --git a/crates/bitwarden-crypto/src/service/mod.rs b/crates/bitwarden-crypto/src/service/mod.rs index f0437cbdc..3906b9301 100644 --- a/crates/bitwarden-crypto/src/service/mod.rs +++ b/crates/bitwarden-crypto/src/service/mod.rs @@ -1,21 +1,28 @@ -use std::sync::Arc; +use std::sync::{Arc, RwLock}; use crate::{EncString, SymmetricCryptoKey}; -mod crypto_engine; +mod context; mod encryptable; pub mod key_ref; mod key_store; -use crypto_engine::{RustCryptoEngine, RustCryptoEngineContext}; +use context::RustCryptoServiceContext; pub use encryptable::{Decryptable, Encryptable, KeyProvided, KeyProvidedExt, UsesKey}; use key_ref::{AsymmetricKeyRef, KeyRef, SymmetricKeyRef}; +use key_store::{create_key_store, KeyStore}; #[derive(Clone)] pub struct CryptoService { // We use an Arc<> to make it easier to pass this service around, as we can // clone it instead of passing references - engine: Arc>, + key_stores: Arc>>, +} + +// This is just a wrapper around the keys so we only deal with one RwLock +struct RustCryptoServiceKeys { + symmetric_keys: Box>, + asymmetric_keys: Box>, } impl @@ -24,24 +31,37 @@ impl #[allow(clippy::new_without_default)] pub fn new() -> Self { Self { - engine: Arc::new(RustCryptoEngine::new()), + key_stores: Arc::new(RwLock::new(RustCryptoServiceKeys { + symmetric_keys: create_key_store(), + asymmetric_keys: create_key_store(), + })), } } pub fn clear(&self) { - self.engine.clear(); + let mut keys = self.key_stores.write().expect("RwLock is poisoned"); + keys.symmetric_keys.clear(); + keys.asymmetric_keys.clear(); } #[deprecated(note = "We should be generating/decrypting the keys into the service directly")] pub fn insert_symmetric_key(&self, key_ref: SymmKeyRef, key: SymmetricCryptoKey) { - #[allow(deprecated)] - self.engine.insert_symmetric_key(key_ref, key); + self.key_stores + .write() + .expect("RwLock is poisoned") + .symmetric_keys + .insert(key_ref, key); } // TODO: Do we want this to be public? pub(crate) fn context(&'_ self) -> CryptoServiceContext<'_, SymmKeyRef, AsymmKeyRef> { CryptoServiceContext { - engine: self.engine.context(), + // TODO: Cache these?, or maybe initialize them lazily? or both? + engine: RustCryptoServiceContext { + global_keys: self.key_stores.read().expect("RwLock is poisoned"), + local_symmetric_keys: create_key_store(), + local_asymmetric_keys: create_key_store(), + }, } } @@ -109,7 +129,7 @@ impl } pub struct CryptoServiceContext<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> { - engine: RustCryptoEngineContext<'a, SymmKeyRef, AsymmKeyRef>, + engine: RustCryptoServiceContext<'a, SymmKeyRef, AsymmKeyRef>, } impl<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> From 30854f705a281db6882767c7738008d2561cde11 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garci=CC=81a?= Date: Mon, 23 Sep 2024 21:00:32 +0200 Subject: [PATCH 09/34] Impl KeyStore in Slice struct directly --- crates/bitwarden-core/src/lib.rs | 4 +- .../bitwarden-crypto/src/service/context.rs | 8 +- .../key_store/linux_memfd_secret_impl.rs | 124 ++++---- .../src/service/key_store/mod.rs | 2 +- .../src/service/key_store/rust_impl.rs | 89 ++---- .../src/service/key_store/util.rs | 270 ++++++++++-------- 6 files changed, 231 insertions(+), 266 deletions(-) diff --git a/crates/bitwarden-core/src/lib.rs b/crates/bitwarden-core/src/lib.rs index 9126eef95..53b2571b5 100644 --- a/crates/bitwarden-core/src/lib.rs +++ b/crates/bitwarden-core/src/lib.rs @@ -151,7 +151,7 @@ mod testcrypto { .unwrap(); // And decrypt values in parallel - let mut data = Vec::with_capacity(10_000_000); + let mut data = Vec::with_capacity(500_000); for _ in 0..data.capacity() { data.push("hello world, this is an encryption test!".using_key(MySymmKeyRef::User)); } @@ -165,6 +165,6 @@ mod testcrypto { } println!("Individual encrypting took {:?}", now.elapsed()); - panic!("DONE") + // panic!("DONE") } } diff --git a/crates/bitwarden-crypto/src/service/context.rs b/crates/bitwarden-crypto/src/service/context.rs index 6334b5794..f8e086ab1 100644 --- a/crates/bitwarden-crypto/src/service/context.rs +++ b/crates/bitwarden-crypto/src/service/context.rs @@ -1,12 +1,13 @@ +use std::sync::RwLockReadGuard; + use rsa::Oaep; +use super::RustCryptoServiceKeys; use crate::{ service::{key_store::KeyStore, AsymmetricKeyRef, SymmetricKeyRef}, AsymmetricCryptoKey, AsymmetricEncString, CryptoError, EncString, SymmetricCryptoKey, }; -use super::RustCryptoServiceKeys; - pub(crate) struct RustCryptoServiceContext< 'a, SymmKeyRef: SymmetricKeyRef, @@ -14,8 +15,7 @@ pub(crate) struct RustCryptoServiceContext< > { // We hold a RwLock read guard to avoid having any nested //calls locking it again and potentially causing a deadlock - pub(crate) global_keys: - std::sync::RwLockReadGuard<'a, RustCryptoServiceKeys>, + pub(crate) global_keys: RwLockReadGuard<'a, RustCryptoServiceKeys>, pub(crate) local_symmetric_keys: Box>, pub(crate) local_asymmetric_keys: Box>, diff --git a/crates/bitwarden-crypto/src/service/key_store/linux_memfd_secret_impl.rs b/crates/bitwarden-crypto/src/service/key_store/linux_memfd_secret_impl.rs index 2192cc681..9081cec61 100644 --- a/crates/bitwarden-crypto/src/service/key_store/linux_memfd_secret_impl.rs +++ b/crates/bitwarden-crypto/src/service/key_store/linux_memfd_secret_impl.rs @@ -1,34 +1,29 @@ -use std::{ptr::NonNull, sync::OnceLock}; - -use zeroize::ZeroizeOnDrop; +use std::{mem::MaybeUninit, ptr::NonNull, sync::OnceLock}; use super::{ - util::{KeyData, SliceKeyContainer}, - KeyRef, KeyStore, + util::{KeyData, SliceKeyStore}, + KeyRef, }; -fn is_memfd_supported() -> 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 - }) -} +// 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 type LinuxMemfdSecretKeyStore = SliceKeyStore; -struct MemPtr { +struct MemfdSecretImplKeyData { ptr: std::ptr::NonNull<[u8]>, capacity: usize, } -// TODO: Is this safe? -unsafe impl Send for MemPtr {} -unsafe impl Sync for MemPtr {} +// 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 MemPtr { +impl Drop for MemfdSecretImplKeyData { fn drop(&mut self) { unsafe { memsec::free_memfd_secret(self.ptr); @@ -36,85 +31,62 @@ impl Drop for MemPtr { } } -impl KeyData for MemPtr { - fn new_with_capacity(capacity: usize) -> Self { +impl KeyData 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("Supported operation"); - MemPtr { ptr, capacity } + 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) } } } -// 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) struct LinuxMemfdSecretKeyStore { - container: SliceKeyContainer, - - _key: std::marker::PhantomData, -} - -impl LinuxMemfdSecretKeyStore { - pub(crate) fn new() -> Option { - Self::with_capacity(0) - } - - pub(crate) fn with_capacity(capacity: usize) -> Option { - if !is_memfd_supported() { - return None; - } - - Some(Self { - container: SliceKeyContainer::new_with_capacity(capacity), - _key: std::marker::PhantomData, - }) - } -} - -// Zeroize is done by the Drop impl of SliceKeyContainer -impl ZeroizeOnDrop for LinuxMemfdSecretKeyStore {} - -impl KeyStore for LinuxMemfdSecretKeyStore { - fn insert(&mut self, key_ref: Key, key: Key::KeyValue) { - self.container.insert(key_ref, key); - } - fn get(&self, key_ref: Key) -> Option<&Key::KeyValue> { - self.container.get(key_ref) - } - fn remove(&mut self, key_ref: Key) { - self.container.remove(key_ref) - } - fn clear(&mut self) { - self.container.clear() - } - fn retain(&mut self, f: fn(Key) -> bool) { - self.container.retain(f); - } -} - #[cfg(test)] mod tests { - use super::{super::util::tests::*, *}; + use super::*; + use crate::service::key_store::{util::tests::*, KeyStore as _}; #[test] fn test_resize() { - let mut store = super::LinuxMemfdSecretKeyStore::::with_capacity(1).unwrap(); + let mut store = LinuxMemfdSecretKeyStore::::with_capacity(1).unwrap(); for (idx, key) in [ TestKey::A, diff --git a/crates/bitwarden-crypto/src/service/key_store/mod.rs b/crates/bitwarden-crypto/src/service/key_store/mod.rs index 4deec3317..ebe31ba38 100644 --- a/crates/bitwarden-crypto/src/service/key_store/mod.rs +++ b/crates/bitwarden-crypto/src/service/key_store/mod.rs @@ -17,7 +17,7 @@ pub(crate) fn create_key_store() -> Box> { return Box::new(key_store); } - Box::new(RustKeyStore::new()) + Box::new(RustKeyStore::new().expect("RustKeyStore should always be available")) } /// This trait represents a platform that can securely store and return keys. The `RustKeyStore` diff --git a/crates/bitwarden-crypto/src/service/key_store/rust_impl.rs b/crates/bitwarden-crypto/src/service/key_store/rust_impl.rs index 877e876d1..67a92fb49 100644 --- a/crates/bitwarden-crypto/src/service/key_store/rust_impl.rs +++ b/crates/bitwarden-crypto/src/service/key_store/rust_impl.rs @@ -1,17 +1,23 @@ -use zeroize::ZeroizeOnDrop; +use std::mem::MaybeUninit; use super::{ - util::{KeyData, SliceKeyContainer}, - KeyRef, KeyStore, + util::{KeyData, SliceKeyStore}, + KeyRef, }; + +// 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 RustKeyStore = SliceKeyStore>; + const ENABLE_MLOCK: bool = true; -struct Mem { +pub(crate) struct RustImplKeyData { #[allow(clippy::type_complexity)] data: Box<[Option<(Key, Key::KeyValue)>]>, } -impl Drop for Mem { +impl Drop for RustImplKeyData { fn drop(&mut self) { #[cfg(not(target_arch = "wasm32"))] if ENABLE_MLOCK { @@ -26,8 +32,12 @@ impl Drop for Mem { } } -impl KeyData for Mem { - fn new_with_capacity(capacity: usize) -> Self { +impl KeyData for RustImplKeyData { + fn is_available() -> bool { + true + } + + fn with_capacity(capacity: usize) -> Self { let mut data: Box<_> = std::iter::repeat_with(|| None).take(capacity).collect(); #[cfg(not(target_arch = "wasm32"))] @@ -37,7 +47,7 @@ impl KeyData for Mem { memsec::mlock(data.as_mut_ptr() as *mut u8, capacity * entry_size); } } - Mem { data } + RustImplKeyData { data } } fn get_key_data(&self) -> &[Option<(Key, Key::KeyValue)>] { @@ -49,71 +59,14 @@ impl KeyData for Mem { } } -// 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) struct RustKeyStore { - #[allow(clippy::type_complexity)] - container: SliceKeyContainer>, -} - -impl RustKeyStore { - pub(crate) fn new() -> Self { - Self::with_capacity(0) - } - - pub(crate) fn with_capacity(capacity: usize) -> Self { - Self { - container: SliceKeyContainer::new_with_capacity(capacity), - } - } -} - -// Zeroize is done by the Drop impl of SliceKeyContainer -impl ZeroizeOnDrop for RustKeyStore {} - -impl KeyStore for RustKeyStore { - fn insert(&mut self, key_ref: Key, key: Key::KeyValue) { - /* - if let Err(new_capacity) = self.container.ensure_capacity(1) { - // Create a new store with the correct capacity and replace self with it - let mut new_self = Self::with_capacity(new_capacity); - new_self.container.copy_from(&mut self.container); - *self = new_self; - }; - - let ok = self.container.insert(key_ref, key); - debug_assert!(ok, "insert failed"); - - */ - - self.container.insert(key_ref, key) - } - - fn get(&self, key_ref: Key) -> Option<&Key::KeyValue> { - self.container.get(key_ref) - } - - fn remove(&mut self, key_ref: Key) { - self.container.remove(key_ref); - } - - fn clear(&mut self) { - self.container.clear(); - } - - fn retain(&mut self, f: fn(Key) -> bool) { - self.container.retain(f); - } -} - #[cfg(test)] mod tests { - use super::{super::util::tests::*, *}; + use super::*; + use crate::service::key_store::{util::tests::*, KeyStore as _}; #[test] fn test_resize() { - let mut store = super::RustKeyStore::::with_capacity(1); + let mut store = RustKeyStore::::with_capacity(1).unwrap(); for (idx, key) in [ TestKey::A, diff --git a/crates/bitwarden-crypto/src/service/key_store/util.rs b/crates/bitwarden-crypto/src/service/key_store/util.rs index 818591bd3..a8bc2f41a 100644 --- a/crates/bitwarden-crypto/src/service/key_store/util.rs +++ b/crates/bitwarden-crypto/src/service/key_store/util.rs @@ -1,19 +1,39 @@ +use std::marker::PhantomData; + +use zeroize::ZeroizeOnDrop; + +use super::KeyStore; use crate::service::key_ref::KeyRef; -// This trait represents some data stored sequentially in memory, with a fixed size. -// We use this to abstract the implementation over Vec/Box<[u8]/NonNull<[u8]>. +/// This trait represents some data stored sequentially in memory, with a fixed size. +/// We use this to abstract the implementation over Vec/Box<[u8]/NonNull<[u8]>, which +/// helps contain any unsafe code to the implementations of this trait. +/// 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 KeyData: Drop { - fn new_with_capacity(capacity: usize) -> Self; +pub(crate) trait KeyData: 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 container over an arbitrary fixed size slice. +/// 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 Vec, a Box<[u8]> or a NonNull. -pub(crate) struct SliceKeyContainer> { +pub(crate) struct SliceKeyStore> { // This represents the number of elements in the container, it's always less than or equal to // the length of `data`. length: usize, @@ -26,41 +46,127 @@ pub(crate) struct SliceKeyContainer> { // uninitialized data: Option, - _key: std::marker::PhantomData, + _key: PhantomData, } -impl> Drop for SliceKeyContainer { +impl> ZeroizeOnDrop for SliceKeyStore {} + +impl> Drop for SliceKeyStore { fn drop(&mut self) { self.clear(); + } +} + +impl> KeyStore for SliceKeyStore { + fn insert(&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; + } + } + } - // Ensure the container gets dropped after the data is cleared - let _ = self.data.take(); + 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) { + self.get_key_data_mut().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; } } #[allow(dead_code)] -impl> SliceKeyContainer { - pub(crate) fn new_with_capacity(capacity: usize) -> Self { +impl> SliceKeyStore { + pub(crate) fn is_available() -> bool { + Data::is_available() + } + + pub(crate) fn new() -> Option { + Self::with_capacity(0) + } + + pub(crate) fn with_capacity(capacity: usize) -> Option { + // If the capacity is 0, we don't need to allocate any memory. + // This allows us to initialize the container lazily. if capacity == 0 { - return Self { + return Some(Self { length: 0, capacity: 0, data: None, - _key: std::marker::PhantomData, - }; + _key: PhantomData, + }); } - let mut container = Self { + Some(Self { length: 0, capacity, - data: Some(Data::new_with_capacity(capacity)), - _key: std::marker::PhantomData, - }; - - // Ensure the container is properly initialized - container.clear(); - - container + data: Some(Data::with_capacity(capacity)), + _key: PhantomData, + }) } /// Check if the container has enough capacity to store `new_elements` more elements. @@ -97,7 +203,8 @@ impl> SliceKeyContainer { 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::new_with_capacity(new_capacity); + let mut new_self = + Self::with_capacity(new_capacity).expect("Could not allocate new store"); new_self.copy_from(self); *self = new_self; } @@ -135,86 +242,6 @@ impl> SliceKeyContainer { }) } - pub(crate) fn clear(&mut self) { - self.get_key_data_mut().fill_with(|| None); - self.length = 0; - } - - pub(crate) 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; - } - } - - pub(crate) fn insert(&mut self, key_ref: 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; - } - } - } - - pub(crate) fn get(&self, key_ref: Key) -> Option<&::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)) - } - - pub(crate) 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; - } - pub(crate) fn copy_from(&mut self, other: &mut Self) -> bool { if other.capacity > self.capacity { return false; @@ -288,7 +315,11 @@ pub(crate) mod tests { } impl KeyData for TestKeyContainer { - fn new_with_capacity(capacity: usize) -> Self { + fn is_available() -> bool { + true + } + + fn with_capacity(capacity: usize) -> Self { Self { keys: std::iter::repeat_with(|| None).take(capacity).collect(), } @@ -305,7 +336,8 @@ pub(crate) mod tests { #[test] fn test_slice_container_insertion() { - let mut container = SliceKeyContainer::>::new_with_capacity(5); + let mut container = + SliceKeyStore::>::with_capacity(5).unwrap(); assert_eq!(container.get_key_data(), [None, None, None, None, None]); @@ -416,7 +448,8 @@ pub(crate) mod tests { #[test] fn test_slice_container_get() { - let mut container = SliceKeyContainer::>::new_with_capacity(5); + let mut container = + SliceKeyStore::>::with_capacity(5).unwrap(); for (key, value) in [ (TestKey::A, TestKeyValue::new(1)), @@ -434,7 +467,8 @@ pub(crate) mod tests { #[test] fn test_slice_container_clear() { - let mut container = SliceKeyContainer::>::new_with_capacity(5); + let mut container = + SliceKeyStore::>::with_capacity(5).unwrap(); for (key, value) in [ (TestKey::A, TestKeyValue::new(1)), @@ -464,7 +498,8 @@ pub(crate) mod tests { #[test] fn test_slice_container_ensure_capacity() { - let mut container = SliceKeyContainer::>::new_with_capacity(5); + let mut container = + SliceKeyStore::>::with_capacity(5).unwrap(); assert_eq!(container.capacity, 5); assert_eq!(container.length, 0); @@ -494,7 +529,8 @@ pub(crate) mod tests { #[test] fn test_slice_container_removal() { - let mut container = SliceKeyContainer::>::new_with_capacity(5); + let mut container = + SliceKeyStore::>::with_capacity(5).unwrap(); for (key, value) in [ (TestKey::A, TestKeyValue::new(1)), @@ -580,7 +616,8 @@ pub(crate) mod tests { #[test] fn test_slice_container_retain_removes_one() { - let mut container = SliceKeyContainer::>::new_with_capacity(5); + let mut container = + SliceKeyStore::>::with_capacity(5).unwrap(); for (key, value) in [ (TestKey::A, TestKeyValue::new(1)), @@ -666,7 +703,8 @@ pub(crate) mod tests { #[test] fn test_slice_container_retain_removes_none() { - let mut container = SliceKeyContainer::>::new_with_capacity(5); + let mut container = + SliceKeyStore::>::with_capacity(5).unwrap(); for (key, value) in [ (TestKey::A, TestKeyValue::new(1)), @@ -693,7 +731,8 @@ pub(crate) mod tests { #[test] fn test_slice_container_retain_removes_some() { - let mut container = SliceKeyContainer::>::new_with_capacity(5); + let mut container = + SliceKeyStore::>::with_capacity(5).unwrap(); for (key, value) in [ (TestKey::A, TestKeyValue::new(1)), @@ -720,7 +759,8 @@ pub(crate) mod tests { #[test] fn test_slice_container_retain_removes_all() { - let mut container = SliceKeyContainer::>::new_with_capacity(5); + let mut container = + SliceKeyStore::>::with_capacity(5).unwrap(); for (key, value) in [ (TestKey::A, TestKeyValue::new(1)), From bed894a9b2baf2a8c9ddfa68474ea186f5cfb090 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garci=CC=81a?= Date: Mon, 23 Sep 2024 21:01:07 +0200 Subject: [PATCH 10/34] Reset the values to None after munlock has zeroized them Otherwise some Drop implementations can crash --- .../src/service/key_store/rust_impl.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/crates/bitwarden-crypto/src/service/key_store/rust_impl.rs b/crates/bitwarden-crypto/src/service/key_store/rust_impl.rs index 67a92fb49..70be4ff63 100644 --- a/crates/bitwarden-crypto/src/service/key_store/rust_impl.rs +++ b/crates/bitwarden-crypto/src/service/key_store/rust_impl.rs @@ -27,6 +27,16 @@ impl Drop for RustImplKeyData { self.data.as_mut_ptr() as *mut u8, self.data.len() * entry_size, ); + + // Note: munlock is zeroing the memory, which leaves the data in an inconsistent state. + // So we need to set it to None again, in case any Drop impl expects the data to be correct. + let uninit_slice: &mut [MaybeUninit<_>] = std::slice::from_raw_parts_mut( + self.data.as_mut_ptr() as *mut MaybeUninit>, + self.data.len(), + ); + for elem in uninit_slice { + elem.write(None); + } } } } From f7eda88f634a042ae66c8d7f63f798a31fb4ccc3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garci=CC=81a?= Date: Mon, 23 Sep 2024 21:01:54 +0200 Subject: [PATCH 11/34] Fmt --- crates/bitwarden-crypto/src/service/key_store/rust_impl.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/crates/bitwarden-crypto/src/service/key_store/rust_impl.rs b/crates/bitwarden-crypto/src/service/key_store/rust_impl.rs index 70be4ff63..3a69c273d 100644 --- a/crates/bitwarden-crypto/src/service/key_store/rust_impl.rs +++ b/crates/bitwarden-crypto/src/service/key_store/rust_impl.rs @@ -28,8 +28,9 @@ impl Drop for RustImplKeyData { self.data.len() * entry_size, ); - // Note: munlock is zeroing the memory, which leaves the data in an inconsistent state. - // So we need to set it to None again, in case any Drop impl expects the data to be correct. + // Note: munlock is zeroing the memory, which leaves the data in an inconsistent + // state. So we need to set it to None again, in case any Drop impl + // expects the data to be correct. let uninit_slice: &mut [MaybeUninit<_>] = std::slice::from_raw_parts_mut( self.data.as_mut_ptr() as *mut MaybeUninit>, self.data.len(), From ce2343ef6045cb7432b606170723308f220c989b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garci=CC=81a?= Date: Tue, 24 Sep 2024 12:22:04 +0200 Subject: [PATCH 12/34] Add benchmark --- crates/bitwarden-core/src/lib.rs | 14 +- crates/bitwarden-crypto/Cargo.toml | 6 +- .../benches/new_encryptable.rs | 180 ++++++++++++++++++ 3 files changed, 187 insertions(+), 13 deletions(-) create mode 100644 crates/bitwarden-crypto/benches/new_encryptable.rs diff --git a/crates/bitwarden-core/src/lib.rs b/crates/bitwarden-core/src/lib.rs index 53b2571b5..17966e61f 100644 --- a/crates/bitwarden-core/src/lib.rs +++ b/crates/bitwarden-core/src/lib.rs @@ -151,20 +151,10 @@ mod testcrypto { .unwrap(); // And decrypt values in parallel - let mut data = Vec::with_capacity(500_000); + let mut data = Vec::with_capacity(250); for _ in 0..data.capacity() { data.push("hello world, this is an encryption test!".using_key(MySymmKeyRef::User)); } - let now = std::time::Instant::now(); - let _ = service.encrypt_list(&data).unwrap(); - println!("Batch encrypting took {:?}", now.elapsed()); - - let now = std::time::Instant::now(); - for d in data { - let _ = service.encrypt(d).unwrap(); - } - println!("Individual encrypting took {:?}", now.elapsed()); - - // panic!("DONE") + service.encrypt_list(&data).unwrap(); } } diff --git a/crates/bitwarden-crypto/Cargo.toml b/crates/bitwarden-crypto/Cargo.toml index 4109b659e..6b0c28079 100644 --- a/crates/bitwarden-crypto/Cargo.toml +++ b/crates/bitwarden-crypto/Cargo.toml @@ -50,7 +50,7 @@ zeroize = { version = ">=1.7.0, <2.0", features = ["derive", "aarch64"] } memsec = { version = "0.7.0", features = ["alloc_ext"] } [dev-dependencies] -criterion = "0.5.1" +criterion = { version = "0.5.1", features = ["html_reports"] } rand_chacha = "0.3.1" serde_json = ">=1.0.96, <2.0" @@ -64,5 +64,9 @@ name = "zeroizing_allocator" harness = false required-features = ["no-memory-hardening"] +[[bench]] +name = "new_encryptable" +harness = false + [lints] workspace = true diff --git a/crates/bitwarden-crypto/benches/new_encryptable.rs b/crates/bitwarden-crypto/benches/new_encryptable.rs new file mode 100644 index 000000000..892bb5ea9 --- /dev/null +++ b/crates/bitwarden-crypto/benches/new_encryptable.rs @@ -0,0 +1,180 @@ +use criterion::{black_box, criterion_group, criterion_main, BenchmarkId, Criterion, Throughput}; + +pub fn criterion_benchmark(c: &mut Criterion) { + let user_key = SymmetricCryptoKey::generate(rand::thread_rng()); + + let org_id = uuid::Uuid::parse_str("91b000b6-81ce-47f4-9802-3390e0b895ed").expect(""); + let org_key = SymmetricCryptoKey::generate(rand::thread_rng()); + + let cipher_key = SymmetricCryptoKey::generate(rand::thread_rng()); + let cipher_key_user_enc = cipher_key.to_vec().encrypt_with_key(&user_key).expect(""); + let cipher_view = CipherView { + key: Some(cipher_key_user_enc.clone()), + name: "test".to_string(), + }; + + let service: CryptoService = CryptoService::new(); + #[allow(deprecated)] + { + service.insert_symmetric_key(MySymmKeyRef::User, user_key.clone()); + service.insert_symmetric_key(MySymmKeyRef::Organization(org_id), org_key.clone()); + } + + let cipher_views = vec![cipher_view.clone(); 10_000]; + + { + let mut group = c.benchmark_group("New encryptable"); + + for size in [1, 10, 100, 1_000, 10_000].iter() { + group.throughput(Throughput::Elements(*size as u64)); + + group.bench_with_input( + BenchmarkId::new("encrypt X ciphers individually", size), + size, + |b, &size| { + b.iter(|| { + for c in cipher_views.iter().take(size).cloned() { + service.encrypt(black_box(c)).expect(""); + } + }); + }, + ); + + group.bench_with_input( + BenchmarkId::new("encrypt X ciphers batch", size), + size, + |b, &size| { + b.iter(|| service.encrypt_list(black_box(&cipher_views[..size]))); + }, + ); + } + + group.finish(); + } + + { + let mut group = c.benchmark_group("Old encryptable"); + + for size in [1, 10, 100, 1_000, 10_000].iter() { + group.throughput(Throughput::Elements(*size as u64)); + + group.bench_with_input( + BenchmarkId::new("encrypt X ciphers individually", size), + size, + |b, &size| { + b.iter(|| { + for c in cipher_views.iter().take(size).cloned() { + black_box(c).encrypt_with_key(&user_key).expect(""); + } + }); + }, + ); + + group.bench_with_input( + BenchmarkId::new("encrypt X ciphers batch", size), + size, + |b, &size| { + b.iter(|| { + black_box(cipher_views[0..size].to_vec()) + .encrypt_with_key(&user_key) + .expect(""); + }); + }, + ); + } + + group.finish(); + } +} + +criterion_group!(benches, criterion_benchmark); +criterion_main!(benches); + +use bitwarden_crypto::{ + key_refs, service::*, CryptoError, EncString, KeyDecryptable, KeyEncryptable, + SymmetricCryptoKey, +}; + +key_refs! { + #[symmetric] + pub enum MySymmKeyRef { + User, + Organization(uuid::Uuid), + #[local] + Local(&'static str), + } + + #[asymmetric] + pub enum MyAsymmKeyRef { + UserPrivateKey, + #[local] + Local(&'static str), + } +} + +#[allow(unused)] +struct Cipher { + key: Option, + name: EncString, +} + +#[derive(Clone)] +struct CipherView { + key: Option, + name: String, +} + +impl UsesKey for Cipher { + fn uses_key(&self) -> MySymmKeyRef { + MySymmKeyRef::User + } +} +impl UsesKey for CipherView { + fn uses_key(&self) -> MySymmKeyRef { + MySymmKeyRef::User + } +} + +const CIPHER_KEY: MySymmKeyRef = MySymmKeyRef::Local("cipher_key"); + +// New encryptable implementations + +impl Encryptable for CipherView { + fn encrypt( + &self, + ctx: &mut CryptoServiceContext, + key: MySymmKeyRef, + ) -> Result { + let cipher_key = match &self.key { + Some(cipher_key) => ctx.decrypt_and_store_symmetric_key(key, CIPHER_KEY, cipher_key)?, + None => key, + }; + + Ok(Cipher { + key: self.key.clone(), + name: self.name.as_str().encrypt(ctx, cipher_key)?, + }) + } +} + +// Old encryptable implementations + +impl KeyEncryptable for CipherView { + fn encrypt_with_key(self, key: &SymmetricCryptoKey) -> Result { + let cipher_key = self + .key + .as_ref() + .map(|k| { + let mut kk: Vec = k.decrypt_with_key(key)?; + SymmetricCryptoKey::try_from(kk.as_mut_slice()) + }) + .transpose()?; + + let key = cipher_key.as_ref().unwrap_or(key); + + Ok(Cipher { + key: self.key.clone(), + name: self.name.encrypt_with_key(key)?, + }) + } +} From bcd712f346c0d181fbfe007f29e9af1038b12996 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garci=CC=81a?= Date: Tue, 24 Sep 2024 12:25:36 +0200 Subject: [PATCH 13/34] Respect no-memory-hardening flag --- crates/bitwarden-crypto/src/service/key_store/mod.rs | 12 ++++-------- .../src/service/key_store/rust_impl.rs | 10 ++++------ 2 files changed, 8 insertions(+), 14 deletions(-) diff --git a/crates/bitwarden-crypto/src/service/key_store/mod.rs b/crates/bitwarden-crypto/src/service/key_store/mod.rs index ebe31ba38..f5bef9ed3 100644 --- a/crates/bitwarden-crypto/src/service/key_store/mod.rs +++ b/crates/bitwarden-crypto/src/service/key_store/mod.rs @@ -2,22 +2,18 @@ use zeroize::ZeroizeOnDrop; use crate::service::KeyRef; -#[cfg(target_os = "linux")] +#[cfg(all(target_os = "linux", not(feature = "no-memory-hardening")))] mod linux_memfd_secret_impl; mod rust_impl; mod util; -#[cfg(target_os = "linux")] -pub(crate) use linux_memfd_secret_impl::LinuxMemfdSecretKeyStore; -pub(crate) use rust_impl::RustKeyStore; - pub(crate) fn create_key_store() -> Box> { - #[cfg(target_os = "linux")] - if let Some(key_store) = LinuxMemfdSecretKeyStore::::new() { + #[cfg(all(target_os = "linux", not(feature = "no-memory-hardening")))] + if let Some(key_store) = linux_memfd_secret_impl::LinuxMemfdSecretKeyStore::::new() { return Box::new(key_store); } - Box::new(RustKeyStore::new().expect("RustKeyStore should always be available")) + Box::new(rust_impl::RustKeyStore::new().expect("RustKeyStore should always be available")) } /// This trait represents a platform that can securely store and return keys. The `RustKeyStore` diff --git a/crates/bitwarden-crypto/src/service/key_store/rust_impl.rs b/crates/bitwarden-crypto/src/service/key_store/rust_impl.rs index 3a69c273d..ac5d51ab9 100644 --- a/crates/bitwarden-crypto/src/service/key_store/rust_impl.rs +++ b/crates/bitwarden-crypto/src/service/key_store/rust_impl.rs @@ -10,8 +10,6 @@ use super::{ // zeroize the values when dropped. pub(crate) type RustKeyStore = SliceKeyStore>; -const ENABLE_MLOCK: bool = true; - pub(crate) struct RustImplKeyData { #[allow(clippy::type_complexity)] data: Box<[Option<(Key, Key::KeyValue)>]>, @@ -19,8 +17,8 @@ pub(crate) struct RustImplKeyData { impl Drop for RustImplKeyData { fn drop(&mut self) { - #[cfg(not(target_arch = "wasm32"))] - if ENABLE_MLOCK { + #[cfg(any(not(target_arch = "wasm32"), not(feature = "no-memory-hardening")))] + { let entry_size = std::mem::size_of::>(); unsafe { memsec::munlock( @@ -51,8 +49,8 @@ impl KeyData for RustImplKeyData { fn with_capacity(capacity: usize) -> Self { let mut data: Box<_> = std::iter::repeat_with(|| None).take(capacity).collect(); - #[cfg(not(target_arch = "wasm32"))] - if ENABLE_MLOCK { + #[cfg(any(not(target_arch = "wasm32"), not(feature = "no-memory-hardening")))] + { let entry_size = std::mem::size_of::>(); unsafe { memsec::mlock(data.as_mut_ptr() as *mut u8, capacity * entry_size); From 38343d2ff60ffe87a9cf3619d6630fec58d5aa28 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garci=CC=81a?= Date: Tue, 24 Sep 2024 13:23:42 +0200 Subject: [PATCH 14/34] Fix cfg flags, silence warnings --- crates/bitwarden-crypto/src/service/context.rs | 1 + .../bitwarden-crypto/src/service/key_store/rust_impl.rs | 9 +++++---- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/crates/bitwarden-crypto/src/service/context.rs b/crates/bitwarden-crypto/src/service/context.rs index f8e086ab1..61b3b9fab 100644 --- a/crates/bitwarden-crypto/src/service/context.rs +++ b/crates/bitwarden-crypto/src/service/context.rs @@ -21,6 +21,7 @@ pub(crate) struct RustCryptoServiceContext< pub(crate) local_asymmetric_keys: Box>, } +#[allow(unused)] impl<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> RustCryptoServiceContext<'a, SymmKeyRef, AsymmKeyRef> { diff --git a/crates/bitwarden-crypto/src/service/key_store/rust_impl.rs b/crates/bitwarden-crypto/src/service/key_store/rust_impl.rs index ac5d51ab9..ee79edd10 100644 --- a/crates/bitwarden-crypto/src/service/key_store/rust_impl.rs +++ b/crates/bitwarden-crypto/src/service/key_store/rust_impl.rs @@ -1,5 +1,3 @@ -use std::mem::MaybeUninit; - use super::{ util::{KeyData, SliceKeyStore}, KeyRef, @@ -17,8 +15,10 @@ pub(crate) struct RustImplKeyData { impl Drop for RustImplKeyData { fn drop(&mut self) { - #[cfg(any(not(target_arch = "wasm32"), not(feature = "no-memory-hardening")))] + #[cfg(all(not(target_arch = "wasm32"), not(feature = "no-memory-hardening")))] { + use std::mem::MaybeUninit; + let entry_size = std::mem::size_of::>(); unsafe { memsec::munlock( @@ -47,9 +47,10 @@ impl KeyData for RustImplKeyData { } fn with_capacity(capacity: usize) -> Self { + #[allow(unused_mut)] let mut data: Box<_> = std::iter::repeat_with(|| None).take(capacity).collect(); - #[cfg(any(not(target_arch = "wasm32"), not(feature = "no-memory-hardening")))] + #[cfg(all(not(target_arch = "wasm32"), not(feature = "no-memory-hardening")))] { let entry_size = std::mem::size_of::>(); unsafe { From f34ce02364895a840c9f81a8bcd7c428ef762ccc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garci=CC=81a?= Date: Tue, 24 Sep 2024 13:33:21 +0200 Subject: [PATCH 15/34] Export memfd correctly --- .../src/service/key_store/linux_memfd_secret_impl.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/bitwarden-crypto/src/service/key_store/linux_memfd_secret_impl.rs b/crates/bitwarden-crypto/src/service/key_store/linux_memfd_secret_impl.rs index 9081cec61..33fe8bebc 100644 --- a/crates/bitwarden-crypto/src/service/key_store/linux_memfd_secret_impl.rs +++ b/crates/bitwarden-crypto/src/service/key_store/linux_memfd_secret_impl.rs @@ -9,9 +9,9 @@ use super::{ // 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 type LinuxMemfdSecretKeyStore = SliceKeyStore; +pub(crate) type LinuxMemfdSecretKeyStore = SliceKeyStore; -struct MemfdSecretImplKeyData { +pub(crate) struct MemfdSecretImplKeyData { ptr: std::ptr::NonNull<[u8]>, capacity: usize, } From cc27320e64b4046e7dd6e7b6f002e842fd474b1e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garci=CC=81a?= Date: Tue, 24 Sep 2024 13:40:08 +0200 Subject: [PATCH 16/34] Fix memtest --- crates/memory-testing/Dockerfile | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/memory-testing/Dockerfile b/crates/memory-testing/Dockerfile index 3df22466d..0565f031d 100644 --- a/crates/memory-testing/Dockerfile +++ b/crates/memory-testing/Dockerfile @@ -16,13 +16,13 @@ RUN mkdir -p /app/crates/bitwarden-crypto/src \ && touch /app/crates/bitwarden-crypto/src/lib.rs \ /app/crates/bitwarden-crypto/benches/default_allocator.rs \ /app/crates/bitwarden-crypto/benches/zeroizing_allocator.rs \ + /app/crates/bitwarden-crypto/benches/new_encryptable.rs \ && echo 'fn main(){}' > /app/crates/memory-testing/src/main.rs \ && cargo build -p memory-testing --release # Delete dummy files and copy the actual source code RUN rm /app/crates/bitwarden-crypto/src/lib.rs \ - /app/crates/bitwarden-crypto/benches/default_allocator.rs \ - /app/crates/bitwarden-crypto/benches/zeroizing_allocator.rs \ + /app/crates/bitwarden-crypto/benches/*.rs \ /app/crates/memory-testing/src/main.rs COPY crates/bitwarden-crypto /app/crates/bitwarden-crypto From 32d298f6107c735a6d7623831acaf6f9c6113bf3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garci=CC=81a?= Date: Tue, 24 Sep 2024 14:40:27 +0200 Subject: [PATCH 17/34] Try fat LTO to fix windows --- Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Cargo.toml b/Cargo.toml index 8c978819b..e3827dc8e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -54,7 +54,7 @@ codegen-units = 1 # This is a workaround until this is fixed: https://github.com/rustls/rustls-platform-verifier/issues/141 [profile.release-windows] inherits = "release" -lto = "off" +lto = "fat" # Stripping the binary reduces the size by ~30%, but the stacktraces won't be usable anymore. # This is fine as long as we don't have any unhandled panics, but let's keep it disabled for now From c43aa0887ec3bd1f3de2d5279ee37999a8827d3d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garci=CC=81a?= Date: Tue, 24 Sep 2024 14:51:32 +0200 Subject: [PATCH 18/34] Disable memsec on windows to fix it --- Cargo.toml | 2 +- crates/bitwarden-crypto/Cargo.toml | 6 +++--- .../src/service/key_store/rust_impl.rs | 12 ++++++++++-- 3 files changed, 14 insertions(+), 6 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index e3827dc8e..8c978819b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -54,7 +54,7 @@ codegen-units = 1 # This is a workaround until this is fixed: https://github.com/rustls/rustls-platform-verifier/issues/141 [profile.release-windows] inherits = "release" -lto = "fat" +lto = "off" # Stripping the binary reduces the size by ~30%, but the stacktraces won't be usable anymore. # This is fine as long as we don't have any unhandled panics, but let's keep it disabled for now diff --git a/crates/bitwarden-crypto/Cargo.toml b/crates/bitwarden-crypto/Cargo.toml index 6b0c28079..2526e7ef1 100644 --- a/crates/bitwarden-crypto/Cargo.toml +++ b/crates/bitwarden-crypto/Cargo.toml @@ -17,7 +17,7 @@ keywords.workspace = true default = [] uniffi = ["dep:uniffi"] # Uniffi bindings -no-memory-hardening = [] # Disable memory hardening features +no-memory-hardening = ["memsec"] # Disable memory hardening features [dependencies] aes = { version = ">=0.8.2, <0.9", features = ["zeroize"] } @@ -46,8 +46,8 @@ uniffi = { version = "=0.28.1", optional = true } uuid = { version = ">=1.3.3, <2.0", features = ["serde"] } zeroize = { version = ">=1.7.0, <2.0", features = ["derive", "aarch64"] } -[target.'cfg(not(target_arch="wasm32"))'.dependencies] -memsec = { version = "0.7.0", features = ["alloc_ext"] } +[target.'cfg(all(not(target_arch = "wasm32"), not(windows)))'.dependencies] +memsec = { version = "0.7.0", features = ["alloc_ext"], optional = true } [dev-dependencies] criterion = { version = "0.5.1", features = ["html_reports"] } diff --git a/crates/bitwarden-crypto/src/service/key_store/rust_impl.rs b/crates/bitwarden-crypto/src/service/key_store/rust_impl.rs index ee79edd10..d5e29ff88 100644 --- a/crates/bitwarden-crypto/src/service/key_store/rust_impl.rs +++ b/crates/bitwarden-crypto/src/service/key_store/rust_impl.rs @@ -15,7 +15,11 @@ pub(crate) struct RustImplKeyData { impl Drop for RustImplKeyData { fn drop(&mut self) { - #[cfg(all(not(target_arch = "wasm32"), not(feature = "no-memory-hardening")))] + #[cfg(all( + not(target_arch = "wasm32"), + not(feature = "no-memory-hardening"), + not(windows) + ))] { use std::mem::MaybeUninit; @@ -50,7 +54,11 @@ impl KeyData for RustImplKeyData { #[allow(unused_mut)] let mut data: Box<_> = std::iter::repeat_with(|| None).take(capacity).collect(); - #[cfg(all(not(target_arch = "wasm32"), not(feature = "no-memory-hardening")))] + #[cfg(all( + not(target_arch = "wasm32"), + not(feature = "no-memory-hardening"), + not(windows) + ))] { let entry_size = std::mem::size_of::>(); unsafe { From dd37d1d53b37090a7a0e2e0cb5a45ae82e15db2f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garci=CC=81a?= Date: Tue, 24 Sep 2024 14:56:14 +0200 Subject: [PATCH 19/34] Incorrect optional dep --- crates/bitwarden-crypto/Cargo.toml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/bitwarden-crypto/Cargo.toml b/crates/bitwarden-crypto/Cargo.toml index 2526e7ef1..d68ec46d5 100644 --- a/crates/bitwarden-crypto/Cargo.toml +++ b/crates/bitwarden-crypto/Cargo.toml @@ -17,7 +17,7 @@ keywords.workspace = true default = [] uniffi = ["dep:uniffi"] # Uniffi bindings -no-memory-hardening = ["memsec"] # Disable memory hardening features +no-memory-hardening = [] # Disable memory hardening features [dependencies] aes = { version = ">=0.8.2, <0.9", features = ["zeroize"] } @@ -47,7 +47,7 @@ uuid = { version = ">=1.3.3, <2.0", features = ["serde"] } zeroize = { version = ">=1.7.0, <2.0", features = ["derive", "aarch64"] } [target.'cfg(all(not(target_arch = "wasm32"), not(windows)))'.dependencies] -memsec = { version = "0.7.0", features = ["alloc_ext"], optional = true } +memsec = { version = "0.7.0", features = ["alloc_ext"] } [dev-dependencies] criterion = { version = "0.5.1", features = ["html_reports"] } From 45f3d3283c81f1a22496a6916305f004609cebbe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garci=CC=81a?= Date: Tue, 24 Sep 2024 17:59:25 +0200 Subject: [PATCH 20/34] Try updating windows in memsec --- Cargo.lock | 71 +------------------ crates/bitwarden-crypto/Cargo.toml | 6 +- .../src/service/key_store/rust_impl.rs | 12 +--- 3 files changed, 8 insertions(+), 81 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index b84cd5fbe..e7c04d17c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2296,12 +2296,11 @@ dependencies = [ [[package]] name = "memsec" version = "0.7.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "c797b9d6bb23aab2fc369c65f871be49214f5c759af65bde26ffaaa2b646b492" +source = "git+https://github.com/dani-garcia/memsec?rev=3d2e272d284442637bac0a7d94f76883960db7e2#3d2e272d284442637bac0a7d94f76883960db7e2" dependencies = [ "getrandom", "libc", - "windows-sys 0.45.0", + "windows-sys 0.59.0", ] [[package]] @@ -4710,15 +4709,6 @@ dependencies = [ "windows-targets 0.52.6", ] -[[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.48.0" @@ -4746,21 +4736,6 @@ dependencies = [ "windows-targets 0.52.6", ] -[[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" @@ -4792,12 +4767,6 @@ dependencies = [ "windows_x86_64_msvc 0.52.6", ] -[[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" @@ -4810,12 +4779,6 @@ version = "0.52.6" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "32a4622180e7a0ec044bb555404c800bc9fd9ec262ec147edd5989ccd0c02cd3" -[[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" @@ -4828,12 +4791,6 @@ version = "0.52.6" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "09ec2a7bb152e2252b53fa7803150007879548bc709c039df7627cabbd05d469" -[[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" @@ -4852,12 +4809,6 @@ version = "0.52.6" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "0eee52d38c090b3caa76c563b86c3a4bd71ef1a819287c19d586d7334ae8ed66" -[[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" @@ -4870,12 +4821,6 @@ version = "0.52.6" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "240948bc05c5e7c6dabba28bf89d89ffce3e303022809e73deaefe4f6ec56c66" -[[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" @@ -4888,12 +4833,6 @@ version = "0.52.6" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "147a5c80aabfbf0c7d901cb5895d1de30ef2907eb21fbbab29ca94c5b08b1a78" -[[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" @@ -4906,12 +4845,6 @@ version = "0.52.6" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "24d5b23dc417412679681396f2b49f3de8c1473deb516bd34410872eff51ed0d" -[[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" diff --git a/crates/bitwarden-crypto/Cargo.toml b/crates/bitwarden-crypto/Cargo.toml index d68ec46d5..cad61c047 100644 --- a/crates/bitwarden-crypto/Cargo.toml +++ b/crates/bitwarden-crypto/Cargo.toml @@ -46,8 +46,10 @@ uniffi = { version = "=0.28.1", optional = true } uuid = { version = ">=1.3.3, <2.0", features = ["serde"] } zeroize = { version = ">=1.7.0, <2.0", features = ["derive", "aarch64"] } -[target.'cfg(all(not(target_arch = "wasm32"), not(windows)))'.dependencies] -memsec = { version = "0.7.0", features = ["alloc_ext"] } +[target.'cfg(not(target_arch = "wasm32"))'.dependencies] +memsec = { version = "0.7.0", features = [ + "alloc_ext", +], git = "https://github.com/dani-garcia/memsec", rev = "3d2e272d284442637bac0a7d94f76883960db7e2" } [dev-dependencies] criterion = { version = "0.5.1", features = ["html_reports"] } diff --git a/crates/bitwarden-crypto/src/service/key_store/rust_impl.rs b/crates/bitwarden-crypto/src/service/key_store/rust_impl.rs index d5e29ff88..ee79edd10 100644 --- a/crates/bitwarden-crypto/src/service/key_store/rust_impl.rs +++ b/crates/bitwarden-crypto/src/service/key_store/rust_impl.rs @@ -15,11 +15,7 @@ pub(crate) struct RustImplKeyData { impl Drop for RustImplKeyData { fn drop(&mut self) { - #[cfg(all( - not(target_arch = "wasm32"), - not(feature = "no-memory-hardening"), - not(windows) - ))] + #[cfg(all(not(target_arch = "wasm32"), not(feature = "no-memory-hardening")))] { use std::mem::MaybeUninit; @@ -54,11 +50,7 @@ impl KeyData for RustImplKeyData { #[allow(unused_mut)] let mut data: Box<_> = std::iter::repeat_with(|| None).take(capacity).collect(); - #[cfg(all( - not(target_arch = "wasm32"), - not(feature = "no-memory-hardening"), - not(windows) - ))] + #[cfg(all(not(target_arch = "wasm32"), not(feature = "no-memory-hardening")))] { let entry_size = std::mem::size_of::>(); unsafe { From 1f7016941c4cea7f8a4fe68a10e53841a9a7fe8f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garci=CC=81a?= Date: Wed, 25 Sep 2024 13:06:40 +0200 Subject: [PATCH 21/34] Remove unnecessary bound --- crates/bitwarden-crypto/src/service/key_ref.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bitwarden-crypto/src/service/key_ref.rs b/crates/bitwarden-crypto/src/service/key_ref.rs index 2b08f40a0..9faf1a470 100644 --- a/crates/bitwarden-crypto/src/service/key_ref.rs +++ b/crates/bitwarden-crypto/src/service/key_ref.rs @@ -11,7 +11,7 @@ use crate::{AsymmetricCryptoKey, CryptoKey, SymmetricCryptoKey}; pub trait KeyRef: Debug + Clone + Copy + Hash + Eq + PartialEq + Ord + PartialOrd + Send + Sync + 'static { - type KeyValue: Debug + CryptoKey + Send + Sync + ZeroizeOnDrop; + type KeyValue: CryptoKey + Send + Sync + ZeroizeOnDrop; /// Returns whether the key is local to the current context or shared globally by the service. fn is_local(&self) -> bool; From 22a8b1745c751f5f240917673f968f4001558c3e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garci=CC=81a?= Date: Wed, 25 Sep 2024 15:05:04 +0200 Subject: [PATCH 22/34] Move store impl around a bit --- .../linux_memfd_secret.rs} | 2 +- .../service/key_store/implementation/mod.rs | 15 ++++ .../rust_slice.rs} | 4 +- .../src/service/key_store/mod.rs | 21 ++--- .../service/key_store/{util.rs => slice.rs} | 76 ++++++------------- 5 files changed, 45 insertions(+), 73 deletions(-) rename crates/bitwarden-crypto/src/service/key_store/{linux_memfd_secret_impl.rs => implementation/linux_memfd_secret.rs} (99%) create mode 100644 crates/bitwarden-crypto/src/service/key_store/implementation/mod.rs rename crates/bitwarden-crypto/src/service/key_store/{rust_impl.rs => implementation/rust_slice.rs} (96%) rename crates/bitwarden-crypto/src/service/key_store/{util.rs => slice.rs} (92%) diff --git a/crates/bitwarden-crypto/src/service/key_store/linux_memfd_secret_impl.rs b/crates/bitwarden-crypto/src/service/key_store/implementation/linux_memfd_secret.rs similarity index 99% rename from crates/bitwarden-crypto/src/service/key_store/linux_memfd_secret_impl.rs rename to crates/bitwarden-crypto/src/service/key_store/implementation/linux_memfd_secret.rs index 33fe8bebc..49e08c0ac 100644 --- a/crates/bitwarden-crypto/src/service/key_store/linux_memfd_secret_impl.rs +++ b/crates/bitwarden-crypto/src/service/key_store/implementation/linux_memfd_secret.rs @@ -1,7 +1,7 @@ use std::{mem::MaybeUninit, ptr::NonNull, sync::OnceLock}; use super::{ - util::{KeyData, SliceKeyStore}, + slice::{KeyData, SliceKeyStore}, KeyRef, }; diff --git a/crates/bitwarden-crypto/src/service/key_store/implementation/mod.rs b/crates/bitwarden-crypto/src/service/key_store/implementation/mod.rs new file mode 100644 index 000000000..d5b21ac21 --- /dev/null +++ b/crates/bitwarden-crypto/src/service/key_store/implementation/mod.rs @@ -0,0 +1,15 @@ +use super::{slice, KeyStore}; +use crate::service::KeyRef; + +#[cfg(all(target_os = "linux", not(feature = "no-memory-hardening")))] +pub(crate) mod linux_memfd_secret; +pub(crate) mod rust_slice; + +pub(crate) fn create_key_store() -> Box> { + #[cfg(all(target_os = "linux", not(feature = "no-memory-hardening")))] + if let Some(key_store) = linux_memfd_secret::LinuxMemfdSecretKeyStore::::new() { + return Box::new(key_store); + } + + Box::new(rust_slice::RustKeyStore::new().expect("RustKeyStore should always be available")) +} diff --git a/crates/bitwarden-crypto/src/service/key_store/rust_impl.rs b/crates/bitwarden-crypto/src/service/key_store/implementation/rust_slice.rs similarity index 96% rename from crates/bitwarden-crypto/src/service/key_store/rust_impl.rs rename to crates/bitwarden-crypto/src/service/key_store/implementation/rust_slice.rs index ee79edd10..e349a7703 100644 --- a/crates/bitwarden-crypto/src/service/key_store/rust_impl.rs +++ b/crates/bitwarden-crypto/src/service/key_store/implementation/rust_slice.rs @@ -1,5 +1,5 @@ use super::{ - util::{KeyData, SliceKeyStore}, + slice::{KeyData, SliceKeyStore}, KeyRef, }; @@ -72,7 +72,7 @@ impl KeyData for RustImplKeyData { #[cfg(test)] mod tests { use super::*; - use crate::service::key_store::{util::tests::*, KeyStore as _}; + use crate::service::key_store::{slice::tests::*, KeyStore as _}; #[test] fn test_resize() { diff --git a/crates/bitwarden-crypto/src/service/key_store/mod.rs b/crates/bitwarden-crypto/src/service/key_store/mod.rs index f5bef9ed3..bd9d2cf87 100644 --- a/crates/bitwarden-crypto/src/service/key_store/mod.rs +++ b/crates/bitwarden-crypto/src/service/key_store/mod.rs @@ -2,22 +2,13 @@ use zeroize::ZeroizeOnDrop; use crate::service::KeyRef; -#[cfg(all(target_os = "linux", not(feature = "no-memory-hardening")))] -mod linux_memfd_secret_impl; -mod rust_impl; -mod util; +mod implementation; +mod slice; -pub(crate) fn create_key_store() -> Box> { - #[cfg(all(target_os = "linux", not(feature = "no-memory-hardening")))] - if let Some(key_store) = linux_memfd_secret_impl::LinuxMemfdSecretKeyStore::::new() { - return Box::new(key_store); - } +pub(crate) use implementation::create_key_store; - Box::new(rust_impl::RustKeyStore::new().expect("RustKeyStore should always be available")) -} - -/// This trait represents a platform that can securely store and return keys. The `RustKeyStore` -/// implementation is a simple in-memory store without any security guarantees. Other +/// This trait represents a platform that can securely store and return keys. The `SliceKeyStore` +/// implementation is a simple in-memory store with some platform-specific security features. Other /// implementations could use secure enclaves or HSMs, or OS provided keychains. #[allow(dead_code)] pub(crate) trait KeyStore: ZeroizeOnDrop + Send + Sync { @@ -28,5 +19,3 @@ pub(crate) trait KeyStore: ZeroizeOnDrop + Send + Sync { fn retain(&mut self, f: fn(Key) -> bool); } - -fn _ensure_that_trait_is_object_safe(_: Box>) {} diff --git a/crates/bitwarden-crypto/src/service/key_store/util.rs b/crates/bitwarden-crypto/src/service/key_store/slice.rs similarity index 92% rename from crates/bitwarden-crypto/src/service/key_store/util.rs rename to crates/bitwarden-crypto/src/service/key_store/slice.rs index a8bc2f41a..36a941123 100644 --- a/crates/bitwarden-crypto/src/service/key_store/util.rs +++ b/crates/bitwarden-crypto/src/service/key_store/slice.rs @@ -100,7 +100,8 @@ impl> KeyStore for SliceKeyStore } fn clear(&mut self) { - self.get_key_data_mut().fill_with(|| None); + let len = self.length; + self.get_key_data_mut()[0..len].fill_with(|| None); self.length = 0; } @@ -176,8 +177,12 @@ impl> SliceKeyStore { fn check_capacity(&self, new_elements: usize) -> Result<(), usize> { let new_size = self.length + new_elements; - // This is the first allocation - if self.capacity == 0 { + // 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::>(); @@ -187,16 +192,12 @@ impl> SliceKeyStore { Err(entries_per_page) // We need to resize the container - } else if new_size > self.capacity { + } 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) - - // We still have enough capacity - } else { - Ok(()) } } @@ -271,7 +272,10 @@ pub(crate) mod tests { use zeroize::Zeroize; use super::*; - use crate::{service::key_ref::KeyRef, CryptoKey}; + use crate::{ + service::{key_ref::KeyRef, key_store::implementation::rust_slice::RustKeyStore}, + CryptoKey, + }; #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] pub enum TestKey { @@ -307,37 +311,9 @@ pub(crate) mod tests { } } - pub struct TestKeyContainer { - keys: Vec>, - } - impl Drop for TestKeyContainer { - fn drop(&mut self) {} - } - - impl KeyData for TestKeyContainer { - fn is_available() -> bool { - true - } - - fn with_capacity(capacity: usize) -> Self { - Self { - keys: std::iter::repeat_with(|| None).take(capacity).collect(), - } - } - - fn get_key_data(&self) -> &[Option<(Key, Key::KeyValue)>] { - self.keys.as_slice() - } - - fn get_key_data_mut(&mut self) -> &mut [Option<(Key, Key::KeyValue)>] { - self.keys.as_mut_slice() - } - } - #[test] fn test_slice_container_insertion() { - let mut container = - SliceKeyStore::>::with_capacity(5).unwrap(); + let mut container = RustKeyStore::::with_capacity(5).unwrap(); assert_eq!(container.get_key_data(), [None, None, None, None, None]); @@ -448,8 +424,7 @@ pub(crate) mod tests { #[test] fn test_slice_container_get() { - let mut container = - SliceKeyStore::>::with_capacity(5).unwrap(); + let mut container = RustKeyStore::::with_capacity(5).unwrap(); for (key, value) in [ (TestKey::A, TestKeyValue::new(1)), @@ -467,8 +442,7 @@ pub(crate) mod tests { #[test] fn test_slice_container_clear() { - let mut container = - SliceKeyStore::>::with_capacity(5).unwrap(); + let mut container = RustKeyStore::::with_capacity(5).unwrap(); for (key, value) in [ (TestKey::A, TestKeyValue::new(1)), @@ -498,8 +472,7 @@ pub(crate) mod tests { #[test] fn test_slice_container_ensure_capacity() { - let mut container = - SliceKeyStore::>::with_capacity(5).unwrap(); + let mut container = RustKeyStore::::with_capacity(5).unwrap(); assert_eq!(container.capacity, 5); assert_eq!(container.length, 0); @@ -529,8 +502,7 @@ pub(crate) mod tests { #[test] fn test_slice_container_removal() { - let mut container = - SliceKeyStore::>::with_capacity(5).unwrap(); + let mut container = RustKeyStore::::with_capacity(5).unwrap(); for (key, value) in [ (TestKey::A, TestKeyValue::new(1)), @@ -616,8 +588,7 @@ pub(crate) mod tests { #[test] fn test_slice_container_retain_removes_one() { - let mut container = - SliceKeyStore::>::with_capacity(5).unwrap(); + let mut container = RustKeyStore::::with_capacity(5).unwrap(); for (key, value) in [ (TestKey::A, TestKeyValue::new(1)), @@ -703,8 +674,7 @@ pub(crate) mod tests { #[test] fn test_slice_container_retain_removes_none() { - let mut container = - SliceKeyStore::>::with_capacity(5).unwrap(); + let mut container = RustKeyStore::::with_capacity(5).unwrap(); for (key, value) in [ (TestKey::A, TestKeyValue::new(1)), @@ -731,8 +701,7 @@ pub(crate) mod tests { #[test] fn test_slice_container_retain_removes_some() { - let mut container = - SliceKeyStore::>::with_capacity(5).unwrap(); + let mut container = RustKeyStore::::with_capacity(5).unwrap(); for (key, value) in [ (TestKey::A, TestKeyValue::new(1)), @@ -759,8 +728,7 @@ pub(crate) mod tests { #[test] fn test_slice_container_retain_removes_all() { - let mut container = - SliceKeyStore::>::with_capacity(5).unwrap(); + let mut container = RustKeyStore::::with_capacity(5).unwrap(); for (key, value) in [ (TestKey::A, TestKeyValue::new(1)), From c63f656a572d4fd0ea8dbbc1e81f4fd4368545c3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garci=CC=81a?= Date: Wed, 25 Sep 2024 15:57:23 +0200 Subject: [PATCH 23/34] Make KeyStore pub --- .../src/service/key_store/implementation/mod.rs | 2 +- crates/bitwarden-crypto/src/service/key_store/mod.rs | 5 ++--- crates/bitwarden-crypto/src/service/mod.rs | 3 ++- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/crates/bitwarden-crypto/src/service/key_store/implementation/mod.rs b/crates/bitwarden-crypto/src/service/key_store/implementation/mod.rs index d5b21ac21..6ecff0cdd 100644 --- a/crates/bitwarden-crypto/src/service/key_store/implementation/mod.rs +++ b/crates/bitwarden-crypto/src/service/key_store/implementation/mod.rs @@ -5,7 +5,7 @@ use crate::service::KeyRef; pub(crate) mod linux_memfd_secret; pub(crate) mod rust_slice; -pub(crate) fn create_key_store() -> Box> { +pub fn create_key_store() -> Box> { #[cfg(all(target_os = "linux", not(feature = "no-memory-hardening")))] if let Some(key_store) = linux_memfd_secret::LinuxMemfdSecretKeyStore::::new() { return Box::new(key_store); diff --git a/crates/bitwarden-crypto/src/service/key_store/mod.rs b/crates/bitwarden-crypto/src/service/key_store/mod.rs index bd9d2cf87..e0d762926 100644 --- a/crates/bitwarden-crypto/src/service/key_store/mod.rs +++ b/crates/bitwarden-crypto/src/service/key_store/mod.rs @@ -5,13 +5,12 @@ use crate::service::KeyRef; mod implementation; mod slice; -pub(crate) use implementation::create_key_store; +pub use implementation::create_key_store; /// This trait represents a platform that can securely store and return keys. The `SliceKeyStore` /// implementation is a simple in-memory store with some platform-specific security features. Other /// implementations could use secure enclaves or HSMs, or OS provided keychains. -#[allow(dead_code)] -pub(crate) trait KeyStore: ZeroizeOnDrop + Send + Sync { +pub trait KeyStore: ZeroizeOnDrop + Send + Sync { fn insert(&mut self, key_ref: Key, key: Key::KeyValue); fn get(&self, key_ref: Key) -> Option<&Key::KeyValue>; fn remove(&mut self, key_ref: Key); diff --git a/crates/bitwarden-crypto/src/service/mod.rs b/crates/bitwarden-crypto/src/service/mod.rs index 3906b9301..423bc9a4f 100644 --- a/crates/bitwarden-crypto/src/service/mod.rs +++ b/crates/bitwarden-crypto/src/service/mod.rs @@ -10,7 +10,8 @@ mod key_store; use context::RustCryptoServiceContext; pub use encryptable::{Decryptable, Encryptable, KeyProvided, KeyProvidedExt, UsesKey}; use key_ref::{AsymmetricKeyRef, KeyRef, SymmetricKeyRef}; -use key_store::{create_key_store, KeyStore}; +pub use key_store::create_key_store; +use key_store::KeyStore; #[derive(Clone)] pub struct CryptoService { From eb816846fce893f95b006f6f317398deacf97f7f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garci=CC=81a?= Date: Wed, 2 Oct 2024 19:49:09 +0200 Subject: [PATCH 24/34] Some renames/simplifications --- crates/bitwarden-core/src/lib.rs | 8 +- .../benches/new_encryptable.rs | 2 +- .../bitwarden-crypto/src/service/context.rs | 146 +++++++++--------- .../src/service/encryptable.rs | 29 ++-- .../bitwarden-crypto/src/service/key_ref.rs | 44 +++--- .../src/service/key_store/slice.rs | 9 +- crates/bitwarden-crypto/src/service/mod.rs | 52 ++----- 7 files changed, 131 insertions(+), 159 deletions(-) diff --git a/crates/bitwarden-core/src/lib.rs b/crates/bitwarden-core/src/lib.rs index 17966e61f..bafb793da 100644 --- a/crates/bitwarden-core/src/lib.rs +++ b/crates/bitwarden-core/src/lib.rs @@ -82,9 +82,7 @@ mod testcrypto { key: MySymmKeyRef, ) -> Result { let cipher_key = match &self.key { - Some(cipher_key) => { - ctx.decrypt_and_store_symmetric_key(key, CIPHER_KEY, cipher_key)? - } + Some(cipher_key) => ctx.decrypt_symmetric_key(key, CIPHER_KEY, cipher_key)?, None => key, }; @@ -102,9 +100,7 @@ mod testcrypto { key: MySymmKeyRef, ) -> Result { let cipher_key = match &self.key { - Some(cipher_key) => { - ctx.decrypt_and_store_symmetric_key(key, CIPHER_KEY, cipher_key)? - } + Some(cipher_key) => ctx.decrypt_symmetric_key(key, CIPHER_KEY, cipher_key)?, None => key, }; diff --git a/crates/bitwarden-crypto/benches/new_encryptable.rs b/crates/bitwarden-crypto/benches/new_encryptable.rs index 892bb5ea9..3bd43f75c 100644 --- a/crates/bitwarden-crypto/benches/new_encryptable.rs +++ b/crates/bitwarden-crypto/benches/new_encryptable.rs @@ -146,7 +146,7 @@ impl Encryptable for CipherVi key: MySymmKeyRef, ) -> Result { let cipher_key = match &self.key { - Some(cipher_key) => ctx.decrypt_and_store_symmetric_key(key, CIPHER_KEY, cipher_key)?, + Some(cipher_key) => ctx.decrypt_symmetric_key(key, CIPHER_KEY, cipher_key)?, None => key, }; diff --git a/crates/bitwarden-crypto/src/service/context.rs b/crates/bitwarden-crypto/src/service/context.rs index 61b3b9fab..9ea2c8d10 100644 --- a/crates/bitwarden-crypto/src/service/context.rs +++ b/crates/bitwarden-crypto/src/service/context.rs @@ -8,23 +8,78 @@ use crate::{ AsymmetricCryptoKey, AsymmetricEncString, CryptoError, EncString, SymmetricCryptoKey, }; -pub(crate) struct RustCryptoServiceContext< - 'a, - SymmKeyRef: SymmetricKeyRef, - AsymmKeyRef: AsymmetricKeyRef, -> { +pub struct CryptoServiceContext<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> { // We hold a RwLock read guard to avoid having any nested //calls locking it again and potentially causing a deadlock - pub(crate) global_keys: RwLockReadGuard<'a, RustCryptoServiceKeys>, + pub(super) global_keys: RwLockReadGuard<'a, RustCryptoServiceKeys>, - pub(crate) local_symmetric_keys: Box>, - pub(crate) local_asymmetric_keys: Box>, + pub(super) local_symmetric_keys: Box>, + pub(super) local_asymmetric_keys: Box>, } -#[allow(unused)] impl<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> - RustCryptoServiceContext<'a, SymmKeyRef, AsymmKeyRef> + CryptoServiceContext<'a, SymmKeyRef, AsymmKeyRef> { + pub fn clear(&mut self) { + self.local_symmetric_keys.clear(); + self.local_asymmetric_keys.clear(); + } + + pub fn decrypt_symmetric_key( + &mut self, + encryption_key: SymmKeyRef, + new_key_ref: SymmKeyRef, + encrypted_key: &EncString, + ) -> Result { + let mut new_key_material = + self.decrypt_data_with_symmetric_key(encryption_key, encrypted_key)?; + + let new_key = SymmetricCryptoKey::try_from(new_key_material.as_mut_slice())?; + self.local_symmetric_keys.insert(new_key_ref, new_key); + + // Returning the new key reference for convenience + Ok(new_key_ref) + } + + pub fn encrypt_symmetric_key( + &self, + encryption_key: SymmKeyRef, + key_to_encrypt: SymmKeyRef, + ) -> Result { + let key_to_encrypt = self.get_symmetric_key(key_to_encrypt)?; + self.encrypt_data_with_symmetric_key(encryption_key, &key_to_encrypt.to_vec()) + } + + pub fn decrypt_asymmetric_key( + &mut self, + encryption_key: AsymmKeyRef, + new_key_ref: AsymmKeyRef, + encrypted_key: &AsymmetricEncString, + ) -> Result { + let new_key_material = + self.decrypt_data_with_asymmetric_key(encryption_key, encrypted_key)?; + + let new_key = AsymmetricCryptoKey::from_der(&new_key_material)?; + self.local_asymmetric_keys.insert(new_key_ref, new_key); + + // Returning the new key reference for convenience + Ok(new_key_ref) + } + + pub fn encrypt_asymmetric_key( + &self, + encryption_key: AsymmKeyRef, + key_to_encrypt: AsymmKeyRef, + ) -> Result { + let encryption_key = self.get_asymmetric_key(encryption_key)?; + let key_to_encrypt = self.get_asymmetric_key(key_to_encrypt)?; + + AsymmetricEncString::encrypt_rsa2048_oaep_sha1( + key_to_encrypt.to_der()?.as_slice(), + encryption_key, + ) + } + fn get_symmetric_key( &self, key_ref: SymmKeyRef, @@ -49,16 +104,7 @@ impl<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> .ok_or_else(|| crate::CryptoError::MissingKey2(format!("{key_ref:?}"))) } - pub fn clear(&mut self) { - self.local_symmetric_keys.clear(); - self.local_asymmetric_keys.clear(); - } - - pub fn remove_symmetric_key(&mut self, key_ref: SymmKeyRef) { - self.local_symmetric_keys.remove(key_ref); - } - - pub fn decrypt_data_with_symmetric_key( + pub(super) fn decrypt_data_with_symmetric_key( &self, key: SymmKeyRef, data: &EncString, @@ -89,21 +135,7 @@ impl<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> } } - pub fn decrypt_and_store_symmetric_key( - &mut self, - encryption_key: SymmKeyRef, - new_key_ref: SymmKeyRef, - encrypted_key: &EncString, - ) -> Result<(), crate::CryptoError> { - let mut new_key_material = - self.decrypt_data_with_symmetric_key(encryption_key, encrypted_key)?; - - let new_key = SymmetricCryptoKey::try_from(new_key_material.as_mut_slice())?; - self.local_symmetric_keys.insert(new_key_ref, new_key); - Ok(()) - } - - pub fn encrypt_data_with_symmetric_key( + pub(super) fn encrypt_data_with_symmetric_key( &self, key: SymmKeyRef, data: &[u8], @@ -116,20 +148,7 @@ impl<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> ) } - pub fn encrypt_symmetric_key( - &self, - encryption_key: SymmKeyRef, - key_to_encrypt: SymmKeyRef, - ) -> Result { - let key_to_encrypt = self.get_symmetric_key(key_to_encrypt)?; - self.encrypt_data_with_symmetric_key(encryption_key, &key_to_encrypt.to_vec()) - } - - pub fn remove_asymmetric_key(&mut self, key_ref: AsymmKeyRef) { - self.local_asymmetric_keys.remove(key_ref); - } - - pub fn decrypt_data_with_asymmetric_key( + pub(super) fn decrypt_data_with_asymmetric_key( &self, key: AsymmKeyRef, data: &AsymmetricEncString, @@ -152,20 +171,7 @@ impl<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> .map_err(|_| CryptoError::KeyDecrypt) } - pub fn decrypt_and_store_asymmetric_key( - &mut self, - encryption_key: AsymmKeyRef, - new_key_ref: AsymmKeyRef, - encrypted_key: &AsymmetricEncString, - ) -> Result<(), crate::CryptoError> { - let new_key_material = - self.decrypt_data_with_asymmetric_key(encryption_key, encrypted_key)?; - - let new_key = AsymmetricCryptoKey::from_der(&new_key_material)?; - self.local_asymmetric_keys.insert(new_key_ref, new_key); - Ok(()) - } - pub fn encrypt_data_with_asymmetric_key( + pub(super) fn encrypt_data_with_asymmetric_key( &self, key: AsymmKeyRef, data: &[u8], @@ -173,18 +179,4 @@ impl<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> let key = self.get_asymmetric_key(key)?; AsymmetricEncString::encrypt_rsa2048_oaep_sha1(data, key) } - - pub fn encrypt_asymmetric_key( - &self, - encryption_key: AsymmKeyRef, - key_to_encrypt: AsymmKeyRef, - ) -> Result { - let encryption_key = self.get_asymmetric_key(encryption_key)?; - let key_to_encrypt = self.get_asymmetric_key(key_to_encrypt)?; - - AsymmetricEncString::encrypt_rsa2048_oaep_sha1( - key_to_encrypt.to_der()?.as_slice(), - encryption_key, - ) - } } diff --git a/crates/bitwarden-crypto/src/service/encryptable.rs b/crates/bitwarden-crypto/src/service/encryptable.rs index 8bd36d0d4..4fa23618c 100644 --- a/crates/bitwarden-crypto/src/service/encryptable.rs +++ b/crates/bitwarden-crypto/src/service/encryptable.rs @@ -12,19 +12,20 @@ pub trait UsesKey { fn uses_key(&self) -> Key; } -// This extension trait allows any type to be wrapped with `KeyProvided` -// to make it easy to encrypt/decrypt it with the desired key -pub trait KeyProvidedExt: Sized { - fn using_key(self, key: Key) -> KeyProvided { - KeyProvided { key, value: self } +// This extension trait allows any type to be wrapped with `UsingKey` +// to make it easy to encrypt/decrypt it with the desired key, +// this way we don't need to have a separate `encrypt_with_key` function +pub trait UsingKeyExt: Sized { + fn using_key(self, key: Key) -> UsingKey { + UsingKey { key, value: self } } } -impl KeyProvidedExt for T {} -pub struct KeyProvided { +impl UsingKeyExt for T {} +pub struct UsingKey { key: Key, value: T, } -impl UsesKey for KeyProvided { +impl UsesKey for UsingKey { fn uses_key(&self) -> Key { self.key } @@ -35,7 +36,7 @@ impl< Key: KeyRef, T: Encryptable, Output, - > Encryptable for KeyProvided + > Encryptable for UsingKey { fn encrypt( &self, @@ -51,7 +52,7 @@ impl< Key: KeyRef, T: Decryptable, Output, - > Decryptable for KeyProvided + > Decryptable for UsingKey { fn decrypt( &self, @@ -100,7 +101,7 @@ impl ctx: &mut CryptoServiceContext, key: SymmKeyRef, ) -> Result, crate::CryptoError> { - ctx.engine.decrypt_data_with_symmetric_key(key, self) + ctx.decrypt_data_with_symmetric_key(key, self) } } @@ -112,7 +113,7 @@ impl ctx: &mut CryptoServiceContext, key: AsymmKeyRef, ) -> Result, crate::CryptoError> { - ctx.engine.decrypt_data_with_asymmetric_key(key, self) + ctx.decrypt_data_with_asymmetric_key(key, self) } } @@ -124,7 +125,7 @@ impl ctx: &mut CryptoServiceContext, key: SymmKeyRef, ) -> Result { - ctx.engine.encrypt_data_with_symmetric_key(key, self) + ctx.encrypt_data_with_symmetric_key(key, self) } } @@ -136,7 +137,7 @@ impl ctx: &mut CryptoServiceContext, key: AsymmKeyRef, ) -> Result { - ctx.engine.encrypt_data_with_asymmetric_key(key, self) + ctx.encrypt_data_with_asymmetric_key(key, self) } } diff --git a/crates/bitwarden-crypto/src/service/key_ref.rs b/crates/bitwarden-crypto/src/service/key_ref.rs index 9faf1a470..5c4cd307b 100644 --- a/crates/bitwarden-crypto/src/service/key_ref.rs +++ b/crates/bitwarden-crypto/src/service/key_ref.rs @@ -1,21 +1,29 @@ -use std::{fmt::Debug, hash::Hash}; +use crate::{AsymmetricCryptoKey, SymmetricCryptoKey}; -use zeroize::ZeroizeOnDrop; +// Hide the `KeyRef` trait from the public API, to avoid confusion +// the trait itself needs to be public to reference it in the macro, so wrap it in a hidden module +#[doc(hidden)] +pub mod __internal { + use std::{fmt::Debug, hash::Hash}; -use crate::{AsymmetricCryptoKey, CryptoKey, SymmetricCryptoKey}; + use zeroize::ZeroizeOnDrop; -/// This trait represents a key reference that can be used to identify cryptographic keys in the key -/// store. It is used to abstract over the different types of keys that can be used in the system, -/// an end user would not implement this trait directly, and would instead use the `SymmetricKeyRef` -/// and `AsymmetricKeyRef` traits. -pub trait KeyRef: - Debug + Clone + Copy + Hash + Eq + PartialEq + Ord + PartialOrd + Send + Sync + 'static -{ - type KeyValue: CryptoKey + Send + Sync + ZeroizeOnDrop; + use crate::CryptoKey; - /// Returns whether the key is local to the current context or shared globally by the service. - fn is_local(&self) -> bool; + /// This trait represents a key reference that can be used to identify cryptographic keys in the key + /// store. It is used to abstract over the different types of keys that can be used in the system, + /// an end user would not implement this trait directly, and would instead use the `SymmetricKeyRef` + /// and `AsymmetricKeyRef` traits. + pub trait KeyRef: + Debug + Clone + Copy + Hash + Eq + PartialEq + Ord + PartialOrd + Send + Sync + 'static + { + type KeyValue: CryptoKey + Send + Sync + ZeroizeOnDrop; + + /// Returns whether the key is local to the current context or shared globally by the service. + fn is_local(&self) -> bool; + } } +pub(crate) use __internal::KeyRef; // These traits below are just basic aliases of the `KeyRef` trait, but they allow us to have two // separate trait bounds @@ -23,13 +31,7 @@ pub trait KeyRef: pub trait SymmetricKeyRef: KeyRef {} pub trait AsymmetricKeyRef: KeyRef {} -// Hide the `KeyRef` trait from the public API, to avoid confusion -#[doc(hidden)] -pub mod __macro_internal { - pub use super::KeyRef; -} - -// Just a test of a derive_like macro that can be used to generate the key reference enums. +// Just a small derive_like macro that can be used to generate the key reference enums. // Example usage: // ```rust // key_refs! { @@ -57,7 +59,7 @@ macro_rules! key_refs { $variant $( ($inner) )? ,)+ } - impl $crate::service::key_ref::__macro_internal::KeyRef for $name { + impl $crate::service::key_ref::__internal::KeyRef for $name { type KeyValue = key_refs!(@key_type $meta_type); fn is_local(&self) -> bool { diff --git a/crates/bitwarden-crypto/src/service/key_store/slice.rs b/crates/bitwarden-crypto/src/service/key_store/slice.rs index 36a941123..1bcc688a5 100644 --- a/crates/bitwarden-crypto/src/service/key_store/slice.rs +++ b/crates/bitwarden-crypto/src/service/key_store/slice.rs @@ -140,17 +140,16 @@ impl> KeyStore for SliceKeyStore } } -#[allow(dead_code)] impl> SliceKeyStore { - pub(crate) fn is_available() -> bool { - Data::is_available() - } - 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. if capacity == 0 { diff --git a/crates/bitwarden-crypto/src/service/mod.rs b/crates/bitwarden-crypto/src/service/mod.rs index 423bc9a4f..986870d43 100644 --- a/crates/bitwarden-crypto/src/service/mod.rs +++ b/crates/bitwarden-crypto/src/service/mod.rs @@ -1,14 +1,14 @@ use std::sync::{Arc, RwLock}; -use crate::{EncString, SymmetricCryptoKey}; +use crate::{AsymmetricCryptoKey, SymmetricCryptoKey}; mod context; mod encryptable; pub mod key_ref; mod key_store; -use context::RustCryptoServiceContext; -pub use encryptable::{Decryptable, Encryptable, KeyProvided, KeyProvidedExt, UsesKey}; +pub use context::CryptoServiceContext; +pub use encryptable::{Decryptable, Encryptable, UsingKey, UsingKeyExt, UsesKey}; use key_ref::{AsymmetricKeyRef, KeyRef, SymmetricKeyRef}; pub use key_store::create_key_store; use key_store::KeyStore; @@ -21,7 +21,8 @@ pub struct CryptoService { +pub(crate) struct RustCryptoServiceKeys +{ symmetric_keys: Box>, asymmetric_keys: Box>, } @@ -54,15 +55,21 @@ impl .insert(key_ref, key); } + #[deprecated(note = "We should be generating/decrypting the keys into the service directly")] + pub fn insert_asymmetric_key(&self, key_ref: AsymmKeyRef, key: AsymmetricCryptoKey) { + self.key_stores + .write() + .expect("RwLock is poisoned") + .asymmetric_keys + .insert(key_ref, key); + } + // TODO: Do we want this to be public? pub(crate) fn context(&'_ self) -> CryptoServiceContext<'_, SymmKeyRef, AsymmKeyRef> { CryptoServiceContext { - // TODO: Cache these?, or maybe initialize them lazily? or both? - engine: RustCryptoServiceContext { - global_keys: self.key_stores.read().expect("RwLock is poisoned"), - local_symmetric_keys: create_key_store(), - local_asymmetric_keys: create_key_store(), - }, + global_keys: self.key_stores.read().expect("RwLock is poisoned"), + local_symmetric_keys: create_key_store(), + local_asymmetric_keys: create_key_store(), } } @@ -128,28 +135,3 @@ impl res } } - -pub struct CryptoServiceContext<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> { - engine: RustCryptoServiceContext<'a, SymmKeyRef, AsymmKeyRef>, -} - -impl<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> - CryptoServiceContext<'a, SymmKeyRef, AsymmKeyRef> -{ - /// Decrypt a key and store it in the local key store - pub fn decrypt_and_store_symmetric_key( - &mut self, - encryption_key: SymmKeyRef, - new_key_ref: SymmKeyRef, - encrypted_key: &EncString, - ) -> Result { - self.engine - .decrypt_and_store_symmetric_key(encryption_key, new_key_ref, encrypted_key)?; - // This is returned for convenience - Ok(new_key_ref) - } - - pub fn clear(&mut self) { - self.engine.clear(); - } -} From d2796269c91f0da8e464d81352a33639433449b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garci=CC=81a?= Date: Wed, 2 Oct 2024 20:17:09 +0200 Subject: [PATCH 25/34] Add some missing implementations --- .../bitwarden-crypto/src/service/context.rs | 16 +++++ crates/bitwarden-crypto/src/service/mod.rs | 68 ++++++++++++++++++- 2 files changed, 81 insertions(+), 3 deletions(-) diff --git a/crates/bitwarden-crypto/src/service/context.rs b/crates/bitwarden-crypto/src/service/context.rs index 9ea2c8d10..f0a3d3bec 100644 --- a/crates/bitwarden-crypto/src/service/context.rs +++ b/crates/bitwarden-crypto/src/service/context.rs @@ -80,6 +80,22 @@ impl<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> ) } + #[deprecated(note = "This function should never be used outside this crate")] + pub fn dangerous_get_symmetric_key( + &self, + key_ref: SymmKeyRef, + ) -> Result<&SymmetricCryptoKey, crate::CryptoError> { + self.get_symmetric_key(key_ref) + } + + #[deprecated(note = "This function should never be used outside this crate")] + pub fn dangerous_get_asymmetric_key( + &self, + key_ref: AsymmKeyRef, + ) -> Result<&AsymmetricCryptoKey, crate::CryptoError> { + self.get_asymmetric_key(key_ref) + } + fn get_symmetric_key( &self, key_ref: SymmKeyRef, diff --git a/crates/bitwarden-crypto/src/service/mod.rs b/crates/bitwarden-crypto/src/service/mod.rs index 986870d43..a00ca6b8d 100644 --- a/crates/bitwarden-crypto/src/service/mod.rs +++ b/crates/bitwarden-crypto/src/service/mod.rs @@ -8,7 +8,7 @@ pub mod key_ref; mod key_store; pub use context::CryptoServiceContext; -pub use encryptable::{Decryptable, Encryptable, UsingKey, UsingKeyExt, UsesKey}; +pub use encryptable::{Decryptable, Encryptable, UsesKey, UsingKey, UsingKeyExt}; use key_ref::{AsymmetricKeyRef, KeyRef, SymmetricKeyRef}; pub use key_store::create_key_store; use key_store::KeyStore; @@ -20,6 +20,14 @@ pub struct CryptoService>>, } +impl std::fmt::Debug + for CryptoService +{ + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("CryptoService").finish() + } +} + // This is just a wrapper around the keys so we only deal with one RwLock pub(crate) struct RustCryptoServiceKeys { @@ -46,6 +54,22 @@ impl keys.asymmetric_keys.clear(); } + pub fn retain_symmetric_keys(&self, f: fn(SymmKeyRef) -> bool) { + self.key_stores + .write() + .expect("RwLock is poisoned") + .symmetric_keys + .retain(f); + } + + pub fn retain_asymmetric_keys(&self, f: fn(AsymmKeyRef) -> bool) { + self.key_stores + .write() + .expect("RwLock is poisoned") + .asymmetric_keys + .retain(f); + } + #[deprecated(note = "We should be generating/decrypting the keys into the service directly")] pub fn insert_symmetric_key(&self, key_ref: SymmKeyRef, key: SymmetricCryptoKey) { self.key_stores @@ -64,8 +88,9 @@ impl .insert(key_ref, key); } - // TODO: Do we want this to be public? - pub(crate) fn context(&'_ self) -> CryptoServiceContext<'_, SymmKeyRef, AsymmKeyRef> { + /// Initiate an encryption/decryption context. This is an advanced API, use with care. + /// Prefer to instead use `encrypt`/`decrypt`/`encrypt_list`/`decrypt_list` methods. + pub fn context(&'_ self) -> CryptoServiceContext<'_, SymmKeyRef, AsymmKeyRef> { CryptoServiceContext { global_keys: self.key_stores.read().expect("RwLock is poisoned"), local_symmetric_keys: create_key_store(), @@ -98,6 +123,43 @@ impl data.encrypt(&mut self.context(), key) } + pub fn decrypt_list< + Key: KeyRef, + Data: Decryptable + UsesKey + Send + Sync, + Output: Send + Sync, + >( + &self, + data: &[Data], + ) -> Result, crate::CryptoError> { + use rayon::prelude::*; + + // We want to split all the data between available threads, but at the + // same time we don't want to split it too much if the amount of data is small. + + // In this case, the minimum chunk size is 50. + let chunk_size = usize::max(1 + data.len() / rayon::current_num_threads(), 50); + + let res: Result, _> = data + .par_chunks(chunk_size) + .map(|chunk| { + let mut context = self.context(); + + let mut result = Vec::with_capacity(chunk.len()); + + for item in chunk { + let key = item.uses_key(); + result.push(item.decrypt(&mut context, key)); + context.clear(); + } + + result + }) + .flatten() + .collect(); + + res + } + pub fn encrypt_list< Key: KeyRef, Data: Encryptable + UsesKey + Send + Sync, From d5f1ede14ba5899ee95cc4c06ceb7dfbe37a03cc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garci=CC=81a?= Date: Thu, 3 Oct 2024 15:00:45 +0200 Subject: [PATCH 26/34] Rename --- crates/bitwarden-crypto/src/service/context.rs | 4 ++-- crates/bitwarden-crypto/src/service/mod.rs | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/crates/bitwarden-crypto/src/service/context.rs b/crates/bitwarden-crypto/src/service/context.rs index f0a3d3bec..f3e8c8877 100644 --- a/crates/bitwarden-crypto/src/service/context.rs +++ b/crates/bitwarden-crypto/src/service/context.rs @@ -2,7 +2,7 @@ use std::sync::RwLockReadGuard; use rsa::Oaep; -use super::RustCryptoServiceKeys; +use super::Keys; use crate::{ service::{key_store::KeyStore, AsymmetricKeyRef, SymmetricKeyRef}, AsymmetricCryptoKey, AsymmetricEncString, CryptoError, EncString, SymmetricCryptoKey, @@ -11,7 +11,7 @@ use crate::{ pub struct CryptoServiceContext<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> { // We hold a RwLock read guard to avoid having any nested //calls locking it again and potentially causing a deadlock - pub(super) global_keys: RwLockReadGuard<'a, RustCryptoServiceKeys>, + pub(super) global_keys: RwLockReadGuard<'a, Keys>, pub(super) local_symmetric_keys: Box>, pub(super) local_asymmetric_keys: Box>, diff --git a/crates/bitwarden-crypto/src/service/mod.rs b/crates/bitwarden-crypto/src/service/mod.rs index a00ca6b8d..793935b94 100644 --- a/crates/bitwarden-crypto/src/service/mod.rs +++ b/crates/bitwarden-crypto/src/service/mod.rs @@ -17,7 +17,7 @@ use key_store::KeyStore; pub struct CryptoService { // We use an Arc<> to make it easier to pass this service around, as we can // clone it instead of passing references - key_stores: Arc>>, + key_stores: Arc>>, } impl std::fmt::Debug @@ -29,7 +29,7 @@ impl std::fmt::Debug } // This is just a wrapper around the keys so we only deal with one RwLock -pub(crate) struct RustCryptoServiceKeys +pub(crate) struct Keys { symmetric_keys: Box>, asymmetric_keys: Box>, @@ -41,7 +41,7 @@ impl #[allow(clippy::new_without_default)] pub fn new() -> Self { Self { - key_stores: Arc::new(RwLock::new(RustCryptoServiceKeys { + key_stores: Arc::new(RwLock::new(Keys { symmetric_keys: create_key_store(), asymmetric_keys: create_key_store(), })), From 8652d794e5874ba88ab30d1f2f8f89bca7109f75 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garci=CC=81a?= Date: Thu, 3 Oct 2024 16:53:59 +0200 Subject: [PATCH 27/34] Introduce mutable context --- crates/bitwarden-core/src/lib.rs | 135 ------------- .../benches/new_encryptable.rs | 4 +- .../bitwarden-crypto/src/service/context.rs | 184 ++++++++++++++---- .../bitwarden-crypto/src/service/key_ref.rs | 11 +- crates/bitwarden-crypto/src/service/mod.rs | 49 ++++- 5 files changed, 198 insertions(+), 185 deletions(-) diff --git a/crates/bitwarden-core/src/lib.rs b/crates/bitwarden-core/src/lib.rs index bafb793da..12b0df3c3 100644 --- a/crates/bitwarden-core/src/lib.rs +++ b/crates/bitwarden-core/src/lib.rs @@ -19,138 +19,3 @@ mod util; pub use bitwarden_crypto::ZeroizingAllocator; pub use client::{Client, ClientSettings, DeviceType}; - -#[allow(warnings)] -#[cfg(test)] -mod testcrypto { - - // TEST IMPL OF THE NEW ENCRYPTABLE/DECRYPTABLE TRAITS - // Note that these never touch the keys at all, they just use the context and key references to - // encrypt/decrypt - - use bitwarden_crypto::{ - key_refs, service::*, AsymmetricCryptoKey, CryptoError, EncString, KeyEncryptable, - SymmetricCryptoKey, - }; - - key_refs! { - #[symmetric] - pub enum MySymmKeyRef { - User, - Organization(uuid::Uuid), - #[local] - Local(&'static str), - } - - #[asymmetric] - pub enum MyAsymmKeyRef { - UserPrivateKey, - #[local] - Local(&'static str), - } - } - - #[derive(Clone)] - struct Cipher { - key: Option, - name: EncString, - } - - #[derive(Clone)] - struct CipherView { - key: Option, - name: String, - } - - impl UsesKey for Cipher { - fn uses_key(&self) -> MySymmKeyRef { - MySymmKeyRef::User - } - } - impl UsesKey for CipherView { - fn uses_key(&self) -> MySymmKeyRef { - MySymmKeyRef::User - } - } - - const CIPHER_KEY: MySymmKeyRef = MySymmKeyRef::Local("cipher_key"); - - impl Encryptable for CipherView { - fn encrypt( - &self, - ctx: &mut CryptoServiceContext, - key: MySymmKeyRef, - ) -> Result { - let cipher_key = match &self.key { - Some(cipher_key) => ctx.decrypt_symmetric_key(key, CIPHER_KEY, cipher_key)?, - None => key, - }; - - Ok(Cipher { - key: self.key.clone(), - name: self.name.as_str().encrypt(ctx, cipher_key)?, - }) - } - } - - impl Decryptable for Cipher { - fn decrypt( - &self, - ctx: &mut CryptoServiceContext, - key: MySymmKeyRef, - ) -> Result { - let cipher_key = match &self.key { - Some(cipher_key) => ctx.decrypt_symmetric_key(key, CIPHER_KEY, cipher_key)?, - None => key, - }; - - Ok(CipherView { - key: self.key.clone(), - name: self.name.decrypt(ctx, cipher_key)?, - }) - } - } - - #[test] - fn test_cipher() { - let user_key = SymmetricCryptoKey::generate(rand::thread_rng()); - - let org_id = uuid::Uuid::parse_str("91b000b6-81ce-47f4-9802-3390e0b895ed").unwrap(); - let org_key = SymmetricCryptoKey::generate(rand::thread_rng()); - - let cipher_key = SymmetricCryptoKey::generate(rand::thread_rng()); - let cipher_key_user_enc = cipher_key.to_vec().encrypt_with_key(&user_key).unwrap(); - let cipher_view = CipherView { - key: Some(cipher_key_user_enc.clone()), - name: "test".to_string(), - }; - - let service: CryptoService = CryptoService::new(); - // Ideally we'd decrypt the keys directly into the service, but that's not implemented yet - #[allow(deprecated)] - { - service.insert_symmetric_key(MySymmKeyRef::User, user_key); - service.insert_symmetric_key(MySymmKeyRef::Organization(org_id), org_key); - } - - let cipher_enc2 = service.encrypt(cipher_view.clone()).unwrap(); - - let cipher_view2 = service.decrypt(&cipher_enc2).unwrap(); - - assert_eq!(cipher_view.name, cipher_view2.name); - - // We can also decrypt a value by tagging it with the key - let text = String::from("test!"); - - let text_enc = service - .encrypt(text.as_str().using_key(MySymmKeyRef::User)) - .unwrap(); - - // And decrypt values in parallel - let mut data = Vec::with_capacity(250); - for _ in 0..data.capacity() { - data.push("hello world, this is an encryption test!".using_key(MySymmKeyRef::User)); - } - service.encrypt_list(&data).unwrap(); - } -} diff --git a/crates/bitwarden-crypto/benches/new_encryptable.rs b/crates/bitwarden-crypto/benches/new_encryptable.rs index 3bd43f75c..b719484f1 100644 --- a/crates/bitwarden-crypto/benches/new_encryptable.rs +++ b/crates/bitwarden-crypto/benches/new_encryptable.rs @@ -146,7 +146,9 @@ impl Encryptable for CipherVi key: MySymmKeyRef, ) -> Result { let cipher_key = match &self.key { - Some(cipher_key) => ctx.decrypt_symmetric_key(key, CIPHER_KEY, cipher_key)?, + Some(cipher_key) => { + ctx.decrypt_symmetric_key_with_symmetric_key(key, CIPHER_KEY, cipher_key)? + } None => key, }; diff --git a/crates/bitwarden-crypto/src/service/context.rs b/crates/bitwarden-crypto/src/service/context.rs index f3e8c8877..81a902164 100644 --- a/crates/bitwarden-crypto/src/service/context.rs +++ b/crates/bitwarden-crypto/src/service/context.rs @@ -1,66 +1,159 @@ -use std::sync::RwLockReadGuard; +use std::sync::{RwLockReadGuard, RwLockWriteGuard}; use rsa::Oaep; use super::Keys; use crate::{ service::{key_store::KeyStore, AsymmetricKeyRef, SymmetricKeyRef}, - AsymmetricCryptoKey, AsymmetricEncString, CryptoError, EncString, SymmetricCryptoKey, + AsymmetricCryptoKey, AsymmetricEncString, CryptoError, EncString, Result, SymmetricCryptoKey, }; -pub struct CryptoServiceContext<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> { - // We hold a RwLock read guard to avoid having any nested - //calls locking it again and potentially causing a deadlock - pub(super) global_keys: RwLockReadGuard<'a, Keys>, +pub trait GlobalAccessMode<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> { + fn get(&self) -> &Keys; + fn get_mut(&mut self) -> Result<&mut Keys>; +} + +pub struct ReadOnlyGlobal<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef>( + pub(super) RwLockReadGuard<'a, Keys>, +); + +impl<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> + GlobalAccessMode<'a, SymmKeyRef, AsymmKeyRef> for ReadOnlyGlobal<'a, SymmKeyRef, AsymmKeyRef> +{ + fn get(&self) -> &Keys { + &self.0 + } + + fn get_mut(&mut self) -> Result<&mut Keys> { + // TODO: This should be a custom error + Err(crate::CryptoError::MissingKey2( + "Cannot modify in read-only mode".to_string(), + )) + } +} + +pub struct ReadWriteGlobal<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef>( + pub(super) RwLockWriteGuard<'a, Keys>, +); + +impl<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> + GlobalAccessMode<'a, SymmKeyRef, AsymmKeyRef> for ReadWriteGlobal<'a, SymmKeyRef, AsymmKeyRef> +{ + fn get(&self) -> &Keys { + &self.0 + } + + fn get_mut(&mut self) -> Result<&mut Keys> { + Ok(&mut self.0) + } +} + +pub struct CryptoServiceContext< + 'a, + SymmKeyRef: SymmetricKeyRef, + AsymmKeyRef: AsymmetricKeyRef, + AccessMode: GlobalAccessMode<'a, SymmKeyRef, AsymmKeyRef> = ReadOnlyGlobal< + 'a, + SymmKeyRef, + AsymmKeyRef, + >, +> { + pub(super) global: AccessMode, pub(super) local_symmetric_keys: Box>, pub(super) local_asymmetric_keys: Box>, + + pub(super) _phantom: std::marker::PhantomData<&'a ()>, } -impl<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> - CryptoServiceContext<'a, SymmKeyRef, AsymmKeyRef> +impl< + 'a, + SymmKeyRef: SymmetricKeyRef, + AsymmKeyRef: AsymmetricKeyRef, + AccessMode: GlobalAccessMode<'a, SymmKeyRef, AsymmKeyRef>, + > CryptoServiceContext<'a, SymmKeyRef, AsymmKeyRef, AccessMode> { pub fn clear(&mut self) { + // Clear global keys if we have write access + if let Ok(keys) = self.global.get_mut() { + keys.symmetric_keys.clear(); + keys.asymmetric_keys.clear(); + } + self.local_symmetric_keys.clear(); self.local_asymmetric_keys.clear(); } - pub fn decrypt_symmetric_key( + /// TODO: All these encrypt x key with x key look like they need to be made generic, + /// but I haven't found the best way to do that yet. + + pub fn decrypt_symmetric_key_with_symmetric_key( &mut self, encryption_key: SymmKeyRef, new_key_ref: SymmKeyRef, encrypted_key: &EncString, - ) -> Result { + ) -> Result { let mut new_key_material = self.decrypt_data_with_symmetric_key(encryption_key, encrypted_key)?; - let new_key = SymmetricCryptoKey::try_from(new_key_material.as_mut_slice())?; - self.local_symmetric_keys.insert(new_key_ref, new_key); + self.set_symmetric_key( + new_key_ref, + SymmetricCryptoKey::try_from(new_key_material.as_mut_slice())?, + )?; // Returning the new key reference for convenience Ok(new_key_ref) } - pub fn encrypt_symmetric_key( + pub fn encrypt_symmetric_key_with_symmetric_key( &self, encryption_key: SymmKeyRef, key_to_encrypt: SymmKeyRef, - ) -> Result { + ) -> Result { let key_to_encrypt = self.get_symmetric_key(key_to_encrypt)?; self.encrypt_data_with_symmetric_key(encryption_key, &key_to_encrypt.to_vec()) } + pub fn decrypt_symmetric_key_with_asymmetric_key( + &mut self, + encryption_key: AsymmKeyRef, + new_key_ref: SymmKeyRef, + encrypted_key: &AsymmetricEncString, + ) -> Result { + let mut new_key_material = + self.decrypt_data_with_asymmetric_key(encryption_key, encrypted_key)?; + + self.set_symmetric_key( + new_key_ref, + SymmetricCryptoKey::try_from(new_key_material.as_mut_slice())?, + )?; + + // Returning the new key reference for convenience + Ok(new_key_ref) + } + + pub fn encrypt_symmetric_key_with_asymmetric_key( + &self, + encryption_key: AsymmKeyRef, + key_to_encrypt: SymmKeyRef, + ) -> Result { + let key_to_encrypt = self.get_symmetric_key(key_to_encrypt)?; + self.encrypt_data_with_asymmetric_key(encryption_key, &key_to_encrypt.to_vec()) + } + pub fn decrypt_asymmetric_key( &mut self, encryption_key: AsymmKeyRef, new_key_ref: AsymmKeyRef, encrypted_key: &AsymmetricEncString, - ) -> Result { + ) -> Result { let new_key_material = self.decrypt_data_with_asymmetric_key(encryption_key, encrypted_key)?; - let new_key = AsymmetricCryptoKey::from_der(&new_key_material)?; - self.local_asymmetric_keys.insert(new_key_ref, new_key); + self.set_asymmetric_key( + new_key_ref, + AsymmetricCryptoKey::from_der(&new_key_material)?, + )?; // Returning the new key reference for convenience Ok(new_key_ref) @@ -70,7 +163,7 @@ impl<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> &self, encryption_key: AsymmKeyRef, key_to_encrypt: AsymmKeyRef, - ) -> Result { + ) -> Result { let encryption_key = self.get_asymmetric_key(encryption_key)?; let key_to_encrypt = self.get_asymmetric_key(key_to_encrypt)?; @@ -80,11 +173,16 @@ impl<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> ) } + pub fn has_symmetric_key(&self, key_ref: SymmKeyRef) -> bool { + self.get_symmetric_key(key_ref).is_ok() + } + + pub fn has_asymmetric_key(&self, key_ref: AsymmKeyRef) -> bool { + self.get_asymmetric_key(key_ref).is_ok() + } + #[deprecated(note = "This function should never be used outside this crate")] - pub fn dangerous_get_symmetric_key( - &self, - key_ref: SymmKeyRef, - ) -> Result<&SymmetricCryptoKey, crate::CryptoError> { + pub fn dangerous_get_symmetric_key(&self, key_ref: SymmKeyRef) -> Result<&SymmetricCryptoKey> { self.get_symmetric_key(key_ref) } @@ -92,39 +190,51 @@ impl<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> pub fn dangerous_get_asymmetric_key( &self, key_ref: AsymmKeyRef, - ) -> Result<&AsymmetricCryptoKey, crate::CryptoError> { + ) -> Result<&AsymmetricCryptoKey> { self.get_asymmetric_key(key_ref) } - fn get_symmetric_key( - &self, - key_ref: SymmKeyRef, - ) -> Result<&SymmetricCryptoKey, crate::CryptoError> { + fn get_symmetric_key(&self, key_ref: SymmKeyRef) -> Result<&SymmetricCryptoKey> { if key_ref.is_local() { self.local_symmetric_keys.get(key_ref) } else { - self.global_keys.symmetric_keys.get(key_ref) + self.global.get().symmetric_keys.get(key_ref) } .ok_or_else(|| crate::CryptoError::MissingKey2(format!("{key_ref:?}"))) } - fn get_asymmetric_key( - &self, - key_ref: AsymmKeyRef, - ) -> Result<&AsymmetricCryptoKey, crate::CryptoError> { + fn get_asymmetric_key(&self, key_ref: AsymmKeyRef) -> Result<&AsymmetricCryptoKey> { if key_ref.is_local() { self.local_asymmetric_keys.get(key_ref) } else { - self.global_keys.asymmetric_keys.get(key_ref) + self.global.get().asymmetric_keys.get(key_ref) } .ok_or_else(|| crate::CryptoError::MissingKey2(format!("{key_ref:?}"))) } + fn set_symmetric_key(&mut self, key_ref: SymmKeyRef, key: SymmetricCryptoKey) -> Result<()> { + if key_ref.is_local() { + self.local_symmetric_keys.insert(key_ref, key); + } else { + self.global.get_mut()?.symmetric_keys.insert(key_ref, key); + } + Ok(()) + } + + fn set_asymmetric_key(&mut self, key_ref: AsymmKeyRef, key: AsymmetricCryptoKey) -> Result<()> { + if key_ref.is_local() { + self.local_asymmetric_keys.insert(key_ref, key); + } else { + self.global.get_mut()?.asymmetric_keys.insert(key_ref, key); + } + Ok(()) + } + pub(super) fn decrypt_data_with_symmetric_key( &self, key: SymmKeyRef, data: &EncString, - ) -> Result, crate::CryptoError> { + ) -> Result> { let key = self.get_symmetric_key(key)?; match data { @@ -155,7 +265,7 @@ impl<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> &self, key: SymmKeyRef, data: &[u8], - ) -> Result { + ) -> Result { let key = self.get_symmetric_key(key)?; EncString::encrypt_aes256_hmac( data, @@ -168,7 +278,7 @@ impl<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> &self, key: AsymmKeyRef, data: &AsymmetricEncString, - ) -> Result, crate::CryptoError> { + ) -> Result> { let key = self.get_asymmetric_key(key)?; use AsymmetricEncString::*; @@ -191,7 +301,7 @@ impl<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> &self, key: AsymmKeyRef, data: &[u8], - ) -> Result { + ) -> Result { let key = self.get_asymmetric_key(key)?; AsymmetricEncString::encrypt_rsa2048_oaep_sha1(data, key) } diff --git a/crates/bitwarden-crypto/src/service/key_ref.rs b/crates/bitwarden-crypto/src/service/key_ref.rs index 5c4cd307b..a26b6d895 100644 --- a/crates/bitwarden-crypto/src/service/key_ref.rs +++ b/crates/bitwarden-crypto/src/service/key_ref.rs @@ -10,16 +10,17 @@ pub mod __internal { use crate::CryptoKey; - /// This trait represents a key reference that can be used to identify cryptographic keys in the key - /// store. It is used to abstract over the different types of keys that can be used in the system, - /// an end user would not implement this trait directly, and would instead use the `SymmetricKeyRef` - /// and `AsymmetricKeyRef` traits. + /// This trait represents a key reference that can be used to identify cryptographic keys in the + /// key store. It is used to abstract over the different types of keys that can be used in + /// the system, an end user would not implement this trait directly, and would instead use + /// the `SymmetricKeyRef` and `AsymmetricKeyRef` traits. pub trait KeyRef: Debug + Clone + Copy + Hash + Eq + PartialEq + Ord + PartialOrd + Send + Sync + 'static { type KeyValue: CryptoKey + Send + Sync + ZeroizeOnDrop; - /// Returns whether the key is local to the current context or shared globally by the service. + /// Returns whether the key is local to the current context or shared globally by the + /// service. fn is_local(&self) -> bool; } } diff --git a/crates/bitwarden-crypto/src/service/mod.rs b/crates/bitwarden-crypto/src/service/mod.rs index 793935b94..b54f879d1 100644 --- a/crates/bitwarden-crypto/src/service/mod.rs +++ b/crates/bitwarden-crypto/src/service/mod.rs @@ -7,7 +7,8 @@ mod encryptable; pub mod key_ref; mod key_store; -pub use context::CryptoServiceContext; +use context::ReadWriteGlobal; +pub use context::{CryptoServiceContext, ReadOnlyGlobal}; pub use encryptable::{Decryptable, Encryptable, UsesKey, UsingKey, UsingKeyExt}; use key_ref::{AsymmetricKeyRef, KeyRef, SymmetricKeyRef}; pub use key_store::create_key_store; @@ -28,9 +29,7 @@ impl std::fmt::Debug } } -// This is just a wrapper around the keys so we only deal with one RwLock -pub(crate) struct Keys -{ +pub struct Keys { symmetric_keys: Box>, asymmetric_keys: Box>, } @@ -88,13 +87,49 @@ impl .insert(key_ref, key); } - /// Initiate an encryption/decryption context. This is an advanced API, use with care. - /// Prefer to instead use `encrypt`/`decrypt`/`encrypt_list`/`decrypt_list` methods. + /// Initiate an encryption/decryption context. This context will have read only access to the + /// global keys, and will have its own local key stores with read/write access. This + /// context-local store will be cleared up when the context is dropped. + /// + /// This is an advanced API, use with care. Prefer to instead use + /// `encrypt`/`decrypt`/`encrypt_list`/`decrypt_list` methods. + /// + /// One of the pitfalls of the current implementations is that keys stored in the context-local + /// store only get cleared automatically when dropped, and not between operations. This + /// means that if you are using the same context for multiple operations, you may want to + /// clear it manually between them. pub fn context(&'_ self) -> CryptoServiceContext<'_, SymmKeyRef, AsymmKeyRef> { CryptoServiceContext { - global_keys: self.key_stores.read().expect("RwLock is poisoned"), + global: ReadOnlyGlobal(self.key_stores.read().expect("RwLock is poisoned")), + local_symmetric_keys: create_key_store(), + local_asymmetric_keys: create_key_store(), + _phantom: std::marker::PhantomData, + } + } + + /// Initiate an encryption/decryption context. This context will have MUTABLE access to the + /// global keys, and will have its own local key stores with read/write access. This + /// context-local store will be cleared up when the context is dropped. + /// + /// This is an advanced API, use with care and ONLY when needing to modify the global keys. + /// + /// The same pitfalls as `context` apply here, but with the added risk of accidentally + /// modifying the global keys and leaving the service in an inconsistent state. + /// + /// TODO: We should work towards making this pub(crate) + pub fn context_mut( + &'_ self, + ) -> CryptoServiceContext< + '_, + SymmKeyRef, + AsymmKeyRef, + ReadWriteGlobal<'_, SymmKeyRef, AsymmKeyRef>, + > { + CryptoServiceContext { + global: ReadWriteGlobal(self.key_stores.write().expect("RwLock is poisoned")), local_symmetric_keys: create_key_store(), local_asymmetric_keys: create_key_store(), + _phantom: std::marker::PhantomData, } } From fdb0263e7618c3548fe4aca2d43a48c57d15c14f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garci=CC=81a?= Date: Fri, 4 Oct 2024 18:29:27 +0200 Subject: [PATCH 28/34] Refactor keyref/encryptable locations --- crates/bitwarden-crypto/benches/new_encryptable.rs | 10 +++++----- .../src/{service => keys}/encryptable.rs | 11 ++--------- .../bitwarden-crypto/src/{service => keys}/key_ref.rs | 6 +++--- crates/bitwarden-crypto/src/keys/mod.rs | 5 +++++ crates/bitwarden-crypto/src/service/context.rs | 8 ++++---- .../bitwarden-crypto/src/service/key_store/slice.rs | 8 +++----- crates/bitwarden-crypto/src/service/mod.rs | 11 ++++++----- 7 files changed, 28 insertions(+), 31 deletions(-) rename crates/bitwarden-crypto/src/{service => keys}/encryptable.rs (96%) rename crates/bitwarden-crypto/src/{service => keys}/key_ref.rs (90%) diff --git a/crates/bitwarden-crypto/benches/new_encryptable.rs b/crates/bitwarden-crypto/benches/new_encryptable.rs index b719484f1..3eff9e98d 100644 --- a/crates/bitwarden-crypto/benches/new_encryptable.rs +++ b/crates/bitwarden-crypto/benches/new_encryptable.rs @@ -1,5 +1,10 @@ use criterion::{black_box, criterion_group, criterion_main, BenchmarkId, Criterion, Throughput}; +use bitwarden_crypto::{ + key_refs, service::*, CryptoError, EncString, Encryptable, KeyDecryptable, KeyEncryptable, + SymmetricCryptoKey, UsesKey, +}; + pub fn criterion_benchmark(c: &mut Criterion) { let user_key = SymmetricCryptoKey::generate(rand::thread_rng()); @@ -90,11 +95,6 @@ pub fn criterion_benchmark(c: &mut Criterion) { criterion_group!(benches, criterion_benchmark); criterion_main!(benches); -use bitwarden_crypto::{ - key_refs, service::*, CryptoError, EncString, KeyDecryptable, KeyEncryptable, - SymmetricCryptoKey, -}; - key_refs! { #[symmetric] pub enum MySymmKeyRef { diff --git a/crates/bitwarden-crypto/src/service/encryptable.rs b/crates/bitwarden-crypto/src/keys/encryptable.rs similarity index 96% rename from crates/bitwarden-crypto/src/service/encryptable.rs rename to crates/bitwarden-crypto/src/keys/encryptable.rs index 4fa23618c..8b74d6009 100644 --- a/crates/bitwarden-crypto/src/service/encryptable.rs +++ b/crates/bitwarden-crypto/src/keys/encryptable.rs @@ -1,10 +1,5 @@ -use super::{ - key_ref::{AsymmetricKeyRef, KeyRef, SymmetricKeyRef}, - CryptoServiceContext, -}; -use crate::{AsymmetricEncString, CryptoError, EncString}; - -/////////////////////// +use super::key_ref::{AsymmetricKeyRef, KeyRef, SymmetricKeyRef}; +use crate::{service::CryptoServiceContext, AsymmetricEncString, CryptoError, EncString}; // Just like LocateKey but this time we're not locating anything, just returning a ref @@ -63,8 +58,6 @@ impl< } } -///////////////////// - pub trait Encryptable< SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef, diff --git a/crates/bitwarden-crypto/src/service/key_ref.rs b/crates/bitwarden-crypto/src/keys/key_ref.rs similarity index 90% rename from crates/bitwarden-crypto/src/service/key_ref.rs rename to crates/bitwarden-crypto/src/keys/key_ref.rs index a26b6d895..39ac2cddc 100644 --- a/crates/bitwarden-crypto/src/service/key_ref.rs +++ b/crates/bitwarden-crypto/src/keys/key_ref.rs @@ -60,7 +60,7 @@ macro_rules! key_refs { $variant $( ($inner) )? ,)+ } - impl $crate::service::key_ref::__internal::KeyRef for $name { + impl $crate::key_ref::__internal::KeyRef for $name { type KeyValue = key_refs!(@key_type $meta_type); fn is_local(&self) -> bool { @@ -78,8 +78,8 @@ macro_rules! key_refs { ( @key_type symmetric ) => { $crate::SymmetricCryptoKey }; ( @key_type asymmetric ) => { $crate::AsymmetricCryptoKey }; - ( @key_trait symmetric $name:ident ) => { impl $crate::service::key_ref::SymmetricKeyRef for $name {} }; - ( @key_trait asymmetric $name:ident ) => { impl $crate::service::key_ref::AsymmetricKeyRef for $name {} }; + ( @key_trait symmetric $name:ident ) => { impl $crate::key_ref::SymmetricKeyRef for $name {} }; + ( @key_trait asymmetric $name:ident ) => { impl $crate::key_ref::AsymmetricKeyRef for $name {} }; ( @variant_match $variant:ident ( $inner:ty ) ) => { $variant (_) }; ( @variant_match $variant:ident ) => { $variant }; diff --git a/crates/bitwarden-crypto/src/keys/mod.rs b/crates/bitwarden-crypto/src/keys/mod.rs index ac1732966..1267fd4fb 100644 --- a/crates/bitwarden-crypto/src/keys/mod.rs +++ b/crates/bitwarden-crypto/src/keys/mod.rs @@ -1,5 +1,10 @@ mod key_encryptable; pub use key_encryptable::{CryptoKey, KeyContainer, KeyDecryptable, KeyEncryptable, LocateKey}; +mod encryptable; +pub use encryptable::{Decryptable, Encryptable, UsesKey, UsingKey, UsingKeyExt}; +pub mod key_ref; +pub(crate) use key_ref::KeyRef; +pub use key_ref::{AsymmetricKeyRef, SymmetricKeyRef}; mod master_key; pub use master_key::{ default_argon2_iterations, default_argon2_memory, default_argon2_parallelism, diff --git a/crates/bitwarden-crypto/src/service/context.rs b/crates/bitwarden-crypto/src/service/context.rs index 81a902164..c4ca497a3 100644 --- a/crates/bitwarden-crypto/src/service/context.rs +++ b/crates/bitwarden-crypto/src/service/context.rs @@ -230,7 +230,7 @@ impl< Ok(()) } - pub(super) fn decrypt_data_with_symmetric_key( + pub(crate) fn decrypt_data_with_symmetric_key( &self, key: SymmKeyRef, data: &EncString, @@ -261,7 +261,7 @@ impl< } } - pub(super) fn encrypt_data_with_symmetric_key( + pub(crate) fn encrypt_data_with_symmetric_key( &self, key: SymmKeyRef, data: &[u8], @@ -274,7 +274,7 @@ impl< ) } - pub(super) fn decrypt_data_with_asymmetric_key( + pub(crate) fn decrypt_data_with_asymmetric_key( &self, key: AsymmKeyRef, data: &AsymmetricEncString, @@ -297,7 +297,7 @@ impl< .map_err(|_| CryptoError::KeyDecrypt) } - pub(super) fn encrypt_data_with_asymmetric_key( + pub(crate) fn encrypt_data_with_asymmetric_key( &self, key: AsymmKeyRef, data: &[u8], diff --git a/crates/bitwarden-crypto/src/service/key_store/slice.rs b/crates/bitwarden-crypto/src/service/key_store/slice.rs index 1bcc688a5..c2143c858 100644 --- a/crates/bitwarden-crypto/src/service/key_store/slice.rs +++ b/crates/bitwarden-crypto/src/service/key_store/slice.rs @@ -2,8 +2,9 @@ use std::marker::PhantomData; use zeroize::ZeroizeOnDrop; +use crate::KeyRef; + use super::KeyStore; -use crate::service::key_ref::KeyRef; /// This trait represents some data stored sequentially in memory, with a fixed size. /// We use this to abstract the implementation over Vec/Box<[u8]/NonNull<[u8]>, which @@ -271,10 +272,7 @@ pub(crate) mod tests { use zeroize::Zeroize; use super::*; - use crate::{ - service::{key_ref::KeyRef, key_store::implementation::rust_slice::RustKeyStore}, - CryptoKey, - }; + use crate::{service::key_store::implementation::rust_slice::RustKeyStore, CryptoKey, KeyRef}; #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] pub enum TestKey { diff --git a/crates/bitwarden-crypto/src/service/mod.rs b/crates/bitwarden-crypto/src/service/mod.rs index b54f879d1..907069c2f 100644 --- a/crates/bitwarden-crypto/src/service/mod.rs +++ b/crates/bitwarden-crypto/src/service/mod.rs @@ -1,16 +1,17 @@ use std::sync::{Arc, RwLock}; -use crate::{AsymmetricCryptoKey, SymmetricCryptoKey}; +use crate::{ + AsymmetricCryptoKey, AsymmetricKeyRef, Decryptable, Encryptable, KeyRef, SymmetricCryptoKey, + SymmetricKeyRef, UsesKey, +}; mod context; -mod encryptable; -pub mod key_ref; + mod key_store; use context::ReadWriteGlobal; pub use context::{CryptoServiceContext, ReadOnlyGlobal}; -pub use encryptable::{Decryptable, Encryptable, UsesKey, UsingKey, UsingKeyExt}; -use key_ref::{AsymmetricKeyRef, KeyRef, SymmetricKeyRef}; + pub use key_store::create_key_store; use key_store::KeyStore; From 6ea626720d5116af9afe6c4fca2aa78f7870bfe6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garci=CC=81a?= Date: Mon, 7 Oct 2024 16:51:27 +0200 Subject: [PATCH 29/34] Some additions needed to migrate the code --- .../benches/new_encryptable.rs | 13 ++- .../bitwarden-crypto/src/keys/encryptable.rs | 104 ++++++++++++++++++ .../bitwarden-crypto/src/service/context.rs | 56 +++++++++- .../src/service/key_store/slice.rs | 3 +- crates/bitwarden-crypto/src/service/mod.rs | 52 ++------- 5 files changed, 173 insertions(+), 55 deletions(-) diff --git a/crates/bitwarden-crypto/benches/new_encryptable.rs b/crates/bitwarden-crypto/benches/new_encryptable.rs index 3eff9e98d..e900d0008 100644 --- a/crates/bitwarden-crypto/benches/new_encryptable.rs +++ b/crates/bitwarden-crypto/benches/new_encryptable.rs @@ -1,9 +1,8 @@ -use criterion::{black_box, criterion_group, criterion_main, BenchmarkId, Criterion, Throughput}; - use bitwarden_crypto::{ key_refs, service::*, CryptoError, EncString, Encryptable, KeyDecryptable, KeyEncryptable, SymmetricCryptoKey, UsesKey, }; +use criterion::{black_box, criterion_group, criterion_main, BenchmarkId, Criterion, Throughput}; pub fn criterion_benchmark(c: &mut Criterion) { let user_key = SymmetricCryptoKey::generate(rand::thread_rng()); @@ -21,8 +20,14 @@ pub fn criterion_benchmark(c: &mut Criterion) { let service: CryptoService = CryptoService::new(); #[allow(deprecated)] { - service.insert_symmetric_key(MySymmKeyRef::User, user_key.clone()); - service.insert_symmetric_key(MySymmKeyRef::Organization(org_id), org_key.clone()); + service + .context_mut() + .set_symmetric_key(MySymmKeyRef::User, user_key.clone()) + .expect(""); + service + .context_mut() + .set_symmetric_key(MySymmKeyRef::Organization(org_id), org_key.clone()) + .expect(""); } let cipher_views = vec![cipher_view.clone(); 10_000]; diff --git a/crates/bitwarden-crypto/src/keys/encryptable.rs b/crates/bitwarden-crypto/src/keys/encryptable.rs index 8b74d6009..5b51f25e6 100644 --- a/crates/bitwarden-crypto/src/keys/encryptable.rs +++ b/crates/bitwarden-crypto/src/keys/encryptable.rs @@ -86,6 +86,8 @@ pub trait Decryptable< ) -> Result; } +// Basic Encryptable/Decryptable implementations to and from bytes + impl Decryptable> for EncString { @@ -134,6 +136,8 @@ impl } } +// Encryptable/Decryptable implementations to and from strings + impl Decryptable for EncString { @@ -183,3 +187,103 @@ impl self.as_bytes().encrypt(ctx, key) } } + +impl + Encryptable for String +{ + fn encrypt( + &self, + ctx: &mut CryptoServiceContext, + key: SymmKeyRef, + ) -> Result { + self.as_bytes().encrypt(ctx, key) + } +} + +impl + Encryptable for String +{ + fn encrypt( + &self, + ctx: &mut CryptoServiceContext, + key: AsymmKeyRef, + ) -> Result { + self.as_bytes().encrypt(ctx, key) + } +} + +// Generic implementations for Optional values + +impl< + SymmKeyRef: SymmetricKeyRef, + AsymmKeyRef: AsymmetricKeyRef, + Key: KeyRef, + T: Encryptable, + Output, + > Encryptable> for Option +{ + fn encrypt( + &self, + ctx: &mut CryptoServiceContext, + key: Key, + ) -> Result, crate::CryptoError> { + self.as_ref() + .map(|value| value.encrypt(ctx, key)) + .transpose() + } +} + +impl< + SymmKeyRef: SymmetricKeyRef, + AsymmKeyRef: AsymmetricKeyRef, + Key: KeyRef, + T: Decryptable, + Output, + > Decryptable> for Option +{ + fn decrypt( + &self, + ctx: &mut CryptoServiceContext, + key: Key, + ) -> Result, crate::CryptoError> { + self.as_ref() + .map(|value| value.decrypt(ctx, key)) + .transpose() + } +} + +// Generic implementations for Vec values + +impl< + SymmKeyRef: SymmetricKeyRef, + AsymmKeyRef: AsymmetricKeyRef, + Key: KeyRef, + T: Encryptable, + Output, + > Encryptable> for Vec +{ + fn encrypt( + &self, + ctx: &mut CryptoServiceContext, + key: Key, + ) -> Result, crate::CryptoError> { + self.iter().map(|value| value.encrypt(ctx, key)).collect() + } +} + +impl< + SymmKeyRef: SymmetricKeyRef, + AsymmKeyRef: AsymmetricKeyRef, + Key: KeyRef, + T: Decryptable, + Output, + > Decryptable> for Vec +{ + fn decrypt( + &self, + ctx: &mut CryptoServiceContext, + key: Key, + ) -> Result, crate::CryptoError> { + self.iter().map(|value| value.decrypt(ctx, key)).collect() + } +} diff --git a/crates/bitwarden-crypto/src/service/context.rs b/crates/bitwarden-crypto/src/service/context.rs index c4ca497a3..422cc7d6d 100644 --- a/crates/bitwarden-crypto/src/service/context.rs +++ b/crates/bitwarden-crypto/src/service/context.rs @@ -1,9 +1,11 @@ use std::sync::{RwLockReadGuard, RwLockWriteGuard}; use rsa::Oaep; +use zeroize::Zeroizing; use super::Keys; use crate::{ + derive_shareable_key, service::{key_store::KeyStore, AsymmetricKeyRef, SymmetricKeyRef}, AsymmetricCryptoKey, AsymmetricEncString, CryptoError, EncString, Result, SymmetricCryptoKey, }; @@ -84,6 +86,20 @@ impl< self.local_asymmetric_keys.clear(); } + pub fn retain_symmetric_keys(&mut self, f: fn(SymmKeyRef) -> bool) { + if let Ok(keys) = self.global.get_mut() { + keys.symmetric_keys.retain(f); + } + self.local_symmetric_keys.retain(f); + } + + pub fn retain_asymmetric_keys(&mut self, f: fn(AsymmKeyRef) -> bool) { + if let Ok(keys) = self.global.get_mut() { + keys.asymmetric_keys.retain(f); + } + self.local_asymmetric_keys.retain(f); + } + /// TODO: All these encrypt x key with x key look like they need to be made generic, /// but I haven't found the best way to do that yet. @@ -96,6 +112,7 @@ impl< let mut new_key_material = self.decrypt_data_with_symmetric_key(encryption_key, encrypted_key)?; + #[allow(deprecated)] self.set_symmetric_key( new_key_ref, SymmetricCryptoKey::try_from(new_key_material.as_mut_slice())?, @@ -123,6 +140,7 @@ impl< let mut new_key_material = self.decrypt_data_with_asymmetric_key(encryption_key, encrypted_key)?; + #[allow(deprecated)] self.set_symmetric_key( new_key_ref, SymmetricCryptoKey::try_from(new_key_material.as_mut_slice())?, @@ -150,6 +168,7 @@ impl< let new_key_material = self.decrypt_data_with_asymmetric_key(encryption_key, encrypted_key)?; + #[allow(deprecated)] self.set_asymmetric_key( new_key_ref, AsymmetricCryptoKey::from_der(&new_key_material)?, @@ -181,12 +200,31 @@ impl< self.get_asymmetric_key(key_ref).is_ok() } - #[deprecated(note = "This function should never be used outside this crate")] + pub fn generate_symmetric_key(&mut self, key_ref: SymmKeyRef) -> Result { + let key = SymmetricCryptoKey::generate(rand::thread_rng()); + #[allow(deprecated)] + self.set_symmetric_key(key_ref, key)?; + Ok(key_ref) + } + + pub fn derive_shareable_key( + &mut self, + key_ref: SymmKeyRef, + secret: Zeroizing<[u8; 16]>, + name: &str, + info: Option<&str>, + ) -> Result { + #[allow(deprecated)] + self.set_symmetric_key(key_ref, derive_shareable_key(secret, name, info))?; + Ok(key_ref) + } + + #[deprecated(note = "This function should ideally never be used outside this crate")] pub fn dangerous_get_symmetric_key(&self, key_ref: SymmKeyRef) -> Result<&SymmetricCryptoKey> { self.get_symmetric_key(key_ref) } - #[deprecated(note = "This function should never be used outside this crate")] + #[deprecated(note = "This function should ideally never be used outside this crate")] pub fn dangerous_get_asymmetric_key( &self, key_ref: AsymmKeyRef, @@ -212,7 +250,12 @@ impl< .ok_or_else(|| crate::CryptoError::MissingKey2(format!("{key_ref:?}"))) } - fn set_symmetric_key(&mut self, key_ref: SymmKeyRef, key: SymmetricCryptoKey) -> Result<()> { + #[deprecated(note = "This function should ideally never be used outside this crate")] + pub fn set_symmetric_key( + &mut self, + key_ref: SymmKeyRef, + key: SymmetricCryptoKey, + ) -> Result<()> { if key_ref.is_local() { self.local_symmetric_keys.insert(key_ref, key); } else { @@ -221,7 +264,12 @@ impl< Ok(()) } - fn set_asymmetric_key(&mut self, key_ref: AsymmKeyRef, key: AsymmetricCryptoKey) -> Result<()> { + #[deprecated(note = "This function should ideally never be used outside this crate")] + pub fn set_asymmetric_key( + &mut self, + key_ref: AsymmKeyRef, + key: AsymmetricCryptoKey, + ) -> Result<()> { if key_ref.is_local() { self.local_asymmetric_keys.insert(key_ref, key); } else { diff --git a/crates/bitwarden-crypto/src/service/key_store/slice.rs b/crates/bitwarden-crypto/src/service/key_store/slice.rs index c2143c858..4ad83c977 100644 --- a/crates/bitwarden-crypto/src/service/key_store/slice.rs +++ b/crates/bitwarden-crypto/src/service/key_store/slice.rs @@ -2,9 +2,8 @@ use std::marker::PhantomData; use zeroize::ZeroizeOnDrop; -use crate::KeyRef; - use super::KeyStore; +use crate::KeyRef; /// This trait represents some data stored sequentially in memory, with a fixed size. /// We use this to abstract the implementation over Vec/Box<[u8]/NonNull<[u8]>, which diff --git a/crates/bitwarden-crypto/src/service/mod.rs b/crates/bitwarden-crypto/src/service/mod.rs index 907069c2f..6d94e29bc 100644 --- a/crates/bitwarden-crypto/src/service/mod.rs +++ b/crates/bitwarden-crypto/src/service/mod.rs @@ -1,9 +1,6 @@ use std::sync::{Arc, RwLock}; -use crate::{ - AsymmetricCryptoKey, AsymmetricKeyRef, Decryptable, Encryptable, KeyRef, SymmetricCryptoKey, - SymmetricKeyRef, UsesKey, -}; +use crate::{AsymmetricKeyRef, Decryptable, Encryptable, KeyRef, SymmetricKeyRef, UsesKey}; mod context; @@ -11,7 +8,6 @@ mod key_store; use context::ReadWriteGlobal; pub use context::{CryptoServiceContext, ReadOnlyGlobal}; - pub use key_store::create_key_store; use key_store::KeyStore; @@ -54,40 +50,6 @@ impl keys.asymmetric_keys.clear(); } - pub fn retain_symmetric_keys(&self, f: fn(SymmKeyRef) -> bool) { - self.key_stores - .write() - .expect("RwLock is poisoned") - .symmetric_keys - .retain(f); - } - - pub fn retain_asymmetric_keys(&self, f: fn(AsymmKeyRef) -> bool) { - self.key_stores - .write() - .expect("RwLock is poisoned") - .asymmetric_keys - .retain(f); - } - - #[deprecated(note = "We should be generating/decrypting the keys into the service directly")] - pub fn insert_symmetric_key(&self, key_ref: SymmKeyRef, key: SymmetricCryptoKey) { - self.key_stores - .write() - .expect("RwLock is poisoned") - .symmetric_keys - .insert(key_ref, key); - } - - #[deprecated(note = "We should be generating/decrypting the keys into the service directly")] - pub fn insert_asymmetric_key(&self, key_ref: AsymmKeyRef, key: AsymmetricCryptoKey) { - self.key_stores - .write() - .expect("RwLock is poisoned") - .asymmetric_keys - .insert(key_ref, key); - } - /// Initiate an encryption/decryption context. This context will have read only access to the /// global keys, and will have its own local key stores with read/write access. This /// context-local store will be cleared up when the context is dropped. @@ -178,14 +140,14 @@ impl let res: Result, _> = data .par_chunks(chunk_size) .map(|chunk| { - let mut context = self.context(); + let mut ctx = self.context(); let mut result = Vec::with_capacity(chunk.len()); for item in chunk { let key = item.uses_key(); - result.push(item.decrypt(&mut context, key)); - context.clear(); + result.push(item.decrypt(&mut ctx, key)); + ctx.clear(); } result @@ -215,14 +177,14 @@ impl let res: Result, _> = data .par_chunks(chunk_size) .map(|chunk| { - let mut context = self.context(); + let mut ctx = self.context(); let mut result = Vec::with_capacity(chunk.len()); for item in chunk { let key = item.uses_key(); - result.push(item.encrypt(&mut context, key)); - context.clear(); + result.push(item.encrypt(&mut ctx, key)); + ctx.clear(); } result From f882fb2ac969ccd2ef2a3b5a5a4e8ffe41ada0ea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garci=CC=81a?= Date: Mon, 7 Oct 2024 17:07:51 +0200 Subject: [PATCH 30/34] Remove bench --- crates/bitwarden-crypto/Cargo.toml | 6 +- .../benches/new_encryptable.rs | 187 ------------------ crates/memory-testing/Dockerfile | 4 +- 3 files changed, 3 insertions(+), 194 deletions(-) delete mode 100644 crates/bitwarden-crypto/benches/new_encryptable.rs diff --git a/crates/bitwarden-crypto/Cargo.toml b/crates/bitwarden-crypto/Cargo.toml index a40a4fab7..5fd9d6144 100644 --- a/crates/bitwarden-crypto/Cargo.toml +++ b/crates/bitwarden-crypto/Cargo.toml @@ -52,7 +52,7 @@ memsec = { version = "0.7.0", features = [ ], git = "https://github.com/dani-garcia/memsec", rev = "3d2e272d284442637bac0a7d94f76883960db7e2" } [dev-dependencies] -criterion = { version = "0.5.1", features = ["html_reports"] } +criterion = "0.5.1" rand_chacha = "0.3.1" serde_json = { workspace = true } @@ -66,9 +66,5 @@ name = "zeroizing_allocator" harness = false required-features = ["no-memory-hardening"] -[[bench]] -name = "new_encryptable" -harness = false - [lints] workspace = true diff --git a/crates/bitwarden-crypto/benches/new_encryptable.rs b/crates/bitwarden-crypto/benches/new_encryptable.rs deleted file mode 100644 index e900d0008..000000000 --- a/crates/bitwarden-crypto/benches/new_encryptable.rs +++ /dev/null @@ -1,187 +0,0 @@ -use bitwarden_crypto::{ - key_refs, service::*, CryptoError, EncString, Encryptable, KeyDecryptable, KeyEncryptable, - SymmetricCryptoKey, UsesKey, -}; -use criterion::{black_box, criterion_group, criterion_main, BenchmarkId, Criterion, Throughput}; - -pub fn criterion_benchmark(c: &mut Criterion) { - let user_key = SymmetricCryptoKey::generate(rand::thread_rng()); - - let org_id = uuid::Uuid::parse_str("91b000b6-81ce-47f4-9802-3390e0b895ed").expect(""); - let org_key = SymmetricCryptoKey::generate(rand::thread_rng()); - - let cipher_key = SymmetricCryptoKey::generate(rand::thread_rng()); - let cipher_key_user_enc = cipher_key.to_vec().encrypt_with_key(&user_key).expect(""); - let cipher_view = CipherView { - key: Some(cipher_key_user_enc.clone()), - name: "test".to_string(), - }; - - let service: CryptoService = CryptoService::new(); - #[allow(deprecated)] - { - service - .context_mut() - .set_symmetric_key(MySymmKeyRef::User, user_key.clone()) - .expect(""); - service - .context_mut() - .set_symmetric_key(MySymmKeyRef::Organization(org_id), org_key.clone()) - .expect(""); - } - - let cipher_views = vec![cipher_view.clone(); 10_000]; - - { - let mut group = c.benchmark_group("New encryptable"); - - for size in [1, 10, 100, 1_000, 10_000].iter() { - group.throughput(Throughput::Elements(*size as u64)); - - group.bench_with_input( - BenchmarkId::new("encrypt X ciphers individually", size), - size, - |b, &size| { - b.iter(|| { - for c in cipher_views.iter().take(size).cloned() { - service.encrypt(black_box(c)).expect(""); - } - }); - }, - ); - - group.bench_with_input( - BenchmarkId::new("encrypt X ciphers batch", size), - size, - |b, &size| { - b.iter(|| service.encrypt_list(black_box(&cipher_views[..size]))); - }, - ); - } - - group.finish(); - } - - { - let mut group = c.benchmark_group("Old encryptable"); - - for size in [1, 10, 100, 1_000, 10_000].iter() { - group.throughput(Throughput::Elements(*size as u64)); - - group.bench_with_input( - BenchmarkId::new("encrypt X ciphers individually", size), - size, - |b, &size| { - b.iter(|| { - for c in cipher_views.iter().take(size).cloned() { - black_box(c).encrypt_with_key(&user_key).expect(""); - } - }); - }, - ); - - group.bench_with_input( - BenchmarkId::new("encrypt X ciphers batch", size), - size, - |b, &size| { - b.iter(|| { - black_box(cipher_views[0..size].to_vec()) - .encrypt_with_key(&user_key) - .expect(""); - }); - }, - ); - } - - group.finish(); - } -} - -criterion_group!(benches, criterion_benchmark); -criterion_main!(benches); - -key_refs! { - #[symmetric] - pub enum MySymmKeyRef { - User, - Organization(uuid::Uuid), - #[local] - Local(&'static str), - } - - #[asymmetric] - pub enum MyAsymmKeyRef { - UserPrivateKey, - #[local] - Local(&'static str), - } -} - -#[allow(unused)] -struct Cipher { - key: Option, - name: EncString, -} - -#[derive(Clone)] -struct CipherView { - key: Option, - name: String, -} - -impl UsesKey for Cipher { - fn uses_key(&self) -> MySymmKeyRef { - MySymmKeyRef::User - } -} -impl UsesKey for CipherView { - fn uses_key(&self) -> MySymmKeyRef { - MySymmKeyRef::User - } -} - -const CIPHER_KEY: MySymmKeyRef = MySymmKeyRef::Local("cipher_key"); - -// New encryptable implementations - -impl Encryptable for CipherView { - fn encrypt( - &self, - ctx: &mut CryptoServiceContext, - key: MySymmKeyRef, - ) -> Result { - let cipher_key = match &self.key { - Some(cipher_key) => { - ctx.decrypt_symmetric_key_with_symmetric_key(key, CIPHER_KEY, cipher_key)? - } - None => key, - }; - - Ok(Cipher { - key: self.key.clone(), - name: self.name.as_str().encrypt(ctx, cipher_key)?, - }) - } -} - -// Old encryptable implementations - -impl KeyEncryptable for CipherView { - fn encrypt_with_key(self, key: &SymmetricCryptoKey) -> Result { - let cipher_key = self - .key - .as_ref() - .map(|k| { - let mut kk: Vec = k.decrypt_with_key(key)?; - SymmetricCryptoKey::try_from(kk.as_mut_slice()) - }) - .transpose()?; - - let key = cipher_key.as_ref().unwrap_or(key); - - Ok(Cipher { - key: self.key.clone(), - name: self.name.encrypt_with_key(key)?, - }) - } -} diff --git a/crates/memory-testing/Dockerfile b/crates/memory-testing/Dockerfile index 0565f031d..3df22466d 100644 --- a/crates/memory-testing/Dockerfile +++ b/crates/memory-testing/Dockerfile @@ -16,13 +16,13 @@ RUN mkdir -p /app/crates/bitwarden-crypto/src \ && touch /app/crates/bitwarden-crypto/src/lib.rs \ /app/crates/bitwarden-crypto/benches/default_allocator.rs \ /app/crates/bitwarden-crypto/benches/zeroizing_allocator.rs \ - /app/crates/bitwarden-crypto/benches/new_encryptable.rs \ && echo 'fn main(){}' > /app/crates/memory-testing/src/main.rs \ && cargo build -p memory-testing --release # Delete dummy files and copy the actual source code RUN rm /app/crates/bitwarden-crypto/src/lib.rs \ - /app/crates/bitwarden-crypto/benches/*.rs \ + /app/crates/bitwarden-crypto/benches/default_allocator.rs \ + /app/crates/bitwarden-crypto/benches/zeroizing_allocator.rs \ /app/crates/memory-testing/src/main.rs COPY crates/bitwarden-crypto /app/crates/bitwarden-crypto From bec4786ec629c146e8c9531d5c5994dfd0b87806 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garci=CC=81a?= Date: Mon, 7 Oct 2024 17:42:03 +0200 Subject: [PATCH 31/34] Remove using --- .../bitwarden-crypto/src/keys/encryptable.rs | 55 +------------------ crates/bitwarden-crypto/src/keys/mod.rs | 2 +- 2 files changed, 3 insertions(+), 54 deletions(-) diff --git a/crates/bitwarden-crypto/src/keys/encryptable.rs b/crates/bitwarden-crypto/src/keys/encryptable.rs index 5b51f25e6..66762a641 100644 --- a/crates/bitwarden-crypto/src/keys/encryptable.rs +++ b/crates/bitwarden-crypto/src/keys/encryptable.rs @@ -1,63 +1,12 @@ use super::key_ref::{AsymmetricKeyRef, KeyRef, SymmetricKeyRef}; use crate::{service::CryptoServiceContext, AsymmetricEncString, CryptoError, EncString}; -// Just like LocateKey but this time we're not locating anything, just returning a ref - +/// This trait should be implemented by any struct capable of knowing which key it needs +/// to encrypt or decrypt itself. pub trait UsesKey { fn uses_key(&self) -> Key; } -// This extension trait allows any type to be wrapped with `UsingKey` -// to make it easy to encrypt/decrypt it with the desired key, -// this way we don't need to have a separate `encrypt_with_key` function -pub trait UsingKeyExt: Sized { - fn using_key(self, key: Key) -> UsingKey { - UsingKey { key, value: self } - } -} -impl UsingKeyExt for T {} -pub struct UsingKey { - key: Key, - value: T, -} -impl UsesKey for UsingKey { - fn uses_key(&self) -> Key { - self.key - } -} -impl< - SymmKeyRef: SymmetricKeyRef, - AsymmKeyRef: AsymmetricKeyRef, - Key: KeyRef, - T: Encryptable, - Output, - > Encryptable for UsingKey -{ - fn encrypt( - &self, - ctx: &mut CryptoServiceContext, - _key: Key, - ) -> Result { - self.value.encrypt(ctx, self.key) - } -} -impl< - SymmKeyRef: SymmetricKeyRef, - AsymmKeyRef: AsymmetricKeyRef, - Key: KeyRef, - T: Decryptable, - Output, - > Decryptable for UsingKey -{ - fn decrypt( - &self, - ctx: &mut CryptoServiceContext, - _key: Key, - ) -> Result { - self.value.decrypt(ctx, self.key) - } -} - pub trait Encryptable< SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef, diff --git a/crates/bitwarden-crypto/src/keys/mod.rs b/crates/bitwarden-crypto/src/keys/mod.rs index 1267fd4fb..a75d9dba0 100644 --- a/crates/bitwarden-crypto/src/keys/mod.rs +++ b/crates/bitwarden-crypto/src/keys/mod.rs @@ -1,7 +1,7 @@ mod key_encryptable; pub use key_encryptable::{CryptoKey, KeyContainer, KeyDecryptable, KeyEncryptable, LocateKey}; mod encryptable; -pub use encryptable::{Decryptable, Encryptable, UsesKey, UsingKey, UsingKeyExt}; +pub use encryptable::{Decryptable, Encryptable, UsesKey}; pub mod key_ref; pub(crate) use key_ref::KeyRef; pub use key_ref::{AsymmetricKeyRef, SymmetricKeyRef}; From c2ffea62f1816e35f7b6688d136907c37948395c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garci=CC=81a?= Date: Tue, 8 Oct 2024 12:40:04 +0200 Subject: [PATCH 32/34] Fix TODO --- crates/bitwarden-crypto/src/error.rs | 2 ++ crates/bitwarden-crypto/src/service/context.rs | 5 +---- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/crates/bitwarden-crypto/src/error.rs b/crates/bitwarden-crypto/src/error.rs index 1496f144d..1fcd967b8 100644 --- a/crates/bitwarden-crypto/src/error.rs +++ b/crates/bitwarden-crypto/src/error.rs @@ -25,6 +25,8 @@ pub enum CryptoError { MissingField(&'static str), #[error("Missing Key for Ref. {0}")] MissingKey2(String), + #[error("Crypto store is read-only")] + ReadOnlyCryptoStore, #[error("Insufficient KDF parameters")] InsufficientKdfParameters, diff --git a/crates/bitwarden-crypto/src/service/context.rs b/crates/bitwarden-crypto/src/service/context.rs index 422cc7d6d..b1102a3df 100644 --- a/crates/bitwarden-crypto/src/service/context.rs +++ b/crates/bitwarden-crypto/src/service/context.rs @@ -27,10 +27,7 @@ impl<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> } fn get_mut(&mut self) -> Result<&mut Keys> { - // TODO: This should be a custom error - Err(crate::CryptoError::MissingKey2( - "Cannot modify in read-only mode".to_string(), - )) + Err(crate::CryptoError::ReadOnlyCryptoStore) } } From c0d2b63841b983a799c9cb901b75ecdbf0a2d21d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garci=CC=81a?= Date: Tue, 8 Oct 2024 12:41:48 +0200 Subject: [PATCH 33/34] Comment --- crates/bitwarden-crypto/src/service/context.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/crates/bitwarden-crypto/src/service/context.rs b/crates/bitwarden-crypto/src/service/context.rs index b1102a3df..9b75135aa 100644 --- a/crates/bitwarden-crypto/src/service/context.rs +++ b/crates/bitwarden-crypto/src/service/context.rs @@ -10,6 +10,10 @@ use crate::{ AsymmetricCryptoKey, AsymmetricEncString, CryptoError, EncString, Result, SymmetricCryptoKey, }; +// This is to abstract over the read-only and read-write access to the global keys +// inside the CryptoServiceContext. The read-write access should only be used internally +// in this crate to avoid users leaving the crypto store in an inconsistent state, +// but for the moment we have some operations that require access to it. pub trait GlobalAccessMode<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> { fn get(&self) -> &Keys; fn get_mut(&mut self) -> Result<&mut Keys>; From f4ca81639dac4c385166159b8789072b2010fac1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garci=CC=81a?= Date: Tue, 8 Oct 2024 12:47:45 +0200 Subject: [PATCH 34/34] Fmt --- crates/bitwarden-crypto/src/service/context.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bitwarden-crypto/src/service/context.rs b/crates/bitwarden-crypto/src/service/context.rs index 9b75135aa..3e966ccbd 100644 --- a/crates/bitwarden-crypto/src/service/context.rs +++ b/crates/bitwarden-crypto/src/service/context.rs @@ -11,7 +11,7 @@ use crate::{ }; // This is to abstract over the read-only and read-write access to the global keys -// inside the CryptoServiceContext. The read-write access should only be used internally +// inside the CryptoServiceContext. The read-write access should only be used internally // in this crate to avoid users leaving the crypto store in an inconsistent state, // but for the moment we have some operations that require access to it. pub trait GlobalAccessMode<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> {