[Secure-testing-team] Bug#487222: tmsnc: remote stack based buffer overflow in UBX parsing code

[Nico Golde]

Package: tmsnc
Severity: grave
Tags: security
Justification: user security hole

Hi,
quoting http://msnpiki.msnfanatic.com/index.php/Command:UBX:
"UBX is the sister command to UUX. UUX is used to set your personal·
message, UBX is sent by the server to all principles to inform them of·
the change (where B means Buddy). The format is similar to UUX; they are·
payload commands where the first parameter is the passport address of·
the contact who has just changed their personal message or currently·
playing song, and the second parameter is the length of the payload.

Syntax:
 >>> UBX passport at hotmail.com xxx\r\n
 <Data><PSM>My Personal Message</PSM><CurrentMedia></CurrentMedia></Data>

as far as I can see this is sent by the original msn client but clients·
like pidgin and tmsnc do not support sending this information but·
receiving it.

Let's have a look at the code for parsing such a message in tmsnc...
>From core_net.c:
    727 int
    728 MSN_server_handle(session, message, message_len)
    729      MSN_session *session;
    730      char *message;
    731      int message_len;
    732 {
    733     time_t tm;
    734     char buf[512], md_hex[48];
    ...
    748     while (getline(buf, sizeof(buf) - 1, session->sd) > 0) {
    ...
    833         } else if (strncmp(buf, "UBX", 3) == 0) {
    834             /*
    835              * we read the payload of this command·
    836              */
    837             /*
    838              * but do not do anything with it······
    839              */
    840             if ((ptr[1] = (char *)split(buf, ' ', 1)) == NULL ||        //by gfhuang
    841                 (ptr[0] = (char *)split(buf, ' ', 2)) == NULL) {
    842                 strncpy(message, "Couldn't parse UBX", message_len - 1);
    843                 return -1;
    844             }
    845             i = atoi(ptr[0]);
    846             free(ptr[0]);
    847·
    848             if (read(session->sd, buf, i) != i) {
    849                 strncpy(message, "Couldn't read UBX payload",
    850                         message_len - 1);
    851                 return -1;
    852             }
    853             // parsing PSM, by gfhuang
    854             if(0 == i) buf[0] = 0;      //important, by gfhuang, when i=0, buf is untouched!

In line 734 the message buffer is declared to store 512 bytes of data.
Line 748 reads a command line coming from a buddy contact.
Line 833 and the following are used if the message buffer contains an UBX message like:
UBX passport at hotmail.com xxx\r\n where xxx is the length of the UBX payload.

Here is the actual bug. If the first 3 bytes of the buffer match to UBX and the string
contains two spaces which are passed to ptr[1] and ptr[0] this is a valid UBX message.

The split function comes from core_misc.c and does basically the same like the strchr
function, returning a pointer to the first occurance of the string passed as second parameter.
So after the call in line 841 ptr[0] will point to the message length.
This value is then converted to an integer using atoi in line 845 and passed to read in line 848.
This will then read the UBX payload from the MSN "packet" through the session socket.

So if the UBX payload length is declared to be more than sizeof(buffer) or the payload is longer
than sizeof(buffer) this results in a stack-based buffer overflow and possibly in arbitrary code
execution.

The code also uses atoi quite a lot without checking for negative values resulting in integer
conversion issues but I guess that those values are correct is ensured by the MSN server itself.

This looks related to #487046.
I already contacted the upstream author because of this.

Kind regards
Nico