From: Michael Altizer (mialtize) Date: Wed, 25 Mar 2020 01:39:33 +0000 (+0000) Subject: Merge pull request #2091 in SNORT/snort3 from ~MIALTIZE/snort3:detained_fixups to... X-Git-Tag: 3.0.0-270~1 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=58f0ee1242b83968a7a982558085597eb76c49fd;p=thirdparty%2Fsnort3.git Merge pull request #2091 in SNORT/snort3 from ~MIALTIZE/snort3:detained_fixups to master Squashed commit of the following: commit 1444a851fb7c3714995441a3cd6385f38e733e5e Author: Michael Altizer Date: Tue Mar 24 12:03:33 2020 -0400 stream_tcp: Cancel hold requests on the current packet when flushing commit 16ab4c97c5342e893a2ab5dc6b50b4c7a909fd79 Author: Michael Altizer Date: Thu Mar 19 13:52:16 2020 -0400 active: Move packet hold realization for Stream detainment to verdict handling commit 3e5d373c511b04dd2fcd61937c75e5ae490bd407 Author: Michael Altizer Date: Thu Mar 19 11:24:46 2020 -0400 active: Base hold_packet() decision on DAQ message pool usage This change cascades into TcpStreamTracker's hold packet logic. commit d61ce2dc2ba3d30bd8347ed6b7885e5bd5699e8a Author: Michael Altizer Date: Thu Mar 19 09:34:10 2020 -0400 stream_tcp: Finalize held packets in TcpSession::clear_session() This ensures that held packets are released even if a flow is cleared without cleanup (as in prune conditions). --- diff --git a/src/main/analyzer.cc b/src/main/analyzer.cc index bb37d83a7..a2023e745 100644 --- a/src/main/analyzer.cc +++ b/src/main/analyzer.cc @@ -339,7 +339,7 @@ void Analyzer::post_process_daq_pkt_msg(Packet* p) retry_queue->put(p->daq_msg); daq_stats.retries_queued++; } - else if (!p->active->is_packet_held()) + else if (!p->active->is_packet_held() || !Stream::set_packet_action_to_hold(p)) verdict = distill_verdict(p); if (PacketTracer::is_active()) diff --git a/src/main/modules.cc b/src/main/modules.cc index b21043ce1..2e348583d 100644 --- a/src/main/modules.cc +++ b/src/main/modules.cc @@ -831,6 +831,8 @@ static PegInfo active_pegs[] { CountType::SUM, "failed_injects", "total crafted packet encode + injects that failed" }, { CountType::SUM, "direct_injects", "total crafted packets directly injected" }, { CountType::SUM, "failed_direct_injects", "total crafted packet direct injects that failed" }, + { CountType::SUM, "holds_denied", "total number of packet hold requests denied" }, + { CountType::SUM, "holds_canceled", "total number of packet hold requests canceled" }, { CountType::END, nullptr, nullptr } }; diff --git a/src/packet_io/active.cc b/src/packet_io/active.cc index 1ca1a1a3d..452c3bdb7 100644 --- a/src/packet_io/active.cc +++ b/src/packet_io/active.cc @@ -586,15 +586,28 @@ bool Active::retry_packet(const Packet* p) return true; } -bool Active::hold_packet(const Packet*) +bool Active::hold_packet(const Packet* p) { - if ( active_action < ACT_HOLD ) + if (active_action >= ACT_HOLD) + return false; + + // FIXIT-L same semi-arbitrary heuristic as the retry queue logic - reevaluate later + if (!p->daq_instance || p->daq_instance->get_pool_available() < p->daq_instance->get_batch_size()) { - active_action = ACT_HOLD; - return true; + active_counts.holds_denied++; + return false; } - return false; + active_action = ACT_HOLD; + + return true; +} + +void Active::cancel_packet_hold() +{ + assert(active_action == ACT_HOLD); + active_counts.holds_canceled++; + active_action = ACT_PASS; } void Active::allow_session(Packet* p) diff --git a/src/packet_io/active.h b/src/packet_io/active.h index f67360390..1ae4ba37b 100644 --- a/src/packet_io/active.h +++ b/src/packet_io/active.h @@ -42,6 +42,8 @@ public: PegCount failed_injects; PegCount direct_injects; PegCount failed_direct_injects; + PegCount holds_denied; + PegCount holds_canceled; }; enum ActiveStatus : uint8_t @@ -94,6 +96,7 @@ public: void daq_drop_packet(const Packet*); bool retry_packet(const Packet*); bool hold_packet(const Packet*); + void cancel_packet_hold(); void allow_session(Packet*); void block_session(Packet*, bool force = false); diff --git a/src/service_inspectors/http_inspect/http_stream_splitter_scan.cc b/src/service_inspectors/http_inspect/http_stream_splitter_scan.cc index 587fd7eaf..d9ed4e59b 100644 --- a/src/service_inspectors/http_inspect/http_stream_splitter_scan.cc +++ b/src/service_inspectors/http_inspect/http_stream_splitter_scan.cc @@ -21,6 +21,8 @@ #include "config.h" #endif +#include "packet_io/active.h" + #include "http_common.h" #include "http_cutter.h" #include "http_enum.h" @@ -28,7 +30,6 @@ #include "http_module.h" #include "http_stream_splitter.h" #include "http_test_input.h" -#include "stream/stream.h" using namespace snort; using namespace HttpCommon; @@ -125,7 +126,7 @@ void HttpStreamSplitter::detain_packet(Packet* pkt) } if (!HttpTestManager::use_test_input(HttpTestManager::IN_HTTP)) #endif - Stream::set_packet_action_to_hold(pkt); + pkt->active->hold_packet(pkt); HttpModule::increment_peg_counts(PEG_DETAINED); } diff --git a/src/stream/tcp/tcp_module.cc b/src/stream/tcp/tcp_module.cc index c8a709667..0b339f2ba 100644 --- a/src/stream/tcp/tcp_module.cc +++ b/src/stream/tcp/tcp_module.cc @@ -83,7 +83,6 @@ const PegInfo tcp_pegs[] = { CountType::SUM, "held_packets_passed", "number of held packets passed" }, { CountType::NOW, "cur_packets_held", "number of packets currently held" }, { CountType::MAX, "max_packets_held", "maximum number of packets held simultaneously" }, - { CountType::SUM, "held_packet_limit_exceeded", "number of times limit of max held packets exceeded" }, { CountType::SUM, "partial_flushes", "number of partial flushes initiated" }, { CountType::SUM, "partial_flush_bytes", "partial flush total bytes" }, { CountType::END, nullptr, nullptr } diff --git a/src/stream/tcp/tcp_module.h b/src/stream/tcp/tcp_module.h index 4587358fe..566e3f0a4 100644 --- a/src/stream/tcp/tcp_module.h +++ b/src/stream/tcp/tcp_module.h @@ -97,7 +97,6 @@ struct TcpStats PegCount held_packets_passed; PegCount current_packets_held; PegCount max_packets_held; - PegCount held_packet_limit_exceeded; PegCount partial_flushes; PegCount partial_flush_bytes; }; diff --git a/src/stream/tcp/tcp_session.cc b/src/stream/tcp/tcp_session.cc index a6803c02e..d186d3cf9 100644 --- a/src/stream/tcp/tcp_session.cc +++ b/src/stream/tcp/tcp_session.cc @@ -169,6 +169,17 @@ void TcpSession::clear_session(bool free_flow_data, bool flush_segments, bool re server.reassembler.flush_queued_segments(flow, true, p); } + if ( p ) + { + client.finalize_held_packet(p); + server.finalize_held_packet(p); + } + else + { + client.finalize_held_packet(flow); + server.finalize_held_packet(flow); + } + client.reassembler.purge_segment_list(); server.reassembler.purge_segment_list(); diff --git a/src/stream/tcp/tcp_stream_session.cc b/src/stream/tcp/tcp_stream_session.cc index eb36a9dc8..5a203bad0 100644 --- a/src/stream/tcp/tcp_stream_session.cc +++ b/src/stream/tcp/tcp_stream_session.cc @@ -355,16 +355,6 @@ void TcpStreamSession::cleanup(Packet* p) clear_session(true, true, false, p); client.normalizer.reset(); server.reassembler.reset(); - if ( p ) - { - client.finalize_held_packet(p); - server.finalize_held_packet(p); - } - else - { - client.finalize_held_packet(flow); - server.finalize_held_packet(flow); - } } void TcpStreamSession::clear() diff --git a/src/stream/tcp/tcp_stream_tracker.cc b/src/stream/tcp/tcp_stream_tracker.cc index dcb2a0df7..e4226efb8 100644 --- a/src/stream/tcp/tcp_stream_tracker.cc +++ b/src/stream/tcp/tcp_stream_tracker.cc @@ -655,27 +655,15 @@ bool TcpStreamTracker::is_segment_seq_valid(TcpSegmentDescriptor& tsd) bool TcpStreamTracker::set_held_packet(Packet* p) { - // FIXIT-M - limit of packets held per packet thread should be determined based on runtime criteria - // such as # of DAQ Msg buffers, # of threads, etc... for now we use small number like 10 if ( held_packet ) return false; - if ( tcpStats.current_packets_held >= 50 ) - { - tcpStats.held_packet_limit_exceeded++; - return false; - } - - if ( p->active->hold_packet(p) ) - { - held_packet = p->daq_msg; - held_seq_num = p->ptrs.tcph->seq(); - tcpStats.total_packets_held++; - if ( ++tcpStats.current_packets_held > tcpStats.max_packets_held ) - tcpStats.max_packets_held = tcpStats.current_packets_held; - return true; - } - return false; + held_packet = p->daq_msg; + held_seq_num = p->ptrs.tcph->seq(); + tcpStats.total_packets_held++; + if ( ++tcpStats.current_packets_held > tcpStats.max_packets_held ) + tcpStats.max_packets_held = tcpStats.current_packets_held; + return true; } bool TcpStreamTracker::is_retransmit_of_held_packet(Packet* cp) @@ -712,13 +700,17 @@ void TcpStreamTracker::finalize_held_packet(Packet* cp) held_seq_num = 0; tcpStats.current_packets_held--; } + + if (cp->active->is_packet_held()) + cp->active->cancel_packet_hold(); } void TcpStreamTracker::finalize_held_packet(Flow* flow) { if ( held_packet ) { - if ( flow->ssn_state.session_flags & SSNFLAG_BLOCK ) + if ( (flow->session_state & STREAM_STATE_BLOCK_PENDING) || + (flow->ssn_state.session_flags & SSNFLAG_BLOCK) ) { Analyzer::get_local_analyzer()->finalize_daq_message(held_packet, DAQ_VERDICT_BLOCK); tcpStats.held_packets_dropped++;