> bluhm points out that the network stack in the kernel would not let such packets through to userland.

Interesting comment about the finding in the CVS log [0].

[0] https://cvsweb.openbsd.org/src/sbin/ping/ping.c#rev1.248

Is this even a bug? An invalid length is already being checked by the kernel.

https://github.com/openbsd/src/blob/4628c0e6cf9bd4a6d679c314...

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.

It's called "Defence in depth".

There could be an unintentional or flawed kernel change. Someone could use the code on a different OS with a different behaving or buggy kernel.

You could also consider it high coupling between two completely different code bases.

Or, the crime of making someone understand everything in order to understand some thing...

> We also need to get rid of the validation of the data packet

yep, he recognizes this also. strange to call this a bug then

i also find it strange that …

> As someone who has done a lot of work on ping(8)

… ping has a lot of work done to it

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.

Rest in peace, Mike. I knew him as an undergrad at Johns Hopkins, then never saw him again before his untimely death.

Didn't know about Mike, very sad. Don't know what I'd do without `ping`.

Here's the Wikipedia link: https://en.wikipedia.org/wiki/Mike_Muuss

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.

https://gitlab.com/nbdkit/libnbd/-/tree/master/fuzzing

I believe there is a bug in the bug fix.

I've explained it here: https://blog.deckc.hair/2022-12-11-incorrect-bug-fix-for-24y...

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.

It is interesting why you chose to write a blog article on a diff, without looking at it in context. What is your end game, lol.

:) I am glad to have enterained you.

My end game is to get people to write

  int const delta = (cp[IPOPT_OLEN] - 1);

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.

There is no bug.

>delta is allowed to be zero, we make no progress (as in the original bug)

It's not zero progress because the for loop increments cp every iteration.

In the original bug delta = -1.

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) {

+ if (cp[IPOPT_OLEN] > 0 && (cp[IPOPT_OLEN] - 1) <= 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.

And `test.c` contains yet another bug, namely using `memcpy` in the wrong direction.

What does the (8) mean after ping or in other command's man pages?

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.

And it's used to disambiguate the same name in different sections, for example:

   $ man 1 printf # prints printf(1)
   $ man 3 printf # prints printf(3)
   $ man printf   # prints printf(1)

(See `man man` in your own installation.)

For OpenBSD that would be http://man.openbsd.org/man.1#s

Further details here: https://unix.stackexchange.com/a/3587

What does the (8) in ping(8) signify?

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".

https://www.kernel.org/doc/man-pages/

It's the category for ping's man page (System administration commands and daemons).