Move tree hash verification details out of ContentHashReader

Upon initialising, ContentHashReader checks that block hashes from
computed_hashes.json are correct by verifying them using tree hash from
verified_contents.json. To keep under ContentHashReader only logic about
using block hashes to check data, this commit moves verification of the
hashes to ContentHash.

Bug: 796395
Change-Id: Ia152559bac3c409c7651b6902948ef72d7744e3b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1768348
Commit-Queue: Oleg Davydov <burunduk@chromium.org>
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#694165}

extensions/browser/content_hash_reader.cc

@@ -7,6 +7,7 @@
 #include "base/files/file_util.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
+#include "base/stl_util.h"
 #include "base/strings/string_util.h"
 #include "base/timer/elapsed_timer.h"
 #include "base/values.h"
@@ -35,12 +36,25 @@ std::unique_ptr<const ContentHashReader> ContentHashReader::Create(
 
   hash_reader->has_content_hashes_ = true;
 
-  const VerifiedContents& verified_contents = content_hash->verified_contents();
+  const ComputedHashes::Reader& reader = content_hash->computed_hashes();
+  base::Optional<std::string> root;
+
+  if (reader.GetHashes(relative_path, &hash_reader->block_size_,
+                       &hash_reader->hashes_) &&
+      hash_reader->block_size_ % crypto::kSHA256Length == 0) {
+    root = ComputeTreeHashRoot(
+        hash_reader->hashes_, hash_reader->block_size_ / crypto::kSHA256Length);
+  }
+
+  ContentHash::TreeHashVerificationResult verification =
+      content_hash->VerifyTreeHashRoot(relative_path,
+                                       base::OptionalOrNullptr(root));
 
   // Extensions sometimes request resources that do not have an entry in
-  // verified_contents.json. This can happen when an extension sends an XHR to a
-  // resource.
-  if (!verified_contents.HasTreeHashRoot(relative_path)) {
+  // computed_hashes.json or verified_content.json. This can happen, for
+  // example, when an extension sends an XHR to a resource. This should not be
+  // considered as a failure.
+  if (verification != ContentHash::TreeHashVerificationResult::SUCCESS) {
     base::FilePath full_path =
         content_hash->extension_root().Append(relative_path);
     // Making a request to a non-existent file or to a directory should not
@@ -49,24 +63,13 @@ std::unique_ptr<const ContentHashReader> ContentHashReader::Create(
     // kept track of whether the file being verified was successfully read.
     // A content verification failure should be triggered if there is a mismatch
     // between the file read state and the existence of verification hashes.
-    if (!base::PathExists(full_path) || base::DirectoryExists(full_path))
+    if (verification == ContentHash::TreeHashVerificationResult::NO_ENTRY &&
+        (!base::PathExists(full_path) || base::DirectoryExists(full_path)))
       hash_reader->file_missing_from_verified_contents_ = true;
 
     return hash_reader;  // FAILURE.
   }
 
-  const ComputedHashes::Reader& reader = content_hash->computed_hashes();
-  if (!reader.GetHashes(relative_path, &hash_reader->block_size_,
-                        &hash_reader->hashes_) ||
-      hash_reader->block_size_ % crypto::kSHA256Length != 0) {
-    return hash_reader;
-  }
-
-  std::string root = ComputeTreeHashRoot(
-      hash_reader->hashes_, hash_reader->block_size_ / crypto::kSHA256Length);
-  if (!verified_contents.TreeHashRootEquals(relative_path, root))
-    return hash_reader;
-
   hash_reader->status_ = SUCCESS;
   UMA_HISTOGRAM_TIMES("ExtensionContentHashReader.InitLatency",
                       timer.Elapsed());

extensions/browser/content_verifier/content_hash.cc

@@ -118,9 +118,17 @@ void ContentHash::ForceBuildComputedHashes(
   std::move(created_callback).Run(this, is_cancelled && is_cancelled.Run());
 }
 
-const VerifiedContents& ContentHash::verified_contents() const {
+ContentHash::TreeHashVerificationResult ContentHash::VerifyTreeHashRoot(
+    const base::FilePath& relative_path,
+    const std::string* root) const {
   DCHECK(status_ >= Status::kHasVerifiedContents && verified_contents_);
-  return *verified_contents_;
+  if (!verified_contents_->HasTreeHashRoot(relative_path))
+    return TreeHashVerificationResult::NO_ENTRY;
+
+  if (!root || !verified_contents_->TreeHashRootEquals(relative_path, *root))
+    return TreeHashVerificationResult::HASH_MISMATCH;
+
+  return TreeHashVerificationResult::SUCCESS;
 }
 
 const ComputedHashes::Reader& ContentHash::computed_hashes() const {

extensions/browser/content_verifier/content_hash.h

@@ -78,6 +78,19 @@ class ContentHash : public base::RefCountedThreadSafe<ContentHash> {
     DISALLOW_COPY_AND_ASSIGN(FetchKey);
   };
 
+  // Result of checking tree hash root (typically calculated from block hashes
+  // in computed_hashes.json) against signed hash from verified_contents.json.
+  enum class TreeHashVerificationResult {
+    // Hash is correct.
+    SUCCESS,
+
+    // There is no such file in verified_contents.json.
+    NO_ENTRY,
+
+    // Hash does not match the one from verified_contents.json.
+    HASH_MISMATCH
+  };
+
   using IsCancelledCallback = base::RepeatingCallback<bool(void)>;
 
   // Factory:
@@ -99,12 +112,18 @@ class ContentHash : public base::RefCountedThreadSafe<ContentHash> {
   void ForceBuildComputedHashes(const IsCancelledCallback& is_cancelled,
                                 CreatedCallback created_callback);
 
-  const VerifiedContents& verified_contents() const;
+  // Returns the result of comparing tree hash |root| for the |relative_path| to
+  // verified_contens.json data.
+  TreeHashVerificationResult VerifyTreeHashRoot(
+      const base::FilePath& relative_path,
+      const std::string* root) const;
+
   const ComputedHashes::Reader& computed_hashes() const;
 
   bool has_verified_contents() const {
     return status_ >= Status::kHasVerifiedContents;
   }
+
   bool succeeded() const { return status_ >= Status::kSucceeded; }
 
   // If ContentHash creation writes computed_hashes.json, then this returns the

extensions/browser/content_verify_job_unittest.cc

@@ -9,6 +9,7 @@
 #include "base/files/scoped_temp_dir.h"
 #include "base/path_service.h"
 #include "base/task/post_task.h"
+#include "base/test/bind_test_util.h"
 #include "base/version.h"
 #include "content/public/browser/browser_task_traits.h"
 #include "content/public/browser/browser_thread.h"
@@ -269,6 +270,116 @@ TEST_F(ContentVerifyJobUnittest, DeletedAndMissingFiles) {
   }
 }
 
+namespace {
+
+void WriteIncorrectComputedHashes(const base::FilePath& extension_path,
+                                  const base::FilePath& resource_path) {
+  // It is important that correct computed_hashes.json already exists, because
+  // we don't want to modify it while it is being created. "source_all.zip"
+  // ensures we already have it.
+  ASSERT_TRUE(
+      base::PathExists(file_util::GetComputedHashesPath(extension_path)));
+
+  base::DeleteFile(file_util::GetComputedHashesPath(extension_path),
+                   false /* recursive */);
+
+  int block_size = extension_misc::kContentVerificationDefaultBlockSize;
+  ComputedHashes::Writer incorrect_computed_hashes_writer;
+
+  // Write a valid computed_hashes.json with incorrect hash for |resource_path|.
+  std::vector<std::string> hashes;
+  const std::string kFakeContents = "fake contents";
+  ComputedHashes::ComputeHashesForContent(kFakeContents, block_size, &hashes);
+  incorrect_computed_hashes_writer.AddHashes(resource_path, block_size, hashes);
+
+  ASSERT_TRUE(incorrect_computed_hashes_writer.WriteToFile(
+      file_util::GetComputedHashesPath(extension_path)));
+}
+
+void WriteEmptyComputedHashes(const base::FilePath& extension_path) {
+  // It is important that correct computed_hashes.json already exists, because
+  // we don't want to modify it while it is being created. "source_all.zip"
+  // ensures we already have it.
+  ASSERT_TRUE(
+      base::PathExists(file_util::GetComputedHashesPath(extension_path)));
+
+  base::DeleteFile(file_util::GetComputedHashesPath(extension_path),
+                   false /* recursive */);
+
+  ComputedHashes::Writer incorrect_computed_hashes_writer;
+
+  ASSERT_TRUE(incorrect_computed_hashes_writer.WriteToFile(
+      file_util::GetComputedHashesPath(extension_path)));
+}
+
+}  // namespace
+
+// Tests that deletion of an extension resource and invalid hash for it in
+// computed_hashes.json won't result in bypassing corruption check.
+TEST_F(ContentVerifyJobUnittest, DeletedResourceAndCorruptedComputedHashes) {
+  base::ScopedTempDir temp_dir;
+
+  const base::FilePath::CharType kResource[] =
+      FILE_PATH_LITERAL("background.js");
+  base::FilePath resource_path(kResource);
+
+  scoped_refptr<Extension> extension = LoadTestExtensionFromZipPathToTempDir(
+      &temp_dir, "with_verified_contents", "source_all.zip");
+  ASSERT_TRUE(extension.get());
+
+  // Tamper the extension: remove resource and place wrong hash for its entry in
+  // computed_hashes.json. Reload content verifier's cache after that because
+  // content verifier may read computed_hashes.json with old values upon
+  // extension loading.
+  base::FilePath unzipped_path = temp_dir.GetPath();
+  WriteIncorrectComputedHashes(unzipped_path, resource_path);
+  EXPECT_TRUE(
+      base::DeleteFile(unzipped_path.Append(base::FilePath(kResource)), false));
+  content_verifier()->ClearCacheForTesting();
+
+  {
+    // By now in tests we serve an empty resource instead of non-existsing one.
+    // See https://crbug.com/999727 for details.
+    std::string empty_contents;
+    EXPECT_EQ(
+        ContentVerifyJob::NO_HASHES_FOR_FILE,
+        RunContentVerifyJob(*extension.get(), resource_path, empty_contents));
+  }
+}
+
+// Tests that deletion of an extension resource and removing its entry from
+// computed_hashes.json won't result in bypassing corruption check.
+TEST_F(ContentVerifyJobUnittest, DeletedResourceAndCleanedComputedHashes) {
+  base::ScopedTempDir temp_dir;
+
+  const base::FilePath::CharType kResource[] =
+      FILE_PATH_LITERAL("background.js");
+  base::FilePath resource_path(kResource);
+
+  scoped_refptr<Extension> extension = LoadTestExtensionFromZipPathToTempDir(
+      &temp_dir, "with_verified_contents", "source_all.zip");
+  ASSERT_TRUE(extension.get());
+
+  // Tamper the extension: remove resource and remove its entry from
+  // computed_hashes.json. Reload content verifier's cache after that because
+  // content verifier may read computed_hashes.json with old values upon
+  // extension loading.
+  base::FilePath unzipped_path = temp_dir.GetPath();
+  WriteEmptyComputedHashes(unzipped_path);
+  EXPECT_TRUE(
+      base::DeleteFile(unzipped_path.Append(base::FilePath(kResource)), false));
+  content_verifier()->ClearCacheForTesting();
+
+  {
+    // By now in tests we serve an empty resource instead of non-existsing one.
+    // See https://crbug.com/999727 for details.
+    std::string empty_contents;
+    EXPECT_EQ(
+        ContentVerifyJob::NO_HASHES_FOR_FILE,
+        RunContentVerifyJob(*extension.get(), resource_path, empty_contents));
+  }
+}
+
 // Tests that extension resources that are originally 0 byte behave correctly
 // with content verification.
 TEST_F(ContentVerifyJobUnittest, LegitimateZeroByteFile) {