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.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 util/qemu-sockets.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index d8183f7..d565499 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -845,6 +845,7 @@ static int unix_listen_saddr(UnixSocketAddress *saddr,
 {
     struct sockaddr_un un;
     int sock, fd;
+    size_t pathlen;
 
     sock = qemu_socket(PF_UNIX, SOCK_STREAM, 0);
     if (sock < 0) {
@@ -854,15 +855,25 @@ static int unix_listen_saddr(UnixSocketAddress *saddr,
 
     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);
+    pathlen = saddr->path ? strlen(saddr->path) : 0;
+    if (pathlen != 0) {
+        if (pathlen > 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;
+        }
+        strncpy(un.sun_path, saddr->path, sizeof(un.sun_path));
     } else {
+        const char *template = "qemu-socket-XXXXXX";
         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);
+        if (snprintf(un.sun_path, sizeof(un.sun_path), "%s/%s",
+                     tmpdir, template) >= sizeof(un.sun_path)) {
+            error_setg(errp,
+                       "TMPDIR environment variable (%s) too large", tmpdir);
+            error_append_hint(errp, "Path must be less than %zu bytes\n",
+                              sizeof(un.sun_path) - strlen(template) - 1);
             goto err;
         }
 
-- 
2.9.3

On 05/24/2017 11:18 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.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  util/qemu-sockets.c | 23 +++++++++++++++++------
>  1 file changed, 17 insertions(+), 6 deletions(-)


> +    pathlen = saddr->path ? strlen(saddr->path) : 0;
> +    if (pathlen != 0) {
> +        if (pathlen > 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;
> +        }
> +        strncpy(un.sun_path, saddr->path, sizeof(un.sun_path));

Does NOT nul-terminate if pathlen is exactly 108 - but it looks like
Linux is just fine with a Unix socket name that long.

>      } else {
> +        const char *template = "qemu-socket-XXXXXX";
>          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);
> +        if (snprintf(un.sun_path, sizeof(un.sun_path), "%s/%s",
> +                     tmpdir, template) >= sizeof(un.sun_path)) {
> +            error_setg(errp,
> +                       "TMPDIR environment variable (%s) too large", tmpdir);
> +            error_append_hint(errp, "Path must be less than %zu bytes\n",
> +                              sizeof(un.sun_path) - strlen(template) - 1);

Meanwhile, this branch requires nul-termination (that is, a maximum
generated name of 107), but that's because we then pass un.sun_path to
mkstemp() and g_strdup(), both of which require nul-termination.  It's a
bit odd to see the asymmetry between a user-specified name going all the
way to 108 while generated cannot, but not the end of the world.

You do, however, have one more problem to fix:

    if (unlink(un.sun_path) < 0 && errno != ENOENT) {

That now needs to be 'if (unlink(saddr->path) ...', because un.sun_path
is no longer guaranteed to be NUL-terminated (although saddr->path is).
Likewise for the two error_setg_errno() calls after that point.

With that fixed,

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

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