diff --git a/src/infra/archive.test.ts b/src/infra/archive.test.ts index 16df391049f..ed09f030162 100644 --- a/src/infra/archive.test.ts +++ b/src/infra/archive.test.ts @@ -4,6 +4,7 @@ import path from "node:path"; import JSZip from "jszip"; import * as tar from "tar"; import { afterAll, beforeAll, describe, expect, it } from "vitest"; +import { withRealpathSymlinkRebindRace } from "../test-utils/symlink-rebind-race.js"; import type { ArchiveSecurityError } from "./archive.js"; import { extractArchive, resolveArchiveKind, resolvePackedRootDir } from "./archive.js"; @@ -147,6 +148,41 @@ describe("archive utils", () => { }); }); + it.runIf(process.platform !== "win32")( + "does not clobber out-of-destination file when parent dir is symlink-rebound during zip extract", + async () => { + await withArchiveCase("zip", async ({ workDir, archivePath, extractDir }) => { + const outsideDir = path.join(workDir, "outside"); + await fs.mkdir(outsideDir, { recursive: true }); + const slotDir = path.join(extractDir, "slot"); + await fs.mkdir(slotDir, { recursive: true }); + + const outsideTarget = path.join(outsideDir, "target.txt"); + await fs.writeFile(outsideTarget, "SAFE"); + + const zip = new JSZip(); + zip.file("slot/target.txt", "owned"); + await fs.writeFile(archivePath, await zip.generateAsync({ type: "nodebuffer" })); + + await withRealpathSymlinkRebindRace({ + shouldFlip: (realpathInput) => realpathInput === slotDir, + symlinkPath: slotDir, + symlinkTarget: outsideDir, + timing: "after-realpath", + run: async () => { + await expect( + extractArchive({ archivePath, destDir: extractDir, timeoutMs: 5_000 }), + ).rejects.toMatchObject({ + code: "destination-symlink-traversal", + } satisfies Partial); + }, + }); + + await expect(fs.readFile(outsideTarget, "utf8")).resolves.toBe("SAFE"); + }); + }, + ); + it("rejects tar path traversal (zip slip)", async () => { await withArchiveCase("tar", async ({ workDir, archivePath, extractDir }) => { const insideDir = path.join(workDir, "inside"); diff --git a/src/infra/archive.ts b/src/infra/archive.ts index c64afbb6251..12a6f1c92d4 100644 --- a/src/infra/archive.ts +++ b/src/infra/archive.ts @@ -1,4 +1,5 @@ import { constants as fsConstants } from "node:fs"; +import type { FileHandle } from "node:fs/promises"; import fs from "node:fs/promises"; import path from "node:path"; import { Readable, Transform } from "node:stream"; @@ -10,6 +11,8 @@ import { stripArchivePath, validateArchiveEntryPath, } from "./archive-path.js"; +import { sameFileIdentity } from "./file-identity.js"; +import { resolveOpenedFileRealPathForHandle } from "./fs-safe.js"; import { isNotFoundPathError, isPathInside, isSymlinkOpenError } from "./path-guards.js"; export type ArchiveKind = "tar" | "zip"; @@ -64,11 +67,14 @@ const ERROR_ARCHIVE_EXTRACTED_SIZE_EXCEEDS_LIMIT = "archive extracted size excee const ERROR_ARCHIVE_ENTRY_TRAVERSES_SYMLINK = "archive entry traverses symlink in destination"; const TAR_SUFFIXES = [".tgz", ".tar.gz", ".tar"]; -const OPEN_WRITE_FLAGS = +const SUPPORTS_NOFOLLOW = process.platform !== "win32" && "O_NOFOLLOW" in fsConstants; +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 | - (process.platform !== "win32" && "O_NOFOLLOW" in fsConstants ? fsConstants.O_NOFOLLOW : 0); + fsConstants.O_EXCL | + (SUPPORTS_NOFOLLOW ? fsConstants.O_NOFOLLOW : 0); export function resolveArchiveKind(filePath: string): ArchiveKind | null { const lower = filePath.toLowerCase(); @@ -275,15 +281,100 @@ async function assertResolvedInsideDestination(params: { } } -async function openZipOutputFile(outPath: string, originalPath: string) { +type OpenZipOutputFileResult = { + handle: FileHandle; + createdForWrite: boolean; + openedRealPath: string; +}; + +async function openZipOutputFile(params: { + outPath: string; + originalPath: string; + destinationRealDir: string; +}): Promise { + let ioPath = params.outPath; try { - return await fs.open(outPath, OPEN_WRITE_FLAGS, 0o666); + const resolvedRealPath = await fs.realpath(params.outPath); + if (!isPathInside(params.destinationRealDir, resolvedRealPath)) { + throw symlinkTraversalError(params.originalPath); + } + ioPath = resolvedRealPath; + } catch (err) { + if (err instanceof ArchiveSecurityError) { + throw err; + } + if (!isNotFoundPathError(err)) { + throw err; + } + } + + let handle: FileHandle; + let createdForWrite = false; + try { + try { + handle = await fs.open(ioPath, OPEN_WRITE_EXISTING_FLAGS, 0o666); + } catch (err) { + if (!isNotFoundPathError(err)) { + throw err; + } + handle = await fs.open(ioPath, OPEN_WRITE_CREATE_FLAGS, 0o666); + createdForWrite = true; + } } catch (err) { if (isSymlinkOpenError(err)) { - throw symlinkTraversalError(originalPath); + throw symlinkTraversalError(params.originalPath); } throw err; } + + let openedRealPath: string | null = null; + try { + const stat = await handle.stat(); + if (!stat.isFile()) { + throw symlinkTraversalError(params.originalPath); + } + + try { + const lstat = await fs.lstat(ioPath); + if (lstat.isSymbolicLink() || !lstat.isFile()) { + throw symlinkTraversalError(params.originalPath); + } + if (!sameFileIdentity(stat, lstat)) { + throw symlinkTraversalError(params.originalPath); + } + } catch (err) { + if (!isNotFoundPathError(err)) { + throw err; + } + } + + const realPath = await resolveOpenedFileRealPathForHandle(handle, ioPath); + openedRealPath = realPath; + const realStat = await fs.stat(realPath); + if (!sameFileIdentity(stat, realStat)) { + throw symlinkTraversalError(params.originalPath); + } + if (!isPathInside(params.destinationRealDir, realPath)) { + throw symlinkTraversalError(params.originalPath); + } + + // Truncate only after identity + boundary checks complete. + if (!createdForWrite) { + await handle.truncate(0); + } + + return { + handle, + createdForWrite, + openedRealPath: realPath, + }; + } catch (err) { + if (createdForWrite && openedRealPath) { + await fs.rm(openedRealPath, { force: true }).catch(() => undefined); + } + await handle.close().catch(() => undefined); + throw err; + } } async function cleanupPartialRegularFile(filePath: string): Promise { @@ -377,12 +468,21 @@ async function prepareZipOutputPath(params: { async function writeZipFileEntry(params: { entry: ZipEntry; outPath: string; + destinationRealDir: string; budget: ZipExtractBudget; }): Promise { - const handle = await openZipOutputFile(params.outPath, params.entry.name); + const opened = await openZipOutputFile({ + outPath: params.outPath, + originalPath: params.entry.name, + destinationRealDir: params.destinationRealDir, + }); params.budget.startEntry(); const readable = await readZipEntryStream(params.entry); - const writable = handle.createWriteStream(); + const writable = opened.handle.createWriteStream(); + let handleClosedByStream = false; + writable.once("close", () => { + handleClosedByStream = true; + }); try { await pipeline( @@ -391,15 +491,23 @@ async function writeZipFileEntry(params: { writable, ); } catch (err) { - await cleanupPartialRegularFile(params.outPath).catch(() => undefined); + if (opened.createdForWrite) { + await fs.rm(opened.openedRealPath, { force: true }).catch(() => undefined); + } else { + await cleanupPartialRegularFile(opened.openedRealPath).catch(() => undefined); + } throw err; + } finally { + if (!handleClosedByStream) { + await opened.handle.close().catch(() => undefined); + } } // Best-effort permission restore for zip entries created on unix. if (typeof params.entry.unixPermissions === "number") { const mode = params.entry.unixPermissions & 0o777; if (mode !== 0) { - await fs.chmod(params.outPath, mode).catch(() => undefined); + await fs.chmod(opened.openedRealPath, mode).catch(() => undefined); } } } @@ -451,6 +559,7 @@ async function extractZip(params: { await writeZipFileEntry({ entry, outPath: output.outPath, + destinationRealDir, budget, }); } diff --git a/src/infra/fs-safe.test.ts b/src/infra/fs-safe.test.ts index 3e3d8cc5fc2..18f6986ff7f 100644 --- a/src/infra/fs-safe.test.ts +++ b/src/infra/fs-safe.test.ts @@ -1,6 +1,7 @@ import fs from "node:fs/promises"; import path from "node:path"; import { afterEach, describe, expect, it, vi } from "vitest"; +import { withRealpathSymlinkRebindRace } from "../test-utils/symlink-rebind-race.js"; import { createTrackedTempDirs } from "../test-utils/tracked-temp-dirs.js"; import { copyFileWithinRoot, @@ -282,29 +283,22 @@ describe("fs-safe", () => { 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); + await withRealpathSymlinkRebindRace({ + shouldFlip: (realpathInput) => realpathInput.endsWith(path.join("slot", "target.txt")), + symlinkPath: slot, + symlinkTarget: outside, + timing: "before-realpath", + run: async () => { + await expect( + writeFileWithinRoot({ + rootDir: root, + relativePath: path.join("slot", "target.txt"), + data: "new-content", + mkdir: false, + }), + ).rejects.toMatchObject({ code: "outside-workspace" }); + }, }); - 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)); }, @@ -325,29 +319,22 @@ describe("fs-safe", () => { 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); + await withRealpathSymlinkRebindRace({ + shouldFlip: (realpathInput) => realpathInput.endsWith(path.join("slot", "target.txt")), + symlinkPath: slot, + symlinkTarget: outside, + timing: "before-realpath", + run: async () => { + await expect( + writeFileFromPathWithinRoot({ + rootDir: root, + relativePath: path.join("slot", "target.txt"), + sourcePath, + mkdir: false, + }), + ).rejects.toMatchObject({ code: "outside-workspace" }); + }, }); - try { - await expect( - writeFileFromPathWithinRoot({ - rootDir: root, - relativePath: path.join("slot", "target.txt"), - sourcePath, - mkdir: false, - }), - ).rejects.toMatchObject({ code: "outside-workspace" }); - } finally { - realpathSpy.mockRestore(); - } await expect(fs.readFile(outsideTarget, "utf8")).resolves.toBe("X".repeat(4096)); }, diff --git a/src/infra/fs-safe.ts b/src/infra/fs-safe.ts index 95d20e1b7fe..ec1bbf131f5 100644 --- a/src/infra/fs-safe.ts +++ b/src/infra/fs-safe.ts @@ -283,15 +283,45 @@ async function readOpenedFileSafely(params: { }; } -async function openWritableFileWithinRoot(params: { - rootDir: string; - relativePath: string; - mkdir?: boolean; -}): Promise<{ +export type SafeWritableOpenResult = { handle: FileHandle; createdForWrite: boolean; openedRealPath: string; -}> { +}; + +export async function resolveOpenedFileRealPathForHandle( + handle: FileHandle, + ioPath: string, +): Promise { + try { + return await fs.realpath(ioPath); + } catch (err) { + if (!isNotFoundPathError(err)) { + throw err; + } + } + + const fdCandidates = + process.platform === "linux" + ? [`/proc/self/fd/${handle.fd}`, `/dev/fd/${handle.fd}`] + : process.platform === "win32" + ? [] + : [`/dev/fd/${handle.fd}`]; + for (const fdPath of fdCandidates) { + try { + return await fs.realpath(fdPath); + } catch { + // try next fd path + } + } + throw new SafeOpenError("path-mismatch", "unable to resolve opened file path"); +} + +export async function openWritableFileWithinRoot(params: { + rootDir: string; + relativePath: string; + mkdir?: boolean; +}): Promise { const { rootReal, rootWithSep, resolved } = await resolvePathWithinRoot(params); try { await assertNoPathAliasEscape({ @@ -346,18 +376,29 @@ async function openWritableFileWithinRoot(params: { let openedRealPath: string | null = null; try { - const [stat, lstat] = await Promise.all([handle.stat(), fs.lstat(ioPath)]); - if (lstat.isSymbolicLink() || !stat.isFile()) { + const stat = await handle.stat(); + if (!stat.isFile()) { throw new SafeOpenError("invalid-path", "path is not a regular file under root"); } if (stat.nlink > 1) { throw new SafeOpenError("invalid-path", "hardlinked path not allowed"); } - if (!sameFileIdentity(stat, lstat)) { - throw new SafeOpenError("path-mismatch", "path changed during write"); + + try { + const lstat = await fs.lstat(ioPath); + if (lstat.isSymbolicLink() || !lstat.isFile()) { + throw new SafeOpenError("invalid-path", "path is not a regular file under root"); + } + if (!sameFileIdentity(stat, lstat)) { + throw new SafeOpenError("path-mismatch", "path changed during write"); + } + } catch (err) { + if (!isNotFoundPathError(err)) { + throw err; + } } - const realPath = await fs.realpath(ioPath); + const realPath = await resolveOpenedFileRealPathForHandle(handle, ioPath); openedRealPath = realPath; const realStat = await fs.stat(realPath); if (!sameFileIdentity(stat, realStat)) { @@ -381,10 +422,12 @@ async function openWritableFileWithinRoot(params: { openedRealPath: realPath, }; } catch (err) { - if (createdForWrite && err instanceof SafeOpenError && openedRealPath) { - await fs.rm(openedRealPath, { force: true }).catch(() => {}); - } + const cleanupCreatedPath = createdForWrite && err instanceof SafeOpenError; + const cleanupPath = openedRealPath ?? ioPath; await handle.close().catch(() => {}); + if (cleanupCreatedPath) { + await fs.rm(cleanupPath, { force: true }).catch(() => {}); + } throw err; } } diff --git a/src/test-utils/symlink-rebind-race.ts b/src/test-utils/symlink-rebind-race.ts new file mode 100644 index 00000000000..5ba92e27d31 --- /dev/null +++ b/src/test-utils/symlink-rebind-race.ts @@ -0,0 +1,36 @@ +import fs from "node:fs/promises"; +import { vi } from "vitest"; + +export async function withRealpathSymlinkRebindRace(params: { + shouldFlip: (realpathInput: string) => boolean; + symlinkPath: string; + symlinkTarget: string; + timing?: "before-realpath" | "after-realpath"; + run: () => Promise; +}): Promise { + const realRealpath = fs.realpath.bind(fs); + let flipped = false; + const realpathSpy = vi + .spyOn(fs, "realpath") + .mockImplementation(async (...args: Parameters) => { + const filePath = String(args[0]); + if (!flipped && params.shouldFlip(filePath)) { + flipped = true; + if (params.timing !== "after-realpath") { + await fs.rm(params.symlinkPath, { recursive: true, force: true }); + await fs.symlink(params.symlinkTarget, params.symlinkPath); + return await realRealpath(...args); + } + const resolved = await realRealpath(...args); + await fs.rm(params.symlinkPath, { recursive: true, force: true }); + await fs.symlink(params.symlinkTarget, params.symlinkPath); + return resolved; + } + return await realRealpath(...args); + }); + try { + return await params.run(); + } finally { + realpathSpy.mockRestore(); + } +}