Fix the 'password_changed' flag in PasswordStoreChange.

Before the CL, implementation compared encrypted password values before
and after an update in the password store. That may not work on Windows:
- old passwords are encrypted with CryptProtectData. That function
produces different results if invoked for the same string twice.
- Currently we use an encryption key and Chrome utilities instead. That
means the value produced with the old and new encryption techniques
can't be compared. The old data hasn't been migrated to use the
encryption key.

The CL compares the raw password values instead.

Bug: 1087310
Change-Id: If48a41b1c5d7cad4471c0f8ee48f9e89e22182d4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2220030
Commit-Queue: Vasilii Sukhanov <vasilii@chromium.org>
Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Cr-Commit-Position: refs/heads/master@{#773168}

components/password_manager/core/browser/login_database.cc

@@ -615,6 +615,12 @@ PasswordForm GetFormForRemoval(const sql::Statement& statement) {
 
 }  // namespace
 
+struct LoginDatabase::PrimaryKeyAndPassword {
+  int primary_key;
+  std::string encrypted_password;
+  base::string16 decrypted_password;
+};
+
 LoginDatabase::LoginDatabase(const base::FilePath& db_path,
                              IsAccountStore is_account_store)
     : db_path_(db_path), is_account_store_(is_account_store) {}
@@ -1157,15 +1163,16 @@ PasswordStoreChangeList LoginDatabase::AddLogin(const PasswordForm& form,
   // Repeat the same statement but with REPLACE semantic.
   sqlite_error_code = 0;
   DCHECK(!add_replace_statement_.empty());
-  const std::string encrpyted_old_password = GetEncryptedPassword(form);
-  bool password_changed = !encrpyted_old_password.empty() &&
-                          encrpyted_old_password != encrypted_password;
-  int old_primary_key = GetPrimaryKey(form);
+  PrimaryKeyAndPassword old_primary_key_password =
+      GetPrimaryKeyAndPassword(form);
+  bool password_changed =
+      form.password_value != old_primary_key_password.decrypted_password;
   s.Assign(
       db_.GetCachedStatement(SQL_FROM_HERE, add_replace_statement_.c_str()));
   BindAddStatement(form_with_encrypted_password, &s);
   if (s.Run()) {
-    list.emplace_back(PasswordStoreChange::REMOVE, form, old_primary_key);
+    list.emplace_back(PasswordStoreChange::REMOVE, form,
+                      old_primary_key_password.primary_key);
     list.emplace_back(PasswordStoreChange::ADD,
                       std::move(form_with_encrypted_password),
                       db_.GetLastInsertRowId(), password_changed);
@@ -1195,12 +1202,12 @@ PasswordStoreChangeList LoginDatabase::UpdateLogin(const PasswordForm& form,
     return PasswordStoreChangeList();
   }
 
-  const std::string encrpyted_old_password = GetEncryptedPassword(form);
-  bool password_changed = !encrpyted_old_password.empty() &&
-                          encrpyted_old_password != encrypted_password;
+  const PrimaryKeyAndPassword old_primary_key_password =
+      GetPrimaryKeyAndPassword(form);
 
 #if defined(OS_IOS)
-  DeleteEncryptedPassword(form);
+  DeleteEncryptedPasswordFromKeychain(
+      old_primary_key_password.encrypted_password);
 #endif
   DCHECK(!update_statement_.empty());
   sql::Statement s(
@@ -1259,11 +1266,13 @@ PasswordStoreChangeList LoginDatabase::UpdateLogin(const PasswordForm& form,
 
   PasswordStoreChangeList list;
   if (db_.GetLastChangeCount()) {
+    bool password_changed =
+        form.password_value != old_primary_key_password.decrypted_password;
     PasswordForm form_with_encrypted_password = form;
     form_with_encrypted_password.encrypted_password = encrypted_password;
     list.emplace_back(PasswordStoreChange::UPDATE,
                       std::move(form_with_encrypted_password),
-                      GetPrimaryKey(form), password_changed);
+                      old_primary_key_password.primary_key, password_changed);
   } else if (error) {
     *error = UpdateLoginError::kNoUpdatedRecords;
   }
@@ -1277,12 +1286,14 @@ bool LoginDatabase::RemoveLogin(const PasswordForm& form,
   if (changes) {
     changes->clear();
   }
+  const PrimaryKeyAndPassword old_primary_key_password =
+      GetPrimaryKeyAndPassword(form);
 #if defined(OS_IOS)
-  DeleteEncryptedPassword(form);
+  DeleteEncryptedPasswordFromKeychain(
+      old_primary_key_password.encrypted_password);
 #endif
   // Remove a login by UNIQUE-constrained fields.
   DCHECK(!delete_statement_.empty());
-  int primary_key = GetPrimaryKey(form);
   sql::Statement s(
       db_.GetCachedStatement(SQL_FROM_HERE, delete_statement_.c_str()));
   s.BindString(0, form.url.spec());
@@ -1295,7 +1306,8 @@ bool LoginDatabase::RemoveLogin(const PasswordForm& form,
     return false;
   }
   if (changes) {
-    changes->emplace_back(PasswordStoreChange::REMOVE, form, primary_key,
+    changes->emplace_back(PasswordStoreChange::REMOVE, form,
+                          old_primary_key_password.primary_key,
                           /*password_changed=*/true);
   }
   return true;
@@ -1353,7 +1365,7 @@ bool LoginDatabase::RemoveLoginsCreatedBetween(
 
 #if defined(OS_IOS)
   for (const auto& pair : key_to_form_map) {
-    DeleteEncryptedPassword(*pair.second);
+    DeleteEncryptedPasswordById(pair.first);
   }
 #endif
 
@@ -1732,26 +1744,6 @@ DatabaseCleanupResult LoginDatabase::DeleteUndecryptableLogins() {
   return DatabaseCleanupResult::kSuccess;
 }
 
-std::string LoginDatabase::GetEncryptedPassword(
-    const PasswordForm& form) const {
-  TRACE_EVENT0("passwords", "LoginDatabase::GetEncryptedPassword");
-  DCHECK(!encrypted_statement_.empty());
-  sql::Statement s(
-      db_.GetCachedStatement(SQL_FROM_HERE, encrypted_statement_.c_str()));
-
-  s.BindString(0, form.url.spec());
-  s.BindString16(1, form.username_element);
-  s.BindString16(2, form.username_value);
-  s.BindString16(3, form.password_element);
-  s.BindString(4, form.signon_realm);
-
-  std::string encrypted_password;
-  if (s.Step()) {
-    s.ColumnBlobAsString(0, &encrypted_password);
-  }
-  return encrypted_password;
-}
-
 std::unique_ptr<syncer::MetadataBatch> LoginDatabase::GetAllSyncMetadata() {
   TRACE_EVENT0("passwords", "LoginDatabase::GetAllSyncMetadata");
   std::unique_ptr<syncer::MetadataBatch> metadata_batch =
@@ -1870,10 +1862,11 @@ bool LoginDatabase::CommitTransaction() {
   return db_.CommitTransaction();
 }
 
-int LoginDatabase::GetPrimaryKey(const PasswordForm& form) const {
-  DCHECK(!id_statement_.empty());
-  sql::Statement s(
-      db_.GetCachedStatement(SQL_FROM_HERE, id_statement_.c_str()));
+LoginDatabase::PrimaryKeyAndPassword LoginDatabase::GetPrimaryKeyAndPassword(
+    const PasswordForm& form) const {
+  DCHECK(!id_and_password_statement_.empty());
+  sql::Statement s(db_.GetCachedStatement(SQL_FROM_HERE,
+                                          id_and_password_statement_.c_str()));
 
   s.BindString(0, form.url.spec());
   s.BindString16(1, form.username_element);
@@ -1882,9 +1875,16 @@ int LoginDatabase::GetPrimaryKey(const PasswordForm& form) const {
   s.BindString(4, form.signon_realm);
 
   if (s.Step()) {
-    return s.ColumnInt(0);
+    PrimaryKeyAndPassword result = {s.ColumnInt(0)};
+    s.ColumnBlobAsString(1, &result.encrypted_password);
+    if (DecryptedString(result.encrypted_password,
+                        &result.decrypted_password) !=
+        ENCRYPTION_RESULT_SUCCESS) {
+      result.decrypted_password.clear();
+    }
+    return result;
   }
-  return -1;
+  return {-1, std::string(), base::string16()};
 }
 
 std::unique_ptr<syncer::MetadataBatch>
@@ -2071,14 +2071,12 @@ void LoginDatabase::InitializeStatementStrings(const SQLTableBuilder& builder) {
   blacklisted_statement_ =
       "SELECT " + all_column_names +
       " FROM logins WHERE blacklisted_by_user == ? ORDER BY origin_url";
-  DCHECK(encrypted_statement_.empty());
-  encrypted_statement_ =
-      "SELECT password_value FROM logins WHERE " + all_unique_key_column_names;
   DCHECK(encrypted_password_statement_by_id_.empty());
   encrypted_password_statement_by_id_ =
       "SELECT password_value FROM logins WHERE id=?";
-  DCHECK(id_statement_.empty());
-  id_statement_ = "SELECT id FROM logins WHERE " + all_unique_key_column_names;
+  DCHECK(id_and_password_statement_.empty());
+  id_and_password_statement_ = "SELECT id, password_value FROM logins WHERE " +
+                               all_unique_key_column_names;
 }
 
 bool LoginDatabase::IsUsingCleanupMechanism() const {

components/password_manager/core/browser/login_database.h

@@ -175,10 +175,6 @@ class LoginDatabase : public PasswordStoreSync::MetadataStore {
   // removed from the database, returns ITEM_FAILURE.
   DatabaseCleanupResult DeleteUndecryptableLogins();
 
-  // Returns the encrypted password value for the specified |form|.  Returns an
-  // empty string if the row for this |form| is not found.
-  std::string GetEncryptedPassword(const autofill::PasswordForm& form) const;
-
   // PasswordStoreSync::MetadataStore implementation.
   std::unique_ptr<syncer::MetadataBatch> GetAllSyncMetadata() override;
   void DeleteAllSyncMetadata() override;
@@ -214,16 +210,18 @@ class LoginDatabase : public PasswordStoreSync::MetadataStore {
 #endif  // defined(OS_POSIX) && !defined(OS_MACOSX)
 
  private:
+  struct PrimaryKeyAndPassword;
 #if defined(OS_IOS)
   friend class LoginDatabaseIOSTest;
   FRIEND_TEST_ALL_PREFIXES(LoginDatabaseIOSTest, KeychainStorage);
 
-  // On iOS, removes the keychain item that is used to store the
-  // encrypted password for the supplied |form|.
-  void DeleteEncryptedPassword(const autofill::PasswordForm& form);
+  // Removes the keychain item corresponding to the look-up key |cipher_text|.
+  // It's stored as the encrypted password value.
+  static void DeleteEncryptedPasswordFromKeychain(
+      const std::string& cipher_text);
 
-  // Similar to DeleteEncryptedPassword() but uses |id| to look for the
-  // password.
+  // On iOS, removes the keychain item that is used to store the encrypted
+  // password for the supplied primary key |id|.
   void DeleteEncryptedPasswordById(int id);
 
   // Returns the encrypted password value for the specified |id|.  Returns an
@@ -295,9 +293,10 @@ class LoginDatabase : public PasswordStoreSync::MetadataStore {
       bool blacklisted,
       std::vector<std::unique_ptr<autofill::PasswordForm>>* forms);
 
-  // Returns the DB primary key for the specified |form|.  Returns -1 if the row
-  // for this |form| is not found.
-  int GetPrimaryKey(const autofill::PasswordForm& form) const;
+  // Returns the DB primary key for the specified |form| and decrypted/encrypted
+  // password. Returns {-1, "", ""} if the row for this |form| is not found.
+  PrimaryKeyAndPassword GetPrimaryKeyAndPassword(
+      const autofill::PasswordForm& form) const;
 
   // Reads all the stored sync entities metadata in a MetadataBatch. Returns
   // nullptr in case of failure.
@@ -349,9 +348,8 @@ class LoginDatabase : public PasswordStoreSync::MetadataStore {
   std::string get_statement_psl_federated_;
   std::string created_statement_;
   std::string blacklisted_statement_;
-  std::string encrypted_statement_;
   std::string encrypted_password_statement_by_id_;
-  std::string id_statement_;
+  std::string id_and_password_statement_;
 
 #if defined(OS_MACOSX) && !defined(OS_IOS)
   std::unique_ptr<PasswordRecoveryUtilMac> password_recovery_util_;

components/password_manager/core/browser/login_database_ios.cc

@@ -23,38 +23,6 @@ using autofill::PasswordForm;
 
 namespace password_manager {
 
-namespace {
-
-void DeleteEncryptedPasswordFromKeychain(const std::string& cipher_text) {
-  if (cipher_text.empty())
-    return;
-
-  ScopedCFTypeRef<CFMutableDictionaryRef> query(
-      CFDictionaryCreateMutable(NULL, 0, &kCFTypeDictionaryKeyCallBacks,
-                                &kCFTypeDictionaryValueCallBacks));
-  CFDictionarySetValue(query, kSecClass, kSecClassGenericPassword);
-
-  ScopedCFTypeRef<CFStringRef> item_ref(
-      base::SysUTF8ToCFStringRef(cipher_text));
-  // We are using the account attribute to store item references.
-  CFDictionarySetValue(query, kSecAttrAccount, item_ref);
-
-  OSStatus status = SecItemDelete(query);
-  if (status != errSecSuccess && status != errSecItemNotFound) {
-    NOTREACHED() << "Unable to remove password from keychain: " << status;
-  }
-
-  // Delete the temporary passwords directory, since there might be leftover
-  // temporary files used for password export that contain the password being
-  // deleted. It can be called for a removal triggered by sync, which might
-  // happen at the same time as an export operation. In the unlikely event
-  // that the file is still needed by the consumer app, the export operation
-  // will fail.
-  password_manager::DeletePasswordsDirectory();
-}
-
-}  // namespace
-
 // On iOS, the LoginDatabase uses Keychain API to store passwords. The
 // "encrypted" version of the password is a unique ID (UUID) that is
 // stored as an attribute along with the password in the keychain.
@@ -145,9 +113,34 @@ LoginDatabase::EncryptionResult LoginDatabase::DecryptedString(
   return ENCRYPTION_RESULT_SUCCESS;
 }
 
-void LoginDatabase::DeleteEncryptedPassword(const PasswordForm& form) {
-  std::string cipher_text = GetEncryptedPassword(form);
-  DeleteEncryptedPasswordFromKeychain(cipher_text);
+// static
+void LoginDatabase::DeleteEncryptedPasswordFromKeychain(
+    const std::string& cipher_text) {
+  if (cipher_text.empty())
+    return;
+
+  ScopedCFTypeRef<CFMutableDictionaryRef> query(
+      CFDictionaryCreateMutable(nullptr, 0, &kCFTypeDictionaryKeyCallBacks,
+                                &kCFTypeDictionaryValueCallBacks));
+  CFDictionarySetValue(query, kSecClass, kSecClassGenericPassword);
+
+  ScopedCFTypeRef<CFStringRef> item_ref(
+      base::SysUTF8ToCFStringRef(cipher_text));
+  // We are using the account attribute to store item references.
+  CFDictionarySetValue(query, kSecAttrAccount, item_ref);
+
+  OSStatus status = SecItemDelete(query);
+  if (status != errSecSuccess && status != errSecItemNotFound) {
+    NOTREACHED() << "Unable to remove password from keychain: " << status;
+  }
+
+  // Delete the temporary passwords directory, since there might be leftover
+  // temporary files used for password export that contain the password being
+  // deleted. It can be called for a removal triggered by sync, which might
+  // happen at the same time as an export operation. In the unlikely event
+  // that the file is still needed by the consumer app, the export operation
+  // will fail.
+  password_manager::DeletePasswordsDirectory();
 }
 
 void LoginDatabase::DeleteEncryptedPasswordById(int id) {

components/password_manager/core/browser/login_database_unittest.cc

@@ -1375,6 +1375,44 @@ TEST_F(LoginDatabaseTest, UpdateLogin) {
   EXPECT_EQ(form, *result[0]);
 }
 
+TEST_F(LoginDatabaseTest, UpdateLoginWithoutPassword) {
+  PasswordForm form;
+  form.url = GURL("http://accounts.google.com/LoginAuth");
+  form.signon_realm = "http://accounts.google.com/";
+  form.username_value = ASCIIToUTF16("my_username");
+  form.password_value = ASCIIToUTF16("my_password");
+  form.blacklisted_by_user = false;
+  form.scheme = PasswordForm::Scheme::kHtml;
+  form.date_last_used = base::Time::Now();
+  EXPECT_EQ(AddChangeForForm(form), db().AddLogin(form));
+
+  form.action = GURL("http://accounts.google.com/login");
+  form.all_possible_usernames.push_back(autofill::ValueElementPair(
+      ASCIIToUTF16("my_new_username"), ASCIIToUTF16("new_username_id")));
+  form.times_used = 20;
+  form.submit_element = ASCIIToUTF16("submit_element");
+  form.date_synced = base::Time::Now();
+  form.date_created = base::Time::Now() - base::TimeDelta::FromDays(1);
+  form.date_last_used = base::Time::Now() + base::TimeDelta::FromDays(1);
+  form.display_name = ASCIIToUTF16("Mr. Smith");
+  form.icon_url = GURL("https://accounts.google.com/Icon");
+  form.skip_zero_click = true;
+  form.moving_blocked_for_list.push_back(GaiaIdHash::FromGaiaId("gaia_id"));
+
+  PasswordStoreChangeList changes = db().UpdateLogin(form);
+  EXPECT_EQ(UpdateChangeForForm(form, /*passwordchanged=*/false), changes);
+  ASSERT_EQ(1U, changes.size());
+  EXPECT_EQ(1, changes[0].primary_key());
+
+  // When we retrieve the form from the store, it should have |in_store| set.
+  form.in_store = PasswordForm::Store::kProfileStore;
+
+  std::vector<std::unique_ptr<PasswordForm>> result;
+  ASSERT_TRUE(db().GetLogins(PasswordStore::FormDigest(form), &result));
+  ASSERT_EQ(1U, result.size());
+  EXPECT_EQ(form, *result[0]);
+}
+
 TEST_F(LoginDatabaseTest, RemoveWrongForm) {
   PasswordForm form;
   // |origin| shouldn't be empty.