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.

diff --git a/src/detect-engine-threshold.c b/src/detect-engine-threshold.c
index 878d1d91aa920a29f4f65a7bbf61f5152790feda..d7d0ce50bb18b21e511d755fc114b2e4bc61078a 100644 (file)
--- a/src/detect-engine-threshold.c
+++ b/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) {

diff --git a/src/stream-tcp.c b/src/stream-tcp.c
index 60c154f8d211b871da8fd04556279d78ed724098..8d1226aa3c0c4d319f864d18f45c4cfaa4edf3a4 100644 (file)
--- a/src/stream-tcp.c
+++ b/src/stream-tcp.c
@@ -6465,8 +6465,8 @@ int StreamTcpSegmentForSession(
             }
             server_node = TCPSEG_RB_NEXT(server_node);
         } else {
-            if (TIMEVAL_EARLIER(
-                        client_node->pcap_hdr_storage->ts, server_node->pcap_hdr_storage->ts)) {
+            if (TimevalEarlier(
+                        &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);

diff --git a/src/util-time.h b/src/util-time.h
index f33e094affb73974c0779718031e5533a9cd775c..79e51e3697c9aa98cf8a516c537e8593544dd339 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, >);
+}
 
 #ifdef UNITTESTS
 void TimeSet(struct timeval *);