refactor: harden safe-bin trusted dir diagnostics

This commit is contained in:
Peter Steinberger
2026-02-24 23:29:12 +00:00
parent 5c2a483375
commit 4355e08262
10 changed files with 391 additions and 7 deletions

View File

@@ -175,6 +175,9 @@ export function createExecTool(
safeBinTrustedDirs: defaults?.safeBinTrustedDirs,
safeBinProfiles: defaults?.safeBinProfiles,
},
onWarning: (message) => {
logInfo(message);
},
});
if (unprofiledSafeBins.length > 0) {
logInfo(

View File

@@ -1,4 +1,8 @@
import fs from "node:fs/promises";
import os from "node:os";
import path from "node:path";
import { beforeEach, describe, expect, it, vi } from "vitest";
import { withEnvAsync } from "../test-utils/env.js";
import { runDoctorConfigWithInput } from "./doctor-config-flow.test-utils.js";
const { noteSpy } = vi.hoisted(() => ({
@@ -86,4 +90,46 @@ describe("doctor config flow safe bins", () => {
"Doctor warnings",
);
});
it("hints safeBinTrustedDirs when safeBins resolve outside default trusted dirs", async () => {
if (process.platform === "win32") {
return;
}
const dir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-doctor-safe-bins-"));
const binPath = path.join(dir, "mydoctorbin");
try {
await fs.writeFile(binPath, "#!/bin/sh\necho ok\n", "utf-8");
await fs.chmod(binPath, 0o755);
await withEnvAsync(
{
PATH: `${dir}${path.delimiter}${process.env.PATH ?? ""}`,
},
async () => {
await runDoctorConfigWithInput({
config: {
tools: {
exec: {
safeBins: ["mydoctorbin"],
safeBinProfiles: {
mydoctorbin: {},
},
},
},
},
run: loadAndMaybeMigrateDoctorConfig,
});
},
);
expect(noteSpy).toHaveBeenCalledWith(
expect.stringContaining("outside trusted safe-bin dirs"),
"Doctor warnings",
);
expect(noteSpy).toHaveBeenCalledWith(
expect.stringContaining("tools.exec.safeBinTrustedDirs"),
"Doctor warnings",
);
} finally {
await fs.rm(dir, { recursive: true, force: true }).catch(() => undefined);
}
});
});

View File

@@ -17,10 +17,16 @@ import {
import { collectProviderDangerousNameMatchingScopes } from "../config/dangerous-name-matching.js";
import { applyPluginAutoEnable } from "../config/plugin-auto-enable.js";
import { parseToolsBySenderTypedKey } from "../config/types.tools.js";
import { resolveCommandResolutionFromArgv } from "../infra/exec-command-resolution.js";
import {
listInterpreterLikeSafeBins,
resolveMergedSafeBinProfileFixtures,
} from "../infra/exec-safe-bin-runtime-policy.js";
import {
getTrustedSafeBinDirs,
isTrustedSafeBinPath,
normalizeTrustedSafeBinDirs,
} from "../infra/exec-safe-bin-trust.js";
import {
isDiscordMutableAllowEntry,
isGoogleChatMutableAllowEntry,
@@ -1001,6 +1007,13 @@ type ExecSafeBinScopeRef = {
safeBins: string[];
exec: Record<string, unknown>;
mergedProfiles: Record<string, unknown>;
trustedSafeBinDirs: ReadonlySet<string>;
};
type ExecSafeBinTrustedDirHintHit = {
scopePath: string;
bin: string;
resolvedPath: string;
};
function normalizeConfiguredSafeBins(entries: unknown): string[] {
@@ -1016,9 +1029,19 @@ function normalizeConfiguredSafeBins(entries: unknown): string[] {
).toSorted();
}
function normalizeConfiguredTrustedSafeBinDirs(entries: unknown): string[] {
if (!Array.isArray(entries)) {
return [];
}
return normalizeTrustedSafeBinDirs(
entries.filter((entry): entry is string => typeof entry === "string"),
);
}
function collectExecSafeBinScopes(cfg: OpenClawConfig): ExecSafeBinScopeRef[] {
const scopes: ExecSafeBinScopeRef[] = [];
const globalExec = asObjectRecord(cfg.tools?.exec);
const globalTrustedDirs = normalizeConfiguredTrustedSafeBinDirs(globalExec?.safeBinTrustedDirs);
if (globalExec) {
const safeBins = normalizeConfiguredSafeBins(globalExec.safeBins);
if (safeBins.length > 0) {
@@ -1030,6 +1053,9 @@ function collectExecSafeBinScopes(cfg: OpenClawConfig): ExecSafeBinScopeRef[] {
resolveMergedSafeBinProfileFixtures({
global: globalExec,
}) ?? {},
trustedSafeBinDirs: getTrustedSafeBinDirs({
extraDirs: globalTrustedDirs,
}),
});
}
}
@@ -1055,6 +1081,12 @@ function collectExecSafeBinScopes(cfg: OpenClawConfig): ExecSafeBinScopeRef[] {
global: globalExec,
local: agentExec,
}) ?? {},
trustedSafeBinDirs: getTrustedSafeBinDirs({
extraDirs: [
...globalTrustedDirs,
...normalizeConfiguredTrustedSafeBinDirs(agentExec.safeBinTrustedDirs),
],
}),
});
}
return scopes;
@@ -1078,6 +1110,32 @@ function scanExecSafeBinCoverage(cfg: OpenClawConfig): ExecSafeBinCoverageHit[]
return hits;
}
function scanExecSafeBinTrustedDirHints(cfg: OpenClawConfig): ExecSafeBinTrustedDirHintHit[] {
const hits: ExecSafeBinTrustedDirHintHit[] = [];
for (const scope of collectExecSafeBinScopes(cfg)) {
for (const bin of scope.safeBins) {
const resolution = resolveCommandResolutionFromArgv([bin]);
if (!resolution?.resolvedPath) {
continue;
}
if (
isTrustedSafeBinPath({
resolvedPath: resolution.resolvedPath,
trustedDirs: scope.trustedSafeBinDirs,
})
) {
continue;
}
hits.push({
scopePath: scope.scopePath,
bin,
resolvedPath: resolution.resolvedPath,
});
}
}
return hits;
}
function maybeRepairExecSafeBinProfiles(cfg: OpenClawConfig): {
config: OpenClawConfig;
changes: string[];
@@ -1488,6 +1546,25 @@ export async function loadAndMaybeMigrateDoctorConfig(params: {
);
note(lines.join("\n"), "Doctor warnings");
}
const safeBinTrustedDirHints = scanExecSafeBinTrustedDirHints(candidate);
if (safeBinTrustedDirHints.length > 0) {
const lines = safeBinTrustedDirHints
.slice(0, 5)
.map(
(hit) =>
`- ${hit.scopePath}.safeBins entry '${hit.bin}' resolves to '${hit.resolvedPath}' outside trusted safe-bin dirs.`,
);
if (safeBinTrustedDirHints.length > 5) {
lines.push(
`- ${safeBinTrustedDirHints.length - 5} more safeBins entries resolve outside trusted safe-bin dirs.`,
);
}
lines.push(
"- If intentional, add the binary directory to tools.exec.safeBinTrustedDirs (global or agent scope).",
);
note(lines.join("\n"), "Doctor warnings");
}
}
const mutableAllowlistHits = scanMutableAllowlistEntries(candidate);

