From e88215633b3fd3889277220dc25cc4343a8ec16b Mon Sep 17 00:00:00 2001
From: Yuwei Huang <[email protected]>
Date: Mon, 27 Apr 2026 11:28:39 -0700
Subject: [PATCH] [remoting] Robust session ID check for ChromotingHostServices on Windows

* Add session_id to ConnectionInfo, which comes from
  GetNamedPipeClientSessionId() and is not susceptible of PID-reuse
  attacks. GetNamedPipeClientSessionId() does not require additional
  permissions, such that if GetNamedPipeClientProcessId() works then it
  also works.
* Move the ChromotingHostServices Windows implementation from the
  network process to the daemon process, and have the daemon process
  pass the ChromotingSessionServices receiver to network via
  DesktopSessionConnectionEvents. This is consistent with the Linux's
  implementation.
* Perform the session ID check right in the daemon process, which
  simplifies the logic and makes it possible to remove the limited
  process info permission grant from ChromotingHostServicesClient.
* Refactor DaemonProcess code. Move shared ChromotingHostServices code
  into the daemon process.
* Remove session_id and peer_pid from the mojo interface and all other
  places since they are no longer needed.

Bug: 496193452
Change-Id: I6e613f8fd05473f814434a1572700e68edd081c8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7789250
Reviewed-by: Joe Downing <[email protected]>
Commit-Queue: Yuwei Huang <[email protected]>
Auto-Submit: Yuwei Huang <[email protected]>
Reviewed-by: Elly <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1621220}
---

diff --git a/components/named_mojo_ipc_server/connection_info.h b/components/named_mojo_ipc_server/connection_info.h
index 18f87c8..dbdad3b 100644
--- a/components/named_mojo_ipc_server/connection_info.h
+++ b/components/named_mojo_ipc_server/connection_info.h
@@ -10,6 +10,8 @@
 #include "build/buildflag.h"
 
 #if BUILDFLAG(IS_WIN)
+#include <cstdint>
+
 #include "base/win/scoped_handle.h"
 #elif BUILDFLAG(IS_MAC)
 #include <bsm/libbsm.h>
@@ -36,6 +38,9 @@
   // The process of the peer. Only valid if `include_peer_process_info` is true
   // in EndpointOptions.
   base::Process process;
+
+  // The Windows session ID of the peer.
+  uint32_t session_id = UINT32_MAX;
 #endif
 };
 
diff --git a/components/named_mojo_ipc_server/named_mojo_server_endpoint_connector_win.cc b/components/named_mojo_ipc_server/named_mojo_server_endpoint_connector_win.cc
index 93a2f08..ca6a8393 100644
--- a/components/named_mojo_ipc_server/named_mojo_server_endpoint_connector_win.cc
+++ b/components/named_mojo_ipc_server/named_mojo_server_endpoint_connector_win.cc
@@ -181,6 +181,14 @@
     OnError();
     return;
   }
+  ULONG peer_session_id;
+  if (!GetNamedPipeClientSessionId(pending_named_pipe_handle_.Get(),
+                                   &peer_session_id)) {
+    PLOG(ERROR) << "Failed to get peer session ID";
+    OnError();
+    return;
+  }
+  info->session_id = peer_session_id;
   if (options_.include_peer_process_info) {
     info->process = base::Process::OpenWithAccess(
         info->pid, PROCESS_QUERY_LIMITED_INFORMATION);
diff --git a/remoting/host/basic_desktop_environment.cc b/remoting/host/basic_desktop_environment.cc
index f08dd364..fecf23a2 100644
--- a/remoting/host/basic_desktop_environment.cc
+++ b/remoting/host/basic_desktop_environment.cc
@@ -145,10 +145,6 @@
 void BasicDesktopEnvironment::SetCapabilities(const std::string& capabilities) {
 }
 
-std::uint32_t BasicDesktopEnvironment::GetDesktopSessionId() const {
-  return UINT32_MAX;
-}
-
 std::unique_ptr<RemoteWebAuthnStateChangeNotifier>
 BasicDesktopEnvironment::CreateRemoteWebAuthnStateChangeNotifier() {
   return std::make_unique<RemoteWebAuthnExtensionNotifier>();
diff --git a/remoting/host/basic_desktop_environment.h b/remoting/host/basic_desktop_environment.h
index 94a7020..deea243 100644
--- a/remoting/host/basic_desktop_environment.h
+++ b/remoting/host/basic_desktop_environment.h
@@ -57,7 +57,6 @@
       override;
   std::string GetCapabilities() const override;
   void SetCapabilities(const std::string& capabilities) override;
-  std::uint32_t GetDesktopSessionId() const override;
   std::unique_ptr<RemoteWebAuthnStateChangeNotifier>
   CreateRemoteWebAuthnStateChangeNotifier() override;
 
diff --git a/remoting/host/chromoting_host.cc b/remoting/host/chromoting_host.cc
index 33b5a1b..efe44282 100644
--- a/remoting/host/chromoting_host.cc
+++ b/remoting/host/chromoting_host.cc
@@ -157,15 +157,18 @@
 void ChromotingHost::BindChromotingHostServicesForServer(
     mojo::PendingReceiver<mojom::ChromotingHostServices> receiver,
     std::unique_ptr<named_mojo_ipc_server::ConnectionInfo> connection_info) {
-  BindChromotingHostServices(std::move(receiver), connection_info->pid);
+  BindChromotingHostServices(std::move(receiver));
 }
 #endif
 
+#if BUILDFLAG(IS_MAC) || BUILDFLAG(IS_LINUX)
 void ChromotingHost::BindChromotingHostServices(
-    mojo::PendingReceiver<mojom::ChromotingHostServices> receiver,
-    base::ProcessId peer_pid) {
-  receivers_.Add(this, std::move(receiver), peer_pid);
+    mojo::PendingReceiver<mojom::ChromotingHostServices> receiver) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  receivers_.Add(this, std::move(receiver));
 }
+#endif
 
 void ChromotingHost::AddExtension(std::unique_ptr<HostExtension> extension) {
   extensions_.push_back(std::move(extension));
@@ -297,6 +300,7 @@
   return per_session_policies_validator_.Run(policies);
 }
 
+#if BUILDFLAG(IS_MAC) || BUILDFLAG(IS_LINUX)
 void ChromotingHost::BindSessionServices(
     mojo::PendingReceiver<mojom::ChromotingSessionServices> receiver) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
@@ -307,24 +311,11 @@
                  << "No connected remote desktop client was found.";
     return;
   }
