Fix content verification code for undreadable and deleted files.

It seems that corruption examples in crbug.com/699420 can cause extension
resource files to go missing or become unreadable.
Fix a bug where we never sent any notification to ContentVerifyJob,
in particular: we never called ContentVerifyJob::DoneReading().

Override URLRequestFileJob::OnOpenComplete in URLRequestExtensionJob
to catch these cases.

Also make ContentHashReader not skip legitimate deleted files: the
files that are listed in verified_contents.json. It will still skip
non-existent resources, e.g. xhrs requesting non-existent file (see
crbug.com/404802 for example).

BUG=703888
Test=Install an extension (not app) in chromeos. Chromeos because you want
content verification enforcement to kick in. Other way could be to use
--extension-content-verification=enforce_strict flag in other platform.
Either:
1) Delete background script file. Content verifcation would kick in when
you try to load/run the extension.
2) Same as above^^^, instead of deleting the file, remove the file
permission. In linux you can just do "chmod -r background.js". Expect
content verification failure.
In both cases extension should be reinstalled in the end.

Review-Url: https://codereview.chromium.org/2771953003
Cr-Commit-Position: refs/heads/master@{#460607}

chrome/browser/extensions/content_verifier_browsertest.cc

@@ -126,7 +126,7 @@ class JobDelegate : public ContentVerifyJob::TestDelegate {
         bytes_read_failed_(0),
         done_reading_failed_(0) {}
 
-  virtual ~JobDelegate() {}
+  ~JobDelegate() override {}
 
   void set_id(const ExtensionId& id) { id_ = id; }
   void fail_next_read() { fail_next_read_ = true; }
@@ -189,7 +189,7 @@ class JobObserver : public ContentVerifyJob::TestObserver {
 
   void JobFinished(const std::string& extension_id,
                    const base::FilePath& relative_path,
-                   bool failed) override;
+                   ContentVerifyJob::FailureReason failure_reason) override;
 
  private:
   struct ExpectedResult {
@@ -241,15 +241,16 @@ void JobObserver::JobStarted(const std::string& extension_id,
 
 void JobObserver::JobFinished(const std::string& extension_id,
                               const base::FilePath& relative_path,
-                              bool failed) {
+                              ContentVerifyJob::FailureReason failure_reason) {
   if (!content::BrowserThread::CurrentlyOn(creation_thread_)) {
     content::BrowserThread::PostTask(
         creation_thread_, FROM_HERE,
         base::Bind(&JobObserver::JobFinished, base::Unretained(this),
-                   extension_id, relative_path, failed));
+                   extension_id, relative_path, failure_reason));
     return;
   }
-  Result result = failed ? Result::FAILURE : Result::SUCCESS;
+  Result result = failure_reason == ContentVerifyJob::NONE ? Result::SUCCESS
+                                                           : Result::FAILURE;
   bool found = false;
   for (std::list<ExpectedResult>::iterator i = expectations_.begin();
        i != expectations_.end(); ++i) {
@@ -265,7 +266,8 @@ void JobObserver::JobFinished(const std::string& extension_id,
       loop_runner_->Quit();
   } else {
     LOG(WARNING) << "Ignoring unexpected JobFinished " << extension_id << "/"
-                 << relative_path.value() << " failed:" << failed;
+                 << relative_path.value()
+                 << " failure_reason:" << failure_reason;
   }
 }
 

extensions/browser/BUILD.gn

@@ -427,6 +427,7 @@ source_set("unit_tests") {
     "computed_hashes_unittest.cc",
     "content_hash_fetcher_unittest.cc",
     "content_hash_tree_unittest.cc",
+    "content_verify_job_unittest.cc",
     "error_map_unittest.cc",
     "event_listener_map_unittest.cc",
     "event_router_unittest.cc",

extensions/browser/content_hash_reader.cc

@@ -35,11 +35,10 @@ ContentHashReader::ContentHashReader(const std::string& extension_id,
       relative_path_(relative_path),
       key_(key),
       status_(NOT_INITIALIZED),
-      content_exists_(false),
       have_verified_contents_(false),
       have_computed_hashes_(false),
-      block_size_(0) {
-}
+      file_missing_from_verified_contents_(false),
+      block_size_(0) {}
 
 ContentHashReader::~ContentHashReader() {
 }
@@ -50,14 +49,6 @@ bool ContentHashReader::Init() {
   status_ = FAILURE;
   base::FilePath verified_contents_path =
       file_util::GetVerifiedContentsPath(extension_root_);
-
-  // Check that this is a valid resource to verify (i.e., it exists).
-  base::FilePath content_path = extension_root_.Append(relative_path_);
-  if (!base::PathExists(content_path) || base::DirectoryExists(content_path))
-    return false;
-
-  content_exists_ = true;
-
   if (!base::PathExists(verified_contents_path))
     return false;
 
@@ -82,6 +73,15 @@ bool ContentHashReader::Init() {
 
   have_computed_hashes_ = true;
 
+  if (!verified_contents.HasTreeHashRoot(relative_path_)) {
+    // Extension is requesting a non-existent resource that does not have an
+    // entry in verified_contents.json. This can happen when an extension sends
+    // XHR to its non-existent resource. This should not result in content
+    // verification failure.
+    file_missing_from_verified_contents_ = true;
+    return false;
+  }
+
   if (!reader.GetHashes(relative_path_, &block_size_, &hashes_) ||
       block_size_ % crypto::kSHA256Length != 0)
     return false;

extensions/browser/content_hash_reader.h

@@ -39,15 +39,17 @@ class ContentHashReader : public base::RefCountedThreadSafe<ContentHashReader> {
   // should likely be discarded.
   bool Init();
 
-  // Indicates whether the content in question exists in the local extension
-  // installation. This may be |false| if Init fails.
-  bool content_exists() const { return content_exists_; }
-
   // 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() const { return have_verified_contents_; }
   bool have_computed_hashes() const { return have_computed_hashes_; }
+  // Returns whether or not this resource's entry exists in
+  // verified_contents.json (given that both |have_verified_contents_| and
+  // |have_computed_hashes_| are true).
+  bool file_missing_from_verified_contents() const {
+    return file_missing_from_verified_contents_;
+  }
 
   // Return the number of blocks and block size, respectively. Only valid after
   // calling Init().
@@ -72,10 +74,9 @@ class ContentHashReader : public base::RefCountedThreadSafe<ContentHashReader> {
 
   InitStatus status_;
 
-  bool content_exists_;
-
   bool have_verified_contents_;
   bool have_computed_hashes_;
+  bool file_missing_from_verified_contents_;
 
   // The blocksize used for generating the hashes.
   int block_size_;

extensions/browser/content_verify_job.cc

@@ -134,17 +134,23 @@ void ContentVerifyJob::DoneReading() {
   }
   done_reading_ = true;
   if (hashes_ready_) {
-    if (!FinishBlock())
+    if (!FinishBlock()) {
       DispatchFailureCallback(HASH_MISMATCH);
-    else if (g_test_observer)
+    } else if (g_test_observer) {
       g_test_observer->JobFinished(hash_reader_->extension_id(),
-                                   hash_reader_->relative_path(), failed_);
+                                   hash_reader_->relative_path(), NONE);
+    }
   }
 }
 
 bool ContentVerifyJob::FinishBlock() {
-  if (current_hash_byte_count_ <= 0)
+  if (!done_reading_ && current_hash_byte_count_ == 0)
     return true;
+  if (!current_hash_) {
+    // This happens when we fail to read the resource. Compute empty content's
+    // hash in this case.
+    current_hash_ = crypto::SecureHash::Create(crypto::SecureHash::SHA256);
+  }
   std::string final(crypto::kSHA256Length, 0);
   current_hash_->Finish(base::string_as_array(& final), final.size());
   current_hash_.reset();
@@ -154,23 +160,33 @@ bool ContentVerifyJob::FinishBlock() {
 
   const std::string* expected_hash = NULL;
   if (!hash_reader_->GetHashForBlock(block, &expected_hash) ||
-      *expected_hash != final)
+      *expected_hash != final) {
     return false;
+  }
 
   return true;
 }
 
 void ContentVerifyJob::OnHashesReady(bool success) {
   if (!success && !g_test_delegate) {
-    if (!hash_reader_->content_exists()) {
+    // TODO(lazyboy): Make ContentHashReader::Init return an enum instead of
+    // bool. This should make the following checks on |hash_reader_| easier
+    // to digest and will avoid future bugs from creeping up.
+    if (!hash_reader_->have_verified_contents() ||
+        !hash_reader_->have_computed_hashes()) {
+      DispatchFailureCallback(MISSING_ALL_HASHES);
+      return;
+    }
+
+    if (hash_reader_->file_missing_from_verified_contents()) {
       // Ignore verification of non-existent resources.
+      if (g_test_observer) {
+        g_test_observer->JobFinished(hash_reader_->extension_id(),
+                                     hash_reader_->relative_path(), NONE);
+      }
       return;
-    } else if (hash_reader_->have_verified_contents() &&
-               hash_reader_->have_computed_hashes()) {
-      DispatchFailureCallback(NO_HASHES_FOR_FILE);
-    } else {
-      DispatchFailureCallback(MISSING_ALL_HASHES);
     }
+    DispatchFailureCallback(NO_HASHES_FOR_FILE);
     return;
   }
 
@@ -186,7 +202,7 @@ void ContentVerifyJob::OnHashesReady(bool success) {
       DispatchFailureCallback(HASH_MISMATCH);
     } else if (g_test_observer) {
       g_test_observer->JobFinished(hash_reader_->extension_id(),
-                                   hash_reader_->relative_path(), failed_);
+                                   hash_reader_->relative_path(), NONE);
     }
   }
 }
@@ -214,9 +230,10 @@ void ContentVerifyJob::DispatchFailureCallback(FailureReason reason) {
     failure_callback_.Run(reason);
     failure_callback_.Reset();
   }
-  if (g_test_observer)
-    g_test_observer->JobFinished(
-        hash_reader_->extension_id(), hash_reader_->relative_path(), failed_);
+  if (g_test_observer) {
+    g_test_observer->JobFinished(hash_reader_->extension_id(),
+                                 hash_reader_->relative_path(), reason);
+  }
 }
 
 }  // namespace extensions

extensions/browser/content_verify_job.h

@@ -72,6 +72,8 @@ class ContentVerifyJob : public base::RefCountedThreadSafe<ContentVerifyJob> {
 
   class TestDelegate {
    public:
+    virtual ~TestDelegate() {}
+
     // These methods will be called inside BytesRead/DoneReading respectively.
     // If either return something other than NONE, then the failure callback
     // will be dispatched with that reason.
@@ -88,7 +90,7 @@ class ContentVerifyJob : public base::RefCountedThreadSafe<ContentVerifyJob> {
 
     virtual void JobFinished(const std::string& extension_id,
                              const base::FilePath& relative_path,
-                             bool failed) = 0;
+                             FailureReason failure_reason) = 0;
   };
 
   // Note: having interleaved delegates is not supported.

extensions/browser/extension_protocols.cc

@@ -237,6 +237,20 @@ class URLRequestExtensionJob : public net::URLRequestFileJob {
     URLRequestFileJob::SetExtraRequestHeaders(headers);
   }
 
+  void OnOpenComplete(int result) override {
+    if (result < 0) {
+      // This can happen when the file is unreadable (which can happen during
+      // corruption or third-party interaction). We need to be sure to inform
+      // the verification job that we've finished reading so that it can
+      // proceed; see crbug.com/703888.
+      if (verify_job_.get()) {
+        std::string tmp;
+        verify_job_->BytesRead(0, base::string_as_array(&tmp));
+        verify_job_->DoneReading();
+      }
+    }
+  }
+
   void OnSeekComplete(int64_t result) override {
     DCHECK_EQ(seek_position_, 0);
     seek_position_ = result;

extensions/browser/scoped_ignore_content_verifier_for_test.h

@@ -19,7 +19,7 @@ class ScopedIgnoreContentVerifierForTest
     : public ContentVerifyJob::TestDelegate {
  public:
   ScopedIgnoreContentVerifierForTest();
-  ~ScopedIgnoreContentVerifierForTest();
+  ~ScopedIgnoreContentVerifierForTest() override;
 
   // ContentVerifyJob::TestDelegate interface
   ContentVerifyJob::FailureReason BytesRead(const std::string& extension_id,

extensions/test/data/content_hash_fetcher/with_verified_contents/README.txt

+The files in this directory came from downloading a test extension from the
+webstore* that had properly signed verified_contents.json file, taking all
+extension files (including _metadata/verified_contents.json and
+_metadata/computed_hashes.json) from the .crx and putting them into
+source_all.zip file.
+
+* https://chrome.google.com/webstore/detail/gpcgjpkhcgbdncmcaddhlhpccfckdoip