From f195af0b22a2e071a935938b6b4fd93a713a3917 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sun, 8 Mar 2026 00:40:41 +0000 Subject: [PATCH] fix(sandbox): anchor fs-bridge destructive ops --- CHANGELOG.md | 1 + src/agents/sandbox/fs-bridge.test.ts | 40 +++++++++++++++++++++++++++ src/agents/sandbox/fs-bridge.ts | 41 ++++++++++++++++++++++++---- 3 files changed, 77 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c8606fc67f6..79e5814d490 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -732,6 +732,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 `remove` and `rename` operations by anchoring destructive actions to verified canonical parent directories plus basenames instead of passing mutable full path strings to `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. - 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.test.ts b/src/agents/sandbox/fs-bridge.test.ts index 0b44729e5a4..0d59d5d604b 100644 --- a/src/agents/sandbox/fs-bridge.test.ts +++ b/src/agents/sandbox/fs-bridge.test.ts @@ -46,6 +46,12 @@ function findCallByScriptFragment(fragment: string) { return mockedExecDockerRaw.mock.calls.find(([args]) => getDockerScript(args).includes(fragment)); } +function findCallsByScriptFragment(fragment: string) { + return mockedExecDockerRaw.mock.calls.filter(([args]) => + getDockerScript(args).includes(fragment), + ); +} + function dockerExecResult(stdout: string) { return { stdout: Buffer.from(stdout), @@ -244,6 +250,40 @@ describe("sandbox fs bridge shell compatibility", () => { expect(scripts.some((script) => script.includes('mv -f -- "$1" "$2"'))).toBe(true); }); + it("anchors remove operations on canonical parent + basename", async () => { + const bridge = createSandboxFsBridge({ sandbox: createSandbox() }); + + await bridge.remove({ filePath: "nested/file.txt" }); + + const removeCall = findCallByScriptFragment('rm -f -- "$2"'); + expect(removeCall).toBeDefined(); + const args = removeCall?.[0] ?? []; + expect(getDockerArg(args, 1)).toBe("/workspace/nested"); + expect(getDockerArg(args, 2)).toBe("file.txt"); + expect(args).not.toContain("/workspace/nested/file.txt"); + + const canonicalCalls = findCallsByScriptFragment('readlink -f -- "$cursor"'); + expect( + canonicalCalls.some(([callArgs]) => getDockerArg(callArgs, 1) === "/workspace/nested"), + ).toBe(true); + }); + + it("anchors rename operations on canonical parents + basenames", async () => { + const bridge = createSandboxFsBridge({ sandbox: createSandbox() }); + + await bridge.rename({ from: "from.txt", to: "nested/to.txt" }); + + const renameCall = findCallByScriptFragment('mv -- "$3" "$2/$4"'); + expect(renameCall).toBeDefined(); + const args = renameCall?.[0] ?? []; + expect(getDockerArg(args, 1)).toBe("/workspace"); + expect(getDockerArg(args, 2)).toBe("/workspace/nested"); + expect(getDockerArg(args, 3)).toBe("from.txt"); + expect(getDockerArg(args, 4)).toBe("to.txt"); + expect(args).not.toContain("/workspace/from.txt"); + expect(args).not.toContain("/workspace/nested/to.txt"); + }); + it("re-validates target before final rename and cleans temp file on failure", async () => { mockedOpenBoundaryFile .mockImplementationOnce(async () => ({ ok: false, reason: "path" })) diff --git a/src/agents/sandbox/fs-bridge.ts b/src/agents/sandbox/fs-bridge.ts index e1cca2912eb..70293e8404f 100644 --- a/src/agents/sandbox/fs-bridge.ts +++ b/src/agents/sandbox/fs-bridge.ts @@ -1,4 +1,5 @@ import fs from "node:fs"; +import path from "node:path"; import { openBoundaryFile } from "../../infra/boundary-file-read.js"; import { PATH_ALIAS_POLICIES, type PathAliasPolicy } from "../../infra/path-alias-guards.js"; import type { SafeOpenSyncAllowedType } from "../../infra/safe-open-sync.js"; @@ -31,6 +32,11 @@ type PathSafetyCheck = { options: PathSafetyOptions; }; +type AnchoredSandboxEntry = { + canonicalParentPath: string; + basename: string; +}; + export type SandboxResolvedPath = { hostPath: string; relativePath: string; @@ -175,6 +181,7 @@ class SandboxFsBridgeImpl implements SandboxFsBridge { }): Promise { const target = this.resolveResolvedPath(params); this.ensureWriteAccess(target, "remove files"); + const anchoredTarget = await this.resolveAnchoredSandboxEntry(target); const flags = [params.force === false ? "" : "-f", params.recursive ? "-r" : ""].filter( Boolean, ); @@ -191,8 +198,8 @@ class SandboxFsBridgeImpl implements SandboxFsBridge { }, ], recheckBeforeCommand: true, - script: `set -eu; ${rmCommand} -- "$1"`, - args: [target.containerPath], + script: `set -eu\ncd -- "$1"\n${rmCommand} -- "$2"`, + args: [anchoredTarget.canonicalParentPath, anchoredTarget.basename], signal: params.signal, }); } @@ -207,6 +214,8 @@ class SandboxFsBridgeImpl implements SandboxFsBridge { const to = this.resolveResolvedPath({ filePath: params.to, cwd: params.cwd }); this.ensureWriteAccess(from, "rename files"); this.ensureWriteAccess(to, "rename files"); + const anchoredFrom = await this.resolveAnchoredSandboxEntry(from); + const anchoredTo = await this.resolveAnchoredSandboxEntry(to); await this.runCheckedCommand({ checks: [ { @@ -226,9 +235,13 @@ class SandboxFsBridgeImpl implements SandboxFsBridge { }, ], recheckBeforeCommand: true, - script: - 'set -eu; dir=$(dirname -- "$2"); if [ "$dir" != "." ]; then mkdir -p -- "$dir"; fi; mv -- "$1" "$2"', - args: [from.containerPath, to.containerPath], + script: ["set -eu", 'mkdir -p -- "$2"', 'cd -- "$1"', 'mv -- "$3" "$2/$4"'].join("\n"), + args: [ + anchoredFrom.canonicalParentPath, + anchoredTo.canonicalParentPath, + anchoredFrom.basename, + anchoredTo.basename, + ], signal: params.signal, }); } @@ -416,6 +429,24 @@ class SandboxFsBridgeImpl implements SandboxFsBridge { return normalizeContainerPath(canonical); } + private async resolveAnchoredSandboxEntry( + target: SandboxResolvedFsPath, + ): Promise { + const basename = path.posix.basename(target.containerPath); + if (!basename || basename === "." || basename === "/") { + throw new Error(`Invalid sandbox entry target: ${target.containerPath}`); + } + const parentPath = normalizeContainerPath(path.posix.dirname(target.containerPath)); + const canonicalParentPath = await this.resolveCanonicalContainerPath({ + containerPath: parentPath, + allowFinalSymlinkForUnlink: false, + }); + return { + canonicalParentPath, + basename, + }; + } + private async writeFileToTempPath(params: { targetContainerPath: string; mkdir: boolean;