Skip to content

Commit 73cc150

Browse files
nikwithaknikwithak
authored
[PM-33122] Propogate Cipher field decryption errors (#818)
## 🎟️ Tracking [PM-33122](https://bitwarden.atlassian.net/browse/PM-33122) ## 📔 Objective When individual fields fail to decrypt on a Cipher, the fields are cleared out and the errors are swallowed. This means the user never knows that their ciphers are failing, and they could lose data if they edit the cipher, which will overwrite the bad data (which could still be recoverable). This change propogates any decryption errors instead of swallowing them. [PM-33122]: https://bitwarden.atlassian.net/browse/PM-33122?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
1 parent edd68d9 commit 73cc150

8 files changed

Lines changed: 169 additions & 121 deletions

File tree

‎crates/bitwarden-vault/src/cipher/card.rs‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,7 @@ impl Decryptable<KeyIds, SymmetricKeyId, CardListView> for Card {
9090
key: SymmetricKeyId,
9191
) -> Result<CardListView, CryptoError> {
9292
Ok(CardListView {
93-
brand: self.brand.decrypt(ctx, key).ok().flatten(),
93+
brand: self.brand.decrypt(ctx, key)?,
9494
})
9595
}
9696
}
@@ -102,12 +102,12 @@ impl Decryptable<KeyIds, SymmetricKeyId, CardView> for Card {
102102
key: SymmetricKeyId,
103103
) -> Result<CardView, CryptoError> {
104104
Ok(CardView {
105-
cardholder_name: self.cardholder_name.decrypt(ctx, key).ok().flatten(),
106-
exp_month: self.exp_month.decrypt(ctx, key).ok().flatten(),
107-
exp_year: self.exp_year.decrypt(ctx, key).ok().flatten(),
108-
code: self.code.decrypt(ctx, key).ok().flatten(),
109-
brand: self.brand.decrypt(ctx, key).ok().flatten(),
110-
number: self.number.decrypt(ctx, key).ok().flatten(),
105+
cardholder_name: self.cardholder_name.decrypt(ctx, key)?,
106+
exp_month: self.exp_month.decrypt(ctx, key)?,
107+
exp_year: self.exp_year.decrypt(ctx, key)?,
108+
code: self.code.decrypt(ctx, key)?,
109+
brand: self.brand.decrypt(ctx, key)?,
110+
number: self.number.decrypt(ctx, key)?,
111111
})
112112
}
113113
}

‎crates/bitwarden-vault/src/cipher/cipher.rs‎

