From 31b757cf79d9c69f9b5451c8892b0159ddbb0d35 Mon Sep 17 00:00:00 2001
From: Mohamed Amir Yosef <[email protected]>
Date: Thu, 06 Nov 2025 11:05:00 -0800
Subject: [PATCH] [DC] Fix a heap-use-after-free crash in the digital credentials cross-device flow.

The crash occurred because a DigitalIdentityProviderDesktop object could
be destroyed while one of its methods was still on the stack. This
happened when TransactionImpl's constructor synchronously invoked its
completion callback on an error path (e.g., no Bluetooth adapter). This
callback would cause DigitalIdentityRequestImpl to destroy the provider,
leading to the UAF.

The fix is to post the completion callback to the task runner in these
error paths, making the call asynchronous and breaking the re-entrant
destruction.

Additionally, this CL:
- Refactors DigitalIdentityProviderDesktop::OnFinished for robustness against similar re-entrancy issues.
- Adds a unit test to verify the async callback behavior in cross_device_transaction_impl_unittest.cc.

Fixed: 458082926
Test: Added content_unittests
Change-Id: I463ed5e5c056d020b33e0f25d666ac0b925f1636
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7127620
Commit-Queue: Mohamed Amir Yosef <[email protected]>
Reviewed-by: Adem Derinel <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1541335}
---

diff --git a/chrome/browser/digital_credentials/digital_identity_provider_desktop.cc b/chrome/browser/digital_credentials/digital_identity_provider_desktop.cc
index d84f860b..1a5ed042 100644
--- a/chrome/browser/digital_credentials/digital_identity_provider_desktop.cc
+++ b/chrome/browser/digital_credentials/digital_identity_provider_desktop.cc
@@ -235,33 +235,34 @@
     return;
   }
 
+  RequestStatusForMetrics status;
   std::visit(
-      absl::Overload{
-          [this](SystemError error) {
-            EndRequestWithError(RequestStatusForMetrics::kErrorOther);
-          },
-          [this](ProtocolError error) {
-            EndRequestWithError(RequestStatusForMetrics::kErrorOther);
-          },
-          [this](RemoteError error) {
-            switch (error) {
-              case RemoteError::kNoCredential:
-                EndRequestWithError(
-                    RequestStatusForMetrics::kErrorNoCredential);
-                break;
-              case RemoteError::kUserCanceled:
-                EndRequestWithError(
-                    RequestStatusForMetrics::kErrorUserDeclined);
-                break;
-              case RemoteError::kDeviceAborted:
-                EndRequestWithError(RequestStatusForMetrics::kErrorAborted);
-                break;
-              case RemoteError::kOther:
-                EndRequestWithError(RequestStatusForMetrics::kErrorOther);
-                break;
-            }
-          }},
+      absl::Overload{[&status](SystemError error) {
+                       status = RequestStatusForMetrics::kErrorOther;
+                     },
+                     [&status](ProtocolError error) {
+                       status = RequestStatusForMetrics::kErrorOther;
+                     },
+                     [&status](RemoteError error) {
+                       switch (error) {
+                         case RemoteError::kNoCredential:
+                           status = RequestStatusForMetrics::kErrorNoCredential;
+                           break;
+                         case RemoteError::kUserCanceled:
+                           status = RequestStatusForMetrics::kErrorUserDeclined;
+                           break;
+                         case RemoteError::kDeviceAborted:
+                           status = RequestStatusForMetrics::kErrorAborted;
+                           break;
+                         case RemoteError::kOther:
+                           status = RequestStatusForMetrics::kErrorOther;
+                           break;
+                       }
+                     }},
       result.error());
+  EndRequestWithError(status);
+  // NOTE: `EndRequestWithError` may delete `this`, so it must be the last
+  // thing called in this method.
 }
 
 DigitalIdentityMultiStepDialog*
diff --git a/content/browser/digital_credentials/cross_device_transaction_impl.cc b/content/browser/digital_credentials/cross_device_transaction_impl.cc
index a09a12e..6298771c 100644
--- a/content/browser/digital_credentials/cross_device_transaction_impl.cc
+++ b/content/browser/digital_credentials/cross_device_transaction_impl.cc
@@ -156,7 +156,10 @@
 
   if (!adapter_->IsPresent()) {
     FIDO_LOG(EVENT) << "No BLE adapter.";
-    std::move(callback_).Run(base::unexpected(SystemError::kNoBleSupport));
+    base::SingleThreadTaskRunner::GetCurrentDefault()->PostTask(
+        FROM_HERE,
+        base::BindOnce(std::move(callback_),
+                       base::unexpected(SystemError::kNoBleSupport)));
     return;
   }
 
@@ -171,8 +174,10 @@
       return;
     case device::BluetoothAdapter::PermissionStatus::kDenied:
       FIDO_LOG(EVENT) << "BLE permission denied.";
-      std::move(callback_).Run(
-          base::unexpected(SystemError::kPermissionDenied));
+      base::SingleThreadTaskRunner::GetCurrentDefault()->PostTask(
+          FROM_HERE,
+          base::BindOnce(std::move(callback_),
+                         base::unexpected(SystemError::kPermissionDenied)));
       return;
     case device::BluetoothAdapter::PermissionStatus::kAllowed:
       break;
@@ -197,8 +202,10 @@
   if (status == device::BluetoothAdapter::PermissionStatus::kDenied) {
     FIDO_LOG(EVENT) << "BLE permission denied.";
     if (callback_) {
-      std::move(callback_).Run(
-          base::unexpected(SystemError::kPermissionDenied));
+      base::SingleThreadTaskRunner::GetCurrentDefault()->PostTask(
+          FROM_HERE,
+          base::BindOnce(std::move(callback_),
+                         base::unexpected(SystemError::kPermissionDenied)));
     }
     return;
   }
diff --git a/content/browser/digital_credentials/cross_device_transaction_impl_unittest.cc b/content/browser/digital_credentials/cross_device_transaction_impl_unittest.cc
index 27dadeb..d9d1c7a 100644
--- a/content/browser/digital_credentials/cross_device_transaction_impl_unittest.cc
+++ b/content/browser/digital_credentials/cross_device_transaction_impl_unittest.cc
@@ -111,6 +111,8 @@
   std::unique_ptr<Transaction> transaction = Transaction::New(
       RequestInfo(request_type(), origin(), request()), qr_generator_key(),
       network_context_factory(), base::DoNothing(), callback_.GetCallback());
+  // Callback should not have been called synchronously.
+  EXPECT_FALSE(callback_.IsReady());
   EXPECT_THAT(callback_.Take(), ContainsError(SystemError::kNoBleSupport));
 }
 

diff --git a/content/browser/digital_credentials/cross_device_transaction_impl_unittest.cc b/content/browser/digital_credentials/cross_device_transaction_impl_unittest.cc
index 27dadeb..d9d1c7a 100644
--- a/content/browser/digital_credentials/cross_device_transaction_impl_unittest.cc
+++ b/content/browser/digital_credentials/cross_device_transaction_impl_unittest.cc
@@ -111,6 +111,8 @@
   std::unique_ptr<Transaction> transaction = Transaction::New(
       RequestInfo(request_type(), origin(), request()), qr_generator_key(),
       network_context_factory(), base::DoNothing(), callback_.GetCallback());
+  // Callback should not have been called synchronously.
+  EXPECT_FALSE(callback_.IsReady());
   EXPECT_THAT(callback_.Take(), ContainsError(SystemError::kNoBleSupport));
 }