[Extensions] Update captureVisibleTab permission checks

tabs.captureVisibleTab() is interesting. It used to work with every
URL ever, and just checked for if activeTab or <all_urls> was present.
However, this led to some issues where the extension could capture pages
it shouldn't be able to, such as https://crbug.com/810220.

Changing this to only allow access to pages that the extension had
explicit access to also broke important use cases. For instance, taking
screenshots of chrome:-scheme pages is important for filing bugs,
tracking features, etc. This similarly broke use cases for other URLs
that extensions don't have full access to, like other extensions' pages.
An exception was added for chrome:-scheme pages, but this was
insufficient for other cases.

Adjust the permissions check to allow page capture of otherwise-
restricted pages if the extension has activeTab granted. We require
activeTab (rather than either <all_urls> or activeTab) because it gives
a stronger guarantee that the user wants the extension to run on the
given site.

Note: this does not allow any other action on these restricted pages;
only capturing the page is permitted.

The new behavior enforces the following permission requirements for
the host types:
<arbitrary web page>: activeTab OR <all_urls>
file:-scheme page: (activeTab OR <all_urls>) AND
                   explicit file access from chrome://extensions.
Pages restricted by enterprise policy: blocked
Extension's own page: activeTab OR <all_urls>
Another extension's page: activeTab
chrome:-scheme page: activeTab

Support for IPv6 pages and pseudo urls is dependent on these being
properly supported with activeTab, which will be pursued in a followup.

Bug: 839857

Change-Id: Ied3a71732cd5d41ad16f9b459f6fda9b1815edaf
Reviewed-on: https://chromium-review.googlesource.com/1102902Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#568471}

chrome/browser/extensions/active_tab_unittest.cc

@@ -331,6 +331,35 @@ TEST_F(ActiveTabTest, GrantToSinglePage) {
   EXPECT_TRUE(IsBlocked(extension_without_active_tab, chromium));
 }
 
