fix(security): harden zip write race handling

2026-03-12 15:30:39 +00:00 · 2026-03-02 17:38:03 +00:00
parent eb35fb745d
commit 7dac9b05dd
5 changed files with 279 additions and 68 deletions
--- 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<ArchiveSecurityError>);
+          },
+        });
+
+        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");
--- 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<OpenZipOutputFileResult> {
+  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<void> {
@@ -377,12 +468,21 @@ async function prepareZipOutputPath(params: {
 async function writeZipFileEntry(params: {
  entry: ZipEntry;
  outPath: string;
+  destinationRealDir: string;
  budget: ZipExtractBudget;
 }): Promise<void> {
-  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,
    });
  }
--- 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));
    },
--- 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<string> {
+  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<SafeWritableOpenResult> {
  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;
  }
 }
--- a/src/test-utils/symlink-rebind-race.ts
+++ 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<T>(params: {
+  shouldFlip: (realpathInput: string) => boolean;
+  symlinkPath: string;
+  symlinkTarget: string;
+  timing?: "before-realpath" | "after-realpath";
+  run: () => Promise<T>;
+}): Promise<T> {
+  const realRealpath = fs.realpath.bind(fs);
+  let flipped = false;
+  const realpathSpy = vi
+    .spyOn(fs, "realpath")
+    .mockImplementation(async (...args: Parameters<typeof fs.realpath>) => {
+      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();
+  }
+}