detect: unify alert handling; fix bugs

Unify handling of signature matches between various rule types and
between noalert and regular rules.

"noalert" sigs are added to the alert queue initially, but removed
from it after handling their actions. This way all actions are applied
from a single place.

Make sure flow drop and pass are mutually exclusive.

The above addresses issue with pass and drops not getting applied
correctly in various cases.

Bug: #4663
Bug: #4670

(cherry picked from commit aa93984b7e58d3d8c1323f86bdaff937f8b8bd1e)

diff --git a/src/decode.h b/src/decode.h
index 01a259637dfca4af7151a2502f60efa309b4d00c..ddb4a105f2552712163db4fabc89c5d003ea551a 100644 (file)
--- a/src/decode.h
+++ b/src/decode.h
@@ -277,10 +277,8 @@ typedef struct PacketAlert_ {
     uint64_t tx_id;
 } PacketAlert;
 
-/** After processing an alert by the thresholding module, if at
- *  last it gets triggered, we might want to stick the drop action to
- *  the flow on IPS mode */
-#define PACKET_ALERT_FLAG_DROP_FLOW     0x01
+/* flag to indicate the rule action (drop/pass) needs to be applied to the flow */
+#define PACKET_ALERT_FLAG_APPLY_ACTION_TO_FLOW 0x1
 /** alert was generated based on state */
 #define PACKET_ALERT_FLAG_STATE_MATCH   0x02
 /** alert was generated based on stream */

