From d426f672125bfa1f92653bd0b4ab39814b4340b5 Mon Sep 17 00:00:00 2001
From: Joe Downing <joedow@google.com>
Date: Mon, 20 Apr 2026 15:10:41 -0700
Subject: [PATCH] CRD: Fix Use-After-Free in WtsSessionProcessDelegate::Core

A race condition during the shutdown of WtsSessionProcessDelegate::Core
in the Windows Remoting Host allowed asynchronous Job object
notifications to be processed after the object's destruction. Because
the object pointer is stored in the Windows kernel as a raw integer,
MiraclePtr protections were completely bypassed.

This patch ensures that WtsSessionProcessDelegate::Core stays alive
until the JOB_OBJECT_MSG_ACTIVE_PROCESS_ZERO notification is received,
confirming that all processes in the job have exited and no further
notifications will be sent to the completion port.

Additionally, all PostTask calls in Core now use base::RetainedRef(this)
to ensure the object is kept alive while tasks are pending on other
threads.

Bug: 501833981
Change-Id: I5dbfc73acf22425f038a381fd852baeaba3ef0d9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7773702
Commit-Queue: Joe Downing <joedow@chromium.org>
Reviewed-by: Yuwei Huang <yuweih@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1617797}
---

diff --git a/remoting/host/win/wts_session_process_delegate.cc b/remoting/host/win/wts_session_process_delegate.cc
index b0c8d60..4bfeed9 100644
--- a/remoting/host/win/wts_session_process_delegate.cc
+++ b/remoting/host/win/wts_session_process_delegate.cc
@@ -7,6 +7,7 @@
 
 #include "remoting/host/win/wts_session_process_delegate.h"
 
+#include <atomic>
 #include <memory>
 #include <utility>
 
@@ -45,7 +46,7 @@
 #include "remoting/host/worker_process_ipc_delegate.h"
 #include "remoting/host/worker_process_launcher.h"
 
-using base::win::ScopedHandle;
+    using base::win::ScopedHandle;
 
 // Name of the default session desktop.
 const char kDefaultDesktopName[] = "winsta0\\default";
@@ -55,6 +56,12 @@
 // A private class actually implementing the functionality provided by
 // |WtsSessionProcessDelegate|. This class is ref-counted and implements
 // asynchronous fire-and-forget shutdown.
+//
+// Most methods of this class run on the caller's thread (the thread that
+// created the Core object). However, it also uses an I/O task runner (the
+// `io_task_runner_` member) to receive and handle job object notifications
+// (e.g., process creation and exit events). The class coordinates between
+// these two threads.
 class WtsSessionProcessDelegate::Core
     : public base::RefCountedThreadSafe<Core>,
       public base::MessagePumpForIO::IOHandler,
@@ -166,15 +173,21 @@
   mojo::PlatformChannelServerEndpoint elevated_server_endpoint_;
 
   // If launching elevated, this is the pid of the launcher process.
-  base::ProcessId elevated_launcher_pid_ = base::kNullProcessId;
+  std::atomic<base::ProcessId> elevated_launcher_pid_ = base::kNullProcessId;
 
   // Tracks the id of the worker process.
-  base::ProcessId worker_process_pid_ = base::kNullProcessId;
+  std::atomic<base::ProcessId> worker_process_pid_ = base::kNullProcessId;
 
   // The pending process connection for the process being launched.
   mojo::OutgoingInvitation mojo_invitation_;
 
   mojo::AssociatedRemote<mojom::WorkerProcessControl> worker_process_control_;
