Adding policy support to Chrome Frame's launcher so that additional parameters can be passed to Chr…

Adding policy support to Chrome Frame's launcher so that additional parameters can be passed to Chrome via Group Policy.

BUG=116783
TEST=See information in comment #5 of the issue.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=129384

[grt (UTC plus 2), 2012-03-23 15:49:12]

Hi Tommi!

The launcher isn't used on XP, so you'll need code in
AutomationProxyCacheEntry::CreateProxy to add the policy args to the command
line there.

Also, this seems to bypass the whole sanitization in the launcher (args are read
from HKCU).  Is this intended?

https://chromiumcodereview.appspot.com/9836037/diff/1/chrome/app/policy/polic...
File chrome/app/policy/policy_templates.json (right):

https://chromiumcodereview.appspot.com/9836037/diff/1/chrome/app/policy/polic...
chrome/app/policy/policy_templates.json:1805: 'caption': '''Additional command
line parameters for <ph name="PRODUCT_FRAME_NAME">$3<ex>Google Chrome
Frame</ex></ph>''',
Nit: the caption makes it sound like the arguments are for GCF rather than
Chrome.

https://chromiumcodereview.appspot.com/9836037/diff/1/chrome_frame/chrome_fra...
File chrome_frame/chrome_frame_automation.cc (right):

https://chromiumcodereview.appspot.com/9836037/diff/1/chrome_frame/chrome_fra...
chrome_frame/chrome_frame_automation.cc:522: DLOG_IF(ERROR, it ==
proxies_.container().end())
nit: make this DLOG(ERROR) and move it into an else clause below the call to
erase()

https://chromiumcodereview.appspot.com/9836037/diff/1/chrome_frame/chrome_fra...
File chrome_frame/chrome_frame_launcher.gyp (right):

https://chromiumcodereview.appspot.com/9836037/diff/1/chrome_frame/chrome_fra...
chrome_frame/chrome_frame_launcher.gyp:1: # Copyright (c) 2011 The Chromium
Authors. All rights reserved.
2012

[slightlyoff1, 2012-03-23 15:57:05]

grt: yeah, we should be skipping the validation here. The whole point of it is
to prevent sending params we don't want to be configured by a normal user. This
key has the behavior of being admin-only and therefore supporting anything (and
everything) they like.

LGTM pending grt's nits.

https://chromiumcodereview.appspot.com/9836037/diff/1/chrome/app/policy/polic...
File chrome/app/policy/policy_templates.json (right):

https://chromiumcodereview.appspot.com/9836037/diff/1/chrome/app/policy/polic...
chrome/app/policy/policy_templates.json:1805: 'caption': '''Additional command
line parameters for <ph name="PRODUCT_FRAME_NAME">$3<ex>Google Chrome
Frame</ex></ph>''',
Further nit: it would be good to call out there that this is for parameter which
aren't normally settable by policy and can only be provided by the administrator
of the system.

On 2012/03/23 15:49:12, grt wrote:
> Nit: the caption makes it sound like the arguments are for GCF rather than
> Chrome.

[grt (UTC plus 2), 2012-03-23 16:44:17]

On 2012/03/23 15:57:05, slightlyoff1 wrote:
> grt: yeah, we should be skipping the validation here. The whole point of it is
> to prevent sending params we don't want to be configured by a normal user.
This
> key has the behavior of being admin-only and therefore supporting anything
(and
> everything) they like.

The policy settings are read out of HKLM and HKCU, so they aren't admin-only.

[robertshield, 2012-03-23 17:04:22]

On 2012/03/23 16:44:17, grt wrote:
> On 2012/03/23 15:57:05, slightlyoff1 wrote:
> > grt: yeah, we should be skipping the validation here. The whole point of it
is
> > to prevent sending params we don't want to be configured by a normal user.
> This
> > key has the behavior of being admin-only and therefore supporting anything
> (and
> > everything) they like.
> 
> The policy settings are read out of HKLM and HKCU, so they aren't admin-only.

Greg's right, this is a dev feature along the same lines as AllowUnsafeURLs et
al. We shouldn't position this as an "only for admins" thing. 

