diff --git a/src/utils/__tests__/env.test.ts b/src/utils/__tests__/env.test.ts index 2a3530b6..fbd79d80 100644 --- a/src/utils/__tests__/env.test.ts +++ b/src/utils/__tests__/env.test.ts @@ -9,6 +9,7 @@ import * as config from '../../config'; import { checkEnvForPrerequisite, sanitizeDynamicLinkerEnv, + sanitizeSpawnEnv, warnIfCraftEnvFileExists, } from '../env'; import { ConfigurationError } from '../errors'; @@ -336,4 +337,88 @@ describe('env utils functions', () => { expect(logger.warn).toHaveBeenCalledTimes(1); }); }); // end describe('sanitizeDynamicLinkerEnv') + + describe('sanitizeSpawnEnv', () => { + beforeEach(() => { + delete process.env.CRAFT_ALLOW_DYNAMIC_LINKER_ENV; + }); + + test('returns undefined for undefined input', () => { + expect(sanitizeSpawnEnv(undefined)).toBeUndefined(); + }); + + test('returns a shallow copy without dynamic-linker keys', () => { + const input = { + PATH: '/usr/bin', + HOME: '/home/runner', + LD_PRELOAD: '/tmp/evil.so', + LD_LIBRARY_PATH: '/opt/lib', + GITHUB_TOKEN: 'gho_xxx', + }; + + const result = sanitizeSpawnEnv(input); + + expect(result).toEqual({ + PATH: '/usr/bin', + HOME: '/home/runner', + GITHUB_TOKEN: 'gho_xxx', + }); + // Input must not be mutated — callers may reuse it. + expect(input.LD_PRELOAD).toBe('/tmp/evil.so'); + expect(input.LD_LIBRARY_PATH).toBe('/opt/lib'); + }); + + test('strips all DYLD_* variants on macOS layouts', () => { + const input = { + PATH: '/usr/bin', + DYLD_INSERT_LIBRARIES: '/tmp/a.dylib', + DYLD_LIBRARY_PATH: '/opt/a', + DYLD_FRAMEWORK_PATH: '/opt/b', + DYLD_FALLBACK_LIBRARY_PATH: '/opt/c', + DYLD_FALLBACK_FRAMEWORK_PATH: '/opt/d', + }; + + const result = sanitizeSpawnEnv(input); + + expect(result).toEqual({ PATH: '/usr/bin' }); + }); + + test('warns once when any dynamic-linker key is stripped', () => { + sanitizeSpawnEnv({ PATH: '/u', LD_PRELOAD: '/x', LD_LIBRARY_PATH: '/y' }); + + expect(logger.warn).toHaveBeenCalledTimes(1); + const msg = (logger.warn as any).mock.calls[0].join(' '); + expect(msg).toContain('subprocess'); + // Value must never appear in the log. + expect(msg).not.toContain('/x'); + expect(msg).not.toContain('/y'); + }); + + test('does not warn when no dynamic-linker keys are present', () => { + sanitizeSpawnEnv({ PATH: '/u', HOME: '/h' }); + expect(logger.warn).not.toHaveBeenCalled(); + }); + + test('honours CRAFT_ALLOW_DYNAMIC_LINKER_ENV=1 opt-out', () => { + process.env.CRAFT_ALLOW_DYNAMIC_LINKER_ENV = '1'; + const input = { PATH: '/u', LD_PRELOAD: '/tmp/legit.so' }; + + const result = sanitizeSpawnEnv(input); + + expect(result).toEqual(input); + expect(result).toBe(input); // same reference — explicit no-op + expect(logger.warn).not.toHaveBeenCalled(); + }); + + test('opt-out only applies when value is exactly "1"', () => { + process.env.CRAFT_ALLOW_DYNAMIC_LINKER_ENV = 'yes'; + const result = sanitizeSpawnEnv({ + PATH: '/u', + LD_PRELOAD: '/tmp/x.so', + }); + + expect(result).toEqual({ PATH: '/u' }); + expect(logger.warn).toHaveBeenCalledTimes(1); + }); + }); // end describe('sanitizeSpawnEnv') }); // end describe('env utils functions') diff --git a/src/utils/__tests__/system.test.ts b/src/utils/__tests__/system.test.ts index 0d1e8f42..d792e84f 100644 --- a/src/utils/__tests__/system.test.ts +++ b/src/utils/__tests__/system.test.ts @@ -96,6 +96,70 @@ describe('spawnProcess', () => { expect(mockedLogInfo).toHaveBeenCalledTimes(1); expect(mockedLogInfo.mock.calls[0][0]).toMatch(/test-string/); }); + + describe('env sanitisation (defence-in-depth)', () => { + const savedEnv = { ...process.env }; + + afterEach(() => { + process.env = { ...savedEnv }; + }); + + test('strips LD_PRELOAD from an explicit options.env', async () => { + const stdout = + (await spawnProcess( + process.execPath, + [ + '-e', + 'process.stdout.write(JSON.stringify({ ld: process.env.LD_PRELOAD, marker: process.env.CHILD_MARKER }))', + ], + { + env: { + PATH: process.env.PATH, + LD_PRELOAD: '/tmp/evil.so', + CHILD_MARKER: 'reached', + }, + }, + )) || ''; + + const parsed = JSON.parse(stdout.toString()); + expect(parsed.ld).toBeUndefined(); + // Other env vars still propagate. + expect(parsed.marker).toBe('reached'); + }); + + test('strips LD_PRELOAD set on process.env after startup', async () => { + // Simulate a post-startup mutation of process.env (hostile or + // accidental). startup-level sanitizeDynamicLinkerEnv() cannot + // catch this; the spawn-level sanitiser must. + process.env.LD_PRELOAD = '/tmp/later-evil.so'; + + const stdout = + (await spawnProcess(process.execPath, [ + '-e', + 'process.stdout.write(process.env.LD_PRELOAD || "undef")', + ])) || ''; + + expect(stdout.toString()).toBe('undef'); + }); + + test('honours CRAFT_ALLOW_DYNAMIC_LINKER_ENV=1 opt-out', async () => { + process.env.CRAFT_ALLOW_DYNAMIC_LINKER_ENV = '1'; + + const stdout = + (await spawnProcess( + process.execPath, + ['-e', 'process.stdout.write(process.env.LD_PRELOAD || "undef")'], + { + env: { + PATH: process.env.PATH, + LD_PRELOAD: '/tmp/allowed.so', + }, + }, + )) || ''; + + expect(stdout.toString()).toBe('/tmp/allowed.so'); + }); + }); }); describe('replaceEnvVariable', () => { diff --git a/src/utils/dynamicLinkerEnv.ts b/src/utils/dynamicLinkerEnv.ts new file mode 100644 index 00000000..168d07ec --- /dev/null +++ b/src/utils/dynamicLinkerEnv.ts @@ -0,0 +1,123 @@ +/** + * Helpers for stripping dynamic-linker environment variables + * (`LD_PRELOAD`, `LD_LIBRARY_PATH`, `DYLD_*`, etc.) from Craft's own + * environment and from environments passed to subprocess spawns. + * + * These variables are a well-known supply-chain attack vector: an + * attacker who can influence the environment of a process can load an + * arbitrary shared library into every subprocess it spawns, gaining + * code execution with access to every credential those subprocesses + * touch (GitHub tokens, npm tokens, GPG keys, ...). + * + * This module has intentionally minimal imports (just the logger) so + * it can be safely imported from low-level utilities like + * `src/utils/system.ts` without creating a circular dependency with + * `src/config.ts` / the artifact providers. + */ + +import { logger } from '../logger'; + +/** + * Environment variable names Craft refuses to propagate. + * + * Any legitimate use (e.g. an instrumented build toolchain that relies + * on `LD_LIBRARY_PATH`) can be re-enabled for a single Craft + * invocation via {@link ALLOW_DYNAMIC_LINKER_ENV_VAR}. + */ +export const DYNAMIC_LINKER_ENV_VARS = [ + // Linux / glibc / musl + 'LD_PRELOAD', + 'LD_LIBRARY_PATH', + 'LD_AUDIT', + // macOS dyld + 'DYLD_INSERT_LIBRARIES', + 'DYLD_LIBRARY_PATH', + 'DYLD_FRAMEWORK_PATH', + 'DYLD_FALLBACK_LIBRARY_PATH', + 'DYLD_FALLBACK_FRAMEWORK_PATH', +] as const; + +/** + * Opt-out environment variable. When set to exactly `"1"` in + * `process.env`, {@link sanitizeDynamicLinkerEnv} and + * {@link sanitizeSpawnEnv} become no-ops. + * + * Noisy by design: both helpers log whenever the opt-out is in effect, + * so the escape hatch is visible in CI logs. + */ +export const ALLOW_DYNAMIC_LINKER_ENV_VAR = 'CRAFT_ALLOW_DYNAMIC_LINKER_ENV'; + +/** + * Strips dynamic-linker environment variables from `process.env` at + * startup, logging a warning per stripped key. Values are never logged. + */ +export function sanitizeDynamicLinkerEnv(): void { + const allowOverride = process.env[ALLOW_DYNAMIC_LINKER_ENV_VAR] === '1'; + const presentKeys = DYNAMIC_LINKER_ENV_VARS.filter( + key => process.env[key] !== undefined, + ); + + if (presentKeys.length === 0) { + return; + } + + if (allowOverride) { + logger.info( + `${ALLOW_DYNAMIC_LINKER_ENV_VAR}=1 set; preserving dynamic-linker environment variables: ${presentKeys.join( + ', ', + )}. This is not recommended.`, + ); + return; + } + + for (const key of presentKeys) { + logger.warn( + `Stripping dynamic-linker environment variable "${key}" for security reasons. ` + + `Set ${ALLOW_DYNAMIC_LINKER_ENV_VAR}=1 to override (not recommended).`, + ); + delete process.env[key]; + } +} + +/** + * Returns a copy of `env` with dynamic-linker environment variables + * removed. If the global opt-out `CRAFT_ALLOW_DYNAMIC_LINKER_ENV=1` is + * set on `process.env`, the input is returned unchanged. + * + * Defence-in-depth for subprocess spawns: even if an earlier code path + * somehow restores one of these variables to `process.env` after + * startup, or a caller explicitly constructs an `env` object that + * contains one (e.g. via `{ ...process.env, ...custom }`), the spawned + * child still won't inherit it. Values are never logged. + * + * @param env Environment variables bag to sanitise. The input is + * never mutated. + * @returns A shallow copy with dynamic-linker keys removed; or the + * original reference when input is `undefined` or when the opt-out + * is in effect. + */ +export function sanitizeSpawnEnv( + env: NodeJS.ProcessEnv | undefined, +): NodeJS.ProcessEnv | undefined { + if (env === undefined) { + return env; + } + if (process.env[ALLOW_DYNAMIC_LINKER_ENV_VAR] === '1') { + return env; + } + const sanitized: NodeJS.ProcessEnv = { ...env }; + let strippedAny = false; + for (const key of DYNAMIC_LINKER_ENV_VARS) { + if (sanitized[key] !== undefined) { + delete sanitized[key]; + strippedAny = true; + } + } + if (strippedAny) { + logger.warn( + `Stripped dynamic-linker environment variable(s) from a subprocess env. ` + + `Set ${ALLOW_DYNAMIC_LINKER_ENV_VAR}=1 to override (not recommended).`, + ); + } + return sanitized; +} diff --git a/src/utils/env.ts b/src/utils/env.ts index 2c87740e..f913bc41 100644 --- a/src/utils/env.ts +++ b/src/utils/env.ts @@ -72,70 +72,17 @@ function envHasVar(envVar: RequiredConfigVar): boolean { return true; } -/** - * Dynamic-linker environment variables that Craft refuses to propagate. - * - * Setting these allows arbitrary code to be loaded into every subprocess - * spawned by Craft (`LD_PRELOAD` on Linux, `DYLD_*` on macOS). They are a - * well-known supply-chain attack vector: an attacker who can influence - * Craft's environment (e.g. via a dotfile, a previous build step, or a - * misconfigured CI secret) can silently execute code with access to every - * release credential Craft touches. We strip these at startup as - * defence-in-depth — legitimate uses are extremely rare and can be - * re-enabled per-invocation via `CRAFT_ALLOW_DYNAMIC_LINKER_ENV=1`. - */ -const DYNAMIC_LINKER_ENV_VARS = [ - // Linux / glibc / musl - 'LD_PRELOAD', - 'LD_LIBRARY_PATH', - 'LD_AUDIT', - // macOS dyld - 'DYLD_INSERT_LIBRARIES', - 'DYLD_LIBRARY_PATH', - 'DYLD_FRAMEWORK_PATH', - 'DYLD_FALLBACK_LIBRARY_PATH', - 'DYLD_FALLBACK_FRAMEWORK_PATH', -] as const; - -/** Opt-out env var for {@link sanitizeDynamicLinkerEnv}. */ -const ALLOW_DYNAMIC_LINKER_ENV_VAR = 'CRAFT_ALLOW_DYNAMIC_LINKER_ENV'; - -/** - * Strips dynamic-linker environment variables (`LD_PRELOAD`, `LD_LIBRARY_PATH`, - * `DYLD_*`, etc.) from `process.env` at startup, logging a warning for each - * stripped key. Values are never logged. - * - * Users who legitimately require these variables (e.g. for an instrumented - * build toolchain) can set `CRAFT_ALLOW_DYNAMIC_LINKER_ENV=1` to opt out; - * this is noisy by design to make the escape hatch visible in CI logs. - */ -export function sanitizeDynamicLinkerEnv(): void { - const allowOverride = process.env[ALLOW_DYNAMIC_LINKER_ENV_VAR] === '1'; - const presentKeys = DYNAMIC_LINKER_ENV_VARS.filter( - key => process.env[key] !== undefined, - ); - - if (presentKeys.length === 0) { - return; - } - - if (allowOverride) { - logger.info( - `${ALLOW_DYNAMIC_LINKER_ENV_VAR}=1 set; preserving dynamic-linker environment variables: ${presentKeys.join( - ', ', - )}. This is not recommended.`, - ); - return; - } - - for (const key of presentKeys) { - logger.warn( - `Stripping dynamic-linker environment variable "${key}" for security reasons. ` + - `Set ${ALLOW_DYNAMIC_LINKER_ENV_VAR}=1 to override (not recommended).`, - ); - delete process.env[key]; - } -} +// Re-exported from `./dynamicLinkerEnv` so that existing callers +// (notably `src/index.ts`) keep working without edits. The logic now +// lives in a leaf module with minimal imports to avoid circular deps +// with `src/utils/system.ts` (see the file header in +// `./dynamicLinkerEnv`). +export { + ALLOW_DYNAMIC_LINKER_ENV_VAR, + DYNAMIC_LINKER_ENV_VARS, + sanitizeDynamicLinkerEnv, + sanitizeSpawnEnv, +} from './dynamicLinkerEnv'; /** * Warns the user if a legacy `.craft.env` file is present in the home diff --git a/src/utils/system.ts b/src/utils/system.ts index c55e9c60..a8ce4b80 100644 --- a/src/utils/system.ts +++ b/src/utils/system.ts @@ -11,6 +11,7 @@ import { logger } from '../logger'; import { reportError } from './errors'; import { isDryRun } from './helpers'; import { isInWorktreeMode } from './dryRun'; +import { sanitizeSpawnEnv } from './dynamicLinkerEnv'; /** * Types of supported hashing algorithms @@ -167,6 +168,15 @@ export async function spawnProcess( replaceEnvVariable(arg, { ...process.env, ...options.env }), ); + // Defence-in-depth: strip dynamic-linker env vars (LD_PRELOAD, + // DYLD_*, etc.) from whatever env the child process will see. + // Startup already sanitises process.env, so the only way one of + // these keys reaches this point is via a later mutation (hostile + // or accidental) or via an explicit `options.env` that spreads + // in attacker-controlled values. Either way, we refuse to + // propagate them. See sanitizeSpawnEnv() in ../utils/env.ts. + options.env = sanitizeSpawnEnv(options.env ?? process.env); + // Allow child to accept input (use 'pipe' for stdin if we need to write to it) options.stdio = [ spawnProcessOptions.stdin !== undefined ? 'pipe' : 'inherit',