From d466aabcadcc2d7fd1f132ea3f580ad102773cf9 Mon Sep 17 00:00:00 2001 From: Petr Vorel Date: Wed, 6 Dec 2023 15:42:16 +0100 Subject: [PATCH] Revert "ping: use random value for the identifier field" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit 5026c2221a15bf13e601eade015c971bf07a27e9. Unlike TCP and UDP, which use port to uniquely identify the socket to deliver data, ICMP use identifier field (ID) to identify the socket. Therefore if on the same machine, at the same time, two ping processes use the same ID, echo reply can be delivered to the wrong socket. This is known problem due 16 bit ID field (65535). We used to use PID to get unique number. The default value of /proc/sys/kernel/pid_max is 32768 (half). The problem is not new, but it was hidden until 5f6bec5 ("ping: Print reply with wrong source with warning"). 5026c22 changed it to use our random implementation to increase security. But that actually increases the collisions on systems that use ping heavily: e.g. ping run with Nagios via Debian specific check-host-alive Nagios plugin: $ ping -n -v -D -W 1 -i 1 -c 5 -M 'do' -s 56 -O "$Host") (75-100 ping instances in the reported issue.) Because we consider warning from 5f6bec5 useful and not consider leaking PID information as a real security issue, we revert 5026c22. getpid() is used in other ping implementations: * fping https://github.com/schweikert/fping/blob/develop/src/fping.c#L496 * busybox https://git.busybox.net/busybox/tree/networking/ping.c#n376 * FreeBSD https://cgit.freebsd.org/src/tree/sbin/ping/ping.c#n632 * inetutils https://git.savannah.gnu.org/cgit/inetutils.git/tree/ping/ping.c#n286 * Apple https://opensource.apple.com/source/network_cmds/network_cmds-433/ping.tproj/ping.c.auto.html In case leaking PID *is* a real problem, we could solve this with comparing the ICMP optional data. We could add 128 bit random value to check. But we already use struct timeval if packet size is big enough for it (>= 16 bits), therefore we could use it for comparing for most of the packet sizes (the default is 56 bits). Fixes: https://github.com/iputils/iputils/issues/489 Closes: https://github.com/iputils/iputils/pull/503 Reported-by: Miloslav Hůla Suggested-by: Cyril Hrubis Acked-by: Johannes Segitz jsegitz@suse.de Acked-by: Cyril Hrubis Signed-off-by: Petr Vorel Conflict:NA Reference:https://github.com/iputils/iputils/commit/d466aabcadcc2d7fd1f132ea3f580ad102773cf9 --- ping/node_info.c | 1 + ping/ping.c | 4 +--- ping/ping.h | 2 +- ping/ping6_common.c | 2 +- ping/ping_common.c | 4 ++-- 5 files changed, 6 insertions(+), 7 deletions(-) diff --git a/ping/node_info.c b/ping/node_info.c index 10a76818..ce392a28 100644 --- a/ping/node_info.c +++ b/ping/node_info.c @@ -91,6 +91,7 @@ int niquery_is_enabled(struct ping_ni *ni) void niquery_init_nonce(struct ping_ni *ni) { #if PING6_NONCE_MEMORY + iputils_srand(); ni->nonce_ptr = calloc(NI_NONCE_SIZE, MAX_DUP_CHK); if (!ni->nonce_ptr) error(2, errno, "calloc"); diff --git a/ping/ping.c b/ping/ping.c index f4707104..0ff5a487 100644 --- a/ping/ping.c +++ b/ping/ping.c @@ -569,8 +569,6 @@ main(int argc, char **argv) if (!argc) error(1, EDESTADDRREQ, "usage error"); - iputils_srand(); - target = argv[argc - 1]; rts.outpack = malloc(rts.datalen + 28); @@ -1527,7 +1525,7 @@ in_cksum(const unsigned short *addr, int len, unsigned short csum) /* * pinger -- * Compose and transmit an ICMP ECHO REQUEST packet. The IP packet - * will be added on by the kernel. The ID field is a random number, + * will be added on by the kernel. The ID field is our UNIX process ID, * and the sequence number is an ascending integer. The first several bytes * of the data portion are used to hold a UNIX "timeval" struct in VAX * byte-order, to compute the round-trip time. diff --git a/ping/ping.h b/ping/ping.h index 04b2ccf4..7799395f 100644 --- a/ping/ping.h +++ b/ping/ping.h @@ -159,7 +159,7 @@ struct ping_rts { size_t datalen; char *hostname; uid_t uid; - int ident; /* random id to identify our packets */ + int ident; /* process id to identify our packets */ int sndbuf; int ttl; diff --git a/ping/ping6_common.c b/ping/ping6_common.c index 7b2bf158..5e78f852 100644 --- a/ping/ping6_common.c +++ b/ping/ping6_common.c @@ -583,7 +583,7 @@ int ping6_receive_error_msg(struct ping_rts *rts, socket_st *sock) /* * pinger -- * Compose and transmit an ICMP ECHO REQUEST packet. The IP packet - * will be added on by the kernel. The ID field is a random number, + * will be added on by the kernel. The ID field is our UNIX process ID, * and the sequence number is an ascending integer. The first several bytes * of the data portion are used to hold a UNIX "timeval" struct in VAX * byte-order, to compute the round-trip time. diff --git a/ping/ping_common.c b/ping/ping_common.c index ed4fee87..6eb1aa4e 100644 --- a/ping/ping_common.c +++ b/ping/ping_common.c @@ -303,7 +303,7 @@ void print_timestamp(struct ping_rts *rts) /* * pinger -- * Compose and transmit an ICMP ECHO REQUEST packet. The IP packet - * will be added on by the kernel. The ID field is a random number, + * will be added on by the kernel. The ID field is our UNIX process ID, * and the sequence number is an ascending integer. The first several bytes * of the data portion are used to hold a UNIX "timeval" struct in VAX * byte-order, to compute the round-trip time. @@ -536,7 +536,7 @@ void setup(struct ping_rts *rts, socket_st *sock) } if (sock->socktype == SOCK_RAW && rts->ident == -1) - rts->ident = rand() & IDENTIFIER_MAX; + rts->ident = htons(getpid() & 0xFFFF); set_signal(SIGINT, sigexit); set_signal(SIGALRM, sigexit);