From 94ccab6abafceeb7c7dc3e633b774b3933dc851b Mon Sep 17 00:00:00 2001 From: Gina Peter Banyard Date: Sun, 15 Dec 2024 21:54:27 +0000 Subject: [PATCH 1/5] ext/pcntl: Refactor usage of strlcpy We allocate the buffer, so we know that it will fit. Drive by refactorings and questions --- ext/pcntl/pcntl.c | 78 ++++++++++++++++++++++++----------------------- 1 file changed, 40 insertions(+), 38 deletions(-) diff --git a/ext/pcntl/pcntl.c b/ext/pcntl/pcntl.c index dc8634be38240..d0c3aed4e4a04 100644 --- a/ext/pcntl/pcntl.c +++ b/ext/pcntl/pcntl.c @@ -620,86 +620,88 @@ PHP_FUNCTION(pcntl_wstopsig) /* {{{ Executes specified program in current process space as defined by exec(2) */ PHP_FUNCTION(pcntl_exec) { - zval *args = NULL, *envs = NULL; + zval *args = NULL; + HashTable *env_vars_ht = NULL; zval *element; - HashTable *args_hash, *envs_hash; - int argc = 0, argi = 0; - int envc = 0, envi = 0; char **argv = NULL, **envp = NULL; - char **current_arg, **pair; - size_t pair_length; - zend_string *key; char *path; size_t path_len; - zend_ulong key_num; ZEND_PARSE_PARAMETERS_START(1, 3) Z_PARAM_PATH(path, path_len) Z_PARAM_OPTIONAL Z_PARAM_ARRAY(args) - Z_PARAM_ARRAY(envs) + Z_PARAM_ARRAY_HT(env_vars_ht) ZEND_PARSE_PARAMETERS_END(); - if (ZEND_NUM_ARGS() > 1) { + if (args != NULL) { + // TODO Check array is a list? /* Build argument list */ SEPARATE_ARRAY(args); - args_hash = Z_ARRVAL_P(args); - argc = zend_hash_num_elements(args_hash); + const HashTable *args_ht = Z_ARRVAL_P(args); + uint32_t argc = zend_hash_num_elements(args_ht); + /* We want a NULL terminated array of char* with the first entry being the path, + * followed by the arguments */ argv = safe_emalloc((argc + 2), sizeof(char *), 0); - *argv = path; - current_arg = argv+1; - ZEND_HASH_FOREACH_VAL(args_hash, element) { - if (argi >= argc) break; + argv[0] = path; + char **current_arg = argv+1; + ZEND_HASH_FOREACH_VAL(args_ht, element) { if (!try_convert_to_string(element)) { efree(argv); RETURN_THROWS(); } + // TODO Check element does not have nul bytes? *current_arg = Z_STRVAL_P(element); - argi++; current_arg++; } ZEND_HASH_FOREACH_END(); *current_arg = NULL; } else { - argv = emalloc(2 * sizeof(char *)); + argv = safe_emalloc(2, sizeof(char *), 0); argv[0] = path; argv[1] = NULL; } - if ( ZEND_NUM_ARGS() == 3 ) { + if (env_vars_ht != NULL) { /* Build environment pair list */ - SEPARATE_ARRAY(envs); - envs_hash = Z_ARRVAL_P(envs); - envc = zend_hash_num_elements(envs_hash); + char **pair; + zend_ulong key_num; + zend_string *key; - size_t envp_len = (envc + 1); + /* We want a NULL terminated array of char* */ + size_t envp_len = zend_hash_num_elements(env_vars_ht) + 1; pair = envp = safe_emalloc(envp_len, sizeof(char *), 0); memset(envp, 0, sizeof(char *) * envp_len); - ZEND_HASH_FOREACH_KEY_VAL(envs_hash, key_num, key, element) { - if (envi >= envc) break; + ZEND_HASH_FOREACH_KEY_VAL(env_vars_ht, key_num, key, element) { + zend_string *element_str = zval_try_get_string(element); + if (element_str == NULL) { + goto cleanup_env_vars; + } + if (!key) { - key = zend_long_to_str(key_num); + // TODO Does it even make sense to have an environment variable which is an integer? + key = zend_long_to_str((zend_long) key_num); } else { zend_string_addref(key); } - if (!try_convert_to_string(element)) { - zend_string_release(key); - goto cleanup_env_vars; - } + // TODO Check key and element do not have nul bytes? /* Length of element + equal sign + length of key + null */ - ZEND_ASSERT(Z_STRLEN_P(element) < SIZE_MAX && ZSTR_LEN(key) < SIZE_MAX); - *pair = safe_emalloc(Z_STRLEN_P(element) + 1, sizeof(char), ZSTR_LEN(key) + 1); - pair_length = Z_STRLEN_P(element) + ZSTR_LEN(key) + 2; - strlcpy(*pair, ZSTR_VAL(key), ZSTR_LEN(key) + 1); - strlcat(*pair, "=", pair_length); - strlcat(*pair, Z_STRVAL_P(element), pair_length); + const uint8_t equal_len = strlen("="); + size_t pair_length = ZSTR_LEN(element_str) + equal_len + ZSTR_LEN(key) + 1; + ZEND_ASSERT(pair_length < SIZE_MAX); + *pair = emalloc(pair_length); + /* Copy key=element + final null byte into buffer */ + memcpy(*pair, ZSTR_VAL(key), ZSTR_LEN(key)); + memcpy(*pair + ZSTR_LEN(key), "=", equal_len); + /* Copy null byte */ + memcpy(*pair + ZSTR_LEN(key) + equal_len, ZSTR_VAL(element_str), ZSTR_LEN(element_str) + 1); /* Cleanup */ - zend_string_release_ex(key, 0); - envi++; + zend_string_release_ex(key, false); + zend_string_release_ex(element_str, false); pair++; } ZEND_HASH_FOREACH_END(); *(pair) = NULL; From 42a70f4ae17e2ce19ef389b6c5393b0e424a2aec Mon Sep 17 00:00:00 2001 From: Gina Peter Banyard Date: Mon, 16 Dec 2024 23:21:55 +0000 Subject: [PATCH 2/5] Review --- ext/pcntl/pcntl.c | 32 +++++++++++------ .../pcntl_exec_strings_with_nul_bytes.phpt | 36 +++++++++++++++++++ 2 files changed, 57 insertions(+), 11 deletions(-) create mode 100644 ext/pcntl/tests/pcntl_exec_strings_with_nul_bytes.phpt diff --git a/ext/pcntl/pcntl.c b/ext/pcntl/pcntl.c index d0c3aed4e4a04..4b5aa561d59ec 100644 --- a/ext/pcntl/pcntl.c +++ b/ext/pcntl/pcntl.c @@ -651,14 +651,18 @@ PHP_FUNCTION(pcntl_exec) efree(argv); RETURN_THROWS(); } - // TODO Check element does not have nul bytes? + if (zend_str_has_nul_byte(Z_STR_P(element))) { + zend_argument_value_error(2, "individual argument must not contain null bytes"); + efree(argv); + RETURN_THROWS(); + } *current_arg = Z_STRVAL_P(element); current_arg++; } ZEND_HASH_FOREACH_END(); *current_arg = NULL; } else { - argv = safe_emalloc(2, sizeof(char *), 0); + argv = emalloc(2 * sizeof(char *)); argv[0] = path; argv[1] = NULL; } @@ -679,25 +683,31 @@ PHP_FUNCTION(pcntl_exec) goto cleanup_env_vars; } + if (zend_str_has_nul_byte(element_str)) { + zend_argument_value_error(3, "value for environment variable must not contain null bytes"); + zend_string_release_ex(element_str, false); + goto cleanup_env_vars; + } + + /* putenv() allows integer environment variables */ if (!key) { - // TODO Does it even make sense to have an environment variable which is an integer? key = zend_long_to_str((zend_long) key_num); } else { + if (zend_str_has_nul_byte(key)) { + zend_argument_value_error(3, "name for environment variable must not contain null bytes"); + zend_string_release_ex(element_str, false); + goto cleanup_env_vars; + } zend_string_addref(key); } - // TODO Check key and element do not have nul bytes? - /* Length of element + equal sign + length of key + null */ - const uint8_t equal_len = strlen("="); - size_t pair_length = ZSTR_LEN(element_str) + equal_len + ZSTR_LEN(key) + 1; - ZEND_ASSERT(pair_length < SIZE_MAX); - *pair = emalloc(pair_length); + *pair = safe_emalloc(ZSTR_LEN(element_str) + 1, sizeof(char), ZSTR_LEN(key) + 1); /* Copy key=element + final null byte into buffer */ memcpy(*pair, ZSTR_VAL(key), ZSTR_LEN(key)); - memcpy(*pair + ZSTR_LEN(key), "=", equal_len); + (*pair)[ZSTR_LEN(key)] = '='; /* Copy null byte */ - memcpy(*pair + ZSTR_LEN(key) + equal_len, ZSTR_VAL(element_str), ZSTR_LEN(element_str) + 1); + memcpy(*pair + ZSTR_LEN(key) + 1, ZSTR_VAL(element_str), ZSTR_LEN(element_str) + 1); /* Cleanup */ zend_string_release_ex(key, false); diff --git a/ext/pcntl/tests/pcntl_exec_strings_with_nul_bytes.phpt b/ext/pcntl/tests/pcntl_exec_strings_with_nul_bytes.phpt new file mode 100644 index 0000000000000..5bf4c9188e941 --- /dev/null +++ b/ext/pcntl/tests/pcntl_exec_strings_with_nul_bytes.phpt @@ -0,0 +1,36 @@ +--TEST-- +pcntl_exec(): Test cleanup after value that contain null bytes has been encountered for $args and $env_vars. +--EXTENSIONS-- +pcntl +--FILE-- +getMessage(), "\n"; +} + +try { + pcntl_exec( + 'cmd', + ['-n'], + ['var1' => 'value1', 'var2' => "value\0null\0byte"], + ); +} catch (Throwable $e) { + echo $e::class, ': ', $e->getMessage(), "\n"; +} + +try { + pcntl_exec( + 'cmd', + ['-n'], + ['var1' => 'value1', "key\0null\0byte" => "value2"], + ); +} catch (Throwable $e) { + echo $e::class, ': ', $e->getMessage(), "\n"; +} +?> +--EXPECT-- +ValueError: pcntl_exec(): Argument #2 ($args) individual argument must not contain null bytes +ValueError: pcntl_exec(): Argument #3 ($env_vars) value for environment variable must not contain null bytes +ValueError: pcntl_exec(): Argument #3 ($env_vars) name for environment variable must not contain null bytes From 0bfdb71987372988f8fefedd942227b263ab06f3 Mon Sep 17 00:00:00 2001 From: Gina Peter Banyard Date: Mon, 16 Dec 2024 23:25:13 +0000 Subject: [PATCH 3/5] UPGRADING --- UPGRADING | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/UPGRADING b/UPGRADING index d0bb9bae6131b..49946f2ecc4eb 100644 --- a/UPGRADING +++ b/UPGRADING @@ -42,6 +42,12 @@ PHP 8.5 UPGRADE NOTES . ldap_get_option() and ldap_set_option() now throw a ValueError when passing an invalid option. +- PCNTL: + . pcntl_exec() now throws ValueErrors when entries of the $args parameter + contain null bytes. + . pcntl_exec() now throws ValueErrors when entries or keys of of the + $env_vars parameter contain null bytes. + - SPL: . ArrayObject no longer accepts enums, as modifying the $name or $value properties can break engine assumptions. From c97e105e5149b0a11674f3f472d60b1f2114aa8c Mon Sep 17 00:00:00 2001 From: Gina Peter Banyard Date: Mon, 16 Dec 2024 23:26:55 +0000 Subject: [PATCH 4/5] exec_pcntl() always return false --- UPGRADING | 3 +++ ext/pcntl/pcntl.stub.php | 2 +- ext/pcntl/pcntl_arginfo.h | 4 ++-- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/UPGRADING b/UPGRADING index 49946f2ecc4eb..b21c3d6381132 100644 --- a/UPGRADING +++ b/UPGRADING @@ -97,6 +97,9 @@ PHP 8.5 UPGRADE NOTES gzfile, gzopen and readgzfile functions had been changed from int to boolean. +- PCNTL: + . pcntl_exec() now has a formal return type of false. + - PDO_PGSQL: . PDO::pgsqlCopyFromArray also supports inputs as Iterable. . Pdo\Pgsql::setAttribute and Pdo\Pgsql::prepare supports diff --git a/ext/pcntl/pcntl.stub.php b/ext/pcntl/pcntl.stub.php index bdae57414b4c3..c62a40139d9ed 100644 --- a/ext/pcntl/pcntl.stub.php +++ b/ext/pcntl/pcntl.stub.php @@ -1055,7 +1055,7 @@ function pcntl_wtermsig(int $status): int|false {} function pcntl_wstopsig(int $status): int|false {} - function pcntl_exec(string $path, array $args = [], array $env_vars = []): bool {} + function pcntl_exec(string $path, array $args = [], array $env_vars = []): false {} function pcntl_alarm(int $seconds): int {} diff --git a/ext/pcntl/pcntl_arginfo.h b/ext/pcntl/pcntl_arginfo.h index 9ae35091c4098..8da7cb70a6aa7 100644 --- a/ext/pcntl/pcntl_arginfo.h +++ b/ext/pcntl/pcntl_arginfo.h @@ -1,5 +1,5 @@ /* This is a generated file, edit the .stub.php file instead. - * Stub hash: 897062ad1dfe06326e561429509360174820379e */ + * Stub hash: c637fe2de641cfd8f0ff83a908ac59bf63a68e44 */ ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_pcntl_fork, 0, 0, IS_LONG, 0) ZEND_END_ARG_INFO() @@ -83,7 +83,7 @@ ZEND_END_ARG_INFO() #define arginfo_pcntl_wstopsig arginfo_pcntl_wexitstatus -ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_pcntl_exec, 0, 1, _IS_BOOL, 0) +ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_pcntl_exec, 0, 1, IS_FALSE, 0) ZEND_ARG_TYPE_INFO(0, path, IS_STRING, 0) ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, args, IS_ARRAY, 0, "[]") ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, env_vars, IS_ARRAY, 0, "[]") From dbd6f7b234002fb2d96a394f7d65f727a8f4c1b9 Mon Sep 17 00:00:00 2001 From: Gina Peter Banyard Date: Tue, 17 Dec 2024 13:16:18 +0000 Subject: [PATCH 5/5] [skip ci] Fix typo Co-authored-by: Niels Dossche <7771979+nielsdos@users.noreply.github.com> --- UPGRADING | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/UPGRADING b/UPGRADING index b21c3d6381132..b7372ecef082d 100644 --- a/UPGRADING +++ b/UPGRADING @@ -45,7 +45,7 @@ PHP 8.5 UPGRADE NOTES - PCNTL: . pcntl_exec() now throws ValueErrors when entries of the $args parameter contain null bytes. - . pcntl_exec() now throws ValueErrors when entries or keys of of the + . pcntl_exec() now throws ValueErrors when entries or keys of the $env_vars parameter contain null bytes. - SPL: