Merge branch 'main' of github.com:warp-tech/russh

Author: Eugeny

.all-contributorsrc

@@ -814,6 +814,15 @@
       "contributions": [
         "code"
       ]
+    },
+    {
+      "login": "tluyben",
+      "name": "tluyben",
+      "avatar_url": "https://avatars.githubusercontent.com/u/623448?v=4",
+      "profile": "https://github.com/tluyben",
+      "contributions": [
+        "code"
+      ]
     }
   ],
   "contributorsPerLine": 7,

README.md

@@ -1,7 +1,7 @@
 # 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-90-orange.svg?style=flat-square)](#contributors-)
+[![All Contributors](https://img.shields.io/badge/all_contributors-91-orange.svg?style=flat-square)](#contributors-)
 <!-- ALL-CONTRIBUTORS-BADGE:END -->
 
 Low-level Tokio SSH2 client and server implementation.
@@ -257,6 +257,7 @@ Thanks goes to these wonderful people ([emoji key](https://allcontributors.org/d
       <td align="center" valign="top" width="14.28%"><a href="http://l9o.dev"><img src="https://avatars.githubusercontent.com/u/112069?v=4?s=100" width="100px;" alt="Luiz Ribeiro"/><br /><sub><b>Luiz Ribeiro</b></sub></a><br /><a href="https://github.com/Eugeny/russh/commits?author=luizribeiro" title="Code">💻</a></td>
       <td align="center" valign="top" width="14.28%"><a href="http://ctx.st"><img src="https://avatars.githubusercontent.com/u/259330610?v=4?s=100" width="100px;" alt="biao29"/><br /><sub><b>biao29</b></sub></a><br /><a href="https://github.com/Eugeny/russh/commits?author=biao29" title="Code">💻</a></td>
       <td align="center" valign="top" width="14.28%"><a href="https://github.com/gvz"><img src="https://avatars.githubusercontent.com/u/3962183?v=4?s=100" width="100px;" alt="Georg von Zengen"/><br /><sub><b>Georg von Zengen</b></sub></a><br /><a href="https://github.com/Eugeny/russh/commits?author=gvz" title="Code">💻</a></td>
+      <td align="center" valign="top" width="14.28%"><a href="https://github.com/tluyben"><img src="https://avatars.githubusercontent.com/u/623448?v=4?s=100" width="100px;" alt="tluyben"/><br /><sub><b>tluyben</b></sub></a><br /><a href="https://github.com/Eugeny/russh/commits?author=tluyben" title="Code">💻</a></td>
     </tr>
   </tbody>
 </table>

russh/src/client/encrypted.rs

@@ -255,6 +255,7 @@ impl Session {
                                             key: key.clone(),
                                             hash_alg,
                                         },
+                                        None,
                                         &mut self.common.buffer,
                                     )?;
                                     let len = self.common.buffer.len();
@@ -284,6 +285,7 @@ impl Session {
                                     let i = enc.client_make_to_sign(
                                         &self.common.auth_user,
                                         &PublicKeyOrCertificate::Certificate(cert.clone()),
+                                        hash_alg,
                                         &mut self.common.buffer,
                                     )?;
                                     let len = self.common.buffer.len();
@@ -1216,6 +1218,100 @@ mod tests {
         assert_eq!(Vec::<u8>::decode(&mut mic).unwrap(), b"mic".to_vec());
         ensure_end(&mic).unwrap();
     }
+
+
+    fn rsa_user_certificate() -> ssh_key::Certificate {
+        let subject = ssh_key::PrivateKey::random(
+            &mut rand::rng(),
+            ssh_key::Algorithm::Rsa { hash: None },
+        )
+        .unwrap();
+        let ca = ssh_key::PrivateKey::random(&mut rand::rng(), ssh_key::Algorithm::Ed25519)
+            .unwrap();
+        let mut builder = ssh_key::certificate::Builder::new_with_random_nonce(
+            &mut rand::rng(),
+            subject.public_key(),
+            0,
+            u64::MAX,
+        )
+        .unwrap();
+        builder.key_id("rsa-user-cert").unwrap();
+        builder
+            .cert_type(ssh_key::certificate::CertType::User)
+            .unwrap();
+        builder.valid_principal("alice").unwrap();
+        builder.sign(&ca).unwrap()
+    }
+
+    fn decode_userauth_certificate_algorithm(
+        mut request: &[u8],
+        expected_has_signature: bool,
+    ) -> String {
+        assert_eq!(u8::decode(&mut request).unwrap(), msg::USERAUTH_REQUEST);
+        assert_eq!(String::decode(&mut request).unwrap(), "alice");
+        assert_eq!(String::decode(&mut request).unwrap(), "ssh-connection");
+        assert_eq!(String::decode(&mut request).unwrap(), "publickey");
+        assert_eq!(
+            u8::decode(&mut request).unwrap(),
+            u8::from(expected_has_signature)
+        );
+        let algorithm = String::decode(&mut request).unwrap();
+        let _certificate = Vec::<u8>::decode(&mut request).unwrap();
+        ensure_end(&request).unwrap();
+        algorithm
+    }
+
+    #[test]
+    fn rsa_future_certificate_hash_is_encoded_in_probe_and_signed_requests() {
+        let certificate = rsa_user_certificate();
+        for (hash_alg, expected) in [
+            (
+                Some(ssh_key::HashAlg::Sha512),
+                "rsa-sha2-512-cert-v01@openssh.com",
+            ),
+            (
+                Some(ssh_key::HashAlg::Sha256),
+                "rsa-sha2-256-cert-v01@openssh.com",
+            ),
+            (None, "ssh-rsa-cert-v01@openssh.com"),
+        ] {
+            let mut encrypted = test_encrypted();
+            encrypted
+                .write_auth_request(
+                    "alice",
+                    &auth::Method::FutureCertificate {
+                        cert: certificate.clone(),
+                        hash_alg,
+                    },
+                )
+                .unwrap();
+            let probe_payloads = payloads(&encrypted.write);
+            assert_eq!(probe_payloads.len(), 1);
+            assert_eq!(
+                decode_userauth_certificate_algorithm(probe_payloads[0], false),
+                expected
+            );
+
+            let mut signed = Vec::new();
+            encrypted
+                .client_make_to_sign(
+                    "alice",
+                    &PublicKeyOrCertificate::Certificate(certificate.clone()),
+                    hash_alg,
+                    &mut signed,
+                )
+                .unwrap();
+            let mut signed_request = signed.as_slice();
+            assert_eq!(
+                Vec::<u8>::decode(&mut signed_request).unwrap(),
+                b"session-id"
+            );
+            assert_eq!(
+                decode_userauth_certificate_algorithm(signed_request, true),
+                expected
+            );
+        }
+    }
 }
 
 impl Encrypted {
@@ -1281,13 +1377,14 @@ impl Encrypted {
                     key.to_bytes()?.as_slice().encode(&mut self.write)?;
                     true
                 }
-                auth::Method::FutureCertificate { ref cert, .. } => {
+                auth::Method::FutureCertificate { ref cert, hash_alg } => {
                     user.as_bytes().encode(&mut self.write)?;
                     "ssh-connection".encode(&mut self.write)?;
                     "publickey".encode(&mut self.write)?;
                     self.write.push(0); // This is a probe
 
                     cert.algorithm()
+                        .with_hash_alg(hash_alg)
                         .to_certificate_type()
                         .encode(&mut self.write)?;
                     cert.to_bytes()?.as_slice().encode(&mut self.write)?;
@@ -1370,6 +1467,7 @@ impl Encrypted {
         &mut self,
         user: &str,
         key: &PublicKeyOrCertificate,
+        certificate_hash_alg: Option<ssh_key::HashAlg>,
         buffer: &mut Vec<u8>,
     ) -> Result<usize, crate::Error> {
         buffer.clear();
@@ -1384,7 +1482,10 @@ impl Encrypted {
 
         match key {
             PublicKeyOrCertificate::Certificate(cert) => {
-                cert.algorithm().to_certificate_type().encode(buffer)?;
+                cert.algorithm()
+                    .with_hash_alg(certificate_hash_alg)
+                    .to_certificate_type()
+                    .encode(buffer)?;
                 cert.to_bytes()?.encode(buffer)?;
             }
             PublicKeyOrCertificate::PublicKey { key, hash_alg } => {
@@ -1404,7 +1505,7 @@ impl Encrypted {
         match method {
             auth::Method::PublicKey { key } => {
                 let i0 =
-                    self.client_make_to_sign(user, &PublicKeyOrCertificate::from(key), buffer)?;
+                    self.client_make_to_sign(user, &PublicKeyOrCertificate::from(key), None, buffer)?;
 
                 // Extend with self-signature.
                 sign_with_hash_alg(key, buffer)?.encode(&mut *buffer)?;
@@ -1418,6 +1519,7 @@ impl Encrypted {
                 let i0 = self.client_make_to_sign(
                     user,
                     &PublicKeyOrCertificate::Certificate(cert.clone()),
+                    None,
                     buffer,
                 )?;
 

russh/src/client/session.rs

@@ -126,7 +126,16 @@ impl Session {
                     pix_width.encode(&mut enc.write)?;
                     pix_height.encode(&mut enc.write)?;
 
-                    ((1 + 5 * terminal_modes.len()) as u32).encode(&mut enc.write)?;
+                    // TTY_OP_END entries are skipped below and the single
+                    // terminator is written afterwards, so the length must
+                    // count only the modes that are actually encoded --
+                    // otherwise the declared string length overruns the
+                    // bytes written and the rest of the packet is garbage.
+                    let encoded_modes = terminal_modes
+                        .iter()
+                        .filter(|&&(code, _)| code != Pty::TTY_OP_END)
+                        .count();
+                    ((1 + 5 * encoded_modes) as u32).encode(&mut enc.write)?;
                     for &(code, value) in terminal_modes {
                         if code == Pty::TTY_OP_END {
                             continue;

russh/src/server/encrypted.rs

@@ -1394,6 +1394,11 @@ impl Session {
                         map_err!(ensure_end(r))?;
 
                         if let Some(chan) = self.channels.get(&channel_num) {
+                            // Only the first `i` entries were decoded from the
+                            // request; the rest of `modes` is padding and must
+                            // not be passed on as if the client had sent it.
+                            #[allow(clippy::indexing_slicing)] // `i <= modes.len()` checked above
+                            let terminal_modes = modes[..i].to_vec();
                             let _ = chan
                                 .send(ChannelMsg::RequestPty {
                                     want_reply: true,
@@ -1402,7 +1407,7 @@ impl Session {
                                     row_height,
                                     pix_width,
                                     pix_height,
-                                    terminal_modes: modes.into(),
+                                    terminal_modes,
                                 })
                                 .await;
                         }

russh/src/tests.rs

@@ -683,6 +683,104 @@ mod channels {
         )
         .await;
     }
+
+    /// What a client sends in a pty-req is what the application on the server
+    /// side must see: exactly those modes, with no padding behind them.
+    #[tokio::test]
+    async fn test_pty_req_modes_are_delivered_unpadded() {
+        let modes = vec![(Pty::VINTR, 3), (Pty::VQUIT, 28)];
+        assert_eq!(send_pty_req(modes.clone()).await, modes);
+    }
+
+    /// A mode list padded with TTY_OP_END entries -- the shape an application
+    /// proxying a pty-req ends up holding -- must still produce a well formed
+    /// request. The encoder skips those entries, so the length it declares for
+    /// the modes string must not count them, or the receiver reads the rest of
+    /// the packet as terminal modes and rejects it.
+    #[tokio::test]
+    async fn test_pty_req_with_padded_modes_is_well_formed() {
+        let mut padded = vec![(Pty::VINTR, 3)];
+        padded.resize(130, (Pty::TTY_OP_END, 0));
+        assert_eq!(send_pty_req(padded).await, vec![(Pty::VINTR, 3)]);
+    }
+
+    /// Send one pty-req and return the modes the server side received.
+    async fn send_pty_req(modes: Vec<(Pty, u32)>) -> Vec<(Pty, u32)> {
+        struct Client {}
+
+        impl client::Handler for Client {
+            type Error = crate::Error;
+
+            async fn check_server_key(
+                &mut self,
+                _server_public_key: &PublicKeyOrCertificate,
+            ) -> Result<bool, Self::Error> {
+                Ok(true)
+            }
+        }
+
+        struct ServerHandle {
+            seen: tokio::sync::mpsc::UnboundedSender<Vec<(Pty, u32)>>,
+        }
+
+        impl server::Handler for ServerHandle {
+            type Error = crate::Error;
+
+            async fn auth_publickey(
+                &mut self,
+                _: &str,
+                _: &crate::keys::ssh_key::PublicKey,
+            ) -> Result<server::Auth, Self::Error> {
+                Ok(server::Auth::Accept)
+            }
+
+            async fn channel_open_session(
+                &mut self,
+                mut channel: Channel<server::Msg>,
+                reply: server::ChannelOpenHandle,
+                session: &mut Session,
+            ) -> Result<(), Self::Error> {
+                reply.accept().await;
+
+                let seen = self.seen.clone();
+                let handle = session.handle();
+                let id = channel.id();
+                tokio::spawn(async move {
+                    while let Some(msg) = channel.wait().await {
+                        if let ChannelMsg::RequestPty { terminal_modes, .. } = msg {
+                            let _ = seen.send(terminal_modes);
+                            // Answer, so the client knows we are done looking.
+                            let _ = handle.channel_success(id).await;
+                            break;
+                        }
+                    }
+                });
+                Ok(())
+            }
+        }
+
+        let (seen, mut received) = tokio::sync::mpsc::unbounded_channel();
+        test_session(
+            Client {},
+            ServerHandle { seen },
+            move |client| async move {
+                let mut channel = client.channel_open_session().await.unwrap();
+                channel
+                    .request_pty(true, "xterm", 80, 24, 0, 0, &modes)
+                    .await
+                    .unwrap();
+                match channel.wait().await {
+                    Some(ChannelMsg::Success) => (),
+                    other => panic!("pty request was not accepted: {other:?}"),
+                }
+                client
+            },
+            |server| async move { server },
+        )
+        .await;
+
+        received.try_recv().expect("server never saw the pty request")
+    }
 }
 
 mod gex {