diff --git a/src/agents/sandbox/bind-spec.test.ts b/src/agents/sandbox/bind-spec.test.ts new file mode 100644 index 00000000000..30d86551cc4 --- /dev/null +++ b/src/agents/sandbox/bind-spec.test.ts @@ -0,0 +1,29 @@ +import { describe, expect, it } from "vitest"; +import { splitSandboxBindSpec } from "./bind-spec.js"; + +describe("splitSandboxBindSpec", () => { + it("splits POSIX bind specs with and without mode", () => { + expect(splitSandboxBindSpec("/tmp/a:/workspace-a:ro")).toEqual({ + host: "/tmp/a", + container: "/workspace-a", + options: "ro", + }); + expect(splitSandboxBindSpec("/tmp/b:/workspace-b")).toEqual({ + host: "/tmp/b", + container: "/workspace-b", + options: "", + }); + }); + + it("preserves Windows drive-letter host paths", () => { + expect(splitSandboxBindSpec("C:\\Users\\kai\\workspace:/workspace:ro")).toEqual({ + host: "C:\\Users\\kai\\workspace", + container: "/workspace", + options: "ro", + }); + }); + + it("returns null when no host/container separator exists", () => { + expect(splitSandboxBindSpec("/tmp/no-separator")).toBeNull(); + }); +}); diff --git a/src/agents/sandbox/bind-spec.ts b/src/agents/sandbox/bind-spec.ts new file mode 100644 index 00000000000..4ce53c251a4 --- /dev/null +++ b/src/agents/sandbox/bind-spec.ts @@ -0,0 +1,34 @@ +type SplitBindSpec = { + host: string; + container: string; + options: string; +}; + +export function splitSandboxBindSpec(spec: string): SplitBindSpec | null { + const separator = getHostContainerSeparatorIndex(spec); + if (separator === -1) { + return null; + } + + const host = spec.slice(0, separator); + const rest = spec.slice(separator + 1); + const optionsStart = rest.indexOf(":"); + if (optionsStart === -1) { + return { host, container: rest, options: "" }; + } + return { + host, + container: rest.slice(0, optionsStart), + options: rest.slice(optionsStart + 1), + }; +} + +function getHostContainerSeparatorIndex(spec: string): number { + const hasDriveLetterPrefix = /^[A-Za-z]:[\\/]/.test(spec); + for (let i = hasDriveLetterPrefix ? 2 : 0; i < spec.length; i += 1) { + if (spec[i] === ":") { + return i; + } + } + return -1; +} diff --git a/src/agents/sandbox/fs-paths.ts b/src/agents/sandbox/fs-paths.ts index 11b5d712040..5de2f9e12ee 100644 --- a/src/agents/sandbox/fs-paths.ts +++ b/src/agents/sandbox/fs-paths.ts @@ -1,6 +1,8 @@ import path from "node:path"; import { resolveSandboxInputPath, resolveSandboxPath } from "../sandbox-paths.js"; +import { splitSandboxBindSpec } from "./bind-spec.js"; import { SANDBOX_AGENT_WORKSPACE_MOUNT } from "./constants.js"; +import { resolveSandboxHostPathViaExistingAncestor } from "./host-paths.js"; import type { SandboxContext } from "./types.js"; export type SandboxFsMount = { @@ -23,19 +25,13 @@ type ParsedBindMount = { writable: boolean; }; -type SplitBindSpec = { - host: string; - container: string; - options: string; -}; - export function parseSandboxBindMount(spec: string): ParsedBindMount | null { const trimmed = spec.trim(); if (!trimmed) { return null; } - const parsed = splitBindSpec(trimmed); + const parsed = splitSandboxBindSpec(trimmed); if (!parsed) { return null; } @@ -60,35 +56,6 @@ export function parseSandboxBindMount(spec: string): ParsedBindMount | null { }; } -function splitBindSpec(spec: string): SplitBindSpec | null { - const separator = getHostContainerSeparatorIndex(spec); - if (separator === -1) { - return null; - } - - const host = spec.slice(0, separator); - const rest = spec.slice(separator + 1); - const optionsStart = rest.indexOf(":"); - if (optionsStart === -1) { - return { host, container: rest, options: "" }; - } - return { - host, - container: rest.slice(0, optionsStart), - options: rest.slice(optionsStart + 1), - }; -} - -function getHostContainerSeparatorIndex(spec: string): number { - const hasDriveLetterPrefix = /^[A-Za-z]:[\\/]/.test(spec); - for (let i = hasDriveLetterPrefix ? 2 : 0; i < spec.length; i += 1) { - if (spec[i] === ":") { - return i; - } - } - return -1; -} - export function buildSandboxFsMounts(sandbox: SandboxContext): SandboxFsMount[] { const mounts: SandboxFsMount[] = [ { @@ -259,7 +226,9 @@ function isPathInsidePosix(root: string, target: string): boolean { } function isPathInsideHost(root: string, target: string): boolean { - const rel = path.relative(root, target); + const canonicalRoot = resolveSandboxHostPathViaExistingAncestor(path.resolve(root)); + const canonicalTarget = resolveSandboxHostPathViaExistingAncestor(path.resolve(target)); + const rel = path.relative(canonicalRoot, canonicalTarget); if (!rel) { return true; } diff --git a/src/agents/sandbox/host-paths.test.ts b/src/agents/sandbox/host-paths.test.ts new file mode 100644 index 00000000000..30933a5e03e --- /dev/null +++ b/src/agents/sandbox/host-paths.test.ts @@ -0,0 +1,38 @@ +import { mkdtempSync, mkdirSync, realpathSync, symlinkSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { describe, expect, it } from "vitest"; +import { + normalizeSandboxHostPath, + resolveSandboxHostPathViaExistingAncestor, +} from "./host-paths.js"; + +describe("normalizeSandboxHostPath", () => { + it("normalizes dot segments and strips trailing slash", () => { + expect(normalizeSandboxHostPath("/tmp/a/../b//")).toBe("/tmp/b"); + }); +}); + +describe("resolveSandboxHostPathViaExistingAncestor", () => { + it("keeps non-absolute paths unchanged", () => { + expect(resolveSandboxHostPathViaExistingAncestor("relative/path")).toBe("relative/path"); + }); + + it("resolves symlink parents when the final leaf does not exist", () => { + if (process.platform === "win32") { + return; + } + + const root = mkdtempSync(join(tmpdir(), "openclaw-host-paths-")); + const workspace = join(root, "workspace"); + const outside = join(root, "outside"); + mkdirSync(workspace, { recursive: true }); + mkdirSync(outside, { recursive: true }); + const link = join(workspace, "alias-out"); + symlinkSync(outside, link); + + const unresolved = join(link, "missing-leaf"); + const resolved = resolveSandboxHostPathViaExistingAncestor(unresolved); + expect(resolved).toBe(join(realpathSync.native(outside), "missing-leaf")); + }); +}); diff --git a/src/agents/sandbox/host-paths.ts b/src/agents/sandbox/host-paths.ts new file mode 100644 index 00000000000..7b99ed0a53c --- /dev/null +++ b/src/agents/sandbox/host-paths.ts @@ -0,0 +1,47 @@ +import { existsSync, realpathSync } from "node:fs"; +import { posix } from "node:path"; + +/** + * Normalize a POSIX host path: resolve `.`, `..`, collapse `//`, strip trailing `/`. + */ +export function normalizeSandboxHostPath(raw: string): string { + const trimmed = raw.trim(); + return posix.normalize(trimmed).replace(/\/+$/, "") || "/"; +} + +/** + * Resolve a path through the deepest existing ancestor so parent symlinks are honored + * even when the final source leaf does not exist yet. + */ +export function resolveSandboxHostPathViaExistingAncestor(sourcePath: string): string { + if (!sourcePath.startsWith("/")) { + return sourcePath; + } + + const normalized = normalizeSandboxHostPath(sourcePath); + let current = normalized; + const missingSegments: string[] = []; + + while (current !== "/" && !existsSync(current)) { + missingSegments.unshift(posix.basename(current)); + const parent = posix.dirname(current); + if (parent === current) { + break; + } + current = parent; + } + + if (!existsSync(current)) { + return normalized; + } + + try { + const resolvedAncestor = normalizeSandboxHostPath(realpathSync.native(current)); + if (missingSegments.length === 0) { + return resolvedAncestor; + } + return normalizeSandboxHostPath(posix.join(resolvedAncestor, ...missingSegments)); + } catch { + return normalized; + } +} diff --git a/src/agents/sandbox/validate-sandbox-security.ts b/src/agents/sandbox/validate-sandbox-security.ts index 44fe9f7ba0d..393d9f4b336 100644 --- a/src/agents/sandbox/validate-sandbox-security.ts +++ b/src/agents/sandbox/validate-sandbox-security.ts @@ -5,9 +5,12 @@ * Enforced at runtime when creating sandbox containers. */ -import { existsSync, realpathSync } from "node:fs"; -import { posix } from "node:path"; +import { splitSandboxBindSpec } from "./bind-spec.js"; import { SANDBOX_AGENT_WORKSPACE_MOUNT } from "./constants.js"; +import { + normalizeSandboxHostPath, + resolveSandboxHostPathViaExistingAncestor, +} from "./host-paths.js"; // Targeted denylist: host paths that should never be exposed inside sandbox containers. // Exported for reuse in security audit collectors. @@ -53,20 +56,11 @@ type ParsedBindSpec = { function parseBindSpec(bind: string): ParsedBindSpec { const trimmed = bind.trim(); - const firstColon = trimmed.indexOf(":"); - if (firstColon <= 0) { + const parsed = splitSandboxBindSpec(trimmed); + if (!parsed) { return { source: trimmed, target: "" }; } - const source = trimmed.slice(0, firstColon); - const rest = trimmed.slice(firstColon + 1); - const secondColon = rest.indexOf(":"); - if (secondColon === -1) { - return { source, target: rest }; - } - return { - source, - target: rest.slice(0, secondColon), - }; + return { source: parsed.host, target: parsed.container }; } /** @@ -85,8 +79,7 @@ export function parseBindTargetPath(bind: string): string { * Normalize a POSIX path: resolve `.`, `..`, collapse `//`, strip trailing `/`. */ export function normalizeHostPath(raw: string): string { - const trimmed = raw.trim(); - return posix.normalize(trimmed).replace(/\/+$/, "") || "/"; + return normalizeSandboxHostPath(raw); } /** @@ -119,41 +112,6 @@ export function getBlockedReasonForSourcePath(sourceNormalized: string): Blocked return null; } -function resolvePathViaExistingAncestor(sourcePath: string): string { - if (!sourcePath.startsWith("/")) { - return sourcePath; - } - - const normalized = normalizeHostPath(sourcePath); - let current = normalized; - const missingSegments: string[] = []; - - // Resolve through the deepest existing ancestor so symlink parents are honored - // even when the final source leaf does not exist yet. - while (current !== "/" && !existsSync(current)) { - missingSegments.unshift(posix.basename(current)); - const parent = posix.dirname(current); - if (parent === current) { - break; - } - current = parent; - } - - if (!existsSync(current)) { - return normalized; - } - - try { - const resolvedAncestor = normalizeHostPath(realpathSync.native(current)); - if (missingSegments.length === 0) { - return resolvedAncestor; - } - return normalizeHostPath(posix.join(resolvedAncestor, ...missingSegments)); - } catch { - return normalized; - } -} - function normalizeAllowedRoots(roots: string[] | undefined): string[] { if (!roots?.length) { return []; @@ -165,7 +123,7 @@ function normalizeAllowedRoots(roots: string[] | undefined): string[] { const expanded = new Set(); for (const root of normalized) { expanded.add(root); - const real = resolvePathViaExistingAncestor(root); + const real = resolveSandboxHostPathViaExistingAncestor(root); if (real !== root) { expanded.add(real); } @@ -217,6 +175,25 @@ function getReservedTargetReason(bind: string): BlockedBindReason | null { return null; } +function enforceSourcePathPolicy(params: { + bind: string; + sourcePath: string; + allowedRoots: string[]; + allowSourcesOutsideAllowedRoots: boolean; +}): void { + const blockedReason = getBlockedReasonForSourcePath(params.sourcePath); + if (blockedReason) { + throw formatBindBlockedError({ bind: params.bind, reason: blockedReason }); + } + if (params.allowSourcesOutsideAllowedRoots) { + return; + } + const allowedReason = getOutsideAllowedRootsReason(params.sourcePath, params.allowedRoots); + if (allowedReason) { + throw formatBindBlockedError({ bind: params.bind, reason: allowedReason }); + } +} + function formatBindBlockedError(params: { bind: string; reason: BlockedBindReason }): Error { if (params.reason.kind === "non_absolute") { return new Error( @@ -281,26 +258,21 @@ export function validateBindMounts( const sourceRaw = parseBindSourcePath(bind); const sourceNormalized = normalizeHostPath(sourceRaw); - - if (!options?.allowSourcesOutsideAllowedRoots) { - const allowedReason = getOutsideAllowedRootsReason(sourceNormalized, allowedRoots); - if (allowedReason) { - throw formatBindBlockedError({ bind, reason: allowedReason }); - } - } + enforceSourcePathPolicy({ + bind, + sourcePath: sourceNormalized, + allowedRoots, + allowSourcesOutsideAllowedRoots: options?.allowSourcesOutsideAllowedRoots === true, + }); // Symlink escape hardening: resolve through existing ancestors and re-check. - const sourceCanonical = resolvePathViaExistingAncestor(sourceNormalized); - const reason = getBlockedReasonForSourcePath(sourceCanonical); - if (reason) { - throw formatBindBlockedError({ bind, reason }); - } - if (!options?.allowSourcesOutsideAllowedRoots) { - const allowedReason = getOutsideAllowedRootsReason(sourceCanonical, allowedRoots); - if (allowedReason) { - throw formatBindBlockedError({ bind, reason: allowedReason }); - } - } + const sourceCanonical = resolveSandboxHostPathViaExistingAncestor(sourceNormalized); + enforceSourcePathPolicy({ + bind, + sourcePath: sourceCanonical, + allowedRoots, + allowSourcesOutsideAllowedRoots: options?.allowSourcesOutsideAllowedRoots === true, + }); } }