[Qemu-devel] [PATCH] qga: set umask 0077 when daemonizing (CVE-2013-2007)

[Qemu-devel] [PATCH] qga: set umask 0077 when daemonizing (CVE-2013-2007)

[Anthony Liguori]

From: Laszlo Ersek <lersek@redhat.com>

The qemu guest agent creates a bunch of files with insecure permissions
when started in daemon mode. For example:

  -rw-rw-rw- 1 root root /var/log/qemu-ga.log
  -rw-rw-rw- 1 root root /var/run/qga.state
  -rw-rw-rw- 1 root root /var/log/qga-fsfreeze-hook.log

In addition, at least all files created with the "guest-file-open" QMP
command, and all files created with shell output redirection (or
otherwise) by utilities invoked by the fsfreeze hook script are affected.

For now mask all file mode bits for "group" and "others" in
become_daemon().

Temporarily, for compatibility reasons, stick with the 0666 file-mode in
case of files newly created by the "guest-file-open" QMP call. Do so
without changing the umask temporarily.

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 qga/commands-posix.c | 123 +++++++++++++++++++++++++++++++++++++++++++++++++--
 qga/main.c           |   2 +-
 2 files changed, 120 insertions(+), 5 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 3b5c536..04c6951 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -18,6 +18,9 @@
 #include <unistd.h>
 #include <errno.h>
 #include <fcntl.h>
+#include <stdio.h>
+#include <string.h>
+#include <sys/stat.h>
 #include <inttypes.h>
 #include "qga/guest-agent-core.h"
 #include "qga-qmp-commands.h"
@@ -237,9 +240,122 @@ static GuestFileHandle *guest_file_handle_find(int64_t id, Error **err)
     return NULL;
 }
 
+typedef const char * const ccpc;
+
+/* http://pubs.opengroup.org/onlinepubs/9699919799/functions/fopen.html */
+static const struct {
+    ccpc *forms;
+    int oflag_base;
+} guest_file_open_modes[] = {
+    { (ccpc[]){ "r",  "rb",         NULL }, O_RDONLY                      },
+    { (ccpc[]){ "w",  "wb",         NULL }, O_WRONLY | O_CREAT | O_TRUNC  },
+    { (ccpc[]){ "a",  "ab",         NULL }, O_WRONLY | O_CREAT | O_APPEND },
+    { (ccpc[]){ "r+", "rb+", "r+b", NULL }, O_RDWR                        },
+    { (ccpc[]){ "w+", "wb+", "w+b", NULL }, O_RDWR   | O_CREAT | O_TRUNC  },
+    { (ccpc[]){ "a+", "ab+", "a+b", NULL }, O_RDWR   | O_CREAT | O_APPEND }
+};
+
+static int
+find_open_flag(const char *mode_str, Error **err)
+{
+    unsigned mode;
+
+    for (mode = 0; mode < ARRAY_SIZE(guest_file_open_modes); ++mode) {
+        ccpc *form;
+
+        form = guest_file_open_modes[mode].forms;
+        while (*form != NULL && strcmp(*form, mode_str) != 0) {
+            ++form;
+        }
+        if (*form != NULL) {
+            break;
+        }
+    }
+
+    if (mode == ARRAY_SIZE(guest_file_open_modes)) {
+        error_setg(err, "invalid file open mode '%s'", mode_str);
+        return -1;
+    }
+    return guest_file_open_modes[mode].oflag_base | O_NOCTTY | O_NONBLOCK;
+}
+
+#define DEFAULT_NEW_FILE_MODE (S_IRUSR | S_IWUSR | \
+                               S_IRGRP | S_IWGRP | \
+                               S_IROTH | S_IWOTH)
+
+static FILE *
+safe_open_or_create(const char *path, const char *mode, Error **err)
+{
+    Error *local_err = NULL;
+    int oflag;
+
+    oflag = find_open_flag(mode, &local_err);
+    if (local_err == NULL) {
+        int fd;
+
+        /* If the caller wants / allows creation of a new file, we implement it
+         * with a two step process: open() + (open() / fchmod()).
+         *
+         * First we insist on creating the file exclusively as a new file. If
+         * that succeeds, we're free to set any file-mode bits on it. (The
+         * motivation is that we want to set those file-mode bits independently
+         * of the current umask.)
+         *
+         * If the exclusive creation fails because the file already exists
+         * (EEXIST is not possible for any other reason), we just attempt to
+         * open the file, but in this case we won't be allowed to change the
+         * file-mode bits on the preexistent file.
+         *
+         * The pathname should never disappear between the two open()s in
+         * practice. If it happens, then someone very likely tried to race us.
+         * In this case just go ahead and report the ENOENT from the second
+         * open() to the caller.
+         *
+         * If the caller wants to open a preexistent file, then the first
+         * open() is decisive and its third argument is ignored, and the second
+         * open() and the fchmod() are never called.
+         */
+        fd = open(path, oflag | ((oflag & O_CREAT) ? O_EXCL : 0), 0);
+        if (fd == -1 && errno == EEXIST) {
+            oflag &= ~(unsigned)O_CREAT;
+            fd = open(path, oflag);
+        }
+
+        if (fd == -1) {
+            error_setg_errno(&local_err, errno, "failed to open file '%s' "
+                             "(mode: '%s')", path, mode);
+        } else {
+            qemu_set_cloexec(fd);
+
+            if ((oflag & O_CREAT) && fchmod(fd, DEFAULT_NEW_FILE_MODE) == -1) {
+                error_setg_errno(&local_err, errno, "failed to set permission "
+                                 "0%03o on new file '%s' (mode: '%s')",
+                                 (unsigned)DEFAULT_NEW_FILE_MODE, path, mode);
+            } else {
+                FILE *f;
+
+                f = fdopen(fd, mode);
+                if (f == NULL) {
+                    error_setg_errno(&local_err, errno, "failed to associate "
+                                     "stdio stream with file descriptor %d, "
+                                     "file '%s' (mode: '%s')", fd, path, mode);
+                } else {
+                    return f;
+                }
+            }
+
+            close(fd);
+        }
+    }
+
+    error_propagate(err, local_err);
+    return NULL;
+}
+
 int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode, Error **err)
 {
     FILE *fh;
+    Error *local_err = NULL;
     int fd;
     int64_t ret = -1, handle;
 
@@ -247,10 +363,9 @@ int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode, E
         mode = "r";
     }
     slog("guest-file-open called, filepath: %s, mode: %s", path, mode);
