From 104d32bb64cdf19d5e77f70553a511a2ae90ad1c Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Mon, 2 Mar 2026 17:11:04 +0000 Subject: [PATCH] fix(security): unify root-bound write hardening --- CHANGELOG.md | 1 + src/agents/sandbox/fs-bridge.ts | 14 ++++ src/agents/skills-install-download.ts | 83 ++++++++++++++---- src/browser/output-atomic.ts | 19 ++++- src/browser/pw-tools-core.downloads.ts | 7 +- src/browser/pw-tools-core.trace.ts | 2 + src/hooks/install.ts | 46 +++++----- src/infra/fs-safe.test.ts | 58 +++++++++++++ src/infra/fs-safe.ts | 112 +++++++++++++++++++++++++ src/infra/install-package-dir.ts | 28 +++++++ src/infra/install-safe-path.test.ts | 46 +++++++++- src/infra/install-safe-path.ts | 42 ++++++++++ src/plugins/install.ts | 10 +++ 13 files changed, 427 insertions(+), 41 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0ac6002af86..1a3315a3c40 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -47,6 +47,7 @@ Docs: https://docs.openclaw.ai ### Fixes - Sandbox/Bootstrap context boundary hardening: reject symlink/hardlink alias bootstrap seed files that resolve outside the source workspace and switch post-compaction `AGENTS.md` context reads to boundary-verified file opens, preventing host file content from being injected via workspace aliasing. Thanks @tdjackey for reporting. +- Browser/Security output boundary hardening: replace check-then-rename output commits with root-bound fd-verified writes, unify install/skills canonical path-boundary checks, and add regression coverage for symlink-rebind race paths across browser output and shared fs-safe write flows. Thanks @tdjackey for reporting. - Gateway/Security hardening: tie loopback-origin dev allowance to actual local socket clients (not Host header claims), add explicit warnings/metrics when `gateway.controlUi.dangerouslyAllowHostHeaderOriginFallback` accepts websocket origins, harden safe-regex detection for quantified ambiguous alternation patterns (for example `(a|aa)+`), and bound large regex-evaluation inputs for session-filter and log-redaction paths. - Tests/Sandbox + archive portability: use junction-compatible directory-link setup on Windows and explicit file-symlink platform guards in symlink escape tests where unprivileged file symlinks are unavailable, reducing false Windows CI failures while preserving traversal checks on supported paths. (#28747) Thanks @arosstale. - Security/Skills archive extraction: unify tar extraction safety checks across tar.gz and tar.bz2 install flows, enforce tar compressed-size limits, and fail closed if tar.bz2 archives change between preflight and extraction to prevent bypasses of entry-type/size guardrails. Thanks @GCXWLP for reporting. diff --git a/src/agents/sandbox/fs-bridge.ts b/src/agents/sandbox/fs-bridge.ts index 92ded714f37..4d28def07d2 100644 --- a/src/agents/sandbox/fs-bridge.ts +++ b/src/agents/sandbox/fs-bridge.ts @@ -170,6 +170,11 @@ class SandboxFsBridgeImpl implements SandboxFsBridge { Boolean, ); const rmCommand = flags.length > 0 ? `rm ${flags.join(" ")}` : "rm"; + await this.assertPathSafety(target, { + action: "remove files", + requireWritable: true, + aliasPolicy: PATH_ALIAS_POLICIES.unlinkTarget, + }); await this.runCommand(`set -eu; ${rmCommand} -- "$1"`, { args: [target.containerPath], signal: params.signal, @@ -195,6 +200,15 @@ class SandboxFsBridgeImpl implements SandboxFsBridge { action: "rename files", requireWritable: true, }); + await this.assertPathSafety(from, { + action: "rename files", + requireWritable: true, + aliasPolicy: PATH_ALIAS_POLICIES.unlinkTarget, + }); + await this.assertPathSafety(to, { + action: "rename files", + requireWritable: true, + }); await this.runCommand( 'set -eu; dir=$(dirname -- "$2"); if [ "$dir" != "." ]; then mkdir -p -- "$dir"; fi; mv -- "$1" "$2"', { diff --git a/src/agents/skills-install-download.ts b/src/agents/skills-install-download.ts index a8c77e1f4c7..2b657956bfb 100644 --- a/src/agents/skills-install-download.ts +++ b/src/agents/skills-install-download.ts @@ -1,4 +1,4 @@ -import { createHash } from "node:crypto"; +import { createHash, randomUUID } from "node:crypto"; import fs from "node:fs"; import path from "node:path"; import { Readable } from "node:stream"; @@ -9,6 +9,8 @@ import { createTarEntrySafetyChecker, extractArchive as extractArchiveSafe, } from "../infra/archive.js"; +import { writeFileFromPathWithinRoot } from "../infra/fs-safe.js"; +import { assertCanonicalPathWithinBase } from "../infra/install-safe-path.js"; import { fetchWithSsrFGuard } from "../infra/net/fetch-guard.js"; import { isWithinDir } from "../infra/path-safety.js"; import { runCommandWithTimeout } from "../process/exec.js"; @@ -157,29 +159,44 @@ async function hashFileSha256(filePath: string): Promise { }); } -async function downloadFile( - url: string, - destPath: string, - timeoutMs: number, -): Promise<{ bytes: number }> { +async function downloadFile(params: { + url: string; + rootDir: string; + relativePath: string; + timeoutMs: number; +}): Promise<{ bytes: number }> { + const destPath = path.resolve(params.rootDir, params.relativePath); + const stagingDir = path.join(params.rootDir, ".openclaw-download-staging"); + await ensureDir(stagingDir); + await assertCanonicalPathWithinBase({ + baseDir: params.rootDir, + candidatePath: stagingDir, + boundaryLabel: "skill tools directory", + }); + const tempPath = path.join(stagingDir, `${randomUUID()}.tmp`); const { response, release } = await fetchWithSsrFGuard({ - url, - timeoutMs: Math.max(1_000, timeoutMs), + url: params.url, + timeoutMs: Math.max(1_000, params.timeoutMs), }); try { if (!response.ok || !response.body) { throw new Error(`Download failed (${response.status} ${response.statusText})`); } - await ensureDir(path.dirname(destPath)); - const file = fs.createWriteStream(destPath); + const file = fs.createWriteStream(tempPath); const body = response.body as unknown; const readable = isNodeReadableStream(body) ? body : Readable.fromWeb(body as NodeReadableStream); await pipeline(readable, file); + await writeFileFromPathWithinRoot({ + rootDir: params.rootDir, + relativePath: params.relativePath, + sourcePath: tempPath, + }); const stat = await fs.promises.stat(destPath); return { bytes: stat.size }; } finally { + await fs.promises.rm(tempPath, { force: true }).catch(() => undefined); await release(); } } @@ -309,6 +326,7 @@ export async function installDownloadSpec(params: { timeoutMs: number; }): Promise { const { entry, spec, timeoutMs } = params; + const safeRoot = resolveSkillToolsRootDir(entry); const url = spec.url?.trim(); if (!url) { return { @@ -335,22 +353,40 @@ export async function installDownloadSpec(params: { try { targetDir = resolveDownloadTargetDir(entry, spec); await ensureDir(targetDir); - const stat = await fs.promises.lstat(targetDir); - if (stat.isSymbolicLink()) { - throw new Error(`targetDir is a symlink: ${targetDir}`); - } - if (!stat.isDirectory()) { - throw new Error(`targetDir is not a directory: ${targetDir}`); - } + await assertCanonicalPathWithinBase({ + baseDir: safeRoot, + candidatePath: targetDir, + boundaryLabel: "skill tools directory", + }); } catch (err) { const message = err instanceof Error ? err.message : String(err); return { ok: false, message, stdout: "", stderr: message, code: null }; } const archivePath = path.join(targetDir, filename); + const archiveRelativePath = path.relative(safeRoot, archivePath); + if ( + !archiveRelativePath || + archiveRelativePath === ".." || + archiveRelativePath.startsWith(`..${path.sep}`) || + path.isAbsolute(archiveRelativePath) + ) { + return { + ok: false, + message: "invalid download archive path", + stdout: "", + stderr: "invalid download archive path", + code: null, + }; + } let downloaded = 0; try { - const result = await downloadFile(url, archivePath, timeoutMs); + const result = await downloadFile({ + url, + rootDir: safeRoot, + relativePath: archiveRelativePath, + timeoutMs, + }); downloaded = result.bytes; } catch (err) { const message = err instanceof Error ? err.message : String(err); @@ -379,6 +415,17 @@ export async function installDownloadSpec(params: { }; } + try { + await assertCanonicalPathWithinBase({ + baseDir: safeRoot, + candidatePath: targetDir, + boundaryLabel: "skill tools directory", + }); + } catch (err) { + const message = err instanceof Error ? err.message : String(err); + return { ok: false, message, stdout: "", stderr: message, code: null }; + } + const extractResult = await extractArchive({ archivePath, archiveType, diff --git a/src/browser/output-atomic.ts b/src/browser/output-atomic.ts index 6d6e6370927..4beaf3cae0a 100644 --- a/src/browser/output-atomic.ts +++ b/src/browser/output-atomic.ts @@ -1,6 +1,7 @@ import crypto from "node:crypto"; import fs from "node:fs/promises"; import path from "node:path"; +import { writeFileFromPathWithinRoot } from "../infra/fs-safe.js"; import { sanitizeUntrustedFileName } from "./safe-filename.js"; function buildSiblingTempPath(targetPath: string): string { @@ -10,15 +11,31 @@ function buildSiblingTempPath(targetPath: string): string { } export async function writeViaSiblingTempPath(params: { + rootDir: string; targetPath: string; writeTemp: (tempPath: string) => Promise; }): Promise { + const rootDir = path.resolve(params.rootDir); const targetPath = path.resolve(params.targetPath); + const relativeTargetPath = path.relative(rootDir, targetPath); + if ( + !relativeTargetPath || + relativeTargetPath === ".." || + relativeTargetPath.startsWith(`..${path.sep}`) || + path.isAbsolute(relativeTargetPath) + ) { + throw new Error("Target path is outside the allowed root"); + } const tempPath = buildSiblingTempPath(targetPath); let renameSucceeded = false; try { await params.writeTemp(tempPath); - await fs.rename(tempPath, targetPath); + await writeFileFromPathWithinRoot({ + rootDir, + relativePath: relativeTargetPath, + sourcePath: tempPath, + mkdir: false, + }); renameSucceeded = true; } finally { if (!renameSucceeded) { diff --git a/src/browser/pw-tools-core.downloads.ts b/src/browser/pw-tools-core.downloads.ts index 0093c8c388f..fc4902428a0 100644 --- a/src/browser/pw-tools-core.downloads.ts +++ b/src/browser/pw-tools-core.downloads.ts @@ -4,7 +4,11 @@ import path from "node:path"; import type { Page } from "playwright-core"; import { resolvePreferredOpenClawTmpDir } from "../infra/tmp-openclaw-dir.js"; import { writeViaSiblingTempPath } from "./output-atomic.js"; -import { DEFAULT_UPLOAD_DIR, resolveStrictExistingPathsWithinRoot } from "./paths.js"; +import { + DEFAULT_DOWNLOAD_DIR, + DEFAULT_UPLOAD_DIR, + resolveStrictExistingPathsWithinRoot, +} from "./paths.js"; import { ensurePageState, getPageForTargetId, @@ -92,6 +96,7 @@ async function saveDownloadPayload(download: DownloadPayload, outPath: string) { await download.saveAs?.(resolvedOutPath); } else { await writeViaSiblingTempPath({ + rootDir: DEFAULT_DOWNLOAD_DIR, targetPath: resolvedOutPath, writeTemp: async (tempPath) => { await download.saveAs?.(tempPath); diff --git a/src/browser/pw-tools-core.trace.ts b/src/browser/pw-tools-core.trace.ts index 43d0dc0b672..ce49eb77e07 100644 --- a/src/browser/pw-tools-core.trace.ts +++ b/src/browser/pw-tools-core.trace.ts @@ -1,4 +1,5 @@ import { writeViaSiblingTempPath } from "./output-atomic.js"; +import { DEFAULT_TRACE_DIR } from "./paths.js"; import { ensureContextState, getPageForTargetId } from "./pw-session.js"; export async function traceStartViaPlaywright(opts: { @@ -34,6 +35,7 @@ export async function traceStopViaPlaywright(opts: { throw new Error("No active trace. Start a trace before stopping it."); } await writeViaSiblingTempPath({ + rootDir: DEFAULT_TRACE_DIR, targetPath: opts.path, writeTemp: async (tempPath) => { await context.tracing.stop({ path: tempPath }); diff --git a/src/hooks/install.ts b/src/hooks/install.ts index c6032b8247e..fe236ceae7b 100644 --- a/src/hooks/install.ts +++ b/src/hooks/install.ts @@ -8,7 +8,11 @@ import { resolveTimedInstallModeOptions, } from "../infra/install-mode-options.js"; import { installPackageDir } from "../infra/install-package-dir.js"; -import { resolveSafeInstallDir, unscopedPackageName } from "../infra/install-safe-path.js"; +import { + assertCanonicalPathWithinBase, + resolveSafeInstallDir, + unscopedPackageName, +} from "../infra/install-safe-path.js"; import { type NpmIntegrityDrift, type NpmSpecResolution, @@ -112,6 +116,15 @@ async function resolveInstallTargetDir( if (!targetDirResult.ok) { return { ok: false, error: targetDirResult.error }; } + try { + await assertCanonicalPathWithinBase({ + baseDir: baseHooksDir, + candidatePath: targetDirResult.path, + boundaryLabel: "hooks directory", + }); + } catch (err) { + return { ok: false, error: err instanceof Error ? err.message : String(err) }; + } return { ok: true, targetDir: targetDirResult.path }; } @@ -289,25 +302,18 @@ async function installHookFromDir(params: { return { ok: true, hookPackId: hookName, hooks: [hookName], targetDir }; } - logger.info?.(`Installing to ${targetDir}…`); - let backupDir: string | null = null; - if (mode === "update" && (await fileExists(targetDir))) { - backupDir = `${targetDir}.backup-${Date.now()}`; - await fs.rename(targetDir, backupDir); - } - - try { - await fs.cp(params.hookDir, targetDir, { recursive: true }); - } catch (err) { - if (backupDir) { - await fs.rm(targetDir, { recursive: true, force: true }).catch(() => undefined); - await fs.rename(backupDir, targetDir).catch(() => undefined); - } - return { ok: false, error: `failed to copy hook: ${String(err)}` }; - } - - if (backupDir) { - await fs.rm(backupDir, { recursive: true, force: true }).catch(() => undefined); + const installRes = await installPackageDir({ + sourceDir: params.hookDir, + targetDir, + mode, + timeoutMs: 120_000, + logger, + copyErrorPrefix: "failed to copy hook", + hasDeps: false, + depsLogMessage: "Installing hook dependencies…", + }); + if (!installRes.ok) { + return installRes; } return { ok: true, hookPackId: hookName, hooks: [hookName], targetDir }; diff --git a/src/infra/fs-safe.test.ts b/src/infra/fs-safe.test.ts index ac9f3df78eb..3e3d8cc5fc2 100644 --- a/src/infra/fs-safe.test.ts +++ b/src/infra/fs-safe.test.ts @@ -11,6 +11,7 @@ import { readPathWithinRoot, readLocalFileSafely, writeFileWithinRoot, + writeFileFromPathWithinRoot, } from "./fs-safe.js"; const tempDirs = createTrackedTempDirs(); @@ -213,6 +214,20 @@ describe("fs-safe", () => { }); }); + it("writes a file within root from another local source path safely", async () => { + const root = await tempDirs.make("openclaw-fs-safe-root-"); + const outside = await tempDirs.make("openclaw-fs-safe-src-"); + const sourcePath = path.join(outside, "source.bin"); + await fs.writeFile(sourcePath, "hello-from-source"); + await writeFileFromPathWithinRoot({ + rootDir: root, + relativePath: "nested/from-source.txt", + sourcePath, + }); + await expect(fs.readFile(path.join(root, "nested", "from-source.txt"), "utf8")).resolves.toBe( + "hello-from-source", + ); + }); it("rejects write traversal outside root", async () => { const root = await tempDirs.make("openclaw-fs-safe-root-"); await expect( @@ -295,6 +310,49 @@ describe("fs-safe", () => { }, ); + it.runIf(process.platform !== "win32")( + "does not clobber out-of-root file when symlink retarget races write-from-path open", + async () => { + const root = await tempDirs.make("openclaw-fs-safe-root-"); + const inside = path.join(root, "inside"); + const outside = await tempDirs.make("openclaw-fs-safe-outside-"); + const sourceDir = await tempDirs.make("openclaw-fs-safe-source-"); + const sourcePath = path.join(sourceDir, "source.txt"); + await fs.writeFile(sourcePath, "new-content"); + await fs.mkdir(inside, { recursive: true }); + const outsideTarget = path.join(outside, "target.txt"); + await fs.writeFile(outsideTarget, "X".repeat(4096)); + const slot = path.join(root, "slot"); + await fs.symlink(inside, slot); + + const realRealpath = fs.realpath.bind(fs); + let flipped = false; + const realpathSpy = vi.spyOn(fs, "realpath").mockImplementation(async (...args) => { + const [filePath] = args; + if (!flipped && String(filePath).endsWith(path.join("slot", "target.txt"))) { + flipped = true; + await fs.rm(slot, { recursive: true, force: true }); + await fs.symlink(outside, slot); + } + return await realRealpath(...args); + }); + try { + await expect( + writeFileFromPathWithinRoot({ + rootDir: root, + relativePath: path.join("slot", "target.txt"), + sourcePath, + mkdir: false, + }), + ).rejects.toMatchObject({ code: "outside-workspace" }); + } finally { + realpathSpy.mockRestore(); + } + + await expect(fs.readFile(outsideTarget, "utf8")).resolves.toBe("X".repeat(4096)); + }, + ); + it.runIf(process.platform !== "win32")( "cleans up created out-of-root file when symlink retarget races create path", async () => { diff --git a/src/infra/fs-safe.ts b/src/infra/fs-safe.ts index 5c7628ace78..5a48e474eb1 100644 --- a/src/infra/fs-safe.ts +++ b/src/infra/fs-safe.ts @@ -464,3 +464,115 @@ export async function copyFileWithinRoot(params: { } } } + +export async function writeFileFromPathWithinRoot(params: { + rootDir: string; + relativePath: string; + sourcePath: string; + mkdir?: boolean; +}): Promise { + const { rootReal, rootWithSep, resolved } = await resolvePathWithinRoot(params); + try { + await assertNoPathAliasEscape({ + absolutePath: resolved, + rootPath: rootReal, + boundaryLabel: "root", + }); + } catch (err) { + throw new SafeOpenError("invalid-path", "path alias escape blocked", { cause: err }); + } + if (params.mkdir !== false) { + await fs.mkdir(path.dirname(resolved), { recursive: true }); + } + + const source = await openVerifiedLocalFile(params.sourcePath, { rejectHardlinks: true }); + let ioPath = resolved; + try { + const resolvedRealPath = await fs.realpath(resolved); + if (!isPathInside(rootWithSep, resolvedRealPath)) { + throw new SafeOpenError("outside-workspace", "file is outside workspace root"); + } + ioPath = resolvedRealPath; + } catch (err) { + if (err instanceof SafeOpenError) { + await source.handle.close().catch(() => {}); + throw err; + } + if (!isNotFoundPathError(err)) { + await source.handle.close().catch(() => {}); + throw err; + } + } + + let handle: FileHandle; + let createdForWrite = false; + try { + try { + handle = await fs.open(ioPath, OPEN_WRITE_EXISTING_FLAGS, 0o600); + } catch (err) { + if (!isNotFoundPathError(err)) { + throw err; + } + handle = await fs.open(ioPath, OPEN_WRITE_CREATE_FLAGS, 0o600); + createdForWrite = true; + } + } catch (err) { + await source.handle.close().catch(() => {}); + if (isNotFoundPathError(err)) { + throw new SafeOpenError("not-found", "file not found"); + } + if (isSymlinkOpenError(err)) { + throw new SafeOpenError("invalid-path", "symlink open blocked", { cause: err }); + } + throw err; + } + + let openedRealPath: string | null = null; + try { + const [stat, lstat] = await Promise.all([handle.stat(), fs.lstat(ioPath)]); + if (lstat.isSymbolicLink() || !stat.isFile()) { + throw new SafeOpenError("invalid-path", "path is not a regular file under root"); + } + if (stat.nlink > 1) { + throw new SafeOpenError("invalid-path", "hardlinked path not allowed"); + } + if (!sameFileIdentity(stat, lstat)) { + throw new SafeOpenError("path-mismatch", "path changed during write"); + } + + const realPath = await fs.realpath(ioPath); + openedRealPath = realPath; + const realStat = await fs.stat(realPath); + if (!sameFileIdentity(stat, realStat)) { + throw new SafeOpenError("path-mismatch", "path mismatch"); + } + if (realStat.nlink > 1) { + throw new SafeOpenError("invalid-path", "hardlinked path not allowed"); + } + if (!isPathInside(rootWithSep, realPath)) { + throw new SafeOpenError("outside-workspace", "file is outside workspace root"); + } + + if (!createdForWrite) { + await handle.truncate(0); + } + const chunk = Buffer.allocUnsafe(64 * 1024); + let sourceOffset = 0; + while (true) { + const { bytesRead } = await source.handle.read(chunk, 0, chunk.length, sourceOffset); + if (bytesRead <= 0) { + break; + } + await handle.write(chunk.subarray(0, bytesRead), 0, bytesRead, null); + sourceOffset += bytesRead; + } + } catch (err) { + if (createdForWrite && err instanceof SafeOpenError && openedRealPath) { + await fs.rm(openedRealPath, { force: true }).catch(() => {}); + } + throw err; + } finally { + await source.handle.close().catch(() => {}); + await handle.close().catch(() => {}); + } +} diff --git a/src/infra/install-package-dir.ts b/src/infra/install-package-dir.ts index d9313164299..a2841d42c07 100644 --- a/src/infra/install-package-dir.ts +++ b/src/infra/install-package-dir.ts @@ -2,6 +2,7 @@ import fs from "node:fs/promises"; import path from "node:path"; import { runCommandWithTimeout } from "../process/exec.js"; import { fileExists } from "./archive.js"; +import { assertCanonicalPathWithinBase } from "./install-safe-path.js"; function isObjectRecord(value: unknown): value is Record { return Boolean(value) && typeof value === "object" && !Array.isArray(value); @@ -60,11 +61,23 @@ export async function installPackageDir(params: { afterCopy?: () => void | Promise; }): Promise<{ ok: true } | { ok: false; error: string }> { params.logger?.info?.(`Installing to ${params.targetDir}…`); + const installBaseDir = path.dirname(params.targetDir); + await fs.mkdir(installBaseDir, { recursive: true }); + await assertCanonicalPathWithinBase({ + baseDir: installBaseDir, + candidatePath: params.targetDir, + boundaryLabel: "install directory", + }); let backupDir: string | null = null; if (params.mode === "update" && (await fileExists(params.targetDir))) { const backupRoot = path.join(path.dirname(params.targetDir), ".openclaw-install-backups"); backupDir = path.join(backupRoot, `${path.basename(params.targetDir)}-${Date.now()}`); await fs.mkdir(backupRoot, { recursive: true }); + await assertCanonicalPathWithinBase({ + baseDir: installBaseDir, + candidatePath: backupDir, + boundaryLabel: "install directory", + }); await fs.rename(params.targetDir, backupDir); } @@ -72,11 +85,26 @@ export async function installPackageDir(params: { if (!backupDir) { return; } + await assertCanonicalPathWithinBase({ + baseDir: installBaseDir, + candidatePath: params.targetDir, + boundaryLabel: "install directory", + }); + await assertCanonicalPathWithinBase({ + baseDir: installBaseDir, + candidatePath: backupDir, + boundaryLabel: "install directory", + }); await fs.rm(params.targetDir, { recursive: true, force: true }).catch(() => undefined); await fs.rename(backupDir, params.targetDir).catch(() => undefined); }; try { + await assertCanonicalPathWithinBase({ + baseDir: installBaseDir, + candidatePath: params.targetDir, + boundaryLabel: "install directory", + }); await fs.cp(params.sourceDir, params.targetDir, { recursive: true }); } catch (err) { await rollback(); diff --git a/src/infra/install-safe-path.test.ts b/src/infra/install-safe-path.test.ts index 1d6b9b6e4e5..3ec0679c6cf 100644 --- a/src/infra/install-safe-path.test.ts +++ b/src/infra/install-safe-path.test.ts @@ -1,5 +1,8 @@ +import fs from "node:fs/promises"; +import os from "node:os"; +import path from "node:path"; import { describe, expect, it } from "vitest"; -import { safePathSegmentHashed } from "./install-safe-path.js"; +import { assertCanonicalPathWithinBase, safePathSegmentHashed } from "./install-safe-path.js"; describe("safePathSegmentHashed", () => { it("keeps safe names unchanged", () => { @@ -20,3 +23,44 @@ describe("safePathSegmentHashed", () => { expect(result).toMatch(/-[a-f0-9]{10}$/); }); }); + +describe("assertCanonicalPathWithinBase", () => { + it("accepts in-base directories", async () => { + const baseDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-install-safe-")); + try { + const candidate = path.join(baseDir, "tools"); + await fs.mkdir(candidate, { recursive: true }); + await expect( + assertCanonicalPathWithinBase({ + baseDir, + candidatePath: candidate, + boundaryLabel: "install directory", + }), + ).resolves.toBeUndefined(); + } finally { + await fs.rm(baseDir, { recursive: true, force: true }); + } + }); + + it.runIf(process.platform !== "win32")( + "rejects symlinked candidate directories that escape the base", + async () => { + const baseDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-install-safe-")); + const outsideDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-install-safe-outside-")); + try { + const linkDir = path.join(baseDir, "alias"); + await fs.symlink(outsideDir, linkDir); + await expect( + assertCanonicalPathWithinBase({ + baseDir, + candidatePath: linkDir, + boundaryLabel: "install directory", + }), + ).rejects.toThrow(/must stay within install directory/i); + } finally { + await fs.rm(baseDir, { recursive: true, force: true }); + await fs.rm(outsideDir, { recursive: true, force: true }); + } + }, + ); +}); diff --git a/src/infra/install-safe-path.ts b/src/infra/install-safe-path.ts index 98da6bba6ec..13cc88562ed 100644 --- a/src/infra/install-safe-path.ts +++ b/src/infra/install-safe-path.ts @@ -1,5 +1,7 @@ import { createHash } from "node:crypto"; +import fs from "node:fs/promises"; import path from "node:path"; +import { isPathInside } from "./path-guards.js"; export function unscopedPackageName(name: string): string { const trimmed = name.trim(); @@ -60,3 +62,43 @@ export function resolveSafeInstallDir(params: { } return { ok: true, path: targetDir }; } + +export async function assertCanonicalPathWithinBase(params: { + baseDir: string; + candidatePath: string; + boundaryLabel: string; +}): Promise { + const baseDir = path.resolve(params.baseDir); + const candidatePath = path.resolve(params.candidatePath); + if (!isPathInside(baseDir, candidatePath)) { + throw new Error(`Invalid path: must stay within ${params.boundaryLabel}`); + } + + const baseLstat = await fs.lstat(baseDir); + if (!baseLstat.isDirectory() || baseLstat.isSymbolicLink()) { + throw new Error(`Invalid ${params.boundaryLabel}: base directory must be a real directory`); + } + const baseRealPath = await fs.realpath(baseDir); + + const validateDirectory = async (dirPath: string): Promise => { + const dirLstat = await fs.lstat(dirPath); + if (!dirLstat.isDirectory() || dirLstat.isSymbolicLink()) { + throw new Error(`Invalid path: must stay within ${params.boundaryLabel}`); + } + const dirRealPath = await fs.realpath(dirPath); + if (!isPathInside(baseRealPath, dirRealPath)) { + throw new Error(`Invalid path: must stay within ${params.boundaryLabel}`); + } + }; + + try { + await validateDirectory(candidatePath); + return; + } catch (err) { + const code = (err as { code?: string }).code; + if (code !== "ENOENT") { + throw err; + } + } + await validateDirectory(path.dirname(candidatePath)); +} diff --git a/src/plugins/install.ts b/src/plugins/install.ts index baf3eb690ad..8cce6b06615 100644 --- a/src/plugins/install.ts +++ b/src/plugins/install.ts @@ -9,6 +9,7 @@ import { } from "../infra/install-mode-options.js"; import { installPackageDir } from "../infra/install-package-dir.js"; import { + assertCanonicalPathWithinBase, resolveSafeInstallDir, safeDirName, unscopedPackageName, @@ -234,6 +235,15 @@ async function installPluginFromPackageDir(params: { return { ok: false, error: targetDirResult.error }; } const targetDir = targetDirResult.path; + try { + await assertCanonicalPathWithinBase({ + baseDir: extensionsDir, + candidatePath: targetDir, + boundaryLabel: "extensions directory", + }); + } catch (err) { + return { ok: false, error: err instanceof Error ? err.message : String(err) }; + } if (mode === "install" && (await fileExists(targetDir))) { return {