Non-web-accessible extension URLs should not load in non-extension processes

Blocking of URL loads in the browser process cannot be as extensive as the one done in the renderer process, as we don't have the frame URL and page URL at resource load time. I've tried to do a similar check with the data available on the browser side.

The main part which I'm not entirely happy about is the check for DevTools pages. Currently, if an extension has DevTools page, all of its resources can be loaded by a compromised renderer. I think this can be tightened up with a follow up CL, if we find a good way of distinguishing DevTools processes or permissioning those requests.

BUG=173688

Review URL: https://chromiumcodereview.appspot.com/12218064

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

6 files changed, 174 insertions, 5 deletions

diff --git a/chrome/browser/chrome_security_exploit_browsertest.cc b/chrome/browser/chrome_security_exploit_browsertest.cc
new file mode 100644
index 0000000..01fd458
--- /dev/null
+++ b/chrome/browser/chrome_security_exploit_browsertest.cc
@@ -0,0 +1,73 @@
+// Copyright (c) 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/command_line.h"
+#include "base/stringprintf.h"
+#include "base/utf_string_conversions.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_commands.h"
+#include "chrome/browser/ui/singleton_tabs.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "chrome/test/base/ui_test_utils.h"
+#include "content/public/browser/notification_observer.h"
+#include "content/public/browser/notification_service.h"
+#include "content/public/browser/notification_types.h"
+#include "content/public/browser/resource_request_details.h"
+#include "content/public/browser/web_contents_observer.h"
+#include "content/public/common/content_switches.h"
+#include "content/public/test/browser_test_utils.h"
+#include "webkit/glue/glue_serialize.h"
+
+namespace content {
+
+// The goal of these tests is to "simulate" exploited renderer processes, which
+// can send arbitrary IPC messages and confuse browser process internal state,
+// leading to security bugs. We are trying to verify that the browser doesn't
+// perform any dangerous operations in such cases.
+// This is similar to the security_exploit_browsertest.cc tests, but also
+// includes chrome/ layer concepts such as extensions.
+class ChromeSecurityExploitBrowserTest : public InProcessBrowserTest {
+ public:
+  ChromeSecurityExploitBrowserTest() {}
+
+  virtual void SetUpCommandLine(CommandLine* command_line) {
+    ASSERT_TRUE(test_server()->Start());
+    net::TestServer https_server(
+        net::TestServer::TYPE_HTTPS,
+        net::TestServer::kLocalhost,
+        base::FilePath(FILE_PATH_LITERAL("chrome/test/data")));
+    ASSERT_TRUE(https_server.Start());
+
+    // Add a host resolver rule to map all outgoing requests to the test server.
+    // This allows us to use "real" hostnames in URLs, which we can use to
+    // create arbitrary SiteInstances.
+    command_line->AppendSwitchASCII(
+        switches::kHostResolverRules,
+        "MAP * " + test_server()->host_port_pair().ToString() +
+            ",EXCLUDE localhost");
+
+    // Since we assume exploited renderer process, it can bypass the same origin
+    // policy at will. Simulate that by passing the disable-web-security flag.
+    command_line->AppendSwitch(switches::kDisableWebSecurity);
+  }
+};
+
+IN_PROC_BROWSER_TEST_F(ChromeSecurityExploitBrowserTest,
+                       ChromeExtensionResources) {
+  // Load a page that requests a chrome-extension:// image through XHR. We
+  // expect this load to fail, as it is an illegal request.
+  GURL foo("http://foo.com/files/chrome_extension_resource.html");
+
+  content::DOMMessageQueue msg_queue;
+
+  ui_test_utils::NavigateToURL(browser(), foo);
+
+  std::string status;
+  std::string expected_status("0");
+  EXPECT_TRUE(msg_queue.WaitForMessage(&status));
+  EXPECT_STREQ(status.c_str(), expected_status.c_str());
+}
+
+}
diff --git a/chrome/browser/extensions/extension_protocols.cc b/chrome/browser/extensions/extension_protocols.cc
index be7bab6..8c8b626 100644
--- a/chrome/browser/extensions/extension_protocols.cc
+++ b/chrome/browser/extensions/extension_protocols.cc
@@ -24,6 +24,7 @@
 #include "chrome/common/extensions/extension.h"
 #include "chrome/common/extensions/extension_file_util.h"
 #include "chrome/common/extensions/extension_resource.h"
