Move preference MACs to the protected preference stores.

chrome/browser/prefs/pref_hash_store_impl.cc

Index: chrome/browser/prefs/pref_hash_store_impl.cc
diff --git a/chrome/browser/prefs/pref_hash_store_impl.cc b/chrome/browser/prefs/pref_hash_store_impl.cc
index 096cbd9470b739d71403d929c9c5a1d745db16de..85eb93ed1a8a430adc7d99aebff5737b55c33be5 100644
--- a/chrome/browser/prefs/pref_hash_store_impl.cc
+++ b/chrome/browser/prefs/pref_hash_store_impl.cc
@@ -10,23 +10,6 @@
 #include "chrome/browser/prefs/pref_hash_store_transaction.h"
 #include "chrome/browser/prefs/tracked/hash_store_contents.h"
 
-namespace {
-
-// Returns true if the dictionary of hashes stored in |contents| is trusted
-// (which implies unknown values can be trusted as newly tracked values).
-bool IsHashDictionaryTrusted(const PrefHashCalculator& calculator,
-                             const HashStoreContents& contents) {
-  const base::DictionaryValue* store_contents = contents.GetContents();
-  std::string super_mac = contents.GetSuperMac();
-  // The store must be initialized and have a valid super MAC to be trusted.
-  return store_contents && !super_mac.empty() &&
-         calculator.Validate(contents.hash_store_id(),
-                             store_contents,
-                             super_mac) == PrefHashCalculator::VALID;
-}
-
-}  // namespace
-
 class PrefHashStoreImpl::PrefHashStoreTransactionImpl
     : public PrefHashStoreTransaction {
  public:
@@ -40,6 +23,12 @@ class PrefHashStoreImpl::PrefHashStoreTransactionImpl
                                 const base::Value* value) const OVERRIDE;
   virtual void StoreHash(const std::string& path,
                          const base::Value* value) OVERRIDE;
+  virtual const base::Value* GetHash(const std::string& path) const OVERRIDE;
+  virtual void ImportHash(const std::string& path,
+                          const base::Value* hash) OVERRIDE;
+  virtual void ClearHash(const std::string& path) OVERRIDE;
+  virtual bool StampSuperMac() OVERRIDE;
+
   virtual ValueState CheckSplitValue(
       const std::string& path,
       const base::DictionaryValue* initial_split_value,
@@ -53,25 +42,41 @@ class PrefHashStoreImpl::PrefHashStoreTransactionImpl
                     std::map<std::string, std::string>* split_macs) const;
   PrefHashStoreImpl* outer_;
   bool has_changed_;
+  bool super_mac_dirty_;
 
   DISALLOW_COPY_AND_ASSIGN(PrefHashStoreTransactionImpl);
 };
 
 PrefHashStoreImpl::PrefHashStoreImpl(const std::string& seed,
-                                     const std::string& device_id,
-                                     scoped_ptr<HashStoreContents> contents)
+                                     const std::string& device_id)
     : pref_hash_calculator_(seed, device_id),
-      contents_(contents.Pass()),
-      initial_hashes_dictionary_trusted_(
-          IsHashDictionaryTrusted(pref_hash_calculator_, *contents_)),
+      initial_hashes_dictionary_trusted_(false),
       has_pending_write_(false) {
-  DCHECK(contents_);
   UMA_HISTOGRAM_BOOLEAN("Settings.HashesDictionaryTrusted",
                         initial_hashes_dictionary_trusted_);

[gab, 2014/06/06 21:54:59]

This histogram should also move.

[erikwright (departed), 2014/06/10 20:27:47]

Done.

 }
 
 PrefHashStoreImpl::~PrefHashStoreImpl() {}
 
