What is your current concrete security flaw scenario with this behavior? Could you please post an evident example? Does the program you're calling something harmful by default?

Internally these escaping functions like php_escape_shell_arg() can be misused same way as many others, however it doesn't mean some flaw in the core is uncovered. Functions that expect a '\0' terminated string are all sensitive to external extensions calling them.

Thank you.

Consider the following scenario, an application among various parameters takes a parameter called "deleteall". The PHP script tries to filter out calling this command of the application but wants to allow the external input to call any other sub-command.

$param = json_decode($_GET['param']);
if ($param !== 'deleteall')
{
  system('application ' . escapeshellarg($param));
}
else
{
  echo 'Action prohibited', "\n";
}


The malicious user calls script.php?param="deleteall\u0000"
The if sees that the param is not equal to "deleteall" and allows it to go through.
escapeshellarg() which is supposed to take external user input and make it safe instead takes a value which was viewed as being safe, truncates it at the NULL byte, and allows it to be malicious instead of rejecting it.

There really should be NULL byte protection on these functions.



I would add to this that php_exec_ex() used by exec(), system(), and passthru() also contains a bug in its NULL byte detection:

    if (strlen(cmd) != cmd_len) {
        php_error_docref(NULL, E_WARNING, "NULL byte detected. Possible attack");
        RETURN_FALSE;
    }

strlen() is unsafe to call on a string which is not NULL terminated. The main PHP internals are pretty good about ensuring strings are NULL terminated despite the fact that they carry length, but it's possible something has been overlooked, and when it comes to various PECL extensions, some of them fail to NULL terminate their strings.

The following should be used instead:

    if (memchr(cmd, 0, cmd_len)) {
        php_error_docref(NULL, E_WARNING, "NULL byte detected. Possible attack");
        RETURN_FALSE;
    }


It would probably be a good idea to follow this up with a check against cmd[cmd_len] to ensure it is a null byte, in case for whatever reason the string in question was not NULL terminated. Passing on a non-NULL terminated string to exec*(), popen() or whatever C function is asking for a segmentation fault, or worse.

if ($param !== 'deleteall')

This wont work, because === leads to is_identical_function() which checks for both string lengths (binary safe), and in your case, they don't match.


Second point :
Having strings that are not NULL terminated is impossible inside the engine, if you use Core extensions.
If in PECL someone doesn't create valid C strings - aka he doesn't terminate them with NULL - then we are not responsible of the problem that can happen into the engine and the bug is a PECL extension bug that the maintainer should fix.

When developing extensions in PECL, the engine checks for non-null-terminated strings and should warn the user against it.
Checks are at http://lxr.php.net/xref/PHP_5_6/Zend/zend_API.h#536

> This wont work, because === leads to is_identical_function() which checks for both string lengths (binary safe), and in your case, they don't match.

I'm not sure what you mean by "wont work". They don't match, correct, which proceeds to run: system('application ' . escapeshellarg($param)); If just system('application ' . $param) was ran, the NULL byte protection in system() would also catch this malicious input and reject it.

