[pve-devel] [PATCH pve-qemu] security patches for libslirp CVE-2020-8608
Oguz Bektas
o.bektas at proxmox.com
Mon Feb 10 16:17:26 CET 2020
hi,
On Fri, Feb 07, 2020 at 09:03:50AM +0100, Thomas Lamprecht wrote:
> 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
>
yes, it seems the slirp networking is only used in qemu user
networking[0], so our regular bridged mode shouldn't be affected.
also we pass the `-nodefaults` option which avoids using the default
networking (slirp). so i think we should be relatively safe.
nevertheless it doesn't hurt to apply it.
for now as far as i'm aware there's no public PoC for this.
also i noticed they disabled the vulnerable `tcp_emu` in libslirp 4.1.0 [1]
[0]: https://wiki.qemu.org/Documentation/Networking#User_Networking_.28SLIRP.29
[1]: https://gitlab.freedesktop.org/slirp/libslirp/-/tags/v4.1.0
> > [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