Re: [Qemu-devel] [PULL 22/33] sockets: improve error reporting if UNIX socket path is too long

[Daniel P. Berrange]

On Tue, Jun 13, 2017 at 05:10:00PM +0100, Peter Maydell wrote:
> On 1 June 2017 at 13:41, Paolo Bonzini <address@hidden> wrote:
> > 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(-)
> 
> It looks like we missed a case where we should have changed
> an un.sun_path usage to something else:
> 
> > @@ -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);
> 
> ...you can see it in this bit of the context: this should be passing
> "path" to the %s format string, shouldn't it?

Yes, you are correct - we must use  "path"

> 
> Spotted looking at coverity issues, though unfortunately coverity
> just always reports the "buffer not nul terminated" error rather
> than only the cases where we don't nul terminate and then hand
> the buffer to a function which consumes a nul-terminated string,
> so we just have to mark the issue 'ignore' and don't get the
> benefit of static checking :-(

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