[Bug] Potential buffer overflow in php_cli_server_startup_workers

[yiyuaner]

Description

In the file sapi/cli/php_cli_server.c, the function php_cli_server_startup_workers has the following code:

void php_cli_server_startup_workers(void) {
    char *workers = getenv("PHP_CLI_SERVER_WORKERS");
    if (!workers) {
        return;
    }
    php_cli_server_workers_max = ZEND_ATOL(workers); 
    
    if (php_cli_server_workers_max > 1) { 
        php_cli_server_workers = calloc(
			php_cli_server_workers_max, sizeof(pid_t));
        ...
    }
}

The variable php_cli_server_workers_max is parsed from environment variable and thus is controlled. When setting php_cli_server_workers_max to a large value (e.g., INT64_MAX), the multiplication php_cli_server_workers_max * sizeof(pid_t) could wrap to a small value. A buffer smaller than expected will be allocated and this can lead to subsequent buffer overflow.

Notice that the C standard does not clearly states that calloc will check for multiplication overflow itself (see here). It will be better to also restrict the maximum value for php_cli_server_workers_max in the code.

PHP Version

github master

Operating System

No response

[devnexen]

It will be better to also restrict the maximum value for php_cli_server_workers_max in the code.

strtoll here is, at least, a better fit in conjunction with ranges check (or using a wrapper "a la" strtonum).

[cmb69]

I agree that this should be improved, but setting a very large number is unlikely to work anyway, or is it?

[yiyuaner]

I agree that this should be improved, but setting a very large number is unlikely to work anyway, or is it?

With a very large number for php_cli_server_workers_max, the actual buffer size of php_cli_server_workers is actually a small value due to the wrap-around. Then the following loop can lead to out of bound buffer access (only require fork to succeed for a small number of times):

for (php_cli_server_worker = 0;
       php_cli_server_worker < php_cli_server_workers_max;
       php_cli_server_worker++) {
    pid_t pid = fork();
    if (pid < 0) {
	/* no more forks allowed, work with what we have ... */
	php_cli_server_workers_max =
	php_cli_server_worker + 1;
	return;
    } else if (pid == 0) {
	return;
    } else {
	php_cli_server_workers[php_cli_server_worker] = pid;
    }
}

[devnexen]

@yiyuaner Can you provide a PR ? if that's the case feel free ;-)