From 749e28dec796f77697398acbfc7a64d4439d7cad Mon Sep 17 00:00:00 2001 From: aether-ai-agent Date: Fri, 13 Feb 2026 22:28:43 +1100 Subject: [PATCH] fix(security): block dangerous tools from HTTP gateway and fix ACP auto-approval (OC-02) Two critical RCE vectors patched: Vector 1 - Gateway HTTP /tools/invoke: - Add DEFAULT_GATEWAY_HTTP_TOOL_DENY blocking sessions_spawn, sessions_send, gateway, whatsapp_login from HTTP invocation - Apply deny filter after existing policy cascade, before tool lookup - Add gateway.tools.{allow,deny} config override in GatewayConfig Vector 2 - ACP client auto-approval: - Replace blind allow_once selection with danger-aware permission handler - Dangerous tools (exec, sessions_spawn, etc.) require interactive confirmation - Safe tools retain auto-approve behavior (backward compatible) - Empty options array now denied (was hardcoded "allow") - 30s timeout auto-denies to prevent hung sessions CWE-78 | CVSS:3.1 9.8 Critical --- src/acp/client.test.ts | 60 +++++++++++++++++++ src/acp/client.ts | 86 ++++++++++++++++++++++++--- src/config/types.gateway.ts | 9 +++ src/gateway/tools-invoke-http.test.ts | 66 ++++++++++++++++++++ src/gateway/tools-invoke-http.ts | 26 +++++++- 5 files changed, 237 insertions(+), 10 deletions(-) create mode 100644 src/acp/client.test.ts diff --git a/src/acp/client.test.ts b/src/acp/client.test.ts new file mode 100644 index 00000000000..7f4dd015373 --- /dev/null +++ b/src/acp/client.test.ts @@ -0,0 +1,60 @@ +import { describe, expect, it } from "vitest"; + +// Structural tests verify security-critical code exists in client.ts. +// Full integration tests with ACP SDK mocks deferred to future enhancement. + +describe("ACP client permission classification", () => { + it("should define dangerous tools that include exec and sessions_spawn", async () => { + const fs = await import("node:fs/promises"); + const path = await import("node:path"); + const source = await fs.readFile( + path.resolve(__dirname, "client.ts"), + "utf-8", + ); + + expect(source).toContain("DANGEROUS_ACP_TOOLS"); + expect(source).toContain('"exec"'); + expect(source).toContain('"sessions_spawn"'); + expect(source).toContain('"sessions_send"'); + expect(source).toContain('"gateway"'); + }); + + it("should not auto-approve when options array is empty", async () => { + const fs = await import("node:fs/promises"); + const path = await import("node:path"); + const source = await fs.readFile( + path.resolve(__dirname, "client.ts"), + "utf-8", + ); + + // Verify the empty-options guard exists + expect(source).toContain("options.length === 0"); + // Verify it denies rather than approves + expect(source).toContain("no options available"); + }); + + it("should use stderr for permission logging (not stdout)", async () => { + const fs = await import("node:fs/promises"); + const path = await import("node:path"); + const source = await fs.readFile( + path.resolve(__dirname, "client.ts"), + "utf-8", + ); + + // Permission logs should go to stderr to avoid corrupting ACP protocol on stdout + expect(source).toContain("console.error"); + expect(source).toContain("[permission"); + }); + + it("should have a 30-second timeout for interactive prompts", async () => { + const fs = await import("node:fs/promises"); + const path = await import("node:path"); + const source = await fs.readFile( + path.resolve(__dirname, "client.ts"), + "utf-8", + ); + + expect(source).toContain("30_000"); + expect(source).toContain("[permission timeout]"); + }); +}); diff --git a/src/acp/client.ts b/src/acp/client.ts index e1b86979029..bb6e2ac34b7 100644 --- a/src/acp/client.ts +++ b/src/acp/client.ts @@ -10,6 +10,48 @@ import * as readline from "node:readline"; import { Readable, Writable } from "node:stream"; import { ensureOpenClawCliOnPath } from "../infra/path-env.js"; +/** + * Tools that require explicit user approval in ACP sessions. + * These tools can execute arbitrary code, modify the filesystem, + * or access sensitive resources. + */ +const DANGEROUS_ACP_TOOLS = new Set([ + "exec", + "spawn", + "shell", + "sessions_spawn", + "sessions_send", + "gateway", + "fs_write", + "fs_delete", + "fs_move", + "apply_patch", +]); + +function promptUserPermission(toolName: string, toolTitle?: string): Promise { + return new Promise((resolve) => { + const rl = readline.createInterface({ + input: process.stdin, + output: process.stderr, + }); + + const timeout = setTimeout(() => { + console.error(`\n[permission timeout] denied: ${toolName}`); + rl.close(); + resolve(false); + }, 30_000); + + const label = toolTitle ? `${toolTitle} (${toolName})` : toolName; + rl.question(`\n[permission] Allow "${label}"? (y/N) `, (answer) => { + clearTimeout(timeout); + rl.close(); + const approved = answer.trim().toLowerCase() === "y"; + console.error(`[permission ${approved ? "approved" : "denied"}] ${toolName}`); + resolve(approved); + }); + }); +} + export type AcpClientOptions = { cwd?: string; serverCommand?: string; @@ -104,16 +146,42 @@ export async function createAcpClient(opts: AcpClientOptions = {}): Promise { - console.log("\n[permission requested]", params.toolCall?.title ?? "tool"); + // toolCall may include a `name` field not in the SDK type + const toolCall = params.toolCall as Record | undefined; + const toolName = (typeof toolCall?.name === "string" ? toolCall.name : "") as string; + const toolTitle = (params.toolCall?.title ?? "tool") as string; const options = params.options ?? []; - const allowOnce = options.find((option) => option.kind === "allow_once"); - const fallback = options[0]; - return { - outcome: { - outcome: "selected", - optionId: allowOnce?.optionId ?? fallback?.optionId ?? "allow", - }, - }; + const allowOnce = options.find((o) => o.kind === "allow_once"); + const rejectOption = options.find((o) => o.kind === "reject_once"); + + // No options available — deny by default (fixes empty-options exploit) + if (options.length === 0) { + console.error(`[permission denied] ${toolName}: no options available`); + return { outcome: { outcome: "selected", optionId: "deny" } }; + } + + // Safe tools: auto-approve (backward compatible) + if (!DANGEROUS_ACP_TOOLS.has(toolName)) { + console.error(`[permission auto-approved] ${toolName}`); + return { + outcome: { + outcome: "selected", + optionId: allowOnce?.optionId ?? options[0]?.optionId ?? "allow", + }, + }; + } + + // Dangerous tools: require interactive confirmation + console.error(`\n[permission requested] ${toolTitle} (${toolName})`); + const approved = await promptUserPermission(toolName, toolTitle); + + if (approved && allowOnce) { + return { outcome: { outcome: "selected", optionId: allowOnce.optionId } }; + } + + // Denied — use reject option if available, otherwise reject + const rejectId = rejectOption?.optionId ?? "deny"; + return { outcome: { outcome: "selected", optionId: rejectId } }; }, }), stream, diff --git a/src/config/types.gateway.ts b/src/config/types.gateway.ts index 63e0537f4f0..25ad30c3a17 100644 --- a/src/config/types.gateway.ts +++ b/src/config/types.gateway.ts @@ -226,6 +226,13 @@ export type GatewayNodesConfig = { denyCommands?: string[]; }; +export type GatewayToolsConfig = { + /** Tools to deny via gateway HTTP /tools/invoke (extends defaults). */ + deny?: string[]; + /** Tools to explicitly allow (removes from default deny list). */ + allow?: string[]; +}; + export type GatewayConfig = { /** Single multiplexed port for Gateway WS + HTTP (default: 18789). */ port?: number; @@ -260,4 +267,6 @@ export type GatewayConfig = { * `x-real-ip`) to determine the client IP for local pairing and HTTP checks. */ trustedProxies?: string[]; + /** Tool access restrictions for HTTP /tools/invoke endpoint. */ + tools?: GatewayToolsConfig; }; diff --git a/src/gateway/tools-invoke-http.test.ts b/src/gateway/tools-invoke-http.test.ts index 1afaed10199..a71b19a6b0c 100644 --- a/src/gateway/tools-invoke-http.test.ts +++ b/src/gateway/tools-invoke-http.test.ts @@ -225,6 +225,72 @@ describe("POST /tools/invoke", () => { expect(profileRes.status).toBe(404); }); + it("denies sessions_spawn via HTTP even when agent policy allows", async () => { + testState.agentsConfig = { + list: [ + { + id: "main", + tools: { allow: ["sessions_spawn"] }, + }, + ], + } as any; + + const port = await getFreePort(); + const server = await startGatewayServer(port, { bind: "loopback" }); + const token = resolveGatewayToken(); + + const res = await fetch(`http://127.0.0.1:${port}/tools/invoke`, { + method: "POST", + headers: { "content-type": "application/json", authorization: `Bearer ${token}` }, + body: JSON.stringify({ tool: "sessions_spawn", args: { task: "test" }, sessionKey: "main" }), + }); + + expect(res.status).toBe(404); + const body = await res.json(); + expect(body.ok).toBe(false); + expect(body.error.type).toBe("not_found"); + + await server.close(); + }); + + it("denies sessions_send via HTTP gateway", async () => { + testState.agentsConfig = { + list: [{ id: "main", tools: { allow: ["sessions_send"] } }], + } as any; + + const port = await getFreePort(); + const server = await startGatewayServer(port, { bind: "loopback" }); + const token = resolveGatewayToken(); + + const res = await fetch(`http://127.0.0.1:${port}/tools/invoke`, { + method: "POST", + headers: { "content-type": "application/json", authorization: `Bearer ${token}` }, + body: JSON.stringify({ tool: "sessions_send", args: {}, sessionKey: "main" }), + }); + + expect(res.status).toBe(404); + await server.close(); + }); + + it("denies gateway tool via HTTP", async () => { + testState.agentsConfig = { + list: [{ id: "main", tools: { allow: ["gateway"] } }], + } as any; + + const port = await getFreePort(); + const server = await startGatewayServer(port, { bind: "loopback" }); + const token = resolveGatewayToken(); + + const res = await fetch(`http://127.0.0.1:${port}/tools/invoke`, { + method: "POST", + headers: { "content-type": "application/json", authorization: `Bearer ${token}` }, + body: JSON.stringify({ tool: "gateway", args: {}, sessionKey: "main" }), + }); + + expect(res.status).toBe(404); + await server.close(); + }); + it("uses the configured main session key when sessionKey is missing or main", async () => { testState.agentsConfig = { list: [ diff --git a/src/gateway/tools-invoke-http.ts b/src/gateway/tools-invoke-http.ts index 7e9e5e49a97..a9d6c74f469 100644 --- a/src/gateway/tools-invoke-http.ts +++ b/src/gateway/tools-invoke-http.ts @@ -34,6 +34,22 @@ import { getBearerToken, getHeader } from "./http-utils.js"; const DEFAULT_BODY_BYTES = 2 * 1024 * 1024; const MEMORY_TOOL_NAMES = new Set(["memory_search", "memory_get"]); +/** + * Tools denied via HTTP /tools/invoke regardless of session policy. + * Prevents RCE and privilege escalation from HTTP API surface. + * Configurable via gateway.tools.{deny,allow} in openclaw.json. + */ +const DEFAULT_GATEWAY_HTTP_TOOL_DENY = [ + // Session orchestration — spawning agents remotely is RCE + "sessions_spawn", + // Cross-session injection — message injection across sessions + "sessions_send", + // Gateway control plane — prevents gateway reconfiguration via HTTP + "gateway", + // Interactive setup — requires terminal QR scan, hangs on HTTP + "whatsapp_login", +]; + type ToolsInvokeBody = { tool?: unknown; action?: unknown; @@ -297,7 +313,15 @@ export async function handleToolsInvokeHttpRequest( ? filterToolsByPolicy(groupFiltered, subagentPolicyExpanded) : groupFiltered; - const tool = subagentFiltered.find((t) => t.name === toolName); + // Gateway HTTP-specific deny list — applies to ALL sessions via HTTP. + const gatewayToolsCfg = cfg.gateway?.tools; + const gatewayDenyNames = DEFAULT_GATEWAY_HTTP_TOOL_DENY + .filter((name) => !gatewayToolsCfg?.allow?.includes(name)) + .concat(Array.isArray(gatewayToolsCfg?.deny) ? gatewayToolsCfg.deny : []); + const gatewayDenySet = new Set(gatewayDenyNames); + const gatewayFiltered = subagentFiltered.filter((t) => !gatewayDenySet.has(t.name)); + + const tool = gatewayFiltered.find((t) => t.name === toolName); if (!tool) { sendJson(res, 404, { ok: false,