GHSA-cqvm-j2r2-hwpg validate DH key range

Author: Eugeny

russh/src/kex/dh/groups.rs

@@ -1,5 +1,3 @@
-use std::ops::Shl;
-
 use hex_literal::hex;
 use num_bigint::{BigUint, RandBigInt};
 use rand;
@@ -69,9 +67,17 @@ impl DH {
         }
     }
 
-    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()
     }
 
@@ -85,7 +91,21 @@ impl DH {
         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
+    }
 }

russh/src/kex/dh/mod.rs

@@ -4,6 +4,7 @@ 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;
@@ -104,16 +105,27 @@ impl<D: Digest> KexAlgorithm for DhGroupKex<D> {
 
         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(())
     }
@@ -124,22 +136,35 @@ impl<D: Digest> KexAlgorithm for DhGroupKex<D> {
         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(())
     }