diff --git a/CHANGELOG.md b/CHANGELOG.md index 12a4a0f9c13..7177cabbb8f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -200,6 +200,7 @@ Docs: https://docs.openclaw.ai - Direct APNs: route direct HTTP/2 delivery through the active managed proxy with redacted proxy diagnostics, so push requests honor configured egress controls and `openclaw proxy validate --apns-reachable` can prove APNs is reachable through the proxy before deployment. (#74905) Thanks @jesse-merhi. - Agents/subagents: detect prefix-only completion announce replies and fall back to the captured child result so requester chats no longer lose most of long sub-agent reports silently. Fixes #76412. Thanks @inxaos and @davemorin. - TUI: replace the stale-response watchdog notice with plain user-facing copy so stalled replies no longer surface backend or streaming internals. (#77120) Thanks @davemorin. +- Security/Windows: validate `SystemRoot`/`WINDIR` env values through the Windows install-root validator and add them to the dangerous-host-env policy when resolving `icacls.exe`/`whoami.exe` for `openclaw security audit`, so workspace `.env` overrides and bare command names cannot redirect Windows ACL helpers to attacker-controlled binaries. (#74458) Thanks @mmaps. ## 2026.5.3-1 diff --git a/apps/macos/Sources/OpenClaw/HostEnvSecurityPolicy.generated.swift b/apps/macos/Sources/OpenClaw/HostEnvSecurityPolicy.generated.swift index d66b3f0920c..1750d3d03d3 100644 --- a/apps/macos/Sources/OpenClaw/HostEnvSecurityPolicy.generated.swift +++ b/apps/macos/Sources/OpenClaw/HostEnvSecurityPolicy.generated.swift @@ -425,6 +425,7 @@ enum HostEnvSecurityPolicy { "SSL_CERT_DIR", "SSL_CERT_FILE", "SUDO_EDITOR", + "SYSTEMROOT", "TF_CLI_CONFIG_FILE", "TF_PLUGIN_CACHE_DIR", "UV_DEFAULT_INDEX", @@ -435,6 +436,7 @@ enum HostEnvSecurityPolicy { "VIRTUAL_ENV", "VISUAL", "WGETRC", + "WINDIR", "XDG_CONFIG_DIRS", "XDG_CONFIG_HOME", "YARN_RC_FILENAME", diff --git a/src/infra/dotenv.ts b/src/infra/dotenv.ts index 5b06b2c6996..d3ac84122b2 100644 --- 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. diff --git a/src/infra/host-env-security-policy.json b/src/infra/host-env-security-policy.json index fdfc7ebe4fe..e1eedafbd4d 100644 --- 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_"], diff --git a/src/infra/host-env-security.reported-baseline.json b/src/infra/host-env-security.reported-baseline.json index 6d877e9bb74..2fed60ecbd3 100644 --- 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 } diff --git a/src/infra/host-env-security.reported-baseline.test.ts b/src/infra/host-env-security.reported-baseline.test.ts index 261abec09c7..99a818e265f 100644 --- 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 = { 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, ); diff --git a/src/infra/host-env-security.test.ts b/src/infra/host-env-security.test.ts index 5387157fddd..39ef0c24201 100644 --- 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); }); diff --git a/src/infra/windows-install-roots.ts b/src/infra/windows-install-roots.ts index b6344bed190..2c580d87028 100644 --- 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): 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")) ?? diff --git a/src/security/windows-acl.test.ts b/src/security/windows-acl.test.ts index 5e530327375..a0e5d496bd7 100644 --- 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`); }); diff --git a/src/security/windows-acl.ts b/src/security/windows-acl.ts index af763b81144..03f739a93c6 100644 --- 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 { } 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), };