Thresholding broken for ip lists

Developers
1

Bug #4377: 64 Character Limit on IP for thresholding.conf - Suricata - Open Information Security Foundation hasn’t seen any activity. It seems like a patch as simple as this would solve the problem for many (most?) cases:

diff --git a/src/util-threshold-config.c b/src/util-threshold-config.c
index b08935db0..caae8c198 100644
--- a/src/util-threshold-config.c
+++ b/src/util-threshold-config.c
@@ -77,6 +77,7 @@ static FILE *g_ut_threshold_fp = NULL;
  *  suppress gen_id 0, sig_id 0, track by_dst, ip 10.88.0.14
  *  suppress gen_id 1, sig_id 2000328
  *  suppress gen_id 1, sig_id 2000328, track by_src, ip fe80::/10
+ * n.b., ip can be [IP, ...]
 */
 #define DETECT_SUPPRESS_REGEX "^,\\s*track\\s*(by_dst|by_src|by_either)\\s*,\\s*ip\\s*([\\[\\],\\$\\s\\da-zA-Z.:/_]+)*\\s*$"
 
@@ -655,7 +656,7 @@ static int ParseThresholdRule(DetectEngineCtx *de_ctx, char *rawstr,
     char th_seconds[16] = "";
     char th_new_action[16] = "";
     char th_timeout[16] = "";
-    char th_ip[64] = "";
+    char th_ip[1024] = "";
 
     uint8_t parsed_type = 0;
     uint8_t parsed_track = 0;

Having an arbitrary (undocumented) limit on the length of an IP list is somewhat ugly, is there a reason this is allocated statically rather than dynamically? Later in the code this static string is strdup’d, and then freed in the caller, making this even more confusing. The problem was introduced by threshold: Change rule parsing to use pcre_copy_substring · OISF/suricata@81d7a7a · GitHub and the accompanying comment says something about a memory leak, and that the IP can’t be more than 48 characters. I suspect the 64 comes from 48 plus “extra”, without realizing that the IP can be a list. (Hence the added comment in the diff above to help avoid future problems.) I’m not sure what the details of the memory leak were, and if simply reverting the threshold IP to use pcre_get_substring would make more sense than a static buffer.

Somewhat related: every single instance of pcre_copy_substring emits the same SCLogError(SC_ERR_PCRE_COPY_SUBSTRING, "pcre_copy_substring failed") message on error. Is there a reason that there’s nothing to indicate which copy failed? This makes it extremely difficult for an end-user to diagnose what’s going on or even where the problem exists. (The suricata log file mentions after the error some statistics about thresholds, but nothing prior to or within the error message indicates what file and line have a problem.)

2

I’m seeing this as well.

3

This has been fixed in master and will be backported to at least 6.0.3.

4

Appreciate it…will wait for 6.0.3 to continue testing.