[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 <rockot@google.com>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615555}

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();
 }

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<PlatformHandle> 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)) {

mojo/core/channel.h

@@ -33,6 +33,16 @@ constexpr bool IsAlignedForChannelMessage(size_t n) {
 class MOJO_SYSTEM_IMPL_EXPORT Channel
     : public base::RefCountedThreadSafe<Channel> {
  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<Message>;
@@ -268,8 +278,12 @@ class MOJO_SYSTEM_IMPL_EXPORT Channel
   static scoped_refptr<Channel> Create(
       Delegate* delegate,
       ConnectionParams connection_params,
+      HandlePolicy handle_policy,
       scoped_refptr<base::TaskRunner> 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<ReadBuffer> read_buffer_;
 
   // Handle to the process on the other end of this Channel, iff known.

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<base::TaskRunner> 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> Channel::Create(
     Delegate* delegate,
     ConnectionParams connection_params,
+    HandlePolicy handle_policy,
     scoped_refptr<base::TaskRunner> 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

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();
 

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<base::TaskRunner> 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> Channel::Create(
     Delegate* delegate,
     ConnectionParams connection_params,
+    HandlePolicy handle_policy,
     scoped_refptr<base::TaskRunner> io_task_runner) {
-  return new ChannelPosix(delegate, std::move(connection_params),
+  return new ChannelPosix(delegate, std::move(connection_params), handle_policy,
                           io_task_runner);
 }
 

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<Channel> client_channel =
-      Channel::Create(nullptr, ConnectionParams(channel.TakeRemoteEndpoint()),
-                      client_thread->task_runner());
+  scoped_refptr<Channel> 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<PlatformHandle> 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<base::RunLoop> 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<Channel> receiver = Channel::Create(
+      &receiver_delegate,
+      ConnectionParams(platform_channel.TakeLocalEndpoint()),
+      Channel::HandlePolicy::kRejectHandles, message_loop.task_runner());
+  receiver->Start();
+
+  RejectHandlesDelegate sender_delegate;
+  scoped_refptr<Channel> 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<mojo::PlatformHandle> handles;
+  handles.push_back(dummy_channel.TakeLocalEndpoint().TakePlatformHandle());
+  auto message = std::make_unique<Channel::Message>(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

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<base::TaskRunner> 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> Channel::Create(
     Delegate* delegate,
     ConnectionParams connection_params,
+    HandlePolicy handle_policy,
     scoped_refptr<base::TaskRunner> 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

mojo/core/node_channel.cc

@@ -160,13 +160,15 @@ bool GetMessagePayload(const void* bytes,
 scoped_refptr<NodeChannel> NodeChannel::Create(
     Delegate* delegate,
     ConnectionParams connection_params,
+    Channel::HandlePolicy channel_handle_policy,
     scoped_refptr<base::TaskRunner> 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,15 +441,18 @@ void NodeChannel::EventMessageFromRelay(const ports::NodeName& source,
 
 NodeChannel::NodeChannel(Delegate* delegate,
                          ConnectionParams connection_params,
+                         Channel::HandlePolicy channel_handle_policy,
                          scoped_refptr<base::TaskRunner> io_task_runner,
                          const ProcessErrorCallback& process_error_callback)
     : delegate_(delegate),
       io_task_runner_(io_task_runner),
       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
 {
 }

mojo/core/node_channel.h

@@ -79,6 +79,7 @@ class NodeChannel : public base::RefCountedThreadSafe<NodeChannel>,
   static scoped_refptr<NodeChannel> Create(
       Delegate* delegate,
       ConnectionParams connection_params,
+      Channel::HandlePolicy channel_handle_policy,
       scoped_refptr<base::TaskRunner> io_task_runner,
       const ProcessErrorCallback& process_error_callback);
 
@@ -91,6 +92,8 @@ class NodeChannel : public base::RefCountedThreadSafe<NodeChannel>,
                                   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>,
 
   NodeChannel(Delegate* delegate,
               ConnectionParams connection_params,
+              Channel::HandlePolicy channel_handle_policy,
               scoped_refptr<base::TaskRunner> io_task_runner,
               const ProcessErrorCallback& process_error_callback);
   ~NodeChannel() override;

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<Channel::Message>(size, 0 /* num_handles */);