Chrome · Views
CVE-2026-79128
UAF in Views
Overview
Medium
Severity
—
CVSS
No
Exploited ITW
Fixed
Fix Status
Changed Functions
| Function | Change | Notes |
|---|---|---|
ASSERT_TRUEchrome/browser/ui/signin/signin_view_controller_interactive_uitest.cc |
modified |
Files Changed
chrome/browser/ui/cocoa/browser_window_mac_browsertest.mmchrome/browser/ui/extensions/settings_overridden_dialog_browsertest.ccchrome/browser/ui/signin/signin_view_controller_interactive_uitest.ccchrome/browser/ui/test/test_browser_dialog.cc
Patch
From c763f3b4837ea55058610ecf628cd140ef40b52e Mon Sep 17 00:00:00 2001 From: Bryan Oltman <[email protected]> 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 <[email protected]> > > > > Reviewed-by: Keren Zhu <[email protected]> > > > > 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 <[email protected]> > > > Commit-Queue: Kalvin Lee <[email protected]> > > > Bot-Commit: [email protected] <[email protected]> > > > 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 <[email protected]> > > Reviewed-by: Avi Drissman <[email protected]> > > Commit-Queue: Bryan Oltman <[email protected]> > > 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 <[email protected]> > Auto-Submit: Eriko Kurimoto <[email protected]> > Bot-Commit: [email protected] <[email protected]> > Commit-Queue: [email protected] <[email protected]> > 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 <[email protected]> Reviewed-by: Avi Drissman <[email protected]> Commit-Queue: Bryan Oltman <[email protected]> Cr-Commit-Position: refs/heads/main@{#1665550} --- 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
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 [email protected]
Potential UAF in app_shim via implicit `this` capture in ShowAsModalSheet
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 unsandboxed app_shim process. An Objective-C block in NativeWidgetNSWindowBridge::ShowAsModalSheet implicitly captures a raw this pointer and is posted to the task runner. If the Mojo pipe is disconnected before the block executes, the bridge is destroyed, leading to a UAF that could potentially be exploited for Remote Code Execution.
Affected files:
components/remote_cocoa/app_shim/native_widget_ns_window_bridge.mmcomponents/remote_cocoa/app_shim/application_bridge.mmcomponents/remote_cocoa/app_shim/native_widget_ns_window_bridge.h
Estimated timestamp from git blame: 2024-03-28
Summary
A potential Use-After-Free (UAF) vulnerability exists in the macOS app_shim process within the NativeWidgetNSWindowBridge::ShowAsModalSheet method. The vulnerability arises because an Objective-C block implicitly captures the raw C++ this pointer by value. When the block is posted to a task runner and delayed, a race condition can allow the NativeWidgetNSWindowBridge instance to be deleted before the block executes, leading to a UAF.
Because the app_shim process on macOS operates without a sandbox (switches::kAppShimProcessType maps to Sandbox::kNoSandbox), successful exploitation of this memory corruption could yield full user privileges (Remote Code Execution).
Technical Details
In components/remote_cocoa/app_shim/native_widget_ns_window_bridge.mm, the ShowAsModalSheet() method prepares an Objective-C block (begin_sheet_closure) to display a modal sheet:
auto begin_sheet_closure = base::BindOnce(^{
[parent_window beginSheet:window_
completionHandler:^(NSModalResponse return_code) {
// ...
wants_to_be_visible_ = false;
OnWindowWillClose();
}];
});
Inside this block, the code accesses the class member variable window_ and within the nested completionHandler, it accesses wants_to_be_visible_ and calls the member function OnWindowWillClose(). In Objective-C++, referencing member variables or functions inside a block implicitly captures the C++ this pointer by value as a raw pointer.
When running in an out-of-process App Shim (e.g., a PWA), host_helper_->MustPostTaskToRunModalSheetAnimation() returns true (see NativeWidgetBridgeOwner in application_bridge.mm). Consequently, ShowAsModalSheet posts begin_sheet_closure to the UI thread’s task runner rather than executing it synchronously.
Potential Exploitation Steps
Note: These are suggested steps; our tooling agent does not have the ability to run code to verify an end-to-end exploit.
- An attacker compromises a PWA renderer process or creates a malicious PWA.
- The attacker’s JavaScript triggers an action that causes the browser to show a window-modal dialog (e.g.,
navigator.subApps.add()). - The browser process sends a
SetVisibilityState(kShow)Mojo message to theapp_shimprocess. - Immediately after triggering the modal, the attacker cancels the dialog or closes the parent window, causing the browser to close the
mojom::NativeWidgetNSWindowMojo pipe. - The
app_shim’s IO thread receives both the visibility message and the pipe disconnection notification sequentially, posting them to the UI thread’s task queue. - The UI thread executes the visibility task, routing to
ShowAsModalSheet(). This creates the block capturing the rawthispointer and appends it as a new task to the end of the UI thread’s queue. - The UI thread then executes the Mojo disconnect task. This calls
NativeWidgetBridgeOwner::OnMojoDisconnect(), which executesdelete this;, destroying theNativeWidgetNSWindowBridgeand freeing its memory. - The attacker sprays the heap via other IPC messages, aiming to reclaim the freed
NativeWidgetNSWindowBridgememory with attacker-controlled data. - The UI thread finally executes the delayed
begin_sheet_closuretask. The block dereferences the implicitly captured, now-freedthispointer. - If the memory was successfully reclaimed, the attacker-controlled data is passed to
[NSWindow beginSheet:]. When AppKit attempts to send an Objective-C message to this corrupted object, it jumps to an attacker-controlled address, leading to RCE in the unsandboxedapp_shimprocess.
Since the captured this pointer is a raw C++ pointer and not explicitly a base::raw_ptr<T>, MiraclePtr protection does not mitigate this vulnerability.
Suggested Fix
To prevent the UAF, use base::WeakPtr to safely capture a reference to the NativeWidgetNSWindowBridge instance inside the Objective-C block. If the object is destroyed before the block executes, the weak pointer will be safely nullified.
base::WeakPtr<NativeWidgetNSWindowBridge> weak_this = factory_.GetWeakPtr();
auto begin_sheet_closure = base::BindOnce(^{
if (!weak_this) {
return;
}
[parent_window beginSheet:weak_this->window_
completionHandler:^(NSModalResponse return_code) {
if (!weak_this) {
return;
}
weak_this->wants_to_be_visible_ = false;
weak_this->OnWindowWillClose();
}];
});
(Note: factory_ must be added to NativeWidgetNSWindowBridge if it does not already exist).
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