Denial of Service Vulnerabilities

[Thijs Alkemade]

On 25 feb. 2013, at 15:14, Fabian Yamaguchi wrote:

> (2) jabber_vcard_parse_avatar/jabber_vcard_parse:
> =================================================
> 
> A missing check when parsing vcards supplied by other users allows
> attackers to trigger a NULL-pointer dereference. Since vcards are
> automatically requested from users appearing online, this allows
> attackers to crash Pidgin simply by logging in. Here is the affected
> code:
> 
> static void
> jabber_vcard_parse_avatar(JabberStream *js, const char *from,
>                          JabberIqType type, const char *id,
>                          xmlnode *packet, gpointer blah)
> {
>        JabberBuddy *jb = NULL;
>        xmlnode *vcard, *photo, *binval, *fn, *nick;
>        char *text;
> 
>        if(!from)
>                return;
> 
>        jb = jabber_buddy_find(js, from, TRUE);
> 
>        js->pending_avatar_requests =
> g_slist_remove(js->pending_avatar_requests, jb);
> 
>        if((vcard = xmlnode_get_child(packet, "vCard")) ||
>                        (vcard =
> xmlnode_get_child_with_namespace(packet, "query", "vcard-temp"))) {
>                /* The logic here regarding the nickname and full name
> is copied from
>                 * buddy.c:jabber_vcard_parse. */
>                gchar *nickname = NULL;
>                if ((fn = xmlnode_get_child(vcard, "FN")))
>                        nickname = xmlnode_get_data(fn);
> 
>                if ((nick = xmlnode_get_child(vcard, "NICKNAME"))) {
>                        char *tmp = xmlnode_get_data(nick);
>                        char *bare_jid = jabber_get_bare_jid(from); [1]
>                        if (tmp && strstr(bare_jid, tmp) == NULL) { [2]
> 		[...]
> }
> 
> The return value of jabber_get_bare_jid, which can be NULL, is not
> checked at [1]. This value is then passed to strstr causing the NULL
> pointer to be dereferenced at [2]. The crash can be triggered as
> follows:
> 
> (1) The attacker logs in
> (2) The user requests the attacker's vcard using a message such as the
> following:
> 
> <iq type='get' id='purple5f8085f8' to='attacker at jabber.org'>
>                              <vCard xmlns='vcard-temp'/></iq>
> (3) The attacker replies with a malformed message containing an
>    invalid "from" attribute of the "iq"-tag, for example:
> 
> <iq type="result" id="purple5f8085f8" from="@"
> to="user at jabber.org/dbe143fe">
> <vCard xmlns="vcard-temp"><NICKNAME>X</NICKNAME></vCard></iq>
> 
> 
> The same check is missing in buddy.c: jabber_vcard_parse, however, we
> did not test whether it can be triggered as well.

This seems to point out a much larger issue: iq replies are only looked up by
their id. If I send:

<iq type='get' id='purple12345' to='paul at darkrain42.org'>(some query)</iq>

and an attacker manages to guess "purple12345" (which is not very hard due to
their numerically increasing nature), they can try to reply before Paul:

<iq type='result' id='purple12345' to='thijs at adium.im' from='hacker at evil.com'>(malicious data)</iq>

In any case, libpurple will remove the callback for 'purple12345', therefore 
Paul's real reply will be ignored.

I started out making a list of callbacks that do not check the 'from' on the
iq (the 'const char *from' argument), but it came down to: lots of them. I
think less than half of the 20 or so iq callbacks that exist currently
properly check if the reply matches who we expect it to come from. That would
mean that all others interpret the malicious data as if coming from Paul. This
could for instance be a custom emoticon, an IBB invitation reply or Paul's
last activity time. The most important ones (obtaining one's roster,
requesting avatars) are not vulnerable to this, but can still cause the real
reply to not be handled.

According to darkrain, the only situation where the "from" on a reply would
not match the "to" on a get legitimately would be situations where sending an
iq to your own JID (for example, retrieving rosters).

I think the code should be changed such that:

1) Registering the iq-callback stores the "to" the iq is sent too.
2) The stored JID can be replaced with another JID, for the edge cases
mentioned above (rosters).
3) When receiving an iq, not only check if the id occurs in the callbacks
table, but also check if it matches the expected JID.

I'm willing to work on a patch for this, but that will probably not be this
week.

Thijs