-    fh = fopen(path, mode);
-    if (!fh) {
-        error_setg_errno(err, errno, "failed to open file '%s' (mode: '%s')",
-                         path, mode);
+    fh = safe_open_or_create(path, mode, &local_err);
+    if (local_err != NULL) {
+        error_propagate(err, local_err);
         return -1;
     }
 
diff --git a/qga/main.c b/qga/main.c
index 1841759..44a2836 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -478,7 +478,7 @@ static void become_daemon(const char *pidfile)
         }
     }
 
-    umask(0);
+    umask(S_IRWXG | S_IRWXO);
     sid = setsid();
     if (sid < 0) {
         goto fail;
-- 
1.8.0

Re: [Qemu-devel] [PATCH] qga: set umask 0077 when daemonizing (CVE-2013-2007)

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 3541 bytes --]

On 05/07/2013 05:47 AM, Anthony Liguori wrote:
> From: Laszlo Ersek <lersek@redhat.com>
> 
> The qemu guest agent creates a bunch of files with insecure permissions
> when started in daemon mode. For example:
> 
>   -rw-rw-rw- 1 root root /var/log/qemu-ga.log
>   -rw-rw-rw- 1 root root /var/run/qga.state
>   -rw-rw-rw- 1 root root /var/log/qga-fsfreeze-hook.log
> 
> In addition, at least all files created with the "guest-file-open" QMP
> command, and all files created with shell output redirection (or
> otherwise) by utilities invoked by the fsfreeze hook script are affected.
> 
> For now mask all file mode bits for "group" and "others" in
> become_daemon().
> 
> Temporarily, for compatibility reasons, stick with the 0666 file-mode in
> case of files newly created by the "guest-file-open" QMP call. Do so
> without changing the umask temporarily.

This points out that:

1. the documentation for guest-file-open is insufficient to describe our
limitations (for example, although C11 requires support for the "wx"
flag, this patch forbids that flag)

2. guest-file-open is rather puny; we may need a newer interface that
allows the user finer-grained control over the actual mode bits set on
the file that they are creating (and maybe something similar to openat()
that would let the user open/create a file relative to an existing
handle to a directory, rather than always having to specify an absolute
path).

But I agree that _this_ patch fixes the CVE, and that any further
changes to resolve the above two issues are not essential to include in 1.5.

