Reply payload and truncation checking is wrong and bypasses gather_statistics() when it should not

[paul-demo]

All past versions of ping on OSX and Ubuntu have allowed for truncated responses. For example, google.com truncates to 96B (68B ICMP payload + 8B ICMP header + 20B IPv4 header).

However, we found that moving from Ubuntu 18 to Ubuntu 20, ping has changed, and now doesn't report the info message "truncated...got X bytes instead of Y", but rather just doesn't show anything when a reply is truncated.

Why did you change this behavior?

[paul-demo]

This commit changed the behavior of ping by adding contains_pattern_in_payload() before gather_statistics() is called. This means "truncated" statistics will never happen and there is unreachable code in gather_statistics() because contains_pattern_in_payload() will always find a mismatch first and nothing will get printed.

f7710a1

An example payload that matches (up until the truncation point) is shown below from wireshark where you can see that the reply includes the first four bytes correctly, but the reply has truncated the 17B request payload to 4B.

The loop in contains_pattern_in_payload() fails on the first iteration. I added some printfs to see what is going on:

cp=0xD15673C8
dp=0xD1566AF8
sizeof(struct timeval)=16
rts->datalen=17
*cp=0x00000000, *dp=0x00000010 

So this loop will execute exactly once and return 0

cp = ((u_char *)ptr) + sizeof(struct timeval);
dp = &rts->outpack[8 + sizeof(struct timeval)];
for (i = sizeof(struct timeval); i < rts->datalen; ++i, ++cp, ++dp) {
    if (*cp != *dp)
        return 0;
}

The above loop bounds may be implemented incorrectly. Based on *dp = 0x10 and the data packet, it appears to be checking the final byte of the payload (and only the final byte, since the loop executes only once based on the start and stop bounds for the variable "i").

It's a bit unclear from the documentation of this function what you intend for it to do:

/*
 * Return 0 if pattern in payload point to be ptr did not match the pattern that was sent  
 */

[paul-demo]

Continued debugging this in ping_common.c It appears that:

hlen = ip->ihl * 4 = 20 = IPv4 header length
cc = starts out as the whole packet length, but then gets hlen subtracted from it, so cc equals just the length of the ICMP packet
icp = (struct icmphdr *)(buf + hlen), so it points to the top of the ICMP packet. sizeof(icmphdr) = 8, so for example icp and icp+1 differ by 8 when you call it here:

contains_pattern_in_payload(rts, (uint8_t *)(icp + 1))

Then when we get into the contains_pattern_in_payload() function, *ptr points to the payload of the ICMP header due to the +1 above.

cp = ((u_char *)ptr) + sizeof(struct timeval);

So cp points to byte 16 now, counting from zero, since sizeof(struct timeval) = 16. So you're essentially ignoring the first 16 bytes of the payload and checking starting at the 17th byte. I assume this is because the first 16 are expected to be a timestamp?

In any case, if the packet is truncated, cp is already pointing beyond the length of the received packet. So you have effectively removed ping's ability to report truncated packets and it will never print "truncated" like it should or like it used to in gather_statistics(). This seems like an unintended consequence since it's expected behvaior for ping to be able to report truncated responses and truncated responses are totally normal (ex: Google.com truncates pings, as do others).

[paul-demo]

I think the main issue is that the "when pattern is provided" part was never implemented, so you're checking the payload even when the pattern was not provided, and going beyond the bounds of the received packet when it is truncated.

[paul-demo]

Furthermore, gather_statistics() already checks the payload! See here:

https://github.com/iputils/iputils/blob/master/ping/ping_common.c#L830

So really the aforementioned CL should be completely rolled back as it broke a working feature of ping and does nothing useful.

[paul-demo]

It's likely I haven't submitted the PR correctly as I don't use forked repos in my git usage, but here it is anyway for you to consider:

#321

[paul-demo]

Thinking about this a bit more, it appears that for the last 4 years at iputils/ping top-of-tree:

