From 710004e011fd587d0f79426a6c8224d023d3bc50 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Mon, 2 Mar 2026 01:27:17 +0000 Subject: [PATCH] fix(security): harden root-scoped writes against symlink races --- CHANGELOG.md | 1 + .../server-methods/agents-mutate.test.ts | 2 + src/infra/fs-safe.test.ts | 83 ++++++++++++++++++- src/infra/fs-safe.ts | 29 ++++++- 4 files changed, 111 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ae589420381..b58e2b56b65 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -104,6 +104,7 @@ Docs: https://docs.openclaw.ai - Security/Sandbox media reads: eliminate sandbox media TOCTOU symlink-retarget escapes by enforcing root-scoped boundary-safe reads at attachment/image load time and consolidating shared safe-read helpers across sandbox media callsites. This ships in the next npm release. Thanks @tdjackey for reporting. - Security/Subagents sandbox inheritance: block sandboxed sessions from spawning cross-agent subagents that would run unsandboxed, preventing runtime sandbox downgrade via `sessions_spawn agentId`. Thanks @tdjackey for reporting. +- Security/Workspace safe writes: harden `writeFileWithinRoot` against symlink-retarget TOCTOU races by opening existing files without truncation, creating missing files with exclusive create, deferring truncation until post-open identity+boundary validation, and removing out-of-root create artifacts on blocked races; added regression tests for truncate/create race paths. This ships in the next npm release (`2026.3.1`). Thanks @tdjackey for reporting. - Security/Node metadata policy: harden node platform classification against Unicode confusables and switch unknown platform defaults to a conservative allowlist that excludes `system.run`/`system.which` unless explicitly allowlisted, preventing metadata canonicalization drift from broadening node command permissions. Thanks @tdjackey for reporting. - Plugins/Discovery precedence: load bundled plugins before auto-discovered global extensions so bundled channel plugins win duplicate-ID resolution by default (explicit `plugins.load.paths` overrides remain highest precedence), with loader regression coverage. Landed from contributor PR #29710 by @Sid-Qin. Thanks @Sid-Qin. - Discord/Reconnect integrity: release Discord message listener lane immediately while preserving serialized handler execution, add HELLO-stall resume-first recovery with bounded fresh-identify fallback after repeated stalls, and extend lifecycle/listener regression coverage for forced reconnect scenarios. Landed from contributor PR #29508 by @cgdusek. Thanks @cgdusek. diff --git a/src/gateway/server-methods/agents-mutate.test.ts b/src/gateway/server-methods/agents-mutate.test.ts index a12db195c3a..04d2a785188 100644 --- a/src/gateway/server-methods/agents-mutate.test.ts +++ b/src/gateway/server-methods/agents-mutate.test.ts @@ -217,6 +217,7 @@ beforeEach(() => { ({ stat: async () => makeFileStat(), readFile: async () => Buffer.from(""), + truncate: async () => {}, writeFile: async () => {}, close: async () => {}, }) as unknown, @@ -621,6 +622,7 @@ describe("agents.files.get/set symlink safety", () => { ({ stat: async () => targetStat, readFile: async () => Buffer.from("inside\n"), + truncate: async () => {}, writeFile: async () => {}, close: async () => {}, }) as unknown, diff --git a/src/infra/fs-safe.test.ts b/src/infra/fs-safe.test.ts index 5c676fb76b8..eaf1edbdd6b 100644 --- a/src/infra/fs-safe.test.ts +++ b/src/infra/fs-safe.test.ts @@ -1,6 +1,6 @@ import fs from "node:fs/promises"; import path from "node:path"; -import { afterEach, describe, expect, it } from "vitest"; +import { afterEach, describe, expect, it, vi } from "vitest"; import { createTrackedTempDirs } from "../test-utils/tracked-temp-dirs.js"; import { createRootScopedReadFile, @@ -197,6 +197,87 @@ describe("fs-safe", () => { } }); + it.runIf(process.platform !== "win32")( + "does not truncate out-of-root file when symlink retarget races write open", + async () => { + const root = await tempDirs.make("openclaw-fs-safe-root-"); + const inside = path.join(root, "inside"); + const outside = await tempDirs.make("openclaw-fs-safe-outside-"); + await fs.mkdir(inside, { recursive: true }); + const insideTarget = path.join(inside, "target.txt"); + const outsideTarget = path.join(outside, "target.txt"); + await fs.writeFile(insideTarget, "inside"); + await fs.writeFile(outsideTarget, "X".repeat(4096)); + const slot = path.join(root, "slot"); + await fs.symlink(inside, slot); + + const realRealpath = fs.realpath.bind(fs); + let flipped = false; + const realpathSpy = vi.spyOn(fs, "realpath").mockImplementation(async (...args) => { + const [filePath] = args; + if (!flipped && String(filePath).endsWith(path.join("slot", "target.txt"))) { + flipped = true; + await fs.rm(slot, { recursive: true, force: true }); + await fs.symlink(outside, slot); + } + return await realRealpath(...args); + }); + try { + await expect( + writeFileWithinRoot({ + rootDir: root, + relativePath: path.join("slot", "target.txt"), + data: "new-content", + mkdir: false, + }), + ).rejects.toMatchObject({ code: "outside-workspace" }); + } finally { + realpathSpy.mockRestore(); + } + + await expect(fs.readFile(outsideTarget, "utf8")).resolves.toBe("X".repeat(4096)); + }, + ); + + it.runIf(process.platform !== "win32")( + "cleans up created out-of-root file when symlink retarget races create path", + async () => { + const root = await tempDirs.make("openclaw-fs-safe-root-"); + const inside = path.join(root, "inside"); + const outside = await tempDirs.make("openclaw-fs-safe-outside-"); + await fs.mkdir(inside, { recursive: true }); + const outsideTarget = path.join(outside, "target.txt"); + const slot = path.join(root, "slot"); + await fs.symlink(inside, slot); + + const realOpen = fs.open.bind(fs); + let flipped = false; + const openSpy = vi.spyOn(fs, "open").mockImplementation(async (...args) => { + const [filePath] = args; + if (!flipped && String(filePath).endsWith(path.join("slot", "target.txt"))) { + flipped = true; + await fs.rm(slot, { recursive: true, force: true }); + await fs.symlink(outside, slot); + } + return await realOpen(...args); + }); + try { + await expect( + writeFileWithinRoot({ + rootDir: root, + relativePath: path.join("slot", "target.txt"), + data: "new-content", + mkdir: false, + }), + ).rejects.toMatchObject({ code: "outside-workspace" }); + } finally { + openSpy.mockRestore(); + } + + await expect(fs.stat(outsideTarget)).rejects.toMatchObject({ code: "ENOENT" }); + }, + ); + it("returns not-found for missing files", async () => { const dir = await tempDirs.make("openclaw-fs-safe-"); const missing = path.join(dir, "missing.txt"); diff --git a/src/infra/fs-safe.ts b/src/infra/fs-safe.ts index f3ae01b01b6..b5e9a375b30 100644 --- a/src/infra/fs-safe.ts +++ b/src/infra/fs-safe.ts @@ -42,10 +42,12 @@ export type SafeLocalReadResult = { const SUPPORTS_NOFOLLOW = process.platform !== "win32" && "O_NOFOLLOW" in fsConstants; const OPEN_READ_FLAGS = fsConstants.O_RDONLY | (SUPPORTS_NOFOLLOW ? fsConstants.O_NOFOLLOW : 0); -const OPEN_WRITE_FLAGS = +const OPEN_WRITE_EXISTING_FLAGS = + fsConstants.O_WRONLY | (SUPPORTS_NOFOLLOW ? fsConstants.O_NOFOLLOW : 0); +const OPEN_WRITE_CREATE_FLAGS = fsConstants.O_WRONLY | fsConstants.O_CREAT | - fsConstants.O_TRUNC | + fsConstants.O_EXCL | (SUPPORTS_NOFOLLOW ? fsConstants.O_NOFOLLOW : 0); const ensureTrailingSep = (value: string) => (value.endsWith(path.sep) ? value : value + path.sep); @@ -301,8 +303,17 @@ export async function writeFileWithinRoot(params: { } let handle: FileHandle; + let createdForWrite = false; try { - handle = await fs.open(ioPath, OPEN_WRITE_FLAGS, 0o600); + try { + handle = await fs.open(ioPath, OPEN_WRITE_EXISTING_FLAGS, 0o600); + } catch (err) { + if (!isNotFoundPathError(err)) { + throw err; + } + handle = await fs.open(ioPath, OPEN_WRITE_CREATE_FLAGS, 0o600); + createdForWrite = true; + } } catch (err) { if (isNotFoundPathError(err)) { throw new SafeOpenError("not-found", "file not found"); @@ -313,6 +324,7 @@ export async function writeFileWithinRoot(params: { throw err; } + let openedRealPath: string | null = null; try { const [stat, lstat] = await Promise.all([handle.stat(), fs.lstat(ioPath)]); if (lstat.isSymbolicLink() || !stat.isFile()) { @@ -326,6 +338,7 @@ export async function writeFileWithinRoot(params: { } const realPath = await fs.realpath(ioPath); + openedRealPath = realPath; const realStat = await fs.stat(realPath); if (!sameFileIdentity(stat, realStat)) { throw new SafeOpenError("path-mismatch", "path mismatch"); @@ -337,11 +350,21 @@ export async function writeFileWithinRoot(params: { throw new SafeOpenError("outside-workspace", "file is outside workspace root"); } + // Truncate only after boundary and identity checks complete. This avoids + // irreversible side effects if a symlink target changes before validation. + if (!createdForWrite) { + await handle.truncate(0); + } if (typeof params.data === "string") { await handle.writeFile(params.data, params.encoding ?? "utf8"); } else { await handle.writeFile(params.data); } + } catch (err) { + if (createdForWrite && err instanceof SafeOpenError && openedRealPath) { + await fs.rm(openedRealPath, { force: true }).catch(() => {}); + } + throw err; } finally { await handle.close().catch(() => {}); }