security review and patches for libpurple

[Dan Auerbach]

Hi Ethan,

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:
> Hi,
>
> 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.

Thanks,
Dan

> Ethan