From 0ac939059e37fddddacb34bb6a7a5508e48ba6d3 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sun, 22 Mar 2026 10:14:42 -0700 Subject: [PATCH] refactor(exec): split safe-bin semantics --- docs/gateway/security/index.md | 1 + docs/tools/exec-approvals.md | 5 +- docs/tools/exec.md | 1 + .../doctor/shared/exec-safe-bins.test.ts | 24 ++++++-- src/commands/doctor/shared/exec-safe-bins.ts | 38 +++++++++++- src/infra/exec-approvals-allowlist.ts | 8 +-- src/infra/exec-approvals-analysis.ts | 2 +- src/infra/exec-command-resolution.ts | 2 - src/infra/exec-safe-bin-policy-profiles.ts | 25 ++++---- src/infra/exec-safe-bin-policy-validator.ts | 29 +++++++-- src/infra/exec-safe-bin-policy.test.ts | 26 ++++++-- src/infra/exec-safe-bin-policy.ts | 2 + src/infra/exec-safe-bin-runtime-policy.ts | 11 +--- src/infra/exec-safe-bin-semantics.ts | 60 +++++++++++++++++++ src/security/audit.test.ts | 40 +++++++++++++ src/security/audit.ts | 28 +++++++++ 16 files changed, 252 insertions(+), 50 deletions(-) create mode 100644 src/infra/exec-safe-bin-semantics.ts diff --git a/docs/gateway/security/index.md b/docs/gateway/security/index.md index 5f115c70d0b..416f6a487b4 100644 --- a/docs/gateway/security/index.md +++ b/docs/gateway/security/index.md @@ -259,6 +259,7 @@ High-signal `checkId` values you will most likely see in real deployments (not e | `tools.exec.auto_allow_skills_enabled` | warn | Exec approvals trust skill bins implicitly | `~/.openclaw/exec-approvals.json` | no | | `tools.exec.allowlist_interpreter_without_strict_inline_eval` | warn | Interpreter allowlists permit inline eval without forced reapproval | `tools.exec.strictInlineEval`, `agents.list[].tools.exec.strictInlineEval`, exec approvals allowlist | no | | `tools.exec.safe_bins_interpreter_unprofiled` | warn | Interpreter/runtime bins in `safeBins` without explicit profiles broaden exec risk | `tools.exec.safeBins`, `tools.exec.safeBinProfiles`, `agents.list[].tools.exec.*` | no | +| `tools.exec.safe_bins_broad_behavior` | warn | Broad-behavior tools in `safeBins` weaken the low-risk stdin-filter trust model | `tools.exec.safeBins`, `agents.list[].tools.exec.safeBins` | no | | `skills.workspace.symlink_escape` | warn | Workspace `skills/**/SKILL.md` resolves outside workspace root (symlink-chain drift) | workspace `skills/**` filesystem state | no | | `security.exposure.open_channels_with_exec` | warn/critical | Shared/public rooms can reach exec-enabled agents | `channels.*.dmPolicy`, `channels.*.groupPolicy`, `tools.exec.*`, `agents.list[].tools.exec.*` | no | | `security.exposure.open_groups_with_elevated` | critical | Open groups + elevated tools create high-impact prompt-injection paths | `channels.*.groupPolicy`, `tools.elevated.*` | no | diff --git a/docs/tools/exec-approvals.md b/docs/tools/exec-approvals.md index f2d4359cced..f8d499108b4 100644 --- a/docs/tools/exec-approvals.md +++ b/docs/tools/exec-approvals.md @@ -215,7 +215,10 @@ etc.) so inner executables are persisted instead of multiplexer binaries. If a w multiplexer cannot be safely unwrapped, no allowlist entry is persisted automatically. If you allowlist interpreters like `python3` or `node`, prefer `tools.exec.strictInlineEval=true` so inline eval still requires an explicit approval. -Default safe bins: `cut`, `uniq`, `head`, `tail`, `tr`, `wc`. +Default safe bins: +[//]: # "SAFE_BIN_DEFAULTS:START" +`cut`, `uniq`, `head`, `tail`, `tr`, `wc` +[//]: # "SAFE_BIN_DEFAULTS:END" `grep` and `sort` are not in the default list. If you opt in, keep explicit allowlist entries for their non-stdin workflows. diff --git a/docs/tools/exec.md b/docs/tools/exec.md index 9e914db46f0..a57a7f3cb6e 100644 --- a/docs/tools/exec.md +++ b/docs/tools/exec.md @@ -144,6 +144,7 @@ Use the two controls for different jobs: Do not treat `safeBins` as a generic allowlist, and do not add interpreter/runtime binaries (for example `python3`, `node`, `ruby`, `bash`). If you need those, use explicit allowlist entries and keep approval prompts enabled. `openclaw security audit` warns when interpreter/runtime `safeBins` entries are missing explicit profiles, and `openclaw doctor --fix` can scaffold missing custom `safeBinProfiles` entries. +`openclaw security audit` and `openclaw doctor` also warn when you explicitly add broad-behavior bins such as `jq` back into `safeBins`. If you explicitly allowlist interpreters, enable `tools.exec.strictInlineEval` so inline code-eval forms still require a fresh approval. For full policy details and examples, see [Exec approvals](/tools/exec-approvals#safe-bins-stdin-only) and [Safe bins versus allowlist](/tools/exec-approvals#safe-bins-versus-allowlist). diff --git a/src/commands/doctor/shared/exec-safe-bins.test.ts b/src/commands/doctor/shared/exec-safe-bins.test.ts index 74894506246..0dfe7058cf3 100644 --- a/src/commands/doctor/shared/exec-safe-bins.test.ts +++ b/src/commands/doctor/shared/exec-safe-bins.test.ts @@ -28,21 +28,36 @@ describe("doctor exec safe bin helpers", () => { }, } as OpenClawConfig); - expect(hits).toEqual([{ scopePath: "tools.exec", bin: "node", isInterpreter: true }]); + expect(hits).toEqual([ + { scopePath: "tools.exec", bin: "node", kind: "missingProfile", isInterpreter: true }, + { + scopePath: "tools.exec", + bin: "jq", + kind: "riskySemantics", + warning: + "jq supports broad jq programs and builtins (for example `env`), so prefer explicit allowlist entries or approval-gated runs instead of safeBins.", + }, + ]); }); it("formats coverage warnings", () => { const warnings = collectExecSafeBinCoverageWarnings({ hits: [ - { scopePath: "tools.exec", bin: "node", isInterpreter: true }, - { scopePath: "agents.list.runner.tools.exec", bin: "jq", isInterpreter: false }, + { scopePath: "tools.exec", bin: "node", kind: "missingProfile", isInterpreter: true }, + { + scopePath: "agents.list.runner.tools.exec", + bin: "jq", + kind: "riskySemantics", + warning: + "jq supports broad jq programs and builtins (for example `env`), so prefer explicit allowlist entries or approval-gated runs instead of safeBins.", + }, ], doctorFixCommand: "openclaw doctor --fix", }); expect(warnings).toEqual([ expect.stringContaining("tools.exec.safeBins includes interpreter/runtime 'node'"), - expect.stringContaining("agents.list.runner.tools.exec.safeBins entry 'jq'"), + expect.stringContaining("agents.list.runner.tools.exec.safeBins includes 'jq'"), expect.stringContaining('Run "openclaw doctor --fix"'), ]); }); @@ -60,6 +75,7 @@ describe("doctor exec safe bin helpers", () => { "- tools.exec.safeBinProfiles.jq: added scaffold profile {} (review and tighten flags/positionals).", ]); expect(result.warnings).toEqual([ + "- tools.exec.safeBins includes 'jq': jq supports broad jq programs and builtins (for example `env`), so prefer explicit allowlist entries or approval-gated runs instead of safeBins.", "- tools.exec.safeBins includes interpreter/runtime 'node' without profile; remove it from safeBins or use explicit allowlist entries.", ]); expect(result.config.tools?.exec?.safeBinProfiles).toEqual({ jq: {} }); diff --git a/src/commands/doctor/shared/exec-safe-bins.ts b/src/commands/doctor/shared/exec-safe-bins.ts index a3e695f180b..c90bd9d749c 100644 --- a/src/commands/doctor/shared/exec-safe-bins.ts +++ b/src/commands/doctor/shared/exec-safe-bins.ts @@ -4,6 +4,7 @@ import { listInterpreterLikeSafeBins, resolveMergedSafeBinProfileFixtures, } from "../../../infra/exec-safe-bin-runtime-policy.js"; +import { listRiskyConfiguredSafeBins } from "../../../infra/exec-safe-bin-semantics.js"; import { getTrustedSafeBinDirs, isTrustedSafeBinPath, @@ -15,7 +16,9 @@ import { asObjectRecord } from "./object.js"; export type ExecSafeBinCoverageHit = { scopePath: string; bin: string; - isInterpreter: boolean; + kind: "missingProfile" | "riskySemantics"; + isInterpreter?: boolean; + warning?: string; }; type ExecSafeBinScopeRef = { @@ -119,9 +122,18 @@ export function scanExecSafeBinCoverage(cfg: OpenClawConfig): ExecSafeBinCoverag hits.push({ scopePath: scope.scopePath, bin, + kind: "missingProfile", isInterpreter: interpreterBins.has(bin), }); } + for (const hit of listRiskyConfiguredSafeBins(scope.safeBins)) { + hits.push({ + scopePath: scope.scopePath, + bin: hit.bin, + kind: "riskySemantics", + warning: hit.warning, + }); + } } return hits; } @@ -161,8 +173,13 @@ export function collectExecSafeBinCoverageWarnings(params: { if (params.hits.length === 0) { return []; } - const interpreterHits = params.hits.filter((hit) => hit.isInterpreter); - const customHits = params.hits.filter((hit) => !hit.isInterpreter); + const interpreterHits = params.hits.filter( + (hit) => hit.kind === "missingProfile" && hit.isInterpreter, + ); + const customHits = params.hits.filter( + (hit) => hit.kind === "missingProfile" && !hit.isInterpreter, + ); + const riskyHits = params.hits.filter((hit) => hit.kind === "riskySemantics"); const lines: string[] = []; if (interpreterHits.length > 0) { for (const hit of interpreterHits.slice(0, 5)) { @@ -186,6 +203,16 @@ export function collectExecSafeBinCoverageWarnings(params: { lines.push(`- ${customHits.length - 5} more custom safeBins entries are missing profiles.`); } } + if (riskyHits.length > 0) { + for (const hit of riskyHits.slice(0, 5)) { + lines.push( + `- ${sanitizeForLog(hit.scopePath)}.safeBins includes '${sanitizeForLog(hit.bin)}': ${sanitizeForLog(hit.warning ?? "prefer explicit allowlist entries or approval-gated runs.")}`, + ); + } + if (riskyHits.length > 5) { + lines.push(`- ${riskyHits.length - 5} more safeBins entries should not use the low-risk safeBins fast path.`); + } + } lines.push( `- Run "${params.doctorFixCommand}" to scaffold missing custom safeBinProfiles entries.`, ); @@ -224,6 +251,11 @@ export function maybeRepairExecSafeBinProfiles(cfg: OpenClawConfig): { for (const scope of collectExecSafeBinScopes(next)) { const interpreterBins = new Set(listInterpreterLikeSafeBins(scope.safeBins)); + for (const hit of listRiskyConfiguredSafeBins(scope.safeBins)) { + warnings.push( + `- ${scope.scopePath}.safeBins includes '${hit.bin}': ${hit.warning}`, + ); + } const missingBins = scope.safeBins.filter((bin) => !scope.mergedProfiles[bin]); if (missingBins.length === 0) { continue; diff --git a/src/infra/exec-approvals-allowlist.ts b/src/infra/exec-approvals-allowlist.ts index 80d9ee32492..113e959958d 100644 --- a/src/infra/exec-approvals-allowlist.ts +++ b/src/infra/exec-approvals-allowlist.ts @@ -1,6 +1,5 @@ import path from "node:path"; import { - DEFAULT_SAFE_BINS, analyzeShellCommand, isWindowsPlatform, matchAllowlist, @@ -13,6 +12,7 @@ import { } from "./exec-approvals-analysis.js"; import type { ExecAllowlistEntry } from "./exec-approvals.js"; import { + DEFAULT_SAFE_BINS, SAFE_BIN_PROFILES, type SafeBinProfile, validateSafeBinArgv, @@ -31,7 +31,7 @@ function hasShellLineContinuation(command: string): boolean { return /\\(?:\r\n|\n|\r)/.test(command); } -export function normalizeSafeBins(entries?: string[]): Set { +export function normalizeSafeBins(entries?: readonly string[]): Set { if (!Array.isArray(entries)) { return new Set(); } @@ -41,7 +41,7 @@ export function normalizeSafeBins(entries?: string[]): Set { return new Set(normalized); } -export function resolveSafeBins(entries?: string[] | null): Set { +export function resolveSafeBins(entries?: readonly string[] | null): Set { if (entries === undefined) { return normalizeSafeBins(DEFAULT_SAFE_BINS); } @@ -92,7 +92,7 @@ export function isSafeBinUsage(params: { if (!profile) { return false; } - return validateSafeBinArgv(argv, profile); + return validateSafeBinArgv(argv, profile, { binName: execName }); } function isPathScopedExecutableToken(token: string): boolean { diff --git a/src/infra/exec-approvals-analysis.ts b/src/infra/exec-approvals-analysis.ts index f55f7c56c53..8f2a6a69008 100644 --- a/src/infra/exec-approvals-analysis.ts +++ b/src/infra/exec-approvals-analysis.ts @@ -3,9 +3,9 @@ import { resolveCommandResolutionFromArgv, type CommandResolution, } from "./exec-command-resolution.js"; +export { DEFAULT_SAFE_BINS } from "./exec-safe-bin-policy.js"; export { - DEFAULT_SAFE_BINS, matchAllowlist, parseExecArgvToken, resolveAllowlistCandidatePath, diff --git a/src/infra/exec-command-resolution.ts b/src/infra/exec-command-resolution.ts index ae6bbc55e30..971f197c8ff 100644 --- a/src/infra/exec-command-resolution.ts +++ b/src/infra/exec-command-resolution.ts @@ -6,8 +6,6 @@ import { resolveDispatchWrapperExecutionPlan } from "./exec-wrapper-resolution.j import { resolveExecutablePath as resolveExecutableCandidatePath } from "./executable-path.js"; import { expandHomePrefix } from "./home-dir.js"; -export const DEFAULT_SAFE_BINS = ["cut", "uniq", "head", "tail", "tr", "wc"]; - export type CommandResolution = { rawExecutable: string; resolvedPath?: string; diff --git a/src/infra/exec-safe-bin-policy-profiles.ts b/src/infra/exec-safe-bin-policy-profiles.ts index 1602a0f2d96..c3c0cdaed45 100644 --- a/src/infra/exec-safe-bin-policy-profiles.ts +++ b/src/infra/exec-safe-bin-policy-profiles.ts @@ -3,7 +3,6 @@ export type SafeBinProfile = { maxPositional?: number; allowedValueFlags?: ReadonlySet; deniedFlags?: ReadonlySet; - positionalValidator?: (positional: readonly string[]) => boolean; // Precomputed long-option metadata for GNU abbreviation resolution. knownLongFlags?: readonly string[]; knownLongFlagsSet?: ReadonlySet; @@ -21,6 +20,8 @@ export type SafeBinProfileFixtures = Readonly = new Set(); +export const DEFAULT_SAFE_BINS = ["cut", "uniq", "head", "tail", "tr", "wc"] as const; + const toFlagSet = (flags?: readonly string[]): ReadonlySet => { if (!flags || flags.length === 0) { return NO_FLAGS; @@ -69,18 +70,7 @@ export function buildLongFlagPrefixMap( return prefixMap; } -const JQ_ENV_FILTER_PATTERN = /(^|[^.$A-Za-z0-9_])env([^A-Za-z0-9_]|$)/; - -function validateJqSafeBinPositional(positional: readonly string[]): boolean { - for (const token of positional) { - if (JQ_ENV_FILTER_PATTERN.test(token)) { - return false; - } - } - return true; -} - -function compileSafeBinProfile(name: string, fixture: SafeBinProfileFixture): SafeBinProfile { +function compileSafeBinProfile(fixture: SafeBinProfileFixture): SafeBinProfile { const allowedValueFlags = toFlagSet(fixture.allowedValueFlags); const deniedFlags = toFlagSet(fixture.deniedFlags); const knownLongFlags = collectKnownLongFlags(allowedValueFlags, deniedFlags); @@ -89,7 +79,6 @@ function compileSafeBinProfile(name: string, fixture: SafeBinProfileFixture): Sa maxPositional: fixture.maxPositional, allowedValueFlags, deniedFlags, - positionalValidator: name === "jq" ? validateJqSafeBinPositional : undefined, knownLongFlags, knownLongFlagsSet: new Set(knownLongFlags), longFlagPrefixMap: buildLongFlagPrefixMap(knownLongFlags), @@ -100,7 +89,7 @@ function compileSafeBinProfiles( fixtures: Record, ): Record { return Object.fromEntries( - Object.entries(fixtures).map(([name, fixture]) => [name, compileSafeBinProfile(name, fixture)]), + Object.entries(fixtures).map(([name, fixture]) => [name, compileSafeBinProfile(fixture)]), ) as Record; } @@ -326,3 +315,9 @@ export function renderSafeBinDeniedFlagsDocBullets( .map((bin) => `- \`${bin}\`: ${deniedByBin[bin].map((flag) => `\`${flag}\``).join(", ")}`) .join("\n"); } + +export function renderDefaultSafeBinsDocText( + defaults: readonly string[] = DEFAULT_SAFE_BINS, +): string { + return defaults.map((bin) => `\`${bin}\``).join(", "); +} diff --git a/src/infra/exec-safe-bin-policy-validator.ts b/src/infra/exec-safe-bin-policy-validator.ts index a495815c8da..cd40d0dac19 100644 --- a/src/infra/exec-safe-bin-policy-validator.ts +++ b/src/infra/exec-safe-bin-policy-validator.ts @@ -1,4 +1,5 @@ import { parseExecArgvToken } from "./exec-approvals-analysis.js"; +import { validateSafeBinSemantics } from "./exec-safe-bin-semantics.js"; import { buildLongFlagPrefixMap, collectKnownLongFlags, @@ -130,10 +131,10 @@ function validatePositionalCount(positional: string[], profile: SafeBinProfile): if (typeof profile.maxPositional === "number" && positional.length > profile.maxPositional) { return false; } - return profile.positionalValidator?.(positional) ?? true; + return true; } -export function validateSafeBinArgv(args: string[], profile: SafeBinProfile): boolean { +function collectPositionalTokens(args: string[], profile: SafeBinProfile): string[] | null { const allowedValueFlags = profile.allowedValueFlags ?? NO_FLAGS; const deniedFlags = profile.deniedFlags ?? NO_FLAGS; const knownLongFlags = @@ -185,7 +186,7 @@ export function validateSafeBinArgv(args: string[], profile: SafeBinProfile): bo longFlagPrefixMap, }); if (nextIndex < 0) { - return false; + return null; } i = nextIndex; continue; @@ -200,10 +201,28 @@ export function validateSafeBinArgv(args: string[], profile: SafeBinProfile): bo deniedFlags, }); if (nextIndex < 0) { - return false; + return null; } i = nextIndex; } - return validatePositionalCount(positional, profile); + return positional; +} + +export function validateSafeBinArgv( + args: string[], + profile: SafeBinProfile, + options?: { binName?: string }, +): boolean { + const positional = collectPositionalTokens(args, profile); + if (!positional) { + return false; + } + if (!validatePositionalCount(positional, profile)) { + return false; + } + return validateSafeBinSemantics({ + binName: options?.binName, + positional, + }); } diff --git a/src/infra/exec-safe-bin-policy.test.ts b/src/infra/exec-safe-bin-policy.test.ts index bc9735050a0..679b77e8009 100644 --- a/src/infra/exec-safe-bin-policy.test.ts +++ b/src/infra/exec-safe-bin-policy.test.ts @@ -2,14 +2,18 @@ import fs from "node:fs"; import path from "node:path"; import { describe, expect, it } from "vitest"; import { + DEFAULT_SAFE_BINS, SAFE_BIN_PROFILE_FIXTURES, SAFE_BIN_PROFILES, buildLongFlagPrefixMap, collectKnownLongFlags, + renderDefaultSafeBinsDocText, renderSafeBinDeniedFlagsDocBullets, validateSafeBinArgv, } from "./exec-safe-bin-policy.js"; +const SAFE_BIN_DOC_DEFAULTS_START = '[//]: # "SAFE_BIN_DEFAULTS:START"'; +const SAFE_BIN_DOC_DEFAULTS_END = '[//]: # "SAFE_BIN_DEFAULTS:END"'; const SAFE_BIN_DOC_DENIED_FLAGS_START = '[//]: # "SAFE_BIN_DENIED_FLAGS:START"'; const SAFE_BIN_DOC_DENIED_FLAGS_END = '[//]: # "SAFE_BIN_DENIED_FLAGS:END"'; @@ -48,14 +52,14 @@ describe("exec safe bin policy jq", () => { const jqProfile = SAFE_BIN_PROFILES.jq; it("allows normal jq field filters", () => { - expect(validateSafeBinArgv([".foo"], jqProfile)).toBe(true); - expect(validateSafeBinArgv([".env"], jqProfile)).toBe(true); + expect(validateSafeBinArgv([".foo"], jqProfile, { binName: "jq" })).toBe(true); + expect(validateSafeBinArgv([".env"], jqProfile, { binName: "jq" })).toBe(true); }); it("blocks jq env builtin filters in safe-bin mode", () => { - expect(validateSafeBinArgv(["env"], jqProfile)).toBe(false); - expect(validateSafeBinArgv(["env.FOO"], jqProfile)).toBe(false); - expect(validateSafeBinArgv([".foo | env"], jqProfile)).toBe(false); + expect(validateSafeBinArgv(["env"], jqProfile, { binName: "jq" })).toBe(false); + expect(validateSafeBinArgv(["env.FOO"], jqProfile, { binName: "jq" })).toBe(false); + expect(validateSafeBinArgv([".foo | env"], jqProfile, { binName: "jq" })).toBe(false); }); }); @@ -160,6 +164,18 @@ describe("exec safe bin policy denied-flag matrix", () => { }); describe("exec safe bin policy docs parity", () => { + it("keeps default safe-bin docs in sync with policy defaults", () => { + const docsPath = path.resolve(process.cwd(), "docs/tools/exec-approvals.md"); + const docs = fs.readFileSync(docsPath, "utf8").replaceAll("\r\n", "\n"); + const start = docs.indexOf(SAFE_BIN_DOC_DEFAULTS_START); + const end = docs.indexOf(SAFE_BIN_DOC_DEFAULTS_END); + expect(start).toBeGreaterThanOrEqual(0); + expect(end).toBeGreaterThan(start); + const actual = docs.slice(start + SAFE_BIN_DOC_DEFAULTS_START.length, end).trim(); + const expected = renderDefaultSafeBinsDocText(DEFAULT_SAFE_BINS); + expect(actual).toBe(expected); + }); + it("keeps denied-flag docs in sync with policy fixtures", () => { const docsPath = path.resolve(process.cwd(), "docs/tools/exec-approvals.md"); const docs = fs.readFileSync(docsPath, "utf8").replaceAll("\r\n", "\n"); diff --git a/src/infra/exec-safe-bin-policy.ts b/src/infra/exec-safe-bin-policy.ts index cd859809828..09f06e0b038 100644 --- a/src/infra/exec-safe-bin-policy.ts +++ b/src/infra/exec-safe-bin-policy.ts @@ -1,9 +1,11 @@ export { + DEFAULT_SAFE_BINS, SAFE_BIN_PROFILE_FIXTURES, SAFE_BIN_PROFILES, buildLongFlagPrefixMap, collectKnownLongFlags, normalizeSafeBinProfileFixtures, + renderDefaultSafeBinsDocText, renderSafeBinDeniedFlagsDocBullets, resolveSafeBinDeniedFlags, resolveSafeBinProfiles, diff --git a/src/infra/exec-safe-bin-runtime-policy.ts b/src/infra/exec-safe-bin-runtime-policy.ts index 203f9d0761f..4a483040dd2 100644 --- a/src/infra/exec-safe-bin-runtime-policy.ts +++ b/src/infra/exec-safe-bin-runtime-policy.ts @@ -6,6 +6,7 @@ import { type SafeBinProfileFixture, type SafeBinProfileFixtures, } from "./exec-safe-bin-policy.js"; +import { normalizeSafeBinName } from "./exec-safe-bin-semantics.js"; import { getTrustedSafeBinDirs, listWritableExplicitTrustedSafeBinDirs, @@ -59,16 +60,6 @@ const INTERPRETER_LIKE_PATTERNS = [ /^node\d+(?:\.\d+)?$/, ]; -function normalizeSafeBinName(raw: string): string { - const trimmed = raw.trim().toLowerCase(); - if (!trimmed) { - return ""; - } - const tail = trimmed.split(/[\\/]/).at(-1); - const normalized = tail ?? trimmed; - return normalized.replace(/\.(?:exe|cmd|bat|com)$/i, ""); -} - export function isInterpreterLikeSafeBin(raw: string): boolean { const normalized = normalizeSafeBinName(raw); if (!normalized) { diff --git a/src/infra/exec-safe-bin-semantics.ts b/src/infra/exec-safe-bin-semantics.ts new file mode 100644 index 00000000000..e764ea17a03 --- /dev/null +++ b/src/infra/exec-safe-bin-semantics.ts @@ -0,0 +1,60 @@ +export type SafeBinSemanticValidationParams = { + binName?: string; + positional: readonly string[]; +}; + +type SafeBinSemanticRule = { + validate?: (params: SafeBinSemanticValidationParams) => boolean; + configWarning?: string; +}; + +const JQ_ENV_FILTER_PATTERN = /(^|[^.$A-Za-z0-9_])env([^A-Za-z0-9_]|$)/; + +const SAFE_BIN_SEMANTIC_RULES: Readonly> = { + jq: { + validate: ({ positional }) => + !positional.some((token) => JQ_ENV_FILTER_PATTERN.test(token)), + configWarning: + "jq supports broad jq programs and builtins (for example `env`), so prefer explicit allowlist entries or approval-gated runs instead of safeBins.", + }, +}; + +export function normalizeSafeBinName(raw: string): string { + const trimmed = raw.trim().toLowerCase(); + if (!trimmed) { + return ""; + } + const tail = trimmed.split(/[\\/]/).at(-1); + const normalized = tail ?? trimmed; + return normalized.replace(/\.(?:exe|cmd|bat|com)$/i, ""); +} + +export function getSafeBinSemanticRule(binName?: string): SafeBinSemanticRule | undefined { + const normalized = typeof binName === "string" ? normalizeSafeBinName(binName) : ""; + return normalized ? SAFE_BIN_SEMANTIC_RULES[normalized] : undefined; +} + +export function validateSafeBinSemantics(params: SafeBinSemanticValidationParams): boolean { + return getSafeBinSemanticRule(params.binName)?.validate?.(params) ?? true; +} + +export function listRiskyConfiguredSafeBins(entries: Iterable): Array<{ + bin: string; + warning: string; +}> { + const hits = new Map(); + for (const entry of entries) { + const normalized = normalizeSafeBinName(entry); + if (!normalized || hits.has(normalized)) { + continue; + } + const warning = getSafeBinSemanticRule(normalized)?.configWarning; + if (!warning) { + continue; + } + hits.set(normalized, warning); + } + return Array.from(hits.entries()) + .map(([bin, warning]) => ({ bin, warning })) + .toSorted((a, b) => a.bin.localeCompare(b.bin)); +} diff --git a/src/security/audit.test.ts b/src/security/audit.test.ts index 5da45e61b15..1f4c5297c67 100644 --- a/src/security/audit.test.ts +++ b/src/security/audit.test.ts @@ -686,6 +686,46 @@ description: test skill ); }); + it("warns when risky broad-behavior bins are explicitly added to safeBins", async () => { + const cases: Array<{ + name: string; + cfg: OpenClawConfig; + expected: boolean; + }> = [ + { + name: "jq configured globally", + cfg: { + tools: { + exec: { + safeBins: ["jq"], + }, + }, + }, + expected: true, + }, + { + name: "jq not configured", + cfg: { + tools: { + exec: { + safeBins: ["cut"], + }, + }, + }, + expected: false, + }, + ]; + await Promise.all( + cases.map(async (testCase) => { + const res = await audit(testCase.cfg); + expect( + hasFinding(res, "tools.exec.safe_bins_broad_behavior", "warn"), + testCase.name, + ).toBe(testCase.expected); + }), + ); + }); + it("evaluates safeBinTrustedDirs risk findings", async () => { const riskyGlobalTrustedDirs = process.platform === "win32" diff --git a/src/security/audit.ts b/src/security/audit.ts index 7aeba20db8f..d0d78582ad2 100644 --- a/src/security/audit.ts +++ b/src/security/audit.ts @@ -17,6 +17,7 @@ import { listInterpreterLikeSafeBins, resolveMergedSafeBinProfileFixtures, } from "../infra/exec-safe-bin-runtime-policy.js"; +import { listRiskyConfiguredSafeBins } from "../infra/exec-safe-bin-semantics.js"; import { normalizeTrustedSafeBinDirs } from "../infra/exec-safe-bin-trust.js"; import { isBlockedHostnameOrIp, isPrivateNetworkAllowedByPolicy } from "../infra/net/ssrf.js"; import { DEFAULT_AGENT_ID } from "../routing/session-key.js"; @@ -1111,6 +1112,7 @@ function collectExecRuntimeFindings(cfg: OpenClawConfig): SecurityAuditFinding[] } const interpreterHits: string[] = []; + const riskySemanticSafeBinHits: string[] = []; const globalSafeBins = normalizeConfiguredSafeBins(globalExec?.safeBins); if (globalSafeBins.length > 0) { const merged = resolveMergedSafeBinProfileFixtures({ global: globalExec }) ?? {}; @@ -1118,6 +1120,9 @@ function collectExecRuntimeFindings(cfg: OpenClawConfig): SecurityAuditFinding[] if (interpreters.length > 0) { interpreterHits.push(`- tools.exec.safeBins: ${interpreters.join(", ")}`); } + for (const hit of listRiskyConfiguredSafeBins(globalSafeBins)) { + riskySemanticSafeBinHits.push(`- tools.exec.safeBins: ${hit.bin} (${hit.warning})`); + } } for (const entry of agents) { @@ -1136,11 +1141,21 @@ function collectExecRuntimeFindings(cfg: OpenClawConfig): SecurityAuditFinding[] }) ?? {}; const interpreters = listInterpreterLikeSafeBins(agentSafeBins).filter((bin) => !merged[bin]); if (interpreters.length === 0) { + for (const hit of listRiskyConfiguredSafeBins(agentSafeBins)) { + riskySemanticSafeBinHits.push( + `- agents.list.${entry.id}.tools.exec.safeBins: ${hit.bin} (${hit.warning})`, + ); + } continue; } interpreterHits.push( `- agents.list.${entry.id}.tools.exec.safeBins: ${interpreters.join(", ")}`, ); + for (const hit of listRiskyConfiguredSafeBins(agentSafeBins)) { + riskySemanticSafeBinHits.push( + `- agents.list.${entry.id}.tools.exec.safeBins: ${hit.bin} (${hit.warning})`, + ); + } } if (interpreterHits.length > 0) { @@ -1156,6 +1171,19 @@ function collectExecRuntimeFindings(cfg: OpenClawConfig): SecurityAuditFinding[] }); } + if (riskySemanticSafeBinHits.length > 0) { + findings.push({ + checkId: "tools.exec.safe_bins_broad_behavior", + severity: "warn", + title: "safeBins includes binaries with broader semantics than low-risk stream filters", + detail: + `Detected risky safeBins entries:\n${riskySemanticSafeBinHits.join("\n")}\n` + + "These tools expose semantics that do not fit the low-risk stdin-filter fast path.", + remediation: + "Remove these binaries from safeBins and prefer explicit allowlist entries or approval-gated execution.", + }); + } + if (riskyTrustedDirHits.length > 0) { findings.push({ checkId: "tools.exec.safe_bin_trusted_dirs_risky",