+void PrefHashStoreImpl::SetHashStoreContents(

[gab, 2014/06/06 21:54:59]

Call this method Init()? And explicitly state that it MUST be called before
using this PrefHashStoreImpl (as the rest of the class assumes its set).

Note: In my proposed model this method is not required as there is no cyclic
dependency; I find that much cleaner.

+    scoped_ptr<HashStoreContents> contents) {
+  contents_ = contents.Pass();
+
+  const base::DictionaryValue* store_contents = contents_->GetContents();
+  std::string super_mac = contents_->GetSuperMac();
+
+  // The store must be initialized and have a valid super MAC to be trusted.
+  initial_hashes_dictionary_trusted_ =
+      store_contents && !super_mac.empty() &&
+      pref_hash_calculator_.Validate(contents_->hash_store_id(),
+                                     store_contents,
+                                     super_mac) == PrefHashCalculator::VALID;
+}
+
+bool PrefHashStoreImpl::IsInitialized() const {
+  return contents_->IsInitialized();
+}
+
 void PrefHashStoreImpl::Reset() {
   contents_->Reset();
 }
@@ -81,19 +86,6 @@ scoped_ptr<PrefHashStoreTransaction> PrefHashStoreImpl::BeginTransaction() {
       new PrefHashStoreTransactionImpl(this));
 }
 
