Chrome · Extensions › Extensions
CVE-2026-87637
UAF in Extensions (Extensions)
Medium
watch
CVSS 9.6
chrome
UAF
📄 Reporter bug report
🔧 Commit mapped
🧬 A nested callout frees the object, then it is used
Overview
Medium
Severity
9.6
CVSS
No
Exploited ITW
Fixed
Fix Status
Background
- `ShowSettingsOverriddenDialog()`
- free function in
chrome/browser/ui/extensions/settings_overridden_dialog.ccthat builds and displays the modal warning shown when an extension overrides browser settings (search engine, homepage, or new-tab page). - `SettingsOverriddenDialogController`
- an interface (
settings_overridden_dialog_controller.h) whose implementation owns the dialog’s data and receives lifecycle callbacks (ShouldShow(),GetShowParams(),OnDialogShown()/OnDialogWillBeShown(),HandleDialogResult()). - `SettingsOverriddenDialogDelegate`
- the views dialog delegate that takes ownership of the
SettingsOverriddenDialogControllerviastd::move; the delegate is in turn owned by the dialogviews::Widget. - Nested run loop
- a message loop spun re-entrantly while
ShowModalDialog()runs, during which the widget (and everything it owns) can be synchronously destroyed before the call returns.
Root Cause Analysis
In the pre-patch ShowSettingsOverriddenDialog(), ownership of the controller was moved into a SettingsOverriddenDialogDelegate, which was moved into the dialog views::Widget, and only after ShowModalDialog(parent, dialog_builder.Build()) returned did the code dereference the raw dialog_delegate pointer to call dialog_delegate->controller()->OnDialogShown().
The invariant that was violated is that dialog_delegate (and the controller it owned) must still be alive when dereferenced: on some platforms ShowModalDialog() spins a nested run loop during which the widget — and thus the delegate and controller it transitively owns — can be destroyed, leaving dialog_delegate dangling and making the subsequent ->controller()->OnDialogShown() a use-after-free. The removed comment-free trailing line and the commit message confirm the old, wrong ordering: notify the controller after the dialog is shown.
The fix moves the notification to controller->OnDialogWillBeShown() before ownership is transferred to the delegate (renaming OnDialogShown() accordingly and deleting the now-unused controller() accessor), so the controller is only ever touched while the caller still holds it. This eliminates any post-ShowModalDialog() access to the freed objects while preserving the guarantee that the notification runs before HandleDialogResult().
Key insight
The single mistake was ordering a callback on an object after handing that object’s ownership to a modal dialog that can synchronously destroy it; the fix reorders the callback to fire while the caller still owns the controller, removing the dangling dereference entirely.
Attack Path
- Trigger the overridden-settings dialog
Cause
ShowSettingsOverriddenDialog()to run, e.g. via an installed extension that overrides the default search engine, homepage, or new-tab page, which schedules the modal warning. - Transfer controller ownership into the widget
The controller is
std::moved into aSettingsOverriddenDialogDelegate, which is moved into the dialogviews::Widget; the rawdialog_delegatepointer is retained for use after showing. - Destroy the widget during show
ShowModalDialog()spins a nested run loop in which the dialog widget is destroyed (the regression test closes the widget from insideWidget::Show()), freeing the delegate and the controller it owns. - Dangling dereference
Control returns to the line
dialog_delegate->controller()->OnDialogShown(), which reads through the freeddialog_delegateand freed controller — the use-after-free. - Memory-corruption escalation (inferred) — Reclaiming the freed allocation before the dereference could turn the read/call into controlled behavior; the patch does not demonstrate a concrete read/write primitive or code execution.
Impact Assessment
An attacker gains a use-after-free of the controller/delegate objects in the browser process (Extensions UI code), which is trusted and unsandboxed. The precondition is that the settings-overridden dialog is shown and its widget is destroyed via a nested run loop before
ShowModalDialog() returns — a platform-dependent condition the commit describes for “some platforms.” Any escalation to controlled memory corruption or code execution is (inferred); the advisory classifies this as a use-after-free in Extensions and I have no report of in-the-wild exploitation.Changed Functions
| Function | Change | Notes |
|---|---|---|
TEST_Fchrome/browser/ui/extensions/settings_overridden_dialog_unittest.cc |
modified | |
BindLambdaForTestingchrome/browser/ui/extensions/settings_overridden_dialog_unittest.cc |
modified |
Files Changed
chrome/browser/ui/extensions/settings_overridden_dialog_unittest.ccchrome/browser/ui/extensions/settings_overridden_dialog_controller.hchrome/browser/ui/search_engines/default_search_extension_controlled_controller_unittest.ccchrome/browser/ui/extensions/extension_settings_overridden_dialog.hchrome/browser/ui/extensions/extension_settings_overridden_dialog.ccchrome/browser/ui/extensions/extension_settings_overridden_dialog_unittest.ccchrome/browser/ui/extensions/settings_overridden_dialog.ccchrome/browser/ui/extensions/settings_overridden_dialog_browsertest.cc
Audit Directions
- Post-show dereference of moved-ownership objectsAudit any code that keeps a raw pointer to an object whose ownership was moved into a widget/dialog and then dereferences it after a
Show/RunModal/ShowModalDialogcall that can spin a nested run loop. - Callbacks ordered after modal displayLook for lifecycle notifications (
OnDialogShown,On*Shown, completion callbacks) invoked after showing a modal, and verify the receiver cannot be destroyed by re-entrant loops before the call. - Nested run loop re-entrancy in UI controllersReview views/dialog controllers on Windows and macOS (
BUILDFLAG(IS_WIN) || BUILDFLAG(IS_MAC)paths) where widget destruction during show is possible, treating any access after the show call as suspect.
Patch
From 2e2a4073d4d202f064bc10f26b61ae5d563c56c2 Mon Sep 17 00:00:00 2001 From: Tim <[email protected]> Date: Tue, 28 Jul 2026 13:36:32 -0700 Subject: [PATCH] [Extensions] Notify settings-overridden controller before showing dialog ShowSettingsOverriddenDialog() dereferenced the dialog delegate after ShowModalDialog() returned to call OnDialogShown() on the controller. On some platforms showing a modal dialog can spin a nested run loop during which the dialog widget, and the delegate and controller it owns, may be destroyed, so the delegate must not be accessed once the dialog has been shown. Call OnDialogShown() before the controller's ownership is passed to the dialog. This also guarantees OnDialogShown() runs before HandleDialogResult(). Remove the now-unused controller() accessor and add a unit test that closes the dialog widget from inside Widget::Show(). Since this also means this is now technically called before the dialog is shown and not after, we have renamed OnDialogShown() to OnDialogWillBeShown(). Bug: 534863145 Change-Id: Ibc3ab29cb210271fb95f2ed5afc173c39efab1a0 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8129643 Auto-Submit: Tim <[email protected]> Commit-Queue: Tim <[email protected]> Reviewed-by: Eva Su <[email protected]> Commit-Queue: Mark Pearson <[email protected]> Reviewed-by: Mark Pearson <[email protected]> Cr-Commit-Position: refs/heads/main@{#1669722} --- diff --git a/chrome/browser/ui/extensions/extension_settings_overridden_dialog.cc b/chrome/browser/ui/extensions/extension_settings_overridden_dialog.cc index 55043ed4..05c9a14 100644 --- a/chrome/browser/ui/extensions/extension_settings_overridden_dialog.cc +++ b/chrome/browser/ui/extensions/extension_settings_overridden_dialog.cc @@ -208,7 +208,7 @@ return params_.content; } -void ExtensionSettingsOverriddenDialog::OnDialogShown() { +void ExtensionSettingsOverriddenDialog::OnDialogWillBeShown() { DCHECK(ShouldShow()); show_time_ = base::TimeTicks::Now(); diff --git a/chrome/browser/ui/extensions/extension_settings_overridden_dialog.h b/chrome/browser/ui/extensions/extension_settings_overridden_dialog.h index 5fcf9740..64f4f5e 100644 --- a/chrome/browser/ui/extensions/extension_settings_overridden_dialog.h +++ b/chrome/browser/ui/extensions/extension_settings_overridden_dialog.h @@ -103,7 +103,7 @@ // SettingsOverriddenDialogController: bool ShouldShow() override; ShowParams GetShowParams() override; - void OnDialogShown() override; + void OnDialogWillBeShown() override; void HandleDialogResult(DialogResult result) override; // Sets a callback to be invoked when the dialog result is handled. diff --git a/chrome/browser/ui/extensions/settings_overridden_dialog.cc b/chrome/browser/ui/extensions/settings_overridden_dialog.cc index 4c054ae56..1dc25ee 100644 --- a/chrome/browser/ui/extensions/settings_overridden_dialog.cc +++ b/chrome/browser/ui/extensions/settings_overridden_dialog.cc @@ -82,8 +82,6 @@ HandleDialogResult(selected_setting_.value()); } - SettingsOverriddenDialogController* controller() { return controller_.get(); } - private: void HandleDialogResult(DialogResult result) { DCHECK(!result_) @@ -203,6 +201,10 @@ SettingsOverriddenDialogController::ShowParams show_params = controller->GetShowParams(); + // Notify the controller before its ownership is passed to the dialog, since + // showing the dialog may run a nested loop that destroys it. + controller->OnDialogWillBeShown(); + auto dialog_delegate_unique = std::make_unique<SettingsOverriddenDialogDelegate>(std::move(controller)); SettingsOverriddenDialogDelegate* dialog_delegate = @@ -227,8 +229,6 @@ #endif // BUILDFLAG(IS_WIN) || BUILDFLAG(IS_MAC) ShowModalDialog(parent, dialog_builder.Build()); - - dialog_delegate->controller()->OnDialogShown(); } } // namespace extensions diff --git a/chrome/browser/ui/extensions/settings_overridden_dialog_controller.h b/chrome/browser/ui/extensions/settings_overridden_dialog_controller.h index 002d8deb..d865afb 100644 --- a/chrome/browser/ui/extensions/settings_overridden_dialog_controller.h +++ b/chrome/browser/ui/extensions/settings_overridden_dialog_controller.h @@ -93,8 +93,8 @@ // synchronously. virtual ShowParams GetShowParams() = 0; - // Notifies the controller that the dialog has been shown. - virtual void OnDialogShown() = 0; + // Notifies the controller that the dialog will be shown. + virtual void OnDialogWillBeShown() = 0; // Handles the result of the dialog being shown. virtual void HandleDialogResult(DialogResult result) = 0; diff --git a/chrome/browser/ui/extensions/extension_settings_overridden_dialog_unittest.cc b/chrome/browser/ui/extensions/extension_settings_overridden_dialog_unittest.cc index c832a2ae..7f64073 100644 --- a/chrome/browser/ui/extensions/extension_settings_overridden_dialog_unittest.cc +++ b/chrome/browser/ui/extensions/extension_settings_overridden_dialog_unittest.cc @@ -130,7 +130,7 @@ ExtensionSettingsOverriddenDialog controller( CreateTestDialogParams(extension->id()), *profile()); EXPECT_TRUE(controller.ShouldShow()); - controller.OnDialogShown(); + controller.OnDialogWillBeShown(); controller.HandleDialogResult(DialogResult::kChangeSettingsBack); histogram_tester.ExpectUniqueSample(kTestDialogResultHistogramName, @@ -151,7 +151,7 @@ ExtensionSettingsOverriddenDialog controller( CreateTestDialogParams(extension->id()), *profile()); EXPECT_TRUE(controller.ShouldShow()); - controller.OnDialogShown(); + controller.OnDialogWillBeShown(); controller.HandleDialogResult(DialogResult::kKeepNewSettings); histogram_tester.ExpectUniqueSample(kTestDialogResultHistogramName, @@ -168,7 +168,7 @@ ExtensionSettingsOverriddenDialog controller( CreateTestDialogParams(extension->id()), *profile()); - controller.OnDialogShown(); + controller.OnDialogWillBeShown(); controller.HandleDialogResult(DialogResult::kDialogDismissed); histogram_tester.ExpectUniqueSample(kTestDialogResultHistogramName, @@ -186,7 +186,7 @@ ExtensionSettingsOverriddenDialog controller( CreateTestDialogParams(extension->id()), *profile()); - controller.OnDialogShown(); + controller.OnDialogWillBeShown(); controller.HandleDialogResult(DialogResult::kDialogClosedWithoutUserAction); histogram_tester.ExpectUniqueSample( @@ -205,7 +205,7 @@ ExtensionSettingsOverriddenDialog controller( CreateTestDialogParams(extension->id()), *profile()); EXPECT_TRUE(controller.ShouldShow()); - controller.OnDialogShown(); + controller.OnDialogWillBeShown(); controller.HandleDialogResult(DialogResult::kDialogDismissed); } @@ -226,7 +226,7 @@ ExtensionSettingsOverriddenDialog controller( CreateTestDialogParams(extension_one->id()), *profile()); EXPECT_TRUE(controller.ShouldShow()); - controller.OnDialogShown(); + controller.OnDialogWillBeShown(); controller.HandleDialogResult(DialogResult::kDialogDismissed); } @@ -245,7 +245,7 @@ ExtensionSettingsOverriddenDialog controller( CreateTestDialogParams(extension->id()), *profile()); EXPECT_TRUE(controller.ShouldShow()); - controller.OnDialogShown(); + controller.OnDialogWillBeShown(); registrar()->UninstallExtension( extension->id(), extensions::UNINSTALL_REASON_FOR_TESTING, nullptr); diff --git a/chrome/browser/ui/extensions/settings_overridden_dialog_browsertest.cc b/chrome/browser/ui/extensions/settings_overridden_dialog_browsertest.cc index 62843b66..b562fd6b 100644 --- a/chrome/browser/ui/extensions/settings_overridden_dialog_browsertest.cc +++ b/chrome/browser/ui/extensions/settings_overridden_dialog_browsertest.cc @@ -63,7 +63,7 @@ private: bool ShouldShow() override { return true; } ShowParams GetShowParams() override { return show_params_; } - void OnDialogShown() override {} + void OnDialogWillBeShown() override {} void HandleDialogResult(DialogResult result) override { ASSERT_FALSE(dialog_result_out_->has_value()); *dialog_result_out_ = result; diff --git a/chrome/browser/ui/extensions/settings_overridden_dialog_unittest.cc b/chrome/browser/ui/extensions/settings_overridden_dialog_unittest.cc index d9fa6bf..d0f22669 100644 --- a/chrome/browser/ui/extensions/settings_overridden_dialog_unittest.cc +++ b/chrome/browser/ui/extensions/settings_overridden_dialog_unittest.cc @@ -7,6 +7,7 @@ #include <optional> #include "base/memory/raw_ptr.h" +#include "base/test/bind.h" #include "chrome/browser/ui/extensions/extensions_dialogs.h" #include "chrome/browser/ui/extensions/settings_overridden_dialog_controller.h" #include "chrome/browser/ui/views/chrome_constrained_window_views_client.h" @@ -40,8 +41,9 @@ private: bool ShouldShow() override { return true; } ShowParams GetShowParams() override { return show_params_; } - void OnDialogShown() override { - EXPECT_FALSE(state_->shown) << "OnDialogShown() called more than once!"; + void OnDialogWillBeShown() override { + EXPECT_FALSE(state_->shown) + << "OnDialogWillBeShown() called more than once!"; state_->shown = true; } void HandleDialogResult(DialogResult result) override { @@ -96,6 +98,10 @@ return dialog; } + gfx::NativeWindow parent_window() { + return parent_widget_->GetNativeWindow(); + } + private: std::unique_ptr<views::Widget> parent_widget_; }; @@ -147,3 +153,29 @@ ASSERT_TRUE(state.result); EXPECT_EQ(DialogResult::kDialogClosedWithoutUserAction, state.result); } + +// Showing a modal dialog can, on some platforms, result in the dialog widget +// being synchronously destroyed before Widget::Show() returns (for example if +// a nested run loop closes the parent window). Verify that the controller is +// notified that the dialog was shown, and that the result is delivered, even +// when the dialog is destroyed as soon as it is shown. +TEST_F(SettingsOverriddenDialogUnitTest, DialogDestroyedWhileBeingShown) { + DialogState state; + + views::AnyWidgetObserver observer(views::test::AnyWidgetTestPasskey{}); + observer.set_shown_callback( + base::BindLambdaForTesting([&](views::Widget* widget) { + if (widget->GetName() != kExtensionSettingsOverriddenDialogName) { + return; + } + EXPECT_TRUE(state.shown); + widget->CloseNow(); + })); + + extensions::ShowSettingsOverriddenDialog( + std::make_unique<TestDialogController>(&state), parent_window()); + + EXPECT_TRUE(state.shown); + ASSERT_TRUE(state.result); + EXPECT_EQ(DialogResult::kDialogClosedWithoutUserAction, state.result); +} diff --git a/chrome/browser/ui/search_engines/default_search_extension_controlled_controller_unittest.cc b/chrome/browser/ui/search_engines/default_search_extension_controlled_controller_unittest.cc index d4fd69f..315ea9b 100644 --- a/chrome/browser/ui/search_engines/default_search_extension_controlled_controller_unittest.cc +++ b/chrome/browser/ui/search_engines/default_search_extension_controlled_controller_unittest.cc @@ -348,7 +348,7 @@ ExtensionSettingsOverriddenDialog::ShowParams(u"Title", u"Body", nullptr)); ExtensionSettingsOverriddenDialog dialog(std::move(params), profile_); - dialog.OnDialogShown(); + dialog.OnDialogWillBeShown(); DefaultSearchExtensionControlledController owned_controller(browser_window_, profile_);
Loading diff…
Regression Test / PoC
shipped with the fix
diff --git a/chrome/browser/ui/extensions/extension_settings_overridden_dialog_unittest.cc b/chrome/browser/ui/extensions/extension_settings_overridden_dialog_unittest.cc
index c832a2ae..7f64073 100644
--- a/chrome/browser/ui/extensions/extension_settings_overridden_dialog_unittest.cc
+++ b/chrome/browser/ui/extensions/extension_settings_overridden_dialog_unittest.cc
@@ -130,7 +130,7 @@
ExtensionSettingsOverriddenDialog controller(
CreateTestDialogParams(extension->id()), *profile());
EXPECT_TRUE(controller.ShouldShow());
- controller.OnDialogShown();
+ controller.OnDialogWillBeShown();
controller.HandleDialogResult(DialogResult::kChangeSettingsBack);
histogram_tester.ExpectUniqueSample(kTestDialogResultHistogramName,
@@ -151,7 +151,7 @@
ExtensionSettingsOverriddenDialog controller(
CreateTestDialogParams(extension->id()), *profile());
EXPECT_TRUE(controller.ShouldShow());
- controller.OnDialogShown();
+ controller.OnDialogWillBeShown();
controller.HandleDialogResult(DialogResult::kKeepNewSettings);
histogram_tester.ExpectUniqueSample(kTestDialogResultHistogramName,
@@ -168,7 +168,7 @@
ExtensionSettingsOverriddenDialog controller(
CreateTestDialogParams(extension->id()), *profile());
- controller.OnDialogShown();
+ controller.OnDialogWillBeShown();
controller.HandleDialogResult(DialogResult::kDialogDismissed);
histogram_tester.ExpectUniqueSample(kTestDialogResultHistogramName,
@@ -186,7 +186,7 @@
ExtensionSettingsOverriddenDialog controller(
CreateTestDialogParams(extension->id()), *profile());
- controller.OnDialogShown();
+ controller.OnDialogWillBeShown();
controller.HandleDialogResult(DialogResult::kDialogClosedWithoutUserAction);
histogram_tester.ExpectUniqueSample(
@@ -205,7 +205,7 @@
ExtensionSettingsOverriddenDialog controller(
CreateTestDialogParams(extension->id()), *profile());
EXPECT_TRUE(controller.ShouldShow());
- controller.OnDialogShown();
+ controller.OnDialogWillBeShown();
controller.HandleDialogResult(DialogResult::kDialogDismissed);
}
@@ -226,7 +226,7 @@
ExtensionSettingsOverriddenDialog controller(
CreateTestDialogParams(extension_one->id()), *profile());
EXPECT_TRUE(controller.ShouldShow());
- controller.OnDialogShown();
+ controller.OnDialogWillBeShown();
controller.HandleDialogResult(DialogResult::kDialogDismissed);
}
@@ -245,7 +245,7 @@
ExtensionSettingsOverriddenDialog controller(
CreateTestDialogParams(extension->id()), *profile());
EXPECT_TRUE(controller.ShouldShow());
- controller.OnDialogShown();
+ controller.OnDialogWillBeShown();
registrar()->UninstallExtension(
extension->id(), extensions::UNINSTALL_REASON_FOR_TESTING, nullptr);
diff --git a/chrome/browser/ui/extensions/settings_overridden_dialog_browsertest.cc b/chrome/browser/ui/extensions/settings_overridden_dialog_browsertest.cc
index 62843b66..b562fd6b 100644
--- a/chrome/browser/ui/extensions/settings_overridden_dialog_browsertest.cc
+++ b/chrome/browser/ui/extensions/settings_overridden_dialog_browsertest.cc
@@ -63,7 +63,7 @@
private:
bool ShouldShow() override { return true; }
ShowParams GetShowParams() override { return show_params_; }
- void OnDialogShown() override {}
+ void OnDialogWillBeShown() override {}
void HandleDialogResult(DialogResult result) override {
ASSERT_FALSE(dialog_result_out_->has_value());
*dialog_result_out_ = result;
diff --git a/chrome/browser/ui/extensions/settings_overridden_dialog_unittest.cc b/chrome/browser/ui/extensions/settings_overridden_dialog_unittest.cc
index d9fa6bf..d0f22669 100644
--- a/chrome/browser/ui/extensions/settings_overridden_dialog_unittest.cc
+++ b/chrome/browser/ui/extensions/settings_overridden_dialog_unittest.cc
@@ -7,6 +7,7 @@
#include <optional>
#include "base/memory/raw_ptr.h"
+#include "base/test/bind.h"
#include "chrome/browser/ui/extensions/extensions_dialogs.h"
#include "chrome/browser/ui/extensions/settings_overridden_dialog_controller.h"
#include "chrome/browser/ui/views/chrome_constrained_window_views_client.h"
@@ -40,8 +41,9 @@
private:
bool ShouldShow() override { return true; }
ShowParams GetShowParams() override { return show_params_; }
- void OnDialogShown() override {
- EXPECT_FALSE(state_->shown) << "OnDialogShown() called more than once!";
+ void OnDialogWillBeShown() override {
+ EXPECT_FALSE(state_->shown)
+ << "OnDialogWillBeShown() called more than once!";
state_->shown = true;
}
void HandleDialogResult(DialogResult result) override {
@@ -96,6 +98,10 @@
return dialog;
}
+ gfx::NativeWindow parent_window() {
+ return parent_widget_->GetNativeWindow();
+ }
+
private:
std::unique_ptr<views::Widget> parent_widget_;
};
@@ -147,3 +153,29 @@
ASSERT_TRUE(state.result);
EXPECT_EQ(DialogResult::kDialogClosedWithoutUserAction, state.result);
}
+
+// Showing a modal dialog can, on some platforms, result in the dialog widget
+// being synchronously destroyed before Widget::Show() returns (for example if
+// a nested run loop closes the parent window). Verify that the controller is
+// notified that the dialog was shown, and that the result is delivered, even
+// when the dialog is destroyed as soon as it is shown.
+TEST_F(SettingsOverriddenDialogUnitTest, DialogDestroyedWhileBeingShown) {
+ DialogState state;
+
+ views::AnyWidgetObserver observer(views::test::AnyWidgetTestPasskey{});
+ observer.set_shown_callback(
+ base::BindLambdaForTesting([&](views::Widget* widget) {
+ if (widget->GetName() != kExtensionSettingsOverriddenDialogName) {
+ return;
+ }
+ EXPECT_TRUE(state.shown);
+ widget->CloseNow();
+ }));
+
+ extensions::ShowSettingsOverriddenDialog(
+ std::make_unique<TestDialogController>(&state), parent_window());
+
+ EXPECT_TRUE(state.shown);
+ ASSERT_TRUE(state.result);
+ EXPECT_EQ(DialogResult::kDialogClosedWithoutUserAction, state.result);
+}
diff --git a/chrome/browser/ui/search_engines/default_search_extension_controlled_controller_unittest.cc b/chrome/browser/ui/search_engines/default_search_extension_controlled_controller_unittest.cc
index d4fd69f..315ea9b 100644
--- a/chrome/browser/ui/search_engines/default_search_extension_controlled_controller_unittest.cc
+++ b/chrome/browser/ui/search_engines/default_search_extension_controlled_controller_unittest.cc
@@ -348,7 +348,7 @@
ExtensionSettingsOverriddenDialog::ShowParams(u"Title", u"Body",
nullptr));
ExtensionSettingsOverriddenDialog dialog(std::move(params), profile_);
- dialog.OnDialogShown();
+ dialog.OnDialogWillBeShown();
DefaultSearchExtensionControlledController owned_controller(browser_window_,
profile_);
Loading diff…
Original Bug Report
reported by [email protected]
Potential Use-After-Free in ShowSettingsOverriddenDialog due to macOS nested run loop
Project Fortify, an experimental security project, has identified the following potential security issue. If you’re a feature owner CC-ed on this bug, please do your best to review these reports. Please see https://chromium.googlesource.com/chromium/src/+/main/docs/security/ai-generated-security-bugs-faq.md for more information.
Overview: A potential Use-After-Free (UAF) vulnerability exists in the browser process when displaying the settings overridden dialog on macOS. Initiating the modal sheet can synchronously trigger a nested event-tracking run loop during which the parent window and dialog widget can be destroyed. This results in stack-local raw pointers being dereferenced upon return, leading to potential memory corruption in the unsandboxed browser process.
Affected files:
chrome/browser/ui/extensions/settings_overridden_dialog.cc
Estimated timestamp from git blame: 2022-08-09
Root Cause Analysis
In chrome/browser/ui/extensions/settings_overridden_dialog.cc, ShowSettingsOverriddenDialog retains a bare stack pointer to a SettingsOverriddenDialogDelegate whose ownership has been transferred to the ui::DialogModel:
void ShowSettingsOverriddenDialog(
std::unique_ptr<SettingsOverriddenDialogController> controller,
gfx::NativeWindow parent) {
...
auto dialog_delegate_unique =
std::make_unique<SettingsOverriddenDialogDelegate>(std::move(controller));
SettingsOverriddenDialogDelegate* dialog_delegate =
dialog_delegate_unique.get();
ui::DialogModel::Builder dialog_builder =
ui::DialogModel::Builder(std::move(dialog_delegate_unique));
...
ShowModalDialog(parent, dialog_builder.Build());
dialog_delegate->controller()->OnDialogShown();
}
On macOS, ShowModalDialog eventually calls NativeWidgetNSWindowBridge::ShowAsModalSheet in components/remote_cocoa/app_shim/native_widget_ns_window_bridge.mm. This synchronously executes -[NSWindow beginSheet:completionHandler:], which enters a synchronous, nested run loop to animate the sheet.
Because the nested run loop continues processing the browser’s UI thread tasks and IPC messages, a queued close event (e.g., from a webpage closing its parent window or via extension APIs) can run and destroy the widget. Since the BubbleDialogModelHost and its underlying DialogModel are owned by the widget, this destruction chain deletes both the SettingsOverriddenDialogDelegate and its owned SettingsOverriddenDialogController.
Upon returning from the nested run loop, the stack-local dialog_delegate pointer is now dangling. Dereferencing it to call dialog_delegate->controller()->OnDialogShown() results in a Use-After-Free and an immediate virtual function dispatch through a potentially attacker-controlled vtable.
Potential Trigger Steps
Note: These are potential steps based on static code analysis; our automated tooling does not currently have the capability to execute proof-of-concept code.
- Install an extension that overrides the New Tab Page (NTP) on macOS.
- Programmatically open the NTP using
chrome.tabs.createor another page-opening mechanism to triggerShowSettingsOverriddenDialog. - While the sheet slide-down animation is executing (spinning the AppKit nested run loop), immediately close the window or tab (e.g., using
chrome.tabs.removefrom the extension’s background script). - The window close IPC is processed during the nested run loop, destroying the widget and the dialog model host.
- Once the animation finishes and the nested loop terminates, control returns to
ShowSettingsOverriddenDialog, which attempts to invokeOnDialogShown()on the freed delegate/controller.
Suggested Fix
To prevent holding a dangling pointer across the potentially blocking ShowModalDialog call, retrieve a weak pointer to the controller prior to showing the dialog, and check its validity before invoking the notification:
// Inside ShowSettingsOverriddenDialog
base::WeakPtr<SettingsOverriddenDialogController> weak_controller = controller->AsWeakPtr();
ShowModalDialog(parent, dialog_builder.Build());
if (weak_controller) {
weak_controller->OnDialogShown();
}
Alternatively, if the design allows, notify the controller that the dialog was shown prior to calling ShowModalDialog.
Evaluated with Chrome root at commit: b5b015ea5f690560237d1f0cff1405844cd12b8d
Results so far have been promising, but there can be wrong deductions. Feel free to adjust as follows:
- If you are familiar with the severity guidelines, you may adjust the severity.
- If this is a false positive, and there’s no work to be done, please close as WAI.
- If there is work to do here but not a vulnerability, please change the issue type to Task/Bug/FR.
Data from false positives will be used to improve accuracy over time. And please feel free to reach out to me directly if you have concerns or feedback on the project.
References
On This Page