From 85ac7c440d792705f612de66772bf67a35613cd2 Mon Sep 17 00:00:00 2001 From: Oscar Hinton Date: Fri, 7 Jun 2024 18:08:28 +0200 Subject: [PATCH] Use Fido2Credential instead of view in CipherView (#821) We don't need to expose the `Fido2CredentialView` for ciphers since the data isn't used by the UI. Instead we can expose the encrypted view and ensure it is re-encrypted on move. --- .../src/platform/fido2/authenticator.rs | 11 ++- crates/bitwarden/src/vault/cipher/cipher.rs | 90 ++++++++++++++++++- crates/bitwarden/src/vault/cipher/login.rs | 40 ++++----- crates/bitwarden/src/vault/cipher/mod.rs | 1 - 4 files changed, 115 insertions(+), 27 deletions(-) diff --git a/crates/bitwarden/src/platform/fido2/authenticator.rs b/crates/bitwarden/src/platform/fido2/authenticator.rs index 69aae6f0c..96e53ad00 100644 --- a/crates/bitwarden/src/platform/fido2/authenticator.rs +++ b/crates/bitwarden/src/platform/fido2/authenticator.rs @@ -15,7 +15,7 @@ use super::{ Fido2UserInterface, SelectedCredential, AAGUID, }; use crate::{ - error::{require, Error, Result}, + error::{Error, Result}, platform::fido2::string_to_guid_bytes, vault::{ login::Fido2CredentialView, CipherView, Fido2CredentialFullView, Fido2CredentialNewView, @@ -163,10 +163,12 @@ impl<'a> Fido2Authenticator<'a> { &mut self, rp_id: String, ) -> Result> { + let enc = self.client.get_encryption_settings()?; let result = self.credential_store.find_credentials(None, rp_id).await?; + Ok(result .into_iter() - .filter_map(|c| c.login?.fido2_credentials) + .flat_map(|c| c.decrypt_fido2_credentials(enc)) .flatten() .collect()) } @@ -198,6 +200,8 @@ impl<'a> Fido2Authenticator<'a> { } pub(super) fn get_selected_credential(&self) -> Result { + let enc = self.client.get_encryption_settings()?; + let cipher = self .selected_credential .lock() @@ -205,8 +209,7 @@ impl<'a> Fido2Authenticator<'a> { .clone() .ok_or("No selected credential available")?; - let login = require!(cipher.login.as_ref()); - let creds = require!(login.fido2_credentials.as_ref()); + let creds = cipher.decrypt_fido2_credentials(enc)?; let credential = creds.first().ok_or("No Fido2 credentials found")?.clone(); diff --git a/crates/bitwarden/src/vault/cipher/cipher.rs b/crates/bitwarden/src/vault/cipher/cipher.rs index 15a1c124c..3b688df32 100644 --- a/crates/bitwarden/src/vault/cipher/cipher.rs +++ b/crates/bitwarden/src/vault/cipher/cipher.rs @@ -15,10 +15,10 @@ use super::{ login, secure_note, }; #[cfg(feature = "uniffi")] -use crate::vault::Fido2CredentialFullView; +use crate::{client::encryption_settings::EncryptionSettings, vault::Fido2CredentialView}; use crate::{ error::{require, Error, Result}, - vault::password_history, + vault::{password_history, Fido2CredentialFullView}, }; #[derive(Clone, Copy, Serialize_repr, Deserialize_repr, Debug, JsonSchema)] @@ -349,6 +349,7 @@ impl CipherView { let new_key = SymmetricCryptoKey::generate(rand::thread_rng()); self.reencrypt_attachment_keys(old_key, &new_key)?; + self.reencrypt_fido2_credentials(old_key, &new_key)?; self.key = Some(new_key.to_vec().encrypt_with_key(key)?); Ok(()) @@ -384,6 +385,40 @@ impl CipherView { Ok(()) } + #[cfg(feature = "uniffi")] + pub(crate) fn decrypt_fido2_credentials( + &self, + enc: &EncryptionSettings, + ) -> Result> { + let key = self.locate_key(enc, &None).ok_or(Error::VaultLocked)?; + let cipher_key = Cipher::get_cipher_key(key, &self.key)?; + + let key = cipher_key.as_ref().unwrap_or(key); + + Ok(self + .login + .as_ref() + .and_then(|l| l.fido2_credentials.as_ref()) + .map(|f| f.decrypt_with_key(key)) + .transpose()? + .unwrap_or_default()) + } + + fn reencrypt_fido2_credentials( + &mut self, + old_key: &SymmetricCryptoKey, + new_key: &SymmetricCryptoKey, + ) -> Result<()> { + if let Some(login) = self.login.as_mut() { + if let Some(fido2_credentials) = &mut login.fido2_credentials { + let dec_fido2_credentials: Vec = + fido2_credentials.decrypt_with_key(old_key)?; + *fido2_credentials = dec_fido2_credentials.encrypt_with_key(new_key)?; + } + } + Ok(()) + } + pub fn move_to_organization( &mut self, enc: &dyn KeyContainer, @@ -409,6 +444,7 @@ impl CipherView { } else { // If the cipher does not have a key, we need to reencrypt all attachment keys self.reencrypt_attachment_keys(old_key, new_key)?; + self.reencrypt_fido2_credentials(old_key, new_key)?; } self.organization_id = Some(organization_id); @@ -572,6 +608,7 @@ mod tests { use std::collections::HashMap; use attachment::AttachmentView; + use login::Fido2Credential; use super::*; @@ -612,6 +649,24 @@ mod tests { } } + fn generate_fido2(key: &SymmetricCryptoKey) -> Fido2Credential { + Fido2Credential { + credential_id: "123".to_string().encrypt_with_key(key).unwrap(), + key_type: "public-key".to_string().encrypt_with_key(key).unwrap(), + key_algorithm: "ECDSA".to_string().encrypt_with_key(key).unwrap(), + key_curve: "P-256".to_string().encrypt_with_key(key).unwrap(), + key_value: "123".to_string().encrypt_with_key(key).unwrap(), + rp_id: "123".to_string().encrypt_with_key(key).unwrap(), + user_handle: None, + user_name: None, + counter: "123".to_string().encrypt_with_key(key).unwrap(), + rp_name: None, + user_display_name: None, + discoverable: "true".to_string().encrypt_with_key(key).unwrap(), + creation_date: "2024-06-07T14:12:36.150Z".parse().unwrap(), + } + } + #[test] fn test_generate_cipher_key() { let key = SymmetricCryptoKey::generate(rand::thread_rng()); @@ -776,6 +831,8 @@ mod tests { key: Some(attachment_key_enc), }; cipher.attachments = Some(vec![attachment]); + let cred = generate_fido2(enc.get_key(&None).unwrap()); + cipher.login.as_mut().unwrap().fido2_credentials = Some(vec![cred]); cipher.move_to_organization(&enc, org).unwrap(); @@ -789,6 +846,18 @@ mod tests { .unwrap(); let new_attachment_key_dec: SymmetricCryptoKey = new_attachment_key_dec.try_into().unwrap(); assert_eq!(new_attachment_key_dec.to_vec(), attachment_key.to_vec()); + + let cred2: Fido2CredentialFullView = cipher + .login + .unwrap() + .fido2_credentials + .unwrap() + .first() + .unwrap() + .decrypt_with_key(enc.get_key(&Some(org)).unwrap()) + .unwrap(); + + assert_eq!(cred2.credential_id, "123"); } #[test] @@ -826,6 +895,9 @@ mod tests { }; cipher.attachments = Some(vec![attachment]); + let cred = generate_fido2(&cipher_key); + cipher.login.as_mut().unwrap().fido2_credentials = Some(vec![cred.clone()]); + cipher.move_to_organization(&enc, org).unwrap(); // Check that the cipher key has been re-encrypted with the org key, @@ -849,6 +921,20 @@ mod tests { .to_string(), attachment_key_enc.to_string() ); + + let cred2: Fido2Credential = cipher + .login + .unwrap() + .fido2_credentials + .unwrap() + .first() + .unwrap() + .clone(); + + assert_eq!( + cred2.credential_id.to_string(), + cred.credential_id.to_string() + ); } #[test] diff --git a/crates/bitwarden/src/vault/cipher/login.rs b/crates/bitwarden/src/vault/cipher/login.rs index 85ab98ff6..47dc71dc5 100644 --- a/crates/bitwarden/src/vault/cipher/login.rs +++ b/crates/bitwarden/src/vault/cipher/login.rs @@ -169,24 +169,24 @@ impl From for Fido2CredentialNewView { } } -impl KeyEncryptable for Fido2CredentialFullView { - fn encrypt_with_key( - self, - key: &SymmetricCryptoKey, - ) -> Result { - Ok(Fido2CredentialView { - credential_id: self.credential_id, - key_type: self.key_type, - key_algorithm: self.key_algorithm, - key_curve: self.key_curve, +impl KeyEncryptable for Fido2CredentialFullView { + fn encrypt_with_key(self, key: &SymmetricCryptoKey) -> Result { + Ok(Fido2Credential { + credential_id: self.credential_id.encrypt_with_key(key)?, + key_type: self.key_type.encrypt_with_key(key)?, + key_algorithm: self.key_algorithm.encrypt_with_key(key)?, + key_curve: self.key_curve.encrypt_with_key(key)?, key_value: self.key_value.encrypt_with_key(key)?, - rp_id: self.rp_id, - user_handle: self.user_handle, - user_name: self.user_name, - counter: self.counter, - rp_name: self.rp_name, - user_display_name: self.user_display_name, - discoverable: self.discoverable, + rp_id: self.rp_id.encrypt_with_key(key)?, + user_handle: self + .user_handle + .map(|h| h.encrypt_with_key(key)) + .transpose()?, + user_name: self.user_name.encrypt_with_key(key)?, + counter: self.counter.encrypt_with_key(key)?, + rp_name: self.rp_name.encrypt_with_key(key)?, + user_display_name: self.user_display_name.encrypt_with_key(key)?, + discoverable: self.discoverable.encrypt_with_key(key)?, creation_date: self.creation_date, }) } @@ -266,7 +266,7 @@ pub struct LoginView { pub autofill_on_page_load: Option, // TODO: Remove this once the SDK supports state - pub fido2_credentials: Option>, + pub fido2_credentials: Option>, } impl KeyEncryptable for LoginUriView { @@ -288,7 +288,7 @@ impl KeyEncryptable for LoginView { uris: self.uris.encrypt_with_key(key)?, totp: self.totp.encrypt_with_key(key)?, autofill_on_page_load: self.autofill_on_page_load, - fido2_credentials: self.fido2_credentials.encrypt_with_key(key)?, + fido2_credentials: self.fido2_credentials, }) } } @@ -312,7 +312,7 @@ impl KeyDecryptable for Login { uris: self.uris.decrypt_with_key(key).ok().flatten(), totp: self.totp.decrypt_with_key(key).ok().flatten(), autofill_on_page_load: self.autofill_on_page_load, - fido2_credentials: self.fido2_credentials.decrypt_with_key(key).ok().flatten(), + fido2_credentials: self.fido2_credentials.clone(), }) } } diff --git a/crates/bitwarden/src/vault/cipher/mod.rs b/crates/bitwarden/src/vault/cipher/mod.rs index 91e5bc7eb..92a4e10ad 100644 --- a/crates/bitwarden/src/vault/cipher/mod.rs +++ b/crates/bitwarden/src/vault/cipher/mod.rs @@ -14,7 +14,6 @@ pub use attachment::{ }; pub use cipher::{Cipher, CipherListView, CipherRepromptType, CipherType, CipherView}; pub use field::FieldView; -#[cfg(feature = "uniffi")] pub(crate) use login::Fido2CredentialFullView; pub use login::{Fido2Credential, Fido2CredentialNewView, Fido2CredentialView}; pub use secure_note::SecureNoteType;