From: Steve Chew (stechew) Date: Mon, 30 Mar 2020 21:36:02 +0000 (+0000) Subject: Merge pull request #2107 in SNORT/snort3 from ~BBANTWAL/snort3:latency_updates to... X-Git-Tag: 3.0.1-1~3 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=ae805bd5ae007df110ffb6889651d6a172992efa;p=thirdparty%2Fsnort3.git Merge pull request #2107 in SNORT/snort3 from ~BBANTWAL/snort3:latency_updates to master Squashed commit of the following: commit 99e8356b5e645aebb676d58acc22462948cab5b8 Author: Bhagya Tholpady Date: Wed Mar 25 10:01:54 2020 -0400 latency: remove action config option and convert the log handler to trace_log message commit d9ce00ad8447b8f376077b249f1a03c7f0c2acbc Author: Bhagya Tholpady Date: Wed Mar 25 09:05:41 2020 -0400 snort2lua: remove conversion of deprecated options pkt-log and rule-log --- diff --git a/src/latency/latency_config.h b/src/latency/latency_config.h index c3ac7ea60..c580c4495 100644 --- a/src/latency/latency_config.h +++ b/src/latency/latency_config.h @@ -21,6 +21,8 @@ #ifndef LATENCY_CONFIG_H #define LATENCY_CONFIG_H +#include "main/snort_debug.h" + #include "packet_latency_config.h" #include "rule_latency_config.h" @@ -30,4 +32,6 @@ struct LatencyConfig RuleLatencyConfig rule_latency; }; +extern snort::Trace latency_trace; + #endif diff --git a/src/latency/latency_module.cc b/src/latency/latency_module.cc index f331636e0..2a5bee245 100644 --- a/src/latency/latency_module.cc +++ b/src/latency/latency_module.cc @@ -42,6 +42,8 @@ using namespace snort; #define s_help \ "packet and rule latency monitoring and control" +Trace latency_trace(s_name); + static const Parameter s_packet_params[] = { { "max_time", Parameter::PT_INT, "0:max53", "500", @@ -50,9 +52,6 @@ static const Parameter s_packet_params[] = { "fastpath", Parameter::PT_BOOL, nullptr, "false", "fastpath expensive packets (max_time exceeded)" }, - { "action", Parameter::PT_ENUM, "none | alert | log | alert_and_log", "none", - "event action if packet times out and is fastpathed" }, - { nullptr, Parameter::PT_MAX, nullptr, nullptr, nullptr } }; @@ -72,9 +71,6 @@ static const Parameter s_rule_params[] = { "max_suspend_time", Parameter::PT_INT, "0:max32", "30000", "set max time for suspending a rule (ms, 0 means permanently disable rule)" }, - { "action", Parameter::PT_ENUM, "none | alert | log | alert_and_log", "none", - "event action for rule latency enable and suspend events" }, - { nullptr, Parameter::PT_MAX, nullptr, nullptr, nullptr } }; @@ -126,9 +122,6 @@ static inline bool latency_set(Value& v, PacketLatencyConfig& config) else if ( v.is("fastpath") ) config.fastpath = v.get_bool(); - else if ( v.is("action") ) - config.action = - static_cast(v.get_uint8()); else return false; @@ -153,9 +146,6 @@ static inline bool latency_set(Value& v, RuleLatencyConfig& config) long t = clock_ticks(v.get_uint32()); config.max_suspend_time = TO_DURATION(config.max_time, t); } - else if ( v.is("action") ) - config.action = - static_cast(v.get_uint8()); else return false; @@ -163,7 +153,7 @@ static inline bool latency_set(Value& v, RuleLatencyConfig& config) } LatencyModule::LatencyModule() : - Module(s_name, s_help, s_params) + Module(s_name, s_help, s_params, false, &latency_trace) { } bool LatencyModule::set(const char* fqn, Value& v, SnortConfig* sc) @@ -177,7 +167,7 @@ bool LatencyModule::set(const char* fqn, Value& v, SnortConfig* sc) else if ( !strncmp(fqn, slr, strlen(slr)) ) return latency_set(v, sc->latency->rule_latency); - return false; + return Module::set(fqn, v, sc); } const RuleMap* LatencyModule::get_rules() const diff --git a/src/latency/packet_latency.cc b/src/latency/packet_latency.cc index 4ff4fa6f8..938987beb 100644 --- a/src/latency/packet_latency.cc +++ b/src/latency/packet_latency.cc @@ -72,7 +72,7 @@ using EventHandler = EventingWrapper; static inline std::ostream& operator<<(std::ostream& os, const Event& e) { - os << "latency: " << e.packet->context->packet_number << " packet"; + os << "packet " << e.packet->context->packet_number; if ( e.fastpathed ) os << " fastpathed: "; @@ -102,7 +102,7 @@ template class Impl { public: - Impl(const ConfigWrapper&, EventHandler&, EventHandler&); + Impl(const ConfigWrapper&, EventHandler&); void push(); bool pop(const Packet*); @@ -114,12 +114,11 @@ private: std::vector> timers; const ConfigWrapper& config; EventHandler& event_handler; - EventHandler& log_handler; }; template -inline Impl::Impl(const ConfigWrapper& cfg, EventHandler& eh, EventHandler& lh) : - config(cfg), event_handler(eh), log_handler(lh) +inline Impl::Impl(const ConfigWrapper& cfg, EventHandler& eh) : + config(cfg), event_handler(eh) { } template @@ -143,11 +142,7 @@ inline bool Impl::pop(const Packet* p) // timer.mark implies fastpath-related timeout Event e { p, timer.marked_as_fastpathed, timer.elapsed() }; - if ( config->action & PacketLatencyConfig::LOG ) - log_handler.handle(e); - - if ( config->action & PacketLatencyConfig::ALERT ) - event_handler.handle(e); + event_handler.handle(e); } elapsed = clock_usecs(TO_USECS(timer.elapsed())); @@ -186,21 +181,17 @@ static struct SnortConfigWrapper : public ConfigWrapper } config; static struct SnortEventHandler : public EventHandler -{ - void handle(const Event&) override - { DetectionEngine::queue_event(GID_LATENCY, LATENCY_EVENT_PACKET_FASTPATHED); } -} event_handler; - -static struct SnortLogHandler : public EventHandler { void handle(const Event& e) override { assert(e.packet); std::ostringstream ss; ss << e; - LogMessage("%s\n", ss.str().c_str()); + debug_logf(latency_trace, "%s\n", ss.str().c_str()); + + DetectionEngine::queue_event(GID_LATENCY, LATENCY_EVENT_PACKET_FASTPATHED); } -} log_handler; +} event_handler; static THREAD_LOCAL Impl<>* impl = nullptr; @@ -208,7 +199,7 @@ static THREAD_LOCAL Impl<>* impl = nullptr; static inline Impl<>& get_impl() { if ( !impl ) - impl = new Impl<>(config, event_handler, log_handler); + impl = new Impl<>(config, event_handler); return *impl; } @@ -312,15 +303,12 @@ TEST_CASE ( "packet latency impl", "[latency]" ) MockConfigWrapper config; EventHandlerSpy event_handler; - EventHandlerSpy log_handler; MockClock::reset(); - packet_latency::Impl impl(config, event_handler, log_handler); + packet_latency::Impl impl(config, event_handler); config.config.max_time = 2_ticks; - config.config.action = PacketLatencyConfig::ALERT_AND_LOG; - SECTION( "fastpath enabled" ) { config.config.fastpath = true; @@ -336,7 +324,6 @@ TEST_CASE ( "packet latency impl", "[latency]" ) CHECK( impl.pop(nullptr) ); CHECK( event_handler.count == 1 ); - CHECK( log_handler.count == 1 ); } SECTION( "no timeout" ) @@ -345,7 +332,6 @@ TEST_CASE ( "packet latency impl", "[latency]" ) CHECK_FALSE( impl.pop(nullptr) ); CHECK( event_handler.count == 0 ); - CHECK( log_handler.count == 0 ); } } @@ -364,7 +350,6 @@ TEST_CASE ( "packet latency impl", "[latency]" ) CHECK( impl.pop(nullptr) ); CHECK( event_handler.count == 1 ); - CHECK( log_handler.count == 1 ); } SECTION( "no timeout" ) @@ -373,7 +358,6 @@ TEST_CASE ( "packet latency impl", "[latency]" ) CHECK_FALSE( impl.pop(nullptr) ); CHECK( event_handler.count == 0 ); - CHECK( log_handler.count == 0 ); } } } diff --git a/src/latency/packet_latency_config.h b/src/latency/packet_latency_config.h index 0821d891d..18501dfe6 100644 --- a/src/latency/packet_latency_config.h +++ b/src/latency/packet_latency_config.h @@ -25,17 +25,8 @@ struct PacketLatencyConfig { - enum Action - { - NONE = 0x00, - ALERT = 0x01, - LOG = 0x02, - ALERT_AND_LOG = ALERT | LOG - }; - hr_duration max_time = CLOCK_ZERO; bool fastpath = false; - Action action = NONE; bool enabled() const { return max_time > CLOCK_ZERO; } }; diff --git a/src/latency/rule_latency.cc b/src/latency/rule_latency.cc index f045b7964..182f32f2a 100644 --- a/src/latency/rule_latency.cc +++ b/src/latency/rule_latency.cc @@ -86,7 +86,7 @@ static inline std::ostream& operator<<(std::ostream& os, const Event& e) using std::chrono::duration_cast; using std::chrono::microseconds; - os << "latency: " << e.packet->context->packet_number << " rule tree "; + os << "packet " << e.packet->context->packet_number << " rule tree "; switch ( e.type ) { @@ -186,24 +186,21 @@ template class Impl { public: - Impl(const ConfigWrapper&, EventHandler&, EventHandler&); + Impl(const ConfigWrapper&, EventHandler&); bool push(detection_option_tree_root_t*, Packet*); bool pop(); bool suspended() const; private: - void handle(const Event&); - std::vector> timers; const ConfigWrapper& config; EventHandler& event_handler; - EventHandler& log_handler; }; template -inline Impl::Impl(const ConfigWrapper& cfg, EventHandler& eh, EventHandler& lh) : - config(cfg), event_handler(eh), log_handler(lh) +inline Impl::Impl(const ConfigWrapper& cfg, EventHandler& eh) : + config(cfg), event_handler(eh) { } template @@ -219,7 +216,7 @@ inline bool Impl::push(detection_option_tree_root_t* root, Pack if ( RuleTree::reenable(*root, config->max_suspend_time, Clock::now()) ) { Event e { Event::EVENT_ENABLED, config->max_suspend_time, root, p }; - handle(e); + event_handler.handle(e); return true; } } @@ -250,7 +247,7 @@ inline bool Impl::pop() timer.elapsed(), timer.root, timer.packet }; - handle(e); + event_handler.handle(e); } } @@ -268,16 +265,6 @@ inline bool Impl::suspended() const return RuleTree::is_suspended(*timers.back().root); } -template -inline void Impl::handle(const Event& e) -{ - if ( config->action & RuleLatencyConfig::LOG ) - log_handler.handle(e); - - if ( config->action & RuleLatencyConfig::ALERT ) - event_handler.handle(e); -} - // ----------------------------------------------------------------------------- // static variables // ----------------------------------------------------------------------------- @@ -293,6 +280,10 @@ static struct SnortEventHandler : public EventHandler { void handle(const Event& e) override { + std::ostringstream ss; + ss << e; + debug_logf(latency_trace, "%s\n", ss.str().c_str()); + switch ( e.type ) { case Event::EVENT_ENABLED: @@ -309,23 +300,13 @@ static struct SnortEventHandler : public EventHandler } } event_handler; -static struct SnortLogHandler : public EventHandler -{ - void handle(const Event& e) override - { - std::ostringstream ss; - ss << e; - LogMessage("%s\n", ss.str().c_str()); - } -} log_handler; - static THREAD_LOCAL Impl<>* impl = nullptr; // FIXIT-L this should probably be put in a tinit static inline Impl<>& get_impl() { if ( !impl ) - impl = new Impl<>(config, event_handler, log_handler); + impl = new Impl<>(config, event_handler); return *impl; } @@ -461,17 +442,14 @@ TEST_CASE ( "rule latency impl", "[latency]" ) MockConfigWrapper config; EventHandlerSpy event_handler; - EventHandlerSpy log_handler; MockClock::reset(); RuleInterfaceSpy::reset(); - config.config.action = RuleLatencyConfig::ALERT_AND_LOG; - detection_option_tree_root_t root; Packet pkt(false); - rule_latency::Impl impl(config, event_handler, log_handler); + rule_latency::Impl impl(config, event_handler); SECTION( "push" ) { @@ -482,7 +460,6 @@ TEST_CASE ( "rule latency impl", "[latency]" ) SECTION( "push rule" ) { CHECK_FALSE( impl.push(&root, &pkt) ); - CHECK( log_handler.count == 0 ); CHECK( event_handler.count == 0 ); CHECK( RuleInterfaceSpy::reenable_called ); } @@ -492,7 +469,6 @@ TEST_CASE ( "rule latency impl", "[latency]" ) RuleInterfaceSpy::reenable_result = true; CHECK( impl.push(&root, &pkt) ); - CHECK( log_handler.count == 1 ); CHECK( event_handler.count == 1 ); CHECK( RuleInterfaceSpy::reenable_called ); } @@ -505,7 +481,6 @@ TEST_CASE ( "rule latency impl", "[latency]" ) SECTION( "push rule" ) { CHECK_FALSE( impl.push(&root, &pkt) ); - CHECK( log_handler.count == 0 ); CHECK( event_handler.count == 0 ); CHECK_FALSE( RuleInterfaceSpy::reenable_called ); } @@ -550,7 +525,6 @@ TEST_CASE ( "rule latency impl", "[latency]" ) RuleInterfaceSpy::is_suspended_result = true; CHECK_FALSE( impl.pop() ); - CHECK( log_handler.count == 0 ); CHECK( event_handler.count == 0 ); CHECK_FALSE( RuleInterfaceSpy::timeout_and_suspend_called ); } @@ -561,7 +535,6 @@ TEST_CASE ( "rule latency impl", "[latency]" ) RuleInterfaceSpy::timeout_and_suspend_result = true; CHECK( impl.pop() ); - CHECK( log_handler.count == 1 ); CHECK( event_handler.count == 1 ); CHECK( RuleInterfaceSpy::timeout_and_suspend_called ); } @@ -571,7 +544,6 @@ TEST_CASE ( "rule latency impl", "[latency]" ) RuleInterfaceSpy::timeout_and_suspend_result = false; CHECK( impl.pop() ); - CHECK( log_handler.count == 1 ); CHECK( event_handler.count == 1 ); CHECK( RuleInterfaceSpy::timeout_and_suspend_called ); } @@ -584,7 +556,6 @@ TEST_CASE ( "rule latency impl", "[latency]" ) RuleInterfaceSpy::is_suspended_result = false; CHECK_FALSE( impl.pop() ); - CHECK( log_handler.count == 0 ); CHECK( event_handler.count == 0 ); CHECK_FALSE( RuleInterfaceSpy::timeout_and_suspend_called ); } diff --git a/src/latency/rule_latency_config.h b/src/latency/rule_latency_config.h index f88f79100..e47afd446 100644 --- a/src/latency/rule_latency_config.h +++ b/src/latency/rule_latency_config.h @@ -25,19 +25,10 @@ struct RuleLatencyConfig { - enum Action - { - NONE = 0x00, - ALERT = 0x01, - LOG = 0x02, - ALERT_AND_LOG = ALERT | LOG - }; - hr_duration max_time = 0_ticks; bool suspend = false; unsigned suspend_threshold = 0; hr_duration max_suspend_time = 0_ticks; - Action action = NONE; bool enabled() const { return max_time > 0_ticks; } bool allow_reenable() const { return max_suspend_time > 0_ticks; } diff --git a/tools/snort2lua/config_states/config_ppm.cc b/tools/snort2lua/config_states/config_ppm.cc index b06aeb9a5..9c352dd76 100644 --- a/tools/snort2lua/config_states/config_ppm.cc +++ b/tools/snort2lua/config_states/config_ppm.cc @@ -47,17 +47,12 @@ bool Ppm::convert(std::istringstream& data_stream) while (data_stream >> keyword) { bool tmpval = true; - bool popped_comma; - + bool popped_comma = false; if (keyword.back() == ',') { keyword.pop_back(); popped_comma = true; } - else - { - popped_comma = false; - } if (keyword.empty()) continue; @@ -125,67 +120,14 @@ bool Ppm::convert(std::istringstream& data_stream) table_api.close_table(); } - else if (keyword == "pkt-log") - { - table_api.add_diff_option_comment("pkt-log", "packet.action"); - table_api.open_table("packet"); - - std::string opt1; - std::string opt2; - - if (popped_comma) - table_api.add_option("action", "log"); - - else if (!(data_stream >> opt1)) - table_api.add_option("action", "log"); - - else if (opt1.back() == ',') - { - opt1.pop_back(); - tmpval = table_api.add_option("action", opt1); - } - - else if (!(data_stream >> opt2)) - tmpval = table_api.add_option("action", opt1); - - else - { - table_api.add_diff_option_comment("'both'", "'alert_and_log'"); - tmpval = table_api.add_option("action", "alert_and_log"); - } - - table_api.close_table(); - } - - else if (keyword == "rule-log") + else if ((keyword == "pkt-log") or (keyword == "rule-log")) { - table_api.add_diff_option_comment("rule-log", "rule.action"); - table_api.open_table("rule"); - - std::string opt1; - std::string opt2; - - if (!(data_stream >> opt1)) - tmpval = false; - - else if (opt1.back() == ',') + table_api.add_deleted_comment(keyword); + if (!popped_comma) { - opt1.pop_back(); - tmpval = table_api.add_option("action", opt1); + while ((data_stream >> keyword) && (keyword.back() != ',')); } - - else if (!(data_stream >> opt2)) - tmpval = table_api.add_option("action", opt1); - - else - { - table_api.add_diff_option_comment("'both'", "'alert_and_log'"); - tmpval = table_api.add_option("action", "alert_and_log"); - } - - table_api.close_table(); } - else tmpval = false;