[mojo] Use a pipe path to initialise Mojo in elevated utility processes.

[mojo] Use a pipe path to initialise Mojo in elevated utility processes.

Elevated processes can't be passed HANDLEs, so instead, IPC channels
must be initialised by passing a pipe path on the command line. For security,
passing a pipe path is only done for elevated process and no other process
types.

BUG=604282
Committed: https://crrev.com/32ca1d648267a6ce2dae1417b6710089b2555f42
Cr-Commit-Position: refs/heads/master@{#395533}

[Anand Mistry (off Chromium), 2016-04-22 04:28:25]

kdfjhkdsjf

[Anand Mistry (off Chromium), 2016-05-10 03:59:56]

Description was changed from

==========
Share a pipe path on the command line.


Some changes.

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

to

==========
[mojo] Use a pipe path to initialise Mojo in elevated utility processes.

Elevated processes can't be passed HANDLEs, so instead, IPC channels
must be initialised by passing a pipe path on the command line. For security,
passing a pipe path is only done for elevated process and no other process
types.

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

[Anand Mistry (off Chromium), 2016-05-10 04:00:24]

Description was changed from

==========
[mojo] Use a pipe path to initialise Mojo in elevated utility processes.

Elevated processes can't be passed HANDLEs, so instead, IPC channels
must be initialised by passing a pipe path on the command line. For security,
passing a pipe path is only done for elevated process and no other process
types.

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

to

==========
[mojo] Use a pipe path to initialise Mojo in elevated utility processes.

Elevated processes can't be passed HANDLEs, so instead, IPC channels
must be initialised by passing a pipe path on the command line. For security,
passing a pipe path is only done for elevated process and no other process
types.

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

[Anand Mistry (off Chromium), 2016-05-10 04:04:54]

amistry@chromium.org changed reviewers:
+ nick@chromium.org, rockot@chromium.org, wfh@chromium.org

[Anand Mistry (off Chromium), 2016-05-10 04:04:55]

nick: for content/
rockot: for mojo/
wfh: for security

[Anand Mistry (off Chromium), 2016-05-10 04:46:17]

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

[commit-bot: I haz the power, 2016-05-10 04:46:25]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1893313003/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1893313003/300001

[commit-bot: I haz the power, 2016-05-10 05:46:28]

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

[commit-bot: I haz the power, 2016-05-10 05:46:29]

Dry run: This issue passed the CQ dry run.

[Ken Rockot(use gerrit already), 2016-05-10 22:41:29]

LG, just one comment first

https://codereview.chromium.org/1893313003/diff/300001/mojo/edk/system/channe...
File mojo/edk/system/channel_win.cc (right):

https://codereview.chromium.org/1893313003/diff/300001/mojo/edk/system/channe...
mojo/edk/system/channel_win.cc:155: &read_context_.overlapped);
I'd rather we use a separate IO context for connection events. That makes
OnIOCompleted easier to understand

[Will Harris, 2016-05-10 22:46:12]

wfh@chromium.org changed reviewers:
+ forshaw@chromium.org

[Will Harris, 2016-05-10 22:46:32]

+forshaw can you take a look at this pipe code?

[forshaw, 2016-05-11 00:01:41]

https://codereview.chromium.org/1893313003/diff/300001/mojo/edk/embedder/name...
File mojo/edk/embedder/named_platform_channel_pair_win.cc (right):

https://codereview.chromium.org/1893313003/diff/300001/mojo/edk/embedder/name...
mojo/edk/embedder/named_platform_channel_pair_win.cc:42: const DWORD kPipeMode =
PIPE_TYPE_BYTE | PIPE_READMODE_BYTE;
I'd recommend specifying PIPE_REJECT_REMOTE_CLIENTS here just in case. Even
though the name should be pretty hard to guess or exploit, no reason to expose
it remotely.

https://codereview.chromium.org/1893313003/diff/300001/mojo/edk/embedder/name...
mojo/edk/embedder/named_platform_channel_pair_win.cc:49: nullptr));   // Default
security descriptor.
Is there any advantage to another user on the system (say on a terminal server)
being able to open the server pipe with only read access? The default DACL gives
access to Everyone and the Anonymous user for Read. If there's some benefit to
having this limited access then we might want to ensure the DACL is explicitly
locked down. Perhaps as this is only expected to be opened by an elevated user
could specify only Administrators group.

[ncarter (slow), 2016-05-11 00:14:23]

https://codereview.chromium.org/1893313003/diff/300001/content/child/child_th...
File content/child/child_thread_impl.cc (right):

