fix(security): unify root-bound write hardening

This commit is contained in:
Peter Steinberger
2026-03-02 17:11:04 +00:00
parent be3a62c5e0
commit 104d32bb64
13 changed files with 427 additions and 41 deletions

View File

@@ -47,6 +47,7 @@ Docs: https://docs.openclaw.ai
### Fixes ### 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. - 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. - 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. - 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. - 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.

View File

@@ -170,6 +170,11 @@ class SandboxFsBridgeImpl implements SandboxFsBridge {
Boolean, Boolean,
); );
const rmCommand = flags.length > 0 ? `rm ${flags.join(" ")}` : "rm"; 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"`, { await this.runCommand(`set -eu; ${rmCommand} -- "$1"`, {
args: [target.containerPath], args: [target.containerPath],
signal: params.signal, signal: params.signal,
@@ -195,6 +200,15 @@ class SandboxFsBridgeImpl implements SandboxFsBridge {
action: "rename files", action: "rename files",
requireWritable: true, 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( await this.runCommand(
'set -eu; dir=$(dirname -- "$2"); if [ "$dir" != "." ]; then mkdir -p -- "$dir"; fi; mv -- "$1" "$2"', 'set -eu; dir=$(dirname -- "$2"); if [ "$dir" != "." ]; then mkdir -p -- "$dir"; fi; mv -- "$1" "$2"',
{ {

View File

@@ -1,4 +1,4 @@
import { createHash } from "node:crypto"; import { createHash, randomUUID } from "node:crypto";
import fs from "node:fs"; import fs from "node:fs";
import path from "node:path"; import path from "node:path";
import { Readable } from "node:stream"; import { Readable } from "node:stream";
@@ -9,6 +9,8 @@ import {
createTarEntrySafetyChecker, createTarEntrySafetyChecker,
extractArchive as extractArchiveSafe, extractArchive as extractArchiveSafe,
} from "../infra/archive.js"; } 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 { fetchWithSsrFGuard } from "../infra/net/fetch-guard.js";
import { isWithinDir } from "../infra/path-safety.js"; import { isWithinDir } from "../infra/path-safety.js";
import { runCommandWithTimeout } from "../process/exec.js"; import { runCommandWithTimeout } from "../process/exec.js";
@@ -157,29 +159,44 @@ async function hashFileSha256(filePath: string): Promise<string> {
}); });
} }
async function downloadFile( async function downloadFile(params: {
url: string, url: string;
destPath: string, rootDir: string;
timeoutMs: number, relativePath: string;
): Promise<{ bytes: number }> { 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({ const { response, release } = await fetchWithSsrFGuard({
url, url: params.url,
timeoutMs: Math.max(1_000, timeoutMs), timeoutMs: Math.max(1_000, params.timeoutMs),
}); });
try { try {
if (!response.ok || !response.body) { if (!response.ok || !response.body) {
throw new Error(`Download failed (${response.status} ${response.statusText})`); throw new Error(`Download failed (${response.status} ${response.statusText})`);
} }
await ensureDir(path.dirname(destPath)); const file = fs.createWriteStream(tempPath);
const file = fs.createWriteStream(destPath);
const body = response.body as unknown; const body = response.body as unknown;
const readable = isNodeReadableStream(body) const readable = isNodeReadableStream(body)
? body ? body
: Readable.fromWeb(body as NodeReadableStream); : Readable.fromWeb(body as NodeReadableStream);
await pipeline(readable, file); await pipeline(readable, file);
await writeFileFromPathWithinRoot({
rootDir: params.rootDir,
relativePath: params.relativePath,
sourcePath: tempPath,
});
const stat = await fs.promises.stat(destPath); const stat = await fs.promises.stat(destPath);
return { bytes: stat.size }; return { bytes: stat.size };
} finally { } finally {
await fs.promises.rm(tempPath, { force: true }).catch(() => undefined);
await release(); await release();
} }
} }
@@ -309,6 +326,7 @@ export async function installDownloadSpec(params: {
timeoutMs: number; timeoutMs: number;
}): Promise<SkillInstallResult> { }): Promise<SkillInstallResult> {
const { entry, spec, timeoutMs } = params; const { entry, spec, timeoutMs } = params;
const safeRoot = resolveSkillToolsRootDir(entry);
const url = spec.url?.trim(); const url = spec.url?.trim();
if (!url) { if (!url) {
return { return {
@@ -335,22 +353,40 @@ export async function installDownloadSpec(params: {
try { try {
targetDir = resolveDownloadTargetDir(entry, spec); targetDir = resolveDownloadTargetDir(entry, spec);
await ensureDir(targetDir); await ensureDir(targetDir);
const stat = await fs.promises.lstat(targetDir); await assertCanonicalPathWithinBase({
if (stat.isSymbolicLink()) { baseDir: safeRoot,
throw new Error(`targetDir is a symlink: ${targetDir}`); candidatePath: targetDir,
} boundaryLabel: "skill tools directory",
if (!stat.isDirectory()) { });
throw new Error(`targetDir is not a directory: ${targetDir}`);
}
} catch (err) { } catch (err) {
const message = err instanceof Error ? err.message : String(err); const message = err instanceof Error ? err.message : String(err);
return { ok: false, message, stdout: "", stderr: message, code: null }; return { ok: false, message, stdout: "", stderr: message, code: null };
} }
const archivePath = path.join(targetDir, filename); 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; let downloaded = 0;
try { try {
const result = await downloadFile(url, archivePath, timeoutMs); const result = await downloadFile({
url,
rootDir: safeRoot,
relativePath: archiveRelativePath,
timeoutMs,
});
downloaded = result.bytes; downloaded = result.bytes;
} catch (err) { } catch (err) {
const message = err instanceof Error ? err.message : String(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({ const extractResult = await extractArchive({
archivePath, archivePath,
archiveType, archiveType,

View File

@@ -1,6 +1,7 @@
import crypto from "node:crypto"; import crypto from "node:crypto";
import fs from "node:fs/promises"; import fs from "node:fs/promises";
import path from "node:path"; import path from "node:path";
import { writeFileFromPathWithinRoot } from "../infra/fs-safe.js";
import { sanitizeUntrustedFileName } from "./safe-filename.js"; import { sanitizeUntrustedFileName } from "./safe-filename.js";
function buildSiblingTempPath(targetPath: string): string { function buildSiblingTempPath(targetPath: string): string {
@@ -10,15 +11,31 @@ function buildSiblingTempPath(targetPath: string): string {
} }
export async function writeViaSiblingTempPath(params: { export async function writeViaSiblingTempPath(params: {
rootDir: string;
targetPath: string; targetPath: string;
writeTemp: (tempPath: string) => Promise<void>; writeTemp: (tempPath: string) => Promise<void>;
}): Promise<void> { }): Promise<void> {
const rootDir = path.resolve(params.rootDir);
const targetPath = path.resolve(params.targetPath); 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); const tempPath = buildSiblingTempPath(targetPath);
let renameSucceeded = false; let renameSucceeded = false;
try { try {
await params.writeTemp(tempPath); await params.writeTemp(tempPath);
await fs.rename(tempPath, targetPath); await writeFileFromPathWithinRoot({
rootDir,
relativePath: relativeTargetPath,
sourcePath: tempPath,
mkdir: false,
});
renameSucceeded = true; renameSucceeded = true;
} finally { } finally {
if (!renameSucceeded) { if (!renameSucceeded) {

View File

@@ -4,7 +4,11 @@ import path from "node:path";
import type { Page } from "playwright-core"; import type { Page } from "playwright-core";
import { resolvePreferredOpenClawTmpDir } from "../infra/tmp-openclaw-dir.js"; import { resolvePreferredOpenClawTmpDir } from "../infra/tmp-openclaw-dir.js";
import { writeViaSiblingTempPath } from "./output-atomic.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 { import {
ensurePageState, ensurePageState,
getPageForTargetId, getPageForTargetId,
@@ -92,6 +96,7 @@ async function saveDownloadPayload(download: DownloadPayload, outPath: string) {
await download.saveAs?.(resolvedOutPath); await download.saveAs?.(resolvedOutPath);
} else { } else {
await writeViaSiblingTempPath({ await writeViaSiblingTempPath({
rootDir: DEFAULT_DOWNLOAD_DIR,
targetPath: resolvedOutPath, targetPath: resolvedOutPath,
writeTemp: async (tempPath) => { writeTemp: async (tempPath) => {
await download.saveAs?.(tempPath); await download.saveAs?.(tempPath);

View File

@@ -1,4 +1,5 @@
import { writeViaSiblingTempPath } from "./output-atomic.js"; import { writeViaSiblingTempPath } from "./output-atomic.js";
import { DEFAULT_TRACE_DIR } from "./paths.js";
import { ensureContextState, getPageForTargetId } from "./pw-session.js"; import { ensureContextState, getPageForTargetId } from "./pw-session.js";
export async function traceStartViaPlaywright(opts: { 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."); throw new Error("No active trace. Start a trace before stopping it.");
} }
await writeViaSiblingTempPath({ await writeViaSiblingTempPath({
rootDir: DEFAULT_TRACE_DIR,
targetPath: opts.path, targetPath: opts.path,
writeTemp: async (tempPath) => { writeTemp: async (tempPath) => {
await context.tracing.stop({ path: tempPath }); await context.tracing.stop({ path: tempPath });

View File

@@ -8,7 +8,11 @@ import {
resolveTimedInstallModeOptions, resolveTimedInstallModeOptions,
} from "../infra/install-mode-options.js"; } from "../infra/install-mode-options.js";
import { installPackageDir } from "../infra/install-package-dir.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 { import {
type NpmIntegrityDrift, type NpmIntegrityDrift,
type NpmSpecResolution, type NpmSpecResolution,
@@ -112,6 +116,15 @@ async function resolveInstallTargetDir(
if (!targetDirResult.ok) { if (!targetDirResult.ok) {
return { ok: false, error: targetDirResult.error }; 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 }; return { ok: true, targetDir: targetDirResult.path };
} }
@@ -289,25 +302,18 @@ async function installHookFromDir(params: {
return { ok: true, hookPackId: hookName, hooks: [hookName], targetDir }; return { ok: true, hookPackId: hookName, hooks: [hookName], targetDir };
} }
logger.info?.(`Installing to ${targetDir}`); const installRes = await installPackageDir({
let backupDir: string | null = null; sourceDir: params.hookDir,
if (mode === "update" && (await fileExists(targetDir))) { targetDir,
backupDir = `${targetDir}.backup-${Date.now()}`; mode,
await fs.rename(targetDir, backupDir); timeoutMs: 120_000,
} logger,
copyErrorPrefix: "failed to copy hook",
try { hasDeps: false,
await fs.cp(params.hookDir, targetDir, { recursive: true }); depsLogMessage: "Installing hook dependencies…",
} catch (err) { });
if (backupDir) { if (!installRes.ok) {
await fs.rm(targetDir, { recursive: true, force: true }).catch(() => undefined); return installRes;
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);
} }
return { ok: true, hookPackId: hookName, hooks: [hookName], targetDir }; return { ok: true, hookPackId: hookName, hooks: [hookName], targetDir };

View File

@@ -11,6 +11,7 @@ import {
readPathWithinRoot, readPathWithinRoot,
readLocalFileSafely, readLocalFileSafely,
writeFileWithinRoot, writeFileWithinRoot,
writeFileFromPathWithinRoot,
} from "./fs-safe.js"; } from "./fs-safe.js";
const tempDirs = createTrackedTempDirs(); 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 () => { it("rejects write traversal outside root", async () => {
const root = await tempDirs.make("openclaw-fs-safe-root-"); const root = await tempDirs.make("openclaw-fs-safe-root-");
await expect( 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")( it.runIf(process.platform !== "win32")(
"cleans up created out-of-root file when symlink retarget races create path", "cleans up created out-of-root file when symlink retarget races create path",
async () => { async () => {

View File

@@ -464,3 +464,115 @@ export async function copyFileWithinRoot(params: {
} }
} }
} }
export async function writeFileFromPathWithinRoot(params: {
rootDir: string;
relativePath: string;
sourcePath: string;
mkdir?: boolean;
}): Promise<void> {
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(() => {});
}
}

View File

@@ -2,6 +2,7 @@ import fs from "node:fs/promises";
import path from "node:path"; import path from "node:path";
import { runCommandWithTimeout } from "../process/exec.js"; import { runCommandWithTimeout } from "../process/exec.js";
import { fileExists } from "./archive.js"; import { fileExists } from "./archive.js";
import { assertCanonicalPathWithinBase } from "./install-safe-path.js";
function isObjectRecord(value: unknown): value is Record<string, unknown> { function isObjectRecord(value: unknown): value is Record<string, unknown> {
return Boolean(value) && typeof value === "object" && !Array.isArray(value); return Boolean(value) && typeof value === "object" && !Array.isArray(value);
@@ -60,11 +61,23 @@ export async function installPackageDir(params: {
afterCopy?: () => void | Promise<void>; afterCopy?: () => void | Promise<void>;
}): Promise<{ ok: true } | { ok: false; error: string }> { }): Promise<{ ok: true } | { ok: false; error: string }> {
params.logger?.info?.(`Installing to ${params.targetDir}`); 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; let backupDir: string | null = null;
if (params.mode === "update" && (await fileExists(params.targetDir))) { if (params.mode === "update" && (await fileExists(params.targetDir))) {
const backupRoot = path.join(path.dirname(params.targetDir), ".openclaw-install-backups"); const backupRoot = path.join(path.dirname(params.targetDir), ".openclaw-install-backups");
backupDir = path.join(backupRoot, `${path.basename(params.targetDir)}-${Date.now()}`); backupDir = path.join(backupRoot, `${path.basename(params.targetDir)}-${Date.now()}`);
await fs.mkdir(backupRoot, { recursive: true }); await fs.mkdir(backupRoot, { recursive: true });
await assertCanonicalPathWithinBase({
baseDir: installBaseDir,
candidatePath: backupDir,
boundaryLabel: "install directory",
});
await fs.rename(params.targetDir, backupDir); await fs.rename(params.targetDir, backupDir);
} }
@@ -72,11 +85,26 @@ export async function installPackageDir(params: {
if (!backupDir) { if (!backupDir) {
return; 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.rm(params.targetDir, { recursive: true, force: true }).catch(() => undefined);
await fs.rename(backupDir, params.targetDir).catch(() => undefined); await fs.rename(backupDir, params.targetDir).catch(() => undefined);
}; };
try { try {
await assertCanonicalPathWithinBase({
baseDir: installBaseDir,
candidatePath: params.targetDir,
boundaryLabel: "install directory",
});
await fs.cp(params.sourceDir, params.targetDir, { recursive: true }); await fs.cp(params.sourceDir, params.targetDir, { recursive: true });
} catch (err) { } catch (err) {
await rollback(); await rollback();

View File

@@ -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 { describe, expect, it } from "vitest";
import { safePathSegmentHashed } from "./install-safe-path.js"; import { assertCanonicalPathWithinBase, safePathSegmentHashed } from "./install-safe-path.js";
describe("safePathSegmentHashed", () => { describe("safePathSegmentHashed", () => {
it("keeps safe names unchanged", () => { it("keeps safe names unchanged", () => {
@@ -20,3 +23,44 @@ describe("safePathSegmentHashed", () => {
expect(result).toMatch(/-[a-f0-9]{10}$/); 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 });
}
},
);
});

View File

@@ -1,5 +1,7 @@
import { createHash } from "node:crypto"; import { createHash } from "node:crypto";
import fs from "node:fs/promises";
import path from "node:path"; import path from "node:path";
import { isPathInside } from "./path-guards.js";
export function unscopedPackageName(name: string): string { export function unscopedPackageName(name: string): string {
const trimmed = name.trim(); const trimmed = name.trim();
@@ -60,3 +62,43 @@ export function resolveSafeInstallDir(params: {
} }
return { ok: true, path: targetDir }; return { ok: true, path: targetDir };
} }
export async function assertCanonicalPathWithinBase(params: {
baseDir: string;
candidatePath: string;
boundaryLabel: string;
}): Promise<void> {
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<void> => {
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));
}

View File

@@ -9,6 +9,7 @@ import {
} from "../infra/install-mode-options.js"; } from "../infra/install-mode-options.js";
import { installPackageDir } from "../infra/install-package-dir.js"; import { installPackageDir } from "../infra/install-package-dir.js";
import { import {
assertCanonicalPathWithinBase,
resolveSafeInstallDir, resolveSafeInstallDir,
safeDirName, safeDirName,
unscopedPackageName, unscopedPackageName,
@@ -234,6 +235,15 @@ async function installPluginFromPackageDir(params: {
return { ok: false, error: targetDirResult.error }; return { ok: false, error: targetDirResult.error };
} }
const targetDir = targetDirResult.path; 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))) { if (mode === "install" && (await fileExists(targetDir))) {
return { return {