> +/* http://pubs.opengroup.org/onlinepubs/9699919799/functions/fopen.html */
> +static const struct {
> +    ccpc *forms;
> +    int oflag_base;
> +} guest_file_open_modes[] = {
> +    { (ccpc[]){ "r",  "rb",         NULL }, O_RDONLY                      },

You are mapping things according to their POSIX definition (POSIX, as an
additional requirement above and beyond C99, states that presence or
absence of 'b' is a no-op because there is no difference between binary
and text mode).  But C99 permits a distinct binary mode, and qga is
compiled for Windows where binary mode is indeed different.

I think that you probably should split this map into:

    { (ccpc[]){ "r",         NULL }, O_RDONLY                      },
    { (ccpc[]){ "rb",        NULL }, O_RDONLY | O_BINARY           },

and so forth (assuming that O_BINARY is properly #defined to 0 on
POSIX-y systems that don't need it), so that you don't regress the qga
server in a Windows guest where the management client is explicitly
passing or omitting 'b' for the intentional difference between text and
binary files.  [1]

> +
> +            if ((oflag & O_CREAT) && fchmod(fd, DEFAULT_NEW_FILE_MODE) == -1) {
> +                error_setg_errno(&local_err, errno, "failed to set permission "
> +                                 "0%03o on new file '%s' (mode: '%s')",
> +                                 (unsigned)DEFAULT_NEW_FILE_MODE, path, mode);

On this particular error path, we've left behind an empty mode 0000
file.  Is it worth trying to unlink() the dead file?

> +            } else {
> +                FILE *f;
> +
> +                f = fdopen(fd, mode);

[1] I don't know if Windows is okay using fdopen() to create a FILE in
binary mode if you didn't match O_BINARY on the fd underlying the FILE.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

[Qemu-devel] [PATCH 1/2] qga: distinguish binary modes in "guest_file_open_modes" map

[Laszlo Ersek]

In Windows guests this may make a difference.

Suggested-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 qga/commands-posix.c |   22 ++++++++++++++++------
 1 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 04c6951..2eec712 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -242,17 +242,27 @@ static GuestFileHandle *guest_file_handle_find(int64_t id, Error **err)
 
 typedef const char * const ccpc;
 
+#ifndef O_BINARY
+#define O_BINARY 0
+#endif
+
 /* http://pubs.opengroup.org/onlinepubs/9699919799/functions/fopen.html */
 static const struct {
     ccpc *forms;
     int oflag_base;
 } guest_file_open_modes[] = {
-    { (ccpc[]){ "r",  "rb",         NULL }, O_RDONLY                      },
-    { (ccpc[]){ "w",  "wb",         NULL }, O_WRONLY | O_CREAT | O_TRUNC  },
-    { (ccpc[]){ "a",  "ab",         NULL }, O_WRONLY | O_CREAT | O_APPEND },
-    { (ccpc[]){ "r+", "rb+", "r+b", NULL }, O_RDWR                        },
-    { (ccpc[]){ "w+", "wb+", "w+b", NULL }, O_RDWR   | O_CREAT | O_TRUNC  },
-    { (ccpc[]){ "a+", "ab+", "a+b", NULL }, O_RDWR   | O_CREAT | O_APPEND }
+    { (ccpc[]){ "r",          NULL }, O_RDONLY                                 },
+    { (ccpc[]){ "rb",         NULL }, O_RDONLY                      | O_BINARY },
+    { (ccpc[]){ "w",          NULL }, O_WRONLY | O_CREAT | O_TRUNC             },
+    { (ccpc[]){ "wb",         NULL }, O_WRONLY | O_CREAT | O_TRUNC  | O_BINARY },
+    { (ccpc[]){ "a",          NULL }, O_WRONLY | O_CREAT | O_APPEND            },
+    { (ccpc[]){ "ab",         NULL }, O_WRONLY | O_CREAT | O_APPEND | O_BINARY },
+    { (ccpc[]){ "r+",         NULL }, O_RDWR                                   },
+    { (ccpc[]){ "rb+", "r+b", NULL }, O_RDWR                        | O_BINARY },
+    { (ccpc[]){ "w+",         NULL }, O_RDWR   | O_CREAT | O_TRUNC             },
+    { (ccpc[]){ "wb+", "w+b", NULL }, O_RDWR   | O_CREAT | O_TRUNC  | O_BINARY },
+    { (ccpc[]){ "a+",         NULL }, O_RDWR   | O_CREAT | O_APPEND            },
+    { (ccpc[]){ "ab+", "a+b", NULL }, O_RDWR   | O_CREAT | O_APPEND | O_BINARY }
 };
 
 static int
-- 
1.7.1

[Qemu-devel] [PATCH 2/2] qga: try to unlink just created guest-file if fchmod() fails on it

[Laszlo Ersek]

We shouldn't allow guest filesystem pollution on error paths.

Suggested-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 qga/commands-posix.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 2eec712..d301b1f 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -341,6 +341,7 @@ safe_open_or_create(const char *path, const char *mode, Error **err)
                 error_setg_errno(&local_err, errno, "failed to set permission "
                                  "0%03o on new file '%s' (mode: '%s')",
                                  (unsigned)DEFAULT_NEW_FILE_MODE, path, mode);
+                unlink(path);
             } else {
                 FILE *f;
 
-- 
1.7.1

Re: [Qemu-devel] [PATCH 1/2] qga: distinguish binary modes in "guest_file_open_modes" map

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 471 bytes --]

On 05/07/2013 10:56 AM, Laszlo Ersek wrote:
> In Windows guests this may make a difference.
> 
> Suggested-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  qga/commands-posix.c |   22 ++++++++++++++++------
>  1 files changed, 16 insertions(+), 6 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH 1/2] qga: distinguish binary modes in "guest_file_open_modes" map

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1217 bytes --]

