Remove re-entrant call to lazy initialize adblock service (#36368)
1. ad_block_service() lazy initializes 2. Inside AdBlockService ctor -> AdBlockSubscriptionServiceManager ctor -> AdBlockSubscriptionDownloadManagerGetter -> MaybeGetDownloadManager-> builds the background download service -> DeferredClientWrapper::InflateClient → CreateAdBlockSubscriptionDownloadClient override 3. That override calls g_brave_browser_process->ad_block_service() again. Since ad_block_service_ is still null (assignment happens after make_unique returns), it constructs a second AdBlockService, assigns it to ad_block_service_, and the client captures a raw_ptr to that inner instance's subscription_service_manager(). 4. Outer make_unique finishes; assignment ad_block_service_ = ... overwrites the unique_ptr -> destroys inner instance. 5. Later, DeferredClientWrapper::DoRunDeferredClosures runs AdBlockSubscriptionDownloadClient::OnServiceInitialized -> dereferences dangling subscription_service_manager_ -> UAF.
This commit is contained in:
@@ -5,6 +5,7 @@
|
||||
|
||||
#include "chrome/browser/download/background_download_service_factory.h"
|
||||
|
||||
#include "base/functional/bind.h"
|
||||
#include "brave/browser/brave_browser_process.h"
|
||||
#include "brave/components/brave_shields/content/browser/ad_block_service.h"
|
||||
#include "brave/components/brave_shields/content/browser/ad_block_subscription_download_client.h"
|
||||
@@ -16,11 +17,16 @@
|
||||
|
||||
namespace {
|
||||
|
||||
brave_shields::AdBlockSubscriptionServiceManager*
|
||||
GetAdBlockSubscriptionServiceManager() {
|
||||
return g_brave_browser_process->ad_block_service()
|
||||
->subscription_service_manager();
|
||||
}
|
||||
|
||||
std::unique_ptr<download::Client> CreateAdBlockSubscriptionDownloadClient(
|
||||
Profile* profile) {
|
||||
return std::make_unique<brave_shields::AdBlockSubscriptionDownloadClient>(
|
||||
g_brave_browser_process->ad_block_service()
|
||||
->subscription_service_manager());
|
||||
base::BindRepeating(&GetAdBlockSubscriptionServiceManager));
|
||||
}
|
||||
|
||||
} // namespace
|
||||
|
||||
@@ -22,15 +22,17 @@
|
||||
namespace brave_shields {
|
||||
|
||||
AdBlockSubscriptionDownloadClient::AdBlockSubscriptionDownloadClient(
|
||||
AdBlockSubscriptionServiceManager* subscription_manager)
|
||||
: subscription_manager_(subscription_manager) {}
|
||||
SubscriptionServiceManagerGetter subscription_manager_getter)
|
||||
: subscription_manager_getter_(std::move(subscription_manager_getter)) {}
|
||||
|
||||
AdBlockSubscriptionDownloadClient::~AdBlockSubscriptionDownloadClient() =
|
||||
default;
|
||||
|
||||
AdBlockSubscriptionDownloadManager*
|
||||
AdBlockSubscriptionDownloadClient::GetAdBlockSubscriptionDownloadManager() {
|
||||
return subscription_manager_->download_manager();
|
||||
auto* subscription_manager = subscription_manager_getter_.Run();
|
||||
return subscription_manager ? subscription_manager->download_manager()
|
||||
: nullptr;
|
||||
}
|
||||
|
||||
void AdBlockSubscriptionDownloadClient::OnServiceInitialized(
|
||||
|
||||
@@ -9,7 +9,7 @@
|
||||
#include <string>
|
||||
#include <vector>
|
||||
|
||||
#include "base/memory/raw_ptr.h"
|
||||
#include "base/functional/callback.h"
|
||||
#include "components/download/public/background_service/client.h"
|
||||
#include "components/download/public/background_service/download_metadata.h"
|
||||
|
||||
@@ -21,8 +21,11 @@ class AdBlockSubscriptionDownloadManager;
|
||||
|
||||
class AdBlockSubscriptionDownloadClient : public download::Client {
|
||||
public:
|
||||
using SubscriptionServiceManagerGetter =
|
||||
base::RepeatingCallback<AdBlockSubscriptionServiceManager*()>;
|
||||
|
||||
explicit AdBlockSubscriptionDownloadClient(
|
||||
AdBlockSubscriptionServiceManager* subscription_manager);
|
||||
SubscriptionServiceManagerGetter subscription_manager_getter);
|
||||
~AdBlockSubscriptionDownloadClient() override;
|
||||
AdBlockSubscriptionDownloadClient(const AdBlockSubscriptionDownloadClient&) =
|
||||
delete;
|
||||
@@ -49,8 +52,7 @@ class AdBlockSubscriptionDownloadClient : public download::Client {
|
||||
// Returns the AdBlockSubscriptionDownloadManager for the profile.
|
||||
AdBlockSubscriptionDownloadManager* GetAdBlockSubscriptionDownloadManager();
|
||||
|
||||
raw_ptr<AdBlockSubscriptionServiceManager> subscription_manager_ =
|
||||
nullptr; // NOT OWNED
|
||||
SubscriptionServiceManagerGetter subscription_manager_getter_;
|
||||
};
|
||||
|
||||
} // namespace brave_shields
|
||||
|
||||
Reference in New Issue
Block a user