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.bek...@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.lur...@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.lur...@redhat.com> > > +Reviewed-by: Samuel Thibault <samuel.thiba...@ens-lyon.org> > > +Message-Id: <20200127092414.169796-2-marcandre.lur...@redhat.com> > > +Signed-off-by: Oguz Bektas <o.bek...@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.lur...@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 <ler...@redhat.com> > > +Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com> > > +Reviewed-by: Samuel Thibault <samuel.thiba...@ens-lyon.org> > > +Message-Id: <20200127092414.169796-7-marcandre.lur...@redhat.com> > > +Signed-off-by: Oguz Bektas <o.bek...@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 > > > > _______________________________________________ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
