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.
OISF · Jun 27, 2023 · 1f8a587 · 1f8a587
1 parent 836fff3
commit 1f8a587
Show file tree

Hide file tree

Showing 7 changed files with 202 additions and 69 deletions.
diff --git 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
@@ -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
@@ -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,13 +53,12 @@ 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;
         return 1;
     }
 
     return 0;
-}
+}
diff --git a/rust/src/rfb/logger.rs b/rust/src/rfb/logger.rs
@@ -17,10 +17,10 @@
 
 // Author: Frank Honza <frank.honza@dcso.de>
 
-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
@@ -20,4 +20,4 @@
 pub mod detect;
 pub mod logger;
 pub mod parser;
-pub mod rfb;
+pub mod rfb;
diff --git a/rust/src/rfb/parser.rs b/rust/src/rfb/parser.rs
@@ -17,15 +17,15 @@
 
 // Author: Frank Honza <frank.honza@dcso.de>
 
-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"),
         }
     }
 }