Reporting bugs

[Matt Jones]

Hello,

I have recently started to look over libpurple source code in some
spare time to help with identifying bugs that could lead to security
issues.

The approach I've taken is to try to identify dangerous or flawed
constructs (top down methodology) with the goal of killing some
potential flaws and preparing notes that could help a more thorough
audit.  I haven't spent time to try to write POC code to trigger the
potential issues I've found.

Below is the first bug I'd like to send through.  It would help a lot
to get some feedback on the level of detail, value, etc. of the
information below so I can factor this in when submitting other issues
in the future.

Please let me know if you have any questions.  Hope this helps.

Regards,

Matt


Potential heap overflow in process_chunked_data (libpurple/Util.c):

static void
process_chunked_data(char *data, gsize *len)
{
gsize sz;
gsize newlen = 0;
char *p = data;
char *s = data;

while (*s) {
 /* Read the size of this chunk */
 if (sscanf(s, "%" G_GSIZE_MODIFIER "x", &sz) != 1) [1]
 {
  purple_debug_error("util", "Error processing chunked data: "
    "Expected data length, found: %s\n", s);
  break;
 }
 if (sz == 0) {
  /* We've reached the last chunk */
  /*
   * TODO: The spec allows "footers" to follow the last chunk.
   *       If there is more data after this line then we should
   *       treat it like a header.
   */
  break;
 }

 /* Advance to the start of the data */
 s = strstr(s, "\r\n");
 if (s == NULL)
  break;
 s += 2;

 if (s + sz > data + *len) { [2]
  purple_debug_error("util", "Error processing chunked data: "
    "Chunk size %" G_GSIZE_FORMAT " bytes was longer "
    "than the data remaining in the buffer (%"
    G_GSIZE_FORMAT " bytes)\n", sz, data + *len - s);
}

 /* Move all data overtop of the chunk length that we read in earlier */
 g_memmove(p, s, sz);
 p += sz;
 s += sz;
 newlen += sz;
 if (*s != '\r' && *(s + 1) != '\n') { [3]
  purple_debug_error("util", "Error processing chunked data: "
    "Expected \\r\\n, found: %s\n", s);
  break;
 }
 s += 2;
}

/* NULL terminate the data */
*p = 0;

*len = newlen;
}

Description:

This function is handling chunked data, using sscanf to extract an
unsigned int from client-supplied data over the network [1].  There
are a few problems present:

1. The potential overflow check [2] is only logged via
purple_debug_error and is missing a break; to prevent an actual
overflow, this is because purple_debug_error is a debug macro and does
not fatally exit or abort.

2. The check to see if there's a potential overflow [2] is susceptible
to an integer overflow due to sz being attacker controlled and not
validated at all, meaning the validation check can be bypassed.

3. There is a potential out of bounds read [2] due to insufficient
bounds checking for the CR and LF read.

The following commit
http://pidgin.im/pipermail/commits/2009-July/013625.html shows the
overflow check being added.

Suggested fix:

The sz parameter read over the wire should be validated if it's of a
sane value, a break; should be added if the validation check is true,
and 2 bytes for the CRLF should be considered for the validation
check.  For example:

 #define MAX_CHUNK_SIZE 4096

 if (sz < 0 || sz > MAX_CHUNK_SIZE || s + sz + 2 > data + *len) { [1]
  purple_debug_error("util", "Error processing chunked data: "
    "Chunk size %" G_GSIZE_FORMAT " bytes was longer "
    "than the data remaining in the buffer (%"
    G_GSIZE_FORMAT " bytes)\n", sz, data + *len - s);
  break;
}