How Command Injection Happens in PHP and How to Fix It

How Command Injection Happens in PHP and How to Fix It

Introduction

In the file import processing logic of a PHP application, a critical security issue was discovered in lib/Controller/ImportController.php at line 128. The extractDocText() and extractPdfText() methods were using shell_exec() and exec() to invoke external command-line tools (catdoc and pdftotext) for document text extraction. While the file paths were properly escaped using escapeshellarg(), the command execution pattern itself—invoking system commands to process untrusted file uploads—created an exploit primitive vulnerable to command injection attacks.

This vulnerability is particularly insidious because it doesn't require direct user control of the command name. Instead, it represents a code pattern that could be chained with other weaknesses by sophisticated or automated attack tools. The security fix addresses this by eliminating the shell command execution entirely.

The Vulnerability Explained

What Made This Code Vulnerable?

Let's examine the original vulnerable code from extractDocText():

private function extractDocText(string $filePath): string {
    $cmd = escapeshellcmd('catdoc') . ' ' . escapeshellarg($filePath);
    $output = shell_exec($cmd);
    if ($output && !empty(trim($output))) {
        return $output;
    }
    // ... fallback logic
}

And the even more dangerous extractPdfText() method:

private function extractPdfText(string $filePath): string {
    $outputFile = tempnam(sys_get_temp_dir(), 'pdf_');
    $cmd = escapeshellcmd('pdftotext') . ' ' . escapeshellarg($filePath) . ' ' . escapeshellarg($outputFile);
    exec($cmd, $output, $returnCode);

    if ($returnCode === 0 && file_exists($outputFile)) {
        $text = file_get_contents($outputFile);
        unlink($outputFile);
        if (!empty(trim($text))) {
            return $text;
        }
    }
    // ... cleanup and error handling
}

The Problem: Non-Constant Command Execution

The Semgrep rule php.lang.security.exec-use.exec-use flags this pattern because:

1. Shell Command Execution Functions: Both shell_exec() (line 127) and exec() (line 161) are inherently dangerous functions that execute OS-level commands through the system shell.

2. Non-Constant Command Structure: While escapeshellarg() was used to protect the $filePath argument, the command itself (catdoc, pdftotext) is hard-coded. The vulnerability isn't that an attacker can directly control these command names—it's that the presence of this code pattern creates an exploit primitive.

3. Exploit Primitive Definition: An exploit primitive is a code pattern that, while not immediately exploitable in isolation, can be weaponized when combined with other vulnerabilities. Automated attack tools increasingly look for these patterns to build sophisticated polyglot attacks.

Why This Matters

Consider this attack scenario:

1. An attacker discovers a separate vulnerability in the application's logging or caching system that allows them to write files to the server's filesystem.
2. The attacker writes a malicious shell script named catdoc to a directory in the system $PATH.
3. When ImportController::extractDocText() executes the $cmd variable, the system executes the attacker's malicious catdoc script instead of the legitimate one.
4. The attacker gains arbitrary code execution.

While this requires chaining vulnerabilities, the presence of the shell_exec() pattern makes it possible. Modern security practices focus on removing these exploit primitives proactively rather than waiting for the perfect combination of weaknesses to emerge.

The Fix

The security fix takes a complete removal approach, eliminating both instances of dangerous shell command execution:

Change 1: Removing Shell Execution from extractDocText()

Before:

private function extractDocText(string $filePath): string {
    $cmd = escapeshellcmd('catdoc') . ' ' . escapeshellarg($filePath);
    $output = shell_exec($cmd);
    if ($output && !empty(trim($output))) {
        return $output;
    }

    $content = file_get_contents($filePath);
    $text = preg_replace('/[^\p{L}\p{N}\s\.\,\!\?\;\:\'\"\(\)\[\]\{\}\<\>\/\-\=\+\*\&\^\%@\#\$\€\£\\\|]/u', ' ', $content);
    $text = preg_replace('/\s+/', ' ', $text);
    return $text;
}

After:

private function extractDocText(string $filePath): string {
    $content = file_get_contents($filePath);
    $text = preg_replace('/[^\p{L}\p{N}\s\.\,\!\?\;\:\'\"\(\)\[\]\{\}\<\>\/\-\=\+\*\&\^\%@\#\$\€\£\\\|]/u', ' ', $content);
    $text = preg_replace('/\s+/', ' ', $text);
    return $text;
}

The fix removes the shell_exec() call entirely and relies on the existing fallback mechanism: reading the file directly with file_get_contents() and parsing it using regex-based text extraction. This is safer because:
- No external process execution
- No system shell involvement
- All parsing happens within the PHP process using built-in functions

Change 2: Removing Shell Execution from extractPdfText()

Before:

private function extractPdfText(string $filePath): string {
    $outputFile = tempnam(sys_get_temp_dir(), 'pdf_');
    $cmd = escapeshellcmd('pdftotext') . ' ' . escapeshellarg($filePath) . ' ' . escapeshellarg($outputFile);
    exec($cmd, $output, $returnCode);

    if ($returnCode === 0 && file_exists($outputFile)) {
        $text = file_get_contents($outputFile);
        unlink($outputFile);
        if (!empty(trim($text))) {
            return $text;
        }
    }

    if (file_exists($outputFile)) unlink($outputFile);
    throw new \Exception('Could not extract text from PDF file. Please ensure pdftotext is installed');
}