View File

@@ -1,5 +1,7 @@
import fs from "node:fs/promises";
import os from "node:os";
import path from "node:path";
import { describe, expect, it } from "vitest";
import { describe, expect, it, vi } from "vitest";
import {
isInterpreterLikeSafeBin,
listInterpreterLikeSafeBins,
@@ -103,4 +105,34 @@ describe("exec safe-bin runtime policy", () => {
expect(optedIn.trustedSafeBinDirs.has(path.resolve("/opt/homebrew/bin"))).toBe(true);
expect(optedIn.trustedSafeBinDirs.has(path.resolve("/usr/local/bin"))).toBe(true);
});
it("emits runtime warning when explicitly trusted dir is writable", async () => {
if (process.platform === "win32") {
return;
}
const dir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-safe-bin-runtime-"));
try {
await fs.chmod(dir, 0o777);
const onWarning = vi.fn();
const policy = resolveExecSafeBinRuntimePolicy({
global: {
safeBinTrustedDirs: [dir],
},
onWarning,
});
expect(policy.writableTrustedSafeBinDirs).toEqual([
{
dir: path.resolve(dir),
groupWritable: true,
worldWritable: true,
},
]);
expect(onWarning).toHaveBeenCalledWith(expect.stringContaining(path.resolve(dir)));
expect(onWarning).toHaveBeenCalledWith(expect.stringContaining("world-writable"));
} finally {
await fs.chmod(dir, 0o755).catch(() => undefined);
await fs.rm(dir, { recursive: true, force: true }).catch(() => undefined);
}
});
});

