From ae072d5c07e0f5e02a8583127c7b3edd97f93d54 Mon Sep 17 00:00:00 2001 From: Victor Julien Date: Tue, 3 Dec 2024 11:36:27 +0100 Subject: [PATCH 1/5] flow/manager: fix multi instance row tracking In multi instance flow manager setups, each flow manager gets a slice of the hash table to manage. Due to a logic error in the chunked scanning of the hash slice, instances beyond the first would always rescan the same (first) subslice of their slice. The `pos` variable that is used to keep the state of what the starting position for the next scan was supposed to be, was treated as if it held a relative value. Relative to the bounds of the slice. It was however, holding an absolute position. This meant that when doing it's bounds check it was always considered out of bounds. This would reset the sub- slice to be scanned to the first part of the instances slice. This patch addresses the issue by correctly handling the fact that the value is absolute. Bug: #7365. Fixes: e9d2417e0ff3 ("flow/manager: adaptive hash eviction timing") --- src/flow-manager.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/flow-manager.c b/src/flow-manager.c index 9da986b22df6..8fb205f5e872 100644 --- a/src/flow-manager.c +++ b/src/flow-manager.c @@ -500,7 +500,7 @@ static uint32_t FlowTimeoutHash(FlowManagerTimeoutThread *td, SCTime_t ts, const * \param hash_max upper bound of the row slice * \param counters Flow timeout counters to be passed * \param rows number of rows for this worker unit - * \param pos position of the beginning of row slice in the hash table + * \param pos absolute position of the beginning of row slice in the hash table * * \retval number of successfully timed out flows */ @@ -514,7 +514,7 @@ static uint32_t FlowTimeoutHashInChunks(FlowManagerTimeoutThread *td, SCTime_t t uint32_t rows_left = rows; again: - start = hash_min + (*pos); + start = (*pos); if (start >= hash_max) { start = hash_min; } @@ -795,7 +795,7 @@ static TmEcode FlowManager(ThreadVars *th_v, void *thread_data) uint32_t emerg_over_cnt = 0; uint64_t next_run_ms = 0; - uint32_t pos = 0; + uint32_t pos = ftd->min; uint32_t rows_sec = 0; uint32_t rows_per_wu = 0; uint64_t sleep_per_wu = 0; From 7fd707a876868b58c7fbff2451cb176f7111a951 Mon Sep 17 00:00:00 2001 From: Victor Julien Date: Wed, 4 Dec 2024 10:51:06 +0100 Subject: [PATCH 2/5] flow/manager: add chunk debug output --- src/flow-manager.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/flow-manager.c b/src/flow-manager.c index 8fb205f5e872..4c6f165888d7 100644 --- a/src/flow-manager.c +++ b/src/flow-manager.c @@ -501,12 +501,13 @@ static uint32_t FlowTimeoutHash(FlowManagerTimeoutThread *td, SCTime_t ts, const * \param counters Flow timeout counters to be passed * \param rows number of rows for this worker unit * \param pos absolute position of the beginning of row slice in the hash table + * \param instance instance id of this FM * * \retval number of successfully timed out flows */ static uint32_t FlowTimeoutHashInChunks(FlowManagerTimeoutThread *td, SCTime_t ts, const uint32_t hash_min, const uint32_t hash_max, FlowTimeoutCounters *counters, - const uint32_t rows, uint32_t *pos) + const uint32_t rows, uint32_t *pos, const uint32_t instance) { uint32_t start = 0; uint32_t end = 0; @@ -525,6 +526,9 @@ static uint32_t FlowTimeoutHashInChunks(FlowManagerTimeoutThread *td, SCTime_t t *pos = (end == hash_max) ? hash_min : end; rows_left = rows_left - (end - start); + SCLogDebug("instance %u: %u:%u (hash_min %u, hash_max %u *pos %u)", instance, start, end, + hash_min, hash_max, *pos); + cnt += FlowTimeoutHash(td, ts, start, end, counters); if (rows_left) { goto again; @@ -859,8 +863,8 @@ static TmEcode FlowManager(ThreadVars *th_v, void *thread_data) rows_per_wu); const uint32_t ppos = pos; - FlowTimeoutHashInChunks( - &ftd->timeout, ts, ftd->min, ftd->max, &counters, rows_per_wu, &pos); + FlowTimeoutHashInChunks(&ftd->timeout, ts, ftd->min, ftd->max, &counters, + rows_per_wu, &pos, ftd->instance); if (ppos > pos) { StatsIncr(th_v, ftd->cnt.flow_mgr_full_pass); } From 261c15d0e1b94cdfdd2515517db1feaf7f9ec634 Mon Sep 17 00:00:00 2001 From: Philippe Antoine Date: Tue, 29 Oct 2024 22:26:37 +0100 Subject: [PATCH 3/5] fuzz: better init for protocol detection Ticket: 7435 --- src/tests/fuzz/fuzz_applayerprotodetectgetproto.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/tests/fuzz/fuzz_applayerprotodetectgetproto.c b/src/tests/fuzz/fuzz_applayerprotodetectgetproto.c index e30f729bc28f..6dcaf1793461 100644 --- a/src/tests/fuzz/fuzz_applayerprotodetectgetproto.c +++ b/src/tests/fuzz/fuzz_applayerprotodetectgetproto.c @@ -9,7 +9,7 @@ #include "suricata.h" #include "app-layer-detect-proto.h" #include "flow-util.h" -#include "app-layer-parser.h" +#include "app-layer.h" #include "util-unittest-helper.h" #include "conf-yaml-loader.h" @@ -43,9 +43,7 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) MpmTableSetup(); SpmTableSetup(); EngineModeSetIDS(); - AppLayerProtoDetectSetup(); - AppLayerParserSetup(); - AppLayerParserRegisterProtocolParsers(); + AppLayerSetup(); alpd_tctx = AppLayerProtoDetectGetCtxThread(); SC_ATOMIC_SET(engine_stage, SURICATA_RUNTIME); } From e5b98be41f191a870e03c15616fabd6e01c2450c Mon Sep 17 00:00:00 2001 From: Philippe Antoine Date: Tue, 29 Oct 2024 22:28:18 +0100 Subject: [PATCH 4/5] fuzz: simplify target for protocol detection As too many cases are found when splitting tcp payload --- .../fuzz/fuzz_applayerprotodetectgetproto.c | 27 +------------------ 1 file changed, 1 insertion(+), 26 deletions(-) diff --git a/src/tests/fuzz/fuzz_applayerprotodetectgetproto.c b/src/tests/fuzz/fuzz_applayerprotodetectgetproto.c index 6dcaf1793461..4473de00941c 100644 --- a/src/tests/fuzz/fuzz_applayerprotodetectgetproto.c +++ b/src/tests/fuzz/fuzz_applayerprotodetectgetproto.c @@ -30,8 +30,6 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) Flow *f; TcpSession ssn; bool reverse; - AppProto alproto; - AppProto alproto2; if (alpd_tctx == NULL) { //global init @@ -66,31 +64,8 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) if (data[0] & STREAM_TOSERVER) { flags = STREAM_TOSERVER; } - alproto = AppLayerProtoDetectGetProto( + AppLayerProtoDetectGetProto( alpd_tctx, f, data + HEADER_LEN, size - HEADER_LEN, f->proto, flags, &reverse); - if (alproto != ALPROTO_UNKNOWN && alproto != ALPROTO_FAILED && f->proto == IPPROTO_TCP) { - /* If we find a valid protocol at the start of a stream : - * check that with smaller input - * we find the same protocol or ALPROTO_UNKNOWN. - * Otherwise, we have evasion with TCP splitting - */ - for (size_t i = 0; i < size-HEADER_LEN && i < PROTO_DETECT_MAX_LEN; i++) { - // reset detection at each try cf probing_parser_toserver_alproto_masks - AppLayerProtoDetectReset(f); - alproto2 = AppLayerProtoDetectGetProto( - alpd_tctx, f, data + HEADER_LEN, i, f->proto, flags, &reverse); - if (alproto2 != ALPROTO_UNKNOWN && alproto2 != alproto) { - printf("Failed with input length %" PRIuMAX " versus %" PRIuMAX - ", found %s instead of %s\n", - (uintmax_t)i, (uintmax_t)size - HEADER_LEN, AppProtoToString(alproto2), - AppProtoToString(alproto)); - printf("Assertion failure: %s-%s\n", AppProtoToString(alproto2), - AppProtoToString(alproto)); - fflush(stdout); - abort(); - } - } - } FlowFree(f); return 0; From 38d7900fa9b8ce1db4cd296412fcfec094ba4794 Mon Sep 17 00:00:00 2001 From: Philippe Antoine Date: Tue, 29 Oct 2024 22:29:06 +0100 Subject: [PATCH 5/5] sip: remove UPDATE method for detection As it is also used for HTTP/1 Remove it only for TCP and keep it for UDP. Ticket: 7436 --- rust/src/sip/sip.rs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/rust/src/sip/sip.rs b/rust/src/sip/sip.rs index 1a73d4e46a66..5f52e0c8db07 100755 --- a/rust/src/sip/sip.rs +++ b/rust/src/sip/sip.rs @@ -496,7 +496,6 @@ fn register_pattern_probe(proto: u8) -> i8 { "ACK\0", "BYE\0", "CANCEL\0", - "UPDATE\0", "REFER\0", "PRACK\0", "SUBSCRIBE\0", @@ -526,6 +525,16 @@ fn register_pattern_probe(proto: u8) -> i8 { 0, core::Direction::ToClient as u8, ); + if proto == core::IPPROTO_UDP { + r |= AppLayerProtoDetectPMRegisterPatternCS( + proto, + ALPROTO_SIP, + "UPDATE\0".as_ptr() as *const std::os::raw::c_char, + "UPDATE".len() as u16, + 0, + core::Direction::ToServer as u8, + ); + } } if r == 0 {