Feat/flowbit prefilter/v24 #12132

victorjulien · 2024-11-19T14:55:23Z

Draft for QA/CI & review. Not fully ready yet.

https://redmine.openinfosecfoundation.org/issues/2486

In preparation of flowbit prefilter work that needs this info earlier. Track potential prefilter sm's to avoid unnecessary looping during setup.

Allow for more efficient rules that 'prefilter' on flowbits with 'isset' logic. This prefilter is enabled by default, which means that if no mpm is present or no explicit prefilter is used, the flowbits prefilter will be set up for a rule. flowbits 'isset' prefilter For rules that have a 'flowbits:isset,<bit>' statement, a "regular" prefilter facility is created. It means that the rules are removed from the normal match list(s) and added to a prefilter engine that runs prior to the individual rule inspection stage. Implementation: the prefilter is implemented as an RB_TREE of flowbits, with the rule id's they "enable" stored per tree node. The matching logic is walking the list of bits set in the flow and looking each of them up in the RB_TREE, adding the rule ids of each of the matching bits to the list of rule candidates. The 'isset' prefilter has one important corner case, which is that bits can in fact be set during the rule evaluation stage. This is different from all other prefilter engines, that evaluate an immutable state (for the lifetime of the packets inspection). flowbits 'set' post-match prefilter For flowbits 'set' action, special post-match 'prefilter' facilities deal with this corner case. The high level logic is that these track which 'isset' sigs depend on them, and add these dependencies to the candidates list when a 'set' action occurs. This is implemented in a few steps: 1. flowbits 'set' is flagged 2. when 'set' action occurs the flowbit is added to a "post rule match work queue" 3. when the rule evaluation ends, the post-match "prefilter" engine is run on each of the flowbits in the "post rule match work queue" 4. these engines ammend the candidates list with the rule id dependencies for the flowbit 5. the candidates list is sorted to make sure within the execution for that packet the inspection order is maintained Ticket: OISF#2486.

victorjulien · 2024-11-19T15:08:34Z

src/detect-engine.c

@@ -2523,6 +2523,9 @@ static DetectEngineCtx *DetectEngineCtxInitReal(
        goto error;
    }

+    // TODO test enable flowbits prefilter


Still need to check how we may enable a selection of prefilter engines for keywords, but not all.

src/detect-flowbits.c

victorjulien · 2024-11-19T15:09:25Z

src/detect-flowbits.c

+        void *ptr = SCRealloc(
+                det_ctx->post_rule_work_queue.q, (det_ctx->post_rule_work_queue.size + QUEUE_STEP) *
+                                                         sizeof(PostRuleMatchWorkQueueItem));
+        BUG_ON(ptr == NULL);


todo: error handle

src/detect-flowbits.c

victorjulien · 2024-11-19T15:11:13Z

src/detect-flowbits.c

+    return 0;
+}
+
+// TODO misses IPOnly rules. IPOnly flowbit rules are set only though.


tests appear to show this is not an issue

src/detect-flowbits.c

victorjulien · 2024-11-19T15:14:30Z

src/detect-flowbits.c

+
+#define BLOCK_SIZE 8
+
+static int AddBitIsset(const DetectEngineCtx *de_ctx, struct PrefilterEngineFlowbits *ctx,


func needs docs and error handling

also needs cleanup as there is duplication between or and non-or support

victorjulien · 2024-11-19T15:17:56Z

src/detect-flowbits.c

+    return added;
+}
+
+/* TODO shouldn't add sids for which Signature::num is < our num. Is this possible after sorting? */


need to see if I can reach this condition in a test

victorjulien · 2024-11-19T15:21:39Z

src/detect-flowbits.c

+            if (fb->cmd == DETECT_FLOWBITS_CMD_SET) {
+                SCLogDebug(
+                        "DETECT_SM_LIST_POSTMATCH: sid %u DETECT_FLOWBITS set %u", s->id, fb->idx);
+                // else if (fb->cmd == DETECT_FLOWBITS_CMD_TOGGLE) {


todo: review 'toggle' support

victorjulien · 2024-11-19T15:23:23Z

src/detect-flowbits.c

+            }
+
+            SCLogDebug("setting up sets/toggles for sid %u", s->id);
+            if (AddBitSetToggle(de_ctx, &fb_analysis, set_ctx, fb, s) == 1) {


so this builds a post-rule-match state, that looks at which isset flowbits depend on the current "set" flowbit. Need to make this code clearer, and doc it

victorjulien · 2024-11-19T15:24:24Z

src/detect-flowbits.c

+                fb->prefilter = true; // flag the set/toggle to trigger the post-rule match logic
+            }
+
+            // TODO don't add for sigs that don't have isset in this sgh. Reasoning:


