From 49051b637e89ffcc6076fc0e8388814238cc2ed3 Mon Sep 17 00:00:00 2001 From: Victor Julien Date: Wed, 31 May 2023 15:45:40 +0200 Subject: [PATCH 01/10] flow: apply flow to packet on flow lookup Issue drop to packet as early as possible. (cherry picked from commit 71a033ac62e0b71953f1884ecba7e6461c744197) --- src/flow.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/flow.c b/src/flow.c index 031728bd60e5..410109766b1e 100644 --- a/src/flow.c +++ b/src/flow.c @@ -25,6 +25,8 @@ #include "suricata-common.h" #include "suricata.h" + +#include "action-globals.h" #include "decode.h" #include "conf.h" #include "threadvars.h" @@ -473,6 +475,9 @@ void FlowHandlePacketUpdate(Flow *f, Packet *p, ThreadVars *tv, DecodeThreadVars FlowUpdateState(f, FLOW_STATE_ESTABLISHED); } + if (f->flags & FLOW_ACTION_DROP) { + PacketDrop(p, ACTION_DROP, PKT_DROP_REASON_FLOW_DROP); + } /*set the detection bypass flags*/ if (f->flags & FLOW_NOPACKET_INSPECTION) { SCLogDebug("setting FLOW_NOPACKET_INSPECTION flag on flow %p", f); From 66aed4471d0b4f401348d07593ceb0f4b6712174 Mon Sep 17 00:00:00 2001 From: Victor Julien Date: Wed, 31 May 2023 15:49:57 +0200 Subject: [PATCH 02/10] detect: update/document drop flow logic Now that flow drop is applied to packets before other processing, no drop has to be issued on a packet. (cherry picked from commit 85ddba63f64e95f4c202f8ef05e8886a0cbac725) --- src/detect.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/detect.c b/src/detect.c index 800c81927b34..724679e9fa0e 100644 --- a/src/detect.c +++ b/src/detect.c @@ -1568,7 +1568,6 @@ static void DetectFlow(ThreadVars *tv, /* if flow is set to drop, we enforce that here */ if (p->flow->flags & FLOW_ACTION_DROP) { - PacketDrop(p, ACTION_DROP, PKT_DROP_REASON_FLOW_DROP); SCReturn; } From 416cc8455fe17a1f0444bbb7f069f76748d47896 Mon Sep 17 00:00:00 2001 From: Victor Julien Date: Wed, 31 May 2023 15:52:14 +0200 Subject: [PATCH 03/10] app-layer: don't update UDP applayer for dropped packets (cherry picked from commit 77f49661fd78df420c4542e230def0682a886c60) --- src/flow-worker.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/flow-worker.c b/src/flow-worker.c index ab8a41c586c0..ab2a2a638548 100644 --- a/src/flow-worker.c +++ b/src/flow-worker.c @@ -34,6 +34,7 @@ #include "suricata-common.h" #include "suricata.h" +#include "action-globals.h" #include "decode.h" #include "detect.h" #include "stream-tcp.h" @@ -538,9 +539,11 @@ static TmEcode FlowWorker(ThreadVars *tv, Packet *p, void *data) /* handle the app layer part of the UDP packet payload */ } else if (p->flow && p->proto == IPPROTO_UDP) { - FLOWWORKER_PROFILING_START(p, PROFILE_FLOWWORKER_APPLAYERUDP); - AppLayerHandleUdp(tv, fw->stream_thread->ra_ctx->app_tctx, p, p->flow); - FLOWWORKER_PROFILING_END(p, PROFILE_FLOWWORKER_APPLAYERUDP); + if (!PACKET_TEST_ACTION(p, ACTION_DROP)) { + FLOWWORKER_PROFILING_START(p, PROFILE_FLOWWORKER_APPLAYERUDP); + AppLayerHandleUdp(tv, fw->stream_thread->ra_ctx->app_tctx, p, p->flow); + FLOWWORKER_PROFILING_END(p, PROFILE_FLOWWORKER_APPLAYERUDP); + } } PacketUpdateEngineEventCounters(tv, fw->dtv, p); From 4b9cac426a859a4129aa4b075291e73e9f72c1af Mon Sep 17 00:00:00 2001 From: Victor Julien Date: Wed, 31 May 2023 15:52:54 +0200 Subject: [PATCH 04/10] stream: simplify drop handling Remove logic to apply flow drop, as this is now handled in the flow engine. However, keep the logic that frees/cleans the session state. (cherry picked from commit d91a1e8bc6b886bdd383f3f7105ef9b2bf3a33fe) --- src/stream-tcp.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/stream-tcp.c b/src/stream-tcp.c index 6c83ffb06117..2a921b31df1a 100644 --- a/src/stream-tcp.c +++ b/src/stream-tcp.c @@ -5107,11 +5107,9 @@ int StreamTcpPacket (ThreadVars *tv, Packet *p, StreamTcpThread *stt, * applayer detection, then drop the rest of the packets of the * same stream and avoid inspecting it any further */ if (StreamTcpCheckFlowDrops(p) == 1) { - SCLogDebug("This flow/stream triggered a drop rule"); - FlowSetNoPacketInspectionFlag(p->flow); - DecodeSetNoPacketInspectionFlag(p); + DEBUG_VALIDATE_BUG_ON(!(PKT_IS_PSEUDOPKT(p)) && !PACKET_TEST_ACTION(p, ACTION_DROP)); + SCLogDebug("flow triggered a drop rule"); StreamTcpDisableAppLayer(p->flow); - PacketDrop(p, ACTION_DROP, PKT_DROP_REASON_FLOW_DROP); /* return the segments to the pool */ StreamTcpSessionPktFree(p); SCReturnInt(0); From 043bbb9f517fed4225acb639f9d356403cd441e6 Mon Sep 17 00:00:00 2001 From: Victor Julien Date: Wed, 31 May 2023 15:56:54 +0200 Subject: [PATCH 05/10] flow/timeout: no pseudo packets for dropped flows When a flow is in the drop flow state, don't use pseudo packets when it is timing out. There should be no work left to do at this point. (cherry picked from commit 2a9515471287d2b8fc5aa2e1879aabadaf5f421e) --- src/flow-manager.c | 3 ++- src/flow-worker.c | 5 +++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/flow-manager.c b/src/flow-manager.c index 44d844643910..9cfd218df703 100644 --- a/src/flow-manager.c +++ b/src/flow-manager.c @@ -279,7 +279,8 @@ static uint32_t ProcessAsideQueue(FlowManagerTimeoutThread *td, FlowTimeoutCount while ((f = FlowQueuePrivateGetFromTop(&td->aside_queue)) != NULL) { /* flow is still locked */ - if (f->proto == IPPROTO_TCP && !(f->flags & FLOW_TIMEOUT_REASSEMBLY_DONE) && + if (f->proto == IPPROTO_TCP && + !(f->flags & (FLOW_TIMEOUT_REASSEMBLY_DONE | FLOW_ACTION_DROP)) && !FlowIsBypassed(f) && FlowForceReassemblyNeedReassembly(f) == 1) { /* Send the flow to its thread */ FlowForceReassemblyForFlow(f); diff --git a/src/flow-worker.c b/src/flow-worker.c index ab2a2a638548..5d4ccd28eb0a 100644 --- a/src/flow-worker.c +++ b/src/flow-worker.c @@ -171,8 +171,9 @@ static void CheckWorkQueue(ThreadVars *tv, FlowWorkerThreadData *fw, FlowTimeout f->flow_end_flags |= FLOW_END_FLAG_TIMEOUT; //TODO emerg if (f->proto == IPPROTO_TCP) { - if (!(f->flags & FLOW_TIMEOUT_REASSEMBLY_DONE) && !FlowIsBypassed(f) && - FlowForceReassemblyNeedReassembly(f) == 1 && f->ffr != 0) { + if (!(f->flags & (FLOW_TIMEOUT_REASSEMBLY_DONE | FLOW_ACTION_DROP)) && + !FlowIsBypassed(f) && FlowForceReassemblyNeedReassembly(f) == 1 && + f->ffr != 0) { /* read detect thread in case we're doing a reload */ void *detect_thread = SC_ATOMIC_GET(fw->detect_thread); int cnt = FlowFinish(tv, f, fw, detect_thread); From 644a231e9ac2860abce0a1e3961aea0a78666256 Mon Sep 17 00:00:00 2001 From: Victor Julien Date: Thu, 1 Jun 2023 08:00:54 +0200 Subject: [PATCH 06/10] detect: fix stateful drops for rate_filter (cherry picked from commit 418cc1fe947dd96a6cadb13fa1fbb5c9d5fb7ce0) --- src/detect-engine-alert.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/detect-engine-alert.c b/src/detect-engine-alert.c index 04c5f3e56793..a40f992ced1d 100644 --- a/src/detect-engine-alert.c +++ b/src/detect-engine-alert.c @@ -322,7 +322,7 @@ static inline void FlowApplySignatureActions( * - sig is IP or PD only * - match is in applayer * - match is in stream */ - if (s->action & (ACTION_DROP | ACTION_PASS)) { + if (pa->action & (ACTION_DROP | ACTION_PASS)) { if ((pa->flags & (PACKET_ALERT_FLAG_STATE_MATCH | PACKET_ALERT_FLAG_STREAM_MATCH)) || (s->flags & (SIG_FLAG_IPONLY | SIG_FLAG_LIKE_IPONLY | SIG_FLAG_PDONLY | SIG_FLAG_APPLAYER))) { From 5c2e6c4b8342f00c506ef466041f6f859ea6251c Mon Sep 17 00:00:00 2001 From: Victor Julien Date: Thu, 1 Jun 2023 10:57:08 +0200 Subject: [PATCH 07/10] detect: add check to validate drops (cherry picked from commit 95bf7248e85d1c3179b4102c37f8845bcbc678b0) --- src/detect.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/detect.c b/src/detect.c index 724679e9fa0e..8420189a1eb6 100644 --- a/src/detect.c +++ b/src/detect.c @@ -1566,8 +1566,11 @@ static void DetectFlow(ThreadVars *tv, return; } - /* if flow is set to drop, we enforce that here */ + /* we check the flow drop here, and not the packet drop. This is + * to allow stream engine "invalid" drop packets to still be + * evaluated by the stream event rules. */ if (p->flow->flags & FLOW_ACTION_DROP) { + DEBUG_VALIDATE_BUG_ON(!(PKT_IS_PSEUDOPKT(p)) && !PACKET_TEST_ACTION(p, ACTION_DROP)); SCReturn; } From 6767b1ce2201a19903d897f9b05800e36828f6c9 Mon Sep 17 00:00:00 2001 From: Victor Julien Date: Thu, 1 Jun 2023 13:18:33 +0200 Subject: [PATCH 08/10] detect: remove flow drop unittest Test broke after recent changes. Functionality is tested in suricata-verify, so just remove the test. (cherry picked from commit 8a535a0b89ee1679c5f31fe37d4c570c099cec41) --- src/tests/detect.c | 112 --------------------------------------------- 1 file changed, 112 deletions(-) diff --git a/src/tests/detect.c b/src/tests/detect.c index 6a0f8e372822..38291b1c9303 100644 --- a/src/tests/detect.c +++ b/src/tests/detect.c @@ -4794,117 +4794,6 @@ static int SigTestDropFlow03(void) return result; } -/** \test test if the engine set flag to drop pkts of a flow that - * triggered a drop action on IDS mode, but continue the inspection - * as usual (instead of on IPS mode) */ -static int SigTestDropFlow04(void) -{ - Flow f; - HtpState *http_state = NULL; - uint8_t http_buf1[] = "POST /one HTTP/1.0\r\n" - "User-Agent: Mozilla/1.0\r\n" - "Cookie: hellocatch\r\n\r\n"; - uint32_t http_buf1_len = sizeof(http_buf1) - 1; - - uint8_t http_buf2[] = "POST /two HTTP/1.0\r\n" - "User-Agent: Mozilla/1.0\r\n" - "Cookie: hellocatch\r\n\r\n"; - uint32_t http_buf2_len = sizeof(http_buf1) - 1; - - TcpSession ssn; - Packet *p1 = NULL; - Packet *p2 = NULL; - Signature *s = NULL; - ThreadVars tv; - DetectEngineThreadCtx *det_ctx = NULL; - AppLayerParserThreadCtx *alp_tctx = AppLayerParserThreadCtxAlloc(); - - memset(&tv, 0, sizeof(ThreadVars)); - memset(&f, 0, sizeof(Flow)); - memset(&ssn, 0, sizeof(TcpSession)); - - p1 = UTHBuildPacket(NULL, 0, IPPROTO_TCP); - p2 = UTHBuildPacket(NULL, 0, IPPROTO_TCP); - - FLOW_INITIALIZE(&f); - f.protoctx = (void *)&ssn; - f.proto = IPPROTO_TCP; - f.flags |= FLOW_IPV4; - - p1->flow = &f; - p1->flowflags |= FLOW_PKT_TOSERVER; - p1->flowflags |= FLOW_PKT_ESTABLISHED; - p1->flags |= PKT_HAS_FLOW|PKT_STREAM_EST; - - p2->flow = &f; - p2->flowflags |= FLOW_PKT_TOSERVER; - p2->flowflags |= FLOW_PKT_ESTABLISHED; - p2->flags |= PKT_HAS_FLOW|PKT_STREAM_EST; - f.alproto = ALPROTO_HTTP; - - StreamTcpInitConfig(TRUE); - - DetectEngineCtx *de_ctx = DetectEngineCtxInit(); - FAIL_IF_NULL(de_ctx); - de_ctx->flags |= DE_QUIET; - - s = DetectEngineAppendSig(de_ctx, "drop tcp any any -> any 80 " - "(msg:\"Test proto match\"; uricontent:\"one\";" - "sid:1;)"); - FAIL_IF_NULL(s); - - /* the no inspection flag should be set after the first sig gets triggered, - * so the second packet should not match the next sig (because of no inspection) */ - s = DetectEngineAppendSig(de_ctx, "alert tcp any any -> any 80 " - "(msg:\"Test proto match\"; uricontent:\"two\";" - "sid:2;)"); - FAIL_IF_NULL(s); - - SigGroupBuild(de_ctx); - DetectEngineThreadCtxInit(&tv, (void *)de_ctx, (void *)&det_ctx); - - int r = AppLayerParserParse( - NULL, alp_tctx, &f, ALPROTO_HTTP, STREAM_TOSERVER, http_buf1, http_buf1_len); - FAIL_IF_NOT(r == 0); - - http_state = f.alstate; - FAIL_IF_NULL(http_state); - - /* do detect */ - SigMatchSignatures(&tv, de_ctx, det_ctx, p1); - - FAIL_IF_NOT(PacketAlertCheck(p1, 1)); - FAIL_IF(PacketAlertCheck(p1, 2)); - - FAIL_IF_NOT(p1->flow->flags & FLOW_ACTION_DROP); - FAIL_IF_NOT(PacketTestActionOnRealPkt(p1, ACTION_DROP)); - - FAIL_IF(p2->flags & PKT_NOPACKET_INSPECTION); - - r = AppLayerParserParse( - NULL, alp_tctx, &f, ALPROTO_HTTP, STREAM_TOSERVER, http_buf2, http_buf2_len); - FAIL_IF_NOT(r == 0); - - /* do detect */ - SigMatchSignatures(&tv, de_ctx, det_ctx, p2); - - FAIL_IF(PacketAlertCheck(p2, 1)); - FAIL_IF(PacketAlertCheck(p2, 2)); - FAIL_IF_NOT(PacketTestActionOnRealPkt(p2, ACTION_DROP)); - - AppLayerParserThreadCtxFree(alp_tctx); - DetectEngineThreadCtxDeinit(&tv, det_ctx); - DetectEngineCtxFree(de_ctx); - - StreamTcpFreeConfig(TRUE); - FLOW_DESTROY(&f); - - UTHFreePackets(&p1, 1); - UTHFreePackets(&p2, 1); - - PASS; -} - /** \test ICMP packet shouldn't be matching port based sig * Bug #611 */ static int SigTestPorts01(void) @@ -5301,7 +5190,6 @@ void SigRegisterTests(void) UtRegisterTest("SigTestDropFlow01", SigTestDropFlow01); UtRegisterTest("SigTestDropFlow02", SigTestDropFlow02); UtRegisterTest("SigTestDropFlow03", SigTestDropFlow03); - UtRegisterTest("SigTestDropFlow04", SigTestDropFlow04); UtRegisterTest("DetectAddressYamlParsing01", DetectAddressYamlParsing01); UtRegisterTest("DetectAddressYamlParsing02", DetectAddressYamlParsing02); From f5f2dc996bb92b38da2b5c94112d36f0407a7320 Mon Sep 17 00:00:00 2001 From: Jason Ish Date: Thu, 11 May 2023 15:49:59 -0600 Subject: [PATCH 09/10] rust/doc: wrap some code examples in backticks (cherry picked from commit 13fe957b7e81801e72b3c1b42f30aeaa19df8d87) --- rust/src/nfs/nfs.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rust/src/nfs/nfs.rs b/rust/src/nfs/nfs.rs index fbccab1bbca6..377b33984669 100644 --- a/rust/src/nfs/nfs.rs +++ b/rust/src/nfs/nfs.rs @@ -175,7 +175,7 @@ pub struct NFSTransaction { pub file_handle: Vec, /// Procedure type specific data - /// TODO see if this can be an Option>. Initial + /// TODO see if this can be an `Option>`. Initial /// attempt failed. pub type_data: Option, From ad041da7156f614d5b07f7d25b7b37ba39720303 Mon Sep 17 00:00:00 2001 From: Victor Julien Date: Mon, 29 May 2023 19:00:36 +0200 Subject: [PATCH 10/10] windivert: fix compile warnings (cherry picked from commit fd93f002a0999fbb0a10f620604234d4f76a51dc) --- src/source-windivert.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/source-windivert.c b/src/source-windivert.c index 95d99845accc..86b9be033c49 100644 --- a/src/source-windivert.c +++ b/src/source-windivert.c @@ -130,8 +130,12 @@ void *WinDivertGetQueue(int n) } // not defined in MinGW winerror.h +#ifndef ERROR_INVALID_IMAGE_HASH #define ERROR_INVALID_IMAGE_HASH 577L +#endif +#ifndef ERROR_DATA_NOT_ACCEPTED #define ERROR_DATA_NOT_ACCEPTED 592L +#endif /** * \brief return an error description for Win32 error values commonly returned