MXit security flaws

[Mark Doliner]

On Mon, Dec 17, 2012 at 2:57 PM, Andrew Victor <Andrew.Victor at mxit.com> wrote:
>> CID 732102: (This is the most critical one)
>> * String not null terminated. In mxit_cb_http_read: A character
>> buffer that has not been null terminated is passed to a function
>> expecting a null terminated string
>>  * libpurple/protocols/mxit/http.c:133 -> ch = strstr( buf, HTTP_11_SEPERATOR );
>
> Definitely a valid issue that needs fixing, and as you pointed out a possible buffer overrun.

Eek.  I took a stab at fixing this.  Can someone please review the
attached patch?  Andrew, it would be especially helpful if you could
apply it, compile, and do some testing.  I tested and I was able to
set my icon and send a file between my test accounts, but I'd feel
much more confident knowing that you tested it, as well.

I think I fixed both problems (strstr on a non-nul terminated string
and potential buffer overflow).  I changed the code to write directly
to session->rx_dbuf instead of using a small temp buffer.  This lets
us avoid copying headers back and forth.  And I added a few more
purple_debug_error messages, and split up the "read == 0" and "read <
0" cases.

We've had similar buffer overflows to this in the past in our MSN code
and in our util.c fetch_url() code.  I'm hoping we can avoid future
problems by using Tomasz's centralized libpurple/http.c and
libpurple/http.h in 3.0.0, but I haven't looked at that code yet.

>> CID 732105:
>> * Copy into fixed size buffer. In mxit_encrypt_password: A source
>> buffer of statically unknown size is copied into a fixed-size
>> destination buffer
>>  * libpurple/protocols/mxit/cipher.c:93 -> strcat( pass,
>> session->acc->password );
>
> This code has been improved and fixed in the 3.0.0 tree.
> http://hg.pidgin.im/pidgin/main/diff/b9ede4a1435b/libpurple/protocols/mxit/cipher.c
> http://hg.pidgin.im/pidgin/main/diff/b4729e4322f3/libpurple/protocols/mxit/cipher.c
>
> We could possibly just use the version from the 3.0.0 tree.

This wasn't a security problem, right?  As in, it wasn't possible for
a remote user or remote server to cause this field to overflow, right?

> There are a number of MXit-specific fixes in the mxit-2.x.y branch of the Mercurial repo.  Most of them are backported from the 3.0.0-dev tree.  Would you consider merging the mxit-2.x.y branch, or do you specifically only want fixes for the above issues?

Andrew, you took care of all the desired merges, right?  Or are there
more changes you're wondering about?  The merges you made looked
reasonable to me.  In general I've been trying to avoid changing our
default-2.x.y branch, to keep it stable and to reduce the amount of
work merging those changes into "default."  But you guys are
developing your prpl more actively than most other parts of Pidgin and
I know it would be painful for you to have to wait until 3.0.0 for
your changes to see the light of day, so I think it's very reasonable
for you to continue making changes to the default-2.x.y branch as you
see fit.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fix_for_CVE-2013-MARK3.diff
Type: application/octet-stream
Size: 5659 bytes
Desc: not available
URL: <http://pidgin.im/cgi-bin/mailman/private/security/attachments/20130129/47531883/attachment.obj>