From de61e9c9771899d76710251c2f445a75d6488644 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Thu, 26 Feb 2026 03:59:08 +0100 Subject: [PATCH] refactor(security): unify path alias guard policies --- src/agents/apply-patch.ts | 13 ++--- src/agents/sandbox-paths.ts | 79 ++++++----------------------- src/agents/sandbox/fs-bridge.ts | 83 +++++------------------------- src/infra/hardlink-guards.ts | 4 +- src/infra/path-alias-guards.ts | 89 +++++++++++++++++++++++++++++++++ 5 files changed, 126 insertions(+), 142 deletions(-) create mode 100644 src/infra/path-alias-guards.ts diff --git a/src/agents/apply-patch.ts b/src/agents/apply-patch.ts index 4b147fd79fb..4f1487d34ea 100644 --- a/src/agents/apply-patch.ts +++ b/src/agents/apply-patch.ts @@ -2,6 +2,7 @@ import fs from "node:fs/promises"; import path from "node:path"; import type { AgentTool } from "@mariozechner/pi-agent-core"; import { Type } from "@sinclair/typebox"; +import { PATH_ALIAS_POLICIES, type PathAliasPolicy } from "../infra/path-alias-guards.js"; import { applyUpdateHunk } from "./apply-patch-update.js"; import { assertSandboxPath, resolveSandboxInputPath } from "./sandbox-paths.js"; import type { SandboxFsBridge } from "./sandbox/fs-bridge.js"; @@ -154,7 +155,7 @@ export async function applyPatch( } if (hunk.kind === "delete") { - const target = await resolvePatchPath(hunk.path, options, "unlink"); + const target = await resolvePatchPath(hunk.path, options, PATH_ALIAS_POLICIES.unlinkTarget); await fileOps.remove(target.resolved); recordSummary(summary, seen, "deleted", target.display); continue; @@ -253,7 +254,7 @@ async function ensureDir(filePath: string, ops: PatchFileOps) { async function resolvePatchPath( filePath: string, options: ApplyPatchOptions, - purpose: "readWrite" | "unlink" = "readWrite", + aliasPolicy: PathAliasPolicy = PATH_ALIAS_POLICIES.strict, ): Promise<{ resolved: string; display: string }> { if (options.sandbox) { const resolved = options.sandbox.bridge.resolvePath({ @@ -265,8 +266,8 @@ async function resolvePatchPath( filePath: resolved.hostPath, cwd: options.cwd, root: options.cwd, - allowFinalSymlink: purpose === "unlink", - allowFinalHardlink: purpose === "unlink", + allowFinalSymlinkForUnlink: aliasPolicy.allowFinalSymlinkForUnlink, + allowFinalHardlinkForUnlink: aliasPolicy.allowFinalHardlinkForUnlink, }); } return { @@ -282,8 +283,8 @@ async function resolvePatchPath( filePath, cwd: options.cwd, root: options.cwd, - allowFinalSymlink: purpose === "unlink", - allowFinalHardlink: purpose === "unlink", + allowFinalSymlinkForUnlink: aliasPolicy.allowFinalSymlinkForUnlink, + allowFinalHardlinkForUnlink: aliasPolicy.allowFinalHardlinkForUnlink, }) ).resolved : resolvePathFromCwd(filePath, options.cwd); diff --git a/src/agents/sandbox-paths.ts b/src/agents/sandbox-paths.ts index b50e90c3241..7cb026c28a4 100644 --- a/src/agents/sandbox-paths.ts +++ b/src/agents/sandbox-paths.ts @@ -1,9 +1,8 @@ -import fs from "node:fs/promises"; import os from "node:os"; import path from "node:path"; import { fileURLToPath, URL } from "node:url"; -import { assertNoHardlinkedFinalPath } from "../infra/hardlink-guards.js"; -import { isNotFoundPathError, isPathInside } from "../infra/path-guards.js"; +import { assertNoPathAliasEscape, type PathAliasPolicy } from "../infra/path-alias-guards.js"; +import { isPathInside } from "../infra/path-guards.js"; import { resolvePreferredOpenClawTmpDir } from "../infra/tmp-openclaw-dir.js"; const UNICODE_SPACES = /[\u00A0\u2000-\u200A\u202F\u205F\u3000]/g; @@ -62,18 +61,19 @@ export async function assertSandboxPath(params: { filePath: string; cwd: string; root: string; - allowFinalSymlink?: boolean; - allowFinalHardlink?: boolean; + allowFinalSymlinkForUnlink?: boolean; + allowFinalHardlinkForUnlink?: boolean; }) { const resolved = resolveSandboxPath(params); - await assertNoSymlinkEscape(resolved.relative, path.resolve(params.root), { - allowFinalSymlink: params.allowFinalSymlink, - }); - await assertNoHardlinkedFinalPath({ - filePath: resolved.resolved, - root: path.resolve(params.root), + const policy: PathAliasPolicy = { + allowFinalSymlinkForUnlink: params.allowFinalSymlinkForUnlink, + allowFinalHardlinkForUnlink: params.allowFinalHardlinkForUnlink, + }; + await assertNoPathAliasEscape({ + absolutePath: resolved.resolved, + rootPath: path.resolve(params.root), boundaryLabel: "sandbox root", - allowFinalHardlink: params.allowFinalHardlink, + policy, }); return resolved; } @@ -202,62 +202,13 @@ async function assertNoTmpAliasEscape(params: { filePath: string; tmpRoot: string; }): Promise { - await assertNoSymlinkEscape(path.relative(params.tmpRoot, params.filePath), params.tmpRoot); - await assertNoHardlinkedFinalPath({ - filePath: params.filePath, - root: params.tmpRoot, + await assertNoPathAliasEscape({ + absolutePath: params.filePath, + rootPath: params.tmpRoot, boundaryLabel: "tmp root", }); } -async function assertNoSymlinkEscape( - relative: string, - root: string, - options?: { allowFinalSymlink?: boolean }, -) { - if (!relative) { - return; - } - const rootReal = await tryRealpath(root); - const parts = relative.split(path.sep).filter(Boolean); - let current = root; - for (let idx = 0; idx < parts.length; idx += 1) { - const part = parts[idx]; - const isLast = idx === parts.length - 1; - current = path.join(current, part); - try { - const stat = await fs.lstat(current); - if (stat.isSymbolicLink()) { - // Unlinking a symlink itself is safe even if it points outside the root. What we - // must prevent is traversing through a symlink to reach targets outside root. - if (options?.allowFinalSymlink && isLast) { - return; - } - const target = await tryRealpath(current); - if (!isPathInside(rootReal, target)) { - throw new Error( - `Symlink escapes sandbox root (${shortPath(rootReal)}): ${shortPath(current)}`, - ); - } - current = target; - } - } catch (err) { - if (isNotFoundPathError(err)) { - return; - } - throw err; - } - } -} - -async function tryRealpath(value: string): Promise { - try { - return await fs.realpath(value); - } catch { - return path.resolve(value); - } -} - function shortPath(value: string) { if (value.startsWith(os.homedir())) { return `~${value.slice(os.homedir().length)}`; diff --git a/src/agents/sandbox/fs-bridge.ts b/src/agents/sandbox/fs-bridge.ts index 18991f60da6..23ebcce51b1 100644 --- a/src/agents/sandbox/fs-bridge.ts +++ b/src/agents/sandbox/fs-bridge.ts @@ -1,7 +1,8 @@ -import fs from "node:fs/promises"; -import path from "node:path"; -import { assertNoHardlinkedFinalPath } from "../../infra/hardlink-guards.js"; -import { isNotFoundPathError, isPathInside } from "../../infra/path-guards.js"; +import { + assertNoPathAliasEscape, + PATH_ALIAS_POLICIES, + type PathAliasPolicy, +} from "../../infra/path-alias-guards.js"; import { execDockerRaw, type ExecDockerRawResult } from "./docker.js"; import { buildSandboxFsMounts, @@ -21,8 +22,7 @@ type RunCommandOptions = { type PathSafetyOptions = { action: string; - allowFinalSymlink?: boolean; - allowFinalHardlink?: boolean; + aliasPolicy?: PathAliasPolicy; requireWritable?: boolean; }; @@ -152,8 +152,7 @@ class SandboxFsBridgeImpl implements SandboxFsBridge { await this.assertPathSafety(target, { action: "remove files", requireWritable: true, - allowFinalSymlink: true, - allowFinalHardlink: true, + aliasPolicy: PATH_ALIAS_POLICIES.unlinkTarget, }); const flags = [params.force === false ? "" : "-f", params.recursive ? "-r" : ""].filter( Boolean, @@ -178,8 +177,7 @@ class SandboxFsBridgeImpl implements SandboxFsBridge { await this.assertPathSafety(from, { action: "rename files", requireWritable: true, - allowFinalSymlink: true, - allowFinalHardlink: true, + aliasPolicy: PATH_ALIAS_POLICIES.unlinkTarget, }); await this.assertPathSafety(to, { action: "rename files", @@ -256,21 +254,16 @@ class SandboxFsBridgeImpl implements SandboxFsBridge { ); } - await assertNoHostSymlinkEscape({ + await assertNoPathAliasEscape({ absolutePath: target.hostPath, rootPath: lexicalMount.hostRoot, - allowFinalSymlink: options.allowFinalSymlink === true, - }); - await assertNoHardlinkedFinalPath({ - filePath: target.hostPath, - root: lexicalMount.hostRoot, boundaryLabel: "sandbox mount root", - allowFinalHardlink: options.allowFinalHardlink === true, + policy: options.aliasPolicy, }); const canonicalContainerPath = await this.resolveCanonicalContainerPath({ containerPath: target.containerPath, - allowFinalSymlink: options.allowFinalSymlink === true, + allowFinalSymlinkForUnlink: options.aliasPolicy?.allowFinalSymlinkForUnlink === true, }); const canonicalMount = this.resolveMountByContainerPath(canonicalContainerPath); if (!canonicalMount) { @@ -297,7 +290,7 @@ class SandboxFsBridgeImpl implements SandboxFsBridge { private async resolveCanonicalContainerPath(params: { containerPath: string; - allowFinalSymlink: boolean; + allowFinalSymlinkForUnlink: boolean; }): Promise { const script = [ "set -eu", @@ -318,7 +311,7 @@ class SandboxFsBridgeImpl implements SandboxFsBridge { 'printf "%s%s\\n" "$canonical" "$suffix"', ].join("\n"); const result = await this.runCommand(script, { - args: [params.containerPath, params.allowFinalSymlink ? "1" : "0"], + args: [params.containerPath, params.allowFinalSymlinkForUnlink ? "1" : "0"], }); const canonical = result.stdout.toString("utf8").trim(); if (!canonical.startsWith("/")) { @@ -361,53 +354,3 @@ function coerceStatType(typeRaw?: string): "file" | "directory" | "other" { } return "other"; } - -async function assertNoHostSymlinkEscape(params: { - absolutePath: string; - rootPath: string; - allowFinalSymlink: boolean; -}): Promise { - const root = path.resolve(params.rootPath); - const target = path.resolve(params.absolutePath); - if (!isPathInside(root, target)) { - throw new Error(`Sandbox path escapes mount root (${root}): ${params.absolutePath}`); - } - const relative = path.relative(root, target); - if (!relative) { - return; - } - const rootReal = await tryRealpath(root); - const parts = relative.split(path.sep).filter(Boolean); - let current = root; - for (let idx = 0; idx < parts.length; idx += 1) { - current = path.join(current, parts[idx] ?? ""); - const isLast = idx === parts.length - 1; - try { - const stat = await fs.lstat(current); - if (!stat.isSymbolicLink()) { - continue; - } - if (params.allowFinalSymlink && isLast) { - return; - } - const symlinkTarget = await tryRealpath(current); - if (!isPathInside(rootReal, symlinkTarget)) { - throw new Error(`Symlink escapes sandbox mount root (${rootReal}): ${current}`); - } - current = symlinkTarget; - } catch (error) { - if (isNotFoundPathError(error)) { - return; - } - throw error; - } - } -} - -async function tryRealpath(value: string): Promise { - try { - return await fs.realpath(value); - } catch { - return path.resolve(value); - } -} diff --git a/src/infra/hardlink-guards.ts b/src/infra/hardlink-guards.ts index 9681bc09b78..ad99729b463 100644 --- a/src/infra/hardlink-guards.ts +++ b/src/infra/hardlink-guards.ts @@ -6,9 +6,9 @@ export async function assertNoHardlinkedFinalPath(params: { filePath: string; root: string; boundaryLabel: string; - allowFinalHardlink?: boolean; + allowFinalHardlinkForUnlink?: boolean; }): Promise { - if (params.allowFinalHardlink) { + if (params.allowFinalHardlinkForUnlink) { return; } let stat: Awaited>; diff --git a/src/infra/path-alias-guards.ts b/src/infra/path-alias-guards.ts new file mode 100644 index 00000000000..86d08a3e44a --- /dev/null +++ b/src/infra/path-alias-guards.ts @@ -0,0 +1,89 @@ +import fs from "node:fs/promises"; +import os from "node:os"; +import path from "node:path"; +import { assertNoHardlinkedFinalPath } from "./hardlink-guards.js"; +import { isNotFoundPathError, isPathInside } from "./path-guards.js"; + +export type PathAliasPolicy = { + allowFinalSymlinkForUnlink?: boolean; + allowFinalHardlinkForUnlink?: boolean; +}; + +export const PATH_ALIAS_POLICIES = { + strict: Object.freeze({ + allowFinalSymlinkForUnlink: false, + allowFinalHardlinkForUnlink: false, + }), + unlinkTarget: Object.freeze({ + allowFinalSymlinkForUnlink: true, + allowFinalHardlinkForUnlink: true, + }), +} as const; + +export async function assertNoPathAliasEscape(params: { + absolutePath: string; + rootPath: string; + boundaryLabel: string; + policy?: PathAliasPolicy; +}): Promise { + const root = path.resolve(params.rootPath); + const target = path.resolve(params.absolutePath); + if (!isPathInside(root, target)) { + throw new Error( + `Path escapes ${params.boundaryLabel} (${shortPath(root)}): ${shortPath(params.absolutePath)}`, + ); + } + const relative = path.relative(root, target); + if (relative) { + const rootReal = await tryRealpath(root); + const parts = relative.split(path.sep).filter(Boolean); + let current = root; + for (let idx = 0; idx < parts.length; idx += 1) { + current = path.join(current, parts[idx] ?? ""); + const isLast = idx === parts.length - 1; + try { + const stat = await fs.lstat(current); + if (!stat.isSymbolicLink()) { + continue; + } + if (params.policy?.allowFinalSymlinkForUnlink && isLast) { + return; + } + const symlinkTarget = await tryRealpath(current); + if (!isPathInside(rootReal, symlinkTarget)) { + throw new Error( + `Symlink escapes ${params.boundaryLabel} (${shortPath(rootReal)}): ${shortPath(current)}`, + ); + } + current = symlinkTarget; + } catch (error) { + if (isNotFoundPathError(error)) { + break; + } + throw error; + } + } + } + + await assertNoHardlinkedFinalPath({ + filePath: target, + root, + boundaryLabel: params.boundaryLabel, + allowFinalHardlinkForUnlink: params.policy?.allowFinalHardlinkForUnlink, + }); +} + +async function tryRealpath(value: string): Promise { + try { + return await fs.realpath(value); + } catch { + return path.resolve(value); + } +} + +function shortPath(value: string) { + if (value.startsWith(os.homedir())) { + return `~${value.slice(os.homedir().length)}`; + } + return value; +}