diff --git a/CHANGELOG.md b/CHANGELOG.md index 034c524dddf..35ebbde8f1f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,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. - 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 1243675ec3c..dffa71a0284 100644 --- a/docs/tools/exec-approvals.md +++ b/docs/tools/exec-approvals.md @@ -127,6 +127,8 @@ positional file args and path-like tokens, so they can only operate on the incom 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. +Safe bins must also resolve from trusted binary directories (system defaults plus the gateway +process `PATH` at startup). This blocks request-scoped PATH hijacking attempts. 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 70770af9f6f..dd222e0bdad 100644 --- a/docs/tools/exec.md +++ b/docs/tools/exec.md @@ -51,7 +51,7 @@ Notes: - `tools.exec.ask` (default: `on-miss`) - `tools.exec.node` (default: unset) - `tools.exec.pathPrepend`: list of directories to prepend to `PATH` for exec runs (gateway + sandbox only). -- `tools.exec.safeBins`: stdin-only safe binaries that can run without explicit allowlist entries. +- `tools.exec.safeBins`: stdin-only safe binaries that can run without explicit allowlist entries (resolved path must come from trusted binary directories). Example: diff --git a/src/infra/exec-approvals-allowlist.ts b/src/infra/exec-approvals-allowlist.ts index deda16e444b..76fa424c9db 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,48 @@ import { type CommandResolution, type ExecCommandSegment, } from "./exec-approvals-analysis.js"; -import type { ExecAllowlistEntry } from "./exec-approvals.js"; + +const DEFAULT_SAFE_BIN_TRUSTED_DIRS = [ + "/bin", + "/usr/bin", + "/usr/local/bin", + "/opt/homebrew/bin", + "/opt/local/bin", + "/snap/bin", + "/run/current-system/sw/bin", +]; + +function normalizeTrustedDir(value: string): string | null { + const trimmed = value.trim(); + if (!trimmed) { + return null; + } + return path.resolve(trimmed); +} + +function collectTrustedSafeBinDirs(): Set { + const trusted = new Set(); + for (const entry of DEFAULT_SAFE_BIN_TRUSTED_DIRS) { + const normalized = normalizeTrustedDir(entry); + if (normalized) { + trusted.add(normalized); + } + } + const pathEntries = (process.env.PATH ?? process.env.Path ?? "") + .split(path.delimiter) + .map((entry) => normalizeTrustedDir(entry)) + .filter((entry): entry is string => Boolean(entry)); + for (const entry of pathEntries) { + trusted.add(entry); + } + return trusted; +} + +const TRUSTED_SAFE_BIN_DIRS = collectTrustedSafeBinDirs(); + +function isTrustedSafeBinPath(resolvedPath: string): boolean { + return TRUSTED_SAFE_BIN_DIRS.has(path.dirname(path.resolve(resolvedPath))); +} function isPathLikeToken(value: string): boolean { const trimmed = value.trim(); @@ -90,6 +132,9 @@ export function isSafeBinUsage(params: { if (!resolution?.resolvedPath) { return false; } + if (!isTrustedSafeBinPath(resolution.resolvedPath)) { + return false; + } const cwd = params.cwd ?? process.cwd(); const exists = params.fileExists ?? defaultFileExists; const argv = params.argv.slice(1); diff --git a/src/infra/exec-approvals.test.ts b/src/infra/exec-approvals.test.ts index 3959b2ef4cb..93d10979dd6 100644 --- a/src/infra/exec-approvals.test.ts +++ b/src/infra/exec-approvals.test.ts @@ -34,26 +34,6 @@ function makeTempDir() { return fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-exec-approvals-")); } -function createSafeBinJqCase(params: { command: string; seedFileName?: string }) { - const dir = makeTempDir(); - const binDir = path.join(dir, "bin"); - fs.mkdirSync(binDir, { recursive: true }); - const exeName = process.platform === "win32" ? "jq.exe" : "jq"; - const exe = path.join(binDir, exeName); - fs.writeFileSync(exe, ""); - fs.chmodSync(exe, 0o755); - if (params.seedFileName) { - fs.writeFileSync(path.join(dir, params.seedFileName), "{}"); - } - const res = analyzeShellCommand({ - command: params.command, - cwd: dir, - env: makePathEnv(binDir), - }); - expect(res.ok).toBe(true); - return { dir, segment: res.segments[0] }; -} - describe("exec approvals allowlist matching", () => { it("ignores basename-only patterns", () => { const resolution = { @@ -409,10 +389,14 @@ describe("exec approvals safe bins", () => { if (process.platform === "win32") { return; } - const { dir, segment } = createSafeBinJqCase({ command: "jq .foo" }); + const dir = makeTempDir(); const ok = isSafeBinUsage({ - argv: segment.argv, - resolution: segment.resolution, + argv: ["jq", ".foo"], + resolution: { + rawExecutable: "jq", + resolvedPath: "/usr/bin/jq", + executableName: "jq", + }, safeBins: normalizeSafeBins(["jq"]), cwd: dir, }); @@ -423,18 +407,37 @@ describe("exec approvals safe bins", () => { if (process.platform === "win32") { return; } - const { dir, segment } = createSafeBinJqCase({ - command: "jq .foo secret.json", - seedFileName: "secret.json", - }); + const dir = makeTempDir(); + fs.writeFileSync(path.join(dir, "secret.json"), "{}"); const ok = isSafeBinUsage({ - argv: segment.argv, - resolution: segment.resolution, + argv: ["jq", ".foo", "secret.json"], + resolution: { + rawExecutable: "jq", + resolvedPath: "/usr/bin/jq", + executableName: "jq", + }, safeBins: normalizeSafeBins(["jq"]), cwd: dir, }); expect(ok).toBe(false); }); + + it("blocks safe bins resolved from untrusted directories", () => { + if (process.platform === "win32") { + return; + } + const ok = isSafeBinUsage({ + argv: ["jq", ".foo"], + resolution: { + rawExecutable: "jq", + resolvedPath: "/tmp/evil-bin/jq", + executableName: "jq", + }, + safeBins: normalizeSafeBins(["jq"]), + cwd: "/tmp", + }); + expect(ok).toBe(false); + }); }); describe("exec approvals allowlist evaluation", () => {