detect: fixes to action handling; fix PASS

Fix PASS handling by setting and checking in the correct packet.

There are 3 types of packets:
1. tunnel packets (inner layer of encapsulation)
2. "root" packets (outmost layer of encapsulated packet)
3. normal packets (no encapsulation)

Tunnel packet have a pointer to their "root". The "root" is the packet
that is ultimately used by the capture method to issue a verdict:
DROP or ACCEPT (forward).

For tunnels:
DROP actions are always issued on the root packet.
The PASS action is issued on the packet currently in the detection
engine.

Non-tunnels:
DROP and PASS are both set in the current packet.

Bug: #5697.

diff --git a/src/decode.h b/src/decode.h
index e141acb9f8880e32a97d3ba16d735d1abae63562..ded72485787bb20fdd87b4cda583747de19f3e11 100644 (file)
--- a/src/decode.h
+++ b/src/decode.h
@@ -887,27 +887,17 @@ void CaptureStatsSetup(ThreadVars *tv, CaptureStats *s);
  * handle the case of a root packet
  * for tunnels */
 
-#define PACKET_SET_ACTION(p, a) do { \
-    ((p)->root ? \
-     ((p)->root->action = a) : \
-     ((p)->action = a)); \
-} while (0)
-
-#define PACKET_ALERT(p) PACKET_SET_ACTION(p, ACTION_ALERT)
-
-#define PACKET_ACCEPT(p) PACKET_SET_ACTION(p, ACTION_ACCEPT)
-
-#define PACKET_PASS(p) PACKET_SET_ACTION(p, ACTION_PASS)
-
-#define PACKET_TEST_ACTION_DO(p, a) (p)->action &(a)
+static inline void PacketSetActionOnCurrentPkt(Packet *p, const uint8_t action)
+{
+    p->action |= action;
+}
 
-#define PACKET_UPDATE_ACTION(p, a) (p)->action |= (a)
-static inline void PacketUpdateAction(Packet *p, const uint8_t a)
+static inline void PacketSetActionOnRealPkt(Packet *p, const uint8_t action)
 {
     if (likely(p->root == NULL)) {
-        PACKET_UPDATE_ACTION(p, a);
+        p->action |= action;
     } else {
-        PACKET_UPDATE_ACTION(p->root, a);
+        p->root->action |= action;
     }
 }
 
@@ -916,19 +906,29 @@ static inline void PacketDrop(Packet *p, const uint8_t action, enum PacketDropRe
     if (p->drop_reason == PKT_DROP_REASON_NOT_SET)
         p->drop_reason = (uint8_t)r;
 
-    PacketUpdateAction(p, action);
+    if (likely(p->root == NULL)) {
+        p->action |= action;
+    } else {
+        p->root->action |= action;
+    }
+}
+
+static inline uint8_t PacketTestActionOnCurrentPkt(const Packet *p, const uint8_t a)
+{
+    return (p->action & a);
 }
-#define PACKET_DROP(p) PacketDrop((p), PKT_DROP_REASON_NOT_SET)
 
-static inline uint8_t PacketTestAction(const Packet *p, const uint8_t a)
+static inline uint8_t PacketTestActionOnRealPkt(const Packet *p, const uint8_t a)
 {
     if (likely(p->root == NULL)) {
-        return PACKET_TEST_ACTION_DO(p, a);
+        return (p->action & a);
     } else {
-        return PACKET_TEST_ACTION_DO(p->root, a);
+        return (p->root->action & a);
     }
 }
-#define PACKET_TEST_ACTION(p, a) PacketTestAction((p), (a))
+
+// Tests on "real" packets. Should only be used to check for ACTION_DROP|ACTION_REJECT*
+#define PACKET_TEST_ACTION(p, a) PacketTestActionOnRealPkt((p), (a))
 
 #define TUNNEL_INCR_PKT_RTV_NOLOCK(p) do {                                          \
         ((p)->root ? (p)->root->tunnel_rtv_cnt++ : (p)->tunnel_rtv_cnt++);          \

diff --git a/src/detect-engine-alert.c b/src/detect-engine-alert.c
index 760a449d520fc210b0abd3844dfa89f363f397da..04c5f3e5679364c66aae8bcaf5d4c6d6b421cd6b 100644 (file)
--- a/src/detect-engine-alert.c
+++ b/src/detect-engine-alert.c
@@ -179,32 +179,32 @@ static inline void RuleActionToFlow(const uint8_t action, Flow *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)
+static void PacketApplySignatureActions(Packet *p, const Signature *s, const PacketAlert *pa)
 {
     SCLogDebug("packet %" PRIu64 " sid %u action %02x alert_flags %02x", p->pcap_cnt, s->id,
-            s->action, alert_flags);
+            s->action, pa->flags);
 
     /* REJECT also sets ACTION_DROP, just make it more visible with this check */
