How Command Injection Vulnerabilities Happen in Node.js Child Process Calls and How to Fix Them

How Command Injection Vulnerabilities Happen in Node.js Child Process Calls and How to Fix Them

Introduction

In the private Node.js application repository, a critical command injection vulnerability was discovered in scripts/setup-harness.js — the setup script responsible for cloning vendor dependencies and building the test harness. The vulnerability was flagged by Semgrep on line 18, within a generic run() function that accepted user-controlled command strings as function arguments and passed them directly to Node.js's child_process.spawnSync().

The problematic code was deceptively simple:

function run(command, args, cwd) {
  console.log(`> ${command} ${args.join(' ')}`);
  const result = spawnSync(command, args, {
    cwd,
    stdio: 'inherit',
    env: harnessEnv(),
  });
  if (result.status !== 0) {
    process.exit(result.status || 1);
  }
}

At first glance, this looks benign — it's just a wrapper around spawnSync(). But here's the critical flaw: the command parameter is user-controlled. If any caller passed an untrusted value as command, an attacker could inject arbitrary shell commands. While this application is private and not directly exposed to internet-facing input, the vulnerability represents an "exploit primitive" — a code pattern that automated attack tools chain together to bypass security defenses. This was the exact pattern that led to the trivy-action and kics-github-action supply-chain compromises in GitHub Actions.

The Vulnerability Explained

What makes setup-harness.js vulnerable?

The vulnerability stems from three critical misconfigurations in the original run() function:

1. Untrusted command parameter: The function accepts command as a variable argument passed directly to spawnSync(). If any code path supplies this argument from untrusted input, command injection becomes possible.

2. Shell interpretation enabled implicitly: The original code does not explicitly set shell: false. While spawnSync() without shell: true is safer than execSync(), the lack of explicit shell: false creates ambiguity and leaves the door open for misconfigurations in future refactors.

3. Generic function design: By creating a one-size-fits-all run() function, the code encourages casual usage patterns. Callers might eventually use this function with dynamic input without thinking through security implications.

The attack scenario:

Imagine a hypothetical scenario where an environment variable (e.g., REPO_CLONE_COMMAND) or a configuration file parameter could influence the command argument. An attacker could inject:

command = "git; rm -rf /"

The function would then execute:

spawnSync("git; rm -rf /", ["clone", "--depth", "1", "--branch", pin.ref, pin.repo, vendor], ...)

While spawnSync() without shell: true treats the entire string as a single command name (not a shell pipeline), this is fragile and creates a dangerous exploit primitive that automated tooling can chain into larger attack chains.

Why does this matter?

According to the PR description, this pattern was specifically used in the recent compromises of trivy-action and kics-github-action — both GitHub Actions that were supply-chain compromised by attackers who detected these exploit primitives and weaponized them. Removing the primitive proactively hardens the codebase against:

• Automated exploit development: Tools like Semgrep's SAST engine or GPT-based code analysis can now longer generate exploits based on this pattern.
• Future refactors: If this code is refactored in six months without security review, the new implementer won't be tempted to pass dynamic input to a generic run() function.
• Dependency confusion attacks: If a third-party module ever needs to call this setup script, it can't inject malicious commands.

The Fix

The PR replaces the generic, unsafe run() function with two hardened, command-specific functions: runGit() and runNode().

Before (vulnerable):

function run(command, args, cwd) {
  console.log(`> ${command} ${args.join(' ')}`);
  const result = spawnSync(command, args, {
    cwd,
    stdio: 'inherit',
    env: harnessEnv(),
  });
  if (result.status !== 0) {
    process.exit(result.status || 1);
  }
}

// Calls to run():
run('git', ['clone', '--depth', '1', '--branch', pin.ref, pin.repo, vendor], root);
run(process.execPath, [pnpm, 'install', '--frozen-lockfile'], vendor);
run(process.execPath, [pnpm, 'run', 'build'], vendor);

After (hardened):

function runGit(args, cwd) {
  console.log(`> git ${args.join(' ')}`);
  const result = spawnSync('git', args, {
    cwd,
    stdio: 'inherit',
    env: harnessEnv(),
    shell: false,
  });
  if (result.status !== 0) {
    process.exit(result.status || 1);
  }
}

function runNode(args, cwd) {
  console.log(`> node ${args.join(' ')}`);
  const result = spawnSync(process.execPath, args, {
    cwd,
    stdio: 'inherit',
    env: harnessEnv(),
    shell: false,
  });
  if (result.status !== 0) {
    process.exit(result.status || 1);
  }
}

// Calls now use specific functions:
runGit(['clone', '--depth', '1', '--branch', pin.ref, pin.repo, vendor], root);
runNode([pnpm, 'install', '--frozen-lockfile'], vendor);
runNode([pnpm, 'run', 'build'], vendor);

Key security improvements:

