mirror of
https://github.com/openclaw/openclaw.git
synced 2026-02-19 18:39:20 -05:00
fix(security): eliminate safeBins file-existence oracle
This commit is contained in:
@@ -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.
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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 () => {
|
||||
|
||||
@@ -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<string> {
|
||||
if (!Array.isArray(entries)) {
|
||||
return new Set();
|
||||
@@ -62,57 +53,253 @@ function hasGlobToken(value: string): boolean {
|
||||
return /[*?[\]]/.test(value);
|
||||
}
|
||||
|
||||
type SafeBinOptionPolicy = {
|
||||
blockedShort?: ReadonlySet<string>;
|
||||
blockedLong?: ReadonlySet<string>;
|
||||
type SafeBinProfile = {
|
||||
minPositional?: number;
|
||||
maxPositional?: number;
|
||||
valueFlags?: ReadonlySet<string>;
|
||||
blockedFlags?: ReadonlySet<string>;
|
||||
};
|
||||
|
||||
const SAFE_BIN_OPTION_POLICIES: Readonly<Record<string, SafeBinOptionPolicy>> = {
|
||||
// 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<string>();
|
||||
const SAFE_BIN_GENERIC_PROFILE: SafeBinProfile = {};
|
||||
const SAFE_BIN_PROFILES: Record<string, SafeBinProfile> = {
|
||||
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 = {
|
||||
|
||||
@@ -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", () => {
|
||||
|
||||
Reference in New Issue
Block a user