Integrate registry_hash_store_contents with the rest of tracked prefs.

chrome/browser/prefs/tracked/pref_hash_browsertest.cc

 // Copyright 2014 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 <memory>
 #include <string>
 
 #include "base/base_switches.h"
 #include "base/command_line.h"
 #include "base/files/file_path.h"
@@ skipping 20 matching lines @@
 #include "chrome/test/base/testing_profile.h"
 #include "components/search_engines/default_search_manager.h"
 #include "components/user_prefs/tracked/tracked_preference_histogram_names.h"
 #include "extensions/browser/pref_names.h"
 #include "extensions/common/extension.h"
 
 #if defined(OS_CHROMEOS)
 #include "chromeos/chromeos_switches.h"
 #endif
 
+#if defined(OS_WIN)
+#include "base/test/test_reg_util_win.h"
+#endif
+
 namespace {
 
 // Extension ID of chrome/test/data/extensions/good.crx
 const char kGoodCrxId[] = "ldnnhddmnhbkjipkidpdiheffobcpfmf";
 
+// Prefix for the registry path.
+constexpr base::char16 kRegistryTestPathPrefix[] =
+    L"SOFTWARE\\Chromium\\PrefHashBrowserTest\\";
+
 // Explicit expectations from the caller of GetTrackedPrefHistogramCount(). This
 // enables detailed reporting of the culprit on failure.
 enum AllowedBuckets {
   // Allow no samples in any buckets.
   ALLOW_NONE = -1,
   // Any integer between BEGIN_ALLOW_SINGLE_BUCKET and END_ALLOW_SINGLE_BUCKET
   // indicates that only this specific bucket is allowed to have a sample.
   BEGIN_ALLOW_SINGLE_BUCKET = 0,
   END_ALLOW_SINGLE_BUCKET = 100,
   // Allow any buckets (no extra verifications performed).
   ALLOW_ANY
 };
 
+base::string16 GetRegistryPathForTestProfile() {
+  base::FilePath profile_dir;
+  EXPECT_TRUE(PathService::Get(chrome::DIR_USER_DATA, &profile_dir));
+  return kRegistryTestPathPrefix + profile_dir.BaseName().value();
+}
+
 // Returns the number of times |histogram_name| was reported so far; adding the
 // results of the first 100 buckets (there are only ~19 reporting IDs as of this
 // writing; varies depending on the platform). |allowed_buckets| hints at extra
 // requirements verified in this method (see AllowedBuckets for details).
 int GetTrackedPrefHistogramCount(const char* histogram_name,
+                                 const char* histogram_suffix,
                                  int allowed_buckets) {
+  std::string full_histogram_name(histogram_name);
+  if (*histogram_suffix != 0)
+    full_histogram_name.append(".").append(histogram_suffix);
   const base::HistogramBase* histogram =
-      base::StatisticsRecorder::FindHistogram(histogram_name);
+      base::StatisticsRecorder::FindHistogram(full_histogram_name);
   if (!histogram)
     return 0;
 
   std::unique_ptr<base::HistogramSamples> samples(histogram->SnapshotSamples());
   int sum = 0;
   for (int i = 0; i < 100; ++i) {
     int count_for_id = samples->GetCount(i);
     EXPECT_GE(count_for_id, 0);
     sum += count_for_id;
 
     if (allowed_buckets == ALLOW_NONE ||
         (allowed_buckets != ALLOW_ANY && i != allowed_buckets)) {
       EXPECT_EQ(0, count_for_id) << "Unexpected reporting_id: " << i;
     }
   }
   return sum;
 }
 
+// Helper function to call GetTrackedPrefHistogramCount with no external
+// validation suffix.
+int GetTrackedPrefHistogramCount(const char* histogram_name,
+                                 int allowed_buckets) {
+  return GetTrackedPrefHistogramCount(histogram_name, "", allowed_buckets);
+}
+
 std::unique_ptr<base::DictionaryValue> ReadPrefsDictionary(
     const base::FilePath& pref_file) {
   JSONFileValueDeserializer deserializer(pref_file);
   int error_code = JSONFileValueDeserializer::JSON_NO_ERROR;
   std::string error_str;
   std::unique_ptr<base::Value> prefs =
       deserializer.Deserialize(&error_code, &error_str);
   if (!prefs || error_code != JSONFileValueDeserializer::JSON_NO_ERROR) {
     ADD_FAILURE() << "Error #" << error_code << ": " << error_str;
     return std::unique_ptr<base::DictionaryValue>();
   }
   if (!prefs->IsType(base::Value::TYPE_DICTIONARY)) {
     ADD_FAILURE();
     return std::unique_ptr<base::DictionaryValue>();
   }
   return std::unique_ptr<base::DictionaryValue>(
       static_cast<base::DictionaryValue*>(prefs.release()));
 }
 
+// Returns whether external validation is supported on the platform through
+// storing MACs in the registry.
+bool SupportsRegistryValidation() {
+#if defined(OS_WIN)
+  return true;
+#else
+  return false;
+#endif
+}
+
 #define PREF_HASH_BROWSER_TEST(fixture, test_name)                          \
   IN_PROC_BROWSER_TEST_P(fixture, PRE_##test_name) {                        \
     SetupPreferences();                                                     \
   }                                                                         \
   IN_PROC_BROWSER_TEST_P(fixture, test_name) {                              \
     VerifyReactionToPrefAttack();                                           \
   }                                                                         \
   INSTANTIATE_TEST_CASE_P(                                                  \
       fixture##Instance,                                                    \
       fixture,                                                              \
@@ skipping 112 matching lines @@
 
     return true;
   }
 
   void SetUpInProcessBrowserTestFixture() override {
     ExtensionBrowserTest::SetUpInProcessBrowserTestFixture();
 
     // Bots are on a domain, turn off the domain check for settings hardening in
     // order to be able to test all SettingsEnforcement groups.
     chrome_prefs::DisableDomainCheckForTesting();
+
+#if defined(OS_WIN)
+    // Avoid polluting prefs for the user and the bots by writing to a specific
+    // testing registry path.
+    base::string16 registry_key = GetRegistryPathForTestProfile();
+    ProfilePrefStoreManager::SetPreferenceValidationRegistryPathForTesting(
+        registry_key);
+
+    // For PRE tests, delete the Registry key to avoid collisions.
+    if (IsPRETest()) {
+      base::win::RegKey key;
+      if (key.Open(HKEY_CURRENT_USER, registry_key.c_str(),
+                   KEY_SET_VALUE | KEY_WOW64_32KEY) == ERROR_SUCCESS) {
+        LONG result = key.DeleteKey(L"");
+        ASSERT_TRUE(result == ERROR_SUCCESS || result == ERROR_FILE_NOT_FOUND);
+      }
+    }
+#endif
+  }
+
+  void TearDown() override {
+#if defined(OS_WIN)
+    // For non-PRE tests, delete the Registry key to avoid polluting the
+    // registry.
+    // TODO(proberge): it would be nice to delete keys from interrupted tests
+    // as well.

[gab, 2016/09/21 17:55:30]

If you mean tests that fail by assertion, I think TearDown() runs regardless
(and otherwise the destructor of the fixture sure does, so maybe put the code
there).

If you mean on crash, there's not much we can do there.

[proberge, 2016/09/21 21:09:24]

Yeah I meant by crash (or CTRL-C). There's some logic in
https://cs.chromium.org/chromium/src/base/test/test_reg_util_win.cc?sq=packag...
we might be able to use. 

[gab, 2016/09/22 18:38:28]

I see, but if we're going to re-use too much of test_reg_util_win, I'd vote for
augmenting it to support our use case instead of stealing from it. Up to you.

[proberge, 2016/09/22 19:10:15]

Thinking about it a bit more, the cleanup logic relies on a timestamp in the
registry override path. We want the PRE_ and normal test to use the same
registry override path, so the timestamp has to be the same. The simplest
solution (rounding or flooring the timestamp by hour or day) has issues on the
time boundaries. 

I think I'll skip addressing this for now.

+    if (!IsPRETest()) {
+      base::string16 registry_key = GetRegistryPathForTestProfile();
+      base::win::RegKey key;
+      if (key.Open(HKEY_CURRENT_USER, registry_key.c_str(),
+                   KEY_SET_VALUE | KEY_WOW64_32KEY) == ERROR_SUCCESS) {
+        LONG result = key.DeleteKey(L"");
+        ASSERT_TRUE(result == ERROR_SUCCESS || result == ERROR_FILE_NOT_FOUND);
+      }
+    }
+#endif
+    ExtensionBrowserTest::TearDown();
   }
 
   // In the PRE_ test, find the number of tracked preferences that were
   // initialized and save it to a file to be read back in the main test and used
   // as the total number of tracked preferences.
   void SetUpOnMainThread() override {
     ExtensionBrowserTest::SetUpOnMainThread();
 
     // File in which the PRE_ test will save the number of tracked preferences
     // on this platform.
@@ skipping 13 matching lines @@
 
       // Split tracked prefs are reported as Unchanged not as NullInitialized
       // when an empty dictionary is encountered on first run (this should only
       // hit for pref #5 in the current design).
       int num_split_tracked_prefs = GetTrackedPrefHistogramCount(
           user_prefs::tracked::kTrackedPrefHistogramUnchanged,
           BEGIN_ALLOW_SINGLE_BUCKET + 5);
       EXPECT_EQ(protection_level_ > PROTECTION_DISABLED_ON_PLATFORM ? 1 : 0,
                 num_split_tracked_prefs);
 
+      if (SupportsRegistryValidation()) {
+        // Same checks as above, but for the registry.
+        num_tracked_prefs_ = GetTrackedPrefHistogramCount(
+            user_prefs::tracked::kTrackedPrefHistogramNullInitialized,
+            user_prefs::tracked::kTrackedPrefRegistryValidationSuffix,
+            ALLOW_ANY);
+        EXPECT_EQ(protection_level_ > PROTECTION_DISABLED_ON_PLATFORM,
+                  num_tracked_prefs_ > 0);
+
+        int num_split_tracked_prefs = GetTrackedPrefHistogramCount(
+            user_prefs::tracked::kTrackedPrefHistogramUnchanged,
+            user_prefs::tracked::kTrackedPrefRegistryValidationSuffix,
+            BEGIN_ALLOW_SINGLE_BUCKET + 5);
+        EXPECT_EQ(protection_level_ > PROTECTION_DISABLED_ON_PLATFORM ? 1 : 0,
+                  num_split_tracked_prefs);
+      }
+
       num_tracked_prefs_ += num_split_tracked_prefs;
 
       std::string num_tracked_prefs_str = base::IntToString(num_tracked_prefs_);
       EXPECT_EQ(static_cast<int>(num_tracked_prefs_str.size()),
                 base::WriteFile(num_tracked_prefs_file,
                                 num_tracked_prefs_str.c_str(),
                                 num_tracked_prefs_str.size()));
     } else {
       std::string num_tracked_prefs_str;
       EXPECT_TRUE(base::ReadFileToString(num_tracked_prefs_file,
@@ skipping 121 matching lines @@
               GetTrackedPrefHistogramCount(
                   user_prefs::tracked::kTrackedPrefHistogramTrustedInitialized,
                   ALLOW_NONE));
     EXPECT_EQ(0, GetTrackedPrefHistogramCount(
                      user_prefs::tracked::kTrackedPrefHistogramNullInitialized,
                      ALLOW_NONE));
     EXPECT_EQ(
         0, GetTrackedPrefHistogramCount(
                user_prefs::tracked::kTrackedPrefHistogramMigratedLegacyDeviceId,
                ALLOW_NONE));
+
+    if (SupportsRegistryValidation()) {
+      // Expect all prefs to be reported as Unchanged with no resets.
+      EXPECT_EQ(protection_level_ > PROTECTION_DISABLED_ON_PLATFORM
+                    ? num_tracked_prefs()
+                    : 0,
+                GetTrackedPrefHistogramCount(
+                    user_prefs::tracked::kTrackedPrefHistogramUnchanged,
+                    user_prefs::tracked::kTrackedPrefRegistryValidationSuffix,
+                    ALLOW_ANY));
+    }
   }
 };
 
 PREF_HASH_BROWSER_TEST(PrefHashBrowserTestUnchangedDefault, UnchangedDefault);
 
 // Augments PrefHashBrowserTestUnchangedDefault to confirm that nothing is reset
 // when nothing is tampered with, even if Chrome itself wrote custom prefs in
 // its last run.
 class PrefHashBrowserTestUnchangedCustom
     : public PrefHashBrowserTestUnchangedDefault {
@@ skipping 70 matching lines @@
               GetTrackedPrefHistogramCount(
                   user_prefs::tracked::kTrackedPrefHistogramTrustedInitialized,
                   ALLOW_NONE));
     EXPECT_EQ(0, GetTrackedPrefHistogramCount(
                      user_prefs::tracked::kTrackedPrefHistogramNullInitialized,
                      ALLOW_NONE));
     EXPECT_EQ(
         0, GetTrackedPrefHistogramCount(
                user_prefs::tracked::kTrackedPrefHistogramMigratedLegacyDeviceId,
                ALLOW_NONE));
+
+    if (SupportsRegistryValidation()) {
+      // Expect homepage to have been cleared.
+      EXPECT_EQ(protection_level_ > PROTECTION_DISABLED_ON_PLATFORM ? 1 : 0,
+                GetTrackedPrefHistogramCount(
+                    user_prefs::tracked::kTrackedPrefHistogramCleared,
+                    user_prefs::tracked::kTrackedPrefRegistryValidationSuffix,
+                    BEGIN_ALLOW_SINGLE_BUCKET + 2));
+    }
   }
 };
 
 PREF_HASH_BROWSER_TEST(PrefHashBrowserTestClearedAtomic, ClearedAtomic);
 
 // Verifies that clearing the MACs results in untrusted Initialized pings for
 // non-null protected prefs.
 class PrefHashBrowserTestUntrustedInitialized : public PrefHashBrowserTestBase {
  public:
   void SetupPreferences() override {
@@ skipping 102 matching lines @@
     EXPECT_EQ(
         0, GetTrackedPrefHistogramCount(
                user_prefs::tracked::kTrackedPrefHistogramChanged, ALLOW_NONE));
     EXPECT_EQ(
         0, GetTrackedPrefHistogramCount(
                user_prefs::tracked::kTrackedPrefHistogramCleared, ALLOW_NONE));
     EXPECT_EQ(
         0, GetTrackedPrefHistogramCount(
                user_prefs::tracked::kTrackedPrefHistogramMigratedLegacyDeviceId,
                ALLOW_NONE));
+
+    if (SupportsRegistryValidation()) {
+      // The MACs have been cleared but the preferences have not been tampered.
+      // The registry should report all prefs as unchanged.
+      EXPECT_EQ(protection_level_ > PROTECTION_DISABLED_ON_PLATFORM
+                    ? num_tracked_prefs()
+                    : 0,
+                GetTrackedPrefHistogramCount(
+                    user_prefs::tracked::kTrackedPrefHistogramUnchanged,
+                    user_prefs::tracked::kTrackedPrefRegistryValidationSuffix,
+                    ALLOW_ANY));
+    }
   }
 };
 
 PREF_HASH_BROWSER_TEST(PrefHashBrowserTestUntrustedInitialized,
                        UntrustedInitialized);
 
 // Verifies that changing an atomic pref results in it being reported (and reset
 // if the protection level allows it).
 class PrefHashBrowserTestChangedAtomic : public PrefHashBrowserTestBase {
  public:
@@ skipping 70 matching lines @@
               GetTrackedPrefHistogramCount(
                   user_prefs::tracked::kTrackedPrefHistogramTrustedInitialized,
                   ALLOW_NONE));
     EXPECT_EQ(0, GetTrackedPrefHistogramCount(
                      user_prefs::tracked::kTrackedPrefHistogramNullInitialized,
                      ALLOW_NONE));
     EXPECT_EQ(
         0, GetTrackedPrefHistogramCount(
                user_prefs::tracked::kTrackedPrefHistogramMigratedLegacyDeviceId,
                ALLOW_NONE));