todo: this is essentially done in AddSids above

victorjulien · 2024-11-19T15:26:11Z

src/detect-flowbits.c

+        }
+
+        const DetectFlowbitsData *fb = (DetectFlowbitsData *)s->init_data->prefilter_sm->ctx;
+        if (fb_analysis.array[fb->idx].toggle_sids_idx ||


this uses the per sgh analysis, but maybe we need to check this for all groups?

victorjulien · 2024-11-19T15:28:36Z

src/detect-flowbits.c

+            }
+
+            if (set_ctx == NULL) {
+                set_ctx = SCCalloc(1, sizeof(*set_ctx));


this "set" post-rule-prefilter state should only be built if there are isset flowbit rules, with prefilter enabled, in the same sgh. Need to review is this is checked correctly.

src/detect-flowbits.h

victorjulien · 2024-11-19T15:31:35Z

src/detect.c

+                /* rewrite match_array to include the new additions */
+                Signature **m = match_array;
+                for (uint32_t x = 0; x < match_cnt; x++) {
+                    *m++ = (Signature *)replace[x];


victorjulien · 2024-11-19T15:31:54Z

src/detect.c

+            /* run post match prefilter engines on work queue */
+            PrefilterPostRuleMatch(det_ctx, scratch->sgh, p, pflow);
+
+            if (det_ctx->pmq.rule_id_array_cnt > 0) {


should probably move this into a helper func

victorjulien · 2024-11-19T15:32:02Z

src/detect.c

+                        de_ctx->sig_array_len, det_ctx->pmq.rule_id_array_cnt);
+                const Signature **r = replace;
+                for (uint32_t x = 0; x < match_cnt; x++) {
+                    *r++ = match_array[x];


victorjulien · 2024-11-19T15:32:51Z

src/detect.c

+                }
+                /* append the prefilter results, then sort it */
+                for (uint32_t x = 0; x < det_ctx->pmq.rule_id_array_cnt; x++) {
+                    // TODO what happens if a tx engine is added?


need to look into this

victorjulien · 2024-11-19T15:33:58Z

src/detect.c

+                        match_cnt++;
+                    }
+                }
+                // TODO if sid 12 is added twice, how do we dedup


will try to add a test for this

src/detect.h

codecov · 2024-11-19T15:38:38Z

Codecov Report

Attention: Patch coverage is 87.86611% with 87 lines in your changes missing coverage. Please review.

Project coverage is 83.01%. Comparing base (5d766df) to head (c036600).
Report is 158 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #12132       +/-   ##
===========================================
+ Coverage   62.68%   83.01%   +20.32%     
===========================================
  Files         840      909       +69     
  Lines      153669   258309   +104640     
===========================================
+ Hits        96323   214427   +118104     
+ Misses      57346    43882    -13464

Flag	Coverage Δ
fuzzcorpus	`60.18% <22.50%> (?)`
livemode	`19.39% <4.01%> (?)`
pcap	`44.27% <17.68%> (?)`
suricata-verify	`62.80% <85.04%> (+0.12%)`	⬆️
unittests	`59.17% <29.42%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

suricata-qa · 2024-11-19T20:40:03Z

Information: QA ran without warnings.

Pipeline 23430

suricata-qa · 2024-11-20T01:25:05Z

Information:

ERROR: QA failed on SURI_TLPR1_alerts_cmp.

field	baseline	test	%
	SURI_TLPR1_stats_chk
.app_layer.flow.dcerpc_tcp	40	42	105.0%

Pipeline 23439

suricata-qa · 2024-11-21T00:35:04Z

Information:

ERROR: QA failed on SURI_TLPR1_alerts_cmp.

field	baseline	test	%
	SURI_TLPR1_stats_chk
.app_layer.flow.dcerpc_tcp	40	42	105.0%

Pipeline 23467

suricata-qa · 2024-11-21T15:25:03Z

Information:

ERROR: QA failed on SURI_TLPR1_alerts_cmp.

field	baseline	test	%
	SURI_TLPR1_stats_chk
.app_layer.flow.dcerpc_tcp	40	42	105.0%

Pipeline 23475

victorjulien · 2025-01-21T13:00:15Z

replaced by #12380

victorjulien and others added 4 commits November 19, 2024 15:18

flowbits: unset returns state of the bit

72b7778

flowbits: allow setter to know if set is new

a81984d

detect: set mpm/prefilter during signature parsing

3fee590

In preparation of flowbit prefilter work that needs this info earlier. Track potential prefilter sm's to avoid unnecessary looping during setup.