security review and patches for libpurple

[Dan Auerbach]

Hi Ethan,

Thanks for your diligence in going through these patches. I don't 
disagree with you about the limitations of static analysis tools, in 
their present incarnation, at least. And I also agree that this round of 
patches needed work before being ready to apply, and appreciate your 
taking the time to work on it. I started drafting a more detailed 
response, but am taking off right now for the weekend (and won't have 
access to email), so that response will have to wait until next week. 
But I just wanted to acknowledge that I got this, and quickly say thanks.

Dan

On 08/11/2011 08:19 AM, Ethan Blanton wrote:
> Ethan Blanton spake unto us the following wisdom:
>> With that in mind, I'd like to ask again if there are any objections
>> to my committing these patches to ipp without embargo or a coordinated
>> release.  If not, I will land them some time tomorrow.  If anyone even
>> simply thinks we should wait a few days or get additional input before
>> landing them, that's fine, too.
> OK.  I apologize for the embarrassing delay on this, but I have just
> landed thirteen EFF patches on im.pidgin.pidgin.  They are:
>
> ZAsynclocate-strcpy.diff
> ZInit-strcpy.diff
> ZRetSubs-strcpy.diff
> Zinternal-strcpy.diff
> cipher-strcpy.diff
> jabber-strcpy.diff
> libc_interface-strcpy.diff
> msn-strcpy.diff
> posixuname-strcpy.diff
> proxy-strcpy.diff
> tcl_ref-strcpy.diff
> tcl_signals-strcpy.diff
> zephyr-strcpy.diff
>
> Many of these have corrections in the form of more effective bounds
> checks, fenceposts, and DRY memos.  I don't think there's anything
> controversial here, but please have a look.  The bank of revisions is
> (I believe) 28e56bcf through 269c6e29.
>
> I have rejected the following patches for rejection, further review,
> and/or correction:
>
> * stringref_strcpy.diff
>
>    This patch is just plain wrong.  It neglects to account for the one
>    byte of the string being copied which resides in the stringref
>    struct.  This is probably something the automated tool will forever
>    be too stupid to understand.  The strlcpy() is entirely unnecessary
>    here, because of the structure of the code -- and if it's messed up,
>    other stuff is more messed up, so it's not fixing anything.
>
>    Also, in general, this is NEVER safe:
>
>      g_strlcpy(dest, src, strlen(src));
>
>    That's even worse than strcpy(dest, src) with no check, because it
>    checks *the wrong thing*.
> * 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.
>
> * auth_cyrus.c
>
>    This is bogus like the first patch; len is strlen(pw).  If we can
>    determine the size of sasl_secret_t.data, we need to use that.  I
>    haven't looked into this yet.
>
> I have not yet processed the following patches at all:
>
> filectl-sprintf.diff
> imgstore-filename.diff
> log-sscanf.diff
> log-strcpy.diff
> oscar-strcpy.diff
> savedstatuses-strcpy.diff
> stun-strcpy.diff
> test_util-addtest.diff
> util-strdup.diff
> yahoo_filexfer-strcpy.diff
> yahoo_packet-strcpy.diff
>
> Sorry again for the delay.  I hope to get to those remaining patches
> Soon, but I can't make any promises.  I'd like to see them in 2.10.0.
>
> Ethan