diff --git a/CHANGELOG.md b/CHANGELOG.md index 0383cfed5e..79be688c99 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,7 @@ Docs: https://docs.openclaw.ai - Security/Node Host: enforce `system.run` rawCommand/argv consistency to prevent allowlist/approval bypass. Thanks @christos-eth. - Security/Exec approvals: prevent safeBins allowlist bypass via shell expansion (host exec allowlist mode only; not enabled by default). Thanks @christos-eth. - Security/Gateway: block `system.execApprovals.*` via `node.invoke` (use `exec.approvals.node.*` instead). Thanks @christos-eth. +- Security/Exec: harden PATH handling by disabling project-local `node_modules/.bin` bootstrapping by default, disallowing node-host `PATH` overrides, and spawning ACP servers via the current executable by default. Thanks @akhmittra. - CLI: fix lazy core command registration so top-level maintenance commands (`doctor`, `dashboard`, `reset`, `uninstall`) resolve correctly instead of exposing a non-functional `maintenance` placeholder command. - Security/Agents: scope CLI process cleanup to owned child PIDs to avoid killing unrelated processes on shared hosts. Thanks @aether-ai-agent. - Security/Agents (macOS): prevent shell injection when writing Claude CLI keychain credentials. (#15924) Thanks @aether-ai-agent. diff --git a/src/acp/client.ts b/src/acp/client.ts index e5fc447dd8..80cbda6013 100644 --- a/src/acp/client.ts +++ b/src/acp/client.ts @@ -7,8 +7,11 @@ import { type SessionNotification, } from "@agentclientprotocol/sdk"; import { spawn, type ChildProcess } from "node:child_process"; +import fs from "node:fs"; +import path from "node:path"; import * as readline from "node:readline"; import { Readable, Writable } from "node:stream"; +import { fileURLToPath } from "node:url"; import { ensureOpenClawCliOnPath } from "../infra/path-env.js"; import { DANGEROUS_ACP_TOOLS } from "../security/dangerous-tools.js"; @@ -260,6 +263,25 @@ function buildServerArgs(opts: AcpClientOptions): string[] { return args; } +function resolveSelfEntryPath(): string | null { + // Prefer a path relative to the built module location (dist/acp/client.js -> dist/entry.js). + try { + const here = fileURLToPath(import.meta.url); + const candidate = path.resolve(path.dirname(here), "..", "entry.js"); + if (fs.existsSync(candidate)) { + return candidate; + } + } catch { + // ignore + } + + const argv1 = process.argv[1]?.trim(); + if (argv1) { + return path.isAbsolute(argv1) ? argv1 : path.resolve(process.cwd(), argv1); + } + return null; +} + function printSessionUpdate(notification: SessionNotification): void { const update = notification.update; if (!("sessionUpdate" in update)) { @@ -300,13 +322,16 @@ export async function createAcpClient(opts: AcpClientOptions = {}): Promise console.error(`[acp-client] ${msg}`) : () => {}; - ensureOpenClawCliOnPath({ cwd }); - const serverCommand = opts.serverCommand ?? "openclaw"; + ensureOpenClawCliOnPath(); const serverArgs = buildServerArgs(opts); - log(`spawning: ${serverCommand} ${serverArgs.join(" ")}`); + const entryPath = resolveSelfEntryPath(); + const serverCommand = opts.serverCommand ?? (entryPath ? process.execPath : "openclaw"); + const effectiveArgs = opts.serverCommand || !entryPath ? serverArgs : [entryPath, ...serverArgs]; - const agent = spawn(serverCommand, serverArgs, { + log(`spawning: ${serverCommand} ${effectiveArgs.join(" ")}`); + + const agent = spawn(serverCommand, effectiveArgs, { stdio: ["pipe", "pipe", "inherit"], cwd, }); diff --git a/src/infra/path-env.test.ts b/src/infra/path-env.test.ts index 49d577ce3e..50d7ec3ac0 100644 --- a/src/infra/path-env.test.ts +++ b/src/infra/path-env.test.ts @@ -75,12 +75,6 @@ describe("ensureOpenClawCliOnPath", () => { await fs.writeFile(appCli, "#!/bin/sh\necho ok\n", "utf-8"); await fs.chmod(appCli, 0o755); - const localBinDir = path.join(tmp, "node_modules", ".bin"); - await fs.mkdir(localBinDir, { recursive: true }); - const localCli = path.join(localBinDir, "openclaw"); - await fs.writeFile(localCli, "#!/bin/sh\necho ok\n", "utf-8"); - await fs.chmod(localCli, 0o755); - const miseDataDir = path.join(tmp, "mise"); const shimsDir = path.join(miseDataDir, "shims"); await fs.mkdir(shimsDir, { recursive: true }); @@ -98,11 +92,9 @@ describe("ensureOpenClawCliOnPath", () => { const updated = process.env.PATH ?? ""; const parts = updated.split(path.delimiter); const appBinIndex = parts.indexOf(appBinDir); - const localIndex = parts.indexOf(localBinDir); const shimsIndex = parts.indexOf(shimsDir); expect(appBinIndex).toBeGreaterThanOrEqual(0); - expect(localIndex).toBeGreaterThan(appBinIndex); - expect(shimsIndex).toBeGreaterThan(localIndex); + expect(shimsIndex).toBeGreaterThan(appBinIndex); } finally { process.env.PATH = originalPath; if (originalFlag === undefined) { @@ -119,6 +111,61 @@ describe("ensureOpenClawCliOnPath", () => { } }); + it("only appends project-local node_modules/.bin when explicitly enabled", async () => { + const tmp = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-path-")); + const originalPath = process.env.PATH; + const originalFlag = process.env.OPENCLAW_PATH_BOOTSTRAPPED; + try { + const appBinDir = path.join(tmp, "AppBin"); + await fs.mkdir(appBinDir, { recursive: true }); + const appCli = path.join(appBinDir, "openclaw"); + await fs.writeFile(appCli, "#!/bin/sh\necho ok\n", "utf-8"); + await fs.chmod(appCli, 0o755); + + const localBinDir = path.join(tmp, "node_modules", ".bin"); + await fs.mkdir(localBinDir, { recursive: true }); + const localCli = path.join(localBinDir, "openclaw"); + await fs.writeFile(localCli, "#!/bin/sh\necho ok\n", "utf-8"); + await fs.chmod(localCli, 0o755); + + process.env.PATH = "/usr/bin"; + delete process.env.OPENCLAW_PATH_BOOTSTRAPPED; + + ensureOpenClawCliOnPath({ + execPath: appCli, + cwd: tmp, + homeDir: tmp, + platform: "darwin", + }); + const withoutOptIn = (process.env.PATH ?? "").split(path.delimiter); + expect(withoutOptIn.includes(localBinDir)).toBe(false); + + process.env.PATH = "/usr/bin"; + delete process.env.OPENCLAW_PATH_BOOTSTRAPPED; + + ensureOpenClawCliOnPath({ + execPath: appCli, + cwd: tmp, + homeDir: tmp, + platform: "darwin", + allowProjectLocalBin: true, + }); + const withOptIn = (process.env.PATH ?? "").split(path.delimiter); + const usrBinIndex = withOptIn.indexOf("/usr/bin"); + const localIndex = withOptIn.indexOf(localBinDir); + expect(usrBinIndex).toBeGreaterThanOrEqual(0); + expect(localIndex).toBeGreaterThan(usrBinIndex); + } finally { + process.env.PATH = originalPath; + if (originalFlag === undefined) { + delete process.env.OPENCLAW_PATH_BOOTSTRAPPED; + } else { + process.env.OPENCLAW_PATH_BOOTSTRAPPED = originalFlag; + } + await fs.rm(tmp, { recursive: true, force: true }); + } + }); + it("prepends Linuxbrew dirs when present", async () => { const tmp = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-path-")); const originalPath = process.env.PATH; diff --git a/src/infra/path-env.ts b/src/infra/path-env.ts index dc7458789b..f00201a962 100644 --- a/src/infra/path-env.ts +++ b/src/infra/path-env.ts @@ -10,6 +10,7 @@ type EnsureOpenClawPathOpts = { homeDir?: string; platform?: NodeJS.Platform; pathEnv?: string; + allowProjectLocalBin?: boolean; }; function isExecutable(filePath: string): boolean { @@ -29,16 +30,17 @@ function isDirectory(dirPath: string): boolean { } } -function mergePath(params: { existing: string; prepend: string[] }): string { +function mergePath(params: { existing: string; prepend?: string[]; append?: string[] }): string { const partsExisting = params.existing .split(path.delimiter) .map((part) => part.trim()) .filter(Boolean); - const partsPrepend = params.prepend.map((part) => part.trim()).filter(Boolean); + const partsPrepend = (params.prepend ?? []).map((part) => part.trim()).filter(Boolean); + const partsAppend = (params.append ?? []).map((part) => part.trim()).filter(Boolean); const seen = new Set(); const merged: string[] = []; - for (const part of [...partsPrepend, ...partsExisting]) { + for (const part of [...partsPrepend, ...partsExisting, ...partsAppend]) { if (!seen.has(part)) { seen.add(part); merged.push(part); @@ -47,54 +49,60 @@ function mergePath(params: { existing: string; prepend: string[] }): string { return merged.join(path.delimiter); } -function candidateBinDirs(opts: EnsureOpenClawPathOpts): string[] { +function candidateBinDirs(opts: EnsureOpenClawPathOpts): { prepend: string[]; append: string[] } { const execPath = opts.execPath ?? process.execPath; const cwd = opts.cwd ?? process.cwd(); const homeDir = opts.homeDir ?? os.homedir(); const platform = opts.platform ?? process.platform; - const candidates: string[] = []; + const prepend: string[] = []; + const append: string[] = []; // Bundled macOS app: `openclaw` lives next to the executable (process.execPath). try { const execDir = path.dirname(execPath); const siblingCli = path.join(execDir, "openclaw"); if (isExecutable(siblingCli)) { - candidates.push(execDir); + prepend.push(execDir); } } catch { // ignore } - // Project-local installs (best effort): if a `node_modules/.bin/openclaw` exists near cwd, - // include it. This helps when running under launchd or other minimal PATH environments. - const localBinDir = path.join(cwd, "node_modules", ".bin"); - if (isExecutable(path.join(localBinDir, "openclaw"))) { - candidates.push(localBinDir); + // Project-local installs are a common repo-based attack vector (bin hijacking). Keep this + // disabled by default; if an operator explicitly enables it, only append (never prepend). + const allowProjectLocalBin = + opts.allowProjectLocalBin === true || + isTruthyEnvValue(process.env.OPENCLAW_ALLOW_PROJECT_LOCAL_BIN); + if (allowProjectLocalBin) { + const localBinDir = path.join(cwd, "node_modules", ".bin"); + if (isExecutable(path.join(localBinDir, "openclaw"))) { + append.push(localBinDir); + } } const miseDataDir = process.env.MISE_DATA_DIR ?? path.join(homeDir, ".local", "share", "mise"); const miseShims = path.join(miseDataDir, "shims"); if (isDirectory(miseShims)) { - candidates.push(miseShims); + prepend.push(miseShims); } - candidates.push(...resolveBrewPathDirs({ homeDir })); + prepend.push(...resolveBrewPathDirs({ homeDir })); // Common global install locations (macOS first). if (platform === "darwin") { - candidates.push(path.join(homeDir, "Library", "pnpm")); + prepend.push(path.join(homeDir, "Library", "pnpm")); } if (process.env.XDG_BIN_HOME) { - candidates.push(process.env.XDG_BIN_HOME); + prepend.push(process.env.XDG_BIN_HOME); } - candidates.push(path.join(homeDir, ".local", "bin")); - candidates.push(path.join(homeDir, ".local", "share", "pnpm")); - candidates.push(path.join(homeDir, ".bun", "bin")); - candidates.push(path.join(homeDir, ".yarn", "bin")); - candidates.push("/opt/homebrew/bin", "/usr/local/bin", "/usr/bin", "/bin"); + prepend.push(path.join(homeDir, ".local", "bin")); + prepend.push(path.join(homeDir, ".local", "share", "pnpm")); + prepend.push(path.join(homeDir, ".bun", "bin")); + prepend.push(path.join(homeDir, ".yarn", "bin")); + prepend.push("/opt/homebrew/bin", "/usr/local/bin", "/usr/bin", "/bin"); - return candidates.filter(isDirectory); + return { prepend: prepend.filter(isDirectory), append: append.filter(isDirectory) }; } /** @@ -108,12 +116,12 @@ export function ensureOpenClawCliOnPath(opts: EnsureOpenClawPathOpts = {}) { process.env.OPENCLAW_PATH_BOOTSTRAPPED = "1"; const existing = opts.pathEnv ?? process.env.PATH ?? ""; - const prepend = candidateBinDirs(opts); - if (prepend.length === 0) { + const { prepend, append } = candidateBinDirs(opts); + if (prepend.length === 0 && append.length === 0) { return; } - const merged = mergePath({ existing, prepend }); + const merged = mergePath({ existing, prepend, append }); if (merged) { process.env.PATH = merged; } diff --git a/src/node-host/invoke.sanitize-env.test.ts b/src/node-host/invoke.sanitize-env.test.ts new file mode 100644 index 0000000000..27f092d0aa --- /dev/null +++ b/src/node-host/invoke.sanitize-env.test.ts @@ -0,0 +1,48 @@ +import { describe, expect, it } from "vitest"; +import { sanitizeEnv } from "./invoke.js"; + +describe("node-host sanitizeEnv", () => { + it("ignores PATH overrides", () => { + const prev = process.env.PATH; + process.env.PATH = "/usr/bin"; + try { + const env = sanitizeEnv({ PATH: "/tmp/evil:/usr/bin" }) ?? {}; + expect(env.PATH).toBe("/usr/bin"); + } finally { + if (prev === undefined) { + delete process.env.PATH; + } else { + process.env.PATH = prev; + } + } + }); + + it("blocks dangerous env keys/prefixes", () => { + const prevPythonPath = process.env.PYTHONPATH; + const prevLdPreload = process.env.LD_PRELOAD; + try { + delete process.env.PYTHONPATH; + delete process.env.LD_PRELOAD; + const env = + sanitizeEnv({ + PYTHONPATH: "/tmp/pwn", + LD_PRELOAD: "/tmp/pwn.so", + FOO: "bar", + }) ?? {}; + expect(env.FOO).toBe("bar"); + expect(env.PYTHONPATH).toBeUndefined(); + expect(env.LD_PRELOAD).toBeUndefined(); + } finally { + if (prevPythonPath === undefined) { + delete process.env.PYTHONPATH; + } else { + process.env.PYTHONPATH = prevPythonPath; + } + if (prevLdPreload === undefined) { + delete process.env.LD_PRELOAD; + } else { + process.env.LD_PRELOAD = prevLdPreload; + } + } + }); +}); diff --git a/src/node-host/invoke.ts b/src/node-host/invoke.ts index 313d66154b..7a6a4d21a1 100644 --- a/src/node-host/invoke.ts +++ b/src/node-host/invoke.ts @@ -135,33 +135,22 @@ function resolveExecAsk(value?: string): ExecAsk { return value === "off" || value === "on-miss" || value === "always" ? value : "on-miss"; } -function sanitizeEnv( +export function sanitizeEnv( overrides?: Record | null, ): Record | undefined { if (!overrides) { return undefined; } const merged = { ...process.env } as Record; - const basePath = process.env.PATH ?? DEFAULT_NODE_PATH; for (const [rawKey, value] of Object.entries(overrides)) { const key = rawKey.trim(); if (!key) { continue; } const upper = key.toUpperCase(); + // PATH is part of the security boundary (command resolution + safe-bin checks). Never allow + // request-scoped PATH overrides from agents/gateways. if (upper === "PATH") { - const trimmed = value.trim(); - if (!trimmed) { - continue; - } - if (!basePath || trimmed === basePath) { - merged[key] = trimmed; - continue; - } - const suffix = `${path.delimiter}${basePath}`; - if (trimmed.endsWith(suffix)) { - merged[key] = trimmed; - } continue; } if (blockedEnvKeys.has(upper)) {