Message ID | 1496320911-51305-23-git-send-email-pbonzini@redhat.com |
---|---|
State | New |
Headers | show |
On 1 June 2017 at 13:41, Paolo Bonzini <pbonzini@redhat.com> wrote: > From: "Daniel P. Berrange" <berrange@redhat.com> > > 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 <berrange@redhat.com> > Message-Id: <20170525155300.22743-1-berrange@redhat.com> > Reviewed-by: Eric Blake <eblake@redhat.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > util/qemu-sockets.c | 68 ++++++++++++++++++++++++++++++++++++----------------- > 1 file changed, 46 insertions(+), 22 deletions(-) It looks like we missed a case where we should have changed an un.sun_path usage to something else: > @@ -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); ...you can see it in this bit of the context: this should be passing "path" to the %s format string, shouldn't it? Spotted looking at coverity issues, though unfortunately coverity just always reports the "buffer not nul terminated" error rather than only the cases where we don't nul terminate and then hand the buffer to a function which consumes a nul-terminated string, so we just have to mark the issue 'ignore' and don't get the benefit of static checking :-( thanks -- PMM
On Tue, Jun 13, 2017 at 05:10:00PM +0100, Peter Maydell wrote: > On 1 June 2017 at 13:41, Paolo Bonzini <pbonzini@redhat.com> wrote: > > From: "Daniel P. Berrange" <berrange@redhat.com> > > > > 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 <berrange@redhat.com> > > Message-Id: <20170525155300.22743-1-berrange@redhat.com> > > Reviewed-by: Eric Blake <eblake@redhat.com> > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > > --- > > util/qemu-sockets.c | 68 ++++++++++++++++++++++++++++++++++++----------------- > > 1 file changed, 46 insertions(+), 22 deletions(-) > > It looks like we missed a case where we should have changed > an un.sun_path usage to something else: > > > @@ -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); > > ...you can see it in this bit of the context: this should be passing > "path" to the %s format string, shouldn't it? Yes, you are correct - we must use "path" > > Spotted looking at coverity issues, though unfortunately coverity > just always reports the "buffer not nul terminated" error rather > than only the cases where we don't nul terminate and then hand > the buffer to a function which consumes a nul-terminated string, > so we just have to mark the issue 'ignore' and don't get the > benefit of static checking :-( Regards, Daniel
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