On 05/07/2013 10:56 AM, Laszlo Ersek wrote:
> In Windows guests this may make a difference.
> 
> Suggested-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  qga/commands-posix.c |   22 ++++++++++++++++------
>  1 files changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 04c6951..2eec712 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c

Oh, and only NOW do I notice that this is in a file named
commands-posix.c that doesn't get compiled into the Windows build of qga
(there, we only build commands-win32.c, and THAT file always fails this
command, because no one has ported guest-file-open there yet).  But I
guess there is still the argument that some weirdnix system exists that
isn't quite POSIX compliant and does have a distinct binary mode (maybe
someone plans on compiling qga for Cygwin instead of native windows,
since at least that would be able to open files when the
commands-win32.c variant doesn't?), so I still think the patch is worth
keeping.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH 2/2] qga: try to unlink just created guest-file if fchmod() fails on it

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1696 bytes --]

On 05/07/2013 10:56 AM, Laszlo Ersek wrote:
> We shouldn't allow guest filesystem pollution on error paths.
> 
> Suggested-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  qga/commands-posix.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 2eec712..d301b1f 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -341,6 +341,7 @@ safe_open_or_create(const char *path, const char *mode, Error **err)
>                  error_setg_errno(&local_err, errno, "failed to set permission "
>                                   "0%03o on new file '%s' (mode: '%s')",
>                                   (unsigned)DEFAULT_NEW_FILE_MODE, path, mode);
> +                unlink(path);

This fixes the case of a mode 0000 file if fchmod fails, but doesn't fix
the case of a mode 0666 file if fchmod succeeds but fdopen fails.  It
also requires that unlink() while open works (true for most Unix
systems, but false for Windows systems and not required by POSIX - but
see my realization on 1/2 that this file isn't compiled on Windows). I
think you want this instead:

diff --git i/qga/commands-posix.c w/qga/commands-posix.c
index 04c6951..89cc6d8 100644
--- i/qga/commands-posix.c
+++ w/qga/commands-posix.c
@@ -345,6 +345,9 @@ safe_open_or_create(const char *path, const char
*mode, Error **err)
             }

             close(fd);
+	    if (oflag & O_CREAT) {
+	        unlink(path);
+	    }
         }
     }

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH] qga: set umask 0077 when daemonizing (CVE-2013-2007)

[Anthony Liguori]

Applied.  Thanks.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 1/2] qga: distinguish binary modes in "guest_file_open_modes" map

[Peter Maydell]

On 7 May 2013 18:27, Eric Blake <eblake@redhat.com> wrote:
> On 05/07/2013 10:56 AM, Laszlo Ersek wrote:
>> In Windows guests this may make a difference.
>>
>> Suggested-by: Eric Blake <eblake@redhat.com>
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>

> Oh, and only NOW do I notice that this is in a file named
> commands-posix.c that doesn't get compiled into the Windows build of qga
> (there, we only build commands-win32.c, and THAT file always fails this
> command, because no one has ported guest-file-open there yet).  But I
> guess there is still the argument that some weirdnix system exists that
> isn't quite POSIX compliant and does have a distinct binary mode (maybe
> someone plans on compiling qga for Cygwin instead of native windows,
> since at least that would be able to open files when the
> commands-win32.c variant doesn't?), so I still think the patch is worth
> keeping.

If we accept this we should at a minimum update the
commit message to say that we're doing this for
theoretical completeness rather than any practical reason.
(Not that "this may make a difference" is a particularly
convincing commit message in the first place :-))

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/2] qga: distinguish binary modes in "guest_file_open_modes" map

[mdroth]

On Tue, May 07, 2013 at 11:27:03AM -0600, Eric Blake wrote:
> On 05/07/2013 10:56 AM, Laszlo Ersek wrote:
> > In Windows guests this may make a difference.
> > 
> > Suggested-by: Eric Blake <eblake@redhat.com>
> > Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> > ---
> >  qga/commands-posix.c |   22 ++++++++++++++++------
> >  1 files changed, 16 insertions(+), 6 deletions(-)
> > 
> > diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> > index 04c6951..2eec712 100644
> > --- a/qga/commands-posix.c
> > +++ b/qga/commands-posix.c
> 
> Oh, and only NOW do I notice that this is in a file named
> commands-posix.c that doesn't get compiled into the Windows build of qga
> (there, we only build commands-win32.c, and THAT file always fails this
> command, because no one has ported guest-file-open there yet).  But I
> guess there is still the argument that some weirdnix system exists that
> isn't quite POSIX compliant and does have a distinct binary mode (maybe
> someone plans on compiling qga for Cygwin instead of native windows,
> since at least that would be able to open files when the
> commands-win32.c variant doesn't?), so I still think the patch is worth
> keeping.

FWIW, I have some rough patches for w32 implementations of guest-file-*
commands queued up that I'll be cleaning up for 1.6, so it's not so much
theoretical as just a tad early :)

> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 

Re: [Qemu-devel] [PATCH] qga: set umask 0077 when daemonizing (CVE-2013-2007)

