From ab3c29ef4c731dfdbff75f2f5c7ec65c63ec195d Mon Sep 17 00:00:00 2001 From: momothemage Date: Wed, 1 Jul 2026 11:02:59 +0800 Subject: [PATCH] fix(security): audit agent skill MCP exec drift --- docs/gateway/security/audit-checks.md | 1 + docs/tools/skills-config.md | 11 ++ docs/tools/skills.md | 3 + src/security/audit-config-basics.test.ts | 87 +++++++++++++ src/security/audit.ts | 157 ++++++++++++++++++++++- 5 files changed, 258 insertions(+), 1 deletion(-) diff --git a/docs/gateway/security/audit-checks.md b/docs/gateway/security/audit-checks.md index 2db09654f679..8f40fc97de89 100644 --- a/docs/gateway/security/audit-checks.md +++ b/docs/gateway/security/audit-checks.md @@ -92,6 +92,7 @@ exhaustive): | `tools.exec.host_sandbox_no_sandbox_defaults` | warn | `exec host=sandbox` fails closed when sandbox is off | `tools.exec.host`, `agents.defaults.sandbox.mode` | no | | `tools.exec.host_sandbox_no_sandbox_agents` | warn | Per-agent `exec host=sandbox` fails closed when sandbox is off | `agents.list[].tools.exec.host`, `agents.list[].sandbox.mode` | no | | `tools.exec.security_full_configured` | warn/critical | Host exec is running with `security="full"` | `tools.exec.security`, `agents.list[].tools.exec.security` | no | +| `tools.exec.agent_skill_mcp_boundary_drift` | warn | Agent skill allowlists are present while host exec can reach MCP clients/registries | `agents.list[].tools.exec.*`, sandbox/OS isolation, MCP server credentials | no | | `tools.exec.fs_tools_disabled_but_exec_enabled` | warn | Filesystem tool policy does not make shell execution read-only | `tools.deny`, `agents.list[].tools.deny`, `agents.*.sandbox.workspaceAccess` | no | | `tools.exec.auto_allow_skills_enabled` | warn | Exec approvals trust skill bins implicitly | host approvals file | no | | `tools.exec.allowlist_interpreter_without_strict_inline_eval` | warn | Interpreter allowlists permit inline eval without forced reapproval | `tools.exec.strictInlineEval`, `agents.list[].tools.exec.strictInlineEval`, exec approvals allowlist | no | diff --git a/docs/tools/skills-config.md b/docs/tools/skills-config.md index 1c837aeb54ca..b2ece4828268 100644 --- a/docs/tools/skills-config.md +++ b/docs/tools/skills-config.md @@ -321,6 +321,17 @@ different visible skill set per agent. defaults — they do not merge. Set to `[]` to expose no skills for that agent. + + Agent skill allowlists are a visibility and loading filter for OpenClaw skill + discovery, prompts, slash-command discovery, sandbox sync, and skill + snapshots. They are not a shell-time authorization boundary. If an agent can + run host `exec`, that shell can still run external clients or read host files + that are visible to the execution user, including MCP client registries such + as `~/.openclaw/skills/config/mcporter.json`. For per-agent MCP isolation, + combine skill allowlists with sandbox/OS-user isolation, deny or tightly + allowlist host exec, and prefer per-agent credentials at the MCP server. + + ## Workshop (`skills.workshop`) diff --git a/docs/tools/skills.md b/docs/tools/skills.md index d17759225ed2..3075dfe56902 100644 --- a/docs/tools/skills.md +++ b/docs/tools/skills.md @@ -104,6 +104,9 @@ regardless of where they are loaded from. merge with defaults. - The effective allowlist applies across prompt building, slash-command discovery, sandbox sync, and skill snapshots. + - This is not a host shell authorization boundary. If the same agent can + use `exec`, constrain that shell separately with sandboxing, OS-user + isolation, exec deny/allowlists, and per-resource credentials. diff --git a/src/security/audit-config-basics.test.ts b/src/security/audit-config-basics.test.ts index ae6b07a35e26..aa5fbb0dce53 100644 --- a/src/security/audit-config-basics.test.ts +++ b/src/security/audit-config-basics.test.ts @@ -1,4 +1,7 @@ // Covers baseline config security audit findings. +import fs from "node:fs/promises"; +import os from "node:os"; +import path from "node:path"; import { describe, expect, it } from "vitest"; import { onInternalDiagnosticEvent, @@ -63,6 +66,90 @@ describe("security audit config basics", () => { ).toBe(true); }); + it("flags per-agent skill allowlists combined with host exec and a global mcporter registry", async () => { + const stateDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-audit-mcporter-")); + try { + await fs.mkdir(path.join(stateDir, "skills", "config"), { recursive: true }); + await fs.writeFile( + path.join(stateDir, "skills", "config", "mcporter.json"), + JSON.stringify({ + mcpServers: { + "hugegraph-asset": { baseUrl: "http://asset.example.test/mcp" }, + "whois-mcp": { baseUrl: "http://whois.example.test/mcp" }, + }, + }), + "utf8", + ); + + const report = await runSecurityAudit({ + config: { + agents: { + list: [ + { + id: "asset-agent", + skills: ["asset-lifecycle-tracking"], + tools: { exec: { host: "gateway", security: "full", ask: "off" } }, + }, + ], + }, + }, + sourceConfig: {}, + env: { OPENCLAW_STATE_DIR: stateDir }, + stateDir, + includeFilesystem: false, + includeChannelSecurity: false, + }); + + expect(report.findings).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + checkId: "tools.exec.agent_skill_mcp_boundary_drift", + severity: "warn", + detail: expect.stringContaining("asset-agent"), + }), + ]), + ); + const finding = report.findings.find( + (entry) => entry.checkId === "tools.exec.agent_skill_mcp_boundary_drift", + ); + expect(finding?.detail).toContain("whois-mcp"); + expect(finding?.detail).toContain("skills/config/mcporter.json"); + } finally { + await fs.rm(stateDir, { recursive: true, force: true }); + } + }); + + it("does not flag per-agent skill allowlists when matching agents deny exec", async () => { + const stateDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-audit-mcporter-deny-")); + try { + const report = await runSecurityAudit({ + config: { + mcp: { + servers: { + docs: { command: "node", args: ["docs-mcp.js"] }, + }, + }, + agents: { + defaults: { skills: ["docs-search"] }, + list: [{ id: "docs-agent", tools: { exec: { security: "deny" } } }], + }, + tools: { exec: { security: "deny" } }, + }, + sourceConfig: {}, + env: { OPENCLAW_STATE_DIR: stateDir }, + stateDir, + includeFilesystem: false, + includeChannelSecurity: false, + }); + + expect(report.findings.map((finding) => finding.checkId)).not.toContain( + "tools.exec.agent_skill_mcp_boundary_drift", + ); + } finally { + await fs.rm(stateDir, { recursive: true, force: true }); + } + }); + it("suppresses configured accepted findings from the active audit report", async () => { const report = await runSecurityAudit({ config: { diff --git a/src/security/audit.ts b/src/security/audit.ts index 51632eb92acc..8a9ef89fa8c6 100644 --- a/src/security/audit.ts +++ b/src/security/audit.ts @@ -1,4 +1,5 @@ // Orchestrates security audit collection and report formatting. +import fs from "node:fs/promises"; import path from "node:path"; import { normalizeProviderId } from "@openclaw/model-catalog-core/provider-id"; import { asNullableRecord } from "@openclaw/normalization-core/record-coerce"; @@ -21,6 +22,7 @@ import { materializeGatewayAuthSecretRefs, } from "../gateway/auth-config-utils.js"; import { isInterpreterLikeAllowlistPattern } from "../infra/command-analysis/inline-eval.js"; +import { emitTrustedSecurityEvent } from "../infra/diagnostic-events.js"; import { type ExecApprovalsFile, loadExecApprovals, @@ -34,7 +36,6 @@ import { } from "../infra/exec-safe-bin-runtime-policy.js"; import { listRiskyConfiguredSafeBins } from "../infra/exec-safe-bin-semantics.js"; import { normalizeTrustedSafeBinDirs } from "../infra/exec-safe-bin-trust.js"; -import { emitTrustedSecurityEvent } from "../infra/diagnostic-events.js"; import { DEFAULT_AGENT_ID } from "../routing/session-key.js"; import { collectDeepCodeSafetyFindings } from "./audit-deep-code-safety.js"; import { collectDeepProbeFindings } from "./audit-deep-probe-findings.js"; @@ -65,6 +66,17 @@ type ClaudePermissionModeHit = { argSet: "args" | "resumeArgs"; mode: string; }; +type McpServerSourceSummary = { + label: string; + names: string[]; +}; +type AgentSkillMcpBoundaryScope = { + id: string; + skillSource: string; + execHost: string; + execSecurity: string; + execAsk: string; +}; export type { SecurityAuditFinding, @@ -1101,6 +1113,148 @@ export function collectExecRuntimeFindings(cfg: OpenClawConfig): SecurityAuditFi return findings; } +function formatNamesPreview(names: readonly string[]): string { + const visible = names.slice(0, 6); + const suffix = names.length > visible.length ? `, +${names.length - visible.length} more` : ""; + return `${visible.join(", ")}${suffix}`; +} + +function listConfiguredMcpServerNames(cfg: OpenClawConfig): string[] { + return Object.entries(cfg.mcp?.servers ?? {}) + .filter(([, server]) => server?.enabled !== false) + .map(([name]) => name) + .toSorted(); +} + +async function readGlobalMcporterRegistrySummary( + stateDir: string, +): Promise { + const registryPath = path.join(stateDir, "skills", "config", "mcporter.json"); + let parsed: unknown; + try { + parsed = JSON.parse(await fs.readFile(registryPath, "utf8")) as unknown; + } catch { + return null; + } + const mcpServers = asNullableRecord(asNullableRecord(parsed)?.mcpServers); + if (!mcpServers) { + return null; + } + const names = Object.entries(mcpServers) + .filter(([, value]) => asNullableRecord(value)?.enabled !== false) + .map(([name]) => name) + .toSorted(); + return names.length > 0 ? { label: "skills/config/mcporter.json", names } : null; +} + +function hasOwnSkillsAllowlist(entry: object | undefined): boolean { + return Boolean(entry && Object.hasOwn(entry, "skills")); +} + +function collectAgentSkillMcpBoundaryScopes(cfg: OpenClawConfig): AgentSkillMcpBoundaryScope[] { + const agents = Array.isArray(cfg.agents?.list) ? cfg.agents.list : []; + const defaultsHaveSkillAllowlist = hasOwnSkillsAllowlist(cfg.agents?.defaults); + const candidates = [ + ...(defaultsHaveSkillAllowlist + ? [ + { + id: DEFAULT_AGENT_ID, + skillSource: "agents.defaults.skills", + agentId: undefined, + }, + ] + : []), + ...agents + .filter( + (entry): entry is NonNullable<(typeof agents)[number]> => + Boolean(entry) && typeof entry === "object" && typeof entry.id === "string", + ) + .flatMap((entry) => { + if (hasOwnSkillsAllowlist(entry)) { + return [{ id: entry.id, skillSource: "agents.list[].skills", agentId: entry.id }]; + } + if (defaultsHaveSkillAllowlist) { + return [ + { + id: entry.id, + skillSource: "agents.defaults.skills (inherited)", + agentId: entry.id, + }, + ]; + } + return []; + }), + ]; + + return candidates.flatMap((candidate) => { + const sandboxMode = resolveSandboxConfigForAgent(cfg, candidate.agentId).mode; + const exec = resolveExecDefaults({ + cfg, + agentId: candidate.agentId, + sandboxAvailable: sandboxMode !== "off", + }); + if (exec.security === "deny" || exec.effectiveHost === "sandbox") { + return []; + } + return [ + { + id: candidate.id, + skillSource: candidate.skillSource, + execHost: exec.effectiveHost, + execSecurity: exec.security, + execAsk: exec.ask, + }, + ]; + }); +} + +export async function collectAgentSkillMcpBoundaryFindings(params: { + cfg: OpenClawConfig; + stateDir: string; +}): Promise { + const sources: McpServerSourceSummary[] = []; + const configServerNames = listConfiguredMcpServerNames(params.cfg); + if (configServerNames.length > 0) { + sources.push({ label: "mcp.servers", names: configServerNames }); + } + const globalMcporterRegistry = await readGlobalMcporterRegistrySummary(params.stateDir); + if (globalMcporterRegistry) { + sources.push(globalMcporterRegistry); + } + if (sources.length === 0) { + return []; + } + + const scopes = collectAgentSkillMcpBoundaryScopes(params.cfg); + if (scopes.length === 0) { + return []; + } + + return [ + { + checkId: "tools.exec.agent_skill_mcp_boundary_drift", + severity: "warn", + title: "Agent skill allowlists do not constrain host exec MCP clients", + detail: + `Detected agent skill allowlists on host-exec-capable scopes:\n${scopes + .slice(0, 8) + .map( + (scope) => + `- ${scope.id}: ${scope.skillSource}, exec.host=${scope.execHost}, security=${scope.execSecurity}, ask=${scope.execAsk}`, + ) + .join("\n")}` + + (scopes.length > 8 ? `\n- +${scopes.length - 8} more scopes.` : "") + + `\nMCP server registries visible to the gateway configuration/state:\n${sources + .map((source) => `- ${source.label}: ${formatNamesPreview(source.names)}`) + .join("\n")}\n` + + "agents.*.skills filters OpenClaw skill visibility and snapshots; it is not a shell-time authorization boundary. " + + "A host exec process can run external MCP clients or read a global mcporter registry unless sandbox, filesystem, network, or MCP credential boundaries block it.", + remediation: + 'For agents that need per-agent MCP isolation, set their exec policy to security="deny" or a tight allowlist, run them in sandbox/container/OS-user isolation where the global MCP registry is not readable, split sensitive MCP servers into a separate gateway/trust boundary, or require per-agent MCP credentials at the server layer.', + }, + ]; +} + function collectOpenExecSurfacePaths(cfg: OpenClawConfig): string[] { const channels = asNullableRecord(cfg.channels); if (!channels) { @@ -1285,6 +1439,7 @@ export async function runSecurityAudit(opts: SecurityAuditOptions): Promise