Hello,

I'm planning to make the PoC exploit for this bug public on Tuesday. Is it OK to you?

We would certainly appreciate more time for looking into this. Is there any reason why you need to publicly disclose this 5 days after notification? 

I understand the issue required misconfigured FPM but if this is a common occurrence then it'd be nice to hold till the next release which fixes the issue if possible.

Well, the date can be rescheduled, I just want to get something about this. What is this next release date you're talking about?

Second, I believe this issue requires a CVE. The configuration required to reproduce the issue (which is uee fastcgi_split_pathinfo in nginx config with cgi.fix_pathinfo = 1) is not that uncommon. What is the process of getting it?

Thanks for reporting this issue!

CVEs are preferrably issued by php.net[1].  The next security
relevant releases are scheduled for 24 Okt 2019.

Does this issue affect PHP 7.1, 7.2 and/or 7.3?

[1] <https://wiki.php.net/cve>

Well, I think that 24 October is too far. Please note that a task with a similar configuration was present on a Real World CTF at the beginning of September (ctf is a competition where teams hack things). While the issue was not the intended way to solve for the task, there might be other people who have noticed strange behavior and was able to understand what happens.

For now, let's define 24 October as a strict deadline. However, I would appreciate it if you allow disclosing the issue earlier.

My exploit works on all 7+ versions, but the core issue seems to exists since the mentioned code was written. I was able to reproduce the crash even on php 5.6.

More details on this:

The code that wrongly assumes that env_path_info is not empty was written in 2013, according to git blame. That means that out-of-bound read and out-of-bound write of a single byte exist in virtually all versions.

However, my exploit uses the presence of _fcgi_data_seg structure and related hash table optimization. It is here since 7.0. So 7.1, 7.2 and 7.3 all have both the issue and a way exploit for it (which is described in the first message).

> Well, I think that 24 October is too far.

Too far for what? Are you on some kind of deadline? What is the nature of that deadline? Is this problem exploited in the wild? Can you provide more information about it? If we waited to October 24, what problem would happen?
We had emergency releases before, but for that we'd need to have strong justification, From your description I conclude that the issue can not happen unless FPM is misconfigured in a way that essentially makes it broken (i.e., PATH_INFO is empty) - something that somebody configuring their own server would likely notice? But I may be wrong, I am just trying so far to evaluate how common is such misconfiguration and assess the urgency of the fix from this. CTF contests are fun, but these are specifically set up to be broken into, and our primary interest is assessing danger to real world production users. 

Thanks again for your report. I have assigned CVE-2019-11043 for it.

I will try to answer your questions in the most detail.

First, while the mentioned misconfiguration is somewhat uncommon, it is not that uncommon, and it does not require the site to be completely broken. As I wrote in the report, the core thing that allows empty PATH_INFO is `fastcgi_split_path_info` directive, which has nothing wrong by itself. However, it usually contains a regexp that can be broken by an attacker by supplying %0a in the path info (that is the crucial thing here!). The $fastcgi_path_info will remain empty in this case, and the thing that usually follows, which is `fastcgi_param PATH_INFO $fastcgi_path_info;`, will set PATH_INFO to an empty value.

A website with such configuration won't seem broken as %0a isn't common in URL paths. Everything will be working as usual.

Of course, there are factors that can prevent exploitation even if fastcgi_split_path_info is present. For example, if nginx and php-fpm share the same filesystem, one can first check the script actually exists, so the malicious request will never make it to php-fpm (it is usually done using something like `try_files $uri =404`). Or there may be cgi.fix_pathinfo=0 --- it will also prevent exploitation. Both settings are present in a lot of config snippets back from the days when php-fpm didn't check extensions of the scripts, but now it does, so some more recent configs lack the checks.

Second, the impact of the vulnerability is pretty significant. Using the overflow, I was able to add fast cgi environment variables for the request, and PHP_VALUE is a very precious one. There is a limitation --- you can't supply values longer than 23 bytes --- however, I was able to get code execution under this condition (and that is pretty tricky stuff by itself, but it is another story).

Third, there's a misunderstanding about the CTF thing.

> CTF contests are fun, but these are specifically set up to be broken into, and our primary interest is assessing danger to real world production users.

As I wrote, the challenge was not about nginx/php-fpm misconfiguration at all (it was a client-side challenge actually). However, the one who configured nginx for the challenge made the configuration mistake mentioned earlier. I'm sure it was accidental, the challenge author probably just copy-pasted nginx config from somewhere.

So, the fact that the vulnerable config was present on a CTF doesn't make the issue like "someone need to intentionally break his own server to make the exploit work", it is more like a (very) weak evidence that the issue is severe, like "someone probably copy-pasted vulnerable config once, so there are vulnerable snippets around".

But the reason why I mentioned the CTF was not this. Our research started by noticing the strange behavior of $_SERVER variables after randomly inserting %0a in the URL. Maybe someone in some other team did the same and investigated it in the same way.

