[libvirt] [PATCH] Fix latent buffer overflow in qemudOpenMonitorUnix.

[Chris Lalancette]

Chris Lalancette wrote:
>> Clearly strncpy() is just unfit for purpose, and we should plan to banish
>> it in favour of a virStrncpy impl that guarnetees to add the trailing \0.
> 
> Yeah, true.  Despite the ACKs from you and markmc, I decided not to commit this
> yet.  It's quite a theoretical bug (you'd have to have
> /var/very/long/non-standard/paths/to/libvirt), so I don't think it's absolutely
> critical right now.  I'll convert the rest of the users, and possible come up
> with a make syntax-check rule to prevent against future use.

Sigh.  Unfortunately, this is a difficult problem.  I did some reading, and
found that besides having the NULL termination problem, strncpy() is also very
slow, so should be avoided.  I then looked at doing an implementation of
strlcpy(), but while it would prevent the buffer overflow, it can truncate the
resulting string.  This could lead to difficult detect errors later on.  Note
that both my original proposed patch for this problem plus many of current
in-tree users of strncpy() suffer from this problem as well.

Therefore, I think the way to go is probably to have a safe version of strcpy()
which checks that the length of destination is sufficient to hold all of the
source plus the \0.  If not, it should return an error, and then the caller can
decide what to do.  Something like this:

/**
 * virStrcpy
 *
 * A safe version of strcpy.  The last parameter is the number of bytes
 * available in the destination string, *not* the number of bytes you want
 * to copy.  If the destination is not large enough to hold all of the
 * src string, NULL is returned and no data is copied.  If the destination
 * is large enough to hold all of the src, then the string is copied and
 * a pointer to the destination string is returned.
 */
char *
virStrcpy(char *dest, const char *src, size_t destbytes)
{
    int len;

    len = strlen(src) + 1;
    if (len > destbytes)
        return NULL;

    return memcpy(dest, src, len);
}

And usage would be:

char foo[5];
virStrcpy(foo, "hello", sizeof(foo));
/* returns an error, foo is not large enough for hello\0 */

char bar[6];
virStrcpy(bar, "hello", sizeof(foo));
/* succeeds, foo is large enough for hello\0 */

A brief look through the current usage of strncpy() in libvirt shows that this
would be a drop in replacement for most of the current users of strncpy(), and
be safer as well.

What do you think?

-- 
Chris Lalancette