fix(security): harden archive extraction (#16203)

* fix(browser): confine upload paths for file chooser * fix(browser): sanitize suggested download filenames * chore(lint): avoid control regex in download sanitizer * test(browser): cover absolute escape paths * docs(browser): update upload example path * refactor(browser): centralize upload path confinement * fix(infra): harden tmp dir selection * fix(security): harden archive extraction * fix(infra): harden tar extraction filter
2026-05-07 18:01:23 +00:00 · 2026-02-14 14:42:08 +01:00
parent 9a134c8a10
commit 3aa94afcfd
19 changed files with 1179 additions and 100 deletions
--- a/src/infra/archive.test.ts
+++ b/src/infra/archive.test.ts
@@ -49,6 +49,21 @@ describe("archive utils", () => {
    expect(content).toBe("hi");
  });

+  it("rejects zip path traversal (zip slip)", async () => {
+    const workDir = await makeTempDir();
+    const archivePath = path.join(workDir, "bundle.zip");
+    const extractDir = path.join(workDir, "a");
+
+    const zip = new JSZip();
+    zip.file("../b/evil.txt", "pwnd");
+    await fs.writeFile(archivePath, await zip.generateAsync({ type: "nodebuffer" }));
+
+    await fs.mkdir(extractDir, { recursive: true });
+    await expect(
+      extractArchive({ archivePath, destDir: extractDir, timeoutMs: 5_000 }),
+    ).rejects.toThrow(/(escapes destination|absolute)/i);
+  });
+
  it("extracts tar archives", async () => {
    const workDir = await makeTempDir();
    const archivePath = path.join(workDir, "bundle.tar");
@@ -65,4 +80,20 @@ describe("archive utils", () => {
    const content = await fs.readFile(path.join(rootDir, "hello.txt"), "utf-8");
    expect(content).toBe("yo");
  });
+
+  it("rejects tar path traversal (zip slip)", async () => {
+    const workDir = await makeTempDir();
+    const archivePath = path.join(workDir, "bundle.tar");
+    const extractDir = path.join(workDir, "extract");
+    const insideDir = path.join(workDir, "inside");
+    await fs.mkdir(insideDir, { recursive: true });
+    await fs.writeFile(path.join(workDir, "outside.txt"), "pwnd");
+
+    await tar.c({ cwd: insideDir, file: archivePath }, ["../outside.txt"]);
+
+    await fs.mkdir(extractDir, { recursive: true });
+    await expect(
+      extractArchive({ archivePath, destDir: extractDir, timeoutMs: 5_000 }),
+    ).rejects.toThrow(/escapes destination/i);
+  });
 });
--- a/src/infra/archive.ts
+++ b/src/infra/archive.ts
@@ -69,29 +69,99 @@ export async function withTimeout<T>(
  }
 }

