mxit_add_buddy flaw

[Andrew Victor]

hi Daniel,

It shouldn't be remotely triggerable - the value is base64-encoded in mxit_show_search_results(), and any username value received from the server would never contain a "#" character.

History: In the beginning of MXit, the UserId of an account was the person's mobile number.  (A few years back we switched to auto-generated Id's but those existing UserId's could not be changed).  We didn't want the "Search for contact" results in Pidgin to possibly expose any mobile numbers, so they were obfuscated by base64-encoding all the UserId's and pre-pending the value with a "#" so eventually if an Invite had to be sent mxit_add_buddy() could know what to do.

* The add_buddy prpl callback does not know if the request was initiated by the user selecting "Add Buddy" or clicking on "Invite" from the search-results."
* The "Add Buddy" dialog displays and allows the Username (UserId) to be edited even when it was initiated due to "Invite" being selected from the search-results.
* For 3.0.0, purple_notify_searchresult_column_set_visible() was added so we can hide the UserId field in the search-results.  So this all becomes less of an issue.

I've tried to cause a crash these ways:
 * Add Buddy and enters an invalid UserId that starts with a "#" character.  eg, #, #f00, #..
 * Performs Invite from the search results and then modify part of the UserId.
And purple_base64_decode() just never seems to return NULL - it either returns a string of length 0, or however many bytes it could successfully decode.  In both cases, the MXit server rejects the request since the contact does not exist.

I also noticed this web-page <http://xorl.wordpress.com/2010/11/15/cve-2010-3711-pidgin-multiple-remote-null-pointer-dereferences/>  and the comments at the end.

So I don't think we need to check the return value of purple_base64_decode() for NULL?


Regards,
  Andrew Victor





________________________________________
From: Daniel Atallah [datallah at pidgin.im]
Sent: 02 March 2013 08:08 PM
To: Andrew Victor; Pieter Loubser
Cc: Pidgin Security
Subject: mxit_add_buddy flaw

Guys,

This was reported as a security issue - at initial glance, it doesn't
appear to be something remotely triggerable, but I'm not familiar
enough with the mxit flow to know that for sure (or to know how best
to fix it).

Please credit "Fabian Yamaguchi and Christian Wressnegger of the
University of Goettingen" with finding the issue.

If it does turn out to be a remotely triggerable thing, please
coordinate with the security mailing list to get the fix committed
appropriately.

Thanks,
-D

(5) mxit_add_buddy:
===================

Finally, in mxit_add_buddy, the return-value of purple_base64_decode
is not checked at [1] and passed to mxit_send_invite at [2] where it
is used as an argument to snprintf. This is the only crash we did not
trigger as we ran out of time. I am reporting it nonetheless. Here is
the affected code:

void mxit_add_buddy( PurpleConnection* gc, PurpleBuddy* buddy,
PurpleGroup* group, const char* message )
{
        struct MXitSession*     session = (struct MXitSession*)
gc->proto_data;
        GSList*                         list    = NULL;
        PurpleBuddy*            mxbuddy = NULL;
        unsigned int            i;
        const gchar *           buddy_name = purple_buddy_get_name( buddy );
        const gchar *           buddy_alias = purple_buddy_get_alias(
buddy );
        const gchar *           group_name = purple_group_get_name( group );

        purple_debug_info( MXIT_PLUGIN_ID, "mxit_add_buddy '%s'
(group='%s')\n", buddy_name, group_name );

        list = purple_find_buddies( session->acc, buddy_name );
        if ( g_slist_length( list ) == 1 ) {
                purple_debug_info( MXIT_PLUGIN_ID, "mxit_add_buddy
(scenario 1) (list:%i)\n", g_slist_length( list ) );
                /*
                 * we only send an invite to MXit when the user is not
already inside our
                 * blist.  this is done because purple does an
add_buddy() call when
                 * you accept an invite.  so in that case the user is
already
                 * in our blist and ready to be chatted to.
                 */

                if ( buddy_name[0] == '#' ) {
                        gchar *tmp = (gchar*) purple_base64_decode(
buddy_name + 1, NULL );
                        mxit_send_invite( session, tmp, FALSE,
buddy_alias, group_name, message );
                        g_free( tmp );
                }
        [...]
}