ping: Fix signed 64-bit integer overflow in RTT calculation by pevik · Pull Request #585 · iputils/iputils

pevik · 2025-05-05T23:45:47Z

Crafted ICMP Echo Reply packet can cause signed integer overflow in

triptime calculation:
triptime = tv->tv_sec * 1000000 + tv->tv_usec;
tsum2 increment which uses triptime
rts->tsum2 += (double)((long long)triptime * (long long)triptime);
final tmvar:
tmvar = (rts->tsum2 / total) - (tmavg * tmavg)

$ export CFLAGS="-O1 -g -fsanitize=address,undefined -fno-omit-frame-pointer"
$ export LDFLAGS="-fsanitize=address,undefined -fno-omit-frame-pointer"
$ meson setup .. -Db_sanitize=address,undefined
$ ninja
$ ./ping/ping -c2 127.0.0.1

PING 127.0.0.1 (127.0.0.1) 56(84) bytes of data.
64 bytes from 127.0.0.1: icmp_seq=1 ttl=64 time=0.061 ms
../ping/ping_common.c:757:25: runtime error: signed integer overflow: -2513732689199106 * 1000000 cannot be represented in type 'long int'
../ping/ping_common.c:757:12: runtime error: signed integer overflow: -4975495174606980224 + -6510615555425289427 cannot be represented in type 'long int'
../ping/ping_common.c:769:47: runtime error: signed integer overflow: 6960633343677281965 * 6960633343677281965 cannot be represented in type 'long int'
24 bytes from 127.0.0.1: icmp_seq=1 ttl=64 (truncated)
./ping/ping: Warning: time of day goes back (-7256972569576721377us), taking countermeasures
./ping/ping: Warning: time of day goes back (-7256972569576721232us), taking countermeasures
24 bytes from 127.0.0.1: icmp_seq=1 ttl=64 (truncated)
../ping/ping_common.c:265:16: runtime error: signed integer overflow: 6960633343677281965 * 2 cannot be represented in type 'long int'
64 bytes from 127.0.0.1: icmp_seq=2 ttl=64 time=0.565 ms

--- 127.0.0.1 ping statistics ---
2 packets transmitted, 2 received, +2 duplicates, 0% packet loss, time 1002ms
../ping/ping_common.c:940:42: runtime error: signed integer overflow: 1740158335919320832 * 1740158335919320832 cannot be represented in type 'long int'
rtt min/avg/max/mdev = 0.000/1740158335919320.832/6960633343677281.965/-1623514645242292.-224 ms

To fix the overflow check allowed ranges of struct timeval members:

tv_sec <-LONG_MAX/1000000, LONG_MAX/1000000>
tv_usec <0, 999999>

Fix includes 2 new error messages (needs translation).

After fix:

$ ./ping/ping -c2 127.0.0.1
PING 127.0.0.1 (127.0.0.1) 56(84) bytes of data.
64 bytes from 127.0.0.1: icmp_seq=1 ttl=64 time=0.059 ms
./ping/ping: Warning: overflow tv_usec -6510615555425457380 us
./ping/ping: Warning: invalid tv_sec -1789369274859522 s
24 bytes from 127.0.0.1: icmp_seq=1 ttl=64 (truncated)
./ping/ping: Warning: overflow tv_usec -6510615555425413387 us
./ping/ping: Warning: invalid tv_sec -2006106209517570 s
24 bytes from 127.0.0.1: icmp_seq=1 ttl=64 (truncated)
64 bytes from 127.0.0.1: icmp_seq=2 ttl=64 time=0.118 ms

--- 127.0.0.1 ping statistics ---
2 packets transmitted, 2 received, +2 duplicates, 0% packet loss, time 1002ms
rtt min/avg/max/mdev = 0.000/0.044/0.118/0.048 ms

Fixes: #584
Fixes: CVE-2025-472
Link: https://github.com/Zephkek/ping-rtt-overflow/
Co-developed-by: Cyril Hrubis chrubis@suse.cz
Reported-by: Mohamed Maatallah hotelsmaatallahrecemail@gmail.com

nmeyerhans · 2025-05-06T00:53:24Z

