What’s wrong with this code (other than the fact the messages are discarded)?

#!c
void read_messages(int fd, int num_msgs) {
  char buf[1024];
  size_t msg_len, bytes_read = 0;

  while(num_msgs--) {
    read(fd, &msg_len, sizeof(size_t));
    if (bytes_read + msg_len > sizeof(buf)) {
      printf("Buffer overflow prevented!\n");
      return;
    }
    bytes_read += read(fd, buf+bytes_read, msg_len);
  }
}

If you answered “nothing”, you’d be missing a significant security issue. In fact, this function contains a trivial buffer overflow. By supplying a length 0 < len_a < 1024 for the first message, then a length INT_MAX-len_a ≤ len_b < UINT_MAX, the value bytes_read + msg_len wraps around past UINT_MAX and is less than sizeof(buf). Then the read proceeds with its very large value, but can only read as much data as is available on the file descriptor (probably a socket, if this is a remote exploit). So by supplying enough data on the socket, the buffer will be overflowed, allowing to overwrite the saved EIP.

One way to fix this vulnerability is to move the bytes_read on the other side of the comparison.

#!c
void read_messages(int fd, int num_msgs) {
  char buf[1024];
  size_t msg_len, bytes_read = 0;

  while(num_msgs--) {
    read(fd, &msg_len, sizeof(size_t));
    if (msg_len > sizeof(buf) - bytes_read) {
      printf("Buffer overflow prevented!\n");
      return;
    }
    bytes_read += read(fd, buf+bytes_read, msg_len);
  }
}

Since 0 ≤ bytes_read ≤ sizeof(buf) the right side will not underflow, and msg_len will obviously always be within the range of a size_t. In this case, we should not be vulnerable to a buffer overflow.

####Other Reading

• Basic Integer Overflows (Phrack)