Move preference MACs to the protected preference stores.

chrome/browser/prefs/tracked/tracked_preferences_migration.cc

Index: chrome/browser/prefs/tracked/tracked_preferences_migration.cc
diff --git a/chrome/browser/prefs/tracked/tracked_preferences_migration.cc b/chrome/browser/prefs/tracked/tracked_preferences_migration.cc
index c358256f06b4f50ff9e88b6045c82634d2b41586..b3beb6da057db0355742d6e42c815e8a9340661c 100644
--- a/chrome/browser/prefs/tracked/tracked_preferences_migration.cc
+++ b/chrome/browser/prefs/tracked/tracked_preferences_migration.cc
@@ -11,6 +11,10 @@
 #include "base/memory/scoped_ptr.h"
 #include "base/values.h"
 #include "chrome/browser/prefs/interceptable_pref_filter.h"
+#include "chrome/browser/prefs/pref_hash_store.h"
+#include "chrome/browser/prefs/pref_hash_store_transaction.h"
+#include "chrome/browser/prefs/tracked/dictionary_hash_store_contents.h"
+#include "chrome/browser/prefs/tracked/hash_store_contents.h"
 
 namespace {
 
@@ -28,6 +32,9 @@ class TrackedPreferencesMigrator
           register_on_successful_unprotected_store_write_callback,
       const base::Callback<void(const base::Closure&)>&
           register_on_successful_protected_store_write_callback,
+      scoped_ptr<PrefHashStore> unprotected_pref_hash_store,
+      scoped_ptr<PrefHashStore> protected_pref_hash_store,
+      scoped_ptr<HashStoreContents> legacy_pref_hash_store,
       InterceptablePrefFilter* unprotected_pref_filter,
       InterceptablePrefFilter* protected_pref_filter);
 
@@ -68,6 +75,10 @@ class TrackedPreferencesMigrator
   InterceptablePrefFilter::FinalizeFilterOnLoadCallback
       finalize_protected_filter_on_load_;
 
+  scoped_ptr<PrefHashStore> unprotected_pref_hash_store_;
+  scoped_ptr<PrefHashStore> protected_pref_hash_store_;
+  scoped_ptr<HashStoreContents> legacy_pref_hash_store_;
+
   scoped_ptr<base::DictionaryValue> unprotected_prefs_;
   scoped_ptr<base::DictionaryValue> protected_prefs_;
 
