How command injection happens in Node.js child_process calls and how to fix it

How Command Injection Happens in Node.js child_process Calls and How to Fix It

Introduction

The npm/holidaytw/lib/installer.js file handles post-install binary verification — a perfectly reasonable task for an npm package that ships a native binary. Its verifyBinaryExecutes function (line 58) accepts a binPath argument and runs that binary with --version to confirm it works. Simple enough. But the original implementation handed that path directly to spawnSync with no normalization, no explicit shell isolation, and no path validation — creating a code pattern that, in the wrong context, becomes a command injection primitive.

Semgrep's javascript.lang.security.detect-child-process.detect-child-process rule flagged this immediately. Let's break down exactly what was wrong, why it matters even in a "local CLI tool," and what the fix actually does.

The Vulnerability Explained

What the Original Code Did

Here's the vulnerable function as it existed before the patch:

// BEFORE — vulnerable
const { spawnSync } = require('child_process');

function verifyBinaryExecutes(binPath, opts = {}) {
  const timeoutMs = opts.timeoutMs ?? config.VERIFY_TIMEOUT_MS;
  const result = spawnSync(binPath, ['--version'], {
    timeout: timeoutMs,
    windowsHide: true,
  });
  if (result.error) {
    throw new VerificationError(`Failed to execute ${binPath} --version: ${result.error.message}`);
  }
  if (typeof result.status !== 'number' || result.status !== 0) {
    const stderrText = result.stderr ? result.stderr.toString('utf8').trim() : '';
    throw new VerificationError(
      `Verification failed: ${binPath} --version exited with status ${result.status}${
        stderrText ? ` (${stderrText})` : ''
      }`
    );
  }
}

The specific problematic pattern is on this line:

const result = spawnSync(binPath, ['--version'], { ... });

binPath flows in from the function's caller — ultimately sourced from command-line arguments, a config file, or the npm install environment — and is passed directly to spawnSync with no transformation.

Why This Is a Problem

While spawnSync does not invoke a shell by default (unlike exec), the path value itself is still dangerous for two reasons:

1. Path traversal sequences like ../../malicious-binary are not normalized. A caller providing a crafted path could redirect execution to an entirely different binary.

2. The pattern is an exploit primitive. Even if today's callers are trusted, this code pattern — unsanitized external input flowing into a child process execution — is exactly what automated exploit-chaining tools look for. If another vulnerability elsewhere in the codebase (e.g., a config file injection or an argument parsing flaw) allows an attacker to influence binPath, this function becomes the execution stage of the chain.

Concrete Attack Scenario

Imagine an attacker who can write to the npm package's configuration file (perhaps via a compromised dependency or a path traversal in a different function). They set binPath to:

../../../../../../tmp/evil-binary

Without path.resolve(), spawnSync will dutifully execute whatever lives at that relative path, resolved from the current working directory at runtime — which in an npm postinstall context is often the package root, a predictable location. The --version argument is passed harmlessly alongside, but the binary being executed is now entirely attacker-controlled.

On Windows, the attack surface is broader still: certain path formats can trigger shell interpretation even through spawn-family calls depending on the runtime version and OS configuration.

The Fix

What Changed

The patch makes four targeted changes, each closing a specific gap:

1. Switch from spawnSync to execFileSync

// BEFORE
const { spawnSync } = require('child_process');

// AFTER
const { execFileSync } = require('child_process');

execFileSync is semantically clearer about intent: execute this specific file, not a shell command. It also throws on non-zero exit (which simplifies error handling) rather than returning a result object that callers must manually inspect.

2. Normalize the path with path.resolve()

// BEFORE
spawnSync(binPath, ['--version'], { ... })

// AFTER
execFileSync(path.resolve(binPath), ['--version'], { ... })

path.resolve() converts the input to an absolute path, collapsing .. sequences and eliminating relative path traversal. An attacker providing ../../tmp/evil now gets /absolute/package/root/tmp/evil — still potentially wrong, but no longer a traversal gadget. Combined with the binary allowlist in resolveTarget, this closes the traversal vector.

3. Explicitly set shell: false

execFileSync(path.resolve(binPath), ['--version'], {
  timeout: timeoutMs,
  windowsHide: true,
  shell: false,          // ← explicit, not just default
  stdio: ['ignore', 'pipe', 'pipe'],
});

While execFileSync doesn't invoke a shell by default, making shell: false explicit serves as documentation and a guard against accidental future refactoring that might change the call signature.

4. Explicit stdio configuration

stdio: ['ignore', 'pipe', 'pipe'],