Lines changed: 75 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -615,29 +615,25 @@ impl Decryptable<KeyIds, SymmetricKeyId, CipherView> for Cipher {
615615
folder_id: self.folder_id,
616616
collection_ids: self.collection_ids.clone(),
617617
key: self.key.clone(),
618-
name: self.name.decrypt(ctx, ciphers_key).ok().unwrap_or_default(),
619-
notes: self.notes.decrypt(ctx, ciphers_key).ok().flatten(),
618+
name: self.name.decrypt(ctx, ciphers_key)?,
619+
notes: self.notes.decrypt(ctx, ciphers_key)?,
620620
r#type: self.r#type,
621-
login: self.login.decrypt(ctx, ciphers_key).ok().flatten(),
622-
identity: self.identity.decrypt(ctx, ciphers_key).ok().flatten(),
623-
card: self.card.decrypt(ctx, ciphers_key).ok().flatten(),
624-
secure_note: self.secure_note.decrypt(ctx, ciphers_key).ok().flatten(),
625-
ssh_key: self.ssh_key.decrypt(ctx, ciphers_key).ok().flatten(),
621+
login: self.login.decrypt(ctx, ciphers_key)?,
622+
identity: self.identity.decrypt(ctx, ciphers_key)?,
623+
card: self.card.decrypt(ctx, ciphers_key)?,
624+
secure_note: self.secure_note.decrypt(ctx, ciphers_key)?,
625+
ssh_key: self.ssh_key.decrypt(ctx, ciphers_key)?,
626626
favorite: self.favorite,
627627
reprompt: self.reprompt,
628628
organization_use_totp: self.organization_use_totp,
629629
edit: self.edit,
630630
permissions: self.permissions,
631631
view_password: self.view_password,
632-
local_data: self.local_data.decrypt(ctx, ciphers_key).ok().flatten(),
632+
local_data: self.local_data.decrypt(ctx, ciphers_key)?,
633633
attachments: Some(attachments),
634634
attachment_decryption_failures: Some(attachment_decryption_failures),
635-
fields: self.fields.decrypt(ctx, ciphers_key).ok().flatten(),
636-
password_history: self
637-
.password_history
638-
.decrypt(ctx, ciphers_key)
639-
.ok()
640-
.flatten(),
635+
fields: self.fields.decrypt(ctx, ciphers_key)?,
636+
password_history: self.password_history.decrypt(ctx, ciphers_key)?,
641637
creation_date: self.creation_date,
642638
deleted_date: self.deleted_date,
643639
revision_date: self.revision_date,
@@ -937,11 +933,8 @@ impl Decryptable<KeyIds, SymmetricKeyId, CipherListView> for Cipher {
937933
folder_id: self.folder_id,
938934
collection_ids: self.collection_ids.clone(),
939935
key: self.key.clone(),
940-
name: self.name.decrypt(ctx, ciphers_key).ok().unwrap_or_default(),
941-
subtitle: self
942-
.decrypt_subtitle(ctx, ciphers_key)
943-
.ok()
944-
.unwrap_or_default(),
936+
name: self.name.decrypt(ctx, ciphers_key)?,
937+
subtitle: self.decrypt_subtitle(ctx, ciphers_key)?,
945938
r#type: match self.r#type {
946939
CipherType::Login => {
947940
let login = self
@@ -984,25 +977,30 @@ impl Decryptable<KeyIds, SymmetricKeyId, CipherListView> for Cipher {
984977
local_data: self.local_data.decrypt(ctx, ciphers_key)?,
985978
archived_date: self.archived_date,
986979
#[cfg(feature = "wasm")]
987-
notes: self.notes.decrypt(ctx, ciphers_key).ok().flatten(),
980+
notes: self.notes.decrypt(ctx, ciphers_key)?,
988981
#[cfg(feature = "wasm")]
989-
fields: self.fields.as_ref().map(|fields| {
990-
fields
991-
.iter()
992-
.filter_map(|f| {
993-
f.decrypt(ctx, ciphers_key)
994-
.ok()
995-
.map(field::FieldListView::from)
996-
})
997-
.collect()
998-
}),
982+
fields: self
983+
.fields
984+
.as_ref()
985+
.map(|fields| {
986+
fields
987+
.iter()
988+
.map(|f| f.decrypt(ctx, ciphers_key).map(field::FieldListView::from))
989+
.collect::<Result<Vec<_>, _>>()
990+
})
991+
.transpose()?,
999992
#[cfg(feature = "wasm")]
1000-
attachment_names: self.attachments.as_ref().map(|attachments| {
1001-
attachments
1002-
.iter()
1003-
.filter_map(|a| a.file_name.decrypt(ctx, ciphers_key).ok().flatten())
1004-
.collect()
1005-
}),
993+
attachment_names: self
994+
.attachments
995+
.as_ref()
996+
.map(|attachments| {
997+
attachments
998+
.iter()
999+
.map(|a| a.file_name.decrypt(ctx, ciphers_key))
1000+
.collect::<Result<Vec<_>, _>>()
1001+
})
1002+
.transpose()?
1003+
.map(|names| names.into_iter().flatten().collect()),
10061004
})
10071005
}
10081006
}
@@ -1508,6 +1506,47 @@ mod tests {
15081506
)
15091507
}
15101508

