[PM-7066] Use Sensitive in MasterKey (#725)

## Type of change
```
- [ ] Bug fix
- [ ] New feature development
- [x] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other
```

## Objective
Use Sensitive in MasterKey and bubble up the Sensitive change for all
the requests that contain a password or pin. Note that this requires
changing the request structs to be taken by value.

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

@@ -5,7 +5,9 @@ use schemars::JsonSchema;
 use serde::{Deserialize, Serialize};
 
 use super::utils::{derive_kdf_key, stretch_kdf_key};
-use crate::{util, CryptoError, EncString, KeyDecryptable, Result, SymmetricCryptoKey, UserKey};
+use crate::{
+    util, CryptoError, EncString, KeyDecryptable, Result, SensitiveVec, SymmetricCryptoKey, UserKey,
+};
 
 /// Key Derivation Function for Bitwarden Account
 ///
@@ -69,13 +71,17 @@ impl MasterKey {
     }
 
     /// Derives a users master key from their password, email and KDF.
-    pub fn derive(password: &[u8], email: &[u8], kdf: &Kdf) -> Result<Self> {
-        derive_kdf_key(password, email, kdf).map(Self)
+    pub fn derive(password: &SensitiveVec, email: &[u8], kdf: &Kdf) -> Result<Self> {
+        derive_kdf_key(password.expose(), email, kdf).map(Self)
     }
 
     /// Derive the master key hash, used for local and remote password validation.
-    pub fn derive_master_key_hash(&self, password: &[u8], purpose: HashPurpose) -> Result<String> {
-        let hash = util::pbkdf2(&self.0.key, password, purpose as u32);
+    pub fn derive_master_key_hash(
+        &self,
+        password: &SensitiveVec,
+        purpose: HashPurpose,
+    ) -> Result<String> {
+        let hash = util::pbkdf2(&self.0.key, password.expose(), purpose as u32);
 
         Ok(STANDARD.encode(hash))
     }
@@ -124,13 +130,15 @@ mod tests {
     use rand::SeedableRng;
 
     use super::{make_user_key, HashPurpose, Kdf, MasterKey};
-    use crate::{keys::symmetric_crypto_key::derive_symmetric_key, SymmetricCryptoKey};
+    use crate::{
+        keys::symmetric_crypto_key::derive_symmetric_key, SensitiveVec, SymmetricCryptoKey,
+    };
 
     #[test]
     fn test_master_key_derive_pbkdf2() {
         let master_key = MasterKey::derive(
-            &b"67t9b5g67$%Dh89n"[..],
-            "test_key".as_bytes(),
+            &SensitiveVec::test(b"67t9b5g67$%Dh89n"),
+            b"test_key",
             &Kdf::PBKDF2 {
                 iterations: NonZeroU32::new(10000).unwrap(),
             },
@@ -150,8 +158,8 @@ mod tests {
     #[test]
     fn test_master_key_derive_argon2() {
         let master_key = MasterKey::derive(
-            &b"67t9b5g67$%Dh89n"[..],
-            "test_key".as_bytes(),
+            &SensitiveVec::test(b"67t9b5g67$%Dh89n"),
+            b"test_key",
             &Kdf::Argon2id {
                 iterations: NonZeroU32::new(4).unwrap(),
                 memory: NonZeroU32::new(32).unwrap(),
@@ -172,38 +180,38 @@ mod tests {
 
     #[test]
     fn test_password_hash_pbkdf2() {
-        let password = "asdfasdf".as_bytes();
-        let salt = "test_salt".as_bytes();
+        let password = SensitiveVec::test(b"asdfasdf");
+        let salt = b"test_salt";
         let kdf = Kdf::PBKDF2 {
             iterations: NonZeroU32::new(100_000).unwrap(),
         };
 
-        let master_key = MasterKey::derive(password, salt, &kdf).unwrap();
+        let master_key = MasterKey::derive(&password, salt, &kdf).unwrap();
 
         assert_eq!(
             "ZF6HjxUTSyBHsC+HXSOhZoXN+UuMnygV5YkWXCY4VmM=",
             master_key
-                .derive_master_key_hash(password, HashPurpose::ServerAuthorization)
+                .derive_master_key_hash(&password, HashPurpose::ServerAuthorization)
                 .unwrap(),
         );
     }
 
     #[test]
     fn test_password_hash_argon2id() {
-        let password = "asdfasdf".as_bytes();
-        let salt = "test_salt".as_bytes();
+        let password = SensitiveVec::test(b"asdfasdf");
+        let salt = b"test_salt";
         let kdf = Kdf::Argon2id {
             iterations: NonZeroU32::new(4).unwrap(),
             memory: NonZeroU32::new(32).unwrap(),
             parallelism: NonZeroU32::new(2).unwrap(),
         };
 
-        let master_key = MasterKey::derive(password, salt, &kdf).unwrap();
+        let master_key = MasterKey::derive(&password, salt, &kdf).unwrap();
 
         assert_eq!(
             "PR6UjYmjmppTYcdyTiNbAhPJuQQOmynKbdEl1oyi/iQ=",
             master_key
-                .derive_master_key_hash(password, HashPurpose::ServerAuthorization)
+                .derive_master_key_hash(&password, HashPurpose::ServerAuthorization)
                 .unwrap(),
         );
     }

crates/bitwarden-crypto/src/sensitive/sensitive.rs

@@ -74,6 +74,13 @@ impl TryFrom<SensitiveVec> for SensitiveString {
     }
 }
 
+impl From<SensitiveString> for SensitiveVec {
+    fn from(mut s: SensitiveString) -> Self {
+        let value = std::mem::take(&mut s.value);
+        Sensitive::new(Box::new(value.into_bytes()))
+    }
+}
+
 impl SensitiveString {
     pub fn decode_base64<T: base64::Engine>(self, engine: T) -> Result<SensitiveVec, CryptoError> {
         // Prevent accidental copies by allocating the full size
@@ -146,15 +153,18 @@ impl<V: Zeroize + JsonSchema> JsonSchema for Sensitive<V> {
     }
 }
 
-impl Sensitive<String> {
-    // 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()))
+// We use a lot of `&str` and `&[u8]` 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 reference we should be able to avoid accidental usages
+impl<V: Zeroize> Sensitive<V> {
+    pub fn test<T: ?Sized>(value: &'static T) -> Self
+    where
+        &'static T: Into<V>,
+    {
+        Self::new(Box::new(value.into()))
     }
 }
 
@@ -166,7 +176,7 @@ mod tests {
 
     #[test]
     fn test_debug() {
-        let string = Sensitive::test("test");
+        let string = SensitiveString::test("test");
         assert_eq!(
             format!("{:?}", string),
             "Sensitive { type: \"alloc::string::String\", value: \"********\" }"

crates/bitwarden-json/src/client.rs

@@ -49,15 +49,15 @@ impl Client {
 
         match cmd {
             #[cfg(feature = "internal")]
-            Command::PasswordLogin(req) => client.auth().login_password(&req).await.into_string(),
+            Command::PasswordLogin(req) => client.auth().login_password(req).await.into_string(),
             #[cfg(feature = "secrets")]
             Command::AccessTokenLogin(req) => {
                 client.auth().login_access_token(&req).await.into_string()
             }
             #[cfg(feature = "internal")]
-            Command::GetUserApiKey(req) => client.get_user_api_key(&req).await.into_string(),
+            Command::GetUserApiKey(req) => client.get_user_api_key(req).await.into_string(),
             #[cfg(feature = "internal")]
-            Command::ApiKeyLogin(req) => client.auth().login_api_key(&req).await.into_string(),
+            Command::ApiKeyLogin(req) => client.auth().login_api_key(req).await.into_string(),
             #[cfg(feature = "internal")]
             Command::Sync(req) => client.sync(&req).await.into_string(),
             #[cfg(feature = "internal")]

crates/bitwarden-uniffi/src/auth/mod.rs

@@ -4,7 +4,9 @@ use bitwarden::auth::{
     password::MasterPasswordPolicyOptions, AuthRequestResponse, RegisterKeyResponse,
     RegisterTdeKeyResponse,
 };
-use bitwarden_crypto::{AsymmetricEncString, HashPurpose, Kdf, TrustDeviceResponse};
+use bitwarden_crypto::{
+    AsymmetricEncString, HashPurpose, Kdf, SensitiveString, TrustDeviceResponse,
+};
 
 use crate::{error::Result, Client};
 
@@ -49,7 +51,7 @@ impl ClientAuth {
     pub async fn hash_password(
         &self,
         email: String,
-        password: String,
+        password: SensitiveString,
         kdf_params: Kdf,
         purpose: HashPurpose,
     ) -> Result<String> {
@@ -67,7 +69,7 @@ impl ClientAuth {
     pub async fn make_register_keys(
         &self,
         email: String,
-        password: String,
+        password: SensitiveString,
         kdf: Kdf,
     ) -> Result<RegisterKeyResponse> {
         Ok(self
@@ -98,7 +100,11 @@ impl ClientAuth {
     /// To retrieve the user's password hash, use [`ClientAuth::hash_password`] with
     /// `HashPurpose::LocalAuthentication` during login and persist it. If the login method has no
     /// password, use the email OTP.
-    pub async fn validate_password(&self, password: String, password_hash: String) -> Result<bool> {
+    pub async fn validate_password(
+        &self,
+        password: SensitiveString,
+        password_hash: String,
+    ) -> Result<bool> {
         Ok(self
             .0
              .0
@@ -116,7 +122,7 @@ impl ClientAuth {
     /// This works by comparing the provided password against the encrypted user key.
     pub async fn validate_password_user_key(
         &self,
-        password: String,
+        password: SensitiveString,
         encrypted_user_key: String,
     ) -> Result<String> {
         Ok(self

crates/bitwarden-uniffi/src/crypto.rs

@@ -53,7 +53,10 @@ impl ClientCrypto {
 
     /// Update the user's password, which will re-encrypt the user's encryption key with the new
     /// password. This returns the new encrypted user key and the new password hash.
-    pub async fn update_password(&self, new_password: String) -> Result<UpdatePasswordResponse> {
+    pub async fn update_password(
+        &self,
+        new_password: SensitiveString,
+    ) -> Result<UpdatePasswordResponse> {
         Ok(self
             .0
              .0
@@ -67,7 +70,7 @@ impl ClientCrypto {
     /// Generates a PIN protected user key from the provided PIN. The result can be stored and later
     /// used to initialize another client instance by using the PIN and the PIN key with
     /// `initialize_user_crypto`.
-    pub async fn derive_pin_key(&self, pin: String) -> Result<DerivePinKeyResponse> {
+    pub async fn derive_pin_key(&self, pin: SensitiveString) -> Result<DerivePinKeyResponse> {
         Ok(self.0 .0.write().await.crypto().derive_pin_key(pin).await?)
     }
 

crates/bitwarden-wasm/Cargo.toml

@@ -19,6 +19,7 @@ argon2 = { version = ">=0.5.0, <0.6", features = [
     "alloc",
     "zeroize",
 ], default-features = false }
+bitwarden-crypto = { workspace = true }
 bitwarden-json = { path = "../bitwarden-json", features = [
     "secrets",
     "internal",

crates/bitwarden-wasm/src/client.rs

@@ -2,6 +2,7 @@ extern crate console_error_panic_hook;
 use std::rc::Rc;
 
 use argon2::{Algorithm, Argon2, Params, Version};
+use bitwarden_crypto::SensitiveVec;
 use bitwarden_json::client::Client as JsonClient;
 use js_sys::Promise;
 use log::Level;
@@ -58,12 +59,15 @@ impl BitwardenClient {
 
 #[wasm_bindgen]
 pub fn argon2(
-    password: &[u8],
-    salt: &[u8],
+    password: Vec<u8>,
+    salt: Vec<u8>,
     iterations: u32,
     memory: u32,
     parallelism: u32,
 ) -> Result<Vec<u8>, JsError> {
+    let password = SensitiveVec::new(Box::new(password));
+    let salt = SensitiveVec::new(Box::new(salt));
+
     let argon = Argon2::new(
         Algorithm::Argon2id,
         Version::V0x13,
@@ -76,6 +80,6 @@ pub fn argon2(
     );
 
     let mut hash = [0u8; 32];
-    argon.hash_password_into(password, salt, &mut hash)?;
+    argon.hash_password_into(password.expose(), salt.expose(), &mut hash)?;
     Ok(hash.to_vec())
 }

crates/bitwarden/src/auth/auth_request.rs

@@ -122,7 +122,7 @@ fn test_auth_request() {
 mod tests {
     use std::num::NonZeroU32;
 
-    use bitwarden_crypto::Kdf;
+    use bitwarden_crypto::{Kdf, SensitiveVec};
 
     use super::*;
     use crate::mobile::crypto::{AuthRequestMethod, InitUserCryptoMethod, InitUserCryptoRequest};
@@ -132,7 +132,7 @@ mod tests {
         let mut client = Client::new(None);
 
         let master_key = bitwarden_crypto::MasterKey::derive(
-            "asdfasdfasdf".as_bytes(),
+            &SensitiveVec::test(b"asdfasdfasdf"),
             "[email protected]".as_bytes(),
             &Kdf::PBKDF2 {
                 iterations: NonZeroU32::new(600_000).unwrap(),
@@ -206,9 +206,12 @@ mod tests {
         // Initialize an existing client which is unlocked
         let mut existing_device = Client::new(None);
 
-        let master_key =
-            bitwarden_crypto::MasterKey::derive("asdfasdfasdf".as_bytes(), email.as_bytes(), &kdf)
-                .unwrap();
+        let master_key = bitwarden_crypto::MasterKey::derive(
+            &SensitiveVec::test(b"asdfasdfasdf"),
+            email.as_bytes(),
+            &kdf,
+        )
+        .unwrap();
 
         existing_device
             .initialize_user_crypto_master_key(master_key, user_key, private_key.parse().unwrap())