From 98ca974c017839632e9293dfa4a8f8332d9bbb8e Mon Sep 17 00:00:00 2001
From: Yuwei Huang <[email protected]>
Date: Thu, 09 Apr 2026 15:12:50 -0700
Subject: [PATCH] [crd host][win] Mitigate TOCTOU in WebAuthn extension notifier on Windows

This CL addresses a potential local privilege escalation vulnerability
in the Chrome Remote Desktop Windows host by implementing user
impersonation when writing WebAuthn extension wakeup files.

Previously, the remoting_desktop.exe process (running as SYSTEM) would
write a 1-byte file to the user's LocalAppData directory without
impersonation. This could allow an unprivileged local attacker to use
NTFS junctions and oplocks to redirect the write to an arbitrary file,
potentially leading to SYSTEM compromise.

Changes:
- Added a ScopedImpersonation helper on Windows to safely impersonate
  the interactive user during file operations.
- Updated RemoteWebAuthnExtensionNotifier to capture and pass the
  Windows user token to its background IO core.
- Modified Core::WakeUpExtension to impersonate the user before
  verifying directory existence and writing the wakeup files.
- Refactored GetRemoteStateChangePaths to return the user token on
  Windows.
- Updated unit tests to handle the new constructor requirements.

Verified: Do WebAuthn forwarding on Chrome within a CRD session.
Bug: 499116954
Change-Id: Ia4dcd4677c37955a448afb296e7c472be1719b96
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7740552
Reviewed-by: Joe Downing <[email protected]>
Commit-Queue: Yuwei Huang <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1612534}
---

diff --git a/remoting/host/webauthn/remote_webauthn_extension_notifier.cc b/remoting/host/webauthn/remote_webauthn_extension_notifier.cc
index ad0c713..29c529a5 100644
--- a/remoting/host/webauthn/remote_webauthn_extension_notifier.cc
+++ b/remoting/host/webauthn/remote_webauthn_extension_notifier.cc
@@ -48,6 +48,47 @@
 // Content of file doesn't matter so we just write an empty string.
 static constexpr char kExtensionWakeupFileContent[] = "";
 
+#if BUILDFLAG(IS_WIN)
+// Helper class to impersonate a user and revert back when it goes out of scope.
+// Note that Windows impersonation is bound to the current thread, so it is
+// thread-safe.
+class ScopedImpersonation {
+ public:
+  ScopedImpersonation(const ScopedImpersonation&) = delete;
+  ScopedImpersonation& operator=(const ScopedImpersonation&) = delete;
+
+  explicit ScopedImpersonation(HANDLE user_token) {
+    if (user_token != nullptr && user_token != INVALID_HANDLE_VALUE) {
+      if (ImpersonateLoggedOnUser(user_token)) {
+        is_impersonating_ = true;
+      } else {
+        PLOG(ERROR) << "ImpersonateLoggedOnUser failed";
+      }
+    }
+  }
+
+  ~ScopedImpersonation() {
+    if (is_impersonating_) {
+      RevertToSelf();
+    }
+  }
+
+  bool is_impersonating() const { return is_impersonating_; }
+
+ private:
+  bool is_impersonating_ = false;
+};
+#endif
+
+}  // namespace
+
+RemoteWebAuthnExtensionNotifier::RemoteStateChangeContext::
+    RemoteStateChangeContext() = default;
+RemoteWebAuthnExtensionNotifier::RemoteStateChangeContext::
+    RemoteStateChangeContext(RemoteStateChangeContext&&) = default;
+RemoteWebAuthnExtensionNotifier::RemoteStateChangeContext::
+    ~RemoteStateChangeContext() = default;
+
 // Returns a list of directories that different Chrome channels might use to
 // watch for file changes for firing the onRemoteSessionStateChange event on the
 // extension.
@@ -71,13 +112,18 @@
 //
 // Caller should check if the directory exists before writing files to it. A
 // directory only exists if the corresponding Chrome version is installed.
