security review and patches for libpurple

[Dan Auerbach]

On 08/11/2011 09:25 PM, Paul Aurich wrote:
> And Ethan Blanton spoke on 08/11/2011 08:19 AM, saying:
>> * zephyr-protectoverflow.diff
>>
>>    This may be OK, but I don't like it.  At the very least, it needs to
>>    reference G_MAXULONG or something of the sort.
>     * Intermingling g_* and libc allocation functions is a no-no (it breaks
> on Win32, where glib doesn't use the underlying allocator -- annoying, but
> such is life)
>     * Exiting when a read fails seems icky.  Then again, this is Zephyr and
> it's right after a select()
>
> I'm assuming the special value (2^27) comes from being "big enough that
> nobody would reasonably expect to allocate/read that much memory".  Is that
> correct?  I'm a bit uneasy about this (and assuming that's what Ethan was
> uneasy about), but I also think it's accurate that nobody would try to
> legitimately need that big of a buffer in either of these functions.
> Famous last words.

That's correct. The general philosophy is to bound all of these 
allocations---I'm certainly not married to 2^27, nor to keeping this 
constant here, nor to exiting when a read fails. But I think the idea of 
bounding allocations is a good one; I'll respond to Mark's email about 
this, and the patch below.


>> * util-strdup.diff
>>
>>    This has the same problem as the Zephyr overflow protection patch.
> If the string is large enough, it seems to me like 'i' would wrap around to
> 0 (I'd need to reason a bit more about this to convince myself the app
> wouldn't die first) and spin in a loop.
>
> I'm also trying to think of a way to avoid the extra strlen() -- does
> checking if destsize<  i work (destsize starts at 1)?
>
> ~Paul
>