diff --git a/CHANGELOG.md b/CHANGELOG.md index 7a0acae2c92..3533fac991c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,7 @@ Docs: https://docs.openclaw.ai - Security/Audit: add `openclaw security audit` detection for open group policies that expose runtime/filesystem tools without sandbox/workspace guards (`security.exposure.open_groups_with_runtime_or_fs`). - Security/Audit: make `gateway.real_ip_fallback_enabled` severity conditional for loopback trusted-proxy setups (warn for loopback-only `trustedProxies`, critical when non-loopback proxies are trusted). (#23428) Thanks @bmendonca3. - Security/Exec env: block request-scoped `HOME` and `ZDOTDIR` overrides in host exec env sanitizers (Node + macOS), preventing shell startup-file execution before allowlist-evaluated command bodies. This ships in the next npm release. Thanks @tdjackey for reporting. +- Security/Exec env: block `SHELLOPTS`/`PS4` in host exec env sanitizers and restrict shell-wrapper (`bash|sh|zsh ... -c/-lc`) request env overrides to a small explicit allowlist (`TERM`, `LANG`, `LC_*`, `COLORTERM`, `NO_COLOR`, `FORCE_COLOR`) on both node host and macOS companion paths, preventing xtrace prompt command-substitution allowlist bypasses. This ships in the next npm release. Thanks @tdjackey for reporting. - Security/Gateway: emit a startup security warning when insecure/dangerous config flags are enabled (including `gateway.controlUi.dangerouslyDisableDeviceAuth=true`) and point operators to `openclaw security audit`. - Security/Hooks auth: normalize hook auth rate-limit client IP keys so IPv4 and IPv4-mapped IPv6 addresses share one throttle bucket, preventing dual-form auth-attempt budget bypasses. This ships in the next npm release. Thanks @aether-ai-agent for reporting. - Security/Exec approvals: treat `env` and shell-dispatch wrappers as transparent during allowlist analysis on node-host and macOS companion paths so policy checks match the effective executable/inline shell payload instead of the wrapper binary, blocking wrapper-smuggled allowlist bypasses. This ships in the next npm release. Thanks @tdjackey for reporting. diff --git a/apps/macos/Sources/OpenClaw/ExecApprovalEvaluation.swift b/apps/macos/Sources/OpenClaw/ExecApprovalEvaluation.swift index 7bb05aff0c9..c7d9d0928e1 100644 --- a/apps/macos/Sources/OpenClaw/ExecApprovalEvaluation.swift +++ b/apps/macos/Sources/OpenClaw/ExecApprovalEvaluation.swift @@ -28,7 +28,8 @@ enum ExecApprovalEvaluator { let approvals = ExecApprovalsStore.resolve(agentId: normalizedAgentId) let security = approvals.agent.security let ask = approvals.agent.ask - let env = HostEnvSanitizer.sanitize(overrides: envOverrides) + let shellWrapper = ExecShellWrapperParser.extract(command: command, rawCommand: rawCommand).isWrapper + let env = HostEnvSanitizer.sanitize(overrides: envOverrides, shellWrapper: shellWrapper) let displayCommand = ExecCommandFormatter.displayString(for: command, rawCommand: rawCommand) let allowlistResolutions = ExecCommandResolution.resolveForAllowlist( command: command, diff --git a/apps/macos/Sources/OpenClaw/HostEnvSanitizer.swift b/apps/macos/Sources/OpenClaw/HostEnvSanitizer.swift index 846c8978191..b9b993299a9 100644 --- a/apps/macos/Sources/OpenClaw/HostEnvSanitizer.swift +++ b/apps/macos/Sources/OpenClaw/HostEnvSanitizer.swift @@ -15,6 +15,8 @@ enum HostEnvSanitizer { "BASH_ENV", "ENV", "SHELL", + "SHELLOPTS", + "PS4", "GCONV_PATH", "IFS", "SSLKEYLOGFILE", @@ -29,13 +31,36 @@ enum HostEnvSanitizer { "HOME", "ZDOTDIR", ] + private static let shellWrapperAllowedOverrideKeys: Set = [ + "TERM", + "LANG", + "LC_ALL", + "LC_CTYPE", + "LC_MESSAGES", + "COLORTERM", + "NO_COLOR", + "FORCE_COLOR", + ] private static func isBlocked(_ upperKey: String) -> Bool { if self.blockedKeys.contains(upperKey) { return true } return self.blockedPrefixes.contains(where: { upperKey.hasPrefix($0) }) } - static func sanitize(overrides: [String: String]?) -> [String: String] { + private static func filterOverridesForShellWrapper(_ overrides: [String: String]?) -> [String: String]? { + guard let overrides else { return nil } + var filtered: [String: String] = [:] + for (rawKey, value) in overrides { + let key = rawKey.trimmingCharacters(in: .whitespacesAndNewlines) + guard !key.isEmpty else { continue } + if self.shellWrapperAllowedOverrideKeys.contains(key.uppercased()) { + filtered[key] = value + } + } + return filtered.isEmpty ? nil : filtered + } + + static func sanitize(overrides: [String: String]?, shellWrapper: Bool = false) -> [String: String] { var merged: [String: String] = [:] for (rawKey, value) in ProcessInfo.processInfo.environment { let key = rawKey.trimmingCharacters(in: .whitespacesAndNewlines) @@ -45,8 +70,12 @@ enum HostEnvSanitizer { merged[key] = value } - guard let overrides else { return merged } - for (rawKey, value) in overrides { + let effectiveOverrides = shellWrapper + ? self.filterOverridesForShellWrapper(overrides) + : overrides + + guard let effectiveOverrides else { return merged } + for (rawKey, value) in effectiveOverrides { let key = rawKey.trimmingCharacters(in: .whitespacesAndNewlines) guard !key.isEmpty else { continue } let upper = key.uppercased() diff --git a/apps/macos/Tests/OpenClawIPCTests/HostEnvSanitizerTests.swift b/apps/macos/Tests/OpenClawIPCTests/HostEnvSanitizerTests.swift new file mode 100644 index 00000000000..7ee15107f40 --- /dev/null +++ b/apps/macos/Tests/OpenClawIPCTests/HostEnvSanitizerTests.swift @@ -0,0 +1,36 @@ +import Testing +@testable import OpenClaw + +struct HostEnvSanitizerTests { + @Test func sanitizeBlocksShellTraceVariables() { + let env = HostEnvSanitizer.sanitize(overrides: [ + "SHELLOPTS": "xtrace", + "PS4": "$(touch /tmp/pwned)", + "OPENCLAW_TEST": "1", + ]) + #expect(env["SHELLOPTS"] == nil) + #expect(env["PS4"] == nil) + #expect(env["OPENCLAW_TEST"] == "1") + } + + @Test func sanitizeShellWrapperAllowsOnlyExplicitOverrideKeys() { + let env = HostEnvSanitizer.sanitize( + overrides: [ + "LANG": "C", + "LC_ALL": "C", + "OPENCLAW_TOKEN": "secret", + "PS4": "$(touch /tmp/pwned)", + ], + shellWrapper: true) + + #expect(env["LANG"] == "C") + #expect(env["LC_ALL"] == "C") + #expect(env["OPENCLAW_TOKEN"] == nil) + #expect(env["PS4"] == nil) + } + + @Test func sanitizeNonShellWrapperKeepsRegularOverrides() { + let env = HostEnvSanitizer.sanitize(overrides: ["OPENCLAW_TOKEN": "secret"]) + #expect(env["OPENCLAW_TOKEN"] == "secret") + } +} diff --git a/docs/nodes/index.md b/docs/nodes/index.md index 9a6f3f1f724..430d07299db 100644 --- a/docs/nodes/index.md +++ b/docs/nodes/index.md @@ -278,8 +278,9 @@ Notes: - `system.run` returns stdout/stderr/exit code in the payload. - `system.notify` respects notification permission state on the macOS app. - `system.run` supports `--cwd`, `--env KEY=VAL`, `--command-timeout`, and `--needs-screen-recording`. +- For shell wrappers (`bash|sh|zsh ... -c/-lc`), request-scoped `--env` values are reduced to an explicit allowlist (`TERM`, `LANG`, `LC_*`, `COLORTERM`, `NO_COLOR`, `FORCE_COLOR`). - `system.notify` supports `--priority ` and `--delivery `. -- Node hosts ignore `PATH` overrides. If you need extra PATH entries, configure the node host service environment (or install tools in standard locations) instead of passing `PATH` via `--env`. +- Node hosts ignore `PATH` overrides and strip dangerous startup/shell keys (`DYLD_*`, `LD_*`, `NODE_OPTIONS`, `PYTHON*`, `PERL*`, `RUBYOPT`, `SHELLOPTS`, `PS4`). If you need extra PATH entries, configure the node host service environment (or install tools in standard locations) instead of passing `PATH` via `--env`. - On macOS node mode, `system.run` is gated by exec approvals in the macOS app (Settings → Exec approvals). Ask/allowlist/full behave the same as the headless node host; denied prompts return `SYSTEM_RUN_DENIED`. - On headless node host, `system.run` is gated by exec approvals (`~/.openclaw/exec-approvals.json`). diff --git a/docs/platforms/macos.md b/docs/platforms/macos.md index 730d7015ad5..ce56aa107bf 100644 --- a/docs/platforms/macos.md +++ b/docs/platforms/macos.md @@ -105,7 +105,8 @@ Notes: - `allowlist` entries are glob patterns for resolved binary paths. - Raw shell command text that contains shell control or expansion syntax (`&&`, `||`, `;`, `|`, `` ` ``, `$`, `<`, `>`, `(`, `)`) is treated as an allowlist miss and requires explicit approval (or allowlisting the shell binary). - Choosing “Always Allow” in the prompt adds that command to the allowlist. -- `system.run` environment overrides are filtered (drops `PATH`, `DYLD_*`, `LD_*`, `NODE_OPTIONS`, `PYTHON*`, `PERL*`, `RUBYOPT`) and then merged with the app’s environment. +- `system.run` environment overrides are filtered (drops `PATH`, `DYLD_*`, `LD_*`, `NODE_OPTIONS`, `PYTHON*`, `PERL*`, `RUBYOPT`, `SHELLOPTS`, `PS4`) and then merged with the app’s environment. +- For shell wrappers (`bash|sh|zsh ... -c/-lc`), request-scoped environment overrides are reduced to a small explicit allowlist (`TERM`, `LANG`, `LC_*`, `COLORTERM`, `NO_COLOR`, `FORCE_COLOR`). ## Deep links diff --git a/docs/tools/exec-approvals.md b/docs/tools/exec-approvals.md index f977952c83c..e57c738b8d7 100644 --- a/docs/tools/exec-approvals.md +++ b/docs/tools/exec-approvals.md @@ -155,6 +155,8 @@ double quotes; use single quotes if you need literal `$()` text. On macOS companion-app approvals, raw shell text containing shell control or expansion syntax (`&&`, `||`, `;`, `|`, `` ` ``, `$`, `<`, `>`, `(`, `)`) is treated as an allowlist miss unless the shell binary itself is allowlisted. +For shell wrappers (`bash|sh|zsh ... -c/-lc`), request-scoped env overrides are reduced to a +small explicit allowlist (`TERM`, `LANG`, `LC_*`, `COLORTERM`, `NO_COLOR`, `FORCE_COLOR`). Default safe bins: `jq`, `cut`, `uniq`, `head`, `tail`, `tr`, `wc`. diff --git a/src/infra/host-env-security-policy.json b/src/infra/host-env-security-policy.json index 341af1c5db3..8b3ec80d5b4 100644 --- a/src/infra/host-env-security-policy.json +++ b/src/infra/host-env-security-policy.json @@ -11,6 +11,8 @@ "BASH_ENV", "ENV", "SHELL", + "SHELLOPTS", + "PS4", "GCONV_PATH", "IFS", "SSLKEYLOGFILE" diff --git a/src/infra/host-env-security.test.ts b/src/infra/host-env-security.test.ts index df1ccd874b8..47ef53a6b9a 100644 --- a/src/infra/host-env-security.test.ts +++ b/src/infra/host-env-security.test.ts @@ -1,9 +1,14 @@ +import { spawn } from "node:child_process"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; import { describe, expect, it } from "vitest"; import { isDangerousHostEnvOverrideVarName, isDangerousHostEnvVarName, normalizeEnvVarKey, sanitizeHostExecEnv, + sanitizeSystemRunEnvOverrides, } from "./host-env-security.js"; describe("isDangerousHostEnvVarName", () => { @@ -11,6 +16,8 @@ describe("isDangerousHostEnvVarName", () => { expect(isDangerousHostEnvVarName("BASH_ENV")).toBe(true); expect(isDangerousHostEnvVarName("bash_env")).toBe(true); expect(isDangerousHostEnvVarName("SHELL")).toBe(true); + expect(isDangerousHostEnvVarName("SHELLOPTS")).toBe(true); + expect(isDangerousHostEnvVarName("ps4")).toBe(true); expect(isDangerousHostEnvVarName("DYLD_INSERT_LIBRARIES")).toBe(true); expect(isDangerousHostEnvVarName("ld_preload")).toBe(true); expect(isDangerousHostEnvVarName("BASH_FUNC_echo%%")).toBe(true); @@ -48,17 +55,37 @@ describe("sanitizeHostExecEnv", () => { HOME: "/tmp/evil-home", ZDOTDIR: "/tmp/evil-zdotdir", BASH_ENV: "/tmp/pwn.sh", + SHELLOPTS: "xtrace", + PS4: "$(touch /tmp/pwned)", SAFE: "ok", }, }); expect(env.PATH).toBe("/usr/bin:/bin"); expect(env.BASH_ENV).toBeUndefined(); + expect(env.SHELLOPTS).toBeUndefined(); + expect(env.PS4).toBeUndefined(); expect(env.SAFE).toBe("ok"); expect(env.HOME).toBe("/tmp/trusted-home"); expect(env.ZDOTDIR).toBe("/tmp/trusted-zdotdir"); }); + it("drops dangerous inherited shell trace keys", () => { + const env = sanitizeHostExecEnv({ + baseEnv: { + PATH: "/usr/bin:/bin", + SHELLOPTS: "xtrace", + PS4: "$(touch /tmp/pwned)", + OK: "1", + }, + }); + + expect(env.PATH).toBe("/usr/bin:/bin"); + expect(env.OK).toBe("1"); + expect(env.SHELLOPTS).toBeUndefined(); + expect(env.PS4).toBeUndefined(); + }); + it("drops non-portable env key names", () => { const env = sanitizeHostExecEnv({ baseEnv: { @@ -94,3 +121,72 @@ describe("normalizeEnvVarKey", () => { expect(normalizeEnvVarKey(" ")).toBeNull(); }); }); + +describe("sanitizeSystemRunEnvOverrides", () => { + it("keeps overrides for non-shell commands", () => { + const overrides = sanitizeSystemRunEnvOverrides({ + shellWrapper: false, + overrides: { + OPENCLAW_TEST: "1", + TOKEN: "abc", + }, + }); + expect(overrides).toEqual({ + OPENCLAW_TEST: "1", + TOKEN: "abc", + }); + }); + + it("drops non-allowlisted overrides for shell wrappers", () => { + const overrides = sanitizeSystemRunEnvOverrides({ + shellWrapper: true, + overrides: { + OPENCLAW_TEST: "1", + TOKEN: "abc", + LANG: "C", + LC_ALL: "C", + }, + }); + expect(overrides).toEqual({ + LANG: "C", + LC_ALL: "C", + }); + }); +}); + +describe("shell wrapper exploit regression", () => { + it("blocks SHELLOPTS/PS4 chain after sanitization", async () => { + const bashPath = "/bin/bash"; + if (process.platform === "win32" || !fs.existsSync(bashPath)) { + return; + } + const marker = path.join(os.tmpdir(), `openclaw-ps4-marker-${process.pid}-${Date.now()}`); + try { + fs.unlinkSync(marker); + } catch { + // no-op + } + + const filteredOverrides = sanitizeSystemRunEnvOverrides({ + shellWrapper: true, + overrides: { + SHELLOPTS: "xtrace", + PS4: `$(touch ${marker})`, + }, + }); + const env = sanitizeHostExecEnv({ + overrides: filteredOverrides, + baseEnv: { + PATH: process.env.PATH ?? "/usr/bin:/bin", + }, + }); + + await new Promise((resolve, reject) => { + const child = spawn(bashPath, ["-lc", "echo SAFE"], { env, stdio: "ignore" }); + child.once("error", reject); + child.once("close", () => resolve()); + }); + + expect(fs.existsSync(marker)).toBe(false); + }); +}); diff --git a/src/infra/host-env-security.ts b/src/infra/host-env-security.ts index b1d869cf9a2..79ccd1f0a7a 100644 --- a/src/infra/host-env-security.ts +++ b/src/infra/host-env-security.ts @@ -19,10 +19,23 @@ export const HOST_DANGEROUS_ENV_PREFIXES: readonly string[] = Object.freeze( export const HOST_DANGEROUS_OVERRIDE_ENV_KEY_VALUES: readonly string[] = Object.freeze( (HOST_ENV_SECURITY_POLICY.blockedOverrideKeys ?? []).map((key) => key.toUpperCase()), ); +export const HOST_SHELL_WRAPPER_ALLOWED_OVERRIDE_ENV_KEY_VALUES: readonly string[] = Object.freeze([ + "TERM", + "LANG", + "LC_ALL", + "LC_CTYPE", + "LC_MESSAGES", + "COLORTERM", + "NO_COLOR", + "FORCE_COLOR", +]); export const HOST_DANGEROUS_ENV_KEYS = new Set(HOST_DANGEROUS_ENV_KEY_VALUES); export const HOST_DANGEROUS_OVERRIDE_ENV_KEYS = new Set( HOST_DANGEROUS_OVERRIDE_ENV_KEY_VALUES, ); +export const HOST_SHELL_WRAPPER_ALLOWED_OVERRIDE_ENV_KEYS = new Set( + HOST_SHELL_WRAPPER_ALLOWED_OVERRIDE_ENV_KEY_VALUES, +); export function normalizeEnvVarKey( rawKey: string, @@ -105,3 +118,31 @@ export function sanitizeHostExecEnv(params?: { return merged; } + +export function sanitizeSystemRunEnvOverrides(params?: { + overrides?: Record | null; + shellWrapper?: boolean; +}): Record | undefined { + const overrides = params?.overrides ?? undefined; + if (!overrides) { + return undefined; + } + if (!params?.shellWrapper) { + return overrides; + } + const filtered: Record = {}; + for (const [rawKey, value] of Object.entries(overrides)) { + if (typeof value !== "string") { + continue; + } + const key = normalizeEnvVarKey(rawKey, { portable: true }); + if (!key) { + continue; + } + if (!HOST_SHELL_WRAPPER_ALLOWED_OVERRIDE_ENV_KEYS.has(key.toUpperCase())) { + continue; + } + filtered[key] = value; + } + return Object.keys(filtered).length > 0 ? filtered : undefined; +} diff --git a/src/node-host/invoke-system-run.ts b/src/node-host/invoke-system-run.ts index c22a65b5120..8bac68d9a73 100644 --- a/src/node-host/invoke-system-run.ts +++ b/src/node-host/invoke-system-run.ts @@ -19,6 +19,7 @@ import { } from "../infra/exec-approvals.js"; import type { ExecHostRequest, ExecHostResponse, ExecHostRunResult } from "../infra/exec-host.js"; import { getTrustedSafeBinDirs } from "../infra/exec-safe-bin-trust.js"; +import { sanitizeSystemRunEnvOverrides } from "../infra/host-env-security.js"; import { resolveSystemRunCommand } from "../infra/system-run-command.js"; import type { ExecEventPayload, @@ -109,7 +110,11 @@ export async function handleSystemRunInvoke(opts: { const autoAllowSkills = approvals.agent.autoAllowSkills; const sessionKey = opts.params.sessionKey?.trim() || "node"; const runId = opts.params.runId?.trim() || crypto.randomUUID(); - const env = opts.sanitizeEnv(opts.params.env ?? undefined); + const envOverrides = sanitizeSystemRunEnvOverrides({ + overrides: opts.params.env ?? undefined, + shellWrapper: shellCommand !== null, + }); + const env = opts.sanitizeEnv(envOverrides); const safeBins = resolveSafeBins(agentExec?.safeBins ?? cfg.tools?.exec?.safeBins); const trustedSafeBinDirs = getTrustedSafeBinDirs(); const bins = autoAllowSkills ? await opts.skillBins.current() : new Set(); @@ -171,7 +176,7 @@ export async function handleSystemRunInvoke(opts: { command: argv, rawCommand: rawCommand || shellCommand || null, cwd: opts.params.cwd ?? null, - env: opts.params.env ?? null, + env: envOverrides ?? null, timeoutMs: opts.params.timeoutMs ?? null, needsScreenRecording: opts.params.needsScreenRecording ?? null, agentId: agentId ?? null, diff --git a/src/node-host/invoke.sanitize-env.test.ts b/src/node-host/invoke.sanitize-env.test.ts index fe91432198b..dfa44ccd0c2 100644 --- a/src/node-host/invoke.sanitize-env.test.ts +++ b/src/node-host/invoke.sanitize-env.test.ts @@ -12,18 +12,25 @@ describe("node-host sanitizeEnv", () => { }); it("blocks dangerous env keys/prefixes", () => { - withEnv({ PYTHONPATH: undefined, LD_PRELOAD: undefined, BASH_ENV: undefined }, () => { - const env = sanitizeEnv({ - PYTHONPATH: "/tmp/pwn", - LD_PRELOAD: "/tmp/pwn.so", - BASH_ENV: "/tmp/pwn.sh", - FOO: "bar", - }); - expect(env.FOO).toBe("bar"); - expect(env.PYTHONPATH).toBeUndefined(); - expect(env.LD_PRELOAD).toBeUndefined(); - expect(env.BASH_ENV).toBeUndefined(); - }); + withEnv( + { PYTHONPATH: undefined, LD_PRELOAD: undefined, BASH_ENV: undefined, SHELLOPTS: undefined }, + () => { + const env = sanitizeEnv({ + PYTHONPATH: "/tmp/pwn", + LD_PRELOAD: "/tmp/pwn.so", + BASH_ENV: "/tmp/pwn.sh", + SHELLOPTS: "xtrace", + PS4: "$(touch /tmp/pwned)", + FOO: "bar", + }); + expect(env.FOO).toBe("bar"); + expect(env.PYTHONPATH).toBeUndefined(); + expect(env.LD_PRELOAD).toBeUndefined(); + expect(env.BASH_ENV).toBeUndefined(); + expect(env.SHELLOPTS).toBeUndefined(); + expect(env.PS4).toBeUndefined(); + }, + ); }); it("blocks dangerous override-only env keys", () => {