security review and patches for libpurple

[Dan Auerbach]

On 08/17/2011 04:39 PM, Ethan Blanton wrote:
> Dan Auerbach spake unto us the following wisdom:
>> [+chris's personal email, as he no longer works at EFF]
> Thanks, I removed him before because his EFF address bounced.
>
>>>>    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.
> I disagree strongly with this.  Anyone looking at the strcpy knows
> that it's not checked.  Anyone glancing at the strlcpy and not looking
> closely will assume that it _is_ safe.  This is exacerbated further
> when the bounds argument is something computed elsewhere in the
> function.
>
> Consider also that your static checker will no longer flag this as a
> potential problem.
>
>> 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.
> It is not "no security improvement" which I object to, but "insertion
> of broken code which was not as broken before".  In each of these
> cases, there is a correct, bounds-checked copy which is not more
> broken.  The effort of finding this fix is not so large that we should
> make cosmetic changes which hide the real problems.

I agree completely with the idea that it's worth making real fixes, not 
just cosmetic changes. When I get a chance, I'll aim to look more 
carefully at the rejected patches to see if I can offer more intelligent 
suggestions.

>> 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.
> I agree that it's not worth arguing about, particularly because in
> this instance I am the gatekeeper of the code in question.  ;-)
>
> As to whether deprecating strcpy entirely or not is a laudable goal, I
> take no position.  I absolutely agree that properly bounds checked
> copies are better in all cases.  In the ideal world, all copies are
> checked, and the compiler removes checks via static analysis where
> they are truly unnecessary.
>
>> 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?
> I am entirely agreement that stringrefs are a bit odd and dangerous if
> the casual user does not pay attention to their semantics.  Stringrefs
> were introduced to solve a specific problem for which they are
> extremely well-suited.  You will find that they are used in exactly
> one place in Pidgin, and that is in legacy code which is no longer
> extended.
>
> The PurpleStringref structure can not be accessed directly in any
> event, without doing Bad Things, because its definition is not public.
> The string value it contains is accessible only via
> purple_stringref_value(), which returns a const pointer, and thus the
> compiler will enforce its immutability.
>
> PurpleStringref is an example of code which could be carefully
> checked, then grandfathered.  Bounds checking its copy is fine, it
> doesn't hurt anything and adds very little overhead; however,
> Stringrefs are written exactly once, at creation time, and the
> allocation and copy are tied directly by size in the code.

I looked a bit at how it was used after sending off the last email, and 
this makes sense. Still, if grandfathering is an option and GCache 
covers the relevant use case, it seems like a good idea to do so in 
order to simplify the code.

>
> So, the bottom line on these changes is:
>
> * strlcpy invocations which do not check a bound which is actually
>    valid will not be accepted.
> * Copies which *do* check a bound which is actually valid, even if the
>    bound cannot be violated for some other reason, are OK with me,
>    provided they don't pose a performance problem.  (The probability of
>    a performance problem in copying strings in Pidgin is near nil.)
>
>
> I was just looking at the stringref code, there is an easy fix which
> makes correct bounds checks, and I will apply it.

Great! Thanks again.

> Ethan