From cedee76ef1a5b82a15021fd843d744702ab7691f Mon Sep 17 00:00:00 2001 From: Victor Julien Date: Wed, 12 Jul 2023 19:31:00 +0200 Subject: [PATCH 1/6] requirements: use libhtp 0.5.45 (cherry picked from commit ce055111fe98d57d7e1c07e42abdd04a2f7d5319) --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index 2f93e685674c..758b56ab7c01 100644 --- a/requirements.txt +++ b/requirements.txt @@ -3,5 +3,5 @@ # Format: # # name {repo} {branch|tag} -libhtp https://github.com/OISF/libhtp 0.5.44 +libhtp https://github.com/OISF/libhtp 0.5.45 suricata-update https://github.com/OISF/suricata-update 1.2.8 From 66d55a7f35ed16601ff1875b38e5819340d57e5d Mon Sep 17 00:00:00 2001 From: Sascha Steinbiss Date: Fri, 9 Jun 2023 11:12:02 +0200 Subject: [PATCH 2/6] rfb: be more strict parsing the version (cherry picked from commit bd1fbf392e04e0bfc4b8f7e680636ddee0a47c60) --- rust/src/rfb/parser.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/rust/src/rfb/parser.rs b/rust/src/rfb/parser.rs index 8aed9c7f817c..9ec311eb3b4f 100644 --- a/rust/src/rfb/parser.rs +++ b/rust/src/rfb/parser.rs @@ -112,12 +112,11 @@ pub struct ServerInit { named!(pub parse_protocol_version, do_parse!( - _rfb_string: take_str!(3) - >> be_u8 + _rfb_string: tag!("RFB ") >> major: take_str!(3) - >> be_u8 + >> _sep1: tag!(".") >> minor: take_str!(3) - >> be_u8 + >> _sep2: tag!("\n") >> ( ProtocolVersion{ major: major.to_string(), From 7a52e39297f498efaeda267b0ea42382a63d6514 Mon Sep 17 00:00:00 2001 From: Sascha Steinbiss Date: Fri, 9 Jun 2023 11:12:24 +0200 Subject: [PATCH 3/6] rfb: add myself as contributor (cherry picked from commit 836fff3679e6ea3b2ed75f87931ff3c7ec0ebd33) --- rust/src/rfb/rfb.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/rust/src/rfb/rfb.rs b/rust/src/rfb/rfb.rs index aa18ab5d4021..e167eb66d865 100644 --- a/rust/src/rfb/rfb.rs +++ b/rust/src/rfb/rfb.rs @@ -16,6 +16,7 @@ */ // Author: Frank Honza +// Sascha Steinbiss use std; use std::ffi::CString; From 8f1e08b28e149c22e1c85e1b0f77b434c8e7ef79 Mon Sep 17 00:00:00 2001 From: Sascha Steinbiss Date: Fri, 9 Jun 2023 11:13:35 +0200 Subject: [PATCH 4/6] rfb: never return error on unknown traffic We only try to parse a small subset of what is possible in RFB. Currently we only understand some standard auth schemes and stop parsing when the server-client handshake is complete. Since in IPS mode returning an error from the parser causes drops that are likely uncalled for, we do not want to return errors when we simply do not understand what happens in the traffic. This addresses Redmine #5912. Bug: #5915. (cherry picked from commit 1f8a5874fbc6816a7aeb59ba668ebd2bf7c206ed) --- rules/Makefile.am | 1 + rules/rfb-events.rules | 10 + rust/src/rfb/detect.rs | 14 +- rust/src/rfb/logger.rs | 38 ++-- rust/src/rfb/mod.rs | 2 +- rust/src/rfb/parser.rs | 47 ++--- rust/src/rfb/rfb.rs | 426 ++++++++++++++++++++++++++++------------- 7 files changed, 357 insertions(+), 181 deletions(-) create mode 100644 rules/rfb-events.rules diff --git a/rules/Makefile.am b/rules/Makefile.am index e4d5635c51ae..57116798368f 100644 --- a/rules/Makefile.am +++ b/rules/Makefile.am @@ -15,6 +15,7 @@ modbus-events.rules \ mqtt-events.rules \ nfs-events.rules \ ntp-events.rules \ +rfb-events.rules \ smb-events.rules \ smtp-events.rules \ ssh-events.rules \ diff --git a/rules/rfb-events.rules b/rules/rfb-events.rules new file mode 100644 index 000000000000..08bc493f4270 --- /dev/null +++ b/rules/rfb-events.rules @@ -0,0 +1,10 @@ +# RFB app-layer event rules. +# +# These SIDs fall in the 2233000+ range. See: +# http://doc.emergingthreats.net/bin/view/Main/SidAllocation and +# https://redmine.openinfosecfoundation.org/projects/suricata/wiki/AppLayer + +alert rfb any any -> any any (msg:"SURICATA RFB Malformed or unknown message"; app-layer-event:rfb.malformed_message; classtype:protocol-command-decode; sid:2233000; rev:1;) +alert rfb any any -> any any (msg:"SURICATA RFB Unimplemented security type"; app-layer-event:rfb.unimplemented_security_type; classtype:protocol-command-decode; sid:2233001; rev:1;) +alert rfb any any -> any any (msg:"SURICATA RFB Unknown security result"; app-layer-event:rfb.unknown_security_result; classtype:protocol-command-decode; sid:2233002; rev:1;) +alert rfb any any -> any any (msg:"SURICATA RFB Unexpected State in Parser"; app-layer-event:rfb.confused_state; classtype:protocol-command-decode; sid:2233003; rev:1;) diff --git a/rust/src/rfb/detect.rs b/rust/src/rfb/detect.rs index d30014a13999..da876457ae54 100644 --- a/rust/src/rfb/detect.rs +++ b/rust/src/rfb/detect.rs @@ -22,9 +22,7 @@ use std::ptr; #[no_mangle] pub unsafe extern "C" fn rs_rfb_tx_get_name( - tx: &mut RFBTransaction, - buffer: *mut *const u8, - buffer_len: *mut u32, + tx: &mut RFBTransaction, buffer: *mut *const u8, buffer_len: *mut u32, ) -> u8 { if let Some(ref r) = tx.tc_server_init { let p = &r.name; @@ -42,10 +40,7 @@ pub unsafe extern "C" fn rs_rfb_tx_get_name( } #[no_mangle] -pub unsafe extern "C" fn rs_rfb_tx_get_sectype( - tx: &mut RFBTransaction, - sectype: *mut u32, -) -> u8 { +pub unsafe extern "C" fn rs_rfb_tx_get_sectype(tx: &mut RFBTransaction, sectype: *mut u32) -> u8 { if let Some(ref r) = tx.chosen_security_type { *sectype = *r; return 1; @@ -58,8 +53,7 @@ pub unsafe extern "C" fn rs_rfb_tx_get_sectype( #[no_mangle] pub unsafe extern "C" fn rs_rfb_tx_get_secresult( - tx: &mut RFBTransaction, - secresult: *mut u32, + tx: &mut RFBTransaction, secresult: *mut u32, ) -> u8 { if let Some(ref r) = tx.tc_security_result { *secresult = r.status; @@ -67,4 +61,4 @@ pub unsafe extern "C" fn rs_rfb_tx_get_secresult( } return 0; -} \ No newline at end of file +} diff --git a/rust/src/rfb/logger.rs b/rust/src/rfb/logger.rs index c990b3dbb647..5f8be358cdd0 100644 --- a/rust/src/rfb/logger.rs +++ b/rust/src/rfb/logger.rs @@ -17,10 +17,10 @@ // Author: Frank Honza -use std; -use std::fmt::Write; use super::rfb::{RFBState, RFBTransaction}; use crate::jsonbuilder::{JsonBuilder, JsonError}; +use std; +use std::fmt::Write; fn log_rfb(tx: &RFBTransaction, js: &mut JsonBuilder) -> Result<(), JsonError> { js.open_object("rfb")?; @@ -63,15 +63,17 @@ fn log_rfb(tx: &RFBTransaction, js: &mut JsonBuilder) -> Result<(), JsonError> { } js.close()?; } - _ => () + _ => (), } if let Some(security_result) = &tx.tc_security_result { let _ = match security_result.status { 0 => js.set_string("security_result", "OK")?, 1 => js.set_string("security-result", "FAIL")?, 2 => js.set_string("security_result", "TOOMANY")?, - _ => js.set_string("security_result", - &format!("UNKNOWN ({})", security_result.status))?, + _ => js.set_string( + "security_result", + &format!("UNKNOWN ({})", security_result.status), + )?, }; } js.close()?; // Close authentication. @@ -91,15 +93,27 @@ fn log_rfb(tx: &RFBTransaction, js: &mut JsonBuilder) -> Result<(), JsonError> { js.set_string_from_bytes("name", &tc_server_init.name)?; js.open_object("pixel_format")?; - js.set_uint("bits_per_pixel", tc_server_init.pixel_format.bits_per_pixel as u64)?; + js.set_uint( + "bits_per_pixel", + tc_server_init.pixel_format.bits_per_pixel as u64, + )?; js.set_uint("depth", tc_server_init.pixel_format.depth as u64)?; - js.set_bool("big_endian", tc_server_init.pixel_format.big_endian_flag != 0)?; - js.set_bool("true_color", tc_server_init.pixel_format.true_colour_flag != 0)?; + js.set_bool( + "big_endian", + tc_server_init.pixel_format.big_endian_flag != 0, + )?; + js.set_bool( + "true_color", + tc_server_init.pixel_format.true_colour_flag != 0, + )?; js.set_uint("red_max", tc_server_init.pixel_format.red_max as u64)?; js.set_uint("green_max", tc_server_init.pixel_format.green_max as u64)?; js.set_uint("blue_max", tc_server_init.pixel_format.blue_max as u64)?; js.set_uint("red_shift", tc_server_init.pixel_format.red_shift as u64)?; - js.set_uint("green_shift", tc_server_init.pixel_format.green_shift as u64)?; + js.set_uint( + "green_shift", + tc_server_init.pixel_format.green_shift as u64, + )?; js.set_uint("blue_shift", tc_server_init.pixel_format.blue_shift as u64)?; js.close()?; @@ -112,9 +126,9 @@ fn log_rfb(tx: &RFBTransaction, js: &mut JsonBuilder) -> Result<(), JsonError> { } #[no_mangle] -pub extern "C" fn rs_rfb_logger_log(_state: &mut RFBState, - tx: *mut std::os::raw::c_void, - js: &mut JsonBuilder) -> bool { +pub extern "C" fn rs_rfb_logger_log( + _state: &mut RFBState, tx: *mut std::os::raw::c_void, js: &mut JsonBuilder, +) -> bool { let tx = cast_pointer!(tx, RFBTransaction); log_rfb(tx, js).is_ok() } diff --git a/rust/src/rfb/mod.rs b/rust/src/rfb/mod.rs index 4bde7935cb71..68d37ec8a0eb 100644 --- a/rust/src/rfb/mod.rs +++ b/rust/src/rfb/mod.rs @@ -20,4 +20,4 @@ pub mod detect; pub mod logger; pub mod parser; -pub mod rfb; \ No newline at end of file +pub mod rfb; diff --git a/rust/src/rfb/parser.rs b/rust/src/rfb/parser.rs index 9ec311eb3b4f..37c6d3d16eb6 100644 --- a/rust/src/rfb/parser.rs +++ b/rust/src/rfb/parser.rs @@ -17,10 +17,11 @@ // Author: Frank Honza -use std::fmt; -use nom::*; use nom::number::streaming::*; +use nom::*; +use std::fmt; +#[derive(Debug, PartialEq)] pub enum RFBGlobalState { TCServerProtocolVersion, TCSupportedSecurityTypes, @@ -33,7 +34,7 @@ pub enum RFBGlobalState { TSVncResponse, TCSecurityResult, TSClientInit, - Message + Skip, } impl fmt::Display for RFBGlobalState { @@ -50,43 +51,43 @@ impl fmt::Display for RFBGlobalState { RFBGlobalState::TCSecurityResult => write!(f, "TCSecurityResult"), RFBGlobalState::TCServerSecurityType => write!(f, "TCServerSecurityType"), RFBGlobalState::TSClientInit => write!(f, "TSClientInit"), - RFBGlobalState::Message => write!(f, "Message") + RFBGlobalState::Skip => write!(f, "Skip"), } } } pub struct ProtocolVersion { pub major: String, - pub minor: String + pub minor: String, } pub struct SupportedSecurityTypes { pub number_of_types: u8, - pub types: Vec + pub types: Vec, } pub struct SecurityTypeSelection { - pub security_type: u8 + pub security_type: u8, } pub struct ServerSecurityType { - pub security_type: u32 + pub security_type: u32, } pub struct SecurityResult { - pub status: u32 + pub status: u32, } pub struct FailureReason { - pub reason_string: String + pub reason_string: String, } pub struct VncAuth { - pub secret: Vec + pub secret: Vec, } pub struct ClientInit { - pub shared: u8 + pub shared: u8, } pub struct PixelFormat { @@ -107,7 +108,7 @@ pub struct ServerInit { pub height: u16, pub pixel_format: PixelFormat, pub name_length: u32, - pub name: Vec + pub name: Vec, } named!(pub parse_protocol_version, @@ -257,8 +258,8 @@ named!(pub parse_server_init, #[cfg(test)] mod tests { - use nom::*; use super::*; + use nom::*; /// Simple test of some valid data. #[test] @@ -277,8 +278,7 @@ mod tests { Err(Err::Incomplete(_)) => { panic!("Result should not have been incomplete."); } - Err(Err::Error(err)) | - Err(Err::Failure(err)) => { + Err(Err::Error(err)) | Err(Err::Failure(err)) => { panic!("Result should not be an error: {:?}.", err); } } @@ -287,13 +287,10 @@ mod tests { #[test] fn test_parse_server_init() { let buf = [ - 0x05, 0x00, 0x03, 0x20, 0x20, 0x18, 0x00, 0x01, - 0x00, 0xff, 0x00, 0xff, 0x00, 0xff, 0x10, 0x08, - 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x1e, - 0x61, 0x6e, 0x65, 0x61, 0x67, 0x6c, 0x65, 0x73, - 0x40, 0x6c, 0x6f, 0x63, 0x61, 0x6c, 0x68, 0x6f, - 0x73, 0x74, 0x2e, 0x6c, 0x6f, 0x63, 0x61, 0x6c, - 0x64, 0x6f, 0x6d, 0x61, 0x69, 0x6e + 0x05, 0x00, 0x03, 0x20, 0x20, 0x18, 0x00, 0x01, 0x00, 0xff, 0x00, 0xff, 0x00, 0xff, + 0x10, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x1e, 0x61, 0x6e, 0x65, 0x61, + 0x67, 0x6c, 0x65, 0x73, 0x40, 0x6c, 0x6f, 0x63, 0x61, 0x6c, 0x68, 0x6f, 0x73, 0x74, + 0x2e, 0x6c, 0x6f, 0x63, 0x61, 0x6c, 0x64, 0x6f, 0x6d, 0x61, 0x69, 0x6e, ]; let result = parse_server_init(&buf); @@ -310,11 +307,9 @@ mod tests { Err(Err::Incomplete(_)) => { panic!("Result should not have been incomplete."); } - Err(Err::Error(err)) | - Err(Err::Failure(err)) => { + Err(Err::Error(err)) | Err(Err::Failure(err)) => { panic!("Result should not be an error: {:?}.", err); } } } - } diff --git a/rust/src/rfb/rfb.rs b/rust/src/rfb/rfb.rs index e167eb66d865..1ad79370dc92 100644 --- a/rust/src/rfb/rfb.rs +++ b/rust/src/rfb/rfb.rs @@ -18,17 +18,37 @@ // Author: Frank Honza // Sascha Steinbiss -use std; -use std::ffi::CString; -use std::mem::transmute; -use crate::core::{self, ALPROTO_UNKNOWN, AppProto, Flow, IPPROTO_TCP}; +use super::parser; use crate::applayer; use crate::applayer::*; +use crate::core::{self, AppProto, Flow, ALPROTO_UNKNOWN, IPPROTO_TCP}; use nom; -use super::parser; +use std; +use std::ffi::{CStr, CString}; +use std::mem::transmute; static mut ALPROTO_RFB: AppProto = ALPROTO_UNKNOWN; +#[derive(FromPrimitive, Debug)] +pub enum RFBEvent { + UnimplementedSecurityType = 0, + UnknownSecurityResult, + MalformedMessage, + ConfusedState, +} + +impl RFBEvent { + fn from_i32(value: i32) -> Option { + match value { + 0 => Some(RFBEvent::UnimplementedSecurityType), + 1 => Some(RFBEvent::UnknownSecurityResult), + 2 => Some(RFBEvent::MalformedMessage), + 3 => Some(RFBEvent::ConfusedState), + _ => None, + } + } +} + pub struct RFBTransaction { tx_id: u64, pub complete: bool, @@ -84,6 +104,10 @@ impl RFBTransaction { core::sc_detect_engine_state_free(state); } } + + fn set_event(&mut self, event: RFBEvent) { + core::sc_app_layer_decoder_events_set_event_raw(&mut self.events, event as u8); + } } impl Drop for RFBTransaction { @@ -95,7 +119,7 @@ impl Drop for RFBTransaction { pub struct RFBState { tx_id: u64, transactions: Vec, - state: parser::RFBGlobalState + state: parser::RFBGlobalState, } impl RFBState { @@ -103,7 +127,7 @@ impl RFBState { Self { tx_id: 0, transactions: Vec::new(), - state: parser::RFBGlobalState::TCServerProtocolVersion + state: parser::RFBGlobalState::TCServerProtocolVersion, } } @@ -180,13 +204,19 @@ impl RFBState { if let Some(current_transaction) = self.get_current_tx() { current_transaction.ts_client_protocol_version = Some(request); } else { - return AppLayerResult::err(); + debug_validate_fail!( + "no transaction set at protocol selection stage" + ); } } Err(nom::Err::Incomplete(_)) => { - return AppLayerResult::incomplete(consumed as u32, (current.len() + 1) as u32); + return AppLayerResult::incomplete( + consumed as u32, + (current.len() + 1) as u32, + ); } Err(_) => { + // We even failed to parse the protocol version. return AppLayerResult::err(); } } @@ -201,21 +231,41 @@ impl RFBState { match chosen_security_type { 2 => self.state = parser::RFBGlobalState::TCVncChallenge, 1 => self.state = parser::RFBGlobalState::TSClientInit, - _ => return AppLayerResult::err(), + _ => { + if let Some(current_transaction) = self.get_current_tx() { + current_transaction + .set_event(RFBEvent::UnimplementedSecurityType); + } + // We have just have seen a security type we don't know about. + // This is not bad per se, it might just mean this is a + // proprietary one not in the spec. + // Continue the flow but stop trying to map the protocol. + self.state = parser::RFBGlobalState::Skip; + return AppLayerResult::ok(); + } } if let Some(current_transaction) = self.get_current_tx() { current_transaction.ts_security_type_selection = Some(request); current_transaction.chosen_security_type = Some(chosen_security_type as u32); } else { - return AppLayerResult::err(); + debug_validate_fail!("no transaction set at security type stage"); } } Err(nom::Err::Incomplete(_)) => { - return AppLayerResult::incomplete(consumed as u32, (current.len() + 1) as u32); + return AppLayerResult::incomplete( + consumed as u32, + (current.len() + 1) as u32, + ); } Err(_) => { - return AppLayerResult::err(); + if let Some(current_transaction) = self.get_current_tx() { + current_transaction.set_event(RFBEvent::MalformedMessage); + } + // We failed to parse the security type. + // Continue the flow but stop trying to map the protocol. + self.state = parser::RFBGlobalState::Skip; + return AppLayerResult::ok(); } } } @@ -230,14 +280,22 @@ impl RFBState { if let Some(current_transaction) = self.get_current_tx() { current_transaction.ts_vnc_response = Some(request); } else { - return AppLayerResult::err(); + debug_validate_fail!("no transaction set at security result stage"); } } Err(nom::Err::Incomplete(_)) => { - return AppLayerResult::incomplete(consumed as u32, (current.len() + 1) as u32); + return AppLayerResult::incomplete( + consumed as u32, + (current.len() + 1) as u32, + ); } Err(_) => { - return AppLayerResult::err(); + if let Some(current_transaction) = self.get_current_tx() { + current_transaction.set_event(RFBEvent::MalformedMessage); + } + // Continue the flow but stop trying to map the protocol. + self.state = parser::RFBGlobalState::Skip; + return AppLayerResult::ok(); } } } @@ -252,28 +310,45 @@ impl RFBState { if let Some(current_transaction) = self.get_current_tx() { current_transaction.ts_client_init = Some(request); } else { - return AppLayerResult::err(); + debug_validate_fail!("no transaction set at client init stage"); } } Err(nom::Err::Incomplete(_)) => { - return AppLayerResult::incomplete(consumed as u32, (current.len() + 1) as u32); + return AppLayerResult::incomplete( + consumed as u32, + (current.len() + 1) as u32, + ); } Err(_) => { - return AppLayerResult::err(); + if let Some(current_transaction) = self.get_current_tx() { + current_transaction.set_event(RFBEvent::MalformedMessage); + } + // We failed to parse the client init. + // Continue the flow but stop trying to map the protocol. + self.state = parser::RFBGlobalState::Skip; + return AppLayerResult::ok(); } } } - parser::RFBGlobalState::Message => { - //todo implement RFB messages, for now we stop here - return AppLayerResult::err(); - } - parser::RFBGlobalState::TCServerProtocolVersion => { - SCLogDebug!("Reversed traffic, expected response."); - return AppLayerResult::err(); + parser::RFBGlobalState::Skip => { + // End of parseable handshake reached, skip rest of traffic + return AppLayerResult::ok(); } _ => { - SCLogDebug!("Invalid state for request {}", self.state); - current = b""; + // We have gotten out of sync with the expected state flow. + // This could happen since we use a global state (i.e. that + // is used for both directions), but if traffic can not be + // parsed as expected elsewhere, we might not have advanced + // a state for one direction but received data in the + // "unexpected" direction, causing the parser to end up + // here. Let's stop trying to parse the traffic but still + // accept it. + SCLogDebug!("Invalid state for request: {}", self.state); + if let Some(current_transaction) = self.get_current_tx() { + current_transaction.set_event(RFBEvent::ConfusedState); + } + self.state = parser::RFBGlobalState::Skip; + return AppLayerResult::ok(); } } } @@ -287,7 +362,11 @@ impl RFBState { let mut current = input; let mut consumed = 0; - SCLogDebug!("response_state {}, response_len {}", self.state, input.len()); + SCLogDebug!( + "response_state {}, response_len {}", + self.state, + input.len() + ); loop { if current.len() == 0 { return AppLayerResult::ok(); @@ -306,13 +385,17 @@ impl RFBState { if let Some(current_transaction) = self.get_current_tx() { current_transaction.tc_server_protocol_version = Some(request); } else { - return AppLayerResult::err(); + debug_validate_fail!("no transaction set but we just set one"); } } Err(nom::Err::Incomplete(_)) => { - return AppLayerResult::incomplete(consumed as u32, (current.len() + 1) as u32); + return AppLayerResult::incomplete( + consumed as u32, + (current.len() + 1) as u32, + ); } Err(_) => { + // We even failed to parse the protocol version. return AppLayerResult::err(); } } @@ -324,8 +407,14 @@ impl RFBState { current = rem; SCLogDebug!( - "supported_security_types: {}, types: {}", request.number_of_types, - request.types.iter().map(ToString::to_string).map(|v| v + " ").collect::() + "supported_security_types: {}, types: {}", + request.number_of_types, + request + .types + .iter() + .map(ToString::to_string) + .map(|v| v + " ") + .collect::() ); self.state = parser::RFBGlobalState::TSSecurityTypeSelection; @@ -336,14 +425,22 @@ impl RFBState { if let Some(current_transaction) = self.get_current_tx() { current_transaction.tc_supported_security_types = Some(request); } else { - return AppLayerResult::err(); + debug_validate_fail!("no transaction set at security type stage"); } } Err(nom::Err::Incomplete(_)) => { - return AppLayerResult::incomplete(consumed as u32, (current.len() + 1) as u32); + return AppLayerResult::incomplete( + consumed as u32, + (current.len() + 1) as u32, + ); } Err(_) => { - return AppLayerResult::err(); + if let Some(current_transaction) = self.get_current_tx() { + current_transaction.set_event(RFBEvent::MalformedMessage); + } + // Continue the flow but stop trying to map the protocol. + self.state = parser::RFBGlobalState::Skip; + return AppLayerResult::ok(); } } } @@ -361,23 +458,44 @@ impl RFBState { 1 => self.state = parser::RFBGlobalState::TSClientInit, 2 => self.state = parser::RFBGlobalState::TCVncChallenge, _ => { - // TODO Event unknown security type - return AppLayerResult::err(); + if let Some(current_transaction) = self.get_current_tx() { + current_transaction + .set_event(RFBEvent::UnimplementedSecurityType); + } else { + debug_validate_fail!( + "no transaction set at security type stage" + ); + } + // We have just have seen a security type we don't know about. + // This is not bad per se, it might just mean this is a + // proprietary one not in the spec. + // Continue the flow but stop trying to map the protocol. + self.state = parser::RFBGlobalState::Skip; + return AppLayerResult::ok(); } } if let Some(current_transaction) = self.get_current_tx() { current_transaction.tc_server_security_type = Some(request); - current_transaction.chosen_security_type = Some(chosen_security_type); + current_transaction.chosen_security_type = + Some(chosen_security_type); } else { - return AppLayerResult::err(); + debug_validate_fail!("no transaction set at security type stage"); } } Err(nom::Err::Incomplete(_)) => { - return AppLayerResult::incomplete(consumed as u32, (current.len() + 1) as u32); + return AppLayerResult::incomplete( + consumed as u32, + (current.len() + 1) as u32, + ); } Err(_) => { - return AppLayerResult::err(); + if let Some(current_transaction) = self.get_current_tx() { + current_transaction.set_event(RFBEvent::MalformedMessage); + } + // Continue the flow but stop trying to map the protocol. + self.state = parser::RFBGlobalState::Skip; + return AppLayerResult::ok(); } } } @@ -392,14 +510,22 @@ impl RFBState { if let Some(current_transaction) = self.get_current_tx() { current_transaction.tc_vnc_challenge = Some(request); } else { - return AppLayerResult::err(); + debug_validate_fail!("no transaction set at auth stage"); } } Err(nom::Err::Incomplete(_)) => { - return AppLayerResult::incomplete(consumed as u32, (current.len() + 1) as u32); + return AppLayerResult::incomplete( + consumed as u32, + (current.len() + 1) as u32, + ); } Err(_) => { - return AppLayerResult::err(); + if let Some(current_transaction) = self.get_current_tx() { + current_transaction.set_event(RFBEvent::MalformedMessage); + } + // Continue the flow but stop trying to map the protocol. + self.state = parser::RFBGlobalState::Skip; + return AppLayerResult::ok(); } } } @@ -415,19 +541,34 @@ impl RFBState { if let Some(current_transaction) = self.get_current_tx() { current_transaction.tc_security_result = Some(request); } else { - return AppLayerResult::err(); + debug_validate_fail!( + "no transaction set at security result stage" + ); } } else if request.status == 1 { self.state = parser::RFBGlobalState::TCFailureReason; } else { - // TODO: Event: unknown security result value + if let Some(current_transaction) = self.get_current_tx() { + current_transaction.set_event(RFBEvent::UnknownSecurityResult); + } + // Continue the flow but stop trying to map the protocol. + self.state = parser::RFBGlobalState::Skip; + return AppLayerResult::ok(); } } Err(nom::Err::Incomplete(_)) => { - return AppLayerResult::incomplete(consumed as u32, (current.len() + 1) as u32); + return AppLayerResult::incomplete( + consumed as u32, + (current.len() + 1) as u32, + ); } Err(_) => { - return AppLayerResult::err(); + if let Some(current_transaction) = self.get_current_tx() { + current_transaction.set_event(RFBEvent::MalformedMessage); + } + // Continue the flow but stop trying to map the protocol. + self.state = parser::RFBGlobalState::Skip; + return AppLayerResult::ok(); } } } @@ -437,15 +578,23 @@ impl RFBState { if let Some(current_transaction) = self.get_current_tx() { current_transaction.tc_failure_reason = Some(request); } else { - return AppLayerResult::err(); + debug_validate_fail!("no transaction set at failure reason stage"); } - return AppLayerResult::err(); + return AppLayerResult::ok(); } Err(nom::Err::Incomplete(_)) => { - return AppLayerResult::incomplete(consumed as u32, (current.len() + 1) as u32); + return AppLayerResult::incomplete( + consumed as u32, + (current.len() + 1) as u32, + ); } Err(_) => { - return AppLayerResult::err(); + if let Some(current_transaction) = self.get_current_tx() { + current_transaction.set_event(RFBEvent::MalformedMessage); + } + // Continue the flow but stop trying to map the protocol. + self.state = parser::RFBGlobalState::Skip; + return AppLayerResult::ok(); } } } @@ -455,40 +604,58 @@ impl RFBState { consumed += current.len() - rem.len(); current = rem; - self.state = parser::RFBGlobalState::Message; + self.state = parser::RFBGlobalState::Skip; if let Some(current_transaction) = self.get_current_tx() { current_transaction.tc_server_init = Some(request); // connection initialization is complete and parsed current_transaction.complete = true; } else { - return AppLayerResult::err(); + debug_validate_fail!("no transaction set at server init stage"); } } Err(nom::Err::Incomplete(_)) => { - return AppLayerResult::incomplete(consumed as u32, (current.len() + 1) as u32); + return AppLayerResult::incomplete( + consumed as u32, + (current.len() + 1) as u32, + ); } Err(_) => { - return AppLayerResult::err(); + if let Some(current_transaction) = self.get_current_tx() { + current_transaction.set_event(RFBEvent::MalformedMessage); + } + // Continue the flow but stop trying to map the protocol. + self.state = parser::RFBGlobalState::Skip; + return AppLayerResult::ok(); } } } - parser::RFBGlobalState::Message => { + parser::RFBGlobalState::Skip => { //todo implement RFB messages, for now we stop here - return AppLayerResult::err(); + return AppLayerResult::ok(); } _ => { - SCLogDebug!("Invalid state for response"); - return AppLayerResult::err(); + // We have gotten out of sync with the expected state flow. + // This could happen since we use a global state (i.e. that + // is used for both directions), but if traffic can not be + // parsed as expected elsewhere, we might not have advanced + // a state for one direction but received data in the + // "unexpected" direction, causing the parser to end up + // here. Let's stop trying to parse the traffic but still + // accept it. + SCLogDebug!("Invalid state for response: {}", self.state); + if let Some(current_transaction) = self.get_current_tx() { + current_transaction.set_event(RFBEvent::ConfusedState); + } + self.state = parser::RFBGlobalState::Skip; + return AppLayerResult::ok(); } } } } fn tx_iterator( - &mut self, - min_tx_id: u64, - state: &mut u64, + &mut self, min_tx_id: u64, state: &mut u64, ) -> Option<(&RFBTransaction, u64, bool)> { let mut index = *state as usize; let len = self.transactions.len(); @@ -509,17 +676,13 @@ impl RFBState { // C exports. -export_tx_get_detect_state!( - rs_rfb_tx_get_detect_state, - RFBTransaction -); -export_tx_set_detect_state!( - rs_rfb_tx_set_detect_state, - RFBTransaction -); +export_tx_get_detect_state!(rs_rfb_tx_get_detect_state, RFBTransaction); +export_tx_set_detect_state!(rs_rfb_tx_set_detect_state, RFBTransaction); #[no_mangle] -pub extern "C" fn rs_rfb_state_new(_orig_state: *mut std::os::raw::c_void, _orig_proto: AppProto) -> *mut std::os::raw::c_void { +pub extern "C" fn rs_rfb_state_new( + _orig_state: *mut std::os::raw::c_void, _orig_proto: AppProto, +) -> *mut std::os::raw::c_void { let state = RFBState::new(); let boxed = Box::new(state); return unsafe { transmute(boxed) }; @@ -532,23 +695,15 @@ pub extern "C" fn rs_rfb_state_free(state: *mut std::os::raw::c_void) { } #[no_mangle] -pub extern "C" fn rs_rfb_state_tx_free( - state: *mut std::os::raw::c_void, - tx_id: u64, -) { +pub extern "C" fn rs_rfb_state_tx_free(state: *mut std::os::raw::c_void, tx_id: u64) { let state = cast_pointer!(state, RFBState); state.free_tx(tx_id); } #[no_mangle] pub extern "C" fn rs_rfb_parse_request( - _flow: *const Flow, - state: *mut std::os::raw::c_void, - _pstate: *mut std::os::raw::c_void, - input: *const u8, - input_len: u32, - _data: *const std::os::raw::c_void, - _flags: u8, + _flow: *const Flow, state: *mut std::os::raw::c_void, _pstate: *mut std::os::raw::c_void, + input: *const u8, input_len: u32, _data: *const std::os::raw::c_void, _flags: u8, ) -> AppLayerResult { let state = cast_pointer!(state, RFBState); let buf = build_slice!(input, input_len as usize); @@ -557,13 +712,8 @@ pub extern "C" fn rs_rfb_parse_request( #[no_mangle] pub extern "C" fn rs_rfb_parse_response( - _flow: *const Flow, - state: *mut std::os::raw::c_void, - _pstate: *mut std::os::raw::c_void, - input: *const u8, - input_len: u32, - _data: *const std::os::raw::c_void, - _flags: u8, + _flow: *const Flow, state: *mut std::os::raw::c_void, _pstate: *mut std::os::raw::c_void, + input: *const u8, input_len: u32, _data: *const std::os::raw::c_void, _flags: u8, ) -> AppLayerResult { let state = cast_pointer!(state, RFBState); let buf = build_slice!(input, input_len as usize); @@ -572,8 +722,7 @@ pub extern "C" fn rs_rfb_parse_response( #[no_mangle] pub extern "C" fn rs_rfb_state_get_tx( - state: *mut std::os::raw::c_void, - tx_id: u64, + state: *mut std::os::raw::c_void, tx_id: u64, ) -> *mut std::os::raw::c_void { let state = cast_pointer!(state, RFBState); match state.get_tx(tx_id) { @@ -587,25 +736,20 @@ pub extern "C" fn rs_rfb_state_get_tx( } #[no_mangle] -pub extern "C" fn rs_rfb_state_get_tx_count( - state: *mut std::os::raw::c_void, -) -> u64 { +pub extern "C" fn rs_rfb_state_get_tx_count(state: *mut std::os::raw::c_void) -> u64 { let state = cast_pointer!(state, RFBState); return state.tx_id; } #[no_mangle] -pub extern "C" fn rs_rfb_state_progress_completion_status( - _direction: u8, -) -> std::os::raw::c_int { +pub extern "C" fn rs_rfb_state_progress_completion_status(_direction: u8) -> std::os::raw::c_int { // This parser uses 1 to signal transaction completion status. return 1; } #[no_mangle] pub extern "C" fn rs_rfb_tx_get_alstate_progress( - tx: *mut std::os::raw::c_void, - _direction: u8, + tx: *mut std::os::raw::c_void, _direction: u8, ) -> std::os::raw::c_int { let tx = cast_pointer!(tx, RFBTransaction); if tx.complete { @@ -616,7 +760,7 @@ pub extern "C" fn rs_rfb_tx_get_alstate_progress( #[no_mangle] pub extern "C" fn rs_rfb_state_get_events( - tx: *mut std::os::raw::c_void + tx: *mut std::os::raw::c_void, ) -> *mut core::AppLayerDecoderEvents { let tx = cast_pointer!(tx, RFBTransaction); return tx.events; @@ -624,38 +768,64 @@ pub extern "C" fn rs_rfb_state_get_events( #[no_mangle] pub extern "C" fn rs_rfb_state_get_event_info( - _event_name: *const std::os::raw::c_char, - _event_id: *mut std::os::raw::c_int, - _event_type: *mut core::AppLayerEventType, + event_name: *const std::os::raw::c_char, event_id: *mut std::os::raw::c_int, + event_type: *mut core::AppLayerEventType, ) -> std::os::raw::c_int { - return -1; + if event_name == std::ptr::null() { + return -1; + } + let c_event_name: &CStr = unsafe { CStr::from_ptr(event_name) }; + let event = match c_event_name.to_str() { + Ok(s) => { + match s { + "unimplemented_security_type" => RFBEvent::UnimplementedSecurityType as i32, + "unknown_security_result" => RFBEvent::UnknownSecurityResult as i32, + "malformed_message" => RFBEvent::MalformedMessage as i32, + "confused_state" => RFBEvent::ConfusedState as i32, + _ => -1, // unknown event + } + } + Err(_) => -1, // UTF-8 conversion failed + }; + unsafe { + *event_type = core::APP_LAYER_EVENT_TYPE_TRANSACTION; + *event_id = event as std::os::raw::c_int; + }; + 0 } #[no_mangle] -pub extern "C" fn rs_rfb_state_get_event_info_by_id(_event_id: std::os::raw::c_int, - _event_name: *mut *const std::os::raw::c_char, - _event_type: *mut core::AppLayerEventType +pub extern "C" fn rs_rfb_state_get_event_info_by_id( + event_id: std::os::raw::c_int, event_name: *mut *const std::os::raw::c_char, + event_type: *mut core::AppLayerEventType, ) -> i8 { - return -1; + if let Some(e) = RFBEvent::from_i32(event_id as i32) { + let estr = match e { + RFBEvent::UnimplementedSecurityType => "unimplemented_security_type\0", + RFBEvent::UnknownSecurityResult => "unknown_security_result\0", + RFBEvent::MalformedMessage => "malformed_message\0", + RFBEvent::ConfusedState => "confused_state\0", + }; + unsafe { + *event_name = estr.as_ptr() as *const std::os::raw::c_char; + *event_type = core::APP_LAYER_EVENT_TYPE_TRANSACTION; + }; + 0 + } else { + -1 + } } + #[no_mangle] pub extern "C" fn rs_rfb_state_get_tx_iterator( - _ipproto: u8, - _alproto: AppProto, - state: *mut std::os::raw::c_void, - min_tx_id: u64, - _max_tx_id: u64, - istate: &mut u64, + _ipproto: u8, _alproto: AppProto, state: *mut std::os::raw::c_void, min_tx_id: u64, + _max_tx_id: u64, istate: &mut u64, ) -> applayer::AppLayerGetTxIterTuple { let state = cast_pointer!(state, RFBState); match state.tx_iterator(min_tx_id, istate) { Some((tx, out_tx_id, has_next)) => { let c_tx = unsafe { transmute(tx) }; - let ires = applayer::AppLayerGetTxIterTuple::with_values( - c_tx, - out_tx_id, - has_next, - ); + let ires = applayer::AppLayerGetTxIterTuple::with_values(c_tx, out_tx_id, has_next); return ires; } None => { @@ -693,7 +863,7 @@ pub unsafe extern "C" fn rs_rfb_register_parser() { set_de_state: rs_rfb_tx_set_detect_state, get_events: Some(rs_rfb_state_get_events), get_eventinfo: Some(rs_rfb_state_get_event_info), - get_eventinfo_byid : Some(rs_rfb_state_get_event_info_by_id), + get_eventinfo_byid: Some(rs_rfb_state_get_event_info_by_id), localstorage_new: None, localstorage_free: None, get_files: None, @@ -706,18 +876,10 @@ pub unsafe extern "C" fn rs_rfb_register_parser() { let ip_proto_str = CString::new("tcp").unwrap(); - if AppLayerProtoDetectConfProtoDetectionEnabled( - ip_proto_str.as_ptr(), - parser.name, - ) != 0 - { + if AppLayerProtoDetectConfProtoDetectionEnabled(ip_proto_str.as_ptr(), parser.name) != 0 { let alproto = AppLayerRegisterProtocolDetection(&parser, 1); ALPROTO_RFB = alproto; - if AppLayerParserConfParserEnabled( - ip_proto_str.as_ptr(), - parser.name, - ) != 0 - { + if AppLayerParserConfParserEnabled(ip_proto_str.as_ptr(), parser.name) != 0 { let _ = AppLayerRegisterParser(&parser, alproto); } SCLogDebug!("Rust rfb parser registered."); From b1d2d786225a047977087396a1ef10eefa834afd Mon Sep 17 00:00:00 2001 From: Sascha Steinbiss Date: Fri, 30 Jun 2023 00:20:12 +0200 Subject: [PATCH 5/6] rfb: ensure logging of incompletely parsed txs (cherry picked from commit 1606aca881c5ba1c2cccbbe0de78530d47a4d8a1) --- rust/src/rfb/rfb.rs | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/rust/src/rfb/rfb.rs b/rust/src/rfb/rfb.rs index 1ad79370dc92..926ea1361447 100644 --- a/rust/src/rfb/rfb.rs +++ b/rust/src/rfb/rfb.rs @@ -261,6 +261,7 @@ impl RFBState { Err(_) => { if let Some(current_transaction) = self.get_current_tx() { current_transaction.set_event(RFBEvent::MalformedMessage); + current_transaction.complete = true; } // We failed to parse the security type. // Continue the flow but stop trying to map the protocol. @@ -292,6 +293,7 @@ impl RFBState { Err(_) => { if let Some(current_transaction) = self.get_current_tx() { current_transaction.set_event(RFBEvent::MalformedMessage); + current_transaction.complete = true; } // Continue the flow but stop trying to map the protocol. self.state = parser::RFBGlobalState::Skip; @@ -322,6 +324,7 @@ impl RFBState { Err(_) => { if let Some(current_transaction) = self.get_current_tx() { current_transaction.set_event(RFBEvent::MalformedMessage); + current_transaction.complete = true; } // We failed to parse the client init. // Continue the flow but stop trying to map the protocol. @@ -346,6 +349,7 @@ impl RFBState { SCLogDebug!("Invalid state for request: {}", self.state); if let Some(current_transaction) = self.get_current_tx() { current_transaction.set_event(RFBEvent::ConfusedState); + current_transaction.complete = true; } self.state = parser::RFBGlobalState::Skip; return AppLayerResult::ok(); @@ -437,6 +441,7 @@ impl RFBState { Err(_) => { if let Some(current_transaction) = self.get_current_tx() { current_transaction.set_event(RFBEvent::MalformedMessage); + current_transaction.complete = true; } // Continue the flow but stop trying to map the protocol. self.state = parser::RFBGlobalState::Skip; @@ -461,6 +466,7 @@ impl RFBState { if let Some(current_transaction) = self.get_current_tx() { current_transaction .set_event(RFBEvent::UnimplementedSecurityType); + current_transaction.complete = true; } else { debug_validate_fail!( "no transaction set at security type stage" @@ -492,6 +498,7 @@ impl RFBState { Err(_) => { if let Some(current_transaction) = self.get_current_tx() { current_transaction.set_event(RFBEvent::MalformedMessage); + current_transaction.complete = true; } // Continue the flow but stop trying to map the protocol. self.state = parser::RFBGlobalState::Skip; @@ -522,6 +529,7 @@ impl RFBState { Err(_) => { if let Some(current_transaction) = self.get_current_tx() { current_transaction.set_event(RFBEvent::MalformedMessage); + current_transaction.complete = true; } // Continue the flow but stop trying to map the protocol. self.state = parser::RFBGlobalState::Skip; @@ -550,6 +558,7 @@ impl RFBState { } else { if let Some(current_transaction) = self.get_current_tx() { current_transaction.set_event(RFBEvent::UnknownSecurityResult); + current_transaction.complete = true; } // Continue the flow but stop trying to map the protocol. self.state = parser::RFBGlobalState::Skip; @@ -565,6 +574,7 @@ impl RFBState { Err(_) => { if let Some(current_transaction) = self.get_current_tx() { current_transaction.set_event(RFBEvent::MalformedMessage); + current_transaction.complete = true; } // Continue the flow but stop trying to map the protocol. self.state = parser::RFBGlobalState::Skip; @@ -591,6 +601,7 @@ impl RFBState { Err(_) => { if let Some(current_transaction) = self.get_current_tx() { current_transaction.set_event(RFBEvent::MalformedMessage); + current_transaction.complete = true; } // Continue the flow but stop trying to map the protocol. self.state = parser::RFBGlobalState::Skip; @@ -623,6 +634,7 @@ impl RFBState { Err(_) => { if let Some(current_transaction) = self.get_current_tx() { current_transaction.set_event(RFBEvent::MalformedMessage); + current_transaction.complete = true; } // Continue the flow but stop trying to map the protocol. self.state = parser::RFBGlobalState::Skip; @@ -646,6 +658,7 @@ impl RFBState { SCLogDebug!("Invalid state for response: {}", self.state); if let Some(current_transaction) = self.get_current_tx() { current_transaction.set_event(RFBEvent::ConfusedState); + current_transaction.complete = true; } self.state = parser::RFBGlobalState::Skip; return AppLayerResult::ok(); From 74cbbd7ef463e01c9faedaee7bcd3272f70889d9 Mon Sep 17 00:00:00 2001 From: Sascha Steinbiss Date: Fri, 30 Jun 2023 10:16:45 +0200 Subject: [PATCH 6/6] rfb: also set unimplemented auth types (cherry picked from commit 1521b77edd04921a9b5f9419f84c62a812315e7a) --- rust/src/rfb/rfb.rs | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/rust/src/rfb/rfb.rs b/rust/src/rfb/rfb.rs index 926ea1361447..14219b04b34b 100644 --- a/rust/src/rfb/rfb.rs +++ b/rust/src/rfb/rfb.rs @@ -228,6 +228,15 @@ impl RFBState { current = rem; let chosen_security_type = request.security_type; + + if let Some(current_transaction) = self.get_current_tx() { + current_transaction.ts_security_type_selection = Some(request); + current_transaction.chosen_security_type = + Some(chosen_security_type as u32); + } else { + debug_validate_fail!("no transaction set at security type stage"); + } + match chosen_security_type { 2 => self.state = parser::RFBGlobalState::TCVncChallenge, 1 => self.state = parser::RFBGlobalState::TSClientInit, @@ -244,13 +253,6 @@ impl RFBState { return AppLayerResult::ok(); } } - - if let Some(current_transaction) = self.get_current_tx() { - current_transaction.ts_security_type_selection = Some(request); - current_transaction.chosen_security_type = Some(chosen_security_type as u32); - } else { - debug_validate_fail!("no transaction set at security type stage"); - } } Err(nom::Err::Incomplete(_)) => { return AppLayerResult::incomplete(