From: Masud Hasan (mashasan) Date: Wed, 2 Feb 2022 00:20:50 +0000 (+0000) Subject: Pull request #3212: Call splitter finish() on end-of-flow data, on a FIN packet. X-Git-Tag: 3.1.23.0~6 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=850602aac789e03fd0df6946133fb6d4964d3cd2;p=thirdparty%2Fsnort3.git Pull request #3212: Call splitter finish() on end-of-flow data, on a FIN packet. Merge in SNORT/snort3 from ~SMINUT/snort3:fin_recv_flush_up to master Squashed commit of the following: commit 638c0494ccdf566b1f82605d43c29c2c24c58527 Author: Silviu Minut Date: Thu Dec 9 11:41:14 2021 -0500 stream_tcp: fix a bug in which in some cases we did not call splitter finish() in each direction, by calling flush_queued_segments() in perform_fin_recv_flush() on FIN with data packets stream: defer flush_queued_segments() if flow->clouseau stream_tcp: introduce TcpStreamTracker::delayed_finish_flag and call splitter finish from flush_on_data_policy if delayed_finish_flag is true stream_tcp: better place for setting delayed_finish_flag call flush_queued_segments() rather than splitter_finish() directly, from flush_on_data_policy() stream_tcp: wrap flow->clouseau in searching_for_service() --- diff --git a/src/flow/flow.h b/src/flow/flow.h index 36e073bcd..356d188bb 100644 --- a/src/flow/flow.h +++ b/src/flow/flow.h @@ -314,6 +314,11 @@ public: clouseau = nullptr; } + bool searching_for_service() + { + return clouseau != nullptr; + } + void set_gadget(Inspector* ins) { gadget = ins; diff --git a/src/managers/inspector_manager.cc b/src/managers/inspector_manager.cc index f43453ff7..7d167dfa0 100644 --- a/src/managers/inspector_manager.cc +++ b/src/managers/inspector_manager.cc @@ -1676,7 +1676,8 @@ void InspectorManager::full_inspection(Packet* p) { Flow* flow = p->flow; - if ( flow->service and flow->clouseau and (!(p->is_cooked()) or p->is_defrag()) ) + if ( flow->service and flow->searching_for_service() + and (!(p->is_cooked()) or p->is_defrag()) ) bumble(p); // For reassembled PDUs, a null data buffer signals no detection. Detection can be required diff --git a/src/stream/tcp/tcp_reassembler.cc b/src/stream/tcp/tcp_reassembler.cc index 89d601510..5e463debf 100644 --- a/src/stream/tcp/tcp_reassembler.cc +++ b/src/stream/tcp/tcp_reassembler.cc @@ -1055,7 +1055,11 @@ int TcpReassembler::flush_on_data_policy(TcpReassemblerState& trs, Packet* p) flags = get_forward_packet_dir(trs, p); int32_t flush_amt = scan_data_pre_ack(trs, &flags, p); if ( flush_amt <= 0 ) + { + if (trs.tracker->delayed_finish()) + flush_queued_segments(trs, p->flow, true, p); break; + } flushed += flush_to_seq(trs, flush_amt, p, flags); } @@ -1142,7 +1146,11 @@ int TcpReassembler::flush_on_ack_policy(TcpReassemblerState& trs, Packet* p) flags = get_reverse_packet_dir(trs, p); flush_amt = scan_data_post_ack(trs, &flags, p); if ( flush_amt <= 0 or trs.paf_state.paf == StreamSplitter::SKIP ) + { + if (trs.tracker->delayed_finish()) + flush_queued_segments(trs, p->flow, true, p); break; + } if ( trs.paf_state.paf == StreamSplitter::ABORT ) trs.tracker->splitter_finish(p->flow); diff --git a/src/stream/tcp/tcp_state_fin_wait1.cc b/src/stream/tcp/tcp_state_fin_wait1.cc index 5e6d8140a..aaba9c7f3 100644 --- a/src/stream/tcp/tcp_state_fin_wait1.cc +++ b/src/stream/tcp/tcp_state_fin_wait1.cc @@ -92,23 +92,22 @@ bool TcpStateFinWait1::fin_sent(TcpSegmentDescriptor& tsd, TcpStreamTracker& trk bool TcpStateFinWait1::fin_recv(TcpSegmentDescriptor& tsd, TcpStreamTracker& trk) { Flow* flow = tsd.get_flow(); + bool is_ack_valid = false; trk.update_tracker_ack_recv(tsd); - if ( trk.update_on_fin_recv(tsd) ) + if ( SEQ_GEQ(tsd.get_end_seq(), trk.r_win_base) and + check_for_window_slam(tsd, trk, &is_ack_valid) ) { - bool is_ack_valid = false; - if ( check_for_window_slam(tsd, trk, &is_ack_valid) ) - { - trk.perform_fin_recv_flush(tsd); + trk.perform_fin_recv_flush(tsd); + trk.update_on_fin_recv(tsd); - if ( !flow->two_way_traffic() ) - trk.set_tf_flags(TF_FORCE_FLUSH); + if ( !flow->two_way_traffic() ) + trk.set_tf_flags(TF_FORCE_FLUSH); - if ( is_ack_valid ) - trk.set_tcp_state(TcpStreamTracker::TCP_TIME_WAIT); - else - trk.set_tcp_state(TcpStreamTracker::TCP_CLOSING); - } + if ( is_ack_valid ) + trk.set_tcp_state(TcpStreamTracker::TCP_TIME_WAIT); + else + trk.set_tcp_state(TcpStreamTracker::TCP_CLOSING); } return true; } diff --git a/src/stream/tcp/tcp_state_fin_wait2.cc b/src/stream/tcp/tcp_state_fin_wait2.cc index 10f1bc9bd..702543ce1 100644 --- a/src/stream/tcp/tcp_state_fin_wait2.cc +++ b/src/stream/tcp/tcp_state_fin_wait2.cc @@ -106,9 +106,10 @@ bool TcpStateFinWait2::fin_recv(TcpSegmentDescriptor& tsd, TcpStreamTracker& trk Flow* flow = tsd.get_flow(); trk.update_tracker_ack_recv(tsd); - if ( trk.update_on_fin_recv(tsd) ) + if ( SEQ_GEQ(tsd.get_end_seq(), trk.r_win_base) ) { trk.perform_fin_recv_flush(tsd); + trk.update_on_fin_recv(tsd); if ( !flow->two_way_traffic() ) trk.set_tf_flags(TF_FORCE_FLUSH); diff --git a/src/stream/tcp/tcp_stream_tracker.cc b/src/stream/tcp/tcp_stream_tracker.cc index 4f3281cb0..19c0fe499 100644 --- a/src/stream/tcp/tcp_stream_tracker.cc +++ b/src/stream/tcp/tcp_stream_tracker.cc @@ -223,6 +223,7 @@ void TcpStreamTracker::init_tcp_state() flush_policy = STREAM_FLPOLICY_IGNORE; reassembler.reset(); splitter_finish_flag = false; + delayed_finish_flag = false; } //------------------------------------------------------------------------- @@ -664,8 +665,18 @@ void TcpStreamTracker::perform_fin_recv_flush(TcpSegmentDescriptor& tsd) { if ( tsd.is_data_segment() ) session->handle_data_segment(tsd); - else if ( flush_policy == STREAM_FLPOLICY_ON_DATA and SEQ_EQ(tsd.get_seq(), rcv_nxt) ) + + // If the packet is in-sequence, call finish and final flush on it. + // FIXIT-L: what do we do about out-of-sequence packets? + if ( flush_policy == STREAM_FLPOLICY_ON_DATA and SEQ_EQ(tsd.get_end_seq(), rcv_nxt) ) + { + if (tsd.get_flow()->searching_for_service()) + { + delayed_finish_flag = true; + return; + } reassembler.flush_queued_segments(tsd.get_flow(), true, tsd.get_pkt()); + } } uint32_t TcpStreamTracker::perform_partial_flush() diff --git a/src/stream/tcp/tcp_stream_tracker.h b/src/stream/tcp/tcp_stream_tracker.h index 5e5e0e3f9..6cdcd00b1 100644 --- a/src/stream/tcp/tcp_stream_tracker.h +++ b/src/stream/tcp/tcp_stream_tracker.h @@ -262,6 +262,8 @@ public: { return splitter && splitter->is_paf(); } bool splitter_finish(snort::Flow* flow); + bool delayed_finish() const + { return delayed_finish_flag; } bool is_reassembly_enabled() const { return ( splitter and (flush_policy != STREAM_FLPOLICY_IGNORE) ); } @@ -357,6 +359,7 @@ protected: bool mac_addr_valid = false; bool fin_seq_set = false; // FIXIT-M should be obviated by tcp state bool splitter_finish_flag = false; + bool delayed_finish_flag = false; }; // <--- note -- the 'state' parameter must be a reference