From 6192ad9e4b4f24b9979c164e5aca27832ffa5273 Mon Sep 17 00:00:00 2001
From: Anna Tsvirchkova <[email protected]>
Date: Mon, 17 Aug 2026 03:16:50 -0700
Subject: [PATCH] Fix Use-after-free in SuccessfulPasswordChangeBubbleController

The use-after-free would be the following:
    User->>View: 1. Clicks Eye Icon to reveal password
    View->>OS: 2. Starts Async Re-Auth (passes callback with raw pointers)
    User->>View: 3. Closes Bubble / Tab while Auth dialog is open
    Note over View: 4. View & Labels destroyed (Heap memory FREED)
    OS->>View: 5. Auth finishes & executes callback
    Note over View: 6. Callback calls password_label->SetObscured()

The CL wraps handling the authentication result into a method, which is
passed to biometric auth via a WeakPtr. It also moves eye button click
handler into a method for conveniency.

Bug: 544484669
Change-Id: Ib6bb0c7e6ddda5b7ed07e0c381d799dc9460c130
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8261134
Commit-Queue: Anna Tsvirchkova <[email protected]>
Reviewed-by: Mohamed Amir Yosef <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1680431}
---

diff --git a/chrome/browser/ui/views/passwords/password_change/successful_password_change_view.cc b/chrome/browser/ui/views/passwords/password_change/successful_password_change_view.cc
index a90fbbb..d0a36bd 100644
--- a/chrome/browser/ui/views/passwords/password_change/successful_password_change_view.cc
+++ b/chrome/browser/ui/views/passwords/password_change/successful_password_change_view.cc
@@ -67,102 +67,6 @@
   return view;
 }
 