+
+    if (SupportsRegistryValidation()) {
+      // Expect a single Changed event for tracked pref #4 (startup URLs).
+      EXPECT_EQ(protection_level_ > PROTECTION_DISABLED_ON_PLATFORM ? 1 : 0,
+                GetTrackedPrefHistogramCount(
+                    user_prefs::tracked::kTrackedPrefHistogramChanged,
+                    user_prefs::tracked::kTrackedPrefRegistryValidationSuffix,
+                    BEGIN_ALLOW_SINGLE_BUCKET + 4));
+    }
   }
 };
 
 PREF_HASH_BROWSER_TEST(PrefHashBrowserTestChangedAtomic, ChangedAtomic);
 
 // Verifies that changing or adding an entry in a split pref results in both
 // items being reported (and remove if the protection level allows it).
 class PrefHashBrowserTestChangedSplitPref : public PrefHashBrowserTestBase {
  public:
   void SetupPreferences() override {
@@ skipping 77 matching lines @@
               GetTrackedPrefHistogramCount(
                   user_prefs::tracked::kTrackedPrefHistogramTrustedInitialized,
                   ALLOW_NONE));
     EXPECT_EQ(0, GetTrackedPrefHistogramCount(
                      user_prefs::tracked::kTrackedPrefHistogramNullInitialized,
                      ALLOW_NONE));
     EXPECT_EQ(
         0, GetTrackedPrefHistogramCount(
                user_prefs::tracked::kTrackedPrefHistogramMigratedLegacyDeviceId,
                ALLOW_NONE));
+
+    if (SupportsRegistryValidation()) {
+      // Expect a single split pref changed report with a count of 2 for tracked
+      // pref #5 (extensions).
+      EXPECT_EQ(protection_level_ > PROTECTION_DISABLED_ON_PLATFORM ? 1 : 0,
+                GetTrackedPrefHistogramCount(
+                    user_prefs::tracked::kTrackedPrefHistogramChanged,
+                    user_prefs::tracked::kTrackedPrefRegistryValidationSuffix,
+                    BEGIN_ALLOW_SINGLE_BUCKET + 5));
+    }
   }
 };
 
 PREF_HASH_BROWSER_TEST(PrefHashBrowserTestChangedSplitPref, ChangedSplitPref);
 
 // Verifies that adding a value to unprotected preferences for a key which is
 // still using the default (i.e. has no value stored in protected preferences)
 // doesn't allow that value to slip in with no valid MAC (regression test for
 // http://crbug.com/414554)
 class PrefHashBrowserTestUntrustedAdditionToPrefs
