security review and patches for libpurple

[Ethan Blanton]

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.

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


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.

Ethan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 482 bytes
Desc: Digital signature
URL: <http://pidgin.im/cgi-bin/mailman/private/security/attachments/20110817/0fa9ec0c/attachment-0001.pgp>