fix(security): block workspace env from overriding Windows system root paths [AI] (#74458)

* fix: address issue * fix: address PR review feedback * fix: address PR review feedback * fix: address PR review feedback * fix: address codex review feedback * fix: address codex review feedback * changelog: PR #74458 --------- Co-authored-by: Devin Robison <drobison@nvidia.com>
2026-05-06 18:30:44 +00:00 · 2026-05-04 12:13:50 -04:00
parent 1df2ac442a
commit c1da0ddd54
10 changed files with 161 additions and 25 deletions
--- a/src/infra/dotenv.ts
+++ b/src/infra/dotenv.ts
@@ -84,9 +84,7 @@ const BLOCKED_WORKSPACE_DOTENV_KEYS = new Set([
  "STATE_DIRECTORY",
  "SYNOLOGY_CHAT_INCOMING_URL",
  "SYNOLOGY_NAS_HOST",
-  "SYSTEMROOT",
  "UV_PYTHON",
-  "WINDIR",
 ]);

 // Block endpoint redirection for any service without overfitting per-provider names.
--- a/src/infra/host-env-security-policy.json
+++ b/src/infra/host-env-security-policy.json
@@ -233,7 +233,9 @@
    "NODE_AUTH_TOKEN",
    "NPM_TOKEN",
    "REDIS_URL",
-    "SSH_AUTH_SOCK"
+    "SSH_AUTH_SOCK",
+    "SYSTEMROOT",
+    "WINDIR"
  ],
  "allowedInheritedOverrideOnlyKeys": [
    "ALL_PROXY",
@@ -263,6 +265,8 @@
    "SSH_AUTH_SOCK",
    "SSL_CERT_DIR",
    "SSL_CERT_FILE",
+    "SYSTEMROOT",
+    "WINDIR",
    "ZDOTDIR"
  ],
  "blockedOverridePrefixes": ["GIT_CONFIG_", "NPM_CONFIG_", "CARGO_REGISTRIES_", "TF_VAR_"],
--- a/src/infra/host-env-security.reported-baseline.json
+++ b/src/infra/host-env-security.reported-baseline.json
@@ -222,6 +222,7 @@
    "SSL_CERT_DIR",
    "SSL_CERT_FILE",
    "SUDO_EDITOR",
+    "SYSTEMROOT",
    "TF_CLI_CONFIG_FILE",
    "TF_PLUGIN_CACHE_DIR",
    "UV_DEFAULT_INDEX",
@@ -232,10 +233,11 @@
    "VIRTUAL_ENV",
    "VISUAL",
    "WGETRC",
+    "WINDIR",
    "XDG_CONFIG_DIRS",
    "XDG_CONFIG_HOME",
    "YARN_RC_FILENAME",
    "ZDOTDIR"
  ],
-  "expectedTotalReportedEntries": 232
+  "expectedTotalReportedEntries": 234
 }
--- a/src/infra/host-env-security.reported-baseline.test.ts
+++ b/src/infra/host-env-security.reported-baseline.test.ts
@@ -47,6 +47,8 @@ const INHERITED_ALLOWLIST_RATIONALE: Record<string, string> = {
  SSH_AUTH_SOCK: "Trusted inherited SSH agent socket from operator runtime.",
  SSL_CERT_DIR: "Trusted inherited OpenSSL certificate directory path.",
  SSL_CERT_FILE: "Trusted inherited OpenSSL certificate file path.",
+  SYSTEMROOT: "Trusted inherited Windows system root selected by the host OS.",
+  WINDIR: "Trusted inherited Windows directory selected by the host OS.",
  ZDOTDIR: "Trusted inherited shell startup directory boundary.",
 };

