contiki-ng / contiki-ng

Contiki-NG: The OS for Next Generation IoT Devices
https://www.contiki-ng.org/
BSD 3-Clause "New" or "Revised" License
1.3k stars 698 forks source link

Out of Bound Read when processing TCP MSS option #2401

Closed AmPaschal closed 1 year ago

AmPaschal commented 1 year ago

There is a possible out of bound read when a TCP packet containing MSS options is being processed.

The function snippet below shows the code that processes the MSS option.

/* Parse the TCP MSS option, if present. */
  if((UIP_TCP_BUF->tcpoffset & 0xf0) > 0x50) {
    for(c = 0; c < ((UIP_TCP_BUF->tcpoffset >> 4) - 5) << 2 ;) {
      opt = uip_buf[UIP_IPTCPH_LEN + c];
      if(opt == TCP_OPT_END) {
        /* End of options. */
        break;
      } else if(opt == TCP_OPT_NOOP) {
        ++c;
        /* NOP option. */
      } else if(opt == TCP_OPT_MSS &&
                uip_buf[UIP_IPTCPH_LEN + 1 + c] == TCP_OPT_MSS_LEN) {
        /* An MSS option with the right option length. */
        tmp16 = ((uint16_t)uip_buf[UIP_IPTCPH_LEN + 2 + c] << 8) |
          (uint16_t)uip_buf[UIP_IPTCPH_LEN + 3 + c];
        uip_connr->initialmss = uip_connr->mss =
          tmp16 > UIP_TCP_MSS? UIP_TCP_MSS: tmp16;

        /* And we are done processing options. */
        break;
      } else {
        /* All other options have a length field, so that we easily
           can skip past them. */
        if(uip_buf[UIP_IPTCPH_LEN + 1 + c] == 0) {
          /* If the length field is zero, the options are malformed
             and we don't process them further. */
          break;
        }
        c += uip_buf[UIP_IPTCPH_LEN + 1 + c];
      }
    }
  }

Each iteration of the for loop only checks that at the start of the iteration, c is within the bounds of TCP option. Within the iteration, c is increased and used to access the buffer without any further checks. Hence, the MSS value can be computed using bytes read out of bound.

For example, if a 4 byte option (ox01010204) is added to a packet, the first 2 bytes are skipped (TCP_OPT_NOOP), the 3rd and 4th byte indicates there is an MSS option with 4 byte of length. Hence, this code would read 2 bytes outside the bounds of the buffer.

MartinNPN commented 1 year ago

I think this Merge request should fix it: https://github.com/contiki-ng/contiki-ng/pull/2434