fix: harden file store writes

This commit is contained in:
Peter Steinberger 2026-05-06 04:12:15 +01:00
parent 409874b507
commit a6149c2cee
No known key found for this signature in database
6 changed files with 437 additions and 77 deletions

View File

@ -6,6 +6,7 @@
- Preserve the caller's destination path spelling during staged archive merges so symlink-rebind checks catch alias races on macOS.
- Reject archive writes that gain a hardlink alias during post-write verification and clean up the destination file.
- Reject `fileStore()` and `fileStoreSync()` writes through symlinked parent directories so store commits cannot escape the configured root.
## 0.1.0 - 2026-05-06

View File

@ -99,7 +99,7 @@
"prepack": "node scripts/prepack-build.mjs",
"test": "vitest run",
"test:coverage": "vitest run --coverage",
"test:security": "vitest run test/fs-safe.test.ts test/openclaw-read-bypass-parity.test.ts test/openclaw-write-bypass-parity.test.ts test/additional-bypass-parity.test.ts",
"test:security": "vitest run test/fs-safe.test.ts test/openclaw-read-bypass-parity.test.ts test/openclaw-write-bypass-parity.test.ts test/additional-bypass-parity.test.ts test/adversarial-boundary-payloads.test.ts",
"check": "pnpm lint:file-size && pnpm build && pnpm test",
"docs:site": "node scripts/build-docs-site.mjs"
},

197
src/file-store-boundary.ts Normal file
View File