-    if (s->action & (ACTION_DROP | ACTION_REJECT_ANY)) {
+    if (pa->action & (ACTION_DROP | ACTION_REJECT_ANY)) {
         /* PacketDrop will update the packet action, too */
-        PacketDrop(p, s->action, PKT_DROP_REASON_RULES);
+        PacketDrop(p, pa->action, PKT_DROP_REASON_RULES);
 
         if (p->alerts.drop.action == 0) {
             p->alerts.drop.num = s->num;
-            p->alerts.drop.action = s->action;
+            p->alerts.drop.action = pa->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);
+        if ((p->flow != NULL) && (pa->flags & PACKET_ALERT_FLAG_APPLY_ACTION_TO_FLOW)) {
+            RuleActionToFlow(pa->action, p->flow);
         }
 
-        DEBUG_VALIDATE_BUG_ON(!PacketTestAction(p, ACTION_DROP));
+        DEBUG_VALIDATE_BUG_ON(!PacketTestActionOnRealPkt(p, ACTION_DROP));
     } else {
-        PACKET_UPDATE_ACTION(p, s->action);
+        p->action |= pa->action;
 
-        if ((s->action & ACTION_PASS) && (p->flow != NULL) &&
-                (alert_flags & PACKET_ALERT_FLAG_APPLY_ACTION_TO_FLOW)) {
-            RuleActionToFlow(s->action, p->flow);
+        if ((pa->action & ACTION_PASS) && (p->flow != NULL) &&
+                (pa->flags & PACKET_ALERT_FLAG_APPLY_ACTION_TO_FLOW)) {
+            RuleActionToFlow(pa->action, p->flow);
         }
     }
 }
@@ -380,7 +380,7 @@ void PacketAlertFinalize(DetectEngineCtx *de_ctx, DetectEngineThreadCtx *det_ctx
                     p, &det_ctx->alert_queue[i], s, det_ctx->alert_queue[i].flags);
 
             /* set actions on packet */
-            PacketApplySignatureActions(p, s, det_ctx->alert_queue[i].flags);
+            PacketApplySignatureActions(p, s, &det_ctx->alert_queue[i]);
         }
 
         /* Thresholding removes this alert */
