Several SSH (RFC 4252 / 4253 / 4254) Compliance Issues in russh

[Zhaodl1]

Hi maintainers,
While reviewing russh's SSH implementation against RFC 4252, RFC 4253, and RFC 4254, we noticed several places where the current behavior appears to differ from the specification. Each item below cites the relevant RFC text alongside the corresponding source code for your reference. We hope this is helpful for improving RFC conformance.

---

1. Client Accepts Server-Initiated Forwarding Channels by Default

RFC References: RFC 4254 Section 6.3.2 (X11), Section 7.2 (forwarded-tcpip and direct-tcpip)

"Implementations MUST reject any X11 channel open requests if they have not requested X11 forwarding." (Section 6.3.2)

"Implementations MUST reject these messages unless they have previously requested a remote TCP/IP port forwarding with the given port number." (Section 7.2, forwarded-tcpip)

"Client implementations SHOULD reject direct TCP/IP open requests for security reasons." (Section 7.2, direct-tcpip)

Analysis:
On the client side, the default client::Handler implementations for server-initiated channel
opens call reply.accept().await, so an x11, forwarded-tcpip, or direct-tcpip channel
opened by the server is confirmed by default, without any check that the corresponding
forwarding was previously requested by the client (client/mod.rs:2408-2420, 2259-2273,
2367-2381). The doc comments on these methods state that "Dropping reply automatically
rejects", and the server-side counterparts in server/mod.rs (e.g. channel_open_x11,
channel_open_forwarded_tcpip, channel_open_direct_tcpip at lines 373-424) default to
async { Ok(()) }, which drops the handle and rejects. The client-side defaults instead accept,
which differs from the RFC text quoted above.

Source Code Evidence (russh/src/client/mod.rs):

// client/mod.rs:2408-2420  (X11)
    fn server_channel_open_x11(
        &mut self,
        channel: Channel<Msg>,
        originator_address: &str,
        originator_port: u32,
        reply: ChannelOpenHandle,
        session: &mut Session,
    ) -> impl Future<Output = Result<(), Self::Error>> + Send {
        async move {
            reply.accept().await; // default accepts the channel
            Ok(())
        }
    }

// client/mod.rs:2259-2273  (forwarded-tcpip)
    fn server_channel_open_forwarded_tcpip(
        &mut self,
        channel: Channel<Msg>,
        connected_address: &str,
        connected_port: u32,
        originator_address: &str,
        originator_port: u32,
        reply: ChannelOpenHandle,
        session: &mut Session,
    ) -> impl Future<Output = Result<(), Self::Error>> + Send {
        async move {
            reply.accept().await; // default accepts the channel
            Ok(())
        }
    }

// client/mod.rs:2367-2381  (direct-tcpip)
    fn server_channel_open_direct_tcpip(
        &mut self,
        channel: Channel<Msg>,
        host_to_connect: &str,
        port_to_connect: u32,
        originator_address: &str,
        originator_port: u32,
        reply: ChannelOpenHandle,
        session: &mut Session,
    ) -> impl Future<Output = Result<(), Self::Error>> + Send {
        async move {
            reply.accept().await; // default accepts the channel
            Ok(())
        }
    }

---

2. No Disconnect on Message Number 80 or Higher Before Authentication Completes

RFC Reference: RFC 4252 Section 6

"Message numbers of 80 and higher are reserved for protocols running after this authentication protocol, so receiving one of them before authentication is complete is an error, to which the server MUST respond by disconnecting, preferably with a proper disconnect message sent to ease troubleshooting."

Analysis:
The server's process_packet dispatches on (enc.state, message). In the pre-authentication
states (WaitingAuthServiceRequest, WaitingAuthRequest, InitCompression) only specific
message types are matched — SERVICE_REQUEST (5), USERAUTH_REQUEST (50), and
USERAUTH_INFO_RESPONSE (61). Any other message, including those with message number 80 or
higher (e.g. GLOBAL_REQUEST = 80, CHANNEL_OPEN = 90), falls through to the catch-all
_ => Ok(()) at server/encrypted.rs:141. The message is discarded and processing continues,
rather than the connection being disconnected as described in the RFC text above.

Source Code Evidence (russh/src/server/encrypted.rs):

// server/encrypted.rs:69-142
        match (&mut enc.state, buf.split_first()) {
            (
                EncryptedState::WaitingAuthServiceRequest { accepted, .. },
                Some((&msg::SERVICE_REQUEST, mut r)),
            ) => { /* ... */ }
            (EncryptedState::WaitingAuthRequest(_), Some((&msg::USERAUTH_REQUEST, mut r))) => { /* ... */ }
            (
                EncryptedState::WaitingAuthRequest(auth),
                Some((&msg::USERAUTH_INFO_RESPONSE, mut r)),
            ) => { /* ... */ }
            (EncryptedState::InitCompression, Some((msg, mut r))) => { /* ... */ }
            (EncryptedState::Authenticated, Some((msg, mut r))) => { /* ... */ }
            _ => Ok(()), // messages >= 80 before auth fall here and are discarded
        }

---

3. Receive Path Does Not Validate Minimum Padding Length

RFC Reference: RFC 4253 Section 6

"There MUST be at least four bytes of padding. The padding SHOULD consist of random bytes. The maximum amount of padding is 255 bytes."

Analysis:
On the sending side, the cipher implementations generate at least four bytes of padding. On the
receiving side, cipher::read reads the padding_length byte from the decrypted plaintext at
cipher/mod.rs:334 and uses it only to compute plaintext_end via checked_sub. There is no
check that padding_length >= 4 (the lower bound from the RFC text above). A packet that carries
fewer than four bytes of padding is therefore currently accepted and processed rather than being
treated as invalid. The checked_sub guards against padding_length exceeding the plaintext
length, so the upper bound is implicitly handled.

