Pull request #3581: ips_options: content retry

Merge in SNORT/snort3 from ~ASERBENI/snort3:russ_content_retry to master

Squashed commit of the following:

commit 25963f71586de50ed65369b8823a3bd3e2513d98
Author: Vitalii <vhorbato@cisco.com>
Date:   Tue Sep 13 15:33:25 2022 +0300

    trace: ips variables are dumped as hex

commit 0efc6a4894c4b65e7b872236b4d8c7bc63e362cd
Author: russ <rucombs@cisco.com>
Date:   Mon Aug 15 21:00:31 2022 -0400

    content: fix retry

    This deprecates the 2nd/"orig" cursor argument to retry.
    The existing Cursor.delta member provides the required information.

    The use of byte_extract variables is also fixed for content.
    Those valuse are used as sizes or offsets and can not be negative.

commit f32ed3f56ce9dab991d7951c9c9107fe83137323
Author: russ <rucombs@cisco.com>
Date:   Tue Aug 16 11:40:40 2022 -0400

    ips: trace all node evaluations

    Iterative evaluations due to retry were not previously traced.

diff --git a/src/detection/detection_options.cc b/src/detection/detection_options.cc
index 488bcb6a647cb3de40404302d5a802a26d140fde..ab2fb627a28bd180bc51ec5d9110ba238569dd62 100644 (file)
--- a/src/detection/detection_options.cc
+++ b/src/detection/detection_options.cc
@@ -565,7 +565,7 @@ int detection_option_node_evaluate(
             rule_vars.reserve(sizeof(var_buf));
             for ( unsigned i = 0; i < NUM_IPS_OPTIONS_VARS; ++i )
             {
-                safe_snprintf(var_buf, sizeof(var_buf), "var[%d]=%d ", i, tmp_byte_extract_vars[i]);
+                safe_snprintf(var_buf, sizeof(var_buf), "var[%u]=0x%X ", i, tmp_byte_extract_vars[i]);
                 rule_vars.append(var_buf);
             }
             debug_logf(detection_trace, TRACE_RULE_VARS, p, "Rule options variables: %s\n",
@@ -698,8 +698,10 @@ int detection_option_node_evaluate(
         // We're essentially checking this node again and it potentially
         // might match again
         if ( continue_loop )
+        {
             state.checks++;
-
+            node_eval_trace(node, cursor, eval_data.p);
+        }
         loop_count++;
     }
     while ( continue_loop );

diff --git a/src/detection/pattern_match_data.h b/src/detection/pattern_match_data.h
index 8297ea6f0c929ca19e3464304681fd96a8431101..e0a1feceb6f3806eb88aa76edfd77abd0659e79f 100644 (file)
--- a/src/detection/pattern_match_data.h
+++ b/src/detection/pattern_match_data.h
@@ -57,7 +57,7 @@ struct PatternMatchData
     unsigned pattern_size;   // size of app layer pattern
 
     int offset;              // pattern search start offset
-    int depth;               // pattern search depth
+    unsigned depth;          // pattern search depth
 
     enum
     {

diff --git a/src/framework/ips_option.h b/src/framework/ips_option.h
index cbd935bcaeefce1ca42d68656e9c28c98de96860..9264832e220f5e5bd928242042ca5349378bae1c 100644 (file)
--- a/src/framework/ips_option.h
+++ b/src/framework/ips_option.h
@@ -78,6 +78,8 @@ public:
 
     // packet threads
     virtual bool is_relative() { return false; }
+
+    // 2nd cursor is deprecated, do not use
     virtual bool retry(Cursor&, const Cursor&) { return false; }
     virtual void action(Packet*) { }
 

diff --git a/src/ips_options/ips_content.cc b/src/ips_options/ips_content.cc
index 1b00b05ca8b165b54836b6ac9a0d69038177f6ab..2083d659f8b42ad0312d9914c9ef0339205b0f86 100644 (file)
--- a/src/ips_options/ips_content.cc
+++ b/src/ips_options/ips_content.cc
@@ -163,7 +163,7 @@ protected:
     ContentData* config;
 };
 
-bool ContentOption::retry(Cursor& current_cursor, const Cursor& orig_cursor)
+bool ContentOption::retry(Cursor& c, const Cursor&)
 {
     if ( config->pmd.is_negated() )
         return false;
@@ -171,12 +171,7 @@ bool ContentOption::retry(Cursor& current_cursor, const Cursor& orig_cursor)
     if ( !config->pmd.depth )
         return true;
 
-    // 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;
+    return c.get_delta() + config->pmd.pattern_size <= config->pmd.depth;
 }
 
 uint32_t ContentOption::hash() const
@@ -277,14 +272,15 @@ bool ContentOption::operator==(const IpsOption& ips) const
  */
 static int uniSearchReal(ContentData* cd, Cursor& c)
 {
-    int offset, depth;
+    // byte_extract variables are strictly unsigned, used for sizes and forward offsets
+    // converting from uint32_t to int64_t ensures all extracted values remain positive
+    int64_t offset, depth;
 
-    /* Get byte_extract variables */
     if (cd->offset_var >= 0 && cd->offset_var < NUM_IPS_OPTIONS_VARS)
     {
         uint32_t extract;
         GetVarValueByIndex(&extract, cd->offset_var);
-        offset = (int)extract;
+        offset = extract;
     }
     else
         offset = cd->pmd.offset;
@@ -293,13 +289,12 @@ static int uniSearchReal(ContentData* cd, Cursor& c)
     {
         uint32_t extract;
         GetVarValueByIndex(&extract, cd->depth_var);
-        depth = (int)extract;
+        depth = extract;
     }
     else
         depth = cd->pmd.depth;
 
-    int pos = c.get_delta();
-    int file_pos = c.get_file_pos();
+    uint32_t file_pos = c.get_file_pos();
 
     if (file_pos and cd->offset_set)
     {
@@ -307,28 +302,35 @@ static int uniSearchReal(ContentData* cd, Cursor& c)
         if (offset < 0)
             return 0;
     }
-    if ( !pos )
+
+    int64_t pos;
+
+    if ( !c.get_delta() )
+        // first - adjust from cursor or buffer start
+        pos = (cd->pmd.is_relative() ? c.get_pos() : 0) + offset;
+    else
     {
-        if ( cd->pmd.is_relative() )
-            pos = c.get_pos();
+        // retry - adjust from start of last match
+        pos = c.get_pos() - cd->pmd.pattern_size + cd->match_delta;
 
-        pos += offset;
+        if ( depth )
+            depth -= c.get_delta();
     }
 
-    if ( pos < 0 )
-        pos = 0;
+    if ( pos < 0 or depth < 0 or pos >= c.size() )
+        return -1;
 
-    int len = c.size() - pos;
+    int64_t len = c.size() - pos;
 
     if ( !depth || len < depth )
         depth = len;
 
-    unsigned end = pos + cd->pmd.pattern_size;
+    int64_t end = pos + cd->pmd.pattern_size;
 
     // If the pattern size is greater than the amount of data we have to
     // search, there's no way we can match, but return 0 here for the
     // case where the match is inverted and there is at least some data.
-    if ( end > c.size() || (int)end > pos + depth )
+    if ( end > c.size() || end > pos + depth )
     {
         if ( cd->pmd.is_negated() && (depth > 0) )
             return 0;
@@ -336,17 +338,13 @@ 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);
+    int found = cd->searcher->search(search_handle, base, (unsigned)depth);
 
     if ( found >= 0 )
     {
-        int at = pos + found;
-        c.set_delta(at + cd->match_delta);
-        c.set_pos(at + cd->pmd.pattern_size);
+        c.set_delta(c.get_delta() + found + cd->match_delta);
+        c.set_pos(pos + found + cd->pmd.pattern_size);
         return 1;
     }
 
@@ -484,15 +482,7 @@ static void parse_depth(ContentData* cd, const char* data)
 
     if (isdigit(data[0]) || data[0] == '-')
     {
-        cd->pmd.depth = parse_int(data, "depth");
-
-        /* check to make sure that this the depth allows this rule to fire */
-        if (cd->pmd.depth < (int)cd->pmd.pattern_size)
-        {
-            ParseError("the depth (%d) is less than the size of the content(%u)",
-                cd->pmd.depth, cd->pmd.pattern_size);
-            return;
-        }
+        cd->pmd.depth = parse_int(data, "depth", cd->pmd.pattern_size);
         cd->depth_var = IPS_OPTIONS_NO_VAR;
     }
     else
@@ -555,13 +545,7 @@ static void parse_within(ContentData* cd, const char* data)
 
     if (isdigit(data[0]) || data[0] == '-')
     {
-        cd->pmd.depth = parse_int(data, "within");
-
-        if (cd->pmd.depth < (int)cd->pmd.pattern_size)
-        {
-            ParseError("within (%d) is smaller than size of pattern", cd->pmd.depth);
-            return;
-        }
+        cd->pmd.depth = parse_int(data, "within", cd->pmd.pattern_size);
         cd->depth_var = IPS_OPTIONS_NO_VAR;
     }
     else
@@ -574,7 +558,6 @@ static void parse_within(ContentData* cd, const char* data)
         }
     }
 
-
     cd->pmd.set_relative();
 }
 

