From patchwork Thu May 25 10:19:00 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: =?utf-8?q?Daniel_P=2E_Berrang=C3=A9?= X-Patchwork-Id: 9748061 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 5C934601E9 for ; Thu, 25 May 2017 10:20:20 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 3D3EF2766D for ; Thu, 25 May 2017 10:20:20 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 2E7F627D0E; Thu, 25 May 2017 10:20:20 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 4F53527CAF for ; Thu, 25 May 2017 10:20:07 +0000 (UTC) Received: from localhost ([::1]:59159 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dDpsE-00065w-Rs for patchwork-qemu-devel@patchwork.kernel.org; Thu, 25 May 2017 06:20:06 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51855) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dDprO-000642-41 for qemu-devel@nongnu.org; Thu, 25 May 2017 06:19:15 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1dDprJ-0004sW-5o for qemu-devel@nongnu.org; Thu, 25 May 2017 06:19:14 -0400 Received: from mx1.redhat.com ([209.132.183.28]:46126) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1dDprI-0004sM-Sv for qemu-devel@nongnu.org; Thu, 25 May 2017 06:19:09 -0400 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id DE9F23DE3D for ; Thu, 25 May 2017 10:19:07 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com DE9F23DE3D Authentication-Results: ext-mx05.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx05.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=berrange@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com DE9F23DE3D Received: from t460.redhat.com (ovpn-116-220.ams2.redhat.com [10.36.116.220]) by smtp.corp.redhat.com (Postfix) with ESMTP id 66EC917243; Thu, 25 May 2017 10:19:04 +0000 (UTC) From: "Daniel P. Berrange" To: qemu-devel@nongnu.org Date: Thu, 25 May 2017 11:19:00 +0100 Message-Id: <20170525101900.15950-1-berrange@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.29]); Thu, 25 May 2017 10:19:08 +0000 (UTC) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] [fuzzy] X-Received-From: 209.132.183.28 Subject: [Qemu-devel] [PATCH v3] sockets: improve error reporting if UNIX socket path is too long X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Paolo Bonzini , Gerd Hoffmann Errors-To: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org Sender: "Qemu-devel" X-Virus-Scanned: ClamAV using ClamSMTP 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 --- 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