-std::vector<base::FilePath> GetRemoteStateChangeDirPaths() {
+RemoteWebAuthnExtensionNotifier::RemoteStateChangeContext
+RemoteWebAuthnExtensionNotifier::GetRemoteStateChangeContext() {
 #if BUILDFLAG(IS_LINUX) || BUILDFLAG(IS_WIN) || BUILDFLAG(IS_MAC)
   constexpr base::FilePath::CharType kStateChangeDirName[] =
       FILE_PATH_LITERAL("WebAuthenticationProxyRemoteSessionStateChange");
 #endif
 
-  std::vector<base::FilePath> dirs;
+  RemoteWebAuthnExtensionNotifier::RemoteStateChangeContext context;
+
+#if BUILDFLAG(IS_LINUX) || BUILDFLAG(IS_WIN) || BUILDFLAG(IS_MAC)
+  std::vector<base::FilePath>& dirs = context.dirs;
+#endif
 
 #if BUILDFLAG(IS_LINUX)
   // See: chrome/common/chrome_paths_linux.cc
@@ -111,15 +157,15 @@
   HANDLE user_token = nullptr;
   if (!WTSQueryUserToken(WTS_CURRENT_SESSION, &user_token)) {
     PLOG(ERROR) << "Failed to get current user token";
-    return dirs;
+    return context;
   }
-  base::win::ScopedHandle scoped_user_token(user_token);
+  context.user_token.Set(user_token);
   base::win::ScopedCoMem<wchar_t> local_app_data_path_buf;
   if (!SUCCEEDED(SHGetKnownFolderPath(FOLDERID_LocalAppData, /* dwFlags= */ 0,
-                                      scoped_user_token.get(),
+                                      context.user_token.get(),
                                       &local_app_data_path_buf))) {
     PLOG(ERROR) << "SHGetKnownFolderPath failed";
-    return dirs;
+    return context;
   }
 
   base::FilePath base_path = base::FilePath(local_app_data_path_buf.get());
@@ -144,7 +190,7 @@
   base::FilePath base_path;
   if (!base::PathService::Get(base::DIR_APP_DATA, &base_path)) {
     LOG(ERROR) << "Failed to get app data dir";
-    return dirs;
+    return context;
   }
   base::FilePath base_path_google = base_path.Append("Google");
   dirs.push_back(base_path_google.Append("Chrome").Append(kStateChangeDirName));
@@ -156,16 +202,14 @@
 #else
   NOTIMPLEMENTED();
 #endif
-  return dirs;
+  return context;
 }
 
-}  // namespace
-
 // Core class for writing wakeup files on the IO sequence. Must be used and
 // deleted on the same sequence.
 class RemoteWebAuthnExtensionNotifier::Core final {
  public:
-  explicit Core(std::vector<base::FilePath> remote_state_change_dirs);
+  explicit Core(RemoteStateChangeContext context);
   ~Core();
 
   void WakeUpExtension();
@@ -173,13 +217,12 @@
  private:
   SEQUENCE_CHECKER(sequence_checker_);
 
-  std::vector<base::FilePath> remote_state_change_dirs_;
+  RemoteStateChangeContext context_;
   base::WeakPtrFactory<Core> weak_factory_{this};
 };
 
-RemoteWebAuthnExtensionNotifier::Core::Core(
-    std::vector<base::FilePath> remote_state_change_dirs)
-    : remote_state_change_dirs_(std::move(remote_state_change_dirs)) {}
+RemoteWebAuthnExtensionNotifier::Core::Core(RemoteStateChangeContext context)
+    : context_(std::move(context)) {}
 
 RemoteWebAuthnExtensionNotifier::Core::~Core() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
@@ -188,7 +231,17 @@
 void RemoteWebAuthnExtensionNotifier::Core::WakeUpExtension() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-  for (const base::FilePath& dir : remote_state_change_dirs_) {
+#if BUILDFLAG(IS_WIN)
+  ScopedImpersonation impersonation(context_.user_token.get());
+  if (context_.user_token.is_valid() && !impersonation.is_impersonating()) {
+    PLOG(ERROR) << "Aborting file writes due to impersonation failure.";
+    return;
+  }
+#endif
+
+  for (const base::FilePath& dir : context_.dirs) {
+    // Note: We check DirectoryExists as the user. If they've swapped the
+    // directory for a junction to a protected area, the check will fail.
     if (!base::DirectoryExists(dir)) {
       VLOG(1) << "Ignored non-directory path: " << dir;
       continue;
@@ -196,8 +249,18 @@
     for (const auto& id : GetRemoteWebAuthnExtensionIds()) {
       auto file_path = dir.Append(id);
       VLOG(1) << "Writing extension wakeup file: " << file_path;
+      // We are impersonating the user, so any junction-based redirection will
+      // be restricted by the user's permissions.
       base::File file(file_path,
                       base::File::FLAG_CREATE_ALWAYS | base::File::FLAG_WRITE);

diff --git a/remoting/host/webauthn/remote_webauthn_extension_notifier_unittest.cc b/remoting/host/webauthn/remote_webauthn_extension_notifier_unittest.cc
index 2e318c0..f4f680a 100644
--- a/remoting/host/webauthn/remote_webauthn_extension_notifier_unittest.cc
+++ b/remoting/host/webauthn/remote_webauthn_extension_notifier_unittest.cc
@@ -5,6 +5,7 @@
 #include "remoting/host/webauthn/remote_webauthn_extension_notifier.h"
 
 #include <memory>
+#include <vector>
 
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
@@ -17,6 +18,7 @@
 #include "base/task/thread_pool.h"
 #include "base/test/bind.h"
 #include "base/test/task_environment.h"
+#include "build/build_config.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace remoting {
@@ -71,12 +73,14 @@
   EXPECT_TRUE(scoped_temp_dir_2_.CreateUniqueTempDir());
   io_task_runner_ = base::ThreadPool::CreateSequencedTaskRunner(
       {base::MayBlock(), base::WithBaseSyncPrimitives()});
-  notifier_ = base::WrapUnique(new RemoteWebAuthnExtensionNotifier(
-      {
-          scoped_temp_dir_1_.GetPath(),
-          scoped_temp_dir_2_.GetPath(),
-      },
-      io_task_runner_));
+
+  RemoteWebAuthnExtensionNotifier::RemoteStateChangeContext context;
+  context.dirs = {
+      scoped_temp_dir_1_.GetPath(),
+      scoped_temp_dir_2_.GetPath(),
+  };
+  notifier_ = base::WrapUnique(
+      new RemoteWebAuthnExtensionNotifier(std::move(context), io_task_runner_));
 }
 
 RemoteWebAuthnExtensionNotifierTest::~RemoteWebAuthnExtensionNotifierTest() {