[mojo-edk] Better validation of untrusted message data

[mojo-edk] Better validation of untrusted message data

Adds some more sanity checks to close a Channel if it
receives bad data.

BUG=611887
R=amistry@chromium.org,ochang@chromium.org

Committed: https://crrev.com/8eb4c80b7558a1bdd547e24b6b129b0d84223b21
Cr-Commit-Position: refs/heads/master@{#393874}

[Ken Rockot(use gerrit already), 2016-05-15 06:28:32]

Patchset #1 (id:1) has been deleted

[Ken Rockot(use gerrit already), 2016-05-15 06:31:52]

Patchset #1 (id:20001) has been deleted

[Ken Rockot(use gerrit already), 2016-05-15 06:43:53]

Please take a look. I've combed through the Channel code and fixed any potential
problems I could find. This CL:

  - Checks num_header_bytes and rejects when it's invalid
  - Does extra validation in Windows' GetReadPlatformHandles
    to ensure agreement between extra header size and handle
    count
  - Does more validation on OSX control messages
  - Avoids trying to memcpy empty payload and extra header
    in Channel::Message::Deserialize

[Anand Mistry (off Chromium), 2016-05-16 04:27:48]

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

https://codereview.chromium.org/1985523002/diff/60001/mojo/edk/system/channel...
mojo/edk/system/channel.cc:153: uint32_t max_handles = extra_header_size /
sizeof(MachPortsEntry);
Hm. This reminds me. This check is incorrect. It should be:
max_handles = (extra_header_size - sizeof(MachPortsExtraHeader)) /
sizeof(MachPortsEntry);

https://codereview.chromium.org/1985523002/diff/60001/mojo/edk/system/channel...
mojo/edk/system/channel.cc:167: if (data_num_bytes > header->num_header_bytes) {
These only protect against 0-byte memcpy's, which I presume memcpy would already
handle cheaply. I personally don't think it's worth it, but at the same time, I
don't care much. Take this as a comment, not a request.

https://codereview.chromium.org/1985523002/diff/60001/mojo/edk/system/channel...
mojo/edk/system/channel.cc:540: header->num_header_bytes > header->num_bytes)
Can you add a LOG(ERROR) here, similar to the one for the header->num_bytes
check above. It is, after all, and exceptional condition which should never
happen.

[Ken Rockot(use gerrit already), 2016-05-16 04:39:14]

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

https://codereview.chromium.org/1985523002/diff/60001/mojo/edk/system/channel...
mojo/edk/system/channel.cc:153: uint32_t max_handles = extra_header_size /
sizeof(MachPortsEntry);
On 2016/05/16 at 04:27:48, Anand Mistry wrote:
> Hm. This reminds me. This check is incorrect. It should be:
> max_handles = (extra_header_size - sizeof(MachPortsExtraHeader)) /
sizeof(MachPortsEntry);

Done

https://codereview.chromium.org/1985523002/diff/60001/mojo/edk/system/channel...
mojo/edk/system/channel.cc:167: if (data_num_bytes > header->num_header_bytes) {
On 2016/05/16 at 04:27:48, Anand Mistry wrote:
> These only protect against 0-byte memcpy's, which I presume memcpy would
already handle cheaply. I personally don't think it's worth it, but at the same
time, I don't care much. Take this as a comment, not a request.

Yeah - mostly I'm just uncomfortable with ever passing an invalid non-null
pointer to anyone. And when the length is zero, data + num_header_bytes is an
invalid pointer. I realize memcpy probably won't ever do anything with it, but
still... :)

https://codereview.chromium.org/1985523002/diff/60001/mojo/edk/system/channel...
mojo/edk/system/channel.cc:540: header->num_header_bytes > header->num_bytes)
On 2016/05/16 at 04:27:48, Anand Mistry wrote:
> Can you add a LOG(ERROR) here, similar to the one for the header->num_bytes
check above. It is, after all, and exceptional condition which should never
happen.

Done - though this can certainly happen with a malicious message input

[Anand Mistry (off Chromium), 2016-05-16 04:45:51]

LGTM

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

