[Backport] CVE-2020-6459

Browser context owned callback.

Before this patch, an unowned function pointer would be invoked
asynchronously with a reference to the possibly freed reference to the
browser context, which could cause use after free in certain
circumstances.

This patch makes the browser context own the callback and binds the
function with a weak pointer, so freeing the browser context invalidates
the weak pointer, which cancels the callback execution.

After this patch, freeing the browser context aborts the asynchronous
callback that dereferences the browser context, so the use after free
is prevented.

Bug: 1065298
Change-Id: Id6de3099a55c4505e94a8a6d21fb25d6d2b34c6c
Reviewed-by: Allan Sandfeld Jensen <allan.jensen@qt.io>

1 files changed, 61 insertions, 21 deletions

diff --git a/chromium/content/browser/payments/payment_app_provider_impl.cc b/chromium/content/browser/payments/payment_app_provider_impl.cc
index 3b813cb22c1..23b5ffb4156 100644
--- a/chromium/content/browser/payments/payment_app_provider_impl.cc
+++ b/chromium/content/browser/payments/payment_app_provider_impl.cc
@@ -17,6 +17,7 @@
 #include "base/strings/string_util.h"
 #include "base/task/post_task.h"
 #include "base/token.h"
+#include "base/supports_user_data.h"
 #include "content/browser/payments/payment_app_context_impl.h"
 #include "content/browser/payments/payment_app_installer.h"
 #include "content/browser/service_worker/service_worker_context_wrapper.h"
@@ -432,28 +433,65 @@ void OnInstallPaymentApp(
   }
 }
 
-void CheckPermissionForPaymentApps(
-    BrowserContext* browser_context,
-    PaymentAppProvider::GetAllPaymentAppsCallback callback,
-    PaymentAppProvider::PaymentApps apps) {
-  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+// Callbacks for checking permissions asynchronously. Owned by the browser
+// context to avoid using the browser context after it has been freed. Deleted
+// after the callback is invoked.
+// Sample usage:
+//   PostTask(&PermissionChecker::CheckPermissionForPaymentApps,
+//       PermissionChecker::Create(browser_context), std::move(callback));
+class PermissionChecker : public base::SupportsUserData::Data {
+ public:
+  static base::WeakPtr<PermissionChecker> Create(
+      BrowserContext* browser_context) {
+    auto owned = std::make_unique<PermissionChecker>(browser_context);
+    auto weak_pointer_result = owned->weak_ptr_factory_.GetWeakPtr();
+    void* key = owned.get();
+    browser_context->SetUserData(key, std::move(owned));
+    return weak_pointer_result;
+  }
+
+  // Do not use this method directly! Use the static PermissionChecker::Create()
+  // method instead. (The constructor must be public for std::make_unique<> in
+  // the Create() method.)
+  explicit PermissionChecker(BrowserContext* browser_context)
+      : browser_context_(browser_context) {}
+  ~PermissionChecker() override = default;
+
+  // Disallow copy and assign.
+  PermissionChecker(const PermissionChecker& other) = delete;
+  PermissionChecker& operator=(const PermissionChecker& other) = delete;
+
+  void CheckPermissionForPaymentApps(
+      PaymentAppProvider::GetAllPaymentAppsCallback callback,
+      PaymentAppProvider::PaymentApps apps) {
+    DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
-  PermissionController* permission_controller =
-      BrowserContext::GetPermissionController(browser_context);
-  DCHECK(permission_controller);
-
-  PaymentAppProvider::PaymentApps permitted_apps;
-  for (auto& app : apps) {
-    GURL origin = app.second->scope.GetOrigin();
-    if (permission_controller->GetPermissionStatus(
-            PermissionType::PAYMENT_HANDLER, origin, origin) ==
-        blink::mojom::PermissionStatus::GRANTED) {
-      permitted_apps[app.first] = std::move(app.second);
+    PermissionController* permission_controller =
+        BrowserContext::GetPermissionController(browser_context_);
+    DCHECK(permission_controller);
+
+    PaymentAppProvider::PaymentApps permitted_apps;
+    for (auto& app : apps) {
+      GURL origin = app.second->scope.GetOrigin();
+      if (permission_controller->GetPermissionStatus(
+              PermissionType::PAYMENT_HANDLER, origin, origin) ==
+          blink::mojom::PermissionStatus::GRANTED) {
+        permitted_apps[app.first] = std::move(app.second);
+      }
     }
+
+    std::move(callback).Run(std::move(permitted_apps));
+
+    // Deletes this PermissionChecker object.
+    browser_context_->RemoveUserData(/*key=*/this);
   }
 
-  std::move(callback).Run(std::move(permitted_apps));
-}
+ private:
+  // Owns this PermissionChecker object, so it's always valid.
+  BrowserContext* browser_context_;
+
+  base::WeakPtrFactory<PermissionChecker> weak_ptr_factory_{this};
+};
 
 void AbortInvokePaymentApp(BrowserContext* browser_context,
                            PaymentEventResponseType reason) {
@@ -606,9 +644,11 @@ void PaymentAppProviderImpl::GetAllPaymentApps(
 
   RunOrPostTaskOnThread(
       FROM_HERE, ServiceWorkerContext::GetCoreThreadId(),
-      base::BindOnce(&GetAllPaymentAppsOnCoreThread, payment_app_context,
-                     base::BindOnce(&CheckPermissionForPaymentApps,
-                                    browser_context, std::move(callback))));
+      base::BindOnce(
+          &GetAllPaymentAppsOnCoreThread, payment_app_context,
+          base::BindOnce(&PermissionChecker::CheckPermissionForPaymentApps,
+                         PermissionChecker::Create(browser_context),
+                         std::move(callback))));
 }
 
 void PaymentAppProviderImpl::InvokePaymentApp(