From 7ad5999e4d55074ac828cf0594ef998aba157fa0 Mon Sep 17 00:00:00 2001 From: Gina Peter Banyard Date: Wed, 29 May 2024 00:39:46 +0100 Subject: [PATCH 1/6] Pull zend_string* from INI directive --- ext/mbstring/mbstring.c | 4 ++-- ext/standard/mail.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/ext/mbstring/mbstring.c b/ext/mbstring/mbstring.c index c6111b49e213..41e58fc30eb7 100644 --- a/ext/mbstring/mbstring.c +++ b/ext/mbstring/mbstring.c @@ -4435,7 +4435,6 @@ PHP_FUNCTION(mb_send_mail) zend_string *str_headers = NULL; size_t i; char *to_r = NULL; - char *force_extra_parameters = INI_STR("mail.force_extra_parameters"); bool suppress_content_type = false; bool suppress_content_transfer_encoding = false; @@ -4653,8 +4652,9 @@ PHP_FUNCTION(mb_send_mail) str_headers = smart_str_extract(&str); + zend_string *force_extra_parameters = zend_ini_str_ex("mail.force_extra_parameters", strlen("mail.force_extra_parameters"), false, NULL); if (force_extra_parameters) { - extra_cmd = php_escape_shell_cmd(force_extra_parameters); + extra_cmd = php_escape_shell_cmd(ZSTR_VAL(force_extra_parameters)); } else if (extra_cmd) { extra_cmd = php_escape_shell_cmd(ZSTR_VAL(extra_cmd)); } diff --git a/ext/standard/mail.c b/ext/standard/mail.c index 48ab8da078fe..b9c75dafb5ed 100644 --- a/ext/standard/mail.c +++ b/ext/standard/mail.c @@ -247,7 +247,6 @@ PHP_FUNCTION(mail) HashTable *headers_ht = NULL; size_t to_len, message_len; size_t subject_len, i; - char *force_extra_parameters = INI_STR("mail.force_extra_parameters"); char *to_r, *subject_r; ZEND_PARSE_PARAMETERS_START(3, 5) @@ -312,8 +311,9 @@ PHP_FUNCTION(mail) subject_r = subject; } + zend_string *force_extra_parameters = zend_ini_str_ex("mail.force_extra_parameters", strlen("mail.force_extra_parameters"), false, NULL); if (force_extra_parameters) { - extra_cmd = php_escape_shell_cmd(force_extra_parameters); + extra_cmd = php_escape_shell_cmd(ZSTR_VAL(force_extra_parameters)); } else if (extra_cmd) { extra_cmd = php_escape_shell_cmd(ZSTR_VAL(extra_cmd)); } From 5c1980644fdab5efe48f1a97bda3d0b75fd27fc3 Mon Sep 17 00:00:00 2001 From: Gina Peter Banyard Date: Wed, 29 May 2024 00:46:59 +0100 Subject: [PATCH 2/6] Ensure that mail.force_extra_parameters INI directive does not have any nul bytes --- main/main.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/main/main.c b/main/main.c index eae997730333..330f87505bf8 100644 --- a/main/main.c +++ b/main/main.c @@ -652,6 +652,11 @@ static PHP_INI_MH(OnUpdateMailLog) /* {{{ PHP_INI_MH */ static PHP_INI_MH(OnChangeMailForceExtra) { + /* Check that INI setting does not have any nul bytes */ + if (new_value && ZSTR_LEN(new_value) != strlen(ZSTR_VAL(new_value))) { + /* TODO Emit warning? */ + return FAILURE; + } /* Don't allow changing it in htaccess */ if (stage == PHP_INI_STAGE_HTACCESS) { return FAILURE; From d06081bd10fa192786752dda9a9895f9a258ed2b Mon Sep 17 00:00:00 2001 From: Gina Peter Banyard Date: Wed, 29 May 2024 00:56:17 +0100 Subject: [PATCH 3/6] ext/standard: Make php_escape_shell_cmd() take a zend_string* instead of char* This saves on an expensive strlen() computation --- UPGRADING.INTERNALS | 3 +++ ext/mbstring/mbstring.c | 4 ++-- ext/standard/exec.c | 21 ++++++++++----------- ext/standard/exec.h | 2 +- ext/standard/mail.c | 4 ++-- 5 files changed, 18 insertions(+), 16 deletions(-) diff --git a/UPGRADING.INTERNALS b/UPGRADING.INTERNALS index 85357b0d97e1..41695dfab432 100644 --- a/UPGRADING.INTERNALS +++ b/UPGRADING.INTERNALS @@ -246,6 +246,9 @@ PHP 8.4 INTERNALS UPGRADE NOTES g. ext/standard - Added the php_base64_encode_ex() API with flag parameters, value can be PHP_BASE64_NO_PADDING to encode without the padding character '='. + - The php_escape_shell_cmd() now takes a zend_string* instead of a char* + Moreover, providing it with a binary safe string is the responsibility of + the caller now. ======================== 4. OpCode changes diff --git a/ext/mbstring/mbstring.c b/ext/mbstring/mbstring.c index 41e58fc30eb7..2d719c7e29ec 100644 --- a/ext/mbstring/mbstring.c +++ b/ext/mbstring/mbstring.c @@ -4654,9 +4654,9 @@ PHP_FUNCTION(mb_send_mail) zend_string *force_extra_parameters = zend_ini_str_ex("mail.force_extra_parameters", strlen("mail.force_extra_parameters"), false, NULL); if (force_extra_parameters) { - extra_cmd = php_escape_shell_cmd(ZSTR_VAL(force_extra_parameters)); + extra_cmd = php_escape_shell_cmd(force_extra_parameters); } else if (extra_cmd) { - extra_cmd = php_escape_shell_cmd(ZSTR_VAL(extra_cmd)); + extra_cmd = php_escape_shell_cmd(extra_cmd); } RETVAL_BOOL(php_mail(to_r, ZSTR_VAL(subject), message, ZSTR_VAL(str_headers), extra_cmd ? ZSTR_VAL(extra_cmd) : NULL)); diff --git a/ext/standard/exec.c b/ext/standard/exec.c index 1b1b0ab9e9ce..27e47f1f0720 100644 --- a/ext/standard/exec.c +++ b/ext/standard/exec.c @@ -279,16 +279,20 @@ PHP_FUNCTION(passthru) *NOT* safe for binary strings */ -PHPAPI zend_string *php_escape_shell_cmd(const char *str) +PHPAPI zend_string *php_escape_shell_cmd(const zend_string *unescaped_cmd) { size_t x, y; - size_t l = strlen(str); - uint64_t estimate = (2 * (uint64_t)l) + 1; zend_string *cmd; #ifndef PHP_WIN32 char *p = NULL; #endif + ZEND_ASSERT(ZSTR_LEN(unescaped_cmd) == strlen(ZSTR_VAL(unescaped_cmd)) && "Must be a binary safe string"); + size_t l = ZSTR_LEN(unescaped_cmd); + const char *str = ZSTR_VAL(unescaped_cmd); + + uint64_t estimate = (2 * (uint64_t)l) + 1; + /* max command line length - two single quotes - \0 byte length */ if (l > cmd_max_len - 2 - 1) { php_error_docref(NULL, E_ERROR, "Command exceeds the allowed length of %zu bytes", cmd_max_len); @@ -471,18 +475,13 @@ PHPAPI zend_string *php_escape_shell_arg(const char *str) /* {{{ Escape shell metacharacters */ PHP_FUNCTION(escapeshellcmd) { - char *command; - size_t command_len; + zend_string *command; ZEND_PARSE_PARAMETERS_START(1, 1) - Z_PARAM_STRING(command, command_len) + Z_PARAM_PATH_STR(command) ZEND_PARSE_PARAMETERS_END(); - if (command_len) { - if (command_len != strlen(command)) { - zend_argument_value_error(1, "must not contain any null bytes"); - RETURN_THROWS(); - } + if (ZSTR_LEN(command)) { RETVAL_STR(php_escape_shell_cmd(command)); } else { RETVAL_EMPTY_STRING(); diff --git a/ext/standard/exec.h b/ext/standard/exec.h index db8c74aa8673..c5a1ddeade32 100644 --- a/ext/standard/exec.h +++ b/ext/standard/exec.h @@ -20,7 +20,7 @@ PHP_MINIT_FUNCTION(proc_open); PHP_MINIT_FUNCTION(exec); -PHPAPI zend_string *php_escape_shell_cmd(const char *str); +PHPAPI zend_string *php_escape_shell_cmd(const zend_string *unescaped_cmd); PHPAPI zend_string *php_escape_shell_arg(const char *str); PHPAPI int php_exec(int type, const char *cmd, zval *array, zval *return_value); diff --git a/ext/standard/mail.c b/ext/standard/mail.c index b9c75dafb5ed..a424825a40dd 100644 --- a/ext/standard/mail.c +++ b/ext/standard/mail.c @@ -313,9 +313,9 @@ PHP_FUNCTION(mail) zend_string *force_extra_parameters = zend_ini_str_ex("mail.force_extra_parameters", strlen("mail.force_extra_parameters"), false, NULL); if (force_extra_parameters) { - extra_cmd = php_escape_shell_cmd(ZSTR_VAL(force_extra_parameters)); + extra_cmd = php_escape_shell_cmd(force_extra_parameters); } else if (extra_cmd) { - extra_cmd = php_escape_shell_cmd(ZSTR_VAL(extra_cmd)); + extra_cmd = php_escape_shell_cmd(extra_cmd); } if (php_mail(to_r, subject_r, message, headers_str && ZSTR_LEN(headers_str) ? ZSTR_VAL(headers_str) : NULL, extra_cmd ? ZSTR_VAL(extra_cmd) : NULL)) { From db6ba01991c63b4db67839fda18090c7f48fe7e0 Mon Sep 17 00:00:00 2001 From: Gina Peter Banyard Date: Wed, 29 May 2024 00:59:56 +0100 Subject: [PATCH 4/6] Convert E_ERROR to ValueError in php_escape_shell_cmd() --- ext/standard/exec.c | 4 ++-- .../general_functions/escapeshellcmd_bug71270.phpt | 10 ++++++++-- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/ext/standard/exec.c b/ext/standard/exec.c index 27e47f1f0720..f4416866f22c 100644 --- a/ext/standard/exec.c +++ b/ext/standard/exec.c @@ -295,7 +295,7 @@ PHPAPI zend_string *php_escape_shell_cmd(const zend_string *unescaped_cmd) /* max command line length - two single quotes - \0 byte length */ if (l > cmd_max_len - 2 - 1) { - php_error_docref(NULL, E_ERROR, "Command exceeds the allowed length of %zu bytes", cmd_max_len); + zend_value_error("Command exceeds the allowed length of %zu bytes", cmd_max_len); return ZSTR_EMPTY_ALLOC(); } @@ -371,7 +371,7 @@ PHPAPI zend_string *php_escape_shell_cmd(const zend_string *unescaped_cmd) ZSTR_VAL(cmd)[y] = '\0'; if (y > cmd_max_len + 1) { - php_error_docref(NULL, E_ERROR, "Escaped command exceeds the allowed length of %zu bytes", cmd_max_len); + zend_value_error("Escaped command exceeds the allowed length of %zu bytes", cmd_max_len); zend_string_release_ex(cmd, 0); return ZSTR_EMPTY_ALLOC(); } diff --git a/ext/standard/tests/general_functions/escapeshellcmd_bug71270.phpt b/ext/standard/tests/general_functions/escapeshellcmd_bug71270.phpt index 4686193d4130..3116283000ea 100644 --- a/ext/standard/tests/general_functions/escapeshellcmd_bug71270.phpt +++ b/ext/standard/tests/general_functions/escapeshellcmd_bug71270.phpt @@ -4,9 +4,15 @@ Test escapeshellcmd() allowed argument length getMessage(), PHP_EOL; +} ?> ===DONE=== --EXPECTF-- -Fatal error: escapeshellcmd(): Command exceeds the allowed length of %d bytes in %s on line %d +ValueError: Command exceeds the allowed length of %d bytes +===DONE=== From 85cc2b3532396a98e510400370a213bf01a41b6c Mon Sep 17 00:00:00 2001 From: Gina Peter Banyard Date: Wed, 29 May 2024 01:15:08 +0100 Subject: [PATCH 5/6] ext/standard: Make php_escape_shell_arg() take a zend_string* instead of char* This saves on an expensive strlen() computation --- UPGRADING.INTERNALS | 3 +++ ext/standard/exec.c | 18 ++++++++---------- ext/standard/exec.h | 2 +- 3 files changed, 12 insertions(+), 11 deletions(-) diff --git a/UPGRADING.INTERNALS b/UPGRADING.INTERNALS index 41695dfab432..f3fece4ed00b 100644 --- a/UPGRADING.INTERNALS +++ b/UPGRADING.INTERNALS @@ -249,6 +249,9 @@ PHP 8.4 INTERNALS UPGRADE NOTES - The php_escape_shell_cmd() now takes a zend_string* instead of a char* Moreover, providing it with a binary safe string is the responsibility of the caller now. + - The php_escape_shell_arg() now takes a zend_string* instead of a char* + Moreover, providing it with a binary safe string is the responsibility of + the caller now. ======================== 4. OpCode changes diff --git a/ext/standard/exec.c b/ext/standard/exec.c index f4416866f22c..27087efa01b9 100644 --- a/ext/standard/exec.c +++ b/ext/standard/exec.c @@ -389,11 +389,15 @@ PHPAPI zend_string *php_escape_shell_cmd(const zend_string *unescaped_cmd) /* }}} */ /* {{{ php_escape_shell_arg */ -PHPAPI zend_string *php_escape_shell_arg(const char *str) +PHPAPI zend_string *php_escape_shell_arg(const zend_string *unescaped_arg) { size_t x, y = 0; - size_t l = strlen(str); zend_string *cmd; + + ZEND_ASSERT(ZSTR_LEN(unescaped_arg) == strlen(ZSTR_VAL(unescaped_arg)) && "Must be a binary safe string"); + size_t l = ZSTR_LEN(unescaped_arg); + const char *str = ZSTR_VAL(unescaped_arg); + uint64_t estimate = (4 * (uint64_t)l) + 3; /* max command line length - two single quotes - \0 byte length */ @@ -492,18 +496,12 @@ PHP_FUNCTION(escapeshellcmd) /* {{{ Quote and escape an argument for use in a shell command */ PHP_FUNCTION(escapeshellarg) { - char *argument; - size_t argument_len; + zend_string *argument; ZEND_PARSE_PARAMETERS_START(1, 1) - Z_PARAM_STRING(argument, argument_len) + Z_PARAM_PATH_STR(argument) ZEND_PARSE_PARAMETERS_END(); - if (argument_len != strlen(argument)) { - zend_argument_value_error(1, "must not contain any null bytes"); - RETURN_THROWS(); - } - RETVAL_STR(php_escape_shell_arg(argument)); } /* }}} */ diff --git a/ext/standard/exec.h b/ext/standard/exec.h index c5a1ddeade32..1ad755363e3d 100644 --- a/ext/standard/exec.h +++ b/ext/standard/exec.h @@ -21,7 +21,7 @@ PHP_MINIT_FUNCTION(proc_open); PHP_MINIT_FUNCTION(exec); PHPAPI zend_string *php_escape_shell_cmd(const zend_string *unescaped_cmd); -PHPAPI zend_string *php_escape_shell_arg(const char *str); +PHPAPI zend_string *php_escape_shell_arg(const zend_string *unescaped_arg); PHPAPI int php_exec(int type, const char *cmd, zval *array, zval *return_value); #endif /* EXEC_H */ From 92589adb2765f4242ed4fb6933c752c64a238fbf Mon Sep 17 00:00:00 2001 From: Gina Peter Banyard Date: Wed, 29 May 2024 01:17:12 +0100 Subject: [PATCH 6/6] Convert E_ERROR to ValueError in php_escape_shell_arg() --- ext/standard/exec.c | 4 ++-- .../general_functions/escapeshellarg_bug71270.phpt | 10 ++++++++-- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/ext/standard/exec.c b/ext/standard/exec.c index 27087efa01b9..5b8ba89de970 100644 --- a/ext/standard/exec.c +++ b/ext/standard/exec.c @@ -402,7 +402,7 @@ PHPAPI zend_string *php_escape_shell_arg(const zend_string *unescaped_arg) /* max command line length - two single quotes - \0 byte length */ if (l > cmd_max_len - 2 - 1) { - php_error_docref(NULL, E_ERROR, "Argument exceeds the allowed length of %zu bytes", cmd_max_len); + zend_value_error("Argument exceeds the allowed length of %zu bytes", cmd_max_len); return ZSTR_EMPTY_ALLOC(); } @@ -461,7 +461,7 @@ PHPAPI zend_string *php_escape_shell_arg(const zend_string *unescaped_arg) ZSTR_VAL(cmd)[y] = '\0'; if (y > cmd_max_len + 1) { - php_error_docref(NULL, E_ERROR, "Escaped argument exceeds the allowed length of %zu bytes", cmd_max_len); + zend_value_error("Escaped argument exceeds the allowed length of %zu bytes", cmd_max_len); zend_string_release_ex(cmd, 0); return ZSTR_EMPTY_ALLOC(); } diff --git a/ext/standard/tests/general_functions/escapeshellarg_bug71270.phpt b/ext/standard/tests/general_functions/escapeshellarg_bug71270.phpt index c57da3691c55..4460f54ba049 100644 --- a/ext/standard/tests/general_functions/escapeshellarg_bug71270.phpt +++ b/ext/standard/tests/general_functions/escapeshellarg_bug71270.phpt @@ -4,9 +4,15 @@ Test escapeshellarg() allowed argument length getMessage(), PHP_EOL; +} ?> ===DONE=== --EXPECTF-- -Fatal error: escapeshellarg(): Argument exceeds the allowed length of %d bytes in %s on line %d +ValueError: Argument exceeds the allowed length of %d bytes +===DONE===