Tighten filesystem: requests to use stronger CanCommitURL security checks.

This CL strenthens security checks in FileSystemEntryURLLoader to
block requests for filesystem: URLs if the requested URL is not
commitable in the current process.  When site isolation is on, this
will prevent one origin from fetching filesystem resources belonging
to another origin.

Note that this will also block web sites from requesting arbitrary
extension filesystem URLs that lead to downloads, which is an
intentional change discussed on 964245.  An existing test in
ProcessManagerBrowserTest is updated accordingly.

Bug: 964245
Change-Id: I09023cc884278efef0bb4d16e584b2c5f1a5fd5b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1635876Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#667356}

chrome/browser/extensions/process_manager_browsertest.cc

@@ -915,10 +915,10 @@ IN_PROC_BROWSER_TEST_F(ProcessManagerBrowserTest,
   }
 }
 
-// Check that browser-side restrictions on extension blob/filesystem URLs allow
+// Check that browser-side restrictions on extension blob URLs allow
 // navigations that will result in downloads.  See https://crbug.com/714373.
 IN_PROC_BROWSER_TEST_F(ProcessManagerBrowserTest,
-                       NestedURLDownloadsToExtensionAllowed) {
+                       BlobURLDownloadsToExtensionAllowed) {
   // Disabling web security is necessary to test the browser enforcement;
   // without it, the loads in this test would be blocked by
   // SecurityOrigin::CanDisplay() as invalid local resource loads.
@@ -949,46 +949,42 @@ IN_PROC_BROWSER_TEST_F(ProcessManagerBrowserTest,
   content::RenderFrameHost* main_frame = tab->GetMainFrame();
   content::RenderFrameHost* extension_frame = ChildFrameAt(main_frame, 0);
 
-  // Create valid blob and filesystem URLs in the extension's origin.
+  // Create a valid blob URL in the extension's origin.
   url::Origin extension_origin(extension_frame->GetLastCommittedOrigin());
   GURL blob_url(CreateBlobURL(extension_frame, "foo"));
   EXPECT_EQ(extension_origin, url::Origin::Create(blob_url));
-  GURL filesystem_url(CreateFileSystemURL(extension_frame, "foo"));
-  EXPECT_EQ(extension_origin, url::Origin::Create(filesystem_url));
-  GURL nested_urls[] = {blob_url, filesystem_url};
 
-  // Check that extension blob/filesystem URLs still can be downloaded via an
-  // HTML anchor tag with the download attribute (i.e., <a download>) (which
-  // starts out as a top-level navigation).
+  // Check that extension blob URLs still can be downloaded via an HTML anchor
+  // tag with the download attribute (i.e., <a download>) (which starts out as
+  // a top-level navigation).
   PermissionRequestManager* permission_request_manager =
       PermissionRequestManager::FromWebContents(tab);
   permission_request_manager->set_auto_response_for_test(
       PermissionRequestManager::ACCEPT_ALL);
-  for (const GURL& nested_url : nested_urls) {
-    content::DownloadTestObserverTerminal observer(
-        content::BrowserContext::GetDownloadManager(profile()), 1,
-        content::DownloadTestObserver::ON_DANGEROUS_DOWNLOAD_FAIL);
-    std::string script = base::StringPrintf(
-        R"(var anchor = document.createElement('a');
-           anchor.href = '%s';
-           anchor.download = '';
-           anchor.click();)",
-        nested_url.spec().c_str());
-    EXPECT_TRUE(ExecuteScript(tab, script));
-    observer.WaitForFinished();
-    EXPECT_EQ(
-        1u, observer.NumDownloadsSeenInState(download::DownloadItem::COMPLETE));
-
-    // This is a top-level navigation that should have resulted in a download.
-    // Ensure that the tab stayed at its original location.
-    EXPECT_NE(nested_url, tab->GetLastCommittedURL());
-    EXPECT_FALSE(extension_origin.IsSameOriginWith(
-        main_frame->GetLastCommittedOrigin()));
-    EXPECT_NE("foo", GetTextContent(main_frame));
 
-    EXPECT_EQ(1u, pm->GetRenderFrameHostsForExtension(extension->id()).size());
-    EXPECT_EQ(1u, pm->GetAllFrames().size());
-  }
+  content::DownloadTestObserverTerminal observer(
+      content::BrowserContext::GetDownloadManager(profile()), 1,
+      content::DownloadTestObserver::ON_DANGEROUS_DOWNLOAD_FAIL);
+  std::string script = base::StringPrintf(
+      R"(var anchor = document.createElement('a');
+         anchor.href = '%s';
+         anchor.download = '';
+         anchor.click();)",
+      blob_url.spec().c_str());
+  EXPECT_TRUE(ExecuteScript(tab, script));
+  observer.WaitForFinished();
+  EXPECT_EQ(1u,
+            observer.NumDownloadsSeenInState(download::DownloadItem::COMPLETE));
+
+  // This is a top-level navigation that should have resulted in a download.
+  // Ensure that the tab stayed at its original location.
+  EXPECT_NE(blob_url, tab->GetLastCommittedURL());
+  EXPECT_FALSE(
+      extension_origin.IsSameOriginWith(main_frame->GetLastCommittedOrigin()));
+  EXPECT_NE("foo", GetTextContent(main_frame));
+
+  EXPECT_EQ(1u, pm->GetRenderFrameHostsForExtension(extension->id()).size());
+  EXPECT_EQ(1u, pm->GetAllFrames().size());
 }
 
 // Test that navigations to blob: and filesystem: URLs with extension origins

