Buffer overflow in jabber prpl

[Thijs Alkemade]

On 14 okt. 2014, at 03:37, Paul Aurich <paul at darkrain42.org> wrote:

> 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

Correct, it would overread into whatever static data happens to be placed
right after idn_buffer. I'm attaching a small test case that demonstrates the
problem. All function calls are the same as in libpurple, but the buffer is
only 64 bytes.

Executing it results in:

$ ./a.out
(65) bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbрф
(59) bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbррaaa

The "aaa" are copied from another static array that happens to be right after
the buffer.

(I'm also tempted to call it a bug in libidn. The pre-conditions of
stringprep() aren't satisfied (the input is not an UTF-8 encoded zero-
terminated string), but the claim that "This function will not read or write
to characters outside that size." is simply wrong.)

[1] https://www.gnu.org/software/libidn/reference/libidn-stringprep.html#stringprep

Regards,
Thijs

-------------- next part --------------
A non-text attachment was scrubbed...
Name: idn.c
Type: application/octet-stream
Size: 899 bytes
Desc: not available
URL: <https://pidgin.im/cgi-bin/mailman/private/security/attachments/20141014/2804cf3f/attachment.obj>
-------------- next part --------------


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 841 bytes
Desc: Message signed with OpenPGP using GPGMail
URL: <https://pidgin.im/cgi-bin/mailman/private/security/attachments/20141014/2804cf3f/attachment.sig>