From 4355e08262029c9fe7f450915f20ef3867aa219e Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Tue, 24 Feb 2026 23:29:12 +0000 Subject: [PATCH] refactor: harden safe-bin trusted dir diagnostics --- src/agents/bash-tools.exec.ts | 3 + .../doctor-config-flow.safe-bins.test.ts | 46 ++++++++++ src/commands/doctor-config-flow.ts | 77 ++++++++++++++++ .../exec-safe-bin-runtime-policy.test.ts | 34 ++++++- src/infra/exec-safe-bin-runtime-policy.ts | 35 ++++++-- src/infra/exec-safe-bin-trust.test.ts | 24 +++++ src/infra/exec-safe-bin-trust.ts | 36 ++++++++ src/node-host/invoke-system-run.ts | 11 +++ src/security/audit.test.ts | 44 ++++++++++ src/security/audit.ts | 88 ++++++++++++++++++- 10 files changed, 391 insertions(+), 7 deletions(-) diff --git a/src/agents/bash-tools.exec.ts b/src/agents/bash-tools.exec.ts index 7fd16e36eaf..a9d230c24b6 100644 --- a/src/agents/bash-tools.exec.ts +++ b/src/agents/bash-tools.exec.ts @@ -175,6 +175,9 @@ export function createExecTool( safeBinTrustedDirs: defaults?.safeBinTrustedDirs, safeBinProfiles: defaults?.safeBinProfiles, }, + onWarning: (message) => { + logInfo(message); + }, }); if (unprofiledSafeBins.length > 0) { logInfo( diff --git a/src/commands/doctor-config-flow.safe-bins.test.ts b/src/commands/doctor-config-flow.safe-bins.test.ts index 3d7a646a8dd..802cfeb8d96 100644 --- a/src/commands/doctor-config-flow.safe-bins.test.ts +++ b/src/commands/doctor-config-flow.safe-bins.test.ts @@ -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); + } + }); }); diff --git a/src/commands/doctor-config-flow.ts b/src/commands/doctor-config-flow.ts index e86dec9e819..f4a7e4132a8 100644 --- a/src/commands/doctor-config-flow.ts +++ b/src/commands/doctor-config-flow.ts @@ -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; mergedProfiles: Record; + trustedSafeBinDirs: ReadonlySet; +}; + +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); diff --git a/src/infra/exec-safe-bin-runtime-policy.test.ts b/src/infra/exec-safe-bin-runtime-policy.test.ts index 94cc868c5b2..af5510be5f2 100644 --- a/src/infra/exec-safe-bin-runtime-policy.test.ts +++ b/src/infra/exec-safe-bin-runtime-policy.test.ts @@ -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); + } + }); }); diff --git a/src/infra/exec-safe-bin-runtime-policy.ts b/src/infra/exec-safe-bin-runtime-policy.ts index 9ed56bfe680..955d130de11 100644 --- a/src/infra/exec-safe-bin-runtime-policy.ts +++ b/src/infra/exec-safe-bin-runtime-policy.ts @@ -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; safeBinProfiles: Readonly>; trustedSafeBinDirs: ReadonlySet; unprofiledSafeBins: string[]; unprofiledInterpreterSafeBins: string[]; + writableTrustedSafeBinDirs: ReadonlyArray; } { 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, }; } diff --git a/src/infra/exec-safe-bin-trust.test.ts b/src/infra/exec-safe-bin-trust.test.ts index eccd6cce986..c22d062b893 100644 --- a/src/infra/exec-safe-bin-trust.test.ts +++ b/src/infra/exec-safe-bin-trust.test.ts @@ -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); + } + }); }); diff --git a/src/infra/exec-safe-bin-trust.ts b/src/infra/exec-safe-bin-trust.ts index e939ac71711..418a6d49200 100644 --- a/src/infra/exec-safe-bin-trust.ts +++ b/src/infra/exec-safe-bin-trust.ts @@ -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; }; +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; +} diff --git a/src/node-host/invoke-system-run.ts b/src/node-host/invoke-system-run.ts index da97464966a..897d8ebd111 100644 --- a/src/node-host/invoke-system-run.ts +++ b/src/node-host/invoke-system-run.ts @@ -55,6 +55,16 @@ type SystemRunAllowlistAnalysis = { segments: ExecCommandSegment[]; }; +const safeBinTrustedDirWarningCache = new Set(); + +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({ diff --git a/src/security/audit.test.ts b/src/security/audit.test.ts index 4354c32b77b..04c45907041 100644 --- a/src/security/audit.test.ts +++ b/src/security/audit.test.ts @@ -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; diff --git a/src/security/audit.ts b/src/security/audit.ts index c1714ca4969..e6254b5cc80 100644 --- a/src/security/audit.ts +++ b/src/security/audit.ts @@ -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; }