Summing up the CTF thing:
1. Someone wanted to make a challenge for a CTF. She chooses nginx + php-fpm for its backend, but the challenge itself was not related to PHP and could be done in any language.
2. A config for nginx was needed, so she (presumably) took one of nginx config snippets (like from "configure nginx + php-fpm" google query), which appeared to suffer from the flaw above.
3. One of the guys I mentioned in my report just %0a randomly in the url and noticed the strange values of some $_SERVER variables.

I hope it is clear now that using the vulnerability was not one of the intended ways to solve the challenge (at least, it seems very unlikely).

So, recapping everything above, my desire to fix this as soon as possible is based on three things:
1. Uncommon, but not that uncommon configuration.
2. Big impact.
3. The mentioned configuration was around lots of security folk recently, and maybe someone found the same thing.

From the tone of the previous comment I've concluded that I've caused some irritation — this was not my intention at all. Obviously, the final decision on the disclosure date is made on your side. If you say October 24th — October 24th it is.

I admit intentionally writing kind of "pushing" comments; however, I'm not trying to push you towards something like an emergency update, I just think the issue deserves to be analyzed.

Jakub, could you please have a look at this?

Will try find some time over the weekend for this!

I have been looking into it and looks quite nasty. I think something like this (based on PHP-7.1 branch) should probably fix it:

