diff --git a/lib/cluelessh-format/src/lib.rs b/lib/cluelessh-format/src/lib.rs index 639d89b..70b545d 100644 --- a/lib/cluelessh-format/src/lib.rs +++ b/lib/cluelessh-format/src/lib.rs @@ -194,6 +194,9 @@ impl<'a> NameList<'a> { pub fn none() -> NameList<'static> { NameList("") } + pub fn contains(&self, name: &str) -> bool { + self.iter().any(|n| n == name) + } pub fn iter(&self) -> std::str::Split<'a, char> { self.0.split(',') } diff --git a/lib/cluelessh-format/src/numbers.rs b/lib/cluelessh-format/src/numbers.rs index 7e5dae9..10299b4 100644 --- a/lib/cluelessh-format/src/numbers.rs +++ b/lib/cluelessh-format/src/numbers.rs @@ -35,6 +35,8 @@ consts! { const SSH_MSG_DEBUG = 4; const SSH_MSG_SERVICE_REQUEST = 5; const SSH_MSG_SERVICE_ACCEPT = 6; + const SSH_MSG_EXT_INFO = 7; + const SSH_MSG_NEWCOMPRESS = 8; // 20 to 29 Algorithm negotiation const SSH_MSG_KEXINIT = 20; diff --git a/lib/cluelessh-transport/README.md b/lib/cluelessh-transport/README.md index 7a95226..6e981d9 100644 --- a/lib/cluelessh-transport/README.md +++ b/lib/cluelessh-transport/README.md @@ -11,5 +11,6 @@ Other relevant RFCs: - [RFC 5649 AES Galois Counter Mode for the Secure Shell Transport Layer Protocol](https://datatracker.ietf.org/doc/html/rfc5647) - [RFC 5656 Elliptic Curve Algorithm Integration in the Secure Shell Transport Layer](https://datatracker.ietf.org/doc/html/rfc5656) - [RFC 6668 SHA-2 Data Integrity Verification for the Secure Shell (SSH) Transport Layer Protocol](https://datatracker.ietf.org/doc/html/rfc6668) +- [RFC 8308 Extension Negotiation in the Secure Shell (SSH) Protocol](https://datatracker.ietf.org/doc/html/rfc8308) - [RFC 8709 Ed25519 and Ed448 Public Key Algorithms for the Secure Shell (SSH) Protocol](https://datatracker.ietf.org/doc/html/rfc8709) - [RFC 8731 Secure Shell (SSH) Key Exchange Method Using Curve25519 and Curve448](https://datatracker.ietf.org/doc/html/rfc8731) diff --git a/lib/cluelessh-transport/src/client.rs b/lib/cluelessh-transport/src/client.rs index 7013cc8..cb04d1c 100644 --- a/lib/cluelessh-transport/src/client.rs +++ b/lib/cluelessh-transport/src/client.rs @@ -7,7 +7,7 @@ use crate::{ self, AlgorithmName, EncodedSshSignature, EncryptionAlgorithm, HostKeyVerifyAlgorithm, KeyExchangeSecret, SharedSecret, SupportedAlgorithms, }, - packet::{Packet, PacketTransport, ProtocolIdentParser}, + packet::{Packet, PacketTransport, ProtocolIdentParser, RecvBytesResult}, peer_error, Msg, Result, SessionId, SshRng, SshStatus, }; use cluelessh_format::{numbers, NameList, Reader, Writer}; @@ -78,7 +78,17 @@ impl ClientConnection { } } - pub fn recv_bytes(&mut self, bytes: &[u8]) -> Result<()> { + pub fn recv_bytes(&mut self, mut bytes: &[u8]) -> Result<()> { + while let RecvBytesResult::Partial { consumed } = self.recv_bytes_inner(bytes)? { + bytes = &bytes[consumed..]; + if bytes.is_empty() { + break; + } + } + Ok(()) + } + + fn recv_bytes_inner(&mut self, bytes: &[u8]) -> Result { if let ClientState::ProtoExchange { ident_parser, client_ident, @@ -89,12 +99,12 @@ impl ClientConnection { let client_ident = mem::take(client_ident); // This moves to the next state. self.send_kexinit(client_ident, server_ident); - return Ok(()); + return Ok(RecvBytesResult::Full); } - return Ok(()); + return Ok(RecvBytesResult::Full); } - self.packet_transport.recv_bytes(bytes)?; + let consumed = self.packet_transport.recv_bytes(bytes)?; while let Some(packet) = self.packet_transport.recv_next_packet() { let packet_type = packet.payload.first().unwrap_or(&0xFF); @@ -335,7 +345,7 @@ impl ClientConnection { } } } - Ok(()) + Ok(consumed) } pub fn next_msg_to_send(&mut self) -> Option { diff --git a/lib/cluelessh-transport/src/packet.rs b/lib/cluelessh-transport/src/packet.rs index a22761a..f3a038a 100644 --- a/lib/cluelessh-transport/src/packet.rs +++ b/lib/cluelessh-transport/src/packet.rs @@ -13,6 +13,7 @@ use cluelessh_format::{NameList, Reader, Writer}; /// Frames the byte stream into packets. pub(crate) struct PacketTransport { + // TODO: I think we need independent keys for either direction to handle NEWKEYS nicely. keys: Box, recv_next_packet: PacketParser, @@ -43,6 +44,23 @@ impl Msg { } } +#[must_use] +pub enum RecvBytesResult { + /// Only some of the bytes were consumed. + /// The caller should advanced its state machine first before calling again. + /// This can happen due to SSH_MSG_NEWKEYS. + Partial { consumed: usize }, + /// All bytes were consumed. + /// There may be new updates. + Full, +} + +#[must_use] +enum RecvBytesStepResult { + Pending, + ReadPacket { consumed: usize, is_new_keys: bool }, +} + impl PacketTransport { pub(crate) fn new() -> Self { PacketTransport { @@ -56,17 +74,28 @@ impl PacketTransport { send_next_seq_nr: 0, } } - pub(crate) fn recv_bytes(&mut self, mut bytes: &[u8]) -> Result<()> { - while let Some(consumed) = self.recv_bytes_step(bytes)? { + pub(crate) fn recv_bytes(&mut self, mut bytes: &[u8]) -> Result { + let mut total_consumed = 0; + while let RecvBytesStepResult::ReadPacket { + consumed, + is_new_keys, + } = self.recv_bytes_step(bytes)? + { + total_consumed += consumed; + if is_new_keys { + return Ok(RecvBytesResult::Partial { + consumed: total_consumed, + }); + } bytes = &bytes[consumed..]; if bytes.is_empty() { break; } } - Ok(()) + Ok(RecvBytesResult::Full) } - fn recv_bytes_step(&mut self, bytes: &[u8]) -> Result> { + fn recv_bytes_step(&mut self, bytes: &[u8]) -> Result { // This would not work if we buffer two packets where one changes keys in between, // but SSH_MSG_NEWKEYS messages guarantee that this cannot happen. @@ -74,13 +103,18 @@ impl PacketTransport { self.recv_next_packet .recv_bytes(bytes, &mut *self.keys, self.recv_next_seq_nr)?; if let Some((consumed, result)) = result { + let is_new_keys = result.packet_type() == numbers::SSH_MSG_NEWKEYS; + self.recv_packets.push_back(result); self.recv_next_seq_nr = self.recv_next_seq_nr.wrapping_add(1); self.recv_next_packet = PacketParser::new(); - return Ok(Some(consumed)); + return Ok(RecvBytesStepResult::ReadPacket { + consumed, + is_new_keys, + }); } - Ok(None) + Ok(RecvBytesStepResult::Pending) } pub(crate) fn queue_packet(&mut self, packet: Packet) { @@ -180,6 +214,10 @@ impl Packet { // return Err(peer_error!("full packet length must be multiple of 8: {}", bytes.len())); //} + if payload.len() < 1 { + return Err(peer_error!("empty packet without a type")); + } + Ok(Self { payload: payload.to_vec(), }) diff --git a/lib/cluelessh-transport/src/server.rs b/lib/cluelessh-transport/src/server.rs index d96e05c..d648e20 100644 --- a/lib/cluelessh-transport/src/server.rs +++ b/lib/cluelessh-transport/src/server.rs @@ -6,6 +6,7 @@ use crate::crypto::{ }; use crate::packet::{ KeyExchangeEcDhInitPacket, KeyExchangeInitPacket, Packet, PacketTransport, ProtocolIdentParser, + RecvBytesResult, }; use crate::{peer_error, Msg, SshRng, SshStatus}; use crate::{Result, SessionId}; @@ -67,6 +68,7 @@ enum ServerState { }, ServiceRequest { session_id: SessionId, + may_send_extensions: bool, }, Open { session_id: SessionId, @@ -103,7 +105,17 @@ impl ServerConnection { } } - pub fn recv_bytes(&mut self, bytes: &[u8]) -> Result<()> { + pub fn recv_bytes(&mut self, mut bytes: &[u8]) -> Result<()> { + while let RecvBytesResult::Partial { consumed } = self.recv_bytes_inner(bytes)? { + bytes = &bytes[consumed..]; + if bytes.is_empty() { + break; + } + } + Ok(()) + } + + fn recv_bytes_inner(&mut self, bytes: &[u8]) -> Result { if let ServerState::ProtoExchange { ident_parser } = &mut self.state { ident_parser.recv_bytes(bytes); if let Some(client_identification) = ident_parser.get_peer_ident() { @@ -114,20 +126,20 @@ impl ServerConnection { }; } // This means that we must be called at least twice, which is fine I think. - return Ok(()); + return Ok(RecvBytesResult::Full); } - self.packet_transport.recv_bytes(bytes)?; + let consumed = self.packet_transport.recv_bytes(bytes)?; while let Some(packet) = self.packet_transport.recv_next_packet() { - let packet_type = packet.payload.first().unwrap_or(&0xFF); - let packet_type_string = numbers::packet_type_to_string(*packet_type); + let packet_type = packet.packet_type(); + let packet_type_string = numbers::packet_type_to_string(packet_type); trace!(%packet_type, %packet_type_string, packet_len = %packet.payload.len(), "Received packet"); // Handle some packets ignoring the state. - match packet.payload.first().copied() { - Some(numbers::SSH_MSG_DISCONNECT) => { + match packet_type { + numbers::SSH_MSG_DISCONNECT => { // let mut disconnect = Reader::new(&packet.payload[1..]); let reason = disconnect.u32()?; @@ -140,13 +152,13 @@ impl ServerConnection { return Err(SshStatus::Disconnect); } - Some(numbers::SSH_MSG_IGNORE) => { + numbers::SSH_MSG_IGNORE => { // let mut p = Reader::new(&packet.payload[1..]); let _ = p.string()?; continue; } - Some(numbers::SSH_MSG_DEBUG) => { + numbers::SSH_MSG_DEBUG => { // let mut p = Reader::new(&packet.payload[1..]); let always_display = p.bool()?; @@ -175,6 +187,11 @@ impl ServerConnection { let kex_algorithm = sup_algs.key_exchange.find(false, kex.kex_algorithms.0)?; debug!(name = %kex_algorithm.name(), "Using KEX algorithm"); + // + // TODO: Send some extensions + // TODO: Because of the terrapin attack, we probably want to implement strict kex for that. + let _client_supports_extensions = kex.kex_algorithms.contains("ext-info-c"); + let server_host_key_algorithm = sup_algs .hostkey_sign .find(false, kex.server_host_key_algorithms.0)?; @@ -218,9 +235,12 @@ impl ServerConnection { let mut cookie = [0; 16]; self.rng.fill_bytes(&mut cookie); + // + let kex_algorithms = format!("{},ext-info-s", kex_algorithm.name()); let server_kexinit = KeyExchangeInitPacket { cookie, - kex_algorithms: NameList::one(kex_algorithm.name()), + // TODO: we should send *all* our algorithms here... + kex_algorithms: NameList::multi(&kex_algorithms), server_host_key_algorithms: NameList::one(server_host_key_algorithm.name()), encryption_algorithms_client_to_server: NameList::one( encryption_client_to_server.name(), @@ -310,38 +330,66 @@ impl ServerConnection { ); self.state = ServerState::ServiceRequest { session_id: SessionId(*h), + may_send_extensions: true, // TODO: false if the client didn't advertise them }; } - ServerState::ServiceRequest { session_id } => { - if packet.payload.first() != Some(&numbers::SSH_MSG_SERVICE_REQUEST) { - return Err(peer_error!("did not send SSH_MSG_SERVICE_REQUEST")); - } - let mut p = Reader::new(&packet.payload[1..]); - let service = p.utf8_string()?; - debug!(%service, "Client requesting service"); + ServerState::ServiceRequest { + session_id, + may_send_extensions, + } => match packet_type { + numbers::SSH_MSG_SERVICE_REQUEST => { + let mut p = packet.payload_parser(); + p.u8()?; + let service = p.utf8_string()?; + debug!(%service, "Client requesting service"); - if service != "ssh-userauth" { - return Err(peer_error!("only supports ssh-userauth")); - } + if service != "ssh-userauth" { + return Err(peer_error!("only supports ssh-userauth")); + } - self.packet_transport.queue_packet(Packet { - payload: { - let mut writer = Writer::new(); - writer.u8(numbers::SSH_MSG_SERVICE_ACCEPT); - writer.string(service.as_bytes()); - writer.finish() - }, - }); - self.state = ServerState::Open { - session_id: *session_id, - }; - } + self.packet_transport.queue_packet(Packet { + payload: { + let mut writer = Writer::new(); + writer.u8(numbers::SSH_MSG_SERVICE_ACCEPT); + writer.string(service.as_bytes()); + writer.finish() + }, + }); + self.state = ServerState::Open { + session_id: *session_id, + }; + } + numbers::SSH_MSG_EXT_INFO if *may_send_extensions => { + let mut p = packet.payload_parser(); + p.u8()?; + let count = p.u32()?; + + debug!(%count, "Received extensions"); + + for _ in 0..count { + // while the spec doesn't say it, if you send an extension name that's invalid UTF-8 you deserve the error + let name = p.utf8_string()?; + let _value = p.string()?; + debug!(?name, "Received extension"); + } + + self.state = ServerState::ServiceRequest { + session_id: *session_id, + may_send_extensions: false, + }; + } + _ => { + return Err(peer_error!( + "unexpected packet: {packet_type}, expected SSH_MSG_SERVICE_REQUEST" + )) + } + }, ServerState::Open { .. } => { self.plaintext_packets.push_back(packet); } } } - Ok(()) + Ok(consumed) } pub fn is_open(&self) -> Option {