From 4cd3afe43493db27c5655796748b3929db0f0f83 Mon Sep 17 00:00:00 2001
From: Joe Downing <joedow@google.com>
Date: Fri, 05 Jun 2026 16:21:45 -0700
Subject: [PATCH] [remoting] Fix handle management and ObjectWatcher violation in UnprivilegedProcessDelegate

This CL addresses two critical handle management flaws in the Windows
UnprivilegedProcessDelegate that allowed a compromised worker to persist
and potentially trigger memory corruption in the privileged daemon.

1. ObjectWatcher Contract Violation: base::win::ObjectWatcher explicitly
   forbids closing a handle while a wait is pending. KillProcess() was
   closing the handle before stopping the watcher. This led to a data
   race on the callback's scoped_refptr during process relaunch.
   We now add StopWatching() to WindowsProcessDelegate to ensure the
   watcher is stopped before the handle is closed.

2. Insufficient Access Rights: The worker process handle duplicated for
   monitoring lacked the PROCESS_TERMINATE right. This prevented
   KillProcess() from successfully calling TerminateProcess(), allowing
   compromised processes to remain active. We now ensure the handle
   has the necessary rights.

Bug: 513480539
Test: remoting_unittests --gtest_filter=UnprivilegedProcessDelegateTest.*
Change-Id: Ib989896c210dc5d5754678752a0d90bb30f0357b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7899634
Commit-Queue: Joe Downing <joedow@chromium.org>
Reviewed-by: Yuwei Huang <yuweih@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1642669}
---

diff --git a/remoting/host/win/BUILD.gn b/remoting/host/win/BUILD.gn
index 9e3eae03..77861b9 100644
--- a/remoting/host/win/BUILD.gn
+++ b/remoting/host/win/BUILD.gn
@@ -169,6 +169,7 @@
     "session_interaction_strategy.h",
     "simple_task_dialog.cc",
     "simple_task_dialog.h",
+    "unprivileged_process_delegate.cc",
     "unprivileged_process_delegate.h",
     "window_station_and_desktop.cc",
     "window_station_and_desktop.h",
@@ -205,6 +206,7 @@
     "//crypto",
     "//ipc",
     "//remoting/base",
+    "//remoting/base/crash",
     "//remoting/host:common_headers",
     "//remoting/host:ipc_constants",
     "//remoting/host:platform_interfaces",
@@ -238,6 +240,7 @@
     "event_trace_data_unittest.cc",
     "mouse_cursor_monitor_win_unittest.cc",
     "rdp_client_unittest.cc",
