Mojo: Add NotifyBadMessage API

Mojo: Add NotifyBadMessage API

Adds a new public API for rejecting user messages read from
a message pipe. The EDK can in some simple cases (e.g. messages
from a direct child to its parent process) determine the source
process of the bad message and notify the embedder via a callback
API.

BUG=607293
Committed: https://crrev.com/b46bf91c3d28d5da2fcbcde544d5aaf8c6fde7c6
Cr-Commit-Position: refs/heads/master@{#399382}

[Ken Rockot(use gerrit already), 2016-06-07 17:51:20]

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

[Anand Mistry (off Chromium), 2016-06-08 13:14:09]

I've taken a first glance, but need a little more time to think about this. But
I have a couple of high-level questions:
1. The error callback has no parameters. Should it pass some information about
the error? Maybe handle, or a void* passed into the bad message function? I'm
just wondering if base::Closure is sufficient.
2. What are your thoughts about signalling a bad message on a message  versus a
pipe handle? Although the message is bad, it is received from a potentially
compromised pipe and it's the pipe which traces back to the source.

[Ken Rockot(use gerrit already), 2016-06-08 14:20:31]

On Jun 8, 2016 6:14 AM, <amistry@chromium.org> wrote:
>
> I've taken a first glance, but need a little more time to think about
this. But
> I have a couple of high-level questions:
> 1. The error callback has no parameters. Should it pass some information
about
> the error? Maybe handle, or a void* passed into the bad message function?
I'm
> just wondering if base::Closure is sufficient.

It's sufficient for now. We can change it if we need to, but I currently
have no use for more information in order to fix the bug.

> 2. What are your thoughts about signalling a bad message on a message
versus a
> pipe handle? Although the message is bad, it is received from a
potentially
> compromised pipe and it's the pipe which traces back to the source.

I started with that. Problem is you can't know which node the bad message
came from if you're just given a pipe handle. The best you could do is
track source nodes seen on messages that have arrived, because handles are
supposed to be thread safe and may have multiple readers. It can't be as
precise in any case.

>
> https://codereview.chromium.org/2043713004/

-- 
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.

[Anand Mistry (off Chromium), 2016-06-10 10:28:20]

lgtm

https://codereview.chromium.org/2043713004/diff/40001/mojo/edk/system/node_co...
File mojo/edk/system/node_controller.cc (right):

https://codereview.chromium.org/2043713004/diff/40001/mojo/edk/system/node_co...
mojo/edk/system/node_controller.cc:317: peer->NotifyBadMessage();
Check peer != nullptr. The peer could have closed (i.e. channel error) in the
time between receiving the message and processing it.

https://codereview.chromium.org/2043713004/diff/40001/mojo/public/c/system/me...
File mojo/public/c/system/message_pipe.h (right):

https://codereview.chromium.org/2043713004/diff/40001/mojo/public/c/system/me...
mojo/public/c/system/message_pipe.h:327: //   |MOJO_RESULT_INVALID_ARGUMENT| if
|message| is not a valid message.
Add a note that the bad message callback may not be called if the source channel
has been shut down for any reason.

[Ken Rockot(use gerrit already), 2016-06-10 17:32:26]

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

[Ken Rockot(use gerrit already), 2016-06-10 17:32:27]

Yuzhu, could you please take a look at the bindings changes? After talking with
John offline I think a good place to start is to simply plumb through the name
of the offending interface in these notifications. In the future we could
explore options for generating more specific error messages based on which
message failed validation, or which validation step failed. WDYT?

[yzshen1, 2016-06-11 00:11:00]

https://codereview.chromium.org/2043713004/diff/120001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/connector.cc (right):

https://codereview.chromium.org/2043713004/diff/120001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/connector.cc:272:
message.NotifyBadMessage(interface_name_);
For multiplex router, we may get here because of message header validation
failure. In that case, it can be any associated interface, not necessarily the
master interface. So the interface name is misleading.

Besides, there are places that this CL doesn't report bad messages:
- in multiplex_router.cc, pipe control message validation failure.
- in router.cc, we may delay processing of async messages, although the messages
have passed default validation, it hasn't done custom validation (enforced by
users' StructTraits, etc.) yet. So when the message is actually dispatched, it
may still fail.

One way that may be easier to catch all cases, is to check the result value here
(the synchronous case) plus Connector::RaiseError (the async case). And we don't
need to worry about inserting NotifyBadMessage at high level.

If we go down this direction, we will need to add parameter to RaiseError to
distinguish bad message from other reasons.

https://codereview.chromium.org/2043713004/diff/120001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/interface_endpoint_client.cc (right):

https://codereview.chromium.org/2043713004/diff/120001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/interface_endpoint_client.cc:260:
message->NotifyBadMessage(interface_name_);
If you think it is useful, we can also record the message type (which is the
ordinal number of the method).

[Ken Rockot(use gerrit already), 2016-06-11 21:44:05]

Description was changed from

==========
Mojo: Add NotifyBadMessage API

Adds a new public API for rejecting user messages read from
a message pipe. The EDK can in some simple cases (e.g. messages
from a direct child to its parent process) determine the source
process of the bad message and notify the embedder via a callback
API.

Changes the bindings to notify the EDK when a message has been
rejected due to validation failure.

A follow-up patch can hook this up to content's process launch
code to allow for killing child processes which send malformed
messages.

BUG=607293
==========

to

==========
Mojo: Add NotifyBadMessage API

Adds a new public API for rejecting user messages read from
a message pipe. The EDK can in some simple cases (e.g. messages
from a direct child to its parent process) determine the source
process of the bad message and notify the embedder via a callback
API.

BUG=607293
==========

[Ken Rockot(use gerrit already), 2016-06-11 21:48:30]

https://codereview.chromium.org/2043713004/diff/120001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/connector.cc (right):

https://codereview.chromium.org/2043713004/diff/120001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/connector.cc:272:
message.NotifyBadMessage(interface_name_);
On 2016/06/11 at 00:10:59, yzshen1 wrote:
> For multiplex router, we may get here because of message header validation
failure. In that case, it can be any associated interface, not necessarily the
master interface. So the interface name is misleading.
> 
> Besides, there are places that this CL doesn't report bad messages:
> - in multiplex_router.cc, pipe control message validation failure.
> - in router.cc, we may delay processing of async messages, although the
messages have passed default validation, it hasn't done custom validation
(enforced by users' StructTraits, etc.) yet. So when the message is actually
dispatched, it may still fail.
> 
> One way that may be easier to catch all cases, is to check the result value
here (the synchronous case) plus Connector::RaiseError (the async case). And we
don't need to worry about inserting NotifyBadMessage at high level.
> 
> If we go down this direction, we will need to add parameter to RaiseError to
distinguish bad message from other reasons.

Ah right. It's a bit more complicated than this too because MessageFilters can
reject along the way. Given the additional complexity required to get this right
in the bindings, I've removed all bindings changes from this CL and will send
you a follow-up CL with those changes. Thanks!

[Ken Rockot(use gerrit already), 2016-06-11 21:48:33]

The CQ bit was checked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2016-06-11 21:48:33]

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

[commit-bot: I haz the power, 2016-06-11 21:48:40]

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

[commit-bot: I haz the power, 2016-06-11 23:18:29]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-06-11 23:18:31]

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-11 23:20:18]

Description was changed from

==========
Mojo: Add NotifyBadMessage API

Adds a new public API for rejecting user messages read from
a message pipe. The EDK can in some simple cases (e.g. messages
from a direct child to its parent process) determine the source
process of the bad message and notify the embedder via a callback
API.

BUG=607293
==========

to

==========
Mojo: Add NotifyBadMessage API

Adds a new public API for rejecting user messages read from
a message pipe. The EDK can in some simple cases (e.g. messages
from a direct child to its parent process) determine the source
process of the bad message and notify the embedder via a callback
API.

BUG=607293

Committed: https://crrev.com/b46bf91c3d28d5da2fcbcde544d5aaf8c6fde7c6
Cr-Commit-Position: refs/heads/master@{#399382}
==========

[commit-bot: I haz the power, 2016-06-11 23:20:20]

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