+		}
+
+		/* 1000001 = 1000000 tv_sec + 1 tv_usec */
+		if (tv->tv_sec > LONG_MAX/1000001 || tv->tv_sec < -LONG_MAX/1000001) {


It might be nice to give a name to LONG_MAX/1000001, maybe with #define?

Something like this would be nice:

#define USEC_PER_SEC 1000000 #define USEC_MAX (USEC_PER_SEC - 1) #define SEC_SAFE_MAX (LONG_MAX / (USEC_PER_SEC + 1))

These replace magic numbers and create a clear boundary for detecting integer overflow when converting time units.

Although the other 2 definitions make sense, I would prefer to postpone adding them later after this is fixed (it's an unrelated cleanup - 1000000 should be used on more places not just here).

pevik · 2025-05-06T07:34:26Z

Please, when you're finish with your review, add your Reviewed-by: or Acked-by: tag.

metan-ucw · 2025-05-06T07:45:56Z

+			tv->tv_usec = 0;
+		}
+
+		if (tv->tv_sec > TV_SEC_MAX_VAL || tv->tv_sec < -TV_SEC_MAX_VAL) {


Isn't tv->tv_sec < 0 invalid anyway? That would mean that the packet traveled back in time.

It makes sense, but IMHO this is handled later this part:

iputils/ping/ping_common.c

Lines 758 to 766 in 3bb2d73

if (triptime < 0) {

error(0, 0, _("Warning: time of day goes back (%ldus), taking countermeasures"), triptime);

triptime = 0;

if (!rts->opt_latency) {

gettimeofday(tv, NULL);

rts->opt_latency = 1;

goto restamp;

}

}

I also wondered if I should move it to handle it via tv->tv_sec < 0 as we now sanitize tv->tv_usec (I guess we should keep time of day goes back warning message for it). But how about if (!rts->opt_latency) { ... } part? Is it relevant for crafted RTT values as well?

At the start of the gather_statistics() we do tv_sub() where we calculate the difference between the time we send the packet and the time we received a reply. We have no way knowing if we got negative value because of wall clock change or because of a crafted value.

And in the case of the crafted value the problem is even worse, I guess that if we send a timestamp that is ahead by hours ping will get stuck in the loop, trying to restamp it for hours consuming 100% of CPU time. It would make more sense to discard such sample from the statistics.

So I would do:

Remove the restamp goto

Check for negative value right in the tv_sec and set triptime to 0 if it was negative

Skip the part where we add to the rts->tsum if triptime == 0

Ah I was blind, we actually use rts->opt_latency to guard against infinite loop. However the restamping is still questionable, we are not getting a good sample by pretending it arrived a tiny bit later.

Also idea for a future, we should switch to CLOCK_MONOTONIC timer that is not going to go backwards unlike the wall clock.

Using CLOCK_MONOTONIC for send times seems practical initially, potentially obtaining a timespec but converting to timeval for the ICMP payload. This mainly improves the reliability of RTTs at the current microsecond precision by avoiding wall-clock issues.

For receive times, kernel monotonic timespec timestamps (via SO_TIMESTAMPNS/SO_TIMESTAMPING) would be ideal, with user-space clock_gettime(CLOCK_MONOTONIC) post-recvmsg as a fallback.

This isolates RTT from wall-clock changes, yielding more reliable results.

You left in the || tv->tv_sec < -TV_SEC_MAX_VAL that shouldn't be needed because we without that we would end up in the if (tv->tv_sec < 0) branch. Otherwise it looks good.

That was deliberate: e.g. first check for long int overflow (outside of range: <-9223362813491, 9223362813491>), then check for smaller negative value in range <-9223362813490, 0) which could be also time back. I think that <-9223362813490, 0) is very long interval, but do you really consider not checking for invalid negative range useful? Because the crafting script sets: tv->tv_sec: -2601281302560770, tv->tv_usec: -6510615555425262901. Therefore the output has:

./ping/ping: Warning: overflow tv_usec -6510615555425218641 us ./ping/ping: Warning: invalid tv_sec -2821724793995266 s

(Maybe tv->tv_usec could have error message just invalid tv_usec - not specify overflow/underflow. Or, if kept, then tv->tv_sec should also specify overflow/underflow).

If we are not doing to do the multiplication in the case that tv_sec < 0 then there is no point in checking for underflow and no point in treating some negative numbers differently. At least that is my reasoning why there is no need to treat a subset of negative numbers differently.

OK, makes sense, therefore modified. Ready to be reviewed, hopefully final version.

I just kept (tv->tv_sec < 0) in else if to make sure triptime = tv->tv_sec * 1000000 + tv->tv_usec; assignment in final else is really done only if tv->tv_sec is valid.

I also unified tv->tv_usec error messages.

if (tv->tv_usec >= 1000000) { error(0, 0, _("Warning: invalid tv_usec %ld us"), tv->tv_usec); tv->tv_usec = 999999; } if (tv->tv_usec < 0) { error(0, 0, _("Warning: invalid tv_usec %ld us"), tv->tv_usec); tv->tv_usec = 0; } if (tv->tv_sec > TV_SEC_MAX_VAL) { error(0, 0, _("Warning: invalid tv_sec %ld s"), tv->tv_sec); triptime = 0; } else if (tv->tv_sec < 0) { error(0, 0, _("Warning: time of day goes back (%ldus), taking countermeasures"), tv->tv_sec); triptime = 0; if (!rts->opt_latency) { gettimeofday(tv, NULL); rts->opt_latency = 1; goto restamp; } } else { triptime = tv->tv_sec * 1000000 + tv->tv_usec; }

Also idea for a future, we should switch to CLOCK_MONOTONIC timer that is not going to go backwards unlike the wall clock.

Created an issue for it: #587.

pevik · 2025-05-06T11:27:14Z

Branch rebased.

nmeyerhans · 2025-05-06T16:03:05Z

Please, when you're finish with your review, add your Reviewed-by: or Acked-by: tag.

Acked in c03bb27 in my fork

Zephkek

There's an integer overflow vulnerability when large values are provided for both packet size (-s) and preload (-l) parameters. When multiplying alloc * rts->preload, the result can exceed INT_MAX without any bounds checking.

rcvbuf = hold = alloc * rts->preload;

This results in incorrect socket buffer allocation and ~99.99% packet loss. Looking at setup() this doesn't have overflow protection like the interval/preload.

Recommend adding bounds checking before multiplication:

if (alloc > INT_MAX / rts->preload) {
    error(0, 0, _("buffer size overflow, reduce packet size or preload"));
    hold = INT_MAX; // Or another reasonable maximum
} else {
    hold = alloc * rts->preload;
}

Reproduction:
Using large values for both packet size and preload:

$ sudo ./ping/ping -l 65536 -s 30000 localhost
../ping/ping_common.c:451:24: runtime error: signed integer overflow: 45348 * 65536 cannot be represented in type 'int'

Zephkek · 2025-05-07T02:13:05Z

Looking further at ping_common I discovered there's a similar overflow issue with the -W option. Interestingly, this one only happens when you set -c 1 (sending just one packet). If you set -W to values > 2147, it causes another integer overflow when converting the time:

$ sudo ./ping/ping -c 1 -W 2148 8.8.8.8
../ping/ping_common.c:269:37: runtime error: signed integer overflow: 2148000 * 1000 cannot be represented in type 'int'

looks like the bug is in __schedule_exit() where it does waittime = global_rts->lingertime * 1000. This function is called during ping's exit sequence, but only when -c 1 is used because it immediately enters the exit path after the first packet. With -c 2 or more, ping never hits this code path in the same way from what it looks like. The -W value gets multiplied by 1000 once during parsing to convert to ms, then multiplied by 1000 again here to convert to μs. Could be fixed by using a long long or just adding another bounds check perhaps?

