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

[Mark Doliner]

On Tue, Aug 11, 2009 at 10:28 PM, Elliott Sales de
Andrade<qulogic at pidgin.im> wrote:
> On Wed, Aug 5, 2009 at 3:03 AM, Paul Aurich <paul at darkrain42.org> wrote:
>> And Paul Aurich spoke on 08/01/2009 12:25 PM, saying:
>> > I've attached a debug log of this crashing (with no relevant changes in
>> > MSN
>> > from 8351342d3a30c75649de9c3d4abc7b6f45ce8f82, which is im.pidgin.pidgin
>> > head at mtn.pidgin.im).
>> >
>> > I'm not familiar with MSN, but is it pertinent that they're using the
>> > base
>> > ID from the "reply of the first message":
>> >
>> >         # Get the base id from the reply of the first message,..
>> >         base_id = sb_msnms_object['p2p_binheader'][4:8]
>> >
>> > It looks abnormal, and the slpmsg that's found during the processing of
>> > the
>> > second message is the *ACK* to the first message (instead of the first
>> > slpmsg created, which actually does have a buffer) -- it's not visible
>> > in
>> > this debug log, but I verified in a second run.
>> >
>> > Anyway, log attached
>> > ~Paul
>> >
>>
>> To prod this process along some more, I'm attaching a patch and debug log
>> (of receiving the second message) that, to the best of my knowledge (and
>> headache), fixes this (specific) issue.  I believe my initial suspicions
>> to
>> be correct (the exploit is targeting the ACK of the first message, which
>> is
>> a MsnSlpMessage which contains no FILE* or allocated buffer). File
>> transfer
>> still works fine, so it doesn't (seem) to have any adverse impact.
>
> This seems about right, but I haven't been able to reproduce with the
> supplied PoC.

Yes, this change looks good and does indeed fix the crash for me when
testing with their proof of concept script.

>> 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.

Yeah, this seems like it would be a complete absolute fix for this
problem.  How does an slpmsg get to a point where size is set to some
large value but buffer is NULL?  I think we should fix this and
include it in the patch to packagers.

-Mark