@@ -83,7 +85,7 @@ describe("host env reported baseline coverage", () => {
      baseline.reportedDangerousEverywhereKeys.length +
        baseline.reportedDangerousOverrideOnlyKeys.length,
    ).toBe(baseline.expectedTotalReportedEntries);
-    expect(baseline.expectedTotalReportedEntries).toBe(232);
+    expect(baseline.expectedTotalReportedEntries).toBe(234);
    expect(sortUniqueUpper(baseline.reportedDangerousEverywhereKeys)).toEqual(
      baseline.reportedDangerousEverywhereKeys,
    );
--- a/src/infra/host-env-security.test.ts
+++ b/src/infra/host-env-security.test.ts
@@ -837,6 +837,8 @@ describe("isDangerousHostEnvOverrideVarName", () => {
    expect(isDangerousHostEnvOverrideVarName("AWS_CONFIG_FILE")).toBe(true);
    expect(isDangerousHostEnvOverrideVarName("aws_config_file")).toBe(true);
    expect(isDangerousHostEnvOverrideVarName("yarn_rc_filename")).toBe(true);
+    expect(isDangerousHostEnvOverrideVarName("SystemRoot")).toBe(true);
+    expect(isDangerousHostEnvOverrideVarName("windir")).toBe(true);
    expect(isDangerousHostEnvOverrideVarName("BASH_ENV")).toBe(false);
    expect(isDangerousHostEnvOverrideVarName("FOO")).toBe(false);
  });
--- a/src/infra/windows-install-roots.ts
+++ b/src/infra/windows-install-roots.ts
@@ -3,7 +3,7 @@ import fs from "node:fs";
 import path from "node:path";
 import { normalizeLowercaseStringOrEmpty } from "../shared/string-coerce.js";

-const DEFAULT_SYSTEM_ROOT = "C:\\Windows";
+export const DEFAULT_WINDOWS_SYSTEM_ROOT = "C:\\Windows";
 const DEFAULT_PROGRAM_FILES = "C:\\Program Files";
 const DEFAULT_PROGRAM_FILES_X86 = "C:\\Program Files (x86)";
 const WINDOWS_NT_CURRENT_VERSION_KEY = "HKLM\\SOFTWARE\\Microsoft\\Windows NT\\CurrentVersion";
@@ -98,7 +98,7 @@ function getWindowsRegExeCandidates(env: Record<string, string | undefined>): re
  for (const root of [
    normalizeWindowsInstallRoot(getEnvValueCaseInsensitive(env, "SystemRoot")),
    normalizeWindowsInstallRoot(getEnvValueCaseInsensitive(env, "WINDIR")),
-    DEFAULT_SYSTEM_ROOT,
+    DEFAULT_WINDOWS_SYSTEM_ROOT,
  ]) {
    if (!root) {
      continue;
@@ -206,7 +206,7 @@ function buildWindowsInstallRoots(
      registryRoots.systemRoot ??
      normalizeWindowsInstallRoot(getEnvValueCaseInsensitive(env, "SystemRoot")) ??
      normalizeWindowsInstallRoot(getEnvValueCaseInsensitive(env, "WINDIR")) ??
-      DEFAULT_SYSTEM_ROOT,
+      DEFAULT_WINDOWS_SYSTEM_ROOT,
    programFiles:
      registryRoots.programFiles ??
      normalizeWindowsInstallRoot(getEnvValueCaseInsensitive(env, "ProgramFiles")) ??
--- a/src/security/windows-acl.test.ts
+++ b/src/security/windows-acl.test.ts
@@ -1,9 +1,15 @@
 import { beforeAll, beforeEach, describe, expect, it, vi } from "vitest";
+import {
+  DEFAULT_WINDOWS_SYSTEM_ROOT,
+  _resetWindowsInstallRootsForTests,
+} from "../infra/windows-install-roots.js";
 import type { WindowsAclEntry, WindowsAclSummary } from "./windows-acl.js";

 const MOCK_USERNAME = "MockUser";
 const mockUserInfo = () => ({ username: MOCK_USERNAME });
 const emptyUserInfo = () => ({ username: "" });
+const DEFAULT_ICACLS = `${DEFAULT_WINDOWS_SYSTEM_ROOT}\\System32\\icacls.exe`;
+const DEFAULT_WHOAMI = `${DEFAULT_WINDOWS_SYSTEM_ROOT}\\System32\\whoami.exe`;

 let createIcaclsResetCommand: typeof import("./windows-acl.js").createIcaclsResetCommand;
 let formatIcaclsResetCommand: typeof import("./windows-acl.js").formatIcaclsResetCommand;
@@ -27,6 +33,7 @@ beforeAll(async () => {

 beforeEach(() => {
  vi.unstubAllEnvs();
+  _resetWindowsInstallRootsForTests();
 });

 function aclEntry(params: {
@@ -420,11 +427,32 @@ Successfully processed 1 files`;

      const result = await inspectWindowsAcl("C:\\test\\file.txt", {
        exec: mockExec,
+        env: {},
      });
      expectInspectSuccess(result, 2);
      // /sid is passed so that account names are printed as SIDs, making the
      // audit locale-independent (fixes #35834).
-      expect(mockExec).toHaveBeenCalledWith("icacls.exe", ["C:\\test\\file.txt", "/sid"]);
+      expect(mockExec).toHaveBeenCalledWith(DEFAULT_ICACLS, ["C:\\test\\file.txt", "/sid"]);
+    });
+
+    it("uses the discovered process SystemRoot when env options are omitted", async () => {
+      _resetWindowsInstallRootsForTests({ queryRegistryValue: () => null });
+      vi.stubEnv("SystemRoot", "D:\\Windows");
+
+      const mockExec = vi.fn().mockResolvedValue({
+        stdout: "C:\\test\\file.txt *S-1-5-18:(F)",
+        stderr: "",
+      });
+
+      const result = await inspectWindowsAcl("C:\\test\\file.txt", {
+        exec: mockExec,
+      });
+
+      expectInspectSuccess(result, 1);
+      expect(mockExec).toHaveBeenCalledWith("D:\\Windows\\System32\\icacls.exe", [
+        "C:\\test\\file.txt",
+        "/sid",
+      ]);
    });

    it("classifies *S-1-5-18 (SID form of SYSTEM from /sid) as trusted", async () => {
@@ -469,8 +497,8 @@ Successfully processed 1 files`;
      expectInspectSuccess(result, 2);
      expect(result.trusted).toHaveLength(2);
      expect(result.untrustedGroup).toHaveLength(0);
-      expect(mockExec).toHaveBeenNthCalledWith(1, "icacls.exe", ["C:\\test\\file.txt", "/sid"]);
-      expect(mockExec).toHaveBeenNthCalledWith(2, "whoami.exe", ["/user", "/fo", "csv", "/nh"]);
+      expect(mockExec).toHaveBeenNthCalledWith(1, DEFAULT_ICACLS, ["C:\\test\\file.txt", "/sid"]);
+      expect(mockExec).toHaveBeenNthCalledWith(2, DEFAULT_WHOAMI, ["/user", "/fo", "csv", "/nh"]);
    });

    it("returns error state on exec failure", async () => {
@@ -533,21 +561,82 @@ Successfully processed 1 files`;

      const result = await inspectWindowsAcl("C:\\test\\file.txt", {
        exec: mockExec,
-        env: { SystemRoot: "C:\\Windows" },
+        env: { SystemRoot: "D:\\Windows" },
      });

      expectInspectSuccess(result, 1);
-      expect(mockExec).toHaveBeenNthCalledWith(1, "C:\\Windows\\System32\\icacls.exe", [
+      expect(mockExec).toHaveBeenNthCalledWith(1, "D:\\Windows\\System32\\icacls.exe", [
        "C:\\test\\file.txt",
        "/sid",
      ]);
-      expect(mockExec).toHaveBeenNthCalledWith(2, "C:\\Windows\\System32\\whoami.exe", [
+      expect(mockExec).toHaveBeenNthCalledWith(2, "D:\\Windows\\System32\\whoami.exe", [
        "/user",
        "/fo",
        "csv",
        "/nh",
      ]);
    });
+
+    it.each([
+      ["systemroot", "D:\\Windows"],
+      ["windir", "E:\\Windows"],
+    ])("resolves explicit env key %s case-insensitively", async (key, root) => {
+      const mockExec = vi.fn().mockResolvedValueOnce({
+        stdout: "C:\\test\\file.txt *S-1-5-18:(F)",
+        stderr: "",
+      });
+
+      const result = await inspectWindowsAcl("C:\\test\\file.txt", {
+        exec: mockExec,
+        env: { [key]: root },
+      });
+
+      expectInspectSuccess(result, 1);
+      expect(mockExec).toHaveBeenCalledWith(`${root}\\System32\\icacls.exe`, [
+        "C:\\test\\file.txt",
+        "/sid",
+      ]);
+    });
+
+    it("does not resolve Windows system commands through a relative SystemRoot", async () => {
+      const mockExec = vi
+        .fn()
+        .mockResolvedValueOnce({
+          stdout: "C:\\test\\file.txt *S-1-5-21-111-222-333-1001:(F)",
+          stderr: "",
+        })
+        .mockResolvedValueOnce({
+          stdout: '"mock-host\\\\MockUser","S-1-5-21-111-222-333-1001"\r\n',
+          stderr: "",
+        });
+
+      const result = await inspectWindowsAcl("C:\\test\\file.txt", {
+        exec: mockExec,
+        env: { SystemRoot: ".\\fake-root" },
+      });
+
+      expectInspectSuccess(result, 1);
+      expect(mockExec).toHaveBeenNthCalledWith(1, DEFAULT_ICACLS, ["C:\\test\\file.txt", "/sid"]);
+      expect(mockExec).toHaveBeenNthCalledWith(2, DEFAULT_WHOAMI, ["/user", "/fo", "csv", "/nh"]);
+    });
+
+    it("uses a valid WINDIR when SystemRoot is invalid", async () => {
+      const mockExec = vi.fn().mockResolvedValueOnce({
+        stdout: "C:\\test\\file.txt *S-1-5-18:(F)",
+        stderr: "",
+      });
+
+      const result = await inspectWindowsAcl("C:\\test\\file.txt", {
+        exec: mockExec,
+        env: { SystemRoot: ".\\fake-root", WINDIR: "E:\\Windows" },
+      });
+
+      expectInspectSuccess(result, 1);
+      expect(mockExec).toHaveBeenCalledWith("E:\\Windows\\System32\\icacls.exe", [
+        "C:\\test\\file.txt",
+        "/sid",
+      ]);
+    });
  });

  describe("formatWindowsAclSummary", () => {
@@ -619,7 +708,15 @@ Successfully processed 1 files`;
        env,
      });
      expect(result).toBe(
-        'icacls "C:\\test\\file.txt" /inheritance:r /grant:r "WORKGROUP\\TestUser:F" /grant:r "*S-1-5-18:F"',
+        [
+          DEFAULT_ICACLS,
+          '"C:\\test\\file.txt"',
+          "/inheritance:r",
+          "/grant:r",
+          '"WORKGROUP\\TestUser:F"',
+          "/grant:r",
+          '"*S-1-5-18:F"',
+        ].join(" "),
      );
    });

@@ -632,6 +729,15 @@ Successfully processed 1 files`;
      expect(result).toContain("(OI)(CI)F");
    });

+    it("uses a validated SystemRoot in the display command", () => {
+      const result = formatIcaclsResetCommand("C:\\test\\file.txt", {
+        isDir: false,
+        env: { SystemRoot: "D:\\Windows", USERNAME: "TestUser" },
+      });
+
+      expect(result.startsWith("D:\\Windows\\System32\\icacls.exe ")).toBe(true);
+    });
+
    it("uses system username when env is empty (falls back to os.userInfo)", () => {
      // When env is empty, resolveWindowsUserPrincipal falls back to os.userInfo().username
      const result = formatIcaclsResetCommand("C:\\test\\file.txt", {
@@ -653,11 +759,20 @@ Successfully processed 1 files`;
        env,
      });
      expect(result).not.toBeNull();
-      expect(result?.command).toBe("icacls");
+      expect(result?.command).toBe(DEFAULT_ICACLS);
      expect(result?.args).toContain("C:\\test\\file.txt");
      expect(result?.args).toContain("/inheritance:r");
    });

