Browse Source

chore: cherry-pick 51daffbf5cd8 from chromium (#35549)

* chore: [20-x-y] cherry-pick 51daffbf5cd8 from chromium

* chore: update patches

* chore: update patches

Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com>
Co-authored-by: Electron Bot <[email protected]>
Pedro Pontes 2 years ago
parent
commit
85ae4c4a77

+ 1 - 1
patches/chromium/.patches

@@ -121,4 +121,4 @@ fix_windows_build_with_enable_plugins_false.patch
 remove_default_window_title.patch
 add_electron_deps_to_license_credits_file.patch
 feat_add_set_can_resize_mutator.patch
-cherry-pick-9b5207569882.patch
+cherry-pick-51daffbf5cd8.patch

+ 45 - 0
patches/chromium/cherry-pick-51daffbf5cd8.patch

@@ -0,0 +1,45 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Yutaka Hirano <[email protected]>
+Date: Mon, 4 Jul 2022 11:48:20 +0000
+Subject: Fix UAF on network::URLLoader
+
+network::URLLoader::SetUpUpload calls NotifyCompleted asynchronously,
+as it can be called in the constructor and we don't want to run
+NotifyCompleted in the constructor.
+
+The problem is that it attaches a raw pointer to the method, which leads to a use-after-free problem if the URLLoader is destructed before
+NotifyCompleted is called.
+
+Use weak pointers instead of raw pointers to avoid the problem.
+
+Bug: 1340253
+Change-Id: Iacb1e772bf7a8e3de4a7bb9de342fea9ba0f3f3c
+Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3740150
+Reviewed-by: Kenichi Ishibashi <[email protected]>
+Commit-Queue: Yutaka Hirano <[email protected]>
+Cr-Commit-Position: refs/heads/main@{#1020539}
+
+diff --git a/services/network/url_loader.cc b/services/network/url_loader.cc
+index 62201fd4f11ce500b3a9d82fffe7776bf8a15eb0..433edfd8be00d912ffe482283d0e0d1b093b8039 100644
+--- a/services/network/url_loader.cc
++++ b/services/network/url_loader.cc
+@@ -989,8 +989,8 @@ void URLLoader::OpenFilesForUpload(const ResourceRequest& request) {
+     // initializing before getting deleted.
+     base::SequencedTaskRunnerHandle::Get()->PostTask(
+         FROM_HERE,
+-        base::BindOnce(&URLLoader::NotifyCompleted, base::Unretained(this),
+-                       net::ERR_ACCESS_DENIED));
++        base::BindOnce(&URLLoader::NotifyCompleted,
++                       weak_ptr_factory_.GetWeakPtr(), net::ERR_ACCESS_DENIED));
+     return;
+   }
+   url_request_->LogBlockedBy("Opening Files");
+@@ -1009,7 +1009,7 @@ void URLLoader::SetUpUpload(const ResourceRequest& request,
+     // initializing before getting deleted.
+     base::SequencedTaskRunnerHandle::Get()->PostTask(
+         FROM_HERE, base::BindOnce(&URLLoader::NotifyCompleted,
+-                                  base::Unretained(this), error_code));
++                                  weak_ptr_factory_.GetWeakPtr(), error_code));
+     return;
+   }
+   scoped_refptr<base::SequencedTaskRunner> task_runner =

+ 0 - 179
patches/chromium/cherry-pick-9b5207569882.patch

