Security report

[Daniel Atallah]

On Thu, Sep 19, 2013 at 2:32 PM, Daniel Atallah <daniel.atallah at gmail.com>wrote:

>
>
>
> On Thu, Sep 12, 2013 at 8:14 PM, Matt Jones <matt at volvent.org> wrote:
>
>> Hi,
>>
>> Here is another issue that should be corrected in the libpurple
>> source.  More to come soon.
>>
>> Regards,
>>
>> Matt
>>
>> msn_message_gen_payload function in libpurple/protocols/msn/Msg.c file.
>>
>> char *
>> msn_message_gen_payload(MsnMessage *msg, size_t *ret_size)
>> {
>> GList *l;
>> char *n, *base, *end;
>> int len;
>> size_t body_len = 0;
>> const void *body;
>>
>> g_return_val_if_fail(msg != NULL, NULL);
>>
>> len = MSN_BUF_LEN;
>>
>> base = n = end = g_malloc(len + 1);
>> end += len;
>>
>> /* Standard header. */
>> if (msg->charset == NULL)
>> {
>>  g_snprintf(n, len,
>>       "MIME-Version: 1.0\r\n"
>>       "Content-Type: %s\r\n",
>>       msg->content_type);
>> }
>> else
>> {
>>  g_snprintf(n, len,
>>       "MIME-Version: 1.0\r\n"
>>       "Content-Type: %s; charset=%s\r\n",
>>       msg->content_type, msg->charset);
>> }
>>
>> n += strlen(n);
>>
>> for (l = msg->header_list; l != NULL; l = l->next)
>> {
>>  const char *key;
>>  const char *value;
>>
>>  key = l->data;
>>  value = msn_message_get_header_value(msg, key);
>>
>>  g_snprintf(n, end - n, "%s: %s\r\n", key, value);
>>  n += strlen(n);
>> }
>>
>> n += g_strlcpy(n, "\r\n", end - n); [1]
>>
>> body = msn_message_get_bin_data(msg, &body_len); [2]
>>
>> if (body != NULL)
>> {
>>  memcpy(n, body, body_len); [3]
>>  n += body_len;
>>  *n = '\0'; [4]
>> }
>>
>> if (ret_size != NULL)
>> {
>>  *ret_size = n - base;
>>
>>  if (*ret_size > 1664)
>>   *ret_size = 1664;
>> }
>>
>> return base;
>> }
>>
>> Description:
>>
>> The function builds an MSN message including the headers and binary
>> data body. It begins by setting a static header, then pushes any
>> optional headers one at a time.
>>
>> At [1] a CRLF is appended and n is incremented, however g_strlcpy
>> always returns the size of the source buffer, so this will always
>> increment n by 2, regardless if there is room for it.
>>
>> At [2] the body is fetched using msn_message_get_bin_data, which is a
>> simple function that just returns msg->body and sets body_len to its
>> length (msg->body_len).
>>
>> At [3] body is concatenated onto where n is pointing to with no
>> validation if there's sufficient room for it, which could result in a
>> heap overflow.
>>
>> At [4] n is NULL terminated, this could be out-of-bounds due to either
>> [1] pr [3].
>>
>> Suggested fix:
>>
>> Do not increment a pointer using the return value from g_strncpy
>> without validating there's sufficient room for the data.  Perform a
>> validation check that the body returned from msn_message_get_bin_data
>> can be copied to its destination.
>>
>
> I thought that this was the same issue as another one that has been fixed
> for the next release, but now that I've had a chance to look a bit harder,
> it appears to be a separate issue.
>

I finally got around to fixing this.

This only happens when generating a message to be sent, so it doesn't
appear to be remotely triggerable.

The patch that has been committed to the private security repo is attached
-  it will be merged when a release occurs.
It didn't really need to go into the private repo, but I figured committing
it there gave someone more MSN savvy a chance to second-guess the remotely
triggerable part of my evaluation.

-D
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://pidgin.im/cgi-bin/mailman/private/security/attachments/20131109/acb3612a/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 2a77da513a03.patch
Type: text/x-diff
Size: 839 bytes
Desc: not available
URL: <http://pidgin.im/cgi-bin/mailman/private/security/attachments/20131109/acb3612a/attachment.patch>