ping utility displays wrong IPv4 address upon receiving ICMP Redirect

[Christoph Viethen]

***** Observed behaviour *****
We ping a host, say 137.226.39.170. While doing so, the router redirects us to 137.226.42.42. (We verify, with Wireshark and taking a close look at RFC 792, that our redirect packet is formatted correctly on-wire.) We see the following output of the ping utility:

64 bytes from 137.226.39.170: icmp_seq=46 ttl=251 time=0.619 ms
64 bytes from 137.226.39.170: icmp_seq=47 ttl=251 time=0.676 ms
64 bytes from 137.226.39.170: icmp_seq=48 ttl=251 time=1.92 ms
From 134.130.3.3 icmp_seq=47 Redirect Host(New nexthop: 42.42.226.137)
64 bytes from 137.226.39.170: icmp_seq=49 ttl=251 time=4.79 ms

The gateway address, as RFC 792 calls it, is displayed the wrong way around. Feels like an Endianness bug right from the start.

***** Expected behaviour *****
ping utility should display the gateway address correctly. ("New nexthop: 137.226.42.42")

***** Inspection of source code; bug theory of operation *****
iputils_20190709.orig.tar.xz --> iputils-20190709/ping.c

Bug happens in the function pr_icmph(). This gets called from one of three different places (lines 899, 1075, and 1101 of ping.c), and only in one case, the fourth parameter is set to NULL (when called from line 899).

The third parameter ("info") of pr_icmph() is generally meant to be understood in host byte order. This can be seen in several places (lines 1185, 1284).

Well, but in one place, this fact is ignored and the content of this third parameter, being in host byte order, gets copied into a structure that, by definition, is supposed to contain that piece of information in network byte order, which constitutes the bug.

lines 1250 etc:
        {
                struct sockaddr_in sin = {
                        .sin_family = AF_INET,
                        .sin_addr = {
                                icp ? icp->un.gateway : info
                        }
                };

                printf(_("(New nexthop: %s)\n"), pr_addr(&sin, sizeof sin));
        }

In the first alternative of the ternary operator, icp->un.gateway is in network byte order, and the value s_addr within struct in_addr that it gets copied to is also in network byte order, so everything's beautiful. But in the second alternative of the ternary operator, i.e. if icp (parameter number four of pr_icmph) happens to be NULL, as happens when pr_icmph() gets called from line 899, unfortunately the value of "info" (in host byte order) gets copied into s_addr, and bad things happen.

***** Bugfix patch *****

--- ping.c 2019-07-09 22:55:49.000000000 +0200
+++ ping_fixed.c 2021-02-09 20:08:18.724747594 +0100
@@ -1251,7 +1251,7 @@
    struct sockaddr_in sin = {
     .sin_family = AF_INET,
     .sin_addr = {
- icp ? icp->un.gateway : info
+ icp ? icp->un.gateway : htonl(info)
     }
    };

Simply converts the contents of info (which is in h_ost byte order) to n_etwork byte order, and everything's fine.

Tried it, and this reliably fixes the bug.

ProblemType: Bug
DistroRelease: Ubuntu 20.04
Package: iputils-ping 3:20190709-3
ProcVersionSignature: Ubuntu 5.4.0-65.73-generic 5.4.78
Uname: Linux 5.4.0-65-generic x86_64
ApportVersion: 2.20.11-0ubuntu27.16
Architecture: amd64
CasperMD5CheckResult: skip
Date: Tue Feb 9 19:33:57 2021
InstallationDate: Installed on 2015-12-09 (1889 days ago)
InstallationMedia: Ubuntu-Server 14.04.3 LTS "Trusty Tahr" - Beta amd64 (20150805)
SourcePackage: iputils
UpgradeStatus: Upgraded to focal on 2020-12-28 (43 days ago)

[Launchpad Janitor (janitor)]

Status changed to 'Confirmed' because the bug affects multiple users.