@@ -1,179 +0,0 @@
-From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
-From: Ken Rockot <[email protected]>
-Date: Wed, 31 Aug 2022 15:39:45 +0000
-Subject: Mojo: Validate response message type
-
-Ensures that a response message is actually the type expected by the
-original request.
-
-Fixed: 1358134
-Change-Id: I8f8f58168764477fbf7a6d2e8aeb040f07793d45
-Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3864274
-Reviewed-by: Robert Sesek <[email protected]>
-Commit-Queue: Ken Rockot <[email protected]>
-Cr-Commit-Position: refs/heads/main@{#1041553}
-
-diff --git a/mojo/public/cpp/bindings/interface_endpoint_client.h b/mojo/public/cpp/bindings/interface_endpoint_client.h
-index df33a20bf5872d5dde43024e3a0722ed4a1d75a3..dcc6e2aa8d9e97ff716d6ab1de02a83eba95eec2 100644
---- a/mojo/public/cpp/bindings/interface_endpoint_client.h
-+++ b/mojo/public/cpp/bindings/interface_endpoint_client.h
-@@ -221,20 +221,32 @@ class COMPONENT_EXPORT(MOJO_CPP_BINDINGS) InterfaceEndpointClient
-   void ForgetAsyncRequest(uint64_t request_id);
- 
-  private:
--  // Maps from the id of a response to the MessageReceiver that handles the
--  // response.
--  using AsyncResponderMap =
--      std::map<uint64_t, std::unique_ptr<MessageReceiver>>;
-+  struct PendingAsyncResponse {
-+   public:
-+    PendingAsyncResponse(uint32_t request_message_name,
-+                         std::unique_ptr<MessageReceiver> responder);
-+    PendingAsyncResponse(PendingAsyncResponse&&);
-+    PendingAsyncResponse(const PendingAsyncResponse&) = delete;
-+    PendingAsyncResponse& operator=(PendingAsyncResponse&&);
-+    PendingAsyncResponse& operator=(const PendingAsyncResponse&) = delete;
-+    ~PendingAsyncResponse();
-+
-+    uint32_t request_message_name;
-+    std::unique_ptr<MessageReceiver> responder;
-+  };
-+
-+  using AsyncResponderMap = std::map<uint64_t, PendingAsyncResponse>;
- 
-   struct SyncResponseInfo {
-    public:
--    explicit SyncResponseInfo(bool* in_response_received);
-+    SyncResponseInfo(uint32_t request_message_name, bool* in_response_received);
- 
-     SyncResponseInfo(const SyncResponseInfo&) = delete;
-     SyncResponseInfo& operator=(const SyncResponseInfo&) = delete;
- 
-     ~SyncResponseInfo();
- 
-+    uint32_t request_message_name;
-     Message response;
- 
-     // Points to a stack-allocated variable.
-diff --git a/mojo/public/cpp/bindings/lib/interface_endpoint_client.cc b/mojo/public/cpp/bindings/lib/interface_endpoint_client.cc
-index b9db8f31a42b956c6125852cf162dc524d5308e6..6e87db197c603d8ac44b591f2cd70023217dcbe2 100644
---- a/mojo/public/cpp/bindings/lib/interface_endpoint_client.cc
-+++ b/mojo/public/cpp/bindings/lib/interface_endpoint_client.cc
-@@ -28,6 +28,7 @@
- #include "mojo/public/cpp/bindings/sync_call_restrictions.h"
- #include "mojo/public/cpp/bindings/sync_event_watcher.h"
- #include "mojo/public/cpp/bindings/thread_safe_proxy.h"
-+#include "third_party/abseil-cpp/absl/types/optional.h"
- #include "third_party/perfetto/protos/perfetto/trace/track_event/chrome_mojo_event_info.pbzero.h"
- 
- namespace mojo {
-@@ -314,9 +315,27 @@ class ResponderThunk : public MessageReceiverWithStatus {
- 
- // ----------------------------------------------------------------------------
- 
-+InterfaceEndpointClient::PendingAsyncResponse::PendingAsyncResponse(
-+    uint32_t request_message_name,
-+    std::unique_ptr<MessageReceiver> responder)
-+    : request_message_name(request_message_name),
-+      responder(std::move(responder)) {}
-+
-+InterfaceEndpointClient::PendingAsyncResponse::PendingAsyncResponse(
-+    PendingAsyncResponse&&) = default;
-+
-+InterfaceEndpointClient::PendingAsyncResponse&
-+InterfaceEndpointClient::PendingAsyncResponse::operator=(
-+    PendingAsyncResponse&&) = default;
-+
-+InterfaceEndpointClient::PendingAsyncResponse::~PendingAsyncResponse() =
-+    default;
-+
- InterfaceEndpointClient::SyncResponseInfo::SyncResponseInfo(
-+    uint32_t request_message_name,
-     bool* in_response_received)
--    : response_received(in_response_received) {}
-+    : request_message_name(request_message_name),
-+      response_received(in_response_received) {}
- 
- InterfaceEndpointClient::SyncResponseInfo::~SyncResponseInfo() {}
- 
-@@ -604,6 +623,7 @@ bool InterfaceEndpointClient::SendMessageWithResponder(
-   // message before calling |SendMessage()| below.
- #endif
- 
-+  const uint32_t message_name = message->name();
-   const bool is_sync = message->has_flag(Message::kFlagIsSync);
-   const bool exclusive_wait = message->has_flag(Message::kFlagNoInterrupt);
-   if (!controller_->SendMessage(message))
-@@ -620,7 +640,8 @@ bool InterfaceEndpointClient::SendMessageWithResponder(
-       controller_->RegisterExternalSyncWaiter(request_id);
-     }
-     base::AutoLock lock(async_responders_lock_);
--    async_responders_[request_id] = std::move(responder);
-+    async_responders_.emplace(
-+        request_id, PendingAsyncResponse{message_name, std::move(responder)});
-     return true;
-   }
- 
-@@ -628,7 +649,8 @@ bool InterfaceEndpointClient::SendMessageWithResponder(
- 
-   bool response_received = false;
-   sync_responses_.insert(std::make_pair(
--      request_id, std::make_unique<SyncResponseInfo>(&response_received)));
-+      request_id,
-+      std::make_unique<SyncResponseInfo>(message_name, &response_received)));
- 
-   base::WeakPtr<InterfaceEndpointClient> weak_self =
-       weak_ptr_factory_.GetWeakPtr();
-@@ -806,13 +828,13 @@ void InterfaceEndpointClient::ResetFromAnotherSequenceUnsafe() {
- }
- 
- void InterfaceEndpointClient::ForgetAsyncRequest(uint64_t request_id) {
--  std::unique_ptr<MessageReceiver> responder;
-+  absl::optional<PendingAsyncResponse> response;
-   {
-     base::AutoLock lock(async_responders_lock_);
-     auto it = async_responders_.find(request_id);
-     if (it == async_responders_.end())
-       return;
--    responder = std::move(it->second);
-+    response = std::move(it->second);
-     async_responders_.erase(it);
-   }
- }
-@@ -893,6 +915,10 @@ bool InterfaceEndpointClient::HandleValidatedMessage(Message* message) {
-         return false;
- 
-       if (it->second) {
-+        if (message->name() != it->second->request_message_name) {
-+          return false;
-+        }
-+
-         it->second->response = std::move(*message);
-         *it->second->response_received = true;
-         return true;
-@@ -903,18 +929,22 @@ bool InterfaceEndpointClient::HandleValidatedMessage(Message* message) {
-       sync_responses_.erase(it);
-     }
- 
--    std::unique_ptr<MessageReceiver> responder;
-+    absl::optional<PendingAsyncResponse> pending_response;
-     {
-       base::AutoLock lock(async_responders_lock_);
-       auto it = async_responders_.find(request_id);
-       if (it == async_responders_.end())
-         return false;
--      responder = std::move(it->second);
-+      pending_response = std::move(it->second);
-       async_responders_.erase(it);
-     }
- 
-+    if (message->name() != pending_response->request_message_name) {
-+      return false;
-+    }
-+
-     internal::MessageDispatchContext dispatch_context(message);
--    return responder->Accept(message);
-+    return pending_response->responder->Accept(message);
-   } else {
-     if (mojo::internal::ControlMessageHandler::IsControlMessage(message))
-       return control_message_handler_.Accept(message);