[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3] sockets: improve error reporting if UNIX soc
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v3] sockets: improve error reporting if UNIX socket path is too long |
Date: |
Thu, 25 May 2017 09:28:31 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.0 |
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 <address@hidden>
> ---
>
> } 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
signature.asc
Description: OpenPGP digital signature