From 5cbdd5f6dea8ae27c6907f16aff7929c5946c7de Mon Sep 17 00:00:00 2001 From: "Christoph M. Becker" Date: Wed, 4 Dec 2024 15:04:48 +0100 Subject: [PATCH] Harden proc_open() 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. 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. [1] Closes GH-17043. --- NEWS | 3 + ext/standard/Makefile.frag.w32 | 4 ++ ext/standard/proc_open.c | 59 ++++++++++++++++++- .../general_functions/proc_open_cmd.phpt | 28 +++++++++ ext/standard/tests/helpers/bad_cmd.c | 7 +++ win32/build/Makefile | 3 +- 6 files changed, 101 insertions(+), 3 deletions(-) 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/NEWS b/NEWS index 547e362ca28..9d72a87d0a9 100644 --- a/NEWS +++ b/NEWS @@ -15,6 +15,9 @@ PHP NEWS . Fixed bug GH-17037 (UAF in user filter when adding existing filter name due to incorrect error handling). (nielsdos) +- Windows: + . Hardened proc_open() against cmd.exe hijacking. (cmb) + 19 Dec 2024, PHP 8.3.15 - Calendar: diff --git a/ext/standard/Makefile.frag.w32 b/ext/standard/Makefile.frag.w32 index 7815c7c97d0..f4655cfa8bb 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/proc_open.c b/ext/standard/proc_open.c index 2d4cb42b7a6..ce771098277 100644 --- a/ext/standard/proc_open.c +++ b/ext/standard/proc_open.c @@ -701,22 +701,77 @@ 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 { + /* the first call to SearchPathW() fails if the buffer is too small, + * what is unlikely but possible; to avoid an explicit second call to + * SeachPathW() and the error handling, we're looping */ + 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 new file mode 100644 index 00000000000..6c80871d47d --- /dev/null +++ b/ext/standard/tests/general_functions/proc_open_cmd.phpt @@ -0,0 +1,28 @@ +--TEST-- +Harden against cmd.exe hijacking +--SKIPIF-- + +--FILE-- + 0) { + foreach ($read as $stream) { + fpassthru($stream); + } +} +?> +--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 00000000000..05de19f5686 --- /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 9d4c1c0800d..1b200502119 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