@ -0,0 +1,197 @@
import syncFs from "node:fs";
import fs from "node:fs/promises";
import path from "node:path";
import { Transform, type Readable } from "node:stream";
import { pipeline } from "node:stream/promises";
import { FsSafeError } from "./errors.js";
import { isPathInside } from "./path.js";
import { resolveOpenedFileRealPathForHandle, root, type Root } from "./root.js";
import { resolveSecureTempRoot } from "./secure-temp-dir.js";
export type SyncParentGuard = {
dir: string;
realPath: string;
};
function parentRelativePath(relativePath: string): string {
const parent = path.posix.dirname(relativePath);
return parent === "." ? "" : parent;
}
export async function ensureParentInRoot(
scopedRoot: Root,
relativePath: string,
mode: number,
): Promise<void> {
const parent = parentRelativePath(relativePath);
if (!parent) {
return;
}
await scopedRoot.mkdir(parent);
await chmodDirectoryInRootBestEffort(scopedRoot, parent, mode).catch(() => undefined);
}
export async function openWritableStoreRoot(params: {
rootDir: string;
dirMode: number;
maxBytes?: number;
}): Promise<Root> {
await fs.mkdir(params.rootDir, { recursive: true, mode: params.dirMode });
await fs.chmod(params.rootDir, params.dirMode).catch(() => undefined);
return await root(params.rootDir, { hardlinks: "reject", maxBytes: params.maxBytes });
}
async function chmodDirectoryInRootBestEffort(
scopedRoot: Root,
relativePath: string,
mode: number,
): Promise<void> {
const dirPath = await scopedRoot.resolve(relativePath);
const directoryFlag = "O_DIRECTORY" in syncFs.constants ? syncFs.constants.O_DIRECTORY : 0;
const noFollowFlag =
process.platform !== "win32" && "O_NOFOLLOW" in syncFs.constants
? syncFs.constants.O_NOFOLLOW
: 0;
const handle = await fs.open(dirPath, syncFs.constants.O_RDONLY | directoryFlag | noFollowFlag);
try {
const stat = await handle.stat();
if (!stat.isDirectory()) {
return;
}
const realPath = await resolveOpenedFileRealPathForHandle(handle, dirPath);
if (!isPathInside(scopedRoot.rootWithSep, realPath)) {
throw new FsSafeError("outside-workspace", "directory is outside store root");
}
await handle.chmod(mode).catch(() => undefined);
} finally {
await handle.close().catch(() => undefined);
}
}
function createMaxBytesTransform(maxBytes: number | undefined): Transform | undefined {
if (maxBytes === undefined) {
return undefined;
}
let total = 0;
return new Transform({
transform(chunk: Buffer | string, _encoding, callback) {
const buffer = Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk);
total += buffer.byteLength;
if (total > maxBytes) {
callback(new FsSafeError("too-large", `file exceeds maximum size of ${maxBytes} bytes`));
return;
}
callback(null, buffer);
},
});
}
export async function writeStreamToTempSource(params: {
stream: Readable;
maxBytes?: number;
mode: number;
}): Promise<{ path: string; cleanup: () => Promise<void> }> {
const tempRoot = resolveSecureTempRoot({
fallbackPrefix: "fs-safe-file-store",
unsafeFallbackLabel: "file store temp dir",
warn: () => undefined,
});
const dir = await fs.mkdtemp(path.join(tempRoot, "fs-safe-file-store-"));
const filePath = path.join(dir, "payload");
let handle: Awaited<ReturnType<typeof fs.open>> | null = null;
let handleClosedByStream = false;
try {
handle = await fs.open(filePath, "wx", params.mode);
const writable = handle.createWriteStream();
writable.once("close", () => {
handleClosedByStream = true;
});
const limiter = createMaxBytesTransform(params.maxBytes);
if (limiter) {
await pipeline(params.stream, limiter, writable);
} else {
await pipeline(params.stream, writable);
}
if (!handleClosedByStream) {
await handle.close().catch(() => undefined);
}
await fs.chmod(filePath, params.mode).catch(() => undefined);
return {
path: filePath,
cleanup: async () => {
await fs.rm(dir, { recursive: true, force: true }).catch(() => undefined);
},
};
} catch (err) {
if (handle && !handleClosedByStream) {
await handle.close().catch(() => undefined);
}
await fs.rm(dir, { recursive: true, force: true }).catch(() => undefined);
throw err;
}
}
export function assertSyncDirectoryGuard(guard: SyncParentGuard): void {
const stat = syncFs.lstatSync(guard.dir);
if (stat.isSymbolicLink() || !stat.isDirectory()) {
throw new FsSafeError("not-file", `store directory component must be a directory: ${guard.dir}`);
}
const realPath = syncFs.realpathSync(guard.dir);
if (realPath !== guard.realPath) {
throw new FsSafeError("path-mismatch", "store directory changed during write");
}
}
function chmodDirectorySyncBestEffort(dir: string, mode: number): void {
try {
syncFs.chmodSync(dir, mode);
} catch {
// Best-effort on platforms that do not enforce POSIX modes.
}
}
export function ensureParentSync(params: {
rootDir: string;
filePath: string;
mode: number;
}): SyncParentGuard {
const rootDir = path.resolve(params.rootDir);
const dir = path.dirname(path.resolve(params.filePath));
const relative = path.relative(rootDir, dir);
if (relative.startsWith("..") || path.isAbsolute(relative)) {
throw new FsSafeError("outside-workspace", "file path escapes store root");
}
syncFs.mkdirSync(rootDir, { recursive: true, mode: params.mode });
const rootStat = syncFs.lstatSync(rootDir);
if (rootStat.isSymbolicLink() || !rootStat.isDirectory()) {
throw new FsSafeError("not-file", `store root must be a directory: ${rootDir}`);
}
const rootReal = syncFs.realpathSync(rootDir);
chmodDirectorySyncBestEffort(rootDir, params.mode);
let current = rootDir;
for (const segment of path.relative(rootDir, dir).split(path.sep).filter(Boolean)) {
current = path.join(current, segment);
try {
const stat = syncFs.lstatSync(current);
if (stat.isSymbolicLink() || !stat.isDirectory()) {
throw new FsSafeError("not-file", `store directory component must be a directory: ${current}`);
}
} catch (error) {
if ((error as NodeJS.ErrnoException).code !== "ENOENT") {
throw error;
}
syncFs.mkdirSync(current, { mode: params.mode });
}
const currentReal = syncFs.realpathSync(current);
if (!isPathInside(rootReal, currentReal)) {
throw new FsSafeError("outside-workspace", "store directory escapes root");
}
chmodDirectorySyncBestEffort(current, params.mode);
}
const guard = { dir, realPath: syncFs.realpathSync(dir) };
assertSyncDirectoryGuard(guard);
return guard;
}

View File

