diff --git a/packages/@aws-cdk/private-tools/lib/subprocess/index.ts b/packages/@aws-cdk/private-tools/lib/subprocess/index.ts index fbf5bb070..60d533425 100644 --- a/packages/@aws-cdk/private-tools/lib/subprocess/index.ts +++ b/packages/@aws-cdk/private-tools/lib/subprocess/index.ts @@ -7,7 +7,9 @@ * the arguments, so shell injection is impossible by construction. * Windows `.cmd`/`.bat` shims (npm, yarn, …) are handled by cross-spawn, * which spawns `cmd.exe /d /s /c` with correct quoting — modern Node does - * not spawn batch shims directly (CVE-2024-27980). + * not spawn batch shims directly (CVE-2024-27980). The executable name is + * resolved against PATH (never the working directory) so a binary planted + * in the cwd cannot shadow the real one (see `resolveExecutable`). * * 2. `runUserCommandLine(line)` — an opaque command line **the user themselves * authored** (e.g. the `app` command from `cdk.json`, the `--browser` flag), @@ -18,6 +20,8 @@ */ // eslint-disable-next-line no-restricted-imports -- this module IS the sanctioned wrapper around child_process import * as child_process from 'child_process'; +import * as fs from 'fs'; +import * as path from 'path'; import { StringDecoder } from 'string_decoder'; import spawn from 'cross-spawn'; @@ -213,7 +217,11 @@ function errorMessage(cause: unknown): string { */ export async function run(argv: readonly string[], options: RunOptions = {}): Promise { assertNonEmptyArgv(argv, 'run'); - const child = spawn(argv[0], argv.slice(1), spawnOptions(options)); + const command = resolveExecutable(argv[0], { env: options.env }); + if (command === undefined) { + return Promise.reject(notFoundError(argv)); + } + const child = spawn(command, argv.slice(1), spawnOptions(options)); return monitor(child, renderForDisplay(argv), options); } @@ -241,7 +249,11 @@ export interface RunSyncOptions { */ export function runSync(argv: readonly string[], options: RunSyncOptions = {}): string { assertNonEmptyArgv(argv, 'runSync'); - const result = spawn.sync(argv[0], argv.slice(1), { + const command = resolveExecutable(argv[0], {}); + if (command === undefined) { + throw notFoundError(argv); + } + const result = spawn.sync(command, argv.slice(1), { cwd: options.cwd, timeout: options.timeoutMs, killSignal: 'SIGTERM', @@ -286,6 +298,111 @@ export async function runUserCommandLine(commandLine: string, options: RunOption return monitor(child, commandLine, options); } +/** + * Resolve an executable name to an absolute path against PATH — never the cwd. + * + * On Windows a bare program name spawned without a shell is searched for in the + * current working directory *before* PATH, so a file planted in the working + * directory (e.g. a `docker.bat` inside a handed-over cloud assembly) can run + * instead of the real binary. Resolving to an absolute PATH hit up front closes + * that: the cwd is never consulted, and a name that is not on PATH is refused + * (returns `undefined`) rather than silently satisfied from the cwd. + * + * POSIX `execvp` already searches PATH only (never the cwd), so there the name + * is returned unchanged. An argument that already contains a path separator is + * an explicit location and is honored verbatim on every platform. + * + * @returns the resolved command (absolute on Windows, unchanged elsewhere), or + * `undefined` when a bare Windows name cannot be found on PATH. + */ +export function resolveExecutable( + command: string, + options: { readonly env?: Record; readonly platform?: NodeJS.Platform } = {}, +): string | undefined { + const platform = options.platform ?? process.platform; + + // An explicit path (absolute, or containing a separator / drive) is used + // verbatim; there is no PATH search to harden. `\\` is checked directly + // because path.isAbsolute uses the *running* platform's rules. + if (path.isAbsolute(command) || command.includes('/') || command.includes('\\')) { + return command; + } + + // POSIX execvp searches PATH only; nothing to harden. + if (platform !== 'win32') { + return command; + } + + const env = options.env ?? process.env; + const dirs = (envValue(env, 'PATH') ?? '').split(path.delimiter).filter(Boolean); + const exts = windowsExtensions(command, envValue(env, 'PATHEXT')); + + for (const dir of dirs) { + for (const ext of exts) { + const candidate = path.join(dir, command + ext); + if (isFile(candidate)) { + return candidate; + } + } + } + // Not on PATH. Deliberately do NOT fall back to the bare name: that would let + // Windows resolve it from the cwd, which is exactly the risk we are closing. + return undefined; +} + +/** Look up an environment variable case-insensitively (the Windows env is). */ +function envValue(env: Record, name: string): string | undefined { + if (env[name] !== undefined) { + return env[name]; + } + const lower = name.toLowerCase(); + const key = Object.keys(env).find((k) => k.toLowerCase() === lower); + return key !== undefined ? env[key] : undefined; +} + +/** + * The extensions to append when searching for `command` on Windows. + * + * If the name already ends in a known executable extension, search for it + * exactly (empty suffix); otherwise try each PATHEXT entry. + */ +function windowsExtensions(command: string, pathext: string | undefined): string[] { + const configured = (pathext ?? '.COM;.EXE;.BAT;.CMD') + .split(';') + .map((e) => e.trim()) + .filter(Boolean); + const lower = command.toLowerCase(); + return configured.some((e) => lower.endsWith(e.toLowerCase())) ? [''] : configured; +} + +function isFile(candidate: string): boolean { + try { + return fs.statSync(candidate).isFile(); + } catch { + return false; + } +} + +/** + * A `SubprocessError` shaped like a real spawn ENOENT, for the case where a + * bare Windows name could not be resolved on PATH. Keeps `kind: 'spawn-failed'` + * and a `cause` carrying `code: 'ENOENT'` so downstream guidance (e.g. + * cdk-assets' "please install docker") still fires. + */ +function notFoundError(argv: readonly string[]): SubprocessError { + const cause = Object.assign(new Error(`spawn ${argv[0]} ENOENT`), { + code: 'ENOENT', errno: -2, syscall: 'spawn', path: argv[0], + }); + return new SubprocessError({ + command: renderForDisplay(argv), + exitCode: null, + signal: null, + stdout: '', + stderr: '', + cause, + }); +} + /** * Render an argv array as a single string for logs and error messages. * diff --git a/packages/@aws-cdk/private-tools/test/subprocess/subprocess.test.ts b/packages/@aws-cdk/private-tools/test/subprocess/subprocess.test.ts index 1aaf16df8..b314bd2cd 100644 --- a/packages/@aws-cdk/private-tools/test/subprocess/subprocess.test.ts +++ b/packages/@aws-cdk/private-tools/test/subprocess/subprocess.test.ts @@ -1,5 +1,8 @@ +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; import type { OutputStream } from '../../lib/subprocess'; -import { run, runSync, runUserCommandLine, renderForDisplay, SubprocessError } from '../../lib/subprocess'; +import { run, runSync, runUserCommandLine, renderForDisplay, resolveExecutable, SubprocessError } from '../../lib/subprocess'; // A cross-platform argv that echoes its arguments exactly as received, // proving no shell interpreted them. `node -e` exists everywhere the @@ -113,6 +116,28 @@ describe('run', () => { expect(result.stdout.trim()).toMatch(/^\d+\.\d+\.\d+/); }, 30000); + // Windows-only: a plain .exe (as in every nodeEval test above) never routes + // through cmd.exe, so this is the one path where cross-spawn's escaping is + // actually exercised. Must run on Windows CI to have any value. + (process.platform === 'win32' ? test : test.skip)( + 'a .cmd shim receives hostile arguments verbatim (cross-spawn escaping)', + async () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'cmd-shim')); + const shim = path.join(dir, 'echo-args.cmd'); + // The shim forwards its args to node, which echoes them back as JSON. + fs.writeFileSync(shim, '@node -e "process.stdout.write(JSON.stringify(process.argv.slice(1)))" %*\r\n'); + try { + // Every one of these would do something (or break) if cmd.exe parsed it. + const hostile = ['a&echo PWNED', 'b|whoami', 'c>out', 'd"q', '%PATH%', 'e^f', '(g)', 'two spaces']; + const result = await run([shim, ...hostile]); + expect(JSON.parse(result.stdout)).toEqual(hostile); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }, + 30000, + ); + test('multi-byte UTF-8 characters split across chunks decode correctly', async () => { // 'é' is 2 bytes in UTF-8; the child writes them in separate chunks with a // delay so they arrive as separate 'data' events. @@ -251,3 +276,60 @@ describe('renderForDisplay', () => { expect(renderForDisplay(['plain'])).toEqual('plain'); }); }); + +describe('resolveExecutable', () => { + let dir: string; + + beforeEach(() => { + dir = fs.mkdtempSync(path.join(os.tmpdir(), 'resolve-exe')); + }); + + afterEach(() => { + fs.rmSync(dir, { recursive: true, force: true }); + }); + + test('POSIX leaves a bare name unchanged (execvp already searches PATH only)', () => { + expect(resolveExecutable('docker', { platform: 'linux' })).toEqual('docker'); + }); + + test('an explicit path is honored verbatim on every platform', () => { + expect(resolveExecutable('/usr/bin/docker', { platform: 'win32' })).toEqual('/usr/bin/docker'); + expect(resolveExecutable('C:\\tools\\docker.exe', { platform: 'win32' })).toEqual('C:\\tools\\docker.exe'); + expect(resolveExecutable('./local-tool', { platform: 'linux' })).toEqual('./local-tool'); + }); + + test('Windows resolves a bare name to its absolute location on PATH', () => { + const target = path.join(dir, 'docker.CMD'); + fs.writeFileSync(target, ''); + + expect(resolveExecutable('docker', { platform: 'win32', env: { PATH: dir, PATHEXT: '.CMD' } })) + .toEqual(target); + }); + + test('Windows searches for an already-suffixed name exactly (no double extension)', () => { + // Casing kept consistent so the assertion is meaningful on a case-sensitive + // filesystem; on Windows the FS match is itself case-insensitive. + fs.writeFileSync(path.join(dir, 'tool.exe'), ''); + + expect(resolveExecutable('tool.exe', { platform: 'win32', env: { PATH: dir, PATHEXT: '.EXE' } })) + .toEqual(path.join(dir, 'tool.exe')); + }); + + test('Windows refuses a name that is not on PATH — never falls back to the cwd', () => { + // The binary exists on disk, but in a directory that is NOT on PATH. + // Resolution must fail rather than let Windows satisfy the bare name from + // the working directory (the shadowing risk this closes). + fs.writeFileSync(path.join(dir, 'docker.CMD'), ''); + + expect(resolveExecutable('docker', { platform: 'win32', env: { PATH: '', PATHEXT: '.CMD' } })) + .toBeUndefined(); + }); + + test('Windows PATH lookup is case-insensitive in the env var name (Path vs PATH)', () => { + const target = path.join(dir, 'git.EXE'); + fs.writeFileSync(target, ''); + + expect(resolveExecutable('git', { platform: 'win32', env: { Path: dir, PATHEXT: '.EXE' } })) + .toEqual(target); + }); +}); diff --git a/packages/@aws-cdk/toolkit-lib/lib/api/cloud-assembly/environment.ts b/packages/@aws-cdk/toolkit-lib/lib/api/cloud-assembly/environment.ts index 54e5010b2..b7641d15e 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/api/cloud-assembly/environment.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/api/cloud-assembly/environment.ts @@ -1,6 +1,7 @@ import * as path from 'path'; import * as cxapi from '@aws-cdk/cx-api'; import * as fs from 'fs-extra'; +import { ToolkitError } from '../../toolkit/toolkit-error'; import type { SdkProvider } from '../aws-auth/private'; import type { Settings } from '../settings'; @@ -260,6 +261,19 @@ function quoteShellPart(part: string) { return part; } if (isWindows) { + // cmd.exe expands `%VAR%` even inside double quotes, and a `cmd /c` command + // line — which is how `runUserCommandLine` reaches the shell on Windows — + // has no reliable way to escape a percent (doubling only works in batch + // files). A discovered path carrying a `%...%` reference would therefore be + // silently rewritten (an env var spliced into the path). Refuse it loudly + // rather than execute something other than what is on disk. + if (/%[^%]*%/.test(part)) { + throw new ToolkitError( + 'UnsafeWindowsPath', + `Cannot safely run a path containing a '%...%' substring through the Windows shell: '${part}'. ` + + 'Rename the file or directory to remove the percent signs.', + ); + } return `"${part}"`; } return `"${part.replace(/([\\"$`])/g, '\\$1')}"`; diff --git a/packages/@aws-cdk/toolkit-lib/test/api/cloud-assembly/environment.test.ts b/packages/@aws-cdk/toolkit-lib/test/api/cloud-assembly/environment.test.ts index f007c3725..f82f19fe5 100644 --- a/packages/@aws-cdk/toolkit-lib/test/api/cloud-assembly/environment.test.ts +++ b/packages/@aws-cdk/toolkit-lib/test/api/cloud-assembly/environment.test.ts @@ -57,6 +57,22 @@ test.each([ expect(actual).toEqual(expected); }); +test('refuses a discovered Windows path containing a %VAR% reference (cmd.exe would expand it)', async () => { + // GIVEN + const appPath = 'C:\\proj\\%USERNAME%\\app'; + Object.defineProperty(process, 'platform', { value: 'win32' }); + jest.spyOn(fs, 'stat').mockImplementation((p) => { + if (p !== appPath) { + throw new Error(`Expected a stat() call on '${appPath}' but got '${p}'`); + } + return Promise.resolve({ mode: 0 }) as any; + }); + + // THEN + await expect(guessExecutable(appPath, (_) => Promise.resolve())) + .rejects.toThrow(/Cannot safely run a path containing a '%\.\.\.%' substring/); +}); + /** * Explode all 'both's in a test array to both false and true */