Skip to content

Commit 104d32b

Browse files
steipetesteipete
committed
fix(security): unify root-bound write hardening
1 parent be3a62c commit 104d32b

File tree

13 files changed

+427
-41
lines changed

13 files changed

+427
-41
lines changed

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@ Docs: https://docs.openclaw.ai
4747
### Fixes
4848

4949
- 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.
50+
- 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.
5051
- 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.
5152
- 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.
5253
- 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.

‎src/agents/sandbox/fs-bridge.ts‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -170,6 +170,11 @@ class SandboxFsBridgeImpl implements SandboxFsBridge {
170170
Boolean,
171171
);
172172
const rmCommand = flags.length > 0 ? `rm ${flags.join(" ")}` : "rm";
173+
await this.assertPathSafety(target, {
174+
action: "remove files",
175+
requireWritable: true,
176+
aliasPolicy: PATH_ALIAS_POLICIES.unlinkTarget,
177+
});
173178
await this.runCommand(`set -eu; ${rmCommand} -- "$1"`, {
174179
args: [target.containerPath],
175180
signal: params.signal,
@@ -195,6 +200,15 @@ class SandboxFsBridgeImpl implements SandboxFsBridge {
195200
action: "rename files",
196201
requireWritable: true,
197202
});
203+
await this.assertPathSafety(from, {
204+
action: "rename files",
205+
requireWritable: true,
206+
aliasPolicy: PATH_ALIAS_POLICIES.unlinkTarget,
207+
});
208+
await this.assertPathSafety(to, {
209+
action: "rename files",
210+
requireWritable: true,
211+
});
198212
await this.runCommand(
199213
'set -eu; dir=$(dirname -- "$2"); if [ "$dir" != "." ]; then mkdir -p -- "$dir"; fi; mv -- "$1" "$2"',
200214
{

‎src/agents/skills-install-download.ts‎

Lines changed: 65 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { createHash } from "node:crypto";
1+
import { createHash, randomUUID } from "node:crypto";
22
import fs from "node:fs";
33
import path from "node:path";
44
import { Readable } from "node:stream";
@@ -9,6 +9,8 @@ import {
99
createTarEntrySafetyChecker,
1010
extractArchive as extractArchiveSafe,
1111
} from "../infra/archive.js";
12+
import { writeFileFromPathWithinRoot } from "../infra/fs-safe.js";
13+
import { assertCanonicalPathWithinBase } from "../infra/install-safe-path.js";
1214
import { fetchWithSsrFGuard } from "../infra/net/fetch-guard.js";
1315
import { isWithinDir } from "../infra/path-safety.js";
1416
import { runCommandWithTimeout } from "../process/exec.js";
@@ -157,29 +159,44 @@ async function hashFileSha256(filePath: string): Promise<string> {
157159
});
158160
}
159161

160-
async function downloadFile(
161-
url: string,
162-
destPath: string,
163-
timeoutMs: number,
164-
): Promise<{ bytes: number }> {
162+
async function downloadFile(params: {
163+
url: string;
164+
rootDir: string;
165+
relativePath: string;
166+
timeoutMs: number;
167+
}): Promise<{ bytes: number }> {
168+
const destPath = path.resolve(params.rootDir, params.relativePath);
169+
const stagingDir = path.join(params.rootDir, ".openclaw-download-staging");
170+
await ensureDir(stagingDir);
171+
await assertCanonicalPathWithinBase({
172+
baseDir: params.rootDir,
173+
candidatePath: stagingDir,
174+
boundaryLabel: "skill tools directory",
175+
});
176+
const tempPath = path.join(stagingDir, `${randomUUID()}.tmp`);
165177
const { response, release } = await fetchWithSsrFGuard({
166-
url,
167-
timeoutMs: Math.max(1_000, timeoutMs),
178+
url: params.url,
179+
timeoutMs: Math.max(1_000, params.timeoutMs),
168180
});
169181
try {
170182
if (!response.ok || !response.body) {
171183
throw new Error(`Download failed (${response.status} ${response.statusText})`);
172184
}
173-
await ensureDir(path.dirname(destPath));
174-
const file = fs.createWriteStream(destPath);
185+
const file = fs.createWriteStream(tempPath);
175186
const body = response.body as unknown;
176187
const readable = isNodeReadableStream(body)
177188
? body
178189
: Readable.fromWeb(body as NodeReadableStream);
179190
await pipeline(readable, file);
191+
await writeFileFromPathWithinRoot({
192+
rootDir: params.rootDir,
193+
relativePath: params.relativePath,
194+
sourcePath: tempPath,
195+
});
180196
const stat = await fs.promises.stat(destPath);
181197
return { bytes: stat.size };
182198
} finally {
199+
await fs.promises.rm(tempPath, { force: true }).catch(() => undefined);
183200
await release();
184201
}
185202
}
@@ -309,6 +326,7 @@ export async function installDownloadSpec(params: {
309326
timeoutMs: number;
310327
}): Promise<SkillInstallResult> {
311328
const { entry, spec, timeoutMs } = params;
329+
const safeRoot = resolveSkillToolsRootDir(entry);
312330
const url = spec.url?.trim();
313331
if (!url) {
314332
return {
@@ -335,22 +353,40 @@ export async function installDownloadSpec(params: {
335353
try {
336354
targetDir = resolveDownloadTargetDir(entry, spec);
337355
await ensureDir(targetDir);
338-
const stat = await fs.promises.lstat(targetDir);
339-
if (stat.isSymbolicLink()) {
340-
throw new Error(`targetDir is a symlink: ${targetDir}`);
341-
}
342-
if (!stat.isDirectory()) {
343-
throw new Error(`targetDir is not a directory: ${targetDir}`);
344-
}
356+
await assertCanonicalPathWithinBase({
357+
baseDir: safeRoot,
358+
candidatePath: targetDir,
359+
boundaryLabel: "skill tools directory",
360+
});
345361
} catch (err) {
346362
const message = err instanceof Error ? err.message : String(err);
347363
return { ok: false, message, stdout: "", stderr: message, code: null };
348364
}
349365

350366
const archivePath = path.join(targetDir, filename);
367+
const archiveRelativePath = path.relative(safeRoot, archivePath);
368+
if (
369+
!archiveRelativePath ||
370+
archiveRelativePath === ".." ||
371+
archiveRelativePath.startsWith(`..${path.sep}`) ||
372+
path.isAbsolute(archiveRelativePath)
373+
) {
374+
return {
375+
ok: false,
376+
message: "invalid download archive path",
377+
stdout: "",
378+
stderr: "invalid download archive path",
379+
code: null,
380+
};
381+
}
351382
let downloaded = 0;
352383
try {
353-
const result = await downloadFile(url, archivePath, timeoutMs);
384+
const result = await downloadFile({
385+
url,
386+
rootDir: safeRoot,
387+
relativePath: archiveRelativePath,
388+
timeoutMs,
389+
});
354390
downloaded = result.bytes;
355391
} catch (err) {
356392
const message = err instanceof Error ? err.message : String(err);
@@ -379,6 +415,17 @@ export async function installDownloadSpec(params: {
379415
};
380416
}
381417

418+
try {
419+
await assertCanonicalPathWithinBase({
420+
baseDir: safeRoot,
421+
candidatePath: targetDir,
422+
boundaryLabel: "skill tools directory",
423+
});
424+
} catch (err) {
425+
const message = err instanceof Error ? err.message : String(err);
426+
return { ok: false, message, stdout: "", stderr: message, code: null };
427+
}
428+
382429
const extractResult = await extractArchive({
383430
archivePath,
384431
archiveType,

‎src/browser/output-atomic.ts‎

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import crypto from "node:crypto";
22
import fs from "node:fs/promises";
33
import path from "node:path";
4+
import { writeFileFromPathWithinRoot } from "../infra/fs-safe.js";
45
import { sanitizeUntrustedFileName } from "./safe-filename.js";
56

67
function buildSiblingTempPath(targetPath: string): string {
@@ -10,15 +11,31 @@ function buildSiblingTempPath(targetPath: string): string {
1011
}
1112

1213
export async function writeViaSiblingTempPath(params: {
14+
rootDir: string;
1315
targetPath: string;
1416
writeTemp: (tempPath: string) => Promise<void>;
1517
}): Promise<void> {
18+
const rootDir = path.resolve(params.rootDir);
1619
const targetPath = path.resolve(params.targetPath);
20+
const relativeTargetPath = path.relative(rootDir, targetPath);
21+
if (
22+
!relativeTargetPath ||
23+
relativeTargetPath === ".." ||
24+
relativeTargetPath.startsWith(`..${path.sep}`) ||
25+
path.isAbsolute(relativeTargetPath)
26+
) {
27+
throw new Error("Target path is outside the allowed root");
28+
}
1729
const tempPath = buildSiblingTempPath(targetPath);
1830
let renameSucceeded = false;
1931
try {
2032
await params.writeTemp(tempPath);
21-
await fs.rename(tempPath, targetPath);
33+
await writeFileFromPathWithinRoot({
34+
rootDir,
35+
relativePath: relativeTargetPath,
36+
sourcePath: tempPath,
37+
mkdir: false,
38+
});
2239
renameSucceeded = true;
2340
} finally {
2441
if (!renameSucceeded) {

‎src/browser/pw-tools-core.downloads.ts‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,11 @@ import path from "node:path";
44
import type { Page } from "playwright-core";
55
import { resolvePreferredOpenClawTmpDir } from "../infra/tmp-openclaw-dir.js";
66
import { writeViaSiblingTempPath } from "./output-atomic.js";
7-
import { DEFAULT_UPLOAD_DIR, resolveStrictExistingPathsWithinRoot } from "./paths.js";
7+
import {
8+
DEFAULT_DOWNLOAD_DIR,
9+
DEFAULT_UPLOAD_DIR,
10+
resolveStrictExistingPathsWithinRoot,
11+
} from "./paths.js";
812
import {
913
ensurePageState,
1014
getPageForTargetId,
@@ -92,6 +96,7 @@ async function saveDownloadPayload(download: DownloadPayload, outPath: string) {
9296
await download.saveAs?.(resolvedOutPath);
9397
} else {
9498
await writeViaSiblingTempPath({
99+
rootDir: DEFAULT_DOWNLOAD_DIR,
95100
targetPath: resolvedOutPath,
96101
writeTemp: async (tempPath) => {
97102
await download.saveAs?.(tempPath);

‎src/browser/pw-tools-core.trace.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import { writeViaSiblingTempPath } from "./output-atomic.js";
2+
import { DEFAULT_TRACE_DIR } from "./paths.js";
23
import { ensureContextState, getPageForTargetId } from "./pw-session.js";
34

45
export async function traceStartViaPlaywright(opts: {
@@ -34,6 +35,7 @@ export async function traceStopViaPlaywright(opts: {
3435
throw new Error("No active trace. Start a trace before stopping it.");
3536
}
3637
await writeViaSiblingTempPath({
38+
rootDir: DEFAULT_TRACE_DIR,
3739
targetPath: opts.path,
3840
writeTemp: async (tempPath) => {
3941
await context.tracing.stop({ path: tempPath });

‎src/hooks/install.ts‎

Lines changed: 26 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,11 @@ import {
88
resolveTimedInstallModeOptions,
99
} from "../infra/install-mode-options.js";
1010
import { installPackageDir } from "../infra/install-package-dir.js";
11-
import { resolveSafeInstallDir, unscopedPackageName } from "../infra/install-safe-path.js";
11+
import {
12+
assertCanonicalPathWithinBase,
13+
resolveSafeInstallDir,
14+
unscopedPackageName,
15+
} from "../infra/install-safe-path.js";
1216
import {
1317
type NpmIntegrityDrift,
1418
type NpmSpecResolution,
@@ -112,6 +116,15 @@ async function resolveInstallTargetDir(
112116
if (!targetDirResult.ok) {
113117
return { ok: false, error: targetDirResult.error };
114118
}
119+
try {
120+
await assertCanonicalPathWithinBase({
121+
baseDir: baseHooksDir,
122+
candidatePath: targetDirResult.path,
123+
boundaryLabel: "hooks directory",
124+
});
125+
} catch (err) {
126+
return { ok: false, error: err instanceof Error ? err.message : String(err) };
127+
}
115128
return { ok: true, targetDir: targetDirResult.path };
116129
}
117130

@@ -289,25 +302,18 @@ async function installHookFromDir(params: {
289302
return { ok: true, hookPackId: hookName, hooks: [hookName], targetDir };
290303
}
291304

292-
logger.info?.(`Installing to ${targetDir}…`);
293-
let backupDir: string | null = null;
294-
if (mode === "update" && (await fileExists(targetDir))) {
295-
backupDir = `${targetDir}.backup-${Date.now()}`;
296-
await fs.rename(targetDir, backupDir);
297-
}
298-
299-
try {
300-
await fs.cp(params.hookDir, targetDir, { recursive: true });
301-
} catch (err) {
302-
if (backupDir) {
303-
await fs.rm(targetDir, { recursive: true, force: true }).catch(() => undefined);
304-
await fs.rename(backupDir, targetDir).catch(() => undefined);
305-
}
306-
return { ok: false, error: `failed to copy hook: ${String(err)}` };
307-
}
308-
309-
if (backupDir) {
310-
await fs.rm(backupDir, { recursive: true, force: true }).catch(() => undefined);
305+
const installRes = await installPackageDir({
306+
sourceDir: params.hookDir,
307+
targetDir,
308+
mode,
309+
timeoutMs: 120_000,
310+
logger,
311+
copyErrorPrefix: "failed to copy hook",
312+
hasDeps: false,
313+
depsLogMessage: "Installing hook dependencies…",
314+
});
315+
if (!installRes.ok) {
316+
return installRes;
311317
}
312318

313319
return { ok: true, hookPackId: hookName, hooks: [hookName], targetDir };

0 commit comments

Comments
 (0)