diff --git a/src/agents/pi-tools.safe-bins.e2e.test.ts b/src/agents/pi-tools.safe-bins.e2e.test.ts index f2d48e5314..3cf93bffc3 100644 --- a/src/agents/pi-tools.safe-bins.e2e.test.ts +++ b/src/agents/pi-tools.safe-bins.e2e.test.ts @@ -222,6 +222,26 @@ describe("createOpenClawCodingTools safeBins", () => { } }); + it("blocks shell redirection metacharacters in safeBins mode", async () => { + if (process.platform === "win32") { + return; + } + + const { tmpDir, execTool } = await createSafeBinsExecTool({ + tmpPrefix: "openclaw-safe-bins-redirect-", + safeBins: ["head"], + files: [{ name: "source.txt", contents: "line1\nline2\n" }], + }); + + await expect( + execTool.execute("call1", { + command: "head -n 1 source.txt > blocked-redirect.txt", + workdir: tmpDir, + }), + ).rejects.toThrow("exec denied: allowlist miss"); + expect(fs.existsSync(path.join(tmpDir, "blocked-redirect.txt"))).toBe(false); + }); + it("blocks grep recursive flags from reading cwd via safeBins", async () => { if (process.platform === "win32") { return; diff --git a/src/channels/plugins/message-actions.security.test.ts b/src/channels/plugins/message-actions.security.test.ts index 530bf5de01..0ca6ec36d1 100644 --- a/src/channels/plugins/message-actions.security.test.ts +++ b/src/channels/plugins/message-actions.security.test.ts @@ -1,10 +1,10 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; -import type { OpenClawConfig } from "../../config/config.js"; -import type { ChannelPlugin } from "./types.js"; import { jsonResult } from "../../agents/tools/common.js"; +import type { OpenClawConfig } from "../../config/config.js"; import { setActivePluginRegistry } from "../../plugins/runtime.js"; import { createTestRegistry } from "../../test-utils/channel-plugins.js"; import { dispatchChannelMessageAction } from "./message-actions.js"; +import type { ChannelPlugin } from "./types.js"; const handleAction = vi.fn(async () => jsonResult({ ok: true })); diff --git a/src/channels/plugins/message-actions.ts b/src/channels/plugins/message-actions.ts index 35dc74d245..da242fa436 100644 --- a/src/channels/plugins/message-actions.ts +++ b/src/channels/plugins/message-actions.ts @@ -1,7 +1,7 @@ import type { AgentToolResult } from "@mariozechner/pi-agent-core"; import type { OpenClawConfig } from "../../config/config.js"; -import type { ChannelMessageActionContext, ChannelMessageActionName } from "./types.js"; import { getChannelPlugin, listChannelPlugins } from "./index.js"; +import type { ChannelMessageActionContext, ChannelMessageActionName } from "./types.js"; const trustedRequesterRequiredByChannel: Readonly< Partial>> diff --git a/src/infra/exec-approvals-allowlist.ts b/src/infra/exec-approvals-allowlist.ts index 03b0c48ec4..b7334f4ed0 100644 --- a/src/infra/exec-approvals-allowlist.ts +++ b/src/infra/exec-approvals-allowlist.ts @@ -13,6 +13,7 @@ import type { ExecAllowlistEntry } from "./exec-approvals.js"; import { SAFE_BIN_GENERIC_PROFILE, SAFE_BIN_PROFILES, + type SafeBinProfile, validateSafeBinArgv, } from "./exec-safe-bin-policy.js"; import { isTrustedSafeBinPath } from "./exec-safe-bin-trust.js"; @@ -38,11 +39,15 @@ export function isSafeBinUsage(params: { argv: string[]; resolution: CommandResolution | null; safeBins: Set; + platform?: string | null; trustedSafeBinDirs?: ReadonlySet; + safeBinProfiles?: Readonly>; + safeBinGenericProfile?: SafeBinProfile; + isTrustedSafeBinPathFn?: typeof isTrustedSafeBinPath; }): boolean { // Windows host exec uses PowerShell, which has different parsing/expansion rules. // Keep safeBins conservative there (require explicit allowlist entries). - if (isWindowsPlatform(process.platform)) { + if (isWindowsPlatform(params.platform ?? process.platform)) { return false; } if (params.safeBins.size === 0) { @@ -60,8 +65,9 @@ export function isSafeBinUsage(params: { if (!resolution?.resolvedPath) { return false; } + const isTrustedPath = params.isTrustedSafeBinPathFn ?? isTrustedSafeBinPath; if ( - !isTrustedSafeBinPath({ + !isTrustedPath({ resolvedPath: resolution.resolvedPath, trustedDirs: params.trustedSafeBinDirs, }) @@ -69,7 +75,9 @@ export function isSafeBinUsage(params: { return false; } const argv = params.argv.slice(1); - const profile = SAFE_BIN_PROFILES[execName] ?? SAFE_BIN_GENERIC_PROFILE; + const safeBinProfiles = params.safeBinProfiles ?? SAFE_BIN_PROFILES; + const genericSafeBinProfile = params.safeBinGenericProfile ?? SAFE_BIN_GENERIC_PROFILE; + const profile = safeBinProfiles[execName] ?? genericSafeBinProfile; return validateSafeBinArgv(argv, profile); } @@ -87,6 +95,7 @@ function evaluateSegments( allowlist: ExecAllowlistEntry[]; safeBins: Set; cwd?: string; + platform?: string | null; trustedSafeBinDirs?: ReadonlySet; skillBins?: Set; autoAllowSkills?: boolean; @@ -114,6 +123,7 @@ function evaluateSegments( argv: segment.argv, resolution: segment.resolution, safeBins: params.safeBins, + platform: params.platform, trustedSafeBinDirs: params.trustedSafeBinDirs, }); const skillAllow = @@ -139,6 +149,7 @@ export function evaluateExecAllowlist(params: { allowlist: ExecAllowlistEntry[]; safeBins: Set; cwd?: string; + platform?: string | null; trustedSafeBinDirs?: ReadonlySet; skillBins?: Set; autoAllowSkills?: boolean; @@ -156,6 +167,7 @@ export function evaluateExecAllowlist(params: { allowlist: params.allowlist, safeBins: params.safeBins, cwd: params.cwd, + platform: params.platform, trustedSafeBinDirs: params.trustedSafeBinDirs, skillBins: params.skillBins, autoAllowSkills: params.autoAllowSkills, @@ -174,6 +186,7 @@ export function evaluateExecAllowlist(params: { allowlist: params.allowlist, safeBins: params.safeBins, cwd: params.cwd, + platform: params.platform, trustedSafeBinDirs: params.trustedSafeBinDirs, skillBins: params.skillBins, autoAllowSkills: params.autoAllowSkills, @@ -231,6 +244,7 @@ export function evaluateShellAllowlist(params: { allowlist: params.allowlist, safeBins: params.safeBins, cwd: params.cwd, + platform: params.platform, trustedSafeBinDirs: params.trustedSafeBinDirs, skillBins: params.skillBins, autoAllowSkills: params.autoAllowSkills, @@ -265,6 +279,7 @@ export function evaluateShellAllowlist(params: { allowlist: params.allowlist, safeBins: params.safeBins, cwd: params.cwd, + platform: params.platform, trustedSafeBinDirs: params.trustedSafeBinDirs, skillBins: params.skillBins, autoAllowSkills: params.autoAllowSkills, diff --git a/src/infra/exec-approvals-analysis.ts b/src/infra/exec-approvals-analysis.ts index 2a197adbea..7971e9b8f7 100644 --- a/src/infra/exec-approvals-analysis.ts +++ b/src/infra/exec-approvals-analysis.ts @@ -228,6 +228,78 @@ export type ExecCommandSegment = { resolution: CommandResolution | null; }; +export type ExecArgvToken = + | { + kind: "empty"; + raw: string; + } + | { + kind: "terminator"; + raw: string; + } + | { + kind: "stdin"; + raw: string; + } + | { + kind: "positional"; + raw: string; + } + | { + kind: "option"; + raw: string; + style: "long"; + flag: string; + inlineValue?: string; + } + | { + kind: "option"; + raw: string; + style: "short-cluster"; + cluster: string; + flags: string[]; + }; + +/** + * Tokenizes a single argv entry into a normalized option/positional model. + * Consumers can share this model to keep argv parsing behavior consistent. + */ +export function parseExecArgvToken(raw: string): ExecArgvToken { + if (!raw) { + return { kind: "empty", raw }; + } + if (raw === "--") { + return { kind: "terminator", raw }; + } + if (raw === "-") { + return { kind: "stdin", raw }; + } + if (!raw.startsWith("-")) { + return { kind: "positional", raw }; + } + if (raw.startsWith("--")) { + const eqIndex = raw.indexOf("="); + if (eqIndex > 0) { + return { + kind: "option", + raw, + style: "long", + flag: raw.slice(0, eqIndex), + inlineValue: raw.slice(eqIndex + 1), + }; + } + return { kind: "option", raw, style: "long", flag: raw }; + } + const cluster = raw.slice(1); + return { + kind: "option", + raw, + style: "short-cluster", + cluster, + flags: cluster.split("").map((entry) => `-${entry}`), + }; +} + export type ExecCommandAnalysis = { ok: boolean; reason?: string; diff --git a/src/infra/exec-approvals.test.ts b/src/infra/exec-approvals.test.ts index dba99d0f7a..887f1980c5 100644 --- a/src/infra/exec-approvals.test.ts +++ b/src/infra/exec-approvals.test.ts @@ -14,6 +14,7 @@ import { mergeExecApprovalsSocketDefaults, minSecurity, normalizeExecApprovals, + parseExecArgvToken, normalizeSafeBins, requiresExecApproval, resolveCommandResolution, @@ -25,6 +26,7 @@ import { type ExecAllowlistEntry, type ExecApprovalsFile, } from "./exec-approvals.js"; +import { SAFE_BIN_PROFILE_FIXTURES, SAFE_BIN_PROFILES } from "./exec-safe-bin-policy.js"; function makePathEnv(binDir: string): NodeJS.ProcessEnv { if (process.platform !== "win32") { @@ -328,6 +330,26 @@ describe("exec approvals shell parsing", () => { expect(res.ok).toBe(true); expect(res.segments[0]?.argv).toEqual(["C:\\Program Files\\Tool\\tool.exe", "--version"]); }); + + it("normalizes short option clusters with attached payloads", () => { + const parsed = parseExecArgvToken("-oblocked.txt"); + expect(parsed.kind).toBe("option"); + if (parsed.kind !== "option" || parsed.style !== "short-cluster") { + throw new Error("expected short-cluster option"); + } + expect(parsed.flags[0]).toBe("-o"); + expect(parsed.cluster).toBe("oblocked.txt"); + }); + + it("normalizes long options with inline payloads", () => { + const parsed = parseExecArgvToken("--output=blocked.txt"); + expect(parsed.kind).toBe("option"); + if (parsed.kind !== "option" || parsed.style !== "long") { + throw new Error("expected long option"); + } + expect(parsed.flag).toBe("--output"); + expect(parsed.inlineValue).toBe("blocked.txt"); + }); }); describe("exec approvals shell allowlist (chained commands)", () => { @@ -515,6 +537,63 @@ describe("exec approvals safe bins", () => { }); expect(ok).toBe(true); }); + + it("supports injected platform for deterministic safe-bin checks", () => { + const ok = isSafeBinUsage({ + argv: ["jq", ".foo"], + resolution: { + rawExecutable: "jq", + resolvedPath: "/usr/bin/jq", + executableName: "jq", + }, + safeBins: normalizeSafeBins(["jq"]), + platform: "win32", + }); + expect(ok).toBe(false); + }); + + it("supports injected trusted path checker for deterministic callers", () => { + if (process.platform === "win32") { + return; + } + const baseParams = { + argv: ["jq", ".foo"], + resolution: { + rawExecutable: "jq", + resolvedPath: "/tmp/custom/jq", + executableName: "jq", + }, + safeBins: normalizeSafeBins(["jq"]), + }; + expect( + isSafeBinUsage({ + ...baseParams, + isTrustedSafeBinPathFn: () => true, + }), + ).toBe(true); + expect( + isSafeBinUsage({ + ...baseParams, + isTrustedSafeBinPathFn: () => false, + }), + ).toBe(false); + }); + + it("keeps safe-bin profile fixtures aligned with compiled profiles", () => { + for (const [name, fixture] of Object.entries(SAFE_BIN_PROFILE_FIXTURES)) { + const profile = SAFE_BIN_PROFILES[name]; + expect(profile).toBeDefined(); + const fixtureBlockedFlags = fixture.blockedFlags ?? []; + const compiledBlockedFlags = profile?.blockedFlags ?? new Set(); + for (const blockedFlag of fixtureBlockedFlags) { + expect(compiledBlockedFlags.has(blockedFlag)).toBe(true); + } + expect(Array.from(compiledBlockedFlags).toSorted()).toEqual( + [...fixtureBlockedFlags].toSorted(), + ); + } + }); + it("does not include sort/grep in default safeBins", () => { const defaults = resolveSafeBins(undefined); expect(defaults.has("jq")).toBe(true); diff --git a/src/infra/exec-safe-bin-policy.ts b/src/infra/exec-safe-bin-policy.ts index 3fe3e46007..dad6af0992 100644 --- a/src/infra/exec-safe-bin-policy.ts +++ b/src/infra/exec-safe-bin-policy.ts @@ -1,3 +1,5 @@ +import { parseExecArgvToken } from "./exec-approvals-analysis.js"; + function isPathLikeToken(value: string): boolean { const trimmed = value.trim(); if (!trimmed) { @@ -28,15 +30,45 @@ export type SafeBinProfile = { blockedFlags?: ReadonlySet; }; -const NO_FLAGS = new Set(); -const asFlagSet = (flags: string[]): ReadonlySet => new Set(flags); +export type SafeBinProfileFixture = { + minPositional?: number; + maxPositional?: number; + valueFlags?: readonly string[]; + blockedFlags?: readonly string[]; +}; -export const SAFE_BIN_GENERIC_PROFILE: SafeBinProfile = {}; +const NO_FLAGS: ReadonlySet = new Set(); -export const SAFE_BIN_PROFILES: Record = { +const toFlagSet = (flags?: readonly string[]): ReadonlySet => { + if (!flags || flags.length === 0) { + return NO_FLAGS; + } + return new Set(flags); +}; + +function compileSafeBinProfile(fixture: SafeBinProfileFixture): SafeBinProfile { + return { + minPositional: fixture.minPositional, + maxPositional: fixture.maxPositional, + valueFlags: toFlagSet(fixture.valueFlags), + blockedFlags: toFlagSet(fixture.blockedFlags), + }; +} + +function compileSafeBinProfiles( + fixtures: Record, +): Record { + return Object.fromEntries( + Object.entries(fixtures).map(([name, fixture]) => [name, compileSafeBinProfile(fixture)]), + ) as Record; +} + +export const SAFE_BIN_GENERIC_PROFILE_FIXTURE: SafeBinProfileFixture = {}; + +export const SAFE_BIN_PROFILE_FIXTURES: Record = { jq: { maxPositional: 1, - valueFlags: asFlagSet([ + valueFlags: [ "--arg", "--argjson", "--argstr", @@ -47,8 +79,8 @@ export const SAFE_BIN_PROFILES: Record = { "--library-path", "-L", "-f", - ]), - blockedFlags: asFlagSet([ + ], + blockedFlags: [ "--argfile", "--rawfile", "--slurpfile", @@ -56,11 +88,11 @@ export const SAFE_BIN_PROFILES: Record = { "--library-path", "-L", "-f", - ]), + ], }, grep: { maxPositional: 1, - valueFlags: asFlagSet([ + valueFlags: [ "--regexp", "--file", "--max-count", @@ -82,8 +114,8 @@ export const SAFE_BIN_PROFILES: Record = { "-C", "-D", "-d", - ]), - blockedFlags: asFlagSet([ + ], + blockedFlags: [ "--file", "--exclude-from", "--dereference-recursive", @@ -93,11 +125,11 @@ export const SAFE_BIN_PROFILES: Record = { "-d", "-r", "-R", - ]), + ], }, cut: { maxPositional: 0, - valueFlags: asFlagSet([ + valueFlags: [ "--bytes", "--characters", "--fields", @@ -107,11 +139,11 @@ export const SAFE_BIN_PROFILES: Record = { "-c", "-f", "-d", - ]), + ], }, sort: { maxPositional: 0, - valueFlags: asFlagSet([ + valueFlags: [ "--key", "--field-separator", "--buffer-size", @@ -127,28 +159,20 @@ export const SAFE_BIN_PROFILES: Record = { "-S", "-T", "-o", - ]), - blockedFlags: asFlagSet(["--files0-from", "--output", "-o"]), + ], + blockedFlags: ["--files0-from", "--output", "-o"], }, uniq: { maxPositional: 0, - valueFlags: asFlagSet([ - "--skip-fields", - "--skip-chars", - "--check-chars", - "--group", - "-f", - "-s", - "-w", - ]), + valueFlags: ["--skip-fields", "--skip-chars", "--check-chars", "--group", "-f", "-s", "-w"], }, head: { maxPositional: 0, - valueFlags: asFlagSet(["--lines", "--bytes", "-n", "-c"]), + valueFlags: ["--lines", "--bytes", "-n", "-c"], }, tail: { maxPositional: 0, - valueFlags: asFlagSet([ + valueFlags: [ "--lines", "--bytes", "--sleep-interval", @@ -156,7 +180,7 @@ export const SAFE_BIN_PROFILES: Record = { "--pid", "-n", "-c", - ]), + ], }, tr: { minPositional: 1, @@ -164,11 +188,16 @@ export const SAFE_BIN_PROFILES: Record = { }, wc: { maxPositional: 0, - valueFlags: asFlagSet(["--files0-from"]), - blockedFlags: asFlagSet(["--files0-from"]), + valueFlags: ["--files0-from"], + blockedFlags: ["--files0-from"], }, }; +export const SAFE_BIN_GENERIC_PROFILE = compileSafeBinProfile(SAFE_BIN_GENERIC_PROFILE_FIXTURE); + +export const SAFE_BIN_PROFILES: Record = + compileSafeBinProfiles(SAFE_BIN_PROFILE_FIXTURES); + function isSafeLiteralToken(value: string): boolean { if (!value || value === "-") { return true; @@ -180,23 +209,19 @@ function isInvalidValueToken(value: string | undefined): boolean { return !value || !isSafeLiteralToken(value); } -function consumeLongFlagToken( +function consumeLongOptionToken( args: string[], index: number, + flag: string, + inlineValue: string | undefined, valueFlags: ReadonlySet, blockedFlags: ReadonlySet, ): number { - const token = args[index]; - if (!token) { - return -1; - } - const eqIndex = token.indexOf("="); - const flag = eqIndex > 0 ? token.slice(0, eqIndex) : token; if (blockedFlags.has(flag)) { return -1; } - if (eqIndex > 0) { - return isSafeLiteralToken(token.slice(eqIndex + 1)) ? index + 1 : -1; + if (inlineValue !== undefined) { + return isSafeLiteralToken(inlineValue) ? index + 1 : -1; } if (!valueFlags.has(flag)) { return index + 1; @@ -204,31 +229,30 @@ function consumeLongFlagToken( return isInvalidValueToken(args[index + 1]) ? -1 : index + 2; } -function consumeShortFlagClusterToken( +function consumeShortOptionClusterToken( args: string[], index: number, + raw: string, + cluster: string, + flags: string[], valueFlags: ReadonlySet, blockedFlags: ReadonlySet, ): number { - const token = args[index]; - if (!token) { - return -1; - } - for (let j = 1; j < token.length; j += 1) { - const flag = `-${token[j]}`; + for (let j = 0; j < flags.length; j += 1) { + const flag = flags[j]; if (blockedFlags.has(flag)) { return -1; } if (!valueFlags.has(flag)) { continue; } - const inlineValue = token.slice(j + 1); + const inlineValue = cluster.slice(j + 1); if (inlineValue) { return isSafeLiteralToken(inlineValue) ? index + 1 : -1; } return isInvalidValueToken(args[index + 1]) ? -1 : index + 2; } - return hasGlobToken(token) ? -1 : index + 1; + return hasGlobToken(raw) ? -1 : index + 1; } function consumePositionalToken(token: string, positional: string[]): boolean { @@ -253,12 +277,15 @@ export function validateSafeBinArgv(args: string[], profile: SafeBinProfile): bo const positional: string[] = []; let i = 0; while (i < args.length) { - const token = args[i]; - if (!token) { + const rawToken = args[i] ?? ""; + const token = parseExecArgvToken(rawToken); + + if (token.kind === "empty" || token.kind === "stdin") { i += 1; continue; } - if (token === "--") { + + if (token.kind === "terminator") { for (let j = i + 1; j < args.length; j += 1) { const rest = args[j]; if (!rest || rest === "-") { @@ -270,20 +297,24 @@ export function validateSafeBinArgv(args: string[], profile: SafeBinProfile): bo } break; } - if (token === "-") { - i += 1; - continue; - } - if (!token.startsWith("-")) { - if (!consumePositionalToken(token, positional)) { + + if (token.kind === "positional") { + if (!consumePositionalToken(token.raw, positional)) { return false; } i += 1; continue; } - if (token.startsWith("--")) { - const nextIndex = consumeLongFlagToken(args, i, valueFlags, blockedFlags); + if (token.style === "long") { + const nextIndex = consumeLongOptionToken( + args, + i, + token.flag, + token.inlineValue, + valueFlags, + blockedFlags, + ); if (nextIndex < 0) { return false; } @@ -291,7 +322,15 @@ export function validateSafeBinArgv(args: string[], profile: SafeBinProfile): bo continue; } - const nextIndex = consumeShortFlagClusterToken(args, i, valueFlags, blockedFlags); + const nextIndex = consumeShortOptionClusterToken( + args, + i, + token.raw, + token.cluster, + token.flags, + valueFlags, + blockedFlags, + ); if (nextIndex < 0) { return false; }