From cc027149e553ff4be1afe2ca9cc3de9ccdea6f68 Mon Sep 17 00:00:00 2001 From: Fede Kamelhar Date: Mon, 27 Jul 2026 03:15:59 -0400 Subject: [PATCH] security fix(agents): close symlink-then-.. workspace boundary bypass in assertSandboxPath (#113405) * fix(agents): close symlink-then-.. workspace boundary bypass in assertSandboxPath resolveSandboxPath builds on path.resolve, which collapses ".." lexically before any symlink is resolved. When a "/.." sequence appears, the lexical collapse lands on a harmless in-root path while the OS resolves the same raw string (symlink first, then "..") to a location outside the workspace, so assertSandboxPath approved inputs whose real resolution escaped the boundary. Add assertRawParentWithinRoot: it resolves the raw (non-collapsed) parent chain via the OS realpath (fs.realpath.native; the JS realpath and path.resolve both pre-collapse "..") and asserts it stays inside the workspace root. It runs after assertNoPathAliasEscape so that guard's more specific messages still win for cases it already catches, adding coverage only for the residual gap it never sees. Not currently reachable through shipped tools (read/write/edit route I/O through fs-safe Root, which opens its own collapsed path; other callers use the returned .resolved) -- this hardens the validator so the boundary no longer depends on caller discipline. Regression test included. * fix(agents): simplify raw sandbox path guard * fix(agents): preserve symlinked sandbox roots * fix(agents): validate raw final path aliases --------- Co-authored-by: Peter Steinberger --- src/agents/sandbox-paths.test.ts | 113 +++++++++++++++++++++++++++++++ src/agents/sandbox-paths.ts | 75 +++++++++++++++++++- 2 files changed, 187 insertions(+), 1 deletion(-) diff --git a/src/agents/sandbox-paths.test.ts b/src/agents/sandbox-paths.test.ts index 5d906222216d..9c43d97646d7 100644 --- a/src/agents/sandbox-paths.test.ts +++ b/src/agents/sandbox-paths.test.ts @@ -7,6 +7,7 @@ import { describe, expect, it, vi } from "vitest"; import { resolvePreferredOpenClawTmpDir } from "../infra/tmp-openclaw-dir.js"; import { withEnvAsync } from "../test-utils/env.js"; import { + assertSandboxPath, resolveAllowedManagedMediaPath, resolveSandboxedMediaSource, resolveSandboxPath, @@ -116,6 +117,118 @@ describe("resolveSandboxPath", () => { }); }); +describe("assertSandboxPath", () => { + it.runIf(process.platform !== "win32")( + "rejects symlink-then-dot-dot traversal for existing and new files", + async () => { + const parent = await fs.realpath( + await fs.mkdtemp(path.join(os.tmpdir(), "sandbox-symlink-dotdot-")), + ); + const root = path.join(parent, "workspace"); + const outside = path.join(parent, "outside"); + try { + await fs.mkdir(path.join(root, "sub"), { recursive: true }); + await fs.mkdir(outside); + await fs.symlink("..", path.join(root, "sub", "up")); + await fs.writeFile(path.join(outside, "secret.txt"), "outside", "utf8"); + + const escapedRead = `${root}/sub/up/../outside/secret.txt`; + await expect(fs.readFile(escapedRead, "utf8")).resolves.toBe("outside"); + await expect(assertSandboxPath({ filePath: escapedRead, cwd: root, root })).rejects.toThrow( + /escapes sandbox root/i, + ); + await expect( + assertSandboxPath({ + filePath: `${root}/sub/up/../outside/new.txt`, + cwd: root, + root, + }), + ).rejects.toThrow(/escapes sandbox root/i); + await expect( + assertSandboxPath({ filePath: `${root}/sub/up/../..`, cwd: root, root }), + ).rejects.toThrow(/escapes sandbox root/i); + + await fs.mkdir(path.join(root, "a")); + await fs.mkdir(path.join(root, "b")); + await fs.symlink("../b", path.join(root, "a", "up")); + await fs.symlink(path.join(outside, "secret.txt"), path.join(root, "escape")); + const escapedFinalSymlink = `${root}/a/up/../escape`; + await expect(fs.readFile(escapedFinalSymlink, "utf8")).resolves.toBe("outside"); + await expect( + assertSandboxPath({ filePath: escapedFinalSymlink, cwd: root, root }), + ).rejects.toThrow(/symlink escapes sandbox root/i); + + await fs.symlink(outside, path.join(root, "outside-link")); + await expect( + assertSandboxPath({ + filePath: `${root}/outside-link/`, + cwd: root, + root, + allowFinalSymlinkForUnlink: true, + }), + ).rejects.toThrow(/escapes sandbox root/i); + + await fs.mkdir(path.join(root, "real")); + await fs.symlink(path.join(root, "real"), path.join(root, "in-root-link")); + await expect( + assertSandboxPath({ + filePath: `${root}/in-root-link/../real/new.txt`, + cwd: root, + root, + }), + ).resolves.toBeTruthy(); + await expect(assertSandboxPath({ filePath: root, cwd: root, root })).resolves.toBeTruthy(); + } finally { + await fs.rm(parent, { recursive: true, force: true }); + } + }, + ); + + it.runIf(process.platform === "win32")( + "pins Win32 junction-then-dot-dot to lexical traversal semantics", + async () => { + const parent = await fs.mkdtemp(path.join(os.tmpdir(), "sandbox-junction-dotdot-")); + const root = path.join(parent, "workspace"); + const outside = path.join(parent, "outside"); + try { + await fs.mkdir(path.join(root, "sub"), { recursive: true }); + await fs.mkdir(outside); + await fs.symlink(root, path.join(root, "sub", "up"), "junction"); + await fs.writeFile(path.join(outside, "secret.txt"), "outside", "utf8"); + const attemptedEscape = `${root}\\sub\\up\\..\\outside\\secret.txt`; + + await expect(fs.readFile(attemptedEscape, "utf8")).rejects.toMatchObject({ + code: "ENOENT", + }); + } finally { + await fs.rm(parent, { recursive: true, force: true }); + } + }, + ); + + it("accepts not-yet-created and symlinked roots", async () => { + const parent = await fs.realpath( + await fs.mkdtemp(path.join(os.tmpdir(), "sandbox-missing-root-")), + ); + try { + const root = path.join(parent, "workspace"); + await expect( + assertSandboxPath({ filePath: "nested/new.txt", cwd: root, root }), + ).resolves.toMatchObject({ relative: path.join("nested", "new.txt") }); + + const realRoot = path.join(parent, "real-workspace"); + const linkedRoot = path.join(parent, "linked-workspace"); + await fs.mkdir(realRoot); + await fs.symlink(realRoot, linkedRoot); + await expect( + assertSandboxPath({ filePath: linkedRoot, cwd: linkedRoot, root: linkedRoot }), + ).resolves.toMatchObject({ relative: "" }); + } finally { + await fs.rm(parent, { recursive: true, force: true }); + } + }); +}); + describe("resolveSandboxedMediaSource", () => { const openClawTmpDir = resolvePreferredOpenClawTmpDir(); diff --git a/src/agents/sandbox-paths.ts b/src/agents/sandbox-paths.ts index b843542f9ea2..c53c21f8259c 100644 --- a/src/agents/sandbox-paths.ts +++ b/src/agents/sandbox-paths.ts @@ -3,9 +3,11 @@ * * Handles host paths, file URLs, temporary media paths, and workspace root assertions. */ +import fs from "node:fs"; import os from "node:os"; import path from "node:path"; import { URL } from "node:url"; +import { promisify } from "node:util"; import { isPassThroughRemoteMediaSource } from "@openclaw/media-core/media-source-url"; import { isWindowsDrivePath } from "../infra/archive-path.js"; import { @@ -14,7 +16,7 @@ import { safeFileURLToPath, } from "../infra/local-file-access.js"; import { assertNoPathAliasEscape, type PathAliasPolicy } from "../infra/path-alias-guards.js"; -import { isPathInside } from "../infra/path-guards.js"; +import { isNotFoundPathError, isPathInside } from "../infra/path-guards.js"; import { resolvePreferredOpenClawTmpDir } from "../infra/tmp-openclaw-dir.js"; import { resolveConfigDir, shortenHomePath } from "../utils.js"; @@ -87,6 +89,66 @@ export function resolveSandboxPath(params: { filePath: string; cwd: string; root return { resolved, relative }; } +const realpathNative = promisify(fs.realpath.native); + +async function resolveRawPathViaExistingAncestor(rawPath: string): Promise { + let cursor = rawPath; + const missingSuffix: string[] = []; + while (true) { + try { + return path.resolve(await realpathNative(cursor), ...missingSuffix); + } catch (error) { + if (!isNotFoundPathError(error)) { + throw error; + } + const parent = path.dirname(cursor); + if (parent === cursor) { + throw error; + } + missingSuffix.unshift(path.basename(cursor)); + cursor = parent; + } + } +} + +async function assertRawParentWithinRoot(params: { + filePath: string; + cwd: string; + root: string; +}): Promise { + // Win32 resolves reparse-point/.. paths lexically, so it has no equivalent escape. + // Avoid adding another realpath to this hot path on Windows, where it is expensive. + if (process.platform === "win32") { + return resolveSandboxInputPath(params.filePath, params.cwd); + } + const expanded = expandPath(params.filePath); + if (isWindowsDrivePath(expanded)) { + return path.win32.normalize(expanded); + } + // Do not use path.resolve here: it would erase the symlink-sensitive `..` before + // native realpath can traverse the raw parent chain. The final component stays + // unresolved so assertNoPathAliasEscape retains final-link policy ownership. + const rawAbsolute = path.isAbsolute(expanded) ? expanded : `${params.cwd}${path.sep}${expanded}`; + const hasTrailingSeparator = rawAbsolute.endsWith(path.sep); + const rawParent = hasTrailingSeparator ? rawAbsolute : path.dirname(rawAbsolute); + const finalSegment = hasTrailingSeparator ? "." : path.basename(rawAbsolute); + const rootResolved = path.resolve(params.root); + const [rootCanonical, parentCanonical] = await Promise.all([ + resolveRawPathViaExistingAncestor(rootResolved), + resolveRawPathViaExistingAncestor(rawParent), + ]); + const targetCanonical = + path.resolve(rawAbsolute) === rootResolved + ? await resolveRawPathViaExistingAncestor(rawAbsolute) + : path.resolve(parentCanonical, finalSegment); + if (targetCanonical !== rootCanonical && !isPathInside(rootCanonical, targetCanonical)) { + throw new Error( + `Path escapes sandbox root (${shortenHomePath(rootCanonical)}): ${params.filePath}`, + ); + } + return targetCanonical; +} + export async function assertSandboxPath(params: { filePath: string; cwd: string; @@ -105,6 +167,17 @@ export async function assertSandboxPath(params: { boundaryLabel: "sandbox root", policy, }); + // The alias guard owns its specific symlink/hardlink errors; this closes the raw + // symlink-then-`..` gap that lexical normalization hides from that guard. + const rawTarget = await assertRawParentWithinRoot(params); + if (path.resolve(rawTarget) !== path.resolve(resolved.resolved)) { + await assertNoPathAliasEscape({ + absolutePath: rawTarget, + rootPath: params.root, + boundaryLabel: "sandbox root", + policy, + }); + } return resolved; }