From 4b226b74f5fd3b106a83a6347fd404172e2fd246 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sat, 21 Feb 2026 19:42:11 +0100 Subject: [PATCH] fix(security): block zip symlink escape in archive extraction --- CHANGELOG.md | 2 + src/infra/archive.test.ts | 26 ++++++++ src/infra/archive.ts | 136 +++++++++++++++++++++++++++++++++++++- 3 files changed, 161 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 50cc758d3..93cfcbf73 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -116,6 +116,8 @@ Docs: https://docs.openclaw.ai - Signal/Outbound: preserve case for Base64 group IDs during outbound target normalization so cross-context routing and policy checks no longer break when group IDs include uppercase characters. (#5578) Thanks @heyhudson. - Anthropic/Agents: preserve required pi-ai default OAuth beta headers when `context1m` injects `anthropic-beta`, preventing 401 auth failures for `sk-ant-oat-*` tokens. (#19789, fixes #19769) Thanks @minupla. - Security/Exec: block unquoted heredoc body expansion tokens in shell allowlist analysis, reject unterminated heredocs, and require explicit approval for allowlisted heredoc execution on gateway hosts to prevent heredoc substitution allowlist bypass. Thanks @torturado for reporting. +- macOS/Security: evaluate `system.run` allowlists per shell segment in macOS node runtime and companion exec host (including chained shell operators), fail closed on shell/process substitution parsing, and require explicit approval on unsafe parse cases to prevent allowlist bypass via `rawCommand` chaining. Thanks @tdjackey for reporting. +- Security/Archive: block ZIP extraction through pre-existing destination symlinks by validating destination path segments and using no-follow file opens for writes, preventing symlink-pivot writes outside the extraction root. This ships in the next npm release. Thanks @tdjackey for reporting. - WhatsApp/Security: enforce allowlist JID authorization for reaction actions so authenticated callers cannot target non-allowlisted chats by forging `chatJid` + valid `messageId` pairs. Thanks @aether-ai-agent for reporting. - ACP/Security: escape control and delimiter characters in ACP `resource_link` title/URI metadata before prompt interpolation to prevent metadata-driven prompt injection through resource links. Thanks @aether-ai-agent for reporting. - TTS/Security: make model-driven provider switching opt-in by default (`messages.tts.modelOverrides.allowProvider=false` unless explicitly enabled), while keeping voice/style overrides available, to reduce prompt-injection-driven provider hops and unexpected TTS cost escalation. Thanks @aether-ai-agent for reporting. diff --git a/src/infra/archive.test.ts b/src/infra/archive.test.ts index fc9d5f391..434cc266d 100644 --- a/src/infra/archive.test.ts +++ b/src/infra/archive.test.ts @@ -79,6 +79,32 @@ describe("archive utils", () => { ).rejects.toThrow(/(escapes destination|absolute)/i); }); + it("rejects zip entries that traverse pre-existing destination symlinks", async () => { + const workDir = await makeTempDir(); + const archivePath = path.join(workDir, "bundle.zip"); + const extractDir = path.join(workDir, "extract"); + const outsideDir = path.join(workDir, "outside"); + + await fs.mkdir(extractDir, { recursive: true }); + await fs.mkdir(outsideDir, { recursive: true }); + await fs.symlink(outsideDir, path.join(extractDir, "escape")); + + const zip = new JSZip(); + zip.file("escape/pwn.txt", "owned"); + await fs.writeFile(archivePath, await zip.generateAsync({ type: "nodebuffer" })); + + await expect( + extractArchive({ archivePath, destDir: extractDir, timeoutMs: 5_000 }), + ).rejects.toThrow(/symlink/i); + + const outsideFile = path.join(outsideDir, "pwn.txt"); + const outsideExists = await fs + .stat(outsideFile) + .then(() => true) + .catch(() => false); + expect(outsideExists).toBe(false); + }); + it("extracts tar archives", async () => { const workDir = await makeTempDir(); const archivePath = path.join(workDir, "bundle.tar"); diff --git a/src/infra/archive.ts b/src/infra/archive.ts index 46d060598..7d3d90457 100644 --- a/src/infra/archive.ts +++ b/src/infra/archive.ts @@ -1,4 +1,4 @@ -import { createWriteStream } from "node:fs"; +import { constants as fsConstants } from "node:fs"; import fs from "node:fs/promises"; import path from "node:path"; import { Readable, Transform } from "node:stream"; @@ -46,8 +46,14 @@ const ERROR_ARCHIVE_ENTRY_COUNT_EXCEEDS_LIMIT = "archive entry count exceeds lim const ERROR_ARCHIVE_ENTRY_EXTRACTED_SIZE_EXCEEDS_LIMIT = "archive entry extracted size exceeds limit"; const ERROR_ARCHIVE_EXTRACTED_SIZE_EXCEEDS_LIMIT = "archive extracted size exceeds limit"; +const ERROR_ARCHIVE_ENTRY_TRAVERSES_SYMLINK = "archive entry traverses symlink in destination"; const TAR_SUFFIXES = [".tgz", ".tar.gz", ".tar"]; +const OPEN_WRITE_FLAGS = + fsConstants.O_WRONLY | + fsConstants.O_CREAT | + fsConstants.O_TRUNC | + (process.platform !== "win32" && "O_NOFOLLOW" in fsConstants ? fsConstants.O_NOFOLLOW : 0); export function resolveArchiveKind(filePath: string): ArchiveKind | null { const lower = filePath.toLowerCase(); @@ -190,6 +196,112 @@ function createExtractBudgetTransform(params: { }); } +function isNodeError(value: unknown): value is NodeJS.ErrnoException { + return Boolean( + value && typeof value === "object" && "code" in (value as Record), + ); +} + +function isNotFoundError(value: unknown): boolean { + return isNodeError(value) && (value.code === "ENOENT" || value.code === "ENOTDIR"); +} + +function isSymlinkOpenError(value: unknown): boolean { + return ( + isNodeError(value) && + (value.code === "ELOOP" || value.code === "EINVAL" || value.code === "ENOTSUP") + ); +} + +function symlinkTraversalError(originalPath: string): Error { + return new Error(`${ERROR_ARCHIVE_ENTRY_TRAVERSES_SYMLINK}: ${originalPath}`); +} + +async function assertDestinationDirReady(destDir: string): Promise { + const stat = await fs.lstat(destDir); + if (stat.isSymbolicLink()) { + throw new Error("archive destination is a symlink"); + } + if (!stat.isDirectory()) { + throw new Error("archive destination is not a directory"); + } + return await fs.realpath(destDir); +} + +function pathInside(root: string, target: string): boolean { + const rel = path.relative(root, target); + return rel === "" || (!rel.startsWith("..") && !path.isAbsolute(rel)); +} + +async function assertNoSymlinkTraversal(params: { + rootDir: string; + relPath: string; + originalPath: string; +}): Promise { + const parts = params.relPath.split("/").filter(Boolean); + let current = path.resolve(params.rootDir); + for (const part of parts) { + current = path.join(current, part); + let stat: Awaited>; + try { + stat = await fs.lstat(current); + } catch (err) { + if (isNotFoundError(err)) { + continue; + } + throw err; + } + if (stat.isSymbolicLink()) { + throw symlinkTraversalError(params.originalPath); + } + } +} + +async function assertResolvedInsideDestination(params: { + destinationRealDir: string; + targetPath: string; + originalPath: string; +}): Promise { + let resolved: string; + try { + resolved = await fs.realpath(params.targetPath); + } catch (err) { + if (isNotFoundError(err)) { + return; + } + throw err; + } + if (!pathInside(params.destinationRealDir, resolved)) { + throw symlinkTraversalError(params.originalPath); + } +} + +async function openZipOutputFile(outPath: string, originalPath: string) { + try { + return await fs.open(outPath, OPEN_WRITE_FLAGS, 0o666); + } catch (err) { + if (isSymlinkOpenError(err)) { + throw symlinkTraversalError(originalPath); + } + throw err; + } +} + +async function cleanupPartialRegularFile(filePath: string): Promise { + let stat: Awaited>; + try { + stat = await fs.lstat(filePath); + } catch (err) { + if (isNotFoundError(err)) { + return; + } + throw err; + } + if (stat.isFile()) { + await fs.unlink(filePath).catch(() => undefined); + } +} + type ZipEntry = { name: string; dir: boolean; @@ -214,6 +326,7 @@ async function extractZip(params: { limits?: ArchiveExtractLimits; }): Promise { const limits = resolveExtractLimits(params.limits); + const destinationRealDir = await assertDestinationDirReady(params.destDir); const stat = await fs.stat(params.archivePath); if (stat.size > limits.maxArchiveBytes) { throw new Error(ERROR_ARCHIVE_SIZE_EXCEEDS_LIMIT); @@ -242,23 +355,40 @@ async function extractZip(params: { relPath, originalPath: entry.name, }); + await assertNoSymlinkTraversal({ + rootDir: params.destDir, + relPath, + originalPath: entry.name, + }); if (entry.dir) { await fs.mkdir(outPath, { recursive: true }); + await assertResolvedInsideDestination({ + destinationRealDir, + targetPath: outPath, + originalPath: entry.name, + }); continue; } await fs.mkdir(path.dirname(outPath), { recursive: true }); + await assertResolvedInsideDestination({ + destinationRealDir, + targetPath: path.dirname(outPath), + originalPath: entry.name, + }); + const handle = await openZipOutputFile(outPath, entry.name); budget.startEntry(); const readable = await readZipEntryStream(entry); + const writable = handle.createWriteStream(); try { await pipeline( readable, createExtractBudgetTransform({ onChunkBytes: budget.addBytes }), - createWriteStream(outPath), + writable, ); } catch (err) { - await fs.unlink(outPath).catch(() => undefined); + await cleanupPartialRegularFile(outPath).catch(() => undefined); throw err; }