Browse Source

chore: [29-x-y] cherry-pick 3 changes from Release-1-M121

* d4a197e4913f from chromium
* 8755f76bec32 from chromium
* e321f354a613 from chromium
Shelley Vohr 1 year ago
parent
commit
29b675a48d

+ 3 - 0
patches/chromium/.patches

@@ -129,3 +129,6 @@ fix_restore_original_resize_performance_on_macos.patch
 feat_allow_code_cache_in_custom_schemes.patch
 build_run_reclient_cfg_generator_after_chrome.patch
 fix_suppress_clang_-wimplicit-const-int-float-conversion_in.patch
+cherry-pick-d4a197e4913f.patch
+cherry-pick-8755f76bec32.patch
+cherry-pick-e321f354a613.patch

+ 111 - 0
patches/chromium/cherry-pick-8755f76bec32.patch

@@ -0,0 +1,111 @@
+From 8755f76bec326c654370de6dd68eea693df74ede Mon Sep 17 00:00:00 2001
+From: Guido Urdaneta <[email protected]>
+Date: Thu, 18 Jan 2024 16:47:18 +0000
+Subject: [PATCH] [RTCPeerConnection] 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.
+
+Bug: 1514777
+Change-Id: I27e9541cfaa74d978799c03e2832a0980f9e5710
+Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5210359
+Reviewed-by: Tomas Gunnarsson <[email protected]>
+Commit-Queue: Guido Urdaneta <[email protected]>
+Cr-Commit-Position: refs/heads/main@{#1248826}
+---
+
+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 03f1b44..f08815c 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
+@@ -839,15 +839,19 @@
+     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_.Get();
++  if (!client_on_stack) {
++    return false;
++  }
++
++  DCHECK(frame);
++  frame_ = frame;
+   peer_connection_tracker_ = PeerConnectionTracker::From(*frame);
+ 
+   configuration_ = server_configuration;
+@@ -2058,10 +2062,13 @@
+                                               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_.Get();
+-  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>(
+diff --git a/third_party/blink/renderer/modules/peerconnection/rtc_peer_connection_handler_test.cc b/third_party/blink/renderer/modules/peerconnection/rtc_peer_connection_handler_test.cc
+index bbbed46..f872765 100644
+--- a/third_party/blink/renderer/modules/peerconnection/rtc_peer_connection_handler_test.cc
++++ b/third_party/blink/renderer/modules/peerconnection/rtc_peer_connection_handler_test.cc
+@@ -541,7 +541,7 @@
+     waitable_event->Signal();
+   }
+ 
+- public:
++ protected:
+   ScopedTestingPlatformSupport<AudioCapturerSourceTestingPlatformSupport>
+       webrtc_audio_device_platform_support_;
+   Persistent<MockRTCPeerConnectionHandlerClient> mock_client_;
+@@ -1054,4 +1054,32 @@
+   observer->OnIceCandidate(native_candidate.get());
+ }
+ 
++TEST_F(RTCPeerConnectionHandlerTest,
++       OnIceCandidateAfterClientGarbageCollectionDoesNothing) {
++  testing::InSequence sequence;
++  EXPECT_CALL(*mock_tracker_.Get(),
++              TrackAddIceCandidate(pc_handler_.get(), _,
++                                   PeerConnectionTracker::kSourceLocal, true))
++      .Times(0);
++
++  std::unique_ptr<webrtc::IceCandidateInterface> native_candidate(
++      mock_dependency_factory_->CreateIceCandidate("sdpMid", 1, kDummySdp));
++  mock_client_ = nullptr;
++  WebHeap::CollectAllGarbageForTesting();
++  pc_handler_->observer()->OnIceCandidate(native_candidate.get());
++  RunMessageLoopsUntilIdle();
++}
++
++TEST_F(RTCPeerConnectionHandlerTest,
++       OnIceCandidateAfterClientGarbageCollectionFails) {
++  DummyExceptionStateForTesting exception_state;
++  auto pc_handler = CreateRTCPeerConnectionHandlerUnderTest();
++  mock_client_ = nullptr;
++  WebHeap::CollectAllGarbageForTesting();
++  EXPECT_FALSE(pc_handler->Initialize(
++      /*context=*/nullptr, webrtc::PeerConnectionInterface::RTCConfiguration(),
++      /*media_constraints=*/nullptr,
++      /*frame=*/nullptr, exception_state));
++}
++
+ }  // namespace blink

+ 120 - 0
patches/chromium/cherry-pick-d4a197e4913f.patch