escapeshellarg() which is supposed to protect against unsafe external input is taking a value which would have been safe (thanks to system()'s NULL byte protection), and turns it into something malicious, "deleteall" in this case. And as you said, thanks to !== being binary safe, the user created check doesn't protect against it either as the user is not considering this side effects of escapeshellarg(), and only uses what turns out to be a naive check.

escapeshellarg() and all the related functions should either have NULL byte protection or ensure the NULL byte is not stripped out, so the follow up calls to an execution function has their NULL byte protection protect against it.

> Having strings that are not NULL terminated is impossible inside the engine

I recall that not being the case with some of the random string generation functions, at least in older versions when I was reviewing those functions. Not that anyone should be passing randomly generated string to external programs, it still looks like a bad idea to assume everything is NULL terminated when strings have an explicit length.

> If in PECL someone doesn't create valid C strings - aka he doesn't terminate them with NULL - then we are not responsible of the problem that can happen into the engine and the bug is a PECL extension bug that the maintainer should fix.

I've spoken to some developers about this when finding some bugs in this regard. I have gotten them to NULL terminate. However their claim is that PHP strings are not "C strings". They have length, and can be assigned with macros like RETURN_STRINGL (no-copy) without NULL terminating them. No warnings are printed when compiling such extensions that use RETURN_STRINGL without NULL termination. Also, documentation on using these macros in various places is lacking in this regard. The developers I've spoken with are simply unaware that everything must be NULL terminated.

Being that NULL termination is not sufficiently warned against, documented, and intuitively it seems they don't need it (they carry length), it seems like a bad idea to assume everything is NULL terminated.

@nachms to the first part. The pattern you brought is unsuitable, because it allows wildcards. It doesn't really matter whether to pass "deleteall" or anything else. When wrapping some shell command allowing access from web, this pattern is insecure per se. Even naming the _GET variables same way as commands is bad. It doesn't regard to any concrete programming language, it's a programmers choice. What I would suggest instead is like

switch ($command) {

    case "delete":
        /* do delete command */
       break;

    case "some_other":
       /* do other command */
       break

......................      

    default:

      /* unknown command */
}

Doing "if (!my_bad_command)" wildcards any possible stuff. Variables SHOULD explicitly match the input. Especially with some "deleteall" - whitelisting is the only feasible way. Bad coding is always bad coding.

With changing the exec API to be binary safe - well, is this going to fix the mistakes like non \0 termitation? Like passing wrong length for example. But if done, it will be a BC break that should be really justified. Some functions in the API are binary safe, some not.

I'm personally afraid that changing the current APIs will be just exchanging one can of bugs with another one. What I personally could imagine - if we had a decision that exceptions in the functions were allowed in any functions, then the binary safe API for exec would help to determine that there were a \0 in a command and throw then (or alike example).

Thanks.

ab, you're dancing around the issue. One could easily "fix" the above case by checking the value of $param *after* it was ran through escapeshellarg(). Whether the PHP developer is using a whitelist/backlist or what the exact code structure is is besides the point.

The issue at hand is that escapeshellarg() (and related functions) can take parameters which within whatever their existing structure was safe, and turn them into something malicious. Developers are not going to take this into account. The existing documentation *does not say it truncates at NULL*, developers are not going to consider that. They are expecting this function to take any variables which are external, and magically make them safe. Pre-existing code here is exploitable without so much as a NOTICE appearing in the logs.

Regarding backwards compatibility, perhaps this change should only be for PHP 7+, which is new anyway and most developers even haven't began porting to yet. Developers have to test everything anew anyhow, and see what the important changes are.

In any case, the exec API was just recently changed (in 5.6 as well) to have NULL byte protection. Which means existing code which called exec and used to do something now has it instead returning false with a warning (and therefore backwards compatibility was just broken anyway). I don't see why altering escapeshellarg() to just not strip the NULL and trigger that behavior is going to be any less backwards compatible than what is currently being done for those that fail to call escapeshellarg() in the first place.


Regarding non-NULL termination, I would change any code I ran across to be binary-safe. It's just all around a better idea considering the PECL extensions I've seen (and the lack of documentation telling new developers to NULL terminate everything). So putting in the memchr() change I suggested doesn't hurt anything and only improves the code, taking the next step towards binary safety on top of the recent changes in the exec family of functions. Some implementations of memchr() may even be faster than strlen() due to it having a known size up-front to work with.

If you have an example of a function into the Engine/Core, that generates a zend_string which is not NULL terminated , I'm really curious to see where.

This is normally impossible, because the string destructor checks for the trailing \0, and if not here, bails out the engine.
This is at http://lxr.php.net/xref/PHP_5_6/Zend/zend_variables.c#31

So into the engine, any string should be NULL terminated.
Eventually external PECL developpers can get rid of this check, but at their own risks.

I'm +1 to turn php_escape_shell_arg() into binary safe function , but that is a PHP_API function and this will break ABI thus should wait for next minor version

I vaguely recall the mcrypt IV generation function did not NULL terminate (nor would it really have a reason to). I'd have to recheck the code though to see if that's still the case today. From analysis a colleague of mine did, he also said it may be possible for some networking code to build a non NULL terminated string, since much of the code in question is expecting other pieces of code to do it in some cases. However he did not yet do full branch coverage to ensure that every branch is indeed safe. He remarked that it does look like a disaster waiting to happen as any code changes performed there may be unaware whether they need to NULL terminate themselves, or if a caller higher up will ensure it or not.

As to CHECK_ZVAL_STRING_REL(zvalue);, perhaps something about it is lacking? I've seen PECL extensions which call "data = emalloc(size);", fill the array without any \0 values, and then call "RETURN_STRINGL(data, size, 0);". No warnings are seen during compile or run-time.

@nachms, Clear there can be some console commands that written an unfortunate way. I just don't really see a solution in your suggestion. We cannot see, what is harmful in an operation. Say they pass delete\0all, then how it is being done now, on one system - it could be ok, on another not. IMHO it doesn't really make sense to choose one strategy on how to handle it over another, because we cannot predict the impact on the concrete system. So instead of dancing around how to split/glue that \0 terminated string, probably just detecting a \0 and throwing an exception were IMO the way to go (but only if it's allowed for functions, and this ticket looks like a perfect case for that).

About the API - of course I was only talking about the C API. The userspace functions will get the strings like hello\0world anyway as "hello\0". You should probably have checked this, @nachms. But please debug the cases you bring to discussion to have concrete factual material.

It's always so that an API defines something, and consumers use it. Using \0 terminated strings is the choice of the current API. Needs of different extensions, external dependency libraries, user preferences and coding style in the PECL extensions are a completely async thing. Wrong usages of the API are wrong usages. Things you present like returning non \0 terminated strings are run time issues. They can be detected either that way (and they are catched in debug builds) or with a static analyzer.

A general discussion about the API is IMHO not required here. @nachms, but if you're in the mood, you can go for an RFC about improving the APIs. What I take from the current discussion - the internal shell/exec APIs could be made binary safe.

Thanks.

ab, you really do not see that escapeshellcmd() circumventing the NULL byte protection in the other exec functions as a problem? Or that the behavior is undocumented?

If you really believe that NULL bytes here are not an issue, then why did PHP even bother adding NULL byte detection to some of the exec functions?

I would buy your argument that perhaps "we cannot predict the impact on the concrete system" if PHP in the most recent releases didn't just add NULL byte protection to some of the functions.

Further, a function with "escape" in its name and is documented to be used on all external input doing something other than "escaping" or checking for unsafe external input is counter-intuitive enough that you can at least warrant it's a documentation bug.

Beyond that I don't understand what you're saying. I'm seeing real problems in real code, I've given you examples. "userspace functions will get the strings like hello\0world anyway as "hello\0"" is simply not true anymore, as the userspace function won't even be called with hello\0world, as PHP returns false in that case with a warning. 

As to the general discussion regarding APIs, I'm not saying you need to change anything, although actually documenting NULL requirements would be helpful, because developers are simply  unaware of the situation. However, while you're fixing up these functions with the \0 handling (with an exception if that makes the most sense), you might as well put in the memcmp() vs. strlen() change, as it hurts nothing, and it can be done faster than it takes to write and review an RFC anyway.

@nachms, I think we talk a bit past each other. Please check this internal argument parsing API https://github.com/php/php-src/blob/master/README.PARAMETER_PARSING_API , and please check the internal code of the escapeshellarg() function. Currently, the argument hello\0world there will arrive in that function through zend_parse_parameters("%s",...) which will stop on the first null byte. So it'll be hello\0. We could change it to "%z", to read it binary safe, then it could look like this

diff --git a/ext/standard/exec.c b/ext/standard/exec.c
index 8dd0d5d..900d22b 100644
--- a/ext/standard/exec.c
+++ b/ext/standard/exec.c
@@ -432,15 +432,18 @@ PHP_FUNCTION(escapeshellcmd)
    Quote and escape an argument for use in a shell command */
 PHP_FUNCTION(escapeshellarg)
 {
-       char *argument;
-       size_t argument_len;
+       zval *argument;

-       if (zend_parse_parameters(ZEND_NUM_ARGS(), "s", &argument, &argument_len) == FAILURE) {
+       if (zend_parse_parameters(ZEND_NUM_ARGS(), "z", &argument) == FAILURE) {
                return;
        }

        if (argument) {
-               RETVAL_STR(php_escape_shell_arg(argument));
+               convert_to_string(argument);
+               if (ZSTR_LEN(Z_STR_P(argument)) != strlen(ZSTR_VAL(Z_STR_P(argument)))) {
+                       RETURN_FALSE;
+               }
+               RETVAL_STR(php_escape_shell_arg(ZSTR_VAL(Z_STR_P(argument))));
        }
 }
 /* }}} */

I wouldn't suggest to judge what \0 stands for, but just bail out. Otherwise, do you know the best way to "escape" \0 in general? 7.0 and below should just give false back. If any later PHP were allowed, an exception should be thrown. Principally, the logic could be moved into the underlying internal functions, which possibly would require a signature change and thus a BC breach.

The null termination cases you talk about, they was always solving a concrete issue. IMHO it is the way to go with any upcoming issues - best BC if possible, best issue solving. Everyone is human, all the vulnerabilities cannot be predicted. At some point it just sounded that you talk about the APIs in general. Please lets stay on the concrete issue. Given the current string APIs, I personally don't think it is much worse than anything else. But having a change in general APIs would mean a wide BC breach and thus a wider discussions.

Now, do you think the approach I've suggested as exemplary would work? I yet didn' tcheck it can be backported to 5.

Thanks.

Hi ab.

I think your proposed changes to escapeshellcmd() is fine (and should also be applied to the other functions I listed in my first post). However, users will need to be made aware to check the return value for false.

As to escaping \0, one option as I've been saying before is to just leave it in along with whatever follows it (no truncation). This would ensure that follow up calls to exec() or system() with concatenated escaped input hit NULL byte detection in those functions and error out there. Users should already be checking return values on those functions.

Throwing an exception I believe would be the most correct procedure here if possible.


In the end any of these 3 approaches pass technical muster for a safe API. The 3rd seems the safest and the least ambiguous. From the first two, which to prefer I think boils down to whether we think users may find other uses for escapeshellcmd() other than escaping parameters for shell commands (database input?, piping commands to applications that accept instruction via stdin? network strings?). If you decide to go with your suggested patch, please apply it to the other functions too, and you have my blessing. Although I would personally change:

if (ZSTR_LEN(Z_STR_P(argument)) != strlen(ZSTR_VAL(Z_STR_P(argument)))) {

to:

if (memchr(ZSTR_VAL(Z_STR_P(argument)), 0, ZSTR_LEN(Z_STR_P(argument)))) {

Or if you like verbosity:

if (memchr(ZSTR_VAL(Z_STR_P(argument)), '\0', ZSTR_LEN(Z_STR_P(argument))) != NULL) {

This accomplishes the same thing with a safer (and potentially faster) idiom. I personally also think it's clearer as to what your intention is (does this string contain a \0 ?).

PHP strings should be null-terminated. If the string is not null-terminated, some bad extension did it, and that's where it should be fixed. A lot of places in the engine assume PHP values are null-terminated, and rewriting them all is infeasible, Inconsistently trying to clean up for broken extensions in random places does not sound like a good idea to me.

Proposed patch: https://gist.github.com/weltling/bb7a27f166c339e6332d