From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:45904) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ugl18-0004pa-HB for qemu-devel@nongnu.org; Sun, 26 May 2013 20:10:36 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Ugl0x-0007Rw-9C for qemu-devel@nongnu.org; Sun, 26 May 2013 20:10:26 -0400 Message-ID: <51A2A4F3.5010006@redhat.com> Date: Mon, 27 May 2013 02:12:35 +0200 From: Laszlo Ersek MIME-Version: 1.0 References: <1369575248-31854-1-git-send-email-afaerber@suse.de> <51A2A4B6.2030400@redhat.com> In-Reply-To: <51A2A4B6.2030400@redhat.com> 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/27/13 02:11, Laszlo Ersek wrote: > On 05/26/13 15:34, Andreas F=C3=A4rber wrote: >> From: Laszlo Ersek >> >> The qemu guest agent creates a bunch of files with insecure permission= s >> 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 affect= ed. >> >> 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 >> Signed-off-by: Anthony Liguori >> (cherry picked from commit c689b4f1bac352dcfd6ecb9a1d45337de0f1de67) >> >> [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(-) >> >> 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(i= nt64_t id) >> return NULL; >> } >> =20 >> +typedef const char * const ccpc; >> + >> +/* http://pubs.opengroup.org/onlinepubs/9699919799/functions/fopen.ht= ml */ >> +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_TR= UNC }, >> + { (ccpc[]){ "a", "ab", NULL }, O_WRONLY | O_CREAT | O_AP= PEND }, >> + { (ccpc[]){ "r+", "rb+", "r+b", NULL }, O_RDWR = }, >> + { (ccpc[]){ "w+", "wb+", "w+b", NULL }, O_RDWR | O_CREAT | O_TR= UNC }, >> + { (ccpc[]){ "a+", "ab+", "a+b", NULL }, O_RDWR | O_CREAT | O_AP= PEND } >> +}; >> + >> +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_NONB= LOCK; >> +} >> + >> +#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 im= plement 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 ind= ependently >> + * of the current umask.) >> + * >> + * If the exclusive creation fails because the file already e= xists >> + * (EEXIST is not possible for any other reason), we just att= empt to >> + * open the file, but in this case we won't be allowed to cha= nge 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 f= irst >> + * open() is decisive and its third argument is ignored, and = the 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 ch= ar *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. Forgot this one (not sure it was needed): Reviewed-by: Laszlo Ersek >=20 > Do you plan to backport >=20 > 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 >=20 > too? These are considered polish for the CVE fix. >=20 > 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").