Launches a privileged utility process.

Creates a way to launch the utility process with elevated privileges on Windows systems for the rare operations that require administrator access.

IPCs to the utility process will be filtered when it is running elevated.

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

[mef, 2013-12-10 15:25:18]

Looks pretty good!

https://codereview.chromium.org/98603007/diff/20001/base/process/launch_win.cc
File base/process/launch_win.cc (right):

https://codereview.chromium.org/98603007/diff/20001/base/process/launch_win.c...
base/process/launch_win.cc:148: shex_info.hwnd = NULL;
We may have to set hwnd to something meaningful, otherwise UAC prompt may get
stuck behind active window (just quietly blinking on taskbar).

https://codereview.chromium.org/98603007/diff/20001/content/browser/utility_p...
File content/browser/utility_process_host_impl.h (right):

https://codereview.chromium.org/98603007/diff/20001/content/browser/utility_p...
content/browser/utility_process_host_impl.h:76: // Whether to pass
switches::kNoSandbox to the child.
wrong comment.

https://codereview.chromium.org/98603007/diff/20001/content/utility/utility_t...
File content/utility/utility_thread_impl.h (right):

https://codereview.chromium.org/98603007/diff/20001/content/utility/utility_t...
content/utility/utility_thread_impl.h:55: void OnEffectiveUser();
Can you explain this?

[mef, 2013-12-18 21:02:10]

Hi Drew, 

I'm just curious about your plans in regards to this CL. I'm very interested in
seeing it landed. :-)

thanks,
-m

https://codereview.chromium.org/98603007/diff/80001/base/process/launch.h
File base/process/launch.h (right):

https://codereview.chromium.org/98603007/diff/80001/base/process/launch.h#new...
base/process/launch.h:222: // and thus some common operations liie OpenProcess
will fail. The process will
nit: like

https://codereview.chromium.org/98603007/diff/80001/base/process/launch_win.cc
File base/process/launch_win.cc (right):

https://codereview.chromium.org/98603007/diff/80001/base/process/launch_win.c...
base/process/launch_win.cc:309: if (process_handle)
don't you need to close it otherwise?

https://codereview.chromium.org/98603007/diff/80001/content/browser/child_pro...
File content/browser/child_process_launcher_elevated.cc (right):

https://codereview.chromium.org/98603007/diff/80001/content/browser/child_pro...
content/browser/child_process_launcher_elevated.cc:40:
terminate_child_on_shutdown_ = true;
nit: tabbing

https://codereview.chromium.org/98603007/diff/80001/content/browser/child_pro...
content/browser/child_process_launcher_elevated.cc:137:
client_->OnProcessLaunched();
But if !handle, then process is NOT launched?

https://codereview.chromium.org/98603007/diff/80001/content/browser/child_pro...
content/browser/child_process_launcher_elevated.cc:171:
process.Terminate(RESULT_CODE_NORMAL_EXIT);
This will probably fail for elevated process, wouldn't it?

https://codereview.chromium.org/98603007/diff/80001/content/browser/child_pro...
content/browser/child_process_launcher_elevated.cc:172: // On POSIX, we must
additionally reap the child.
Not sure what to do about POSIX stuff, given that this is windows-specific?

https://codereview.chromium.org/98603007/diff/80001/content/browser/child_pro...
File content/browser/child_process_launcher_elevated.h (right):

https://codereview.chromium.org/98603007/diff/80001/content/browser/child_pro...
content/browser/child_process_launcher_elevated.h:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
nit: 2013

https://codereview.chromium.org/98603007/diff/80001/content/browser/child_pro...
content/browser/child_process_launcher_elevated.h:10: #include
"base/process/kill.h"
is kill.h needed?

https://codereview.chromium.org/98603007/diff/80001/content/browser/child_pro...
content/browser/child_process_launcher_elevated.h:34: int* exit_code) OVERRIDE;
nit: tab

https://codereview.chromium.org/98603007/diff/80001/content/browser/utility_p...
File content/browser/utility_process_host_impl.h (right):

https://codereview.chromium.org/98603007/diff/80001/content/browser/utility_p...
content/browser/utility_process_host_impl.h:79: // Whether to launche the
process with elevated permissions.
nit: launche

[Drew Haven, 2013-12-18 21:38:40]

My plan right now is that I'm finishing up some ChromeOS and
platform-agnostic stuff for the recovery tool, but now that the branch is
done I'll come back to this after the break.

What I want to do is clean this up and make it into more of a real CL and
not just a proof-of-concept since there aren't any major objections from
the people I've talked to so far.  Then I'll start formal review and we can
try to get it in first thing next year.


On Wed, Dec 18, 2013 at 1:02 PM, <mef@chromium.org> wrote:

> Hi Drew,
>
> I'm just curious about your plans in regards to this CL. I'm very
> interested in
> seeing it landed. :-)
>
> thanks,
> -m
>
>
> https://codereview.chromium.org/98603007/diff/80001/base/process/launch.h
> File base/process/launch.h (right):
>
> https://codereview.chromium.org/98603007/diff/80001/base/
> process/launch.h#newcode222
> base/process/launch.h:222: // and thus some common operations liie
> OpenProcess will fail. The process will
> nit: like
>
> https://codereview.chromium.org/98603007/diff/80001/base/
> process/launch_win.cc
> File base/process/launch_win.cc (right):
>
> https://codereview.chromium.org/98603007/diff/80001/base/
> process/launch_win.cc#newcode309
> base/process/launch_win.cc:309: if (process_handle)
> don't you need to close it otherwise?
>
> https://codereview.chromium.org/98603007/diff/80001/
> content/browser/child_process_launcher_elevated.cc
> File content/browser/child_process_launcher_elevated.cc (right):
>
> https://codereview.chromium.org/98603007/diff/80001/
> content/browser/child_process_launcher_elevated.cc#newcode40
> content/browser/child_process_launcher_elevated.cc:40:
> terminate_child_on_shutdown_ = true;
> nit: tabbing
>
> https://codereview.chromium.org/98603007/diff/80001/
> content/browser/child_process_launcher_elevated.cc#newcode137
> content/browser/child_process_launcher_elevated.cc:137:
> client_->OnProcessLaunched();
> But if !handle, then process is NOT launched?
>
> https://codereview.chromium.org/98603007/diff/80001/
> content/browser/child_process_launcher_elevated.cc#newcode171
> content/browser/child_process_launcher_elevated.cc:171:
> process.Terminate(RESULT_CODE_NORMAL_EXIT);
> This will probably fail for elevated process, wouldn't it?
>
> https://codereview.chromium.org/98603007/diff/80001/
> content/browser/child_process_launcher_elevated.cc#newcode172
> content/browser/child_process_launcher_elevated.cc:172: // On POSIX, we
> must additionally reap the child.
> Not sure what to do about POSIX stuff, given that this is
> windows-specific?
>
> https://codereview.chromium.org/98603007/diff/80001/
> content/browser/child_process_launcher_elevated.h
> File content/browser/child_process_launcher_elevated.h (right):
>
> https://codereview.chromium.org/98603007/diff/80001/
> content/browser/child_process_launcher_elevated.h#newcode1
> content/browser/child_process_launcher_elevated.h:1: // Copyright (c)
> 2012 The Chromium Authors. All rights reserved.
> nit: 2013
>
> https://codereview.chromium.org/98603007/diff/80001/
> content/browser/child_process_launcher_elevated.h#newcode10
> content/browser/child_process_launcher_elevated.h:10: #include
> "base/process/kill.h"
> is kill.h needed?
>
> https://codereview.chromium.org/98603007/diff/80001/
> content/browser/child_process_launcher_elevated.h#newcode34
> content/browser/child_process_launcher_elevated.h:34: int* exit_code)
> OVERRIDE;
> nit: tab
>
> https://codereview.chromium.org/98603007/diff/80001/
> content/browser/utility_process_host_impl.h
> File content/browser/utility_process_host_impl.h (right):
>
> https://codereview.chromium.org/98603007/diff/80001/
> content/browser/utility_process_host_impl.h#newcode79
> content/browser/utility_process_host_impl.h:79: // Whether to launche
> the process with elevated permissions.
> nit: launche
>
> https://codereview.chromium.org/98603007/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Drew Haven, 2014-01-09 01:15:13]

Alright, I addressed some comments and did some clean up.  I also integrated the
message filtering.

Mef, can you start taking a look and catch the little errors and make sure some
of the design is decent?  Then we can rope in the others for the final review.

https://codereview.chromium.org/98603007/diff/80001/base/process/launch.h
File base/process/launch.h (right):

https://codereview.chromium.org/98603007/diff/80001/base/process/launch.h#new...
base/process/launch.h:222: // and thus some common operations liie OpenProcess
will fail. The process will
On 2013/12/18 21:02:10, mef wrote:
> nit: like

