From 8e5df2035745a77017fe0f8c62a3ba8f91149abe Mon Sep 17 00:00:00 2001 From: Haleema Khan Date: Mon, 30 Jan 2023 19:24:05 +0500 Subject: [PATCH 01/17] cov Revert "cov" This reverts commit 2e8a89ac48590f53c403322d8f894de231288190. From be26b8719349b5d690157072f411c35ce1021f06 Mon Sep 17 00:00:00 2001 From: Victor Julien Date: Fri, 27 Jan 2023 20:22:44 +0100 Subject: [PATCH 02/17] stream: remove unused pseudo packet function --- src/stream-tcp.c | 51 ------------------------------------------------ src/stream-tcp.h | 2 -- 2 files changed, 53 deletions(-) diff --git a/src/stream-tcp.c b/src/stream-tcp.c index 82dac90a4bf9..59fa5965a181 100644 --- a/src/stream-tcp.c +++ b/src/stream-tcp.c @@ -6153,57 +6153,6 @@ void StreamTcpSetSessionBypassFlag(TcpSession *ssn) (ntcph)->th_ack = (tcph)->th_seq; \ } while (0) -/** - * \brief Function to fetch a packet from the packet allocation queue for - * creation of the pseudo packet from the reassembled stream. - * - * @param parent Pointer to the parent of the pseudo packet - * @param pkt pointer to the raw packet of the parent - * @param len length of the packet - * @return upon success returns the pointer to the new pseudo packet - * otherwise NULL - */ -Packet *StreamTcpPseudoSetup(Packet *parent, uint8_t *pkt, uint32_t len) -{ - SCEnter(); - - if (len == 0) { - SCReturnPtr(NULL, "Packet"); - } - - Packet *p = PacketGetFromQueueOrAlloc(); - if (p == NULL) { - SCReturnPtr(NULL, "Packet"); - } - - /* set the root ptr to the lowest layer */ - if (parent->root != NULL) - p->root = parent->root; - else - p->root = parent; - - /* copy packet and set length, proto */ - p->proto = parent->proto; - p->datalink = parent->datalink; - - PacketCopyData(p, pkt, len); - p->recursion_level = parent->recursion_level + 1; - p->ts = parent->ts; - - FlowReference(&p->flow, parent->flow); - /* set tunnel flags */ - - /* tell new packet it's part of a tunnel */ - SET_TUNNEL_PKT(p); - /* tell parent packet it's part of a tunnel */ - SET_TUNNEL_PKT(parent); - - /* increment tunnel packet refcnt in the root packet */ - TUNNEL_INCR_PKT_TPR(p); - - return p; -} - /** \brief Create a pseudo packet injected into the engine to signal the * opposing direction of this stream trigger detection/logging. * diff --git a/src/stream-tcp.h b/src/stream-tcp.h index ad26779f6dbf..c652a785b407 100644 --- a/src/stream-tcp.h +++ b/src/stream-tcp.h @@ -127,8 +127,6 @@ int StreamTcpCheckMemcap(uint64_t); uint64_t StreamTcpMemuseCounter(void); uint64_t StreamTcpReassembleMemuseGlobalCounter(void); -Packet *StreamTcpPseudoSetup(Packet *, uint8_t *, uint32_t); - int StreamTcpSegmentForEach(const Packet *p, uint8_t flag, StreamSegmentCallback CallbackFunc, void *data); From 9bffe4c7813cfdad5a8574aca31a4069d2c113c3 Mon Sep 17 00:00:00 2001 From: Victor Julien Date: Fri, 27 Jan 2023 20:30:20 +0100 Subject: [PATCH 03/17] flowworker: don't keep unnecessary flow reference Flow stream/detect/log flush packets, don't hold on to the flow beyond the flow worker module. --- src/flow-worker.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/flow-worker.c b/src/flow-worker.c index 98e6c8210e32..5e4588ff4bcc 100644 --- a/src/flow-worker.c +++ b/src/flow-worker.c @@ -406,6 +406,9 @@ static inline void FlowWorkerStreamTCPUpdate(ThreadVars *tv, FlowWorkerThreadDat x->flow, x->flowflags & FLOW_PKT_TOSERVER ? STREAM_TOSERVER : STREAM_TOCLIENT); FLOWWORKER_PROFILING_END(x, PROFILE_FLOWWORKER_TCPPRUNE); + /* no need to keep a flow ref beyond this point */ + FlowDeReference(&x->flow); + if (timeout) { PacketPoolReturnPacket(x); } else { From 8344f934b93c24b29621b027805dc356ee8c8794 Mon Sep 17 00:00:00 2001 From: Victor Julien Date: Fri, 27 Jan 2023 20:47:46 +0100 Subject: [PATCH 04/17] flow: remove use_cnt Packets only ever reference the flow while holding its lock. This means than any code possibly evicting the flow will have to wait for the existing users to complete their work. Therefore the use_cnt serves no function anymore and can be removed. --- src/flow-hash.c | 30 ++++++++---------------------- src/flow-manager.c | 9 ++------- src/flow-util.h | 2 -- src/flow-worker.c | 5 +---- src/flow.c | 2 -- src/flow.h | 36 +----------------------------------- src/stream-tcp.c | 7 +------ src/util-unittest-helper.c | 1 - 8 files changed, 13 insertions(+), 79 deletions(-) diff --git a/src/flow-hash.c b/src/flow-hash.c index 9801dfde90b7..89180d7ad807 100644 --- a/src/flow-hash.c +++ b/src/flow-hash.c @@ -840,23 +840,18 @@ Flow *FlowGetFlowFromHash(ThreadVars *tv, FlowLookupStruct *fls, Packet *p, Flow FlowIsTimedOut(f, (uint32_t)SCTIME_SECS(p->ts), emerg)); if (timedout) { FLOWLOCK_WRLOCK(f); - if (likely(f->use_cnt == 0)) { - next_f = f->next; - MoveToWorkQueue(tv, fls, fb, f, prev_f); - FLOWLOCK_UNLOCK(f); - goto flow_removed; - } + next_f = f->next; + MoveToWorkQueue(tv, fls, fb, f, prev_f); FLOWLOCK_UNLOCK(f); + goto flow_removed; } else if (FlowCompare(f, p) != 0) { FLOWLOCK_WRLOCK(f); /* found a matching flow that is not timed out */ if (unlikely(TcpSessionPacketSsnReuse(p, f, f->protoctx) == 1)) { Flow *new_f = TcpReuseReplace(tv, fls, fb, f, hash, p); - if (likely(f->use_cnt == 0)) { - if (prev_f == NULL) /* if we have no prev it means new_f is now our prev */ - prev_f = new_f; - MoveToWorkQueue(tv, fls, fb, f, prev_f); /* evict old flow */ - } + if (prev_f == NULL) /* if we have no prev it means new_f is now our prev */ + prev_f = new_f; + MoveToWorkQueue(tv, fls, fb, f, prev_f); /* evict old flow */ FLOWLOCK_UNLOCK(f); /* unlock old replaced flow */ if (new_f == NULL) { @@ -1150,8 +1145,8 @@ static inline bool StillAlive(const Flow *f, const SCTime_t ts) * * Called in conditions where the spare queue is empty and memcap is reached. * - * Walks the hash until a flow can be freed. Timeouts are disregarded, use_cnt - * is adhered to. "flow_prune_idx" atomic int makes sure we don't start at the + * Walks the hash until a flow can be freed. Timeouts are disregarded. + * "flow_prune_idx" atomic int makes sure we don't start at the * top each time since that would clear the top of the hash leading to longer * and longer search times under high pressure (observed). * @@ -1195,15 +1190,6 @@ static Flow *FlowGetUsedFlow(ThreadVars *tv, DecodeThreadVars *dtv, const SCTime continue; } - /** never prune a flow that is used by a packet or stream msg - * we are currently processing in one of the threads */ - if (f->use_cnt > 0) { - STATSADDUI64(counter_flow_get_used_eval_busy, 1); - FBLOCK_UNLOCK(fb); - FLOWLOCK_UNLOCK(f); - continue; - } - if (StillAlive(f, ts)) { STATSADDUI64(counter_flow_get_used_eval_reject, 1); FBLOCK_UNLOCK(fb); diff --git a/src/flow-manager.c b/src/flow-manager.c index 25bd114e1e79..264a030ca6e5 100644 --- a/src/flow-manager.c +++ b/src/flow-manager.c @@ -348,12 +348,9 @@ static void FlowManagerHashRowTimeout(FlowManagerTimeoutThread *td, Flow *f, SCT /* never prune a flow that is used by a packet we * are currently processing in one of the threads */ - if (f->use_cnt > 0 || !FlowBypassedTimeout(f, ts, counters)) { + if (!FlowBypassedTimeout(f, ts, counters)) { FLOWLOCK_UNLOCK(f); prev_f = f; - if (f->use_cnt > 0) { - counters->flows_timeout_inuse++; - } f = f->next; continue; } @@ -384,8 +381,6 @@ static void FlowManagerHashRowClearEvictedList( f->next = NULL; f->fb = NULL; - DEBUG_VALIDATE_BUG_ON(f->use_cnt > 0); - FlowQueuePrivateAppendFlow(&td->aside_queue, f); /* flow is still locked in the queue */ @@ -545,7 +540,7 @@ static uint32_t FlowManagerHashRowCleanup(Flow *f, FlowQueuePrivate *recycle_q, } f->flow_end_flags |= FLOW_END_FLAG_SHUTDOWN; - /* no one is referring to this flow, use_cnt 0, removed from hash + /* no one is referring to this flow, removed from hash * so we can unlock it and move it to the recycle queue. */ FLOWLOCK_UNLOCK(f); FlowQueuePrivateAppendFlow(recycle_q, f); diff --git a/src/flow-util.h b/src/flow-util.h index 52ffe1d61dba..8c8adfc87307 100644 --- a/src/flow-util.h +++ b/src/flow-util.h @@ -48,7 +48,6 @@ (f)->vlan_idx = 0; \ (f)->next = NULL; \ (f)->flow_state = 0; \ - (f)->use_cnt = 0; \ (f)->tenant_id = 0; \ (f)->parent_id = 0; \ (f)->probing_parser_toserver_alproto_masks = 0; \ @@ -94,7 +93,6 @@ (f)->timeout_at = 0; \ (f)->timeout_policy = 0; \ (f)->flow_state = 0; \ - (f)->use_cnt = 0; \ (f)->tenant_id = 0; \ (f)->parent_id = 0; \ (f)->probing_parser_toserver_alproto_masks = 0; \ diff --git a/src/flow-worker.c b/src/flow-worker.c index 5e4588ff4bcc..8a6928af5f8f 100644 --- a/src/flow-worker.c +++ b/src/flow-worker.c @@ -189,10 +189,7 @@ static void CheckWorkQueue(ThreadVars *tv, FlowWorkerThreadData *fw, } } - /* this should not be possible */ - BUG_ON(f->use_cnt > 0); - - /* no one is referring to this flow, use_cnt 0, removed from hash + /* no one is referring to this flow, removed from hash * so we can unlock it and pass it to the flow recycler */ if (fw->output_thread_flow != NULL) diff --git a/src/flow.c b/src/flow.c index 24f1d4fe2fad..42be3795cd7f 100644 --- a/src/flow.c +++ b/src/flow.c @@ -699,7 +699,6 @@ void FlowShutdown(void) for (uint32_t u = 0; u < flow_config.hash_size; u++) { f = flow_hash[u].head; while (f) { - DEBUG_VALIDATE_BUG_ON(f->use_cnt != 0); Flow *n = f->next; uint8_t proto_map = FlowGetProtoMapping(f->proto); FlowClearMemory(f, proto_map); @@ -708,7 +707,6 @@ void FlowShutdown(void) } f = flow_hash[u].evicted; while (f) { - DEBUG_VALIDATE_BUG_ON(f->use_cnt != 0); Flow *n = f->next; uint8_t proto_map = FlowGetProtoMapping(f->proto); FlowClearMemory(f, proto_map); diff --git a/src/flow.h b/src/flow.h index 7c7d75744883..1eb08a3f356d 100644 --- a/src/flow.h +++ b/src/flow.h @@ -379,12 +379,6 @@ typedef struct Flow_ uint8_t proto; uint8_t recursion_level; uint16_t vlan_id[2]; - /** how many references exist to this flow *right now* - * - * On receiving a packet the counter is incremented while the flow - * bucked is locked, which is also the case on timeout pruning. - */ - FlowRefCount use_cnt; uint8_t vlan_idx; @@ -644,33 +638,7 @@ static inline void FlowSetNoPayloadInspectionFlag(Flow *f) SCReturn; } -/** - * \brief increase the use count of a flow - * - * \param f flow to decrease use count for - */ -static inline void FlowIncrUsecnt(Flow *f) -{ - if (f == NULL) - return; - - f->use_cnt++; -} - -/** - * \brief decrease the use count of a flow - * - * \param f flow to decrease use count for - */ -static inline void FlowDecrUsecnt(Flow *f) -{ - if (f == NULL) - return; - - f->use_cnt--; -} - -/** \brief Reference the flow, bumping the flows use_cnt +/** \brief Reference the flow * \note This should only be called once for a destination * pointer */ static inline void FlowReference(Flow **d, Flow *f) @@ -682,7 +650,6 @@ static inline void FlowReference(Flow **d, Flow *f) if (*d == f) return; #endif - FlowIncrUsecnt(f); *d = f; } } @@ -690,7 +657,6 @@ static inline void FlowReference(Flow **d, Flow *f) static inline void FlowDeReference(Flow **d) { if (likely(*d != NULL)) { - FlowDecrUsecnt(*d); *d = NULL; } } diff --git a/src/stream-tcp.c b/src/stream-tcp.c index 59fa5965a181..c23c547ab695 100644 --- a/src/stream-tcp.c +++ b/src/stream-tcp.c @@ -238,7 +238,7 @@ void StreamTcpSessionCleanup(TcpSession *ssn) * * This function is called when the flow is destroyed, so it should free * *everything* related to the tcp session. So including the app layer - * data. We are guaranteed to only get here when the flow's use_cnt is 0. + * data. * * \param ssn Void ptr to the ssn. */ @@ -269,11 +269,6 @@ void StreamTcpSessionClear(void *ssnptr) /** * \brief Function to return the stream segments back to the pool. * - * We don't clear out the app layer storage here as that is under protection - * of the "use_cnt" reference counter in the flow. This function is called - * when the use_cnt is always at least 1 (this pkt has incremented the flow - * use_cnt itself), so we don't bother. - * * \param p Packet used to identify the stream. */ void StreamTcpSessionPktFree (Packet *p) diff --git a/src/util-unittest-helper.c b/src/util-unittest-helper.c index f0c7de3d5ddf..35eeb9146e9b 100644 --- a/src/util-unittest-helper.c +++ b/src/util-unittest-helper.c @@ -908,7 +908,6 @@ uint32_t UTHBuildPacketOfFlows(uint32_t start, uint32_t end, uint8_t dir) } FlowHandlePacket(NULL, &fls, p); if (p->flow != NULL) { - p->flow->use_cnt = 0; FLOWLOCK_UNLOCK(p->flow); } From 97403a79e4832cb2692db97e70d9d3e7696332c1 Mon Sep 17 00:00:00 2001 From: Victor Julien Date: Sun, 29 Jan 2023 10:47:53 +0100 Subject: [PATCH 05/17] flow: rearrange Flow struct to be more compact --- src/flow.h | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/flow.h b/src/flow.h index 1eb08a3f356d..de8d21f97ea7 100644 --- a/src/flow.h +++ b/src/flow.h @@ -406,17 +406,15 @@ typedef struct Flow_ /** flow hash - the flow hash before hash table size mod. */ uint32_t flow_hash; + /** timeout policy value in seconds to add to the lastts.tv_sec + * when a packet has been received. */ + uint32_t timeout_policy; + /* time stamp of last update (last packet). Set/updated under the * flow and flow hash row locks, safe to read under either the * flow lock or flow hash row lock. */ SCTime_t lastts; - /* end of flow "header" */ - - /** timeout policy value in seconds to add to the lastts.tv_sec - * when a packet has been received. */ - uint32_t timeout_policy; - FlowStateType flow_state; /** flow tenant id, used to setup flow timeout and stream pseudo From 2d54b0ca53679f53491f0f32513b2b1da57a6acd Mon Sep 17 00:00:00 2001 From: Victor Julien Date: Mon, 30 Jan 2023 11:07:45 +0100 Subject: [PATCH 06/17] flow/mgr: remove flows_timeout_inuse counter --- etc/schema.json | 6 ------ src/flow-manager.c | 9 +++------ 2 files changed, 3 insertions(+), 12 deletions(-) diff --git a/etc/schema.json b/etc/schema.json index 15214e12ad58..2d3d471ad9e1 100644 --- a/etc/schema.json +++ b/etc/schema.json @@ -5010,9 +5010,6 @@ "flows_timeout": { "type": "integer" }, - "flows_timeout_inuse": { - "type": "integer" - }, "full_hash_pass": { "type": "integer" }, @@ -5127,9 +5124,6 @@ "flows_timeout": { "type": "integer" }, - "flows_timeout_inuse": { - "type": "integer" - }, "new_pruned": { "type": "integer" }, diff --git a/src/flow-manager.c b/src/flow-manager.c index 264a030ca6e5..dea1e8704111 100644 --- a/src/flow-manager.c +++ b/src/flow-manager.c @@ -124,7 +124,6 @@ typedef struct FlowTimeoutCounters_ { uint32_t flows_checked; uint32_t flows_notimeout; uint32_t flows_timeout; - uint32_t flows_timeout_inuse; uint32_t flows_removed; uint32_t flows_aside; uint32_t flows_aside_needs_work; @@ -605,7 +604,6 @@ typedef struct FlowCounters_ { uint16_t flow_mgr_flows_checked; uint16_t flow_mgr_flows_notimeout; uint16_t flow_mgr_flows_timeout; - uint16_t flow_mgr_flows_timeout_inuse; uint16_t flow_mgr_flows_aside; uint16_t flow_mgr_flows_aside_needs_work; @@ -642,7 +640,6 @@ static void FlowCountersInit(ThreadVars *t, FlowCounters *fc) fc->flow_mgr_flows_checked = StatsRegisterCounter("flow.mgr.flows_checked", t); fc->flow_mgr_flows_notimeout = StatsRegisterCounter("flow.mgr.flows_notimeout", t); fc->flow_mgr_flows_timeout = StatsRegisterCounter("flow.mgr.flows_timeout", t); - fc->flow_mgr_flows_timeout_inuse = StatsRegisterCounter("flow.mgr.flows_timeout_inuse", t); fc->flow_mgr_flows_aside = StatsRegisterCounter("flow.mgr.flows_evicted", t); fc->flow_mgr_flows_aside_needs_work = StatsRegisterCounter("flow.mgr.flows_evicted_needs_work", t); @@ -661,8 +658,6 @@ static void FlowCountersUpdate( StatsAddUI64(th_v, ftd->cnt.flow_mgr_flows_notimeout, (uint64_t)counters->flows_notimeout); StatsAddUI64(th_v, ftd->cnt.flow_mgr_flows_timeout, (uint64_t)counters->flows_timeout); - StatsAddUI64( - th_v, ftd->cnt.flow_mgr_flows_timeout_inuse, (uint64_t)counters->flows_timeout_inuse); StatsAddUI64(th_v, ftd->cnt.flow_mgr_flows_aside, (uint64_t)counters->flows_aside); StatsAddUI64(th_v, ftd->cnt.flow_mgr_flows_aside_needs_work, (uint64_t)counters->flows_aside_needs_work); @@ -827,7 +822,9 @@ static TmEcode FlowManager(ThreadVars *th_v, void *thread_data) } /* try to time out flows */ - FlowTimeoutCounters counters = { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 }; + // clang-format off + FlowTimeoutCounters counters = { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, }; + // clang-format on if (emerg) { /* in emergency mode, do a full pass of the hash table */ From fb53d18012eb2b9dacfb021060eae2480b1f3024 Mon Sep 17 00:00:00 2001 From: Victor Julien Date: Mon, 30 Jan 2023 11:27:37 +0100 Subject: [PATCH 07/17] flow: enforce flow assumption Enforce assumption that packets in ThreadVars::decode_pq have no flow attached to it because this is only true for packets while they are in the FlowWorker. --- src/tm-threads.c | 4 ++++ src/tm-threads.h | 3 +++ src/tmqh-packetpool.c | 4 +++- 3 files changed, 10 insertions(+), 1 deletion(-) diff --git a/src/tm-threads.c b/src/tm-threads.c index 5668dbc94a36..69ca2e040299 100644 --- a/src/tm-threads.c +++ b/src/tm-threads.c @@ -43,6 +43,7 @@ #include "util-profiling.h" #include "util-signal.h" #include "queue.h" +#include "util-validate.h" #ifdef PROFILE_LOCKING thread_local uint64_t mutex_lock_contention; @@ -117,6 +118,7 @@ TmEcode TmThreadsSlotVarRun(ThreadVars *tv, Packet *p, TmSlot *slot) PACKET_PROFILING_TMM_START(p, s->tm_id); TmEcode r = s->SlotFunc(tv, p, SC_ATOMIC_GET(s->slot_data)); PACKET_PROFILING_TMM_END(p, s->tm_id); + DEBUG_VALIDATE_BUG_ON(p->flow != NULL); /* handle error */ if (unlikely(r == TM_ECODE_FAILED)) { @@ -130,6 +132,7 @@ TmEcode TmThreadsSlotVarRun(ThreadVars *tv, Packet *p, TmSlot *slot) Packet *extra_p = PacketDequeueNoLock(&tv->decode_pq); if (unlikely(extra_p == NULL)) continue; + DEBUG_VALIDATE_BUG_ON(extra_p->flow != NULL); /* see if we need to process the packet */ if (s->slot_next != NULL) { @@ -175,6 +178,7 @@ static int TmThreadTimeoutLoop(ThreadVars *tv, TmSlot *s) Packet *p = PacketDequeue(tv->stream_pq); SCMutexUnlock(&tv->stream_pq->mutex_q); if (likely(p)) { + DEBUG_VALIDATE_BUG_ON(p->flow != NULL); r = TmThreadsSlotProcessPkt(tv, fw_slot, p); if (r == TM_ECODE_FAILED) { break; diff --git a/src/tm-threads.h b/src/tm-threads.h index 6507ce16f550..6a5346be3629 100644 --- a/src/tm-threads.h +++ b/src/tm-threads.h @@ -165,6 +165,9 @@ static inline bool TmThreadsHandleInjectedPackets(ThreadVars *tv) SCMutexUnlock(&pq->mutex_q); if (extra_p == NULL) break; +#ifdef DEBUG_VALIDATION + BUG_ON(extra_p->flow != NULL); +#endif TmEcode r = TmThreadsSlotVarRun(tv, extra_p, tv->tm_flowworker); if (r == TM_ECODE_FAILED) { TmThreadsSlotProcessPktFail(tv, tv->tm_flowworker, extra_p); diff --git a/src/tmqh-packetpool.c b/src/tmqh-packetpool.c index bf69b9932442..431fa2956e72 100644 --- a/src/tmqh-packetpool.c +++ b/src/tmqh-packetpool.c @@ -470,8 +470,10 @@ void TmqhReleasePacketsToPacketPool(PacketQueue *pq) if (pq == NULL) return; - while ( (p = PacketDequeue(pq)) != NULL) + while ((p = PacketDequeue(pq)) != NULL) { + DEBUG_VALIDATE_BUG_ON(p->flow != NULL); TmqhOutputPacketpool(NULL, p); + } return; } From 19151f945b1eaa928e249cc33e67d66e13c366c4 Mon Sep 17 00:00:00 2001 From: Shivani Bhardwaj Date: Mon, 30 Jan 2023 15:14:06 +0530 Subject: [PATCH 08/17] rules/decoder: fix sid for udp.len_invalid rule --- rules/decoder-events.rules | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rules/decoder-events.rules b/rules/decoder-events.rules index 1972a5afacad..bcdbe078569c 100644 --- a/rules/decoder-events.rules +++ b/rules/decoder-events.rules @@ -67,7 +67,7 @@ alert pkthdr any any -> any any (msg:"SURICATA TCP option invalid length"; decod alert pkthdr any any -> any any (msg:"SURICATA TCP duplicated option"; decode-event:tcp.opt_duplicate; classtype:protocol-command-decode; sid:2200037; rev:2;) alert pkthdr any any -> any any (msg:"SURICATA UDP packet too small"; decode-event:udp.pkt_too_small; classtype:protocol-command-decode; sid:2200038; rev:2;) alert pkthdr any any -> any any (msg:"SURICATA UDP header length too small"; decode-event:udp.hlen_too_small; classtype:protocol-command-decode; sid:2200039; rev:2;) -alert pkthdr any any -> any any (msg:"SURICATA UDP invalid length field in the header"; decode-event:udp.len_invalid; classtype:protocol-command-decode; sid:2200220; rev:2;) +alert pkthdr any any -> any any (msg:"SURICATA UDP invalid length field in the header"; decode-event:udp.len_invalid; classtype:protocol-command-decode; sid:2200120; rev:2;) alert pkthdr any any -> any any (msg:"SURICATA SLL packet too small"; decode-event:sll.pkt_too_small; classtype:protocol-command-decode; sid:2200041; rev:2;) alert pkthdr any any -> any any (msg:"SURICATA Ethernet packet too small"; decode-event:ethernet.pkt_too_small; classtype:protocol-command-decode; sid:2200042; rev:2;) alert pkthdr any any -> any any (msg:"SURICATA PPP packet too small"; decode-event:ppp.pkt_too_small; classtype:protocol-command-decode; sid:2200043; rev:2;) @@ -150,5 +150,5 @@ alert pkthdr any any -> any any (msg:"SURICATA CHDLC packet too small"; decode-e alert pkthdr any any -> any any (msg:"SURICATA packet with too many layers"; decode-event:too_many_layers; classtype:protocol-command-decode; sid:2200116; rev:1;) -# next sid is 2200120 +# next sid is 2200121 From 548734e09b40e4e2a7339641d6f193bbd6b8e6b8 Mon Sep 17 00:00:00 2001 From: Victor Julien Date: Mon, 30 Jan 2023 14:32:28 +0100 Subject: [PATCH 09/17] decoder: mention removal of udp.hlen_invalid sig --- rules/decoder-events.rules | 1 + 1 file changed, 1 insertion(+) diff --git a/rules/decoder-events.rules b/rules/decoder-events.rules index bcdbe078569c..e3f1e30bfa01 100644 --- a/rules/decoder-events.rules +++ b/rules/decoder-events.rules @@ -67,6 +67,7 @@ alert pkthdr any any -> any any (msg:"SURICATA TCP option invalid length"; decod alert pkthdr any any -> any any (msg:"SURICATA TCP duplicated option"; decode-event:tcp.opt_duplicate; classtype:protocol-command-decode; sid:2200037; rev:2;) alert pkthdr any any -> any any (msg:"SURICATA UDP packet too small"; decode-event:udp.pkt_too_small; classtype:protocol-command-decode; sid:2200038; rev:2;) alert pkthdr any any -> any any (msg:"SURICATA UDP header length too small"; decode-event:udp.hlen_too_small; classtype:protocol-command-decode; sid:2200039; rev:2;) +# 2200040 "udp.hlen_invalid" has been retired. alert pkthdr any any -> any any (msg:"SURICATA UDP invalid length field in the header"; decode-event:udp.len_invalid; classtype:protocol-command-decode; sid:2200120; rev:2;) alert pkthdr any any -> any any (msg:"SURICATA SLL packet too small"; decode-event:sll.pkt_too_small; classtype:protocol-command-decode; sid:2200041; rev:2;) alert pkthdr any any -> any any (msg:"SURICATA Ethernet packet too small"; decode-event:ethernet.pkt_too_small; classtype:protocol-command-decode; sid:2200042; rev:2;) From d78f9975d7f53ca17d714f967140c50c12056922 Mon Sep 17 00:00:00 2001 From: Victor Julien Date: Mon, 30 Jan 2023 17:06:10 +0100 Subject: [PATCH 10/17] exception/policy: fix formatting issues --- src/util-exception-policy.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/util-exception-policy.c b/src/util-exception-policy.c index 1d2f1a637e90..22181cc66ffb 100644 --- a/src/util-exception-policy.c +++ b/src/util-exception-policy.c @@ -109,7 +109,7 @@ static enum ExceptionPolicy SetIPSOption( const char *option, const char *value_str, enum ExceptionPolicy p) { if (!EngineModeIsIPS()) { - SCLogConfig("%s: %s not a valid config in IDS mode. Ignoring it.)", option, value_str); + SCLogConfig("%s: %s not a valid config in IDS mode. Ignoring it.", option, value_str); return EXCEPTION_POLICY_NOT_SET; } return p; @@ -175,7 +175,7 @@ enum ExceptionPolicy ExceptionPolicyParse(const char *option, const bool support /* If the master switch was set and the Exception Policy option was not individually set, use the defined master Exception Policy */ const char *value = ExceptionPolicyEnumToString(master_policy); - SCLogConfig("%s: %s (defined via 'exception-policy' master switch", option, value); + SCLogConfig("%s: %s (defined via 'exception-policy' master switch)", option, value); policy = master_policy; } } From 0877c44ffb58495f2f54a4827385cb4b4180c54c Mon Sep 17 00:00:00 2001 From: Victor Julien Date: Mon, 30 Jan 2023 17:15:49 +0100 Subject: [PATCH 11/17] exception/policy: 'auto' sets IPS to 'drop-flow' In IPS mode set all exception policies to drop-flow by default, both in the default yaml and if no `exception-policy` is defined. --- src/util-exception-policy.c | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/src/util-exception-policy.c b/src/util-exception-policy.c index 22181cc66ffb..53eabac7e18d 100644 --- a/src/util-exception-policy.c +++ b/src/util-exception-policy.c @@ -122,28 +122,24 @@ enum ExceptionPolicy ExceptionPolicyParse(const char *option, const bool support if ((ConfGet(option, &value_str)) == 1 && value_str != NULL) { if (strcmp(value_str, "drop-flow") == 0) { policy = SetIPSOption(option, value_str, EXCEPTION_POLICY_DROP_FLOW); - SCLogConfig("%s: %s", option, value_str); } else if (strcmp(value_str, "pass-flow") == 0) { policy = EXCEPTION_POLICY_PASS_FLOW; - SCLogConfig("%s: %s", option, value_str); } else if (strcmp(value_str, "bypass") == 0) { policy = EXCEPTION_POLICY_BYPASS_FLOW; - SCLogConfig("%s: %s", option, value_str); } else if (strcmp(value_str, "drop-packet") == 0) { policy = SetIPSOption(option, value_str, EXCEPTION_POLICY_DROP_PACKET); - SCLogConfig("%s: %s", option, value_str); } else if (strcmp(value_str, "pass-packet") == 0) { policy = EXCEPTION_POLICY_PASS_PACKET; - SCLogConfig("%s: %s", option, value_str); } else if (strcmp(value_str, "reject") == 0) { policy = EXCEPTION_POLICY_REJECT; - SCLogConfig("%s: %s", option, value_str); } else if (strcmp(value_str, "ignore") == 0) { // TODO name? policy = EXCEPTION_POLICY_NOT_SET; - SCLogConfig("%s: %s", option, value_str); } else if (strcmp(value_str, "auto") == 0) { - policy = SetIPSOption(option, value_str, EXCEPTION_POLICY_DROP_FLOW); - SCLogConfig("%s: %s", option, value_str); + if (!EngineModeIsIPS()) { + policy = EXCEPTION_POLICY_NOT_SET; + } else { + policy = EXCEPTION_POLICY_DROP_FLOW; + } } else { FatalErrorOnInit( "\"%s\" is not a valid exception policy value. Valid options are drop-flow, " @@ -158,14 +154,16 @@ enum ExceptionPolicy ExceptionPolicyParse(const char *option, const bool support policy = EXCEPTION_POLICY_NOT_SET; } } + SCLogConfig("%s: %s", option, ExceptionPolicyEnumToString(policy)); } else if (strcmp(option, "exception-policy") == 0) { /* not enabled, we won't change the master exception policy, for now */ - SCLogWarning("'exception-policy' master switch not set, so ignoring it." - " This behavior will change in Suricata 8, so please update your" - " config. See ticket #5219 for more details."); - g_eps_master_switch = EXCEPTION_POLICY_NOT_SET; + if (!EngineModeIsIPS()) { + policy = EXCEPTION_POLICY_NOT_SET; + } else { + policy = EXCEPTION_POLICY_DROP_FLOW; + } } else { /* Exception Policy was not defined individually */ enum ExceptionPolicy master_policy = GetMasterExceptionPolicy(option); From 9b8eefc32f7fe5af3401ee678fcd80878d0c4b1f Mon Sep 17 00:00:00 2001 From: Victor Julien Date: Mon, 30 Jan 2023 18:02:39 +0100 Subject: [PATCH 12/17] stream/midstream: add bug number to policy warning --- src/stream-tcp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/stream-tcp.c b/src/stream-tcp.c index c23c547ab695..2ab8ef081c96 100644 --- a/src/stream-tcp.c +++ b/src/stream-tcp.c @@ -467,7 +467,7 @@ void StreamTcpInitConfig(bool quiet) stream_config.midstream_policy = ExceptionPolicyParse("stream.midstream-policy", true); if (stream_config.midstream && stream_config.midstream_policy != EXCEPTION_POLICY_NOT_SET) { SCLogWarning("stream.midstream_policy setting conflicting with stream.midstream enabled. " - "Ignoring stream.midstream_policy."); + "Ignoring stream.midstream_policy. Bug #5825."); stream_config.midstream_policy = EXCEPTION_POLICY_NOT_SET; } From 0c1464be0c4d7a53a25bd1256c80b9b4be6a89e3 Mon Sep 17 00:00:00 2001 From: Victor Julien Date: Mon, 30 Jan 2023 18:28:04 +0100 Subject: [PATCH 13/17] exception/policy: add more info on defaults Be more informative where a exception value came from: defaults, master switch or an explicit setting. --- src/util-exception-policy.c | 34 +++++++++++++++++++++++++--------- 1 file changed, 25 insertions(+), 9 deletions(-) diff --git a/src/util-exception-policy.c b/src/util-exception-policy.c index 53eabac7e18d..efc3f9a009df 100644 --- a/src/util-exception-policy.c +++ b/src/util-exception-policy.c @@ -28,6 +28,8 @@ #include "action-globals.h" enum ExceptionPolicy g_eps_master_switch = EXCEPTION_POLICY_NOT_SET; +/** true if exception policy was defined in config */ +static bool g_eps_have_exception_policy = false; static const char *ExceptionPolicyEnumToString(enum ExceptionPolicy policy) { @@ -154,7 +156,19 @@ enum ExceptionPolicy ExceptionPolicyParse(const char *option, const bool support policy = EXCEPTION_POLICY_NOT_SET; } } - SCLogConfig("%s: %s", option, ExceptionPolicyEnumToString(policy)); + + if (strcmp(option, "exception-policy") == 0) { + g_eps_have_exception_policy = true; + + if (strcmp(value_str, "auto") == 0) { + SCLogConfig("%s: %s (because of 'auto' setting in %s-mode)", option, + ExceptionPolicyEnumToString(policy), EngineModeIsIPS() ? "IPS" : "IDS"); + } else { + SCLogConfig("%s: %s", option, ExceptionPolicyEnumToString(policy)); + } + } else { + SCLogConfig("%s: %s", option, ExceptionPolicyEnumToString(policy)); + } } else if (strcmp(option, "exception-policy") == 0) { /* not enabled, we won't change the master exception policy, @@ -164,19 +178,21 @@ enum ExceptionPolicy ExceptionPolicyParse(const char *option, const bool support } else { policy = EXCEPTION_POLICY_DROP_FLOW; } + SCLogConfig("%s: %s (%s-mode)", option, ExceptionPolicyEnumToString(policy), + EngineModeIsIPS() ? "IPS" : "IDS"); + } else { /* Exception Policy was not defined individually */ - enum ExceptionPolicy master_policy = GetMasterExceptionPolicy(option); - if (master_policy == EXCEPTION_POLICY_NOT_SET) { - SCLogConfig("%s: ignore", option); + policy = GetMasterExceptionPolicy(option); + if (g_eps_have_exception_policy) { + SCLogConfig("%s: %s (defined via 'exception-policy' master switch)", option, + ExceptionPolicyEnumToString(policy)); } else { - /* If the master switch was set and the Exception Policy option was not - individually set, use the defined master Exception Policy */ - const char *value = ExceptionPolicyEnumToString(master_policy); - SCLogConfig("%s: %s (defined via 'exception-policy' master switch)", option, value); - policy = master_policy; + SCLogConfig("%s: %s (defined via 'built-in default' for %s-mode)", option, + ExceptionPolicyEnumToString(policy), EngineModeIsIPS() ? "IPS" : "IDS"); } } + return policy; } From 5105d2c1afd35afa3e7632ae56c03c9abe52c656 Mon Sep 17 00:00:00 2001 From: Victor Julien Date: Mon, 30 Jan 2023 18:42:38 +0100 Subject: [PATCH 14/17] doc: warn IPS users on new exception policy default --- doc/userguide/upgrade.rst | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/doc/userguide/upgrade.rst b/doc/userguide/upgrade.rst index 473c8cd488aa..aad149ae08e6 100644 --- a/doc/userguide/upgrade.rst +++ b/doc/userguide/upgrade.rst @@ -36,11 +36,9 @@ Upgrading 6.0 to 7.0 Major changes ~~~~~~~~~~~~~ - Upgrade of PCRE1 to PCRE2. See :ref:`pcre-update-v1-to-v2` for more details. -- Introducing the :ref:`Exception Policy's Master Switch `. This - allows to setup a single policy for all traffic exceptions. This is a breaking - change for the default behavior in the Exception Policies: in IPS mode, if an - exception policy is not set, it will fall back to the the master switch now, - instead of being ignored. Prevent this by disabling the master switch. +- IPS users: by default various new "exception policies" are set to DROP + traffic. Please see :ref:`Exception Policies ` for details + on the settings and their scope. Security changes ~~~~~~~~~~~~~~~~ From 503946b4f4ee0b4e00fa2ab45ef5312276c58ea8 Mon Sep 17 00:00:00 2001 From: Jason Ish Date: Mon, 30 Jan 2023 11:15:37 -0600 Subject: [PATCH 15/17] requirements: use suricata-update 1.3.0rc1 --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index 6df1358f075f..111123bc7d51 100644 --- a/requirements.txt +++ b/requirements.txt @@ -4,4 +4,4 @@ # # name {repo} {branch|tag} libhtp https://github.com/OISF/libhtp 0.5.x -suricata-update https://github.com/OISF/suricata-update master +suricata-update https://github.com/OISF/suricata-update 1.3.0rc1 From 917ba8079dcba1fa122b13345ddc1b75a78f5e45 Mon Sep 17 00:00:00 2001 From: Shivani Bhardwaj Date: Tue, 31 Jan 2023 11:57:46 +0530 Subject: [PATCH 16/17] release: 7.0.0-rc1; update changelog --- ChangeLog | 112 +++++++++++++++++++++++++++++++++++++++++++++++++++ configure.ac | 2 +- 2 files changed, 113 insertions(+), 1 deletion(-) diff --git a/ChangeLog b/ChangeLog index 460455ac76bf..8ea76515374e 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,115 @@ +7.0.0-rc1 -- 2023-01-31 + +Feature #5761: Unknown ethertype packets are not counted +Feature #5516: tls: client cert detection +Feature #5384: Thread Synchronisation: wait for all threads to be in an operating state before continuing initialisation +Feature #5383: Support for IP addresses in dataset +Feature #5219: ips: add 'master switch' to enable dropping on traffic (handling) exceptions +Feature #5184: Add more dataset user interaction +Feature #4981: frames: add general .stream frames +Feature #4979: frames: implement dynamic logic to disable frames of a type +Feature #4751: dns/eve: add 'HTTPS' type logging +Feature #4269: Additional dataset operations +Feature #3306: Support AF_XDP capture method +Feature #3086: app_proto for Torrent traffic +Feature #2497: error messages usability improvement +Security #5712: tcp: crafted packets lead to resource starvation +Security #5703: smb: crash inside of streaming buffer Grow() +Security #5701: Suricata crashes while processing FTP +Security #5700: SCRealloc of large chunk crashes Suricata +Security #5686: decoder/tunnel: tunnel depth not limited properly +Security #5623: smtp/base64: crash / memory corruption +Bug #5817: tls: certificates with dates prior to 1970 are not logged correctly +Bug #5814: smb: duplicate interface fields logged +Bug #5813: rfb/eve: depth in pixel format logged twice +Bug #5811: smb: tx logs sometimes have duplicate `tree_id` output +Bug #5781: smb: unbounded file chunk queuing after gap +Bug #5779: dcerpc: max-tx config parameter +Bug #5769: Incomplete values for .stats."app_layer".flow.proto +Bug #5765: exceptions: midstream flows are dropped if midstream=true && stream.midstream-policy=drop-flow +Bug #5753: smb: convert transaction list to vecdeque +Bug #5747: iprep/ipv6: warning issued on valid reputation input +Bug #5725: smtp: quoted-printable encoding skips empty lines in files +Bug #5707: quic: ja3 Stack-use-after-return READ 1 +Bug #5706: app-layer-htp: Condition depending on enabled IPS mode never true +Bug #5693: decode: Padded packet to minimal Ethernet length marked with invalid length event +Bug #5691: HTTP/2 decompression bug +Bug #5663: tls: buffer overhead off by one in TLSDecodeHSHelloExtensionSupportedVersions +Bug #5661: security.limit-noproc: break ASAN/LSAN when non-root user +Bug #5658: SMTP: segfault on boundary data +Bug #5654: readthedocs: not showing pdf download option for recent versions +Bug #5644: Integer overflow at dcerpc.rs:846 +Bug #5637: quic: convert to vecdeque +Bug #5624: quic: rule with ja3.hash keyword fails to load +Bug #5617: dpdk: avoid per thread warnings +Bug #5580: dpdk: IDS vs IPS confusion +Bug #5579: pgsql: support out of order parameter in startup message +Bug #5574: base64: skip over all invalid characters for RFC 2045 mode +Bug #5572: pcre2: allow different include/lib paths +Bug #5567: smb: failed assertion (!((f->alproto == ALPROTO_SMB && txd->files_logged != 0))), function CloseFile, file output-file.c +Bug #5564: tls: buffer overread +Bug #5558: detect: invalid hex character in content leads to bad debug message +Bug #5557: dcerpc: rust integer underflow +Bug #5553: dpdk: Packets with invalid checksums are not counted in DPDK capture mode +Bug #5530: frames: buffer overflow in signatures parsing +Bug #5529: frame: memory leak in signature parsing +Bug #5528: tcp: assertion failed in function DoInsertSegment +Bug #5456: detect: config keyword prevents tx cleanup +Bug #5444: dns: allow dns messages with invalid opcodes +Bug #5379: detect/udp: different detection from rules when UDP/TCP header is broken +Bug #5374: pcap-log: breaking change in file names +Bug #5258: smb/ntlmssp: parser incorrectly assumes fixed field order +Bug #5235: ftp: add event when command request or response is too long +Bug #5205: FTP-data unrecognized depending on multi-threading +Bug #5198: eve/stats: ASAN error when eve output file can't be opened. +Bug #5161: smb: file not tracked on smb2 async +Bug #4580: smb: large streams can cause large memory moves (memmove) +Bug #4554: Configuration test mode succeeds when classification.config file contains invalid content +Bug #3253: tls: handling of 'Not Before' date before unix epoch +Bug #2982: invalid dsize distance rule being loaded by suricata +Optimization #5782: smb: set defaults for file chunk limits +Optimization #5373: Prevent process creation by Suricata process +Optimization #4977: frames: gap handling in inspection +Optimization #4908: ftp: use AppLayerResult instead of buffering wherever possible +Optimization #4614: Fix warning about "field reassign with default" +Optimization #4612: Fix warning about "nonminimal bool" +Optimization #4611: Fix warning about "extra unused lifetimes" +Optimization #4610: Fix warning about "explicit counter loop" +Optimization #4608: Fix warning about "redundant pattern matching" +Optimization #4606: Fix warning about "match ref pats" +Optimization #4603: Fix warning about "type complexity" +Optimization #4602: Fix warning about "new without default" +Optimization #4601: Fix warning about "while let loop" +Optimization #4600: Fix warning about "needless lifetimes" +Optimization #4598: Fix warning about "needless_range_loop" +Optimization #4596: Fix warning about "single match" +Optimization #4594: Fix warning about "this loop never actually loops" +Optimization #4592: Fix warning about "for loop over fallibles" +Optimization #4591: Fix Rust clippy lints +Optimization #3160: clean up error codes +Task #5638: SWF decompression: Do not depend on libhtp +Task #5632: Disable swf decompression by default +Task #5587: ips/tap: in layer 2 ips/tap setups, warn that mixed usage of ips and tap will be removed in 8.0 +Task #5586: rust/applayertemplate: remove pub and no_mangle from extern functions that don't need it +Task #5504: exceptions: error out when invalid configuration value is passed +Task #5496: detect/parse: add tests for parsing signatures with reject and drop action +Task #4939: app-layer: template and setup script +Task #4054: Convert unittests to new FAIL/PASS API: detect-replace.c +Task #4050: Convert unittests to new FAIL/PASS API: detect-l3proto.c +Task #4049: Convert unittests to new FAIL/PASS API: detect-itype.c +Task #4043: Convert unittests to new FAIL/PASS API: detect-icmp-seq.c +Task #4042: Convert unittests to new FAIL/PASS API: detect-icmp-id.c +Task #4039: Convert unittests to new FAIL/PASS API: detect-filesize.c +Task #4030: Convert unittests to new FAIL/PASS API: detect-engine-tag.c +Task #4029: Convert unittests to new FAIL/PASS API: detect-engine-sigorder.c +Task #4020: Convert unittests to new FAIL/PASS API - detect-distance.c +Documentation #5616: Ubuntu PPA: Package software-properties-common +Documentation #5585: devguide: bring section about installation from redmine wiki into DevGuide +Documentation #5515: userguide: add a dedicated chapter/section for the Exception Policies +Documentation #5129: devguide: clarify style guide for getframe functions +Documentation #4929: devguide: bring Contributing process page into it +Documentation #4697: devguide: document app-layer frame support + 7.0.0-beta1 -- 2022-10-26 Feature #5509: App-layer event for protocol change failure diff --git a/configure.ac b/configure.ac index 95775e80a757..de199d1dee17 100644 --- a/configure.ac +++ b/configure.ac @@ -1,4 +1,4 @@ - AC_INIT([suricata],[7.0.0-rc1-dev]) + AC_INIT([suricata],[7.0.0-rc1]) m4_ifndef([AM_SILENT_RULES], [m4_define([AM_SILENT_RULES],[])])AM_SILENT_RULES([yes]) AC_CONFIG_HEADERS([src/autoconf.h]) AC_CONFIG_SRCDIR([src/suricata.c]) From 9b169e1a21d0f1f598dcb1988dddceea20aaddee Mon Sep 17 00:00:00 2001 From: Haleema Khan Date: Fri, 13 Jan 2023 17:25:37 +0500 Subject: [PATCH 17/17] mqtt: add mqtt frames Adds PDU, Header and Data frame to the MQTT parser. Ticket: 5731 --- rust/src/mqtt/mqtt.rs | 111 ++++++++++++++++++++++++++++++++++++++---- 1 file changed, 102 insertions(+), 9 deletions(-) diff --git a/rust/src/mqtt/mqtt.rs b/rust/src/mqtt/mqtt.rs index d771d86ef484..8a509781c1fb 100644 --- a/rust/src/mqtt/mqtt.rs +++ b/rust/src/mqtt/mqtt.rs @@ -23,6 +23,7 @@ use crate::applayer::*; use crate::applayer::{self, LoggerFlags}; use crate::conf::conf_get; use crate::core::*; +use crate::frames::*; use nom7::Err; use std; use std::collections::VecDeque; @@ -41,6 +42,13 @@ static mut MQTT_MAX_TX: usize = 1024; static mut ALPROTO_MQTT: AppProto = ALPROTO_UNKNOWN; +#[derive(AppLayerFrameType)] +pub enum MQTTFrameType { + Pdu, + Header, + Data, +} + #[derive(FromPrimitive, Debug, AppLayerEvent)] pub enum MQTTEvent { MissingConnect, @@ -422,8 +430,10 @@ impl MQTTState { } } - fn parse_request(&mut self, input: &[u8]) -> AppLayerResult { + fn parse_request(&mut self, flow: *const Flow, stream_slice: StreamSlice) -> AppLayerResult { + let input = stream_slice.as_slice(); let mut current = input; + if input.is_empty() { return AppLayerResult::ok(); } @@ -455,6 +465,13 @@ impl MQTTState { SCLogDebug!("request: handling {}", current.len()); match parse_message(current, self.protocol_version, self.max_msg_len) { Ok((rem, msg)) => { + let _pdu = Frame::new( + flow, + &stream_slice, + input, + current.len() as i64, + MQTTFrameType::Pdu as u8, + ); SCLogDebug!("request msg {:?}", msg); if let MQTTOperation::TRUNCATED(ref trunc) = msg.op { SCLogDebug!( @@ -463,10 +480,12 @@ impl MQTTState { current.len() ); if trunc.skipped_length >= current.len() { + self.mqtt_frames_trunc(flow, &stream_slice, &trunc, ¤t, &msg); self.skip_request = trunc.skipped_length - current.len(); self.handle_msg(msg, true); return AppLayerResult::ok(); } else { + self.mqtt_frames_trunc(flow, &stream_slice, &trunc, ¤t, &msg); consumed += trunc.skipped_length; current = ¤t[trunc.skipped_length..]; self.handle_msg(msg, true); @@ -474,6 +493,8 @@ impl MQTTState { continue; } } + + self.mqtt_frames(flow, &stream_slice, &msg); self.handle_msg(msg, false); consumed += current.len() - rem.len(); current = rem; @@ -497,8 +518,10 @@ impl MQTTState { return AppLayerResult::ok(); } - fn parse_response(&mut self, input: &[u8]) -> AppLayerResult { + fn parse_response(&mut self, flow: *const Flow, stream_slice: StreamSlice) -> AppLayerResult { + let input = stream_slice.as_slice(); let mut current = input; + if input.is_empty() { return AppLayerResult::ok(); } @@ -529,6 +552,14 @@ impl MQTTState { SCLogDebug!("response: handling {}", current.len()); match parse_message(current, self.protocol_version, self.max_msg_len) { Ok((rem, msg)) => { + let _pdu = Frame::new( + flow, + &stream_slice, + input, + input.len() as i64, + MQTTFrameType::Pdu as u8, + ); + SCLogDebug!("response msg {:?}", msg); if let MQTTOperation::TRUNCATED(ref trunc) = msg.op { SCLogDebug!( @@ -537,11 +568,13 @@ impl MQTTState { current.len() ); if trunc.skipped_length >= current.len() { + self.mqtt_frames_trunc(flow, &stream_slice, &trunc, ¤t, &msg); self.skip_response = trunc.skipped_length - current.len(); self.handle_msg(msg, true); SCLogDebug!("skip_response now {}", self.skip_response); return AppLayerResult::ok(); } else { + self.mqtt_frames_trunc(flow, &stream_slice, &trunc, ¤t, &msg); consumed += trunc.skipped_length; current = ¤t[trunc.skipped_length..]; self.handle_msg(msg, true); @@ -549,6 +582,8 @@ impl MQTTState { continue; } } + + self.mqtt_frames(flow, &stream_slice, &msg); self.handle_msg(msg, true); consumed += current.len() - rem.len(); current = rem; @@ -589,6 +624,64 @@ impl MQTTState { tx.tx_data.set_event(event as u8); self.transactions.push_back(tx); } + + fn mqtt_frames(&mut self, flow: *const Flow, stream_slice: &StreamSlice, input: &MQTTMessage) { + let hdr = stream_slice.as_slice(); + //MQTT payload has a fixed header of 2 bytes + let _mqtt_hdr = Frame::new(flow, stream_slice, hdr, 2, MQTTFrameType::Header as u8); + SCLogDebug!("mqtt_hdr Frame {:?}", _mqtt_hdr); + let rem_length = input.header.remaining_length as usize; + let data = &hdr[2..rem_length + 2]; + let _mqtt_data = Frame::new( + flow, + stream_slice, + data, + rem_length as i64, + MQTTFrameType::Data as u8, + ); + SCLogDebug!("mqtt_data Frame {:?}", _mqtt_data); + } + fn mqtt_frames_trunc( + &mut self, flow: *const Flow, stream_slice: &StreamSlice, input: &MQTTTruncatedData, + current: &[u8], msg: &MQTTMessage, + ) { + let hdr = stream_slice.as_slice(); + let hdr_length = input.skipped_length - msg.header.remaining_length as usize; + let _mqtt_hdr = Frame::new( + flow, + stream_slice, + hdr, + hdr_length as i64, + MQTTFrameType::Header as u8, + ); + SCLogDebug!("mqtt_hdr Frame {:?}", _mqtt_hdr); + + if input.skipped_length >= current.len() { + //taking current.len() as reference as trunc.skipped_length >= current.len() + + let rem_length = current.len() - hdr_length as usize; + let data = &hdr[hdr_length..current.len()]; + let _mqtt_data = Frame::new( + flow, + stream_slice, + data, + rem_length as i64, + MQTTFrameType::Data as u8, + ); + SCLogDebug!("mqtt_data Frame {:?}", _mqtt_data); + } else { + let rem_length = input.skipped_length - hdr_length as usize; + let data = &hdr[hdr_length..rem_length + hdr_length]; + let _mqtt_data = Frame::new( + flow, + stream_slice, + data, + rem_length as i64, + MQTTFrameType::Data as u8, + ); + SCLogDebug!("mqtt_data Frame {:?}", _mqtt_data); + } + } } // C exports. @@ -637,20 +730,20 @@ pub unsafe extern "C" fn rs_mqtt_state_tx_free(state: *mut std::os::raw::c_void, #[no_mangle] pub unsafe extern "C" fn rs_mqtt_parse_request( - _flow: *const Flow, state: *mut std::os::raw::c_void, _pstate: *mut std::os::raw::c_void, + flow: *const Flow, state: *mut std::os::raw::c_void, _pstate: *mut std::os::raw::c_void, stream_slice: StreamSlice, _data: *const std::os::raw::c_void, ) -> AppLayerResult { let state = cast_pointer!(state, MQTTState); - return state.parse_request(stream_slice.as_slice()); + return state.parse_request(flow, stream_slice); } #[no_mangle] pub unsafe extern "C" fn rs_mqtt_parse_response( - _flow: *const Flow, state: *mut std::os::raw::c_void, _pstate: *mut std::os::raw::c_void, + flow: *const Flow, state: *mut std::os::raw::c_void, _pstate: *mut std::os::raw::c_void, stream_slice: StreamSlice, _data: *const std::os::raw::c_void, ) -> AppLayerResult { let state = cast_pointer!(state, MQTTState); - return state.parse_response(stream_slice.as_slice()); + return state.parse_response(flow, stream_slice); } #[no_mangle] @@ -761,8 +854,8 @@ pub unsafe extern "C" fn rs_mqtt_register_parser(cfg_max_msg_len: u32) { apply_tx_config: None, flags: APP_LAYER_PARSER_OPT_UNIDIR_TXS, truncate: None, - get_frame_id_by_name: None, - get_frame_name_by_id: None, + get_frame_id_by_name: Some(MQTTFrameType::ffi_id_from_name), + get_frame_name_by_id: Some(MQTTFrameType::ffi_name_from_id), }; let ip_proto_str = CString::new("tcp").unwrap(); @@ -783,4 +876,4 @@ pub unsafe extern "C" fn rs_mqtt_register_parser(cfg_max_msg_len: u32) { } else { SCLogDebug!("Protocol detector and parser disabled for MQTT."); } -} +} \ No newline at end of file