parser: reject trailing KEX and channel-open payloads

Author: mjc

russh/src/client/kex.rs

@@ -14,6 +14,7 @@ use crate::kex::dh::groups::DhGroup;
 use crate::kex::{KEXES, KexAlgorithm, KexAlgorithmImplementor, KexCause, KexProgress};
 use crate::keys::key::parse_public_key;
 use crate::negotiation::{Names, Select};
+use crate::parsing::ensure_end;
 use crate::session::Exchange;
 use crate::sshbuffer::PacketWriter;
 use crate::{CryptoVec, Error, SshId, msg, negotiation, strict_kex_violation};
@@ -195,6 +196,7 @@ impl ClientKex {
 
                 let prime = Mpint::decode(&mut r)?;
                 let generator = Mpint::decode(&mut r)?;
+                ensure_end(&r)?;
                 debug!("received gex group: prime={prime}, generator={generator}");
 
                 let group = DhGroup {
@@ -287,7 +289,10 @@ impl ClientKex {
                 })?;
 
                 let signature = Bytes::decode(r)?;
-                let signature = Signature::decode(&mut &signature[..])?;
+                let mut signature_reader = &signature[..];
+                let signature = Signature::decode(&mut signature_reader)?;
+                ensure_end(&signature_reader)?;
+                ensure_end(r)?;
 
                 if let Err(e) =
                     signature::Verifier::verify(&server_host_key, hash.as_ref(), &signature)
@@ -338,6 +343,8 @@ impl ClientKex {
                     );
                     return Err(Error::Kex);
                 }
