[Extensions] Have URLPattern::Contains() properly check schemes

Have URLPattern::Contains() properly check the schemes of the patterns
when evaluating if one pattern contains another. This is important in
order to prevent extensions from requesting chrome:-scheme permissions
via the permissions API when <all_urls> is specified as an optional
permission.

Bug: 859600,918470

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

chrome/browser/extensions/api/permissions/permissions_api_helpers.cc

@@ -10,6 +10,7 @@
 #include "base/json/json_writer.h"
 #include "base/values.h"
 #include "chrome/common/extensions/api/permissions.h"
+#include "content/public/common/url_constants.h"
 #include "extensions/common/error_utils.h"
 #include "extensions/common/extension.h"
 #include "extensions/common/permissions/permission_set.h"
@@ -149,6 +150,25 @@ bool UnpackOriginPermissions(const std::vector<std::string>& origins_input,
     explicit_schemes &= ~URLPattern::SCHEME_FILE;
   }
 
+  auto filter_chrome_scheme = [](URLPattern* pattern) {
+    // We disallow the chrome:-scheme unless the pattern is explicitly
+    // "chrome://..." - that is, <all_urls> should not match the chrome:-scheme.
+    // Patterns which explicitly specify the chrome:-scheme are safe, since
+    // manifest parsing won't allow them unless the kExtensionsOnChromeURLs
+    // switch is enabled.
+    // Note that we don't check PermissionsData::AllUrlsIncludesChromeUrls()
+    // here, since that's only needed for Chromevox (which doesn't use optional
+    // permissions).
+    if (pattern->scheme() != content::kChromeUIScheme) {
+      // NOTE: We use pattern->valid_schemes() here (instead of
+      // |user_script_schemes| or |explicit_schemes|) because
+      // URLPattern::Parse() can mutate the valid schemes for a pattern, and we
+      // don't want to override those changes.
+      pattern->SetValidSchemes(pattern->valid_schemes() &
+                               ~URLPattern::SCHEME_CHROMEUI);
+    }
+  };
+
   for (const auto& origin_str : origins_input) {
     URLPattern explicit_origin(explicit_schemes);
     URLPattern::ParseResult parse_result = explicit_origin.Parse(origin_str);
@@ -159,6 +179,8 @@ bool UnpackOriginPermissions(const std::vector<std::string>& origins_input,
       return false;
     }
 
+    filter_chrome_scheme(&explicit_origin);
+
     bool used_origin = false;
     if (required_permissions.explicit_hosts().ContainsPattern(
             explicit_origin)) {
@@ -172,11 +194,13 @@ bool UnpackOriginPermissions(const std::vector<std::string>& origins_input,
 
     URLPattern scriptable_origin(user_script_schemes);
     if (scriptable_origin.Parse(origin_str) ==
-            URLPattern::ParseResult::kSuccess &&
-        required_permissions.scriptable_hosts().ContainsPattern(
-            scriptable_origin)) {
-      used_origin = true;
-      result->required_scriptable_hosts.AddPattern(scriptable_origin);
+        URLPattern::ParseResult::kSuccess) {
+      filter_chrome_scheme(&scriptable_origin);
+      if (required_permissions.scriptable_hosts().ContainsPattern(
+              scriptable_origin)) {
+        used_origin = true;
+        result->required_scriptable_hosts.AddPattern(scriptable_origin);
+      }
     }
 
     if (!used_origin)

chrome/browser/extensions/api/permissions/permissions_api_helpers_unittest.cc

@@ -330,4 +330,28 @@ TEST(ExtensionPermissionsAPIHelpers, Unpack_UnsupportedAPIPermission) {
       APIPermission::kWallpaper));
 }
 
+// Tests that unpacking works correctly with wildcard schemes (which are
+// interesting, because they only match http | https, and not all schemes).
+TEST(ExtensionPermissionsAPIHelpers, Unpack_WildcardSchemes) {
+  constexpr char kWildcardSchemePattern[] = "*://*/*";
+
+  PermissionSet optional_permissions(
+      APIPermissionSet(), ManifestPermissionSet(),
+      URLPatternSet({URLPattern(Extension::kValidHostPermissionSchemes,
+                                kWildcardSchemePattern)}),
+      URLPatternSet());
+
+  Permissions permissions_object;
+  permissions_object.origins = std::make_unique<std::vector<std::string>>(
+      std::vector<std::string>({kWildcardSchemePattern}));
+
+  std::string error;
+  std::unique_ptr<UnpackPermissionSetResult> unpack_result =
+      UnpackPermissionSet(permissions_object, PermissionSet(),
+                          optional_permissions, true, &error);
+  ASSERT_TRUE(unpack_result) << error;
+  EXPECT_THAT(GetPatternsAsStrings(unpack_result->optional_explicit_hosts),
+              testing::ElementsAre(kWildcardSchemePattern));
+}
+
 }  // namespace extensions

chrome/browser/extensions/api/permissions/permissions_api_unittest.cc

@@ -19,6 +19,7 @@
 #include "components/crx_file/id_util.h"
 #include "extensions/common/extension_builder.h"
 #include "extensions/common/extension_features.h"
