Skip to content

CWE-88: Improper Neutralization of Argument Delimiters in a Command ('Argument Injection') - PHP

Overview

PHP's shell helpers pull in two different weaknesses, and the escaping functions that address one do nothing for the other. escapeshellcmd() neutralises shell metacharacters in a command string; escapeshellarg() wraps a single argument in quotes so the shell treats it as one token. Neither prevents the program from reading a quoted, metacharacter-free -o/var/www/html/shell.php as an option.

proc_open() accepts an argument array as of PHP 7.4, which removes the shell entirely. That is the right baseline, and it is still not sufficient on its own.

Common Vulnerable Patterns

Escaped command string passed to an archiver

<?php
$file = $_POST['file'];

// VULNERABLE - escapeshellcmd() blocks metacharacters, not options
exec(escapeshellcmd("tar -cf backup.tar manifest.json $file"));

// Attack: file=--use-compress-program=/var/app/uploads/evil.sh
// Result: tar runs the uploaded script in place of a compressor

Why this is vulnerable: escapeshellcmd() escapes characters that are special to the shell. A leading - is not one of them, because it is not special to the shell at all - it is special to tar. The escaped string still contains an attacker-chosen option when the shell splits it into arguments, and tar runs the program the option names (measured on GNU tar 1.35, exit 0).

The payload has to survive word splitting here, which is the opposite constraint from the array form. escapeshellcmd() does not escape spaces and does not quote, so the shell splits the value: the version with a space, --use-compress-program=touch /tmp/pwned, reaches tar as two arguments and is inert - measured, touch: missing file operand, /tmp/pwned treated as an archive member, exit 2 and no file created. A path with no spaces keeps the option and its value in one word. The next example is the mirror image: once escapeshellarg() quotes the value, the spaced payload is the one that works.

That splitting is itself part of this weakness rather than a different one - CWE-88 is Improper Neutralization of Argument Delimiters in a Command, and a space that turns one intended argument into two is exactly an injected delimiter. It becomes CWE-78 only when the injected text runs as a command, which needs a metacharacter escapeshellcmd() does escape.

--use-compress-program conflicts with -z: against tar -czf it fails with "Conflicting compression options", which is why the command above compresses nothing.

escapeshellarg() mistaken for argument validation

<?php
// VULNERABLE - the argument is one token, and that token is still an option
$file = escapeshellarg($_GET['file']);
exec("tar -cf backup.tar manifest.json $file");

// Attack: file=--use-compress-program=touch /tmp/pwned
// Result: the quoted value is passed as a single argument - which tar reads as an option

Why this is vulnerable: escapeshellarg() guarantees the value arrives as exactly one argument. That is precisely what the attacker needs: one argument, beginning with -, delivered intact to tar.

A payload therefore has to work as a single argument. --checkpoint-action=exec=sh exploit.sh, the string usually quoted here, does nothing on its own: GNU tar runs checkpoint actions only when --checkpoint=N is also present, and a single quoted argument cannot supply it. Measured on tar 1.35 - the archive was created and no command ran.

Secure Patterns

Argument array with a first-character allowlist

<?php
declare(strict_types=1);

const BASE_DIR = '/var/app/uploads/';
const OUTPUT_DIR = '/var/app/converted/';

// SECURE - no shell, and no accepted value can begin with a dash
function convert(string $inputName, string $outputName): void
{
    if (!preg_match('/\A[A-Za-z0-9][A-Za-z0-9_.-]{0,254}\z/', $inputName)) {
        throw new InvalidArgumentException('Invalid input filename');
    }
    if (!preg_match('/\A[A-Za-z0-9][A-Za-z0-9_.-]{0,250}\.(mp4|webm|avi)\z/', $outputName)) {
        throw new InvalidArgumentException('Invalid output filename');
    }

    $inputPath = realpath(BASE_DIR . $inputName);
    if ($inputPath === false || !str_starts_with($inputPath, BASE_DIR) || !is_file($inputPath)) {
        throw new RuntimeException('Unknown file');
    }

    // proc_open() with an array (PHP 7.4+) bypasses the shell entirely.
    // stdout and stderr go to temp files, not pipes: a pipe nobody is
    // draining fills up and blocks the child - see below.
    $out = tmpfile();
    $err = tmpfile();
    $descriptors = [0 => ['pipe', 'r'], 1 => $out, 2 => $err];
    $process = proc_open(
        ['ffmpeg', '-y', '-i', $inputPath, '--', OUTPUT_DIR . $outputName],
        $descriptors,
        $pipes
    );

    if (!is_resource($process)) {
        throw new RuntimeException('Conversion failed to start');
    }

    fclose($pipes[0]);            // the child sees EOF on stdin rather than waiting
    $status = proc_close($process);

    if ($status !== 0) {
        rewind($err);
        $message = stream_get_contents($err);
        fclose($out);
        fclose($err);
        throw new RuntimeException("Conversion failed: $message");
    }
    fclose($out);
    fclose($err);
}

