Harden file store writes against symlink swaps
This commit is contained in:
Sarah Fortune
parent
1cd216921d
commit
14aace8597
@ -1,11 +1,19 @@
|
||||
import { constants as fsConstants } from "node:fs";
|
||||
import fs from "node:fs/promises";
|
||||
import path from "node:path";
|
||||
import type { Readable } from "node:stream";
|
||||
import { Transform, type Readable } from "node:stream";
|
||||
import { pipeline } from "node:stream/promises";
|
||||
import { FsSafeError } from "./errors.js";
|
||||
import { root, type OpenResult, type ReadResult, type Root, type RootReadOptions } from "./root.js";
|
||||
import { writeSiblingTempFile } from "./sibling-temp.js";
|
||||
import { resolveSafeRelativePath } from "./path.js";
|
||||
import {
|
||||
resolveOpenedFileRealPathForHandle,
|
||||
root,
|
||||
type OpenResult,
|
||||
type ReadResult,
|
||||
type Root,
|
||||
type RootReadOptions,
|
||||
} from "./root.js";
|
||||
import { isPathInside, resolveSafeRelativePath } from "./path.js";
|
||||
import { resolveSecureTempRoot } from "./secure-temp-dir.js";
|
||||
|
||||
export type FileStoreOptions = {
|
||||
rootDir: string;
|
||||
@ -63,19 +71,135 @@ function assertRelativePath(relativePath: string): string {
|
||||
return raw.replaceAll("\\", "/");
|
||||
}
|
||||
|
||||
function assertMaxBytes(size: number, maxBytes?: number): void {
|
||||
if (maxBytes !== undefined && size > maxBytes) {
|
||||
throw new FsSafeError("too-large", `file exceeds maximum size of ${maxBytes} bytes`);
|
||||
}
|
||||
}
|
||||
|
||||
function resolveStorePath(rootDir: string, relativePath: string): string {
|
||||
return resolveSafeRelativePath(rootDir, assertRelativePath(relativePath));
|
||||
}
|
||||
|
||||
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 parentRelativePath(relativePath: string): string {
|
||||
const parent = path.posix.dirname(assertRelativePath(relativePath));
|
||||
return parent === "." ? "" : parent;
|
||||
}
|
||||
|
||||
function assertMaxBytes(size: number, maxBytes?: number): void {
|
||||
if (maxBytes !== undefined && size > maxBytes) {
|
||||
throw new FsSafeError("too-large", `file exceeds maximum size of ${maxBytes} bytes`);
|
||||
async function chmodDirectoryInRootBestEffort(
|
||||
scopedRoot: Root,
|
||||
relativePath: string,
|
||||
mode: number,
|
||||
): Promise<void> {
|
||||
if (!relativePath) {
|
||||
return;
|
||||
}
|
||||
const dirPath = await scopedRoot.resolve(relativePath);
|
||||
const directoryFlag = "O_DIRECTORY" in fsConstants ? (fsConstants.O_DIRECTORY as number) : 0;
|
||||
const noFollowFlag =
|
||||
process.platform !== "win32" && "O_NOFOLLOW" in fsConstants
|
||||
? (fsConstants.O_NOFOLLOW as number)
|
||||
: 0;
|
||||
const handle = await fs.open(dirPath, fsConstants.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);
|
||||
}
|
||||
}
|
||||
|
||||
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);
|
||||
}
|
||||
|
||||
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 });
|
||||
}
|
||||
|
||||
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);
|
||||
},
|
||||
});
|
||||
}
|
||||
|
||||
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);
|
||||
handleClosedByStream = true;
|
||||
}
|
||||
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;
|
||||
}
|
||||
}
|
||||
|
||||
@ -88,26 +212,26 @@ export 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 {
|
||||
@ -125,56 +249,46 @@ export function fileStore(options: FileStoreOptions): FileStore {
|
||||
path: (relativePath) => resolveStorePath(rootDir, relativePath),
|
||||
root: openRoot,
|
||||
write: async (relativePath, data, writeOptions) => {
|
||||
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);
|
||||
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;
|
||||
},
|
||||
writeStream: async (relativePath, stream, writeOptions) => {
|
||||
const destination = resolveStorePath(rootDir, relativePath);
|
||||
const safeRelativePath = assertRelativePath(relativePath);
|
||||
const destination = resolveStorePath(rootDir, safeRelativePath);
|
||||
const limit = writeOptions?.maxBytes ?? maxBytes;
|
||||
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) =>
|
||||
await copyIntoRoot({
|
||||
|
||||
@ -1,7 +1,9 @@
|
||||
import { randomUUID } from "node:crypto";
|
||||
import fs from "node:fs/promises";
|
||||
import syncFs from "node:fs";
|
||||
import os from "node:os";
|
||||
import path from "node:path";
|
||||
import { Readable } from "node:stream";
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
|
||||
import {
|
||||
privateStateStore,
|
||||
@ -34,7 +36,7 @@ import { replaceFileAtomic, replaceFileAtomicSync } from "../src/replace-file.js
|
||||
import { movePathWithCopyFallback } from "../src/move-path.js";
|
||||
import { writeSiblingTempFile } from "../src/sibling-temp.js";
|
||||
import { createSidecarLockManager } from "../src/sidecar-lock.js";
|
||||
import { fileStore } from "../src/file-store.js";
|
||||
import { copyIntoRoot, fileStore } from "../src/file-store.js";
|
||||
import { jsonStore } from "../src/json-store.js";
|
||||
import {
|
||||
createIcaclsResetCommand,
|
||||
@ -128,6 +130,9 @@ describe("file store", () => {
|
||||
await store.write("media/a.txt", "hello");
|
||||
await expect(store.readBytes("media/a.txt")).resolves.toEqual(Buffer.from("hello"));
|
||||
|
||||
await store.writeStream("media/stream.txt", Readable.from(["stream"]));
|
||||
await expect(store.readBytes("media/stream.txt")).resolves.toEqual(Buffer.from("stream"));
|
||||
|
||||
const source = path.join(root, "source.bin");
|
||||
await fs.writeFile(source, "source", "utf8");
|
||||
await store.copyIn("media/b.txt", source);
|
||||
@ -145,6 +150,90 @@ describe("file store", () => {
|
||||
code: "too-large",
|
||||
});
|
||||
});
|
||||
|
||||
it.runIf(process.platform !== "win32")("rejects symlink parent writes", async () => {
|
||||
const storeRoot = path.join(root, "store");
|
||||
const outside = path.join(root, "outside");
|
||||
const source = path.join(root, "source.txt");
|
||||
await fs.mkdir(storeRoot);
|
||||
await fs.mkdir(outside);
|
||||
await fs.writeFile(source, "pwned", "utf8");
|
||||
await fs.symlink(outside, path.join(storeRoot, "link"), "dir");
|
||||
const store = fileStore({ rootDir: storeRoot });
|
||||
|
||||
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();
|
||||
await expect(
|
||||
copyIntoRoot({
|
||||
rootDir: storeRoot,
|
||||
relativePath: "link/advanced-copy.txt",
|
||||
sourcePath: source,
|
||||
}),
|
||||
).rejects.toBeTruthy();
|
||||
await expect(fs.readdir(outside)).resolves.toEqual([]);
|
||||
});
|
||||
|
||||
it.runIf(process.platform !== "win32")(
|
||||
"rejects symlink parent swaps during a single write operation",
|
||||
async () => {
|
||||
async function expectSwapBlocked(
|
||||
operation: (params: {
|
||||
store: ReturnType<typeof fileStore>;
|
||||
storeRoot: string;
|
||||
source: string;
|
||||
}) => Promise<unknown>,
|
||||
): Promise<void> {
|
||||
const storeRoot = path.join(root, `store-${randomUUID()}`);
|
||||
const outside = path.join(root, `outside-${randomUUID()}`);
|
||||
const link = path.join(storeRoot, "link");
|
||||
const source = path.join(root, `source-${randomUUID()}.txt`);
|
||||
await fs.mkdir(link, { recursive: true });
|
||||
await fs.mkdir(outside);
|
||||
await fs.writeFile(source, "pwned", "utf8");
|
||||
const canonicalLink = path.join(await fs.realpath(storeRoot), "link");
|
||||
const store = fileStore({ rootDir: storeRoot });
|
||||
const realOpen = fs.open.bind(fs);
|
||||
let swapped = false;
|
||||
const openSpy = vi.spyOn(fs, "open").mockImplementation(
|
||||
(async (
|
||||
target: Parameters<typeof fs.open>[0],
|
||||
flags?: Parameters<typeof fs.open>[1],
|
||||
mode?: Parameters<typeof fs.open>[2],
|
||||
) => {
|
||||
const targetPath = path.resolve(String(target));
|
||||
if (!swapped && (targetPath === link || targetPath === canonicalLink)) {
|
||||
swapped = true;
|
||||
await fs.rm(link, { recursive: true, force: true });
|
||||
await fs.symlink(outside, link, "dir");
|
||||
}
|
||||
return await realOpen(target, flags, mode);
|
||||
}) as typeof fs.open,
|
||||
);
|
||||
try {
|
||||
await expect(operation({ store, storeRoot, source })).rejects.toBeTruthy();
|
||||
expect(swapped).toBe(true);
|
||||
await expect(fs.readdir(outside)).resolves.toEqual([]);
|
||||
} finally {
|
||||
openSpy.mockRestore();
|
||||
}
|
||||
}
|
||||
|
||||
await expectSwapBlocked(({ store }) => store.write("link/write.txt", "pwned"));
|
||||
await expectSwapBlocked(({ store }) =>
|
||||
store.writeStream("link/stream.txt", Readable.from(["pwned"])),
|
||||
);
|
||||
await expectSwapBlocked(({ store, source }) => store.copyIn("link/copy.txt", source));
|
||||
await expectSwapBlocked(({ storeRoot, source }) =>
|
||||
copyIntoRoot({
|
||||
rootDir: storeRoot,
|
||||
relativePath: "link/advanced-copy.txt",
|
||||
sourcePath: source,
|
||||
}),
|
||||
);
|
||||
},
|
||||
);
|
||||
});
|
||||
|
||||
describe("json store", () => {
|
||||
|
||||
Loading…
Reference in New Issue
Block a user