1509+
#[test]
1510+
fn test_decrypt_cipher_fails_with_invalid_name() {
1511+
let key_store =
1512+
create_test_crypto_with_user_key(SymmetricCryptoKey::make_aes256_cbc_hmac_key());
1513+
1514+
// Encrypt a valid cipher, then swap name with an EncString from a different key
1515+
let cipher = key_store.encrypt(generate_cipher()).unwrap();
1516+
let cipher = Cipher {
1517+
name: TEST_CIPHER_NAME.parse().unwrap(), // encrypted with a different key
1518+
..cipher
1519+
};
1520+
1521+
let result: Result<CipherView, _> = key_store.decrypt(&cipher);
1522+
assert!(
1523+
result.is_err(),
1524+
"Decryption should fail when name is encrypted with a different key"
1525+
);
1526+
}
1527+
1528+
#[test]
1529+
fn test_decrypt_cipher_fails_with_invalid_login() {
1530+
let key_store =
1531+
create_test_crypto_with_user_key(SymmetricCryptoKey::make_aes256_cbc_hmac_key());
1532+
1533+
// Encrypt a valid cipher, then corrupt the login username
1534+
let cipher = key_store.encrypt(generate_cipher()).unwrap();
1535+
let cipher = Cipher {
1536+
login: Some(Login {
1537+
username: Some(TEST_CIPHER_NAME.parse().unwrap()), // encrypted with a different key
1538+
..cipher.login.unwrap()
1539+
}),
1540+
..cipher
1541+
};
1542+
1543+
let result: Result<CipherView, _> = key_store.decrypt(&cipher);
1544+
assert!(
1545+
result.is_err(),
1546+
"Decryption should fail when login username is encrypted with a different key"
1547+
);
1548+
}
1549+
15111550
#[test]
15121551
fn test_generate_cipher_key() {
15131552
let key = SymmetricCryptoKey::make_aes256_cbc_hmac_key();

‎crates/bitwarden-vault/src/cipher/cipher_client/admin/get.rs‎

Lines changed: 12 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,7 @@ mod tests {
7474
models::{CipherMiniDetailsResponseModel, CipherMiniDetailsResponseModelListResponseModel},
7575
};
7676
use bitwarden_core::key_management::{KeyIds, SymmetricKeyId};
77-
use bitwarden_crypto::{KeyStore, SymmetricCryptoKey};
77+
use bitwarden_crypto::{KeyStore, PrimitiveEncryptable, SymmetricCryptoKey};
7878
use chrono::Utc;
7979

8080
use super::*;
@@ -84,10 +84,13 @@ mod tests {
8484
const TEST_CIPHER_ID_1: &str = "5faa9684-c793-4a2d-8a12-b33900187097";
8585
const TEST_CIPHER_ID_2: &str = "6faa9684-c793-4a2d-8a12-b33900187098";
8686

87-
fn generate_test_cipher() -> Cipher {
87+
fn generate_test_cipher(store: &KeyStore<KeyIds>) -> Cipher {
88+
let mut ctx = store.context();
8889
Cipher {
8990
id: TEST_CIPHER_ID_1.parse().ok(),
90-
name: "2.pMS6/icTQABtulw52pq2lg==|XXbxKxDTh+mWiN1HjH2N1w==|Q6PkuT+KX/axrgN9ubD5Ajk2YNwxQkgs3WJM0S0wtG8=".parse().unwrap(),
91+
name: "Test cipher"
92+
.encrypt(&mut ctx, SymmetricKeyId::User)
93+
.unwrap(),
9194
r#type: CipherType::Login,
9295
notes: Default::default(),
9396
organization_id: Default::default(),
@@ -149,8 +152,9 @@ mod tests {
149152

150153
#[tokio::test]
151154
async fn test_list_org_ciphers_all_success() {
152-
let cipher_1 = generate_test_cipher();
153-
let mut cipher_2 = generate_test_cipher();
155+
let store = setup_key_store();
156+
let cipher_1 = generate_test_cipher(&store);
157+
let mut cipher_2 = generate_test_cipher(&store);
154158
cipher_2.id = TEST_CIPHER_ID_2.parse().ok();
155159

156160
let response_1 = mock_api_response(&cipher_1);
@@ -167,8 +171,6 @@ mod tests {
167171
})
168172
});
169173
});
170-
171-
let store = setup_key_store();
172174
let result = list_org_ciphers(TEST_ORG_ID.parse().unwrap(), true, &api_client, &store)
173175
.await
174176
.unwrap();
@@ -181,8 +183,9 @@ mod tests {
181183

182184
#[tokio::test]
183185
async fn test_list_org_ciphers_with_failures() {
184-
let cipher = generate_test_cipher();
185-
let mut cipher_with_bad_key = generate_test_cipher();
186+
let store = setup_key_store();
187+
let cipher = generate_test_cipher(&store);
188+
let mut cipher_with_bad_key = generate_test_cipher(&store);
186189
cipher_with_bad_key.id = TEST_CIPHER_ID_2.parse().ok();
187190

188191
let response_good = mock_api_response(&cipher);
@@ -201,8 +204,6 @@ mod tests {
201204
})
202205
});
203206
});
204-
205-
let store = setup_key_store();
206207
let result = list_org_ciphers(TEST_ORG_ID.parse().unwrap(), true, &api_client, &store)
207208
.await
208209
.unwrap();

‎crates/bitwarden-vault/src/cipher/cipher_client/admin/restore.rs‎

