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

Daniel P. Berrange posted 1 patch 6 years, 10 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20170525101900.15950-1-berrange@redhat.com
Test checkpatch passed
Test docker passed
Test s390x passed
There is a newer version of this series
util/qemu-sockets.c | 64 +++++++++++++++++++++++++++++++++++------------------
1 file changed, 43 insertions(+), 21 deletions(-)
[Qemu-devel] [PATCH v3] sockets: improve error reporting if UNIX socket path is too long
Posted by Daniel P. Berrange 6 years, 10 months ago
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 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 | 64 +++++++++++++++++++++++++++++++++++------------------
 1 file changed, 43 insertions(+), 21 deletions(-)

diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index d8183f7..59c774e 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,19 +854,12 @@ 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);
 
         /*
          * This dummy fd usage silences the mktemp() unsecure warning.
@@ -873,24 +868,32 @@ 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);
             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 (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 (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 +903,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 +942,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 +973,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 v3] sockets: improve error reporting if UNIX socket path is too long
Posted by Eric Blake 6 years, 10 months ago
On 05/25/2017 05:19 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>
> ---
> 

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

The old code fails early if the generated name is too long,...

> -        }
> +        path = pathbuf = g_strdup_printf("%s/qemu-socket-XXXXXX", tmpdir);
>  
>          /*
>           * This dummy fd usage silences the mktemp() unsecure warning.
> @@ -873,24 +868,32 @@ 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);

...while the new code ends up creating a too-long name in the
file-system anyways,...

>          if (fd < 0) {
>              error_setg_errno(errp, errno,
>                               "Failed to make a temporary socket name in %s", tmpdir);
>              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 (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));

The old message mentioned the name that was too long, the new does not.
That's a potential slight loss in error message quality, particularly
since you are no longer mentioning a UNIX socket (which may give the
user a clue why 108 is special), but I can probably live with it (it's
still pretty obvious from the message that you should investigate what
is too long).

> +        goto err;

...here, you finally diagnose that the name is too long,...

> +    }
> +
> +    if (unlink(path) < 0 && errno != ENOENT) {

...but skip the unlink of the just-created too-long name.  Oops.

>          error_setg_errno(errp, errno,
> -                         "Failed to unlink socket %s", un.sun_path);
> +                         "Failed to unlink socket %s", path);
>          goto err;
>      }

I think you're okay if you just swap the unlink() with the length check.
 Although it seems rather sys-call heavy to mkstemp/unlink compared to
just doing a length check first, I do like how your refactoring ended up
with fewer conditionals rather than splitting the generated name code
across multiple conditionals.

> @@ -932,9 +942,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));

Again, potential loss in error message quality.

Hopefully fourth time is the charm!

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

Re: [Qemu-devel] [PATCH v3] sockets: improve error reporting if UNIX socket path is too long
Posted by Daniel P. Berrange 6 years, 10 months ago
On Thu, May 25, 2017 at 09:28:31AM -0500, Eric Blake wrote:
> On 05/25/2017 05:19 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>
> > ---
> > 
> 
> >      } 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;
> 
> The old code fails early if the generated name is too long,...
> 
> > -        }
> > +        path = pathbuf = g_strdup_printf("%s/qemu-socket-XXXXXX", tmpdir);
> >  
> >          /*
> >           * This dummy fd usage silences the mktemp() unsecure warning.
> > @@ -873,24 +868,32 @@ 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);
> 
> ...while the new code ends up creating a too-long name in the
> file-system anyways,...

Opps, yes, that's not good.

> 
> >          if (fd < 0) {
> >              error_setg_errno(errp, errno,
> >                               "Failed to make a temporary socket name in %s", tmpdir);
> >              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 (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));
> 
> The old message mentioned the name that was too long, the new does not.
> That's a potential slight loss in error message quality, particularly
> since you are no longer mentioning a UNIX socket (which may give the
> user a clue why 108 is special), but I can probably live with it (it's
> still pretty obvious from the message that you should investigate what
> is too long).

I'm not sure why you are saying I don't mention the path or that it
is a UNIX socket - I do exactly that right there.

$ ./x86_64-softmmu/qemu-system-x86_64 -monitor unix:/home/berrange/src/virt/qemu/01234567890123456789012345678901234567890123456789012345678901234567890123456789,server
qemu-system-x86_64: -monitor unix:/home/berrange/src/virt/qemu/01234567890123456789012345678901234567890123456789012345678901234567890123456789,server: UNIX socket path '/home/berrange/src/virt/qemu/01234567890123456789012345678901234567890123456789012345678901234567890123456789' is too long
Path must be less than 108 bytes

> > +    if (unlink(path) < 0 && errno != ENOENT) {
> 
> ...but skip the unlink of the just-created too-long name.  Oops.

yeah will fix that too

> 
> >          error_setg_errno(errp, errno,
> > -                         "Failed to unlink socket %s", un.sun_path);
> > +                         "Failed to unlink socket %s", path);
> >          goto err;
> >      }
> 
> I think you're okay if you just swap the unlink() with the length check.
>  Although it seems rather sys-call heavy to mkstemp/unlink compared to
> just doing a length check first, I do like how your refactoring ended up
> with fewer conditionals rather than splitting the generated name code
> across multiple conditionals.
> 
> > @@ -932,9 +942,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));
> 
> Again, potential loss in error message quality.

I don't think so.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH v3] sockets: improve error reporting if UNIX socket path is too long
Posted by Eric Blake 6 years, 10 months ago
On 05/25/2017 09:40 AM, Daniel P. Berrange wrote:
> On Thu, May 25, 2017 at 09:28:31AM -0500, Eric Blake wrote:
>> On 05/25/2017 05:19 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>

>>>  
>>> -    if (unlink(un.sun_path) < 0 && errno != ENOENT) {
>>> +    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));
>>
>> The old message mentioned the name that was too long, the new does not.
>> That's a potential slight loss in error message quality, particularly
>> since you are no longer mentioning a UNIX socket (which may give the
>> user a clue why 108 is special), but I can probably live with it (it's
>> still pretty obvious from the message that you should investigate what
>> is too long).
> 
> I'm not sure why you are saying I don't mention the path or that it
> is a UNIX socket - I do exactly that right there.

Oh shoot, I was misreading things.  I saw two lines of error_*, and must
have thought that the first was the deletion and the second an
insertion. But you're right that they are both insertions, that the
first is an accurate message, and the second is a hint.

Never mind, I'm obviously not fully awake yet today ;)


>>> @@ -932,9 +942,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));
>>
>> Again, potential loss in error message quality.
> 
> I don't think so.

Yep, another instance of me reading the patch wrong.  This part's fine,
after all.

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