From a775361f525f4a2054eec3c75be57bb0aa966356 Mon Sep 17 00:00:00 2001 From: Ken Rockot Date: Wed, 31 Mar 2021 18:44:06 +0000 Subject: [Backport] CVE-2021-21198: Out of bounds read in IPC MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Manual backport of patch originally reviewed on https://chromium-review.googlesource.com/c/chromium/src/+/2779918: Don't use BigBuffer for IPC::Message transport M86 merge conflicts and resolution: * ipc/ipc_message_pipe_reader.cc Fixed extra include. (cherry picked from commit 85bd7c88523545ab0e497d5e7b3e929793813358) (cherry picked from commit fad3b9ffe7c7ff82909d911c573bd185aa3b3b50) Fixed: 1184399 Change-Id: Iddd91ae8d7ae63022b61c96239f5e39261dfb735 Commit-Queue: Ken Rockot Reviewed-by: Daniel Cheng Cr-Original-Original-Commit-Position: refs/heads/master@{#860010} Auto-Submit: Ken Rockot Reviewed-by: Adrian Taylor Reviewed-by: Alex Gough Commit-Queue: Alex Gough Cr-Original-Commit-Position: refs/branch-heads/4389@{#1597} Cr-Original-Branched-From: 9251c5db2b6d5a59fe4eac7aafa5fed37c139bb7-refs/heads/master@{#843830} Reviewed-by: Victor-Gabriel Savu Reviewed-by: Artem Sumaneev Reviewed-by: Ken Rockot Auto-Submit: Artem Sumaneev Commit-Queue: Artem Sumaneev Cr-Commit-Position: refs/branch-heads/4240@{#1587} Cr-Branched-From: f297677702651916bbf65e59c0d4bbd4ce57d1ee-refs/heads/master@{#800218} Reviewed-by: Jüri Valdmann --- chromium/ipc/BUILD.gn | 5 +---- chromium/ipc/ipc.mojom | 4 ++-- chromium/ipc/ipc_message_pipe_reader.cc | 10 +++++++--- chromium/ipc/message.typemap | 5 +---- chromium/ipc/message_mojom_traits.cc | 16 +++++++--------- chromium/ipc/message_mojom_traits.h | 4 ++-- chromium/ipc/message_view.cc | 12 ++---------- chromium/ipc/message_view.h | 19 +++---------------- 8 files changed, 25 insertions(+), 50 deletions(-) diff --git a/chromium/ipc/BUILD.gn b/chromium/ipc/BUILD.gn index bfebba750fb..4fc97bac279 100644 --- a/chromium/ipc/BUILD.gn +++ b/chromium/ipc/BUILD.gn @@ -181,10 +181,7 @@ mojom_component("mojom") { sources = [ "ipc.mojom", ] - public_deps = [ - "//mojo/public/interfaces/bindings", - "//mojo/public/mojom/base", - ] + public_deps = [ "//mojo/public/interfaces/bindings" ] # Don't generate a variant sources since we depend on generated internal # bindings types and we don't generate or build variants of those. diff --git a/chromium/ipc/ipc.mojom b/chromium/ipc/ipc.mojom index 9631ce8fd23..4fb8881fcfc 100644 --- a/chromium/ipc/ipc.mojom +++ b/chromium/ipc/ipc.mojom @@ -4,7 +4,6 @@ module IPC.mojom; -import "mojo/public/mojom/base/big_buffer.mojom"; import "mojo/public/interfaces/bindings/native_struct.mojom"; // A placeholder interface type since we don't yet support generic associated @@ -14,7 +13,7 @@ interface GenericInterface {}; // Typemapped such that arbitrarily large IPC::Message objects can be sent and // received with minimal copying. struct Message { - mojo_base.mojom.BigBuffer buffer; + array bytes; array? handles; }; @@ -24,6 +23,7 @@ interface Channel { SetPeerPid(int32 pid); // Transmits a classical Chrome IPC message. + [UnlimitedSize] Receive(Message message); // Requests a Channel-associated interface. diff --git a/chromium/ipc/ipc_message_pipe_reader.cc b/chromium/ipc/ipc_message_pipe_reader.cc index b055d413f45..72d20244547 100644 --- a/chromium/ipc/ipc_message_pipe_reader.cc +++ b/chromium/ipc/ipc_message_pipe_reader.cc @@ -10,6 +10,7 @@ #include "base/bind.h" #include "base/bind_helpers.h" +#include "base/containers/span.h" #include "base/location.h" #include "base/logging.h" #include "base/macros.h" @@ -63,7 +64,9 @@ bool MessagePipeReader::Send(std::unique_ptr message) { if (!sender_) return false; - sender_->Receive(MessageView(*message, std::move(handles))); + base::span bytes(static_cast(message->data()), + message->size()); + sender_->Receive(MessageView(bytes, std::move(handles))); DVLOG(4) << "Send " << message->type() << ": " << message->size(); return true; } @@ -82,11 +85,12 @@ void MessagePipeReader::SetPeerPid(int32_t peer_pid) { } void MessagePipeReader::Receive(MessageView message_view) { - if (!message_view.size()) { + if (message_view.bytes().empty()) { delegate_->OnBrokenDataReceived(); return; } - Message message(message_view.data(), message_view.size()); + Message message(reinterpret_cast(message_view.bytes().data()), + message_view.bytes().size()); if (!message.IsValid()) { delegate_->OnBrokenDataReceived(); return; diff --git a/chromium/ipc/message.typemap b/chromium/ipc/message.typemap index e150786b3ff..a99406ffece 100644 --- a/chromium/ipc/message.typemap +++ b/chromium/ipc/message.typemap @@ -11,8 +11,5 @@ sources = [ "//ipc/message_view.cc", "//ipc/message_view.h", ] -public_deps = [ - "//ipc:message_support", - "//mojo/public/cpp/base:shared_typemap_traits", -] +public_deps = [ "//ipc:message_support" ] type_mappings = [ "IPC.mojom.Message=IPC::MessageView[move_only]" ] diff --git a/chromium/ipc/message_mojom_traits.cc b/chromium/ipc/message_mojom_traits.cc index 4aab9248e9f..d8ad4a2f919 100644 --- a/chromium/ipc/message_mojom_traits.cc +++ b/chromium/ipc/message_mojom_traits.cc @@ -4,15 +4,13 @@ #include "ipc/message_mojom_traits.h" -#include "mojo/public/cpp/base/big_buffer_mojom_traits.h" - namespace mojo { // static -mojo_base::BigBufferView -StructTraits::buffer( +base::span +StructTraits::bytes( IPC::MessageView& view) { - return view.TakeBufferView(); + return view.bytes(); } // static @@ -26,14 +24,14 @@ StructTraits::handles( bool StructTraits::Read( IPC::mojom::MessageDataView data, IPC::MessageView* out) { - mojo_base::BigBufferView buffer_view; - if (!data.ReadBuffer(&buffer_view)) - return false; + mojo::ArrayDataView bytes; + data.GetBytesDataView(&bytes); + base::Optional> handles; if (!data.ReadHandles(&handles)) return false; - *out = IPC::MessageView(std::move(buffer_view), std::move(handles)); + *out = IPC::MessageView(bytes, std::move(handles)); return true; } diff --git a/chromium/ipc/message_mojom_traits.h b/chromium/ipc/message_mojom_traits.h index 617ffbe3730..6b5064a1219 100644 --- a/chromium/ipc/message_mojom_traits.h +++ b/chromium/ipc/message_mojom_traits.h @@ -7,10 +7,10 @@ #include +#include "base/containers/span.h" #include "base/optional.h" #include "ipc/ipc.mojom-shared.h" #include "ipc/message_view.h" -#include "mojo/public/cpp/base/big_buffer.h" #include "mojo/public/cpp/bindings/struct_traits.h" #include "mojo/public/interfaces/bindings/native_struct.mojom.h" @@ -19,7 +19,7 @@ namespace mojo { template <> class StructTraits { public: - static mojo_base::BigBufferView buffer(IPC::MessageView& view); + static base::span bytes(IPC::MessageView& view); static base::Optional> handles( IPC::MessageView& view); diff --git a/chromium/ipc/message_view.cc b/chromium/ipc/message_view.cc index d6ff4fb05f7..a8b9e43dc92 100644 --- a/chromium/ipc/message_view.cc +++ b/chromium/ipc/message_view.cc @@ -9,17 +9,9 @@ namespace IPC { MessageView::MessageView() = default; MessageView::MessageView( - const Message& message, + base::span bytes, base::Optional> handles) - : buffer_view_(base::make_span( - static_cast(message.data()), - message.size())), - handles_(std::move(handles)) {} - -MessageView::MessageView( - mojo_base::BigBufferView buffer_view, - base::Optional> handles) - : buffer_view_(std::move(buffer_view)), handles_(std::move(handles)) {} + : bytes_(bytes), handles_(std::move(handles)) {} MessageView::MessageView(MessageView&&) = default; diff --git a/chromium/ipc/message_view.h b/chromium/ipc/message_view.h index 23e5da6e16f..17efcdaa269 100644 --- a/chromium/ipc/message_view.h +++ b/chromium/ipc/message_view.h @@ -11,7 +11,6 @@ #include "base/containers/span.h" #include "base/macros.h" #include "ipc/ipc_message.h" -#include "mojo/public/cpp/base/big_buffer.h" #include "mojo/public/interfaces/bindings/native_struct.mojom.h" namespace IPC { @@ -20,32 +19,20 @@ class COMPONENT_EXPORT(IPC_MOJOM) MessageView { public: MessageView(); MessageView( - const Message& message, - base::Optional> handles); - MessageView( - mojo_base::BigBufferView buffer_view, + base::span bytes, base::Optional> handles); MessageView(MessageView&&); ~MessageView(); MessageView& operator=(MessageView&&); - const char* data() const { - return reinterpret_cast(buffer_view_.data().data()); - } - - uint32_t size() const { - return static_cast(buffer_view_.data().size()); - } - - mojo_base::BigBufferView TakeBufferView() { return std::move(buffer_view_); } - + base::span bytes() const { return bytes_; } base::Optional> TakeHandles() { return std::move(handles_); } private: - mojo_base::BigBufferView buffer_view_; + base::span bytes_; base::Optional> handles_; DISALLOW_COPY_AND_ASSIGN(MessageView); -- cgit v1.2.1