+TEST_F(ActiveTabTest, CapturingPagesWithActiveTab) {
+  std::vector<GURL> test_urls = {
+      GURL("https://example.com"), GURL("chrome://version"),
+      GURL("chrome://newtab"),
+      // IPv6 addresses don't work with activeTab: https://crbug.com/853064.
+      //    {"http://[2607:f8b0:4005:805::200e]"},
+      extension->GetResourceURL("test.html"),
+      another_extension->GetResourceURL("test.html"),
+  };
+
+  const GURL kAboutBlank("about:blank");
+
+  for (const GURL& url : test_urls) {
+    SCOPED_TRACE(url);
+    NavigateAndCommit(url);
+    // By default, there should be no access.
+    EXPECT_FALSE(extension->permissions_data()->CanCaptureVisiblePage(
+        url, tab_id(), nullptr /*error*/));
+    // Granting permission should allow page capture.
+    active_tab_permission_granter()->GrantIfRequested(extension.get());
+    EXPECT_TRUE(extension->permissions_data()->CanCaptureVisiblePage(
+        url, tab_id(), nullptr /*error*/));
+    // Navigating away should revoke access.
+    NavigateAndCommit(kAboutBlank);
+    EXPECT_FALSE(extension->permissions_data()->CanCaptureVisiblePage(
+        url, tab_id(), nullptr /*error*/));
+  }
+}
+
 TEST_F(ActiveTabTest, Uninstalling) {
   // Some semi-arbitrary setup.
   GURL google("http://www.google.com");

chrome/common/extensions/permissions/permissions_data_unittest.cc

@@ -32,6 +32,7 @@
 #include "extensions/common/value_builder.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/gurl.h"
+#include "url/origin.h"
 
 using base::UTF16ToUTF8;
 using content::SocketPermissionRequest;
@@ -1059,4 +1060,195 @@ TEST_F(ExtensionScriptAndCaptureVisibleTest, PolicyHostRestrictions) {
   EXPECT_EQ(DISALLOWED, GetExtensionAccess(extension.get(), settings_url));
 }
 
+class CaptureVisiblePageTest : public testing::Test {
+ public:
+  CaptureVisiblePageTest() = default;
+  ~CaptureVisiblePageTest() override = default;
+
+  bool CanCapture(const Extension& extension, const GURL& url) {
+    return extension.permissions_data()->CanCaptureVisiblePage(
+        url, kTabId, nullptr /*error*/);
+  }
+
+  void GrantActiveTab(const GURL& url) {
+    APIPermissionSet tab_api_permissions;
+    tab_api_permissions.insert(APIPermission::kTab);
+    URLPatternSet tab_hosts;
+    tab_hosts.AddOrigin(UserScript::ValidUserScriptSchemes(),
+                        url::Origin::Create(url).GetURL());
+    PermissionSet tab_permissions(tab_api_permissions, ManifestPermissionSet(),
+                                  tab_hosts, tab_hosts);
+    active_tab_->permissions_data()->UpdateTabSpecificPermissions(
+        kTabId, tab_permissions);
+  }
+
+  void ClearActiveTab() {
+    active_tab_->permissions_data()->ClearTabSpecificPermissions(kTabId);
+  }
+
+  const Extension& all_urls() { return *all_urls_; }
+
+  const Extension& active_tab() { return *active_tab_; }
+
+  static constexpr int kTabId = 42;
+
+ private:
+  void SetUp() override {
+    all_urls_ = ExtensionBuilder("all urls")
+                    .AddPermission("<all_urls>")
+                    .SetID(std::string(32, 'a'))
+                    .Build();
+    active_tab_ = ExtensionBuilder("active tab")
+                      .AddPermission("activeTab")
+                      .SetID(std::string(32, 'b'))
+                      .Build();
+  }
+
+  void TearDown() override {
+    all_urls_ = nullptr;
+    active_tab_ = nullptr;
+  }
+
+  scoped_refptr<const Extension> all_urls_;
+  scoped_refptr<const Extension> active_tab_;
+
+  DISALLOW_COPY_AND_ASSIGN(CaptureVisiblePageTest);
+};
+
+TEST_F(CaptureVisiblePageTest, URLsCapturableWithEitherActiveTabOrAllURLs) {
+  const GURL test_urls[] = {
+      // Normal web page.
+      GURL("https://example.com"),
+
+      // TODO(https://crbug.com/853064): IPv6 pages should behave like normal
+      // web pages.
+      // GURL("http://[2607:f8b0:4005:805::200e]"),
+
+      // filesystem: urls with web origins should behave like normal web pages.
+      // TODO(https://crbug.com/853392): filesystem: URLs don't work with
+      // activeTab.
+      // GURL("filesystem:http://example.com/foo"),
+
+      // blob: urls with web origins should behave like normal web pages.
+      // TODO(https://crbug.com/853392): blob: URLs don't work with activeTab.
+      // GURL("blob:http://example.com/bar"),
+  };
+
+  for (const GURL& url : test_urls) {
+    SCOPED_TRACE(url.spec());
+    EXPECT_TRUE(CanCapture(all_urls(), url));
+
+    EXPECT_FALSE(CanCapture(active_tab(), url));
+    GrantActiveTab(url);
+    EXPECT_TRUE(CanCapture(active_tab(), url));
+    ClearActiveTab();
+    EXPECT_FALSE(CanCapture(active_tab(), url));
+  }
+}
+
+TEST_F(CaptureVisiblePageTest, URLsCapturableOnlyWithActiveTab) {
+  // The following URLs are origins that extensions are able to capture only if
+  // they have activeTab granted. We require explicit user action for a higher
+  // bar, since normally these pages are completely restricted to extensions at
+  // all.
+  const GURL test_urls[] = {
+      // Another extension's URL.
+      GURL("chrome-extension://cccccccccccccccccccccccccccccccc/foo.html"),
+
+      // filesystem: urls behave like the underlying origin.
+      // https://crbug.com/853392: filesystem: URLs don't work with activeTab.
+      // GURL("filesystem:chrome-extension://cccccccccccccccccccccccccccccccc/foo"),
+
+      // blob: urls behave like the underlying origin.
+      // https://crbug.com/853392: blob: URLs don't work with activeTab.
+      // GURL("blob:chrome-extension://cccccccccccccccccccccccccccccccc/bar"),
+
+      // data: urls have no associated origin, so are more restricted.
+      GURL("data:text/html;charset=utf-8,<html>Hello!</html>"),
+
+      // A chrome:-scheme page.
+      GURL("chrome://settings"),
+
+      // The NTP.
+      GURL("chrome://newtab"),
+  };
+
+  for (const GURL& url : test_urls) {
+    SCOPED_TRACE(url.spec());
+    EXPECT_FALSE(CanCapture(all_urls(), url));
+
+    EXPECT_FALSE(CanCapture(active_tab(), url));
+    GrantActiveTab(url);
+    EXPECT_TRUE(CanCapture(active_tab(), url));
+    ClearActiveTab();
+    EXPECT_FALSE(CanCapture(active_tab(), url));
+  }
+}
+
+TEST_F(CaptureVisiblePageTest, SelfExtensionURLs) {
+  auto get_filesystem_url_for_extension = [](const Extension& extension) {
+    return GURL(base::StringPrintf("filesystem:chrome-extension://%s/foo",
+                                   extension.id().c_str()));
+  };
+  auto get_blob_url_for_extension = [](const Extension& extension) {
+    return GURL(base::StringPrintf("blob:chrome-extension://%s/foo",
+                                   extension.id().c_str()));
+  };
+
+  // Extensions should be allowed to capture their own pages with either
+  // activeTab or <all_urls>. We still require one of the two because there may
+  // be other web content within the extension page, so we can't auto-grant
+  // access.
+
+  {
+    EXPECT_TRUE(CanCapture(all_urls(), all_urls().GetResourceURL("foo.html")));
+    EXPECT_TRUE(
+        CanCapture(all_urls(), get_filesystem_url_for_extension(all_urls())));
+    EXPECT_TRUE(CanCapture(all_urls(), get_blob_url_for_extension(all_urls())));
+  }
+
+  const GURL active_tab_extension_urls[] = {
+      active_tab().GetResourceURL("foo.html"),
+      // https://crbug.com/853392: filesystem: URLs don't work with activeTab.
+      // get_filesystem_url_for_extension(active_tab()),
+      // https://crbug.com/853392: blob: URLs don't work with activeTab.
+      // get_blob_url_for_extension(active_tab()),
+  };
+
+  for (const GURL& url : active_tab_extension_urls) {
+    SCOPED_TRACE(url);
+
+    EXPECT_FALSE(CanCapture(active_tab(), url));
+    GrantActiveTab(url);
+    EXPECT_TRUE(CanCapture(active_tab(), url));
+    ClearActiveTab();
+    EXPECT_FALSE(CanCapture(active_tab(), url));
+  }
+}
+
+TEST_F(CaptureVisiblePageTest, PolicyBlockedURLs) {
+  {
+    URLPattern example_com(URLPattern::SCHEME_ALL, "https://example.com/*");
+    URLPattern chrome_settings(URLPattern::SCHEME_ALL, "chrome://settings/*");
+    URLPatternSet blocked_patterns({example_com, chrome_settings});
+    PermissionsData::SetDefaultPolicyHostRestrictions(blocked_patterns,
+                                                      URLPatternSet());
+  }
+
+  const GURL test_urls[] = {
+      GURL("https://example.com"), GURL("chrome://settings/"),
+  };
+
+  for (const GURL& url : test_urls) {
+    SCOPED_TRACE(url);
+    EXPECT_FALSE(CanCapture(all_urls(), url));
+
+    EXPECT_FALSE(CanCapture(active_tab(), url));
+    GrantActiveTab(url);
+    EXPECT_FALSE(CanCapture(active_tab(), url));
+    ClearActiveTab();
+    EXPECT_FALSE(CanCapture(active_tab(), url));
+  }
+}
+
 }  // namespace extensions

