From c796b6e9d1d2b1c0d64095c0c14faaff24423eab Mon Sep 17 00:00:00 2001 From: Shivani Bhardwaj Date: Tue, 19 Aug 2025 16:39:15 +0530 Subject: [PATCH 01/12] rust: add new dependencies petgraph and itertools --- rust/Cargo.lock.in | 56 +++++++++++++++++++++++++++++++++++++++++++--- rust/Cargo.toml.in | 3 +++ 2 files changed, 56 insertions(+), 3 deletions(-) diff --git a/rust/Cargo.lock.in b/rust/Cargo.lock.in index 9c80e26f9b..7fc8c5076f 100644 --- a/rust/Cargo.lock.in +++ b/rust/Cargo.lock.in @@ -414,6 +414,12 @@ dependencies = [ "syn 2.0.117", ] +[[package]] +name = "either" +version = "1.16.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "91622ff5e7162018101f2fea40d6ebf4a78bbe5a49736a2020649edf9693679e" + [[package]] name = "endian-type" version = "0.1.2" @@ -489,6 +495,12 @@ version = "0.1.9" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "5baebc0774151f905a1a2cc41989300b1e6fbb29aff0ceffa1064fdd3088d582" +[[package]] +name = "fixedbitset" +version = "0.5.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1d674e81391d1e1ab681a28d99df07927c6d4aa5b027d7da16ba32d1d21ecd99" + [[package]] name = "flate2" version = "1.0.35" @@ -500,6 +512,12 @@ dependencies = [ "miniz_oxide", ] +[[package]] +name = "foldhash" +version = "0.1.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d9c4f5dac5e15c24eb999c26181a6ca40b39fe946cbe4c263c7209467bc83af2" + [[package]] name = "foldhash" version = "0.2.0" @@ -637,6 +655,15 @@ dependencies = [ "polyval", ] +[[package]] +name = "hashbrown" +version = "0.15.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9229cfe53dfd69f0609a49f65461bd93001ea1ef889cd5529dd176593f5338a1" +dependencies = [ + "foldhash 0.1.5", +] + [[package]] name = "hashbrown" version = "0.16.1" @@ -645,7 +672,7 @@ checksum = "841d1cc9bed7f9236f321df977030373f4a4163ae1a7dbfe1a51a2c1a51d9100" dependencies = [ "allocator-api2", "equivalent", - "foldhash", + "foldhash 0.2.0", ] [[package]] @@ -700,7 +727,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "4b0f83760fb341a774ed326568e19f5a863af4a952def8c39f9ab92fd95b88e5" dependencies = [ "equivalent", - "hashbrown", + "hashbrown 0.16.1", ] [[package]] @@ -722,6 +749,15 @@ dependencies = [ "rusticata-macros", ] +[[package]] +name = "itertools" +version = "0.14.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2b192c782037fadd9cfa75548310488aabdbf3d2da73885b31bd0abd03351285" +dependencies = [ + "either", +] + [[package]] name = "itoa" version = "1.0.18" @@ -792,7 +828,7 @@ version = "0.16.4" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7f66e8d5d03f609abc3a39e6f08e4164ebf1447a732906d39eb9b99b7919ef39" dependencies = [ - "hashbrown", + "hashbrown 0.16.1", ] [[package]] @@ -1071,6 +1107,18 @@ version = "0.3.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "c08d65885ee38876c4f86fa503fb49d7b507c2b62552df7c70b2fce627e06381" +[[package]] +name = "petgraph" +version = "0.8.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8701b58ea97060d5e5b155d383a69952a60943f0e6dfe30b04c287beb0b27455" +dependencies = [ + "fixedbitset", + "hashbrown 0.15.5", + "indexmap", + "serde", +] + [[package]] name = "phf" version = "0.10.1" @@ -1546,6 +1594,7 @@ dependencies = [ "hkdf", "humantime", "ipsec-parser", + "itertools", "kerberos-parser", "lazy_static", "ldap-parser", @@ -1560,6 +1609,7 @@ dependencies = [ "num", "num-derive", "num-traits 0.2.19", + "petgraph", "psl", "regex", "sawp", diff --git a/rust/Cargo.toml.in b/rust/Cargo.toml.in index 95002c83d4..433db9ee56 100644 --- a/rust/Cargo.toml.in +++ b/rust/Cargo.toml.in @@ -60,6 +60,9 @@ lru = "~0.16.3" der-parser = { version = "~9.0.0", default-features = false } kerberos-parser = { version = "~0.8.0", default-features = false } +petgraph = "~0.8.2" +itertools = "~0.14.0" + sawp-modbus = "~0.13.1" sawp-pop3 = "~0.13.1" sawp = "~0.13.1" From 659e7f924a3e53a6f5720fd653f6831aa23fb2fb Mon Sep 17 00:00:00 2001 From: Shivani Bhardwaj Date: Mon, 20 Apr 2026 20:26:14 +0530 Subject: [PATCH 02/12] rust: add flowbit dependency resolver This solution makes use of the petgraph crate to create a directed stable graph of signatures. The algorithm is as follows: - A stable Directed Graph is created - A Flowbit Analyzer array is created which stores the sids per flowbit per command - for each entry in the flowbit analyzer "READ" array of type isset - set array is walked => a directed edge* from the set sid to isset sid with the weight of SET command is added. - for each entry in the flowbit analyzer "READ" array of type isnotset - unset array is walked => a directed edge from the set sid to isnotset sid with the weight of SET command is added. - If there was no error (cycle), this means that the current graph is a DAG (Directed Acyclic Graph) - Perform a topological sort of the DAG 1. If a cycle is formed by varying edge weights, it's an invalid cycle. This would be formed by usage of different "WRITE" commands in different signatures e.g. sid: 1 => set, A; isset, B; # a directed edge of weight 0 is created from 1 to 2 for flowbit A. sid: 2 => isset, A; unset, B; # a directed edge of weight 2 is created from 2 to 1 for flowbit B. 2. If the cycle is formed by the same edge weights, it is considered unsatisfiable at runtime. Bug 7771 Bug 7638 Bug 1399 --- rust/src/utils/flowbits_resolver.rs | 286 ++++++++++++++++++++++++++++ rust/src/utils/mod.rs | 3 +- 2 files changed, 288 insertions(+), 1 deletion(-) create mode 100644 rust/src/utils/flowbits_resolver.rs diff --git a/rust/src/utils/flowbits_resolver.rs b/rust/src/utils/flowbits_resolver.rs new file mode 100644 index 0000000000..0fd41e9272 --- /dev/null +++ b/rust/src/utils/flowbits_resolver.rs @@ -0,0 +1,286 @@ +/* Copyright (C) 2026 Open Information Security Foundation +* + * You can copy, redistribute or modify this Program under the terms of + * the GNU General Public License version 2 as published by the Free + * Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * version 2 along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA + * 02110-1301, USA. + */ + +// Author: Shivani Bhardwaj + +use petgraph::algo::{is_cyclic_directed, tarjan_scc, toposort}; +use petgraph::graph::{GraphError, NodeIndex}; +use petgraph::stable_graph::StableDiGraph; +use petgraph::visit::EdgeRef; +use petgraph::Direction; +use std::collections::{HashMap, HashSet}; +use std::os::raw::c_void; + +/// Special Graph Node storing flowbit or signature +#[derive(Debug, Copy, Clone)] +struct SCGNode { + iid: u32, + sid: u32, + nidx: NodeIndex, /* Graph's internal node index */ +} + +/// Edge object to determine a genuine cycle breaker +#[derive(Debug, Copy, Clone)] +struct SCGEdge { + cmd: u8, + fb_id: u32, +} + +#[derive(Debug)] +struct FlowbitSidStore { + graph: StableDiGraph, + iid_map: HashMap, +} + +/// Function to create an empty directed Graph +#[no_mangle] +pub unsafe extern "C" fn SCCreateDirectedGraph() -> *mut c_void { + // StableDiGraph is the ideal choice here for there is removal of + // nodes in the line later and this type of graph guarantees to + // not re-use any existing node indices + let fb_sid_store = FlowbitSidStore { + graph: StableDiGraph::new(), + iid_map: HashMap::new(), + }; + + /* Make an opaque pointer for C as nothing is changed there */ + return Box::into_raw(Box::new(fb_sid_store)) as *mut c_void; +} + +/// Drop the directed Graph. Called from C. +#[no_mangle] +pub unsafe extern "C" fn SCFreeDirectedGraph(store: *mut c_void) { + let _ = Box::from_raw(store as *mut FlowbitSidStore); +} + +#[no_mangle] +pub unsafe extern "C" fn SCGetOrCreateNodeGraph(graph: *mut c_void, iid: u32, sid: u32) -> i64 { + let g = &mut *(graph as *mut FlowbitSidStore); + + let Some(node_idx) = get_or_create_node(g, iid, sid) else { + SCLogError!("Error adding node; Graph is at full capacity"); + return -2; + }; + + node_idx.index() as i64 +} + +/// Function to get or create a node and add an appropriate directed +/// edge based on its type +#[no_mangle] +pub unsafe extern "C" fn SCCreateNodeEdgeDirectedGraph( + fss_void: *mut c_void, from: u32, to: u32, cmd: u8, fb_id: u32, +) -> i64 { + let fss = &mut *(fss_void as *mut FlowbitSidStore); + + let from_idx = NodeIndex::from(from); + let to_idx = NodeIndex::from(to); + + match fss + .graph + .try_update_edge(from_idx, to_idx, SCGEdge { cmd, fb_id }) + { + /* edge from a flowbit setter to a flowbit reader */ + Ok(_) => { + SCLogDebug!( + "Created an edge from {:?} -> {:?} for flowbit {:?} with command: {:?}", + from, + to, + fb_id, + cmd + ); + } + Err(GraphError::EdgeIxLimit) => { + SCLogError!("Error adding edge; Graph is at full capacity"); + return -2; + } + Err(GraphError::NodeOutBounds) => { + SCLogError!("Error adding edge; node does not exist"); + return -2; + } + Err(_) => { + SCLogError!("Error adding edge to the Graph"); + return -2; + } + } + + return 0; +} + +fn check_cycle_update_graph(graph: &mut StableDiGraph) -> i8 { + let mut MAX_STACK_SIZE: usize = 100; + + for i in 0..MAX_STACK_SIZE { + /* Check graph for any cycles */ + if !is_cyclic_directed(&*graph) { + SCLogDebug!("no cycles after {} tries", i); + return 0; + } + + SCLogDebug!("Found a cycle in i {}. Checking if it's valid..", i); + + if !try_resolve_one_cycle(graph) { + /* If we can't resolve any cycle, we're stuck */ + SCLogError!("Unable to resolve cycles after {} tries", i); + return -1; + } + } + + SCLogError!( + "Maximum tries ({}) reached while trying to resolve cycles", + MAX_STACK_SIZE + ); + return -1; +} + +fn try_resolve_one_cycle(graph: &mut StableDiGraph) -> bool { + let sccs = tarjan_scc(&*graph); + let mut edge_map: HashMap = HashMap::new(); + + /* Find the first multi-node SCC */ + for scc in sccs { + if scc.len() == 1 { + let self_loop_edges: Vec<_> = graph + .edges(scc[0]) + .filter(|edge| edge.target() == scc[0]) + .map(|edge| edge.id()) + .collect(); + + if let Some(edge) = self_loop_edges.into_iter().next() { + graph.remove_edge(edge); + return true; + } + continue; + } + + SCLogDebug!("Current scc: {:?}", scc); + let scc_set: HashSet<_> = scc.iter().copied().collect(); + let edge_indices: Vec<_> = graph.edge_indices().collect(); + + for edge_idx in edge_indices { + let Some((_src, tgt)) = graph.edge_endpoints(edge_idx) else { + continue; + }; + if !(scc_set.contains(&_src) && scc_set.contains(&tgt)) { + continue; + } + let SCGEdge { cmd, fb_id } = graph[edge_idx]; + let same_bit_setters = graph + .edges_directed(tgt, Direction::Incoming) + .filter(|e| e.weight().fb_id == fb_id) + .count(); + if same_bit_setters > 1 { + graph.remove_edge(edge_idx); + return true; + } + /* store cycle edges to inspect their commands (weights) later */ + edge_map.insert(edge_idx, cmd); + } + /* Find if the cycle causing edges are made up of differing weights */ + let distinct_weights = edge_map.values().copied().collect::>().len(); + debug_validate_bug_on!(distinct_weights == 0); + if distinct_weights > 1 { + /* Find and remove the edge with highest weight (lowest priority) */ + if let Some((cur_e, _)) = edge_map.iter().max_by_key(|(_, &weight)| weight) { + graph.remove_edge(*cur_e); + return true; + } + } else { + /* Valid cycle with same weights -- can't resolve */ + let sids: Vec<_> = scc_set.into_iter().map(|a| graph[a].sid).collect(); + SCLogError!( + "Cyclic dependency found between flowbits from signatures: {:?}", + sids + ); + return false; + } + break; + } + + /* couldn't resolve */ + false +} + +/// Wrapper function to resolve flowbit dependencies +#[no_mangle] +pub unsafe extern "C" fn SCResolveFlowbitDependencies( + fss_void: *mut c_void, sorted_iid_list: *mut u32, sorted_iid_list_len: u32, +) -> i8 { + SCLogDebug!("Attempting to resolve flowbit dependencies"); + let fss = &mut *(fss_void as *mut FlowbitSidStore); + + let r = check_cycle_update_graph(&mut fss.graph); + if r == -1 { + SCLogError!("Couldn't do anything to fix the graph. Retreating.."); + return -1; + } + + debug_validate_bug_on!(fss.graph.node_count() == 0); + + let sorted_iid_list = + std::slice::from_raw_parts_mut(&mut *sorted_iid_list, sorted_iid_list_len as usize); + + /* No need for all the extra work if there's just one node */ + if fss.graph.node_count() == 1 { + debug_validate_bug_on!(sorted_iid_list_len != 1); + sorted_iid_list[0] = fss.graph[NodeIndex::from(0)].iid; + return 0; + } + + /* At this point, it must be a DAG, so perform a topological sort to find + * out the correct order of signatures */ + return toposort_dag(&fss.graph, sorted_iid_list); +} + +fn get_or_create_node(fss: &mut FlowbitSidStore, iid: u32, sid: u32) -> Option { + if let Some(&nidx) = fss.iid_map.get(&iid) { + return Some(nidx); /* O(1) */ + } + let nd = SCGNode { + iid, + sid, + nidx: NodeIndex::from(u32::MAX), + }; + if let Ok(idx) = fss.graph.try_add_node(nd) { + /* O(1) */ + fss.graph[idx].nidx = idx; + fss.iid_map.insert(iid, idx); + SCLogDebug!("Created node: {:?}", fss.graph[idx]); + return Some(idx); + } + + None +} + +/// Produce a topological ordering of the DAG +fn toposort_dag(graph: &StableDiGraph, sorted_iid_list: &mut [u32]) -> i8 { + match toposort(graph, None) { + Ok(order) => { + debug_validate_bug_on!(order.len() != sorted_iid_list.len()); + for (i, idx) in order.into_iter().enumerate() { + SCLogDebug!("[{:?}]: {:?}", i, graph[idx]); + sorted_iid_list[i] = graph[idx].iid; + } + 0 + } + Err(_) => { + /* Unreachable in practice: cycles are resolved before this point */ + SCLogError!("Graph still contains a cycle; cannot produce an order"); + -1 + } + } +} diff --git a/rust/src/utils/mod.rs b/rust/src/utils/mod.rs index 542aa7791f..36aea88ec4 100644 --- a/rust/src/utils/mod.rs +++ b/rust/src/utils/mod.rs @@ -1,4 +1,4 @@ -/* Copyright (C) 2024 Open Information Security Foundation +/* Copyright (C) 2024-2026 Open Information Security Foundation * * You can copy, redistribute or modify this Program under the terms of * the GNU General Public License version 2 as published by the Free @@ -17,4 +17,5 @@ pub mod base64; pub mod datalink; +pub mod flowbits_resolver; pub mod ipaddr; From e48c309fad73d89a21e344727e95253547fb7cd7 Mon Sep 17 00:00:00 2001 From: Shivani Bhardwaj Date: Mon, 20 Apr 2026 20:35:18 +0530 Subject: [PATCH 03/12] detect/sigorder: resolve flowbit dependencies before adding them to the Detection Context's signature list. The de_ctx->sig_list serves as a sorted signature list that is later passed on to the grouping fns. If no property of higher value changes the order of the signatures, the order coming from de_ctx->sig_list is final. Add the appropriate calls to resolve flowbit dependencies before adding them to the sig_list. This is especially important for flowbits with complex ordering involved. Bug 7771 Bug 7638 Bug 1399 --- rust/src/utils/flowbits_resolver.rs | 1 - src/detect-engine-sigorder.c | 255 +++++++++++++++++++++++++++- 2 files changed, 247 insertions(+), 9 deletions(-) diff --git a/rust/src/utils/flowbits_resolver.rs b/rust/src/utils/flowbits_resolver.rs index 0fd41e9272..c875d6758b 100644 --- a/rust/src/utils/flowbits_resolver.rs +++ b/rust/src/utils/flowbits_resolver.rs @@ -222,7 +222,6 @@ pub unsafe extern "C" fn SCResolveFlowbitDependencies( ) -> i8 { SCLogDebug!("Attempting to resolve flowbit dependencies"); let fss = &mut *(fss_void as *mut FlowbitSidStore); - let r = check_cycle_update_graph(&mut fss.graph); if r == -1 { SCLogError!("Couldn't do anything to fix the graph. Retreating.."); diff --git a/src/detect-engine-sigorder.c b/src/detect-engine-sigorder.c index 98012cf9d3..c108c3ddfb 100644 --- a/src/detect-engine-sigorder.c +++ b/src/detect-engine-sigorder.c @@ -1,4 +1,4 @@ -/* Copyright (C) 2007-2024 Open Information Security Foundation +/* Copyright (C) 2007-2026 Open Information Security Foundation * * You can copy, redistribute or modify this Program under the terms of * the GNU General Public License version 2 as published by the Free @@ -40,6 +40,8 @@ #include "action-globals.h" #include "flow-util.h" #include "util-validate.h" +#include "suricata.h" +#include "rust.h" #define DETECT_FLOWVAR_NOT_USED 1 #define DETECT_FLOWVAR_TYPE_READ 2 @@ -619,6 +621,192 @@ static SCSigSignatureWrapper *SCSigOrder(SCSigSignatureWrapper *sw, return result; } +static int CreateGraphFromFlowbitAnalyzer( + void *graph, SCSigSignatureWrapper *sw, uint32_t max_fb_id) +{ + SCSigSignatureWrapper *tmp = sw; + if (max_fb_id == 0) + return 0; + + struct FBAnalyzer fba = { .array = NULL, .array_size = 0 }; + const uint32_t array_size = max_fb_id + 1; + struct FBAnalyze *array = SCCalloc(array_size, sizeof(struct FBAnalyze)); + if (array == NULL) { + SCLogError("Unable to allocate flowbit analyze array"); + return -1; + } + fba.array = array; + fba.array_size = array_size; + + SCLogDebug( + "fb analyzer array size: %" PRIu64, (uint64_t)(array_size * sizeof(struct FBAnalyze))); + + /* fill flowbit array, updating counters per sig */ + while (tmp != NULL) { + const Signature *s = tmp->sig; + + int64_t node = SCGetOrCreateNodeGraph(graph, s->iid, s->id); + if (node < 0) { + FatalErrorOnInit("Couldn't create or get a node in the graph for sid: %d", s->id); + } + SCLogDebug("added node: %ld", node); + + int r = DetectFlowbitsAnalyzeSignature(s, &fba); + if (r < 0) { + FBAnalyzerFree(&fba); + return -1; + } + tmp = tmp->next; + } + + // varnamestore ids start at 1 + for (uint32_t x = 1; x < array_size; x++) { + for (uint32_t i = 0; i < fba.array[x].isset_iids_idx; i++) { + SCLogDebug("Getting or creating node for sid: %d", fba.array[x].isset_iids[i].sid); + int64_t to = SCGetOrCreateNodeGraph( + graph, fba.array[x].isset_iids[i].iid, fba.array[x].isset_iids[i].sid); + if (to < 0) { + FatalErrorOnInit("Couldn't create or get a node in the graph for sid: %d", + fba.array[x].isset_iids[i].sid); + } + SCLogDebug("added node: %ld", to); + for (uint32_t y = 0; y < fba.array[x].set_iids_idx; y++) { + SCLogDebug("Getting or creating node for iid: %d", fba.array[x].set_iids[y].sid); + int64_t from = SCGetOrCreateNodeGraph( + graph, fba.array[x].set_iids[y].iid, fba.array[x].set_iids[y].sid); + if (from < 0) { + FatalErrorOnInit("Couldn't create or get a node in the graph for sid: %d", + fba.array[x].set_iids[y].sid); + } + SCLogDebug("added node: %ld", from); + if (SCCreateNodeEdgeDirectedGraph( + graph, (uint32_t)from, (uint32_t)to, DETECT_FLOWBITS_CMD_SET, x) < 0) { + FatalErrorOnInit( + "Flowbits signatures that are unsatisfiable at runtime found: %d, %d", + fba.array[x].isset_iids[i].sid, fba.array[x].set_iids[y].sid); + } + } + } + } + + for (uint32_t x = 1; x < array_size; x++) { + for (uint32_t i = 0; i < fba.array[x].isnotset_iids_idx; i++) { + int64_t to = SCGetOrCreateNodeGraph( + graph, fba.array[x].isnotset_iids[i].iid, fba.array[x].isnotset_iids[i].sid); + if (to < 0) { + FatalErrorOnInit("Couldn't create or get a node in the graph for sid: %d", + fba.array[x].isnotset_iids[i].sid); + } + SCLogDebug("added node: %ld", to); + for (uint32_t y = 0; y < fba.array[x].unset_iids_idx; y++) { + int64_t from = SCGetOrCreateNodeGraph( + graph, fba.array[x].unset_iids[y].iid, fba.array[x].unset_iids[y].sid); + if (from < 0) { + FatalErrorOnInit("Couldn't create or get a node in the graph for sid: %d", + fba.array[x].unset_iids[y].sid); + } + SCLogDebug("added node: %ld", from); + if (SCCreateNodeEdgeDirectedGraph(graph, (uint32_t)from, (uint32_t)to, + DETECT_FLOWBITS_CMD_UNSET, x) < 0) { + FatalErrorOnInit( + "Flowbits signatures that are unsatisfiable at runtime found: %d, %d", + fba.array[x].isnotset_iids[i].sid, fba.array[x].unset_iids[y].sid); + } + } + } + } + + FBAnalyzerFree(&fba); + return 0; +} + +/** + * \brief Function to Resolve dependencies among flowbits + * + * \param arg_sw Signature Wrapper containing all flowbits of SET_READ type + * + * \return SCSigSignatureWrapper list post dependency resolution + */ +static SCSigSignatureWrapper *SCSigResolveFlowbitDependencies( + SCSigSignatureWrapper *head, uint32_t max_fb_id) +{ + uint32_t sig_cnt = 0; + uint32_t *sorted_iids = NULL; + SCSigSignatureWrapper *tmp = NULL; + + SCLogInfo("Signatures with flowbits found"); + + if (head == NULL) { + return NULL; + } + if (head->next == NULL) { + return head; + } + tmp = head; + while (tmp != NULL) { + ++sig_cnt; + tmp = tmp->next; + } + SCLogDebug("sig_cnt: %d", sig_cnt); + + DEBUG_VALIDATE_BUG_ON(max_fb_id == 0); + DEBUG_VALIDATE_BUG_ON(sig_cnt == 0); + void *graph = SCCreateDirectedGraph(); + if (CreateGraphFromFlowbitAnalyzer(graph, head, max_fb_id) < 0) { + FatalErrorOnInit("Could not create Flowbit Analyzer Graph"); + } + + sorted_iids = SCCalloc(sig_cnt, sizeof(uint32_t)); + if (sorted_iids == NULL) { + goto error; + } + int ret = SCResolveFlowbitDependencies(graph, sorted_iids, sig_cnt); + if (ret < 0) { + goto error; + } + + SCSigSignatureWrapper *fin = NULL; + tmp = NULL; + + for (uint32_t i = 0; i < sig_cnt; i++) { + SCSigSignatureWrapper *prev = NULL; + SCSigSignatureWrapper *cur = head; + + while (cur != NULL) { + if (sorted_iids[i] == cur->sig->iid) { + if (prev == NULL) { + head = cur->next; + } else { + prev->next = cur->next; + } + + cur->next = NULL; + if (tmp != NULL) { + tmp->next = cur; + } else { + fin = cur; + } + tmp = cur; + break; + } + prev = cur; + cur = cur->next; + } + } + + SCFree(sorted_iids); /* No longer needed */ + SCFreeDirectedGraph(graph); + return fin; + +error: + SCLogError("Error resolving flowbit dependencies"); + if (sorted_iids != NULL) { + SCFree(sorted_iids); + } + SCFreeDirectedGraph(graph); + return NULL; +} + /** * \brief Orders an incoming Signature based on its action * @@ -809,12 +997,17 @@ int SCSigOrderSignatures(DetectEngineCtx *de_ctx) int retval = 0; SCLogDebug("ordering signatures in memory"); SCSigSignatureWrapper *sigw = NULL; - SCSigSignatureWrapper *td_sigw_list = NULL; /* unified td list */ + + SCSigSignatureWrapper *td_sigw_list_start = NULL; /* unified td list start */ + SCSigSignatureWrapper *td_sigw_list_end = NULL; /* unified td list end */ + + SCSigSignatureWrapper *fb_sigw_list_start = NULL; /* flowbits list start */ SCSigSignatureWrapper *fw_pf_sigw_list = NULL; /* hook: packet_filter */ SCSigSignatureWrapper *fw_af_sigw_list = NULL; /* hook: app_filter */ Signature *sig = de_ctx->sig_list; + while (sig != NULL) { sigw = SCSigAllocSignatureWrapper(sig); if (sigw == NULL) { @@ -834,11 +1027,26 @@ int SCSigOrderSignatures(DetectEngineCtx *de_ctx) fw_af_sigw_list = sigw; } } else { - sigw->next = td_sigw_list; - td_sigw_list = sigw; + /* Flowbit specific handling. Store the three types of flowbit signatures + * separately for an easy merge of the lists by priority later on */ + if (sigw->user[DETECT_SIGORDER_FLOWBITS] > DETECT_FLOWBITS_NOT_USED) { + sigw->next = fb_sigw_list_start; + fb_sigw_list_start = sigw; + } else { + sigw->next = td_sigw_list_start; + if (!td_sigw_list_end) + td_sigw_list_end = sigw; + td_sigw_list_start = sigw; + } } sig = sig->next; } +#if DEBUG + SCLogDebug("Initial list:"); + for (sigw = fb_sigw_list_start; sigw != NULL; sigw = sigw->next) { + SCLogDebug("sig id: %d, iid: %d", sigw->sig->id, sigw->sig->iid); + } +#endif /* despite having Append in the name, the new Sig/Rule funcs actually prepend with some special * logic around bidir sigs. So to respect the firewall rule order, we sort this part of the list @@ -851,10 +1059,41 @@ int SCSigOrderSignatures(DetectEngineCtx *de_ctx) SCSigOrderFunc OrderFn = { .SWCompare = SCSigOrderByAppFirewall, .next = NULL }; fw_af_sigw_list = SCSigOrder(fw_af_sigw_list, &OrderFn); } - if (td_sigw_list) { - /* Sort the list */ - td_sigw_list = SCSigOrder(td_sigw_list, de_ctx->sc_sig_order_funcs); + if (fb_sigw_list_start) { + /* Resolve any complex dependencies, if possible, or roll back to the original ruleset */ + SCSigSignatureWrapper *tmp = + SCSigResolveFlowbitDependencies(fb_sigw_list_start, de_ctx->max_fb_id); + if (tmp == NULL) { + SC_ATOMIC_EXTERN(unsigned int, engine_stage); + if (SC_ATOMIC_GET(engine_stage) == SURICATA_INIT) { + FatalErrorOnInit( + "Flowbits have circular dependencies that cannot be met at runtime"); + } else { + SCLogWarning("Applying the existing ruleset as flowbit dependencies could not be " + "resolved"); + } + } else { + fb_sigw_list_start = tmp; + } + } + +#if DEBUG + SCLogDebug("Right BEFORE SigOrder:"); + for (sigw = fb_sigw_list_start; sigw != NULL; sigw = sigw->next) { + SCLogDebug("sig id: %d", sigw->sig->id); + } +#endif + + if (td_sigw_list_end) { + td_sigw_list_end->next = fb_sigw_list_start; + } else if (fb_sigw_list_start) { + DEBUG_VALIDATE_BUG_ON(td_sigw_list_start != NULL); + td_sigw_list_start = fb_sigw_list_start; } + if (td_sigw_list_start) { + td_sigw_list_start = SCSigOrder(td_sigw_list_start, de_ctx->sc_sig_order_funcs); + } + /* Recreate the sig list in order */ de_ctx->sig_list = NULL; @@ -894,7 +1133,7 @@ cleanup: SCFree(sigw_to_free); } /* threat detect list for hook app_td */ - for (sigw = td_sigw_list; sigw != NULL;) { + for (sigw = td_sigw_list_start; sigw != NULL;) { sigw->sig->next = NULL; if (de_ctx->sig_list == NULL) { /* First entry on the list */ From fdea828bcc404eb719a98f89e796043da540e918 Mon Sep 17 00:00:00 2001 From: Shivani Bhardwaj Date: Mon, 20 Apr 2026 18:32:03 +0530 Subject: [PATCH 04/12] detect/sigorder: do not disturb the order of SET_READ flowbit sigs The final list of signatures must be sorted with all the sorting fns available. However, flowbits SET_READ have their order derived from special handling between the dependent flowbits. Using the usual sorting criteria will mess up the order that was created. Bug 7771 Bug 7638 Bug 1399 --- src/detect-engine-sigorder.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/src/detect-engine-sigorder.c b/src/detect-engine-sigorder.c index c108c3ddfb..4a548ea51c 100644 --- a/src/detect-engine-sigorder.c +++ b/src/detect-engine-sigorder.c @@ -537,6 +537,14 @@ static int SCSigLessThan(SCSigSignatureWrapper *sw1, { SCSigOrderFunc *funcs = cmp_func_list; + // Special handling for flowbits, do not touch the order if the comparison fn + // calculates two signatures to be equal + if ((sw1->user[DETECT_SIGORDER_FLOWBITS] > DETECT_FLOWBITS_NOT_USED) && + (sw1->user[DETECT_SIGORDER_FLOWBITS] == DETECT_FLOWBITS_TYPE_SET_READ) && + (sw1->user[DETECT_SIGORDER_FLOWBITS] == sw2->user[DETECT_SIGORDER_FLOWBITS])) { + return 1; + } + while (funcs != NULL) { int delta = funcs->SWCompare(sw1, sw2); if (delta > 0) @@ -546,6 +554,7 @@ static int SCSigLessThan(SCSigSignatureWrapper *sw1, funcs = funcs->next; } + // They are equal, so use sid as the final decider. return sw1->sig->id < sw2->sig->id; } @@ -745,6 +754,14 @@ static SCSigSignatureWrapper *SCSigResolveFlowbitDependencies( tmp = head; while (tmp != NULL) { ++sig_cnt; + /* The dependency-resolved order overrides any explicit priority for + * SET_READ flowbit rules. Warn once per such rule that set a priority. */ + if (tmp->user[DETECT_SIGORDER_FLOWBITS] == DETECT_FLOWBITS_TYPE_SET_READ && + (tmp->sig->init_data->init_flags & SIG_FLAG_INIT_PRIO_EXPLICIT)) { + SCLogWarning("sid %u: explicitly set priority is overridden by flowbits " + "dependency resolution ordering", + tmp->sig->id); + } tmp = tmp->next; } SCLogDebug("sig_cnt: %d", sig_cnt); From afdb89facd7a1047070c4c5317ad3c114bede18e Mon Sep 17 00:00:00 2001 From: Shivani Bhardwaj Date: Mon, 20 Apr 2026 18:31:39 +0530 Subject: [PATCH 05/12] flowbits/resolver: add a JSON output of the graph Add an output file called "flowbits_dependency_graph.json" that outputs the graph in the following format: 12 : { # sid of the node in: [{ # all incoming edges id: "1234" # internal ID of the node, weight: 2, sid: 13 }], out: [{ # all outgoing edges id: "124" # internal ID of the node, weight: 2, sid: 10 }] } in two conditions: 1. Right before any unrecoverable error. 2. At the end of the successful graph creation. --- rust/src/utils/flowbits_resolver.rs | 41 ++++++++++++++++++++++++++++- src/detect-engine-sigorder.c | 37 ++++++++++++++++++++++++++ 2 files changed, 77 insertions(+), 1 deletion(-) diff --git a/rust/src/utils/flowbits_resolver.rs b/rust/src/utils/flowbits_resolver.rs index c875d6758b..697c58681b 100644 --- a/rust/src/utils/flowbits_resolver.rs +++ b/rust/src/utils/flowbits_resolver.rs @@ -17,6 +17,7 @@ // Author: Shivani Bhardwaj +use crate::jsonbuilder::*; use petgraph::algo::{is_cyclic_directed, tarjan_scc, toposort}; use petgraph::graph::{GraphError, NodeIndex}; use petgraph::stable_graph::StableDiGraph; @@ -121,8 +122,46 @@ pub unsafe extern "C" fn SCCreateNodeEdgeDirectedGraph( return 0; } +fn log_graph( + js: &mut JsonBuilder, graph: &mut StableDiGraph, +) -> Result<(), JsonError> { + SCLogDebug!("Starting the logging.."); + for node in graph.node_weights() { + SCLogDebug!("{:?}", node.nidx.index()); + js.open_object(&node.sid.to_string())?; + js.open_array("in")?; + for edge in graph.edges_directed(node.nidx, Direction::Incoming) { + js.start_object()?; + js.set_uint("id", edge.source().index() as u64)?; + js.set_uint("weight", edge.weight().cmd as u64)?; + js.set_uint("sid", graph[edge.source()].sid as u64)?; + js.close()?; + } + js.close()?; + js.open_array("out")?; + for edge in graph.edges_directed(node.nidx, Direction::Outgoing) { + js.start_object()?; + js.set_uint("id", edge.target().index() as u64)?; + js.set_uint("weight", edge.weight().cmd as u64)?; + js.set_uint("sid", graph[edge.target()].sid as u64)?; + js.close()?; + } + js.close()?; + js.close()?; + } + Ok(()) +} + +#[no_mangle] +pub unsafe extern "C" fn SCDebugLogFlowbitGraph( + jsb: &mut JsonBuilder, fss_void: *mut c_void, +) -> bool { + let fss = &mut *(fss_void as *mut FlowbitSidStore); + log_graph(jsb, &mut fss.graph).is_ok() +} + fn check_cycle_update_graph(graph: &mut StableDiGraph) -> i8 { - let mut MAX_STACK_SIZE: usize = 100; + let MAX_STACK_SIZE: usize = 100; for i in 0..MAX_STACK_SIZE { /* Check graph for any cycles */ diff --git a/src/detect-engine-sigorder.c b/src/detect-engine-sigorder.c index 4a548ea51c..07e3710aad 100644 --- a/src/detect-engine-sigorder.c +++ b/src/detect-engine-sigorder.c @@ -37,6 +37,7 @@ #include "util-unittest-helper.h" #include "util-debug.h" #include "util-action.h" +#include "util-conf.h" #include "action-globals.h" #include "flow-util.h" #include "util-validate.h" @@ -68,6 +69,9 @@ #define DETECT_XBITS_TYPE_SET_READ 3 #define DETECT_XBITS_TYPE_SET 4 +extern bool rule_engine_analysis_set; +SCMutex g_flowbits_graph_dump_write_m = SCMUTEX_INITIALIZER; + /** * \brief Different kinds of helper data that can be used by the signature * ordering module. Used by the "user" field in SCSigSignatureWrapper @@ -630,6 +634,30 @@ static SCSigSignatureWrapper *SCSigOrder(SCSigSignatureWrapper *sw, return result; } +static inline void SCLogFlowbitsGraph(void *graph) +{ + SCJsonBuilder *jb = SCJbNewObject(); + if (jb == NULL) + return; + + SCDebugLogFlowbitGraph(jb, graph); + SCJbClose(jb); + const char *filename = "flowbits_dependency_graph.json"; + const char *log_dir = SCConfigGetLogDirectory(); + char log_path[PATH_MAX] = ""; + snprintf(log_path, sizeof(log_path), "%s/%s", log_dir, filename); + + SCMutexLock(&g_flowbits_graph_dump_write_m); + FILE *fp = fopen(log_path, "w"); + if (fp != NULL) { + fwrite(SCJbPtr(jb), SCJbLen(jb), 1, fp); + fprintf(fp, "\n"); + fclose(fp); + } + SCMutexUnlock(&g_flowbits_graph_dump_write_m); + SCJbFree(jb); +} + static int CreateGraphFromFlowbitAnalyzer( void *graph, SCSigSignatureWrapper *sw, uint32_t max_fb_id) { @@ -690,6 +718,9 @@ static int CreateGraphFromFlowbitAnalyzer( SCLogDebug("added node: %ld", from); if (SCCreateNodeEdgeDirectedGraph( graph, (uint32_t)from, (uint32_t)to, DETECT_FLOWBITS_CMD_SET, x) < 0) { + if (rule_engine_analysis_set) { + SCLogFlowbitsGraph(graph); + } FatalErrorOnInit( "Flowbits signatures that are unsatisfiable at runtime found: %d, %d", fba.array[x].isset_iids[i].sid, fba.array[x].set_iids[y].sid); @@ -717,6 +748,9 @@ static int CreateGraphFromFlowbitAnalyzer( SCLogDebug("added node: %ld", from); if (SCCreateNodeEdgeDirectedGraph(graph, (uint32_t)from, (uint32_t)to, DETECT_FLOWBITS_CMD_UNSET, x) < 0) { + if (rule_engine_analysis_set) { + SCLogFlowbitsGraph(graph); + } FatalErrorOnInit( "Flowbits signatures that are unsatisfiable at runtime found: %d, %d", fba.array[x].isnotset_iids[i].sid, fba.array[x].unset_iids[y].sid); @@ -777,6 +811,9 @@ static SCSigSignatureWrapper *SCSigResolveFlowbitDependencies( if (sorted_iids == NULL) { goto error; } + if (rule_engine_analysis_set) { + SCLogFlowbitsGraph(graph); + } int ret = SCResolveFlowbitDependencies(graph, sorted_iids, sig_cnt); if (ret < 0) { goto error; From 84995ea7532b23dfafee52c08a28f5684f9c6565 Mon Sep 17 00:00:00 2001 From: Shivani Bhardwaj Date: Fri, 29 May 2026 13:19:13 +0530 Subject: [PATCH 06/12] detect/flowbits: add max-cycle-resolution config In order to deal with the complex flowbits ordering, Suricata internally makes a graph of the signature dependencies created by the flowbits. In this graph, some dependencies may lead to cycles that are not always valid. Suricata tries to resolve these cycles and come up with a final order of signatures that makes sense. Add a configurable limit detect.flowbits.max-cycle-resolution that can be used to asses when to give up. Bug 7638 --- rust/src/utils/flowbits_resolver.rs | 21 ++++++++++++++++++--- suricata.yaml.in | 6 ++++++ 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/rust/src/utils/flowbits_resolver.rs b/rust/src/utils/flowbits_resolver.rs index 697c58681b..9dd204400d 100644 --- a/rust/src/utils/flowbits_resolver.rs +++ b/rust/src/utils/flowbits_resolver.rs @@ -17,6 +17,7 @@ // Author: Shivani Bhardwaj +use crate::conf::*; use crate::jsonbuilder::*; use petgraph::algo::{is_cyclic_directed, tarjan_scc, toposort}; use petgraph::graph::{GraphError, NodeIndex}; @@ -161,9 +162,20 @@ pub unsafe extern "C" fn SCDebugLogFlowbitGraph( } fn check_cycle_update_graph(graph: &mut StableDiGraph) -> i8 { - let MAX_STACK_SIZE: usize = 100; + let mut max_stack_size: usize = 100; + if let Some(val) = conf_get("detect.flowbits.max-cycle-resolution") { + if let Ok(v) = val.parse::() { + if v > u8::MAX as usize { + SCLogError!("Invalid value for max-cycle-resolution"); + } else { + max_stack_size = v; + } + } else { + SCLogError!("Invalid value for max-cycle-resolution"); + } + } - for i in 0..MAX_STACK_SIZE { + for i in 0..=max_stack_size { /* Check graph for any cycles */ if !is_cyclic_directed(&*graph) { SCLogDebug!("no cycles after {} tries", i); @@ -171,6 +183,9 @@ fn check_cycle_update_graph(graph: &mut StableDiGraph) -> i8 { } SCLogDebug!("Found a cycle in i {}. Checking if it's valid..", i); + if i == max_stack_size { + break; + } if !try_resolve_one_cycle(graph) { /* If we can't resolve any cycle, we're stuck */ @@ -181,7 +196,7 @@ fn check_cycle_update_graph(graph: &mut StableDiGraph) -> i8 { SCLogError!( "Maximum tries ({}) reached while trying to resolve cycles", - MAX_STACK_SIZE + max_stack_size ); return -1; } diff --git a/suricata.yaml.in b/suricata.yaml.in index c4a4983bd6..87d10aea8a 100644 --- a/suricata.yaml.in +++ b/suricata.yaml.in @@ -1897,6 +1897,12 @@ detect: #flowbits: # max-per-signature: 8 # maximum number of flowbits keyword usage per signature, minimum value allowed is 1 + # In order to determine the correct order of signatures when flowbits are present + # and may have inter dependencies, Suricata makes a dependency graph of the + # signatures with flowbits and attempts to resolve any cycles that can be + # fixed to give a plausible order of signatures. This value determines how many + # times Suricata will try to resolve cycles before it gives up. + #max-cycle-resolution: 100 # Select the multi pattern algorithm you want to run for scan/search the # in the engine. From 1d77925a04f6251c867c67faad3870b33123412b Mon Sep 17 00:00:00 2001 From: Shivani Bhardwaj Date: Mon, 20 Apr 2026 15:58:35 +0530 Subject: [PATCH 07/12] detect/sigorder: make merge sort stable Merge sort by default is a stable sorting algorithm which means that the relative order of all elements must be preserved in the final list. However, the implementation in SCSigOrder, instead of breaking each of the arrays into halves and going about the usual divide and conquer, ends up doing two things that make it unstable: 1. each array has a reversed order 2. the two halves at any point are created as by picking one item from the main array and putting it in the first half followed by picking second item from the main array and putting it in the second half. Fix this by first finding the midpoint of the main array in the current recursive call and breaking the array there. The midpoint is found using the Tortoise and Hare algorithm. The heads of each of the divided arrays point to the beginning. This guarantees the relative order among the elements to be stable. This becomes a bug when the final order is not to be messed with as done in the solution for Bug 7638 Bug 7771 Bug 1399 --- src/detect-engine-sigorder.c | 56 ++++++++++++++++++++++-------------- 1 file changed, 34 insertions(+), 22 deletions(-) diff --git a/src/detect-engine-sigorder.c b/src/detect-engine-sigorder.c index 07e3710aad..426a74a780 100644 --- a/src/detect-engine-sigorder.c +++ b/src/detect-engine-sigorder.c @@ -570,38 +570,50 @@ static SCSigSignatureWrapper *SCSigOrder(SCSigSignatureWrapper *sw, { DEBUG_VALIDATE_BUG_ON(sw == NULL); + if (sw->next == NULL) { + /* only one element in the list */ + return sw; + } + SCSigSignatureWrapper *subA = NULL; SCSigSignatureWrapper *subB = NULL; - SCSigSignatureWrapper *first; - SCSigSignatureWrapper *second; SCSigSignatureWrapper *result = NULL; SCSigSignatureWrapper *last = NULL; SCSigSignatureWrapper *new = NULL; - /* Divide input list into two sub-lists. */ - while (sw != NULL) { - first = sw; - sw = sw->next; - /* Push the first element onto sub-list A */ - first->next = subA; - subA = first; - - if (sw == NULL) - break; - second = sw; - sw = sw->next; - /* Push the second element onto sub-list B */ - second->next = subB; - subB = second; - } - if (subB == NULL) { - /* Only zero or one element on the list. */ - return subA; + /* Divide input list into two sub-lists using Tortoise and Hare algorithm */ + SCSigSignatureWrapper *slow = sw; + SCSigSignatureWrapper *fast = sw; /* 2x faster pointer */ + SCSigSignatureWrapper *prev = NULL; + + while (fast != NULL && fast->next != NULL) { + prev = slow; + slow = slow->next; + fast = fast->next->next; } + + prev->next = NULL; /* Cut the first half */ + subA = sw; /* First half */ + subB = slow; /* Second half */ + + SCLogDebug("subA is: %d; subB is: %d", subA->sig->id, subB->sig->id); + DEBUG_VALIDATE_BUG_ON(subA == NULL); + DEBUG_VALIDATE_BUG_ON(subB == NULL); - /* Now sort each list */ +#ifdef DEBUG + SCLogDebug("subA:"); + for (SCSigSignatureWrapper *sigw = subA; sigw != NULL; sigw = sigw->next) { + SCLogDebug("sig_id: %d", sigw->sig->id); + } +#endif subA = SCSigOrder(subA, cmp_func_list); +#ifdef DEBUG + SCLogDebug("subB:"); + for (SCSigSignatureWrapper *sigw = subB; sigw != NULL; sigw = sigw->next) { + SCLogDebug("sig_id: %d", sigw->sig->id); + } +#endif subB = SCSigOrder(subB, cmp_func_list); DEBUG_VALIDATE_BUG_ON(subA == NULL); DEBUG_VALIDATE_BUG_ON(subB == NULL); From cf1714eb3d5206eab37b51c4fe8cc8c8f3265131 Mon Sep 17 00:00:00 2001 From: Shivani Bhardwaj Date: Thu, 3 Sep 2026 13:43:47 +0530 Subject: [PATCH 08/12] flowbits_resolver: add parallel different edges --- rust/src/utils/flowbits_resolver.rs | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/rust/src/utils/flowbits_resolver.rs b/rust/src/utils/flowbits_resolver.rs index 9dd204400d..f35115cd06 100644 --- a/rust/src/utils/flowbits_resolver.rs +++ b/rust/src/utils/flowbits_resolver.rs @@ -46,6 +46,7 @@ struct SCGEdge { struct FlowbitSidStore { graph: StableDiGraph, iid_map: HashMap, + edge_set: HashSet<(u32, u32, u8, u32)>, } /// Function to create an empty directed Graph @@ -57,6 +58,7 @@ pub unsafe extern "C" fn SCCreateDirectedGraph() -> *mut c_void { let fb_sid_store = FlowbitSidStore { graph: StableDiGraph::new(), iid_map: HashMap::new(), + edge_set: HashSet::new(), }; /* Make an opaque pointer for C as nothing is changed there */ @@ -92,9 +94,20 @@ pub unsafe extern "C" fn SCCreateNodeEdgeDirectedGraph( let from_idx = NodeIndex::from(from); let to_idx = NodeIndex::from(to); + if !fss.edge_set.insert((from, to, cmd, fb_id)) { + SCLogDebug!( + "Skipped duplicate edge from {:?} -> {:?} for flowbit {:?} with command: {:?}", + from, + to, + fb_id, + cmd + ); + return 0; + } + match fss .graph - .try_update_edge(from_idx, to_idx, SCGEdge { cmd, fb_id }) + .try_add_edge(from_idx, to_idx, SCGEdge { cmd, fb_id }) { /* edge from a flowbit setter to a flowbit reader */ Ok(_) => { From 5f4dd6ed5797d223957c6cbceee2332feaab55af Mon Sep 17 00:00:00 2001 From: Shivani Bhardwaj Date: Thu, 3 Sep 2026 13:51:44 +0530 Subject: [PATCH 09/12] detect/sigorder: leave firewall rules out from flowbit exception --- src/detect-engine-sigorder.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/detect-engine-sigorder.c b/src/detect-engine-sigorder.c index 426a74a780..f49b0c4cc5 100644 --- a/src/detect-engine-sigorder.c +++ b/src/detect-engine-sigorder.c @@ -543,7 +543,8 @@ static int SCSigLessThan(SCSigSignatureWrapper *sw1, // Special handling for flowbits, do not touch the order if the comparison fn // calculates two signatures to be equal - if ((sw1->user[DETECT_SIGORDER_FLOWBITS] > DETECT_FLOWBITS_NOT_USED) && + if (!(sw1->sig->init_data->firewall_rule || sw2->sig->init_data->firewall_rule) && + (sw1->user[DETECT_SIGORDER_FLOWBITS] > DETECT_FLOWBITS_NOT_USED) && (sw1->user[DETECT_SIGORDER_FLOWBITS] == DETECT_FLOWBITS_TYPE_SET_READ) && (sw1->user[DETECT_SIGORDER_FLOWBITS] == sw2->user[DETECT_SIGORDER_FLOWBITS])) { return 1; From 9fdca04c60e1231fc40bdd9b2b745a7d724d0140 Mon Sep 17 00:00:00 2001 From: Shivani Bhardwaj Date: Thu, 3 Sep 2026 14:39:01 +0530 Subject: [PATCH 10/12] flowbits: make max-cycle-resolution reloadable --- rust/src/utils/flowbits_resolver.rs | 21 ++++++--------------- src/detect-engine-sigorder.c | 8 ++++---- src/detect-engine.c | 22 ++++++++++++++++++++++ src/detect.h | 2 ++ 4 files changed, 34 insertions(+), 19 deletions(-) diff --git a/rust/src/utils/flowbits_resolver.rs b/rust/src/utils/flowbits_resolver.rs index f35115cd06..c87b137a6a 100644 --- a/rust/src/utils/flowbits_resolver.rs +++ b/rust/src/utils/flowbits_resolver.rs @@ -17,7 +17,6 @@ // Author: Shivani Bhardwaj -use crate::conf::*; use crate::jsonbuilder::*; use petgraph::algo::{is_cyclic_directed, tarjan_scc, toposort}; use petgraph::graph::{GraphError, NodeIndex}; @@ -174,19 +173,10 @@ pub unsafe extern "C" fn SCDebugLogFlowbitGraph( log_graph(jsb, &mut fss.graph).is_ok() } -fn check_cycle_update_graph(graph: &mut StableDiGraph) -> i8 { - let mut max_stack_size: usize = 100; - if let Some(val) = conf_get("detect.flowbits.max-cycle-resolution") { - if let Ok(v) = val.parse::() { - if v > u8::MAX as usize { - SCLogError!("Invalid value for max-cycle-resolution"); - } else { - max_stack_size = v; - } - } else { - SCLogError!("Invalid value for max-cycle-resolution"); - } - } +fn check_cycle_update_graph( + graph: &mut StableDiGraph, max_cycle_resolution: u8, +) -> i8 { + let max_stack_size = max_cycle_resolution as usize; for i in 0..=max_stack_size { /* Check graph for any cycles */ @@ -286,10 +276,11 @@ fn try_resolve_one_cycle(graph: &mut StableDiGraph) -> bool { #[no_mangle] pub unsafe extern "C" fn SCResolveFlowbitDependencies( fss_void: *mut c_void, sorted_iid_list: *mut u32, sorted_iid_list_len: u32, + max_cycle_resolution: u8, ) -> i8 { SCLogDebug!("Attempting to resolve flowbit dependencies"); let fss = &mut *(fss_void as *mut FlowbitSidStore); - let r = check_cycle_update_graph(&mut fss.graph); + let r = check_cycle_update_graph(&mut fss.graph, max_cycle_resolution); if r == -1 { SCLogError!("Couldn't do anything to fix the graph. Retreating.."); return -1; diff --git a/src/detect-engine-sigorder.c b/src/detect-engine-sigorder.c index f49b0c4cc5..04dc7dd5e8 100644 --- a/src/detect-engine-sigorder.c +++ b/src/detect-engine-sigorder.c @@ -784,7 +784,7 @@ static int CreateGraphFromFlowbitAnalyzer( * \return SCSigSignatureWrapper list post dependency resolution */ static SCSigSignatureWrapper *SCSigResolveFlowbitDependencies( - SCSigSignatureWrapper *head, uint32_t max_fb_id) + SCSigSignatureWrapper *head, uint32_t max_fb_id, uint8_t max_cycle_resolution) { uint32_t sig_cnt = 0; uint32_t *sorted_iids = NULL; @@ -827,7 +827,7 @@ static SCSigSignatureWrapper *SCSigResolveFlowbitDependencies( if (rule_engine_analysis_set) { SCLogFlowbitsGraph(graph); } - int ret = SCResolveFlowbitDependencies(graph, sorted_iids, sig_cnt); + int ret = SCResolveFlowbitDependencies(graph, sorted_iids, sig_cnt, max_cycle_resolution); if (ret < 0) { goto error; } @@ -1128,8 +1128,8 @@ int SCSigOrderSignatures(DetectEngineCtx *de_ctx) } if (fb_sigw_list_start) { /* Resolve any complex dependencies, if possible, or roll back to the original ruleset */ - SCSigSignatureWrapper *tmp = - SCSigResolveFlowbitDependencies(fb_sigw_list_start, de_ctx->max_fb_id); + SCSigSignatureWrapper *tmp = SCSigResolveFlowbitDependencies( + fb_sigw_list_start, de_ctx->max_fb_id, de_ctx->max_flowbits_cycle_resolution); if (tmp == NULL) { SC_ATOMIC_EXTERN(unsigned int, engine_stage); if (SC_ATOMIC_GET(engine_stage) == SURICATA_INIT) { diff --git a/src/detect-engine.c b/src/detect-engine.c index 14c1740003..a7bc4ea5da 100644 --- a/src/detect-engine.c +++ b/src/detect-engine.c @@ -95,6 +95,7 @@ #define DETECT_ENGINE_DEFAULT_INSPECTION_RECURSION_LIMIT 3000 +#define DEFAULT_MAX_FLOWBITS_CYCLE_RESOLUTION_ATTEMPTS 100 #define DEFAULT_MAX_FLOWBITS_PER_SIGNATURE 8 static int DetectEngineCtxLoadConf(DetectEngineCtx *); @@ -2477,6 +2478,27 @@ static void DetectEngineLoadFlowbitSettings(DetectEngineCtx *de_ctx) } } SCLogConfig("Setting flowbits.max-per-signature to %d", de_ctx->max_flowbits); + + de_ctx->max_flowbits_cycle_resolution = DEFAULT_MAX_FLOWBITS_CYCLE_RESOLUTION_ATTEMPTS; + char cycle_varname[128] = "detect.flowbits.max-cycle-resolution"; + if (strlen(de_ctx->config_prefix) > 0) { + snprintf(cycle_varname, sizeof(cycle_varname), "%s.detect.flowbits.max-cycle-resolution", + de_ctx->config_prefix); + } + if (SCConfGet(cycle_varname, &str) == 1) { + uint8_t val = 0; + int ret = StringParseUint8(&val, 10, 0, str); + if (ret > 0) { + /* 0 is a valid setting: cycles are reported without any attempt to + * break them */ + de_ctx->max_flowbits_cycle_resolution = val; + } else { + SCLogWarning( + "Invalid setting for flowbits.max-cycle-resolution, resetting to the default"); + } + } + SCLogConfig( + "Setting flowbits.max-cycle-resolution to %d", de_ctx->max_flowbits_cycle_resolution); } /** \internal diff --git a/src/detect.h b/src/detect.h index 030a6a0e05..b81f9fc7a9 100644 --- a/src/detect.h +++ b/src/detect.h @@ -1052,6 +1052,8 @@ typedef struct DetectEngineCtx_ { /* max flowbit id that is used */ uint32_t max_fb_id; + uint8_t max_flowbits_cycle_resolution; + MpmCtxFactoryContainer *mpm_ctx_factory_container; /* array containing all sgh's in use so we can loop From a2db025d85583dca81e300fcf55732cf48b1d617 Mon Sep 17 00:00:00 2001 From: Shivani Bhardwaj Date: Fri, 4 Sep 2026 10:12:07 +0530 Subject: [PATCH 11/12] sigorder: except action from flowbit special handling --- src/detect-engine-sigorder.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/detect-engine-sigorder.c b/src/detect-engine-sigorder.c index 04dc7dd5e8..dc4f93d86a 100644 --- a/src/detect-engine-sigorder.c +++ b/src/detect-engine-sigorder.c @@ -543,7 +543,8 @@ static int SCSigLessThan(SCSigSignatureWrapper *sw1, // Special handling for flowbits, do not touch the order if the comparison fn // calculates two signatures to be equal - if (!(sw1->sig->init_data->firewall_rule || sw2->sig->init_data->firewall_rule) && + if ((ActionOrderVal(sw1->sig->action) == ActionOrderVal(sw2->sig->action)) && + !(sw1->sig->init_data->firewall_rule || sw2->sig->init_data->firewall_rule) && (sw1->user[DETECT_SIGORDER_FLOWBITS] > DETECT_FLOWBITS_NOT_USED) && (sw1->user[DETECT_SIGORDER_FLOWBITS] == DETECT_FLOWBITS_TYPE_SET_READ) && (sw1->user[DETECT_SIGORDER_FLOWBITS] == sw2->user[DETECT_SIGORDER_FLOWBITS])) { From 688141f915dda55b8e606144ad1c5a2b76a297f6 Mon Sep 17 00:00:00 2001 From: Shivani Bhardwaj Date: Fri, 4 Sep 2026 11:44:56 +0530 Subject: [PATCH 12/12] detect: reorder items in DetectEngineCtx to make scan-build happy --- src/detect.h | 200 ++++++++++++++++++++++++--------------------------- 1 file changed, 93 insertions(+), 107 deletions(-) diff --git a/src/detect.h b/src/detect.h index b81f9fc7a9..6c7a7de7fb 100644 --- a/src/detect.h +++ b/src/detect.h @@ -985,93 +985,35 @@ typedef uint8_t (*SCDetectRateFilterFunc)(const Packet *p, uint32_t sid, uint32_ /** \brief main detection engine ctx */ typedef struct DetectEngineCtx_ { - bool failure_fatal; - uint8_t flags; /**< only DE_QUIET */ - uint8_t mpm_matcher; /**< mpm matcher this ctx uses */ - uint8_t max_flowbits; /**< maximum number of flowbits per signature */ - uint32_t tenant_id; - MpmConfig *mpm_cfg; - uint8_t spm_matcher; /**< spm matcher this ctx uses */ - Signature *sig_list; - uint32_t sig_cnt; - - /* version of the srep data */ - uint32_t srep_version; /* reputation for netblocks */ SRepCIDRTree *srepCIDR_ctx; - Signature **sig_array; - uint32_t sig_array_len; /* size in array members */ - - uint32_t signum; /* used by the signature ordering module */ struct SCSigOrderFunc_ *sc_sig_order_funcs; - /* main sigs */ - DetectEngineLookupFlow flow_gh[FLOW_STATES]; - /** firewall policy table entry point */ struct DetectFirewallPolicies *fw_policies; /* init phase vars */ HashListTable *sgh_hash_table; - HashListTable *mpm_hash_table; HashListTable *pattern_hash_table; /* hash table used to cull out duplicate sigs */ HashListTable *dup_sig_hash_table; - DetectEngineIPOnlyCtx io_ctx; - - /* maximum recursion depth for content inspection */ - int inspection_recursion_limit; - - /* maximum number of times a tx will get logged for rules not using app-layer keywords */ - uint8_t guess_applayer_log_limit; - - /* force app-layer tx finding for alerts with signatures not having app-layer keywords */ - bool guess_applayer; - - /* registration id for per thread ctx for the filemagic/file.magic keywords */ - int filemagic_thread_ctx_id; - /* spm thread context prototype, built as spm matchers are constructed and * later used to construct thread context for each thread. */ SpmGlobalThreadCtx *spm_global_thread_ctx; - - /* Config options */ - - uint16_t max_uniq_toclient_groups; - uint16_t max_uniq_toserver_groups; - - /* max flowbit id that is used */ - uint32_t max_fb_id; - - uint8_t max_flowbits_cycle_resolution; - MpmCtxFactoryContainer *mpm_ctx_factory_container; /* array containing all sgh's in use so we can loop * through it in Stage4. */ struct SigGroupHead_ **sgh_array; - uint32_t sgh_array_cnt; - uint32_t sgh_array_size; - - int32_t sgh_mpm_context_proto_tcp_packet; - int32_t sgh_mpm_context_proto_udp_packet; - int32_t sgh_mpm_context_proto_other_packet; - int32_t sgh_mpm_context_stream; - - /* the max local id used amongst all sigs */ - int32_t byte_extract_max_local_id; - - /** version of the detect engine. The version is incremented on reloads */ - uint32_t version; /** sgh for signatures that match against invalid packets. In those cases * we can't lookup by proto, address, port as we don't have these */ @@ -1079,29 +1021,12 @@ typedef struct DetectEngineCtx_ { /** sgh for `alert ether` / `alert arp` etc. */ struct SigGroupHead_ *eth_non_ip_sgh; - - /* Maximum size of the buffer for decoded base64 data. */ - uint16_t base64_decode_max_len; - - /** Store rule file and line so that parsers can use them in errors. */ - int rule_line; const char *rule_file; const char *sigerror; - bool sigerror_silent; - bool sigerror_ok; - - /** The rule errored out due to missing requirements. */ - bool sigerror_requires; - - /* specify the configuration for mpm context factory */ - uint8_t sgh_mpm_ctx_cnf; - int keyword_id; /** hash list of keywords that need thread local ctxs */ HashListTable *keyword_hash; - DetectFileDataCfg *filedata_config; - #ifdef PROFILE_RULES struct SCProfileDetectCtx_ *profile_ctx; #endif @@ -1112,23 +1037,10 @@ typedef struct DetectEngineCtx_ { struct SCProfileSghDetectCtx_ *profile_sgh_ctx; uint32_t profile_match_logging_threshold; #endif - char config_prefix[64]; - enum DetectEngineType type; - - /** how many de_ctx' are referencing this */ - uint32_t ref_cnt; /** list in master: either active or freelist */ struct DetectEngineCtx_ *next; - - /** id of loader thread 'owning' this de_ctx */ - int loader_id; - - /** are we using just mpm or also other prefilters */ - enum DetectEnginePrefilterSetting prefilter_setting; - HashListTable *dport_hash_table; - DetectPort *tcp_priorityports; DetectPort *udp_priorityports; @@ -1142,29 +1054,17 @@ typedef struct DetectEngineCtx_ { * is in detect-engine.c::g_buffer_type_hash */ HashListTable *buffer_type_hash_name; HashListTable *buffer_type_hash_id; - uint32_t buffer_type_id; - - uint32_t app_mpms_list_cnt; DetectBufferMpmRegistry *app_mpms_list; + /* list with app inspect engines. Both the start-time registered ones and * the rule-time registered ones. */ DetectEngineAppInspectionEngine *app_inspect_engines; DetectEnginePktInspectionEngine *pkt_inspect_engines; DetectBufferMpmRegistry *pkt_mpms_list; - uint32_t pkt_mpms_list_cnt; DetectEngineFrameInspectionEngine *frame_inspect_engines; DetectBufferMpmRegistry *frame_mpms_list; - uint32_t frame_mpms_list_cnt; - - uint32_t prefilter_id; HashListTable *prefilter_hash_table; - /** time of last ruleset reload */ - struct timeval last_reload; - - /** signatures stats */ - SigFileLoaderStat sig_stat; - /* list of Fast Pattern registrations. Initially filled using a copy of * `g_fp_support_smlist_list`, then extended at rule loading time if needed */ SCFPSupportSMList *fp_support_smlist_list; @@ -1198,13 +1098,9 @@ typedef struct DetectEngineCtx_ { /* Track rule requirements for reporting after loading rules. */ SCDetectRequiresStatus *requirements; - /* number of signatures using filestore, limited as u16 */ - uint16_t filestore_cnt; - /* name store for non-prefilter engines. Used in profiling but * part of the API, so hash is always used. */ HashTable *non_pf_engine_names; - const char *firewall_rule_file_exclusive; /* user provided rate filter callbacks. */ @@ -1215,13 +1111,103 @@ typedef struct DetectEngineCtx_ { /* Hook for pre_stream engine if it is used. */ DetectPacketHookFunc PreStreamHook; - /** TCP pre_stream hook rule groups. One per direction. */ - struct SigGroupHead_ *pre_stream_sgh[2]; /* Hook for pre_flow engine if it is used. */ DetectPacketHookFunc PreFlowHook; + /** pre_flow hook rule groups. Before flow we don't know a direction yet. */ struct SigGroupHead_ *pre_flow_sgh; + + /** time of last ruleset reload */ + struct timeval last_reload; + + /** TCP pre_stream hook rule groups. One per direction. */ + struct SigGroupHead_ *pre_stream_sgh[2]; + + /** signatures stats */ + SigFileLoaderStat sig_stat; + DetectEngineIPOnlyCtx io_ctx; + + /* main sigs */ + DetectEngineLookupFlow flow_gh[FLOW_STATES]; + uint32_t tenant_id; + uint32_t sig_cnt; + + /* version of the srep data */ + uint32_t srep_version; + uint32_t sig_array_len; /* size in array members */ + uint32_t signum; + + /* maximum recursion depth for content inspection */ + int inspection_recursion_limit; + + /* registration id for per thread ctx for the filemagic/file.magic keywords */ + int filemagic_thread_ctx_id; + + /* max flowbit id that is used */ + uint32_t max_fb_id; + uint32_t sgh_array_cnt; + uint32_t sgh_array_size; + int32_t sgh_mpm_context_proto_tcp_packet; + int32_t sgh_mpm_context_proto_udp_packet; + int32_t sgh_mpm_context_proto_other_packet; + int32_t sgh_mpm_context_stream; + + /* the max local id used amongst all sigs */ + int32_t byte_extract_max_local_id; + + /** version of the detect engine. The version is incremented on reloads */ + uint32_t version; + + /** Store rule file and line so that parsers can use them in errors. */ + int rule_line; + int keyword_id; + enum DetectEngineType type; + + /** how many de_ctx' are referencing this */ + uint32_t ref_cnt; + + /** id of loader thread 'owning' this de_ctx */ + int loader_id; + + /** are we using just mpm or also other prefilters */ + enum DetectEnginePrefilterSetting prefilter_setting; + uint32_t buffer_type_id; + uint32_t app_mpms_list_cnt; + uint32_t pkt_mpms_list_cnt; + uint32_t frame_mpms_list_cnt; + uint32_t prefilter_id; + + /* Config options */ + uint16_t max_uniq_toclient_groups; + uint16_t max_uniq_toserver_groups; + + /* Maximum size of the buffer for decoded base64 data. */ + uint16_t base64_decode_max_len; + + /* number of signatures using filestore, limited as u16 */ + uint16_t filestore_cnt; + bool failure_fatal; + uint8_t flags; /**< only DE_QUIET */ + uint8_t mpm_matcher; /**< mpm matcher this ctx uses */ + uint8_t max_flowbits; /**< maximum number of flowbits per signature */ + uint8_t spm_matcher; /**< spm matcher this ctx uses */ + + /* maximum number of times a tx will get logged for rules not using app-layer keywords */ + uint8_t guess_applayer_log_limit; + + /* force app-layer tx finding for alerts with signatures not having app-layer keywords */ + bool guess_applayer; + uint8_t max_flowbits_cycle_resolution; + bool sigerror_silent; + bool sigerror_ok; + + /** The rule errored out due to missing requirements. */ + bool sigerror_requires; + + /* specify the configuration for mpm context factory */ + uint8_t sgh_mpm_ctx_cnf; + char config_prefix[64]; } DetectEngineCtx; /**