On 25/05/2017 17:53, Daniel P. Berrange wrote: > The 'struct sockaddr_un' only allows 108 bytes for the socket > path. > > If the user supplies a path, QEMU uses snprintf() to silently > truncate it when too long. This is undesirable because the user > will then be unable to connect to the path they asked for. > > If the user doesn't supply a path, QEMU builds one based on > TMPDIR, but if that leads to an overlong path, it mistakenly > uses error_setg_errno() with a stale errno value, because > snprintf() does not set errno on truncation. > > In solving this the code needed some refactoring to ensure we > don't pass 'un.sun_path' directly to any APIs which expect > NUL-terminated strings, because the path is not required to > be terminated. > > Signed-off-by: Daniel P. Berrange <berra...@redhat.com> > --- > > Changed in v4: > > - Do length check before any mkstemp to avoid leaving long > tmpfile on disk on error > > Changed in v3: > > - Also fix unix_connect_saddr error reporting > - Avoid calling unlink with path that might not be nul terminated > - Ensure the TMPDIR derived path can fill up sun_path space. > - Don't update saddr->path until we have succesfully listened > - Unify error reporting across both explicit path & TMPDIR path > branches > > util/qemu-sockets.c | 68 > ++++++++++++++++++++++++++++++++++++----------------- > 1 file changed, 46 insertions(+), 22 deletions(-) > > diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c > index d8183f7..dfaf4e1 100644 > --- a/util/qemu-sockets.c > +++ b/util/qemu-sockets.c > @@ -845,6 +845,8 @@ static int unix_listen_saddr(UnixSocketAddress *saddr, > { > struct sockaddr_un un; > int sock, fd; > + char *pathbuf = NULL; > + const char *path; > > sock = qemu_socket(PF_UNIX, SOCK_STREAM, 0); > if (sock < 0) { > @@ -852,20 +854,22 @@ static int unix_listen_saddr(UnixSocketAddress *saddr, > return -1; > } > > - memset(&un, 0, sizeof(un)); > - un.sun_family = AF_UNIX; > - if (saddr->path && strlen(saddr->path)) { > - snprintf(un.sun_path, sizeof(un.sun_path), "%s", saddr->path); > + if (saddr->path && saddr->path[0]) { > + path = saddr->path; > } else { > const char *tmpdir = getenv("TMPDIR"); > tmpdir = tmpdir ? tmpdir : "/tmp"; > - if (snprintf(un.sun_path, sizeof(un.sun_path), > "%s/qemu-socket-XXXXXX", > - tmpdir) >= sizeof(un.sun_path)) { > - error_setg_errno(errp, errno, > - "TMPDIR environment variable (%s) too large", > tmpdir); > - goto err; > - } > + path = pathbuf = g_strdup_printf("%s/qemu-socket-XXXXXX", tmpdir); > + } > > + if (strlen(path) > sizeof(un.sun_path)) { > + error_setg(errp, "UNIX socket path '%s' is too long", path); > + error_append_hint(errp, "Path must be less than %zu bytes\n", > + sizeof(un.sun_path)); > + goto err; > + } > + > + if (pathbuf != NULL) { > /* > * This dummy fd usage silences the mktemp() unsecure warning. > * Using mkstemp() doesn't make things more secure here > @@ -873,24 +877,25 @@ static int unix_listen_saddr(UnixSocketAddress *saddr, > * to unlink first and thus re-open the race window. The > * worst case possible is bind() failing, i.e. a DoS attack. > */ > - fd = mkstemp(un.sun_path); > + fd = mkstemp(pathbuf); > if (fd < 0) { > error_setg_errno(errp, errno, > - "Failed to make a temporary socket name in %s", > tmpdir); > + "Failed to make a temporary socket %s", > pathbuf); > goto err; > } > close(fd); > - if (update_addr) { > - g_free(saddr->path); > - saddr->path = g_strdup(un.sun_path); > - } > } > > - if (unlink(un.sun_path) < 0 && errno != ENOENT) { > + if (unlink(path) < 0 && errno != ENOENT) { > error_setg_errno(errp, errno, > - "Failed to unlink socket %s", un.sun_path); > + "Failed to unlink socket %s", path); > goto err; > } > + > + memset(&un, 0, sizeof(un)); > + un.sun_family = AF_UNIX; > + strncpy(un.sun_path, path, sizeof(un.sun_path)); > + > if (bind(sock, (struct sockaddr*) &un, sizeof(un)) < 0) { > error_setg_errno(errp, errno, "Failed to bind socket to %s", > un.sun_path); > goto err; > @@ -900,9 +905,16 @@ static int unix_listen_saddr(UnixSocketAddress *saddr, > goto err; > } > > + if (update_addr && pathbuf) { > + g_free(saddr->path); > + saddr->path = pathbuf; > + } else { > + g_free(pathbuf); > + } > return sock; > > err: > + g_free(pathbuf); > closesocket(sock); > return -1; > } > @@ -932,9 +944,16 @@ static int unix_connect_saddr(UnixSocketAddress *saddr, > qemu_set_nonblock(sock); > } > > + if (strlen(saddr->path) > sizeof(un.sun_path)) { > + error_setg(errp, "UNIX socket path '%s' is too long", saddr->path); > + error_append_hint(errp, "Path must be less than %zu bytes\n", > + sizeof(un.sun_path)); > + goto err; > + } > + > memset(&un, 0, sizeof(un)); > un.sun_family = AF_UNIX; > - snprintf(un.sun_path, sizeof(un.sun_path), "%s", saddr->path); > + strncpy(un.sun_path, saddr->path, sizeof(un.sun_path)); > > /* connect to peer */ > do { > @@ -956,13 +975,18 @@ static int unix_connect_saddr(UnixSocketAddress *saddr, > } > > if (rc < 0) { > - error_setg_errno(errp, -rc, "Failed to connect socket"); > - close(sock); > - sock = -1; > + error_setg_errno(errp, -rc, "Failed to connect socket %s", > + saddr->path); > + goto err; > } > > g_free(connect_state); > return sock; > + > + err: > + close(sock); > + g_free(connect_state); > + return -1; > } > > #else >
Queued, thanks. Paolo