mirror of
https://github.com/openclaw/openclaw.git
synced 2026-04-19 11:28:38 +00:00
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
This commit is contained in:
committed by
Peter Steinberger
parent
8899f9e94a
commit
749e28dec7
60
src/acp/client.test.ts
Normal file
60
src/acp/client.test.ts
Normal file
@@ -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]");
|
||||||
|
});
|
||||||
|
});
|
||||||
@@ -10,6 +10,48 @@ import * as readline from "node:readline";
|
|||||||
import { Readable, Writable } from "node:stream";
|
import { Readable, Writable } from "node:stream";
|
||||||
import { ensureOpenClawCliOnPath } from "../infra/path-env.js";
|
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<boolean> {
|
||||||
|
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 = {
|
export type AcpClientOptions = {
|
||||||
cwd?: string;
|
cwd?: string;
|
||||||
serverCommand?: string;
|
serverCommand?: string;
|
||||||
@@ -104,16 +146,42 @@ export async function createAcpClient(opts: AcpClientOptions = {}): Promise<AcpC
|
|||||||
printSessionUpdate(params);
|
printSessionUpdate(params);
|
||||||
},
|
},
|
||||||
requestPermission: async (params: RequestPermissionRequest) => {
|
requestPermission: async (params: RequestPermissionRequest) => {
|
||||||
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<string, unknown> | undefined;
|
||||||
|
const toolName = (typeof toolCall?.name === "string" ? toolCall.name : "") as string;
|
||||||
|
const toolTitle = (params.toolCall?.title ?? "tool") as string;
|
||||||
const options = params.options ?? [];
|
const options = params.options ?? [];
|
||||||
const allowOnce = options.find((option) => option.kind === "allow_once");
|
const allowOnce = options.find((o) => o.kind === "allow_once");
|
||||||
const fallback = options[0];
|
const rejectOption = options.find((o) => o.kind === "reject_once");
|
||||||
return {
|
|
||||||
outcome: {
|
// No options available — deny by default (fixes empty-options exploit)
|
||||||
outcome: "selected",
|
if (options.length === 0) {
|
||||||
optionId: allowOnce?.optionId ?? fallback?.optionId ?? "allow",
|
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,
|
stream,
|
||||||
|
|||||||
@@ -226,6 +226,13 @@ export type GatewayNodesConfig = {
|
|||||||
denyCommands?: string[];
|
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 = {
|
export type GatewayConfig = {
|
||||||
/** Single multiplexed port for Gateway WS + HTTP (default: 18789). */
|
/** Single multiplexed port for Gateway WS + HTTP (default: 18789). */
|
||||||
port?: number;
|
port?: number;
|
||||||
@@ -260,4 +267,6 @@ export type GatewayConfig = {
|
|||||||
* `x-real-ip`) to determine the client IP for local pairing and HTTP checks.
|
* `x-real-ip`) to determine the client IP for local pairing and HTTP checks.
|
||||||
*/
|
*/
|
||||||
trustedProxies?: string[];
|
trustedProxies?: string[];
|
||||||
|
/** Tool access restrictions for HTTP /tools/invoke endpoint. */
|
||||||
|
tools?: GatewayToolsConfig;
|
||||||
};
|
};
|
||||||
|
|||||||
@@ -225,6 +225,72 @@ describe("POST /tools/invoke", () => {
|
|||||||
expect(profileRes.status).toBe(404);
|
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 () => {
|
it("uses the configured main session key when sessionKey is missing or main", async () => {
|
||||||
testState.agentsConfig = {
|
testState.agentsConfig = {
|
||||||
list: [
|
list: [
|
||||||
|
|||||||
@@ -34,6 +34,22 @@ import { getBearerToken, getHeader } from "./http-utils.js";
|
|||||||
const DEFAULT_BODY_BYTES = 2 * 1024 * 1024;
|
const DEFAULT_BODY_BYTES = 2 * 1024 * 1024;
|
||||||
const MEMORY_TOOL_NAMES = new Set(["memory_search", "memory_get"]);
|
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 = {
|
type ToolsInvokeBody = {
|
||||||
tool?: unknown;
|
tool?: unknown;
|
||||||
action?: unknown;
|
action?: unknown;
|
||||||
@@ -297,7 +313,15 @@ export async function handleToolsInvokeHttpRequest(
|
|||||||
? filterToolsByPolicy(groupFiltered, subagentPolicyExpanded)
|
? filterToolsByPolicy(groupFiltered, subagentPolicyExpanded)
|
||||||
: groupFiltered;
|
: 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) {
|
if (!tool) {
|
||||||
sendJson(res, 404, {
|
sendJson(res, 404, {
|
||||||
ok: false,
|
ok: false,
|
||||||
|
|||||||
Reference in New Issue
Block a user