Sec Bug->Doc #78569 [ReO->Ver]: proc_open() may require extra quoting

Edit report at https://bugs.php.net/bug.php?id=78569&edit=1 ID: 78569 Updated by: cmb@php.net Reported by: 64796c6e69 at gmail dot com -Summary: proc_open() arbitrary write with escapeshellarg() +Summary: proc_open() may require extra quoting -Status: Re-Opened +Status: Verified -Type: Security +Type: Documentation Problem -Package: CGI/CLI related +Package: Program Execution Operating System: Windows PHP Version: 7.2.22 Assigned To: cmb Block user comment: N Private report: Y New Comment: 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> Previous Comments: ------------------------------------------------------------------------ [2019-12-01 16:14:11] cmb@php.net 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> ------------------------------------------------------------------------ [2019-11-18 17:16:38] 64796c6e69 at gmail dot com 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. ------------------------------------------------------------------------ [2019-11-18 15:18:09] ab@php.net 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. ------------------------------------------------------------------------ [2019-11-15 20:56:23] 64796c6e69 at gmail dot com 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. ------------------------------------------------------------------------ [2019-11-15 12:05:56] cmb@php.net 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. ------------------------------------------------------------------------ The remainder of the comments for this report are too long. To view the rest of the comments, please view the bug report online at https://bugs.php.net/bug.php?id=78569 -- Edit this bug report at https://bugs.php.net/bug.php?id=78569&edit=1