diff --git a/soulseek-rs-lib/src/message/peer/shared_file_list.rs b/soulseek-rs-lib/src/message/peer/shared_file_list.rs index 2537563..51c5205 100644 --- a/soulseek-rs-lib/src/message/peer/shared_file_list.rs +++ b/soulseek-rs-lib/src/message/peer/shared_file_list.rs @@ -71,16 +71,29 @@ pub fn parse_shared_file_list(message: &mut Message) -> Vec { let dir_count = body.read_int32(); let mut dirs = Vec::new(); for _ in 0..dir_count { + // Stop if a hostile count outruns the (decompressed) payload, so a + // bogus length can't spin us into a huge allocation loop. + if body.get_pointer() >= body.get_size() { + break; + } let name = body.read_string(); let file_count = body.read_int32(); let mut files = Vec::new(); for _ in 0..file_count { + if body.get_pointer() >= body.get_size() { + break; + } body.read_int8(); // code let filename = body.read_string(); let file_size = body.read_int64(); body.read_string(); // extension let attr_count = body.read_int32(); for _ in 0..attr_count { + // Each attribute is two int32s (8 bytes); read_int32 does not + // advance past the end, so bound the loop explicitly. + if body.get_pointer() + 8 > body.get_size() { + break; + } body.read_int32(); body.read_int32(); } @@ -91,6 +104,19 @@ pub fn parse_shared_file_list(message: &mut Message) -> Vec { dirs } +#[test] +fn hostile_dir_count_does_not_hang() { + // A compressed body claiming ~4 billion directories with no data must + // parse to empty promptly rather than looping into an OOM. + let compressed = + crate::utils::zlib::compress_stored(&u32::MAX.to_le_bytes()); + let mut message = Message::new(); + message.write_raw_bytes(vec![0u8; 8]); + message.write_raw_bytes(compressed); + message.set_pointer(8); + assert!(parse_shared_file_list(&mut message).is_empty()); +} + #[test] fn shared_file_list_roundtrips() { let dirs = vec![ diff --git a/soulseek-rs-lib/src/message/server/excluded_search_phrases.rs b/soulseek-rs-lib/src/message/server/excluded_search_phrases.rs index c2f3328..ef7b72e 100644 --- a/soulseek-rs-lib/src/message/server/excluded_search_phrases.rs +++ b/soulseek-rs-lib/src/message/server/excluded_search_phrases.rs @@ -17,10 +17,30 @@ impl MessageHandler for ExcludedSearchPhrasesHandler { let mut exluded_phrases: Vec = Vec::new(); for _ in 0..item_count { - // Read the file name, size, and path (structure can vary) + // Guard against a hostile item_count outrunning the payload. + if message.get_pointer() + 4 > message.get_size() { + break; + } let phrase = message.read_string(); exluded_phrases.push(phrase); } debug!("Excluded search phrases: {:?}", exluded_phrases); } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn hostile_item_count_does_not_hang() { + // item_count=u32::MAX with no phrases: the guard must make this return + // promptly instead of looping ~4 billion times. + let (tx, _rx) = std::sync::mpsc::channel(); + let mut message = Message::new(); + message.write_raw_bytes(vec![0u8; 8]); + message.write_int32(u32::MAX); + message.set_pointer(8); + ExcludedSearchPhrasesHandler.handle(&mut message, tx); + } +} diff --git a/soulseek-rs-lib/src/types.rs b/soulseek-rs-lib/src/types.rs index 490e08a..a22486f 100644 --- a/soulseek-rs-lib/src/types.rs +++ b/soulseek-rs-lib/src/types.rs @@ -49,6 +49,11 @@ impl SearchResult { let n_files = message.read_int32(); let mut files: Vec = Vec::new(); for _ in 0..n_files { + // Stop if a hostile n_files count outruns the payload, so a bogus + // length can't spin us into a huge allocation loop. + if message.get_pointer() >= message.get_size() { + break; + } message.read_int8(); let name = message.read_string(); let size = message.read_int64(); @@ -57,6 +62,11 @@ impl SearchResult { let mut attribs: HashMap = HashMap::new(); for _ in 0..n_attribs { + // Each attribute is two int32s (8 bytes); guard against a bogus + // count since read_int32 does not advance past the buffer end. + if message.get_pointer() + 8 > message.get_size() { + break; + } attribs.insert(message.read_int32(), message.read_int32()); } files.push(File { @@ -213,6 +223,22 @@ impl Transfer { mod tests { use super::*; + // A FileSearchResponse whose n_files claims ~4 billion entries with no + // file data must parse to an empty result promptly, not loop into an OOM. + #[test] + fn search_result_hostile_file_count_does_not_hang() { + let mut body = Vec::new(); + body.extend_from_slice(&0u32.to_le_bytes()); // username "" (len 0) + body.extend_from_slice(&7u32.to_le_bytes()); // token + body.extend_from_slice(&u32::MAX.to_le_bytes()); // n_files (hostile) + let compressed = crate::utils::zlib::compress_stored(&body); + let mut message = Message::new_with_data(compressed); + let result = SearchResult::new_from_message(&mut message) + .expect("hostile count should parse, not error"); + assert_eq!(result.token, 7); + assert!(result.files.is_empty()); + } + // A truncated TransferRequest from an untrusted peer must parse to defaults // rather than panic (the read_* primitives are bounds-checked). #[test]