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]) 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 ~~~~~~~~~~~~~~~~ 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/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 diff --git a/rules/decoder-events.rules b/rules/decoder-events.rules index 1972a5afacad..e3f1e30bfa01 100644 --- a/rules/decoder-events.rules +++ b/rules/decoder-events.rules @@ -67,7 +67,8 @@ 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;) +# 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;) 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 +151,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 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 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..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; @@ -348,12 +347,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 +380,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 +539,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); @@ -610,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; @@ -647,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); @@ -666,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); @@ -832,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 */ 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 98e6c8210e32..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) @@ -406,6 +403,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 { 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..de8d21f97ea7 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; @@ -412,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 @@ -644,33 +636,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 +648,6 @@ static inline void FlowReference(Flow **d, Flow *f) if (*d == f) return; #endif - FlowIncrUsecnt(f); *d = f; } } @@ -690,7 +655,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 82dac90a4bf9..2ab8ef081c96 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) @@ -472,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; } @@ -6153,57 +6148,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); 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; } diff --git a/src/util-exception-policy.c b/src/util-exception-policy.c index 1d2f1a637e90..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) { @@ -109,7 +111,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; @@ -122,28 +124,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, " @@ -159,26 +157,42 @@ enum ExceptionPolicy ExceptionPolicyParse(const char *option, const bool support } } + 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, 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; + } + 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; } 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); }