-async function extractZip(params: { archivePath: string; destDir: string }): Promise<void> {
+function resolveSafeBaseDir(destDir: string): string {
+  const resolved = path.resolve(destDir);
+  return resolved.endsWith(path.sep) ? resolved : `${resolved}${path.sep}`;
+}
+
+function normalizeArchivePath(raw: string): string {
+  // Archives may contain Windows separators; treat them as separators.
+  return raw.replaceAll("\\", "/");
+}
+
+function isWindowsDrivePath(p: string): boolean {
+  return /^[a-zA-Z]:[\\/]/.test(p);
+}
+
+function validateArchiveEntryPath(entryPath: string): void {
+  if (!entryPath || entryPath === "." || entryPath === "./") {
+    return;
+  }
+  if (isWindowsDrivePath(entryPath)) {
+    throw new Error(`archive entry uses a drive path: ${entryPath}`);
+  }
+  const normalized = path.posix.normalize(normalizeArchivePath(entryPath));
+  if (normalized === ".." || normalized.startsWith("../")) {
+    throw new Error(`archive entry escapes destination: ${entryPath}`);
+  }
+  if (path.posix.isAbsolute(normalized) || normalized.startsWith("//")) {
+    throw new Error(`archive entry is absolute: ${entryPath}`);
+  }
+}
+
+function stripArchivePath(entryPath: string, stripComponents: number): string | null {
+  const normalized = path.posix.normalize(normalizeArchivePath(entryPath));
+  if (!normalized || normalized === "." || normalized === "./") {
+    return null;
+  }
+
+  // Keep the validation separate so callers can reject traversal in the original
+  // path even if stripping could make it "look" safe.
+  const parts = normalized.split("/").filter((part) => part.length > 0 && part !== ".");
+  const strip = Math.max(0, Math.floor(stripComponents));
+  const stripped = strip === 0 ? parts.join("/") : parts.slice(strip).join("/");
+  const result = path.posix.normalize(stripped);
+  if (!result || result === "." || result === "./") {
+    return null;
+  }
+  return result;
+}
+
+function resolveCheckedOutPath(destDir: string, relPath: string, original: string): string {
+  const safeBase = resolveSafeBaseDir(destDir);
+  const outPath = path.resolve(destDir, relPath);
+  if (!outPath.startsWith(safeBase)) {
+    throw new Error(`archive entry escapes destination: ${original}`);
+  }
+  return outPath;
+}
+
+async function extractZip(params: {
+  archivePath: string;
+  destDir: string;
+  stripComponents?: number;
+}): Promise<void> {
  const buffer = await fs.readFile(params.archivePath);
  const zip = await JSZip.loadAsync(buffer);
  const entries = Object.values(zip.files);
+  const strip = Math.max(0, Math.floor(params.stripComponents ?? 0));

  for (const entry of entries) {
-    const entryPath = entry.name.replaceAll("\\", "/");
-    if (!entryPath || entryPath.endsWith("/")) {
-      const dirPath = path.resolve(params.destDir, entryPath);
-      if (!dirPath.startsWith(params.destDir)) {
-        throw new Error(`zip entry escapes destination: ${entry.name}`);
-      }
-      await fs.mkdir(dirPath, { recursive: true });
+    validateArchiveEntryPath(entry.name);
+
+    const relPath = stripArchivePath(entry.name, strip);
+    if (!relPath) {
+      continue;
+    }
+    validateArchiveEntryPath(relPath);
+
+    const outPath = resolveCheckedOutPath(params.destDir, relPath, entry.name);
+    if (entry.dir) {
+      await fs.mkdir(outPath, { recursive: true });
      continue;
    }

-    const outPath = path.resolve(params.destDir, entryPath);
-    if (!outPath.startsWith(params.destDir)) {
-      throw new Error(`zip entry escapes destination: ${entry.name}`);
-    }
    await fs.mkdir(path.dirname(outPath), { recursive: true });
    const data = await entry.async("nodebuffer");
    await fs.writeFile(outPath, data);
+
+    // Best-effort permission restore for zip entries created on unix.
+    if (typeof entry.unixPermissions === "number") {
+      const mode = entry.unixPermissions & 0o777;
+      if (mode !== 0) {
+        await fs.chmod(outPath, mode).catch(() => undefined);
+      }
+    }
  }
 }

@@ -99,24 +169,82 @@ export async function extractArchive(params: {
  archivePath: string;
  destDir: string;
  timeoutMs: number;
+  kind?: ArchiveKind;
+  stripComponents?: number;
+  tarGzip?: boolean;
  logger?: ArchiveLogger;
 }): Promise<void> {
-  const kind = resolveArchiveKind(params.archivePath);
+  const kind = params.kind ?? resolveArchiveKind(params.archivePath);
  if (!kind) {
    throw new Error(`unsupported archive: ${params.archivePath}`);
  }

  const label = kind === "zip" ? "extract zip" : "extract tar";
  if (kind === "tar") {
+    const strip = Math.max(0, Math.floor(params.stripComponents ?? 0));
+    let firstError: Error | undefined;
    await withTimeout(
-      tar.x({ file: params.archivePath, cwd: params.destDir }),
+      tar.x({
+        file: params.archivePath,
+        cwd: params.destDir,
+        strip,
+        gzip: params.tarGzip,
+        preservePaths: false,
+        strict: true,
+        filter: (entryPath, entry) => {
+          try {
+            validateArchiveEntryPath(entryPath);
+            // `tar`'s filter callback can pass either a ReadEntry or a Stats-ish object;
+            // fail closed for any link-like entries.
+            const entryType =
+              typeof entry === "object" &&
+              entry !== null &&
+              "type" in entry &&
+              typeof (entry as { type?: unknown }).type === "string"
+                ? (entry as { type: string }).type
+                : undefined;
+            const isSymlink =
+              typeof entry === "object" &&
+              entry !== null &&
+              "isSymbolicLink" in entry &&
+              typeof (entry as { isSymbolicLink?: unknown }).isSymbolicLink === "function" &&
+              Boolean((entry as { isSymbolicLink: () => boolean }).isSymbolicLink());
+            if (entryType === "SymbolicLink" || entryType === "Link" || isSymlink) {
+              throw new Error(`tar entry is a link: ${entryPath}`);
+            }
+            const relPath = stripArchivePath(entryPath, strip);
+            if (!relPath) {
+              return false;
+            }
+            validateArchiveEntryPath(relPath);
+            resolveCheckedOutPath(params.destDir, relPath, entryPath);
+            return true;
+          } catch (err) {
+            if (!firstError) {
+              firstError = err instanceof Error ? err : new Error(String(err));
+            }
+            return false;
+          }
+        },
+      }),
      params.timeoutMs,
      label,
    );
+    if (firstError) {
+      throw firstError;
+    }
    return;
  }

-  await withTimeout(extractZip(params), params.timeoutMs, label);
+  await withTimeout(
+    extractZip({
+      archivePath: params.archivePath,
+      destDir: params.destDir,
+      stripComponents: params.stripComponents,
+    }),
+    params.timeoutMs,
+    label,
+  );
 }

 export async function fileExists(filePath: string): Promise<boolean> {
--- a/src/infra/tmp-openclaw-dir.test.ts
+++ b/src/infra/tmp-openclaw-dir.test.ts
@@ -5,12 +5,25 @@ import { POSIX_OPENCLAW_TMP_DIR, resolvePreferredOpenClawTmpDir } from "./tmp-op
 describe("resolvePreferredOpenClawTmpDir", () => {
  it("prefers /tmp/openclaw when it already exists and is writable", () => {
    const accessSync = vi.fn();
-    const statSync = vi.fn(() => ({ isDirectory: () => true }));
+    const lstatSync = vi.fn(() => ({
+      isDirectory: () => true,
+      isSymbolicLink: () => false,
+      uid: 501,
+      mode: 0o40700,
+    }));
+    const mkdirSync = vi.fn();
+    const getuid = vi.fn(() => 501);
    const tmpdir = vi.fn(() => "/var/fallback");

-    const resolved = resolvePreferredOpenClawTmpDir({ accessSync, statSync, tmpdir });
+    const resolved = resolvePreferredOpenClawTmpDir({
+      accessSync,
+      lstatSync,
+      mkdirSync,
+      getuid,
+      tmpdir,
+    });

-    expect(statSync).toHaveBeenCalledTimes(1);
+    expect(lstatSync).toHaveBeenCalledTimes(1);
    expect(accessSync).toHaveBeenCalledTimes(1);
    expect(resolved).toBe(POSIX_OPENCLAW_TMP_DIR);
    expect(tmpdir).not.toHaveBeenCalled();
@@ -18,28 +31,63 @@ describe("resolvePreferredOpenClawTmpDir", () => {

  it("prefers /tmp/openclaw when it does not exist but /tmp is writable", () => {
    const accessSync = vi.fn();
-    const statSync = vi.fn(() => {
+    const lstatSync = vi.fn(() => {
      const err = new Error("missing") as Error & { code?: string };
      err.code = "ENOENT";
      throw err;
    });
+    const mkdirSync = vi.fn();
+    const getuid = vi.fn(() => 501);
    const tmpdir = vi.fn(() => "/var/fallback");

-    const resolved = resolvePreferredOpenClawTmpDir({ accessSync, statSync, tmpdir });
+    // second lstat call (after mkdir) should succeed
+    lstatSync.mockImplementationOnce(() => {
+      const err = new Error("missing") as Error & { code?: string };
+      err.code = "ENOENT";
+      throw err;
+    });
+    lstatSync.mockImplementationOnce(() => ({
+      isDirectory: () => true,
+      isSymbolicLink: () => false,
+      uid: 501,
+      mode: 0o40700,
+    }));
+
+    const resolved = resolvePreferredOpenClawTmpDir({
+      accessSync,
+      lstatSync,
+      mkdirSync,
+      getuid,
+      tmpdir,
+    });

    expect(resolved).toBe(POSIX_OPENCLAW_TMP_DIR);
    expect(accessSync).toHaveBeenCalledWith("/tmp", expect.any(Number));
+    expect(mkdirSync).toHaveBeenCalledWith(POSIX_OPENCLAW_TMP_DIR, expect.any(Object));
    expect(tmpdir).not.toHaveBeenCalled();
  });

  it("falls back to os.tmpdir()/openclaw when /tmp/openclaw is not a directory", () => {
    const accessSync = vi.fn();
-    const statSync = vi.fn(() => ({ isDirectory: () => false }));
+    const lstatSync = vi.fn(() => ({
+      isDirectory: () => false,
+      isSymbolicLink: () => false,
+      uid: 501,
+      mode: 0o100644,
+    }));
+    const mkdirSync = vi.fn();
+    const getuid = vi.fn(() => 501);
    const tmpdir = vi.fn(() => "/var/fallback");

-    const resolved = resolvePreferredOpenClawTmpDir({ accessSync, statSync, tmpdir });
+    const resolved = resolvePreferredOpenClawTmpDir({
+      accessSync,
+      lstatSync,
+      mkdirSync,
+      getuid,
+      tmpdir,
+    });

-    expect(resolved).toBe(path.join("/var/fallback", "openclaw"));
+    expect(resolved).toBe(path.join("/var/fallback", "openclaw-501"));
    expect(tmpdir).toHaveBeenCalledTimes(1);
  });

@@ -49,16 +97,96 @@ describe("resolvePreferredOpenClawTmpDir", () => {
        throw new Error("read-only");
      }
    });
-    const statSync = vi.fn(() => {
+    const lstatSync = vi.fn(() => {
      const err = new Error("missing") as Error & { code?: string };
      err.code = "ENOENT";
      throw err;
    });
+    const mkdirSync = vi.fn();
+    const getuid = vi.fn(() => 501);
    const tmpdir = vi.fn(() => "/var/fallback");

-    const resolved = resolvePreferredOpenClawTmpDir({ accessSync, statSync, tmpdir });
+    const resolved = resolvePreferredOpenClawTmpDir({
+      accessSync,
+      lstatSync,
+      mkdirSync,
+      getuid,
+      tmpdir,
+    });

-    expect(resolved).toBe(path.join("/var/fallback", "openclaw"));
+    expect(resolved).toBe(path.join("/var/fallback", "openclaw-501"));
+    expect(tmpdir).toHaveBeenCalledTimes(1);
+  });
+
+  it("falls back when /tmp/openclaw is a symlink", () => {
+    const accessSync = vi.fn();
+    const lstatSync = vi.fn(() => ({
+      isDirectory: () => true,
+      isSymbolicLink: () => true,
+      uid: 501,
+      mode: 0o120777,
+    }));
+    const mkdirSync = vi.fn();
+    const getuid = vi.fn(() => 501);
+    const tmpdir = vi.fn(() => "/var/fallback");
+
+    const resolved = resolvePreferredOpenClawTmpDir({
+      accessSync,
+      lstatSync,
+      mkdirSync,
+      getuid,
+      tmpdir,
+    });
+
+    expect(resolved).toBe(path.join("/var/fallback", "openclaw-501"));
+    expect(tmpdir).toHaveBeenCalledTimes(1);
+  });
+
+  it("falls back when /tmp/openclaw is not owned by the current user", () => {
+    const accessSync = vi.fn();
+    const lstatSync = vi.fn(() => ({
+      isDirectory: () => true,
+      isSymbolicLink: () => false,
+      uid: 0,
+      mode: 0o40700,
+    }));
+    const mkdirSync = vi.fn();
+    const getuid = vi.fn(() => 501);
+    const tmpdir = vi.fn(() => "/var/fallback");
+
+    const resolved = resolvePreferredOpenClawTmpDir({
+      accessSync,
+      lstatSync,
+      mkdirSync,
+      getuid,
+      tmpdir,
+    });
+
+    expect(resolved).toBe(path.join("/var/fallback", "openclaw-501"));
+    expect(tmpdir).toHaveBeenCalledTimes(1);
+  });
+
+  it("falls back when /tmp/openclaw is group/other writable", () => {
+    const accessSync = vi.fn();
+    const lstatSync = vi.fn(() => ({
+      isDirectory: () => true,
+      isSymbolicLink: () => false,
+      uid: 501,
+      mode: 0o40777,
+    }));
+    const mkdirSync = vi.fn();
+    const getuid = vi.fn(() => 501);
+    const tmpdir = vi.fn(() => "/var/fallback");
+
+    const resolved = resolvePreferredOpenClawTmpDir({
+      accessSync,
+      lstatSync,
+      mkdirSync,
+      getuid,
+      tmpdir,
+    });
+
+    expect(resolved).toBe(path.join("/var/fallback", "openclaw-501"));
    expect(tmpdir).toHaveBeenCalledTimes(1);
  });
 });
--- a/src/infra/tmp-openclaw-dir.ts
+++ b/src/infra/tmp-openclaw-dir.ts
@@ -6,7 +6,14 @@ export const POSIX_OPENCLAW_TMP_DIR = "/tmp/openclaw";

 type ResolvePreferredOpenClawTmpDirOptions = {
  accessSync?: (path: string, mode?: number) => void;
-  statSync?: (path: string) => { isDirectory(): boolean };
+  lstatSync?: (path: string) => {
+    isDirectory(): boolean;
+    isSymbolicLink(): boolean;
+    mode?: number;
+    uid?: number;
+  };
+  mkdirSync?: (path: string, opts: { recursive: boolean; mode?: number }) => void;
+  getuid?: () => number | undefined;
  tmpdir?: () => string;
 };

@@ -25,26 +32,73 @@ export function resolvePreferredOpenClawTmpDir(
  options: ResolvePreferredOpenClawTmpDirOptions = {},
 ): string {
  const accessSync = options.accessSync ?? fs.accessSync;
-  const statSync = options.statSync ?? fs.statSync;
+  const lstatSync = options.lstatSync ?? fs.lstatSync;
+  const mkdirSync = options.mkdirSync ?? fs.mkdirSync;
+  const getuid =
+    options.getuid ??
+    (() => {
+      try {
+        return typeof process.getuid === "function" ? process.getuid() : undefined;
+      } catch {
+        return undefined;
+      }
+    });
  const tmpdir = options.tmpdir ?? os.tmpdir;
+  const uid = getuid();
+
+  const isSecureDirForUser = (st: { mode?: number; uid?: number }): boolean => {
+    if (uid === undefined) {
+      return true;
+    }
+    if (typeof st.uid === "number" && st.uid !== uid) {
+      return false;
+    }
+    // Avoid group/other writable dirs when running on multi-user hosts.
+    if (typeof st.mode === "number" && (st.mode & 0o022) !== 0) {
+      return false;
+    }
+    return true;
+  };
+
+  const fallback = (): string => {
+    const base = tmpdir();
+    const suffix = uid === undefined ? "openclaw" : `openclaw-${uid}`;
+    return path.join(base, suffix);
+  };

  try {
-    const preferred = statSync(POSIX_OPENCLAW_TMP_DIR);
-    if (!preferred.isDirectory()) {
-      return path.join(tmpdir(), "openclaw");
+    const preferred = lstatSync(POSIX_OPENCLAW_TMP_DIR);
+    if (!preferred.isDirectory() || preferred.isSymbolicLink()) {
+      return fallback();
    }
    accessSync(POSIX_OPENCLAW_TMP_DIR, fs.constants.W_OK | fs.constants.X_OK);
+    if (!isSecureDirForUser(preferred)) {
+      return fallback();
+    }
    return POSIX_OPENCLAW_TMP_DIR;
  } catch (err) {
    if (!isNodeErrorWithCode(err, "ENOENT")) {
-      return path.join(tmpdir(), "openclaw");
+      return fallback();
    }
  }

  try {
    accessSync("/tmp", fs.constants.W_OK | fs.constants.X_OK);
+    // Create with a safe default; subsequent callers expect it exists.
+    mkdirSync(POSIX_OPENCLAW_TMP_DIR, { recursive: true, mode: 0o700 });
+    try {
+      const preferred = lstatSync(POSIX_OPENCLAW_TMP_DIR);
+      if (!preferred.isDirectory() || preferred.isSymbolicLink()) {
+        return fallback();
+      }
+      if (!isSecureDirForUser(preferred)) {
+        return fallback();
+      }
+    } catch {
+      return fallback();
+    }
    return POSIX_OPENCLAW_TMP_DIR;
  } catch {
-    return path.join(tmpdir(), "openclaw");
+    return fallback();
  }
 }