From 448fb48e02947647f8ab7d50af1a530c6f050090 Mon Sep 17 00:00:00 2001
From: Yuwei Huang <[email protected]>
Date: Wed, 20 May 2026 10:33:31 -0700
Subject: [PATCH] [remoting][win] Harden remote WebAuthn NMH trust verification

* Use PathService instead of environment variables to query paths.
* Update IsBinaryTrusted to verify that a binary is signed by Google.

Verified: Tweaked code locally and verified that the logic works
properly for verifying the NMH caller (Chrome). Saw the "Signature
verified and publisher pinned for" log. Could not verify mojo caller
trust because the caller (e.g. desktop process) is unsigned, but it
should work as expected. I'll try ToT official build once CL is
submitted.

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

diff --git a/remoting/host/webauthn/remote_webauthn_caller_security_utils.cc b/remoting/host/webauthn/remote_webauthn_caller_security_utils.cc
index dac24cc..851d59d 100644
--- a/remoting/host/webauthn/remote_webauthn_caller_security_utils.cc
+++ b/remoting/host/webauthn/remote_webauthn_caller_security_utils.cc
@@ -6,12 +6,11 @@
 
 #include <optional>
 
-#include "base/environment.h"
+#include "base/base_paths.h"
 #include "base/logging.h"
 #include "base/notimplemented.h"
+#include "base/path_service.h"
 #include "base/process/process_handle.h"
-#include "base/strings/cstring_view.h"
-#include "base/strings/utf_string_conversions.h"
 #include "build/build_config.h"
 
 #if BUILDFLAG(IS_LINUX) || BUILDFLAG(IS_WIN)
@@ -60,21 +59,14 @@
 
 #elif BUILDFLAG(IS_WIN)
 
-// Names of environment variables that store the path to directories where apps
-// are installed.
-constexpr auto kAppsDirectoryEnvVars =
-    base::MakeFixedFlatSet<base::cstring_view>({
-        "PROGRAMFILES",
-
-        // May happen if Chrome is upgraded from a 32-bit version.
-        "PROGRAMFILES(X86)",
-
-        // Refers to "C:\Program Files" if current process is 32-bit.
-        "ProgramW6432",
-
-        // For per-user installations.
-        "LOCALAPPDATA",
-    });
+// Keys for base::PathService to retrieve paths to directories where apps are
+// installed.
+constexpr int kAppsDirectoryPathKeys[] = {
+    base::DIR_PROGRAM_FILES,
+    base::DIR_PROGRAM_FILESX86,
+    base::DIR_PROGRAM_FILES6432,
+    base::DIR_LOCAL_APP_DATA,
+};
 
 // Relative to the Program Files directory.
 constexpr auto kAllowedCallerPrograms =
@@ -85,6 +77,39 @@
         L"Google\\Chrome Dev\\Application\\chrome.exe",
     });
 
