From 538b9f20d157a0bc124effa0cd612d08c453ef8d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa?= Date: Mon, 1 Apr 2024 16:03:50 +0200 Subject: [PATCH] Add sensitive types to SymmetricCryptoKey (#672) ## Type of change ``` - [ ] Bug fix - [ ] New feature development - [x] Tech debt (refactoring, code cleanup, dependency upgrades, etc) - [ ] Build/deploy pipeline (DevOps) - [ ] Other ``` ## Objective This is a small subset of #536, which only contains the minimum code to protect the import/export functions on SymmetricCryptoKey. It also enables the from_base64 test on SymmetricCryptoKey as that passes now. After this PR is merged I'll be expanding the use of Sensitive to other parts of the codebase while adding some extra memory testing checks to validate that it works for them. --- .../src/enc_string/symmetric.rs | 16 +- .../bitwarden-crypto/src/keys/device_key.rs | 34 ++- .../bitwarden-crypto/src/keys/master_key.rs | 2 +- crates/bitwarden-crypto/src/keys/mod.rs | 2 +- .../src/keys/shareable_key.rs | 4 +- .../src/keys/symmetric_crypto_key.rs | 53 +++-- crates/bitwarden-crypto/src/lib.rs | 2 + .../src/sensitive/decrypted.rs | 16 ++ crates/bitwarden-crypto/src/sensitive/mod.rs | 5 + .../src/sensitive/sensitive.rs | 220 ++++++++++++++++++ crates/bitwarden-crypto/src/uniffi_support.rs | 25 +- crates/bitwarden-uniffi/src/crypto.rs | 4 +- crates/bitwarden-uniffi/src/uniffi_support.rs | 3 +- crates/bitwarden/src/auth/access_token.rs | 2 +- crates/bitwarden/src/auth/auth_request.rs | 14 +- .../bitwarden/src/auth/login/access_token.rs | 12 +- crates/bitwarden/src/auth/tde.rs | 2 +- crates/bitwarden/src/mobile/client_crypto.rs | 4 +- crates/bitwarden/src/mobile/crypto.rs | 25 +- crates/bitwarden/src/secrets_manager/state.rs | 6 +- crates/bitwarden/src/uniffi_support.rs | 7 +- .../bitwarden/src/vault/cipher/attachment.rs | 11 +- crates/bitwarden/src/vault/cipher/cipher.rs | 2 +- crates/bitwarden/src/vault/send.rs | 2 +- .../memory-testing/src/bin/analyze-dumps.rs | 4 +- crates/memory-testing/src/main.rs | 9 +- 26 files changed, 381 insertions(+), 105 deletions(-) create mode 100644 crates/bitwarden-crypto/src/sensitive/decrypted.rs create mode 100644 crates/bitwarden-crypto/src/sensitive/mod.rs create mode 100644 crates/bitwarden-crypto/src/sensitive/sensitive.rs diff --git a/crates/bitwarden-crypto/src/enc_string/symmetric.rs b/crates/bitwarden-crypto/src/enc_string/symmetric.rs index d5489fc96..0217a4b93 100644 --- a/crates/bitwarden-crypto/src/enc_string/symmetric.rs +++ b/crates/bitwarden-crypto/src/enc_string/symmetric.rs @@ -290,14 +290,16 @@ mod tests { use schemars::schema_for; use super::EncString; - use crate::{derive_symmetric_key, KeyDecryptable, KeyEncryptable, SymmetricCryptoKey}; + use crate::{ + derive_symmetric_key, KeyDecryptable, KeyEncryptable, SensitiveString, SymmetricCryptoKey, + }; #[test] fn test_enc_string_roundtrip() { let key = derive_symmetric_key("test"); - let test_string = "encrypted_test_string".to_string(); - let cipher = test_string.clone().encrypt_with_key(&key).unwrap(); + let test_string = "encrypted_test_string"; + let cipher = test_string.to_owned().encrypt_with_key(&key).unwrap(); let decrypted_str: String = cipher.decrypt_with_key(&key).unwrap(); assert_eq!(decrypted_str, test_string); @@ -390,8 +392,8 @@ mod tests { #[test] fn test_decrypt_cbc256() { - let key = "hvBMMb1t79YssFZkpetYsM3deyVuQv4r88Uj9gvYe08="; - let key: SymmetricCryptoKey = key.parse().unwrap(); + let key = SensitiveString::test("hvBMMb1t79YssFZkpetYsM3deyVuQv4r88Uj9gvYe08="); + let key = SymmetricCryptoKey::try_from(key).unwrap(); let enc_str = "0.NQfjHLr6za7VQVAbrpL81w==|wfrjmyJ0bfwkQlySrhw8dA=="; let enc_string: EncString = enc_str.parse().unwrap(); @@ -403,8 +405,8 @@ mod tests { #[test] fn test_decrypt_cbc128_hmac() { - let key = "Gt1aZ8kTTgkF80bLtb7LiMZBcxEA2FA5mbvV4x7K208="; - let key: SymmetricCryptoKey = key.parse().unwrap(); + let key = SensitiveString::test("Gt1aZ8kTTgkF80bLtb7LiMZBcxEA2FA5mbvV4x7K208="); + let key = SymmetricCryptoKey::try_from(key).unwrap(); let enc_str = "1.CU/oG4VZuxbHoZSDZjCLQw==|kb1HGwAk+fQ275ORfLf5Ew==|8UaEYHyqRZcG37JWhYBOBdEatEXd1u1/wN7OuImolcM="; let enc_string: EncString = enc_str.parse().unwrap(); diff --git a/crates/bitwarden-crypto/src/keys/device_key.rs b/crates/bitwarden-crypto/src/keys/device_key.rs index 876ae7b75..d33e0dc32 100644 --- a/crates/bitwarden-crypto/src/keys/device_key.rs +++ b/crates/bitwarden-crypto/src/keys/device_key.rs @@ -1,8 +1,6 @@ -use std::str::FromStr; - use crate::{ - error::Result, AsymmetricCryptoKey, AsymmetricEncString, CryptoError, EncString, - KeyDecryptable, KeyEncryptable, SymmetricCryptoKey, + error::Result, AsymmetricCryptoKey, AsymmetricEncString, CryptoError, DecryptedVec, EncString, + KeyDecryptable, KeyEncryptable, SensitiveString, SymmetricCryptoKey, }; /// Device Key @@ -16,7 +14,7 @@ pub struct DeviceKey(SymmetricCryptoKey); #[cfg_attr(feature = "mobile", derive(uniffi::Record))] pub struct TrustDeviceResponse { /// Base64 encoded device key - pub device_key: String, + pub device_key: SensitiveString, /// UserKey encrypted with DevicePublicKey pub protected_user_key: AsymmetricEncString, /// DevicePrivateKey encrypted with [DeviceKey] @@ -40,7 +38,7 @@ impl DeviceKey { let data = user_key.to_vec(); let protected_user_key = - AsymmetricEncString::encrypt_rsa2048_oaep_sha1(&data, &device_private_key)?; + AsymmetricEncString::encrypt_rsa2048_oaep_sha1(data.expose(), &device_private_key)?; let protected_device_public_key = device_private_key .to_public_der()? @@ -65,25 +63,25 @@ impl DeviceKey { protected_user_key: AsymmetricEncString, ) -> Result { let device_private_key: Vec = protected_device_private_key.decrypt_with_key(&self.0)?; - let device_private_key = AsymmetricCryptoKey::from_der(device_private_key.as_slice())?; + let device_private_key = AsymmetricCryptoKey::from_der(&device_private_key)?; - let mut dec: Vec = protected_user_key.decrypt_with_key(&device_private_key)?; - let user_key: SymmetricCryptoKey = dec.as_mut_slice().try_into()?; + let dec: Vec = protected_user_key.decrypt_with_key(&device_private_key)?; + let dec = DecryptedVec::new(Box::new(dec)); + let user_key = SymmetricCryptoKey::try_from(dec)?; Ok(user_key) } - fn to_base64(&self) -> String { + fn to_base64(&self) -> SensitiveString { self.0.to_base64() } } -impl FromStr for DeviceKey { - type Err = CryptoError; +impl TryFrom for DeviceKey { + type Error = CryptoError; - fn from_str(s: &str) -> Result { - let key = s.parse::()?; - Ok(DeviceKey(key)) + fn try_from(value: SensitiveString) -> Result { + SymmetricCryptoKey::try_from(value).map(DeviceKey) } } @@ -98,10 +96,8 @@ mod tests { let result = DeviceKey::trust_device(&key).unwrap(); - let decrypted = result - .device_key - .parse::() - .unwrap() + let device_key = DeviceKey::try_from(result.device_key).unwrap(); + let decrypted = device_key .decrypt_user_key( result.protected_device_private_key, result.protected_user_key, diff --git a/crates/bitwarden-crypto/src/keys/master_key.rs b/crates/bitwarden-crypto/src/keys/master_key.rs index 8e6d2575b..b6921fc6b 100644 --- a/crates/bitwarden-crypto/src/keys/master_key.rs +++ b/crates/bitwarden-crypto/src/keys/master_key.rs @@ -67,7 +67,7 @@ impl MasterKey { let stretched_key = stretch_kdf_key(&self.0)?; EncString::encrypt_aes256_hmac( - user_key.to_vec().as_slice(), + user_key.to_vec().expose(), stretched_key .mac_key .as_ref() diff --git a/crates/bitwarden-crypto/src/keys/mod.rs b/crates/bitwarden-crypto/src/keys/mod.rs index 5bfd32b8b..435f1011b 100644 --- a/crates/bitwarden-crypto/src/keys/mod.rs +++ b/crates/bitwarden-crypto/src/keys/mod.rs @@ -1,5 +1,5 @@ mod key_encryptable; -pub use key_encryptable::{KeyDecryptable, KeyEncryptable}; +pub use key_encryptable::{CryptoKey, KeyDecryptable, KeyEncryptable}; mod master_key; pub use master_key::{HashPurpose, Kdf, MasterKey}; mod shareable_key; diff --git a/crates/bitwarden-crypto/src/keys/shareable_key.rs b/crates/bitwarden-crypto/src/keys/shareable_key.rs index fbb2d2e44..2b9ec837f 100644 --- a/crates/bitwarden-crypto/src/keys/shareable_key.rs +++ b/crates/bitwarden-crypto/src/keys/shareable_key.rs @@ -37,9 +37,9 @@ mod tests { #[test] fn test_derive_shareable_key() { let key = derive_shareable_key(*b"&/$%F1a895g67HlX", "test_key", None); - assert_eq!(key.to_base64(), "4PV6+PcmF2w7YHRatvyMcVQtI7zvCyssv/wFWmzjiH6Iv9altjmDkuBD1aagLVaLezbthbSe+ktR+U6qswxNnQ=="); + assert_eq!(key.to_base64().expose(), "4PV6+PcmF2w7YHRatvyMcVQtI7zvCyssv/wFWmzjiH6Iv9altjmDkuBD1aagLVaLezbthbSe+ktR+U6qswxNnQ=="); let key = derive_shareable_key(*b"67t9b5g67$%Dh89n", "test_key", Some("test")); - assert_eq!(key.to_base64(), "F9jVQmrACGx9VUPjuzfMYDjr726JtL300Y3Yg+VYUnVQtQ1s8oImJ5xtp1KALC9h2nav04++1LDW4iFD+infng=="); + assert_eq!(key.to_base64().expose(), "F9jVQmrACGx9VUPjuzfMYDjr726JtL300Y3Yg+VYUnVQtQ1s8oImJ5xtp1KALC9h2nav04++1LDW4iFD+infng=="); } } diff --git a/crates/bitwarden-crypto/src/keys/symmetric_crypto_key.rs b/crates/bitwarden-crypto/src/keys/symmetric_crypto_key.rs index 0c165bb40..23d09cbca 100644 --- a/crates/bitwarden-crypto/src/keys/symmetric_crypto_key.rs +++ b/crates/bitwarden-crypto/src/keys/symmetric_crypto_key.rs @@ -1,13 +1,13 @@ -use std::{pin::Pin, str::FromStr}; +use std::pin::Pin; use aes::cipher::typenum::U32; -use base64::{engine::general_purpose::STANDARD, Engine}; +use base64::engine::general_purpose::STANDARD; use generic_array::GenericArray; use rand::Rng; -use zeroize::{Zeroize, Zeroizing}; +use zeroize::Zeroize; use super::key_encryptable::CryptoKey; -use crate::CryptoError; +use crate::{CryptoError, SensitiveString, SensitiveVec}; /// A symmetric encryption key. Used to encrypt and decrypt [`EncString`](crate::EncString) pub struct SymmetricCryptoKey { @@ -59,31 +59,35 @@ impl SymmetricCryptoKey { self.key.len() + self.mac_key.as_ref().map_or(0, |mac| mac.len()) } - pub fn to_base64(&self) -> String { - let mut buf = self.to_vec(); - - let result = STANDARD.encode(&buf); - buf.zeroize(); - result + pub fn to_base64(&self) -> SensitiveString { + self.to_vec().encode_base64(STANDARD) } - pub fn to_vec(&self) -> Zeroizing> { - let mut buf = Vec::with_capacity(self.total_len()); + pub fn to_vec(&self) -> SensitiveVec { + let mut buf = SensitiveVec::new(Box::new(Vec::with_capacity(self.total_len()))); - buf.extend_from_slice(&self.key); + buf.expose_mut().extend_from_slice(&self.key); if let Some(mac) = &self.mac_key { - buf.extend_from_slice(mac); + buf.expose_mut().extend_from_slice(mac); } - Zeroizing::new(buf) + buf } } -impl FromStr for SymmetricCryptoKey { - type Err = CryptoError; +impl TryFrom for SymmetricCryptoKey { + type Error = CryptoError; - fn from_str(s: &str) -> Result { - let mut bytes = STANDARD.decode(s).map_err(|_| CryptoError::InvalidKey)?; - SymmetricCryptoKey::try_from(bytes.as_mut_slice()) + fn try_from(value: SensitiveString) -> Result { + SymmetricCryptoKey::try_from(value.decode_base64(STANDARD)?) + } +} + +impl TryFrom for SymmetricCryptoKey { + type Error = CryptoError; + + fn try_from(mut value: SensitiveVec) -> Result { + let val = value.expose_mut(); + SymmetricCryptoKey::try_from(val.as_mut_slice()) } } @@ -138,19 +142,18 @@ pub fn derive_symmetric_key(name: &str) -> SymmetricCryptoKey { #[cfg(test)] mod tests { - use std::str::FromStr; - use super::{derive_symmetric_key, SymmetricCryptoKey}; + use crate::SensitiveString; #[test] fn test_symmetric_crypto_key() { let key = derive_symmetric_key("test"); - let key2 = SymmetricCryptoKey::from_str(&key.to_base64()).unwrap(); + let key2 = SymmetricCryptoKey::try_from(key.to_base64()).unwrap(); assert_eq!(key.key, key2.key); assert_eq!(key.mac_key, key2.mac_key); - let key = "UY4B5N4DA4UisCNClgZtRr6VLy9ZF5BXXC7cDZRqourKi4ghEMgISbCsubvgCkHf5DZctQjVot11/vVvN9NNHQ=="; - let key2 = SymmetricCryptoKey::from_str(key).unwrap(); + let key = SensitiveString::test("UY4B5N4DA4UisCNClgZtRr6VLy9ZF5BXXC7cDZRqourKi4ghEMgISbCsubvgCkHf5DZctQjVot11/vVvN9NNHQ=="); + let key2 = SymmetricCryptoKey::try_from(key.clone()).unwrap(); assert_eq!(key, key2.to_base64()); } } diff --git a/crates/bitwarden-crypto/src/lib.rs b/crates/bitwarden-crypto/src/lib.rs index 3c1aced24..8c368580a 100644 --- a/crates/bitwarden-crypto/src/lib.rs +++ b/crates/bitwarden-crypto/src/lib.rs @@ -40,6 +40,8 @@ pub use util::generate_random_bytes; mod wordlist; pub use util::pbkdf2; pub use wordlist::EFF_LONG_WORD_LIST; +mod sensitive; +pub use sensitive::*; #[cfg(feature = "mobile")] uniffi::setup_scaffolding!(); diff --git a/crates/bitwarden-crypto/src/sensitive/decrypted.rs b/crates/bitwarden-crypto/src/sensitive/decrypted.rs new file mode 100644 index 000000000..02096d081 --- /dev/null +++ b/crates/bitwarden-crypto/src/sensitive/decrypted.rs @@ -0,0 +1,16 @@ +use zeroize::Zeroize; + +use crate::{CryptoError, CryptoKey, KeyEncryptable, Sensitive}; + +/// Type alias for a [`Sensitive`] value to denote decrypted data. +pub type Decrypted = Sensitive; +pub type DecryptedVec = Decrypted>; +pub type DecryptedString = Decrypted; + +impl + Zeroize + Clone, Key: CryptoKey, Output> + KeyEncryptable for Decrypted +{ + fn encrypt_with_key(self, key: &Key) -> Result { + self.value.clone().encrypt_with_key(key) + } +} diff --git a/crates/bitwarden-crypto/src/sensitive/mod.rs b/crates/bitwarden-crypto/src/sensitive/mod.rs new file mode 100644 index 000000000..0da2329b9 --- /dev/null +++ b/crates/bitwarden-crypto/src/sensitive/mod.rs @@ -0,0 +1,5 @@ +#[allow(clippy::module_inception)] +mod sensitive; +pub use sensitive::{Sensitive, SensitiveString, SensitiveVec}; +mod decrypted; +pub use decrypted::{Decrypted, DecryptedString, DecryptedVec}; diff --git a/crates/bitwarden-crypto/src/sensitive/sensitive.rs b/crates/bitwarden-crypto/src/sensitive/sensitive.rs new file mode 100644 index 000000000..996b20330 --- /dev/null +++ b/crates/bitwarden-crypto/src/sensitive/sensitive.rs @@ -0,0 +1,220 @@ +use std::{ + borrow::Cow, + fmt::{self, Formatter}, +}; + +use schemars::JsonSchema; +use serde::{Deserialize, Deserializer, Serialize, Serializer}; +use zeroize::{Zeroize, ZeroizeOnDrop}; + +use crate::CryptoError; + +/// Wrapper for sensitive values which makes a best effort to enforce zeroization of the inner value +/// on drop. The inner value exposes a [`Sensitive::expose`] method which returns a reference to the +/// inner value. Care must be taken to avoid accidentally exposing the inner value through copying +/// or cloning. +/// +/// Internally [`Sensitive`] contains a [`Box`] which ensures the value is placed on the heap. It +/// implements the [`Drop`] trait which calls `zeroize` on the inner value. +#[derive(PartialEq, Clone, Zeroize, ZeroizeOnDrop)] +pub struct Sensitive { + pub(super) value: Box, +} + +/// Important: This type does not protect against reallocations made by the Vec. +/// This means that if you insert any elements past the capacity, the data will be copied to a +/// new allocation and the old allocation will not be zeroized. +/// To avoid this, use Vec::with_capacity to preallocate the capacity you need. +pub type SensitiveVec = Sensitive>; + +/// Important: This type does not protect against reallocations made by the String. +/// This means that if you insert any characters past the capacity, the data will be copied to a +/// new allocation and the old allocation will not be zeroized. +/// To avoid this, use String::with_capacity to preallocate the capacity you need. +pub type SensitiveString = Sensitive; + +impl Sensitive { + /// Create a new [`Sensitive`] value. In an attempt to avoid accidentally placing this on the + /// stack it only accepts a [`Box`] value. The rust compiler should be able to optimize away the + /// initial stack allocation presuming the value is not used before being boxed. + pub fn new(value: Box) -> Self { + Self { value } + } + + /// Expose the inner value. By exposing the inner value, you take responsibility for ensuring + /// that any copy of the value is zeroized. + pub fn expose(&self) -> &V { + &self.value + } + + /// Expose the inner value mutable. By exposing the inner value, you take responsibility for + /// ensuring that any copy of the value is zeroized. + pub fn expose_mut(&mut self) -> &mut V { + &mut self.value + } +} + +/// Helper to convert a `Sensitive>` to a `Sensitive`, care is taken to ensure any +/// intermediate copies are zeroed to avoid leaking sensitive data. +impl TryFrom for SensitiveString { + type Error = CryptoError; + + fn try_from(mut v: SensitiveVec) -> Result { + let value = std::mem::take(&mut v.value); + + let rtn = String::from_utf8(*value).map_err(|_| CryptoError::InvalidUtf8String); + rtn.map(|v| Sensitive::new(Box::new(v))) + } +} + +impl SensitiveString { + pub fn decode_base64(self, engine: T) -> Result { + // Prevent accidental copies by allocating the full size + let len = base64::decoded_len_estimate(self.value.len()); + let mut value = SensitiveVec::new(Box::new(Vec::with_capacity(len))); + + engine + .decode_vec(self.value.as_ref(), &mut value.value) + .map_err(|_| CryptoError::InvalidKey)?; + + Ok(value) + } +} + +impl SensitiveVec { + pub fn encode_base64(self, engine: T) -> SensitiveString { + use base64::engine::Config; + + // Prevent accidental copies by allocating the full size + let padding = engine.config().encode_padding(); + let len = base64::encoded_len(self.value.len(), padding).expect("Valid length"); + let mut value = SensitiveString::new(Box::new(String::with_capacity(len))); + + engine.encode_string(self.value.as_ref(), &mut value.value); + + value + } +} + +impl Default for Sensitive { + fn default() -> Self { + Self::new(Box::default()) + } +} + +impl fmt::Debug for Sensitive { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + f.debug_struct("Sensitive") + .field("type", &std::any::type_name::()) + .field("value", &"********") + .finish() + } +} + +/// Unfortunately once we serialize a `SensitiveString` we can't control the future memory. +impl Serialize for Sensitive { + fn serialize(&self, serializer: S) -> Result { + self.value.serialize(serializer) + } +} + +impl<'de, V: Zeroize + Deserialize<'de>> Deserialize<'de> for Sensitive { + fn deserialize>(deserializer: D) -> Result { + Ok(Self::new(Box::new(V::deserialize(deserializer)?))) + } +} + +/// Transparently expose the inner value for serialization +impl JsonSchema for Sensitive { + fn schema_name() -> String { + V::schema_name() + } + + fn schema_id() -> Cow<'static, str> { + V::schema_id() + } + + fn json_schema(gen: &mut schemars::gen::SchemaGenerator) -> schemars::schema::Schema { + V::json_schema(gen) + } +} + +impl Sensitive { + // We use a lot of `&str` in our tests, so we expose this helper + // to make it easier. + // IMPORTANT: This should not be used outside of test code + // Note that we can't just mark it with #[cfg(test)] because that only applies + // when testing this crate, not when testing other crates that depend on it. + // By at least limiting it to &'static str we should be able to avoid accidental usages + pub fn test(value: &'static str) -> Self { + Self::new(Box::new(value.to_string())) + } +} + +#[cfg(test)] +mod tests { + use schemars::schema_for; + + use super::*; + + #[test] + fn test_debug() { + let string = Sensitive::test("test"); + assert_eq!( + format!("{:?}", string), + "Sensitive { type: \"alloc::string::String\", value: \"********\" }" + ); + + let vector = Sensitive::new(Box::new(vec![1, 2, 3])); + assert_eq!( + format!("{:?}", vector), + "Sensitive { type: \"alloc::vec::Vec\", value: \"********\" }" + ); + } + + #[test] + fn test_schemars() { + #[derive(JsonSchema)] + struct TestStruct { + #[allow(dead_code)] + s: SensitiveString, + #[allow(dead_code)] + v: SensitiveVec, + } + + let schema = schema_for!(TestStruct); + let json = serde_json::to_string_pretty(&schema).unwrap(); + let expected = r##"{ + "$schema": "http://json-schema.org/draft-07/schema#", + "title": "TestStruct", + "type": "object", + "required": ["s", "v"], + "properties": { + "s": { + "$ref": "#/definitions/String" + }, + "v": { + "$ref": "#/definitions/Array_of_uint8" + } + }, + "definitions": { + "Array_of_uint8": { + "type": "array", + "items": { + "type": "integer", + "format": "uint8", + "minimum": 0.0 + } + }, + "String": { + "type": "string" + } + } + }"##; + + assert_eq!( + json.parse::().unwrap(), + expected.parse::().unwrap() + ); + } +} diff --git a/crates/bitwarden-crypto/src/uniffi_support.rs b/crates/bitwarden-crypto/src/uniffi_support.rs index 7f1249da0..f99b55da5 100644 --- a/crates/bitwarden-crypto/src/uniffi_support.rs +++ b/crates/bitwarden-crypto/src/uniffi_support.rs @@ -1,6 +1,8 @@ use std::{num::NonZeroU32, str::FromStr}; -use crate::{AsymmetricEncString, CryptoError, EncString, UniffiCustomTypeConverter}; +use crate::{ + AsymmetricEncString, CryptoError, EncString, SensitiveString, UniffiCustomTypeConverter, +}; uniffi::custom_type!(NonZeroU32, u32); @@ -43,3 +45,24 @@ impl UniffiCustomTypeConverter for AsymmetricEncString { obj.to_string() } } + +uniffi::custom_type!(SensitiveString, String); + +impl UniffiCustomTypeConverter for SensitiveString { + type Builtin = String; + + fn into_custom(val: Self::Builtin) -> uniffi::Result { + Ok(SensitiveString::new(Box::new(val))) + } + + fn from_custom(obj: Self) -> Self::Builtin { + obj.expose().to_owned() + } +} + +/// Uniffi doesn't seem to be generating the SensitiveString unless it's being used by +/// a record somewhere. This is a workaround to make sure the type is generated. +#[derive(uniffi::Record)] +struct SupportSensitiveString { + sensitive_string: SensitiveString, +} diff --git a/crates/bitwarden-uniffi/src/crypto.rs b/crates/bitwarden-uniffi/src/crypto.rs index 0f877d52e..f3abe694a 100644 --- a/crates/bitwarden-uniffi/src/crypto.rs +++ b/crates/bitwarden-uniffi/src/crypto.rs @@ -3,7 +3,7 @@ use std::sync::Arc; use bitwarden::mobile::crypto::{ DerivePinKeyResponse, InitOrgCryptoRequest, InitUserCryptoRequest, UpdatePasswordResponse, }; -use bitwarden_crypto::{AsymmetricEncString, EncString}; +use bitwarden_crypto::{AsymmetricEncString, EncString, SensitiveString}; use crate::{error::Result, Client}; @@ -40,7 +40,7 @@ impl ClientCrypto { /// Get the uses's decrypted encryption key. Note: It's very important /// to keep this key safe, as it can be used to decrypt all of the user's data - pub async fn get_user_encryption_key(&self) -> Result { + pub async fn get_user_encryption_key(&self) -> Result { Ok(self .0 .0 diff --git a/crates/bitwarden-uniffi/src/uniffi_support.rs b/crates/bitwarden-uniffi/src/uniffi_support.rs index 663d5c41e..a49e347b6 100644 --- a/crates/bitwarden-uniffi/src/uniffi_support.rs +++ b/crates/bitwarden-uniffi/src/uniffi_support.rs @@ -1,7 +1,8 @@ -use bitwarden_crypto::{AsymmetricEncString, EncString}; +use bitwarden_crypto::{AsymmetricEncString, EncString, SensitiveString}; // Forward the type definitions to the main bitwarden crate type DateTime = chrono::DateTime; uniffi::ffi_converter_forward!(DateTime, bitwarden::UniFfiTag, crate::UniFfiTag); uniffi::ffi_converter_forward!(EncString, bitwarden::UniFfiTag, crate::UniFfiTag); uniffi::ffi_converter_forward!(AsymmetricEncString, bitwarden::UniFfiTag, crate::UniFfiTag); +uniffi::ffi_converter_forward!(SensitiveString, bitwarden::UniFfiTag, crate::UniFfiTag); diff --git a/crates/bitwarden/src/auth/access_token.rs b/crates/bitwarden/src/auth/access_token.rs index 667a78529..4b6f050c8 100644 --- a/crates/bitwarden/src/auth/access_token.rs +++ b/crates/bitwarden/src/auth/access_token.rs @@ -79,7 +79,7 @@ mod tests { "ec2c1d46-6a4b-4751-a310-af9601317f2d" ); assert_eq!(token.client_secret, "C2IgxjjLF7qSshsbwe8JGcbM075YXw"); - assert_eq!(token.encryption_key.to_base64(), "H9/oIRLtL9nGCQOVDjSMoEbJsjWXSOCb3qeyDt6ckzS3FhyboEDWyTP/CQfbIszNmAVg2ExFganG1FVFGXO/Jg=="); + assert_eq!(token.encryption_key.to_base64().expose(), "H9/oIRLtL9nGCQOVDjSMoEbJsjWXSOCb3qeyDt6ckzS3FhyboEDWyTP/CQfbIszNmAVg2ExFganG1FVFGXO/Jg=="); } #[test] diff --git a/crates/bitwarden/src/auth/auth_request.rs b/crates/bitwarden/src/auth/auth_request.rs index 18c71afba..04dc9fdce 100644 --- a/crates/bitwarden/src/auth/auth_request.rs +++ b/crates/bitwarden/src/auth/auth_request.rs @@ -92,15 +92,13 @@ pub(crate) fn approve_auth_request( let key = enc.get_key(&None).ok_or(Error::VaultLocked)?; Ok(AsymmetricEncString::encrypt_rsa2048_oaep_sha1( - &key.to_vec(), + key.to_vec().expose(), &public_key, )?) } #[test] fn test_auth_request() { - use zeroize::Zeroizing; - let request = new_auth_request("test@bitwarden.com").unwrap(); let secret: &[u8] = &[ @@ -117,7 +115,7 @@ fn test_auth_request() { let decrypted = auth_request_decrypt_user_key(request.private_key, encrypted).unwrap(); - assert_eq!(decrypted.to_vec(), Zeroizing::new(secret.to_owned())); + assert_eq!(decrypted.to_vec().expose(), secret); } #[cfg(test)] @@ -167,8 +165,8 @@ mod tests { let dec = auth_request_decrypt_user_key(private_key.to_owned(), enc_user_key).unwrap(); assert_eq!( - dec.to_vec().as_ref(), - vec![ + dec.to_vec().expose(), + &[ 201, 37, 234, 213, 21, 75, 40, 70, 149, 213, 234, 16, 19, 251, 162, 245, 161, 74, 34, 245, 211, 151, 211, 192, 95, 10, 117, 50, 88, 223, 23, 157 ] @@ -186,8 +184,8 @@ mod tests { .unwrap(); assert_eq!( - dec.to_vec().as_ref(), - vec![ + dec.to_vec().expose(), + &[ 109, 128, 172, 147, 206, 123, 134, 95, 16, 36, 155, 113, 201, 18, 186, 230, 216, 212, 173, 188, 74, 11, 134, 131, 137, 242, 105, 178, 105, 126, 52, 139, 248, 91, 215, 21, 128, 91, 226, 222, 165, 67, 251, 34, 83, 81, 77, 147, 225, 76, 13, 41, diff --git a/crates/bitwarden/src/auth/login/access_token.rs b/crates/bitwarden/src/auth/login/access_token.rs index 46c3f2b50..a8c017009 100644 --- a/crates/bitwarden/src/auth/login/access_token.rs +++ b/crates/bitwarden/src/auth/login/access_token.rs @@ -1,7 +1,7 @@ use std::path::{Path, PathBuf}; -use base64::{engine::general_purpose::STANDARD, Engine}; -use bitwarden_crypto::{EncString, KeyDecryptable, SymmetricCryptoKey}; +use base64::engine::general_purpose::STANDARD; +use bitwarden_crypto::{EncString, KeyDecryptable, SensitiveString, SymmetricCryptoKey}; use chrono::Utc; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; @@ -59,12 +59,12 @@ pub(crate) async fn login_access_token( #[derive(serde::Deserialize)] struct Payload { #[serde(rename = "encryptionKey")] - encryption_key: String, + encryption_key: SensitiveString, } let payload: Payload = serde_json::from_slice(&decrypted_payload)?; - let mut encryption_key = STANDARD.decode(payload.encryption_key.clone())?; - let encryption_key = SymmetricCryptoKey::try_from(encryption_key.as_mut_slice())?; + let encryption_key = payload.encryption_key.clone().decode_base64(STANDARD)?; + let encryption_key = SymmetricCryptoKey::try_from(encryption_key)?; let access_token_obj: JWTToken = r.access_token.parse()?; @@ -125,7 +125,7 @@ fn load_tokens_from_state( let organization_id: Uuid = organization_id .parse() .map_err(|_| "Bad organization id.")?; - let encryption_key: SymmetricCryptoKey = client_state.encryption_key.parse()?; + let encryption_key = SymmetricCryptoKey::try_from(client_state.encryption_key)?; client.set_tokens(client_state.token, None, time_till_expiration as u64); client.initialize_crypto_single_key(encryption_key); diff --git a/crates/bitwarden/src/auth/tde.rs b/crates/bitwarden/src/auth/tde.rs index 1a3de3026..0bbbb904a 100644 --- a/crates/bitwarden/src/auth/tde.rs +++ b/crates/bitwarden/src/auth/tde.rs @@ -22,7 +22,7 @@ pub(super) fn make_register_tde_keys( let key_pair = user_key.make_key_pair()?; let admin_reset = - AsymmetricEncString::encrypt_rsa2048_oaep_sha1(&user_key.0.to_vec(), &public_key)?; + AsymmetricEncString::encrypt_rsa2048_oaep_sha1(user_key.0.to_vec().expose(), &public_key)?; let device_key = if remember_device { Some(DeviceKey::trust_device(&user_key.0)?) diff --git a/crates/bitwarden/src/mobile/client_crypto.rs b/crates/bitwarden/src/mobile/client_crypto.rs index 6ef65975d..26451ffaa 100644 --- a/crates/bitwarden/src/mobile/client_crypto.rs +++ b/crates/bitwarden/src/mobile/client_crypto.rs @@ -1,5 +1,5 @@ #[cfg(feature = "internal")] -use bitwarden_crypto::{AsymmetricEncString, EncString}; +use bitwarden_crypto::{AsymmetricEncString, EncString, SensitiveString}; use crate::Client; #[cfg(feature = "internal")] @@ -28,7 +28,7 @@ impl<'a> ClientCrypto<'a> { } #[cfg(feature = "internal")] - pub async fn get_user_encryption_key(&mut self) -> Result { + pub async fn get_user_encryption_key(&mut self) -> Result { get_user_encryption_key(self.client).await } diff --git a/crates/bitwarden/src/mobile/crypto.rs b/crates/bitwarden/src/mobile/crypto.rs index 214643023..b8891aa09 100644 --- a/crates/bitwarden/src/mobile/crypto.rs +++ b/crates/bitwarden/src/mobile/crypto.rs @@ -2,7 +2,9 @@ use std::collections::HashMap; use bitwarden_crypto::{AsymmetricEncString, EncString}; #[cfg(feature = "internal")] -use bitwarden_crypto::{KeyDecryptable, KeyEncryptable, MasterKey, SymmetricCryptoKey}; +use bitwarden_crypto::{ + KeyDecryptable, KeyEncryptable, MasterKey, SensitiveString, SymmetricCryptoKey, +}; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; @@ -86,7 +88,7 @@ pub enum AuthRequestMethod { #[cfg(feature = "internal")] pub async fn initialize_user_crypto(client: &mut Client, req: InitUserCryptoRequest) -> Result<()> { - use bitwarden_crypto::DeviceKey; + use bitwarden_crypto::{DecryptedString, DeviceKey}; use crate::auth::{auth_request_decrypt_master_key, auth_request_decrypt_user_key}; @@ -105,7 +107,8 @@ pub async fn initialize_user_crypto(client: &mut Client, req: InitUserCryptoRequ client.initialize_user_crypto(&password, user_key, private_key)?; } InitUserCryptoMethod::DecryptedKey { decrypted_user_key } => { - let user_key = decrypted_user_key.parse::()?; + let decrypted_user_key = DecryptedString::new(Box::new(decrypted_user_key)); + let user_key = SymmetricCryptoKey::try_from(decrypted_user_key)?; client.initialize_user_crypto_decrypted_key(user_key, private_key)?; } InitUserCryptoMethod::Pin { @@ -138,7 +141,8 @@ pub async fn initialize_user_crypto(client: &mut Client, req: InitUserCryptoRequ protected_device_private_key, device_protected_user_key, } => { - let device_key = device_key.parse::()?; + let device_key = DecryptedString::new(Box::new(device_key)); + let device_key = DeviceKey::try_from(device_key)?; let user_key = device_key .decrypt_user_key(protected_device_private_key, device_protected_user_key)?; @@ -166,7 +170,7 @@ pub async fn initialize_org_crypto(client: &mut Client, req: InitOrgCryptoReques } #[cfg(feature = "internal")] -pub async fn get_user_encryption_key(client: &mut Client) -> Result { +pub async fn get_user_encryption_key(client: &mut Client) -> Result { let user_key = client .get_encryption_settings()? .get_key(&None) @@ -299,7 +303,7 @@ pub(super) fn enroll_admin_password_reset( let key = enc.get_key(&None).ok_or(Error::VaultLocked)?; Ok(AsymmetricEncString::encrypt_rsa2048_oaep_sha1( - &key.to_vec(), + key.to_vec().expose(), &public_key, )?) } @@ -483,7 +487,7 @@ mod tests { #[cfg(feature = "internal")] #[test] fn test_enroll_admin_password_reset() { - use std::{num::NonZeroU32, ops::Deref}; + use std::num::NonZeroU32; use base64::{engine::general_purpose::STANDARD, Engine}; use bitwarden_crypto::AsymmetricCryptoKey; @@ -516,10 +520,7 @@ mod tests { .get_encryption_settings() .unwrap() .get_key(&None) - .unwrap() - .to_vec() - .deref() - .clone(); - assert_eq!(decrypted, expected); + .unwrap(); + assert_eq!(&decrypted, expected.to_vec().expose()); } } diff --git a/crates/bitwarden/src/secrets_manager/state.rs b/crates/bitwarden/src/secrets_manager/state.rs index b2b6f6a8e..c677ab7e9 100644 --- a/crates/bitwarden/src/secrets_manager/state.rs +++ b/crates/bitwarden/src/secrets_manager/state.rs @@ -1,6 +1,6 @@ use std::{fmt::Debug, path::Path}; -use bitwarden_crypto::{EncString, KeyDecryptable, KeyEncryptable}; +use bitwarden_crypto::{EncString, KeyDecryptable, KeyEncryptable, SensitiveString}; use serde::{Deserialize, Serialize}; use crate::{ @@ -15,11 +15,11 @@ const STATE_VERSION: u32 = 1; pub struct ClientState { pub(crate) version: u32, pub(crate) token: String, - pub(crate) encryption_key: String, + pub(crate) encryption_key: SensitiveString, } impl ClientState { - pub fn new(token: String, encryption_key: String) -> Self { + pub fn new(token: String, encryption_key: SensitiveString) -> Self { Self { version: STATE_VERSION, token, diff --git a/crates/bitwarden/src/uniffi_support.rs b/crates/bitwarden/src/uniffi_support.rs index b23a9cbef..bf1eade70 100644 --- a/crates/bitwarden/src/uniffi_support.rs +++ b/crates/bitwarden/src/uniffi_support.rs @@ -1,6 +1,6 @@ use std::num::NonZeroU32; -use bitwarden_crypto::{AsymmetricEncString, EncString}; +use bitwarden_crypto::{AsymmetricEncString, EncString, SensitiveString}; use uuid::Uuid; use crate::UniffiCustomTypeConverter; @@ -12,6 +12,11 @@ uniffi::ffi_converter_forward!( bitwarden_crypto::UniFfiTag, crate::UniFfiTag ); +uniffi::ffi_converter_forward!( + SensitiveString, + bitwarden_crypto::UniFfiTag, + crate::UniFfiTag +); type DateTime = chrono::DateTime; uniffi::custom_type!(DateTime, std::time::SystemTime); diff --git a/crates/bitwarden/src/vault/cipher/attachment.rs b/crates/bitwarden/src/vault/cipher/attachment.rs index 1ec6be6fe..a573e92b0 100644 --- a/crates/bitwarden/src/vault/cipher/attachment.rs +++ b/crates/bitwarden/src/vault/cipher/attachment.rs @@ -66,7 +66,12 @@ impl<'a> KeyEncryptable for Attachm // with it, and then encrypt the key with the cipher key let attachment_key = SymmetricCryptoKey::generate(rand::thread_rng()); let encrypted_contents = self.contents.encrypt_with_key(&attachment_key)?; - attachment.key = Some(attachment_key.to_vec().encrypt_with_key(ciphers_key)?); + attachment.key = Some( + attachment_key + .to_vec() + .expose() + .encrypt_with_key(ciphers_key)?, + ); Ok(AttachmentEncryptResult { attachment: attachment.encrypt_with_key(ciphers_key)?, @@ -136,7 +141,7 @@ impl TryFrom for Attachment #[cfg(test)] mod tests { use base64::{engine::general_purpose::STANDARD, Engine}; - use bitwarden_crypto::{EncString, KeyDecryptable, SymmetricCryptoKey}; + use bitwarden_crypto::{EncString, KeyDecryptable, SensitiveString, SymmetricCryptoKey}; use crate::vault::{ cipher::cipher::{CipherRepromptType, CipherType}, @@ -145,7 +150,7 @@ mod tests { #[test] fn test_attachment_key() { - let user_key : SymmetricCryptoKey = "w2LO+nwV4oxwswVYCxlOfRUseXfvU03VzvKQHrqeklPgiMZrspUe6sOBToCnDn9Ay0tuCBn8ykVVRb7PWhub2Q==".parse().unwrap(); + let user_key : SymmetricCryptoKey = SensitiveString::test("w2LO+nwV4oxwswVYCxlOfRUseXfvU03VzvKQHrqeklPgiMZrspUe6sOBToCnDn9Ay0tuCBn8ykVVRb7PWhub2Q==").try_into().unwrap(); let attachment = Attachment { id: None, diff --git a/crates/bitwarden/src/vault/cipher/cipher.rs b/crates/bitwarden/src/vault/cipher/cipher.rs index f0138beb9..5df26ab34 100644 --- a/crates/bitwarden/src/vault/cipher/cipher.rs +++ b/crates/bitwarden/src/vault/cipher/cipher.rs @@ -308,7 +308,7 @@ impl CipherView { let new_key = SymmetricCryptoKey::generate(rand::thread_rng()); - self.key = Some(new_key.to_vec().encrypt_with_key(key)?); + self.key = Some(new_key.to_vec().expose().encrypt_with_key(key)?); Ok(()) } diff --git a/crates/bitwarden/src/vault/send.rs b/crates/bitwarden/src/vault/send.rs index 144933899..b6351baa1 100644 --- a/crates/bitwarden/src/vault/send.rs +++ b/crates/bitwarden/src/vault/send.rs @@ -394,7 +394,7 @@ mod tests { // Get the send key let send_key = Send::get_key(&send_key, k).unwrap(); let send_key_b64 = send_key.to_base64(); - assert_eq!(send_key_b64, "IR9ImHGm6rRuIjiN7csj94bcZR5WYTJj5GtNfx33zm6tJCHUl+QZlpNPba8g2yn70KnOHsAODLcR0um6E3MAlg=="); + assert_eq!(send_key_b64.expose(), "IR9ImHGm6rRuIjiN7csj94bcZR5WYTJj5GtNfx33zm6tJCHUl+QZlpNPba8g2yn70KnOHsAODLcR0um6E3MAlg=="); } fn build_encryption_settings() -> EncryptionSettings { diff --git a/crates/memory-testing/src/bin/analyze-dumps.rs b/crates/memory-testing/src/bin/analyze-dumps.rs index fee72f2e5..9957bc1cd 100644 --- a/crates/memory-testing/src/bin/analyze-dumps.rs +++ b/crates/memory-testing/src/bin/analyze-dumps.rs @@ -115,9 +115,7 @@ fn main() -> io::Result<()> { mac_final_pos.is_empty(), ); - // TODO: At the moment we are not zeroizing the base64 key in from_str, so this test is - // ignored - add_row( + error |= add_row( &mut table, format!("Symm. Key in Base64, case {}", idx), &b64_initial_pos, diff --git a/crates/memory-testing/src/main.rs b/crates/memory-testing/src/main.rs index 96e4ae175..ee781683c 100644 --- a/crates/memory-testing/src/main.rs +++ b/crates/memory-testing/src/main.rs @@ -1,6 +1,6 @@ -use std::{env, io::Read, path::Path, process, str::FromStr}; +use std::{env, io::Read, path::Path, process}; -use bitwarden_crypto::SymmetricCryptoKey; +use bitwarden_crypto::{SensitiveString, SymmetricCryptoKey}; fn wait_for_dump() { println!("Waiting for dump..."); @@ -22,8 +22,9 @@ fn main() { let mut symmetric_keys = Vec::new(); let mut symmetric_keys_as_vecs = Vec::new(); - for case in &cases.symmetric_key { - let key = SymmetricCryptoKey::from_str(&case.key).unwrap(); + for case in cases.symmetric_key { + let key = SensitiveString::new(Box::new(case.key)); + let key = SymmetricCryptoKey::try_from(key).unwrap(); symmetric_keys_as_vecs.push(key.to_vec()); symmetric_keys.push(key); }