Bug #73342 [Ver]: Vulnerability in php-fpm by changing stdin to non-blocking

Edit report at https://bugs.php.net/bug.php?id=73342&edit=1 ID: 73342 Updated by: bukka@php.net Reported by: xuavis at gmail dot com Summary: Vulnerability in php-fpm by changing stdin to non-blocking Status: Verified Type: Bug Package: FPM related Operating System: Ubuntu 16.04 PHP Version: 7.0Git-2016-10-18 (Git) Assigned To: bukka Block user comment: N Private report: N New Comment: @nikic I think that the reason why FPM cares about STDIN is to conform FastCGI spec - namely section 2.2 ( http://www.mit.edu/~yandros/doc/specs/fcgi-spec.html#S2.2 ) that states "FCGI_LISTENSOCK_FILENO equals STDIN_FILENO". I guess it's because maybe some FastCGI application could access the data directly from the record but not really sure why. I can't really see the reason for that in the PHP case. I don't see any side effects of the patch at the moment but it might need a bit more thinking and testing. Previous Comments: ------------------------------------------------------------------------ [2018-02-23 21:10:52] nikic@php.net Related To: Bug #67796 ------------------------------------------------------------------------ [2018-02-23 20:48:52] nikic@php.net Related To: Bug #73056 ------------------------------------------------------------------------ [2018-02-23 20:41:40] nikic@php.net I think the patch from the first comment only works around the problem ... the real question is this: Why does FPM care about STDIN at all? After some looking around, this seems to be what happens: * wp->listening_socket is what we actually want to listen on. * fpm_globals.listening_socket is always 0 (effectively STDIN), because that's what the global is initialized to. It's never changed. * fpm_run() always returns fpm_globals.listening_socket and that's what fcgi listens on. * to make things line up fpm_stdio_init_child() does a dup2(wp->listening_socket, STDIN_FILENO). So effectively we take the listening socket, dup2 it to STDIN and then listen on STDIN. The following patch removes the indirection through STDIN: diff --git a/sapi/fpm/fpm/fpm_children.c b/sapi/fpm/fpm/fpm_children.c index b48fa54..4ee316b 100644 --- a/sapi/fpm/fpm/fpm_children.c +++ b/sapi/fpm/fpm/fpm_children.c @@ -146,6 +146,7 @@ static struct fpm_child_s *fpm_child_find(pid_t pid) /* {{{ */ static void fpm_child_init(struct fpm_worker_pool_s *wp) /* {{{ */ { fpm_globals.max_requests = wp->config->pm_max_requests; + fpm_globals.listening_socket = dup(wp->listening_socket); if (0 > fpm_stdio_init_child(wp) || 0 > fpm_log_init_child(wp) || diff --git a/sapi/fpm/fpm/fpm_stdio.c b/sapi/fpm/fpm/fpm_stdio.c index 4072017..76e8b32 100644 --- a/sapi/fpm/fpm/fpm_stdio.c +++ b/sapi/fpm/fpm/fpm_stdio.c @@ -103,12 +103,6 @@ int fpm_stdio_init_child(struct fpm_worker_pool_s *wp) /* {{{ */ fpm_globals.error_log_fd = -1; zlog_set_fd(-1); - if (wp->listening_socket != STDIN_FILENO) { - if (0 > dup2(wp->listening_socket, STDIN_FILENO)) { - zlog(ZLOG_SYSERROR, "failed to init child stdio: dup2()"); - return -1; - } - } return 0; } /* }}} */ This also resolves the issue for me. However, I don't know if this has any side-effects, because something else relies on the STDIN mapping. ------------------------------------------------------------------------ [2018-02-23 16:55:15] nikic@php.net Related To: Bug #70185 ------------------------------------------------------------------------ [2018-02-23 16:54:23] nikic@php.net Related To: Bug #75968 ------------------------------------------------------------------------ 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=73342 -- Edit this bug report at https://bugs.php.net/bug.php?id=73342&edit=1