From: Victor Julien Date: Thu, 24 Nov 2022 20:35:30 +0000 (+0100) Subject: detect: fixes to action handling; fix PASS X-Git-Tag: suricata-6.0.9~19 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=82c24bf7384bfa8bbba7e3b3bb37cf6c45a5f316;p=thirdparty%2Fsuricata.git 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 e141acb9f8..ded7248578 100644 --- 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 760a449d52..04c5f3e567 100644 --- 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 16e77b2b0d..5d1e77be71 100644 --- 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 0bf565c0d0..3f708eb188 100644 --- 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 86bc8d4a28..94d7be0da8 100644 --- 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 73ab59a2a0..6a0f8e3728 100644 --- 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 e400da5849..23c88852e1 100644 --- 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 b8361c6328..6d19a423ec 100644 --- 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; } }