diff --git a/CHANGELOG.md b/CHANGELOG.md index b17936fa2..456764a2e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,7 @@ Docs: https://docs.openclaw.ai - Media: accept `MEDIA:`-prefixed paths (lenient whitespace) when loading outbound media to prevent `ENOENT` for tool-returned local media paths. (#13107) Thanks @mcaxtr. - Agents/Image tool: allow workspace-local image paths by including the active workspace directory in local media allowlists, and trust sandbox-validated paths in image loaders to prevent false "not under an allowed directory" rejections. (#15541) - Agents/Image tool: propagate the effective workspace root into tool wiring so workspace-local image paths are accepted by default when running without an explicit `workspaceDir`. (#16722) +- Media/Security: harden local media allowlist bypasses by requiring an explicit `readFile` override when callers mark paths as validated, and reject filesystem-root `localRoots` entries. (#16739) - Cron/Slack: preserve agent identity (name and icon) when cron jobs deliver outbound messages. (#16242) Thanks @robbyczgw-cla. - Cron: prevent `cron list`/`cron status` from silently skipping past-due recurring jobs by using maintenance recompute semantics. (#16156) Thanks @zerone0x. - Cron: repair missing/corrupt `nextRunAtMs` for the updated job without globally recomputing unrelated due jobs during `cron update`. (#15750) diff --git a/src/agents/openclaw-tools.ts b/src/agents/openclaw-tools.ts index 4825ebccb..eed73b85c 100644 --- a/src/agents/openclaw-tools.ts +++ b/src/agents/openclaw-tools.ts @@ -19,6 +19,7 @@ import { createSessionsSendTool } from "./tools/sessions-send-tool.js"; import { createSessionsSpawnTool } from "./tools/sessions-spawn-tool.js"; import { createTtsTool } from "./tools/tts-tool.js"; import { createWebFetchTool, createWebSearchTool } from "./tools/web-tools.js"; +import { resolveWorkspaceRoot } from "./workspace-dir.js"; export function createOpenClawTools(options?: { sandboxBrowserBridgeUrl?: string; @@ -60,7 +61,7 @@ export function createOpenClawTools(options?: { /** If true, omit the message tool from the tool list. */ disableMessageTool?: boolean; }): AnyAgentTool[] { - const workspaceDir = options?.workspaceDir?.trim() || process.cwd(); + const workspaceDir = resolveWorkspaceRoot(options?.workspaceDir); const imageTool = options?.agentDir?.trim() ? createImageTool({ config: options?.config, diff --git a/src/agents/pi-embedded-runner/run/images.ts b/src/agents/pi-embedded-runner/run/images.ts index 9b2d09e36..83ed67058 100644 --- a/src/agents/pi-embedded-runner/run/images.ts +++ b/src/agents/pi-embedded-runner/run/images.ts @@ -211,7 +211,7 @@ export async function loadImageFromRef( const media = options?.sandbox ? await loadWebMedia(targetPath, { maxBytes: options.maxBytes, - localRoots: "any", + sandboxValidated: true, readFile: (filePath) => options.sandbox!.bridge.readFile({ filePath, cwd: options.sandbox!.root }), }) diff --git a/src/agents/pi-tools.ts b/src/agents/pi-tools.ts index 15809c1c4..413deb32e 100644 --- a/src/agents/pi-tools.ts +++ b/src/agents/pi-tools.ts @@ -53,6 +53,7 @@ import { collectExplicitAllowlist, resolveToolProfilePolicy, } from "./tool-policy.js"; +import { resolveWorkspaceRoot } from "./workspace-dir.js"; function isOpenAIProvider(provider?: string) { const normalized = provider?.trim().toLowerCase(); @@ -253,7 +254,7 @@ export function createOpenClawCodingTools(options?: { const sandboxRoot = sandbox?.workspaceDir; const sandboxFsBridge = sandbox?.fsBridge; const allowWorkspaceWrites = sandbox?.workspaceAccess !== "ro"; - const workspaceRoot = options?.workspaceDir ?? process.cwd(); + const workspaceRoot = resolveWorkspaceRoot(options?.workspaceDir); const workspaceOnly = fsConfig.workspaceOnly === true; const applyPatchConfig = execConfig.applyPatch; // Secure by default: apply_patch is workspace-contained unless explicitly disabled. diff --git a/src/agents/tools/image-tool.ts b/src/agents/tools/image-tool.ts index 849853402..896b74471 100644 --- a/src/agents/tools/image-tool.ts +++ b/src/agents/tools/image-tool.ts @@ -14,6 +14,7 @@ import { runWithImageModelFallback } from "../model-fallback.js"; import { resolveConfiguredModelRef } from "../model-selection.js"; import { ensureOpenClawModelsJson } from "../models-config.js"; import { discoverAuthStorage, discoverModels } from "../pi-model-discovery.js"; +import { normalizeWorkspaceDir } from "../workspace-dir.js"; import { coerceImageAssistantText, coerceImageModelConfig, @@ -354,20 +355,11 @@ export function createImageTool(options?: { const localRoots = (() => { const roots = getDefaultLocalRoots(); - const workspaceDir = options?.workspaceDir?.trim(); + const workspaceDir = normalizeWorkspaceDir(options?.workspaceDir); if (!workspaceDir) { return roots; } - const expanded = workspaceDir.startsWith("~") ? resolveUserPath(workspaceDir) : workspaceDir; - const resolved = path.resolve(expanded); - // Defensive: never allow "/" as an implicit media root. - if (resolved === path.parse(resolved).root) { - return roots; - } - if (!roots.includes(resolved)) { - roots.push(resolved); - } - return roots; + return Array.from(new Set([...roots, workspaceDir])); })(); return { @@ -460,7 +452,7 @@ export function createImageTool(options?: { : sandboxConfig ? await loadWebMedia(resolvedPath ?? resolvedImage, { maxBytes, - localRoots: "any", + sandboxValidated: true, readFile: (filePath) => sandboxConfig.bridge.readFile({ filePath, cwd: sandboxConfig.root }), }) diff --git a/src/agents/workspace-dir.ts b/src/agents/workspace-dir.ts new file mode 100644 index 000000000..4d9bdb40a --- /dev/null +++ b/src/agents/workspace-dir.ts @@ -0,0 +1,20 @@ +import path from "node:path"; +import { resolveUserPath } from "../utils.js"; + +export function normalizeWorkspaceDir(workspaceDir?: string): string | null { + const trimmed = workspaceDir?.trim(); + if (!trimmed) { + return null; + } + const expanded = trimmed.startsWith("~") ? resolveUserPath(trimmed) : trimmed; + const resolved = path.resolve(expanded); + // Refuse filesystem roots as "workspace" (too broad; almost always a bug). + if (resolved === path.parse(resolved).root) { + return null; + } + return resolved; +} + +export function resolveWorkspaceRoot(workspaceDir?: string): string { + return normalizeWorkspaceDir(workspaceDir) ?? process.cwd(); +} diff --git a/src/infra/outbound/message-action-params.ts b/src/infra/outbound/message-action-params.ts index bb57a12bb..a0c2c8350 100644 --- a/src/infra/outbound/message-action-params.ts +++ b/src/infra/outbound/message-action-params.ts @@ -1,3 +1,4 @@ +import fs from "node:fs/promises"; import path from "node:path"; import { fileURLToPath } from "node:url"; import type { @@ -200,8 +201,12 @@ async function hydrateAttachmentPayload(params: { channel: params.channel, accountId: params.accountId, }); - // localRoots: "any" — media paths are already validated by normalizeSandboxMediaList above. - const media = await loadWebMedia(mediaSource, maxBytes, { localRoots: "any" }); + // mediaSource already validated by normalizeSandboxMediaList; allow bypass but force explicit readFile. + const media = await loadWebMedia(mediaSource, { + maxBytes, + sandboxValidated: true, + readFile: (filePath: string) => fs.readFile(filePath), + }); params.args.buffer = media.buffer.toString("base64"); if (!contentTypeParam && media.contentType) { params.args.contentType = media.contentType; diff --git a/src/web/media.test.ts b/src/web/media.test.ts index 3573f5afb..4a04d28cb 100644 --- a/src/web/media.test.ts +++ b/src/web/media.test.ts @@ -329,10 +329,22 @@ describe("local media root guard", () => { }); it("allows any path when localRoots is 'any'", async () => { - const result = await loadWebMedia(tinyPngFile, 1024 * 1024, { localRoots: "any" }); + const result = await loadWebMedia(tinyPngFile, { + maxBytes: 1024 * 1024, + localRoots: "any", + readFile: (filePath) => fs.readFile(filePath), + }); expect(result.kind).toBe("image"); }); + it("rejects filesystem root entries in localRoots", async () => { + await expect( + loadWebMedia(tinyPngFile, 1024 * 1024, { + localRoots: [path.parse(tinyPngFile).root], + }), + ).rejects.toThrow(/refuses filesystem root/i); + }); + it("allows default OpenClaw state workspace and sandbox roots", async () => { const { STATE_DIR } = await import("../config/paths.js"); const readFile = vi.fn(async () => Buffer.from("generated-media")); diff --git a/src/web/media.ts b/src/web/media.ts index 95887cbb0..c6dd11d05 100644 --- a/src/web/media.ts +++ b/src/web/media.ts @@ -27,12 +27,14 @@ type WebMediaOptions = { maxBytes?: number; optimizeImages?: boolean; ssrfPolicy?: SsrFPolicy; - /** Allowed root directories for local path reads. "any" skips the check (caller already validated). */ - localRoots?: string[] | "any"; + /** Allowed root directories for local path reads. "any" is deprecated; prefer sandboxValidated + readFile. */ + localRoots?: readonly string[] | "any"; + /** Caller already validated the local path (sandbox/other guards); requires readFile override. */ + sandboxValidated?: boolean; readFile?: (filePath: string) => Promise; }; -export function getDefaultLocalRoots(): string[] { +export function getDefaultLocalRoots(): readonly string[] { return [ os.tmpdir(), path.join(STATE_DIR, "media"), @@ -44,7 +46,7 @@ export function getDefaultLocalRoots(): string[] { async function assertLocalMediaAllowed( mediaPath: string, - localRoots: string[] | "any" | undefined, + localRoots: readonly string[] | "any" | undefined, ): Promise { if (localRoots === "any") { return; @@ -64,6 +66,11 @@ async function assertLocalMediaAllowed( } catch { resolvedRoot = path.resolve(root); } + if (resolvedRoot === path.parse(resolvedRoot).root) { + throw new Error( + `Invalid localRoots entry (refuses filesystem root): ${root}. Pass a narrower directory.`, + ); + } if (resolved === resolvedRoot || resolved.startsWith(resolvedRoot + path.sep)) { return; } @@ -173,6 +180,7 @@ async function loadWebMediaInternal( optimizeImages = true, ssrfPolicy, localRoots, + sandboxValidated = false, readFile: readFileOverride, } = options; // Strip MEDIA: prefix used by agent tools (e.g. TTS) to tag media paths. @@ -275,8 +283,16 @@ async function loadWebMediaInternal( mediaUrl = resolveUserPath(mediaUrl); } + if ((sandboxValidated || localRoots === "any") && !readFileOverride) { + throw new Error( + "Refusing localRoots bypass without readFile override. Use sandboxValidated with readFile, or pass explicit localRoots.", + ); + } + // Guard local reads against allowed directory roots to prevent file exfiltration. - await assertLocalMediaAllowed(mediaUrl, localRoots); + if (!(sandboxValidated || localRoots === "any")) { + await assertLocalMediaAllowed(mediaUrl, localRoots); + } // Local path const data = readFileOverride ? await readFileOverride(mediaUrl) : await fs.readFile(mediaUrl); @@ -300,7 +316,7 @@ async function loadWebMediaInternal( export async function loadWebMedia( mediaUrl: string, maxBytesOrOptions?: number | WebMediaOptions, - options?: { ssrfPolicy?: SsrFPolicy; localRoots?: string[] | "any" }, + options?: { ssrfPolicy?: SsrFPolicy; localRoots?: readonly string[] | "any" }, ): Promise { if (typeof maxBytesOrOptions === "number" || maxBytesOrOptions === undefined) { return await loadWebMediaInternal(mediaUrl, { @@ -319,7 +335,7 @@ export async function loadWebMedia( export async function loadWebMediaRaw( mediaUrl: string, maxBytesOrOptions?: number | WebMediaOptions, - options?: { ssrfPolicy?: SsrFPolicy; localRoots?: string[] | "any" }, + options?: { ssrfPolicy?: SsrFPolicy; localRoots?: readonly string[] | "any" }, ): Promise { if (typeof maxBytesOrOptions === "number" || maxBytesOrOptions === undefined) { return await loadWebMediaInternal(mediaUrl, {