fix(security): bind system.run approvals to argv identity

This commit is contained in:
Peter Steinberger
2026-02-26 03:40:42 +01:00
parent baf656bc6f
commit 03e689fc89
12 changed files with 102 additions and 9 deletions

View File

@@ -21,6 +21,7 @@ Docs: https://docs.openclaw.ai
- Slack/Session threads: prevent oversized parent-session inheritance from silently bricking new thread sessions, surface embedded context-overflow empty-result failures to users, and add configurable `session.parentForkMaxTokens` (default `100000`, `0` disables). (#26912) Thanks @markshields-tl. - Slack/Session threads: prevent oversized parent-session inheritance from silently bricking new thread sessions, surface embedded context-overflow empty-result failures to users, and add configurable `session.parentForkMaxTokens` (default `100000`, `0` disables). (#26912) Thanks @markshields-tl.
- Models/Auth probes: map permanent auth failover reasons (`auth_permanent`, for example revoked keys) into probe auth status instead of `unknown`, so `openclaw models status --probe` reports actionable auth failures. (#25754) thanks @rrenamed. - Models/Auth probes: map permanent auth failover reasons (`auth_permanent`, for example revoked keys) into probe auth status instead of `unknown`, so `openclaw models status --probe` reports actionable auth failures. (#25754) thanks @rrenamed.
- Security/Signal: enforce DM/group authorization before reaction-only notification enqueue so unauthorized senders can no longer inject Signal reaction system events under `dmPolicy`/`groupPolicy`; reaction notifications now require channel access checks first. This ships in the next npm release (`2026.2.25`). Thanks @tdjackey for reporting. - Security/Signal: enforce DM/group authorization before reaction-only notification enqueue so unauthorized senders can no longer inject Signal reaction system events under `dmPolicy`/`groupPolicy`; reaction notifications now require channel access checks first. This ships in the next npm release (`2026.2.25`). Thanks @tdjackey for reporting.
- Security/Exec approvals: bind `system.run` approval matching to exact argv identity and preserve argv whitespace in rendered command text, preventing trailing-space executable path swaps from reusing a mismatched approval. This ships in the next npm release (`2026.2.25`). Thanks @tdjackey for reporting.
- Security/Discord + Slack reactions: enforce DM policy/allowlist authorization before reaction-event system enqueue in direct messages; Discord reaction handling now also honors DM/group-DM enablement and guild `groupPolicy` channel gating to keep reaction ingress aligned with normal message preflight. This ships in the next npm release (`2026.2.25`). Thanks @tdjackey for reporting. - Security/Discord + Slack reactions: enforce DM policy/allowlist authorization before reaction-event system enqueue in direct messages; Discord reaction handling now also honors DM/group-DM enablement and guild `groupPolicy` channel gating to keep reaction ingress aligned with normal message preflight. This ships in the next npm release (`2026.2.25`). Thanks @tdjackey for reporting.
- Security/Telegram reactions: enforce `dmPolicy`/`allowFrom` and group allowlist authorization on `message_reaction` events before enqueueing reaction system events, preventing unauthorized reaction-triggered input in DMs and groups; ships in the next npm release (`2026.2.25`). Thanks @tdjackey for reporting. - Security/Telegram reactions: enforce `dmPolicy`/`allowFrom` and group allowlist authorization on `message_reaction` events before enqueueing reaction system events, preventing unauthorized reaction-triggered input in DMs and groups; ships in the next npm release (`2026.2.25`). Thanks @tdjackey for reporting.
- Security/Slack interactions: enforce channel/DM authorization and modal actor binding (`private_metadata.userId`) before enqueueing `block_action`/`view_submission`/`view_closed` system events, with regression coverage for unauthorized senders and missing/mismatched actor metadata. This ships in the next npm release (`2026.2.25`). Thanks @tdjackey for reporting. - Security/Slack interactions: enforce channel/DM authorization and modal actor binding (`private_metadata.userId`) before enqueueing `block_action`/`view_submission`/`view_closed` system events, with regression coverage for unauthorized senders and missing/mismatched actor metadata. This ships in the next npm release (`2026.2.25`). Thanks @tdjackey for reporting.

View File

@@ -8,6 +8,7 @@ import { callGatewayTool } from "./tools/gateway.js";
export type RequestExecApprovalDecisionParams = { export type RequestExecApprovalDecisionParams = {
id: string; id: string;
command: string; command: string;
commandArgv?: string[];
cwd: string; cwd: string;
nodeId?: string; nodeId?: string;
host: "gateway" | "node"; host: "gateway" | "node";
@@ -62,6 +63,7 @@ export async function registerExecApprovalRequest(
{ {
id: params.id, id: params.id,
command: params.command, command: params.command,
commandArgv: params.commandArgv,
cwd: params.cwd, cwd: params.cwd,
nodeId: params.nodeId, nodeId: params.nodeId,
host: params.host, host: params.host,
@@ -116,6 +118,7 @@ export async function requestExecApprovalDecision(
export async function requestExecApprovalDecisionForHost(params: { export async function requestExecApprovalDecisionForHost(params: {
approvalId: string; approvalId: string;
command: string; command: string;
commandArgv?: string[];
workdir: string; workdir: string;
host: "gateway" | "node"; host: "gateway" | "node";
nodeId?: string; nodeId?: string;
@@ -128,6 +131,7 @@ export async function requestExecApprovalDecisionForHost(params: {
return await requestExecApprovalDecision({ return await requestExecApprovalDecision({
id: params.approvalId, id: params.approvalId,
command: params.command, command: params.command,
commandArgv: params.commandArgv,
cwd: params.workdir, cwd: params.workdir,
nodeId: params.nodeId, nodeId: params.nodeId,
host: params.host, host: params.host,
@@ -142,6 +146,7 @@ export async function requestExecApprovalDecisionForHost(params: {
export async function registerExecApprovalRequestForHost(params: { export async function registerExecApprovalRequestForHost(params: {
approvalId: string; approvalId: string;
command: string; command: string;
commandArgv?: string[];
workdir: string; workdir: string;
host: "gateway" | "node"; host: "gateway" | "node";
nodeId?: string; nodeId?: string;
@@ -154,6 +159,7 @@ export async function registerExecApprovalRequestForHost(params: {
return await registerExecApprovalRequest({ return await registerExecApprovalRequest({
id: params.approvalId, id: params.approvalId,
command: params.command, command: params.command,
commandArgv: params.commandArgv,
cwd: params.workdir, cwd: params.workdir,
nodeId: params.nodeId, nodeId: params.nodeId,
host: params.host, host: params.host,

View File

@@ -194,6 +194,7 @@ export async function executeNodeHostCommand(
const registration = await registerExecApprovalRequestForHost({ const registration = await registerExecApprovalRequestForHost({
approvalId, approvalId,
command: params.command, command: params.command,
commandArgv: argv,
workdir: params.workdir, workdir: params.workdir,
host: "node", host: "node",
nodeId, nodeId,

View File

@@ -18,6 +18,7 @@ import {
} from "../../cli/nodes-screen.js"; } from "../../cli/nodes-screen.js";
import { parseDurationMs } from "../../cli/parse-duration.js"; import { parseDurationMs } from "../../cli/parse-duration.js";
import type { OpenClawConfig } from "../../config/config.js"; import type { OpenClawConfig } from "../../config/config.js";
import { formatExecCommand } from "../../infra/system-run-command.js";
import { imageMimeFromFormat } from "../../media/mime.js"; import { imageMimeFromFormat } from "../../media/mime.js";
import { resolveSessionAgentId } from "../agent-scope.js"; import { resolveSessionAgentId } from "../agent-scope.js";
import { resolveImageSanitizationLimits } from "../image-sanitization.js"; import { resolveImageSanitizationLimits } from "../image-sanitization.js";
@@ -473,7 +474,7 @@ export function createNodesTool(options?: {
// Node requires approval create a pending approval request on // Node requires approval create a pending approval request on
// the gateway and wait for the user to approve/deny via the UI. // the gateway and wait for the user to approve/deny via the UI.
const APPROVAL_TIMEOUT_MS = 120_000; const APPROVAL_TIMEOUT_MS = 120_000;
const cmdText = command.join(" "); const cmdText = formatExecCommand(command);
const approvalId = crypto.randomUUID(); const approvalId = crypto.randomUUID();
const approvalResult = await callGatewayTool( const approvalResult = await callGatewayTool(
"exec.approval.request", "exec.approval.request",
@@ -481,6 +482,7 @@ export function createNodesTool(options?: {
{ {
id: approvalId, id: approvalId,
command: cmdText, command: cmdText,
commandArgv: command,
cwd, cwd,
nodeId, nodeId,
host: "node", host: "node",

View File

@@ -6,6 +6,7 @@ const RESOLVED_ENTRY_GRACE_MS = 15_000;
export type ExecApprovalRequestPayload = { export type ExecApprovalRequestPayload = {
command: string; command: string;
commandArgv?: string[] | null;
cwd?: string | null; cwd?: string | null;
nodeId?: string | null; nodeId?: string | null;
host?: string | null; host?: string | null;

View File

@@ -13,13 +13,14 @@ describe("sanitizeSystemRunParamsForForwarding", () => {
}, },
}; };
function makeRecord(command: string): ExecApprovalRecord { function makeRecord(command: string, commandArgv?: string[] | null): ExecApprovalRecord {
return { return {
id: "approval-1", id: "approval-1",
request: { request: {
host: "node", host: "node",
nodeId: "node-1", nodeId: "node-1",
command, command,
commandArgv: commandArgv ?? null,
cwd: null, cwd: null,
agentId: null, agentId: null,
sessionKey: null, sessionKey: null,
@@ -139,6 +140,64 @@ describe("sanitizeSystemRunParamsForForwarding", () => {
}); });
expectAllowOnceForwardingResult(result); expectAllowOnceForwardingResult(result);
}); });
test("rejects trailing-space argv mismatch against legacy command-only approval", () => {
const result = sanitizeSystemRunParamsForForwarding({
rawParams: {
command: ["runner "],
runId: "approval-1",
approved: true,
approvalDecision: "allow-once",
},
nodeId: "node-1",
client,
execApprovalManager: manager(makeRecord("runner")),
nowMs: now,
});
expect(result.ok).toBe(false);
if (result.ok) {
throw new Error("unreachable");
}
expect(result.message).toContain("approval id does not match request");
expect(result.details?.code).toBe("APPROVAL_REQUEST_MISMATCH");
});
test("enforces commandArgv identity when approval includes argv binding", () => {
const result = sanitizeSystemRunParamsForForwarding({
rawParams: {
command: ["echo", "SAFE"],
runId: "approval-1",
approved: true,
approvalDecision: "allow-once",
},
nodeId: "node-1",
client,
execApprovalManager: manager(makeRecord("echo SAFE", ["echo SAFE"])),
nowMs: now,
});
expect(result.ok).toBe(false);
if (result.ok) {
throw new Error("unreachable");
}
expect(result.message).toContain("approval id does not match request");
expect(result.details?.code).toBe("APPROVAL_REQUEST_MISMATCH");
});
test("accepts matching commandArgv binding for trailing-space argv", () => {
const result = sanitizeSystemRunParamsForForwarding({
rawParams: {
command: ["runner "],
runId: "approval-1",
approved: true,
approvalDecision: "allow-once",
},
nodeId: "node-1",
client,
execApprovalManager: manager(makeRecord('"runner "', ["runner "])),
nowMs: now,
});
expectAllowOnceForwardingResult(result);
});
test("consumes allow-once approvals and blocks same runId replay", async () => { test("consumes allow-once approvals and blocks same runId replay", async () => {
const approvalManager = new ExecApprovalManager(); const approvalManager = new ExecApprovalManager();
const runId = "approval-replay-1"; const runId = "approval-replay-1";

View File

@@ -55,6 +55,7 @@ function clientHasApprovals(client: ApprovalClient | null): boolean {
function approvalMatchesRequest( function approvalMatchesRequest(
cmdText: string, cmdText: string,
argv: string[],
params: SystemRunParamsLike, params: SystemRunParamsLike,
record: ExecApprovalRecord, record: ExecApprovalRecord,
): boolean { ): boolean {
@@ -62,7 +63,19 @@ function approvalMatchesRequest(
return false; return false;
} }
if (!cmdText || record.request.command !== cmdText) { const requestedArgv = Array.isArray(record.request.commandArgv)
? record.request.commandArgv
: null;
if (requestedArgv) {
if (requestedArgv.length === 0 || requestedArgv.length !== argv.length) {
return false;
}
for (let i = 0; i < requestedArgv.length; i += 1) {
if (requestedArgv[i] !== argv[i]) {
return false;
}
}
} else if (!cmdText || record.request.command !== cmdText) {
return false; return false;
} }
@@ -237,7 +250,7 @@ export function sanitizeSystemRunParamsForForwarding(opts: {
}; };
} }
if (!approvalMatchesRequest(cmdText, p, snapshot)) { if (!approvalMatchesRequest(cmdText, cmdTextResolution.argv, p, snapshot)) {
return { return {
ok: false, ok: false,
message: "approval id does not match request", message: "approval id does not match request",

View File

@@ -89,6 +89,7 @@ export const ExecApprovalRequestParamsSchema = Type.Object(
{ {
id: Type.Optional(NonEmptyString), id: Type.Optional(NonEmptyString),
command: NonEmptyString, command: NonEmptyString,
commandArgv: Type.Optional(Type.Union([Type.Array(Type.String()), Type.Null()])),
cwd: Type.Optional(Type.Union([Type.String(), Type.Null()])), cwd: Type.Optional(Type.Union([Type.String(), Type.Null()])),
nodeId: Type.Optional(Type.Union([NonEmptyString, Type.Null()])), nodeId: Type.Optional(Type.Union([NonEmptyString, Type.Null()])),
host: Type.Optional(Type.Union([Type.String(), Type.Null()])), host: Type.Optional(Type.Union([Type.String(), Type.Null()])),

View File

@@ -43,6 +43,7 @@ export function createExecApprovalHandlers(
const p = params as { const p = params as {
id?: string; id?: string;
command: string; command: string;
commandArgv?: string[] | null;
cwd?: string; cwd?: string;
nodeId?: string; nodeId?: string;
host?: string; host?: string;
@@ -60,6 +61,9 @@ export function createExecApprovalHandlers(
const explicitId = typeof p.id === "string" && p.id.trim().length > 0 ? p.id.trim() : null; const explicitId = typeof p.id === "string" && p.id.trim().length > 0 ? p.id.trim() : null;
const host = typeof p.host === "string" ? p.host.trim() : ""; const host = typeof p.host === "string" ? p.host.trim() : "";
const nodeId = typeof p.nodeId === "string" ? p.nodeId.trim() : ""; const nodeId = typeof p.nodeId === "string" ? p.nodeId.trim() : "";
const commandArgv = Array.isArray(p.commandArgv)
? p.commandArgv.map((entry) => String(entry))
: null;
if (host === "node" && !nodeId) { if (host === "node" && !nodeId) {
respond( respond(
false, false,
@@ -78,6 +82,7 @@ export function createExecApprovalHandlers(
} }
const request = { const request = {
command: p.command, command: p.command,
commandArgv,
cwd: p.cwd ?? null, cwd: p.cwd ?? null,
nodeId: host === "node" ? nodeId : null, nodeId: host === "node" ? nodeId : null,
host: host || null, host: host || null,

View File

@@ -15,6 +15,7 @@ export type ExecApprovalRequest = {
id: string; id: string;
request: { request: {
command: string; command: string;
commandArgv?: string[] | null;
cwd?: string | null; cwd?: string | null;
nodeId?: string | null; nodeId?: string | null;
host?: string | null; host?: string | null;

View File

@@ -21,6 +21,10 @@ describe("system run command helpers", () => {
expect(formatExecCommand(["echo", "hi there"])).toBe('echo "hi there"'); expect(formatExecCommand(["echo", "hi there"])).toBe('echo "hi there"');
}); });
test("formatExecCommand preserves trailing whitespace in argv tokens", () => {
expect(formatExecCommand(["runner "])).toBe('"runner "');
});
test("extractShellCommandFromArgv extracts sh -lc command", () => { test("extractShellCommandFromArgv extracts sh -lc command", () => {
expect(extractShellCommandFromArgv(["/bin/sh", "-lc", "echo hi"])).toBe("echo hi"); expect(extractShellCommandFromArgv(["/bin/sh", "-lc", "echo hi"])).toBe("echo hi");
}); });

View File

@@ -35,15 +35,14 @@ export type ResolvedSystemRunCommand =
export function formatExecCommand(argv: string[]): string { export function formatExecCommand(argv: string[]): string {
return argv return argv
.map((arg) => { .map((arg) => {
const trimmed = arg.trim(); if (arg.length === 0) {
if (!trimmed) {
return '""'; return '""';
} }
const needsQuotes = /\s|"/.test(trimmed); const needsQuotes = /\s|"/.test(arg);
if (!needsQuotes) { if (!needsQuotes) {
return trimmed; return arg;
} }
return `"${trimmed.replace(/"/g, '\\"')}"`; return `"${arg.replace(/"/g, '\\"')}"`;
}) })
.join(" "); .join(" ");
} }