From 1f8a5874fbc6816a7aeb59ba668ebd2bf7c206ed Mon Sep 17 00:00:00 2001 From: Sascha Steinbiss Date: Fri, 9 Jun 2023 11:13:35 +0200 Subject: [PATCH] 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: #5912. --- rules/Makefile.am | 1 + rules/rfb-events.rules | 10 +++ rust/src/rfb/detect.rs | 14 +-- rust/src/rfb/logger.rs | 37 +++++--- rust/src/rfb/mod.rs | 2 +- rust/src/rfb/parser.rs | 8 +- rust/src/rfb/rfb.rs | 199 ++++++++++++++++++++++++++++++++--------- 7 files changed, 202 insertions(+), 69 deletions(-) create mode 100644 rules/rfb-events.rules diff --git a/rules/Makefile.am b/rules/Makefile.am index 4c0c744c9b..d0ea6eda62 100644 --- a/rules/Makefile.am +++ b/rules/Makefile.am @@ -17,6 +17,7 @@ mqtt-events.rules \ nfs-events.rules \ ntp-events.rules \ quic-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 0000000000..08bc493f42 --- /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 7cfd7776b3..1cf0887c4f 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 392b825b19..62bb20966f 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::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")?; @@ -64,15 +64,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. @@ -92,15 +94,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()?; @@ -113,8 +127,9 @@ fn log_rfb(tx: &RFBTransaction, js: &mut JsonBuilder) -> Result<(), JsonError> { } #[no_mangle] -pub unsafe extern "C" fn rs_rfb_logger_log(tx: *mut std::os::raw::c_void, - js: &mut JsonBuilder) -> bool { +pub unsafe extern "C" fn rs_rfb_logger_log( + 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 4bde7935cb..68d37ec8a0 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 bdbc1b7e73..73958062d2 100644 --- a/rust/src/rfb/parser.rs +++ b/rust/src/rfb/parser.rs @@ -17,15 +17,15 @@ // Author: Frank Honza -use nom7::bytes::streaming::take; use nom7::bytes::streaming::tag; +use nom7::bytes::streaming::take; use nom7::combinator::map_res; use nom7::number::streaming::*; use nom7::*; use std::fmt; use std::str; -#[derive(Debug,PartialEq)] +#[derive(Debug, PartialEq)] pub enum RFBGlobalState { TCServerProtocolVersion, TCSupportedSecurityTypes, @@ -38,7 +38,7 @@ pub enum RFBGlobalState { TSVncResponse, TCSecurityResult, TSClientInit, - Message, + Skip, } impl fmt::Display for RFBGlobalState { @@ -55,7 +55,7 @@ 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"), } } } diff --git a/rust/src/rfb/rfb.rs b/rust/src/rfb/rfb.rs index 6ddd7a25f4..940417e831 100644 --- a/rust/src/rfb/rfb.rs +++ b/rust/src/rfb/rfb.rs @@ -29,6 +29,14 @@ use std::ffi::CString; static mut ALPROTO_RFB: AppProto = ALPROTO_UNKNOWN; +#[derive(FromPrimitive, Debug, AppLayerEvent)] +pub enum RFBEvent { + UnimplementedSecurityType, + UnknownSecurityResult, + MalformedMessage, + ConfusedState, +} + #[derive(AppLayerFrameType)] pub enum RFBFrameType { Pdu, @@ -87,6 +95,10 @@ impl RFBTransaction { tx_data: applayer::AppLayerTxData::new(), } } + + fn set_event(&mut self, event: RFBEvent) { + self.tx_data.set_event(event as u8); + } } pub struct RFBState { @@ -196,7 +208,9 @@ 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(Err::Incomplete(_)) => { @@ -206,6 +220,7 @@ impl RFBState { ); } Err(_) => { + // We even failed to parse the protocol version. return AppLayerResult::err(); } } @@ -228,7 +243,18 @@ 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() { @@ -236,7 +262,7 @@ impl RFBState { 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(Err::Incomplete(_)) => { @@ -246,7 +272,13 @@ impl RFBState { ); } 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(); } } } @@ -268,7 +300,7 @@ 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(Err::Incomplete(_)) => { @@ -278,7 +310,12 @@ impl RFBState { ); } 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::TSClientInit => match parser::parse_client_init(current) { @@ -299,7 +336,7 @@ 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(Err::Incomplete(_)) => { @@ -309,20 +346,34 @@ impl RFBState { ); } 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(); } } } @@ -368,7 +419,7 @@ 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(Err::Incomplete(_)) => { @@ -378,6 +429,7 @@ impl RFBState { ); } Err(_) => { + // We even failed to parse the protocol version. return AppLayerResult::err(); } } @@ -415,7 +467,7 @@ 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(Err::Incomplete(_)) => { @@ -425,7 +477,12 @@ impl RFBState { ); } 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(); } } } @@ -451,8 +508,20 @@ 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(); } } @@ -461,7 +530,7 @@ impl RFBState { current_transaction.chosen_security_type = Some(chosen_security_type); } else { - return AppLayerResult::err(); + debug_validate_fail!("no transaction set at security type stage"); } } Err(Err::Incomplete(_)) => { @@ -471,7 +540,12 @@ impl RFBState { ); } 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(); } } } @@ -493,7 +567,7 @@ 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(Err::Incomplete(_)) => { @@ -503,7 +577,12 @@ impl RFBState { ); } 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::TCSecurityResult => { @@ -526,12 +605,19 @@ 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(Err::Incomplete(_)) => { @@ -541,7 +627,12 @@ impl RFBState { ); } 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(); } } } @@ -551,9 +642,9 @@ 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(Err::Incomplete(_)) => { return AppLayerResult::incomplete( @@ -562,7 +653,12 @@ impl RFBState { ); } 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(); } } } @@ -580,14 +676,14 @@ impl RFBState { 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(Err::Incomplete(_)) => { @@ -597,17 +693,34 @@ impl RFBState { ); } 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(); } } } @@ -713,8 +826,8 @@ pub unsafe extern "C" fn rs_rfb_register_parser() { tx_comp_st_ts: 1, tx_comp_st_tc: 1, tx_get_progress: rs_rfb_tx_get_alstate_progress, - get_eventinfo: None, - get_eventinfo_byid: None, + get_eventinfo: Some(RFBEvent::get_event_info), + get_eventinfo_byid: Some(RFBEvent::get_event_info_by_id), localstorage_new: None, localstorage_free: None, get_tx_files: None, @@ -757,7 +870,7 @@ mod test { 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 r = state.parse_request( + let r = state.parse_response( std::ptr::null(), StreamSlice::from_slice(buf, STREAM_START, 0), ); @@ -862,7 +975,7 @@ mod test { std::ptr::null(), StreamSlice::from_slice(&buf[36..90], STREAM_START, 0), ); - ok_state = parser::RFBGlobalState::Message; + ok_state = parser::RFBGlobalState::Skip; assert_eq!(init_state.state, ok_state); } }