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: 9fafc1031c ("time: Add TIMEVAL_EARLIER and TIMEVAL_DIFF_SEC macros.")
Fixes: 82dc61f4c3 ("detect/threshold: Refactor threshold calculation to handle by_rule and by_both.")

Uses add `timeradd` specific version where available.

Bug: #5386.

src/detect-engine-threshold.c

@@ -167,7 +167,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 (TIMEVAL_EARLIER(*tv, tmp->tv1) || TIMEVAL_DIFF_SEC(*tv, tmp->tv1) <= tmp->seconds) {
+        struct timeval entry = TimevalWithSeconds(&tmp->tv1, (time_t)tmp->seconds);
+        if (TimevalEarlier(tv, &entry)) {
             prev = tmp;
             tmp = tmp->next;
             continue;
@@ -345,7 +346,8 @@ static int IsThresholdReached(DetectThresholdEntry* lookup_tsh, const DetectThre
     }
     else {
         /* Update the matching state with the timeout interval */
-        if (TIMEVAL_DIFF_SEC(packet_time, lookup_tsh->tv1) < 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
@@ -353,8 +355,7 @@ static int IsThresholdReached(DetectThresholdEntry* lookup_tsh, const DetectThre
                 lookup_tsh->tv_timeout = packet_time.tv_sec;
                 ret = 1;
             }
-        }
+        } else {
-        else {
             lookup_tsh->tv1 = packet_time;
             lookup_tsh->current_count = 1;
         }
@@ -405,7 +406,8 @@ static int ThresholdHandlePacket(Packet *p, DetectThresholdEntry *lookup_tsh,
             SCLogDebug("limit");
 
             if (lookup_tsh != NULL)  {
-                if (TIMEVAL_DIFF_SEC(p->ts, lookup_tsh->tv1) < 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) {
@@ -413,7 +415,7 @@ static int ThresholdHandlePacket(Packet *p, DetectThresholdEntry *lookup_tsh,
                     } else {
                         ret = 2;
                     }
-                } else    {
+                } else {
                     lookup_tsh->tv1 = p->ts;
                     lookup_tsh->current_count = 1;
 
@@ -431,7 +433,8 @@ static int ThresholdHandlePacket(Packet *p, DetectThresholdEntry *lookup_tsh,
             SCLogDebug("threshold");
 
             if (lookup_tsh != NULL)  {
-                if (TIMEVAL_DIFF_SEC(p->ts, lookup_tsh->tv1) < 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) {
@@ -456,7 +459,8 @@ static int ThresholdHandlePacket(Packet *p, DetectThresholdEntry *lookup_tsh,
             SCLogDebug("both");
 
             if (lookup_tsh != NULL) {
-                if (TIMEVAL_DIFF_SEC(p->ts, lookup_tsh->tv1) < 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++;
@@ -493,7 +497,8 @@ static int ThresholdHandlePacket(Packet *p, DetectThresholdEntry *lookup_tsh,
             SCLogDebug("detection_filter");
 
             if (lookup_tsh != NULL) {
-                if (TIMEVAL_DIFF_SEC(p->ts, lookup_tsh->tv1) < td->seconds) {
+                struct timeval entry = TimevalWithSeconds(&lookup_tsh->tv1, (time_t)td->seconds);
+                if (TimevalEarlier(&p->ts, &entry)) {
                     /* within timeout */
                     lookup_tsh->current_count++;
                     if (lookup_tsh->current_count > td->count) {

src/stream-tcp.c

@@ -6465,8 +6465,8 @@ int StreamTcpSegmentForSession(
             }
             server_node = TCPSEG_RB_NEXT(server_node);
         } else {
-            if (TIMEVAL_EARLIER(
+            if (TimevalEarlier(
-                        client_node->pcap_hdr_storage->ts, server_node->pcap_hdr_storage->ts)) {
+                        &client_node->pcap_hdr_storage->ts, &server_node->pcap_hdr_storage->ts)) {
                 StreamingBufferSegmentGetData(
                         &client_stream->sb, &client_node->sbseg, &seg_data, &seg_datalen);
                 ret = CallbackFunc(p, client_node, data, seg_data, seg_datalen);

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 */
+static inline struct timeval TimevalWithSeconds(const struct timeval *ts, const time_t sec_add)
-#define TIMEVAL_DIFF_SEC(tv_new, tv_old) \
+{
-    (uint64_t)((((uint64_t)(tv_new).tv_sec * 1000000 + (tv_new).tv_usec) - \
+#ifdef timeradd
-                ((uint64_t)(tv_old).tv_sec * 1000000 + (tv_old).tv_usec)) / \
+    struct timeval add = { .tv_sec = sec_add, .tv_usec = 0 };
-               1000000)
+    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) \
+static inline bool TimevalEarlier(struct timeval *first, struct timeval *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))
+    /* 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, >);
+}
 
 #ifdef UNITTESTS
 void TimeSet(struct timeval *);