Move preference MACs to the protected preference stores.

chrome/browser/prefs/pref_hash_store_impl.cc

 // Copyright 2013 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 "chrome/browser/prefs/pref_hash_store_impl.h"
 
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
 #include "base/values.h"
 #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
+#include "chrome/browser/prefs/tracked/dictionary_hash_store_contents.h"
 
 class PrefHashStoreImpl::PrefHashStoreTransactionImpl
     : public PrefHashStoreTransaction {
  public:
   // Constructs a PrefHashStoreTransactionImpl which can use the private
   // members of its |outer| PrefHashStoreImpl.
-  explicit PrefHashStoreTransactionImpl(PrefHashStoreImpl* outer);
+  PrefHashStoreTransactionImpl(PrefHashStoreImpl* outer,
+                               base::DictionaryValue* storage);
   virtual ~PrefHashStoreTransactionImpl();
 
   // PrefHashStoreTransaction implementation.
   virtual ValueState CheckValue(const std::string& path,
                                 const base::Value* value) const OVERRIDE;
   virtual void StoreHash(const std::string& path,
                          const base::Value* value) OVERRIDE;
+  virtual bool StampSuperMac() OVERRIDE;
   virtual ValueState CheckSplitValue(
       const std::string& path,
       const base::DictionaryValue* initial_split_value,
       std::vector<std::string>* invalid_keys) const OVERRIDE;
   virtual void StoreSplitHash(
       const std::string& path,
       const base::DictionaryValue* split_value) OVERRIDE;
+  virtual bool HasHash(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;
 
  private:
   bool GetSplitMacs(const std::string& path,
                     std::map<std::string, std::string>* split_macs) const;
+
   PrefHashStoreImpl* outer_;
-  bool has_changed_;
+  DictionaryHashStoreContents contents_;
+
+  bool super_mac_valid_;
+  bool super_mac_dirty_;
 
   DISALLOW_COPY_AND_ASSIGN(PrefHashStoreTransactionImpl);
 };
 
 PrefHashStoreImpl::PrefHashStoreImpl(const std::string& seed,
                                      const std::string& device_id,
-                                     scoped_ptr<HashStoreContents> contents,
                                      bool use_super_mac)
     : pref_hash_calculator_(seed, device_id),
-      contents_(contents.Pass()),
-      initial_hashes_dictionary_trusted_(
-          use_super_mac
-              ? IsHashDictionaryTrusted(pref_hash_calculator_, *contents_)
-              : false),
-      use_super_mac_(use_super_mac),
-      has_pending_write_(false) {
-  DCHECK(contents_);
-  UMA_HISTOGRAM_BOOLEAN("Settings.HashesDictionaryTrusted",

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

Regarding what to do with this histogram: I think we should expose an
IsSuperMacTrusted() method on the PrefHashStoreTransaction and let
FilterOnLoad() report it.

-                        initial_hashes_dictionary_trusted_);
+      use_super_mac_(use_super_mac) {
 }
 
-PrefHashStoreImpl::~PrefHashStoreImpl() {}
-
-void PrefHashStoreImpl::Reset() {
-  contents_->Reset();
+PrefHashStoreImpl::~PrefHashStoreImpl() {
 }
 
-scoped_ptr<PrefHashStoreTransaction> PrefHashStoreImpl::BeginTransaction() {
+scoped_ptr<PrefHashStoreTransaction> PrefHashStoreImpl::BeginTransaction(
+    base::DictionaryValue* storage) {
   return scoped_ptr<PrefHashStoreTransaction>(
-      new PrefHashStoreTransactionImpl(this));
-}
-
-void PrefHashStoreImpl::CommitPendingWrite() {
-  if (has_pending_write_) {
-    contents_->CommitPendingWrite();
-    has_pending_write_ = false;
-  }
+      new PrefHashStoreTransactionImpl(this, storage));
 }
 
 PrefHashStoreImpl::PrefHashStoreTransactionImpl::PrefHashStoreTransactionImpl(
-    PrefHashStoreImpl* outer) : outer_(outer), has_changed_(false) {
+    PrefHashStoreImpl* outer,
+    base::DictionaryValue* storage)
+    : outer_(outer),
+      contents_(storage),
+      super_mac_valid_(false),
+      super_mac_dirty_(false) {
+  if (outer_->use_super_mac_) {
+    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.
+    super_mac_valid_ =

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

So if a previous transaction stamped a super MAC, all follow-up transactions
will trust the store, this *may* be okay since this state only matters in the
initial FilterOnLoad(), but this is not the way this used to work (where the
initial trust was the only thing that mattered throughout the lifetime of the
PrefHashStore).

Something to keep in mind as we refine this CL.

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

Yeah, there is kind of limited choice. I don't think it's appropriate to leave
state in the PrefHashStore instance since it is no longer bound to a single
underlying storage (even if, in principle, it will only be used with one
storage).

I had considered introducing PrefHashStore::BeginFiltering which would
essentially be BeginTransaction but with the side-effect of updating the super
MAC. But I don't think it's that useful unless it provides the _only_ way to
perform the operations associated with filtering
(Check(Split)Value/Store(Split)Hash).

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

How about adding a PrefHashFilter::filter_on_load_performed_ member to
PrefHashFilter and DCHECK'ing that FilterOnLoad() only happens once?

This depends on that fact (and much of the framework kind of assumes this as
well).

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

Personally, I'm not worried about a single instance of PrefHashFilter calling
filter_on_load twice. The real risk is that something in the migration code ends
up triggering an unintended super MAC update, given that that component has a
completely distinct instance of PrefHashStore.

To protect against this, I would change the object model for a PrefHashStore to
guarantee that it can only be used once for load-time enforcement. For example,
forcing you to give up your only reference to the object at the end of
enforcement.

I don't think it's critical at the moment. Probably gold-plating, especially if
there are regression tests to catch it.

But I definitely think the boolean "did this once" flag is at best superficial
and a false sense of security.

+        store_contents && !super_mac.empty() &&
+        outer_->pref_hash_calculator_.Validate(
+            contents_.hash_store_id(), store_contents, super_mac) ==
+            PrefHashCalculator::VALID;
+  }
 }
 
 PrefHashStoreImpl::PrefHashStoreTransactionImpl::
     ~PrefHashStoreTransactionImpl() {
   // Update the super MAC if and only if the hashes dictionary has been
   // modified in this transaction.

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

Remove the comment above, the new variable name makes this explicit :-)!

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

Done.

-  if (has_changed_) {
-    if (outer_->use_super_mac_) {
-      // 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;
+  if (super_mac_dirty_ && outer_->use_super_mac_) {
+    // Get the dictionary of hashes (or NULL if it doesn't exist).
+    const base::DictionaryValue* hashes_dict = contents_.GetContents();
+    contents_.SetSuperMac(outer_->pref_hash_calculator_.Calculate(
+        contents_.hash_store_id(), hashes_dict));
   }
-
 }
 
 PrefHashStoreTransaction::ValueState
 PrefHashStoreImpl::PrefHashStoreTransactionImpl::CheckValue(
-    const std::string& path, const base::Value* initial_value) const {
-  const base::DictionaryValue* hashed_prefs = outer_->contents_->GetContents();
+    const std::string& path,
+    const base::Value* initial_value) const {
+  const base::DictionaryValue* hashed_prefs = contents_.GetContents();
 
   std::string last_hash;
   if (hashed_prefs)
     hashed_prefs->GetString(path, &last_hash);
 
   if (last_hash.empty()) {
     // 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 || outer_->initial_hashes_dictionary_trusted_) ?
-               TRUSTED_UNKNOWN_VALUE : UNTRUSTED_UNKNOWN_VALUE;
+    return (!initial_value || super_mac_valid_) ? TRUSTED_UNKNOWN_VALUE
+                                                : UNTRUSTED_UNKNOWN_VALUE;
   }
 
   PrefHashCalculator::ValidationResult validation_result =
       outer_->pref_hash_calculator_.Validate(path, initial_value, last_hash);
   switch (validation_result) {
     case PrefHashCalculator::VALID:
       return UNCHANGED;
     case PrefHashCalculator::VALID_WEAK_LEGACY:
       return WEAK_LEGACY;
     case PrefHashCalculator::VALID_SECURE_LEGACY:
       return SECURE_LEGACY;
     case PrefHashCalculator::INVALID:
       return initial_value ? CHANGED : CLEARED;
   }
   NOTREACHED() << "Unexpected PrefHashCalculator::ValidationResult: "
                << validation_result;
   return UNTRUSTED_UNKNOWN_VALUE;
 }
 
 void PrefHashStoreImpl::PrefHashStoreTransactionImpl::StoreHash(
-    const std::string& path, const base::Value* new_value) {
+    const std::string& path,
+    const base::Value* new_value) {
   const std::string mac =
       outer_->pref_hash_calculator_.Calculate(path, new_value);
-  (*outer_->contents_->GetMutableContents())->SetString(path, mac);
-  has_changed_ = true;
+  (*contents_.GetMutableContents())->SetString(path, mac);
+  super_mac_dirty_ = true;
+}
+
+bool PrefHashStoreImpl::PrefHashStoreTransactionImpl::HasHash(
+    const std::string& path) const {
+  const base::DictionaryValue* hashed_prefs = contents_.GetContents();
+  return hashed_prefs && hashed_prefs->Get(path, NULL);
+}
+
+void PrefHashStoreImpl::PrefHashStoreTransactionImpl::ImportHash(
+    const std::string& path,
+    const base::Value* hash) {
+  if (!hash)

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

Start if/else block with the positive condition when possible (so I was told to
do by my C++ readability reviewer).

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

Done.

+    (*contents_.GetMutableContents())->RemovePath(path, NULL);
+  else
+    (*contents_.GetMutableContents())->Set(path, hash->DeepCopy());
+  super_mac_dirty_ = super_mac_dirty_ || super_mac_valid_;

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

I find:

if (super_mac_valid_)
  super_mac_dirty_ = true;

to be more readable here (and I suspect it may even generate slightly more
efficient assembly code since it doesn't always force a write).

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

Done.

+}
+
+void PrefHashStoreImpl::PrefHashStoreTransactionImpl::ClearHash(
+    const std::string& path) {
+  if ((*contents_.GetMutableContents())->RemovePath(path, NULL) &&
+      super_mac_valid_) {
+    super_mac_dirty_ = true;
+  }
+}
+
+bool PrefHashStoreImpl::PrefHashStoreTransactionImpl::StampSuperMac() {
+  if (!outer_->use_super_mac_ || super_mac_valid_)
+    return false;
+  super_mac_dirty_ = true;
+  return true;
 }
 
 PrefHashStoreTransaction::ValueState
 PrefHashStoreImpl::PrefHashStoreTransactionImpl::CheckSplitValue(
     const std::string& path,
     const base::DictionaryValue* initial_split_value,
     std::vector<std::string>* invalid_keys) const {
   DCHECK(invalid_keys && invalid_keys->empty());
 
   std::map<std::string, std::string> split_macs;
   const bool has_hashes = GetSplitMacs(path, &split_macs);
 
   // Treat NULL and empty the same; otherwise we would need to store a hash
   // for the entire dictionary (or some other special beacon) to
   // differentiate these two cases which are really the same for
   // dictionaries.
   if (!initial_split_value || initial_split_value->empty())
     return has_hashes ? CLEARED : UNCHANGED;
 
-  if (!has_hashes) {
-    return outer_->initial_hashes_dictionary_trusted_ ?
-        TRUSTED_UNKNOWN_VALUE : UNTRUSTED_UNKNOWN_VALUE;
-  }
+  if (!has_hashes)
+    return super_mac_valid_ ? TRUSTED_UNKNOWN_VALUE : UNTRUSTED_UNKNOWN_VALUE;
 
   bool has_secure_legacy_id_hashes = false;
   std::string keyed_path(path);
   keyed_path.push_back('.');
   const size_t common_part_length = keyed_path.length();
   for (base::DictionaryValue::Iterator it(*initial_split_value); !it.IsAtEnd();
        it.Advance()) {
     std::map<std::string, std::string>::iterator entry =
         split_macs.find(it.key());
     if (entry == split_macs.end()) {
@@ skipping 31 matching lines @@
 
   // Anything left in the map is missing from the data.
   for (std::map<std::string, std::string>::const_iterator it =
            split_macs.begin();
        it != split_macs.end();
        ++it) {
     invalid_keys->push_back(it->first);
   }
 
   return invalid_keys->empty()
-      ? (has_secure_legacy_id_hashes ? SECURE_LEGACY : UNCHANGED)
-      : CHANGED;
+             ? (has_secure_legacy_id_hashes ? SECURE_LEGACY : UNCHANGED)
+             : CHANGED;
 }
 
 void PrefHashStoreImpl::PrefHashStoreTransactionImpl::StoreSplitHash(
     const std::string& path,
     const base::DictionaryValue* split_value) {
   scoped_ptr<HashStoreContents::MutableDictionary> mutable_dictionary =
-      outer_->contents_->GetMutableContents();
+      contents_.GetMutableContents();
   (*mutable_dictionary)->Remove(path, NULL);
 
   if (split_value) {
     std::string keyed_path(path);
     keyed_path.push_back('.');
     const size_t common_part_length = keyed_path.length();
     for (base::DictionaryValue::Iterator it(*split_value); !it.IsAtEnd();
          it.Advance()) {
       // Keep the common part from the old |keyed_path| and replace the key to
       // get the new |keyed_path|.
       keyed_path.replace(common_part_length, std::string::npos, it.key());
       (*mutable_dictionary)->SetString(
           keyed_path,
           outer_->pref_hash_calculator_.Calculate(keyed_path, &it.value()));
     }
   }
-  has_changed_ = true;
+  super_mac_dirty_ = true;
 }
 
 bool PrefHashStoreImpl::PrefHashStoreTransactionImpl::GetSplitMacs(
     const std::string& key,
     std::map<std::string, std::string>* split_macs) const {
   DCHECK(split_macs);
   DCHECK(split_macs->empty());
 
-  const base::DictionaryValue* hashed_prefs = outer_->contents_->GetContents();
+  const base::DictionaryValue* hashed_prefs = contents_.GetContents();
   const base::DictionaryValue* split_mac_dictionary = NULL;
   if (!hashed_prefs || !hashed_prefs->GetDictionary(key, &split_mac_dictionary))
     return false;
   for (base::DictionaryValue::Iterator it(*split_mac_dictionary); !it.IsAtEnd();
        it.Advance()) {
     std::string mac_string;
     if (!it.value().GetAsString(&mac_string)) {
       NOTREACHED();
       continue;
     }
     split_macs->insert(make_pair(it.key(), mac_string));
   }
   return true;
 }