diff --git a/CHANGELOG.md b/CHANGELOG.md index bd5a0d856b5..f8d3619d49a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ Docs: https://docs.openclaw.ai ### Fixes +- Security/Node Host: enforce `system.run` rawCommand/argv consistency to prevent allowlist/approval bypass. Thanks @christos-eth. - Security/Agents: scope CLI process cleanup to owned child PIDs to avoid killing unrelated processes on shared hosts. Thanks @aether-ai-agent. - Security/Agents (macOS): prevent shell injection when writing Claude CLI keychain credentials. (#15924) Thanks @aether-ai-agent. - Security: fix Chutes manual OAuth login state validation (thanks @aether-ai-agent). (#16058) diff --git a/src/gateway/node-invoke-system-run-approval.ts b/src/gateway/node-invoke-system-run-approval.ts index 4635bfb0143..73e7d83c710 100644 --- a/src/gateway/node-invoke-system-run-approval.ts +++ b/src/gateway/node-invoke-system-run-approval.ts @@ -1,5 +1,9 @@ import type { ExecApprovalManager, ExecApprovalRecord } from "./exec-approval-manager.js"; import type { GatewayClient } from "./server-methods/types.js"; +import { + formatExecCommand, + validateSystemRunCommandConsistency, +} from "../infra/system-run-command.js"; type SystemRunParamsLike = { command?: unknown; @@ -48,7 +52,7 @@ function getCmdText(params: SystemRunParamsLike): string { if (Array.isArray(params.command)) { const parts = params.command.map((v) => String(v)); if (parts.length > 0) { - return parts.join(" "); + return formatExecCommand(parts); } } return ""; @@ -126,6 +130,26 @@ export function sanitizeSystemRunParamsForForwarding(opts: { } const p = obj as SystemRunParamsLike; + const argv = Array.isArray(p.command) ? p.command.map((v) => String(v)) : []; + const raw = normalizeString(p.rawCommand); + if (raw) { + if (!Array.isArray(p.command) || argv.length === 0) { + return { + ok: false, + message: "rawCommand requires params.command", + details: { code: "MISSING_COMMAND" }, + }; + } + const validation = validateSystemRunCommandConsistency({ argv, rawCommand: raw }); + if (!validation.ok) { + return { + ok: false, + message: validation.message, + details: validation.details ?? { code: "RAW_COMMAND_MISMATCH" }, + }; + } + } + const approved = p.approved === true; const requestedDecision = normalizeApprovalDecision(p.approvalDecision); const wantsApprovalOverride = approved || requestedDecision !== null; diff --git a/src/gateway/server.node-invoke-approval-bypass.e2e.test.ts b/src/gateway/server.node-invoke-approval-bypass.e2e.test.ts index 1cc6bda685d..e8103d0e552 100644 --- a/src/gateway/server.node-invoke-approval-bypass.e2e.test.ts +++ b/src/gateway/server.node-invoke-approval-bypass.e2e.test.ts @@ -124,6 +124,41 @@ describe("node.invoke approval bypass", () => { return client; }; + test("rejects rawCommand/command mismatch before forwarding to node", async () => { + let sawInvoke = false; + const node = await connectLinuxNode(() => { + sawInvoke = true; + }); + const ws = await connectOperator(["operator.write"]); + + const nodes = await rpcReq<{ nodes?: Array<{ nodeId: string; connected?: boolean }> }>( + ws, + "node.list", + {}, + ); + expect(nodes.ok).toBe(true); + const nodeId = nodes.payload?.nodes?.find((n) => n.connected)?.nodeId ?? ""; + expect(nodeId).toBeTruthy(); + + const res = await rpcReq(ws, "node.invoke", { + nodeId, + command: "system.run", + params: { + command: ["uname", "-a"], + rawCommand: "echo hi", + }, + idempotencyKey: crypto.randomUUID(), + }); + expect(res.ok).toBe(false); + expect(res.error?.message ?? "").toContain("rawCommand does not match command"); + + await sleep(50); + expect(sawInvoke).toBe(false); + + ws.close(); + node.stop(); + }); + test("rejects injecting approved/approvalDecision without approval id", async () => { let sawInvoke = false; const node = await connectLinuxNode(() => { diff --git a/src/infra/system-run-command.test.ts b/src/infra/system-run-command.test.ts new file mode 100644 index 00000000000..28fa16cec20 --- /dev/null +++ b/src/infra/system-run-command.test.ts @@ -0,0 +1,54 @@ +import { describe, expect, test } from "vitest"; +import { + extractShellCommandFromArgv, + formatExecCommand, + validateSystemRunCommandConsistency, +} from "./system-run-command.js"; + +describe("system run command helpers", () => { + test("formatExecCommand quotes args with spaces", () => { + expect(formatExecCommand(["echo", "hi there"])).toBe('echo "hi there"'); + }); + + test("extractShellCommandFromArgv extracts sh -lc command", () => { + expect(extractShellCommandFromArgv(["/bin/sh", "-lc", "echo hi"])).toBe("echo hi"); + }); + + test("extractShellCommandFromArgv extracts cmd.exe /c command", () => { + expect(extractShellCommandFromArgv(["cmd.exe", "/d", "/s", "/c", "echo hi"])).toBe("echo hi"); + }); + + test("validateSystemRunCommandConsistency accepts rawCommand matching direct argv", () => { + const res = validateSystemRunCommandConsistency({ + argv: ["echo", "hi"], + rawCommand: "echo hi", + }); + expect(res.ok).toBe(true); + if (!res.ok) { + throw new Error("unreachable"); + } + expect(res.shellCommand).toBe(null); + expect(res.cmdText).toBe("echo hi"); + }); + + test("validateSystemRunCommandConsistency rejects mismatched rawCommand vs direct argv", () => { + const res = validateSystemRunCommandConsistency({ + argv: ["uname", "-a"], + rawCommand: "echo hi", + }); + expect(res.ok).toBe(false); + if (res.ok) { + throw new Error("unreachable"); + } + expect(res.message).toContain("rawCommand does not match command"); + expect(res.details?.code).toBe("RAW_COMMAND_MISMATCH"); + }); + + test("validateSystemRunCommandConsistency accepts rawCommand matching sh wrapper argv", () => { + const res = validateSystemRunCommandConsistency({ + argv: ["/bin/sh", "-lc", "echo hi"], + rawCommand: "echo hi", + }); + expect(res.ok).toBe(true); + }); +}); diff --git a/src/infra/system-run-command.ts b/src/infra/system-run-command.ts new file mode 100644 index 00000000000..5ba6e669cba --- /dev/null +++ b/src/infra/system-run-command.ts @@ -0,0 +1,106 @@ +import path from "node:path"; + +export type SystemRunCommandValidation = + | { + ok: true; + shellCommand: string | null; + cmdText: string; + } + | { + ok: false; + message: string; + details?: Record; + }; + +function basenameLower(token: string): string { + const win = path.win32.basename(token); + const posix = path.posix.basename(token); + const base = win.length < posix.length ? win : posix; + return base.trim().toLowerCase(); +} + +export function formatExecCommand(argv: string[]): string { + return argv + .map((arg) => { + const trimmed = arg.trim(); + if (!trimmed) { + return '""'; + } + const needsQuotes = /\s|"/.test(trimmed); + if (!needsQuotes) { + return trimmed; + } + return `"${trimmed.replace(/"/g, '\\"')}"`; + }) + .join(" "); +} + +export function extractShellCommandFromArgv(argv: string[]): string | null { + const token0 = argv[0]?.trim(); + if (!token0) { + return null; + } + + const base0 = basenameLower(token0); + + // POSIX-style shells: sh -lc "" + if ( + base0 === "sh" || + base0 === "bash" || + base0 === "zsh" || + base0 === "dash" || + base0 === "ksh" + ) { + const flag = argv[1]?.trim(); + if (flag !== "-lc" && flag !== "-c") { + return null; + } + const cmd = argv[2]; + return typeof cmd === "string" ? cmd : null; + } + + // Windows cmd.exe: cmd.exe /d /s /c "" + if (base0 === "cmd.exe" || base0 === "cmd") { + const idx = argv.findIndex((item) => String(item).trim().toLowerCase() === "/c"); + if (idx === -1) { + return null; + } + const cmd = argv[idx + 1]; + return typeof cmd === "string" ? cmd : null; + } + + return null; +} + +export function validateSystemRunCommandConsistency(params: { + argv: string[]; + rawCommand?: string | null; +}): SystemRunCommandValidation { + const raw = + typeof params.rawCommand === "string" && params.rawCommand.trim().length > 0 + ? params.rawCommand.trim() + : null; + const shellCommand = extractShellCommandFromArgv(params.argv); + const inferred = shellCommand ? shellCommand.trim() : formatExecCommand(params.argv); + + if (raw && raw !== inferred) { + return { + ok: false, + message: "INVALID_REQUEST: rawCommand does not match command", + details: { + code: "RAW_COMMAND_MISMATCH", + rawCommand: raw, + inferred, + }, + }; + } + + return { + ok: true, + // Only treat this as a shell command when argv is a recognized shell wrapper. + // For direct argv execution, rawCommand is purely display/approval text and + // must match the formatted argv. + shellCommand: shellCommand ? (raw ?? shellCommand) : null, + cmdText: raw ?? shellCommand ?? inferred, + }; +} diff --git a/src/node-host/invoke.ts b/src/node-host/invoke.ts index 5cd9cc326b0..313d66154b0 100644 --- a/src/node-host/invoke.ts +++ b/src/node-host/invoke.ts @@ -31,6 +31,7 @@ import { type ExecHostResponse, type ExecHostRunResult, } from "../infra/exec-host.js"; +import { validateSystemRunCommandConsistency } from "../infra/system-run-command.js"; import { runBrowserProxyCommand } from "./invoke-browser.js"; const OUTPUT_CAP = 200_000; @@ -174,22 +175,6 @@ function sanitizeEnv( return merged; } -function formatCommand(argv: string[]): string { - return argv - .map((arg) => { - const trimmed = arg.trim(); - if (!trimmed) { - return '""'; - } - const needsQuotes = /\s|"/.test(trimmed); - if (!needsQuotes) { - return trimmed; - } - return `"${trimmed.replace(/"/g, '\\"')}"`; - }) - .join(" "); -} - function truncateOutput(raw: string, maxChars: number): { text: string; truncated: boolean } { if (raw.length <= maxChars) { return { text: raw, truncated: false }; @@ -514,7 +499,20 @@ export async function handleInvoke( const argv = params.command.map((item) => String(item)); const rawCommand = typeof params.rawCommand === "string" ? params.rawCommand.trim() : ""; - const cmdText = rawCommand || formatCommand(argv); + const consistency = validateSystemRunCommandConsistency({ + argv, + rawCommand: rawCommand || null, + }); + if (!consistency.ok) { + await sendInvokeResult(client, frame, { + ok: false, + error: { code: "INVALID_REQUEST", message: consistency.message }, + }); + return; + } + + const shellCommand = consistency.shellCommand; + const cmdText = consistency.cmdText; const agentId = params.agentId?.trim() || undefined; const cfg = loadConfig(); const agentExec = agentId ? resolveAgentConfig(cfg, agentId)?.tools?.exec : undefined; @@ -536,9 +534,9 @@ export async function handleInvoke( let allowlistMatches: ExecAllowlistEntry[] = []; let allowlistSatisfied = false; let segments: ExecCommandSegment[] = []; - if (rawCommand) { + if (shellCommand) { const allowlistEval = evaluateShellAllowlist({ - command: rawCommand, + command: shellCommand, allowlist: approvals.allowlist, safeBins, cwd: params.cwd ?? undefined, @@ -569,7 +567,7 @@ export async function handleInvoke( segments = analysis.segments; } const isWindows = process.platform === "win32"; - const cmdInvocation = rawCommand + const cmdInvocation = shellCommand ? isCmdExeInvocation(segments[0]?.argv ?? []) : isCmdExeInvocation(argv); if (security === "allowlist" && isWindows && cmdInvocation) { @@ -585,7 +583,7 @@ export async function handleInvoke( : null; const execRequest: ExecHostRequest = { command: argv, - rawCommand: rawCommand || null, + rawCommand: rawCommand || shellCommand || null, cwd: params.cwd ?? null, env: params.env ?? null, timeoutMs: params.timeoutMs ?? null, @@ -780,7 +778,7 @@ export async function handleInvoke( security === "allowlist" && isWindows && !approvedByAsk && - rawCommand && + shellCommand && analysisOk && allowlistSatisfied && segments.length === 1 &&