Add a centralized mechanism for whitelisting access to extension permissions.

Add a centralized mechanism for whitelisting access to extension permissions.

This also updates the following permissions to use the whitelist:
 - terminalPrivate
 - webSocketProxyPrivate
 - chromePrivate
 - inputMethodPrivate
 - chromeAuthPrivate
 - webstorePrivate

Includes more tests to verify that Extension loading fails for different permission parameters:
 - by extension type
 - component only flag
 - whitelists

BUG=84211, 111314
TEST=extension unit and browser tests


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=121111

[jstritar, 2012-02-01 16:09:51]

@tbarzic: Could you take a look at the terminalPrivate changes?

http://codereview.chromium.org/9317013/diff/5005/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/terminal/terminal_extension_helper.cc (left):

http://codereview.chromium.org/9317013/diff/5005/chrome/browser/extensions/ap...
chrome/browser/extensions/api/terminal/terminal_extension_helper.cc:43: GURL
TerminalExtensionHelper::GetCroshExtensionURL(Profile* profile) {
What does Crosh stand for? Chrome shell? Just wondering what the capitalization
should be.

http://codereview.chromium.org/9317013/diff/5005/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/terminal/terminal_extension_helper.cc
(right):

http://codereview.chromium.org/9317013/diff/5005/chrome/browser/extensions/ap...
chrome/browser/extensions/api/terminal/terminal_extension_helper.cc:20:
service->GetExtensionById(extension_misc::kHTermAppId, false);
Do you ever really need to fall back to the test extension here? I didn't see
any tests that require this, so just updated it to use the HTerm app ID.

[jstritar, 2012-02-01 16:10:22]

+tbarzic: see the previous email, forgot to add you.

[tbarzic, 2012-02-01 18:32:43]

http://codereview.chromium.org/9317013/diff/5005/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/terminal/terminal_extension_helper.cc (left):

http://codereview.chromium.org/9317013/diff/5005/chrome/browser/extensions/ap...
chrome/browser/extensions/api/terminal/terminal_extension_helper.cc:43: GURL
TerminalExtensionHelper::GetCroshExtensionURL(Profile* profile) {
On 2012/02/01 16:09:51, jstritar wrote:
> What does Crosh stand for? Chrome shell? Just wondering what the
capitalization
> should be.

It's chrome os shell

http://codereview.chromium.org/9317013/diff/5005/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/terminal/terminal_extension_helper.cc
(right):

http://codereview.chromium.org/9317013/diff/5005/chrome/browser/extensions/ap...
chrome/browser/extensions/api/terminal/terminal_extension_helper.cc:20:
service->GetExtensionById(extension_misc::kHTermAppId, false);
On 2012/02/01 16:09:51, jstritar wrote:
> Do you ever really need to fall back to the test extension here? I didn't see
> any tests that require this, so just updated it to use the HTerm app ID.

Yeah,we'll need to fallback occasionally; test extension is extension we use to
develop and test hterm extension before it is uploaded to the gallery

[tbarzic, 2012-02-02 00:36:07]

http://codereview.chromium.org/9317013/diff/5005/chrome/common/extensions/ext...
File chrome/common/extensions/extension_permission_set.cc (right):

http://codereview.chromium.org/9317013/diff/5005/chrome/common/extensions/ext...
chrome/common/extensions/extension_permission_set.cc:84:
"okddffdblfhhnmhodogpojmfkjmhinfp",
okddffdblfhhnmhodogpojmfkjmhinfp should also be added to terminalPrivate
whitelist (that's hterm "development" extension)

[Aaron Boodman, 2012-02-02 08:09:24]

Hooray for all the deleted test code.

http://codereview.chromium.org/9317013/diff/6002/chrome/common/extensions/ext...
File chrome/common/extensions/extension_permission_set.h (right):

http://codereview.chromium.org/9317013/diff/6002/chrome/common/extensions/ext...
chrome/common/extensions/extension_permission_set.h:159: kFlagEnforceWhitelist =
1 << 4,
Using 'and' here doesn't seem very useful. Are there really cases where we want
to restrict to a whitelist of specific component extensions? If we wanted that,
why not just use the whitelist only? Since they are component extensions, we
know their IDs ahead of time, and the design of the extension system makes it
impossible (for practical definitions) to spoof an ID.

More broadly, why have this flag at all? Why not just: AddToWhitelist(),
IsWhitelisted(), and HasWhitelist()?

The RegisterPermission() call could be changed to return the APIPermission
instance, and then the caller could call AddToWhitelist() as needed.

This seems like it would simplify the implementation and usage of this
functionality. You could also get rid of the RegisterWhitelist() methods that
way.

http://codereview.chromium.org/9317013/diff/6002/chrome/common/extensions/ext...
chrome/common/extensions/extension_permission_set.h:223: // method if
should_enforce_whitelist() is true.
Why is the second sentence important to note?

http://codereview.chromium.org/9317013/diff/6002/chrome/common/extensions/ext...
chrome/common/extensions/extension_permission_set.h:270: static void
RegisterWhitelists(ExtensionPermissionsInfo* info);
Not clear what this method does from just its name and comments.

[jstritar, 2012-02-02 17:51:09]

Anyone know a good reviewer for the web_socket_proxy changes? I think the author
left Google.

http://codereview.chromium.org/9317013/diff/6002/chrome/common/extensions/ext...
File chrome/common/extensions/extension_permission_set.h (right):

http://codereview.chromium.org/9317013/diff/6002/chrome/common/extensions/ext...
chrome/common/extensions/extension_permission_set.h:159: kFlagEnforceWhitelist =
1 << 4,
On 2012/02/02 08:09:24, Aaron Boodman wrote:
> Using 'and' here doesn't seem very useful. Are there really cases where we
want
> to restrict to a whitelist of specific component extensions? If we wanted
that,
> why not just use the whitelist only? Since they are component extensions, we
> know their IDs ahead of time, and the design of the extension system makes it
> impossible (for practical definitions) to spoof an ID.
> 
> More broadly, why have this flag at all? Why not just: AddToWhitelist(),
> IsWhitelisted(), and HasWhitelist()?
> 
> The RegisterPermission() call could be changed to return the APIPermission
> instance, and then the caller could call AddToWhitelist() as needed.
> 
> This seems like it would simplify the implementation and usage of this
> functionality. You could also get rid of the RegisterWhitelist() methods that
> way.

Done and deprecated kFlagComponentOnly.

http://codereview.chromium.org/9317013/diff/6002/chrome/common/extensions/ext...
chrome/common/extensions/extension_permission_set.h:223: // method if
should_enforce_whitelist() is true.
On 2012/02/02 08:09:24, Aaron Boodman wrote:
> Why is the second sentence important to note?

Updated comment.

http://codereview.chromium.org/9317013/diff/6002/chrome/common/extensions/ext...
chrome/common/extensions/extension_permission_set.h:270: static void
RegisterWhitelists(ExtensionPermissionsInfo* info);
On 2012/02/02 08:09:24, Aaron Boodman wrote:
> Not clear what this method does from just its name and comments.

Removed method.

http://codereview.chromium.org/9317013/diff/5005/chrome/common/extensions/ext...
File chrome/common/extensions/extension_permission_set.cc (right):

http://codereview.chromium.org/9317013/diff/5005/chrome/common/extensions/ext...
chrome/common/extensions/extension_permission_set.cc:84:
"okddffdblfhhnmhodogpojmfkjmhinfp",
On 2012/02/02 00:36:07, tbarzic wrote:
> okddffdblfhhnmhodogpojmfkjmhinfp should also be added to terminalPrivate
> whitelist (that's hterm "development" extension)

Done. Also added made sure that TerminalExtensionHelper::GetCroshExtensionURL
falls back to that again.

[Aaron Boodman, 2012-02-02 18:17:31]

http://codereview.chromium.org/9317013/diff/42/chrome/common/extensions/exten...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/9317013/diff/42/chrome/common/extensions/exten...
chrome/common/extensions/extension.cc:2838: if ((permission->is_component_only()
&& location_ != COMPONENT) ||
I think this logic should be:

if (permission->is_component_only() && location_ == COMPONENT)
  return true;

if (permission->HasWhitelist() && permission->IsWhitelisted(id_))
 return true;

(and then get rid of the lines at 2852 and 2853.

http://codereview.chromium.org/9317013/diff/42/chrome/common/extensions/exten...
File chrome/common/extensions/extension_permission_set.cc (right):

http://codereview.chromium.org/9317013/diff/42/chrome/common/extensions/exten...
chrome/common/extensions/extension_permission_set.cc:10: #include
"base/command_line.h"
Not needed.

http://codereview.chromium.org/9317013/diff/42/chrome/common/extensions/exten...
chrome/common/extensions/extension_permission_set.cc:15: #include
"chrome/common/chrome_switches.h"
Not needed?

http://codereview.chromium.org/9317013/diff/42/chrome/common/extensions/exten...
chrome/common/extensions/extension_permission_set.cc:71: // TODO(jstritar): We
should find out what these extensions are.
This is a pretty ominous comment :). Can you track down the people who added
these to the codebase (git log -S<id> will do it) and mail them and ask them to
add comments?

http://codereview.chromium.org/9317013/diff/42/chrome/common/extensions/exten...
chrome/common/extensions/extension_permission_set.cc:348: for (size_t i = 0; i <
arraysize(kTerminalPrivateWhitelist); ++i)
Nit: instead of duplicating this loop everywhere, you can just get rid of the
constant array and call AddToWhitelist() for each entry. You're not saving
anything abstracting it to an array.

http://codereview.chromium.org/9317013/diff/42/chrome/common/extensions/exten...
chrome/common/extensions/extension_permission_set.cc:350:
ExtensionAPIPermission* webSocketProxyPrivate = info->RegisterPermission(
Maybe also move the ones that have whitelists to the end of this section and put
line breaks between them for readability.

http://codereview.chromium.org/9317013/diff/42/chrome/common/extensions/exten...
File chrome/common/extensions/extension_permission_set.h (right):

http://codereview.chromium.org/9317013/diff/42/chrome/common/extensions/exten...
chrome/common/extensions/extension_permission_set.h:153:
kFlagComponentOnly_Deprecated = 1 << 2,
Sorry another nit here:

I think it is fine to have the component-only bit in addition to the whitelist.
It is more flexible than a whitelist. I think we really only need the whitelist
in cases where the extension we want to restrict to is _not_ a component
extension.

[jstritar, 2012-02-02 20:18:04]

Stop reviewing CLs.... you're OOO today!

I think we're viewing the whitelists differently:
- Me: "if a whitelist is present, the extension must be on it"
- You: "if an extension is on the whitelist, they can always access the
permission"
 
The ad-hoc whitelists that are already present are more in line with what I was
doing. In the other scenario, how would we restrict access to only extensions on
the whitelist? Even if we use the kTypeNone restriction, component extensions
would be able to specify the permission since they bypass type restrictions. I
think there should be a way of preventing component extensions from accessing
these permissions too.

[jstritar, 2012-02-03 14:33:17]

http://codereview.chromium.org/9317013/diff/42/chrome/common/extensions/exten...
File chrome/common/extensions/extension_permission_set.cc (right):

http://codereview.chromium.org/9317013/diff/42/chrome/common/extensions/exten...
chrome/common/extensions/extension_permission_set.cc:10: #include
"base/command_line.h"
Woops, I do still need this. I added the command line check back to
IsWhitelisted.

http://codereview.chromium.org/9317013/diff/42/chrome/common/extensions/exten...
chrome/common/extensions/extension_permission_set.cc:15: #include
"chrome/common/chrome_switches.h"
On 2012/02/02 18:17:31, Aaron Boodman wrote:
> Not needed?

ditto

http://codereview.chromium.org/9317013/diff/42/chrome/common/extensions/exten...
chrome/common/extensions/extension_permission_set.cc:348: for (size_t i = 0; i <
arraysize(kTerminalPrivateWhitelist); ++i)
On 2012/02/02 18:17:31, Aaron Boodman wrote:
> Nit: instead of duplicating this loop everywhere, you can just get rid of the
> constant array and call AddToWhitelist() for each entry. You're not saving
> anything abstracting it to an array.

Done.

http://codereview.chromium.org/9317013/diff/42/chrome/common/extensions/exten...
chrome/common/extensions/extension_permission_set.cc:350:
ExtensionAPIPermission* webSocketProxyPrivate = info->RegisterPermission(
On 2012/02/02 18:17:31, Aaron Boodman wrote:
> Maybe also move the ones that have whitelists to the end of this section and
put
> line breaks between them for readability.

Done.

http://codereview.chromium.org/9317013/diff/42/chrome/common/extensions/exten...
File chrome/common/extensions/extension_permission_set.h (right):

http://codereview.chromium.org/9317013/diff/42/chrome/common/extensions/exten...
chrome/common/extensions/extension_permission_set.h:153:
kFlagComponentOnly_Deprecated = 1 << 2,
On 2012/02/02 18:17:31, Aaron Boodman wrote:
> Sorry another nit here:
> 
> I think it is fine to have the component-only bit in addition to the
whitelist.
> It is more flexible than a whitelist. I think we really only need the
whitelist
> in cases where the extension we want to restrict to is _not_ a component
> extension.

Done.

[jstritar, 2012-02-03 14:33:17]

http://codereview.chromium.org/9317013/diff/42/chrome/common/extensions/exten...
File chrome/common/extensions/extension_permission_set.cc (right):

http://codereview.chromium.org/9317013/diff/42/chrome/common/extensions/exten...
chrome/common/extensions/extension_permission_set.cc:10: #include
"base/command_line.h"
Woops, I do still need this. I added the command line check back to
IsWhitelisted.

http://codereview.chromium.org/9317013/diff/42/chrome/common/extensions/exten...
chrome/common/extensions/extension_permission_set.cc:15: #include
"chrome/common/chrome_switches.h"
On 2012/02/02 18:17:31, Aaron Boodman wrote:
> Not needed?

ditto

http://codereview.chromium.org/9317013/diff/42/chrome/common/extensions/exten...
chrome/common/extensions/extension_permission_set.cc:348: for (size_t i = 0; i <
arraysize(kTerminalPrivateWhitelist); ++i)
On 2012/02/02 18:17:31, Aaron Boodman wrote:
> Nit: instead of duplicating this loop everywhere, you can just get rid of the
> constant array and call AddToWhitelist() for each entry. You're not saving
> anything abstracting it to an array.

Done.

http://codereview.chromium.org/9317013/diff/42/chrome/common/extensions/exten...
chrome/common/extensions/extension_permission_set.cc:350:
ExtensionAPIPermission* webSocketProxyPrivate = info->RegisterPermission(
On 2012/02/02 18:17:31, Aaron Boodman wrote:
> Maybe also move the ones that have whitelists to the end of this section and
put
> line breaks between them for readability.

Done.

http://codereview.chromium.org/9317013/diff/42/chrome/common/extensions/exten...
File chrome/common/extensions/extension_permission_set.h (right):

http://codereview.chromium.org/9317013/diff/42/chrome/common/extensions/exten...
chrome/common/extensions/extension_permission_set.h:153:
kFlagComponentOnly_Deprecated = 1 << 2,
On 2012/02/02 18:17:31, Aaron Boodman wrote:
> Sorry another nit here:
> 
> I think it is fine to have the component-only bit in addition to the
whitelist.
> It is more flexible than a whitelist. I think we really only need the
whitelist
> in cases where the extension we want to restrict to is _not_ a component
> extension.

Done.

[jstritar, 2012-02-03 14:50:18]

@vandebo: could you take a look at the web_socket_proxy parts (and any other
chromeos if you're up for it)? the original author doesn't work on chrome
anymore and you reviewed some of the old CLs in that area. also, see the
question below

http://codereview.chromium.org/9317013/diff/7096/chrome/common/extensions/ext...
File chrome/common/extensions/extension_permission_set.cc (right):

http://codereview.chromium.org/9317013/diff/7096/chrome/common/extensions/ext...
chrome/common/extensions/extension_permission_set.cc:305:
chromePrivate->AddToWhitelist("fjcibdnjlbfnbfdjneajpipnlcppleek");
Do you know what extensions these are? Or who might know?

[jstritar, 2012-02-03 15:49:00]

@aa: can you take another look? CanSpecifyAPIPermission is slightly different
from our IM conversation, in that it also returns early if HasWhitelist and
IsWhitelisted, so the whitelist bypasses type restrictions.

[Aaron Boodman, 2012-02-03 17:29:36]

lgtm

[vandebo (ex-Chrome), 2012-02-03 17:35:32]

I'm not a good reviewer for this.  I can't find the old CLs you refer to, but I
was probably a reviewer for the network parts of those changes, and not the
chromeos/extension parts.

On 2012/02/03 14:50:18, jstritar wrote:
> @vandebo: could you take a look at the web_socket_proxy parts (and any other
> chromeos if you're up for it)? the original author doesn't work on chrome
> anymore and you reviewed some of the old CLs in that area. also, see the
> question below
> 
>
http://codereview.chromium.org/9317013/diff/7096/chrome/common/extensions/ext...
> File chrome/common/extensions/extension_permission_set.cc (right):
> 
>
http://codereview.chromium.org/9317013/diff/7096/chrome/common/extensions/ext...
> chrome/common/extensions/extension_permission_set.cc:305:
> chromePrivate->AddToWhitelist("fjcibdnjlbfnbfdjneajpipnlcppleek");
> Do you know what extensions these are? Or who might know?

[tbarzic, 2012-02-03 17:39:24]

On 2012/02/03 17:35:32, vandebo wrote:
> I'm not a good reviewer for this.  I can't find the old CLs you refer to, but
I
> was probably a reviewer for the network parts of those changes, and not the
> chromeos/extension parts.
> 
> On 2012/02/03 14:50:18, jstritar wrote:
> > @vandebo: could you take a look at the web_socket_proxy parts (and any other
> > chromeos if you're up for it)? the original author doesn't work on chrome
> > anymore and you reviewed some of the old CLs in that area. also, see the
> > question below
> > 
> >
>
http://codereview.chromium.org/9317013/diff/7096/chrome/common/extensions/ext...
> > File chrome/common/extensions/extension_permission_set.cc (right):
> > 
> >
>
http://codereview.chromium.org/9317013/diff/7096/chrome/common/extensions/ext...
> > chrome/common/extensions/extension_permission_set.cc:305:
> > chromePrivate->AddToWhitelist("fjcibdnjlbfnbfdjneajpipnlcppleek");
> > Do you know what extensions these are? Or who might know?

terminalPrivate..  LGTM

[jstritar, 2012-02-03 17:56:08]

One more try...
@zel: are you a good person to review the
chrome/browser/chromeos/web_socket_proxy changes?

[zelidrag1, 2012-02-03 18:01:56]

dpolukhin@ and ygorshenin@ are a better choices for this review.



On Fri, Feb 3, 2012 at 9:56 AM, <jstritar@chromium.org> wrote:

> One more try...
> @zel: are you a good person to review the
> chrome/browser/chromeos/web_**socket_proxy changes?
>
>
http://codereview.chromium.**org/9317013/<http://codereview.chromium.org/9317...
>

[Dmitry Polukhin, 2012-02-06 08:04:50]

LGTM for webSocketProxyPrivate changes

But it looks like for TCP/UDP sockets we should also create special extension
permission (see check in kPredefinedAllowedSocketOrigins in
chrome/renderer/chrome_content_renderer_client.cc). Jon, will you do it yourself
or you would like us to do it and send to you for review?

[jstritar, 2012-02-08 16:16:06]

Thanks for pointing out the TCP/UDP socket white list. It looks like that's used
by pepper APIs? I'm not familiar with how permissions work in pepper plugins. We
do have a 'socket' permission for the TCP/UDP sockets in extensions, but I don't
think the pepper plugins use that. 

Dmitry, do you know which extensions these are below?

http://codereview.chromium.org/9317013/diff/6049/chrome/common/extensions/ext...
File chrome/common/extensions/extension_permission_set.cc (right):

http://codereview.chromium.org/9317013/diff/6049/chrome/common/extensions/ext...
chrome/common/extensions/extension_permission_set.cc:327:
webSocketProxyPrivate->AddToWhitelist("gnedhmakppccajfpfiihfcdlnpgomkcf");
do you know what extension this is?

http://codereview.chromium.org/9317013/diff/6049/chrome/common/extensions/ext...
chrome/common/extensions/extension_permission_set.cc:328:
webSocketProxyPrivate->AddToWhitelist("fjcibdnjlbfnbfdjneajpipnlcppleek");
ditto^

[Dmitry Polukhin, 2012-02-08 17:40:29]

I'll take care about TCP/UDP because I'm moving check from render to browser
right now. When I finish I'll create new permission and send this CL to you for
review.

http://codereview.chromium.org/9317013/diff/6049/chrome/common/extensions/ext...
File chrome/common/extensions/extension_permission_set.cc (right):

http://codereview.chromium.org/9317013/diff/6049/chrome/common/extensions/ext...
chrome/common/extensions/extension_permission_set.cc:328:
webSocketProxyPrivate->AddToWhitelist("fjcibdnjlbfnbfdjneajpipnlcppleek");
On 2012/02/08 16:16:07, jstritar wrote:
> ditto^

All these 3 IDs are Citrix Receiver (stable, beta and dev).

[commit-bot: I haz the power, 2012-02-08 21:33:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jstritar@chromium.org/9317013/14001

[commit-bot: I haz the power, 2012-02-09 00:07:41]

Change committed as 121111