>>> On 5/7/2013 at 05:47 AM, Anthony Liguori <aligu...@us.ibm.com> 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> > --- > 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