+#include "chrome/common/extensions/manifest_url_handler.h"
 #include "chrome/common/extensions/web_accessible_resources_handler.h"
 #include "chrome/common/url_constants.h"
 #include "content/public/browser/resource_request_info.h"
@@ -265,6 +266,7 @@ bool ExtensionCanLoadInIncognito(const ResourceRequestInfo* info,
 // first need to find a way to get CanLoadInIncognito state into the renderers.
 bool AllowExtensionResourceLoad(net::URLRequest* request,
                                 bool is_incognito,
+                                const Extension* extension,
                                 ExtensionInfoMap* extension_info_map) {
   const ResourceRequestInfo* info = ResourceRequestInfo::ForRequest(request);
 
@@ -281,6 +283,56 @@ bool AllowExtensionResourceLoad(net::URLRequest* request,
     return false;
   }
 
+  // The following checks are meant to replicate similar set of checks in the
+  // renderer process, performed by ResourceRequestPolicy::CanRequestResource.
+  // These are not exactly equivalent, because we don't have the same bits of
+  // information. The two checks need to be kept in sync as much as possible, as
+  // an exploited renderer can bypass the checks in ResourceRequestPolicy.
+
+  // Check if the extension for which this request is made is indeed loaded in
+  // the process sending the request. If not, we need to explicitly check if
+  // the resource is explicitly accessible or fits in a set of exception cases.
+  // Note: This allows a case where two extensions execute in the same renderer
+  // process to request each other's resources. We can't do more precise check,
+  // since the renderer can lie which extension has made the request.
+  if (extension_info_map->process_map().Contains(
+      request->url().host(), info->GetChildID())) {
+    return true;
+  }
+
+  if (!content::PageTransitionIsWebTriggerable(info->GetPageTransition()))
+    return false;
+
+  // The following checks require that we have an actual extension object. If we
+  // don't have it, allow the request handling to continue with the rest of the
+  // checks.
+  if (!extension)
+    return true;
+
+  // Disallow loading of packaged resources for hosted apps. We don't allow
+  // hybrid hosted/packaged apps. The one exception is access to icons, since
+  // some extensions want to be able to do things like create their own
+  // launchers.
+  std::string resource_root_relative_path =
+      request->url().path().empty() ? "" : request->url().path().substr(1);
+  if (extension->is_hosted_app() &&
+      !extension->icons().ContainsPath(resource_root_relative_path)) {
+    LOG(ERROR) << "Denying load of " << request->url().spec() << " from "
+               << "hosted app.";
+    return false;
+  }
+
+  // If the resource is not expicitly marked as web accessible, it should only
+  // be allowed if it is being loaded by DevTools. A close approximation is
+  // checking if the extension contains DevTools page.
+  // IsResourceWebAccessible already does the manifest version check, so no
+  // need to explicitly do it.
+  if (!extensions::WebAccessibleResourcesInfo::IsResourceWebAccessible(
+          extension, request->url().path()) &&
+      extensions::ManifestURL::GetDevToolsPage(extension).is_empty()) {
+    return false;
+  }
+
   return true;
 }
 
@@ -322,17 +374,18 @@ class ExtensionProtocolHandler
 net::URLRequestJob*
 ExtensionProtocolHandler::MaybeCreateJob(
     net::URLRequest* request, net::NetworkDelegate* network_delegate) const {
+  // chrome-extension://extension-id/resource/path.js
+  const std::string& extension_id = request->url().host();
+  const Extension* extension =
+      extension_info_map_->extensions().GetByID(extension_id);
+
   // TODO(mpcomplete): better error code.
   if (!AllowExtensionResourceLoad(
-           request, is_incognito_, extension_info_map_)) {
+           request, is_incognito_, extension, extension_info_map_)) {
     return new net::URLRequestErrorJob(
         request, network_delegate, net::ERR_ADDRESS_UNREACHABLE);
   }
 
-  // chrome-extension://extension-id/resource/path.js
-  const std::string& extension_id = request->url().host();
-  const Extension* extension =
-      extension_info_map_->extensions().GetByID(extension_id);
   base::FilePath directory_path;
   if (extension)
     directory_path = extension->path();
diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi
index 3fd2f79..81cecd3 100644
--- a/chrome/chrome_tests.gypi
+++ b/chrome/chrome_tests.gypi
@@ -1037,6 +1037,7 @@
         'browser/chrome_content_browser_client_browsertest.cc',
         'browser/chrome_main_browsertest.cc',
         'browser/chrome_plugin_browsertest.cc',
+        'browser/chrome_security_exploit_browsertest.cc',
         'browser/chrome_switches_browsertest.cc',
         'browser/chromeos/accessibility/magnification_manager_browsertest.cc',
         'browser/chromeos/app_mode/kiosk_app_manager_browsertest.cc',
diff --git a/chrome/common/extensions/extension.h b/chrome/common/extensions/extension.h
index 23a292b..fb7f3c8 100644
--- a/chrome/common/extensions/extension.h
+++ b/chrome/common/extensions/extension.h
@@ -56,6 +56,9 @@ class APIPermissionSet;
 class PermissionSet;
 
 // Represents a Chrome extension.
+// Once created, an Extension object is immutable, with the exception of its
+// RuntimeData. This makes it safe to use on any thread, since access to the
+// RuntimeData is protected by a lock.
 class Extension : public base::RefCountedThreadSafe<Extension> {
  public:
   struct ManifestData;
diff --git a/chrome/renderer/extensions/resource_request_policy.cc b/chrome/renderer/extensions/resource_request_policy.cc
index 194ae25..9f8e336 100644
--- a/chrome/renderer/extensions/resource_request_policy.cc
+++ b/chrome/renderer/extensions/resource_request_policy.cc
@@ -23,6 +23,12 @@
 
 namespace extensions {
 
+// This method does a security check whether chrome-extension:// URLs can be
+// requested by the renderer. Since this is in an untrusted process, the browser
+// has a similar check to enforce the policy, in case this process is exploited.
+// If you are changing this function, ensure equivalent checks are added to
+// extension_protocols.cc AllowExtensionResourceLoad.
+
 // static
 bool ResourceRequestPolicy::CanRequestResource(
     const GURL& resource_url,
diff --git a/chrome/test/data/chrome_extension_resource.html b/chrome/test/data/chrome_extension_resource.html
new file mode 100644
index 0000000..34c996c
--- /dev/null
+++ b/chrome/test/data/chrome_extension_resource.html
@@ -0,0 +1,33 @@
+<html>
+<head>
+<script>
+var xhrStatus = -1;
+var imgUrl = 'chrome-extension://eemcgdkfndhakfknompkggombfjjjeno/images/bookmark_manager_recent.png';
+
+window.onload = function() {
+  // The call to pushState with chrome-extension:// URL will succeed, since the
+  // test uses --disable-web-security.
+  history.pushState('', '',
+      'chrome-extension://eemcgdkfndhakfknompkggombfjjjeno/main.html');
+  var xhr = new XMLHttpRequest();
+  xhr.onreadystatechange = function() {
+    if (xhr.readyState == 4) {
+      xhrStatus = xhr.status;    
+      if (xhrStatus == 200) {
+        document.getElementById('star').src =
+            window.URL.createObjectURL(this.response);
+      }
+      domAutomationController.setAutomationId(0);
+      domAutomationController.send(xhr.status);
+    }
+  }
+  xhr.open('GET', imgUrl);
+  xhr.responseType = 'blob';
+  xhr.send();
+}
+</script>
+</head>
+<body>
+<img id='star'>
+</body>
+</html>