+                let r = &input.buffer[1..];
+                ensure_end(&r)?;
 
                 Ok(KexProgress::Done {
                     newkeys,

russh/src/negotiation.rs

@@ -26,6 +26,7 @@ use crate::kex::{
     EXTENSION_OPENSSH_STRICT_KEX_AS_CLIENT, EXTENSION_OPENSSH_STRICT_KEX_AS_SERVER, KexCause,
 };
 use crate::keys::key::safe_rng;
+use crate::parsing::ensure_end;
 #[cfg(not(target_arch = "wasm32"))]
 use crate::server::Config;
 use crate::sshbuffer::PacketWriter;
@@ -343,6 +344,8 @@ pub(crate) trait Select {
         String::decode(&mut r)?; // languages server-to-client
 
         let follows = u8::decode(&mut r)? != 0;
+        u32::decode(&mut r)?;
+        ensure_end(&r)?;
         Ok(Names {
             kex: kex_algorithm,
             key: key_algorithm,

russh/src/parsing.rs

@@ -4,6 +4,23 @@ use crate::msg;
 
 use crate::map_err;
 
+/// Require a decoded known-message payload to be fully consumed.
+///
+/// SSH RFCs and implemented OpenSSH extensions define exact field layouts for
+/// known message types. Callers use this after decoding those fields so
+/// malformed packets with trailing payload bytes are rejected instead of being
+/// treated as canonical messages.
+pub(crate) fn ensure_end(reader: &impl Reader) -> Result<(), crate::Error> {
+    if reader.is_finished() {
+        Ok(())
+    } else {
+        Err(ssh_encoding::Error::TrailingData {
+            remaining: reader.remaining_len(),
+        }
+        .into())
+    }
+}
+
 #[derive(Debug)]
 pub struct OpenChannelMessage {
     pub typ: ChannelType,
@@ -13,6 +30,12 @@ pub struct OpenChannelMessage {
 }
 
 impl OpenChannelMessage {
+    /// Parse an SSH `CHANNEL_OPEN` payload.
+    ///
+    /// Known channel types are parsed according to their fixed layouts and must
+    /// not contain trailing bytes. Unknown extension channel types remain
+    /// intentionally opaque so applications can implement extension-specific
+    /// parsing and compatibility behavior.
     pub fn parse<R: Reader>(r: &mut R) -> Result<Self, crate::Error> {
         // https://tools.ietf.org/html/rfc4254#section-5.1
         let typ = map_err!(String::decode(r))?;
@@ -21,24 +44,46 @@ impl OpenChannelMessage {
         let maxpacket = map_err!(u32::decode(r))?;
 
         let typ = match typ.as_str() {
-            "session" => ChannelType::Session,
+            "session" => {
+                ensure_end(r)?;
+                ChannelType::Session
+            }
             "x11" => {
                 let originator_address = map_err!(String::decode(r))?;
                 let originator_port = map_err!(u32::decode(r))?;
+                ensure_end(r)?;
                 ChannelType::X11 {
                     originator_address,
                     originator_port,
                 }
             }
-            "direct-tcpip" => ChannelType::DirectTcpip(TcpChannelInfo::decode(r)?),
+            "direct-tcpip" => {
+                let info = TcpChannelInfo::decode(r)?;
+                ensure_end(r)?;
+                ChannelType::DirectTcpip(info)
+            }
             "direct-streamlocal@openssh.com" => {
-                ChannelType::DirectStreamLocal(StreamLocalChannelInfo::decode(r)?)
+                let info = StreamLocalChannelInfo::decode(r)?;
+                String::decode(r)?; // originator address/reserved
+                u32::decode(r)?; // originator port/reserved
+                ensure_end(r)?;
+                ChannelType::DirectStreamLocal(info)
+            }
+            "forwarded-tcpip" => {
+                let info = TcpChannelInfo::decode(r)?;
+                ensure_end(r)?;
+                ChannelType::ForwardedTcpIp(info)
             }
-            "forwarded-tcpip" => ChannelType::ForwardedTcpIp(TcpChannelInfo::decode(r)?),
             "forwarded-streamlocal@openssh.com" => {
-                ChannelType::ForwardedStreamLocal(StreamLocalChannelInfo::decode(r)?)
+                let info = StreamLocalChannelInfo::decode(r)?;
+                String::decode(r)?; // reserved
+                ensure_end(r)?;
+                ChannelType::ForwardedStreamLocal(info)
+            }
+            "auth-agent@openssh.com" => {
+                ensure_end(r)?;
+                ChannelType::AgentForward
             }
-            "auth-agent@openssh.com" => ChannelType::AgentForward,
             _ => ChannelType::Unknown { typ },
         };
 
@@ -177,3 +222,51 @@ impl Decode for ChannelOpenConfirmation {
         })
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::{ChannelType, OpenChannelMessage};
+    use crate::tests::raw_no_crypto::{channel_open_payload, encode_string, push_u32};
+
+    #[test]
+    fn known_channel_open_with_trailing_bytes_is_rejected() {
+        let mut payload = channel_open_payload(b"session");
+        payload.push(0);
+
+        assert!(
+            OpenChannelMessage::parse(&mut payload.as_slice()).is_err(),
+            "known channel-open type accepted trailing bytes"
+        );
+    }
+
+    #[test]
+    fn unknown_channel_open_with_extra_payload_stays_permissive() {
+        let mut payload = channel_open_payload(b"unknown@example.com");
+        payload.extend_from_slice(b"opaque");
+
+        let parsed = OpenChannelMessage::parse(&mut payload.as_slice())
+            .expect("unknown channel-open payload should remain opaque");
+
+        assert!(matches!(parsed.typ, ChannelType::Unknown { .. }));
+    }
+
+    #[test]
+    fn openssh_streamlocal_channel_open_reserved_fields_are_consumed() {
+        let mut direct = channel_open_payload(b"direct-streamlocal@openssh.com");
+        encode_string(&mut direct, b"/tmp/socket");
+        encode_string(&mut direct, b"");
+        push_u32(&mut direct, 0);
+
+        let parsed = OpenChannelMessage::parse(&mut direct.as_slice())
+            .expect("direct streamlocal reserved fields should be consumed");
+        assert!(matches!(parsed.typ, ChannelType::DirectStreamLocal(_)));
+
+        let mut forwarded = channel_open_payload(b"forwarded-streamlocal@openssh.com");
+        encode_string(&mut forwarded, b"/tmp/socket");
+        encode_string(&mut forwarded, b"");
+
+        let parsed = OpenChannelMessage::parse(&mut forwarded.as_slice())
+            .expect("forwarded streamlocal reserved field should be consumed");
+        assert!(matches!(parsed.typ, ChannelType::ForwardedStreamLocal(_)));
+    }
+}

russh/src/server/kex.rs

@@ -13,6 +13,7 @@ use crate::kex::dh::biguint_to_mpint;
 use crate::kex::{KexAlgorithm, KexAlgorithmImplementor, KexCause, KEXES};
 use crate::keys::key::PrivateKeyWithHashAlg;
 use crate::negotiation::{is_key_compatible_with_algo, Names, Select};
+use crate::parsing::ensure_end;
 use crate::{msg, negotiation};
 
 thread_local! {
@@ -173,7 +174,9 @@ impl ServerKex {
                 }
 
                 #[allow(clippy::indexing_slicing)] // length checked
-                let gex_params = GexParams::decode(&mut &input.buffer[1..])?;
+                let mut r = &input.buffer[1..];
+                let gex_params = GexParams::decode(&mut r)?;
+                ensure_end(&r)?;
                 debug!("client requests a gex group: {gex_params:?}");
 
                 let Some(dh_group) = handler.lookup_dh_gex_group(&gex_params).await? else {
@@ -236,6 +239,7 @@ impl ServerKex {
                 self.exchange
                     .client_ephemeral
                     .extend_from_slice(&Bytes::decode(&mut r).map_err(Into::into)?);
+                ensure_end(&r)?;
 
                 let exchange = &mut self.exchange;
                 kex.server_dh(exchange, &input.buffer)?;
@@ -324,6 +328,8 @@ impl ServerKex {
                     );
                     return Err(Error::Kex.into());
                 }
+                let r = &input.buffer[1..];
+                ensure_end(&r)?;
 
                 debug!("new keys received");
                 Ok(KexProgress::Done {
@@ -372,3 +378,21 @@ fn compute_keys(
         session_id: session_id_cv,
     })
 }
+
+#[cfg(test)]
+mod tests {
+    use crate::tests::raw_no_crypto::{
+        assert_rejected, kexinit_payload, raw_kex_signal, timeout,
+    };
+
+    #[tokio::test]
+    async fn kexinit_with_trailing_bytes_rejected_by_server() {
+        let result = timeout(raw_kex_signal(|payload| {
+            payload.extend_from_slice(&kexinit_payload("none"));
+            payload.push(0);
+        }))
+        .await;
+
+        assert_rejected(result, "server accepted a kexinit with trailing bytes");
+    }
+}