diff --git a/CHANGELOG.md b/CHANGELOG.md index f3d9de2d885..bd443141b40 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,8 @@ Docs: https://docs.openclaw.ai ### Fixes +- Security/Skills: harden archive extraction for download-installed skills to prevent path traversal outside the target directory. Thanks @markmusson. +- Security/Signal: harden signal-cli archive extraction during install to prevent path traversal outside the install root. - Security/Hooks: restrict hook transform modules to `~/.openclaw/hooks/transforms` (prevents path traversal/escape module loads via config). Config note: `hooks.transformsDir` must now be within that directory. Thanks @akhmittra. - Security/Hooks: ignore hook package manifest entries that point outside the package directory (prevents out-of-tree handler loads during hook discovery). - Ollama/Agents: avoid forcing `` tag enforcement for Ollama models, which could suppress all output as `(no output)`. (#16191) Thanks @Glucksberg. @@ -85,6 +87,8 @@ Docs: https://docs.openclaw.ai - Security/Gateway: breaking default-behavior change - canvas IP-based auth fallback now only accepts machine-scoped addresses (RFC1918, link-local, ULA IPv6, CGNAT); public-source IP matches now require bearer token auth. (#14661) Thanks @sumleo. - Security/Link understanding: block loopback/internal host patterns and private/mapped IPv6 addresses in extracted URL handling to close SSRF bypasses in link CLI flows. (#15604) Thanks @AI-Reviewer-QS. - Security/Browser: constrain `POST /trace/stop`, `POST /wait/download`, and `POST /download` output paths to OpenClaw temp roots and reject traversal/escape paths. +- Security/Browser: sanitize download `suggestedFilename` to keep implicit `wait/download` paths within the downloads root. Thanks @1seal. +- Security/Browser: confine `POST /hooks/file-chooser` upload paths to an OpenClaw temp uploads root and reject traversal/escape paths. Thanks @1seal. - Security/Browser: require auth for the sandbox browser bridge server (protects `/profiles`, `/tabs`, CDP URLs, and other control endpoints). Thanks @jackhax. - Security: bind local helper servers to loopback and fail closed on non-loopback OAuth callback hosts (reduces localhost/LAN attack surface). - Security/Canvas: serve A2UI assets via the shared safe-open path (`openFileWithinRoot`) to close traversal/TOCTOU gaps, with traversal and symlink regression coverage. (#10525) Thanks @abdelsfane. diff --git a/docs/tools/browser.md b/docs/tools/browser.md index 107c92b9911..74f42472439 100644 --- a/docs/tools/browser.md +++ b/docs/tools/browser.md @@ -411,7 +411,7 @@ Actions: - `openclaw browser select 9 OptionA OptionB` - `openclaw browser download e12 report.pdf` - `openclaw browser waitfordownload report.pdf` -- `openclaw browser upload /tmp/file.pdf` +- `openclaw browser upload /tmp/openclaw/uploads/file.pdf` - `openclaw browser fill --fields '[{"ref":"1","type":"text","value":"Ada"}]'` - `openclaw browser dialog --accept` - `openclaw browser wait --text "Done"` @@ -447,6 +447,8 @@ Notes: - Download and trace output paths are constrained to OpenClaw temp roots: - traces: `/tmp/openclaw` (fallback: `${os.tmpdir()}/openclaw`) - downloads: `/tmp/openclaw/downloads` (fallback: `${os.tmpdir()}/openclaw/downloads`) +- Upload paths are constrained to an OpenClaw temp uploads root: + - uploads: `/tmp/openclaw/uploads` (fallback: `${os.tmpdir()}/openclaw/uploads`) - `upload` can also set file inputs directly via `--input-ref` or `--element`. - `snapshot`: - `--format ai` (default when Playwright is installed): returns an AI snapshot with numeric refs (`aria-ref=""`). diff --git a/src/agents/skills-install.e2e.test.ts b/src/agents/skills-install.e2e.test.ts index 696b03e828b..eeb64121b20 100644 --- a/src/agents/skills-install.e2e.test.ts +++ b/src/agents/skills-install.e2e.test.ts @@ -1,16 +1,23 @@ +import JSZip from "jszip"; import fs from "node:fs/promises"; import os from "node:os"; import path from "node:path"; +import * as tar from "tar"; import { beforeEach, describe, expect, it, vi } from "vitest"; import { installSkill } from "./skills-install.js"; const runCommandWithTimeoutMock = vi.fn(); const scanDirectoryWithSummaryMock = vi.fn(); +const fetchWithSsrFGuardMock = vi.fn(); vi.mock("../process/exec.js", () => ({ runCommandWithTimeout: (...args: unknown[]) => runCommandWithTimeoutMock(...args), })); +vi.mock("../infra/net/fetch-guard.js", () => ({ + fetchWithSsrFGuard: (...args: unknown[]) => fetchWithSsrFGuardMock(...args), +})); + vi.mock("../security/skill-scanner.js", async (importOriginal) => { const actual = await importOriginal(); return { @@ -38,10 +45,62 @@ metadata: {"openclaw":{"install":[{"id":"deps","kind":"node","package":"example- return skillDir; } +async function writeDownloadSkill(params: { + workspaceDir: string; + name: string; + installId: string; + url: string; + archive: "tar.gz" | "tar.bz2" | "zip"; + stripComponents?: number; + targetDir: string; +}): Promise { + const skillDir = path.join(params.workspaceDir, "skills", params.name); + await fs.mkdir(skillDir, { recursive: true }); + const meta = { + openclaw: { + install: [ + { + id: params.installId, + kind: "download", + url: params.url, + archive: params.archive, + extract: true, + stripComponents: params.stripComponents, + targetDir: params.targetDir, + }, + ], + }, + }; + await fs.writeFile( + path.join(skillDir, "SKILL.md"), + `--- +name: ${params.name} +description: test skill +metadata: ${JSON.stringify(meta)} +--- + +# ${params.name} +`, + "utf-8", + ); + await fs.writeFile(path.join(skillDir, "runner.js"), "export {};\n", "utf-8"); + return skillDir; +} + +async function fileExists(filePath: string): Promise { + try { + await fs.stat(filePath); + return true; + } catch { + return false; + } +} + describe("installSkill code safety scanning", () => { beforeEach(() => { runCommandWithTimeoutMock.mockReset(); scanDirectoryWithSummaryMock.mockReset(); + fetchWithSsrFGuardMock.mockReset(); runCommandWithTimeoutMock.mockResolvedValue({ code: 0, stdout: "ok", @@ -112,3 +171,346 @@ describe("installSkill code safety scanning", () => { } }); }); + +describe("installSkill download extraction safety", () => { + beforeEach(() => { + runCommandWithTimeoutMock.mockReset(); + scanDirectoryWithSummaryMock.mockReset(); + fetchWithSsrFGuardMock.mockReset(); + scanDirectoryWithSummaryMock.mockResolvedValue({ + scannedFiles: 0, + critical: 0, + warn: 0, + info: 0, + findings: [], + }); + }); + + it("rejects zip slip traversal", async () => { + const workspaceDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-skills-install-")); + try { + const targetDir = path.join(workspaceDir, "target"); + const outsideWriteDir = path.join(workspaceDir, "outside-write"); + const outsideWritePath = path.join(outsideWriteDir, "pwned.txt"); + const url = "https://example.invalid/evil.zip"; + + const zip = new JSZip(); + zip.file("../outside-write/pwned.txt", "pwnd"); + const buffer = await zip.generateAsync({ type: "nodebuffer" }); + + fetchWithSsrFGuardMock.mockResolvedValue({ + response: new Response(buffer, { status: 200 }), + release: async () => undefined, + }); + + await writeDownloadSkill({ + workspaceDir, + name: "zip-slip", + installId: "dl", + url, + archive: "zip", + targetDir, + }); + + const result = await installSkill({ workspaceDir, skillName: "zip-slip", installId: "dl" }); + expect(result.ok).toBe(false); + expect(await fileExists(outsideWritePath)).toBe(false); + } finally { + await fs.rm(workspaceDir, { recursive: true, force: true }).catch(() => undefined); + } + }); + + it("rejects tar.gz traversal", async () => { + const workspaceDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-skills-install-")); + try { + const targetDir = path.join(workspaceDir, "target"); + const insideDir = path.join(workspaceDir, "inside"); + const outsideWriteDir = path.join(workspaceDir, "outside-write"); + const outsideWritePath = path.join(outsideWriteDir, "pwned.txt"); + const archivePath = path.join(workspaceDir, "evil.tgz"); + const url = "https://example.invalid/evil"; + + await fs.mkdir(insideDir, { recursive: true }); + await fs.mkdir(outsideWriteDir, { recursive: true }); + await fs.writeFile(outsideWritePath, "pwnd", "utf-8"); + + await tar.c({ cwd: insideDir, file: archivePath, gzip: true }, [ + "../outside-write/pwned.txt", + ]); + await fs.rm(outsideWriteDir, { recursive: true, force: true }); + + const buffer = await fs.readFile(archivePath); + fetchWithSsrFGuardMock.mockResolvedValue({ + response: new Response(buffer, { status: 200 }), + release: async () => undefined, + }); + + await writeDownloadSkill({ + workspaceDir, + name: "tar-slip", + installId: "dl", + url, + archive: "tar.gz", + targetDir, + }); + + const result = await installSkill({ workspaceDir, skillName: "tar-slip", installId: "dl" }); + expect(result.ok).toBe(false); + expect(await fileExists(outsideWritePath)).toBe(false); + } finally { + await fs.rm(workspaceDir, { recursive: true, force: true }).catch(() => undefined); + } + }); + + it("extracts zip with stripComponents safely", async () => { + const workspaceDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-skills-install-")); + try { + const targetDir = path.join(workspaceDir, "target"); + const url = "https://example.invalid/good.zip"; + + const zip = new JSZip(); + zip.file("package/hello.txt", "hi"); + const buffer = await zip.generateAsync({ type: "nodebuffer" }); + fetchWithSsrFGuardMock.mockResolvedValue({ + response: new Response(buffer, { status: 200 }), + release: async () => undefined, + }); + + await writeDownloadSkill({ + workspaceDir, + name: "zip-good", + installId: "dl", + url, + archive: "zip", + stripComponents: 1, + targetDir, + }); + + const result = await installSkill({ workspaceDir, skillName: "zip-good", installId: "dl" }); + expect(result.ok).toBe(true); + expect(await fs.readFile(path.join(targetDir, "hello.txt"), "utf-8")).toBe("hi"); + } finally { + await fs.rm(workspaceDir, { recursive: true, force: true }).catch(() => undefined); + } + }); + + it("rejects tar.bz2 traversal before extraction", async () => { + const workspaceDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-skills-install-")); + try { + const targetDir = path.join(workspaceDir, "target"); + const url = "https://example.invalid/evil.tbz2"; + + fetchWithSsrFGuardMock.mockResolvedValue({ + response: new Response(new Uint8Array([1, 2, 3]), { status: 200 }), + release: async () => undefined, + }); + + runCommandWithTimeoutMock.mockImplementation(async (argv: unknown[]) => { + const cmd = argv as string[]; + if (cmd[0] === "tar" && cmd[1] === "tf") { + return { code: 0, stdout: "../outside.txt\n", stderr: "", signal: null, killed: false }; + } + if (cmd[0] === "tar" && cmd[1] === "tvf") { + return { + code: 0, + stdout: "-rw-r--r-- 0 0 0 0 Jan 1 00:00 ../outside.txt\n", + stderr: "", + signal: null, + killed: false, + }; + } + if (cmd[0] === "tar" && cmd[1] === "xf") { + throw new Error("should not extract"); + } + return { code: 0, stdout: "", stderr: "", signal: null, killed: false }; + }); + + await writeDownloadSkill({ + workspaceDir, + name: "tbz2-slip", + installId: "dl", + url, + archive: "tar.bz2", + targetDir, + }); + + const result = await installSkill({ workspaceDir, skillName: "tbz2-slip", installId: "dl" }); + expect(result.ok).toBe(false); + expect( + runCommandWithTimeoutMock.mock.calls.some((call) => (call[0] as string[])[1] === "xf"), + ).toBe(false); + } finally { + await fs.rm(workspaceDir, { recursive: true, force: true }).catch(() => undefined); + } + }); + + it("rejects tar.bz2 archives containing symlinks", async () => { + const workspaceDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-skills-install-")); + try { + const targetDir = path.join(workspaceDir, "target"); + const url = "https://example.invalid/evil.tbz2"; + + fetchWithSsrFGuardMock.mockResolvedValue({ + response: new Response(new Uint8Array([1, 2, 3]), { status: 200 }), + release: async () => undefined, + }); + + runCommandWithTimeoutMock.mockImplementation(async (argv: unknown[]) => { + const cmd = argv as string[]; + if (cmd[0] === "tar" && cmd[1] === "tf") { + return { + code: 0, + stdout: "link\nlink/pwned.txt\n", + stderr: "", + signal: null, + killed: false, + }; + } + if (cmd[0] === "tar" && cmd[1] === "tvf") { + return { + code: 0, + stdout: "lrwxr-xr-x 0 0 0 0 Jan 1 00:00 link -> ../outside\n", + stderr: "", + signal: null, + killed: false, + }; + } + if (cmd[0] === "tar" && cmd[1] === "xf") { + throw new Error("should not extract"); + } + return { code: 0, stdout: "", stderr: "", signal: null, killed: false }; + }); + + await writeDownloadSkill({ + workspaceDir, + name: "tbz2-symlink", + installId: "dl", + url, + archive: "tar.bz2", + targetDir, + }); + + const result = await installSkill({ + workspaceDir, + skillName: "tbz2-symlink", + installId: "dl", + }); + expect(result.ok).toBe(false); + expect(result.stderr.toLowerCase()).toContain("link"); + } finally { + await fs.rm(workspaceDir, { recursive: true, force: true }).catch(() => undefined); + } + }); + + it("extracts tar.bz2 with stripComponents safely (preflight only)", async () => { + const workspaceDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-skills-install-")); + try { + const targetDir = path.join(workspaceDir, "target"); + const url = "https://example.invalid/good.tbz2"; + + fetchWithSsrFGuardMock.mockResolvedValue({ + response: new Response(new Uint8Array([1, 2, 3]), { status: 200 }), + release: async () => undefined, + }); + + runCommandWithTimeoutMock.mockImplementation(async (argv: unknown[]) => { + const cmd = argv as string[]; + if (cmd[0] === "tar" && cmd[1] === "tf") { + return { + code: 0, + stdout: "package/hello.txt\n", + stderr: "", + signal: null, + killed: false, + }; + } + if (cmd[0] === "tar" && cmd[1] === "tvf") { + return { + code: 0, + stdout: "-rw-r--r-- 0 0 0 0 Jan 1 00:00 package/hello.txt\n", + stderr: "", + signal: null, + killed: false, + }; + } + if (cmd[0] === "tar" && cmd[1] === "xf") { + return { code: 0, stdout: "ok", stderr: "", signal: null, killed: false }; + } + return { code: 0, stdout: "", stderr: "", signal: null, killed: false }; + }); + + await writeDownloadSkill({ + workspaceDir, + name: "tbz2-ok", + installId: "dl", + url, + archive: "tar.bz2", + stripComponents: 1, + targetDir, + }); + + const result = await installSkill({ workspaceDir, skillName: "tbz2-ok", installId: "dl" }); + expect(result.ok).toBe(true); + expect( + runCommandWithTimeoutMock.mock.calls.some((call) => (call[0] as string[])[1] === "xf"), + ).toBe(true); + } finally { + await fs.rm(workspaceDir, { recursive: true, force: true }).catch(() => undefined); + } + }); + + it("rejects tar.bz2 stripComponents escape", async () => { + const workspaceDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-skills-install-")); + try { + const targetDir = path.join(workspaceDir, "target"); + const url = "https://example.invalid/evil.tbz2"; + + fetchWithSsrFGuardMock.mockResolvedValue({ + response: new Response(new Uint8Array([1, 2, 3]), { status: 200 }), + release: async () => undefined, + }); + + runCommandWithTimeoutMock.mockImplementation(async (argv: unknown[]) => { + const cmd = argv as string[]; + if (cmd[0] === "tar" && cmd[1] === "tf") { + return { code: 0, stdout: "a/../b.txt\n", stderr: "", signal: null, killed: false }; + } + if (cmd[0] === "tar" && cmd[1] === "tvf") { + return { + code: 0, + stdout: "-rw-r--r-- 0 0 0 0 Jan 1 00:00 a/../b.txt\n", + stderr: "", + signal: null, + killed: false, + }; + } + if (cmd[0] === "tar" && cmd[1] === "xf") { + throw new Error("should not extract"); + } + return { code: 0, stdout: "", stderr: "", signal: null, killed: false }; + }); + + await writeDownloadSkill({ + workspaceDir, + name: "tbz2-strip-escape", + installId: "dl", + url, + archive: "tar.bz2", + stripComponents: 1, + targetDir, + }); + + const result = await installSkill({ + workspaceDir, + skillName: "tbz2-strip-escape", + installId: "dl", + }); + expect(result.ok).toBe(false); + expect( + runCommandWithTimeoutMock.mock.calls.some((call) => (call[0] as string[])[1] === "xf"), + ).toBe(false); + } finally { + await fs.rm(workspaceDir, { recursive: true, force: true }).catch(() => undefined); + } + }); +}); diff --git a/src/agents/skills-install.ts b/src/agents/skills-install.ts index d1dd5b6bf48..deee4b425f7 100644 --- a/src/agents/skills-install.ts +++ b/src/agents/skills-install.ts @@ -4,6 +4,7 @@ import path from "node:path"; import { Readable } from "node:stream"; import { pipeline } from "node:stream/promises"; import type { OpenClawConfig } from "../config/config.js"; +import { extractArchive as extractArchiveSafe } from "../infra/archive.js"; import { resolveBrewExecutable } from "../infra/brew.js"; import { fetchWithSsrFGuard } from "../infra/net/fetch-guard.js"; import { runCommandWithTimeout } from "../process/exec.js"; @@ -225,6 +226,66 @@ function resolveArchiveType(spec: SkillInstallSpec, filename: string): string | return undefined; } +function normalizeArchiveEntryPath(raw: string): string { + 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(normalizeArchiveEntryPath(entryPath)); + if (normalized === ".." || normalized.startsWith("../")) { + throw new Error(`archive entry escapes targetDir: ${entryPath}`); + } + if (path.posix.isAbsolute(normalized) || normalized.startsWith("//")) { + throw new Error(`archive entry is absolute: ${entryPath}`); + } +} + +function resolveSafeBaseDir(rootDir: string): string { + const resolved = path.resolve(rootDir); + return resolved.endsWith(path.sep) ? resolved : `${resolved}${path.sep}`; +} + +function stripArchivePath(entryPath: string, stripComponents: number): string | null { + const raw = normalizeArchiveEntryPath(entryPath); + if (!raw || raw === "." || raw === "./") { + return null; + } + + // Important: tar's --strip-components semantics operate on raw path segments, + // before any normalization that would collapse "..". We mimic that so we + // can detect strip-induced escapes like "a/../b" with stripComponents=1. + const parts = raw.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 validateExtractedPathWithinRoot(params: { + rootDir: string; + relPath: string; + originalPath: string; +}): void { + const safeBase = resolveSafeBaseDir(params.rootDir); + const outPath = path.resolve(params.rootDir, params.relPath); + if (!outPath.startsWith(safeBase)) { + throw new Error(`archive entry escapes targetDir: ${params.originalPath}`); + } +} + async function downloadFile( url: string, destPath: string, @@ -260,22 +321,99 @@ async function extractArchive(params: { timeoutMs: number; }): Promise<{ stdout: string; stderr: string; code: number | null }> { const { archivePath, archiveType, targetDir, stripComponents, timeoutMs } = params; - if (archiveType === "zip") { - if (!hasBinary("unzip")) { - return { stdout: "", stderr: "unzip not found on PATH", code: null }; - } - const argv = ["unzip", "-q", archivePath, "-d", targetDir]; - return await runCommandWithTimeout(argv, { timeoutMs }); - } + const strip = + typeof stripComponents === "number" && Number.isFinite(stripComponents) + ? Math.max(0, Math.floor(stripComponents)) + : 0; - if (!hasBinary("tar")) { - return { stdout: "", stderr: "tar not found on PATH", code: null }; + try { + if (archiveType === "zip") { + await extractArchiveSafe({ + archivePath, + destDir: targetDir, + timeoutMs, + kind: "zip", + stripComponents: strip, + }); + return { stdout: "", stderr: "", code: 0 }; + } + + if (archiveType === "tar.gz") { + await extractArchiveSafe({ + archivePath, + destDir: targetDir, + timeoutMs, + kind: "tar", + stripComponents: strip, + tarGzip: true, + }); + return { stdout: "", stderr: "", code: 0 }; + } + + if (archiveType === "tar.bz2") { + if (!hasBinary("tar")) { + return { stdout: "", stderr: "tar not found on PATH", code: null }; + } + + // Preflight list to prevent zip-slip style traversal before extraction. + const listResult = await runCommandWithTimeout(["tar", "tf", archivePath], { timeoutMs }); + if (listResult.code !== 0) { + return { + stdout: listResult.stdout, + stderr: listResult.stderr || "tar list failed", + code: listResult.code, + }; + } + const entries = listResult.stdout + .split("\n") + .map((line) => line.trim()) + .filter(Boolean); + + const verboseResult = await runCommandWithTimeout(["tar", "tvf", archivePath], { timeoutMs }); + if (verboseResult.code !== 0) { + return { + stdout: verboseResult.stdout, + stderr: verboseResult.stderr || "tar verbose list failed", + code: verboseResult.code, + }; + } + for (const line of verboseResult.stdout.split("\n")) { + const trimmed = line.trim(); + if (!trimmed) { + continue; + } + const typeChar = trimmed[0]; + if (typeChar === "l" || typeChar === "h" || trimmed.includes(" -> ")) { + return { + stdout: verboseResult.stdout, + stderr: "tar archive contains link entries; refusing to extract for safety", + code: 1, + }; + } + } + + for (const entry of entries) { + validateArchiveEntryPath(entry); + const relPath = stripArchivePath(entry, strip); + if (!relPath) { + continue; + } + validateArchiveEntryPath(relPath); + validateExtractedPathWithinRoot({ rootDir: targetDir, relPath, originalPath: entry }); + } + + const argv = ["tar", "xf", archivePath, "-C", targetDir]; + if (strip > 0) { + argv.push("--strip-components", String(strip)); + } + return await runCommandWithTimeout(argv, { timeoutMs }); + } + + return { stdout: "", stderr: `unsupported archive type: ${archiveType}`, code: null }; + } catch (err) { + const message = err instanceof Error ? err.message : String(err); + return { stdout: "", stderr: message, code: 1 }; } - const argv = ["tar", "xf", archivePath, "-C", targetDir]; - if (typeof stripComponents === "number" && Number.isFinite(stripComponents)) { - argv.push("--strip-components", String(Math.max(0, Math.floor(stripComponents)))); - } - return await runCommandWithTimeout(argv, { timeoutMs }); } async function installDownloadSpec(params: { diff --git a/src/agents/tools/browser-tool.ts b/src/agents/tools/browser-tool.ts index eeb2dae5026..7065ae99336 100644 --- a/src/agents/tools/browser-tool.ts +++ b/src/agents/tools/browser-tool.ts @@ -21,6 +21,7 @@ import { } from "../../browser/client.js"; import { resolveBrowserConfig } from "../../browser/config.js"; import { DEFAULT_AI_SNAPSHOT_MAX_CHARS } from "../../browser/constants.js"; +import { DEFAULT_UPLOAD_DIR, resolvePathsWithinRoot } from "../../browser/paths.js"; import { loadConfig } from "../../config/config.js"; import { saveMediaBuffer } from "../../media/store.js"; import { wrapExternalContent } from "../../security/external-content.js"; @@ -724,6 +725,15 @@ export function createBrowserTool(opts?: { if (paths.length === 0) { throw new Error("paths required"); } + const uploadPathsResult = resolvePathsWithinRoot({ + rootDir: DEFAULT_UPLOAD_DIR, + requestedPaths: paths, + scopeLabel: `uploads directory (${DEFAULT_UPLOAD_DIR})`, + }); + if (!uploadPathsResult.ok) { + throw new Error(uploadPathsResult.error); + } + const normalizedPaths = uploadPathsResult.paths; const ref = readStringParam(params, "ref"); const inputRef = readStringParam(params, "inputRef"); const element = readStringParam(params, "element"); @@ -738,7 +748,7 @@ export function createBrowserTool(opts?: { path: "/hooks/file-chooser", profile, body: { - paths, + paths: normalizedPaths, ref, inputRef, element, @@ -750,7 +760,7 @@ export function createBrowserTool(opts?: { } return jsonResult( await browserArmFileChooser(baseUrl, { - paths, + paths: normalizedPaths, ref, inputRef, element, diff --git a/src/browser/paths.ts b/src/browser/paths.ts new file mode 100644 index 00000000000..5d91c8287b6 --- /dev/null +++ b/src/browser/paths.ts @@ -0,0 +1,49 @@ +import path from "node:path"; +import { resolvePreferredOpenClawTmpDir } from "../infra/tmp-openclaw-dir.js"; + +export const DEFAULT_BROWSER_TMP_DIR = resolvePreferredOpenClawTmpDir(); +export const DEFAULT_TRACE_DIR = DEFAULT_BROWSER_TMP_DIR; +export const DEFAULT_DOWNLOAD_DIR = path.join(DEFAULT_BROWSER_TMP_DIR, "downloads"); +export const DEFAULT_UPLOAD_DIR = path.join(DEFAULT_BROWSER_TMP_DIR, "uploads"); + +export function resolvePathWithinRoot(params: { + rootDir: string; + requestedPath: string; + scopeLabel: string; + defaultFileName?: string; +}): { ok: true; path: string } | { ok: false; error: string } { + const root = path.resolve(params.rootDir); + const raw = params.requestedPath.trim(); + if (!raw) { + if (!params.defaultFileName) { + return { ok: false, error: "path is required" }; + } + return { ok: true, path: path.join(root, params.defaultFileName) }; + } + const resolved = path.resolve(root, raw); + const rel = path.relative(root, resolved); + if (!rel || rel.startsWith("..") || path.isAbsolute(rel)) { + return { ok: false, error: `Invalid path: must stay within ${params.scopeLabel}` }; + } + return { ok: true, path: resolved }; +} + +export function resolvePathsWithinRoot(params: { + rootDir: string; + requestedPaths: string[]; + scopeLabel: string; +}): { ok: true; paths: string[] } | { ok: false; error: string } { + const resolvedPaths: string[] = []; + for (const raw of params.requestedPaths) { + const pathResult = resolvePathWithinRoot({ + rootDir: params.rootDir, + requestedPath: raw, + scopeLabel: params.scopeLabel, + }); + if (!pathResult.ok) { + return { ok: false, error: pathResult.error }; + } + resolvedPaths.push(pathResult.path); + } + return { ok: true, paths: resolvedPaths }; +} diff --git a/src/browser/pw-tools-core.downloads.ts b/src/browser/pw-tools-core.downloads.ts index a2884d4eb71..0a242082927 100644 --- a/src/browser/pw-tools-core.downloads.ts +++ b/src/browser/pw-tools-core.downloads.ts @@ -18,9 +18,38 @@ import { toAIFriendlyError, } from "./pw-tools-core.shared.js"; +function sanitizeDownloadFileName(fileName: string): string { + const trimmed = String(fileName ?? "").trim(); + if (!trimmed) { + return "download.bin"; + } + + // `suggestedFilename()` is untrusted (influenced by remote servers). Force a basename so + // path separators/traversal can't escape the downloads dir on any platform. + let base = path.posix.basename(trimmed); + base = path.win32.basename(base); + let cleaned = ""; + for (let i = 0; i < base.length; i++) { + const code = base.charCodeAt(i); + if (code < 0x20 || code === 0x7f) { + continue; + } + cleaned += base[i]; + } + base = cleaned.trim(); + + if (!base || base === "." || base === "..") { + return "download.bin"; + } + if (base.length > 200) { + base = base.slice(0, 200); + } + return base; +} + function buildTempDownloadPath(fileName: string): string { const id = crypto.randomUUID(); - const safeName = fileName.trim() ? fileName.trim() : "download.bin"; + const safeName = sanitizeDownloadFileName(fileName); return path.join(resolvePreferredOpenClawTmpDir(), "downloads", `${id}-${safeName}`); } diff --git a/src/browser/pw-tools-core.waits-next-download-saves-it.test.ts b/src/browser/pw-tools-core.waits-next-download-saves-it.test.ts index 59d233e0005..71be19a2d13 100644 --- a/src/browser/pw-tools-core.waits-next-download-saves-it.test.ts +++ b/src/browser/pw-tools-core.waits-next-download-saves-it.test.ts @@ -171,6 +171,46 @@ describe("pw-tools-core", () => { expect(path.normalize(res.path)).toContain(path.normalize(expectedDownloadsTail)); expect(tmpDirMocks.resolvePreferredOpenClawTmpDir).toHaveBeenCalled(); }); + + it("sanitizes suggested download filenames to prevent traversal escapes", async () => { + let downloadHandler: ((download: unknown) => void) | undefined; + const on = vi.fn((event: string, handler: (download: unknown) => void) => { + if (event === "download") { + downloadHandler = handler; + } + }); + const off = vi.fn(); + + const saveAs = vi.fn(async () => {}); + const download = { + url: () => "https://example.com/evil", + suggestedFilename: () => "../../../../etc/passwd", + saveAs, + }; + + tmpDirMocks.resolvePreferredOpenClawTmpDir.mockReturnValue("/tmp/openclaw-preferred"); + currentPage = { on, off }; + + const p = mod.waitForDownloadViaPlaywright({ + cdpUrl: "http://127.0.0.1:18792", + targetId: "T1", + timeoutMs: 1000, + }); + + await Promise.resolve(); + downloadHandler?.(download); + + const res = await p; + const outPath = vi.mocked(saveAs).mock.calls[0]?.[0]; + expect(typeof outPath).toBe("string"); + expect(path.dirname(String(outPath))).toBe( + path.join(path.sep, "tmp", "openclaw-preferred", "downloads"), + ); + expect(path.basename(String(outPath))).toMatch(/-passwd$/); + expect(path.normalize(res.path)).toContain( + path.normalize(`${path.join("tmp", "openclaw-preferred", "downloads")}${path.sep}`), + ); + }); it("waits for a matching response and returns its body", async () => { let responseHandler: ((resp: unknown) => void) | undefined; const on = vi.fn((event: string, handler: (resp: unknown) => void) => { diff --git a/src/browser/routes/agent.act.ts b/src/browser/routes/agent.act.ts index 6c6e31153b0..b2d34ee242b 100644 --- a/src/browser/routes/agent.act.ts +++ b/src/browser/routes/agent.act.ts @@ -14,7 +14,12 @@ import { resolveProfileContext, SELECTOR_UNSUPPORTED_MESSAGE, } from "./agent.shared.js"; -import { DEFAULT_DOWNLOAD_DIR, resolvePathWithinRoot } from "./path-output.js"; +import { + DEFAULT_DOWNLOAD_DIR, + DEFAULT_UPLOAD_DIR, + resolvePathWithinRoot, + resolvePathsWithinRoot, +} from "./path-output.js"; import { jsonError, toBoolean, toNumber, toStringArray, toStringOrEmpty } from "./utils.js"; export function registerBrowserAgentActRoutes( @@ -355,6 +360,17 @@ export function registerBrowserAgentActRoutes( return jsonError(res, 400, "paths are required"); } try { + const uploadPathsResult = resolvePathsWithinRoot({ + rootDir: DEFAULT_UPLOAD_DIR, + requestedPaths: paths, + scopeLabel: `uploads directory (${DEFAULT_UPLOAD_DIR})`, + }); + if (!uploadPathsResult.ok) { + res.status(400).json({ error: uploadPathsResult.error }); + return; + } + const resolvedPaths = uploadPathsResult.paths; + const tab = await profileCtx.ensureTabAvailable(targetId); const pw = await requirePwAi(res, "file chooser hook"); if (!pw) { @@ -369,13 +385,13 @@ export function registerBrowserAgentActRoutes( targetId: tab.targetId, inputRef, element, - paths, + paths: resolvedPaths, }); } else { await pw.armFileUploadViaPlaywright({ cdpUrl: profileCtx.profile.cdpUrl, targetId: tab.targetId, - paths, + paths: resolvedPaths, timeoutMs: timeoutMs ?? undefined, }); if (ref) { diff --git a/src/browser/routes/path-output.ts b/src/browser/routes/path-output.ts index 137b625210e..e23da97e1b2 100644 --- a/src/browser/routes/path-output.ts +++ b/src/browser/routes/path-output.ts @@ -1,28 +1 @@ -import path from "node:path"; -import { resolvePreferredOpenClawTmpDir } from "../../infra/tmp-openclaw-dir.js"; - -export const DEFAULT_BROWSER_TMP_DIR = resolvePreferredOpenClawTmpDir(); -export const DEFAULT_TRACE_DIR = DEFAULT_BROWSER_TMP_DIR; -export const DEFAULT_DOWNLOAD_DIR = path.join(DEFAULT_BROWSER_TMP_DIR, "downloads"); - -export function resolvePathWithinRoot(params: { - rootDir: string; - requestedPath: string; - scopeLabel: string; - defaultFileName?: string; -}): { ok: true; path: string } | { ok: false; error: string } { - const root = path.resolve(params.rootDir); - const raw = params.requestedPath.trim(); - if (!raw) { - if (!params.defaultFileName) { - return { ok: false, error: "path is required" }; - } - return { ok: true, path: path.join(root, params.defaultFileName) }; - } - const resolved = path.resolve(root, raw); - const rel = path.relative(root, resolved); - if (!rel || rel.startsWith("..") || path.isAbsolute(rel)) { - return { ok: false, error: `Invalid path: must stay within ${params.scopeLabel}` }; - } - return { ok: true, path: resolved }; -} +export * from "../paths.js"; diff --git a/src/browser/server.agent-contract-form-layout-act-commands.test.ts b/src/browser/server.agent-contract-form-layout-act-commands.test.ts index c4f44ed9c9c..35f0a980c98 100644 --- a/src/browser/server.agent-contract-form-layout-act-commands.test.ts +++ b/src/browser/server.agent-contract-form-layout-act-commands.test.ts @@ -4,6 +4,7 @@ import os from "node:os"; import path from "node:path"; import { fetch as realFetch } from "undici"; import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it, vi } from "vitest"; +import { DEFAULT_UPLOAD_DIR } from "./paths.js"; let testPort = 0; let cdpBaseUrl = ""; @@ -413,31 +414,31 @@ describe("browser control server", () => { const base = await startServerAndBase(); const upload = await postJson(`${base}/hooks/file-chooser`, { - paths: ["/tmp/a.txt"], + paths: ["a.txt"], timeoutMs: 1234, }); expect(upload).toMatchObject({ ok: true }); expect(pwMocks.armFileUploadViaPlaywright).toHaveBeenCalledWith({ cdpUrl: cdpBaseUrl, targetId: "abcd1234", - paths: ["/tmp/a.txt"], + paths: [path.join(DEFAULT_UPLOAD_DIR, "a.txt")], timeoutMs: 1234, }); const uploadWithRef = await postJson(`${base}/hooks/file-chooser`, { - paths: ["/tmp/b.txt"], + paths: ["b.txt"], ref: "e12", }); expect(uploadWithRef).toMatchObject({ ok: true }); const uploadWithInputRef = await postJson(`${base}/hooks/file-chooser`, { - paths: ["/tmp/c.txt"], + paths: ["c.txt"], inputRef: "e99", }); expect(uploadWithInputRef).toMatchObject({ ok: true }); const uploadWithElement = await postJson(`${base}/hooks/file-chooser`, { - paths: ["/tmp/d.txt"], + paths: ["d.txt"], element: "input[type=file]", }); expect(uploadWithElement).toMatchObject({ ok: true }); diff --git a/src/cli/browser-cli-actions-input/register.files-downloads.ts b/src/cli/browser-cli-actions-input/register.files-downloads.ts index 7cb9728e239..4a25f88e41f 100644 --- a/src/cli/browser-cli-actions-input/register.files-downloads.ts +++ b/src/cli/browser-cli-actions-input/register.files-downloads.ts @@ -1,10 +1,23 @@ import type { Command } from "commander"; +import { DEFAULT_UPLOAD_DIR, resolvePathsWithinRoot } from "../../browser/paths.js"; import { danger } from "../../globals.js"; import { defaultRuntime } from "../../runtime.js"; import { shortenHomePath } from "../../utils.js"; import { callBrowserRequest, type BrowserParentOpts } from "../browser-cli-shared.js"; import { resolveBrowserActionContext } from "./shared.js"; +function normalizeUploadPaths(paths: string[]): string[] { + const result = resolvePathsWithinRoot({ + rootDir: DEFAULT_UPLOAD_DIR, + requestedPaths: paths, + scopeLabel: `uploads directory (${DEFAULT_UPLOAD_DIR})`, + }); + if (!result.ok) { + throw new Error(result.error); + } + return result.paths; +} + export function registerBrowserFilesAndDownloadsCommands( browser: Command, parentOpts: (cmd: Command) => BrowserParentOpts, @@ -12,7 +25,10 @@ export function registerBrowserFilesAndDownloadsCommands( browser .command("upload") .description("Arm file upload for the next file chooser") - .argument("", "File paths to upload") + .argument( + "", + "File paths to upload (must be within OpenClaw temp uploads dir, e.g. /tmp/openclaw/uploads/file.pdf)", + ) .option("--ref ", "Ref id from snapshot to click after arming") .option("--input-ref ", "Ref id for to set directly") .option("--element ", "CSS selector for ") @@ -25,6 +41,7 @@ export function registerBrowserFilesAndDownloadsCommands( .action(async (paths: string[], opts, cmd) => { const { parent, profile } = resolveBrowserActionContext(cmd, parentOpts); try { + const normalizedPaths = normalizeUploadPaths(paths); const timeoutMs = Number.isFinite(opts.timeoutMs) ? opts.timeoutMs : undefined; const result = await callBrowserRequest<{ download: { path: string } }>( parent, @@ -33,7 +50,7 @@ export function registerBrowserFilesAndDownloadsCommands( path: "/hooks/file-chooser", query: profile ? { profile } : undefined, body: { - paths, + paths: normalizedPaths, ref: opts.ref?.trim() || undefined, inputRef: opts.inputRef?.trim() || undefined, element: opts.element?.trim() || undefined, diff --git a/src/cli/browser-cli-examples.ts b/src/cli/browser-cli-examples.ts index 6e51108dd30..7e6df7cd6db 100644 --- a/src/cli/browser-cli-examples.ts +++ b/src/cli/browser-cli-examples.ts @@ -24,7 +24,7 @@ export const browserActionExamples = [ "openclaw browser hover 44", "openclaw browser drag 10 11", "openclaw browser select 9 OptionA OptionB", - "openclaw browser upload /tmp/file.pdf", + "openclaw browser upload /tmp/openclaw/uploads/file.pdf", 'openclaw browser fill --fields \'[{"ref":"1","value":"Ada"}]\'', "openclaw browser dialog --accept", 'openclaw browser wait --text "Done"', diff --git a/src/commands/signal-install.test.ts b/src/commands/signal-install.test.ts index 018caf6c9a5..ccde686f62d 100644 --- a/src/commands/signal-install.test.ts +++ b/src/commands/signal-install.test.ts @@ -1,6 +1,11 @@ +import JSZip from "jszip"; +import fs from "node:fs/promises"; +import os from "node:os"; +import path from "node:path"; +import * as tar from "tar"; import { describe, expect, it } from "vitest"; import type { ReleaseAsset } from "./signal-install.js"; -import { looksLikeArchive, pickAsset } from "./signal-install.js"; +import { extractSignalCliArchive, looksLikeArchive, pickAsset } from "./signal-install.js"; // Realistic asset list modelled after an actual signal-cli GitHub release. const SAMPLE_ASSETS: ReleaseAsset[] = [ @@ -126,3 +131,44 @@ describe("pickAsset", () => { }); }); }); + +describe("extractSignalCliArchive", () => { + it("rejects zip slip path traversal", async () => { + const workDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-signal-install-")); + try { + const archivePath = path.join(workDir, "bad.zip"); + const extractDir = path.join(workDir, "extract"); + await fs.mkdir(extractDir, { recursive: true }); + + const zip = new JSZip(); + zip.file("../pwned.txt", "pwnd"); + await fs.writeFile(archivePath, await zip.generateAsync({ type: "nodebuffer" })); + + await expect(extractSignalCliArchive(archivePath, extractDir, 5_000)).rejects.toThrow( + /(escapes destination|absolute)/i, + ); + } finally { + await fs.rm(workDir, { recursive: true, force: true }).catch(() => undefined); + } + }); + + it("extracts tar.gz archives", async () => { + const workDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-signal-install-")); + try { + const archivePath = path.join(workDir, "ok.tgz"); + const extractDir = path.join(workDir, "extract"); + const rootDir = path.join(workDir, "root"); + await fs.mkdir(rootDir, { recursive: true }); + await fs.writeFile(path.join(rootDir, "signal-cli"), "bin", "utf-8"); + await tar.c({ cwd: workDir, file: archivePath, gzip: true }, ["root"]); + + await fs.mkdir(extractDir, { recursive: true }); + await extractSignalCliArchive(archivePath, extractDir, 5_000); + + const extracted = await fs.readFile(path.join(extractDir, "root", "signal-cli"), "utf-8"); + expect(extracted).toBe("bin"); + } finally { + await fs.rm(workDir, { recursive: true, force: true }).catch(() => undefined); + } + }); +}); diff --git a/src/commands/signal-install.ts b/src/commands/signal-install.ts index 4e68a813185..768d09f33cd 100644 --- a/src/commands/signal-install.ts +++ b/src/commands/signal-install.ts @@ -5,6 +5,7 @@ import os from "node:os"; import path from "node:path"; import { pipeline } from "node:stream/promises"; import type { RuntimeEnv } from "../runtime.js"; +import { extractArchive } from "../infra/archive.js"; import { resolveBrewExecutable } from "../infra/brew.js"; import { runCommandWithTimeout } from "../process/exec.js"; import { CONFIG_DIR } from "../utils.js"; @@ -31,6 +32,15 @@ export type SignalInstallResult = { error?: string; }; +/** @internal Exported for testing. */ +export async function extractSignalCliArchive( + archivePath: string, + installRoot: string, + timeoutMs: number, +): Promise { + await extractArchive({ archivePath, destDir: installRoot, timeoutMs }); +} + /** @internal Exported for testing. */ export function looksLikeArchive(name: string): boolean { return name.endsWith(".tar.gz") || name.endsWith(".tgz") || name.endsWith(".zip"); @@ -241,17 +251,18 @@ async function installSignalCliFromRelease(runtime: RuntimeEnv): Promise { 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); + }); }); diff --git a/src/infra/archive.ts b/src/infra/archive.ts index 305b8f14719..94266ee46b4 100644 --- a/src/infra/archive.ts +++ b/src/infra/archive.ts @@ -69,29 +69,99 @@ export async function withTimeout( } } -async function extractZip(params: { archivePath: string; destDir: string }): Promise { +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 { 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 { - 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 { diff --git a/src/infra/tmp-openclaw-dir.test.ts b/src/infra/tmp-openclaw-dir.test.ts index 1eea9a1bb4c..7dc134af6ee 100644 --- 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); }); }); diff --git a/src/infra/tmp-openclaw-dir.ts b/src/infra/tmp-openclaw-dir.ts index ab4038b7c95..d2377f57961 100644 --- 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(); } }