]> git.ipfire.org Git - thirdparty/snort3.git/commitdiff
Merge pull request #2461 in SNORT/snort3 from ~RUCOMBS/snort3:fb1 to master
authorRuss Combs (rucombs) <rucombs@cisco.com>
Mon, 21 Sep 2020 22:58:00 +0000 (22:58 +0000)
committerRuss Combs (rucombs) <rucombs@cisco.com>
Mon, 21 Sep 2020 22:58:00 +0000 (22:58 +0000)
Squashed commit of the following:

commit 74da689cbda24e7aeb634f85c6bcdc8b08166ec3
Author: russ <rucombs@cisco.com>
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 <rucombs@cisco.com>
Date:   Mon Sep 14 17:56:19 2020 -0400

    search_engine: fix peg type for max_queued

commit f424d598d7d3a02e3333a79718768391ffe1fe71
Author: russ <rucombs@cisco.com>
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 <rucombs@cisco.com>
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.

lua/max_detect.lua
src/detection/fp_config.cc
src/detection/fp_detect.cc
src/detection/treenodes.h
src/ips_options/ips_flowbits.cc
src/main/modules.cc
src/search_engines/bnfa_search.cc

index 29ec2ea5a8602d792f86f97144e690064f607a12..19a8c57514ecaa3f063dac0536347f4bed226cca 100644 (file)
@@ -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
index ebeabcc75903846e1e871ace227df6171aab85e6..6bad3598868bfb97832be849fdf0032236be27cb 100644 (file)
@@ -98,3 +98,4 @@ void FastPatternConfig::set_queue_limit(unsigned int limit)
 {
     queue_limit = limit;
 }
+
index 87f8cecaafe90540c5abd91e8b4c4cbba095d117..7fe2ac525035d4c86be77eeab6e99a0e91081a74 100644 (file)
@@ -39,6 +39,8 @@
 
 #include "fp_detect.h"
 
+#include <vector>
+
 #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<MatchData>;
+
+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<Node> 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<snort::IpsContext*>(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<snort::IpsContext*>(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<snort::IpsContext*>(context)->packet,
-            "Processing pattern match #%d\n", i);
-        int res = match(node.user, node.tree, node.index, context, node.list);
+            static_cast<snort::IpsContext*>(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);
     }
 }
 
index f259dc7f6c53bbf5650761299ac56063000fc1cf..f7f90927ff03aa7d27c7b1e0986cbca7d690ecf2 100644 (file)
@@ -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*);
 };
 
index 0092dae1d1ed104f6a2507e8106939d6266a89c0..5dbc941c67db8245b060c911a421b56d4717d482 100644 (file)
@@ -26,6 +26,7 @@
 
 #include <unordered_map>
 
+#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<std::string>& 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)
index 4a463d4e6253d487d44b61d373b4f8a521d9279b..34d6d5437616b38c8066d7eb22ccc3f28fc0ac09 100644 (file)
@@ -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" },
index 92d869bbb671597f8086706cebe638327bece3f8..87cc199767534713bc59782425a8a7386f551433 100644 (file)
@@ -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;
 }