diff --git a/CHANGELOG.md b/CHANGELOG.md index b8f37dccdab..e234bcdc434 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ Docs: https://docs.openclaw.ai ### Fixes +- fix: block side-effecting command wrappers [AI]. (#87292) Thanks @pgondhi987. - Block unsafe Node runtime env overrides [AI]. (#87308) Thanks @pgondhi987. - Telegram: route `sendMessage` action replies through durable outbound delivery so completed agent responses remain retryable when the gateway send path times out. (#87261) Thanks @mbelinky. - Gateway/security: require `operator.admin` for node and other non-operator device-role pairing approvals, including trusted-proxy sessions, while keeping pairing-only approvals available for operator-role requests. (#87146) diff --git a/src/infra/dispatch-wrapper-resolution.ts b/src/infra/dispatch-wrapper-resolution.ts index 5ba37f75ae6..fc93552fc5a 100644 --- a/src/infra/dispatch-wrapper-resolution.ts +++ b/src/infra/dispatch-wrapper-resolution.ts @@ -235,12 +235,44 @@ function unwrapTimeInvocation(argv: string[]): string[] | null { }); } +function timeInvocationWritesOutputFile(argv: string[]): boolean { + let expectsOptionValue = false; + for (let idx = 1; idx < argv.length; idx += 1) { + const token = argv[idx]?.trim() ?? ""; + if (!token) { + continue; + } + if (expectsOptionValue) { + expectsOptionValue = false; + continue; + } + if (token === "--") { + return false; + } + if (!token.startsWith("-") || token === "-") { + return false; + } + const lower = normalizeLowercaseStringOrEmpty(token); + const [flag] = lower.split("=", 2); + if (flag === "-o" || flag === "--output") { + return true; + } + if (TIME_OPTIONS_WITH_VALUE.has(flag) && !lower.includes("=")) { + expectsOptionValue = true; + } + } + return false; +} + function supportsScriptPositionalCommand(platform: NodeJS.Platform = process.platform): boolean { return platform === "darwin" || platform === "freebsd"; } -function unwrapScriptInvocation(argv: string[]): string[] | null { - if (!supportsScriptPositionalCommand()) { +function unwrapScriptInvocation( + argv: string[], + platform: NodeJS.Platform = process.platform, +): string[] | null { + if (!supportsScriptPositionalCommand(platform)) { return null; } return scanWrapperInvocation(argv, { @@ -368,12 +400,16 @@ const DISPATCH_WRAPPER_SPECS: readonly DispatchWrapperSpec[] = [ { name: "nice", unwrap: unwrapNiceInvocation, transparentUsage: true }, { name: "nohup", unwrap: unwrapNohupInvocation, transparentUsage: true }, { name: "sandbox-exec", unwrap: unwrapSandboxExecInvocation, transparentUsage: true }, - { name: "script", unwrap: unwrapScriptInvocation, transparentUsage: true }, + { name: "script", unwrap: unwrapScriptInvocation, transparentUsage: false }, { name: "setsid" }, { name: "stdbuf", unwrap: unwrapStdbufInvocation, transparentUsage: true }, { name: "sudo" }, { name: "taskset" }, - { name: "time", unwrap: unwrapTimeInvocation, transparentUsage: true }, + { + name: "time", + unwrap: unwrapTimeInvocation, + transparentUsage: (argv) => !timeInvocationWritesOutputFile(argv), + }, { name: "timeout", unwrap: unwrapTimeoutInvocation, transparentUsage: true }, { name: "xcrun", diff --git a/src/infra/exec-approvals-allowlist.ts b/src/infra/exec-approvals-allowlist.ts index 95d6c48ecbd..778ca651358 100644 --- a/src/infra/exec-approvals-allowlist.ts +++ b/src/infra/exec-approvals-allowlist.ts @@ -408,6 +408,7 @@ function resolveSegmentAllowlistMatch(params: { segment: allowlistSegment, cwd: params.context.cwd, env: params.context.env, + platform: params.context.platform, }) : undefined; const shellPositionalArgvMatch = shellPositionalArgvCandidatePath @@ -822,6 +823,7 @@ function resolveShellWrapperPositionalArgvCandidatePath(params: { segment: ExecCommandSegment; cwd?: string; env?: NodeJS.ProcessEnv; + platform?: string | null; }): string | undefined { if (!isShellWrapperSegment(params.segment)) { return undefined; @@ -860,7 +862,12 @@ function resolveShellWrapperPositionalArgvCandidatePath(params: { return undefined; } - const resolution = resolveCommandResolutionFromArgv([carriedExecutable], params.cwd, params.env); + const resolution = resolveCommandResolutionFromArgv( + [carriedExecutable], + params.cwd, + params.env, + (params.platform ?? undefined) as NodeJS.Platform | undefined, + ); return resolveExecutionTargetCandidatePath(resolution, params.cwd); } @@ -965,7 +972,11 @@ function collectAllowAlwaysPatterns(params: { return; } - const trustPlan = resolveExecWrapperTrustPlan(params.segment.argv); + const trustPlan = resolveExecWrapperTrustPlan( + params.segment.argv, + undefined, + (params.platform ?? undefined) as NodeJS.Platform | undefined, + ); if (trustPlan.policyBlocked) { return; } @@ -976,7 +987,12 @@ function collectAllowAlwaysPatterns(params: { raw: trustPlan.argv.join(" "), argv: trustPlan.argv, sourceArgv: params.segment.sourceArgv, - resolution: resolveCommandResolutionFromArgv(trustPlan.argv, params.cwd, params.env), + resolution: resolveCommandResolutionFromArgv( + trustPlan.argv, + params.cwd, + params.env, + (params.platform ?? undefined) as NodeJS.Platform | undefined, + ), }; const candidatePath = resolveExecutionTargetTrustPath(segment.resolution, params.cwd); @@ -1005,6 +1021,7 @@ function collectAllowAlwaysPatterns(params: { segment, cwd: params.cwd, env: params.env, + platform: params.platform, }) : undefined; if (positionalArgvPath) { diff --git a/src/infra/exec-approvals-analysis.test.ts b/src/infra/exec-approvals-analysis.test.ts index b7f5362cedc..93e84e89c1c 100644 --- a/src/infra/exec-approvals-analysis.test.ts +++ b/src/infra/exec-approvals-analysis.test.ts @@ -624,6 +624,49 @@ describe("exec approvals shell analysis", () => { expect(result.segmentSatisfiedBy).toEqual(["allowlist"]); }); + it.each([ + { + name: "BSD script transcript", + command: "script ~/.zshenv git log -1 --format='payload'", + platform: "darwin" as const, + blockedWrapper: "script", + }, + { + name: "GNU time output file", + command: "/usr/bin/time -o ~/.bashrc -a -f 'payload' git status", + platform: "linux" as const, + blockedWrapper: "time", + }, + ])("rejects side-effecting dispatch wrapper allowlist bypasses for $name", (testCase) => { + const result = evaluateShellAllowlist({ + command: testCase.command, + allowlist: [{ pattern: "git" }], + safeBins: new Set(), + cwd: "/tmp", + platform: testCase.platform, + }); + + expect(result.analysisOk).toBe(true); + expect(result.allowlistSatisfied).toBe(false); + expect(result.segments[0]?.resolution?.policyBlocked).toBe(true); + expect(result.segments[0]?.resolution?.blockedWrapper).toBe(testCase.blockedWrapper); + expect(result.segmentSatisfiedBy).toEqual([null]); + }); + + it("keeps GNU time transparent when it only reports to stderr", () => { + const result = evaluateShellAllowlist({ + command: "/usr/bin/time -p git status", + allowlist: [{ pattern: "git" }], + safeBins: new Set(), + cwd: "/tmp", + platform: "linux", + }); + + expect(result.analysisOk).toBe(true); + expect(result.allowlistSatisfied).toBe(true); + expect(result.segmentSatisfiedBy).toEqual(["allowlist"]); + }); + it("rejects the legacy skill display prelude when only the wrapper is allowlisted", () => { if (process.platform === "win32") { return; diff --git a/src/infra/exec-approvals-analysis.ts b/src/infra/exec-approvals-analysis.ts index 0979b138ace..18c492eaddf 100644 --- a/src/infra/exec-approvals-analysis.ts +++ b/src/infra/exec-approvals-analysis.ts @@ -644,6 +644,7 @@ function analyzeWindowsShellCommand(params: { command: string; cwd?: string; env?: NodeJS.ProcessEnv; + platform?: string | null; }): ExecCommandAnalysis { const effective = stripWindowsShellWrapper(params.command.trim()); const unsupported = findWindowsUnsupportedToken(effective); @@ -664,7 +665,12 @@ function analyzeWindowsShellCommand(params: { { raw: params.command, argv, - resolution: resolveCommandResolutionFromArgv(argv, params.cwd, params.env), + resolution: resolveCommandResolutionFromArgv( + argv, + params.cwd, + params.env, + (params.platform ?? undefined) as NodeJS.Platform | undefined, + ), }, ], }; @@ -679,6 +685,7 @@ function parseSegmentsFromParts( parts: string[], cwd?: string, env?: NodeJS.ProcessEnv, + platform?: string | null, ): ExecCommandSegment[] | null { const segments: ExecCommandSegment[] = []; for (const raw of parts) { @@ -689,7 +696,12 @@ function parseSegmentsFromParts( segments.push({ raw, argv, - resolution: resolveCommandResolutionFromArgv(argv, cwd, env), + resolution: resolveCommandResolutionFromArgv( + argv, + cwd, + env, + (platform ?? undefined) as NodeJS.Platform | undefined, + ), }); } return segments; @@ -1206,7 +1218,12 @@ export function analyzeShellCommand(params: { if (!pipelineSplit.ok) { return { ok: false, reason: pipelineSplit.reason, segments: [] }; } - const segments = parseSegmentsFromParts(pipelineSplit.segments, params.cwd, params.env); + const segments = parseSegmentsFromParts( + pipelineSplit.segments, + params.cwd, + params.env, + params.platform, + ); if (!segments) { return { ok: false, reason: "unable to parse shell segment", segments: [] }; } @@ -1222,7 +1239,7 @@ export function analyzeShellCommand(params: { if (!split.ok) { return { ok: false, reason: split.reason, segments: [] }; } - const segments = parseSegmentsFromParts(split.segments, params.cwd, params.env); + const segments = parseSegmentsFromParts(split.segments, params.cwd, params.env, params.platform); if (!segments) { return { ok: false, reason: "unable to parse shell segment", segments: [] }; } @@ -1233,6 +1250,7 @@ export function analyzeArgvCommand(params: { argv: string[]; cwd?: string; env?: NodeJS.ProcessEnv; + platform?: string | null; }): ExecCommandAnalysis { const argv = params.argv.filter((entry) => entry.trim().length > 0); if (argv.length === 0) { @@ -1245,7 +1263,12 @@ export function analyzeArgvCommand(params: { raw: argv.join(" "), argv, sourceArgv: [...params.argv], - resolution: resolveCommandResolutionFromArgv(argv, params.cwd, params.env), + resolution: resolveCommandResolutionFromArgv( + argv, + params.cwd, + params.env, + (params.platform ?? undefined) as NodeJS.Platform | undefined, + ), }, ], }; diff --git a/src/infra/exec-command-resolution.test.ts b/src/infra/exec-command-resolution.test.ts index a6aaae09e93..bd61252ba00 100644 --- a/src/infra/exec-command-resolution.test.ts +++ b/src/infra/exec-command-resolution.test.ts @@ -189,6 +189,32 @@ describe("exec-command-resolution", () => { expect(timeResolution?.execution.executableName).toBe(fixture.exeName); }); + it("keeps file-writing dispatch wrappers on the policy boundary", () => { + const timeResolution = resolveCommandResolutionFromArgv([ + "/usr/bin/time", + "-o", + "/tmp/time.log", + "-a", + "-f", + "payload", + "git", + "status", + ]); + expect(timeResolution?.policyBlocked).toBe(true); + expect(timeResolution?.blockedWrapper).toBe("time"); + expect(timeResolution?.execution.rawExecutable).toBe("/usr/bin/time"); + + const scriptResolution = resolveCommandResolutionFromArgv( + ["script", "/tmp/session.log", "git", "status"], + undefined, + undefined, + "darwin", + ); + expect(scriptResolution?.policyBlocked).toBe(true); + expect(scriptResolution?.blockedWrapper).toBe("script"); + expect(scriptResolution?.execution.rawExecutable).toBe("script"); + }); + it("keeps shell multiplexer wrappers as a separate policy target", () => { if (process.platform === "win32") { return; diff --git a/src/infra/exec-command-resolution.ts b/src/infra/exec-command-resolution.ts index bd3bccd372f..149a39dd2b8 100644 --- a/src/infra/exec-command-resolution.ts +++ b/src/infra/exec-command-resolution.ts @@ -145,8 +145,9 @@ export function resolveCommandResolutionFromArgv( argv: string[], cwd?: string, env?: NodeJS.ProcessEnv, + platform: NodeJS.Platform = process.platform, ): CommandResolution | null { - const plan = resolveExecWrapperTrustPlan(argv); + const plan = resolveExecWrapperTrustPlan(argv, undefined, platform); const effectiveArgv = plan.argv; const rawExecutable = effectiveArgv[0]?.trim(); if (!rawExecutable) { diff --git a/src/infra/exec-wrapper-resolution.test.ts b/src/infra/exec-wrapper-resolution.test.ts index 89e20931d87..3cc12b199ba 100644 --- a/src/infra/exec-wrapper-resolution.test.ts +++ b/src/infra/exec-wrapper-resolution.test.ts @@ -366,6 +366,33 @@ describe("resolveDispatchWrapperTrustPlan", () => { }); }); + test("blocks script transcript wrappers even when the inner command is parseable", () => { + expect( + resolveDispatchWrapperTrustPlan( + ["script", "-q", "/tmp/session.log", "bash", "-lc", "echo hi"], + undefined, + "darwin", + ), + ).toEqual({ + argv: ["script", "-q", "/tmp/session.log", "bash", "-lc", "echo hi"], + wrappers: ["script"], + policyBlocked: true, + blockedWrapper: "script", + }); + }); + + test.each([ + ["short output option", ["time", "-o", "/tmp/time.log", "bash", "-lc", "echo hi"]], + ["long output option", ["time", "--output=/tmp/time.log", "bash", "-lc", "echo hi"]], + ])("blocks GNU time file-output wrappers for %s", (_name, argv) => { + expect(resolveDispatchWrapperTrustPlan(argv)).toEqual({ + argv, + wrappers: ["time"], + policyBlocked: true, + blockedWrapper: "time", + }); + }); + test("blocks wrapper overflow beyond the configured depth", () => { expect( resolveDispatchWrapperTrustPlan(["nohup", "timeout", "5s", "bash", "-lc", "echo hi"], 1), diff --git a/src/infra/exec-wrapper-trust-plan.ts b/src/infra/exec-wrapper-trust-plan.ts index 04c90681294..f6101ba7b7c 100644 --- a/src/infra/exec-wrapper-trust-plan.ts +++ b/src/infra/exec-wrapper-trust-plan.ts @@ -65,13 +65,18 @@ function finalizeExecWrapperTrustPlan( export function resolveExecWrapperTrustPlan( argv: string[], maxDepth = MAX_DISPATCH_WRAPPER_DEPTH, + platform: NodeJS.Platform = process.platform, ): ExecWrapperTrustPlan { let current = argv; let policyArgv = argv; let sawShellMultiplexer = false; const wrapperChain: string[] = []; for (let depth = 0; depth < maxDepth; depth += 1) { - const dispatchPlan = resolveDispatchWrapperTrustPlan(current, maxDepth - wrapperChain.length); + const dispatchPlan = resolveDispatchWrapperTrustPlan( + current, + maxDepth - wrapperChain.length, + platform, + ); if (dispatchPlan.policyBlocked) { return blockedExecWrapperTrustPlan({ argv: dispatchPlan.argv, @@ -119,7 +124,7 @@ export function resolveExecWrapperTrustPlan( } if (wrapperChain.length >= maxDepth) { - const dispatchOverflow = unwrapKnownDispatchWrapperInvocation(current); + const dispatchOverflow = unwrapKnownDispatchWrapperInvocation(current, platform); if (dispatchOverflow.kind === "blocked" || dispatchOverflow.kind === "unwrapped") { return blockedExecWrapperTrustPlan({ argv: current,