-PrefHashStoreImpl::StoreVersion PrefHashStoreImpl::GetCurrentVersion() const {
-  if (!contents_->IsInitialized())
-    return VERSION_UNINITIALIZED;
-
-  int current_version;
-  if (!contents_->GetVersion(&current_version)) {
-    return VERSION_PRE_MIGRATION;
-  }
-
-  DCHECK_GT(current_version, VERSION_PRE_MIGRATION);
-  return static_cast<StoreVersion>(current_version);
-}
-
 void PrefHashStoreImpl::CommitPendingWrite() {
   if (has_pending_write_) {
     contents_->CommitPendingWrite();
@@ -102,35 +94,36 @@ void PrefHashStoreImpl::CommitPendingWrite() {
 }
 
 PrefHashStoreImpl::PrefHashStoreTransactionImpl::PrefHashStoreTransactionImpl(
-    PrefHashStoreImpl* outer) : outer_(outer), has_changed_(false) {
+    PrefHashStoreImpl* outer)
+    : outer_(outer), has_changed_(false), super_mac_dirty_(false) {
 }
 
 PrefHashStoreImpl::PrefHashStoreTransactionImpl::
     ~PrefHashStoreTransactionImpl() {
-  // Update the super MAC if and only if the hashes dictionary has been
-  // modified in this transaction.
   if (has_changed_) {
-    // Get the dictionary of hashes (or NULL if it doesn't exist).
-    const base::DictionaryValue* hashes_dict = outer_->contents_->GetContents();
-    outer_->contents_->SetSuperMac(outer_->pref_hash_calculator_.Calculate(
-        outer_->contents_->hash_store_id(), hashes_dict));
+    // Update the super MAC if and only if the hashes dictionary has been
+    // modified in this transaction. Imports do not necessarily affect the super
+    // MAC (will not transition it from invalid to valid).
+    if (super_mac_dirty_) {
+      // Get the dictionary of hashes (or NULL if it doesn't exist).
+      const base::DictionaryValue* hashes_dict =
+          outer_->contents_->GetContents();
+      outer_->contents_->SetSuperMac(outer_->pref_hash_calculator_.Calculate(
+          outer_->contents_->hash_store_id(), hashes_dict));
+    }
 
     outer_->has_pending_write_ = true;
   }
+}
 
-  // Mark this hash store has having been updated to the latest version (in
-  // practice only initialization transactions will actually do this, but
-  // since they always occur before minor update transaction it's okay
-  // to unconditionally do this here). Only do this if this store's version
-  // isn't already at VERSION_LATEST (to avoid scheduling a write when
-  // unecessary). Note, this is outside of |if (has_changed)| to also seed
-  // version number of otherwise unchanged profiles.
-  int current_version;
-  if (!outer_->contents_->GetVersion(&current_version) ||
-      current_version != VERSION_LATEST) {
-    outer_->contents_->SetVersion(VERSION_LATEST);
-    outer_->has_pending_write_ = true;
-  }
+const base::Value* PrefHashStoreImpl::PrefHashStoreTransactionImpl::GetHash(
+    const std::string& path) const {
+  const base::DictionaryValue* hashed_prefs = outer_->contents_->GetContents();
+
+  const base::Value* last_hash = NULL;
+  if (hashed_prefs)
+    hashed_prefs->Get(path, &last_hash);
+  return last_hash;
 }
 
 PrefHashStoreTransaction::ValueState
@@ -172,6 +165,35 @@ void PrefHashStoreImpl::PrefHashStoreTransactionImpl::StoreHash(
       outer_->pref_hash_calculator_.Calculate(path, new_value);
   (*outer_->contents_->GetMutableContents())->SetString(path, mac);
   has_changed_ = true;
+  super_mac_dirty_ = true;
+}
+
+void PrefHashStoreImpl::PrefHashStoreTransactionImpl::ImportHash(
+    const std::string& path,
+    const base::Value* hash) {

[gab, 2014/06/06 21:54:59]

I think |hash| should be passed as a std::string. The fact that it's stored as a
Value* is not relevant to this API. Passing it by pointer could perhaps save us
a few string copies, but in this case it doesn't even help with that either
since you have to DeepCopy() the Value* below anyways.

So I say pass it by std::string and use SetString below (this class is well
aware that hashes are stored as strings anyways).

[erikwright (departed), 2014/06/09 18:34:50]

That requires separate pairs of methods for atomic and split prefs. The benefit
of using a Value is that it works for either a dictionary or a string.

I could also introduce a purpose-built type (PreferenceMac, say) that would hold
either a string or a dictionary.

Returning anything other than a Value requires me to copy everything that comes
out. Probably not a performance issue, but it will be extra code.

Let me know what you think, given that.

[gab, 2014/06/09 20:24:33]

Duh, of course, passing it via a Value object is fine to me then.

Perhaps renaming the parameter and method from "hash" to something more generic
like "hash_content" would help here; otherwise a dictionary is not really a hash
per se.

How about:

ImportHashContent(const std::string& path, const base::Value* hash_content);

[erikwright (departed), 2014/06/10 20:27:47]

I thought about that, but it's consistent with StoreSplitHash.

+  if (!hash)
+    (*outer_->contents_->GetMutableContents())->RemovePath(path, NULL);
+  else
+    (*outer_->contents_->GetMutableContents())->Set(path, hash->DeepCopy());
+  has_changed_ = true;
+  super_mac_dirty_ =
+      super_mac_dirty_ || outer_->initial_hashes_dictionary_trusted_;

[gab, 2014/06/06 21:54:59]

It's incorrect to re-write the super MAC here if the super MAC of the source
store is not trusted.

I think it's possible in the current model for an attacker to completely delete
this hash store, put a single hash for an irrelevant pref in the other store,
have that hash be migrated and generate a new super MAC... and then we happily
TrustInitialize every other pref... :(.


The way I see super MACs working in the model I proposed is as follows:
TrackedPreferencesMigrator can easily keep track of which stores it pushed new
MACs into. It can thus re-generate super MACs for those stores if and only if
the super MAC for both the source store(s) and the destination store were
previously valid.

[erikwright (departed), 2014/06/09 18:34:50]

We talked about this offline. It's correct to keep the super MAC validity if and
only if the target store has a valid super MAC.

+}
+
+void PrefHashStoreImpl::PrefHashStoreTransactionImpl::ClearHash(
+    const std::string& path) {
+  (*outer_->contents_->GetMutableContents())->RemovePath(path, NULL);
+  has_changed_ = true;
+  super_mac_dirty_ =
+      super_mac_dirty_ || outer_->initial_hashes_dictionary_trusted_;
+}
+
+bool PrefHashStoreImpl::PrefHashStoreTransactionImpl::StampSuperMac() {
+  if (outer_->initial_hashes_dictionary_trusted_)
+    return false;
+  has_changed_ = true;
+  super_mac_dirty_ = true;
+  return true;
 }
 
 PrefHashStoreTransaction::ValueState
@@ -272,6 +294,7 @@ void PrefHashStoreImpl::PrefHashStoreTransactionImpl::StoreSplitHash(
     }
   }
   has_changed_ = true;
+  super_mac_dirty_ = true;
 }
 
 bool PrefHashStoreImpl::PrefHashStoreTransactionImpl::GetSplitMacs(