From c20ee113486119859827a0a6a8ac87de2703e4b3 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Mon, 2 Mar 2026 23:36:19 +0000 Subject: [PATCH] fix: harden fs-safe write boundary checks --- CHANGELOG.md | 1 + src/infra/fs-safe.test.ts | 54 +++++++++++++++++++++++ src/infra/fs-safe.ts | 93 ++++++++++++++++++++++++++++++++++----- 3 files changed, 137 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0279641d77e..259ffadfb4a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -50,6 +50,7 @@ Docs: https://docs.openclaw.ai - Security/Node exec approvals: preserve shell/dispatch-wrapper argv semantics during approval hardening so approved wrapper commands (for example `env sh -c ...`) cannot drift into a different runtime command shape, and add regression coverage for both approval-plan generation and approved runtime execution paths. Thanks @tdjackey for reporting. - Security/ACP sandbox inheritance: enforce fail-closed runtime guardrails for `sessions_spawn` with `runtime="acp"` by rejecting ACP spawns from sandboxed requester sessions and rejecting `sandbox="require"` for ACP runtime, preventing sandbox-boundary bypass via host-side ACP initialization. (#32254) Thanks @tdjackey for reporting, and @dutifulbob for the fix. - Browser/Security output boundary hardening: replace check-then-rename output commits with root-bound fd-verified writes, unify install/skills canonical path-boundary checks, and add regression coverage for symlink-rebind race paths across browser output and shared fs-safe write flows. Thanks @tdjackey for reporting. +- Security/fs-safe write hardening: make `writeFileWithinRoot` use same-directory temp writes plus atomic rename, add post-write inode/hardlink revalidation with security warnings on boundary drift, and avoid truncating existing targets when final rename fails. - Security/Webhook request hardening: enforce auth-before-body parsing for BlueBubbles and Google Chat webhook handlers, add strict pre-auth body/time budgets for webhook auth paths (including LINE signature verification), and add shared in-flight/request guardrails plus regression tests/lint checks to prevent reintroducing unauthenticated slow-body DoS patterns. Thanks @GCXWLP for reporting. - Gateway/Security hardening: tie loopback-origin dev allowance to actual local socket clients (not Host header claims), add explicit warnings/metrics when `gateway.controlUi.dangerouslyAllowHostHeaderOriginFallback` accepts websocket origins, harden safe-regex detection for quantified ambiguous alternation patterns (for example `(a|aa)+`), and bound large regex-evaluation inputs for session-filter and log-redaction paths. - Security/Skills archive extraction: unify tar extraction safety checks across tar.gz and tar.bz2 install flows, enforce tar compressed-size limits, and fail closed if tar.bz2 archives change between preflight and extraction to prevent bypasses of entry-type/size guardrails. Thanks @GCXWLP for reporting. diff --git a/src/infra/fs-safe.test.ts b/src/infra/fs-safe.test.ts index 4ee2da1b210..ff0c4388caa 100644 --- a/src/infra/fs-safe.test.ts +++ b/src/infra/fs-safe.test.ts @@ -246,6 +246,60 @@ describe("fs-safe", () => { await expect(fs.readFile(path.join(root, "nested", "out.txt"), "utf8")).resolves.toBe("hello"); }); + it("does not truncate existing target when atomic rename fails", async () => { + const root = await tempDirs.make("openclaw-fs-safe-root-"); + const targetPath = path.join(root, "nested", "out.txt"); + await fs.mkdir(path.dirname(targetPath), { recursive: true }); + await fs.writeFile(targetPath, "existing-content"); + const renameSpy = vi + .spyOn(fs, "rename") + .mockRejectedValue(Object.assign(new Error("rename blocked"), { code: "EACCES" })); + try { + await expect( + writeFileWithinRoot({ + rootDir: root, + relativePath: "nested/out.txt", + data: "new-content", + }), + ).rejects.toMatchObject({ code: "EACCES" }); + } finally { + renameSpy.mockRestore(); + } + await expect(fs.readFile(targetPath, "utf8")).resolves.toBe("existing-content"); + }); + + it.runIf(process.platform !== "win32")( + "rejects when a hardlink appears after atomic write rename", + async () => { + const root = await tempDirs.make("openclaw-fs-safe-root-"); + const targetPath = path.join(root, "nested", "out.txt"); + const aliasPath = path.join(root, "nested", "alias.txt"); + await fs.mkdir(path.dirname(targetPath), { recursive: true }); + await fs.writeFile(targetPath, "existing-content"); + const realRename = fs.rename.bind(fs); + let linked = false; + const renameSpy = vi.spyOn(fs, "rename").mockImplementation(async (...args) => { + await realRename(...args); + if (!linked) { + linked = true; + await fs.link(String(args[1]), aliasPath); + } + }); + try { + await expect( + writeFileWithinRoot({ + rootDir: root, + relativePath: "nested/out.txt", + data: "new-content", + }), + ).rejects.toMatchObject({ code: "invalid-path" }); + } finally { + renameSpy.mockRestore(); + } + await expect(fs.readFile(aliasPath, "utf8")).resolves.toBe("new-content"); + }, + ); + it("copies a file within root safely", async () => { const root = await tempDirs.make("openclaw-fs-safe-root-"); const sourceDir = await tempDirs.make("openclaw-fs-safe-source-"); diff --git a/src/infra/fs-safe.ts b/src/infra/fs-safe.ts index e653229cc1a..e9940c73e7c 100644 --- a/src/infra/fs-safe.ts +++ b/src/infra/fs-safe.ts @@ -1,3 +1,4 @@ +import { randomUUID } from "node:crypto"; import type { Stats } from "node:fs"; import { constants as fsConstants } from "node:fs"; import type { FileHandle } from "node:fs/promises"; @@ -5,6 +6,7 @@ import fs from "node:fs/promises"; import os from "node:os"; import path from "node:path"; import { pipeline } from "node:stream/promises"; +import { logWarn } from "../logger.js"; import { sameFileIdentity } from "./file-identity.js"; import { expandHomePrefix } from "./home-dir.js"; import { assertNoPathAliasEscape } from "./path-alias-guards.js"; @@ -287,8 +289,58 @@ export type SafeWritableOpenResult = { handle: FileHandle; createdForWrite: boolean; openedRealPath: string; + openedStat: Stats; }; +function emitWriteBoundaryWarning(reason: string) { + logWarn(`security: fs-safe write boundary warning (${reason})`); +} + +function buildAtomicWriteTempPath(targetPath: string): string { + const dir = path.dirname(targetPath); + const base = path.basename(targetPath); + return path.join(dir, `.${base}.${process.pid}.${randomUUID()}.tmp`); +} + +async function writeTempFileForAtomicReplace(params: { + tempPath: string; + data: string | Buffer; + encoding?: BufferEncoding; + mode: number; +}): Promise { + const tempHandle = await fs.open(params.tempPath, OPEN_WRITE_CREATE_FLAGS, params.mode); + try { + if (typeof params.data === "string") { + await tempHandle.writeFile(params.data, params.encoding ?? "utf8"); + } else { + await tempHandle.writeFile(params.data); + } + return await tempHandle.stat(); + } finally { + await tempHandle.close().catch(() => {}); + } +} + +async function verifyAtomicWriteResult(params: { + rootDir: string; + targetPath: string; + expectedStat: Stats; +}): Promise { + const rootReal = await fs.realpath(params.rootDir); + const rootWithSep = ensureTrailingSep(rootReal); + const opened = await openVerifiedLocalFile(params.targetPath, { rejectHardlinks: true }); + try { + if (!sameFileIdentity(opened.stat, params.expectedStat)) { + throw new SafeOpenError("path-mismatch", "path changed during write"); + } + if (!isPathInside(rootWithSep, opened.realPath)) { + throw new SafeOpenError("outside-workspace", "file is outside workspace root"); + } + } finally { + await opened.handle.close().catch(() => {}); + } +} + export async function resolveOpenedFileRealPathForHandle( handle: FileHandle, ioPath: string, @@ -322,6 +374,7 @@ export async function openWritableFileWithinRoot(params: { relativePath: string; mkdir?: boolean; mode?: number; + truncateExisting?: boolean; }): Promise { const { rootReal, rootWithSep, resolved } = await resolvePathWithinRoot(params); try { @@ -416,13 +469,14 @@ export async function openWritableFileWithinRoot(params: { // Truncate only after boundary and identity checks complete. This avoids // irreversible side effects if a symlink target changes before validation. - if (!createdForWrite) { + if (params.truncateExisting !== false && !createdForWrite) { await handle.truncate(0); } return { handle, createdForWrite, openedRealPath: realPath, + openedStat: stat, }; } catch (err) { const cleanupCreatedPath = createdForWrite && err instanceof SafeOpenError; @@ -446,15 +500,36 @@ export async function writeFileWithinRoot(params: { rootDir: params.rootDir, relativePath: params.relativePath, mkdir: params.mkdir, + truncateExisting: false, }); + const destinationPath = target.openedRealPath; + const targetMode = target.openedStat.mode & 0o777; + await target.handle.close().catch(() => {}); + let tempPath: string | null = null; try { - if (typeof params.data === "string") { - await target.handle.writeFile(params.data, params.encoding ?? "utf8"); - } else { - await target.handle.writeFile(params.data); + tempPath = buildAtomicWriteTempPath(destinationPath); + const writtenStat = await writeTempFileForAtomicReplace({ + tempPath, + data: params.data, + encoding: params.encoding, + mode: targetMode || 0o600, + }); + await fs.rename(tempPath, destinationPath); + tempPath = null; + try { + await verifyAtomicWriteResult({ + rootDir: params.rootDir, + targetPath: destinationPath, + expectedStat: writtenStat, + }); + } catch (err) { + emitWriteBoundaryWarning(`post-write verification failed: ${String(err)}`); + throw err; } } finally { - await target.handle.close().catch(() => {}); + if (tempPath) { + await fs.rm(tempPath, { force: true }).catch(() => {}); + } } } @@ -477,11 +552,7 @@ export async function copyFileWithinRoot(params: { ); } - let target: { - handle: FileHandle; - createdForWrite: boolean; - openedRealPath: string; - } | null = null; + let target: SafeWritableOpenResult | null = null; let sourceClosedByStream = false; let targetClosedByStream = false; try {