extensions/common/permissions/permissions_data.cc

@@ -19,6 +19,7 @@
 #include "extensions/common/switches.h"
 #include "extensions/common/url_pattern_set.h"
 #include "url/gurl.h"
+#include "url/origin.h"
 #include "url/url_constants.h"
 
 namespace extensions {
@@ -366,52 +367,100 @@ bool PermissionsData::CanCaptureVisiblePage(const GURL& document_url,
                                             int tab_id,
                                             std::string* error) const {
   bool has_active_tab = false;
+  bool has_all_urls = false;
+  // Check the real origin, in order to account for filesystem:, blob:, etc.
+  // (url::Origin grabs the inner origin of these, whereas GURL::GetOrigin()
+  // does not.)
+  const GURL origin = url::Origin::Create(document_url).GetURL();
   {
     base::AutoLock auto_lock(runtime_lock_);
+    // Disallow capturing policy-blocked hosts. No exceptions.
+    // Note: This isn't foolproof, since an extension could embed a policy-
+    // blocked host in a different page and then capture that, but it's better
+    // than nothing (and policy hosts can set their x-frame options
+    // accordingly).
+    if (location_ != Manifest::COMPONENT && IsPolicyBlockedHostUnsafe(origin)) {
+      if (error)
+        *error = extension_misc::kPolicyBlockedScripting;
+      return false;
+    }
+
     const PermissionSet* tab_permissions = GetTabSpecificPermissions(tab_id);
     has_active_tab = tab_permissions &&
                      tab_permissions->HasAPIPermission(APIPermission::kTab);
-  }
-  // We check GetPageAccess() (in addition the the <all_urls> and activeTab
-  // checks below) for the case of URLs that can be conditionally granted (such
-  // as file:// URLs or chrome:// URLs for component extensions), and to respect
-  // policy restrictions, if any. If an extension has <all_urls>,
-  // GetPageAccess() will still (correctly) return false if, for instance, the
-  // URL is a file:// URL and the extension does not have file access.
-  // See https://crbug.com/810220.
-  if (GetPageAccess(document_url, tab_id, error) != PageAccess::kAllowed) {
-    if (!document_url.SchemeIs(content::kChromeUIScheme))
-      return false;
 
-    // Most extensions will not have (and cannot get) access to chrome:// URLs
-    // (which are restricted). However, allowing them to capture these URLs can
-    // be useful, such as in the case of capturing a screenshot. Allow
-    // extensions that have been explicitly invoked (and have the activeTab)
-    // permission to capture chrome:// URLs.
-    if (has_active_tab)
-      return true;
+    const URLPattern all_urls(URLPattern::SCHEME_ALL,
+                              URLPattern::kAllUrlsPattern);
+    has_all_urls =
+        active_permissions_unsafe_->explicit_hosts().ContainsPattern(all_urls);
+  }
 
+  // At least one of activeTab or <all_urls> is always required; no exceptions.
+  if (!has_active_tab && !has_all_urls) {
     if (error)
-      *error = manifest_errors::kActiveTabPermissionNotGranted;
-
+      *error = manifest_errors::kAllURLOrActiveTabNeeded;
     return false;
   }
 
-  const URLPattern all_urls(URLPattern::SCHEME_ALL,
-                            URLPattern::kAllUrlsPattern);
-
-  base::AutoLock auto_lock(runtime_lock_);
-  if (active_permissions_unsafe_->explicit_hosts().ContainsPattern(all_urls))
+  // We check GetPageAccess() (in addition to the <all_urls> and activeTab
+  // checks below) for the case of URLs that can be conditionally granted (such
+  // as file:// URLs or chrome:// URLs for component extensions).
+  // If an extension has <all_urls>, GetPageAccess() will still (correctly)
+  // return false if, for instance, the URL is a file:// URL and the extension
+  // does not have file access.
+  // See https://crbug.com/810220.
+  // If the extension has page access (and has activeTab or <all_urls>, as
+  // checked above), allow the capture.
+  std::string access_error;
+  if (GetPageAccess(origin, tab_id, &access_error) == PageAccess::kAllowed)
     return true;
 
-  const PermissionSet* tab_permissions = GetTabSpecificPermissions(tab_id);
-  if (tab_permissions &&
-      tab_permissions->HasAPIPermission(APIPermission::kTab)) {
+  // The extension doesn't have explicit page access. However, there are a
+  // number of cases where tab capture may still be allowed.
+
+  // First special case: an extension's own pages.
+  // These aren't restricted URLs, but won't be matched by <all_urls> or
+  // activeTab (since the extension scheme is not included in the list of valid
+  // schemes for extension permissions).
+  // To capture an extension's own page, either activeTab or <all_urls> is
+  // needed (it's no higher privilege than a normal web page). At least one
+  // of these is still needed because the extension page may have embedded
+  // web content.
+  // TODO(devlin): Should activeTab/<all_urls> account for the extension's own
+  // domain?
+  if (origin.host() == extension_id_)
     return true;
+
+  // The following are special cases that require activeTab explicitly. Normal
+  // extensions will never have full access to these pages (i.e., can never
+  // inject scripts or otherwise modify the page), but capturing the page can
+  // still be useful for e.g. screenshots. We allow these pages only if the
+  // extension has been explicitly granted activeTab, which serves as a
+  // stronger guarantee that the user wants to run the extension on the site.
+  // These origins include:
+  // - chrome:-scheme pages.
+  // - Other extension's pages.
+  // - data: URLs (which don't have a defined underlying origin).
+  // TODO(devlin): Include the Webstore in this list?
+  bool allowed_with_active_tab =
+      origin.SchemeIs(content::kChromeUIScheme) ||
+      origin.SchemeIs(kExtensionScheme) ||
+      // Note: The origin of a data: url is empty, so check the url itself.
+      document_url.SchemeIs(url::kDataScheme);
+
+  if (!allowed_with_active_tab) {
+    if (error)
+      *error = access_error;
+    return false;
   }
 
+  // If the extension has activeTab, these origins are allowed.
+  if (has_active_tab)
+    return true;
+
+  // Otherwise, access is denied.
   if (error)
-    *error = manifest_errors::kAllURLOrActiveTabNeeded;
+    *error = manifest_errors::kActiveTabPermissionNotGranted;
   return false;
 }