Enforce variations signature verification.

chrome/browser/metrics/variations/variations_seed_store.cc

Index: chrome/browser/metrics/variations/variations_seed_store.cc
===================================================================
--- chrome/browser/metrics/variations/variations_seed_store.cc	(revision 254843)
+++ chrome/browser/metrics/variations/variations_seed_store.cc	(working copy)
@@ -18,13 +18,6 @@
 
 namespace {
 
-// Computes a hash of the serialized variations seed data.
-// TODO(asvitkine): Remove this once the seed signature is ubiquitous.
-std::string HashSeed(const std::string& seed_data) {
-  const std::string sha1 = base::SHA1HashString(seed_data);
-  return base::HexEncode(sha1.data(), sha1.size());
-}
-
 // Signature verification is disabled on mobile platforms for now, since it
 // adds about ~15ms to the startup time on mobile (vs. a couple ms on desktop).
 bool SignatureVerificationEnabled() {
@@ -66,49 +59,11 @@
 };
 
 // Note: UMA histogram enum - don't re-order or remove entries.
-enum VariationSeedSignatureState {
-  VARIATIONS_SEED_SIGNATURE_MISSING,
-  VARIATIONS_SEED_SIGNATURE_DECODE_FAILED,
-  VARIATIONS_SEED_SIGNATURE_INVALID_SIGNATURE,
-  VARIATIONS_SEED_SIGNATURE_INVALID_SEED,
-  VARIATIONS_SEED_SIGNATURE_VALID,
-  VARIATIONS_SEED_SIGNATURE_ENUM_SIZE,
-};
-
-// Verifies a variations seed (the serialized proto bytes) with the specified
-// base-64 encoded signate that was received from the server and returns the
-// result. The signature is assumed to be an "ECDSA with SHA-256" signature
-// (see kECDSAWithSHA256AlgorithmID above).
-VariationSeedSignatureState VerifySeedSignature(
-    const std::string& seed_bytes,
-    const std::string& base64_seed_signature) {
-  if (base64_seed_signature.empty())
-    return VARIATIONS_SEED_SIGNATURE_MISSING;
-
-  std::string signature;
-  if (!base::Base64Decode(base64_seed_signature, &signature))
-    return VARIATIONS_SEED_SIGNATURE_DECODE_FAILED;
-
-  crypto::SignatureVerifier verifier;
-  if (!verifier.VerifyInit(
-          kECDSAWithSHA256AlgorithmID, sizeof(kECDSAWithSHA256AlgorithmID),
-          reinterpret_cast<const uint8*>(signature.data()), signature.size(),
-          kPublicKey, arraysize(kPublicKey))) {
-    return VARIATIONS_SEED_SIGNATURE_INVALID_SIGNATURE;
-  }
-
-  verifier.VerifyUpdate(reinterpret_cast<const uint8*>(seed_bytes.data()),
-                        seed_bytes.size());
-  if (verifier.VerifyFinal())
-    return VARIATIONS_SEED_SIGNATURE_VALID;
-  return VARIATIONS_SEED_SIGNATURE_INVALID_SEED;
-}
-
-// Note: UMA histogram enum - don't re-order or remove entries.
 enum VariationSeedEmptyState {
   VARIATIONS_SEED_NOT_EMPTY,
   VARIATIONS_SEED_EMPTY,
   VARIATIONS_SEED_CORRUPT,
+  VARIATIONS_SEED_INVALID_SIGNATURE,
   VARIATIONS_SEED_EMPTY_ENUM_SIZE,
 };
 
@@ -134,12 +89,9 @@
     return false;
   }
 
-  const std::string hash_from_pref =
-      local_state_->GetString(prefs::kVariationsSeedHash);
   // If the decode process fails, assume the pref value is corrupt and clear it.
   std::string seed_data;
   if (!base::Base64Decode(base64_seed_data, &seed_data) ||
-      (!hash_from_pref.empty() && HashSeed(seed_data) != hash_from_pref) ||
       !seed->ParseFromString(seed_data)) {
     VLOG(1) << "Variations seed data in local pref is corrupt, clearing the "
             << "pref.";
@@ -148,13 +100,20 @@
     return false;
   }
 
