diff --git a/CHANGELOG.md b/CHANGELOG.md index ec759b137e0..39a2596febb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,7 +15,7 @@ Docs: https://docs.openclaw.ai - Security/Voice Call: harden Twilio webhook replay handling by preserving provider event IDs through normalization, adding bounded replay dedupe, and enforcing per-call turn-token matching for call-state transitions. This ships in the next npm release. Thanks @jiseoung for reporting. - Security/Export session HTML: escape raw HTML markdown tokens in the exported session viewer, harden tree/header metadata rendering against HTML injection, and sanitize image data-URL MIME types in export output to prevent stored XSS when opening exported HTML files. This ships in the next npm release. Thanks @allsmog for reporting. - Security/iOS deep links: require local confirmation (or trusted key) before forwarding `openclaw://agent` requests from iOS to gateway `agent.request`, and strip unkeyed delivery-routing fields to reduce exfiltration risk. This ships in the next npm release. Thanks @GCXWLP for reporting. -- Security/Exec approvals: harden `autoAllowSkills` matching to require pathless invocations with resolved executables, blocking `./`/absolute-path basename collisions from satisfying skill auto-allow checks under allowlist mode. +- Security/Exec approvals: for non-default setups that enable `autoAllowSkills`, require pathless invocations plus trusted resolved-path matches so `./`/absolute-path basename collisions cannot satisfy skill auto-allow checks under allowlist mode. This ships in the next npm release. Thanks @akhmittra for reporting. - Security/Commands: enforce sender-only matching for `commands.allowFrom` by blocking conversation-shaped `From` identities (`channel:`, `group:`, `thread:`, `@g.us`) while preserving direct-message fallback when sender fields are missing. Ships in the next npm release. Thanks @jiseoung. - Config/Kilo Gateway: Kilo provider flow now surfaces an updated list of models. (#24921) thanks @gumadeiras. - Security/Sandbox: enforce `tools.exec.applyPatch.workspaceOnly` and `tools.fs.workspaceOnly` for `apply_patch` in sandbox-mounted paths so writes/deletes cannot escape the workspace boundary via mounts like `/agent` unless explicitly opted out (`tools.exec.applyPatch.workspaceOnly=false`). This ships in the next npm release. Thanks @tdjackey for reporting. diff --git a/src/infra/exec-approvals-allowlist.ts b/src/infra/exec-approvals-allowlist.ts index 25d06994977..687ce3039ba 100644 --- a/src/infra/exec-approvals-allowlist.ts +++ b/src/infra/exec-approvals-allowlist.ts @@ -1,3 +1,4 @@ +import path from "node:path"; import { DEFAULT_SAFE_BINS, analyzeShellCommand, @@ -104,6 +105,71 @@ export type ExecAllowlistEvaluation = { }; export type ExecSegmentSatisfiedBy = "allowlist" | "safeBins" | "skills" | null; +export type SkillBinTrustEntry = { + name: string; + resolvedPath: string; +}; + +function normalizeSkillBinName(value: string | undefined): string | null { + const trimmed = value?.trim().toLowerCase(); + return trimmed && trimmed.length > 0 ? trimmed : null; +} + +function normalizeSkillBinResolvedPath(value: string | undefined): string | null { + const trimmed = value?.trim(); + if (!trimmed) { + return null; + } + const resolved = path.resolve(trimmed); + if (process.platform === "win32") { + return resolved.replace(/\\/g, "/").toLowerCase(); + } + return resolved; +} + +function buildSkillBinTrustIndex( + entries: readonly SkillBinTrustEntry[] | undefined, +): Map> { + const trustByName = new Map>(); + if (!entries || entries.length === 0) { + return trustByName; + } + for (const entry of entries) { + const name = normalizeSkillBinName(entry.name); + const resolvedPath = normalizeSkillBinResolvedPath(entry.resolvedPath); + if (!name || !resolvedPath) { + continue; + } + const paths = trustByName.get(name) ?? new Set(); + paths.add(resolvedPath); + trustByName.set(name, paths); + } + return trustByName; +} + +function isSkillAutoAllowedSegment(params: { + segment: ExecCommandSegment; + allowSkills: boolean; + skillBinTrust: ReadonlyMap>; +}): boolean { + if (!params.allowSkills) { + return false; + } + const resolution = params.segment.resolution; + if (!resolution?.resolvedPath) { + return false; + } + const rawExecutable = resolution.rawExecutable?.trim() ?? ""; + if (!rawExecutable || isPathScopedExecutableToken(rawExecutable)) { + return false; + } + const executableName = normalizeSkillBinName(resolution.executableName); + const resolvedPath = normalizeSkillBinResolvedPath(resolution.resolvedPath); + if (!executableName || !resolvedPath) { + return false; + } + return Boolean(params.skillBinTrust.get(executableName)?.has(resolvedPath)); +} function evaluateSegments( segments: ExecCommandSegment[], @@ -114,7 +180,7 @@ function evaluateSegments( cwd?: string; platform?: string | null; trustedSafeBinDirs?: ReadonlySet; - skillBins?: Set; + skillBins?: readonly SkillBinTrustEntry[]; autoAllowSkills?: boolean; }, ): { @@ -123,7 +189,8 @@ function evaluateSegments( segmentSatisfiedBy: ExecSegmentSatisfiedBy[]; } { const matches: ExecAllowlistEntry[] = []; - const allowSkills = params.autoAllowSkills === true && (params.skillBins?.size ?? 0) > 0; + const skillBinTrust = buildSkillBinTrustIndex(params.skillBins); + const allowSkills = params.autoAllowSkills === true && skillBinTrust.size > 0; const segmentSatisfiedBy: ExecSegmentSatisfiedBy[] = []; const satisfied = segments.every((segment) => { @@ -152,19 +219,11 @@ function evaluateSegments( platform: params.platform, trustedSafeBinDirs: params.trustedSafeBinDirs, }); - const rawExecutable = segment.resolution?.rawExecutable?.trim() ?? ""; - const executableName = segment.resolution?.executableName; - const usesExplicitPath = isPathScopedExecutableToken(rawExecutable); - let skillAllow = false; - if ( - allowSkills && - segment.resolution?.resolvedPath && - rawExecutable.length > 0 && - !usesExplicitPath && - executableName - ) { - skillAllow = Boolean(params.skillBins?.has(executableName)); - } + const skillAllow = isSkillAutoAllowedSegment({ + segment, + allowSkills, + skillBinTrust, + }); const by: ExecSegmentSatisfiedBy = match ? "allowlist" : safe @@ -194,7 +253,7 @@ export function evaluateExecAllowlist(params: { cwd?: string; platform?: string | null; trustedSafeBinDirs?: ReadonlySet; - skillBins?: Set; + skillBins?: readonly SkillBinTrustEntry[]; autoAllowSkills?: boolean; }): ExecAllowlistEvaluation { const allowlistMatches: ExecAllowlistEntry[] = []; @@ -393,7 +452,7 @@ export function evaluateShellAllowlist(params: { cwd?: string; env?: NodeJS.ProcessEnv; trustedSafeBinDirs?: ReadonlySet; - skillBins?: Set; + skillBins?: readonly SkillBinTrustEntry[]; autoAllowSkills?: boolean; platform?: string | null; }): ExecAllowlistAnalysis { diff --git a/src/infra/exec-approvals.test.ts b/src/infra/exec-approvals.test.ts index 49d2319dd32..6b405b466d3 100644 --- a/src/infra/exec-approvals.test.ts +++ b/src/infra/exec-approvals.test.ts @@ -621,7 +621,7 @@ describe("exec approvals allowlist evaluation", () => { analysis, allowlist: [], safeBins: new Set(), - skillBins: new Set(["skill-bin"]), + skillBins: [{ name: "skill-bin", resolvedPath: "/opt/skills/skill-bin" }], autoAllowSkills: true, cwd: "/tmp", }); @@ -647,7 +647,7 @@ describe("exec approvals allowlist evaluation", () => { analysis, allowlist: [], safeBins: new Set(), - skillBins: new Set(["skill-bin"]), + skillBins: [{ name: "skill-bin", resolvedPath: "/tmp/skill-bin" }], autoAllowSkills: true, cwd: "/tmp", }); @@ -673,7 +673,7 @@ describe("exec approvals allowlist evaluation", () => { analysis, allowlist: [], safeBins: new Set(), - skillBins: new Set(["skill-bin"]), + skillBins: [{ name: "skill-bin", resolvedPath: "/opt/skills/skill-bin" }], autoAllowSkills: true, cwd: "/tmp", }); diff --git a/src/node-host/invoke-system-run.test.ts b/src/node-host/invoke-system-run.test.ts index 410382a5aad..2c6c55bd1ab 100644 --- a/src/node-host/invoke-system-run.test.ts +++ b/src/node-host/invoke-system-run.test.ts @@ -2,6 +2,7 @@ import fs from "node:fs"; import os from "node:os"; import path from "node:path"; import { describe, expect, it, vi } from "vitest"; +import { saveExecApprovals } from "../infra/exec-approvals.js"; import type { ExecHostResponse } from "../infra/exec-host.js"; import { handleSystemRunInvoke, formatSystemRunAllowlistMissMessage } from "./invoke-system-run.js"; @@ -49,7 +50,7 @@ describe("handleSystemRunInvoke mac app exec host routing", () => { sessionKey: "agent:main:main", }, skillBins: { - current: async () => new Set(), + current: async () => [], }, execHostEnforced: false, execHostFallbackAllowed: true, @@ -187,7 +188,7 @@ describe("handleSystemRunInvoke mac app exec host routing", () => { sessionKey: "agent:main:main", }, skillBins: { - current: async () => new Set(), + current: async () => [], }, execHostEnforced: false, execHostFallbackAllowed: true, @@ -226,6 +227,85 @@ describe("handleSystemRunInvoke mac app exec host routing", () => { } }); + it("denies ./skill-bin even when autoAllowSkills trust entry exists", async () => { + const tempHome = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-skill-path-spoof-")); + const previousOpenClawHome = process.env.OPENCLAW_HOME; + const skillBinPath = path.join(tempHome, "skill-bin"); + fs.writeFileSync(skillBinPath, "#!/bin/sh\necho should-not-run\n", { mode: 0o755 }); + fs.chmodSync(skillBinPath, 0o755); + process.env.OPENCLAW_HOME = tempHome; + saveExecApprovals({ + version: 1, + defaults: { + security: "allowlist", + ask: "on-miss", + askFallback: "deny", + autoAllowSkills: true, + }, + agents: {}, + }); + const runCommand = vi.fn(async () => ({ + success: true, + stdout: "local-ok", + stderr: "", + timedOut: false, + truncated: false, + exitCode: 0, + error: null, + })); + const sendInvokeResult = vi.fn(async () => {}); + const sendNodeEvent = vi.fn(async () => {}); + + try { + await handleSystemRunInvoke({ + client: {} as never, + params: { + command: ["./skill-bin", "--help"], + cwd: tempHome, + sessionKey: "agent:main:main", + }, + skillBins: { + current: async () => [{ name: "skill-bin", resolvedPath: skillBinPath }], + }, + execHostEnforced: false, + execHostFallbackAllowed: true, + resolveExecSecurity: () => "allowlist", + resolveExecAsk: () => "on-miss", + isCmdExeInvocation: () => false, + sanitizeEnv: () => undefined, + runCommand, + runViaMacAppExecHost: vi.fn(async () => null), + sendNodeEvent, + buildExecEventPayload: (payload) => payload, + sendInvokeResult, + sendExecFinishedEvent: vi.fn(async () => {}), + preferMacAppExecHost: false, + }); + } finally { + if (previousOpenClawHome === undefined) { + delete process.env.OPENCLAW_HOME; + } else { + process.env.OPENCLAW_HOME = previousOpenClawHome; + } + fs.rmSync(tempHome, { recursive: true, force: true }); + } + + expect(runCommand).not.toHaveBeenCalled(); + expect(sendNodeEvent).toHaveBeenCalledWith( + expect.anything(), + "exec.denied", + expect.objectContaining({ reason: "approval-required" }), + ); + expect(sendInvokeResult).toHaveBeenCalledWith( + expect.objectContaining({ + ok: false, + error: expect.objectContaining({ + message: "SYSTEM_RUN_DENIED: approval required", + }), + }), + ); + }); + it("denies env -S shell payloads in allowlist mode", async () => { const { runCommand, sendInvokeResult } = await runSystemInvoke({ preferMacAppExecHost: false, diff --git a/src/node-host/invoke-system-run.ts b/src/node-host/invoke-system-run.ts index aeef1522fcc..da97464966a 100644 --- a/src/node-host/invoke-system-run.ts +++ b/src/node-host/invoke-system-run.ts @@ -14,6 +14,7 @@ import { type ExecAsk, type ExecCommandSegment, type ExecSecurity, + type SkillBinTrustEntry, } from "../infra/exec-approvals.js"; import type { ExecHostRequest, ExecHostResponse, ExecHostRunResult } from "../infra/exec-host.js"; import { resolveExecSafeBinRuntimePolicy } from "../infra/exec-safe-bin-runtime-policy.js"; @@ -145,7 +146,7 @@ function evaluateSystemRunAllowlist(params: { trustedSafeBinDirs: ReturnType["trustedSafeBinDirs"]; cwd: string | undefined; env: Record | undefined; - skillBins: Set; + skillBins: SkillBinTrustEntry[]; autoAllowSkills: boolean; }): SystemRunAllowlistAnalysis { if (params.shellCommand) { @@ -310,7 +311,7 @@ export async function handleSystemRunInvoke(opts: HandleSystemRunInvokeOptions): global: cfg.tools?.exec, local: agentExec, }); - const bins = autoAllowSkills ? await opts.skillBins.current() : new Set(); + const bins = autoAllowSkills ? await opts.skillBins.current() : []; let { analysisOk, allowlistMatches, allowlistSatisfied, segments } = evaluateSystemRunAllowlist({ shellCommand, argv, diff --git a/src/node-host/invoke-types.ts b/src/node-host/invoke-types.ts index ae41d56b961..7246ba2925f 100644 --- a/src/node-host/invoke-types.ts +++ b/src/node-host/invoke-types.ts @@ -1,3 +1,5 @@ +import type { SkillBinTrustEntry } from "../infra/exec-approvals.js"; + export type SystemRunParams = { command: string[]; rawCommand?: string | null; @@ -35,5 +37,5 @@ export type ExecEventPayload = { }; export type SkillBinsProvider = { - current(force?: boolean): Promise>; + current(force?: boolean): Promise; }; diff --git a/src/node-host/runner.ts b/src/node-host/runner.ts index e8b5df74f0e..edf2cc12215 100644 --- a/src/node-host/runner.ts +++ b/src/node-host/runner.ts @@ -1,7 +1,10 @@ +import fs from "node:fs"; +import path from "node:path"; import { resolveBrowserConfig } from "../browser/config.js"; import { loadConfig } from "../config/config.js"; import { GatewayClient } from "../gateway/client.js"; import { loadOrCreateDeviceIdentity } from "../infra/device-identity.js"; +import type { SkillBinTrustEntry } from "../infra/exec-approvals.js"; import { getMachineDisplayName } from "../infra/machine-name.js"; import { ensureOpenClawCliOnPath } from "../infra/path-env.js"; import { GATEWAY_CLIENT_MODES, GATEWAY_CLIENT_NAMES } from "../utils/message-channel.js"; @@ -27,17 +30,83 @@ type NodeHostRunOptions = { const DEFAULT_NODE_PATH = "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"; +function isExecutableFile(filePath: string): boolean { + try { + const stat = fs.statSync(filePath); + if (!stat.isFile()) { + return false; + } + if (process.platform !== "win32") { + fs.accessSync(filePath, fs.constants.X_OK); + } + return true; + } catch { + return false; + } +} + +function resolveExecutablePathFromEnv(bin: string, pathEnv: string): string | null { + if (bin.includes("/") || bin.includes("\\")) { + return null; + } + const hasExtension = process.platform === "win32" && path.extname(bin).length > 0; + const extensions = + process.platform === "win32" + ? hasExtension + ? [""] + : (process.env.PATHEXT ?? process.env.PathExt ?? ".EXE;.CMD;.BAT;.COM") + .split(";") + .map((ext) => ext.toLowerCase()) + : [""]; + for (const dir of pathEnv.split(path.delimiter).filter(Boolean)) { + for (const ext of extensions) { + const candidate = path.join(dir, bin + ext); + if (isExecutableFile(candidate)) { + return candidate; + } + } + } + return null; +} + +function resolveSkillBinTrustEntries(bins: string[], pathEnv: string): SkillBinTrustEntry[] { + const trustEntries: SkillBinTrustEntry[] = []; + const seen = new Set(); + for (const bin of bins) { + const name = bin.trim(); + if (!name) { + continue; + } + const resolvedPath = resolveExecutablePathFromEnv(name, pathEnv); + if (!resolvedPath) { + continue; + } + const key = `${name}\u0000${resolvedPath}`; + if (seen.has(key)) { + continue; + } + seen.add(key); + trustEntries.push({ name, resolvedPath }); + } + return trustEntries.toSorted( + (left, right) => + left.name.localeCompare(right.name) || left.resolvedPath.localeCompare(right.resolvedPath), + ); +} + class SkillBinsCache implements SkillBinsProvider { - private bins = new Set(); + private bins: SkillBinTrustEntry[] = []; private lastRefresh = 0; private readonly ttlMs = 90_000; private readonly fetch: () => Promise; + private readonly pathEnv: string; - constructor(fetch: () => Promise) { + constructor(fetch: () => Promise, pathEnv: string) { this.fetch = fetch; + this.pathEnv = pathEnv; } - async current(force = false): Promise> { + async current(force = false): Promise { if (force || Date.now() - this.lastRefresh > this.ttlMs) { await this.refresh(); } @@ -47,11 +116,11 @@ class SkillBinsCache implements SkillBinsProvider { private async refresh() { try { const bins = await this.fetch(); - this.bins = new Set(bins); + this.bins = resolveSkillBinTrustEntries(bins, this.pathEnv); this.lastRefresh = Date.now(); } catch { if (!this.lastRefresh) { - this.bins = new Set(); + this.bins = []; } } } @@ -155,7 +224,7 @@ export async function runNodeHost(opts: NodeHostRunOptions): Promise { const res = await client.request<{ bins: Array }>("skills.bins", {}); const bins = Array.isArray(res?.bins) ? res.bins.map((bin) => String(bin)) : []; return bins; - }); + }, pathEnv); client.start(); await new Promise(() => {});