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..390e6c09d4927043539c3839c14870e791e0ed1d 100644
--- a/chrome/browser/signin/local_auth.cc
+++ b/chrome/browser/signin/local_auth.cc
@@ -22,21 +22,65 @@
 
 namespace {
 
+struct HashEncoding {

[bcwhite, 2015/01/28 15:34:12]

I'm not sure it's worth the trouble to create an array of information like this.
 You're making assumptions about the future that everything will fit into this
model.  Plus, there will likely never be more than 2 encoding supported at any
one time: the current and the previous (for upgrade purposes).

Given the security aspects of this, there is certain benefit to clearly
delineating each encoding method such that it can easily be evaluated.

[Mike Lerman, 2015/01/28 15:38:38]

We'll always need to support older versions. I may lock my Chrome and not come
back to it for a while - perhaps several releases and iterations later (visiting
a relatives'... whatever). I'll always need to backward support all releases. If
things need to be added that don't fit this model, we'll create more variables,
or cross that bridge when we come to it.

Having a nice structure makes the rest of the code much easier, other than
having little if statements everywhere. This is much more readable down below.

As for evaluating each encoding method as a unit, I agree, and that's why I kept
each block of constants defined, going so far as to defined kHash2Bits = 256,
even though it's the same as kHash1Bits. That way, you can easily review Hash2
as a complete encoding entity.

[bcwhite, 2015/01/28 16:01:35]

Not true.  The locally saved hash is required only for offline authentication. 
In the case of not logging in over the span of months, falling back to online
authentication is not unreasonable.  In the other case of upgrading Chrome by
multiple versions then it's also reasonable.

I'd rather have a few *big* if statements so an auditor can clearly follow
exactly what Encoding-v2 does without having to constantly reference how a
structure was datafilled.  It also makes it easier to remove older encodings
that are no longer needed.

[Mike Lerman, 2015/01/28 16:45:55]

The way an auditor would reference each encoding's configuration is identical!
In either case, the details are stored in a big block of comments, either lines
51-54 or lines 58-63, depending on your desired encoding.

I'd still rather keep the door open for backward compatibility. We're going to
the effort to support the offline auth case, let's do it properly. There's no
gain, at this point, in drawing an arbitrary line to say we'll backward support
1 version but not 0 or 2. Especially since some channels upgrade more rapidly -
if we run into a problem with an encoding and need to modify it in a subsequent
release, our canary and dev channels could get burned. I just don't see a
technical reason to not, at least right now, build in multiple version - it
benefits our users and is, at this point, still maintainable.

An unrelated point is that having this data structure makes the rest of the code
much, much more readable and also more maintainable (future encodings won't need
any changes to the body of the code, unless they include more features).

+  char version;
+  unsigned hash_bits;
+  unsigned hash_bytes;
+  unsigned iteration_count;
+  unsigned stored_bits;
+  unsigned stored_bytes;
+
+  public:
+   HashEncoding(char version,
+                unsigned hash_bits,
+                unsigned hash_bytes,
+                unsigned iteration_count,
+                unsigned stored_bits,
+                unsigned stored_bytes) :
+      version(version),
+      hash_bits(hash_bits),
+      hash_bytes(hash_bytes),
+      iteration_count(iteration_count),
+      stored_bits(stored_bits),
+      stored_bytes(stored_bytes) {}
+};
 // 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';
+// these safely, add a new HashEncoding object below and increment
+// NUM_HASH_ENCODINGS.
+const char kHash1Version = '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 char kHash2Version = '2';
+const unsigned kHash2Bits = 256;
+const unsigned kHash2Bytes = kHash2Bits / 8;
+const unsigned kHash2IterationCount = 100000;
+const unsigned kHash2StoredBits = 13;
+const unsigned kHash2StoredBytes = (kHash2StoredBits + 7) / 8;
+
+const int NUM_HASH_ENCODINGS = 2;
+HashEncoding encodings[NUM_HASH_ENCODINGS] = {
+  HashEncoding(
+      kHash1Version, kHash1Bits, kHash1Bytes, kHash1IterationCount, 0, 0),
+  HashEncoding(
+      kHash2Version, kHash2Bits, kHash2Bytes, kHash2IterationCount,
+          kHash2StoredBits, kHash2StoredBytes)
+};
+
+const HashEncoding* GetEncodingForVersion(char version) {
+  // Note that versions are 1-indexed.
+  DCHECK(version > '0' && version <= ('0' + NUM_HASH_ENCODINGS));
+  return &encodings[(version - '0') - 1];
+}
+
 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.
-
+                                     const HashEncoding& encoding) {
+  DCHECK_EQ(encoding.hash_bytes, salt.length());
   base::Time start_time = base::Time::Now();
 
   // Library call to create secure password hash as SymmetricKey (uses PBKDF2).
@@ -44,22 +88,27 @@ std::string CreateSecurePasswordHash(const std::string& salt,
       crypto::SymmetricKey::DeriveKeyFromPassword(
           crypto::SymmetricKey::AES,
           password, salt,
-          kHash1IterationCount, kHash1Bits));
+          encoding.iteration_count, encoding.hash_bits));
   std::string password_hash;
   const bool success = password_key->GetRawKey(&password_hash);
   DCHECK(success);
