MSN DC null pointer dereference

[Elliott Sales de Andrade]

On Thu, Dec 16, 2010 at 12:52 PM, Elliott Sales de Andrade
<qulogic at pidgin.im> wrote:
> On Tue, Dec 14, 2010 at 10:15 PM, Stu Tomlinson <stu at nosnilmot.com> wrote:
>> Hello all,
>>
>> I fear we have a potential DoS bug through null pointer dereference that
>> can be triggered through MSN direct connection.
>>
>
> It appears you are correct. Fortunately, we are close to a release,
> and probably won't have to scramble to fix it.
>
>> From this Fedora bug:
>> https://bugzilla.redhat.com/show_bug.cgi?id=661606
>> And the backtrace:
>> https://bugzilla.redhat.com/attachment.cgi?id=467697
>>
>> #4  msn_slplink_process_msg (slplink=0x2effe90, part=0x0) at slplink.c:580
>>        slpmsg = <value optimized out>
>>        header = <value optimized out>
>>        offset = <value optimized out>
>> #5  0x00007f4c518f3045 in msn_dc_process_packet (data=0x2e5cd60, fd=<value optimized out>, cond=<value optimized out>) at directconn.c:635
>>        part = 0x0
>>
>> I haven't looked at this in any detail and probably won't have time to
>> anytime soon, but it looks like if msn_dc_process_packet() is called
>> with a short packet when in state DC_STATE_ESTABLISHED,
>> msn_slpmsgpart_new_from_data() will return NULL "part" if the packet is
>> smaller than the size of MsnP2PHeader struct, which is then dereferenced
>> by msn_slplink_process_msg().
>>
>
> In fact, it's pretty easy to reproduce with libpurple. Just go to the
> DC_STATE_HANDSHAKE_REPLY case, and add a call to msn_dc_send_foo(dc);
> which will send a non-slp packet. Then initiate something long enough
> to use a DC (sending a large file is a guaranteed crash, a buddy icon
> may not take long enough).
>
>> Someone more familiar with MSN direct connections would need to
>> determine why msn_dc_process_packet() is being called with a short
>> packet. Simple "fix" might be to add NULL check on "part" in
>> msn_slplink_process_msg() but that smells like papering over the actual
>> problem.
>>
>
> The simple fix is probably adequate. I will have to take a look at
> what the official client does and see if it drops the DC back to the
> old SB connection, or simply ignores the short packets. It may also be
> that the short packets have some other meaning, but I have not seen
> any reference to it as yet.
>

It appears that we are receiving short packets because the official
client thinks we implement p2pv2. Obviously, since we don't do that
yet, it shouldn't be any harm if we just drop those packets.

I have attached a patch to do so. The patch also fixes an invalid
(logic-wise) copy of the buffer. This copy is harmless, since it would
never be acted upon once the packet is ignored, but it's wrong,
nonetheless.

>> Regards,
>> Stu.
>>
>
> --
> Elliott aka QuLogic
> Pidgin developer
>

-- 
Elliott aka QuLogic
Pidgin developer
-------------- next part --------------
A non-text attachment was scrubbed...
Name: purple-msn-dc-null-deref.diff
Type: application/octet-stream
Size: 1093 bytes
Desc: not available
URL: <http://pidgin.im/cgi-bin/mailman/private/security/attachments/20101219/b9f2ef05/attachment.obj>