diff --git a/doc/userguide/configuration/suricata-yaml.rst b/doc/userguide/configuration/suricata-yaml.rst index b2b20d08f497..5b374bbe2d35 100644 --- a/doc/userguide/configuration/suricata-yaml.rst +++ b/doc/userguide/configuration/suricata-yaml.rst @@ -2015,6 +2015,19 @@ default is 1 MB. mqtt: max-msg-length: 1mb +RFB +~~~ + +RFB can have some strings whose maximum length according to the RFC is 4GiB. +In order to limit ram consumption and log output, a configuration parameter ``max-string-length`` is available. +This limit will also apply during detection. +An event ``rfb.too_long_string`` will be emitted when a string exceeds the limit. The default is 4 KiB. + +:: + + rfb: + max-string-length: 4 KiB + SMTP ~~~~~~ diff --git a/rules/rfb-events.rules b/rules/rfb-events.rules index 866a23851bb0..294d8b74bf9d 100644 --- a/rules/rfb-events.rules +++ b/rules/rfb-events.rules @@ -6,3 +6,4 @@ alert rfb any any -> any any (msg:"SURICATA RFB Malformed or unknown message"; a 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;) +alert rfb any any -> any any (msg:"SURICATA RFB too long string"; app-layer-event:rfb.too_long_string; classtype:protocol-command-decode; sid:2233004; rev:1;) diff --git a/rust/src/rfb/logger.rs b/rust/src/rfb/logger.rs index 9934aae05733..d677bef6947d 100644 --- a/rust/src/rfb/logger.rs +++ b/rust/src/rfb/logger.rs @@ -81,7 +81,10 @@ fn log_rfb(tx: &RFBTransaction, js: &mut JsonBuilder) -> Result<(), JsonError> { js.close()?; // Close authentication. if let Some(ref reason) = tx.tc_failure_reason { - js.set_string("server_security_failure_reason", &reason.reason_string)?; + js.set_string( + "server_security_failure_reason", + &String::from_utf8_lossy(&reason.reason_string), + )?; } // Client/Server init diff --git a/rust/src/rfb/parser.rs b/rust/src/rfb/parser.rs index 2faaaaaa6b8f..30a735e66c9b 100644 --- a/rust/src/rfb/parser.rs +++ b/rust/src/rfb/parser.rs @@ -93,7 +93,8 @@ pub struct SecurityResult { } pub struct FailureReason { - pub reason_string: String, + pub reason_string: Vec, + pub to_skip: u32, } pub struct VncAuth { @@ -123,6 +124,7 @@ pub struct ServerInit { pub pixel_format: PixelFormat, pub name_length: u32, pub name: Vec, + pub to_skip: u32, } pub fn parse_protocol_version(i: &[u8]) -> IResult<&[u8], ProtocolVersion> { @@ -177,13 +179,15 @@ pub fn parse_security_result(i: &[u8]) -> IResult<&[u8], SecurityResult> { Ok((i, SecurityResult { status })) } -pub fn parse_failure_reason(i: &[u8]) -> IResult<&[u8], FailureReason> { +pub fn parse_failure_reason(i: &[u8], max_len: u32) -> IResult<&[u8], FailureReason> { let (i, reason_length) = be_u32(i)?; - let (i, reason_string) = map_res(take(reason_length as usize), str::from_utf8).parse(i)?; + let to_skip = reason_length.saturating_sub(max_len); + let (i, reason_string) = take((reason_length - to_skip) as usize).parse(i)?; Ok(( i, FailureReason { - reason_string: reason_string.to_string(), + reason_string: reason_string.to_vec(), + to_skip, }, )) } @@ -220,18 +224,21 @@ pub fn parse_pixel_format(i: &[u8]) -> IResult<&[u8], PixelFormat> { Ok((i, format)) } -pub fn parse_server_init(i: &[u8]) -> IResult<&[u8], ServerInit> { +pub fn parse_server_init(i: &[u8], max_len: u32) -> IResult<&[u8], ServerInit> { let (i, width) = be_u16(i)?; let (i, height) = be_u16(i)?; let (i, pixel_format) = parse_pixel_format(i)?; let (i, name_length) = be_u32(i)?; - let (i, name) = take(name_length as usize)(i)?; + let to_skip = name_length.saturating_sub(max_len); + + let (i, name) = take((name_length - to_skip) as usize)(i)?; let init = ServerInit { width, height, pixel_format, name_length, name: name.to_vec(), + to_skip, }; Ok((i, init)) } @@ -273,7 +280,7 @@ mod tests { 0x2e, 0x6c, 0x6f, 0x63, 0x61, 0x6c, 0x64, 0x6f, 0x6d, 0x61, 0x69, 0x6e, ]; - let result = parse_server_init(&buf); + let result = parse_server_init(&buf, 4096); match result { Ok((remainder, message)) => { // Check the first message. diff --git a/rust/src/rfb/rfb.rs b/rust/src/rfb/rfb.rs index 1decbf3c6933..359a82c44bd4 100644 --- a/rust/src/rfb/rfb.rs +++ b/rust/src/rfb/rfb.rs @@ -21,6 +21,7 @@ use super::parser; use crate::applayer; use crate::applayer::*; +use crate::conf::{conf_get, get_memval}; use crate::core::{ sc_app_layer_parser_trigger_raw_stream_inspection, ALPROTO_UNKNOWN, IPPROTO_TCP, }; @@ -38,6 +39,9 @@ use suricata_sys::sys::{ }; pub(super) static mut ALPROTO_RFB: AppProto = ALPROTO_UNKNOWN; +// Maximum strings length in bytes. +// If some string exceeds this length, it will be truncated. +static mut MAX_STR_LEN: u32 = 4096; #[derive(FromPrimitive, Debug, AppLayerEvent)] pub enum RFBEvent { @@ -45,6 +49,7 @@ pub enum RFBEvent { UnknownSecurityResult, MalformedMessage, ConfusedState, + TooLongString, } #[derive(AppLayerFrameType)] @@ -116,6 +121,7 @@ pub struct RFBState { tx_id: u64, transactions: Vec, state: parser::RFBGlobalState, + to_skip_tc: u32, } impl State for RFBState { @@ -141,6 +147,7 @@ impl RFBState { tx_id: 0, transactions: Vec::new(), state: parser::RFBGlobalState::TCServerProtocolVersion, + to_skip_tc: 0, } } @@ -426,9 +433,16 @@ impl RFBState { if input.is_empty() { return AppLayerResult::ok(); } - - let mut current = input; let mut consumed = 0; + let mut current = input; + if self.to_skip_tc >= input.len() as u32 { + self.to_skip_tc -= input.len() as u32; + return AppLayerResult::ok(); + } else if self.to_skip_tc > 0 { + consumed += self.to_skip_tc as usize; + current = ¤t[self.to_skip_tc as usize..]; + self.to_skip_tc = 0; + } SCLogDebug!( "response_state {}, response_len {}", self.state, @@ -709,9 +723,15 @@ impl RFBState { } } parser::RFBGlobalState::TCFailureReason => { - match parser::parse_failure_reason(current) { - Ok((_rem, request)) => { + match parser::parse_failure_reason(current, unsafe { MAX_STR_LEN }) { + Ok((rem, request)) => { + if request.to_skip >= rem.len() as u32 { + self.to_skip_tc = request.to_skip - rem.len() as u32; + } if let Some(current_transaction) = self.get_current_tx() { + if request.to_skip > 0 { + current_transaction.set_event(RFBEvent::TooLongString); + } current_transaction.tc_failure_reason = Some(request); sc_app_layer_parser_trigger_raw_stream_inspection( flow, @@ -740,23 +760,34 @@ impl RFBState { } } parser::RFBGlobalState::TCServerInit => { - match parser::parse_server_init(current) { + match parser::parse_server_init(current, unsafe { MAX_STR_LEN }) { Ok((rem, request)) => { consumed += current.len() - rem.len(); let _pdu = Frame::new( flow, &stream_slice, current, - consumed as i64, + consumed as i64 + request.to_skip as i64, RFBFrameType::Pdu as u8, None, ); - current = rem; + if request.to_skip > 0 { + current = &rem[request.to_skip as usize..]; + consumed += request.to_skip as usize; + } else { + current = rem; + } + if request.to_skip >= rem.len() as u32 { + self.to_skip_tc = request.to_skip - rem.len() as u32; + } self.state = parser::RFBGlobalState::Skip; if let Some(current_transaction) = self.get_current_tx() { + if request.to_skip > 0 { + current_transaction.set_event(RFBEvent::TooLongString); + } current_transaction.tc_server_init = Some(request); sc_app_layer_parser_trigger_raw_stream_inspection( flow, @@ -950,6 +981,17 @@ pub unsafe extern "C" fn SCRfbRegisterParser() { { SCLogDebug!("Failed to register protocol detection pattern for direction TOCLIENT"); } + if let Some(val) = conf_get("app-layer.protocols.rfb.max-string-length") { + if let Ok(v) = get_memval(val) { + if v <= u32::MAX.into() { + MAX_STR_LEN = v as u32; + } else { + SCLogWarning!("rfb.max-string-length max is {}", u32::MAX); + } + } else { + SCLogWarning!("Invalid value for rfb.max-string-length: {}", val); + } + } } else { SCLogDebug!("Protocol detector and parser disabled for RFB."); } diff --git a/suricata.yaml.in b/suricata.yaml.in index c4a4983bd6ba..055668b1c523 100644 --- a/suricata.yaml.in +++ b/suricata.yaml.in @@ -947,6 +947,7 @@ app-layer: enabled: yes detection-ports: dp: 5900, 5901, 5902, 5903, 5904, 5905, 5906, 5907, 5908, 5909 + # max-string-length: 4 KiB mqtt: enabled: yes # max-msg-length: 1 MiB