diff --git a/CHANGELOG.md b/CHANGELOG.md index 2254328bd5c..1017e5f5aa0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,7 @@ Docs: https://docs.openclaw.ai - macOS/LaunchAgent install: tighten LaunchAgent directory and plist permissions during install so launchd bootstrap does not fail when the target home path or generated plist inherited group/world-writable modes. - Gateway/Control UI: keep dashboard auth tokens in session-scoped browser storage so same-tab refreshes preserve remote token auth without restoring long-lived localStorage token persistence, while scoping tokens to the selected gateway URL and fragment-only bootstrap flow. (#40892) thanks @velvet-shark. - Secret files: harden CLI and channel credential file reads against path-swap races by requiring direct regular files for `*File` secret inputs and rejecting symlink-backed secret files. +- Archive extraction: harden TAR and external `tar.bz2` installs against destination symlink and pre-existing child-symlink escapes by extracting into staging first and merging into the canonical destination with safe file opens. - Models/Kimi Coding: send `anthropic-messages` tools in native Anthropic format again so `kimi-coding` stops degrading tool calls into XML/plain-text pseudo invocations instead of real `tool_use` blocks. (#38669, #39907, #40552) Thanks @opriz. - Context engine/tests: add bundled-registry regression coverage for cross-chunk resolution, plugin-sdk re-exports, and concurrent chunk registration. (#40460) thanks @dsantoreis. - Agents/embedded runner: bound compaction retry waiting and drain embedded runs during SIGUSR1 restart so session lanes recover instead of staying blocked behind compaction. (#40324) thanks @cgdusek. diff --git a/src/agents/skills-install-extract.ts b/src/agents/skills-install-extract.ts index 4578935378f..faa0fe27fc5 100644 --- a/src/agents/skills-install-extract.ts +++ b/src/agents/skills-install-extract.ts @@ -3,6 +3,9 @@ import fs from "node:fs"; import { createTarEntrySafetyChecker, extractArchive as extractArchiveSafe, + mergeExtractedTreeIntoDestination, + prepareArchiveDestinationDir, + withStagedArchiveDestination, } from "../infra/archive.js"; import { runCommandWithTimeout } from "../process/exec.js"; import { parseTarVerboseMetadata } from "./skills-install-tar-verbose.js"; @@ -66,6 +69,7 @@ export async function extractArchive(params: { return { stdout: "", stderr: "tar not found on PATH", code: null }; } + const destinationRealDir = await prepareArchiveDestinationDir(targetDir); const preflightHash = await hashFileSha256(archivePath); // Preflight list to prevent zip-slip style traversal before extraction. @@ -99,7 +103,7 @@ export async function extractArchive(params: { }; } const checkTarEntrySafety = createTarEntrySafetyChecker({ - rootDir: targetDir, + rootDir: destinationRealDir, stripComponents: strip, escapeLabel: "targetDir", }); @@ -129,11 +133,25 @@ export async function extractArchive(params: { }; } - const argv = ["tar", "xf", archivePath, "-C", targetDir]; - if (strip > 0) { - argv.push("--strip-components", String(strip)); - } - return await runCommandWithTimeout(argv, { timeoutMs }); + return await withStagedArchiveDestination({ + destinationRealDir, + run: async (stagingDir) => { + const argv = ["tar", "xf", archivePath, "-C", stagingDir]; + if (strip > 0) { + argv.push("--strip-components", String(strip)); + } + const extractResult = await runCommandWithTimeout(argv, { timeoutMs }); + if (extractResult.code !== 0) { + return extractResult; + } + await mergeExtractedTreeIntoDestination({ + sourceDir: stagingDir, + destinationDir: destinationRealDir, + destinationRealDir, + }); + return extractResult; + }, + }); } return { stdout: "", stderr: `unsupported archive type: ${archiveType}`, code: null }; diff --git a/src/agents/skills-install.download.test.ts b/src/agents/skills-install.download.test.ts index 0c357089678..cee0d37b876 100644 --- a/src/agents/skills-install.download.test.ts +++ b/src/agents/skills-install.download.test.ts @@ -425,4 +425,47 @@ describe("installDownloadSpec extraction safety (tar.bz2)", () => { .some((call) => (call[0] as string[])[1] === "xf"); expect(extractionAttempted).toBe(false); }); + + it("rejects tar.bz2 entries that traverse pre-existing targetDir symlinks", async () => { + const entry = buildEntry("tbz2-targetdir-symlink"); + const targetDir = path.join(resolveSkillToolsRootDir(entry), "target"); + const outsideDir = path.join(workspaceDir, "tbz2-targetdir-outside"); + await fs.mkdir(targetDir, { recursive: true }); + await fs.mkdir(outsideDir, { recursive: true }); + await fs.symlink( + outsideDir, + path.join(targetDir, "escape"), + process.platform === "win32" ? "junction" : undefined, + ); + + mockArchiveResponse(new Uint8Array([1, 2, 3])); + + runCommandWithTimeoutMock.mockImplementation(async (...argv: unknown[]) => { + const cmd = (argv[0] ?? []) as string[]; + if (cmd[0] === "tar" && cmd[1] === "tf") { + return runCommandResult({ stdout: "escape/pwn.txt\n" }); + } + if (cmd[0] === "tar" && cmd[1] === "tvf") { + return runCommandResult({ stdout: "-rw-r--r-- 0 0 0 0 Jan 1 00:00 escape/pwn.txt\n" }); + } + if (cmd[0] === "tar" && cmd[1] === "xf") { + const stagingDir = String(cmd[cmd.indexOf("-C") + 1] ?? ""); + await fs.mkdir(path.join(stagingDir, "escape"), { recursive: true }); + await fs.writeFile(path.join(stagingDir, "escape", "pwn.txt"), "owned"); + return runCommandResult({ stdout: "ok" }); + } + return runCommandResult(); + }); + + const result = await installDownloadSkill({ + name: "tbz2-targetdir-symlink", + url: "https://example.invalid/evil.tbz2", + archive: "tar.bz2", + targetDir, + }); + + expect(result.ok).toBe(false); + expect(result.stderr.toLowerCase()).toContain("archive entry traverses symlink in destination"); + expect(await fileExists(path.join(outsideDir, "pwn.txt"))).toBe(false); + }); }); diff --git a/src/infra/archive.test.ts b/src/infra/archive.test.ts index 175d68a48e3..82ae804f50c 100644 --- a/src/infra/archive.test.ts +++ b/src/infra/archive.test.ts @@ -105,6 +105,39 @@ describe("archive utils", () => { }, ); + it.each([{ ext: "zip" as const }, { ext: "tar" as const }])( + "rejects $ext extraction when destination dir is a symlink", + async ({ ext }) => { + await withArchiveCase(ext, async ({ workDir, archivePath, extractDir }) => { + const realExtractDir = path.join(workDir, "real-extract"); + await fs.mkdir(realExtractDir, { recursive: true }); + await writePackageArchive({ + ext, + workDir, + archivePath, + fileName: "hello.txt", + content: "hi", + }); + await fs.rm(extractDir, { recursive: true, force: true }); + await fs.symlink( + realExtractDir, + extractDir, + process.platform === "win32" ? "junction" : undefined, + ); + + await expect( + extractArchive({ archivePath, destDir: extractDir, timeoutMs: 5_000 }), + ).rejects.toMatchObject({ + code: "destination-symlink", + } satisfies Partial); + + await expect( + fs.stat(path.join(realExtractDir, "package", "hello.txt")), + ).rejects.toMatchObject({ code: "ENOENT" }); + }); + }, + ); + it("rejects zip path traversal (zip slip)", async () => { await withArchiveCase("zip", async ({ archivePath, extractDir }) => { const zip = new JSZip(); @@ -233,6 +266,32 @@ describe("archive utils", () => { }); }); + it("rejects tar entries that traverse pre-existing destination symlinks", async () => { + await withArchiveCase("tar", async ({ workDir, archivePath, extractDir }) => { + const outsideDir = path.join(workDir, "outside"); + const archiveRoot = path.join(workDir, "archive-root"); + await fs.mkdir(outsideDir, { recursive: true }); + await fs.mkdir(path.join(archiveRoot, "escape"), { recursive: true }); + await fs.writeFile(path.join(archiveRoot, "escape", "pwn.txt"), "owned"); + await fs.symlink( + outsideDir, + path.join(extractDir, "escape"), + process.platform === "win32" ? "junction" : undefined, + ); + await tar.c({ cwd: archiveRoot, file: archivePath }, ["escape"]); + + await expect( + extractArchive({ archivePath, destDir: extractDir, timeoutMs: 5_000 }), + ).rejects.toMatchObject({ + code: "destination-symlink-traversal", + } satisfies Partial); + + await expect(fs.stat(path.join(outsideDir, "pwn.txt"))).rejects.toMatchObject({ + code: "ENOENT", + }); + }); + }); + it.each([{ ext: "zip" as const }, { ext: "tar" as const }])( "rejects $ext archives that exceed extracted size budget", async ({ ext }) => { diff --git a/src/infra/archive.ts b/src/infra/archive.ts index 694560b4d31..313cdbab439 100644 --- a/src/infra/archive.ts +++ b/src/infra/archive.ts @@ -14,7 +14,12 @@ import { validateArchiveEntryPath, } from "./archive-path.js"; import { sameFileIdentity } from "./file-identity.js"; -import { openFileWithinRoot, openWritableFileWithinRoot, SafeOpenError } from "./fs-safe.js"; +import { + copyFileWithinRoot, + openFileWithinRoot, + openWritableFileWithinRoot, + SafeOpenError, +} from "./fs-safe.js"; import { isNotFoundPathError, isPathInside } from "./path-guards.js"; export type ArchiveKind = "tar" | "zip"; @@ -224,7 +229,7 @@ function symlinkTraversalError(originalPath: string): ArchiveSecurityError { ); } -async function assertDestinationDirReady(destDir: string): Promise { +export async function prepareArchiveDestinationDir(destDir: string): Promise { const stat = await fs.lstat(destDir); if (stat.isSymbolicLink()) { throw new ArchiveSecurityError("destination-symlink", "archive destination is a symlink"); @@ -243,7 +248,7 @@ async function assertNoSymlinkTraversal(params: { relPath: string; originalPath: string; }): Promise { - const parts = params.relPath.split("/").filter(Boolean); + const parts = params.relPath.split(/[\\/]+/).filter(Boolean); let current = path.resolve(params.rootDir); for (const part of parts) { current = path.join(current, part); @@ -281,6 +286,135 @@ async function assertResolvedInsideDestination(params: { } } +async function prepareArchiveOutputPath(params: { + destinationDir: string; + destinationRealDir: string; + relPath: string; + outPath: string; + originalPath: string; + isDirectory: boolean; +}): Promise { + await assertNoSymlinkTraversal({ + rootDir: params.destinationDir, + relPath: params.relPath, + originalPath: params.originalPath, + }); + + if (params.isDirectory) { + await fs.mkdir(params.outPath, { recursive: true }); + await assertResolvedInsideDestination({ + destinationRealDir: params.destinationRealDir, + targetPath: params.outPath, + originalPath: params.originalPath, + }); + return; + } + + const parentDir = path.dirname(params.outPath); + await fs.mkdir(parentDir, { recursive: true }); + await assertResolvedInsideDestination({ + destinationRealDir: params.destinationRealDir, + targetPath: parentDir, + originalPath: params.originalPath, + }); +} + +async function applyStagedEntryMode(params: { + destinationRealDir: string; + relPath: string; + mode: number; + originalPath: string; +}): Promise { + const destinationPath = path.join(params.destinationRealDir, params.relPath); + await assertResolvedInsideDestination({ + destinationRealDir: params.destinationRealDir, + targetPath: destinationPath, + originalPath: params.originalPath, + }); + if (params.mode !== 0) { + await fs.chmod(destinationPath, params.mode).catch(() => undefined); + } +} + +export async function withStagedArchiveDestination(params: { + destinationRealDir: string; + run: (stagingDir: string) => Promise; +}): Promise { + const stagingDir = await fs.mkdtemp(path.join(params.destinationRealDir, ".openclaw-archive-")); + try { + return await params.run(stagingDir); + } finally { + await fs.rm(stagingDir, { recursive: true, force: true }).catch(() => undefined); + } +} + +export async function mergeExtractedTreeIntoDestination(params: { + sourceDir: string; + destinationDir: string; + destinationRealDir: string; +}): Promise { + const walk = async (currentSourceDir: string): Promise => { + const entries = await fs.readdir(currentSourceDir, { withFileTypes: true }); + for (const entry of entries) { + const sourcePath = path.join(currentSourceDir, entry.name); + const relPath = path.relative(params.sourceDir, sourcePath); + const originalPath = relPath.split(path.sep).join("/"); + const destinationPath = path.join(params.destinationDir, relPath); + const sourceStat = await fs.lstat(sourcePath); + + if (sourceStat.isSymbolicLink()) { + throw symlinkTraversalError(originalPath); + } + + if (sourceStat.isDirectory()) { + await prepareArchiveOutputPath({ + destinationDir: params.destinationDir, + destinationRealDir: params.destinationRealDir, + relPath, + outPath: destinationPath, + originalPath, + isDirectory: true, + }); + await walk(sourcePath); + await applyStagedEntryMode({ + destinationRealDir: params.destinationRealDir, + relPath, + mode: sourceStat.mode & 0o777, + originalPath, + }); + continue; + } + + if (!sourceStat.isFile()) { + throw new Error(`archive staging contains unsupported entry: ${originalPath}`); + } + + await prepareArchiveOutputPath({ + destinationDir: params.destinationDir, + destinationRealDir: params.destinationRealDir, + relPath, + outPath: destinationPath, + originalPath, + isDirectory: false, + }); + await copyFileWithinRoot({ + sourcePath, + rootDir: params.destinationRealDir, + relativePath: relPath, + mkdir: true, + }); + await applyStagedEntryMode({ + destinationRealDir: params.destinationRealDir, + relPath, + mode: sourceStat.mode & 0o777, + originalPath, + }); + } + }; + + await walk(params.sourceDir); +} + type OpenZipOutputFileResult = { handle: FileHandle; createdForWrite: boolean; @@ -403,29 +537,7 @@ async function prepareZipOutputPath(params: { originalPath: string; isDirectory: boolean; }): Promise { - await assertNoSymlinkTraversal({ - rootDir: params.destinationDir, - relPath: params.relPath, - originalPath: params.originalPath, - }); - - if (params.isDirectory) { - await fs.mkdir(params.outPath, { recursive: true }); - await assertResolvedInsideDestination({ - destinationRealDir: params.destinationRealDir, - targetPath: params.outPath, - originalPath: params.originalPath, - }); - return; - } - - const parentDir = path.dirname(params.outPath); - await fs.mkdir(parentDir, { recursive: true }); - await assertResolvedInsideDestination({ - destinationRealDir: params.destinationRealDir, - targetPath: parentDir, - originalPath: params.originalPath, - }); + await prepareArchiveOutputPath(params); } async function writeZipFileEntry(params: { @@ -511,7 +623,7 @@ async function extractZip(params: { limits?: ArchiveExtractLimits; }): Promise { const limits = resolveExtractLimits(params.limits); - const destinationRealDir = await assertDestinationDirReady(params.destDir); + const destinationRealDir = await prepareArchiveDestinationDir(params.destDir); const stat = await fs.stat(params.archivePath); if (stat.size > limits.maxArchiveBytes) { throw new Error(ERROR_ARCHIVE_SIZE_EXCEEDS_LIMIT); @@ -641,37 +753,50 @@ export async function extractArchive(params: { const label = kind === "zip" ? "extract zip" : "extract tar"; if (kind === "tar") { - const limits = resolveExtractLimits(params.limits); - const stat = await fs.stat(params.archivePath); - if (stat.size > limits.maxArchiveBytes) { - throw new Error(ERROR_ARCHIVE_SIZE_EXCEEDS_LIMIT); - } - - const checkTarEntrySafety = createTarEntrySafetyChecker({ - rootDir: params.destDir, - stripComponents: params.stripComponents, - limits, - }); await withTimeout( - tar.x({ - file: params.archivePath, - cwd: params.destDir, - strip: Math.max(0, Math.floor(params.stripComponents ?? 0)), - gzip: params.tarGzip, - preservePaths: false, - strict: true, - onReadEntry(entry) { - try { - checkTarEntrySafety(readTarEntryInfo(entry)); - } catch (err) { - const error = err instanceof Error ? err : new Error(String(err)); - // Node's EventEmitter calls listeners with `this` bound to the - // emitter (tar.Unpack), which exposes Parser.abort(). - const emitter = this as unknown as { abort?: (error: Error) => void }; - emitter.abort?.(error); - } - }, - }), + (async () => { + const limits = resolveExtractLimits(params.limits); + const stat = await fs.stat(params.archivePath); + if (stat.size > limits.maxArchiveBytes) { + throw new Error(ERROR_ARCHIVE_SIZE_EXCEEDS_LIMIT); + } + + const destinationRealDir = await prepareArchiveDestinationDir(params.destDir); + await withStagedArchiveDestination({ + destinationRealDir, + run: async (stagingDir) => { + const checkTarEntrySafety = createTarEntrySafetyChecker({ + rootDir: destinationRealDir, + stripComponents: params.stripComponents, + limits, + }); + await tar.x({ + file: params.archivePath, + cwd: stagingDir, + strip: Math.max(0, Math.floor(params.stripComponents ?? 0)), + gzip: params.tarGzip, + preservePaths: false, + strict: true, + onReadEntry(entry) { + try { + checkTarEntrySafety(readTarEntryInfo(entry)); + } catch (err) { + const error = err instanceof Error ? err : new Error(String(err)); + // Node's EventEmitter calls listeners with `this` bound to the + // emitter (tar.Unpack), which exposes Parser.abort(). + const emitter = this as unknown as { abort?: (error: Error) => void }; + emitter.abort?.(error); + } + }, + }); + await mergeExtractedTreeIntoDestination({ + sourceDir: stagingDir, + destinationDir: destinationRealDir, + destinationRealDir, + }); + }, + }); + })(), params.timeoutMs, label, );