fix(exec): harden safe-bin trust and add explicit trusted dirs

This commit is contained in:
Peter Steinberger
2026-02-22 22:42:29 +01:00
parent 08fb38f729
commit 64b273a71c
18 changed files with 123 additions and 55 deletions

View File

@@ -882,6 +882,15 @@ function renderQuotedArgv(argv: string[]): string {
return argv.map((token) => shellEscapeSingleArg(token)).join(" ");
}
function renderSafeBinSegmentArgv(segment: ExecCommandSegment): string {
if (segment.argv.length === 0) {
return "";
}
const resolvedExecutable = segment.resolution?.resolvedPath?.trim();
const argv = resolvedExecutable ? [resolvedExecutable, ...segment.argv.slice(1)] : segment.argv;
return renderQuotedArgv(argv);
}
/**
* Rebuilds a shell command and selectively single-quotes argv tokens for segments that
* must be treated as literal (safeBins hardening) while preserving the rest of the
@@ -920,7 +929,7 @@ export function buildSafeBinsShellCommand(params: {
return { ok: false, reason: "segment mapping failed" };
}
const needsLiteral = by === "safeBins";
rendered.push(needsLiteral ? renderQuotedArgv(seg.argv) : raw.trim());
rendered.push(needsLiteral ? renderSafeBinSegmentArgv(seg) : raw.trim());
segIndex += 1;
}

View File

@@ -195,8 +195,8 @@ describe("exec approvals safe shell command builder", () => {
expect(res.ok).toBe(true);
// Preserve non-safeBins segment raw (glob stays unquoted)
expect(res.command).toContain("rg foo src/*.ts");
// SafeBins segment is fully quoted
expect(res.command).toContain("'head' '-n' '5'");
// SafeBins segment is fully quoted and pinned to its resolved absolute path.
expect(res.command).toMatch(/'[^']*\/head' '-n' '5'/);
});
});
@@ -936,6 +936,30 @@ describe("exec approvals safe bins", () => {
});
expect(allowed.allowlistSatisfied).toBe(true);
});
it("does not auto-trust PATH-shadowed safe bins without explicit trusted dirs", () => {
if (process.platform === "win32") {
return;
}
const tmp = makeTempDir();
const fakeDir = path.join(tmp, "fake-bin");
fs.mkdirSync(fakeDir, { recursive: true });
const fakeHead = path.join(fakeDir, "head");
fs.writeFileSync(fakeHead, "#!/bin/sh\nexit 0\n");
fs.chmodSync(fakeHead, 0o755);
const result = evaluateShellAllowlist({
command: "head -n 1",
allowlist: [],
safeBins: normalizeSafeBins(["head"]),
env: makePathEnv(fakeDir),
cwd: tmp,
});
expect(result.analysisOk).toBe(true);
expect(result.allowlistSatisfied).toBe(false);
expect(result.segmentSatisfiedBy).toEqual([null]);
expect(result.segments[0]?.resolution?.resolvedPath).toBe(fakeHead);
});
});
describe("exec approvals allowlist evaluation", () => {

View File

@@ -70,4 +70,18 @@ describe("exec safe-bin runtime policy", () => {
expect(policy.unprofiledSafeBins).toEqual(["python3"]);
expect(policy.unprofiledInterpreterSafeBins).toEqual(["python3"]);
});
it("merges explicit safe-bin trusted dirs from global and local config", () => {
const policy = resolveExecSafeBinRuntimePolicy({
global: {
safeBinTrustedDirs: [" /custom/bin ", "/custom/bin"],
},
local: {
safeBinTrustedDirs: ["/agent/bin"],
},
});
expect(policy.trustedSafeBinDirs.has("/custom/bin")).toBe(true);
expect(policy.trustedSafeBinDirs.has("/agent/bin")).toBe(true);
});
});

View File

@@ -11,6 +11,7 @@ import { getTrustedSafeBinDirs } from "./exec-safe-bin-trust.js";
export type ExecSafeBinConfigScope = {
safeBins?: string[] | null;
safeBinProfiles?: SafeBinProfileFixtures | null;
safeBinTrustedDirs?: string[] | null;
};
const INTERPRETER_LIKE_SAFE_BINS = new Set([
@@ -78,6 +79,14 @@ export function listInterpreterLikeSafeBins(entries: Iterable<string>): string[]
.toSorted();
}
function normalizeTrustedDirs(entries?: string[] | null): string[] {
if (!Array.isArray(entries)) {
return [];
}
const normalized = entries.map((entry) => entry.trim()).filter((entry) => entry.length > 0);
return Array.from(new Set(normalized));
}
export function resolveMergedSafeBinProfileFixtures(params: {
global?: ExecSafeBinConfigScope | null;
local?: ExecSafeBinConfigScope | null;
@@ -96,7 +105,6 @@ export function resolveMergedSafeBinProfileFixtures(params: {
export function resolveExecSafeBinRuntimePolicy(params: {
global?: ExecSafeBinConfigScope | null;
local?: ExecSafeBinConfigScope | null;
pathEnv?: string | null;
}): {
safeBins: Set<string>;
safeBinProfiles: Readonly<Record<string, SafeBinProfile>>;
@@ -114,9 +122,12 @@ export function resolveExecSafeBinRuntimePolicy(params: {
const unprofiledSafeBins = Array.from(safeBins)
.filter((entry) => !safeBinProfiles[entry])
.toSorted();
const trustedSafeBinDirs = params.pathEnv
? getTrustedSafeBinDirs({ pathEnv: params.pathEnv })
: getTrustedSafeBinDirs();
const trustedSafeBinDirs = getTrustedSafeBinDirs({
extraDirs: [
...normalizeTrustedDirs(params.global?.safeBinTrustedDirs),
...normalizeTrustedDirs(params.local?.safeBinTrustedDirs),
],
});
return {
safeBins,
safeBinProfiles,

View File

@@ -8,11 +8,10 @@ import {
} from "./exec-safe-bin-trust.js";
describe("exec safe bin trust", () => {
it("builds trusted dirs from defaults and injected PATH", () => {
it("builds trusted dirs from defaults and explicit extra dirs", () => {
const dirs = buildTrustedSafeBinDirs({
pathEnv: "/custom/bin:/alt/bin:/custom/bin",
delimiter: ":",
baseDirs: ["/usr/bin"],
extraDirs: ["/custom/bin", "/alt/bin", "/custom/bin"],
});
expect(dirs.has(path.resolve("/usr/bin"))).toBe(true);
@@ -21,19 +20,16 @@ describe("exec safe bin trust", () => {
expect(dirs.size).toBe(3);
});
it("memoizes trusted dirs per PATH snapshot", () => {
it("memoizes trusted dirs per explicit trusted-dir snapshot", () => {
const a = getTrustedSafeBinDirs({
pathEnv: "/first/bin",
delimiter: ":",
extraDirs: ["/first/bin"],
refresh: true,
});
const b = getTrustedSafeBinDirs({
pathEnv: "/first/bin",
delimiter: ":",
extraDirs: ["/first/bin"],
});
const c = getTrustedSafeBinDirs({
pathEnv: "/second/bin",
delimiter: ":",
extraDirs: ["/second/bin"],
});
expect(a).toBe(b);
@@ -56,14 +52,12 @@ describe("exec safe bin trust", () => {
).toBe(false);
});
it("uses startup PATH snapshot when pathEnv is omitted", () => {
it("does not trust PATH entries by default", () => {
const injected = `/tmp/openclaw-path-injected-${Date.now()}`;
const initial = getTrustedSafeBinDirs({ refresh: true });
withEnv({ PATH: `${injected}${path.delimiter}${process.env.PATH ?? ""}` }, () => {
const refreshed = getTrustedSafeBinDirs({ refresh: true });
expect(refreshed.has(path.resolve(injected))).toBe(false);
expect([...refreshed].toSorted()).toEqual([...initial].toSorted());
});
});
});

View File

@@ -11,16 +11,13 @@ const DEFAULT_SAFE_BIN_TRUSTED_DIRS = [
];
type TrustedSafeBinDirsParams = {
pathEnv?: string | null;
delimiter?: string;
baseDirs?: readonly string[];
extraDirs?: readonly string[];
};
type TrustedSafeBinPathParams = {
resolvedPath: string;
trustedDirs?: ReadonlySet<string>;
pathEnv?: string | null;
delimiter?: string;
};
type TrustedSafeBinCache = {
@@ -29,7 +26,6 @@ type TrustedSafeBinCache = {
};
let trustedSafeBinCache: TrustedSafeBinCache | null = null;
const STARTUP_PATH_ENV = process.env.PATH ?? process.env.Path ?? "";
function normalizeTrustedDir(value: string): string | null {
const trimmed = value.trim();
@@ -39,64 +35,54 @@ function normalizeTrustedDir(value: string): string | null {
return path.resolve(trimmed);
}
function buildTrustedSafeBinCacheKey(pathEnv: string, delimiter: string): string {
return `${delimiter}\u0000${pathEnv}`;
function buildTrustedSafeBinCacheKey(params: {
baseDirs: readonly string[];
extraDirs: readonly string[];
}): string {
return `${params.baseDirs.join("\u0001")}\u0000${params.extraDirs.join("\u0001")}`;
}
export function buildTrustedSafeBinDirs(params: TrustedSafeBinDirsParams = {}): Set<string> {
const delimiter = params.delimiter ?? path.delimiter;
const pathEnv = params.pathEnv ?? "";
const baseDirs = params.baseDirs ?? DEFAULT_SAFE_BIN_TRUSTED_DIRS;
const extraDirs = params.extraDirs ?? [];
const trusted = new Set<string>();
for (const entry of baseDirs) {
// Trust is explicit only. Do not derive from PATH, which is user/environment controlled.
for (const entry of [...baseDirs, ...extraDirs]) {
const normalized = normalizeTrustedDir(entry);
if (normalized) {
trusted.add(normalized);
}
}
const pathEntries = pathEnv
.split(delimiter)
.map((entry) => normalizeTrustedDir(entry))
.filter((entry): entry is string => Boolean(entry));
for (const entry of pathEntries) {
trusted.add(entry);
}
return trusted;
}
export function getTrustedSafeBinDirs(
params: {
pathEnv?: string | null;
delimiter?: string;
baseDirs?: readonly string[];
extraDirs?: readonly string[];
refresh?: boolean;
} = {},
): Set<string> {
const delimiter = params.delimiter ?? path.delimiter;
const pathEnv = params.pathEnv ?? STARTUP_PATH_ENV;
const key = buildTrustedSafeBinCacheKey(pathEnv, delimiter);
const baseDirs = params.baseDirs ?? DEFAULT_SAFE_BIN_TRUSTED_DIRS;
const extraDirs = params.extraDirs ?? [];
const key = buildTrustedSafeBinCacheKey({ baseDirs, extraDirs });
if (!params.refresh && trustedSafeBinCache?.key === key) {
return trustedSafeBinCache.dirs;
}
const dirs = buildTrustedSafeBinDirs({
pathEnv,
delimiter,
baseDirs,
extraDirs,
});
trustedSafeBinCache = { key, dirs };
return dirs;
}
export function isTrustedSafeBinPath(params: TrustedSafeBinPathParams): boolean {
const trustedDirs =
params.trustedDirs ??
getTrustedSafeBinDirs({
pathEnv: params.pathEnv,
delimiter: params.delimiter,
});
const trustedDirs = params.trustedDirs ?? getTrustedSafeBinDirs();
const resolvedDir = path.dirname(path.resolve(params.resolvedPath));
return trustedDirs.has(resolvedDir);
}