How Command Injection happens in PHP and how to fix it

Introduction

In a PHP application's lib/Controller/Helper.php file, we discovered a high-severity command injection vulnerability at line 257. The corruptline() method used exec() to run sed and awk shell commands for reading and replacing specific lines in log files. While the code attempted to use escapeshellarg() for protection, the presence of shell execution with user-controllable parameters created an unnecessary and dangerous attack surface. This vulnerability demonstrates why avoiding shell execution entirely is always preferable to relying on sanitization functions.

The vulnerable code pattern appeared in two locations within the same method: first when reading a line with sed, and again when replacing a line with a complex awk command. Both operations could be performed safely using native PHP file functions, making the shell execution completely unnecessary.

The Vulnerability Explained

The corruptline() method in lib/Controller/Helper.php was designed to read and replace specific lines in log files. Here's the vulnerable code that read a line:

if ($this->isExecAvailable()) {
    $fragment = exec("sed -n '{$zeile}p' " . escapeshellarg($file));
} else {
    // fallback code...
}

The critical issue is on line 257 where $zeile (a line number parameter) is directly interpolated into the sed command string: "sed -n '{$zeile}p' ". While $file is protected with escapeshellarg(), the $zeile variable is embedded directly into the command without any escaping.

The Attack Vector:

An attacker controlling the $zeile parameter could inject shell metacharacters to execute arbitrary commands. For example, if $zeile contained:

1'; whoami; echo '

The resulting command would become:

sed -n '1'; whoami; echo 'p' /path/to/file

This executes three separate commands: a sed command, whoami (or any malicious command), and an echo statement. The attacker gains arbitrary command execution with the privileges of the web server process.

The Second Vulnerability:

The same method contained an even more complex command injection point when replacing lines:

$cmd = sprintf(
    "awk -v n=%d -v f=%s 'NR==n{while((getline line < f)>0){print line}; close(f); next} {print}' %s > %s && mv %s %s",
    $zeile,
    escapeshellarg($tmpNew),
    escapeshellarg($file),
    escapeshellarg($file . '.tmp'),
    escapeshellarg($file . '.tmp'),
    escapeshellarg($file)
);
exec($cmd . ' 2>&1');

While this code uses %d for $zeile (which provides some type coercion), the complexity of the shell command chain with multiple file operations, redirections, and the && operator creates multiple potential injection points. The use of sprintf() with %d offers minimal protection if $zeile contains non-numeric data or if PHP's type juggling allows unexpected values through.

Real-World Impact:

In this application, the corruptline() method likely handles log file manipulation. An attacker who could control the line number parameter (perhaps through a log viewing interface or API endpoint) could:

1. Execute arbitrary system commands
2. Read sensitive files from the server
3. Modify or delete critical files
4. Establish persistent backdoors
5. Pivot to other systems on the network

The vulnerability is particularly dangerous because log viewing functionality is often exposed to authenticated users, and developers may not consider line numbers to be security-sensitive input.

The Fix

The fix eliminates shell execution entirely by replacing sed and awk with native PHP file operations. Here's the before and after comparison for reading a line:

Before (Vulnerable):

if ($this->isExecAvailable()) {
    $fragment = exec("sed -n '{$zeile}p' " . escapeshellarg($file));
} else {
    try {
        $fileObj = new \SplFileObject($file);
        $fileObj->seek($zeile - 1);
        $fragment = $fileObj->current();
    } catch (\Exception $e) {
        $fragment = 'could not read line';
    }
}

After (Secure):

try {
    $fileObj = new \SplFileObject($file);
    $fileObj->seek($zeile - 1);
    $fragment = $fileObj->current();
} catch (\Exception $e) {
    $fragment = 'could not read line';
}

The fix makes the fallback code path the primary implementation. SplFileObject is a native PHP class that provides object-oriented file access without any shell involvement. The seek() method jumps directly to the specified line number, and current() returns that line's content. Critically, no shell metacharacters can be injected because no shell is ever invoked.

For the line replacement operation:

Before (Vulnerable):

if ($this->isExecAvailable()) {
    $tmpNew = tempnam(sys_get_temp_dir(), 'newline_');
    file_put_contents($tmpNew, $replaceWith . PHP_EOL);

    $cmd = sprintf(
        "awk -v n=%d -v f=%s 'NR==n{while((getline line < f)>0){print line}; close(f); next} {print}' %s > %s && mv %s %s",
        $zeile,
        escapeshellarg($tmpNew),
        escapeshellarg($file),
        escapeshellarg($file . '.tmp'),
        escapeshellarg($file . '.tmp'),
        escapeshellarg($file)
    );
    exec($cmd . ' 2>&1');
    @unlink($tmpNew);
} else {
    $tempFile = $file . '.tmp';
    $handleIn = fopen($file, 'r');
    $handleOut = fopen($tempFile, 'w');
    // ... line-by-line copy and replace logic
}

After (Secure):

$tempFile = $file . '.tmp';
$handleIn = fopen($file, 'r');
$handleOut = fopen($tempFile, 'w');
// ... line-by-line copy and replace logic

Again, the fix removes the entire shell execution branch and uses only the native PHP implementation. The code now uses fopen() to read the original file and write to a temporary file, copying lines one by one and replacing the target line when encountered. This approach:

1. Eliminates command injection risk - No shell is ever invoked
2. Removes unnecessary complexity - The awk command with redirections and chained commands is replaced with straightforward PHP
3. Improves portability - No dependency on sed/awk being available
4. Maintains functionality - The behavior is identical for all valid inputs

The security improvement is absolute: there is no longer any code path that constructs shell commands from user input, making command injection impossible in this method.

Key Takeaways

• The corruptline() method in Helper.php used exec() with sed/awk to perform file operations that could be done safely with native PHP functions, creating an unnecessary command injection risk.

• Direct interpolation of $zeile into the sed command ("sed -n '{$zeile}p' ") allowed shell metacharacter injection despite escapeshellarg() being used on other parameters.

• SplFileObject and fopen() provide safer alternatives to shell-based file manipulation, eliminating the attack surface entirely rather than relying on sanitization.

• The fix removed all shell execution from the method, proving that the original implementation's complexity was unnecessary and the fallback code path was actually the better solution.

• Even with escapeshellarg(), shell execution creates exploit primitives that automated attack tools can potentially chain with other vulnerabilities, making complete avoidance the best practice.

How Orbis AppSec Detected This

• Source: The $zeile parameter passed to the corruptline() method, which could originate from HTTP requests or other untrusted sources
• Sink: exec() calls at line 257 and subsequent lines in lib/Controller/Helper.php where sed and awk commands are constructed and executed
• Missing control: The $zeile parameter was directly interpolated into shell commands without validation or escaping, and shell execution was used unnecessarily
• CWE: CWE-78 (Improper Neutralization of Special Elements used in an OS Command)
• Fix: Removed all shell execution paths and replaced them with native PHP file operations using SplFileObject and fopen()

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 lib/Controller/Helper.php demonstrates a critical security principle: the best defense is elimination, not sanitization. While escapeshellarg() provides some protection, the presence of shell execution with user-controllable parameters creates unnecessary risk. By replacing sed and awk with native PHP file operations, the fix not only eliminates the vulnerability but also simplifies the code and removes external dependencies.

For developers working on similar code, the lesson is clear: always prefer native language functions over shell execution. When manipulating files, reading lines, or performing common operations, modern programming languages provide safe, efficient alternatives that don't require invoking a shell. This approach eliminates entire classes of vulnerabilities and makes your code more portable and maintainable.