Merge pull request #2527 in SNORT/snort3 from ~STECHEW/snort3:ips_infinite_loop to...

Squashed commit of the following:

commit acc6832a9d351f2376404f3be7596c29e93993f8
Author: Steve Chew <stechew@cisco.com>
Date:   Thu Oct 1 15:45:47 2020 -0400

    ips_options: Fix retry calculation in IPS content when handling "within" field.

diff --git a/src/detection/detection_options.cc b/src/detection/detection_options.cc
index b9e311c7f86bf692c8a52364b18ec444a8682dc8..3ba459478bdba6173608a9776889d20bcc642a63 100644 (file)
--- a/src/detection/detection_options.cc
+++ b/src/detection/detection_options.cc
@@ -355,7 +355,7 @@ int detection_option_node_evaluate(
     RuleContext profile(state);
 
     int result = 0;
-    int rval = (int)IpsOption::NO_MATCH;  // FIXIT-L refactor to eliminate casts to int
+    int rval;
     char tmp_noalert_flag = 0;
     Cursor cursor = orig_cursor;
     bool continue_loop = true;
@@ -411,6 +411,7 @@ int detection_option_node_evaluate(
     // No, haven't evaluated this one before... Check it.
     do
     {
+        rval = (int)IpsOption::NO_MATCH;  // FIXIT-L refactor to eliminate casts to int.
         if ( node->otn )
         {
             SnortProtocolId snort_protocol_id = p->get_snort_protocol_id();
@@ -681,7 +682,7 @@ int detection_option_node_evaluate(
         if ( continue_loop && rval == (int)IpsOption::MATCH && node->relative_children )
         {
             IpsOption* opt = (IpsOption*)node->option_data;
-            continue_loop = opt->retry(cursor);
+            continue_loop = opt->retry(cursor, orig_cursor);
         }
         else
             continue_loop = false;

diff --git a/src/framework/ips_option.h b/src/framework/ips_option.h
index afe1539ed2b2013549012327726b4f3dc3feae0d..8eb88609772cfa540bc6c13598c2638ce8eefda9 100644 (file)
--- a/src/framework/ips_option.h
+++ b/src/framework/ips_option.h
@@ -87,7 +87,7 @@ public:
 
     // packet threads
     virtual bool is_relative() { return false; }
-    virtual bool retry(Cursor&) { return false; }
+    virtual bool retry(Cursor&, const Cursor&) { return false; }
     virtual void action(Packet*) { }
 
     enum EvalStatus { NO_MATCH, MATCH, NO_ALERT, FAILED_BIT };

diff --git a/src/ips_options/ips_content.cc b/src/ips_options/ips_content.cc
index 62e6a188410b572cf9b6694f57f55769b855fecb..4d918324faffcfbf6dc9d26a0bce0971fbfe0171 100644 (file)
--- a/src/ips_options/ips_content.cc
+++ b/src/ips_options/ips_content.cc
@@ -144,7 +144,7 @@ public:
     bool is_relative() override
     { return config->pmd.is_relative(); }
 
-    bool retry(Cursor&) override;
+    bool retry(Cursor&, const Cursor&) override;
 
     ContentData* get_data()
     { return config; }
@@ -162,7 +162,7 @@ protected:
     ContentData* config;
 };
 
-bool ContentOption::retry(Cursor& c)
+bool ContentOption::retry(Cursor& current_cursor, const Cursor& orig_cursor)
 {
     if ( config->pmd.is_negated() )
         return false;
@@ -170,11 +170,10 @@ bool ContentOption::retry(Cursor& c)
     if ( !config->pmd.depth )
         return true;
 
-    // FIXIT-L consider moving adjusting delta from eval to retry
-    assert(c.get_delta() >= config->match_delta);
-
-    unsigned min = c.get_delta() + config->pmd.pattern_size;
-    unsigned max = c.get_delta() - config->match_delta + config->pmd.offset + config->pmd.depth;
+    // Do not retry if the current pattern would extend beyond the area
+    // in which we're looking for it.
+    unsigned min = current_cursor.get_delta() + config->pmd.pattern_size;
+    unsigned max = orig_cursor.get_delta() + config->pmd.offset + config->pmd.depth;
 
     return min <= max;
 }
@@ -331,6 +330,9 @@ static int uniSearchReal(ContentData* cd, Cursor& c)
         return -1;
     }
 
+    // FIXIT-M: The depth on retries should not be the same as that used
+    // on the first try.
+
     const uint8_t* base = c.buffer() + pos;
     int found = cd->searcher->search(search_handle, base, depth);
 

diff --git a/src/ips_options/ips_pcre.cc b/src/ips_options/ips_pcre.cc
index ddced62840d1118915628bb64ecb95b0b181250a..b5812f99a8f29fd2d13158f9f3b079002c1b78e6 100644 (file)
--- a/src/ips_options/ips_pcre.cc
+++ b/src/ips_options/ips_pcre.cc
@@ -444,7 +444,7 @@ public:
     { return (config->options & SNORT_PCRE_RELATIVE) != 0; }
 
     EvalStatus eval(Cursor&, Packet*) override;
-    bool retry(Cursor&) override;
+    bool retry(Cursor&, const Cursor&) override;
 
     PcreData* get_data()
     { return config; }
@@ -593,7 +593,7 @@ IpsOption::EvalStatus PcreOption::eval(Cursor& c, Packet* p)
 // using content, but more advanced pcre won't work for the relative /
 // overlap case.
 
-bool PcreOption::retry(Cursor&)
+bool PcreOption::retry(Cursor&, const Cursor&)
 {
     if ((config->options & (SNORT_PCRE_INVERT | SNORT_PCRE_ANCHORED)))
     {

diff --git a/src/ips_options/ips_regex.cc b/src/ips_options/ips_regex.cc
index 4daae47106af5fe44db8ff3e2c127b6005dfd54c..0932c92f2b9e4936ede8e3ec782a6d09e2e80eb3 100644 (file)
--- a/src/ips_options/ips_regex.cc
+++ b/src/ips_options/ips_regex.cc
@@ -87,7 +87,7 @@ public:
     bool is_relative() override
     { return config.pmd.is_relative(); }
 
-    bool retry(Cursor&) override;
+    bool retry(Cursor&, const Cursor&) override;
 
     PatternMatchData* get_pattern(SnortProtocolId, RuleDirection) override
     { return &config.pmd; }
@@ -194,7 +194,7 @@ IpsOption::EvalStatus RegexOption::eval(Cursor& c, Packet*)
     return NO_MATCH;
 }
 
-bool RegexOption::retry(Cursor&)
+bool RegexOption::retry(Cursor&, const Cursor&)
 { return !is_relative(); }
 
 //-------------------------------------------------------------------------

diff --git a/src/ips_options/test/ips_regex_test.cc b/src/ips_options/test/ips_regex_test.cc
index a35a1b4f34c498056d294f362425d4dd84597063..0fbac8844d3a174fa7d9412d93cc0ed4d1dc958d 100644 (file)
--- a/src/ips_options/test/ips_regex_test.cc
+++ b/src/ips_options/test/ips_regex_test.cc
@@ -315,7 +315,7 @@ TEST(ips_regex_option, match_absolute)
     Cursor c(&pkt);
     CHECK(opt->eval(c, &pkt) == IpsOption::MATCH);
     CHECK(!strcmp((const char*) c.start(), " stew *"));
-    CHECK(opt->retry(c));
+    CHECK(opt->retry(c,c));
 }
 
 TEST(ips_regex_option, no_match_delta)
@@ -370,7 +370,7 @@ TEST(ips_regex_option_relative, no_match)
 
     CHECK(opt->is_relative());
     CHECK(opt->eval(c, &pkt) == IpsOption::NO_MATCH);
-    CHECK(!opt->retry(c));
+    CHECK(!opt->retry(c,c));
 }
 
 //-------------------------------------------------------------------------

diff --git a/src/service_inspectors/http_inspect/ips_http.cc b/src/service_inspectors/http_inspect/ips_http.cc
index 8815684c741f00d1caea1649e1fff960818e7718..d629636324bb7fe87a4f5a4f6fdef954c51b70e1 100644 (file)
--- a/src/service_inspectors/http_inspect/ips_http.cc
+++ b/src/service_inspectors/http_inspect/ips_http.cc
@@ -221,11 +221,11 @@ bool HttpIpsOption::operator==(const IpsOption& ips) const
            buffer_info == hio.buffer_info;
 }
 
-bool HttpIpsOption::retry(Cursor& c)
+bool HttpIpsOption::retry(Cursor& current_cursor, const Cursor&)
 {
     if (buffer_info.type == HTTP_BUFFER_PARAM)
     {
-        HttpCursorData* cd = (HttpCursorData*)c.get_data(HttpCursorData::id);
+        HttpCursorData* cd = (HttpCursorData*)current_cursor.get_data(HttpCursorData::id);
 
         if (cd)
             return cd->retry();

diff --git a/src/service_inspectors/http_inspect/ips_http.h b/src/service_inspectors/http_inspect/ips_http.h
index f5db9026f8af6cc4029f5d8c306f7b238180f36b..202b0fb869328ec1e31c98d6184a0f849f1954c8 100644 (file)
--- a/src/service_inspectors/http_inspect/ips_http.h
+++ b/src/service_inspectors/http_inspect/ips_http.h
@@ -102,7 +102,7 @@ public:
     EvalStatus eval(Cursor&, snort::Packet*) override;
     uint32_t hash() const override;
     bool operator==(const snort::IpsOption& ips) const override;
-    bool retry(Cursor&) override;
+    bool retry(Cursor&, const Cursor&) override;
     static IpsOption* opt_ctor(snort::Module* m, OptTreeNode*)
         { return new HttpIpsOption((HttpCursorModule*)m); }
     static void opt_dtor(snort::IpsOption* p) { delete p; }