diff --git a/CHANGELOG.md b/CHANGELOG.md index 63f30850777..e3159266f1e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ Docs: https://docs.openclaw.ai ### Fixes +- fix(exec): replace TOCTOU check-then-read with atomic pinned-fd open in script preflight [AI]. (#62333) Thanks @pgondhi987. - WhatsApp/auto-reply: keep inbound reply, media, and composing sends on the current socket across reconnects, wait through reconnect gaps, and retry timeout-only send failures without dropping the active socket ref. (#62892) Thanks @mcaxtr. - Config/plugins: let config writes keep disabled plugin entries without forcing required plugin config schemas or crashing raw plugin validation, so slot switches and similar plugin-state updates persist cleanly. (#63296) Thanks @fuller-stack-dev. diff --git a/src/agents/bash-tools.exec.script-preflight.test.ts b/src/agents/bash-tools.exec.script-preflight.test.ts index bd21cfd5391..b7b7f8a1837 100644 --- a/src/agents/bash-tools.exec.script-preflight.test.ts +++ b/src/agents/bash-tools.exec.script-preflight.test.ts @@ -1,6 +1,7 @@ +import { constants as fsConstants } from "node:fs"; import fs from "node:fs/promises"; import path from "node:path"; -import { describe, expect, it } from "vitest"; +import { describe, expect, it, vi } from "vitest"; import { withTempDir } from "../test-utils/temp-dir.js"; import { createExecTool } from "./bash-tools.exec.js"; @@ -74,6 +75,54 @@ describeNonWin("exec script preflight", () => { }); }); + it("validates in-workdir scripts whose names start with '..'", async () => { + await withTempDir("openclaw-exec-preflight-", async (tmp) => { + const jsPath = path.join(tmp, "..bad.js"); + await fs.writeFile(jsPath, "const value = $DM_JSON;", "utf-8"); + + const tool = createExecTool({ host: "gateway", security: "full", ask: "off" }); + await expect( + tool.execute("call-dotdot-prefix-script", { + command: "node ..bad.js", + workdir: tmp, + }), + ).rejects.toThrow(/exec preflight: detected likely shell variable injection \(\$DM_JSON\)/); + }); + }); + + it("validates in-workdir symlinked script entrypoints", async () => { + await withTempDir("openclaw-exec-preflight-", async (tmp) => { + const targetPath = path.join(tmp, "bad-target.js"); + const linkPath = path.join(tmp, "link.js"); + await fs.writeFile(targetPath, "const value = $DM_JSON;", "utf-8"); + await fs.symlink(targetPath, linkPath); + + const tool = createExecTool({ host: "gateway", security: "full", ask: "off" }); + await expect( + tool.execute("call-symlink-entrypoint", { + command: "node link.js", + workdir: tmp, + }), + ).rejects.toThrow(/exec preflight: detected likely shell variable injection \(\$DM_JSON\)/); + }); + }); + + it("validates scripts under literal tilde directories in workdir", async () => { + await withTempDir("openclaw-exec-preflight-", async (tmp) => { + const literalTildeDir = path.join(tmp, "~"); + await fs.mkdir(literalTildeDir, { recursive: true }); + await fs.writeFile(path.join(literalTildeDir, "bad.js"), "const value = $DM_JSON;", "utf-8"); + + const tool = createExecTool({ host: "gateway", security: "full", ask: "off" }); + await expect( + tool.execute("call-literal-tilde-path", { + command: 'node "~/bad.js"', + workdir: tmp, + }), + ).rejects.toThrow(/exec preflight: detected likely shell variable injection \(\$DM_JSON\)/); + }); + }); + it("validates python scripts when interpreter is prefixed with env", async () => { await withTempDir("openclaw-exec-preflight-", async (tmp) => { const pyPath = path.join(tmp, "bad.py"); @@ -268,6 +317,115 @@ describeNonWin("exec script preflight", () => { }); }); + it("does not trust a swapped script pathname between validation and read", async () => { + await withTempDir("openclaw-exec-preflight-race-", async (parent) => { + const workdir = path.join(parent, "workdir"); + const scriptPath = path.join(workdir, "script.js"); + const outsidePath = path.join(parent, "outside.js"); + await fs.mkdir(workdir, { recursive: true }); + await fs.writeFile(scriptPath, 'console.log("inside")', "utf-8"); + await fs.writeFile(outsidePath, 'console.log("$DM_JSON outside")', "utf-8"); + + const originalStat = fs.stat.bind(fs); + let swapped = false; + const statSpy = vi.spyOn(fs, "stat").mockImplementation(async (...args) => { + const target = args[0]; + if (!swapped && typeof target === "string" && path.resolve(target) === scriptPath) { + const original = await originalStat(target); + await fs.rm(scriptPath, { force: true }); + await fs.symlink(outsidePath, scriptPath); + swapped = true; + return original; + } + return await originalStat(...args); + }); + + try { + const tool = createExecTool({ host: "gateway", security: "full", ask: "off" }); + const result = await tool.execute("call-swapped-pathname", { + command: "node script.js", + workdir, + }); + const text = result.content.find((block) => block.type === "text")?.text ?? ""; + expect(swapped).toBe(true); + expect(text).not.toMatch(/exec preflight:/); + } finally { + statSpy.mockRestore(); + } + }); + }); + + it("handles pre-open symlink swaps without surfacing preflight errors", async () => { + await withTempDir("openclaw-exec-preflight-open-race-", async (parent) => { + const workdir = path.join(parent, "workdir"); + const scriptPath = path.join(workdir, "script.js"); + const outsidePath = path.join(parent, "outside.js"); + await fs.mkdir(workdir, { recursive: true }); + await fs.writeFile(scriptPath, 'console.log("inside")', "utf-8"); + await fs.writeFile(outsidePath, 'console.log("$DM_JSON outside")', "utf-8"); + + const originalOpen = fs.open.bind(fs); + let swapped = false; + const openSpy = vi.spyOn(fs, "open").mockImplementation(async (...args) => { + const target = args[0]; + if (!swapped && typeof target === "string" && path.resolve(target) === scriptPath) { + await fs.rm(scriptPath, { force: true }); + await fs.symlink(outsidePath, scriptPath); + swapped = true; + } + return await originalOpen(...args); + }); + + try { + const tool = createExecTool({ host: "gateway", security: "full", ask: "off" }); + const result = await tool.execute("call-pre-open-swapped-pathname", { + command: "node script.js", + workdir, + }); + const text = result.content.find((block) => block.type === "text")?.text ?? ""; + expect(swapped).toBe(true); + expect(text).not.toMatch(/exec preflight:/); + } finally { + openSpy.mockRestore(); + } + }); + }); + + it("opens preflight script reads with O_NONBLOCK to avoid FIFO stalls", async () => { + await withTempDir("openclaw-exec-preflight-nonblock-", async (tmp) => { + const scriptPath = path.join(tmp, "script.js"); + await fs.writeFile(scriptPath, 'console.log("ok")', "utf-8"); + + const originalOpen = fs.open.bind(fs); + const scriptOpenFlags: number[] = []; + const openSpy = vi.spyOn(fs, "open").mockImplementation(async (...args) => { + const [target, flags] = args; + if ( + typeof target === "string" && + path.resolve(target) === scriptPath && + typeof flags === "number" + ) { + scriptOpenFlags.push(flags); + } + return await originalOpen(...args); + }); + + try { + const tool = createExecTool({ host: "gateway", security: "full", ask: "off" }); + const result = await tool.execute("call-nonblocking-preflight-open", { + command: "node script.js", + workdir: tmp, + }); + const text = result.content.find((block) => block.type === "text")?.text ?? ""; + expect(scriptOpenFlags.length).toBeGreaterThan(0); + expect(scriptOpenFlags.some((flags) => (flags & fsConstants.O_NONBLOCK) !== 0)).toBe(true); + expect(text).not.toMatch(/exec preflight:/); + } finally { + openSpy.mockRestore(); + } + }); + }); + it("fails closed for piped interpreter commands that bypass direct script parsing", async () => { await withTempDir("openclaw-exec-preflight-", async (tmp) => { const pyPath = path.join(tmp, "bad.py"); diff --git a/src/agents/bash-tools.exec.ts b/src/agents/bash-tools.exec.ts index 7e4f2c4a1e5..9cedfc9d325 100644 --- a/src/agents/bash-tools.exec.ts +++ b/src/agents/bash-tools.exec.ts @@ -1,4 +1,3 @@ -import fs from "node:fs/promises"; import path from "node:path"; import type { AgentToolResult } from "@mariozechner/pi-agent-core"; import { analyzeShellCommand } from "../infra/exec-approvals-analysis.js"; @@ -10,6 +9,7 @@ import { resolveExecApprovalsFromFile, } from "../infra/exec-approvals.js"; import { resolveExecSafeBinRuntimePolicy } from "../infra/exec-safe-bin-runtime-policy.js"; +import { SafeOpenError, readFileWithinRoot } from "../infra/fs-safe.js"; import { sanitizeHostExecEnvWithDiagnostics } from "../infra/host-env-security.js"; import { getShellPathFromLoginShell, @@ -56,7 +56,6 @@ import { resolveWorkdir, truncateMiddle, } from "./bash-tools.shared.js"; -import { assertSandboxPath } from "./sandbox-paths.js"; import { EXEC_TOOL_DISPLAY_SUMMARY } from "./tool-description-presets.js"; import { type AgentToolWithMeta, failedTextResult, textResult } from "./tools/common.js"; @@ -105,6 +104,44 @@ const PREFLIGHT_ENV_OPTIONS_WITH_VALUES = new Set([ "--unset", ]); +const SKIPPABLE_SCRIPT_PREFLIGHT_FS_ERROR_CODES = new Set([ + "EACCES", + "EISDIR", + "ELOOP", + "EINVAL", + "ENAMETOOLONG", + "ENOENT", + "ENOTDIR", + "EPERM", +]); + +function getNodeErrorCode(error: unknown): string | undefined { + if (typeof error !== "object" || error === null || !("code" in error)) { + return undefined; + } + return String((error as { code?: unknown }).code); +} + +function shouldSkipScriptPreflightPathError(error: unknown): boolean { + if (error instanceof SafeOpenError) { + return true; + } + const errorCode = getNodeErrorCode(error); + return !!(errorCode && SKIPPABLE_SCRIPT_PREFLIGHT_FS_ERROR_CODES.has(errorCode)); +} + +function resolvePreflightRelativePath(params: { rootDir: string; absPath: string }): string | null { + const root = path.resolve(params.rootDir); + const candidate = path.resolve(params.absPath); + const relative = path.relative(root, candidate); + if (/^\.\.(?:[\\/]|$)/u.test(relative) || path.isAbsolute(relative)) { + return null; + } + // Preserve literal "~" path segments under the workdir. `readFileWithinRoot` + // expands home prefixes for relative paths, so normalize `~/...` to `./~/...`. + return /^~(?:$|[\\/])/u.test(relative) ? `.${path.sep}${relative}` : relative; +} + function isShellEnvAssignmentToken(token: string): boolean { return /^[A-Za-z_][A-Za-z0-9_]*=.*$/u.test(token); } @@ -921,27 +958,36 @@ async function validateScriptFileForShellBleed(params: { const absPath = path.isAbsolute(relOrAbsPath) ? path.resolve(relOrAbsPath) : path.resolve(params.workdir, relOrAbsPath); + const relativePath = resolvePreflightRelativePath({ + rootDir: params.workdir, + absPath, + }); + if (!relativePath) { + continue; + } - // Best-effort: only validate if file exists and is reasonably small. - let stat: { isFile(): boolean; size: number }; + // Best-effort: only validate files that safely resolve within workdir and + // are reasonably small. This keeps preflight checks on a pinned file + // identity instead of trusting mutable pathnames across multiple ops. + // Use non-blocking open to avoid stalls if a path is swapped to a FIFO. + let content: string; try { - await assertSandboxPath({ - filePath: absPath, - cwd: params.workdir, - root: params.workdir, + const safeRead = await readFileWithinRoot({ + rootDir: params.workdir, + relativePath, + nonBlockingRead: true, + allowSymlinkTargetWithinRoot: true, + maxBytes: 512 * 1024, }); - stat = await fs.stat(absPath); - } catch { - continue; + content = safeRead.buffer.toString("utf-8"); + } catch (error) { + if (shouldSkipScriptPreflightPathError(error)) { + // Preflight validation is best-effort: skip path/read failures and + // continue to execute the command normally. + continue; + } + throw error; } - if (!stat.isFile()) { - continue; - } - if (stat.size > 512 * 1024) { - continue; - } - - const content = await fs.readFile(absPath, "utf-8"); // Common failure mode: shell env var syntax leaking into Python/JS. // We deliberately match all-caps/underscore vars to avoid false positives with `$` as a JS identifier. diff --git a/src/infra/fs-safe.ts b/src/infra/fs-safe.ts index 509dc26f86f..2814fb56d9b 100644 --- a/src/infra/fs-safe.ts +++ b/src/infra/fs-safe.ts @@ -51,7 +51,11 @@ export type SafeLocalReadResult = { }; const SUPPORTS_NOFOLLOW = process.platform !== "win32" && "O_NOFOLLOW" in fsConstants; +const NONBLOCK_OPEN_FLAG = "O_NONBLOCK" in fsConstants ? fsConstants.O_NONBLOCK : 0; const OPEN_READ_FLAGS = fsConstants.O_RDONLY | (SUPPORTS_NOFOLLOW ? fsConstants.O_NOFOLLOW : 0); +const OPEN_READ_NONBLOCK_FLAGS = OPEN_READ_FLAGS | NONBLOCK_OPEN_FLAG; +const OPEN_READ_FOLLOW_FLAGS = fsConstants.O_RDONLY; +const OPEN_READ_FOLLOW_NONBLOCK_FLAGS = OPEN_READ_FOLLOW_FLAGS | NONBLOCK_OPEN_FLAG; const OPEN_WRITE_EXISTING_FLAGS = fsConstants.O_WRONLY | (SUPPORTS_NOFOLLOW ? fsConstants.O_NOFOLLOW : 0); const OPEN_WRITE_CREATE_FLAGS = @@ -84,6 +88,8 @@ async function openVerifiedLocalFile( filePath: string, options?: { rejectHardlinks?: boolean; + nonBlockingRead?: boolean; + allowSymlinkTargetWithinRoot?: boolean; }, ): Promise { // Reject directories before opening so we never surface EISDIR to callers (e.g. tool @@ -102,7 +108,14 @@ async function openVerifiedLocalFile( let handle: FileHandle; try { - handle = await fs.open(filePath, OPEN_READ_FLAGS); + const openFlags = options?.allowSymlinkTargetWithinRoot + ? options?.nonBlockingRead + ? OPEN_READ_FOLLOW_NONBLOCK_FLAGS + : OPEN_READ_FOLLOW_FLAGS + : options?.nonBlockingRead + ? OPEN_READ_NONBLOCK_FLAGS + : OPEN_READ_FLAGS; + handle = await fs.open(filePath, openFlags); } catch (err) { if (isNotFoundPathError(err)) { throw new SafeOpenError("not-found", "file not found"); @@ -118,8 +131,11 @@ async function openVerifiedLocalFile( } try { - const [stat, lstat] = await Promise.all([handle.stat(), fs.lstat(filePath)]); - if (lstat.isSymbolicLink()) { + const [stat, pathStat] = await Promise.all([ + handle.stat(), + options?.allowSymlinkTargetWithinRoot ? fs.stat(filePath) : fs.lstat(filePath), + ]); + if (!options?.allowSymlinkTargetWithinRoot && pathStat.isSymbolicLink()) { throw new SafeOpenError("symlink", "symlink not allowed"); } if (!stat.isFile()) { @@ -128,7 +144,7 @@ async function openVerifiedLocalFile( if (options?.rejectHardlinks && stat.nlink > 1) { throw new SafeOpenError("invalid-path", "hardlinked path not allowed"); } - if (!sameFileIdentity(stat, lstat)) { + if (!sameFileIdentity(stat, pathStat)) { throw new SafeOpenError("path-mismatch", "path changed during read"); } @@ -180,12 +196,17 @@ export async function openFileWithinRoot(params: { rootDir: string; relativePath: string; rejectHardlinks?: boolean; + nonBlockingRead?: boolean; + allowSymlinkTargetWithinRoot?: boolean; }): Promise { const { rootWithSep, resolved } = await resolvePathWithinRoot(params); let opened: SafeOpenResult; try { - opened = await openVerifiedLocalFile(resolved); + opened = await openVerifiedLocalFile(resolved, { + nonBlockingRead: params.nonBlockingRead, + allowSymlinkTargetWithinRoot: params.allowSymlinkTargetWithinRoot, + }); } catch (err) { if (err instanceof SafeOpenError) { if (err.code === "not-found") { @@ -215,12 +236,16 @@ export async function readFileWithinRoot(params: { rootDir: string; relativePath: string; rejectHardlinks?: boolean; + nonBlockingRead?: boolean; + allowSymlinkTargetWithinRoot?: boolean; maxBytes?: number; }): Promise { const opened = await openFileWithinRoot({ rootDir: params.rootDir, relativePath: params.relativePath, rejectHardlinks: params.rejectHardlinks, + nonBlockingRead: params.nonBlockingRead, + allowSymlinkTargetWithinRoot: params.allowSymlinkTargetWithinRoot, }); try { return await readOpenedFileSafely({ opened, maxBytes: params.maxBytes }); diff --git a/src/infra/path-guards.test.ts b/src/infra/path-guards.test.ts index 9c2aa69edc5..a9090e56c3f 100644 --- a/src/infra/path-guards.test.ts +++ b/src/infra/path-guards.test.ts @@ -72,6 +72,7 @@ describe("isPathInside", () => { it.each([ ["/workspace/root", "/workspace/root", true], ["/workspace/root", "/workspace/root/nested/file.txt", true], + ["/workspace/root", "/workspace/root/..file.txt", true], ["/workspace/root", "/workspace/root/../escape.txt", false], ])("checks posix containment %s -> %s", (basePath, targetPath, expected) => { expect(isPathInside(basePath, targetPath)).toBe(expected); @@ -83,6 +84,7 @@ describe("isPathInside", () => { for (const [basePath, targetPath, expected] of [ [String.raw`C:\workspace\root`, String.raw`C:\workspace\root`, true], [String.raw`C:\workspace\root`, String.raw`C:\workspace\root\Nested\File.txt`, true], + [String.raw`C:\workspace\root`, String.raw`C:\workspace\root\..file.txt`, true], [String.raw`C:\workspace\root`, String.raw`C:\workspace\root\..\escape.txt`, false], [String.raw`C:\workspace\root`, String.raw`D:\workspace\root\file.txt`, false], ] as const) { diff --git a/src/infra/path-guards.ts b/src/infra/path-guards.ts index 3cb3a3d80db..e40c4649e1a 100644 --- a/src/infra/path-guards.ts +++ b/src/infra/path-guards.ts @@ -3,6 +3,7 @@ import { normalizeLowercaseStringOrEmpty } from "../shared/string-coerce.js"; const NOT_FOUND_CODES = new Set(["ENOENT", "ENOTDIR"]); const SYMLINK_OPEN_CODES = new Set(["ELOOP", "EINVAL", "ENOTSUP"]); +const PARENT_SEGMENT_PREFIX = /^\.\.(?:[\\/]|$)/u; export function normalizeWindowsPathForComparison(input: string): string { let normalized = path.win32.normalize(input); @@ -38,11 +39,13 @@ export function isPathInside(root: string, target: string): boolean { const rootForCompare = normalizeWindowsPathForComparison(path.win32.resolve(root)); const targetForCompare = normalizeWindowsPathForComparison(path.win32.resolve(target)); const relative = path.win32.relative(rootForCompare, targetForCompare); - return relative === "" || (!relative.startsWith("..") && !path.win32.isAbsolute(relative)); + return ( + relative === "" || (!PARENT_SEGMENT_PREFIX.test(relative) && !path.win32.isAbsolute(relative)) + ); } const resolvedRoot = path.resolve(root); const resolvedTarget = path.resolve(target); const relative = path.relative(resolvedRoot, resolvedTarget); - return relative === "" || (!relative.startsWith("..") && !path.isAbsolute(relative)); + return relative === "" || (!PARENT_SEGMENT_PREFIX.test(relative) && !path.isAbsolute(relative)); }