Fix several problems with the content verification code

-While we're doing the initial scan of files to build up
 computed_hashes.json, if we notice a mismatch, report that right
 away. This also fixes problems where if you modify/delete
 computed_hashes.json, enforcement would never work.

-Avoid an infinite loop in ContentVerifyJob by making sure we stop
 checking newly read bytes after we've already reported a failure
 (we were stuck failing to advance to the next block after reporting
  a failure)

BUG=375992,375953

Review URL: https://codereview.chromium.org/329303007

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@278071 0039d316-1c4b-4281-b951-d872f2087c98

extensions/browser/content_hash_fetcher.h

@@ -5,6 +5,11 @@
 #ifndef EXTENSIONS_BROWSER_CONTENT_HASH_FETCHER_H_
 #define EXTENSIONS_BROWSER_CONTENT_HASH_FETCHER_H_
 
+#include <set>
+#include <string>
+
+#include "base/callback.h"
+#include "base/files/file_path.h"
 #include "base/memory/weak_ptr.h"
 #include "base/scoped_observer.h"
 #include "extensions/browser/content_verifier_delegate.h"
@@ -28,18 +33,31 @@ class ContentHashFetcherJob;
 // sure they match the signed treehash root hash).
 class ContentHashFetcher : public ExtensionRegistryObserver {
  public:
+  // A callback for when a fetch is complete. This reports back:
+  // -extension id
+  // -whether we were successful or not (have verified_contents.json and
+  // -computed_hashes.json files)
+  // -was it a forced check?
+  // -a set of paths whose contents didn't match expected values
+  typedef base::Callback<
+      void(const std::string&, bool, bool, const std::set<base::FilePath>&)>
+      FetchCallback;
+
   // The consumer of this class needs to ensure that context and delegate
   // outlive this object.
   ContentHashFetcher(content::BrowserContext* context,
-                     ContentVerifierDelegate* delegate);
+                     ContentVerifierDelegate* delegate,
+                     const FetchCallback& callback);
   virtual ~ContentHashFetcher();
 
   // Begins the process of trying to fetch any needed verified contents, and
   // listening for extension load/unload.
   void Start();
 
