Skip to content

Commit 68bd5bd

Browse files
krockotmibrunin
authored andcommitted
[Backport] Security bug 1162198
Cherry-pick of patch originally reviewed on https://chromium-review.googlesource.com/c/chromium/src/+/2612085: Mojo: Fix UAF on NodeChannel Fixed: 1162198 Change-Id: Ief850903a7e6ba3d7c5c0129704d1f80aa3467ce Commit-Queue: Ken Rockot <[email protected]> Reviewed-by: Ken Rockot <[email protected]> Reviewed-by: Robert Sesek <[email protected]> Cr-Commit-Position: refs/heads/master@{#840942} Reviewed-by: Allan Sandfeld Jensen <[email protected]>
1 parent 8c1fdee commit 68bd5bd

File tree

3 files changed

+15
-12
lines changed

3 files changed

+15
-12
lines changed

chromium/mojo/core/core.cc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -153,7 +153,7 @@ void Core::SetIOTaskRunner(scoped_refptr<base::TaskRunner> io_task_runner) {
153153
NodeController* Core::GetNodeController() {
154154
base::AutoLock lock(node_controller_lock_);
155155
if (!node_controller_)
156-
node_controller_.reset(new NodeController(this));
156+
node_controller_ = std::make_unique<NodeController>();
157157
return node_controller_.get();
158158
}
159159

chromium/mojo/core/node_controller.cc

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@
2222
#include "mojo/core/broker.h"
2323
#include "mojo/core/broker_host.h"
2424
#include "mojo/core/configuration.h"
25-
#include "mojo/core/core.h"
2625
#include "mojo/core/request_context.h"
2726
#include "mojo/core/user_message_impl.h"
2827
#include "mojo/public/cpp/platform/named_platform_channel.h"
@@ -149,10 +148,8 @@ class ThreadDestructionObserver
149148

150149
NodeController::~NodeController() {}
151150

152-
NodeController::NodeController(Core* core)
153-
: core_(core),
154-
name_(GetRandomNodeName()),
155-
node_(new ports::Node(name_, this)) {
151+
NodeController::NodeController()
152+
: name_(GetRandomNodeName()), node_(new ports::Node(name_, this)) {
156153
DVLOG(1) << "Initializing node " << name_;
157154
}
158155

@@ -507,10 +504,17 @@ void NodeController::AddPeer(const ports::NodeName& name,
507504
}
508505
}
509506

510-
void NodeController::DropPeer(const ports::NodeName& name,
507+
void NodeController::DropPeer(const ports::NodeName& node_name,
511508
NodeChannel* channel) {
512509
DCHECK(io_task_runner_->RunsTasksInCurrentSequence());
513510

511+
// NOTE: Either the `peers_` erasure or the `pending_invitations_` erasure
512+
// below, if executed, may drop the last reference to the named NodeChannel
513+
// and thus result in its deletion. The passed `node_name` argument may be
514+
// owned by that same NodeChannel, so we make a copy of it here to avoid
515+
// potentially unsafe references further below.
516+
ports::NodeName name = node_name;
517+
514518
{
515519
base::AutoLock lock(peers_lock_);
516520
auto it = peers_.find(name);

chromium/mojo/core/node_controller.h

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -56,11 +56,10 @@ class MOJO_SYSTEM_IMPL_EXPORT NodeController : public ports::NodeDelegate,
5656
};
5757

5858
// |core| owns and out-lives us.
59-
explicit NodeController(Core* core);
59+
NodeController();
6060
~NodeController() override;
6161

6262
const ports::NodeName& name() const { return name_; }
63-
Core* core() const { return core_; }
6463
ports::Node* node() const { return node_.get(); }
6564
scoped_refptr<base::TaskRunner> io_task_runner() const {
6665
return io_task_runner_;
@@ -130,6 +129,8 @@ class MOJO_SYSTEM_IMPL_EXPORT NodeController : public ports::NodeDelegate,
130129
void NotifyBadMessageFrom(const ports::NodeName& source_node,
131130
const std::string& error);
132131

132+
scoped_refptr<NodeChannel> GetBrokerChannel();
133+
133134
private:
134135
friend Core;
135136

@@ -170,7 +171,6 @@ class MOJO_SYSTEM_IMPL_EXPORT NodeController : public ports::NodeDelegate,
170171

171172
scoped_refptr<NodeChannel> GetPeerChannel(const ports::NodeName& name);
172173
scoped_refptr<NodeChannel> GetInviterChannel();
173-
scoped_refptr<NodeChannel> GetBrokerChannel();
174174

175175
void AddPeer(const ports::NodeName& name,
176176
scoped_refptr<NodeChannel> channel,
@@ -249,7 +249,6 @@ class MOJO_SYSTEM_IMPL_EXPORT NodeController : public ports::NodeDelegate,
249249
void AttemptShutdownIfRequested();
250250

251251
// These are safe to access from any thread as long as the Node is alive.
252-
Core* const core_;
253252
const ports::NodeName name_;
254253
const std::unique_ptr<ports::Node> node_;
255254
scoped_refptr<base::TaskRunner> io_task_runner_;
@@ -314,7 +313,7 @@ class MOJO_SYSTEM_IMPL_EXPORT NodeController : public ports::NodeDelegate,
314313
AtomicFlag shutdown_callback_flag_;
315314

316315
// All other fields below must only be accessed on the I/O thread, i.e., the
317-
// thread on which core_->io_task_runner() runs tasks.
316+
// thread on which `io_task_runner_` runs tasks.
318317

319318
// Channels to invitees during handshake.
320319
NodeMap pending_invitations_;

0 commit comments

Comments
 (0)