From 713c379427c43aa9be9be1dcf7c3ed79a91cab94 Mon Sep 17 00:00:00 2001 From: Sascha Steinbiss Date: Sun, 29 Mar 2020 19:33:29 +0200 Subject: [PATCH] rfb: make sure size calculations do not overflow Addresses #3570 by extra checking of calculated size requests. With the given input, the parser eventually arrived at parser::parse_failure_reason() which parsed from the remaining four bytes (describing the string length) that the failure string to follow would be 4294967295 bytes long. While calculating the total size of the data to request via AppLayerResult::incomplete(), adding the four bytes for the parsed but not consumed string length caused the u32 length to overflow, resulting in a much smaller value triggering the bug condition. This problem was addressed by more careful checking of values in each step that could overflow: one subtraction, one addition (which could overflow the usize length values), and a final check to determine whether the result still fit into the u32 values required by AppLayerResult::incomplete(). If so, we would safely convert the values and pass them to the result type. If not, we simply return AppLayerResult::err() but do not erroneously and silently request the wrong amount. --- rust/src/rfb/rfb.rs | 80 ++++++++++++++------------------------------- 1 file changed, 25 insertions(+), 55 deletions(-) diff --git a/rust/src/rfb/rfb.rs b/rust/src/rfb/rfb.rs index c581a0f2d4..5e22d1880d 100644 --- a/rust/src/rfb/rfb.rs +++ b/rust/src/rfb/rfb.rs @@ -100,6 +100,20 @@ pub struct RFBState { state: parser::RFBGlobalState } +#[inline] +fn handle_incomplete(input: &[u8], current: &[u8], nom_needed: nom::Needed) -> AppLayerResult { + if let nom::Needed::Size(needed_size) = nom_needed { + if let Some(consumed) = input.len().checked_sub(current.len()) { + if let Some(needed) = current.len().checked_add(needed_size) { + if consumed <= (std::u32::MAX as usize) && needed <= (std::u32::MAX as usize) { + return AppLayerResult::incomplete(consumed as u32, needed as u32); + } + } + } + } + return AppLayerResult::err(); +} + impl RFBState { pub fn new() -> Self { Self { @@ -183,11 +197,7 @@ impl RFBState { } } Err(nom::Err::Incomplete(v)) => { - if let nom::Needed::Size(n) = v { - return AppLayerResult::incomplete((input.len() - current.len()) as u32, - (current.len() + n) as u32); - } - return AppLayerResult::err(); + return handle_incomplete(input, current, v); } Err(_) => { return AppLayerResult::err(); @@ -214,11 +224,7 @@ impl RFBState { } } Err(nom::Err::Incomplete(v)) => { - if let nom::Needed::Size(n) = v { - return AppLayerResult::incomplete((input.len() - current.len()) as u32, - (current.len() + n) as u32); - } - return AppLayerResult::err(); + return handle_incomplete(input, current, v); } Err(_) => { return AppLayerResult::err(); @@ -239,11 +245,7 @@ impl RFBState { } } Err(nom::Err::Incomplete(v)) => { - if let nom::Needed::Size(n) = v { - return AppLayerResult::incomplete((input.len() - current.len()) as u32, - (current.len() + n) as u32); - } - return AppLayerResult::err(); + return handle_incomplete(input, current, v); } Err(_) => { return AppLayerResult::err(); @@ -263,11 +265,7 @@ impl RFBState { } } Err(nom::Err::Incomplete(v)) => { - if let nom::Needed::Size(n) = v { - return AppLayerResult::incomplete((input.len() - current.len()) as u32, - (current.len() + n) as u32); - } - return AppLayerResult::err(); + return handle_incomplete(input, current, v); } Err(_) => { return AppLayerResult::err(); @@ -318,11 +316,7 @@ impl RFBState { } } Err(nom::Err::Incomplete(v)) => { - if let nom::Needed::Size(n) = v { - return AppLayerResult::incomplete((input.len() - current.len()) as u32, - (current.len() + n) as u32); - } - return AppLayerResult::err(); + return handle_incomplete(input, current, v); } Err(_) => { return AppLayerResult::err(); @@ -350,11 +344,7 @@ impl RFBState { } } Err(nom::Err::Incomplete(v)) => { - if let nom::Needed::Size(n) = v { - return AppLayerResult::incomplete((input.len() - current.len()) as u32, - (current.len() + n) as u32); - } - return AppLayerResult::err(); + return handle_incomplete(input, current, v); } Err(_) => { return AppLayerResult::err(); @@ -386,11 +376,7 @@ impl RFBState { } } Err(nom::Err::Incomplete(v)) => { - if let nom::Needed::Size(n) = v { - return AppLayerResult::incomplete((input.len() - current.len()) as u32, - (current.len() + n) as u32); - } - return AppLayerResult::err(); + return handle_incomplete(input, current, v); } Err(_) => { return AppLayerResult::err(); @@ -411,11 +397,7 @@ impl RFBState { } } Err(nom::Err::Incomplete(v)) => { - if let nom::Needed::Size(n) = v { - return AppLayerResult::incomplete((input.len() - current.len()) as u32, - (current.len() + n) as u32); - } - return AppLayerResult::err(); + return handle_incomplete(input, current, v); } Err(_) => { return AppLayerResult::err(); @@ -442,11 +424,7 @@ impl RFBState { } } Err(nom::Err::Incomplete(v)) => { - if let nom::Needed::Size(n) = v { - return AppLayerResult::incomplete((input.len() - current.len()) as u32, - (current.len() + n) as u32); - } - return AppLayerResult::err(); + return handle_incomplete(input, current, v); } Err(_) => { return AppLayerResult::err(); @@ -464,11 +442,7 @@ impl RFBState { return AppLayerResult::err(); } Err(nom::Err::Incomplete(v)) => { - if let nom::Needed::Size(n) = v { - return AppLayerResult::incomplete((input.len() - current.len()) as u32, - (current.len() + n) as u32); - } - return AppLayerResult::err(); + return handle_incomplete(input, current, v); } Err(_) => { return AppLayerResult::err(); @@ -490,11 +464,7 @@ impl RFBState { } } Err(nom::Err::Incomplete(v)) => { - if let nom::Needed::Size(n) = v { - return AppLayerResult::incomplete((input.len() - current.len()) as u32, - (current.len() + n) as u32); - } - return AppLayerResult::err(); + return handle_incomplete(input, current, v); } Err(_) => { return AppLayerResult::err(); -- 2.47.2