[mojo-edk] Add some buffer checks and fix UAF on NodeChannel

[mojo-edk] Add some buffer checks and fix UAF on NodeChannel

Adds message sanity checks at the dispatcher layer for
message and data pipes.

Also eliminates potential UAFs on NodeChannel by ensuring
it stays alive through any delegate calls which might have
otherwise deleted it.

BUG=613698
Committed: https://crrev.com/99a83d5bc1df0c4d6d660dedf45c9b0e14a9df3f
Cr-Commit-Position: refs/heads/master@{#395373}

[Ken Rockot(use gerrit already), 2016-05-21 23:25:53]

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

[Ken Rockot(use gerrit already), 2016-05-21 23:25:53]

PTAL - I think this should address everything in the linked bug

[Oliver Chang, 2016-05-23 17:25:14]

https://codereview.chromium.org/2004763002/diff/1/mojo/edk/system/message_pip...
File mojo/edk/system/message_pipe_dispatcher.cc (right):

https://codereview.chromium.org/2004763002/diff/1/mojo/edk/system/message_pip...
mojo/edk/system/message_pipe_dispatcher.cc:295: if (msg->num_payload_bytes() <
data_payload_index + dh.num_bytes)
It's possible this addition can overflow on 32-bit machines right?

https://codereview.chromium.org/2004763002/diff/1/mojo/edk/system/message_pip...
mojo/edk/system/message_pipe_dispatcher.cc:297: if (msg->num_ports() <
port_index + dh.num_ports)
ditto for the these two other checks.

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

On 2016/05/23 at 17:25:14, ochang wrote:
>
https://codereview.chromium.org/2004763002/diff/1/mojo/edk/system/message_pip...
> File mojo/edk/system/message_pipe_dispatcher.cc (right):
> 
>
https://codereview.chromium.org/2004763002/diff/1/mojo/edk/system/message_pip...
> mojo/edk/system/message_pipe_dispatcher.cc:295: if (msg->num_payload_bytes() <
data_payload_index + dh.num_bytes)
> It's possible this addition can overflow on 32-bit machines right?
> 
>
https://codereview.chromium.org/2004763002/diff/1/mojo/edk/system/message_pip...
> mojo/edk/system/message_pipe_dispatcher.cc:297: if (msg->num_ports() <
port_index + dh.num_ports)
> ditto for the these two other checks.

Added overflow checks

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

On 2016/05/23 at 17:25:14, ochang wrote:
>
https://codereview.chromium.org/2004763002/diff/1/mojo/edk/system/message_pip...
> File mojo/edk/system/message_pipe_dispatcher.cc (right):
> 
>
https://codereview.chromium.org/2004763002/diff/1/mojo/edk/system/message_pip...
> mojo/edk/system/message_pipe_dispatcher.cc:295: if (msg->num_payload_bytes() <
data_payload_index + dh.num_bytes)
> It's possible this addition can overflow on 32-bit machines right?
> 
>
https://codereview.chromium.org/2004763002/diff/1/mojo/edk/system/message_pip...
> mojo/edk/system/message_pipe_dispatcher.cc:297: if (msg->num_ports() <
port_index + dh.num_ports)
> ditto for the these two other checks.

Added overflow checks

[Oliver Chang, 2016-05-23 17:50:20]

LGTM, thanks! Might be worth it to start using base::CheckedNumeric<> for things
like this in the future though :)

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

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2016-05-23 18:01:56]

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

[commit-bot: I haz the power, 2016-05-23 18:42:20]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-05-23 18:44:48]

Description was changed from

==========
[mojo-edk] Add some buffer checks and fix UAF on NodeChannel

Adds message sanity checks at the dispatcher layer for
message and data pipes.

Also eliminates potential UAFs on NodeChannel by ensuring
it stays alive through any delegate calls which might have
otherwise deleted it.

BUG=613698
==========

to

==========
[mojo-edk] Add some buffer checks and fix UAF on NodeChannel

Adds message sanity checks at the dispatcher layer for
message and data pipes.

Also eliminates potential UAFs on NodeChannel by ensuring
it stays alive through any delegate calls which might have
otherwise deleted it.

BUG=613698

Committed: https://crrev.com/99a83d5bc1df0c4d6d660dedf45c9b0e14a9df3f
Cr-Commit-Position: refs/heads/master@{#395373}
==========

[commit-bot: I haz the power, 2016-05-23 18:44:49]

Patchset 2 (id:??) landed as
https://crrev.com/99a83d5bc1df0c4d6d660dedf45c9b0e14a9df3f
Cr-Commit-Position: refs/heads/master@{#395373}