Skip to content

Commit 9382673

Browse files
nielsdosramsey
authored andcommitted
The original code is error-prone due to the "best fit mapping" that happens with the argument parsing but not with the query string. When we get a non-ASCII character, try to remap it and see if it becomes a hyphen. An alternative approach is to create a custom main `wmain` receiving wide-character variations that does the ANSI transformation with the best-fit mapping, but that's more error-prone and could cause unexpected breakage. Another alternative was just don't doing this check altogether and always check for `cgi || fastcgi` instead, but that breaks real-world use-cases.
1 parent 4b15f5d commit 9382673

File tree

2 files changed

+60
-1
lines changed

2 files changed

+60
-1
lines changed

sapi/cgi/cgi_main.c

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1798,8 +1798,13 @@ int main(int argc, char *argv[])
17981798
}
17991799
}
18001800

1801+
/* Apache CGI will pass the query string to the command line if it doesn't contain a '='.
1802+
* This can create an issue where a malicious request can pass command line arguments to
1803+
* the executable. Ideally we skip argument parsing when we're in cgi or fastcgi mode,
1804+
* but that breaks PHP scripts on Linux with a hashbang: `#!/php-cgi -d option=value`.
1805+
* Therefore, this code only prevents passing arguments if the query string starts with a '-'.
1806+
* Similarly, scripts spawned in subprocesses on Windows may have the same issue. */
18011807
if((query_string = getenv("QUERY_STRING")) != NULL && strchr(query_string, '=') == NULL) {
1802-
/* we've got query string that has no = - apache CGI will pass it to command line */
18031808
unsigned char *p;
18041809
decoded_query_string = strdup(query_string);
18051810
php_url_decode(decoded_query_string, strlen(decoded_query_string));
@@ -1809,6 +1814,22 @@ int main(int argc, char *argv[])
18091814
if(*p == '-') {
18101815
skip_getopt = 1;
18111816
}
1817+
1818+
/* On Windows we have to take into account the "best fit" mapping behaviour. */
1819+
#ifdef PHP_WIN32
1820+
if (*p >= 0x80) {
1821+
wchar_t wide_buf[1];
1822+
wide_buf[0] = *p;
1823+
char char_buf[4];
1824+
size_t wide_buf_len = sizeof(wide_buf) / sizeof(wide_buf[0]);
1825+
size_t char_buf_len = sizeof(char_buf) / sizeof(char_buf[0]);
1826+
if (WideCharToMultiByte(CP_ACP, 0, wide_buf, wide_buf_len, char_buf, char_buf_len, NULL, NULL) == 0
1827+
|| char_buf[0] == '-') {
1828+
skip_getopt = 1;
1829+
}
1830+
}
1831+
#endif
1832+
18121833
free(decoded_query_string);
18131834
}
18141835

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
--TEST--
2+
GHSA-3qgc-jrrr-25jv
3+
--SKIPIF--
4+
<?php
5+
include 'skipif.inc';
6+
if (PHP_OS_FAMILY !== "Windows") die("skip Only for Windows");
7+
8+
$codepage = trim(shell_exec("powershell Get-ItemPropertyValue HKLM:\\SYSTEM\\CurrentControlSet\\Control\\Nls\\CodePage ACP"));
9+
if ($codepage !== '932' && $codepage !== '936' && $codepage !== '950') die("skip Wrong codepage");
10+
?>
11+
--FILE--
12+
<?php
13+
include 'include.inc';
14+
15+
$filename = __DIR__."/GHSA-3qgc-jrrr-25jv_tmp.php";
16+
$script = '<?php echo "hello "; echo "world"; ?>';
17+
file_put_contents($filename, $script);
18+
19+
$php = get_cgi_path();
20+
reset_env_vars();
21+
22+
putenv("SERVER_NAME=Test");
23+
putenv("SCRIPT_FILENAME=$filename");
24+
putenv("QUERY_STRING=%ads");
25+
putenv("REDIRECT_STATUS=1");
26+
27+
passthru("$php -s");
28+
29+
?>
30+
--CLEAN--
31+
<?php
32+
@unlink(__DIR__."/GHSA-3qgc-jrrr-25jv_tmp.php");
33+
?>
34+
--EXPECTF--
35+
X-Powered-By: PHP/%s
36+
Content-type: %s
37+
38+
hello world

0 commit comments

Comments
 (0)