From 84d4cae4c55f2a0a011160acaded3f57ce4bca57 Mon Sep 17 00:00:00 2001 From: Thomas Nguyen Date: Thu, 30 Mar 2023 12:23:35 +0000 Subject: [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 Reviewed-by: Andy Paicu Cr-Commit-Position: refs/heads/main@{#1124133} Reviewed-on: https://codereview.qt-project.org/c/qt/qtwebengine-chromium/+/476753 Reviewed-by: Michal Klocek --- .../components/permissions/permission_request.cc | 4 + .../components/permissions/permission_request.h | 6 ++ .../permissions/permission_request_manager.cc | 116 +++++++++++++++++---- .../permissions/permission_request_manager.h | 28 ++++- 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::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 #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 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 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& 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& 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& 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& 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; + 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>>; + 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&)>; + 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 - 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. -- cgit v1.2.1