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

@@ -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
 ~~~~~~
 

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

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<u8>,
+    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.

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<RFBTransaction>,
     state: parser::RFBGlobalState,
+    to_skip_tc: u32,
 }
 
 impl State<RFBTransaction> 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 = &current[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) {
+                    match parser::parse_failure_reason(current, unsafe { MAX_STR_LEN }) {
-                        Ok((_rem, request)) => {
+                        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.");
     }

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