From d5bb57ecca979ae8f660d6c06b6e84c37d459bc4 Mon Sep 17 00:00:00 2001 From: Robert Date: Tue, 9 Jul 2024 12:25:20 -0400 Subject: [PATCH 01/10] Refactor to added helper enforcing message type expectation on retrieved packets, connection close on timeout --- phpseclib/Exception/TimeoutException.php | 10 ++ phpseclib/Net/SSH2.php | 143 ++++++++++------------- 2 files changed, 71 insertions(+), 82 deletions(-) create mode 100644 phpseclib/Exception/TimeoutException.php diff --git a/phpseclib/Exception/TimeoutException.php b/phpseclib/Exception/TimeoutException.php new file mode 100644 index 00000000..8701f8d7 --- /dev/null +++ b/phpseclib/Exception/TimeoutException.php @@ -0,0 +1,10 @@ +send_kex_first) { - $response = $this->get_binary_packet(); - - if (is_bool($response) || !strlen($response) || ord($response[0]) != NET_SSH2_MSG_KEXINIT) { - $this->bitmap = 0; - throw new \UnexpectedValueException('Expected SSH_MSG_KEXINIT'); - } - + $response = $this->get_binary_packet_or_close(NET_SSH2_MSG_KEXINIT); $this->key_exchange($response); } @@ -1607,17 +1602,7 @@ class SSH2 $this->send_binary_packet($kexinit_payload_client); $this->extra_packets = 0; - $kexinit_payload_server = $this->get_binary_packet(); - - if ( - is_bool($kexinit_payload_server) - || !strlen($kexinit_payload_server) - || ord($kexinit_payload_server[0]) != NET_SSH2_MSG_KEXINIT - ) { - $this->disconnect_helper(NET_SSH2_DISCONNECT_PROTOCOL_ERROR); - throw new \UnexpectedValueException('Expected SSH_MSG_KEXINIT'); - } - + $kexinit_payload_server = $this->get_binary_packet_or_close(NET_SSH2_MSG_KEXINIT); $send_kex = false; } @@ -1761,13 +1746,9 @@ class SSH2 $this->send_binary_packet($packet); $this->updateLogHistory('UNKNOWN (34)', 'NET_SSH2_MSG_KEXDH_GEX_REQUEST'); - $response = $this->get_binary_packet(); - + $response = $this->get_binary_packet_or_close(NET_SSH2_MSG_KEXDH_GEX_GROUP); list($type, $primeBytes, $gBytes) = Strings::unpackSSH2('Css', $response); - if ($type != NET_SSH2_MSG_KEXDH_GEX_GROUP) { - $this->disconnect_helper(NET_SSH2_DISCONNECT_PROTOCOL_ERROR); - throw new \UnexpectedValueException('Expected SSH_MSG_KEX_DH_GEX_GROUP'); - } + $this->updateLogHistory('NET_SSH2_MSG_KEXDH_REPLY', 'NET_SSH2_MSG_KEXDH_GEX_GROUP'); $prime = new BigInteger($primeBytes, -256); $g = new BigInteger($gBytes, -256); @@ -1806,7 +1787,7 @@ class SSH2 $this->updateLogHistory('UNKNOWN (32)', 'NET_SSH2_MSG_KEXDH_GEX_INIT'); } - $response = $this->get_binary_packet(); + $response = $this->get_binary_packet_or_close(constant($serverKexReplyMessage)); list( $type, @@ -1815,10 +1796,6 @@ class SSH2 $this->signature ) = Strings::unpackSSH2('Csss', $response); - if ($type != constant($serverKexReplyMessage)) { - $this->disconnect_helper(NET_SSH2_DISCONNECT_PROTOCOL_ERROR); - throw new \UnexpectedValueException("Expected $serverKexReplyMessage"); - } switch ($serverKexReplyMessage) { case 'NET_SSH2_MSG_KEX_ECDH_REPLY': $this->updateLogHistory('NET_SSH2_MSG_KEXDH_REPLY', 'NET_SSH2_MSG_KEX_ECDH_REPLY'); @@ -1885,19 +1862,7 @@ class SSH2 $packet = pack('C', NET_SSH2_MSG_NEWKEYS); $this->send_binary_packet($packet); - - $response = $this->get_binary_packet(); - - if ($response === false) { - $this->disconnect_helper(NET_SSH2_DISCONNECT_CONNECTION_LOST); - throw new ConnectionClosedException('Connection closed by server'); - } - - list($type) = Strings::unpackSSH2('C', $response); - if ($type != NET_SSH2_MSG_NEWKEYS) { - $this->disconnect_helper(NET_SSH2_DISCONNECT_PROTOCOL_ERROR); - throw new \UnexpectedValueException('Expected SSH_MSG_NEWKEYS'); - } + $response = $this->get_binary_packet_or_close(NET_SSH2_MSG_NEWKEYS); if (in_array('kex-strict-s-v00@openssh.com', $this->kex_algorithms)) { $this->get_seq_no = $this->send_seq_no = 0; @@ -2349,7 +2314,7 @@ class SSH2 $this->send_binary_packet($packet); try { - $response = $this->get_binary_packet(); + $response = $this->get_binary_packet_or_close(NET_SSH2_MSG_SERVICE_ACCEPT); } catch (InvalidPacketLengthException $e) { // the first opportunity to encounter the "bad key size" error if (!$this->bad_key_size_fix && $this->decryptName != null && self::bad_algorithm_candidate($this->decryptName)) { @@ -2360,15 +2325,12 @@ class SSH2 return $this->reconnect(); } throw $e; - } catch (\Exception $e) { - $this->disconnect_helper(NET_SSH2_DISCONNECT_CONNECTION_LOST); - throw $e; } list($type) = Strings::unpackSSH2('C', $response); list($service) = Strings::unpackSSH2('s', $response); - if ($type != NET_SSH2_MSG_SERVICE_ACCEPT || $service != 'ssh-userauth') { + if ($service != 'ssh-userauth') { $this->disconnect_helper(NET_SSH2_DISCONNECT_PROTOCOL_ERROR); throw new \UnexpectedValueException('Expected SSH_MSG_SERVICE_ACCEPT'); } @@ -2406,7 +2368,7 @@ class SSH2 $this->send_binary_packet($packet); - $response = $this->get_binary_packet(); + $response = $this->get_binary_packet_or_close(); list($type) = Strings::unpackSSH2('C', $response); switch ($type) { @@ -2449,10 +2411,8 @@ class SSH2 $this->send_binary_packet($packet, $logged); - $response = $this->get_binary_packet(); - if ($response === false) { - return false; - } + $response = $this->get_binary_packet_or_close(); + list($type) = Strings::unpackSSH2('C', $response); switch ($type) { case NET_SSH2_MSG_USERAUTH_PASSWD_CHANGEREQ: // in theory, the password can be changed @@ -2520,7 +2480,7 @@ class SSH2 if (strlen($this->last_interactive_response)) { $response = $this->last_interactive_response; } else { - $orig = $response = $this->get_binary_packet(); + $orig = $response = $this->get_binary_packet_or_close(); } list($type) = Strings::unpackSSH2('C', $response); @@ -2710,7 +2670,11 @@ class SSH2 $packet = $part1 . chr(0) . $part2; $this->send_binary_packet($packet); - $response = $this->get_binary_packet(); + $response = $this->get_binary_packet_or_close([ + NET_SSH2_MSG_USERAUTH_SUCCESS, + NET_SSH2_MSG_USERAUTH_FAILURE, + NET_SSH2_MSG_USERAUTH_PK_OK, + ]); list($type) = Strings::unpackSSH2('C', $response); switch ($type) { @@ -2731,9 +2695,6 @@ class SSH2 case NET_SSH2_MSG_USERAUTH_SUCCESS: $this->bitmap |= self::MASK_LOGIN; return true; - default: - $this->disconnect_helper(NET_SSH2_DISCONNECT_BY_APPLICATION); - throw new ConnectionClosedException('Unexpected response to publickey authentication pt 1'); } $packet = $part1 . chr(1) . $part2; @@ -2746,7 +2707,10 @@ class SSH2 $this->send_binary_packet($packet); - $response = $this->get_binary_packet(); + $response = $this->get_binary_packet_or_close([ + NET_SSH2_MSG_USERAUTH_SUCCESS, + NET_SSH2_MSG_USERAUTH_FAILURE, + ]); list($type) = Strings::unpackSSH2('C', $response); switch ($type) { @@ -2759,9 +2723,6 @@ class SSH2 $this->bitmap |= self::MASK_LOGIN; return true; } - - $this->disconnect_helper(NET_SSH2_DISCONNECT_BY_APPLICATION); - throw new ConnectionClosedException('Unexpected response to publickey authentication pt 2'); } /** @@ -3509,13 +3470,41 @@ class SSH2 return [$sec, $usec]; } + /** + * Retrieves the next packet with added timeout and type handling + * + * @param string|string[]|null $message_types Message types to enforce in response, closing if not met + * @return string + * @throws ConnectionClosedException If an error has occurred preventing read of the next packet + */ + private function get_binary_packet_or_close($message_types = null) + { + if (is_int($message_types)) { + $message_types = [$message_types]; + } + try { + $packet = $this->get_binary_packet(); + if (is_array($message_types) && !in_array(ord($packet[0]), $message_types)) { + $this->disconnect_helper(NET_SSH2_DISCONNECT_PROTOCOL_ERROR); + throw new ConnectionClosedException('Bad message type. Expected: #' + . implode(', #', $message_types) . '. Got: #' . ord($packet[0])); + } + return $packet; + } catch (TimeoutException $e) { + $this->disconnect_helper(NET_SSH2_DISCONNECT_BY_APPLICATION); + throw new ConnectionClosedException('Connection closed due to timeout'); + } + } + /** * Gets Binary Packets * * See '6. Binary Packet Protocol' of rfc4253 for more info. * * @see self::_send_binary_packet() - * @return bool|string + * @return string + * @throws TimeoutException If user requested timeout was reached while waiting for next packet + * @throws ConnectionClosedException If an error has occurred preventing read of the next packet */ private function get_binary_packet() { @@ -3541,7 +3530,7 @@ class SSH2 } if ($this->curTimeout < 0) { $this->is_timeout = true; - return true; + throw new TimeoutException('Timed out waiting for server'); } $this->send_keep_alive(); @@ -3782,8 +3771,8 @@ class SSH2 Strings::shift($payload, 1); list($reason_code, $message) = Strings::unpackSSH2('Ns', $payload); $this->errors[] = 'SSH_MSG_DISCONNECT: ' . self::$disconnect_reasons[$reason_code] . "\r\n$message"; - $this->bitmap = 0; - return false; + $this->disconnect_helper(NET_SSH2_DISCONNECT_CONNECTION_LOST); + throw new ConnectionClosedException('Connection closed by server'); case NET_SSH2_MSG_IGNORE: $this->extra_packets++; $payload = $this->get_binary_packet(); @@ -3796,13 +3785,13 @@ class SSH2 $payload = $this->get_binary_packet(); break; case NET_SSH2_MSG_UNIMPLEMENTED: - return false; + break; // return payload case NET_SSH2_MSG_KEXINIT: // this is here for key re-exchanges after the initial key exchange if ($this->session_id !== false) { if (!$this->key_exchange($payload)) { - $this->bitmap = 0; - return false; + $this->disconnect_helper(NET_SSH2_DISCONNECT_KEY_EXCHANGE_FAILED); + throw new ConnectionClosedException('Key exchange failed'); } $payload = $this->get_binary_packet(); } @@ -3820,7 +3809,7 @@ class SSH2 } // see http://tools.ietf.org/html/rfc4252#section-5.4; only called when the encryption has been activated and when we haven't already logged in - if (($this->bitmap & self::MASK_CONNECTED) && !$this->isAuthenticated() && !is_bool($payload) && ord($payload[0]) == NET_SSH2_MSG_USERAUTH_BANNER) { + if (($this->bitmap & self::MASK_CONNECTED) && !$this->isAuthenticated() && ord($payload[0]) == NET_SSH2_MSG_USERAUTH_BANNER) { Strings::shift($payload, 1); list($this->banner_message) = Strings::unpackSSH2('s', $payload); $payload = $this->get_binary_packet(); @@ -3828,10 +3817,6 @@ class SSH2 // only called when we've already logged in if (($this->bitmap & self::MASK_CONNECTED) && $this->isAuthenticated()) { - if (is_bool($payload)) { - return $payload; - } - switch (ord($payload[0])) { case NET_SSH2_MSG_CHANNEL_REQUEST: if (strlen($payload) == 31) { @@ -4030,20 +4015,14 @@ class SSH2 } while (true) { - $response = $this->get_binary_packet(); - if ($response === true && $this->is_timeout) { + try { + $response = $this->get_binary_packet(); + } catch (TimeoutException $e) { if ($client_channel == self::CHANNEL_EXEC && !$this->request_pty) { $this->close_channel($client_channel); } return true; } - if ($response === false) { - $this->disconnect_helper(NET_SSH2_DISCONNECT_CONNECTION_LOST); - throw new ConnectionClosedException('Connection closed by server'); - } - if ($client_channel == -1 && $response === true) { - return true; - } list($type, $channel) = Strings::unpackSSH2('CN', $response); // will not be setup yet on incoming channel open request From 1617746239cff904b8acf6cae14d4204e7d18a2b Mon Sep 17 00:00:00 2001 From: Robert Date: Tue, 9 Jul 2024 15:02:58 -0400 Subject: [PATCH 02/10] Check packet size before extracting channel --- phpseclib/Net/SSH2.php | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/phpseclib/Net/SSH2.php b/phpseclib/Net/SSH2.php index cd813c6f..b68e6cdd 100644 --- a/phpseclib/Net/SSH2.php +++ b/phpseclib/Net/SSH2.php @@ -1748,7 +1748,6 @@ class SSH2 $response = $this->get_binary_packet_or_close(NET_SSH2_MSG_KEXDH_GEX_GROUP); list($type, $primeBytes, $gBytes) = Strings::unpackSSH2('Css', $response); - $this->updateLogHistory('NET_SSH2_MSG_KEXDH_REPLY', 'NET_SSH2_MSG_KEXDH_GEX_GROUP'); $prime = new BigInteger($primeBytes, -256); $g = new BigInteger($gBytes, -256); @@ -2412,7 +2411,6 @@ class SSH2 $this->send_binary_packet($packet, $logged); $response = $this->get_binary_packet_or_close(); - list($type) = Strings::unpackSSH2('C', $response); switch ($type) { case NET_SSH2_MSG_USERAUTH_PASSWD_CHANGEREQ: // in theory, the password can be changed @@ -4023,7 +4021,10 @@ class SSH2 } return true; } - list($type, $channel) = Strings::unpackSSH2('CN', $response); + list($type) = Strings::unpackSSH2('C', $response); + if (strlen($response) >= 4) { + list($channel) = Strings::unpackSSH2('N', $response); + } // will not be setup yet on incoming channel open request if (isset($channel) && isset($this->channel_status[$channel]) && isset($this->window_size_server_to_client[$channel])) { From 51a3c5f050c8e319c5580d3c201db1a4117e3b96 Mon Sep 17 00:00:00 2001 From: Robert Date: Tue, 9 Jul 2024 15:56:49 -0400 Subject: [PATCH 03/10] Remove remaining boolean retval and runtime exception handling in filter() --- phpseclib/Net/SSH2.php | 21 +++++---------------- 1 file changed, 5 insertions(+), 16 deletions(-) diff --git a/phpseclib/Net/SSH2.php b/phpseclib/Net/SSH2.php index b68e6cdd..e6b95ae3 100644 --- a/phpseclib/Net/SSH2.php +++ b/phpseclib/Net/SSH2.php @@ -3760,7 +3760,7 @@ class SSH2 * * @see self::_get_binary_packet() * @param string $payload - * @return string|bool + * @return string */ private function filter($payload) { @@ -3831,13 +3831,7 @@ class SSH2 Strings::shift($payload, 1); list($request_name) = Strings::unpackSSH2('s', $payload); $this->errors[] = "SSH_MSG_GLOBAL_REQUEST: $request_name"; - - try { - $this->send_binary_packet(pack('C', NET_SSH2_MSG_REQUEST_FAILURE)); - } catch (\RuntimeException $e) { - return $this->disconnect_helper(NET_SSH2_DISCONNECT_BY_APPLICATION); - } - + $this->send_binary_packet(pack('C', NET_SSH2_MSG_REQUEST_FAILURE)); $payload = $this->get_binary_packet(); break; case NET_SSH2_MSG_CHANNEL_OPEN: // see http://tools.ietf.org/html/rfc4254#section-5.1 @@ -3883,12 +3877,7 @@ class SSH2 '', // description '' // language tag ); - - try { - $this->send_binary_packet($packet); - } catch (\RuntimeException $e) { - return $this->disconnect_helper(NET_SSH2_DISCONNECT_BY_APPLICATION); - } + $this->send_binary_packet($packet); } $payload = $this->get_binary_packet(); @@ -4214,7 +4203,7 @@ class SSH2 protected function send_binary_packet($data, $logged = null) { if (!is_resource($this->fsock) || feof($this->fsock)) { - $this->bitmap = 0; + $this->disconnect_helper(NET_SSH2_DISCONNECT_CONNECTION_LOST); throw new ConnectionClosedException('Connection closed prematurely'); } @@ -4342,7 +4331,7 @@ class SSH2 $this->last_packet = microtime(true); if (strlen($packet) != $sent) { - $this->bitmap = 0; + $this->disconnect_helper(NET_SSH2_DISCONNECT_BY_APPLICATION); $message = $sent === false ? 'Unable to write ' . strlen($packet) . ' bytes' : "Only $sent of " . strlen($packet) . " bytes were sent"; From 35fcd1984bc022bbe539af760aeea752f93c94ca Mon Sep 17 00:00:00 2001 From: Robert Date: Fri, 12 Jul 2024 14:49:11 -0400 Subject: [PATCH 04/10] Remove yet another remaining boolean retval, relocating window adjust recv packet handling to get_channel_packet. Ensuring window is adjusted for empty channel prior to send. --- phpseclib/Net/SSH2.php | 34 +++++++++++++++++++++------------- 1 file changed, 21 insertions(+), 13 deletions(-) diff --git a/phpseclib/Net/SSH2.php b/phpseclib/Net/SSH2.php index e6b95ae3..7aa6a111 100644 --- a/phpseclib/Net/SSH2.php +++ b/phpseclib/Net/SSH2.php @@ -104,7 +104,6 @@ class SSH2 const MASK_LOGIN_REQ = 0x00000004; const MASK_LOGIN = 0x00000008; const MASK_SHELL = 0x00000010; - const MASK_WINDOW_ADJUST = 0x00000020; /* * Channel constants @@ -3157,6 +3156,7 @@ class SSH2 * @return void * @throws \RuntimeException on connection error * @throws InsufficientSetupException on unexpected channel status, possibly due to closure + * @throws TimeoutException if the write could not be completed within the requested self::setTimeout() */ public function write($cmd, $channel = null) { @@ -3176,6 +3176,8 @@ class SSH2 } } + $this->curTimeout = $this->timeout; + $this->is_timeout = false; $this->send_channel_packet($channel, $cmd); } @@ -3882,13 +3884,6 @@ class SSH2 $payload = $this->get_binary_packet(); break; - case NET_SSH2_MSG_CHANNEL_WINDOW_ADJUST: - Strings::shift($payload, 1); - list($channel, $window_size) = Strings::unpackSSH2('NN', $payload); - - $this->window_size_client_to_server[$channel] += $window_size; - - $payload = ($this->bitmap & self::MASK_WINDOW_ADJUST) ? true : $this->get_binary_packet(); } } @@ -3969,6 +3964,7 @@ class SSH2 * * - the server closes the channel * - if the connection times out + * - if a window adjust packet is received on the given negated client channel * - if the channel status is CHANNEL_OPEN and the response was CHANNEL_OPEN_CONFIRMATION * - if the channel status is CHANNEL_REQUEST and the response was CHANNEL_SUCCESS * - if the channel status is CHANNEL_CLOSE and the response was CHANNEL_CLOSE @@ -3977,7 +3973,10 @@ class SSH2 * * - if the channel status is CHANNEL_REQUEST and the response was CHANNEL_FAILURE * - * @param int $client_channel + * @param int $client_channel Specifies the channel to return data for, and data received + * on other channels is buffered. The respective negative value of a channel is + * also supported for the case that the caller is awaiting adjustment of the data + * window, and where data received on that respective channel is also buffered. * @param bool $skip_extended * @return mixed * @throws \RuntimeException on connection error @@ -4029,6 +4028,14 @@ class SSH2 } switch ($type) { + case NET_SSH2_MSG_CHANNEL_WINDOW_ADJUST: + list($window_size) = Strings::unpackSSH2('N', $response); + $this->window_size_client_to_server[$channel] += $window_size; + if ($channel == -$client_channel) { + return true; + } + + continue 2; case NET_SSH2_MSG_CHANNEL_EXTENDED_DATA: /* if ($client_channel == self::CHANNEL_EXEC) { @@ -4474,11 +4481,12 @@ class SSH2 protected function send_channel_packet($client_channel, $data) { while (strlen($data)) { - if (!$this->window_size_client_to_server[$client_channel]) { - $this->bitmap ^= self::MASK_WINDOW_ADJUST; + while (!$this->window_size_client_to_server[$client_channel]) { + if ($this->isTimeout()) { + throw new TimeoutException('Timed out waiting for server'); + } // using an invalid channel will let the buffers be built up for the valid channels - $this->get_channel_packet(-1); - $this->bitmap ^= self::MASK_WINDOW_ADJUST; + $this->get_channel_packet(-$client_channel); } /* The maximum amount of data allowed is determined by the maximum From d9d2ba59cd31b9e950cf77d0ad8d1abefac0fb29 Mon Sep 17 00:00:00 2001 From: Robert Date: Fri, 12 Jul 2024 15:01:35 -0400 Subject: [PATCH 05/10] Reset is_timeout when sending and receiving SFTP packets --- phpseclib/Net/SFTP.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/phpseclib/Net/SFTP.php b/phpseclib/Net/SFTP.php index 5731170e..be243259 100644 --- a/phpseclib/Net/SFTP.php +++ b/phpseclib/Net/SFTP.php @@ -3269,6 +3269,7 @@ class SFTP extends SSH2 // in SSH2.php the timeout is cumulative per function call. eg. exec() will // timeout after 10s. but for SFTP.php it's cumulative per packet $this->curTimeout = $this->timeout; + $this->is_timeout = false; $packet = $this->use_request_id ? pack('NCNa*', strlen($data) + 5, $type, $request_id, $data) : @@ -3331,6 +3332,7 @@ class SFTP extends SSH2 // in SSH2.php the timeout is cumulative per function call. eg. exec() will // timeout after 10s. but for SFTP.php it's cumulative per packet $this->curTimeout = $this->timeout; + $this->is_timeout = false; $start = microtime(true); From 18d4c79bd46bdd257d3580385d3c67f4cf5966ef Mon Sep 17 00:00:00 2001 From: Robert Date: Fri, 12 Jul 2024 15:30:43 -0400 Subject: [PATCH 06/10] Revert risky while-loop change, too fancy --- phpseclib/Net/SSH2.php | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/phpseclib/Net/SSH2.php b/phpseclib/Net/SSH2.php index 7aa6a111..088713e8 100644 --- a/phpseclib/Net/SSH2.php +++ b/phpseclib/Net/SSH2.php @@ -4481,12 +4481,14 @@ class SSH2 protected function send_channel_packet($client_channel, $data) { while (strlen($data)) { - while (!$this->window_size_client_to_server[$client_channel]) { - if ($this->isTimeout()) { - throw new TimeoutException('Timed out waiting for server'); - } + if (!$this->window_size_client_to_server[$client_channel]) { // using an invalid channel will let the buffers be built up for the valid channels $this->get_channel_packet(-$client_channel); + if ($this->isTimeout()) { + throw new TimeoutException('Timed out waiting for server'); + } elseif (!$this->window_size_client_to_server[$client_channel]) { + throw new \RuntimeException('Client to server window was not adjusted'); + } } /* The maximum amount of data allowed is determined by the maximum From e401ee05f5e984f28a3f638ae6ca5b5848d9dfd1 Mon Sep 17 00:00:00 2001 From: Robert Date: Fri, 12 Jul 2024 16:40:13 -0400 Subject: [PATCH 07/10] Introduce buffering to send channel packet for capability to resume across timeout --- phpseclib/Net/SSH2.php | 27 +++++++- tests/Unit/Net/SSH2UnitTest.php | 113 ++++++++++++++++++++++++++++++++ 2 files changed, 138 insertions(+), 2 deletions(-) diff --git a/phpseclib/Net/SSH2.php b/phpseclib/Net/SSH2.php index 088713e8..df8fd3d7 100644 --- a/phpseclib/Net/SSH2.php +++ b/phpseclib/Net/SSH2.php @@ -626,7 +626,7 @@ class SSH2 protected $server_channels = []; /** - * Channel Buffers + * Channel Read Buffers * * If a client requests a packet from one channel but receives two packets from another those packets should * be placed in a buffer @@ -637,6 +637,17 @@ class SSH2 */ private $channel_buffers = []; + /** + * Channel Write Buffers + * + * If a client sends a packet and receives a timeout error mid-transmission, buffer the data written so it + * can be de-duplicated upon resuming write + * + * @see self::send_channel_packet() + * @var array + */ + private $channel_buffers_write = []; + /** * Channel Status * @@ -3446,6 +3457,8 @@ class SSH2 $this->get_seq_no = $this->send_seq_no = 0; $this->channel_status = []; $this->channel_id_last_interactive = 0; + $this->channel_buffers = []; + $this->channel_buffers_write = []; } /** @@ -4480,6 +4493,14 @@ class SSH2 */ protected function send_channel_packet($client_channel, $data) { + if (isset($this->channel_buffers_write[$client_channel]) + && strpos($data, $this->channel_buffers_write[$client_channel]) === 0 + ) { + // if buffer holds identical initial data content, resume send from the unmatched data portion + $data = substr($data, strlen($this->channel_buffers_write[$client_channel])); + } else { + $this->channel_buffers_write[$client_channel] = ''; + } while (strlen($data)) { if (!$this->window_size_client_to_server[$client_channel]) { // using an invalid channel will let the buffers be built up for the valid channels @@ -4487,7 +4508,7 @@ class SSH2 if ($this->isTimeout()) { throw new TimeoutException('Timed out waiting for server'); } elseif (!$this->window_size_client_to_server[$client_channel]) { - throw new \RuntimeException('Client to server window was not adjusted'); + throw new \RuntimeException('Data window was not adjusted'); } } @@ -4509,7 +4530,9 @@ class SSH2 ); $this->window_size_client_to_server[$client_channel] -= strlen($temp); $this->send_binary_packet($packet); + $this->channel_buffers_write[$client_channel] .= $temp; } + unset($this->channel_buffers_write[$client_channel]); } /** diff --git a/tests/Unit/Net/SSH2UnitTest.php b/tests/Unit/Net/SSH2UnitTest.php index 0d8a1817..2f285012 100644 --- a/tests/Unit/Net/SSH2UnitTest.php +++ b/tests/Unit/Net/SSH2UnitTest.php @@ -8,8 +8,11 @@ namespace phpseclib3\Tests\Unit\Net; +use phpseclib3\Common\Functions\Strings; use phpseclib3\Exception\InsufficientSetupException; +use phpseclib3\Exception\TimeoutException; use phpseclib3\Net\SSH2; +use phpseclib3\Net\SSH2\MessageType; use phpseclib3\Tests\PhpseclibTestCase; class SSH2UnitTest extends PhpseclibTestCase @@ -271,6 +274,116 @@ class SSH2UnitTest extends PhpseclibTestCase $this->assertEquals([0, 0], self::callFunc($ssh, 'get_stream_timeout')); } + /** + * @requires PHPUnit < 10 + */ + public function testSendChannelPacketNoBufferedData() + { + $ssh = $this->getMockBuilder('phpseclib3\Net\SSH2') + ->disableOriginalConstructor() + ->setMethods(['get_channel_packet', 'send_binary_packet']) + ->getMock(); + $ssh->expects($this->once()) + ->method('get_channel_packet') + ->with(-1) + ->willReturnCallback(function () use ($ssh) { + self::setVar($ssh, 'window_size_client_to_server', [1 => 0x7FFFFFFF]); + }); + $ssh->expects($this->once()) + ->method('send_binary_packet') + ->with(Strings::packSSH2('CNs', MessageType::CHANNEL_DATA, 1, 'hello world')); + self::setVar($ssh, 'server_channels', [1 => 1]); + self::setVar($ssh, 'packet_size_client_to_server', [1 => 0x7FFFFFFF]); + self::setVar($ssh, 'window_size_client_to_server', [1 => 0]); + self::setVar($ssh, 'window_size_server_to_client', [1 => 0x7FFFFFFF]); + + self::callFunc($ssh, 'send_channel_packet', [1, 'hello world']); + $this->assertEmpty(self::getVar($ssh, 'channel_buffers_write')); + } + + /** + * @requires PHPUnit < 10 + */ + public function testSendChannelPacketBufferedData() + { + $ssh = $this->getMockBuilder('phpseclib3\Net\SSH2') + ->disableOriginalConstructor() + ->setMethods(['get_channel_packet', 'send_binary_packet']) + ->getMock(); + $ssh->expects($this->once()) + ->method('get_channel_packet') + ->with(-1) + ->willReturnCallback(function () use ($ssh) { + self::setVar($ssh, 'window_size_client_to_server', [1 => 0x7FFFFFFF]); + }); + $ssh->expects($this->once()) + ->method('send_binary_packet') + ->with(Strings::packSSH2('CNs', MessageType::CHANNEL_DATA, 1, ' world')); + self::setVar($ssh, 'channel_buffers_write', [1 => 'hello']); + self::setVar($ssh, 'server_channels', [1 => 1]); + self::setVar($ssh, 'packet_size_client_to_server', [1 => 0x7FFFFFFF]); + self::setVar($ssh, 'window_size_client_to_server', [1 => 0]); + self::setVar($ssh, 'window_size_server_to_client', [1 => 0x7FFFFFFF]); + + self::callFunc($ssh, 'send_channel_packet', [1, 'hello world']); + $this->assertEmpty(self::getVar($ssh, 'channel_buffers_write')); + } + + /** + * @requires PHPUnit < 10 + */ + public function testSendChannelPacketTimeout() + { + $this->expectException(TimeoutException::class); + $this->expectExceptionMessage('Timed out waiting for server'); + + $ssh = $this->getMockBuilder('phpseclib3\Net\SSH2') + ->disableOriginalConstructor() + ->setMethods(['get_channel_packet', 'send_binary_packet']) + ->getMock(); + $ssh->expects($this->once()) + ->method('get_channel_packet') + ->with(-1) + ->willReturnCallback(function () use ($ssh) { + self::setVar($ssh, 'is_timeout', true); + }); + $ssh->expects($this->once()) + ->method('send_binary_packet') + ->with(Strings::packSSH2('CNs', MessageType::CHANNEL_DATA, 1, 'hello')); + self::setVar($ssh, 'server_channels', [1 => 1]); + self::setVar($ssh, 'packet_size_client_to_server', [1 => 0x7FFFFFFF]); + self::setVar($ssh, 'window_size_client_to_server', [1 => 5]); + self::setVar($ssh, 'window_size_server_to_client', [1 => 0x7FFFFFFF]); + + self::callFunc($ssh, 'send_channel_packet', [1, 'hello world']); + $this->assertEquals([1 => 'hello'], self::getVar($ssh, 'channel_buffers_write')); + } + + /** + * @requires PHPUnit < 10 + */ + public function testSendChannelPacketNoWindowAdjustment() + { + $this->expectException(\RuntimeException::class); + $this->expectExceptionMessage('Data window was not adjusted'); + + $ssh = $this->getMockBuilder('phpseclib3\Net\SSH2') + ->disableOriginalConstructor() + ->setMethods(['get_channel_packet', 'send_binary_packet']) + ->getMock(); + $ssh->expects($this->once()) + ->method('get_channel_packet') + ->with(-1); + $ssh->expects($this->never()) + ->method('send_binary_packet'); + self::setVar($ssh, 'server_channels', [1 => 1]); + self::setVar($ssh, 'packet_size_client_to_server', [1 => 0x7FFFFFFF]); + self::setVar($ssh, 'window_size_client_to_server', [1 => 0]); + self::setVar($ssh, 'window_size_server_to_client', [1 => 0x7FFFFFFF]); + + self::callFunc($ssh, 'send_channel_packet', [1, 'hello world']); + } + /** * @return \phpseclib3\Net\SSH2 */ From c1e69ddb7950c7f980e184a66b30ef147258b778 Mon Sep 17 00:00:00 2001 From: Robert Date: Fri, 12 Jul 2024 17:14:54 -0400 Subject: [PATCH 08/10] Remove use of master MessageType --- tests/Unit/Net/SSH2UnitTest.php | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/tests/Unit/Net/SSH2UnitTest.php b/tests/Unit/Net/SSH2UnitTest.php index 2f285012..d8d3951d 100644 --- a/tests/Unit/Net/SSH2UnitTest.php +++ b/tests/Unit/Net/SSH2UnitTest.php @@ -12,7 +12,6 @@ use phpseclib3\Common\Functions\Strings; use phpseclib3\Exception\InsufficientSetupException; use phpseclib3\Exception\TimeoutException; use phpseclib3\Net\SSH2; -use phpseclib3\Net\SSH2\MessageType; use phpseclib3\Tests\PhpseclibTestCase; class SSH2UnitTest extends PhpseclibTestCase @@ -291,7 +290,7 @@ class SSH2UnitTest extends PhpseclibTestCase }); $ssh->expects($this->once()) ->method('send_binary_packet') - ->with(Strings::packSSH2('CNs', MessageType::CHANNEL_DATA, 1, 'hello world')); + ->with(Strings::packSSH2('CNs', NET_SSH2_MSG_CHANNEL_DATA, 1, 'hello world')); self::setVar($ssh, 'server_channels', [1 => 1]); self::setVar($ssh, 'packet_size_client_to_server', [1 => 0x7FFFFFFF]); self::setVar($ssh, 'window_size_client_to_server', [1 => 0]); @@ -318,7 +317,7 @@ class SSH2UnitTest extends PhpseclibTestCase }); $ssh->expects($this->once()) ->method('send_binary_packet') - ->with(Strings::packSSH2('CNs', MessageType::CHANNEL_DATA, 1, ' world')); + ->with(Strings::packSSH2('CNs', NET_SSH2_MSG_CHANNEL_DATA, 1, ' world')); self::setVar($ssh, 'channel_buffers_write', [1 => 'hello']); self::setVar($ssh, 'server_channels', [1 => 1]); self::setVar($ssh, 'packet_size_client_to_server', [1 => 0x7FFFFFFF]); @@ -349,7 +348,7 @@ class SSH2UnitTest extends PhpseclibTestCase }); $ssh->expects($this->once()) ->method('send_binary_packet') - ->with(Strings::packSSH2('CNs', MessageType::CHANNEL_DATA, 1, 'hello')); + ->with(Strings::packSSH2('CNs', NET_SSH2_MSG_CHANNEL_DATA, 1, 'hello')); self::setVar($ssh, 'server_channels', [1 => 1]); self::setVar($ssh, 'packet_size_client_to_server', [1 => 0x7FFFFFFF]); self::setVar($ssh, 'window_size_client_to_server', [1 => 5]); From cc7fdd59e0ae902f95bf37b5e05931d2d286da31 Mon Sep 17 00:00:00 2001 From: Robert Date: Wed, 24 Jul 2024 09:25:45 -0400 Subject: [PATCH 09/10] Improve get binary packet helper with variadic argument --- phpseclib/Net/SSH2.php | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/phpseclib/Net/SSH2.php b/phpseclib/Net/SSH2.php index df8fd3d7..5a9e0872 100644 --- a/phpseclib/Net/SSH2.php +++ b/phpseclib/Net/SSH2.php @@ -2678,11 +2678,11 @@ class SSH2 $packet = $part1 . chr(0) . $part2; $this->send_binary_packet($packet); - $response = $this->get_binary_packet_or_close([ + $response = $this->get_binary_packet_or_close( NET_SSH2_MSG_USERAUTH_SUCCESS, NET_SSH2_MSG_USERAUTH_FAILURE, NET_SSH2_MSG_USERAUTH_PK_OK, - ]); + ); list($type) = Strings::unpackSSH2('C', $response); switch ($type) { @@ -2715,10 +2715,10 @@ class SSH2 $this->send_binary_packet($packet); - $response = $this->get_binary_packet_or_close([ + $response = $this->get_binary_packet_or_close( NET_SSH2_MSG_USERAUTH_SUCCESS, NET_SSH2_MSG_USERAUTH_FAILURE, - ]); + ); list($type) = Strings::unpackSSH2('C', $response); switch ($type) { @@ -3486,18 +3486,15 @@ class SSH2 /** * Retrieves the next packet with added timeout and type handling * - * @param string|string[]|null $message_types Message types to enforce in response, closing if not met + * @param string $message_types Message types to enforce in response, closing if not met * @return string * @throws ConnectionClosedException If an error has occurred preventing read of the next packet */ - private function get_binary_packet_or_close($message_types = null) + private function get_binary_packet_or_close(...$message_types) { - if (is_int($message_types)) { - $message_types = [$message_types]; - } try { $packet = $this->get_binary_packet(); - if (is_array($message_types) && !in_array(ord($packet[0]), $message_types)) { + if (count($message_types) > 0 && !in_array(ord($packet[0]), $message_types)) { $this->disconnect_helper(NET_SSH2_DISCONNECT_PROTOCOL_ERROR); throw new ConnectionClosedException('Bad message type. Expected: #' . implode(', #', $message_types) . '. Got: #' . ord($packet[0])); From e5d94c817c26de42470ae146a322c3e8ceaa7633 Mon Sep 17 00:00:00 2001 From: Robert Date: Wed, 24 Jul 2024 09:27:44 -0400 Subject: [PATCH 10/10] Remove trailing commas in modified method calls --- phpseclib/Net/SSH2.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/phpseclib/Net/SSH2.php b/phpseclib/Net/SSH2.php index 5a9e0872..4bd4c9ee 100644 --- a/phpseclib/Net/SSH2.php +++ b/phpseclib/Net/SSH2.php @@ -2681,7 +2681,7 @@ class SSH2 $response = $this->get_binary_packet_or_close( NET_SSH2_MSG_USERAUTH_SUCCESS, NET_SSH2_MSG_USERAUTH_FAILURE, - NET_SSH2_MSG_USERAUTH_PK_OK, + NET_SSH2_MSG_USERAUTH_PK_OK ); list($type) = Strings::unpackSSH2('C', $response); @@ -2717,7 +2717,7 @@ class SSH2 $response = $this->get_binary_packet_or_close( NET_SSH2_MSG_USERAUTH_SUCCESS, - NET_SSH2_MSG_USERAUTH_FAILURE, + NET_SSH2_MSG_USERAUTH_FAILURE ); list($type) = Strings::unpackSSH2('C', $response);