https://codereview.chromium.org/1893313003/diff/300001/content/child/child_th...
content/child/child_thread_impl.cc:234:
mojo::edk::PlatformChannelPair::kMojoPlatformChannelHandleSwitch)) {
The layering here feels little awkward -- content has to poll for the presence
of a mojo-edk switch, to know which mojo-edk function to call?

Would this be cleaner if it were just be a content switch, and we pass the
string value on to the mojo edk function?

https://codereview.chromium.org/1893313003/diff/300001/mojo/edk/embedder/name...
File mojo/edk/embedder/named_platform_channel_pair_win.cc (right):

https://codereview.chromium.org/1893313003/diff/300001/mojo/edk/embedder/name...
mojo/edk/embedder/named_platform_channel_pair_win.cc:26:
"mojo-named-platform-channel-pipe";
It seems unusual to have switches defined outside of a switches namespace.

[Anand Mistry (off Chromium), 2016-05-11 04:15:20]

https://codereview.chromium.org/1893313003/diff/300001/content/child/child_th...
File content/child/child_thread_impl.cc (right):

https://codereview.chromium.org/1893313003/diff/300001/content/child/child_th...
content/child/child_thread_impl.cc:234:
mojo::edk::PlatformChannelPair::kMojoPlatformChannelHandleSwitch)) {
On 2016/05/11 00:14:23, ncarter wrote:
> The layering here feels little awkward -- content has to poll for the presence
> of a mojo-edk switch, to know which mojo-edk function to call?
> 
> Would this be cleaner if it were just be a content switch, and we pass the
> string value on to the mojo edk function?

Hm. I agree that the switch should be moved to content instead of being part of
Mojo, but this code follows the existing pattern in
mojo::edk::PlatformChannelPair and I'd like to keep the two consistent. But even
with that, this should still be two different function calls. The fact we have a
named pipe should be a very explicit choice.

So my question is, can this change be submitted with the expectation that this
(and PlatformChannelPair) is cleaned up soon after. Or do you want me to do the
clean up first so this CL follows a cleaned up pattern?

https://codereview.chromium.org/1893313003/diff/300001/mojo/edk/embedder/name...
File mojo/edk/embedder/named_platform_channel_pair_win.cc (right):

https://codereview.chromium.org/1893313003/diff/300001/mojo/edk/embedder/name...
mojo/edk/embedder/named_platform_channel_pair_win.cc:26:
"mojo-named-platform-channel-pipe";
On 2016/05/11 00:14:23, ncarter wrote:
> It seems unusual to have switches defined outside of a switches namespace.

I'm following the pattern in PlatformChannelPair. PlatformChannelPair needs a
bit of a refactor (it has some dead code and obsolete comments) and I'd prefer
to keep these two looking similar until I get around to it.

https://codereview.chromium.org/1893313003/diff/300001/mojo/edk/embedder/name...
mojo/edk/embedder/named_platform_channel_pair_win.cc:42: const DWORD kPipeMode =
PIPE_TYPE_BYTE | PIPE_READMODE_BYTE;
On 2016/05/11 00:01:40, forshaw wrote:
> I'd recommend specifying PIPE_REJECT_REMOTE_CLIENTS here just in case. Even
> though the name should be pretty hard to guess or exploit, no reason to expose
> it remotely.

Done.

https://codereview.chromium.org/1893313003/diff/300001/mojo/edk/embedder/name...
mojo/edk/embedder/named_platform_channel_pair_win.cc:49: nullptr));   // Default
security descriptor.
On 2016/05/11 00:01:40, forshaw wrote:
> Is there any advantage to another user on the system (say on a terminal
server)
> being able to open the server pipe with only read access? The default DACL
gives
> access to Everyone and the Anonymous user for Read. If there's some benefit to
> having this limited access then we might want to ensure the DACL is explicitly
> locked down. Perhaps as this is only expected to be opened by an elevated user
> could specify only Administrators group.

I've tried to add an acl here to restrict access, but can't get it to work.
Frankly, this is far beyond my level of windows knowledge.

https://codereview.chromium.org/1893313003/diff/300001/mojo/edk/system/channe...
File mojo/edk/system/channel_win.cc (right):

https://codereview.chromium.org/1893313003/diff/300001/mojo/edk/system/channe...
mojo/edk/system/channel_win.cc:155: &read_context_.overlapped);
On 2016/05/10 22:41:28, Ken Rockot wrote:
> I'd rather we use a separate IO context for connection events. That makes
> OnIOCompleted easier to understand

Done.

[forshaw, 2016-05-11 12:03:15]

https://codereview.chromium.org/1893313003/diff/300001/mojo/edk/embedder/name...
File mojo/edk/embedder/named_platform_channel_pair_win.cc (right):

