[mojo-edk] Explicitly serialise HANDLEs into messages instead of PlatformHandles.

[mojo-edk] Explicitly serialise HANDLEs into messages instead of PlatformHandles.

This changes HANDLEs on windows to have an explicit serialised form instead of
copying the in-memory PlatformHandle class. This is intended to prevent
accidental leakage of information between processes, and address serialisation
issues such as padding.

BUG=604282
Committed: https://crrev.com/9a14e9bc319a664d5175a3cd65ea297e06f7391a
Cr-Commit-Position: refs/heads/master@{#396667}

[Anand Mistry (off Chromium), 2016-05-25 01:21:17]

amistry@chromium.org changed reviewers:
+ rockot@chromium.org

[Anand Mistry (off Chromium), 2016-05-25 01:21:17]

I think memcpy'ing PlatformHandles to/from messages is sorta brittle and
error-prone. Fds and Mach ports are explicitly converted to/from PlatformHandle
and I think it's time we do the same on windows.

Also, I'm being lazy by just using HANDLE. If you want, I can introduce a
ChannelMessage::HandleEntry struct to make it easy to extend in the future if we
need to.

I'll add a security reviewer if you're ok with the approach.

[Ken Rockot(use gerrit already), 2016-05-25 03:44:36]

On 2016/05/25 at 01:21:17, amistry wrote:
> I think memcpy'ing PlatformHandles to/from messages is sorta brittle and
error-prone. Fds and Mach ports are explicitly converted to/from PlatformHandle
and I think it's time we do the same on windows.
> 
> Also, I'm being lazy by just using HANDLE. If you want, I can introduce a
ChannelMessage::HandleEntry struct to make it easy to extend in the future if we
need to.
> 
> I'll add a security reviewer if you're ok with the approach.

If we're going to duplicate storage for Windows handles, we need to eliminate
the non-const PlatformHandle* accessor and limit handle changes to a
well-defined entry point (e.g. SetHandles) so the HANDLE data always gets
updated properly. Right now we have a static RewriteHandles that's used to dupe
arbitrary PlatformHandle arrays between processes and this is used to rewrite
Message handles in-place. The duplicated HANDLE data isn't rewritten here so no
handle passing will actually work across processes.

[Anand Mistry (off Chromium), 2016-05-25 05:37:39]

On 2016/05/25 03:44:36, Ken Rockot wrote:
> On 2016/05/25 at 01:21:17, amistry wrote:
> > I think memcpy'ing PlatformHandles to/from messages is sorta brittle and
> error-prone. Fds and Mach ports are explicitly converted to/from
PlatformHandle
> and I think it's time we do the same on windows.
> > 
> > Also, I'm being lazy by just using HANDLE. If you want, I can introduce a
> ChannelMessage::HandleEntry struct to make it easy to extend in the future if
we
> need to.
> > 
> > I'll add a security reviewer if you're ok with the approach.
> 
> If we're going to duplicate storage for Windows handles, we need to eliminate
> the non-const PlatformHandle* accessor and limit handle changes to a
> well-defined entry point (e.g. SetHandles) so the HANDLE data always gets
> updated properly. Right now we have a static RewriteHandles that's used to
dupe
> arbitrary PlatformHandle arrays between processes and this is used to rewrite
> Message handles in-place. The duplicated HANDLE data isn't rewritten here so
no
> handle passing will actually work across processes.

D'oh! That's what I get for trying to write windows code by editing on a Linux
box and using the trybots to compile/test. I'll ping this again when this change
is ready to review.

[Anand Mistry (off Chromium), 2016-05-25 10:00:16]

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

[commit-bot: I haz the power, 2016-05-25 10:00:34]

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

[commit-bot: I haz the power, 2016-05-25 11:08:01]

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

[commit-bot: I haz the power, 2016-05-25 11:08:02]

Dry run: This issue passed the CQ dry run.

[commit-bot: I haz the power, 2016-05-25 11:08:08]

Dry run: None

[Anand Mistry (off Chromium), 2016-05-25 12:04:41]

Ok, I think this is ready for review.

[Ken Rockot(use gerrit already), 2016-05-25 15:48:13]

lgtm

https://codereview.chromium.org/2008953003/diff/60001/mojo/edk/system/message...
File mojo/edk/system/message_pipe_dispatcher.cc (left):

https://codereview.chromium.org/2008953003/diff/60001/mojo/edk/system/message...
mojo/edk/system/message_pipe_dispatcher.cc:308: if (msg->num_handles() <
next_platform_handle_index ||
nit: Could you create a local num_handles near msg_handles to store this
(msg_handles ? msg_handles->size() : 0) so these conditions are more readable?

https://codereview.chromium.org/2008953003/diff/60001/mojo/edk/system/node_ch...
File mojo/edk/system/node_channel.cc (right):

https://codereview.chromium.org/2008953003/diff/60001/mojo/edk/system/node_ch...
mojo/edk/system/node_channel.cc:732: if
(!message->RewriteHandles(base::GetCurrentProcessHandle(),
nit: While you're here please update this call site to be an obviously static
method call. Looks like this is a remnant of when it was non-static, and it's
confusing.

[Anand Mistry (off Chromium), 2016-05-25 23:36:37]

amistry@chromium.org changed reviewers:
+ dcheng@chromium.org

[Anand Mistry (off Chromium), 2016-05-25 23:36:39]

+dcheng for security

https://codereview.chromium.org/2008953003/diff/60001/mojo/edk/system/message...
File mojo/edk/system/message_pipe_dispatcher.cc (left):

https://codereview.chromium.org/2008953003/diff/60001/mojo/edk/system/message...
mojo/edk/system/message_pipe_dispatcher.cc:308: if (msg->num_handles() <
next_platform_handle_index ||
On 2016/05/25 15:48:12, Ken Rockot wrote:
> nit: Could you create a local num_handles near msg_handles to store this
> (msg_handles ? msg_handles->size() : 0) so these conditions are more readable?

Done.

https://codereview.chromium.org/2008953003/diff/60001/mojo/edk/system/node_ch...
File mojo/edk/system/node_channel.cc (right):

https://codereview.chromium.org/2008953003/diff/60001/mojo/edk/system/node_ch...
mojo/edk/system/node_channel.cc:732: if
(!message->RewriteHandles(base::GetCurrentProcessHandle(),
On 2016/05/25 15:48:12, Ken Rockot wrote:
> nit: While you're here please update this call site to be an obviously static
> method call. Looks like this is a remnant of when it was non-static, and it's
> confusing.

Done.

[dcheng, 2016-05-26 01:15:07]

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

[dcheng, 2016-05-26 01:15:08]

Delegating this to wfh as a windows expert.

[Will Harris, 2016-05-26 18:32:13]

On 2016/05/26 01:15:08, dcheng wrote:
> Delegating this to wfh as a windows expert.

can you describe in the CL description what this CL is intended to do. Is it
intended to change any behavior, if so, what exactly?

[Ken Rockot(use gerrit already), 2016-05-26 19:03:09]

I'll leave Anand to update the description, but the purpose of the CL is to
change Channel::Message buffers on Windows to serialize each transferred
handle as a HANDLE instead of a mojo::edk::PlatformHandle (which on Windows
is a HANDLE plus some additional state that doesn't need to be serialized).

On Thu, May 26, 2016 at 11:32 AM, <wfh@chromium.org> wrote:

> On 2016/05/26 01:15:08, dcheng wrote:
> > Delegating this to wfh as a windows expert.
>
> can you describe in the CL description what this CL is intended to do. Is
> it
> intended to change any behavior, if so, what exactly?
>
> https://codereview.chromium.org/2008953003/
>

-- 
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-26 19:23:12]

https://codereview.chromium.org/2008953003/diff/80001/mojo/edk/system/channel.cc
File mojo/edk/system/channel.cc (right):

https://codereview.chromium.org/2008953003/diff/80001/mojo/edk/system/channel...
mojo/edk/system/channel.cc:51: extra_header_size = max_handles_ *
sizeof(HANDLE);
sizeof(HANDLE) is 8 on 64-bit, but handles are actually guaranteed to fit in
32-bit value, so you could reduce this if you wanted, but it would require casts
see base::win::HandleToUint32()

[Anand Mistry (off Chromium), 2016-05-27 00:17:23]

Description was changed from

==========
[mojo-edk] Serialise HANDLEs into messages instead of PlatformHandles.

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

to

==========
[mojo-edk] Explicitly serialise HANDLEs into messages instead of
PlatformHandles.

This changes HANDLEs on windows to have an explicit serialised form instead of
copying the in-memory PlatformHandle class. This is intended to prevent
accidental leakage of information between processes, and address serialisation
issues such as padding.

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

[Anand Mistry (off Chromium), 2016-05-27 03:44:35]

https://codereview.chromium.org/2008953003/diff/80001/mojo/edk/system/channel.cc
File mojo/edk/system/channel.cc (right):

https://codereview.chromium.org/2008953003/diff/80001/mojo/edk/system/channel...
mojo/edk/system/channel.cc:51: extra_header_size = max_handles_ *
sizeof(HANDLE);
On 2016/05/26 19:23:11, Will Harris (slow thru 23 May) wrote:
> sizeof(HANDLE) is 8 on 64-bit, but handles are actually guaranteed to fit in
> 32-bit value, so you could reduce this if you wanted, but it would require
casts
> see base::win::HandleToUint32()

I've changed this to be a uint32_t. But I've change this to use a struct instead
which follows how mach ports are handled, and adds some extensibility (and
indirection) if needed.

[Will Harris, 2016-05-27 20:19:09]

lgtm

[Anand Mistry (off Chromium), 2016-05-29 23:59:52]

The CQ bit was checked by amistry@chromium.org

[Anand Mistry (off Chromium), 2016-05-29 23:59:53]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org
Link to the patchset: https://codereview.chromium.org/2008953003/#ps140001
(title: "cast2")

[commit-bot: I haz the power, 2016-05-30 00:00:01]

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

[commit-bot: I haz the power, 2016-05-30 01:56:53]

Description was changed from

==========
[mojo-edk] Explicitly serialise HANDLEs into messages instead of
PlatformHandles.

This changes HANDLEs on windows to have an explicit serialised form instead of
copying the in-memory PlatformHandle class. This is intended to prevent
accidental leakage of information between processes, and address serialisation
issues such as padding.

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

to

==========
[mojo-edk] Explicitly serialise HANDLEs into messages instead of
PlatformHandles.

This changes HANDLEs on windows to have an explicit serialised form instead of
copying the in-memory PlatformHandle class. This is intended to prevent
accidental leakage of information between processes, and address serialisation
issues such as padding.

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

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

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-05-30 01:58:08]

Description was changed from

==========
[mojo-edk] Explicitly serialise HANDLEs into messages instead of
PlatformHandles.

This changes HANDLEs on windows to have an explicit serialised form instead of
copying the in-memory PlatformHandle class. This is intended to prevent
accidental leakage of information between processes, and address serialisation
issues such as padding.

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

to

==========
[mojo-edk] Explicitly serialise HANDLEs into messages instead of
PlatformHandles.

This changes HANDLEs on windows to have an explicit serialised form instead of
copying the in-memory PlatformHandle class. This is intended to prevent
accidental leakage of information between processes, and address serialisation
issues such as padding.

BUG=604282

Committed: https://crrev.com/9a14e9bc319a664d5175a3cd65ea297e06f7391a
Cr-Commit-Position: refs/heads/master@{#396667}
==========

[commit-bot: I haz the power, 2016-05-30 01:58:09]

Patchset 8 (id:??) landed as
https://crrev.com/9a14e9bc319a664d5175a3cd65ea297e06f7391a
Cr-Commit-Position: refs/heads/master@{#396667}