electron · codebytere · Feb 2, 2024 · Jan 30, 2024 · Feb 1, 2024
@@ -158,3 +158,6 @@ reland_mojom_ts_generator_handle_empty_module_path_identically_to.patch
 cherry-pick-c1cda70a433a.patch
 cherry-pick-cc07a95bc309.patch
 safely_crash_on_dangling_profile.patch
+cherry-pick-ee0b8769f428.patch
+cherry-pick-1f8bec968902.patch
+cherry-pick-4a98f9e304be.patch
@@ -0,0 +1,125 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Tsuyoshi Horo <horo@chromium.org>
+Date: Wed, 24 Jan 2024 02:04:24 +0000
+Subject: Fix UAF in SourceStreamToDataPipe
+
+SourceStreamToDataPipe::ReadMore() is passing a callback with
+Unretained(this) to net::SourceStream::Read(). But this callback may be
+called even after the SourceStream is destructed. This is causing UAF
+issue (crbug.com/1511085).
+
+To solve this problem, this CL changes ReadMore() method to pass a
+callback with a weak ptr of this.
+
+(cherry picked from commit 6e36a69da1b73f9aea9c54bfbe6c5b9cb2c672a5)
+
+Bug: 1511085
+Change-Id: Idd4e34ff300ff5db2de1de7b303841c7db3a964a
+Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5179746
+Reviewed-by: Adam Rice <ricea@chromium.org>
+Commit-Queue: Tsuyoshi Horo <horo@chromium.org>
+Cr-Original-Commit-Position: refs/heads/main@{#1244526}
+Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5231558
+Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
+Cr-Commit-Position: refs/branch-heads/6099@{#1860}
+Cr-Branched-From: e6ee4500f7d6549a9ac1354f8d056da49ef406be-refs/heads/main@{#1217362}
+
+diff --git a/services/network/public/cpp/source_stream_to_data_pipe.cc b/services/network/public/cpp/source_stream_to_data_pipe.cc
+index bfd85b1a00b216b52ae816ca29cb66ddabe20b6d..07afd58a40f92485ded07c535092a891c5140c7b 100644
+--- a/services/network/public/cpp/source_stream_to_data_pipe.cc
++++ b/services/network/public/cpp/source_stream_to_data_pipe.cc
+@@ -55,9 +55,9 @@ void SourceStreamToDataPipe::ReadMore() {
+
+   scoped_refptr<net::IOBuffer> buffer(
+       new network::NetToMojoIOBuffer(pending_write_.get()));
+-  int result = source_->Read(
+-      buffer.get(), base::checked_cast<int>(num_bytes),
+-      base::BindOnce(&SourceStreamToDataPipe::DidRead, base::Unretained(this)));
++  int result = source_->Read(buffer.get(), base::checked_cast<int>(num_bytes),
++                             base::BindOnce(&SourceStreamToDataPipe::DidRead,
++                                            weak_factory_.GetWeakPtr()));
+
+   if (result != net::ERR_IO_PENDING)
+     DidRead(result);
+diff --git a/services/network/public/cpp/source_stream_to_data_pipe_unittest.cc b/services/network/public/cpp/source_stream_to_data_pipe_unittest.cc
+index 7061418c5141d936f04b1193c98e66efc5e72ac5..54159df39afa7cf6e2faa51da185dc034b923209 100644
+--- a/services/network/public/cpp/source_stream_to_data_pipe_unittest.cc
++++ b/services/network/public/cpp/source_stream_to_data_pipe_unittest.cc
+@@ -6,7 +6,9 @@
+
+ #include "base/functional/bind.h"
+ #include "base/memory/raw_ptr.h"
++#include "base/test/bind.h"
+ #include "base/test/task_environment.h"
++#include "net/base/net_errors.h"
+ #include "net/filter/mock_source_stream.h"
+ #include "testing/gtest/include/gtest/gtest.h"
+ #include "third_party/abseil-cpp/absl/types/optional.h"
+@@ -42,6 +44,33 @@ struct SourceStreamToDataPipeTestParam {
+   const ReadResultType read_result_type;
+ };
+
++class DummyPendingSourceStream : public net::SourceStream {
++ public:
++  DummyPendingSourceStream() : net::SourceStream(SourceStream::TYPE_NONE) {}
++  ~DummyPendingSourceStream() override = default;
++
++  DummyPendingSourceStream(const DummyPendingSourceStream&) = delete;
++  DummyPendingSourceStream& operator=(const DummyPendingSourceStream&) = delete;
++
++  // SourceStream implementation
++  int Read(net::IOBuffer* dest_buffer,
++           int buffer_size,
++           net::CompletionOnceCallback callback) override {
++    callback_ = std::move(callback);
++    return net::ERR_IO_PENDING;
++  }
++  std::string Description() const override { return ""; }
++  bool MayHaveMoreBytes() const override { return true; }
++
++  net::CompletionOnceCallback TakeCompletionCallback() {
++    CHECK(callback_);
++    return std::move(callback_);
++  }
++
++ private:
++  net::CompletionOnceCallback callback_;
++};
++
+ }  // namespace
+
+ class SourceStreamToDataPipeTest
+@@ -212,4 +241,33 @@ TEST_P(SourceStreamToDataPipeTest, MayHaveMoreBytes) {
+   EXPECT_EQ(ReadPipe(&output), net::OK);
+   EXPECT_EQ(output, message);
+ }
++
++TEST(SourceStreamToDataPipeCallbackTest, CompletionCallbackAfterDestructed) {
++  base::test::TaskEnvironment task_environment;
++
++  std::unique_ptr<DummyPendingSourceStream> source =
++      std::make_unique<DummyPendingSourceStream>();
++  DummyPendingSourceStream* source_ptr = source.get();
++  const MojoCreateDataPipeOptions data_pipe_options{
++      sizeof(MojoCreateDataPipeOptions), MOJO_CREATE_DATA_PIPE_FLAG_NONE, 1, 1};
++  mojo::ScopedDataPipeProducerHandle producer_end;
++  mojo::ScopedDataPipeConsumerHandle consumer_end;
++  CHECK_EQ(MOJO_RESULT_OK, mojo::CreateDataPipe(&data_pipe_options,
++                                                producer_end, consumer_end));
++
++  std::unique_ptr<SourceStreamToDataPipe> adapter =
++      std::make_unique<SourceStreamToDataPipe>(std::move(source),
++                                               std::move(producer_end));
++  bool callback_called = false;
++  adapter->Start(
++      base::BindLambdaForTesting([&](int result) { callback_called = true; }));
++  net::CompletionOnceCallback callback = source_ptr->TakeCompletionCallback();
++  adapter.reset();
++
++  // Test that calling `callback` after deleting `adapter` must not cause UAF
++  // (crbug.com/1511085).
++  std::move(callback).Run(net::ERR_FAILED);
++  EXPECT_FALSE(callback_called);
++}
++
+ }  // namespace network
@@ -0,0 +1,67 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: =?UTF-8?q?Peter=20Bostr=C3=B6m?= <pbos@chromium.org>
+Date: Fri, 26 Jan 2024 19:37:57 +0000
+Subject: Speculatively fix race in mojo ShutDownOnIOThread
+MIME-Version: 1.0
+Content-Type: text/plain; charset=UTF-8
+Content-Transfer-Encoding: 8bit
+
+This acquires `write_lock_` before resetting handles used by WriteNoLock
+(which is called under the same lock in another thread). We also set
+`reject_writes_` to prevent future write attempts after shutdown. That
+seems strictly more correct.
+
+We also acquire `fds_to_close_lock_` before clearing the FDs.
+
+I was unable to repro locally as content_browsertests just times out
+in my local setup without reporting anything interesting. This seems
+strictly more correct though.
+
+(cherry picked from commit 9755d9d81e4a8cb5b4f76b23b761457479dbb06b)
+
+Bug: 1519980
+Change-Id: I96279936ca908ecb98eddd381df20d61597cba43
+Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5226127
+Auto-Submit: Peter Boström <pbos@chromium.org>
+Reviewed-by: Ken Rockot <rockot@google.com>
+Commit-Queue: Ken Rockot <rockot@google.com>
+Commit-Queue: Peter Boström <pbos@chromium.org>
+Cr-Original-Commit-Position: refs/heads/main@{#1250580}
+Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5239564
+Cr-Commit-Position: refs/branch-heads/6099@{#1883}
+Cr-Branched-From: e6ee4500f7d6549a9ac1354f8d056da49ef406be-refs/heads/main@{#1217362}
+
+diff --git a/mojo/core/channel_posix.cc b/mojo/core/channel_posix.cc
+index 0a3596382d0e9a40c72bfb4ead6f0338a61253d6..eae6b0768463679b5043514dc5745da52b80ae10 100644
+--- a/mojo/core/channel_posix.cc
++++ b/mojo/core/channel_posix.cc
+@@ -246,16 +246,21 @@ void ChannelPosix::WaitForWriteOnIOThreadNoLock() {
+ void ChannelPosix::ShutDownOnIOThread() {
+   base::CurrentThread::Get()->RemoveDestructionObserver(this);
+
+-  read_watcher_.reset();
+-  write_watcher_.reset();
+-  if (leak_handle_) {
+-    std::ignore = socket_.release();
+-  } else {
+-    socket_.reset();
+-  }
++  {
++    base::AutoLock lock(write_lock_);
++    reject_writes_ = true;
++    read_watcher_.reset();
++    write_watcher_.reset();
++    if (leak_handle_) {
++      std::ignore = socket_.release();
++    } else {
++      socket_.reset();
++    }
+ #if BUILDFLAG(IS_IOS)
+-  fds_to_close_.clear();
++    base::AutoLock fd_lock(fds_to_close_lock_);
++    fds_to_close_.clear();
+ #endif
++  }
+
+   // May destroy the |this| if it was the last reference.
+   self_ = nullptr;
@@ -0,0 +1,72 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Guido Urdaneta <guidou@chromium.org>
+Date: Wed, 24 Jan 2024 18:40:01 +0000
+Subject: Exit early from RTCPeerConnectionHandler
+
+For certain operations that require a live client
+(i.e., RTCPeerConnection, which is garbage collected),
+PeerConnectionHandler keeps a pointer to the client on the stack
+to prevent garbage collection.
+
+In some cases, the client may have already been garbage collected
+(the client is null). In that case, there is no point in doing the
+operation and it should exit early to avoid UAF/crashes.
+
+This CL adds early exit to the cases that do not already have it.
+
+(cherry picked from commit 8755f76bec326c654370de6dd68eea693df74ede)
+
+Bug: 1514777
+Change-Id: I27e9541cfaa74d978799c03e2832a0980f9e5710
+Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5210359
+Reviewed-by: Tomas Gunnarsson <tommi@chromium.org>
+Commit-Queue: Guido Urdaneta <guidou@chromium.org>
+Cr-Original-Commit-Position: refs/heads/main@{#1248826}
+Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5233883
+Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
+Auto-Submit: Guido Urdaneta <guidou@chromium.org>
+Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
+Cr-Commit-Position: refs/branch-heads/6099@{#1867}
+Cr-Branched-From: e6ee4500f7d6549a9ac1354f8d056da49ef406be-refs/heads/main@{#1217362}
+
+diff --git a/third_party/blink/renderer/modules/peerconnection/rtc_peer_connection_handler.cc b/third_party/blink/renderer/modules/peerconnection/rtc_peer_connection_handler.cc
+index 76fa93800543ff134859c8fc0c0fa63123cf9772..9e5ce0572cfd1d2dd729e5f560b021aba05653f3 100644
+--- a/third_party/blink/renderer/modules/peerconnection/rtc_peer_connection_handler.cc
++++ b/third_party/blink/renderer/modules/peerconnection/rtc_peer_connection_handler.cc
+@@ -1057,15 +1057,19 @@ bool RTCPeerConnectionHandler::Initialize(
+     WebLocalFrame* frame,
+     ExceptionState& exception_state) {
+   DCHECK(task_runner_->RunsTasksInCurrentSequence());
+-  DCHECK(frame);
+   DCHECK(dependency_factory_);
+-  frame_ = frame;
+
+   CHECK(!initialize_called_);
+   initialize_called_ = true;
+
+   // Prevent garbage collection of client_ during processing.
+   auto* client_on_stack = client_;
++  if (!client_on_stack) {
++    return false;
++  }
++
++  DCHECK(frame);
++  frame_ = frame;
+   peer_connection_tracker_ = PeerConnectionTracker::From(*frame);
+
+   configuration_ = server_configuration;
+@@ -2312,10 +2316,13 @@ void RTCPeerConnectionHandler::OnIceCandidate(const String& sdp,
+                                               int sdp_mline_index,
+                                               int component,
+                                               int address_family) {
++  DCHECK(task_runner_->RunsTasksInCurrentSequence());
+   // In order to ensure that the RTCPeerConnection is not garbage collected
+   // from under the function, we keep a pointer to it on the stack.
+   auto* client_on_stack = client_;
+-  DCHECK(task_runner_->RunsTasksInCurrentSequence());
++  if (!client_on_stack) {
++    return;
++  }
+   TRACE_EVENT0("webrtc", "RTCPeerConnectionHandler::OnIceCandidateImpl");
+   // This line can cause garbage collection.
+   auto* platform_candidate = MakeGarbageCollected<RTCIceCandidatePlatform>(