From 33ba3ce951e34c1a24889c82a4dffd621a17633a Mon Sep 17 00:00:00 2001 From: Vincent Koc Date: Thu, 12 Mar 2026 13:28:35 -0400 Subject: [PATCH] fix(node-host): harden ambiguous approval operand binding (#44247) * fix(node-host): harden approval operand binding * test(node-host): cover approval parser hardening * docs(changelog): note approval hardening GHSA cluster * Update CHANGELOG.md * fix(node-host): remove dead approval parser entries * test(node-host): cover bunx approval wrapper * fix(node-host): unwrap pnpm shim exec forms * test(node-host): cover pnpm shim wrappers --- CHANGELOG.md | 1 + src/node-host/invoke-system-run-plan.test.ts | 157 ++++++++++++- src/node-host/invoke-system-run-plan.ts | 222 ++++++++++++++++++- 3 files changed, 372 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a3dce87855e..483fd2a1bfe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,6 +44,7 @@ Docs: https://docs.openclaw.ai - Security/Feishu reactions: preserve looked-up group chat typing and fail closed on ambiguous reaction context so group authorization and mention gating cannot be bypassed through synthetic `p2p` reactions. (`GHSA-m69h-jm2f-2pv8`)(#44088) Thanks @zpbrent and @vincentkoc. - Security/LINE webhook: require signatures for empty-event POST probes too so unsigned requests no longer confirm webhook reachability with a `200` response. (`GHSA-mhxh-9pjm-w7q5`)(#44090) Thanks @TerminalsandCoffee and @vincentkoc. - Security/Zalo webhook: rate limit invalid secret guesses before auth so weak webhook secrets cannot be brute-forced through unauthenticated churned requests without pre-auth `429` responses. (`GHSA-5m9r-p9g7-679c`)(#44173) Thanks @zpbrent and @vincentkoc. +- Security/exec approvals: fail closed for ambiguous inline loader and shell-payload script execution, bind the real script after POSIX shell value-taking flags, and unwrap `pnpm`/`npm exec`/`npx` script runners before approval binding. (`GHSA-57jw-9722-6rf2`)(`GHSA-jvqh-rfmh-jh27`)(`GHSA-x7pp-23xv-mmr4`)(`GHSA-jc5j-vg4r-j5jx`)(#44247) Thanks @tdjackey and @vincentkoc. - Doctor/gateway service audit: canonicalize service entrypoint paths before comparing them so symlink-vs-realpath installs no longer trigger false "entrypoint does not match the current install" repair prompts. (#43882) Thanks @ngutman. - Doctor/gateway service audit: earlier groundwork for this fix landed in the superseded #28338 branch. Thanks @realriphub. diff --git a/src/node-host/invoke-system-run-plan.test.ts b/src/node-host/invoke-system-run-plan.test.ts index 3e1736000aa..010e7b5e4ef 100644 --- a/src/node-host/invoke-system-run-plan.test.ts +++ b/src/node-host/invoke-system-run-plan.test.ts @@ -6,6 +6,7 @@ import { formatExecCommand } from "../infra/system-run-command.js"; import { buildSystemRunApprovalPlan, hardenApprovedExecutionPaths, + resolveMutableFileOperandSnapshotSync, } from "./invoke-system-run-plan.js"; type PathTokenSetup = { @@ -94,6 +95,36 @@ function withFakeRuntimeBin(params: { binName: string; run: () => T }): T { } } +function withFakeRuntimeBins(params: { binNames: string[]; run: () => T }): T { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-runtime-bins-")); + const binDir = path.join(tmp, "bin"); + fs.mkdirSync(binDir, { recursive: true }); + for (const binName of params.binNames) { + const runtimePath = + process.platform === "win32" + ? path.join(binDir, `${binName}.cmd`) + : path.join(binDir, binName); + const runtimeBody = + process.platform === "win32" ? "@echo off\r\nexit /b 0\r\n" : "#!/bin/sh\nexit 0\n"; + fs.writeFileSync(runtimePath, runtimeBody, { mode: 0o755 }); + if (process.platform !== "win32") { + fs.chmodSync(runtimePath, 0o755); + } + } + const oldPath = process.env.PATH; + process.env.PATH = `${binDir}${path.delimiter}${oldPath ?? ""}`; + try { + return params.run(); + } finally { + if (oldPath === undefined) { + delete process.env.PATH; + } else { + process.env.PATH = oldPath; + } + fs.rmSync(tmp, { recursive: true, force: true }); + } +} + describe("hardenApprovedExecutionPaths", () => { const cases: HardeningCase[] = [ { @@ -318,16 +349,67 @@ describe("hardenApprovedExecutionPaths", () => { initialBody: 'console.log("SAFE");\n', expectedArgvIndex: 2, }, + { + name: "pnpm exec tsx file", + argv: ["pnpm", "exec", "tsx", "./run.ts"], + scriptName: "run.ts", + initialBody: 'console.log("SAFE");\n', + expectedArgvIndex: 3, + }, + { + name: "pnpm js shim exec tsx file", + argv: ["./pnpm.js", "exec", "tsx", "./run.ts"], + scriptName: "run.ts", + initialBody: 'console.log("SAFE");\n', + expectedArgvIndex: 3, + }, + { + name: "pnpm exec double-dash tsx file", + argv: ["pnpm", "exec", "--", "tsx", "./run.ts"], + scriptName: "run.ts", + initialBody: 'console.log("SAFE");\n', + expectedArgvIndex: 4, + }, + { + name: "npx tsx file", + argv: ["npx", "tsx", "./run.ts"], + scriptName: "run.ts", + initialBody: 'console.log("SAFE");\n', + expectedArgvIndex: 2, + }, + { + name: "bunx tsx file", + argv: ["bunx", "tsx", "./run.ts"], + scriptName: "run.ts", + initialBody: 'console.log("SAFE");\n', + expectedArgvIndex: 2, + }, + { + name: "npm exec tsx file", + argv: ["npm", "exec", "--", "tsx", "./run.ts"], + scriptName: "run.ts", + initialBody: 'console.log("SAFE");\n', + expectedArgvIndex: 4, + }, ]; for (const runtimeCase of mutableOperandCases) { it(`captures mutable ${runtimeCase.name} operands in approval plans`, () => { - withFakeRuntimeBin({ - binName: runtimeCase.binName!, + const binNames = runtimeCase.binName + ? [runtimeCase.binName] + : ["bunx", "pnpm", "npm", "npx", "tsx"]; + withFakeRuntimeBins({ + binNames, run: () => { const tmp = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-approval-script-plan-")); const fixture = createScriptOperandFixture(tmp, runtimeCase); fs.writeFileSync(fixture.scriptPath, fixture.initialBody); + const executablePath = fixture.command[0]; + if (executablePath?.endsWith("pnpm.js")) { + const shimPath = path.join(tmp, "pnpm.js"); + fs.writeFileSync(shimPath, "#!/usr/bin/env node\nconsole.log('shim')\n"); + fs.chmodSync(shimPath, 0o755); + } try { const prepared = buildSystemRunApprovalPlan({ command: fixture.command, @@ -441,4 +523,75 @@ describe("hardenApprovedExecutionPaths", () => { }, }); }); + + it("rejects node inline import operands that cannot be bound to one stable file", () => { + withFakeRuntimeBin({ + binName: "node", + run: () => { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-node-import-inline-")); + try { + fs.writeFileSync(path.join(tmp, "main.mjs"), 'console.log("SAFE")\n'); + fs.writeFileSync(path.join(tmp, "preload.mjs"), 'console.log("SAFE")\n'); + const prepared = buildSystemRunApprovalPlan({ + command: ["node", "--import=./preload.mjs", "./main.mjs"], + cwd: tmp, + }); + expect(prepared).toEqual({ + ok: false, + message: + "SYSTEM_RUN_DENIED: approval cannot safely bind this interpreter/runtime command", + }); + } finally { + fs.rmSync(tmp, { recursive: true, force: true }); + } + }, + }); + }); + + it("rejects shell payloads that hide mutable interpreter scripts", () => { + withFakeRuntimeBin({ + binName: "node", + run: () => { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-inline-shell-node-")); + try { + fs.writeFileSync(path.join(tmp, "run.js"), 'console.log("SAFE")\n'); + const prepared = buildSystemRunApprovalPlan({ + command: ["sh", "-lc", "node ./run.js"], + cwd: tmp, + }); + expect(prepared).toEqual({ + ok: false, + message: + "SYSTEM_RUN_DENIED: approval cannot safely bind this interpreter/runtime command", + }); + } finally { + fs.rmSync(tmp, { recursive: true, force: true }); + } + }, + }); + }); + + it("captures the real shell script operand after value-taking shell flags", () => { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-shell-option-value-")); + try { + const scriptPath = path.join(tmp, "run.sh"); + fs.writeFileSync(scriptPath, "#!/bin/sh\necho SAFE\n"); + fs.writeFileSync(path.join(tmp, "errexit"), "decoy\n"); + const snapshot = resolveMutableFileOperandSnapshotSync({ + argv: ["/bin/bash", "-o", "errexit", "./run.sh"], + cwd: tmp, + shellCommand: null, + }); + expect(snapshot).toEqual({ + ok: true, + snapshot: { + argvIndex: 3, + path: fs.realpathSync(scriptPath), + sha256: expect.any(String), + }, + }); + } finally { + fs.rmSync(tmp, { recursive: true, force: true }); + } + }); }); diff --git a/src/node-host/invoke-system-run-plan.ts b/src/node-host/invoke-system-run-plan.ts index 1b46312c3a1..afcc2963e9d 100644 --- a/src/node-host/invoke-system-run-plan.ts +++ b/src/node-host/invoke-system-run-plan.ts @@ -19,6 +19,7 @@ import { resolveInlineCommandMatch, } from "../infra/shell-inline-command.js"; import { formatExecCommand, resolveSystemRunCommandRequest } from "../infra/system-run-command.js"; +import { splitShellArgs } from "../utils/shell-argv.js"; export type ApprovedCwdSnapshot = { cwd: string; @@ -125,6 +126,47 @@ const DENO_RUN_OPTIONS_WITH_VALUE = new Set([ "-L", ]); +const NODE_OPTIONS_WITH_FILE_VALUE = new Set([ + "-r", + "--experimental-loader", + "--import", + "--loader", + "--require", +]); + +const POSIX_SHELL_OPTIONS_WITH_VALUE = new Set([ + "--init-file", + "--rcfile", + "--startup-script", + "-o", +]); + +const NPM_EXEC_OPTIONS_WITH_VALUE = new Set([ + "--cache", + "--package", + "--prefix", + "--script-shell", + "--userconfig", + "--workspace", + "-p", + "-w", +]); + +const NPM_EXEC_FLAG_OPTIONS = new Set([ + "--no", + "--quiet", + "--ws", + "--workspaces", + "--yes", + "-q", + "-y", +]); + +type FileOperandCollection = { + hits: number[]; + sawOptionValueFile: boolean; +}; + function normalizeString(value: unknown): string | null { if (typeof value !== "string") { return null; @@ -225,10 +267,129 @@ function unwrapArgvForMutableOperand(argv: string[]): { argv: string[]; baseInde current = shellMultiplexerUnwrap.argv; continue; } + const packageManagerUnwrap = unwrapKnownPackageManagerExecInvocation(current); + if (packageManagerUnwrap) { + baseIndex += current.length - packageManagerUnwrap.length; + current = packageManagerUnwrap; + continue; + } return { argv: current, baseIndex }; } } +function unwrapKnownPackageManagerExecInvocation(argv: string[]): string[] | null { + const executable = normalizePackageManagerExecToken(argv[0] ?? ""); + switch (executable) { + case "npm": + return unwrapNpmExecInvocation(argv); + case "npx": + case "bunx": + return unwrapDirectPackageExecInvocation(argv); + case "pnpm": + return unwrapPnpmExecInvocation(argv); + default: + return null; + } +} + +function normalizePackageManagerExecToken(token: string): string { + const normalized = normalizeExecutableToken(token); + if (!normalized) { + return normalized; + } + return normalized.replace(/\.(?:c|m)?js$/i, ""); +} + +function unwrapPnpmExecInvocation(argv: string[]): string[] | null { + let idx = 1; + while (idx < argv.length) { + const token = argv[idx]?.trim() ?? ""; + if (!token) { + idx += 1; + continue; + } + if (token === "--") { + idx += 1; + continue; + } + if (!token.startsWith("-")) { + if (token !== "exec" || idx + 1 >= argv.length) { + return null; + } + const tail = argv.slice(idx + 1); + return tail[0] === "--" ? (tail.length > 1 ? tail.slice(1) : null) : tail; + } + if ((token === "-C" || token === "--dir" || token === "--filter") && !token.includes("=")) { + idx += 2; + continue; + } + idx += 1; + } + return null; +} + +function unwrapDirectPackageExecInvocation(argv: string[]): string[] | null { + let idx = 1; + while (idx < argv.length) { + const token = argv[idx]?.trim() ?? ""; + if (!token) { + idx += 1; + continue; + } + if (!token.startsWith("-")) { + return argv.slice(idx); + } + const [flag] = token.toLowerCase().split("=", 2); + if (flag === "-c" || flag === "--call") { + return null; + } + if (NPM_EXEC_OPTIONS_WITH_VALUE.has(flag)) { + idx += token.includes("=") ? 1 : 2; + continue; + } + if (NPM_EXEC_FLAG_OPTIONS.has(flag)) { + idx += 1; + continue; + } + return null; + } + return null; +} + +function unwrapNpmExecInvocation(argv: string[]): string[] | null { + let idx = 1; + while (idx < argv.length) { + const token = argv[idx]?.trim() ?? ""; + if (!token) { + idx += 1; + continue; + } + if (!token.startsWith("-")) { + if (token !== "exec") { + return null; + } + idx += 1; + break; + } + if ( + (token === "-C" || token === "--prefix" || token === "--userconfig") && + !token.includes("=") + ) { + idx += 2; + continue; + } + idx += 1; + } + if (idx >= argv.length) { + return null; + } + const tail = argv.slice(idx); + if (tail[0] === "--") { + return tail.length > 1 ? tail.slice(1) : null; + } + return unwrapDirectPackageExecInvocation(["npx", ...tail]); +} + function resolvePosixShellScriptOperandIndex(argv: string[]): number | null { if ( resolveInlineCommandMatch(argv, POSIX_INLINE_COMMAND_FLAGS, { @@ -254,6 +415,13 @@ function resolvePosixShellScriptOperandIndex(argv: string[]): number | null { return null; } if (!afterDoubleDash && token.startsWith("-")) { + const [flag] = token.toLowerCase().split("=", 2); + if (POSIX_SHELL_OPTIONS_WITH_VALUE.has(flag)) { + if (!token.includes("=")) { + i += 1; + } + continue; + } continue; } return i; @@ -330,7 +498,8 @@ function collectExistingFileOperandIndexes(params: { argv: string[]; startIndex: number; cwd: string | undefined; -}): number[] { + optionsWithFileValue?: ReadonlySet; +}): FileOperandCollection { let afterDoubleDash = false; const hits: number[] = []; for (let i = params.startIndex; i < params.argv.length; i += 1) { @@ -349,28 +518,45 @@ function collectExistingFileOperandIndexes(params: { continue; } if (token === "-") { - return []; + return { hits: [], sawOptionValueFile: false }; } if (token.startsWith("-")) { + const [flag, inlineValue] = token.split("=", 2); + if (params.optionsWithFileValue?.has(flag.toLowerCase())) { + if (inlineValue && resolvesToExistingFileSync(inlineValue, params.cwd)) { + hits.push(i); + return { hits, sawOptionValueFile: true }; + } + const nextToken = params.argv[i + 1]?.trim() ?? ""; + if (!inlineValue && nextToken && resolvesToExistingFileSync(nextToken, params.cwd)) { + hits.push(i + 1); + return { hits, sawOptionValueFile: true }; + } + } continue; } if (resolvesToExistingFileSync(token, params.cwd)) { hits.push(i); } } - return hits; + return { hits, sawOptionValueFile: false }; } function resolveGenericInterpreterScriptOperandIndex(params: { argv: string[]; cwd: string | undefined; + optionsWithFileValue?: ReadonlySet; }): number | null { - const hits = collectExistingFileOperandIndexes({ + const collection = collectExistingFileOperandIndexes({ argv: params.argv, startIndex: 1, cwd: params.cwd, + optionsWithFileValue: params.optionsWithFileValue, }); - return hits.length === 1 ? hits[0] : null; + if (collection.sawOptionValueFile) { + return null; + } + return collection.hits.length === 1 ? collection.hits[0] : null; } function resolveBunScriptOperandIndex(params: { @@ -462,16 +648,39 @@ function resolveMutableFileOperandIndex(argv: string[], cwd: string | undefined) const genericIndex = resolveGenericInterpreterScriptOperandIndex({ argv: unwrapped.argv, cwd, + optionsWithFileValue: + executable === "node" || executable === "nodejs" ? NODE_OPTIONS_WITH_FILE_VALUE : undefined, }); return genericIndex === null ? null : unwrapped.baseIndex + genericIndex; } +function shellPayloadNeedsStableBinding(shellCommand: string, cwd: string | undefined): boolean { + const argv = splitShellArgs(shellCommand); + if (!argv || argv.length === 0) { + return false; + } + const snapshot = resolveMutableFileOperandSnapshotSync({ + argv, + cwd, + shellCommand: null, + }); + if (!snapshot.ok) { + return true; + } + if (snapshot.snapshot) { + return true; + } + const firstToken = argv[0]?.trim() ?? ""; + return resolvesToExistingFileSync(firstToken, cwd); +} + function requiresStableInterpreterApprovalBindingWithShellCommand(params: { argv: string[]; shellCommand: string | null; + cwd: string | undefined; }): boolean { if (params.shellCommand !== null) { - return false; + return shellPayloadNeedsStableBinding(params.shellCommand, params.cwd); } const unwrapped = unwrapArgvForMutableOperand(params.argv); const executable = normalizeExecutableToken(unwrapped.argv[0] ?? ""); @@ -495,6 +704,7 @@ export function resolveMutableFileOperandSnapshotSync(params: { requiresStableInterpreterApprovalBindingWithShellCommand({ argv: params.argv, shellCommand: params.shellCommand, + cwd: params.cwd, }) ) { return {