From 6fc9a98d42fa5c53515b435a629299ee4f08ccec Mon Sep 17 00:00:00 2001 From: Robert Date: Tue, 28 May 2024 11:19:37 -0400 Subject: [PATCH 1/3] Reorganize get_binary_packet to fetch entire payload before decrypt processing, buffering for graceful handling across timeouts. Remove skip filter parameter from method signatures, now technically defunct as all requests through get_binary_packet incorporate the same timeout during blocking IO calls. Introduce InvalidPacketLength exception and employ to detect OpenSSL 0.9.8e flaw at higher logical level than binary packet processing. Removing case logic in binary packet filtering for channel message types, made extraneous by use of get_channel_packet, and possibly leading to discarded data packets. Reset connection properties during disconnect. Rework callers of reset_connection to use disconnect_helper. Bugfix for no encyrption algorithms negotiated with server. --- .../InvalidPacketLengthException.php | 10 + phpseclib/Net/SFTP.php | 6 +- phpseclib/Net/SSH2.php | 462 ++++++++---------- tests/Functional/Net/SSH2Test.php | 93 ++++ 4 files changed, 320 insertions(+), 251 deletions(-) create mode 100644 phpseclib/Exception/InvalidPacketLengthException.php diff --git a/phpseclib/Exception/InvalidPacketLengthException.php b/phpseclib/Exception/InvalidPacketLengthException.php new file mode 100644 index 00000000..b96ead1e --- /dev/null +++ b/phpseclib/Exception/InvalidPacketLengthException.php @@ -0,0 +1,10 @@ +reset_sftp(); } diff --git a/phpseclib/Net/SSH2.php b/phpseclib/Net/SSH2.php index ef16cd32..1bed4dfd 100644 --- a/phpseclib/Net/SSH2.php +++ b/phpseclib/Net/SSH2.php @@ -64,6 +64,7 @@ use phpseclib3\Crypt\TripleDES; // Used to do Diffie-Hellman key exchange and DS use phpseclib3\Crypt\Twofish; use phpseclib3\Exception\ConnectionClosedException; use phpseclib3\Exception\InsufficientSetupException; +use phpseclib3\Exception\InvalidPacketLengthException; use phpseclib3\Exception\NoSupportedAlgorithmsException; use phpseclib3\Exception\UnableToConnectException; use phpseclib3\Exception\UnsupportedAlgorithmException; @@ -784,6 +785,14 @@ class SSH2 */ private $keepAlive; + /** + * Timestamp for the last sent keep alive message + * + * @see self::send_keep_alive() + * @var float|null + */ + private $keep_alive_sent = null; + /** * Real-time log file pointer * @@ -1005,9 +1014,9 @@ class SSH2 /** * Binary Packet Buffer * - * @var string|false + * @var object|null */ - private $binary_packet_buffer = false; + private $binary_packet_buffer = null; /** * Preferred Signature Format @@ -1654,15 +1663,13 @@ class SSH2 // we don't initialize any crypto-objects, yet - we do that, later. for now, we need the lengths to make the // diffie-hellman key exchange as fast as possible $decrypt = self::array_intersect_first($s2c_encryption_algorithms, $this->encryption_algorithms_server_to_client); - $decryptKeyLength = $this->encryption_algorithm_to_key_size($decrypt); - if ($decryptKeyLength === null) { + if (!$decrypt || ($decryptKeyLength = $this->encryption_algorithm_to_key_size($decrypt)) === null) { $this->disconnect_helper(NET_SSH2_DISCONNECT_KEY_EXCHANGE_FAILED); throw new NoSupportedAlgorithmsException('No compatible server to client encryption algorithms found'); } $encrypt = self::array_intersect_first($c2s_encryption_algorithms, $this->encryption_algorithms_client_to_server); - $encryptKeyLength = $this->encryption_algorithm_to_key_size($encrypt); - if ($encryptKeyLength === null) { + if (!$encrypt || ($encryptKeyLength = $this->encryption_algorithm_to_key_size($encrypt)) === null) { $this->disconnect_helper(NET_SSH2_DISCONNECT_KEY_EXCHANGE_FAILED); throw new NoSupportedAlgorithmsException('No compatible client to server encryption algorithms found'); } @@ -2184,7 +2191,7 @@ class SSH2 } } - /* + /** * Tests whether or not proposed algorithm has a potential for issues * * @link https://www.chiark.greenend.org.uk/~sgtatham/putty/wishlist/ssh2-aesctr-openssh.html @@ -2350,17 +2357,18 @@ class SSH2 $this->send_binary_packet($packet); try { - $bad_key_size_fix = $this->bad_key_size_fix; $response = $this->get_binary_packet(); - } catch (\Exception $e) { - // bad_key_size_fix is only ever re-assigned to true - // under certain conditions. when it's newly set we'll - // retry the connection with that new setting but we'll - // only try it once. - if ($bad_key_size_fix != $this->bad_key_size_fix) { - $this->connect(); - return $this->login_helper($username, $password); + } catch (InvalidPacketLengthException $e) { + // the first opportunity to encounter the "bad key size" error + if (!$this->bad_key_size_fix && self::bad_algorithm_candidate($this->decryptName ?? '')) { + // bad_key_size_fix is only ever re-assigned to true here + // retry the connection with that new setting but we'll + // only try it once. + $this->bad_key_size_fix = true; + return $this->reconnect(); } + throw $e; + } catch (\Exception $e) { $this->disconnect_helper(NET_SSH2_DISCONNECT_CONNECTION_LOST); throw $e; } @@ -3457,7 +3465,7 @@ class SSH2 */ private function reconnect() { - $this->reset_connection(NET_SSH2_DISCONNECT_CONNECTION_LOST); + $this->disconnect_helper(NET_SSH2_DISCONNECT_BY_APPLICATION); $this->connect(); foreach ($this->auth as $auth) { $result = $this->login(...$auth); @@ -3467,103 +3475,113 @@ class SSH2 /** * Resets a connection for re-use - * - * @param int $reason */ - protected function reset_connection($reason) + protected function reset_connection() { - $this->disconnect_helper($reason); + if (is_resource($this->fsock) && get_resource_type($this->fsock) === 'stream') { + fclose($this->fsock); + } + $this->fsock = null; + $this->bitmap = 0; + $this->binary_packet_buffer = null; $this->decrypt = $this->encrypt = false; $this->decrypt_block_size = $this->encrypt_block_size = 8; $this->hmac_check = $this->hmac_create = false; $this->hmac_size = false; $this->session_id = false; + $this->keep_alive_sent = null; $this->get_seq_no = $this->send_seq_no = 0; $this->channel_status = []; $this->channel_id_last_interactive = 0; } + /** + * @return int[] second and microsecond stream timeout options based on user-requested timeout and keep-alive, 0 by default + */ + private function get_stream_timeout() + { + $sec = 0; + $usec = 0; + if ($this->curTimeout > 0) { + $sec = (int) floor($this->curTimeout); + $usec = (int) (1000000 * ($this->curTimeout - $sec)); + } + if ($this->keepAlive > 0) { + $elapsed = microtime(true) - $this->keep_alive_sent; + if ($elapsed < $this->curTimeout) { + $sec = (int) floor($elapsed); + $usec = (int) (1000000 * ($elapsed - $sec)); + } + } + return [$sec, $usec]; + } + /** * Gets Binary Packets * * See '6. Binary Packet Protocol' of rfc4253 for more info. * * @see self::_send_binary_packet() - * @param bool $skip_channel_filter * @return bool|string */ - private function get_binary_packet($skip_channel_filter = false) + private function get_binary_packet() { - if ($skip_channel_filter) { - if (!is_resource($this->fsock)) { - throw new \InvalidArgumentException('fsock is not a resource.'); + if (!is_resource($this->fsock)) { + throw new \InvalidArgumentException('fsock is not a resource.'); + } + if ($this->binary_packet_buffer == null) { + // buffer the packet to permit continued reads across timeouts + $this->binary_packet_buffer = (object) [ + 'raw' => '', // the raw payload read from the socket + 'plain' => '', // the packet in plain text, excluding packet_length header + 'packet_length' => null, // the packet_length value pulled from the payload + 'size' => $this->decrypt_block_size, // the total size of this packet to be read from the socket + // initialize to read single block until packet_length is available + ]; + } + $packet = $this->binary_packet_buffer; + while (strlen($packet->raw) < $packet->size) { + if (feof($this->fsock)) { + $this->disconnect_helper(NET_SSH2_DISCONNECT_CONNECTION_LOST); + throw new ConnectionClosedException('Connection closed by server'); } - $read = [$this->fsock]; - $write = $except = null; + if ($this->curTimeout < 0) { + $this->is_timeout = true; + return true; + } + $this->send_keep_alive(); - if (!$this->curTimeout) { - if ($this->keepAlive <= 0) { - static::stream_select($read, $write, $except, null); - } else { - if (!static::stream_select($read, $write, $except, $this->keepAlive)) { - $this->send_binary_packet(pack('CN', NET_SSH2_MSG_IGNORE, 0)); - return $this->get_binary_packet(true); - } - } - } else { - if ($this->curTimeout < 0) { - $this->is_timeout = true; - return true; - } - - $start = microtime(true); - - if ($this->keepAlive > 0 && $this->keepAlive < $this->curTimeout) { - if (!static::stream_select($read, $write, $except, $this->keepAlive)) { - $this->send_binary_packet(pack('CN', NET_SSH2_MSG_IGNORE, 0)); - $elapsed = microtime(true) - $start; - $this->curTimeout -= $elapsed; - return $this->get_binary_packet(true); - } - $elapsed = microtime(true) - $start; - $this->curTimeout -= $elapsed; - } - - $sec = (int) floor($this->curTimeout); - $usec = (int) (1000000 * ($this->curTimeout - $sec)); - - // this can return a "stream_select(): unable to select [4]: Interrupted system call" error - if (!static::stream_select($read, $write, $except, $sec, $usec)) { - $this->is_timeout = true; - return true; - } - $elapsed = microtime(true) - $start; + [$sec, $usec] = $this->get_stream_timeout(); + stream_set_timeout($this->fsock, $sec, $usec); + $start = microtime(true); + $raw = stream_get_contents($this->fsock, $packet->size - strlen($packet->raw)); + $elapsed = microtime(true) - $start; + if ($this->curTimeout > 0) { $this->curTimeout -= $elapsed; } - } - - if (!is_resource($this->fsock) || feof($this->fsock)) { - $this->bitmap = 0; - $str = 'Connection closed (by server) prematurely'; - if (isset($elapsed)) { - $str .= ' ' . $elapsed . 's'; + if ($raw === false) { + $this->disconnect_helper(NET_SSH2_DISCONNECT_CONNECTION_LOST); + throw new ConnectionClosedException('Connection closed by server'); + } elseif (!strlen($raw)) { + continue; } - throw new ConnectionClosedException($str); - } + $packet->raw .= $raw; + if (!$packet->packet_length) { + $this->get_binary_packet_size($packet); + } + }; - $start = microtime(true); - if ($this->curTimeout) { - $sec = (int) floor($this->curTimeout); - $usec = (int) (1000000 * ($this->curTimeout - $sec)); - stream_set_timeout($this->fsock, $sec, $usec); + if (strlen($packet->raw) != $packet->size) { + throw new \RuntimeException('Size of packet was not expected length'); } - $raw = stream_get_contents($this->fsock, $this->decrypt_block_size); - - if (!strlen($raw)) { - $this->bitmap = 0; - throw new ConnectionClosedException('No data received from server'); + // destroy buffer as packet represents the entire payload and should be processed in full + $this->binary_packet_buffer = null; + // copy the raw payload, so as not to destroy original + $raw = $packet->raw; + if ($this->hmac_check instanceof Hash) { + $hmac = Strings::pop($raw, $this->hmac_size); } - + $packet_length_header_size = 4; if ($this->decrypt) { switch ($this->decryptName) { case 'aes128-gcm@openssh.com': @@ -3573,40 +3591,16 @@ class SSH2 $this->decryptInvocationCounter ); Strings::increment_str($this->decryptInvocationCounter); - $this->decrypt->setAAD($temp = Strings::shift($raw, 4)); - extract(unpack('Npacket_length', $temp)); - /** - * @var integer $packet_length - */ - - $raw .= $this->read_remaining_bytes($packet_length - $this->decrypt_block_size + 4); - $stop = microtime(true); - $tag = stream_get_contents($this->fsock, $this->decrypt_block_size); - $this->decrypt->setTag($tag); - $raw = $this->decrypt->decrypt($raw); - $raw = $temp . $raw; - $remaining_length = 0; + $this->decrypt->setAAD(Strings::shift($raw, $packet_length_header_size)); + $this->decrypt->setTag(Strings::pop($raw, $this->decrypt_block_size)); + $packet->plain = $this->decrypt->decrypt($raw); break; case 'chacha20-poly1305@openssh.com': // This should be impossible, but we are checking anyway to narrow the type for Psalm. if (!($this->decrypt instanceof ChaCha20)) { throw new \LogicException('$this->decrypt is not a ' . ChaCha20::class); } - - $nonce = pack('N2', 0, $this->get_seq_no); - - $this->lengthDecrypt->setNonce($nonce); - $temp = $this->lengthDecrypt->decrypt($aad = Strings::shift($raw, 4)); - extract(unpack('Npacket_length', $temp)); - /** - * @var integer $packet_length - */ - - $raw .= $this->read_remaining_bytes($packet_length - $this->decrypt_block_size + 4); - $stop = microtime(true); - $tag = stream_get_contents($this->fsock, 16); - - $this->decrypt->setNonce($nonce); + $this->decrypt->setNonce(pack('N2', 0, $this->get_seq_no)); $this->decrypt->setCounter(0); // this is the same approach that's implemented in Salsa20::createPoly1305Key() // but we don't want to use the same AEAD construction that RFC8439 describes @@ -3614,79 +3608,55 @@ class SSH2 $this->decrypt->setPoly1305Key( $this->decrypt->encrypt(str_repeat("\0", 32)) ); - $this->decrypt->setAAD($aad); + $this->decrypt->setAAD(Strings::shift($raw, $packet_length_header_size)); $this->decrypt->setCounter(1); - $this->decrypt->setTag($tag); - $raw = $this->decrypt->decrypt($raw); - $raw = $temp . $raw; - $remaining_length = 0; + $this->decrypt->setTag(Strings::pop($raw, 16)); + $packet->plain = $this->decrypt->decrypt($raw); break; default: if (!$this->hmac_check instanceof Hash || !$this->hmac_check_etm) { - $raw = $this->decrypt->decrypt($raw); - break; + // first block was already decrypted for contained packet_length header + Strings::shift($raw, $this->decrypt_block_size); + if (strlen($raw) > 0) { + $packet->plain .= $this->decrypt->decrypt($raw); + } + } else { + Strings::shift($raw, $packet_length_header_size); + $packet->plain = $this->decrypt->decrypt($raw); } - extract(unpack('Npacket_length', $temp = Strings::shift($raw, 4))); - /** - * @var integer $packet_length - */ - $raw .= $this->read_remaining_bytes($packet_length - $this->decrypt_block_size + 4); - $stop = microtime(true); - $encrypted = $temp . $raw; - $raw = $temp . $this->decrypt->decrypt($raw); - $remaining_length = 0; + break; } + } else { + Strings::shift($raw, $packet_length_header_size); + $packet->plain = $raw; } - - if (strlen($raw) < 5) { - $this->bitmap = 0; - throw new \RuntimeException('Plaintext is too short'); - } - extract(unpack('Npacket_length/Cpadding_length', Strings::shift($raw, 5))); - /** - * @var integer $packet_length - * @var integer $padding_length - */ - - if (!isset($remaining_length)) { - $remaining_length = $packet_length + 4 - $this->decrypt_block_size; - } - - $buffer = $this->read_remaining_bytes($remaining_length); - - if (!isset($stop)) { - $stop = microtime(true); - } - if (strlen($buffer)) { - $raw .= $this->decrypt ? $this->decrypt->decrypt($buffer) : $buffer; - } - - $payload = Strings::shift($raw, $packet_length - $padding_length - 1); - $padding = Strings::shift($raw, $padding_length); // should leave $raw empty - if ($this->hmac_check instanceof Hash) { - $hmac = stream_get_contents($this->fsock, $this->hmac_size); - if ($hmac === false || strlen($hmac) != $this->hmac_size) { - $this->disconnect_helper(NET_SSH2_DISCONNECT_MAC_ERROR); - throw new \RuntimeException('Error reading socket'); - } - $reconstructed = !$this->hmac_check_etm ? - pack('NCa*', $packet_length, $padding_length, $payload . $padding) : - $encrypted; + pack('Na*', $packet->packet_length, $packet->plain) : + substr($packet->raw, 0, -$this->hmac_size); if (($this->hmac_check->getHash() & "\xFF\xFF\xFF\xFF") == 'umac') { $this->hmac_check->setNonce("\0\0\0\0" . pack('N', $this->get_seq_no)); if ($hmac != $this->hmac_check->hash($reconstructed)) { $this->disconnect_helper(NET_SSH2_DISCONNECT_MAC_ERROR); - throw new \RuntimeException('Invalid UMAC'); + throw new ConnectionClosedException('Invalid UMAC'); } } else { if ($hmac != $this->hmac_check->hash(pack('Na*', $this->get_seq_no, $reconstructed))) { $this->disconnect_helper(NET_SSH2_DISCONNECT_MAC_ERROR); - throw new \RuntimeException('Invalid HMAC'); + throw new ConnectionClosedException('Invalid HMAC'); } } } + $padding_length = 0; + $payload = $packet->plain; + extract(unpack('Cpadding_length', Strings::shift($payload, 1))); + if ($padding_length > 0) { + Strings::pop($payload, $padding_length); + } + if (empty($payload)) { + $this->disconnect_helper(NET_SSH2_DISCONNECT_PROTOCOL_ERROR); + throw new ConnectionClosedException('Plaintext is too short'); + } switch ($this->decompress) { case self::NET_SSH2_COMPRESSION_ZLIB_AT_OPENSSH: @@ -3736,63 +3706,69 @@ class SSH2 $this->last_packet = $current; } - return $this->filter($payload, $skip_channel_filter); + return $this->filter($payload); } /** - * Read Remaining Bytes - * - * @see self::get_binary_packet() - * @param int $remaining_length - * @return string + * @param object $packet The packet object being constructed, passed by reference + * The size, packet_length, and plain properties of this object may be modified in processing + * @throws InvalidPacketLengthException if the packet length header is invalid */ - private function read_remaining_bytes($remaining_length) + private function get_binary_packet_size(&$packet) { - if (!$remaining_length) { - return ''; + $packet_length_header_size = 4; + if (strlen($packet->raw) < $packet_length_header_size) { + return; } - - $adjustLength = false; + $packet_length = 0; + $added_validation_length = 0; // indicates when the packet length header is included when validating packet length against block size if ($this->decrypt) { - switch (true) { - case $this->decryptName == 'aes128-gcm@openssh.com': - case $this->decryptName == 'aes256-gcm@openssh.com': - case $this->decryptName == 'chacha20-poly1305@openssh.com': - case $this->hmac_check instanceof Hash && $this->hmac_check_etm: - $remaining_length += $this->decrypt_block_size - 4; - $adjustLength = true; + switch ($this->decryptName) { + case 'aes128-gcm@openssh.com': + case 'aes256-gcm@openssh.com': + extract(unpack('Npacket_length', substr($packet->raw, 0, $packet_length_header_size))); + $packet->size = $packet_length_header_size + $packet_length + $this->decrypt_block_size; // expect tag + break; + case 'chacha20-poly1305@openssh.com': + $this->lengthDecrypt->setNonce(pack('N2', 0, $this->get_seq_no)); + $packet_length_header = $this->lengthDecrypt->decrypt(substr($packet->raw, 0, $packet_length_header_size)); + extract(unpack('Npacket_length', $packet_length_header)); + $packet->size = $packet_length_header_size + $packet_length + 16; // expect tag + break; + default: + if (!$this->hmac_check instanceof Hash || !$this->hmac_check_etm) { + if (strlen($packet->raw) < $this->decrypt_block_size) { + return; + } + $packet->plain = $this->decrypt->decrypt(substr($packet->raw, 0, $this->decrypt_block_size)); + extract(unpack('Npacket_length', Strings::shift($packet->plain, $packet_length_header_size))); + $packet->size = $packet_length_header_size + $packet_length; + $added_validation_length = $packet_length_header_size; + } else { + extract(unpack('Npacket_length', substr($packet->raw, 0, $packet_length_header_size))); + $packet->size = $packet_length_header_size + $packet_length; + } + break; } + } else { + extract(unpack('Npacket_length', substr($packet->raw, 0, $packet_length_header_size))); + $packet->size = $packet_length_header_size + $packet_length; + $added_validation_length = $packet_length_header_size; } - // quoting , // "implementations SHOULD check that the packet length is reasonable" // PuTTY uses 0x9000 as the actual max packet size and so to shall we - // don't do this when GCM mode is used since GCM mode doesn't encrypt the length - if ($remaining_length < -$this->decrypt_block_size || $remaining_length > 0x9000 || $remaining_length % $this->decrypt_block_size != 0) { - if (!$this->bad_key_size_fix && self::bad_algorithm_candidate($this->decrypt ? $this->decryptName : '') && !($this->bitmap & SSH2::MASK_LOGIN)) { - $this->bad_key_size_fix = true; - $this->reset_connection(NET_SSH2_DISCONNECT_KEY_EXCHANGE_FAILED); - return false; - } - throw new \RuntimeException('Invalid size'); + if ( + $packet_length <= 0 || $packet_length > 0x9000 + || ($packet_length + $added_validation_length) % $this->decrypt_block_size != 0 + ) { + $this->disconnect_helper(NET_SSH2_DISCONNECT_PROTOCOL_ERROR); + throw new InvalidPacketLengthException('Invalid packet length'); } - - if ($adjustLength) { - $remaining_length -= $this->decrypt_block_size - 4; + if ($this->hmac_check instanceof Hash) { + $packet->size += $this->hmac_size; } - - $buffer = ''; - while ($remaining_length > 0) { - $temp = stream_get_contents($this->fsock, $remaining_length); - if ($temp === false || feof($this->fsock)) { - $this->disconnect_helper(NET_SSH2_DISCONNECT_CONNECTION_LOST); - throw new \RuntimeException('Error reading from socket'); - } - $buffer .= $temp; - $remaining_length -= strlen($temp); - } - - return $buffer; + $packet->packet_length = $packet_length; } /** @@ -3802,10 +3778,9 @@ class SSH2 * * @see self::_get_binary_packet() * @param string $payload - * @param bool $skip_channel_filter * @return string|bool */ - private function filter($payload, $skip_channel_filter) + private function filter($payload) { switch (ord($payload[0])) { case NET_SSH2_MSG_DISCONNECT: @@ -3816,14 +3791,14 @@ class SSH2 return false; case NET_SSH2_MSG_IGNORE: $this->extra_packets++; - $payload = $this->get_binary_packet($skip_channel_filter); + $payload = $this->get_binary_packet(); break; case NET_SSH2_MSG_DEBUG: $this->extra_packets++; Strings::shift($payload, 2); // second byte is "always_display" list($message) = Strings::unpackSSH2('s', $payload); $this->errors[] = "SSH_MSG_DEBUG: $message"; - $payload = $this->get_binary_packet($skip_channel_filter); + $payload = $this->get_binary_packet(); break; case NET_SSH2_MSG_UNIMPLEMENTED: return false; @@ -3834,7 +3809,7 @@ class SSH2 $this->bitmap = 0; return false; } - $payload = $this->get_binary_packet($skip_channel_filter); + $payload = $this->get_binary_packet(); } break; case NET_SSH2_MSG_EXT_INFO: @@ -3870,20 +3845,10 @@ class SSH2 if (ord(substr($payload, 9 + $length))) { // want reply $this->send_binary_packet(pack('CN', NET_SSH2_MSG_CHANNEL_SUCCESS, $this->server_channels[$channel])); } - $payload = $this->get_binary_packet($skip_channel_filter); + $payload = $this->get_binary_packet(); } } break; - case NET_SSH2_MSG_CHANNEL_DATA: - case NET_SSH2_MSG_CHANNEL_EXTENDED_DATA: - case NET_SSH2_MSG_CHANNEL_CLOSE: - case NET_SSH2_MSG_CHANNEL_EOF: - if (!$skip_channel_filter && !empty($this->server_channels)) { - $this->binary_packet_buffer = $payload; - $this->get_channel_packet(true); - $payload = $this->get_binary_packet(); - } - break; case NET_SSH2_MSG_GLOBAL_REQUEST: // see http://tools.ietf.org/html/rfc4254#section-4 Strings::shift($payload, 1); list($request_name) = Strings::unpackSSH2('s', $payload); @@ -3895,7 +3860,7 @@ class SSH2 return $this->disconnect_helper(NET_SSH2_DISCONNECT_BY_APPLICATION); } - $payload = $this->get_binary_packet($skip_channel_filter); + $payload = $this->get_binary_packet(); break; case NET_SSH2_MSG_CHANNEL_OPEN: // see http://tools.ietf.org/html/rfc4254#section-5.1 Strings::shift($payload, 1); @@ -3948,7 +3913,7 @@ class SSH2 } } - $payload = $this->get_binary_packet($skip_channel_filter); + $payload = $this->get_binary_packet(); break; case NET_SSH2_MSG_CHANNEL_WINDOW_ADJUST: Strings::shift($payload, 1); @@ -3956,7 +3921,7 @@ class SSH2 $this->window_size_client_to_server[$channel] += $window_size; - $payload = ($this->bitmap & self::MASK_WINDOW_ADJUST) ? true : $this->get_binary_packet($skip_channel_filter); + $payload = ($this->bitmap & self::MASK_WINDOW_ADJUST) ? true : $this->get_binary_packet(); } } @@ -4070,23 +4035,17 @@ class SSH2 } while (true) { - if ($this->binary_packet_buffer !== false) { - $response = $this->binary_packet_buffer; - $this->binary_packet_buffer = false; - } else { - $response = $this->get_binary_packet(true); - if ($response === true && $this->is_timeout) { - 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'); + $response = $this->get_binary_packet(); + if ($response === true && $this->is_timeout) { + 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; } @@ -4416,6 +4375,18 @@ class SSH2 } } + /** + * Sends a keep-alive message, if keep-alive is enabled and interval is met + */ + private function send_keep_alive() + { + $elapsed = microtime(true) - $this->keep_alive_sent; + if ($this->keepAlive > 0 && $elapsed >= $this->keepAlive) { + $this->keep_alive_sent = microtime(true); + $this->send_binary_packet(pack('CN', NET_SSH2_MSG_IGNORE, 0)); + } + } + /** * Logs data packets * @@ -4637,10 +4608,7 @@ class SSH2 } } - $this->bitmap = 0; - if (is_resource($this->fsock) && get_resource_type($this->fsock) === 'stream') { - fclose($this->fsock); - } + $this->reset_connection(); return false; } diff --git a/tests/Functional/Net/SSH2Test.php b/tests/Functional/Net/SSH2Test.php index 8603e13f..d565a1da 100644 --- a/tests/Functional/Net/SSH2Test.php +++ b/tests/Functional/Net/SSH2Test.php @@ -8,6 +8,7 @@ namespace phpseclib3\Tests\Functional\Net; +use phpseclib3\Exception\NoSupportedAlgorithmsException; use phpseclib3\Net\SSH2; use phpseclib3\Tests\PhpseclibFunctionalTestCase; @@ -559,4 +560,96 @@ class SSH2Test extends PhpseclibFunctionalTestCase $ssh->reset(SSH2::CHANNEL_SHELL); $this->assertSame(0, $ssh->getOpenChannelCount()); } + + public function testPing() + { + $ssh = $this->getSSH2(); + // assert on unauthenticated ssh2 + $this->assertNotEmpty($ssh->getServerIdentification()); + $this->assertFalse($ssh->ping()); + $this->assertTrue($ssh->isConnected()); + $this->assertSame(0, $ssh->getOpenChannelCount()); + + $ssh = $this->getSSH2Login(); + $this->assertTrue($ssh->ping()); + $this->assertSame(0, $ssh->getOpenChannelCount()); + } + + /** + * @return array + */ + public function getCryptoAlgorithms() + { + $map = [ + 'kex' => SSH2::getSupportedKEXAlgorithms(), + 'hostkey' => SSH2::getSupportedHostKeyAlgorithms(), + 'comp' => SSH2::getSupportedCompressionAlgorithms(), + 'crypt' => SSH2::getSupportedEncryptionAlgorithms(), + 'mac' => SSH2::getSupportedMACAlgorithms(), + ]; + $tests = []; + foreach ($map as $type => $algorithms) { + foreach ($algorithms as $algorithm) { + $tests[] = [$type, $algorithm]; + } + } + return $tests; + } + + /** + * @dataProvider getCryptoAlgorithms + * @param string $type + * @param string $algorithm + */ + public function testCryptoAlgorithms($type, $algorithm) + { + $ssh = $this->getSSH2(); + try { + switch ($type) { + case 'kex': + case 'hostkey': + $ssh->setPreferredAlgorithms([$type => [$algorithm]]); + $this->assertEquals($algorithm, $ssh->getAlgorithmsNegotiated()[$type]); + break; + case 'comp': + case 'crypt': + $ssh->setPreferredAlgorithms([ + 'client_to_server' => [$type => [$algorithm]], + 'server_to_client' => [$type => [$algorithm]], + ]); + $this->assertEquals($algorithm, $ssh->getAlgorithmsNegotiated()['client_to_server'][$type]); + $this->assertEquals($algorithm, $ssh->getAlgorithmsNegotiated()['server_to_client'][$type]); + break; + case 'mac': + $macCryptAlgorithms = array_filter( + SSH2::getSupportedEncryptionAlgorithms(), + function ($algorithm) use ($ssh) { + return !self::callFunc($ssh, 'encryption_algorithm_to_crypt_instance', [$algorithm]) + ->usesNonce(); + } + ); + $ssh->setPreferredAlgorithms([ + 'client_to_server' => ['crypt' => $macCryptAlgorithms, 'mac' => [$algorithm]], + 'server_to_client' => ['crypt' => $macCryptAlgorithms, 'mac' => [$algorithm]], + ]); + $this->assertEquals($algorithm, $ssh->getAlgorithmsNegotiated()['client_to_server']['mac']); + $this->assertEquals($algorithm, $ssh->getAlgorithmsNegotiated()['server_to_client']['mac']); + break; + } + } catch (NoSupportedAlgorithmsException $e) { + self::markTestSkipped("{$type} algorithm {$algorithm} is not supported by server"); + } + + $username = $this->getEnv('SSH_USERNAME'); + $password = $this->getEnv('SSH_PASSWORD'); + $this->assertTrue( + $ssh->login($username, $password), + "SSH2 login using {$type} {$algorithm} failed." + ); + + $ssh->setTimeout(1); + $ssh->write("pwd\n"); + $this->assertNotEmpty($ssh->read('', SSH2::READ_NEXT)); + $ssh->disconnect(); + } } From 69c70cfc034fd091429e1441fea981a0f98708ed Mon Sep 17 00:00:00 2001 From: Robert Date: Tue, 28 May 2024 11:38:23 -0400 Subject: [PATCH 2/3] Correct to PHP 5.6 syntax. Remove remaining reference to removed skip_channel_filter --- phpseclib/Net/SSH2.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/phpseclib/Net/SSH2.php b/phpseclib/Net/SSH2.php index 1bed4dfd..738b8e9c 100644 --- a/phpseclib/Net/SSH2.php +++ b/phpseclib/Net/SSH2.php @@ -2360,7 +2360,7 @@ class SSH2 $response = $this->get_binary_packet(); } catch (InvalidPacketLengthException $e) { // the first opportunity to encounter the "bad key size" error - if (!$this->bad_key_size_fix && self::bad_algorithm_candidate($this->decryptName ?? '')) { + if (!$this->bad_key_size_fix && $this->decryptName != null && self::bad_algorithm_candidate($this->decryptName)) { // bad_key_size_fix is only ever re-assigned to true here // retry the connection with that new setting but we'll // only try it once. @@ -3551,7 +3551,7 @@ class SSH2 } $this->send_keep_alive(); - [$sec, $usec] = $this->get_stream_timeout(); + list($sec, $usec) = $this->get_stream_timeout(); stream_set_timeout($this->fsock, $sec, $usec); $start = microtime(true); $raw = stream_get_contents($this->fsock, $packet->size - strlen($packet->raw)); @@ -3821,7 +3821,7 @@ class SSH2 $this->supported_private_key_algorithms = explode(',', $extension_value); } } - $payload = $this->get_binary_packet($skip_channel_filter); + $payload = $this->get_binary_packet(); } // 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 From 962cfa607d9014a55fb50a07a6a5f3ccb967dfef Mon Sep 17 00:00:00 2001 From: Robert Date: Tue, 28 May 2024 12:08:32 -0400 Subject: [PATCH 3/3] Make data provider static --- tests/Functional/Net/SSH2Test.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/Functional/Net/SSH2Test.php b/tests/Functional/Net/SSH2Test.php index d565a1da..0f2bd8b5 100644 --- a/tests/Functional/Net/SSH2Test.php +++ b/tests/Functional/Net/SSH2Test.php @@ -578,7 +578,7 @@ class SSH2Test extends PhpseclibFunctionalTestCase /** * @return array */ - public function getCryptoAlgorithms() + public static function getCryptoAlgorithms() { $map = [ 'kex' => SSH2::getSupportedKEXAlgorithms(),