pevik · 2025-05-07T07:14:27Z

Please, when you're finish with your review, add your Reviewed-by: or Acked-by: tag.

Acked in c03bb27 in my fork

@nmeyerhans Thank you! Can you please quickly check final changes? Diff against the version you acked below. May I add your Reviewed-by: now?
Also feel free to propose better error messages for new strings (NOTE: later after the release I would change %ldus => %ld us, that's why it's now the difference).

+++ ping/ping_common.c
@@ -756,30 +756,28 @@ restamp:
 		tvsub(tv, &tmp_tv);
 
 		if (tv->tv_usec >= 1000000) {
-			error(0, 0, _("Warning: underflow tv_usec %ld us"), tv->tv_usec);
+			error(0, 0, _("Warning: invalid tv_usec %ld us"), tv->tv_usec);
 			tv->tv_usec = 999999;
 		}
 
 		if (tv->tv_usec < 0) {
-			error(0, 0, _("Warning: overflow tv_usec %ld us"), tv->tv_usec);
+			error(0, 0, _("Warning: invalid tv_usec %ld us"), tv->tv_usec);
 			tv->tv_usec = 0;
 		}
 
-		if (tv->tv_sec > TV_SEC_MAX_VAL || tv->tv_sec < -TV_SEC_MAX_VAL) {
+		if (tv->tv_sec > TV_SEC_MAX_VAL) {
 			error(0, 0, _("Warning: invalid tv_sec %ld s"), tv->tv_sec);
 			triptime = 0;
-		} else {
-			triptime = tv->tv_sec * 1000000 + tv->tv_usec;
-		}
-
-		if (triptime < 0) {
-			error(0, 0, _("Warning: time of day goes back (%ldus), taking countermeasures"), triptime);
+		} else if (tv->tv_sec < 0) {
+			error(0, 0, _("Warning: time of day goes back (%ldus), taking countermeasures"), tv->tv_sec);
 			triptime = 0;
 			if (!rts->opt_latency) {
 				gettimeofday(tv, NULL);
 				rts->opt_latency = 1;
 				goto restamp;
 			}
+		} else {
+			triptime = tv->tv_sec * 1000000 + tv->tv_usec;
 		}
 
 		if (!csfailed) {

Zephkek

Other than the formatting for tv_sec this part looks good to me.

Reviewed-by: Mohamed Maatallah hotelsmaatallahrecemail@gmail.com

pevik · 2025-05-07T07:44:56Z

Looking further at ping_common I discovered there's a similar overflow issue with the -W option. Interestingly, this one only happens when you set -c 1 (sending just one packet). If you set -W to values > 2147, it causes another integer overflow when converting the time:
$ sudo ./ping/ping -c 1 -W 2148 8.8.8.8
../ping/ping_common.c:269:37: runtime error: signed integer overflow: 2148000 * 1000 cannot be represented in type 'int'
looks like the bug is in __schedule_exit() where it does waittime = global_rts->lingertime * 1000. This function is called during ping's exit sequence, but only when -c 1 is used because it immediately enters the exit path after the first packet. With -c 2 or more, ping never hits this code path in the same way from what it looks like. The -W value gets multiplied by 1000 once during parsing to convert to ms, then multiplied by 1000 again here to convert to μs. Could be fixed by using a long long or just adding another bounds check perhaps?

Good catch, this will need to be handled as well (I might do it in a separate PR).

pevik · 2025-05-07T07:51:41Z

$ sudo ./ping/ping -l 65536 -s 30000 localhost
../ping/ping_common.c:451:24: runtime error: signed integer overflow: 45348 * 65536 cannot be represented in type 'int'

And this one as well. Thanks for your reports.

pevik · 2025-05-07T08:06:56Z

Other than the formatting for tv_sec this part looks good to me.

I miss this, feel free to point what you mean.

Reviewed-by: Mohamed Maatalllah hotelsmaatallahrecemail@gmail.com

Thanks, added (Maatalllah => Maatallah)

Zephkek · 2025-05-07T08:08:31Z

Yep good catch 😅, updated the comment.

Zephkek · 2025-05-07T08:23:34Z

As for the formatting im talking about the last error message where tv_sec is logged with format %ldus
since its seconds thought should be %lds

pevik · 2025-05-07T08:51:20Z

As for the formatting im talking about the last error message where tv_sec is logged with format %ldus
since its seconds thought should be %lds

OK, updated. I'm surprised, that previous -7256972569576721232us got changed into -4243165695442945 s: i.e. only 1000x smaller (I would expect it 1000000x).

metan-ucw · 2025-05-07T09:09:30Z

+
 		if (!csfailed) {
 			rts->tsum += triptime;
 			rts->tsum2 += (double)((long long)triptime * (long long)triptime);


So we still update the statistics even if triptime == 0 I guess that this is okay for a quick fix but should be fixed later.

Is it really wrong? 2 packets transmitted, 2 received, +2 duplicates, 0% packet loss, time 101ms looks correct to me (+2 duplicates).

$ ping 127.0.0.1 -i0.1 -c2 # run with python script doing crafting on master PING 127.0.0.1 (127.0.0.1) 56(84) bytes of data. 64 bytes from 127.0.0.1: icmp_seq=1 ttl=64 time=0.065 ms ../ping/ping_common.c:757:25: runtime error: signed integer overflow: -1157944657838082 * 1000000 cannot be represented in type 'long int' ./ping/ping: Warning: time of day goes back (-2310396749805860133us), taking countermeasures ./ping/ping: Warning: time of day goes back (-2310396749805859590us), taking countermeasures 24 bytes from 127.0.0.1: icmp_seq=1 ttl=64 (truncated) ./ping/ping: Warning: time of day goes back (-4708252664467859731us), taking countermeasures 24 bytes from 127.0.0.1: icmp_seq=1 ttl=64 (truncated) 64 bytes from 127.0.0.1: icmp_seq=2 ttl=64 time=0.119 ms --- 127.0.0.1 ping statistics --- 2 packets transmitted, 2 received, +2 duplicates, 0% packet loss, time 101ms $ ping 127.0.0.1 -i0.1 -c2 # run with python script doing crafting on this PR 64 bytes from 127.0.0.1: icmp_seq=1 ttl=64 time=0.064 ms ./ping/ping: Warning: invalid tv_usec -6510615555425847804 us ./ping/ping: Warning: time of day goes back (-53614076755970 s), taking countermeasures ./ping/ping: Warning: invalid tv_usec -6510615555425847641 us ./ping/ping: Warning: time of day goes back (-53614076755970 s), taking countermeasures 24 bytes from 127.0.0.1: icmp_seq=1 ttl=64 (truncated) ./ping/ping: Warning: invalid tv_usec -6510615555425803771 us ./ping/ping: Warning: time of day goes back (-347681897578498 s), taking countermeasures 24 bytes from 127.0.0.1: icmp_seq=1 ttl=64 (truncated) 64 bytes from 127.0.0.1: icmp_seq=2 ttl=64 time=0.161 ms --- 127.0.0.1 ping statistics --- 2 packets transmitted, 2 received, +2 duplicates, 0% packet loss, time 101ms rtt min/avg/max/mdev = 0.000/0.056/0.161/0.065 ms

If it's really a bug, it would not be introduced by this. Trying to just return 1 on wrong tv_sec leads to obviously wrong:

64 bytes from 127.0.0.1: icmp_seq=1 ttl=64 time=0.061 ms ./ping/ping: Warning: invalid tv_usec -6510615555425219887 us ./ping/ping: Warning: time of day goes back (-2799219165364226 s), taking countermeasures --- 127.0.0.1 ping statistics --- 1 packets transmitted, 2 received, -100% packet loss, time 0ms rtt min/avg/max/mdev = 0.061/0.030/0.061/0.030 ms

Or you mean skip on triptime == 0? (I don't think it's correct)

- if (!csfailed) { + if (!csfailed && !triptime) {

metan-ucw · 2025-05-07T09:12:34Z

The commit description talks about limiting the tv_sec to <-LONG_MAX/1000000, LONG_MAX/1000000> but now we limit it to <0, LONG_MAX/1000000> , other than that the patch looks good.

Reviewed-by: Cyril Hrubis chrubis@suse.cz

Crafted ICMP Echo Reply packet can cause signed integer overflow in 1) triptime calculation: triptime = tv->tv_sec * 1000000 + tv->tv_usec; 2) tsum2 increment which uses triptime rts->tsum2 += (double)((long long)triptime * (long long)triptime); 3) final tmvar: tmvar = (rts->tsum2 / total) - (tmavg * tmavg) $ export CFLAGS="-O1 -g -fsanitize=address,undefined -fno-omit-frame-pointer" $ export LDFLAGS="-fsanitize=address,undefined -fno-omit-frame-pointer" $ meson setup .. -Db_sanitize=address,undefined $ ninja $ ./ping/ping -c2 127.0.0.1 PING 127.0.0.1 (127.0.0.1) 56(84) bytes of data. 64 bytes from 127.0.0.1: icmp_seq=1 ttl=64 time=0.061 ms ../ping/ping_common.c:757:25: runtime error: signed integer overflow: -2513732689199106 * 1000000 cannot be represented in type 'long int' ../ping/ping_common.c:757:12: runtime error: signed integer overflow: -4975495174606980224 + -6510615555425289427 cannot be represented in type 'long int' ../ping/ping_common.c:769:47: runtime error: signed integer overflow: 6960633343677281965 * 6960633343677281965 cannot be represented in type 'long int' 24 bytes from 127.0.0.1: icmp_seq=1 ttl=64 (truncated) ./ping/ping: Warning: time of day goes back (-7256972569576721377us), taking countermeasures ./ping/ping: Warning: time of day goes back (-7256972569576721232us), taking countermeasures 24 bytes from 127.0.0.1: icmp_seq=1 ttl=64 (truncated) ../ping/ping_common.c:265:16: runtime error: signed integer overflow: 6960633343677281965 * 2 cannot be represented in type 'long int' 64 bytes from 127.0.0.1: icmp_seq=2 ttl=64 time=0.565 ms --- 127.0.0.1 ping statistics --- 2 packets transmitted, 2 received, +2 duplicates, 0% packet loss, time 1002ms ../ping/ping_common.c:940:42: runtime error: signed integer overflow: 1740158335919320832 * 1740158335919320832 cannot be represented in type 'long int' rtt min/avg/max/mdev = 0.000/1740158335919320.832/6960633343677281.965/-1623514645242292.-224 ms To fix the overflow check allowed ranges of struct timeval members: * tv_sec <0, LONG_MAX/1000000> * tv_usec <0, 999999> Fix includes 2 new error messages (needs translation). Also existing message "time of day goes back ..." needed to be modified as it now prints tv->tv_sec which is a second (needs translation update). After fix: $ ./ping/ping -c2 127.0.0.1 64 bytes from 127.0.0.1: icmp_seq=1 ttl=64 time=0.057 ms ./ping/ping: Warning: invalid tv_usec -6510615555424928611 us ./ping/ping: Warning: time of day goes back (-3985394643238914 s), taking countermeasures ./ping/ping: Warning: invalid tv_usec -6510615555424928461 us ./ping/ping: Warning: time of day goes back (-3985394643238914 s), taking countermeasures 24 bytes from 127.0.0.1: icmp_seq=1 ttl=64 (truncated) ./ping/ping: Warning: invalid tv_usec -6510615555425884541 us ./ping/ping: Warning: time of day goes back (-4243165695442945 s), taking countermeasures 24 bytes from 127.0.0.1: icmp_seq=1 ttl=64 (truncated) 64 bytes from 127.0.0.1: icmp_seq=2 ttl=64 time=0.111 ms --- 127.0.0.1 ping statistics --- 2 packets transmitted, 2 received, +2 duplicates, 0% packet loss, time 101ms rtt min/avg/max/mdev = 0.000/0.042/0.111/0.046 ms Fixes: iputils#584 Fixes: CVE-2025-472 Link: https://github.com/Zephkek/ping-rtt-overflow/ Co-developed-by: Cyril Hrubis <chrubis@suse.cz> Reported-by: Mohamed Maatallah <hotelsmaatallahrecemail@gmail.com> Reviewed-by: Mohamed Maatallah <hotelsmaatallahrecemail@gmail.com> Reviewed-by: Cyril Hrubis <chrubis@suse.cz> Signed-off-by: Petr Vorel <pvorel@suse.cz>

nmeyerhans · 2025-05-07T16:49:34Z

updated signoff in b146202

Zephkek

Found a tricky integer overflow in ping's RTT calculation. When receiving crafted ICMP replies with large timestamps (~268s), the RTT accumulates until it exceeds INT_MAX and wraps to negative. This becomes a problem only in adaptive mode because update_interval() uses this negative RTT value to calculate interval, resulting in negative intervals which causes the main loop to run without delay.

The issue is specifically here

rts->rtt += triptime - rts->rtt / 8;

to trigger this overflow, i made this script to craft the rtt values:
https://gist.github.com/Zephkek/e8697cdbcc190dde673bcba0b18f5710

then ran ping in adaptive mode:

sudo ./ping/ping -A 127.0.0.1

I also added a few debug prints because ASan & UBSan issues..:

DEBUG AFTER RTT UPDATE: new rtt=2147299198, addition=268864057, division=268412399
64 bytes from 127.0.0.1: icmp_seq=56 ttl=64 time=268864 ms (DUP!)
DEBUG BEFORE RTT UPDATE: triptime=268933983, current rtt=2147299198
DEBUG AFTER RTT UPDATE: new rtt=-2147146514, addition=268933983, division=-268393314
64 bytes from 127.0.0.1: icmp_seq=57 ttl=64 time=268934 ms (DUP!)
DEBUG BEFORE RTT UPDATE: triptime=268994142, current rtt=-2147146514
DEBUG AFTER RTT UPDATE: new rtt=-1609759058, addition=268994142, division=-201219882
64 bytes from 127.0.0.1: icmp_seq=58 ttl=64 time=268994 ms (DUP!)
DEBUG BEFORE RTT UPDATE: triptime=268064302, current rtt=-1609759058
DEBUG AFTER RTT UPDATE: new rtt=-1140474874, addition=268064302, division=-142559359
64 bytes from 127.0.0.1: icmp_seq=59 ttl=64 time=268064 ms (DUP!)
DEBUG BEFORE RTT UPDATE: triptime=268124126, current rtt=-1140474874
DEBUG AFTER RTT UPDATE: new rtt=-729791389, addition=268124126, division=-91223923
64 bytes from 127.0.0.1: icmp_seq=60 ttl=64 time=268124 ms (DUP!)
DEBUG BEFORE RTT UPDATE: triptime=268194229, current rtt=-729791389
DEBUG AFTER RTT UPDATE: new rtt=-370373237, addition=268194229, division=-46296654
64 bytes from 127.0.0.1: icmp_seq=61 ttl=64 time=268194 ms (DUP!)

This in turn will cause interval to be negative, thus next to be negative thus stuck in this loop:

		 do {
			next = pinger(rts, fset, sock);
			next = schedule_exit(rts, next);
		} while (next <= 0);

Weirdly, neither ASAn nor UBSan catch this...

2025-05-07.18-41-26.mp4

pevik · 2025-05-07T18:02:45Z

Merged this as 070cfac, thanks all to review.
I'll try to fix the other 3 issues next week.

@Zephkek It'd be better to add reports into separate issues or at #584 (which should be reopened then), but it's ok to keep it here (fix is more important than the formality).

$ export CFLAGS="-O1 -g -fsanitize=address,undefined -fno-omit-frame-pointer" $ meson setup .. $ ninja && sudo ./ping/ping -c1 -l 65536 -s 30000 ::1 ../ping/ping_common.c:451:24: runtime error: signed integer overflow: 65536 * 46528 cannot be represented in type 'int' PING ::1 (::1) 30000 data bytes 30008 bytes from ::1: icmp_seq=1 ttl=64 time=0.052 ms After fix: $ sudo ./ping/ping -c1 -l 65536 -s 30000 127.0.0.1 ./ping/ping: WARNING: buffer size overflow, reduce packet size or preload ./ping/ping: WARNING: probably, rcvbuf is not enough to hold preload PING 127.0.0.1 (127.0.0.1) 30000(30028) bytes of data. 30008 bytes from 127.0.0.1: icmp_seq=1 ttl=64 time=0.053 ms Link: iputils#585 (review) Reported-by: Mohamed Maatallah <hotelsmaatallahrecemail@gmail.com> Suggested-by: Mohamed Maatallah <hotelsmaatallahrecemail@gmail.com> Signed-off-by: Petr Vorel <pvorel@suse.cz>

Maximum of preload value (-l) is 65536, but due multiplication with packat size (-s) there can be integer overflow: $ export CFLAGS="-O1 -g -fsanitize=address,undefined -fno-omit-frame-pointer" $ meson setup .. $ ninja && sudo ./ping/ping -c1 -l 65536 -s 30000 ::1 ../ping/ping_common.c:451:24: runtime error: signed integer overflow: 65536 * 46528 cannot be represented in type 'int' PING ::1 (::1) 30000 data bytes 30008 bytes from ::1: icmp_seq=1 ttl=64 time=0.052 ms Because setsockopt() requires int, instead of making hold and rcvbuf variables bigger (long int) limit them to INT_MAX. This will often lead to warning about rcvbuf is not enough to hold preload, because on current kernel 6.14 and ICMP datagram socket is the max. size 425984, but probably better not to depend on this value. After fix: $ sudo ./ping/ping -c1 -l 65536 -s 30000 127.0.0.1 ./ping/ping: WARNING: buffer size overflow, reduce packet size or preload ./ping/ping: WARNING: probably, rcvbuf is not enough to hold preload PING 127.0.0.1 (127.0.0.1) 30000(30028) bytes of data. 30008 bytes from 127.0.0.1: icmp_seq=1 ttl=64 time=0.053 ms Link: iputils#585 (review) Reported-by: Mohamed Maatallah <hotelsmaatallahrecemail@gmail.com> Suggested-by: Mohamed Maatallah <hotelsmaatallahrecemail@gmail.com> Signed-off-by: Petr Vorel <pvorel@suse.cz>

Maximum of preload value (-l) is 65536, but due multiplication with packat size (-s) there can be integer overflow: $ export CFLAGS="-O1 -g -fsanitize=address,undefined -fno-omit-frame-pointer" $ meson setup .. $ ninja && sudo ./ping/ping -c1 -l 65536 -s 30000 ::1 ../ping/ping_common.c:451:24: runtime error: signed integer overflow: 65536 * 46528 cannot be represented in type 'int' PING ::1 (::1) 30000 data bytes 30008 bytes from ::1: icmp_seq=1 ttl=64 time=0.052 ms Because setsockopt() requires int, instead of making hold and rcvbuf variables bigger (long int) limit them to INT_MAX. This will often lead to warning about rcvbuf is not enough to hold preload, because on current kernel 6.14 and ICMP datagram socket is the max. socket buffer size 425984, but probably better not to depend on this value. After fix: $ sudo ./ping/ping -c1 -l 65536 -s 30000 127.0.0.1 ./ping/ping: WARNING: buffer size overflow, reduce packet size or preload ./ping/ping: WARNING: probably, rcvbuf is not enough to hold preload PING 127.0.0.1 (127.0.0.1) 30000(30028) bytes of data. 30008 bytes from 127.0.0.1: icmp_seq=1 ttl=64 time=0.053 ms Link: iputils#585 (review) Reported-by: Mohamed Maatallah <hotelsmaatallahrecemail@gmail.com> Suggested-by: Mohamed Maatallah <hotelsmaatallahrecemail@gmail.com> Signed-off-by: Petr Vorel <pvorel@suse.cz>

Maximum of preload value (-l) is 65536, but due multiplication with packat size (-s) there can be integer overflow: $ export CFLAGS="-O1 -g -fsanitize=address,undefined -fno-omit-frame-pointer" $ meson setup .. $ ninja && sudo ./ping/ping -c1 -l 65536 -s 30000 ::1 ../ping/ping_common.c:451:24: runtime error: signed integer overflow: 65536 * 46528 cannot be represented in type 'int' PING ::1 (::1) 30000 data bytes 30008 bytes from ::1: icmp_seq=1 ttl=64 time=0.052 ms Because setsockopt() requires int, instead of making hold and rcvbuf variables bigger (long int) limit them to INT_MAX. This will often lead to warning about rcvbuf is not enough to hold preload, because on current kernel 6.14 and ICMP datagram socket is the max. socket buffer size 425984, but probably better not to depend on this value. After fix: $ sudo ./ping/ping -c1 -l 65536 -s 30000 127.0.0.1 ./ping/ping: WARNING: buffer size overflow, reduce packet size or preload ./ping/ping: WARNING: probably, rcvbuf is not enough to hold preload PING 127.0.0.1 (127.0.0.1) 30000(30028) bytes of data. 30008 bytes from 127.0.0.1: icmp_seq=1 ttl=64 time=0.053 ms Link: iputils#585 (review) Closes: iputils#586 Reported-by: Mohamed Maatallah <hotelsmaatallahrecemail@gmail.com> Suggested-by: Mohamed Maatallah <hotelsmaatallahrecemail@gmail.com> Signed-off-by: Petr Vorel <pvorel@suse.cz>

yvs2014

changes around restamp

restamp:
                tvsub(tv, &tmp_tv);
-                 triptime = tv->tv_sec * 1000000 + tv->tv_usec;
-                 if (triptime < 0) {
-                         error(0, 0, _("Warning: time of day goes back (%ldus), taking countermeasures"), triptime);
-                         triptime = 0;
-                         if (!rts->opt_latency) {
-                                 gettimeofday(tv, NULL);
-                                 rts->opt_latency = 1;
-                                 goto restamp;
-                         }
-                 }
 
+                 if (tv->tv_usec >= 1000000) {
+                         error(0, 0, _("Warning: invalid tv_usec %ld us"), tv->tv_usec);
+                         tv->tv_usec = 999999;
+                 }
+ 
+                 if (tv->tv_usec < 0) {
+                         error(0, 0, _("Warning: invalid tv_usec %ld us"), tv->tv_usec);
+                         tv->tv_usec = 0;
+                 }
+ 
+                 if (tv->tv_sec > TV_SEC_MAX_VAL) {
+                         error(0, 0, _("Warning: invalid tv_sec %ld s"), tv->tv_sec);
+                         triptime = 0;
+                 } else if (tv->tv_sec < 0) {
+                         error(0, 0, _("Warning: time of day goes back (%ld s), taking countermeasures"), tv->tv_sec);
+                         triptime = 0;
+                         if (!rts->opt_latency) {
+                                 gettimeofday(tv, NULL);
+                                 rts->opt_latency = 1;
+                                 goto restamp;
+                         }
+                 } else {
+                         triptime = tv->tv_sec * 1000000 + tv->tv_usec;
+                 }

If I got it right: the purpose of (two-pass max) restamp section is to set triptime value >= 0 mandatory, plus to set latency option (to not call gettimeofday() further) if something is incorrect with triptime at first pass.

In both cases (prev and curr versions) we have `triptime` value >= 0, but it's a bit different with some timevals:
let's say tv(.sec=12345 .usec=0) and tmp_tv(.sec=12346, .usec=-1500000), then tvsub() returns tv(.sec=0, .usec=-500000), and so that there's triptime=0 and no "time of day goes back" message and latency option is not set with this example in current version. Or I got it incorrectly?

Maximum of preload value (-l) is 65536, but due multiplication with packat size (-s) there can be integer overflow: $ export CFLAGS="-O1 -g -fsanitize=address,undefined -fno-omit-frame-pointer" $ meson setup .. $ ninja && sudo ./ping/ping -c1 -l 65536 -s 30000 ::1 ../ping/ping_common.c:451:24: runtime error: signed integer overflow: 65536 * 46528 cannot be represented in type 'int' PING ::1 (::1) 30000 data bytes 30008 bytes from ::1: icmp_seq=1 ttl=64 time=0.052 ms Because setsockopt() requires int, instead of making hold and rcvbuf variables bigger (long int) limit them to INT_MAX. This will often lead to warning about rcvbuf is not enough to hold preload, because on current kernel 6.14 and ICMP datagram socket is the max. socket buffer size 425984, but probably better not to depend on this value. After fix: $ sudo ./ping/ping -c1 -l 65536 -s 30000 127.0.0.1 ./ping/ping: WARNING: buffer size overflow, reduce packet size or preload ./ping/ping: WARNING: probably, rcvbuf is not enough to hold preload PING 127.0.0.1 (127.0.0.1) 30000(30028) bytes of data. 30008 bytes from 127.0.0.1: icmp_seq=1 ttl=64 time=0.053 ms Link: iputils#585 (review) Closes: iputils#586 Reported-by: Mohamed Maatallah <hotelsmaatallahrecemail@gmail.com> Suggested-by: Mohamed Maatallah <hotelsmaatallahrecemail@gmail.com> Reviewed-by: Cyril Hrubis <chrubis@suse.cz> Signed-off-by: Petr Vorel <pvorel@suse.cz>

Maximum of preload value (-l) is 65536, but due multiplication with packat size (-s) there can be integer overflow: $ export CFLAGS="-O1 -g -fsanitize=address,undefined -fno-omit-frame-pointer" $ meson setup .. $ ninja && sudo ./ping/ping -c1 -l 65536 -s 30000 ::1 ../ping/ping_common.c:451:24: runtime error: signed integer overflow: 65536 * 46528 cannot be represented in type 'int' PING ::1 (::1) 30000 data bytes 30008 bytes from ::1: icmp_seq=1 ttl=64 time=0.052 ms Because setsockopt() requires int, instead of making hold and rcvbuf variables bigger (long int) limit them to INT_MAX. This will often lead to warning about rcvbuf is not enough to hold preload, because on current kernel 6.14 and ICMP datagram socket is the max. socket buffer size 425984, but probably better not to depend on this value. After fix: $ sudo ./ping/ping -c1 -l 65536 -s 30000 127.0.0.1 ./ping/ping: WARNING: buffer size overflow, reduce packet size or preload ./ping/ping: WARNING: probably, rcvbuf is not enough to hold preload PING 127.0.0.1 (127.0.0.1) 30000(30028) bytes of data. 30008 bytes from 127.0.0.1: icmp_seq=1 ttl=64 time=0.053 ms Link: iputils#585 (review) Closes: iputils#586 Reported-by: Mohamed Maatallah <hotelsmaatallahrecemail@gmail.com> Suggested-by: Mohamed Maatallah <hotelsmaatallahrecemail@gmail.com> Reviewed-by: Cyril Hrubis <chrubis@suse.cz> [ pvorel: backport of upstream f30f0e5 to 20211215 ] Signed-off-by: Petr Vorel <pvorel@suse.cz>

Crafted ICMP Echo Reply packet can cause signed integer overflow in 1) triptime calculation: triptime = tv->tv_sec * 1000000 + tv->tv_usec; 2) tsum2 increment which uses triptime rts->tsum2 += (double)((long long)triptime * (long long)triptime); 3) final tmvar: tmvar = (rts->tsum2 / total) - (tmavg * tmavg) $ export CFLAGS="-O1 -g -fsanitize=address,undefined -fno-omit-frame-pointer" $ export LDFLAGS="-fsanitize=address,undefined -fno-omit-frame-pointer" $ meson setup .. -Db_sanitize=address,undefined $ ninja $ ./ping/ping -c2 127.0.0.1 PING 127.0.0.1 (127.0.0.1) 56(84) bytes of data. 64 bytes from 127.0.0.1: icmp_seq=1 ttl=64 time=0.061 ms ../ping/ping_common.c:757:25: runtime error: signed integer overflow: -2513732689199106 * 1000000 cannot be represented in type 'long int' ../ping/ping_common.c:757:12: runtime error: signed integer overflow: -4975495174606980224 + -6510615555425289427 cannot be represented in type 'long int' ../ping/ping_common.c:769:47: runtime error: signed integer overflow: 6960633343677281965 * 6960633343677281965 cannot be represented in type 'long int' 24 bytes from 127.0.0.1: icmp_seq=1 ttl=64 (truncated) ./ping/ping: Warning: time of day goes back (-7256972569576721377us), taking countermeasures ./ping/ping: Warning: time of day goes back (-7256972569576721232us), taking countermeasures 24 bytes from 127.0.0.1: icmp_seq=1 ttl=64 (truncated) ../ping/ping_common.c:265:16: runtime error: signed integer overflow: 6960633343677281965 * 2 cannot be represented in type 'long int' 64 bytes from 127.0.0.1: icmp_seq=2 ttl=64 time=0.565 ms --- 127.0.0.1 ping statistics --- 2 packets transmitted, 2 received, +2 duplicates, 0% packet loss, time 1002ms ../ping/ping_common.c:940:42: runtime error: signed integer overflow: 1740158335919320832 * 1740158335919320832 cannot be represented in type 'long int' rtt min/avg/max/mdev = 0.000/1740158335919320.832/6960633343677281.965/-1623514645242292.-224 ms To fix the overflow check allowed ranges of struct timeval members: * tv_sec <0, LONG_MAX/1000000> * tv_usec <0, 999999> Fix includes 2 new error messages (needs translation). Also existing message "time of day goes back ..." needed to be modified as it now prints tv->tv_sec which is a second (needs translation update). After fix: $ ./ping/ping -c2 127.0.0.1 64 bytes from 127.0.0.1: icmp_seq=1 ttl=64 time=0.057 ms ./ping/ping: Warning: invalid tv_usec -6510615555424928611 us ./ping/ping: Warning: time of day goes back (-3985394643238914 s), taking countermeasures ./ping/ping: Warning: invalid tv_usec -6510615555424928461 us ./ping/ping: Warning: time of day goes back (-3985394643238914 s), taking countermeasures 24 bytes from 127.0.0.1: icmp_seq=1 ttl=64 (truncated) ./ping/ping: Warning: invalid tv_usec -6510615555425884541 us ./ping/ping: Warning: time of day goes back (-4243165695442945 s), taking countermeasures 24 bytes from 127.0.0.1: icmp_seq=1 ttl=64 (truncated) 64 bytes from 127.0.0.1: icmp_seq=2 ttl=64 time=0.111 ms --- 127.0.0.1 ping statistics --- 2 packets transmitted, 2 received, +2 duplicates, 0% packet loss, time 101ms rtt min/avg/max/mdev = 0.000/0.042/0.111/0.046 ms Fixes: #584 Fixes: CVE-2025-472 Link: https://github.com/Zephkek/ping-rtt-overflow/ Co-developed-by: Cyril Hrubis <chrubis@suse.cz> Reported-by: Mohamed Maatallah <hotelsmaatallahrecemail@gmail.com> Reviewed-by: Mohamed Maatallah <hotelsmaatallahrecemail@gmail.com> Reviewed-by: Cyril Hrubis <chrubis@suse.cz> Reviewed-by: Noah Meyerhans <noahm@debian.org> Signed-off-by: Petr Vorel <pvorel@suse.cz>

