changeset
yahoo: Fix reading memory locations past the buffer bounds
Sat, 16 Mar 2013 14:02:10 -0400
- author
- Daniel Atallah <datallah@pidgin.im>
- date
- Sat, 16 Mar 2013 14:02:10 -0400
- branch
- release-2.x.y
- changeset 35199
- 4d139ce8f7ec
- parent 33823
- 944ec96bb103
- child 35200
- 932b985540e9
yahoo: Fix reading memory locations past the buffer bounds
* Also improve behavior when parsing malformed P2P packets (it seems like it
might be better to just disconnect when this happens, but I'm not familiar
with why the code as added to try to handle the malformed packets).
* Also, add a comment about the mistaken assumption in p2p packet handling that
reads are being buffered and that partial packets can be subsequently filled
in.
| libpurple/protocols/yahoo/libymsg.c | file | annotate | diff | comparison | revisions |
--- a/libpurple/protocols/yahoo/libymsg.c Sun Mar 10 23:31:01 2013 -0400 +++ b/libpurple/protocols/yahoo/libymsg.c Sat Mar 16 14:02:10 2013 -0400 @@ -2536,7 +2536,7 @@ int pos = 0; int pktlen; struct yahoo_packet *pkt; - guchar *start = NULL; + guchar *start; struct yahoo_p2p_data *p2p_data; YahooData *yd; @@ -2558,19 +2558,29 @@ return; } + /* TODO: It looks like there's a bug here (and above) where an incorrect + * assumtion is being made that the buffer will be added to when this + * is next called, but that's not really the case! */ if(len < YAHOO_PACKET_HDRLEN) return; - if(strncmp((char *)buf, "YMSG", MIN(4, len)) != 0) { + if(strncmp((char *)buf, "YMSG", 4) != 0) { /* Not a YMSG packet */ + purple_debug_warning("yahoo", "p2p: Got something other than YMSG packet\n"); + + start = (guchar *) g_strstr_len((char *) buf + 1, len - 1 ,"YMSG"); + if (start == NULL) { + /* remove from p2p connection lists, also calls yahoo_p2p_disconnect_destroy_data */ + if (g_hash_table_lookup(yd->peers, p2p_data->host_username)) + g_hash_table_remove(yd->peers, p2p_data->host_username); + else + yahoo_p2p_disconnect_destroy_data(data); + return; + } purple_debug_warning("yahoo","p2p: Got something other than YMSG packet\n"); - start = memchr(buf + 1, 'Y', len - 1); - if (start == NULL) - return; - - g_memmove(buf, start, len - (start - buf)); - len -= start - buf; + len -= (start - buf); + g_memmove(buf, start, len); } pos += 4; /* YMSG */ @@ -2578,7 +2588,17 @@ pos += 2; pktlen = yahoo_get16(buf + pos); pos += 2; - purple_debug_misc("yahoo", "p2p: %d bytes to read\n", len); + if (len < (YAHOO_PACKET_HDRLEN + pktlen)) { + purple_debug_error("yahoo", "p2p: packet length(%d) > buffer length(%d)\n", + pktlen, (len - pos)); + /* remove from p2p connection lists, also calls yahoo_p2p_disconnect_destroy_data */ + if (g_hash_table_lookup(yd->peers, p2p_data->host_username)) + g_hash_table_remove(yd->peers, p2p_data->host_username); + else + yahoo_p2p_disconnect_destroy_data(data); + return; + } else + purple_debug_misc("yahoo", "p2p: %d bytes to read\n", pktlen); pkt = yahoo_packet_new(0, 0, 0); pkt->service = yahoo_get16(buf + pos); pos += 2;
