[Backport] CVE-2023-2459: Inappropriate implementation in Prompts

Manual backport of patch originally reviewed on
https://chromium-review.googlesource.com/c/chromium/src/+/4365724:
Use weak pointer to store duplicate requests

Bug: 1423304
Change-Id: I7ab170f085c3d05c582f7065b88c1ad2510cc633
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4365724
Commit-Queue: Thomas Nguyen <tungnh@chromium.org>
Reviewed-by: Andy Paicu <andypaicu@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1124133}
Reviewed-on: https://codereview.qt-project.org/c/qt/qtwebengine-chromium/+/476753
Reviewed-by: Michal Klocek <michal.klocek@qt.io>

4 files changed, 131 insertions, 23 deletions

diff --git a/chromium/components/permissions/permission_request.cc b/chromium/components/permissions/permission_request.cc
index 283538068f3..e85e7d044a5 100644
--- a/chromium/components/permissions/permission_request.cc
+++ b/chromium/components/permissions/permission_request.cc
@@ -40,6 +40,10 @@ bool PermissionRequest::IsDuplicateOf(PermissionRequest* other_request) const {
          requesting_origin() == other_request->requesting_origin();
 }
 
+base::WeakPtr<PermissionRequest> PermissionRequest::GetWeakPtr() {
+  return weak_factory_.GetWeakPtr();
+}
+
 #if BUILDFLAG(IS_ANDROID)
 std::u16string PermissionRequest::GetDialogMessageText() const {
   int message_id = 0;
diff --git a/chromium/components/permissions/permission_request.h b/chromium/components/permissions/permission_request.h
index d688ff5dacf..d983e42e9f7 100644
--- a/chromium/components/permissions/permission_request.h
+++ b/chromium/components/permissions/permission_request.h
@@ -8,6 +8,7 @@
 #include <string>
 
 #include "base/callback.h"
+#include "base/memory/weak_ptr.h"
 #include "build/build_config.h"
 #include "components/content_settings/core/common/content_settings.h"
 #include "components/content_settings/core/common/content_settings_types.h"
@@ -75,6 +76,9 @@ class PermissionRequest {
   // need to be shown in the UI.
   virtual bool IsDuplicateOf(PermissionRequest* other_request) const;
 
+  // Returns a weak pointer to this instance.
+  base::WeakPtr<PermissionRequest> GetWeakPtr();
+
 #if BUILDFLAG(IS_ANDROID)
   // Returns prompt text appropriate for displaying in an Android dialog.
   virtual std::u16string GetDialogMessageText() const;
@@ -150,6 +154,8 @@ class PermissionRequest {
   // Called when the request is no longer in use so it can be deleted by the
   // caller.
   base::OnceClosure delete_callback_;
+
+  base::WeakPtrFactory<PermissionRequest> weak_factory_{this};
 };
 
 }  // namespace permissions
diff --git a/chromium/components/permissions/permission_request_manager.cc b/chromium/components/permissions/permission_request_manager.cc
index 75d014e9c55..1dd2f3c30a6 100644
--- a/chromium/components/permissions/permission_request_manager.cc
+++ b/chromium/components/permissions/permission_request_manager.cc
@@ -241,18 +241,25 @@ void PermissionRequestManager::AddRequest(
   }
 
   // Don't re-add an existing request or one with a duplicate text request.
-  PermissionRequest* existing_request = GetExistingRequest(request);
-  if (existing_request) {
+  if (auto* existing_request = GetExistingRequest(request)) {
+    if (request == existing_request) {
+      return;
+    }
+
     // |request| is a duplicate. Add it to |duplicate_requests_| unless it's the
     // same object as |existing_request| or an existing duplicate.
-    if (request == existing_request)
+    auto iter = FindDuplicateRequestList(existing_request);
+    if (iter == duplicate_requests_.end()) {
+      duplicate_requests_.push_back({request->GetWeakPtr()});
       return;
-    auto range = duplicate_requests_.equal_range(existing_request);
-    for (auto it = range.first; it != range.second; ++it) {
-      if (request == it->second)
+    }
+
+    for (const auto& weak_request : (*iter)) {
+      if (weak_request && request == weak_request.get()) {
         return;
+      }
     }
-    duplicate_requests_.insert(std::make_pair(existing_request, request));
+    iter->push_back(request->GetWeakPtr());
     return;
   }
 
@@ -969,12 +976,60 @@ void PermissionRequestManager::CleanUpRequests() {
 PermissionRequest* PermissionRequestManager::GetExistingRequest(
     PermissionRequest* request) const {
   for (PermissionRequest* existing_request : requests_) {
-    if (request->IsDuplicateOf(existing_request))
+    if (request->IsDuplicateOf(existing_request)) {
       return existing_request;
+    }
   }
   return pending_permission_requests_.FindDuplicate(request);
 }
 
+PermissionRequestManager::WeakPermissionRequestList::iterator
+PermissionRequestManager::FindDuplicateRequestList(PermissionRequest* request) {
+  for (auto request_list = duplicate_requests_.begin();
+       request_list != duplicate_requests_.end(); ++request_list) {
+    for (auto iter = request_list->begin(); iter != request_list->end();) {
+      // Remove any requests that have been destroyed.
+      const auto& weak_request = (*iter);
+      if (!weak_request) {
+        iter = request_list->erase(iter);
+        continue;
+      }
+
+      // The first valid request in the list will indicate whether all other
+      // members are duplicate or not.
+      if (weak_request->IsDuplicateOf(request)) {
+        return request_list;
+      }
+
+      break;
+    }
+  }
+
+  return duplicate_requests_.end();
+}
+
+PermissionRequestManager::WeakPermissionRequestList::iterator
+PermissionRequestManager::VisitDuplicateRequests(
+    DuplicateRequestVisitor visitor,
+    PermissionRequest* request) {
+  auto request_list = FindDuplicateRequestList(request);
+  if (request_list == duplicate_requests_.end()) {
+    return request_list;
+  }
+
+  for (auto iter = request_list->begin(); iter != request_list->end();) {
+    if (auto& weak_request = (*iter)) {
+      visitor.Run(weak_request);
+      ++iter;
+    } else {
+      // Remove any requests that have been destroyed.
+      iter = request_list->erase(iter);
+    }
+  }
+
+  return request_list;
+}
+
 void PermissionRequestManager::PermissionGrantedIncludingDuplicates(
     PermissionRequest* request,
     bool is_one_time) {
@@ -982,9 +1037,14 @@ void PermissionRequestManager::PermissionGrantedIncludingDuplicates(
                      pending_permission_requests_.Count(request))
       << "Only requests in [pending_permission_]requests_ can have duplicates";
   request->PermissionGranted(is_one_time);
-  auto range = duplicate_requests_.equal_range(request);
-  for (auto it = range.first; it != range.second; ++it)
-    it->second->PermissionGranted(is_one_time);
+  VisitDuplicateRequests(
+      base::BindRepeating(
+          [](bool is_one_time,
+             const base::WeakPtr<PermissionRequest>& weak_request) {
+            weak_request->PermissionGranted(is_one_time);
+          },
+          is_one_time),
+      request);
 }
 
 void PermissionRequestManager::PermissionDeniedIncludingDuplicates(
@@ -993,9 +1053,12 @@ void PermissionRequestManager::PermissionDeniedIncludingDuplicates(
                      pending_permission_requests_.Count(request))
       << "Only requests in [pending_permission_]requests_ can have duplicates";
   request->PermissionDenied();
-  auto range = duplicate_requests_.equal_range(request);
-  for (auto it = range.first; it != range.second; ++it)
-    it->second->PermissionDenied();
+  VisitDuplicateRequests(
+      base::BindRepeating(
+          [](const base::WeakPtr<PermissionRequest>& weak_request) {
+            weak_request->PermissionDenied();
+          }),
+      request);
 }
 
 void PermissionRequestManager::CancelledIncludingDuplicates(
@@ -1004,6 +1067,13 @@ void PermissionRequestManager::CancelledIncludingDuplicates(
                      pending_permission_requests_.Count(request))
       << "Only requests in [pending_permission_]requests_ can have duplicates";
   request->Cancelled();
+  VisitDuplicateRequests(
+      base::BindRepeating(
+          [](const base::WeakPtr<PermissionRequest>& weak_request) {
+            weak_request->Cancelled();
+          }),
+      request);
+
   auto range = duplicate_requests_.equal_range(request);
   for (auto it = range.first; it != range.second; ++it)
     it->second->Cancelled();
@@ -1014,13 +1084,21 @@ void PermissionRequestManager::RequestFinishedIncludingDuplicates(
   DCHECK_EQ(1ul, base::STLCount(requests_, request) +
                      pending_permission_requests_.Count(request))
       << "Only requests in [pending_permission_]requests_ can have duplicates";
+  auto duplicate_list = VisitDuplicateRequests(
+      base::BindRepeating(
+          [](const base::WeakPtr<PermissionRequest>& weak_request) {
+            weak_request->RequestFinished();
+          }),
+      request); 
+
+  // Note: beyond this point, |request| has probably been deleted, any
+  // dereference of |request| must be done prior this point.
   request->RequestFinished();
-  // Beyond this point, |request| has probably been deleted.
-  auto range = duplicate_requests_.equal_range(request);
-  for (auto it = range.first; it != range.second; ++it)
-    it->second->RequestFinished();
+
   // Additionally, we can now remove the duplicates.
-  duplicate_requests_.erase(request);
+  if (duplicate_list != duplicate_requests_.end()) {
+    duplicate_requests_.erase(duplicate_list);
+  }
 }
 
 void PermissionRequestManager::AddObserver(Observer* observer) {
diff --git a/chromium/components/permissions/permission_request_manager.h b/chromium/components/permissions/permission_request_manager.h
index 7f9081495bb..009bd656cdc 100644
--- a/chromium/components/permissions/permission_request_manager.h
+++ b/chromium/components/permissions/permission_request_manager.h
@@ -227,6 +227,9 @@ class PermissionRequestManager
  private:
   friend class test::PermissionRequestManagerTestApi;
   friend class content::WebContentsUserData<PermissionRequestManager>;
+  FRIEND_TEST_ALL_PREFIXES(PermissionRequestManagerTest, WeakDuplicateRequests);
+  FRIEND_TEST_ALL_PREFIXES(PermissionRequestManagerTest,
+                           WeakDuplicateRequestsAccept);
 
   explicit PermissionRequestManager(content::WebContents* web_contents);
 
@@ -301,6 +304,25 @@ class PermissionRequestManager
   // may or may not be the same object as |request|.
   PermissionRequest* GetExistingRequest(PermissionRequest* request) const;
 
+  // Returns an iterator into |duplicate_requests_|, points the matching list,
+  // or duplicate_requests_.end() if no match. The matching list contains all
+  // the weak requests which are duplicate of the given |request| (see
+  // |IsDuplicateOf|)
+  using WeakPermissionRequestList =
+      std::list<std::list<base::WeakPtr<PermissionRequest>>>;
+  WeakPermissionRequestList::iterator FindDuplicateRequestList(
+      PermissionRequest* request);
+
+  // Trigger |visitor| for each live weak request which matches the given
+  // request (see |IsDuplicateOf|) in the |duplicate_requests_|. Returns an
+  // iterator into |duplicate_requests_|, points the matching list, or
+  // duplicate_requests_.end() if no match.
+  using DuplicateRequestVisitor =
+      base::RepeatingCallback<void(const base::WeakPtr<PermissionRequest>&)>;
+  WeakPermissionRequestList::iterator VisitDuplicateRequests(
+      DuplicateRequestVisitor visitor,
+      PermissionRequest* request);
+
   // Calls PermissionGranted on a request and all its duplicates.
   void PermissionGrantedIncludingDuplicates(PermissionRequest* request,
                                             bool is_one_time);
@@ -362,10 +384,8 @@ class PermissionRequestManager
 
   PermissionRequestQueue pending_permission_requests_;
 
-  // Maps from the first request of a kind to subsequent requests that were
-  // duped against it.
-  std::unordered_multimap<PermissionRequest*, PermissionRequest*>
-      duplicate_requests_;
+  // Stores the weak pointers of duplicated requests in a 2D list.
+  WeakPermissionRequestList duplicate_requests_;
 
   // Maps each PermissionRequest currently in |requests_| or
   // |pending_permission_requests_| to which RenderFrameHost it originated from.