Skip to content

Commit

Permalink
Use Fido2Credential instead of view in CipherView (#821)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
Hinton authored Jun 7, 2024
1 parent 3c16656 commit 85ac7c4
Show file tree
Hide file tree
Showing 4 changed files with 115 additions and 27 deletions.
11 changes: 7 additions & 4 deletions crates/bitwarden/src/platform/fido2/authenticator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -163,10 +163,12 @@ impl<'a> Fido2Authenticator<'a> {
&mut self,
rp_id: String,
) -> Result<Vec<Fido2CredentialView>> {
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())
}
Expand Down Expand Up @@ -198,15 +200,16 @@ impl<'a> Fido2Authenticator<'a> {
}

pub(super) fn get_selected_credential(&self) -> Result<SelectedCredential> {
let enc = self.client.get_encryption_settings()?;

let cipher = self
.selected_credential
.lock()
.expect("Mutex is not poisoned")
.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();

Expand Down
90 changes: 88 additions & 2 deletions crates/bitwarden/src/vault/cipher/cipher.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Expand Down Expand Up @@ -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(())
Expand Down Expand Up @@ -384,6 +385,40 @@ impl CipherView {
Ok(())
}

#[cfg(feature = "uniffi")]
pub(crate) fn decrypt_fido2_credentials(
&self,
enc: &EncryptionSettings,
) -> Result<Vec<Fido2CredentialView>> {
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<Fido2CredentialFullView> =
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,
Expand All @@ -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);
Expand Down Expand Up @@ -572,6 +608,7 @@ mod tests {
use std::collections::HashMap;

use attachment::AttachmentView;
use login::Fido2Credential;

use super::*;

Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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();

Expand All @@ -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]
Expand Down Expand Up @@ -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,
Expand All @@ -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]
Expand Down
40 changes: 20 additions & 20 deletions crates/bitwarden/src/vault/cipher/login.rs
Original file line number Diff line number Diff line change
Expand Up @@ -169,24 +169,24 @@ impl From<Fido2CredentialFullView> for Fido2CredentialNewView {
}
}

impl KeyEncryptable<SymmetricCryptoKey, Fido2CredentialView> for Fido2CredentialFullView {
fn encrypt_with_key(
self,
key: &SymmetricCryptoKey,
) -> Result<Fido2CredentialView, CryptoError> {
Ok(Fido2CredentialView {
credential_id: self.credential_id,
key_type: self.key_type,
key_algorithm: self.key_algorithm,
key_curve: self.key_curve,
impl KeyEncryptable<SymmetricCryptoKey, Fido2Credential> for Fido2CredentialFullView {
fn encrypt_with_key(self, key: &SymmetricCryptoKey) -> Result<Fido2Credential, CryptoError> {
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,
})
}
Expand Down Expand Up @@ -266,7 +266,7 @@ pub struct LoginView {
pub autofill_on_page_load: Option<bool>,

// TODO: Remove this once the SDK supports state
pub fido2_credentials: Option<Vec<Fido2CredentialView>>,
pub fido2_credentials: Option<Vec<Fido2Credential>>,
}

impl KeyEncryptable<SymmetricCryptoKey, LoginUri> for LoginUriView {
Expand All @@ -288,7 +288,7 @@ impl KeyEncryptable<SymmetricCryptoKey, Login> 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,
})
}
}
Expand All @@ -312,7 +312,7 @@ impl KeyDecryptable<SymmetricCryptoKey, LoginView> 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(),
})
}
}
Expand Down
1 change: 0 additions & 1 deletion crates/bitwarden/src/vault/cipher/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

0 comments on commit 85ac7c4

Please sign in to comment.