From e3c784f2bfb6029b49d27783b2efc87ee6923f79 Mon Sep 17 00:00:00 2001 From: Jakub Zelenka Date: Thu, 15 Feb 2024 23:53:49 +0000 Subject: [PATCH 1/4] Add proc_open escaping for cmd file execution --- ext/standard/proc_open.c | 86 +++++++++++++++++-- .../ghsa-pc52-254m-w9w7_1.phpt | 29 +++++++ .../ghsa-pc52-254m-w9w7_2.phpt | 29 +++++++ .../ghsa-pc52-254m-w9w7_3.phpt | 29 +++++++ 4 files changed, 168 insertions(+), 5 deletions(-) create mode 100644 ext/standard/tests/general_functions/ghsa-pc52-254m-w9w7_1.phpt create mode 100644 ext/standard/tests/general_functions/ghsa-pc52-254m-w9w7_2.phpt create mode 100644 ext/standard/tests/general_functions/ghsa-pc52-254m-w9w7_3.phpt diff --git a/ext/standard/proc_open.c b/ext/standard/proc_open.c index 3f8eaafd6d2..8aae5407265 100644 --- a/ext/standard/proc_open.c +++ b/ext/standard/proc_open.c @@ -492,11 +492,32 @@ static void append_backslashes(smart_str *str, size_t num_bs) } } -/* See https://docs.microsoft.com/en-us/cpp/cpp/parsing-cpp-command-line-arguments */ -static void append_win_escaped_arg(smart_str *str, zend_string *arg) +const char *special_chars = "()!^\"<>&|%"; + +static bool is_special_character_present(const zend_string *arg) +{ + for (size_t i = 0; i < ZSTR_LEN(arg); ++i) { + if (strchr(special_chars, ZSTR_VAL(arg)[i]) != NULL) { + return true; + } + } + return false; +} + +/* See https://docs.microsoft.com/en-us/cpp/cpp/parsing-cpp-command-line-arguments and + * https://learn.microsoft.com/en-us/archive/blogs/twistylittlepassagesallalike/everyone-quotes-command-line-arguments-the-wrong-way */ +static void append_win_escaped_arg(smart_str *str, zend_string *arg, bool is_cmd_argument) { size_t num_bs = 0; + bool has_special_character = false; + if (is_cmd_argument) { + has_special_character = is_special_character_present(arg); + if (has_special_character) { + /* Escape double quote with ^ if executed by cmd.exe. */ + smart_str_appendc(str, '^'); + } + } smart_str_appendc(str, '"'); for (size_t i = 0; i < ZSTR_LEN(arg); ++i) { char c = ZSTR_VAL(arg)[i]; @@ -510,18 +531,71 @@ static void append_win_escaped_arg(smart_str *str, zend_string *arg) num_bs = num_bs * 2 + 1; } append_backslashes(str, num_bs); + if (has_special_character && strchr(special_chars, c) != NULL) { + /* Escape special chars with ^ if executed by cmd.exe. */ + smart_str_appendc(str, '^'); + } smart_str_appendc(str, c); num_bs = 0; } append_backslashes(str, num_bs * 2); + if (has_special_character) { + /* Escape double quote with ^ if executed by cmd.exe. */ + smart_str_appendc(str, '^'); + } smart_str_appendc(str, '"'); } +static inline int stricmp_end(const char* suffix, const char* str) { + size_t suffix_len = strlen(suffix); + size_t str_len = strlen(str); + + if (suffix_len > str_len) { + return -1; /* Suffix is longer than string, cannot match. */ + } + + /* Compare the end of the string with the suffix, ignoring case. */ + return _stricmp(str + (str_len - suffix_len), suffix); +} + +static bool is_executed_by_cmd(const char *prog_name) +{ + /* If program name is cmd.exe, then return true. */ + if (_stricmp("cmd.exe", prog_name) == 0 || _stricmp("cmd", prog_name) == 0 + || stricmp_end("\\cmd.exe", prog_name) == 0 || stricmp_end("\\cmd", prog_name) == 0) { + return true; + } + + /* Find the last occurrence of the directory separator (backslash or forward slash). */ + char *last_separator = strrchr(prog_name, '\\'); + char *last_separator_fwd = strrchr(prog_name, '/'); + if (last_separator_fwd && (!last_separator || last_separator < last_separator_fwd)) { + last_separator = last_separator_fwd; + } + + /* Find the last dot in the filename after the last directory separator. */ + char *extension = NULL; + if (last_separator != NULL) { + extension = strrchr(last_separator, '.'); + } else { + extension = strrchr(prog_name, '.'); + } + + if (extension == NULL || extension == prog_name) { + /* No file extension found, it is not batch file. */ + return false; + } + + /* Check if the file extension is ".bat" or ".cmd" which is always executed by cmd.exe. */ + return _stricmp(extension, ".bat") == 0 || _stricmp(extension, ".cmd") == 0; +} + static zend_string *create_win_command_from_args(HashTable *args) { smart_str str = {0}; zval *arg_zv; - bool is_prog_name = 1; + bool is_prog_name = true; + bool is_cmd_execution = false; int elem_num = 0; ZEND_HASH_FOREACH_VAL(args, arg_zv) { @@ -531,11 +605,13 @@ static zend_string *create_win_command_from_args(HashTable *args) return NULL; } - if (!is_prog_name) { + if (is_prog_name) { + is_cmd_execution = is_executed_by_cmd(ZSTR_VAL(arg_str)); + } else { smart_str_appendc(&str, ' '); } - append_win_escaped_arg(&str, arg_str); + append_win_escaped_arg(&str, arg_str, !is_prog_name && is_cmd_execution); is_prog_name = 0; zend_string_release(arg_str); diff --git a/ext/standard/tests/general_functions/ghsa-pc52-254m-w9w7_1.phpt b/ext/standard/tests/general_functions/ghsa-pc52-254m-w9w7_1.phpt new file mode 100644 index 00000000000..8d0939cdf1b --- /dev/null +++ b/ext/standard/tests/general_functions/ghsa-pc52-254m-w9w7_1.phpt @@ -0,0 +1,29 @@ +--TEST-- +GHSA-54hq-v5wp-fqgv - proc_open does not correctly escape args for bat files +--SKIPIF-- + +--FILE-- + +--EXPECT-- +"¬epad.exe +--CLEAN-- + diff --git a/ext/standard/tests/general_functions/ghsa-pc52-254m-w9w7_2.phpt b/ext/standard/tests/general_functions/ghsa-pc52-254m-w9w7_2.phpt new file mode 100644 index 00000000000..a1e39d7ef9b --- /dev/null +++ b/ext/standard/tests/general_functions/ghsa-pc52-254m-w9w7_2.phpt @@ -0,0 +1,29 @@ +--TEST-- +GHSA-54hq-v5wp-fqgv - proc_open does not correctly escape args for cmd files +--SKIPIF-- + +--FILE-- +^()!.exe"], $descriptorspec, $pipes); +proc_close($proc); + +?> +--EXPECT-- +"¬epad<>^()!.exe +--CLEAN-- + diff --git a/ext/standard/tests/general_functions/ghsa-pc52-254m-w9w7_3.phpt b/ext/standard/tests/general_functions/ghsa-pc52-254m-w9w7_3.phpt new file mode 100644 index 00000000000..69f12d7b358 --- /dev/null +++ b/ext/standard/tests/general_functions/ghsa-pc52-254m-w9w7_3.phpt @@ -0,0 +1,29 @@ +--TEST-- +GHSA-54hq-v5wp-fqgv - proc_open does not correctly escape args for cmd executing batch files +--SKIPIF-- + +--FILE-- + +--EXPECT-- +"¬epad.exe +--CLEAN-- + From 093c08af25fb323efa0c8e6154aa9fdeae3d3b53 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Sun, 17 Mar 2024 21:04:47 +0100 Subject: [PATCH 2/4] Fix GHSA-wpj3-hf5j-x4v4: __Host-/__Secure- cookie bypass due to partial CVE-2022-31629 fix The check happened too early as later code paths may perform more mangling rules. Move the check downwards right before adding the actual variable. --- ext/standard/tests/ghsa-wpj3-hf5j-x4v4.phpt | 63 +++++++++++++++++++++ main/php_variables.c | 41 +++++++++----- 2 files changed, 90 insertions(+), 14 deletions(-) create mode 100644 ext/standard/tests/ghsa-wpj3-hf5j-x4v4.phpt diff --git a/ext/standard/tests/ghsa-wpj3-hf5j-x4v4.phpt b/ext/standard/tests/ghsa-wpj3-hf5j-x4v4.phpt new file mode 100644 index 00000000000..77fcb680894 --- /dev/null +++ b/ext/standard/tests/ghsa-wpj3-hf5j-x4v4.phpt @@ -0,0 +1,63 @@ +--TEST-- +ghsa-wpj3-hf5j-x4v4 (__Host-/__Secure- cookie bypass due to partial CVE-2022-31629 fix) +--COOKIE-- +..Host-test=ignore_1; +._Host-test=ignore_2; +.[Host-test=ignore_3; +_.Host-test=ignore_4; +__Host-test=ignore_5; +_[Host-test=ignore_6; +[.Host-test=ignore_7; +[_Host-test=ignore_8; +[[Host-test=ignore_9; +..Host-test[]=ignore_10; +._Host-test[]=ignore_11; +.[Host-test[]=ignore_12; +_.Host-test[]=ignore_13; +__Host-test[]=legitimate_14; +_[Host-test[]=legitimate_15; +[.Host-test[]=ignore_16; +[_Host-test[]=ignore_17; +[[Host-test[]=ignore_18; +..Secure-test=ignore_1; +._Secure-test=ignore_2; +.[Secure-test=ignore_3; +_.Secure-test=ignore_4; +__Secure-test=ignore_5; +_[Secure-test=ignore_6; +[.Secure-test=ignore_7; +[_Secure-test=ignore_8; +[[Secure-test=ignore_9; +..Secure-test[]=ignore_10; +._Secure-test[]=ignore_11; +.[Secure-test[]=ignore_12; +_.Secure-test[]=ignore_13; +__Secure-test[]=legitimate_14; +_[Secure-test[]=legitimate_15; +[.Secure-test[]=ignore_16; +[_Secure-test[]=ignore_17; +[[Secure-test[]=ignore_18; +--FILE-- + +--EXPECT-- +array(3) { + ["__Host-test"]=> + array(1) { + [0]=> + string(13) "legitimate_14" + } + ["_"]=> + array(2) { + ["Host-test["]=> + string(13) "legitimate_15" + ["Secure-test["]=> + string(13) "legitimate_15" + } + ["__Secure-test"]=> + array(1) { + [0]=> + string(13) "legitimate_14" + } +} diff --git a/main/php_variables.c b/main/php_variables.c index 17e4a1e5d2c..da7266416a5 100644 --- a/main/php_variables.c +++ b/main/php_variables.c @@ -54,6 +54,21 @@ static zend_always_inline void php_register_variable_quick(const char *name, siz zend_string_release_ex(key, 0); } +/* Discard variable if mangling made it start with __Host-, where pre-mangling it did not start with __Host- + * Discard variable if mangling made it start with __Secure-, where pre-mangling it did not start with __Secure- */ +static bool php_is_forbidden_variable_name(const char *mangled_name, size_t mangled_name_len, const char *pre_mangled_name) +{ + if (mangled_name_len >= sizeof("__Host-")-1 && strncmp(mangled_name, "__Host-", sizeof("__Host-")-1) == 0 && strncmp(pre_mangled_name, "__Host-", sizeof("__Host-")-1) != 0) { + return true; + } + + if (mangled_name_len >= sizeof("__Secure-")-1 && strncmp(mangled_name, "__Secure-", sizeof("__Secure-")-1) == 0 && strncmp(pre_mangled_name, "__Secure-", sizeof("__Secure-")-1) != 0) { + return true; + } + + return false; +} + PHPAPI void php_register_variable_ex(const char *var_name, zval *val, zval *track_vars_array) { char *p = NULL; @@ -104,20 +119,6 @@ PHPAPI void php_register_variable_ex(const char *var_name, zval *val, zval *trac } var_len = p - var; - /* Discard variable if mangling made it start with __Host-, where pre-mangling it did not start with __Host- */ - if (strncmp(var, "__Host-", sizeof("__Host-")-1) == 0 && strncmp(var_name, "__Host-", sizeof("__Host-")-1) != 0) { - zval_ptr_dtor_nogc(val); - free_alloca(var_orig, use_heap); - return; - } - - /* Discard variable if mangling made it start with __Secure-, where pre-mangling it did not start with __Secure- */ - if (strncmp(var, "__Secure-", sizeof("__Secure-")-1) == 0 && strncmp(var_name, "__Secure-", sizeof("__Secure-")-1) != 0) { - zval_ptr_dtor_nogc(val); - free_alloca(var_orig, use_heap); - return; - } - if (var_len==0) { /* empty variable name, or variable name with a space in it */ zval_ptr_dtor_nogc(val); free_alloca(var_orig, use_heap); @@ -221,6 +222,12 @@ PHPAPI void php_register_variable_ex(const char *var_name, zval *val, zval *trac return; } } else { + if (php_is_forbidden_variable_name(index, index_len, var_name)) { + zval_ptr_dtor_nogc(val); + free_alloca(var_orig, use_heap); + return; + } + gpc_element_p = zend_symtable_str_find(symtable1, index, index_len); if (!gpc_element_p) { zval tmp; @@ -258,6 +265,12 @@ plain_var: zval_ptr_dtor_nogc(val); } } else { + if (php_is_forbidden_variable_name(index, index_len, var_name)) { + zval_ptr_dtor_nogc(val); + free_alloca(var_orig, use_heap); + return; + } + zend_ulong idx; /* From 0ba5229a3f7572846e91c8f5382e87785f543826 Mon Sep 17 00:00:00 2001 From: Jakub Zelenka Date: Fri, 29 Mar 2024 15:27:59 +0000 Subject: [PATCH 3/4] Fix bug GHSA-q6x7-frmf-grcw: password_verify can erroneously return true Disallow null character in bcrypt password --- ext/standard/password.c | 5 +++++ ext/standard/tests/password/password_bcrypt_errors.phpt | 7 +++++++ 2 files changed, 12 insertions(+) diff --git a/ext/standard/password.c b/ext/standard/password.c index 651cffc9fe6..fbe58da6030 100644 --- a/ext/standard/password.c +++ b/ext/standard/password.c @@ -184,6 +184,11 @@ static zend_string* php_password_bcrypt_hash(const zend_string *password, zend_a zval *zcost; zend_long cost = PHP_PASSWORD_BCRYPT_COST; + if (memchr(ZSTR_VAL(password), '\0', ZSTR_LEN(password))) { + zend_value_error("Bcrypt password must not contain null character"); + return NULL; + } + if (options && (zcost = zend_hash_str_find(options, "cost", sizeof("cost")-1)) != NULL) { cost = zval_get_long(zcost); } diff --git a/ext/standard/tests/password/password_bcrypt_errors.phpt b/ext/standard/tests/password/password_bcrypt_errors.phpt index 10c3483f5a8..5d823cba021 100644 --- a/ext/standard/tests/password/password_bcrypt_errors.phpt +++ b/ext/standard/tests/password/password_bcrypt_errors.phpt @@ -14,7 +14,14 @@ try { } catch (ValueError $exception) { echo $exception->getMessage() . "\n"; } + +try { + var_dump(password_hash("null\0password", PASSWORD_BCRYPT)); +} catch (ValueError $e) { + echo $e->getMessage(), "\n"; +} ?> --EXPECT-- Invalid bcrypt cost parameter specified: 3 Invalid bcrypt cost parameter specified: 32 +Bcrypt password must not contain null character From de4f7f932166e66ff0950d97ae3bda5e355003d7 Mon Sep 17 00:00:00 2001 From: Ben Ramsey Date: Tue, 9 Apr 2024 23:41:29 -0500 Subject: [PATCH 4/4] Update NEWS --- NEWS | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/NEWS b/NEWS index 8373c66329a..0539611812d 100644 --- a/NEWS +++ b/NEWS @@ -2,6 +2,13 @@ PHP NEWS ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||| ?? ??? ????, PHP 8.1.28 +- Standard: + . Fixed bug GHSA-pc52-254m-w9w7 (Command injection via array-ish $command + parameter of proc_open). (CVE-2024-1874) (Jakub Zelenka) + . Fixed bug GHSA-wpj3-hf5j-x4v4 (__Host-/__Secure- cookie bypass due to + partial CVE-2022-31629 fix). (CVE-2024-2756) (nielsdos) + . Fixed bug GHSA-h746-cjrr-wfmr (password_verify can erroneously return true, + opening ATO risk). (CVE-2024-3096) (Jakub Zelenka) 21 Dec 2023, PHP 8.1.27