fix(lib): bound untrusted count loops in existing protocol parsers
The same unbounded-count DoS class fixed in the room parsers also exists in three pre-existing parsers that read a length-prefixed vector count straight off the wire: FileSearchResponse (SearchResult), the browse SharedFileListResponse, and ExcludedSearchPhrases. A malicious/MITM server or peer could send a tiny frame with a ~4-billion element count and spin the actor thread into an OOM allocation loop. Bound each loop (and the inner attribute loops, where read_int32 does not advance past the buffer end) by the remaining payload. Regression tests feed a hostile count and assert prompt, empty parses.
This commit is contained in:
@@ -71,16 +71,29 @@ pub fn parse_shared_file_list(message: &mut Message) -> Vec<SharedDirectory> {
|
|||||||
let dir_count = body.read_int32();
|
let dir_count = body.read_int32();
|
||||||
let mut dirs = Vec::new();
|
let mut dirs = Vec::new();
|
||||||
for _ in 0..dir_count {
|
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 name = body.read_string();
|
||||||
let file_count = body.read_int32();
|
let file_count = body.read_int32();
|
||||||
let mut files = Vec::new();
|
let mut files = Vec::new();
|
||||||
for _ in 0..file_count {
|
for _ in 0..file_count {
|
||||||
|
if body.get_pointer() >= body.get_size() {
|
||||||
|
break;
|
||||||
|
}
|
||||||
body.read_int8(); // code
|
body.read_int8(); // code
|
||||||
let filename = body.read_string();
|
let filename = body.read_string();
|
||||||
let file_size = body.read_int64();
|
let file_size = body.read_int64();
|
||||||
body.read_string(); // extension
|
body.read_string(); // extension
|
||||||
let attr_count = body.read_int32();
|
let attr_count = body.read_int32();
|
||||||
for _ in 0..attr_count {
|
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();
|
||||||
body.read_int32();
|
body.read_int32();
|
||||||
}
|
}
|
||||||
@@ -91,6 +104,19 @@ pub fn parse_shared_file_list(message: &mut Message) -> Vec<SharedDirectory> {
|
|||||||
dirs
|
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]
|
#[test]
|
||||||
fn shared_file_list_roundtrips() {
|
fn shared_file_list_roundtrips() {
|
||||||
let dirs = vec![
|
let dirs = vec![
|
||||||
|
|||||||
@@ -17,10 +17,30 @@ impl MessageHandler<ServerMessage> for ExcludedSearchPhrasesHandler {
|
|||||||
|
|
||||||
let mut exluded_phrases: Vec<String> = Vec::new();
|
let mut exluded_phrases: Vec<String> = Vec::new();
|
||||||
for _ in 0..item_count {
|
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();
|
let phrase = message.read_string();
|
||||||
exluded_phrases.push(phrase);
|
exluded_phrases.push(phrase);
|
||||||
}
|
}
|
||||||
debug!("Excluded search phrases: {:?}", exluded_phrases);
|
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);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|||||||
@@ -49,6 +49,11 @@ impl SearchResult {
|
|||||||
let n_files = message.read_int32();
|
let n_files = message.read_int32();
|
||||||
let mut files: Vec<File> = Vec::new();
|
let mut files: Vec<File> = Vec::new();
|
||||||
for _ in 0..n_files {
|
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();
|
message.read_int8();
|
||||||
let name = message.read_string();
|
let name = message.read_string();
|
||||||
let size = message.read_int64();
|
let size = message.read_int64();
|
||||||
@@ -57,6 +62,11 @@ impl SearchResult {
|
|||||||
let mut attribs: HashMap<u32, u32> = HashMap::new();
|
let mut attribs: HashMap<u32, u32> = HashMap::new();
|
||||||
|
|
||||||
for _ in 0..n_attribs {
|
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());
|
attribs.insert(message.read_int32(), message.read_int32());
|
||||||
}
|
}
|
||||||
files.push(File {
|
files.push(File {
|
||||||
@@ -213,6 +223,22 @@ impl Transfer {
|
|||||||
mod tests {
|
mod tests {
|
||||||
use super::*;
|
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
|
// A truncated TransferRequest from an untrusted peer must parse to defaults
|
||||||
// rather than panic (the read_* primitives are bounds-checked).
|
// rather than panic (the read_* primitives are bounds-checked).
|
||||||
#[test]
|
#[test]
|
||||||
|
|||||||
Reference in New Issue
Block a user