Browse Source

chore: cherry pick 06fe641d21bd from chromium (#27405)

Pedro Pontes 4 years ago
parent
commit
36a3b6b8cc
2 changed files with 196 additions and 0 deletions
  1. 1 0
      patches/chromium/.patches
  2. 195 0
      patches/chromium/mojo_fix_uaf_on_nodechannel.patch

+ 1 - 0
patches/chromium/.patches

@@ -168,3 +168,4 @@ backport_1142331.patch
 backport_1151865.patch
 cherry-pick-861253f1de98.patch
 cherry-pick-d866af575997.patch
+mojo_fix_uaf_on_nodechannel.patch

+ 195 - 0
patches/chromium/mojo_fix_uaf_on_nodechannel.patch

@@ -0,0 +1,195 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Ken Rockot <[email protected]>
+Date: Tue, 12 Jan 2021 18:36:22 +0000
+Subject: Mojo: Fix UAF on NodeChannel
+
[email protected]
+
+(cherry picked from commit 9c8a98b3983dd1c7828ceae2fc8a5a2e9bad1f68)
+
+(cherry picked from commit 06fe641d21bda1b5869a46e59b13873762ce1324)
+
+Fixed: 1162198
+Change-Id: Ief850903a7e6ba3d7c5c0129704d1f80aa3467ce
+Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2612085
+Commit-Queue: Ken Rockot <[email protected]>
+Reviewed-by: Ken Rockot <[email protected]>
+Reviewed-by: Robert Sesek <[email protected]>
+Cr-Original-Original-Commit-Position: refs/heads/master@{#840942}
+Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2621996
+Cr-Original-Commit-Position: refs/branch-heads/4324@{#1637}
+Cr-Original-Branched-From: c73b5a651d37a6c4d0b8e3262cc4015a5579c6c8-refs/heads/master@{#827102}
+Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2623929
+Reviewed-by: Achuith Bhandarkar <[email protected]>
+Commit-Queue: Victor-Gabriel Savu <[email protected]>
+Cr-Commit-Position: refs/branch-heads/4240@{#1517}
+Cr-Branched-From: f297677702651916bbf65e59c0d4bbd4ce57d1ee-refs/heads/master@{#800218}
+
+diff --git a/mojo/core/BUILD.gn b/mojo/core/BUILD.gn
+index 6282eed9158c691a9ca49d1ec9a57cedc4392741..ee48a2f686077a32503d96d196b270368b2931ee 100644
+--- a/mojo/core/BUILD.gn
++++ b/mojo/core/BUILD.gn
+@@ -305,6 +305,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",
+     ]
+   }
+diff --git a/mojo/core/core.cc b/mojo/core/core.cc
+index d6092658b753561065ce2394d5a9ca9fc5303790..04e0a6fe2e7ebbc0c64fc2cd3ff4348444a3af16 100644
+--- a/mojo/core/core.cc
++++ b/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();
+ }
+ 
+diff --git a/mojo/core/node_controller.cc b/mojo/core/node_controller.cc
+index 345d22ae06bc737c087e68e7e13a2eb4eb2730cd..4505b66eb9ad90cea6f6aa25ff9f7895b0ee27a0 100644
+--- a/mojo/core/node_controller.cc
++++ b/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() {}
+ 
+-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_;
+ }
+ 
+@@ -587,10 +584,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);
+diff --git a/mojo/core/node_controller.h b/mojo/core/node_controller.h
+index e96450840378bb972cb518747d26895eb74ac623..6719845893d361cc7502561db24426dce394dd4c 100644
+--- a/mojo/core/node_controller.h
++++ b/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_;
+diff --git a/mojo/core/node_controller_unittest.cc b/mojo/core/node_controller_unittest.cc
+new file mode 100644
+index 0000000000000000000000000000000000000000..316e162376323f27f83868d6b943c40e395511bb
+--- /dev/null
++++ b/mojo/core/node_controller_unittest.cc
+@@ -0,0 +1,42 @@
++// 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