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

[Mark Doliner]

On Wed, Aug 12, 2009 at 5:38 PM, Mark Doliner<mark at kingant.net> wrote:
> 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.

What do people think about the attached revised patch?  It's Paul's
original patch combined with the suggestion from his comment #4.  I
tested each piece by itself and either one is enough to prevent this
crash.  I've dug through the code and I feel this fix is complete.

It applies cleanly to mtn trunk, and applies correctly to 2.5.8 with a
wee offset and 1 fuzz.

-Mark
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fix_for_msn_remote_crash.diff
Type: text/x-patch
Size: 908 bytes
Desc: not available
URL: <http://pidgin.im/cgi-bin/mailman/private/packagers/attachments/20090813/85cf4cb6/attachment.bin>