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

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

This blocks extensions from allowing themselves or other extensions microphone and video permissions.

BUG=677714

[meacer, 2017-03-02 00:25:25]

The CQ bit was checked by meacer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-02 00:25:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[meacer, 2017-03-02 00:39:11]

The CQ bit was checked by meacer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-02 00:39:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[meacer, 2017-03-02 00:40:06]

Description was changed from

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

BUG=
==========

to

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

BUG=677714
==========

[meacer, 2017-03-02 00:40:25]

meacer@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[meacer, 2017-03-02 00:40:26]

rdevlin.cronin@: PTAL?

[meacer, 2017-03-02 00:41:29]

Description was changed from

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

BUG=677714
==========

to

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

This blocks extensions from allowing themselves or other extensions microphone
and video permissions.

BUG=677714
==========

[commit-bot: I haz the power, 2017-03-02 04:53:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-02 04:53:39]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[msramek, 2017-03-02 09:50:39]

msramek@chromium.org changed reviewers:
+ msramek@chromium.org

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

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

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

https://codereview.chromium.org/2730533002/diff/20001/components/content_sett...
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_sett...
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...

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

https://codereview.chromium.org/2730533002/diff/20001/components/content_sett...
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).

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

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

https://codereview.chromium.org/2730533002/diff/20001/chrome/browser/extensio...
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/extensio...
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_sett...
File components/content_settings/core/common/content_settings_pattern.cc
(right):

https://codereview.chromium.org/2730533002/diff/20001/components/content_sett...
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...

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

Sorry for the delay, PTAL?

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

https://codereview.chromium.org/2730533002/diff/20001/chrome/browser/extensio...
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/extensio...
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_sett...
File components/content_settings/core/common/content_settings_pattern.cc
(right):

https://codereview.chromium.org/2730533002/diff/20001/components/content_sett...
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...

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

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

Sorry for the delay, PTAL?

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

https://codereview.chromium.org/2730533002/diff/20001/chrome/browser/extensio...
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/extensio...
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_sett...
File components/content_settings/core/common/content_settings_pattern.cc
(right):

https://codereview.chromium.org/2730533002/diff/20001/components/content_sett...
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...

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

[meacer, 2017-03-13 21:28:36]

ping :)

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

Sorry! Perf time...

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

https://codereview.chromium.org/2730533002/diff/20001/components/content_sett...
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...

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.

[Devlin, 2017-03-16 19:10:33]

extensions lgtm

[punit.g7, 2017-04-11 17:10:34]

On 2017/03/16 19:10:33, Devlin wrote:
> extensions lgtm

Ping :)

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

Thanks for the ping. I've been working on the higher priority crbug.com/594215
so I somewhat lost context on this one. Sending comments I added earlier without
making any changes.

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

https://codereview.chromium.org/2730533002/diff/20001/components/content_sett...
components/content_settings/core/common/content_settings_pattern.cc:539: return
(parts_.is_scheme_wildcard || parts_.scheme == "chrome-extension") &&
On 2017/03/13 22:05:25, msramek wrote:
> 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.

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.

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

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.

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

Thanks for the ping. I've been working on the higher priority crbug.com/594215
so I somewhat lost context on this one. Sending comments I added earlier without
making any changes.

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

https://codereview.chromium.org/2730533002/diff/20001/components/content_sett...
components/content_settings/core/common/content_settings_pattern.cc:539: return
(parts_.is_scheme_wildcard || parts_.scheme == "chrome-extension") &&
On 2017/03/13 22:05:25, msramek wrote:
> 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.

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.

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

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]

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

https://codereview.chromium.org/2730533002/diff/20001/components/content_sett...
components/content_settings/core/common/content_settings_pattern.cc:539: return
(parts_.is_scheme_wildcard || parts_.scheme == "chrome-extension") &&
On 2017/04/11 18:30:47, Mustafa Emre Acer wrote:
> On 2017/03/13 22:05:25, msramek wrote:
> > 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.
> 
> 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.
> 

Acknowledged. Can you please explain that reasoning in a comment in the code as
well?

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

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.

[raymes, 2017-04-19 01:52:55]

Sorry, I haven't been following along with this CL, but had a question. I'm
wondering whether it's actually effective to restrict extensions here? Couldn't
an extension simply allow mic/camera on an attacker controlled non-extension
domain? Does this prevent anything that wouldn't otherwise be possible?

[meacer, 2017-04-19 19:32:08]

On 2017/04/19 01:52:55, raymes wrote:
> Sorry, I haven't been following along with this CL, but had a question. I'm
> wondering whether it's actually effective to restrict extensions here?
Couldn't
> an extension simply allow mic/camera on an attacker controlled non-extension
> domain? Does this prevent anything that wouldn't otherwise be possible?

