From 99cfd271d00b2ab0f7825bf7831de59e4ff6aec1 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sun, 8 Mar 2026 01:08:54 +0000 Subject: [PATCH] fix(sandbox): pin fs bridge readfile handles --- CHANGELOG.md | 2 +- src/agents/sandbox/fs-bridge-command-plans.ts | 8 -- src/agents/sandbox/fs-bridge-path-safety.ts | 41 +++++- src/agents/sandbox/fs-bridge.boundary.test.ts | 7 +- src/agents/sandbox/fs-bridge.shell.test.ts | 124 ++++++++++++------ src/agents/sandbox/fs-bridge.ts | 10 +- 6 files changed, 125 insertions(+), 67 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 584dcf42c2e..5976c42a221 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -742,7 +742,7 @@ Docs: https://docs.openclaw.ai - Slack/download-file scoping: thread/channel-aware `download-file` actions now propagate optional scope context and reject downloads when Slack metadata definitively shows the file is outside the requested channel/thread, while preserving legacy behavior when share metadata is unavailable. - Security/Sandbox media reads: eliminate sandbox media TOCTOU symlink-retarget escapes by enforcing root-scoped boundary-safe reads at attachment/image load time and consolidating shared safe-read helpers across sandbox media callsites. This ships in the next npm release. Thanks @tdjackey for reporting. - Security/Sandbox media staging: block destination symlink escapes in `stageSandboxMedia` by replacing direct destination copies with root-scoped safe writes for both local and SCP-staged attachments, preventing out-of-workspace file overwrite through `media/inbound` alias traversal. This ships in the next npm release (`2026.3.2`). Thanks @tdjackey for reporting. -- Security/Sandbox fs bridge: harden sandbox `mkdirp`, `remove`, and `rename` operations by anchoring filesystem changes to verified canonical parent directories plus basenames instead of passing mutable full path strings to `mkdir -p`, `rm`, and `mv`, reducing parent-directory symlink-rebind TOCTOU exposure in sandbox file operations. This ships in the next npm release. Thanks @tdjackey for reporting. +- Security/Sandbox fs bridge: harden sandbox `readFile`, `mkdirp`, `remove`, and `rename` operations by pinning reads to boundary-opened file descriptors and anchoring filesystem changes to verified canonical parent directories plus basenames instead of passing mutable full path strings to `mkdir -p`, `rm`, and `mv`, reducing TOCTOU race exposure in sandbox file operations. This ships in the next npm release. Thanks @tdjackey for reporting. - Node host/service auth env: include `OPENCLAW_GATEWAY_TOKEN` in `openclaw node install` service environments (with `CLAWDBOT_GATEWAY_TOKEN` compatibility fallback) so installed node services keep remote gateway token auth across restart/reboot. Fixes #31041. Thanks @OneStepAt4time for reporting, @byungsker, @liuxiaopai-ai, and @vincentkoc. - Security/Subagents sandbox inheritance: block sandboxed sessions from spawning cross-agent subagents that would run unsandboxed, preventing runtime sandbox downgrade via `sessions_spawn agentId`. Thanks @tdjackey for reporting. - Security/Workspace safe writes: harden `writeFileWithinRoot` against symlink-retarget TOCTOU races by opening existing files without truncation, creating missing files with exclusive create, deferring truncation until post-open identity+boundary validation, and removing out-of-root create artifacts on blocked races; added regression tests for truncate/create race paths. This ships in the next npm release (`2026.3.2`). Thanks @tdjackey for reporting. diff --git a/src/agents/sandbox/fs-bridge-command-plans.ts b/src/agents/sandbox/fs-bridge-command-plans.ts index f411cb81b8e..4c1a9b8d64f 100644 --- a/src/agents/sandbox/fs-bridge-command-plans.ts +++ b/src/agents/sandbox/fs-bridge-command-plans.ts @@ -10,14 +10,6 @@ export type SandboxFsCommandPlan = { allowFailure?: boolean; }; -export function buildReadFilePlan(target: SandboxResolvedFsPath): SandboxFsCommandPlan { - return { - checks: [{ target, options: { action: "read files" } }], - script: 'set -eu; cat -- "$1"', - args: [target.containerPath], - }; -} - export function buildWriteCommitPlan( target: SandboxResolvedFsPath, tempPath: string, diff --git a/src/agents/sandbox/fs-bridge-path-safety.ts b/src/agents/sandbox/fs-bridge-path-safety.ts index 2b8402260db..1c7330ca526 100644 --- a/src/agents/sandbox/fs-bridge-path-safety.ts +++ b/src/agents/sandbox/fs-bridge-path-safety.ts @@ -1,6 +1,6 @@ import fs from "node:fs"; import path from "node:path"; -import { openBoundaryFile } from "../../infra/boundary-file-read.js"; +import { openBoundaryFile, type BoundaryFileOpenResult } from "../../infra/boundary-file-read.js"; import type { PathAliasPolicy } from "../../infra/path-alias-guards.js"; import type { SafeOpenSyncAllowedType } from "../../infra/safe-open-sync.js"; import type { SandboxResolvedFsPath, SandboxFsMount } from "./fs-paths.js"; @@ -49,13 +49,40 @@ export class SandboxFsPathGuard { } async assertPathSafety(target: SandboxResolvedFsPath, options: PathSafetyOptions) { - const lexicalMount = this.resolveMountByContainerPath(target.containerPath); - if (!lexicalMount) { - throw new Error( - `Sandbox path escapes allowed mounts; cannot ${options.action}: ${target.containerPath}`, - ); - } + const lexicalMount = this.resolveRequiredMount(target.containerPath, options.action); + await this.assertPathSafetyWithinMount(target, options, lexicalMount); + } + async openReadableFile( + target: SandboxResolvedFsPath, + ): Promise { + const lexicalMount = this.resolveRequiredMount(target.containerPath, "read files"); + const opened = await openBoundaryFile({ + absolutePath: target.hostPath, + rootPath: lexicalMount.hostRoot, + boundaryLabel: "sandbox mount root", + }); + if (!opened.ok) { + throw opened.error instanceof Error + ? opened.error + : new Error(`Sandbox boundary checks failed; cannot read files: ${target.containerPath}`); + } + return opened; + } + + private resolveRequiredMount(containerPath: string, action: string): SandboxFsMount { + const lexicalMount = this.resolveMountByContainerPath(containerPath); + if (!lexicalMount) { + throw new Error(`Sandbox path escapes allowed mounts; cannot ${action}: ${containerPath}`); + } + return lexicalMount; + } + + private async assertPathSafetyWithinMount( + target: SandboxResolvedFsPath, + options: PathSafetyOptions, + lexicalMount: SandboxFsMount, + ) { const guarded = await openBoundaryFile({ absolutePath: target.hostPath, rootPath: lexicalMount.hostRoot, diff --git a/src/agents/sandbox/fs-bridge.boundary.test.ts b/src/agents/sandbox/fs-bridge.boundary.test.ts index dae841d3796..3b86496fac6 100644 --- a/src/agents/sandbox/fs-bridge.boundary.test.ts +++ b/src/agents/sandbox/fs-bridge.boundary.test.ts @@ -7,7 +7,6 @@ import { createSandboxFsBridge, expectMkdirpAllowsExistingDirectory, getScriptsFromCalls, - installDockerReadMock, installFsBridgeTestHarness, mockedExecDockerRaw, withTempDir, @@ -109,11 +108,9 @@ describe("sandbox fs bridge boundary validation", () => { }); }); - it("rejects container-canonicalized paths outside allowed mounts", async () => { - installDockerReadMock({ canonicalPath: "/etc/passwd" }); - + it("rejects missing files before any docker read command runs", async () => { const bridge = createSandboxFsBridge({ sandbox: createSandbox() }); - await expect(bridge.readFile({ filePath: "a.txt" })).rejects.toThrow(/escapes allowed mounts/i); + await expect(bridge.readFile({ filePath: "a.txt" })).rejects.toThrow(/ENOENT|no such file/i); const scripts = getScriptsFromCalls(); expect(scripts.some((script) => script.includes('cat -- "$1"'))).toBe(false); }); diff --git a/src/agents/sandbox/fs-bridge.shell.test.ts b/src/agents/sandbox/fs-bridge.shell.test.ts index dd61ceadd22..d8b29c0f5d5 100644 --- a/src/agents/sandbox/fs-bridge.shell.test.ts +++ b/src/agents/sandbox/fs-bridge.shell.test.ts @@ -1,41 +1,54 @@ +import fs from "node:fs/promises"; +import path from "node:path"; import { describe, expect, it } from "vitest"; import { createSandbox, createSandboxFsBridge, - findCallByScriptFragment, - getDockerPathArg, getScriptsFromCalls, installFsBridgeTestHarness, mockedExecDockerRaw, + withTempDir, } from "./fs-bridge.test-helpers.js"; describe("sandbox fs bridge shell compatibility", () => { installFsBridgeTestHarness(); it("uses POSIX-safe shell prologue in all bridge commands", async () => { - const bridge = createSandboxFsBridge({ sandbox: createSandbox() }); + await withTempDir("openclaw-fs-bridge-shell-", async (stateDir) => { + const workspaceDir = path.join(stateDir, "workspace"); + await fs.mkdir(workspaceDir, { recursive: true }); + await fs.writeFile(path.join(workspaceDir, "a.txt"), "hello"); + await fs.writeFile(path.join(workspaceDir, "b.txt"), "bye"); - await bridge.readFile({ filePath: "a.txt" }); - await bridge.writeFile({ filePath: "b.txt", data: "hello" }); - await bridge.mkdirp({ filePath: "nested" }); - await bridge.remove({ filePath: "b.txt" }); - await bridge.rename({ from: "a.txt", to: "c.txt" }); - await bridge.stat({ filePath: "c.txt" }); + const bridge = createSandboxFsBridge({ + sandbox: createSandbox({ + workspaceDir, + agentWorkspaceDir: workspaceDir, + }), + }); - expect(mockedExecDockerRaw).toHaveBeenCalled(); + await bridge.readFile({ filePath: "a.txt" }); + await bridge.writeFile({ filePath: "b.txt", data: "hello" }); + await bridge.mkdirp({ filePath: "nested" }); + await bridge.remove({ filePath: "b.txt" }); + await bridge.rename({ from: "a.txt", to: "c.txt" }); + await bridge.stat({ filePath: "c.txt" }); - const scripts = getScriptsFromCalls(); - const executables = mockedExecDockerRaw.mock.calls.map(([args]) => args[3] ?? ""); + expect(mockedExecDockerRaw).toHaveBeenCalled(); - expect(executables.every((shell) => shell === "sh")).toBe(true); - expect(scripts.every((script) => /set -eu[;\n]/.test(script))).toBe(true); - expect(scripts.some((script) => script.includes("pipefail"))).toBe(false); + const scripts = getScriptsFromCalls(); + const executables = mockedExecDockerRaw.mock.calls.map(([args]) => args[3] ?? ""); + + expect(executables.every((shell) => shell === "sh")).toBe(true); + expect(scripts.every((script) => /set -eu[;\n]/.test(script))).toBe(true); + expect(scripts.some((script) => script.includes("pipefail"))).toBe(false); + }); }); it("resolveCanonicalContainerPath script is valid POSIX sh (no do; token)", async () => { const bridge = createSandboxFsBridge({ sandbox: createSandbox() }); - await bridge.readFile({ filePath: "a.txt" }); + await bridge.mkdirp({ filePath: "nested" }); const scripts = getScriptsFromCalls(); const canonicalScript = scripts.find((script) => script.includes("allow_final")); @@ -45,44 +58,69 @@ describe("sandbox fs bridge shell compatibility", () => { }); it("reads inbound media-style filenames with triple-dash ids", async () => { - const bridge = createSandboxFsBridge({ sandbox: createSandbox() }); - const inboundPath = "media/inbound/file_1095---f00a04a2-99a0-4d98-99b0-dfe61c5a4198.ogg"; + await withTempDir("openclaw-fs-bridge-read-", async (stateDir) => { + const workspaceDir = path.join(stateDir, "workspace"); + const inboundPath = "media/inbound/file_1095---f00a04a2-99a0-4d98-99b0-dfe61c5a4198.ogg"; + await fs.mkdir(path.join(workspaceDir, "media", "inbound"), { recursive: true }); + await fs.writeFile(path.join(workspaceDir, inboundPath), "voice"); - await bridge.readFile({ filePath: inboundPath }); + const bridge = createSandboxFsBridge({ + sandbox: createSandbox({ + workspaceDir, + agentWorkspaceDir: workspaceDir, + }), + }); - const readCall = findCallByScriptFragment('cat -- "$1"'); - expect(readCall).toBeDefined(); - const readPath = readCall ? getDockerPathArg(readCall[0]) : ""; - expect(readPath).toContain("file_1095---"); + await expect(bridge.readFile({ filePath: inboundPath })).resolves.toEqual( + Buffer.from("voice"), + ); + expect(mockedExecDockerRaw).not.toHaveBeenCalled(); + }); }); it("resolves dash-leading basenames into absolute container paths", async () => { - const bridge = createSandboxFsBridge({ sandbox: createSandbox() }); + await withTempDir("openclaw-fs-bridge-read-", async (stateDir) => { + const workspaceDir = path.join(stateDir, "workspace"); + await fs.mkdir(workspaceDir, { recursive: true }); + await fs.writeFile(path.join(workspaceDir, "--leading.txt"), "dash"); - await bridge.readFile({ filePath: "--leading.txt" }); + const bridge = createSandboxFsBridge({ + sandbox: createSandbox({ + workspaceDir, + agentWorkspaceDir: workspaceDir, + }), + }); - const readCall = findCallByScriptFragment('cat -- "$1"'); - expect(readCall).toBeDefined(); - const readPath = readCall ? getDockerPathArg(readCall[0]) : ""; - expect(readPath).toBe("/workspace/--leading.txt"); + await expect(bridge.readFile({ filePath: "--leading.txt" })).resolves.toEqual( + Buffer.from("dash"), + ); + expect(mockedExecDockerRaw).not.toHaveBeenCalled(); + }); }); it("resolves bind-mounted absolute container paths for reads", async () => { - const sandbox = createSandbox({ - docker: { - ...createSandbox().docker, - binds: ["/tmp/workspace-two:/workspace-two:ro"], - }, + await withTempDir("openclaw-fs-bridge-bind-read-", async (stateDir) => { + const workspaceDir = path.join(stateDir, "workspace"); + const bindRoot = path.join(stateDir, "workspace-two"); + await fs.mkdir(workspaceDir, { recursive: true }); + await fs.mkdir(bindRoot, { recursive: true }); + await fs.writeFile(path.join(bindRoot, "README.md"), "bind-read"); + + const sandbox = createSandbox({ + workspaceDir, + agentWorkspaceDir: workspaceDir, + docker: { + ...createSandbox().docker, + binds: [`${bindRoot}:/workspace-two:ro`], + }, + }); + const bridge = createSandboxFsBridge({ sandbox }); + + await expect(bridge.readFile({ filePath: "/workspace-two/README.md" })).resolves.toEqual( + Buffer.from("bind-read"), + ); + expect(mockedExecDockerRaw).not.toHaveBeenCalled(); }); - const bridge = createSandboxFsBridge({ sandbox }); - - await bridge.readFile({ filePath: "/workspace-two/README.md" }); - - const args = mockedExecDockerRaw.mock.calls.at(-1)?.[0] ?? []; - expect(args).toEqual( - expect.arrayContaining(["moltbot-sbx-test", "sh", "-c", 'set -eu; cat -- "$1"']), - ); - expect(getDockerPathArg(args)).toBe("/workspace-two/README.md"); }); it("writes via temp file + atomic rename (never direct truncation)", async () => { diff --git a/src/agents/sandbox/fs-bridge.ts b/src/agents/sandbox/fs-bridge.ts index a52a8acfd76..37bf569e706 100644 --- a/src/agents/sandbox/fs-bridge.ts +++ b/src/agents/sandbox/fs-bridge.ts @@ -1,7 +1,7 @@ +import fs from "node:fs"; import { execDockerRaw, type ExecDockerRawResult } from "./docker.js"; import { buildMkdirpPlan, - buildReadFilePlan, buildRemovePlan, buildRenamePlan, buildStatPlan, @@ -99,8 +99,12 @@ class SandboxFsBridgeImpl implements SandboxFsBridge { signal?: AbortSignal; }): Promise { const target = this.resolveResolvedPath(params); - const result = await this.runPlannedCommand(buildReadFilePlan(target), params.signal); - return result.stdout; + const opened = await this.pathGuard.openReadableFile(target); + try { + return fs.readFileSync(opened.fd); + } finally { + fs.closeSync(opened.fd); + } } async writeFile(params: {