Implement Mac Keychain migration algorithm.

chrome/browser/password_manager/password_store_mac.cc

Index: chrome/browser/password_manager/password_store_mac.cc
diff --git a/chrome/browser/password_manager/password_store_mac.cc b/chrome/browser/password_manager/password_store_mac.cc
index 13ceae02d4991cf0dd38976bee75bc2e15dc9fb7..6e165c2e21482087f93bdd69d5320613435350b3 100644
--- a/chrome/browser/password_manager/password_store_mac.cc
+++ b/chrome/browser/password_manager/password_store_mac.cc
@@ -16,10 +16,12 @@
 #include "base/mac/foundation_util.h"
 #include "base/mac/mac_logging.h"
 #include "base/message_loop/message_loop.h"
+#include "base/metrics/histogram_macros.h"
 #include "base/stl_util.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/mac/security_wrappers.h"
+#include "components/os_crypt/os_crypt.h"
 #include "components/password_manager/core/browser/affiliation_utils.h"
 #include "components/password_manager/core/browser/login_database.h"
 #include "components/password_manager/core/browser/password_store_change.h"
@@ -435,6 +437,35 @@ bool FillPasswordFormFromKeychainItem(const AppleKeychain& keychain,
   return true;
 }
 
+bool HasChromeCreatorCode(const AppleKeychain& keychain,
+                          const SecKeychainItemRef& keychain_item) {
+  SecKeychainAttributeInfo attrInfo;

[stuartmorgan, 2015/06/26 21:26:08]

attr_info

[vasilii, 2015/06/29 14:12:45]

Done.

+  UInt32 tags[] = {kSecCreatorItemAttr};
+  attrInfo.count = arraysize(tags);
+  attrInfo.tag = tags;
+  attrInfo.format = NULL;

[stuartmorgan, 2015/06/26 21:26:08]

nullptr

[vasilii, 2015/06/29 14:12:45]

Done.

+
+  SecKeychainAttributeList* attrList;

[stuartmorgan, 2015/06/26 21:26:08]

attr_list

[vasilii, 2015/06/29 14:12:44]

Done.

+  UInt32 password_length;
+  OSStatus result = keychain.ItemCopyAttributesAndData(
+      keychain_item, &attrInfo, nullptr, &attrList, &password_length, nullptr);
+  if (result != noErr)
+    return false;
+  OSType creator_code = 0;
+  for (unsigned int i = 0; i < attrList->count; i++) {
+    SecKeychainAttribute attr = attrList->attr[i];
+    if (!attr.data) {
+      continue;
+    }
+    if (attr.tag == kSecCreatorItemAttr) {
+      creator_code = *(static_cast<FourCharCode*>(attr.data));
+      break;
+    }
+  }
+  keychain.ItemFreeAttributesAndData(attrList, nullptr);
+  return creator_code && creator_code == base::mac::CreatorCodeForApplication();
+}
+
 bool FormsMatchForMerge(const PasswordForm& form_a,
                         const PasswordForm& form_b,
                         FormMatchStrictness strictness) {
@@ -922,7 +953,7 @@ PasswordStoreMac::PasswordStoreMac(
     : password_manager::PasswordStore(main_thread_runner, db_thread_runner),
       keychain_(keychain.Pass()),
       login_metadata_db_(login_db) {
-  DCHECK(keychain_.get());
+  DCHECK(keychain_);
   login_metadata_db_->set_clear_password_values(true);
 }
 
@@ -934,6 +965,87 @@ void PasswordStoreMac::InitWithTaskRunner(
   db_thread_runner_ = background_task_runner;
 }
 
+PasswordStoreMac::MigrationResult PasswordStoreMac::ImportFromKeychain() {
+  DCHECK(GetBackgroundTaskRunner()->BelongsToCurrentThread());
+  if (!login_metadata_db_)
+    return LOGIN_DB_UNAVAILABLE;
+  ScopedVector<PasswordForm> database_forms;
+  if (!login_metadata_db_->GetAutofillableLogins(&database_forms))
+    return LOGIN_DB_FAILURE;
+  ScopedVector<PasswordForm> uninteresting_forms;

[Ryan Sleevi, 2015/06/26 16:44:35]

nits (feel free to ignore) - I found the transition of lines 971->972, 974->975,
and 987->989 a bit hard to read. It wouldn't hurt to add newlines in between
(for example, you do this on 980 -> 981 as a separator for "edge condition" from
"code flow". Not required, worth considering :)

[vasilii, 2015/06/29 14:12:45]

Done.

+  internal_keychain_helpers::ExtractNonKeychainForms(&database_forms,
+                                                     &uninteresting_forms);
+  // If there are no passwords to lookup in the Keychain then we're done.
+  if (database_forms.empty())
+    return MIGRATION_OK;
+
+  // Make sure that the encryption key is retrieved from the Keychain so the
+  // encryption can be done.
+  DCHECK(!database_forms[0]->origin.is_empty());
+  std::string ciphertext;
+  if (!OSCrypt::EncryptString(database_forms[0]->origin.spec(), &ciphertext))

[stuartmorgan, 2015/06/26 21:26:07]

Why are you pulling a random field out for this, instead of just using some
hard-coded dummy text?

[vasilii, 2015/06/29 14:12:44]

Done. Binary size?

+    return ENCRYPTOR_FAILURE;
+  // Mute the Keychain.
+  chrome::ScopedSecKeychainSetUserInteractionAllowed user_interaction_allowed(
+      false);
+
+  // Retrieve the passwords.
+  // Get all the password attributes first.
+  std::vector<SecKeychainItemRef> keychain_items;
+  std::vector<internal_keychain_helpers::ItemFormPair> item_form_pairs =
+      internal_keychain_helpers::
+      ExtractAllKeychainItemAttributesIntoPasswordForms(&keychain_items,

[stuartmorgan, 2015/06/26 21:26:08]

Indent 4 more; it's a continuation

[vasilii, 2015/06/29 14:12:44]

Done.

+                                                        *keychain_);
+
+  // Next, compare the attributes of the PasswordForms in |database_forms|
+  // against those in |item_form_pairs|, and extract password data for each
+  // matching PasswordForm using its corresponding SecKeychainItemRef.
+  int unmerged_forms_count = 0;
+  int chrome_owned_locked_forms_count = 0;

[Ryan Sleevi, 2015/06/26 16:44:35]

STYLE: (usually a SECURITY BUG, but here it just seems 'danger')

https://www.chromium.org/developers/coding-style#TOC-Types
Use size_t for object and allocation sizes, object counts, array and pointer
offsets, vector indices, and so on. The signed types are incorrect and unsafe
for these purposes. (Integer overflow behavior for signed types is undefined in
the C and C++ standards, while the behavior is defined for unsigned types.) The
C++ STL is a guide here: they use size_t and foo::size_type for very good
reasons.

(Namely, you're iterating though database_forms, which is of size size_t, but
you're counting with int, which may lead to undefined overflow; this would throw
off your metrics, since you don't use these for offsets, but you hopefully see
the issue)

[vasilii, 2015/06/29 14:12:44]

Done. Though implicit conversion to int in UMA_HISTOGRAM_COUNTS isn't defined
too.

[Ilya Sherman, 2015/06/29 23:10:52]

You could use a checked_cast (from safe_numerics.h) if you'd like to be really
thorough.

[vasilii, 2015/06/30 08:45:49]

It contains CHECK(), and I don't want to have a crash here. I think
Histogram::Add does exactly what I need.

+  for (PasswordForm* form : database_forms) {
+    ScopedVector<autofill::PasswordForm> keychain_matches =
+        internal_keychain_helpers::ExtractPasswordsMergeableWithForm(

[stuartmorgan, 2015/06/26 21:26:08]

This seems wasteful (it may do extra Keychain lookups that you'll just discard
below), but I guess given the current structure of the existing flows it's the
best option.

I will not miss this class when it's gone...

[vasilii, 2015/06/29 14:12:44]

I think it should return exactly one form (which is the best match below) in
most of the cases. PSL-matched passwords are discarded in
FormIsValidAndMatchesOtherForm.

+            *keychain_, item_form_pairs, *form);
+
+    const PasswordForm* best_match =
+        BestKeychainFormForForm(*form, keychain_matches.get());
+    if (best_match) {
+      form->password_value = best_match->password_value;
+    } else {
+      unmerged_forms_count++;
+      // Check if the corresponding keychain items are created by Chrome

[stuartmorgan, 2015/06/26 21:26:07]

s/the/any/ (since there may be none). Also, missing a final period.

[vasilii, 2015/06/29 14:12:45]

Done.

+      for (const auto& item_form_pair : item_form_pairs) {
+        if (internal_keychain_helpers::FormIsValidAndMatchesOtherForm(
+                *form, *item_form_pair.second) &&
+            internal_keychain_helpers::HasChromeCreatorCode(
+                *keychain_, *item_form_pair.first))
+          chrome_owned_locked_forms_count++;
+      }
+    }
+  }
+  STLDeleteContainerPairSecondPointers(item_form_pairs.begin(),
+                                       item_form_pairs.end());
+  for (SecKeychainItemRef item : keychain_items) {
+    keychain_->Free(item);
+  }

[Ryan Sleevi, 2015/06/26 16:44:35]

nit: Since it looks like the previous comment wasn't clear - you use {} here for
a one-line conditional, but you don't elsewhere in this function. That's the
consistency bit.

That is, your early return conditionals (e.g. 986-987) and your other loop (line
1044-1045) don't use braces, so it's inconsistent that you do.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Conditionals 's
mantra is about ensuring consistency - it's fine to pick either, but if you do,
it's better to be consistent within a function, file, and 'section' of code.
That's why I flagged this - it simply stood out to me as "one of these things it
not like the other"

[vasilii, 2015/06/29 14:12:44]

Done.

+  if (unmerged_forms_count) {
+    UMA_HISTOGRAM_COUNTS(
+      "PasswordManager.KeychainMigration.NumPasswordsOnFailure",

[stuartmorgan, 2015/06/26 21:26:08]

Indent these lines 4, not 2.

[vasilii, 2015/06/29 14:12:44]

Done.

+      database_forms.size());
+    UMA_HISTOGRAM_COUNTS("PasswordManager.KeychainMigration.NumFailedPasswords",
+                         unmerged_forms_count);
+    UMA_HISTOGRAM_COUNTS(
+        "PasswordManager.KeychainMigration.NumChromeOwnedFailedPasswords",
+        chrome_owned_locked_forms_count);
+    return KEYCHAIN_BLOCKED;
+  }
+  // Now all the passwords are available. It's time to update LoginDatabase.
+  login_metadata_db_->set_clear_password_values(false);
+  for (PasswordForm* form : database_forms)
+    login_metadata_db_->UpdateLogin(*form);
+  return MIGRATION_OK;
+}
+
 bool PasswordStoreMac::Init(
     const syncer::SyncableService::StartSyncFlare& flare) {
   // The class should be used inside PasswordStoreProxyMac only.