-  DCHECK_EQ(kHash1Bytes, password_hash.length());
+  DCHECK_EQ(encoding.hash_bytes, password_hash.length());
 
   UMA_HISTOGRAM_TIMES("PasswordHash.CreateTime",
                       base::Time::Now() - start_time);
 
+  if (encoding.stored_bits) {
+    password_hash = chrome::TruncateStringByBits(
+        password_hash, encoding.stored_bits);
+    DCHECK_EQ(encoding.stored_bytes, password_hash.length());
+  }
+  DCHECK_EQ(encoding.stored_bytes ? encoding.stored_bytes : encoding.hash_bytes,
+            password_hash.length());
   return password_hash;
 }
 
 std::string EncodePasswordHashRecord(const std::string& record,
-                                     char encoding) {
-  DCHECK_EQ(kHash1Encoding, encoding);  // Currently support only one method.
-
+                                     const HashEncoding& encoding) {
   // Encrypt the hash using the OS account-password protection (if available).
   std::string encoded;
   const bool success = OSCrypt::EncryptString(record, &encoded);
@@ -70,7 +119,7 @@ std::string EncodePasswordHashRecord(const std::string& record,
   base::Base64Encode(encoded, &encoded64);
 
   // Stuff the "encoding" value into the first byte.
-  encoded64.insert(0, &encoding, sizeof(encoding));
+  encoded64.insert(0, &encoding.version, sizeof(encoding.version));
 
   return encoded64;
 }
@@ -82,7 +131,7 @@ bool DecodePasswordHashRecord(const std::string& encoded,
   if (encoded.length() < 1)
     return false;
   *encoding = encoded[0];
-  if (*encoding != kHash1Encoding)
+  if (!GetEncodingForVersion(*encoding))
     return false;
 
   // Stored record is base64; convert to binary.
@@ -106,6 +155,21 @@ size_t GetProfileInfoIndexOfProfile(const Profile* profile) {
 
 namespace chrome {
 
+std::string TruncateStringByBits(const std::string& str,
+                                 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 = (1 << (len_bits % 8)) - 1;
+  truncated_string[number_bytes - 1] &= last_char_bitmask;
+  return truncated_string;
+}
+
 void RegisterLocalAuthPrefs(user_prefs::PrefRegistrySyncable* registry) {
   registry->RegisterStringPref(
       prefs::kGoogleServicesPasswordHash,
@@ -113,24 +177,21 @@ void RegisterLocalAuthPrefs(user_prefs::PrefRegistrySyncable* registry) {
       user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
 }
 
-void SetLocalAuthCredentials(size_t info_index,
-                             const std::string& password) {
-  if (info_index == std::string::npos) {
-    NOTREACHED();
-    return;
-  }
-  DCHECK(password.length());
+void SetLocalAuthCredentialsWithEncoding(size_t info_index,
+                                         const std::string& password,
+                                         char encoding_version) {
+  const HashEncoding& encoding = encodings[(encoding_version - '0') - 1];
 
   // 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());
+  crypto::RandBytes(WriteInto(&salt_str, encoding.hash_bytes + 1),
+                              encoding.hash_bytes);
+  DCHECK_EQ(encoding.hash_bytes, salt_str.length());

[jww, 2015/01/29 11:44:45]

This seems to me like an unnecessary DCHECK. This is testing a property that the
API of crypto::RandBytes promises, so if anyone is going to DCHECK this, it
should be crypto::RandBytes, not the caller.

[Mike Lerman, 2015/01/29 16:57:42]

Done. I ain't touching crypto:: anything :)

 
   // Perform secure hash of password for storage.
   std::string password_hash = CreateSecurePasswordHash(
-      salt_str, password, kHash1Encoding);
-  DCHECK_EQ(kHash1Bytes, password_hash.length());
+      salt_str, password, encoding);
 
   // Group all fields into a single record for storage;
   std::string record;
@@ -138,12 +199,23 @@ void SetLocalAuthCredentials(size_t info_index,
   record.append(password_hash);
 
   // Encode it and store it.
-  std::string encoded = EncodePasswordHashRecord(record, kHash1Encoding);
+  std::string encoded = EncodePasswordHashRecord(record, encoding);
   ProfileInfoCache& info =
       g_browser_process->profile_manager()->GetProfileInfoCache();
   info.SetLocalAuthCredentialsOfProfileAtIndex(info_index, encoded);
 }
 
+void SetLocalAuthCredentials(size_t info_index,
+                             const std::string& password) {
+  if (info_index == std::string::npos) {
+    NOTREACHED();
+    return;
+  }
+  DCHECK(password.length());
+  SetLocalAuthCredentialsWithEncoding(
+      info_index, password, '0' + NUM_HASH_ENCODINGS);
+}
+
 void SetLocalAuthCredentials(const Profile* profile,
                              const std::string& password) {
   SetLocalAuthCredentials(GetProfileInfoIndexOfProfile(profile), password);
@@ -174,22 +246,27 @@ bool ValidateLocalAuthCredentials(size_t info_index,
   const char* password_check;
   size_t password_length;
 
-  if (encoding == '1') {
-    // Validate correct length; extract salt and password hash.
-    if (record.length() != 2 * kHash1Bytes)
-      return false;
-    std::string salt_str(record.data(), kHash1Bytes);
-    password_saved = record.data() + kHash1Bytes;
-    password_hash = CreateSecurePasswordHash(salt_str, password, encoding);
-    password_check = password_hash.data();
-    password_length = kHash1Bytes;
-  } else {
-    // unknown encoding
+  const HashEncoding* hash_encoding = GetEncodingForVersion(encoding);
+  if (!hash_encoding) {
+    // Unknown encoding.
     return false;
   }
 
-  return crypto::SecureMemEqual(password_saved, password_check,
-                                password_length);
+  // Extract salt.
+  std::string salt_str(record.data(), hash_encoding->hash_bytes);
+  // Extract password.
+  password_saved = record.data() + hash_encoding->hash_bytes;
+  password_hash = CreateSecurePasswordHash(salt_str, password, *hash_encoding);
+  password_length = hash_encoding->stored_bytes;
+  password_check = password_hash.data();
+
+  bool passwords_match = crypto::SecureMemEqual(
+      password_saved, password_check, password_length);
+
+  // Update the stored credentials to the latest encoding if necessary.
+  if (passwords_match && (hash_encoding->version - '0') != NUM_HASH_ENCODINGS)
+    SetLocalAuthCredentials(info_index, password);
+  return passwords_match;
 }
 
 bool ValidateLocalAuthCredentials(const Profile* profile,