chrome.contentSettings API: Do not allow wildcard patterns that match extension URLs (issue 2730533002 by meacer@chromium.org)

[mea...@chromium.org]

Reviewers: Devlin
CL: https://codereview.chromium.org/2730533002/

Message:
rdevlin.cronin@: PTAL?

Description:
chrome.contentSettings API: Do not allow wildcard patterns that match extension
URLs

BUG=677714

Affected files (+60, -3 lines):
M chrome/browser/extensions/api/content_settings/content_settings_api.cc
M chrome/test/data/extensions/api_test/content_settings/unsupporteddefaultsettings/test.js
M components/content_settings/core/common/BUILD.gn
M components/content_settings/core/common/DEPS
M components/content_settings/core/common/content_settings_pattern.h
M components/content_settings/core/common/content_settings_pattern.cc
M components/content_settings/core/common/content_settings_pattern_unittest.cc


Index: chrome/browser/extensions/api/content_settings/content_settings_api.cc
diff --git a/chrome/browser/extensions/api/content_settings/content_settings_api.cc b/chrome/browser/extensions/api/content_settings/content_settings_api.cc
index c63d8f45e015fc794c3b8d4ccf7e9ed7c85be237..51e795481ab3848523cd0944a7f7b29c069ca747 100644
--- a/chrome/browser/extensions/api/content_settings/content_settings_api.cc
+++ b/chrome/browser/extensions/api/content_settings/content_settings_api.cc
@@ -214,8 +214,11 @@ ContentSettingsContentSettingSetFunction::Run() {
// some values might not be supported.
// For example, camera supports [allow, ask, block] for exceptions, but only
// [ask, block] for the default setting.
- if (primary_pattern == ContentSettingsPattern::Wildcard() &&
- secondary_pattern == ContentSettingsPattern::Wildcard() &&
+ // Also, do not allow wildcard patterns that match chrome-extension URLs.
+ if (((primary_pattern.MatchesExtensionUrls() &&
+ secondary_pattern.MatchesExtensionUrls()) ||
+ (primary_pattern == ContentSettingsPattern::Wildcard() &&
+ secondary_pattern == ContentSettingsPattern::Wildcard())) &&
!HostContentSettingsMap::IsDefaultSettingAllowedForType(setting,
content_type)) {
static const char kUnsupportedDefaultSettingError[] =
Index: chrome/test/data/extensions/api_test/content_settings/unsupporteddefaultsettings/test.js
diff --git a/chrome/test/data/extensions/api_test/content_settings/unsupporteddefaultsettings/test.js b/chrome/test/data/extensions/api_test/content_settings/unsupporteddefaultsettings/test.js
index d57487041ffcc461c9b2bdcb6f39504ca3c8f575..e7574e7903f73a78f0dc75290b7daf6e12da4fd3 100644
--- a/chrome/test/data/extensions/api_test/content_settings/unsupporteddefaultsettings/test.js
+++ b/chrome/test/data/extensions/api_test/content_settings/unsupporteddefaultsettings/test.js
@@ -36,7 +36,7 @@ function expectFalse(message) {
}

chrome.test.runTests([
- function setDefaultContentSettings() {
+ function setDefaultContentSettingsAllUrls() {
settings.forEach(function(type, setting) {
cs[type].set({
'primaryPattern': '<all_urls>',
@@ -47,6 +47,32 @@ chrome.test.runTests([
"' is not supported as the default setting of " + type + "."));
});
},
+
+ // Patterns with scheme wildcards and extension IDs shouldn't be added.
+ function setDefaultContentSettingsExtensionUrlWildcard() {
+ settings.forEach(function(type, setting) {
+ cs[type].set({
+ 'primaryPattern': '*://' + chrome.runtime.id + '/*',
+ 'secondaryPattern': '*://' + chrome.runtime.id + '/*',
+ 'setting': setting
+ },
+ chrome.test.callbackFail("'" + setting +
+ "' is not supported as the default setting of " + type + "."));
+ });
+ },
+
+ // Patterns with chrome-extension scheme and extension IDs shouldn't be added.
+ function setDefaultContentSettingsExtensionUrlWildcard() {
+ settings.forEach(function(type, setting) {
+ cs[type].set({
+ 'primaryPattern': 'chrome-extension://' + chrome.runtime.id + '/*',
+ 'secondaryPattern': 'chrome-extension://' + chrome.runtime.id + '/*',
+ 'setting': setting
+ },
+ chrome.test.callbackFail("Invalid scheme."));
+ });
+ },
+
function setExceptions() {
settings.forEach(function(type, setting) {
cs[type].set({
Index: components/content_settings/core/common/BUILD.gn
diff --git a/components/content_settings/core/common/BUILD.gn b/components/content_settings/core/common/BUILD.gn
index 42404221620090bf37de7d687fefa8a87d135a1c..d00951022c6806bc2e155a59ddc28fda40a44d45 100644
--- a/components/content_settings/core/common/BUILD.gn
+++ b/components/content_settings/core/common/BUILD.gn
@@ -23,6 +23,7 @@ static_library("common") {
"//base",
"//mojo/public/cpp/bindings:struct_traits",
"//net",
+ "//third_party/re2",
"//url",
]
}
Index: components/content_settings/core/common/DEPS
diff --git a/components/content_settings/core/common/DEPS b/components/content_settings/core/common/DEPS
index 4e501b277a109fbcd61ef6b99845c9137f4b6775..426d09ff5227aa39bcaebb73b0504f173db831f6 100644
--- a/components/content_settings/core/common/DEPS
+++ b/components/content_settings/core/common/DEPS
@@ -2,5 +2,6 @@ include_rules = [
"+mojo/public/cpp/bindings",
"+net/base",
"+testing",
+ "+third_party/re2",
"+url",
]
Index: components/content_settings/core/common/content_settings_pattern.cc
diff --git a/components/content_settings/core/common/content_settings_pattern.cc b/components/content_settings/core/common/content_settings_pattern.cc
index a83a6959a02240575eae95fe7168e4ea75c644bc..f2694c27eae180ff92e4582aaf1c26275df800b0 100644
--- a/components/content_settings/core/common/content_settings_pattern.cc
+++ b/components/content_settings/core/common/content_settings_pattern.cc
@@ -14,6 +14,7 @@
#include "base/strings/string_util.h"
#include "components/content_settings/core/common/content_settings_pattern_parser.h"
#include "net/base/url_util.h"
+#include "third_party/re2/src/re2/re2.h"
#include "url/gurl.h"

namespace {
@@ -533,6 +534,13 @@ ContentSettingsPattern::ContentSettingsPattern(
is_valid_(valid) {
}

+bool ContentSettingsPattern::MatchesExtensionUrls() const {
+ const char kExtensionIdRegex[] = "^[a-zA-Z]{32}";
+ return (parts_.is_scheme_wildcard || parts_.scheme == "chrome-extension") &&
+ (parts_.has_domain_wildcard ||
+ RE2::FullMatch(parts_.host, kExtensionIdRegex));
+}
+
bool ContentSettingsPattern::Matches(
const GURL& url) const {
// An invalid pattern matches nothing.
Index: components/content_settings/core/common/content_settings_pattern.h
diff --git a/components/content_settings/core/common/content_settings_pattern.h b/components/content_settings/core/common/content_settings_pattern.h
index 7d6fc8d00dd04d3e700a806eccc368907dd6f21e..36db8f72a9a41b274af36196836d6a6fd911ed12 100644
--- a/components/content_settings/core/common/content_settings_pattern.h
+++ b/components/content_settings/core/common/content_settings_pattern.h
@@ -190,6 +190,11 @@ class ContentSettingsPattern {
// True if |url| matches this pattern.
bool Matches(const GURL& url) const;

+ // True if the pattern matches any extension URL (i.e. the scheme is
+ // a wildcard or chrome-extension, and the hostname is a wildcard or looks
+ // like an extension ID).
+ bool MatchesExtensionUrls() const;
+
// True if this pattern matches all hosts (i.e. it has a host wildcard).
bool MatchesAllHosts() const;

Index: components/content_settings/core/common/content_settings_pattern_unittest.cc
diff --git a/components/content_settings/core/common/content_settings_pattern_unittest.cc b/components/content_settings/core/common/content_settings_pattern_unittest.cc
index d5aab80ab86a3d0d69fde5eef8acd91633b49729..90a1dc13aded144c831c4d53fa179bdb31bb8cb4 100644
--- a/components/content_settings/core/common/content_settings_pattern_unittest.cc
+++ b/components/content_settings/core/common/content_settings_pattern_unittest.cc
@@ -317,6 +317,19 @@ TEST(ContentSettingsPatternTest, FromString_FilePatterns) {
pattern.Compare(file_wildcard));
}

+TEST(ContentSettingsPatternTest, FromString_MatchesExtensionUrls) {
+ EXPECT_TRUE(Pattern("chrome-extension://peoadpeiejnhkmpaakpnompolbglelel/")
+ .MatchesExtensionUrls());
+ EXPECT_FALSE(
+ Pattern("chrome-extension://peoadpeiejnhkmpaakpnompolbglelelaaaa/")
+ .MatchesExtensionUrls());
+
+ EXPECT_TRUE(
+ Pattern("*://peoadpeiejnhkmpaakpnompolbglelel/").MatchesExtensionUrls());
+ EXPECT_FALSE(Pattern("*://peoadpeiejnhkmpaakpnompolbglelelaaaa/")
+ .MatchesExtensionUrls());
+}
+
TEST(ContentSettingsPatternTest, FromString_ExtensionPatterns) {
EXPECT_TRUE(Pattern("chrome-extension://peoadpeiejnhkmpaakpnompolbglelel/")
.IsValid());

[msr...@chromium.org]

Drive-by, since the current approach disables much more than just extension
patterns.


https://codereview.chromium.org/2730533002/diff/20001/components/content_settings/core/common/content_settings_pattern.cc
File components/content_settings/core/common/content_settings_pattern.cc
(right):

https://codereview.chromium.org/2730533002/diff/20001/components/content_settings/core/common/content_settings_pattern.cc#newcode537
components/content_settings/core/common/content_settings_pattern.cc:537:
bool ContentSettingsPattern::MatchesExtensionUrls() const {
This is a layering violation. It's not the first extensions-related
layering violation in content settings code, so I guess this is more of
a question to Devlin - can we do something around it?

Could we have something like components/extensions_base with very basic
information about extensions, e.g. that they have a 32-char ID?

https://codereview.chromium.org/2730533002/diff/20001/components/content_settings/core/common/content_settings_pattern.cc#newcode538
components/content_settings/core/common/content_settings_pattern.cc:538:

const char kExtensionIdRegex[] = "^[a-zA-Z]{32}";

It's actually [a-p]{32}.

https://cs.chromium.org/chromium/src/extensions/common/extension_id.h?type=cs&q=extension+ID+32&l=13

Also, why no "$" at the end? This would match longer IDs as well.

https://codereview.chromium.org/2730533002/diff/20001/components/content_settings/core/common/content_settings_pattern.cc#newcode539
components/content_settings/core/common/content_settings_pattern.cc:539:

return (parts_.is_scheme_wildcard || parts_.scheme ==
"chrome-extension") &&

"*://[*.]google.com" is a valid scheme that both is_scheme_wildcard and
has_domain_wildcard, yet it doesn't match extensions. Adding a check if
host is empty will probably help. Because if host is nonempty and there
is a wildcard, then the hostname must contain at least one dot between
them, and thus it cannot be an extension.

"*://mylocalnetworkfileserverabcdefgh" is an internal hostname that will
be matched, but it shouldn't be.

This also matches "*", i.e. ContentSettingsPattern::Wildcard(), thus
preventing extensions from setting the default setting.

I'd prefer the following solution:
a) if the scheme is chrome-extension://, the API returns an error
message and refuses to add it
b) for broader exceptions, try to resolve them in GetWebsiteSetting()
(though I'd have to think a bit how exactly).

https://codereview.chromium.org/2730533002/

[rdevlin...@chromium.org]

https://codereview.chromium.org/2730533002/diff/20001/chrome/browser/extensions/api/content_settings/content_settings_api.cc
File
chrome/browser/extensions/api/content_settings/content_settings_api.cc
(right):

https://codereview.chromium.org/2730533002/diff/20001/chrome/browser/extensions/api/content_settings/content_settings_api.cc#newcode217
chrome/browser/extensions/api/content_settings/content_settings_api.cc:217:

// Also, do not allow wildcard patterns that match chrome-extension
URLs.

I don't know if this is desirable. What if I have an extension that
manages my content settings and I want it to be able to restrict what
other extensions can do?

https://codereview.chromium.org/2730533002/diff/20001/chrome/browser/extensions/api/content_settings/content_settings_api.cc#newcode219
chrome/browser/extensions/api/content_settings/content_settings_api.cc:219:
secondary_pattern.MatchesExtensionUrls()) ||
Some settings don't have a secondary url, or don't always consider it.
Those would never be taken into account since the pattern wouldn't match
extension urls.

https://codereview.chromium.org/2730533002/diff/20001/components/content_settings/core/common/content_settings_pattern.cc
File components/content_settings/core/common/content_settings_pattern.cc
(right):

https://codereview.chromium.org/2730533002/diff/20001/components/content_settings/core/common/content_settings_pattern.cc#newcode537
components/content_settings/core/common/content_settings_pattern.cc:537:
bool ContentSettingsPattern::MatchesExtensionUrls() const {

On 2017/03/02 09:50:40, msramek wrote:
> This is a layering violation. It's not the first extensions-related
layering
> violation in content settings code, so I guess this is more of a
question to
> Devlin - can we do something around it?
>
> Could we have something like components/extensions_base with very
basic
> information about extensions, e.g. that they have a 32-char ID?

For this, we should just be able to use crx_file::id_util::IdIsValid
(which is in components/crx_file).

https://cs.chromium.org/chromium/src/components/crx_file/id_util.h?q=crx_file/id_util+package:%5Echromium$&l=39

https://codereview.chromium.org/2730533002/

[mea...@chromium.org]

Sorry for the delay, PTAL?

https://codereview.chromium.org/2730533002/diff/20001/chrome/browser/extensions/api/content_settings/content_settings_api.cc
File
chrome/browser/extensions/api/content_settings/content_settings_api.cc
(right):

https://codereview.chromium.org/2730533002/diff/20001/chrome/browser/extensions/api/content_settings/content_settings_api.cc#newcode217
chrome/browser/extensions/api/content_settings/content_settings_api.cc:217:
// Also, do not allow wildcard patterns that match chrome-extension
URLs.

On 2017/03/02 21:55:02, Devlin wrote:
> I don't know if this is desirable. What if I have an extension that
manages my
> content settings and I want it to be able to restrict what other
extensions can
> do?

Note that this CL only changes behavior for microphone and video
permissions.

But besides that, adding chrome-extension patterns currently doesn't
work either. I also don't think we should allow them unless we go around
and divide contentSettings API into two (one that can only downgrade
permissions and one that can do everything).

Otherwise, an extension can similarly grant all these permissions to
other extensions. Also, our stance has been that one extension shouldn't
be able to tamper with another extension and this seems consistent with
that.

https://codereview.chromium.org/2730533002/diff/20001/chrome/browser/extensions/api/content_settings/content_settings_api.cc#newcode219
chrome/browser/extensions/api/content_settings/content_settings_api.cc:219:
secondary_pattern.MatchesExtensionUrls()) ||

On 2017/03/02 21:55:02, Devlin wrote:
> Some settings don't have a secondary url, or don't always consider it.
Those
> would never be taken into account since the pattern wouldn't match
extension
> urls.

I added the secondary pattern for consistency with the wildcard check.
The secondary pattern seems to be used for iframes by most permissions,
and for subresources by cookies.

The scenario this prevents is as follows: Extension A grants permissions
to extension B by adding it as a secondary pattern, then iframes it (if
extension B has a web accessible resource), then communicates with it.
That way extension A will be able to use granted permissions itself, if
extension B cooperates.

https://codereview.chromium.org/2730533002/diff/20001/components/content_settings/core/common/content_settings_pattern.cc
File components/content_settings/core/common/content_settings_pattern.cc
(right):

https://codereview.chromium.org/2730533002/diff/20001/components/content_settings/core/common/content_settings_pattern.cc#newcode537
components/content_settings/core/common/content_settings_pattern.cc:537:
bool ContentSettingsPattern::MatchesExtensionUrls() const {

On 2017/03/02 21:55:02, Devlin wrote:
> On 2017/03/02 09:50:40, msramek wrote:
> > This is a layering violation. It's not the first extensions-related
layering
> > violation in content settings code, so I guess this is more of a
question to
> > Devlin - can we do something around it?
> >
> > Could we have something like components/extensions_base with very
basic
> > information about extensions, e.g. that they have a 32-char ID?
>
> For this, we should just be able to use crx_file::id_util::IdIsValid
(which is
> in components/crx_file).
>
>
https://cs.chromium.org/chromium/src/components/crx_file/id_util.h?q=crx_file/id_util+package:%5Echromium$&l=39

Thanks! I searched a lot for an existing function but couldn't find it.
(was searching for "ExtensionId" though :)

As for the layering violation: The other option is to expose the host
portion of the pattern and do the matching in the extension code. But
given that it's only going to be used here, it seemed better to have a
specific function.

https://codereview.chromium.org/2730533002/diff/20001/components/content_settings/core/common/content_settings_pattern.cc#newcode538
components/content_settings/core/common/content_settings_pattern.cc:538:
const char kExtensionIdRegex[] = "^[a-zA-Z]{32}";

On 2017/03/02 09:50:40, msramek wrote:

> It's actually [a-p]{32}.
>
>
https://cs.chromium.org/chromium/src/extensions/common/extension_id.h?type=cs&q=extension+ID+32&l=13
>
> Also, why no "$" at the end? This would match longer IDs as well.

Not sure why I put a-z. But removed in favor of IsIdValid.

https://codereview.chromium.org/2730533002/diff/20001/components/content_settings/core/common/content_settings_pattern.cc#newcode539
components/content_settings/core/common/content_settings_pattern.cc:539:
return (parts_.is_scheme_wildcard || parts_.scheme ==
"chrome-extension") &&

On 2017/03/02 09:50:40, msramek wrote:

> "*://[*.]google.com" is a valid scheme that both is_scheme_wildcard
and
> has_domain_wildcard, yet it doesn't match extensions. Adding a check
if host is
> empty will probably help. Because if host is nonempty and there is a
wildcard,
> then the hostname must contain at least one dot between them, and thus
it cannot
> be an extension.

Thanks for pointing this out, I though has_domain_wildcard actually
meant "*" and not subdomain wildcard.

>
> "*://mylocalnetworkfileserverabcdefgh" is an internal hostname that
will be
> matched, but it shouldn't be.

That is correct, but I don't think there is much we can do here. There
is no way to distinguish 32 character hostnames from extension IDs, so
we are being extra restrictive.

>
> This also matches "*", i.e. ContentSettingsPattern::Wildcard(), thus
preventing
> extensions from setting the default setting.
>
> I'd prefer the following solution:
> a) if the scheme is chrome-extension://, the API returns an error
message and
> refuses to add it

(a) is already true, the API returns "Invalid Scheme" error for
chrome-extension patterns.

> > b) for broader exceptions, try to resolve them in
GetWebsiteSetting() (though
> I'd have to think a bit how exactly).

I thought about doing this too, but that sounds like it'll to break the
case where a user enters the pattern manually. Is that correct?

It might actually be a good idea to do that since the page info bubble
doesn't allow changing permissions for chrome-extension patterns, but I
think we should consider it for a separate CL.

https://codereview.chromium.org/2730533002/

[mea...@chromium.org]

[mea...@chromium.org]

ping :)

https://codereview.chromium.org/2730533002/

[msr...@chromium.org]

Sorry! Perf time...

https://codereview.chromium.org/2730533002/diff/20001/components/content_settings/core/common/content_settings_pattern.cc
File components/content_settings/core/common/content_settings_pattern.cc
(right):

https://codereview.chromium.org/2730533002/diff/20001/components/content_settings/core/common/content_settings_pattern.cc#newcode539
components/content_settings/core/common/content_settings_pattern.cc:539:
return (parts_.is_scheme_wildcard || parts_.scheme ==
"chrome-extension") &&

On 2017/03/07 20:50:38, Mustafa Emre Acer wrote:
> On 2017/03/02 09:50:40, msramek wrote:
> > "*://[*.]google.com" is a valid scheme that both is_scheme_wildcard
and
> > has_domain_wildcard, yet it doesn't match extensions. Adding a check
if host
> is
> > empty will probably help. Because if host is nonempty and there is a
wildcard,
> > then the hostname must contain at least one dot between them, and
thus it
> cannot
> > be an extension.
>
> Thanks for pointing this out, I though has_domain_wildcard actually
meant "*"
> and not subdomain wildcard.
>
> >
> > "*://mylocalnetworkfileserverabcdefgh" is an internal hostname that
will be
> > matched, but it shouldn't be.
>
> That is correct, but I don't think there is much we can do here. There
is no way
> to distinguish 32 character hostnames from extension IDs, so we are
being extra
> restrictive.

Well, that's not exactly true. There's no way in this layer indeed, but
in the chrome/ layer, you could query ExtensionService whether an
extension with such an ID exists. It's of course still possible that
there is a local hostname equal to an ID of an installed extension,
but... smaller chance.

>
> >
> > This also matches "*", i.e. ContentSettingsPattern::Wildcard(), thus
> preventing
> > extensions from setting the default setting.

Note that this is still true for the current implementation.
ContentSettingsPattern::Wildcard() has all its |parts_| set to
wildcards, and therefore matches the second disjunct of the condition.

And while it is true that Wildcard() does match extensions, it also
represents the default setting which is now out of reach for extensions.
That's a major regression. There's no way around that on this layer, and
therefore I maintain that the solution should not be done here.

> >
> > I'd prefer the following solution:
> > a) if the scheme is chrome-extension://, the API returns an error
message and
> > refuses to add it
>
> (a) is already true, the API returns "Invalid Scheme" error for
chrome-extension
> patterns.
>
> > > b) for broader exceptions, try to resolve them in
GetWebsiteSetting()
> (though
> > I'd have to think a bit how exactly).
>
> I thought about doing this too, but that sounds like it'll to break
the case
> where a user enters the pattern manually. Is that correct?

Correct. Look into HostContentSettingsMap::GetWebsiteSettingInternal().
There's a for loop that iterates over providers. See the list of
providers here:

https://cs.chromium.org/chromium/src/components/content_settings/core/browser/host_content_settings_map.h?sq=package:chromium&dr=CSs&l=51

You'll want to add a parameter that excludes CUSTOM_EXTENSION_PROVIDER.

>
> It might actually be a good idea to do that since the page info bubble
doesn't
> allow changing permissions for chrome-extension patterns, but I think
we should
> consider it for a separate CL.

To summarize the above paragraphs: I think the correct solution is not
to completely remove offending patterns. Instead, when we're calculating
the permission for a chrome-extension:// origin, we should tell
HostContentSettingsMap to ignore all rules that were sourced by an
extension. Which means adding

if (skip_extensions && provider == CUSTOM_EXTENSION_PROVIDER)
continue;

to HostContentSettingsMap::GetWebsiteSettingInternal.

This is still in components/, so still a layering violation, but this
one is already there, so I'll live with it.

https://codereview.chromium.org/2730533002/

[rdevlin...@chromium.org]

extensions lgtm

https://codereview.chromium.org/2730533002/