https://codereview.chromium.org/1985523002/diff/60001/mojo/edk/system/channel...
mojo/edk/system/channel.cc:540: header->num_header_bytes > header->num_bytes)
On 2016/05/16 04:39:14, Ken Rockot wrote:
> On 2016/05/16 at 04:27:48, Anand Mistry wrote:
> > Can you add a LOG(ERROR) here, similar to the one for the header->num_bytes
> check above. It is, after all, and exceptional condition which should never
> happen.
> 
> Done - though this can certainly happen with a malicious message input

Thanks. I'm assuming malicious input is not normal :P

[Ken Rockot(use gerrit already), 2016-05-16 13:59:49]

ochang@ mind taking a look for security?

[Oliver Chang, 2016-05-16 17:11:34]

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

https://codereview.chromium.org/1985523002/diff/80001/mojo/edk/system/channel...
mojo/edk/system/channel_win.cc:122: size_t handles_size = sizeof(PlatformHandle)
* num_handles;
|num_handles| here is a size_t, which means that it's possible for this
calculation to overflow (similar in channel_posix.cc).

It seems that mojo::edk::Channel::Message::Header::num_handles is uint16_t, but
could we still get a sanity check for num_handles <=
numeric_limits<uint16_t>::max()?

[Ken Rockot(use gerrit already), 2016-05-16 17:15:32]

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

https://codereview.chromium.org/1985523002/diff/80001/mojo/edk/system/channel...
mojo/edk/system/channel_win.cc:122: size_t handles_size = sizeof(PlatformHandle)
* num_handles;
On 2016/05/16 at 17:11:34, Oliver Chang wrote:
> |num_handles| here is a size_t, which means that it's possible for this
calculation to overflow (similar in channel_posix.cc).
> 
> It seems that mojo::edk::Channel::Message::Header::num_handles is uint16_t,
but could we still get a sanity check for num_handles <=
numeric_limits<uint16_t>::max()?

Done

[Oliver Chang, 2016-05-16 17:17:10]

lgtm, thanks for the fixes!

https://codereview.chromium.org/1985523002/diff/100001/mojo/edk/system/channe...
File mojo/edk/system/channel_posix.cc (right):

https://codereview.chromium.org/1985523002/diff/100001/mojo/edk/system/channe...
mojo/edk/system/channel_posix.cc:136: size_t num_handles,
could we get a similar check here too for num_handles?

[Ken Rockot(use gerrit already), 2016-05-16 17:23:48]

On 2016/05/16 at 17:17:10, ochang wrote:
> lgtm, thanks for the fixes!
> 
>
https://codereview.chromium.org/1985523002/diff/100001/mojo/edk/system/channe...
> File mojo/edk/system/channel_posix.cc (right):
> 
>
https://codereview.chromium.org/1985523002/diff/100001/mojo/edk/system/channe...
> mojo/edk/system/channel_posix.cc:136: size_t num_handles,
> could we get a similar check here too for num_handles?

Done

[Ken Rockot(use gerrit already), 2016-05-16 17:23:51]

The CQ bit was checked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2016-05-16 17:23:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
amistry@chromium.org, ochang@chromium.org
Link to the patchset: https://codereview.chromium.org/1985523002/#ps120001
(title: " ")

[commit-bot: I haz the power, 2016-05-16 17:24:21]

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

[commit-bot: I haz the power, 2016-05-16 18:25:14]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2016-05-16 18:26:40]

Description was changed from

==========
[mojo-edk] Better validation of untrusted message data

Adds some more sanity checks to close a Channel if it
receives bad data.

BUG=611887
R=amistry@chromium.org,ochang@chromium.org
==========

to

==========
[mojo-edk] Better validation of untrusted message data

Adds some more sanity checks to close a Channel if it
receives bad data.

BUG=611887
R=amistry@chromium.org,ochang@chromium.org

Committed: https://crrev.com/8eb4c80b7558a1bdd547e24b6b129b0d84223b21
Cr-Commit-Position: refs/heads/master@{#393874}
==========

[commit-bot: I haz the power, 2016-05-16 18:26:42]

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