Skip to content

Commit

Permalink
[mojo-bindings]: Validate message headers sooner
Browse files Browse the repository at this point in the history
Message header validation has been tied to interface message dispatch,
but not all mojo::Message consumers are interface bindings.

mojo::Connector is a more general-purpose entry point through which
incoming messages are received and transformed into mojo::Message
objects. Blink's MessagePort implementation uses Connector directly to
transmit and receive raw serialized object data.

This change moves MessageHeaderValidator ownership into Connector and
always applies its validation immediately after reading a message from
the pipe, thereby ensuring that all mojo::Message objects used in
production have validated headers before use.

Fixed: 1281908
Change-Id: Ie0e251ab04681a4fd4b849d82c247e0ed800dc04
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3462461
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Ken Rockot <rockot@google.com>
Cr-Commit-Position: refs/heads/main@{#971263}
  • Loading branch information
krockot authored and Chromium LUCI CQ committed Feb 15, 2022
1 parent da2576d commit 8d5bc69
Show file tree
Hide file tree
Showing 4 changed files with 14 additions and 15 deletions.
3 changes: 3 additions & 0 deletions mojo/public/cpp/bindings/connector.h
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#include "base/task/sequenced_task_runner.h"
#include "mojo/public/cpp/bindings/connection_group.h"
#include "mojo/public/cpp/bindings/message.h"
#include "mojo/public/cpp/bindings/message_header_validator.h"
#include "mojo/public/cpp/system/core.h"
#include "mojo/public/cpp/system/handle_signal_tracker.h"
#include "mojo/public/cpp/system/simple_watcher.h"
Expand Down Expand Up @@ -353,6 +354,8 @@ class COMPONENT_EXPORT(MOJO_CPP_BINDINGS) Connector : public MessageReceiver {
// The number of pending tasks for |CallDispatchNextMessageFromPipe|.
size_t num_pending_dispatch_tasks_ = 0;

MessageHeaderValidator header_validator_;

#if defined(ENABLE_IPC_FUZZER)
std::unique_ptr<MessageReceiver> message_dumper_;
#endif
Expand Down
12 changes: 11 additions & 1 deletion mojo/public/cpp/bindings/lib/connector.cc
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
#include "base/rand_util.h"
#include "base/run_loop.h"
#include "base/strings/strcat.h"
#include "base/strings/string_util.h"
#include "base/synchronization/lock.h"
#include "base/task/current_thread.h"
#include "base/threading/sequence_local_storage_slot.h"
Expand Down Expand Up @@ -156,7 +157,11 @@ Connector::Connector(ScopedMessagePipeHandle message_pipe,
force_immediate_dispatch_(!EnableTaskPerMessage()),
outgoing_serialization_mode_(g_default_outgoing_serialization_mode),
incoming_serialization_mode_(g_default_incoming_serialization_mode),
interface_name_(interface_name) {
interface_name_(interface_name),
header_validator_(
base::JoinString({interface_name ? interface_name : "Generic",
"MessageHeaderValidator"},
"")) {
if (config == MULTI_THREADED_SEND)
lock_.emplace();

Expand Down Expand Up @@ -496,6 +501,7 @@ MojoResult Connector::ReadMessage(Message* message) {
return result;

*message = Message::CreateFromMessageHandle(&handle);

if (message->IsNull()) {
// Even if the read was successful, the Message may still be null if there
// was a problem extracting handles from it. We treat this essentially as
Expand All @@ -511,6 +517,10 @@ MojoResult Connector::ReadMessage(Message* message) {
return MOJO_RESULT_ABORTED;
}

if (!header_validator_.Accept(message)) {
return MOJO_RESULT_ABORTED;
}

return MOJO_RESULT_OK;
}

Expand Down
8 changes: 0 additions & 8 deletions mojo/public/cpp/bindings/lib/multiplex_router.cc
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
#include "mojo/public/cpp/bindings/interface_endpoint_controller.h"
#include "mojo/public/cpp/bindings/lib/may_auto_lock.h"
#include "mojo/public/cpp/bindings/lib/message_quota_checker.h"
#include "mojo/public/cpp/bindings/message_header_validator.h"
#include "mojo/public/cpp/bindings/sequence_local_sync_event_watcher.h"

namespace mojo {
Expand Down Expand Up @@ -394,14 +393,7 @@ MultiplexRouter::MultiplexRouter(
if (quota_checker)
connector_.SetMessageQuotaChecker(std::move(quota_checker));

std::unique_ptr<MessageHeaderValidator> header_validator =
std::make_unique<MessageHeaderValidator>();
header_validator_ = header_validator.get();
dispatcher_.SetValidator(std::move(header_validator));

if (primary_interface_name) {
header_validator_->SetDescription(base::JoinString(
{primary_interface_name, "[primary] MessageHeaderValidator"}, " "));
control_message_handler_.SetDescription(base::JoinString(
{primary_interface_name, "[primary] PipeControlMessageHandler"}, " "));
}
Expand Down
6 changes: 0 additions & 6 deletions mojo/public/cpp/bindings/lib/multiplex_router.h
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ class SequencedTaskRunner;
namespace mojo {

class AsyncFlusher;
class MessageHeaderValidator;
class PendingFlush;

namespace internal {
Expand Down Expand Up @@ -303,11 +302,6 @@ class COMPONENT_EXPORT(MOJO_CPP_BINDINGS) MultiplexRouter

scoped_refptr<base::SequencedTaskRunner> task_runner_;

// Owned by |dispatcher_| below.
// `header_validator_` is not a raw_ptr<...> for performance reasons (based on
// analysis of sampling profiler data).
MessageHeaderValidator* header_validator_ = nullptr;

MessageDispatcher dispatcher_;
Connector connector_;

Expand Down

0 comments on commit 8d5bc69

Please sign in to comment.