The security story is probably still fine since chrome_launcher is the low ->
medium integrity escape hatch and afaik, low integrity generally can't write to
HKCU as most keys there have the default (medium) integrity level.

The main comment is that, as Greg mentioned, this won't work on XP. Feels like
the simplest thing is to have an XP-only codepath in CF to add the extra
command-line string as well.

[slightlyoff1, 2012-03-23 17:06:48]

On 2012/03/23 17:04:22, robertshield wrote:
> On 2012/03/23 16:44:17, grt wrote:
> > On 2012/03/23 15:57:05, slightlyoff1 wrote:
> > > grt: yeah, we should be skipping the validation here. The whole point of
it
> is
> > > to prevent sending params we don't want to be configured by a normal user.
> > This
> > > key has the behavior of being admin-only and therefore supporting anything
> > (and
> > > everything) they like.
> > 
> > The policy settings are read out of HKLM and HKCU, so they aren't
admin-only.
> 
> Greg's right, this is a dev feature along the same lines as AllowUnsafeURLs et
> al. We shouldn't position this as an "only for admins" thing. 

I disagree with this. ISTM that we don't want GCF to be trivially attackable by
anyone who can set a registry key on the system. Or are we already? If you can
convince me that we're already vulnerable to users of this type being able to
send --disable-sandbox or similar, fine. But for my money, I'd like this to be
admin only.

> The security story is probably still fine since chrome_launcher is the low ->
> medium integrity escape hatch and afaik, low integrity generally can't write
to
> HKCU as most keys there have the default (medium) integrity level.
> 
> The main comment is that, as Greg mentioned, this won't work on XP. Feels like
> the simplest thing is to have an XP-only codepath in CF to add the extra
> command-line string as well.

[tommi (sloooow) - chröme, 2012-03-24 09:25:01]

Hey guys,

So, just to confirm, I'm purposely adding the arguments to the command line
after sanitization and I figured that we wanted this for both admin and devs, so
both HKLM and HKCU are supported.  The reason I'm not sanitizing the arguments
is basically that if users can write to HKCU, they can also launch chrome with
whatever arguments they like from the command line.

Makes sense?

Thanks for the heads up on XP.  I'll fix that.

[tommi (sloooow) - chröme, 2012-03-26 10:12:35]

XP support added.

gfeher - can you take a look at the changes in chrome/app/policy please?

[gfeher, 2012-03-26 10:27:54]

Looks okay, but please check with Mattias. (I am not working on policies any
more.)

[Mattias Nissler (ping if slow), 2012-03-26 11:04:40]

LGTM with nits.

https://chromiumcodereview.appspot.com/9836037/diff/8001/chrome/app/policy/po...
File chrome/app/policy/policy_templates.json (right):

https://chromiumcodereview.appspot.com/9836037/diff/8001/chrome/app/policy/po...
chrome/app/policy/policy_templates.json:1806: 'desc': '''Allows you to specify
additional parameters that are used when <ph
name="PRODUCT_FRAME_NAME">$3<ex>Google Chrome Frame</ex></ph> launches <ph
name="PRODUCT_NAME">$1<ex>Google Chrome</ex></ph>.''',
Please add the additional pro-forma blurb on what happens if the policy is not
specified.

https://chromiumcodereview.appspot.com/9836037/diff/8001/chrome_frame/chrome_...
File chrome_frame/chrome_launcher.cc (right):

