Security report

[Daniel Atallah]

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.

Thanks for the report.

-D
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://pidgin.im/cgi-bin/mailman/private/security/attachments/20130919/54ebf4ba/attachment.html>