From: Victor Julien <vjulien@oisf.net>
Date: Tue, 7 Jun 2022 20:57:39 +0000 (+0200)
Subject: detect/threshold: fix offline time handling issue
X-Git-Tag: suricata-5.0.10~19
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=72fad4a9f8be9dbc35a445fce572f8fe59e7ec27;p=thirdparty%2Fsuricata.git

detect/threshold: fix offline time handling issue

Due to the TIMEVAL_DIFF_SEC calculating the delta into an unsigned
integer, it would underflow to a high positive value leading to
and incorrect result if the packet timestamp was below the timestamp
for the threshold entry. In normal conditions this shouldn't happen,
but in offline mode each thread has its own concept of time which
might differ significantly based on the pcap. In this case the
overflow would be very common.

Changing it to a signed value calculation triggered fuzz undefined
behavior if the packet timeval was very high, so this patch takes a
new approach where it no longer calculates a diff but sets up the
"seconds" value we compare against as a timeval itself, and uses
that to compare.

Fixes: 9fafc1031c0c ("time: Add TIMEVAL_EARLIER and TIMEVAL_DIFF_SEC macros.")
Fixes: 82dc61f4c3e3 ("detect/threshold: Refactor threshold calculation to handle by_rule and by_both.")

Uses add `timeradd` specific version where available.

Bug: #5386.
(cherry picked from commit df2e408d96d0e37a0599f885dc29fff4011f8899)
---

