diff --git a/components/payments/content/secure_payment_confirmation_controller.h b/components/payments/content/secure_payment_confirmation_controller.h index 0dcd267c2f0d9f..5b8d5318db1b85 100644 --- a/components/payments/content/secure_payment_confirmation_controller.h +++ b/components/payments/content/secure_payment_confirmation_controller.h @@ -68,6 +68,9 @@ class SecurePaymentConfirmationController void SetIsErrorDialogForTesting(bool is_error_dialog) { is_error_dialog_ = is_error_dialog; } + void SetViewForTesting(base::WeakPtr view) { + view_ = view; + } base::WeakPtr GetWeakPtr(); diff --git a/components/payments/content/secure_payment_confirmation_controller_unittest.cc b/components/payments/content/secure_payment_confirmation_controller_unittest.cc index 9168d6553e9460..4ffddcc9af43e6 100644 --- a/components/payments/content/secure_payment_confirmation_controller_unittest.cc +++ b/components/payments/content/secure_payment_confirmation_controller_unittest.cc @@ -6,6 +6,8 @@ #include +#include "base/functional/callback.h" +#include "base/test/bind.h" #include "base/test/metrics/histogram_tester.h" #include "components/autofill/core/browser/data_manager/test_personal_data_manager.h" #include "components/autofill/core/browser/test_utils/test_event_waiter.h" @@ -21,6 +23,51 @@ namespace payments { +namespace { + +// A fake SecurePaymentConfirmationView that, when HideDialog() is called, +// runs a closure. This is used to simulate the production scenario in which +// SecurePaymentConfirmationDialogView::HideDialog() -> Widget::Close() +// synchronously triggers activation observers (see crbug.com/521495992) that +// destroy the hosting WebContents, leading to ~PaymentRequest -> +// ChromePaymentRequestDelegate::CloseDialog() -> spc_dialog_.reset() while +// the controller's user-interaction handler is still on the stack. +class FakeReentrantView : public SecurePaymentConfirmationView { + public: + FakeReentrantView() = default; + ~FakeReentrantView() override = default; + + void set_hide_dialog_closure(base::OnceClosure closure) { + hide_dialog_closure_ = std::move(closure); + } + + base::WeakPtr GetWeakPtr() { + return weak_ptr_factory_.GetWeakPtr(); + } + + // SecurePaymentConfirmationView: + void ShowDialog(content::WebContents*, + base::WeakPtr, + VerifyCallback, + AnotherWayCallback, + CancelCallback, + OptOutCallback) override {} + void OnModelUpdated() override {} + void HideDialog() override { + if (hide_dialog_closure_) { + std::move(hide_dialog_closure_).Run(); + } + } + bool ClickOptOutForTesting() override { return false; } + + private: + base::OnceClosure hide_dialog_closure_; + base::WeakPtrFactory weak_ptr_factory_{this}; +}; + +} // namespace + + class SecurePaymentConfirmationControllerTest : public testing::Test, public PaymentRequest::ObserverForTest { @@ -197,5 +244,76 @@ TEST_F(SecurePaymentConfirmationControllerTest, /*expected_bucket_count=*/1); } +// Regression test for the implicit-`this` use-after-free in OnCancel() / +// OnAnotherWay() / OnOptOut() / OnConfirm()-error-branch. +// +// In production the controller is owned by +// ChromePaymentRequestDelegate::spc_dialog_ (a unique_ptr). When the user +// clicks Cancel on the SPC dialog, OnCancel() calls CloseDialog() which calls +// view_->HideDialog() -> GetWidget()->Close(). Per the in-tree comments at +// content/browser/webauth/authenticator_common_impl.cc:3225 and +// chrome/browser/digital_credentials/digital_identity_provider_desktop.cc:402, +// Widget::Close() can synchronously trigger activation observers that destroy +// the hosting WebContents, which destroys the frame-bound PaymentRequest +// DocumentService, whose destructor calls +// ChromePaymentRequestDelegate::CloseDialog() -> spc_dialog_.reset(), +// freeing `this` while OnCancel() is still on the stack. OnCancel() then +// reads this->request_ from freed memory. +// +// This test simulates that exact reentrancy by injecting a fake view whose +// HideDialog() synchronously resets the unique_ptr that owns the controller. +// +// Without a self-liveness guard around CloseDialog() this triggers +// AddressSanitizer: heap-use-after-free at the `if (!request_)` line. +class SecurePaymentConfirmationControllerSelfDeleteTest : public testing::Test { + protected: + // Builds a controller owned by |spc_dialog_| (mirroring + // ChromePaymentRequestDelegate::spc_dialog_) with a fake view whose + // HideDialog() synchronously does spc_dialog_.reset(). + void SetUpController() { + spc_dialog_ = std::make_unique( + base::WeakPtr()); + fake_view_.set_hide_dialog_closure(base::BindLambdaForTesting([this]() { + // Simulates ChromePaymentRequestDelegate::CloseDialog() line 129: + // spc_dialog_.reset(); + // reached synchronously from inside Widget::Close(). + spc_dialog_.reset(); + })); + spc_dialog_->SetViewForTesting(fake_view_.GetWeakPtr()); + spc_dialog_->SetIsDialogShowingForTesting(true); + } + + content::BrowserTaskEnvironment task_environment_; + FakeReentrantView fake_view_; + std::unique_ptr spc_dialog_; +}; + +TEST_F(SecurePaymentConfirmationControllerSelfDeleteTest, OnCancel) { + SetUpController(); + // Hold a raw pointer because |spc_dialog_| becomes null mid-call. + SecurePaymentConfirmationController* controller = spc_dialog_.get(); + // Triggers the UAF: CloseDialog() -> HideDialog() -> spc_dialog_.reset() + // frees |controller|; OnCancel() then reads this->request_. + controller->OnCancel(); +} + +TEST_F(SecurePaymentConfirmationControllerSelfDeleteTest, OnAnotherWay) { + SetUpController(); + SecurePaymentConfirmationController* controller = spc_dialog_.get(); + controller->OnAnotherWay(); +} + +TEST_F(SecurePaymentConfirmationControllerSelfDeleteTest, OnOptOut) { + SetUpController(); + SecurePaymentConfirmationController* controller = spc_dialog_.get(); + controller->OnOptOut(); +} + +TEST_F(SecurePaymentConfirmationControllerSelfDeleteTest, OnConfirmErrorDialog) { + SetUpController(); + spc_dialog_->SetIsErrorDialogForTesting(true); + SecurePaymentConfirmationController* controller = spc_dialog_.get(); + controller->OnConfirm(); +} } // namespace payments diff --git a/components/payments/content/secure_payment_confirmation_view_stub.cc b/components/payments/content/secure_payment_confirmation_view_stub.cc index 5d0a2f7b68b50d..a567cb862aa52b 100644 --- a/components/payments/content/secure_payment_confirmation_view_stub.cc +++ b/components/payments/content/secure_payment_confirmation_view_stub.cc @@ -6,6 +6,9 @@ namespace payments { +SecurePaymentConfirmationView::SecurePaymentConfirmationView() = default; +SecurePaymentConfirmationView::~SecurePaymentConfirmationView() = default; + // static base::WeakPtr SecurePaymentConfirmationView::Create(