On Tue, Jun 13, 2017 at 05:10:00PM +0100, Peter Maydell wrote:
> On 1 June 2017 at 13:41, Paolo Bonzini <pbonz...@redhat.com> wrote:
> > From: "Daniel P. Berrange" <berra...@redhat.com>
> >
> > 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 <berra...@redhat.com>
> > Message-Id: <20170525155300.22743-1-berra...@redhat.com>
> > Reviewed-by: Eric Blake <ebl...@redhat.com>
> > Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
> > ---
> > 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 :|
