From f9515dc71dbadba3ceaf34af6921aa4dc5fe8d04 Mon Sep 17 00:00:00 2001 From: Philippe Antoine Date: Fri, 17 Jul 2026 09:11:40 +0200 Subject: [PATCH] rfb: limit strings length Ticket: 8731 Adds a configurable limit to string lengths to avoid to retain too much memory for too long, and avoid producing log events that are too big --- doc/userguide/configuration/suricata-yaml.rst | 13 +++++ rules/rfb-events.rules | 1 + rust/src/rfb/parser.rs | 17 ++++-- rust/src/rfb/rfb.rs | 57 ++++++++++++++++--- suricata.yaml.in | 1 + 5 files changed, 77 insertions(+), 12 deletions(-) diff --git a/doc/userguide/configuration/suricata-yaml.rst b/doc/userguide/configuration/suricata-yaml.rst index b2b20d08f4..5b374bbe2d 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 866a23851b..294d8b74bf 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/parser.rs b/rust/src/rfb/parser.rs index 2faaaaaa6b..94ec6be2c4 100644 --- a/rust/src/rfb/parser.rs +++ b/rust/src/rfb/parser.rs @@ -94,6 +94,7 @@ pub struct SecurityResult { pub struct FailureReason { pub reason_string: String, + 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) = map_res(take((reason_length - to_skip) as usize), str::from_utf8).parse(i)?; Ok(( i, FailureReason { reason_string: reason_string.to_string(), + 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 1decbf3c69..931d2feace 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,35 @@ 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 >= rem.len() as u32 { + current = &rem[rem.len()..]; + consumed += rem.len(); + self.to_skip_tc = request.to_skip - rem.len() as u32; + } else if request.to_skip > 0 { + current = &rem[request.to_skip as usize..]; + consumed += request.to_skip as usize; + } else { + current = rem; + } 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 +982,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 c4a4983bd6..055668b1c5 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