[QEMU,for-4.7] main loop: Big hammer to fix logfile disk DoS in Xen setups

Commit Message

Wei Liu May 26, 2016, 3:21 p.m. UTC

From: Ian Jackson <[e-mail]>

Each time round the main loop, we now fstat stderr.  If it is too big,
we dup2 /dev/null onto it.  This is not a very pretty patch but it is
very simple, easy to see that it's correct, and has a low risk of
collateral damage.

The limit is 1Mby by default but can be adjusted by setting a new
environment variable.

This fixes CVE-2014-3672.

Signed-off-by: Ian Jackson <[e-mail]>
Tested-by: Ian Jackson <[e-mail]>

Set the default to 0 so that it won't affect non-xen installation. The
limit will be set by Xen toolstack.

Signed-off-by: Wei Liu <[e-mail]>
---
 main-loop.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

Comments

George Dunlap May 26, 2016, 3:36 p.m. UTC | #1

On Thu, May 26, 2016 at 4:21 PM, Wei Liu <[e-mail]> wrote:
> From: Ian Jackson <[e-mail]>
>
> Each time round the main loop, we now fstat stderr.  If it is too big,
> we dup2 /dev/null onto it.  This is not a very pretty patch but it is
> very simple, easy to see that it's correct, and has a low risk of
> collateral damage.
>
> The limit is 1Mby by default but can be adjusted by setting a new
> environment variable.

There is no limit by default; a companion patch for libxl sets this to
1MiB.  This is so that this patch may be applied on a system qemu
which is shared between multiple use cases (including non-Xen cases).

 -George

Wei Liu May 26, 2016, 3:39 p.m. UTC | #2

On Thu, May 26, 2016 at 04:36:57PM +0100, George Dunlap wrote:
> On Thu, May 26, 2016 at 4:21 PM, Wei Liu <[e-mail]> wrote:
> > From: Ian Jackson <[e-mail]>
> >
> > Each time round the main loop, we now fstat stderr.  If it is too big,
> > we dup2 /dev/null onto it.  This is not a very pretty patch but it is
> > very simple, easy to see that it's correct, and has a low risk of
> > collateral damage.
> >
> > The limit is 1Mby by default but can be adjusted by setting a new
> > environment variable.
> 
> There is no limit by default; a companion patch for libxl sets this to
> 1MiB.  This is so that this patch may be applied on a system qemu
> which is shared between multiple use cases (including non-Xen cases).
> 

I added a paragraph in that patch and have my S-o-B.

Did you miss that? Or do you mean I should rewrite the commit message
altogether?

Wei.

>  -George

Ian Jackson May 26, 2016, 3:42 p.m. UTC | #3

Wei Liu writes ("[PATCH QEMU for-4.7] main loop: Big hammer to fix logfile disk DoS in Xen setups"):
> The limit is 1Mby by default but can be adjusted by setting a new
> environment variable.
> 
> This fixes CVE-2014-3672.
> 
> Signed-off-by: Ian Jackson <[e-mail]>
> Tested-by: Ian Jackson <[e-mail]>
> 
> Set the default to 0 so that it won't affect non-xen installation. The
> limit will be set by Xen toolstack.

Maybe the commit message, earlier, could be adjusted ?  But otherwise

Acked-by: Ian Jackson <[e-mail]>

Anthony PERARD May 26, 2016, 4:10 p.m. UTC | #4

On Thu, May 26, 2016 at 04:21:56PM +0100, Wei Liu wrote:
> From: Ian Jackson <[e-mail]>
> 
> Each time round the main loop, we now fstat stderr.  If it is too big,
> we dup2 /dev/null onto it.  This is not a very pretty patch but it is
> very simple, easy to see that it's correct, and has a low risk of
> collateral damage.
> 
> The limit is 1Mby by default but can be adjusted by setting a new
> environment variable.
> 
> This fixes CVE-2014-3672.
> 
> Signed-off-by: Ian Jackson <[e-mail]>
> Tested-by: Ian Jackson <[e-mail]>
> 
> Set the default to 0 so that it won't affect non-xen installation. The
> limit will be set by Xen toolstack.
> 
> Signed-off-by: Wei Liu <[e-mail]>

Beside the confusing commit message and the call in the WIN32 specific
mainloop, the patch look good. So,

Acked-by: Anthony PERARD <[e-mail]>