diff --git a/src/detect-engine-alert.c b/src/detect-engine-alert.c
index f570fe94f90d2e9ebc07ed1df96c81b20c2df57d..87315cf37ac3513247d3cbb37782ed0480450290 100644 (file)
--- a/src/detect-engine-alert.c
+++ b/src/detect-engine-alert.c
@@ -228,14 +228,47 @@ int PacketAlertAppend(DetectEngineThreadCtx *det_ctx, const Signature *s,
 
 static inline void RuleActionToFlow(const uint8_t action, Flow *f)
 {
-    if (action & ACTION_DROP)
-        f->flags |= FLOW_ACTION_DROP;
-
-    if (action & ACTION_REJECT_ANY)
-        f->flags |= FLOW_ACTION_DROP;
+    if (action & (ACTION_DROP | ACTION_REJECT_ANY | ACTION_PASS)) {
+        if (f->flags & (FLOW_ACTION_DROP | FLOW_ACTION_PASS)) {
+            /* drop or pass already set. First to set wins. */
+            SCLogDebug("not setting %s flow already set to %s",
+                    (action & ACTION_PASS) ? "pass" : "drop",
+                    (f->flags & FLOW_ACTION_DROP) ? "drop" : "pass");
+        } else {
+            if (action & (ACTION_DROP | ACTION_REJECT_ANY)) {
+                f->flags |= FLOW_ACTION_DROP;
+                SCLogDebug("setting flow action drop");
+            }
+            if (action & ACTION_PASS) {
+                f->flags |= FLOW_ACTION_PASS;
+                SCLogDebug("setting flow action pass");
+                FlowSetNoPacketInspectionFlag(f);
+            }
+        }
+    }
+}
 
-    if (action & ACTION_PASS) {
-        FlowSetNoPacketInspectionFlag(f);
+/** \brief Apply action(s) and Set 'drop' sig info,
+ *         if applicable */
+static void PacketApplySignatureActions(Packet *p, const Signature *s, const uint8_t alert_flags)
+{
+    SCLogDebug("packet %" PRIu64 " sid %u action %02x alert_flags %02x", p->pcap_cnt, s->id,
+            s->action, alert_flags);
+    PACKET_UPDATE_ACTION(p, s->action);
+
+    if (s->action & ACTION_DROP) {
+        if (p->alerts.drop.action == 0) {
+            p->alerts.drop.num = s->num;
+            p->alerts.drop.action = s->action;
+            p->alerts.drop.s = (Signature *)s;
+        }
+        if ((p->flow != NULL) && (alert_flags & PACKET_ALERT_FLAG_APPLY_ACTION_TO_FLOW)) {
+            RuleActionToFlow(s->action, p->flow);
+        }
+    } else if (s->action & ACTION_PASS) {
+        if ((p->flow != NULL) && (alert_flags & PACKET_ALERT_FLAG_APPLY_ACTION_TO_FLOW)) {
+            RuleActionToFlow(s->action, p->flow);
+        }
     }
 }
 
@@ -254,8 +287,8 @@ void PacketAlertFinalize(DetectEngineCtx *de_ctx, DetectEngineThreadCtx *det_ctx
     int i = 0;
 
     while (i < p->alerts.cnt) {
-        SCLogDebug("Sig->num: %"PRIu32, p->alerts.alerts[i].num);
         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);
 
         int res = PacketAlertHandle(de_ctx, det_ctx, s, p, &p->alerts.alerts[i]);
         if (res > 0) {
@@ -275,36 +308,34 @@ void PacketAlertFinalize(DetectEngineCtx *de_ctx, DetectEngineThreadCtx *det_ctx
                 }
             }
 
-            /* IP-only and PD-only matches should apply to the flow */
-            if (s->flags & (SIG_FLAG_IPONLY | SIG_FLAG_PDONLY)) {
-                if (p->flow != NULL) {
-                    RuleActionToFlow(s->action, p->flow);
+            /* For DROP and PASS sigs we need to apply the action to the flow if
+             * - sig is IP or PD only
+             * - match is in applayer
+             * - match is in stream */
+            if (s->action & (ACTION_DROP | ACTION_PASS)) {
+                if ((p->alerts.alerts[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;
+                    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);
                 }
             }
 
             /* set actions on packet */
-            DetectSignatureApplyActions(p, p->alerts.alerts[i].s, p->alerts.alerts[i].flags);
+            PacketApplySignatureActions(p, p->alerts.alerts[i].s, p->alerts.alerts[i].flags);
 
             if (PACKET_TEST_ACTION(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;
-
-            /* if the signature wants to drop, check if the
-             * PACKET_ALERT_FLAG_DROP_FLOW flag is set. */
-            } else if ((PACKET_TEST_ACTION(p, ACTION_DROP)) &&
-                    ((p->alerts.alerts[i].flags & PACKET_ALERT_FLAG_DROP_FLOW) ||
-                         (s->flags & SIG_FLAG_APPLAYER))
-                       && p->flow != NULL)
-            {
-                /* This will apply only on IPS mode (check StreamTcpPacket) */
-                p->flow->flags |= FLOW_ACTION_DROP; // XXX API?
             }
         }
 
         /* Thresholding removes this alert */
-        if (res == 0 || res == 2) {
+        if (res == 0 || res == 2 || (s->flags & SIG_FLAG_NOALERT)) {
             PacketAlertRemove(p, i);
 
             if (p->alerts.cnt == 0)

diff --git a/src/detect-engine-iponly.c b/src/detect-engine-iponly.c
index 5ce360688d67032eb4dc0af8ca03bb1c1d96678b..782a4c8f1b54419dd9ef01f6332adaf03cc7b982 100644 (file)
--- a/src/detect-engine-iponly.c
+++ b/src/detect-engine-iponly.c
@@ -1111,15 +1111,7 @@ void IPOnlyMatchPacket(ThreadVars *tv,
                             }
                         }
                     }
-                    if (!(s->flags & SIG_FLAG_NOALERT)) {
-                        if (s->action & ACTION_DROP)
-                            PacketAlertAppend(det_ctx, s, p, 0, PACKET_ALERT_FLAG_DROP_FLOW);
-                        else
-                            PacketAlertAppend(det_ctx, s, p, 0, 0);
-                    } else {
-                        /* apply actions for noalert/rule suppressed as well */
-                        DetectSignatureApplyActions(p, s, 0);
-                    }
+                    PacketAlertAppend(det_ctx, s, p, 0, 0);
                 }
             }
         }

diff --git a/src/detect-engine.c b/src/detect-engine.c
index e91c9924713491d5cd31c10d6104aaa54d18b173..9877c6c906b2f420bd9e218cb8baf9c6739b98fa 100644 (file)
--- a/src/detect-engine.c
+++ b/src/detect-engine.c
@@ -1456,11 +1456,6 @@ static int DetectEngineInspectRulePayloadMatches(
             pmatch = DetectEngineInspectStreamPayload(de_ctx, det_ctx, s, p->flow, p);
             if (pmatch) {
                 det_ctx->flags |= DETECT_ENGINE_THREAD_CTX_STREAM_CONTENT_MATCH;
-                /* Tell the engine that this reassembled stream can drop the
-                 * rest of the pkts with no further inspection */
-                if (s->action & ACTION_DROP)
-                    *alert_flags |= PACKET_ALERT_FLAG_DROP_FLOW;
-
                 *alert_flags |= PACKET_ALERT_FLAG_STREAM_MATCH;
             }
         }

diff --git a/src/detect.c b/src/detect.c
index 3c4f3d709a02f72fca54dc0cc58213872010ad65..aeb836eace99d14a02a2aa997e5f6d8de8953f28 100644 (file)
--- a/src/detect.c
+++ b/src/detect.c
@@ -547,18 +547,12 @@ static void DetectRunInspectIPOnly(ThreadVars *tv, const DetectEngineCtx *de_ctx
 
             /* save in the flow that we scanned this direction... */
             FlowSetIPOnlyFlag(pflow, p->flowflags & FLOW_PKT_TOSERVER ? 1 : 0);
-
-        } else if (((p->flowflags & FLOW_PKT_TOSERVER) &&
-                   (pflow->flags & FLOW_TOSERVER_IPONLY_SET)) ||
-                   ((p->flowflags & FLOW_PKT_TOCLIENT) &&
-                   (pflow->flags & FLOW_TOCLIENT_IPONLY_SET)))
-        {
-            /* If we have a drop from IP only module,
-             * we will drop the rest of the flow packets
-             * This will apply only to inline/IPS */
-            if (pflow->flags & FLOW_ACTION_DROP) {
-                PACKET_DROP(p);
-            }
+        }
+        /* If we have a drop from IP only module,
+         * we will drop the rest of the flow packets
+         * This will apply only to inline/IPS */
+        if (pflow->flags & FLOW_ACTION_DROP) {
+            PACKET_DROP(p);
         }
     } else { /* p->flags & PKT_HAS_FLOW */
         /* no flow */
@@ -804,12 +798,7 @@ static inline void DetectRulePacketRules(
 #endif
         DetectRunPostMatch(tv, det_ctx, p, s);
 
-        if (!(sflags & SIG_FLAG_NOALERT)) {
-            PacketAlertAppend(det_ctx, s, p, 0, alert_flags);
-        } else {
-            /* apply actions even if not alerting */
-            DetectSignatureApplyActions(p, s, alert_flags);
-        }
+        PacketAlertAppend(det_ctx, s, p, 0, alert_flags);
 next:
         DetectVarProcessList(det_ctx, pflow, p);
         DetectReplaceFree(det_ctx);
@@ -1462,16 +1451,9 @@ static void DetectRunTx(ThreadVars *tv,
                 /* match */
                 DetectRunPostMatch(tv, det_ctx, p, s);
 
-                uint8_t alert_flags = (PACKET_ALERT_FLAG_STATE_MATCH|PACKET_ALERT_FLAG_TX);
-                if (s->action & ACTION_DROP)
-                    alert_flags |= PACKET_ALERT_FLAG_DROP_FLOW;
-
+                const uint8_t alert_flags = (PACKET_ALERT_FLAG_STATE_MATCH | PACKET_ALERT_FLAG_TX);
                 SCLogDebug("%p/%"PRIu64" sig %u (%u) matched", tx.tx_ptr, tx.tx_id, s->id, s->num);
-                if (!(s->flags & SIG_FLAG_NOALERT)) {
-                    PacketAlertAppend(det_ctx, s, p, tx.tx_id, alert_flags);
-                } else {
-                    DetectSignatureApplyActions(p, s, alert_flags);
-                }
+                PacketAlertAppend(det_ctx, s, p, tx.tx_id, alert_flags);
             }
             DetectVarProcessList(det_ctx, p->flow, p);
             RULE_PROFILING_END(det_ctx, s, r, p);
@@ -1517,30 +1499,6 @@ next:
     }
 }
 
-/** \brief Apply action(s) and Set 'drop' sig info,
- *         if applicable */
-void DetectSignatureApplyActions(Packet *p,
-        const Signature *s, const uint8_t alert_flags)
-{
-    PACKET_UPDATE_ACTION(p, s->action);
-
-    if (s->action & ACTION_DROP) {
-        if (p->alerts.drop.action == 0) {
-            p->alerts.drop.num = s->num;
-            p->alerts.drop.action = s->action;
-            p->alerts.drop.s = (Signature *)s;
-        }
-    } else if (s->action & ACTION_PASS) {
-        /* if an stream/app-layer match we enforce the pass for the flow */
-        if ((p->flow != NULL) &&
-                (alert_flags & (PACKET_ALERT_FLAG_STATE_MATCH|PACKET_ALERT_FLAG_STREAM_MATCH)))
-        {
-            FlowSetNoPacketInspectionFlag(p->flow);
-        }
-
-    }
-}
-
 static DetectEngineThreadCtx *GetTenantById(HashTable *h, uint32_t id)
 {
     /* technically we need to pass a DetectEngineThreadCtx struct with the

diff --git a/src/detect.h b/src/detect.h
index fa4a2116c4e66e44cb2281121c7439a6fe394515..e81f335ba071a830e2cfb4a4a7c2c9d4ef4450d3 100644 (file)
--- a/src/detect.h
+++ b/src/detect.h
@@ -1490,8 +1490,6 @@ int DetectUnregisterThreadCtxFuncs(DetectEngineCtx *, DetectEngineThreadCtx *,vo
 int DetectRegisterThreadCtxFuncs(DetectEngineCtx *, const char *name, void *(*InitFunc)(void *), void *data, void (*FreeFunc)(void *), int);
 void *DetectThreadCtxGetKeywordThreadCtx(DetectEngineThreadCtx *, int);
 
-void DetectSignatureApplyActions(Packet *p, const Signature *s, const uint8_t);
-
 void RuleMatchCandidateTxArrayInit(DetectEngineThreadCtx *det_ctx, uint32_t size);
 void RuleMatchCandidateTxArrayFree(DetectEngineThreadCtx *det_ctx);
 

diff --git a/src/flow.h b/src/flow.h
index 5cdbdbc64affbd0b4f4c232cc75d13c971f1be46..aa4b71d0ff5c6b6a53a5997d8ed7b705d9d00114 100644 (file)
--- a/src/flow.h
+++ b/src/flow.h
@@ -107,6 +107,9 @@ typedef struct AppLayerParserState_ AppLayerParserState;
 /** Indicate that the flow did trigger an expectation creation */
 #define FLOW_HAS_EXPECTATION            BIT_U32(27)
 
+/** All packets in this flow should be passed */
+#define FLOW_ACTION_PASS BIT_U32(28)
+
 /* File flags */
 
 /** no magic on files in this flow */