fix(kex): separate GEX peer request validation from client config (#684)

Author: DDtKey

russh/src/client/mod.rs

@@ -1685,22 +1685,42 @@ impl GexParams {
         preferred_group_size: usize,
         max_group_size: usize,
     ) -> Result<Self, Error> {
-        let this = Self {
+        Self::for_client_config(min_group_size, preferred_group_size, max_group_size)
+    }
+
+    pub fn for_client_config(
+        min_group_size: usize,
+        preferred_group_size: usize,
+        max_group_size: usize,
+    ) -> Result<Self, Error> {
+        Self::build(
             min_group_size,
             preferred_group_size,
             max_group_size,
-        };
-        this.validate()?;
-        Ok(this)
-    }
-
-    pub(crate) fn validate(&self) -> Result<(), Error> {
-        if self.min_group_size < 2048 {
-            return Err(Error::InvalidConfig(format!(
-                "min_group_size must be at least 2048 bits. We got {} bits",
-                self.min_group_size
-            )));
+            ValidationKind::ClientConfig,
+        )
+    }
+
+    fn validate(&self, kind: ValidationKind) -> Result<(), Error> {
+        match kind {
+            ValidationKind::ClientConfig => {
+                if self.min_group_size < 2048 {
+                    return Err(Error::InvalidConfig(format!(
+                        "min_group_size must be at least 2048 bits. We got {} bits",
+                        self.min_group_size
+                    )));
+                }
+            }
+            ValidationKind::PeerRequest => {
+                if self.max_group_size < 2048 {
+                    return Err(Error::InvalidConfig(format!(
+                        "max_group_size must be at least 2048 bits. We got {} bits",
+                        self.max_group_size
+                    )));
+                }
+            }
         }
+
         if self.preferred_group_size < self.min_group_size {
             return Err(Error::InvalidConfig(format!(
                 "preferred_group_size must be at least as large as min_group_size. We have preferred_group_size = {} < min_group_size = {}",
@@ -1713,9 +1733,38 @@ impl GexParams {
                 self.max_group_size, self.preferred_group_size
             )));
         }
+
         Ok(())
     }
 
+    pub(crate) fn from_peer_request(
+        min_group_size: usize,
+        preferred_group_size: usize,
+        max_group_size: usize,
+    ) -> Result<Self, Error> {
+        Self::build(
+            min_group_size,
+            preferred_group_size,
+            max_group_size,
+            ValidationKind::PeerRequest,
+        )
+    }
+
+    fn build(
+        min_group_size: usize,
+        preferred_group_size: usize,
+        max_group_size: usize,
+        kind: ValidationKind,
+    ) -> Result<Self, Error> {
+        let this = Self {
+            min_group_size,
+            preferred_group_size,
+            max_group_size,
+        };
+        this.validate(kind)?;
+        Ok(this)
+    }
+
     pub fn min_group_size(&self) -> usize {
         self.min_group_size
     }
@@ -1729,6 +1778,12 @@ impl GexParams {
     }
 }
 
+#[derive(Clone, Copy, Eq, PartialEq)]
+enum ValidationKind {
+    ClientConfig,
+    PeerRequest,
+}
+
 impl Default for GexParams {
     fn default() -> GexParams {
         GexParams {

russh/src/kex/dh/mod.rs

@@ -347,7 +347,7 @@ impl Decode for GexParams {
         let min_group_size = u32::decode(reader)? as usize;
         let preferred_group_size = u32::decode(reader)? as usize;
         let max_group_size = u32::decode(reader)? as usize;
-        GexParams::new(min_group_size, preferred_group_size, max_group_size)
+        GexParams::from_peer_request(min_group_size, preferred_group_size, max_group_size)
     }
 
     type Error = Error;

russh/src/tests.rs

@@ -643,6 +643,26 @@ mod channels {
     }
 }
 
+mod gex {
+    use super::*;
+
+    #[test]
+    fn peer_request_accepts_rfc4419_minimum_when_server_can_choose_stronger_group() {
+        let params = client::GexParams::from_peer_request(1024, 4097, 8192).unwrap();
+
+        assert_eq!(params.min_group_size(), 1024);
+        assert_eq!(params.preferred_group_size(), 4097);
+        assert_eq!(params.max_group_size(), 8192);
+    }
+
+    #[test]
+    fn local_client_config_still_rejects_minimum_below_2048() {
+        let error = client::GexParams::for_client_config(1024, 4097, 8192).unwrap_err();
+
+        assert!(matches!(error, Error::InvalidConfig(_)));
+    }
+}
+
 mod server_kex_junk {
     use std::sync::Arc;
 

russh/tests/test_kex_shared_secret.rs

@@ -187,6 +187,77 @@ async fn test_kex_done_with_ecdh_nistp256() {
         .unwrap();
 }
 
+#[tokio::test]
+async fn test_kex_done_with_dh_gex_sha256_and_rfc4419_minimum() {
+    let _ = env_logger::try_init();
+
+    let client_key = PrivateKey::random(&mut OsRng, ssh_key::Algorithm::Ed25519).unwrap();
+
+    let mut server_config = server::Config::default();
+    server_config.inactivity_timeout = None;
+    server_config.auth_rejection_time = std::time::Duration::from_secs(3);
+    server_config
+        .keys
+        .push(PrivateKey::random(&mut OsRng, ssh_key::Algorithm::Ed25519).unwrap());
+    server_config.preferred = {
+        let mut p = Preferred::default();
+        p.kex = Cow::Borrowed(&[kex::DH_GEX_SHA256]);
+        p
+    };
+    let server_config = Arc::new(server_config);
+
+    let socket = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap();
+    let addr = socket.local_addr().unwrap();
+
+    tokio::spawn(async move {
+        let (socket, _) = socket.accept().await.unwrap();
+        server::run_stream(server_config, socket, TestServer {})
+            .await
+            .unwrap();
+    });
+
+    let captured_secret: Arc<Mutex<Option<Vec<u8>>>> = Arc::new(Mutex::new(None));
+    let captured_names: Arc<Mutex<Option<Names>>> = Arc::new(Mutex::new(None));
+
+    let mut client_config = client::Config::default();
+    client_config.preferred = {
+        let mut p = Preferred::default();
+        p.kex = Cow::Borrowed(&[kex::DH_GEX_SHA256]);
+        p
+    };
+    client_config.gex = client::GexParams::for_client_config(2048, 4097, 8192).unwrap();
+    let client_config = Arc::new(client_config);
+
+    let client = TestClientWithKexCapture {
+        shared_secret: captured_secret.clone(),
+        negotiated_cipher: captured_names.clone(),
+    };
+
+    let mut session = client::connect(client_config, addr, client).await.unwrap();
+
+    let authenticated = session
+        .authenticate_publickey(
+            std::env::var("USER").unwrap_or("user".to_owned()),
+            PrivateKeyWithHashAlg::new(Arc::new(client_key), None),
+        )
+        .await
+        .unwrap()
+        .success();
+    assert!(authenticated);
+
+    let secret = captured_secret.lock().unwrap();
+    assert!(secret.is_some(), "Shared secret should be captured");
+    assert!(!secret.as_ref().unwrap().is_empty());
+
+    let kex_alg = captured_names.lock().unwrap();
+    assert_eq!(kex_alg.as_ref().unwrap().kex, kex::DH_GEX_SHA256);
+
+    session
+        .disconnect(Disconnect::ByApplication, "", "")
+        .await
+        .unwrap();
+}
+
 /// Test that kex_done is called on rekey
 #[tokio::test]
 async fn test_kex_done_on_rekey() {