security review and patches for libpurple

[Ethan Blanton]

Dan Auerbach spake unto us the following wisdom:
> >I also note a number of changes of the form:
> >
> >  buffer = malloc(some expression);
> >  [...]
> >-strcpy(buffer, source);
> >+g_strlcpy(buffer, source, some expression);
> >
> >This is not substantially superior to the preexisting strcpy.  It may
> >silence some automated tools, but it introduces an additional way in
> >which the code in question can be wrong.  Specifically, the expression
> >which is duplicated in the malloc and strlcpy can diverge with
> >disastrous consequences.  Assuming the expression is correct, the
> >strcpy implementation is more robust than the strlcpy implementation.
> >If the expression is incorrect, all bets are off in both
> >implementations without more contextual information.
> 
> I understand the concern about the expressions diverging. However, I
> think the benefits of following the standard of using strlcpy outweigh
> these concerns. There are two points that I would raise in support:
>
> 1. There are many other ways in which the code might change in which
>    leaving strcpy would actually be worse than strlcpy. For example,
>    code might be copied and pasted such that malloc was no longer
>    invoked before strcpy. So I'd say analysis above that strcpy is
>    more robust is true modulo certain assumptions about how the code
>    may change with time, but that without those assumptions it's not
>    as clear as to which implementation is safer in the long run.

Sure.  I'm not sure, on a whole, the strlcpy is really safer, though,
without more work (see below).

> 2. More generally, there may be situations in which the semantics of
>    an "unsafe" function such as strcpy are more desirable for a number
>    of reasons. But I think it still is good to exercise discipline and
>    stick to safe functions, even if they have locally undesirable
>    effects, for the long-term security and stylistic consistency of
>    the code base.

Maybe.  That depends a *lot* on context.  This is a real disadvantage
of static analysis tools -- the person reading the output has to take
a careful look at what the code actually does.  There are other errors
in the patches which I believe are the result of static analysis
limitations (such as a +1 added to a malloc where it doesn't belong).
I bring this up as a general "gotcha" to keep an eye on, not as
specific criticism.

> That said, there are other ways to change the code that use safe
> functions and also avoid or minimize the concern above, and looking
> into this is certainly worthwhile. For example, there could be a
> wrapper that handles the malloc call and saves the mallocd size
> "some expression" to another variable, used later in strlcpy. Even
> the small step of setting "var = some expression", and then passing
> in var both to malloc and to strlcpy helps to mitigate the concern,
> and should be implemented. That we failed to implement these easy
> mitigation strategies is an error, and in the future we will try to
> be more diligent in making the patches as good as possible.

In quite a few places, I have added a variable to store the results of
the expression.  Things like:

+len = strlen(str) + 1;
-buf = malloc(strlen(str) + 1);
-g_strlcpy(buf, str, strlen(str) + 1);
+buf = malloc(len);
+g_strlcpy(buf, str, len);

In general, this is a much superior solution.  This was actually the
point of my note, not that strlcpy is *inferior* to strcpy.  Bounds
checks with on-the-spot calculated bounds must be used *very*
carefully, whereas bounds which are tied by the compiler to other
manipulations and allocations are much safer.

The real cake is, of course, sizeof() where applicable.  :-)

I have corrected these patterns in the patches where they occurred,
and I have corrected fencepost errors where they occurred.  I've
applied about half of the patches with or without some modification.
Some of the patches have more serious errors or make assumptions I
need to validate before they can be applied.  I hope to send a list of
those patches and my specific concerns "soon".

Thanks for your continued input,
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/20110717/50066038/attachment.pgp>