diff --git a/Cargo.lock b/Cargo.lock index 51209168d97..3cbcb3ba531 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -867,6 +867,7 @@ dependencies = [ "fxa-client 0.1.0", "lazy_static 1.2.0 (registry+https://github.com/rust-lang/crates.io-index)", "log 0.4.6 (registry+https://github.com/rust-lang/crates.io-index)", + "memchr 2.2.0 (registry+https://github.com/rust-lang/crates.io-index)", "more-asserts 0.2.1 (registry+https://github.com/rust-lang/crates.io-index)", "prettytable-rs 0.7.0 (registry+https://github.com/rust-lang/crates.io-index)", "rusqlite 0.16.0 (registry+https://github.com/rust-lang/crates.io-index)", diff --git a/components/logins/Cargo.toml b/components/logins/Cargo.toml index a18680733c7..3c5e47a38b6 100644 --- a/components/logins/Cargo.toml +++ b/components/logins/Cargo.toml @@ -20,6 +20,7 @@ url = "1.7.1" failure = "0.1.3" sql-support = { path = "../support/sql" } ffi-support = { path = "../support/ffi", optional = true } +memchr = "2.2.0" [dependencies.rusqlite] version = "0.16.0" diff --git a/components/logins/src/db.rs b/components/logins/src/db.rs index eebe5b170b9..0fefa8fc4f9 100644 --- a/components/logins/src/db.rs +++ b/components/logins/src/db.rs @@ -41,19 +41,17 @@ impl LoginDb { // "Raw Key Data" example. Note that this would be required to open // existing iOS sqlcipher databases). format!( - " - PRAGMA key = '{}'; - - -- SQLcipher pre-4.0.0 compatibility. Using SHA1 still - -- is less than ideal, but should be fine. Real uses of - -- this (lockbox, etc) use a real random string for the - -- encryption key, so the reduced KDF iteration count - -- is fine. - PRAGMA cipher_page_size = 1024; - PRAGMA kdf_iter = 64000; - PRAGMA cipher_hmac_algorithm = HMAC_SHA1; - PRAGMA cipher_kdf_algorithm = PBKDF2_HMAC_SHA1; - ", + "PRAGMA key = '{}'; + + -- SQLcipher pre-4.0.0 compatibility. Using SHA1 still + -- is less than ideal, but should be fine. Real uses of + -- this (lockbox, etc) use a real random string for the + -- encryption key, so the reduced KDF iteration count + -- is fine. + PRAGMA cipher_page_size = 1024; + PRAGMA kdf_iter = 64000; + PRAGMA cipher_hmac_algorithm = HMAC_SHA1; + PRAGMA cipher_kdf_algorithm = PBKDF2_HMAC_SHA1; ", sql_support::escape_string_for_pragma(key) ) } else { @@ -64,13 +62,7 @@ impl LoginDb { // files in memory, since on Android there's no tmp partition. See // https://github.com/mozilla/mentat/issues/505. Ideally we'd only // do this on Android, or allow caller to configure it. - let initial_pragmas = format!( - " - {} - PRAGMA temp_store = 2; - ", - encryption_pragmas - ); + let initial_pragmas = encryption_pragmas + " PRAGMA temp_store = 2;"; db.execute_batch(&initial_pragmas)?; @@ -78,6 +70,7 @@ impl LoginDb { let tx = logins.db.transaction()?; schema::init(&tx)?; tx.commit()?; + crate::fixup::maybe_fixup_logins(&logins)?; Ok(logins) } @@ -126,13 +119,12 @@ impl LoginDb { self.db.execute( &format!( - " - INSERT OR IGNORE INTO loginsM ( - {common_cols}, is_overridden, server_modified - ) - SELECT {common_cols}, 0, {modified_ms_i64} - FROM loginsL - WHERE is_deleted = 0 AND guid IN ({vars})", + "INSERT OR IGNORE INTO loginsM + ({common_cols}, is_overridden, server_modified) + SELECT {common_cols}, 0, {modified_ms_i64} + FROM loginsL + WHERE is_deleted = 0 + AND guid IN ({vars})", common_cols = schema::COMMON_COLS, modified_ms_i64 = ts.as_millis() as i64, vars = sql_support::repeat_sql_vars(chunk.len()) @@ -181,35 +173,35 @@ impl LoginDb { write!(f, "({},?)", i + offset) }); let query = format!( - " - WITH to_fetch(guid_idx, fetch_guid) AS (VALUES {vals}) - SELECT - {common_cols}, - is_overridden, - server_modified, - NULL as local_modified, - NULL as is_deleted, - NULL as sync_status, - 1 as is_mirror, - to_fetch.guid_idx as guid_idx - FROM loginsM - JOIN to_fetch - ON loginsM.guid = to_fetch.fetch_guid - - UNION ALL - - SELECT - {common_cols}, - NULL as is_overridden, - NULL as server_modified, - local_modified, - is_deleted, - sync_status, - 0 as is_mirror, - to_fetch.guid_idx as guid_idx - FROM loginsL - JOIN to_fetch - ON loginsL.guid = to_fetch.fetch_guid", + "WITH to_fetch(guid_idx, fetch_guid) AS (VALUES {vals}) + + SELECT + {common_cols}, + is_overridden, + server_modified, + NULL as local_modified, + NULL as is_deleted, + NULL as sync_status, + 1 as is_mirror, + to_fetch.guid_idx as guid_idx + FROM loginsM + JOIN to_fetch + ON loginsM.guid = to_fetch.fetch_guid + + UNION ALL + + SELECT + {common_cols}, + NULL as is_overridden, + NULL as server_modified, + local_modified, + is_deleted, + sync_status, + 0 as is_mirror, + to_fetch.guid_idx as guid_idx + FROM loginsL + JOIN to_fetch + ON loginsL.guid = to_fetch.fetch_guid", // give each VALUES item 2 entries, an index and the parameter. vals = values_with_idx, common_cols = schema::COMMON_COLS, @@ -242,28 +234,27 @@ impl LoginDb { // It would be nice if this were a batch-ish api (e.g. takes a slice of records and finds dupes // for each one if they exist)... I can't think of how to write that query, though. fn find_dupe(&self, l: &Login) -> Result> { - let form_submit_host_port = l + let form_submit_value = l .form_submit_url .as_ref() - .and_then(|s| util::url_host_port(&s)); + .map(|s| form_submit_url_dedupe_key(&s)); let args = &[ - (":hostname", &l.hostname as &ToSql), - (":http_realm", &l.http_realm as &ToSql), - (":username", &l.username as &ToSql), - (":form_submit", &form_submit_host_port as &ToSql), + (":hostname", &l.hostname as &dyn ToSql), + (":http_realm", &l.http_realm), + (":username", &l.username), + (":form_submit", &form_submit_value), ]; let mut query = format!( - " - SELECT {common} - FROM loginsL - WHERE hostname IS :hostname - AND httpRealm IS :http_realm - AND username IS :username", + "SELECT {common} + FROM loginsL + WHERE hostname IS :hostname + AND httpRealm IS :http_realm + AND username IS :username", common = schema::COMMON_COLS, ); - if form_submit_host_port.is_some() { + if form_submit_value.is_some() { // Stolen from iOS - query += " AND (formSubmitURL = '' OR (instr(formSubmitURL, :form_submit) > 0))"; + query += " AND (formSubmitURL = '' OR formSubmitURL IS :form_submit)"; } else { query += " AND formSubmitURL IS :form_submit" } @@ -277,12 +268,7 @@ impl LoginDb { } pub fn get_by_id(&self, id: &str) -> Result> { - self.try_query_row( - &GET_BY_GUID_SQL, - &[(":guid", &id as &ToSql)], - Login::from_row, - true, - ) + self.try_query_row(&GET_BY_GUID_SQL, &[(":guid", &id)], Login::from_row, true) } pub fn touch(&self, id: &str) -> Result<()> { @@ -292,14 +278,13 @@ impl LoginDb { // As on iOS, just using a record doesn't flip it's status to changed. // TODO: this might be wrong for lockbox! self.execute_named_cached( - " - UPDATE loginsL + "UPDATE loginsL SET timeLastUsed = :now_millis, timesUsed = timesUsed + 1, local_modified = :now_millis WHERE guid = :guid AND is_deleted = 0", - &[(":now_millis", &now_ms as &ToSql), (":guid", &id as &ToSql)], + &[(":now_millis", &now_ms), (":guid", &id)], )?; Ok(()) } @@ -321,6 +306,7 @@ impl LoginDb { login.id = sync15::random_guid() .expect("Failed to generate failed to generate random bytes for GUID"); } + crate::fixup::fixup_record_for_database(&mut login); // Fill in default metadata. // TODO: allow this to be provided for testing? @@ -330,8 +316,7 @@ impl LoginDb { login.times_used = 1; let sql = format!( - " - INSERT OR IGNORE INTO loginsL ( + "INSERT OR IGNORE INTO loginsL ( hostname, httpRealm, formSubmitURL, @@ -370,22 +355,19 @@ impl LoginDb { let rows_changed = self.execute_named( &sql, &[ - (":hostname", &login.hostname as &ToSql), - (":http_realm", &login.http_realm as &ToSql), - (":form_submit_url", &login.form_submit_url as &ToSql), - (":username_field", &login.username_field as &ToSql), - (":password_field", &login.password_field as &ToSql), - (":username", &login.username as &ToSql), - (":password", &login.password as &ToSql), - (":guid", &login.id as &ToSql), - (":time_created", &login.time_created as &ToSql), - (":times_used", &login.times_used as &ToSql), - (":time_last_used", &login.time_last_used as &ToSql), - ( - ":time_password_changed", - &login.time_password_changed as &ToSql, - ), - (":local_modified", &now_ms as &ToSql), + (":hostname", &login.hostname), + (":http_realm", &login.http_realm), + (":form_submit_url", &login.form_submit_url), + (":username_field", &login.username_field), + (":password_field", &login.password_field), + (":username", &login.username), + (":password", &login.password), + (":guid", &login.id), + (":time_created", &login.time_created), + (":times_used", &login.times_used), + (":time_last_used", &login.time_last_used), + (":time_password_changed", &login.time_password_changed), + (":local_modified", &now_ms), ], )?; if rows_changed == 0 { @@ -398,7 +380,7 @@ impl LoginDb { Ok(login) } - pub fn update(&self, login: Login) -> Result<()> { + pub fn update(&self, mut login: Login) -> Result<()> { login.check_valid()?; // Note: These fail with DuplicateGuid if the record doesn't exist. self.ensure_local_overlay_exists(login.guid_str())?; @@ -406,10 +388,11 @@ impl LoginDb { let now_ms = util::system_time_ms_i64(SystemTime::now()); + crate::fixup::fixup_record_for_database(&mut login); + let sql = format!( - " - UPDATE loginsL - SET local_modified = :now_millis, + "UPDATE loginsL SET + local_modified = :now_millis, timeLastUsed = :now_millis, -- Only update timePasswordChanged if, well, the password changed. timePasswordChanged = (CASE @@ -434,15 +417,15 @@ impl LoginDb { self.db.execute_named( &sql, &[ - (":hostname", &login.hostname as &ToSql), - (":username", &login.username as &ToSql), - (":password", &login.password as &ToSql), - (":http_realm", &login.http_realm as &ToSql), - (":form_submit_url", &login.form_submit_url as &ToSql), - (":username_field", &login.username_field as &ToSql), - (":password_field", &login.password_field as &ToSql), - (":guid", &login.id as &ToSql), - (":now_millis", &now_ms as &ToSql), + (":hostname", &login.hostname), + (":username", &login.username), + (":password", &login.password), + (":http_realm", &login.http_realm), + (":form_submit_url", &login.form_submit_url), + (":username_field", &login.username_field), + (":password_field", &login.password_field), + (":guid", &login.id), + (":now_millis", &now_ms), ], )?; Ok(()) @@ -450,15 +433,14 @@ impl LoginDb { pub fn exists(&self, id: &str) -> Result { Ok(self.db.query_row_named( - " - SELECT EXISTS( + "SELECT EXISTS( SELECT 1 FROM loginsL WHERE guid = :guid AND is_deleted = 0 UNION ALL SELECT 1 FROM loginsM WHERE guid = :guid AND is_overridden IS NOT 1 )", - &[(":guid", &id as &ToSql)], + &[(":guid", &id)], |row| row.get(0), )?) } @@ -472,61 +454,55 @@ impl LoginDb { // Directly delete IDs that have not yet been synced to the server self.execute_named( &format!( - " - DELETE FROM loginsL - WHERE guid = :guid - AND sync_status = {status_new}", + "DELETE FROM loginsL + WHERE guid = :guid + AND sync_status = {status_new}", status_new = SyncStatus::New as u8 ), - &[(":guid", &id as &ToSql)], + &[(":guid", &id)], )?; // For IDs that have, mark is_deleted and clear sensitive fields self.execute_named( &format!( - " - UPDATE loginsL - SET local_modified = :now_ms, - sync_status = {status_changed}, - is_deleted = 1, - password = '', - hostname = '', - username = '' - WHERE guid = :guid", + "UPDATE loginsL SET + local_modified = :now_ms, + sync_status = {status_changed}, + is_deleted = 1, + password = '', + hostname = '', + username = '' + WHERE guid = :guid", status_changed = SyncStatus::Changed as u8 ), - &[(":now_ms", &now_ms as &ToSql), (":guid", &id as &ToSql)], + &[(":now_ms", &now_ms), (":guid", &id)], )?; // Mark the mirror as overridden self.execute_named( "UPDATE loginsM SET is_overridden = 1 WHERE guid = :guid", - &[(":guid", &id as &ToSql)], + &[(":guid", &id)], )?; // If we don't have a local record for this ID, but do have it in the mirror // insert a tombstone. - self.execute_named(&format!(" - INSERT OR IGNORE INTO loginsL - (guid, local_modified, is_deleted, sync_status, hostname, timeCreated, timePasswordChanged, password, username) - SELECT guid, :now_ms, 1, {changed}, '', timeCreated, :now_ms, '', '' - FROM loginsM - WHERE guid = :guid", - changed = SyncStatus::Changed as u8), - &[(":now_ms", &now_ms as &ToSql), - (":guid", &id as &ToSql)])?; + self.execute_named(&format!( + "INSERT OR IGNORE INTO loginsL + (guid, local_modified, is_deleted, sync_status, hostname, timeCreated, timePasswordChanged, password, username) + SELECT guid, :now_ms, 1, {changed}, '', timeCreated, :now_ms, '', '' + FROM loginsM + WHERE guid = :guid", + changed = SyncStatus::Changed as u8 + ), + &[(":now_ms", &now_ms), (":guid", &id)])?; Ok(exists) } fn mark_mirror_overridden(&self, guid: &str) -> Result<()> { self.execute_named_cached( - " - UPDATE loginsM SET - is_overridden = 1 - WHERE guid = :guid - ", - &[(":guid", &guid as &ToSql)], + "UPDATE loginsM SET is_overridden = 1 WHERE guid = :guid", + &[(":guid", &guid)], )?; Ok(()) } @@ -534,7 +510,7 @@ impl LoginDb { fn ensure_local_overlay_exists(&self, guid: &str) -> Result<()> { let already_have_local: bool = self.db.query_row_named( "SELECT EXISTS(SELECT 1 FROM loginsL WHERE guid = :guid)", - &[(":guid", &guid as &ToSql)], + &[(":guid", &guid)], |row| row.get(0), )?; @@ -552,7 +528,7 @@ impl LoginDb { } fn clone_mirror_to_overlay(&self, guid: &str) -> Result { - Ok(self.execute_named_cached(&*CLONE_SINGLE_MIRROR_SQL, &[(":guid", &guid as &ToSql)])?) + Ok(self.execute_named_cached(&*CLONE_SINGLE_MIRROR_SQL, &[(":guid", &guid)])?) } pub fn reset(&self) -> Result<()> { @@ -579,9 +555,8 @@ impl LoginDb { )?; self.execute_named( &format!( - " - UPDATE loginsL - SET local_modified = :now_ms, + "UPDATE loginsL SET + local_modified = :now_ms, sync_status = {changed}, is_deleted = 1, password = '', @@ -590,7 +565,7 @@ impl LoginDb { WHERE is_deleted = 0", changed = SyncStatus::Changed as u8 ), - &[(":now_ms", &now_ms as &ToSql)], + &[(":now_ms", &now_ms)], )?; self.execute("UPDATE loginsM SET is_overridden = 1", NO_PARAMS)?; @@ -602,7 +577,7 @@ impl LoginDb { SELECT guid, :now_ms, 1, {changed}, '', timeCreated, :now_ms, '', '' FROM loginsM", changed = SyncStatus::Changed as u8), - &[(":now_ms", &now_ms as &ToSql)])?; + &[(":now_ms", &now_ms)])?; Ok(()) } @@ -715,19 +690,18 @@ impl LoginDb { Ok(self.fetch_outgoing(inbound.timestamp)?) } - fn put_meta(&self, key: &str, value: &ToSql) -> Result<()> { + pub(crate) fn put_meta(&self, key: &str, value: &dyn ToSql) -> Result<()> { self.execute_named_cached( "REPLACE INTO loginsSyncMeta (key, value) VALUES (:key, :value)", - &[(":key", &key as &ToSql), (":value", value)], + &[(":key", &key), (":value", value)], )?; Ok(()) } - fn get_meta(&self, key: &str) -> Result> { - Ok(self.try_query_row( + pub(crate) fn get_meta(&self, key: &str) -> Result> { + Ok(self.try_query_one( "SELECT value FROM loginsSyncMeta WHERE key = :key", - &[(":key", &key as &ToSql)], - |row| Ok::<_, Error>(row.get_checked(0)?), + &[(":key", &key)], true, )?) } @@ -800,39 +774,50 @@ impl Store for LoginDb { } } +fn form_submit_url_dedupe_key(url_str: &str) -> String { + // force normalization. + let url = if let Ok(url) = url::Url::parse(url_str) { + url + } else { + return url_str.into(); + }; + let (prefix, hostport) = crate::util::prefix_hostport(url.as_str()); + if hostport.len() != 0 { + prefix.to_string() + hostport + } else { + url_str.into() + } +} + lazy_static! { static ref GET_ALL_SQL: String = format!( - " - SELECT {common_cols} FROM loginsL WHERE is_deleted = 0 - UNION ALL - SELECT {common_cols} FROM loginsM WHERE is_overridden = 0 - ", + "SELECT {common_cols} FROM loginsL WHERE is_deleted = 0 + UNION ALL + SELECT {common_cols} FROM loginsM WHERE is_overridden = 0", common_cols = schema::COMMON_COLS, ); static ref GET_BY_GUID_SQL: String = format!( - " - SELECT {common_cols} - FROM loginsL - WHERE is_deleted = 0 - AND guid = :guid - - UNION ALL - - SELECT {common_cols} - FROM loginsM - WHERE is_overridden IS NOT 1 - AND guid = :guid - ORDER BY hostname ASC - - LIMIT 1 - ", + "SELECT {common_cols} + FROM loginsL + WHERE is_deleted = 0 + AND guid = :guid + + UNION ALL + + SELECT {common_cols} + FROM loginsM + WHERE is_overridden IS NOT 1 + AND guid = :guid + ORDER BY hostname ASC + + LIMIT 1", common_cols = schema::COMMON_COLS, ); static ref CLONE_ENTIRE_MIRROR_SQL: String = format!( - " - INSERT OR IGNORE INTO loginsL ({common_cols}, local_modified, is_deleted, sync_status) - SELECT {common_cols}, NULL AS local_modified, 0 AS is_deleted, 0 AS sync_status - FROM loginsM", + "INSERT OR IGNORE INTO loginsL + ({common_cols}, local_modified, is_deleted, sync_status) + SELECT {common_cols}, NULL AS local_modified, 0 AS is_deleted, 0 AS sync_status + FROM loginsM", common_cols = schema::COMMON_COLS, ); static ref CLONE_SINGLE_MIRROR_SQL: String = diff --git a/components/logins/src/engine.rs b/components/logins/src/engine.rs index 99075459886..5e5529e3ac3 100644 --- a/components/logins/src/engine.rs +++ b/components/logins/src/engine.rs @@ -136,7 +136,7 @@ mod test { let a = Login { id: "aaaaaaaaaaaa".into(), hostname: "https://www.example.com".into(), - form_submit_url: Some("https://www.example.com/login".into()), + form_submit_url: Some("https://login.example.com".into()), username: "coolperson21".into(), password: "p4ssw0rd".into(), username_field: "user_input".into(), diff --git a/components/logins/src/fixup.rs b/components/logins/src/fixup.rs new file mode 100644 index 00000000000..c49fc3b77df --- /dev/null +++ b/components/logins/src/fixup.rs @@ -0,0 +1,350 @@ +/* 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/. */ + +use crate::db::LoginDb; +use crate::error::Result; +use crate::login::Login; +use crate::util; +use url::{Origin, Url}; + +pub fn has_bad_character(field: &str) -> bool { + memchr::memchr3(b'\r', b'\n', b'\0', field.as_bytes()).is_some() +} + +/// Fixes `l` in the following ways: +/// +/// 1. If possible, ensure `hostname` is a sane origin. +/// - Failing it being a sane origin, we tries to ensure it's a parsable URL +/// - Failing that, leave it alone. +/// +/// 2. Reconcile inconsistent or corrupted `formSubmitURL` and `httpRealm` values. +/// - If neither are present, assume `form_submit_url` is actually "" +/// and some client messed it up. +/// +/// - If both are present, resolve in favor of `httpRealm` unless it's an +/// empty string, or contains characters that cause desktop to barf +/// (`\n`, `\r`, `\0`). +/// +/// - If only `formSubmitURL` is present, or both are present but we don't want +/// the httpRealm value, use `formSubmitURL`, possibly fixing it up +/// - If it's an empty string, we use the empty string. +/// - Otherwise, attempt to use it's origin, if we can get one. +/// - If we can't, but we can get a parsable URL, use that. +/// - Otherwise, use the (fixed up version of the) hostname. +/// +/// - If only `httpRealm` is present and it only contains valid characters, +/// take it. +/// +/// - If only `httpRealm` is present and it contains `\r`, `\n`, or `\0`, +/// replace them with spaces. (This is not likely to work for logging in, +/// but seems better than discarding a password the user may not remember). +/// +/// 3. If `username_field` or `password_field` contain characters that would break +/// desktop, then set the fields to "". Those fields are optional and +/// semi-deprecated anyway. +/// +/// 4. Remove any `\0` that happen to exist in `username` or `password`. This is +/// slightly dubious, but it's unclear to me what the better option is. +/// Deletion seems bad, and it's likely to cause us problems in the FFI (at +/// least, before we get around to using protobufs) +fn fix_login(l: &Login) -> Login { + // Wasteful but whatever. + let mut fixed = l.clone(); + let hostname = if let Some(fixed) = try_fixup_origin_string(&l.hostname, true, false) { + fixed + } else { + // Hostname is extremely important, but I'm just going to keep + // the original if we can't fix it. + l.hostname.clone() + }; + + if hostname != l.hostname { + log::trace!(" Fixing {} hostname", l.id); + fixed.hostname = hostname.into(); + } + + let (next_url, next_realm) = match (&l.form_submit_url, &l.http_realm) { + (None, None) => { + // If they're both missing, assume the empty form url is supposed to + // be the empty string and some client (maybe us in a previous version!) + // confused things. + (Some("".into()), None) + } + + (Some(_), Some(realm)) if !realm.is_empty() && !has_bad_character(realm) => { + // If the realm exists and is valid, go with that over the form + // url unless it's empty. + (None, Some(realm.clone())) + } + + // If the URL exists, and the realm doesn't or we can't use it, + // then use the url, possibly fixing it up and turning it into an + // origin as needed. In the case of a url that doesn't parse as + // such, either use the empty string (if it was empty/entirely + // whitespace), or the hostname. + (Some(url), _) => { + // No realm, or empty/corrupt realm. + let res_url = if let Some(s) = try_fixup_origin_string(url, true, true) { + s + } else { + // Can't borrow from fixed.hostname since we need + // to assign to it later + fixed.hostname.clone() + }; + (Some(res_url), None) + } + + // The 'only realm' case is straightforward, with the unfortunate + // caveat that if the realm is not valid, we try to replace the + // illegal characters with spaces. This probably won't actually work, + // seems better than throwing the record away and possibly losing + // a password that the user no longer remembers. + // + // In practice this should never happen, as 'invalid' means it has + // characters which aren't even allowed in HTTP headers to begin with. + (None, Some(realm)) => { + if has_bad_character(realm) { + log::trace!(" Fixup {}: Invalid realm", l.id); + let realm = realm.replace(|c| c == '\r' || c == '\n' || c == '\0', " "); + (None, Some(realm)) + } else { + (None, Some(realm.clone())) + } + } + }; + + if next_url != l.form_submit_url { + log::trace!(" Fixup {}: Changed form_submit_url", l.id); + fixed.form_submit_url = next_url; + } + if next_realm != l.http_realm { + // already logged about this. + fixed.http_realm = next_realm.into(); + } + + // username_field and password_field are pseudo-deprecated (as far as I + // understand it), so if they're causing problems, then clear them. + if has_bad_character(&l.username_field) || l.username_field == "." { + log::trace!(" Fixup {}: Invalid username_field", l.id); + fixed.username_field.clear(); + } + if has_bad_character(&l.password_field) { + log::trace!(" Fixup {}: Invalid password_field", l.id); + fixed.password_field.clear(); + } + + // This is wrong, but should be so rare that doesn't matter. + // Remove '\0' from the username and password, if present. + fixed.password = l.password.replace('\0', ""); + fixed.username = l.username.replace('\0', ""); + + fixed +} + +pub fn maybe_fixup_logins(db: &LoginDb) -> Result<()> { + // If we've ever done the fixup, don't bother doing it again. Eventually + // we might want to change that, but for now it should just be a 'run once' + // thing + if db + .get_meta::(crate::schema::LAST_FIXUP_TIME_META_KEY)? + .is_some() + { + return Ok(()); + } + log::info!("Running login fixup"); + let now = util::system_time_ms_i64(std::time::SystemTime::now()); + // Write it in advance. If something goes wrong, we don't want to + // keep trying this over and over again. + db.put_meta(crate::schema::LAST_FIXUP_TIME_META_KEY, &now)?; + + let records = db.get_all()?.into_iter().filter_map(|record| { + let new_record = fix_login(&record); + if new_record != record { + Some(new_record) + } else { + None + } + }); + + // Not bothering with a transaction since these changes should all + // be improvements -- we dont want to roll back on failure. + for rec in records { + log::debug!("Applying change for record {}", rec.id); + db.update(rec)?; + } + + log::info!("Fixup finished"); + // Note: Arguably, we should dedupe here, but for now, we aren't, since we + // aren't in a good position to resolve any conflicts we find. + Ok(()) +} + +// `allow_non_origin` indicates if we're willing to take a non-origin if its a valid url but +// that the URL spec has defined as having an opaque origin (includes all schemes +// other than `"ftp" | "gopher" | "http" | "https" | "ws" | "wss"`) +pub fn try_fixup_origin_string( + url_str: &str, + allow_non_origin: bool, + allow_empty_string: bool, +) -> Option { + let url_str = url_str + .trim() + .replace(|c| c == '\0' || c == '\r' || c == '\n', ""); + if url_str == "" || url_str == "." { + return if allow_empty_string { + Some("".into()) + } else { + None + }; + } + let url = match Url::parse(&url_str) { + Ok(v) => v, + Err(_) => { + // try again with only the parts we actually want, in case + // some garbage is in the path or userinfo bits that we + // ignore. We also remove spaces from the result, in desperation. + let (prefix, hostport) = util::prefix_hostport(&url_str); + let to_parse = (prefix.to_string() + hostport).replace(|c| c == ' ' || c == '\t', ""); + if to_parse.is_empty() && allow_empty_string { + return Some("".into()); + } + + Url::parse(&to_parse).ok()? + } + }; + match url.origin() { + // All schemes other than a few well known ones come through as opaque, + // and stringify as "null" :| + Origin::Opaque(_) => { + if allow_non_origin { + Some(url.to_string()) + } else { + None + } + } + tuple_origin => Some(tuple_origin.ascii_serialization()), + } +} + +pub fn fixup_record_for_database(login: &mut Login) { + if let Some(hostname) = try_fixup_origin_string(&login.hostname, true, false) { + login.hostname = hostname; + } + + let maybe_url = login + .form_submit_url + .as_ref() + .map(|url| try_fixup_origin_string(url, true, true)); + + if let Some(Some(fixed_up_url)) = maybe_url { + login.form_submit_url = Some(fixed_up_url); + } +} +#[cfg(test)] +mod test { + use super::*; + fn login<'a>( + hostname: &str, + user: &str, + pass: &str, + form_submit_url: impl Into>, + http_realm: impl Into>, + ) -> Login { + Login { + id: "".into(), + hostname: hostname.into(), + username: user.into(), + password: pass.into(), + form_submit_url: form_submit_url.into().map(|s| s.into()), + http_realm: http_realm.into().map(|s| s.into()), + username_field: "".into(), + password_field: "".into(), + time_created: 0, + time_password_changed: 0, + time_last_used: 0, + times_used: 0, + } + } + fn login_with_fields<'a>( + hostname: &str, + user: &str, + pass: &str, + form_submit_url: impl Into>, + http_realm: impl Into>, + user_field: &str, + pass_field: &str, + ) -> Login { + let mut l = login(hostname, user, pass, form_submit_url, http_realm); + l.username_field = user_field.into(); + l.password_field = pass_field.into(); + l + } + + fn check_fixed(l0: Login, l1: Login) { + assert_eq!(fix_login(&l0), l1) + } + #[test] + #[rustfmt::skip] + // This is a lot harder to read what each thing is testing when things get split out a lot + fn test_fix_login() { + check_fixed( + login("garbage hostname", "username", "password", "", None), + login("garbage hostname", "username", "password", "", None) + ); + + check_fixed( + login("http://www.example.com", "username\0", "password\0", "", None), + login("http://www.example.com", "username", "password", "", None) + ); + + check_fixed( + login("http://www.example\n.com", "username", "password", "", None), + login("http://www.example.com", "username", "password", "", None) + ); + + check_fixed( + login("http://www.example.com/path", "username", "password", "", None), + login("http://www.example.com", "username", "password", "", None) + ); + + check_fixed( + login("http://www.example.com/path", "username", "password", "garbage form submit url", None), + login("http://www.example.com", "username", "password", "http://www.example.com", None) + ); + + check_fixed( + login("http://www.example.com/path", "username", "password", "http://user:stuff@www.notexample.com:8080/path", None), + login("http://www.example.com", "username", "password", "http://www.notexample.com:8080", None) + ); + + check_fixed( + login("http://www.example.com", "user\0name", "pass\0word", "", None), + login("http://www.example.com", "username", "password", "", None) + ); + + check_fixed( + login("http://www.example.com", "username", "password", None, None), + login("http://www.example.com", "username", "password", "", None) + ); + + check_fixed( + login("http://www.example.com", "username", "password", None, "invalid\nrealm"), + login("http://www.example.com", "username", "password", None, "invalid realm") + ); + check_fixed( + login("http://www.example.com", "username", "password", None, "invalid\nrealm"), + login("http://www.example.com", "username", "password", None, "invalid realm") + ); + + check_fixed( + login_with_fields("http://www.example.com", "username", "password", "", None, "gar\0\nbage", "pass-field"), + login_with_fields("http://www.example.com", "username", "password", "", None, "", "pass-field") + ); + + check_fixed( + login_with_fields("http://www.example.com", "username", "password", "", None, "user-field", "gar\0\nbage"), + login_with_fields("http://www.example.com", "username", "password", "", None, "user-field", "") + ); + } +} diff --git a/components/logins/src/lib.rs b/components/logins/src/lib.rs index 6d0f125d8df..e65737bb6fd 100644 --- a/components/logins/src/lib.rs +++ b/components/logins/src/lib.rs @@ -8,6 +8,7 @@ mod login; mod db; mod engine; +mod fixup; pub mod schema; mod update_plan; mod util; diff --git a/components/logins/src/login.rs b/components/logins/src/login.rs index 0048d234e2d..1710199225c 100644 --- a/components/logins/src/login.rs +++ b/components/logins/src/login.rs @@ -15,10 +15,12 @@ pub struct Login { // TODO: consider `#[serde(rename = "id")] pub guid: String` to avoid confusion pub id: String, + /// Note: Despite it's name, this is an origin and not a hostname. pub hostname: String, - // rename_all = "camelCase" by default will do formSubmitUrl, but we can just - // override this one field. + /// Note: Despite it's name, this is an origin and not a URL. + /// + /// (It may also be the empty string) #[serde(rename = "formSubmitURL")] #[serde(skip_serializing_if = "Option::is_none")] pub form_submit_url: Option, diff --git a/components/logins/src/schema.rs b/components/logins/src/schema.rs index eeb511bfc43..e659bddbaa0 100644 --- a/components/logins/src/schema.rs +++ b/components/logins/src/schema.rs @@ -207,6 +207,7 @@ const UPDATE_MIRROR_TIMESTAMPS_TO_MILLIS_SQL: &'static str = " pub(crate) static LAST_SYNC_META_KEY: &'static str = "last_sync_time"; pub(crate) static GLOBAL_STATE_META_KEY: &'static str = "global_state"; +pub(crate) static LAST_FIXUP_TIME_META_KEY: &'static str = "last_fixup_time"; pub(crate) fn init(db: &Connection) -> Result<()> { let user_version = db.query_one::("PRAGMA user_version")?; diff --git a/components/logins/src/update_plan.rs b/components/logins/src/update_plan.rs index f620cdfa915..b12495f8a53 100644 --- a/components/logins/src/update_plan.rs +++ b/components/logins/src/update_plan.rs @@ -5,7 +5,7 @@ use crate::error::*; use crate::login::{LocalLogin, Login, MirrorLogin, SyncStatus}; use crate::util; -use rusqlite::{types::ToSql, Connection}; +use rusqlite::Connection; use std::time::SystemTime; use sync15::ServerTimestamp; @@ -118,22 +118,19 @@ impl UpdatePlan { for (login, timestamp) in &self.mirror_updates { log::trace!("Updating mirror {:?}", login.guid_str()); stmt.execute_named(&[ - (":server_modified", timestamp as &ToSql), - (":http_realm", &login.http_realm as &ToSql), - (":form_submit_url", &login.form_submit_url as &ToSql), - (":username_field", &login.username_field as &ToSql), - (":password_field", &login.password_field as &ToSql), - (":password", &login.password as &ToSql), - (":hostname", &login.hostname as &ToSql), - (":username", &login.username as &ToSql), - (":times_used", &login.times_used as &ToSql), - (":time_last_used", &login.time_last_used as &ToSql), - ( - ":time_password_changed", - &login.time_password_changed as &ToSql, - ), - (":time_created", &login.time_created as &ToSql), - (":guid", &login.guid_str() as &ToSql), + (":server_modified", timestamp), + (":http_realm", &login.http_realm), + (":form_submit_url", &login.form_submit_url), + (":username_field", &login.username_field), + (":password_field", &login.password_field), + (":password", &login.password), + (":hostname", &login.hostname), + (":username", &login.username), + (":times_used", &login.times_used), + (":time_last_used", &login.time_last_used), + (":time_password_changed", &login.time_password_changed), + (":time_created", &login.time_created), + (":guid", &login.guid_str()), ])?; } Ok(()) @@ -183,23 +180,20 @@ impl UpdatePlan { for (login, timestamp, is_overridden) in &self.mirror_inserts { log::trace!("Inserting mirror {:?}", login.guid_str()); stmt.execute_named(&[ - (":is_overridden", is_overridden as &ToSql), - (":server_modified", timestamp as &ToSql), - (":http_realm", &login.http_realm as &ToSql), - (":form_submit_url", &login.form_submit_url as &ToSql), - (":username_field", &login.username_field as &ToSql), - (":password_field", &login.password_field as &ToSql), - (":password", &login.password as &ToSql), - (":hostname", &login.hostname as &ToSql), - (":username", &login.username as &ToSql), - (":times_used", &login.times_used as &ToSql), - (":time_last_used", &login.time_last_used as &ToSql), - ( - ":time_password_changed", - &login.time_password_changed as &ToSql, - ), - (":time_created", &login.time_created as &ToSql), - (":guid", &login.guid_str() as &ToSql), + (":is_overridden", is_overridden), + (":server_modified", timestamp), + (":http_realm", &login.http_realm), + (":form_submit_url", &login.form_submit_url), + (":username_field", &login.username_field), + (":password_field", &login.password_field), + (":password", &login.password), + (":hostname", &login.hostname), + (":username", &login.username), + (":times_used", &login.times_used), + (":time_last_used", &login.time_last_used), + (":time_password_changed", &login.time_password_changed), + (":time_created", &login.time_created), + (":guid", &login.guid_str()), ])?; } Ok(()) @@ -207,9 +201,8 @@ impl UpdatePlan { fn perform_local_updates(&self, conn: &Connection) -> Result<()> { let sql = format!( - " - UPDATE loginsL - SET local_modified = :local_modified, + "UPDATE loginsL SET + local_modified = :local_modified, httpRealm = :http_realm, formSubmitURL = :form_submit_url, usernameField = :username_field, @@ -230,21 +223,18 @@ impl UpdatePlan { for l in &self.local_updates { log::trace!("Updating local {:?}", l.guid_str()); stmt.execute_named(&[ - (":local_modified", &local_ms as &ToSql), - (":http_realm", &l.login.http_realm as &ToSql), - (":form_submit_url", &l.login.form_submit_url as &ToSql), - (":username_field", &l.login.username_field as &ToSql), - (":password_field", &l.login.password_field as &ToSql), - (":password", &l.login.password as &ToSql), - (":hostname", &l.login.hostname as &ToSql), - (":username", &l.login.username as &ToSql), - (":time_last_used", &l.login.time_last_used as &ToSql), - ( - ":time_password_changed", - &l.login.time_password_changed as &ToSql, - ), - (":times_used", &l.login.times_used as &ToSql), - (":guid", &l.guid_str() as &ToSql), + (":local_modified", &local_ms), + (":http_realm", &l.login.http_realm), + (":form_submit_url", &l.login.form_submit_url), + (":username_field", &l.login.username_field), + (":password_field", &l.login.password_field), + (":password", &l.login.password), + (":hostname", &l.login.hostname), + (":username", &l.login.username), + (":time_last_used", &l.login.time_last_used), + (":time_password_changed", &l.login.time_password_changed), + (":times_used", &l.login.times_used), + (":guid", &l.guid_str()), ])?; } Ok(()) diff --git a/components/logins/src/util.rs b/components/logins/src/util.rs index 542d25c6df3..d8589a3dd9b 100644 --- a/components/logins/src/util.rs +++ b/components/logins/src/util.rs @@ -5,16 +5,38 @@ use crate::error::*; use rusqlite::Row; use std::time; -use url::Url; - -pub fn url_host_port(url_str: &str) -> Option { - let url = Url::parse(url_str).ok()?; - let host = url.host_str()?; - Some(if let Some(p) = url.port() { - format!("{}:{}", host, p) - } else { - host.to_string() - }) + +// from places +fn split_after_prefix(href: &str) -> (&str, &str) { + match memchr::memchr(b':', href.as_bytes()) { + None => ("", href), + Some(index) => { + let hb = href.as_bytes(); + let mut end = index + 1; + if hb.len() >= end + 2 && hb[end] == b'/' && hb[end + 1] == b'/' { + end += 2; + } + href.split_at(end) + } + } +} + +/// Returns: +/// +/// - the prefix (scheme, colon, and '//' if present) +/// - host:port +/// +/// e.g. removes path, query, fragment, and userinfo. +pub fn prefix_hostport(href: &str) -> (&str, &str) { + let (prefix, remainder) = split_after_prefix(href); + + let start = memchr::memchr(b'@', remainder.as_bytes()) + .map(|i| i + 1) + .unwrap_or(0); + + let remainder = &remainder[start..]; + let end = memchr::memchr3(b'/', b'?', b'#', remainder.as_bytes()).unwrap_or(remainder.len()); + (prefix, &remainder[..end]) } pub fn system_time_millis_from_row(row: &Row, col_name: &str) -> Result {