Use ChannelMojo between the remoting daemon and desktop processes.

Use ChannelMojo between the remoting daemon and desktop processes.

BUG=604282
Committed: https://crrev.com/34c69a1081becc6c7cb22e8429bd499f91ca885c
Cr-Commit-Position: refs/heads/master@{#428169}

[Sam McNally, 2016-10-25 06:41:11]

Patchset #1 (id:1) has been deleted

[Sam McNally, 2016-10-25 06:55:27]

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

[commit-bot: I haz the power, 2016-10-25 06:55:43]

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

[Sam McNally, 2016-10-25 06:57:25]

sammc@chromium.org changed reviewers:
+ joedow@chromium.org, sergeyu@chromium.org

[commit-bot: I haz the power, 2016-10-25 07:22:30]

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

[commit-bot: I haz the power, 2016-10-25 07:22:30]

Dry run: This issue passed the CQ dry run.

[Sam McNally, 2016-10-26 03:46:23]

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

[commit-bot: I haz the power, 2016-10-26 03:46:58]

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

[commit-bot: I haz the power, 2016-10-26 04:20:38]

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

[commit-bot: I haz the power, 2016-10-26 04:20:39]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[joedow, 2016-10-26 21:48:21]

Overall looks good, just one question to make sure I understand the security
aspect of the change.

One broader question, we are using Mojo w/ the IPC Support classes, is that the
correct pattern long term or is this an intermediate step to full mojo
conversion?

https://codereview.chromium.org/2446053002/diff/40001/remoting/host/desktop_p...
File remoting/host/desktop_process.h (right):

https://codereview.chromium.org/2446053002/diff/40001/remoting/host/desktop_p...
remoting/host/desktop_process.h:77: // Used for IPC.
nit: perhaps make this a bit more verbose: "Used for IPC communication with
Daemon process."

https://codereview.chromium.org/2446053002/diff/40001/remoting/host/desktop_p...
File remoting/host/desktop_process_main.cc (right):

https://codereview.chromium.org/2446053002/diff/40001/remoting/host/desktop_p...
remoting/host/desktop_process_main.cc:65: return kInvalidCommandLineExitCode;
nit: I'd prefer braces for single line conditions.

https://codereview.chromium.org/2446053002/diff/40001/remoting/host/win/wts_s...
File remoting/host/win/wts_session_process_delegate.cc (right):

https://codereview.chromium.org/2446053002/diff/40001/remoting/host/win/wts_s...
remoting/host/win/wts_session_process_delegate.cc:512: base::ProcessId pid) {
Thanks for moving this out of the OnClientConnected method, this feels cleaner
to me.  Just to make sure I understand correctly, we no longer need to verify
the pid in that function since Mojo will have already rejected the connection if
the process attempting to connect did not provide the correct token.

[Sam McNally, 2016-10-27 00:36:31]

Patchset #3 (id:60001) has been deleted

[Sam McNally, 2016-10-27 00:37:40]

Patchset #3 (id:80001) has been deleted

[Sam McNally, 2016-10-27 00:43:46]

On 2016/10/26 21:48:21, joedow wrote:
> Overall looks good, just one question to make sure I understand the security
> aspect of the change.
> 
> One broader question, we are using Mojo w/ the IPC Support classes, is that
the
> correct pattern long term or is this an intermediate step to full mojo
> conversion?
> 
It's an intermediate step. The long-term plan is to use mojo directly.

https://codereview.chromium.org/2446053002/diff/40001/remoting/host/desktop_p...
File remoting/host/desktop_process.h (right):

https://codereview.chromium.org/2446053002/diff/40001/remoting/host/desktop_p...
remoting/host/desktop_process.h:77: // Used for IPC.
On 2016/10/26 21:48:20, joedow wrote:
> nit: perhaps make this a bit more verbose: "Used for IPC communication with
> Daemon process."

Done.

https://codereview.chromium.org/2446053002/diff/40001/remoting/host/desktop_p...
File remoting/host/desktop_process_main.cc (right):

https://codereview.chromium.org/2446053002/diff/40001/remoting/host/desktop_p...
remoting/host/desktop_process_main.cc:65: return kInvalidCommandLineExitCode;
On 2016/10/26 21:48:20, joedow wrote:
> nit: I'd prefer braces for single line conditions.

Done.

https://codereview.chromium.org/2446053002/diff/40001/remoting/host/win/wts_s...
File remoting/host/win/wts_session_process_delegate.cc (right):

https://codereview.chromium.org/2446053002/diff/40001/remoting/host/win/wts_s...
remoting/host/win/wts_session_process_delegate.cc:512: base::ProcessId pid) {
On 2016/10/26 21:48:20, joedow wrote:
> Thanks for moving this out of the OnClientConnected method, this feels cleaner
> to me.  Just to make sure I understand correctly, we no longer need to verify
> the pid in that function since Mojo will have already rejected the connection
if
> the process attempting to connect did not provide the correct token.

Indeed, the connection will not be usable by another process that intercepts the
pipe name, but not the token. However, the reason for this change is that the
mojo system requires a process handle before the connection is usable, but
OnChannelConnected is only called after.

Assuming the launcher child process behaves correctly and only launches the
single elevated process, the process handle here will be the worker process. An
implementation detail of the mojo system is that the initial pipe used as a
broker pipe (for synchronously requesting shared memory and for passing a second
pipe handle for everything else); thus, if the process that receives the initial
pipe is not the worker process, then the second pipe handle will still be sent
to the correct worker process and the connection will be unusable.

[Sam McNally, 2016-10-27 00:43:55]

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

[commit-bot: I haz the power, 2016-10-27 00:44:36]

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

[commit-bot: I haz the power, 2016-10-27 01:25:23]

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

[commit-bot: I haz the power, 2016-10-27 01:25:23]

Dry run: This issue passed the CQ dry run.

[joedow, 2016-10-27 20:18:54]

lgtm

[Sam McNally, 2016-10-27 22:26:54]

The CQ bit was checked by sammc@chromium.org

[commit-bot: I haz the power, 2016-10-27 22:27:53]

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

[commit-bot: I haz the power, 2016-10-27 22:44:10]

Committed patchset #3 (id:100001)

[commit-bot: I haz the power, 2016-10-27 22:47:57]

Description was changed from

==========
Use ChannelMojo between the remoting daemon and desktop processes.

BUG=604282
==========

to

==========
Use ChannelMojo between the remoting daemon and desktop processes.

BUG=604282

Committed: https://crrev.com/34c69a1081becc6c7cb22e8429bd499f91ca885c
Cr-Commit-Position: refs/heads/master@{#428169}
==========

[commit-bot: I haz the power, 2016-10-27 22:47:58]

Patchset 3 (id:??) landed as
https://crrev.com/34c69a1081becc6c7cb22e8429bd499f91ca885c
Cr-Commit-Position: refs/heads/master@{#428169}

[Will Harris, 2016-10-28 00:15:46]

this has possibly caused breakage:

buildbot failure in Chromium on Win x64, revision
fed4fc1df8bab7159f19e04ed658c1f417e4e499

http://build.chromium.org/p/chromium/builders/Win%20x64/builds/5466

please look at this asap.

[Sam McNally, 2016-10-28 00:20:05]

On 2016/10/28 00:15:46, Will Harris wrote:
> this has possibly caused breakage:
> 
> buildbot failure in Chromium on Win x64, revision
> fed4fc1df8bab7159f19e04ed658c1f417e4e499
> 
> http://build.chromium.org/p/chromium/builders/Win%20x64/builds/5466
> 
> please look at this asap.

https://codereview.chromium.org/2461563002/ should fix this.

[Will Harris, 2016-10-28 03:17:21]

On 2016/10/28 00:20:05, Sam McNally wrote:
> On 2016/10/28 00:15:46, Will Harris wrote:
> > this has possibly caused breakage:
> > 
> > buildbot failure in Chromium on Win x64, revision
> > fed4fc1df8bab7159f19e04ed658c1f417e4e499
> > 
> > http://build.chromium.org/p/chromium/builders/Win%20x64/builds/5466
> > 
> > please look at this asap.
> 
> https://codereview.chromium.org/2461563002/ should fix this.

Thanks!