https://chromiumcodereview.appspot.com/9836037/diff/8001/chrome_frame/chrome_...
chrome_frame/chrome_launcher.cc:167: result = ::RegOpenKeyExW(kRootKeys[i],
policy::kRegistryMandatorySubKey, 0,
Why are you not using base/win/registry.h? Is there some no-dependencies rule
for this module?

https://chromiumcodereview.appspot.com/9836037/diff/8001/chrome_frame/policy_...
File chrome_frame/policy_settings.cc (right):

https://chromiumcodereview.appspot.com/9836037/diff/8001/chrome_frame/policy_...
chrome_frame/policy_settings.cc:125: const char* value_name, std::wstring*
value) {
nit: each parameter on separate line, non need for the break after (

https://chromiumcodereview.appspot.com/9836037/diff/8001/chrome_frame/test/po...
File chrome_frame/test/policy_settings_unittest.cc (right):

https://chromiumcodereview.appspot.com/9836037/diff/8001/chrome_frame/test/po...
chrome_frame/test/policy_settings_unittest.cc:92: bool SetCFPolicyString(HKEY
policy_root, const char* policy_name,
nit: declare each parameter on separate line

[tommi (sloooow) - chröme, 2012-03-26 11:24:37]

https://chromiumcodereview.appspot.com/9836037/diff/1/chrome/app/policy/polic...
File chrome/app/policy/policy_templates.json (right):

https://chromiumcodereview.appspot.com/9836037/diff/1/chrome/app/policy/polic...
chrome/app/policy/policy_templates.json:1805: 'caption': '''Additional command
line parameters for <ph name="PRODUCT_FRAME_NAME">$3<ex>Google Chrome
Frame</ex></ph>''',
On 2012/03/23 15:49:12, grt wrote:
> Nit: the caption makes it sound like the arguments are for GCF rather than
> Chrome.

Changed to Chrome proper.

https://chromiumcodereview.appspot.com/9836037/diff/1/chrome_frame/chrome_fra...
File chrome_frame/chrome_frame_automation.cc (right):

https://chromiumcodereview.appspot.com/9836037/diff/1/chrome_frame/chrome_fra...
chrome_frame/chrome_frame_automation.cc:522: DLOG_IF(ERROR, it ==
proxies_.container().end())
On 2012/03/23 15:49:12, grt wrote:
> nit: make this DLOG(ERROR) and move it into an else clause below the call to
> erase()

Done.

https://chromiumcodereview.appspot.com/9836037/diff/1/chrome_frame/chrome_fra...
File chrome_frame/chrome_frame_launcher.gyp (right):

https://chromiumcodereview.appspot.com/9836037/diff/1/chrome_frame/chrome_fra...
chrome_frame/chrome_frame_launcher.gyp:1: # Copyright (c) 2011 The Chromium
Authors. All rights reserved.
On 2012/03/23 15:49:12, grt wrote:
> 2012

Done.

https://chromiumcodereview.appspot.com/9836037/diff/8001/chrome/app/policy/po...
File chrome/app/policy/policy_templates.json (right):

https://chromiumcodereview.appspot.com/9836037/diff/8001/chrome/app/policy/po...
chrome/app/policy/policy_templates.json:1806: 'desc': '''Allows you to specify
additional parameters that are used when <ph
name="PRODUCT_FRAME_NAME">$3<ex>Google Chrome Frame</ex></ph> launches <ph
name="PRODUCT_NAME">$1<ex>Google Chrome</ex></ph>.''',
On 2012/03/26 11:04:41, Mattias Nissler wrote:
> Please add the additional pro-forma blurb on what happens if the policy is not
> specified.

Done.

https://chromiumcodereview.appspot.com/9836037/diff/8001/chrome_frame/chrome_...
File chrome_frame/chrome_launcher.cc (right):

https://chromiumcodereview.appspot.com/9836037/diff/8001/chrome_frame/chrome_...
chrome_frame/chrome_launcher.cc:167: result = ::RegOpenKeyExW(kRootKeys[i],
policy::kRegistryMandatorySubKey, 0,
On 2012/03/26 11:04:41, Mattias Nissler wrote:
> Why are you not using base/win/registry.h? Is there some no-dependencies rule
> for this module?

Yes, the chrome launcher doesn't have the normal Chrome dependencies.  E.g.
you'll notice the absence of the CommandLine class when dealing with command
line and use of wstring.

https://chromiumcodereview.appspot.com/9836037/diff/8001/chrome_frame/policy_...
File chrome_frame/policy_settings.cc (right):

https://chromiumcodereview.appspot.com/9836037/diff/8001/chrome_frame/policy_...
chrome_frame/policy_settings.cc:125: const char* value_name, std::wstring*
value) {
On 2012/03/26 11:04:41, Mattias Nissler wrote:
> nit: each parameter on separate line, non need for the break after (

Done.

https://chromiumcodereview.appspot.com/9836037/diff/8001/chrome_frame/test/po...
File chrome_frame/test/policy_settings_unittest.cc (right):

https://chromiumcodereview.appspot.com/9836037/diff/8001/chrome_frame/test/po...
chrome_frame/test/policy_settings_unittest.cc:92: bool SetCFPolicyString(HKEY
policy_root, const char* policy_name,
On 2012/03/26 11:04:41, Mattias Nissler wrote:
> nit: declare each parameter on separate line

this is the convention in this file so if you don't mind I'd like to keep it
this way.

[Mattias Nissler (ping if slow), 2012-03-26 11:40:55]

https://chromiumcodereview.appspot.com/9836037/diff/8001/chrome_frame/test/po...
File chrome_frame/test/policy_settings_unittest.cc (right):

https://chromiumcodereview.appspot.com/9836037/diff/8001/chrome_frame/test/po...
chrome_frame/test/policy_settings_unittest.cc:92: bool SetCFPolicyString(HKEY
policy_root, const char* policy_name,
On 2012/03/26 11:24:37, tommi wrote:
> On 2012/03/26 11:04:41, Mattias Nissler wrote:
> > nit: declare each parameter on separate line
> 
> this is the convention in this file so if you don't mind I'd like to keep it
> this way.

I'd rather encourage you to fix the entire file, but this is CF land in which I
don't have much experience, so I don't really care.

[tommi (sloooow) - chröme, 2012-03-26 11:44:40]

On 2012/03/26 11:40:55, Mattias Nissler wrote:
> I'd rather encourage you to fix the entire file, but this is CF land in which
I
> don't have much experience, so I don't really care.

Well, CF is just another part of Chrome so if there's something in our style
guide that says we should have all arguments on separate lines, I don't have a
problem with fixing the entire file.

[tommi (sloooow) - chröme, 2012-03-26 11:48:18]

https://chromiumcodereview.appspot.com/9836037/diff/8001/chrome_frame/test/po...
File chrome_frame/test/policy_settings_unittest.cc (right):

https://chromiumcodereview.appspot.com/9836037/diff/8001/chrome_frame/test/po...
chrome_frame/test/policy_settings_unittest.cc:92: bool SetCFPolicyString(HKEY
policy_root, const char* policy_name,
On 2012/03/26 11:40:55, Mattias Nissler wrote:
> On 2012/03/26 11:24:37, tommi wrote:
> > On 2012/03/26 11:04:41, Mattias Nissler wrote:
> > > nit: declare each parameter on separate line
> > 
> > this is the convention in this file so if you don't mind I'd like to keep it
> > this way.
> 
> I'd rather encourage you to fix the entire file, but this is CF land in which
I
> don't have much experience, so I don't really care.

Done.

[robertshield, 2012-03-26 13:36:41]

LGTM, w/ year nits.

https://chromiumcodereview.appspot.com/9836037/diff/9011/chrome_frame/policy_...
File chrome_frame/policy_settings.cc (right):

https://chromiumcodereview.appspot.com/9836037/diff/9011/chrome_frame/policy_...
chrome_frame/policy_settings.cc:1: // Copyright (c) 2010 The Chromium Authors.
All rights reserved.
2012

https://chromiumcodereview.appspot.com/9836037/diff/9011/chrome_frame/policy_...
File chrome_frame/policy_settings.h (right):

https://chromiumcodereview.appspot.com/9836037/diff/9011/chrome_frame/policy_...
chrome_frame/policy_settings.h:1: // Copyright (c) 2011 The Chromium Authors.
All rights reserved.
2012

https://chromiumcodereview.appspot.com/9836037/diff/9011/chrome_frame/test/po...
File chrome_frame/test/policy_settings_unittest.cc (right):

https://chromiumcodereview.appspot.com/9836037/diff/9011/chrome_frame/test/po...
chrome_frame/test/policy_settings_unittest.cc:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
2012

[tommi (sloooow) - chröme, 2012-03-26 15:33:24]

https://chromiumcodereview.appspot.com/9836037/diff/9011/chrome_frame/policy_...
File chrome_frame/policy_settings.cc (right):

https://chromiumcodereview.appspot.com/9836037/diff/9011/chrome_frame/policy_...
chrome_frame/policy_settings.cc:1: // Copyright (c) 2010 The Chromium Authors.
All rights reserved.
On 2012/03/26 13:36:42, robertshield wrote:
> 2012

Done.

https://chromiumcodereview.appspot.com/9836037/diff/9011/chrome_frame/policy_...
File chrome_frame/policy_settings.h (right):

https://chromiumcodereview.appspot.com/9836037/diff/9011/chrome_frame/policy_...
chrome_frame/policy_settings.h:1: // Copyright (c) 2011 The Chromium Authors.
All rights reserved.
On 2012/03/26 13:36:42, robertshield wrote:
> 2012

Done.

https://chromiumcodereview.appspot.com/9836037/diff/9011/chrome_frame/test/po...
File chrome_frame/test/policy_settings_unittest.cc (right):

https://chromiumcodereview.appspot.com/9836037/diff/9011/chrome_frame/test/po...
chrome_frame/test/policy_settings_unittest.cc:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
On 2012/03/26 13:36:42, robertshield wrote:
> 2012

Done.

[grt (UTC plus 2), 2012-03-27 20:00:43]

LGTM.  You're gonna get a load of presubmit warnings from std::wstring.  sad
face.

https://chromiumcodereview.appspot.com/9836037/diff/13007/chrome_frame/chrome...
File chrome_frame/chrome_launcher.cc (right):

https://chromiumcodereview.appspot.com/9836037/diff/13007/chrome_frame/chrome...
chrome_frame/chrome_launcher.cc:168: KEY_READ, &key);
nit: you could use KEY_QUERY_VALUE here to request least required privs.

https://chromiumcodereview.appspot.com/9836037/diff/13007/chrome_frame/policy...
File chrome_frame/policy_settings.cc (right):

https://chromiumcodereview.appspot.com/9836037/diff/13007/chrome_frame/policy...
chrome_frame/policy_settings.cc:126: value->clear();
DCHECK(value);

[tommi (sloooow) - chröme, 2012-03-27 21:19:23]

On 2012/03/27 20:00:43, grt wrote:
> LGTM.  You're gonna get a load of presubmit warnings from std::wstring.  sad
> face.
> 
>
https://chromiumcodereview.appspot.com/9836037/diff/13007/chrome_frame/chrome...
> File chrome_frame/chrome_launcher.cc (right):
> 
>
https://chromiumcodereview.appspot.com/9836037/diff/13007/chrome_frame/chrome...
> chrome_frame/chrome_launcher.cc:168: KEY_READ, &key);
> nit: you could use KEY_QUERY_VALUE here to request least required privs.
> 
>
https://chromiumcodereview.appspot.com/9836037/diff/13007/chrome_frame/policy...
> File chrome_frame/policy_settings.cc (right):
> 
>
https://chromiumcodereview.appspot.com/9836037/diff/13007/chrome_frame/policy...
> chrome_frame/policy_settings.cc:126: value->clear();
> DCHECK(value);

Thanks Greg - all done.

[commit-bot: I haz the power, 2012-03-27 22:48:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommi@chromium.org/9836037/18001

[commit-bot: I haz the power, 2012-03-27 22:48:10]

Presubmit check for 9836037-18001 failed and returned exit status 1.

Running presubmit commit checks ...
Finished checking
/mnt/data/b/commit-queue/workdir/chromium/chrome/app/policy/policy_templates.json.
0 errors, 0 warnings.

** Presubmit Warnings **
New code should not use wstrings.  If you are calling an API that accepts a
wstring, fix the API.
    chrome_frame/chrome_launcher.cc:147
    chrome_frame/chrome_launcher.cc:153
    chrome_frame/policy_settings.cc:125
    chrome_frame/policy_settings.cc:129
    chrome_frame/policy_settings.cc:146
    chrome_frame/policy_settings.h:52

Presubmit checks took 1.8s to calculate.