diff --git a/soulseek-rs-lib/src/message/server/connect_to_peer.rs b/soulseek-rs-lib/src/message/server/connect_to_peer.rs index c7a903a..d0a930d 100644 --- a/soulseek-rs-lib/src/message/server/connect_to_peer.rs +++ b/soulseek-rs-lib/src/message/server/connect_to_peer.rs @@ -9,7 +9,9 @@ impl MessageHandler for ConnectToPeerHandler { 18 } fn handle(&self, message: &mut Message, sender: Sender) { - let peer = Peer::new_from_message(message); - sender.send(ServerMessage::ConnectToPeer(peer)).unwrap(); + let Some(peer) = Peer::new_from_message(message) else { + return; + }; + let _ = sender.send(ServerMessage::ConnectToPeer(peer)); } } diff --git a/soulseek-rs-lib/src/peer/listen.rs b/soulseek-rs-lib/src/peer/listen.rs index 8f660c1..ab26426 100644 --- a/soulseek-rs-lib/src/peer/listen.rs +++ b/soulseek-rs-lib/src/peer/listen.rs @@ -49,9 +49,13 @@ fn parse_peer_init_message(mut message: Message) -> Option { return None; } + let username = message.read_string(); + // An untrusted peer can send any connection-type string; an unknown value + // must be rejected, not panic the listener accept loop. + let connection_type = message.read_string().parse().ok()?; Some(PeerInitData { - username: message.read_string(), - connection_type: message.read_string().parse().unwrap(), + username, + connection_type, token: message.read_int32(), }) } diff --git a/soulseek-rs-lib/src/peer/mod.rs b/soulseek-rs-lib/src/peer/mod.rs index 92e117f..541d0f8 100644 --- a/soulseek-rs-lib/src/peer/mod.rs +++ b/soulseek-rs-lib/src/peer/mod.rs @@ -23,17 +23,17 @@ impl NewPeer { pub fn new_from_message( message: &mut Message, tcp_stream: TcpStream, - ) -> Self { + ) -> Option { let username = message.read_string(); - let connection_type = message.read_string().parse().unwrap(); + let connection_type = message.read_string().parse().ok()?; let token = message.read_int32(); - Self { + Some(Self { username, connection_type, token, tcp_stream, - } + }) } } @@ -115,9 +115,11 @@ impl Peer { } } #[allow(dead_code)] - pub fn new_from_message(message: &mut Message) -> Self { + pub fn new_from_message(message: &mut Message) -> Option { let username = message.read_string(); - let connection_type = message.read_string().parse().unwrap(); + // The connection type is an untrusted string; an unknown value must not + // panic the actor that parses it. + let connection_type = message.read_string().parse().ok()?; let ip = [ message.read_int8(), @@ -135,7 +137,7 @@ impl Peer { message.read_int8(), ); - Self { + Some(Self { username, connection_type, host, @@ -144,9 +146,25 @@ impl Peer { privileged: Some(privileged), unknown: Some(unknown), obfuscated_port: Some(obfuscated_port), - } + }) } } +#[test] +fn new_from_message_returns_none_on_invalid_connection_type() { + // username "ab", connection_type "X" (not P/F/D) from an untrusted server. + let mut data: Vec = vec![0, 0, 0, 0, 0, 0, 0, 0]; + data.extend([2, 0, 0, 0, 97, 98]); // username = "ab" + data.extend([1, 0, 0, 0, 88]); // connection_type = "X" + data.extend([1, 2, 3, 4]); // ip + data.extend([0, 0, 0, 0]); // port + data.extend([0, 0, 0, 0]); // token + data.extend([0, 0, 0]); // privileged, unknown, obfuscated_port + let mut message = Message::new_with_data(data); + message.set_pointer(8); + + assert!(Peer::new_from_message(&mut message).is_none()); +} + #[test] fn test_new_from_message() { let data: Vec = [ @@ -157,7 +175,7 @@ fn test_new_from_message() { let mut message = Message::new_with_data(data); message.set_pointer(8); - let peer = Peer::new_from_message(&mut message); + let peer = Peer::new_from_message(&mut message).unwrap(); assert_eq!(peer.username, "dp"); assert!(matches!(peer.connection_type, ConnectionType::P)); @@ -182,7 +200,7 @@ fn test_new_from_message2() { println!("code: {}", message.get_message_code_u32()); - let peer = Peer::new_from_message(&mut message); + let peer = Peer::new_from_message(&mut message).unwrap(); assert_eq!(peer.username, "grandpag"); assert!(matches!(peer.connection_type, ConnectionType::P)); diff --git a/soulseek-rs-lib/src/types.rs b/soulseek-rs-lib/src/types.rs index e69c9ec..b2e992c 100644 --- a/soulseek-rs-lib/src/types.rs +++ b/soulseek-rs-lib/src/types.rs @@ -174,3 +174,20 @@ impl Transfer { } } } + +#[cfg(test)] +mod tests { + use super::*; + + // A truncated TransferRequest from an untrusted peer must parse to defaults + // rather than panic (the read_* primitives are bounds-checked). + #[test] + fn transfer_new_from_truncated_message_does_not_panic() { + let mut message = Message::new_with_data(vec![1, 0, 0]); + let transfer = Transfer::new_from_message(&mut message); + assert_eq!(transfer.direction, 0); + assert_eq!(transfer.token, 0); + assert_eq!(transfer.filename, ""); + assert_eq!(transfer.size, 0); + } +}