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 c4229bd2cc14f6eca32121fe7cf948355afe67ed..8fede1973212adba85e5308b59e264c836d4dcd7 100644 (file)
--- 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 8b410e01480d12de231ac836ed7ac03305195714..fc6b7a24fe1e7946876135620170bd0b9919cb8d 100644 (file)
--- 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 9e189176dbe856e534f54c0c6eadd39b84b60d7e..23aa0af46b6c238ffd13d6b22dc48d2bd6926f84 100644 (file)
--- 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)                                                                          \