[Qemu-devel] [PATCH v4] sockets: improve error reporting if UNIX socket path is too long

[Qemu-devel] [PATCH v4] sockets: improve error reporting if UNIX socket path is too long

[Daniel P. Berrange]

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>
---

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
-- 
2.9.3

Re: [Qemu-devel] [PATCH v4] sockets: improve error reporting if UNIX socket path is too long

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1194 bytes --]

On 05/25/2017 10:53 AM, 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 <berrange@redhat.com>
> ---
> 
> Changed in v4:
> 
>  - Do length check before any mkstemp to avoid leaving long
>    tmpfile on disk on error

Took us a few iterations, but it was worth it.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v4] sockets: improve error reporting if UNIX socket path is too long

[Paolo Bonzini]

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 <berrange@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