diff options
author | Ken Rockot <rockot@google.com> | 2022-02-24 15:13:13 +0000 |
---|---|---|
committer | Michael BrĂ¼ning <michael.bruning@qt.io> | 2022-05-03 20:15:37 +0000 |
commit | afe46e6fa7b5dee8f666ef76404bf80fa37a9a56 (patch) | |
tree | a269721f02e82f90225c6e7800ad498db2958ba6 | |
parent | 3a0d560d3c1b8b21afc792dde24fd7aa5ccac31b (diff) | |
download | qtwebengine-chromium-afe46e6fa7b5dee8f666ef76404bf80fa37a9a56.tar.gz |
[Backport] CVE-2022-0797: Out of bounds memory access in Mojo
Cherry-pick of patch originally reviewed on
https://chromium-review.googlesource.com/c/chromium/src/+/3483815:
Validate message headers sooner
M96 merge issues:
- multiplex_router.h: conflict in removed lines because of
differences in comments above header_validator_
- connector.h: conflicting includes
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.
(cherry picked from commit 8d5bc69146505785ce299c490e35e3f3ef19f69c)
Fixed: 1281908
Change-Id: Ie0e251ab04681a4fd4b849d82c247e0ed800dc04
Commit-Queue: Ken Rockot <rockot@google.com>
Cr-Original-Commit-Position: refs/heads/main@{#971263}
Reviewed-by: Victor-Gabriel Savu <vsavu@google.com>
Owners-Override: Victor-Gabriel Savu <vsavu@google.com>
Commit-Queue: Roger Felipe Zanoni da Silva <rzanoni@google.com>
Cr-Commit-Position: refs/branch-heads/4664@{#1505}
Cr-Branched-From: 24dc4ee75e01a29d390d43c9c264372a169273a7-refs/heads/main@{#929512}
Reviewed-by: Michal Klocek <michal.klocek@qt.io>
4 files changed, 12 insertions, 10 deletions
diff --git a/chromium/mojo/public/cpp/bindings/connector.h b/chromium/mojo/public/cpp/bindings/connector.h index 905829cdfb1..e8c8fc5242e 100644 --- a/chromium/mojo/public/cpp/bindings/connector.h +++ b/chromium/mojo/public/cpp/bindings/connector.h @@ -19,6 +19,7 @@ #include "base/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/bindings/sync_handle_watcher.h" #include "mojo/public/cpp/system/core.h" #include "mojo/public/cpp/system/handle_signal_tracker.h" @@ -319,6 +320,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 diff --git a/chromium/mojo/public/cpp/bindings/lib/connector.cc b/chromium/mojo/public/cpp/bindings/lib/connector.cc index 2766f8d65c3..59ece29ab62 100644 --- a/chromium/mojo/public/cpp/bindings/lib/connector.cc +++ b/chromium/mojo/public/cpp/bindings/lib/connector.cc @@ -17,6 +17,7 @@ #include "base/no_destructor.h" #include "base/rand_util.h" #include "base/run_loop.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" @@ -154,6 +155,10 @@ Connector::Connector(ScopedMessagePipeHandle message_pipe, outgoing_serialization_mode_(g_default_outgoing_serialization_mode), incoming_serialization_mode_(g_default_incoming_serialization_mode), interface_name_(interface_name), + header_validator_( + base::JoinString({interface_name ? interface_name : "Generic", + "MessageHeaderValidator"}, + "")), nesting_observer_(RunLoopNestingObserver::GetForThread()) { if (config == MULTI_THREADED_SEND) lock_.emplace(); @@ -469,6 +474,10 @@ MojoResult Connector::ReadMessage(Message* message) { return MOJO_RESULT_ABORTED; } + if (!header_validator_.Accept(message)) { + return MOJO_RESULT_ABORTED; + } + return MOJO_RESULT_OK; } diff --git a/chromium/mojo/public/cpp/bindings/lib/multiplex_router.cc b/chromium/mojo/public/cpp/bindings/lib/multiplex_router.cc index 5f2e8141fe1..fa04dc6c680 100644 --- a/chromium/mojo/public/cpp/bindings/lib/multiplex_router.cc +++ b/chromium/mojo/public/cpp/bindings/lib/multiplex_router.cc @@ -351,15 +351,8 @@ 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) { DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); - header_validator_->SetDescription(base::JoinString( - {primary_interface_name, "[primary] MessageHeaderValidator"}, " ")); control_message_handler_.SetDescription(base::JoinString( {primary_interface_name, "[primary] PipeControlMessageHandler"}, " ")); } diff --git a/chromium/mojo/public/cpp/bindings/lib/multiplex_router.h b/chromium/mojo/public/cpp/bindings/lib/multiplex_router.h index 49c6785ca05..c233e43c770 100644 --- a/chromium/mojo/public/cpp/bindings/lib/multiplex_router.h +++ b/chromium/mojo/public/cpp/bindings/lib/multiplex_router.h @@ -259,9 +259,6 @@ class COMPONENT_EXPORT(MOJO_CPP_BINDINGS) MultiplexRouter scoped_refptr<base::SequencedTaskRunner> task_runner_; - // Owned by |dispatcher_| below. - MessageHeaderValidator* header_validator_ = nullptr; - MessageDispatcher dispatcher_; Connector connector_; |