A completely normal-looking function
A search feature that shells out to a command-line tool is a common real pattern, searching log files with grep or findstr, resizing images with a CLI tool, converting documents. The obvious first version looks like this:
import subprocess
def search_logs(term):
return subprocess.run(
f"findstr {term} access.log",
shell=True, capture_output=True, text=True
).stdout
Takes a search term, builds a shell command string, runs it. It works correctly for every normal search term anyone tests it with.
What "term" can actually contain
malicious_term = "error & type secret.txt & rem"
result = search_logs(malicious_term)
print(result)
real secret content, not meant to be read
The function was asked to search for log lines matching an error. What it actually did was read out the contents of a completely unrelated file on the same machine. Nothing about the function's own code changed between the normal case and this one, only the string that got passed in.
Why
shell=True hands the entire command string to a real system shell (cmd.exe on Windows, sh on Linux and macOS) to interpret before running anything. Shells treat certain characters as instructions rather than plain text: & and ; chain multiple separate commands together, | pipes one command's output into another, ` and $() run a command and substitute its output inline. An f-string that inserts a search term directly into a shell command string doesn't know the difference between "text the user wants to search for" and "a new command the shell should run." Whatever the caller passes in, the shell will happily interpret as shell syntax if it looks like some.
This is the exact same category of bug as SQL injection, string-built input crossing into executable syntax, just for a different interpreter.
The fix
def search_logs(term):
return subprocess.run(
["findstr", term, "access.log"],
capture_output=True, text=True
).stdout
Two changes: a list of arguments instead of one interpolated string, and no shell=True. Run the exact same malicious input through this version:
result = subprocess.run(
["findstr", "error & type secret.txt & rem", "access.log"],
capture_output=True, text=True
)
print(result.stdout)
10.0.0.2 GET /login error
The entire malicious-looking string gets passed to findstr as one single, literal search argument. No shell ever parses it, so &, ;, and every other shell-special character are just ordinary characters as far as the search tool is concerned. It correctly finds the one real log line containing the literal text "error" and nothing else happens.
Why this is easy to end up with anyway
shell=True is genuinely necessary sometimes, real shell features like pipes (|), wildcard expansion, or environment variable substitution only work when a real shell is involved. The mistake isn't using shell=True itself; it's combining it with any value that came from outside the program, a user, a config file, another service's response, an environment variable someone else controls. The list-of-arguments form should be the default reflex for anything involving external input, with shell=True reserved for commands built entirely from fixed, hardcoded strings.
The takeaway
The vulnerable version and the safe version differ by a data type, a Python string versus a Python list, not by any obvious red flag in the code. Any subprocess call that builds a command from shell=True plus a value that isn't a hardcoded literal is worth checking directly: try passing a value containing &, ;, or | and see what actually happens, the same way this one was checked here.
Comments
Loading comments...