From a7e32a2ba31a12631ec65c0681cf91ccbd46f16f Mon Sep 17 00:00:00 2001 From: "Christoph M. Becker" Date: Wed, 4 Dec 2024 14:45:01 +0100 Subject: [PATCH 1/2] Add test case regarding cmd.exe hijacking To be able to easily test this, we provide a minimalist C file which will be build as test_helper, and used by the new test case. --- ext/standard/Makefile.frag.w32 | 4 +++ .../general_functions/proc_open_cmd.phpt | 29 +++++++++++++++++++ ext/standard/tests/helpers/bad_cmd.c | 7 +++++ win32/build/Makefile | 3 +- 4 files changed, 42 insertions(+), 1 deletion(-) create mode 100644 ext/standard/tests/general_functions/proc_open_cmd.phpt create mode 100644 ext/standard/tests/helpers/bad_cmd.c diff --git a/ext/standard/Makefile.frag.w32 b/ext/standard/Makefile.frag.w32 index 7815c7c97d035..f4655cfa8bb27 100644 --- a/ext/standard/Makefile.frag.w32 +++ b/ext/standard/Makefile.frag.w32 @@ -7,3 +7,7 @@ ext\standard\url_scanner_ex.c: ext\standard\url_scanner_ex.re $(RE2C) $(RE2C_FLAGS) --no-generation-date -b -o ext/standard/url_scanner_ex.c ext/standard/url_scanner_ex.re $(BUILD_DIR)\ext\standard\basic_functions.obj: $(PHP_SRC_DIR)\Zend\zend_language_parser.h + +$(PHP_SRC_DIR)\ext\standard\tests\helpers\bad_cmd.exe: $(PHP_SRC_DIR)\ext\standard\tests\helpers\bad_cmd.c + cd $(PHP_SRC_DIR)\ext\standard\tests\helpers + $(PHP_CL) /nologo bad_cmd.c diff --git a/ext/standard/tests/general_functions/proc_open_cmd.phpt b/ext/standard/tests/general_functions/proc_open_cmd.phpt new file mode 100644 index 0000000000000..2c62eb4a48b7b --- /dev/null +++ b/ext/standard/tests/general_functions/proc_open_cmd.phpt @@ -0,0 +1,29 @@ +--TEST-- +Harden against cmd.exe hijacking +--SKIPIF-- + +--FILE-- + 0) { + foreach ($read as $stream) { + fpassthru($stream); + } +} +@unlink("cmd.exe"); // TODO remove when fix has been applied +?> +--EXPECTF-- +resource(%d) of type (process) +hello +--CLEAN-- + diff --git a/ext/standard/tests/helpers/bad_cmd.c b/ext/standard/tests/helpers/bad_cmd.c new file mode 100644 index 0000000000000..05de19f5686cf --- /dev/null +++ b/ext/standard/tests/helpers/bad_cmd.c @@ -0,0 +1,7 @@ +#include + +int main() +{ + printf("pwnd!\n"); + return 0; +} diff --git a/win32/build/Makefile b/win32/build/Makefile index 9d4c1c0800dc7..1b200502119a7 100644 --- a/win32/build/Makefile +++ b/win32/build/Makefile @@ -54,7 +54,7 @@ DEBUGGER_CMD= DEBUGGER_ARGS= !endif -all: generated_files $(EXT_TARGETS) $(PECL_TARGETS) $(SAPI_TARGETS) +all: generated_files $(EXT_TARGETS) $(PECL_TARGETS) $(SAPI_TARGETS) test_helpers build_dirs: $(BUILD_DIR) $(BUILD_DIRS_SUB) $(BUILD_DIR_DEV) @@ -185,6 +185,7 @@ clean-pgo: clean-all -del /f /q $(BUILD_DIR)\$(DIST_ZIP_PECL) -del /f /q $(BUILD_DIR)\$(DIST_ZIP_TEST_PACK) +test_helpers: $(PHP_SRC_DIR)\ext\standard\tests\helpers\bad_cmd.exe !if $(PHP_TEST_INI_PATH) == "" test: set-tmp-env From 9f19a8c647fcbf1b5c017badb2c84f9833682f66 Mon Sep 17 00:00:00 2001 From: "Christoph M. Becker" Date: Wed, 4 Dec 2024 15:04:48 +0100 Subject: [PATCH 2/2] Harden against cmd.exe hijacking As is, whenever `proc_open()` needs to invoke the shell, cmd.exe is looked up in the usual executable search path. That implies that any cmd.exe which is placed in the current working directory (which is not necessarily what is reported by `getcwd()` for ZTS builds), will be used. This is a known attack vector, and Microsoft recommends to always use the fully qualified path to cmd.exe. To prevent any cmd.exe in the current working directory to be used, but to still allow users to use a drop in replacement for cmd.exe, we search only the `PATH` for cmd.exe (and pass the fully qualified path to `CreateProcessW`), instead of relying on automatic executable search by passing the base name only. [1] --- ext/standard/proc_open.c | 56 ++++++++++++++++++- .../general_functions/proc_open_cmd.phpt | 1 - 2 files changed, 54 insertions(+), 3 deletions(-) diff --git a/ext/standard/proc_open.c b/ext/standard/proc_open.c index 2d4cb42b7a661..456620b9b58d6 100644 --- a/ext/standard/proc_open.c +++ b/ext/standard/proc_open.c @@ -701,22 +701,74 @@ static void init_process_info(PROCESS_INFORMATION *pi) memset(&pi, 0, sizeof(pi)); } +/* on success, returns length of *comspec, which then needs to be efree'd by caller */ +static size_t find_comspec_nt(wchar_t **comspec) +{ + zend_string *path = NULL; + wchar_t *pathw = NULL; + wchar_t *bufp = NULL; + DWORD buflen = MAX_PATH, len = 0; + + path = php_getenv("PATH", 4); + if (path == NULL) { + goto out; + } + pathw = php_win32_cp_any_to_w(ZSTR_VAL(path)); + if (pathw == NULL) { + goto out; + } + bufp = emalloc(buflen * sizeof(wchar_t)); + do { + len = SearchPathW(pathw, L"cmd.exe", NULL, buflen, bufp, NULL); + if (len == 0) { + goto out; + } + if (len < buflen) { + break; + } + buflen = len; + bufp = erealloc(bufp, buflen * sizeof(wchar_t)); + } while (1); + *comspec = bufp; + +out: + if (path != NULL) { + zend_string_release(path); + } + if (pathw != NULL) { + free(pathw); + } + if (bufp != NULL && bufp != *comspec) { + efree(bufp); + } + return len; +} + static zend_result convert_command_to_use_shell(wchar_t **cmdw, size_t cmdw_len) { - size_t len = sizeof(COMSPEC_NT) + sizeof(" /s /c ") + cmdw_len + 3; + wchar_t *comspec; + size_t len = find_comspec_nt(&comspec); + if (len == 0) { + php_error_docref(NULL, E_WARNING, "Command conversion failed"); + return FAILURE; + } + len += sizeof(" /s /c ") + cmdw_len + 3; wchar_t *cmdw_shell = (wchar_t *)malloc(len * sizeof(wchar_t)); if (cmdw_shell == NULL) { + efree(comspec); php_error_docref(NULL, E_WARNING, "Command conversion failed"); return FAILURE; } - if (_snwprintf(cmdw_shell, len, L"%hs /s /c \"%s\"", COMSPEC_NT, *cmdw) == -1) { + if (_snwprintf(cmdw_shell, len, L"%s /s /c \"%s\"", comspec, *cmdw) == -1) { + efree(comspec); free(cmdw_shell); php_error_docref(NULL, E_WARNING, "Command conversion failed"); return FAILURE; } + efree(comspec); free(*cmdw); *cmdw = cmdw_shell; diff --git a/ext/standard/tests/general_functions/proc_open_cmd.phpt b/ext/standard/tests/general_functions/proc_open_cmd.phpt index 2c62eb4a48b7b..6c80871d47df8 100644 --- a/ext/standard/tests/general_functions/proc_open_cmd.phpt +++ b/ext/standard/tests/general_functions/proc_open_cmd.phpt @@ -18,7 +18,6 @@ if (($num = stream_select($read, $write, $except, 1000)) === false) { fpassthru($stream); } } -@unlink("cmd.exe"); // TODO remove when fix has been applied ?> --EXPECTF-- resource(%d) of type (process)