[pecl-networking-ssh2] master: fix: correct SSH2 shell and exec environments

Author: Raj Siva-Rajah (binaryfire) Pusher: langemeijer Date: 2026-07-21T20:31:42Z Commit: https://github.com/php/pecl-networking-ssh2/commit/1ed821258cab58c8e7cc4720fa652ae195de483f Raw diff: https://github.com/php/pecl-networking-ssh2/commit/1ed821258cab58c8e7cc4720fa652ae195de483f.diff 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. Changed paths: A tests/ssh2_shell_arguments.phpt A tests/ssh2_shell_exec_environment_iteration.phpt A tests/ssh2_shell_exec_environment_redaction.phpt M package.xml M ssh2_fopen_wrappers.c Diff: diff --git a/package.xml b/package.xml index 07c0427..2faf69f 100644 --- a/package.xml +++ b/package.xml @@ -82,6 +82,9 @@ <file role="test" name="ssh2_sftp_001.phpt"/> <file role="test" name="ssh2_sftp_002.phpt"/> <file role="test" name="ssh2_shell.phpt"/> + <file role="test" name="ssh2_shell_arguments.phpt"/> + <file role="test" name="ssh2_shell_exec_environment_iteration.phpt"/> + <file role="test" name="ssh2_shell_exec_environment_redaction.phpt"/> <file role="test" name="ssh2_skip.inc"/> <file role="test" name="ssh2_stream_select.phpt"/> <file role="test" name="ssh2_test.inc"/> 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(&copyval); - convert_to_string(&copyval); - 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(&copyval); + zval *value; + + ZEND_HASH_FOREACH_STR_KEY_VAL(Z_ARRVAL_P(environment), key, value) { + if (key) { + zval copyval = *value; + + zval_copy_ctor(&copyval); + convert_to_string(&copyval); + 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(&copyval); } 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(&copyval); - convert_to_string(&copyval); - 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(&copyval); + 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(&copyval); + convert_to_string(&copyval); + 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(&copyval); } 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-- +<?php +require 'ssh2_skip.inc'; +ssh2t_needs_auth(); +?> +--FILE-- +<?php +require 'ssh2_test.inc'; + +$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); +} + +var_dump(ssh2_shell($session, 'xterm', [], 80)); + +ssh2_disconnect($session); +?> +--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-- +<?php +require 'ssh2_skip.inc'; +ssh2t_needs_auth(); +?> +--FILE-- +<?php +require 'ssh2_test.inc'; + +final class EnvironmentValue +{ + public static array $converted = []; + + public function __construct(private string $value) + { + } + + public function __toString(): string + { + self::$converted[] = $this->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-- +<?php +require 'ssh2_skip.inc'; +ssh2t_needs_auth(); +?> +--FILE-- +<?php +require 'ssh2_test.inc'; + +$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.'); +} + +$secret = 'secret-value-must-not-appear'; +$warnings = []; + +set_error_handler(static function (int $severity, string $message) use (&$warnings): bool { + if ($severity !== E_WARNING || !str_contains($message, 'Failed setting environment variable')) { + return false; + } + + $warnings[] = $message; + + return true; +}); + +try { + $environment = ['PHP_SSH2_REDACTION_TEST' => $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)