From bbe9137f2072d4f8e4fe2dd9b709346bf7d8c60a Mon Sep 17 00:00:00 2001 From: frank honza Date: Tue, 14 Apr 2020 12:04:13 +0200 Subject: [PATCH] rfb: Update incomplete handling in parser. This commit adds an updated incomplete handling for the RFB-Parser. If incomplete data is processed, the successfully consumed position and length of remainder + 1 is returned. If the next packet is not empty suricata will call the parser again. This commit is a result of discussion on https://github.com/OISF/suricata/pull/4792. --- rust/src/rfb/rfb.rs | 76 ++++++++++++++++++++++++--------------------- 1 file changed, 40 insertions(+), 36 deletions(-) diff --git a/rust/src/rfb/rfb.rs b/rust/src/rfb/rfb.rs index 5e22d1880d..f783d41dcf 100644 --- a/rust/src/rfb/rfb.rs +++ b/rust/src/rfb/rfb.rs @@ -100,20 +100,6 @@ 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 { @@ -173,6 +159,7 @@ impl RFBState { } let mut current = input; + let mut consumed = 0; SCLogDebug!("request_state {}, input_len {}", self.state, input.len()); loop { if current.len() == 0 { @@ -182,7 +169,9 @@ impl RFBState { parser::RFBGlobalState::TSClientProtocolVersion => { match parser::parse_protocol_version(current) { Ok((rem, request)) => { + consumed += current.len() - rem.len(); current = rem; + if request.major == "003" && request.minor == "003" { // in version 3.3 the server decided security type self.state = parser::RFBGlobalState::TCServerSecurityType; @@ -196,8 +185,8 @@ impl RFBState { return AppLayerResult::err(); } } - Err(nom::Err::Incomplete(v)) => { - return handle_incomplete(input, current, v); + Err(nom::Err::Incomplete(_)) => { + return AppLayerResult::incomplete(consumed as u32, (current.len() + 1) as u32); } Err(_) => { return AppLayerResult::err(); @@ -207,6 +196,7 @@ impl RFBState { parser::RFBGlobalState::TSSecurityTypeSelection => { match parser::parse_security_type_selection(current) { Ok((rem, request)) => { + consumed += current.len() - rem.len(); current = rem; let chosen_security_type = request.security_type; @@ -223,8 +213,8 @@ impl RFBState { return AppLayerResult::err(); } } - Err(nom::Err::Incomplete(v)) => { - return handle_incomplete(input, current, v); + Err(nom::Err::Incomplete(_)) => { + return AppLayerResult::incomplete(consumed as u32, (current.len() + 1) as u32); } Err(_) => { return AppLayerResult::err(); @@ -234,6 +224,7 @@ impl RFBState { parser::RFBGlobalState::TSVncResponse => { match parser::parse_vnc_auth(current) { Ok((rem, request)) => { + consumed += current.len() - rem.len(); current = rem; self.state = parser::RFBGlobalState::TCSecurityResult; @@ -244,8 +235,8 @@ impl RFBState { return AppLayerResult::err(); } } - Err(nom::Err::Incomplete(v)) => { - return handle_incomplete(input, current, v); + Err(nom::Err::Incomplete(_)) => { + return AppLayerResult::incomplete(consumed as u32, (current.len() + 1) as u32); } Err(_) => { return AppLayerResult::err(); @@ -255,7 +246,9 @@ impl RFBState { parser::RFBGlobalState::TSClientInit => { match parser::parse_client_init(current) { Ok((rem, request)) => { + consumed += current.len() - rem.len(); current = rem; + self.state = parser::RFBGlobalState::TCServerInit; if let Some(current_transaction) = self.get_current_tx() { @@ -264,8 +257,8 @@ impl RFBState { return AppLayerResult::err(); } } - Err(nom::Err::Incomplete(v)) => { - return handle_incomplete(input, current, v); + Err(nom::Err::Incomplete(_)) => { + return AppLayerResult::incomplete(consumed as u32, (current.len() + 1) as u32); } Err(_) => { return AppLayerResult::err(); @@ -295,6 +288,7 @@ impl RFBState { } let mut current = input; + let mut consumed = 0; SCLogDebug!("response_state {}, response_len {}", self.state, input.len()); loop { if current.len() == 0 { @@ -304,7 +298,9 @@ impl RFBState { parser::RFBGlobalState::TCServerProtocolVersion => { match parser::parse_protocol_version(current) { Ok((rem, request)) => { + consumed += current.len() - rem.len(); current = rem; + self.state = parser::RFBGlobalState::TSClientProtocolVersion; let tx = self.new_tx(); self.transactions.push(tx); @@ -315,8 +311,8 @@ impl RFBState { return AppLayerResult::err(); } } - Err(nom::Err::Incomplete(v)) => { - return handle_incomplete(input, current, v); + Err(nom::Err::Incomplete(_)) => { + return AppLayerResult::incomplete(consumed as u32, (current.len() + 1) as u32); } Err(_) => { return AppLayerResult::err(); @@ -326,7 +322,9 @@ impl RFBState { parser::RFBGlobalState::TCSupportedSecurityTypes => { match parser::parse_supported_security_types(current) { Ok((rem, request)) => { + consumed += current.len() - rem.len(); current = rem; + SCLogDebug!( "supported_security_types: {}, types: {}", request.number_of_types, request.types.iter().map(ToString::to_string).map(|v| v + " ").collect::() @@ -343,8 +341,8 @@ impl RFBState { return AppLayerResult::err(); } } - Err(nom::Err::Incomplete(v)) => { - return handle_incomplete(input, current, v); + Err(nom::Err::Incomplete(_)) => { + return AppLayerResult::incomplete(consumed as u32, (current.len() + 1) as u32); } Err(_) => { return AppLayerResult::err(); @@ -355,7 +353,9 @@ impl RFBState { // In RFB 3.3, the server decides the authentication type match parser::parse_server_security_type(current) { Ok((rem, request)) => { + consumed += current.len() - rem.len(); current = rem; + let chosen_security_type = request.security_type; SCLogDebug!("chosen_security_type: {}", chosen_security_type); match chosen_security_type { @@ -375,8 +375,8 @@ impl RFBState { return AppLayerResult::err(); } } - Err(nom::Err::Incomplete(v)) => { - return handle_incomplete(input, current, v); + Err(nom::Err::Incomplete(_)) => { + return AppLayerResult::incomplete(consumed as u32, (current.len() + 1) as u32); } Err(_) => { return AppLayerResult::err(); @@ -386,6 +386,7 @@ impl RFBState { parser::RFBGlobalState::TCVncChallenge => { match parser::parse_vnc_auth(current) { Ok((rem, request)) => { + consumed += current.len() - rem.len(); current = rem; self.state = parser::RFBGlobalState::TSVncResponse; @@ -396,8 +397,8 @@ impl RFBState { return AppLayerResult::err(); } } - Err(nom::Err::Incomplete(v)) => { - return handle_incomplete(input, current, v); + Err(nom::Err::Incomplete(_)) => { + return AppLayerResult::incomplete(consumed as u32, (current.len() + 1) as u32); } Err(_) => { return AppLayerResult::err(); @@ -407,6 +408,7 @@ impl RFBState { parser::RFBGlobalState::TCSecurityResult => { match parser::parse_security_result(current) { Ok((rem, request)) => { + consumed += current.len() - rem.len(); current = rem; if request.status == 0 { @@ -423,8 +425,8 @@ impl RFBState { // TODO: Event: unknown security result value } } - Err(nom::Err::Incomplete(v)) => { - return handle_incomplete(input, current, v); + Err(nom::Err::Incomplete(_)) => { + return AppLayerResult::incomplete(consumed as u32, (current.len() + 1) as u32); } Err(_) => { return AppLayerResult::err(); @@ -441,8 +443,8 @@ impl RFBState { } return AppLayerResult::err(); } - Err(nom::Err::Incomplete(v)) => { - return handle_incomplete(input, current, v); + Err(nom::Err::Incomplete(_)) => { + return AppLayerResult::incomplete(consumed as u32, (current.len() + 1) as u32); } Err(_) => { return AppLayerResult::err(); @@ -452,7 +454,9 @@ impl RFBState { parser::RFBGlobalState::TCServerInit => { match parser::parse_server_init(current) { Ok((rem, request)) => { + consumed += current.len() - rem.len(); current = rem; + self.state = parser::RFBGlobalState::Message; if let Some(current_transaction) = self.get_current_tx() { @@ -463,8 +467,8 @@ impl RFBState { return AppLayerResult::err(); } } - Err(nom::Err::Incomplete(v)) => { - return handle_incomplete(input, current, v); + Err(nom::Err::Incomplete(_)) => { + return AppLayerResult::incomplete(consumed as u32, (current.len() + 1) as u32); } Err(_) => { return AppLayerResult::err(); -- 2.47.2