#259, #245, ref #227 - fixed host key algo selection when Preferred::…

…key and the available host keys don't match (#262)
Eugeny · Mar 21, 2024 · 62366e9 · 62366e9
1 parent 0fcb1ec
commit 62366e9
Show file tree

Hide file tree

Showing 8 changed files with 92 additions and 51 deletions.
diff --git a/.all-contributorsrc b/.all-contributorsrc
@@ -247,6 +247,15 @@
       "contributions": [
         "code"
       ]
+    },
+    {
+      "login": "T0b1-iOS",
+      "name": "T0b1-iOS",
+      "avatar_url": "https://avatars.githubusercontent.com/u/15174814?v=4",
+      "profile": "https://github.com/T0b1-iOS",
+      "contributions": [
+        "code"
+      ]
     }
   ],
   "contributorsPerLine": 7,

diff --git a/README.md b/README.md
@@ -1,6 +1,6 @@
 # Russh
 [![Rust](https://github.com/warp-tech/russh/actions/workflows/rust.yml/badge.svg)](https://github.com/warp-tech/russh/actions/workflows/rust.yml)  <!-- ALL-CONTRIBUTORS-BADGE:START - Do not remove or modify this section -->
-[![All Contributors](https://img.shields.io/badge/all_contributors-27-orange.svg?style=flat-square)](#contributors-)
+[![All Contributors](https://img.shields.io/badge/all_contributors-28-orange.svg?style=flat-square)](#contributors-)
 <!-- ALL-CONTRIBUTORS-BADGE:END -->
 
 Low-level Tokio SSH2 client and server implementation.
@@ -109,6 +109,7 @@ Thanks goes to these wonderful people ([emoji key](https://allcontributors.org/d
       <td align="center" valign="top" width="14.28%"><a href="http://samlikes.pizza/"><img src="https://avatars.githubusercontent.com/u/226872?v=4?s=100" width="100px;" alt="Samuel Ainsworth"/><br /><sub><b>Samuel Ainsworth</b></sub></a><br /><a href="https://github.com/warp-tech/russh/commits?author=samuela" title="Code">💻</a></td>
       <td align="center" valign="top" width="14.28%"><a href="https://github.com/Sherlock-Holo"><img src="https://avatars.githubusercontent.com/u/10096425?v=4?s=100" width="100px;" alt="Sherlock Holo"/><br /><sub><b>Sherlock Holo</b></sub></a><br /><a href="https://github.com/warp-tech/russh/commits?author=sherlock-holo" title="Code">💻</a></td>
       <td align="center" valign="top" width="14.28%"><a href="https://github.com/ricott1"><img src="https://avatars.githubusercontent.com/u/16502243?v=4?s=100" width="100px;" alt="Alessandro Ricottone"/><br /><sub><b>Alessandro Ricottone</b></sub></a><br /><a href="https://github.com/warp-tech/russh/commits?author=ricott1" title="Code">💻</a></td>
+      <td align="center" valign="top" width="14.28%"><a href="https://github.com/T0b1-iOS"><img src="https://avatars.githubusercontent.com/u/15174814?v=4?s=100" width="100px;" alt="T0b1-iOS"/><br /><sub><b>T0b1-iOS</b></sub></a><br /><a href="https://github.com/warp-tech/russh/commits?author=T0b1-iOS" title="Code">💻</a></td>
     </tr>
   </tbody>
 </table>

diff --git a/russh/src/client/encrypted.rs b/russh/src/client/encrypted.rs
@@ -60,7 +60,11 @@ impl Session {
                 } else if let Some(exchange) = enc.exchange.take() {
                     Some(KexInit::received_rekey(
                         exchange,
-                        negotiation::Client::read_kex(buf, &self.common.config.as_ref().preferred)?,
+                        negotiation::Client::read_kex(
+                            buf,
+                            &self.common.config.as_ref().preferred,
+                            None,
+                        )?,
                         &enc.session_id,
                     ))
                 } else {

diff --git a/russh/src/client/kex.rs b/russh/src/client/kex.rs
@@ -21,7 +21,7 @@ impl KexInit {
             // read algorithms from packet.
             debug!("extending {:?}", &self.exchange.server_kex_init[..]);
             self.exchange.server_kex_init.extend(buf);
-            negotiation::Client::read_kex(buf, &config.preferred)?
+            negotiation::Client::read_kex(buf, &config.preferred, None)?
         };
         debug!("algo = {:?}", algo);
         debug!("write = {:?}", &write_buffer.buffer[..]);

diff --git a/russh/src/lib.rs b/russh/src/lib.rs
@@ -94,10 +94,8 @@
 //! messages sent through a `server::Handle` are processed when there
 //! is no incoming packet to read.
 
-use std::{
-    convert::TryFrom,
-    fmt::{Debug, Display, Formatter},
-};
+use std::convert::TryFrom;
+use std::fmt::{Debug, Display, Formatter};
 
 use log::debug;
 use parsing::ChannelOpenConfirmation;

diff --git a/russh/src/negotiation.rs b/russh/src/negotiation.rs
@@ -55,6 +55,18 @@ pub struct Preferred {
     pub compression: &'static [&'static str],
 }
 
+impl Preferred {
+    pub(crate) fn possible_host_key_algos_for_keys(
+        &self,
+        available_host_keys: &[KeyPair],
+    ) -> Vec<&'static key::Name> {
+        self.key
+            .iter()
+            .filter(|n| available_host_keys.iter().any(|k| k.name() == n.0))
+            .collect::<Vec<_>>()
+    }
+}
+
 const SAFE_KEX_ORDER: &[kex::Name] = &[
     kex::CURVE25519,
     kex::CURVE25519_PRE_RFC_8731,
@@ -158,19 +170,28 @@ pub(crate) trait Select {
 
     fn select<S: AsRef<str> + Copy>(a: &[S], b: &[u8]) -> Option<(bool, S)>;
 
-    fn read_kex(buffer: &[u8], pref: &Preferred) -> Result<Names, Error> {
+    /// `available_host_keys`, if present, is used to limit the host key algorithms to the ones we have keys for.
+    fn read_kex(
+        buffer: &[u8],
+        pref: &Preferred,
+        available_host_keys: Option<&[KeyPair]>,
+    ) -> Result<Names, Error> {
         let mut r = buffer.reader(17);
+
+        // Key exchange
+
         let kex_string = r.read_string()?;
-        let (kex_both_first, kex_algorithm) = if let Some(x) = Self::select(pref.kex, kex_string) {
-            x
-        } else {
+        let (kex_both_first, kex_algorithm) = Self::select(pref.kex, kex_string).ok_or_else(||
+        {
             debug!(
                 "Could not find common kex algorithm, other side only supports {:?}, we only support {:?}",
                 from_utf8(kex_string),
                 pref.kex
             );
-            return Err(Error::NoCommonKexAlgo);
-        };
+            Error::NoCommonKexAlgo
+        })?;
+
+        // Strict kex detection
 
         let strict_kex_requested = pref.kex.contains(if Self::is_server() {
             &EXTENSION_OPENSSH_STRICT_KEX_AS_SERVER
@@ -190,35 +211,42 @@ pub(crate) trait Select {
             debug!("strict kex enabled")
         }
 
-        let key_string = r.read_string()?;
-        let (key_both_first, key_algorithm) = if let Some(x) = Self::select(pref.key, key_string) {
-            x
-        } else {
-            debug!(
-                "Could not find common key algorithm, other side only supports {:?}, we only support {:?}",
-                from_utf8(key_string),
-                pref.key
-            );
-            return Err(Error::NoCommonKeyAlgo);
+        // Host key
+
+        let key_string: &[u8] = r.read_string()?;
+        let possible_host_key_algos = match available_host_keys {
+            Some(available_host_keys) => pref.possible_host_key_algos_for_keys(available_host_keys),
+            None => pref.key.iter().collect::<Vec<_>>(),
         };
 
+        let (key_both_first, key_algorithm) =
+            Self::select(&possible_host_key_algos[..], key_string).ok_or_else(|| {
+                debug!(
+                    "Could not find common key algorithm, other side only supports {:?}, we only support {:?}",
+                    from_utf8(key_string),
+                    pref.key
+                );
+                Error::NoCommonKeyAlgo
+            })?;
+
+        // Cipher
+
         let cipher_string = r.read_string()?;
-        let cipher = Self::select(pref.cipher, cipher_string);
-        if cipher.is_none() {
-            debug!(
+        let (_cipher_both_first, cipher) =
+            Self::select(pref.cipher, cipher_string).ok_or_else(|| {
+                debug!(
                 "Could not find common cipher, other side only supports {:?}, we only support {:?}",
                 from_utf8(cipher_string),
                 pref.cipher
             );
-            return Err(Error::NoCommonCipher);
-        }
+                Error::NoCommonCipher
+            })?;
         r.read_string()?; // cipher server-to-client.
         debug!("kex {}", line!());
 
-        let need_mac = cipher
-            .and_then(|x| CIPHERS.get(&x.1))
-            .map(|x| x.needs_mac())
-            .unwrap_or(false);
+        // MAC
+
+        let need_mac = CIPHERS.get(&cipher).map(|x| x.needs_mac()).unwrap_or(false);
 
         let client_mac = if let Some((_, m)) = Self::select(pref.mac, r.read_string()?) {
             m
@@ -235,6 +263,8 @@ pub(crate) trait Select {
             mac::NONE
         };
 
+        // Compression
+
         debug!("kex {}", line!());
         // client-to-server compression.
         let client_compression =
@@ -256,23 +286,18 @@ pub(crate) trait Select {
         r.read_string()?; // languages server-to-client
 
         let follows = r.read_byte()? != 0;
-        match (cipher, follows) {
-            (Some((_, cipher)), fol) => {
-                Ok(Names {
-                    kex: kex_algorithm,
-                    key: key_algorithm,
-                    cipher,
-                    client_mac,
-                    server_mac,
-                    client_compression,
-                    server_compression,
-                    // Ignore the next packet if (1) it follows and (2) it's not the correct guess.
-                    ignore_guessed: fol && !(kex_both_first && key_both_first),
-                    strict_kex: strict_kex_requested && strict_kex_provided,
-                })
-            }
-            _ => Err(Error::KexInit),
-        }
+        Ok(Names {
+            kex: kex_algorithm,
+            key: *key_algorithm,
+            cipher,
+            client_mac,
+            server_mac,
+            client_compression,
+            server_compression,
+            // Ignore the next packet if (1) it follows and (2) it's not the correct guess.
+            ignore_guessed: follows && !(kex_both_first && key_both_first),
+            strict_kex: strict_kex_requested && strict_kex_provided,
+        })
     }
 }
 

diff --git a/russh/src/server/encrypted.rs b/russh/src/server/encrypted.rs
@@ -61,7 +61,11 @@ impl Session {
             } else if let Some(exchange) = enc.exchange.take() {
                 let kexinit = KexInit::received_rekey(
                     exchange,
-                    negotiation::Server::read_kex(buf, &self.common.config.as_ref().preferred)?,
+                    negotiation::Server::read_kex(
+                        buf,
+                        &self.common.config.as_ref().preferred,
+                        Some(&self.common.config.as_ref().keys),
+                    )?,
                     &enc.session_id,
                 );
                 enc.rekey = Some(kexinit.server_parse(

diff --git a/russh/src/server/kex.rs b/russh/src/server/kex.rs
@@ -26,7 +26,7 @@ impl KexInit {
             let algo = {
                 // read algorithms from packet.
                 self.exchange.client_kex_init.extend(buf);
-                super::negotiation::Server::read_kex(buf, &config.preferred)?
+                super::negotiation::Server::read_kex(buf, &config.preferred, Some(&config.keys))?
             };
             if !self.sent {
                 self.server_write(config, cipher, write_buffer)?