The fix achieves defense in depth by combining three hardening techniques:

1. Hardcoding the executable: The command parameter is eliminated entirely. runGit() always uses 'git', and runNode() always uses process.execPath. This removes the injection point.

2. Explicit shell: false: This ensures that spawnSync() treats the executable name as a literal program name, not a shell command string. Even if an attacker somehow manipulates arguments, no shell metacharacters are interpreted.

3. Command-specific functions: By creating separate runGit() and runNode() functions, the code communicates intent clearly and prevents accidental misuse. Future developers can't "just call run() with a dynamic command" because run() no longer exists.

Prevention & Best Practices

To avoid command injection vulnerabilities in Node.js applications:

1. Never pass user input as the command argument to child_process functions:
   ```javascript
   // UNSAFE
   spawnSync(userInput, args); // command injection!

// SAFE
spawnSync('/usr/bin/git', args); // hardcoded executable
```

1. Always set shell: false explicitly:
   javascript spawnSync('git', ['clone', repo], { shell: false }); // Recommended

2. Use the array form of arguments, not shell strings:
   ``javascript // UNSAFE spawnSync('sh', ['-c',git clone ${repo}`]); // vulnerable to injection

// SAFE
spawnSync('git', ['clone', repo]); // arguments as array
```

1. Use allowlists for any dynamic command selection:
   javascript const commands = { git: '/usr/bin/git', npm: '/usr/bin/npm' }; const cmd = commands[userChoice]; // Validate against allowlist if (!cmd) throw new Error('Invalid command'); spawnSync(cmd, args);

2. Apply the principle of least privilege:
   - Run child processes with minimal required permissions.
   - Use uid and gid options in spawnSync() to drop privileges if possible.

3. Use static analysis tools to catch these patterns:
   - Semgrep: Rule javascript.lang.security.detect-child-process detects child_process calls with untrusted input.
   - CodeQL: Query js/command-injection identifies potential command injection sinks.
   - ESLint: Plugin eslint-plugin-security flags suspicious child_process patterns.

CWE Reference:
- CWE-78: Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection')
- CWE-94: Improper Control of Generation of Code ('Code Injection')

Key Takeaways

• Never accept command names as function parameters. The run(command, args) pattern in setup-harness.js was fundamentally unsafe because attackers could inject arbitrary executables. Hardcoding the executable eliminates this risk entirely.

• Explicit shell: false is not optional. While spawnSync() defaults to safe behavior, explicitly setting shell: false prevents future refactors from accidentally enabling shell interpretation, and it documents security intent for code reviewers.

• Command-specific wrapper functions prevent misuse. By replacing the generic run() function with runGit() and runNode(), the codebase makes it impossible to accidentally pass untrusted input to child_process, because the executable is selected by the function name, not the caller.

• Exploit primitives matter in supply-chain security. The PR description specifically notes that this code pattern was weaponized in the trivy-action and kics-github-action compromises. Removing exploit primitives proactively hardens defenses against automated attack tooling.

• Static analysis catches these patterns reliably. Semgrep's detect-child-process rule flagged this vulnerability automatically, demonstrating that SAST tools should be integrated into CI/CD pipelines to detect similar issues before they reach production.

How Orbis AppSec Detected This

• Source: The command parameter passed to the run() function (line 18 in the original code), which could accept untrusted values from callers unaware of security implications.

• Sink: The spawnSync(command, args, ...) call on line 20, where the untrusted command string is passed directly as the executable argument.

• Missing control: No validation of the command parameter, no explicit shell: false configuration, and no architectural barrier preventing untrusted input from reaching spawnSync().

• CWE: CWE-78 (Improper Neutralization of Special Elements used in an OS Command).

• Fix: Replaced the generic run() function with hardened, command-specific functions (runGit() and runNode()) that hardcode the executable path, explicitly set shell: false, and accept only arguments as an array, eliminating the attack surface entirely.

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 command injection vulnerability in setup-harness.js demonstrates a critical security principle: generic wrapper functions around dangerous APIs create exploit primitives. By refactoring the unsafe run() function into hardened, purpose-specific functions (runGit() and runNode()), the fix eliminates the injection vector while maintaining code readability and functionality.

This is not a theoretical risk. The trivy-action and kics-github-action compromises were built on exactly this pattern. Proactive removal of exploit primitives — even in private codebases — hardens defenses against increasingly sophisticated automated attack tools that can chain multiple vulnerabilities into complete exploits.

As developers, adopt this mindset: avoid generic "run anything" functions, hardcode executables at code-write time, and use static analysis tools to catch these patterns before they reach production.

References

• CWE-78: Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection')
• OWASP Command Injection
• Node.js child_process Documentation
• Semgrep Rule: javascript.lang.security.detect-child-process
• GitHub Security Advisory: trivy-action Supply Chain Compromise
• harden: sanitize child_process call in setup-harness.js...