[Extensions] Restrict debugging file:-scheme URLs

Don't allow extensions to debug file:-scheme URLs if the extension does
not have explicit file access (as set in chrome://extensions). Achieve
this by introducing a new virtual method on DevToolsAgentHostClient to
allow the implementor to check if a given host is allowed to be
inspected.

Add regression tests for the same.

Bug: 666299

Change-Id: Icb5ee89bf788643eee166eef83802d10ab825a6c
Reviewed-on: https://chromium-review.googlesource.com/1104954
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#569828}

chrome/browser/extensions/api/debugger/debugger_api.cc

@@ -29,6 +29,7 @@
 #include "chrome/browser/extensions/api/debugger/extension_dev_tools_infobar.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_tab_util.h"
+#include "chrome/browser/extensions/extension_util.h"
 #include "chrome/browser/infobars/infobar_service.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
@@ -83,6 +84,26 @@ void CopyDebuggee(Debuggee* dst, const Debuggee& src) {
     dst->target_id.reset(new std::string(*src.target_id));
 }
 
+// Returns true if the given |Extension| is allowed to attach to the specified
+// |url|.
+bool ExtensionCanAttachToURL(const Extension& extension,
+                             const GURL& url,
+                             Profile* profile,
+                             std::string* error) {
+  // NOTE: The `debugger` permission implies all URLs access (and indicates
+  // such to the user), so we don't check explicit page access. However, we
+  // still need to check if it's an otherwise-restricted URL.
+  if (extension.permissions_data()->IsRestrictedUrl(url, error))
+    return false;
+
+  if (url.SchemeIsFile() && !util::AllowFileAccess(extension.id(), profile)) {
+    *error = debugger_api_constants::kRestrictedError;
+    return false;
+  }
+
+  return true;
+}
+
 }  // namespace
 
 // ExtensionDevToolsClientHost ------------------------------------------------
@@ -97,14 +118,13 @@ class ExtensionDevToolsClientHost : public content::DevToolsAgentHostClient,
  public:
   ExtensionDevToolsClientHost(Profile* profile,
                               DevToolsAgentHost* agent_host,
-                              const std::string& extension_id,
-                              const std::string& extension_name,
+                              scoped_refptr<const Extension> extension,
                               const Debuggee& debuggee);
 
   ~ExtensionDevToolsClientHost() override;
 
   bool Attach();
-  const std::string& extension_id() { return extension_id_; }
+  const std::string& extension_id() { return extension_->id(); }
   DevToolsAgentHost* agent_host() { return agent_host_.get(); }
   void RespondDetachedToPendingRequests();
   void Close();
@@ -119,6 +139,8 @@ class ExtensionDevToolsClientHost : public content::DevToolsAgentHostClient,
   void AgentHostClosed(DevToolsAgentHost* agent_host) override;
   void DispatchProtocolMessage(DevToolsAgentHost* agent_host,
                                const std::string& message) override;
+  bool MayAttachToRenderer(content::RenderFrameHost* render_frame_host,
+                           bool is_webui) override;
 
  private:
   using PendingRequests =
@@ -138,8 +160,7 @@ class ExtensionDevToolsClientHost : public content::DevToolsAgentHostClient,
 
   Profile* profile_;
   scoped_refptr<DevToolsAgentHost> agent_host_;
-  std::string extension_id_;
-  std::string extension_name_;
+  scoped_refptr<const Extension> extension_;
   Debuggee debuggee_;
   content::NotificationRegistrar registrar_;
   int last_request_id_;
@@ -157,13 +178,11 @@ class ExtensionDevToolsClientHost : public content::DevToolsAgentHostClient,
 ExtensionDevToolsClientHost::ExtensionDevToolsClientHost(
     Profile* profile,
     DevToolsAgentHost* agent_host,
-    const std::string& extension_id,
-    const std::string& extension_name,
+    scoped_refptr<const Extension> extension,
     const Debuggee& debuggee)
     : profile_(profile),
       agent_host_(agent_host),
-      extension_id_(extension_id),
-      extension_name_(extension_name),
+      extension_(std::move(extension)),
       last_request_id_(0),
       infobar_(nullptr),
       detach_reason_(api::debugger::DETACH_REASON_TARGET_CLOSED),