[mdroth]

On Tue, May 07, 2013 at 09:55:06AM -0600, Eric Blake wrote:
> On 05/07/2013 05:47 AM, Anthony Liguori wrote:
> > From: Laszlo Ersek <lersek@redhat.com>
> > 
> > The qemu guest agent creates a bunch of files with insecure permissions
> > when started in daemon mode. For example:
> > 
> >   -rw-rw-rw- 1 root root /var/log/qemu-ga.log
> >   -rw-rw-rw- 1 root root /var/run/qga.state
> >   -rw-rw-rw- 1 root root /var/log/qga-fsfreeze-hook.log
> > 
> > In addition, at least all files created with the "guest-file-open" QMP
> > command, and all files created with shell output redirection (or
> > otherwise) by utilities invoked by the fsfreeze hook script are affected.
> > 
> > For now mask all file mode bits for "group" and "others" in
> > become_daemon().
> > 
> > Temporarily, for compatibility reasons, stick with the 0666 file-mode in
> > case of files newly created by the "guest-file-open" QMP call. Do so
> > without changing the umask temporarily.
> 
> This points out that:
> 
> 1. the documentation for guest-file-open is insufficient to describe our
> limitations (for example, although C11 requires support for the "wx"
> flag, this patch forbids that flag)

Got a pointer? I can fix up the docs if need be, but fopen() docs that
qemu-ga points at seem to indicate 0666 will be used for new files.
That's no longer the case?

> 
> 2. guest-file-open is rather puny; we may need a newer interface that
> allows the user finer-grained control over the actual mode bits set on

Yes, shame it wasn't there at the start. a guest-file-open-full or
something with support for specifying creation mode will have to do it.

