[pve-devel] [PATCH pve-qemu] security patches for libslirp CVE-2020-8608
Thomas Lamprecht
t.lamprecht at proxmox.com
Fri Feb 7 09:03:50 CET 2020
On 2/6/20 3:25 PM, Oguz Bektas wrote:
> original commits and email can be found here[0]
>
> A out-of-bounds heap buffer access issue was found in the SLiRP
> networking implementation of the QEMU emulator. It occurs in tcp_emu()
> routine while emulating IRC and other protocols due to unsafe usage of
> snprintf(3) function.
>
> A user/process could use this flaw to crash the Qemu process on the host
> resulting in DoS or potentially execute arbitrary code with privileges
> of the QEMU process on the host.
>
for the record before anybody starts another offlist discussion :)
looks OK, but AFAICT the bridged mode we use is not affected by this,
the NAT (settable through CLI only) may be, but not to sure.
It really would help to reason why we need this, what is affected and how
one can check if it actual helped against an issue - the latter part is often
non-trivial, be it because an issue is theoretical or no reproducer is
available and not easy to think of oneself.
That said, the fmt sanitizing looks OK and won't hurt, so taking this in
is OK nonetheless, IMO
> [0]: https://seclists.org/oss-sec/2020/q1/64
>
> Signed-off-by: Oguz Bektas <o.bektas at proxmox.com>
> ---
> .../0003-util-add-slirp_fmt-helpers.patch | 126 ++++++++++++++++
> ...4-tcp_emu-fix-unsafe-snprintf-usages.patch | 135 ++++++++++++++++++
> debian/patches/series | 2 +
> 3 files changed, 263 insertions(+)
> create mode 100644 debian/patches/extra/0003-util-add-slirp_fmt-helpers.patch
> create mode 100644 debian/patches/extra/0004-tcp_emu-fix-unsafe-snprintf-usages.patch
>
> diff --git a/debian/patches/extra/0003-util-add-slirp_fmt-helpers.patch b/debian/patches/extra/0003-util-add-slirp_fmt-helpers.patch
> new file mode 100644
> index 0000000..af944f8
> --- /dev/null
> +++ b/debian/patches/extra/0003-util-add-slirp_fmt-helpers.patch
> @@ -0,0 +1,126 @@
> +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
> +From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= <marcandre.lureau at redhat.com>
> +Date: Mon, 27 Jan 2020 10:24:09 +0100
> +Subject: [PATCH 1/2] util: add slirp_fmt() helpers
> +MIME-Version: 1.0
> +Content-Type: text/plain; charset=UTF-8
> +Content-Transfer-Encoding: 8bit
> +
> +Various calls to snprintf() in libslirp assume that snprintf() returns
> +"only" the number of bytes written (excluding terminating NUL).
> +
> +https://pubs.opengroup.org/onlinepubs/9699919799/functions/snprintf.html#tag_16_159_04
> +
> +"Upon successful completion, the snprintf() function shall return the
> +number of bytes that would be written to s had n been sufficiently
> +large excluding the terminating null byte."
> +
> +Introduce slirp_fmt() that handles several pathological cases the
> +way libslirp usually expect:
> +
> +- treat error as fatal (instead of silently returning -1)
> +
> +- fmt0() will always \0 end
> +
> +- return the number of bytes actually written (instead of what would
> +have been written, which would usually result in OOB later), including
> +the ending \0 for fmt0()
> +
> +- warn if truncation happened (instead of ignoring)
> +
> +Other less common cases can still be handled with strcpy/snprintf() etc.
> +
> +Signed-off-by: Marc-André Lureau <marcandre.lureau at redhat.com>
> +Reviewed-by: Samuel Thibault <samuel.thibault at ens-lyon.org>
> +Message-Id: <20200127092414.169796-2-marcandre.lureau at redhat.com>
> +Signed-off-by: Oguz Bektas <o.bektas at proxmox.com>
> +---
> + slirp/src/util.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> + slirp/src/util.h | 3 +++
> + 2 files changed, 65 insertions(+)
> +
> +diff --git a/slirp/src/util.c b/slirp/src/util.c
> +index e596087..e3b6257 100644
> +--- a/slirp/src/util.c
> ++++ b/slirp/src/util.c
> +@@ -364,3 +364,65 @@ void slirp_pstrcpy(char *buf, int buf_size, const char *str)
> + }
> + *q = '\0';
> + }
> ++
> ++static int slirp_vsnprintf(char *str, size_t size,
> ++ const char *format, va_list args)
> ++{
> ++ int rv = vsnprintf(str, size, format, args);
> ++
> ++ if (rv < 0) {
> ++ g_error("vsnprintf() failed: %s", g_strerror(errno));
> ++ }
> ++
> ++ return rv;
> ++}
> ++
> ++/*
> ++ * A snprintf()-like function that:
> ++ * - returns the number of bytes written (excluding optional \0-ending)
> ++ * - dies on error
> ++ * - warn on truncation
> ++ */
> ++int slirp_fmt(char *str, size_t size, const char *format, ...)
> ++{
> ++ va_list args;
> ++ int rv;
> ++
> ++ va_start(args, format);
> ++ rv = slirp_vsnprintf(str, size, format, args);
> ++ va_end(args);
> ++
> ++ if (rv > size) {
> ++ g_critical("vsnprintf() truncation");
> ++ }
> ++
> ++ return MIN(rv, size);
> ++}
> ++
> ++/*
> ++ * A snprintf()-like function that:
> ++ * - always \0-end (unless size == 0)
> ++ * - returns the number of bytes actually written, including \0 ending
> ++ * - dies on error
> ++ * - warn on truncation
> ++ */
> ++int slirp_fmt0(char *str, size_t size, const char *format, ...)
> ++{
> ++ va_list args;
> ++ int rv;
> ++
> ++ va_start(args, format);
> ++ rv = slirp_vsnprintf(str, size, format, args);
> ++ va_end(args);
> ++
> ++ if (rv >= size) {
> ++ g_critical("vsnprintf() truncation");
> ++ if (size > 0)
> ++ str[size - 1] = '\0';
> ++ rv = size;
> ++ } else {
> ++ rv += 1; /* include \0 */
> ++ }
> ++
> ++ return rv;
> ++}
> +diff --git a/slirp/src/util.h b/slirp/src/util.h
> +index 3c6223c..0558dfc 100644
> +--- a/slirp/src/util.h
> ++++ b/slirp/src/util.h
> +@@ -177,4 +177,7 @@ static inline int slirp_socket_set_fast_reuse(int fd)
> +
> + void slirp_pstrcpy(char *buf, int buf_size, const char *str);
> +
> ++int slirp_fmt(char *str, size_t size, const char *format, ...);
> ++int slirp_fmt0(char *str, size_t size, const char *format, ...);
> ++
> + #endif
> +--
> +2.20.1
> +
> diff --git a/debian/patches/extra/0004-tcp_emu-fix-unsafe-snprintf-usages.patch b/debian/patches/extra/0004-tcp_emu-fix-unsafe-snprintf-usages.patch
> new file mode 100644
> index 0000000..099fecd
> --- /dev/null
> +++ b/debian/patches/extra/0004-tcp_emu-fix-unsafe-snprintf-usages.patch
> @@ -0,0 +1,135 @@
> +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
> +From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= <marcandre.lureau at redhat.com>
> +Date: Mon, 27 Jan 2020 10:24:14 +0100
> +Subject: [PATCH 2/2] tcp_emu: fix unsafe snprintf() usages
> +MIME-Version: 1.0
> +Content-Type: text/plain; charset=UTF-8
> +Content-Transfer-Encoding: 8bit
> +
> +Various calls to snprintf() assume that snprintf() returns "only" the
> +number of bytes written (excluding terminating NUL).
> +
> +https://pubs.opengroup.org/onlinepubs/9699919799/functions/snprintf.html#tag_16_159_04
> +
> +"Upon successful completion, the snprintf() function shall return the
> +number of bytes that would be written to s had n been sufficiently
> +large excluding the terminating null byte."
> +
> +Before patch ce131029, if there isn't enough room in "m_data" for the
> +"DCC ..." message, we overflow "m_data".
> +
> +After the patch, if there isn't enough room for the same, we don't
> +overflow "m_data", but we set "m_len" out-of-bounds. The next time an
> +access is bounded by "m_len", we'll have a buffer overflow then.
> +
> +Use slirp_fmt*() to fix potential OOB memory access.
> +
> +Reported-by: Laszlo Ersek <lersek at redhat.com>
> +Signed-off-by: Marc-André Lureau <marcandre.lureau at redhat.com>
> +Reviewed-by: Samuel Thibault <samuel.thibault at ens-lyon.org>
> +Message-Id: <20200127092414.169796-7-marcandre.lureau at redhat.com>
> +Signed-off-by: Oguz Bektas <o.bektas at proxmox.com>
> +---
> + slirp/src/tcp_subr.c | 44 +++++++++++++++++++++-----------------------
> + 1 file changed, 21 insertions(+), 23 deletions(-)
> +
> +diff --git a/slirp/src/tcp_subr.c b/slirp/src/tcp_subr.c
> +index d6dd133..93b3488 100644
> +--- a/slirp/src/tcp_subr.c
> ++++ b/slirp/src/tcp_subr.c
> +@@ -655,8 +655,7 @@ int tcp_emu(struct socket *so, struct mbuf *m)
> + NTOHS(n1);
> + NTOHS(n2);
> + m_inc(m, snprintf(NULL, 0, "%d,%d\r\n", n1, n2) + 1);
> +- m->m_len = snprintf(m->m_data, M_ROOM(m), "%d,%d\r\n", n1, n2);
> +- assert(m->m_len < M_ROOM(m));
> ++ m->m_len = slirp_fmt(m->m_data, M_ROOM(m), "%d,%d\r\n", n1, n2);
> + } else {
> + *eol = '\r';
> + }
> +@@ -696,9 +695,9 @@ int tcp_emu(struct socket *so, struct mbuf *m)
> + n4 = (laddr & 0xff);
> +
> + m->m_len = bptr - m->m_data; /* Adjust length */
> +- m->m_len += snprintf(bptr, m->m_size - m->m_len,
> +- "ORT %d,%d,%d,%d,%d,%d\r\n%s", n1, n2, n3, n4,
> +- n5, n6, x == 7 ? buff : "");
> ++ m->m_len += slirp_fmt(bptr, M_FREEROOM(m),
> ++ "ORT %d,%d,%d,%d,%d,%d\r\n%s",
> ++ n1, n2, n3, n4, n5, n6, x == 7 ? buff : "");
> + return 1;
> + } else if ((bptr = (char *)strstr(m->m_data, "27 Entering")) != NULL) {
> + /*
> +@@ -731,11 +730,9 @@ int tcp_emu(struct socket *so, struct mbuf *m)
> + n4 = (laddr & 0xff);
> +
> + m->m_len = bptr - m->m_data; /* Adjust length */
> +- m->m_len +=
> +- snprintf(bptr, m->m_size - m->m_len,
> +- "27 Entering Passive Mode (%d,%d,%d,%d,%d,%d)\r\n%s",
> +- n1, n2, n3, n4, n5, n6, x == 7 ? buff : "");
> +-
> ++ m->m_len += slirp_fmt(bptr, M_FREEROOM(m),
> ++ "27 Entering Passive Mode (%d,%d,%d,%d,%d,%d)\r\n%s",
> ++ n1, n2, n3, n4, n5, n6, x == 7 ? buff : "");
> + return 1;
> + }
> +
> +@@ -758,8 +755,8 @@ int tcp_emu(struct socket *so, struct mbuf *m)
> + if (m->m_data[m->m_len - 1] == '\0' && lport != 0 &&
> + (so = tcp_listen(slirp, INADDR_ANY, 0, so->so_laddr.s_addr,
> + htons(lport), SS_FACCEPTONCE)) != NULL)
> +- m->m_len =
> +- snprintf(m->m_data, m->m_size, "%d", ntohs(so->so_fport)) + 1;
> ++ m->m_len = slirp_fmt0(m->m_data, M_ROOM(m),
> ++ "%d", ntohs(so->so_fport));
> + return 1;
> +
> + case EMU_IRC:
> +@@ -778,9 +775,10 @@ int tcp_emu(struct socket *so, struct mbuf *m)
> + return 1;
> + }
> + m->m_len = bptr - m->m_data; /* Adjust length */
> +- m->m_len += snprintf(bptr, m->m_size, "DCC CHAT chat %lu %u%c\n",
> +- (unsigned long)ntohl(so->so_faddr.s_addr),
> +- ntohs(so->so_fport), 1);
> ++ m->m_len += slirp_fmt(bptr, M_FREEROOM(m),
> ++ "DCC CHAT chat %lu %u%c\n",
> ++ (unsigned long)ntohl(so->so_faddr.s_addr),
> ++ ntohs(so->so_fport), 1);
> + } else if (sscanf(bptr, "DCC SEND %256s %u %u %u", buff, &laddr, &lport,
> + &n1) == 4) {
> + if ((so = tcp_listen(slirp, INADDR_ANY, 0, htonl(laddr),
> +@@ -788,10 +786,10 @@ int tcp_emu(struct socket *so, struct mbuf *m)
> + return 1;
> + }
> + m->m_len = bptr - m->m_data; /* Adjust length */
> +- m->m_len +=
> +- snprintf(bptr, m->m_size, "DCC SEND %s %lu %u %u%c\n", buff,
> +- (unsigned long)ntohl(so->so_faddr.s_addr),
> +- ntohs(so->so_fport), n1, 1);
> ++ m->m_len += slirp_fmt(bptr, M_FREEROOM(m),
> ++ "DCC SEND %s %lu %u %u%c\n", buff,
> ++ (unsigned long)ntohl(so->so_faddr.s_addr),
> ++ ntohs(so->so_fport), n1, 1);
> + } else if (sscanf(bptr, "DCC MOVE %256s %u %u %u", buff, &laddr, &lport,
> + &n1) == 4) {
> + if ((so = tcp_listen(slirp, INADDR_ANY, 0, htonl(laddr),
> +@@ -799,10 +797,10 @@ int tcp_emu(struct socket *so, struct mbuf *m)
> + return 1;
> + }
> + m->m_len = bptr - m->m_data; /* Adjust length */
> +- m->m_len +=
> +- snprintf(bptr, m->m_size, "DCC MOVE %s %lu %u %u%c\n", buff,
> +- (unsigned long)ntohl(so->so_faddr.s_addr),
> +- ntohs(so->so_fport), n1, 1);
> ++ m->m_len += slirp_fmt(bptr, M_FREEROOM(m),
> ++ "DCC MOVE %s %lu %u %u%c\n", buff,
> ++ (unsigned long)ntohl(so->so_faddr.s_addr),
> ++ ntohs(so->so_fport), n1, 1);
> + }
> + return 1;
> +
> +--
> +2.20.1
> +
> diff --git a/debian/patches/series b/debian/patches/series
> index c37c4e1..dd65043 100644
> --- a/debian/patches/series
> +++ b/debian/patches/series
> @@ -1,5 +1,7 @@
> extra/0001-monitor-qmp-resume-monitor-when-clearing-its-queue.patch
> extra/0002-virtio-blk-schedule-virtio_notify_config-to-run-on-m.patch
> +extra/0003-util-add-slirp_fmt-helpers.patch
> +extra/0004-tcp_emu-fix-unsafe-snprintf-usages.patch
> pve/0001-PVE-Config-block-file-change-locking-default-to-off.patch
> pve/0002-PVE-Config-Adjust-network-script-path-to-etc-kvm.patch
> pve/0003-PVE-Config-set-the-CPU-model-to-kvm64-32-instead-of-.patch
>
More information about the pve-devel
mailing list