Done.

https://codereview.chromium.org/98603007/diff/80001/base/process/launch_win.cc
File base/process/launch_win.cc (right):

https://codereview.chromium.org/98603007/diff/80001/base/process/launch_win.c...
base/process/launch_win.cc:309: if (process_handle)
On 2013/12/18 21:02:10, mef wrote:
> don't you need to close it otherwise?

You know, this was just taken directly from the default implementation, which
doesn't close it either.  It looks like I should be closing it off.

https://codereview.chromium.org/98603007/diff/80001/content/browser/child_pro...
File content/browser/child_process_launcher_elevated.cc (right):

https://codereview.chromium.org/98603007/diff/80001/content/browser/child_pro...
content/browser/child_process_launcher_elevated.cc:40:
terminate_child_on_shutdown_ = true;
On 2013/12/18 21:02:10, mef wrote:
> nit: tabbing

Done.

https://codereview.chromium.org/98603007/diff/80001/content/browser/child_pro...
content/browser/child_process_launcher_elevated.cc:137:
client_->OnProcessLaunched();
On 2013/12/18 21:02:10, mef wrote:
> But if !handle, then process is NOT launched?

I'm not really sure what's going on here.  It's just what the existing code was
doing.

https://codereview.chromium.org/98603007/diff/80001/content/browser/child_pro...
content/browser/child_process_launcher_elevated.cc:171:
process.Terminate(RESULT_CODE_NORMAL_EXIT);
On 2013/12/18 21:02:10, mef wrote:
> This will probably fail for elevated process, wouldn't it?

I tested it on my Windows 7 box and the parent process does get
PROCESS_TERMINATE permissions for the child handle.

https://codereview.chromium.org/98603007/diff/80001/content/browser/child_pro...
content/browser/child_process_launcher_elevated.cc:172: // On POSIX, we must
additionally reap the child.
On 2013/12/18 21:02:10, mef wrote:
> Not sure what to do about POSIX stuff, given that this is windows-specific?

For now, at least, I made this class entirely windows-only.  I'm not sure if
that's the right solution, but it shouldn't be used outside of the windows
environment right now.

https://codereview.chromium.org/98603007/diff/80001/content/browser/child_pro...
File content/browser/child_process_launcher_elevated.h (right):

https://codereview.chromium.org/98603007/diff/80001/content/browser/child_pro...
content/browser/child_process_launcher_elevated.h:10: #include
"base/process/kill.h"
On 2013/12/18 21:02:10, mef wrote:
> is kill.h needed?

In fact, most of these aren't needed.

https://codereview.chromium.org/98603007/diff/80001/content/browser/utility_p...
File content/browser/utility_process_host_impl.h (right):

https://codereview.chromium.org/98603007/diff/80001/content/browser/utility_p...
content/browser/utility_process_host_impl.h:79: // Whether to launche the
process with elevated permissions.
On 2013/12/18 21:02:10, mef wrote:
> nit: launche

Done.

[mef, 2014-01-10 18:22:54]

https://codereview.chromium.org/98603007/diff/140001/base/process/launch.h
File base/process/launch.h (right):

https://codereview.chromium.org/98603007/diff/140001/base/process/launch.h#ne...
base/process/launch.h:224: // option that is supported from LaunchOptions is
start_hidden.
and |wait|.

https://codereview.chromium.org/98603007/diff/140001/chrome/utility/chrome_co...
File chrome/utility/chrome_content_utility_client.cc (right):

