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

chrome/browser/extensions/extension_protocols.cc

 // Copyright (c) 2012 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 "chrome/browser/extensions/extension_protocols.h"
 
 #include <algorithm>
 
 #include "base/compiler_specific.h"
 #include "base/file_path.h"
 #include "base/logging.h"
 #include "base/memory/weak_ptr.h"
 #include "base/message_loop.h"
 #include "base/path_service.h"
 #include "base/string_util.h"
 #include "base/stringprintf.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/threading/worker_pool.h"
 #include "build/build_config.h"
 #include "chrome/browser/extensions/extension_info_map.h"
 #include "chrome/browser/extensions/image_loader.h"
 #include "chrome/browser/net/chrome_url_request_context.h"
 #include "chrome/common/chrome_paths.h"
 #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"
 #include "extensions/common/constants.h"
 #include "googleurl/src/url_util.h"
 #include "grit/component_extension_resources_map.h"
 #include "net/base/mime_util.h"
 #include "net/base/net_errors.h"
 #include "net/http/http_response_headers.h"
 #include "net/http/http_response_info.h"
@@ skipping 221 matching lines @@
   }
 
   return true;
 }
 
 // Returns true if an chrome-extension:// resource should be allowed to load.
 // TODO(aa): This should be moved into ExtensionResourceRequestPolicy, but we
 // 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);
 
   // We have seen crashes where info is NULL: crbug.com/52374.
   if (!info) {
     LOG(ERROR) << "Allowing load of " << request->url().spec()
                << "from unknown origin. Could not find user data for "
                << "request.";
     return true;
   }
 
   if (is_incognito && !ExtensionCanLoadInIncognito(info, request->url().host(),
                                                    extension_info_map)) {
     return false;
   }
 
+  // The following checks are meant to replicate similar set of checks in the

[Charlie Reis, 2013/02/22 02:45:17]

Should we add a similar warning to resource_request_policy.cc, so that people
who make changes there know they need to look here as well?  I would never be
able to find both places on my own.  :)

[nasko, 2013/02/22 17:14:35]

Done.

+  // renderer process, performed by ResourceRequestPolicy::CanRequestResource.
+  // These are not exact equivalent, because we don't have the same bits of

[Charlie Reis, 2013/02/22 02:45:17]

nit: exact -> exactly

[nasko, 2013/02/22 17:14:35]

Done.

+  // 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;
+  }
+
+  // TODO(nasko): Based on MaybeCreateJob, extension can be null, so check here.
+  // Should we succeed or fail here?

[Charlie Reis, 2013/02/22 02:45:17]

Can we decide this, rather than leaving it as a TODO?

If there's no extension found for the request's URL, then it seems safe to
return true here.

[nasko, 2013/02/22 17:14:35]

Why allow it if there is no existing extension for what is being requested? I'm
actually for a fail-safe behavior, so I've changed it to return false.

[Matt Perry, 2013/02/22 18:56:40]

It likely means the extension has been unloaded, so it's fine to disallow the
request. It will have failed anyway.

+  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.

[Charlie Reis, 2013/02/22 02:45:17]

Duplicating this code from resource_request_policy.cc makes me a bit
uncomfortable, but I guess it's a small enough chunk that it's not worth
factoring out, given your comment above.

[nasko, 2013/02/22 17:14:35]

Done.

+  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;
+  }
+
+  if (!content::PageTransitionIsWebTriggerable(info->GetPageTransition()))
+    return false;
+
   return true;
 }
 
 // Returns true if the given URL references an icon in the given extension.
 bool URLIsForExtensionIcon(const GURL& url, const Extension* extension) {
   DCHECK(url.SchemeIs(extensions::kExtensionScheme));
 
   if (!extension)
     return false;
 
@@ skipping 21 matching lines @@
  private:
   const bool is_incognito_;
   ExtensionInfoMap* const extension_info_map_;
   DISALLOW_COPY_AND_ASSIGN(ExtensionProtocolHandler);
 };
 
 // Creates URLRequestJobs for extension:// URLs.
 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();
   if (directory_path.value().empty()) {
     const Extension* disabled_extension =
         extension_info_map_->disabled_extensions().GetByID(extension_id);
     if (URLIsForExtensionIcon(request->url(), disabled_extension))
       directory_path = disabled_extension->path();
     if (directory_path.value().empty()) {
       LOG(WARNING) << "Failed to GetPathForExtension: " << extension_id;
       return NULL;
     }
   }
 
   std::string content_security_policy;
   bool send_cors_header = false;
   if (extension) {
     std::string resource_path = request->url().path();
     content_security_policy =
         extension->GetResourceContentSecurityPolicy(resource_path);
     if ((extension->manifest_version() >= 2 ||
          extensions::WebAccessibleResourcesInfo::HasWebAccessibleResources(
              extension)) &&
         extensions::WebAccessibleResourcesInfo::IsResourceWebAccessible(
-            extension, resource_path))
+            extension, resource_path)) {

[Charlie Reis, 2013/02/22 02:45:17]

nit: Technically this is legal by the style guide, so we probably shouldn't add
the code churn.  Just makes svn annotate pages harder to follow.

[nasko, 2013/02/22 17:14:35]

Done.

       send_cors_header = true;
+    }
   }
 
   std::string path = request->url().path();
   if (path.size() > 1 &&
       path.substr(1) == extension_filenames::kGeneratedBackgroundPageFilename) {
     return new GeneratedBackgroundPageJob(
         request, network_delegate, extension, content_security_policy);
   }
 
   base::FilePath resources_path;
@@ skipping 31 matching lines @@
                                     send_cors_header);
 }
 
 }  // namespace
 
 net::URLRequestJobFactory::ProtocolHandler* CreateExtensionProtocolHandler(
     bool is_incognito,
     ExtensionInfoMap* extension_info_map) {
   return new ExtensionProtocolHandler(is_incognito, extension_info_map);
 }