diff --git a/sapi/fpm/fpm/fpm_main.c b/sapi/fpm/fpm/fpm_main.c
index 24a7e5d56a..50f92981f1 100644
--- a/sapi/fpm/fpm/fpm_main.c
+++ b/sapi/fpm/fpm/fpm_main.c
@@ -1209,8 +1209,8 @@ static void init_request_info(void)
                                                                path_info = script_path_translated + ptlen;
                                                                tflag = (slen != 0 && (!orig_path_info || strcmp(orig_path_info, path_info) != 0));
                                                        } else {
-                                                               path_info = env_path_info ? env_path_info + pilen - slen : NULL;
-                                                               tflag = (orig_path_info != path_info);
+                                                               path_info = (env_path_info && pilen > slen) ? env_path_info + pilen - slen : NULL;
+                                                               tflag = path_info && (orig_path_info != path_info);
                                                        }
 
                                                        if (tflag) {

However it's just based on code logic. It means it's not really tested. I started looking to creating a test but need to figure out what exactly nginx sends. Tried with
        [
            'SCRIPT_FILENAME'   => $uri,
            'SCRIPT_NAME'       => $uri . '/abcd',
            'PATH_INFO'         => '',
        ]

but it didn't crash so will need to try it with nginx and gdb or wireshark to get all fcgi params. If anyone can get all fcgi params causing a crash, that would be really helpful as I don't have much free time...

Formatting is not exactly great in the comment so attached it as a patch in case someone wants to try it.

Just a note that it's not ready for merging as it doesn't have a test...

From your comment, it looks like you're sending correct values. Please note that the length matters here: $uri must be about 2000 bytes to make the crash happen. The reason is the following:

The nearly only place where the crash can happen is here:

https://github.com/php/php-src/blob/master/sapi/fpm/fpm/fpm_main.c#L1150

the first byte of the path_info points outside of the string because of the bug.

However, most of the time it will point to the region of memory obtained with the same allocation, so the crash won't happen even under ASAN. Names and values of FastCGI variables are stored in this structure:

https://github.com/php/php-src/blob/master/main/fastcgi.c#L186

The "data" field here will be actually 4096 bytes long, and both names and values of FastCGI variables are stored in it like this: | name_1 | 00 | value_1 | 00 | ...

If there's no room left for a name or value, a new _fcgi_data_seg is created:

https://github.com/php/php-src/blob/master/main/fastcgi.c#L312

To make the crash happen, you need the name and value of PATH_INFO to be stored precisely at the beginning of a _fcgi_data_seg. Otherwise, the underflowed path_info pointer will point to the previous variable, which is SCRIPT_NAME (both in your example and in nginx). You cannot make it point before SCRIPT_NAME: to get slen bigger, you need to make SCRIPT_NAME longer, so path_info will actually point to the same byte of SCRIPT_NAME.

Please note that the total length of all previous variables obviously matters. That means that e.g., the length of the filesystem path to the script must be considered. I'm just bruting QUERY_STRING length to get PATH_INFO placed precisely at the beginning of a _fcgi_data_seg.

Also, "/asdf" is too short for path info. Even if you have PATH_INFO value placed at the beginning of a segment, you need to jump over the header of the _fcgi_data_seg structure. That means you need to send longer path info to get the crash.

I think I can just dump the full set of the fastcgi variables using some printf's if you need it (but there will be some long ones). However, I'm not sure this is good for a test: it will be relying not only on the code related to the bug but also on the hash table code in fastcgi.c. If, for example, you change the length of the _fcgi_data_seg's data length to another value, the crash won't happen even if the bug returns.

Thanks for the reply. It makes a bit more sense. I was thinking about having a test that just confirms that the issue got fixed. It means crashes before the fix and does not crash afterwards.

The tester script allows customizing the variables and I could modify the currently default ones:

https://github.com/php/php-src/blob/9b28553d4581f21e8e788ad60fc9948565093ad8/sapi/fpm/tests/tester.inc#L516-L534

The length should not be an issue as we could just use str_repeat for some values so it should be still relatively clean in the test.

Do you still think it might not be a good test? If so, do you have any suggestion for a good test?

No, currently I don't have any ideas on making the test more reliable. However, I was able to reproduce the crash using the test suite --- hope it helps. The patch indeed fixes it.

I'm using the following set of variables (did it by locally modifying tester.inc):

[
    'GATEWAY_INTERFACE' => 'FastCGI/1.0',
    'REQUEST_METHOD'    => 'GET',
    'SCRIPT_FILENAME'   => $uri."/".str_repeat('A', 35),
    'SCRIPT_NAME'       => $uri,
    'QUERY_STRING'      => $query,
    'REQUEST_URI'       => $uri . ($query ? '?'.$query : ""),
    'DOCUMENT_URI'      => $uri,
    'SERVER_SOFTWARE'   => 'php/fcgiclient',
    'REMOTE_ADDR'       => '127.0.0.1',
    'REMOTE_PORT'       => '7777',
    'SERVER_ADDR'       => '127.0.0.1',
    'SERVER_PORT'       => '80',
    'SERVER_NAME'       => php_uname('n'),
    'SERVER_PROTOCOL'   => 'HTTP/1.1',
    'DOCUMENT_ROOT'     => __DIR__,
    'CONTENT_TYPE'      => '',
    'CONTENT_LENGTH'    => 0,
    'HTTP_HUI'          => str_repeat('PIZDA', 1000),
    'PATH_INFO'         => '',
],

Changes are: SCRIPT_FILENAME has an appended suffix, HTTP_HUI and PATH_INFO have been added.

Things to note:
1. There was a mistake in all recent comments; the path_info should be appended to SCRIPT_FILENAME, not to SCRIPT_NAME.
2. There's a "request header" above right before PATH_INFO. The length of its' value is long enough to make fcgi_hash_strndup create new _fcgi_data_seg both for the said value and the following PATH_INFO (name and value).
3. The path info (which is appended to SCRIPT_FILENAME) is exactly 35 bytes long. This number is calculated as follows: size of _fcgi_data_seg's header is 3 * 8 = 24 bytes, length of string "PATH_INFO" (which is the variable name and is also stored inside _fcgi_data_seg) is 9 bytes plus the final zero byte, and an additional byte is needed to make path_info variable point right before the _fcgi_data_seg to make ASAN scream.

This test is still not very nice as it relies on the order of variables, length of _fcgi_data_seg, etc. However, if you just need a test that don't pass without the patch, it is it. The error message is "Not in white list. Check listen.allowed_clients.", but I believe it is always shown in case of incorrect FastCGI reply.

It looks like to make the set of the variables above a regular test, we need to implement path_info support in Tester::request. Currently, it doesn't call makeFile if $uri is supplied and doesn't allow to append path_info otherwise.

Thanks for the info about the variables needed for the test. I managed to see the issue and create a test for it. Also thanks for confirming the fix. It also fixed the failing test.

I have attached the full patch. It's ready to be applied to 7.1!

Stas, are you ok to handle it from here?

Thanks

As the fixes have been released, I've published the exploit. It is available at https://github.com/neex/phuip-fpizdam

A method to quick fix this problem.
If you want to use PATH_INFO in php, and do not want to patch and recompile PHP.

Add this line before ALL YOUR "location ~ \.php(/|$) {" LINES in nginx confs:
```
rewrite ^(.*?)\n $1;  #Fix CVE-2019-11043 (THIS LINE!!!)
location ~ \.php(/|$) {
  ...
  fastcgi_split_path_info ^((?U).+\.php)(/?.+)$;
  ...
```

That will truncate PATH_INFO after "\n" while URL contains "%0a".

As fpm_main.c is based on sapi/cgi/cgi_main.c, could it be that the same vulnerability exists in the CGI sapi too?

This pointer arithmetics looks very similar:
https://github.com/php/php-src/blob/master/sapi/cgi/cgi_main.c#L1293

> As fpm_main.c is based on sapi/cgi/cgi_main.c,
> could it be that the same vulnerability exists in the CGI sapi too?

FWIW I did a quick test with FCGI using:
USER=www-data PATH=/usr/bin PHP_FCGI_CHILDREN=1 PHP_FCGI_MAX_REQUESTS=10 valgrind /usr/bin/php5-cgi -b 127.0.0.1:9000
(same nginx conf)
and could not reproduce the crash.

I'd welcome other eyeballs on this though.

There are better workarounds for this on the Nginx level than the one presented in an earlier comment.

The simplest is to conditionally set PATH_INFO if it's not empty:

fastcgi_param PATH_INFO $fastcgi_path_info if_not_empty;

In my tests, that successfully prevents an attack.

Another option is to explicitly test whether the FCGI script path exists; this is useful anyway because otherwise, PHP-FPM will produce the 404 error page, which isn't customizable:

if (!-f $document_root$fastcgi_script_name) {
	# check if the script exists
	# otherwise, /foo.jpg/bar.php would get passed to FPM, which wouldn't run it as it's not in the list of allowed extensions, but this check is a good idea anyway, just in case
	return 404;
}