[PM-5791] Change decrypt to return Sensitive

crates/bitwarden-crypto/Cargo.toml

@@ -41,7 +41,10 @@ subtle = ">=2.5.0, <3.0"
 thiserror = ">=1.0.40, <2.0"
 uniffi = { version = "=0.25.2", optional = true }
 uuid = { version = ">=1.3.3, <2.0", features = ["serde"] }
-zeroize = { version = ">=1.7.0, <2.0", features = ["derive", "aarch64"] }
+zeroize = { version = ">=1.7.0, <2.0", features = [
+    "zeroize_derive",
+    "aarch64",
+] }
 
 [dev-dependencies]
 rand_chacha = "0.3.1"

crates/bitwarden-crypto/src/decrypted.rs

@@ -0,0 +1,131 @@
+use std::{
+    borrow::Cow,
+    fmt::{self, Formatter},
+};
+
+use schemars::JsonSchema;
+use serde::{Deserialize, Deserializer, Serialize, Serializer};
+use zeroize::Zeroize;
+
+use crate::{CryptoError, CryptoKey, KeyEncryptable};
+
+/// Wrapper for decrypted values which makes a best effort to enforce zeroization of the inner value
+/// on drop. The inner value exposes a [`Decrypted::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 [`Decrypted`] 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)]
+pub struct Decrypted<V: Zeroize> {
+    value: Box<V>,
+}
+
+impl<V: Zeroize> Decrypted<V> {
+    /// Create a new [`Decrypted`] 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<V>) -> 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
+    }
+}
+
+impl<V: Zeroize> Zeroize for Decrypted<V> {
+    fn zeroize(&mut self) {
+        self.value.zeroize()
+    }
+}
+
+impl<V: Zeroize> Drop for Decrypted<V> {
+    fn drop(&mut self) {
+        self.zeroize()
+    }
+}
+
+/// Helper to convert a `Decrypted<Vec<u8>>` to a `Decrypted<String>`, care is taken to ensure any
+/// intermediate copies are zeroed to avoid leaking sensitive data.
+impl TryFrom<DecryptedVec> for DecryptedString {
+    type Error = CryptoError;
+
+    fn try_from(mut v: DecryptedVec) -> Result<Self, CryptoError> {
+        let value = std::mem::take(&mut v.value);
+
+        let rtn = String::from_utf8(*value).map_err(|_| CryptoError::InvalidUtf8String);
+        rtn.map(|v| Decrypted::new(Box::new(v)))
+    }
+}
+
+impl<V: Zeroize + Default> Default for Decrypted<V> {
+    fn default() -> Self {
+        Self::new(Box::default())
+    }
+}
+
+impl<V: Zeroize + Serialize> fmt::Debug for Decrypted<V> {
+    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
+        f.debug_struct("Decrypted")
+            .field("value", &"********")
+            .finish()
+    }
+}
+
+/// Unfortunately once we serialize a `DecryptedString` we can't control the future memory.
+impl<V: Zeroize + Serialize> Serialize for Decrypted<V> {
+    fn serialize<S: Serializer>(&self, serializer: S) -> Result<S::Ok, S::Error> {
+        self.value.serialize(serializer)
+    }
+}
+
+impl<'de, V: Zeroize + Deserialize<'de>> Deserialize<'de> for Decrypted<V> {
+    fn deserialize<D: Deserializer<'de>>(deserializer: D) -> Result<Self, D::Error> {
+        Ok(Self::new(Box::new(V::deserialize(deserializer)?)))
+    }
+}
+
+/// Transparently expose the inner value for serialization
+impl<V: Zeroize + JsonSchema> JsonSchema for Decrypted<V> {
+    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<K: CryptoKey, V: Zeroize + Clone + KeyEncryptable<K, O>> KeyEncryptable<K, V>
+    for Decrypted<V>
+{
+    fn encrypt_with_key(self, key: &K) -> Result<O, CryptoError> {
+        self.value.clone().encrypt_with_key(key)
+    }
+}
+**/
+
+impl<T: KeyEncryptable<Key, Output> + Zeroize + Clone, Key: CryptoKey, Output>
+    KeyEncryptable<Key, Output> for Decrypted<T>
+{
+    fn encrypt_with_key(self, key: &Key) -> Result<Output, CryptoError> {
+        self.value.clone().encrypt_with_key(key)
+    }
+}
+
+pub type DecryptedVec = Decrypted<Vec<u8>>;
+pub type DecryptedString = Decrypted<String>;

crates/bitwarden-crypto/src/enc_string/asymmetric.rs

@@ -7,6 +7,7 @@ use serde::Deserialize;
 
 use super::{from_b64_vec, split_enc_string};
 use crate::{
+    decrypted::{DecryptedString, DecryptedVec},
     error::{CryptoError, EncStringParseError, Result},
     rsa::encrypt_rsa2048_oaep_sha1,
     AsymmetricCryptoKey, AsymmetricEncryptable, KeyDecryptable,
@@ -166,8 +167,8 @@ impl AsymmetricEncString {
     }
 }
 
-impl KeyDecryptable<AsymmetricCryptoKey, Vec<u8>> for AsymmetricEncString {
-    fn decrypt_with_key(&self, key: &AsymmetricCryptoKey) -> Result<Vec<u8>> {
+impl KeyDecryptable<AsymmetricCryptoKey, DecryptedVec> for AsymmetricEncString {
+    fn decrypt_with_key(&self, key: &AsymmetricCryptoKey) -> Result<DecryptedVec> {
         use AsymmetricEncString::*;
         match self {
             Rsa2048_OaepSha256_B64 { data } => key.key.decrypt(Oaep::new::<sha2::Sha256>(), data),
@@ -181,14 +182,15 @@ impl KeyDecryptable<AsymmetricCryptoKey, Vec<u8>> for AsymmetricEncString {
                 key.key.decrypt(Oaep::new::<sha1::Sha1>(), data)
             }
         }
+        .map(|v| DecryptedVec::new(Box::new(v)))
         .map_err(|_| CryptoError::KeyDecrypt)
     }
 }
 
-impl KeyDecryptable<AsymmetricCryptoKey, String> for AsymmetricEncString {
-    fn decrypt_with_key(&self, key: &AsymmetricCryptoKey) -> Result<String> {
-        let dec: Vec<u8> = self.decrypt_with_key(key)?;
-        String::from_utf8(dec).map_err(|_| CryptoError::InvalidUtf8String)
+impl KeyDecryptable<AsymmetricCryptoKey, DecryptedString> for AsymmetricEncString {
+    fn decrypt_with_key(&self, key: &AsymmetricCryptoKey) -> Result<DecryptedString> {
+        let dec: DecryptedVec = self.decrypt_with_key(key)?;
+        dec.try_into()
     }
 }
 
@@ -207,6 +209,7 @@ impl schemars::JsonSchema for AsymmetricEncString {
 #[cfg(test)]
 mod tests {
     use super::{AsymmetricCryptoKey, AsymmetricEncString, KeyDecryptable};
+    use crate::DecryptedString;
 
     const RSA_PRIVATE_KEY: &str = "-----BEGIN PRIVATE KEY-----
 MIIEvQIBADANBgkqhkiG9w0BAQEFAASCBKcwggSjAgEAAoIBAQCXRVrCX+2hfOQS
@@ -245,8 +248,8 @@ XKZBokBGnjFnTnKcs7nv/O8=
 
         assert_eq!(enc_string.enc_type(), 3);
 
-        let res: String = enc_string.decrypt_with_key(&private_key).unwrap();
-        assert_eq!(res, "EncryptMe!");
+        let res: DecryptedString = enc_string.decrypt_with_key(&private_key).unwrap();
+        assert_eq!(res.expose(), "EncryptMe!");
     }
 
     #[test]
@@ -257,8 +260,8 @@ XKZBokBGnjFnTnKcs7nv/O8=
 
         assert_eq!(enc_string.enc_type(), 4);
 
-        let res: String = enc_string.decrypt_with_key(&private_key).unwrap();
-        assert_eq!(res, "EncryptMe!");
+        let res: DecryptedString = enc_string.decrypt_with_key(&private_key).unwrap();
+        assert_eq!(res.expose(), "EncryptMe!");
     }
 
     #[test]
@@ -269,8 +272,8 @@ XKZBokBGnjFnTnKcs7nv/O8=
 
         assert_eq!(enc_string.enc_type(), 6);
 
-        let res: String = enc_string.decrypt_with_key(&private_key).unwrap();
-        assert_eq!(res, "EncryptMe!");
+        let res: DecryptedString = enc_string.decrypt_with_key(&private_key).unwrap();
+        assert_eq!(res.expose(), "EncryptMe!");
     }
 
     #[test]

crates/bitwarden-crypto/src/enc_string/symmetric.rs

@@ -7,6 +7,7 @@ use serde::Deserialize;
 
 use super::{check_length, from_b64, from_b64_vec, split_enc_string};
 use crate::{
+    decrypted::{DecryptedString, DecryptedVec},
     error::{CryptoError, EncStringParseError, Result},
     KeyDecryptable, KeyEncryptable, LocateKey, SymmetricCryptoKey,
 };
@@ -233,13 +234,18 @@ impl KeyEncryptable<SymmetricCryptoKey, EncString> for &[u8] {
     }
 }
 
-impl KeyDecryptable<SymmetricCryptoKey, Vec<u8>> for EncString {
-    fn decrypt_with_key(&self, key: &SymmetricCryptoKey) -> Result<Vec<u8>> {
+impl KeyDecryptable<SymmetricCryptoKey, DecryptedVec> for EncString {
+    fn decrypt_with_key(&self, key: &SymmetricCryptoKey) -> Result<DecryptedVec> {
         match self {
             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)?;
+                let dec = DecryptedVec::new(Box::new(crate::aes::decrypt_aes256_hmac(
+                    iv,
+                    mac,
+                    data.clone(),
+                    mac_key,
+                    &key.key,
+                )?));
                 Ok(dec)
             }
             _ => Err(CryptoError::InvalidKey),
@@ -253,10 +259,10 @@ impl KeyEncryptable<SymmetricCryptoKey, EncString> for String {
     }
 }
 
-impl KeyDecryptable<SymmetricCryptoKey, String> for EncString {
-    fn decrypt_with_key(&self, key: &SymmetricCryptoKey) -> Result<String> {
-        let dec: Vec<u8> = self.decrypt_with_key(key)?;
-        String::from_utf8(dec).map_err(|_| CryptoError::InvalidUtf8String)
+impl KeyDecryptable<SymmetricCryptoKey, DecryptedString> for EncString {
+    fn decrypt_with_key(&self, key: &SymmetricCryptoKey) -> Result<DecryptedString> {
+        let dec: DecryptedVec = self.decrypt_with_key(key)?;
+        dec.try_into()
     }
 }
 
@@ -275,17 +281,17 @@ impl schemars::JsonSchema for EncString {
 #[cfg(test)]
 mod tests {
     use super::EncString;
-    use crate::{derive_symmetric_key, KeyDecryptable, KeyEncryptable};
+    use crate::{decrypted::DecryptedString, derive_symmetric_key, KeyDecryptable, KeyEncryptable};
 
     #[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);
+        let decrypted_str: DecryptedString = cipher.decrypt_with_key(&key).unwrap();
+        assert_eq!(decrypted_str.expose(), test_string);
     }
 
     #[test]

crates/bitwarden-crypto/src/keys/asymmetric_crypto_key.rs

@@ -120,7 +120,8 @@ mod tests {
     use base64::{engine::general_purpose::STANDARD, Engine};
 
     use crate::{
-        AsymmetricCryptoKey, AsymmetricEncString, AsymmetricPublicCryptoKey, KeyDecryptable,
+        AsymmetricCryptoKey, AsymmetricEncString, AsymmetricPublicCryptoKey, DecryptedString,
+        KeyDecryptable,
     };
 
     #[test]
@@ -218,8 +219,8 @@ DnqOsltgPomWZ7xVfMkm9niL2OA=
         let encrypted =
             AsymmetricEncString::encrypt_rsa2048_oaep_sha1(plaintext.as_bytes(), &public_key)
                 .unwrap();
-        let decrypted: String = encrypted.decrypt_with_key(&private_key).unwrap();
+        let decrypted: DecryptedString = encrypted.decrypt_with_key(&private_key).unwrap();
 
-        assert_eq!(plaintext, decrypted);
+        assert_eq!(plaintext, decrypted.expose());
     }
 }

crates/bitwarden-crypto/src/keys/device_key.rs

@@ -1,6 +1,6 @@
 use crate::{
-    error::Result, AsymmetricCryptoKey, AsymmetricEncString, EncString, KeyDecryptable,
-    KeyEncryptable, SymmetricCryptoKey, UserKey,
+    decrypted::DecryptedVec, error::Result, AsymmetricCryptoKey, AsymmetricEncString, EncString,
+    KeyDecryptable, KeyEncryptable, SymmetricCryptoKey, UserKey,
 };
 
 /// Device Key
@@ -60,11 +60,13 @@ impl DeviceKey {
         protected_device_private_key: EncString,
         protected_user_key: AsymmetricEncString,
     ) -> Result<UserKey> {
-        let device_private_key: Vec<u8> = 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: DecryptedVec =
+            protected_device_private_key.decrypt_with_key(&self.0)?;
+        let device_private_key =
+            AsymmetricCryptoKey::from_der(device_private_key.expose().as_slice())?;
 
-        let mut dec: Vec<u8> = protected_user_key.decrypt_with_key(&device_private_key)?;
-        let user_key: SymmetricCryptoKey = dec.as_mut_slice().try_into()?;
+        let mut dec: DecryptedVec = protected_user_key.decrypt_with_key(&device_private_key)?;
+        let user_key: SymmetricCryptoKey = dec.expose_mut().as_mut_slice().try_into()?;
 
         Ok(UserKey(user_key))
     }

crates/bitwarden-crypto/src/keys/master_key.rs

@@ -8,6 +8,7 @@ use serde::{Deserialize, Serialize};
 use sha2::Digest;
 
 use crate::{
+    decrypted::DecryptedVec,
     util::{self, hkdf_expand},
     EncString, KeyDecryptable, Result, SymmetricCryptoKey, UserKey,
 };
@@ -60,8 +61,8 @@ impl MasterKey {
     pub fn decrypt_user_key(&self, user_key: EncString) -> Result<SymmetricCryptoKey> {
         let stretched_key = stretch_master_key(self)?;
 
-        let mut dec: Vec<u8> = user_key.decrypt_with_key(&stretched_key)?;
-        SymmetricCryptoKey::try_from(dec.as_mut_slice())
+        let mut dec: DecryptedVec = user_key.decrypt_with_key(&stretched_key)?;
+        SymmetricCryptoKey::try_from(dec.expose_mut().as_mut_slice())
     }
 
     pub fn encrypt_user_key(&self, user_key: &SymmetricCryptoKey) -> Result<EncString> {

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;

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 decrypted;
+pub use decrypted::{Decrypted, DecryptedString, DecryptedVec};
 
 #[cfg(feature = "mobile")]
 uniffi::setup_scaffolding!();

crates/bitwarden/src/auth/auth_request.rs

@@ -57,10 +57,12 @@ pub(crate) fn auth_request_decrypt_user_key(
     private_key: String,
     user_key: AsymmetricEncString,
 ) -> Result<SymmetricCryptoKey, Error> {
+    use bitwarden_crypto::DecryptedString;
+
     let key = AsymmetricCryptoKey::from_der(&STANDARD.decode(private_key)?)?;
-    let key: String = user_key.decrypt_with_key(&key)?;
+    let key: DecryptedString = user_key.decrypt_with_key(&key)?;
 
-    Ok(key.parse()?)
+    Ok(key.expose().parse()?)
 }
 
 /// Approve an auth request.