From e65c265e8972e48293e47183c4e3a9372aa98eb9 Mon Sep 17 00:00:00 2001 From: Agustin Rivera <31522568+eleqtrizit@users.noreply.github.com> Date: Mon, 30 Mar 2026 11:35:04 -0700 Subject: [PATCH] Security: block exec approval shell carrier targets (#57871) * Security: block exec approval shell carrier targets * Tests: tighten exec approval carrier regression assertions --- src/infra/exec-approvals-allow-always.test.ts | 98 +++++++++---------- src/infra/exec-approvals-allowlist.ts | 94 +----------------- 2 files changed, 46 insertions(+), 146 deletions(-) diff --git a/src/infra/exec-approvals-allow-always.test.ts b/src/infra/exec-approvals-allow-always.test.ts index 849cd491c4b..0e12cc5917f 100644 --- a/src/infra/exec-approvals-allow-always.test.ts +++ b/src/infra/exec-approvals-allow-always.test.ts @@ -120,38 +120,31 @@ describe("resolveAllowAlwaysPatterns", () => { expect(second.allowlistSatisfied).toBe(true); } - function expectPositionalArgvCarrierResult(params: { - command: string; - expectPersisted: boolean; - }) { + function expectPositionalArgvCarrierRejected(command: string) { const dir = makeTempDir(); const touch = makeExecutable(dir, "touch"); const env = { PATH: `${dir}${path.delimiter}${process.env.PATH ?? ""}` }; const safeBins = resolveSafeBins(undefined); const marker = path.join(dir, "marker"); - const command = params.command.replaceAll("{marker}", marker); + const expandedCommand = command.replaceAll("{marker}", marker); const { persisted } = resolvePersistedPatterns({ - command, + command: expandedCommand, dir, env, safeBins, }); - if (params.expectPersisted) { - expect(persisted).toEqual([touch]); - } else { - expect(persisted).not.toContain(touch); - } + expect(persisted).toEqual([]); const second = evaluateShellAllowlist({ - command, + command: expandedCommand, allowlist: [{ pattern: touch }], safeBins, cwd: dir, env, platform: process.platform, }); - expect(second.allowlistSatisfied).toBe(params.expectPersisted); + expect(second.allowlistSatisfied).toBe(false); } it("returns direct executable paths for non-shell segments", () => { @@ -290,63 +283,34 @@ describe("resolveAllowAlwaysPatterns", () => { }); }); - it("persists carried executables for shell-wrapper positional argv carriers", () => { + it("rejects shell-wrapper positional argv carriers", () => { if (process.platform === "win32") { return; } - const dir = makeTempDir(); - const touch = makeExecutable(dir, "touch"); - const env = { PATH: `${dir}${path.delimiter}${process.env.PATH ?? ""}` }; - const safeBins = resolveSafeBins(undefined); - - const { persisted } = resolvePersistedPatterns({ - command: `sh -lc '$0 "$1"' touch ${path.join(dir, "marker")}`, - dir, - env, - safeBins, - }); - expect(persisted).toEqual([touch]); - - const second = evaluateShellAllowlist({ - command: `sh -lc '$0 "$1"' touch ${path.join(dir, "second-marker")}`, - allowlist: [{ pattern: touch }], - safeBins, - cwd: dir, - env, - platform: process.platform, - }); - expect(second.allowlistSatisfied).toBe(true); + expectPositionalArgvCarrierRejected(`sh -lc '$0 "$1"' touch {marker}`); }); - it("persists carried executables for exec -- positional argv carriers", () => { + it("rejects exec positional argv carriers", () => { if (process.platform === "win32") { return; } - expectPositionalArgvCarrierResult({ - command: `sh -lc 'exec -- "$0" "$1"' touch {marker}`, - expectPersisted: true, - }); + expectPositionalArgvCarrierRejected(`sh -lc 'exec -- "$0" "$1"' touch {marker}`); }); it("rejects positional argv carriers when $0 is single-quoted", () => { if (process.platform === "win32") { return; } - expectPositionalArgvCarrierResult({ - command: `sh -lc "'$0' "$1"" touch {marker}`, - expectPersisted: false, - }); + expectPositionalArgvCarrierRejected(`sh -lc "'$0' "$1"" touch {marker}`); }); it("rejects positional argv carriers when exec is separated from $0 by a newline", () => { if (process.platform === "win32") { return; } - expectPositionalArgvCarrierResult({ - command: `sh -lc "exec + expectPositionalArgvCarrierRejected(`sh -lc "exec $0 \\"$1\\"" touch {marker}`, - expectPersisted: false, - }); + ); }); it("rejects positional argv carriers when inline command contains extra shell operations", () => { @@ -742,7 +706,7 @@ $0 \\"$1\\"" touch {marker}`, return; } const dir = makeTempDir(); - makeExecutable(dir, "env"); + const envPath = makeExecutable(dir, "env"); const env = makePathEnv(dir); const safeBins = resolveSafeBins(undefined); @@ -756,7 +720,7 @@ $0 \\"$1\\"" touch {marker}`, const second = evaluateShellAllowlist({ command: `sh -lc '$0 "$@"' env BASH_ENV=/tmp/payload.sh bash -lc 'id > /tmp/pwned'`, - allowlist: persisted.map((pattern) => ({ pattern })), + allowlist: [{ pattern: envPath }], safeBins, cwd: dir, env, @@ -770,7 +734,7 @@ $0 \\"$1\\"" touch {marker}`, return; } const dir = makeTempDir(); - makeExecutable(dir, "bash"); + const bashPath = makeExecutable(dir, "bash"); const env = makePathEnv(dir); const safeBins = resolveSafeBins(undefined); @@ -784,7 +748,35 @@ $0 \\"$1\\"" touch {marker}`, const second = evaluateShellAllowlist({ command: `sh -lc '$0 "$@"' bash -lc 'id > /tmp/pwned'`, - allowlist: persisted.map((pattern) => ({ pattern })), + allowlist: [{ pattern: bashPath }], + safeBins, + cwd: dir, + env, + platform: process.platform, + }); + expect(second.allowlistSatisfied).toBe(false); + }); + + it("rejects positional carrier when carried executable is an unknown dispatch carrier", () => { + if (process.platform === "win32") { + return; + } + const dir = makeTempDir(); + const xargsPath = makeExecutable(dir, "xargs"); + const env = makePathEnv(dir); + const safeBins = resolveSafeBins(undefined); + + const { persisted } = resolvePersistedPatterns({ + command: `sh -lc '$0 "$@"' xargs echo SAFE`, + dir, + env, + safeBins, + }); + expect(persisted).toEqual([]); + + const second = evaluateShellAllowlist({ + command: `sh -lc '$0 "$@"' xargs sh -lc 'id > /tmp/pwned'`, + allowlist: [{ pattern: xargsPath }], safeBins, cwd: dir, env, diff --git a/src/infra/exec-approvals-allowlist.ts b/src/infra/exec-approvals-allowlist.ts index c37578344e9..ccc3b259aba 100644 --- a/src/infra/exec-approvals-allowlist.ts +++ b/src/infra/exec-approvals-allowlist.ts @@ -1,5 +1,4 @@ import path from "node:path"; -import { isDispatchWrapperExecutable } from "./dispatch-wrapper-resolution.js"; import { analyzeShellCommand, isWindowsPlatform, @@ -26,11 +25,9 @@ import { isTrustedSafeBinPath } from "./exec-safe-bin-trust.js"; import { extractShellWrapperInlineCommand, isShellWrapperExecutable, - normalizeExecutableToken, } from "./exec-wrapper-resolution.js"; import { resolveExecWrapperTrustPlan } from "./exec-wrapper-trust-plan.js"; import { expandHomePrefix } from "./home-dir.js"; -import { POSIX_INLINE_COMMAND_FLAGS, resolveInlineCommandMatch } from "./shell-inline-command.js"; function hasShellLineContinuation(command: string): boolean { return /\\(?:\r\n|\n|\r)/.test(command); @@ -235,18 +232,6 @@ function evaluateSegments( : executableResolution; const executableMatch = matchAllowlist(params.allowlist, candidateResolution); const inlineCommand = extractShellWrapperInlineCommand(allowlistSegment.argv); - const shellPositionalArgvCandidatePath = resolveShellWrapperPositionalArgvCandidatePath({ - segment: allowlistSegment, - cwd: params.cwd, - env: params.env, - }); - const shellPositionalArgvMatch = shellPositionalArgvCandidatePath - ? matchAllowlist(params.allowlist, { - rawExecutable: shellPositionalArgvCandidatePath, - resolvedPath: shellPositionalArgvCandidatePath, - executableName: path.basename(shellPositionalArgvCandidatePath), - }) - : null; const shellScriptCandidatePath = inlineCommand === null ? resolveShellWrapperScriptCandidatePath({ @@ -261,7 +246,7 @@ function evaluateSegments( executableName: path.basename(shellScriptCandidatePath), }) : null; - const match = executableMatch ?? shellPositionalArgvMatch ?? shellScriptMatch; + const match = executableMatch ?? shellScriptMatch; if (match) { matches.push(match); } @@ -428,71 +413,6 @@ function resolveShellWrapperScriptCandidatePath(params: { return path.resolve(base, expanded); } -function resolveShellWrapperPositionalArgvCandidatePath(params: { - segment: ExecCommandSegment; - cwd?: string; - env?: NodeJS.ProcessEnv; -}): string | undefined { - if (!isShellWrapperSegment(params.segment)) { - return undefined; - } - - const argv = params.segment.argv; - if (!Array.isArray(argv) || argv.length < 4) { - return undefined; - } - - const wrapper = normalizeExecutableToken(argv[0] ?? ""); - if (!["ash", "bash", "dash", "fish", "ksh", "sh", "zsh"].includes(wrapper)) { - return undefined; - } - - const inlineMatch = resolveInlineCommandMatch(argv, POSIX_INLINE_COMMAND_FLAGS, { - allowCombinedC: true, - }); - if (inlineMatch.valueTokenIndex === null || !inlineMatch.command) { - return undefined; - } - if (!isDirectShellPositionalCarrierInvocation(inlineMatch.command)) { - return undefined; - } - - const carriedExecutable = argv - .slice(inlineMatch.valueTokenIndex + 1) - .map((token) => token.trim()) - .find((token) => token.length > 0); - if (!carriedExecutable) { - return undefined; - } - - // Reject wrapper targets carried through `$0 "$@"` because their trailing argv can - // widen execution semantics beyond the original approved command. - const carriedName = normalizeExecutableToken(carriedExecutable); - if (isDispatchWrapperExecutable(carriedName) || isShellWrapperExecutable(carriedName)) { - return undefined; - } - - const resolution = resolveCommandResolutionFromArgv([carriedExecutable], params.cwd, params.env); - return resolveExecutionTargetCandidatePath(resolution, params.cwd); -} - -function isDirectShellPositionalCarrierInvocation(command: string): boolean { - const trimmed = command.trim(); - if (trimmed.length === 0) { - return false; - } - - // Keep carrier matching strict: only allow direct `$0` execution with positional arguments. - // This prevents payloads like `echo blocked; $0 "$1"` from satisfying allowlist checks. - const shellWhitespace = String.raw`[^\S\r\n]+`; - const positionalZero = String.raw`(?:\$(?:0|\{0\})|"\$(?:0|\{0\})")`; - const positionalArg = String.raw`(?:\$(?:[@*]|[1-9]|\{[@*1-9]\})|"\$(?:[@*]|[1-9]|\{[@*1-9]\})")`; - return new RegExp( - `^(?:exec${shellWhitespace}(?:--${shellWhitespace})?)?${positionalZero}(?:${shellWhitespace}${positionalArg})*$`, - "u", - ).test(trimmed); -} - function collectAllowAlwaysPatterns(params: { segment: ExecCommandSegment; cwd?: string; @@ -529,18 +449,6 @@ function collectAllowAlwaysPatterns(params: { params.out.add(candidatePath); return; } - const positionalArgvPath = resolveShellWrapperPositionalArgvCandidatePath({ - segment, - cwd: params.cwd, - env: params.env, - }); - if (positionalArgvPath) { - if (isInterpreterLikeAllowlistPattern(positionalArgvPath)) { - return; - } - params.out.add(positionalArgvPath); - return; - } const inlineCommand = trustPlan.shellInlineCommand ?? extractShellWrapperInlineCommand(segment.argv); if (!inlineCommand) {