Gadu-Gadu security issues

[Tomasz Wasilczyk]

I've checked all of these issues. Most of them are harmless, but I'm
unsure about importance of vsnprintf related one. All of them are
located in imported libgadu source code. Also, we don't use any code
located in dcc(7).c files.

Proposed patch attached, I can commit it when necessary. When should I
push this to upstream libgadu dev team?

Comments below.

2012/12/18 Daniel Atallah <daniel.atallah at gmail.com>:
> CID 732074
> libpurple/protocols/gg/lib/libgadu.c:528
> gg_debug_session(sess, GG_DEBUG_MISC, "// gg_recv_packet() header
> recv(%d,%p,%d) = %d\n", sess->fd, &h + sess->header_done, sizeof(h) -
> sess->header_done, ret);
>  * Illegal address computation (OVERRUN)At (7): "&h +
> sess->header_done" evaluates to an address that is at byte offset 56
> of an array of 8 bytes.
>
> CID 731944 (This is kind of the same thing as the previous one)
> libpurple/protocols/gg/lib/libgadu.c:528
> * Out-of-bounds access (ARRAY_VS_SINGLETON)At (7): Using "&h" as an
> array. This might corrupt or misinterpret adjacent memory locations.

These two are bugs, but totally harmless - it results with improper
addresses written to debug log.


> CID 732164 (I think this is a false positive)
> libpurple/protocols/gg/lib/libgadu.c:1187
> gg_image_queue_remove(sess, sess->images, 1);
>  * Use after free (USE_AFTER_FREE)At (9): Passing freed pointer
> "sess->images" as an argument to function
> "gg_image_queue_remove(struct gg_session *, struct gg_image_queue *,
> int)".
>  ** It thinks that because gg_image_queue_remove frees the
> "sess->images", the while loop conditional will dereference freed
> memory, but I don't think that's actually possible.

False positive - sess->images will be set to sess->images->next,
before free (events.c:199).


> CID 731948
> libpurple/protocols/gg/lib/dcc7.c:658
> strncpy((char*) s.filename, (char*) tmp->filename, GG_DCC7_FILENAME_LEN);
>  * Buffer not null terminated (BUFFER_SIZE_WARNING)At (15): Calling
> strncpy with a maximum size argument of 255 bytes on destination array
> "s.filename" of size 255 bytes might leave the destination string
> unterminated.

False positive, but not so obvious. I'll change strncpy to memcpy to
make this code look cleaner.


> CID 732124
> libpurple/protocols/gg/lib/dcc7.c:252
> if (bind(fd, (struct sockaddr*) &sin, sizeof(sin)) == -1) {
>  * Uninitialized scalar variable (UNINIT)At (6): Using uninitialized
> value "sin": field "sin"."sin_zero" is uninitialized when calling
> "bind(int, struct sockaddr const *, socklen_t)".
>
> CID 732123 (This is the same as the last one in a different place)
> libpurple/protocols/gg/lib/dcc.c:432
> if (!bind(sock, (struct sockaddr*) &sin, sizeof(sin)))
>
> CID 732122 (This is the same as the last one in a different place)
> libpurple/protocols/gg/lib/common.c:283
> if (connect(sock, (struct sockaddr*) &sin, sizeof(sin)) == -1) {

Fixed. Looks harmless for me.


> CID 732073
> libpurple/protocols/gg/lib/common.c:117
> vsnprintf(buf, size + 1, format, ap);
>  * Out-of-bounds access (OVERRUN)At (11): Overrunning dynamic array
> "buf" by passing it to a function that indexes it with "size + 1" - 1.
>  ** I'm not sure under which conditions this code would be used - it
> looks like GG_CONFIG_HAVE_C99_VSNPRINTF must not be defined

In newest libgadu trunk its totally rewritten. So, I've just bandaged
it by adding +1 to realloc size.


> For example, the following is interesting:
> CID 73200
> libpurple/protocols/gg/lib/dcc7.c:149
> if (tmp->peer_uin == uin && !tmp->state == GG_STATE_WAITING_FOR_ACCEPT)
> * Missing parentheses (CONSTANT_EXPRESSION_RESULT) [select issue]
> * At condition "!tmp->state == 39", the value of "tmp->state" must be
> between 0 and 1.
> * The condition "!tmp->state == 39" cannot be true.

This one is definitely a bug, but we don't use this code, so it wont
harm us. I can just ask libgadu author.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pidgin-security-libgadu.patch
Type: application/octet-stream
Size: 2567 bytes
Desc: not available
URL: <http://pidgin.im/cgi-bin/mailman/private/security/attachments/20121218/fbec086b/attachment-0001.obj>