vultr/openclaw
1
0
Fork 0
mirror of https://github.com/openclaw/openclaw.git synced 2026-05-06 14:20:44 +00:00
Code Issues Packages Projects Releases Wiki Activity

Harden Windows command wrapper resolution (#77472)

Browse Source 
* Harden Windows command wrapper resolution

* clawsweeper: route Windows cmd.exe wrapper through getWindowsInstallRoots

Replace the local SystemRoot/windir/SYSTEMROOT/WINDIR scan in
resolveTrustedWindowsCmdExe with the shared getWindowsInstallRoots()
resolver from src/infra/windows-install-roots.ts. The shared resolver
already rejects UNC paths, root-relative values, semicolon-delimited
path-lists, and missing-drive-letter roots, and prefers registry-derived
roots over env, so the wrapper-launch trust boundary now matches the
existing Windows install-root boundary on main.

Tests:
- _resetWindowsInstallRootsForTests in beforeEach so cached roots track
  per-test process.env mutations
- expectedTrustedCmdExe helper now joins the resolved systemRoot, so the
  expected wrapper executable matches the production resolver on Linux
  CI (where it falls back to DEFAULT_WINDOWS_SYSTEM_ROOT)
- new "rejects unsafe Windows root values" test covers UNC,
  semicolon-delimited path-list, root-relative, and bare-relative
  SystemRoot inputs

* Add CHANGELOG entry for #77472 Windows command wrapper hardening

* clawsweeper: stub registry probe in Windows wrapper tests

On real Windows CI runners getWindowsInstallRoots() reads the canonical
SystemRoot from the registry (e.g. C:\WINDOWS) before falling back to
process.env, which shadowed the env-only setup in the ComSpec-poisoning
and unsafe-root tests and produced casing mismatches like
"C:\WINDOWS\System32\cmd.exe" vs the expected "C:\Windows\...". Pass a
queryRegistryValue stub returning null in beforeEach (and inside the
unsafe-root loop) so install-root resolution is fully driven by the
test's process.env setup on every platform.

* clawsweeper: overwrite WINDIR alongside SystemRoot in unsafe-root test

Real Windows runners did not honor `delete process.env.windir`, so the
unsafe-root iteration's WINDIR fallback still resolved to the canonical
`C:\WINDOWS` and produced a casing mismatch against the expected default
`C:\Windows\System32\cmd.exe`. Set both `SystemRoot` and `WINDIR` to the
unsafe payload so every install-root env source is rejected by
`normalizeWindowsInstallRoot` and the resolver falls through to
`DEFAULT_WINDOWS_SYSTEM_ROOT`.
This commit is contained in:
Devin Robison
2026-05-04 14:33:18 -06:00
committed by GitHub
parent 4fab34a63b
commit 982d123b80
3 changed files with 108 additions and 5 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
CHANGELOG.md
View File

@@ -215,6 +215,7 @@ Docs: https://docs.openclaw.ai
- QQBot: preserve the framework command authorization decision when converting framework command contexts into engine slash command contexts, so downstream slash handlers see `commandAuthorized` matching the channel's resolved `isAuthorizedSender` instead of a hardcoded `true`. (#77453) Thanks @drobison00.
- Security/Windows: block `LOCALAPPDATA` from workspace `.env` and resolve Windows update-flow portable Git path prepends from the trusted process-local `LOCALAPPDATA` only, so workspace-supplied values cannot redirect `git` discovery during `openclaw update`. (#77470) Thanks @drobison00.
- Browser/SSRF: enforce the existing current-tab URL navigation policy before tab-scoped debug, export, and read routes (console, page errors, network requests, trace start/stop, response body, screenshot, snapshot, storage, etc.) collect from an already-selected tab, so blocked tabs return a policy error instead of being read first and redacted only at response time. (#75731) Thanks @eleqtrizit.
- Security/Windows: route the `.cmd`/`.bat` process wrapper through the shared Windows install-root resolver instead of `process.env.ComSpec`, so workspace dotenv-blocked `SystemRoot`/`WINDIR` overrides and unsafe values like UNC paths or path-lists cannot redirect `cmd.exe` selection on Windows. (#77472) Thanks @drobison00.
## 2026.5.3-1

10
src/process/exec.ts
View File

@@ -9,6 +9,7 @@ import {
decodeWindowsOutputBuffer,
resolveWindowsConsoleEncoding,
} from "../infra/windows-encoding.js";
import { getWindowsInstallRoots } from "../infra/windows-install-roots.js";
import { logDebug, logError } from "../logger.js";
import { normalizeLowercaseStringOrEmpty } from "../shared/string-coerce.js";
import { resolveCommandStdio } from "./spawn-utils.js";
@@ -45,6 +46,13 @@ function buildCmdExeCommandLine(resolvedCommand: string, args: string[]): string
return [escapeForCmdExe(resolvedCommand), ...args.map(escapeForCmdExe)].join(" ");
}
function resolveTrustedWindowsCmdExe(): string {
if (process.platform !== "win32") {
return "cmd.exe";
}
return path.win32.join(getWindowsInstallRoots().systemRoot, "System32", "cmd.exe");
}
/**
* On Windows, Node 18.20.2+ (CVE-2024-27980) rejects spawning .cmd/.bat directly
* without shell, causing EINVAL. Resolve npm/npx to node + cli script so we
@@ -107,7 +115,7 @@ function resolveChildProcessInvocation(params: {
const useCmdWrapper = isWindowsBatchCommand(resolvedCommand);
return {
command: useCmdWrapper ? (process.env.ComSpec ?? "cmd.exe") : resolvedCommand,
command: useCmdWrapper ? resolveTrustedWindowsCmdExe() : resolvedCommand,
args: useCmdWrapper
? ["/d", "/s", "/c", buildCmdExeCommandLine(resolvedCommand, finalArgv.slice(1))]
: finalArgv.slice(1),

102
src/process/exec.windows.test.ts
View File

@@ -3,6 +3,10 @@ import { EventEmitter } from "node:events";
import fs from "node:fs";
import path from "node:path";
import { afterEach, beforeAll, beforeEach, describe, expect, it, vi } from "vitest";
import {
_resetWindowsInstallRootsForTests,
getWindowsInstallRoots,
} from "../infra/windows-install-roots.js";
const { spawnMock, spawnSyncMock, execFileMock, execFilePromisifyMock } = vi.hoisted(() => {
const execFilePromisifyMock = vi.fn();
@@ -101,6 +105,10 @@ function expectCmdWrappedInvocation(params: {
expect(params.captured[2].windowsVerbatimArguments).toBe(true);
}
function expectedTrustedCmdExe(): string {
return path.win32.join(getWindowsInstallRoots().systemRoot, "System32", "cmd.exe");
}
async function expectShimmedWindowsCommandWithoutExitCodeSucceeds(params?: { killed?: boolean }) {
const platformSpy = vi.spyOn(process, "platform", "get").mockReturnValue("win32");
const child = createMockChild({
@@ -127,6 +135,10 @@ describe("windows command wrapper behavior", () => {
});
beforeEach(() => {
// Stub the registry probe so install-root resolution is fully driven by
// process.env in tests; on real Windows runners the registry returns the
// canonical SystemRoot and would shadow the test's env setup.
_resetWindowsInstallRootsForTests({ queryRegistryValue: () => null });
spawnMock.mockReset();
spawnSyncMock.mockReset();
spawnSyncMock.mockReturnValue({ stdout: "Active code page: 936", stderr: "" });
@@ -157,7 +169,7 @@ describe("windows command wrapper behavior", () => {
it("wraps .cmd commands via cmd.exe in runCommandWithTimeout", async () => {
const platformSpy = vi.spyOn(process, "platform", "get").mockReturnValue("win32");
const expectedComSpec = process.env.ComSpec ?? "cmd.exe";
const expectedComSpec = expectedTrustedCmdExe();
spawnMock.mockImplementation(
(_command: string, _args: string[], _options: Record<string, unknown>) => createMockChild(),
@@ -173,9 +185,91 @@ describe("windows command wrapper behavior", () => {
}
});
it("ignores ComSpec when selecting the Windows command wrapper", async () => {
const platformSpy = vi.spyOn(process, "platform", "get").mockReturnValue("win32");
const previousComSpec = process.env.ComSpec;
const previousSystemRoot = process.env.SystemRoot;
process.env.ComSpec = "C:\\workspace\\evil\\cmd.exe";
process.env.SystemRoot = "C:\\Windows";
spawnMock.mockImplementation(
(_command: string, _args: string[], _options: Record<string, unknown>) => createMockChild(),
);
try {
const result = await runCommandWithTimeout(["pnpm", "--version"], { timeoutMs: 1000 });
expect(result.code).toBe(0);
const captured = spawnMock.mock.calls[0] as SpawnCall | undefined;
expectCmdWrappedInvocation({
captured,
expectedComSpec: path.win32.join("C:\\Windows", "System32", "cmd.exe"),
});
} finally {
if (previousComSpec === undefined) {
delete process.env.ComSpec;
} else {
process.env.ComSpec = previousComSpec;
}
if (previousSystemRoot === undefined) {
delete process.env.SystemRoot;
} else {
process.env.SystemRoot = previousSystemRoot;
}
platformSpy.mockRestore();
}
});
it("rejects unsafe Windows root values when selecting the command wrapper", async () => {
const platformSpy = vi.spyOn(process, "platform", "get").mockReturnValue("win32");
const previousSystemRoot = process.env.SystemRoot;
const previousWindir = process.env.WINDIR;
spawnMock.mockImplementation(
(_command: string, _args: string[], _options: Record<string, unknown>) => createMockChild(),
);
try {
for (const unsafeRoot of [
"\\\\evil\\share",
"C:\\Windows;C:\\evil",
"\\Windows",
"relative\\path",
]) {
_resetWindowsInstallRootsForTests({ queryRegistryValue: () => null });
// Set every install-root env source to the unsafe value so the
// resolver rejects each one and falls through to the safe default.
// Deleting WINDIR here is unreliable on real Windows runners, so
// overwrite it with the same rejected payload.
process.env.SystemRoot = unsafeRoot;
process.env.WINDIR = unsafeRoot;
spawnMock.mockClear();
const result = await runCommandWithTimeout(["pnpm", "--version"], { timeoutMs: 1000 });
expect(result.code).toBe(0);
const captured = spawnMock.mock.calls[0] as SpawnCall | undefined;
expectCmdWrappedInvocation({
captured,
expectedComSpec: path.win32.join("C:\\Windows", "System32", "cmd.exe"),
});
}
} finally {
if (previousSystemRoot === undefined) {
delete process.env.SystemRoot;
} else {
process.env.SystemRoot = previousSystemRoot;
}
if (previousWindir === undefined) {
delete process.env.WINDIR;
} else {
process.env.WINDIR = previousWindir;
}
platformSpy.mockRestore();
}
});
it("wraps corepack.cmd via cmd.exe in runCommandWithTimeout", async () => {
const platformSpy = vi.spyOn(process, "platform", "get").mockReturnValue("win32");
const expectedComSpec = process.env.ComSpec ?? "cmd.exe";
const expectedComSpec = expectedTrustedCmdExe();
spawnMock.mockImplementation(
(_command: string, _args: string[], _options: Record<string, unknown>) => createMockChild(),
@@ -243,7 +337,7 @@ describe("windows command wrapper behavior", () => {
it("falls back to npm.cmd when npm-cli.js is unavailable", async () => {
const platformSpy = vi.spyOn(process, "platform", "get").mockReturnValue("win32");
const existsSpy = vi.spyOn(fs, "existsSync").mockReturnValue(false);
const expectedComSpec = process.env.ComSpec ?? "cmd.exe";
const expectedComSpec = expectedTrustedCmdExe();
spawnMock.mockImplementation(
(_command: string, _args: string[], _options: Record<string, unknown>) => createMockChild(),
@@ -297,7 +391,7 @@ describe("windows command wrapper behavior", () => {
it("uses cmd.exe wrapper with windowsVerbatimArguments in runExec for .cmd shims", async () => {
const platformSpy = vi.spyOn(process, "platform", "get").mockReturnValue("win32");
const expectedComSpec = process.env.ComSpec ?? "cmd.exe";
const expectedComSpec = expectedTrustedCmdExe();
execFileMock.mockImplementation(
(