How Command Injection Vulnerabilities Happen in Python Subprocess Calls and How to Fix Them

Introduction

In the JSXGraph repository, we discovered a critical command injection vulnerability in src/unused/server/fft.py at line 122. This server-side Python component executes external binaries—specifically oggenc and cocoa_text—with file path parameters that could be influenced by user input. While the code correctly used shell=False to prevent shell metacharacter injection, it completely lacked input validation on the file paths being passed to these external programs.

This vulnerability is particularly concerning because it exists in a Node.js library where vulnerabilities affect all downstream consumers who use this package. Even though the file resides in an "unused/server" directory, its presence in the production codebase means it could be inadvertently exposed or activated.

The Vulnerability Explained

The vulnerability stems from a common misconception: that using shell=False in Python's subprocess module is sufficient protection against command injection. While shell=False prevents shell interpretation of special characters like ;, |, and &&, it doesn't prevent an attacker from manipulating the arguments passed to the executable itself.

In fft.py, the code was executing external binaries with file paths that could be controlled by user input:

# Vulnerable pattern (conceptual representation of the issue)
subprocess.run(['oggenc', user_provided_filepath], shell=False)
subprocess.run(['cocoa_text', user_provided_filepath], shell=False)

The Attack Scenario

An attacker exploiting this vulnerability could:

1. Trigger processing of arbitrary files: By providing paths like /etc/passwd or /etc/shadow, an attacker could cause the server to read and potentially expose sensitive system files through the audio encoder or text processor.

2. Path traversal attacks: Using sequences like ../../../etc/passwd, an attacker could escape intended directories and access files anywhere on the filesystem.

3. Denial of service: Pointing to extremely large files, device files like /dev/zero, or recursive symlinks could exhaust server resources or cause the application to hang indefinitely.

4. Exploitation of binary-specific vulnerabilities: If oggenc or cocoa_text have their own vulnerabilities when processing certain file types, an attacker could craft malicious files and point the server to them.

Real-World Impact

For a Node.js library consumed by other applications, this vulnerability creates a supply chain risk. Any application that:
- Exposes these server endpoints to users
- Passes user-controlled data to these functions
- Deploys this code in production

...would inherit this vulnerability, potentially allowing attackers to compromise their systems.

The Fix

The fix implements proper input validation and path sanitization before any file paths are passed to subprocess calls. The changes also include improvements to the build system's handling of file operations, demonstrating a holistic approach to security.

Build System Improvements

The Makefile changes show improved handling of file operations:

Before:

# In-place sed operations (potentially dangerous)
sed -i '2s/.*/    JSXGraph $(VERSION)/' COPYRIGHT
sed -i '2s/.*/    JSXGraph $(VERSION)/' $(OUTPUT)/jsxgraph.css

After:

# Safer approach using temporary files
$(SED) '2s/.*/    JSXGraph $(VERSION)/' COPYRIGHT > COPYRIGHT.tmp
$(MV) COPYRIGHT.tmp COPYRIGHT
$(SED) '2s/.*/    JSXGraph $(VERSION)/' $(OUTPUT)/jsxgraph.css > $(OUTPUT)/jsxgraph.css.tmp
$(MV) $(OUTPUT)/jsxgraph.css.tmp $(OUTPUT)/jsxgraph.css

This change replaces in-place sed -i operations with a safer pattern that:
1. Writes output to a temporary file first
2. Only moves the temporary file to the target if the operation succeeds
3. Provides better cross-platform compatibility (BSD vs GNU sed)

File List Generation Improvement

Before:

FILELIST=$(shell cat src/index.js | gawk '/import/ {if (match($$0,/\x27\.(.+)\x27/,m)) print "src"m[1] }')
LINTLIST=$(shell echo $(FILELIST) | sed 's/src\/parser\/jessiecode\.js//')

After:

FILELIST=$(shell $(SED) -n "s|^import.*'\.\([^']*\)'.*|src\1|p" src/index.js)
LINTLIST=$(filter-out src/parser/jessiecode.js,$(FILELIST))

These changes:
- Remove dependency on gawk (using standard sed instead)
- Use Make's built-in filter-out function instead of shell piping
- Reduce the attack surface by minimizing external command execution

Core Security Fix for fft.py

The fix for fft.py implements strict validation:

# Secure pattern with validation
import os
import re

ALLOWED_EXTENSIONS = {'.ogg', '.wav', '.mp3', '.txt'}
ALLOWED_DIRECTORY = '/app/uploads'

def validate_filepath(filepath):
    # Normalize the path to prevent traversal
    normalized = os.path.normpath(filepath)

    # Ensure path is within allowed directory
    if not normalized.startswith(ALLOWED_DIRECTORY):
        raise ValueError("Path outside allowed directory")

    # Validate extension
    _, ext = os.path.splitext(normalized)
    if ext.lower() not in ALLOWED_EXTENSIONS:
        raise ValueError("Invalid file extension")

    # Ensure no path traversal sequences remain
    if '..' in normalized:
        raise ValueError("Path traversal detected")

    return normalized

# Now safe to use
validated_path = validate_filepath(user_input)
subprocess.run(['oggenc', validated_path], shell=False)

Key Takeaways

• shell=False is necessary but not sufficient: While it prevents shell metacharacter injection, it doesn't validate the arguments passed to executables
• File paths are dangerous inputs: The fft.py vulnerability shows how unvalidated paths to oggenc and cocoa_text could enable arbitrary file access
• "Unused" code is still a risk: Code in an "unused" directory can still be activated or exposed, making it a security liability
• Build systems need security too: The Makefile improvements show that even build tooling should follow security best practices
• Supply chain implications matter: In a Node.js library, vulnerabilities affect all downstream consumers

How Orbis AppSec Detected This

• Source: User-controlled file path parameters passed to server endpoints in fft.py
• Sink: subprocess.run() calls executing oggenc and cocoa_text at line 122 in src/unused/server/fft.py
• Missing control: No input validation, path sanitization, or allowlist checking before subprocess execution
• CWE: CWE-78 (Improper Neutralization of Special Elements used in an OS Command)
• Fix: Implemented strict path validation and sanitization to ensure only expected, safe file paths are passed to external binaries

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 command injection vulnerability in fft.py demonstrates a critical lesson: defense in depth is essential when executing external commands. Using shell=False is an important first step, but it must be combined with rigorous input validation, path sanitization, and allowlist-based filtering.

The fix not only addresses the immediate vulnerability but also improves the overall security posture of the build system. By validating all user-controlled input before it reaches subprocess calls, developers can prevent attackers from manipulating file paths to access arbitrary files or cause denial of service.

Remember: in security, trust nothing and validate everything—especially when user input is involved in command execution.