qemu: avoid denial of service reading from QEMU monitor (CVE-2018-xxxx)

[libvirt] [PATCH] qemu: avoid denial of service reading from QEMU monitor (CVE-2018-xxxx)

Posted by Daniel P. Berrange 6 years, 3 months ago

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu: avoid denial of service reading from QEMU monitor (CVE-2018-xxxx)

Posted by Michal Privoznik 6 years, 3 months ago

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu: avoid denial of service reading from QEMU monitor (CVE-2018-xxxx)

Posted by Daniel P. Berrange 6 years, 3 months ago

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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu: avoid denial of service reading from QEMU monitor (CVE-2018-xxxx)

Posted by Eric Blake 6 years, 3 months ago

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list