> ---
>  main-loop.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
> 
> diff --git a/main-loop.c b/main-loop.c
> index 3997043..aa32f5b 100644
> --- a/main-loop.c
> +++ b/main-loop.c
> @@ -164,6 +164,50 @@ int qemu_init_main_loop(Error **errp)
>      return 0;
>  }
>  
> +static void check_cve_2014_3672_xen(void)
> +{
> +    static unsigned long limit = ~0UL;
> +    const int fd = 2;
> +    struct stat stab;
> +
> +    if (limit == ~0UL) {
> +        const char *s = getenv("XEN_QEMU_CONSOLE_LIMIT");
> +        /* XEN_QEMU_CONSOLE_LIMIT=0 means no limit */
> +        limit = s ? strtoul(s,0,0) : 0;
> +    }
> +    if (limit == 0)
> +        return;
> +
> +    int r = fstat(fd, &stab);
> +    if (r) {
> +        perror("fstat stderr (for CVE-2014-3672 check)");
> +        exit(-1);
> +    }
> +    if (!S_ISREG(stab.st_mode))
> +        return;
> +    if (stab.st_size <= limit)
> +        return;
> +
> +    /* oh dear */
> +    fprintf(stderr,"\r\n"
> +            "Closing stderr due to CVE-2014-3672 limit. "
> +            " Set XEN_QEMU_CONSOLE_LIMIT to number of bytes to override,"
> +            " or 0 for no limit.\n");
> +    fflush(stderr);
> +
> +    int nfd = open("/dev/null", O_WRONLY);
> +    if (nfd < 0) {
> +        perror("open /dev/null (for CVE-2014-3672 check)");
> +        exit(-1);
> +    }
> +    r = dup2(nfd, fd);
> +    if (r != fd) {
> +        perror("dup2 /dev/null (for CVE-2014-3672 check)");
> +        exit(-1);
> +    }
> +    close(nfd);
> +}
> +
>  static int max_priority;
>  
>  #ifndef _WIN32
> @@ -216,6 +260,8 @@ static int os_host_main_loop_wait(int64_t timeout)
>      int ret;
>      static int spin_counter;
>  
> +    check_cve_2014_3672_xen();
> +
>      glib_pollfds_fill(&timeout);
>  
>      /* If the I/O thread is very busy or we are incorrectly busy waiting in
> @@ -407,6 +453,8 @@ static int os_host_main_loop_wait(int64_t timeout)
>      fd_set rfds, wfds, xfds;
>      int nfds;
>  
> +    check_cve_2014_3672_xen();
> +

That's the call within an #ifdef _WIN32.

>      /* XXX: need to suppress polling by better using win32 events */
>      ret = 0;
>      for (pe = first_polling_entry; pe != NULL; pe = pe->next) {

Anthony PERARD May 26, 2016, 4:50 p.m. UTC | #5

On Thu, May 26, 2016 at 05:10:52PM +0100, Anthony PERARD wrote:
> On Thu, May 26, 2016 at 04:21:56PM +0100, Wei Liu wrote:
> > From: Ian Jackson <[e-mail]>
> > 
> > Each time round the main loop, we now fstat stderr.  If it is too big,
> > we dup2 /dev/null onto it.  This is not a very pretty patch but it is
> > very simple, easy to see that it's correct, and has a low risk of
> > collateral damage.
> > 
> > The limit is 1Mby by default but can be adjusted by setting a new
> > environment variable.
> > 
> > This fixes CVE-2014-3672.
> > 
> > Signed-off-by: Ian Jackson <[e-mail]>
> > Tested-by: Ian Jackson <[e-mail]>
> > 
> > Set the default to 0 so that it won't affect non-xen installation. The
> > limit will be set by Xen toolstack.
> > 
> > Signed-off-by: Wei Liu <[e-mail]>
> 
> Beside the confusing commit message and the call in the WIN32 specific
> mainloop, the patch look good. So,
> 
> Acked-by: Anthony PERARD <[e-mail]>

I have ajusted the commit message with
`s/The limit is 1Mby/There is no limit/` and going to push to staging.

Patch

diff --git a/main-loop.c b/main-loop.c
index 3997043..aa32f5b 100644
--- a/main-loop.c
+++ b/main-loop.c
@@ -164,6 +164,50 @@  int qemu_init_main_loop(Error **errp)
     return 0;
 }
 
+static void check_cve_2014_3672_xen(void)
+{
+    static unsigned long limit = ~0UL;
+    const int fd = 2;
+    struct stat stab;
+
+    if (limit == ~0UL) {
+        const char *s = getenv("XEN_QEMU_CONSOLE_LIMIT");
+        /* XEN_QEMU_CONSOLE_LIMIT=0 means no limit */
+        limit = s ? strtoul(s,0,0) : 0;
+    }
+    if (limit == 0)
+        return;
+
+    int r = fstat(fd, &stab);
+    if (r) {
+        perror("fstat stderr (for CVE-2014-3672 check)");
+        exit(-1);
+    }
+    if (!S_ISREG(stab.st_mode))
+        return;
+    if (stab.st_size <= limit)
+        return;
+
+    /* oh dear */
+    fprintf(stderr,"\r\n"
+            "Closing stderr due to CVE-2014-3672 limit. "
+            " Set XEN_QEMU_CONSOLE_LIMIT to number of bytes to override,"
+            " or 0 for no limit.\n");
+    fflush(stderr);
+
+    int nfd = open("/dev/null", O_WRONLY);
+    if (nfd < 0) {
+        perror("open /dev/null (for CVE-2014-3672 check)");
+        exit(-1);
+    }
+    r = dup2(nfd, fd);
+    if (r != fd) {
+        perror("dup2 /dev/null (for CVE-2014-3672 check)");
+        exit(-1);
+    }
+    close(nfd);
+}
+
 static int max_priority;
 
 #ifndef _WIN32
@@ -216,6 +260,8 @@  static int os_host_main_loop_wait(int64_t timeout)
     int ret;
     static int spin_counter;
 
+    check_cve_2014_3672_xen();
+
     glib_pollfds_fill(&timeout);
 
     /* If the I/O thread is very busy or we are incorrectly busy waiting in
@@ -407,6 +453,8 @@  static int os_host_main_loop_wait(int64_t timeout)
     fd_set rfds, wfds, xfds;
     int nfds;
 
+    check_cve_2014_3672_xen();
+
     /* XXX: need to suppress polling by better using win32 events */
     ret = 0;
     for (pe = first_polling_entry; pe != NULL; pe = pe->next) {