From 1ed821258cab58c8e7cc4720fa652ae195de483f Mon Sep 17 00:00:00 2001 From: Raj Siva-Rajah Date: Tue, 21 Jul 2026 20:31:42 +0000 Subject: [PATCH] fix: correct SSH2 shell and exec environments Validate the actual width-without-height argument count in ssh2_shell and report the invalid call as a recoverable warning. Complete width and height pairs now reach the normal shell path. Traverse shell and exec environment arrays with paired Zend hash iterators. This makes empty shell environments safe and prevents exec keys from being combined with values from a different iterator position. Keep environment values out of setenv failure warnings while retaining the variable name for diagnosis. Add PHPT coverage for dimension validation, empty and multi-value environments, and warning redaction, and include the tests in the PECL package manifest. --- package.xml | 3 + ssh2_fopen_wrappers.c | 69 ++++++++----------- tests/ssh2_shell_arguments.phpt | 34 +++++++++ ...ssh2_shell_exec_environment_iteration.phpt | 62 +++++++++++++++++ ...ssh2_shell_exec_environment_redaction.phpt | 65 +++++++++++++++++ 5 files changed, 191 insertions(+), 42 deletions(-) create mode 100644 tests/ssh2_shell_arguments.phpt create mode 100644 tests/ssh2_shell_exec_environment_iteration.phpt create mode 100644 tests/ssh2_shell_exec_environment_redaction.phpt diff --git a/package.xml b/package.xml index 07c0427..2faf69f 100644 --- a/package.xml +++ b/package.xml @@ -82,6 +82,9 @@ + + + diff --git a/ssh2_fopen_wrappers.c b/ssh2_fopen_wrappers.c index f1b3ce1..76585ec 100644 --- a/ssh2_fopen_wrappers.c +++ b/ssh2_fopen_wrappers.c @@ -540,29 +540,22 @@ static php_stream *php_ssh2_shell_open(LIBSSH2_SESSION *session, zend_resource * if (environment) { zend_string *key; - int key_type; - zend_ulong idx; - - for(zend_hash_internal_pointer_reset(HASH_OF(environment)); - (key_type = zend_hash_get_current_key(HASH_OF(environment), &key, &idx)) != HASH_KEY_NON_EXISTENT; - zend_hash_move_forward(HASH_OF(environment))) { - if (key_type == HASH_KEY_IS_STRING) { - zval *value; - - if ((value = zend_hash_get_current_data(HASH_OF(environment))) != NULL) { - zval copyval = *value; - - zval_copy_ctor(©val); - convert_to_string(©val); - if (libssh2_channel_setenv_ex(channel, key->val, key->len, Z_STRVAL(copyval), Z_STRLEN(copyval))) { - php_error_docref(NULL, E_WARNING, "Failed setting %s=%s on remote end", ZSTR_VAL(key), Z_STRVAL(copyval)); - } - zval_dtor(©val); + zval *value; + + ZEND_HASH_FOREACH_STR_KEY_VAL(Z_ARRVAL_P(environment), key, value) { + if (key) { + zval copyval = *value; + + zval_copy_ctor(©val); + convert_to_string(©val); + if (libssh2_channel_setenv_ex(channel, key->val, key->len, Z_STRVAL(copyval), Z_STRLEN(copyval))) { + php_error_docref(NULL, E_WARNING, "Failed setting environment variable %s on remote end", ZSTR_VAL(key)); } + zval_dtor(©val); } else { php_error_docref(NULL, E_NOTICE, "Skipping numeric index in environment array"); } - } + } ZEND_HASH_FOREACH_END(); } if (type == PHP_SSH2_TERM_UNIT_CHARS) { @@ -737,8 +730,8 @@ PHP_FUNCTION(ssh2_shell) zend_long type = PHP_SSH2_DEFAULT_TERM_UNIT; int argc = ZEND_NUM_ARGS(); - if (argc == 5) { - php_error_docref(NULL, E_ERROR, "width specified without height parameter"); + if (argc == 4) { + php_error_docref(NULL, E_WARNING, "width specified without height parameter"); RETURN_FALSE; } @@ -812,31 +805,23 @@ static php_stream *php_ssh2_exec_command(LIBSSH2_SESSION *session, zend_resource } if (environment) { - zend_string *key = NULL; - int key_type; - zend_ulong idx = 0; - HashPosition pos; - - for(zend_hash_internal_pointer_reset_ex(HASH_OF(environment), &pos); - (key_type = zend_hash_get_current_key_ex(HASH_OF(environment), &key, &idx, &pos)) != HASH_KEY_NON_EXISTENT; - zend_hash_move_forward_ex(HASH_OF(environment), &pos)) { - if (key_type == HASH_KEY_IS_STRING) { - zval *value; - - if ((value = zend_hash_get_current_data(HASH_OF(environment))) != NULL) { - zval copyval = *value; - - zval_copy_ctor(©val); - convert_to_string(©val); - if (libssh2_channel_setenv_ex(channel, key->val, key->len, Z_STRVAL(copyval), Z_STRLEN(copyval))) { - php_error_docref(NULL, E_WARNING, "Failed setting %s=%s on remote end", ZSTR_VAL(key), Z_STRVAL(copyval)); - } - zval_dtor(©val); + zend_string *key; + zval *value; + + ZEND_HASH_FOREACH_STR_KEY_VAL(Z_ARRVAL_P(environment), key, value) { + if (key) { + zval copyval = *value; + + zval_copy_ctor(©val); + convert_to_string(©val); + if (libssh2_channel_setenv_ex(channel, key->val, key->len, Z_STRVAL(copyval), Z_STRLEN(copyval))) { + php_error_docref(NULL, E_WARNING, "Failed setting environment variable %s on remote end", ZSTR_VAL(key)); } + zval_dtor(©val); } else { php_error_docref(NULL, E_NOTICE, "Skipping numeric index in environment array"); } - } + } ZEND_HASH_FOREACH_END(); } if (term) { diff --git a/tests/ssh2_shell_arguments.phpt b/tests/ssh2_shell_arguments.phpt new file mode 100644 index 0000000..a31312c --- /dev/null +++ b/tests/ssh2_shell_arguments.phpt @@ -0,0 +1,34 @@ +--TEST-- +ssh2_shell() validates paired terminal dimensions +--SKIPIF-- + +--FILE-- + +--EXPECTF-- +bool(true) + +Warning: ssh2_shell(): width specified without height parameter in %s on line %d +bool(false) diff --git a/tests/ssh2_shell_exec_environment_iteration.phpt b/tests/ssh2_shell_exec_environment_iteration.phpt new file mode 100644 index 0000000..d72f627 --- /dev/null +++ b/tests/ssh2_shell_exec_environment_iteration.phpt @@ -0,0 +1,62 @@ +--TEST-- +Shell and exec environment arrays are traversed safely +--SKIPIF-- + +--FILE-- +value; + + return $this->value; + } +} + +$session = ssh2_connect(TEST_SSH2_HOSTNAME, TEST_SSH2_PORT); + +if ($session === false || !ssh2t_auth($session)) { + throw new RuntimeException('Failed to connect to the SSH fixture.'); +} + +$shell = ssh2_shell($session, 'xterm', [], 80, 24); + +var_dump(is_resource($shell)); + +if (is_resource($shell)) { + fclose($shell); +} + +$stream = @ssh2_exec($session, 'true', null, [ + 'PHP_SSH2_TEST_A' => new EnvironmentValue('alpha'), + 'PHP_SSH2_TEST_B' => new EnvironmentValue('bravo'), +]); + +var_dump(EnvironmentValue::$converted); + +if (is_resource($stream)) { + fclose($stream); +} + +ssh2_disconnect($session); +?> +--EXPECT-- +bool(true) +array(2) { + [0]=> + string(5) "alpha" + [1]=> + string(5) "bravo" +} diff --git a/tests/ssh2_shell_exec_environment_redaction.phpt b/tests/ssh2_shell_exec_environment_redaction.phpt new file mode 100644 index 0000000..1fb57b2 --- /dev/null +++ b/tests/ssh2_shell_exec_environment_redaction.phpt @@ -0,0 +1,65 @@ +--TEST-- +Shell and exec environment warnings do not expose values +--SKIPIF-- + +--FILE-- + $secret]; + $shell = ssh2_shell($session, 'xterm', $environment, 80, 24); + $stream = ssh2_exec($session, 'true', null, $environment); +} finally { + restore_error_handler(); +} + +var_dump(is_resource($shell)); +var_dump(is_resource($stream)); + +foreach ($warnings as $warning) { + echo $warning, PHP_EOL; +} + +var_dump(count($warnings)); +var_dump(str_contains(implode("\n", $warnings), $secret)); + +if (is_resource($shell)) { + fclose($shell); +} + +if (is_resource($stream)) { + fclose($stream); +} + +ssh2_disconnect($session); +?> +--EXPECT-- +bool(true) +bool(true) +ssh2_shell(): Failed setting environment variable PHP_SSH2_REDACTION_TEST on remote end +ssh2_exec(): Failed setting environment variable PHP_SSH2_REDACTION_TEST on remote end +int(2) +bool(false)