Introduce a hash_of_hashes dictionary.

It allows us to know whether the existing hashes dictionary can be trusted on startup (and thus whether seeding should be allowed).

Note: seeding a NULL preference is always allowed.

Review URL: https://codereview.chromium.org/126093007

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@243776 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/prefs/pref_hash_filter.cc

@@ -31,10 +31,14 @@ void ReportValidationResult(PrefHashStore::ValueState value_state,
       UMA_HISTOGRAM_ENUMERATION("Settings.TrackedPreferenceChanged",
                                 value_index, num_values);
       return;
-    case PrefHashStore::UNKNOWN_VALUE:
+    case PrefHashStore::UNTRUSTED_UNKNOWN_VALUE:
       UMA_HISTOGRAM_ENUMERATION("Settings.TrackedPreferenceInitialized",
                                 value_index, num_values);
       return;
+    case PrefHashStore::TRUSTED_UNKNOWN_VALUE:
+      UMA_HISTOGRAM_ENUMERATION("Settings.TrackedPreferenceTrustedInitialized",
+                                value_index, num_values);
+      return;
   }
   NOTREACHED() << "Unexpected PrefHashStore::ValueState: " << value_state;
 }
@@ -99,10 +103,13 @@ void PrefHashFilter::FilterOnLoad(base::DictionaryValue* pref_store_contents) {
         case PrefHashStore::CLEARED:
           // Unfortunate case, but there is nothing we can do.
           break;
+        case PrefHashStore::TRUSTED_UNKNOWN_VALUE:
+          // It is okay to seed the hash in this case.
+          break;
         case PrefHashStore::MIGRATED:
           reset_state = no_migration_ ? DO_RESET : WANTED_RESET;
           break;
-        case PrefHashStore::UNKNOWN_VALUE:
+        case PrefHashStore::UNTRUSTED_UNKNOWN_VALUE:
           reset_state = no_seeding_ ? DO_RESET : WANTED_RESET;
           break;
         case PrefHashStore::CHANGED:

chrome/browser/prefs/pref_hash_filter_unittest.cc

@@ -177,8 +177,9 @@ TEST_P(PrefHashFilterTest, FilterUntrackedPrefUpdate) {
 
 TEST_P(PrefHashFilterTest, EmptyAndUnknown) {
   ASSERT_FALSE(pref_store_contents_.Get(kTestPref, NULL));
+  // NULL values are always trusted by the PrefHashStore.
   mock_pref_hash_store_->SetCheckResult(kTestPref,
-                                        PrefHashStore::UNKNOWN_VALUE);
+                                        PrefHashStore::TRUSTED_UNKNOWN_VALUE);
   pref_hash_filter_->FilterOnLoad(&pref_store_contents_);
   ASSERT_EQ(arraysize(kTestTrackedPrefs),
             mock_pref_hash_store_->checked_paths_count());
@@ -196,7 +197,7 @@ TEST_P(PrefHashFilterTest, InitialValueUnknown) {
   ASSERT_TRUE(pref_store_contents_.Get(kTestPref, NULL));
 
   mock_pref_hash_store_->SetCheckResult(kTestPref,
-                                        PrefHashStore::UNKNOWN_VALUE);
+                                        PrefHashStore::UNTRUSTED_UNKNOWN_VALUE);
   pref_hash_filter_->FilterOnLoad(&pref_store_contents_);
   ASSERT_EQ(arraysize(kTestTrackedPrefs),
             mock_pref_hash_store_->checked_paths_count());
@@ -217,6 +218,27 @@ TEST_P(PrefHashFilterTest, InitialValueUnknown) {
   }
 }
 
+TEST_P(PrefHashFilterTest, InitialValueTrustedUnknown) {
+  // Ownership of this value is transfered to |pref_store_contents_|.
+  base::Value* string_value = base::Value::CreateStringValue("test");
+  pref_store_contents_.Set(kTestPref, string_value);
+
+  ASSERT_TRUE(pref_store_contents_.Get(kTestPref, NULL));
+
+  mock_pref_hash_store_->SetCheckResult(kTestPref,
+                                        PrefHashStore::TRUSTED_UNKNOWN_VALUE);
+  pref_hash_filter_->FilterOnLoad(&pref_store_contents_);
+  ASSERT_EQ(arraysize(kTestTrackedPrefs),
+            mock_pref_hash_store_->checked_paths_count());
+  ASSERT_EQ(1u, mock_pref_hash_store_->stored_paths_count());
+
+  // Seeding is always allowed for trusted unknown values.
+  const base::Value* value_in_store;
+  ASSERT_TRUE(pref_store_contents_.Get(kTestPref, &value_in_store));
+  ASSERT_EQ(string_value, value_in_store);
+  ASSERT_EQ(string_value, mock_pref_hash_store_->stored_value(kTestPref));
+}
+
 TEST_P(PrefHashFilterTest, InitialValueChanged) {
   // Ownership of this value is transfered to |pref_store_contents_|.
   base::Value* int_value = base::Value::CreateIntegerValue(1234);

chrome/browser/prefs/pref_hash_store.h

@@ -13,6 +13,16 @@ namespace base {
 class Value;
 }  // namespace base
 
+namespace internals {
+
+// Hash of hashes for each profile, used to validate the existing hashes when
+// debating whether an unknown value is to be trusted, will be stored as a
+// string under
+// |kProfilePreferenceHashes|.|kHashOfHashesPref|.|hash_stored_id_|.
+const char kHashOfHashesPref[] = "hash_of_hashes";
+
+}  // namespace internals
+
 // Stores hashes of and verifies preference values. To use, first call
 // |InitializeTrackedValue| with each preference that should be tracked. Then
 // call |OnPrefValueChanged| to update the hash store when preference values
@@ -32,7 +42,13 @@ class PrefHashStore {
     // The preference value has been changed since the last hash.
     CHANGED,
     // No stored hash exists for the preference value.
-    UNKNOWN_VALUE,
+    UNTRUSTED_UNKNOWN_VALUE,
+    // No stored hash exists for the preference value, but the current set of
+    // hashes stored is trusted and thus this value can safely be seeded. This
+    // happens when all hashes are already properly seeded and a newly
+    // tracked value needs to be seeded). NULL values are inherently trusted as
+    // well.
+    TRUSTED_UNKNOWN_VALUE,
   };
 
   // Checks |initial_value| against the existing stored value hash.

chrome/browser/prefs/pref_hash_store_impl.cc

@@ -5,6 +5,7 @@
 #include "chrome/browser/prefs/pref_hash_store_impl.h"
 
 #include "base/logging.h"
+#include "base/metrics/histogram.h"
 #include "base/prefs/pref_registry_simple.h"
 #include "base/prefs/pref_service.h"
 #include "base/prefs/scoped_user_pref_update.h"
@@ -17,7 +18,11 @@ PrefHashStoreImpl::PrefHashStoreImpl(const std::string& hash_store_id,
                                      PrefService* local_state)
     : hash_store_id_(hash_store_id),
       pref_hash_calculator_(seed, device_id),
-      local_state_(local_state) {}
+      local_state_(local_state),
+      initial_hashes_dictionary_trusted_(IsHashDictionaryTrusted()) {
+  UMA_HISTOGRAM_BOOLEAN("Settings.HashesDictionaryTrusted",
+                        initial_hashes_dictionary_trusted_);
+}
 
 // static
 void PrefHashStoreImpl::RegisterPrefs(PrefRegistrySimple* registry) {
@@ -35,42 +40,82 @@ PrefHashStore::ValueState PrefHashStoreImpl::CheckValue(
                                                      &hashed_prefs);
 
   std::string last_hash;
-  if (!hashed_prefs || !hashed_prefs->GetString(path, &last_hash))
-    return PrefHashStore::UNKNOWN_VALUE;
+  if (!hashed_prefs || !hashed_prefs->GetString(path, &last_hash)) {
+    // In the absence of a hash for this pref, always trust a NULL value, but
+    // only trust an existing value if the initial hashes dictionary is trusted.
+    return (!initial_value || initial_hashes_dictionary_trusted_) ?
+               TRUSTED_UNKNOWN_VALUE : UNTRUSTED_UNKNOWN_VALUE;
+  }
 
   PrefHashCalculator::ValidationResult validation_result =
       pref_hash_calculator_.Validate(path, initial_value, last_hash);
   switch (validation_result) {
     case PrefHashCalculator::VALID:
-      return PrefHashStore::UNCHANGED;
+      return UNCHANGED;
     case PrefHashCalculator::VALID_LEGACY:
-      return PrefHashStore::MIGRATED;
+      return MIGRATED;
     case PrefHashCalculator::INVALID:
-      return initial_value ? PrefHashStore::CHANGED : PrefHashStore::CLEARED;
+      return initial_value ? CHANGED : CLEARED;
   }
   NOTREACHED() << "Unexpected PrefHashCalculator::ValidationResult: "
                << validation_result;
-  return PrefHashStore::UNKNOWN_VALUE;
+  return UNTRUSTED_UNKNOWN_VALUE;
 }
 
 void PrefHashStoreImpl::StoreHash(
     const std::string& path, const base::Value* new_value) {
   {
     DictionaryPrefUpdate update(local_state_, prefs::kProfilePreferenceHashes);
-    base::DictionaryValue* child_dictionary = NULL;
 
     // Get the dictionary corresponding to the profile name, which may have a
     // '.'
+    base::DictionaryValue* hashes_dict = NULL;
     if (!update->GetDictionaryWithoutPathExpansion(hash_store_id_,
-                                                   &child_dictionary)) {
-      child_dictionary = new base::DictionaryValue;
-      update->SetWithoutPathExpansion(hash_store_id_, child_dictionary);
+                                                   &hashes_dict)) {
+      hashes_dict = new base::DictionaryValue;
+      update->SetWithoutPathExpansion(hash_store_id_, hashes_dict);
     }
 
-    child_dictionary->SetString(
+    hashes_dict->SetString(
         path, pref_hash_calculator_.Calculate(path, new_value));
+
+    // Get the dictionary where the hash of hashes are stored.
+    base::DictionaryValue* hash_of_hashes_dict = NULL;
+    if (!update->GetDictionaryWithoutPathExpansion(internals::kHashOfHashesPref,
+                                                   &hash_of_hashes_dict)) {
+      hash_of_hashes_dict = new base::DictionaryValue;
+      update->SetWithoutPathExpansion(internals::kHashOfHashesPref,
+                                      hash_of_hashes_dict);
+    }
+    // Use the |hash_store_id_| as the hashed path to avoid having the hash
+    // depend on kProfilePreferenceHashes.
+    std::string hash_of_hashes(pref_hash_calculator_.Calculate(hash_store_id_,
+                                                               hashes_dict));
+    hash_of_hashes_dict->SetStringWithoutPathExpansion(hash_store_id_,
+                                                       hash_of_hashes);
   }
   // TODO(erikwright): During tests, pending writes were still waiting when the
   // IO thread is already gone. Consider other solutions.
   local_state_->CommitPendingWrite();
 }
+
+bool PrefHashStoreImpl::IsHashDictionaryTrusted() const {
+  const base::DictionaryValue* pref_hash_dicts =
+      local_state_->GetDictionary(prefs::kProfilePreferenceHashes);
+  const base::DictionaryValue* hashes_dict = NULL;
+  const base::DictionaryValue* hash_of_hashes_dict = NULL;
+  std::string hash_of_hashes;
+  // The absence of the hashes dictionary isn't trusted. Nor is the absence of
+  // the hash of hashes for this |hash_store_id_|.
+  if (!pref_hash_dicts->GetDictionaryWithoutPathExpansion(
+          hash_store_id_, &hashes_dict) ||
+      !pref_hash_dicts->GetDictionaryWithoutPathExpansion(
+          internals::kHashOfHashesPref, &hash_of_hashes_dict) ||
+      !hash_of_hashes_dict->GetStringWithoutPathExpansion(
+          hash_store_id_, &hash_of_hashes)) {
+    return false;
+  }
+
+  return pref_hash_calculator_.Validate(
+      hash_store_id_, hashes_dict, hash_of_hashes) == PrefHashCalculator::VALID;
+}

chrome/browser/prefs/pref_hash_store_impl.h

@@ -46,9 +46,15 @@ class PrefHashStoreImpl : public PrefHashStore {
                          const base::Value* value) OVERRIDE;
 
  private:
+  // Returns true if the dictionary of hashes stored for |hash_store_id_| is
+  // trusted (which implies unknown values can be trusted as newly tracked
+  // values).
+  bool IsHashDictionaryTrusted() const;
+
   std::string hash_store_id_;
   PrefHashCalculator pref_hash_calculator_;
   PrefService* local_state_;
+  bool initial_hashes_dictionary_trusted_;
 
   DISALLOW_COPY_AND_ASSIGN(PrefHashStoreImpl);
 };

chrome/browser/prefs/pref_hash_store_impl_unittest.cc

@@ -25,16 +25,20 @@ TEST(PrefHashStoreImplTest, TestCase) {
   PrefHashStoreImpl pref_hash_store(
       "store_id", std::string(32,0), "device_id", &local_state);
 
-  ASSERT_EQ(PrefHashStore::UNKNOWN_VALUE,
+  // Only NULL should be trusted in the absence of a hash.
+  EXPECT_EQ(PrefHashStore::UNTRUSTED_UNKNOWN_VALUE,
             pref_hash_store.CheckValue("path1", &string_1));
+  EXPECT_EQ(PrefHashStore::TRUSTED_UNKNOWN_VALUE,
+            pref_hash_store.CheckValue("path1", NULL));
+
   pref_hash_store.StoreHash("path1", &string_1);
-  ASSERT_EQ(PrefHashStore::UNCHANGED,
+  EXPECT_EQ(PrefHashStore::UNCHANGED,
             pref_hash_store.CheckValue("path1", &string_1));
-  ASSERT_EQ(PrefHashStore::CLEARED, pref_hash_store.CheckValue("path1", NULL));
+  EXPECT_EQ(PrefHashStore::CLEARED, pref_hash_store.CheckValue("path1", NULL));
   pref_hash_store.StoreHash("path1", NULL);
-  ASSERT_EQ(PrefHashStore::UNCHANGED,
+  EXPECT_EQ(PrefHashStore::UNCHANGED,
             pref_hash_store.CheckValue("path1", NULL));
-  ASSERT_EQ(PrefHashStore::CHANGED,
+  EXPECT_EQ(PrefHashStore::CHANGED,
             pref_hash_store.CheckValue("path1", &string_2));
 
   base::DictionaryValue dict;
@@ -43,17 +47,51 @@ TEST(PrefHashStoreImplTest, TestCase) {
   dict.Set("b", new base::StringValue("bar"));
   dict.Set("c", new base::StringValue("baz"));
 
-  // Manually shove in a legacy hash.
-  DictionaryPrefUpdate update(&local_state, prefs::kProfilePreferenceHashes);
-  base::DictionaryValue* child_dictionary = NULL;
-  ASSERT_TRUE(update->GetDictionary("store_id", &child_dictionary));
-  child_dictionary->SetString(
-      "path1",
-      "C503FB7C65EEFD5C07185F616A0AA67923C069909933F362022B1F187E73E9A2");
-
-  ASSERT_EQ(PrefHashStore::MIGRATED,
+  {
+    // Manually shove in a legacy hash.
+    DictionaryPrefUpdate update(&local_state, prefs::kProfilePreferenceHashes);
+    base::DictionaryValue* child_dictionary = NULL;
+    ASSERT_TRUE(update->GetDictionary("store_id", &child_dictionary));
+    child_dictionary->SetString(
+        "path1",
+        "C503FB7C65EEFD5C07185F616A0AA67923C069909933F362022B1F187E73E9A2");
+  }
+  EXPECT_EQ(PrefHashStore::MIGRATED,
             pref_hash_store.CheckValue("path1", &dict));
   pref_hash_store.StoreHash("path1", &dict);
-  ASSERT_EQ(PrefHashStore::UNCHANGED,
+  EXPECT_EQ(PrefHashStore::UNCHANGED,
             pref_hash_store.CheckValue("path1", &dict));
+
+  // |pref_hash_store2| should trust its initial hashes dictionary and thus
+  // trust new unknown values.
+  PrefHashStoreImpl pref_hash_store2(
+      "store_id", std::string(32, 0), "device_id", &local_state);
+  EXPECT_EQ(PrefHashStore::TRUSTED_UNKNOWN_VALUE,
+            pref_hash_store2.CheckValue("new_path", &string_1));
+  EXPECT_EQ(PrefHashStore::TRUSTED_UNKNOWN_VALUE,
+            pref_hash_store2.CheckValue("new_path", &string_2));
+  EXPECT_EQ(PrefHashStore::TRUSTED_UNKNOWN_VALUE,
+            pref_hash_store2.CheckValue("new_path", NULL));
+
+  {
+    // Manually corrupt the hash of hashes for "store_id".
+    DictionaryPrefUpdate update(&local_state, prefs::kProfilePreferenceHashes);
+    base::DictionaryValue* hash_of_hashes_dict = NULL;
+    ASSERT_TRUE(update->GetDictionaryWithoutPathExpansion(
+        internals::kHashOfHashesPref, &hash_of_hashes_dict));
+    hash_of_hashes_dict->SetString("store_id", std::string(64, 'A'));
+    // This shouldn't have increased the number of existing hash of hashes.
+    ASSERT_EQ(1U, hash_of_hashes_dict->size());
+  }
+
+  // |pref_hash_store3| should no longer trust its initial hashes dictionary and
+  // thus shouldn't trust non-NULL unknown values.
+  PrefHashStoreImpl pref_hash_store3(
+      "store_id", std::string(32, 0), "device_id", &local_state);
+  EXPECT_EQ(PrefHashStore::UNTRUSTED_UNKNOWN_VALUE,
+            pref_hash_store3.CheckValue("new_path", &string_1));
+  EXPECT_EQ(PrefHashStore::UNTRUSTED_UNKNOWN_VALUE,
+            pref_hash_store3.CheckValue("new_path", &string_2));
+  EXPECT_EQ(PrefHashStore::TRUSTED_UNKNOWN_VALUE,
+            pref_hash_store3.CheckValue("new_path", NULL));
 }

tools/metrics/histograms/histograms.xml

@@ -16925,6 +16925,13 @@ other types of suffix sets.
   </summary>
 </histogram>
 
+<histogram name="Settings.HashesDictionaryTrusted" enum="BooleanValid">
+  <summary>
+    Logged on profile load. Indicates whether the hashes dictionary for this
+    profile is trusted.
+  </summary>
+</histogram>
+
 <histogram name="Settings.HomePageDomain" enum="OmniboxSearchEngine">
   <obsolete>
     Deprecated in Chrome 30.  Replaced by Settings.HomePageEngineType.
@@ -17097,6 +17104,15 @@ other types of suffix sets.
   <summary>The id of a tracked preference which was reset by Chrome.</summary>
 </histogram>
 
+<histogram name="Settings.TrackedPreferenceTrustedInitialized"
+    enum="TrackedPreference">
+  <summary>
+    The id of a tracked preference which was initialized despite the absence of
+    a MAC as either (1) the current MACs are trusted, infering that this is a
+    newly tracked pref, or (2) its value is NULL.
+  </summary>
+</histogram>
+
 <histogram name="Settings.TrackedPreferenceUnchanged" enum="TrackedPreference">
   <summary>
     The id of a tracked preference whose value has not changed since the last