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

["Daniel P. Berrange" <berrange@redhat.com>]

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