+// Helper to verify that the cmd.exe binary is located in a trusted system
+// directory.
+bool IsSystemCmd(const base::FilePath& process_image_path) {
+  if (!base::FilePath::CompareEqualIgnoreCase(
+          process_image_path.BaseName().value(), L"cmd.exe")) {
+    return false;
+  }
+
+  base::FilePath process_dir = process_image_path.DirName();
+
+  base::FilePath system_dir;
+  if (base::PathService::Get(base::DIR_SYSTEM, &system_dir)) {
+    if (base::FilePath::CompareEqualIgnoreCase(process_dir.value(),
+                                               system_dir.value())) {
+      return true;
+    }
+  }
+
+  // Also check for the native system directory if it's different (e.g.
+  // SysWOW64 vs System32).
+  base::FilePath windows_dir;
+  if (base::PathService::Get(base::DIR_WINDOWS, &windows_dir)) {
+    if (base::FilePath::CompareEqualIgnoreCase(
+            process_dir.value(), windows_dir.Append(L"System32").value()) ||
+        base::FilePath::CompareEqualIgnoreCase(
+            process_dir.value(), windows_dir.Append(L"SysWOW64").value())) {
+      return true;
+    }
+  }
+
+  return false;
+}
+
 #elif BUILDFLAG(IS_MAC)
 
 constexpr auto kAllowedIdentifiers = std::to_array<const std::string_view>(
@@ -109,8 +134,6 @@
   base::FilePath parent_image_path = GetProcessImagePath(parent_pid);
   return kAllowedCallerPrograms.contains(parent_image_path.value());
 #elif BUILDFLAG(IS_WIN)
-  auto environment = base::Environment::Create();
-
   base::ProcessId parent_pid =
       base::GetParentProcessId(base::GetCurrentProcessHandle());
   base::FilePath parent_image_path = GetProcessImagePath(parent_pid);
@@ -122,36 +145,28 @@
   // that's the case. It's possible to do stdio communications without cmd, so
   // we don't require the parent to always be cmd.
 
-  // COMSPEC is generally "C:\WINDOWS\system32\cmd.exe". Note that the casing
-  // does not match the actual file path's casing.
-  std::optional<std::string> comspec_utf8 = environment->GetVar("COMSPEC");
-  if (comspec_utf8.has_value()) {
-    base::FilePath::StringType comspec = base::UTF8ToWide(comspec_utf8.value());
-    if (base::FilePath::CompareEqualIgnoreCase(parent_image_path.value(),
-                                               comspec)) {
-      // Skip to the grandparent.
-      base::win::ScopedHandle parent_handle(
-          OpenProcess(PROCESS_QUERY_LIMITED_INFORMATION, false, parent_pid));
-      if (parent_handle.is_valid()) {
-        parent_pid = base::GetParentProcessId(parent_handle.Get());
-        parent_image_path = GetProcessImagePath(parent_pid);
-      } else {
-        PLOG(ERROR) << "Failed to query parent info.";
-      }
+  // Use IsSystemCmd() for comparison as a 64-bit Chrome launches a 64-bit
+  // cmd.exe which is in C:\Windows\System32, but for a 32-bit native messaging
+  // host, C:\Windows\System32 will be redirected to C:\Windows\SysWOW64, so we
+  // perform case-insensitive directory checks.
+  if (IsSystemCmd(parent_image_path)) {
+    // Skip to the grandparent.
+    base::win::ScopedHandle parent_handle(
+        OpenProcess(PROCESS_QUERY_LIMITED_INFORMATION, false, parent_pid));
+    if (parent_handle.is_valid()) {
+      parent_pid = base::GetParentProcessId(parent_handle.Get());
+      parent_image_path = GetProcessImagePath(parent_pid);
+    } else {
+      PLOG(ERROR) << "Failed to query parent info.";
     }
-  } else {
-    LOG(ERROR) << "COMSPEC is not set";
   }
 
   // Check if the caller's image path is allowlisted.
-  for (base::cstring_view apps_dir_env_var : kAppsDirectoryEnvVars) {
-    std::optional<std::string> apps_dir_path_utf8 =
-        environment->GetVar(apps_dir_env_var);
-    if (!apps_dir_path_utf8.has_value()) {
+  for (int path_key : kAppsDirectoryPathKeys) {
+    base::FilePath apps_dir_path;
+    if (!base::PathService::Get(path_key, &apps_dir_path)) {
       continue;
     }
-    auto apps_dir_path =
-        base::FilePath::FromUTF8Unsafe(apps_dir_path_utf8.value());
     if (!apps_dir_path.IsParent(parent_image_path)) {
       continue;
     }
diff --git a/remoting/host/win/trust_util.cc b/remoting/host/win/trust_util.cc
index 93af48a..e73a4ed0 100644
--- a/remoting/host/win/trust_util.cc
+++ b/remoting/host/win/trust_util.cc
@@ -9,10 +9,73 @@
 #include <softpub.h>
 #include <wintrust.h>
 
+#include <string>
+
 #include "base/logging.h"
+#include "base/strings/string_util.h"
+
+#if defined(OFFICIAL_BUILD)
+#include <wincrypt.h>
+#endif
 
 namespace remoting {
 
+namespace {
+
+#if defined(OFFICIAL_BUILD)
+constexpr wchar_t kGooglePublisherName[] = L"Google LLC";
+
+bool IsBinarySignedByGoogle(HANDLE verify_trust_state_data) {