From: Juliana Fajardini <jufajardini@gmail.com>
Date: Tue, 19 Apr 2022 20:43:10 +0000 (-0300)
Subject: detect/alert: preprocess then append alert queue
X-Git-Tag: suricata-7.0.0-beta1~657
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=185b43edff7f3f9db0c919663eb02ceb49787a8f;p=thirdparty%2Fsuricata.git

detect/alert: preprocess then append alert queue

Do all alert queue processing before actually appending
the PacketAlerts to the Packet's alert queue.

Task #4943
---

diff --git a/src/decode.h b/src/decode.h
index 57cef10eae..a8d4075ab6 100644
--- a/src/decode.h
+++ b/src/decode.h
@@ -279,10 +279,10 @@ typedef uint16_t Port;
  * found in this packet */
 typedef struct PacketAlert_ {
     SigIntId num; /* Internal num, used for sorting */
-    uint8_t action; /* Internal num, used for sorting */
+    uint8_t action; /* Internal num, used for thresholding */
     uint8_t flags;
     const struct Signature_ *s;
-    uint64_t tx_id;
+    uint64_t tx_id; /* Used for sorting */
     int64_t frame_id;
 } PacketAlert;
 
diff --git a/src/detect-engine-alert.c b/src/detect-engine-alert.c
index 9f7db969f1..4780c48a36 100644
--- a/src/detect-engine-alert.c
+++ b/src/detect-engine-alert.c
@@ -357,6 +357,23 @@ void AlertQueueAppend(DetectEngineThreadCtx *det_ctx, const Signature *s, Packet
     return;
 }
 
+/** \internal
+ * \brief sort helper for sorting alerts by priority
+ *
+ * Sorting is done first based on num and then using tx_id, if nums are equal.
+ * 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)
+{
+    const PacketAlert *pa0 = a;
+    const PacketAlert *pa1 = b;
+    if (pa1->num == pa0->num)
+        return pa0->tx_id < pa1->tx_id ? 1 : -1;
+    else
+        return pa0->num > pa1->num ? 1 : -1;
+}
+
 /**
  * \brief Check the threshold of the sigs that match, set actions, break on pass action
  *        This function iterate the packet alerts array, removing those that didn't match
@@ -369,13 +386,18 @@ void AlertQueueAppend(DetectEngineThreadCtx *det_ctx, const Signature *s, Packet
 void PacketAlertFinalize(DetectEngineCtx *de_ctx, DetectEngineThreadCtx *det_ctx, Packet *p)
 {
     SCEnter();
+
+    /* sort the alert queue before thresholding and appending to Packet */
+    qsort(det_ctx->alert_queue, det_ctx->alert_queue_size, sizeof(PacketAlert),
+            AlertQueueSortHelper);
+
     int i = 0;
+    uint16_t max_pos = det_ctx->alert_queue_size;
 
-    while (i < p->alerts.cnt) {
-        const Signature *s = de_ctx->sig_array[p->alerts.alerts[i].num];
-        SCLogDebug("Sig->num: %" PRIu32 " SID %u", p->alerts.alerts[i].num, s->id);
+    while (i < max_pos) {
+        const Signature *s = de_ctx->sig_array[det_ctx->alert_queue[i].num];
+        uint8_t res = PacketAlertHandle(de_ctx, det_ctx, s, p, &det_ctx->alert_queue[i]);
 
-        int res = PacketAlertHandle(de_ctx, det_ctx, s, p, &p->alerts.alerts[i]);
         if (res > 0) {
             /* Now, if we have an alert, we have to check if we want
              * to tag this session or src/dst host */
@@ -398,36 +420,35 @@ void PacketAlertFinalize(DetectEngineCtx *de_ctx, DetectEngineThreadCtx *det_ctx
              * - match is in applayer
              * - match is in stream */
             if (s->action & (ACTION_DROP | ACTION_PASS)) {
-                if ((p->alerts.alerts[i].flags &
+                if ((det_ctx->alert_queue[i].flags &
                             (PACKET_ALERT_FLAG_STATE_MATCH | PACKET_ALERT_FLAG_STREAM_MATCH)) ||
                         (s->flags & (SIG_FLAG_IPONLY | SIG_FLAG_PDONLY | SIG_FLAG_APPLAYER))) {
-                    p->alerts.alerts[i].flags |= PACKET_ALERT_FLAG_APPLY_ACTION_TO_FLOW;
+                    det_ctx->alert_queue[i].flags |= PACKET_ALERT_FLAG_APPLY_ACTION_TO_FLOW;
                     SCLogDebug("packet %" PRIu64 " sid %u action %02x alert_flags %02x (set "
                                "PACKET_ALERT_FLAG_APPLY_ACTION_TO_FLOW)",
-                            p->pcap_cnt, s->id, s->action, p->alerts.alerts[i].flags);
+                            p->pcap_cnt, s->id, s->action, det_ctx->alert_queue[i].flags);
                 }
             }
 
             /* set actions on packet */
-            PacketApplySignatureActions(p, p->alerts.alerts[i].s, p->alerts.alerts[i].flags);
-
-            if (PacketTestAction(p, ACTION_PASS)) {
-                /* Ok, reset the alert cnt to end in the previous of pass
-                 * so we ignore the rest with less prio */
-                p->alerts.cnt = i;
-                break;
-            }
+            PacketApplySignatureActions(p, s, det_ctx->alert_queue[i].flags);
         }
 
         /* Thresholding removes this alert */
         if (res == 0 || res == 2 || (s->flags & SIG_FLAG_NOALERT)) {
-            PacketAlertRemove(p, i);
+            /* we will not copy this to the AlertQueue */
+        } else if (p->alerts.cnt < packet_alert_max) {
+            p->alerts.alerts[p->alerts.cnt] = det_ctx->alert_queue[i];
+            SCLogDebug("Appending sid %" PRIu32 " alert to Packet::alerts at pos %u", s->id, i);
 
-            if (p->alerts.cnt == 0)
+            if (PacketTestAction(p, ACTION_PASS)) {
+                /* Ok, reset the alert cnt to end in the previous of pass
+                 * so we ignore the rest with less prio */
                 break;
-        } else {
-            i++;
+            }
+            p->alerts.cnt++;
         }
+        i++;
     }
 
     /* At this point, we should have all the new alerts. Now check the tag