pevik mentioned this pull request May 5, 2025

Signed 64-bit integer overflow in RTT calculation #584

Closed

pevik requested a review from a team May 5, 2025 23:49

nmeyerhans reviewed May 6, 2025

View reviewed changes

pevik force-pushed the CVE-2025-47268 branch 2 times, most recently from d17b6d0 to 23db9b0 Compare May 6, 2025 01:13

pevik requested a review from a team May 6, 2025 07:41

metan-ucw reviewed May 6, 2025

View reviewed changes

pevik force-pushed the CVE-2025-47268 branch from 23db9b0 to 4c799c7 Compare May 6, 2025 11:26

Zephkek suggested changes May 7, 2025

View reviewed changes

pevik force-pushed the CVE-2025-47268 branch from 4c799c7 to 02748b9 Compare May 7, 2025 07:05

pevik requested review from Zephkek, metan-ucw and nmeyerhans May 7, 2025 07:15

Zephkek approved these changes May 7, 2025

View reviewed changes

pevik force-pushed the CVE-2025-47268 branch from 02748b9 to c7fc969 Compare May 7, 2025 08:05

pevik force-pushed the CVE-2025-47268 branch from c7fc969 to 817b6a3 Compare May 7, 2025 08:47

metan-ucw reviewed May 7, 2025