After:

private function extractPdfText(string $filePath): string {
    throw new \Exception('PDF text extraction is not supported. Please convert to a supported format.');
}

This change takes a more aggressive approach: the method now explicitly rejects PDF processing rather than attempting external command execution. This is a deliberate trade-off:

• Security gain: Complete elimination of command injection risk
• Functional change: Users must convert PDFs to supported formats (DOC, ODT, HTML) before import

The decision to throw an exception instead of silently falling back reflects the principle that security is more important than feature completeness. This forces developers and users to be intentional about PDF handling rather than relying on an unsafe external tool.

Prevention & Best Practices

1. Avoid Shell Execution Functions Entirely

Never use these functions unless absolutely necessary:
- shell_exec()
- exec()
- system()
- passthru()
- proc_open() with shell=true
- Backtick operator (`)

Instead, use PHP-native alternatives:

2. If External Commands Are Unavoidable

Use the proc_open() function family with proper safeguards:

// Better approach: explicit argument array instead of shell string
$descriptorspec = [
    0 => ["pipe", "r"],
    1 => ["pipe", "w"],
    2 => ["pipe", "w"]
];

$process = proc_open('/usr/bin/catdoc', $descriptorspec, $pipes, null, [
    // Never mix arguments into a single string
]);

// Always verify the process succeeds and validate output
if ($process !== false) {
    // Handle pipes safely
    fclose($pipes[0]);
    $output = stream_get_contents($pipes[1]);
    fclose($pipes[1]);
    proc_close($process);
}

3. Leverage Static Analysis Tools

Use tools like Semgrep to catch these patterns automatically:

# Semgrep rule to detect dangerous command execution
semgrep --config "p/security-audit" lib/Controller/

This catches shell_exec(), exec(), and similar patterns in your codebase.

4. Code Review Checklist for File Processing

• ✅ Does the code execute external commands? If yes, require security review.
• ✅ Are all command components hard-coded? Even then, reconsider if a PHP library exists.
• ✅ Is input validated and typed? Type hints alone don't prevent injection.
• ✅ Are there tests covering malicious input? (File names with special characters, etc.)
• ✅ Is there a documented reason why a PHP library couldn't be used?

Key Takeaways

• Never invoke external commands for user-uploaded file processing: The extractDocText() and extractPdfText() methods attempted to use system tools on potentially malicious files, creating an attack surface.

• escapeshellarg() is not sufficient: Even though file paths were properly escaped in both methods, the presence of shell_exec() and exec() calls creates an exploit primitive that Semgrep correctly flagged.

• Removing functionality is sometimes the right security choice: Rather than patching extractPdfText() to make PDF processing "safer," the fix entirely removes it and forces users to preprocess PDFs, eliminating the attack vector entirely.

• Exploit primitives matter in threat modeling: The fix demonstrates that security hardening isn't just about fixing active vulnerabilities but removing patterns that sophisticated automated tools could chain together.

• File processing should stay in-process: The fixed extractDocText() uses regex-based text extraction via file_get_contents(), keeping all processing within the PHP runtime where you have full control and visibility.

How Orbis AppSec Detected This

• Source: File paths from the $filePath parameter in the extractDocText() (line 127) and extractPdfText() (line 161) methods, ultimately derived from user-uploaded files via the import functionality.

• Sink: The shell_exec($cmd) call at line 128 and exec($cmd, $output, $returnCode) at line 161, both executing dynamically constructed command strings through the system shell.

• Missing control: While escapeshellarg() was used to escape file path arguments, there was no validation preventing the execution of any external command, and the command-execution pattern itself created an exploit primitive.

• CWE: CWE-78 (Improper Neutralization of Special Elements used in an OS Command / 'OS Command Injection')

• Fix: Removed the shell_exec('catdoc ...') invocation entirely from extractDocText(), relying on safe PHP-native file reading and regex parsing instead. Removed the exec('pdftotext ...') invocation entirely from extractPdfText(), replacing it with an explicit exception directing users to convert PDFs to supported formats.

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 command injection vulnerability in ImportController.php demonstrates why security isn't just about fixing immediate exploits—it's about removing exploit primitives that future attackers or automated tools might weaponize. By eliminating shell command execution from document processing and adopting PHP-native alternatives, the application becomes fundamentally safer.

For developers working with file uploads and document processing, the lesson is clear: prefer native language features and well-maintained libraries over external command execution. Use static analysis tools like Semgrep to catch these patterns before they reach production, and embrace security fixes that remove functionality when necessary—it's often the right trade-off.

References

• CWE-78: Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection')
• OWASP Command Injection
• PHP Security: Avoiding Shell Injection
• Semgrep Rule: php.lang.security.exec-use.exec-use
• GitHub PR: harden: executing non-constant commands in ImportController.php...