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); } }