Buffer overflow in jabber prpl

[Paul Aurich]

Thanks for writing this up!  The patch looks correct to me.

On 10/13/2014 01:16 PM, Thijs Alkemade wrote:
> Hello,
> 
> While investigating [1], darkrain and I discovered a off-by-one error in
> jabber_idn_validate that may lead to libpurple reading memory outside of the
> bounds of an array.
> 
> The problem is caused by [2]. The "+ 1" here is wrong and causes domain_len to
> be one too few. This can lead to an incomplete UTF-8 codepoint for
> internationalized TLDs such as .рф, which, for unclear reasons, causes
> stringprep_nameprep to skip the terminating NULL byte by interpreting it as
> part of the codepoint. By doing this at the end of the idn_buffer, it is
> possible to read more than sizeof(idn_buffer) bytes, despite claims from the
> stringprep documentation that it will not read bytes past the maxlen.

I'm thinking this through as I type (please bear with me):

idn_buffer is statically-allocated; parts of the JID are copied into
it.  The source string itself should always be null-terminated, so even
with the off-by-one calculations, strncpy should never copy past the end
of the original string.

So it shouldn't ever over-read from the heap-allocated source string
(right?)

As demonstrated in the ticket, or based on your analysis, it is possible
either to read data previously-normalized, or potentially beyond the
static buffer (to whatever has been allocated there).

~Paul

> A malicious server can likely abuse this to cause libpurple to read the data
> after idn_buffer. Any scenario where libpurple parses a JID and then sends a
> response to the JID could be used to return the extra data to the server.
> Whether only the user's own server could I don't know.
> 
> There are some other issues in jabber_idn_validate too:
> 
> * Line 84 is missing a "+ 1", causing the '/' to be part of the resource and
>   the resource also missing the last character. This is likely also
>   exploitable in the same way, probably even easier as it's much easier to
>   construct weird resources than weird but valid domain names.
> 
> * The 'then' case of the if-statement on line 116 never assigns the domain to
>   jid->domain, likely causing JIDs with raw IPv6 addresses to cause problems.
> 
> A patch for all of these problems is attached at the bottom of this email.
> 
> Regards,
> Thijs Alkemade
> 
> [1] = https://developer.pidgin.im/ticket/16310
> [2] = https://hg.pidgin.im/pidgin/main/file/1a8a6a18e76e/libpurple/protocols/jabber/jutil.c#l87
> 
> 
> 
> diff -r d8d96a636413 libpurple/protocols/jabber/jutil.c
> --- a/libpurple/protocols/jabber/jutil.c        Fri Feb 21 11:08:18 2014 +0100
> +++ b/libpurple/protocols/jabber/jutil.c        Mon Oct 13 22:13:07 2014 +0200
> @@ -81,10 +81,10 @@
> 
>                 if (slash) {
>                         domain_len = slash - str;
> -                       resource = slash;
> +                       resource = slash + 1;
>                         resource_len = null - (slash + 1);
>                 } else {
> -                       domain_len = null - (str + 1);
> +                       domain_len = null - str;
>                 }
>         }
> 
> @@ -126,6 +126,8 @@
>                         jid = NULL;
>                         goto out;
>                 }
> +
> +               jid->domain = g_strndup(domain, domain_len);
>         } else {
>                 /* Apply nameprep */
>                 if (stringprep_nameprep(idn_buffer, sizeof(idn_buffer)) != STRINGPREP_OK) {
> 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <https://pidgin.im/cgi-bin/mailman/private/security/attachments/20141013/dfeaee99/attachment.sig>