-// Row containing favicon, username, password and eye icon:
-// *--------------------------------------------------------*
-// |         | Username                          |          |
-// | Favicon |-----------------------------------| Eye icon |
-// |         | Password                          |          |
-// *--------------------------------------------------------*
-std::unique_ptr<views::View> CreateUsernamePasswordWithEyeIcon(
-    SuccessfulPasswordChangeBubbleController* controller) {
-  CHECK(controller);
-
-  auto parent_view = std::make_unique<views::BoxLayoutView>();
-  parent_view->SetInsideBorderInsets(ComputeRowMargins());
-
-  // Set the same spacing as for RichHoverButton which is displayed bellow.
-  const int icon_label_spacing = ChromeLayoutProvider::Get()->GetDistanceMetric(
-      DISTANCE_RICH_HOVER_BUTTON_ICON_HORIZONTAL);
-  parent_view->SetBetweenChildSpacing(icon_label_spacing);
-
-  // Add favicon.
-  views::ImageView* favicon_view =
-      parent_view->AddChildView(std::make_unique<views::ImageView>());
-  const int icon_size = GetLayoutConstant(LayoutConstant::kPageInfoIconSize);
-  favicon_view->SetImageSize({icon_size, icon_size});
-  favicon_view->SetImage(ui::ImageModel::FromVectorIcon(
-      features::IsRoundedIconsEnabled() ? vector_icons::kGlobeIcon
-                                        : vector_icons::kGlobeOldIcon,
-      ui::kColorIcon, gfx::kFaviconSize));
-  controller->RequestFavicon(base::BindOnce(
-      [](views::ImageView* favicon_view, const gfx::Image& favicon) {
-        if (!favicon.IsEmpty()) {
-          favicon_view->SetImage(ui::ImageModel::FromImage(favicon));
-        }
-      },
-      favicon_view));
-
-  // Add username/password labels.
-  auto* username_password_view =
-      parent_view->AddChildView(CreateVerticalStackView());
-  username_password_view->SetProperty(views::kBoxLayoutFlexKey,
-                                      views::BoxLayoutFlexSpecification());
-  username_password_view->AddChildView(
-      CreateUsernameLabel(controller->GetUsername()));
-  views::Label* password_label = username_password_view->AddChildView(
-      CreatePasswordLabel(controller->GetNewPassword()));
-
-  // Add eye icon which allows to reveal a password.
-  auto* eye_icon = parent_view->AddChildView(
-      CreateVectorToggleImageButton(views::Button::PressedCallback()));
-  eye_icon->SetTooltipText(
-      l10n_util::GetStringUTF16(IDS_MANAGE_PASSWORDS_SHOW_PASSWORD));
-  eye_icon->SetToggledTooltipText(
-      l10n_util::GetStringUTF16(IDS_MANAGE_PASSWORDS_HIDE_PASSWORD));
-  eye_icon->SetImageVerticalAlignment(views::ImageButton::ALIGN_MIDDLE);
-  views::SetImageFromVectorIconWithColor(eye_icon,
-                                         features::IsRoundedIconsEnabled()
-                                             ? views::kVisibilityFilledIcon
-                                             : views::kEyeOldIcon,
-                                         {ui::kColorIcon, ui::kColorIcon});
-  views::SetToggledImageFromVectorIconWithColor(
-      eye_icon,
-      features::IsRoundedIconsEnabled() ? views::kVisibilityOffFilledIcon
-                                        : views::kEyeCrossedOldIcon,
-      {ui::kColorIcon, ui::kColorIcon});
-
-  base::RepeatingCallback<void(bool)> auth_result_callback =
-      base::BindRepeating(
-          [](views::ToggleImageButton* toggle_button,
-             views::Label* password_label, bool auth_result) {
-            if (!auth_result) {
-              return;
-            }
-            password_label->SetObscured(!password_label->GetObscured());
-            toggle_button->SetToggled(!toggle_button->GetToggled());
-          },
-          eye_icon, password_label);
-
-  eye_icon->SetCallback(base::BindRepeating(
-      [](base::WeakPtr<SuccessfulPasswordChangeBubbleController> controller,
-         views::Label* password_label,
-         base::RepeatingCallback<void(bool)> auth_callback) {
-        if (!password_label->GetObscured()) {
-          // Run callback to hide the password. No auth needed to do it.
-          auth_callback.Run(true);
-          return;
-        }
-        if (controller) {
-          controller->AuthenticateUser(auth_callback);
-        }
-      },
-      controller->GetWeakPtr(), password_label,
-      std::move(auth_result_callback)));
-  eye_icon->SetID(SuccessfulPasswordChangeView::kEyeIconButtonId);
-
-  return parent_view;
-}
-
 std::unique_ptr<views::View> CreateManagePasswordsView(
     base::RepeatingClosure open_password_manager_closure) {
   auto manage_passwords_button = std::make_unique<RichHoverButton>(
@@ -217,15 +121,14 @@
   // width.
   set_margins(gfx::Insets());
 
-  views::View* username_password_row = root_view->AddChildView(
-      CreateUsernamePasswordWithEyeIcon(controller_.get()));
+  root_view->AddChildView(CreateUsernamePasswordWithEyeIcon());
   root_view->AddChildView(std::make_unique<views::Separator>());
   root_view->AddChildView(CreateManagePasswordsView(base::BindRepeating(
       &SuccessfulPasswordChangeBubbleController::OpenPasswordManager,
       controller_->GetWeakPtr())));
 
   SetShowIcon(true);
-  SetInitiallyFocusedView(username_password_row->GetViewByID(kEyeIconButtonId));
+  SetInitiallyFocusedView(eye_icon_);
   SetButtons(static_cast<int>(ui::mojom::DialogButton::kNone));
   SetCloseCallback(base::BindRepeating(
       [](SuccessfulPasswordChangeView* view) {
@@ -259,5 +162,85 @@
   SetBubbleHeaderLottie(IDR_PASSWORD_CHANGE_SUCCESS_LOTTIE);
 }
 
+std::unique_ptr<views::View>
+SuccessfulPasswordChangeView::CreateUsernamePasswordWithEyeIcon() {
+  auto parent_view = std::make_unique<views::BoxLayoutView>();
+  parent_view->SetInsideBorderInsets(ComputeRowMargins());
+
+  // Set the same spacing as for RichHoverButton which is displayed below.
+  const int icon_label_spacing = ChromeLayoutProvider::Get()->GetDistanceMetric(
+      DISTANCE_RICH_HOVER_BUTTON_ICON_HORIZONTAL);
+  parent_view->SetBetweenChildSpacing(icon_label_spacing);
+
+  // Add favicon.
+  views::ImageView* favicon_view =
+      parent_view->AddChildView(std::make_unique<views::ImageView>());
+  const int icon_size = GetLayoutConstant(LayoutConstant::kPageInfoIconSize);
+  favicon_view->SetImageSize({icon_size, icon_size});
+  favicon_view->SetImage(ui::ImageModel::FromVectorIcon(
+      features::IsRoundedIconsEnabled() ? vector_icons::kGlobeIcon
+                                        : vector_icons::kGlobeOldIcon,
+      ui::kColorIcon, gfx::kFaviconSize));
+  controller_->RequestFavicon(base::BindOnce(
+      [](views::ImageView* favicon_view, const gfx::Image& favicon) {
+        if (!favicon.IsEmpty()) {
+          favicon_view->SetImage(ui::ImageModel::FromImage(favicon));
+        }
+      },
+      favicon_view));
+
+  // Add username/password labels.
+  auto* username_password_view =
+      parent_view->AddChildView(CreateVerticalStackView());
+  username_password_view->SetProperty(views::kBoxLayoutFlexKey,
+                                      views::BoxLayoutFlexSpecification());
+  username_password_view->AddChildView(
+      CreateUsernameLabel(controller_->GetUsername()));
+  password_label_ = username_password_view->AddChildView(
+      CreatePasswordLabel(controller_->GetNewPassword()));
+
+  // Add eye icon which allows to reveal a password.
+  eye_icon_ = parent_view->AddChildView(views::CreateVectorToggleImageButton(
+      base::BindRepeating(&SuccessfulPasswordChangeView::OnEyeIconClicked,
+                          base::Unretained(this))));
+  eye_icon_->SetTooltipText(
+      l10n_util::GetStringUTF16(IDS_MANAGE_PASSWORDS_SHOW_PASSWORD));
+  eye_icon_->SetToggledTooltipText(
+      l10n_util::GetStringUTF16(IDS_MANAGE_PASSWORDS_HIDE_PASSWORD));
+  eye_icon_->SetImageVerticalAlignment(views::ImageButton::ALIGN_MIDDLE);

diff --git a/chrome/browser/ui/views/passwords/password_change/successful_password_change_view_unittest.cc b/chrome/browser/ui/views/passwords/password_change/successful_password_change_view_unittest.cc
index 7e29468..d6d78ae 100644
--- a/chrome/browser/ui/views/passwords/password_change/successful_password_change_view_unittest.cc
+++ b/chrome/browser/ui/views/passwords/password_change/successful_password_change_view_unittest.cc
@@ -40,9 +40,11 @@
   }
 
   void TearDown() override {
-    view_->GetWidget()->CloseWithReason(
-        views::Widget::ClosedReason::kUnspecified);
-    view_ = nullptr;
+    if (view_) {
+      view_->GetWidget()->CloseWithReason(
+          views::Widget::ClosedReason::kUnspecified);
+      view_ = nullptr;
+    }
     PasswordBubbleViewTestBase::TearDown();
   }
 
@@ -55,6 +57,7 @@
   }
 
   SuccessfulPasswordChangeView* view() { return view_; }
+  void reset_view() { view_ = nullptr; }
 
   views::Label* GetLabelById(int id) {
     return static_cast<views::Label*>(view()->GetViewByID(id));
@@ -128,3 +131,30 @@
   EXPECT_TRUE(GetLabelById(SuccessfulPasswordChangeView::kPasswordLabelId)
                   ->GetObscured());
 }
+
+TEST_F(SuccessfulPasswordChangeViewTest, AuthCallbackAfterViewDestruction) {
+  CreateAndShowView();
+
+  base::OnceCallback<void(bool)> captured_auth_callback;
+  EXPECT_CALL(*model_delegate_mock(), AuthenticateUserWithMessage)
+      .WillOnce(
+          testing::WithArg<1>([&](base::OnceCallback<void(bool)> callback) {
+            captured_auth_callback = std::move(callback);
+          }));
+
+  views::Button* eye_icon = static_cast<views::Button*>(
+      view()->GetViewByID(SuccessfulPasswordChangeView::kEyeIconButtonId));
+  EXPECT_TRUE(eye_icon);
+
+  views::test::ButtonTestApi(eye_icon).NotifyClick(ui::test::TestEvent());
+  EXPECT_TRUE(captured_auth_callback);
+
+  // Destroy the widget and view before the async auth callback runs.
+  views::Widget* widget = view()->GetWidget();
+  reset_view();
+  widget->CloseNow();
+
+  // Executing the callback when the view is destroyed must not crash or trigger
+  // UAF.
+  std::move(captured_auth_callback).Run(true);
+}