[Fwd: Re: [Advisories] Libpurple security vulnerability CORE-2009-0727]

[Paul Aurich]

And Elliott Sales de Andrade spoke on 08/11/2009 10:28 PM, saying:
>     3. Why aren't MsnSlpMessage:s destroyed by the varying
>     msn_slplink_send_*
>     functions? Actually...how are they destroyed? The msn_slplink_destroy()
>     function doesn't do anything with any slpmsgs remaining, so they must be
>     being freed somewhere (or I've never used my MSN account while running
>     Valgrind, which is way more probable).
> 
> 
> After some time, msn_slpcall_timeout is called which destroys the
> MsnSlpCall. This destroys all the MsnSlpMessages linked to that call.

So memory isn't being leaked, but the messages (like the ACK in this
particular case) are being kept around longer than they need to be.

> 
>     4. There's no good reason a msg with no buffer should be allowed to
>     get to
>     the memcpy. slplink_process_msg should also be changed to read:
>            if (slpmsg->fp)
>            {
>                    /* fwrite */
>            }
>            else if (slpmsg->size && ***slpmsg->buffer***)
>            {
>                    /* bounds checking for oversized message and then
>     write */
>                    /* memcpy */
>            }
>            else
>            {
>                    /* error */
>            }
> 
> 
> Yes, this code section is totally wrong. It doesn't look very extensible
> and seems very file/object transfer specific.
> 
>     5. In the above, when the bounds checking fails (and in the final else),
>     shouldn't the slpmsg be destroyed ?
> 
> 
> I'm not sure where you mean, but I don't think so. The initial
> MsnSlpMessage is used to hold on to the # transferred bytes and total
> file size. It should only be destroyed at the end of the slpcall.

The code currently looks like this:
	if (slpmsg->fp)
	{
		/* fseek(slpmsg->fp, offset, SEEK_SET); */
		len = fwrite(data, 1, len, slpmsg->fp);
	}
	else if (slpmsg->size)
	{
		if (G_MAXSIZE - len < offset || (offset + len) > slpmsg->size)
		{
			purple_debug_error("msn",
				"Oversized slpmsg - msgsize=%lld offset=%" G_GUINT64_FORMAT " len=%"
G_GSIZE_FORMAT "\n",
				slpmsg->size, offset, len);
			g_return_if_reached();
		}
		else
			memcpy(slpmsg->buffer + offset, data, len);
	}


If there is an oversized write (where the overflows were before and the
g_return_if_reached() is now) and in my proposed case where both the fp and
buffer are NULL, shouldn't that be treated as a fatal error and terminate
the slpcall?

Just returning from the function in those situations strikes me as being
too lax.

>     6. A brief audit of every call to msn_slpmsg_new makes me think khc,
>     QuLogic, or Stu needs to look at all of those and check that the
>     message is
>     destroyed when it should be. msn_slpmsg_sip_new looks like it
>     allocates and
>     returns an MsnSlpMessage*, so its usages should also be looked at.
> 
> 
> What's that? It took me long enough just to figure out what it was doing
> to answer your questions. :P

Yes, I know exactly what you mean.

Basically, I looked for all the other functions in the prpl that seem to
allocate MsnSlpMessages and then not free them explicitly (and therefore
could be used to exploit the memcpy since there's a NULL buffer hanging
around).

I guess if we just the code in slplink.c so there must be a buffer
allocated before getting to the memcpy, this should be okay.

I just think the MsnSlpMessages should have a more tightly scoped lifetime. :)

~Paul

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 898 bytes
Desc: OpenPGP digital signature
URL: <http://pidgin.im/cgi-bin/mailman/private/packagers/attachments/20090812/e2fe3e13/attachment.pgp>