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
-
 class PrefHashStoreImpl::PrefHashStoreTransactionImpl
     : public PrefHashStoreTransaction {
  public:
   // Constructs a PrefHashStoreTransactionImpl which can use the private
   // members of its |outer| PrefHashStoreImpl.
   explicit PrefHashStoreTransactionImpl(PrefHashStoreImpl* outer);
   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 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,
       std::vector<std::string>* invalid_keys) const OVERRIDE;
   virtual void StoreSplitHash(
       const std::string& path,
       const base::DictionaryValue* split_value) OVERRIDE;
 
  private:
   bool GetSplitMacs(const std::string& path,
                     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();
 }
 
 scoped_ptr<PrefHashStoreTransaction> PrefHashStoreImpl::BeginTransaction() {
   return scoped_ptr<PrefHashStoreTransaction>(
       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();
     has_pending_write_ = false;
   }
 }
 
 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
 PrefHashStoreImpl::PrefHashStoreTransactionImpl::CheckValue(
     const std::string& path, const base::Value* initial_value) const {
   const base::DictionaryValue* hashed_prefs = outer_->contents_->GetContents();
 
   std::string last_hash;
   if (hashed_prefs)
     hashed_prefs->GetString(path, &last_hash);
 
   if (last_hash.empty()) {
@@ skipping 19 matching lines @@
                << validation_result;
   return UNTRUSTED_UNKNOWN_VALUE;
 }
 
 void PrefHashStoreImpl::PrefHashStoreTransactionImpl::StoreHash(
     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;
+  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
 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;
@@ skipping 80 matching lines @@
          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* 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;
 }