> the file that they are creating (and maybe something similar to openat()
> that would let the user open/create a file relative to an existing
> handle to a directory, rather than always having to specify an absolute
> path).
> 
> But I agree that _this_ patch fixes the CVE, and that any further
> changes to resolve the above two issues are not essential to include in 1.5.
> 
> > +/* http://pubs.opengroup.org/onlinepubs/9699919799/functions/fopen.html */
> > +static const struct {
> > +    ccpc *forms;
> > +    int oflag_base;
> > +} guest_file_open_modes[] = {
> > +    { (ccpc[]){ "r",  "rb",         NULL }, O_RDONLY                      },
> 
> You are mapping things according to their POSIX definition (POSIX, as an
> additional requirement above and beyond C99, states that presence or
> absence of 'b' is a no-op because there is no difference between binary
> and text mode).  But C99 permits a distinct binary mode, and qga is
> compiled for Windows where binary mode is indeed different.
> 
> I think that you probably should split this map into:
> 
>     { (ccpc[]){ "r",         NULL }, O_RDONLY                      },
>     { (ccpc[]){ "rb",        NULL }, O_RDONLY | O_BINARY           },
> 
> and so forth (assuming that O_BINARY is properly #defined to 0 on
> POSIX-y systems that don't need it), so that you don't regress the qga
> server in a Windows guest where the management client is explicitly
> passing or omitting 'b' for the intentional difference between text and
> binary files.  [1]
> 
> > +
> > +            if ((oflag & O_CREAT) && fchmod(fd, DEFAULT_NEW_FILE_MODE) == -1) {
> > +                error_setg_errno(&local_err, errno, "failed to set permission "
> > +                                 "0%03o on new file '%s' (mode: '%s')",
> > +                                 (unsigned)DEFAULT_NEW_FILE_MODE, path, mode);
> 
> On this particular error path, we've left behind an empty mode 0000
> file.  Is it worth trying to unlink() the dead file?
> 
> > +            } else {
> > +                FILE *f;
> > +
> > +                f = fdopen(fd, mode);
> 
> [1] I don't know if Windows is okay using fdopen() to create a FILE in
> binary mode if you didn't match O_BINARY on the fd underlying the FILE.
> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 

Re: [Qemu-devel] [PATCH] qga: set umask 0077 when daemonizing (CVE-2013-2007)

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2643 bytes --]

On 05/07/2013 02:28 PM, mdroth wrote:
>>
>> This points out that:
>>
>> 1. the documentation for guest-file-open is insufficient to describe our
>> limitations (for example, although C11 requires support for the "wx"
>> flag, this patch forbids that flag)
> 
> Got a pointer? I can fix up the docs if need be, but fopen() docs that
> qemu-ga points at seem to indicate 0666 will be used for new files.
> That's no longer the case?

C99 and C11 don't care about permission bits of files created by fopen()
- that's a concept added by POSIX.  POSIX says that fopen() creates new
files with respect to the current umask settings (in other words, 0666
minus bits that umask forbids).  But since we weren't forbidding any
bits, that meant we were getting 0666 files.
http://pubs.opengroup.org/onlinepubs/9699919799/functions/fopen.html

This patch intentionally leaves things unchanged so that any file
created via guest-file-open still has mode 0666, just as it was
pre-patch (it's just that the mode bits are now set via fchmod instead
of implied via a umask of 0).

But pre-patch, we handed any string on to fopen (whether bogus, such as
"read", or valid C11, such as "wx", or even glibc extension, such as
"we") and whether it succeeded or failed was dependent on the extensions
supported in the version of libc running the guest agent.  Now
post-patch, we only accept a finite set of mode strings (those
documented in C99) and explicitly reject others, even if they used to
succeed.

The documentation in qga/qapi-schema.json doesn't mention anything about
the permissions given to created files (other than what you can infer by
chasing down the POSIX rather than the C99 definition of fopen), and it
only says that @mode is as per fopen() without saying whether it is C99
or C11 fopen().

> 
>>
>> 2. guest-file-open is rather puny; we may need a newer interface that
>> allows the user finer-grained control over the actual mode bits set on
> 
> Yes, shame it wasn't there at the start. a guest-file-open-full or
> something with support for specifying creation mode will have to do it.

It boils down to fopen() mode argument being puny when compared to
open()'s bit flags and optional mode_t argument.  We inherently limited
ourselves by designing after the higher-level C interface instead of the
lower-level POSIX interface.  So yes, hopefully when we design a new
more powerful qga command, we will put more thought into designing it to
do everything that we really want.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH 2/2] qga: try to unlink just created guest-file if fchmod() fails on it

[Laszlo Ersek]

On 05/07/13 19:30, Eric Blake wrote:
> On 05/07/2013 10:56 AM, Laszlo Ersek wrote:
>> We shouldn't allow guest filesystem pollution on error paths.
>>
>> Suggested-by: Eric Blake <eblake@redhat.com>
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>  qga/commands-posix.c |    1 +
>>  1 files changed, 1 insertions(+), 0 deletions(-)
>>
>> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
>> index 2eec712..d301b1f 100644
>> --- a/qga/commands-posix.c
>> +++ b/qga/commands-posix.c
>> @@ -341,6 +341,7 @@ safe_open_or_create(const char *path, const char *mode, Error **err)
>>                  error_setg_errno(&local_err, errno, "failed to set permission "
>>                                   "0%03o on new file '%s' (mode: '%s')",
>>                                   (unsigned)DEFAULT_NEW_FILE_MODE, path, mode);
>> +                unlink(path);
> 
> This fixes the case of a mode 0000 file if fchmod fails, but doesn't fix
> the case of a mode 0666 file if fchmod succeeds but fdopen fails.  It
> also requires that unlink() while open works (true for most Unix
> systems, but false for Windows systems and not required by POSIX

Please provide a reference for "not required by POSIX". The EBUSY
condition I'm looking at says "The file named by the path argument
cannot be unlinked because it is being used by the system or another
process and the implementation considers this an error".

"Used by the system or another process" shouldn't really be the case here.

 - but
> see my realization on 1/2 that this file isn't compiled on Windows). I
> think you want this instead:
> 
> diff --git i/qga/commands-posix.c w/qga/commands-posix.c
> index 04c6951..89cc6d8 100644
> --- i/qga/commands-posix.c
> +++ w/qga/commands-posix.c
> @@ -345,6 +345,9 @@ safe_open_or_create(const char *path, const char
> *mode, Error **err)
>              }
> 
>              close(fd);
> +	    if (oflag & O_CREAT) {
> +	        unlink(path);
> +	    }
>          }
>      }
> 

I'll give myself some time on this. Clearly if I rush a patch I make a mess.

Thanks,
Laszlo

Re: [Qemu-devel] [PATCH] qga: set umask 0077 when daemonizing (CVE-2013-2007)

[Anthony Liguori]

Anthony Liguori <aliguori@us.ibm.com> writes:

> Applied.  Thanks.

Hi,

This was an automated response so it doesn't acknowledge the fact that
since this was a CVE, I applied the original patch regardless of review
feedback to avoid any confusion about whether the CVE has been addressed.

In the past, we've modified the patches published with CVEs because of
feedback on the list and this creates tremendous confusion.  This even
resulted in a distro including an incorrect patches because they
mistakenly thought a CVE wasn't addressed.

Please do review and provide feedback for this patch and we'll
incorporate that in follow-ups as Laszlo has already done.

And as usual, thanks to everyone involved in reporting, reviewing, and
coordinating the handling of this CVE!

Regards,

Anthony Liguori

>
> Regards,
>
> Anthony Liguori

Re: [Qemu-devel] [PATCH 2/2] qga: try to unlink just created guest-file if fchmod() fails on it

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2714 bytes --]

