[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Qemu-devel] [PULL 22/33] sockets: improve error reporting if UNIX socke
From: |
Paolo Bonzini |
Subject: |
[Qemu-devel] [PULL 22/33] sockets: improve error reporting if UNIX socket path is too long |
Date: |
Thu, 1 Jun 2017 14:41:40 +0200 |
From: "Daniel P. Berrange" <address@hidden>
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 <address@hidden>
Message-Id: <address@hidden>
Reviewed-by: Eric Blake <address@hidden>
Signed-off-by: Paolo Bonzini <address@hidden>
---
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
--
1.8.3.1
- Re: [Qemu-devel] [PULL 17/33] vhost-user-scsi: Introduce vhost-user-scsi host device, (continued)
[Qemu-devel] [PULL 19/33] target/i386: enable A20 automatically in system management mode, Paolo Bonzini, 2017/06/01
[Qemu-devel] [PULL 18/33] vhost-user-scsi: Introduce a vhost-user-scsi sample application, Paolo Bonzini, 2017/06/01
[Qemu-devel] [PULL 21/33] i386: fix read/write cr with icount option, Paolo Bonzini, 2017/06/01
[Qemu-devel] [PULL 20/33] target/i386: use multiple CPU AddressSpaces, Paolo Bonzini, 2017/06/01
[Qemu-devel] [PULL 23/33] exec: fix address_space_get_iotlb_entry page mask, Paolo Bonzini, 2017/06/01
[Qemu-devel] [PULL 24/33] nbd: Fully initialize client in case of failed negotiation, Paolo Bonzini, 2017/06/01
[Qemu-devel] [PULL 22/33] sockets: improve error reporting if UNIX socket path is too long,
Paolo Bonzini <=
[Qemu-devel] [PULL 26/33] kvmclock: update system_time_msr address forcibly, Paolo Bonzini, 2017/06/01
[Qemu-devel] [PULL 25/33] qtest: add rtc periodic timer test, Paolo Bonzini, 2017/06/01
[Qemu-devel] [PULL 27/33] linuxboot_dma: compile for i486, Paolo Bonzini, 2017/06/01
[Qemu-devel] [PULL 28/33] edu: fix memory leak on msi_broken platforms, Paolo Bonzini, 2017/06/01
[Qemu-devel] [PULL 30/33] target/i386: Add GDB XML description for SSE registers, Paolo Bonzini, 2017/06/01
[Qemu-devel] [PULL 31/33] hw/core: nmi.c can be compiled as common-obj nowadays, Paolo Bonzini, 2017/06/01
[Qemu-devel] [PULL 29/33] i386/kvm: do not zero out segment flags if segment is unusable or not present, Paolo Bonzini, 2017/06/01
[Qemu-devel] [PULL 33/33] kvm: don't register smram_listener when smm is off, Paolo Bonzini, 2017/06/01