Extensions: Do not skip content verification on contiguous seek of a resource load.
With network service, all extension resource load that is greater than 1k
bytes in size generate OnSeekComplete notification. The existing code we
had around this before network service assumed the seek won't be contiguous
and would reset/nuke ContentVerifyJob in that case.
This CL allows ContentVerifyJob to continue to run if OnSeekComplete was
triggered that was result of a contiguous seek.
This CL also adds a regression test for this issue.
Bug: 964043
Change-Id: Ib9df8bd818fb8dd3abb4f48bf80d8eda6216f04b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1615434
Commit-Queue: Istiaque Ahmed <lazyboy@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Chris Mumford <cmumford@google.com>
Cr-Commit-Position: refs/heads/master@{#661459}
diff --git a/chrome/browser/extensions/content_verifier_browsertest.cc b/chrome/browser/extensions/content_verifier_browsertest.cc
index d1b8689..faf1971 100644
--- a/chrome/browser/extensions/content_verifier_browsertest.cc
+++ b/chrome/browser/extensions/content_verifier_browsertest.cc
@@ -115,6 +115,21 @@
EXPECT_TRUE(job_observer.WaitForExpectedJobs());
}
+ void NavigateToResourceAndExpectExtensionDisabled(
+ const ExtensionId& extension_id,
+ const GURL& extension_resource) {
+ TestExtensionRegistryObserver unload_observer(
+ ExtensionRegistry::Get(profile()), extension_id);
+ ui_test_utils::NavigateToURLWithDisposition(
+ browser(), extension_resource,
+ WindowOpenDisposition::NEW_FOREGROUND_TAB,
+ ui_test_utils::BROWSER_TEST_NONE);
+ EXPECT_TRUE(unload_observer.WaitForExtensionUnloaded());
+ ExtensionPrefs* prefs = ExtensionPrefs::Get(profile());
+ int reasons = prefs->GetDisableReasons(extension_id);
+ EXPECT_EQ(disable_reason::DISABLE_CORRUPTED, reasons);
+ }
+
protected:
base::test::ScopedFeatureList scoped_feature_list_;
};
@@ -282,18 +297,34 @@
}
GURL page_url = extension->GetResourceURL("page.html");
- TestExtensionRegistryObserver unload_observer(
- ExtensionRegistry::Get(profile()), kExtensionId);
- // Wait for 0 navigations to complete because with PlzNavigate it's racy
- // when the didstop IPC arrives relative to the tab being closed. The
- // wait call below is what the tests care about.
- ui_test_utils::NavigateToURLWithDispositionBlockUntilNavigationsComplete(
- browser(), page_url, 0, WindowOpenDisposition::NEW_FOREGROUND_TAB,
- ui_test_utils::BROWSER_TEST_NONE);
- EXPECT_TRUE(unload_observer.WaitForExtensionUnloaded());
- ExtensionPrefs* prefs = ExtensionPrefs::Get(profile());
- int reasons = prefs->GetDisableReasons(kExtensionId);
- EXPECT_TRUE(reasons & disable_reason::DISABLE_CORRUPTED);
+ NavigateToResourceAndExpectExtensionDisabled(kExtensionId, page_url);
+}
+
+// Tests that tampering with a large resource fails content verification as
+// expected. The size of the resource is such that it would trigger
+// FileLoaderObserver::OnSeekComplete in extension_protocols.cc.
+//
+// Regression test for: http://crbug.com/965043.
+IN_PROC_BROWSER_TEST_F(ContentVerifierTest, TamperLargeSizedResource) {
+ // This test extension is copied from the webstore that has actual
+ // signatures.
+ const Extension* extension = InstallExtensionFromWebstore(
+ test_data_dir_.AppendASCII("content_verifier/different_sized_files.crx"),
+ 1);
+ ASSERT_TRUE(extension);
+
+ const char kResource[] = "jquery-3.2.0.min.js";
+ {
+ // Modify content so that content verification fails.
+ base::ScopedAllowBlockingForTesting allow_blocking;
+ base::FilePath real_path = extension->path().AppendASCII(kResource);
+ ASSERT_TRUE(base::PathExists(real_path));
+ std::string extra = "some_extra_function_call();";
+ ASSERT_TRUE(base::AppendToFile(real_path, extra.data(), extra.size()));
+ }
+
+ NavigateToResourceAndExpectExtensionDisabled(
+ extension->id(), extension->GetResourceURL(kResource));
}
class ContentVerifierPolicyTest : public ContentVerifierTest {
diff --git a/chrome/test/data/extensions/content_verifier/different_sized_files.crx b/chrome/test/data/extensions/content_verifier/different_sized_files.crx
new file mode 100644
index 0000000..5382980
--- /dev/null
+++ b/chrome/test/data/extensions/content_verifier/different_sized_files.crx
Binary files differ
diff --git a/chrome/test/data/extensions/content_verifier/different_sized_files.crx.INFO b/chrome/test/data/extensions/content_verifier/different_sized_files.crx.INFO
new file mode 100644
index 0000000..eff32db
--- /dev/null
+++ b/chrome/test/data/extensions/content_verifier/different_sized_files.crx.INFO
@@ -0,0 +1,7 @@
+Fetched from Chrome Web Store.
+CRX ID = dlefkgcbefcjoiheimkdkkhdcejpbgda
+
+This extension has a file that has interesting size w.r.t
+FileURLLoaderFactory's mime-sniffing code. Specifically,
+jquery-3.2.0.min.js file is larger than 1024 bytes. This will trigger
+FileURLLoaderObserver::OnSeekComplete that we care about in our test.
diff --git a/extensions/browser/extension_protocols.cc b/extensions/browser/extension_protocols.cc
index e00a9cff6..0065675 100644
--- a/extensions/browser/extension_protocols.cc
+++ b/extensions/browser/extension_protocols.cc
@@ -277,8 +277,9 @@
seek_position_ = result;
// TODO(asargent) - we'll need to add proper support for range headers.
// crbug.com/369895.
- if (result > 0 && verify_job_.get())
- verify_job_ = NULL;
+ const bool is_seek_contiguous = result == bytes_read_;
+ if (result > 0 && verify_job_.get() && !is_seek_contiguous)
+ verify_job_ = nullptr;
}
void OnReadComplete(net::IOBuffer* buffer, int result) override {
@@ -702,7 +703,8 @@
seek_position_ = result;
// TODO(asargent) - we'll need to add proper support for range headers.
// crbug.com/369895.
- if (result > 0 && verify_job_.get())
+ const bool is_seek_contiguous = result == bytes_read_;
+ if (result > 0 && verify_job_.get() && !is_seek_contiguous)
verify_job_ = nullptr;
}