-#if BUILDFLAG(IS_WIN)
-  DWORD peer_session_id;
-  if (!ProcessIdToSessionId(receivers_.current_context(), &peer_session_id)) {
-    PLOG(ERROR) << "Session services bind request rejected: "
-                   "ProcessIdToSessionId failed";
-    return;
-  }
-  if (connected_client->desktop_session_id() != peer_session_id) {
-    LOG(WARNING)
-        << "Session services bind request rejected: "
-        << "Remote desktop client is not connected to the current session.";
-    return;
-  }
-#endif
   connected_client->OnSessionServicesClientConnected(std::move(receiver));
   VLOG(1) << "Session services bound for receiver ID: "
           << receivers_.current_receiver();
 }
+#endif
 
 void ChromotingHost::OnIncomingSession(
     protocol::Session* session,
diff --git a/remoting/host/chromoting_host.h b/remoting/host/chromoting_host.h
index 71357bab..82867fe 100644
--- a/remoting/host/chromoting_host.h
+++ b/remoting/host/chromoting_host.h
@@ -76,8 +76,17 @@
 //    all pending tasks to complete. After all of that has completed, we
 //    return to the idle state. We then go to step (2) to wait for a new
 //    incoming connection.
-class ChromotingHost : public ClientSession::EventHandler,
-                       public mojom::ChromotingHostServices {
+class ChromotingHost :
+// The ChromotingHostServices inheritance is currently needed by the Mac host
+// and the single-process Linux host. For the Windows host and the Linux
+// multi-process host, ChromotingHostServices is implemented by the daemon
+// process and the ChromotingSessionServices receiver is passed through
+// DesktopSessionConnectionEvents.
+#if BUILDFLAG(IS_MAC) || BUILDFLAG(IS_LINUX)
+    public mojom::ChromotingHostServices,
+
+#endif
+    public ClientSession::EventHandler {
  public:
   // This is a multimap to allow for multiple unauthenticated sessions. For each
   // client ID, there can be up to one authenticated session and multiple
@@ -124,8 +133,8 @@
 #if BUILDFLAG(IS_LINUX)
   // Starts running the ChromotingHostServices server and listening for incoming
   // IPC binding requests.
-  // Currently only Linux runs the ChromotingHostServices server on the host
-  // process.
+  // Currently only the single-process Linux host runs the
+  // ChromotingHostServices server on the host process.
   void StartChromotingHostServices();
 
   void BindChromotingHostServicesForServer(
@@ -133,9 +142,10 @@
       std::unique_ptr<named_mojo_ipc_server::ConnectionInfo> connection_info);

diff --git a/remoting/host/chromoting_host_unittest.cc b/remoting/host/chromoting_host_unittest.cc
index e669be2..2e1c0bc9 100644
--- a/remoting/host/chromoting_host_unittest.cc
+++ b/remoting/host/chromoting_host_unittest.cc
@@ -51,10 +51,6 @@
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
-#if BUILDFLAG(IS_WIN)
-#include <windows.h>
-#endif
-
 using ::remoting::protocol::MockClientStub;
 using ::remoting::protocol::MockConnectionToClientEventHandler;
 using ::remoting::protocol::MockHostStub;
@@ -278,43 +274,12 @@
     host_->per_session_policies_validator_ = validator;
   }
 
+#if BUILDFLAG(IS_MAC) || BUILDFLAG(IS_LINUX)
   mojo::Remote<mojom::ChromotingHostServices> BindChromotingHostServices() {
     mojo::Remote<mojom::ChromotingHostServices> remote;
-    // ChromotingHost::BindSessionServices calls ProcessIdToSessionId() on the
-    // IPC client's PID. The PID we know that always works is the current
-    // process' PID.
-    auto current_pid = base::GetCurrentProcId();
-    host_->BindChromotingHostServices(remote.BindNewPipeAndPassReceiver(),
-                                      current_pid);
+    host_->BindChromotingHostServices(remote.BindNewPipeAndPassReceiver());
     return remote;
   }
-
-#if BUILDFLAG(IS_WIN)
-  // Simulates the IPC client's session ID for the session ID check in
-  // ChromotingHost::BindSessionServices.
-  //
-  // |is_remote_desktop_session_id|: True if the simulated session ID should be
-  // exactly the session ID of the fake desktop environment. If false, the
-  // simulated session ID is guaranteed to be different from the desktop
-  // environment's session ID.
-  void SimulateIpcClientSessionId(bool is_remote_desktop_session_id) {
-    // ChromotingHost::BindSessionServices calls ProcessIdToSessionId() on the
-    // IPC client's PID. The PID we know that always works is the current
-    // process' PID.
-    auto current_pid = base::GetCurrentProcId();
-    DWORD current_session_id;
-    bool success = ProcessIdToSessionId(current_pid, &current_session_id);
-    ASSERT_TRUE(success);
-    // The IPC client's session ID is exactly the current process' session ID
-    // at this point, so we change the fake desktop environment's session ID
-    // here.
-    if (is_remote_desktop_session_id) {
-      desktop_environment_factory_->set_desktop_session_id(current_session_id);
-    } else {
-      desktop_environment_factory_->set_desktop_session_id(current_session_id +
-                                                           1);
-    }
-  }
 #endif
 
  protected:
@@ -645,6 +610,7 @@
   SimulateClientConnection(0, /* authenticate= */ true, /* reject= */ true);
 }
 
+#if BUILDFLAG(IS_MAC) || BUILDFLAG(IS_LINUX)
 TEST_F(ChromotingHostTest, BindSessionServicesWithNoConnectedSession_Rejected) {
   StartHost();
 
@@ -655,13 +621,12 @@
   host_->BindSessionServices(std::move(receiver));
   wait_for_disconnect_run_loop.Run();
 }
+#endif
 
+#if BUILDFLAG(IS_MAC) || BUILDFLAG(IS_LINUX)
 TEST_F(ChromotingHostTest, BindSessionServicesWithConnectedSession_Accepted) {
   StartHost();
   auto host_services_remote = BindChromotingHostServices();
-#if BUILDFLAG(IS_WIN)
-  SimulateIpcClientSessionId(/* is_remote_desktop_session_id= */ true);
-#endif
   auto future = ExpectClientConnected(0);
   SimulateClientConnection(0, true, false);
   future->Get();
@@ -683,25 +648,6 @@
   host_services_remote->BindSessionServices(std::move(receiver));
   wait_for_version_run_loop.Run();
 }
-
-#if BUILDFLAG(IS_WIN)
-TEST_F(ChromotingHostTest, BindSessionServicesWithWrongSession_Rejected) {
-  StartHost();
-  auto host_services_remote = BindChromotingHostServices();
-  SimulateIpcClientSessionId(/* is_remote_desktop_session_id= */ false);
-  auto future = ExpectClientConnected(0);
-  SimulateClientConnection(0, true, false);
-  future->Get();
-
-  mojo::Remote<mojom::ChromotingSessionServices> remote;
-  auto receiver = remote.BindNewPipeAndPassReceiver();
-  base::RunLoop wait_for_disconnect_run_loop;
-  remote.set_disconnect_handler(wait_for_disconnect_run_loop.QuitClosure());
-  // Note that we can't just call host_->BindSessionServices(), since that
-  // doesn't have the peer PID context.
-  host_services_remote->BindSessionServices(std::move(receiver));
-  wait_for_disconnect_run_loop.Run();
-}
 #endif
 
 }  // namespace remoting
