diff --git a/CHANGELOG.md b/CHANGELOG.md index 787454baa2..839679eb49 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -65,6 +65,7 @@ Docs: https://docs.openclaw.ai - Commands/Doctor: avoid rewriting invalid configs with new `gateway.auth.token` defaults during repair and only write when real config changes are detected, preventing accidental token duplication and backup churn. - 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/Exec: remove file-existence oracle behavior from `tools.exec.safeBins` by using deterministic argv-only stdin-safe validation and blocking file-oriented flags (for example `sort -o`, `jq -f`, `grep -f`) so allow/deny results no longer disclose host file presence. This ships in the next npm release. Thanks @nedlir 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. diff --git a/docs/tools/exec-approvals.md b/docs/tools/exec-approvals.md index bd68277b80..f813b89c87 100644 --- a/docs/tools/exec-approvals.md +++ b/docs/tools/exec-approvals.md @@ -124,6 +124,10 @@ 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. +Validation is deterministic from argv shape only (no host filesystem existence checks), which +prevents file-existence oracle behavior from allow/deny differences. +File-oriented options are denied for default safe bins (for example `sort -o`, `sort --output`, +`sort --files0-from`, `wc --files0-from`, `jq -f/--from-file`, `grep -f/--file`). 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 diff --git a/src/agents/pi-tools.safe-bins.e2e.test.ts b/src/agents/pi-tools.safe-bins.e2e.test.ts index 694bda8293..314d801c69 100644 --- a/src/agents/pi-tools.safe-bins.e2e.test.ts +++ b/src/agents/pi-tools.safe-bins.e2e.test.ts @@ -123,8 +123,7 @@ describe("createOpenClawCodingTools safeBins", () => { 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"); + fs.writeFileSync(path.join(tmpDir, "secret.txt"), "TOP_SECRET\n", "utf8"); const cfg: OpenClawConfig = { tools: { @@ -146,16 +145,57 @@ describe("createOpenClawCodingTools safeBins", () => { 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 ?? ""; + await expect( + execTool!.execute("call1", { + command: "head $FOO ; wc -l", + workdir: tmpDir, + env: { FOO: "secret.txt" }, + }), + ).rejects.toThrow("exec denied: allowlist miss"); + }); - const blockedResultDetails = result.details as { status?: string }; - expect(blockedResultDetails.status).toBe("completed"); - expect(text).not.toContain(secret); + it("does not leak file existence from sort output flags", async () => { + if (process.platform === "win32") { + return; + } + + const { createOpenClawCodingTools } = await import("./pi-tools.js"); + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-safe-bins-oracle-")); + fs.writeFileSync(path.join(tmpDir, "existing.txt"), "x\n", "utf8"); + + 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 run = async (command: string) => { + try { + const result = await execTool!.execute("call-oracle", { command, workdir: tmpDir }); + const text = result.content.find((content) => content.type === "text")?.text ?? ""; + return { kind: "result" as const, status: result.details.status, text }; + } catch (err) { + return { kind: "error" as const, message: String(err) }; + } + }; + + const existing = await run("sort -o existing.txt"); + const missing = await run("sort -o missing.txt"); + expect(existing).toEqual(missing); }); it("blocks sort output flags from writing files via safeBins", async () => { diff --git a/src/infra/exec-approvals-allowlist.ts b/src/infra/exec-approvals-allowlist.ts index a20c4672bc..4ebe6bbd3a 100644 --- a/src/infra/exec-approvals-allowlist.ts +++ b/src/infra/exec-approvals-allowlist.ts @@ -1,4 +1,3 @@ -import fs from "node:fs"; import path from "node:path"; import type { ExecAllowlistEntry } from "./exec-approvals.js"; import { @@ -31,14 +30,6 @@ function isPathLikeToken(value: string): boolean { return /^[A-Za-z]:[\\/]/.test(trimmed); } -function defaultFileExists(filePath: string): boolean { - try { - return fs.existsSync(filePath); - } catch { - return false; - } -} - export function normalizeSafeBins(entries?: string[]): Set { if (!Array.isArray(entries)) { return new Set(); @@ -62,57 +53,253 @@ function hasGlobToken(value: string): boolean { return /[*?[\]]/.test(value); } -type SafeBinOptionPolicy = { - blockedShort?: ReadonlySet; - blockedLong?: ReadonlySet; +type SafeBinProfile = { + minPositional?: number; + maxPositional?: number; + valueFlags?: ReadonlySet; + blockedFlags?: 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"]), +const NO_FLAGS = new Set(); +const SAFE_BIN_GENERIC_PROFILE: SafeBinProfile = {}; +const SAFE_BIN_PROFILES: Record = { + jq: { + maxPositional: 1, + valueFlags: new Set([ + "--arg", + "--argjson", + "--argstr", + "--argfile", + "--rawfile", + "--slurpfile", + "--from-file", + "--library-path", + "-L", + "-f", + ]), + blockedFlags: new Set([ + "--argfile", + "--rawfile", + "--slurpfile", + "--from-file", + "--library-path", + "-L", + "-f", + ]), }, - // 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"]), + maxPositional: 1, + valueFlags: new Set([ + "--regexp", + "--file", + "--max-count", + "--after-context", + "--before-context", + "--context", + "--devices", + "--directories", + "--binary-files", + "--exclude", + "--exclude-from", + "--include", + "--label", + "-e", + "-f", + "-m", + "-A", + "-B", + "-C", + "-D", + "-d", + ]), + blockedFlags: new Set([ + "--file", + "--exclude-from", + "--dereference-recursive", + "--directories", + "--recursive", + "-f", + "-d", + "-r", + "-R", + ]), + }, + cut: { + maxPositional: 0, + valueFlags: new Set([ + "--bytes", + "--characters", + "--fields", + "--delimiter", + "--output-delimiter", + "-b", + "-c", + "-f", + "-d", + ]), + }, + sort: { + maxPositional: 0, + valueFlags: new Set([ + "--key", + "--field-separator", + "--buffer-size", + "--temporary-directory", + "--compress-program", + "--parallel", + "--batch-size", + "--random-source", + "--files0-from", + "--output", + "-k", + "-t", + "-S", + "-T", + "-o", + ]), + blockedFlags: new Set(["--files0-from", "--output", "-o"]), + }, + uniq: { + maxPositional: 0, + valueFlags: new Set([ + "--skip-fields", + "--skip-chars", + "--check-chars", + "--group", + "-f", + "-s", + "-w", + ]), + }, + head: { + maxPositional: 0, + valueFlags: new Set(["--lines", "--bytes", "-n", "-c"]), + }, + tail: { + maxPositional: 0, + valueFlags: new Set([ + "--lines", + "--bytes", + "--sleep-interval", + "--max-unchanged-stats", + "--pid", + "-n", + "-c", + ]), + }, + tr: { + minPositional: 1, + maxPositional: 2, + }, + wc: { + maxPositional: 0, + valueFlags: new Set(["--files0-from"]), + blockedFlags: new Set(["--files0-from"]), }, }; -function parseLongOptionName(token: string): string | null { - if (!token.startsWith("--") || token === "--") { - return null; +function isSafeLiteralToken(value: string): boolean { + if (!value || value === "-") { + return true; } - 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; + return !hasGlobToken(value) && !isPathLikeToken(value); } -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; +function validateSafeBinArgv(args: string[], profile: SafeBinProfile): boolean { + const valueFlags = profile.valueFlags ?? NO_FLAGS; + const blockedFlags = profile.blockedFlags ?? NO_FLAGS; + const positional: string[] = []; + + for (let i = 0; i < args.length; i += 1) { + const token = args[i]; + if (!token) { + continue; + } + if (token === "--") { + for (let j = i + 1; j < args.length; j += 1) { + const rest = args[j]; + if (!rest || rest === "-") { + continue; + } + if (!isSafeLiteralToken(rest)) { + return false; + } + positional.push(rest); + } + break; + } + if (token === "-") { + continue; + } + if (!token.startsWith("-")) { + if (!isSafeLiteralToken(token)) { + return false; + } + positional.push(token); + continue; + } + + if (token.startsWith("--")) { + const eqIndex = token.indexOf("="); + const flag = eqIndex > 0 ? token.slice(0, eqIndex) : token; + if (blockedFlags.has(flag)) { + return false; + } + if (eqIndex > 0) { + if (!isSafeLiteralToken(token.slice(eqIndex + 1))) { + return false; + } + continue; + } + if (!valueFlags.has(flag)) { + continue; + } + const value = args[i + 1]; + if (!value || !isSafeLiteralToken(value)) { + return false; + } + i += 1; + continue; + } + + let consumedValue = false; + for (let j = 1; j < token.length; j += 1) { + const flag = `-${token[j]}`; + if (blockedFlags.has(flag)) { + return false; + } + if (!valueFlags.has(flag)) { + continue; + } + const inlineValue = token.slice(j + 1); + if (inlineValue) { + if (!isSafeLiteralToken(inlineValue)) { + return false; + } + } else { + const value = args[i + 1]; + if (!value || !isSafeLiteralToken(value)) { + return false; + } + i += 1; + } + consumedValue = true; + break; + } + if (!consumedValue && hasGlobToken(token)) { + return false; } } - return false; -} + const minPositional = profile.minPositional ?? 0; + if (positional.length < minPositional) { + return false; + } + if (typeof profile.maxPositional === "number" && positional.length > profile.maxPositional) { + return false; + } + return true; +} export function isSafeBinUsage(params: { argv: string[]; resolution: CommandResolution | null; @@ -151,44 +338,9 @@ export function isSafeBinUsage(params: { ) { return false; } - const cwd = params.cwd ?? process.cwd(); - const exists = params.fileExists ?? defaultFileExists; const argv = params.argv.slice(1); - for (let i = 0; i < argv.length; i += 1) { - const token = argv[i]; - if (!token) { - continue; - } - if (token === "-") { - continue; - } - if (token.startsWith("-")) { - if (hasBlockedSafeBinOption(execName, token)) { - return false; - } - const eqIndex = token.indexOf("="); - if (eqIndex > 0) { - const value = token.slice(eqIndex + 1); - if (value && hasGlobToken(value)) { - return false; - } - if (value && (isPathLikeToken(value) || exists(path.resolve(cwd, value)))) { - return false; - } - } - continue; - } - if (hasGlobToken(token)) { - return false; - } - if (isPathLikeToken(token)) { - return false; - } - if (exists(path.resolve(cwd, token))) { - return false; - } - } - return true; + const profile = SAFE_BIN_PROFILES[execName] ?? SAFE_BIN_GENERIC_PROFILE; + return validateSafeBinArgv(argv, profile); } export type ExecAllowlistEvaluation = { diff --git a/src/infra/exec-approvals.test.ts b/src/infra/exec-approvals.test.ts index 7daf609200..e3a3914efe 100644 --- a/src/infra/exec-approvals.test.ts +++ b/src/infra/exec-approvals.test.ts @@ -524,6 +524,64 @@ describe("exec approvals safe bins", () => { expect(defaults.has("sort")).toBe(false); expect(defaults.has("grep")).toBe(false); }); + + it("blocks sort output flags independent of file existence", () => { + if (process.platform === "win32") { + return; + } + const cwd = makeTempDir(); + fs.writeFileSync(path.join(cwd, "existing.txt"), "x"); + const resolution = { + rawExecutable: "sort", + resolvedPath: "/usr/bin/sort", + executableName: "sort", + }; + const safeBins = normalizeSafeBins(["sort"]); + const existing = isSafeBinUsage({ + argv: ["sort", "-o", "existing.txt"], + resolution, + safeBins, + cwd, + }); + const missing = isSafeBinUsage({ + argv: ["sort", "-o", "missing.txt"], + resolution, + safeBins, + cwd, + }); + const longFlag = isSafeBinUsage({ + argv: ["sort", "--output=missing.txt"], + resolution, + safeBins, + cwd, + }); + expect(existing).toBe(false); + expect(missing).toBe(false); + expect(longFlag).toBe(false); + }); + + it("does not consult file existence callbacks for safe-bin decisions", () => { + if (process.platform === "win32") { + return; + } + let checkedExists = false; + const ok = isSafeBinUsage({ + argv: ["sort", "-o", "target.txt"], + resolution: { + rawExecutable: "sort", + resolvedPath: "/usr/bin/sort", + executableName: "sort", + }, + safeBins: normalizeSafeBins(["sort"]), + cwd: "/tmp", + fileExists: () => { + checkedExists = true; + return true; + }, + }); + expect(ok).toBe(false); + expect(checkedExists).toBe(false); + }); }); describe("exec approvals allowlist evaluation", () => {