Skip to content

Commit

Permalink
chore: cherry-pick 62142d222a80 from chromium (electron#33185)
Browse files Browse the repository at this point in the history
* chore: cherry-pick 62142d222a80 from chromium

* chore: update patches

Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com>
Co-authored-by: Electron Bot <electron@github.com>
  • Loading branch information
3 people committed Mar 9, 2022
1 parent 8e51720 commit 0c371e5
Show file tree
Hide file tree
Showing 2 changed files with 152 additions and 0 deletions.
1 change: 1 addition & 0 deletions patches/chromium/.patches
Original file line number Diff line number Diff line change
Expand Up @@ -156,5 +156,6 @@ cherry-pick-be50c60b4225.patch
cherry-pick-ebc188ad769e.patch
cherry-pick-e3805f29fed7.patch
cherry-pick-0081bb347e67.patch
cherry-pick-62142d222a80.patch
cherry-pick-1887414c016d.patch
cherry-pick-6b2643846ae3.patch
151 changes: 151 additions & 0 deletions patches/chromium/cherry-pick-62142d222a80.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,151 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Ken Rockot <rockot@google.com>
Date: Thu, 24 Feb 2022 15:13:13 +0000
Subject: 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
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3462461
Commit-Queue: Ken Rockot <rockot@google.com>
Cr-Original-Commit-Position: refs/heads/main@{#971263}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3483815
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}

diff --git a/mojo/public/cpp/bindings/connector.h b/mojo/public/cpp/bindings/connector.h
index 3975d01a434d6caf229c6d8eaedfe5e2acf684f8..ff17af9b378a1992dbd681e80e957826ebfd83dd 100644
--- a/mojo/public/cpp/bindings/connector.h
+++ b/mojo/public/cpp/bindings/connector.h
@@ -18,6 +18,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"
@@ -345,6 +346,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/mojo/public/cpp/bindings/lib/connector.cc b/mojo/public/cpp/bindings/lib/connector.cc
index 92a9756cbaebe12377e9c9b81467260c73528311..cb08f13b1bfa13a1a79f7f39005f826e51c12a69 100644
--- a/mojo/public/cpp/bindings/lib/connector.cc
+++ b/mojo/public/cpp/bindings/lib/connector.cc
@@ -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"
@@ -152,7 +153,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();

@@ -492,6 +497,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
@@ -507,6 +513,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/mojo/public/cpp/bindings/lib/multiplex_router.cc b/mojo/public/cpp/bindings/lib/multiplex_router.cc
index 605e51344d64eb5ede3ab475bc8833b37a458535..23e3970089e33d07b18d734d4f599c9c9f9e4bc8 100644
--- a/mojo/public/cpp/bindings/lib/multiplex_router.cc
+++ b/mojo/public/cpp/bindings/lib/multiplex_router.cc
@@ -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 {
@@ -389,14 +388,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"}, " "));
}
diff --git a/mojo/public/cpp/bindings/lib/multiplex_router.h b/mojo/public/cpp/bindings/lib/multiplex_router.h
index 3d3bbb16e25b1adc678e536bd977fa22fc478920..3a9f76a57e301aa89d1bdc5aa1000f815793e238 100644
--- a/mojo/public/cpp/bindings/lib/multiplex_router.h
+++ b/mojo/public/cpp/bindings/lib/multiplex_router.h
@@ -38,7 +38,6 @@ class SequencedTaskRunner;
namespace mojo {

class AsyncFlusher;
-class MessageHeaderValidator;
class PendingFlush;

namespace internal {
@@ -301,9 +300,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_;

0 comments on commit 0c371e5

Please sign in to comment.