Why this works: Each control covers a different failure:

  • The array form of proc_open() hands argv to the process directly, so no shell parses the command and no escaping function is needed or useful.
  • The patterns require an alphanumeric first character. Stating it that way, rather than rejecting a leading -, also excludes --, a unicode dash, and a space-prefixed value - all of which a "must not start with a dash" check misses.
  • realpath() plus the prefix check confines the resolved file to the upload directory, which the pattern alone does not do.
  • -- stops option parsing in tools that support it, covering a later relaxation of the pattern. ffmpeg does - verified on 8.0.1, an output named -weird.mp4 is written rather than parsed.

The stream handles are the part that is easy to get wrong, and ffmpeg is the worst case for it. The obvious spelling declares 1 => ['pipe', 'w'] and 2 => ['pipe', 'w'], then reads stdout and stderr in that order. It deadlocks: ffmpeg writes its progress to stderr, that pipe fills, the child blocks in write(), and stream_get_contents($pipes[1]) waits for a stdout EOF that can never arrive. Measured on PHP 8.5.8 against a child emitting 400 KB on stderr, that shape hung until an external 30-second timeout killed it, having read 4 KB. proc_close()'s documentation warns that open pipes can stop a child exiting, which is true and no help - execution never reaches proc_close(). Temp files cannot fill; the alternative is stream_set_blocking() on both pipes and a stream_select() loop that drains them together. The same hazard, in a shell-injection context, is on CWE-77.

-y is not part of the security fix. Without it, an existing output file makes ffmpeg print "Not overwriting - exiting" and exit 0 (measured on 8.0.1), so the $status !== 0 check reports success for a conversion that never happened.

proc_open() has no timeout of its own and proc_close() blocks until the child exits, so a hung conversion hangs the request. Where the command can be slow, poll proc_get_status() against a deadline and proc_terminate() when it passes.

Use a PHP extension instead of a subprocess

<?php
// SECURE - no argument vector exists
$archive = new ZipArchive();
if ($archive->open('/var/app/archive.zip', ZipArchive::CREATE | ZipArchive::OVERWRITE) !== true) {
    throw new RuntimeException('Cannot create archive');
}
$archive->addFile($inputPath, basename($inputPath));
$archive->close();

Why this works: ZipArchive takes paths as typed arguments, so there is no argument vector for an option to appear in - the weakness is removed rather than constrained. The same reasoning applies to curl (use the cURL extension or an HTTP client library rather than the curl binary) and to image work (use GD or Imagick rather than shelling out to a converter).

Considerations

  • Neither escaping function is the fix, and reaching for the "stronger" one is the usual wrong turn. escapeshellarg() makes the value a single argument, which is what the attack needs. If a finding was closed by swapping escapeshellcmd() for escapeshellarg(), it was not closed.
  • escapeshellarg() is also locale- and platform-dependent. It strips bytes that are invalid in the current locale, and on Windows it quotes with " rather than '. Even where it is the right tool for a shell string, it is not a validator.
  • What the binary exposes sets the severity. tar, curl and git all have options that read or write arbitrary paths; GTFOBins is the per-binary reference. Where the reachable options cannot touch the filesystem or spawn a process, recording a false positive with that reasoning is a legitimate outcome.
  • Whether the value can still split into several arguments. This is what separates the two vulnerable examples above from the proc_open() array form, and it changes which payloads are possible. Through a shell, an unquoted value becomes as many arguments as it has spaces, so an option and its value can be supplied together. Through an array - or through escapeshellarg() - the value is exactly one argument, so only options carrying their own value (--opt=value, or an attached short option such as -K/path) can be delivered. ffmpeg has no --opt=value form at all, which makes it much harder to attack through the array form than through the string form.
  • Check every exec-family call, not only the reported one. exec(), shell_exec(), system(), passthru(), popen() and the backtick operator all reach the same place, and a codebase that has one usually has several.

Testing

The scanner sees an exec call with an escaped argument before and after the fix, so it cannot tell you whether anything changed. Assert the behaviour.

  • Send --use-compress-program=touch /tmp/pwned and assert both a rejection and that /tmp/pwned does not exist. Run it against the unfixed code first: --checkpoint-action=exec=touch /tmp/pwned, the payload usually reached for, is inert without a companion --checkpoint=N and so passes this assertion whether the fix works or not.
  • Send -weird.mp4 as the output name and assert rejection; confirm no file was written outside the output directory.
  • Send --help and assert rejection. Tools usually exit 0 for it, so this is the payload that most easily looks like a successful conversion.
  • Assert that legitimate uploads still convert: names with internal hyphens and dots, and every extension in the allowlist. Tightened validation breaking real filenames is the failure mode a re-scan will never show.

Additional Resources