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

[Bruce Rogers]

 >>> On 5/7/2013 at 05:47 AM, Anthony Liguori <address@hidden> wrote: 
> From: Laszlo Ersek <address@hidden>
> 
> 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 <address@hidden>
> Signed-off-by: Anthony Liguori <address@hidden>
> ---
>  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