View reviewed changes

metan-ucw approved these changes May 7, 2025

View reviewed changes

pevik force-pushed the CVE-2025-47268 branch from 817b6a3 to b41e4a1 Compare May 7, 2025 11:22

nmeyerhans approved these changes May 7, 2025

View reviewed changes

Zephkek suggested changes May 7, 2025

View reviewed changes

pevik closed this May 7, 2025

pevik mentioned this pull request May 9, 2025

ping: Fix integer overflow in large -s and -l values #586

Merged

pevik mentioned this pull request May 9, 2025

ping RTT: replace gettimeofday() to CLOCK_MONOTONIC, struct timespec, clock_gettime() #587

Open

yvs2014 reviewed May 10, 2025

View reviewed changes

	if (triptime < 0) {
	error(0, 0, _("Warning: time of day goes back (%ldus), taking countermeasures"), triptime);
	triptime = 0;
	if (!rts->opt_latency) {
	gettimeofday(tv, NULL);
	rts->opt_latency = 1;
	goto restamp;
	}
	}

Conversation

pevik commented May 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Zephkek May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pevik commented May 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pevik May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Zephkek May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

metan-ucw May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

metan-ucw May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pevik commented May 6, 2025

Uh oh!

nmeyerhans commented May 6, 2025

Uh oh!

Zephkek left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Zephkek commented May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pevik commented May 7, 2025

Uh oh!

Zephkek left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pevik commented May 7, 2025

Uh oh!

pevik commented May 7, 2025

Uh oh!

pevik commented May 7, 2025

Uh oh!

Zephkek commented May 7, 2025

Uh oh!

Zephkek commented May 7, 2025

Uh oh!

pevik commented May 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pevik May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

metan-ucw commented May 7, 2025

Uh oh!

nmeyerhans commented May 7, 2025

Uh oh!

Zephkek May 6, 2025 •

edited

Loading

pevik May 6, 2025 •

edited

Loading

Zephkek May 6, 2025 •

edited

Loading

metan-ucw May 6, 2025 •

edited

Loading

metan-ucw May 6, 2025 •

edited

Loading

Zephkek left a comment •

edited

Loading

Zephkek commented May 7, 2025 •

edited

Loading

Zephkek left a comment •

edited

Loading

pevik May 7, 2025 •

edited

Loading

Zephkek left a comment •

edited

Loading

yvs2014 left a comment •

edited

Loading