Chrome · Views
CVE-2026-79140
UAF in Views
Overview
Medium
Severity
9.6
CVSS
No
Exploited ITW
Fixed
Fix Status
Background
- Modal sheet
- a macOS/AppKit dialog attached to a parent window, presented via
[parent_window beginSheet:completionHandler:], whose presentation animation runs a nested run loop. - Nested run loop
- an inner event loop that continues to pump and dispatch main-thread messages (including window-close events) while the outer call is still on the stack.
- `NativeWidgetNSWindowBridge`
- the C++ bridge object (in
components/remote_cocoa/app_shim/native_widget_ns_window_bridge.mm) that owns and mediates a native Cocoa window; it can be destroyed when its widget is closed. - `factory_.GetWeakPtr()`
- a
base::WeakPtrfactory used to detect whetherthisbridge has been destroyed after re-entrant work.
Root Cause Analysis
When a modal sheet was shown for an in-process (non-remote-cocoa) window, the sheet-presentation closure was invoked synchronously via std::move(begin_sheet_closure).Run(), because host_helper_->MustPostTaskToRunModalSheetAnimation() returned false for NativeWidgetMacNSWindowHost. AppKit’s beginSheet: runs a nested run loop synchronously during the animation, and while that inner loop pumps main-thread events the parent or child widget can be closed, synchronously destroying the NativeWidgetNSWindowBridge (this). When the nested loop exits and control returns into the now-freed bridge’s member function, the code touches freed member state (e.g. wants_to_be_visible_, OnWindowWillClose()), a use-after-free.
The fix always posts the sheet-presentation work to the message loop via PostTask, so the animation runs on a fresh stack, and re-guards every access to the bridge behind weak_this (weak_this.get(), if (weak_this)), returning early if the bridge was destroyed. A previously-necessary weak-pointer check in the fullscreen-transition path (OrderChildren()) is removed because the presentation is now asynchronous and can no longer re-enter and destroy the bridge inline.
Key insight
The single mistake was presenting the modal sheet synchronously for in-process windows, allowing AppKit’s nested run loop to destroy the bridge underneath a still-executing member function; the fix makes presentation unconditionally asynchronous and gates all post-loop bridge accesses on a
base::WeakPtr.Attack Path
- Trigger a modal sheet
Cause a modal dialog to be shown as a macOS sheet on an in-process window (e.g. a bookmark editor, sign-in, or extension dialog), entering
ShowAsModalSheetwhere the presentation closure runs synchronously. - Enter the nested run loop
beginSheet:begins the sheet animation and spins a nested run loop that continues pumping main-thread events on the same stack. - Close the widget re-entrantly
During that inner loop, drive an event that closes the parent or child widget (window close), which synchronously destroys the owning
NativeWidgetNSWindowBridge(this). - Resume into freed memory When the nested loop exits, execution returns into the destroyed bridge’s completion/member code and dereferences freed member state, producing the use-after-free. (inferred)
Impact Assessment
An attacker or unlucky timing that closes a widget while a modal sheet is animating on macOS gains a use-after-free of the
NativeWidgetNSWindowBridge in the browser process, which the advisory classifies as “Use after free in Views.” Preconditions are macOS with an in-process native window presenting a modal sheet; the diff shows the UAF window but does not itself demonstrate a controlled read/write primitive or code execution, so any escalation beyond the crash is (inferred). The advisory does not state exploited-in-the-wild status.Changed Functions
| Function | Change | Notes |
|---|---|---|
ifcomponents/remote_cocoa/app_shim/native_widget_ns_window_bridge.mm |
modified | |
ASSERT_TRUEchrome/browser/ui/signin/signin_view_controller_interactive_uitest.cc |
modified | |
ifchrome/browser/ui/test/test_browser_dialog.cc |
modified |
Files Changed
ui/views/widget/native_widget_mac_unittest.mmchrome/browser/ui/test/test_browser_dialog.ccchrome/browser/ui/views/safe_browsing/tailored_security_desktop_dialog_manager_browsertest.cccomponents/remote_cocoa/app_shim/application_bridge.mmcomponents/remote_cocoa/app_shim/native_widget_ns_window_host_helper.hchrome/browser/ui/signin/signin_view_controller_interactive_uitest.ccchrome/browser/ui/views/policy/enterprise_startup_dialog_view_browsertest.cccomponents/remote_cocoa/app_shim/native_widget_ns_window_bridge.mmchrome/browser/ui/extensions/settings_overridden_dialog_browsertest.ccchrome/browser/ui/views/web_dialog_view_interactive_uitest.ccchrome/browser/ui/cocoa/browser_window_mac_browsertest.mmchrome/browser/ui/views/web_apps/web_app_update_review_dialog_browsertest.ccchrome/browser/ui/views/relaunch_notification/relaunch_notification_controller_platform_impl_desktop_browsertest.ccui/views/cocoa/native_widget_mac_ns_window_host.mmchrome/browser/ui/test/test_browser_dialog.hui/views/widget/widget_interactive_uitest.ccui/views/cocoa/native_widget_mac_ns_window_host.hchrome/browser/ui/views/web_apps/protocol_handler_launch_dialog_browsertest.cc
Audit Directions
- Synchronous nested run loopsAudit any AppKit call that spins a nested run loop (
beginSheet:, modal panels,runModal) reached while a C++ owner is on the stack; verify the owner cannot be destroyed re-entrantly, or guard post-loop access with aWeakPtr. - Re-entrancy after re-entrant callbacksLook for member functions that continue to touch
this/member state after calling out to code that can process events (OrderChildren, close paths); confirm a liveness check precedes each such access. - Platform-conditional async behaviorReview interfaces like
MustPostTaskToRunModalSheetAnimationwhere one platform posts and another runs inline; the inline path often hides re-entrancy UAFs the posted path avoids.
Patch
From c763f3b4837ea55058610ecf628cd140ef40b52e Mon Sep 17 00:00:00 2001
From: Bryan Oltman <bryanoltman@google.com>
Date: Tue, 21 Jul 2026 09:30:20 -0700
Subject: [PATCH] Reland "Reland "[macOS] Make modal sheet presentation asynchronous to prevent UAF bugs""
This reverts commit d4c2f03a85a5eb43c9bb0f43088cfe3acfd0efec.
Reason for revert: msan tests were failing, this failure has been fixed
Original change's description:
> Revert "Reland "[macOS] Make modal sheet presentation asynchronous to prevent UAF bugs""
>
> This reverts commit 3c030585436edb1989cd19af246f8e4db610ebeb.
>
> Reason for revert: Culprit of failure of
> TailoredSecurityDesktopDialogManagerTest.OpeningANewDisableDialogWillCloseAnyOpenEnableDialogs
>
> Failure Link: https://ci.chromium.org/ui/p/chromium/builders/ci/Linux%20MSan%20Tests/64261/overview
>
> Original change's description:
> > Reland "[macOS] Make modal sheet presentation asynchronous to prevent UAF bugs"
> >
> > This reverts commit 879c1fa69bbe9480b7aaaa1b8e4b16881642b266.
> >
> > Reason for revert: Rolling forward with fixes
> >
> > Original change's description:
> > > Revert "[macOS] Make modal sheet presentation asynchronous to prevent UAF bugs"
> > >
> > > This reverts commit 90309b0d2c7d8f975a2e3cc672d3e59b3e19947e.
> > >
> > > Reason for revert:
> > > LUCI Bisection has identified this change as the cause of a test failure. See the analysis: https://ci.chromium.org/ui/p/chromium/bisection/test-analysis/b/5250851556818944
> > >
> > > Sample build with failed test: https://ci.chromium.org/b/8676059972782943729
> > > Affected test(s):
> > > [://chrome/test\:browser_tests!gtest::BookmarkAccountStorageMoveDialogPixelTest#InvokeUi_ShowMoveBookmarkToAccount](https://ci.chromium.org/ui/test/chromium/:%2F%2Fchrome%2Ftest%5C:browser_tests%21gtest::BookmarkAccountStorageMoveDialogPixelTest%23InvokeUi_ShowMoveBookmarkToAccount?q=VHash%3A812306a38c719b85)
> > > [://chrome/test\:browser_tests!gtest::BookmarkAccountStorageMoveDialogPixelTest#InvokeUi_ShowMoveFolderToAccount](https://ci.chromium.org/ui/test/chromium/:%2F%2Fchrome%2Ftest%5C:browser_tests%21gtest::BookmarkAccountStorageMoveDialogPixelTest%23InvokeUi_ShowMoveFolderToAccount?q=VHash%3A812306a38c719b85)
> > > [://chrome/test\:browser_tests!gtest::ExtensionInstallBlockedByParentDialogTest#InvokeUi_app](https://ci.chromium.org/ui/test/chromium/:%2F%2Fchrome%2Ftest%5C:browser_tests%21gtest::ExtensionInstallBlockedByParentDialogTest%23InvokeUi_app?q=VHash%3A812306a38c719b85)
> > > [://chrome/test\:browser_tests!gtest::ExtensionInstallBlockedByParentDialogTest#InvokeUi_extension](https://ci.chromium.org/ui/test/chromium/:%2F%2Fchrome%2Ftest%5C:browser_tests%21gtest::ExtensionInstallBlockedByParentDialogTest%23InvokeUi_extension?q=VHash%3A812306a38c719b85)
> > > [://chrome/test\:browser_tests!gtest::SettingsOverriddenDialogBrowserTest#InvokeUi_SimpleDialog](https://ci.chromium.org/ui/test/chromium/:%2F%2Fchrome%2Ftest%5C:browser_tests%21gtest::SettingsOverriddenDialogBrowserTest%23InvokeUi_SimpleDialog?q=VHash%3A812306a38c719b85)
> > > and 14 more ...
> > >
> > > If this is a false positive, please report it at http://b.corp.google.com/createIssue?component=1199205&description=Analysis%3A+https%3A%2F%2Fci.chromium.org%2Fui%2Fp%2Fchromium%2Fbisection%2Ftest-analysis%2Fb%2F5250851556818944&format=PLAIN&priority=P3&title=Wrongly+blamed+https%3A%2F%2Fchromium-review.googlesource.com%2Fc%2Fchromium%2Fsrc%2F%2B%2F8086464&type=BUG
> > >
> > > Original change's description:
> > > > [macOS] Make modal sheet presentation asynchronous to prevent UAF bugs
> > > >
> > > > Modal sheet animations run a nested run loop synchronously in AppKit.
> > > > During this loop, the main thread remains active, allowing the parent or
> > > > child widget to be closed and its C++ bridge synchronously destroyed.
> > > > This leads to Use-After-Free (UAF) bugs when the nested loop exits and
> > > > execution resumes in the member functions of the deleted bridge.
> > > >
> > > > This CL runs modal sheet presentations asynchronously to prevent UAF:
> > > > - Always post the sheet animation task to the message loop (previously
> > > > only done for remote cocoa).
> > > > - Add the SetVisibilityStateBridgeDestruction regression test.
> > > >
> > > > This CL also removes a weak pointer check in
> > > > FullscreenControllerTransitionComplete that is no longer necessary now
> > > > that the modal sheet presentation is asynchronous.
> > > >
> > > > Include-Ci-Only-Tests: true
> > > > Fixed: 518006007,502109333,502101200
> > > > Change-Id: I9a911b80d087f79920cbd944af854dbf82c6f6ae
> > > > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8086464
> > > > Commit-Queue: Bryan Oltman <bryanoltman@google.com>
> > > > Reviewed-by: Keren Zhu <kerenzhu@chromium.org>
> > > > Cr-Commit-Position: refs/heads/main@{#1663648}
> > > >
> > >
> > > No-Presubmit: true
> > > No-Tree-Checks: true
> > > No-Try: true
> > > Change-Id: I4604e56d435714b79d85c20029981319dd61c0e5
> > > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8114268
> > > Owners-Override: Kalvin Lee <kdlee@chromium.org>
> > > Commit-Queue: Kalvin Lee <kdlee@chromium.org>
> > > Bot-Commit: rubber-stamper@appspot.gserviceaccount.com <rubber-stamper@appspot.gserviceaccount.com>
> > > Cr-Commit-Position: refs/heads/main@{#1663689}
> >
> > Fixed: 518006007,502109333,502101200
> > Include-Ci-Only-Tests: chromium.mac:mac15-x64-rel-tests|browser_tests
> > Cq-Include-Trybots: luci.chromium.try:mac15-x64-rel-tests
> > Change-Id: I1ae6f9509f889b4b492c8ef4de0f27f90388913a
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8117687
> > Reviewed-by: Keren Zhu <kerenzhu@chromium.org>
> > Reviewed-by: Avi Drissman <avi@chromium.org>
> > Commit-Queue: Bryan Oltman <bryanoltman@google.com>
> > Cr-Commit-Position: refs/heads/main@{#1664862}
>
> Bug: 518006007,502109333,502101200
> Cq-Include-Trybots: luci.chromium.try:mac15-x64-rel-tests
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Change-Id: Ic23979a97b595e8a1ce8bb238acbdde41fd0b174
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8128775
> Owners-Override: Eriko Kurimoto <elkurin@chromium.org>
> Auto-Submit: Eriko Kurimoto <elkurin@chromium.org>
> Bot-Commit: rubber-stamper@appspot.gserviceaccount.com <rubber-stamper@appspot.gserviceaccount.com>
> Commit-Queue: rubber-stamper@appspot.gserviceaccount.com <rubber-stamper@appspot.gserviceaccount.com>
> Cr-Commit-Position: refs/heads/main@{#1665037}
Bug: 518006007,502109333,502101200
Cq-Include-Trybots: luci.chromium.try:mac15-x64-rel-tests; luci.chromium.try:linux_chromium_msan_rel_ng
Change-Id: I13a6db96aa0200411b93e2712340cb297cbac952
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8131917
Reviewed-by: Keren Zhu <kerenzhu@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Commit-Queue: Bryan Oltman <bryanoltman@google.com>
Cr-Commit-Position: refs/heads/main@{#1665550}
---
diff --git a/components/remote_cocoa/app_shim/application_bridge.mm b/components/remote_cocoa/app_shim/application_bridge.mm
index d5afd4b..af1a802 100644
--- a/components/remote_cocoa/app_shim/application_bridge.mm
+++ b/components/remote_cocoa/app_shim/application_bridge.mm
@@ -96,7 +96,6 @@
// Text input doesn't work across mojo yet.
return nullptr;
}
- bool MustPostTaskToRunModalSheetAnimation() const override { return true; }
mojo::AssociatedRemote<mojom::NativeWidgetNSWindowHost> host_remote_;
mojo::AssociatedRemote<mojom::TextInputHost> text_input_host_remote_;
diff --git a/components/remote_cocoa/app_shim/native_widget_ns_window_bridge.mm b/components/remote_cocoa/app_shim/native_widget_ns_window_bridge.mm
index 955e295..26132796 100644
--- a/components/remote_cocoa/app_shim/native_widget_ns_window_bridge.mm
+++ b/components/remote_cocoa/app_shim/native_widget_ns_window_bridge.mm
@@ -1648,14 +1648,7 @@
UpdateWindowDisplay();
// Add any children that were skipped during the fullscreen transition.
- // A weak pointer is needed because OrderChildren() can synchronously run a
- // modal sheet animation (ShowAsModalSheet), entering a nested run-loop during
- // which this bridge may be destroyed.
- base::WeakPtr<NativeWidgetNSWindowBridge> weak_ptr = factory_.GetWeakPtr();
OrderChildren();
- if (!weak_ptr) {
- return;
- }
host_->OnWindowFullscreenTransitionComplete(is_fullscreen);
if (is_fullscreen && immersive_mode_controller_) {
@@ -2228,42 +2221,47 @@
return;
}
- auto begin_sheet_closure = base::BindOnce(^{
- [parent_window beginSheet:window_
- completionHandler:^(NSModalResponse return_code) {
- // This class, NativeWidgetNSWindowBridge, clears the window's
- // delegate as an indication of its death, in which case this
- // completion handler will no-op. This is necessary to handle
- // AppKit invoking this selector via a posted task. See
- // https://crbug.com/851376.
- NSWindow* window = weak_window;
- if (!window.delegate) {
- return;
- }
- // Make sure to mark ourselves as not wanting to be visible.
- // Otherwise if during the orderOut call our parent becomes the
- // key window, it would try to show us as a new modal sheet.
- wants_to_be_visible_ = false;
- [window orderOut:nil];
- // Notify that the sheet window is closing.
- OnWindowWillClose();
- }];
- });
-
- if (host_helper_->MustPostTaskToRunModalSheetAnimation()) {
- // This function is called via mojo when using remote cocoa. Inside the
- // nested run loop, we will wait for a message providing the correctly-sized
- // frame for the new sheet. This message will not be processed until we
- // return from handling this message, because it will coming on the same
- // pipe. Avoid the resulting hang by posting a task to show the modal
- // sheet (which will be executed on a fresh stack, which will not block
- // the message).
- // https://crbug.com/1234509
- base::SingleThreadTaskRunner::GetCurrentDefault()->PostTask(
- FROM_HERE, std::move(begin_sheet_closure));
- } else {
- std::move(begin_sheet_closure).Run();
- }
+ base::WeakPtr<NativeWidgetNSWindowBridge> weak_this = factory_.GetWeakPtr();
+ // AppKit's sheet presentation animation runs a nested run loop. We post
+ // this task to run the animation asynchronously for two reasons:
+ // 1. For remote cocoa (out-of-process), it prevents a deadlock by allowing
+ // the sizing message to be processed before entering the nested run loop.
+ // 2. For in-process windows, it prevents Use-After-Free (UAF) bugs and
+ // undefined behavior when returning from the nested run loop if the
+ // window was closed during presentation, causing its bridge (this) to be
+ // destroyed.
+ // https://crbug.com/40781530, https://crbug.com/517040438,
+ // https://crbug.com/518006007
+ base::SingleThreadTaskRunner::GetCurrentDefault()->PostTask(
+ FROM_HERE, base::BindOnce(^{
+ NativeWidgetNSWindowBridge* bridge = weak_this.get();
+ if (!bridge || !bridge->wants_to_be_visible_) {
+ return;
+ }
+ [parent_window beginSheet:bridge->ns_window()
+ completionHandler:^(NSModalResponse return_code) {
+ // This class, NativeWidgetNSWindowBridge, clears the window's
+ // delegate as an indication of its death, in which case this
+ // completion handler will no-op. This is necessary to handle
+ // AppKit invoking this selector via a posted task. See
+ // https://crbug.com/41393772
+ NSWindow* window = weak_window;
+ if (!window.delegate) {
+ return;
+ }
+ // Make sure to mark ourselves as not wanting to be visible.
+ // Otherwise if during the orderOut call our parent becomes
+ // the key window, it would try to show us as a new modal
+ // sheet.
+ if (weak_this) {
+ weak_this->wants_to_be_visible_ = false;
+ }
+ [window orderOut:nil];
+ if (weak_this) {
+ weak_this->OnWindowWillClose();
+ }
+ }];
+ }));
}
} // namespace remote_cocoa
diff --git a/components/remote_cocoa/app_shim/native_widget_ns_window_host_helper.h b/components/remote_cocoa/app_shim/native_widget_ns_window_host_helper.h
index 298afa0..0b0dd04f 100644
--- a/components/remote_cocoa/app_shim/native_widget_ns_window_host_helper.h
+++ b/components/remote_cocoa/app_shim/native_widget_ns_window_host_helper.h
@@ -60,10 +60,6 @@
// TODO(ccameron): Remove the needs for this call.
virtual ui::TextInputClient* GetTextInputClient() = 0;
- // Return whether or not the nested run loop to animate in a sheet must be
- // made on a fresh stack, produced by posting a task to make the call.
- // https://crbug.com/1234509
- virtual bool MustPostTaskToRunModalSheetAnimation() const = 0;
};
} // namespace remote_cocoa
diff --git a/ui/views/cocoa/native_widget_mac_ns_window_host.h b/ui/views/cocoa/native_widget_mac_ns_window_host.h
index d3dcbb57..9e748f5 100644
--- a/ui/views/cocoa/native_widget_mac_ns_window_host.h
+++ b/ui/views/cocoa/native_widget_mac_ns_window_host.h
@@ -330,7 +330,6 @@
gfx::Point* baseline_point) override;
remote_cocoa::DragDropClient* GetDragDropClient() override;
ui::TextInputClient* GetTextInputClient() override;
- bool MustPostTaskToRunModalSheetAnimation() const override;
// remote_cocoa::ApplicationHost::Observer:
void OnApplicationHostDestroying(
diff --git a/ui/views/cocoa/native_widget_mac_ns_window_host.mm b/ui/views/cocoa/native_widget_mac_ns_window_host.mm
index 63428cd..6c60734 100644
--- a/ui/views/cocoa/native_widget_mac_ns_window_host.mm
+++ b/ui/views/cocoa/native_widget_mac_ns_window_host.mm
@@ -1068,10 +1068,6 @@
return text_input_host_->GetTextInputClient();
}
-bool NativeWidgetMacNSWindowHost::MustPostTaskToRunModalSheetAnimation() const {
- return false;
-}
-
////////////////////////////////////////////////////////////////////////////////
// NativeWidgetMacNSWindowHost, remote_cocoa::ApplicationHost::Observer:
void NativeWidgetMacNSWindowHost::OnApplicationHostDestroying(
diff --git a/chrome/browser/ui/cocoa/browser_window_mac_browsertest.mm b/chrome/browser/ui/cocoa/browser_window_mac_browsertest.mm
index 8c297816..1fb89328 100644
--- a/chrome/browser/ui/cocoa/browser_window_mac_browsertest.mm
+++ b/chrome/browser/ui/cocoa/browser_window_mac_browsertest.mm
@@ -8,6 +8,7 @@
#include <memory>
+#include "base/test/run_until.h"
#include "chrome/app/chrome_command_ids.h"
#include "chrome/browser/app_controller_mac.h"
#include "chrome/browser/bookmarks/bookmark_model_factory.h"
@@ -189,7 +190,8 @@
BookmarkEditor::SHOW_TREE, base::DoNothing());
editor->Show(browser()->GetWindow()->GetNativeWindow());
auto* editor_raw = editor.release();
- ASSERT_TRUE([AppController.sharedController keyWindowIsModal]);
+ ASSERT_TRUE(base::test::RunUntil(
+ [&]() { return [AppController.sharedController keyWindowIsModal]; }));
// These commands should be disabled when the sheet is attached.
EXPECT_FALSE([window validateUserInterfaceItem:bookmark_all_tabs_item]);
@@ -198,7 +200,8 @@
// Close the sheet dialog.
editor_raw->GetWidget()->CloseNow();
- ASSERT_FALSE([AppController.sharedController keyWindowIsModal]);
+ ASSERT_TRUE(base::test::RunUntil(
+ [&]() { return ![AppController.sharedController keyWindowIsModal]; }));
// These commands should be enabled again when the sheet is removed.
EXPECT_TRUE([window validateUserInterfaceItem:bookmark_all_tabs_item]);
diff --git a/chrome/browser/ui/extensions/settings_overridden_dialog_browsertest.cc b/chrome/browser/ui/extensions/settings_overridden_dialog_browsertest.cc
index 43d7471..62843b66 100644
--- a/chrome/browser/ui/extensions/settings_overridden_dialog_browsertest.cc
+++ b/chrome/browser/ui/extensions/settings_overridden_dialog_browsertest.cc
@@ -13,6 +13,7 @@
#include "base/memory/raw_ptr.h"
#include "base/path_service.h"
#include "base/strings/string_util.h"
+#include "base/task/current_thread.h"
#include "base/test/scoped_feature_list.h"
#include "base/time/time.h"
#include "chrome/browser/extensions/chrome_test_extension_loader.h"
@@ -132,7 +133,9 @@
std::make_unique<TestDialogController>(std::move(params),
&dialog_result_),
browser->GetWindow()->GetNativeWindow());
- return waiter.WaitIfNeededAndGet();
+ views::Widget* widget = waiter.WaitIfNeededAndGet();
+ EXPECT_TRUE(base::test::RunUntil([&] { return widget->IsVisible(); }));
+ return widget;
}
void ShowNtpOverriddenDefaultDialog() {
diff --git a/chrome/browser/ui/signin/signin_view_controller_interactive_uitest.cc b/chrome/browser/ui/signin/signin_view_controller_interactive_uitest.cc
index 3dfb0e6..8d01dd8a 100644
--- a/chrome/browser/ui/signin/signin_view_controller_interactive_uitest.cc
+++ b/chrome/browser/ui/signin/signin_view_controller_interactive_uitest.cc
@@ -42,6 +42,7 @@
#include "components/signin/public/identity_manager/identity_manager.h"
#include "components/signin/public/identity_manager/identity_test_utils.h"
#include "components/sync/base/features.h"
+#include "content/public/browser/render_widget_host_view.h"
#include "content/public/browser/web_contents.h"
#include "content/public/test/browser_test.h"
#include "content/public/test/browser_test_utils.h"
@@ -231,6 +232,14 @@
EXPECT_TRUE(signin_view_controller->ShowsModalDialog());
content_observer.Wait();
+ // Wait for the modal dialog sheet window to be presented and receive focus.
+ ASSERT_TRUE(base::test::RunUntil([&]() {
+ auto* web_contents =
+ signin_view_controller->GetModalDialogWebContentsForTesting();
+ return web_contents && web_contents->GetRenderWidgetHostView() &&
+ web_contents->GetRenderWidgetHostView()->HasFocus();
+ }));
+
content::WebContentsDestroyedWatcher dialog_destroyed_watcher(
signin_view_controller->GetModalDialogWebContentsForTesting());
diff --git a/chrome/browser/ui/test/test_browser_dialog.cc b/chrome/browser/ui/test/test_browser_dialog.cc
index 3ca4d2f..ef42317 100644
--- a/chrome/browser/ui/test/test_browser_dialog.cc
+++ b/chrome/browser/ui/test/test_browser_dialog.cc
@@ -28,6 +28,7 @@
#if defined(TOOLKIT_VIEWS)
#include "base/strings/strcat.h"
+#include "base/test/run_until.h"
#include "third_party/abseil-cpp/absl/cleanup/cleanup.h"
#include "ui/display/display.h"
#include "ui/display/screen.h"
@@ -119,6 +120,13 @@
}
views::Widget* dialog_widget = *(added.begin());
+ if (ShouldWaitForDialogBeforeVerify()) {
+ if (!base::test::RunUntil([&]() { return dialog_widget->IsVisible(); })) {
+ LOG(ERROR) << "VerifyUi(): Widget failed to become visible.";
+ return false;
+ }
+ }
+
dialog_widget->SetBlockCloseForTesting(true);
// Deactivate before taking screenshot. Deactivated dialog pixel outputs
// is more predictable than activated dialog.
@@ -200,6 +208,10 @@
return false;
}
+bool TestBrowserDialog::ShouldWaitForDialogBeforeVerify() {
+ return BUILDFLAG(IS_MAC);
+}
+
std::string TestBrowserDialog::GetNonDialogName() {
return std::string();
}
diff --git a/chrome/browser/ui/test/test_browser_dialog.h b/chrome/browser/ui/test/test_browser_dialog.h
index 08da2610..80a3388c 100644
--- a/chrome/browser/ui/test/test_browser_dialog.h
+++ b/chrome/browser/ui/test/test_browser_dialog.h
@@ -65,6 +65,12 @@
// Widget::CanClose() and DialogDelegate::Close().
virtual bool AlwaysCloseAsynchronously();
+ // Whether to wait for the dialog widget to become visible in VerifyUi() by
+ // pumping the run loop. Subclasses that run blocking modal loops should
... (file diff truncated)
Loading diff…
Regression Test / PoC
shipped with the fix
diff --git a/chrome/browser/ui/cocoa/browser_window_mac_browsertest.mm b/chrome/browser/ui/cocoa/browser_window_mac_browsertest.mm
index 8c297816..1fb89328 100644
--- a/chrome/browser/ui/cocoa/browser_window_mac_browsertest.mm
+++ b/chrome/browser/ui/cocoa/browser_window_mac_browsertest.mm
@@ -8,6 +8,7 @@
#include <memory>
+#include "base/test/run_until.h"
#include "chrome/app/chrome_command_ids.h"
#include "chrome/browser/app_controller_mac.h"
#include "chrome/browser/bookmarks/bookmark_model_factory.h"
@@ -189,7 +190,8 @@
BookmarkEditor::SHOW_TREE, base::DoNothing());
editor->Show(browser()->GetWindow()->GetNativeWindow());
auto* editor_raw = editor.release();
- ASSERT_TRUE([AppController.sharedController keyWindowIsModal]);
+ ASSERT_TRUE(base::test::RunUntil(
+ [&]() { return [AppController.sharedController keyWindowIsModal]; }));
// These commands should be disabled when the sheet is attached.
EXPECT_FALSE([window validateUserInterfaceItem:bookmark_all_tabs_item]);
@@ -198,7 +200,8 @@
// Close the sheet dialog.
editor_raw->GetWidget()->CloseNow();
- ASSERT_FALSE([AppController.sharedController keyWindowIsModal]);
+ ASSERT_TRUE(base::test::RunUntil(
+ [&]() { return ![AppController.sharedController keyWindowIsModal]; }));
// These commands should be enabled again when the sheet is removed.
EXPECT_TRUE([window validateUserInterfaceItem:bookmark_all_tabs_item]);
diff --git a/chrome/browser/ui/extensions/settings_overridden_dialog_browsertest.cc b/chrome/browser/ui/extensions/settings_overridden_dialog_browsertest.cc
index 43d7471..62843b66 100644
--- a/chrome/browser/ui/extensions/settings_overridden_dialog_browsertest.cc
+++ b/chrome/browser/ui/extensions/settings_overridden_dialog_browsertest.cc
@@ -13,6 +13,7 @@
#include "base/memory/raw_ptr.h"
#include "base/path_service.h"
#include "base/strings/string_util.h"
+#include "base/task/current_thread.h"
#include "base/test/scoped_feature_list.h"
#include "base/time/time.h"
#include "chrome/browser/extensions/chrome_test_extension_loader.h"
@@ -132,7 +133,9 @@
std::make_unique<TestDialogController>(std::move(params),
&dialog_result_),
browser->GetWindow()->GetNativeWindow());
- return waiter.WaitIfNeededAndGet();
+ views::Widget* widget = waiter.WaitIfNeededAndGet();
+ EXPECT_TRUE(base::test::RunUntil([&] { return widget->IsVisible(); }));
+ return widget;
}
void ShowNtpOverriddenDefaultDialog() {
diff --git a/chrome/browser/ui/signin/signin_view_controller_interactive_uitest.cc b/chrome/browser/ui/signin/signin_view_controller_interactive_uitest.cc
index 3dfb0e6..8d01dd8a 100644
--- a/chrome/browser/ui/signin/signin_view_controller_interactive_uitest.cc
+++ b/chrome/browser/ui/signin/signin_view_controller_interactive_uitest.cc
@@ -42,6 +42,7 @@
#include "components/signin/public/identity_manager/identity_manager.h"
#include "components/signin/public/identity_manager/identity_test_utils.h"
#include "components/sync/base/features.h"
+#include "content/public/browser/render_widget_host_view.h"
#include "content/public/browser/web_contents.h"
#include "content/public/test/browser_test.h"
#include "content/public/test/browser_test_utils.h"
@@ -231,6 +232,14 @@
EXPECT_TRUE(signin_view_controller->ShowsModalDialog());
content_observer.Wait();
+ // Wait for the modal dialog sheet window to be presented and receive focus.
+ ASSERT_TRUE(base::test::RunUntil([&]() {
+ auto* web_contents =
+ signin_view_controller->GetModalDialogWebContentsForTesting();
+ return web_contents && web_contents->GetRenderWidgetHostView() &&
+ web_contents->GetRenderWidgetHostView()->HasFocus();
+ }));
+
content::WebContentsDestroyedWatcher dialog_destroyed_watcher(
signin_view_controller->GetModalDialogWebContentsForTesting());
diff --git a/chrome/browser/ui/test/test_browser_dialog.cc b/chrome/browser/ui/test/test_browser_dialog.cc
index 3ca4d2f..ef42317 100644
--- a/chrome/browser/ui/test/test_browser_dialog.cc
+++ b/chrome/browser/ui/test/test_browser_dialog.cc
@@ -28,6 +28,7 @@
#if defined(TOOLKIT_VIEWS)
#include "base/strings/strcat.h"
+#include "base/test/run_until.h"
#include "third_party/abseil-cpp/absl/cleanup/cleanup.h"
#include "ui/display/display.h"
#include "ui/display/screen.h"
@@ -119,6 +120,13 @@
}
views::Widget* dialog_widget = *(added.begin());
+ if (ShouldWaitForDialogBeforeVerify()) {
+ if (!base::test::RunUntil([&]() { return dialog_widget->IsVisible(); })) {
+ LOG(ERROR) << "VerifyUi(): Widget failed to become visible.";
+ return false;
+ }
+ }
+
dialog_widget->SetBlockCloseForTesting(true);
// Deactivate before taking screenshot. Deactivated dialog pixel outputs
// is more predictable than activated dialog.
@@ -200,6 +208,10 @@
return false;
}
+bool TestBrowserDialog::ShouldWaitForDialogBeforeVerify() {
+ return BUILDFLAG(IS_MAC);
+}
+
std::string TestBrowserDialog::GetNonDialogName() {
return std::string();
}
diff --git a/chrome/browser/ui/test/test_browser_dialog.h b/chrome/browser/ui/test/test_browser_dialog.h
index 08da2610..80a3388c 100644
--- a/chrome/browser/ui/test/test_browser_dialog.h
+++ b/chrome/browser/ui/test/test_browser_dialog.h
@@ -65,6 +65,12 @@
// Widget::CanClose() and DialogDelegate::Close().
virtual bool AlwaysCloseAsynchronously();
+ // Whether to wait for the dialog widget to become visible in VerifyUi() by
+ // pumping the run loop. Subclasses that run blocking modal loops should
+ // return false to avoid deadlocks. Defaults to true on macOS to account for
+ // asynchronous modal sheet presentation, and false on other platforms.
+ virtual bool ShouldWaitForDialogBeforeVerify();
+
// Get the name of a non-dialog window that should be included in testing.
// VerifyUi() only considers dialog windows and windows with a matching name.
virtual std::string GetNonDialogName();
diff --git a/chrome/browser/ui/views/policy/enterprise_startup_dialog_view_browsertest.cc b/chrome/browser/ui/views/policy/enterprise_startup_dialog_view_browsertest.cc
index dea0cc68..604f521 100644
--- a/chrome/browser/ui/views/policy/enterprise_startup_dialog_view_browsertest.cc
+++ b/chrome/browser/ui/views/policy/enterprise_startup_dialog_view_browsertest.cc
@@ -53,6 +53,10 @@
}
#endif
+ // EnterpriseStartupDialogView runs a blocking modal loop, so return false to
+ // avoid deadlocks when pumping the run loop in VerifyUi().
+ bool ShouldWaitForDialogBeforeVerify() override { return false; }
+
private:
raw_ptr<EnterpriseStartupDialogView, AcrossTasksDanglingUntriaged> dialog;
};
diff --git a/chrome/browser/ui/views/relaunch_notification/relaunch_notification_controller_platform_impl_desktop_browsertest.cc b/chrome/browser/ui/views/relaunch_notification/relaunch_notification_controller_platform_impl_desktop_browsertest.cc
index f52abab..0cbee8be 100644
--- a/chrome/browser/ui/views/relaunch_notification/relaunch_notification_controller_platform_impl_desktop_browsertest.cc
+++ b/chrome/browser/ui/views/relaunch_notification/relaunch_notification_controller_platform_impl_desktop_browsertest.cc
@@ -5,6 +5,7 @@
#include "chrome/browser/ui/views/relaunch_notification/relaunch_notification_controller_platform_impl_desktop.h"
#include "base/run_loop.h"
+#include "base/test/run_until.h"
#include "base/time/time.h"
#include "chrome/browser/ui/views/frame/browser_view.h"
#include "chrome/test/base/in_process_browser_test.h"
@@ -49,13 +50,18 @@
impl.NotifyRelaunchRequired(deadline,
/*is_notification_style_ap_required=*/false,
base::OnceCallback<base::Time()>());
+ ASSERT_TRUE(base::test::RunUntil(
+ [&]() { return impl.GetWidgetForTesting() != nullptr; }));
views::test::WidgetDestroyedWaiter fourth_destroyed_waiter(
impl.GetWidgetForTesting());
impl.CloseRelaunchNotification();
fourth_destroyed_waiter.Wait();
+
impl.NotifyRelaunchRequired(deadline,
/*is_notification_style_ap_required=*/false,
base::OnceCallback<base::Time()>());
+ ASSERT_TRUE(base::test::RunUntil(
+ [&]() { return impl.GetWidgetForTesting() != nullptr; }));
views::test::WidgetDestroyedWaiter fifth_destroyed_waiter(
impl.GetWidgetForTesting());
impl.CloseRelaunchNotification();
@@ -65,6 +71,8 @@
impl.NotifyRelaunchRequired(deadline,
/*is_notification_style_ap_required=*/true,
base::OnceCallback<base::Time()>());
+ ASSERT_TRUE(base::test::RunUntil(
+ [&]() { return impl.GetWidgetForTesting() != nullptr; }));
views::test::WidgetDestroyedWaiter sixth_destroyed_waiter(
impl.GetWidgetForTesting());
impl.CloseRelaunchNotification();
diff --git a/chrome/browser/ui/views/safe_browsing/tailored_security_desktop_dialog_manager_browsertest.cc b/chrome/browser/ui/views/safe_browsing/tailored_security_desktop_dialog_manager_browsertest.cc
index 0bdb675d..b1a4c64 100644
--- a/chrome/browser/ui/views/safe_browsing/tailored_security_desktop_dialog_manager_browsertest.cc
+++ b/chrome/browser/ui/views/safe_browsing/tailored_security_desktop_dialog_manager_browsertest.cc
@@ -7,6 +7,7 @@
#include "base/command_line.h"
#include "base/test/metrics/histogram_tester.h"
#include "base/test/metrics/user_action_tester.h"
+#include "base/test/run_until.h"
#include "chrome/browser/ui/browser.h"
#include "chrome/browser/ui/tabs/tab_strip_model.h"
#include "chrome/browser/ui/test/test_browser_dialog.h"
@@ -21,6 +22,7 @@
#include "ui/gfx/geometry/point.h"
#include "ui/gfx/scoped_animation_duration_scale_mode.h"
#include "ui/views/bubble/bubble_dialog_delegate_view.h"
+#include "ui/views/test/widget_test.h"
#include "ui/views/widget/any_widget_observer.h"
#include "ui/views/widget/widget.h"
#include "url/gurl.h"
@@ -106,7 +108,9 @@
safe_browsing::kTailoredSecurityNoticeDialog);
dialog_manager_->ShowEnabledDialogForBrowser(browser, base::DoNothing());
- return waiter.WaitIfNeededAndGet();
+ views::Widget* widget = waiter.WaitIfNeededAndGet();
+ EXPECT_TRUE(base::test::RunUntil([&]() { return widget->IsVisible(); }));
+ return widget;
}
views::Widget* ShowTailoredSecurityDisabledDialog(Browser* browser) {
@@ -115,7 +119,9 @@
safe_browsing::kTailoredSecurityNoticeDialog);
dialog_manager_->ShowDisabledDialogForBrowser(browser, base::DoNothing());
- return waiter.WaitIfNeededAndGet();
+ views::Widget* widget = waiter.WaitIfNeededAndGet();
+ EXPECT_TRUE(base::test::RunUntil([&]() { return widget->IsVisible(); }));
+ return widget;
}
private:
@@ -308,8 +314,9 @@
IN_PROC_BROWSER_TEST_P(TailoredSecurityDesktopDialogManagerTest,
OpeningANewEnableDialogWillCloseAnyOpenDisableDialogs) {
auto* disabled_dialog = ShowTailoredSecurityDisabledDialog(browser());
+ views::test::WidgetDestroyedWaiter destroyed_waiter(disabled_dialog);
auto* enabled_dialog = ShowTailoredSecurityEnabledDialog(browser());
- EXPECT_TRUE(disabled_dialog->IsClosed());
+ destroyed_waiter.Wait();
EXPECT_FALSE(enabled_dialog->IsClosed());
}
@@ -336,8 +343,9 @@
IN_PROC_BROWSER_TEST_P(TailoredSecurityDesktopDialogManagerTest,
OpeningANewDisableDialogWillCloseAnyOpenEnableDialogs) {
auto* enabled_dialog = ShowTailoredSecurityEnabledDialog(browser());
+ views::test::WidgetDestroyedWaiter destroyed_waiter(enabled_dialog);
auto* disabled_dialog = ShowTailoredSecurityDisabledDialog(browser());
- EXPECT_TRUE(enabled_dialog->IsClosed());
+ destroyed_waiter.Wait();
EXPECT_FALSE(disabled_dialog->IsClosed());
}
diff --git a/chrome/browser/ui/views/web_apps/protocol_handler_launch_dialog_browsertest.cc b/chrome/browser/ui/views/web_apps/protocol_handler_launch_dialog_browsertest.cc
index 4e874453..868700c 100644
--- a/chrome/browser/ui/views/web_apps/protocol_handler_launch_dialog_browsertest.cc
+++ b/chrome/browser/ui/views/web_apps/protocol_handler_launch_dialog_browsertest.cc
@@ -37,6 +37,7 @@
#include "services/network/public/mojom/permissions_policy/permissions_policy_feature.mojom.h"
#include "third_party/blink/public/common/features.h"
#include "ui/views/test/dialog_test.h"
+#include "ui/views/test/widget_test.h"
#include "ui/views/view.h"
#include "ui/views/widget/any_widget_observer.h"
#include "ui/views/widget/widget.h"
@@ -152,9 +153,19 @@
webapps::AppId test_app_id = InstallTestWebApp(browser()->GetProfile());
ShowWebAppProtocolLaunchDialog(protocol_url, browser()->GetProfile(),
test_app_id, base::DoNothing());
- waiter.WaitIfNeededAndGet()->CloseWithReason(
- views::Widget::ClosedReason::kEscKeyPressed);
+ widget_ = waiter.WaitIfNeededAndGet()->GetWeakPtr();
}
+
+ void DismissUi() override {
+ if (widget_) {
+ views::test::WidgetDestroyedWaiter waiter(widget_.get());
+ widget_->CloseWithReason(views::Widget::ClosedReason::kEscKeyPressed);
+ waiter.Wait();
+ }
+ }
+
+ private:
+ base::WeakPtr<views::Widget> widget_;
};
IN_PROC_BROWSER_TEST_F(
diff --git a/chrome/browser/ui/views/web_apps/web_app_update_review_dialog_browsertest.cc b/chrome/browser/ui/views/web_apps/web_app_update_review_dialog_browsertest.cc
index d65d724..ba35a3f 100644
--- a/chrome/browser/ui/views/web_apps/web_app_update_review_dialog_browsertest.cc
+++ b/chrome/browser/ui/views/web_apps/web_app_update_review_dialog_browsertest.cc
@@ -16,6 +16,7 @@
#include "base/test/bind.h"
#include "base/test/metrics/histogram_tester.h"
#include "base/test/metrics/user_action_tester.h"
+#include "base/test/run_until.h"
#include "base/test/test_future.h"
#include "base/threading/thread_restrictions.h"
#include "base/time/time.h"
@@ -448,6 +449,8 @@
... (truncated)
Loading diff…
Original Bug Report
reported by vm...@google.com
Potential UAF via iterator invalidation in NativeWidgetNSWindowBridge::OrderChildren
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 without the Chrome Security team.
Overview: A potential Use-After-Free (UAF) exists in the macOS browser process due to iterator invalidation in NativeWidgetNSWindowBridge::OrderChildren. A synchronous nested run loop allows task execution that can mutate the child_windows_ vector during a range-based for loop iteration. Resuming the loop dereferences a dangling pointer, potentially leading to arbitrary code execution.
Affected files:
components/remote_cocoa/app_shim/native_widget_ns_window_bridge.mmui/views/cocoa/native_widget_mac_ns_window_host.mmui/views/widget/native_widget_mac.mmcomponents/remote_cocoa/app_shim/views_nswindow_delegate.mm
Estimated timestamp from git blame: 2023-07-07
Description
A potential Use-After-Free (UAF) vulnerability exists in the macOS implementation of NativeWidgetNSWindowBridge::OrderChildren.
When a window’s visibility changes (e.g., from an AppKit windowDidDeminiaturize: callback), OrderChildren iterates over the child_windows_ vector using a C++ range-based for loop (for (auto* child : child_windows_)). A range-based for loop caches the end iterator of the vector before the loop begins.
During iteration, if a child window is a modal sheet, the code calls child->ShowAsModalSheet(). For in-process Mac windows, this synchronously invokes the native macOS API -[NSWindow beginSheet:completionHandler:]. This native method spins a nested run loop to handle the sheet animation.
Because the call stack originated directly from a native OS AppKit callback and not from within a standard Chrome task loop, Chrome’s re-entrancy protection (task_execution_allowed_ being set to false in DoWorkImpl) is bypassed. The nested message pump will process pending Chrome application tasks.
If one of these pending tasks closes another child window attached to the same parent, that child window is deleted and erased from the child_windows_ vector. This vector mutation shrinks the logical size of the vector, invalidating the cached end iterator of the paused range-based for loop.
When the nested run loop exits and the iteration resumes, the loop reads past the new logical end of the vector, fetching a stale raw pointer from the vector’s capacity buffer. This dangling pointer points to the newly freed child window object. The loop then reads member variables from this freed object (child->wants_to_be_visible(), child->window_) and passes the attacker-controlled NSWindow* to [window addChildWindow:child_window ordered:NSWindowAbove], providing a strong primitive for Remote Code Execution (RCE) in the unsandboxed browser process.
Suggested Reproduction Steps
Note: These are suggested/potential steps to trigger the vulnerability. Our tooling agent does not yet have the ability to run code to verify them dynamically.
- Open a standard browser window on macOS (ensuring an in-process NativeWidgetNSWindowBridge).
- Trigger the creation of two child windows on this parent: a modal dialog (e.g., a certificate selector) and a standard bubble (e.g., a permission prompt).
- Minimize the parent window. The modal sheet is queued to be shown but remains hidden because the parent is minimized.
- Queue a malicious task or Mojo IPC message that will close the standard bubble when executed.
- Restore (deminiaturize) the parent window.
- The OS invokes
windowDidDeminiaturize:, leading toOrderChildren. OrderChildrenprocesses the modal dialog and calls-[NSWindow beginSheet:], spinning the nested run loop.- The queued task executes during the nested loop, destroying the standard bubble and erasing it from
child_windows_. - The nested loop returns, the range-based for loop resumes with an invalid iterator, and a UAF occurs.
Suggested Fix
Avoid iterating directly over the mutable child_windows_ vector if the loop body can spin a nested run loop.
Since NativeWidgetNSWindowBridge supports base::WeakPtr (via its factory_), a safe approach is to create a local vector of weak pointers to the children before iterating:
std::vector<base::WeakPtr<NativeWidgetNSWindowBridge>> children_copy;
children_copy.reserve(child_windows_.size());
for (auto* child : child_windows_) {
children_copy.push_back(child->GetWeakPtr());
}
for (auto& weak_child : children_copy) {
if (!weak_child)
continue;
// ... existing loop logic ...
}
This ensures that if a child is destroyed during the nested run loop, the weak pointer will safely evaluate to null when the iteration resumes.
Evaluated with Chrome root at commit: 096fc8fdbfacf2546485756d03f160a3d04fcc9b
Results so far have been promising, but there can be wrong deductions. If this proves to be a false positive, please close as WAI; 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