Skip to content

Commit

Permalink
[WebPayments] Unify early exits in AndroidPaymentAppFactory
Browse files Browse the repository at this point in the history 
(M110 merge)

This CL corrects an issue where AppFinder::OnGetAppDescriptions had a
different early-exit check than AppFinder::IsReadyToPay. This could
cause OnGetAppDescriptions to reach AppFinder::OnDoneCreatingPaymentApps
and delete the current instance whilst it was still in use.

A test is added that exercises the failing path before the fix.

(cherry picked from commit f06f8ce)

Bug: 1414738
Change-Id: I31dd8a62a83acf60412df1d055aeba311f2df430
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4245784
Reviewed-by: Nick Burris <nburris@chromium.org>
Commit-Queue: Stephen McGruer <smcgruer@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1104754}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4262809
Auto-Submit: Stephen McGruer <smcgruer@chromium.org>
Cr-Commit-Position: refs/branch-heads/5481@{#1165}
Cr-Branched-From: 130f3e4-refs/heads/main@{#1084008}
  • Loading branch information
@stephenmcgruer
stephenmcgruer authored and Chromium LUCI CQ committed Feb 17, 2023
1 parent 3ab02c6 commit a88bdd6
Show file tree
Hide file tree
Showing 5 changed files with 197 additions and 34 deletions.
2 changes: 2 additions & 0 deletions components/payments/content/BUILD.gn
View file
Expand Up @@ -210,6 +210,8 @@ source_set("unit_tests") {
"android_app_communication_unittest.cc",
"android_payment_app_factory_unittest.cc",
"android_payment_app_unittest.cc",
"mock_android_app_communication.cc",
"mock_android_app_communication.h",
"mock_payment_app_factory_delegate.cc",
"mock_payment_app_factory_delegate.h",
"payment_event_response_util_unittest.cc",
Expand Down
13 changes: 10 additions & 3 deletions components/payments/content/android_payment_app_factory.cc
View file
Expand Up @@ -84,13 +84,21 @@ class AppFinder : public base::SupportsUserData::Data {
}

private:
// Check that our required dependencies are still valid, i.e. that the page
// isn't currently being torn down.
bool PageIsValid() {
return communication_ && delegate_ && delegate_->GetSpec() &&
delegate_->GetInitiatorRenderFrameHost();
}

void OnGetAppDescriptions(
const absl::optional<std::string>& error_message,
std::vector<std::unique_ptr<AndroidAppDescription>> app_descriptions) {
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
// The browser could be shutting down.
if (!communication_ || !delegate_ || !delegate_->GetSpec())
if (!PageIsValid()) {
return;
}

if (error_message.has_value()) {
delegate_->OnPaymentAppCreationError(error_message.value());
Expand Down Expand Up @@ -180,8 +188,7 @@ class AppFinder : public base::SupportsUserData::Data {
DCHECK_LT(0U, number_of_pending_is_ready_to_pay_queries_);

// The browser could be shutting down.
if (!communication_ || !delegate_ || !delegate_->GetSpec() ||
!delegate_->GetInitiatorRenderFrameHost()) {
if (!PageIsValid()) {
OnDoneCreatingPaymentApps();
return;
}
Expand Down
123 changes: 92 additions & 31 deletions components/payments/content/android_payment_app_factory_unittest.cc
View file
Expand Up @@ -11,13 +11,16 @@
#include "base/memory/weak_ptr.h"
#include "components/payments/content/android_app_communication.h"
#include "components/payments/content/android_app_communication_test_support.h"
#include "components/payments/content/mock_android_app_communication.h"
#include "components/payments/content/mock_payment_app_factory_delegate.h"
#include "components/payments/content/payment_app_factory.h"
#include "components/payments/content/payment_manifest_web_data_service.h"
#include "components/payments/content/payment_request_spec.h"
#include "components/payments/core/android_app_description.h"
#include "components/webauthn/core/browser/internal_authenticator.h"
#include "content/public/browser/web_contents.h"
#include "content/public/test/browser_task_environment.h"
#include "content/public/test/test_browser_context.h"
#include "content/public/test/test_web_contents_factory.h"
#include "testing/gmock/include/gmock/gmock.h"
#include "testing/gtest/include/gtest/gtest.h"
Expand All @@ -35,6 +38,73 @@ namespace {
class AndroidPaymentAppFactoryTest : public testing::Test {
public:
AndroidPaymentAppFactoryTest()
: mock_communication_(
std::make_unique<MockAndroidAppCommunication>(&context_)),
factory_(mock_communication_->GetWeakPtr()) {
auto method_data = mojom::PaymentMethodData::New();
method_data->supported_method = "https://play.google.com/billing";
method_data->stringified_data = "{}";
delegate_ = std::make_unique<MockPaymentAppFactoryDelegate>(
web_contents_factory_.CreateWebContents(&context_),
std::move(method_data));
}

protected:
content::BrowserTaskEnvironment task_environment_;
content::TestBrowserContext context_;
content::TestWebContentsFactory web_contents_factory_;
std::unique_ptr<MockPaymentAppFactoryDelegate> delegate_;
std::unique_ptr<MockAndroidAppCommunication> mock_communication_;
AndroidPaymentAppFactory factory_;
};

// This is a regression test for crbug.com/1414738. A mismatch in early-return
// checks could result in calling IsReadyToPay with a null RenderFrameHost in a
// loop - however the first call would end up deleting |this| and cause a UAF.
TEST_F(AndroidPaymentAppFactoryTest, NullRenderFrameHost) {
EXPECT_CALL(*delegate_, GetTwaPackageName)
.WillRepeatedly(testing::Return("com.example.app"));

// In order to reach the problematic code, we need a null RenderFrameHost and
// also to be in off the record mode.
EXPECT_CALL(*delegate_, GetInitiatorRenderFrameHost)
.WillRepeatedly(testing::Return(nullptr));
delegate_->set_is_off_the_record();

// Return an app with multiple activities. This causes multiple iterations
// when looping over `single_activity_apps` - the UAF would trigger on the
// second iteration.
EXPECT_CALL(*mock_communication_, GetAppDescriptions)
.WillRepeatedly(
[](const std::string& twa_package_name,
MockAndroidAppCommunication::GetAppDescriptionsCallback callback) {
std::vector<std::unique_ptr<AndroidAppDescription>> apps;
apps.emplace_back(std::make_unique<AndroidAppDescription>());
apps.back()->package = "com.example.app";
apps.back()->service_names.push_back("com.example.app.Service");
apps.back()->activities.emplace_back(
std::make_unique<AndroidActivityDescription>());
apps.back()->activities.back()->name = "com.example.app.Activity";
apps.back()->activities.back()->default_payment_method =
"https://play.google.com/billing";
apps.back()->activities.emplace_back(
std::make_unique<AndroidActivityDescription>());
apps.back()->activities.back()->name = "com.example.app.Activity2";
apps.back()->activities.back()->default_payment_method =
"https://play.google.com/billing";
std::move(callback).Run(absl::nullopt, std::move(apps));
});

// Now trigger app finding. This should near immediately bail in
// AppFinder::OnGetAppDescriptions and not make it to IsReadyToPay.
factory_.Create(delegate_->GetWeakPtr());
}

// This test class uses a deeper integration into the underlying app support,
// and as such some tests may only run on certain platforms (e.g., ChromeOS).
class AndroidPaymentAppFactoryIntegrationTest : public testing::Test {
public:
AndroidPaymentAppFactoryIntegrationTest()
: support_(AndroidAppCommunicationTestSupport::Create()),
factory_(GetCommunication(support_->context())) {
auto method_data = mojom::PaymentMethodData::New();
Expand All @@ -43,6 +113,10 @@ class AndroidPaymentAppFactoryTest : public testing::Test {
delegate_ = std::make_unique<MockPaymentAppFactoryDelegate>(
web_contents_factory_.CreateWebContents(support_->context()),
std::move(method_data));

EXPECT_CALL(*delegate_, GetInitiatorRenderFrameHost())
.WillRepeatedly(testing::Return(
delegate_->GetWebContents()->GetPrimaryMainFrame()));
}

std::unique_ptr<AndroidAppCommunicationTestSupport> support_;
Expand All @@ -64,7 +138,7 @@ class AndroidPaymentAppFactoryTest : public testing::Test {
// The payment app factory should return an error if it's unable to invoke
// Aneroid payment apps on a platform that supports such apps, e.g, when ARC is
// disabled on Chrome OS.
TEST_F(AndroidPaymentAppFactoryTest, FactoryReturnsErrorWithoutArc) {
TEST_F(AndroidPaymentAppFactoryIntegrationTest, FactoryReturnsErrorWithoutArc) {
EXPECT_CALL(*delegate_, GetTwaPackageName())
.WillRepeatedly(testing::Return("com.example.app"));
EXPECT_CALL(*delegate_, OnDoneCreatingPaymentApps());
Expand All @@ -82,7 +156,7 @@ TEST_F(AndroidPaymentAppFactoryTest, FactoryReturnsErrorWithoutArc) {

// The payment app factory should not return any errors when there're no Android
// payment apps available.
TEST_F(AndroidPaymentAppFactoryTest, NoErrorsWhenNoApps) {
TEST_F(AndroidPaymentAppFactoryIntegrationTest, NoErrorsWhenNoApps) {
// Enable invoking Android payment apps on those platforms that support it.
auto scoped_initialization_ = support_->CreateScopedInitialization();

Expand All @@ -107,15 +181,13 @@ MATCHER_P3(PaymentAppMatches, type, package, method, "") {

// The payment app factory should return the TWA payment app when running in TWA
// mode, even when it does not have an IS_READY_TO_PAY service.
TEST_F(AndroidPaymentAppFactoryTest, FindAppsThatDoNotHaveReadyToPayService) {
TEST_F(AndroidPaymentAppFactoryIntegrationTest,
FindAppsThatDoNotHaveReadyToPayService) {
// Enable invoking Android payment apps on those platforms that support it.
auto scoped_initialization_ = support_->CreateScopedInitialization();

EXPECT_CALL(*delegate_, GetTwaPackageName())
.WillRepeatedly(testing::Return("com.example.app"));
EXPECT_CALL(*delegate_, GetInitiatorRenderFrameHost())
.WillRepeatedly(
testing::Return(delegate_->GetWebContents()->GetPrimaryMainFrame()));
EXPECT_CALL(*delegate_, OnDoneCreatingPaymentApps());

EXPECT_CALL(*delegate_, OnPaymentAppCreationError(testing::_, testing::_))
Expand Down Expand Up @@ -146,7 +218,7 @@ TEST_F(AndroidPaymentAppFactoryTest, FindAppsThatDoNotHaveReadyToPayService) {

// The payment app factory should return one payment app and should not query
// the IS_READY_TO_PAY service, because of being off the record.
TEST_F(AndroidPaymentAppFactoryTest,
TEST_F(AndroidPaymentAppFactoryIntegrationTest,
DoNotQueryReadyToPaySericeWhenOffTheRecord) {
// Enable invoking Android payment apps on those platforms that support it.
auto scoped_initialization_ = support_->CreateScopedInitialization();
Expand All @@ -156,9 +228,6 @@ TEST_F(AndroidPaymentAppFactoryTest,

EXPECT_CALL(*delegate_, GetTwaPackageName())
.WillRepeatedly(testing::Return("com.example.app"));
EXPECT_CALL(*delegate_, GetInitiatorRenderFrameHost())
.WillRepeatedly(
testing::Return(delegate_->GetWebContents()->GetPrimaryMainFrame()));
EXPECT_CALL(*delegate_, OnDoneCreatingPaymentApps());

EXPECT_CALL(*delegate_, OnPaymentAppCreationError(testing::_, testing::_))
Expand Down Expand Up @@ -189,16 +258,13 @@ TEST_F(AndroidPaymentAppFactoryTest,

// The payment app factory should return the TWA payment app that returns true
// from IS_READY_TO_PAY service when running in TWA mode.
TEST_F(AndroidPaymentAppFactoryTest,
TEST_F(AndroidPaymentAppFactoryIntegrationTest,
FindTheTwaPaymentAppThatIsReadyToPayInTwaMode) {
// Enable invoking Android payment apps on those platforms that support it.
auto scoped_initialization_ = support_->CreateScopedInitialization();

EXPECT_CALL(*delegate_, GetTwaPackageName())
.WillRepeatedly(testing::Return("com.twa.app"));
EXPECT_CALL(*delegate_, GetInitiatorRenderFrameHost())
.WillRepeatedly(
testing::Return(delegate_->GetWebContents()->GetPrimaryMainFrame()));
EXPECT_CALL(*delegate_, OnDoneCreatingPaymentApps());

EXPECT_CALL(*delegate_, OnPaymentAppCreationError(testing::_, testing::_))
Expand Down Expand Up @@ -227,15 +293,13 @@ TEST_F(AndroidPaymentAppFactoryTest,

// The payment app factory should return no payment apps when IS_READY_TO_PAY
// service returns false.
TEST_F(AndroidPaymentAppFactoryTest, IgnoreAppsThatAreNotReadyToPay) {
TEST_F(AndroidPaymentAppFactoryIntegrationTest,
IgnoreAppsThatAreNotReadyToPay) {
// Enable invoking Android payment apps on those platforms that support it.
auto scoped_initialization_ = support_->CreateScopedInitialization();

EXPECT_CALL(*delegate_, GetTwaPackageName())
.WillRepeatedly(testing::Return("com.example.app"));
EXPECT_CALL(*delegate_, GetInitiatorRenderFrameHost())
.WillRepeatedly(
testing::Return(delegate_->GetWebContents()->GetPrimaryMainFrame()));
EXPECT_CALL(*delegate_, OnDoneCreatingPaymentApps());
EXPECT_CALL(*delegate_, OnPaymentAppCreationError(testing::_, testing::_))
.Times(0);
Expand All @@ -258,15 +322,12 @@ TEST_F(AndroidPaymentAppFactoryTest, IgnoreAppsThatAreNotReadyToPay) {

// The payment app factory should return the correct TWA payment app out of two
// installed payment apps, when running in TWA mode.
TEST_F(AndroidPaymentAppFactoryTest, FindTheCorrectTwaAppInTwaMode) {
TEST_F(AndroidPaymentAppFactoryIntegrationTest, FindTheCorrectTwaAppInTwaMode) {
// Enable invoking Android payment apps on those platforms that support it.
auto scoped_initialization_ = support_->CreateScopedInitialization();

EXPECT_CALL(*delegate_, GetTwaPackageName())
.WillRepeatedly(testing::Return("com.correct-twa.app"));
EXPECT_CALL(*delegate_, GetInitiatorRenderFrameHost())
.WillRepeatedly(
testing::Return(delegate_->GetWebContents()->GetPrimaryMainFrame()));
EXPECT_CALL(*delegate_, OnDoneCreatingPaymentApps());

EXPECT_CALL(*delegate_, OnPaymentAppCreationError(testing::_, testing::_))
Expand Down Expand Up @@ -310,7 +371,7 @@ TEST_F(AndroidPaymentAppFactoryTest, FindTheCorrectTwaAppInTwaMode) {

// The payment app factory does not return non-TWA payment apps when running in
// TWA mode.
TEST_F(AndroidPaymentAppFactoryTest, IgnoreNonTwaAppsInTwaMode) {
TEST_F(AndroidPaymentAppFactoryIntegrationTest, IgnoreNonTwaAppsInTwaMode) {
// Enable invoking Android payment apps on those platforms that support it.
auto scoped_initialization_ = support_->CreateScopedInitialization();

Expand Down Expand Up @@ -338,7 +399,8 @@ TEST_F(AndroidPaymentAppFactoryTest, IgnoreNonTwaAppsInTwaMode) {

// The payment app factory does not return any payment apps when not running
// inside of TWA.
TEST_F(AndroidPaymentAppFactoryTest, DoNotLookForAppsWhenOutsideOfTwaMode) {
TEST_F(AndroidPaymentAppFactoryIntegrationTest,
DoNotLookForAppsWhenOutsideOfTwaMode) {
// Enable invoking Android payment apps on those platforms that support it.
auto scoped_initialization_ = support_->CreateScopedInitialization();

Expand All @@ -354,7 +416,8 @@ TEST_F(AndroidPaymentAppFactoryTest, DoNotLookForAppsWhenOutsideOfTwaMode) {
}

// The Android payment app factory works only with TWA specific payment methods.
TEST_F(AndroidPaymentAppFactoryTest, DoNotLookForAppsForNonTwaMethod) {
TEST_F(AndroidPaymentAppFactoryIntegrationTest,
DoNotLookForAppsForNonTwaMethod) {
// Enable invoking Android payment apps on those platforms that support it.
auto scoped_initialization_ = support_->CreateScopedInitialization();

Expand All @@ -378,7 +441,7 @@ TEST_F(AndroidPaymentAppFactoryTest, DoNotLookForAppsForNonTwaMethod) {

// If the TWA supports a non-TWA-specific payment method, then it should be
// ignored.
TEST_F(AndroidPaymentAppFactoryTest, IgnoreNonTwaMethodInTheTwa) {
TEST_F(AndroidPaymentAppFactoryIntegrationTest, IgnoreNonTwaMethodInTheTwa) {
// Enable invoking Android payment apps on those platforms that support it.
auto scoped_initialization_ = support_->CreateScopedInitialization();

Expand Down Expand Up @@ -407,16 +470,13 @@ TEST_F(AndroidPaymentAppFactoryTest, IgnoreNonTwaMethodInTheTwa) {
// If the TWA supports both a TWA-specific and a non-TWA-specific payment
// method, then only the TWA-specific payment method activity should be
// returned.
TEST_F(AndroidPaymentAppFactoryTest,
TEST_F(AndroidPaymentAppFactoryIntegrationTest,
FindOnlyActivitiesWithTwaSpecificMethodName) {
// Enable invoking Android payment apps on those platforms that support it.
auto scoped_initialization_ = support_->CreateScopedInitialization();

EXPECT_CALL(*delegate_, GetTwaPackageName())
.WillRepeatedly(testing::Return("com.twa.app"));
EXPECT_CALL(*delegate_, GetInitiatorRenderFrameHost())
.WillRepeatedly(
testing::Return(delegate_->GetWebContents()->GetPrimaryMainFrame()));
EXPECT_CALL(*delegate_, OnDoneCreatingPaymentApps());
EXPECT_CALL(*delegate_, OnPaymentAppCreationError(testing::_, testing::_))
.Times(0);
Expand Down Expand Up @@ -454,7 +514,8 @@ TEST_F(AndroidPaymentAppFactoryTest,
}

// At most one IS_READY_TO_PAY service is allowed in an Android payment app.
TEST_F(AndroidPaymentAppFactoryTest, ReturnErrorWhenMoreThanOneServiceInApp) {
TEST_F(AndroidPaymentAppFactoryIntegrationTest,
ReturnErrorWhenMoreThanOneServiceInApp) {
// Enable invoking Android payment apps on those platforms that support it.
auto scoped_initialization_ = support_->CreateScopedInitialization();

Expand Down
16 changes: 16 additions & 0 deletions components/payments/content/mock_android_app_communication.cc
View file
@@ -0,0 +1,16 @@
// Copyright 2023 The Chromium Authors
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#include "components/payments/content/mock_android_app_communication.h"
#include "content/public/browser/browser_context.h"

namespace payments {

MockAndroidAppCommunication::MockAndroidAppCommunication(
content::BrowserContext* context)
: AndroidAppCommunication(context) {}

MockAndroidAppCommunication::~MockAndroidAppCommunication() = default;

} // namespace payments

0 comments on commit a88bdd6

Please sign in to comment.