diff --git a/remoting/host/daemon_process_unittest.cc b/remoting/host/daemon_process_unittest.cc
index 66643f3..c54488c 100644
--- a/remoting/host/daemon_process_unittest.cc
+++ b/remoting/host/daemon_process_unittest.cc
@@ -66,7 +66,7 @@
 
   MOCK_METHOD(bool,
               OnDesktopSessionAgentAttached,
-              (int, int, mojo::ScopedMessagePipeHandle),
+              (int, mojo::ScopedMessagePipeHandle),
               (override));
 
   MOCK_METHOD(DesktopSession*, DoCreateDesktopSessionPtr, (int));
@@ -77,7 +77,12 @@
               (const std::string&),
               (override));
   MOCK_METHOD(void, SendTerminalDisconnected, (int terminal_id), (override));
-  MOCK_METHOD(void, StartChromotingHostServices, (), (override));
+
+  // mojom::ChromotingHostServices implementation.
+  MOCK_METHOD(void,
+              BindSessionServices,
+              (mojo::PendingReceiver<mojom::ChromotingSessionServices>),
+              (override));
 };
 
 FakeDesktopSession::FakeDesktopSession(DaemonProcess* daemon_process, int id)
@@ -165,8 +170,6 @@
   EXPECT_CALL(*daemon_process_, LaunchNetworkProcess())
       .Times(AnyNumber())
       .WillRepeatedly(Invoke(this, &DaemonProcessTest::LaunchNetworkProcess));