raymes: Yes, it's possible. I mentioned this in https://crbug.com/677714#c6 but
not sure what the right fix is, short of blocking the combination of host +
contentSettings permissions.

[raymes, 2017-04-19 23:08:30]

On 2017/04/19 19:32:08, Mustafa Emre Acer wrote:
> On 2017/04/19 01:52:55, raymes wrote:
> > Sorry, I haven't been following along with this CL, but had a question. I'm
> > wondering whether it's actually effective to restrict extensions here?
> Couldn't
> > an extension simply allow mic/camera on an attacker controlled non-extension
> > domain? Does this prevent anything that wouldn't otherwise be possible?
> 
> raymes: Yes, it's possible. I mentioned this in https://crbug.com/677714#c6
but
> not sure what the right fix is, short of blocking the combination of host +
> contentSettings permissions.

Thanks meacer. I would just be afraid that we add extra complexity into the
system without getting any real protection. Do you think that's a valid concern
or do you think this actually buys something in terms of security?

I'm wondering if we can just deprecate this API instead? Let's see where that
thread goes.

[msramek, 2017-04-20 09:12:56]

With the content settings permission, extensions indeed can change content
settings on attacker-controlled sites. Why is that a problem, though?

With the right permission, extensions can, for example, read your typed
passwords. And that is intentional - otherwise we couldn't have password
management extensions.

Or see your network communication. Or read your history. Extension APIs are
fundamentally dangerous because we want extensions to be powerful.

My understanding was that the problem Mustafa is solving is not a security risk
associated with this API, but the fact that an extension changing content
settings for other extensions kinda breaks the extensions permission model.

[meacer, 2017-04-20 18:01:15]

On 2017/04/20 09:12:56, msramek wrote:
> With the content settings permission, extensions indeed can change content
> settings on attacker-controlled sites. Why is that a problem, though?
> 
> With the right permission, extensions can, for example, read your typed
> passwords. And that is intentional - otherwise we couldn't have password
> management extensions.

I think host permissions and camera/microphone are fundamentally different. An
extension can read your typed passwords only on the sites it has permission to.
Whereas there is only one camera stream and its contents don't change per
origin, so accessing the camera stream is a global capability. In a way it's
similar to filesystem access: You either have access to it or not, accessing it
from different origins (i.e. different chrome-extension URLs) doesn't give you
different results.

> 
> Or see your network communication. Or read your history. Extension APIs are
> fundamentally dangerous because we want extensions to be powerful.
> 
> My understanding was that the problem Mustafa is solving is not a security
risk
> associated with this API, but the fact that an extension changing content
> settings for other extensions kinda breaks the extensions permission model.

That's accurate for this CL, but extensions granting themselves global
capabilities also breaks the permission model, so I don't think we can ignore
it. 

IMO the best mitigation is to make sure global capabilities have global
indicators. E.g. if the camera is being used, there should be a system modal UI
bubble for it, similar to what ChromeOS does for screen sharing (see the bubble
with "Stop Sharing" button at the bottom of the screen):
https://flashphoner.com/docs/wcs5/wcs_docs/html/en/wcs-user-guide/demo_screen...

That doesn't prevent the problem described here, but makes it obvious to the
user that their camera/microphone is being used by some app and gives them the
ability to stop. (as a side effect it'd also fix crbug.com/453150)

[raymes, 2017-04-20 22:46:02]

> > My understanding was that the problem Mustafa is solving is not a security
> risk
> > associated with this API, but the fact that an extension changing content
> > settings for other extensions kinda breaks the extensions permission model.

I think it's just hard for me to see the benefit of preventing extensions from
granting camera/mic to other extensions, but still allowing them to grant
camera/mic to other websites. What benefit does it actually provide? I'm just a
bit worried that we're adding complexity that doesn't get us anything concrete.

I agree that it's weird and the permission model feels broken but I feel like
that might be a bigger problem that we might want to think about?

[palmer, 2017-05-01 21:36:49]

As described in the bug, this CL is not a complete fix, but it does seem
necessary as a minimum precondition for fixing the bug. Hence it's a net good?
raymes, I added you to the bug.

[raymes, 2017-05-02 00:01:16]

On 2017/05/01 21:36:49, palmer wrote:
> As described in the bug, this CL is not a complete fix, but it does seem
> necessary as a minimum precondition for fixing the bug. Hence it's a net good?
> raymes, I added you to the bug.

I guess I just don't really see this as preventing a malicious extension from
doing anything that it couldn't otherwise do. We've been having a discussion on
email about how to move forward but I'll update the bug too.