New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 607293 link

Starred by 6 users
Status: Fixed
Owner:
█ roc...@chromium.org
please use my google.com address
Closed: Jun 2016
Cc:
amistry@chromium.org
█ nick@chromium.org
█ roc...@chromium.org
█ yzshen@chromium.org
jsb...@chromium.org
creis@chromium.org
dcheng@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug



Sign in to add a comment

Need an IPC::Listener::OnBadMessageReceived equivalent for Mojo

Project Member Reported by jam@chromium.org, Apr 27 2016 
With Chrome IPC, if the browser receives a malformed IPC message it will kill the renderer automatically. This works for all IPCs with no setup needed; i.e. because there's one IPC::Channel and the process host object overrides OnBadMessageReceived.

We need something similar for Mojo, so that if any message pipe sees a malformed request the process host object for that process is notified so that it can kill it. This needs to work once, i.e. not require any setup per binding.

Comment 1 by jam@chromium.org, Apr 27 2016

Cc: dcheng@chromium.org

Comment 2 by yzshen@chromium.org, Apr 27 2016

Cc: jsb...@chromium.org
 Issue 583362  has been merged into this issue.

Comment 3 by creis@chromium.org, Jun 2 2016

Cc: nick@chromium.org creis@chromium.org
Yes, there's extensive support for renderer kills after untrustworthy messages in the various BadMessage enums, and we're generating some very useful crash reports to go along with them in issue 473370.  This will be important to preserve.
Project Member

Comment 5 by bugdroid1@chromium.org, Jun 11 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b46bf91c3d28d5da2fcbcde544d5aaf8c6fde7c6

commit b46bf91c3d28d5da2fcbcde544d5aaf8c6fde7c6
Author: rockot <rockot@chromium.org>
Date: Sat Jun 11 23:18:16 2016

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 