https://codereview.chromium.org/98603007/diff/140001/chrome/utility/chrome_co...
chrome/utility/chrome_content_utility_client.cc:88: : filter_messages_(false) {
nit: tab

https://codereview.chromium.org/98603007/diff/140001/chrome/utility/chrome_co...
chrome/utility/chrome_content_utility_client.cc:90: handlers_.push_back(new
ProfileImportHandler());
nit: tab not needed?

https://codereview.chromium.org/98603007/diff/140001/chrome/utility/chrome_co...
File chrome/utility/chrome_content_utility_client_unittest.cc (right):

https://codereview.chromium.org/98603007/diff/140001/chrome/utility/chrome_co...
chrome/utility/chrome_content_utility_client_unittest.cc:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
nit: 2014

https://codereview.chromium.org/98603007/diff/140001/content/browser/child_pr...
File content/browser/child_process_launcher.cc (right):

https://codereview.chromium.org/98603007/diff/140001/content/browser/child_pr...
content/browser/child_process_launcher.cc:31: base::EnvironmentMap& environ,
does base::EnvironmentMap& belong here?

https://codereview.chromium.org/98603007/diff/140001/content/browser/child_pr...
File content/browser/child_process_launcher_elevated_win.cc (right):

https://codereview.chromium.org/98603007/diff/140001/content/browser/child_pr...
content/browser/child_process_launcher_elevated_win.cc:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
nit: Copyright 2014

https://codereview.chromium.org/98603007/diff/140001/content/browser/child_pr...
content/browser/child_process_launcher_elevated_win.cc:23: // Having the
functionality of ChildProcessLauncher be in an internal
Could you re-word it?

https://codereview.chromium.org/98603007/diff/140001/content/browser/child_pr...
content/browser/child_process_launcher_elevated_win.cc:76: base::TimeDelta
launch_time = base::TimeTicks::Now() - begin_launch_time;
Does this include the time of user sitting on UAC prompt?

https://codereview.chromium.org/98603007/diff/140001/content/browser/child_pr...
content/browser/child_process_launcher_elevated_win.cc:115:
RecordHistograms(begin_launch_time);
Would we want to histogram failures as well?

https://codereview.chromium.org/98603007/diff/140001/content/browser/child_pr...
content/browser/child_process_launcher_elevated_win.cc:130: LOG(ERROR) <<
"Failed to launch child process";
It seems that if launch fails, then there is no notification to client.

https://codereview.chromium.org/98603007/diff/140001/content/browser/child_pr...
content/browser/child_process_launcher_elevated_win.cc:146: // On Posix,
EnsureProcessTerminated can lead to 2 seconds of sleep!  So
Stale comment?

https://codereview.chromium.org/98603007/diff/140001/content/browser/child_pr...
content/browser/child_process_launcher_elevated_win.cc:159:
process.SetProcessBackgrounded(background);
Does this actually work on elevated process?

https://codereview.chromium.org/98603007/diff/140001/content/browser/child_pr...
content/browser/child_process_launcher_elevated_win.cc:168: // On POSIX, we must
additionally reap the child.
This is _win module...

https://codereview.chromium.org/98603007/diff/140001/content/browser/child_pr...
content/browser/child_process_launcher_elevated_win.cc:176: int exit_code_;
not populated?

https://codereview.chromium.org/98603007/diff/140001/content/browser/child_pr...
content/browser/child_process_launcher_elevated_win.cc:180: bool
terminate_child_on_shutdown_;
is it implemented?

https://codereview.chromium.org/98603007/diff/140001/content/browser/child_pr...
File content/browser/child_process_launcher_elevated_win.h (right):

https://codereview.chromium.org/98603007/diff/140001/content/browser/child_pr...
content/browser/child_process_launcher_elevated_win.h:1: // Copyright (c) 2013
The Chromium Authors. All rights reserved.
nit: Copyright 2014

https://codereview.chromium.org/98603007/diff/140001/content/browser/child_pr...
File content/browser/child_process_launcher_impl.cc (right):

https://codereview.chromium.org/98603007/diff/140001/content/browser/child_pr...
content/browser/child_process_launcher_impl.cc:197: 
nit: nl

https://codereview.chromium.org/98603007/diff/140001/content/browser/utility_...
File content/browser/utility_process_host_impl_browsertest.cc (right):

https://codereview.chromium.org/98603007/diff/140001/content/browser/utility_...
content/browser/utility_process_host_impl_browsertest.cc:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
nit: 2014

https://codereview.chromium.org/98603007/diff/140001/content/browser/utility_...
content/browser/utility_process_host_impl_browsertest.cc:15: // requires
interaction in the form of a UAC prompt
I wonder, how would this whole thing work on XP?

https://codereview.chromium.org/98603007/diff/140001/content/common/child_pro...
File content/common/child_process_host_impl.cc (right):

https://codereview.chromium.org/98603007/diff/140001/content/common/child_pro...
content/common/child_process_host_impl.cc:267: !(peer_handle_ =
delegate_->GetHandle())) {
What if user has cancelled the UAC, wouldn't delegate_->GetHandle be NULL?

https://codereview.chromium.org/98603007/diff/140001/content/common/utility_m...
File content/common/utility_messages.h (right):

https://codereview.chromium.org/98603007/diff/140001/content/common/utility_m...
content/common/utility_messages.h:58: // TODO(haven): Debugging only.  Remove
before submission.
Is this correct comment? 
It seems to report whether utility process is actually elevated.

[Drew Haven, 2014-01-16 02:52:04]

There are still a few things I need to work on around UAC cancelling and XP. 
I'll keep working on those but in the mean time here is a response to the
initial comments.

Thanks for taking a look.  You're making me actually have to understand all the
original process launching code that I was mostly taking for granted to begin
with.

https://codereview.chromium.org/98603007/diff/140001/base/process/launch.h
File base/process/launch.h (right):

https://codereview.chromium.org/98603007/diff/140001/base/process/launch.h#ne...
base/process/launch.h:224: // option that is supported from LaunchOptions is
start_hidden.
On 2014/01/10 18:22:55, mef wrote:
> and |wait|.

Good catch. Done.

https://codereview.chromium.org/98603007/diff/140001/chrome/utility/chrome_co...
File chrome/utility/chrome_content_utility_client.cc (right):

https://codereview.chromium.org/98603007/diff/140001/chrome/utility/chrome_co...
chrome/utility/chrome_content_utility_client.cc:88: : filter_messages_(false) {
On 2014/01/10 18:22:55, mef wrote:
> nit: tab

Done.

https://codereview.chromium.org/98603007/diff/140001/chrome/utility/chrome_co...
chrome/utility/chrome_content_utility_client.cc:90: handlers_.push_back(new
ProfileImportHandler());
On 2014/01/10 18:22:55, mef wrote:
> nit: tab not needed?

Thanks.  My windows vim indentation-config needs needs to be fixed.

https://codereview.chromium.org/98603007/diff/140001/chrome/utility/chrome_co...
File chrome/utility/chrome_content_utility_client_unittest.cc (right):

https://codereview.chromium.org/98603007/diff/140001/chrome/utility/chrome_co...
chrome/utility/chrome_content_utility_client_unittest.cc:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
On 2014/01/10 18:22:55, mef wrote:
> nit: 2014

Done.

https://codereview.chromium.org/98603007/diff/140001/content/browser/child_pr...
File content/browser/child_process_launcher.cc (right):

https://codereview.chromium.org/98603007/diff/140001/content/browser/child_pr...
content/browser/child_process_launcher.cc:31: base::EnvironmentMap& environ,
On 2014/01/10 18:22:55, mef wrote:
> does base::EnvironmentMap& belong here?

Not at all. :)

https://codereview.chromium.org/98603007/diff/140001/content/browser/child_pr...
File content/browser/child_process_launcher_elevated_win.cc (right):

https://codereview.chromium.org/98603007/diff/140001/content/browser/child_pr...
content/browser/child_process_launcher_elevated_win.cc:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
On 2014/01/10 18:22:55, mef wrote:
> nit: Copyright 2014

Done.

https://codereview.chromium.org/98603007/diff/140001/content/browser/child_pr...
content/browser/child_process_launcher_elevated_win.cc:23: // Having the
functionality of ChildProcessLauncher be in an internal
On 2014/01/10 18:22:55, mef wrote:
> Could you re-word it?

Done.

https://codereview.chromium.org/98603007/diff/140001/content/browser/child_pr...
content/browser/child_process_launcher_elevated_win.cc:76: base::TimeDelta
launch_time = base::TimeTicks::Now() - begin_launch_time;
On 2014/01/10 18:22:55, mef wrote:
> Does this include the time of user sitting on UAC prompt?

Yes it will.  That might mess up our statistics?  Think we should just remove it
or give it its own histogram?

https://codereview.chromium.org/98603007/diff/140001/content/browser/child_pr...
content/browser/child_process_launcher_elevated_win.cc:115:
RecordHistograms(begin_launch_time);
On 2014/01/10 18:22:55, mef wrote:
> Would we want to histogram failures as well?

In this case I'm duplicating what the non-elevated code has historically done. 
It's probably a lot more relevant in our case because the user could cancel or
not have the proper permissions.

https://codereview.chromium.org/98603007/diff/140001/content/browser/child_pr...
content/browser/child_process_launcher_elevated_win.cc:130: LOG(ERROR) <<
"Failed to launch child process";
On 2014/01/10 18:22:55, mef wrote:
> It seems that if launch fails, then there is no notification to client.

This is a good point.  I'll add a method with a default no-op implementation for
that so that going forward the client can subscribe to failure notifications.

https://codereview.chromium.org/98603007/diff/140001/content/browser/child_pr...
content/browser/child_process_launcher_elevated_win.cc:146: // On Posix,
EnsureProcessTerminated can lead to 2 seconds of sleep!  So
On 2014/01/10 18:22:55, mef wrote:
> Stale comment?

Done.

https://codereview.chromium.org/98603007/diff/140001/content/browser/child_pr...
content/browser/child_process_launcher_elevated_win.cc:159:
process.SetProcessBackgrounded(background);
On 2014/01/10 18:22:55, mef wrote:
> Does this actually work on elevated process?

You're right.  Since we shouldn't be able to lower the priority of the process
this should not be allowed.  I've changed the implementation of the interface to
be a no-op for the launcher.

https://codereview.chromium.org/98603007/diff/140001/content/browser/child_pr...
content/browser/child_process_launcher_elevated_win.cc:168: // On POSIX, we must
additionally reap the child.
On 2014/01/10 18:22:55, mef wrote:
> This is _win module...

Done.

Yeah... bulk copies.  Oops.  Fortunately that comment is just an FYI.

https://codereview.chromium.org/98603007/diff/140001/content/browser/child_pr...
content/browser/child_process_launcher_elevated_win.cc:176: int exit_code_;
On 2014/01/10 18:22:55, mef wrote:
> not populated?

It is, see line 217.  Interestingly those members aren't private, however to
mirror the design of the other process launcher I think maybe I should leave
them public?

https://codereview.chromium.org/98603007/diff/140001/content/browser/child_pr...
content/browser/child_process_launcher_elevated_win.cc:180: bool
terminate_child_on_shutdown_;
On 2014/01/10 18:22:55, mef wrote:
> is it implemented?

It might be better phrased as terminate_child_on_destruction_  It will
essentially cause the destructor to be a no-op and not force the process to be
terminated if the launcher is destroyed.

https://codereview.chromium.org/98603007/diff/140001/content/browser/child_pr...
File content/browser/child_process_launcher_elevated_win.h (right):

https://codereview.chromium.org/98603007/diff/140001/content/browser/child_pr...
content/browser/child_process_launcher_elevated_win.h:1: // Copyright (c) 2013
The Chromium Authors. All rights reserved.
On 2014/01/10 18:22:55, mef wrote:
> nit: Copyright 2014

Done.

https://codereview.chromium.org/98603007/diff/140001/content/browser/child_pr...
File content/browser/child_process_launcher_impl.cc (right):

https://codereview.chromium.org/98603007/diff/140001/content/browser/child_pr...
content/browser/child_process_launcher_impl.cc:197: 
On 2014/01/10 18:22:55, mef wrote:
> nit: nl

Done.

https://codereview.chromium.org/98603007/diff/140001/content/browser/utility_...
File content/browser/utility_process_host_impl_browsertest.cc (right):

https://codereview.chromium.org/98603007/diff/140001/content/browser/utility_...
content/browser/utility_process_host_impl_browsertest.cc:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
On 2014/01/10 18:22:55, mef wrote:
> nit: 2014

Done.

https://codereview.chromium.org/98603007/diff/140001/content/browser/utility_...
content/browser/utility_process_host_impl_browsertest.cc:15: // requires
interaction in the form of a UAC prompt
On 2014/01/10 18:22:55, mef wrote:
> I wonder, how would this whole thing work on XP?

Good point.  I'll try to find an XP box for testing.

https://codereview.chromium.org/98603007/diff/140001/content/common/child_pro...
File content/common/child_process_host_impl.cc (right):

https://codereview.chromium.org/98603007/diff/140001/content/common/child_pro...
content/common/child_process_host_impl.cc:267: !(peer_handle_ =
delegate_->GetHandle())) {
On 2014/01/10 18:22:55, mef wrote:
> What if user has cancelled the UAC, wouldn't delegate_->GetHandle be NULL? 

If the user didn't accept the UAC prompt then the channel should never be
connected because there's no process to connect on the other side.  At least,
that's my guess, but my test is crashing and I need to look into it further. 
Good catch.

https://codereview.chromium.org/98603007/diff/140001/content/common/utility_m...
File content/common/utility_messages.h (right):

https://codereview.chromium.org/98603007/diff/140001/content/common/utility_m...
content/common/utility_messages.h:58: // TODO(haven): Debugging only.  Remove
before submission.
On 2014/01/10 18:22:55, mef wrote:
> Is this correct comment? 
> It seems to report whether utility process is actually elevated.

I had originally thought it would be moved to the test file, but I need to leave
it in if I want to run the test in content.  Also, I don't think it hurts
anything.

[mef, 2014-01-16 16:22:31]

Drew, thanks! I am very excited to see it coming together. 
Did you actually upload the new patch? 
The last one I see is patch #8, which is a week old.

https://codereview.chromium.org/98603007/diff/140001/content/browser/child_pr...
File content/browser/child_process_launcher_elevated_win.cc (right):

https://codereview.chromium.org/98603007/diff/140001/content/browser/child_pr...
content/browser/child_process_launcher_elevated_win.cc:76: base::TimeDelta
launch_time = base::TimeTicks::Now() - begin_launch_time;
On 2014/01/16 02:52:05, Drew Haven wrote:
> On 2014/01/10 18:22:55, mef wrote:
> > Does this include the time of user sitting on UAC prompt?
> 
> Yes it will.  That might mess up our statistics?  Think we should just remove
it
> or give it its own histogram?
Good question. I think it is pretty useless, given that it includes random user
interaction time. :-(

https://codereview.chromium.org/98603007/diff/140001/content/browser/child_pr...
content/browser/child_process_launcher_elevated_win.cc:115:
RecordHistograms(begin_launch_time);
On 2014/01/16 02:52:05, Drew Haven wrote:
> On 2014/01/10 18:22:55, mef wrote:
> > Would we want to histogram failures as well?
> 
> In this case I'm duplicating what the non-elevated code has historically done.

> It's probably a lot more relevant in our case because the user could cancel or
> not have the proper permissions.
Good point, especially if we can get meaningful failure code.

https://codereview.chromium.org/98603007/diff/140001/content/common/utility_m...
File content/common/utility_messages.h (right):

https://codereview.chromium.org/98603007/diff/140001/content/common/utility_m...
content/common/utility_messages.h:58: // TODO(haven): Debugging only.  Remove
before submission.
On 2014/01/16 02:52:05, Drew Haven wrote:
> On 2014/01/10 18:22:55, mef wrote:
> > Is this correct comment? 
> > It seems to report whether utility process is actually elevated.
> 
> I had originally thought it would be moved to the test file, but I need to
leave
> it in if I want to run the test in content.  Also, I don't think it hurts
> anything.
I agree.

[Drew Haven, 2014-01-16 20:13:44]

Oops, I did forget to finish the upload.  It's up now.

On 2014/01/16 16:22:31, mef wrote:
> Drew, thanks! I am very excited to see it coming together. 
> Did you actually upload the new patch? 
> The last one I see is patch #8, which is a week old.
> 
>
https://codereview.chromium.org/98603007/diff/140001/content/browser/child_pr...
> File content/browser/child_process_launcher_elevated_win.cc (right):
> 
>
https://codereview.chromium.org/98603007/diff/140001/content/browser/child_pr...
> content/browser/child_process_launcher_elevated_win.cc:76: base::TimeDelta
> launch_time = base::TimeTicks::Now() - begin_launch_time;
> On 2014/01/16 02:52:05, Drew Haven wrote:
> > On 2014/01/10 18:22:55, mef wrote:
> > > Does this include the time of user sitting on UAC prompt?
> > 
> > Yes it will.  That might mess up our statistics?  Think we should just
remove
> it
> > or give it its own histogram?
> Good question. I think it is pretty useless, given that it includes random
user
> interaction time. :-(
> 
>
https://codereview.chromium.org/98603007/diff/140001/content/browser/child_pr...
> content/browser/child_process_launcher_elevated_win.cc:115:
> RecordHistograms(begin_launch_time);
> On 2014/01/16 02:52:05, Drew Haven wrote:
> > On 2014/01/10 18:22:55, mef wrote:
> > > Would we want to histogram failures as well?
> > 
> > In this case I'm duplicating what the non-elevated code has historically
done.
> 
> > It's probably a lot more relevant in our case because the user could cancel
or
> > not have the proper permissions.
> Good point, especially if we can get meaningful failure code.
> 
>
https://codereview.chromium.org/98603007/diff/140001/content/common/utility_m...
> File content/common/utility_messages.h (right):
> 
>
https://codereview.chromium.org/98603007/diff/140001/content/common/utility_m...
> content/common/utility_messages.h:58: // TODO(haven): Debugging only.  Remove
> before submission.
> On 2014/01/16 02:52:05, Drew Haven wrote:
> > On 2014/01/10 18:22:55, mef wrote:
> > > Is this correct comment? 
> > > It seems to report whether utility process is actually elevated.
> > 
> > I had originally thought it would be moved to the test file, but I need to
> leave
> > it in if I want to run the test in content.  Also, I don't think it hurts
> > anything.
> I agree.

[mef, 2014-01-17 16:57:06]

Looks pretty good to me. I can't wait to try it out.

https://codereview.chromium.org/98603007/diff/300001/content/browser/child_pr...
File content/browser/child_process_launcher.h (right):

https://codereview.chromium.org/98603007/diff/300001/content/browser/child_pr...
content/browser/child_process_launcher.h:42: // this object destructs, it will
be terminated.
I think this comment is out of date, especially regards to cmd_line parameter.

https://codereview.chromium.org/98603007/diff/300001/content/browser/child_pr...
File content/browser/child_process_launcher_elevated_win.cc (right):

https://codereview.chromium.org/98603007/diff/300001/content/browser/child_pr...
content/browser/child_process_launcher_elevated_win.cc:132: if (client_) {
BUG? Did you mean if (handle)?

[Drew Haven, 2014-01-22 23:14:24]

Alright, things are starting to look good.  I'm going to start looping in some
security and core folks maybe to get more approval.  Anyone you think I should
be sure to include?

https://codereview.chromium.org/98603007/diff/300001/content/browser/child_pr...
File content/browser/child_process_launcher.h (right):

https://codereview.chromium.org/98603007/diff/300001/content/browser/child_pr...
content/browser/child_process_launcher.h:42: // this object destructs, it will
be terminated.
On 2014/01/17 16:57:06, mef wrote:
> I think this comment is out of date, especially regards to cmd_line parameter.

Right.  This is an interface now, so it doesn't take ownership of anything. I
moved this comment down to the static methods where it now belongs.

https://codereview.chromium.org/98603007/diff/300001/content/browser/child_pr...
File content/browser/child_process_launcher_elevated_win.cc (right):

https://codereview.chromium.org/98603007/diff/300001/content/browser/child_pr...
content/browser/child_process_launcher_elevated_win.cc:132: if (client_) {
On 2014/01/17 16:57:06, mef wrote:
> BUG? Did you mean if (handle)?

You're right.  Looking at it we don't seem to have a good way to notify the
clients that the process failed to launch.  We have ProcessCrashed, but that is
designed to take an exit code.  So I added an additional method
"OnProcessLaunchFailed" to handle the case of a launch failing and thus no
exit-code being generated.  I wasn't sure if there was a good way to represent
that as an exit code.

[mef, 2014-01-23 16:08:45]

On 2014/01/22 23:14:24, Drew Haven wrote:
> Alright, things are starting to look good.  I'm going to start looping in some
> security and core folks maybe to get more approval.  Anyone you think I should
> be sure to include?
> 
>
https://codereview.chromium.org/98603007/diff/300001/content/browser/child_pr...
> File content/browser/child_process_launcher.h (right):
> 
>
https://codereview.chromium.org/98603007/diff/300001/content/browser/child_pr...
> content/browser/child_process_launcher.h:42: // this object destructs, it will
> be terminated.
> On 2014/01/17 16:57:06, mef wrote:
> > I think this comment is out of date, especially regards to cmd_line
parameter.
> 
> Right.  This is an interface now, so it doesn't take ownership of anything. I
> moved this comment down to the static methods where it now belongs.
> 
>
https://codereview.chromium.org/98603007/diff/300001/content/browser/child_pr...
> File content/browser/child_process_launcher_elevated_win.cc (right):
> 
>
https://codereview.chromium.org/98603007/diff/300001/content/browser/child_pr...
> content/browser/child_process_launcher_elevated_win.cc:132: if (client_) {
> On 2014/01/17 16:57:06, mef wrote:
> > BUG? Did you mean if (handle)?
> 
> You're right.  Looking at it we don't seem to have a good way to notify the
> clients that the process failed to launch.  We have ProcessCrashed, but that
is
> designed to take an exit code.  So I added an additional method
> "OnProcessLaunchFailed" to handle the case of a launch failing and thus no
> exit-code being generated.  I wasn't sure if there was a good way to represent
> that as an exit code.

Looks good to me, I'd include jorgelo@, and I think cpu@ was commenting on this
topic before.

[jam, 2014-01-24 22:14:02]

some initial comments based on a quick look

https://codereview.chromium.org/98603007/diff/600001/chrome/service/service_u...
File chrome/service/service_utility_process_host.cc (right):

https://codereview.chromium.org/98603007/diff/600001/chrome/service/service_u...
chrome/service/service_utility_process_host.cc:210: return handle_;
I see where you call this, but it's not clear to me why this is needed?

https://codereview.chromium.org/98603007/diff/600001/content/browser/child_pr...
File content/browser/child_process_launcher_impl.h (right):

https://codereview.chromium.org/98603007/diff/600001/content/browser/child_pr...
content/browser/child_process_launcher_impl.h:21: class CONTENT_EXPORT
ChildProcessLauncherImpl : public ChildProcessLauncher {
why are you adding an interface around ChildProcessLauncher to support elevated
instead of adding an extra flag? we have supported different security mechanisms
like disabling sandbox or allowing a directory through that way.

[Drew Haven, 2014-01-27 21:43:51]

https://codereview.chromium.org/98603007/diff/600001/chrome/service/service_u...
File chrome/service/service_utility_process_host.cc (right):

https://codereview.chromium.org/98603007/diff/600001/chrome/service/service_u...
chrome/service/service_utility_process_host.cc:210: return handle_;
On 2014/01/24 22:14:03, jam wrote:
> I see where you call this, but it's not clear to me why this is needed?

It's because when I ran the elevated process I found that
OpenPrivilegedProcessHandle was failing.  We already have a handle, but not the
PID from ShellExecuteEx.  I couldn't find a way to get the process ID associated
with a handle without doing some complicated code execution in the context of
the other process.  As such, I just put in a way to directly get the handle we
already had instead of going back through the process ID.

I'm just going on what I could find through MSDN documentation and searches
though.  I could be wrong.

https://codereview.chromium.org/98603007/diff/600001/content/browser/child_pr...
File content/browser/child_process_launcher_impl.h (right):

https://codereview.chromium.org/98603007/diff/600001/content/browser/child_pr...
content/browser/child_process_launcher_impl.h:21: class CONTENT_EXPORT
ChildProcessLauncherImpl : public ChildProcessLauncher {
On 2014/01/24 22:14:03, jam wrote:
> why are you adding an interface around ChildProcessLauncher to support
elevated
> instead of adding an extra flag? we have supported different security
mechanisms
> like disabling sandbox or allowing a directory through that way.

When I started writing this I only added the LaunchElevated function for
Windows.  It might not be necessary or possible to do cleanly on some other
platforms and I didn't want to affect that code.  Perhaps I should add an error
for those platforms?  What I ended up with was a mess of #if defined(OS_WIN). 
In order to simply things, largely just so I could understand it, I duplicated
the ChildProcessLauncher code and started pulling out the stuff that was
irrelevant or unnecessary.  The implementation may not have ended up that
different, so perhaps I should attempt to merge them back together.  If we do
that I'd like to add implementations for the function to other OSes, but I think
at least for a long time those would be errors because we really only need this
in Windows (so far) for a few specific functions.

I tried to stick with the flag-based approach above this level.

[jam, 2014-01-27 21:55:44]

https://codereview.chromium.org/98603007/diff/600001/chrome/service/service_u...
File chrome/service/service_utility_process_host.cc (right):

https://codereview.chromium.org/98603007/diff/600001/chrome/service/service_u...
chrome/service/service_utility_process_host.cc:210: return handle_;
On 2014/01/27 21:43:51, Drew Haven wrote:
> On 2014/01/24 22:14:03, jam wrote:
> > I see where you call this, but it's not clear to me why this is needed?
> 
> It's because when I ran the elevated process I found that
> OpenPrivilegedProcessHandle was failing.  We already have a handle, but not
the
> PID from ShellExecuteEx.  I couldn't find a way to get the process ID
associated
> with a handle without doing some complicated code execution in the context of
> the other process.  As such, I just put in a way to directly get the handle we
> already had instead of going back through the process ID.
> 
> I'm just going on what I could find through MSDN documentation and searches
> though.  I could be wrong.

I may be missing something. you say you already have a handle but not the pid.
do you mean the reverse? the caller is ChildProcessHostImpl::OnChannelConnected
which has a peer_pid.

https://codereview.chromium.org/98603007/diff/600001/content/browser/child_pr...
File content/browser/child_process_launcher_impl.h (right):

https://codereview.chromium.org/98603007/diff/600001/content/browser/child_pr...
content/browser/child_process_launcher_impl.h:21: class CONTENT_EXPORT
ChildProcessLauncherImpl : public ChildProcessLauncher {
On 2014/01/27 21:43:51, Drew Haven wrote:
> On 2014/01/24 22:14:03, jam wrote:
> > why are you adding an interface around ChildProcessLauncher to support
> elevated
> > instead of adding an extra flag? we have supported different security
> mechanisms
> > like disabling sandbox or allowing a directory through that way.
> 
> When I started writing this I only added the LaunchElevated function for
> Windows.  It might not be necessary or possible to do cleanly on some other
> platforms and I didn't want to affect that code.  Perhaps I should add an
error
> for those platforms?  What I ended up with was a mess of #if defined(OS_WIN). 
> In order to simply things, largely just so I could understand it, I duplicated
> the ChildProcessLauncher code and started pulling out the stuff that was
> irrelevant or unnecessary.  The implementation may not have ended up that
> different, so perhaps I should attempt to merge them back together.  If we do
> that I'd like to add implementations for the function to other OSes, but I
think
> at least for a long time those would be errors because we really only need
this
> in Windows (so far) for a few specific functions.
> 
> I tried to stick with the flag-based approach above this level.

Yeah, please use one implementation. child_process_launcher.cc is hard to read
because of all the ifdefs. It's time to make the arguments be in a struct, which
will make things cleaner as it gets passed around to methods and across threads.

[Drew Haven, 2014-01-29 01:07:48]

On 2014/01/27 21:55:44, jam wrote:
>
https://codereview.chromium.org/98603007/diff/600001/chrome/service/service_u...
> File chrome/service/service_utility_process_host.cc (right):
> 
>
https://codereview.chromium.org/98603007/diff/600001/chrome/service/service_u...
> chrome/service/service_utility_process_host.cc:210: return handle_;
> On 2014/01/27 21:43:51, Drew Haven wrote:
> > On 2014/01/24 22:14:03, jam wrote:
> > > I see where you call this, but it's not clear to me why this is needed?
> > 
> > It's because when I ran the elevated process I found that
> > OpenPrivilegedProcessHandle was failing.  We already have a handle, but not
> the
> > PID from ShellExecuteEx.  I couldn't find a way to get the process ID
> associated
> > with a handle without doing some complicated code execution in the context
of
> > the other process.  As such, I just put in a way to directly get the handle
we
> > already had instead of going back through the process ID.
> > 
> > I'm just going on what I could find through MSDN documentation and searches
> > though.  I could be wrong.
> 
> I may be missing something. you say you already have a handle but not the pid.
> do you mean the reverse? the caller is
ChildProcessHostImpl::OnChannelConnected
> which has a peer_pid.

I looked at it again you're right and I was confused.  What it is is that
base::OpenPrivilegedProcessHandle fails to open a handle on the privileged
process.  However, we can get the handle directly from ShellExecuteEx and we
already have it.  I figured instead of informing the ChildProcessHost that the
process is running elevated and switching on that we can just reuse the handle
if we already have one.

PROCESS_QUERY_INFORMATION, PROCESS_VM_READ and PROCESS_DUP_HANDLE all appear to
be restricted when opening a handle to this PID.

> 
>
https://codereview.chromium.org/98603007/diff/600001/content/browser/child_pr...
> File content/browser/child_process_launcher_impl.h (right):
> 
>
https://codereview.chromium.org/98603007/diff/600001/content/browser/child_pr...
> content/browser/child_process_launcher_impl.h:21: class CONTENT_EXPORT
> ChildProcessLauncherImpl : public ChildProcessLauncher {
> On 2014/01/27 21:43:51, Drew Haven wrote:
> > On 2014/01/24 22:14:03, jam wrote:
> > > why are you adding an interface around ChildProcessLauncher to support
> > elevated
> > > instead of adding an extra flag? we have supported different security
> > mechanisms
> > > like disabling sandbox or allowing a directory through that way.
> > 
> > When I started writing this I only added the LaunchElevated function for
> > Windows.  It might not be necessary or possible to do cleanly on some other
> > platforms and I didn't want to affect that code.  Perhaps I should add an
> error
> > for those platforms?  What I ended up with was a mess of #if
defined(OS_WIN). 
> > In order to simply things, largely just so I could understand it, I
duplicated
> > the ChildProcessLauncher code and started pulling out the stuff that was
> > irrelevant or unnecessary.  The implementation may not have ended up that
> > different, so perhaps I should attempt to merge them back together.  If we
do
> > that I'd like to add implementations for the function to other OSes, but I
> think
> > at least for a long time those would be errors because we really only need
> this
> > in Windows (so far) for a few specific functions.
> > 
> > I tried to stick with the flag-based approach above this level.
> 
> Yeah, please use one implementation. child_process_launcher.cc is hard to read
> because of all the ifdefs. It's time to make the arguments be in a struct,
which
> will make things cleaner as it gets passed around to methods and across
threads.

Alright, I merged the implementations, which wasn't nearly as bad as I expected.
 Do you want me to do the struct-args in this CL, or should we do it in a
follow-up CL?  For the moment I left the arguments expanded.

[jam, 2014-01-30 21:41:02]

apologies for the delay, I just saw this. in the future please feel free to IM
me if I don't respond in a timely amount i.e. a few hours)

https://codereview.chromium.org/98603007/diff/760001/chrome/utility/chrome_co...
File chrome/utility/chrome_content_utility_client.h (right):

https://codereview.chromium.org/98603007/diff/760001/chrome/utility/chrome_co...
chrome/utility/chrome_content_utility_client.h:45: void
AddHandler(UtilityMessageHandler* handler);
hmm, these two methods are only added for a unittest. given how simple the
functionality is that we're testing, are you sure we need to test this?

https://codereview.chromium.org/98603007/diff/760001/content/browser/browser_...
File content/browser/browser_child_process_host_impl.cc (right):

https://codereview.chromium.org/98603007/diff/760001/content/browser/browser_...
content/browser/browser_child_process_host_impl.cc:319:
delegate_->OnProcessLaunchFailed();
nit: two space indent

https://codereview.chromium.org/98603007/diff/760001/content/browser/browser_...
File content/browser/browser_child_process_host_impl.h (right):

https://codereview.chromium.org/98603007/diff/760001/content/browser/browser_...
content/browser/browser_child_process_host_impl.h:64: virtual
base::ProcessHandle GetHandle() const;
since this is part of ChildProcessHostDelegate, drop the comment and move to
line 100 and add an OVERRIDE

https://codereview.chromium.org/98603007/diff/760001/content/browser/child_pr...
File content/browser/child_process_launcher.cc (right):

https://codereview.chromium.org/98603007/diff/760001/content/browser/child_pr...
content/browser/child_process_launcher.cc:114: 
nit: drop the empty line

https://codereview.chromium.org/98603007/diff/760001/content/browser/child_pr...
content/browser/child_process_launcher.cc:220: base::StatsTable* stats_table =
base::StatsTable::current();
this, and some of the other changes in this file are not from your change and
are from changes that are already landed. can you update your checkout so that
it shows diff from trunk instead of an earlier revision?

https://codereview.chromium.org/98603007/diff/760001/content/browser/utility_...
File content/browser/utility_process_host_impl_browsertest.cc (right):

https://codereview.chromium.org/98603007/diff/760001/content/browser/utility_...
content/browser/utility_process_host_impl_browsertest.cc:15: // requires
interaction in the form of a UAC prompt
why are including it then?

https://codereview.chromium.org/98603007/diff/760001/content/common/utility_m...
File content/common/utility_messages.h (right):

https://codereview.chromium.org/98603007/diff/760001/content/common/utility_m...
content/common/utility_messages.h:30: // Checks whether the utility process is
running elevated.
hmm, we shouldn't be putting testing IPCs here. especially since this test is
disabled and you can't run it on bots because it needs a UAC prompt

https://codereview.chromium.org/98603007/diff/760001/content/public/common/ch...
File content/public/common/child_process_host_delegate.h (right):

https://codereview.chromium.org/98603007/diff/760001/content/public/common/ch...
content/public/common/child_process_host_delegate.h:31: virtual
base::ProcessHandle GetHandle() const = 0;
nit: add documentation

[Drew Haven, 2014-01-31 20:00:11]

Alright, I think I got all the sync issues fixed and resolved feedback.  One or
two outstanding comments though, such as whether to remove the test around the
whitelist filtering.  Jorge still wants to get a good look at that Monday
anyway.

And thanks for reviewing.  Don't worry about the delay, as long as I can still
make M34. :)

https://codereview.chromium.org/98603007/diff/760001/chrome/utility/chrome_co...
File chrome/utility/chrome_content_utility_client.h (right):

https://codereview.chromium.org/98603007/diff/760001/chrome/utility/chrome_co...
chrome/utility/chrome_content_utility_client.h:45: void
AddHandler(UtilityMessageHandler* handler);
On 2014/01/30 21:41:03, jam wrote:
> hmm, these two methods are only added for a unittest. given how simple the
> functionality is that we're testing, are you sure we need to test this?

Since this has security implications, I feel like we'd like to have tests.  For
the other test with the UAC I'll remove it, but I'd like to leave this one. 
Perhaps I should change the functions to make the names more test-oriented or
reduce the access (e.g. protected).

https://codereview.chromium.org/98603007/diff/760001/content/browser/browser_...
File content/browser/browser_child_process_host_impl.cc (right):

https://codereview.chromium.org/98603007/diff/760001/content/browser/browser_...
content/browser/browser_child_process_host_impl.cc:319:
delegate_->OnProcessLaunchFailed();
On 2014/01/30 21:41:03, jam wrote:
> nit: two space indent

Done.

https://codereview.chromium.org/98603007/diff/760001/content/browser/browser_...
File content/browser/browser_child_process_host_impl.h (right):

https://codereview.chromium.org/98603007/diff/760001/content/browser/browser_...
content/browser/browser_child_process_host_impl.h:64: virtual
base::ProcessHandle GetHandle() const;
On 2014/01/30 21:41:03, jam wrote:
> since this is part of ChildProcessHostDelegate, drop the comment and move to
> line 100 and add an OVERRIDE

Done.

https://codereview.chromium.org/98603007/diff/760001/content/browser/child_pr...
File content/browser/child_process_launcher.cc (right):

https://codereview.chromium.org/98603007/diff/760001/content/browser/child_pr...
content/browser/child_process_launcher.cc:114: 
On 2014/01/30 21:41:03, jam wrote:
> nit: drop the empty line

Done.

https://codereview.chromium.org/98603007/diff/760001/content/browser/child_pr...
content/browser/child_process_launcher.cc:220: base::StatsTable* stats_table =
base::StatsTable::current();
On 2014/01/30 21:41:03, jam wrote:
> this, and some of the other changes in this file are not from your change and
> are from changes that are already landed. can you update your checkout so that
> it shows diff from trunk instead of an earlier revision?

Done.

https://codereview.chromium.org/98603007/diff/760001/content/browser/utility_...
File content/browser/utility_process_host_impl_browsertest.cc (right):

https://codereview.chromium.org/98603007/diff/760001/content/browser/utility_...
content/browser/utility_process_host_impl_browsertest.cc:15: // requires
interaction in the form of a UAC prompt
On 2014/01/30 21:41:03, jam wrote:
> why are including it then?

It's just here because I wanted a quick way to test.  We don't actually have
anything that uses this new functionality yet, so I had to set something fake
up.  It's also just easy to develop when I can run smaller test binaries instead
of starting up a whole Chrome instance.

We can definitely remove this test, but it's very useful for me during
development.

https://codereview.chromium.org/98603007/diff/760001/content/common/utility_m...
File content/common/utility_messages.h (right):

https://codereview.chromium.org/98603007/diff/760001/content/common/utility_m...
content/common/utility_messages.h:30: // Checks whether the utility process is
running elevated.
On 2014/01/30 21:41:03, jam wrote:
> hmm, we shouldn't be putting testing IPCs here. especially since this test is
> disabled and you can't run it on bots because it needs a UAC prompt

Ah, in order to create the test I needed the utility process to respond to be
launched elevated and then confirm that that bi-directional communication was
working, so this is shared with the code that does the response and the test,
hence it's here and not in the test file.

I couldn't really think of an easier way to test it while developing.  In the
long run we can remove it.  I'd be more comfortable if there were a way to
automatically test it, but I can't see any.

https://codereview.chromium.org/98603007/diff/760001/content/public/common/ch...
File content/public/common/child_process_host_delegate.h (right):

https://codereview.chromium.org/98603007/diff/760001/content/public/common/ch...
content/public/common/child_process_host_delegate.h:31: virtual
base::ProcessHandle GetHandle() const = 0;
On 2014/01/30 21:41:03, jam wrote:
> nit: add documentation

Done.

[jam, 2014-02-01 01:09:10]

https://codereview.chromium.org/98603007/diff/760001/chrome/utility/chrome_co...
File chrome/utility/chrome_content_utility_client.h (right):

https://codereview.chromium.org/98603007/diff/760001/chrome/utility/chrome_co...
chrome/utility/chrome_content_utility_client.h:45: void
AddHandler(UtilityMessageHandler* handler);
On 2014/01/31 20:00:12, Drew Haven wrote:
> On 2014/01/30 21:41:03, jam wrote:
> > hmm, these two methods are only added for a unittest. given how simple the
> > functionality is that we're testing, are you sure we need to test this?
> 
> Since this has security implications, I feel like we'd like to have tests. 
For
> the other test with the UAC I'll remove it, but I'd like to leave this one. 
> Perhaps I should change the functions to make the names more test-oriented or
> reduce the access (e.g. protected).

I understand the need to test. Perhaps there are other ways.

It seems that ChromeContentUtilityClientTest.MessageWhitelistAccept can be added
later when we actually have a whitelisted IPC. Then the test can use an existing
IPC used for production.

ChromeContentUtilityClientTest.MessageWhitelistReject doesn't need to add a
handler? i.e. it can check the return code of OnMessageReceived

ChromeContentUtilityClientTest.NoWhitelistUnelevated is checking that current
behavior of utility process works. It seems you can check the return code of
OnMessageReceived here as well. Also note that this is already tested through
other unittests which use the utility process code.

https://codereview.chromium.org/98603007/diff/840001/content/browser/ppapi_pl...
File content/browser/ppapi_plugin_process_host.cc (right):

https://codereview.chromium.org/98603007/diff/840001/content/browser/ppapi_pl...
content/browser/ppapi_plugin_process_host.cc:360:
static_cast<ChildProcessHostDelegate*>(process_.get())->GetHandle());
I just noticed this and the other places that case. It's probably cleaner to
just make BrowserChildProcessHostImpl's implementation of
ChildProcessHostDelegate be public, since it's legitimate to call one of these
methods and the casting is ugly

[Jorge Lucangeli Obes, 2014-02-03 22:14:57]

What will be needed in the future to whitelist a message? If it's a code change,
I'd like to add a specific OWNERS-file entry to gate that on a security team
LGTM.

https://codereview.chromium.org/98603007/diff/840001/content/browser/utility_...
File content/browser/utility_process_host_impl.h (right):

https://codereview.chromium.org/98603007/diff/840001/content/browser/utility_...
content/browser/utility_process_host_impl.h:80: // Whether to launch the process
with elevated permissions.
Nit: either change this to "privileges" or the above to "permissions".

[Robert Sesek, 2014-02-03 22:30:21]

(not adding myself as a reviewer, just observed this since I watch base/process)

https://codereview.chromium.org/98603007/diff/840001/base/process/launch.h
File base/process/launch.h (right):

https://codereview.chromium.org/98603007/diff/840001/base/process/launch.h#ne...
base/process/launch.h:225: BASE_EXPORT bool LaunchElevatedProcess(const
CommandLine& cmdline,
There's already a OS_WIN specific Launch function above. Can you put it there
(line 150) instead of just at the bottom of the file? Same in the .cc file.

[Drew Haven, 2014-02-04 21:25:54]

Alright, I removed those other tests that weren't really testing much and
cleaned a few minor things up.  That feature will be tested as we add more.

I'm not sure how to add a watch for an update to the whitelist.  There's no
owners file in that directory.  Do I add a per-file reviewer or is there another
OWNERS directive for what you want, jorge?

https://codereview.chromium.org/98603007/diff/760001/chrome/utility/chrome_co...
File chrome/utility/chrome_content_utility_client.h (right):

https://codereview.chromium.org/98603007/diff/760001/chrome/utility/chrome_co...
chrome/utility/chrome_content_utility_client.h:45: void
AddHandler(UtilityMessageHandler* handler);
On 2014/02/01 01:09:10, jam wrote:
> On 2014/01/31 20:00:12, Drew Haven wrote:
> > On 2014/01/30 21:41:03, jam wrote:
> > > hmm, these two methods are only added for a unittest. given how simple the
> > > functionality is that we're testing, are you sure we need to test this?
> > 
> > Since this has security implications, I feel like we'd like to have tests. 
> For
> > the other test with the UAC I'll remove it, but I'd like to leave this one. 
> > Perhaps I should change the functions to make the names more test-oriented
or
> > reduce the access (e.g. protected).
> 
> I understand the need to test. Perhaps there are other ways.
> 
> It seems that ChromeContentUtilityClientTest.MessageWhitelistAccept can be
added
> later when we actually have a whitelisted IPC. Then the test can use an
existing
> IPC used for production.
> 
> ChromeContentUtilityClientTest.MessageWhitelistReject doesn't need to add a
> handler? i.e. it can check the return code of OnMessageReceived
> 
> ChromeContentUtilityClientTest.NoWhitelistUnelevated is checking that current
> behavior of utility process works. It seems you can check the return code of
> OnMessageReceived here as well. Also note that this is already tested through
> other unittests which use the utility process code.

Okay, that generally sounds good.  Once we start adding new IPCs that should
only be run when elevated then we can test that they are properly whitelisted in
their tests and we don't really need these generic tests and extra methods.

I'll pull out all these tests.  In my next CL when I actually use this feature,
I'll test there.

https://codereview.chromium.org/98603007/diff/840001/base/process/launch.h
File base/process/launch.h (right):

https://codereview.chromium.org/98603007/diff/840001/base/process/launch.h#ne...
base/process/launch.h:225: BASE_EXPORT bool LaunchElevatedProcess(const
CommandLine& cmdline,
On 2014/02/03 22:30:22, rsesek wrote:
> There's already a OS_WIN specific Launch function above. Can you put it there
> (line 150) instead of just at the bottom of the file? Same in the .cc file.

Done.

https://codereview.chromium.org/98603007/diff/840001/content/browser/ppapi_pl...
File content/browser/ppapi_plugin_process_host.cc (right):

https://codereview.chromium.org/98603007/diff/840001/content/browser/ppapi_pl...
content/browser/ppapi_plugin_process_host.cc:360:
static_cast<ChildProcessHostDelegate*>(process_.get())->GetHandle());
On 2014/02/01 01:09:10, jam wrote:
> I just noticed this and the other places that case. It's probably cleaner to
> just make BrowserChildProcessHostImpl's implementation of
> ChildProcessHostDelegate be public, since it's legitimate to call one of these
> methods and the casting is ugly

Okay, I sort of thought the same thing.  But I didn't know which was the lesser
of the two evils.

https://codereview.chromium.org/98603007/diff/840001/content/browser/utility_...
File content/browser/utility_process_host_impl.h (right):

https://codereview.chromium.org/98603007/diff/840001/content/browser/utility_...
content/browser/utility_process_host_impl.h:80: // Whether to launch the process
with elevated permissions.
On 2014/02/03 22:14:59, Jorge Lucangeli Obes wrote:
> Nit: either change this to "privileges" or the above to "permissions".

Ah yes.  I'm trying to be consisted with "elevated privileges" and never
anything like "escalated permissions".  Good catch.

[jam, 2014-02-04 23:29:06]

lgtm

https://codereview.chromium.org/98603007/diff/1030001/chrome/utility/chrome_c...
File chrome/utility/chrome_content_utility_client.cc (right):

https://codereview.chromium.org/98603007/diff/1030001/chrome/utility/chrome_c...
chrome/utility/chrome_content_utility_client.cc:73: static const size_t
kMessageWhitelistSize = arraysize(kMessageWhitelist);
since you want to add owners checks for this list, i suggest putting it in a
separate file. then you can add content/utility/OWNERS and add a per-file owners
adding you and other security people. see
https://code.google.com/p/chromium/codesearch#chromium/src/content/common/OWN...
as an example

https://codereview.chromium.org/98603007/diff/1030001/content/browser/browser...
File content/browser/browser_child_process_host_impl.h (right):

https://codereview.chromium.org/98603007/diff/1030001/content/browser/browser...
content/browser/browser_child_process_host_impl.h:60: virtual
base::ProcessHandle GetHandle() const OVERRIDE;
sorry if I wasn't clear. since this is part of ChildProcessHostDelegate, this
belongs on line 93 right after OnChildDisconnected. Move that entire block (i.e.
now line 90-95) to the public section

[Jorge Lucangeli Obes, 2014-02-05 00:15:55]

https://codereview.chromium.org/98603007/diff/1030001/chrome/utility/chrome_c...
File chrome/utility/chrome_content_utility_client.cc (right):

https://codereview.chromium.org/98603007/diff/1030001/chrome/utility/chrome_c...
chrome/utility/chrome_content_utility_client.cc:73: static const size_t
kMessageWhitelistSize = arraysize(kMessageWhitelist);
On 2014/02/04 23:29:06, jam wrote:
> since you want to add owners checks for this list, i suggest putting it in a
> separate file. then you can add content/utility/OWNERS and add a per-file
owners
> adding you and other security people. see
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/common/OWN...
> as an example

Yes, this is exactly what I was suggesting.

[Drew Haven, 2014-02-05 02:36:30]

I'm looking for someone to LGTM the changes to base/process.

Otherwise things are looking good and I made those last few changes.

https://codereview.chromium.org/98603007/diff/1030001/chrome/utility/chrome_c...
File chrome/utility/chrome_content_utility_client.cc (right):

https://codereview.chromium.org/98603007/diff/1030001/chrome/utility/chrome_c...
chrome/utility/chrome_content_utility_client.cc:73: static const size_t
kMessageWhitelistSize = arraysize(kMessageWhitelist);
On 2014/02/05 00:15:55, Jorge Lucangeli Obes wrote:
> On 2014/02/04 23:29:06, jam wrote:
> > since you want to add owners checks for this list, i suggest putting it in a
> > separate file. then you can add content/utility/OWNERS and add a per-file
> owners
> > adding you and other security people. see
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/common/OWN...
> > as an example
> 
> Yes, this is exactly what I was suggesting.

Done.  I hope I set it up correctly with a .h file, .cc file and per-file rules
in the OWNERs list.  I used the same owners as were specified for IPC message
reviews for chrome/common/*messages*.h.

https://codereview.chromium.org/98603007/diff/1030001/content/browser/browser...
File content/browser/browser_child_process_host_impl.h (right):

https://codereview.chromium.org/98603007/diff/1030001/content/browser/browser...
content/browser/browser_child_process_host_impl.h:60: virtual
base::ProcessHandle GetHandle() const OVERRIDE;
On 2014/02/04 23:29:06, jam wrote:
> sorry if I wasn't clear. since this is part of ChildProcessHostDelegate, this
> belongs on line 93 right after OnChildDisconnected. Move that entire block
(i.e.
> now line 90-95) to the public section

Ah, I see.  We'll just make the whole implementation public.  Done, added those
here right under the implementation of BrowserChildProcessHost.

[Nico, 2014-02-05 05:38:10]

lgtm, but please expand the CL description so that someone who's not familiar
with this change and reads the CL description in a year can understand what this
is about.

https://codereview.chromium.org/98603007/diff/1110001/base/process/launch_win.cc
File base/process/launch_win.cc (right):

https://codereview.chromium.org/98603007/diff/1110001/base/process/launch_win...
base/process/launch_win.cc:202: const std::wstring file =
cmdline.GetProgram().value();
No wstrings in new code, use string16.

https://codereview.chromium.org/98603007/diff/1110001/chrome/utility/chrome_c...
File chrome/utility/chrome_content_utility_client.cc (right):

https://codereview.chromium.org/98603007/diff/1110001/chrome/utility/chrome_c...
chrome/utility/chrome_content_utility_client.cc:116: DVLOG(0) << "Utility
process is running elevated, restricting messages.";
Do you need this logging statement? (likely not, consider removing it.)

https://codereview.chromium.org/98603007/diff/1110001/chrome/utility/chrome_c...
chrome/utility/chrome_content_utility_client.cc:119: }
message_id_whitelist_.insert(kMessageWhitelist, kMessageWhiteliste +
kMessageWhitelistSize);

https://codereview.chromium.org/98603007/diff/1110001/chrome/utility/chrome_c...
chrome/utility/chrome_content_utility_client.cc:127: &&
message_id_whitelist_.find(message.type())
&& goes at the end of the line, not at the start.

(run `git cl format` to autoformat your current branch for style.)

https://codereview.chromium.org/98603007/diff/1110001/chrome/utility/chrome_c...
chrome/utility/chrome_content_utility_client.cc:130: << message.type();
remove logging

[mef, 2014-02-05 16:28:14]

lgtm

https://codereview.chromium.org/98603007/diff/1110001/base/process/launch.h
File base/process/launch.h (right):

https://codereview.chromium.org/98603007/diff/1110001/base/process/launch.h#n...
base/process/launch.h:157: BASE_EXPORT bool LaunchElevatedProcess(const
CommandLine& cmdline,
nit: Currently only supported LaunchOptions are |start_hidden| and |wait|.

[Jorge Lucangeli Obes, 2014-02-05 16:58:22]

not lgtm. did you forget to git add the OWNERS file? I don't see it in the list
of files.

[Robert Sesek, 2014-02-05 17:11:49]

https://codereview.chromium.org/98603007/diff/1110001/base/process/launch.h
File base/process/launch.h (left):

https://codereview.chromium.org/98603007/diff/1110001/base/process/launch.h#o...
base/process/launch.h:217: 
nit: please restore/diff noise

[Drew Haven, 2014-02-05 20:13:20]

Yep, I forgot to add the OWNERS file.  

jorgelo: can you double-check that OWNERS file?

https://codereview.chromium.org/98603007/diff/1110001/base/process/launch.h
File base/process/launch.h (left):

https://codereview.chromium.org/98603007/diff/1110001/base/process/launch.h#o...
base/process/launch.h:217: 
On 2014/02/05 17:11:50, rsesek wrote:
> nit: please restore/diff noise

Done.

https://codereview.chromium.org/98603007/diff/1110001/base/process/launch.h
File base/process/launch.h (right):

https://codereview.chromium.org/98603007/diff/1110001/base/process/launch.h#n...
base/process/launch.h:157: BASE_EXPORT bool LaunchElevatedProcess(const
CommandLine& cmdline,
On 2014/02/05 16:28:15, mef wrote:
> nit: Currently only supported LaunchOptions are |start_hidden| and |wait|.

Done.

https://codereview.chromium.org/98603007/diff/1110001/base/process/launch_win.cc
File base/process/launch_win.cc (right):

https://codereview.chromium.org/98603007/diff/1110001/base/process/launch_win...
base/process/launch_win.cc:202: const std::wstring file =
cmdline.GetProgram().value();
On 2014/02/05 05:38:11, Nico wrote:
> No wstrings in new code, use string16.

Done.

https://codereview.chromium.org/98603007/diff/1110001/chrome/utility/chrome_c...
File chrome/utility/chrome_content_utility_client.cc (right):

https://codereview.chromium.org/98603007/diff/1110001/chrome/utility/chrome_c...
chrome/utility/chrome_content_utility_client.cc:116: DVLOG(0) << "Utility
process is running elevated, restricting messages.";
On 2014/02/05 05:38:11, Nico wrote:
> Do you need this logging statement? (likely not, consider removing it.)

Done.

https://codereview.chromium.org/98603007/diff/1110001/chrome/utility/chrome_c...
chrome/utility/chrome_content_utility_client.cc:119: }
On 2014/02/05 05:38:11, Nico wrote:
> message_id_whitelist_.insert(kMessageWhitelist, kMessageWhiteliste +
> kMessageWhitelistSize);

Done.

https://codereview.chromium.org/98603007/diff/1110001/chrome/utility/chrome_c...
chrome/utility/chrome_content_utility_client.cc:127: &&
message_id_whitelist_.find(message.type())
On 2014/02/05 05:38:11, Nico wrote:
> && goes at the end of the line, not at the start.
> 
> (run `git cl format` to autoformat your current branch for style.)

Oh, I didn't know about `git cl format`.  Thanks.

https://codereview.chromium.org/98603007/diff/1110001/chrome/utility/chrome_c...
chrome/utility/chrome_content_utility_client.cc:130: << message.type();
On 2014/02/05 05:38:11, Nico wrote:
> remove logging

Done.

[Drew Haven, 2014-02-05 20:14:52]

Oops, uploaded a bad patchset.  I'll replace it but it has a "std::string16"
instead of "string16".

[Drew Haven, 2014-02-05 20:22:17]

On 2014/02/05 20:14:52, Drew Haven wrote:
> Oops, uploaded a bad patchset.  I'll replace it but it has a "std::string16"
> instead of "string16".

Fixed.

[cpu_(ooo_6.6-7.5), 2014-02-05 21:08:44]

lgtm

[Jorge Lucangeli Obes, 2014-02-05 23:14:49]

On 2014/02/05 21:08:44, cpu wrote:
> lgtm

lgtm

[Drew Haven, 2014-02-08 19:29:53]

jvoung,

I added a flag to BrowserChildProcessHost::Launch on Windows to launch a process
with elevated privileges.  It's called two places in NaCl code.  I set them to
call it with false which should default to previous behavior.  Would you please
take a look and approve if acceptable?

-- Drew

[jvoung (off chromium), 2014-02-10 15:00:42]

lgtm

[Drew Haven, 2014-02-11 08:27:07]

The CQ bit was checked by haven@chromium.org

[commit-bot: I haz the power, 2014-02-11 08:27:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haven@chromium.org/98603007/1790001

[commit-bot: I haz the power, 2014-02-11 14:45:39]

Change committed as 250409