Mojo: Fix UAF on NodeChannel

Fixed: 1162198
Change-Id: Ief850903a7e6ba3d7c5c0129704d1f80aa3467ce
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2612085
Commit-Queue: Ken Rockot <rockot@google.com>
Reviewed-by: Ken Rockot <rockot@google.com>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#840942}

mojo/core/BUILD.gn

@@ -289,6 +289,7 @@ source_set("test_sources") {
       "data_pipe_unittest.cc",
       "invitation_unittest.cc",
       "multiprocess_message_pipe_unittest.cc",
+      "node_controller_unittest.cc",
       "platform_wrapper_unittest.cc",
     ]
   }

mojo/core/core.cc

@@ -137,7 +137,7 @@ void Core::SetIOTaskRunner(
 NodeController* Core::GetNodeController() {
   base::AutoLock lock(node_controller_lock_);
   if (!node_controller_)
-    node_controller_.reset(new NodeController(this));
+    node_controller_ = std::make_unique<NodeController>();
   return node_controller_.get();
 }
 

mojo/core/node_controller.cc

@@ -21,7 +21,6 @@
 #include "mojo/core/broker.h"
 #include "mojo/core/broker_host.h"
 #include "mojo/core/configuration.h"
-#include "mojo/core/core.h"
 #include "mojo/core/request_context.h"
 #include "mojo/core/user_message_impl.h"
 #include "mojo/public/cpp/platform/named_platform_channel.h"
@@ -146,10 +145,8 @@ class ThreadDestructionObserver
 
 NodeController::~NodeController() = default;
 
-NodeController::NodeController(Core* core)
-    : core_(core),
-      name_(GetRandomNodeName()),
-      node_(new ports::Node(name_, this)) {
+NodeController::NodeController()
+    : name_(GetRandomNodeName()), node_(new ports::Node(name_, this)) {
   DVLOG(1) << "Initializing node " << name_;
 }
 
@@ -586,10 +583,17 @@ void NodeController::AddPeer(const ports::NodeName& name,
   }
 }
 
-void NodeController::DropPeer(const ports::NodeName& name,
+void NodeController::DropPeer(const ports::NodeName& node_name,
                               NodeChannel* channel) {
   DCHECK(io_task_runner_->RunsTasksInCurrentSequence());
 
+  // NOTE: Either the `peers_` erasure or the `pending_invitations_` erasure
+  // below, if executed, may drop the last reference to the named NodeChannel
+  // and thus result in its deletion. The passed `node_name` argument may be
+  // owned by that same NodeChannel, so we make a copy of it here to avoid
+  // potentially unsafe references further below.
+  ports::NodeName name = node_name;
+
   {
     base::AutoLock lock(peers_lock_);
     auto it = peers_.find(name);

mojo/core/node_controller.h

@@ -52,11 +52,10 @@ class MOJO_SYSTEM_IMPL_EXPORT NodeController : public ports::NodeDelegate,
   };
 
   // |core| owns and out-lives us.
-  explicit NodeController(Core* core);
+  NodeController();
   ~NodeController() override;
 
   const ports::NodeName& name() const { return name_; }
-  Core* core() const { return core_; }
   ports::Node* node() const { return node_.get(); }
   scoped_refptr<base::SingleThreadTaskRunner> io_task_runner() const {
     return io_task_runner_;
@@ -135,6 +134,8 @@ class MOJO_SYSTEM_IMPL_EXPORT NodeController : public ports::NodeDelegate,
       base::span<const unsigned char> data);
   static void DeserializeMessageAsEventForFuzzer(Channel::MessagePtr message);
 
+  scoped_refptr<NodeChannel> GetBrokerChannel();
+
  private:
   friend Core;
 
@@ -176,7 +177,6 @@ class MOJO_SYSTEM_IMPL_EXPORT NodeController : public ports::NodeDelegate,
 
   scoped_refptr<NodeChannel> GetPeerChannel(const ports::NodeName& name);
   scoped_refptr<NodeChannel> GetInviterChannel();
-  scoped_refptr<NodeChannel> GetBrokerChannel();
 
   void AddPeer(const ports::NodeName& name,
                scoped_refptr<NodeChannel> channel,
@@ -254,7 +254,6 @@ class MOJO_SYSTEM_IMPL_EXPORT NodeController : public ports::NodeDelegate,
   void ForceDisconnectProcessForTestingOnIOThread(base::ProcessId process_id);
 
   // These are safe to access from any thread as long as the Node is alive.
-  Core* const core_;
   const ports::NodeName name_;
   const std::unique_ptr<ports::Node> node_;
   scoped_refptr<base::SingleThreadTaskRunner> io_task_runner_;
@@ -319,7 +318,7 @@ class MOJO_SYSTEM_IMPL_EXPORT NodeController : public ports::NodeDelegate,
   AtomicFlag shutdown_callback_flag_;
 
   // All other fields below must only be accessed on the I/O thread, i.e., the
-  // thread on which core_->io_task_runner() runs tasks.
+  // thread on which `io_task_runner_` runs tasks.
 
   // Channels to invitees during handshake.
   NodeMap pending_invitations_;

mojo/core/node_controller_unittest.cc

+// Copyright 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/logging.h"
+#include "mojo/core/core.h"
+#include "mojo/core/test/mojo_test_base.h"
+#include "mojo/public/c/system/types.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace mojo {
+namespace core {
+namespace {
+
+using NodeControllerTest = test::MojoTestBase;
+
+TEST_F(NodeControllerTest, AcceptInvitationFailure) {
+  // Spawn a child process that will send an invalid AcceptInvitation
+  // NodeChannel message. This is a regression test for
+  // https://crbug.com/1162198.
+  RunTestClient("SendInvalidAcceptInvitation",
+                [&](MojoHandle h) { WriteMessage(h, "hi"); });
+}
+
+DEFINE_TEST_CLIENT_TEST_WITH_PIPE(SendInvalidAcceptInvitation,
+                                  NodeControllerTest,
+                                  h) {
+  // A little communication to synchronize against Mojo bringup. By the time
+  // this read completes, we must have an internal NodeController with the
+  // parent test process connected as its broker.
+  EXPECT_EQ("hi", ReadMessage(h));
+
+  // Send an unexpected AcceptInvitation message to the parent process. This
+  // exercises the regression code path in the parent process.
+  NodeController* controller = Core::Get()->GetNodeController();
+  scoped_refptr<NodeChannel> channel = controller->GetBrokerChannel();
+  channel->AcceptInvitation(ports::NodeName{0, 0}, ports::NodeName{0, 0});
+}
+
+}  // namespace
+}  // namespace core
+}  // namespace mojo