Fix the hash generation algorithm to be consistent with prior implementation.

chrome/browser/prefs/pref_hash_calculator.cc

Index: chrome/browser/prefs/pref_hash_calculator.cc
diff --git a/chrome/browser/prefs/pref_hash_calculator.cc b/chrome/browser/prefs/pref_hash_calculator.cc
index 4a787a8a1abb699d8a3447c3a34caf3eee638bb8..80be10198b33cd91cfba8036be91cd2a8662581c 100644
--- a/chrome/browser/prefs/pref_hash_calculator.cc
+++ b/chrome/browser/prefs/pref_hash_calculator.cc
@@ -16,6 +16,31 @@
 
 namespace {
 
+// Calculates an HMAC of |message| using |key|, encoded as a hexadecimal string.
+std::string GetDigestString(const std::string& key,
+                            const std::string& message) {
+  crypto::HMAC hmac(crypto::HMAC::SHA256);
+  std::vector<uint8> digest(hmac.DigestLength());
+  if (!hmac.Init(key) || !hmac.Sign(message, &digest[0], digest.size())) {

[erikwright (departed), 2014/01/10 15:20:02]

After discussion with Ryan, I understand the following:

Essentially any cryptographic operation can fail at any time, either due to
invalid parameters or system configuration. Configuration problems would be
persistent barring configuration changes. Invalid parameters mean that one
operation could fail (repeatably with the same inputs) while a subsequent
operation with different inputs might succeed.

Untrusted parameters could be exploited to cause an operation to fail, in
particular if they exceed size limits. Our only untrusted parameter is the
preference value. If, somehow, it prevented verification of a signature we would
discard the preference value anyway, so we would ideally just prevent a settings
change to a value that is incompatible with our cryptographic operations. On the
other hand, if we allow it to be set, it will be interpreted as an external
change on next launch.

If a configuration failure were to occur, however, we would neither be able to
store, nor verify, any MACs. All tracked preferences would appear to have been
externally changed at next launch.

We cannot distinguish a system failure from an invalid hash as both would lead
to a 'false' return from Verify.

In the face of a failure to Verify, then, we must assume the value is changed.
In the face of that, there is little harm of storing an empty MAC when an
operation fails while a preference is updated.

I suggest to DLOG(FATAL) for any failure from Init or Sign so that we catch
programming errors on bots.

[Bernhard Bauer, 2014/01/10 15:51:57]

NOTREACHED() is fine with me.

+    NOTREACHED();
+    return std::string();
+  }
+  return base::HexEncode(digest.data(), digest.size());
+}
+
+// Validates an HMAC of |message| using |key|, encoded as a hexadecimal string.

[gab, 2014/01/10 15:56:45]

Seems like you just took GetDigestString's comment and changed the verb for
"Validates"; please update this comment (in particular add a section for
|digest_string|).

[erikwright (departed), 2014/01/10 16:10:34]

I did, indeed. This method is GetDigestString with the action of "Get" replaced
by "Validate" so it makes sense that the comments would be analogously similar.

But I understand that you would like the string '|digest_string|' to appear in
the comments, so I have updated it. Let me know if you had something else in
mind.

+bool VerifyDigestString(const std::string& key,
+                        const std::string& message,
+                        const std::string& digest_string) {
+  crypto::HMAC hmac(crypto::HMAC::SHA256);
+  std::vector<uint8> digest;
+  return base::HexStringToBytes(digest_string, &digest) &&
+      hmac.Init(key) &&
+      hmac.Verify(message,
+                  base::StringPiece(reinterpret_cast<char*>(&digest[0]),
+                                    digest.size()));
+}
+
 // Renders |value| as a string. |value| may be NULL, in which case the result
 // is an empty string.
 std::string ValueAsString(const base::Value* value) {
@@ -38,53 +63,63 @@ std::string ValueAsString(const base::Value* value) {
 }
 
 // Common helper for all hash algorithms.
-std::string CalculateFromValueAndComponents(
-    const std::string& seed,
+std::string GetMessageFromValueAndComponents(
     const base::Value* value,
     const std::vector<std::string>& extra_components) {
-  static const size_t kSHA256DigestSize = 32;
+  return JoinString(extra_components, "") + ValueAsString(value);
+}
 
-  std::string message = JoinString(extra_components, "") + ValueAsString(value);
 
-  crypto::HMAC hmac(crypto::HMAC::SHA256);
-  unsigned char digest[kSHA256DigestSize];
-  if (!hmac.Init(seed) || !hmac.Sign(message, digest, arraysize(digest))) {
-    NOTREACHED();
+// Generates a device ID based on the input device ID. The derived device ID has
+// no useful properties beyond those of the input device ID except that it is
+// consistent with previous implementations.
+std::string GenerateDeviceIdLikePrefMetricsServiceDid(
+    const std::string& original_device_id) {
+  if (original_device_id.empty())
     return std::string();
-  }
+  return StringToLowerASCII(
+      GetDigestString(original_device_id, "PrefMetricsService"));
+}
 
-  return base::HexEncode(digest, arraysize(digest));
+// Verifies a hash using a deprecated hash algorithm. For validating old
+// hashes during migration.
+bool VerifyLegacyHash(const std::string& seed,
+                      const base::Value* value,
+                      const std::string& digest_string) {
+  return VerifyDigestString(
+      seed,
+      GetMessageFromValueAndComponents(value, std::vector<std::string>()),
+      digest_string);
 }
 
 }  // namespace
 
 PrefHashCalculator::PrefHashCalculator(const std::string& seed,
                                        const std::string& device_id)
-    : seed_(seed), device_id_(device_id) {}
+    : seed_(seed),
+      device_id_(GenerateDeviceIdLikePrefMetricsServiceDid(device_id)) {}
 
 std::string PrefHashCalculator::Calculate(const std::string& path,
                                           const base::Value* value) const {
+  return GetDigestString(seed_, GetMessage(path, value));
+}
+
+std::string PrefHashCalculator::GetMessage(const std::string& path,

[gab, 2014/01/10 15:56:45]

Put GetMessage() impl at bottom of this file to match header ordering.

[erikwright (departed), 2014/01/10 16:10:34]

Done.

+                                           const base::Value* value) const {
   std::vector<std::string> components;
   if (!device_id_.empty())
     components.push_back(device_id_);
   components.push_back(path);
-  return CalculateFromValueAndComponents(seed_, value, components);
+  return GetMessageFromValueAndComponents(value, components);
 }
 
 PrefHashCalculator::ValidationResult PrefHashCalculator::Validate(
     const std::string& path,
     const base::Value* value,
-    const std::string& hash) const {
-  if (hash == Calculate(path, value))
+    const std::string& digest_string) const {
+  if (VerifyDigestString(seed_, GetMessage(path, value), digest_string))
     return VALID;
-  if (hash == CalculateLegacyHash(path, value))
+  if (VerifyLegacyHash(seed_, value, digest_string))
     return VALID_LEGACY;
   return INVALID;
 }
-
-std::string PrefHashCalculator::CalculateLegacyHash(
-    const std::string& path, const base::Value* value) const {
-  return CalculateFromValueAndComponents(seed_,
-                                         value,
-                                         std::vector<std::string>());
-}