diff --git a/CHANGELOG.md b/CHANGELOG.md index 4d84ad124a0..a8712622dca 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,6 +43,8 @@ Docs: https://docs.openclaw.ai - Chat/UI: strip inline reply/audio directive tags (`[[reply_to_current]]`, `[[reply_to:]]`, `[[audio_as_voice]]`) from displayed chat history, live chat event output, and session preview snippets so control tags no longer leak into user-visible surfaces. - BlueBubbles/DM history: restore DM backfill context with account-scoped rolling history, bounded backfill retries, and safer history payload limits. (#20302) Thanks @Ryan-Haines. - BlueBubbles/Webhooks: accept inbound/reaction webhook payloads when BlueBubbles omits `handle` but provides DM `chatGuid`, and harden payload extraction for array/string-wrapped message bodies so valid webhook events no longer get rejected as unparseable. (#23275) Thanks @toph31. +- Security/Audit: add `openclaw security audit` finding `gateway.nodes.allow_commands_dangerous` for risky `gateway.nodes.allowCommands` overrides, with severity upgraded to critical on remote gateway exposure. +- Gateway/Control plane: reduce cross-client write limiter contention by adding `connId` fallback keying when device ID and client IP are both unavailable. - Security/Config: block prototype-key traversal during config merge patch and legacy migration merge helpers (`__proto__`, `constructor`, `prototype`) to prevent prototype pollution during config mutation flows. (#22968) Thanks @Clawborn. - Security/Shell env: validate login-shell executable paths for shell-env fallback (`/etc/shells` + trusted prefixes) and block `SHELL` in dangerous env override policy paths so untrusted shell-path injection falls back safely to `/bin/sh`. Thanks @athuljayaram for reporting. - Security/Config: make parsed chat allowlist checks fail closed when `allowFrom` is empty, restoring expected DM/pairing gating. diff --git a/docs/cli/security.md b/docs/cli/security.md index 20def711197..964e33824e2 100644 --- a/docs/cli/security.md +++ b/docs/cli/security.md @@ -27,7 +27,7 @@ The audit warns when multiple DM senders share the main session and recommends * This is for cooperative/shared inbox hardening. A single Gateway shared by mutually untrusted/adversarial operators is not a recommended setup; split trust boundaries with separate gateways (or separate OS users/hosts). It also warns when small models (`<=300B`) are used without sandboxing and with web/browser tools enabled. For webhook ingress, it warns when `hooks.defaultSessionKey` is unset, when request `sessionKey` overrides are enabled, and when overrides are enabled without `hooks.allowedSessionKeyPrefixes`. -It also warns when sandbox Docker settings are configured while sandbox mode is off, when `gateway.nodes.denyCommands` uses ineffective pattern-like/unknown entries, when global `tools.profile="minimal"` is overridden by agent tool profiles, when open groups expose runtime/filesystem tools without sandbox/workspace guards, and when installed extension plugin tools may be reachable under permissive tool policy. +It also warns when sandbox Docker settings are configured while sandbox mode is off, when `gateway.nodes.denyCommands` uses ineffective pattern-like/unknown entries, when `gateway.nodes.allowCommands` explicitly enables dangerous node commands, when global `tools.profile="minimal"` is overridden by agent tool profiles, when open groups expose runtime/filesystem tools without sandbox/workspace guards, and when installed extension plugin tools may be reachable under permissive tool policy. It also warns when sandbox browser uses Docker `bridge` network without `sandbox.browser.cdpSourceRange`. It also warns when existing sandbox browser Docker containers have missing/stale hash labels (for example pre-migration containers missing `openclaw.browserConfigEpoch`) and recommends `openclaw sandbox recreate --browser --all`. It also warns when npm-based plugin/hook install records are unpinned, missing integrity metadata, or drift from currently installed package versions. diff --git a/docs/gateway/security/index.md b/docs/gateway/security/index.md index afcd045936f..d8df6dade76 100644 --- a/docs/gateway/security/index.md +++ b/docs/gateway/security/index.md @@ -84,7 +84,7 @@ If more than one person can DM your bot: - **Browser control exposure** (remote nodes, relay ports, remote CDP endpoints). - **Local disk hygiene** (permissions, symlinks, config includes, “synced folder” paths). - **Plugins** (extensions exist without an explicit allowlist). -- **Policy drift/misconfig** (sandbox docker settings configured but sandbox mode off; ineffective `gateway.nodes.denyCommands` patterns; global `tools.profile="minimal"` overridden by per-agent profiles; extension plugin tools reachable under permissive tool policy). +- **Policy drift/misconfig** (sandbox docker settings configured but sandbox mode off; ineffective `gateway.nodes.denyCommands` patterns; dangerous `gateway.nodes.allowCommands` entries; global `tools.profile="minimal"` overridden by per-agent profiles; extension plugin tools reachable under permissive tool policy). - **Runtime expectation drift** (for example `tools.exec.host="sandbox"` while sandbox mode is off, which runs directly on the gateway host). - **Model hygiene** (warn when configured models look legacy; not a hard block). @@ -117,30 +117,31 @@ When the audit prints findings, treat this as a priority order: High-signal `checkId` values you will most likely see in real deployments (not exhaustive): -| `checkId` | Severity | Why it matters | Primary fix key/path | Auto-fix | -| -------------------------------------------------- | ------------- | ------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------- | -------- | -| `fs.state_dir.perms_world_writable` | critical | Other users/processes can modify full OpenClaw state | filesystem perms on `~/.openclaw` | yes | -| `fs.config.perms_writable` | critical | Others can change auth/tool policy/config | filesystem perms on `~/.openclaw/openclaw.json` | yes | -| `fs.config.perms_world_readable` | critical | Config can expose tokens/settings | filesystem perms on config file | yes | -| `gateway.bind_no_auth` | critical | Remote bind without shared secret | `gateway.bind`, `gateway.auth.*` | no | -| `gateway.loopback_no_auth` | critical | Reverse-proxied loopback may become unauthenticated | `gateway.auth.*`, proxy setup | no | -| `gateway.http.no_auth` | warn/critical | Gateway HTTP APIs reachable with `auth.mode="none"` | `gateway.auth.mode`, `gateway.http.endpoints.*` | no | -| `gateway.tools_invoke_http.dangerous_allow` | warn/critical | Re-enables dangerous tools over HTTP API | `gateway.tools.allow` | no | -| `gateway.tailscale_funnel` | critical | Public internet exposure | `gateway.tailscale.mode` | no | -| `gateway.control_ui.insecure_auth` | warn | Insecure-auth compatibility toggle enabled | `gateway.controlUi.allowInsecureAuth` | no | -| `gateway.control_ui.device_auth_disabled` | critical | Disables device identity check | `gateway.controlUi.dangerouslyDisableDeviceAuth` | no | -| `config.insecure_or_dangerous_flags` | warn | Any insecure/dangerous debug flags enabled | multiple keys (see finding detail) | no | -| `hooks.token_too_short` | warn | Easier brute force on hook ingress | `hooks.token` | no | -| `hooks.request_session_key_enabled` | warn/critical | External caller can choose sessionKey | `hooks.allowRequestSessionKey` | no | -| `hooks.request_session_key_prefixes_missing` | warn/critical | No bound on external session key shapes | `hooks.allowedSessionKeyPrefixes` | no | -| `logging.redact_off` | warn | Sensitive values leak to logs/status | `logging.redactSensitive` | yes | -| `sandbox.docker_config_mode_off` | warn | Sandbox Docker config present but inactive | `agents.*.sandbox.mode` | no | -| `tools.exec.host_sandbox_no_sandbox_defaults` | warn | `exec host=sandbox` resolves to host exec 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` resolves to host exec when sandbox is off | `agents.list[].tools.exec.host`, `agents.list[].sandbox.mode` | no | +| `checkId` | Severity | Why it matters | Primary fix key/path | Auto-fix | +| -------------------------------------------------- | ------------- | ------------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------- | -------- | +| `fs.state_dir.perms_world_writable` | critical | Other users/processes can modify full OpenClaw state | filesystem perms on `~/.openclaw` | yes | +| `fs.config.perms_writable` | critical | Others can change auth/tool policy/config | filesystem perms on `~/.openclaw/openclaw.json` | yes | +| `fs.config.perms_world_readable` | critical | Config can expose tokens/settings | filesystem perms on config file | yes | +| `gateway.bind_no_auth` | critical | Remote bind without shared secret | `gateway.bind`, `gateway.auth.*` | no | +| `gateway.loopback_no_auth` | critical | Reverse-proxied loopback may become unauthenticated | `gateway.auth.*`, proxy setup | no | +| `gateway.http.no_auth` | warn/critical | Gateway HTTP APIs reachable with `auth.mode="none"` | `gateway.auth.mode`, `gateway.http.endpoints.*` | no | +| `gateway.tools_invoke_http.dangerous_allow` | warn/critical | Re-enables dangerous tools over HTTP API | `gateway.tools.allow` | no | +| `gateway.nodes.allow_commands_dangerous` | warn/critical | Enables high-impact node commands (camera/screen/contacts/calendar/SMS) | `gateway.nodes.allowCommands` | no | +| `gateway.tailscale_funnel` | critical | Public internet exposure | `gateway.tailscale.mode` | no | +| `gateway.control_ui.insecure_auth` | warn | Insecure-auth compatibility toggle enabled | `gateway.controlUi.allowInsecureAuth` | no | +| `gateway.control_ui.device_auth_disabled` | critical | Disables device identity check | `gateway.controlUi.dangerouslyDisableDeviceAuth` | no | +| `config.insecure_or_dangerous_flags` | warn | Any insecure/dangerous debug flags enabled | multiple keys (see finding detail) | no | +| `hooks.token_too_short` | warn | Easier brute force on hook ingress | `hooks.token` | no | +| `hooks.request_session_key_enabled` | warn/critical | External caller can choose sessionKey | `hooks.allowRequestSessionKey` | no | +| `hooks.request_session_key_prefixes_missing` | warn/critical | No bound on external session key shapes | `hooks.allowedSessionKeyPrefixes` | no | +| `logging.redact_off` | warn | Sensitive values leak to logs/status | `logging.redactSensitive` | yes | +| `sandbox.docker_config_mode_off` | warn | Sandbox Docker config present but inactive | `agents.*.sandbox.mode` | no | +| `tools.exec.host_sandbox_no_sandbox_defaults` | warn | `exec host=sandbox` resolves to host exec 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` resolves to host exec when sandbox is off | `agents.list[].tools.exec.host`, `agents.list[].sandbox.mode` | no | | `security.exposure.open_groups_with_runtime_or_fs` | critical/warn | Open groups can reach command/file tools without sandbox/workspace guards | `channels.*.groupPolicy`, `tools.profile/deny`, `tools.fs.workspaceOnly`, `agents.*.sandbox.mode` | no | -| `tools.profile_minimal_overridden` | warn | Agent overrides bypass global minimal profile | `agents.list[].tools.profile` | no | -| `plugins.tools_reachable_permissive_policy` | warn | Extension tools reachable in permissive contexts | `tools.profile` + tool allow/deny | no | -| `models.small_params` | critical/info | Small models + unsafe tool surfaces raise injection risk | model choice + sandbox/tool policy | no | +| `tools.profile_minimal_overridden` | warn | Agent overrides bypass global minimal profile | `agents.list[].tools.profile` | no | +| `plugins.tools_reachable_permissive_policy` | warn | Extension tools reachable in permissive contexts | `tools.profile` + tool allow/deny | no | +| `models.small_params` | critical/info | Small models + unsafe tool surfaces raise injection risk | model choice + sandbox/tool policy | no | ## Control UI over HTTP diff --git a/src/config/schema.help.ts b/src/config/schema.help.ts index 75f6bb82062..144a72ecd23 100644 --- a/src/config/schema.help.ts +++ b/src/config/schema.help.ts @@ -51,7 +51,7 @@ export const FIELD_HELP: Record = { 'Node browser routing ("auto" = pick single connected browser node, "manual" = require node param, "off" = disable).', "gateway.nodes.browser.node": "Pin browser routing to a specific node id or name (optional).", "gateway.nodes.allowCommands": - "Extra node.invoke commands to allow beyond the gateway defaults (array of command strings).", + "Extra node.invoke commands to allow beyond the gateway defaults (array of command strings). Enabling dangerous commands here is a security-sensitive override and is flagged by `openclaw security audit`.", "gateway.nodes.denyCommands": "Commands to block even if present in node claims or default allowlist.", "nodeHost.browserProxy.enabled": "Expose the local browser control server via node proxy.", diff --git a/src/gateway/control-plane-rate-limit.ts b/src/gateway/control-plane-rate-limit.ts index b7a3fc49dcc..6e05a53e30d 100644 --- a/src/gateway/control-plane-rate-limit.ts +++ b/src/gateway/control-plane-rate-limit.ts @@ -21,6 +21,13 @@ function normalizePart(value: unknown, fallback: string): string { export function resolveControlPlaneRateLimitKey(client: GatewayClient | null): string { const deviceId = normalizePart(client?.connect?.device?.id, "unknown-device"); const clientIp = normalizePart(client?.clientIp, "unknown-ip"); + if (deviceId === "unknown-device" && clientIp === "unknown-ip") { + // Last-resort fallback: avoid cross-client contention when upstream identity is missing. + const connId = normalizePart(client?.connId, ""); + if (connId) { + return `${deviceId}|${clientIp}|conn=${connId}`; + } + } return `${deviceId}|${clientIp}`; } diff --git a/src/gateway/server-methods.control-plane-rate-limit.test.ts b/src/gateway/server-methods.control-plane-rate-limit.test.ts index a9174a746a7..364e817c66a 100644 --- a/src/gateway/server-methods.control-plane-rate-limit.test.ts +++ b/src/gateway/server-methods.control-plane-rate-limit.test.ts @@ -1,5 +1,8 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; -import { __testing as controlPlaneRateLimitTesting } from "./control-plane-rate-limit.js"; +import { + __testing as controlPlaneRateLimitTesting, + resolveControlPlaneRateLimitKey, +} from "./control-plane-rate-limit.js"; import { handleGatewayRequest } from "./server-methods.js"; import type { GatewayRequestHandler } from "./server-methods/types.js"; @@ -121,4 +124,43 @@ describe("gateway control-plane write rate limit", () => { expect(allowed).toHaveBeenCalledWith(true, undefined, undefined); expect(handlerCalls).toHaveBeenCalledTimes(4); }); + + it("uses connId fallback when both device and client IP are unknown", () => { + const key = resolveControlPlaneRateLimitKey({ + connect: { + role: "operator", + scopes: ["operator.admin"], + client: { + id: "openclaw-control-ui", + version: "1.0.0", + platform: "darwin", + mode: "ui", + }, + minProtocol: 1, + maxProtocol: 1, + }, + connId: "conn-fallback", + }); + expect(key).toBe("unknown-device|unknown-ip|conn=conn-fallback"); + }); + + it("keeps device/IP-based key when identity is present", () => { + const key = resolveControlPlaneRateLimitKey({ + connect: { + role: "operator", + scopes: ["operator.admin"], + client: { + id: "openclaw-control-ui", + version: "1.0.0", + platform: "darwin", + mode: "ui", + }, + minProtocol: 1, + maxProtocol: 1, + }, + connId: "conn-fallback", + clientIp: "10.0.0.10", + }); + expect(key).toBe("unknown-device|10.0.0.10"); + }); }); diff --git a/src/security/audit-extra.sync.ts b/src/security/audit-extra.sync.ts index 0fe7a8a6157..fa13e9b53f7 100644 --- a/src/security/audit-extra.sync.ts +++ b/src/security/audit-extra.sync.ts @@ -16,7 +16,10 @@ import { formatCliCommand } from "../cli/command-format.js"; import type { OpenClawConfig } from "../config/config.js"; import type { AgentToolsConfig } from "../config/types.tools.js"; import { resolveGatewayAuth } from "../gateway/auth.js"; -import { resolveNodeCommandAllowlist } from "../gateway/node-command-policy.js"; +import { + DEFAULT_DANGEROUS_NODE_COMMANDS, + resolveNodeCommandAllowlist, +} from "../gateway/node-command-policy.js"; import { inferParamBFromIdOrName } from "../shared/model-param-b.js"; import { pickSandboxToolPolicy } from "./audit-tool-policy.js"; @@ -805,6 +808,43 @@ export function collectNodeDenyCommandPatternFindings(cfg: OpenClawConfig): Secu return findings; } +export function collectNodeDangerousAllowCommandFindings( + cfg: OpenClawConfig, +): SecurityAuditFinding[] { + const findings: SecurityAuditFinding[] = []; + const allowRaw = cfg.gateway?.nodes?.allowCommands; + if (!Array.isArray(allowRaw) || allowRaw.length === 0) { + return findings; + } + + const allow = new Set(allowRaw.map(normalizeNodeCommand).filter(Boolean)); + if (allow.size === 0) { + return findings; + } + + const deny = new Set((cfg.gateway?.nodes?.denyCommands ?? []).map(normalizeNodeCommand)); + const dangerousAllowed = DEFAULT_DANGEROUS_NODE_COMMANDS.filter( + (cmd) => allow.has(cmd) && !deny.has(cmd), + ); + if (dangerousAllowed.length === 0) { + return findings; + } + + findings.push({ + checkId: "gateway.nodes.allow_commands_dangerous", + severity: isGatewayRemotelyExposed(cfg) ? "critical" : "warn", + title: "Dangerous node commands explicitly enabled", + detail: + `gateway.nodes.allowCommands includes: ${dangerousAllowed.join(", ")}. ` + + "These commands can trigger high-impact device actions (camera/screen/contacts/calendar/reminders/SMS).", + remediation: + "Remove these entries from gateway.nodes.allowCommands (recommended). " + + "If you keep them, treat gateway auth as full operator access and keep gateway exposure local/tailnet-only.", + }); + + return findings; +} + export function collectMinimalProfileOverrideFindings(cfg: OpenClawConfig): SecurityAuditFinding[] { const findings: SecurityAuditFinding[] = []; if (cfg.tools?.profile !== "minimal") { diff --git a/src/security/audit-extra.ts b/src/security/audit-extra.ts index d38e753ca3e..fa2b82fa150 100644 --- a/src/security/audit-extra.ts +++ b/src/security/audit-extra.ts @@ -16,6 +16,7 @@ export { collectHooksHardeningFindings, collectMinimalProfileOverrideFindings, collectModelHygieneFindings, + collectNodeDangerousAllowCommandFindings, collectNodeDenyCommandPatternFindings, collectSandboxDangerousConfigFindings, collectSandboxDockerNoopFindings, diff --git a/src/security/audit.test.ts b/src/security/audit.test.ts index 0bdc93463ff..5eb4651f7f5 100644 --- a/src/security/audit.test.ts +++ b/src/security/audit.test.ts @@ -767,6 +767,59 @@ describe("security audit", () => { expect(finding?.detail).toContain("system.runx"); }); + it("scores dangerous gateway.nodes.allowCommands by exposure", async () => { + const cases: Array<{ + name: string; + cfg: OpenClawConfig; + expectedSeverity: "warn" | "critical"; + }> = [ + { + name: "loopback gateway", + cfg: { + gateway: { + bind: "loopback", + nodes: { allowCommands: ["camera.snap", "screen.record"] }, + }, + }, + expectedSeverity: "warn", + }, + { + name: "lan-exposed gateway", + cfg: { + gateway: { + bind: "lan", + nodes: { allowCommands: ["camera.snap", "screen.record"] }, + }, + }, + expectedSeverity: "critical", + }, + ]; + + for (const testCase of cases) { + const res = await audit(testCase.cfg); + const finding = res.findings.find( + (f) => f.checkId === "gateway.nodes.allow_commands_dangerous", + ); + expect(finding?.severity, testCase.name).toBe(testCase.expectedSeverity); + expect(finding?.detail, testCase.name).toContain("camera.snap"); + expect(finding?.detail, testCase.name).toContain("screen.record"); + } + }); + + it("does not flag dangerous allowCommands entries when denied again", async () => { + const cfg: OpenClawConfig = { + gateway: { + nodes: { + allowCommands: ["camera.snap", "screen.record"], + denyCommands: ["camera.snap", "screen.record"], + }, + }, + }; + + const res = await audit(cfg); + expectNoFinding(res, "gateway.nodes.allow_commands_dangerous"); + }); + it("flags agent profile overrides when global tools.profile is minimal", async () => { const cfg: OpenClawConfig = { tools: { diff --git a/src/security/audit.ts b/src/security/audit.ts index 92bf54f49e5..dc6d14a14cb 100644 --- a/src/security/audit.ts +++ b/src/security/audit.ts @@ -22,6 +22,7 @@ import { collectSandboxBrowserHashLabelFindings, collectMinimalProfileOverrideFindings, collectModelHygieneFindings, + collectNodeDangerousAllowCommandFindings, collectNodeDenyCommandPatternFindings, collectSmallModelRiskFindings, collectSandboxDangerousConfigFindings, @@ -717,6 +718,7 @@ export async function runSecurityAudit(opts: SecurityAuditOptions): Promise