Re: [PATCH 3/4] tools/xenstored: Avoid buffer overflows while setting up sockets

[Matthew Daley <mattd@bugfuzz.com>]

On Tue, Nov 26, 2013 at 12:07 AM, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> Coverity ID: 1055996 1056002
>
> Use strncpy in preference to strcpy, and use the correct failing path for
> error messages.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> ---
>  tools/xenstore/xenstored_core.c |    9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
> index ccfdaa3..3c13c64 100644
> --- a/tools/xenstore/xenstored_core.c
> +++ b/tools/xenstore/xenstored_core.c
> @@ -1735,13 +1735,12 @@ static void init_sockets(int **psock, int **pro_sock)
>         unlink(xs_daemon_socket_ro());
>
>         addr.sun_family = AF_UNIX;
> -       strcpy(addr.sun_path, xs_daemon_socket());
> +       strncpy(addr.sun_path, xs_daemon_socket(), sizeof(addr.sun_path));

Nitpick: Using strncpy in this manner is unsafe as it does not
guarantee the null-termination of the result in the destination buffer
(which would presumably make barf_perror unhappy). For that reason,
and to avoid truncation, I check the source string length directly
instead in commit f220279c14, for example.

- Matthew

>         if (bind(*sock, (struct sockaddr *)&addr, sizeof(addr)) != 0)
> -               barf_perror("Could not bind socket to %s", xs_daemon_socket());
> -       strcpy(addr.sun_path, xs_daemon_socket_ro());
> +               barf_perror("Could not bind socket to %s", addr.sun_path);
> +       strncpy(addr.sun_path, xs_daemon_socket_ro(), sizeof(addr.sun_path));
>         if (bind(*ro_sock, (struct sockaddr *)&addr, sizeof(addr)) != 0)
> -               barf_perror("Could not bind socket to %s",
> -                           xs_daemon_socket_ro());
> +               barf_perror("Could not bind socket to %s", addr.sun_path);
>         if (chmod(xs_daemon_socket(), 0600) != 0
>             || chmod(xs_daemon_socket_ro(), 0660) != 0)
>                 barf_perror("Could not chmod sockets");
> --
> 1.7.10.4
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel