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

[Laszlo Ersek]

On 05/27/13 02:11, Laszlo Ersek wrote:
> On 05/26/13 15:34, Andreas Färber 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>
>> (cherry picked from commit c689b4f1bac352dcfd6ecb9a1d45337de0f1de67)
>>
>> [AF: Use error_set() instead of error_setg*()]
>> Signed-off-by: Andreas Färber <address@hidden>
>> ---
>>  qemu-ga.c            |   2 +-
>>  qga/commands-posix.c | 117 
>> +++++++++++++++++++++++++++++++++++++++++++++++++--
>>  2 files changed, 115 insertions(+), 4 deletions(-)
>>
>> diff --git a/qemu-ga.c b/qemu-ga.c
>> index 1b00c2f..086b1b9 100644
>> --- a/qemu-ga.c
>> +++ b/qemu-ga.c
>> @@ -424,7 +424,7 @@ static void become_daemon(const char *pidfile)
>>          }
>>      }
>>  
>> -    umask(0);
>> +    umask(S_IRWXG | S_IRWXO);
>>      sid = setsid();
>>      if (sid < 0) {
>>          goto fail;
>> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
>> index 00d035d..c0a1bd4 100644
>> --- a/qga/commands-posix.c
>> +++ b/qga/commands-posix.c
>> @@ -15,6 +15,9 @@
>>  #include <sys/types.h>
>>  #include <sys/ioctl.h>
>>  #include <sys/wait.h>
>> +#include <stdio.h>
>> +#include <string.h>
>> +#include <sys/stat.h>
>>  #include "qga/guest-agent-core.h"
>>  #include "qga-qmp-commands.h"
>>  #include "qerror.h"
>> @@ -122,9 +125,117 @@ static GuestFileHandle *guest_file_handle_find(int64_t 
>> id)
>>      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_set(err, QERR_UNSUPPORTED);
>> +        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_set(&local_err, QERR_OPEN_FILE_FAILED, path);
>> +        } else {
>> +            qemu_set_cloexec(fd);
>> +
>> +            if ((oflag & O_CREAT) && fchmod(fd, DEFAULT_NEW_FILE_MODE) == 
>> -1) {
>> +                error_set(&local_err, QERR_IO_ERROR);
>> +            } else {
>> +                FILE *f;
>> +
>> +                f = fdopen(fd, mode);
>> +                if (f == NULL) {
>> +                    error_set(&local_err, QERR_IO_ERROR);
>> +                } 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;
>>  
>> @@ -132,9 +243,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_set(err, QERR_OPEN_FILE_FAILED, path);
>> +    fh = safe_open_or_create(path, mode, &local_err);
>> +    if (local_err != NULL) {
>> +        error_propagate(err, local_err);
>>          return -1;
>>      }
>>  
>>
> 
> Looks good to me.

Forgot this one (not sure it was needed):

Reviewed-by: Laszlo Ersek <address@hidden>

> 
> Do you plan to backport
> 
>   8fe6bbc qga: distinguish binary modes in "guest_file_open_modes" map
>   2b72001 qga: unlink just created guest-file if fchmod() or fdopen()
>           fails on it
> 
> too? These are considered polish for the CVE fix.
> 
> Also, a side-note: existing world-writable log files etc. are not
> recreated nor have their modes changed, so maybe a release note or some
> such would be useful for admins ("delete your previous logfile &
> optional unix domain socket, or change their modes manually").