@ -3,13 +3,19 @@ import syncFs from "node:fs";
import fs from "node:fs/promises";
import path from "node:path";
import type { Readable } from "node:stream";
import { pipeline } from "node:stream/promises";
import { FsSafeError } from "./errors.js";
import {
assertSyncDirectoryGuard,
ensureParentInRoot,
ensureParentSync,
openWritableStoreRoot,
type SyncParentGuard,
writeStreamToTempSource,
} from "./file-store-boundary.js";
import { createJsonStore, type JsonFileStoreOptions, type JsonStore } from "./json-document-store.js";
import { isPathInside, resolveSafeRelativePath } from "./path.js";
import { root, type OpenResult, type ReadResult, type Root, type RootReadOptions } from "./root.js";
import { writeSecretFileAtomic } from "./secret-file.js";
import { writeSiblingTempFile } from "./sibling-temp.js";
export type FileStoreOptions = {
rootDir: string;
@ -115,12 +121,6 @@ function assertStoreFilePath(rootDir: string, filePath: string): void {
}
}
async function ensureParent(filePath: string, mode: number): Promise<void> {
const dir = path.dirname(filePath);
await fs.mkdir(dir, { recursive: true, mode });
await fs.chmod(dir, mode).catch(() => undefined);
}
function assertMaxBytes(size: number, maxBytes?: number): void {
if (maxBytes !== undefined && size > maxBytes) {
throw new FsSafeError("too-large", `file exceeds maximum size of ${maxBytes} bytes`);
@ -143,26 +143,26 @@ async function copyIntoRoot(params: {
mode?: number;
tempPrefix?: string;
}): Promise<string> {
const destination = resolveStorePath(params.rootDir, params.relativePath);
const relativePath = assertRelativePath(params.relativePath);
const destination = resolveStorePath(params.rootDir, relativePath);
const sourceStat = await fs.lstat(params.sourcePath);
if (sourceStat.isSymbolicLink() || !sourceStat.isFile()) {
throw new FsSafeError("not-file", "source path is not a file");
}
assertMaxBytes(sourceStat.size, params.maxBytes);
await ensureParent(destination, params.dirMode ?? 0o700);
const result = await writeSiblingTempFile({
dir: path.dirname(destination),
dirMode: params.dirMode ?? 0o700,
mode: params.mode ?? 0o600,
tempPrefix: params.tempPrefix ?? `.${path.basename(destination)}`,
writeTemp: async (tempPath) => {
await fs.copyFile(params.sourcePath, tempPath);
},
resolveFinalPath: () => destination,
syncTempFile: true,
syncParentDir: true,
const dirMode = params.dirMode ?? 0o700;
const scopedRoot = await openWritableStoreRoot({
rootDir: params.rootDir,
dirMode,
maxBytes: params.maxBytes,
});
return result.filePath;
await ensureParentInRoot(scopedRoot, relativePath, dirMode);
await scopedRoot.copyIn(relativePath, params.sourcePath, {
maxBytes: params.maxBytes,
mkdir: false,
mode: params.mode ?? 0o600,
});
return destination;
}
export function fileStore(options: FileStoreOptions): FileStore {
@ -181,7 +181,8 @@ export function fileStore(options: FileStoreOptions): FileStore {
data: string | Uint8Array,
writeOptions?: FileStoreWriteOptions,
): Promise<string> {
const destination = resolveStorePath(rootDir, relativePath);
const safeRelativePath = assertRelativePath(relativePath);
const destination = resolveStorePath(rootDir, safeRelativePath);
const content = Buffer.isBuffer(data) ? data : Buffer.from(data);
assertMaxBytes(content.byteLength, writeOptions?.maxBytes ?? maxBytes);
if (privateMode) {
@ -194,20 +195,18 @@ export function fileStore(options: FileStoreOptions): FileStore {
});
return destination;
}
await ensureParent(destination, writeOptions?.dirMode ?? dirMode);
const result = await writeSiblingTempFile({
dir: path.dirname(destination),
dirMode: writeOptions?.dirMode ?? dirMode,
mode: writeOptions?.mode ?? mode,
tempPrefix: writeOptions?.tempPrefix ?? `.${path.basename(destination)}`,
writeTemp: async (tempPath) => {
await fs.writeFile(tempPath, content);
},
resolveFinalPath: () => destination,
syncTempFile: true,
syncParentDir: true,
const writeDirMode = writeOptions?.dirMode ?? dirMode;
const scopedRoot = await openWritableStoreRoot({
rootDir,
dirMode: writeDirMode,
maxBytes: writeOptions?.maxBytes ?? maxBytes,
});
return result.filePath;
await ensureParentInRoot(scopedRoot, safeRelativePath, writeDirMode);
await scopedRoot.write(safeRelativePath, content, {
mkdir: false,
mode: writeOptions?.mode ?? mode,
});
return destination;
}
return {
@ -216,7 +215,8 @@ export function fileStore(options: FileStoreOptions): FileStore {
root: openRoot,
write,
writeStream: async (relativePath, stream, writeOptions) => {
const destination = resolveStorePath(rootDir, relativePath);
const safeRelativePath = assertRelativePath(relativePath);
const destination = resolveStorePath(rootDir, safeRelativePath);
const limit = writeOptions?.maxBytes ?? maxBytes;
if (privateMode) {
const chunks: Buffer[] = [];
@ -237,35 +237,25 @@ export function fileStore(options: FileStoreOptions): FileStore {
});
return destination;
}
await ensureParent(destination, writeOptions?.dirMode ?? dirMode);
let total = 0;
const result = await writeSiblingTempFile({
dir: path.dirname(destination),
dirMode: writeOptions?.dirMode ?? dirMode,
const staged = await writeStreamToTempSource({
stream,
maxBytes: limit,
mode: writeOptions?.mode ?? mode,
tempPrefix: writeOptions?.tempPrefix ?? `.${path.basename(destination)}`,
writeTemp: async (tempPath) => {
const writable = await fs.open(tempPath, "w", writeOptions?.mode ?? mode);
try {
const out = writable.createWriteStream();
stream.on("data", (chunk: Buffer | string) => {
total += Buffer.byteLength(chunk);
if (limit !== undefined && total > limit) {
stream.destroy(
new FsSafeError("too-large", `file exceeds maximum size of ${limit} bytes`),
);
}
});
await pipeline(stream, out);
} finally {
await writable.close().catch(() => undefined);
}
},
resolveFinalPath: () => destination,
syncTempFile: true,
syncParentDir: true,
});
return result.filePath;
try {
await copyIntoRoot({
rootDir,
relativePath: safeRelativePath,
sourcePath: staged.path,
maxBytes: limit,
mode: writeOptions?.mode ?? mode,
tempPrefix: writeOptions?.tempPrefix,
dirMode: writeOptions?.dirMode ?? dirMode,
});
} finally {
await staged.cleanup();
}
return destination;
},
copyIn: async (relativePath, sourcePath, writeOptions) =>
privateMode
@ -405,16 +395,6 @@ export function fileStore(options: FileStoreOptions): FileStore {
};
}
function ensureParentSync(filePath: string, mode: number): void {
const dir = path.dirname(filePath);
syncFs.mkdirSync(dir, { recursive: true, mode });
try {
syncFs.chmodSync(dir, mode);
} catch {
// Best-effort on platforms that do not enforce POSIX modes.
}
}
function ensurePrivateDirectorySync(rootDir: string, targetDir: string, mode: number): void {
const root = path.resolve(rootDir);
const target = path.resolve(targetDir);
@ -469,6 +449,7 @@ function writeFileSyncAtomic(params: {
}): string {
const filePath = path.resolve(params.filePath);
assertStoreFilePath(params.rootDir, filePath);
let parentGuard: SyncParentGuard | undefined;
if (params.privateMode) {
ensurePrivateDirectorySync(params.rootDir, path.dirname(filePath), params.dirMode);
try {
@ -482,11 +463,21 @@ function writeFileSyncAtomic(params: {
}
}
} else {
ensureParentSync(filePath, params.dirMode);
parentGuard = ensureParentSync({
rootDir: params.rootDir,
filePath,
mode: params.dirMode,
});
}
const tempPath = path.join(path.dirname(filePath), `.fs-safe-${process.pid}-${randomUUID()}.tmp`);
const tempPath = path.join(
parentGuard?.dir ?? path.dirname(filePath),
`.fs-safe-${process.pid}-${randomUUID()}.tmp`,
);
let tempExists = false;
try {
if (parentGuard) {
assertSyncDirectoryGuard(parentGuard);
}
syncFs.writeFileSync(tempPath, params.content, { flag: "wx", mode: params.mode });
tempExists = true;
try {
@ -494,8 +485,14 @@ function writeFileSyncAtomic(params: {
} catch {
// Best-effort on platforms that do not enforce POSIX modes.
}
if (parentGuard) {
assertSyncDirectoryGuard(parentGuard);
}
syncFs.renameSync(tempPath, filePath);
tempExists = false;
if (parentGuard) {
assertSyncDirectoryGuard(parentGuard);
}
try {
syncFs.chmodSync(filePath, params.mode);
} catch {

View File

@ -0,0 +1,144 @@
import fsp from "node:fs/promises";
import os from "node:os";
import path from "node:path";
import { afterEach, describe, expect, it } from "vitest";
import { root as openRoot } from "../src/index.js";
type TempLayout = {
outside: string;
outsideFile: string;
root: string;
};
const tempDirs: string[] = [];
const DOT_SEGMENTS = ["..", "...", ". .", ".. ", " ..", "%2e%2e", "%252e%252e"] as const;
const SEPARATORS = ["/", "//", "\\", "\\\\", "%2f", "%5c"] as const;
const TARGETS = ["secret.txt", "etc/passwd", "Windows/win.ini", "CON", "NUL", "aux.txt"] as const;
const CONTROL_PAYLOADS = [
"..\u0000/secret.txt",
"..\u0001/secret.txt",
"..\u001f/secret.txt",
"safe\u0000name.txt",
"safe\u202ename.txt",
"safe\ufeffname.txt",
] as const;
async function makeTempLayout(prefix: string): Promise<TempLayout> {
const root = await fsp.mkdtemp(path.join(os.tmpdir(), `${prefix}-root-`));
const outside = await fsp.mkdtemp(path.join(os.tmpdir(), `${prefix}-outside-`));
tempDirs.push(root, outside);
const outsideFile = path.join(outside, "secret.txt");
await fsp.writeFile(outsideFile, "outside secret");
return { outside, outsideFile, root };
}
function buildPayloadCorpus(): string[] {
const payloads = new Set<string>();
for (const dot of DOT_SEGMENTS) {
for (const sep of SEPARATORS) {
for (const target of TARGETS) {
payloads.add(`${dot}${sep}${target}`);
payloads.add(`nested${sep}${dot}${sep}${dot}${sep}${target}`);
}
}
}
payloads.add("/etc/passwd");
payloads.add("//server/share/secret.txt");
payloads.add("C:/Windows/win.ini");
payloads.add("C:\\Windows\\win.ini");
for (const payload of CONTROL_PAYLOADS) {
payloads.add(payload);
}
return [...payloads];
}
async function expectOutsideUntouched(layout: TempLayout): Promise<void> {
await expect(fsp.readFile(layout.outsideFile, "utf8")).resolves.toBe("outside secret");
}
async function closeIfOpened(value: unknown): Promise<void> {
if (typeof value !== "object" || value === null) {
return;
}
if (Symbol.asyncDispose in value) {
await (value as { [Symbol.asyncDispose](): Promise<void> })[Symbol.asyncDispose]();
return;
}
if ("handle" in value) {
await (value as { handle: { close(): Promise<void> } }).handle.close();
return;
}
if ("close" in value) {
await (value as { close(): Promise<void> }).close();
}
}
async function attemptAll(rootDir: Awaited<ReturnType<typeof openRoot>>, payload: string): Promise<void> {
const opened = await rootDir.open(payload).catch((error: unknown) => error);
await closeIfOpened(opened);
const writable = await rootDir.openWritable(payload).catch((error: unknown) => error);
await closeIfOpened(writable);
await Promise.allSettled([
rootDir.read(payload),
rootDir.stat(payload),
rootDir.write(payload, "payload"),
rootDir.create(payload, "payload"),
rootDir.append(payload, "payload"),
]);
}
afterEach(async () => {
await Promise.all(tempDirs.splice(0).map((dir) => fsp.rm(dir, { force: true, recursive: true })));
});
describe("adversarial boundary payloads", () => {
it("never reads, writes, or deletes outside the root for a generated traversal corpus", async () => {
const layout = await makeTempLayout("fs-safe-adversarial-corpus");
await fsp.mkdir(path.join(layout.root, "nested"), { recursive: true });
await fsp.mkdir(path.join(layout.root, "safe"), { recursive: true });
const safeRoot = await openRoot(layout.root);
const payloads = buildPayloadCorpus().slice(0, 96);
for (const payload of payloads) {
await attemptAll(safeRoot, payload);
await expectOutsideUntouched(layout);
}
}, 15_000);
it("rejects chained symlink parent escapes across read and write surfaces", async () => {
const layout = await makeTempLayout("fs-safe-symlink-chain");
await fsp.mkdir(path.join(layout.root, "a"), { recursive: true });
await fsp.symlink(path.join(layout.root, "a"), path.join(layout.root, "link-a"), "dir");
await fsp.symlink(layout.outside, path.join(layout.root, "a", "link-out"), "dir");
const safeRoot = await openRoot(layout.root);
for (const payload of ["link-a/link-out/secret.txt", "a/link-out/secret.txt"]) {
await expect(safeRoot.read(payload), `read ${payload}`).rejects.toBeTruthy();
await expect(safeRoot.write(payload, "pwned"), `write ${payload}`).rejects.toBeTruthy();
await expect(safeRoot.remove(payload), `remove ${payload}`).rejects.toBeTruthy();
}
await expectOutsideUntouched(layout);
});
it("does not clobber outside files when copy and move payloads mix source and destination attacks", async () => {
const layout = await makeTempLayout("fs-safe-copy-move-corpus");
const source = path.join(layout.root, "source.txt");
await fsp.writeFile(source, "source");
await fsp.symlink(layout.outsideFile, path.join(layout.root, "outside-link.txt"), "file");
const safeRoot = await openRoot(layout.root);
const payloads = buildPayloadCorpus().slice(0, 48);
for (const payload of payloads) {
await Promise.allSettled([
safeRoot.copyIn(payload, source),
safeRoot.copyIn("copied.txt", layout.outsideFile),
safeRoot.move("source.txt", payload, { overwrite: true }),
safeRoot.move(payload, "moved.txt", { overwrite: true }),
safeRoot.move("outside-link.txt", "moved-link.txt", { overwrite: true }),
]);
await expectOutsideUntouched(layout);
await fsp.writeFile(source, "source");
}
});
});

View File

@ -1,6 +1,8 @@
import fsp from "node:fs/promises";
import path from "node:path";
import { Readable } from "node:stream";
import { afterEach, describe, expect, it } from "vitest";
import { fileStore, fileStoreSync } from "../src/file-store.js";
import { root as openRoot } from "../src/index.js";
import {
ESCAPING_DIRECTORY_PAYLOADS,
@ -91,6 +93,25 @@ describe("OpenClaw write/move/delete bypass parity", () => {
await expectNoOutsideWrite(layout);
});
it.runIf(process.platform !== "win32")("rejects file store symlink parent writes", async () => {
const layout = await makeTempLayout("fs-safe-file-store-symlink-parent");
const source = path.join(layout.root, "source.txt");
await fsp.writeFile(source, "source");
await fsp.symlink(layout.outside, path.join(layout.root, "link"), "dir");
const store = fileStore({ rootDir: layout.root });
const syncStore = fileStoreSync({ rootDir: layout.root });
await expect(store.write("link/write.txt", "pwned")).rejects.toBeTruthy();
await expect(store.writeStream("link/stream.txt", Readable.from(["pwned"]))).rejects
.toBeTruthy();
await expect(store.copyIn("link/copy.txt", source)).rejects.toBeTruthy();
expect(() => syncStore.write("link/sync-write.txt", "pwned")).toThrow();
expect(() => syncStore.writeText("link/sync-text.txt", "pwned")).toThrow();
expect(() => syncStore.writeJson("link/sync-json.json", { pwned: true })).toThrow();
await expectNoOutsideWrite(layout);
await expect(fsp.readdir(layout.outside)).resolves.toEqual(["secret.txt"]);
});
it("rejects final symlink leaf write/append/openWritable/copy targets without clobbering their target", async () => {
const layout = await makeTempLayout("fs-safe-write-symlink-leaf");
await fsp.symlink(layout.outsideFile, path.join(layout.root, "link.txt"), "file");