[libvirt] [PATCH] qemu: avoid denial of service reading from QEMU monitor (CVE-2018-xxxx)
We read from QEMU until seeing a \r\n pair to indicate a completed reply or event. To avoid memory denial-of-service though, we must have a size limit on amount of data we buffer. 10 MB is large enough that it ought to cope with normal QEMU replies, and small enough that we're not consuming unreasonable mem. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/qemu/qemu_monitor.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 046caf001c..85c7d68a13 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -55,6 +55,15 @@ VIR_LOG_INIT("qemu.qemu_monitor"); #define DEBUG_IO 0 #define DEBUG_RAW_IO 0 +/* We read from QEMU until seeing a \r\n pair to indicate a + * completed reply or event. To avoid memory denial-of-service + * though, we must have a size limit on amount of data we + * buffer. 10 MB is large enough that it ought to cope with + * normal QEMU replies, and small enough that we're not + * consuming unreasonable mem. + */ +#define QEMU_MONITOR_MAX_RESPONSE (10 * 1024 * 1024) + struct _qemuMonitor { virObjectLockable parent; @@ -575,6 +584,12 @@ qemuMonitorIORead(qemuMonitorPtr mon) int ret = 0; if (avail < 1024) { + if (mon->bufferLength >= QEMU_MONITOR_MAX_RESPONSE) { + virReportSystemError(ERANGE, + _("No complete monitor response found in %d bytes"), + QEMU_MONITOR_MAX_RESPONSE); + return -1; + } if (VIR_REALLOC_N(mon->buffer, mon->bufferLength + 1024) < 0) return -1; -- 2.14.3
On 01/16/2018 06:01 PM, Daniel P. Berrange wrote:
We read from QEMU until seeing a \r\n pair to indicate a completed reply or event. To avoid memory denial-of-service though, we must have a size limit on amount of data we buffer. 10 MB is large enough that it ought to cope with normal QEMU replies, and small enough that we're not consuming unreasonable mem.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/qemu/qemu_monitor.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 046caf001c..85c7d68a13 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -55,6 +55,15 @@ VIR_LOG_INIT("qemu.qemu_monitor"); #define DEBUG_IO 0 #define DEBUG_RAW_IO 0
+/* We read from QEMU until seeing a \r\n pair to indicate a + * completed reply or event. To avoid memory denial-of-service + * though, we must have a size limit on amount of data we + * buffer. 10 MB is large enough that it ought to cope with + * normal QEMU replies, and small enough that we're not + * consuming unreasonable mem. + */ +#define QEMU_MONITOR_MAX_RESPONSE (10 * 1024 * 1024) + struct _qemuMonitor { virObjectLockable parent;
@@ -575,6 +584,12 @@ qemuMonitorIORead(qemuMonitorPtr mon) int ret = 0;
if (avail < 1024) { + if (mon->bufferLength >= QEMU_MONITOR_MAX_RESPONSE) { + virReportSystemError(ERANGE, + _("No complete monitor response found in %d bytes"), + QEMU_MONITOR_MAX_RESPONSE); + return -1; + } if (VIR_REALLOC_N(mon->buffer, mon->bufferLength + 1024) < 0) return -1;
ACK, although is this really a CVE? Doesn't look that harmful to me. I mean, owning qemu is not that easy, is it? Michal
On Wed, Jan 17, 2018 at 05:13:06PM +0100, Michal Privoznik wrote:
On 01/16/2018 06:01 PM, Daniel P. Berrange wrote:
We read from QEMU until seeing a \r\n pair to indicate a completed reply or event. To avoid memory denial-of-service though, we must have a size limit on amount of data we buffer. 10 MB is large enough that it ought to cope with normal QEMU replies, and small enough that we're not consuming unreasonable mem.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/qemu/qemu_monitor.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 046caf001c..85c7d68a13 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -55,6 +55,15 @@ VIR_LOG_INIT("qemu.qemu_monitor"); #define DEBUG_IO 0 #define DEBUG_RAW_IO 0
+/* We read from QEMU until seeing a \r\n pair to indicate a + * completed reply or event. To avoid memory denial-of-service + * though, we must have a size limit on amount of data we + * buffer. 10 MB is large enough that it ought to cope with + * normal QEMU replies, and small enough that we're not + * consuming unreasonable mem. + */ +#define QEMU_MONITOR_MAX_RESPONSE (10 * 1024 * 1024) + struct _qemuMonitor { virObjectLockable parent;
@@ -575,6 +584,12 @@ qemuMonitorIORead(qemuMonitorPtr mon) int ret = 0;
if (avail < 1024) { + if (mon->bufferLength >= QEMU_MONITOR_MAX_RESPONSE) { + virReportSystemError(ERANGE, + _("No complete monitor response found in %d bytes"), + QEMU_MONITOR_MAX_RESPONSE); + return -1; + } if (VIR_REALLOC_N(mon->buffer, mon->bufferLength + 1024) < 0) return -1;
ACK, although is this really a CVE? Doesn't look that harmful to me. I mean, owning qemu is not that easy, is it?
There are a never ending stream of CVEs in QEMU guest device models. These are mitigated by SELinux preventing a compromised QEMU from attacking resources on the host it isn't granted access to. So attention would naturally focus on attacking things it already has access to like the libvirt monitor connection. Memory denial of service in libvirt is not too serious, but still a CVE bug. Worse would be if libvirtd has buffer overflow/crash in parsing a JSON response... Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 01/17/2018 10:13 AM, Michal Privoznik wrote:
On 01/16/2018 06:01 PM, Daniel P. Berrange wrote:
We read from QEMU until seeing a \r\n pair to indicate a completed reply or event. To avoid memory denial-of-service though, we must have a size limit on amount of data we buffer. 10 MB is large enough that it ought to cope with normal QEMU replies, and small enough that we're not consuming unreasonable mem.
ACK, although is this really a CVE? Doesn't look that harmful to me. I mean, owning qemu is not that easy, is it?
We treat qemu as untrusted, in case a guest escapes qemu due to some other CVE. If a guest really did cause qemu to emit unbounded QMP text, and it starves libvirtd, then that guest has mounted a denial of service against anything else libvirtd is starved from doing. So yes, in my opinion it is a CVE, even if it is an unlikely case because it won't trigger without a flaw in more than one layer. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Michal Privoznik