CVE-2023-28113: Improper Verification of Cryptographic Signature (CWE-347)
value: russh is a Rust SSH client and server library. Starting in version 0.34.0 and prior to versions 0.36.2 and 0.37.1, Diffie-Hellman key validation is insufficient, which can lead to insecure shared secrets and therefore breaks confidentiality. Connections between a russh client and server or those of a russh peer with some other misbehaving peer are most likely to be problematic. These may vulnerable to eavesdropping. Most other implementations reject such keys, so this is mainly an interoperability issue in such a case. This issue is fixed in versions 0.36.2 and 0.37.1
CWE description: The product does not verify, or incorrectly verifies, the cryptographic signature for data.
CVSS v3 base score: 5.9
Published: 2023-03-16T21:15Z
Repository: https://github.com/warp-tech/russh
Language: Rust
Files changed: russh/src/kex/dh/groups.rs, russh/src/kex/dh/mod.rs
Commit date: 2023-03-16 19:23:05 +0100
Commit message: GHSA-cqvm-j2r2-hwpg validate DH key range

Diff: diff --git a/russh/src/kex/dh/groups.rs b/russh/src/kex/dh/groups.rs
index cbe07ce..56248c9 100644
--- a/russh/src/kex/dh/groups.rs
+++ b/russh/src/kex/dh/groups.rs
@@ -1,7 +1,5 @@
-use std::ops::Shl;
-
 use hex_literal::hex;
 use num_bigint::{BigUint, RandBigInt};
 use rand;
 
 pub struct DhGroup {
@@ -67,13 +65,21 @@ impl DH {
             public_key: BigUint::default(),
             shared_secret: BigUint::default(),
         }
     }
 