• no one has ever observed a truncated reply when sending a request longer than 16B! (https://github.com/iputils/iputils/blob/master/ping/ping_common.c#L809 is unreachable)

• no bit errors have been detected in any payloads longer than 16B!(https://github.com/iputils/iputils/blob/master/ping/ping_common.c#L835 is unreachable)

• no statistics have been generated for any replies with malformed payloads longer than 16B

I'll point out that pretty much all requests and replies are longer than 16B ICMP payload so this is a big problem.

All of these are because gather_statistics() is not called when it should be (https://github.com/iputils/iputils/blob/master/ping/ping.c#L1554)

This basically negates many of the primary use cases of ping! Again, I might be misinterpreting the consequences, but this seems to have the potential for enormous damage, and it's on Ubuntu 20 this way so it's very widespread. The lack of testing to cover these basic cases (bit errors and truncation), or to examine code coverage which would also show it, is pretty appalling. I'm going to reiterate my caveat for the third time that I could be wrong about this, and I'd like one of the owners of this repo to take a closer look.

Edited several times for clarity.

[pavlix]

This basically negates many of the primary use cases of ping!

Care to elaborate which primary use cases are affected and how?

Again, I might be misinterpreting the consequences, but this seems to have the potential for enormous damage, and it's on Ubuntu 20 this way so it's very widespread.

What exactly is the enormous damage here?

The lack of testing to cover these basic cases (bit errors and truncation), or to examine code coverage which would also show it, is pretty appalling.

Are you willing to help providing tests that would cover those?

Note: I haven't looked at the code base for quite some time now. But I was here when the repo started.

[paul-demo]

Hi pavlix,

Sure, I don't mind elaborating on the issue to orient you when you or another maintainer gets a chance to look at the code.

What I mean is that the main purpose of ping is to issue ICMP echo request messages and report the match or mismatch of ICMP echo reply messages generated in response. This is commonly used as the most basic test to discover two things: connectivity (ie, "Can I reach some remote host?") and link quality (ie, "Can I send and receive a correctly formatted message to/from some remote host?").

The issue is that in its current form, iputils/ping, due to the aforementioned commit, does not report on a large class of mismatched responses as it intends to do. I am judging intention based on reading its source code, and also comparing it to how all prior versions of ping have worked. In fact it reports nothing when the code--as well as all historical versions of ping--are clearly designed to report details of these forms of mismatched echo reply messages. Up until the commit I mentioned, it did in fact do that. Now most of that code is unreachable due to that bad commit.

Below are some specific examples to help guide you in understanding the code.

Suppose you receive an ICMP echo reply which has a truncated ICMP payload field, or which is the full payload but the payload has a bit error in any byte 17 or above.

The former (truncation) happens for example if you do this, since Google truncates ICMP echo requests when issuing a reply:

ping -s 100 google.com

The latter (a bit error) can happen if there is a flaky physical connection between any two networked computers on the path between you and the remote host.

In this case (line numbers below refer to ping_common.c):

• ping is supposed to update its counters for statistics generation (lines 738-740). It does not.
• ping is supposed to keep track of timestamps and time-of-flight using the first section of the payload (lines 747-772 and/or 812-824). It does not.
• ping is supposed to check for duplicate replies and report them (lines 777-785 and 825-826). It does not.
• ping is supposed to print basic info about the reply (lines 799-806). It does not.
• ping is supposed to print "(truncated)" if the reply was truncated (lines 808-811). It does not.
• ping is supposed to check the checksum (lines 774-776 and 827-828). It does not.
• ping is supposed to validate the rest of the payload (bytes 17 and above) against the ICMP request that was sent, and report any differences (lines 830-845). It does not.

So basically a large chunk of the "checking" that is implemented in gather_statistics() does not execute as it should and like ping is intended to do. If it were not intended to do this checking, then there would be no reason to have sections like lines 830-845 in the code. It is unreachable dead code which is no longer possible to reach due to the bad commit I linked above.

I hope that helps you in confirming this issue, feel free to reach out with more questions/concerns.

[paul-demo]

@pevik Let me know when you've had a chance to confirm this major bug.

[paul-demo]

Also pinging @sunjeets who was responsible for this issue, should anyone actually be behind that ghost account with 1 commit.

[pevik]

@paul-demo I'm sorry, currently my time for iputils is very limited, I'm not sure when I get into the issue.
@kerolasa @pavlix @jsynacek anybody having time to look into this?

[pevik]

@paul-demo Yes, you're right, that this is a regression:

Running ping from both s20161105 (which does not contain f7710a1) and reverted f7710a1 on master shows the same:

$ ping -c3 -s100 google.com
PING google.com (216.58.212.142) 100(128) bytes of data.
76 bytes from ams15s21-in-f14.1e100.net (216.58.212.142): icmp_seq=1 ttl=116 (truncated)
76 bytes from ams15s21-in-f14.1e100.net (216.58.212.142): icmp_seq=2 ttl=116 (truncated)
76 bytes from ams15s21-in-f14.1e100.net (216.58.212.142): icmp_seq=3 ttl=116 (truncated)

And I can see the packets has been sent

# tcpdump src google.com and icmp -v
08:30:57.024731 IP (tos 0x0, ttl 116, id 0, offset 0, flags [none], proto ICMP (1), length 96)
    ams15s21-in-f14.1e100.net > 192.168.5.2: ICMP echo reply, id 53, seq 1, length 76
08:30:58.028943 IP (tos 0x0, ttl 116, id 0, offset 0, flags [none], proto ICMP (1), length 96)
    ams15s21-in-f14.1e100.net > 192.168.5.2: ICMP echo reply, id 53, seq 2, length 76
08:30:59.028181 IP (tos 0x0, ttl 116, id 0, offset 0, flags [none], proto ICMP (1), length 96)
    ams15s21-in-f14.1e100.net > 192.168.5.2: ICMP echo reply, id 53, seq 3, length 76

Running ping from master I see with tcpdump the same packets has been sent, but there is no output:

$ ping -c3 -s100 google.com
PING google.com (216.58.212.142) 100(128) bytes of data.

But I have to think twice what to do with regression introduced in f7710a1.
@pavlix @kerolasa @jsynacek @nmeyerhans @vapier @nmav any opinion about it?

[paul-demo]

I sent you a PR, see earlier in this thread. The problematic change does nothing at the moment except cause this issue. I recommend you just roll it back as I showed in the PR but obviously you can decide whether to do that or something else.

[tmittelstaedt]

Please guys if there's any way you can put some time into correcting this I would be most appreciative! I am currently dealing with a bug in OpenVPN where it's incorrectly handling MTU size and the OLD behavior of ping is really important for troubleshooting. Thanks!!!!