Lines changed: 24 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -105,7 +105,7 @@ mod tests {
105105
models::{CipherMiniResponseModel, CipherMiniResponseModelListResponseModel},
106106
};
107107
use bitwarden_core::key_management::{KeyIds, SymmetricKeyId};
108-
use bitwarden_crypto::{KeyStore, SymmetricCryptoKey};
108+
use bitwarden_crypto::{KeyStore, PrimitiveEncryptable, SymmetricCryptoKey};
109109
use chrono::Utc;
110110

111111
use super::*;
@@ -115,10 +115,23 @@ mod tests {
115115
const TEST_CIPHER_ID_2: &str = "6faa9684-c793-4a2d-8a12-b33900187098";
116116
const TEST_ORG_ID: &str = "1bc9ac1e-f5aa-45f2-94bf-b181009709b8";
117117

118-
fn generate_test_cipher() -> Cipher {
118+
fn setup_key_store() -> KeyStore<KeyIds> {
119+
let store: KeyStore<KeyIds> = KeyStore::default();
120+
#[allow(deprecated)]
121+
let _ = store.context_mut().set_symmetric_key(
122+
SymmetricKeyId::User,
123+
SymmetricCryptoKey::make_aes256_cbc_hmac_key(),
124+
);
125+
store
126+
}
127+
128+
fn generate_test_cipher(store: &KeyStore<KeyIds>) -> Cipher {
129+
let mut ctx = store.context();
119130
Cipher {
120131
id: TEST_CIPHER_ID.parse().ok(),
121-
name: "2.pMS6/icTQABtulw52pq2lg==|XXbxKxDTh+mWiN1HjH2N1w==|Q6PkuT+KX/axrgN9ubD5Ajk2YNwxQkgs3WJM0S0wtG8=".parse().unwrap(),
132+
name: "Test cipher"
133+
.encrypt(&mut ctx, SymmetricKeyId::User)
134+
.unwrap(),
122135
r#type: crate::CipherType::Login,
123136
notes: Default::default(),
124137
organization_id: Default::default(),
@@ -128,11 +141,12 @@ mod tests {
128141
fields: Default::default(),
129142
collection_ids: Default::default(),
130143
key: Default::default(),
131-
login: Some(Login{
144+
login: Some(Login {
132145
username: None,
133146
password: None,
134147
password_revision_date: None,
135-
uris: None, totp: None,
148+
uris: None,
149+
totp: None,
136150
autofill_on_page_load: None,
137151
fido2_credentials: None,
138152
}),
@@ -157,7 +171,8 @@ mod tests {
157171

158172
#[tokio::test]
159173
async fn test_restore_as_admin() {
160-
let mut cipher = generate_test_cipher();
174+
let store = setup_key_store();
175+
let mut cipher = generate_test_cipher(&store);
161176
cipher.deleted_date = Some(Utc::now());
162177

163178
let api_client = {
@@ -179,12 +194,6 @@ mod tests {
179194
})
180195
};
181196

182-
let store: KeyStore<KeyIds> = KeyStore::default();
183-
#[allow(deprecated)]
184-
let _ = store.context_mut().set_symmetric_key(
185-
SymmetricKeyId::User,
186-
SymmetricCryptoKey::make_aes256_cbc_hmac_key(),
187-
);
188197
let start_time = Utc::now();
189198
let updated_cipher = restore_as_admin(TEST_CIPHER_ID.parse().unwrap(), &api_client, &store)
190199
.await
@@ -199,10 +208,11 @@ mod tests {
199208

200209
#[tokio::test]
201210
async fn test_restore_many_as_admin() {
211+
let store = setup_key_store();
202212
let cipher_id_2: CipherId = TEST_CIPHER_ID_2.parse().unwrap();
203-
let mut cipher_1 = generate_test_cipher();
213+
let mut cipher_1 = generate_test_cipher(&store);
204214
cipher_1.deleted_date = Some(Utc::now());
205-
let mut cipher_2 = generate_test_cipher();
215+
let mut cipher_2 = generate_test_cipher(&store);
206216
cipher_2.deleted_date = Some(Utc::now());
207217
cipher_2.id = Some(cipher_id_2);
208218

@@ -238,12 +248,6 @@ mod tests {
238248
})
239249
});
240250
});
241-
let store: KeyStore<KeyIds> = KeyStore::default();
242-
#[allow(deprecated)]
243-
let _ = store.context_mut().set_symmetric_key(
244-
SymmetricKeyId::User,
245-
SymmetricCryptoKey::make_aes256_cbc_hmac_key(),
246-
);
247251

248252
let start_time = Utc::now();
249253
let ciphers = restore_many_as_admin(

0 commit comments

Comments
 (0)