fixes

Cargo.toml

@@ -29,7 +29,7 @@ signature = "2.2"
 ssh-encoding = { version = "0.2", features = [
     "bytes",
 ] }
-ssh-key = { version = "=0.6.8+upstream-0.6.7", features = [
+ssh-key = { version = "=0.6.8", features = [
     "ed25519",
     "rsa",
     "rsa-sha1",

russh/examples/echoserver.rs

@@ -1,11 +1,8 @@
-use std::borrow::Cow;
 use std::collections::HashMap;
 use std::sync::Arc;
 
 use async_trait::async_trait;
-use kex::dh::groups::DhGroup;
 use rand_core::OsRng;
-use russh::client::GexParams;
 use russh::keys::*;
 use russh::server::{Msg, Server as _, Session};
 use russh::*;
@@ -26,7 +23,7 @@ async fn main() {
             russh_keys::PrivateKey::random(&mut OsRng, russh_keys::Algorithm::Ed25519).unwrap(),
         ],
         preferred: Preferred {
-            kex: Cow::Owned(vec![russh::kex::DH_GEX_SHA256]),
+            // kex: std::borrow::Cow::Owned(vec![russh::kex::DH_GEX_SHA256]),
             ..Preferred::default()
         },
         ..Default::default()
@@ -136,13 +133,6 @@ impl server::Handler for Server {
         });
         Ok(true)
     }
-
-    async fn lookup_dh_gex_group(
-        &mut self,
-        gex_params: &GexParams,
-    ) -> Result<Option<DhGroup>, Self::Error> {
-        Ok(Some(russh::kex::dh::groups::DH_GROUP16))
-    }
 }
 
 impl Drop for Server {

russh/src/client/kex.rs

@@ -156,7 +156,6 @@ impl Kex for ClientKex {
                 }
 
                 if kex.is_dh_gex() {
-                    // TODO values
                     output.packet(|w| {
                         kex.client_dh_gex_init(&self.config.gex, w)?;
                         Ok(())
@@ -196,25 +195,22 @@ impl Kex for ClientKex {
                 let prime = Mpint::decode(&mut r)?;
                 let gen = Mpint::decode(&mut r)?;
                 debug!("received gex group: prime={}, gen={}", prime, gen);
-                //todo validate group
 
-                let Some(prime_bit_length) = prime.as_positive_bytes().map(|x| x.len() * 8) else {
-                    warn!("negative DH prime received");
-                    return Err(Error::KexInit);
+                let group = DhGroup {
+                    prime: prime.as_bytes().to_vec().into(),
+                    generator: gen.as_bytes().to_vec().into(),
                 };
 
-                if prime_bit_length < self.config.gex.min_group_size
-                    || prime_bit_length > self.config.gex.max_group_size
+                if group.bit_size() < self.config.gex.min_group_size
+                    || group.bit_size() > self.config.gex.max_group_size
                 {
-                    warn!("DH prime size ({prime_bit_length} bits) not within acceptable range");
+                    warn!(
+                        "DH prime size ({} bits) not within requested range",
+                        group.bit_size()
+                    );
                     return Err(Error::KexInit);
                 }
 
-                let group = DhGroup {
-                    prime: prime.as_bytes().to_vec().into(),
-                    generator: gen.as_bytes().to_vec().into(),
-                };
-
                 let exchange = &mut self.exchange;
                 exchange.gex = Some((self.config.gex.clone(), group.clone()));
                 kex.dh_gex_set_group(group)?;

russh/src/kex/dh/groups.rs

@@ -41,6 +41,15 @@ pub struct DhGroup {
     // pub(crate) exp_size: u64,
 }
 
+impl DhGroup {
+    pub fn bit_size(&self) -> usize {
+        let Some(fsb_idx) = self.prime.deref().iter().position(|&x| x != 0) else {
+            return 0;
+        };
+        (self.prime.deref().len() - fsb_idx) * 8
+    }
+}
+
 impl Debug for DhGroup {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         f.debug_struct("DhGroup")
@@ -184,3 +193,5 @@ impl DH {
         public_key > &one && public_key < &prime_minus_one
     }
 }
+
+pub(crate) const BUILTIN_SAFE_DH_GROUPS: &[&DhGroup] = &[&DH_GROUP14, &DH_GROUP16];

russh/src/kex/mod.rs

@@ -236,9 +236,9 @@ impl TryFrom<&str> for Name {
 pub const CURVE25519: Name = Name("curve25519-sha256");
 /// `curve25519-sha256@libssh.org`
 pub const CURVE25519_PRE_RFC_8731: Name = Name("curve25519-sha256@libssh.org");
-/// `diffie-hellman-group-exchange-sha1`
+/// `diffie-hellman-group-exchange-sha1`.
 pub const DH_GEX_SHA1: Name = Name("diffie-hellman-group-exchange-sha1");
-/// `diffie-hellman-group-exchange-sha256`
+/// `diffie-hellman-group-exchange-sha256`.
 pub const DH_GEX_SHA256: Name = Name("diffie-hellman-group-exchange-sha256");
 /// `diffie-hellman-group1-sha1`
 pub const DH_G1_SHA1: Name = Name("diffie-hellman-group1-sha1");

russh/src/server/kex.rs

@@ -183,9 +183,6 @@ impl ServerKex {
                 let prime = biguint_to_mpint(&BigUint::from_bytes_be(&dh_group.prime));
                 let generator = biguint_to_mpint(&BigUint::from_bytes_be(&dh_group.generator));
 
-                dbg!(&prime);
-                dbg!(&generator);
-
                 self.exchange.gex = Some((gex_params, dh_group.clone()));
                 kex.dh_gex_set_group(dh_group)?;
 

russh/src/server/mod.rs

@@ -50,7 +50,7 @@ use tokio::net::{TcpListener, ToSocketAddrs};
 use tokio::pin;
 
 use crate::cipher::{clear, OpeningKey};
-use crate::kex::dh::groups::DhGroup;
+use crate::kex::dh::groups::{DhGroup, BUILTIN_SAFE_DH_GROUPS, DH_GROUP14};
 use crate::kex::{KexProgress, SessionKexState};
 use crate::session::*;
 use crate::ssh_read::*;
@@ -730,22 +730,44 @@ pub trait Handler: Sized {
         Ok(false)
     }
 
-    /// Implement when enabling the `diffie-hellman-group-exchange-*` key exchange methods.
+    /// Override when enabling the `diffie-hellman-group-exchange-*` key exchange methods.
     /// Should return a Diffie-Hellman group with a safe prime whose length is
     /// between `gex_params.min_group_size` and `gex_params.max_group_size` and
     /// (if possible) over and as close as possible to `gex_params.preferred_group_size`.
     ///
     /// OpenSSH uses a pre-generated database of safe primes stored in `/etc/ssh/moduli`
     ///
-    /// The default implementation returns `None`, aborting the key exchange
+    /// The default implementation picks a group from a very short static list
+    /// of built-in standard groups and is not really taking advantage of the security
+    /// offered by these kex methods.
     ///
     /// See https://datatracker.ietf.org/doc/html/rfc4419#section-3
     #[allow(unused_variables)]
     async fn lookup_dh_gex_group(
         &mut self,
         gex_params: &GexParams,
     ) -> Result<Option<DhGroup>, Self::Error> {
-        Ok(None)
+        let mut best_group = &DH_GROUP14;
+
+        // Find _some_ matching group
+        for group in BUILTIN_SAFE_DH_GROUPS.iter() {
+            if group.bit_size() >= gex_params.min_group_size()
+                && group.bit_size() <= gex_params.max_group_size()
+            {
+                best_group = *group;
+                break;
+            }
+        }
+
+        // Find _closest_ matching group
+        for group in BUILTIN_SAFE_DH_GROUPS.iter() {
+            if group.bit_size() > gex_params.preferred_group_size() {
+                best_group = *group;
+                break;
+            }
+        }
+
+        Ok(Some(best_group.clone()))
     }
 }