content/browser/fileapi/file_system_url_loader_factory.cc

@@ -161,8 +161,11 @@ class FileSystemEntryURLLoader
       return;
     }
 
+    // If the requested URL is not commitable in the current process, block the
+    // request.  This prevents one origin from fetching filesystem: resources
+    // belonging to another origin, see https://crbug.com/964245.
     if (params_.render_process_host_id != ChildProcessHost::kInvalidUniqueID &&
-        !ChildProcessSecurityPolicyImpl::GetInstance()->CanRequestURL(
+        !ChildProcessSecurityPolicyImpl::GetInstance()->CanCommitURL(
             params_.render_process_host_id, request.url)) {
       DVLOG(1) << "Denied unauthorized request for "
                << request.url.possibly_invalid_spec();

content/browser/fileapi/file_system_url_loader_factory_browsertest.cc

@@ -21,9 +21,12 @@
 #include "content/browser/fileapi/file_system_url_loader_factory.h"
 #include "content/browser/web_contents/web_contents_impl.h"
 #include "content/public/browser/browser_task_traits.h"
+#include "content/public/test/browser_test_utils.h"
 #include "content/public/test/content_browser_test.h"
+#include "content/public/test/test_utils.h"
 #include "content/shell/browser/shell.h"
 #include "net/base/mime_util.h"
+#include "net/dns/mock_host_resolver.h"
 #include "net/http/http_util.h"
 #include "net/traffic_annotation/network_traffic_annotation_test_helper.h"
 #include "services/network/public/cpp/features.h"
@@ -185,6 +188,9 @@ class FileSystemURLLoaderFactoryTest
 
     special_storage_policy_ = new MockSpecialStoragePolicy;
 
+    // Support multiple sites on the test server.
+    host_resolver()->AddRule("*", "127.0.0.1");
+
     ContentBrowserTest::SetUpOnMainThread();
 
     // We use a test FileSystemContext which runs on the main thread, so we
@@ -704,6 +710,30 @@ IN_PROC_BROWSER_TEST_P(FileSystemURLLoaderFactoryTest, FileTest) {
   EXPECT_EQ("no-cache", cache_control);
 }
 
+// Verify that when site isolation is enabled, a renderer process for one
+// origin can't request filesystem: URLs belonging to another origin.  See
+// https://crbug.com/964245.
+IN_PROC_BROWSER_TEST_P(FileSystemURLLoaderFactoryTest, CrossOriginFileBlocked) {
+  IsolateAllSitesForTesting(base::CommandLine::ForCurrentProcess());
+
+  base::ScopedAllowBlockingForTesting allow_blocking;
+  WriteFile("file1.dat", kTestFileData, base::size(kTestFileData) - 1);
+
+  // Navigate main frame to foo.com.
+  ASSERT_TRUE(embedded_test_server()->Start());
+  EXPECT_TRUE(
+      NavigateToURL(shell()->web_contents(),
+                    embedded_test_server()->GetURL("foo.com", "/title1.html")));
+
+  // Try requesting filesystem:http://remote/temporary/file1.dat from that
+  // frame.  This should be blocked, as foo.com isn't allowed to request a
+  // filesystem URL for the http://remote origin.
+  auto client = TestLoad(CreateFileSystemURL("file1.dat"));
+  EXPECT_FALSE(client->has_received_response());
+  ASSERT_TRUE(client->has_received_completion());
+  EXPECT_EQ(net::ERR_INVALID_URL, client->completion_status().error_code);
+}
+
 IN_PROC_BROWSER_TEST_P(FileSystemURLLoaderFactoryTest,
                        FileTestFullSpecifiedRange) {
   base::ScopedAllowBlockingForTesting allow_blocking;