+#include "extensions/common/manifest_handlers/permissions_parser.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -26,6 +27,9 @@ namespace extensions {
 
 namespace {
 
+constexpr char kNotInManifestError[] =
+    "Only permissions specified in the manifest may be requested.";
+
 using permissions_test_util::GetPatternsAsStrings;
 
 scoped_refptr<const Extension> CreateExtensionWithPermissions(
@@ -556,7 +560,7 @@ TEST_F(PermissionsAPIUnitTest, RequestingPermissionsNotSpecifiedInManifest) {
   auto function = base::MakeRefCounted<PermissionsRequestFunction>();
   function->set_user_gesture(true);
   function->set_extension(extension.get());
-  EXPECT_EQ("Only permissions specified in the manifest may be requested.",
+  EXPECT_EQ(kNotInManifestError,
             extension_function_test_utils::RunFunctionAndReturnError(
                 function.get(),
                 R"([{
@@ -617,4 +621,52 @@ TEST_F(PermissionsAPIUnitTest, RequestingAlreadyGrantedWithheldPermissions) {
           kGoogleCom));
 }
 
+// Test that requesting chrome:-scheme URLs is disallowed in the permissions
+// API.
+TEST_F(PermissionsAPIUnitTest, RequestingChromeURLs) {
+  scoped_refptr<const Extension> extension =
+      ExtensionBuilder("extension")
+          .SetManifestKey("optional_permissions",
+                          ListBuilder().Append("<all_urls>").Build())
+          .Build();
+  AddExtensionAndGrantPermissions(*extension);
+
+  const GURL chrome_url("chrome://settings");
+
+  // By default, the extension should not have access to chrome://settings.
+  EXPECT_FALSE(extension->permissions_data()->HasHostPermission(chrome_url));
+  // The optional permissions should also omit the chrome:-scheme for the
+  // <all_urls> pattern.
+  EXPECT_FALSE(PermissionsParser::GetOptionalPermissions(extension.get())
+                   .explicit_hosts()
+                   .MatchesURL(chrome_url));
+
+  {
+    // Trying to request "chrome://settings/*" should fail, since it's not in
+    // the optional permissions.
+    auto function = base::MakeRefCounted<PermissionsRequestFunction>();
+    function->set_user_gesture(true);
+    function->set_extension(extension.get());
+    std::string error =
+        extension_function_test_utils::RunFunctionAndReturnError(
+            function.get(), R"([{"origins": ["chrome://settings/*"]}])",
+            browser(), api_test_utils::NONE);
+    EXPECT_EQ(kNotInManifestError, error);
+  }
+  // chrome://settings should still be restricted.
+  EXPECT_FALSE(extension->permissions_data()->HasHostPermission(chrome_url));
+
+  // The extension can request <all_urls>, but it should not grant access to the
+  // chrome:-scheme.
+  std::unique_ptr<const PermissionSet> prompted_permissions;
+  RunRequestFunction(*extension, browser(), R"([{"origins": ["<all_urls>"]}])",
+                     &prompted_permissions);
+  EXPECT_THAT(GetPatternsAsStrings(prompted_permissions->effective_hosts()),
+              testing::UnorderedElementsAre("<all_urls>"));
+
+  EXPECT_FALSE(extension->permissions_data()->HasHostPermission(chrome_url));
+  EXPECT_TRUE(extension->permissions_data()->HasHostPermission(
+      GURL("https://example.com")));
+}
+
 }  // namespace extensions

chrome/browser/extensions/permissions_updater_unittest.cc

@@ -703,8 +703,10 @@ TEST_F(PermissionsUpdaterTest, ChromeFaviconIsNotARevokableHost) {
         ExtensionBuilder("all urls extension")
             .AddPermission("<all_urls>")
             .Build();
-    URLPattern all_urls_pattern(Extension::kValidHostPermissionSchemes,
-                                "<all_urls>");
+    URLPattern all_urls_pattern(
+        Extension::kValidHostPermissionSchemes &
+            ~(URLPattern::SCHEME_CHROMEUI | URLPattern::SCHEME_FILE),
+        "<all_urls>");
     PermissionsUpdater updater(profile());
     updater.InitializePermissions(extension.get());
 
@@ -720,11 +722,8 @@ TEST_F(PermissionsUpdaterTest, ChromeFaviconIsNotARevokableHost) {
 
     std::unique_ptr<const PermissionSet> revokable_permissions =
         updater.GetRevokablePermissions(extension.get());
-    // TODO(https://crbug.com/859600): <all_urls> will report containing
-    // chrome://favicon/, even though it shouldn't since the scheme doesn't
-    // match.
-    // EXPECT_FALSE(revokable_permissions->explicit_hosts().ContainsPattern(
-    //     chrome_favicon_pattern));
+    EXPECT_FALSE(revokable_permissions->explicit_hosts().ContainsPattern(
+        chrome_favicon_pattern));
     EXPECT_TRUE(revokable_permissions->explicit_hosts().ContainsPattern(
         all_urls_pattern));
 

extensions/common/permissions/permissions_data.cc

@@ -387,10 +387,16 @@ bool PermissionsData::CanCaptureVisiblePage(
     has_active_tab = tab_permissions &&
                      tab_permissions->HasAPIPermission(APIPermission::kTab);
 
-    const URLPattern all_urls(URLPattern::SCHEME_ALL,
-                              URLPattern::kAllUrlsPattern);
-    has_all_urls =
-        active_permissions_unsafe_->explicit_hosts().ContainsPattern(all_urls);
+    // Check if any of the host permissions match all urls. We don't use
+    // URLPatternSet::ContainsPattern() here because a) the schemes may be
+    // different and b) this is more efficient.
+    for (const auto& pattern : active_permissions_unsafe_->explicit_hosts()) {
+      if (pattern.match_all_urls()) {
+        has_all_urls = true;
+        break;
+      }
+    }
+
     has_page_capture = active_permissions_unsafe_->HasAPIPermission(
         APIPermission::kPageCapture);
   }

extensions/common/url_pattern.cc

@@ -352,6 +352,10 @@ URLPattern::ParseResult URLPattern::Parse(base::StringPiece pattern,
 }
 
 void URLPattern::SetValidSchemes(int valid_schemes) {
+  // TODO(devlin): Should we check that valid_schemes agrees with |scheme_|
+  // here? Otherwise, valid_schemes_ and schemes_ may stop agreeing with each
+  // other (e.g., in the case of `*://*/*`, where the scheme should only be
+  // http or https).
   spec_.clear();
   valid_schemes_ = valid_schemes;
 }
@@ -433,6 +437,8 @@ bool URLPattern::MatchesURL(const GURL& test) const {
     test_url = test.inner_url();
   }
 
+  // Ensure the scheme matches first, since <all_urls> may not match this URL if
+  // the scheme is excluded.
   if (!MatchesScheme(test_url->scheme_piece()))
     return false;
 
@@ -632,8 +638,14 @@ bool URLPattern::OverlapsWith(const URLPattern& other) const {
 }
 
 bool URLPattern::Contains(const URLPattern& other) const {
-  if (match_all_urls())
+  // Important: it's not enough to just check match_all_urls(); we also need to
+  // make sure that the schemes in this pattern are a superset of those in
+  // |other|.
+  if (match_all_urls() &&
+      (valid_schemes_ & other.valid_schemes_) == other.valid_schemes_) {
     return true;
+  }
+
   return MatchesAllSchemes(other.GetExplicitSchemes()) &&
          MatchesHost(other.host()) &&
          (!other.match_subdomains_ || match_subdomains_) &&

extensions/common/url_pattern_unittest.cc

@@ -1250,4 +1250,53 @@ TEST(ExtensionURLPatternTest, ValidSchemeIntersection) {
   }
 }
 
+// Tests that <all_urls> patterns correctly check schemes when testing if one
+// contains the other.
+TEST(ExtensionURLPatternTest, ContainsSchemes) {
+  const URLPattern http(URLPattern::SCHEME_HTTP, URLPattern::kAllUrlsPattern);
+  const URLPattern chrome(URLPattern::SCHEME_CHROMEUI,
+                          URLPattern::kAllUrlsPattern);
+  const URLPattern http_and_https(
+      URLPattern::SCHEME_HTTP | URLPattern::SCHEME_HTTPS,
+      URLPattern::kAllUrlsPattern);
+  const URLPattern http_https_and_chrome(URLPattern::SCHEME_HTTP |
+                                             URLPattern::SCHEME_HTTPS |
+                                             URLPattern::SCHEME_CHROMEUI,
+                                         URLPattern::kAllUrlsPattern);
+
+  // A map between each URLPattern and the other patterns it should contain.
+  const std::map<const URLPattern*, std::set<const URLPattern*>> contains_map =
+      {
+          {&http, {}},
+          {&chrome, {}},
+          {&http_and_https, {&http}},
+          {&http_https_and_chrome, {&http, &http_and_https, &chrome}},
+      };
+
+  const URLPattern* all_patterns[] = {&http, &chrome, &http_and_https,
+                                      &http_https_and_chrome};
+
+  // Verify that each pattern contains exactly the expected patterns.
+  for (const auto& entry : contains_map) {
+    const URLPattern* pattern = entry.first;
+    const std::set<const URLPattern*>& contains_patterns = entry.second;
+    for (const URLPattern* other_pattern : all_patterns) {
+      SCOPED_TRACE(base::StringPrintf("Checking if %d contains %d",
+                                      pattern->valid_schemes(),
+                                      other_pattern->valid_schemes()));
+      bool expect_contains =
+          // Patterns should always contain themselves.
+          pattern == other_pattern || contains_patterns.count(other_pattern);
+      EXPECT_EQ(expect_contains, pattern->Contains(*other_pattern));
+    }
+  }
+
+  // Fun edge case for bonus points: |http| doesn't contain all the valid
+  // schemes of the other pattern, but does in practice (since the scheme is
+  // restricted to http by the match pattern).
+  EXPECT_TRUE(http.Contains(
+      URLPattern(URLPattern::SCHEME_HTTP | URLPattern::SCHEME_HTTPS,
+                 "http://google.com/*")));
+}
+
 }  // namespace