Reporting bugs

[Matt Jones]

Hello,

I just want to follow up on this email.  Please let me know if you
have any questions/feedback on this issue and also any suggestions or
comments on how you'd prefer I go about reporting issues in the
future.

Thanks,

Matt

On Thu, Aug 8, 2013 at 6:58 PM, Matt Jones <matt at volvent.org> wrote:
> 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;
> }