security review and patches for libpurple

[Ethan Blanton]

Hi,

A few comments on the supplied patches.

If I am reading the documentation correctly, some of these uses of
g_strlcpy() are incorrect.  For example:

 vals[i] = ckalloc(strlen(*strs[i]) + 1);
-strcpy(vals[i], *strs[i]);
+g_strlcpy(vals[i], *strs[i], strlen(*strs[i]));

From the g_strlcpy documentation:

    [...] dest_size is the buffer size, not the number of chars to copy.

    At most dest_size - 1 characters will be copied.  [...]

The result of this is that vals[i] will be truncated one byte too early.

Many uses of strlcpy seem to be correct in these patches, so I assume
this is not a conceptual error, but a simple mistake.  However, since
this does affect correctness, I wanted to point it out to make sure.

I also note a number of changes of the form:

 buffer = malloc(some expression);
 [...]
-strcpy(buffer, source);
+g_strlcpy(buffer, source, some expression);

This is not substantially superior to the preexisting strcpy.  It may
silence some automated tools, but it introduces an additional way in
which the code in question can be wrong.  Specifically, the expression
which is duplicated in the malloc and strlcpy can diverge with
disastrous consequences.  Assuming the expression is correct, the
strcpy implementation is more robust than the strlcpy implementation.
If the expression is incorrect, all bets are off in both
implementations without more contextual information.

Ethan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 482 bytes
Desc: Digital signature
URL: <http://pidgin.im/cgi-bin/mailman/private/security/attachments/20110717/72faf0d9/attachment.pgp>