mxit_add_buddy flaw

[Daniel Atallah]

On Tue, Mar 5, 2013 at 5:10 PM, Andrew Victor <Andrew.Victor at mxit.com> wrote:
> 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.

Interesting - with the current glib implementation, it does appear to
be the case that you are guaranteed to get a non-NULL,  NUL-terminated
buffer.

I looked back at the history of the glib implementation and that
wasn't always the case - the behavior was corrected with glib 2.20.

We also didn't always use the glib implementation, although that was
quite a long time ago.

That CVE came out of some crashes, we could have been seeing issues
with people using  glib < 2.20.

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

I guess that's true - there was a CVE for the glib fix, so it's
probably not a horrible assumption that people should be using a new
enough version/patched version.

On the flip side, it's an easy check to add, and actually it is
possible to make purple_base64_decode() return NULL (although, not in
this situation) so perhaps it would be best to just add the check for
consistency?

Thanks for investigating this,
-D