@@ -109,35 +120,77 @@ void ScheduleSourcePrefStoreCleanup(
 // true if any old duplicates remain in |old_store| and sets |new_store_altered|
 // to true if any value was copied to |new_store|.
 void MigratePrefsFromOldToNewStore(const std::set<std::string>& pref_names,
-                                   const base::DictionaryValue* old_store,
+                                   base::DictionaryValue* old_store,
                                    base::DictionaryValue* new_store,
+                                   PrefHashStore* new_hash_store,
                                    bool* old_store_needs_cleanup,
-                                   bool* new_store_altered) {
+                                   bool* new_store_altered,
+                                   HashStoreContents* legacy_hash_store) {
+  DictionaryHashStoreContents old_hash_store(old_store);
+  const base::DictionaryValue* old_hash_store_contents =
+      old_hash_store.GetContents();

[gab, 2014/06/13 01:57:44]

Merge the above 2 statements into 1:

const base::DictionaryValue* old_hash_store_contents =
    DictionaryHashStoreContents(old_store).GetContents();

[erikwright (departed), 2014/06/16 20:51:27]

Done.

+  const base::DictionaryValue* legacy_hash_store_contents =
+      legacy_hash_store->GetContents();
+  scoped_ptr<PrefHashStoreTransaction> new_hash_store_transaction(
+      new_hash_store->BeginTransaction(new_store));
+
   for (std::set<std::string>::const_iterator it = pref_names.begin();
-       it != pref_names.end(); ++it) {
-    const std::string& pref_name = *it;
-
-    const base::Value* value_in_old_store = NULL;
-    if (!old_store->Get(pref_name, &value_in_old_store))
-      continue;
-
-    // Whether this value ends up being copied below or was left behind by a
-    // previous incomplete migration, it should be cleaned up.
-    *old_store_needs_cleanup = true;
-
-    if (new_store->Get(pref_name, NULL))
-      continue;
-
-    // Copy the value from |old_store| to |new_store| rather than moving it to
-    // avoid data loss should |old_store| be flushed to disk without |new_store|
-    // having equivalently been successfully flushed to disk (e.g., on crash or
-    // in cases where |new_store| is read-only following a read error on
-    // startup).
-    new_store->Set(pref_name, value_in_old_store->DeepCopy());
-    *new_store_altered = true;
+       it != pref_names.end();
+       ++it) {
+     const std::string& pref_name = *it;
+     const base::Value* value_in_old_store = NULL;
+
+    // If the destination does not have a hash for this pref we will
+    // unconditionally attempt to move it.
+    bool destination_hash_missing =
+        !new_hash_store_transaction->HasHash(pref_name);
+    // If we migrate the value we will also attempt to migrate the hash.
+    bool migrated_value = false;
+    if (old_store->Get(pref_name, &value_in_old_store)) {
+      // Whether this value ends up being copied below or was left behind by a
+      // previous incomplete migration, it should be cleaned up.
+      *old_store_needs_cleanup = true;
+
+      if (!new_store->Get(pref_name, NULL)) {
+        // Copy the value from |old_store| to |new_store| rather than moving it
+        // to avoid data loss should |old_store| be flushed to disk without
+        // |new_store| having equivalently been successfully flushed to disk
+        // (e.g., on crash or in cases where |new_store| is read-only following
+        // a read error on startup).
+        scoped_ptr<base::Value> value_copy(value_in_old_store->DeepCopy());

[gab, 2014/06/13 01:57:44]

inline this below as it was before

[erikwright (departed), 2014/06/16 20:51:27]

Done.

+        new_store->Set(pref_name, value_copy.release());
+        migrated_value = true;
+        *new_store_altered = true;
+      }
+    }
+
+    if (destination_hash_missing || migrated_value) {
+      const base::Value* old_hash = NULL;
+      if (old_hash_store_contents)
+        old_hash_store_contents->Get(pref_name, &old_hash);
+      if (!old_hash && legacy_hash_store_contents)
+        legacy_hash_store_contents->Get(pref_name, &old_hash);
+      if (old_hash || !destination_hash_missing) {
+        new_hash_store_transaction->ImportHash(pref_name, old_hash);
+        *new_store_altered = true;
+      }
+    }
+  }
+}
+
+void CleanupMigratedHashes(const std::set<std::string>& migrated_pref_names,
+                           PrefHashStore* origin_pref_hash_store,
+                           base::DictionaryValue* origin_pref_store) {
+  scoped_ptr<PrefHashStoreTransaction> transaction(
+      origin_pref_hash_store->BeginTransaction(origin_pref_store));
+  for (std::set<std::string>::const_iterator it = migrated_pref_names.begin();
+       it != migrated_pref_names.end();
+       ++it) {
+    transaction->ClearHash(*it);
   }
 }
 
+

[gab, 2014/06/13 01:57:44]

nit: rm empty line

[erikwright (departed), 2014/06/16 20:51:27]

Done.

 TrackedPreferencesMigrator::TrackedPreferencesMigrator(
     const std::set<std::string>& unprotected_pref_names,
     const std::set<std::string>& protected_pref_names,
@@ -148,6 +201,9 @@ TrackedPreferencesMigrator::TrackedPreferencesMigrator(
         register_on_successful_unprotected_store_write_callback,
     const base::Callback<void(const base::Closure&)>&
         register_on_successful_protected_store_write_callback,
+    scoped_ptr<PrefHashStore> unprotected_pref_hash_store,
+    scoped_ptr<PrefHashStore> protected_pref_hash_store,
+    scoped_ptr<HashStoreContents> legacy_pref_hash_store,
     InterceptablePrefFilter* unprotected_pref_filter,
     InterceptablePrefFilter* protected_pref_filter)
     : unprotected_pref_names_(unprotected_pref_names),
@@ -157,7 +213,10 @@ TrackedPreferencesMigrator::TrackedPreferencesMigrator(
       register_on_successful_unprotected_store_write_callback_(
           register_on_successful_unprotected_store_write_callback),
       register_on_successful_protected_store_write_callback_(
-          register_on_successful_protected_store_write_callback) {
+          register_on_successful_protected_store_write_callback),
+      unprotected_pref_hash_store_(unprotected_pref_hash_store.Pass()),
+      protected_pref_hash_store_(protected_pref_hash_store.Pass()),
+      legacy_pref_hash_store_(legacy_pref_hash_store.Pass()) {
   // The callbacks bound below will own this TrackedPreferencesMigrator by
   // reference.
   unprotected_pref_filter->InterceptNextFilterOnLoad(
@@ -201,15 +260,31 @@ void TrackedPreferencesMigrator::MigrateIfReady() {
   MigratePrefsFromOldToNewStore(unprotected_pref_names_,
                                 protected_prefs_.get(),
                                 unprotected_prefs_.get(),
+                                unprotected_pref_hash_store_.get(),
                                 &protected_prefs_need_cleanup,
-                                &unprotected_prefs_altered);
+                                &unprotected_prefs_altered,
+                                legacy_pref_hash_store_.get());
   bool unprotected_prefs_need_cleanup = false;
   bool protected_prefs_altered = false;
   MigratePrefsFromOldToNewStore(protected_pref_names_,
                                 unprotected_prefs_.get(),
                                 protected_prefs_.get(),
+                                protected_pref_hash_store_.get(),
                                 &unprotected_prefs_need_cleanup,
-                                &protected_prefs_altered);
+                                &protected_prefs_altered,
+                                legacy_pref_hash_store_.get());
+
+
+

[gab, 2014/06/13 01:57:44]

nit: rm 2/3 empty lines

[erikwright (departed), 2014/06/16 20:51:27]

Done.

+  if (!unprotected_prefs_altered && !protected_prefs_altered) {

[gab, 2014/06/13 01:57:44]

So you will always cleanup the hashes in the next run following a successful
migration rather than try to do it in the same run?

I agree that this may be simpler.

FTR, the reason we attempt to cleanup values in the same run is that leaving old
values behind is problematic in some scenarios, e.g.:
 - Run #1 (unprotected to protected) => values are copied (new values are saved
to protected).
 - Run #2 (protected to unprotected) => old unprotected pref applies without
migration if its still around :(
and we want to minimize the number of users affected by this as much as possible
(we can't make it zero because clean up is still a best effort operation that
can only occur when we have confirmation of the destination's store's write (so
that the number of users losing their settings is zero) -- i.e., we prefer some
users getting their old settings back in failed cleanup scenarios to some users
losing their settings in failed migration write scenarios).

Whereas the way your MAC migration work I think this is okay. As long as the
value itself was cleaned up from the old store, the MAC will be migrated back
with its value, overwriting the MAC left behind, should its value's destination
store be flipped back to the old store.
The MACs left behind are essentially never used and it's okay to wait until the
next run to clean them up since it makes code here simpler.

I just wanted to make sure that this was intentional and that we're on the same
page as to why it's done this way.

Please add a comment here explaining that this code is intended to run a cleanup
step in a run following a successful migration and why this is okay and simpler.

Tests for the scenarios described above will be necessary; I believe there
already exists cleanup tests for the values, you'll have to augment them to
support these scenarios in a world with MACs being added to the picture.

[erikwright (departed), 2014/06/16 20:51:27]

Yes it was intentional, and your understanding of why it's safe and how it works
is the same as mine.

+    CleanupMigratedHashes(unprotected_pref_names_,
+                          protected_pref_hash_store_.get(),
+                          protected_prefs_.get());
+    CleanupMigratedHashes(protected_pref_names_,
+                          unprotected_pref_hash_store_.get(),
+                          unprotected_prefs_.get());
+    legacy_pref_hash_store_->Reset();

[gab, 2014/06/13 01:57:44]

One impact of doing this is that we will lose our accumulated super MACs in
Local State and since we will most likely first run this code in NO_ENFORCEMENT
mode, the MACs will be migrated to Preferences without a super MAC. We will thus
be re-seeding super MACs from scratch when we enable enforcement...

This is probably okay as it would only prevent us from adding protection to
existing prefs in the short term (new prefs can be protected right away as
they'll be NULL when our framework first loads with them). But we should bring
this up in a discussion tomorrow to make sure everyone is aware of this fact.

One way to fix this could be to generate a super MAC for unprotected preferences
if and only if no preferences are protected and have some sort of migration for
the super MAC, but that doesn't sound trivial to implement.

[erikwright (departed), 2014/06/16 20:51:27]

I believe that one of the side effects of this CL is that we now write a super
MAC even if the Protected store is empty. That may be a bad idea, as that super
MAC could be used at any time in the future to blindly TrustedInitialize all
values. An attacker could store it before protection is on or (if they are able,
even briefly, to trick Finch), generate it in the future.
Did we already discuss this? I don't think so; if not, we definitely should.

[gab, 2014/06/17 02:00:05]

This doesn't matter, an uninitialized  store is also untrusted (even if the
super MAC is valid); we should probably also not trust an initialized store with
no MACs in it; WDYT?
 
Yep, please ping us tomorrow for a chat.

[erikwright (departed), 2014/06/17 17:54:05]

Sure, but then probably we should not bother writing a super MAC that we
wouldn't trust anyways.

[gab, 2014/06/17 18:24:30]

True, but not trusting it in these scenarios no matter what is easier IMO.

+  }
 
   // Hand the processed prefs back to their respective filters.
   finalize_unprotected_filter_on_load_.Run(unprotected_prefs_.Pass(),
@@ -240,6 +315,7 @@ void TrackedPreferencesMigrator::MigrateIfReady() {
   }
 }
 
+

[gab, 2014/06/13 01:57:44]

nit: rm empty line

[erikwright (departed), 2014/06/16 20:51:27]

Done.

 }  // namespace
 
 void SetupTrackedPreferencesMigration(
@@ -252,6 +328,9 @@ void SetupTrackedPreferencesMigration(
         register_on_successful_unprotected_store_write_callback,
     const base::Callback<void(const base::Closure&)>&
         register_on_successful_protected_store_write_callback,
+    scoped_ptr<PrefHashStore> unprotected_pref_hash_store,
+    scoped_ptr<PrefHashStore> protected_pref_hash_store,
+    scoped_ptr<HashStoreContents> legacy_pref_hash_store,
     InterceptablePrefFilter* unprotected_pref_filter,
     InterceptablePrefFilter* protected_pref_filter) {
   scoped_refptr<TrackedPreferencesMigrator> prefs_migrator(
@@ -262,6 +341,9 @@ void SetupTrackedPreferencesMigration(
           protected_store_cleaner,
           register_on_successful_unprotected_store_write_callback,
           register_on_successful_protected_store_write_callback,
+          unprotected_pref_hash_store.Pass(),
+          protected_pref_hash_store.Pass(),
+          legacy_pref_hash_store.Pass(),
           unprotected_pref_filter,
           protected_pref_filter));
 }