From a54bf71b4c0cbe554a84340b773df37ee8e959de Mon Sep 17 00:00:00 2001 From: Robin Waslander Date: Sat, 14 Mar 2026 00:27:58 +0100 Subject: [PATCH] fix(imessage): sanitize SCP remote path to prevent shell metacharacter injection References GHSA-g2f6-pwvx-r275. --- CHANGELOG.md | 1 + ...tage-sandbox-media.scp-remote-path.test.ts | 75 +++++++++++++++++++ src/auto-reply/reply/stage-sandbox-media.ts | 8 +- src/infra/scp-host.test.ts | 44 ++++++++++- src/infra/scp-host.ts | 24 ++++++ 5 files changed, 149 insertions(+), 3 deletions(-) create mode 100644 src/auto-reply/reply.stage-sandbox-media.scp-remote-path.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 2a08c7fb2e8..e02a0555fe0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ Docs: https://docs.openclaw.ai ### Fixes +- iMessage/remote attachments: reject unsafe remote attachment paths before spawning SCP, so sender-controlled filenames can no longer inject shell metacharacters into remote media staging. Thanks @lintsinghua. - Telegram/webhook auth: validate the Telegram webhook secret before reading or parsing request bodies, so unauthenticated requests are rejected immediately instead of consuming up to 1 MB first. Thanks @space08. - Build/plugin-sdk bundling: bundle plugin-sdk subpath entries in one shared build pass so published packages stop duplicating shared chunks and avoid the recent plugin-sdk memory blow-up. (#45426) Thanks @TarasShyn. - Browser/existing-session: accept text-only `list_pages` and `new_page` responses from Chrome DevTools MCP so live-session tab discovery and new-tab open flows keep working when the server omits structured page metadata. diff --git a/src/auto-reply/reply.stage-sandbox-media.scp-remote-path.test.ts b/src/auto-reply/reply.stage-sandbox-media.scp-remote-path.test.ts new file mode 100644 index 00000000000..d5d628421d9 --- /dev/null +++ b/src/auto-reply/reply.stage-sandbox-media.scp-remote-path.test.ts @@ -0,0 +1,75 @@ +import fs from "node:fs/promises"; +import { basename, join } from "node:path"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { + createSandboxMediaContexts, + createSandboxMediaStageConfig, + withSandboxMediaTempHome, +} from "./stage-sandbox-media.test-harness.js"; + +const sandboxMocks = vi.hoisted(() => ({ + ensureSandboxWorkspaceForSession: vi.fn(), +})); +const childProcessMocks = vi.hoisted(() => ({ + spawn: vi.fn(), +})); + +vi.mock("../agents/sandbox.js", () => sandboxMocks); +vi.mock("node:child_process", () => childProcessMocks); + +import { stageSandboxMedia } from "./reply/stage-sandbox-media.js"; + +afterEach(() => { + vi.restoreAllMocks(); + childProcessMocks.spawn.mockClear(); +}); + +function createRemoteStageParams(home: string): { + cfg: ReturnType; + workspaceDir: string; + sessionKey: string; + remoteCacheDir: string; +} { + const sessionKey = "agent:main:main"; + vi.mocked(sandboxMocks.ensureSandboxWorkspaceForSession).mockResolvedValue(null); + return { + cfg: createSandboxMediaStageConfig(home), + workspaceDir: join(home, "openclaw"), + sessionKey, + remoteCacheDir: join(home, ".openclaw", "media", "remote-cache", sessionKey), + }; +} + +function createRemoteContexts(remotePath: string) { + const { ctx, sessionCtx } = createSandboxMediaContexts(remotePath); + ctx.Provider = "imessage"; + ctx.MediaRemoteHost = "user@gateway-host"; + sessionCtx.Provider = "imessage"; + sessionCtx.MediaRemoteHost = "user@gateway-host"; + return { ctx, sessionCtx }; +} + +describe("stageSandboxMedia scp remote paths", () => { + it("rejects remote attachment filenames with shell metacharacters before spawning scp", async () => { + await withSandboxMediaTempHome("openclaw-triggers-", async (home) => { + const { cfg, workspaceDir, sessionKey, remoteCacheDir } = createRemoteStageParams(home); + const remotePath = "/Users/demo/Library/Messages/Attachments/ab/cd/evil$(touch pwned).jpg"; + const { ctx, sessionCtx } = createRemoteContexts(remotePath); + + await stageSandboxMedia({ + ctx, + sessionCtx, + cfg, + sessionKey, + workspaceDir, + }); + + expect(childProcessMocks.spawn).not.toHaveBeenCalled(); + await expect(fs.stat(join(remoteCacheDir, basename(remotePath)))).rejects.toThrow(); + expect(ctx.MediaPath).toBe(remotePath); + expect(sessionCtx.MediaPath).toBe(remotePath); + expect(ctx.MediaUrl).toBe(remotePath); + expect(sessionCtx.MediaUrl).toBe(remotePath); + }); + }); +}); diff --git a/src/auto-reply/reply/stage-sandbox-media.ts b/src/auto-reply/reply/stage-sandbox-media.ts index d364fa6a554..3d3dec1738f 100644 --- a/src/auto-reply/reply/stage-sandbox-media.ts +++ b/src/auto-reply/reply/stage-sandbox-media.ts @@ -7,7 +7,7 @@ import { ensureSandboxWorkspaceForSession } from "../../agents/sandbox.js"; import type { OpenClawConfig } from "../../config/config.js"; import { logVerbose } from "../../globals.js"; import { copyFileWithinRoot, SafeOpenError } from "../../infra/fs-safe.js"; -import { normalizeScpRemoteHost } from "../../infra/scp-host.js"; +import { normalizeScpRemoteHost, normalizeScpRemotePath } from "../../infra/scp-host.js"; import { resolvePreferredOpenClawTmpDir } from "../../infra/tmp-openclaw-dir.js"; import { isInboundPathAllowed, @@ -293,6 +293,10 @@ async function scpFile(remoteHost: string, remotePath: string, localPath: string if (!safeRemoteHost) { throw new Error("invalid remote host for SCP"); } + const safeRemotePath = normalizeScpRemotePath(remotePath); + if (!safeRemotePath) { + throw new Error("invalid remote path for SCP"); + } return new Promise((resolve, reject) => { const child = spawn( "/usr/bin/scp", @@ -302,7 +306,7 @@ async function scpFile(remoteHost: string, remotePath: string, localPath: string "-o", "StrictHostKeyChecking=yes", "--", - `${safeRemoteHost}:${remotePath}`, + `${safeRemoteHost}:${safeRemotePath}`, localPath, ], { stdio: ["ignore", "ignore", "pipe"] }, diff --git a/src/infra/scp-host.test.ts b/src/infra/scp-host.test.ts index 78498b997ce..ab8517b5f69 100644 --- a/src/infra/scp-host.test.ts +++ b/src/infra/scp-host.test.ts @@ -1,5 +1,10 @@ import { describe, expect, it } from "vitest"; -import { isSafeScpRemoteHost, normalizeScpRemoteHost } from "./scp-host.js"; +import { + isSafeScpRemoteHost, + isSafeScpRemotePath, + normalizeScpRemoteHost, + normalizeScpRemotePath, +} from "./scp-host.js"; describe("scp remote host", () => { it.each([ @@ -33,3 +38,40 @@ describe("scp remote host", () => { expect(isSafeScpRemoteHost(value)).toBe(false); }); }); + +describe("scp remote path", () => { + it.each([ + { + value: "/Users/demo/Library/Messages/Attachments/ab/cd/photo.jpg", + expected: "/Users/demo/Library/Messages/Attachments/ab/cd/photo.jpg", + }, + { + value: " /Users/demo/Library/Messages/Attachments/ab/cd/IMG 1234 (1).jpg ", + expected: "/Users/demo/Library/Messages/Attachments/ab/cd/IMG 1234 (1).jpg", + }, + ])("normalizes safe paths for %j", ({ value, expected }) => { + expect(normalizeScpRemotePath(value)).toBe(expected); + expect(isSafeScpRemotePath(value)).toBe(true); + }); + + it.each([ + null, + undefined, + "", + " ", + "relative/path.jpg", + "/Users/demo/Library/Messages/Attachments/ab/cd/bad$path.jpg", + "/Users/demo/Library/Messages/Attachments/ab/cd/bad`path`.jpg", + "/Users/demo/Library/Messages/Attachments/ab/cd/bad;path.jpg", + "/Users/demo/Library/Messages/Attachments/ab/cd/bad|path.jpg", + "/Users/demo/Library/Messages/Attachments/ab/cd/bad&path.jpg", + "/Users/demo/Library/Messages/Attachments/ab/cd/badpath.jpg", + '/Users/demo/Library/Messages/Attachments/ab/cd/bad"path.jpg', + "/Users/demo/Library/Messages/Attachments/ab/cd/bad'path.jpg", + "/Users/demo/Library/Messages/Attachments/ab/cd/bad\\path.jpg", + ])("rejects unsafe path tokens: %j", (value) => { + expect(normalizeScpRemotePath(value)).toBeUndefined(); + expect(isSafeScpRemotePath(value)).toBe(false); + }); +}); diff --git a/src/infra/scp-host.ts b/src/infra/scp-host.ts index fb81171af4a..8b09163c202 100644 --- a/src/infra/scp-host.ts +++ b/src/infra/scp-host.ts @@ -1,6 +1,7 @@ const SSH_TOKEN = /^[A-Za-z0-9._-]+$/; const BRACKETED_IPV6 = /^\[[0-9A-Fa-f:.%]+\]$/; const WHITESPACE = /\s/; +const SCP_REMOTE_PATH_UNSAFE_CHARS = new Set(["\\", "'", '"', "`", "$", ";", "|", "&", "<", ">"]); function hasControlOrWhitespace(value: string): boolean { for (const char of value) { @@ -60,3 +61,26 @@ export function normalizeScpRemoteHost(value: string | null | undefined): string export function isSafeScpRemoteHost(value: string | null | undefined): boolean { return normalizeScpRemoteHost(value) !== undefined; } + +export function normalizeScpRemotePath(value: string | null | undefined): string | undefined { + if (typeof value !== "string") { + return undefined; + } + const trimmed = value.trim(); + if (!trimmed || !trimmed.startsWith("/")) { + return undefined; + } + + for (const char of trimmed) { + const code = char.charCodeAt(0); + if (code <= 0x1f || code === 0x7f || SCP_REMOTE_PATH_UNSAFE_CHARS.has(char)) { + return undefined; + } + } + + return trimmed; +} + +export function isSafeScpRemotePath(value: string | null | undefined): boolean { + return normalizeScpRemotePath(value) !== undefined; +}