@@ skipping 53 matching lines @@
               GetTrackedPrefHistogramCount(
                   user_prefs::tracked::kTrackedPrefHistogramTrustedInitialized,
                   ALLOW_NONE));
     EXPECT_EQ(0, GetTrackedPrefHistogramCount(
                      user_prefs::tracked::kTrackedPrefHistogramNullInitialized,
                      ALLOW_NONE));
     EXPECT_EQ(
         0, GetTrackedPrefHistogramCount(
                user_prefs::tracked::kTrackedPrefHistogramMigratedLegacyDeviceId,
                ALLOW_NONE));
+
+    if (SupportsRegistryValidation()) {
+      EXPECT_EQ((protection_level_ > PROTECTION_DISABLED_ON_PLATFORM &&
+                 protection_level_ < PROTECTION_ENABLED_BASIC)
+                    ? changed_expected
+                    : 0,
+                GetTrackedPrefHistogramCount(
+                    user_prefs::tracked::kTrackedPrefHistogramChanged,
+                    user_prefs::tracked::kTrackedPrefRegistryValidationSuffix,
+                    BEGIN_ALLOW_SINGLE_BUCKET + 3));
+    }
   }
 };
 
 PREF_HASH_BROWSER_TEST(PrefHashBrowserTestUntrustedAdditionToPrefs,
                        UntrustedAdditionToPrefs);
 
 // Verifies that adding a value to unprotected preferences while wiping a
 // user-selected value from protected preferences doesn't allow that value to
 // slip in with no valid MAC (regression test for http://crbug.com/414554).
 class PrefHashBrowserTestUntrustedAdditionToPrefsAfterWipe