@@ -0,0 +1,120 @@
+From d4a197e4913f8e5072263b59aedc29f2b5af3e93 Mon Sep 17 00:00:00 2001
+From: Jean-Philippe Gravel <[email protected]>
+Date: Wed, 17 Jan 2024 17:45:45 +0000
+Subject: [PATCH] Fix use-after-free in DrawTextInternal
+
+DrawTextInternal was calling GetOrCreatePaintCanvas multiple times,
+once at the start of the function, once inside of the
+BaseRenderingContext2DAutoRestoreSkCanvas helper class and once in the
+Draw call. GetOrCreatePaintCanvas destroys the canvas resource provider
+if the GPU context is lost. If this happens on the second call to
+GetOrCreatePaintCanvas, destroying the resource provider will
+invalidate the cc::PaintCanvas returned by the first call to
+GetOrCreatePaintCanvas.
+
+The GPU process can technically crash at any point during the renderer
+process execution (perhaps because of something another renderer
+process did). We therefore have to assume that any call to
+GetOrCreatePaintCanvas can invalidate previously returned
+cc::PaintCanvas.
+
+Change-Id: Ifa77735ab1b2b55b3d494f886b8566299937f6fe
+Fixed: 1511567
+Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5198419
+Reviewed-by: Fernando Serboncini <[email protected]>
+Commit-Queue: Jean-Philippe Gravel <[email protected]>
+Cr-Commit-Position: refs/heads/main@{#1248204}
+---
+
+diff --git a/third_party/blink/renderer/modules/canvas/canvas2d/base_rendering_context_2d.cc b/third_party/blink/renderer/modules/canvas/canvas2d/base_rendering_context_2d.cc
+index a3c71e6..f024973 100644
+--- a/third_party/blink/renderer/modules/canvas/canvas2d/base_rendering_context_2d.cc
++++ b/third_party/blink/renderer/modules/canvas/canvas2d/base_rendering_context_2d.cc
+@@ -2866,40 +2866,6 @@
+   return state.GetFont();
+ }
+ 
+-namespace {
+-
+-// Drawing methods need to use this instead of SkAutoCanvasRestore in case
+-// overdraw detection substitutes the recording canvas (to discard overdrawn
+-// draw calls).
+-class BaseRenderingContext2DAutoRestoreSkCanvas {
+-  STACK_ALLOCATED();
+-
+- public:
+-  explicit BaseRenderingContext2DAutoRestoreSkCanvas(
+-      BaseRenderingContext2D* context)
+-      : context_(context) {
+-    DCHECK(context_);
+-    cc::PaintCanvas* c = context_->GetOrCreatePaintCanvas();
+-    if (c) {
+-      save_count_ = c->getSaveCount();
+-    }
+-  }
+-
+-  ~BaseRenderingContext2DAutoRestoreSkCanvas() {
+-    cc::PaintCanvas* c = context_->GetOrCreatePaintCanvas();
+-    if (c) {
+-      c->restoreToCount(save_count_);
+-    }
+-    context_->ValidateStateStack();
+-  }
+-
+- private:
+-  BaseRenderingContext2D* context_;
+-  int save_count_ = 0;
+-};
+-
+-}  // namespace
+-
+ void BaseRenderingContext2D::DrawTextInternal(
+     const String& text,
+     double x,
+@@ -2920,8 +2886,10 @@
+     canvas->GetDocument().UpdateStyleAndLayoutTreeForElement(
+         canvas, DocumentUpdateReason::kCanvas);
+   }
+-  cc::PaintCanvas* c = GetOrCreatePaintCanvas();
+-  if (!c) {
++
++  // Abort if we don't have a paint canvas (e.g. the context was lost).
++  cc::PaintCanvas* paint_canvas = GetOrCreatePaintCanvas();
++  if (!paint_canvas) {
+     return;
+   }
+ 
+@@ -2993,14 +2961,13 @@
+     InflateStrokeRect(bounds);
+   }
+ 
+-  BaseRenderingContext2DAutoRestoreSkCanvas state_restorer(this);
+   if (use_max_width) {
+-    c->save();
++    paint_canvas->save();
+     // We draw when fontWidth is 0 so compositing operations (eg, a "copy" op)
+     // still work. As the width of canvas is scaled, so text can be scaled to
+     // match the given maxwidth, update text location so it appears on desired
+     // place.
+-    c->scale(ClampTo<float>(width / font_width), 1);
++    paint_canvas->scale(ClampTo<float>(width / font_width), 1);
+     location.set_x(location.x() / ClampTo<float>(width / font_width));
+   }
+ 
+@@ -3031,6 +2998,16 @@
+       { return false; },
+       bounds, paint_type, CanvasRenderingContext2DState::kNoImage,
+       CanvasPerformanceMonitor::DrawType::kText);
++
++  if (use_max_width) {
++    // Cannot use `paint_canvas` in case recording canvas was substituted or
++    // destroyed during draw call.
++    cc::PaintCanvas* c = GetPaintCanvas();
++    if (c) {
++      c->restore();
++    }
++  }
++  ValidateStateStack();
+ }
+ 
+ TextMetrics* BaseRenderingContext2D::measureText(const String& text) {

+ 126 - 0
patches/chromium/cherry-pick-e321f354a613.patch

@@ -0,0 +1,126 @@
+From e321f354a6137a45a7bffb5485a7fcd0ca61ff8a Mon Sep 17 00:00:00 2001
+From: Tsuyoshi Horo <[email protected]>
+Date: Wed, 24 Jan 2024 00:59:57 +0000
+Subject: [PATCH] [M121] 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 <[email protected]>
+Commit-Queue: Tsuyoshi Horo <[email protected]>
+Cr-Original-Commit-Position: refs/heads/main@{#1244526}
+Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5231537
+Reviewed-by: Kenichi Ishibashi <[email protected]>
+Cr-Commit-Position: refs/branch-heads/6167@{#1621}
+Cr-Branched-From: 222e786949e76e342d325ea0d008b4b6273f3a89-refs/heads/main@{#1233107}
+---
+
+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 f1dca731..c3700ab 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 @@
+   }
+   int num_bytes = base::checked_cast<int>(pending_write_->size());
+   auto buffer = base::MakeRefCounted<NetToMojoIOBuffer>(pending_write_);
+-  int result = source_->Read(
+-      buffer.get(), num_bytes,
+-      base::BindOnce(&SourceStreamToDataPipe::DidRead, base::Unretained(this)));
++  int result = source_->Read(buffer.get(), 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 7061418..54159df 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 @@
+   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 @@
+   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