Review-Url: https://codereview.chromium.org/2043713004
Cr-Commit-Position: refs/heads/master@{#399382}

[modify] https://crrev.com/b46bf91c3d28d5da2fcbcde544d5aaf8c6fde7c6/mojo/edk/embedder/embedder.cc
[modify] https://crrev.com/b46bf91c3d28d5da2fcbcde544d5aaf8c6fde7c6/mojo/edk/embedder/embedder.h
[modify] https://crrev.com/b46bf91c3d28d5da2fcbcde544d5aaf8c6fde7c6/mojo/edk/embedder/entrypoints.cc
[modify] https://crrev.com/b46bf91c3d28d5da2fcbcde544d5aaf8c6fde7c6/mojo/edk/system/core.cc
[modify] https://crrev.com/b46bf91c3d28d5da2fcbcde544d5aaf8c6fde7c6/mojo/edk/system/core.h
[modify] https://crrev.com/b46bf91c3d28d5da2fcbcde544d5aaf8c6fde7c6/mojo/edk/system/message_for_transit.h
[modify] https://crrev.com/b46bf91c3d28d5da2fcbcde544d5aaf8c6fde7c6/mojo/edk/system/multiprocess_message_pipe_unittest.cc
[modify] https://crrev.com/b46bf91c3d28d5da2fcbcde544d5aaf8c6fde7c6/mojo/edk/system/node_channel.cc
[modify] https://crrev.com/b46bf91c3d28d5da2fcbcde544d5aaf8c6fde7c6/mojo/edk/system/node_channel.h
[modify] https://crrev.com/b46bf91c3d28d5da2fcbcde544d5aaf8c6fde7c6/mojo/edk/system/node_controller.cc
[modify] https://crrev.com/b46bf91c3d28d5da2fcbcde544d5aaf8c6fde7c6/mojo/edk/system/node_controller.h
[modify] https://crrev.com/b46bf91c3d28d5da2fcbcde544d5aaf8c6fde7c6/mojo/edk/system/ports_message.h
[modify] https://crrev.com/b46bf91c3d28d5da2fcbcde544d5aaf8c6fde7c6/mojo/edk/test/mojo_test_base.cc
[modify] https://crrev.com/b46bf91c3d28d5da2fcbcde544d5aaf8c6fde7c6/mojo/edk/test/mojo_test_base.h
[modify] https://crrev.com/b46bf91c3d28d5da2fcbcde544d5aaf8c6fde7c6/mojo/edk/test/multiprocess_test_helper.cc
[modify] https://crrev.com/b46bf91c3d28d5da2fcbcde544d5aaf8c6fde7c6/mojo/edk/test/multiprocess_test_helper.h
[modify] https://crrev.com/b46bf91c3d28d5da2fcbcde544d5aaf8c6fde7c6/mojo/public/c/system/message_pipe.h
[modify] https://crrev.com/b46bf91c3d28d5da2fcbcde544d5aaf8c6fde7c6/mojo/public/c/system/thunks.cc
[modify] https://crrev.com/b46bf91c3d28d5da2fcbcde544d5aaf8c6fde7c6/mojo/public/c/system/thunks.h
[modify] https://crrev.com/b46bf91c3d28d5da2fcbcde544d5aaf8c6fde7c6/mojo/public/cpp/system/message.h
Project Member

Comment 6 by bugdroid1@chromium.org, Jun 15 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b46bf91c3d28d5da2fcbcde544d5aaf8c6fde7c6

commit b46bf91c3d28d5da2fcbcde544d5aaf8c6fde7c6
Author: rockot <rockot@chromium.org>
Date: Sat Jun 11 23:18:16 2016

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 

Review-Url: https://codereview.chromium.org/2043713004
Cr-Commit-Position: refs/heads/master@{#399382}

[modify] https://crrev.com/b46bf91c3d28d5da2fcbcde544d5aaf8c6fde7c6/mojo/edk/embedder/embedder.cc
[modify] https://crrev.com/b46bf91c3d28d5da2fcbcde544d5aaf8c6fde7c6/mojo/edk/embedder/embedder.h
[modify] https://crrev.com/b46bf91c3d28d5da2fcbcde544d5aaf8c6fde7c6/mojo/edk/embedder/entrypoints.cc
[modify] https://crrev.com/b46bf91c3d28d5da2fcbcde544d5aaf8c6fde7c6/mojo/edk/system/core.cc
[modify] https://crrev.com/b46bf91c3d28d5da2fcbcde544d5aaf8c6fde7c6/mojo/edk/system/core.h
[modify] https://crrev.com/b46bf91c3d28d5da2fcbcde544d5aaf8c6fde7c6/mojo/edk/system/message_for_transit.h
[modify] https://crrev.com/b46bf91c3d28d5da2fcbcde544d5aaf8c6fde7c6/mojo/edk/system/multiprocess_message_pipe_unittest.cc
[modify] https://crrev.com/b46bf91c3d28d5da2fcbcde544d5aaf8c6fde7c6/mojo/edk/system/node_channel.cc
[modify] https://crrev.com/b46bf91c3d28d5da2fcbcde544d5aaf8c6fde7c6/mojo/edk/system/node_channel.h
[modify] https://crrev.com/b46bf91c3d28d5da2fcbcde544d5aaf8c6fde7c6/mojo/edk/system/node_controller.cc
[modify] https://crrev.com/b46bf91c3d28d5da2fcbcde544d5aaf8c6fde7c6/mojo/edk/system/node_controller.h
[modify] https://crrev.com/b46bf91c3d28d5da2fcbcde544d5aaf8c6fde7c6/mojo/edk/system/ports_message.h
[modify] https://crrev.com/b46bf91c3d28d5da2fcbcde544d5aaf8c6fde7c6/mojo/edk/test/mojo_test_base.cc
[modify] https://crrev.com/b46bf91c3d28d5da2fcbcde544d5aaf8c6fde7c6/mojo/edk/test/mojo_test_base.h
[modify] https://crrev.com/b46bf91c3d28d5da2fcbcde544d5aaf8c6fde7c6/mojo/edk/test/multiprocess_test_helper.cc
[modify] https://crrev.com/b46bf91c3d28d5da2fcbcde544d5aaf8c6fde7c6/mojo/edk/test/multiprocess_test_helper.h
[modify] https://crrev.com/b46bf91c3d28d5da2fcbcde544d5aaf8c6fde7c6/mojo/public/c/system/message_pipe.h
[modify] https://crrev.com/b46bf91c3d28d5da2fcbcde544d5aaf8c6fde7c6/mojo/public/c/system/thunks.cc
[modify] https://crrev.com/b46bf91c3d28d5da2fcbcde544d5aaf8c6fde7c6/mojo/public/c/system/thunks.h
[modify] https://crrev.com/b46bf91c3d28d5da2fcbcde544d5aaf8c6fde7c6/mojo/public/cpp/system/message.h
Project Member

Comment 7 by bugdroid1@chromium.org, Jun 16 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8a88e80c4bea32f83842729d33db85a36ef5b3fa

commit 8a88e80c4bea32f83842729d33db85a36ef5b3fa
Author: rockot <rockot@chromium.org>
Date: Thu Jun 16 02:30:30 2016

Mojo: Report bindings validation errors via MojoNotifyBadMessage

Renames BoundsChecker to ValidationContext.

Plumbs a ValidationContext through to any code which might
report a message validation error. Calls NotifyBadMessage from
ReportValidationError so that all such errors may be bubbled
up to the embedder through the EDK.

R=yzshen@chromium.org
BUG= 607293 

Review-Url: https://codereview.chromium.org/2064903002
Cr-Commit-Position: refs/heads/master@{#400075}

[modify] https://crrev.com/8a88e80c4bea32f83842729d33db85a36ef5b3fa/mojo/mojo_edk_tests.gyp
[modify] https://crrev.com/8a88e80c4bea32f83842729d33db85a36ef5b3fa/mojo/mojo_public.gypi
[modify] https://crrev.com/8a88e80c4bea32f83842729d33db85a36ef5b3fa/mojo/public/cpp/bindings/BUILD.gn
[modify] https://crrev.com/8a88e80c4bea32f83842729d33db85a36ef5b3fa/mojo/public/cpp/bindings/lib/array_internal.h
[modify] https://crrev.com/8a88e80c4bea32f83842729d33db85a36ef5b3fa/mojo/public/cpp/bindings/lib/binding_state.h
[modify] https://crrev.com/8a88e80c4bea32f83842729d33db85a36ef5b3fa/mojo/public/cpp/bindings/lib/filter_chain.h
[modify] https://crrev.com/8a88e80c4bea32f83842729d33db85a36ef5b3fa/mojo/public/cpp/bindings/lib/interface_ptr_state.h
[modify] https://crrev.com/8a88e80c4bea32f83842729d33db85a36ef5b3fa/mojo/public/cpp/bindings/lib/map_data_internal.h
[modify] https://crrev.com/8a88e80c4bea32f83842729d33db85a36ef5b3fa/mojo/public/cpp/bindings/lib/message.cc
[modify] https://crrev.com/8a88e80c4bea32f83842729d33db85a36ef5b3fa/mojo/public/cpp/bindings/lib/message_buffer.cc
[modify] https://crrev.com/8a88e80c4bea32f83842729d33db85a36ef5b3fa/mojo/public/cpp/bindings/lib/message_buffer.h
[modify] https://crrev.com/8a88e80c4bea32f83842729d33db85a36ef5b3fa/mojo/public/cpp/bindings/lib/message_header_validator.cc
[modify] https://crrev.com/8a88e80c4bea32f83842729d33db85a36ef5b3fa/mojo/public/cpp/bindings/lib/message_header_validator.h
[modify] https://crrev.com/8a88e80c4bea32f83842729d33db85a36ef5b3fa/mojo/public/cpp/bindings/lib/multiplex_router.cc
[modify] https://crrev.com/8a88e80c4bea32f83842729d33db85a36ef5b3fa/mojo/public/cpp/bindings/lib/multiplex_router.h
[modify] https://crrev.com/8a88e80c4bea32f83842729d33db85a36ef5b3fa/mojo/public/cpp/bindings/lib/native_enum_data.h
[modify] https://crrev.com/8a88e80c4bea32f83842729d33db85a36ef5b3fa/mojo/public/cpp/bindings/lib/native_struct_data.cc
[modify] https://crrev.com/8a88e80c4bea32f83842729d33db85a36ef5b3fa/mojo/public/cpp/bindings/lib/native_struct_data.h
[modify] https://crrev.com/8a88e80c4bea32f83842729d33db85a36ef5b3fa/mojo/public/cpp/bindings/lib/pipe_control_message_handler.cc
[modify] https://crrev.com/8a88e80c4bea32f83842729d33db85a36ef5b3fa/mojo/public/cpp/bindings/lib/pipe_control_message_handler.h
[modify] https://crrev.com/8a88e80c4bea32f83842729d33db85a36ef5b3fa/mojo/public/cpp/bindings/lib/validate_params.h
[rename] https://crrev.com/8a88e80c4bea32f83842729d33db85a36ef5b3fa/mojo/public/cpp/bindings/lib/validation_context.cc
[rename] https://crrev.com/8a88e80c4bea32f83842729d33db85a36ef5b3fa/mojo/public/cpp/bindings/lib/validation_context.h
[modify] https://crrev.com/8a88e80c4bea32f83842729d33db85a36ef5b3fa/mojo/public/cpp/bindings/lib/validation_errors.cc
[modify] https://crrev.com/8a88e80c4bea32f83842729d33db85a36ef5b3fa/mojo/public/cpp/bindings/lib/validation_errors.h
[modify] https://crrev.com/8a88e80c4bea32f83842729d33db85a36ef5b3fa/mojo/public/cpp/bindings/lib/validation_util.cc
[modify] https://crrev.com/8a88e80c4bea32f83842729d33db85a36ef5b3fa/mojo/public/cpp/bindings/lib/validation_util.h
[modify] https://crrev.com/8a88e80c4bea32f83842729d33db85a36ef5b3fa/mojo/public/cpp/bindings/message.h
[modify] https://crrev.com/8a88e80c4bea32f83842729d33db85a36ef5b3fa/mojo/public/cpp/bindings/tests/BUILD.gn
[delete] https://crrev.com/3a276ce1616a0ae9202677aeb3d3b1652228527f/mojo/public/cpp/bindings/tests/bounds_checker_unittest.cc
[modify] https://crrev.com/8a88e80c4bea32f83842729d33db85a36ef5b3fa/mojo/public/cpp/bindings/tests/union_unittest.cc
[add] https://crrev.com/8a88e80c4bea32f83842729d33db85a36ef5b3fa/mojo/public/cpp/bindings/tests/validation_context_unittest.cc
[modify] https://crrev.com/8a88e80c4bea32f83842729d33db85a36ef5b3fa/mojo/public/tools/bindings/generators/cpp_templates/enum_macros.tmpl
[modify] https://crrev.com/8a88e80c4bea32f83842729d33db85a36ef5b3fa/mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl
[modify] https://crrev.com/8a88e80c4bea32f83842729d33db85a36ef5b3fa/mojo/public/tools/bindings/generators/cpp_templates/module-internal.h.tmpl
[modify] https://crrev.com/8a88e80c4bea32f83842729d33db85a36ef5b3fa/mojo/public/tools/bindings/generators/cpp_templates/module.cc.tmpl
[modify] https://crrev.com/8a88e80c4bea32f83842729d33db85a36ef5b3fa/mojo/public/tools/bindings/generators/cpp_templates/struct_declaration.tmpl
[modify] https://crrev.com/8a88e80c4bea32f83842729d33db85a36ef5b3fa/mojo/public/tools/bindings/generators/cpp_templates/struct_definition.tmpl
[modify] https://crrev.com/8a88e80c4bea32f83842729d33db85a36ef5b3fa/mojo/public/tools/bindings/generators/cpp_templates/union_declaration.tmpl
[modify] https://crrev.com/8a88e80c4bea32f83842729d33db85a36ef5b3fa/mojo/public/tools/bindings/generators/cpp_templates/union_definition.tmpl
[modify] https://crrev.com/8a88e80c4bea32f83842729d33db85a36ef5b3fa/mojo/public/tools/bindings/generators/cpp_templates/validation_macros.tmpl
Project Member

Comment 8 by bugdroid1@chromium.org, Jun 16 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/229fb28e6c8dc41792ea517a401529c14214ea3c

commit 229fb28e6c8dc41792ea517a401529c14214ea3c
Author: rockot <rockot@chromium.org>
Date: Thu Jun 16 04:46:16 2016

Kill child processes on bad Mojo messages

Hooks RenderProcessHostImpl and BrowserChildProcessHostImpl
up to the new EDK API for process error notifications. If
any Mojo bindings receive a malformed message directly from
a child process, this will result in the child's termination.

BUG= 607293 

Review-Url: https://codereview.chromium.org/2054303002
Cr-Commit-Position: refs/heads/master@{#400085}

[modify] https://crrev.com/229fb28e6c8dc41792ea517a401529c14214ea3c/content/browser/bad_message.h
[modify] https://crrev.com/229fb28e6c8dc41792ea517a401529c14214ea3c/content/browser/browser_child_process_host_impl.cc
[modify] https://crrev.com/229fb28e6c8dc41792ea517a401529c14214ea3c/content/browser/browser_child_process_host_impl.h
[modify] https://crrev.com/229fb28e6c8dc41792ea517a401529c14214ea3c/content/browser/child_process_launcher.cc
[modify] https://crrev.com/229fb28e6c8dc41792ea517a401529c14214ea3c/content/browser/child_process_launcher.h
[modify] https://crrev.com/229fb28e6c8dc41792ea517a401529c14214ea3c/content/browser/renderer_host/render_process_host_browsertest.cc
[modify] https://crrev.com/229fb28e6c8dc41792ea517a401529c14214ea3c/content/browser/renderer_host/render_process_host_impl.cc
[modify] https://crrev.com/229fb28e6c8dc41792ea517a401529c14214ea3c/content/browser/renderer_host/render_process_host_impl.h
[modify] https://crrev.com/229fb28e6c8dc41792ea517a401529c14214ea3c/content/public/renderer/content_renderer_client.h
[modify] https://crrev.com/229fb28e6c8dc41792ea517a401529c14214ea3c/content/renderer/render_thread_impl.cc
[modify] https://crrev.com/229fb28e6c8dc41792ea517a401529c14214ea3c/content/shell/renderer/shell_content_renderer_client.cc
[modify] https://crrev.com/229fb28e6c8dc41792ea517a401529c14214ea3c/content/shell/renderer/shell_content_renderer_client.h
[modify] https://crrev.com/229fb28e6c8dc41792ea517a401529c14214ea3c/tools/metrics/histograms/histograms.xml

Comment 9 by jam@chromium.org, Jun 21 2016

Owner: roc...@chromium.org
Status: Fixed (was: Untriaged)

Comment 10 by finnur@chromium.org, Nov 21 

Does the cheat sheet need to be updated?
https://www.chromium.org/developers/design-documents/mojo/chrome-ipc-to-mojo-ipc-cheat-sheet

(it says processed as not kiled and refers to this bug as tracker)

Comment 11 by finnur@chromium.org, Nov 21 

This was supposed to read: "processes are not killed"

Comment 12 by rockot@google.com, Nov 26 

I haven't looked around in depth but I'm pretty sure that page should just be deleted. It should be supplanted entirely by existing docs in the repo, either ipc/README.md, or stuff under mojo/.
►

Sign in to add a comment