On 05/07/2013 06:35 PM, Laszlo Ersek wrote:
>> This fixes the case of a mode 0000 file if fchmod fails, but doesn't fix
>> the case of a mode 0666 file if fchmod succeeds but fdopen fails.  It
>> also requires that unlink() while open works (true for most Unix
>> systems, but false for Windows systems and not required by POSIX
> 
> Please provide a reference for "not required by POSIX". The EBUSY
> condition I'm looking at says "The file named by the path argument
> cannot be unlinked because it is being used by the system or another
> process and the implementation considers this an error".

That's precisely what I'm looking at.  The Windows implementation
considers any open file as in use by the system, and gives an error when
trying to unlink an open file (it also gives an error when trying to
rmdir a directory that serves as the current working directory of any
process).

Another case is NFS, which has historically had not-so-nice behavior if
you attempt to unlink an open file (such as the file still being
resident in cache for several seconds longer and preventing removal of a
directory), whereas closing first and then unlinking suffers from no
delay or weird caching effects (admittedly, NFS as a file system breaks
several POSIX rules).

You are right that POSIX unlink() also says "If one or more processes
have the file open when the last link is removed, the link shall be
removed before unlink() returns, but the removal of the file contents
shall be postponed until all references to the file are closed."  But I
read that as permitting (but not requiring) an implementation to unlink
an open fd, if it further follows the rules about the file contents when
unlinking an open file.  Meanwhile, the EBUSY error permits (but not
requires) implementations to let unlink fail on an open file, and NFS
serves as a case study of this behavior even on Unix-y systems.

Even if you don't buy my argument about EBUSY, and even though we both
agree that Unix-y systems have historically allowed unlink of an open
file on a local file system (as well as rmdir of the current working
directory), it still doesn't change the fact that in reality, such
actions are not portable to Windows or NFS.  And whether or not Windows
is non-compliant with POSIX doesn't matter quite as much as whether our
code is portable to the systems we are porting to.

> 
> "Used by the system or another process" shouldn't really be the case here.

Used by another process - no.  Used by the system - yes, if the system
considers all open files to be in use.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH] qga: set umask 0077 when daemonizing (CVE-2013-2007)

[Bruce Rogers]

 >>> On 5/7/2013 at 05:47 AM, Anthony Liguori <aliguori@us.ibm.com> wrote: 
