From 77b89719d5b7e271f48b6f49e334a8b991468c3b Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sat, 14 Feb 2026 19:42:52 +0100 Subject: [PATCH] fix(security): block safeBins shell expansion --- CHANGELOG.md | 1 + docs/tools/exec-approvals.md | 2 + docs/tools/exec.md | 3 +- src/agents/bash-tools.exec-runtime.ts | 12 +- src/agents/bash-tools.exec.ts | 22 +++ src/agents/pi-tools.safe-bins.e2e.test.ts | 42 ++++++ src/infra/exec-approvals-analysis.ts | 169 ++++++++++++++++++++++ src/infra/exec-approvals.test.ts | 20 +++ 8 files changed, 266 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d99bbb80eb0..52cdc1022b8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ Docs: https://docs.openclaw.ai - BlueBubbles: include sender identity in group chat envelopes and pass clean message text to the agent prompt, aligning with iMessage/Signal formatting. (#16210) Thanks @zerone0x. - WhatsApp: honor per-account `dmPolicy` overrides (account-level settings now take precedence over channel defaults for inbound DMs). (#10082) Thanks @mcaxtr. - Security/Node Host: enforce `system.run` rawCommand/argv consistency to prevent allowlist/approval bypass. Thanks @christos-eth. +- Security/Exec approvals: prevent safeBins allowlist bypass via shell expansion (host exec allowlist mode only; not enabled by default). Thanks @christos-eth. - Security/Gateway: block `system.execApprovals.*` via `node.invoke` (use `exec.approvals.node.*` instead). Thanks @christos-eth. - CLI: fix lazy core command registration so top-level maintenance commands (`doctor`, `dashboard`, `reset`, `uninstall`) resolve correctly instead of exposing a non-functional `maintenance` placeholder command. - Security/Agents: scope CLI process cleanup to owned child PIDs to avoid killing unrelated processes on shared hosts. Thanks @aether-ai-agent. diff --git a/docs/tools/exec-approvals.md b/docs/tools/exec-approvals.md index 2f446c30684..8aea7a48958 100644 --- a/docs/tools/exec-approvals.md +++ b/docs/tools/exec-approvals.md @@ -124,6 +124,8 @@ are treated as allowlisted on nodes (macOS node or headless node host). This use `tools.exec.safeBins` defines a small list of **stdin-only** binaries (for example `jq`) that can run in allowlist mode **without** explicit allowlist entries. Safe bins reject positional file args and path-like tokens, so they can only operate on the incoming stream. +Safe bins also force argv tokens to be treated as **literal text** at execution time (no globbing +and no `$VARS` expansion), so patterns like `*` or `$HOME/...` cannot be used to smuggle file reads. Shell chaining and redirections are not auto-allowed in allowlist mode. Shell chaining (`&&`, `||`, `;`) is allowed when every top-level segment satisfies the allowlist diff --git a/docs/tools/exec.md b/docs/tools/exec.md index cda1406ca86..31d2f8d2865 100644 --- a/docs/tools/exec.md +++ b/docs/tools/exec.md @@ -120,7 +120,8 @@ running after `tools.exec.approvalRunningNoticeMs`, a single `Exec running` noti Allowlist enforcement matches **resolved binary paths only** (no basename matches). When `security=allowlist`, shell commands are auto-allowed only if every pipeline segment is allowlisted or a safe bin. Chaining (`;`, `&&`, `||`) and redirections are rejected in -allowlist mode. +allowlist mode unless every top-level segment satisfies the allowlist (including safe bins). +Redirections remain unsupported. ## Examples diff --git a/src/agents/bash-tools.exec-runtime.ts b/src/agents/bash-tools.exec-runtime.ts index 1d7f8e18e54..5fa7fb4bc2a 100644 --- a/src/agents/bash-tools.exec-runtime.ts +++ b/src/agents/bash-tools.exec-runtime.ts @@ -338,6 +338,9 @@ export function emitExecSystemEvent( export async function runExecProcess(opts: { command: string; + // Execute this instead of `command` (which is kept for display/session/logging). + // Used to sanitize safeBins execution while preserving the original user input. + execCommand?: string; workdir: string; env: Record; sandbox?: BashSandboxConfig; @@ -357,6 +360,7 @@ export async function runExecProcess(opts: { let child: ChildProcessWithoutNullStreams | null = null; let pty: PtyHandle | null = null; let stdin: SessionStdin | undefined; + const execCommand = opts.execCommand ?? opts.command; if (opts.sandbox) { const { child: spawned } = await spawnWithFallback({ @@ -364,7 +368,7 @@ export async function runExecProcess(opts: { "docker", ...buildDockerExecArgs({ containerName: opts.sandbox.containerName, - command: opts.command, + command: execCommand, workdir: opts.containerWorkdir ?? opts.sandbox.containerWorkdir, env: opts.env, tty: opts.usePty, @@ -403,7 +407,7 @@ export async function runExecProcess(opts: { if (!spawnPty) { throw new Error("PTY support is unavailable (node-pty spawn not found)."); } - pty = spawnPty(shell, [...shellArgs, opts.command], { + pty = spawnPty(shell, [...shellArgs, execCommand], { cwd: opts.workdir, env: opts.env, name: process.env.TERM ?? "xterm-256color", @@ -435,7 +439,7 @@ export async function runExecProcess(opts: { logWarn(`exec: PTY spawn failed (${errText}); retrying without PTY for "${opts.command}".`); opts.warnings.push(warning); const { child: spawned } = await spawnWithFallback({ - argv: [shell, ...shellArgs, opts.command], + argv: [shell, ...shellArgs, execCommand], options: { cwd: opts.workdir, env: opts.env, @@ -462,7 +466,7 @@ export async function runExecProcess(opts: { } else { const { shell, args: shellArgs } = getShellConfig(); const { child: spawned } = await spawnWithFallback({ - argv: [shell, ...shellArgs, opts.command], + argv: [shell, ...shellArgs, execCommand], options: { cwd: opts.workdir, env: opts.env, diff --git a/src/agents/bash-tools.exec.ts b/src/agents/bash-tools.exec.ts index 9a2d57c45b4..9a99346ca7e 100644 --- a/src/agents/bash-tools.exec.ts +++ b/src/agents/bash-tools.exec.ts @@ -15,6 +15,7 @@ import { recordAllowlistUse, resolveExecApprovals, resolveExecApprovalsFromFile, + buildSafeShellCommand, } from "../infra/exec-approvals.js"; import { buildNodeShellCommand } from "../infra/node-shell.js"; import { @@ -170,6 +171,7 @@ export function createExecTool( const maxOutput = DEFAULT_MAX_OUTPUT; const pendingMaxOutput = DEFAULT_PENDING_MAX_OUTPUT; const warnings: string[] = []; + let execCommandOverride: string | undefined; const backgroundRequested = params.background === true; const yieldRequested = typeof params.yieldMs === "number"; if (!allowBackground && (backgroundRequested || yieldRequested)) { @@ -804,6 +806,25 @@ export function createExecTool( throw new Error("exec denied: allowlist miss"); } + // If allowlist is satisfied only via safeBins (no explicit allowlist match), + // run a sanitized `shell -c` command that disables glob/var expansion by + // forcing every argv token to be literal via single-quoting. + if ( + hostSecurity === "allowlist" && + analysisOk && + allowlistSatisfied && + allowlistMatches.length === 0 + ) { + const safe = buildSafeShellCommand({ + command: params.command, + platform: process.platform, + }); + if (!safe.ok || !safe.command) { + throw new Error(`exec denied: safeBins sanitize failed (${safe.reason ?? "unknown"})`); + } + execCommandOverride = safe.command; + } + if (allowlistMatches.length > 0) { const seen = new Set(); for (const match of allowlistMatches) { @@ -828,6 +849,7 @@ export function createExecTool( const usePty = params.pty === true && !sandbox; const run = await runExecProcess({ command: params.command, + execCommand: execCommandOverride, workdir, env, sandbox, diff --git a/src/agents/pi-tools.safe-bins.e2e.test.ts b/src/agents/pi-tools.safe-bins.e2e.test.ts index 20c2a87eb72..665059035d2 100644 --- a/src/agents/pi-tools.safe-bins.e2e.test.ts +++ b/src/agents/pi-tools.safe-bins.e2e.test.ts @@ -130,4 +130,46 @@ describe("createOpenClawCodingTools safeBins", () => { expect(result.details.status).toBe("completed"); expect(text).toContain(marker); }); + + it("does not allow env var expansion to smuggle file args via safeBins", async () => { + if (process.platform === "win32") { + return; + } + + const { createOpenClawCodingTools } = await import("./pi-tools.js"); + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-safe-bins-expand-")); + + const secret = `TOP_SECRET_${Date.now()}`; + fs.writeFileSync(path.join(tmpDir, "secret.txt"), `${secret}\n`, "utf8"); + + const cfg: OpenClawConfig = { + tools: { + exec: { + host: "gateway", + security: "allowlist", + ask: "off", + safeBins: ["head", "wc"], + }, + }, + }; + + const tools = createOpenClawCodingTools({ + config: cfg, + sessionKey: "agent:main:main", + workspaceDir: tmpDir, + agentDir: path.join(tmpDir, "agent"), + }); + const execTool = tools.find((tool) => tool.name === "exec"); + expect(execTool).toBeDefined(); + + const result = await execTool!.execute("call1", { + command: "head $FOO ; wc -l", + workdir: tmpDir, + env: { FOO: "secret.txt" }, + }); + const text = result.content.find((content) => content.type === "text")?.text ?? ""; + + expect(result.details.status).toBe("completed"); + expect(text).not.toContain(secret); + }); }); diff --git a/src/infra/exec-approvals-analysis.ts b/src/infra/exec-approvals-analysis.ts index e40525a439f..4c1246791ed 100644 --- a/src/infra/exec-approvals-analysis.ts +++ b/src/infra/exec-approvals-analysis.ts @@ -248,6 +248,13 @@ export type ExecCommandAnalysis = { chains?: ExecCommandSegment[][]; // Segments grouped by chain operator (&&, ||, ;) }; +export type ShellChainOperator = "&&" | "||" | ";"; + +export type ShellChainPart = { + part: string; + opToNext: ShellChainOperator | null; +}; + const DISALLOWED_PIPELINE_TOKENS = new Set([">", "<", "`", "\n", "\r", "(", ")"]); const DOUBLE_QUOTE_ESCAPES = new Set(["\\", '"', "$", "`", "\n", "\r"]); const WINDOWS_UNSUPPORTED_TOKENS = new Set([ @@ -603,6 +610,168 @@ function parseSegmentsFromParts( return segments; } +/** + * Splits a command string by chain operators (&&, ||, ;) while preserving the operators. + * Returns null when no chain is present or when the chain is malformed. + */ +export function splitCommandChainWithOperators(command: string): ShellChainPart[] | null { + const parts: ShellChainPart[] = []; + let buf = ""; + let inSingle = false; + let inDouble = false; + let escaped = false; + let foundChain = false; + let invalidChain = false; + + const pushPart = (opToNext: ShellChainOperator | null) => { + const trimmed = buf.trim(); + buf = ""; + if (!trimmed) { + return false; + } + parts.push({ part: trimmed, opToNext }); + return true; + }; + + for (let i = 0; i < command.length; i += 1) { + const ch = command[i]; + const next = command[i + 1]; + if (escaped) { + buf += ch; + escaped = false; + continue; + } + if (!inSingle && !inDouble && ch === "\\") { + escaped = true; + buf += ch; + continue; + } + if (inSingle) { + if (ch === "'") { + inSingle = false; + } + buf += ch; + continue; + } + if (inDouble) { + if (ch === "\\" && isDoubleQuoteEscape(next)) { + buf += ch; + buf += next; + i += 1; + continue; + } + if (ch === '"') { + inDouble = false; + } + buf += ch; + continue; + } + if (ch === "'") { + inSingle = true; + buf += ch; + continue; + } + if (ch === '"') { + inDouble = true; + buf += ch; + continue; + } + + if (ch === "&" && next === "&") { + if (!pushPart("&&")) { + invalidChain = true; + } + i += 1; + foundChain = true; + continue; + } + if (ch === "|" && next === "|") { + if (!pushPart("||")) { + invalidChain = true; + } + i += 1; + foundChain = true; + continue; + } + if (ch === ";") { + if (!pushPart(";")) { + invalidChain = true; + } + foundChain = true; + continue; + } + + buf += ch; + } + + if (!foundChain) { + return null; + } + const trimmed = buf.trim(); + if (!trimmed) { + return null; + } + parts.push({ part: trimmed, opToNext: null }); + if (invalidChain || parts.length === 0) { + return null; + } + return parts; +} + +function shellEscapeSingleArg(value: string): string { + // Shell-safe across sh/bash/zsh: single-quote everything, escape embedded single quotes. + // Example: foo'bar -> 'foo'"'"'bar' + const singleQuoteEscape = `'"'"'`; + return `'${value.replace(/'/g, singleQuoteEscape)}'`; +} + +/** + * Builds a shell command string that preserves pipes/chaining, but forces *arguments* to be + * literal (no globbing, no env-var expansion) by single-quoting every argv token. + * + * Used to make "safe bins" actually stdin-only even though execution happens via `shell -c`. + */ +export function buildSafeShellCommand(params: { command: string; platform?: string | null }): { + ok: boolean; + command?: string; + reason?: string; +} { + const platform = params.platform ?? null; + if (isWindowsPlatform(platform)) { + return { ok: false, reason: "unsupported platform" }; + } + const source = params.command.trim(); + if (!source) { + return { ok: false, reason: "empty command" }; + } + + const chain = splitCommandChainWithOperators(source); + const chainParts = chain ?? [{ part: source, opToNext: null }]; + let out = ""; + + for (let i = 0; i < chainParts.length; i += 1) { + const part = chainParts[i]; + const pipelineSplit = splitShellPipeline(part.part); + if (!pipelineSplit.ok) { + return { ok: false, reason: pipelineSplit.reason ?? "unable to parse pipeline" }; + } + const renderedSegments: string[] = []; + for (const segmentRaw of pipelineSplit.segments) { + const argv = splitShellArgs(segmentRaw); + if (!argv || argv.length === 0) { + return { ok: false, reason: "unable to parse shell segment" }; + } + renderedSegments.push(argv.map((token) => shellEscapeSingleArg(token)).join(" ")); + } + out += renderedSegments.join(" | "); + if (part.opToNext) { + out += ` ${part.opToNext} `; + } + } + + return { ok: true, command: out }; +} + /** * Splits a command string by chain operators (&&, ||, ;) while respecting quotes. * Returns null when no chain is present or when the chain is malformed. diff --git a/src/infra/exec-approvals.test.ts b/src/infra/exec-approvals.test.ts index 26c50c12455..b6f6e9725ed 100644 --- a/src/infra/exec-approvals.test.ts +++ b/src/infra/exec-approvals.test.ts @@ -5,6 +5,7 @@ import { describe, expect, it, vi } from "vitest"; import { analyzeArgvCommand, analyzeShellCommand, + buildSafeShellCommand, evaluateExecAllowlist, evaluateShellAllowlist, isSafeBinUsage, @@ -78,6 +79,25 @@ describe("exec approvals allowlist matching", () => { }); }); +describe("exec approvals safe shell command builder", () => { + it("single-quotes argv tokens while preserving pipes/chaining", () => { + if (process.platform === "win32") { + return; + } + const res = buildSafeShellCommand({ + command: 'head $FOO | grep * && echo "a\'b" ; wc -l', + platform: process.platform, + }); + expect(res.ok).toBe(true); + expect(res.command).toContain("'$FOO'"); + expect(res.command).toContain("'*'"); + expect(res.command).toContain("&&"); + expect(res.command).toContain(";"); + expect(res.command).toContain("|"); + expect(res.command).toContain("'a'\"'\"'b'"); + }); +}); + describe("exec approvals command resolution", () => { it("resolves PATH executables", () => { const dir = makeTempDir();