https://codereview.chromium.org/1893313003/diff/300001/mojo/edk/embedder/name...
mojo/edk/embedder/named_platform_channel_pair_win.cc:49: nullptr));   // Default
security descriptor.
> I've tried to add an acl here to restrict access, but can't get it to work.
> Frankly, this is far beyond my level of windows knowledge.

Well it's probably not needed, but a simple example would be this:

#include <sddl.h>

PSECURITY_DESCRIPTOR security_desc = nullptr;
ULONG security_desc_len = 0;
if
(!::ConvertStringSecurityDescriptorToSecurityDescriptor(L"D:(A;;GA;;;SY)(A;;GA;;;BA)(A;;GA;;;OW)",

    SDDL_REVISION_1, &security_desc, &security_desc_len)) {
  return false;
}
std::unique_ptr<void, decltype(::LocalFree)*> p(security_desc, ::LocalFree);
SECURITY_ATTRIBUTES sa = {};
sa.nLength = sizeof(sa);
sa.lpSecurityDescriptor = security_desc;

Just pass the sa variable as the last parameter to CreateNamedPipeW. That will
setup the pipe so only Local System, Local Administrators and the Owner of the
pipe can access it. This removes the implicit Everyone and Anonymous group
access control entries.

[ncarter (slow), 2016-05-11 16:06:07]

lgtm from a content/ perspective.

https://codereview.chromium.org/1893313003/diff/300001/content/child/child_th...
File content/child/child_thread_impl.cc (right):

https://codereview.chromium.org/1893313003/diff/300001/content/child/child_th...
content/child/child_thread_impl.cc:234:
mojo::edk::PlatformChannelPair::kMojoPlatformChannelHandleSwitch)) {
On 2016/05/11 04:15:20, Anand Mistry wrote:
> On 2016/05/11 00:14:23, ncarter wrote:
> > The layering here feels little awkward -- content has to poll for the
presence
> > of a mojo-edk switch, to know which mojo-edk function to call?
> > 
> > Would this be cleaner if it were just be a content switch, and we pass the
> > string value on to the mojo edk function?
> 
> Hm. I agree that the switch should be moved to content instead of being part
of
> Mojo, but this code follows the existing pattern in
> mojo::edk::PlatformChannelPair and I'd like to keep the two consistent. But
even
> with that, this should still be two different function calls. The fact we have
a
> named pipe should be a very explicit choice.
> 
> So my question is, can this change be submitted with the expectation that this
> (and PlatformChannelPair) is cleaned up soon after. Or do you want me to do
the
> clean up first so this CL follows a cleaned up pattern?

Follow-on CL is totally fine.

[Anand Mistry (off Chromium), 2016-05-12 01:17:07]

https://codereview.chromium.org/1893313003/diff/300001/mojo/edk/embedder/name...
File mojo/edk/embedder/named_platform_channel_pair_win.cc (right):

https://codereview.chromium.org/1893313003/diff/300001/mojo/edk/embedder/name...
mojo/edk/embedder/named_platform_channel_pair_win.cc:49: nullptr));   // Default
security descriptor.
On 2016/05/11 12:03:15, forshaw wrote:
> > I've tried to add an acl here to restrict access, but can't get it to work.
> > Frankly, this is far beyond my level of windows knowledge.
> 
> Well it's probably not needed, but a simple example would be this:
> 
> #include <sddl.h>
> 
> PSECURITY_DESCRIPTOR security_desc = nullptr;
> ULONG security_desc_len = 0;
> if
>
(!::ConvertStringSecurityDescriptorToSecurityDescriptor(L"D:(A;;GA;;;SY)(A;;GA;;;BA)(A;;GA;;;OW)",
> 
>     SDDL_REVISION_1, &security_desc, &security_desc_len)) {
>   return false;
> }
> std::unique_ptr<void, decltype(::LocalFree)*> p(security_desc, ::LocalFree);
> SECURITY_ATTRIBUTES sa = {};
> sa.nLength = sizeof(sa);
> sa.lpSecurityDescriptor = security_desc;
> 
> Just pass the sa variable as the last parameter to CreateNamedPipeW. That will
> setup the pipe so only Local System, Local Administrators and the Owner of the
> pipe can access it. This removes the implicit Everyone and Anonymous group
> access control entries.

Thanks. Added and it appears to work on my windows box. Lets see if it passes
the trybots.

[Ken Rockot(use gerrit already), 2016-05-13 05:32:42]

lgtm

[Anand Mistry (off Chromium), 2016-05-16 23:26:41]

forshaw: Ping!

