From c823a85302119044bc26763644c15fe35dd04f4a Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Mon, 2 Mar 2026 01:03:40 +0000 Subject: [PATCH] fix: harden sandbox media reads against TOCTOU escapes --- CHANGELOG.md | 1 + src/agents/pi-embedded-runner/run/images.ts | 8 ++- src/agents/pi-tools.read.ts | 15 +++-- src/agents/sandbox-media-paths.test.ts | 22 +++++++ src/agents/sandbox-media-paths.ts | 10 +++ src/agents/tools/image-tool.ts | 4 +- src/infra/fs-safe.test.ts | 34 ++++++++++ src/infra/fs-safe.ts | 65 +++++++++++++++++++ .../outbound/message-action-params.test.ts | 57 ++++++++++++++++ src/infra/outbound/message-action-params.ts | 7 +- src/media/server.outside-workspace.test.ts | 6 +- src/media/server.ts | 21 +++--- 12 files changed, 223 insertions(+), 27 deletions(-) create mode 100644 src/agents/sandbox-media-paths.test.ts create mode 100644 src/infra/outbound/message-action-params.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 6f693d768..b9f9dc4ab 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -94,6 +94,7 @@ Docs: https://docs.openclaw.ai ### Fixes +- 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/Node metadata policy: harden node platform classification against Unicode confusables and switch unknown platform defaults to a conservative allowlist that excludes `system.run`/`system.which` unless explicitly allowlisted, preventing metadata canonicalization drift from broadening node command permissions. Thanks @tdjackey for reporting. - Plugins/Discovery precedence: load bundled plugins before auto-discovered global extensions so bundled channel plugins win duplicate-ID resolution by default (explicit `plugins.load.paths` overrides remain highest precedence), with loader regression coverage. Landed from contributor PR #29710 by @Sid-Qin. Thanks @Sid-Qin. - Discord/Reconnect integrity: release Discord message listener lane immediately while preserving serialized handler execution, add HELLO-stall resume-first recovery with bounded fresh-identify fallback after repeated stalls, and extend lifecycle/listener regression coverage for forced reconnect scenarios. Landed from contributor PR #29508 by @cgdusek. Thanks @cgdusek. diff --git a/src/agents/pi-embedded-runner/run/images.ts b/src/agents/pi-embedded-runner/run/images.ts index bcd25e724..caf78f739 100644 --- a/src/agents/pi-embedded-runner/run/images.ts +++ b/src/agents/pi-embedded-runner/run/images.ts @@ -4,7 +4,10 @@ import type { ImageContent } from "@mariozechner/pi-ai"; import { resolveUserPath } from "../../../utils.js"; import { loadWebMedia } from "../../../web/media.js"; import type { ImageSanitizationLimits } from "../../image-sanitization.js"; -import { resolveSandboxedBridgeMediaPath } from "../../sandbox-media-paths.js"; +import { + createSandboxBridgeReadFile, + resolveSandboxedBridgeMediaPath, +} from "../../sandbox-media-paths.js"; import { assertSandboxPath } from "../../sandbox-paths.js"; import type { SandboxFsBridge } from "../../sandbox/fs-bridge.js"; import { sanitizeImageBlocks } from "../../tool-images.js"; @@ -235,8 +238,7 @@ export async function loadImageFromRef( ? await loadWebMedia(targetPath, { maxBytes: options.maxBytes, sandboxValidated: true, - readFile: (filePath) => - options.sandbox!.bridge.readFile({ filePath, cwd: options.sandbox!.root }), + readFile: createSandboxBridgeReadFile({ sandbox: options.sandbox }), }) : await loadWebMedia(targetPath, options?.maxBytes); diff --git a/src/agents/pi-tools.read.ts b/src/agents/pi-tools.read.ts index f4a31d604..de3fcd166 100644 --- a/src/agents/pi-tools.read.ts +++ b/src/agents/pi-tools.read.ts @@ -3,7 +3,12 @@ import path from "node:path"; import { fileURLToPath } from "node:url"; import type { AgentToolResult } from "@mariozechner/pi-agent-core"; import { createEditTool, createReadTool, createWriteTool } from "@mariozechner/pi-coding-agent"; -import { SafeOpenError, openFileWithinRoot, writeFileWithinRoot } from "../infra/fs-safe.js"; +import { + SafeOpenError, + openFileWithinRoot, + readFileWithinRoot, + writeFileWithinRoot, +} from "../infra/fs-safe.js"; import { detectMime } from "../media/mime.js"; import { sniffMimeFromBase64 } from "../media/sniff-mime-from-base64.js"; import type { ImageSanitizationLimits } from "./image-sanitization.js"; @@ -823,15 +828,11 @@ function createHostEditOperations(root: string, options?: { workspaceOnly?: bool return { readFile: async (absolutePath: string) => { const relative = toRelativePathInRoot(root, absolutePath); - const opened = await openFileWithinRoot({ + const safeRead = await readFileWithinRoot({ rootDir: root, relativePath: relative, }); - try { - return await opened.handle.readFile(); - } finally { - await opened.handle.close().catch(() => {}); - } + return safeRead.buffer; }, writeFile: async (absolutePath: string, content: string) => { const relative = toRelativePathInRoot(root, absolutePath); diff --git a/src/agents/sandbox-media-paths.test.ts b/src/agents/sandbox-media-paths.test.ts new file mode 100644 index 000000000..4179c2a68 --- /dev/null +++ b/src/agents/sandbox-media-paths.test.ts @@ -0,0 +1,22 @@ +import { describe, expect, it, vi } from "vitest"; +import { createSandboxBridgeReadFile } from "./sandbox-media-paths.js"; +import type { SandboxFsBridge } from "./sandbox/fs-bridge.js"; + +describe("createSandboxBridgeReadFile", () => { + it("delegates reads through the sandbox bridge with sandbox root cwd", async () => { + const readFile = vi.fn(async () => Buffer.from("ok")); + const scopedRead = createSandboxBridgeReadFile({ + sandbox: { + root: "/tmp/sandbox-root", + bridge: { + readFile, + } as unknown as SandboxFsBridge, + }, + }); + await expect(scopedRead("media/inbound/example.png")).resolves.toEqual(Buffer.from("ok")); + expect(readFile).toHaveBeenCalledWith({ + filePath: "media/inbound/example.png", + cwd: "/tmp/sandbox-root", + }); + }); +}); diff --git a/src/agents/sandbox-media-paths.ts b/src/agents/sandbox-media-paths.ts index b4b0f7b30..3c6b2614c 100644 --- a/src/agents/sandbox-media-paths.ts +++ b/src/agents/sandbox-media-paths.ts @@ -8,6 +8,16 @@ export type SandboxedBridgeMediaPathConfig = { workspaceOnly?: boolean; }; +export function createSandboxBridgeReadFile(params: { + sandbox: Pick; +}): (filePath: string) => Promise { + return async (filePath: string) => + await params.sandbox.bridge.readFile({ + filePath, + cwd: params.sandbox.root, + }); +} + export async function resolveSandboxedBridgeMediaPath(params: { sandbox: SandboxedBridgeMediaPathConfig; mediaPath: string; diff --git a/src/agents/tools/image-tool.ts b/src/agents/tools/image-tool.ts index d186744ef..7bb479cbd 100644 --- a/src/agents/tools/image-tool.ts +++ b/src/agents/tools/image-tool.ts @@ -12,6 +12,7 @@ import { resolveConfiguredModelRef } from "../model-selection.js"; import { ensureOpenClawModelsJson } from "../models-config.js"; import { discoverAuthStorage, discoverModels } from "../pi-model-discovery.js"; import { + createSandboxBridgeReadFile, resolveSandboxedBridgeMediaPath, type SandboxedBridgeMediaPathConfig, } from "../sandbox-media-paths.js"; @@ -496,8 +497,7 @@ export function createImageTool(options?: { ? await loadWebMedia(resolvedPath ?? resolvedImage, { maxBytes, sandboxValidated: true, - readFile: (filePath) => - sandboxConfig.bridge.readFile({ filePath, cwd: sandboxConfig.root }), + readFile: createSandboxBridgeReadFile({ sandbox: sandboxConfig }), }) : await loadWebMedia(resolvedPath ?? resolvedImage, { maxBytes, diff --git a/src/infra/fs-safe.test.ts b/src/infra/fs-safe.test.ts index 359348ed3..5c676fb76 100644 --- a/src/infra/fs-safe.test.ts +++ b/src/infra/fs-safe.test.ts @@ -3,8 +3,11 @@ import path from "node:path"; import { afterEach, describe, expect, it } from "vitest"; import { createTrackedTempDirs } from "../test-utils/tracked-temp-dirs.js"; import { + createRootScopedReadFile, SafeOpenError, openFileWithinRoot, + readFileWithinRoot, + readPathWithinRoot, readLocalFileSafely, writeFileWithinRoot, } from "./fs-safe.js"; @@ -70,6 +73,37 @@ describe("fs-safe", () => { ).rejects.toMatchObject({ code: "outside-workspace" }); }); + it("reads a file within root", async () => { + const root = await tempDirs.make("openclaw-fs-safe-root-"); + await fs.writeFile(path.join(root, "inside.txt"), "inside"); + const result = await readFileWithinRoot({ + rootDir: root, + relativePath: "inside.txt", + }); + expect(result.buffer.toString("utf8")).toBe("inside"); + expect(result.realPath).toContain("inside.txt"); + expect(result.stat.size).toBe(6); + }); + + it("reads an absolute path within root via readPathWithinRoot", async () => { + const root = await tempDirs.make("openclaw-fs-safe-root-"); + const insidePath = path.join(root, "absolute.txt"); + await fs.writeFile(insidePath, "absolute"); + const result = await readPathWithinRoot({ + rootDir: root, + filePath: insidePath, + }); + expect(result.buffer.toString("utf8")).toBe("absolute"); + }); + + it("creates a root-scoped read callback", async () => { + const root = await tempDirs.make("openclaw-fs-safe-root-"); + const insidePath = path.join(root, "scoped.txt"); + await fs.writeFile(insidePath, "scoped"); + const readScoped = createRootScopedReadFile({ rootDir: root }); + await expect(readScoped(insidePath)).resolves.toEqual(Buffer.from("scoped")); + }); + it.runIf(process.platform !== "win32")("blocks symlink escapes under root", async () => { const root = await tempDirs.make("openclaw-fs-safe-root-"); const outside = await tempDirs.make("openclaw-fs-safe-outside-"); diff --git a/src/infra/fs-safe.ts b/src/infra/fs-safe.ts index f7d1f97d6..f3ae01b01 100644 --- a/src/infra/fs-safe.ts +++ b/src/infra/fs-safe.ts @@ -165,6 +165,71 @@ export async function openFileWithinRoot(params: { return opened; } +export async function readFileWithinRoot(params: { + rootDir: string; + relativePath: string; + rejectHardlinks?: boolean; + maxBytes?: number; +}): Promise { + const opened = await openFileWithinRoot({ + rootDir: params.rootDir, + relativePath: params.relativePath, + rejectHardlinks: params.rejectHardlinks, + }); + try { + if (params.maxBytes !== undefined && opened.stat.size > params.maxBytes) { + throw new SafeOpenError( + "too-large", + `file exceeds limit of ${params.maxBytes} bytes (got ${opened.stat.size})`, + ); + } + const buffer = await opened.handle.readFile(); + return { + buffer, + realPath: opened.realPath, + stat: opened.stat, + }; + } finally { + await opened.handle.close().catch(() => {}); + } +} + +export async function readPathWithinRoot(params: { + rootDir: string; + filePath: string; + rejectHardlinks?: boolean; + maxBytes?: number; +}): Promise { + const rootDir = path.resolve(params.rootDir); + const candidatePath = path.isAbsolute(params.filePath) + ? path.resolve(params.filePath) + : path.resolve(rootDir, params.filePath); + const relativePath = path.relative(rootDir, candidatePath); + return await readFileWithinRoot({ + rootDir, + relativePath, + rejectHardlinks: params.rejectHardlinks, + maxBytes: params.maxBytes, + }); +} + +export function createRootScopedReadFile(params: { + rootDir: string; + rejectHardlinks?: boolean; + maxBytes?: number; +}): (filePath: string) => Promise { + const rootDir = path.resolve(params.rootDir); + return async (filePath: string) => { + const safeRead = await readPathWithinRoot({ + rootDir, + filePath, + rejectHardlinks: params.rejectHardlinks, + maxBytes: params.maxBytes, + }); + return safeRead.buffer; + }; +} + export async function readLocalFileSafely(params: { filePath: string; maxBytes?: number; diff --git a/src/infra/outbound/message-action-params.test.ts b/src/infra/outbound/message-action-params.test.ts new file mode 100644 index 000000000..996db9682 --- /dev/null +++ b/src/infra/outbound/message-action-params.test.ts @@ -0,0 +1,57 @@ +import fs from "node:fs/promises"; +import os from "node:os"; +import path from "node:path"; +import { describe, expect, it } from "vitest"; +import type { OpenClawConfig } from "../../config/config.js"; +import { + hydrateAttachmentParamsForAction, + normalizeSandboxMediaParams, +} from "./message-action-params.js"; + +const cfg = {} as OpenClawConfig; +const maybeIt = process.platform === "win32" ? it.skip : it; + +describe("message action sandbox media hydration", () => { + maybeIt("rejects symlink retarget escapes after sandbox media normalization", async () => { + const sandboxRoot = await fs.mkdtemp(path.join(os.tmpdir(), "msg-params-sandbox-")); + const outsideRoot = await fs.mkdtemp(path.join(os.tmpdir(), "msg-params-outside-")); + try { + const insideDir = path.join(sandboxRoot, "inside"); + await fs.mkdir(insideDir, { recursive: true }); + await fs.writeFile(path.join(insideDir, "note.txt"), "INSIDE_SECRET", "utf8"); + await fs.writeFile(path.join(outsideRoot, "note.txt"), "OUTSIDE_SECRET", "utf8"); + + const slotLink = path.join(sandboxRoot, "slot"); + await fs.symlink(insideDir, slotLink); + + const args: Record = { + media: "slot/note.txt", + }; + const mediaPolicy = { + mode: "sandbox", + sandboxRoot, + } as const; + + await normalizeSandboxMediaParams({ + args, + mediaPolicy, + }); + + await fs.rm(slotLink, { recursive: true, force: true }); + await fs.symlink(outsideRoot, slotLink); + + await expect( + hydrateAttachmentParamsForAction({ + cfg, + channel: "slack", + args, + action: "sendAttachment", + mediaPolicy, + }), + ).rejects.toThrow(/outside workspace root|outside/i); + } finally { + await fs.rm(sandboxRoot, { recursive: true, force: true }); + await fs.rm(outsideRoot, { recursive: true, force: true }); + } + }); +}); diff --git a/src/infra/outbound/message-action-params.ts b/src/infra/outbound/message-action-params.ts index a73912edc..bdc1cdedc 100644 --- a/src/infra/outbound/message-action-params.ts +++ b/src/infra/outbound/message-action-params.ts @@ -1,4 +1,3 @@ -import fs from "node:fs/promises"; import path from "node:path"; import { fileURLToPath } from "node:url"; import { assertMediaNotDataUrl, resolveSandboxedMediaSource } from "../../agents/sandbox-paths.js"; @@ -9,6 +8,7 @@ import type { ChannelThreadingToolContext, } from "../../channels/plugins/types.js"; import type { OpenClawConfig } from "../../config/config.js"; +import { createRootScopedReadFile } from "../../infra/fs-safe.js"; import { extensionForMime } from "../../media/mime.js"; import { parseSlackTarget } from "../../slack/targets.js"; import { parseTelegramTarget } from "../../telegram/targets.js"; @@ -210,10 +210,13 @@ function buildAttachmentMediaLoadOptions(params: { localRoots?: readonly string[]; } { if (params.policy.mode === "sandbox") { + const readSandboxFile = createRootScopedReadFile({ + rootDir: params.policy.sandboxRoot.trim(), + }); return { maxBytes: params.maxBytes, sandboxValidated: true, - readFile: (filePath: string) => fs.readFile(filePath), + readFile: readSandboxFile, }; } return { diff --git a/src/media/server.outside-workspace.test.ts b/src/media/server.outside-workspace.test.ts index be626bf3e..0ae31de3e 100644 --- a/src/media/server.outside-workspace.test.ts +++ b/src/media/server.outside-workspace.test.ts @@ -5,7 +5,7 @@ import path from "node:path"; import { afterAll, beforeAll, describe, expect, it, vi } from "vitest"; const mocks = vi.hoisted(() => ({ - openFileWithinRoot: vi.fn(), + readFileWithinRoot: vi.fn(), cleanOldMedia: vi.fn().mockResolvedValue(undefined), })); @@ -15,7 +15,7 @@ vi.mock("../infra/fs-safe.js", async (importOriginal) => { const actual = await importOriginal(); return { ...actual, - openFileWithinRoot: mocks.openFileWithinRoot, + readFileWithinRoot: mocks.readFileWithinRoot, }; }); @@ -48,7 +48,7 @@ describe("media server outside-workspace mapping", () => { }); it("returns 400 with a specific outside-workspace message", async () => { - mocks.openFileWithinRoot.mockRejectedValueOnce( + mocks.readFileWithinRoot.mockRejectedValueOnce( new SafeOpenError("outside-workspace", "file is outside workspace root"), ); diff --git a/src/media/server.ts b/src/media/server.ts index 8f3cc8198..cdd1d27e3 100644 --- a/src/media/server.ts +++ b/src/media/server.ts @@ -2,7 +2,7 @@ import fs from "node:fs/promises"; import type { Server } from "node:http"; import express, { type Express } from "express"; import { danger } from "../globals.js"; -import { SafeOpenError, openFileWithinRoot } from "../infra/fs-safe.js"; +import { SafeOpenError, readFileWithinRoot } from "../infra/fs-safe.js"; import { defaultRuntime, type RuntimeEnv } from "../runtime.js"; import { detectMime } from "./mime.js"; import { cleanOldMedia, getMediaDir, MEDIA_MAX_BYTES } from "./store.js"; @@ -39,23 +39,20 @@ export function attachMediaRoutes( return; } try { - const { handle, realPath, stat } = await openFileWithinRoot({ + const { + buffer: data, + realPath, + stat, + } = await readFileWithinRoot({ rootDir: mediaDir, relativePath: id, + maxBytes: MAX_MEDIA_BYTES, }); - if (stat.size > MAX_MEDIA_BYTES) { - await handle.close().catch(() => {}); - res.status(413).send("too large"); - return; - } if (Date.now() - stat.mtimeMs > ttlMs) { - await handle.close().catch(() => {}); await fs.rm(realPath).catch(() => {}); res.status(410).send("expired"); return; } - const data = await handle.readFile(); - await handle.close().catch(() => {}); const mime = await detectMime({ buffer: data, filePath: realPath }); if (mime) { res.type(mime); @@ -87,6 +84,10 @@ export function attachMediaRoutes( res.status(404).send("not found"); return; } + if (err.code === "too-large") { + res.status(413).send("too large"); + return; + } } res.status(404).send("not found"); }