@@ skipping 54 matching lines @@
               GetTrackedPrefHistogramCount(
                   user_prefs::tracked::kTrackedPrefHistogramTrustedInitialized,
                   ALLOW_NONE));
     EXPECT_EQ(0, GetTrackedPrefHistogramCount(
                      user_prefs::tracked::kTrackedPrefHistogramNullInitialized,
                      ALLOW_NONE));
     EXPECT_EQ(
         0, GetTrackedPrefHistogramCount(
                user_prefs::tracked::kTrackedPrefHistogramMigratedLegacyDeviceId,
                ALLOW_NONE));
+
+    if (SupportsRegistryValidation()) {
+      EXPECT_EQ(changed_expected,
+                GetTrackedPrefHistogramCount(
+                    user_prefs::tracked::kTrackedPrefHistogramChanged,
+                    user_prefs::tracked::kTrackedPrefRegistryValidationSuffix,
+                    BEGIN_ALLOW_SINGLE_BUCKET + 2));
+      EXPECT_EQ(cleared_expected,
+                GetTrackedPrefHistogramCount(
+                    user_prefs::tracked::kTrackedPrefHistogramCleared,
+                    user_prefs::tracked::kTrackedPrefRegistryValidationSuffix,
+                    BEGIN_ALLOW_SINGLE_BUCKET + 2));
+    }
   }
 };
 
 PREF_HASH_BROWSER_TEST(PrefHashBrowserTestUntrustedAdditionToPrefsAfterWipe,
                        UntrustedAdditionToPrefsAfterWipe);
