From 95119017c847c737bd113f0bff728c4666d79c45 Mon Sep 17 00:00:00 2001 From: Devin Robison Date: Wed, 22 Apr 2026 16:44:25 -0600 Subject: [PATCH] fix(openshell): pin sandbox file reads (#69798) * fix(openshell): pin sandbox file reads against parent symlink swaps * docs(changelog): note openshell sandbox read pinning (#69798) * fix(openshell): containment-check against literal root and self-contain file-identity helper * test(openshell): spy on fsPromises.open for swap races, skip dev=0 test on win32 * fix(openshell): single-syscall fallback identity check + tighten sameFileIdentity types * fix(openshell): re-fstat pinned handle after identity check for defense-in-depth * fix(openshell): lstat leaf on platforms without O_NOFOLLOW to close windows symlink gap * fix(openshell): expose test seam for O_NOFOLLOW availability instead of patching native constants --- CHANGELOG.md | 1 + extensions/openshell/src/fs-bridge.ts | 214 ++++++++++++++- .../openshell/src/openshell-core.test.ts | 258 ++++++++++++++++++ 3 files changed, 467 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7809e6b2b17..95be1c766e6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -112,6 +112,7 @@ Docs: https://docs.openclaw.ai - Telegram: require the same `/models` authorization for group model-picker callbacks, so unauthorized participants can no longer browse or change the session model through inline buttons. (#70235) Thanks @drobison00. - Agents/Pi: keep the filtered tool-name allowlist active for embedded OpenAI/OpenAI Codex GPT-5 runs and compaction sessions, so bundled and client tools still execute after the Pi `0.68.1` session-tool allowlist change instead of stopping at plan-only replies with no tool call. (#70281) Thanks @jalehman. - Agents/Pi: honor explicit `strict-agentic` execution contracts for incomplete-turn retry guards across providers, so manually opted-in local or compatible models get the same retry behavior without relying on OpenAI model inference. (#66750) Thanks @ziomancer. +- OpenShell/sandbox: pin verified file reads to an already-opened descriptor, walk the ancestor chain for symlinked parents on platforms without fd-path readlink, and re-check file identity so parent symlink swaps cannot redirect in-sandbox reads to host files outside the allowed mount root. (#69798) Thanks @drobison00. ## 2026.4.21 diff --git a/extensions/openshell/src/fs-bridge.ts b/extensions/openshell/src/fs-bridge.ts index 2a0e74e6e3a..fbfa1c5ce68 100644 --- a/extensions/openshell/src/fs-bridge.ts +++ b/extensions/openshell/src/fs-bridge.ts @@ -1,4 +1,6 @@ +import fs from "node:fs"; import fsPromises from "node:fs/promises"; +import type { FileHandle } from "node:fs/promises"; import path from "node:path"; import { writeFileWithinRoot } from "openclaw/plugin-sdk/infra-runtime"; import type { @@ -50,13 +52,16 @@ class OpenShellFsBridge implements SandboxFsBridge { }): Promise { const target = this.resolveTarget(params); const hostPath = this.requireHostPath(target); - await assertLocalPathSafety({ - target, - root: target.mountHostRoot, - allowMissingLeaf: false, - allowFinalSymlinkForUnlink: false, + const handle = await openPinnedReadableFile({ + absolutePath: hostPath, + rootPath: target.mountHostRoot, + containerPath: target.containerPath, }); - return await fsPromises.readFile(hostPath); + try { + return (await handle.readFile()) as Buffer; + } finally { + await handle.close(); + } } async writeFile(params: { @@ -352,3 +357,200 @@ async function resolveCanonicalCandidate(targetPath: string): Promise { cursor = parent; } } + +async function openPinnedReadableFile(params: { + absolutePath: string; + rootPath: string; + containerPath: string; +}): Promise { + // The literal root is what `resolveTarget` joins caller-provided relative + // paths against, so pre-open containment must be checked in literal form. + // The canonical root is derived separately and used for the post-open + // path checks (fd-path readlink and realpath cross-check), so a workspace + // that is itself configured as a symlink still works. + const literalRoot = path.resolve(params.rootPath); + const canonicalRoot = await fsPromises.realpath(literalRoot).catch(() => literalRoot); + const literalPath = path.resolve(params.absolutePath); + // Cheap string-prefix check on the caller-provided absolute path; no + // filesystem state is read here, so there is no TOCTOU window. Deeper + // checks run after the fd is pinned. + if (!isPathInside(literalRoot, literalPath)) { + throw new Error(`Sandbox path escapes allowed mounts; cannot access: ${params.containerPath}`); + } + const { flags: openReadFlags, supportsNoFollow } = resolveOpenReadFlags(); + // Open first so every later check runs against an fd that is already pinned + // to one specific inode. `O_NOFOLLOW` prevents the final path component from + // being a symlink; the ancestor walk below handles parent-directory symlink + // swaps on platforms where fd-path readlink is not available. + const handle = await fsPromises.open(literalPath, openReadFlags); + try { + const openedStat = await handle.stat(); + if (!openedStat.isFile()) { + throw new Error(`Sandbox boundary checks failed; cannot read files: ${params.containerPath}`); + } + if (openedStat.nlink > 1) { + throw new Error(`Sandbox boundary checks failed; cannot read files: ${params.containerPath}`); + } + const resolvedPath = await resolveOpenedReadablePath(handle.fd); + if (resolvedPath !== null) { + // Primary guarantee on Linux: the fd's resolved path is derived from the + // kernel, so a parent-directory swap cannot make this return a stale path. + if (!isPathInside(canonicalRoot, resolvedPath)) { + throw new Error( + `Sandbox boundary checks failed; cannot read files: ${params.containerPath}`, + ); + } + return handle; + } + // Fallback for platforms where fd-path readlink is unavailable. On macOS, + // `/dev/fd/N` is a character device so readlink returns EINVAL; on Windows + // there is no `/proc` equivalent. With no kernel-backed path readback we + // must prove the pinned fd is in-root without trusting a separate + // `realpath` + `lstat` pair that would race between the two awaits. Walk + // every ancestor between `literalRoot` and `literalPath` — the actual + // on-disk chain — and reject if any ancestor is a symlink, then use a + // single `stat` call to confirm that the path still resolves to the + // same file the fd has pinned. `fs.promises.stat` resolves the path and + // returns the final file's identity in one syscall, so there is no + // between-await window for an attacker to race. + await assertAncestorChainHasNoSymlinks(literalRoot, literalPath, params.containerPath, { + // On platforms where `O_NOFOLLOW` is unavailable (Windows), the open + // call would have transparently followed a final-component symlink, so + // the ancestor walk has to lstat the leaf as well. + includeLeaf: !supportsNoFollow, + }); + const currentResolvedStat = await fsPromises.stat(literalPath); + if (!sameFileIdentity(currentResolvedStat, openedStat)) { + throw new Error(`Sandbox boundary checks failed; cannot read files: ${params.containerPath}`); + } + // Belt-and-suspenders: re-fstat the pinned fd after the identity check and + // confirm the file type and link count are still trustworthy. A hardlink + // that appeared between the initial fstat and here is not exploitable for + // the read (the fd is already pinned to the original inode), but failing + // closed here keeps the guarantee simple: the bytes we return always come + // from a file that was a single-linked regular file at verification time. + const postCheckStat = await handle.stat(); + if (!postCheckStat.isFile() || postCheckStat.nlink > 1) { + throw new Error(`Sandbox boundary checks failed; cannot read files: ${params.containerPath}`); + } + return handle; + } catch (error) { + await handle.close(); + throw error; + } +} + +// Walks each directory between canonicalRoot (exclusive) and +// targetAbsolutePath, `lstat`'ing each segment. Rejects if any intermediate +// segment is a symlink or a non-directory. By default the final component is +// not walked because `O_NOFOLLOW` already protects it on the open call. Pass +// `includeLeaf: true` on platforms where `O_NOFOLLOW` is unavailable +// (Windows) so a symlinked leaf cannot be followed silently by `open`. +async function assertAncestorChainHasNoSymlinks( + canonicalRoot: string, + targetAbsolutePath: string, + containerPath: string, + options: { includeLeaf?: boolean } = {}, +): Promise { + const relative = path.relative(canonicalRoot, targetAbsolutePath); + if (relative === "" || relative.startsWith("..") || path.isAbsolute(relative)) { + return; + } + const segments = relative.split(path.sep).filter((segment) => segment.length > 0); + const lastIndex = options.includeLeaf ? segments.length : segments.length - 1; + let cursor = canonicalRoot; + for (let i = 0; i < lastIndex; i += 1) { + cursor = path.join(cursor, segments[i]); + const stat = await fsPromises.lstat(cursor).catch(() => null); + if (!stat) { + throw new Error(`Sandbox boundary checks failed; cannot read files: ${containerPath}`); + } + const isLeaf = i === segments.length - 1; + if (stat.isSymbolicLink()) { + throw new Error(`Sandbox boundary checks failed; cannot read files: ${containerPath}`); + } + if (!isLeaf && !stat.isDirectory()) { + throw new Error(`Sandbox boundary checks failed; cannot read files: ${containerPath}`); + } + } +} + +type ReadOpenFlagsResolution = { flags: number; supportsNoFollow: boolean }; + +let readOpenFlagsResolverForTest: (() => ReadOpenFlagsResolution) | undefined; + +function resolveOpenReadFlags(): ReadOpenFlagsResolution { + if (readOpenFlagsResolverForTest) { + return readOpenFlagsResolverForTest(); + } + const closeOnExec = (fs.constants as Record).O_CLOEXEC ?? 0; + const supportsNoFollow = typeof fs.constants.O_NOFOLLOW === "number"; + const noFollow = supportsNoFollow ? fs.constants.O_NOFOLLOW : 0; + return { + flags: fs.constants.O_RDONLY | noFollow | closeOnExec, + supportsNoFollow, + }; +} + +/** + * Test-only seam for forcing the open-flag/`O_NOFOLLOW` resolution. Used to + * exercise the Windows-style fallback (no `O_NOFOLLOW`, ancestor walk + * includes the leaf) on platforms where `fs.constants.O_NOFOLLOW` is a + * non-configurable native data property and cannot be patched directly. + * + * @internal + */ +export function setReadOpenFlagsResolverForTest( + resolver: (() => ReadOpenFlagsResolution) | undefined, +): void { + readOpenFlagsResolverForTest = resolver; +} + +// Resolves the absolute path associated with an open fd via the kernel-backed +// `/proc/self/fd/` (Linux) or `/dev/fd/` (some BSDs). Returns null +// when no fd-path endpoint is available. Note: on macOS `/dev/fd/N` is a +// character device rather than a symlink, so `readlink` fails with EINVAL +// there and the caller must use the ancestor-walk fallback instead. +async function resolveOpenedReadablePath(fd: number): Promise { + for (const fdPath of [`/proc/self/fd/${fd}`, `/dev/fd/${fd}`]) { + try { + const openedPath = await fsPromises.readlink(fdPath); + return normalizeOpenedReadablePath(openedPath); + } catch { + continue; + } + } + return null; +} + +function normalizeOpenedReadablePath(openedPath: string): string { + const deletedSuffix = " (deleted)"; + const withoutDeletedSuffix = openedPath.endsWith(deletedSuffix) + ? openedPath.slice(0, -deletedSuffix.length) + : openedPath; + return path.resolve(withoutDeletedSuffix); +} + +// File identity comparison with win32-aware `dev=0` handling, matching the +// shared `src/infra/file-identity.ts` contract. Kept local because extension +// production code is not allowed to reach into core `src/**` by relative +// import, and this helper is not yet part of the `openclaw/plugin-sdk/*` +// public surface. Stats here come from `FileHandle.stat()` / `fs.promises.stat()` +// with no `{ bigint: true }` option, so all fields are numbers. +function sameFileIdentity( + left: { dev: number; ino: number }, + right: { dev: number; ino: number }, + platform: NodeJS.Platform = process.platform, +): boolean { + if (left.ino !== right.ino) { + return false; + } + if (left.dev === right.dev) { + return true; + } + // On Windows, path-based stat can report `dev=0` while fd-based stat reports + // a real volume serial. Treat either side `dev=0` as "unknown device" + // rather than a mismatch so legitimate Windows fallback reads are not + // rejected. + return platform === "win32" && (left.dev === 0 || right.dev === 0); +} diff --git a/extensions/openshell/src/openshell-core.test.ts b/extensions/openshell/src/openshell-core.test.ts index b5273f82c29..e54e76c2f0e 100644 --- a/extensions/openshell/src/openshell-core.test.ts +++ b/extensions/openshell/src/openshell-core.test.ts @@ -1,3 +1,4 @@ +import nodeFs from "node:fs"; import fs from "node:fs/promises"; import os from "node:os"; import path from "node:path"; @@ -200,6 +201,22 @@ afterEach(async () => { await Promise.all(tempDirs.splice(0).map((dir) => fs.rm(dir, { recursive: true, force: true }))); }); +function cloneStatWithDev( + stat: T, + dev: number | bigint, +): T { + return Object.defineProperty( + Object.create(Object.getPrototypeOf(stat), Object.getOwnPropertyDescriptors(stat)), + "dev", + { + value: dev, + configurable: true, + enumerable: true, + writable: true, + }, + ) as T; +} + function createMirrorBackendMock(): OpenShellSandboxBackend { return { id: "openshell", @@ -307,6 +324,247 @@ describe("openshell fs bridges", () => { expect(backend.syncLocalPathToRemote).not.toHaveBeenCalled(); }); + it("rejects a parent symlink swap that lands outside the sandbox root", async () => { + const workspaceDir = await makeTempDir("openclaw-openshell-fs-"); + const outsideDir = await makeTempDir("openclaw-openshell-outside-"); + await fs.mkdir(path.join(workspaceDir, "subdir"), { recursive: true }); + await fs.writeFile(path.join(workspaceDir, "subdir", "secret.txt"), "inside", "utf8"); + await fs.writeFile(path.join(outsideDir, "secret.txt"), "outside", "utf8"); + const backend = createMirrorBackendMock(); + const sandbox = createSandboxTestContext({ + overrides: { + backendId: "openshell", + workspaceDir, + agentWorkspaceDir: workspaceDir, + containerWorkdir: "/sandbox", + }, + }); + + const { createOpenShellFsBridge } = await import("./fs-bridge.js"); + const bridge = createOpenShellFsBridge({ sandbox, backend }); + const originalOpen = fs.open.bind(fs); + const targetPath = path.join(workspaceDir, "subdir", "secret.txt"); + let swapped = false; + const openSpy = vi.spyOn(fs, "open").mockImplementation((async (...args: unknown[]) => { + const filePath = args[0]; + if (!swapped && filePath === targetPath) { + swapped = true; + nodeFs.rmSync(path.join(workspaceDir, "subdir"), { recursive: true, force: true }); + nodeFs.symlinkSync(outsideDir, path.join(workspaceDir, "subdir")); + } + return await (originalOpen as (...delegated: unknown[]) => Promise)(...args); + }) as unknown as typeof fs.open); + + try { + await expect(bridge.readFile({ filePath: "subdir/secret.txt" })).rejects.toThrow( + "Sandbox boundary checks failed", + ); + expect(openSpy).toHaveBeenCalled(); + } finally { + openSpy.mockRestore(); + } + }); + + it("falls back to inode checks when fd path resolution is unavailable", async () => { + const workspaceDir = await makeTempDir("openclaw-openshell-fs-"); + await fs.mkdir(path.join(workspaceDir, "subdir"), { recursive: true }); + await fs.writeFile(path.join(workspaceDir, "subdir", "secret.txt"), "inside", "utf8"); + + const backend = createMirrorBackendMock(); + const sandbox = createSandboxTestContext({ + overrides: { + backendId: "openshell", + workspaceDir, + agentWorkspaceDir: workspaceDir, + containerWorkdir: "/sandbox", + }, + }); + + const { createOpenShellFsBridge } = await import("./fs-bridge.js"); + const bridge = createOpenShellFsBridge({ sandbox, backend }); + const readlinkSpy = vi.spyOn(fs, "readlink").mockRejectedValue(new Error("fd path unavailable")); + + try { + await expect(bridge.readFile({ filePath: "subdir/secret.txt" })).resolves.toEqual( + Buffer.from("inside"), + ); + expect(readlinkSpy).toHaveBeenCalled(); + } finally { + readlinkSpy.mockRestore(); + } + }); + + // The shared `sameFileIdentity` contract intentionally treats either-side + // `dev=0` as "unknown device" on win32 (path-based stat can legitimately + // report `dev=0` there) and only fails closed on other platforms. Skip the + // Linux/macOS rejection expectation on Windows runners. + it.skipIf(process.platform === "win32")( + "rejects fallback reads when path stats report an unknown device id", + async () => { + const workspaceDir = await makeTempDir("openclaw-openshell-fs-"); + const targetPath = path.join(workspaceDir, "subdir", "secret.txt"); + await fs.mkdir(path.join(workspaceDir, "subdir"), { recursive: true }); + await fs.writeFile(targetPath, "inside", "utf8"); + + const backend = createMirrorBackendMock(); + const sandbox = createSandboxTestContext({ + overrides: { + backendId: "openshell", + workspaceDir, + agentWorkspaceDir: workspaceDir, + containerWorkdir: "/sandbox", + }, + }); + + const { createOpenShellFsBridge } = await import("./fs-bridge.js"); + const bridge = createOpenShellFsBridge({ sandbox, backend }); + const readlinkSpy = vi.spyOn(fs, "readlink").mockRejectedValue(new Error("fd path unavailable")); + const originalStat = fs.stat.bind(fs); + const statSpy = vi.spyOn(fs, "stat").mockImplementation(async (...args) => { + const stat = await originalStat(...args); + if (args[0] === targetPath) { + return cloneStatWithDev(stat, 0); + } + return stat; + }); + + try { + await expect(bridge.readFile({ filePath: "subdir/secret.txt" })).rejects.toThrow( + "Sandbox boundary checks failed", + ); + expect(readlinkSpy).toHaveBeenCalled(); + expect(statSpy).toHaveBeenCalledWith(targetPath); + } finally { + statSpy.mockRestore(); + readlinkSpy.mockRestore(); + } + }, + ); + + it("rejects fallback reads when an ancestor directory is swapped to a symlink", async () => { + const workspaceDir = await makeTempDir("openclaw-openshell-fs-"); + const outsideDir = await makeTempDir("openclaw-openshell-outside-"); + await fs.mkdir(path.join(workspaceDir, "subdir"), { recursive: true }); + await fs.writeFile(path.join(workspaceDir, "subdir", "secret.txt"), "inside", "utf8"); + await fs.writeFile(path.join(outsideDir, "secret.txt"), "outside", "utf8"); + + const backend = createMirrorBackendMock(); + const sandbox = createSandboxTestContext({ + overrides: { + backendId: "openshell", + workspaceDir, + agentWorkspaceDir: workspaceDir, + containerWorkdir: "/sandbox", + }, + }); + + const { createOpenShellFsBridge } = await import("./fs-bridge.js"); + const bridge = createOpenShellFsBridge({ sandbox, backend }); + const originalOpen = fs.open.bind(fs); + const targetPath = path.join(workspaceDir, "subdir", "secret.txt"); + let swapped = false; + const openSpy = vi.spyOn(fs, "open").mockImplementation((async (...args: unknown[]) => { + const filePath = args[0]; + if (!swapped && filePath === targetPath) { + swapped = true; + nodeFs.rmSync(path.join(workspaceDir, "subdir"), { recursive: true, force: true }); + nodeFs.symlinkSync(outsideDir, path.join(workspaceDir, "subdir")); + } + return await (originalOpen as (...delegated: unknown[]) => Promise)(...args); + }) as unknown as typeof fs.open); + // Force the fallback verification path even on Linux so the ancestor-walk + // guard is exercised directly. + const readlinkSpy = vi.spyOn(fs, "readlink").mockRejectedValue(new Error("fd path unavailable")); + + try { + await expect(bridge.readFile({ filePath: "subdir/secret.txt" })).rejects.toThrow( + "Sandbox boundary checks failed", + ); + expect(openSpy).toHaveBeenCalled(); + expect(readlinkSpy).toHaveBeenCalled(); + } finally { + readlinkSpy.mockRestore(); + openSpy.mockRestore(); + } + }); + + it("rejects fallback reads of a symlinked leaf when O_NOFOLLOW is unavailable", async () => { + const workspaceDir = await makeTempDir("openclaw-openshell-fs-"); + const outsideDir = await makeTempDir("openclaw-openshell-outside-"); + await fs.mkdir(path.join(workspaceDir, "subdir"), { recursive: true }); + await fs.writeFile(path.join(outsideDir, "secret.txt"), "outside", "utf8"); + // The workspace contains a symlink as the FINAL path component pointing + // out-of-root. On Windows `O_NOFOLLOW` is `undefined`, so `open` would + // silently traverse the symlink to the outside file; the ancestor walk + // must lstat the leaf in that case to fail closed. + await fs.symlink( + path.join(outsideDir, "secret.txt"), + path.join(workspaceDir, "subdir", "secret.txt"), + ); + + const backend = createMirrorBackendMock(); + const sandbox = createSandboxTestContext({ + overrides: { + backendId: "openshell", + workspaceDir, + agentWorkspaceDir: workspaceDir, + containerWorkdir: "/sandbox", + }, + }); + + const { createOpenShellFsBridge, setReadOpenFlagsResolverForTest } = await import( + "./fs-bridge.js" + ); + const bridge = createOpenShellFsBridge({ sandbox, backend }); + // Force the fallback path so the leaf-lstat guard is exercised. + const readlinkSpy = vi.spyOn(fs, "readlink").mockRejectedValue(new Error("fd path unavailable")); + // Simulate a host that lacks `O_NOFOLLOW` (e.g. Windows) without touching + // the non-configurable native `fs.constants` data property. The bridge + // exposes a test-only seam for exactly this case. + setReadOpenFlagsResolverForTest(() => ({ + flags: nodeFs.constants.O_RDONLY, + supportsNoFollow: false, + })); + + try { + await expect(bridge.readFile({ filePath: "subdir/secret.txt" })).rejects.toThrow( + "Sandbox boundary checks failed", + ); + expect(readlinkSpy).toHaveBeenCalled(); + } finally { + setReadOpenFlagsResolverForTest(undefined); + readlinkSpy.mockRestore(); + } + }); + + it("rejects hardlinked files inside the sandbox root", async () => { + const workspaceDir = await makeTempDir("openclaw-openshell-fs-"); + const outsideDir = await makeTempDir("openclaw-openshell-outside-"); + await fs.mkdir(path.join(workspaceDir, "subdir"), { recursive: true }); + await fs.writeFile(path.join(outsideDir, "secret.txt"), "outside", "utf8"); + await fs.link( + path.join(outsideDir, "secret.txt"), + path.join(workspaceDir, "subdir", "secret.txt"), + ); + + const backend = createMirrorBackendMock(); + const sandbox = createSandboxTestContext({ + overrides: { + backendId: "openshell", + workspaceDir, + agentWorkspaceDir: workspaceDir, + containerWorkdir: "/sandbox", + }, + }); + + const { createOpenShellFsBridge } = await import("./fs-bridge.js"); + const bridge = createOpenShellFsBridge({ sandbox, backend }); + + await expect(bridge.readFile({ filePath: "subdir/secret.txt" })).rejects.toThrow( + "Sandbox boundary checks failed", + ); + }); + it("maps agent mount paths when the sandbox workspace is read-only", async () => { const workspaceDir = await makeTempDir("openclaw-openshell-fs-"); const agentWorkspaceDir = await makeTempDir("openclaw-openshell-agent-");