@@ -195,17 +214,11 @@ bool ExtensionDevToolsClientHost::Attach() {
 
   // We allow policy-installed extensions to circumvent the normal
   // infobar warning. See crbug.com/693621.
-  const Extension* extension =
-      ExtensionRegistry::Get(profile_)->enabled_extensions().GetByID(
-          extension_id_);
-  // TODO(dgozman): null-checking |extension| below is sketchy.
-  // We probably should not allow debugging in this case. Or maybe
-  // it's never null?
-  if (extension && Manifest::IsPolicyLocation(extension->location()))
+  if (Manifest::IsPolicyLocation(extension_->location()))
     return true;
 
   infobar_ = ExtensionDevToolsInfoBar::Create(
-      extension_id_, extension_name_, this,
+      extension_id(), extension_->name(), this,
       base::Bind(&ExtensionDevToolsClientHost::InfoBarDismissed,
                  base::Unretained(this)));
   return true;
@@ -272,15 +285,15 @@ void ExtensionDevToolsClientHost::SendDetachedEvent() {
   auto event =
       std::make_unique<Event>(events::DEBUGGER_ON_DETACH, OnDetach::kEventName,
                               std::move(args), profile_);
-  EventRouter::Get(profile_)
-      ->DispatchEventToExtension(extension_id_, std::move(event));
+  EventRouter::Get(profile_)->DispatchEventToExtension(extension_id(),
+                                                       std::move(event));
 }
 
 void ExtensionDevToolsClientHost::OnExtensionUnloaded(
     content::BrowserContext* browser_context,
     const Extension* extension,
     UnloadedExtensionReason reason) {
-  if (extension->id() == extension_id_)
+  if (extension->id() == extension_id())
     Close();
 }
 
@@ -320,8 +333,8 @@ void ExtensionDevToolsClientHost::DispatchProtocolMessage(
     auto event =
         std::make_unique<Event>(events::DEBUGGER_ON_EVENT, OnEvent::kEventName,
                                 std::move(args), profile_);
-    EventRouter::Get(profile_)
-        ->DispatchEventToExtension(extension_id_, std::move(event));
+    EventRouter::Get(profile_)->DispatchEventToExtension(extension_id(),
+                                                         std::move(event));
   } else {
     DebuggerSendCommandFunction* function = pending_requests_[id].get();
     if (!function)
@@ -332,6 +345,31 @@ void ExtensionDevToolsClientHost::DispatchProtocolMessage(
   }
 }
 
+bool ExtensionDevToolsClientHost::MayAttachToRenderer(
+    content::RenderFrameHost* render_frame_host,
+    bool is_webui) {
+  if (is_webui)
+    return false;
+
+  if (!render_frame_host)
+    return true;
+
+  std::string error;
+  // We check the site instance URL here (instead of
+  // RenderFrameHost::GetLastCommittedURL()) because it's too early in the
+  // navigation for anything else.
+  const GURL& site_instance_url =
+      render_frame_host->GetSiteInstance()->GetSiteURL();
+
+  if (site_instance_url.is_empty()) {
+    // |site_instance_url| is empty for about:blank. Allow the extension to
+    // attach.
+    return true;
+  }
+
+  return ExtensionCanAttachToURL(*extension_, site_instance_url, profile_,
+                                 &error);
+}
 
 // DebuggerFunction -----------------------------------------------------------
 
@@ -366,8 +404,10 @@ bool DebuggerFunction::InitAgentHost() {
     if (result && web_contents) {
       // TODO(rdevlin.cronin) This should definitely be GetLastCommittedURL().
       GURL url = web_contents->GetVisibleURL();
-      if (extension()->permissions_data()->IsRestrictedUrl(url, &error_))
+
+      if (!ExtensionCanAttachToURL(*extension(), url, GetProfile(), &error_))
         return false;
+
       agent_host_ = DevToolsAgentHost::GetOrCreateFor(web_contents);
     }
   } else if (debuggee_.extension_id) {
@@ -463,8 +503,7 @@ bool DebuggerAttachFunction::RunAsync() {
   }
 
   auto host = std::make_unique<ExtensionDevToolsClientHost>(
-      GetProfile(), agent_host_.get(), extension()->id(), extension()->name(),
-      debuggee_);
+      GetProfile(), agent_host_.get(), extension(), debuggee_);
 
   if (!host->Attach()) {
     FormatErrorMessage(debugger_api_constants::kRestrictedError);

chrome/browser/extensions/api/debugger/debugger_apitest.cc

@@ -189,6 +189,17 @@ IN_PROC_BROWSER_TEST_F(DebuggerApiTest,
   EXPECT_TRUE(RunAttachFunction(other_ext_url, std::string()));
 }
 
+IN_PROC_BROWSER_TEST_F(DebuggerApiTest,
+                       DebuggerAllowedOnFileUrlsWithFileAccess) {
+  EXPECT_TRUE(RunExtensionTestWithArg("debugger_file_access", "enabled"))
+      << message_;
+}
+
+IN_PROC_BROWSER_TEST_F(DebuggerApiTest,
+                       DebuggerNotAllowedOnFileUrlsWithoutAccess) {
+  EXPECT_TRUE(RunExtensionTestNoFileAccess("debugger_file_access")) << message_;
+}
+
 IN_PROC_BROWSER_TEST_F(DebuggerApiTest, InfoBar) {
   int tab_id = SessionTabHelper::IdForTab(
                    browser()->tab_strip_model()->GetActiveWebContents())

chrome/test/data/extensions/api_test/debugger_file_access/background.js

+// Copyright 2018 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.
+
+function openTab(url) {
+  return new Promise((resolve) => {
+    chrome.tabs.onUpdated.addListener(
+        function listener(tabId, changeInfo, tab) {
+      // Note: Use new URL(...).href to compare in order to normalize the URL,
+      // which is important if the path referenced a parent (as happens in the
+      // file urls).
+      if (changeInfo.status !== 'complete' ||
+          (new URL(tab.url)).href !== (new URL(url)).href) {
+        return;
+      }
+      chrome.tabs.onUpdated.removeListener(listener);
+      resolve(tab.id);
+    });
+    chrome.tabs.create({url: url});
+  });
+}
+
+chrome.test.getConfig((config) => {
+  const fileUrl = config.testDataDirectory + '/../body1.html';
+  const expectFileAccess = !!config.customArg;
+
+  console.log(fileUrl);
+
+  chrome.test.runTests([
+    function verifyInitialState() {
+      if (config.customArg)
+        chrome.test.assertEq('enabled', config.customArg);
+      chrome.extension.isAllowedFileSchemeAccess((allowed) => {
+        chrome.test.assertEq(expectFileAccess, allowed);
+        chrome.test.succeed();
+      });
+    },
+
+    function testAttach() {
+      openTab(fileUrl).then((tabId) => {
+        chrome.debugger.attach({tabId: tabId}, '1.1', function() {
+          if (expectFileAccess) {
+            chrome.test.assertNoLastError();
+            chrome.debugger.detach({tabId: tabId}, function() {
+              chrome.test.assertNoLastError();
+              chrome.test.succeed();
+            });
+          } else {
+            chrome.test.assertLastError('Cannot attach to this target.');
+            chrome.test.succeed();
+          }
+        });
+      });
+    },
+
+    function testAttachAndNavigate() {
+      openTab(chrome.runtime.getURL('dummy.html')).then((tabId) => {
+        chrome.debugger.attach({tabId: tabId}, '1.1', function() {
+          chrome.test.assertNoLastError();
+          let responded = false;
+
+          function onResponse() {
+            responded = true;
+            if (expectFileAccess) {
+              chrome.test.assertNoLastError();
+              chrome.tabs.remove(tabId);
+            } else {
+              chrome.test.assertLastError('Detached while handling command.');
+            }
+          }
+
+          function onDetach(from, reason) {
+            console.warn('Detached');
+            chrome.debugger.onDetach.removeListener(onDetach);
+            chrome.test.assertTrue(responded);
+            chrome.test.assertEq(tabId, from.tabId);
+            chrome.test.assertEq('target_closed', reason);
+            chrome.test.succeed();
+          }
+
+          chrome.debugger.onDetach.addListener(onDetach);
+          chrome.debugger.sendCommand({tabId: tabId}, 'Page.navigate',
+                                      {url: fileUrl}, onResponse);
+        });
+      });
+    },
+  ]);
+});

chrome/test/data/extensions/api_test/debugger_file_access/dummy.html

+<html></html>

chrome/test/data/extensions/api_test/debugger_file_access/manifest.json

+{
+  "name": "Debugger File Access Test",
+  "manifest_version": 2,
+  "version": "0.1",
+  "permissions": ["debugger", "tabs"],
+  "background": {
+    "scripts": ["background.js"]
+  }
+}

content/browser/devtools/devtools_agent_host_impl.cc

@@ -310,16 +310,11 @@ void DevToolsAgentHostImpl::ForceDetachAllSessions() {
   }
 }
 
-void DevToolsAgentHostImpl::ForceDetachRestrictedSessions() {
-  if (sessions_.empty())
-    return;
+void DevToolsAgentHostImpl::ForceDetachRestrictedSessions(
+    const std::vector<DevToolsSession*>& restricted_sessions) {
   scoped_refptr<DevToolsAgentHostImpl> protect(this);
-  std::vector<DevToolsSession*> restricted;
-  for (DevToolsSession* session : sessions_) {
-    if (session->restricted())
-      restricted.push_back(session);
-  }
-  for (DevToolsSession* session : restricted) {
+
+  for (DevToolsSession* session : restricted_sessions) {
     DevToolsAgentHostClient* client = session->client();
     DetachClient(client);
     client->AgentHostClosed(this);

content/browser/devtools/devtools_agent_host_impl.h

@@ -74,7 +74,8 @@ class CONTENT_EXPORT DevToolsAgentHostImpl : public DevToolsAgentHost {
   void NotifyNavigated();
   void NotifyCrashed(base::TerminationStatus status);
   void ForceDetachAllSessions();
-  void ForceDetachRestrictedSessions();
+  void ForceDetachRestrictedSessions(
+      const std::vector<DevToolsSession*>& restricted_sessions);
   DevToolsIOContext* GetIOContext() { return &io_context_; }
 
   base::flat_set<DevToolsSession*>& sessions() { return sessions_; }

content/browser/devtools/render_frame_devtools_agent_host.cc

@@ -427,8 +427,11 @@ WebContents* RenderFrameDevToolsAgentHost::GetWebContents() {
 }
 
 bool RenderFrameDevToolsAgentHost::AttachSession(DevToolsSession* session) {
-  if (session->restricted() && !IsFrameHostAllowedForRestrictedSessions())
+  const bool is_webui =
+      frame_host_ && (frame_host_->web_ui() || frame_host_->pending_web_ui());
+  if (!session->client()->MayAttachToRenderer(frame_host_, is_webui))
     return false;
+
   session->SetRenderer(frame_host_ ? frame_host_->GetProcess()->GetID()
                                    : ChildProcessHost::kInvalidUniqueID,
                        frame_host_);
@@ -592,8 +595,17 @@ void RenderFrameDevToolsAgentHost::UpdateFrameHost(
   frame_host_ = frame_host;
   agent_ptr_.reset();
 
-  if (!IsFrameHostAllowedForRestrictedSessions())
-    ForceDetachRestrictedSessions();
+  std::vector<DevToolsSession*> restricted_sessions;
+  const bool is_webui =
+      frame_host && (frame_host->web_ui() || frame_host->pending_web_ui());
+
+  for (DevToolsSession* session : sessions()) {
+    if (!session->client()->MayAttachToRenderer(frame_host, is_webui))
+      restricted_sessions.push_back(session);
+  }
+
+  if (!restricted_sessions.empty())
+    ForceDetachRestrictedSessions(restricted_sessions);
 
   if (!render_frame_alive_) {
     render_frame_alive_ = true;
@@ -961,9 +973,4 @@ bool RenderFrameDevToolsAgentHost::IsChildFrame() {
   return frame_tree_node_ && frame_tree_node_->parent();
 }
 
-bool RenderFrameDevToolsAgentHost::IsFrameHostAllowedForRestrictedSessions() {
-  return !frame_host_ ||
-         (!frame_host_->web_ui() && !frame_host_->pending_web_ui());
-}
-
 }  // namespace content

content/browser/devtools/render_frame_devtools_agent_host.h

@@ -183,7 +183,6 @@ class CONTENT_EXPORT RenderFrameDevToolsAgentHost
   void OnPageScaleFactorChanged(float page_scale_factor) override;
 
   bool IsChildFrame();
-  bool IsFrameHostAllowedForRestrictedSessions();
 
   void OnSwapCompositorFrame(const IPC::Message& message);
   void DestroyOnRenderFrameGone();

content/public/browser/BUILD.gn

@@ -102,6 +102,7 @@ jumbo_source_set("browser_sources") {
     "desktop_media_id.cc",
     "desktop_media_id.h",
     "devtools_agent_host.h",
+    "devtools_agent_host_client.cc",
     "devtools_agent_host_client.h",
     "devtools_agent_host_observer.cc",
     "devtools_agent_host_observer.h",

content/public/browser/devtools_agent_host_client.cc

+// Copyright 2018 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 "content/public/browser/devtools_agent_host_client.h"
+
+namespace content {
+
+bool DevToolsAgentHostClient::MayAttachToRenderer(
+    content::RenderFrameHost* render_frame_host,
+    bool is_webui) {
+  return true;
+}
+
+}  // namespace content

content/public/browser/devtools_agent_host_client.h

@@ -12,6 +12,7 @@
 namespace content {
 
 class DevToolsAgentHost;
+class RenderFrameHost;
 
 // DevToolsAgentHostClient can attach to a DevToolsAgentHost and start
 // debugging it.
@@ -25,6 +26,11 @@ class CONTENT_EXPORT DevToolsAgentHostClient {
 
   // This method is called when attached agent host is closed.
   virtual void AgentHostClosed(DevToolsAgentHost* agent_host) = 0;
+
+  // Returns true if the client is allowed to attach to the given renderer.
+  // Note: this method may be called before navigation commits.
+  virtual bool MayAttachToRenderer(content::RenderFrameHost* render_frame_host,
+                                   bool is_webui);
 };
 
 }  // namespace content