security review and patches for libpurple
dtauerbach at eff.org
Sun Jul 17 17:39:47 EDT 2011
Thanks for going through our patches, and for your diligence about the
changes being made!
On 07/16/2011 09:11 PM, Ethan Blanton wrote:
> A few comments on the supplied patches.
> If I am reading the documentation correctly, some of these uses of
> g_strlcpy() are incorrect. For example:
> vals[i] = ckalloc(strlen(*strs[i]) + 1);
> -strcpy(vals[i], *strs[i]);
> +g_strlcpy(vals[i], *strs[i], strlen(*strs[i]));
> From the g_strlcpy documentation:
> [...] dest_size is the buffer size, not the number of chars to copy.
> At most dest_size - 1 characters will be copied. [...]
> The result of this is that vals[i] will be truncated one byte too early.
> Many uses of strlcpy seem to be correct in these patches, so I assume
> this is not a conceptual error, but a simple mistake. However, since
> this does affect correctness, I wanted to point it out to make sure.
Yes, this is an error. Sorry about that!
> 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.
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.
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.
More information about the security