github/openclaw
1
1
Fork 0
mirror of https://github.com/openclaw/openclaw.git synced 2026-02-19 18:39:20 -05:00
Code Issues Packages Projects Releases Wiki Activity

refactor(security): unify safe-bin argv parsing and harden regressions

Browse Source
This commit is contained in:
Peter Steinberger
2026-02-19 16:04:51 +01:00
parent 2e421f32df
commit a688ccf24a
7 changed files with 292 additions and 67 deletions
Download Patch File Download Diff File Expand all files Collapse all files

20
src/agents/pi-tools.safe-bins.e2e.test.ts
View File

@@ -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;

4
src/channels/plugins/message-actions.security.test.ts
View File

@@ -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 }));

2
src/channels/plugins/message-actions.ts
View File

@@ -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<Record<string, ReadonlySet<ChannelMessageActionName>>>

21
src/infra/exec-approvals-allowlist.ts
View File

@@ -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<string>;
platform?: string | null;
trustedSafeBinDirs?: ReadonlySet<string>;
safeBinProfiles?: Readonly<Record<string, SafeBinProfile>>;
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<string>;
cwd?: string;
platform?: string | null;
trustedSafeBinDirs?: ReadonlySet<string>;
skillBins?: Set<string>;
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<string>;
cwd?: string;
platform?: string | null;
trustedSafeBinDirs?: ReadonlySet<string>;
skillBins?: Set<string>;
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,

72
src/infra/exec-approvals-analysis.ts
View File

@@ -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;

79
src/infra/exec-approvals.test.ts
View File

@@ -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<string>();
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);

161
src/infra/exec-safe-bin-policy.ts
View File

@@ -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<string>;
};
const NO_FLAGS = new Set<string>();
const asFlagSet = (flags: string[]): ReadonlySet<string> => 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<string> = new Set();
export const SAFE_BIN_PROFILES: Record<string, SafeBinProfile> = {
const toFlagSet = (flags?: readonly string[]): ReadonlySet<string> => {
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<string, SafeBinProfileFixture>,
): Record<string, SafeBinProfile> {
return Object.fromEntries(
Object.entries(fixtures).map(([name, fixture]) => [name, compileSafeBinProfile(fixture)]),
) as Record<string, SafeBinProfile>;
}
export const SAFE_BIN_GENERIC_PROFILE_FIXTURE: SafeBinProfileFixture = {};
export const SAFE_BIN_PROFILE_FIXTURES: Record<string, SafeBinProfileFixture> = {
jq: {
maxPositional: 1,
valueFlags: asFlagSet([
valueFlags: [
"--arg",
"--argjson",
"--argstr",
@@ -47,8 +79,8 @@ export const SAFE_BIN_PROFILES: Record<string, SafeBinProfile> = {
"--library-path",
"-L",
"-f",
]),
blockedFlags: asFlagSet([
],
blockedFlags: [
"--argfile",
"--rawfile",
"--slurpfile",
@@ -56,11 +88,11 @@ export const SAFE_BIN_PROFILES: Record<string, SafeBinProfile> = {
"--library-path",
"-L",
"-f",
]),
],
},
grep: {
maxPositional: 1,
valueFlags: asFlagSet([
valueFlags: [
"--regexp",
"--file",
"--max-count",
@@ -82,8 +114,8 @@ export const SAFE_BIN_PROFILES: Record<string, SafeBinProfile> = {
"-C",
"-D",
"-d",
]),
blockedFlags: asFlagSet([
],
blockedFlags: [
"--file",
"--exclude-from",
"--dereference-recursive",
@@ -93,11 +125,11 @@ export const SAFE_BIN_PROFILES: Record<string, SafeBinProfile> = {
"-d",
"-r",
"-R",
]),
],
},
cut: {
maxPositional: 0,
valueFlags: asFlagSet([
valueFlags: [
"--bytes",
"--characters",
"--fields",
@@ -107,11 +139,11 @@ export const SAFE_BIN_PROFILES: Record<string, SafeBinProfile> = {
"-c",
"-f",
"-d",
]),
],
},
sort: {
maxPositional: 0,
valueFlags: asFlagSet([
valueFlags: [
"--key",
"--field-separator",
"--buffer-size",
@@ -127,28 +159,20 @@ export const SAFE_BIN_PROFILES: Record<string, SafeBinProfile> = {
"-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<string, SafeBinProfile> = {
"--pid",
"-n",
"-c",
]),
],
},
tr: {
minPositional: 1,
@@ -164,11 +188,16 @@ export const SAFE_BIN_PROFILES: Record<string, SafeBinProfile> = {
},
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<string, SafeBinProfile> =
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<string>,
blockedFlags: ReadonlySet<string>,
): 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<string>,
blockedFlags: ReadonlySet<string>,
): 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;
}