Security report

[Matt Jones]

Hi Daniel, great, thanks. Looks good on my end.  I also agree it is
probably not triggerable by a remote user, but hard to say for sure.

Is there a date on the next major release?

I have a few more issues I will send through soon, just have been very busy.

On Sun, Nov 10, 2013 at 8:54 AM, Daniel Atallah <datallah at pidgin.im> wrote:
>
>
>
> 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