security review and patches for libpurple

[Dan Auerbach]

Thanks for taking a look, Mark!

On 08/16/2011 12:24 AM, Mark Doliner wrote:
> Ethan I looked through your commits and they look good to me.
>
> Comments about the remaining patches:
> ** imgstore-filename.diff **
> This sanitizes filenames to be alphanumeric.  It seems like this would
> make the filename much less useful to non-English speakers.  I'm not
> convinced this is a good solution to this problem.  If we really think
> this is a good idea then we should at least change the regexp to not
> penalize international users so heavily.
Good point; a better regex is certainly preferable for this reason. I 
still think sanitizing remains a good thing to do, given the many places 
non-sanitized filenames can lead to exploits (e.g. MIME injection)

> ** test_util-addtest.diff **
> This adds a unit test.  I haven't seen anyone mention this one
> recently.  It looks ok to me so I committed it.
>
> ** util-strdup.diff **
> This adds a size check before allocating memory.  I'm don't like this
> change.  If we're concerned about g_malloc() crashing if we try to
> allocate too much memory then we should use g_try_malloc().  If we
> want to avoid allocating a massive amount of memory then maybe we
> should use setrlimit() to set the max amount of heap that our process
> can use (possibly in conjunction with g_try_malloc()).
>
> Then there's the question of what do you set the limit to (and your
> suggested change has this same problem).  No matter what size you
> choose you're setting an artificial limit for yourself, which always
> has its pros and cons.  And setting a limit for a single allocation
> doesn't buy you much--what if someone does something that causes you
> to make hundreds of allocations that are each just smaller than the
> limit?
>
> We strdup and malloc memory all over the place.  Would you argue that
> we should do this kind of check every time we allocate memory?  I
> think this would quickly become cumbersome and error-prone.
>
> This check is attempting to anticipate some sort of future remote
> Denial of Service vulnerability, but I don't think it's worth it.
Not only this, but integer overflows and heap spray are both issues that 
bounding allocations helps to mitigate.

>    All
> things considered I think this type of change makes users worse off.
> The code becomes more complex and error prone, which potentially
> INCREASES the chance of remotely-exploitable bugs.
>
> If we're aware of a place where this function is used and there is a
> potential to allocate a large chunk of memory, then we should handle
> that in a more targeted change.
My inclination is to say that it wouldn't be too cumbersome to set a 
(relatively high) arbitrary limit and that doing so might offer some 
protection, at least against overflows. I'm not sure what best practices 
are with respect to bounding allocations in an untargeted way (e.g. a 
library), but if there are good standard ways of doing this (such as 
setrlimit), then it seems to me that it's worth looking at another code 
base that does so just to get a sense of how well it works and if there 
are any problems. But I'll give it some thought, and poke around in 
different places to see what I find with respect to this issue.
> ** yahoo_packet-strcpy.diff **
> The code that constructs outgoing packets currently makes two passes
> through the outgoing data--one to determine the buffer length needed,
> and a second to write data to the buffer.  If we wanted to use
> g_strlcpy() here then we would need to start passing around the buffer
> end offset or maybe the buffer, current buffer position and buffer
> length.  Maybe using a more advanced data structure.
>
> ** zephyr-protectoverflow.diff **
> This adds a size check before allocating memory.  This is similar to
> the change to util.c, but this one actually IS targeted, and so it
> might be a good idea--I haven't looked at it in detail.  (Calling
> exit() when read fails is definitely not acceptable.)
>
> --Mark
>