From 5276e1f0af0360d197840f927a0401c5acc238e3 Mon Sep 17 00:00:00 2001 From: "Juweria Ali Imran (jaliimra)" Date: Thu, 17 Jul 2025 18:07:18 +0000 Subject: [PATCH] Pull request #4813: stream_tcp: separate logs and counters for left and right invalid sequence numbers Merge in SNORT/snort3 from ~JALIIMRA/snort3:left_right_invalid_seq2 to master Squashed commit of the following: commit a5a8276c436d1a0e07dcf680b7f549bc2c3c9dd9 Author: Juweria Ali Imran Date: Tue Feb 4 06:24:29 2025 -0500 stream_tcp: separate logs and counters for left and right invalid sequence numbers --- src/stream/tcp/tcp_module.cc | 5 ++- src/stream/tcp/tcp_module.h | 3 +- src/stream/tcp/tcp_normalizer.cc | 38 ++++++++++++++++++----- src/stream/tcp/tcp_reassembly_segments.cc | 6 ++-- src/stream/tcp/tcp_session.cc | 15 ++++++--- src/stream/tcp/tcp_stream_tracker.cc | 10 +++--- src/stream/tcp/tcp_stream_tracker.h | 3 +- 7 files changed, 57 insertions(+), 23 deletions(-) diff --git a/src/stream/tcp/tcp_module.cc b/src/stream/tcp/tcp_module.cc index db4ea1990..ac2567d43 100644 --- a/src/stream/tcp/tcp_module.cc +++ b/src/stream/tcp/tcp_module.cc @@ -61,7 +61,10 @@ const PegInfo tcp_pegs[] = { CountType::SUM, "resyns", "SYN received on established session" }, { CountType::SUM, "discards", "tcp packets discarded" }, { CountType::SUM, "discards_skipped", "tcp packet discards skipped due to normalization disabled" }, - { CountType::SUM, "invalid_seq_num", "tcp packets received with an invalid sequence number" }, + { CountType::SUM, "invalid_seq_left", + "tcp packets received that fall to the left of the current TCP window (spurious retransmits)" }, + { CountType::SUM, "invalid_seq_right", + "tcp packets received that fall to the right of the current TCP window" }, { CountType::SUM, "invalid_ack", "tcp packets received with an invalid ack number" }, { CountType::SUM, "no_flags_set", "tcp packets received with no TCP flags set" }, { CountType::SUM, "events", "events generated" }, diff --git a/src/stream/tcp/tcp_module.h b/src/stream/tcp/tcp_module.h index d6cf39921..7bcaccd2e 100644 --- a/src/stream/tcp/tcp_module.h +++ b/src/stream/tcp/tcp_module.h @@ -65,7 +65,8 @@ struct TcpStats PegCount resyns; PegCount discards; PegCount discards_skipped; - PegCount invalid_seq_num; + PegCount invalid_seq_left; + PegCount invalid_seq_right; PegCount invalid_ack; PegCount no_flags_set; PegCount events; diff --git a/src/stream/tcp/tcp_normalizer.cc b/src/stream/tcp/tcp_normalizer.cc index 70deb56a3..c6acc6f89 100644 --- a/src/stream/tcp/tcp_normalizer.cc +++ b/src/stream/tcp/tcp_normalizer.cc @@ -40,14 +40,32 @@ TcpNormalizer::NormStatus TcpNormalizer::apply_normalizations( TcpNormalizerState& tns, TcpSegmentDescriptor& tsd, uint32_t seq, bool stream_is_inorder) { // drop packet if sequence num is invalid - if ( !tns.tracker->is_segment_seq_valid(tsd) ) + ValidSeqStatus invalid_seq = tns.tracker->is_segment_seq_valid(tsd); + if (invalid_seq) { - if ( is_keep_alive_probe(tns, tsd) ) - return NORM_BAD_SEQ; - bool inline_mode = tsd.is_nap_policy_inline(); - tcpStats.invalid_seq_num++; - log_drop_reason(tns, tsd, inline_mode, "stream", "Normalizer: Sequence number is invalid\n"); + switch (invalid_seq) + { + case ValidSeqStatus::LEFT_INVALID: + if (is_keep_alive_probe(tns, tsd)) + return NORM_BAD_SEQ; + + tcpStats.invalid_seq_left++; + log_drop_reason(tns, tsd, inline_mode, "stream", "Normalizer: Received a Spurious Retransmission\n"); + break; + + case ValidSeqStatus::RIGHT_INVALID: + tcpStats.invalid_seq_right++; + log_drop_reason(tns, tsd, inline_mode, "stream", "Normalizer: Received a packet to the right of the allowed TCP window \n"); + break; + + default: + break; + } + if (PacketTracer::is_active()) + PacketTracer::log("Current TCP window : [ %u - %u ]\n" + , tns.tracker->r_win_base, tns.tracker->r_win_base + tns.tracker->get_snd_wnd()); + tns.tracker->seglist.print_stream_state(tsd.get_talker()); trim_win_payload(tns, tsd, 0, inline_mode); return NORM_BAD_SEQ; } @@ -71,6 +89,10 @@ TcpNormalizer::NormStatus TcpNormalizer::apply_normalizations( if ( !data_inside_window(tns, tsd) ) { log_drop_reason(tns, tsd, inline_mode, "stream", "Normalizer: Data is outside the TCP Window\n"); + if (PacketTracer::is_active()) + PacketTracer::log("Current TCP window : [ %u - %u ]\n" + , tns.tracker->r_win_base, tns.tracker->r_win_base + tns.tracker->get_snd_wnd()); + tns.tracker->seglist.print_stream_state(tsd.get_talker()); trim_win_payload(tns, tsd, 0, inline_mode); return NORM_TRIMMED; } @@ -99,7 +121,9 @@ TcpNormalizer::NormStatus TcpNormalizer::apply_normalizations( } log_drop_reason(tns, tsd, inline_mode, "stream", - "Normalizer: Received data during a Zero Window that is not a Zero Window Probe\n"); + "Normalizer: Received seq during a Zero Window that is not a Zero Window Probe (" + + std::to_string(tns.tracker->rcv_nxt) + ")\n"); + tns.tracker->seglist.print_stream_state(tsd.get_talker()); trim_win_payload(tns, tsd, 0, inline_mode); return NORM_TRIMMED; } diff --git a/src/stream/tcp/tcp_reassembly_segments.cc b/src/stream/tcp/tcp_reassembly_segments.cc index 04e03cff1..0b5ad0402 100644 --- a/src/stream/tcp/tcp_reassembly_segments.cc +++ b/src/stream/tcp/tcp_reassembly_segments.cc @@ -102,11 +102,9 @@ uint32_t TcpReassemblySegments::get_pending_segment_count(const unsigned max) co bool TcpReassemblySegments::segment_within_seglist_window(TcpSegmentDescriptor& tsd) { - if ( !head ) - return true; - - // Left side + assert (head); uint32_t start; + if ( SEQ_LT(seglist_base_seq, head->start_seq()) ) start = seglist_base_seq; else diff --git a/src/stream/tcp/tcp_session.cc b/src/stream/tcp/tcp_session.cc index 5c5915b85..387f4b906 100644 --- a/src/stream/tcp/tcp_session.cc +++ b/src/stream/tcp/tcp_session.cc @@ -599,8 +599,7 @@ void TcpSession::mark_packet_for_drop(TcpSegmentDescriptor& tsd) bool TcpSession::check_reassembly_queue_thresholds(TcpSegmentDescriptor& tsd, TcpStreamTracker* listener) { - // if this packet fits within the current queue limit window then it's good - if ( listener->seglist.segment_within_seglist_window(tsd) ) + if ( !listener->seglist.head ) return false; bool inline_mode = tsd.is_nap_policy_inline(); @@ -612,13 +611,15 @@ bool TcpSession::check_reassembly_queue_thresholds(TcpSegmentDescriptor& tsd, Tc if ( space_left < (int32_t)tsd.get_len() ) { + // if this packet fits within the current queue limit window then it's good + if ( listener->seglist.segment_within_seglist_window(tsd) ) + return false; + tcpStats.exceeded_max_bytes++; if ( is_hole_present(listener, tsd.get_pkt()) ) tcpStats.max_bytes_exceeded_hole++; - bool ret_val = true; - // if this is an asymmetric flow then skip over any seglist holes // and flush to free up seglist space if ( !tsd.get_pkt()->flow->two_way_traffic() ) @@ -628,6 +629,8 @@ bool TcpSession::check_reassembly_queue_thresholds(TcpSegmentDescriptor& tsd, Tc return false; } + bool ret_val = true; + if ( space_left > 0 ) ret_val = !inline_mode; // For partial trim, reassemble only if we can force an inject else @@ -653,6 +656,10 @@ bool TcpSession::check_reassembly_queue_thresholds(TcpSegmentDescriptor& tsd, Tc { if ( listener->seglist.get_seg_count() + 1 > tcp_config->max_queued_segs ) { + // if this packet fits within the current queue limit window then it's good + if ( listener->seglist.segment_within_seglist_window(tsd) ) + return false; + tcpStats.exceeded_max_segs++; if ( is_hole_present(listener, tsd.get_pkt()) ) diff --git a/src/stream/tcp/tcp_stream_tracker.cc b/src/stream/tcp/tcp_stream_tracker.cc index 38c66f788..aecce6a77 100644 --- a/src/stream/tcp/tcp_stream_tracker.cc +++ b/src/stream/tcp/tcp_stream_tracker.cc @@ -784,9 +784,9 @@ bool TcpStreamTracker::update_on_fin_sent(TcpSegmentDescriptor& tsd) return true; } -bool TcpStreamTracker::is_segment_seq_valid(TcpSegmentDescriptor& tsd) +ValidSeqStatus TcpStreamTracker::is_segment_seq_valid(TcpSegmentDescriptor& tsd) { - bool valid_seq = true; + ValidSeqStatus valid_seq = ValidSeqStatus::VALID; int right_ok; uint32_t left_seq; @@ -806,12 +806,12 @@ bool TcpStreamTracker::is_segment_seq_valid(TcpSegmentDescriptor& tsd) uint32_t win = normalizer.get_stream_window(tsd); if ( SEQ_LEQ(tsd.get_seq(), r_win_base + win) ) - return true; + return ValidSeqStatus::VALID; else - valid_seq = false; + valid_seq = ValidSeqStatus::RIGHT_INVALID; } else - valid_seq = false; + valid_seq = ValidSeqStatus::LEFT_INVALID; return valid_seq; } diff --git a/src/stream/tcp/tcp_stream_tracker.h b/src/stream/tcp/tcp_stream_tracker.h index 5f7d23bbc..97c255bdd 100644 --- a/src/stream/tcp/tcp_stream_tracker.h +++ b/src/stream/tcp/tcp_stream_tracker.h @@ -46,6 +46,7 @@ class HeldPacket; class TcpSession; enum FinSeqNumStatus : uint8_t { FIN_NOT_SEEN, FIN_WITH_SEQ_SEEN, FIN_WITH_SEQ_ACKED }; +enum ValidSeqStatus : uint8_t { VALID, LEFT_INVALID, RIGHT_INVALID }; class TcpStreamTracker { @@ -296,7 +297,7 @@ public: void update_on_rst_sent(); bool update_on_fin_recv(TcpSegmentDescriptor&); bool update_on_fin_sent(TcpSegmentDescriptor&); - bool is_segment_seq_valid(TcpSegmentDescriptor&); + ValidSeqStatus is_segment_seq_valid(TcpSegmentDescriptor&); bool set_held_packet(snort::Packet*); bool is_retransmit_of_held_packet(snort::Packet*); void finalize_held_packet(snort::Packet*); -- 2.47.2