From f789f880c934caa8be25b38832f27f90f37903db Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Thu, 26 Feb 2026 04:13:59 +0100 Subject: [PATCH] fix(security): harden approval-bound node exec cwd handling --- CHANGELOG.md | 1 + src/cli/nodes-cli.coverage.test.ts | 9 ++ src/cli/nodes-cli/register.invoke.ts | 1 + src/node-host/invoke-system-run.test.ts | 67 ++++++++++++++ src/node-host/invoke-system-run.ts | 113 ++++++++++++++++++++++++ 5 files changed, 191 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 83c55a178fa..c472e545356 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,7 @@ Docs: https://docs.openclaw.ai - Models/Auth probes: map permanent auth failover reasons (`auth_permanent`, for example revoked keys) into probe auth status instead of `unknown`, so `openclaw models status --probe` reports actionable auth failures. (#25754) thanks @rrenamed. - Security/Signal: enforce DM/group authorization before reaction-only notification enqueue so unauthorized senders can no longer inject Signal reaction system events under `dmPolicy`/`groupPolicy`; reaction notifications now require channel access checks first. This ships in the next npm release (`2026.2.25`). Thanks @tdjackey for reporting. - Security/Exec approvals: bind `system.run` approval matching to exact argv identity and preserve argv whitespace in rendered command text, preventing trailing-space executable path swaps from reusing a mismatched approval. This ships in the next npm release (`2026.2.25`). Thanks @tdjackey for reporting. +- Security/Exec approvals: harden approval-bound `system.run` execution on node hosts by rejecting symlink `cwd` paths and canonicalizing path-like executable argv before spawn, blocking mutable-cwd symlink retarget chains between approval and execution. This ships in the next npm release (`2026.2.25`). Thanks @tdjackey for reporting. - Security/Discord reactions: enforce DM policy/allowlist authorization before reaction-event system enqueue in direct messages; Discord reaction handling now also honors DM/group-DM enablement and guild `groupPolicy` channel gating to keep reaction ingress aligned with normal message preflight. This ships in the next npm release (`2026.2.25`). Thanks @tdjackey for reporting. - Security/Slack reactions + pins: gate `reaction_*` and `pin_*` system-event enqueue through shared sender authorization so DM `dmPolicy`/`allowFrom` and channel `users` allowlists are enforced consistently for non-message ingress, with regression coverage for denied/allowed sender paths. This ships in the next npm release (`2026.2.25`). Thanks @tdjackey for reporting. - Security/Telegram reactions: enforce `dmPolicy`/`allowFrom` and group allowlist authorization on `message_reaction` events before enqueueing reaction system events, preventing unauthorized reaction-triggered input in DMs and groups; ships in the next npm release (`2026.2.25`). Thanks @tdjackey for reporting. diff --git a/src/cli/nodes-cli.coverage.test.ts b/src/cli/nodes-cli.coverage.test.ts index b8ddc75308c..cd3fe62857d 100644 --- a/src/cli/nodes-cli.coverage.test.ts +++ b/src/cli/nodes-cli.coverage.test.ts @@ -83,6 +83,11 @@ describe("nodes-cli coverage", () => { const getNodeInvokeCall = () => callGateway.mock.calls.find((call) => call[0]?.method === "node.invoke")?.[0] as NodeInvokeCall; + const getApprovalRequestCall = () => + callGateway.mock.calls.find((call) => call[0]?.method === "exec.approval.request")?.[0] as { + params?: Record; + }; + const createNodesProgram = () => { const program = new Command(); program.exitOverride(); @@ -140,6 +145,8 @@ describe("nodes-cli coverage", () => { runId: expect.any(String), }); expect(invoke?.params?.timeoutMs).toBe(5000); + const approval = getApprovalRequestCall(); + expect(approval?.params?.["commandArgv"]).toEqual(["echo", "hi"]); }); it("invokes system.run with raw command", async () => { @@ -165,6 +172,8 @@ describe("nodes-cli coverage", () => { approvalDecision: "allow-once", runId: expect.any(String), }); + const approval = getApprovalRequestCall(); + expect(approval?.params?.["commandArgv"]).toEqual(["/bin/sh", "-lc", "echo hi"]); }); it("invokes system.notify with provided fields", async () => { diff --git a/src/cli/nodes-cli/register.invoke.ts b/src/cli/nodes-cli/register.invoke.ts index a53cc783041..e644d754d12 100644 --- a/src/cli/nodes-cli/register.invoke.ts +++ b/src/cli/nodes-cli/register.invoke.ts @@ -252,6 +252,7 @@ export function registerNodesInvokeCommands(nodes: Command) { { id: approvalId, command: rawCommand ?? argv.join(" "), + commandArgv: argv, cwd: opts.cwd, nodeId, host: "node", diff --git a/src/node-host/invoke-system-run.test.ts b/src/node-host/invoke-system-run.test.ts index d1917199067..2682edd2423 100644 --- a/src/node-host/invoke-system-run.test.ts +++ b/src/node-host/invoke-system-run.test.ts @@ -49,6 +49,7 @@ describe("handleSystemRunInvoke mac app exec host routing", () => { preferMacAppExecHost: boolean; runViaResponse?: ExecHostResponse | null; command?: string[]; + cwd?: string; security?: "full" | "allowlist"; ask?: "off" | "on-miss" | "always"; approved?: boolean; @@ -70,6 +71,7 @@ describe("handleSystemRunInvoke mac app exec host routing", () => { client: {} as never, params: { command: params.command ?? ["echo", "ok"], + cwd: params.cwd, approved: params.approved ?? false, sessionKey: "agent:main:main", }, @@ -214,6 +216,71 @@ describe("handleSystemRunInvoke mac app exec host routing", () => { }), ); }); + + it.runIf(process.platform !== "win32")( + "denies approval-based execution when cwd is a symlink", + async () => { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-approval-cwd-link-")); + const safeDir = path.join(tmp, "safe"); + const linkDir = path.join(tmp, "cwd-link"); + const script = path.join(safeDir, "run.sh"); + fs.mkdirSync(safeDir, { recursive: true }); + fs.writeFileSync(script, "#!/bin/sh\necho SAFE\n"); + fs.chmodSync(script, 0o755); + fs.symlinkSync(safeDir, linkDir, "dir"); + try { + const { runCommand, sendInvokeResult } = await runSystemInvoke({ + preferMacAppExecHost: false, + command: ["./run.sh"], + cwd: linkDir, + approved: true, + security: "full", + ask: "off", + }); + expect(runCommand).not.toHaveBeenCalled(); + expect(sendInvokeResult).toHaveBeenCalledWith( + expect.objectContaining({ + ok: false, + error: expect.objectContaining({ + message: expect.stringContaining("canonical cwd"), + }), + }), + ); + } finally { + fs.rmSync(tmp, { recursive: true, force: true }); + } + }, + ); + + it("uses canonical executable path for approval-based relative command execution", async () => { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-approval-cwd-real-")); + const script = path.join(tmp, "run.sh"); + fs.writeFileSync(script, "#!/bin/sh\necho SAFE\n"); + fs.chmodSync(script, 0o755); + try { + const { runCommand, sendInvokeResult } = await runSystemInvoke({ + preferMacAppExecHost: false, + command: ["./run.sh", "--flag"], + cwd: tmp, + approved: true, + security: "full", + ask: "off", + }); + expect(runCommand).toHaveBeenCalledWith( + [fs.realpathSync(script), "--flag"], + fs.realpathSync(tmp), + undefined, + undefined, + ); + expect(sendInvokeResult).toHaveBeenCalledWith( + expect.objectContaining({ + ok: true, + }), + ); + } finally { + fs.rmSync(tmp, { recursive: true, force: true }); + } + }); it("denies ./sh wrapper spoof in allowlist on-miss mode before execution", async () => { const marker = path.join(os.tmpdir(), `openclaw-wrapper-spoof-${process.pid}-${Date.now()}`); const runCommand = vi.fn(async () => { diff --git a/src/node-host/invoke-system-run.ts b/src/node-host/invoke-system-run.ts index 39e6766f7d5..93edb85e0b7 100644 --- a/src/node-host/invoke-system-run.ts +++ b/src/node-host/invoke-system-run.ts @@ -1,4 +1,6 @@ import crypto from "node:crypto"; +import fs from "node:fs"; +import path from "node:path"; import { resolveAgentConfig } from "../agents/agent-scope.js"; import { loadConfig } from "../config/config.js"; import type { GatewayClient } from "../gateway/client.js"; @@ -18,6 +20,7 @@ import { } 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"; +import { sameFileIdentity } from "../infra/file-identity.js"; import { sanitizeSystemRunEnvOverrides } from "../infra/host-env-security.js"; import { resolveSystemRunCommand } from "../infra/system-run-command.js"; import { evaluateSystemRunPolicy, resolveExecApprovalDecision } from "./exec-policy.js"; @@ -110,6 +113,100 @@ function normalizeDeniedReason(reason: string | null | undefined): SystemRunDeni } } +function isPathLikeExecutableToken(value: string): boolean { + if (!value) { + return false; + } + if (value.startsWith(".") || value.startsWith("/") || value.startsWith("\\")) { + return true; + } + if (value.includes("/") || value.includes("\\")) { + return true; + } + if (process.platform === "win32" && /^[a-zA-Z]:[\\/]/.test(value)) { + return true; + } + return false; +} + +function hardenApprovedExecutionPaths(params: { + approvedByAsk: boolean; + argv: string[]; + shellCommand: string | null; + cwd: string | undefined; +}): { ok: true; argv: string[]; cwd: string | undefined } | { ok: false; message: string } { + if (!params.approvedByAsk) { + return { ok: true, argv: params.argv, cwd: params.cwd }; + } + + let hardenedCwd = params.cwd; + if (hardenedCwd) { + const requestedCwd = path.resolve(hardenedCwd); + let cwdLstat: fs.Stats; + let cwdStat: fs.Stats; + let cwdReal: string; + let cwdRealStat: fs.Stats; + try { + cwdLstat = fs.lstatSync(requestedCwd); + cwdStat = fs.statSync(requestedCwd); + cwdReal = fs.realpathSync(requestedCwd); + cwdRealStat = fs.statSync(cwdReal); + } catch { + return { + ok: false, + message: "SYSTEM_RUN_DENIED: approval requires an existing canonical cwd", + }; + } + if (!cwdStat.isDirectory()) { + return { + ok: false, + message: "SYSTEM_RUN_DENIED: approval requires cwd to be a directory", + }; + } + if (cwdLstat.isSymbolicLink()) { + return { + ok: false, + message: "SYSTEM_RUN_DENIED: approval requires canonical cwd (no symlink cwd)", + }; + } + if ( + !sameFileIdentity(cwdStat, cwdLstat) || + !sameFileIdentity(cwdStat, cwdRealStat) || + !sameFileIdentity(cwdLstat, cwdRealStat) + ) { + return { + ok: false, + message: "SYSTEM_RUN_DENIED: approval cwd identity mismatch", + }; + } + hardenedCwd = cwdReal; + } + + if (params.shellCommand !== null || params.argv.length === 0) { + return { ok: true, argv: params.argv, cwd: hardenedCwd }; + } + + const argv = [...params.argv]; + const rawExecutable = argv[0] ?? ""; + if (!isPathLikeExecutableToken(rawExecutable)) { + return { ok: true, argv, cwd: hardenedCwd }; + } + + const base = hardenedCwd ?? process.cwd(); + const candidate = path.isAbsolute(rawExecutable) + ? rawExecutable + : path.resolve(base, rawExecutable); + try { + argv[0] = fs.realpathSync(candidate); + } catch { + return { + ok: false, + message: "SYSTEM_RUN_DENIED: approval requires a stable executable path", + }; + } + return { ok: true, argv, cwd: hardenedCwd }; +} + export type HandleSystemRunInvokeOptions = { client: GatewayClient; params: SystemRunParams; @@ -422,6 +519,20 @@ async function evaluateSystemRunPolicyPhase( return null; } + const hardenedPaths = hardenApprovedExecutionPaths({ + approvedByAsk: policy.approvedByAsk, + argv: parsed.argv, + shellCommand: parsed.shellCommand, + cwd: parsed.cwd, + }); + if (!hardenedPaths.ok) { + await sendSystemRunDenied(opts, parsed.execution, { + reason: "approval-required", + message: hardenedPaths.message, + }); + return null; + } + const plannedAllowlistArgv = resolvePlannedAllowlistArgv({ security, shellCommand: parsed.shellCommand, @@ -437,6 +548,8 @@ async function evaluateSystemRunPolicyPhase( } return { ...parsed, + argv: hardenedPaths.argv, + cwd: hardenedPaths.cwd, approvals, security, policy,