From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:45814) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ugl06-00033Q-Pp for qemu-devel@nongnu.org; Sun, 26 May 2013 20:09:30 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Ugkzz-00079Y-D9 for qemu-devel@nongnu.org; Sun, 26 May 2013 20:09:22 -0400 Message-ID: <51A2A4B6.2030400@redhat.com> Date: Mon, 27 May 2013 02:11:34 +0200 From: Laszlo Ersek MIME-Version: 1.0 References: <1369575248-31854-1-git-send-email-afaerber@suse.de> In-Reply-To: <1369575248-31854-1-git-send-email-afaerber@suse.de> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH stable-1.1] qga: set umask 0077 when daemonizing (CVE-2013-2007) List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?B?QW5kcmVhcyBGw6RyYmVy?= Cc: Anthony Liguori , qemu-devel@nongnu.org, qemu-stable@nongnu.org On 05/26/13 15:34, Andreas F=C3=A4rber wrote: > From: Laszlo Ersek >=20 > The qemu guest agent creates a bunch of files with insecure permissions > when started in daemon mode. For example: >=20 > -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 >=20 > 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 affecte= d. >=20 > For now mask all file mode bits for "group" and "others" in > become_daemon(). >=20 > Temporarily, for compatibility reasons, stick with the 0666 file-mode i= n > case of files newly created by the "guest-file-open" QMP call. Do so > without changing the umask temporarily. >=20 > Signed-off-by: Laszlo Ersek > Signed-off-by: Anthony Liguori > (cherry picked from commit c689b4f1bac352dcfd6ecb9a1d45337de0f1de67) >=20 > [AF: Use error_set() instead of error_setg*()] > Signed-off-by: Andreas F=C3=A4rber > --- > qemu-ga.c | 2 +- > qga/commands-posix.c | 117 +++++++++++++++++++++++++++++++++++++++++++= ++++++-- > 2 files changed, 115 insertions(+), 4 deletions(-) >=20 > 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) > } > } > =20 > - umask(0); > + umask(S_IRWXG | S_IRWXO); > sid =3D 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 > #include > #include > +#include > +#include > +#include > #include "qga/guest-agent-core.h" > #include "qga-qmp-commands.h" > #include "qerror.h" > @@ -122,9 +125,117 @@ static GuestFileHandle *guest_file_handle_find(in= t64_t id) > return NULL; > } > =20 > +typedef const char * const ccpc; > + > +/* http://pubs.opengroup.org/onlinepubs/9699919799/functions/fopen.htm= l */ > +static const struct { > + ccpc *forms; > + int oflag_base; > +} guest_file_open_modes[] =3D { > + { (ccpc[]){ "r", "rb", NULL }, O_RDONLY = }, > + { (ccpc[]){ "w", "wb", NULL }, O_WRONLY | O_CREAT | O_TRU= NC }, > + { (ccpc[]){ "a", "ab", NULL }, O_WRONLY | O_CREAT | O_APP= END }, > + { (ccpc[]){ "r+", "rb+", "r+b", NULL }, O_RDWR = }, > + { (ccpc[]){ "w+", "wb+", "w+b", NULL }, O_RDWR | O_CREAT | O_TRU= NC }, > + { (ccpc[]){ "a+", "ab+", "a+b", NULL }, O_RDWR | O_CREAT | O_APP= END } > +}; > + > +static int > +find_open_flag(const char *mode_str, Error **err) > +{ > + unsigned mode; > + > + for (mode =3D 0; mode < ARRAY_SIZE(guest_file_open_modes); ++mode)= { > + ccpc *form; > + > + form =3D guest_file_open_modes[mode].forms; > + while (*form !=3D NULL && strcmp(*form, mode_str) !=3D 0) { > + ++form; > + } > + if (*form !=3D NULL) { > + break; > + } > + } > + > + if (mode =3D=3D ARRAY_SIZE(guest_file_open_modes)) { > + error_set(err, QERR_UNSUPPORTED); > + return -1; > + } > + return guest_file_open_modes[mode].oflag_base | O_NOCTTY | O_NONBL= OCK; > +} > + > +#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 =3D NULL; > + int oflag; > + > + oflag =3D find_open_flag(mode, &local_err); > + if (local_err =3D=3D NULL) { > + int fd; > + > + /* If the caller wants / allows creation of a new file, we imp= lement it > + * with a two step process: open() + (open() / fchmod()). > + * > + * First we insist on creating the file exclusively as a new f= ile. 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 inde= pendently > + * of the current umask.) > + * > + * If the exclusive creation fails because the file already ex= ists > + * (EEXIST is not possible for any other reason), we just atte= mpt to > + * open the file, but in this case we won't be allowed to chan= ge 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 s= econd > + * open() to the caller. > + * > + * If the caller wants to open a preexistent file, then the fi= rst > + * open() is decisive and its third argument is ignored, and t= he second > + * open() and the fchmod() are never called. > + */ > + fd =3D open(path, oflag | ((oflag & O_CREAT) ? O_EXCL : 0), 0)= ; > + if (fd =3D=3D -1 && errno =3D=3D EEXIST) { > + oflag &=3D ~(unsigned)O_CREAT; > + fd =3D open(path, oflag); > + } > + > + if (fd =3D=3D -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)= =3D=3D -1) { > + error_set(&local_err, QERR_IO_ERROR); > + } else { > + FILE *f; > + > + f =3D fdopen(fd, mode); > + if (f =3D=3D 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 cha= r *mode, Error **err) > { > FILE *fh; > + Error *local_err =3D NULL; > int fd; > int64_t ret =3D -1; > =20 > @@ -132,9 +243,9 @@ int64_t qmp_guest_file_open(const char *path, bool = has_mode, const char *mode, E > mode =3D "r"; > } > slog("guest-file-open called, filepath: %s, mode: %s", path, mode)= ; > - fh =3D fopen(path, mode); > - if (!fh) { > - error_set(err, QERR_OPEN_FILE_FAILED, path); > + fh =3D safe_open_or_create(path, mode, &local_err); > + if (local_err !=3D NULL) { > + error_propagate(err, local_err); > return -1; > } > =20 >=20 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