chrome.contentSettings API: Block patterns that match extension URLs for mic and video

components/content_settings/core/common/content_settings_pattern.cc

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 {

[msramek, 2017/03/02 09:50:40]

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?

[Devlin, 2017/03/02 21:55:02]

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...

[meacer, 2017/03/07 20:50:38]

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.

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

[msramek, 2017/03/02 09:50:40]

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

https://cs.chromium.org/chromium/src/extensions/common/extension_id.h?type=cs...

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

[meacer, 2017/03/07 20:50:38]

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

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

[msramek, 2017/03/02 09:50:40]

"*://[*.]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).

[meacer, 2017/03/07 20:50:38]

Thanks for pointing this out, I though has_domain_wildcard actually meant "*"
and not subdomain wildcard.
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.
(a) is already true, the API returns "Invalid Scheme" error for chrome-extension
patterns.
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.

[msramek, 2017/03/13 22:05:25]

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.
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.
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...

You'll want to add a parameter that excludes CUSTOM_EXTENSION_PROVIDER.
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.

[meacer, 2017/04/11 18:30:47]

I'm explicitly avoiding querying for installed extensions because restricting
this only for currently installed extensions doesn't fix the issue.
InstalledExtension will then be able to grant permissions to a large list of
extension IDs that are controlled by the same developer. Any time the user
installs one of those extensions, that new extension will now automatically be
granted the permission.

I don't think the breakage here is significant. The only time it will matter is
if the user has a 32 character local hostname that only contains letters a-p. If
there is a conflict they can simply change that hostname. For this particular
bug, not having any false negatives is more important than having false
positives.
It looks like this will require all callers of
HostContentSettingsMap::GetWebsiteSetting to determine the value of
|skip_extensions|. According to codesearch there is 215+ of them. Do we really
want all of those places to check if the caller is an extension URL? I'm asking
because I might be misunderstanding your comment.

[msramek, 2017/04/12 09:53:13]

Acknowledged. Can you please explain that reasoning in a comment in the code as
well?
That was indeed my first (naïve) suggestion, but I think in practice the value
of the parameter can always be derived inside GetWebsiteSettingInternal().

The goal is that an extension should not be able to determine a content setting
for an extension, right? A user can add a content setting for an extension, and
an extension can add a content setting for a random website. We only need to
focus on cases where there's an extension on both sides of the equation.

So if GetWebsiteSettingInternal() sees that the URL we're asking about is
chrome-extension:// AND the provider offering the rule is the extension
provider, we ignore it and continue iterating.

In practice, you also have to decide whether you care about only the primary url
or also the secondary; the semantics are usually  (requester URL, top-level
frame URL), but theoretically, people can read directly from
HostContentSettingsMap with their own semantics. It's probably safer to skip if
either of them is an extension URL.

In cases where the content setting only matches extensions, we can also modify
SetWebsiteSetting() and refuse to set it. But for settings that also match
something else, especially Wildcard(), we need to solve the problem on
GetWebsiteSetting() side.

+         (parts_.has_domain_wildcard ||
+          RE2::FullMatch(parts_.host, kExtensionIdRegex));
+}
+
 bool ContentSettingsPattern::Matches(
     const GURL& url) const {
   // An invalid pattern matches nothing.