Sure his fix made it more robust, but the problem only showed up because he had his fuzzer bypass validation that was being done outside of the function.
Although not exploitable, the bug is still an issue. Here are reasons to fix it.
* Puts burden on readers to know or find out if something else already protects the bug from being exploitable, instead of just having a check in the code. That's trivia masquerading as an optimization.
* A bug or change in behavior in one program (the kernel) could cause a bug in another program (ping) due to assumptions, which may not even have been documented as never changing guarantees. Could make a bug somewhere grow into a worse bug elsewhere, as well as being harder to debug.
* It allows the program to be fuzzed or statically analyzed for any bugs without having to constrain what inputs are considered valid enough, adding maintenance costs and opening for accidentally limiting too much and hiding exploitable bugs.
* Another option to be clear that this is impossible and thus correct code would have been to assert that the user data never looks like this. A failed assert is better than an infinite loop, so this is an improvement. However this would make it blindingly clear that user input is trusted, making it too obvious to ignore the next improvement; to check and handle the issue, even if that input passed through another program first.
Below is the small framework we use to fuzz a network client. It works by creating a Unix socket, forking, then having the client (under test) on one side of the socket and a small loop which reads the input file into the socket on the other side.
In brief (cp[IPOPT_OLEN] - 1) is the delta, not (cp[IPOPT_OLEN]); according to the original code. (I don't pretend to know where the original -1 comes from).
This means that the IF-statement check is incorrect.
instead of repeating expressions like this: (cp[IPOPT_OLEN] - 1) and adding variants elsewhere like this: cp[IPOPT_OLEN]. It's confusing and causes bugs. Naming things helps and costs nothing.
Thank you for the clarification. I did not look at the surrounding code.
However, my point still stands that extracting the complex expression as delta would have helped and possibly avoided the original bug.
int const delta = (cp[IPOPT_OLEN] - 1);
Further proof is that the original author first commited a broken fix. Again this was caused by the confusion between delta and delta-1, which would have been clear had delta been explicitly named.
This sort of code is just begging to be misunderstood.
- if (cp[IPOPT_OLEN] > 0 && cp[IPOPT_OLEN] < hlen) {
The "bug" is from assuming you are parsing a valid packet. The option length field is the length of the option legnth and option data combined. The minimum value it can have is 1 which is when the length of the option data is 0.
cp[IPOPT_OLEN] refers to the option length. Your delta variable is the length of the option data. The confusing part is that in most network protocols length fields do not count the size of the length number.
MANUAL SECTIONS
The standard sections of the manual include:
1 User Commands
2 System Calls
3 C Library Functions
4 Devices and Special Files
5 File Formats and Conventions
6 Games et. al.
7 Miscellanea
8 System Administration tools and Daemons
It’s worth mentioning that in online conversations or documentation, rather than referring to manpages specifically, it’s often used simply to disambiguate “the Unix command ping” from anything else that might be called ping. E.g. in a casual conversation the word “open” can refer to a hundred different things, but if someone writes “open(2)” it’s unambiguous that they’re talking about the syscall.
Kinda like how macOS users will write “Mail.app” to disambiguate the built-in email client from the concept of email in general.
The section of the system man pages. It's often used to disambiguate a tool from a libc function, when there are two manpages with the same name, but in different sections.
I believe it's an old convention of indicating what section of the manpages describes the command. Section 8 is "superuser and system administration commands".
Interesting comment about the finding in the CVS log [0].
[0] https://cvsweb.openbsd.org/src/sbin/ping/ping.c#rev1.248