Fix the command injection in this thumbnail helper that calls out to a shell
This helper renders a thumbnail by shelling out to an image tool. filename arrives from an HTTP request and is not checked anywhere else.
Constraints:
- keep using the external
convertbinary - the caller must still receive the rendered bytes
- do not rely on a WAF or on filtering metacharacters
import subprocess
def make_thumbnail(filename: str) -> bytes:
cmd = f"convert /var/uploads/{filename} -resize 200x200 png:-"
return subprocess.check_output(cmd, shell=True)Find and fix the vulnerability.
With shell=True the shell parses the whole assembled string, so filename can append further commands. Remove the shell and pass an argument list, then constrain the value: reduce it to a basename matching a strict allowed pattern.
- ✗Quoting the interpolated path instead of dropping
shell=True - ✗Filtering a blocklist of metacharacters as the primary defense
- ✗Passing the raw name to the argument list without any validation
- →Why does an argument list stop the injection even without any validation?
- →Why should the path be rebuilt from a basename rather than trusted as sent?
The vulnerability
shell=True makes /bin/sh parse the whole assembled string. filename lands inside it as text, so shell metacharacters turn one command into several — the input becomes code.
The fix
Two independent layers: remove the interpreter, then constrain the value.
import re
import subprocess
from pathlib import Path
UPLOADS = Path("/var/uploads")
SAFE_NAME = re.compile(r"\A[A-Za-z0-9_-]{1,64}\.(png|jpg|jpeg)\Z")
def make_thumbnail(filename: str) -> bytes:
name = Path(filename).name # drop any path segments
if not SAFE_NAME.fullmatch(name):
raise ValueError("bad filename")
src = UPLOADS / name
return subprocess.check_output(
["convert", "--", str(src), "-resize", "200x200", "png:-"],
)✅ The argument list alone means no shell is in the path at all. The pattern check and Path(...).name additionally close path traversal, and -- stops the name being read as an option.