-  // Explicitly ask to fetch hashes for |extension|.
-  void DoFetch(const Extension* extension);
+  // Explicitly ask to fetch hashes for |extension|. If |force| is true,
+  // we will always check the validity of the verified_contents.json and
+  // re-check the contents of the files in the filesystem.
+  void DoFetch(const Extension* extension, bool force);
 
   // ExtensionRegistryObserver interface
   virtual void OnExtensionLoaded(content::BrowserContext* browser_context,
@@ -55,6 +73,7 @@ class ContentHashFetcher : public ExtensionRegistryObserver {
 
   content::BrowserContext* context_;
   ContentVerifierDelegate* delegate_;
+  FetchCallback fetch_callback_;
 
   // We keep around pointers to in-progress jobs, both so we can avoid
   // scheduling duplicate work if fetching is already in progress, and so that

extensions/browser/content_hash_reader.cc

@@ -33,6 +33,8 @@ ContentHashReader::ContentHashReader(const std::string& extension_id,
       relative_path_(relative_path),
       key_(key),
       status_(NOT_INITIALIZED),
+      have_verified_contents_(false),
+      have_computed_hashes_(false),
       block_size_(0) {
 }
 
@@ -52,10 +54,10 @@ bool ContentHashReader::Init() {
   if (!verified_contents_->InitFrom(verified_contents_path, false) ||
       !verified_contents_->valid_signature() ||
       !verified_contents_->version().Equals(extension_version_) ||
-      verified_contents_->extension_id() != extension_id_) {
-    base::DeleteFile(verified_contents_path, false /* recursive */);
+      verified_contents_->extension_id() != extension_id_)
     return false;
-  }
+
+  have_verified_contents_ = true;
 
   base::FilePath computed_hashes_path =
       file_util::GetComputedHashesPath(extension_root_);
@@ -63,21 +65,24 @@ bool ContentHashReader::Init() {
     return false;
 
   ComputedHashes::Reader reader;
-  if (!reader.InitFromFile(computed_hashes_path) ||
-      !reader.GetHashes(relative_path_, &block_size_, &hashes_) ||
-      block_size_ % crypto::kSHA256Length != 0) {
-    base::DeleteFile(computed_hashes_path, false /* recursive */);
+  if (!reader.InitFromFile(computed_hashes_path))
+    return false;
+
+  have_computed_hashes_ = true;
+
+  if (!reader.GetHashes(relative_path_, &block_size_, &hashes_) ||
+      block_size_ % crypto::kSHA256Length != 0)
+    return false;
+
+  const std::string* expected_root =
+      verified_contents_->GetTreeHashRoot(relative_path_);
+  if (!expected_root)
     return false;
-  }
 
   std::string root =
       ComputeTreeHashRoot(hashes_, block_size_ / crypto::kSHA256Length);
-  const std::string* expected_root = NULL;
-  expected_root = verified_contents_->GetTreeHashRoot(relative_path_);
-  if (expected_root && *expected_root != root) {
-    base::DeleteFile(computed_hashes_path, false /* recursive */);
+  if (*expected_root != root)
     return false;
-  }
 
   status_ = SUCCESS;
   return true;

extensions/browser/content_hash_reader.h

@@ -40,6 +40,12 @@ class ContentHashReader : public base::RefCountedThreadSafe<ContentHashReader> {
   // should likely be discarded.
   bool Init();
 
+  // These return whether we found valid verified_contents.json /
+  // computed_hashes.json files respectively. Note that both of these can be
+  // true but we still didn't find an entry for |relative_path_| in them.
+  bool have_verified_contents() { return have_verified_contents_; }
+  bool have_computed_hashes() { return have_computed_hashes_; }
+
   // Return the number of blocks and block size, respectively. Only valid after
   // calling Init().
   int block_count() const;
@@ -63,6 +69,9 @@ class ContentHashReader : public base::RefCountedThreadSafe<ContentHashReader> {
 
   InitStatus status_;
 
+  bool have_verified_contents_;
+  bool have_computed_hashes_;
+
   // The blocksize used for generating the hashes.
   int block_size_;
 

extensions/browser/content_verifier.cc

@@ -32,7 +32,10 @@ ContentVerifier::ContentVerifier(content::BrowserContext* context,
     : mode_(GetMode()),
       context_(context),
       delegate_(delegate),
-      fetcher_(new ContentHashFetcher(context, delegate)) {
+      fetcher_(new ContentHashFetcher(
+          context,
+          delegate,
+          base::Bind(&ContentVerifier::OnFetchComplete, this))) {
 }
 
 ContentVerifier::~ContentVerifier() {
@@ -59,33 +62,11 @@ ContentVerifyJob* ContentVerifier::CreateJobFor(
   const Extension* extension =
       registry->GetExtensionById(extension_id, ExtensionRegistry::EVERYTHING);
 
-  if (!extension || !extension->version() ||
-      !delegate_->ShouldBeVerified(*extension))
-    return NULL;
-
-  // Images used in the browser get transcoded during install, so skip checking
-  // them for now.  TODO(asargent) - see if we can cache this list for a given
-  // extension id/version pair.
-  std::set<base::FilePath> browser_images =
-      delegate_->GetBrowserImagePaths(extension);
-  if (ContainsKey(browser_images, relative_path))
+  std::set<base::FilePath> paths;
+  paths.insert(relative_path);
+  if (!ShouldVerifyAnyPaths(extension, paths))
     return NULL;
 
-  base::FilePath locales_dir = extension_root.Append(kLocaleFolder);
-  base::FilePath full_path = extension_root.Append(relative_path);
-  if (locales_dir.IsParent(full_path)) {
-    // TODO(asargent) - see if we can cache this list to avoid having to fetch
-    // it every time. Maybe it can never change at runtime? (Or if it can,
-    // maybe there is an event we can listen for to know to drop our cache).
-    std::set<std::string> all_locales;
-    extension_l10n_util::GetAllLocales(&all_locales);
-    // Since message catalogs get transcoded during installation, we want to
-    // ignore only those paths that the localization transcoding *did* ignore.
-    if (!extension_l10n_util::ShouldSkipValidation(
-            locales_dir, full_path, all_locales))
-      return NULL;
-  }
-
   // TODO(asargent) - we can probably get some good performance wins by having
   // a cache of ContentHashReader's that we hold onto past the end of each job.
   return new ContentVerifyJob(
@@ -107,21 +88,96 @@ void ContentVerifier::VerifyFailed(const std::string& extension_id,
     return;
   }
 
-  if (!delegate_ || mode_ < ENFORCE)
+  VLOG(1) << "VerifyFailed " << extension_id << " reason:" << reason;
+
+  if (!delegate_ || !fetcher_.get() || mode_ < ENFORCE)
     return;
 
-  if (reason == ContentVerifyJob::NO_HASHES && mode_ < ENFORCE_STRICT &&
-      fetcher_.get()) {
+  if (reason == ContentVerifyJob::MISSING_ALL_HASHES) {
     // If we failed because there were no hashes yet for this extension, just
     // request some.
     ExtensionRegistry* registry = ExtensionRegistry::Get(context_);
     const Extension* extension =
         registry->GetExtensionById(extension_id, ExtensionRegistry::EVERYTHING);
     if (extension)
-      fetcher_->DoFetch(extension);
+      fetcher_->DoFetch(extension, true /* force */);
+  } else {
+    delegate_->VerifyFailed(extension_id);
+  }
+}
+
+void ContentVerifier::OnFetchComplete(
+    const std::string& extension_id,
+    bool success,
+    bool was_force_check,
+    const std::set<base::FilePath>& hash_mismatch_paths) {
+  VLOG(1) << "OnFetchComplete " << extension_id << " success:" << success;
+
+  if (!delegate_ || mode_ < ENFORCE)
     return;
+
+  if (!success && mode_ < ENFORCE_STRICT)
+    return;
+
+  ExtensionRegistry* registry = ExtensionRegistry::Get(context_);
+  const Extension* extension =
+      registry->GetExtensionById(extension_id, ExtensionRegistry::EVERYTHING);
+  if (!extension)
+    return;
+
+  if ((was_force_check && !success) ||
+      ShouldVerifyAnyPaths(extension, hash_mismatch_paths))
+    delegate_->VerifyFailed(extension_id);
+}
+
+bool ContentVerifier::ShouldVerifyAnyPaths(
+    const Extension* extension,
+    const std::set<base::FilePath>& relative_paths) {
+  if (!extension || !extension->version() ||
+      !delegate_->ShouldBeVerified(*extension))
+    return false;
+
+  // Images used in the browser get transcoded during install, so skip
+  // checking them for now.  TODO(asargent) - see if we can cache this list
+  // for a given extension id/version pair.
+  std::set<base::FilePath> browser_images =
+      delegate_->GetBrowserImagePaths(extension);
+
+  base::FilePath locales_dir = extension->path().Append(kLocaleFolder);
+  scoped_ptr<std::set<std::string> > all_locales;
+
+  for (std::set<base::FilePath>::const_iterator i = relative_paths.begin();
+       i != relative_paths.end();
+       ++i) {
+    const base::FilePath& relative_path = *i;
+
+    if (relative_path == base::FilePath(kManifestFilename))
+      continue;
+
+    if (ContainsKey(browser_images, relative_path))
+      continue;
+
+    base::FilePath full_path = extension->path().Append(relative_path);
+    if (locales_dir.IsParent(full_path)) {
+      if (!all_locales) {
+        // TODO(asargent) - see if we can cache this list longer to avoid
+        // having to fetch it more than once for a given run of the
+        // browser. Maybe it can never change at runtime? (Or if it can, maybe
+        // there is an event we can listen for to know to drop our cache).
+        all_locales.reset(new std::set<std::string>);
+        extension_l10n_util::GetAllLocales(all_locales.get());
+      }
+
+      // Since message catalogs get transcoded during installation, we want
+      // to skip those paths.
+      if (full_path.DirName().DirName() == locales_dir &&
+          !extension_l10n_util::ShouldSkipValidation(
+              locales_dir, full_path.DirName(), *all_locales))
+        continue;
+    }
+    return true;
   }
-  delegate_->VerifyFailed(extension_id);
+  return false;
 }
 
 // static

extensions/browser/content_verifier.h

@@ -5,6 +5,9 @@
 #ifndef EXTENSIONS_BROWSER_CONTENT_VERIFIER_H_
 #define EXTENSIONS_BROWSER_CONTENT_VERIFIER_H_
 
+#include <set>
+#include <string>
+
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
@@ -46,12 +49,24 @@ class ContentVerifier : public base::RefCountedThreadSafe<ContentVerifier> {
   void VerifyFailed(const std::string& extension_id,
                     ContentVerifyJob::FailureReason reason);
 
+  void OnFetchComplete(const std::string& extension_id,
+                       bool success,
+                       bool was_force_check,
+                       const std::set<base::FilePath>& hash_mismatch_paths);
+
  private:
   DISALLOW_COPY_AND_ASSIGN(ContentVerifier);
 
   friend class base::RefCountedThreadSafe<ContentVerifier>;
   virtual ~ContentVerifier();
 
+  // Returns true if any of the paths in |relative_paths| *should* have their
+  // contents verified. (Some files get transcoded during the install process,
+  // so we don't want to verify their contents because they are expected not
+  // to match).
+  bool ShouldVerifyAnyPaths(const Extension* extension,
+                            const std::set<base::FilePath>& relative_paths);
+
   // Note that it is important for these to appear in increasing "severity"
   // order, because we use this to let command line flags increase, but not
   // decrease, the mode you're running in compared to the experiment group.

extensions/browser/content_verifier_delegate.h

@@ -23,6 +23,8 @@ struct ContentVerifierKey {
   const uint8* data;
   int size;
 
+  ContentVerifierKey() : data(NULL), size(0) {}
+
   ContentVerifierKey(const uint8* data, int size) {
     this->data = data;
     this->size = size;

extensions/browser/content_verify_job.cc

@@ -29,7 +29,8 @@ ContentVerifyJob::ContentVerifyJob(ContentHashReader* hash_reader,
       current_block_(0),
       current_hash_byte_count_(0),
       hash_reader_(hash_reader),
-      failure_callback_(failure_callback) {
+      failure_callback_(failure_callback),
+      failed_(false) {
   // It's ok for this object to be constructed on a different thread from where
   // it's used.
   thread_checker_.DetachFromThread();
@@ -49,6 +50,8 @@ void ContentVerifyJob::Start() {
 
 void ContentVerifyJob::BytesRead(int count, const char* data) {
   DCHECK(thread_checker_.CalledOnValidThread());
+  if (failed_)
+    return;
   if (g_test_delegate) {
     FailureReason reason =
         g_test_delegate->BytesRead(hash_reader_->extension_id(), count, data);
@@ -75,6 +78,7 @@ void ContentVerifyJob::BytesRead(int count, const char* data) {
     int bytes_to_hash =
         std::min(hash_reader_->block_size() - current_hash_byte_count_,
                  count - bytes_added);
+    DCHECK(bytes_to_hash > 0);
     current_hash_->Update(data + bytes_added, bytes_to_hash);
     bytes_added += bytes_to_hash;
     current_hash_byte_count_ += bytes_to_hash;
@@ -82,13 +86,18 @@ void ContentVerifyJob::BytesRead(int count, const char* data) {
 
     // If we finished reading a block worth of data, finish computing the hash
     // for it and make sure the expected hash matches.
-    if (current_hash_byte_count_ == hash_reader_->block_size())
-      FinishBlock();
+    if (current_hash_byte_count_ == hash_reader_->block_size() &&
+        !FinishBlock()) {
+      DispatchFailureCallback(HASH_MISMATCH);
+      return;
+    }
   }
 }
 
 void ContentVerifyJob::DoneReading() {
   DCHECK(thread_checker_.CalledOnValidThread());
+  if (failed_)
+    return;
   if (g_test_delegate) {
     FailureReason reason =
         g_test_delegate->DoneReading(hash_reader_->extension_id());
@@ -98,31 +107,36 @@ void ContentVerifyJob::DoneReading() {
     }
   }
   done_reading_ = true;
-  if (hashes_ready_)
-    FinishBlock();
+  if (hashes_ready_ && !FinishBlock())
+    DispatchFailureCallback(HASH_MISMATCH);
 }
 
-void ContentVerifyJob::FinishBlock() {
+bool ContentVerifyJob::FinishBlock() {
   if (current_hash_byte_count_ <= 0)
-    return;
+    return true;
   std::string final(crypto::kSHA256Length, 0);
   current_hash_->Finish(string_as_array(&final), final.size());
 
   const std::string* expected_hash = NULL;
-  if (!hash_reader_->GetHashForBlock(current_block_, &expected_hash))
-    return DispatchFailureCallback(HASH_MISMATCH);
-
-  if (*expected_hash != final)
-    return DispatchFailureCallback(HASH_MISMATCH);
+  if (!hash_reader_->GetHashForBlock(current_block_, &expected_hash) ||
+      *expected_hash != final)
+    return false;
 
   current_hash_.reset();
   current_hash_byte_count_ = 0;
   current_block_++;
+  return true;
 }
 
 void ContentVerifyJob::OnHashesReady(bool success) {
-  if (!success && !g_test_delegate)
-    return DispatchFailureCallback(NO_HASHES);
+  if (!success && !g_test_delegate) {
+    if (hash_reader_->have_verified_contents() &&
+        hash_reader_->have_computed_hashes())
+      DispatchFailureCallback(NO_HASHES_FOR_FILE);
+    else
+      DispatchFailureCallback(MISSING_ALL_HASHES);
+    return;
+  }
 
   hashes_ready_ = true;
   if (!queue_.empty()) {
@@ -130,8 +144,8 @@ void ContentVerifyJob::OnHashesReady(bool success) {
     queue_.swap(tmp);
     BytesRead(tmp.size(), string_as_array(&tmp));
   }
-  if (done_reading_)
-    FinishBlock();
+  if (done_reading_ && !FinishBlock())
+    DispatchFailureCallback(HASH_MISMATCH);
 }
 
 // static
@@ -140,7 +154,12 @@ void ContentVerifyJob::SetDelegateForTests(TestDelegate* delegate) {
 }
 
 void ContentVerifyJob::DispatchFailureCallback(FailureReason reason) {
+  DCHECK(!failed_);
+  failed_ = true;
   if (!failure_callback_.is_null()) {
+    VLOG(1) << "job failed for " << hash_reader_->extension_id() << " "
+            << hash_reader_->relative_path().MaybeAsASCII()
+            << " reason:" << reason;
     failure_callback_.Run(reason);
     failure_callback_.Reset();
   }

extensions/browser/content_verify_job.h

@@ -34,8 +34,12 @@ class ContentVerifyJob : public base::RefCountedThreadSafe<ContentVerifyJob> {
     // No failure.
     NONE,
 
-    // Failed because there were no expected hashes.
-    NO_HASHES,
+    // Failed because there were no expected hashes at all (eg they haven't
+    // been fetched yet).
+    MISSING_ALL_HASHES,
+
+    // Failed because this file wasn't found in the list of expected hashes.
+    NO_HASHES_FOR_FILE,
 
     // Some of the content read did not match the expected hash.
     HASH_MISMATCH
@@ -81,9 +85,10 @@ class ContentVerifyJob : public base::RefCountedThreadSafe<ContentVerifyJob> {
   friend class base::RefCountedThreadSafe<ContentVerifyJob>;
 
   // Called each time we're done adding bytes for the current block, and are
-  // ready to finish the hash operation for those bytes and make sure it matches
-  // what was expected for that block.
-  void FinishBlock();
+  // ready to finish the hash operation for those bytes and make sure it
+  // matches what was expected for that block. Returns true if everything is
+  // still ok so far, or false if a mismatch was detected.
+  bool FinishBlock();
 
   // Dispatches the failure callback with the given reason.
   void DispatchFailureCallback(FailureReason reason);
@@ -118,6 +123,9 @@ class ContentVerifyJob : public base::RefCountedThreadSafe<ContentVerifyJob> {
   // Called once if verification fails.
   FailureCallback failure_callback_;
 
+  // Set to true if we detected a mismatch and called the failure callback.
+  bool failed_;
+
   // For ensuring methods on called on the right thread.
   base::ThreadChecker thread_checker_;
 };