ping/node_info.c | 1 1 + 0 - 0 !
ping/ping.c | 4 1 + 3 - 0 !
ping/ping.h | 2 1 + 1 - 0 !
ping/ping6_common.c | 2 1 + 1 - 0 !
ping/ping_common.c | 4 2 + 2 - 0 !
5 files changed, 6 insertions(+), 7 deletions(-)

 [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 Hla <miloslav.hula@gmail.com>
Suggested-by: Cyril Hrubis <chrubis@suse.cz>