+
+  // Keeps this object alive until all job object notifications are received.
+  scoped_refptr<Core> self_;
+
+  // True if Stop() has been called.
+  bool stopped_ = false;
 };
 
 WtsSessionProcessDelegate::Core::Core(
@@ -236,6 +249,8 @@
 void WtsSessionProcessDelegate::Core::Stop() {
   DCHECK(caller_task_runner_->BelongsToCurrentThread());
 
+  stopped_ = true;
+
   KillProcess();
 
   // Drain the completion queue to make sure all job object notifications have
@@ -328,27 +343,27 @@
       break;
     }
     case JOB_OBJECT_MSG_NEW_PROCESS: {
-      if (elevated_launcher_pid_ == base::kNullProcessId) {
+      if (elevated_launcher_pid_.load() == base::kNullProcessId) {
         // Ignore process launch events when we don't have a valid launcher pid.
         return;
       }
 
-      if (process_id != elevated_launcher_pid_) {
-        DCHECK_EQ(worker_process_pid_, base::kNullProcessId);
+      if (process_id != elevated_launcher_pid_.load()) {
+        DCHECK_EQ(worker_process_pid_.load(), base::kNullProcessId);
         worker_process_pid_ = process_id;
       }
       break;
     }
     case JOB_OBJECT_MSG_EXIT_PROCESS: {
-      if (process_id == worker_process_pid_) {
+      if (process_id == worker_process_pid_.load()) {
         // In official builds the first launch of a UiAccess enabled binary
         // will fail due to 'STATUS_ELEVATION_REQUIRED'.  This is an artifact of
         // using ShellExecuteEx() to launch the process.  In this scenario, we
         // will clear out the previously stored value for |worker_process_pid_|
         // and retry after the subsequent relaunch of the worker process.
         worker_process_pid_ = base::kNullProcessId;
-      } else if (process_id == elevated_launcher_pid_) {
-        if (worker_process_pid_ == base::kNullProcessId) {
+      } else if (process_id == elevated_launcher_pid_.load()) {
+        if (worker_process_pid_.load() == base::kNullProcessId) {
           // The elevated launcher process can fail to launch without attemping
           // to launch the worker.  In this scenario, the failure will be
           // detected outside this method and the elevated launcher will be
@@ -358,7 +373,7 @@
 
         caller_task_runner_->PostTask(
             FROM_HERE, base::BindOnce(&Core::OnProcessLaunchDetected, this,
-                                      worker_process_pid_));
+                                      worker_process_pid_.load()));
       }
       break;
     }
@@ -455,6 +470,7 @@
   }
 
   if (launch_elevated_) {
+    elevated_launcher_pid_ = GetProcessId(worker_process.Get());
     if (!AssignProcessToJobObject(job_.Get(), worker_process.Get())) {
       PLOG(ERROR) << "Failed to assign the worker to the job object";
       ReportFatalError();
@@ -474,7 +490,6 @@
     // worker process launch is detected. Until then, store the values needed in
     // fields. See OnProcessLaunchDetected for their use.
     elevated_server_endpoint_ = elevated_mojo_channel->TakeServerEndpoint();
-    elevated_launcher_pid_ = GetProcessId(worker_process.Get());
     DCHECK(elevated_server_endpoint_.is_valid());
   } else {
     mojo::OutgoingInvitation::Send(std::move(mojo_invitation_),
@@ -500,10 +515,14 @@
   if (job_.is_valid()) {
     job_.Close();
 
-    // Drain the completion queue to make sure all job object notification have
+    // Drain the completion queue to make sure all job object notifications have
     // been received.
     io_task_runner_->PostTask(
         FROM_HERE, base::BindOnce(&Core::DrainJobNotifications, this));
+  } else {
+    // The job object has been closed and the completion port queue has been
+    // drained.
+    self_ = nullptr;
   }
 }
 
@@ -526,8 +545,23 @@
   DCHECK(caller_task_runner_->BelongsToCurrentThread());
   DCHECK(!job_.is_valid());
 
+  if (stopped_) {
+    // If Stop() was called before the job was initialized, we must still ensure
+    // the completion port is drained because InitializeJob() has already
+    // called RegisterJobObject().
+    job_ = std::move(job);
+    self_ = this;
+    DrainJobNotificationsCompleted();
+    return;
+  }
+
   job_ = std::move(job);
 
+  // Keep this object alive until the job object notifications have been
+  // drained. This ensures that OnIOCompleted() is not called with a dangling
+  // pointer.
+  self_ = this;
+
   if (launch_pending_) {
     DoLaunchProcess();
   }
@@ -546,7 +580,7 @@
 void WtsSessionProcessDelegate::Core::OnProcessLaunchDetected(
     base::ProcessId pid) {
   DCHECK(caller_task_runner_->BelongsToCurrentThread());
-  DCHECK_NE(pid, elevated_launcher_pid_);
+  DCHECK_NE(pid, elevated_launcher_pid_.load());