+    "unprivileged_process_delegate_unittest.cc",
   ]
 
   deps = [
@@ -422,7 +425,6 @@
     "host_service.cc",
     "rdp_desktop_session.cc",
     "rdp_desktop_session.h",
-    "unprivileged_process_delegate.cc",
     "wts_session_process_delegate.cc",
   ]
   deps = [
diff --git a/remoting/host/win/unprivileged_process_delegate.cc b/remoting/host/win/unprivileged_process_delegate.cc
index b82eceb..7d11ce5f 100644
--- a/remoting/host/win/unprivileged_process_delegate.cc
+++ b/remoting/host/win/unprivileged_process_delegate.cc
@@ -403,8 +403,8 @@
 
   if (worker_process_.is_valid()) {
     TerminateProcess(worker_process_.Get(), CONTROL_C_EXIT);
-    worker_process_.Close();
   }
+  StopWatching();
 }
 
 void UnprivilegedProcessDelegate::OnChannelConnected(int32_t peer_pid) {
@@ -454,9 +454,10 @@
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
   // Report a handle that can be used to wait for the worker process completion,
-  // query information about the process and duplicate handles.
-  DWORD desired_access =
-      SYNCHRONIZE | PROCESS_DUP_HANDLE | PROCESS_QUERY_INFORMATION;
+  // query information about the process, duplicate handles, and terminate the
+  // process.
+  DWORD desired_access = SYNCHRONIZE | PROCESS_DUP_HANDLE |
+                         PROCESS_QUERY_INFORMATION | PROCESS_TERMINATE;
   HANDLE temp_handle;
   if (!DuplicateHandle(GetCurrentProcess(), worker_process.Get(),
                        GetCurrentProcess(), &temp_handle, desired_access, FALSE,
diff --git a/remoting/host/win/unprivileged_process_delegate.h b/remoting/host/win/unprivileged_process_delegate.h
index 28a3027..e18710b5 100644
--- a/remoting/host/win/unprivileged_process_delegate.h
+++ b/remoting/host/win/unprivileged_process_delegate.h
@@ -61,6 +61,9 @@
   void CrashProcess(const base::Location& location) override;
   void KillProcess() override;
 
+ protected:
+  virtual void ReportProcessLaunched(base::win::ScopedHandle worker_process);
+
  private:
   // IPC::Listener implementation.
   void OnChannelConnected(int32_t peer_pid) override;
@@ -70,7 +73,6 @@
       mojo::ScopedInterfaceEndpointHandle handle) override;
 
   void ReportFatalError();
-  void ReportProcessLaunched(base::win::ScopedHandle worker_process);
 
   // The task runner serving job object notifications.
   scoped_refptr<base::SingleThreadTaskRunner> io_task_runner_;
diff --git a/remoting/host/win/unprivileged_process_delegate_unittest.cc b/remoting/host/win/unprivileged_process_delegate_unittest.cc
new file mode 100644
index 0000000..a4647248
--- /dev/null
+++ b/remoting/host/win/unprivileged_process_delegate_unittest.cc
@@ -0,0 +1,146 @@
+// Copyright 2026 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "remoting/host/win/unprivileged_process_delegate.h"
+
+#include <memory>
+
+#include "base/command_line.h"
+#include "base/run_loop.h"
+#include "base/task/single_thread_task_runner.h"
+#include "base/test/multiprocess_test.h"
+#include "base/test/task_environment.h"
+#include "remoting/host/worker_process_ipc_delegate.h"
+#include "remoting/host/worker_process_launcher.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "testing/multiprocess_func_list.h"
+
+namespace remoting {
+
+namespace {
+
+// Use a simple test delegate to satisfy the UnprivilegedProcessDelegate.
+class FakeDelegate : public WorkerProcessIpcDelegate {
+ public:
+  FakeDelegate() {}
+  ~FakeDelegate() override {}
+
+  // WorkerProcessIpcDelegate implementation.
+  void OnChannelConnected(int32_t peer_pid) override {}
+  void OnPermanentError(int exit_code) override {}
+  void OnWorkerProcessStopped() override {}
+  void OnAssociatedInterfaceRequest(
+      const std::string& interface_name,
+      mojo::ScopedInterfaceEndpointHandle handle) override {}
+};
+
+class FakeUnprivilegedProcessDelegate : public UnprivilegedProcessDelegate {
+ public:
+  using UnprivilegedProcessDelegate::UnprivilegedProcessDelegate;
+
+  void LaunchProcess(WorkerProcessLauncher* event_handler) override {
+    event_handler_ = event_handler;
+
+    base::CommandLine command_line =
+        base::GetMultiProcessTestChildBaseCommandLine();
+    base::Process process = base::SpawnMultiProcessTestChild(
+        "UnprivilegedProcessDelegateTestChild", command_line, {});
+    EXPECT_TRUE(process.IsValid());
+
+    ReportProcessLaunched(base::win::ScopedHandle(process.Release()));
+  }
+};
+
+class VerifyingDelegate : public FakeUnprivilegedProcessDelegate {
+ public:
+  VerifyingDelegate(scoped_refptr<base::SingleThreadTaskRunner> io_task_runner,
+                    std::unique_ptr<base::CommandLine> target_command,
+                    IntegrityLevel integrity_level,
+                    base::win::ScopedHandle* observed_process_out)
+      : FakeUnprivilegedProcessDelegate(std::move(io_task_runner),
+                                        std::move(target_command),
+                                        integrity_level),
+        observed_process_out_(observed_process_out) {}
+
+  void ReportProcessLaunched(base::win::ScopedHandle worker_process) override {
+    CHECK(worker_process.is_valid());
+    HANDLE handle;
+    if (DuplicateHandle(GetCurrentProcess(), worker_process.Get(),
+                        GetCurrentProcess(), &handle,
+                        PROCESS_QUERY_INFORMATION | SYNCHRONIZE, FALSE, 0)) {
+      observed_process_out_->Set(handle);
+    }
+    FakeUnprivilegedProcessDelegate::ReportProcessLaunched(
+        std::move(worker_process));
+  }
+

diff --git a/remoting/host/win/unprivileged_process_delegate_unittest.cc b/remoting/host/win/unprivileged_process_delegate_unittest.cc
new file mode 100644
index 0000000..a4647248
--- /dev/null
+++ b/remoting/host/win/unprivileged_process_delegate_unittest.cc
@@ -0,0 +1,146 @@
+// Copyright 2026 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "remoting/host/win/unprivileged_process_delegate.h"
+
+#include <memory>
+
+#include "base/command_line.h"
+#include "base/run_loop.h"
+#include "base/task/single_thread_task_runner.h"
+#include "base/test/multiprocess_test.h"
+#include "base/test/task_environment.h"
+#include "remoting/host/worker_process_ipc_delegate.h"
+#include "remoting/host/worker_process_launcher.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "testing/multiprocess_func_list.h"
+
+namespace remoting {
+
+namespace {
+
+// Use a simple test delegate to satisfy the UnprivilegedProcessDelegate.
+class FakeDelegate : public WorkerProcessIpcDelegate {
+ public:
+  FakeDelegate() {}
+  ~FakeDelegate() override {}
+
+  // WorkerProcessIpcDelegate implementation.
+  void OnChannelConnected(int32_t peer_pid) override {}
+  void OnPermanentError(int exit_code) override {}
+  void OnWorkerProcessStopped() override {}
+  void OnAssociatedInterfaceRequest(
+      const std::string& interface_name,
+      mojo::ScopedInterfaceEndpointHandle handle) override {}
+};
+
+class FakeUnprivilegedProcessDelegate : public UnprivilegedProcessDelegate {
+ public:
+  using UnprivilegedProcessDelegate::UnprivilegedProcessDelegate;
+
+  void LaunchProcess(WorkerProcessLauncher* event_handler) override {
+    event_handler_ = event_handler;
+
+    base::CommandLine command_line =
+        base::GetMultiProcessTestChildBaseCommandLine();
+    base::Process process = base::SpawnMultiProcessTestChild(
+        "UnprivilegedProcessDelegateTestChild", command_line, {});
+    EXPECT_TRUE(process.IsValid());
+
+    ReportProcessLaunched(base::win::ScopedHandle(process.Release()));
+  }
+};
+
+class VerifyingDelegate : public FakeUnprivilegedProcessDelegate {
+ public:
+  VerifyingDelegate(scoped_refptr<base::SingleThreadTaskRunner> io_task_runner,
+                    std::unique_ptr<base::CommandLine> target_command,
+                    IntegrityLevel integrity_level,
+                    base::win::ScopedHandle* observed_process_out)
+      : FakeUnprivilegedProcessDelegate(std::move(io_task_runner),
+                                        std::move(target_command),
+                                        integrity_level),
+        observed_process_out_(observed_process_out) {}
+
+  void ReportProcessLaunched(base::win::ScopedHandle worker_process) override {
+    CHECK(worker_process.is_valid());
+    HANDLE handle;
+    if (DuplicateHandle(GetCurrentProcess(), worker_process.Get(),
+                        GetCurrentProcess(), &handle,
+                        PROCESS_QUERY_INFORMATION | SYNCHRONIZE, FALSE, 0)) {
+      observed_process_out_->Set(handle);
+    }
+    FakeUnprivilegedProcessDelegate::ReportProcessLaunched(
+        std::move(worker_process));
+  }
+
+ private:
+  raw_ptr<base::win::ScopedHandle> observed_process_out_;
+};
+
+}  // namespace
+
+class UnprivilegedProcessDelegateTest : public testing::Test {
+ public:
+  UnprivilegedProcessDelegateTest()
+      : task_environment_(base::test::TaskEnvironment::MainThreadType::UI) {}
+
+  void TearDown() override { observed_process_.Close(); }
+
+ protected:
+  base::test::TaskEnvironment task_environment_;
+  base::win::ScopedHandle observed_process_;
+};
+
+TEST_F(UnprivilegedProcessDelegateTest, KillProcessLifecycle) {
+  base::CommandLine target_command(base::CommandLine::NO_PROGRAM);
+  auto delegate = std::make_unique<FakeUnprivilegedProcessDelegate>(
+      base::SingleThreadTaskRunner::GetCurrentDefault(),
+      std::make_unique<base::CommandLine>(target_command),
+      UnprivilegedProcessDelegate::IntegrityLevel::kLow);
+
+  FakeDelegate fake_delegate;
+  auto worker_launcher = std::make_unique<WorkerProcessLauncher>(
+      std::move(delegate), &fake_delegate);
+
+  worker_launcher.reset();
+}
+
+TEST_F(UnprivilegedProcessDelegateTest, KillProcessTerminatesWorker) {
+  base::CommandLine target_command(base::CommandLine::NO_PROGRAM);
+
+  auto verifying_delegate = std::make_unique<VerifyingDelegate>(
+      base::SingleThreadTaskRunner::GetCurrentDefault(),
+      std::make_unique<base::CommandLine>(target_command),
+      UnprivilegedProcessDelegate::IntegrityLevel::kLow, &observed_process_);
+
+  FakeDelegate fake_delegate;
+  auto worker_launcher = std::make_unique<WorkerProcessLauncher>(
+      std::move(verifying_delegate), &fake_delegate);
+
+  ASSERT_TRUE(observed_process_.is_valid());
+
+  DWORD exit_code;
+  ASSERT_TRUE(GetExitCodeProcess(observed_process_.Get(), &exit_code));
+  ASSERT_EQ(exit_code, static_cast<DWORD>(STILL_ACTIVE));
+
+  worker_launcher.reset();
+
+  // Verify the process is terminated.
+  ASSERT_TRUE(observed_process_.is_valid());
+  EXPECT_EQ(WAIT_OBJECT_0, WaitForSingleObject(observed_process_.Get(), 5000));
+
+  ASSERT_TRUE(GetExitCodeProcess(observed_process_.Get(), &exit_code));
+  EXPECT_NE(exit_code, static_cast<DWORD>(STILL_ACTIVE));
+}
+
+MULTIPROCESS_TEST_MAIN(UnprivilegedProcessDelegateTestChild) {
+  base::test::SingleThreadTaskEnvironment task_environment(
+      base::test::SingleThreadTaskEnvironment::MainThreadType::IO);
+
+  base::RunLoop().Run();
+  return 0;
+}
+
+}  // namespace remoting