Source Code Evidence (russh/src/cipher/mod.rs):

// cipher/mod.rs:334-349
    let padding_length = *plaintext.first().to_owned().unwrap_or(&0) as usize;
    trace!("reading, padding_length {padding_length:?}");
    let plaintext_end = plaintext
        .len()
        .checked_sub(padding_length)        // upper bound only
        .ok_or(Error::IndexOutOfBounds)?;   // no `padding_length >= 4` check

    buffer.seqn += Wrapping(1);
    buffer.len = 0;

    buffer.buffer.resize(plaintext_end + 4, 0);

    Ok(plaintext_end + 4)

---

4. Identification String Not Checked for the NUL Character

RFC Reference: RFC 4253 Section 4.2

"The null character MUST NOT be sent. The maximum length of the string is 255 characters, including the Carriage Return and Line Feed."

Analysis:
When reading a peer's identification string, read_ssh_id_inner validates UTF-8 and checks for
the SSH-1.99- or SSH-2.0- prefix and the line length, but does not check for the presence of
a NUL byte (ssh_read.rs:189-197). Because std::str::from_utf8 treats 0x00 as valid UTF-8 and
starts_with does not reject it, an identification string containing an embedded NUL byte is
currently accepted, whereas the RFC text above specifies that the NUL character must not appear
in the identification string.

Source Code Evidence (russh/src/ssh_read.rs):

// ssh_read.rs:189-198
                if i >= 8 {
                    // Check if we have a valid SSH protocol identifier
                    if let Ok(s) = std::str::from_utf8(&ssh_id.buf[..i]) {
                        if s.starts_with("SSH-1.99-") || s.starts_with("SSH-2.0-") {
                            ssh_id.sshid_len = i; // accepted; no NUL-byte check
                            return Ok(ssh_id.id());
                        }
                    }
                }

---

5. Unsupported Service Request Not Rejected with Disconnect

RFC Reference: RFC 4253 Section 10

"If the server rejects the service request, it SHOULD send an appropriate SSH_MSG_DISCONNECT message and MUST disconnect."

Analysis:
In process_packet, the SERVICE_REQUEST handler accepts the service only when the requested
name equals ssh-userauth (server/encrypted.rs:77). When a different service name is
requested, the if request == "ssh-userauth" block is skipped and the handler returns Ok(())
at line 86. No SSH_MSG_DISCONNECT is sent and the connection is not closed, which differs from
the RFC text above describing rejection of an unsupported service request.

Source Code Evidence (russh/src/server/encrypted.rs):

// server/encrypted.rs:70-87
            (
                EncryptedState::WaitingAuthServiceRequest { accepted, .. },
                Some((&msg::SERVICE_REQUEST, mut r)),
            ) => {
                let request = map_err!(String::decode(&mut r))?;
                map_err!(ensure_end(&r))?;
                debug!("request: {request:?}");
                if request == "ssh-userauth" {
                    let auth_request = server_accept_service(/* ... */)?;
                    *accepted = true;
                    enc.state = EncryptedState::WaitingAuthRequest(auth_request);
                }
                Ok(()) // other service names: no DISCONNECT, connection continues
            }

---

6. SSH_MSG_UNIMPLEMENTED Not Sent for Unrecognized Messages

RFC Reference: RFC 4253 Section 11.4

"An implementation MUST respond to all unrecognized messages with an SSH_MSG_UNIMPLEMENTED message in the order in which the messages were received. Such messages MUST be otherwise ignored."

Analysis:
Unrecognized messages received in the authenticated state fall through to the catch-all arm in
server_read_authenticated, which logs a debug line and returns Ok(())
(server/encrypted.rs:1537-1540); the same applies to the _ => Ok(()) arm in process_packet
(line 141). The constant msg::UNIMPLEMENTED (value 3) is defined but is not sent as an outgoing
message in response to unrecognized input. The current behavior ignores such messages without
the SSH_MSG_UNIMPLEMENTED response described in the RFC text above.

Source Code Evidence (russh/src/server/encrypted.rs):

// server/encrypted.rs:1537-1540
            m => {
                debug!("unknown message received: {m:?}");
                Ok(()) // no SSH_MSG_UNIMPLEMENTED is sent
            }

---

7. Password Authentication Not Disabled Under the "none" Cipher

RFC Reference: RFC 4252 Section 8

"Both the server and the client should check whether the underlying transport layer provides confidentiality (i.e., if encryption is being used). If no confidentiality is provided ("none" cipher), password authentication SHOULD be disabled."

Analysis:
The password authentication path in server_read_auth_request (server/encrypted.rs:541-562)
does not consult the negotiated cipher before processing a password authentication request. The
none cipher is mapped to the Clear (no-encryption) implementation in cipher/mod.rs:137, but
the authentication code proceeds identically regardless of which cipher is in effect. As a result,
password authentication is not disabled when the transport provides no confidentiality, which
differs from the RFC text above. (The none cipher is not part of the default preference list, so
this requires explicit configuration.)

Source Code Evidence (russh/src/server/encrypted.rs):

// server/encrypted.rs:541-562
            if method == "password" {
                // ...
                let change = map_err!(u8::decode(r))? != 0;
                let password = map_err!(String::decode(r))?;
                if change {
                    let _new_password = map_err!(String::decode(r))?;
                }
                map_err!(ensure_end(r))?;
                let auth = if change {
                    Auth::Reject { proceed_with_methods: None, partial_success: false }
                } else {
                    handler.auth_password(&user, &password).await? // no confidentiality check
                };