Skip to content

security(spawn): strip dynamic-linker env vars from subprocess env#800

Merged
BYK merged 1 commit intomasterfrom
security/subprocess-env-sanitization
Apr 21, 2026
Merged

security(spawn): strip dynamic-linker env vars from subprocess env#800
BYK merged 1 commit intomasterfrom
security/subprocess-env-sanitization

Conversation

@BYK
Copy link
Copy Markdown
Member

@BYK BYK commented Apr 21, 2026

Summary

Extends the dynamic-linker env-var sanitisation landed in #794 so it also applies to every subprocess Craft spawns — not just to Craft's own process.env at startup. Defence-in-depth against two residual attack surfaces:

  1. Post-startup mutation of process.env — hostile or accidental code that sets LD_PRELOAD back into process.env after Craft's startup sanitiser has run. The subprocess would still inherit it.
  2. Caller-constructed options.env — any future refactor that does { ...process.env, ...custom } (or takes env from an external source) could smuggle a dynamic-linker key into a child without going through process.env.

Neither is exploitable today — #794 closed the primary .craft.env vector and Craft doesn't currently mutate process.env or take env from external files — but the pattern is cheap to make robust before it becomes a regression footgun.

Change

src/utils/system.ts:spawnProcess now routes options.env ?? process.env through a new sanitizeSpawnEnv() helper before handing it to child_process.spawn. The helper:

  • Strips LD_PRELOAD, LD_LIBRARY_PATH, LD_AUDIT, and the DYLD_* family (same set as the startup sanitiser).
  • Honours the same CRAFT_ALLOW_DYNAMIC_LINKER_ENV=1 opt-out as sanitizeDynamicLinkerEnv.
  • Never logs values (only key names).
  • Returns a fresh shallow copy — input is never mutated, so callers can reuse their env object.

Where the code lives

A new leaf module src/utils/dynamicLinkerEnv.ts holds the constants and both sanitisers. src/utils/env.ts re-exports the same names so existing imports (notably src/index.ts) keep working.

Why the move: src/utils/system.ts needs the sanitiser, but src/utils/env.ts transitively imports src/config.ts → artifact providers → which import back into src/utils/system.ts. A direct system.ts → env.ts edge creates a cycle that breaks test loading (observed it locally — Class extends value undefined when BaseArtifactProvider wasn't ready yet). Leaf module with only logger as a dep sidesteps this cleanly.

Tests

  • 7 new unit tests in src/utils/__tests__/env.test.ts on sanitizeSpawnEnv (undefined input passthrough, shallow copy behaviour, input-not-mutated invariant, stripping all DYLD_* variants, opt-out behaviour, value-not-in-log property, strict opt-out equality check).
  • 3 new integration tests in src/utils/__tests__/system.test.ts that actually spawn node as a subprocess and read back process.env.LD_PRELOAD from inside the child:
    • explicit LD_PRELOAD in options.env → child sees undefined;
    • LD_PRELOAD set on process.env post-startup → child sees undefined;
    • same with CRAFT_ALLOW_DYNAMIC_LINKER_ENV=1 → child sees the value (opt-out works).

Full suite: 969 passed (same 7 pre-existing e2e failures unrelated to this work). pnpm build + pnpm lint src/ clean.

Notes for reviewers

  • The opt-out key is still CRAFT_ALLOW_DYNAMIC_LINKER_ENV=1 and must be set in process.env, not options.env. If a caller passes LD_PRELOAD in options.env and the opt-out is not set on process.env, it gets stripped — even if the caller "intended" it. This is consistent with the startup sanitiser.
  • buildReleaseCommandEnv() (PR fix(security): disable .craft.env reading and harden release subprocess env #794) already produces an env that cannot contain dynamic-linker keys because its allowlist doesn't include them. This PR is a backstop behind that.

Craft 2.25.5 sanitised process.env at startup to prevent LD_PRELOAD
injection (PR #794). That closes the primary vector — .craft.env on
disk can no longer hydrate process.env — but it does not help if some
later code path re-adds LD_PRELOAD to process.env (hostile or
accidental mutation) or if a caller explicitly constructs an
`options.env` object that contains one of these keys (e.g. via a
future `{ ...process.env, ...custom }` spread). In either case the
child process would still inherit the injection.

Defence-in-depth: sanitise the env that reaches child_process.spawn
itself. Every call to `spawnProcess` in src/utils/system.ts now
routes `options.env ?? process.env` through sanitizeSpawnEnv()
before handing it to spawn(), stripping LD_PRELOAD, LD_LIBRARY_PATH,
LD_AUDIT, and the DYLD_* family. The same CRAFT_ALLOW_DYNAMIC_LINKER_ENV=1
escape hatch honoured at startup also applies here; both paths log
a one-time info message when the opt-out is in effect.

Implementation:
- Moved the dynamic-linker key list + opt-out name + both sanitisers
  into a new leaf module src/utils/dynamicLinkerEnv.ts. src/utils/env.ts
  re-exports the same names for backward compat. The move avoids a
  circular import: src/utils/system.ts needs the sanitiser, but
  src/utils/env.ts transitively pulls in src/config.ts and the
  artifact providers, which import back into src/utils/system.ts.

Tests:
- 7 new unit tests on sanitizeSpawnEnv (no-op for undefined, shallow
  copy, no mutation, stripping all DYLD_* variants, opt-out behaviour,
  values never logged).
- 3 new integration tests on spawnProcess that actually spawn a
  subprocess and verify LD_PRELOAD does not reach it, including the
  "set on process.env after startup" regression scenario.
@BYK BYK merged commit 2f55139 into master Apr 21, 2026
16 checks passed
@BYK BYK deleted the security/subprocess-env-sanitization branch April 21, 2026 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant