From cfe8457a0f4aae5324daec261d3b0aad1461a4bc Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Thu, 19 Feb 2026 14:07:43 +0100 Subject: [PATCH] fix(security): harden safeBins stdin-only enforcement --- CHANGELOG.md | 1 + docs/tools/exec-approvals.md | 7 +- src/agents/pi-tools.safe-bins.e2e.test.ts | 88 +++++++++++++++++++++++ src/infra/exec-approvals-allowlist.ts | 56 ++++++++++++++- src/infra/exec-approvals-analysis.ts | 4 +- src/infra/exec-approvals.test.ts | 51 ++++++++++++- 6 files changed, 200 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1fcb5160f2..787454baa2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -66,6 +66,7 @@ Docs: https://docs.openclaw.ai - Sandbox/Registry: serialize container and browser registry writes with shared file locks and atomic replacement to prevent lost updates and delete rollback races from desyncing `sandbox list`, `prune`, and `recreate --all`. Thanks @kexinoh. - Security/Exec: require `tools.exec.safeBins` binaries to resolve from trusted bin directories (system defaults plus gateway startup `PATH`) so PATH-hijacked trojan binaries cannot bypass allowlist checks. Thanks @jackhax for reporting. - Security/Browser: route browser URL navigation through one SSRF-guarded validation path for tab-open/CDP-target/Playwright navigation flows and block private/metadata destinations by default (configurable via `browser.ssrfPolicy`). This ships in the next npm release. Thanks @dorjoos for reporting. +- Security/Exec: for the next npm release, harden safe-bin stdin-only enforcement by blocking output/recursive flags (`sort -o/--output`, grep recursion) and tightening default safe bins to remove `sort`/`grep`, preventing safe-bin allowlist bypass for file writes/recursive reads. Thanks @nedlir for reporting. - Cron/Webhooks: protect cron webhook POST delivery with SSRF-guarded outbound fetch (`fetchWithSsrFGuard`) to block private/metadata destinations before request dispatch. Thanks @Adam55A-code. - Security/Net: block SSRF bypass via NAT64 (`64:ff9b::/96`, `64:ff9b:1::/48`), 6to4 (`2002::/16`), and Teredo (`2001:0000::/32`) IPv6 transition addresses, and fail closed on IPv6 parse errors. Thanks @jackhax. diff --git a/docs/tools/exec-approvals.md b/docs/tools/exec-approvals.md index dffa71a028..bd68277b80 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 enforce explicit per-binary flag policy for options that break stdin-only +behavior (for example `sort -o/--output` and grep recursive flags). Safe bins also force argv tokens to be treated as **literal text** at execution time (no globbing and no `$VARS` expansion) for stdin-only segments, so patterns like `*` or `$HOME/...` cannot be used to smuggle file reads. @@ -136,7 +138,10 @@ Shell chaining (`&&`, `||`, `;`) is allowed when every top-level segment satisfi Command substitution (`$()` / backticks) is rejected during allowlist parsing, including inside double quotes; use single quotes if you need literal `$()` text. -Default safe bins: `jq`, `grep`, `cut`, `sort`, `uniq`, `head`, `tail`, `tr`, `wc`. +Default safe bins: `jq`, `cut`, `uniq`, `head`, `tail`, `tr`, `wc`. + +`grep` and `sort` are not in the default list. If you opt in, keep explicit allowlist entries for +their non-stdin workflows. ## Control UI editing diff --git a/src/agents/pi-tools.safe-bins.e2e.test.ts b/src/agents/pi-tools.safe-bins.e2e.test.ts index 6e8c3747bc..694bda8293 100644 --- a/src/agents/pi-tools.safe-bins.e2e.test.ts +++ b/src/agents/pi-tools.safe-bins.e2e.test.ts @@ -157,4 +157,92 @@ describe("createOpenClawCodingTools safeBins", () => { expect(blockedResultDetails.status).toBe("completed"); expect(text).not.toContain(secret); }); + + it("blocks sort output flags from writing files 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-sort-")); + + const cfg: OpenClawConfig = { + tools: { + exec: { + host: "gateway", + security: "allowlist", + ask: "off", + safeBins: ["sort"], + }, + }, + }; + + 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 shortTarget = path.join(tmpDir, "blocked-short.txt"); + await expect( + execTool!.execute("call1", { + command: "sort -oblocked-short.txt", + workdir: tmpDir, + }), + ).rejects.toThrow("exec denied: allowlist miss"); + expect(fs.existsSync(shortTarget)).toBe(false); + + const longTarget = path.join(tmpDir, "blocked-long.txt"); + await expect( + execTool!.execute("call2", { + command: "sort --output=blocked-long.txt", + workdir: tmpDir, + }), + ).rejects.toThrow("exec denied: allowlist miss"); + expect(fs.existsSync(longTarget)).toBe(false); + }); + + it("blocks grep recursive flags from reading cwd 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-grep-")); + fs.writeFileSync( + path.join(tmpDir, "secret.txt"), + "SAFE_BINS_RECURSIVE_SHOULD_NOT_LEAK\n", + "utf8", + ); + + const cfg: OpenClawConfig = { + tools: { + exec: { + host: "gateway", + security: "allowlist", + ask: "off", + safeBins: ["grep"], + }, + }, + }; + + 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(); + + await expect( + execTool!.execute("call1", { + command: "grep -R SAFE_BINS_RECURSIVE_SHOULD_NOT_LEAK", + workdir: tmpDir, + }), + ).rejects.toThrow("exec denied: allowlist miss"); + }); }); diff --git a/src/infra/exec-approvals-allowlist.ts b/src/infra/exec-approvals-allowlist.ts index fb953d5a6d..a20c4672bc 100644 --- a/src/infra/exec-approvals-allowlist.ts +++ b/src/infra/exec-approvals-allowlist.ts @@ -1,5 +1,6 @@ import fs from "node:fs"; import path from "node:path"; +import type { ExecAllowlistEntry } from "./exec-approvals.js"; import { DEFAULT_SAFE_BINS, analyzeShellCommand, @@ -11,7 +12,6 @@ import { type CommandResolution, type ExecCommandSegment, } from "./exec-approvals-analysis.js"; -import type { ExecAllowlistEntry } from "./exec-approvals.js"; import { isTrustedSafeBinPath } from "./exec-safe-bin-trust.js"; function isPathLikeToken(value: string): boolean { @@ -62,6 +62,57 @@ function hasGlobToken(value: string): boolean { return /[*?[\]]/.test(value); } +type SafeBinOptionPolicy = { + blockedShort?: ReadonlySet; + blockedLong?: ReadonlySet; +}; + +const SAFE_BIN_OPTION_POLICIES: Readonly> = { + // sort can write arbitrary output paths via -o/--output, which breaks stdin-only guarantees. + sort: { + blockedShort: new Set(["o"]), + blockedLong: new Set(["output"]), + }, + // grep recursion flags read from cwd (or provided roots), so they are not stdin-only. + grep: { + blockedShort: new Set(["d", "r"]), + blockedLong: new Set(["dereference-recursive", "directories", "recursive"]), + }, +}; + +function parseLongOptionName(token: string): string | null { + if (!token.startsWith("--") || token === "--") { + return null; + } + const body = token.slice(2); + if (!body) { + return null; + } + const eqIndex = body.indexOf("="); + const name = (eqIndex >= 0 ? body.slice(0, eqIndex) : body).trim().toLowerCase(); + return name.length > 0 ? name : null; +} + +function hasBlockedSafeBinOption(execName: string, token: string): boolean { + const policy = SAFE_BIN_OPTION_POLICIES[execName]; + if (!policy || !token.startsWith("-")) { + return false; + } + const longName = parseLongOptionName(token); + if (longName) { + return policy.blockedLong?.has(longName) ?? false; + } + if (token === "-" || token === "--") { + return false; + } + for (const ch of token.slice(1)) { + if (policy.blockedShort?.has(ch.toLowerCase())) { + return true; + } + } + return false; +} + export function isSafeBinUsage(params: { argv: string[]; resolution: CommandResolution | null; @@ -112,6 +163,9 @@ export function isSafeBinUsage(params: { continue; } if (token.startsWith("-")) { + if (hasBlockedSafeBinOption(execName, token)) { + return false; + } const eqIndex = token.indexOf("="); if (eqIndex > 0) { const value = token.slice(eqIndex + 1); diff --git a/src/infra/exec-approvals-analysis.ts b/src/infra/exec-approvals-analysis.ts index a41d334771..29dd80aff4 100644 --- a/src/infra/exec-approvals-analysis.ts +++ b/src/infra/exec-approvals-analysis.ts @@ -1,10 +1,10 @@ import fs from "node:fs"; import path from "node:path"; -import { splitShellArgs } from "../utils/shell-argv.js"; import type { ExecAllowlistEntry } from "./exec-approvals.js"; +import { splitShellArgs } from "../utils/shell-argv.js"; import { expandHomePrefix } from "./home-dir.js"; -export const DEFAULT_SAFE_BINS = ["jq", "grep", "cut", "sort", "uniq", "head", "tail", "tr", "wc"]; +export const DEFAULT_SAFE_BINS = ["jq", "cut", "uniq", "head", "tail", "tr", "wc"]; export type CommandResolution = { rawExecutable: string; diff --git a/src/infra/exec-approvals.test.ts b/src/infra/exec-approvals.test.ts index 0a7e1784c8..7daf609200 100644 --- a/src/infra/exec-approvals.test.ts +++ b/src/infra/exec-approvals.test.ts @@ -21,6 +21,7 @@ import { resolveExecApprovalsFromFile, resolveExecApprovalsPath, resolveExecApprovalsSocketPath, + resolveSafeBins, type ExecAllowlistEntry, type ExecApprovalsFile, } from "./exec-approvals.js"; @@ -414,6 +415,9 @@ describe("exec approvals safe bins", () => { argv: string[]; resolvedPath: string; expected: boolean; + safeBins?: string[]; + executableName?: string; + rawExecutable?: string; cwd?: string; setup?: (cwd: string) => void; }; @@ -439,6 +443,38 @@ describe("exec approvals safe bins", () => { expected: false, cwd: "/tmp", }, + { + name: "blocks sort output path via -o ", + argv: ["sort", "-o", "malicious.sh"], + resolvedPath: "/usr/bin/sort", + expected: false, + safeBins: ["sort"], + executableName: "sort", + }, + { + name: "blocks sort output path via attached short option (-ofile)", + argv: ["sort", "-omalicious.sh"], + resolvedPath: "/usr/bin/sort", + expected: false, + safeBins: ["sort"], + executableName: "sort", + }, + { + name: "blocks sort output path via --output=file", + argv: ["sort", "--output=malicious.sh"], + resolvedPath: "/usr/bin/sort", + expected: false, + safeBins: ["sort"], + executableName: "sort", + }, + { + name: "blocks grep recursive flags that read cwd", + argv: ["grep", "-R", "needle"], + resolvedPath: "/usr/bin/grep", + expected: false, + safeBins: ["grep"], + executableName: "grep", + }, ]; for (const testCase of cases) { @@ -448,14 +484,16 @@ describe("exec approvals safe bins", () => { } const cwd = testCase.cwd ?? makeTempDir(); testCase.setup?.(cwd); + const executableName = testCase.executableName ?? "jq"; + const rawExecutable = testCase.rawExecutable ?? executableName; const ok = isSafeBinUsage({ argv: testCase.argv, resolution: { - rawExecutable: "jq", + rawExecutable, resolvedPath: testCase.resolvedPath, - executableName: "jq", + executableName, }, - safeBins: normalizeSafeBins(["jq"]), + safeBins: normalizeSafeBins(testCase.safeBins ?? [executableName]), cwd, }); expect(ok).toBe(testCase.expected); @@ -479,6 +517,13 @@ describe("exec approvals safe bins", () => { }); expect(ok).toBe(true); }); + + it("does not include sort/grep in default safeBins", () => { + const defaults = resolveSafeBins(undefined); + expect(defaults.has("jq")).toBe(true); + expect(defaults.has("sort")).toBe(false); + expect(defaults.has("grep")).toBe(false); + }); }); describe("exec approvals allowlist evaluation", () => {