-  if (SignatureVerificationEnabled()) {
-    const std::string base64_seed_signature =
-        local_state_->GetString(prefs::kVariationsSeedSignature);
-    const VariationSeedSignatureState signature_state =
-        VerifySeedSignature(seed_data, base64_seed_signature);
-    UMA_HISTOGRAM_ENUMERATION("Variations.LoadSeedSignature", signature_state,
+  const std::string base64_seed_signature =
+      local_state_->GetString(prefs::kVariationsSeedSignature);
+  const VerifySignatureResult result =
+      VerifySeedSignature(seed_data, base64_seed_signature);
+  if (result != VARIATIONS_SEED_SIGNATURE_ENUM_SIZE) {
+    UMA_HISTOGRAM_ENUMERATION("Variations.LoadSeedSignature", result,
                               VARIATIONS_SEED_SIGNATURE_ENUM_SIZE);
+    if (result != VARIATIONS_SEED_SIGNATURE_VALID) {
+      VLOG(1) << "Variations seed signature in local pref missing or invalid "

[jwd, 2014/03/05 17:56:27]

Do these need to be VLOGs?

[Alexei Svitkine (slow), 2014/03/05 18:21:43]

I've followed the convention that's used in this file already. What alternative
do you propose?

1. Change them to LOG?
2. Change them to DVLOG?
3. Not have these at all - and let users debug via
chrome://histograms/Variations?
4. Something else?

[jwd, 2014/03/05 18:36:00]

Ah, didn't notice the convention. I was thinking DVLOG.

+              << "with result: " << result << ". Clearing the pref.";
+      ClearPrefs();
+      RecordVariationSeedEmptyHistogram(VARIATIONS_SEED_INVALID_SIGNATURE);
+      return false;
+    }
   }
 
   variations_serial_number_ = seed->serial_number();
@@ -179,18 +138,26 @@
     return false;
   }
 
-  if (SignatureVerificationEnabled()) {
-    const VariationSeedSignatureState signature_state =
-        VerifySeedSignature(seed_data, base64_seed_signature);
-    UMA_HISTOGRAM_ENUMERATION("Variations.StoreSeedSignature", signature_state,
+  const VerifySignatureResult result =
+      VerifySeedSignature(seed_data, base64_seed_signature);
+  if (result != VARIATIONS_SEED_SIGNATURE_ENUM_SIZE) {
+    UMA_HISTOGRAM_ENUMERATION("Variations.StoreSeedSignature", result,
                               VARIATIONS_SEED_SIGNATURE_ENUM_SIZE);
+    if (result != VARIATIONS_SEED_SIGNATURE_VALID) {
+      VLOG(1) << "Variations seed signature missing or invalid with result: "
+              << result << ". Rejecting the seed.";
+      return false;
+    }
   }
 
   std::string base64_seed_data;
   base::Base64Encode(seed_data, &base64_seed_data);
 
+  // TODO(asvitkine): This pref is no longer being used. Remove it completely
+  // in a couple of releases.
+  local_state_->ClearPref(prefs::kVariationsSeedHash);
+
   local_state_->SetString(prefs::kVariationsSeed, base64_seed_data);
-  local_state_->SetString(prefs::kVariationsSeedHash, HashSeed(seed_data));
   local_state_->SetInt64(prefs::kVariationsSeedDate,
                          date_fetched.ToInternalValue());
   local_state_->SetString(prefs::kVariationsSeedSignature,
@@ -216,4 +183,33 @@
   local_state_->ClearPref(prefs::kVariationsSeedSignature);
 }
 
+VariationsSeedStore::VerifySignatureResult
+VariationsSeedStore::VerifySeedSignature(

[jwd, 2014/03/05 17:56:27]

Can this be tested? What about having a testing key pair or having a pre-signed
base-64 blob in the test?

[Alexei Svitkine (slow), 2014/03/05 18:21:43]

A testing key pair would require passing the public key as a param and also
writing C++ signing code, so I'd rather not go that route. We can add some
base-64 blobs to the test though (one for the payload and one for the
signature). I'm okay doing that, though I'll probably need to generate something
smaller than the current variations seed.

[jwd, 2014/03/05 18:36:00]

Base-64 blob seems fine then.

[Alexei Svitkine (slow), 2014/03/06 18:16:07]

Done. Added checking for all the possible return values.

+    const std::string& seed_bytes,
+    const std::string& base64_seed_signature) {
+  if (!SignatureVerificationEnabled())
+    return VARIATIONS_SEED_SIGNATURE_ENUM_SIZE;
+
+  if (base64_seed_signature.empty())
+    return VARIATIONS_SEED_SIGNATURE_MISSING;
+
+  std::string signature;
+  if (!base::Base64Decode(base64_seed_signature, &signature))
+    return VARIATIONS_SEED_SIGNATURE_DECODE_FAILED;
+
+  crypto::SignatureVerifier verifier;
+  if (!verifier.VerifyInit(
+          kECDSAWithSHA256AlgorithmID, sizeof(kECDSAWithSHA256AlgorithmID),
+          reinterpret_cast<const uint8*>(signature.data()), signature.size(),
+          kPublicKey, arraysize(kPublicKey))) {
+    return VARIATIONS_SEED_SIGNATURE_INVALID_SIGNATURE;
+  }
+
+  verifier.VerifyUpdate(reinterpret_cast<const uint8*>(seed_bytes.data()),
+                        seed_bytes.size());
+  if (verifier.VerifyFinal())
+    return VARIATIONS_SEED_SIGNATURE_VALID;
+  return VARIATIONS_SEED_SIGNATURE_INVALID_SEED;
+}
+
 }  // namespace chrome_variations