From d861d5195906832f094e9a3431a61980ba8b3b06 Mon Sep 17 00:00:00 2001 From: YeonGyu-Kim Date: Thu, 2 Apr 2026 15:01:15 +0900 Subject: [PATCH] fix(security): add archive extraction containment validation Validate tar and zip entries before extraction to prevent path traversal: - Reject absolute paths in archives - Reject .. traversal paths - Reject symlinks pointing outside extraction dir - New archive-entry-validator module with comprehensive tests Addresses: security audit finding for unsafe archive extraction. --- src/shared/archive-entry-validator.test.ts | 184 +++++++++++++++++++++ src/shared/archive-entry-validator.ts | 74 +++++++++ src/shared/binary-downloader.ts | 55 ++++++ src/shared/zip-entry-listing.ts | 122 ++++++++++++++ src/shared/zip-extractor.ts | 19 +++ 5 files changed, 454 insertions(+) create mode 100644 src/shared/archive-entry-validator.test.ts create mode 100644 src/shared/archive-entry-validator.ts create mode 100644 src/shared/zip-entry-listing.ts diff --git a/src/shared/archive-entry-validator.test.ts b/src/shared/archive-entry-validator.test.ts new file mode 100644 index 000000000..96c1bbbba --- /dev/null +++ b/src/shared/archive-entry-validator.test.ts @@ -0,0 +1,184 @@ +/// + +import { afterEach, describe, expect, it } from "bun:test" +import { lstatSync, mkdirSync, mkdtempSync, readFileSync, rmSync, symlinkSync, writeFileSync } from "node:fs" +import { tmpdir } from "node:os" +import { join } from "node:path" +import { spawnSync } from "bun" + +import { extractTarGz } from "./binary-downloader" +import { validateArchiveEntries } from "./archive-entry-validator" +import { extractZip } from "./zip-extractor" + +const testDirs: string[] = [] + +function createTestDir(): string { + const dir = mkdtempSync(join(tmpdir(), "archive-entry-validator-")) + testDirs.push(dir) + return dir +} + +function runCommand(command: string, cwd?: string): void { + const result = spawnSync(["bash", "-lc", command], { cwd, stderr: "pipe", stdout: "pipe" }) + if (result.exitCode !== 0) { + throw new Error(result.stderr.toString()) + } +} + +function writePythonScript(dir: string, filename: string, content: string): string { + const scriptPath = join(dir, filename) + writeFileSync(scriptPath, content) + return scriptPath +} + +afterEach(() => { + for (const dir of testDirs.splice(0)) { + rmSync(dir, { recursive: true, force: true }) + } +}) + +describe("validateArchiveEntries", () => { + it("rejects absolute paths and traversal entries", () => { + //#given + const destDir = "/tmp/archive-root" + + //#when + const rejectAbsolutePath = () => + validateArchiveEntries([{ path: "/etc/passwd", type: "file" }], destDir) + const rejectTraversalPath = () => + validateArchiveEntries([{ path: "nested/../../evil.txt", type: "file" }], destDir) + + //#then + expect(rejectAbsolutePath).toThrow(/absolute path/i) + expect(rejectTraversalPath).toThrow(/path traversal/i) + }) + + it("rejects symlink targets that escape the extraction directory", () => { + //#given + const destDir = "/tmp/archive-root" + + //#when + const rejectEscapeSymlink = () => + validateArchiveEntries( + [{ path: "bin/tool", type: "symlink", linkPath: "../../outside/tool" }], + destDir + ) + + //#then + expect(rejectEscapeSymlink).toThrow(/symlink target/i) + }) + + it("accepts contained files, directories, and symlinks", () => { + //#given + const destDir = "/tmp/archive-root" + const entries = [ + { path: "bin/", type: "directory" as const }, + { path: "bin/tool", type: "file" as const }, + { path: "bin/tool-link", type: "symlink" as const, linkPath: "tool" }, + ] + + //#when + const validateContainedEntries = () => validateArchiveEntries(entries, destDir) + + //#then + expect(validateContainedEntries).not.toThrow() + }) +}) + +describe("archive extraction preflight", () => { + it("rejects tar archives with traversal entries before extraction", async () => { + //#given + const rootDir = createTestDir() + const archivePath = join(rootDir, "malicious.tar.gz") + const destDir = join(rootDir, "dest") + mkdirSync(destDir, { recursive: true }) + const scriptPath = writePythonScript( + rootDir, + "make-malicious-tar.py", + [ + "import io", + "import sys", + "import tarfile", + "with tarfile.open(sys.argv[1], 'w:gz') as archive:", + " data = b'owned'", + " info = tarfile.TarInfo('../escape.txt')", + " info.size = len(data)", + " archive.addfile(info, io.BytesIO(data))", + ].join("\n") + ) + runCommand(`python3 "${scriptPath}" "${archivePath}"`) + + //#when + let errorMessage = "" + try { + await extractTarGz(archivePath, destDir) + } catch (error) { + errorMessage = error instanceof Error ? error.message : String(error) + } + + //#then + expect(errorMessage).toMatch(/path traversal/i) + }) + + it("rejects zip archives with symlink escapes before extraction", async () => { + //#given + const rootDir = createTestDir() + const archivePath = join(rootDir, "malicious.zip") + const destDir = join(rootDir, "dest") + mkdirSync(destDir, { recursive: true }) + const scriptPath = writePythonScript( + rootDir, + "make-malicious-zip.py", + [ + "import stat", + "import sys", + "import zipfile", + "archive = zipfile.ZipFile(sys.argv[1], 'w')", + "entry = zipfile.ZipInfo('bin/tool-link')", + "entry.create_system = 3", + "entry.external_attr = (stat.S_IFLNK | 0o777) << 16", + "archive.writestr(entry, '../../escape.txt')", + "archive.close()", + ].join("\n") + ) + runCommand(`python3 "${scriptPath}" "${archivePath}"`) + + //#when + let errorMessage = "" + try { + await extractZip(archivePath, destDir) + } catch (error) { + errorMessage = error instanceof Error ? error.message : String(error) + } + + //#then + expect(errorMessage).toMatch(/symlink target/i) + }) + + it("extracts safe tar and zip archives into the destination directory", async () => { + //#given + const rootDir = createTestDir() + const sourceDir = join(rootDir, "source") + const tarArchivePath = join(rootDir, "safe.tar.gz") + const zipArchivePath = join(rootDir, "safe.zip") + const tarDestDir = join(rootDir, "tar-dest") + const zipDestDir = join(rootDir, "zip-dest") + mkdirSync(join(sourceDir, "bin"), { recursive: true }) + mkdirSync(tarDestDir, { recursive: true }) + mkdirSync(zipDestDir, { recursive: true }) + writeFileSync(join(sourceDir, "bin", "tool.txt"), "safe") + symlinkSync("tool.txt", join(sourceDir, "bin", "tool-link")) + runCommand(`tar -czf "${tarArchivePath}" -C "${sourceDir}" .`) + runCommand(`zip -qry "${zipArchivePath}" .`, sourceDir) + + //#when + await extractTarGz(tarArchivePath, tarDestDir) + await extractZip(zipArchivePath, zipDestDir) + + //#then + expect(readFileSync(join(tarDestDir, "bin", "tool.txt"), "utf8")).toBe("safe") + expect(readFileSync(join(zipDestDir, "bin", "tool.txt"), "utf8")).toBe("safe") + expect(lstatSync(join(tarDestDir, "bin", "tool-link")).isSymbolicLink()).toBe(true) + expect(lstatSync(join(zipDestDir, "bin", "tool-link")).isSymbolicLink()).toBe(true) + }) +}) diff --git a/src/shared/archive-entry-validator.ts b/src/shared/archive-entry-validator.ts new file mode 100644 index 000000000..39d117759 --- /dev/null +++ b/src/shared/archive-entry-validator.ts @@ -0,0 +1,74 @@ +import { dirname, isAbsolute, relative, resolve, sep } from "node:path" + +export type ArchiveEntry = { + path: string + type: "file" | "directory" | "symlink" + linkPath?: string +} + +function normalizeArchivePath(filePath: string): string { + return filePath.replaceAll("\\", "/") +} + +function containsTraversalSegment(filePath: string): boolean { + return normalizeArchivePath(filePath) + .split("/") + .some(segment => segment === "..") +} + +function isArchiveAbsolutePath(filePath: string): boolean { + const normalizedPath = normalizeArchivePath(filePath) + return isAbsolute(normalizedPath) || /^[A-Za-z]:\//.test(normalizedPath) || normalizedPath.startsWith("//") +} + +function escapesDirectory(rootDir: string, candidatePath: string): boolean { + const relativePath = relative(rootDir, candidatePath) + return relativePath === ".." || relativePath.startsWith(`..${sep}`) || isAbsolute(relativePath) +} + +function resolveContainedPath(rootDir: string, filePath: string, errorLabel: string): string { + const normalizedPath = normalizeArchivePath(filePath) + if (isArchiveAbsolutePath(normalizedPath)) { + throw new Error(`Unsafe archive entry: ${errorLabel} uses an absolute path (${filePath})`) + } + + if (containsTraversalSegment(normalizedPath)) { + throw new Error(`Unsafe archive entry: ${errorLabel} contains path traversal (${filePath})`) + } + + const resolvedPath = resolve(rootDir, normalizedPath) + if (escapesDirectory(rootDir, resolvedPath)) { + throw new Error(`Unsafe archive entry: ${errorLabel} contains path traversal (${filePath})`) + } + + return resolvedPath +} + +export function validateArchiveEntries(entries: ArchiveEntry[], destDir: string): void { + const resolvedDestDir = resolve(destDir) + + for (const entry of entries) { + const resolvedEntryPath = resolveContainedPath(resolvedDestDir, entry.path, "path") + if (entry.type !== "symlink") { + continue + } + + if (!entry.linkPath) { + throw new Error(`Unsafe archive entry: symlink target missing for ${entry.path}`) + } + + const normalizedLinkPath = normalizeArchivePath(entry.linkPath) + if (isArchiveAbsolutePath(normalizedLinkPath)) { + throw new Error(`Unsafe archive entry: symlink target uses an absolute path (${entry.linkPath})`) + } + + if (containsTraversalSegment(normalizedLinkPath)) { + throw new Error(`Unsafe archive entry: symlink target contains path traversal (${entry.linkPath})`) + } + + const resolvedLinkPath = resolve(dirname(resolvedEntryPath), normalizedLinkPath) + if (escapesDirectory(resolvedDestDir, resolvedLinkPath)) { + throw new Error(`Unsafe archive entry: symlink target escapes extraction directory (${entry.linkPath})`) + } + } +} diff --git a/src/shared/binary-downloader.ts b/src/shared/binary-downloader.ts index a47056cab..28b737311 100644 --- a/src/shared/binary-downloader.ts +++ b/src/shared/binary-downloader.ts @@ -1,6 +1,7 @@ import { chmodSync, existsSync, mkdirSync, unlinkSync } from "node:fs"; import * as path from "node:path"; import { spawn } from "bun"; +import { validateArchiveEntries, type ArchiveEntry } from "./archive-entry-validator"; import { extractZip } from "./zip-extractor"; export function getCachedBinaryPath(cacheDir: string, binaryName: string): string | null { @@ -29,6 +30,9 @@ export async function extractTarGz( destDir: string, options?: { args?: string[]; cwd?: string } ): Promise { + const entries = await listTarEntries(archivePath, options?.cwd) + validateArchiveEntries(entries, destDir) + const args = options?.args ?? ["tar", "-xzf", archivePath, "-C", destDir]; const proc = spawn(args, { cwd: options?.cwd, @@ -58,3 +62,54 @@ export function ensureExecutable(binaryPath: string): void { chmodSync(binaryPath, 0o755); } } + +function parseTarEntry(line: string): ArchiveEntry | null { + const match = line.match(/^([^\s])\S*\s+\d+\s+\S+\s+\S+\s+\d+\s+\w+\s+\d+\s+(?:\d{2}:\d{2}|\d{4})\s+(.*)$/) + if (!match) { + return null + } + + const [, rawType, rawEntryPath] = match + if (rawType === "l") { + const arrowIndex = rawEntryPath.lastIndexOf(" -> ") + if (arrowIndex === -1) { + return { path: rawEntryPath, type: "symlink" } + } + + return { + path: rawEntryPath.slice(0, arrowIndex), + type: "symlink", + linkPath: rawEntryPath.slice(arrowIndex + 4), + } + } + + return { + path: rawEntryPath, + type: rawType === "d" ? "directory" : "file", + } +} + +async function listTarEntries(archivePath: string, cwd?: string): Promise { + const proc = spawn(["tar", "-tvzf", archivePath], { + cwd, + stdout: "pipe", + stderr: "pipe", + }) + + const [exitCode, stdout, stderr] = await Promise.all([ + proc.exited, + new Response(proc.stdout).text(), + new Response(proc.stderr).text(), + ]) + + if (exitCode !== 0) { + throw new Error(`tar entry listing failed (exit ${exitCode}): ${stderr}`) + } + + return stdout + .split(/\r?\n/) + .map(line => line.trim()) + .filter(Boolean) + .map(line => parseTarEntry(line)) + .filter((entry): entry is ArchiveEntry => entry !== null) +} diff --git a/src/shared/zip-entry-listing.ts b/src/shared/zip-entry-listing.ts new file mode 100644 index 000000000..730713c84 --- /dev/null +++ b/src/shared/zip-entry-listing.ts @@ -0,0 +1,122 @@ +import { spawn } from "bun" + +import type { ArchiveEntry } from "./archive-entry-validator" + +function parseTarListedZipEntry(line: string): ArchiveEntry | null { + const match = line.match(/^([^\s])\S*\s+\d+\s+\S+\s+\S+\s+\d+\s+\w+\s+\d+\s+(?:\d{2}:\d{2}|\d{4})\s+(.*)$/) + if (!match) { + return null + } + + const [, rawType, rawEntryPath] = match + if (rawType === "l") { + const arrowIndex = rawEntryPath.lastIndexOf(" -> ") + return { + path: arrowIndex === -1 ? rawEntryPath : rawEntryPath.slice(0, arrowIndex), + type: "symlink", + linkPath: arrowIndex === -1 ? undefined : rawEntryPath.slice(arrowIndex + 4), + } + } + + return { + path: rawEntryPath, + type: rawType === "d" ? "directory" : "file", + } +} + +export async function listZipEntriesWithTar(archivePath: string): Promise { + const proc = spawn(["tar", "-tvf", archivePath], { + stdout: "pipe", + stderr: "pipe", + }) + + const [exitCode, stdout, stderr] = await Promise.all([ + proc.exited, + new Response(proc.stdout).text(), + new Response(proc.stderr).text(), + ]) + + if (exitCode !== 0) { + throw new Error(`zip entry listing failed (exit ${exitCode}): ${stderr}`) + } + + return stdout + .split(/\r?\n/) + .map(line => line.trim()) + .filter(Boolean) + .map(line => parseTarListedZipEntry(line)) + .filter((entry): entry is ArchiveEntry => entry !== null) +} + +export async function listZipEntriesWithPowerShell( + archivePath: string, + escapePowerShellPath: (path: string) => string, + extractor: "pwsh" | "powershell" +): Promise { + const proc = spawn( + [ + extractor, + "-Command", + [ + "Add-Type -AssemblyName System.IO.Compression.FileSystem", + `$archive = [System.IO.Compression.ZipFile]::OpenRead('${escapePowerShellPath(archivePath)}')`, + "try {", + " foreach ($entry in $archive.Entries) {", + " $mode = ($entry.ExternalAttributes -shr 16) -band 0xFFFF", + " $type = if (($mode -band 0xF000) -eq 0xA000) { 'symlink' } elseif ($entry.FullName.EndsWith('/')) { 'directory' } else { 'file' }", + " $target = ''", + " if ($type -eq 'symlink') {", + " $stream = $entry.Open()", + " try {", + " $reader = New-Object System.IO.StreamReader($stream)", + " try { $target = $reader.ReadToEnd() } finally { $reader.Dispose() }", + " } finally { $stream.Dispose() }", + " }", + " Write-Output ($type + \"`t\" + $entry.FullName + \"`t\" + $target)", + " }", + "} finally {", + " $archive.Dispose()", + "}", + ].join("; "), + ], + { + stdout: "pipe", + stderr: "pipe", + } + ) + + const [exitCode, stdout, stderr] = await Promise.all([ + proc.exited, + new Response(proc.stdout).text(), + new Response(proc.stderr).text(), + ]) + + if (exitCode !== 0) { + throw new Error(`zip entry listing failed (exit ${exitCode}): ${stderr}`) + } + + return stdout + .split(/\r?\n/) + .map(line => line.trim()) + .filter(Boolean) + .map((line): ArchiveEntry | null => { + const [type, entryPath, linkPath = ""] = line.split("\t") + if (type !== "file" && type !== "directory" && type !== "symlink") { + return null + } + + if (type === "symlink") { + return { + path: entryPath, + type, + linkPath, + } + } + + return { + path: entryPath, + type, + } + }) + .filter((entry): entry is ArchiveEntry => entry !== null) +} diff --git a/src/shared/zip-extractor.ts b/src/shared/zip-extractor.ts index ee961722f..58da48ebf 100644 --- a/src/shared/zip-extractor.ts +++ b/src/shared/zip-extractor.ts @@ -1,6 +1,9 @@ import { spawn, spawnSync } from "bun" import { release } from "os" +import { validateArchiveEntries } from "./archive-entry-validator" +import { listZipEntriesWithPowerShell, listZipEntriesWithTar } from "./zip-entry-listing" + const WINDOWS_BUILD_WITH_TAR = 17134 function getWindowsBuildNumber(): number | null { @@ -41,6 +44,9 @@ function getWindowsZipExtractor(): WindowsZipExtractor { } export async function extractZip(archivePath: string, destDir: string): Promise { + const entries = await listZipEntries(archivePath) + validateArchiveEntries(entries, destDir) + let proc if (process.platform === "win32") { @@ -81,3 +87,16 @@ export async function extractZip(archivePath: string, destDir: string): Promise< throw new Error(`zip extraction failed (exit ${exitCode}): ${stderr}`) } } + +async function listZipEntries(archivePath: string) { + if (process.platform === "win32") { + const extractor = getWindowsZipExtractor() + if (extractor === "tar") { + return listZipEntriesWithTar(archivePath) + } + + return listZipEntriesWithPowerShell(archivePath, escapePowerShellPath, extractor) + } + + return listZipEntriesWithTar(archivePath) +}