[forshaw, 2016-05-17 11:05:35]

On 2016/05/16 23:26:41, Anand Mistry wrote:
> forshaw: Ping!

Sorry, lgtm.

[Anand Mistry (off Chromium), 2016-05-18 00:54:24]

FYI. To avoid messing up M52, I plan on submitting this after the branch point.

[Ken Rockot(use gerrit already), 2016-05-18 01:41:36]

Sgtm
On May 17, 2016 5:54 PM, <amistry@chromium.org> wrote:

> FYI. To avoid messing up M52, I plan on submitting this after the branch
> point.
>
> https://codereview.chromium.org/1893313003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Will Harris, 2016-05-18 09:08:22]

wfh@chromium.org changed reviewers:
- wfh@chromium.org

[Anand Mistry (off Chromium), 2016-05-24 05:48:35]

The CQ bit was checked by amistry@chromium.org

[Anand Mistry (off Chromium), 2016-05-24 05:48:36]

The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org,
forshaw@chromium.org, rockot@chromium.org
Link to the patchset: https://codereview.chromium.org/1893313003/#ps420001
(title: "rebase")

[commit-bot: I haz the power, 2016-05-24 05:49:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1893313003/420001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1893313003/420001

[commit-bot: I haz the power, 2016-05-24 05:52:59]

Description was changed from

==========
[mojo] Use a pipe path to initialise Mojo in elevated utility processes.

Elevated processes can't be passed HANDLEs, so instead, IPC channels
must be initialised by passing a pipe path on the command line. For security,
passing a pipe path is only done for elevated process and no other process
types.

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

to

==========
[mojo] Use a pipe path to initialise Mojo in elevated utility processes.

Elevated processes can't be passed HANDLEs, so instead, IPC channels
must be initialised by passing a pipe path on the command line. For security,
passing a pipe path is only done for elevated process and no other process
types.

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

[commit-bot: I haz the power, 2016-05-24 05:53:00]

Committed patchset #22 (id:420001)

[commit-bot: I haz the power, 2016-05-24 05:54:01]

Description was changed from

==========
[mojo] Use a pipe path to initialise Mojo in elevated utility processes.

Elevated processes can't be passed HANDLEs, so instead, IPC channels
must be initialised by passing a pipe path on the command line. For security,
passing a pipe path is only done for elevated process and no other process
types.

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

to

==========
[mojo] Use a pipe path to initialise Mojo in elevated utility processes.

Elevated processes can't be passed HANDLEs, so instead, IPC channels
must be initialised by passing a pipe path on the command line. For security,
passing a pipe path is only done for elevated process and no other process
types.

BUG=604282

Committed: https://crrev.com/32ca1d648267a6ce2dae1417b6710089b2555f42
Cr-Commit-Position: refs/heads/master@{#395533}
==========

[commit-bot: I haz the power, 2016-05-24 05:54:02]

Patchset 22 (id:??) landed as
https://crrev.com/32ca1d648267a6ce2dae1417b6710089b2555f42
Cr-Commit-Position: refs/heads/master@{#395533}

[Irfan, 2016-05-24 20:18:21]

A revert of this CL (patchset #22 id:420001) has been created in
https://codereview.chromium.org/2009033002/ by isheriff@chromium.org.

The reason for reverting is: This is failing DrMemory tests. See
https://build.chromium.org/p/chromium.memory.fyi/builders/Windows%20Unit%20%2...
for details.

[Anand Mistry (off Chromium), 2016-05-24 21:38:48]

On 2016/05/24 20:18:21, Irfan wrote:
> A revert of this CL (patchset #22 id:420001) has been created in
> https://codereview.chromium.org/2009033002/ by mailto:isheriff@chromium.org.
> 
> The reason for reverting is: This is failing DrMemory tests. See
>
https://build.chromium.org/p/chromium.memory.fyi/builders/Windows%20Unit%20%2...
> for details.

Weird. The new code paths aren't even run in that test.

[Ken Rockot(use gerrit already), 2016-05-24 21:47:08]

On 2016/05/24 at 21:38:48, amistry wrote:
> On 2016/05/24 20:18:21, Irfan wrote:
> > A revert of this CL (patchset #22 id:420001) has been created in
> > https://codereview.chromium.org/2009033002/ by mailto:isheriff@chromium.org.
> > 
> > The reason for reverting is: This is failing DrMemory tests. See
> >
https://build.chromium.org/p/chromium.memory.fyi/builders/Windows%20Unit%20%2...
> > for details.
> 
> Weird. The new code paths aren't even run in that test.

I think it's because we store PlatformHandle in buffers, and we have to
explicitly initialize that storage.