From cbfbb1759d83ae04e7d8f46b43d1a079c8060145 Mon Sep 17 00:00:00 2001 From: theidkamp Date: Wed, 2 Sep 2026 14:18:29 +0200 Subject: [PATCH 1/3] FXCM-2281: Integrate EncryptorDecryptor into the autofill database Move autofill's credit-card encryption onto the shared db-crypto crate and let AutofillDb own the encryptor, as logins' LoginDb does. The consumer supplies it when building the store, so no key is passed into individual calls or down through the sync layers. --- CHANGELOG.md | 6 ++ Cargo.lock | 1 + components/autofill/Cargo.toml | 1 + components/autofill/src/autofill.udl | 7 +- components/autofill/src/db/credit_cards.rs | 38 ++++----- components/autofill/src/db/mod.rs | 24 ++++-- components/autofill/src/db/schema.rs | 3 +- components/autofill/src/db/store.rs | 38 ++++----- components/autofill/src/encryption.rs | 83 ++++++++++++++----- components/autofill/src/error.rs | 18 ++-- components/autofill/src/lib.rs | 1 + components/autofill/src/sync/address/mod.rs | 7 +- components/autofill/src/sync/bridge.rs | 9 +- .../autofill/src/sync/credit_card/incoming.rs | 71 ++++++++-------- .../autofill/src/sync/credit_card/mod.rs | 36 ++++---- .../autofill/src/sync/credit_card/outgoing.rs | 45 +++++----- components/autofill/src/sync/engine.rs | 34 +++----- 17 files changed, 245 insertions(+), 177 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ffce80ff4d3..23d8b3af5fd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,12 @@ [Full Changelog](In progress) +## ⚠️ Breaking Changes ⚠️ + +### Autofill + +- **BREAKING**: `Store::new()` now takes an `EncryptorDecryptor`, which the store hands to the database and which is used for every encrypted column, and `scrub_undecryptable_credit_card_data_for_remote_replacement()` no longer takes an encryption key. Credit-card encryption moved to the shared `db-crypto` crate. `encrypt_string()` and `decrypt_string()` are unchanged. + ## ✨ What's Changed ✨ ### Autofill diff --git a/Cargo.lock b/Cargo.lock index 625905a7e70..1845cbe5235 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -208,6 +208,7 @@ name = "autofill" version = "0.1.0" dependencies = [ "anyhow", + "db-crypto", "error-support", "interrupt-support", "jwcrypto", diff --git a/components/autofill/Cargo.toml b/components/autofill/Cargo.toml index ed0f3199966..6d52686307b 100644 --- a/components/autofill/Cargo.toml +++ b/components/autofill/Cargo.toml @@ -12,6 +12,7 @@ exclude = ["/android", "/ios"] anyhow = "1.0" error-support = { path = "../support/error" } interrupt-support = { path = "../support/interrupt" } +db-crypto = { path = "../support/db-crypto" } jwcrypto = { path = "../support/jwcrypto" } lazy_static = "1.4" parking_lot = ">=0.11,<=0.12" diff --git a/components/autofill/src/autofill.udl b/components/autofill/src/autofill.udl index 7fc40bace05..ab5471fba64 100644 --- a/components/autofill/src/autofill.udl +++ b/components/autofill/src/autofill.udl @@ -164,9 +164,12 @@ interface AutofillApiError { UnexpectedAutofillApiError(string reason); }; +[External = "db_crypto"] +typedef trait_with_foreign EncryptorDecryptor; + interface Store { [Throws=AutofillApiError] - constructor(string dbpath); + constructor(string dbpath, EncryptorDecryptor encdec); [Throws=AutofillApiError] CreditCard add_credit_card(UpdatableCreditCardFields cc); @@ -257,7 +260,7 @@ interface Store { /// NB: This function was created to unblock iOS credit card users who are unable to sync records and should not be used /// outside of this use case. [Throws=AutofillApiError, Self=ByArc] - CreditCardsDeletionMetrics scrub_undecryptable_credit_card_data_for_remote_replacement(string local_encryption_key); + CreditCardsDeletionMetrics scrub_undecryptable_credit_card_data_for_remote_replacement(); /// Run maintenance on the DB /// diff --git a/components/autofill/src/db/credit_cards.rs b/components/autofill/src/db/credit_cards.rs index 3499b646621..ab3902bd5a4 100644 --- a/components/autofill/src/db/credit_cards.rs +++ b/components/autofill/src/db/credit_cards.rs @@ -9,10 +9,11 @@ use crate::db::{ Metadata, }, schema::{CREDIT_CARD_COMMON_COLS, CREDIT_CARD_COMMON_VALS}, + AutofillDb, }; +use crate::encryption::decrypt_str; use crate::error::*; -use jwcrypto::EncryptorDecryptor; use rusqlite::{Connection, Transaction}; use sync_guid::Guid; use types::Timestamp; @@ -226,16 +227,15 @@ pub fn scrub_encrypted_credit_card_data(conn: &Connection) -> Result<()> { } pub fn scrub_undecryptable_credit_card_data_for_remote_replacement( - conn: &Connection, - local_encryption_key: String, + db: &AutofillDb, ) -> Result { + let conn = &db.writer; let tx = conn.unchecked_transaction()?; let mut scrubbed_records = 0; - let encdec = EncryptorDecryptor::new(local_encryption_key.as_str()).unwrap(); let undecryptable_record_ids = get_all_credit_cards(conn)? .into_iter() - .filter(|credit_card| encdec.decrypt(&credit_card.cc_number_enc).is_err()) + .filter(|credit_card| decrypt_str(db.encdec.as_ref(), &credit_card.cc_number_enc).is_err()) .map(|credit_card| credit_card.guid) .collect::>(); @@ -290,7 +290,7 @@ pub fn touch(conn: &Connection, guid: &Guid) -> Result<()> { pub(crate) mod tests { use super::*; use crate::db::test::new_mem_db; - use crate::encryption::EncryptorDecryptor; + use crate::encryption::{encrypt_str, random_key_encryptor}; use nss_as::ensure_initialized; use sync15::bso::IncomingBso; @@ -588,13 +588,13 @@ pub(crate) mod tests { fn test_credit_card_delete() -> Result<()> { ensure_initialized(); let db = new_mem_db(); - let encdec = EncryptorDecryptor::new_with_random_key().unwrap(); + let encdec = db.encdec.clone(); let saved_credit_card = add_credit_card( &db, UpdatableCreditCardFields { cc_name: "john deer".to_string(), - cc_number_enc: encdec.encrypt("1234567812345678")?, + cc_number_enc: encrypt_str(encdec.as_ref(), "1234567812345678")?, cc_number_last_4: "5678".to_string(), cc_exp_month: 10, cc_exp_year: 2025, @@ -610,7 +610,7 @@ pub(crate) mod tests { &db, UpdatableCreditCardFields { cc_name: "john doe".to_string(), - cc_number_enc: encdec.encrypt("1234123412341234")?, + cc_number_enc: encrypt_str(encdec.as_ref(), "1234123412341234")?, cc_number_last_4: "1234".to_string(), cc_exp_month: 5, cc_exp_year: 2024, @@ -620,7 +620,8 @@ pub(crate) mod tests { // create a mirror record to check that a tombstone record is created upon deletion let cc2_guid = saved_credit_card2.guid.clone(); - let payload = saved_credit_card2.into_test_incoming_bso(&encdec, Default::default()); + let payload = + saved_credit_card2.into_test_incoming_bso(encdec.as_ref(), Default::default()); test_insert_mirror_record(&db, payload); @@ -656,14 +657,14 @@ pub(crate) mod tests { fn test_scrub_encrypted_credit_card_data() -> Result<()> { ensure_initialized(); let db = new_mem_db(); - let encdec = EncryptorDecryptor::new_with_random_key().unwrap(); + let encdec = db.encdec.clone(); let mut saved_credit_cards = Vec::with_capacity(10); for _ in 0..5 { saved_credit_cards.push(add_credit_card( &db, UpdatableCreditCardFields { cc_name: "john deer".to_string(), - cc_number_enc: encdec.encrypt("1234567812345678")?, + cc_number_enc: encrypt_str(encdec.as_ref(), "1234567812345678")?, cc_number_last_4: "5678".to_string(), cc_exp_month: 10, cc_exp_year: 2025, @@ -682,19 +683,16 @@ pub(crate) mod tests { } #[test] - fn test_scrub_undecryptable_credit_card_date_for_remote_replacement() -> Result<()> { + fn test_scrub_undecryptable_credit_card_data_for_remote_replacement() -> Result<()> { ensure_initialized(); let db = new_mem_db(); - let old_key = EncryptorDecryptor::create_key()?; - let old_encdec = EncryptorDecryptor::new(&old_key)?; - let key = EncryptorDecryptor::create_key()?; - let encdec = EncryptorDecryptor::new(&key)?; + let foreign_encdec = random_key_encryptor()?; let undecryptable_credit_card = add_credit_card( &db, UpdatableCreditCardFields { cc_name: "jane doe".to_string(), - cc_number_enc: old_encdec.encrypt("2345678923456789")?, + cc_number_enc: encrypt_str(&foreign_encdec, "2345678923456789")?, cc_number_last_4: "6789".to_string(), cc_exp_month: 9, cc_exp_year: 2027, @@ -702,7 +700,7 @@ pub(crate) mod tests { }, )?; - let encrypted_cc_number = encdec.encrypt("567812345678123456781")?; + let encrypted_cc_number = encrypt_str(db.encdec.as_ref(), "567812345678123456781")?; let credit_card = add_credit_card( &db, UpdatableCreditCardFields { @@ -715,7 +713,7 @@ pub(crate) mod tests { }, )?; - let metrics = scrub_undecryptable_credit_card_data_for_remote_replacement(&db.writer, key)?; + let metrics = scrub_undecryptable_credit_card_data_for_remote_replacement(&db)?; assert_eq!(metrics.total_scrubbed_records, 1); let credit_cards = get_all_credit_cards(&db)?; diff --git a/components/autofill/src/db/mod.rs b/components/autofill/src/db/mod.rs index cfb5e35500f..5ba5ac88f26 100644 --- a/components/autofill/src/db/mod.rs +++ b/components/autofill/src/db/mod.rs @@ -9,6 +9,7 @@ pub mod passports; pub mod schema; pub mod store; +use crate::encryption::EncryptorDecryptor; use crate::error::*; use error_support::error; @@ -24,21 +25,22 @@ use url::Url; pub struct AutofillDb { pub writer: Connection, + pub encdec: Arc, interrupt_handle: Arc, } impl AutofillDb { - pub fn new(db_path: impl AsRef) -> Result { + pub fn new(db_path: impl AsRef, encdec: Arc) -> Result { let db_path = normalize_path(db_path)?; - Self::new_named(db_path) + Self::new_named(db_path, encdec) } - pub fn new_memory(db_path: &str) -> Result { + pub fn new_memory(db_path: &str, encdec: Arc) -> Result { let name = PathBuf::from(format!("file:{}?mode=memory&cache=shared", db_path)); - Self::new_named(name) + Self::new_named(name, encdec) } - fn new_named(db_path: PathBuf) -> Result { + fn new_named(db_path: PathBuf, encdec: Arc) -> Result { // We always create the read-write connection for an initial open so // we can create the schema and/or do version upgrades. let flags = OpenFlags::SQLITE_OPEN_NO_MUTEX @@ -55,6 +57,7 @@ impl AutofillDb { Ok(Self { interrupt_handle: Arc::new(SqlInterruptHandle::new(&conn)), writer: conn, + encdec, }) } @@ -152,10 +155,19 @@ pub mod test { // A helper for our tests to get their own memory Api. static ATOMIC_COUNTER: AtomicUsize = AtomicUsize::new(0); + pub fn test_encdec() -> Arc { + nss_as::ensure_initialized(); + Arc::new(crate::encryption::random_key_encryptor().expect("should get a key")) + } + pub fn new_mem_db() -> AutofillDb { + new_mem_db_with_encdec(test_encdec()) + } + + pub fn new_mem_db_with_encdec(encdec: Arc) -> AutofillDb { error_support::init_for_tests(); let counter = ATOMIC_COUNTER.fetch_add(1, Ordering::Relaxed); - AutofillDb::new_memory(&format!("test_autofill-api-{}", counter)) + AutofillDb::new_memory(&format!("test_autofill-api-{}", counter), encdec) .expect("should get an API") } } diff --git a/components/autofill/src/db/schema.rs b/components/autofill/src/db/schema.rs index cf2e0f2d9ce..85a32c7fa9c 100644 --- a/components/autofill/src/db/schema.rs +++ b/components/autofill/src/db/schema.rs @@ -306,7 +306,8 @@ mod tests { fn test_wal_size_is_bounded() { // A memory database has no -wal file, so open a real one. let db_file = MigratedDatabaseFile::new(AutofillConnectionInitializer, ""); - let db = AutofillDb::new(&db_file.path).expect("should open the database"); + let db = AutofillDb::new(&db_file.path, crate::db::test::test_encdec()) + .expect("should open the database"); let journal_mode: String = db .query_row("PRAGMA journal_mode", [], |row| row.get(0)) diff --git a/components/autofill/src/db/store.rs b/components/autofill/src/db/store.rs index 4bb7d1cd257..efb46c7874d 100644 --- a/components/autofill/src/db/store.rs +++ b/components/autofill/src/db/store.rs @@ -11,6 +11,7 @@ use crate::db::models::passport::{Passport, UpdatablePassportFields}; use crate::db::{ addresses, credit_cards, credit_cards::CreditCardsDeletionMetrics, passports, AutofillDb, }; +use crate::encryption::EncryptorDecryptor; use crate::error::*; use error_support::handle_error; use parking_lot::{MappedMutexGuard, Mutex, MutexGuard}; @@ -58,9 +59,9 @@ pub struct Store { impl Store { #[handle_error(Error)] - pub fn new(db_path: impl AsRef) -> ApiResult { + pub fn new(db_path: impl AsRef, encdec: Arc) -> ApiResult { Ok(Self { - db: Mutex::new(Some(AutofillDb::new(db_path)?)), + db: Mutex::new(Some(AutofillDb::new(db_path, encdec)?)), }) } @@ -74,9 +75,12 @@ impl Store { /// Creates a store backed by an in-memory database that shares its memory API (required for autofill sync tests). #[handle_error(Error)] - pub fn new_shared_memory(db_name: &str) -> ApiResult { + pub fn new_shared_memory( + db_name: &str, + encdec: Arc, + ) -> ApiResult { Ok(Self { - db: Mutex::new(Some(AutofillDb::new_memory(db_name)?)), + db: Mutex::new(Some(AutofillDb::new_memory(db_name, encdec)?)), }) } @@ -312,14 +316,10 @@ impl Store { #[handle_error(Error)] pub fn scrub_undecryptable_credit_card_data_for_remote_replacement( self: Arc, - local_encryption_key: String, ) -> ApiResult { let db = self.lock_db()?; let deletion_stats = - credit_cards::scrub_undecryptable_credit_card_data_for_remote_replacement( - &db.writer, - local_encryption_key, - )?; + credit_cards::scrub_undecryptable_credit_card_data_for_remote_replacement(&db)?; // Here we reset the local sync data so that the credit card engine syncs as if // it were the first sync. This will potentially allow a previous sync of the @@ -393,8 +393,8 @@ pub(crate) fn delete_meta(conn: &Connection, key: &str) -> Result<()> { #[cfg(test)] mod tests { use super::*; - use crate::db::test::new_mem_db; - use crate::encryption::EncryptorDecryptor; + use crate::db::test::{new_mem_db, test_encdec}; + use crate::encryption::encrypt_str; use nss_as::ensure_initialized; #[test] @@ -434,7 +434,7 @@ mod tests { #[test] fn test_sync_manager_registration() { - let store = Arc::new(Store::new_shared_memory("sync-mgr-test").unwrap()); + let store = Arc::new(Store::new_shared_memory("sync-mgr-test", test_encdec()).unwrap()); assert_eq!(Arc::strong_count(&store), 1); assert_eq!(Arc::weak_count(&store), 0); Arc::clone(&store).register_with_sync_manager(); @@ -457,7 +457,7 @@ mod tests { #[test] fn test_shutdown_closes_the_store() { - let store = Store::new_shared_memory("shutdown-test").expect("create store"); + let store = Store::new_shared_memory("shutdown-test", test_encdec()).expect("create store"); // Operations succeed before shutdown. assert_eq!(store.count_all_passports().expect("count"), 0); @@ -474,15 +474,15 @@ mod tests { #[test] fn test_scrub_undecryptable_credit_card_data_for_remote_replacement() { ensure_initialized(); - let store = Arc::new(Store::new_shared_memory("sync-mgr-test").expect("create store")); - let key = EncryptorDecryptor::create_key().expect("create key"); - let encdec = EncryptorDecryptor::new(&key).expect("create EncryptorDecryptor"); + let store = + Arc::new(Store::new_shared_memory("scrub-test", test_encdec()).expect("create store")); + // The guard has to go out of scope before we touch the store again. + let encdec = store.lock_db().expect("db").encdec.clone(); store .add_credit_card(UpdatableCreditCardFields { cc_name: "john deer".to_string(), - cc_number_enc: encdec - .encrypt("567812345678123456781") + cc_number_enc: encrypt_str(encdec.as_ref(), "567812345678123456781") .expect("encrypt cc number"), cc_number_last_4: "6781".to_string(), cc_exp_month: 10, @@ -492,7 +492,7 @@ mod tests { .expect("add credit card to database"); store - .scrub_undecryptable_credit_card_data_for_remote_replacement(key) + .scrub_undecryptable_credit_card_data_for_remote_replacement() .expect("scrub credit card record"); } } diff --git a/components/autofill/src/encryption.rs b/components/autofill/src/encryption.rs index b9fa9b1cfac..4d5ae8a6319 100644 --- a/components/autofill/src/encryption.rs +++ b/components/autofill/src/encryption.rs @@ -10,9 +10,10 @@ // * We use regular sqlite, but want to ensure the credit-card numbers are // encrypted in the DB - so we store the number encrypted, and the key // is managed by the app. -// * The credit-card API always just accepts and returns the encrypted string, -// so we also expose encryption and decryption public functions that take -// the key and text. The core storage API never knows the unencrypted number. +// * The app hands us an `EncryptorDecryptor` when it builds the store, and the +// db holds on to it. Everything that reads or writes an encrypted column +// takes it from the db. The core storage API never knows the unencrypted +// number. // // This makes life tricky for Sync - sync has its own encryption and its own // management of sync keys. The entire records are encrypted on the server - @@ -23,14 +24,39 @@ // * When transforming a record from Sync into a DB record, we need to *encrypt* // the field. // -// So Sync needs to know the key etc, and that needs to get passed down -// multiple layers, from the app saying "sync now" all the way down to the -// low level sync code. -// To make life a little easier, we do that via a struct. +// The sync code takes the encryptor from the store it already holds. use crate::error::*; use error_support::handle_error; -pub use jwcrypto::EncryptorDecryptor; +use std::sync::Arc; + +pub use db_crypto::{EncryptorDecryptor, KeyManager, ManagedEncryptorDecryptor, StaticKeyManager}; + +// TODO(FXCM-2282): only `encrypt_string` and `decrypt_string` still build an +// encryptor from a key. When those go, so does this. +pub(crate) fn static_key_encryptor(key: &str) -> Result { + // Validate eagerly so an invalid key isn't treated as undecryptable card data. + jwcrypto::EncryptorDecryptor::new(key)?; + + Ok(ManagedEncryptorDecryptor::new(Arc::new( + StaticKeyManager::new(key.to_string()), + ))) +} + +#[cfg(test)] +pub(crate) fn random_key_encryptor() -> Result { + static_key_encryptor(&db_crypto::create_key()?) +} + +pub(crate) fn encrypt_str(encdec: &dyn EncryptorDecryptor, cleartext: &str) -> Result { + let ciphertext = encdec.encrypt(cleartext.as_bytes().to_vec())?; + String::from_utf8(ciphertext).map_err(|e| Error::CryptoNotUtf8(format!("encrypting: {e}"))) +} + +pub(crate) fn decrypt_str(encdec: &dyn EncryptorDecryptor, ciphertext: &str) -> Result { + let cleartext = encdec.decrypt(ciphertext.as_bytes().to_vec())?; + String::from_utf8(cleartext).map_err(|e| Error::CryptoNotUtf8(format!("decrypting: {e}"))) +} // public functions we expose over the FFI (which is why they take `String` // rather than the `&str` you'd otherwise expect) @@ -38,19 +64,19 @@ pub use jwcrypto::EncryptorDecryptor; pub fn encrypt_string(key: String, cleartext: String) -> ApiResult { // It would be nice to have more detailed error messages, but that would require the consumer // to pass them in. Let's not change the API yet. - Ok(EncryptorDecryptor::new(&key)?.encrypt(&cleartext)?) + encrypt_str(&static_key_encryptor(&key)?, &cleartext) } #[handle_error(Error)] pub fn decrypt_string(key: String, ciphertext: String) -> ApiResult { // It would be nice to have more detailed error messages, but that would require the consumer // to pass them in. Let's not change the API yet. - Ok(EncryptorDecryptor::new(&key)?.decrypt(&ciphertext)?) + decrypt_str(&static_key_encryptor(&key)?, &ciphertext) } #[handle_error(Error)] pub fn create_autofill_key() -> ApiResult { - Ok(EncryptorDecryptor::create_key()?) + Ok(db_crypto::create_key()?) } #[cfg(test)] @@ -61,28 +87,41 @@ mod test { #[test] fn test_encrypt() { ensure_initialized(); - let ed = EncryptorDecryptor::new(&create_autofill_key().unwrap()).unwrap(); + let ed = static_key_encryptor(&create_autofill_key().unwrap()).unwrap(); let cleartext = "secret"; - let ciphertext = ed.encrypt(cleartext).unwrap(); - assert_eq!(ed.decrypt(&ciphertext).unwrap(), cleartext); - let ed2 = EncryptorDecryptor::new(&create_autofill_key().unwrap()).unwrap(); + let ciphertext = encrypt_str(&ed, cleartext).unwrap(); + assert_eq!(decrypt_str(&ed, &ciphertext).unwrap(), cleartext); + let ed2 = static_key_encryptor(&create_autofill_key().unwrap()).unwrap(); assert!(matches!( - ed2.decrypt(&ciphertext).map_err(Error::from), - Err(Error::CryptoError(_)) + decrypt_str(&ed2, &ciphertext), + Err(Error::EncryptionError( + db_crypto::DbCryptoApiError::DecryptionFailed { .. } + )) )); } #[test] fn test_decryption_errors() { + // The shared crate maps all jwcrypto decryption failures to DecryptionFailed. ensure_initialized(); - let ed = EncryptorDecryptor::new(&create_autofill_key().unwrap()).unwrap(); + let ed = static_key_encryptor(&create_autofill_key().unwrap()).unwrap(); assert!(matches!( - ed.decrypt("invalid-ciphertext").map_err(Error::from), - Err(Error::CryptoError(_)), + decrypt_str(&ed, "invalid-ciphertext"), + Err(Error::EncryptionError( + db_crypto::DbCryptoApiError::DecryptionFailed { .. } + )), )); assert!(matches!( - ed.decrypt("").unwrap_err(), - jwcrypto::JwCryptoError::EmptyCyphertext, + decrypt_str(&ed, ""), + Err(Error::EncryptionError( + db_crypto::DbCryptoApiError::DecryptionFailed { .. } + )), )); } + + #[test] + fn test_an_invalid_key_is_rejected_up_front() { + ensure_initialized(); + assert!(static_key_encryptor("not-a-key").is_err()); + } } diff --git a/components/autofill/src/error.rs b/components/autofill/src/error.rs index 5eab83daba6..d38f5d8a512 100644 --- a/components/autofill/src/error.rs +++ b/components/autofill/src/error.rs @@ -71,8 +71,11 @@ pub enum Error { #[error("Crypto Error: {0}")] CryptoError(#[from] jwcrypto::JwCryptoError), - #[error("Missing local encryption key")] - MissingEncryptionKey, + #[error("Encryption Error: {0}")] + EncryptionError(#[from] db_crypto::DbCryptoApiError), + + #[error("Crypto data is not valid UTF-8: {0}")] + CryptoNotUtf8(String), #[error("No record with guid exists: {0}")] NoSuchRecord(String), @@ -131,10 +134,15 @@ impl GetErrorHandling for Error { }) .report_error("autofill-crypto-error"), - Self::MissingEncryptionKey => ErrorHandling::convert(AutofillApiError::CryptoError { - reason: "Missing encryption key".to_string(), + Self::EncryptionError(e) => ErrorHandling::convert(AutofillApiError::CryptoError { + reason: e.to_string(), + }) + .report_error("autofill-encryption-error"), + + Self::CryptoNotUtf8(reason) => ErrorHandling::convert(AutofillApiError::CryptoError { + reason: reason.clone(), }) - .report_error("autofill-missing-encryption-key"), + .report_error("autofill-crypto-not-utf8"), Self::NoSuchRecord(guid) => { ErrorHandling::convert(AutofillApiError::NoSuchRecord { guid: guid.clone() }) diff --git a/components/autofill/src/lib.rs b/components/autofill/src/lib.rs index 5eae09c76e6..c4fb3dff5cd 100644 --- a/components/autofill/src/lib.rs +++ b/components/autofill/src/lib.rs @@ -22,6 +22,7 @@ use crate::db::models::passport::*; use crate::db::store::Store; use crate::encryption::{create_autofill_key, decrypt_string, encrypt_string}; pub use crate::sync::AddressesBridgedEngine; +use db_crypto::EncryptorDecryptor; pub use error::{ApiResult, AutofillApiError, Error, Result}; uniffi::include_scaffolding!("autofill"); diff --git a/components/autofill/src/sync/address/mod.rs b/components/autofill/src/sync/address/mod.rs index 61ef0e6d701..566a4f062e2 100644 --- a/components/autofill/src/sync/address/mod.rs +++ b/components/autofill/src/sync/address/mod.rs @@ -13,6 +13,7 @@ use super::{ UnknownFields, }; use crate::db::models::address::InternalAddress; +use crate::encryption::EncryptorDecryptor; use crate::error::*; use crate::sync_merge_field_check; use incoming::IncomingAddressesImpl; @@ -41,9 +42,8 @@ pub(super) struct AddressesEngineStorageImpl {} impl SyncEngineStorageImpl for AddressesEngineStorageImpl { fn get_incoming_impl( &self, - enc_key: &Option, + _encdec: &Arc, ) -> Result>> { - assert!(enc_key.is_none()); Ok(Box::new(IncomingAddressesImpl {})) } @@ -57,9 +57,8 @@ impl SyncEngineStorageImpl for AddressesEngineStorageImpl { fn get_outgoing_impl( &self, - enc_key: &Option, + _encdec: &Arc, ) -> Result>> { - assert!(enc_key.is_none()); Ok(Box::new(OutgoingAddressesImpl {})) } } diff --git a/components/autofill/src/sync/bridge.rs b/components/autofill/src/sync/bridge.rs index 58715aa696a..631ffb39000 100644 --- a/components/autofill/src/sync/bridge.rs +++ b/components/autofill/src/sync/bridge.rs @@ -30,7 +30,9 @@ mod tests { fn test_sync_meta() { error_support::init_for_tests(); - let store = Arc::new(Store::new_shared_memory("addresses-bridge").unwrap()); + let store = Arc::new( + Store::new_shared_memory("addresses-bridge", crate::db::test::test_encdec()).unwrap(), + ); let bridge = store.addresses_bridged_engine(); bridge.sync_started().unwrap(); @@ -67,7 +69,10 @@ mod tests { fn test_sync_via_bridge() { error_support::init_for_tests(); - let store = Arc::new(Store::new_shared_memory("addresses-bridge-roundtrip").unwrap()); + let store = Arc::new( + Store::new_shared_memory("addresses-bridge-roundtrip", crate::db::test::test_encdec()) + .unwrap(), + ); // A local-only address: nothing on the server knows about it yet, so it // should be uploaded. diff --git a/components/autofill/src/sync/credit_card/incoming.rs b/components/autofill/src/sync/credit_card/incoming.rs index f73feb3298c..76f38adad73 100644 --- a/components/autofill/src/sync/credit_card/incoming.rs +++ b/components/autofill/src/sync/credit_card/incoming.rs @@ -7,7 +7,7 @@ use super::CreditCardPayload; use crate::db::credit_cards::{add_internal_credit_card, update_internal_credit_card}; use crate::db::models::credit_card::InternalCreditCard; use crate::db::schema::CREDIT_CARD_COMMON_COLS; -use crate::encryption::EncryptorDecryptor; +use crate::encryption::{decrypt_str, encrypt_str, EncryptorDecryptor}; use crate::error::*; use crate::sync::common::*; use crate::sync::{ @@ -17,6 +17,7 @@ use crate::sync::{ use interrupt_support::Interruptee; use rusqlite::{named_params, Transaction}; use sql_support::ConnExt; +use std::sync::Arc; use sync_guid::Guid as SyncGuid; // Takes a raw payload, as stored in our database, and returns an InternalCreditCard @@ -25,9 +26,9 @@ use sync_guid::Guid as SyncGuid; fn raw_payload_to_incoming( id: SyncGuid, raw: String, - encdec: &EncryptorDecryptor, + encdec: &dyn EncryptorDecryptor, ) -> Result> { - let payload = encdec.decrypt(&raw)?; + let payload = decrypt_str(encdec, &raw)?; // Turn it into a BSO let bso = IncomingBso { envelope: IncomingEnvelope { @@ -58,7 +59,7 @@ fn raw_payload_to_incoming( } pub(super) struct IncomingCreditCardsImpl { - pub(super) encdec: EncryptorDecryptor, + pub(super) encdec: Arc, } impl ProcessIncomingRecordImpl for IncomingCreditCardsImpl { @@ -76,7 +77,7 @@ impl ProcessIncomingRecordImpl for IncomingCreditCardsImpl { .into_iter() .map(|bso| { // consider turning this into malformed? - let encrypted = self.encdec.encrypt(&bso.payload)?; + let encrypted = encrypt_str(self.encdec.as_ref(), &bso.payload)?; Ok((bso.envelope.id, encrypted, bso.envelope.modified)) }) .collect::>()?; @@ -121,7 +122,7 @@ impl ProcessIncomingRecordImpl for IncomingCreditCardsImpl { // the 'guid' and 's_payload' rows must be non-null. let guid: SyncGuid = row.get("guid")?; let incoming = - raw_payload_to_incoming(guid.clone(), row.get("s_payload")?, &self.encdec)?; + raw_payload_to_incoming(guid.clone(), row.get("s_payload")?, self.encdec.as_ref())?; Ok(IncomingState { incoming, local: match row.get_unwrap::<_, Option>("l_guid") { @@ -155,7 +156,8 @@ impl ProcessIncomingRecordImpl for IncomingCreditCardsImpl { match row.get::<_, Option>("m_payload")? { Some(m_payload) => { // a tombstone in the mirror can be treated as though it's missing. - raw_payload_to_incoming(guid, m_payload, &self.encdec)?.content() + raw_payload_to_incoming(guid, m_payload, self.encdec.as_ref())? + .content() } None => None, } @@ -208,9 +210,9 @@ impl ProcessIncomingRecordImpl for IncomingCreditCardsImpl { Ok(Self::Record::from_row(row)?) })?; - let incoming_cc_number = self.encdec.decrypt(&incoming.cc_number_enc)?; + let incoming_cc_number = decrypt_str(self.encdec.as_ref(), &incoming.cc_number_enc)?; for record in records { - if self.encdec.decrypt(&record.cc_number_enc)? == incoming_cc_number { + if decrypt_str(self.encdec.as_ref(), &record.cc_number_enc)? == incoming_cc_number { return Ok(Some(record)); } } @@ -264,6 +266,7 @@ mod tests { use super::super::super::test::new_syncable_mem_db; use super::*; use crate::db::credit_cards::get_credit_card; + use crate::encryption::random_key_encryptor; use crate::sync::common::tests::*; use error_support::{info, trace}; @@ -332,7 +335,7 @@ mod tests { .clone() } - fn test_record(guid_prefix: char, encdec: &EncryptorDecryptor) -> InternalCreditCard { + fn test_record(guid_prefix: char, encdec: &dyn EncryptorDecryptor) -> InternalCreditCard { let json = test_json_record(guid_prefix); let payload = serde_json::from_value(json).unwrap(); InternalCreditCard::from_payload(payload, encdec).expect("should be valid") @@ -385,7 +388,7 @@ mod tests { for tc in test_cases { info!("starting new testcase"); let tx = db.transaction().unwrap(); - let encdec = EncryptorDecryptor::new_with_random_key().unwrap(); + let encdec: Arc = Arc::new(random_key_encryptor().unwrap()); // Add required items to the mirrors. let mirror_sql = "INSERT OR REPLACE INTO credit_cards_mirror (guid, payload) @@ -395,7 +398,7 @@ mod tests { mirror_sql, rusqlite::named_params! { ":guid": payload["id"].as_str().unwrap(), - ":payload": encdec.encrypt(&payload.to_string())?, + ":payload": encrypt_str(encdec.as_ref(), &payload.to_string())?, }, ) .expect("should insert mirror record"); @@ -414,7 +417,7 @@ mod tests { |row| -> Result> { let guid: SyncGuid = row.get_unwrap("guid"); let enc_payload: String = row.get_unwrap("payload"); - raw_payload_to_incoming(guid, enc_payload, &ri.encdec) + raw_payload_to_incoming(guid, enc_payload, ri.encdec.as_ref()) }, )?; @@ -441,10 +444,10 @@ mod tests { let mut db = new_syncable_mem_db(); let tx = db.transaction()?; let ri = IncomingCreditCardsImpl { - encdec: EncryptorDecryptor::new_with_random_key().unwrap(), + encdec: Arc::new(random_key_encryptor().unwrap()), }; - ri.insert_local_record(&tx, test_record('C', &ri.encdec))?; + ri.insert_local_record(&tx, test_record('C', ri.encdec.as_ref()))?; ri.change_record_guid( &tx, @@ -463,12 +466,12 @@ mod tests { let mut db = new_syncable_mem_db(); let tx = db.transaction().expect("should get tx"); let ci = IncomingCreditCardsImpl { - encdec: EncryptorDecryptor::new_with_random_key().unwrap(), + encdec: Arc::new(random_key_encryptor().unwrap()), }; - let record = test_record('C', &ci.encdec); + let record = test_record('C', ci.encdec.as_ref()); let bso = record .clone() - .into_test_incoming_bso(&ci.encdec, Default::default()); + .into_test_incoming_bso(ci.encdec.as_ref(), Default::default()); do_test_incoming_same(&ci, &tx, record, bso); } @@ -478,9 +481,9 @@ mod tests { let mut db = new_syncable_mem_db(); let tx = db.transaction().expect("should get tx"); let ci = IncomingCreditCardsImpl { - encdec: EncryptorDecryptor::new_with_random_key().unwrap(), + encdec: Arc::new(random_key_encryptor().unwrap()), }; - do_test_incoming_tombstone(&ci, &tx, test_record('C', &ci.encdec)); + do_test_incoming_tombstone(&ci, &tx, test_record('C', ci.encdec.as_ref())); } #[test] @@ -489,12 +492,12 @@ mod tests { let mut db = new_syncable_mem_db(); let tx = db.transaction().expect("should get tx"); let ci = IncomingCreditCardsImpl { - encdec: EncryptorDecryptor::new_with_random_key().unwrap(), + encdec: Arc::new(random_key_encryptor().unwrap()), }; - let mut scrubbed_record = test_record('A', &ci.encdec); + let mut scrubbed_record = test_record('A', ci.encdec.as_ref()); let bso = scrubbed_record .clone() - .into_test_incoming_bso(&ci.encdec, Default::default()); + .into_test_incoming_bso(ci.encdec.as_ref(), Default::default()); scrubbed_record.cc_number_enc = "".to_string(); do_test_scrubbed_local_data(&ci, &tx, scrubbed_record, bso); } @@ -505,12 +508,12 @@ mod tests { let mut db = new_syncable_mem_db(); let tx = db.transaction().expect("should get tx"); let ci = IncomingCreditCardsImpl { - encdec: EncryptorDecryptor::new_with_random_key().unwrap(), + encdec: Arc::new(random_key_encryptor().unwrap()), }; - let record = test_record('C', &ci.encdec); + let record = test_record('C', ci.encdec.as_ref()); let bso = record .clone() - .into_test_incoming_bso(&ci.encdec, Default::default()); + .into_test_incoming_bso(ci.encdec.as_ref(), Default::default()); do_test_staged_to_mirror(&ci, &tx, record, bso, "credit_cards_mirror"); } @@ -519,15 +522,16 @@ mod tests { ensure_initialized(); let mut db = new_syncable_mem_db(); let tx = db.transaction().expect("should get tx"); - let encdec = EncryptorDecryptor::new_with_random_key().unwrap(); - let ci = IncomingCreditCardsImpl { encdec }; - let local_record = test_record('C', &ci.encdec); + let ci = IncomingCreditCardsImpl { + encdec: Arc::new(random_key_encryptor().unwrap()), + }; + let local_record = test_record('C', ci.encdec.as_ref()); let local_guid = local_record.guid.clone(); ci.insert_local_record(&tx, local_record.clone()).unwrap(); // Now the same record incoming - it should find the one we just added // above as a dupe. - let mut incoming_record = test_record('C', &ci.encdec); + let mut incoming_record = test_record('C', ci.encdec.as_ref()); // sanity check that the encrypted numbers are different even though // the decrypted numbers are identical. assert_ne!(local_record.cc_number_enc, incoming_record.cc_number_enc); @@ -555,9 +559,10 @@ mod tests { ensure_initialized(); let mut db = new_syncable_mem_db(); let tx = db.transaction().expect("should get tx"); - let encdec = EncryptorDecryptor::new_with_random_key().unwrap(); - let ci = IncomingCreditCardsImpl { encdec }; - let local_record = test_record('C', &ci.encdec); + let ci = IncomingCreditCardsImpl { + encdec: Arc::new(random_key_encryptor().unwrap()), + }; + let local_record = test_record('C', ci.encdec.as_ref()); let local_guid = local_record.guid.clone(); ci.insert_local_record(&tx, local_record.clone()).unwrap(); diff --git a/components/autofill/src/sync/credit_card/mod.rs b/components/autofill/src/sync/credit_card/mod.rs index 213f8bf158c..27c96a63d09 100644 --- a/components/autofill/src/sync/credit_card/mod.rs +++ b/components/autofill/src/sync/credit_card/mod.rs @@ -12,7 +12,9 @@ use super::{ UnknownFields, }; use crate::db::models::credit_card::InternalCreditCard; -use crate::encryption::EncryptorDecryptor; +#[cfg(test)] +use crate::encryption::static_key_encryptor; +use crate::encryption::{decrypt_str, encrypt_str, EncryptorDecryptor}; use crate::error::*; use crate::sync_merge_field_check; use incoming::IncomingCreditCardsImpl; @@ -40,14 +42,11 @@ pub(super) struct CreditCardsEngineStorageImpl {} impl SyncEngineStorageImpl for CreditCardsEngineStorageImpl { fn get_incoming_impl( &self, - enc_key: &Option, + encdec: &Arc, ) -> Result>> { - let enc_key = match enc_key { - None => return Err(Error::MissingEncryptionKey), - Some(enc_key) => enc_key, - }; - let encdec = EncryptorDecryptor::new(enc_key)?; - Ok(Box::new(IncomingCreditCardsImpl { encdec })) + Ok(Box::new(IncomingCreditCardsImpl { + encdec: Arc::clone(encdec), + })) } fn reset_storage(&self, tx: &Transaction<'_>) -> Result<()> { @@ -60,14 +59,11 @@ impl SyncEngineStorageImpl for CreditCardsEngineStorageImpl fn get_outgoing_impl( &self, - enc_key: &Option, + encdec: &Arc, ) -> Result>> { - let enc_key = match enc_key { - None => return Err(Error::MissingEncryptionKey), - Some(enc_key) => enc_key, - }; - let encdec = EncryptorDecryptor::new(enc_key)?; - Ok(Box::new(OutgoingCreditCardsImpl { encdec })) + Ok(Box::new(OutgoingCreditCardsImpl { + encdec: Arc::clone(encdec), + })) } } @@ -113,7 +109,7 @@ pub(super) struct PayloadEntry { } impl InternalCreditCard { - fn from_payload(p: CreditCardPayload, encdec: &EncryptorDecryptor) -> Result { + fn from_payload(p: CreditCardPayload, encdec: &dyn EncryptorDecryptor) -> Result { if p.entry.version != 3 { // when new versions are introduced we will start accepting and // converting old ones - but 3 is the lowest we support. @@ -123,7 +119,7 @@ impl InternalCreditCard { ))); } // need to encrypt the cleartext in the sync record. - let cc_number_enc = encdec.encrypt(&p.entry.cc_number)?; + let cc_number_enc = encrypt_str(encdec, &p.entry.cc_number)?; let cc_number_last_4 = get_last_4(&p.entry.cc_number); Ok(InternalCreditCard { @@ -144,8 +140,8 @@ impl InternalCreditCard { }) } - pub(crate) fn into_payload(self, encdec: &EncryptorDecryptor) -> Result { - let cc_number = encdec.decrypt(&self.cc_number_enc)?; + pub(crate) fn into_payload(self, encdec: &dyn EncryptorDecryptor) -> Result { + let cc_number = decrypt_str(encdec, &self.cc_number_enc)?; Ok(CreditCardPayload { id: self.guid, entry: PayloadEntry { @@ -269,7 +265,7 @@ fn test_to_from_payload() { cc_type: "foo".to_string(), ..Default::default() }; - let encdec = EncryptorDecryptor::new(&key).unwrap(); + let encdec = static_key_encryptor(&key).unwrap(); let payload: CreditCardPayload = cc.clone().into_payload(&encdec).unwrap(); assert_eq!(payload.id, cc.guid); diff --git a/components/autofill/src/sync/credit_card/outgoing.rs b/components/autofill/src/sync/credit_card/outgoing.rs index cb315a729ba..9bda3a4f63c 100644 --- a/components/autofill/src/sync/credit_card/outgoing.rs +++ b/components/autofill/src/sync/credit_card/outgoing.rs @@ -5,11 +5,12 @@ use crate::db::models::credit_card::InternalCreditCard; use crate::db::schema::CREDIT_CARD_COMMON_COLS; -use crate::encryption::EncryptorDecryptor; +use crate::encryption::{decrypt_str, encrypt_str, EncryptorDecryptor}; use crate::error::*; use crate::sync::common::*; use crate::sync::{credit_card::CreditCardPayload, OutgoingBso, ProcessOutgoingRecordImpl}; use rusqlite::{Row, Transaction}; +use std::sync::Arc; use sync_guid::Guid as SyncGuid; const DATA_TABLE_NAME: &str = "credit_cards_data"; @@ -17,7 +18,7 @@ const MIRROR_TABLE_NAME: &str = "credit_cards_mirror"; const STAGING_TABLE_NAME: &str = "credit_cards_sync_outgoing_staging"; pub(super) struct OutgoingCreditCardsImpl { - pub(super) encdec: EncryptorDecryptor, + pub(super) encdec: Arc, } impl ProcessOutgoingRecordImpl for OutgoingCreditCardsImpl { @@ -47,12 +48,13 @@ impl ProcessOutgoingRecordImpl for OutgoingCreditCardsImpl { common_cols = CREDIT_CARD_COMMON_COLS, ); let record_from_data_row: &dyn Fn(&Row<'_>) -> Result<(OutgoingBso, i64)> = &|row| { - let mut record = InternalCreditCard::from_row(row)?.into_payload(&self.encdec)?; + let mut record = + InternalCreditCard::from_row(row)?.into_payload(self.encdec.as_ref())?; // If the server had unknown fields we fetch it and add it to the record if let Some(enc_s) = row.get::<_, Option>("payload")? { // The full payload in the credit cards mirror is encrypted let mirror_payload: CreditCardPayload = - serde_json::from_str(&self.encdec.decrypt(&enc_s)?)?; + serde_json::from_str(&decrypt_str(self.encdec.as_ref(), &enc_s)?)?; record.entry.unknown_fields = mirror_payload.entry.unknown_fields; }; @@ -74,7 +76,7 @@ impl ProcessOutgoingRecordImpl for OutgoingCreditCardsImpl { .into_iter() .map(|(bso, change_counter)| { // Turn the record into an encrypted repr to save in the mirror. - let encrypted = self.encdec.encrypt(&bso.payload)?; + let encrypted = encrypt_str(self.encdec.as_ref(), &bso.payload)?; Ok((bso.envelope.id, encrypted, change_counter)) }) .collect::>()?; @@ -110,6 +112,7 @@ impl ProcessOutgoingRecordImpl for OutgoingCreditCardsImpl { mod tests { use super::*; use crate::db::credit_cards::{add_internal_credit_card, tests::test_insert_mirror_record}; + use crate::encryption::random_key_encryptor; use crate::sync::{common::tests::*, test::new_syncable_mem_db, UnknownFields}; use serde_json::{json, Map, Value}; use types::Timestamp; @@ -164,7 +167,7 @@ mod tests { .clone() } - fn test_record(guid_prefix: char, encdec: &EncryptorDecryptor) -> InternalCreditCard { + fn test_record(guid_prefix: char, encdec: &dyn EncryptorDecryptor) -> InternalCreditCard { let json = test_json_record(guid_prefix); let payload = serde_json::from_value(json).unwrap(); InternalCreditCard::from_payload(payload, encdec).expect("should be valid") @@ -175,9 +178,9 @@ mod tests { let mut db = new_syncable_mem_db(); let tx = db.transaction().expect("should get tx"); let co = OutgoingCreditCardsImpl { - encdec: EncryptorDecryptor::new_with_random_key().unwrap(), + encdec: Arc::new(random_key_encryptor().unwrap()), }; - let test_record = test_record('C', &co.encdec); + let test_record = test_record('C', co.encdec.as_ref()); // create date record assert!(add_internal_credit_card(&tx, &test_record).is_ok()); @@ -196,9 +199,9 @@ mod tests { let mut db = new_syncable_mem_db(); let tx = db.transaction().expect("should get tx"); let co = OutgoingCreditCardsImpl { - encdec: EncryptorDecryptor::new_with_random_key().unwrap(), + encdec: Arc::new(random_key_encryptor().unwrap()), }; - let test_record = test_record('C', &co.encdec); + let test_record = test_record('C', co.encdec.as_ref()); // create tombstone record assert!(tx @@ -231,19 +234,19 @@ mod tests { let mut db = new_syncable_mem_db(); let tx = db.transaction().expect("should get tx"); let co = OutgoingCreditCardsImpl { - encdec: EncryptorDecryptor::new_with_random_key().unwrap(), + encdec: Arc::new(random_key_encryptor().unwrap()), }; // create synced record with non-zero sync_change_counter - let mut test_record = test_record('C', &co.encdec); + let mut test_record = test_record('C', co.encdec.as_ref()); let initial_change_counter_val = 2; test_record.metadata.sync_change_counter = initial_change_counter_val; assert!(add_internal_credit_card(&tx, &test_record).is_ok()); let guid = test_record.guid.clone(); //test_insert_mirror_record doesn't encrypt the mirror payload, but in reality we do // so we encrypt here so our fetch_outgoing_records doesn't break - let mut bso = test_record.into_test_incoming_bso(&co.encdec, Default::default()); - bso.payload = co.encdec.encrypt(&bso.payload).unwrap(); + let mut bso = test_record.into_test_incoming_bso(co.encdec.as_ref(), Default::default()); + bso.payload = encrypt_str(co.encdec.as_ref(), &bso.payload).unwrap(); test_insert_mirror_record(&tx, bso); exists_with_counter_value_in_table(&tx, DATA_TABLE_NAME, &guid, initial_change_counter_val); @@ -262,16 +265,16 @@ mod tests { let mut db = new_syncable_mem_db(); let tx = db.transaction().expect("should get tx"); let co = OutgoingCreditCardsImpl { - encdec: EncryptorDecryptor::new_with_random_key().unwrap(), + encdec: Arc::new(random_key_encryptor().unwrap()), }; // create synced record with no changes (sync_change_counter = 0) - let test_record = test_record('C', &co.encdec); + let test_record = test_record('C', co.encdec.as_ref()); let guid = test_record.guid.clone(); assert!(add_internal_credit_card(&tx, &test_record).is_ok()); test_insert_mirror_record( &tx, - test_record.into_test_incoming_bso(&co.encdec, Default::default()), + test_record.into_test_incoming_bso(co.encdec.as_ref(), Default::default()), ); do_test_outgoing_synced_with_no_change( @@ -288,11 +291,11 @@ mod tests { let mut db = new_syncable_mem_db(); let tx = db.transaction().expect("should get tx"); let co = OutgoingCreditCardsImpl { - encdec: EncryptorDecryptor::new_with_random_key().unwrap(), + encdec: Arc::new(random_key_encryptor().unwrap()), }; // create synced record with non-zero sync_change_counter - let mut test_record = test_record('D', &co.encdec); + let mut test_record = test_record('D', co.encdec.as_ref()); let initial_change_counter_val = 2; test_record.metadata.sync_change_counter = initial_change_counter_val; assert!(add_internal_credit_card(&tx, &test_record).is_ok()); @@ -304,8 +307,8 @@ mod tests { // so we encrypt here so our fetch_outgoing_records doesn't break let mut bso = test_record .clone() - .into_test_incoming_bso(&co.encdec, unknown_fields); - bso.payload = co.encdec.encrypt(&bso.payload).unwrap(); + .into_test_incoming_bso(co.encdec.as_ref(), unknown_fields); + bso.payload = encrypt_str(co.encdec.as_ref(), &bso.payload).unwrap(); test_insert_mirror_record(&tx, bso); exists_with_counter_value_in_table( &tx, diff --git a/components/autofill/src/sync/engine.rs b/components/autofill/src/sync/engine.rs index 204d0e7eedd..47cfa645165 100644 --- a/components/autofill/src/sync/engine.rs +++ b/components/autofill/src/sync/engine.rs @@ -3,6 +3,7 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ use super::{plan_incoming, ProcessIncomingRecordImpl, ProcessOutgoingRecordImpl, SyncRecord}; +use crate::encryption::EncryptorDecryptor; use crate::error::*; use crate::Store; use error_support::warn; @@ -33,12 +34,12 @@ pub const COLLECTION_SYNCID_META_KEY: &str = "sync_id"; pub trait SyncEngineStorageImpl: Send + Sync { fn get_incoming_impl( &self, - enc_key: &Option, + encdec: &Arc, ) -> Result>>; fn reset_storage(&self, conn: &Transaction<'_>) -> Result<()>; fn get_outgoing_impl( &self, - enc_key: &Option, + encdec: &Arc, ) -> Result>>; } @@ -47,7 +48,6 @@ pub struct ConfigSyncEngine { pub(crate) config: EngineConfig, pub(crate) store: Arc, pub(crate) storage_impl: Box>, - local_enc_key: Option, } impl ConfigSyncEngine { @@ -60,7 +60,6 @@ impl ConfigSyncEngine { config, store, storage_impl, - local_enc_key: None, } } fn put_meta(&self, conn: &Connection, tail: &str, value: &dyn ToSql) -> Result<()> { @@ -102,11 +101,6 @@ impl SyncEngine for ConfigSyncEngine { self.config.collection.clone() } - fn set_local_encryption_key(&mut self, key: &str) -> anyhow::Result<()> { - self.local_enc_key = Some(key.to_string()); - Ok(()) - } - fn sync_started(&self) -> anyhow::Result<()> { let db = self.store.lock_db()?; let signal = db.begin_interrupt_scope()?; @@ -128,7 +122,7 @@ impl SyncEngine for ConfigSyncEngine { incoming_telemetry.applied(inbound.len() as u32); telem.incoming(incoming_telemetry); let tx = db.writer.unchecked_transaction()?; - let incoming_impl = self.storage_impl.get_incoming_impl(&self.local_enc_key)?; + let incoming_impl = self.storage_impl.get_incoming_impl(&db.encdec)?; incoming_impl.stage_incoming(&tx, inbound, &signal)?; tx.commit()?; @@ -143,8 +137,8 @@ impl SyncEngine for ConfigSyncEngine { let db = self.store.lock_db()?; let signal = db.begin_interrupt_scope()?; let tx = db.writer.unchecked_transaction()?; - let incoming_impl = self.storage_impl.get_incoming_impl(&self.local_enc_key)?; - let outgoing_impl = self.storage_impl.get_outgoing_impl(&self.local_enc_key)?; + let incoming_impl = self.storage_impl.get_incoming_impl(&db.encdec)?; + let outgoing_impl = self.storage_impl.get_outgoing_impl(&db.encdec)?; // Get "states" for each record... for state in incoming_impl.fetch_incoming_states(&tx)? { @@ -178,7 +172,7 @@ impl SyncEngine for ConfigSyncEngine { let db = self.store.lock_db()?; self.put_meta(&db.writer, LAST_SYNC_META_KEY, &new_timestamp.as_millis())?; let tx = db.writer.unchecked_transaction()?; - let outgoing_impl = self.storage_impl.get_outgoing_impl(&self.local_enc_key)?; + let outgoing_impl = self.storage_impl.get_outgoing_impl(&db.encdec)?; outgoing_impl.finish_synced_items(&tx, ids)?; tx.commit()?; Ok(()) @@ -268,7 +262,7 @@ mod tests { }; use crate::db::models::credit_card::InternalCreditCard; use crate::db::schema::create_empty_sync_temp_tables; - use crate::encryption::EncryptorDecryptor; + use crate::encryption::{encrypt_str, random_key_encryptor, EncryptorDecryptor}; use crate::sync::{IncomingBso, UnknownFields}; use nss_as::ensure_initialized; use sql_support::ConnExt; @@ -276,7 +270,7 @@ mod tests { impl InternalCreditCard { pub fn into_test_incoming_bso( self, - encdec: &EncryptorDecryptor, + encdec: &dyn EncryptorDecryptor, unknown_fields: UnknownFields, ) -> IncomingBso { let mut payload = self.into_payload(encdec).expect("is json"); @@ -303,11 +297,7 @@ mod tests { #[test] fn test_credit_card_engine_apply_timestamp() -> Result<()> { ensure_initialized(); - let mut credit_card_engine = create_engine(); - let test_key = crate::encryption::create_autofill_key().unwrap(); - credit_card_engine - .set_local_encryption_key(&test_key) - .unwrap(); + let credit_card_engine = create_engine(); { let db = credit_card_engine.store.lock_db()?; create_empty_sync_temp_tables(&db.writer)?; @@ -367,12 +357,12 @@ mod tests { fn test_engine_sync_reset() -> Result<()> { ensure_initialized(); let engine = create_engine(); - let encdec = EncryptorDecryptor::new_with_random_key().unwrap(); + let encdec = random_key_encryptor().unwrap(); let cc = InternalCreditCard { guid: Guid::random(), cc_name: "Ms Jane Doe".to_string(), - cc_number_enc: encdec.encrypt("12341232412341234")?, + cc_number_enc: encrypt_str(&encdec, "12341232412341234")?, cc_number_last_4: "1234".to_string(), cc_exp_month: 12, cc_exp_year: 2021, From 82ed8ea24c5a1d59e7cb511340c82227914cb0b5 Mon Sep 17 00:00:00 2001 From: theidkamp Date: Wed, 2 Sep 2026 15:58:39 +0200 Subject: [PATCH 2/3] FXCM-2279: Add SecureCreditCardFields as the one place for card crypto Move encrypting and decrypting a card number behind one type, so the knowledge of what the stored value looks like lives in a single place instead of being spread across the db and sync code. A struct rather than a bare string is deliberate: a CVV is expected as a second encrypted field, which needs a structured encoding and a rewrite of existing rows (FXCM-2280). No stored data changes here. --- components/autofill/src/db/credit_cards.rs | 12 +- .../autofill/src/db/models/credit_card.rs | 106 ++++++++++++++++++ components/autofill/src/error.rs | 31 +++++ .../autofill/src/sync/credit_card/incoming.rs | 18 ++- .../autofill/src/sync/credit_card/mod.rs | 13 ++- 5 files changed, 170 insertions(+), 10 deletions(-) diff --git a/components/autofill/src/db/credit_cards.rs b/components/autofill/src/db/credit_cards.rs index ab3902bd5a4..47def3cb5f4 100644 --- a/components/autofill/src/db/credit_cards.rs +++ b/components/autofill/src/db/credit_cards.rs @@ -5,13 +5,12 @@ use crate::db::{ models::{ - credit_card::{InternalCreditCard, UpdatableCreditCardFields}, + credit_card::{InternalCreditCard, SecureCreditCardFields, UpdatableCreditCardFields}, Metadata, }, schema::{CREDIT_CARD_COMMON_COLS, CREDIT_CARD_COMMON_VALS}, AutofillDb, }; -use crate::encryption::decrypt_str; use crate::error::*; use rusqlite::{Connection, Transaction}; @@ -235,7 +234,14 @@ pub fn scrub_undecryptable_credit_card_data_for_remote_replacement( let undecryptable_record_ids = get_all_credit_cards(conn)? .into_iter() - .filter(|credit_card| decrypt_str(db.encdec.as_ref(), &credit_card.cc_number_enc).is_err()) + .filter(|credit_card| { + SecureCreditCardFields::decrypt( + &credit_card.cc_number_enc, + db.encdec.as_ref(), + credit_card.guid.as_str(), + ) + .is_err() + }) .map(|credit_card| credit_card.guid) .collect::>(); diff --git a/components/autofill/src/db/models/credit_card.rs b/components/autofill/src/db/models/credit_card.rs index a4149c6e52f..107c383c590 100644 --- a/components/autofill/src/db/models/credit_card.rs +++ b/components/autofill/src/db/models/credit_card.rs @@ -3,7 +3,19 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ +//! Credit-card models and the cleartext fields that are stored encrypted. +//! +//! Only the number is encrypted today, and the encrypted value is that number +//! verbatim - nothing about the stored format changes here. +//! +//! `SecureCreditCardFields` is a struct because a CVV is expected as a second +//! encrypted field. Two values need a structured encoding, and existing rows +//! then have to be rewritten: they decrypt to a bare number, where the new code +//! would expect a structure. That migration is a separate ticket. + use super::Metadata; +use crate::encryption::{decrypt_str, encrypt_str, EncryptorDecryptor}; +use crate::error::Error; use rusqlite::Row; use sync_guid::Guid; @@ -104,3 +116,97 @@ impl InternalCreditCard { self.cc_number_enc.is_empty() } } + +/// Cleartext credit-card fields that are encrypted for local storage. +#[derive(Debug, Clone, Hash, PartialEq, Eq, Default)] +pub struct SecureCreditCardFields { + pub cc_number: String, +} + +impl SecureCreditCardFields { + /// `guid` only identifies the record in error messages. + pub fn encrypt( + &self, + encdec: &dyn EncryptorDecryptor, + guid: &str, + ) -> crate::error::Result { + encrypt_str(encdec, &self.cc_number) + .map_err(|e| Error::EncryptionFailed(format!("{e} (encrypting {guid})"))) + } + + pub fn decrypt( + ciphertext: &str, + encdec: &dyn EncryptorDecryptor, + guid: &str, + ) -> crate::error::Result { + let cc_number = decrypt_str(encdec, ciphertext).map_err(|e| { + Error::DecryptionFailed(format!( + "{e} (decrypting {guid}, ciphertext length: {})", + ciphertext.len() + )) + })?; + Ok(Self { cc_number }) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::encryption::{random_key_encryptor, ManagedEncryptorDecryptor}; + use nss_as::ensure_initialized; + + fn encdec() -> ManagedEncryptorDecryptor { + ensure_initialized(); + random_key_encryptor().unwrap() + } + + #[test] + fn test_roundtrip() { + let encdec = encdec(); + let stored = SecureCreditCardFields { + cc_number: "4111111111117629".to_string(), + } + .encrypt(&encdec, "test-guid") + .unwrap(); + + assert!(!stored.is_empty()); + assert_ne!( + stored, "4111111111117629", + "the stored value must not be the cleartext" + ); + assert_eq!( + SecureCreditCardFields::decrypt(&stored, &encdec, "test-guid") + .unwrap() + .cc_number, + "4111111111117629" + ); + } + + #[test] + fn test_decrypt_with_the_wrong_key_fails() { + let stored = SecureCreditCardFields { + cc_number: "4111111111117629".to_string(), + } + .encrypt(&encdec(), "test-guid") + .unwrap(); + assert!(SecureCreditCardFields::decrypt(&stored, &encdec(), "test-guid").is_err()); + } + + #[test] + fn test_scrubbed_is_the_default() { + // Empty ciphertext marks data to be replaced from Sync. + assert!(InternalCreditCard::default().has_scrubbed_data()); + } + + #[test] + fn test_encrypting_twice_gives_different_ciphertext() { + let encdec = encdec(); + let fields = SecureCreditCardFields { + cc_number: "4111111111117629".to_string(), + }; + assert_ne!( + fields.encrypt(&encdec, "test-guid").unwrap(), + fields.encrypt(&encdec, "test-guid").unwrap() + ); + } +} diff --git a/components/autofill/src/error.rs b/components/autofill/src/error.rs index d38f5d8a512..a9804a331d5 100644 --- a/components/autofill/src/error.rs +++ b/components/autofill/src/error.rs @@ -77,6 +77,16 @@ pub enum Error { #[error("Crypto data is not valid UTF-8: {0}")] CryptoNotUtf8(String), + // Encrypting or decrypting a record's secure fields failed. The string + // carries the underlying error plus the record guid, never the data itself + // - see the PII warning below. Mirrors logins, which reports the login id + // the same way (components/logins/src/error.rs:81). + #[error("Encryption failed: {0}")] + EncryptionFailed(String), + + #[error("Decryption failed: {0}")] + DecryptionFailed(String), + #[error("No record with guid exists: {0}")] NoSuchRecord(String), @@ -144,6 +154,27 @@ impl GetErrorHandling for Error { }) .report_error("autofill-crypto-not-utf8"), + // Logged locally, deliberately NOT reported: the message carries the + // record guid so an operator can tell which record failed, and a + // guid is stable and also exists on the sync server, so shipping it + // off-device would make a report linkable to one user's record. + // This matches how autofill already treats `NoSuchRecord`. logins + // does report the equivalent, via its catch-all arm - we are being + // stricter on purpose, because these are card numbers. + Self::EncryptionFailed(reason) => { + ErrorHandling::convert(AutofillApiError::CryptoError { + reason: reason.clone(), + }) + .log_warning() + } + + Self::DecryptionFailed(reason) => { + ErrorHandling::convert(AutofillApiError::CryptoError { + reason: reason.clone(), + }) + .log_warning() + } + Self::NoSuchRecord(guid) => { ErrorHandling::convert(AutofillApiError::NoSuchRecord { guid: guid.clone() }) .log_warning() diff --git a/components/autofill/src/sync/credit_card/incoming.rs b/components/autofill/src/sync/credit_card/incoming.rs index 76f38adad73..d777d55d915 100644 --- a/components/autofill/src/sync/credit_card/incoming.rs +++ b/components/autofill/src/sync/credit_card/incoming.rs @@ -5,7 +5,7 @@ use super::CreditCardPayload; use crate::db::credit_cards::{add_internal_credit_card, update_internal_credit_card}; -use crate::db::models::credit_card::InternalCreditCard; +use crate::db::models::credit_card::{InternalCreditCard, SecureCreditCardFields}; use crate::db::schema::CREDIT_CARD_COMMON_COLS; use crate::encryption::{decrypt_str, encrypt_str, EncryptorDecryptor}; use crate::error::*; @@ -210,9 +210,21 @@ impl ProcessIncomingRecordImpl for IncomingCreditCardsImpl { Ok(Self::Record::from_row(row)?) })?; - let incoming_cc_number = decrypt_str(self.encdec.as_ref(), &incoming.cc_number_enc)?; + let incoming_cc_number = SecureCreditCardFields::decrypt( + &incoming.cc_number_enc, + self.encdec.as_ref(), + incoming.guid.as_str(), + )? + .cc_number; for record in records { - if decrypt_str(self.encdec.as_ref(), &record.cc_number_enc)? == incoming_cc_number { + if SecureCreditCardFields::decrypt( + &record.cc_number_enc, + self.encdec.as_ref(), + record.guid.as_str(), + )? + .cc_number + == incoming_cc_number + { return Ok(Some(record)); } } diff --git a/components/autofill/src/sync/credit_card/mod.rs b/components/autofill/src/sync/credit_card/mod.rs index 27c96a63d09..a38149ce9d8 100644 --- a/components/autofill/src/sync/credit_card/mod.rs +++ b/components/autofill/src/sync/credit_card/mod.rs @@ -11,10 +11,10 @@ use super::{ MergeResult, Metadata, ProcessIncomingRecordImpl, ProcessOutgoingRecordImpl, SyncRecord, UnknownFields, }; -use crate::db::models::credit_card::InternalCreditCard; +use crate::db::models::credit_card::{InternalCreditCard, SecureCreditCardFields}; #[cfg(test)] use crate::encryption::static_key_encryptor; -use crate::encryption::{decrypt_str, encrypt_str, EncryptorDecryptor}; +use crate::encryption::EncryptorDecryptor; use crate::error::*; use crate::sync_merge_field_check; use incoming::IncomingCreditCardsImpl; @@ -119,8 +119,11 @@ impl InternalCreditCard { ))); } // need to encrypt the cleartext in the sync record. - let cc_number_enc = encrypt_str(encdec, &p.entry.cc_number)?; let cc_number_last_4 = get_last_4(&p.entry.cc_number); + let cc_number_enc = SecureCreditCardFields { + cc_number: p.entry.cc_number, + } + .encrypt(encdec, p.id.as_str())?; Ok(InternalCreditCard { guid: p.id, @@ -141,7 +144,9 @@ impl InternalCreditCard { } pub(crate) fn into_payload(self, encdec: &dyn EncryptorDecryptor) -> Result { - let cc_number = decrypt_str(encdec, &self.cc_number_enc)?; + let cc_number = + SecureCreditCardFields::decrypt(&self.cc_number_enc, encdec, self.guid.as_str())? + .cc_number; Ok(CreditCardPayload { id: self.guid, entry: PayloadEntry { From eb4fa2f3ba08c735e8857216d6b99816291e913e Mon Sep 17 00:00:00 2001 From: theidkamp Date: Mon, 31 Aug 2026 18:24:42 +0200 Subject: [PATCH 3/3] FXCM-2280: Migrate stored card numbers to a versioned secure-fields blob Store the encrypted number as {"v":1,"n":""} rather than the bare number, so a CVV can be added as a second encrypted field later without rewriting every row again. db::migrate_cc_secure_fields rewrites existing rows in place, on the raw table so sync_change_counter and time_last_modified do not move, and matching on the old ciphertext so a concurrent write is not clobbered. It runs from run_maintenance before the vacuum there, guarded by a moz_meta flag that is only set when no row was skipped. Bump the schema to 6. No tables change; the bump makes a downgrade fail in open_database rather than read a blob as a card number and upload it. --- CHANGELOG.md | 1 + components/autofill/src/autofill.udl | 3 + .../src/db/migrate_cc_secure_fields.rs | 389 ++++++++++++++++++ components/autofill/src/db/mod.rs | 1 + .../autofill/src/db/models/credit_card.rs | 83 +++- components/autofill/src/db/schema.rs | 11 +- components/autofill/src/db/store.rs | 6 +- components/autofill/src/encryption.rs | 11 +- 8 files changed, 492 insertions(+), 13 deletions(-) create mode 100644 components/autofill/src/db/migrate_cc_secure_fields.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 23d8b3af5fd..d5b37905415 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ ### Autofill - `update_address()` now sets `time_last_modified` to the time of the update, matching `update_credit_card()` and `update_passport()`. +- Credit-card numbers are now stored as a versioned JSON blob rather than the bare number, so a second encrypted field can be added later without rewriting every row again. `run_maintenance()` rewrites existing rows once, in place, without touching sync metadata. The schema version is bumped to 6, so opening the database with an older build now fails rather than reading a blob as a card number. ### Ads-Client diff --git a/components/autofill/src/autofill.udl b/components/autofill/src/autofill.udl index ab5471fba64..c2ba882d1d6 100644 --- a/components/autofill/src/autofill.udl +++ b/components/autofill/src/autofill.udl @@ -264,6 +264,9 @@ interface Store { /// Run maintenance on the DB /// + /// Also completes the one-off rewrite of credit-card rows that predate the + /// versioned secure-fields encoding. Cheap once that has finished. + /// /// This is intended to be run during idle time and will take steps / to clean up / shrink the /// database. [Throws=AutofillApiError] diff --git a/components/autofill/src/db/migrate_cc_secure_fields.rs b/components/autofill/src/db/migrate_cc_secure_fields.rs new file mode 100644 index 00000000000..fe2055c198e --- /dev/null +++ b/components/autofill/src/db/migrate_cc_secure_fields.rs @@ -0,0 +1,389 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public +* License, v. 2.0. If a copy of the MPL was not distributed with this +* file, You can obtain one at http://mozilla.org/MPL/2.0/. +*/ + +//! Rewrites credit-card rows that predate the versioned secure-fields blob. + +use crate::db::store::{get_meta, put_meta}; +use crate::db::AutofillDb; +use crate::encryption::{decrypt_str, encrypt_str, EncryptorDecryptor}; +use crate::error::Result; +use error_support::info; +use rusqlite::{named_params, Transaction}; +use serde::{Deserialize, Serialize}; + +const MIGRATION_DONE_META_KEY: &str = "cc_secure_fields_migrated"; + +/// Frozen on purpose: a later change to `SecureCreditCardFields` must not alter +/// what this migration already wrote for rows it has rewritten. +#[derive(Serialize)] +struct FrozenV1<'a> { + v: u8, + n: &'a str, +} + +/// Only the presence of `v` matters, so the payload is irrelevant. +#[derive(Deserialize)] +struct VersionProbe { + #[allow(dead_code)] + v: u8, +} + +#[derive(Debug, Default, PartialEq, Eq)] +pub struct CreditCardMigrationMetrics { + pub migrated: u64, + pub already_migrated: u64, + pub undecryptable: u64, + pub conflicted: u64, +} + +/// The flag is only set when nothing was skipped: a row the key could not read +/// is left for a later run, because the key can be wrong now and right later. +pub(crate) fn migrate_cc_secure_fields_if_needed( + db: &AutofillDb, +) -> Result { + if get_meta::(&db.writer, MIGRATION_DONE_META_KEY)?.unwrap_or(false) { + return Ok(CreditCardMigrationMetrics::default()); + } + + let tx = db.writer.unchecked_transaction()?; + let metrics = migrate_cc_secure_fields(&tx, db.encdec.as_ref())?; + if metrics.undecryptable == 0 && metrics.conflicted == 0 { + put_meta(&tx, MIGRATION_DONE_META_KEY, &true)?; + } + tx.commit()?; + + if metrics != CreditCardMigrationMetrics::default() { + // No guids and no card data - just counts, so this is safe to log. + info!( + "cc secure-fields migration: {} migrated, {} already migrated, \ + {} unreadable, {} conflicted", + metrics.migrated, metrics.already_migrated, metrics.undecryptable, metrics.conflicted + ); + } + Ok(metrics) +} + +pub(crate) fn migrate_cc_secure_fields( + tx: &Transaction<'_>, + encdec: &dyn EncryptorDecryptor, +) -> Result { + let mut metrics = CreditCardMigrationMetrics::default(); + + // Empty ciphertext marks scrubbed data waiting to be replaced from Sync, + // not an encrypted number, so it has to stay empty. + let rows = tx + .prepare("SELECT guid, cc_number_enc FROM credit_cards_data WHERE cc_number_enc != ''")? + .query_map([], |row| { + Ok((row.get::<_, String>(0)?, row.get::<_, String>(1)?)) + })? + .collect::>>()?; + + for (guid, old_ciphertext) in rows { + // Giving up on a card is what + // `scrub_undecryptable_credit_card_data_for_remote_replacement` is for. + let Ok(cleartext) = decrypt_str(encdec, &old_ciphertext) else { + metrics.undecryptable += 1; + continue; + }; + + if serde_json::from_str::(&cleartext).is_ok() { + metrics.already_migrated += 1; + continue; + } + + let blob = serde_json::to_string(&FrozenV1 { + v: 1, + n: &cleartext, + })?; + let new_ciphertext = encrypt_str(encdec, &blob)?; + + // Matching the old ciphertext makes this a compare-and-swap, so a row + // written between the SELECT above and here is not clobbered. No other + // column is named, which is the point of doing this in raw SQL. + let updated = tx.execute( + "UPDATE credit_cards_data + SET cc_number_enc = :new + WHERE guid = :guid AND cc_number_enc = :old", + named_params! { + ":new": &new_ciphertext, + ":guid": &guid, + ":old": &old_ciphertext, + }, + )?; + + if updated == 1 { + metrics.migrated += 1; + } else { + metrics.conflicted += 1; + } + } + + Ok(metrics) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::db::credit_cards::{add_credit_card, get_credit_card}; + use crate::db::models::credit_card::{ + InternalCreditCard, SecureCreditCardFields, UpdatableCreditCardFields, + }; + use crate::db::test::{new_mem_db, new_mem_db_with_encdec}; + use crate::encryption::{static_key_encryptor, ManagedEncryptorDecryptor}; + use nss_as::ensure_initialized; + use std::sync::Arc; + use sync_guid::Guid; + + const NUMBER: &str = "4111111111117629"; + + fn add_legacy_card( + db: &AutofillDb, + encdec: &dyn EncryptorDecryptor, + number: &str, + ) -> InternalCreditCard { + add_credit_card( + db, + UpdatableCreditCardFields { + cc_name: "jane doe".to_string(), + cc_number_enc: encrypt_str(encdec, number).unwrap(), + cc_number_last_4: number[number.len() - 4..].to_string(), + cc_exp_month: 9, + cc_exp_year: 2027, + cc_type: "visa".to_string(), + }, + ) + .unwrap() + } + + fn run(db: &AutofillDb) -> CreditCardMigrationMetrics { + let tx = db.writer.unchecked_transaction().unwrap(); + let metrics = migrate_cc_secure_fields(&tx, db.encdec.as_ref()).unwrap(); + tx.commit().unwrap(); + metrics + } + + fn run_if_needed(db: &AutofillDb) -> CreditCardMigrationMetrics { + migrate_cc_secure_fields_if_needed(db).unwrap() + } + + fn new_encdec() -> ManagedEncryptorDecryptor { + ensure_initialized(); + static_key_encryptor(&db_crypto::create_key().unwrap()).unwrap() + } + + #[test] + fn test_legacy_row_is_rewritten_as_a_blob() { + let db = new_mem_db(); + let card = add_legacy_card(&db, db.encdec.as_ref(), NUMBER); + + let metrics = run(&db); + assert_eq!(metrics.migrated, 1); + assert_eq!(metrics.already_migrated, 0); + + let stored = get_credit_card(&db, &card.guid).unwrap().cc_number_enc; + assert_ne!( + stored, card.cc_number_enc, + "the ciphertext must have been replaced" + ); + assert_eq!( + decrypt_str(db.encdec.as_ref(), &stored).unwrap(), + format!(r#"{{"v":1,"n":"{NUMBER}"}}"#), + "the on-disk format is frozen - a change here breaks v1 readers" + ); + } + + #[test] + fn test_the_number_survives_the_rewrite() { + let db = new_mem_db(); + let card = add_legacy_card(&db, db.encdec.as_ref(), NUMBER); + run(&db); + + let stored = get_credit_card(&db, &card.guid).unwrap().cc_number_enc; + assert_eq!( + SecureCreditCardFields::decrypt(&stored, db.encdec.as_ref(), card.guid.as_str()) + .unwrap() + .cc_number, + NUMBER + ); + } + + #[test] + fn test_sync_metadata_is_not_touched() { + let db = new_mem_db(); + let card = add_legacy_card(&db, db.encdec.as_ref(), NUMBER); + let before = get_credit_card(&db, &card.guid).unwrap().metadata; + + run(&db); + + let after = get_credit_card(&db, &card.guid).unwrap().metadata; + assert_eq!( + after.sync_change_counter, before.sync_change_counter, + "a re-encryption is not a user edit - bumping this uploads every card" + ); + assert_eq!(after.time_last_modified, before.time_last_modified); + assert_eq!(after.time_created, before.time_created); + assert_eq!(after.time_last_used, before.time_last_used); + assert_eq!(after.times_used, before.times_used); + } + + #[test] + fn test_second_run_is_a_noop() { + let db = new_mem_db(); + let card = add_legacy_card(&db, db.encdec.as_ref(), NUMBER); + + assert_eq!(run(&db).migrated, 1); + let after_first = get_credit_card(&db, &card.guid).unwrap().cc_number_enc; + + let metrics = run(&db); + assert_eq!(metrics.migrated, 0); + assert_eq!(metrics.already_migrated, 1); + assert_eq!( + get_credit_card(&db, &card.guid).unwrap().cc_number_enc, + after_first, + "a row already in the new format must not be re-encrypted" + ); + } + + #[test] + fn test_undecryptable_row_is_left_alone() { + let db = new_mem_db_with_encdec(Arc::new(new_encdec())); + let old_encdec = new_encdec(); + let card = add_legacy_card(&db, &old_encdec, "2345678923456789"); + + let metrics = run(&db); + assert_eq!(metrics.undecryptable, 1); + assert_eq!(metrics.migrated, 0); + assert_eq!( + get_credit_card(&db, &card.guid).unwrap().cc_number_enc, + card.cc_number_enc, + "an unreadable row must survive untouched, not be scrubbed" + ); + } + + #[test] + fn test_scrubbed_row_stays_scrubbed() { + let db = new_mem_db(); + let card = add_credit_card( + &db, + UpdatableCreditCardFields { + cc_name: "jane doe".to_string(), + cc_number_enc: String::new(), + cc_number_last_4: "7629".to_string(), + cc_exp_month: 9, + cc_exp_year: 2027, + cc_type: "visa".to_string(), + }, + ) + .unwrap(); + + let metrics = run(&db); + assert_eq!(metrics, CreditCardMigrationMetrics::default()); + assert!( + get_credit_card(&db, &card.guid) + .unwrap() + .cc_number_enc + .is_empty(), + "empty ciphertext means 'replace from Sync', not 'encrypted'" + ); + } + + #[test] + fn test_mixed_formats_in_one_pass() { + let db = new_mem_db(); + let legacy = add_legacy_card(&db, db.encdec.as_ref(), NUMBER); + let already = add_credit_card( + &db, + UpdatableCreditCardFields { + cc_name: "john doe".to_string(), + cc_number_enc: SecureCreditCardFields { + cc_number: "5500005555555559".to_string(), + } + .encrypt(db.encdec.as_ref(), "new-row") + .unwrap(), + cc_number_last_4: "5559".to_string(), + cc_exp_month: 1, + cc_exp_year: 2030, + cc_type: "mastercard".to_string(), + }, + ) + .unwrap(); + + let metrics = run(&db); + assert_eq!(metrics.migrated, 1); + assert_eq!(metrics.already_migrated, 1); + + for (guid, expected) in [(&legacy.guid, NUMBER), (&already.guid, "5500005555555559")] { + let stored = get_credit_card(&db, guid).unwrap().cc_number_enc; + assert_eq!( + SecureCreditCardFields::decrypt(&stored, db.encdec.as_ref(), guid.as_str()) + .unwrap() + .cc_number, + expected + ); + } + } + + #[test] + fn test_empty_table_is_fine() { + let db = new_mem_db(); + let metrics = run(&db); + assert_eq!(metrics, CreditCardMigrationMetrics::default()); + let _ = Guid::new("unused"); + } + + #[test] + fn test_the_flag_stops_the_second_pass() { + let db = new_mem_db(); + let card = add_legacy_card(&db, db.encdec.as_ref(), NUMBER); + + assert_eq!(run_if_needed(&db).migrated, 1); + let after_first = get_credit_card(&db, &card.guid).unwrap().cc_number_enc; + + let metrics = run_if_needed(&db); + assert_eq!( + metrics, + CreditCardMigrationMetrics::default(), + "a finished migration must not look at the rows again - not even to \ + count them as already migrated" + ); + assert_eq!( + get_credit_card(&db, &card.guid).unwrap().cc_number_enc, + after_first + ); + } + + #[test] + fn test_the_flag_is_withheld_while_a_row_is_unreadable() { + let db = new_mem_db_with_encdec(Arc::new(new_encdec())); + let old_encdec = new_encdec(); + add_legacy_card(&db, &old_encdec, "2345678923456789"); + add_legacy_card(&db, db.encdec.as_ref(), NUMBER); + + let first = run_if_needed(&db); + assert_eq!(first.migrated, 1); + assert_eq!(first.undecryptable, 1); + + let second = run_if_needed(&db); + assert_eq!(second.undecryptable, 1, "the pass was wrongly marked done"); + assert_eq!(second.already_migrated, 1); + assert_eq!(second.migrated, 0); + } + + #[test] + fn test_an_empty_table_still_marks_the_migration_done() { + let db = new_mem_db(); + assert_eq!(run_if_needed(&db), CreditCardMigrationMetrics::default()); + + let card = add_legacy_card(&db, db.encdec.as_ref(), NUMBER); + let metrics = run_if_needed(&db); + assert_eq!(metrics, CreditCardMigrationMetrics::default()); + assert_eq!( + get_credit_card(&db, &card.guid).unwrap().cc_number_enc, + card.cc_number_enc, + "the flag is set, so this legacy row is not picked up - which is why \ + the format switch has to ship with the migration, not after it" + ); + } +} diff --git a/components/autofill/src/db/mod.rs b/components/autofill/src/db/mod.rs index 5ba5ac88f26..3ca5c26efe7 100644 --- a/components/autofill/src/db/mod.rs +++ b/components/autofill/src/db/mod.rs @@ -4,6 +4,7 @@ pub mod addresses; pub mod credit_cards; +pub(crate) mod migrate_cc_secure_fields; pub mod models; pub mod passports; pub mod schema; diff --git a/components/autofill/src/db/models/credit_card.rs b/components/autofill/src/db/models/credit_card.rs index 107c383c590..8e733625bb4 100644 --- a/components/autofill/src/db/models/credit_card.rs +++ b/components/autofill/src/db/models/credit_card.rs @@ -5,18 +5,17 @@ //! Credit-card models and the cleartext fields that are stored encrypted. //! -//! Only the number is encrypted today, and the encrypted value is that number -//! verbatim - nothing about the stored format changes here. +//! Only the number is encrypted today, but the encrypted value is a versioned +//! JSON blob rather than the bare number, so a CVV can be added later. //! -//! `SecureCreditCardFields` is a struct because a CVV is expected as a second -//! encrypted field. Two values need a structured encoding, and existing rows -//! then have to be rewritten: they decrypt to a bare number, where the new code -//! would expect a structure. That migration is a separate ticket. +//! Rows written before that change still decrypt to a bare number. `decrypt` +//! accepts both, and `db::migrate_cc_secure_fields` rewrites the old ones. use super::Metadata; use crate::encryption::{decrypt_str, encrypt_str, EncryptorDecryptor}; use crate::error::Error; use rusqlite::Row; +use serde::{Deserialize, Serialize}; use sync_guid::Guid; #[derive(Debug, Clone, Default)] @@ -117,6 +116,18 @@ impl InternalCreditCard { } } +/// The version written today. A reader that meets a higher version fails rather +/// than guessing, so a future format cannot be misread as this one. +const SECURE_FIELDS_VERSION: u8 = 1; + +/// `db::migrate_cc_secure_fields` keeps its own frozen copy of the v1 shape, so +/// changing this struct does not change what it already wrote. +#[derive(Serialize, Deserialize)] +struct StoredSecureFields { + v: u8, + n: String, +} + /// Cleartext credit-card fields that are encrypted for local storage. #[derive(Debug, Clone, Hash, PartialEq, Eq, Default)] pub struct SecureCreditCardFields { @@ -130,7 +141,13 @@ impl SecureCreditCardFields { encdec: &dyn EncryptorDecryptor, guid: &str, ) -> crate::error::Result { - encrypt_str(encdec, &self.cc_number) + let stored = StoredSecureFields { + v: SECURE_FIELDS_VERSION, + n: self.cc_number.clone(), + }; + let cleartext = serde_json::to_string(&stored) + .map_err(|e| Error::EncryptionFailed(format!("{e} (encrypting {guid})")))?; + encrypt_str(encdec, &cleartext) .map_err(|e| Error::EncryptionFailed(format!("{e} (encrypting {guid})"))) } @@ -139,13 +156,27 @@ impl SecureCreditCardFields { encdec: &dyn EncryptorDecryptor, guid: &str, ) -> crate::error::Result { - let cc_number = decrypt_str(encdec, ciphertext).map_err(|e| { + let cleartext = decrypt_str(encdec, ciphertext).map_err(|e| { Error::DecryptionFailed(format!( "{e} (decrypting {guid}, ciphertext length: {})", ciphertext.len() )) })?; - Ok(Self { cc_number }) + + match serde_json::from_str::(&cleartext) { + Ok(stored) if stored.v == SECURE_FIELDS_VERSION => Ok(Self { + cc_number: stored.n, + }), + Ok(stored) => Err(Error::DecryptionFailed(format!( + "unsupported secure-fields version {} (decrypting {guid})", + stored.v + ))), + // A bare number is not valid JSON for the blob, so a parse failure + // is how a row written before the migration identifies itself. + Err(_) => Ok(Self { + cc_number: cleartext, + }), + } } } @@ -198,6 +229,40 @@ mod tests { assert!(InternalCreditCard::default().has_scrubbed_data()); } + #[test] + fn test_decrypt_accepts_a_pre_migration_row() { + let encdec = encdec(); + let legacy = crate::encryption::encrypt_str(&encdec, "4111111111117629").unwrap(); + assert_eq!( + SecureCreditCardFields::decrypt(&legacy, &encdec, "test-guid") + .unwrap() + .cc_number, + "4111111111117629" + ); + } + + #[test] + fn test_encrypt_writes_a_versioned_blob() { + let encdec = encdec(); + let stored = SecureCreditCardFields { + cc_number: "4111111111117629".to_string(), + } + .encrypt(&encdec, "test-guid") + .unwrap(); + assert_eq!( + crate::encryption::decrypt_str(&encdec, &stored).unwrap(), + r#"{"v":1,"n":"4111111111117629"}"# + ); + } + + #[test] + fn test_decrypt_refuses_an_unknown_version() { + let encdec = encdec(); + let future = + crate::encryption::encrypt_str(&encdec, r#"{"v":2,"n":"4111111111117629"}"#).unwrap(); + assert!(SecureCreditCardFields::decrypt(&future, &encdec, "test-guid").is_err()); + } + #[test] fn test_encrypting_twice_gives_different_ciphertext() { let encdec = encdec(); diff --git a/components/autofill/src/db/schema.rs b/components/autofill/src/db/schema.rs index 85a32c7fa9c..d286d4d680d 100644 --- a/components/autofill/src/db/schema.rs +++ b/components/autofill/src/db/schema.rs @@ -108,7 +108,7 @@ pub struct AutofillConnectionInitializer; impl ConnectionInitializer for AutofillConnectionInitializer { const NAME: &'static str = "autofill db"; - const END_VERSION: u32 = 5; + const END_VERSION: u32 = 6; fn prepare(&self, conn: &Connection, _db_empty: bool) -> Result<()> { define_functions(conn)?; @@ -139,6 +139,7 @@ impl ConnectionInitializer for AutofillConnectionInitializer { 2 => upgrade_from_v2(db), 3 => upgrade_from_v3(db), 4 => upgrade_from_v4(db), + 5 => upgrade_from_v5(db), _ => Err(Error::IncompatibleVersion(version)), } } @@ -279,6 +280,14 @@ fn upgrade_from_v4(db: &Connection) -> Result<()> { Ok(()) } +fn upgrade_from_v5(_db: &Connection) -> Result<()> { + // v5 -> v6 changes no tables. The bump exists so that a downgrade to a + // build without the versioned secure-fields blob fails in `open_database` + // with `IncompatibleVersion`, instead of reading a blob as a card number + // and uploading it to the server. + Ok(()) +} + pub fn create_empty_sync_temp_tables(db: &Connection) -> Result<()> { debug!("Initializing sync temp tables"); db.execute_batch(CREATE_SYNC_TEMP_TABLES_SQL)?; diff --git a/components/autofill/src/db/store.rs b/components/autofill/src/db/store.rs index efb46c7874d..07ce5598808 100644 --- a/components/autofill/src/db/store.rs +++ b/components/autofill/src/db/store.rs @@ -9,7 +9,8 @@ use crate::db::models::address::{ use crate::db::models::credit_card::{CreditCard, UpdatableCreditCardFields}; use crate::db::models::passport::{Passport, UpdatablePassportFields}; use crate::db::{ - addresses, credit_cards, credit_cards::CreditCardsDeletionMetrics, passports, AutofillDb, + addresses, credit_cards, credit_cards::CreditCardsDeletionMetrics, + migrate_cc_secure_fields::migrate_cc_secure_fields_if_needed, passports, AutofillDb, }; use crate::encryption::EncryptorDecryptor; use crate::error::*; @@ -333,6 +334,9 @@ impl Store { #[handle_error(Error)] pub fn run_maintenance(&self) -> ApiResult<()> { let conn = self.lock_db()?; + // Before the vacuum below, so the old ciphertext does not stay behind + // in freed pages. + migrate_cc_secure_fields_if_needed(&conn)?; run_maintenance(&conn)?; Ok(()) } diff --git a/components/autofill/src/encryption.rs b/components/autofill/src/encryption.rs index 4d5ae8a6319..2fba15662f0 100644 --- a/components/autofill/src/encryption.rs +++ b/components/autofill/src/encryption.rs @@ -26,6 +26,7 @@ // // The sync code takes the encryptor from the store it already holds. +use crate::db::models::credit_card::SecureCreditCardFields; use crate::error::*; use error_support::handle_error; use std::sync::Arc; @@ -64,14 +65,20 @@ pub(crate) fn decrypt_str(encdec: &dyn EncryptorDecryptor, ciphertext: &str) -> pub fn encrypt_string(key: String, cleartext: String) -> ApiResult { // It would be nice to have more detailed error messages, but that would require the consumer // to pass them in. Let's not change the API yet. - encrypt_str(&static_key_encryptor(&key)?, &cleartext) + SecureCreditCardFields { + cc_number: cleartext, + } + .encrypt(&static_key_encryptor(&key)?, "") } #[handle_error(Error)] pub fn decrypt_string(key: String, ciphertext: String) -> ApiResult { // It would be nice to have more detailed error messages, but that would require the consumer // to pass them in. Let's not change the API yet. - decrypt_str(&static_key_encryptor(&key)?, &ciphertext) + Ok( + SecureCreditCardFields::decrypt(&ciphertext, &static_key_encryptor(&key)?, "")? + .cc_number, + ) } #[handle_error(Error)]