Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: cherry-pick 1536a564d959 from chromium #28812

Merged
merged 3 commits into from Apr 26, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions patches/chromium/.patches
Expand Up @@ -113,4 +113,5 @@ word_break_between_space_and_alphanumeric.patch
moves_background_color_setter_of_webview_to_blinks_webprefs_logic.patch
blink_wasm_eval_csp.patch
cherry-pick-162efe98330e.patch
cherry-pick-1536a564d959.patch
cherry-pick-e4abe032f3ad.patch
99 changes: 99 additions & 0 deletions patches/chromium/cherry-pick-1536a564d959.patch
@@ -0,0 +1,99 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Ken Rockot <rockot@google.com>
Date: Thu, 15 Apr 2021 18:14:57 +0000
Subject: Mojo: Properly validate broadcast events

This corrects broadcast event deserialization by adding a missing
validation step when decoding the outer message header.

(cherry picked from commit 6740adb28374ddeee13febfd5e5d20cb8a365979)

Fixed: 1195308
Change-Id: Ia67a20e48614e7ef00b1b32f7f4e5f20235be310
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2808678
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Ken Rockot <rockot@google.com>
Cr-Original-Commit-Position: refs/heads/master@{#870238}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2827760
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Auto-Submit: Ken Rockot <rockot@google.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/branch-heads/4430@{#1290}
Cr-Branched-From: e5ce7dc4f7518237b3d9bb93cccca35d25216cbe-refs/heads/master@{#857950}

diff --git a/mojo/core/node_channel.cc b/mojo/core/node_channel.cc
index c48fb573fea961aa4125c25f065a5177e26b7732..31da822da87abbfa0e2fb416e62e8009d028bde1 100644
--- a/mojo/core/node_channel.cc
+++ b/mojo/core/node_channel.cc
@@ -251,13 +251,16 @@ Channel::MessagePtr NodeChannel::CreateEventMessage(size_t capacity,
}

// static
-void NodeChannel::GetEventMessageData(Channel::Message* message,
+bool NodeChannel::GetEventMessageData(Channel::Message& message,
void** data,
size_t* num_data_bytes) {
- // NOTE: OnChannelMessage guarantees that we never accept a Channel::Message
- // with a payload of fewer than |sizeof(Header)| bytes.
- *data = reinterpret_cast<Header*>(message->mutable_payload()) + 1;
- *num_data_bytes = message->payload_size() - sizeof(Header);
+ // NOTE: Callers must guarantee that the payload in `message` must be at least
+ // large enough to hold a Header.
+ if (message.payload_size() < sizeof(Header))
+ return false;
+ *data = reinterpret_cast<Header*>(message.mutable_payload()) + 1;
+ *num_data_bytes = message.payload_size() - sizeof(Header);
+ return true;
}

void NodeChannel::Start() {
diff --git a/mojo/core/node_channel.h b/mojo/core/node_channel.h
index 74306a56359cdc44b79797e6cc3c134b8029d69e..d2a273811c6db1ad8a37bd69fa81eb99d7ba4177 100644
--- a/mojo/core/node_channel.h
+++ b/mojo/core/node_channel.h
@@ -90,7 +90,9 @@ class MOJO_SYSTEM_IMPL_EXPORT NodeChannel
void** payload,
size_t num_handles);

- static void GetEventMessageData(Channel::Message* message,
+ // Retrieves address and size of an Event message's underlying message data.
+ // Returns `false` if the message is not a valid Event message.
+ static bool GetEventMessageData(Channel::Message& message,
void** data,
size_t* num_data_bytes);

diff --git a/mojo/core/node_controller.cc b/mojo/core/node_controller.cc
index 86b397df1431b2e83ccfceafe0af99e2f6d38c9e..351a34a40f0613ce167fd8158f5e3f494e95ce5e 100644
--- a/mojo/core/node_controller.cc
+++ b/mojo/core/node_controller.cc
@@ -76,7 +76,9 @@ ports::ScopedEvent DeserializeEventMessage(
Channel::MessagePtr channel_message) {
void* data;
size_t size;
- NodeChannel::GetEventMessageData(channel_message.get(), &data, &size);
+ bool valid = NodeChannel::GetEventMessageData(*channel_message, &data, &size);
+ if (!valid)
+ return nullptr;
auto event = ports::Event::Deserialize(data, size);
if (!event)
return nullptr;
diff --git a/mojo/core/user_message_impl.cc b/mojo/core/user_message_impl.cc
index 1f3f30abb4bbaca15513730691a0d52dfaebe0d7..f96c92077ee19dab8676e120684c6ac2f224358f 100644
--- a/mojo/core/user_message_impl.cc
+++ b/mojo/core/user_message_impl.cc
@@ -417,7 +417,14 @@ Channel::MessagePtr UserMessageImpl::FinalizeEventMessage(
if (channel_message) {
void* data;
size_t size;
- NodeChannel::GetEventMessageData(channel_message.get(), &data, &size);
+ // The `channel_message` must either be produced locally or must have
+ // already been validated by the caller, as is done for example by
+ // NodeController::DeserializeEventMessage before
+ // NodeController::OnBroadcast re-serializes each copy of the message it
+ // received.
+ bool result =
+ NodeChannel::GetEventMessageData(*channel_message, &data, &size);
+ DCHECK(result);
message_event->Serialize(data);
}