+class PrefHashBrowserTestRegistryValidationFailure
+    : public PrefHashBrowserTestBase {
+ public:
+  void SetupPreferences() override {
+    profile()->GetPrefs()->SetString(prefs::kHomePage, "http://example.com");
+  }
+
+  void AttackPreferencesOnDisk(
+      base::DictionaryValue* unprotected_preferences,
+      base::DictionaryValue* protected_preferences) override {
+#if defined(OS_WIN)

[gab, 2016/09/21 17:55:30]

Wrap the 1125-1166 in the if-def? Or is there a point verifying a non-attack on
an inactive external hash store?

[proberge, 2016/09/21 21:09:25]

Done.

+    base::string16 registry_key =
+        GetRegistryPathForTestProfile() + L"\\PreferenceMACs\\Default";
+    base::win::RegKey key;
+    ASSERT_EQ(ERROR_SUCCESS, key.Open(HKEY_CURRENT_USER, registry_key.c_str(),
+                                      KEY_SET_VALUE | KEY_WOW64_32KEY));
+    // An incorrect hash should still have the correct size.
+    ASSERT_EQ(ERROR_SUCCESS,
+              key.WriteValue(L"homepage", base::string16(64, 'A').c_str()));
+#endif
+  }
+
+  void VerifyReactionToPrefAttack() override {
+    EXPECT_EQ(
+        protection_level_ > PROTECTION_DISABLED_ON_PLATFORM
+            ? num_tracked_prefs()
+            : 0,
+        GetTrackedPrefHistogramCount(
+            user_prefs::tracked::kTrackedPrefHistogramUnchanged, ALLOW_ANY));
+
+    if (SupportsRegistryValidation()) {
+      EXPECT_EQ(protection_level_ > PROTECTION_DISABLED_ON_PLATFORM ? 1 : 0,
+                GetTrackedPrefHistogramCount(
+                    user_prefs::tracked::kTrackedPrefHistogramChanged,
+                    user_prefs::tracked::kTrackedPrefRegistryValidationSuffix,
+                    BEGIN_ALLOW_SINGLE_BUCKET + 2));
+    }
+  }
+};
+
+PREF_HASH_BROWSER_TEST(PrefHashBrowserTestRegistryValidationFailure,
+                       RegistryValidationFailure);