From 7abfff756d6c68d17e21d1657bbacbaec86de232 Mon Sep 17 00:00:00 2001 From: Josh Avant <830519+joshavant@users.noreply.github.com> Date: Fri, 20 Mar 2026 15:44:15 -0500 Subject: [PATCH] Exec: harden host env override handling across gateway and node (#51207) * Exec: harden host env override enforcement and fail closed * Node host: enforce env override diagnostics before shell filtering * Env overrides: align Windows key handling and mac node rejection --- CHANGELOG.md | 1 + .../Sources/OpenClaw/HostEnvSanitizer.swift | 69 ++++++++- .../HostEnvSecurityPolicy.generated.swift | 18 ++- .../OpenClaw/NodeMode/MacNodeRuntime.swift | 17 +++ .../HostEnvSanitizerTests.swift | 20 +++ .../MacNodeRuntimeTests.swift | 26 ++++ src/agents/bash-tools.exec.path.test.ts | 16 ++ src/agents/bash-tools.exec.ts | 69 ++++++--- src/infra/host-env-security-policy.json | 18 ++- src/infra/host-env-security.test.ts | 61 +++++++- src/infra/host-env-security.ts | 141 +++++++++++++++--- src/node-host/invoke-system-run.test.ts | 61 ++++++++ src/node-host/invoke-system-run.ts | 33 +++- src/node-host/invoke.sanitize-env.test.ts | 7 + 14 files changed, 510 insertions(+), 47 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 15fe8b08613..4f533794769 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -153,6 +153,7 @@ Docs: https://docs.openclaw.ai - Hardening: refresh stale device pairing requests and pending metadata (#50695) Thanks @smaeljaish771 and @joshavant. - Gateway: harden OpenResponses file-context escaping (#50782) Thanks @YLChen-007 and @joshavant. - LINE: harden Express webhook parsing to verified raw body (#51202) Thanks @gladiator9797 and @joshavant. +- Exec: harden host env override handling across gateway and node (#51207) Thanks @gladiator9797 and @joshavant. - xAI/models: rename the bundled Grok 4.20 catalog entries to the GA IDs and normalize saved deprecated beta IDs at runtime so existing configs and sessions keep resolving. (#50772) thanks @Jaaneek ### Fixes diff --git a/apps/macos/Sources/OpenClaw/HostEnvSanitizer.swift b/apps/macos/Sources/OpenClaw/HostEnvSanitizer.swift index d5d27a212f5..a3d92efa3f1 100644 --- a/apps/macos/Sources/OpenClaw/HostEnvSanitizer.swift +++ b/apps/macos/Sources/OpenClaw/HostEnvSanitizer.swift @@ -1,5 +1,10 @@ import Foundation +struct HostEnvOverrideDiagnostics: Equatable { + var blockedKeys: [String] + var invalidKeys: [String] +} + enum HostEnvSanitizer { /// Generated from src/infra/host-env-security-policy.json via scripts/generate-host-env-security-policy-swift.mjs. /// Parity is validated by src/infra/host-env-security.policy-parity.test.ts. @@ -41,6 +46,67 @@ enum HostEnvSanitizer { return filtered.isEmpty ? nil : filtered } + private static func isPortableHead(_ scalar: UnicodeScalar) -> Bool { + let value = scalar.value + return value == 95 || (65...90).contains(value) || (97...122).contains(value) + } + + private static func isPortableTail(_ scalar: UnicodeScalar) -> Bool { + let value = scalar.value + return self.isPortableHead(scalar) || (48...57).contains(value) + } + + private static func normalizeOverrideKey(_ rawKey: String) -> String? { + let key = rawKey.trimmingCharacters(in: .whitespacesAndNewlines) + guard !key.isEmpty else { return nil } + guard let first = key.unicodeScalars.first, self.isPortableHead(first) else { + return nil + } + for scalar in key.unicodeScalars.dropFirst() { + if self.isPortableTail(scalar) || scalar == "(" || scalar == ")" { + continue + } + return nil + } + return key + } + + private static func sortedUnique(_ values: [String]) -> [String] { + Array(Set(values)).sorted() + } + + static func inspectOverrides( + overrides: [String: String]?, + blockPathOverrides: Bool = true) -> HostEnvOverrideDiagnostics + { + guard let overrides else { + return HostEnvOverrideDiagnostics(blockedKeys: [], invalidKeys: []) + } + + var blocked: [String] = [] + var invalid: [String] = [] + for (rawKey, _) in overrides { + let candidate = rawKey.trimmingCharacters(in: .whitespacesAndNewlines) + guard let normalized = self.normalizeOverrideKey(rawKey) else { + invalid.append(candidate.isEmpty ? rawKey : candidate) + continue + } + let upper = normalized.uppercased() + if blockPathOverrides, upper == "PATH" { + blocked.append(upper) + continue + } + if self.isBlockedOverride(upper) || self.isBlocked(upper) { + blocked.append(upper) + continue + } + } + + return HostEnvOverrideDiagnostics( + blockedKeys: self.sortedUnique(blocked), + invalidKeys: self.sortedUnique(invalid)) + } + static func sanitize(overrides: [String: String]?, shellWrapper: Bool = false) -> [String: String] { var merged: [String: String] = [:] for (rawKey, value) in ProcessInfo.processInfo.environment { @@ -57,8 +123,7 @@ enum HostEnvSanitizer { guard let effectiveOverrides else { return merged } for (rawKey, value) in effectiveOverrides { - let key = rawKey.trimmingCharacters(in: .whitespacesAndNewlines) - guard !key.isEmpty else { continue } + guard let key = self.normalizeOverrideKey(rawKey) else { continue } let upper = key.uppercased() // PATH is part of the security boundary (command resolution + safe-bin checks). Never // allow request-scoped PATH overrides from agents/gateways. diff --git a/apps/macos/Sources/OpenClaw/HostEnvSecurityPolicy.generated.swift b/apps/macos/Sources/OpenClaw/HostEnvSecurityPolicy.generated.swift index 40db384b226..e45261cda2e 100644 --- a/apps/macos/Sources/OpenClaw/HostEnvSecurityPolicy.generated.swift +++ b/apps/macos/Sources/OpenClaw/HostEnvSecurityPolicy.generated.swift @@ -63,7 +63,23 @@ enum HostEnvSecurityPolicy { "OPENSSL_ENGINES", "PYTHONSTARTUP", "WGETRC", - "CURL_HOME" + "CURL_HOME", + "CLASSPATH", + "CGO_CFLAGS", + "CGO_LDFLAGS", + "GOFLAGS", + "CORECLR_PROFILER_PATH", + "PHPRC", + "PHP_INI_SCAN_DIR", + "DENO_DIR", + "BUN_CONFIG_REGISTRY", + "LUA_PATH", + "LUA_CPATH", + "GEM_HOME", + "GEM_PATH", + "BUNDLE_GEMFILE", + "COMPOSER_HOME", + "XDG_CONFIG_HOME" ] static let blockedOverridePrefixes: [String] = [ diff --git a/apps/macos/Sources/OpenClaw/NodeMode/MacNodeRuntime.swift b/apps/macos/Sources/OpenClaw/NodeMode/MacNodeRuntime.swift index c24f5d0f1b8..956abf94ad6 100644 --- a/apps/macos/Sources/OpenClaw/NodeMode/MacNodeRuntime.swift +++ b/apps/macos/Sources/OpenClaw/NodeMode/MacNodeRuntime.swift @@ -465,6 +465,23 @@ actor MacNodeRuntime { ? params.sessionKey!.trimmingCharacters(in: .whitespacesAndNewlines) : self.mainSessionKey let runId = UUID().uuidString + let envOverrideDiagnostics = HostEnvSanitizer.inspectOverrides( + overrides: params.env, + blockPathOverrides: true) + if !envOverrideDiagnostics.blockedKeys.isEmpty || !envOverrideDiagnostics.invalidKeys.isEmpty { + var details: [String] = [] + if !envOverrideDiagnostics.blockedKeys.isEmpty { + details.append("blocked override keys: \(envOverrideDiagnostics.blockedKeys.joined(separator: ", "))") + } + if !envOverrideDiagnostics.invalidKeys.isEmpty { + details.append( + "invalid non-portable override keys: \(envOverrideDiagnostics.invalidKeys.joined(separator: ", "))") + } + return Self.errorResponse( + req, + code: .invalidRequest, + message: "SYSTEM_RUN_DENIED: environment override rejected (\(details.joined(separator: "; ")))") + } let evaluation = await ExecApprovalEvaluator.evaluate( command: command, rawCommand: params.rawCommand, diff --git a/apps/macos/Tests/OpenClawIPCTests/HostEnvSanitizerTests.swift b/apps/macos/Tests/OpenClawIPCTests/HostEnvSanitizerTests.swift index 1e9da910b2a..55a15419576 100644 --- a/apps/macos/Tests/OpenClawIPCTests/HostEnvSanitizerTests.swift +++ b/apps/macos/Tests/OpenClawIPCTests/HostEnvSanitizerTests.swift @@ -33,4 +33,24 @@ struct HostEnvSanitizerTests { let env = HostEnvSanitizer.sanitize(overrides: ["OPENCLAW_TOKEN": "secret"]) #expect(env["OPENCLAW_TOKEN"] == "secret") } + + @Test func `inspect overrides rejects blocked and invalid keys`() { + let diagnostics = HostEnvSanitizer.inspectOverrides(overrides: [ + "CLASSPATH": "/tmp/evil-classpath", + "BAD-KEY": "x", + "ProgramFiles(x86)": "C:\\Program Files (x86)", + ]) + + #expect(diagnostics.blockedKeys == ["CLASSPATH"]) + #expect(diagnostics.invalidKeys == ["BAD-KEY"]) + } + + @Test func `sanitize accepts Windows-style override key names`() { + let env = HostEnvSanitizer.sanitize(overrides: [ + "ProgramFiles(x86)": "D:\\SDKs", + "CommonProgramFiles(x86)": "D:\\Common", + ]) + #expect(env["ProgramFiles(x86)"] == "D:\\SDKs") + #expect(env["CommonProgramFiles(x86)"] == "D:\\Common") + } } diff --git a/apps/macos/Tests/OpenClawIPCTests/MacNodeRuntimeTests.swift b/apps/macos/Tests/OpenClawIPCTests/MacNodeRuntimeTests.swift index 20b4184f5c9..38c4211f014 100644 --- a/apps/macos/Tests/OpenClawIPCTests/MacNodeRuntimeTests.swift +++ b/apps/macos/Tests/OpenClawIPCTests/MacNodeRuntimeTests.swift @@ -21,6 +21,32 @@ struct MacNodeRuntimeTests { #expect(response.ok == false) } + @Test func `handle invoke rejects blocked system run env override before execution`() async throws { + let runtime = MacNodeRuntime() + let params = OpenClawSystemRunParams( + command: ["/bin/sh", "-lc", "echo ok"], + env: ["CLASSPATH": "/tmp/evil-classpath"]) + let json = try String(data: JSONEncoder().encode(params), encoding: .utf8) + let response = await runtime.handleInvoke( + BridgeInvokeRequest(id: "req-2c", command: OpenClawSystemCommand.run.rawValue, paramsJSON: json)) + #expect(response.ok == false) + #expect(response.error?.message.contains("SYSTEM_RUN_DENIED: environment override rejected") == true) + #expect(response.error?.message.contains("CLASSPATH") == true) + } + + @Test func `handle invoke rejects invalid system run env override key before execution`() async throws { + let runtime = MacNodeRuntime() + let params = OpenClawSystemRunParams( + command: ["/bin/sh", "-lc", "echo ok"], + env: ["BAD-KEY": "x"]) + let json = try String(data: JSONEncoder().encode(params), encoding: .utf8) + let response = await runtime.handleInvoke( + BridgeInvokeRequest(id: "req-2d", command: OpenClawSystemCommand.run.rawValue, paramsJSON: json)) + #expect(response.ok == false) + #expect(response.error?.message.contains("SYSTEM_RUN_DENIED: environment override rejected") == true) + #expect(response.error?.message.contains("BAD-KEY") == true) + } + @Test func `handle invoke rejects empty system which`() async throws { let runtime = MacNodeRuntime() let params = OpenClawSystemWhichParams(bins: []) diff --git a/src/agents/bash-tools.exec.path.test.ts b/src/agents/bash-tools.exec.path.test.ts index 766bfe22107..247c21aede9 100644 --- a/src/agents/bash-tools.exec.path.test.ts +++ b/src/agents/bash-tools.exec.path.test.ts @@ -130,6 +130,22 @@ describe("exec PATH login shell merge", () => { expect(shellPathMock).not.toHaveBeenCalled(); }); + it("fails closed when a blocked runtime override key is requested", async () => { + if (isWin) { + return; + } + const tool = createExecTool({ host: "gateway", security: "full", ask: "off" }); + + await expect( + tool.execute("call-blocked-runtime-env", { + command: "echo ok", + env: { CLASSPATH: "/tmp/evil-classpath" }, + }), + ).rejects.toThrow( + /Security Violation: Environment variable 'CLASSPATH' is forbidden during host execution\./, + ); + }); + it("does not apply login-shell PATH when probe rejects unregistered absolute SHELL", async () => { if (isWin) { return; diff --git a/src/agents/bash-tools.exec.ts b/src/agents/bash-tools.exec.ts index 5fe0f7deac4..dcb50c0344c 100644 --- a/src/agents/bash-tools.exec.ts +++ b/src/agents/bash-tools.exec.ts @@ -3,6 +3,7 @@ import path from "node:path"; import type { AgentTool, AgentToolResult } from "@mariozechner/pi-agent-core"; import { type ExecHost, loadExecApprovals, maxAsk, minSecurity } from "../infra/exec-approvals.js"; import { resolveExecSafeBinRuntimePolicy } from "../infra/exec-safe-bin-runtime-policy.js"; +import { sanitizeHostExecEnvWithDiagnostics } from "../infra/host-env-security.js"; import { getShellPathFromLoginShell, resolveShellEnvFallbackTimeoutMs, @@ -25,9 +26,7 @@ import { renderExecHostLabel, resolveApprovalRunningNoticeMs, runExecProcess, - sanitizeHostBaseEnv, execSchema, - validateHostEnv, } from "./bash-tools.exec-runtime.js"; import type { ExecElevatedDefaults, @@ -362,24 +361,58 @@ export function createExecTool( } const inheritedBaseEnv = coerceEnv(process.env); - const baseEnv = host === "sandbox" ? inheritedBaseEnv : sanitizeHostBaseEnv(inheritedBaseEnv); - - // Logic: Sandbox gets raw env. Host (gateway/node) must pass validation. - // We validate BEFORE merging to prevent any dangerous vars from entering the stream. - if (host !== "sandbox" && params.env) { - validateHostEnv(params.env); + const hostEnvResult = + host === "sandbox" + ? null + : sanitizeHostExecEnvWithDiagnostics({ + baseEnv: inheritedBaseEnv, + overrides: params.env, + blockPathOverrides: true, + }); + if ( + hostEnvResult && + params.env && + (hostEnvResult.rejectedOverrideBlockedKeys.length > 0 || + hostEnvResult.rejectedOverrideInvalidKeys.length > 0) + ) { + const blockedKeys = hostEnvResult.rejectedOverrideBlockedKeys; + const invalidKeys = hostEnvResult.rejectedOverrideInvalidKeys; + const pathBlocked = blockedKeys.includes("PATH"); + if (pathBlocked && blockedKeys.length === 1 && invalidKeys.length === 0) { + throw new Error( + "Security Violation: Custom 'PATH' variable is forbidden during host execution.", + ); + } + if (blockedKeys.length === 1 && invalidKeys.length === 0) { + throw new Error( + `Security Violation: Environment variable '${blockedKeys[0]}' is forbidden during host execution.`, + ); + } + const details: string[] = []; + if (blockedKeys.length > 0) { + details.push(`blocked override keys: ${blockedKeys.join(", ")}`); + } + if (invalidKeys.length > 0) { + details.push(`invalid non-portable override keys: ${invalidKeys.join(", ")}`); + } + const suffix = details.join("; "); + if (pathBlocked) { + throw new Error( + `Security Violation: Custom 'PATH' variable is forbidden during host execution (${suffix}).`, + ); + } + throw new Error(`Security Violation: ${suffix}.`); } - const mergedEnv = params.env ? { ...baseEnv, ...params.env } : baseEnv; - - const env = sandbox - ? buildSandboxEnv({ - defaultPath: DEFAULT_PATH, - paramsEnv: params.env, - sandboxEnv: sandbox.env, - containerWorkdir: containerWorkdir ?? sandbox.containerWorkdir, - }) - : mergedEnv; + const env = + sandbox && host === "sandbox" + ? buildSandboxEnv({ + defaultPath: DEFAULT_PATH, + paramsEnv: params.env, + sandboxEnv: sandbox.env, + containerWorkdir: containerWorkdir ?? sandbox.containerWorkdir, + }) + : (hostEnvResult?.env ?? inheritedBaseEnv); if (!sandbox && host === "gateway" && !params.env?.PATH) { const shellPath = getShellPathFromLoginShell({ diff --git a/src/infra/host-env-security-policy.json b/src/infra/host-env-security-policy.json index 785b8e37049..2f6cd25bde6 100644 --- a/src/infra/host-env-security-policy.json +++ b/src/infra/host-env-security-policy.json @@ -56,7 +56,23 @@ "OPENSSL_ENGINES", "PYTHONSTARTUP", "WGETRC", - "CURL_HOME" + "CURL_HOME", + "CLASSPATH", + "CGO_CFLAGS", + "CGO_LDFLAGS", + "GOFLAGS", + "CORECLR_PROFILER_PATH", + "PHPRC", + "PHP_INI_SCAN_DIR", + "DENO_DIR", + "BUN_CONFIG_REGISTRY", + "LUA_PATH", + "LUA_CPATH", + "GEM_HOME", + "GEM_PATH", + "BUNDLE_GEMFILE", + "COMPOSER_HOME", + "XDG_CONFIG_HOME" ], "blockedOverridePrefixes": ["GIT_CONFIG_", "NPM_CONFIG_"], "blockedPrefixes": ["DYLD_", "LD_", "BASH_FUNC_"] diff --git a/src/infra/host-env-security.test.ts b/src/infra/host-env-security.test.ts index cd3edb3e06b..f326a0c75ed 100644 --- a/src/infra/host-env-security.test.ts +++ b/src/infra/host-env-security.test.ts @@ -8,6 +8,7 @@ import { isDangerousHostEnvVarName, normalizeEnvVarKey, sanitizeHostExecEnv, + sanitizeHostExecEnvWithDiagnostics, sanitizeSystemRunEnvOverrides, } from "./host-env-security.js"; import { OPENCLAW_CLI_ENV_VALUE } from "./openclaw-exec-env.js"; @@ -114,6 +115,10 @@ describe("sanitizeHostExecEnv", () => { GIT_CONFIG_GLOBAL: "/tmp/gitconfig", SHELLOPTS: "xtrace", PS4: "$(touch /tmp/pwned)", + CLASSPATH: "/tmp/evil-classpath", + GOFLAGS: "-mod=mod", + PHPRC: "/tmp/evil-php.ini", + XDG_CONFIG_HOME: "/tmp/evil-config", SAFE: "ok", }, }); @@ -128,6 +133,10 @@ describe("sanitizeHostExecEnv", () => { expect(env.GIT_CONFIG_GLOBAL).toBeUndefined(); expect(env.SHELLOPTS).toBeUndefined(); expect(env.PS4).toBeUndefined(); + expect(env.CLASSPATH).toBeUndefined(); + expect(env.GOFLAGS).toBeUndefined(); + expect(env.PHPRC).toBeUndefined(); + expect(env.XDG_CONFIG_HOME).toBeUndefined(); expect(env.SAFE).toBe("ok"); expect(env.HOME).toBe("/tmp/trusted-home"); expect(env.ZDOTDIR).toBe("/tmp/trusted-zdotdir"); @@ -183,7 +192,7 @@ describe("sanitizeHostExecEnv", () => { expect(env.OPENCLAW_CLI).toBe(OPENCLAW_CLI_ENV_VALUE); }); - it("drops non-string inherited values and non-portable inherited keys", () => { + it("drops non-string inherited values while preserving non-portable inherited keys", () => { const env = sanitizeHostExecEnv({ baseEnv: { PATH: "/usr/bin:/bin", @@ -191,6 +200,7 @@ describe("sanitizeHostExecEnv", () => { // oxlint-disable-next-line typescript/no-explicit-any BAD_NUMBER: 1 as any, "NOT-PORTABLE": "x", + "ProgramFiles(x86)": "C:\\Program Files (x86)", }, }); @@ -198,6 +208,8 @@ describe("sanitizeHostExecEnv", () => { OPENCLAW_CLI: OPENCLAW_CLI_ENV_VALUE, PATH: "/usr/bin:/bin", GOOD: "1", + "NOT-PORTABLE": "x", + "ProgramFiles(x86)": "C:\\Program Files (x86)", }); }); }); @@ -212,11 +224,58 @@ describe("isDangerousHostEnvOverrideVarName", () => { expect(isDangerousHostEnvOverrideVarName("git_config_global")).toBe(true); expect(isDangerousHostEnvOverrideVarName("GRADLE_USER_HOME")).toBe(true); expect(isDangerousHostEnvOverrideVarName("gradle_user_home")).toBe(true); + expect(isDangerousHostEnvOverrideVarName("CLASSPATH")).toBe(true); + expect(isDangerousHostEnvOverrideVarName("classpath")).toBe(true); + expect(isDangerousHostEnvOverrideVarName("GOFLAGS")).toBe(true); + expect(isDangerousHostEnvOverrideVarName("goflags")).toBe(true); + expect(isDangerousHostEnvOverrideVarName("CORECLR_PROFILER_PATH")).toBe(true); + expect(isDangerousHostEnvOverrideVarName("coreclr_profiler_path")).toBe(true); + expect(isDangerousHostEnvOverrideVarName("XDG_CONFIG_HOME")).toBe(true); + expect(isDangerousHostEnvOverrideVarName("xdg_config_home")).toBe(true); expect(isDangerousHostEnvOverrideVarName("BASH_ENV")).toBe(false); expect(isDangerousHostEnvOverrideVarName("FOO")).toBe(false); }); }); +describe("sanitizeHostExecEnvWithDiagnostics", () => { + it("reports blocked and invalid requested overrides", () => { + const result = sanitizeHostExecEnvWithDiagnostics({ + baseEnv: { + PATH: "/usr/bin:/bin", + }, + overrides: { + PATH: "/tmp/evil", + CLASSPATH: "/tmp/evil-classpath", + SAFE_KEY: "ok", + "BAD-KEY": "bad", + }, + }); + + expect(result.rejectedOverrideBlockedKeys).toEqual(["CLASSPATH", "PATH"]); + expect(result.rejectedOverrideInvalidKeys).toEqual(["BAD-KEY"]); + expect(result.env.SAFE_KEY).toBe("ok"); + expect(result.env.PATH).toBe("/usr/bin:/bin"); + expect(result.env.CLASSPATH).toBeUndefined(); + }); + + it("allows Windows-style override names while still rejecting invalid keys", () => { + const result = sanitizeHostExecEnvWithDiagnostics({ + baseEnv: { + PATH: "/usr/bin:/bin", + "ProgramFiles(x86)": "C:\\Program Files (x86)", + }, + overrides: { + "ProgramFiles(x86)": "D:\\SDKs", + "BAD-KEY": "bad", + }, + }); + + expect(result.rejectedOverrideBlockedKeys).toEqual([]); + expect(result.rejectedOverrideInvalidKeys).toEqual(["BAD-KEY"]); + expect(result.env["ProgramFiles(x86)"]).toBe("D:\\SDKs"); + }); +}); + describe("normalizeEnvVarKey", () => { it("normalizes and validates keys", () => { expect(normalizeEnvVarKey(" OPENROUTER_API_KEY ")).toBe("OPENROUTER_API_KEY"); diff --git a/src/infra/host-env-security.ts b/src/infra/host-env-security.ts index 11d6b8e9f3c..c6ac3dded61 100644 --- a/src/infra/host-env-security.ts +++ b/src/infra/host-env-security.ts @@ -2,6 +2,7 @@ import HOST_ENV_SECURITY_POLICY_JSON from "./host-env-security-policy.json" with import { markOpenClawExecEnv } from "./openclaw-exec-env.js"; const PORTABLE_ENV_VAR_KEY = /^[A-Za-z_][A-Za-z0-9_]*$/; +const WINDOWS_COMPAT_OVERRIDE_ENV_VAR_KEY = /^[A-Za-z_][A-Za-z0-9_()]*$/; type HostEnvSecurityPolicy = { blockedKeys: string[]; @@ -42,6 +43,17 @@ export const HOST_SHELL_WRAPPER_ALLOWED_OVERRIDE_ENV_KEYS = new Set( HOST_SHELL_WRAPPER_ALLOWED_OVERRIDE_ENV_KEY_VALUES, ); +export type HostExecEnvSanitizationResult = { + env: Record; + rejectedOverrideBlockedKeys: string[]; + rejectedOverrideInvalidKeys: string[]; +}; + +export type HostExecEnvOverrideDiagnostics = { + rejectedOverrideBlockedKeys: string[]; + rejectedOverrideInvalidKeys: string[]; +}; + export function normalizeEnvVarKey( rawKey: string, options?: { portable?: boolean }, @@ -56,6 +68,17 @@ export function normalizeEnvVarKey( return key; } +function normalizeHostOverrideEnvVarKey(rawKey: string): string | null { + const key = normalizeEnvVarKey(rawKey); + if (!key) { + return null; + } + if (PORTABLE_ENV_VAR_KEY.test(key) || WINDOWS_COMPAT_OVERRIDE_ENV_VAR_KEY.test(key)) { + return key; + } + return null; +} + export function isDangerousHostEnvVarName(rawKey: string): boolean { const key = normalizeEnvVarKey(rawKey); if (!key) { @@ -80,15 +103,16 @@ export function isDangerousHostEnvOverrideVarName(rawKey: string): boolean { return HOST_DANGEROUS_OVERRIDE_ENV_PREFIXES.some((prefix) => upper.startsWith(prefix)); } -function listNormalizedPortableEnvEntries( +function listNormalizedEnvEntries( source: Record, + options?: { portable?: boolean }, ): Array<[string, string]> { const entries: Array<[string, string]> = []; for (const [rawKey, value] of Object.entries(source)) { if (typeof value !== "string") { continue; } - const key = normalizeEnvVarKey(rawKey, { portable: true }); + const key = normalizeEnvVarKey(rawKey, options); if (!key) { continue; } @@ -97,41 +121,112 @@ function listNormalizedPortableEnvEntries( return entries; } -export function sanitizeHostExecEnv(params?: { +function sortUnique(values: Iterable): string[] { + return Array.from(new Set(values)).toSorted((a, b) => a.localeCompare(b)); +} + +function sanitizeHostEnvOverridesWithDiagnostics(params?: { + overrides?: Record | null; + blockPathOverrides?: boolean; +}): { + acceptedOverrides?: Record; + rejectedOverrideBlockedKeys: string[]; + rejectedOverrideInvalidKeys: string[]; +} { + const overrides = params?.overrides ?? undefined; + if (!overrides) { + return { + acceptedOverrides: undefined, + rejectedOverrideBlockedKeys: [], + rejectedOverrideInvalidKeys: [], + }; + } + + const blockPathOverrides = params?.blockPathOverrides ?? true; + const acceptedOverrides: Record = {}; + const rejectedBlocked: string[] = []; + const rejectedInvalid: string[] = []; + + for (const [rawKey, value] of Object.entries(overrides)) { + if (typeof value !== "string") { + continue; + } + const normalized = normalizeHostOverrideEnvVarKey(rawKey); + if (!normalized) { + const candidate = rawKey.trim(); + rejectedInvalid.push(candidate || rawKey); + continue; + } + const upper = normalized.toUpperCase(); + // PATH is part of the security boundary (command resolution + safe-bin checks). Never allow + // request-scoped PATH overrides from agents/gateways. + if (blockPathOverrides && upper === "PATH") { + rejectedBlocked.push(upper); + continue; + } + if (isDangerousHostEnvVarName(upper) || isDangerousHostEnvOverrideVarName(upper)) { + rejectedBlocked.push(upper); + continue; + } + acceptedOverrides[normalized] = value; + } + + return { + acceptedOverrides, + rejectedOverrideBlockedKeys: sortUnique(rejectedBlocked), + rejectedOverrideInvalidKeys: sortUnique(rejectedInvalid), + }; +} + +export function sanitizeHostExecEnvWithDiagnostics(params?: { baseEnv?: Record; overrides?: Record | null; blockPathOverrides?: boolean; -}): Record { +}): HostExecEnvSanitizationResult { const baseEnv = params?.baseEnv ?? process.env; - const overrides = params?.overrides ?? undefined; - const blockPathOverrides = params?.blockPathOverrides ?? true; const merged: Record = {}; - for (const [key, value] of listNormalizedPortableEnvEntries(baseEnv)) { + for (const [key, value] of listNormalizedEnvEntries(baseEnv)) { if (isDangerousHostEnvVarName(key)) { continue; } merged[key] = value; } - if (!overrides) { - return markOpenClawExecEnv(merged); + const overrideResult = sanitizeHostEnvOverridesWithDiagnostics({ + overrides: params?.overrides ?? undefined, + blockPathOverrides: params?.blockPathOverrides ?? true, + }); + if (overrideResult.acceptedOverrides) { + for (const [key, value] of Object.entries(overrideResult.acceptedOverrides)) { + merged[key] = value; + } } - for (const [key, value] of listNormalizedPortableEnvEntries(overrides)) { - const upper = key.toUpperCase(); - // PATH is part of the security boundary (command resolution + safe-bin checks). Never allow - // request-scoped PATH overrides from agents/gateways. - if (blockPathOverrides && upper === "PATH") { - continue; - } - if (isDangerousHostEnvVarName(upper) || isDangerousHostEnvOverrideVarName(upper)) { - continue; - } - merged[key] = value; - } + return { + env: markOpenClawExecEnv(merged), + rejectedOverrideBlockedKeys: overrideResult.rejectedOverrideBlockedKeys, + rejectedOverrideInvalidKeys: overrideResult.rejectedOverrideInvalidKeys, + }; +} - return markOpenClawExecEnv(merged); +export function inspectHostExecEnvOverrides(params?: { + overrides?: Record | null; + blockPathOverrides?: boolean; +}): HostExecEnvOverrideDiagnostics { + const result = sanitizeHostEnvOverridesWithDiagnostics(params); + return { + rejectedOverrideBlockedKeys: result.rejectedOverrideBlockedKeys, + rejectedOverrideInvalidKeys: result.rejectedOverrideInvalidKeys, + }; +} + +export function sanitizeHostExecEnv(params?: { + baseEnv?: Record; + overrides?: Record | null; + blockPathOverrides?: boolean; +}): Record { + return sanitizeHostExecEnvWithDiagnostics(params).env; } export function sanitizeSystemRunEnvOverrides(params?: { @@ -146,7 +241,7 @@ export function sanitizeSystemRunEnvOverrides(params?: { return overrides; } const filtered: Record = {}; - for (const [key, value] of listNormalizedPortableEnvEntries(overrides)) { + for (const [key, value] of listNormalizedEnvEntries(overrides, { portable: true })) { if (!HOST_SHELL_WRAPPER_ALLOWED_OVERRIDE_ENV_KEYS.has(key.toUpperCase())) { continue; } diff --git a/src/node-host/invoke-system-run.test.ts b/src/node-host/invoke-system-run.test.ts index 045897a5fc4..02457b98b4d 100644 --- a/src/node-host/invoke-system-run.test.ts +++ b/src/node-host/invoke-system-run.test.ts @@ -336,6 +336,7 @@ describe("handleSystemRunInvoke mac app exec host routing", () => { preferMacAppExecHost: boolean; runViaResponse?: ExecHostResponse | null; command?: string[]; + env?: Record; rawCommand?: string | null; systemRunPlan?: SystemRunApprovalPlan | null; cwd?: string; @@ -391,6 +392,7 @@ describe("handleSystemRunInvoke mac app exec host routing", () => { client: {} as never, params: { command: params.command ?? ["echo", "ok"], + env: params.env, rawCommand: params.rawCommand, systemRunPlan: params.systemRunPlan, cwd: params.cwd, @@ -1106,6 +1108,65 @@ describe("handleSystemRunInvoke mac app exec host routing", () => { expectApprovalRequiredDenied({ sendNodeEvent, sendInvokeResult }); }); + it("rejects blocked environment overrides before execution", async () => { + const { runCommand, sendInvokeResult } = await runSystemInvoke({ + preferMacAppExecHost: false, + security: "full", + ask: "off", + env: { CLASSPATH: "/tmp/evil-classpath" }, + }); + + expect(runCommand).not.toHaveBeenCalled(); + expectInvokeErrorMessage(sendInvokeResult, { + message: "SYSTEM_RUN_DENIED: environment override rejected", + }); + expectInvokeErrorMessage(sendInvokeResult, { + message: "CLASSPATH", + }); + }); + + it("rejects blocked environment overrides for shell-wrapper commands", async () => { + const shellCommand = + process.platform === "win32" + ? ["cmd.exe", "/d", "/s", "/c", "echo ok"] + : ["/bin/sh", "-lc", "echo ok"]; + const { runCommand, sendInvokeResult } = await runSystemInvoke({ + preferMacAppExecHost: false, + security: "full", + ask: "off", + command: shellCommand, + env: { + CLASSPATH: "/tmp/evil-classpath", + LANG: "C", + }, + }); + + expect(runCommand).not.toHaveBeenCalled(); + expectInvokeErrorMessage(sendInvokeResult, { + message: "SYSTEM_RUN_DENIED: environment override rejected", + }); + expectInvokeErrorMessage(sendInvokeResult, { + message: "CLASSPATH", + }); + }); + + it("rejects invalid non-portable environment override keys before execution", async () => { + const { runCommand, sendInvokeResult } = await runSystemInvoke({ + preferMacAppExecHost: false, + security: "full", + ask: "off", + env: { "BAD-KEY": "x" }, + }); + + expect(runCommand).not.toHaveBeenCalled(); + expectInvokeErrorMessage(sendInvokeResult, { + message: "SYSTEM_RUN_DENIED: environment override rejected", + }); + expectInvokeErrorMessage(sendInvokeResult, { + message: "BAD-KEY", + }); + }); + async function expectNestedEnvShellDenied(params: { depth: number; markerName: string; diff --git a/src/node-host/invoke-system-run.ts b/src/node-host/invoke-system-run.ts index c38094dc683..b530b980840 100644 --- a/src/node-host/invoke-system-run.ts +++ b/src/node-host/invoke-system-run.ts @@ -14,7 +14,10 @@ import { } from "../infra/exec-approvals.js"; import type { ExecHostRequest, ExecHostResponse, ExecHostRunResult } from "../infra/exec-host.js"; import { resolveExecSafeBinRuntimePolicy } from "../infra/exec-safe-bin-runtime-policy.js"; -import { sanitizeSystemRunEnvOverrides } from "../infra/host-env-security.js"; +import { + inspectHostExecEnvOverrides, + sanitizeSystemRunEnvOverrides, +} from "../infra/host-env-security.js"; import { normalizeSystemRunApprovalPlan } from "../infra/system-run-approval-binding.js"; import { resolveSystemRunCommandRequest } from "../infra/system-run-command.js"; import { logWarn } from "../logger.js"; @@ -244,6 +247,34 @@ async function parseSystemRunPhase( const sessionKey = opts.params.sessionKey?.trim() || "node"; const runId = opts.params.runId?.trim() || crypto.randomUUID(); const suppressNotifyOnExit = opts.params.suppressNotifyOnExit === true; + const envOverrideDiagnostics = inspectHostExecEnvOverrides({ + overrides: opts.params.env ?? undefined, + blockPathOverrides: true, + }); + if ( + envOverrideDiagnostics.rejectedOverrideBlockedKeys.length > 0 || + envOverrideDiagnostics.rejectedOverrideInvalidKeys.length > 0 + ) { + const details: string[] = []; + if (envOverrideDiagnostics.rejectedOverrideBlockedKeys.length > 0) { + details.push( + `blocked override keys: ${envOverrideDiagnostics.rejectedOverrideBlockedKeys.join(", ")}`, + ); + } + if (envOverrideDiagnostics.rejectedOverrideInvalidKeys.length > 0) { + details.push( + `invalid non-portable override keys: ${envOverrideDiagnostics.rejectedOverrideInvalidKeys.join(", ")}`, + ); + } + await opts.sendInvokeResult({ + ok: false, + error: { + code: "INVALID_REQUEST", + message: `SYSTEM_RUN_DENIED: environment override rejected (${details.join("; ")})`, + }, + }); + return null; + } const envOverrides = sanitizeSystemRunEnvOverrides({ overrides: opts.params.env ?? undefined, shellWrapper: shellPayload !== null, diff --git a/src/node-host/invoke.sanitize-env.test.ts b/src/node-host/invoke.sanitize-env.test.ts index aa55a24047e..c53d7b08953 100644 --- a/src/node-host/invoke.sanitize-env.test.ts +++ b/src/node-host/invoke.sanitize-env.test.ts @@ -51,6 +51,13 @@ describe("node-host sanitizeEnv", () => { expect(env.BASH_ENV).toBeUndefined(); }); }); + + it("preserves inherited non-portable Windows-style env keys", () => { + withEnv({ "ProgramFiles(x86)": "C:\\Program Files (x86)" }, () => { + const env = sanitizeEnv(undefined); + expect(env["ProgramFiles(x86)"]).toBe("C:\\Program Files (x86)"); + }); + }); }); describe("node-host output decoding", () => {