-  EXPECT_CALL(*daemon_process_, StartChromotingHostServices())
-      .Times(AnyNumber());
 }
 
 void DaemonProcessTest::TearDown() {
diff --git a/remoting/host/ipc_desktop_environment_unittest.cc b/remoting/host/ipc_desktop_environment_unittest.cc
index 1289184..2e33479 100644
--- a/remoting/host/ipc_desktop_environment_unittest.cc
+++ b/remoting/host/ipc_desktop_environment_unittest.cc
@@ -387,7 +387,7 @@
   EXPECT_CALL(client_session_control_, SetDisableInputs(_)).Times(0);
 
   // Most tests will only call this once but reattach will call multiple times.
-  EXPECT_CALL(client_session_events_, OnDesktopAttached(_))
+  EXPECT_CALL(client_session_events_, OnDesktopAttached())
       .Times(AnyNumber())
       .WillRepeatedly(InvokeWithoutArgs(
           this, &IpcDesktopEnvironmentTest::QuitSetupRunLoop));
@@ -578,7 +578,7 @@
     mojo::ScopedMessagePipeHandle desktop_pipe) {
   // Instruct DesktopSessionProxy to connect to the network-to-desktop pipe.
   desktop_environment_factory_->OnDesktopSessionAgentAttached(
-      terminal_id_, /*session_id=*/0, std::move(desktop_pipe));
+      terminal_id_, std::move(desktop_pipe));
 }
 
 void IpcDesktopEnvironmentTest::RunMainLoopUntilDone() {
diff --git a/remoting/host/mac/agent_process_broker_unittest.cc b/remoting/host/mac/agent_process_broker_unittest.cc
index a24032f9..78be392 100644
--- a/remoting/host/mac/agent_process_broker_unittest.cc
+++ b/remoting/host/mac/agent_process_broker_unittest.cc
@@ -87,8 +87,7 @@
   void BindRemotingHostControl(
       mojo::PendingReceiver<mojom::RemotingHostControl> receiver) override;
   void BindChromotingHostServices(
-      mojo::PendingReceiver<mojom::ChromotingHostServices> receiver,
-      int32_t peer_pid) override;
+      mojo::PendingReceiver<mojom::ChromotingHostServices> receiver) override;
 
  private:
   void WriteAgentState(std::string_view state);
@@ -121,8 +120,7 @@
 }
 
 void TestAgentProcess::BindChromotingHostServices(
-    mojo::PendingReceiver<mojom::ChromotingHostServices> receiver,
-    int32_t peer_pid) {
+    mojo::PendingReceiver<mojom::ChromotingHostServices> receiver) {
   WriteAgentState(kAgentStateChromotingHostServicesBound);
 }
diff --git a/remoting/host/security_key/security_key_extension_session_unittest.cc b/remoting/host/security_key/security_key_extension_session_unittest.cc
index ef41018..07396764 100644
--- a/remoting/host/security_key/security_key_extension_session_unittest.cc
+++ b/remoting/host/security_key/security_key_extension_session_unittest.cc
@@ -158,13 +158,9 @@
   ~TestClientSessionDetails() override;
 
   // ClientSessionDetails interface.
-  uint32_t desktop_session_id() const override { return desktop_session_id_; }
   ClientSessionControl* session_control() override { return nullptr; }
 
-  void set_desktop_session_id(uint32_t new_id) { desktop_session_id_ = new_id; }
-
  private:
-  uint32_t desktop_session_id_ = UINT32_MAX;
 };
 
 TestClientSessionDetails::TestClientSessionDetails() = default;