From 8f8492d172f4c5b4fd7dd9a47855ed620c8770ab Mon Sep 17 00:00:00 2001 From: Pavan Kumar Gondhi Date: Mon, 13 Apr 2026 11:48:42 +0530 Subject: [PATCH] fix(security): broaden shell-wrapper detection and block env-argv assignment injection [AI-assisted] (#65717) * fix: address issue * fix: address PR review feedback * fix: address PR review feedback * docs: add changelog entry for PR merge --- CHANGELOG.md | 1 + src/infra/dispatch-wrapper-resolution.ts | 107 ++++++++++++++++++---- src/infra/exec-wrapper-resolution.test.ts | 48 ++++++++++ src/infra/host-env-security.test.ts | 4 + src/infra/host-env-security.ts | 18 +++- src/infra/shell-wrapper-resolution.ts | 32 +++++++ src/node-host/exec-policy.test.ts | 10 ++ src/node-host/invoke-system-run.test.ts | 46 +++++++++- src/node-host/invoke-system-run.ts | 34 ++++++- 9 files changed, 277 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0bce56bb3e4..38e1a6a2ac8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ Docs: https://docs.openclaw.ai ### Fixes +- fix(security): broaden shell-wrapper detection and block env-argv assignment injection [AI-assisted]. (#65717) Thanks @pgondhi987. - Gateway/startup: defer scheduled services until sidecars finish, gate chat history and model listing during sidecar resume, and let Control UI retry startup-gated history loads so Sandbox wake resumes channels first. (#65365) Thanks @lml2468. - Control UI/chat: load the live gateway slash-command catalog into the composer and command palette so dock commands, plugin commands, and direct skill aliases appear in chat, while keeping trusted local commands authoritative and bounding remote command metadata. (#65620) Thanks @BunsDev. diff --git a/src/infra/dispatch-wrapper-resolution.ts b/src/infra/dispatch-wrapper-resolution.ts index 5a946492767..2b0db746f7b 100644 --- a/src/infra/dispatch-wrapper-resolution.ts +++ b/src/infra/dispatch-wrapper-resolution.ts @@ -152,28 +152,97 @@ function scanWrapperInvocation( } export function unwrapEnvInvocation(argv: string[]): string[] | null { - return scanWrapperInvocation(argv, { - separators: new Set(["--", "-"]), - onToken: (token, lower) => { - if (isEnvAssignment(token)) { - return "continue"; + const parsed = parseEnvInvocationPrelude(argv); + return parsed ? argv.slice(parsed.commandIndex) : null; +} + +type ParsedEnvInvocationPrelude = { + assignmentKeys: string[]; + commandIndex: number; +}; + +function parseEnvInvocationPrelude(argv: string[]): ParsedEnvInvocationPrelude | null { + let idx = 1; + let expectsOptionValue = false; + const assignmentKeys: string[] = []; + while (idx < argv.length) { + const token = argv[idx]?.trim() ?? ""; + if (!token) { + idx += 1; + continue; + } + if (expectsOptionValue) { + expectsOptionValue = false; + idx += 1; + continue; + } + if (token === "--" || token === "-") { + idx += 1; + break; + } + if (isEnvAssignment(token)) { + const delimiter = token.indexOf("="); + if (delimiter > 0) { + assignmentKeys.push(token.slice(0, delimiter)); } - if (!token.startsWith("-") || token === "-") { - return "stop"; + idx += 1; + continue; + } + if (!token.startsWith("-") || token === "-") { + break; + } + const lower = normalizeLowercaseStringOrEmpty(token); + const [flag] = lower.split("=", 2); + if (ENV_FLAG_OPTIONS.has(flag)) { + idx += 1; + continue; + } + if (ENV_OPTIONS_WITH_VALUE.has(flag)) { + if (lower.includes("=")) { + idx += 1; + continue; } - const [flag] = lower.split("=", 2); - if (ENV_FLAG_OPTIONS.has(flag)) { - return "continue"; + expectsOptionValue = true; + idx += 1; + continue; + } + if (hasEnvInlineValuePrefix(lower)) { + idx += 1; + continue; + } + return null; + } + + if (expectsOptionValue || idx >= argv.length) { + return null; + } + + return { + assignmentKeys, + commandIndex: idx, + }; +} + +export function extractEnvAssignmentKeysFromDispatchWrappers( + argv: string[], + maxDepth = MAX_DISPATCH_WRAPPER_DEPTH, +): string[] { + let current = argv; + const assignmentKeys: string[] = []; + for (let depth = 0; depth < maxDepth; depth += 1) { + const unwrap = unwrapKnownDispatchWrapperInvocation(current); + if (unwrap.kind !== "unwrapped" || unwrap.argv.length === 0) { + break; + } + if (unwrap.wrapper === "env") { + const parsed = parseEnvInvocationPrelude(current); + if (parsed) { + assignmentKeys.push(...parsed.assignmentKeys); } - if (ENV_OPTIONS_WITH_VALUE.has(flag)) { - return lower.includes("=") ? "continue" : "consume-next"; - } - if (hasEnvInlineValuePrefix(lower)) { - return "continue"; - } - return "invalid"; - }, - }); + } + current = unwrap.argv; + } + return Array.from(new Set(assignmentKeys)).toSorted((a, b) => a.localeCompare(b)); } function envInvocationUsesModifiers(argv: string[]): boolean { diff --git a/src/infra/exec-wrapper-resolution.test.ts b/src/infra/exec-wrapper-resolution.test.ts index e3159c3a593..dc7bfed74de 100644 --- a/src/infra/exec-wrapper-resolution.test.ts +++ b/src/infra/exec-wrapper-resolution.test.ts @@ -1,11 +1,13 @@ import { describe, expect, test } from "vitest"; import { basenameLower, + extractEnvAssignmentKeysFromDispatchWrappers, extractShellWrapperCommand, extractShellWrapperInlineCommand, hasEnvManipulationBeforeShellWrapper, isDispatchWrapperExecutable, isShellWrapperExecutable, + isShellWrapperInvocation, normalizeExecutableToken, resolveDispatchWrapperTrustPlan, resolveShellWrapperTransportArgv, @@ -402,6 +404,52 @@ describe("resolveShellWrapperTransportArgv", () => { }); }); +describe("isShellWrapperInvocation", () => { + test.each([ + { + argv: ["bash", "script.sh"], + expected: true, + }, + { + argv: ["/usr/bin/env", "SHELLOPTS=xtrace", "bash", "-lc", "echo hi"], + expected: true, + }, + { + argv: ["busybox", "sh", "script.sh"], + expected: true, + }, + { + argv: ["/usr/bin/env", "FOO=bar", "/usr/bin/printf", "ok"], + expected: false, + }, + ])("detects shell-wrapper executable invocations for %j", ({ argv, expected }) => { + expect(isShellWrapperInvocation(argv)).toBe(expected); + }); +}); + +describe("extractEnvAssignmentKeysFromDispatchWrappers", () => { + test.each([ + { + argv: ["env", "FOO=bar", "BAR=baz", "bash", "-lc", "echo hi"], + expected: ["BAR", "FOO"], + }, + { + argv: ["nice", "-n", "5", "env", "-u", "PATH", "TERM=xterm", "bash", "-lc", "echo hi"], + expected: ["TERM"], + }, + { + argv: ["env", "--split-string", "FOO=bar", "bash", "-lc", "echo hi"], + expected: [], + }, + { + argv: ["env", "--", "bash", "-lc", "echo hi"], + expected: [], + }, + ])("extracts env assignment prelude keys for %j", ({ argv, expected }) => { + expect(extractEnvAssignmentKeysFromDispatchWrappers(argv)).toEqual(expected); + }); +}); + describe("extractShellWrapperCommand", () => { test.each([ { diff --git a/src/infra/host-env-security.test.ts b/src/infra/host-env-security.test.ts index 8c163705660..5387157fddd 100644 --- a/src/infra/host-env-security.test.ts +++ b/src/infra/host-env-security.test.ts @@ -1201,11 +1201,13 @@ describe("sanitizeSystemRunEnvOverrides", () => { TOKEN: "abc", LANG: "C", LC_ALL: "C", + LC_TIME: "C", }, }); expect(overrides).toEqual({ LANG: "C", LC_ALL: "C", + LC_TIME: "C", }); }); @@ -1228,11 +1230,13 @@ describe("sanitizeSystemRunEnvOverrides", () => { overrides: { lang: "C", ColorTerm: "truecolor", + lc_numeric: "C", }, }), ).toEqual({ lang: "C", ColorTerm: "truecolor", + lc_numeric: "C", }); }); }); diff --git a/src/infra/host-env-security.ts b/src/infra/host-env-security.ts index 3b93f3e5637..31d43024121 100644 --- a/src/infra/host-env-security.ts +++ b/src/infra/host-env-security.ts @@ -32,6 +32,8 @@ export const HOST_SHELL_WRAPPER_ALLOWED_OVERRIDE_ENV_KEY_VALUES: readonly string "NO_COLOR", "FORCE_COLOR", ]); +export const HOST_SHELL_WRAPPER_ALLOWED_OVERRIDE_ENV_PREFIX_VALUES: readonly string[] = + Object.freeze(["LC_"]); export const HOST_DANGEROUS_ENV_KEYS = new Set(HOST_DANGEROUS_ENV_KEY_VALUES); export const HOST_DANGEROUS_INHERITED_ENV_KEYS = new Set( HOST_DANGEROUS_INHERITED_ENV_KEY_VALUES, @@ -43,6 +45,20 @@ export const HOST_SHELL_WRAPPER_ALLOWED_OVERRIDE_ENV_KEYS = new Set( HOST_SHELL_WRAPPER_ALLOWED_OVERRIDE_ENV_KEY_VALUES, ); +function isShellWrapperAllowedOverrideEnvVarName(rawKey: string): boolean { + const key = normalizeEnvVarKey(rawKey, { portable: true }); + if (!key) { + return false; + } + const upper = key.toUpperCase(); + if (HOST_SHELL_WRAPPER_ALLOWED_OVERRIDE_ENV_KEYS.has(upper)) { + return true; + } + return HOST_SHELL_WRAPPER_ALLOWED_OVERRIDE_ENV_PREFIX_VALUES.some((prefix) => + upper.startsWith(prefix), + ); +} + export type HostExecEnvSanitizationResult = { env: Record; rejectedOverrideBlockedKeys: string[]; @@ -254,7 +270,7 @@ export function sanitizeSystemRunEnvOverrides(params?: { } const filtered: Record = {}; for (const [key, value] of listNormalizedEnvEntries(overrides, { portable: true })) { - if (!HOST_SHELL_WRAPPER_ALLOWED_OVERRIDE_ENV_KEYS.has(key.toUpperCase())) { + if (!isShellWrapperAllowedOverrideEnvVarName(key)) { continue; } filtered[key] = value; diff --git a/src/infra/shell-wrapper-resolution.ts b/src/infra/shell-wrapper-resolution.ts index 4ff6342015c..59e433a6ebb 100644 --- a/src/infra/shell-wrapper-resolution.ts +++ b/src/infra/shell-wrapper-resolution.ts @@ -107,6 +107,38 @@ export function isShellWrapperExecutable(token: string): boolean { return SHELL_WRAPPER_CANONICAL.has(normalizeExecutableToken(token)); } +function isShellWrapperInvocationInternal(argv: string[], depth: number): boolean { + if (!isWithinDispatchClassificationDepth(depth)) { + return false; + } + const token0 = argv[0]?.trim(); + if (!token0) { + return false; + } + + const dispatchUnwrap = unwrapKnownDispatchWrapperInvocation(argv); + if (dispatchUnwrap.kind === "blocked") { + return false; + } + if (dispatchUnwrap.kind === "unwrapped") { + return isShellWrapperInvocationInternal(dispatchUnwrap.argv, depth + 1); + } + + const shellMultiplexerUnwrap = unwrapKnownShellMultiplexerInvocation(argv); + if (shellMultiplexerUnwrap.kind === "blocked") { + return false; + } + if (shellMultiplexerUnwrap.kind === "unwrapped") { + return isShellWrapperInvocationInternal(shellMultiplexerUnwrap.argv, depth + 1); + } + + return isShellWrapperExecutable(token0); +} + +export function isShellWrapperInvocation(argv: string[]): boolean { + return isShellWrapperInvocationInternal(argv, 0); +} + function normalizeRawCommand(rawCommand?: string | null): string | null { const trimmed = rawCommand?.trim() ?? ""; return trimmed.length > 0 ? trimmed : null; diff --git a/src/node-host/exec-policy.test.ts b/src/node-host/exec-policy.test.ts index 6dab750fe1b..ccce761db9b 100644 --- a/src/node-host/exec-policy.test.ts +++ b/src/node-host/exec-policy.test.ts @@ -147,6 +147,16 @@ describe("evaluateSystemRunPolicy", () => { expect(denied.errorMessage).toContain("Windows shell wrappers like cmd.exe /c"); }); + it("does not block Windows cmd.exe invocations without inline shell-wrapper transport", () => { + const allowed = expectAllowedDecision( + evaluateSystemRunPolicy( + buildPolicyParams({ isWindows: true, cmdInvocation: true, shellWrapperInvocation: false }), + ), + ); + expect(allowed.shellWrapperBlocked).toBe(false); + expect(allowed.windowsShellWrapperBlocked).toBe(false); + }); + it("allows execution when policy checks pass", () => { const allowed = expectAllowedDecision( evaluateSystemRunPolicy(buildPolicyParams({ ask: "on-miss" })), diff --git a/src/node-host/invoke-system-run.test.ts b/src/node-host/invoke-system-run.test.ts index 3b8a8d9dba1..880dd26c1d8 100644 --- a/src/node-host/invoke-system-run.test.ts +++ b/src/node-host/invoke-system-run.test.ts @@ -384,6 +384,7 @@ describe("handleSystemRunInvoke mac app exec host routing", () => { sendNodeEvent?: HandleSystemRunInvokeOptions["sendNodeEvent"]; skillBinsCurrent?: () => Promise>; isCmdExeInvocation?: HandleSystemRunInvokeOptions["isCmdExeInvocation"]; + sanitizeEnv?: HandleSystemRunInvokeOptions["sanitizeEnv"]; }): Promise<{ runCommand: MockedRunCommand; runViaMacAppExecHost: MockedRunViaMacAppExecHost; @@ -443,7 +444,7 @@ describe("handleSystemRunInvoke mac app exec host routing", () => { resolveExecSecurity: () => params.security ?? "full", resolveExecAsk: () => params.ask ?? "off", isCmdExeInvocation: params.isCmdExeInvocation ?? (() => false), - sanitizeEnv: () => undefined, + sanitizeEnv: params.sanitizeEnv ?? (() => undefined), runCommand, runViaMacAppExecHost, sendNodeEvent, @@ -1186,6 +1187,49 @@ describe("handleSystemRunInvoke mac app exec host routing", () => { }); }); + it("applies shell-wrapper env allowlist for shell executable commands without inline payload", async () => { + const { runCommand, sendInvokeResult } = await runSystemInvoke({ + preferMacAppExecHost: false, + security: "full", + ask: "off", + command: ["/bin/sh", "./script.sh"], + env: { + OPENCLAW_TEST: "1", + LANG: "C", + LC_TIME: "C", + }, + sanitizeEnv: (overrides) => overrides ?? undefined, + }); + + expect(runCommand).toHaveBeenCalledTimes(1); + const passedEnv = runCommand.mock.calls[0]?.[2]; + expect(passedEnv).toEqual({ + LANG: "C", + LC_TIME: "C", + }); + expectInvokeOk(sendInvokeResult); + }); + + it("rejects blocked env assignment keys embedded in command argv", async () => { + const { runCommand, sendInvokeResult } = await runSystemInvoke({ + preferMacAppExecHost: false, + security: "full", + ask: "off", + command: ["/usr/bin/env", "SHELLOPTS=xtrace", "PS4=$(id)", "bash", "-lc", "echo ok"], + }); + + expect(runCommand).not.toHaveBeenCalled(); + expectInvokeErrorMessage(sendInvokeResult, { + message: "SYSTEM_RUN_DENIED: command env assignment rejected", + }); + expectInvokeErrorMessage(sendInvokeResult, { + message: "SHELLOPTS", + }); + expectInvokeErrorMessage(sendInvokeResult, { + message: "PS4", + }); + }); + it("rejects invalid non-portable environment override keys before execution", async () => { const { runCommand, sendInvokeResult } = await runSystemInvoke({ preferMacAppExecHost: false, diff --git a/src/node-host/invoke-system-run.ts b/src/node-host/invoke-system-run.ts index abd51fe450b..5147b5d92df 100644 --- a/src/node-host/invoke-system-run.ts +++ b/src/node-host/invoke-system-run.ts @@ -20,7 +20,11 @@ import { detectInterpreterInlineEvalArgv, } from "../infra/exec-inline-eval.js"; import { resolveExecSafeBinRuntimePolicy } from "../infra/exec-safe-bin-runtime-policy.js"; -import { resolveShellWrapperTransportArgv } from "../infra/exec-wrapper-resolution.js"; +import { + extractEnvAssignmentKeysFromDispatchWrappers, + isShellWrapperInvocation, + resolveShellWrapperTransportArgv, +} from "../infra/exec-wrapper-resolution.js"; import { inspectHostExecEnvOverrides, sanitizeSystemRunEnvOverrides, @@ -78,6 +82,7 @@ type ResolvedExecApprovals = ReturnType; type SystemRunParsePhase = { argv: string[]; shellPayload: string | null; + shellWrapperInvocation: boolean; commandText: string; commandPreview: string | null; approvalPlan: import("../infra/exec-approvals.js").SystemRunApprovalPlan | null; @@ -242,6 +247,7 @@ async function parseSystemRunPhase( } const shellPayload = command.shellPayload; + const shellWrapperInvocation = isShellWrapperInvocation(command.argv); const commandText = command.commandText; const approvalPlan = opts.params.systemRunPlan === undefined @@ -258,6 +264,27 @@ async function parseSystemRunPhase( const sessionKey = normalizeOptionalString(opts.params.sessionKey) ?? "node"; const runId = normalizeOptionalString(opts.params.runId) ?? crypto.randomUUID(); const suppressNotifyOnExit = opts.params.suppressNotifyOnExit === true; + const envAssignmentKeys = extractEnvAssignmentKeysFromDispatchWrappers(command.argv); + const envAssignmentOverrides = + envAssignmentKeys.length > 0 + ? Object.fromEntries(envAssignmentKeys.map((key) => [key, "1"])) + : undefined; + const envAssignmentDiagnostics = inspectHostExecEnvOverrides({ + overrides: envAssignmentOverrides, + blockPathOverrides: true, + }); + // `extractEnvAssignmentKeysFromDispatchWrappers` only emits keys that satisfy + // `isEnvAssignment` and therefore portable env-key syntax by construction. + if (envAssignmentDiagnostics.rejectedOverrideBlockedKeys.length > 0) { + await opts.sendInvokeResult({ + ok: false, + error: { + code: "INVALID_REQUEST", + message: `SYSTEM_RUN_DENIED: command env assignment rejected (blocked env assignment keys: ${envAssignmentDiagnostics.rejectedOverrideBlockedKeys.join(", ")})`, + }, + }); + return null; + } const envOverrideDiagnostics = inspectHostExecEnvOverrides({ overrides: opts.params.env ?? undefined, blockPathOverrides: true, @@ -288,11 +315,12 @@ async function parseSystemRunPhase( } const envOverrides = sanitizeSystemRunEnvOverrides({ overrides: opts.params.env ?? undefined, - shellWrapper: shellPayload !== null, + shellWrapper: shellWrapperInvocation, }); return { argv: command.argv, shellPayload, + shellWrapperInvocation, commandText, commandPreview: command.previewText, approvalPlan, @@ -384,6 +412,8 @@ async function evaluateSystemRunPolicyPhase( approved: parsed.approved, isWindows, cmdInvocation, + // Keep cmd.exe approval gating scoped to inline shell-wrapper transport. + // Env sanitization uses broader shell-wrapper detection in parse phase. shellWrapperInvocation: parsed.shellPayload !== null, }); analysisOk = policy.analysisOk;