From 2c846dfbcce5d4cae21eba596a59e8b537e1cea8 Mon Sep 17 00:00:00 2001 From: Ken Rockot Date: Tue, 22 Nov 2022 22:03:40 +0000 Subject: [Backport] Security bug 1325096 Manual cherry-pick of patch originally reviewed on https://chromium-review.googlesource.com/c/chromium/src/+/4024654: Mojo: Add release-mode sequence checks A fairly common class of bugs is for Mojo consumers to use thread-affine objects like Remote from the wrong thread. These bugs are dangerous since they can lead to UAFs etc. This CL enables release-mode sequence checks on InterfaceEndpointClient, the main thread-affine state object underying Remote etc. Fixed: 1325096 Change-Id: I362b798b015c021316ddca14ea35cb7618942538 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4024654 Commit-Queue: Ken Rockot Reviewed-by: Oksana Zhuravlova Cr-Commit-Position: refs/heads/main@{#1074883} Reviewed-on: https://codereview.qt-project.org/c/qt/qtwebengine-chromium/+/460503 Reviewed-by: Allan Sandfeld Jensen --- .../public/cpp/bindings/interface_endpoint_client.h | 12 +++++++----- .../cpp/bindings/lib/interface_endpoint_client.cc | 20 ++++++++++---------- 2 files changed, 17 insertions(+), 15 deletions(-) diff --git a/chromium/mojo/public/cpp/bindings/interface_endpoint_client.h b/chromium/mojo/public/cpp/bindings/interface_endpoint_client.h index fa4b4704f88..bb270e0780c 100644 --- a/chromium/mojo/public/cpp/bindings/interface_endpoint_client.h +++ b/chromium/mojo/public/cpp/bindings/interface_endpoint_client.h @@ -73,27 +73,27 @@ class COMPONENT_EXPORT(MOJO_CPP_BINDINGS) InterfaceEndpointClient // Sets the error handler to receive notifications when an error is // encountered. void set_connection_error_handler(base::OnceClosure error_handler) { - DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); + CHECK(sequence_checker_.CalledOnValidSequence()); error_handler_ = std::move(error_handler); error_with_reason_handler_.Reset(); } void set_connection_error_with_reason_handler( ConnectionErrorWithReasonCallback error_handler) { - DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); + CHECK(sequence_checker_.CalledOnValidSequence()); error_with_reason_handler_ = std::move(error_handler); error_handler_.Reset(); } // Returns true if an error was encountered. bool encountered_error() const { - DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); + CHECK(sequence_checker_.CalledOnValidSequence()); return encountered_error_; } // Returns true if this endpoint has any pending callbacks. bool has_pending_responders() const { - DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); + CHECK(sequence_checker_.CalledOnValidSequence()); base::AutoLock lock(async_responders_lock_); return !async_responders_.empty() || !sync_responses_.empty(); } @@ -354,7 +354,9 @@ class COMPONENT_EXPORT(MOJO_CPP_BINDINGS) InterfaceEndpointClient base::Location next_call_location_; #endif - SEQUENCE_CHECKER(sequence_checker_); + // We use SequenceCheckerImpl directly, to assert some sequence checks even in + // release builds. See https://crbug.com/1325096. + base::SequenceCheckerImpl sequence_checker_; base::WeakPtrFactory weak_ptr_factory_{this}; }; diff --git a/chromium/mojo/public/cpp/bindings/lib/interface_endpoint_client.cc b/chromium/mojo/public/cpp/bindings/lib/interface_endpoint_client.cc index c1e7efe6954..c7aade50db3 100644 --- a/chromium/mojo/public/cpp/bindings/lib/interface_endpoint_client.cc +++ b/chromium/mojo/public/cpp/bindings/lib/interface_endpoint_client.cc @@ -455,7 +455,7 @@ InterfaceEndpointClient::InterfaceEndpointClient( ipc_hash_callback_(ipc_hash_callback), method_name_callback_(method_name_callback) { DCHECK(handle_.is_valid()); - DETACH_FROM_SEQUENCE(sequence_checker_); + sequence_checker_.DetachFromSequence(); // TODO(yzshen): the way to use validator (or message filter in general) // directly is a little awkward. @@ -479,7 +479,7 @@ InterfaceEndpointClient::InterfaceEndpointClient( } InterfaceEndpointClient::~InterfaceEndpointClient() { - DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); + CHECK(sequence_checker_.CalledOnValidSequence()); if (controller_) handle_.group_controller()->DetachEndpointClient(handle_); } @@ -498,7 +498,7 @@ scoped_refptr InterfaceEndpointClient::CreateThreadSafeProxy( } ScopedInterfaceEndpointHandle InterfaceEndpointClient::PassHandle() { - DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); + CHECK(sequence_checker_.CalledOnValidSequence()); DCHECK(!has_pending_responders()); if (!handle_.is_valid()) @@ -520,7 +520,7 @@ void InterfaceEndpointClient::SetFilter(std::unique_ptr filter) { } void InterfaceEndpointClient::RaiseError() { - DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); + CHECK(sequence_checker_.CalledOnValidSequence()); if (!handle_.pending_association()) handle_.group_controller()->RaiseError(); @@ -528,7 +528,7 @@ void InterfaceEndpointClient::RaiseError() { void InterfaceEndpointClient::CloseWithReason(uint32_t custom_reason, base::StringPiece description) { - DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); + CHECK(sequence_checker_.CalledOnValidSequence()); auto handle = PassHandle(); handle.ResetWithReason(custom_reason, description); @@ -564,7 +564,7 @@ bool InterfaceEndpointClient::AcceptWithResponder( bool InterfaceEndpointClient::SendMessage(Message* message, bool is_control_message) { - DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); + CHECK(sequence_checker_.CalledOnValidSequence()); DCHECK(!message->has_flag(Message::kFlagExpectsResponse)); DCHECK(!handle_.pending_association()); @@ -600,7 +600,7 @@ bool InterfaceEndpointClient::SendMessageWithResponder( bool is_control_message, SyncSendMode sync_send_mode, std::unique_ptr responder) { - DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); + CHECK(sequence_checker_.CalledOnValidSequence()); DCHECK(message->has_flag(Message::kFlagExpectsResponse)); DCHECK(!handle_.pending_association()); @@ -681,7 +681,7 @@ bool InterfaceEndpointClient::SendMessageWithResponder( } bool InterfaceEndpointClient::HandleIncomingMessage(Message* message) { - DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); + CHECK(sequence_checker_.CalledOnValidSequence()); // Accept() may invalidate `this` and `message` so we need to copy the // members we need for logging in case of an error. @@ -698,7 +698,7 @@ bool InterfaceEndpointClient::HandleIncomingMessage(Message* message) { void InterfaceEndpointClient::NotifyError( const absl::optional& reason) { - DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); + CHECK(sequence_checker_.CalledOnValidSequence()); if (encountered_error_) return; @@ -821,7 +821,7 @@ void InterfaceEndpointClient::MaybeSendNotifyIdle() { } void InterfaceEndpointClient::ResetFromAnotherSequenceUnsafe() { - DETACH_FROM_SEQUENCE(sequence_checker_); + sequence_checker_.DetachFromSequence(); if (controller_) { controller_ = nullptr; -- cgit v1.2.1