fix: harden sandbox media reads against TOCTOU escapes

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.

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);
 

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);

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",
+    });
+  });
+});

src/agents/sandbox-media-paths.ts

@@ -8,6 +8,16 @@ export type SandboxedBridgeMediaPathConfig = {
   workspaceOnly?: boolean;
 };
 
+export function createSandboxBridgeReadFile(params: {
+  sandbox: Pick<SandboxedBridgeMediaPathConfig, "root" | "bridge">;
+}): (filePath: string) => Promise<Buffer> {
+  return async (filePath: string) =>
+    await params.sandbox.bridge.readFile({
+      filePath,
+      cwd: params.sandbox.root,
+    });
+}
+
 export async function resolveSandboxedBridgeMediaPath(params: {
   sandbox: SandboxedBridgeMediaPathConfig;
   mediaPath: string;

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,

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-");

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<SafeLocalReadResult> {
+  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<SafeLocalReadResult> {
+  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<Buffer> {
+  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;

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<string, unknown> = {
+        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 });
+    }
+  });
+});

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 {

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<typeof import("../infra/fs-safe.js")>();
   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"),
     );
 

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");
     }