security review and patches for libpurple

[Dan Auerbach]

[+chris's personal email, as he no longer works at EFF]

On 08/11/2011 05:33 PM, Dan Auerbach wrote:
> 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*.

Yes, of course I agree this is wrong in general. However, I'm not sure I 
agree it is WORSE than strcpy---they both seem equally broken to me. 
There's also a possible philosophical difference that we might have: in 
my view it's worth considering strcpy "deprecated" entirely and 
insisting on changing every occurrence, unless doing so will have an 
explicit negative impact. In other words, the burden of proof should be 
to prove that strcpy is actually better or necessary in a particular 
instance, not merely that there are no security risks with a given usage 
of strcpy, or that replacing a usage yields no security improvement. If 
we are committed to deprecating strcpy, we either have to live with an 
equally bad usage of strlcpy, or rewrite the code that necessitates 
strcpy. This is probably not worth arguing about too much, and I try to 
avoid being dogmatic when it comes to this sort of thing. Of course this 
comment applies not only here but to all the relevant patches below.

As for this patch, it seems there are potentially larger issues relating 
to the unusual semantics of the PurpleStringref struct and the 
non-enforceable requirement that it is never accessed directly. As I've 
said, the fact that strcpy isn't improved upon at all by strlcpy in this 
instance strikes me as a reason to rewrite this code. If we are canning 
this patch, can we add a bug to "replace this whole thing with a 
GCache", which is a TODO that appears at the top of stringref.h?

>> * 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.

Will respond to this in the relevant later email.
>>
>>
>> * 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.

Will respond to the latest email about this patch.

>>
>>
>> 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
>