Isolate PasswordHashData in BUILD.gn

As pointed out in a chromium-dev discussion [1], PasswordHashData is the
only piece of the password_manager component used in //chromeos.
However, it is part of a target which ultimately depends on a big chunk
of components.

This CL therefore isolates the struct to a separate target with no
dependencies other than base and crypto. It also ensures that the struct
is always initialized and const methods marked as such. Finally, it also
moves the older and related SyncPasswordData struct and some helper
functions into the new target, to avoid reintroducing the large
dependencies through HashPasswordManager.

[1] https://groups.google.com/a/chromium.org/d/msg/chromium-dev/nX63vp-75_U/h7H19i9dAQAJ

Bug: 850931
Change-Id: I0733f46bbc924bf63c43ff3cf25b148c2baef8e8
Reviewed-on: https://chromium-review.googlesource.com/1092863
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566380}

chromeos/BUILD.gn

@@ -39,7 +39,7 @@ component("chromeos") {
     "//components/account_id",
     "//components/device_event_log",
     "//components/onc",
-    "//components/password_manager/core/browser:hash_password_manager",
+    "//components/password_manager/core/browser:password_hash_data",
     "//components/policy:cloud_policy_proto_generated_compile",
     "//components/policy/proto",
     "//components/pref_registry",

chromeos/login/DEPS

 include_rules = [
   "+components/account_id",
-  "+components/password_manager/core/browser/hash_password_manager.h",
+  "+components/password_manager/core/browser/password_hash_data.h",
   "+components/user_manager",
   "+google_apis",
 ]

chromeos/login/auth/user_context.h

@@ -12,7 +12,7 @@
 #include "chromeos/login/auth/challenge_response_key.h"
 #include "chromeos/login/auth/key.h"
 #include "components/account_id/account_id.h"
-#include "components/password_manager/core/browser/hash_password_manager.h"
+#include "components/password_manager/core/browser/password_hash_data.h"
 #include "components/user_manager/user_type.h"
 
 class AccountId;

components/password_manager/core/browser/BUILD.gn

@@ -179,6 +179,7 @@ static_library("browser") {
   ]
   deps = [
     ":hash_password_manager",
+    ":password_hash_data",
     ":proto",
     "//base:i18n",
     "//components/autofill/core/browser",
@@ -236,17 +237,28 @@ proto_library("proto") {
   ]
 }
 
+static_library("password_hash_data") {
+  sources = [
+    "password_hash_data.cc",
+    "password_hash_data.h",
+  ]
+  deps = [
+    "//base",
+    "//crypto",
+  ]
+}
+
 static_library("hash_password_manager") {
   sources = [
     "hash_password_manager.cc",
     "hash_password_manager.h",
   ]
   deps = [
+    ":password_hash_data",
     "//base:base",
     "//components/os_crypt",
     "//components/password_manager/core/common",
     "//components/prefs",
-    "//crypto",
   ]
 }
 
@@ -367,6 +379,7 @@ source_set("unit_tests") {
     "password_form_manager_unittest.cc",
     "password_form_metrics_recorder_unittest.cc",
     "password_generation_manager_unittest.cc",
+    "password_hash_data_unittest.cc",
     "password_list_sorter_unittest.cc",
     "password_manager_metrics_recorder_unittest.cc",
     "password_manager_unittest.cc",
@@ -404,6 +417,7 @@ source_set("unit_tests") {
 
   deps = [
     ":hash_password_manager",
+    ":password_hash_data",
     ":test_support",
     ":unit_tests_bundle_data",
     "//base/test:test_support",

components/password_manager/core/browser/hash_password_manager.cc

@@ -12,12 +12,9 @@
 #include "components/password_manager/core/common/password_manager_pref_names.h"
 #include "components/prefs/pref_service.h"
 #include "components/prefs/scoped_user_pref_update.h"
-#include "crypto/openssl_util.h"
-#include "crypto/random.h"
-#include "third_party/boringssl/src/include/openssl/evp.h"
 
 namespace {
-constexpr size_t kSyncPasswordSaltLength = 16;
+
 constexpr char kSeparator = '.';
 constexpr char kHashFieldKey[] = "hash";
 constexpr char kLastSignInTimeFieldKey[] = "last_signin";
@@ -27,6 +24,7 @@ constexpr char kIsGaiaFieldKey[] = "is_gaia";
 
 // The maximum number of password hash data we store in prefs.
 constexpr size_t kMaxPasswordHashDataDictSize = 5;
+
 }  // namespace
 
 namespace password_manager {
@@ -140,47 +138,6 @@ base::Optional<PasswordHashData> ConvertToPasswordHashData(
 
 }  // namespace
 
-SyncPasswordData::SyncPasswordData(const base::string16& password,
-                                   bool force_update)
-    : length(password.size()),
-      salt(HashPasswordManager::CreateRandomSalt()),
-      hash(HashPasswordManager::CalculatePasswordHash(password, salt)),
-      force_update(force_update) {}
-
-bool SyncPasswordData::MatchesPassword(const base::string16& password) {
-  if (password.size() != this->length)
-    return false;
-  return HashPasswordManager::CalculatePasswordHash(password, this->salt) ==
-         this->hash;
-}
-
-PasswordHashData::PasswordHashData(const std::string& username,
-                                   const base::string16& password,
-                                   bool force_update,
-                                   bool is_gaia_password)
-    : username(username),
-      length(password.size()),
-      salt(HashPasswordManager::CreateRandomSalt()),
-      hash(HashPasswordManager::CalculatePasswordHash(password, salt)),
-      force_update(force_update),
-      is_gaia_password(is_gaia_password) {}
-
-PasswordHashData::PasswordHashData() = default;
-
-PasswordHashData::PasswordHashData(const PasswordHashData& other) = default;
-
-bool PasswordHashData::MatchesPassword(const std::string& username,
-                                       const base::string16& password,
-                                       bool is_gaia_password) {
-  if (password.size() != this->length || username != this->username ||
-      is_gaia_password != this->is_gaia_password) {
-    return false;
-  }
-
-  return HashPasswordManager::CalculatePasswordHash(password, this->salt) ==
-         this->hash;
-}
-
 HashPasswordManager::HashPasswordManager(PrefService* prefs) : prefs_(prefs) {}
 
 bool HashPasswordManager::SavePasswordHash(const base::string16& password) {
@@ -389,52 +346,6 @@ void HashPasswordManager::MaybeMigrateExistingSyncPasswordHash(
   prefs_->ClearPref(prefs::kSyncPasswordLengthAndHashSalt);
 }
 
-// static
-std::string HashPasswordManager::CreateRandomSalt() {
-  char buffer[kSyncPasswordSaltLength];
-  crypto::RandBytes(buffer, kSyncPasswordSaltLength);
-  // Explicit std::string constructor with a string length must be used in order
-  // to avoid treating '\0' symbols as a string ends.
-  std::string result(buffer, kSyncPasswordSaltLength);
-  return result;
-}
-
-// static
-uint64_t HashPasswordManager::CalculatePasswordHash(
-    const base::StringPiece16& text,
-    const std::string& salt) {
-  crypto::OpenSSLErrStackTracer err_tracer(FROM_HERE);
-  constexpr size_t kBytesFromHash = 8;
-  constexpr uint64_t kScryptCost = 32;  // It must be power of 2.
-  constexpr uint64_t kScryptBlockSize = 8;
-  constexpr uint64_t kScryptParallelization = 1;
-  constexpr size_t kScryptMaxMemory = 1024 * 1024;
-
-  uint8_t hash[kBytesFromHash];
-  base::StringPiece text_8bits(reinterpret_cast<const char*>(text.data()),
-                               text.size() * 2);
-  const uint8_t* salt_ptr = reinterpret_cast<const uint8_t*>(salt.c_str());
-
-  int scrypt_ok = EVP_PBE_scrypt(text_8bits.data(), text_8bits.size(), salt_ptr,
-                                 salt.size(), kScryptCost, kScryptBlockSize,
-                                 kScryptParallelization, kScryptMaxMemory, hash,
-                                 kBytesFromHash);
-
-  // EVP_PBE_scrypt can only fail due to memory allocation error (which aborts
-  // Chromium) or invalid parameters. In case of a failure a hash could leak
-  // information from the stack, so using CHECK is better than DCHECK.
-  CHECK(scrypt_ok);
-
-  // Take 37 bits of |hash|.
-  uint64_t hash37 = ((static_cast<uint64_t>(hash[0]))) |
-                    ((static_cast<uint64_t>(hash[1])) << 8) |
-                    ((static_cast<uint64_t>(hash[2])) << 16) |
-                    ((static_cast<uint64_t>(hash[3])) << 24) |
-                    (((static_cast<uint64_t>(hash[4])) & 0x1F) << 32);
-
-  return hash37;
-}
-
 bool HashPasswordManager::EncryptAndSaveToPrefs(const std::string& pref_name,
                                                 const std::string& s) {
   std::string encrypted_base64_text = EncryptString(s);

components/password_manager/core/browser/hash_password_manager.h

@@ -8,45 +8,12 @@
 #include "base/macros.h"
 #include "base/optional.h"
 #include "base/strings/string16.h"
+#include "components/password_manager/core/browser/password_hash_data.h"
 
 class PrefService;
 
 namespace password_manager {
 
-// SyncPasswordData is being deprecated. Please use PasswordHashData instead.
-struct SyncPasswordData {
-  SyncPasswordData() = default;
-  SyncPasswordData(const SyncPasswordData& other) = default;
-  SyncPasswordData(const base::string16& password, bool force_update);
-  bool MatchesPassword(const base::string16& password);
-
-  size_t length;
-  std::string salt;
-  uint64_t hash;
-  // Signal that we need to update password hash, salt, and length in profile
-  // prefs.
-  bool force_update;
-};
-
-struct PasswordHashData {
-  PasswordHashData();
-  PasswordHashData(const PasswordHashData& other);
-  PasswordHashData(const std::string& username,
-                   const base::string16& password,
-                   bool force_update,
-                   bool is_gaia_password = true);
-  bool MatchesPassword(const std::string& username,
-                       const base::string16& password,
-                       bool is_gaia_password);
-
-  std::string username;
-  size_t length;
-  std::string salt;
-  uint64_t hash;
-  bool force_update;
-  bool is_gaia_password = true;
-};
-
 // Responsible for saving, clearing, retrieving and encryption of a password
 // hash data in preferences.
 // All methods should be called on UI thread.
@@ -91,13 +58,6 @@ class HashPasswordManager {
   // that has already been captured.
   void MaybeMigrateExistingSyncPasswordHash(const std::string& sync_username);
 
-  static std::string CreateRandomSalt();
-
-  // Calculates 37 bits hash for a password. The calculation is based on a slow
-  // hash function. The running time is ~10^{-4} seconds on Desktop.
-  static uint64_t CalculatePasswordHash(const base::StringPiece16& text,
-                                        const std::string& salt);
-
  private:
   // Saves encrypted string |s| in a preference |pref_name|. Returns true on
   // success.

components/password_manager/core/browser/hash_password_manager_unittest.cc

@@ -6,6 +6,7 @@
 
 #include "base/strings/utf_string_conversions.h"
 #include "components/os_crypt/os_crypt_mocker.h"
+#include "components/password_manager/core/browser/password_hash_data.h"
 #include "components/password_manager/core/common/password_manager_pref_names.h"
 #include "components/prefs/pref_registry_simple.h"
 #include "components/prefs/testing_pref_service.h"
@@ -223,7 +224,7 @@ TEST_F(HashPasswordManagerTest, RetrievingSyncPasswordData) {
   ASSERT_TRUE(sync_password_data);
   EXPECT_EQ(13u, sync_password_data->length);
   EXPECT_EQ(16u, sync_password_data->salt.size());
-  uint64_t expected_hash = HashPasswordManager::CalculatePasswordHash(
+  uint64_t expected_hash = CalculatePasswordHash(
       base::UTF8ToUTF16("sync_password"), sync_password_data->salt);
   EXPECT_EQ(expected_hash, sync_password_data->hash);
 }
@@ -246,8 +247,8 @@ TEST_F(HashPasswordManagerTest, RetrievingPasswordHashData) {
   ASSERT_TRUE(password_hash_data);
   EXPECT_EQ(8u, password_hash_data->length);
   EXPECT_EQ(16u, password_hash_data->salt.size());
-  uint64_t expected_hash = HashPasswordManager::CalculatePasswordHash(
-      base::UTF8ToUTF16("password"), password_hash_data->salt);
+  uint64_t expected_hash = CalculatePasswordHash(base::UTF8ToUTF16("password"),
+                                                 password_hash_data->salt);
   EXPECT_EQ(expected_hash, password_hash_data->hash);
 
   base::Optional<PasswordHashData> non_existing_data =
@@ -255,28 +256,6 @@ TEST_F(HashPasswordManagerTest, RetrievingPasswordHashData) {
   ASSERT_FALSE(non_existing_data);
 }
 
-TEST_F(HashPasswordManagerTest, CalculatePasswordHash) {
-  const char* kPlainText[] = {"", "password", "password", "secret"};
-  const char* kSalt[] = {"", "salt", "123", "456"};
-
-  constexpr uint64_t kExpectedHash[] = {
-      UINT64_C(0x1c610a7950), UINT64_C(0x1927dc525e), UINT64_C(0xf72f81aa6),
-      UINT64_C(0x3645af77f),
-  };
-
-  static_assert(arraysize(kPlainText) == arraysize(kSalt),
-                "Arrays must have the same size");
-  static_assert(arraysize(kPlainText) == arraysize(kExpectedHash),
-                "Arrays must have the same size");
-
-  for (size_t i = 0; i < arraysize(kPlainText); ++i) {
-    SCOPED_TRACE(i);
-    base::string16 text = base::UTF8ToUTF16(kPlainText[i]);
-    EXPECT_EQ(kExpectedHash[i],
-              HashPasswordManager::CalculatePasswordHash(text, kSalt[i]));
-  }
-}
-
 TEST_F(HashPasswordManagerTest, MigrateCapturedPasswordHash) {
   ASSERT_FALSE(prefs_.HasPrefPath(prefs::kSyncPasswordHash));
   HashPasswordManager hash_password_manager;

components/password_manager/core/browser/password_hash_data.cc

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/password_manager/core/browser/password_hash_data.h"
+
+#include "base/strings/string_piece.h"
+#include "crypto/openssl_util.h"
+#include "crypto/random.h"
+#include "third_party/boringssl/src/include/openssl/evp.h"
+
+namespace password_manager {
+
+namespace {
+
+std::string CreateRandomSalt() {
+  constexpr size_t kSyncPasswordSaltLength = 16;
+
+  char buffer[kSyncPasswordSaltLength];
+  crypto::RandBytes(buffer, kSyncPasswordSaltLength);
+  // Explicit std::string constructor with a string length must be used in order
+  // to avoid treating '\0' symbols as a string ends.
+  std::string result(buffer, kSyncPasswordSaltLength);
+  return result;
+}
+
+}  // namespace
+
+PasswordHashData::PasswordHashData() = default;
+
+PasswordHashData::PasswordHashData(const PasswordHashData& other) = default;
+
+PasswordHashData::PasswordHashData(const std::string& username,
+                                   const base::string16& password,
+                                   bool force_update,
+                                   bool is_gaia_password)
+    : username(username),
+      length(password.size()),
+      salt(CreateRandomSalt()),
+      hash(CalculatePasswordHash(password, salt)),
+      force_update(force_update),
+      is_gaia_password(is_gaia_password) {}
+
+bool PasswordHashData::MatchesPassword(const std::string& username,
+                                       const base::string16& password,
+                                       bool is_gaia_password) const {
+  if (password.size() != this->length || username != this->username ||
+      is_gaia_password != this->is_gaia_password) {
+    return false;
+  }
+
+  return CalculatePasswordHash(password, this->salt) == this->hash;
+}
+
+SyncPasswordData::SyncPasswordData(const base::string16& password,
+                                   bool force_update)
+    : length(password.size()),
+      salt(CreateRandomSalt()),
+      hash(CalculatePasswordHash(password, salt)),
+      force_update(force_update) {}
+
+bool SyncPasswordData::MatchesPassword(const base::string16& password) const {
+  if (password.size() != this->length)
+    return false;
+  return CalculatePasswordHash(password, this->salt) == this->hash;
+}
+
+uint64_t CalculatePasswordHash(const base::StringPiece16& text,
+                               const std::string& salt) {
+  crypto::OpenSSLErrStackTracer err_tracer(FROM_HERE);
+  constexpr size_t kBytesFromHash = 8;
+  constexpr uint64_t kScryptCost = 32;  // It must be a power of 2.
+  constexpr uint64_t kScryptBlockSize = 8;
+  constexpr uint64_t kScryptParallelization = 1;
+  constexpr size_t kScryptMaxMemory = 1024 * 1024;
+
+  uint8_t hash[kBytesFromHash];
+  base::StringPiece text_8bits(reinterpret_cast<const char*>(text.data()),
+                               text.size() * 2);
+  const uint8_t* salt_ptr = reinterpret_cast<const uint8_t*>(salt.c_str());
+
+  int scrypt_ok = EVP_PBE_scrypt(text_8bits.data(), text_8bits.size(), salt_ptr,
+                                 salt.size(), kScryptCost, kScryptBlockSize,
+                                 kScryptParallelization, kScryptMaxMemory, hash,
+                                 kBytesFromHash);
+
+  // EVP_PBE_scrypt can only fail due to memory allocation error (which aborts
+  // Chromium) or invalid parameters. In case of a failure a hash could leak
+  // information from the stack, so using CHECK is better than DCHECK.
+  CHECK(scrypt_ok);
+
+  // Take 37 bits of |hash|.
+  uint64_t hash37 = ((static_cast<uint64_t>(hash[0]))) |
+                    ((static_cast<uint64_t>(hash[1])) << 8) |
+                    ((static_cast<uint64_t>(hash[2])) << 16) |
+                    ((static_cast<uint64_t>(hash[3])) << 24) |
+                    (((static_cast<uint64_t>(hash[4])) & 0x1F) << 32);
+
+  return hash37;
+}
+
+}  // namespace password_manager

components/password_manager/core/browser/password_hash_data.h

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_PASSWORD_HASH_DATA_H_
+#define COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_PASSWORD_HASH_DATA_H_
+
+#include <stdint.h>
+
+#include <string>
+
+#include "base/strings/string16.h"
+#include "base/strings/string_piece_forward.h"
+
+namespace password_manager {
+
+struct PasswordHashData {
+  PasswordHashData();
+  PasswordHashData(const PasswordHashData& other);
+  PasswordHashData(const std::string& username,
+                   const base::string16& password,
+                   bool force_update,
+                   bool is_gaia_password = true);
+  // Returns true iff |*this| represents the credential (|username|,
+  // |password|), also with respect to whether it |is_gaia_password|.
+  bool MatchesPassword(const std::string& username,
+                       const base::string16& password,
+                       bool is_gaia_password) const;
+
+  std::string username;
+  size_t length = 0;
+  std::string salt;
+  uint64_t hash = 0;
+  bool force_update = false;
+  bool is_gaia_password = true;
+};
+
+// SyncPasswordData is being deprecated. Please use PasswordHashData instead.
+struct SyncPasswordData {
+  SyncPasswordData() = default;
+  SyncPasswordData(const SyncPasswordData& other) = default;
+  SyncPasswordData(const base::string16& password, bool force_update);
+  // Returns true iff |*this| represents |password|.
+  bool MatchesPassword(const base::string16& password) const;
+
+  size_t length = 0;
+  std::string salt;
+  uint64_t hash = 0;
+  // Signal that we need to update password hash, salt, and length in profile
+  // prefs.
+  bool force_update = false;
+};
+
+// Calculates 37 bits hash for a password. The calculation is based on a slow
+// hash function. The running time is ~10^{-4} seconds on Desktop.
+uint64_t CalculatePasswordHash(const base::StringPiece16& text,
+                               const std::string& salt);
+
+}  // namespace password_manager
+
+#endif  // COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_PASSWORD_HASH_DATA_H_

components/password_manager/core/browser/password_hash_data_unittest.cc

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/password_manager/core/browser/password_hash_data.h"
+
+#include "base/stl_util.h"
+#include "base/strings/utf_string_conversions.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace password_manager {
+namespace {
+
+TEST(PasswordHashDataTest, CalculatePasswordHash) {
+  const char* kPlainText[] = {"", "password", "password", "secret"};
+  const char* kSalt[] = {"", "salt", "123", "456"};
+
+  constexpr uint64_t kExpectedHash[] = {
+      UINT64_C(0x1c610a7950), UINT64_C(0x1927dc525e), UINT64_C(0xf72f81aa6),
+      UINT64_C(0x3645af77f),
+  };
+
+  static_assert(base::size(kPlainText) == base::size(kSalt),
+                "Arrays must have the same size");
+  static_assert(base::size(kPlainText) == base::size(kExpectedHash),
+                "Arrays must have the same size");
+
+  for (size_t i = 0; i < base::size(kPlainText); ++i) {
+    SCOPED_TRACE(i);
+    base::string16 text = base::UTF8ToUTF16(kPlainText[i]);
+    EXPECT_EQ(kExpectedHash[i], CalculatePasswordHash(text, kSalt[i]));
+  }
+}
+
+}  // namespace
+}  // namespace password_manager

components/password_manager/core/browser/password_reuse_detector.cc

@@ -9,6 +9,7 @@
 
 #include "components/autofill/core/common/password_form.h"
 #include "components/password_manager/core/browser/hash_password_manager.h"
+#include "components/password_manager/core/browser/password_hash_data.h"
 #include "components/password_manager/core/browser/password_reuse_detector_consumer.h"
 #include "components/password_manager/core/browser/psl_matching_helper.h"
 #include "components/safe_browsing/common/safe_browsing_prefs.h"
@@ -49,8 +50,8 @@ base::Optional<PasswordHashData> FindPasswordReuse(
     base::string16 reuse_candidate = input.substr(offset);
     // It is possible that input matches multiple passwords in the list,
     // we only return the first match due to simplicity.
-    if (HashPasswordManager::CalculatePasswordHash(
-            reuse_candidate, hash_data.salt) == hash_data.hash &&
+    if (CalculatePasswordHash(reuse_candidate, hash_data.salt) ==
+            hash_data.hash &&
         hash_data.length > longest_match_size) {
       longest_match_size = hash_data.length;
       longest_match = hash_data;