> From: Laszlo Ersek <lersek@redhat.com>
> 
> The qemu guest agent creates a bunch of files with insecure permissions
> when started in daemon mode. For example:
> 
>   -rw-rw-rw- 1 root root /var/log/qemu-ga.log
>   -rw-rw-rw- 1 root root /var/run/qga.state
>   -rw-rw-rw- 1 root root /var/log/qga-fsfreeze-hook.log
> 
> In addition, at least all files created with the "guest-file-open" QMP
> command, and all files created with shell output redirection (or
> otherwise) by utilities invoked by the fsfreeze hook script are affected.
> 
> For now mask all file mode bits for "group" and "others" in
> become_daemon().
> 
> Temporarily, for compatibility reasons, stick with the 0666 file-mode in
> case of files newly created by the "guest-file-open" QMP call. Do so
> without changing the umask temporarily.
> 
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  qga/commands-posix.c | 123 +++++++++++++++++++++++++++++++++++++++++++++++++--
>  qga/main.c           |   2 +-
>  2 files changed, 120 insertions(+), 5 deletions(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 3b5c536..04c6951 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -18,6 +18,9 @@
>  #include <unistd.h>
>  #include <errno.h>
>  #include <fcntl.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <sys/stat.h>
>  #include <inttypes.h>
>  #include "qga/guest-agent-core.h"
>  #include "qga-qmp-commands.h"
> @@ -237,9 +240,122 @@ static GuestFileHandle *guest_file_handle_find(int64_t 
> id, Error **err)
>      return NULL;
>  }
>  
> +typedef const char * const ccpc;
> +
> +/* http://pubs.opengroup.org/onlinepubs/9699919799/functions/fopen.html */
> +static const struct {
> +    ccpc *forms;
> +    int oflag_base;
> +} guest_file_open_modes[] = {
> +    { (ccpc[]){ "r",  "rb",         NULL }, O_RDONLY                      
> },
> +    { (ccpc[]){ "w",  "wb",         NULL }, O_WRONLY | O_CREAT | O_TRUNC  
> },
> +    { (ccpc[]){ "a",  "ab",         NULL }, O_WRONLY | O_CREAT | O_APPEND 
> },
> +    { (ccpc[]){ "r+", "rb+", "r+b", NULL }, O_RDWR                        
> },
> +    { (ccpc[]){ "w+", "wb+", "w+b", NULL }, O_RDWR   | O_CREAT | O_TRUNC  
> },
> +    { (ccpc[]){ "a+", "ab+", "a+b", NULL }, O_RDWR   | O_CREAT | O_APPEND }
> +};
> +
> +static int
> +find_open_flag(const char *mode_str, Error **err)
> +{
> +    unsigned mode;
> +
> +    for (mode = 0; mode < ARRAY_SIZE(guest_file_open_modes); ++mode) {
> +        ccpc *form;
> +
> +        form = guest_file_open_modes[mode].forms;
> +        while (*form != NULL && strcmp(*form, mode_str) != 0) {
> +            ++form;
> +        }
> +        if (*form != NULL) {
> +            break;
> +        }
> +    }
> +
> +    if (mode == ARRAY_SIZE(guest_file_open_modes)) {
> +        error_setg(err, "invalid file open mode '%s'", mode_str);
> +        return -1;
> +    }
> +    return guest_file_open_modes[mode].oflag_base | O_NOCTTY | O_NONBLOCK;
> +}
> +
> +#define DEFAULT_NEW_FILE_MODE (S_IRUSR | S_IWUSR | \
> +                               S_IRGRP | S_IWGRP | \
> +                               S_IROTH | S_IWOTH)
> +
> +static FILE *
> +safe_open_or_create(const char *path, const char *mode, Error **err)
> +{
> +    Error *local_err = NULL;
> +    int oflag;
> +
> +    oflag = find_open_flag(mode, &local_err);
> +    if (local_err == NULL) {
> +        int fd;
> +
> +        /* If the caller wants / allows creation of a new file, we 
> implement it
> +         * with a two step process: open() + (open() / fchmod()).
> +         *
> +         * First we insist on creating the file exclusively as a new file. 
> If
> +         * that succeeds, we're free to set any file-mode bits on it. (The
> +         * motivation is that we want to set those file-mode bits 
> independently
> +         * of the current umask.)
> +         *
> +         * If the exclusive creation fails because the file already exists
> +         * (EEXIST is not possible for any other reason), we just attempt 
> to
> +         * open the file, but in this case we won't be allowed to change 
> the
> +         * file-mode bits on the preexistent file.
> +         *
> +         * The pathname should never disappear between the two open()s in
> +         * practice. If it happens, then someone very likely tried to race 
> us.
> +         * In this case just go ahead and report the ENOENT from the second
> +         * open() to the caller.
> +         *
> +         * If the caller wants to open a preexistent file, then the first
> +         * open() is decisive and its third argument is ignored, and the 
> second
> +         * open() and the fchmod() are never called.
> +         */
> +        fd = open(path, oflag | ((oflag & O_CREAT) ? O_EXCL : 0), 0);
> +        if (fd == -1 && errno == EEXIST) {
> +            oflag &= ~(unsigned)O_CREAT;
> +            fd = open(path, oflag);
> +        }
> +
> +        if (fd == -1) {
> +            error_setg_errno(&local_err, errno, "failed to open file '%s' "
> +                             "(mode: '%s')", path, mode);
> +        } else {
> +            qemu_set_cloexec(fd);
> +
> +            if ((oflag & O_CREAT) && fchmod(fd, DEFAULT_NEW_FILE_MODE) == -1) {
> +                error_setg_errno(&local_err, errno, "failed to set 
> permission "
> +                                 "0%03o on new file '%s' (mode: '%s')",
> +                                 (unsigned)DEFAULT_NEW_FILE_MODE, path, 
> mode);
> +            } else {
> +                FILE *f;
> +
> +                f = fdopen(fd, mode);
> +                if (f == NULL) {
> +                    error_setg_errno(&local_err, errno, "failed to associate 
> "
> +                                     "stdio stream with file descriptor %d, 
> "
> +                                     "file '%s' (mode: '%s')", fd, path, 
> mode);
> +                } else {
> +                    return f;
> +                }
> +            }
> +
> +            close(fd);
> +        }
> +    }
> +
> +    error_propagate(err, local_err);
> +    return NULL;
> +}
> +
>  int64_t qmp_guest_file_open(const char *path, bool has_mode, const char 
> *mode, Error **err)
>  {
>      FILE *fh;
> +    Error *local_err = NULL;
>      int fd;
>      int64_t ret = -1, handle;
>  
> @@ -247,10 +363,9 @@ int64_t qmp_guest_file_open(const char *path, bool 
> has_mode, const char *mode, E
>          mode = "r";
>      }
>      slog("guest-file-open called, filepath: %s, mode: %s", path, mode);
> -    fh = fopen(path, mode);
> -    if (!fh) {
> -        error_setg_errno(err, errno, "failed to open file '%s' (mode: 
> '%s')",
> -                         path, mode);
> +    fh = safe_open_or_create(path, mode, &local_err);
> +    if (local_err != NULL) {
> +        error_propagate(err, local_err);
>          return -1;
>      }
>  
> diff --git a/qga/main.c b/qga/main.c
> index 1841759..44a2836 100644
> --- a/qga/main.c
> +++ b/qga/main.c
> @@ -478,7 +478,7 @@ static void become_daemon(const char *pidfile)
>          }
>      }
>  
> -    umask(0);
> +    umask(S_IRWXG | S_IRWXO);
>      sid = setsid();
>      if (sid < 0) {
>          goto fail;

I believe we would want this patch for our stable releases as well.

Bruce