View File

@@ -6,7 +6,12 @@ import {
type SafeBinProfileFixture,
type SafeBinProfileFixtures,
} from "./exec-safe-bin-policy.js";
import { getTrustedSafeBinDirs, normalizeTrustedSafeBinDirs } from "./exec-safe-bin-trust.js";
import {
getTrustedSafeBinDirs,
listWritableExplicitTrustedSafeBinDirs,
normalizeTrustedSafeBinDirs,
type WritableTrustedSafeBinDir,
} from "./exec-safe-bin-trust.js";
export type ExecSafeBinConfigScope = {
safeBins?: string[] | null;
@@ -99,12 +104,14 @@ export function resolveMergedSafeBinProfileFixtures(params: {
export function resolveExecSafeBinRuntimePolicy(params: {
global?: ExecSafeBinConfigScope | null;
local?: ExecSafeBinConfigScope | null;
onWarning?: (message: string) => void;
}): {
safeBins: Set<string>;
safeBinProfiles: Readonly<Record<string, SafeBinProfile>>;
trustedSafeBinDirs: ReadonlySet<string>;
unprofiledSafeBins: string[];
unprofiledInterpreterSafeBins: string[];
writableTrustedSafeBinDirs: ReadonlyArray<WritableTrustedSafeBinDir>;
} {
const safeBins = resolveSafeBins(params.local?.safeBins ?? params.global?.safeBins);
const safeBinProfiles = resolveSafeBinProfiles(
@@ -116,17 +123,35 @@ export function resolveExecSafeBinRuntimePolicy(params: {
const unprofiledSafeBins = Array.from(safeBins)
.filter((entry) => !safeBinProfiles[entry])
.toSorted();
const explicitTrustedSafeBinDirs = [
...normalizeTrustedSafeBinDirs(params.global?.safeBinTrustedDirs),
...normalizeTrustedSafeBinDirs(params.local?.safeBinTrustedDirs),
];
const trustedSafeBinDirs = getTrustedSafeBinDirs({
extraDirs: [
...normalizeTrustedSafeBinDirs(params.global?.safeBinTrustedDirs),
...normalizeTrustedSafeBinDirs(params.local?.safeBinTrustedDirs),
],
extraDirs: explicitTrustedSafeBinDirs,
});
const writableTrustedSafeBinDirs = listWritableExplicitTrustedSafeBinDirs(
explicitTrustedSafeBinDirs,
);
if (params.onWarning) {
for (const hit of writableTrustedSafeBinDirs) {
const scope =
hit.worldWritable || hit.groupWritable
? hit.worldWritable
? "world-writable"
: "group-writable"
: "writable";
params.onWarning(
`exec: safeBinTrustedDirs includes ${scope} directory '${hit.dir}'; remove trust or tighten permissions (for example chmod 755).`,
);
}
}
return {
safeBins,
safeBinProfiles,
trustedSafeBinDirs,
unprofiledSafeBins,
unprofiledInterpreterSafeBins: listInterpreterLikeSafeBins(unprofiledSafeBins),
writableTrustedSafeBinDirs,
};
}

View File

@@ -1,3 +1,5 @@
import fs from "node:fs/promises";
import os from "node:os";
import path from "node:path";
import { describe, expect, it } from "vitest";
import { withEnv } from "../test-utils/env.js";
@@ -5,6 +7,7 @@ import {
buildTrustedSafeBinDirs,
getTrustedSafeBinDirs,
isTrustedSafeBinPath,
listWritableExplicitTrustedSafeBinDirs,
} from "./exec-safe-bin-trust.js";
describe("exec safe bin trust", () => {
@@ -69,4 +72,25 @@ describe("exec safe bin trust", () => {
expect(refreshed.has(path.resolve(injected))).toBe(false);
});
});
it("flags explicitly trusted dirs that are group/world writable", async () => {
if (process.platform === "win32") {
return;
}
const dir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-safe-bin-trust-"));
try {
await fs.chmod(dir, 0o777);
const hits = listWritableExplicitTrustedSafeBinDirs([dir]);
expect(hits).toEqual([
{
dir: path.resolve(dir),
groupWritable: true,
worldWritable: true,
},
]);
} finally {
await fs.chmod(dir, 0o755).catch(() => undefined);
await fs.rm(dir, { recursive: true, force: true }).catch(() => undefined);
}
});
});

View File

@@ -1,3 +1,4 @@
import fs from "node:fs";
import path from "node:path";
// Keep defaults to OS-managed immutable bins only.
@@ -19,6 +20,12 @@ type TrustedSafeBinCache = {
dirs: Set<string>;
};
export type WritableTrustedSafeBinDir = {
dir: string;
groupWritable: boolean;
worldWritable: boolean;
};
let trustedSafeBinCache: TrustedSafeBinCache | null = null;
function normalizeTrustedDir(value: string): string | null {
@@ -88,3 +95,32 @@ export function isTrustedSafeBinPath(params: TrustedSafeBinPathParams): boolean
const resolvedDir = path.dirname(path.resolve(params.resolvedPath));
return trustedDirs.has(resolvedDir);
}
export function listWritableExplicitTrustedSafeBinDirs(
entries?: readonly string[] | null,
): WritableTrustedSafeBinDir[] {
if (process.platform === "win32") {
return [];
}
const resolved = resolveTrustedSafeBinDirs(normalizeTrustedSafeBinDirs(entries));
const hits: WritableTrustedSafeBinDir[] = [];
for (const dir of resolved) {
let stat: fs.Stats;
try {
stat = fs.statSync(dir);
} catch {
continue;
}
if (!stat.isDirectory()) {
continue;
}
const mode = stat.mode & 0o777;
const groupWritable = (mode & 0o020) !== 0;
const worldWritable = (mode & 0o002) !== 0;
if (!groupWritable && !worldWritable) {
continue;
}
hits.push({ dir, groupWritable, worldWritable });
}
return hits;
}

View File

@@ -55,6 +55,16 @@ type SystemRunAllowlistAnalysis = {
segments: ExecCommandSegment[];
};
const safeBinTrustedDirWarningCache = new Set<string>();
function warnWritableTrustedDirOnce(message: string): void {
if (safeBinTrustedDirWarningCache.has(message)) {
return;
}
safeBinTrustedDirWarningCache.add(message);
console.warn(message);
}
function normalizeDeniedReason(reason: string | null | undefined): SystemRunDeniedReason {
switch (reason) {
case "security=deny":
@@ -310,6 +320,7 @@ export async function handleSystemRunInvoke(opts: HandleSystemRunInvokeOptions):
const { safeBins, safeBinProfiles, trustedSafeBinDirs } = resolveExecSafeBinRuntimePolicy({
global: cfg.tools?.exec,
local: agentExec,
onWarning: warnWritableTrustedDirOnce,
});
const bins = autoAllowSkills ? await opts.skillBins.current() : [];
let { analysisOk, allowlistMatches, allowlistSatisfied, segments } = evaluateSystemRunAllowlist({

View File

@@ -438,6 +438,50 @@ describe("security audit", () => {
);
});
it("warns for risky safeBinTrustedDirs entries", async () => {
const cfg: OpenClawConfig = {
tools: {
exec: {
safeBinTrustedDirs: ["/usr/local/bin", "/tmp/openclaw-safe-bins"],
},
},
agents: {
list: [
{
id: "ops",
tools: {
exec: {
safeBinTrustedDirs: ["./relative-bin-dir"],
},
},
},
],
},
};
const res = await audit(cfg);
const finding = res.findings.find(
(f) => f.checkId === "tools.exec.safe_bin_trusted_dirs_risky",
);
expect(finding?.severity).toBe("warn");
expect(finding?.detail).toContain("/usr/local/bin");
expect(finding?.detail).toContain("/tmp/openclaw-safe-bins");
expect(finding?.detail).toContain("agents.list.ops.tools.exec");
});
it("does not warn for non-risky absolute safeBinTrustedDirs entries", async () => {
const cfg: OpenClawConfig = {
tools: {
exec: {
safeBinTrustedDirs: ["/usr/libexec"],
},
},
};
const res = await audit(cfg);
expectNoFinding(res, "tools.exec.safe_bin_trusted_dirs_risky");
});
it("evaluates loopback control UI and logging exposure findings", async () => {
const cases: Array<{
name: string;

View File

@@ -1,4 +1,5 @@
import { isIP } from "node:net";
import path from "node:path";
import { resolveSandboxConfigForAgent } from "../agents/sandbox.js";
import { execDockerRaw } from "../agents/sandbox/docker.js";
import { resolveBrowserConfig, resolveProfile } from "../browser/config.js";
@@ -15,6 +16,7 @@ import {
listInterpreterLikeSafeBins,
resolveMergedSafeBinProfileFixtures,
} from "../infra/exec-safe-bin-runtime-policy.js";
import { normalizeTrustedSafeBinDirs } from "../infra/exec-safe-bin-trust.js";
import { collectChannelSecurityFindings } from "./audit-channel.js";
import {
collectAttackSurfaceSummaryFindings,
@@ -748,8 +750,77 @@ function collectExecRuntimeFindings(cfg: OpenClawConfig): SecurityAuditFinding[]
),
).toSorted();
};
const interpreterHits: string[] = [];
const normalizeConfiguredTrustedDirs = (entries: unknown): string[] => {
if (!Array.isArray(entries)) {
return [];
}
return normalizeTrustedSafeBinDirs(
entries.filter((entry): entry is string => typeof entry === "string"),
);
};
const classifyRiskySafeBinTrustedDir = (entry: string): string | null => {
const raw = entry.trim();
if (!raw) {
return null;
}
if (!path.isAbsolute(raw)) {
return "relative path (trust boundary depends on process cwd)";
}
const normalized = path.resolve(raw).replace(/\\/g, "/").toLowerCase();
if (
normalized === "/tmp" ||
normalized.startsWith("/tmp/") ||
normalized === "/var/tmp" ||
normalized.startsWith("/var/tmp/") ||
normalized === "/private/tmp" ||
normalized.startsWith("/private/tmp/")
) {
return "temporary directory is mutable and easy to poison";
}
if (
normalized === "/usr/local/bin" ||
normalized === "/opt/homebrew/bin" ||
normalized === "/opt/local/bin" ||
normalized === "/home/linuxbrew/.linuxbrew/bin"
) {
return "package-manager bin directory (often user-writable)";
}
if (
normalized.startsWith("/users/") ||
normalized.startsWith("/home/") ||
normalized.includes("/.local/bin")
) {
return "home-scoped bin directory (typically user-writable)";
}
if (/^[a-z]:\/users\//.test(normalized)) {
return "home-scoped bin directory (typically user-writable)";
}
return null;
};
const globalExec = cfg.tools?.exec;
const riskyTrustedDirHits: string[] = [];
const collectRiskyTrustedDirHits = (scopePath: string, entries: unknown): void => {
for (const entry of normalizeConfiguredTrustedDirs(entries)) {
const reason = classifyRiskySafeBinTrustedDir(entry);
if (!reason) {
continue;
}
riskyTrustedDirHits.push(`- ${scopePath}.safeBinTrustedDirs: ${entry} (${reason})`);
}
};
collectRiskyTrustedDirHits("tools.exec", globalExec?.safeBinTrustedDirs);
for (const entry of agents) {
if (!entry || typeof entry !== "object" || typeof entry.id !== "string") {
continue;
}
collectRiskyTrustedDirHits(
`agents.list.${entry.id}.tools.exec`,
entry.tools?.exec?.safeBinTrustedDirs,
);
}
const interpreterHits: string[] = [];
const globalSafeBins = normalizeConfiguredSafeBins(globalExec?.safeBins);
if (globalSafeBins.length > 0) {
const merged = resolveMergedSafeBinProfileFixtures({ global: globalExec }) ?? {};
@@ -795,6 +866,21 @@ function collectExecRuntimeFindings(cfg: OpenClawConfig): SecurityAuditFinding[]
});
}
if (riskyTrustedDirHits.length > 0) {
findings.push({
checkId: "tools.exec.safe_bin_trusted_dirs_risky",
severity: "warn",
title: "safeBinTrustedDirs includes risky mutable directories",
detail:
`Detected risky safeBinTrustedDirs entries:\n${riskyTrustedDirHits.slice(0, 10).join("\n")}` +
(riskyTrustedDirHits.length > 10
? `\n- +${riskyTrustedDirHits.length - 10} more entries.`
: ""),
remediation:
"Prefer root-owned immutable bins, keep default trust dirs (/bin, /usr/bin), and avoid trusting temporary/home/package-manager paths unless tightly controlled.",
});
}
return findings;
}