diff --git a/CHANGELOG.md b/CHANGELOG.md index 95ca6ed974..2601696b95 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,7 +1,12 @@ # v152.0 (In progress) +## ✨ What's New ✨ + ### Breach Alerts -- New component: `breach-alerts` for storing and retrieving breach alert dismissals by breach ID. +* New component: `breach-alerts` for storing and retrieving breach alert dismissals by breach ID. + +### Logins +* Wipe individual login data on decryption errors (https://bugzilla.mozilla.org/show_bug.cgi?id=2007416) [Full Changelog](In progress) diff --git a/components/logins/src/db.rs b/components/logins/src/db.rs index 1c987ecc8a..8aee4cb215 100644 --- a/components/logins/src/db.rs +++ b/components/logins/src/db.rs @@ -265,16 +265,12 @@ impl LoginDb { // with a blank username. // // Returns an Err if the new login is not valid and could not be fixed up - pub fn find_login_to_update( - &self, - look: LoginEntry, - encdec: &dyn EncryptorDecryptor, - ) -> Result> { + pub fn find_login_to_update(&self, look: LoginEntry) -> Result> { let look = look.fixup()?; let logins = self .get_by_entry_target(&look)? .into_iter() - .map(|enc_login| enc_login.decrypt(encdec)) + .map(|enc_login| enc_login.decrypt(self)) .collect::>>()?; Ok(logins // First, try to match the username @@ -297,12 +293,14 @@ impl LoginDb { "UPDATE loginsL SET timeLastUsed = :now_millis, timesUsed = timesUsed + 1, - local_modified = :now_millis + local_modified = :now_millis, + sync_status = :sync_status WHERE guid = :guid AND is_deleted = 0", named_params! { ":now_millis": now_ms, ":guid": id, + ":sync_status": SyncStatus::Changed as u8, }, )?; tx.commit()?; @@ -313,22 +311,14 @@ impl LoginDb { /// /// Encrypts and stores passwords, automatically filtering out duplicates. /// Used by `add_many_with_meta()` to populate the breach database during import. - pub fn record_potentially_vulnerable_passwords( - &self, - passwords: Vec, - encdec: &dyn EncryptorDecryptor, - ) -> Result<()> { + pub fn record_potentially_vulnerable_passwords(&self, passwords: Vec) -> Result<()> { let tx = self.unchecked_transaction()?; - self.insert_potentially_vulnerable_passwords(passwords, encdec)?; + self.insert_potentially_vulnerable_passwords(passwords)?; tx.commit()?; Ok(()) } - fn insert_potentially_vulnerable_passwords( - &self, - passwords: Vec, - encdec: &dyn EncryptorDecryptor, - ) -> Result<()> { + fn insert_potentially_vulnerable_passwords(&self, passwords: Vec) -> Result<()> { let encrypted_existing_potentially_vulnerable_passwords: Vec = self .db .query_rows_and_then_cached("SELECT encryptedPassword FROM breachesL", [], |row| { @@ -338,8 +328,10 @@ impl LoginDb { encrypted_existing_potentially_vulnerable_passwords .iter() .map(|ciphertext| { - let decrypted_bytes = - encdec.decrypt(ciphertext.as_bytes().into()).map_err(|e| { + let decrypted_bytes = self + .encdec + .decrypt(ciphertext.as_bytes().into()) + .map_err(|e| { Error::DecryptionFailed(format!( "Failed to decrypt password from breachesL: {}", e @@ -367,7 +359,8 @@ impl LoginDb { .collect(); for password in difference { - let encrypted_password_bytes = encdec + let encrypted_password_bytes = self + .encdec .encrypt(password.as_bytes().into()) .map_err(|e| Error::EncryptionFailed(format!("{e} (encrypting password)")))?; let encrypted_password = @@ -395,11 +388,7 @@ impl LoginDb { /// Performance: O(M + N) where M = breached passwords, N = logins to check /// - Single check: Use `is_potentially_vulnerable_password()` (simpler) /// - Multiple checks: Use this method (faster) - pub fn are_potentially_vulnerable_passwords( - &self, - guids: &[&str], - encdec: &dyn EncryptorDecryptor, - ) -> Result> { + pub fn are_potentially_vulnerable_passwords(&self, guids: &[&str]) -> Result> { if guids.is_empty() { return Ok(Vec::new()); } @@ -413,9 +402,15 @@ impl LoginDb { let mut breached_passwords = std::collections::HashSet::new(); for ciphertext in &all_encrypted_passwords { - let decrypted_bytes = encdec.decrypt(ciphertext.as_bytes().into()).map_err(|e| { - Error::DecryptionFailed(format!("Failed to decrypt password from breachesL: {}", e)) - })?; + let decrypted_bytes = + self.encdec + .decrypt(ciphertext.as_bytes().into()) + .map_err(|e| { + Error::DecryptionFailed(format!( + "Failed to decrypt password from breachesL: {}", + e + )) + })?; let decrypted_password = std::str::from_utf8(&decrypted_bytes).map_err(|e| { Error::DecryptionFailed(format!( @@ -431,7 +426,7 @@ impl LoginDb { let mut vulnerable_guids = Vec::new(); for guid in guids { if let Some(login) = self.get_by_id(guid)? { - let decrypted_login = login.decrypt(encdec)?; + let decrypted_login = login.decrypt(self)?; if breached_passwords.contains(&decrypted_login.password) { vulnerable_guids.push(guid.to_string()); } @@ -441,13 +436,9 @@ impl LoginDb { Ok(vulnerable_guids) } - pub fn is_potentially_vulnerable_password( - &self, - guid: &str, - encdec: &dyn EncryptorDecryptor, - ) -> Result { + pub fn is_potentially_vulnerable_password(&self, guid: &str) -> Result { // Delegate to batch method for code reuse - let vulnerable = self.are_potentially_vulnerable_passwords(&[guid], encdec)?; + let vulnerable = self.are_potentially_vulnerable_passwords(&[guid])?; Ok(!vulnerable.is_empty()) } @@ -591,11 +582,7 @@ impl LoginDb { } /// Adds multiple logins within a single transaction and returns the successfully saved logins. - pub fn add_many( - &self, - entries: Vec, - encdec: &dyn EncryptorDecryptor, - ) -> Result>> { + pub fn add_many(&self, entries: Vec) -> Result>> { let now_ms = util::system_time_ms_i64(SystemTime::now()); let entries_with_meta = entries @@ -616,7 +603,7 @@ impl LoginDb { }) .collect(); - self.add_many_with_meta(entries_with_meta, encdec) + self.add_many_with_meta(entries_with_meta) } /// Adds multiple logins **including metadata** within a single transaction and returns the successfully saved logins. @@ -626,19 +613,18 @@ impl LoginDb { pub fn add_many_with_meta( &self, entries_with_meta: Vec, - encdec: &dyn EncryptorDecryptor, ) -> Result>> { let tx = self.unchecked_transaction()?; let mut results = vec![]; for entry_with_meta in entries_with_meta { let guid = Guid::from_string(entry_with_meta.meta.id.clone()); - match self.fixup_and_check_for_dupes(&guid, entry_with_meta.entry, encdec) { + match self.fixup_and_check_for_dupes(&guid, entry_with_meta.entry) { Ok(new_entry) => { let sec_fields = SecureLoginFields { username: new_entry.username, password: new_entry.password, } - .encrypt(encdec, &entry_with_meta.meta.id)?; + .encrypt(self.encdec.as_ref(), &entry_with_meta.meta.id)?; let encrypted_login = EncryptedLogin { meta: entry_with_meta.meta, fields: LoginFields { @@ -665,11 +651,7 @@ impl LoginDb { Ok(results) } - pub fn add( - &self, - entry: LoginEntry, - encdec: &dyn EncryptorDecryptor, - ) -> Result { + pub fn add(&self, entry: LoginEntry) -> Result { let guid = Guid::random(); let now_ms = util::system_time_ms_i64(SystemTime::now()); @@ -685,27 +667,18 @@ impl LoginDb { }, }; - self.add_with_meta(entry_with_meta, encdec) + self.add_with_meta(entry_with_meta) } /// Adds a login **including metadata**. /// Normally, you will use `add` instead, and AS Logins will take care of the metadata (setting timestamps, generating an ID) itself. /// However, in some cases, this method is necessary, for example when migrating data from another store that already contains the metadata. - pub fn add_with_meta( - &self, - entry_with_meta: LoginEntryWithMeta, - encdec: &dyn EncryptorDecryptor, - ) -> Result { - let mut results = self.add_many_with_meta(vec![entry_with_meta], encdec)?; + pub fn add_with_meta(&self, entry_with_meta: LoginEntryWithMeta) -> Result { + let mut results = self.add_many_with_meta(vec![entry_with_meta])?; results.pop().expect("there should be a single result") } - pub fn update( - &self, - sguid: &str, - entry: LoginEntry, - encdec: &dyn EncryptorDecryptor, - ) -> Result { + pub fn update(&self, sguid: &str, entry: LoginEntry) -> Result { let guid = Guid::new(sguid); let now_ms = util::system_time_ms_i64(SystemTime::now()); let tx = self.unchecked_transaction()?; @@ -717,7 +690,7 @@ impl LoginDb { // just log an error and continue. This avoids a crash on android-components // (mozilla-mobile/android-components#11251). - if self.check_for_dupes(&guid, &entry, encdec).is_err() { + if self.check_for_dupes(&guid, &entry).is_err() { // Try to detect if sync is enabled by checking if there are any mirror logins let has_mirror_row: bool = self .db @@ -735,7 +708,7 @@ impl LoginDb { // We must read the existing record so we can correctly manage timePasswordChanged. let existing = match self.get_by_id(sguid)? { - Some(e) => e.decrypt(encdec)?, + Some(e) => e.decrypt(self)?, None => return Err(Error::NoSuchRecord(sguid.to_owned())), }; let time_password_changed = if existing.password == entry.password { @@ -749,7 +722,7 @@ impl LoginDb { username: entry.username, password: entry.password, } - .encrypt(encdec, &existing.id)?; + .encrypt(self.encdec.as_ref(), &existing.id)?; let result = EncryptedLogin { meta: LoginMeta { id: existing.id, @@ -774,60 +747,36 @@ impl LoginDb { Ok(result) } - pub fn add_or_update( - &self, - entry: LoginEntry, - encdec: &dyn EncryptorDecryptor, - ) -> Result { + pub fn add_or_update(&self, entry: LoginEntry) -> Result { // Make sure to fixup the entry first, in case that changes the username let entry = entry.fixup()?; - match self.find_login_to_update(entry.clone(), encdec)? { - Some(login) => self.update(&login.id, entry, encdec), - None => self.add(entry, encdec), + match self.find_login_to_update(entry.clone())? { + Some(login) => self.update(&login.id, entry), + None => self.add(entry), } } - pub fn fixup_and_check_for_dupes( - &self, - guid: &Guid, - entry: LoginEntry, - encdec: &dyn EncryptorDecryptor, - ) -> Result { + pub fn fixup_and_check_for_dupes(&self, guid: &Guid, entry: LoginEntry) -> Result { let entry = entry.fixup()?; - self.check_for_dupes(guid, &entry, encdec)?; + self.check_for_dupes(guid, &entry)?; Ok(entry) } - pub fn check_for_dupes( - &self, - guid: &Guid, - entry: &LoginEntry, - encdec: &dyn EncryptorDecryptor, - ) -> Result<()> { - if self.dupe_exists(guid, entry, encdec)? { + pub fn check_for_dupes(&self, guid: &Guid, entry: &LoginEntry) -> Result<()> { + if self.dupe_exists(guid, entry)? { return Err(InvalidLogin::DuplicateLogin.into()); } Ok(()) } - pub fn dupe_exists( - &self, - guid: &Guid, - entry: &LoginEntry, - encdec: &dyn EncryptorDecryptor, - ) -> Result { - Ok(self.find_dupe(guid, entry, encdec)?.is_some()) + pub fn dupe_exists(&self, guid: &Guid, entry: &LoginEntry) -> Result { + Ok(self.find_dupe(guid, entry)?.is_some()) } - pub fn find_dupe( - &self, - guid: &Guid, - entry: &LoginEntry, - encdec: &dyn EncryptorDecryptor, - ) -> Result> { + pub fn find_dupe(&self, guid: &Guid, entry: &LoginEntry) -> Result> { for possible in self.get_by_entry_target(entry)? { if possible.guid() != *guid { - let pos_sec_fields = possible.decrypt_fields(encdec)?; + let pos_sec_fields = possible.decrypt_fields(self)?; if pos_sec_fields.username == entry.username { return Ok(Some(possible.guid())); } @@ -983,13 +932,12 @@ impl LoginDb { pub fn delete_undecryptable_records_for_remote_replacement( &self, - encdec: &dyn EncryptorDecryptor, ) -> Result { // Retrieve a list of guids for logins that cannot be decrypted let corrupted_logins = self .get_all()? .into_iter() - .filter(|login| login.clone().decrypt(encdec).is_err()) + .filter(|login| login.clone().decrypt(self).is_err()) .collect::>(); let ids = corrupted_logins .iter() @@ -1086,6 +1034,18 @@ impl LoginDb { Ok(row_count) } + /// Wipe local data for a single login + pub fn wipe_local_login_data(&self, guid: &str) -> Result<()> { + info!("Executing wipe_local on password engine!"); + let tx = self.unchecked_transaction()?; + self.execute("DELETE FROM loginsL WHERE guid=?", [guid])?; + self.execute("DELETE FROM loginsM WHERE guid=?", [guid])?; + // Delete the LAST_SYNC_META_KEY so that we resync the logins data next time + self.delete_meta(schema::LAST_SYNC_META_KEY)?; + tx.commit()?; + Ok(()) + } + pub fn shutdown(self) -> Result<()> { self.db.close().map_err(|(_, e)| Error::SqlError(e)) } @@ -1326,6 +1286,7 @@ mod tests { use crate::sync::merge::LocalLogin; use nss::ensure_initialized; use std::{thread, time}; + use sync15::ServerTimestamp; #[test] fn test_username_dupe_semantics() { @@ -1339,27 +1300,18 @@ mod tests { }; let db = LoginDb::open_in_memory(); - db.add(login.clone(), &*TEST_ENCDEC) + db.add(login.clone()) .expect("should be able to add first login"); // We will reject new logins with the same username value... let exp_err = "Invalid login: Login already exists"; - assert_eq!( - db.add(login.clone(), &*TEST_ENCDEC) - .unwrap_err() - .to_string(), - exp_err - ); + assert_eq!(db.add(login.clone()).unwrap_err().to_string(), exp_err); // Add one with an empty username - not a dupe. login.username = "".to_string(); - db.add(login.clone(), &*TEST_ENCDEC) - .expect("empty login isn't a dupe"); + db.add(login.clone()).expect("empty login isn't a dupe"); - assert_eq!( - db.add(login, &*TEST_ENCDEC).unwrap_err().to_string(), - exp_err - ); + assert_eq!(db.add(login).unwrap_err().to_string(), exp_err); // one with a username, 1 without. assert_eq!(db.get_all().unwrap().len(), 2); @@ -1387,7 +1339,7 @@ mod tests { let db = LoginDb::open_in_memory(); let added = db - .add_many(vec![login_a.clone(), login_b.clone()], &*TEST_ENCDEC) + .add_many(vec![login_a.clone(), login_b.clone()]) .expect("should be able to add logins"); let [added_a, added_b] = added.as_slice() else { @@ -1442,11 +1394,8 @@ mod tests { }; let db = LoginDb::open_in_memory(); - db.add_many( - vec![login_a.clone(), login_b.clone(), login_umlaut.clone()], - &*TEST_ENCDEC, - ) - .expect("should be able to add logins"); + db.add_many(vec![login_a.clone(), login_b.clone(), login_umlaut.clone()]) + .expect("should be able to add logins"); assert_eq!(db.count_by_origin(origin_a).unwrap(), 1); assert_eq!(db.count_by_origin(origin_umlaut).unwrap(), 1); @@ -1486,11 +1435,8 @@ mod tests { }; let db = LoginDb::open_in_memory(); - db.add_many( - vec![login_a.clone(), login_b.clone(), login_umlaut.clone()], - &*TEST_ENCDEC, - ) - .expect("should be able to add logins"); + db.add_many(vec![login_a.clone(), login_b.clone(), login_umlaut.clone()]) + .expect("should be able to add logins"); assert_eq!(db.count_by_form_action_origin(origin_a).unwrap(), 1); assert_eq!(db.count_by_form_action_origin(origin_umlaut).unwrap(), 1); @@ -1510,7 +1456,7 @@ mod tests { }; let db = LoginDb::open_in_memory(); - db.add(login, &*TEST_ENCDEC) + db.add(login) .expect("should be able to add login with invalid form_action_origin"); assert_eq!(db.count_by_form_action_origin("email").unwrap(), 1); } @@ -1538,7 +1484,7 @@ mod tests { let db = LoginDb::open_in_memory(); let added = db - .add_many(vec![login_a.clone(), login_b.clone()], &*TEST_ENCDEC) + .add_many(vec![login_a.clone(), login_b.clone()]) .expect("should be able to add logins"); let [added_a, added_b] = added.as_slice() else { @@ -1585,7 +1531,7 @@ mod tests { meta: meta.clone(), }; - db.add_with_meta(entry_with_meta, &*TEST_ENCDEC) + db.add_with_meta(entry_with_meta) .expect("should be able to add login with record"); let fetched = db @@ -1609,10 +1555,11 @@ mod tests { assert_eq!(count, 0); // Record some passwords - db.record_potentially_vulnerable_passwords( - vec!["password1".into(), "password2".into(), "password3".into()], - &*TEST_ENCDEC, - ) + db.record_potentially_vulnerable_passwords(vec![ + "password1".into(), + "password2".into(), + "password3".into(), + ]) .unwrap(); // Verify they were inserted @@ -1623,11 +1570,8 @@ mod tests { assert_eq!(count, 3); // Try to insert duplicates - should be filtered out - db.record_potentially_vulnerable_passwords( - vec!["password1".into(), "password4".into()], - &*TEST_ENCDEC, - ) - .unwrap(); + db.record_potentially_vulnerable_passwords(vec!["password1".into(), "password4".into()]) + .unwrap(); // Only password4 should have been added let count: i64 = db @@ -1637,11 +1581,8 @@ mod tests { assert_eq!(count, 4); // Try to insert only duplicates - should be a no-op - db.record_potentially_vulnerable_passwords( - vec!["password1".into(), "password2".into()], - &*TEST_ENCDEC, - ) - .unwrap(); + db.record_potentially_vulnerable_passwords(vec!["password1".into(), "password2".into()]) + .unwrap(); let count: i64 = db .db @@ -1678,7 +1619,7 @@ mod tests { meta: meta.clone(), }; - db.add_with_meta(entry_with_meta, &*TEST_ENCDEC) + db.add_with_meta(entry_with_meta) .expect("should be able to add login with record"); db.delete(&guid).expect("should be able to delete login"); @@ -1688,7 +1629,7 @@ mod tests { meta: meta.clone(), }; - db.add_with_meta(entry_with_meta2, &*TEST_ENCDEC) + db.add_with_meta(entry_with_meta2) .expect("should be able to re-add login with record"); let fetched = db @@ -1704,18 +1645,15 @@ mod tests { ensure_initialized(); let db = LoginDb::open_in_memory(); let added = db - .add( - LoginEntry { - form_action_origin: Some("http://😍.com".into()), - origin: "http://😍.com".into(), - http_realm: None, - username_field: "😍".into(), - password_field: "😍".into(), - username: "😍".into(), - password: "😍".into(), - }, - &*TEST_ENCDEC, - ) + .add(LoginEntry { + form_action_origin: Some("http://😍.com".into()), + origin: "http://😍.com".into(), + http_realm: None, + username_field: "😍".into(), + password_field: "😍".into(), + username: "😍".into(), + password: "😍".into(), + }) .unwrap(); let fetched = db .get_by_id(&added.meta.id) @@ -1729,7 +1667,7 @@ mod tests { ); assert_eq!(fetched.fields.username_field, "😍"); assert_eq!(fetched.fields.password_field, "😍"); - let sec_fields = fetched.decrypt_fields(&*TEST_ENCDEC).unwrap(); + let sec_fields = fetched.decrypt_fields(&db).unwrap(); assert_eq!(sec_fields.username, "😍"); assert_eq!(sec_fields.password, "😍"); } @@ -1739,17 +1677,14 @@ mod tests { ensure_initialized(); let db = LoginDb::open_in_memory(); let added = db - .add( - LoginEntry { - form_action_origin: None, - origin: "http://😍.com".into(), - http_realm: Some("😍😍".into()), - username: "😍".into(), - password: "😍".into(), - ..Default::default() - }, - &*TEST_ENCDEC, - ) + .add(LoginEntry { + form_action_origin: None, + origin: "http://😍.com".into(), + http_realm: Some("😍😍".into()), + username: "😍".into(), + password: "😍".into(), + ..Default::default() + }) .unwrap(); let fetched = db .get_by_id(&added.meta.id) @@ -1781,15 +1716,12 @@ mod tests { ) { let db = LoginDb::open_in_memory(); for h in good.iter().chain(bad.iter()) { - db.add( - LoginEntry { - origin: (*h).into(), - http_realm: Some((*h).into()), - password: "test".into(), - ..Default::default() - }, - &*TEST_ENCDEC, - ) + db.add(LoginEntry { + origin: (*h).into(), + http_realm: Some((*h).into()), + password: "test".into(), + ..Default::default() + }) .unwrap(); } for query in good_queries { @@ -1883,7 +1815,7 @@ mod tests { password: "test_password".into(), ..Default::default() }; - let login = db.add(to_add, &*TEST_ENCDEC).unwrap(); + let login = db.add(to_add).unwrap(); let login2 = db.get_by_id(&login.meta.id).unwrap().unwrap(); assert_eq!(login.fields.origin, login2.fields.origin); @@ -1896,16 +1828,13 @@ mod tests { ensure_initialized(); let db = LoginDb::open_in_memory(); let login = db - .add( - LoginEntry { - origin: "https://www.example.com".into(), - http_realm: Some("https://www.example.com".into()), - username: "user1".into(), - password: "password1".into(), - ..Default::default() - }, - &*TEST_ENCDEC, - ) + .add(LoginEntry { + origin: "https://www.example.com".into(), + http_realm: Some("https://www.example.com".into()), + username: "user1".into(), + password: "password1".into(), + ..Default::default() + }) .unwrap(); db.update( &login.meta.id, @@ -1916,7 +1845,6 @@ mod tests { password: "password2".into(), ..Default::default() // TODO: check and fix if needed }, - &*TEST_ENCDEC, ) .unwrap(); @@ -1927,26 +1855,67 @@ mod tests { login2.fields.http_realm, Some("https://www.example2.com".into()) ); - let sec_fields = login2.decrypt_fields(&*TEST_ENCDEC).unwrap(); + let sec_fields = login2.decrypt_fields(&db).unwrap(); assert_eq!(sec_fields.username, "user2"); assert_eq!(sec_fields.password, "password2"); } + #[test] + fn test_decryption_errors() { + ensure_initialized(); + let db = LoginDb::open_in_memory(); + let entry = LoginEntry { + origin: "https://www.example.com".into(), + http_realm: Some("https://www.example.com".into()), + username: "test".into(), + password: "test".into(), + ..Default::default() + }; + let mut enc_login = db.add(entry).unwrap(); + // Also add a mirror entry to make sure that's also removed + test_utils::add_mirror(&db, &enc_login, &ServerTimestamp(0), false).unwrap(); + // Mess with the `sec_fields` data so that the login is no longer decryptable + enc_login.sec_fields = "corrupted-data".into(); + assert!(matches!( + enc_login.decrypt_fields(&db), + Err(Error::DecryptionFailed(_)) + )); + + // The login should have been wiped from the DB + assert_eq!( + db.conn() + .query_one( + "SELECT COUNT(*) FROM loginsL WHERE guid=?", + (&enc_login.meta.id,), + |row| row.get::<_, usize>(0), + ) + .unwrap(), + 0 + ); + assert_eq!( + db.conn() + .query_one( + "SELECT COUNT(*) FROM loginsM WHERE guid=?", + (&enc_login.meta.id,), + |row| row.get::<_, usize>(0), + ) + .unwrap(), + 0 + ); + } + #[test] fn test_touch() { ensure_initialized(); let db = LoginDb::open_in_memory(); let login = db - .add( - LoginEntry { - origin: "https://www.example.com".into(), - http_realm: Some("https://www.example.com".into()), - username: "user1".into(), - password: "password1".into(), - ..Default::default() - }, - &*TEST_ENCDEC, - ) + .add(LoginEntry { + origin: "https://www.example.com".into(), + http_realm: Some("https://www.example.com".into()), + username: "user1".into(), + password: "password1".into(), + ..Default::default() + }) .unwrap(); // Simulate touch happening at another "time" thread::sleep(time::Duration::from_millis(50)); @@ -1961,16 +1930,13 @@ mod tests { ensure_initialized(); let db = LoginDb::open_in_memory(); let login = db - .add( - LoginEntry { - origin: "https://www.example.com".into(), - http_realm: Some("https://www.example.com".into()), - username: "user1".into(), - password: "password1".into(), - ..Default::default() - }, - &*TEST_ENCDEC, - ) + .add(LoginEntry { + origin: "https://www.example.com".into(), + http_realm: Some("https://www.example.com".into()), + username: "user1".into(), + password: "password1".into(), + ..Default::default() + }) .unwrap(); // initial state assert!(login.meta.time_last_breach_alert_dismissed.is_none()); @@ -1986,16 +1952,13 @@ mod tests { ensure_initialized(); let db = LoginDb::open_in_memory(); let login = db - .add( - LoginEntry { - origin: "https://www.example.com".into(), - http_realm: Some("https://www.example.com".into()), - username: "user1".into(), - password: "password1".into(), - ..Default::default() - }, - &*TEST_ENCDEC, - ) + .add(LoginEntry { + origin: "https://www.example.com".into(), + http_realm: Some("https://www.example.com".into()), + username: "user1".into(), + password: "password1".into(), + ..Default::default() + }) .unwrap(); let dismiss_time = login.meta.time_password_changed + 1000; @@ -2006,7 +1969,7 @@ mod tests { .get_by_id(&login.meta.id) .unwrap() .unwrap() - .decrypt(&*TEST_ENCDEC) + .decrypt(&db) .unwrap(); assert_eq!( retrieved.time_last_breach_alert_dismissed, @@ -2019,16 +1982,13 @@ mod tests { ensure_initialized(); let db = LoginDb::open_in_memory(); let login = db - .add( - LoginEntry { - origin: "https://www.example.com".into(), - http_realm: Some("https://www.example.com".into()), - username: "test_user".into(), - password: "test_password".into(), - ..Default::default() - }, - &*TEST_ENCDEC, - ) + .add(LoginEntry { + origin: "https://www.example.com".into(), + http_realm: Some("https://www.example.com".into()), + username: "test_user".into(), + password: "test_password".into(), + ..Default::default() + }) .unwrap(); assert!(db.delete(login.guid_str()).unwrap()); @@ -2052,29 +2012,23 @@ mod tests { let db = LoginDb::open_in_memory(); let login_a = db - .add( - LoginEntry { - origin: "https://a.example.com".into(), - http_realm: Some("https://www.example.com".into()), - username: "test_user".into(), - password: "test_password".into(), - ..Default::default() - }, - &*TEST_ENCDEC, - ) + .add(LoginEntry { + origin: "https://a.example.com".into(), + http_realm: Some("https://www.example.com".into()), + username: "test_user".into(), + password: "test_password".into(), + ..Default::default() + }) .unwrap(); let login_b = db - .add( - LoginEntry { - origin: "https://b.example.com".into(), - http_realm: Some("https://www.example.com".into()), - username: "test_user".into(), - password: "test_password".into(), - ..Default::default() - }, - &*TEST_ENCDEC, - ) + .add(LoginEntry { + origin: "https://b.example.com".into(), + http_realm: Some("https://www.example.com".into()), + username: "test_user".into(), + password: "test_password".into(), + ..Default::default() + }) .unwrap(); let result = db @@ -2092,16 +2046,13 @@ mod tests { let db = LoginDb::open_in_memory(); let login = db - .add( - LoginEntry { - origin: "https://a.example.com".into(), - http_realm: Some("https://www.example.com".into()), - username: "test_user".into(), - password: "test_password".into(), - ..Default::default() - }, - &*TEST_ENCDEC, - ) + .add(LoginEntry { + origin: "https://a.example.com".into(), + http_realm: Some("https://www.example.com".into()), + username: "test_user".into(), + password: "test_password".into(), + ..Default::default() + }) .unwrap(); let result = db.delete_many(vec![login.guid_str()]).unwrap(); @@ -2126,16 +2077,13 @@ mod tests { ensure_initialized(); let db = LoginDb::open_in_memory(); let login = db - .add( - LoginEntry { - origin: "https://www.example.com".into(), - http_realm: Some("https://www.example.com".into()), - username: "test_user".into(), - password: "test_password".into(), - ..Default::default() - }, - &*TEST_ENCDEC, - ) + .add(LoginEntry { + origin: "https://www.example.com".into(), + http_realm: Some("https://www.example.com".into()), + username: "test_user".into(), + password: "test_password".into(), + ..Default::default() + }) .unwrap(); let result = db @@ -2165,9 +2113,9 @@ mod tests { } fn make_saved_login(db: &LoginDb, username: &str, password: &str) -> Login { - db.add(make_entry(username, password), &*TEST_ENCDEC) + db.add(make_entry(username, password)) .unwrap() - .decrypt(&*TEST_ENCDEC) + .decrypt(&db) .unwrap() } @@ -2178,8 +2126,7 @@ mod tests { let login = make_saved_login(&db, "user", "pass"); assert_eq!( Some(login), - db.find_login_to_update(make_entry("user", "pass"), &*TEST_ENCDEC) - .unwrap(), + db.find_login_to_update(make_entry("user", "pass")).unwrap(), ); } @@ -2190,33 +2137,26 @@ mod tests { // Non-match because the username is different make_saved_login(&db, "other-user", "pass"); // Non-match because the http_realm is different - db.add( - LoginEntry { - origin: "https://www.example.com".into(), - http_realm: Some("the other website".into()), - username: "user".into(), - password: "pass".into(), - ..Default::default() - }, - &*TEST_ENCDEC, - ) + db.add(LoginEntry { + origin: "https://www.example.com".into(), + http_realm: Some("the other website".into()), + username: "user".into(), + password: "pass".into(), + ..Default::default() + }) .unwrap(); // Non-match because it uses form_action_origin instead of http_realm - db.add( - LoginEntry { - origin: "https://www.example.com".into(), - form_action_origin: Some("https://www.example.com/".into()), - username: "user".into(), - password: "pass".into(), - ..Default::default() - }, - &*TEST_ENCDEC, - ) + db.add(LoginEntry { + origin: "https://www.example.com".into(), + form_action_origin: Some("https://www.example.com/".into()), + username: "user".into(), + password: "pass".into(), + ..Default::default() + }) .unwrap(); assert_eq!( None, - db.find_login_to_update(make_entry("user", "pass"), &*TEST_ENCDEC) - .unwrap(), + db.find_login_to_update(make_entry("user", "pass")).unwrap(), ); } @@ -2227,8 +2167,7 @@ mod tests { let login = make_saved_login(&db, "", "pass"); assert_eq!( Some(login), - db.find_login_to_update(make_entry("user", "pass"), &*TEST_ENCDEC) - .unwrap(), + db.find_login_to_update(make_entry("user", "pass")).unwrap(), ); } @@ -2240,8 +2179,7 @@ mod tests { let username_match = make_saved_login(&db, "user", "pass"); assert_eq!( Some(username_match), - db.find_login_to_update(make_entry("user", "pass"), &*TEST_ENCDEC) - .unwrap(), + db.find_login_to_update(make_entry("user", "pass")).unwrap(), ); } @@ -2250,14 +2188,11 @@ mod tests { ensure_initialized(); let db = LoginDb::open_in_memory(); assert!(db - .find_login_to_update( - LoginEntry { - http_realm: None, - form_action_origin: None, - ..LoginEntry::default() - }, - &*TEST_ENCDEC - ) + .find_login_to_update(LoginEntry { + http_realm: None, + form_action_origin: None, + ..LoginEntry::default() + }) .is_err()); } @@ -2274,11 +2209,11 @@ mod tests { let mut entry = login.entry(); entry.password = "pass2".to_string(); - db.update(&login.id, entry, &*TEST_ENCDEC).unwrap(); + db.update(&login.id, entry).unwrap(); let mut entry = login.entry(); entry.password = "pass3".to_string(); - db.add_or_update(entry, &*TEST_ENCDEC).unwrap(); + db.add_or_update(entry).unwrap(); } #[test] @@ -2288,64 +2223,49 @@ mod tests { // Create two logins with the same password let login1 = db - .add( - LoginEntry { - origin: "https://site1.com".into(), - http_realm: Some("realm".into()), - username: "user1".into(), - password: "shared_password".into(), - ..Default::default() - }, - &*TEST_ENCDEC, - ) + .add(LoginEntry { + origin: "https://site1.com".into(), + http_realm: Some("realm".into()), + username: "user1".into(), + password: "shared_password".into(), + ..Default::default() + }) .unwrap(); let login2 = db - .add( - LoginEntry { - origin: "https://site2.com".into(), - http_realm: Some("realm".into()), - username: "user2".into(), - password: "shared_password".into(), - ..Default::default() - }, - &*TEST_ENCDEC, - ) + .add(LoginEntry { + origin: "https://site2.com".into(), + http_realm: Some("realm".into()), + username: "user2".into(), + password: "shared_password".into(), + ..Default::default() + }) .unwrap(); // Initially, neither login is vulnerable assert!(!db - .is_potentially_vulnerable_password(&login1.meta.id, &*TEST_ENCDEC) + .is_potentially_vulnerable_password(&login1.meta.id) .unwrap()); assert!(!db - .is_potentially_vulnerable_password(&login2.meta.id, &*TEST_ENCDEC) + .is_potentially_vulnerable_password(&login2.meta.id) .unwrap()); // And checking both logins should return empty (none are vulnerable yet) let vulnerable = db - .are_potentially_vulnerable_passwords( - &[&login1.meta.id, &login2.meta.id], - &*TEST_ENCDEC, - ) + .are_potentially_vulnerable_passwords(&[&login1.meta.id, &login2.meta.id]) .unwrap(); assert_eq!(vulnerable.len(), 0); // Record "shared_password" as a vulnerable password - db.record_potentially_vulnerable_passwords( - vec!["shared_password".into()], - &*TEST_ENCDEC, - ) - .unwrap(); + db.record_potentially_vulnerable_passwords(vec!["shared_password".into()]) + .unwrap(); // login2 should be recognized as vulnerable (same password as breached login1) assert!(db - .is_potentially_vulnerable_password(&login2.meta.id, &*TEST_ENCDEC) + .is_potentially_vulnerable_password(&login2.meta.id) .unwrap()); // Batch check: both logins should be vulnerable (they share the same password) let vulnerable = db - .are_potentially_vulnerable_passwords( - &[&login1.meta.id, &login2.meta.id], - &*TEST_ENCDEC, - ) + .are_potentially_vulnerable_passwords(&[&login1.meta.id, &login2.meta.id]) .unwrap(); assert_eq!(vulnerable.len(), 2); assert!(vulnerable.contains(&login1.meta.id)); @@ -2361,12 +2281,11 @@ mod tests { password: "different_password".into(), ..Default::default() }, - &*TEST_ENCDEC, ) .unwrap(); assert!(!db - .is_potentially_vulnerable_password(&login2.meta.id, &*TEST_ENCDEC) + .is_potentially_vulnerable_password(&login2.meta.id) .unwrap()); } @@ -2376,19 +2295,16 @@ mod tests { let db = LoginDb::open_in_memory(); let login = db - .add( - LoginEntry { - origin: "https://example.com".into(), - http_realm: Some("realm".into()), - username: "user".into(), - password: "password123".into(), - ..Default::default() - }, - &*TEST_ENCDEC, - ) + .add(LoginEntry { + origin: "https://example.com".into(), + http_realm: Some("realm".into()), + username: "user".into(), + password: "password123".into(), + ..Default::default() + }) .unwrap(); - db.record_potentially_vulnerable_passwords(vec!["password123".into()], &*TEST_ENCDEC) + db.record_potentially_vulnerable_passwords(vec!["password123".into()]) .unwrap(); // Verify that breachesL has an entry @@ -2399,7 +2315,7 @@ mod tests { assert_eq!(count, 1); // And verify via the API that this login is vulnerable let vulnerable = db - .are_potentially_vulnerable_passwords(&[&login.meta.id], &*TEST_ENCDEC) + .are_potentially_vulnerable_passwords(&[&login.meta.id]) .unwrap(); assert_eq!(vulnerable.len(), 1); assert_eq!(vulnerable[0], login.meta.id); @@ -2415,7 +2331,7 @@ mod tests { assert_eq!(count, 0); // And verify via the API that no logins are vulnerable anymore let vulnerable = db - .are_potentially_vulnerable_passwords(&[&login.meta.id], &*TEST_ENCDEC) + .are_potentially_vulnerable_passwords(&[&login.meta.id]) .unwrap(); assert_eq!(vulnerable.len(), 0); } @@ -2426,45 +2342,36 @@ mod tests { let db = LoginDb::open_in_memory(); let login1 = db - .add( - LoginEntry { - origin: "https://site1.com".into(), - http_realm: Some("realm".into()), - username: "user".into(), - password: "password_A".into(), - ..Default::default() - }, - &*TEST_ENCDEC, - ) + .add(LoginEntry { + origin: "https://site1.com".into(), + http_realm: Some("realm".into()), + username: "user".into(), + password: "password_A".into(), + ..Default::default() + }) .unwrap(); let login2 = db - .add( - LoginEntry { - origin: "https://site2.com".into(), - http_realm: Some("realm".into()), - username: "user".into(), - password: "password_B".into(), - ..Default::default() - }, - &*TEST_ENCDEC, - ) + .add(LoginEntry { + origin: "https://site2.com".into(), + http_realm: Some("realm".into()), + username: "user".into(), + password: "password_B".into(), + ..Default::default() + }) .unwrap(); - db.record_potentially_vulnerable_passwords(vec!["password_A".into()], &*TEST_ENCDEC) + db.record_potentially_vulnerable_passwords(vec!["password_A".into()]) .unwrap(); // login2 has a different password → not vulnerable assert!(!db - .is_potentially_vulnerable_password(&login2.meta.id, &*TEST_ENCDEC) + .is_potentially_vulnerable_password(&login2.meta.id) .unwrap()); // Batch check: login1 should be vulnerable (its password is in breachesL) // login2 has a different password, so it's not vulnerable let vulnerable = db - .are_potentially_vulnerable_passwords( - &[&login1.meta.id, &login2.meta.id], - &*TEST_ENCDEC, - ) + .are_potentially_vulnerable_passwords(&[&login1.meta.id, &login2.meta.id]) .unwrap(); assert_eq!(vulnerable.len(), 1); assert!(vulnerable.contains(&login1.meta.id)); diff --git a/components/logins/src/login.rs b/components/logins/src/login.rs index afef2c0fd1..fad70d7a96 100644 --- a/components/logins/src/login.rs +++ b/components/logins/src/login.rs @@ -278,7 +278,7 @@ //! - `Login::fixup()`: Returns either the existing login if it is valid, a clone with invalid fields //! fixed up if it was safe to do so, or an error if the login is irreparably invalid. -use crate::{encryption::EncryptorDecryptor, error::*}; +use crate::{encryption::EncryptorDecryptor, error::*, warn, LoginDb}; use rusqlite::Row; use serde_derive::*; use sync_guid::Guid; @@ -668,13 +668,21 @@ impl EncryptedLogin { &self.meta.id } - pub fn decrypt(self, encdec: &dyn EncryptorDecryptor) -> Result { - let sec_fields = self.decrypt_fields(encdec)?; + pub fn decrypt(self, db: &LoginDb) -> Result { + let sec_fields = self.decrypt_fields(db)?; Ok(Login::new(self.meta, self.fields, sec_fields)) } - pub fn decrypt_fields(&self, encdec: &dyn EncryptorDecryptor) -> Result { - SecureLoginFields::decrypt(&self.sec_fields, encdec, &self.meta.id) + pub fn decrypt_fields(&self, db: &LoginDb) -> Result { + SecureLoginFields::decrypt(&self.sec_fields, db.encdec.as_ref(), &self.meta.id).inspect_err( + |e| { + if matches!(e, Error::DecryptionFailed(_)) { + if let Err(e) = db.wipe_local_login_data(&self.meta.id) { + warn!("Failed to wipe login on decryption failure: {e}"); + } + } + }, + ) } pub(crate) fn from_row(row: &Row<'_>) -> Result { diff --git a/components/logins/src/store.rs b/components/logins/src/store.rs index 1ddeebc487..a77a7cc1eb 100644 --- a/components/logins/src/store.rs +++ b/components/logins/src/store.rs @@ -57,12 +57,9 @@ fn create_sync_engine( } } -fn map_bulk_result_entry( - enc_login: Result, - encdec: &dyn EncryptorDecryptor, -) -> BulkResultEntry { +fn map_bulk_result_entry(enc_login: Result, db: &LoginDb) -> BulkResultEntry { match enc_login { - Ok(enc_login) => match enc_login.decrypt(encdec) { + Ok(enc_login) => match enc_login.decrypt(db) { Ok(login) => BulkResultEntry::Success { login }, Err(error) => { warn!("Login could not be decrypted. This indicates a fundamental problem with the encryption key."); @@ -113,12 +110,8 @@ impl LoginStore { #[handle_error(Error)] pub fn list(&self) -> ApiResult> { let db = self.lock_db()?; - db.get_all().and_then(|logins| { - logins - .into_iter() - .map(|login| login.decrypt(db.encdec.as_ref())) - .collect() - }) + db.get_all() + .and_then(|logins| logins.into_iter().map(|login| login.decrypt(&db)).collect()) } #[handle_error(Error)] @@ -142,7 +135,7 @@ impl LoginStore { let db = self.lock_db()?; match db.get_by_id(id) { Ok(result) => match result { - Some(enc_login) => enc_login.decrypt(db.encdec.as_ref()).map(Some), + Some(enc_login) => enc_login.decrypt(&db).map(Some), None => Ok(None), }, Err(err) => Err(err), @@ -152,12 +145,8 @@ impl LoginStore { #[handle_error(Error)] pub fn get_by_base_domain(&self, base_domain: &str) -> ApiResult> { let db = self.lock_db()?; - db.get_by_base_domain(base_domain).and_then(|logins| { - logins - .into_iter() - .map(|login| login.decrypt(db.encdec.as_ref())) - .collect() - }) + db.get_by_base_domain(base_domain) + .and_then(|logins| logins.into_iter().map(|login| login.decrypt(&db)).collect()) } #[handle_error(Error)] @@ -170,7 +159,7 @@ impl LoginStore { #[handle_error(Error)] pub fn find_login_to_update(&self, entry: LoginEntry) -> ApiResult> { let db = self.lock_db()?; - db.find_login_to_update(entry, db.encdec.as_ref()) + db.find_login_to_update(entry) } #[handle_error(Error)] @@ -183,19 +172,19 @@ impl LoginStore { // Note: Vec<&str> is not supported with UDL, so we receive Vec and convert let db = self.lock_db()?; let ids: Vec<&str> = ids.iter().map(|id| &**id).collect(); - db.are_potentially_vulnerable_passwords(&ids, db.encdec.as_ref()) + db.are_potentially_vulnerable_passwords(&ids) } #[handle_error(Error)] pub fn is_potentially_vulnerable_password(&self, id: &str) -> ApiResult { let db = self.lock_db()?; - db.is_potentially_vulnerable_password(id, db.encdec.as_ref()) + db.is_potentially_vulnerable_password(id) } #[handle_error(Error)] pub fn record_potentially_vulnerable_passwords(&self, passwords: Vec) -> ApiResult<()> { let db = self.lock_db()?; - db.record_potentially_vulnerable_passwords(passwords, db.encdec.as_ref()) + db.record_potentially_vulnerable_passwords(passwords) } #[handle_error(Error)] @@ -239,8 +228,7 @@ impl LoginStore { let engine = LoginsSyncEngine::new(Arc::clone(&self))?; let db = self.lock_db()?; - let deletion_stats = - db.delete_undecryptable_records_for_remote_replacement(db.encdec.as_ref())?; + let deletion_stats = db.delete_undecryptable_records_for_remote_replacement()?; engine.set_last_sync(&db, ServerTimestamp(0))?; Ok(deletion_stats) } @@ -264,24 +252,23 @@ impl LoginStore { #[handle_error(Error)] pub fn update(&self, id: &str, entry: LoginEntry) -> ApiResult { let db = self.lock_db()?; - db.update(id, entry, db.encdec.as_ref()) - .and_then(|enc_login| enc_login.decrypt(db.encdec.as_ref())) + db.update(id, entry) + .and_then(|enc_login| enc_login.decrypt(&db)) } #[handle_error(Error)] pub fn add(&self, entry: LoginEntry) -> ApiResult { let db = self.lock_db()?; - db.add(entry, db.encdec.as_ref()) - .and_then(|enc_login| enc_login.decrypt(db.encdec.as_ref())) + db.add(entry).and_then(|enc_login| enc_login.decrypt(&db)) } #[handle_error(Error)] pub fn add_many(&self, entries: Vec) -> ApiResult> { let db = self.lock_db()?; - db.add_many(entries, db.encdec.as_ref()).map(|enc_logins| { + db.add_many(entries).map(|enc_logins| { enc_logins .into_iter() - .map(|enc_login| map_bulk_result_entry(enc_login, db.encdec.as_ref())) + .map(|enc_login| map_bulk_result_entry(enc_login, &db)) .collect() }) } @@ -292,8 +279,8 @@ impl LoginStore { #[handle_error(Error)] pub fn add_with_meta(&self, entry_with_meta: LoginEntryWithMeta) -> ApiResult { let db = self.lock_db()?; - db.add_with_meta(entry_with_meta, db.encdec.as_ref()) - .and_then(|enc_login| enc_login.decrypt(db.encdec.as_ref())) + db.add_with_meta(entry_with_meta) + .and_then(|enc_login| enc_login.decrypt(&db)) } #[handle_error(Error)] @@ -302,20 +289,19 @@ impl LoginStore { entries_with_meta: Vec, ) -> ApiResult> { let db = self.lock_db()?; - db.add_many_with_meta(entries_with_meta, db.encdec.as_ref()) - .map(|enc_logins| { - enc_logins - .into_iter() - .map(|enc_login| map_bulk_result_entry(enc_login, db.encdec.as_ref())) - .collect() - }) + db.add_many_with_meta(entries_with_meta).map(|enc_logins| { + enc_logins + .into_iter() + .map(|enc_login| map_bulk_result_entry(enc_login, &db)) + .collect() + }) } #[handle_error(Error)] pub fn add_or_update(&self, entry: LoginEntry) -> ApiResult { let db = self.lock_db()?; - db.add_or_update(entry, db.encdec.as_ref()) - .and_then(|enc_login| enc_login.decrypt(db.encdec.as_ref())) + db.add_or_update(entry) + .and_then(|enc_login| enc_login.decrypt(&db)) } #[handle_error(Error)] @@ -368,7 +354,6 @@ impl LoginStore { #[cfg(test)] mod tests { use super::*; - use crate::encryption::test_utils::TEST_ENCDEC; use crate::util; use nss::ensure_initialized; use std::cmp::Reverse; @@ -534,18 +519,15 @@ mod tests { ensure_initialized(); // If the database has data, then wipe_local() returns > 0 rows deleted let db = LoginDb::open_in_memory(); - db.add_or_update( - LoginEntry { - origin: "https://www.example.com".into(), - form_action_origin: Some("https://www.example.com".into()), - username_field: "user_input".into(), - password_field: "pass_input".into(), - username: "coolperson21".into(), - password: "p4ssw0rd".into(), - ..Default::default() - }, - &TEST_ENCDEC.clone(), - ) + db.add_or_update(LoginEntry { + origin: "https://www.example.com".into(), + form_action_origin: Some("https://www.example.com".into()), + username_field: "user_input".into(), + password_field: "pass_input".into(), + username: "coolperson21".into(), + password: "p4ssw0rd".into(), + ..Default::default() + }) .unwrap(); assert!(db.wipe_local().unwrap() > 0); diff --git a/components/logins/src/sync/engine.rs b/components/logins/src/sync/engine.rs index 2dc721c6bc..8ff6d97c10 100644 --- a/components/logins/src/sync/engine.rs +++ b/components/logins/src/sync/engine.rs @@ -6,7 +6,6 @@ use super::merge::{LocalLogin, MirrorLogin, SyncLoginData}; use super::update_plan::UpdatePlan; use super::SyncStatus; use crate::db::CLONE_ENTIRE_MIRROR_SQL; -use crate::encryption::EncryptorDecryptor; use crate::error::*; use crate::login::EncryptedLogin; use crate::schema; @@ -29,7 +28,6 @@ use sync_guid::Guid; pub struct LoginsSyncEngine { pub store: Arc, pub scope: SqlInterruptScope, - pub encdec: Arc, pub staged: RefCell>, } @@ -37,11 +35,9 @@ impl LoginsSyncEngine { pub fn new(store: Arc) -> Result { let db = store.lock_db()?; let scope = db.begin_interrupt_scope()?; - let encdec = db.encdec.clone(); drop(db); Ok(Self { store, - encdec, scope, staged: RefCell::new(vec![]), }) @@ -75,7 +71,7 @@ impl LoginsSyncEngine { upstream, upstream_time, server_now, - self.encdec.as_ref(), + &*self.store.lock_db()?, )?; telem.reconciled(1); } @@ -138,7 +134,12 @@ impl LoginsSyncEngine { let mut seen_ids: HashSet = HashSet::with_capacity(records.len()); for incoming in records.into_iter() { let id = incoming.envelope.id.clone(); - match SyncLoginData::from_bso(incoming, self.encdec.as_ref()) { + let encdec = self.store.lock_db()?.encdec.clone(); + // Clone the `encdec` from our database so that we can encrypt logins. + // This ensures we don't keep the DB locked longer than we need, since we only need + // the `encdec`. + + match SyncLoginData::from_bso(incoming, encdec.as_ref()) { Ok(v) => sync_data.push(v), Err(e) => { match e { @@ -259,8 +260,7 @@ impl LoginsSyncEngine { OutgoingBso::new_tombstone(envelope) } else { let unknown = row.get::<_, Option>("enc_unknown_fields")?; - let mut bso = - EncryptedLogin::from_row(row)?.into_bso(self.encdec.as_ref(), unknown)?; + let mut bso = EncryptedLogin::from_row(row)?.into_bso(&db, unknown)?; bso.envelope.sortindex = Some(DEFAULT_SORTINDEX); bso }) @@ -293,8 +293,9 @@ impl LoginsSyncEngine { } fn get_last_sync(&self, db: &LoginDb) -> Result> { - let millis = db.get_meta::(schema::LAST_SYNC_META_KEY)?.unwrap(); - Ok(Some(ServerTimestamp(millis))) + Ok(db + .get_meta::(schema::LAST_SYNC_META_KEY)? + .map(ServerTimestamp)) } fn mark_as_synchronized(&self, guids: &[&str], ts: ServerTimestamp) -> Result<()> { @@ -373,12 +374,13 @@ impl LoginsSyncEngine { // This is subtly different from dupe handling by the main API and maybe // could be consolidated, but for now it remains sync specific. pub(crate) fn find_dupe_login(&self, l: &EncryptedLogin) -> Result> { + let db = self.store.lock_db()?; let form_submit_host_port = l .fields .form_action_origin .as_ref() .and_then(|s| util::url_host_port(s)); - let enc_fields = l.decrypt_fields(self.encdec.as_ref())?; + let enc_fields = l.decrypt_fields(&db)?; let args = named_params! { ":origin": l.fields.origin, ":http_realm": l.fields.http_realm, @@ -397,13 +399,12 @@ impl LoginsSyncEngine { } else { query += " AND formActionOrigin IS :form_submit" } - let db = self.store.lock_db()?; let mut stmt = db.prepare_cached(&query)?; for login in stmt .query_and_then(args, EncryptedLogin::from_row)? .collect::>>()? { - let this_enc_fields = login.decrypt_fields(self.encdec.as_ref())?; + let this_enc_fields = login.decrypt_fields(&db)?; if enc_fields.username == this_enc_fields.username { return Ok(Some(login)); } @@ -504,50 +505,52 @@ mod tests { engine.fetch_outgoing().unwrap() } + fn apply_incoming_payload( + engine: &LoginsSyncEngine, + server_timestamp: ServerTimestamp, + payload: serde_json::Value, + ) { + let bso = IncomingBso::from_test_content(payload); + let mut telem = sync15::telemetry::Engine::new(engine.collection_name()); + engine.stage_incoming(vec![bso], &mut telem).unwrap(); + engine.apply(server_timestamp, &mut telem).unwrap(); + } + #[test] fn test_fetch_login_data() { ensure_initialized(); // Test some common cases with fetch_login data - let store = LoginStore::new_in_memory(); - insert_login( - &store.lock_db().unwrap(), - "updated_remotely", - None, - Some("password"), - ); - insert_login( - &store.lock_db().unwrap(), - "deleted_remotely", - None, - Some("password"), - ); + let store = Arc::new(LoginStore::new_in_memory()); + let db = store.lock_db().unwrap(); + insert_login(&db, "updated_remotely", None, Some("password")); + insert_login(&db, "deleted_remotely", None, Some("password")); insert_login( - &store.lock_db().unwrap(), + &db, "three_way_merge", Some("new-local-password"), Some("password"), ); + let expected_fetch_results = vec![ + IncomingBso::new_test_tombstone(Guid::new("deleted_remotely")), + enc_login("added_remotely", "password") + .into_bso(&db, None) + .unwrap() + .to_test_incoming(), + enc_login("updated_remotely", "new-password") + .into_bso(&db, None) + .unwrap() + .to_test_incoming(), + enc_login("three_way_merge", "new-remote-password") + .into_bso(&db, None) + .unwrap() + .to_test_incoming(), + ]; + // make sure to drop our mutex guard to avoid deadlocks + drop(db); - let mut engine = LoginsSyncEngine::new(Arc::new(store)).unwrap(); + let mut engine = LoginsSyncEngine::new(store.clone()).unwrap(); - let (res, _) = run_fetch_login_data( - &mut engine, - vec![ - IncomingBso::new_test_tombstone(Guid::new("deleted_remotely")), - enc_login("added_remotely", "password") - .into_bso(&*TEST_ENCDEC, None) - .unwrap() - .to_test_incoming(), - enc_login("updated_remotely", "new-password") - .into_bso(&*TEST_ENCDEC, None) - .unwrap() - .to_test_incoming(), - enc_login("three_way_merge", "new-remote-password") - .into_bso(&*TEST_ENCDEC, None) - .unwrap() - .to_test_incoming(), - ], - ); + let (res, _) = run_fetch_login_data(&mut engine, expected_fetch_results); // For simpler testing, extract/decrypt passwords and put them in a hash map #[derive(Debug, PartialEq)] struct SyncPasswords { @@ -558,6 +561,7 @@ mod tests { let extracted_passwords: HashMap = res .into_iter() .map(|sync_login_data| { + let db = store.lock_db().unwrap(); let mut guids_seen = HashSet::new(); let passwords = SyncPasswords { local: sync_login_data.local.map(|local_login| { @@ -565,23 +569,15 @@ mod tests { let LocalLogin::Alive { login, .. } = local_login else { unreachable!("this test is not expecting a tombstone"); }; - login.decrypt_fields(&*TEST_ENCDEC).unwrap().password + login.decrypt_fields(&db).unwrap().password }), mirror: sync_login_data.mirror.map(|mirror_login| { guids_seen.insert(mirror_login.login.meta.id.clone()); - mirror_login - .login - .decrypt_fields(&*TEST_ENCDEC) - .unwrap() - .password + mirror_login.login.decrypt_fields(&db).unwrap().password }), inbound: sync_login_data.inbound.map(|incoming| { guids_seen.insert(incoming.login.meta.id.clone()); - incoming - .login - .decrypt_fields(&*TEST_ENCDEC) - .unwrap() - .password + incoming.login.decrypt_fields(&db).unwrap().password }), }; (guids_seen.into_iter().next().unwrap(), passwords) @@ -975,15 +971,9 @@ mod tests { #[test] fn test_roundtrip_unknown() { ensure_initialized(); - // A couple of helpers - fn apply_incoming_payload(engine: &LoginsSyncEngine, payload: serde_json::Value) { - let bso = IncomingBso::from_test_content(payload); - let mut telem = sync15::telemetry::Engine::new(engine.collection_name()); - engine.stage_incoming(vec![bso], &mut telem).unwrap(); - engine - .apply(ServerTimestamp::from_millis(0), &mut telem) - .unwrap(); - } + // The test itself... + let store = LoginStore::new_in_memory(); + let engine = LoginsSyncEngine::new(Arc::new(store)).unwrap(); fn get_outgoing_payload(engine: &LoginsSyncEngine) -> serde_json::Value { // Edit it so it's considered outgoing. @@ -1005,12 +995,9 @@ mod tests { serde_json::from_str::(&changeset[0].payload).unwrap() } - // The test itself... - let store = LoginStore::new_in_memory(); - let engine = LoginsSyncEngine::new(Arc::new(store)).unwrap(); - apply_incoming_payload( &engine, + ServerTimestamp::from_millis(0), serde_json::json!({ "id": "dummy_000001", "formSubmitURL": "https://www.example.com/submit", @@ -1035,6 +1022,7 @@ mod tests { // incoming with different unknown fields. apply_incoming_payload( &engine, + ServerTimestamp::from_millis(0), serde_json::json!({ "id": "dummy_000001", "formSubmitURL": "https://www.example.com/submit", @@ -1078,15 +1066,6 @@ mod tests { fn do_test_incoming_with_local_unmirrored_tombstone(local_newer: bool) { ensure_initialized(); - fn apply_incoming_payload(engine: &LoginsSyncEngine, payload: serde_json::Value) { - let bso = IncomingBso::from_test_content(payload); - let mut telem = sync15::telemetry::Engine::new(engine.collection_name()); - engine.stage_incoming(vec![bso], &mut telem).unwrap(); - engine - .apply(ServerTimestamp::from_millis(0), &mut telem) - .unwrap(); - } - // The test itself... let (local_timestamp, remote_timestamp) = if local_newer { (123, 0) } else { (0, 123) }; @@ -1096,6 +1075,7 @@ mod tests { // apply an incoming record - will be in the mirror. apply_incoming_payload( &engine, + ServerTimestamp::from_millis(0), serde_json::json!({ "id": "dummy_000001", "formSubmitURL": "https://www.example.com/submit", @@ -1132,6 +1112,7 @@ mod tests { // Now we assume we've been reconnected to sync and have an incoming change for the record. apply_incoming_payload( &engine, + ServerTimestamp::from_millis(0), serde_json::json!({ "id": "dummy_000001", "formSubmitURL": "https://www.example.com/submit", @@ -1170,4 +1151,143 @@ mod tests { fn test_incoming_non_mirror_tombstone_local_older() { do_test_incoming_with_local_unmirrored_tombstone(false); } + + #[test] + fn test_sync_after_wipe_local_login_data() { + ensure_initialized(); + + let store = Arc::new(LoginStore::new_in_memory()); + let engine = LoginsSyncEngine::new(store.clone()).unwrap(); + + apply_incoming_payload( + &engine, + ServerTimestamp::from_millis(0), + serde_json::json!({ + "id": "dummy_000001", + "formSubmitURL": "https://www.example.com/submit", + "hostname": "https://www.example.com", + "username": "test", + "password": "test", + }), + ); + // Wiping a local login means it's removed from the DB + store + .lock_db() + .unwrap() + .wipe_local_login_data("dummy_000001") + .unwrap(); + assert!(store + .lock_db() + .unwrap() + .get_by_id("dummy_000001") + .unwrap() + .is_none()); + // If we sync again with that login, the data should be restored + apply_incoming_payload( + &engine, + ServerTimestamp::from_millis(0), + serde_json::json!({ + "id": "dummy_000001", + "formSubmitURL": "https://www.example.com/submit", + "hostname": "https://www.example.com", + "username": "test", + "password": "test", + }), + ); + // There should be no outgoing records, we do not want to send out a tombstone for the + // wiped login + assert_eq!(engine.fetch_outgoing().unwrap().len(), 0); + // The wiped login should be restored + assert!(store + .lock_db() + .unwrap() + .get_by_id("dummy_000001") + .unwrap() + .is_some()); + } + + #[test] + fn test_decryption_errors() { + ensure_initialized(); + + let store = Arc::new(LoginStore::new_in_memory()); + let engine = LoginsSyncEngine::new(store.clone()).unwrap(); + engine + .set_last_sync(&store.lock_db().unwrap(), ServerTimestamp::from_millis(0)) + .unwrap(); + + // Simulate the initial sync where we get one login + apply_incoming_payload( + &engine, + ServerTimestamp::from_millis(100), + serde_json::json!({ + "id": "dummy_000001", + "formSubmitURL": "https://www.example.com/submit", + "hostname": "https://www.example.com", + "username": "test", + "password": "test", + }), + ); + engine + .set_last_sync(&store.lock_db().unwrap(), ServerTimestamp::from_millis(100)) + .unwrap(); + + // Simulate `secFields` being corrupted so that the login can't be decrypted + let db = store.lock_db().unwrap(); + db.conn() + .execute( + "UPDATE loginsL SET secFields ='corrupted-data' WHERE guid = 'dummy_000001'", + (), + ) + .unwrap(); + db.conn() + .execute( + "UPDATE loginsM SET secFields ='corrupted-data' WHERE guid = 'dummy_000001'", + (), + ) + .unwrap(); + db.touch("dummy_000001").unwrap(); + drop(db); + + // On the next sync, we should notice that the login is undecryptable when + // preparing our outgoing payload. + assert!(matches!( + engine.fetch_outgoing(), + Err(Error::DecryptionFailed(_)) + )); + + // On the sync after that, we should request all the sync data again + assert_eq!( + engine + .get_collection_request(ServerTimestamp::from_millis(100)) + .unwrap(), + Some( + CollectionRequest::new("passwords".into()) + .full() + .newer_than(ServerTimestamp::from_millis(0)), + ) + ); + // And after applying the incoming payload, we should have the login back + apply_incoming_payload( + &engine, + ServerTimestamp::from_millis(100), + serde_json::json!({ + "id": "dummy_000001", + "formSubmitURL": "https://www.example.com/submit", + "hostname": "https://www.example.com", + "username": "test", + "password": "test", + }), + ); + let db = store.lock_db().unwrap(); + assert_eq!( + db.get_by_id("dummy_000001") + .unwrap() + .unwrap() + .decrypt(&db) + .unwrap() + .username, + "test" + ); + } } diff --git a/components/logins/src/sync/merge.rs b/components/logins/src/sync/merge.rs index a64edf67a9..8917a60c29 100644 --- a/components/logins/src/sync/merge.rs +++ b/components/logins/src/sync/merge.rs @@ -5,9 +5,9 @@ // Merging for Sync. use super::{IncomingLogin, LoginPayload}; use crate::encryption::EncryptorDecryptor; -use crate::error::*; use crate::login::EncryptedLogin; use crate::util; +use crate::{error::*, LoginDb}; use rusqlite::Row; use std::time::SystemTime; use sync15::bso::{IncomingBso, IncomingKind}; @@ -277,11 +277,7 @@ macro_rules! apply_metadata_field { } impl EncryptedLogin { - pub(crate) fn apply_delta( - &mut self, - mut delta: LoginDelta, - encdec: &dyn EncryptorDecryptor, - ) -> Result<()> { + pub(crate) fn apply_delta(&mut self, mut delta: LoginDelta, db: &LoginDb) -> Result<()> { apply_field!(self, delta, origin); apply_metadata_field!(self, delta, time_created); @@ -291,14 +287,14 @@ impl EncryptedLogin { apply_field!(self, delta, password_field); apply_field!(self, delta, username_field); - let mut sec_fields = self.decrypt_fields(encdec)?; + let mut sec_fields = self.decrypt_fields(db)?; if let Some(password) = delta.password.take() { sec_fields.password = password; } if let Some(username) = delta.username.take() { sec_fields.username = username; } - self.sec_fields = sec_fields.encrypt(encdec, &self.meta.id)?; + self.sec_fields = sec_fields.encrypt(db.encdec.as_ref(), &self.meta.id)?; // Use Some("") to indicate that it should be changed to be None (hacky...) if let Some(realm) = delta.http_realm.take() { @@ -313,11 +309,7 @@ impl EncryptedLogin { Ok(()) } - pub(crate) fn delta( - &self, - older: &EncryptedLogin, - encdec: &dyn EncryptorDecryptor, - ) -> Result { + pub(crate) fn delta(&self, older: &EncryptedLogin, db: &LoginDb) -> Result { let mut delta = LoginDelta::default(); if self.fields.form_action_origin != older.fields.form_action_origin { @@ -332,8 +324,8 @@ impl EncryptedLogin { if self.fields.origin != older.fields.origin { delta.origin = Some(self.fields.origin.clone()); } - let older_sec_fields = older.decrypt_fields(encdec)?; - let self_sec_fields = self.decrypt_fields(encdec)?; + let older_sec_fields = older.decrypt_fields(db)?; + let self_sec_fields = self.decrypt_fields(db)?; if self_sec_fields.username != older_sec_fields.username { delta.username = Some(self_sec_fields.username.clone()); } diff --git a/components/logins/src/sync/payload.rs b/components/logins/src/sync/payload.rs index a273221f95..162764e574 100644 --- a/components/logins/src/sync/payload.rs +++ b/components/logins/src/sync/payload.rs @@ -11,7 +11,7 @@ use crate::encryption::EncryptorDecryptor; use crate::error::*; use crate::login::ValidateAndFixup; use crate::SecureLoginFields; -use crate::{EncryptedLogin, LoginEntry, LoginFields, LoginMeta}; +use crate::{EncryptedLogin, LoginDb, LoginEntry, LoginFields, LoginMeta}; use serde_derive::*; use sync15::bso::OutgoingBso; use sync_guid::Guid; @@ -178,16 +178,12 @@ pub struct LoginPayload { // These probably should be on the payload itself, but one refactor at a time! impl EncryptedLogin { - pub fn into_bso( - self, - encdec: &dyn EncryptorDecryptor, - enc_unknown_fields: Option, - ) -> Result { + pub fn into_bso(self, db: &LoginDb, enc_unknown_fields: Option) -> Result { let unknown_fields = match enc_unknown_fields { - Some(s) => UnknownFields::decrypt(&s, encdec)?, + Some(s) => UnknownFields::decrypt(&s, db.encdec.as_ref())?, None => Default::default(), }; - let sec_fields = SecureLoginFields::decrypt(&self.sec_fields, encdec, &self.meta.id)?; + let sec_fields = self.decrypt_fields(db)?; Ok(OutgoingBso::from_content_with_id( crate::sync::LoginPayload { guid: self.guid(), @@ -263,7 +259,8 @@ mod tests { assert_eq!(login.fields.http_realm, Some("test".to_string())); assert_eq!(login.fields.origin, "https://www.example.com"); assert_eq!(login.fields.form_action_origin, None); - let sec_fields = login.decrypt_fields(&*TEST_ENCDEC).unwrap(); + let db = LoginDb::open_in_memory(); + let sec_fields = login.decrypt_fields(&db).unwrap(); assert_eq!(sec_fields.username, "user"); assert_eq!(sec_fields.password, "password"); } @@ -290,11 +287,13 @@ mod tests { assert_eq!(login.fields.form_action_origin, Some("".to_string())); assert_eq!(login.fields.http_realm, None); assert_eq!(login.fields.origin, "https://www.example.com"); - let sec_fields = login.decrypt_fields(&*TEST_ENCDEC).unwrap(); + let db = LoginDb::open_in_memory(); + let sec_fields = login.decrypt_fields(&db).unwrap(); assert_eq!(sec_fields.username, "user"); assert_eq!(sec_fields.password, "password"); - let bso = login.into_bso(&*TEST_ENCDEC, None).unwrap(); + let db = LoginDb::open_in_memory(); + let bso = login.into_bso(&db, None).unwrap(); assert_eq!(bso.envelope.id, "123412341234"); let payload_data: serde_json::Value = serde_json::from_str(&bso.payload).unwrap(); assert_eq!(payload_data["httpRealm"], serde_json::Value::Null); @@ -335,7 +334,8 @@ mod tests { .unwrap() .login; // The raw outgoing payload should have it back. - let outgoing = login.into_bso(&*TEST_ENCDEC, unknown).unwrap(); + let db = LoginDb::open_in_memory(); + let outgoing = login.into_bso(&db, unknown).unwrap(); let json = serde_json::from_str::>(&outgoing.payload) .unwrap(); @@ -366,7 +366,8 @@ mod tests { Some("https://www.example.com".to_string()) ); assert_eq!(login.fields.username_field, "username-field"); - let sec_fields = login.decrypt_fields(&*TEST_ENCDEC).unwrap(); + let db = LoginDb::open_in_memory(); + let sec_fields = login.decrypt_fields(&db).unwrap(); assert_eq!(sec_fields.username, "user"); assert_eq!(sec_fields.password, "password"); } @@ -388,7 +389,8 @@ mod tests { password: "password".into(), }), }; - let bso = login.into_bso(&*TEST_ENCDEC, None).unwrap(); + let db = LoginDb::open_in_memory(); + let bso = login.into_bso(&db, None).unwrap(); assert_eq!(bso.envelope.id, "123412341234"); let payload_data: serde_json::Value = serde_json::from_str(&bso.payload).unwrap(); assert_eq!(payload_data["httpRealm"], "test".to_string()); diff --git a/components/logins/src/sync/update_plan.rs b/components/logins/src/sync/update_plan.rs index 50a77583cc..9f1178c30f 100644 --- a/components/logins/src/sync/update_plan.rs +++ b/components/logins/src/sync/update_plan.rs @@ -4,9 +4,8 @@ use super::merge::{LocalLogin, MirrorLogin}; use super::{IncomingLogin, SyncStatus}; -use crate::encryption::EncryptorDecryptor; -use crate::error::*; use crate::util; +use crate::{error::*, LoginDb}; use interrupt_support::SqlInterruptScope; use rusqlite::{named_params, Connection}; use std::time::SystemTime; @@ -54,7 +53,7 @@ impl UpdatePlan { upstream: IncomingLogin, upstream_time: ServerTimestamp, server_now: ServerTimestamp, - encdec: &dyn EncryptorDecryptor, + db: &LoginDb, ) -> Result<()> { let local_age = SystemTime::now() .duration_since(local.local_modified()) @@ -62,7 +61,7 @@ impl UpdatePlan { let remote_age = server_now.duration_since(upstream_time).unwrap_or_default(); let delta = { - let upstream_delta = upstream.login.delta(&shared.login, encdec)?; + let upstream_delta = upstream.login.delta(&shared.login, db)?; match local { LocalLogin::Tombstone { .. } => { // If the login was deleted locally, the merged delta is the @@ -78,7 +77,7 @@ impl UpdatePlan { upstream_delta } LocalLogin::Alive { login, .. } => { - let local_delta = login.delta(&shared.login, encdec)?; + let local_delta = login.delta(&shared.login, db)?; local_delta.merge(upstream_delta, remote_age < local_age) } } @@ -89,7 +88,7 @@ impl UpdatePlan { .push((upstream, upstream_time.as_millis())); let mut new = shared; - new.login.apply_delta(delta, encdec)?; + new.login.apply_delta(delta, db)?; new.server_modified = upstream_time; self.local_updates.push(new); Ok(()) @@ -325,7 +324,6 @@ mod tests { get_server_modified, insert_encrypted_login, insert_login, }; use crate::db::LoginDb; - use crate::encryption::test_utils::TEST_ENCDEC; use crate::login::test_utils::enc_login; fn inc_login(id: &str, password: &str) -> crate::sync::IncomingLogin { @@ -477,7 +475,7 @@ mod tests { upstream_login, server_record_timestamp.try_into().unwrap(), server_timestamp.try_into().unwrap(), - &*TEST_ENCDEC, + &db, ) .unwrap(); update_plan @@ -544,7 +542,7 @@ mod tests { upstream_login, server_record_timestamp.try_into().unwrap(), server_timestamp.try_into().unwrap(), - &*TEST_ENCDEC, + &db, ) .unwrap(); update_plan @@ -616,7 +614,7 @@ mod tests { upstream_login, server_record_timestamp.try_into().unwrap(), server_timestamp.try_into().unwrap(), - &*TEST_ENCDEC, + &db, ) .unwrap(); update_plan