Only store leading 13 bits of password hash.

chrome/browser/signin/local_auth.cc

 // Copyright 2013 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 "chrome/browser/signin/local_auth.h"
 
 #include "base/base64.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/metrics/histogram.h"
@@ skipping 13 matching lines @@
 
 // WARNING: Changing these values will make it impossible to do off-line
 // authentication until the next successful on-line authentication.  To change
 // these safely, change the "encoding" version below and make verification
 // handle multiple values.
 const char kHash1Encoding = '1';
 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) {
   DCHECK_EQ(kHash1Bytes, salt.length());
   DCHECK_EQ(kHash1Encoding, encoding);  // Currently support only one method.
 
   base::Time start_time = base::Time::Now();
 
   // Library call to create secure password hash as SymmetricKey (uses PBKDF2).
   scoped_ptr<crypto::SymmetricKey> password_key(
@@ skipping 80 matching lines @@
   // Salt should be random data, as long as the hash length, and different with
   // every save.
   std::string salt_str;
   crypto::RandBytes(WriteInto(&salt_str, kHash1Bytes + 1), kHash1Bytes);
   DCHECK_EQ(kHash1Bytes, salt_str.length());
 
   // Perform secure hash of password for storage.
   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;
   record.append(salt_str);
   record.append(password_hash);
 
   // Encode it and store it.
   std::string encoded = EncodePasswordHashRecord(record, kHash1Encoding);
   ProfileInfoCache& info =
       g_browser_process->profile_manager()->GetProfileInfoCache();
   info.SetLocalAuthCredentialsOfProfileAtIndex(info_index, encoded);
 }
 
 void SetLocalAuthCredentials(const Profile* profile,
                              const std::string& password) {
   SetLocalAuthCredentials(GetProfileInfoIndexOfProfile(profile), password);
 }
 
 bool ValidateLocalAuthCredentials(size_t info_index,
                                   const std::string& password) {
   if (info_index == std::string::npos) {
     NOTREACHED();
     return false;
   }
 
   std::string record;
   char encoding;
+  bool update_stored_credential = false;
 
   ProfileInfoCache& info =
       g_browser_process->profile_manager()->GetProfileInfoCache();
 
   std::string encodedhash =
       info.GetLocalAuthCredentialsOfProfileAtIndex(info_index);
   if (encodedhash.length() == 0 && password.length() == 0)
     return true;
   if (!DecodePasswordHashRecord(encodedhash, &record, &encoding))
     return false;
 
   std::string password_hash;
   const char* password_saved;
   const char* password_check;
   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,
                                   const std::string& password) {
   return ValidateLocalAuthCredentials(GetProfileInfoIndexOfProfile(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