@@ -391,7 +391,7 @@ void PacketAlertFinalize(DetectEngineCtx *de_ctx, DetectEngineThreadCtx *det_ctx
             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 (PACKET_TEST_ACTION(p, ACTION_PASS)) {
+            if (p->alerts.alerts[p->alerts.cnt].action & ACTION_PASS) {
                 /* Ok, reset the alert cnt to end in the previous of pass
                  * so we ignore the rest with less prio */
                 break;

diff --git a/src/detect-engine-threshold.c b/src/detect-engine-threshold.c
index 16e77b2b0dbff44bc2df33ee4ecd77e16557ceda..5d1e77be71221fbd64849902ea6720b5960e0e70 100644 (file)
--- a/src/detect-engine-threshold.c
+++ b/src/detect-engine-threshold.c
@@ -296,20 +296,21 @@ static inline void RateFilterSetAction(Packet *p, PacketAlert *pa, uint8_t new_a
 {
     switch (new_action) {
         case TH_ACTION_ALERT:
-            PACKET_ALERT(p);
             pa->flags |= PACKET_ALERT_RATE_FILTER_MODIFIED;
+            pa->action = ACTION_ALERT;
             break;
         case TH_ACTION_DROP:
-            PacketDrop(p, ACTION_DROP, PKT_DROP_REASON_RULES_THRESHOLD);
             pa->flags |= PACKET_ALERT_RATE_FILTER_MODIFIED;
+            pa->action = ACTION_DROP;
+            SCLogNotice("DROP DROP DROP DROP DROP DROP");
             break;
         case TH_ACTION_REJECT:
-            PacketDrop(p, (ACTION_REJECT | ACTION_DROP), PKT_DROP_REASON_RULES_THRESHOLD);
             pa->flags |= PACKET_ALERT_RATE_FILTER_MODIFIED;
+            pa->action = (ACTION_REJECT | ACTION_DROP);
             break;
         case TH_ACTION_PASS:
-            PACKET_PASS(p);
             pa->flags |= PACKET_ALERT_RATE_FILTER_MODIFIED;
+            pa->action = ACTION_PASS;
             break;
         default:
             /* Weird, leave the default action */

diff --git a/src/source-nfq.c b/src/source-nfq.c
index 0bf565c0d0fdda5d223280f97e2750a632fe6229..3f708eb1888cddde2d2a04d96e9d91bf48c5b3d1 100644 (file)
--- a/src/source-nfq.c
+++ b/src/source-nfq.c
@@ -475,7 +475,7 @@ static int NFQSetupPkt (Packet *p, struct nfq_q_handle *qh, void *data)
 static void NFQReleasePacket(Packet *p)
 {
     if (unlikely(!p->nfq_v.verdicted)) {
-        PACKET_UPDATE_ACTION(p, ACTION_DROP);
+        p->action |= ACTION_DROP;
         NFQSetVerdict(p);
     }
     PacketFreeOrRelease(p);

diff --git a/src/tests/detect-engine-alert.c b/src/tests/detect-engine-alert.c
index 86bc8d4a28585a796807845e76263eb48a7f08dd..94d7be0da8a9b3c5e4098708a92ad8c1595afacb 100644 (file)
--- a/src/tests/detect-engine-alert.c
+++ b/src/tests/detect-engine-alert.c
@@ -39,7 +39,7 @@ static int TestDetectAlertPacketApplySignatureActions01(void)
 
     const char sig[] = "reject tcp any any -> any 80 (content:\"Hi all\"; sid:1; rev:1;)";
     FAIL_IF(UTHPacketMatchSig(p, sig) == 0);
-    FAIL_IF_NOT(PacketTestAction(p, ACTION_REJECT_ANY));
+    FAIL_IF_NOT(PacketTestActionOnRealPkt(p, ACTION_REJECT_ANY));
 
     UTHFreePackets(&p, 1);
 #endif /* HAVE_LIBNET11 */
@@ -59,7 +59,7 @@ static int TestDetectAlertPacketApplySignatureActions02(void)
 
     const char sig[] = "drop tcp any any -> any any (msg:\"sig 1\"; content:\"Hi all\"; sid:1;)";
     FAIL_IF(UTHPacketMatchSig(p, sig) == 0);
-    FAIL_IF_NOT(PacketTestAction(p, ACTION_DROP));
+    FAIL_IF_NOT(PacketTestActionOnRealPkt(p, ACTION_DROP));
 
     UTHFreePackets(&p, 1);
     PASS;

diff --git a/src/tests/detect.c b/src/tests/detect.c
index 73ab59a2a0613b026a1765a655d59cadd4088b72..6a0f8e3728227519e73c57f6f5c18d587184b72c 100644 (file)
--- a/src/tests/detect.c
+++ b/src/tests/detect.c
@@ -4877,7 +4877,7 @@ static int SigTestDropFlow04(void)
     FAIL_IF(PacketAlertCheck(p1, 2));
 
     FAIL_IF_NOT(p1->flow->flags & FLOW_ACTION_DROP);
-    FAIL_IF_NOT(PacketTestAction(p1, ACTION_DROP));
+    FAIL_IF_NOT(PacketTestActionOnRealPkt(p1, ACTION_DROP));
 
     FAIL_IF(p2->flags & PKT_NOPACKET_INSPECTION);
 
@@ -4890,7 +4890,7 @@ static int SigTestDropFlow04(void)
 
     FAIL_IF(PacketAlertCheck(p2, 1));
     FAIL_IF(PacketAlertCheck(p2, 2));
-    FAIL_IF_NOT(PacketTestAction(p2, ACTION_DROP));
+    FAIL_IF_NOT(PacketTestActionOnRealPkt(p2, ACTION_DROP));
 
     AppLayerParserThreadCtxFree(alp_tctx);
     DetectEngineThreadCtxDeinit(&tv, det_ctx);

diff --git a/src/tmqh-packetpool.c b/src/tmqh-packetpool.c
index e400da5849a0fac22be8b1b29b8e4329b64a2eb5..23c88852e1298f9bdadfa8594e65b91d09a0e96b 100644 (file)
--- a/src/tmqh-packetpool.c
+++ b/src/tmqh-packetpool.c
@@ -451,11 +451,6 @@ void TmqhOutputPacketpool(ThreadVars *t, Packet *p)
         SCLogDebug("tunnel stuff done, move on (proot %d)", proot);
     }
 
-    /* Check that the drop reason has been set, if we have a drop */
-    if (PacketTestAction(p, ACTION_DROP)) {
-        DEBUG_VALIDATE_BUG_ON((p)->drop_reason == PKT_DROP_REASON_NOT_SET);
-    }
-
     /* we're done with the tunnel root now as well */
     if (proot == true) {
         SCLogDebug("getting rid of root pkt... alloc'd %s", p->root->flags & PKT_ALLOC ? "true" : "false");

diff --git a/src/util-exception-policy.c b/src/util-exception-policy.c
index b8361c63283db2112d28781a3cece80918985747..6d19a423ec6c12189cd9e25623879c5a09861fa4 100644 (file)
--- a/src/util-exception-policy.c
+++ b/src/util-exception-policy.c
@@ -64,7 +64,7 @@ void ExceptionPolicyApply(Packet *p, enum ExceptionPolicy policy, enum PacketDro
                 SCLogDebug("EXCEPTION_POLICY_PASS_PACKET");
                 DecodeSetNoPayloadInspectionFlag(p);
                 DecodeSetNoPacketInspectionFlag(p);
-                PACKET_PASS(p);
+                PacketSetActionOnCurrentPkt(p, ACTION_PASS);
                 break;
         }
     }