From f1fdc1801e432ef7b227bd3ae516e6e108d839c6 Mon Sep 17 00:00:00 2001 From: Victor Julien Date: Thu, 5 Jun 2025 10:43:22 +0200 Subject: [PATCH] detect: set detect table for non-firewall mode as well This also exposed a difference between the handling of TD alerts in firewall vs non-firewall mode. In firewall mode the table/hook is also part of the alert ordering to make sure actions from packet:td are applied before app:td. Handle that explicitly for now. --- src/detect-engine-alert.c | 35 +++++++++++++++++++++++++---------- src/detect-engine.c | 16 ++++++++-------- src/detect-engine.h | 2 +- src/detect-parse.c | 29 ++++++++++++++--------------- src/detect.h | 35 +++++++++++++++++------------------ 5 files changed, 65 insertions(+), 52 deletions(-) diff --git a/src/detect-engine-alert.c b/src/detect-engine-alert.c index 571b8aede8..6988b2825b 100644 --- a/src/detect-engine-alert.c +++ b/src/detect-engine-alert.c @@ -192,12 +192,10 @@ static void PacketApplySignatureActions(Packet *p, const Signature *s, const Pac /* REJECT also sets ACTION_DROP, just make it more visible with this check */ if (pa->action & ACTION_DROP_REJECT) { uint8_t drop_reason = PKT_DROP_REASON_RULES; - if (s->flags & SIG_FLAG_FIREWALL) { - if (s->firewall_table == FIREWALL_TABLE_PACKET_PRE_STREAM) { - drop_reason = PKT_DROP_REASON_STREAM_PRE_HOOK; - } else if (s->firewall_table == FIREWALL_TABLE_PACKET_PRE_FLOW) { - drop_reason = PKT_DROP_REASON_FLOW_PRE_HOOK; - } + if (s->detect_table == DETECT_TABLE_PACKET_PRE_STREAM) { + drop_reason = PKT_DROP_REASON_STREAM_PRE_HOOK; + } else if (s->detect_table == DETECT_TABLE_PACKET_PRE_FLOW) { + drop_reason = PKT_DROP_REASON_FLOW_PRE_HOOK; } /* PacketDrop will update the packet action, too */ @@ -336,11 +334,11 @@ void AlertQueueAppend(DetectEngineThreadCtx *det_ctx, const Signature *s, Packet * The Signature::num field is set based on internal priority. Higher priority * rules have lower nums. */ -static int AlertQueueSortHelper(const void *a, const void *b) +static int AlertQueueSortHelperFirewall(const void *a, const void *b) { const PacketAlert *pa0 = a; const PacketAlert *pa1 = b; - if (pa0->s->firewall_table == pa1->s->firewall_table) { + if (pa0->s->detect_table == pa1->s->detect_table) { if (pa1->iid == pa0->iid) { if (pa1->tx_id == PACKET_ALERT_NOTX) { return -1; @@ -352,7 +350,23 @@ static int AlertQueueSortHelper(const void *a, const void *b) return pa0->iid < pa1->iid ? -1 : 1; } } - return pa0->s->firewall_table < pa1->s->firewall_table ? -1 : 1; + return pa0->s->detect_table < pa1->s->detect_table ? -1 : 1; +} + +static int AlertQueueSortHelper(const void *a, const void *b) +{ + const PacketAlert *pa0 = a; + const PacketAlert *pa1 = b; + if (pa1->iid == pa0->iid) { + if (pa1->tx_id == PACKET_ALERT_NOTX) { + return -1; + } else if (pa0->tx_id == PACKET_ALERT_NOTX) { + return 1; + } + return pa0->tx_id < pa1->tx_id ? 1 : -1; + } else { + return pa0->iid < pa1->iid ? -1 : 1; + } } /** \internal @@ -400,7 +414,8 @@ static inline void PacketAlertFinalizeProcessQueue( if (det_ctx->alert_queue_size > 1) { /* sort the alert queue before thresholding and appending to Packet */ qsort(det_ctx->alert_queue, det_ctx->alert_queue_size, sizeof(PacketAlert), - AlertQueueSortHelper); + (de_ctx->flags & DE_HAS_FIREWALL) ? AlertQueueSortHelperFirewall + : AlertQueueSortHelper); } bool dropped = false; diff --git a/src/detect-engine.c b/src/detect-engine.c index 999c96c002..42b28f711a 100644 --- a/src/detect-engine.c +++ b/src/detect-engine.c @@ -126,22 +126,22 @@ const struct SignatureProperties signature_properties[SIG_TYPE_MAX] = { // rule types documentation tag end: SignatureProperties // clang-format on -const char *DetectTableToString(enum FirewallTable table) +const char *DetectTableToString(enum DetectTable table) { switch (table) { - case FIREWALL_TABLE_NOT_SET: + case DETECT_TABLE_NOT_SET: return "not_set"; - case FIREWALL_TABLE_PACKET_PRE_FLOW: + case DETECT_TABLE_PACKET_PRE_FLOW: return "pre_flow"; - case FIREWALL_TABLE_PACKET_PRE_STREAM: + case DETECT_TABLE_PACKET_PRE_STREAM: return "pre_stream"; - case FIREWALL_TABLE_PACKET_FILTER: + case DETECT_TABLE_PACKET_FILTER: return "packet_filter"; - case FIREWALL_TABLE_PACKET_TD: + case DETECT_TABLE_PACKET_TD: return "packet_td"; - case FIREWALL_TABLE_APP_FILTER: + case DETECT_TABLE_APP_FILTER: return "app_filter"; - case FIREWALL_TABLE_APP_TD: + case DETECT_TABLE_APP_TD: return "app_td"; default: return "unknown"; diff --git a/src/detect-engine.h b/src/detect-engine.h index 4d22b8075b..f6b233ba03 100644 --- a/src/detect-engine.h +++ b/src/detect-engine.h @@ -27,7 +27,7 @@ #include "detect.h" #include "suricata.h" -const char *DetectTableToString(enum FirewallTable table); +const char *DetectTableToString(enum DetectTable table); /* start up registry funcs */ diff --git a/src/detect-parse.c b/src/detect-parse.c index bb304e2f7a..47bd49d563 100644 --- a/src/detect-parse.c +++ b/src/detect-parse.c @@ -2460,16 +2460,16 @@ static void SigSetupPrefilter(DetectEngineCtx *de_ctx, Signature *s) */ static bool DetectRuleValidateTable(const Signature *s) { - if (s->firewall_table == 0) + if (s->detect_table == 0) return true; - const uint8_t table_as_flag = BIT_U8(s->firewall_table); + const uint8_t table_as_flag = BIT_U8(s->detect_table); for (SigMatch *sm = s->init_data->smlists[DETECT_SM_LIST_MATCH]; sm != NULL; sm = sm->next) { const uint8_t kw_tables_supported = sigmatch_table[sm->type].tables; if (kw_tables_supported != 0 && (kw_tables_supported & table_as_flag) == 0) { SCLogError("rule %u uses hook \"%s\", but keyword \"%s\" doesn't support this hook", - s->id, DetectTableToString(s->firewall_table), sigmatch_table[sm->type].name); + s->id, DetectTableToString(s->detect_table), sigmatch_table[sm->type].name); return false; } } @@ -2487,33 +2487,34 @@ static bool DetectFirewallRuleValidate(const DetectEngineCtx *de_ctx, const Sign return true; } -static void DetectFirewallRuleSetTable(Signature *s) +static void DetectRuleSetTable(Signature *s) { - enum FirewallTable table; + enum DetectTable table; if (s->flags & SIG_FLAG_FIREWALL) { if (s->type == SIG_TYPE_PKT) { if (s->init_data->hook.type == SIGNATURE_HOOK_TYPE_PKT && s->init_data->hook.t.pkt.ph == SIGNATURE_HOOK_PKT_PRE_STREAM) - table = FIREWALL_TABLE_PACKET_PRE_STREAM; + table = DETECT_TABLE_PACKET_PRE_STREAM; else if (s->init_data->hook.type == SIGNATURE_HOOK_TYPE_PKT && s->init_data->hook.t.pkt.ph == SIGNATURE_HOOK_PKT_PRE_FLOW) - table = FIREWALL_TABLE_PACKET_PRE_FLOW; + table = DETECT_TABLE_PACKET_PRE_FLOW; else - table = FIREWALL_TABLE_PACKET_FILTER; + table = DETECT_TABLE_PACKET_FILTER; } else if (s->type == SIG_TYPE_APP_TX) { - table = FIREWALL_TABLE_APP_FILTER; + table = DETECT_TABLE_APP_FILTER; } else { BUG_ON(1); } } else { + // TODO pre_flow/pre_stream if (s->type != SIG_TYPE_APP_TX) { - table = FIREWALL_TABLE_PACKET_TD; + table = DETECT_TABLE_PACKET_TD; } else { - table = FIREWALL_TABLE_APP_TD; + table = DETECT_TABLE_APP_TD; } } - s->firewall_table = (uint8_t)table; + s->detect_table = (uint8_t)table; } static int SigValidateFirewall(const DetectEngineCtx *de_ctx, const Signature *s) @@ -2829,9 +2830,7 @@ static int SigValidateConsolidate( SigConsolidateTcpBuffer(s); SignatureSetType(de_ctx, s); - if (de_ctx->flags & DE_HAS_FIREWALL) { - DetectFirewallRuleSetTable(s); - } + DetectRuleSetTable(s); int r = SigValidateFileHandling(s); if (r == 0) { diff --git a/src/detect.h b/src/detect.h index d6370ca603..dca1a0e16f 100644 --- a/src/detect.h +++ b/src/detect.h @@ -550,22 +550,21 @@ enum SignatureHookType { SIGNATURE_HOOK_TYPE_APP, }; -// TODO should probably be renamed to DetectTable, similar for values -enum FirewallTable { - FIREWALL_TABLE_NOT_SET = 0, - FIREWALL_TABLE_PACKET_PRE_FLOW, - FIREWALL_TABLE_PACKET_PRE_STREAM, - FIREWALL_TABLE_PACKET_FILTER, - FIREWALL_TABLE_PACKET_TD, - FIREWALL_TABLE_APP_FILTER, - FIREWALL_TABLE_APP_TD, - -#define DETECT_TABLE_PACKET_PRE_FLOW_FLAG BIT_U8(FIREWALL_TABLE_PACKET_PRE_FLOW) -#define DETECT_TABLE_PACKET_PRE_STREAM_FLAG BIT_U8(FIREWALL_TABLE_PACKET_PRE_STREAM) -#define DETECT_TABLE_PACKET_FILTER_FLAG BIT_U8(FIREWALL_TABLE_PACKET_FILTER) -#define DETECT_TABLE_PACKET_TD_FLAG BIT_U8(FIREWALL_TABLE_PACKET_TD) -#define DETECT_TABLE_APP_FILTER_FLAG BIT_U8(FIREWALL_TABLE_APP_FILTER) -#define DETECT_TABLE_APP_TD_FLAG BIT_U8(FIREWALL_TABLE_APP_TD) +enum DetectTable { + DETECT_TABLE_NOT_SET = 0, + DETECT_TABLE_PACKET_PRE_FLOW, + DETECT_TABLE_PACKET_PRE_STREAM, + DETECT_TABLE_PACKET_FILTER, + DETECT_TABLE_PACKET_TD, + DETECT_TABLE_APP_FILTER, + DETECT_TABLE_APP_TD, + +#define DETECT_TABLE_PACKET_PRE_FLOW_FLAG BIT_U8(DETECT_TABLE_PACKET_PRE_FLOW) +#define DETECT_TABLE_PACKET_PRE_STREAM_FLAG BIT_U8(DETECT_TABLE_PACKET_PRE_STREAM) +#define DETECT_TABLE_PACKET_FILTER_FLAG BIT_U8(DETECT_TABLE_PACKET_FILTER) +#define DETECT_TABLE_PACKET_TD_FLAG BIT_U8(DETECT_TABLE_PACKET_TD) +#define DETECT_TABLE_APP_FILTER_FLAG BIT_U8(DETECT_TABLE_APP_FILTER) +#define DETECT_TABLE_APP_TD_FLAG BIT_U8(DETECT_TABLE_APP_TD) }; // dns:request_complete should add DetectBufferTypeGetByName("dns:request_complete"); @@ -700,8 +699,8 @@ typedef struct Signature_ { /** classification id **/ uint16_t class_id; - /** firewall: pseudo table this rule is part of (enum FirewallTable) */ - uint8_t firewall_table; + /** detect: pseudo table this rule is part of (enum DetectTable) */ + uint8_t detect_table; /** firewall: progress value for this signature */ uint8_t app_progress_hook;