OpenBSD Ping Bug On Record Route Option Parsing

Summary

OpenBSD's ping has a function called pr_ipopt which displays options encoded in network packets. I found a bug in the option record decoding. The record format is described in RFC 791,

IPOPT_RR: If Pointer Greater Than Length

The ip option IPOPT_RR normally uses the pointer field as a size for the ip addr array which follows. However, if the pointer field is greater than the length, it defaults to using the length field instead.

The problem arises since the pointer field has an offset of 4 (IPOPT_MINOFF) but the length has an offset of 3. The offset 4 (IPOPT_MINOFF) is subtracted from the size whether or not the code has defaulted to the length field. There is then a check for empty list which fails since (unsigned) negative 1 will pass the check i<=0 (equivalent to i==0, for unsigned i). This is the first failure point: When the ip addr list is empty, the code will continue and try reading it.

I could not find a clear, explicit definition for the min value of the length field in RFC 791, which describes the relevant data structures. Only the pointer min (4) is explicitly mentioned. However, the length min (3) can be inferred from the formula given for the overall option length, and also from code in ping which builds an option byte sequence.

There is then a loop which does i-=4, assuming that this will eventually bring i to zero. However, if i starts as the length field it will eventually go to 3 and then -1. i is not used to index the data, so the fact that it is off-by-one doesn't cause a problem there. The problem is that the ip list display loop doesn't stop. The loop would be infinite except there is another counter which restricts the list to MAX_IPOPTLEN (in my code 10 addresses are printed and the list is 'truncated'). In the meantime possibly invalid data has been read, parsed and printed.

The code also checks i<=0 in a few places but since i is unsigned this is just i==0. Probably this code was correct until some other programmer changed variables i,j to unsigned.

void
pr_ipopt(int hlen, u_char *buf)
{
  u_int i, j;
  ...
  case IPOPT_RR:
    j = *++cp;              /* get length */
    i = *++cp;              /* and pointer */
    hlen -= 2;
    if (i > j)
      i = j;
    i -= IPOPT_MINOFF; // maybe underflow here
    if (i <= 0) // nonsense check for unsigned
      continue;
    ...
    // loop
      ...
      i -= 4; // eventually underflow here

The only example of valid option data I can find is the option created by openbsd ping itself for the -R, record route option. I used a copy of that code to build a byte sequence, then forced the pointer field > length field and passed the sequence to a copy of the function pr_ipopt. This may not happen in normal cases, but ping/pr_ipopt parse network packets, which could be malicious.

enum{hdrsize=sizeof(struct ip)};

// original code (with header added for pr_ipopt() to skip)
enum{NROUTES=9};
static u_char rspace[hdrsize + 3 + 4 * NROUTES + 1];
rspace[hdrsize+IPOPT_OPTVAL] = IPOPT_RR;
rspace[hdrsize+IPOPT_OLEN] = sizeof(rspace)-1;
rspace[hdrsize+IPOPT_OFFSET] = IPOPT_MINOFF;

// to trigger bug force pointer > length
if(testbug) rspace[hdrsize+IPOPT_OFFSET] = rspace[hdrsize+IPOPT_OLEN] + 1;

// call
pr_ipopt(sizeof(rspace), rspace);

This test on the pr_ipopt() function, using a option sequence created by ping itself (so, presumably of valid structure) (and modified to be malicious, but structure conforming), causes an infinite loop (which is then truncated by some tests) and the read of invalid memory (also calling other functions on the invalid memory read).