From: Russ Combs (rucombs) Date: Mon, 21 Sep 2020 22:58:00 +0000 (+0000) Subject: Merge pull request #2461 in SNORT/snort3 from ~RUCOMBS/snort3:fb1 to master X-Git-Tag: 3.0.3-1~7 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=72c185149b62535934923234b1d695faaeb782c9;p=thirdparty%2Fsnort3.git Merge pull request #2461 in SNORT/snort3 from ~RUCOMBS/snort3:fb1 to master Squashed commit of the following: commit 74da689cbda24e7aeb634f85c6bcdc8b08166ec3 Author: russ Date: Sun Sep 13 18:44:03 2020 -0400 ac_bnfa: disable broken fail state reduction Given sids 1 and 2 with contents |BB CC DD| and |AA BB| respectively, only sid 2 would fire for buffer |AA BB CC DD|. This change increases chasing your fail states for the sake of correctness. For best performance, prefer hyperscan or, failing that, ac_full. commit 46ef119fb723a68b016e12593a940b253bdbd404 Author: russ Date: Mon Sep 14 17:56:19 2020 -0400 search_engine: fix peg type for max_queued commit f424d598d7d3a02e3333a79718768391ffe1fe71 Author: russ Date: Tue Sep 15 11:28:20 2020 -0400 profiler: fix issue where flushed pattern matches caused rule_eval to be profiled under mpse commit 227d230faf4c4b3fa0d4ead38ccc1873e09f2067 Author: russ Date: Sun Sep 6 14:24:03 2020 -0400 flowbits: evaluate checkers after setters for fast pattern matches Simplified flowbits sequencing that ensures that checkers (isset, isnotset) are evaluated after changers (set, unset). This solves a common problem for Talos rules, particularly with file identity flow bits. * Any fast-pattern rule with a check is guaranteed to be evaluated after any rule that does not have a check. * Flowbits sequencing for rules that both change and check is undefined. * No change for non-fast-pattern rules. Non-fast-pattern rules are always evaluated after fast pattern rules, but flowbits sequencing among non-fast-pattern rules is still undefined. * Sequencing applies for any given call to detect, which notably means PDUs and raw packets are processed separately. * Only the first rule in a tree is used to categorize the tree as a checker or non-checker. Hyperscan results in exactly one rule per tree so only the builtin MPSE have the first rule limitation. --- diff --git a/lua/max_detect.lua b/lua/max_detect.lua index 29ec2ea5a..19a8c5751 100644 --- a/lua/max_detect.lua +++ b/lua/max_detect.lua @@ -39,7 +39,6 @@ pop.decompress_zip = true port_scan = nil search_engine.detect_raw_tcp = true -search_engine.queue_limit = 0 smtp.decompress_pdf = true smtp.decompress_swf = true diff --git a/src/detection/fp_config.cc b/src/detection/fp_config.cc index ebeabcc75..6bad35988 100644 --- a/src/detection/fp_config.cc +++ b/src/detection/fp_config.cc @@ -98,3 +98,4 @@ void FastPatternConfig::set_queue_limit(unsigned int limit) { queue_limit = limit; } + diff --git a/src/detection/fp_detect.cc b/src/detection/fp_detect.cc index 87f8cecaa..7fe2ac525 100644 --- a/src/detection/fp_detect.cc +++ b/src/detection/fp_detect.cc @@ -39,6 +39,8 @@ #include "fp_detect.h" +#include + #include "events/event.h" #include "filters/rate_filter.h" #include "filters/sfthreshold.h" @@ -320,8 +322,7 @@ int fp_eval_option(void* v, Cursor& c, Packet* p) static int detection_option_tree_evaluate(detection_option_tree_root_t* root, detection_option_eval_data_t& eval_data) { - if ( !root ) - return 0; + assert(root); RuleLatency::Context rule_latency_ctx(root, eval_data.p); @@ -343,8 +344,8 @@ static int detection_option_tree_evaluate(detection_option_tree_root_t* root, return rval; } -static int rule_tree_match( - void* user, void* tree, int index, void* context, void* neg_list) +static void rule_tree_match( + IpsContext* context, void* user, void* tree, int index, void* neg_list) { PMX* pmx = (PMX*)user; @@ -352,7 +353,7 @@ static int rule_tree_match( detection_option_eval_data_t eval_data; NCListNode* ncl; - eval_data.p = ((IpsContext*)context)->packet; + eval_data.p = context->packet; eval_data.pmd = pmx->pmd; eval_data.flowbit_failed = 0; eval_data.flowbit_noalert = 0; @@ -376,7 +377,7 @@ static int rule_tree_match( last_check->ts.tv_sec = eval_data.p->pkth->ts.tv_sec; last_check->ts.tv_usec = eval_data.p->pkth->ts.tv_usec; last_check->run_num = get_run_num(); - last_check->context_num = eval_data.p->context->context_num; + last_check->context_num = context->context_num; last_check->rebuild_flag = (eval_data.p->packet_flags & PKT_REBUILT_STREAM); } @@ -389,7 +390,7 @@ static int rule_tree_match( } if (eval_data.flowbit_failed) - return -1; + return; /* If this is for an IP rule set, evaluate the rules from * the inner IP offset as well */ @@ -416,7 +417,7 @@ static int rule_tree_match( eval_data.p->dsize = eval_data.p->ptrs.ip_api.pay_len(); /* Recurse, and evaluate with the inner IP */ - rule_tree_match(user, tree, index, context, nullptr); + rule_tree_match(context, user, tree, index, nullptr); } while (layer::set_inner_ip_api(eval_data.p, eval_data.p->ptrs.ip_api, curr_layer) && (eval_data.p->ptrs.ip_api != tmp_api)); @@ -429,7 +430,6 @@ static int rule_tree_match( eval_data.p->dsize = tmp_dsize; } } - return 0; } static int sortOrderByPriority(const void* e1, const void* e2) @@ -696,127 +696,112 @@ static inline int fpFinalSelectEvent(OtnxMatchData* omd, Packet* p) return 0; } -struct Node -{ - void* user; - void* tree; - void* list; - int index; -}; - - class MpseStash { public: - MpseStash(unsigned limit) - : max(limit) - { } - - void init() + struct MatchData { - if ( enable ) - count = 0; - } + void* user; + void* tree; + void* list; + int index; + }; + + using MatchStore = std::vector; + +public: + MpseStash(unsigned limit) : max(limit) { } // this is done in the offload thread - bool push(void* user, void* tree, int index, void* list); + bool push(void* user, void* tree, int index, void* context, void* list); // this is done in the packet thread - bool process(MpseMatch, void*); + void process(IpsContext*); - void disable_process() - { enable = false; } - - void enable_process() - { enable = true; } +private: + void process(IpsContext*, MatchStore&); private: - bool enable = false; - unsigned count = 0; + // balance insertion vs search cache + static constexpr unsigned qmax = 128; + + unsigned inserts = 0; unsigned max; - std::vector queue; - - // perf trade-off, same as Snort 2 - // queue to keep mpse search cache warm - // but limit to avoid the O(n**2) effect of inserts - // and to get any rule hits before exhaustive searching - // consider a map in lieu of vector - const unsigned queue_limit = 32; + + MatchStore queue; + MatchStore defer; }; -// uniquely insert into q, should splay elements for performance -// return true if maxed out to trigger a flush -bool MpseStash::push(void* user, void* tree, int index, void* list) +bool MpseStash::push(void* user, void* tree, int index, void* context, void* list) { + detection_option_tree_root_t* root = (detection_option_tree_root_t*)tree; + bool checker = root->otn->checks_flowbits(); + MatchStore& store = checker ? defer : queue; - for ( auto it = queue.rbegin(); it != queue.rend(); it++ ) + for ( auto it = store.rbegin(); it != store.rend(); it++ ) { if ( tree == (*it).tree ) { - pmqs.tot_inq_inserts++; - return false; + inserts++; + return true; } } - if ( max and ( count == max ) ) + if ( max and inserts == max ) { pmqs.tot_inq_overruns++; return false; } - Node node; - node.user = user; - node.tree = tree; - node.index = index; - node.list = list; - queue.push_back(node); - pmqs.tot_inq_uinserts++; - pmqs.tot_inq_inserts++; - count++; + if ( !checker and qmax == queue.size() ) + { + Profile rule_profile(rulePerfStats); + process((IpsContext*)context, queue); + } - if ( queue.size() == queue_limit ) - return true; // process now + store.push_back({ user, tree, list, index }); - return false; + pmqs.tot_inq_uinserts++; + inserts++; + + return true; } -bool MpseStash::process(MpseMatch match, void* context) +void MpseStash::process(IpsContext* context) { - if ( !enable ) - return true; // maxed out - quit, FIXIT-RC count this condition + if ( !inserts ) + { + debug_log(detection_trace, TRACE_RULE_EVAL, + static_cast(context)->packet, + "Fast pattern processing - no matches found\n"); + return; + } - if ( count > pmqs.max_inq ) - pmqs.max_inq = count; + process(context, queue); + process(context, defer); + if ( inserts > pmqs.max_inq ) + pmqs.max_inq = inserts; + pmqs.tot_inq_inserts += inserts; + inserts = 0; +} + +void MpseStash::process(IpsContext* context, MatchStore& store) +{ #ifdef DEBUG_MSGS - if (count == 0) - debug_log(detection_trace, TRACE_RULE_EVAL, - static_cast(context)->packet, - "Fast pattern processing - no matches found\n"); -#endif unsigned i = 0; - for ( auto it : queue ) +#endif + + for ( auto it : store ) { - Node& node = it; - i++; - // process a pattern - case is handled by otn processing debug_logf(detection_trace, TRACE_RULE_EVAL, - static_cast(context)->packet, - "Processing pattern match #%d\n", i); - int res = match(node.user, node.tree, node.index, context, node.list); + static_cast(context)->packet, "Processing pattern match #%d\n", ++i); - if ( res > 0 ) - { - /* terminate matching */ - pmqs.tot_inq_flush += i; - queue.clear(); - return true; - } + rule_tree_match(context, it.user, it.tree, it.index, it.list); } - pmqs.tot_inq_flush += i; - queue.clear(); - return false; + pmqs.tot_inq_flush += store.size(); + store.clear(); } void fp_set_context(IpsContext& c) @@ -835,17 +820,12 @@ void fp_clear_context(IpsContext& c) snort_free(c.otnx); } -// rule_tree_match() could be used instead to bypass the queuing static int rule_tree_queue( void* user, void* tree, int index, void* context, void* list) { MpseStash* stash = ((IpsContext*)context)->stash; - - if ( stash->push(user, tree, index, list) ) - { - if ( stash->process(rule_tree_match, context) ) - return 1; - } + if ( !stash->push(user, tree, index, context, list) ) + return 1; return 0; } @@ -1308,10 +1288,6 @@ void fp_partial(Packet* p) { Profile mpse_profile(mpsePerfStats); IpsContext* c = p->context; - MpseStash* stash = c->stash; - stash->enable_process(); - stash->init(); - stash->disable_process(); init_match_info(c); c->searches.mf = rule_tree_queue; c->searches.context = c; @@ -1324,7 +1300,6 @@ void fp_complete(Packet* p, bool search) { IpsContext* c = p->context; MpseStash* stash = c->stash; - stash->enable_process(); if ( search ) { @@ -1333,7 +1308,7 @@ void fp_complete(Packet* p, bool search) } { Profile rule_profile(rulePerfStats); - stash->process(rule_tree_match, c); + stash->process(c); print_pkt_info(p, "non-fast-patterns"); fpEvalPacket(p, FPTask::NON_FP); fpFinalSelectEvent(c->otnx, p); @@ -1353,12 +1328,11 @@ static void fp_immediate(Packet* p) MpseStash* stash = c->stash; { Profile mpse_profile(mpsePerfStats); - stash->enable_process(); c->searches.search_sync(); } { Profile rule_profile(rulePerfStats); - stash->process(rule_tree_match, c); + stash->process(c); c->searches.items.clear(); } } @@ -1369,12 +1343,11 @@ static void fp_immediate(MpseGroup* so, Packet* p, const uint8_t* buf, unsigned { Profile mpse_profile(mpsePerfStats); int start_state = 0; - stash->init(); so->get_normal_mpse()->search(buf, len, rule_tree_queue, p->context, &start_state); } { Profile rule_profile(rulePerfStats); - stash->process(rule_tree_match, p->context); + stash->process(p->context); } } diff --git a/src/detection/treenodes.h b/src/detection/treenodes.h index f259dc7f6..f7f90927f 100644 --- a/src/detection/treenodes.h +++ b/src/detection/treenodes.h @@ -172,6 +172,7 @@ struct OptTreeNode static constexpr Flag META_MATCH = 0x08; static constexpr Flag TO_CLIENT = 0x10; static constexpr Flag TO_SERVER = 0x20; + static constexpr Flag BIT_CHECK = 0x40; /* metadata about signature */ SigInfo sigInfo; @@ -239,15 +240,21 @@ struct OptTreeNode void set_to_client() { flags |= TO_CLIENT; } - bool to_client() + bool to_client() const { return (flags & TO_CLIENT) != 0; } void set_to_server() { flags |= TO_SERVER; } - bool to_server() + bool to_server() const { return (flags & TO_SERVER) != 0; } + void set_flowbits_check() + { flags |= BIT_CHECK; } + + bool checks_flowbits() const + { return (flags & BIT_CHECK) != 0; } + void update_fp(snort::IpsOption*); }; diff --git a/src/ips_options/ips_flowbits.cc b/src/ips_options/ips_flowbits.cc index 0092dae1d..5dbc941c6 100644 --- a/src/ips_options/ips_flowbits.cc +++ b/src/ips_options/ips_flowbits.cc @@ -26,6 +26,7 @@ #include +#include "detection/treenodes.h" #include "framework/ips_option.h" #include "framework/module.h" #include "hash/hash_defs.h" @@ -66,10 +67,10 @@ struct FlowBitCheck FlowBitCheck(Op t) : type(t) { } bool validate(); - bool is_setter() + bool is_setter() const { return type == SET or type == UNSET; } - bool is_checker() + bool is_checker() const { return type == IS_SET or type == IS_NOT_SET; } void add(uint16_t); @@ -149,9 +150,12 @@ public: EvalStatus eval(Cursor&, Packet*) override; - bool is_setter() + bool is_setter() const { return config->is_setter(); } + bool is_checker() const + { return config->is_checker(); } + void get_dependencies(bool& set, std::vector& bits); private: @@ -523,11 +527,16 @@ static void mod_dtor(Module* m) delete fb; } -static IpsOption* flowbits_ctor(Module* p, OptTreeNode*) +static IpsOption* flowbits_ctor(Module* p, OptTreeNode* otn) { FlowbitsModule* m = (FlowbitsModule*)p; FlowBitCheck* fbc = m->get_data(); - return new FlowBitsOption(fbc); + FlowBitsOption* opt = new FlowBitsOption(fbc); + + if ( opt->is_checker() ) + otn->set_flowbits_check(); + + return opt; } static void flowbits_dtor(IpsOption* p) diff --git a/src/main/modules.cc b/src/main/modules.cc index 4a463d4e6..34d6d5437 100644 --- a/src/main/modules.cc +++ b/src/main/modules.cc @@ -190,8 +190,8 @@ static const Parameter search_engine_params[] = { "split_any_any", Parameter::PT_BOOL, nullptr, "true", "evaluate any-any rules separately to save memory" }, - { "queue_limit", Parameter::PT_INT, "0:max32", "128", - "maximum number of fast pattern matches to queue per packet (0 means no maximum)" }, + { "queue_limit", Parameter::PT_INT, "0:max32", "0", + "maximum number of fast pattern matches to queue per packet (0 is unlimited)" }, { nullptr, Parameter::PT_MAX, nullptr, nullptr, nullptr } }; @@ -206,7 +206,7 @@ THREAD_LOCAL PatMatQStat pmqs; const PegInfo mpse_pegs[] = { - { CountType::SUM, "max_queued", "maximum fast pattern matches queued for further evaluation" }, + { CountType::MAX, "max_queued", "maximum fast pattern matches queued for further evaluation" }, { CountType::SUM, "total_flushed", "total fast pattern matches processed" }, { CountType::SUM, "total_inserts", "total fast pattern hits" }, { CountType::SUM, "total_overruns", "fast pattern matches discarded due to overflow" }, diff --git a/src/search_engines/bnfa_search.cc b/src/search_engines/bnfa_search.cc index 92d869bbb..87cc19976 100644 --- a/src/search_engines/bnfa_search.cc +++ b/src/search_engines/bnfa_search.cc @@ -594,8 +594,9 @@ int _bnfa_list_get_next_state(bnfa_struct_t* bnfa, int state, int input) return BNFA_FAIL_STATE; /* Fail state */ } } - #endif + +#ifdef ENABLE_BNFA_FAIL_STATE_OPT /* used only by KcontainsJ() */ static int _bnfa_conv_node_to_full(bnfa_trans_node_t* t, bnfa_state_t* full) { @@ -617,11 +618,8 @@ static int _bnfa_conv_node_to_full(bnfa_trans_node_t* t, bnfa_state_t* full) return tcnt; } -/* - * containment test - - * test if all of tj transitions are in tk - */ #ifdef XXXX +// containment test - test if all of tj transitions are in tk static int KcontainsJx(bnfa_trans_node_t* tk, bnfa_trans_node_t* tj) { while ( tj ) @@ -642,8 +640,8 @@ static int KcontainsJx(bnfa_trans_node_t* tk, bnfa_trans_node_t* tj) } return 1; } - #endif + static int KcontainsJ(bnfa_trans_node_t* tk, bnfa_trans_node_t* tj) { bnfa_state_t full[BNFA_MAX_ALPHABET_SIZE]; @@ -697,6 +695,7 @@ static int _bnfa_opt_nfa(bnfa_struct_t* bnfa) #endif return 0; } +#endif // ENABLE_BNFA_FAIL_STATE_OPT /* * Build a non-deterministic finite automata using Aho-Corasick construction @@ -780,9 +779,12 @@ static int _bnfa_build_nfa(bnfa_struct_t* bnfa) } } - /* optimize the failure states */ +#ifdef ENABLE_BNFA_FAIL_STATE_OPT + // FIXIT-L low priority performance issue: bnfa fail state reduction + // optimize the failure states if ( bnfa->bnfaOpt ) _bnfa_opt_nfa(bnfa); +#endif return 0; }