github/electron
1
1
Fork 0
mirror of https://github.com/electron/electron.git synced 2026-04-10 03:01:51 -04:00
Code Issues Packages Projects Releases Wiki Activity

chore: cherry-pick 5e61913985df from chromium (#25389)

Browse Source
This commit is contained in:
Cheng Zhao
2020-09-14 09:50:03 +09:00
committed by GitHub
parent 75f193a089
commit 20f58fddf1
2 changed files with 550 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
patches/chromium/.patches
View File

@@ -132,3 +132,4 @@ indexeddb_fix_crash_in_webidbgetdbnamescallbacksimpl.patch
indexeddb_reset_async_tasks_in_webidbgetdbnamescallbacksimpl.patch
reland_fix_uaf_in_selecttype.patch
cherry-pick-f6cb89728f04.patch
backport_1081874.patch

549
patches/chromium/backport_1081874.patch Normal file
View File

@@ -0,0 +1,549 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Cheng Zhao <zcbenz@gmail.com>
Date: Thu, 4 Oct 2018 14:57:02 -0700
Subject: fix: data race on NodeChannel destruction
[1081874] [High] [CVE-2020-6575]: Double free on NodeChannel
Backport https://chromium.googlesource.com/chromium/src/+/5e61913985df0cc621bf72f7fa75e76759ffde15.
diff --git a/mojo/core/BUILD.gn b/mojo/core/BUILD.gn
index e6fcb96256f6fdb867a362588547a7829f28efe8..6282eed9158c691a9ca49d1ec9a57cedc4392741 100644
--- a/mojo/core/BUILD.gn
+++ b/mojo/core/BUILD.gn
@@ -18,6 +18,7 @@ component("embedder_internal") {
":test_sources",
"//mojo:*",
"//mojo/core/embedder",
+ "//mojo/core/test:test_support",
]
}
@@ -57,6 +58,7 @@ template("core_impl_source_set") {
"handle_table.h",
"invitation_dispatcher.h",
"message_pipe_dispatcher.h",
+ "node_channel.h",
"node_controller.h",
"options_validation.h",
"platform_handle_dispatcher.h",
@@ -86,7 +88,6 @@ template("core_impl_source_set") {
"invitation_dispatcher.cc",
"message_pipe_dispatcher.cc",
"node_channel.cc",
- "node_channel.h",
"node_controller.cc",
"platform_handle_dispatcher.cc",
"platform_handle_in_transit.cc",
@@ -289,6 +290,7 @@ source_set("test_sources") {
"handle_table_unittest.cc",
"message_pipe_unittest.cc",
"message_unittest.cc",
+ "node_channel_unittest.cc",
"options_validation_unittest.cc",
"platform_handle_dispatcher_unittest.cc",
"quota_unittest.cc",
@@ -387,6 +389,7 @@ fuzzer_test("mojo_core_node_channel_fuzzer") {
deps = [
":core_impl_for_fuzzers",
"//base",
+ "//mojo/core/test:test_support",
"//mojo/public/cpp/platform",
]
}
diff --git a/mojo/core/embedder/embedder.cc b/mojo/core/embedder/embedder.cc
index ec68e37d09888f672759f79961e3e6e4d18f0c5c..f70c73be4df529cce09cb503dfce28f2a04e7e1a 100644
--- a/mojo/core/embedder/embedder.cc
+++ b/mojo/core/embedder/embedder.cc
@@ -35,7 +35,7 @@ void SetDefaultProcessErrorCallback(ProcessErrorCallback callback) {
Core::Get()->SetDefaultProcessErrorCallback(std::move(callback));
}
-scoped_refptr<base::TaskRunner> GetIOTaskRunner() {
+scoped_refptr<base::SingleThreadTaskRunner> GetIOTaskRunner() {
return Core::Get()->GetNodeController()->io_task_runner();
}
diff --git a/mojo/core/embedder/embedder.h b/mojo/core/embedder/embedder.h
index 5d65400987728940a296ba2a84d40158f52d6d34..96dc44c0a78e0e6cb6e5aa511bd2fd4f1252cf16 100644
--- a/mojo/core/embedder/embedder.h
+++ b/mojo/core/embedder/embedder.h
@@ -13,7 +13,7 @@
#include "base/component_export.h"
#include "base/memory/ref_counted.h"
#include "base/process/process_handle.h"
-#include "base/task_runner.h"
+#include "base/single_thread_task_runner.h"
#include "build/build_config.h"
#include "mojo/core/embedder/configuration.h"
@@ -42,9 +42,10 @@ void SetDefaultProcessErrorCallback(ProcessErrorCallback callback);
// Initialialization/shutdown for interprocess communication (IPC) -------------
-// Retrieves the TaskRunner used for IPC I/O, as set by ScopedIPCSupport.
+// Retrieves the SequencedTaskRunner used for IPC I/O, as set by
+// ScopedIPCSupport.
COMPONENT_EXPORT(MOJO_CORE_EMBEDDER)
-scoped_refptr<base::TaskRunner> GetIOTaskRunner();
+scoped_refptr<base::SingleThreadTaskRunner> GetIOTaskRunner();
} // namespace core
} // namespace mojo
diff --git a/mojo/core/node_channel.cc b/mojo/core/node_channel.cc
index e898b044286e019b5e423b941502030ce3094582..061ea1026e95d1b1f80a762ce377aebdd97e1b42 100644
--- a/mojo/core/node_channel.cc
+++ b/mojo/core/node_channel.cc
@@ -228,7 +228,7 @@ void NodeChannel::NotifyBadMessage(const std::string& error) {
}
void NodeChannel::SetRemoteProcessHandle(ScopedProcessHandle process_handle) {
- DCHECK(io_task_runner_->RunsTasksInCurrentSequence());
+ DCHECK(owning_task_runner()->RunsTasksInCurrentSequence());
{
base::AutoLock lock(channel_lock_);
if (channel_)
@@ -253,7 +253,7 @@ ScopedProcessHandle NodeChannel::CloneRemoteProcessHandle() {
}
void NodeChannel::SetRemoteNodeName(const ports::NodeName& name) {
- DCHECK(io_task_runner_->RunsTasksInCurrentSequence());
+ DCHECK(owning_task_runner()->RunsTasksInCurrentSequence());
remote_node_name_ = name;
}
@@ -468,15 +468,15 @@ NodeChannel::NodeChannel(
Channel::HandlePolicy channel_handle_policy,
scoped_refptr<base::SingleThreadTaskRunner> io_task_runner,
const ProcessErrorCallback& process_error_callback)
- : delegate_(delegate),
- io_task_runner_(io_task_runner),
+ : base::RefCountedDeleteOnSequence<NodeChannel>(io_task_runner),
+ delegate_(delegate),
process_error_callback_(process_error_callback)
#if !defined(OS_NACL_SFI)
,
channel_(Channel::Create(this,
std::move(connection_params),
channel_handle_policy,
- io_task_runner_))
+ std::move(io_task_runner)))
#endif
{
}
@@ -499,15 +499,10 @@ void NodeChannel::CreateAndBindLocalBrokerHost(
void NodeChannel::OnChannelMessage(const void* payload,
size_t payload_size,
std::vector<PlatformHandle> handles) {
- DCHECK(io_task_runner_->RunsTasksInCurrentSequence());
+ DCHECK(owning_task_runner()->RunsTasksInCurrentSequence());
RequestContext request_context(RequestContext::Source::SYSTEM);
- // Ensure this NodeChannel stays alive through the extent of this method. The
- // delegate may have the only other reference to this object and it may choose
- // to drop it here in response to, e.g., a malformed message.
- scoped_refptr<NodeChannel> keepalive = this;
-
if (payload_size <= sizeof(Header)) {
delegate_->OnChannelError(remote_node_name_, this);
return;
@@ -739,7 +734,7 @@ void NodeChannel::OnChannelMessage(const void* payload,
}
void NodeChannel::OnChannelError(Channel::Error error) {
- DCHECK(io_task_runner_->RunsTasksInCurrentSequence());
+ DCHECK(owning_task_runner()->RunsTasksInCurrentSequence());
RequestContext request_context(RequestContext::Source::SYSTEM);
diff --git a/mojo/core/node_channel.h b/mojo/core/node_channel.h
index ea91f927049befa258884b13e7f7966c09518687..04501da0fb6cd36df1b332135282104dd442b41e 100644
--- a/mojo/core/node_channel.h
+++ b/mojo/core/node_channel.h
@@ -11,7 +11,7 @@
#include "base/callback.h"
#include "base/containers/queue.h"
#include "base/macros.h"
-#include "base/memory/ref_counted.h"
+#include "base/memory/ref_counted_delete_on_sequence.h"
#include "base/process/process_handle.h"
#include "base/single_thread_task_runner.h"
#include "base/synchronization/lock.h"
@@ -21,13 +21,15 @@
#include "mojo/core/embedder/process_error_callback.h"
#include "mojo/core/ports/name.h"
#include "mojo/core/scoped_process_handle.h"
+#include "mojo/core/system_impl_export.h"
namespace mojo {
namespace core {
// Wraps a Channel to send and receive Node control messages.
-class NodeChannel : public base::RefCountedThreadSafe<NodeChannel>,
- public Channel::Delegate {
+class MOJO_SYSTEM_IMPL_EXPORT NodeChannel
+ : public base::RefCountedDeleteOnSequence<NodeChannel>,
+ public Channel::Delegate {
public:
class Delegate {
public:
@@ -92,8 +94,6 @@ class NodeChannel : public base::RefCountedThreadSafe<NodeChannel>,
void** data,
size_t* num_data_bytes);
- Channel* channel() const { return channel_.get(); }
-
// Start receiving messages.
void Start();
@@ -155,7 +155,8 @@ class NodeChannel : public base::RefCountedThreadSafe<NodeChannel>,
#endif
private:
- friend class base::RefCountedThreadSafe<NodeChannel>;
+ friend class base::RefCountedDeleteOnSequence<NodeChannel>;
+ friend class base::DeleteHelper<NodeChannel>;
using PendingMessageQueue = base::queue<Channel::MessagePtr>;
using PendingRelayMessageQueue =
@@ -181,13 +182,12 @@ class NodeChannel : public base::RefCountedThreadSafe<NodeChannel>,
void WriteChannelMessage(Channel::MessagePtr message);
Delegate* const delegate_;
- const scoped_refptr<base::SingleThreadTaskRunner> io_task_runner_;
const ProcessErrorCallback process_error_callback_;
base::Lock channel_lock_;
- scoped_refptr<Channel> channel_;
+ scoped_refptr<Channel> channel_ GUARDED_BY(channel_lock_);
- // Must only be accessed from |io_task_runner_|'s thread.
+ // Must only be accessed from the owning task runner's thread.
ports::NodeName remote_node_name_;
base::Lock remote_process_handle_lock_;
diff --git a/mojo/core/node_channel_fuzzer.cc b/mojo/core/node_channel_fuzzer.cc
index 99047c000dbe6be90f1d28b4b6817e608f9853a6..54fe757e0dec8aa138af96aa1a0afe596563c26c 100644
--- a/mojo/core/node_channel_fuzzer.cc
+++ b/mojo/core/node_channel_fuzzer.cc
@@ -14,6 +14,7 @@
#include "mojo/core/connection_params.h"
#include "mojo/core/entrypoints.h"
#include "mojo/core/node_channel.h" // nogncheck
+#include "mojo/core/test/mock_node_channel_delegate.h"
#include "mojo/public/cpp/platform/platform_channel.h"
#if defined(OS_WIN)
@@ -24,60 +25,6 @@ using mojo::core::Channel;
using mojo::core::ConnectionParams;
using mojo::core::ports::NodeName;
-// Implementation of NodeChannel::Delegate which does nothing. All of the
-// interesting NodeChannel control message message parsing is done by
-// NodeChannel by the time any of the delegate methods are invoked, so there's
-// no need for this to do any work.
-class FakeNodeChannelDelegate : public mojo::core::NodeChannel::Delegate {
- public:
- FakeNodeChannelDelegate() = default;
- ~FakeNodeChannelDelegate() override = default;
-
- void OnAcceptInvitee(const NodeName& from_node,
- const NodeName& inviter_name,
- const NodeName& token) override {}
- void OnAcceptInvitation(const NodeName& from_node,
- const NodeName& token,
- const NodeName& invitee_name) override {}
- void OnAddBrokerClient(const NodeName& from_node,
- const NodeName& client_name,
- base::ProcessHandle process_handle) override {}
- void OnBrokerClientAdded(const NodeName& from_node,
- const NodeName& client_name,
- mojo::PlatformHandle broker_channel) override {}
- void OnAcceptBrokerClient(const NodeName& from_node,
- const NodeName& broker_name,
- mojo::PlatformHandle broker_channel) override {}
- void OnEventMessage(const NodeName& from_node,
- Channel::MessagePtr message) override {}
- void OnRequestPortMerge(
- const NodeName& from_node,
- const mojo::core::ports::PortName& connector_port_name,
- const std::string& token) override {}
- void OnRequestIntroduction(const NodeName& from_node,
- const NodeName& name) override {}
- void OnIntroduce(const NodeName& from_node,
- const NodeName& name,
- mojo::PlatformHandle channel_handle) override {}
- void OnBroadcast(const NodeName& from_node,
- Channel::MessagePtr message) override {}
-#if defined(OS_WIN)
- void OnRelayEventMessage(const NodeName& from_node,
- base::ProcessHandle from_process,
- const NodeName& destination,
- Channel::MessagePtr message) override {}
- void OnEventMessageFromRelay(const NodeName& from_node,
- const NodeName& source_node,
- Channel::MessagePtr message) override {}
-#endif
- void OnAcceptPeer(const NodeName& from_node,
- const NodeName& token,
- const NodeName& peer_name,
- const mojo::core::ports::PortName& port_name) override {}
- void OnChannelError(const NodeName& node,
- mojo::core::NodeChannel* channel) override {}
-};
-
// A fake delegate for the sending Channel endpoint. The sending Channel is not
// being fuzzed and won't receive any interesting messages, so this doesn't need
// to do anything.
@@ -109,7 +56,7 @@ extern "C" int LLVMFuzzerTestOneInput(const unsigned char* data, size_t size) {
// used to carry messages between processes.
mojo::PlatformChannel channel;
- FakeNodeChannelDelegate receiver_delegate;
+ mojo::core::MockNodeChannelDelegate receiver_delegate;
auto receiver = mojo::core::NodeChannel::Create(
&receiver_delegate, ConnectionParams(channel.TakeLocalEndpoint()),
Channel::HandlePolicy::kRejectHandles,
diff --git a/mojo/core/node_channel_unittest.cc b/mojo/core/node_channel_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..13c46f13fea6342316534a7a843debbf7586108d
--- /dev/null
+++ b/mojo/core/node_channel_unittest.cc
@@ -0,0 +1,72 @@
+// Copyright 2020 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 "mojo/core/node_channel.h"
+
+#include "base/bind_helpers.h"
+#include "base/memory/scoped_refptr.h"
+#include "base/message_loop/message_pump_type.h"
+#include "base/test/task_environment.h"
+#include "base/threading/thread.h"
+#include "mojo/core/embedder/embedder.h"
+#include "mojo/core/test/mock_node_channel_delegate.h"
+#include "mojo/public/cpp/platform/platform_channel.h"
+#include "mojo/public/cpp/platform/platform_channel_endpoint.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace mojo {
+namespace core {
+namespace {
+
+using NodeChannelTest = testing::Test;
+using ports::NodeName;
+
+scoped_refptr<NodeChannel> CreateNodeChannel(NodeChannel::Delegate* delegate,
+ PlatformChannelEndpoint endpoint) {
+ return NodeChannel::Create(delegate, ConnectionParams(std::move(endpoint)),
+ Channel::HandlePolicy::kAcceptHandles,
+ GetIOTaskRunner(), base::NullCallback());
+}
+
+TEST_F(NodeChannelTest, DestructionIsSafe) {
+ // Regression test for https://crbug.com/1081874.
+ base::test::TaskEnvironment task_environment;
+
+ PlatformChannel channel;
+ MockNodeChannelDelegate local_delegate;
+ auto local_channel =
+ CreateNodeChannel(&local_delegate, channel.TakeLocalEndpoint());
+ local_channel->Start();
+ MockNodeChannelDelegate remote_delegate;
+ auto remote_channel =
+ CreateNodeChannel(&remote_delegate, channel.TakeRemoteEndpoint());
+ remote_channel->Start();
+
+ // Verify end-to-end operation
+ const NodeName kRemoteNodeName{123, 456};
+ const NodeName kToken{987, 654};
+ base::RunLoop loop;
+ EXPECT_CALL(local_delegate,
+ OnAcceptInvitee(ports::kInvalidNodeName, kRemoteNodeName, kToken))
+ .WillRepeatedly([&] { loop.Quit(); });
+ remote_channel->AcceptInvitee(kRemoteNodeName, kToken);
+ loop.Run();
+
+ // Now send another message to the local endpoint but tear it down
+ // immediately. This will race with the message being received on the IO
+ // thread, and although the corresponding delegate call may or may not
+ // dispatch as a result, the race should still be memory-safe.
+ remote_channel->AcceptInvitee(kRemoteNodeName, kToken);
+
+ base::RunLoop error_loop;
+ EXPECT_CALL(remote_delegate, OnChannelError).WillOnce([&] {
+ error_loop.Quit();
+ });
+ local_channel.reset();
+ error_loop.Run();
+}
+
+} // namespace
+} // namespace core
+} // namespace mojo
diff --git a/mojo/core/test/BUILD.gn b/mojo/core/test/BUILD.gn
index 1abadfc503d5176d2af1dcecfde153f17488546d..9429c61853059e10ca2430ad79ec8d9a39d90906 100644
--- a/mojo/core/test/BUILD.gn
+++ b/mojo/core/test/BUILD.gn
@@ -7,6 +7,8 @@ import("//third_party/protobuf/proto_library.gni")
static_library("test_support") {
testonly = true
sources = [
+ "mock_node_channel_delegate.cc",
+ "mock_node_channel_delegate.h",
"mojo_test_base.cc",
"mojo_test_base.h",
"test_utils.h",
@@ -27,8 +29,10 @@ static_library("test_support") {
public_deps = [
"//base",
"//base/test:test_support",
+ "//mojo/core:embedder_internal",
"//mojo/core/embedder",
"//mojo/public/cpp/system",
+ "//testing/gmock",
"//testing/gtest",
]
}
diff --git a/mojo/core/test/mock_node_channel_delegate.cc b/mojo/core/test/mock_node_channel_delegate.cc
new file mode 100644
index 0000000000000000000000000000000000000000..d257c3e1dc03857f91e94807328a0dc176f332f4
--- /dev/null
+++ b/mojo/core/test/mock_node_channel_delegate.cc
@@ -0,0 +1,15 @@
+// Copyright 2020 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 "mojo/core/test/mock_node_channel_delegate.h"
+
+namespace mojo {
+namespace core {
+
+MockNodeChannelDelegate::MockNodeChannelDelegate() = default;
+
+MockNodeChannelDelegate::~MockNodeChannelDelegate() = default;
+
+} // namespace core
+} // namespace mojo
diff --git a/mojo/core/test/mock_node_channel_delegate.h b/mojo/core/test/mock_node_channel_delegate.h
new file mode 100644
index 0000000000000000000000000000000000000000..06ca96857b9472682d577a802c68a56a7af0aacd
--- /dev/null
+++ b/mojo/core/test/mock_node_channel_delegate.h
@@ -0,0 +1,114 @@
+// Copyright 2020 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.
+
+#ifndef MOJO_CORE_TEST_MOCK_NODE_CHANNEL_DELEGATE_H_
+#define MOJO_CORE_TEST_MOCK_NODE_CHANNEL_DELEGATE_H_
+
+#include "build/build_config.h"
+#include "mojo/core/node_channel.h"
+#include "testing/gmock/include/gmock/gmock.h"
+
+namespace mojo {
+namespace core {
+
+// A NodeChannel Delegate implementation which can be used by NodeChannel unit
+// tests and fuzzers.
+class MockNodeChannelDelegate
+ : public testing::NiceMock<NodeChannel::Delegate> {
+ public:
+ using NodeName = ports::NodeName;
+ using PortName = ports::PortName;
+
+ MockNodeChannelDelegate();
+ MockNodeChannelDelegate(const MockNodeChannelDelegate&) = delete;
+ MockNodeChannelDelegate& operator=(const MockNodeChannelDelegate&) = delete;
+ ~MockNodeChannelDelegate() override;
+
+ // testing::NiceMock<NodeChannel::Delegate> implementation:
+ MOCK_METHOD(void,
+ OnAcceptInvitee,
+ (const NodeName& from_node,
+ const NodeName& inviter_name,
+ const NodeName& token),
+ (override));
+ MOCK_METHOD(void,
+ OnAcceptInvitation,
+ (const NodeName& from_node,
+ const NodeName& token,
+ const NodeName& invitee_name),
+ (override));
+ MOCK_METHOD(void,
+ OnAddBrokerClient,
+ (const NodeName& from_node,
+ const NodeName& client_name,
+ base::ProcessHandle process_handle),
+ (override));
+ MOCK_METHOD(void,
+ OnBrokerClientAdded,
+ (const NodeName& from_node,
+ const NodeName& client_name,
+ PlatformHandle broker_channel),
+ (override));
+ MOCK_METHOD(void,
+ OnAcceptBrokerClient,
+ (const NodeName& from_node,
+ const NodeName& broker_name,
+ PlatformHandle broker_channel),
+ (override));
+ MOCK_METHOD(void,
+ OnEventMessage,
+ (const NodeName& from_node, Channel::MessagePtr message),
+ (override));
+ MOCK_METHOD(void,
+ OnRequestPortMerge,
+ (const NodeName& from_node,
+ const PortName& connector_port_name,
+ const std::string& token),
+ (override));
+ MOCK_METHOD(void,
+ OnRequestIntroduction,
+ (const NodeName& from_node, const NodeName& name),
+ (override));
+ MOCK_METHOD(void,
+ OnIntroduce,
+ (const NodeName& from_node,
+ const NodeName& name,
+ PlatformHandle channel_handle),
+ (override));
+ MOCK_METHOD(void,
+ OnBroadcast,
+ (const NodeName& from_node, Channel::MessagePtr message),
+ (override));
+#if defined(OS_WIN)
+ MOCK_METHOD(void,
+ OnRelayEventMessage,
+ (const NodeName& from_node,
+ base::ProcessHandle from_process,
+ const NodeName& destination,
+ Channel::MessagePtr message),
+ (override));
+ MOCK_METHOD(void,
+ OnEventMessageFromRelay,
+ (const NodeName& from_node,
+ const NodeName& source_node,
+ Channel::MessagePtr message),
+ (override));
+#endif
+ MOCK_METHOD(void,
+ OnAcceptPeer,
+ (const NodeName& from_node,
+ const NodeName& token,
+ const NodeName& peer_name,
+ const PortName& port_name),
+ (override));
+ MOCK_METHOD(void,
+ OnChannelError,
+ (const NodeName& node, NodeChannel* channel),
+ (override));
+};
+
+} // namespace core
+} // namespace mojo
+
+#endif // MOJO_CORE_TEST_MOCK_NODE_CHANNEL_DELEGATE_H_