-    pub fn generate_private_key(&mut self) -> BigUint {
+    pub fn generate_private_key(&mut self, is_server: bool) -> BigUint {
+        let q = (&self.prime_num - &BigUint::from(1u8)) / &BigUint::from(2u8);
         let mut rng = rand::thread_rng();
-        self.private_key = rng.gen_biguint((self.exp_size * 8) - 2u64).shl(1);
+        self.private_key = rng.gen_biguint_range(
+            &if is_server {
+                1u8.into()
+            } else {
+                2u8.into()
+            },
+            &q,
+        );
         self.private_key.clone()
     }
 
     pub fn generate_public_key(&mut self) -> BigUint {
         self.public_key = BigUint::from(self.generator).modpow(&self.private_key, &self.prime_num);
@@ -83,9 +89,23 @@ impl DH {
     pub fn compute_shared_secret(&mut self, other_public_key: BigUint) -> BigUint {
         self.shared_secret = other_public_key.modpow(&self.private_key, &self.prime_num);
         self.shared_secret.clone()
     }
 
+    pub fn validate_shared_secret(&self, shared_secret: &BigUint) -> bool {
+        let one = BigUint::from(1u8);
+        let prime_minus_one = &self.prime_num - &one;
+
+        shared_secret > &one && shared_secret < &prime_minus_one
+    }
+
     pub fn decode_public_key(buffer: &[u8]) -> BigUint {
         BigUint::from_bytes_be(buffer)
     }
+
+    pub fn validate_public_key(&self, public_key: &BigUint) -> bool {
+        let one = BigUint::from(1u8);
+        let prime_minus_one = &self.prime_num - &one;
+
+        public_key > &one && public_key < &prime_minus_one
+    }
 }
diff --git a/russh/src/kex/dh/mod.rs b/russh/src/kex/dh/mod.rs
index 85d833e..0a8b8dd 100644
--- a/russh/src/kex/dh/mod.rs
+++ b/russh/src/kex/dh/mod.rs
@@ -2,10 +2,11 @@ mod groups;
 use std::marker::PhantomData;
 
 use byteorder::{BigEndian, ByteOrder};
 use digest::Digest;
 use groups::DH;
+use log::debug;
 use num_bigint::BigUint;
 use russh_cryptovec::CryptoVec;
 use russh_keys::encoding::Encoding;
 use sha1::Sha1;
 use sha2::Sha256;
@@ -102,46 +103,70 @@ impl<D: Digest> KexAlgorithm for DhGroupKex<D> {
                 .ok_or(crate::Error::Inconsistent)?
         };
 
         debug!("client_pubkey: {:?}", client_pubkey);
 
-        self.dh.generate_private_key();
-        let server_pubkey = biguint_to_mpint(&self.dh.generate_public_key());
+        self.dh.generate_private_key(true);
+        let server_pubkey = &self.dh.generate_public_key();
+        if !self.dh.validate_public_key(server_pubkey) {
+            return Err(crate::Error::Inconsistent);
+        }
+
+        let encoded_server_pubkey = biguint_to_mpint(server_pubkey);
 
         // fill exchange.
         exchange.server_ephemeral.clear();
-        exchange.server_ephemeral.extend(&server_pubkey);
+        exchange.server_ephemeral.extend(&encoded_server_pubkey);
+
+        let decoded_client_pubkey = DH::decode_public_key(client_pubkey);
+        if !self.dh.validate_public_key(&decoded_client_pubkey) {
+            return Err(crate::Error::Inconsistent);
+        }
 
-        let shared = self
-            .dh
-            .compute_shared_secret(DH::decode_public_key(client_pubkey));
+        let shared = self.dh.compute_shared_secret(decoded_client_pubkey);
+        if !self.dh.validate_shared_secret(&shared) {
+            return Err(crate::Error::Inconsistent);
+        }
         self.shared_secret = Some(biguint_to_mpint(&shared));
         Ok(())
     }
 
     #[doc(hidden)]
     fn client_dh(
         &mut self,
         client_ephemeral: &mut CryptoVec,
         buf: &mut CryptoVec,
     ) -> Result<(), crate::Error> {
-        self.dh.generate_private_key();
-        let client_pubkey = biguint_to_mpint(&self.dh.generate_public_key());
+        self.dh.generate_private_key(false);
+        let client_pubkey = &self.dh.generate_public_key();
+
+        if !self.dh.validate_public_key(client_pubkey) {
+            return Err(crate::Error::Inconsistent);
+        }
 
         // fill exchange.
+        let encoded_pubkey = biguint_to_mpint(client_pubkey);
         client_ephemeral.clear();
-        client_ephemeral.extend(&client_pubkey);
+        client_ephemeral.extend(&encoded_pubkey);
 
         buf.push(msg::KEX_ECDH_INIT);
-        buf.extend_ssh_string(&client_pubkey);
+        buf.extend_ssh_string(&encoded_pubkey);
 
         Ok(())
     }
 
     fn compute_shared_secret(&mut self, remote_pubkey_: &[u8]) -> Result<(), crate::Error> {
         let remote_pubkey = DH::decode_public_key(remote_pubkey_);
+
+        if !self.dh.validate_public_key(&remote_pubkey) {
+            return Err(crate::Error::Inconsistent);
+        }
+
         let shared = self.dh.compute_shared_secret(remote_pubkey);
+        if !self.dh.validate_shared_secret(&shared) {
+            return Err(crate::Error::Inconsistent);
+        }
         self.shared_secret = Some(biguint_to_mpint(&shared));
         Ok(())
     }
 
     fn compute_exchange_hash(

Vulnerable code: use std::ops::Shl;

    pub fn generate_private_key(&mut self) -> BigUint {
        self.private_key = rng.gen_biguint((self.exp_size * 8) - 2u64).shl(1);
        self.dh.generate_private_key();
        let server_pubkey = biguint_to_mpint(&self.dh.generate_public_key());
        exchange.server_ephemeral.extend(&server_pubkey);
        let shared = self
            .dh
            .compute_shared_secret(DH::decode_public_key(client_pubkey));
        self.dh.generate_private_key();
        let client_pubkey = biguint_to_mpint(&self.dh.generate_public_key());
        client_ephemeral.extend(&client_pubkey);
        buf.extend_ssh_string(&client_pubkey);
Fixed code:     pub fn generate_private_key(&mut self, is_server: bool) -> BigUint {
        let q = (&self.prime_num - &BigUint::from(1u8)) / &BigUint::from(2u8);
        self.private_key = rng.gen_biguint_range(
            &if is_server {
                1u8.into()
            } else {
                2u8.into()
            },
            &q,
        );
    pub fn validate_shared_secret(&self, shared_secret: &BigUint) -> bool {
        let one = BigUint::from(1u8);
        let prime_minus_one = &self.prime_num - &one;

        shared_secret > &one && shared_secret < &prime_minus_one
    }


    pub fn validate_public_key(&self, public_key: &BigUint) -> bool {
        let one = BigUint::from(1u8);
        let prime_minus_one = &self.prime_num - &one;

        public_key > &one && public_key < &prime_minus_one
    }
use log::debug;
        self.dh.generate_private_key(true);
        let server_pubkey = &self.dh.generate_public_key();
        if !self.dh.validate_public_key(server_pubkey) {
            return Err(crate::Error::Inconsistent);
        }

        let encoded_server_pubkey = biguint_to_mpint(server_pubkey);
        exchange.server_ephemeral.extend(&encoded_server_pubkey);

        let decoded_client_pubkey = DH::decode_public_key(client_pubkey);
        if !self.dh.validate_public_key(&decoded_client_pubkey) {
            return Err(crate::Error::Inconsistent);
        }
        let shared = self.dh.compute_shared_secret(decoded_client_pubkey);
        if !self.dh.validate_shared_secret(&shared) {
            return Err(crate::Error::Inconsistent);
        }
        self.dh.generate_private_key(false);
        let client_pubkey = &self.dh.generate_public_key();

        if !self.dh.validate_public_key(client_pubkey) {
            return Err(crate::Error::Inconsistent);
        }
        let encoded_pubkey = biguint_to_mpint(client_pubkey);
        client_ephemeral.extend(&encoded_pubkey);
        buf.extend_ssh_string(&encoded_pubkey);

        if !self.dh.validate_public_key(&remote_pubkey) {
            return Err(crate::Error::Inconsistent);
        }

        if !self.dh.validate_shared_secret(&shared) {
            return Err(crate::Error::Inconsistent);
        }