From fada5812d8c5cfd49fb05cdefe77c703997a687e Mon Sep 17 00:00:00 2001 From: Ken Rockot Date: Tue, 11 Dec 2018 16:49:54 +0000 Subject: [PATCH] [mojo-core] Don't accept HANDLEs from non-brokers On Windows, non-broker processes should only accept HANDLEs in messages coming from the broker and/or inviter process. This is because a non-broker or invitee receiver has to assume the sender has already duplicated sent HANDLEs into the receiver's process, and making that assumption requires a level of trust that should not be granted to arbitrary peers. This constraint is already met today under normal circumstances, but a malicious or misbehaving process could easily violate it, tricking an unassuming receiver into attempting double-ownership of an existing handle or closure of an invalid handle value, both of which can result in a crash (though thankfully nothing worse, because of ScopedHandleVerifier). This adds an option to Channel which allows it to reject incoming platform handles and essentially treat them as malformed messages. The option is set on Windows for any non-broker process's receiving Channel endpoint which is not connected directly to a broker process or to the process which invited that client. The Channel fuzzer also sets this option to avoid a crash since it exercises only the non-broker receiving path today. A follow-up CL will extend fuzzer coverage to include broker receivers as well. Bug: 909713 Change-Id: Ie0fece347fcf23d6f8111be4e41398f22d617531 Reviewed-on: https://chromium-review.googlesource.com/c/1363649 Commit-Queue: Ken Rockot Reviewed-by: Reilly Grant Cr-Commit-Position: refs/heads/master@{#615555} --- mojo/core/broker_host.cc | 1 + mojo/core/channel.cc | 9 +++- mojo/core/channel.h | 17 ++++++- mojo/core/channel_fuchsia.cc | 6 ++- mojo/core/channel_fuzzer.cc | 2 + mojo/core/channel_posix.cc | 8 ++- mojo/core/channel_unittest.cc | 75 +++++++++++++++++++++++++++-- mojo/core/channel_win.cc | 9 +++- mojo/core/node_channel.cc | 11 +++-- mojo/core/node_channel.h | 4 ++ mojo/core/node_channel_fuzzer.cc | 2 + mojo/core/node_controller.cc | 41 +++++++++++----- mojo/core/test/run_all_unittests.cc | 9 +++- services/test/run_all_unittests.cc | 5 +- 14 files changed, 168 insertions(+), 31 deletions(-) diff --git a/mojo/core/broker_host.cc b/mojo/core/broker_host.cc index 875cc2fe4907c5..286562c368a777 100644 --- a/mojo/core/broker_host.cc +++ b/mojo/core/broker_host.cc @@ -36,6 +36,7 @@ BrokerHost::BrokerHost(base::ProcessHandle client_process, base::MessageLoopCurrent::Get()->AddDestructionObserver(this); channel_ = Channel::Create(this, std::move(connection_params), + Channel::HandlePolicy::kAcceptHandles, base::ThreadTaskRunnerHandle::Get()); channel_->Start(); } diff --git a/mojo/core/channel.cc b/mojo/core/channel.cc index 2bdb86df6d8a33..274e2790198c38 100644 --- a/mojo/core/channel.cc +++ b/mojo/core/channel.cc @@ -613,8 +613,10 @@ class Channel::ReadBuffer { DISALLOW_COPY_AND_ASSIGN(ReadBuffer); }; -Channel::Channel(Delegate* delegate) - : delegate_(delegate), read_buffer_(new ReadBuffer) {} +Channel::Channel(Delegate* delegate, HandlePolicy handle_policy) + : delegate_(delegate), + handle_policy_(handle_policy), + read_buffer_(new ReadBuffer) {} Channel::~Channel() {} @@ -701,6 +703,9 @@ bool Channel::OnReadComplete(size_t bytes_read, size_t* next_read_size_hint) { std::vector handles; bool deferred = false; if (num_handles > 0) { + if (handle_policy_ == HandlePolicy::kRejectHandles) + return false; + if (!GetReadPlatformHandles(payload, payload_size, num_handles, extra_header, extra_header_size, &handles, &deferred)) { diff --git a/mojo/core/channel.h b/mojo/core/channel.h index 15a1a2df358de1..5ab84f2578cd1f 100644 --- a/mojo/core/channel.h +++ b/mojo/core/channel.h @@ -33,6 +33,16 @@ constexpr bool IsAlignedForChannelMessage(size_t n) { class MOJO_SYSTEM_IMPL_EXPORT Channel : public base::RefCountedThreadSafe { public: + enum class HandlePolicy { + // If a Channel is constructed in this mode, it will accept messages with + // platform handle attachements. + kAcceptHandles, + + // If a Channel is constructed in this mode, it will reject messages with + // platform handle attachments and treat them as malformed messages. + kRejectHandles, + }; + struct Message; using MessagePtr = std::unique_ptr; @@ -268,8 +278,12 @@ class MOJO_SYSTEM_IMPL_EXPORT Channel static scoped_refptr Create( Delegate* delegate, ConnectionParams connection_params, + HandlePolicy handle_policy, scoped_refptr io_task_runner); + // Allows the caller to change the Channel's HandlePolicy after construction. + void set_handle_policy(HandlePolicy policy) { handle_policy_ = policy; } + // Request that the channel be shut down. This should always be called before // releasing the last reference to a Channel to ensure that it's cleaned up // on its I/O task runner's thread. @@ -302,7 +316,7 @@ class MOJO_SYSTEM_IMPL_EXPORT Channel virtual void LeakHandle() = 0; protected: - explicit Channel(Delegate* delegate); + Channel(Delegate* delegate, HandlePolicy handle_policy); virtual ~Channel(); Delegate* delegate() const { return delegate_; } @@ -361,6 +375,7 @@ class MOJO_SYSTEM_IMPL_EXPORT Channel class ReadBuffer; Delegate* delegate_; + HandlePolicy handle_policy_; const std::unique_ptr read_buffer_; // Handle to the process on the other end of this Channel, iff known. diff --git a/mojo/core/channel_fuchsia.cc b/mojo/core/channel_fuchsia.cc index 8b06133297fd0f..0dc6794b0462fd 100644 --- a/mojo/core/channel_fuchsia.cc +++ b/mojo/core/channel_fuchsia.cc @@ -194,8 +194,9 @@ class ChannelFuchsia : public Channel, public: ChannelFuchsia(Delegate* delegate, ConnectionParams connection_params, + HandlePolicy handle_policy, scoped_refptr io_task_runner) - : Channel(delegate), + : Channel(delegate, handle_policy), self_(this), handle_( connection_params.TakeEndpoint().TakePlatformHandle().TakeHandle()), @@ -457,9 +458,10 @@ class ChannelFuchsia : public Channel, scoped_refptr Channel::Create( Delegate* delegate, ConnectionParams connection_params, + HandlePolicy handle_policy, scoped_refptr io_task_runner) { return new ChannelFuchsia(delegate, std::move(connection_params), - std::move(io_task_runner)); + handle_policy, std::move(io_task_runner)); } } // namespace core diff --git a/mojo/core/channel_fuzzer.cc b/mojo/core/channel_fuzzer.cc index b94d636f1ea937..b72f9e74ba6328 100644 --- a/mojo/core/channel_fuzzer.cc +++ b/mojo/core/channel_fuzzer.cc @@ -49,12 +49,14 @@ extern "C" int LLVMFuzzerTestOneInput(const unsigned char* data, size_t size) { FakeChannelDelegate receiver_delegate; auto receiver = Channel::Create(&receiver_delegate, ConnectionParams(channel.TakeLocalEndpoint()), + Channel::HandlePolicy::kRejectHandles, environment->message_loop.task_runner()); receiver->Start(); FakeChannelDelegate sender_delegate; auto sender = Channel::Create(&sender_delegate, ConnectionParams(channel.TakeRemoteEndpoint()), + Channel::HandlePolicy::kRejectHandles, environment->message_loop.task_runner()); sender->Start(); diff --git a/mojo/core/channel_posix.cc b/mojo/core/channel_posix.cc index f17d73a2992aae..430dbb740541c7 100644 --- a/mojo/core/channel_posix.cc +++ b/mojo/core/channel_posix.cc @@ -102,8 +102,11 @@ class ChannelPosix : public Channel, public: ChannelPosix(Delegate* delegate, ConnectionParams connection_params, + HandlePolicy handle_policy, scoped_refptr io_task_runner) - : Channel(delegate), self_(this), io_task_runner_(io_task_runner) { + : Channel(delegate, handle_policy), + self_(this), + io_task_runner_(io_task_runner) { if (connection_params.server_endpoint().is_valid()) server_ = connection_params.TakeServerEndpoint(); else @@ -766,8 +769,9 @@ class ChannelPosix : public Channel, scoped_refptr Channel::Create( Delegate* delegate, ConnectionParams connection_params, + HandlePolicy handle_policy, scoped_refptr io_task_runner) { - return new ChannelPosix(delegate, std::move(connection_params), + return new ChannelPosix(delegate, std::move(connection_params), handle_policy, io_task_runner); } diff --git a/mojo/core/channel_unittest.cc b/mojo/core/channel_unittest.cc index e53cc0ffbc12a3..48253e2116983b 100644 --- a/mojo/core/channel_unittest.cc +++ b/mojo/core/channel_unittest.cc @@ -7,6 +7,8 @@ #include "base/bind.h" #include "base/memory/ptr_util.h" #include "base/message_loop/message_loop.h" +#include "base/optional.h" +#include "base/run_loop.h" #include "base/threading/thread.h" #include "mojo/core/platform_handle_utils.h" #include "mojo/public/cpp/platform/platform_channel.h" @@ -19,7 +21,8 @@ namespace { class TestChannel : public Channel { public: - TestChannel(Channel::Delegate* delegate) : Channel(delegate) {} + TestChannel(Channel::Delegate* delegate) + : Channel(delegate, Channel::HandlePolicy::kAcceptHandles) {} char* GetReadBufferTest(size_t* buffer_capacity) { return GetReadBuffer(buffer_capacity); @@ -193,6 +196,7 @@ class ChannelTestShutdownAndWriteDelegate : public Channel::Delegate { client_channel_(std::move(client_channel)), client_thread_(std::move(client_thread)) { channel_ = Channel::Create(this, ConnectionParams(std::move(endpoint)), + Channel::HandlePolicy::kAcceptHandles, std::move(task_runner)); channel_->Start(); } @@ -249,9 +253,9 @@ TEST(ChannelTest, PeerShutdownDuringRead) { client_thread->StartWithOptions( base::Thread::Options(base::MessageLoop::TYPE_IO, 0)); - scoped_refptr client_channel = - Channel::Create(nullptr, ConnectionParams(channel.TakeRemoteEndpoint()), - client_thread->task_runner()); + scoped_refptr client_channel = Channel::Create( + nullptr, ConnectionParams(channel.TakeRemoteEndpoint()), + Channel::HandlePolicy::kAcceptHandles, client_thread->task_runner()); client_channel->Start(); // On the "client" IO thread, create and write a message. @@ -273,6 +277,69 @@ TEST(ChannelTest, PeerShutdownDuringRead) { run_loop.Run(); } +class RejectHandlesDelegate : public Channel::Delegate { + public: + RejectHandlesDelegate() = default; + + size_t num_messages() const { return num_messages_; } + + // Channel::Delegate: + void OnChannelMessage(const void* payload, + size_t payload_size, + std::vector handles) override { + ++num_messages_; + } + + void OnChannelError(Channel::Error error) override { + if (wait_for_error_loop_) + wait_for_error_loop_->Quit(); + } + + void WaitForError() { + wait_for_error_loop_.emplace(); + wait_for_error_loop_->Run(); + } + + private: + size_t num_messages_ = 0; + base::Optional wait_for_error_loop_; + + DISALLOW_COPY_AND_ASSIGN(RejectHandlesDelegate); +}; + +TEST(ChannelTest, RejectHandles) { + base::MessageLoop message_loop(base::MessageLoop::TYPE_IO); + PlatformChannel platform_channel; + + RejectHandlesDelegate receiver_delegate; + scoped_refptr receiver = Channel::Create( + &receiver_delegate, + ConnectionParams(platform_channel.TakeLocalEndpoint()), + Channel::HandlePolicy::kRejectHandles, message_loop.task_runner()); + receiver->Start(); + + RejectHandlesDelegate sender_delegate; + scoped_refptr sender = Channel::Create( + &sender_delegate, ConnectionParams(platform_channel.TakeRemoteEndpoint()), + Channel::HandlePolicy::kRejectHandles, message_loop.task_runner()); + sender->Start(); + + // Create another platform channel just to stuff one of its endpoint handles + // into a message. Sending this message to the receiver should cause the + // receiver to reject it and close the Channel without ever dispatching the + // message. + PlatformChannel dummy_channel; + std::vector handles; + handles.push_back(dummy_channel.TakeLocalEndpoint().TakePlatformHandle()); + auto message = std::make_unique(0 /* payload_size */, + 1 /* max_handles */); + message->SetHandles(std::move(handles)); + sender->Write(std::move(message)); + + receiver_delegate.WaitForError(); + EXPECT_EQ(0u, receiver_delegate.num_messages()); +} + } // namespace } // namespace core } // namespace mojo diff --git a/mojo/core/channel_win.cc b/mojo/core/channel_win.cc index bca080c412ed10..0a2000c272894b 100644 --- a/mojo/core/channel_win.cc +++ b/mojo/core/channel_win.cc @@ -35,8 +35,11 @@ class ChannelWin : public Channel, public: ChannelWin(Delegate* delegate, ConnectionParams connection_params, + HandlePolicy handle_policy, scoped_refptr io_task_runner) - : Channel(delegate), self_(this), io_task_runner_(io_task_runner) { + : Channel(delegate, handle_policy), + self_(this), + io_task_runner_(io_task_runner) { if (connection_params.server_endpoint().is_valid()) { handle_ = connection_params.TakeServerEndpoint() .TakePlatformHandle() @@ -378,8 +381,10 @@ class ChannelWin : public Channel, scoped_refptr Channel::Create( Delegate* delegate, ConnectionParams connection_params, + HandlePolicy handle_policy, scoped_refptr io_task_runner) { - return new ChannelWin(delegate, std::move(connection_params), io_task_runner); + return new ChannelWin(delegate, std::move(connection_params), handle_policy, + io_task_runner); } } // namespace core diff --git a/mojo/core/node_channel.cc b/mojo/core/node_channel.cc index 53b752f22d1e3b..cabbe8809e6f79 100644 --- a/mojo/core/node_channel.cc +++ b/mojo/core/node_channel.cc @@ -160,13 +160,15 @@ bool GetMessagePayload(const void* bytes, scoped_refptr NodeChannel::Create( Delegate* delegate, ConnectionParams connection_params, + Channel::HandlePolicy channel_handle_policy, scoped_refptr io_task_runner, const ProcessErrorCallback& process_error_callback) { #if defined(OS_NACL_SFI) LOG(FATAL) << "Multi-process not yet supported on NaCl-SFI"; return nullptr; #else - return new NodeChannel(delegate, std::move(connection_params), io_task_runner, + return new NodeChannel(delegate, std::move(connection_params), + channel_handle_policy, io_task_runner, process_error_callback); #endif } @@ -439,6 +441,7 @@ void NodeChannel::EventMessageFromRelay(const ports::NodeName& source, NodeChannel::NodeChannel(Delegate* delegate, ConnectionParams connection_params, + Channel::HandlePolicy channel_handle_policy, scoped_refptr io_task_runner, const ProcessErrorCallback& process_error_callback) : delegate_(delegate), @@ -446,8 +449,10 @@ NodeChannel::NodeChannel(Delegate* delegate, process_error_callback_(process_error_callback) #if !defined(OS_NACL_SFI) , - channel_( - Channel::Create(this, std::move(connection_params), io_task_runner_)) + channel_(Channel::Create(this, + std::move(connection_params), + channel_handle_policy, + io_task_runner_)) #endif { } diff --git a/mojo/core/node_channel.h b/mojo/core/node_channel.h index 5573305013f015..554860077430b5 100644 --- a/mojo/core/node_channel.h +++ b/mojo/core/node_channel.h @@ -79,6 +79,7 @@ class NodeChannel : public base::RefCountedThreadSafe, static scoped_refptr Create( Delegate* delegate, ConnectionParams connection_params, + Channel::HandlePolicy channel_handle_policy, scoped_refptr io_task_runner, const ProcessErrorCallback& process_error_callback); @@ -91,6 +92,8 @@ class NodeChannel : public base::RefCountedThreadSafe, void** data, size_t* num_data_bytes); + Channel* channel() const { return channel_.get(); } + // Start receiving messages. void Start(); @@ -154,6 +157,7 @@ class NodeChannel : public base::RefCountedThreadSafe, NodeChannel(Delegate* delegate, ConnectionParams connection_params, + Channel::HandlePolicy channel_handle_policy, scoped_refptr io_task_runner, const ProcessErrorCallback& process_error_callback); ~NodeChannel() override; diff --git a/mojo/core/node_channel_fuzzer.cc b/mojo/core/node_channel_fuzzer.cc index 63de8425ee914d..c9276098393583 100644 --- a/mojo/core/node_channel_fuzzer.cc +++ b/mojo/core/node_channel_fuzzer.cc @@ -102,6 +102,7 @@ extern "C" int LLVMFuzzerTestOneInput(const unsigned char* data, size_t size) { FakeNodeChannelDelegate receiver_delegate; auto receiver = NodeChannel::Create( &receiver_delegate, ConnectionParams(channel.TakeLocalEndpoint()), + Channel::HandlePolicy::kRejectHandles, environment->message_loop.task_runner(), base::DoNothing()); receiver->Start(); @@ -113,6 +114,7 @@ extern "C" int LLVMFuzzerTestOneInput(const unsigned char* data, size_t size) { FakeChannelDelegate sender_delegate; auto sender = Channel::Create(&sender_delegate, ConnectionParams(channel.TakeRemoteEndpoint()), + Channel::HandlePolicy::kRejectHandles, environment->message_loop.task_runner()); sender->Start(); auto message = std::make_unique(size, 0 /* num_handles */); diff --git a/mojo/core/node_controller.cc b/mojo/core/node_controller.cc index 2174de03856f89..44b8ef341fc70d 100644 --- a/mojo/core/node_controller.cc +++ b/mojo/core/node_controller.cc @@ -369,13 +369,14 @@ void NodeController::SendBrokerClientInvitationOnIOThread( scoped_refptr channel = NodeChannel::Create(this, std::move(node_connection_params), + Channel::HandlePolicy::kAcceptHandles, io_task_runner_, process_error_callback); -#else // !defined(OS_MACOSX) && !defined(OS_NACL) - scoped_refptr channel = - NodeChannel::Create(this, std::move(connection_params), io_task_runner_, - process_error_callback); -#endif // !defined(OS_MACOSX) && !defined(OS_NACL) +#else // !defined(OS_MACOSX) && !defined(OS_NACL) && !defined(OS_FUCHSIA) + scoped_refptr channel = NodeChannel::Create( + this, std::move(connection_params), Channel::HandlePolicy::kAcceptHandles, + io_task_runner_, process_error_callback); +#endif // !defined(OS_MACOSX) && !defined(OS_NACL) && !defined(OS_FUCHSIA) // We set up the invitee channel with a temporary name so it can be identified // as a pending invitee if it writes any messages to the channel. We may start @@ -403,8 +404,9 @@ void NodeController::AcceptBrokerClientInvitationOnIOThread( // into our |peers_| map. That will happen as soon as we receive an // AcceptInvitee message from them. bootstrap_inviter_channel_ = - NodeChannel::Create(this, std::move(connection_params), io_task_runner_, - ProcessErrorCallback()); + NodeChannel::Create(this, std::move(connection_params), + Channel::HandlePolicy::kAcceptHandles, + io_task_runner_, ProcessErrorCallback()); // Prevent the inviter pipe handle from being closed on shutdown. Pipe // closure may be used by the inviter to detect the invitee process has // exited. @@ -419,8 +421,12 @@ void NodeController::ConnectIsolatedOnIOThread( const std::string& connection_name) { DCHECK(io_task_runner_->RunsTasksInCurrentSequence()); + // Processes using isolated connections to communicate have no ability to lean + // on a broker for handle relaying, so we allow them to send handles to each + // other at their own peril. scoped_refptr channel = NodeChannel::Create( - this, std::move(connection_params), io_task_runner_, {}); + this, std::move(connection_params), Channel::HandlePolicy::kAcceptHandles, + io_task_runner_, {}); RequestContext request_context; ports::NodeName token; @@ -852,9 +858,9 @@ void NodeController::OnAddBrokerClient(const ports::NodeName& from_node, PlatformChannel broker_channel; ConnectionParams connection_params(broker_channel.TakeLocalEndpoint()); - scoped_refptr client = - NodeChannel::Create(this, std::move(connection_params), io_task_runner_, - ProcessErrorCallback()); + scoped_refptr client = NodeChannel::Create( + this, std::move(connection_params), Channel::HandlePolicy::kAcceptHandles, + io_task_runner_, ProcessErrorCallback()); #if defined(OS_WIN) // The broker must have a working handle to the client process in order to @@ -934,7 +940,8 @@ void NodeController::OnAcceptBrokerClient(const ports::NodeName& from_node, broker = NodeChannel::Create( this, ConnectionParams(PlatformChannelEndpoint(std::move(broker_channel))), - io_task_runner_, ProcessErrorCallback()); + Channel::HandlePolicy::kAcceptHandles, io_task_runner_, + ProcessErrorCallback()); AddPeer(broker_name, broker, true /* start_channel */); } @@ -1075,10 +1082,18 @@ void NodeController::OnIntroduce(const ports::NodeName& from_node, return; } +#if defined(OS_WIN) + // Introduced peers are never our broker nor our inviter, so we never accept + // handles from them directly. + constexpr auto kPeerHandlePolicy = Channel::HandlePolicy::kRejectHandles; +#else + constexpr auto kPeerHandlePolicy = Channel::HandlePolicy::kAcceptHandles; +#endif + scoped_refptr channel = NodeChannel::Create( this, ConnectionParams(PlatformChannelEndpoint(std::move(channel_handle))), - io_task_runner_, ProcessErrorCallback()); + kPeerHandlePolicy, io_task_runner_, ProcessErrorCallback()); DVLOG(1) << "Adding new peer " << name << " via broker introduction."; AddPeer(name, channel, true /* start_channel */); diff --git a/mojo/core/test/run_all_unittests.cc b/mojo/core/test/run_all_unittests.cc index 5bc8ccba9f726b..882cd250bdce8a 100644 --- a/mojo/core/test/run_all_unittests.cc +++ b/mojo/core/test/run_all_unittests.cc @@ -4,6 +4,7 @@ #include +#include "base/base_switches.h" #include "base/bind.h" #include "base/command_line.h" #include "base/test/launcher/unit_test_launcher.h" @@ -11,6 +12,7 @@ #include "base/test/test_io_thread.h" #include "base/test/test_suite.h" #include "build/build_config.h" +#include "mojo/core/embedder/configuration.h" #include "mojo/core/embedder/embedder.h" #include "mojo/core/embedder/scoped_ipc_support.h" #include "mojo/core/test/multiprocess_test_helper.h" @@ -40,7 +42,12 @@ int main(int argc, char** argv) { base::TestSuite test_suite(argc, argv); - mojo::core::Init(); + mojo::core::Configuration mojo_config; + if (!base::CommandLine::ForCurrentProcess()->HasSwitch( + switches::kTestChildProcess)) { + mojo_config.is_broker_process = true; + } + mojo::core::Init(mojo_config); #if defined(OS_MACOSX) && !defined(OS_IOS) mojo::core::SetMachPortProvider( diff --git a/services/test/run_all_unittests.cc b/services/test/run_all_unittests.cc index 7e14ead62839c2..864c71832bbe95 100644 --- a/services/test/run_all_unittests.cc +++ b/services/test/run_all_unittests.cc @@ -12,6 +12,7 @@ #include "base/test/test_suite.h" #include "base/threading/thread.h" #include "build/build_config.h" +#include "mojo/core/embedder/configuration.h" #include "mojo/core/embedder/embedder.h" #include "mojo/core/embedder/scoped_ipc_support.h" #include "ui/base/resource/resource_bundle.h" @@ -77,7 +78,9 @@ class ServiceTestSuite : public base::TestSuite { int main(int argc, char** argv) { ServiceTestSuite test_suite(argc, argv); - mojo::core::Init(); + mojo::core::Configuration mojo_config; + mojo_config.is_broker_process = true; + mojo::core::Init(mojo_config); #if defined(OS_MACOSX) && !defined(OS_IOS) mojo::core::SetMachPortProvider(