diff --git a/src/detect-engine-threshold.c b/src/detect-engine-threshold.c
index c4229bd2cc..8fede19732 100644
--- a/src/detect-engine-threshold.c
+++ b/src/detect-engine-threshold.c
@@ -174,7 +174,8 @@ static DetectThresholdEntry* ThresholdTimeoutCheck(DetectThresholdEntry *head, s
         /* check if the 'check' timestamp is not before the creation ts.
          * This can happen due to the async nature of the host timeout
          * code that also calls this code from a management thread. */
-        if (((uint32_t)tv->tv_sec < tmp->tv_sec1) || (tv->tv_sec - tmp->tv_sec1) <= tmp->seconds) {
+        struct timeval entry = TimevalWithSeconds(&tmp->tv1, (time_t)tmp->seconds);
+        if (TimevalEarlier(tv, &entry)) {
             prev = tmp;
             tmp = tmp->next;
             continue;
@@ -333,14 +334,14 @@ static inline void RateFilterSetAction(Packet *p, PacketAlert *pa, uint8_t new_a
 * \retval int 1 if threshold reached for this entry
 *
 */
-static int IsThresholdReached(DetectThresholdEntry* lookup_tsh, const DetectThresholdData *td, uint32_t packet_time)
+static int IsThresholdReached(DetectThresholdEntry* lookup_tsh, const DetectThresholdData *td, struct timeval packet_time)
 {
     int ret = 0;
 
     /* Check if we have a timeout enabled, if so,
     * we still matching (and enabling the new_action) */
     if (lookup_tsh->tv_timeout != 0) {
-        if ((packet_time - lookup_tsh->tv_timeout) > td->timeout) {
+        if ((packet_time.tv_sec - lookup_tsh->tv_timeout) > td->timeout) {
             /* Ok, we are done, timeout reached */
             lookup_tsh->tv_timeout = 0;
         }
@@ -352,17 +353,17 @@ static int IsThresholdReached(DetectThresholdEntry* lookup_tsh, const DetectThre
     }
     else {
         /* Update the matching state with the timeout interval */
-        if ((packet_time - lookup_tsh->tv_sec1) < td->seconds) {
+        struct timeval entry = TimevalWithSeconds(&lookup_tsh->tv1, (time_t)td->seconds);
+        if (TimevalEarlier(&packet_time, &entry)) {
             lookup_tsh->current_count++;
             if (lookup_tsh->current_count > td->count) {
                 /* Then we must enable the new action by setting a
                 * timeout */
-                lookup_tsh->tv_timeout = packet_time;
+                lookup_tsh->tv_timeout = packet_time.tv_sec;
                 ret = 1;
             }
-        }
-        else {
-            lookup_tsh->tv_sec1 = packet_time;
+        } else {
+            lookup_tsh->tv1 = packet_time;
             lookup_tsh->current_count = 1;
         }
     } /* else - if (lookup_tsh->tv_timeout != 0) */
@@ -370,22 +371,22 @@ static int IsThresholdReached(DetectThresholdEntry* lookup_tsh, const DetectThre
     return ret;
 }
 
-static void AddEntryToHostStorage(Host *h, DetectThresholdEntry *e, uint32_t packet_time)
+static void AddEntryToHostStorage(Host *h, DetectThresholdEntry *e, struct timeval packet_time)
 {
     if (h && e) {
         e->current_count = 1;
-        e->tv_sec1 = packet_time;
+        e->tv1 = packet_time;
         e->tv_timeout = 0;
         e->next = HostGetStorageById(h, host_threshold_id);
         HostSetStorageById(h, host_threshold_id, e);
     }
 }
 
-static void AddEntryToIPPairStorage(IPPair *pair, DetectThresholdEntry *e, uint32_t packet_time)
+static void AddEntryToIPPairStorage(IPPair *pair, DetectThresholdEntry *e, struct timeval packet_time)
 {
     if (pair && e) {
         e->current_count = 1;
-        e->tv_sec1 = packet_time;
+        e->tv1 = packet_time;
         e->tv_timeout = 0;
         e->next = IPPairGetStorageById(pair, ippair_threshold_id);
         IPPairSetStorageById(pair, ippair_threshold_id, e);
@@ -405,11 +406,11 @@ static int ThresholdHandlePacketIPPair(IPPair *pair, Packet *p, const DetectThre
         {
             SCLogDebug("rate_filter");
             ret = 1;
-            if (lookup_tsh && IsThresholdReached(lookup_tsh, td, p->ts.tv_sec)) {
+            if (lookup_tsh && IsThresholdReached(lookup_tsh, td, p->ts)) {
                 RateFilterSetAction(p, pa, td->new_action);
             } else if (!lookup_tsh) {
                 DetectThresholdEntry *e = DetectThresholdEntryAlloc(td, p, sid, gid);
-                AddEntryToIPPairStorage(pair, e, p->ts.tv_sec);
+                AddEntryToIPPairStorage(pair, e, p->ts);
             }
             break;
         }
@@ -442,7 +443,8 @@ static int ThresholdHandlePacketHost(Host *h, Packet *p, const DetectThresholdDa
             SCLogDebug("limit");
 
             if (lookup_tsh != NULL)  {
-                if ((p->ts.tv_sec - lookup_tsh->tv_sec1) < td->seconds) {
+                struct timeval entry = TimevalWithSeconds(&lookup_tsh->tv1, (time_t)td->seconds);
+                if (TimevalEarlier(&p->ts, &entry)) {
                     lookup_tsh->current_count++;
 
                     if (lookup_tsh->current_count <= td->count) {
@@ -450,8 +452,8 @@ static int ThresholdHandlePacketHost(Host *h, Packet *p, const DetectThresholdDa
                     } else {
                         ret = 2;
                     }
-                } else    {
-                    lookup_tsh->tv_sec1 = p->ts.tv_sec;
+                } else {
+                    lookup_tsh->tv1 = p->ts;
                     lookup_tsh->current_count = 1;
 
                     ret = 1;
@@ -462,7 +464,7 @@ static int ThresholdHandlePacketHost(Host *h, Packet *p, const DetectThresholdDa
                     break;
                 }
 
-                e->tv_sec1 = p->ts.tv_sec;
+                e->tv1 = p->ts;
                 e->current_count = 1;
 
                 ret = 1;
@@ -477,7 +479,8 @@ static int ThresholdHandlePacketHost(Host *h, Packet *p, const DetectThresholdDa
             SCLogDebug("threshold");
 
             if (lookup_tsh != NULL)  {
-                if ((p->ts.tv_sec - lookup_tsh->tv_sec1) < td->seconds) {
+                struct timeval entry = TimevalWithSeconds(&lookup_tsh->tv1, (time_t)td->seconds);
+                if (TimevalEarlier(&p->ts, &entry)) {
                     lookup_tsh->current_count++;
 
                     if (lookup_tsh->current_count >= td->count) {
@@ -485,7 +488,7 @@ static int ThresholdHandlePacketHost(Host *h, Packet *p, const DetectThresholdDa
                         lookup_tsh->current_count = 0;
                     }
                 } else {
-                    lookup_tsh->tv_sec1 = p->ts.tv_sec;
+                    lookup_tsh->tv1 = p->ts;
                     lookup_tsh->current_count = 1;
                 }
             } else {
@@ -498,7 +501,7 @@ static int ThresholdHandlePacketHost(Host *h, Packet *p, const DetectThresholdDa
                     }
 
                     e->current_count = 1;
-                    e->tv_sec1 = p->ts.tv_sec;
+                    e->tv1 = p->ts;
 
                     e->next = HostGetStorageById(h, host_threshold_id);
                     HostSetStorageById(h, host_threshold_id, e);
@@ -511,7 +514,8 @@ static int ThresholdHandlePacketHost(Host *h, Packet *p, const DetectThresholdDa
             SCLogDebug("both");
 
             if (lookup_tsh != NULL) {
-                if ((p->ts.tv_sec - lookup_tsh->tv_sec1) < td->seconds) {
+                struct timeval entry = TimevalWithSeconds(&lookup_tsh->tv1, (time_t)td->seconds);
+                if (TimevalEarlier(&p->ts, &entry)) {
                     /* within time limit */
 
                     lookup_tsh->current_count++;
@@ -523,7 +527,7 @@ static int ThresholdHandlePacketHost(Host *h, Packet *p, const DetectThresholdDa
                     }
                 } else {
                     /* expired, so reset */
-                    lookup_tsh->tv_sec1 = p->ts.tv_sec;
+                    lookup_tsh->tv1 = p->ts;
                     lookup_tsh->current_count = 1;
 
                     /* if we have a limit of 1, this is a match */
@@ -538,7 +542,7 @@ static int ThresholdHandlePacketHost(Host *h, Packet *p, const DetectThresholdDa
                 }
 
                 e->current_count = 1;
-                e->tv_sec1 = p->ts.tv_sec;
+                e->tv1 = p->ts;
 
                 e->next = HostGetStorageById(h, host_threshold_id);
                 HostSetStorageById(h, host_threshold_id, e);
@@ -557,10 +561,8 @@ static int ThresholdHandlePacketHost(Host *h, Packet *p, const DetectThresholdDa
             SCLogDebug("detection_filter");
 
             if (lookup_tsh != NULL) {
-                long double time_diff = ((p->ts.tv_sec + p->ts.tv_usec/1000000.0) -
-                                         (lookup_tsh->tv_sec1 + lookup_tsh->tv_usec1/1000000.0));
-
-                if (time_diff < td->seconds) {
+                struct timeval entry = TimevalWithSeconds(&lookup_tsh->tv1, (time_t)td->seconds);
+                if (TimevalEarlier(&p->ts, &entry)) {
                     /* within timeout */
 
                     lookup_tsh->current_count++;
@@ -569,9 +571,7 @@ static int ThresholdHandlePacketHost(Host *h, Packet *p, const DetectThresholdDa
                     }
                 } else {
                     /* expired, reset */
-
-                    lookup_tsh->tv_sec1 = p->ts.tv_sec;
-                    lookup_tsh->tv_usec1 = p->ts.tv_usec;
+                    lookup_tsh->tv1 = p->ts;
                     lookup_tsh->current_count = 1;
                 }
             } else {
@@ -581,8 +581,7 @@ static int ThresholdHandlePacketHost(Host *h, Packet *p, const DetectThresholdDa
                 }
 
                 e->current_count = 1;
-                e->tv_sec1 = p->ts.tv_sec;
-                e->tv_usec1 = p->ts.tv_usec;
+                e->tv1 = p->ts;
 
                 e->next = HostGetStorageById(h, host_threshold_id);
                 HostSetStorageById(h, host_threshold_id, e);
@@ -594,11 +593,11 @@ static int ThresholdHandlePacketHost(Host *h, Packet *p, const DetectThresholdDa
         {
             SCLogDebug("rate_filter");
             ret = 1;
-            if (lookup_tsh && IsThresholdReached(lookup_tsh, td, p->ts.tv_sec)) {
+            if (lookup_tsh && IsThresholdReached(lookup_tsh, td, p->ts)) {
                 RateFilterSetAction(p, pa, td->new_action);
             } else if (!lookup_tsh) {
                 DetectThresholdEntry *e = DetectThresholdEntryAlloc(td, p, sid, gid);
-                AddEntryToHostStorage(h, e, p->ts.tv_sec);
+                AddEntryToHostStorage(h, e, p->ts);
             }
             break;
         }
@@ -622,14 +621,14 @@ static int ThresholdHandlePacketRule(DetectEngineCtx *de_ctx, Packet *p,
         case TYPE_RATE:
         {
             ret = 1;
-            if (lookup_tsh && IsThresholdReached(lookup_tsh, td, p->ts.tv_sec)) {
+            if (lookup_tsh && IsThresholdReached(lookup_tsh, td, p->ts)) {
                 RateFilterSetAction(p, pa, td->new_action);
             }
             else if (!lookup_tsh) {
                 DetectThresholdEntry *e = DetectThresholdEntryAlloc(td, p, s->id, s->gid);
                 if (e != NULL) {
                     e->current_count = 1;
-                    e->tv_sec1 = p->ts.tv_sec;
+                    e->tv1 = p->ts;
                     e->tv_timeout = 0;
 
                     de_ctx->ths_ctx.th_entry[s->num] = e;
diff --git a/src/detect-threshold.h b/src/detect-threshold.h
index 8b410e0148..fc6b7a24fe 100644
--- a/src/detect-threshold.h
+++ b/src/detect-threshold.h
@@ -77,6 +77,7 @@ typedef struct DetectThresholdEntry_ {
     uint32_t current_count; /**< Var for count control */
     int track;          /**< Track type: by_src, by_src */
 
+    struct timeval tv1;     /**< Var for time control */
     struct DetectThresholdEntry_ *next;
 } DetectThresholdEntry;
 
diff --git a/src/util-time.h b/src/util-time.h
index 9e189176db..23aa0af46b 100644
--- a/src/util-time.h
+++ b/src/util-time.h
@@ -33,16 +33,28 @@ void TimeGet(struct timeval *);
 /** \brief intialize a 'struct timespec' from a 'struct timeval'. */
 #define FROM_TIMEVAL(timev) { .tv_sec = (timev).tv_sec, .tv_nsec = (timev).tv_usec * 1000 }
 
-/** \brief compare two 'struct timeval' and return the difference in seconds */
-#define TIMEVAL_DIFF_SEC(tv_new, tv_old) \
-    (uint64_t)((((uint64_t)(tv_new).tv_sec * 1000000 + (tv_new).tv_usec) - \
-                ((uint64_t)(tv_old).tv_sec * 1000000 + (tv_old).tv_usec)) / \
-               1000000)
+static inline struct timeval TimevalWithSeconds(const struct timeval *ts, const time_t sec_add)
+{
+#ifdef timeradd
+    struct timeval add = { .tv_sec = sec_add, .tv_usec = 0 };
+    struct timeval result;
+    timeradd(ts, &add, &result);
+    return result;
+#else
+    const time_t sec = ts->tv_sec + sec_add;
+    struct timeval result = { .tv_sec = sec, .tv_usec = ts->tv_usec };
+    return result;
+#endif
+}
 
 /** \brief compare two 'struct timeval' and return if the first is earlier than the second */
-#define TIMEVAL_EARLIER(tv_first, tv_second) \
-    (((tv_first).tv_sec < (tv_second).tv_sec) || \
-     ((tv_first).tv_sec == (tv_second).tv_sec && (tv_first).tv_usec < (tv_second).tv_usec))
+static inline bool TimevalEarlier(struct timeval *first, struct timeval *second)
+{
+    /* from man timercmp on Linux: "Some systems (but not Linux/glibc), have a broken timercmp()
+     * implementation, in which CMP of >=, <=, and == do not work; portable applications can instead
+     * use ... !timercmp(..., >) */
+    return !timercmp(first, second, >);
+}
 
 #ifndef timeradd
 #define timeradd(a, b, r)                                                                          \