transport improvements

lib/cluelessh-format/src/lib.rs

@@ -194,6 +194,9 @@ impl<'a> NameList<'a> {
     pub fn none() -> NameList<'static> {
         NameList("")
     }
+    pub fn contains(&self, name: &str) -> bool {
+        self.iter().any(|n| n == name)
+    }
     pub fn iter(&self) -> std::str::Split<'a, char> {
         self.0.split(',')
     }

lib/cluelessh-format/src/numbers.rs

@@ -35,6 +35,8 @@ consts! {
     const SSH_MSG_DEBUG = 4;
     const SSH_MSG_SERVICE_REQUEST = 5;
     const SSH_MSG_SERVICE_ACCEPT = 6;
+    const SSH_MSG_EXT_INFO = 7;
+    const SSH_MSG_NEWCOMPRESS = 8;
 
     // 20 to 29 Algorithm negotiation
     const SSH_MSG_KEXINIT = 20;

lib/cluelessh-transport/README.md

@@ -11,5 +11,6 @@ Other relevant RFCs:
 - [RFC 5649 AES Galois Counter Mode for the Secure Shell Transport Layer Protocol](https://datatracker.ietf.org/doc/html/rfc5647)
 - [RFC 5656 Elliptic Curve Algorithm Integration in the Secure Shell Transport Layer](https://datatracker.ietf.org/doc/html/rfc5656)
 - [RFC 6668 SHA-2 Data Integrity Verification for the Secure Shell (SSH) Transport Layer Protocol](https://datatracker.ietf.org/doc/html/rfc6668)
+- [RFC 8308 Extension Negotiation in the Secure Shell (SSH) Protocol](https://datatracker.ietf.org/doc/html/rfc8308)
 - [RFC 8709 Ed25519 and Ed448 Public Key Algorithms for the Secure Shell (SSH) Protocol](https://datatracker.ietf.org/doc/html/rfc8709)
 - [RFC 8731 Secure Shell (SSH) Key Exchange Method Using Curve25519 and Curve448](https://datatracker.ietf.org/doc/html/rfc8731)

lib/cluelessh-transport/src/client.rs

@@ -7,7 +7,7 @@ use crate::{
         self, AlgorithmName, EncodedSshSignature, EncryptionAlgorithm, HostKeyVerifyAlgorithm,
         KeyExchangeSecret, SharedSecret, SupportedAlgorithms,
     },
-    packet::{Packet, PacketTransport, ProtocolIdentParser},
+    packet::{Packet, PacketTransport, ProtocolIdentParser, RecvBytesResult},
     peer_error, Msg, Result, SessionId, SshRng, SshStatus,
 };
 use cluelessh_format::{numbers, NameList, Reader, Writer};
@@ -78,7 +78,17 @@ impl ClientConnection {
         }
     }
 
-    pub fn recv_bytes(&mut self, bytes: &[u8]) -> Result<()> {
+    pub fn recv_bytes(&mut self, mut bytes: &[u8]) -> Result<()> {
+        while let RecvBytesResult::Partial { consumed } = self.recv_bytes_inner(bytes)? {
+            bytes = &bytes[consumed..];
+            if bytes.is_empty() {
+                break;
+            }
+        }
+        Ok(())
+    }
+
+    fn recv_bytes_inner(&mut self, bytes: &[u8]) -> Result<RecvBytesResult> {
         if let ClientState::ProtoExchange {
             ident_parser,
             client_ident,
@@ -89,12 +99,12 @@ impl ClientConnection {
                 let client_ident = mem::take(client_ident);
                 // This moves to the next state.
                 self.send_kexinit(client_ident, server_ident);
-                return Ok(());
+                return Ok(RecvBytesResult::Full);
             }
-            return Ok(());
+            return Ok(RecvBytesResult::Full);
         }
 
-        self.packet_transport.recv_bytes(bytes)?;
+        let consumed = self.packet_transport.recv_bytes(bytes)?;
 
         while let Some(packet) = self.packet_transport.recv_next_packet() {
             let packet_type = packet.payload.first().unwrap_or(&0xFF);
@@ -335,7 +345,7 @@ impl ClientConnection {
                 }
             }
         }
-        Ok(())
+        Ok(consumed)
     }
 
     pub fn next_msg_to_send(&mut self) -> Option<Msg> {

lib/cluelessh-transport/src/packet.rs

@@ -13,6 +13,7 @@ use cluelessh_format::{NameList, Reader, Writer};
 
 /// Frames the byte stream into packets.
 pub(crate) struct PacketTransport {
+    // TODO: I think we need independent keys for either direction to handle NEWKEYS nicely.
     keys: Box<dyn Keys>,
     recv_next_packet: PacketParser,
 
@@ -43,6 +44,23 @@ impl Msg {
     }
 }
 
+#[must_use]
+pub enum RecvBytesResult {
+    /// Only some of the bytes were consumed.
+    /// The caller should advanced its state machine first before calling again.
+    /// This can happen due to SSH_MSG_NEWKEYS.
+    Partial { consumed: usize },
+    /// All bytes were consumed.
+    /// There may be new updates.
+    Full,
+}
+
+#[must_use]
+enum RecvBytesStepResult {
+    Pending,
+    ReadPacket { consumed: usize, is_new_keys: bool },
+}
+
 impl PacketTransport {
     pub(crate) fn new() -> Self {
         PacketTransport {
@@ -56,17 +74,28 @@ impl PacketTransport {
             send_next_seq_nr: 0,
         }
     }
-    pub(crate) fn recv_bytes(&mut self, mut bytes: &[u8]) -> Result<()> {
+    pub(crate) fn recv_bytes(&mut self, mut bytes: &[u8]) -> Result<RecvBytesResult> {
-        while let Some(consumed) = self.recv_bytes_step(bytes)? {
+        let mut total_consumed = 0;
+        while let RecvBytesStepResult::ReadPacket {
+            consumed,
+            is_new_keys,
+        } = self.recv_bytes_step(bytes)?
+        {
+            total_consumed += consumed;
+            if is_new_keys {
+                return Ok(RecvBytesResult::Partial {
+                    consumed: total_consumed,
+                });
+            }
             bytes = &bytes[consumed..];
             if bytes.is_empty() {
                 break;
             }
         }
-        Ok(())
+        Ok(RecvBytesResult::Full)
     }
 
-    fn recv_bytes_step(&mut self, bytes: &[u8]) -> Result<Option<usize>> {
+    fn recv_bytes_step(&mut self, bytes: &[u8]) -> Result<RecvBytesStepResult> {
         // This would not work if we buffer two packets where one changes keys in between,
         // but SSH_MSG_NEWKEYS messages guarantee that this cannot happen.
 
@@ -74,13 +103,18 @@ impl PacketTransport {
             self.recv_next_packet
                 .recv_bytes(bytes, &mut *self.keys, self.recv_next_seq_nr)?;
         if let Some((consumed, result)) = result {
+            let is_new_keys = result.packet_type() == numbers::SSH_MSG_NEWKEYS;
+
             self.recv_packets.push_back(result);
             self.recv_next_seq_nr = self.recv_next_seq_nr.wrapping_add(1);
             self.recv_next_packet = PacketParser::new();
-            return Ok(Some(consumed));
+            return Ok(RecvBytesStepResult::ReadPacket {
+                consumed,
+                is_new_keys,
+            });
         }
 
-        Ok(None)
+        Ok(RecvBytesStepResult::Pending)
     }
 
     pub(crate) fn queue_packet(&mut self, packet: Packet) {
@@ -180,6 +214,10 @@ impl Packet {
         //    return Err(peer_error!("full packet length must be multiple of 8: {}", bytes.len()));
         //}
 
+        if payload.len() < 1 {
+            return Err(peer_error!("empty packet without a type"));
+        }
+
         Ok(Self {
             payload: payload.to_vec(),
         })

lib/cluelessh-transport/src/server.rs

@@ -6,6 +6,7 @@ use crate::crypto::{
 };
 use crate::packet::{
     KeyExchangeEcDhInitPacket, KeyExchangeInitPacket, Packet, PacketTransport, ProtocolIdentParser,
+    RecvBytesResult,
 };
 use crate::{peer_error, Msg, SshRng, SshStatus};
 use crate::{Result, SessionId};
@@ -67,6 +68,7 @@ enum ServerState {
     },
     ServiceRequest {
         session_id: SessionId,
+        may_send_extensions: bool,
     },
     Open {
         session_id: SessionId,
@@ -103,7 +105,17 @@ impl ServerConnection {
         }
     }
 
-    pub fn recv_bytes(&mut self, bytes: &[u8]) -> Result<()> {
+    pub fn recv_bytes(&mut self, mut bytes: &[u8]) -> Result<()> {
+        while let RecvBytesResult::Partial { consumed } = self.recv_bytes_inner(bytes)? {
+            bytes = &bytes[consumed..];
+            if bytes.is_empty() {
+                break;
+            }
+        }
+        Ok(())
+    }
+
+    fn recv_bytes_inner(&mut self, bytes: &[u8]) -> Result<RecvBytesResult> {
         if let ServerState::ProtoExchange { ident_parser } = &mut self.state {
             ident_parser.recv_bytes(bytes);
             if let Some(client_identification) = ident_parser.get_peer_ident() {
@@ -114,20 +126,20 @@ impl ServerConnection {
                 };
             }
             // This means that we must be called at least twice, which is fine I think.
-            return Ok(());
+            return Ok(RecvBytesResult::Full);
         }
 
-        self.packet_transport.recv_bytes(bytes)?;
+        let consumed = self.packet_transport.recv_bytes(bytes)?;
 
         while let Some(packet) = self.packet_transport.recv_next_packet() {
-            let packet_type = packet.payload.first().unwrap_or(&0xFF);
+            let packet_type = packet.packet_type();
-            let packet_type_string = numbers::packet_type_to_string(*packet_type);
+            let packet_type_string = numbers::packet_type_to_string(packet_type);
 
             trace!(%packet_type, %packet_type_string, packet_len = %packet.payload.len(), "Received packet");
 
             // Handle some packets ignoring the state.
-            match packet.payload.first().copied() {
+            match packet_type {
-                Some(numbers::SSH_MSG_DISCONNECT) => {
+                numbers::SSH_MSG_DISCONNECT => {
                     // <https://datatracker.ietf.org/doc/html/rfc4253#section-11.1>
                     let mut disconnect = Reader::new(&packet.payload[1..]);
                     let reason = disconnect.u32()?;
@@ -140,13 +152,13 @@ impl ServerConnection {
 
                     return Err(SshStatus::Disconnect);
                 }
-                Some(numbers::SSH_MSG_IGNORE) => {
+                numbers::SSH_MSG_IGNORE => {
                     // <https://datatracker.ietf.org/doc/html/rfc4253#section-11.2>
                     let mut p = Reader::new(&packet.payload[1..]);
                     let _ = p.string()?;
                     continue;
                 }
-                Some(numbers::SSH_MSG_DEBUG) => {
+                numbers::SSH_MSG_DEBUG => {
                     // <https://datatracker.ietf.org/doc/html/rfc4253#section-11.3>
                     let mut p = Reader::new(&packet.payload[1..]);
                     let always_display = p.bool()?;
@@ -175,6 +187,11 @@ impl ServerConnection {
                     let kex_algorithm = sup_algs.key_exchange.find(false, kex.kex_algorithms.0)?;
                     debug!(name = %kex_algorithm.name(), "Using KEX algorithm");
 
+                    // <https://datatracker.ietf.org/doc/html/rfc8308#section-2.1>
+                    // TODO: Send some extensions
+                    // TODO: Because of the terrapin attack, we probably want to implement strict kex for that.
+                    let _client_supports_extensions = kex.kex_algorithms.contains("ext-info-c");
+
                     let server_host_key_algorithm = sup_algs
                         .hostkey_sign
                         .find(false, kex.server_host_key_algorithms.0)?;
@@ -218,9 +235,12 @@ impl ServerConnection {
 
                     let mut cookie = [0; 16];
                     self.rng.fill_bytes(&mut cookie);
+                    // <https://datatracker.ietf.org/doc/html/rfc8308#section-2.1>
+                    let kex_algorithms = format!("{},ext-info-s", kex_algorithm.name());
                     let server_kexinit = KeyExchangeInitPacket {
                         cookie,
-                        kex_algorithms: NameList::one(kex_algorithm.name()),
+                        // TODO: we should send *all* our algorithms here...
+                        kex_algorithms: NameList::multi(&kex_algorithms),
                         server_host_key_algorithms: NameList::one(server_host_key_algorithm.name()),
                         encryption_algorithms_client_to_server: NameList::one(
                             encryption_client_to_server.name(),
@@ -310,13 +330,16 @@ impl ServerConnection {
                     );
                     self.state = ServerState::ServiceRequest {
                         session_id: SessionId(*h),
+                        may_send_extensions: true, // TODO: false if the client didn't advertise them
                     };
                 }
-                ServerState::ServiceRequest { session_id } => {
+                ServerState::ServiceRequest {
-                    if packet.payload.first() != Some(&numbers::SSH_MSG_SERVICE_REQUEST) {
+                    session_id,
-                        return Err(peer_error!("did not send SSH_MSG_SERVICE_REQUEST"));
+                    may_send_extensions,
-                    }
+                } => match packet_type {
-                    let mut p = Reader::new(&packet.payload[1..]);
+                    numbers::SSH_MSG_SERVICE_REQUEST => {
+                        let mut p = packet.payload_parser();
+                        p.u8()?;
                         let service = p.utf8_string()?;
                         debug!(%service, "Client requesting service");
 
@@ -336,12 +359,37 @@ impl ServerConnection {
                             session_id: *session_id,
                         };
                     }
+                    numbers::SSH_MSG_EXT_INFO if *may_send_extensions => {
+                        let mut p = packet.payload_parser();
+                        p.u8()?;
+                        let count = p.u32()?;
+
+                        debug!(%count, "Received extensions");
+
+                        for _ in 0..count {
+                            // while the spec doesn't say it, if you send an extension name that's invalid UTF-8 you deserve the error
+                            let name = p.utf8_string()?;
+                            let _value = p.string()?;
+                            debug!(?name, "Received extension");
+                        }
+
+                        self.state = ServerState::ServiceRequest {
+                            session_id: *session_id,
+                            may_send_extensions: false,
+                        };
+                    }
+                    _ => {
+                        return Err(peer_error!(
+                            "unexpected packet: {packet_type}, expected SSH_MSG_SERVICE_REQUEST"
+                        ))
+                    }
+                },
                 ServerState::Open { .. } => {
                     self.plaintext_packets.push_back(packet);
                 }
             }
         }
-        Ok(())
+        Ok(consumed)
     }
 
     pub fn is_open(&self) -> Option<SessionId> {