Only store leading 13 bits of password hash.

chrome/browser/signin/local_auth.cc

Index: chrome/browser/signin/local_auth.cc
diff --git a/chrome/browser/signin/local_auth.cc b/chrome/browser/signin/local_auth.cc
index 7748898f8e7a2c08decd9f033792208cb5f62c9c..c1db0ad86d205ccdb31d74d54341bda19c8f45e6 100644
--- a/chrome/browser/signin/local_auth.cc
+++ b/chrome/browser/signin/local_auth.cc
@@ -31,6 +31,11 @@ const unsigned kHash1Bits = 256;
 const unsigned kHash1Bytes = kHash1Bits / 8;
 const unsigned kHash1IterationCount = 100000;
 
+// Store 13 bits to provide pin-like security (8192 possible values), without
+// providing a complete oracle for the user's GAIA password.
+const unsigned kHash1StoredBits = 13;

[bcwhite, 2015/01/21 21:04:40]

"Hash2"  This is storage method #2.  Use the same names as above (where
applicable) but change "1" to "2".

[Mike Lerman, 2015/01/27 21:02:26]

Done.

+const unsigned kHash1StoredBytes = (kHash1StoredBits + 7) / 8;
+
 std::string CreateSecurePasswordHash(const std::string& salt,
                                      const std::string& password,
                                      char encoding) {
@@ -131,6 +136,7 @@ void SetLocalAuthCredentials(size_t info_index,
   std::string password_hash = CreateSecurePasswordHash(
       salt_str, password, kHash1Encoding);
   DCHECK_EQ(kHash1Bytes, password_hash.length());
+  password_hash = TruncateStringByBits(password_hash, kHash1StoredBits);
 
   // Group all fields into a single record for storage;
   std::string record;
@@ -158,6 +164,7 @@ bool ValidateLocalAuthCredentials(size_t info_index,
 
   std::string record;
   char encoding;
+  bool update_stored_credential = false;
 
   ProfileInfoCache& info =
       g_browser_process->profile_manager()->GetProfileInfoCache();
@@ -175,21 +182,33 @@ bool ValidateLocalAuthCredentials(size_t info_index,
   size_t password_length;
 
   if (encoding == '1') {

[bcwhite, 2015/01/21 21:04:40]

The handling of v1 encoding should never change.  New saves need to use a new
encoding.

[Mike Lerman, 2015/01/27 21:02:26]

Done.

-    // Validate correct length; extract salt and password hash.
-    if (record.length() != 2 * kHash1Bytes)
-      return false;
+    // Extract salt.
     std::string salt_str(record.data(), kHash1Bytes);
     password_saved = record.data() + kHash1Bytes;
-    password_hash = CreateSecurePasswordHash(salt_str, password, encoding);
+    // Extract password hash.
+    if (record.length() == 2 * kHash1Bytes) {
+      password_hash = CreateSecurePasswordHash(salt_str, password, encoding);
+      password_length = kHash1Bytes;
+      update_stored_credential = true;
+    } else if (record.length() == kHash1Bytes + kHash1StoredBytes) {
+      password_hash = CreateSecurePasswordHash(salt_str, password, encoding);

[jww, 2015/01/22 21:48:18]

Given bcwhite's comment about never changing the handling of v1 encodings (which
seems correct to me as well), it might be clearer if CreateSecurePasswordHash()
uses the passed in encoding type and, for v2 encodings (that is, your new 13 bit
hash value) just calls TruncateStringByBits() itself. Then, either way, you get
the a password_hash value that you handle the same way.

[bcwhite, 2015/01/23 01:28:38]

I didn't pass an encoding version to "create" because I assumed that Chrome
would always be able to create only one type -- the latest.  This could be
changed if necessary but it allows ripping out code that is no longer used.

[jww, 2015/01/26 18:31:51]

CreateSecurePasswordHash() is used to validate the hash (as well as to store it
in the first place). And, as you pointed out before, we have to keep the v1
validation for those with old hash versions lying around anyway, so we'll need
to handle CreateSecurePasswordHash() for both v1 and v2 anyway, it's just a
matter of if that's done surrounding the calls to CreateSecurePasswordHash or
internally. It seems cleaner to me to do it inside CreateSecurePasswordHash, but
I certainly don't feel strongly about it.

[bcwhite, 2015/01/27 18:02:25]

I guess I always imagined that a new method would involve a significantly
different function.  No problem passing it though since this one is so similar.

[Mike Lerman, 2015/01/27 21:02:26]

Ok, CreateSecurePasswordHash will call Truncate.

+      password_hash = TruncateStringByBits(password_hash, kHash1StoredBits);
+      password_length = kHash1StoredBytes;
+    } else {
+      // Unknown record length.
+      return false;
+    }
     password_check = password_hash.data();
-    password_length = kHash1Bytes;
   } else {
-    // unknown encoding
+    // Unknown encoding.
     return false;
   }
 
-  return crypto::SecureMemEqual(password_saved, password_check,
-                                password_length);
+  bool passwords_match = crypto::SecureMemEqual(
+      password_saved, password_check, password_length);
+  if (passwords_match && update_stored_credential)
+    SetLocalAuthCredentials(info_index, password);
+  return passwords_match;
 }
 
 bool ValidateLocalAuthCredentials(const Profile* profile,
@@ -198,4 +217,19 @@ bool ValidateLocalAuthCredentials(const Profile* profile,
                                       password);
 }
 
+std::string TruncateStringByBits(const std::string& str,

[bcwhite, 2015/01/21 21:04:40]

Rest of the file calls upward so put this function higher up, before the call to
it.

[Mike Lerman, 2015/01/27 21:02:26]

Done.

+                                 const size_t len_bits) {
+  if (len_bits % 8 == 0)
+    return str.substr(0, len_bits / 8);
+
+  // The initial truncation copies whole bytes
+  int number_bytes = (len_bits + 7) / 8;
+  std::string truncated_string = str.substr(0, number_bytes);
+
+  // Keep the prescribed number of bits from the last byte.
+  unsigned last_char_bitmask = pow(2, len_bits % 8) - 1;

[bcwhite, 2015/01/21 21:04:40]

pow(2, len_bits % 8)  ==  (1 << (len_bits % 8))

[Mike Lerman, 2015/01/27 21:02:26]

Done.

+  truncated_string[number_bytes - 1] &= last_char_bitmask;
+  return truncated_string;
+}
+
 }  // namespace chrome