Incorrect Bug Fix For 24yr Old Ping Bug

Interesting Article on Fuzzing Ping with AFL

An interesting article on fuzzing ping (by extracting some internal function which parses network data), ends with a patch , reproduced here:

-  hlen = hlen - (cp[IPOPT_OLEN] - 1);
-  cp = cp + (cp[IPOPT_OLEN] - 1);
+  if (cp[IPOPT_OLEN] > 0 && (cp[IPOPT_OLEN] - 1) <= hlen) {
+    hlen = hlen - (cp[IPOPT_OLEN] - 1);
+    cp = cp + (cp[IPOPT_OLEN] - 1);
+  } else
+    hlen = 0;

There is a bug in the fix. In fact, the same no-progress bug as it intends to fix. I speculate that this bug is caused by a lack of extracting common subexpressions into constants, thus allowing the bug to hide in plain sight.

No slight is intended toward the original author, whose article was both interesting and educational, and against which this post is a mere nitpick.

Isolate Awkward Expressions

int const delta = (cp[IPOPT_OLEN] - 1);
-  hlen -= delta;
-  cp += delta;
+  if (cp[IPOPT_OLEN] > 0 && delta <= hlen) {
+    hlen -= delta;
+    cp += delta;
+  } else
+    hlen = 0;

Now we have a very similar ugly expression cp[IPOPT_OLEN]. Noting that it is actually equal to delta+1. Let's rewrite:

-  if (cp[IPOPT_OLEN] > 0 && delta <= hlen) {
+  if ((delta+1) > 0 && delta <= hlen) {

That's terrifying, so we notice that x+1>0 => x>-1, or, nicelier x>=0 (for integer x).

+  if (delta >= 0 && delta <= hlen) {
+    hlen -= delta;
+    cp += delta;

Now, with this clarity, we can see the bug.

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

Of course, there may be code elsewhere which somehow prevents this situation, I have only looked at the patch given in the blog post above.

I'm also not so sure about the else clause, just setting hlen=0 (I suppose it may cause a loop exit somewhere else). I will ignore that as out of scope.

The Proper Way

That repeated expression (cp[IPOPT_OLEN] - 1) in the original code jarred me and I would have replaced it immediately. Later on the expression is repeated even more, compounding the horror. If delta were isolated early the if statement would have been trivial to formulate correctly.

int const delta = (cp[IPOPT_OLEN] - 1);
-  hlen -= delta;
-  cp += delta;
+  if(delta>0 && delta<=hlen){
+    hlen -= delta;
+    cp += delta;
+  } else
+    hlen = 0;

There is no runtime cost to this sort of thing. The delta is a const and will be optimized away by the compiler, no extra stack allocation will be created.

No doubt I would have made some different, stupid misteak instead...

Further Thoughts

Looking further at the surrounding code. I have another thought: Parser code which tries to update the pointers at the end of the loop and then undo that change in some cases, is always error prone.

It's better that each case updates the pointers depending on what is parsed in that case. Yes, it means some repetition in the other cases but it was cause of the original bug. We would have avoided the -1 which caused the problem.

Length fields should be unsigned since we know they are 0..N. Indeed the length is, correctly, unsigned. There is no need to check unsigned >= 0, that's redundant. Note: In this structure the length includes the length of the following field, and the length byte itself.

size_t hlen = ....;
while(hlen > sizeof(struct ip)){
  ...
  size_t const olen = cp[IPOPT_OLEN];
  if(olen<=hlen){
    hlen -= olen;
    cp += olen;
  } else
    hlen = 0;
}