From: Victor Julien Date: Tue, 7 Jun 2022 20:57:39 +0000 (+0200) Subject: detect/threshold: fix offline time handling issue X-Git-Tag: suricata-6.0.6~32 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=e9b084e8ca505948efec5aa37b9c9331dab6e7a9;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 ad0a6c2ba8..826ceeacdf 100644 --- 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/util-time.h b/src/util-time.h index 501a2ebc3f..a75ed2ddec 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, >); +} #ifdef UNITTESTS void TimeSet(struct timeval *);