Skip to content

GitLab

T
tangled
Commit 03633051 authored by David Bertoni's avatar David Bertoni Committed by Commit Bot
Browse files
Options

[Extensions] Prevent <all_urls> pattern from matching chrome:// scheme 

The chrome:// scheme is sensitive enough that any extension that can
have permission to run on a chrome:// page should have to specially
request access to that page. Prevent <all_urls> from implicitly matching
the chrome:// scheme.

Bug: 821858
Change-Id: Ie321749cd93ee4b51e89f501a9a25088d3df84c4
Reviewed-on: https://chromium-review.googlesource.com/993753
Commit-Queue: David Bertoni <dbertoni@chromium.org>
Reviewed-by: default avatarDevlin <rdevlin.cronin@chromium.org>
Reviewed-by: default avatarDominic Mazzoni <dmazzoni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561688}
parent 74b53bf1
Hide whitespace changes
Inline Side-by-side
Showing with 97 additions and 43 deletions
chrome/browser/extensions/all_urls_apitest.cc
View file @ 03633051
......@@ -87,13 +87,9 @@ IN_PROC_BROWSER_TEST_F(AllUrlsApiTest, WhitelistedExtension) {
ASSERT_TRUE(StartEmbeddedTestServer());
// Now verify that we run content scripts on different URLs, including
// internal pages, data URLs, regular HTTP pages, and resource URLs from
// extensions.
// data URLs, regular HTTP pages, and resource URLs from extensions.
const std::string test_urls[] = {
"chrome://newtab/",
"data:text/html;charset=utf-8,<html>asdf</html>",
"chrome://version/",
"about:blank",
embedded_test_server()->GetURL(kAllUrlsTarget).spec(),
bystander->GetResourceURL("page.html").spec()
};
......
chrome/common/extensions/permissions/permissions_data_unittest.cc
View file @ 03633051
......@@ -323,23 +323,6 @@ TEST(PermissionsDataTest, GetPermissionMessages_ManyHosts) {
"Read and change your data on encrypted.google.com and www.google.com"));
}
TEST(PermissionsDataTest, ExternalFiles) {
GURL external_file("externalfile:abc");
scoped_refptr<const Extension> extension;
// A regular extension shouldn't get access to externalfile: scheme URLs
// even with <all_urls> specified.
extension = GetExtensionWithHostPermission(
"regular_extension", "<all_urls>", Manifest::UNPACKED);
ASSERT_FALSE(extension->permissions_data()->HasHostPermission(external_file));
// Component extensions should get access to externalfile: scheme URLs when
// <all_urls> is specified.
extension = GetExtensionWithHostPermission(
"component_extension", "<all_urls>", Manifest::COMPONENT);
ASSERT_TRUE(extension->permissions_data()->HasHostPermission(external_file));
}
TEST(PermissionsDataTest, ExtensionScheme) {
GURL external_file(
"chrome-extension://abcdefghijklmnopabcdefghijklmnop/index.html");
......@@ -551,20 +534,18 @@ TEST_F(ExtensionScriptAndCaptureVisibleTest, Permissions) {
EXPECT_EQ(DISALLOWED, GetExtensionAccess(extension.get(), settings_url));
EXPECT_EQ(DISALLOWED, GetExtensionAccess(extension.get(), favicon_url));
// Component extensions with <all_urls> should get everything.
// Component extensions with <all_urls> should get everything except for
// "chrome" scheme URLs.
extension = LoadManifest("script_and_capture", "extension_component_all.json",
Manifest::COMPONENT, Extension::NO_FLAGS);
EXPECT_EQ(ALLOWED_SCRIPT_AND_CAPTURE,
GetExtensionAccess(extension.get(), http_url));
EXPECT_EQ(ALLOWED_SCRIPT_AND_CAPTURE,
GetExtensionAccess(extension.get(), https_url));
EXPECT_EQ(ALLOWED_SCRIPT_AND_CAPTURE,
GetExtensionAccess(extension.get(), settings_url));
EXPECT_EQ(ALLOWED_SCRIPT_AND_CAPTURE,
GetExtensionAccess(extension.get(), about_flags_url));
EXPECT_EQ(ALLOWED_SCRIPT_AND_CAPTURE,
GetExtensionAccess(extension.get(), favicon_url));
EXPECT_TRUE(extension->permissions_data()->HasHostPermission(favicon_url));
EXPECT_EQ(DISALLOWED, GetExtensionAccess(extension.get(), settings_url));
// Component extensions should only get access to what they ask for.
extension = LoadManifest("script_and_capture",
......@@ -660,20 +641,18 @@ TEST_F(ExtensionScriptAndCaptureVisibleTest, PermissionsWithChromeURLsEnabled) {
EXPECT_EQ(DISALLOWED, GetExtensionAccess(extension.get(), settings_url));
EXPECT_EQ(DISALLOWED, GetExtensionAccess(extension.get(), favicon_url));
// Component extensions with <all_urls> should get everything.
// Component extensions with <all_urls> should get everything except for
// "chrome" scheme URLs.
extension = LoadManifest("script_and_capture", "extension_component_all.json",
Manifest::COMPONENT, Extension::NO_FLAGS);
EXPECT_EQ(ALLOWED_SCRIPT_AND_CAPTURE,
GetExtensionAccess(extension.get(), http_url));
EXPECT_EQ(ALLOWED_SCRIPT_AND_CAPTURE,
GetExtensionAccess(extension.get(), https_url));
EXPECT_EQ(ALLOWED_SCRIPT_AND_CAPTURE,
GetExtensionAccess(extension.get(), settings_url));
EXPECT_EQ(ALLOWED_SCRIPT_AND_CAPTURE,
GetExtensionAccess(extension.get(), about_flags_url));
EXPECT_EQ(ALLOWED_SCRIPT_AND_CAPTURE,
GetExtensionAccess(extension.get(), favicon_url));
EXPECT_TRUE(extension->permissions_data()->HasHostPermission(favicon_url));
EXPECT_EQ(DISALLOWED, GetExtensionAccess(extension.get(), settings_url));
// Component extensions should only get access to what they ask for.
extension =
......@@ -1059,7 +1038,7 @@ TEST_F(ExtensionScriptAndCaptureVisibleTest, PolicyHostRestrictions) {
EXPECT_EQ(DISALLOWED, GetExtensionAccess(extension.get(), settings_url));
// Component extensions with <all_urls> should get everything regardless of
// policy.
// policy, except for chrome scheme URLs.
extension = LoadManifest("script_and_capture", "extension_component_all.json",
Manifest::COMPONENT, Extension::NO_FLAGS);
extension->permissions_data()->SetDefaultPolicyHostRestrictions(
......@@ -1074,13 +1053,10 @@ TEST_F(ExtensionScriptAndCaptureVisibleTest, PolicyHostRestrictions) {
GetExtensionAccess(extension.get(), test_example_com));
EXPECT_EQ(ALLOWED_SCRIPT_AND_CAPTURE,
GetExtensionAccess(extension.get(), sample_example_com));
EXPECT_EQ(ALLOWED_SCRIPT_AND_CAPTURE,
GetExtensionAccess(extension.get(), settings_url));
EXPECT_EQ(ALLOWED_SCRIPT_AND_CAPTURE,
GetExtensionAccess(extension.get(), about_flags_url));
EXPECT_EQ(ALLOWED_SCRIPT_AND_CAPTURE,
GetExtensionAccess(extension.get(), favicon_url));
EXPECT_TRUE(extension->permissions_data()->HasHostPermission(favicon_url));
EXPECT_EQ(DISALLOWED, GetExtensionAccess(extension.get(), settings_url));
}
} // namespace extensions
extensions/common/BUILD.gn
View file @ 03633051
......@@ -369,6 +369,7 @@ if (enable_extensions) {
"api/declarative_net_request/dnr_manifest_unittest.cc",
"api/printer_provider/usb_printer_manifest_unittest.cc",
"api/sockets/sockets_manifest_permission_unittest.cc",
"component_extension_url_pattern_unittest.cc",
"csp_validator_unittest.cc",
"event_filter_unittest.cc",
"extension_builder_unittest.cc",
......
extensions/common/component_extension_url_pattern_unittest.cc 0 → 100644
View file @ 03633051
// Copyright (c) 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 "extensions/common/constants.h"
#include "extensions/common/extension.h"
#include "extensions/common/extension_builder.h"
#include "extensions/common/manifest.h"
#include "extensions/common/permissions/permissions_data.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "url/gurl.h"
namespace extensions {
namespace {
constexpr int kTabId = 42;
}
TEST(ComponentExtensionUrlPattern, AllUrls) {
// Component extensions do not have access to "chrome" scheme URLs through
// the "<all_urls>" meta-pattern.
auto all_urls = ExtensionBuilder("all urls")
.AddPermission("<all_urls>")
.SetLocation(Manifest::COMPONENT)
.Build();
std::string error;
EXPECT_FALSE(all_urls->permissions_data()->CanAccessPage(
GURL("chrome://settings"), kTabId, &error))
<< error;
// Non-chrome scheme should be fine.
EXPECT_TRUE(all_urls->permissions_data()->CanAccessPage(
GURL("https://example.com"), kTabId, &error))
<< error;
}
TEST(ComponentExtensionUrlPattern, ChromeVoxExtension) {
// The ChromeVox extension has access to "chrome" scheme URLs through the
// "<all_urls>" meta-pattern because it's whitelisted.
auto all_urls = ExtensionBuilder("all urls")
.AddPermission("<all_urls>")
.SetLocation(Manifest::COMPONENT)
.SetID(extension_misc::kChromeVoxExtensionId)
.Build();
std::string error;
EXPECT_TRUE(all_urls->permissions_data()->CanAccessPage(
GURL("chrome://settings"), kTabId, &error))
<< error;
}
TEST(ComponentExtensionUrlPattern, ExplicitChromeUrl) {
// Explicitly specifying a pattern that allows access to the chrome
// scheme is OK.
auto chrome_urls = ExtensionBuilder("chrome urls")
.AddPermission("chrome://*/*")
.SetLocation(Manifest::COMPONENT)
.Build();
std::string error;
EXPECT_TRUE(chrome_urls->permissions_data()->CanAccessPage(
GURL("chrome://settings"), kTabId, &error))
<< error;
}
} // namespace extensions
extensions/common/manifest_handlers/content_scripts_handler.cc
View file @ 03633051
......@@ -144,6 +144,9 @@ std::unique_ptr<UserScript> LoadUserScriptFromDictionary(
const int valid_schemes =
UserScript::ValidUserScriptSchemes(can_execute_script_everywhere);
const bool all_urls_includes_chrome_urls =
PermissionsData::AllUrlsIncludesChromeUrls(extension->id());
for (size_t j = 0; j < matches->GetSize(); ++j) {
std::string match_str;
if (!matches->GetString(j, &match_str)) {
......@@ -165,9 +168,10 @@ std::unique_ptr<UserScript> LoadUserScriptFromDictionary(
}
// TODO(aboxhall): check for webstore
if (!can_execute_script_everywhere &&
if (!all_urls_includes_chrome_urls &&
pattern.scheme() != content::kChromeUIScheme) {
// Exclude SCHEME_CHROMEUI unless it's been explicitly requested.
// Exclude SCHEME_CHROMEUI unless it's been explicitly requested or
// been granted by extension ID.
// If the --extensions-on-chrome-urls flag has not been passed, requesting
// a chrome:// url will cause a parse failure above, so there's no need to
// check the flag here.
......
extensions/common/manifest_handlers/permissions_parser.cc
View file @ 03633051
......@@ -168,6 +168,9 @@ bool ParseHelper(Extension* extension,
? URLPattern::SCHEME_ALL
: Extension::kValidHostPermissionSchemes;
const bool all_urls_includes_chrome_urls =
PermissionsData::AllUrlsIncludesChromeUrls(extension->id());
for (std::vector<std::string>::const_iterator iter = host_data.begin();
iter != host_data.end();
++iter) {
......@@ -189,11 +192,11 @@ bool ParseHelper(Extension* extension,
}
if (pattern.scheme() != content::kChromeUIScheme &&
!can_execute_script_everywhere) {
!all_urls_includes_chrome_urls) {
// Keep chrome:// in allowed schemes only if it's explicitly requested
// or can_execute_script_everywhere is true. If the
// extensions_on_chrome_urls flag is not set, CanSpecifyHostPermission
// will fail, so don't check the flag here.
// or been granted by extension ID. If the extensions_on_chrome_urls
// flag is not set, CanSpecifyHostPermission will fail, so don't check
// the flag here.
valid_schemes &= ~URLPattern::SCHEME_CHROMEUI;
}
pattern.SetValidSchemes(valid_schemes);
......
extensions/common/permissions/permissions_data.cc
View file @ 03633051
......@@ -138,6 +138,12 @@ bool PermissionsData::IsRestrictedUrl(const GURL& document_url,
return false;
}
// static
bool PermissionsData::AllUrlsIncludesChromeUrls(
const std::string& extension_id) {
return extension_id == extension_misc::kChromeVoxExtensionId;
}
bool PermissionsData::UsesDefaultPolicyHostRestrictions() const {
DCHECK(!thread_checker_ || thread_checker_->CalledOnValidThread());
return uses_default_policy_host_restrictions;
......
extensions/common/permissions/permissions_data.h
View file @ 03633051
......@@ -86,6 +86,11 @@ class PermissionsData {
// NOTE: You probably want to use CanAccessPage().
bool IsRestrictedUrl(const GURL& document_url, std::string* error) const;
// Returns true if the "all_urls" meta-pattern should include access to
// URLs with the "chrome" scheme. Access to these URLs is limited as they
// are sensitive.
static bool AllUrlsIncludesChromeUrls(const std::string& extension_id);
// Is this extension using the default scope for policy_blocked_hosts and
// policy_allowed_hosts of the ExtensionSettings policy.
bool UsesDefaultPolicyHostRestrictions() const;
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment