On 05/26/13 15:34, Andreas Färber wrote:
> From: Laszlo Ersek <ler...@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 <ler...@redhat.com>
> Signed-off-by: Anthony Liguori <aligu...@us.ibm.com>
> (cherry picked from commit c689b4f1bac352dcfd6ecb9a1d45337de0f1de67)
>
> [AF: Use error_set() instead of error_setg*()]
> Signed-off-by: Andreas Färber <afaer...@suse.de>
> ---
> 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.
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").
Laszlo
