From 5aaf50760f546e9047da508f725f43a7ad9b8a35 Mon Sep 17 00:00:00 2001 From: Eric Leblond Date: Mon, 23 Jan 2023 20:01:05 +0100 Subject: [PATCH] app-layer: add flag to skip detection on TX Stamus team did discover a problem were a signature can shadow other signatures. For example, on a PCAP only containing Kerberos protocol and where the following signature is matching: alert krb5 $HOME_NET any -> any any (msg:"krb match"; krb5_cname; content:"marlo"; sid:3; rev:1;) If we add the following signature to the list of signature alert ssh $HOME_NET any -> any any (msg:"rr"; content:"rr"; flow:established,to_server; sid:4; rev:2;) Then the Kerberos signature is not matching anymore. To understand this case, we need some information: - The krb5_cname is a to_client keyword - The signal on ssh is to_server - Kerberos has unidirectional transaction - kerberos application state progress is a function always returning 1 As the two signatures are in opposite side, they end up in separate sig group head. Another fact is that, in the PCAP, the to_server side of the session is sent first to the detection. It thus hit the sig group head of the SSH signature. When Suricata runs detection in this direction the Kerberos application layer send the transaction as it is existing and because the alstate progress function just return 1 if the transaction exists. So Suricata runs DetectRunTx() and stops when it sees that sgh->tx_engines is NULL. But the transaction is consumed by the engine as it has been evaluated in one direction and the kerberos transaction are unidirectional so there is no need to continue looking at it. This results in no matching of the kerberos signature as the match should occur in the evaluation of the other side but the transaction with the data is already seen has been handled. This problem was discovered on this Kerberos signature but all the application layer with unidirectional transaction are impacted. This patch introduces a flag that can be used by application layer to signal that the TX should not be inspected. By using this flag on the directional detect_flags_[ts|tc] the application layer can prevent the TX to be consumed in the wrong direction. Application layers with unidirectional TX will be updated in separate commits to set the flag on the direction opposite to the one they are. Ticket: #5799 --- rust/src/applayer.rs | 12 +++++++++++- src/app-layer-parser.h | 5 +++-- src/detect.c | 16 ++++++++++++++++ 3 files changed, 30 insertions(+), 3 deletions(-) diff --git a/rust/src/applayer.rs b/rust/src/applayer.rs index d7dbfe6ab90e..fd189c68656f 100644 --- a/rust/src/applayer.rs +++ b/rust/src/applayer.rs @@ -18,7 +18,7 @@ //! Parser registration functions and common interface use std; -use crate::core::{self,DetectEngineState,Flow,AppLayerEventType,AppProto}; +use crate::core::{self,DetectEngineState,Flow,AppLayerEventType,AppProto,Direction}; use crate::filecontainer::FileContainer; use crate::applayer; use std::os::raw::{c_void,c_char,c_int}; @@ -180,6 +180,14 @@ impl AppLayerTxData { self.file_flags |= state_flags; } } + + pub fn set_inspect_direction(&mut self, direction: Direction) { + if direction == Direction::ToClient { + self.detect_flags_ts |= APP_LAYER_TX_SKIP_INSPECT_FLAG; + } else { + self.detect_flags_tc |= APP_LAYER_TX_SKIP_INSPECT_FLAG; + } + } } #[macro_export] @@ -474,6 +482,8 @@ pub const APP_LAYER_PARSER_TRUNC_TC : u16 = BIT_U16!(8); pub const APP_LAYER_PARSER_OPT_ACCEPT_GAPS: u32 = BIT_U32!(0); pub const APP_LAYER_PARSER_OPT_UNIDIR_TXS: u32 = BIT_U32!(1); +pub const APP_LAYER_TX_SKIP_INSPECT_FLAG: u64 = BIT_U64!(62); + pub type AppLayerGetTxIteratorFn = unsafe extern "C" fn (ipproto: u8, alproto: AppProto, alstate: *mut c_void, diff --git a/src/app-layer-parser.h b/src/app-layer-parser.h index ead069184c91..4836ab06be25 100644 --- a/src/app-layer-parser.h +++ b/src/app-layer-parser.h @@ -66,7 +66,6 @@ #define APP_LAYER_TX_RESERVED12_FLAG BIT_U64(59) #define APP_LAYER_TX_RESERVED13_FLAG BIT_U64(60) #define APP_LAYER_TX_RESERVED14_FLAG BIT_U64(61) -#define APP_LAYER_TX_RESERVED15_FLAG BIT_U64(62) #define APP_LAYER_TX_RESERVED_FLAGS \ (APP_LAYER_TX_RESERVED1_FLAG | APP_LAYER_TX_RESERVED2_FLAG | APP_LAYER_TX_RESERVED3_FLAG | \ @@ -75,8 +74,10 @@ APP_LAYER_TX_RESERVED8_FLAG | APP_LAYER_TX_RESERVED9_FLAG | \ APP_LAYER_TX_RESERVED10_FLAG | APP_LAYER_TX_RESERVED11_FLAG | \ APP_LAYER_TX_RESERVED12_FLAG | APP_LAYER_TX_RESERVED13_FLAG | \ - APP_LAYER_TX_RESERVED14_FLAG | APP_LAYER_TX_RESERVED15_FLAG) + APP_LAYER_TX_RESERVED14_FLAG) +/** should inspection be skipped in that direction */ +#define APP_LAYER_TX_SKIP_INSPECT_FLAG BIT_U64(62) /** is tx fully inspected? */ #define APP_LAYER_TX_INSPECTED_FLAG BIT_U64(63) /** other 63 bits are for tracking which prefilter engine is already diff --git a/src/detect.c b/src/detect.c index 2dbbdc0c4574..65ca4ebfbbef 100644 --- a/src/detect.c +++ b/src/detect.c @@ -1256,6 +1256,22 @@ static DetectTransaction GetDetectTx(const uint8_t ipproto, const AppProto alpro DetectTransaction no_tx = { NULL, 0, NULL, NULL, 0, 0, 0, 0, 0, }; return no_tx; } + if (detect_flags & APP_LAYER_TX_SKIP_INSPECT_FLAG) { + SCLogDebug("%" PRIu64 " tx should not be inspected in direction %s. Flags %016" PRIx64, + tx_id, flow_flags & STREAM_TOSERVER ? "toserver" : "toclient", detect_flags); + DetectTransaction no_tx = { + NULL, + 0, + NULL, + NULL, + 0, + 0, + 0, + 0, + 0, + }; + return no_tx; + } const int tx_progress = AppLayerParserGetStateProgress(ipproto, alproto, tx_ptr, flow_flags); const int dir_int = (flow_flags & STREAM_TOSERVER) ? 0 : 1;