+    it("uses a validated SystemRoot for the structured command executable", () => {
+      const result = createIcaclsResetCommand("C:\\test\\file.txt", {
+        isDir: false,
+        env: { SystemRoot: "D:\\Windows", USERNAME: "TestUser" },
+      });
+
+      expect(result?.command).toBe("D:\\Windows\\System32\\icacls.exe");
+    });
+
    it("returns command with system username when env is empty (falls back to os.userInfo)", () => {
      // When env is empty, resolveWindowsUserPrincipal falls back to os.userInfo().username
      const result = createIcaclsResetCommand("C:\\test\\file.txt", {
@@ -667,7 +782,7 @@ Successfully processed 1 files`;
      });
      // Should return a valid command using the system username
      expect(result).not.toBeNull();
-      expect(result?.command).toBe("icacls");
+      expect(result?.command).toBe(DEFAULT_ICACLS);
      expect(result?.args).toContain(`${MOCK_USERNAME}:F`);
    });

--- a/src/security/windows-acl.ts
+++ b/src/security/windows-acl.ts
@@ -1,5 +1,6 @@
 import os from "node:os";
 import path from "node:path";
+import { getWindowsInstallRoots } from "../infra/windows-install-roots.js";
 import { createSubsystemLogger } from "../logging/subsystem.js";
 import { runExec } from "../process/exec.js";
 import { normalizeLowercaseStringOrEmpty } from "../shared/string-coerce.js";
@@ -117,12 +118,10 @@ function buildTrustedPrincipals(env?: NodeJS.ProcessEnv): Set<string> {
 }

 function resolveWindowsSystemCommand(command: string, env?: NodeJS.ProcessEnv): string {
-  const root =
-    env?.SystemRoot?.trim() ||
-    env?.SYSTEMROOT?.trim() ||
-    env?.windir?.trim() ||
-    env?.WINDIR?.trim();
-  return root ? path.win32.join(root, "System32", command) : command;
+  // Never fall back to a bare helper name here; Windows command search can
+  // consult the current directory and PATH before the real System32 helper.
+  const root = getWindowsInstallRoots(env ?? process.env).systemRoot;
+  return path.win32.join(root, "System32", command);
 }

 function classifyPrincipal(
@@ -375,9 +374,20 @@ export function formatIcaclsResetCommand(
  targetPath: string,
  opts: IcaclsResetCommandOptions,
 ): string {
+  const command = resolveWindowsSystemCommand("icacls.exe", opts.env);
  const user = resolveWindowsUserPrincipal(opts.env, opts.userInfo) ?? "%USERNAME%";
  const grant = opts.isDir ? "(OI)(CI)F" : "F";
-  return `icacls "${targetPath}" /inheritance:r /grant:r "${user}:${grant}" /grant:r "*S-1-5-18:${grant}"`;
+  // Quoted executable paths need shell-specific handling in PowerShell; keep
+  // the resolved System32 helper as the command token and quote only arguments.
+  return [
+    command,
+    `"${targetPath}"`,
+    "/inheritance:r",
+    "/grant:r",
+    `"${user}:${grant}"`,
+    "/grant:r",
+    `"*S-1-5-18:${grant}"`,
+  ].join(" ");
 }

 export function createIcaclsResetCommand(
@@ -398,7 +408,7 @@ export function createIcaclsResetCommand(
    `*S-1-5-18:${grant}`,
  ];
  return {
-    command: "icacls",
+    command: resolveWindowsSystemCommand("icacls.exe", opts.env),
    args,
    display: formatIcaclsResetCommand(targetPath, opts),
  };