diff --git a/chrome/browser/ui/views/digital_credentials/digital_identity_multi_step_dialog_browsertest.cc b/chrome/browser/ui/views/digital_credentials/digital_identity_multi_step_dialog_browsertest.cc index 30059c9f1b939..0b3ff653da9aa 100644 --- a/chrome/browser/ui/views/digital_credentials/digital_identity_multi_step_dialog_browsertest.cc +++ b/chrome/browser/ui/views/digital_credentials/digital_identity_multi_step_dialog_browsertest.cc @@ -6,20 +6,26 @@ #include "base/scoped_observation.h" #include "base/test/scoped_feature_list.h" +#include "chrome/browser/digital_credentials/digital_identity_provider_desktop.h" #include "chrome/browser/ui/browser.h" #include "chrome/browser/ui/tabs/tab_strip_model.h" +#include "chrome/grit/generated_resources.h" #include "chrome/test/base/in_process_browser_test.h" +#include "content/public/browser/cross_device_request_info.h" #include "content/public/browser/web_contents.h" #include "content/public/common/content_features.h" #include "content/public/test/browser_test.h" #include "testing/gtest/include/gtest/gtest-spi.h" +#include "ui/base/l10n/l10n_util.h" #include "ui/base/mojom/dialog_button.mojom.h" #include "ui/events/test/test_event.h" #include "ui/views/bubble/bubble_dialog_delegate_view.h" #include "ui/views/controls/label.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" +#include "ui/views/widget/widget_delegate.h" #include "ui/views/widget/widget_observer.h" namespace { @@ -233,3 +239,123 @@ IN_PROC_BROWSER_TEST_F(DigitalIdentityMultiStepDialogBrowserTest, ui::mojom::DialogButton::kOk)); } } + +// ----------------------------------------------------------------------------- +// Regression test for a heap-use-after-free in +// DigitalIdentityProviderDesktop::EndRequestWithError. +// +// EndRequestWithError() does: +// dialog_.reset(); // closes the Views widget +// std::move(callback_).Run(...); // dereferences `this->callback_` +// +// `dialog_.reset()` synchronously runs ~DigitalIdentityMultiStepDialog which +// calls Widget::CloseWithReason(). The developers acknowledge in +// content/browser/digital_credentials/digital_identity_request_impl.cc that +// this CloseWithReason path "(via activation observers) may destroy the hosting +// WebContents and therefore `this`". When the hosting WebContents dies, +// ~DigitalIdentityRequestImpl runs and its `provider_` unique_ptr deletes the +// DigitalIdentityProviderDesktop *while it is still on the stack in +// EndRequestWithError*. The subsequent read of `callback_` is then a UAF. +// +// This test models the activation-observer chain with a WidgetObserver that +// resets the owning unique_ptr from inside OnWidgetClosing - exactly what the +// production chain (extension popup -> WebContents teardown -> +// ~DigitalIdentityRequestImpl -> provider_.reset()) does synchronously. + +namespace { + +class UafTestDigitalIdentityProvider : public DigitalIdentityProviderDesktop { + public: + UafTestDigitalIdentityProvider() = default; + ~UafTestDigitalIdentityProvider() override = default; + + void SetUpAndShowQrDialog(content::WebContents* web_contents) { + set_web_contents_for_testing(web_contents->GetWeakPtr()); + set_rp_origin_for_testing(url::Origin::Create(GURL("https://rp.example"))); + set_callback_for_testing(base::BindOnce( + [](base::expected) {})); + ShowQrCodeDialog("FIDO:/0123456789", + content::digital_credentials::cross_device::RequestInfo:: + RequestType::kGet); + } +}; + +// Stands in for the production activation-observer chain: when the dialog +// widget closes, the transient host (e.g. extension popup) tears down its +// WebContents, whose RenderFrameHost destruction runs +// ~DigitalIdentityRequestImpl, which owns the provider via std::unique_ptr. +class ProviderDestroyingWidgetObserver : public views::WidgetObserver { + public: + explicit ProviderDestroyingWidgetObserver( + std::unique_ptr* owner) + : owner_(owner) {} + + void OnWidgetClosing(views::Widget* widget) override { + widget->RemoveObserver(this); + owner_->reset(); + } + + private: + raw_ptr> owner_; +}; + +} // namespace + +IN_PROC_BROWSER_TEST_F(DigitalIdentityMultiStepDialogBrowserTest, + EndRequestWithErrorOwnerDestroyedDuringDialogClose) { + // The provider is owned via unique_ptr exactly as in + // DigitalIdentityRequestImpl::provider_. + std::unique_ptr provider = + std::make_unique(); + + // Capture the QR dialog widget when it is created. + views::Widget* dialog_widget = nullptr; + std::u16string qr_title = + l10n_util::GetStringUTF16(IDS_WEB_DIGITAL_CREDENTIALS_QR_TITLE); + views::AnyWidgetObserver any_observer{views::test::AnyWidgetTestPasskey()}; + any_observer.set_initialized_callback(base::BindRepeating( + [](views::Widget** out, const std::u16string& title, + views::Widget* widget) { + if (widget->widget_delegate() && + widget->widget_delegate()->GetWindowTitle() == title) { + *out = widget; + } + }, + &dialog_widget, qr_title)); + + static_cast(provider.get()) + ->SetUpAndShowQrDialog(GetActiveWebContents()); + ASSERT_NE(dialog_widget, nullptr); + base::WeakPtr weak_dialog_widget = + dialog_widget->GetWeakPtr(); + + // When EndRequestWithError() resets `dialog_` and the widget fires + // OnWidgetClosing, this observer deletes the provider - mimicking the + // activation-observer -> WebContents teardown -> ~DigitalIdentityRequestImpl + // -> provider_.reset() chain. + ProviderDestroyingWidgetObserver destroyer(&provider); + dialog_widget->AddObserver(&destroyer); + + // Click "Cancel" on the QR dialog. OnDialogCanceled() PostTasks the bound + // OnCanceled() callback and returns false so the widget stays open. + static_cast(dialog_widget->widget_delegate()) + ->CancelDialog(); + ASSERT_FALSE(dialog_widget->IsClosed()); + + // Runs the posted OnCanceled() -> EndRequestWithError(): + // dialog_.reset() + // -> ~DigitalIdentityMultiStepDialog + // -> Widget::CloseWithReason() + // -> OnWidgetClosing -> provider.reset() -> delete provider + // std::move(callback_).Run(...) <-- reads freed `this->callback_` + base::RunLoop().RunUntilIdle(); + + // If we get here without ASAN tripping, the bug is fixed. + EXPECT_EQ(provider.get(), nullptr); + if (weak_dialog_widget) { + weak_dialog_widget->RemoveObserver(&destroyer); + views::test::WidgetDestroyedWaiter(weak_dialog_widget.get()).Wait(); + } +}