MSN DC null pointer dereference

[Elliott Sales de Andrade]

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.

> Regards,
> Stu.
>

-- 
Elliott aka QuLogic
Pidgin developer