qga: set umask 0077 when daemonizing (CVE-2013-2007)

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(-)

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.

Applied.  Thanks.

Regards,

Anthony Liguori

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
>

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.

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

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