diff --git a/src/parser/parse_utils.cc b/src/parser/parse_utils.cc
index 13c0a252fbafeec5d85bc49617fd670724302c92..b06650ff9c12a7325249226ad3db485143c79e2e 100644 (file)
--- a/src/parser/parse_utils.cc
+++ b/src/parser/parse_utils.cc
@@ -144,10 +144,72 @@ int parse_int(const char* data, const char* tag, int low, int high)
 
     if ((value > high) || (value < low))
     {
-        ParseError("'%s' must in %d:%d", tag, low, high);
+        ParseError("'%s' must be in %d:%d, inclusive", tag, low, high);
         return value;
     }
 
     return value;
 }
 
+//--------------------------------------------------------------------------
+// unit tests
+//--------------------------------------------------------------------------
+
+#ifdef UNIT_TEST
+
+#include "catch/snort_catch.h"
+
+TEST_CASE("parse_int", "[ParseUtils]")
+{
+    SECTION("invalid format")
+    {
+        int res = parse_int("10  ", "test");
+
+        CHECK(res == 10);
+    }
+
+    SECTION("not a number")
+    {
+        const char* data = "test";
+
+        int res = parse_int(data, data);
+
+        CHECK(res == 0);
+    }
+
+    SECTION("int overflow")
+    {
+        const char* data = "99999999999999999999999999999999999999999999999999";
+
+        int res = parse_int(data, "test");
+
+        CHECK(res == -1);
+        CHECK(errno == ERANGE);
+    }
+
+    SECTION("int underflow")
+    {
+        const char* data = "-9999999999999999999999999999999999999999999999999";
+
+        int res = parse_int(data, "test");
+
+        CHECK(res == 0);
+        CHECK(errno == ERANGE);
+    }
+
+    SECTION("below the limit")
+    {
+        int res = parse_int("1", "test", 2, 3);
+
+        CHECK(res == 1);
+    }
+
+    SECTION("above the limit")
+    {
+        int res = parse_int("4", "test", 2, 3);
+
+        CHECK(res == 4);
+    }
+}
+
+#endif