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] <https://msrc.microsoft.com/blog/2014/04/ms14-019-fixing-a-binary-hijacking-via-cmd-or-bat-file/>

Closes GH-17043.

Author: cmb69

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:

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

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;
 

ext/standard/tests/general_functions/proc_open_cmd.phpt

@@ -0,0 +1,28 @@
+--TEST--
+Harden against cmd.exe hijacking
+--SKIPIF--
+<?php
+if (PHP_OS_FAMILY !== "Windows") die("skip only for Windows");
+?>
+--FILE--
+<?php
+copy(__DIR__ . "/../helpers/bad_cmd.exe", "cmd.exe");
+$spec = [["pipe", "r"], ["pipe", "w"], ["pipe", "w"]];
+var_dump($proc = proc_open("@echo hello", $spec, $pipes, null));
+$read = [$pipes[1], $pipes[2]];
+$write = $except = null;
+if (($num = stream_select($read, $write, $except, 1000)) === false) {
+    echo "stream_select() failed\n";
+} elseif ($num > 0) {
+    foreach ($read as $stream) {
+        fpassthru($stream);
+    }
+}
+?>
+--EXPECTF--
+resource(%d) of type (process)
+hello
+--CLEAN--
+<?php
+@unlink("cmd.exe");
+?>

ext/standard/tests/helpers/bad_cmd.c

@@ -0,0 +1,7 @@
+#include <stdio.h>
+
+int main()
+{
+	printf("pwnd!\n");
+	return 0;
+}

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