stdin is ignored (the child can never block waiting for input), and both stdout and stderr are piped (captured rather than leaked into npm's install log). This is a defense-in-depth measure — it prevents information leakage from the child process's output into the install environment.

Full Before/After Comparison

// ─── BEFORE ───────────────────────────────────────────────────────────────
const { spawnSync } = require('child_process');

function verifyBinaryExecutes(binPath, opts = {}) {
  const timeoutMs = opts.timeoutMs ?? config.VERIFY_TIMEOUT_MS;
  const result = spawnSync(binPath, ['--version'], {
    timeout: timeoutMs,
    windowsHide: true,
  });
  if (result.error) {
    throw new VerificationError(`Failed to execute ${binPath} --version: ${result.error.message}`);
  }
  if (typeof result.status !== 'number' || result.status !== 0) {
    const stderrText = result.stderr ? result.stderr.toString('utf8').trim() : '';
    throw new VerificationError(
      `Verification failed: ${binPath} --version exited with status ${result.status}${
        stderrText ? ` (${stderrText})` : ''
      }`
    );
  }
}

// ─── AFTER ────────────────────────────────────────────────────────────────
const { execFileSync } = require('child_process');

function verifyBinaryExecutes(binPath, opts = {}) {
  const timeoutMs = opts.timeoutMs ?? config.VERIFY_TIMEOUT_MS;
  let stdout;
  try {
    stdout = execFileSync(path.resolve(binPath), ['--version'], {
      timeout: timeoutMs,
      windowsHide: true,
      shell: false,
      stdio: ['ignore', 'pipe', 'pipe'],
    });
  } catch (err) {
    if (typeof err.status === 'number') {
      const stderrText = err.stderr ? err.stderr.toString('utf8').trim() : '';
      throw new VerificationError(
        `Verification failed: ${binPath} --version exited with status ${err.status}${
          stderrText ? ` (${stderrText})` : ''
        }`
      );
    }
    // spawn failures (ENOENT/EACCES) or timeouts (signal kill, status is null)
    throw new VerificationError(`Failed to execute ${binPath} --version: ${err.message}`);
  }
}

The error handling is also improved: execFileSync throws a single error object for all failure modes (spawn failure, timeout, non-zero exit), and the catch block now correctly distinguishes between a non-zero exit (where err.status is a number) and a spawn/timeout failure (where err.status is null).

Key Takeaways

• verifyBinaryExecutes in installer.js was passing binPath directly to spawnSync without any path normalization — a pattern that enables path traversal and exploit chaining even without shell expansion.
• Switching to execFileSync with path.resolve() and shell: false closes three distinct attack vectors in a single function: path traversal, accidental shell invocation, and ambiguous error handling.
• "Local CLI tool" does not mean "safe from injection." npm postinstall scripts run in environments where config files, environment variables, and arguments can be influenced by other packages in the dependency tree.
• The stdio: ['ignore', 'pipe', 'pipe'] addition is not cosmetic — it prevents the child process from blocking on stdin and stops its output from leaking into the npm install log, both of which are defense-in-depth improvements.
• Semgrep's detect-child-process rule is a reliable signal — when it fires on a function-argument path, treat it as a real finding and resolve the path before execution.

How Orbis AppSec Detected This

• Source: The binPath parameter of verifyBinaryExecutes(binPath, opts) — ultimately sourced from command-line arguments or install-time configuration passed through resolveTarget.
• Sink: spawnSync(binPath, ['--version'], { ... }) at npm/holidaytw/lib/installer.js:58 — the unsanitized path flows directly into a child process execution call.
• Missing control: No path.resolve() normalization, no explicit shell: false declaration, and no validation of binPath against the expected resolved binary path before execution.
• CWE: CWE-78 — Improper Neutralization of Special Elements used in an OS Command (OS Command Injection).
• Fix: Replaced spawnSync(binPath, ...) with execFileSync(path.resolve(binPath), ..., { shell: false, stdio: ['ignore', 'pipe', 'pipe'] }), eliminating path traversal and shell injection vectors in a single change.

Orbis AppSec automatically detected this vulnerability and opened a pull request with the fix. Try Orbis AppSec on your repositories to find and fix issues like this automatically.

Conclusion

The holidaytw installer vulnerability is a textbook example of why "it's just a local tool" is not a sufficient security argument. The verifyBinaryExecutes function's unsanitized binPath argument created a path traversal and command injection primitive that could be chained with other weaknesses in an automated attack. The fix — four targeted lines replacing spawnSync with execFileSync, adding path.resolve(), and making shell: false explicit — eliminates the attack surface without changing any observable behavior for legitimate inputs.

The broader lesson: every time you pass a function argument into a child_process call, ask yourself three questions: Is this path normalized? Is shell invocation explicitly disabled? And is this value validated against what I actually expect? If the answer to any of those is "no" or "I'm not sure," you have a finding worth fixing.