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

diff --git a/src/decode.h b/src/decode.h
index 05f981bb25f50aa9b612c5597cbab292e7fcb466..a93608351494c9d13ec217bb155bfd3c1442e975 100644 (file)
--- a/src/decode.h
+++ b/src/decode.h
@@ -282,10 +282,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 f2f4ce7ca71a2305e3f83eea3836bed9678e2763..cdfcb527ada7f1838678874fc6931a5407543dcd 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);
+    PacketUpdateAction(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,35 +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 (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;
-
-            /* if the signature wants to drop, check if the
-             * PACKET_ALERT_FLAG_DROP_FLOW flag is set. */
-            } else if ((PacketTestAction(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 521d2accf3943c8ea1f558b4d0fe28b4e800ac85..4ebc26caeed471297fd7b996f9cebf656b396838 100644 (file)
--- a/src/detect-engine-iponly.c
+++ b/src/detect-engine-iponly.c
@@ -1115,15 +1115,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 032731ef3680039463f0d38ed6ae77da4013418a..c4adc50fcc85da3261969e18086b21007f7e6787 100644 (file)
--- a/src/detect-engine.c
+++ b/src/detect-engine.c
@@ -1403,11 +1403,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 8d8c5038331f0fd65906735276f060945c0ec8dc..9bca53c9c87d662e7815e2a5861f19c757d77fe8 100644 (file)
--- a/src/detect.c
+++ b/src/detect.c
@@ -548,18 +548,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 */
@@ -800,12 +794,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);
@@ -1475,16 +1464,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);
@@ -1530,30 +1512,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)
-{
-    PacketUpdateAction(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 0ee38427579131a52513040e2ee7f9281de84366..05d3f3d29e8094b000df5e7c6b8343189457a565 100644 (file)
--- a/src/detect.h
+++ b/src/detect.h
@@ -1483,8 +1483,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 ff3022856ac7ce7b3c92ed8b960a1ddf4d2a8d33..b57c6bd857949b9feabe99ae62b249403fae99a5 100644 (file)
--- a/src/flow.h
+++ b/src/flow.h
@@ -111,6 +111,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 */
 
 #define FLOWFILE_INIT                   0