From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:34265) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UZkES-00072Z-OE for qemu-devel@nongnu.org; Tue, 07 May 2013 11:55:17 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UZkEP-0002Jv-A3 for qemu-devel@nongnu.org; Tue, 07 May 2013 11:55:12 -0400 Received: from mx1.redhat.com ([209.132.183.28]:56598) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UZkEP-0002Jl-2M for qemu-devel@nongnu.org; Tue, 07 May 2013 11:55:09 -0400 Message-ID: <518923DA.6030105@redhat.com> Date: Tue, 07 May 2013 09:55:06 -0600 From: Eric Blake MIME-Version: 1.0 References: <1367927231-24291-1-git-send-email-aliguori@us.ibm.com> In-Reply-To: <1367927231-24291-1-git-send-email-aliguori@us.ibm.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="----enig2EKRFOQEFMKEOMSXHIKTI" Subject: Re: [Qemu-devel] [PATCH] qga: set umask 0077 when daemonizing (CVE-2013-2007) List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: Laszlo Ersek , qemu-devel@nongnu.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) ------enig2EKRFOQEFMKEOMSXHIKTI Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 05/07/2013 05:47 AM, Anthony Liguori 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. This points out that: 1. the documentation for guest-file-open is insufficient to describe our limitations (for example, although C11 requires support for the "wx" flag, this patch forbids that flag) 2. guest-file-open is rather puny; we may need a newer interface that allows the user finer-grained control over the actual mode bits set on the file that they are creating (and maybe something similar to openat() that would let the user open/create a file relative to an existing handle to a directory, rather than always having to specify an absolute path). But I agree that _this_ patch fixes the CVE, and that any further changes to resolve the above two issues are not essential to include in 1= =2E5. > +/* 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 = }, You are mapping things according to their POSIX definition (POSIX, as an additional requirement above and beyond C99, states that presence or absence of 'b' is a no-op because there is no difference between binary and text mode). But C99 permits a distinct binary mode, and qga is compiled for Windows where binary mode is indeed different. I think that you probably should split this map into: { (ccpc[]){ "r", NULL }, O_RDONLY }, { (ccpc[]){ "rb", NULL }, O_RDONLY | O_BINARY }, and so forth (assuming that O_BINARY is properly #defined to 0 on POSIX-y systems that don't need it), so that you don't regress the qga server in a Windows guest where the management client is explicitly passing or omitting 'b' for the intentional difference between text and binary files. [1] > + > + if ((oflag & O_CREAT) && fchmod(fd, DEFAULT_NEW_FILE_MODE)= =3D=3D -1) { > + error_setg_errno(&local_err, errno, "failed to set per= mission " > + "0%03o on new file '%s' (mode: '%s')"= , > + (unsigned)DEFAULT_NEW_FILE_MODE, path= , mode); On this particular error path, we've left behind an empty mode 0000 file. Is it worth trying to unlink() the dead file? > + } else { > + FILE *f; > + > + f =3D fdopen(fd, mode); [1] I don't know if Windows is okay using fdopen() to create a FILE in binary mode if you didn't match O_BINARY on the fd underlying the FILE. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org ------enig2EKRFOQEFMKEOMSXHIKTI Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.13 (GNU/Linux) Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJRiSPaAAoJEKeha0olJ0NqXjQH/iKJMZlbDQFnI8BhytkJUH3A kyxySwZRgKtSMbYhr65vZdWl5e5zV0AzZAiW8lcAdfuYAxcLtZHHONdKEMXotY9s dcBZyFfk50kcFe0Nhe5kbOqrheUHjy6uezJ6isP3FVot55jfBwS/3Mxsk99AfWgE OY1DuYp3Gj7q3d/5aYhb2EBbLSXf1HCvWwg5kOhtFC0t4pMvHYkYALQDf22cCCfo 8gvV7PpXoH9gTXT0MsJXVirCC8FGk+8rmK5VOEvzF31+W9Xc6bA4Gw8GFeAzshYw tdqvEA0lZ5bhA3o4Zp5YaihhAzoUsDwkkg1hb7Fnkul6SRc3dsjeHrwSxvx74Yg= =izl2 -----END PGP SIGNATURE----- ------enig2EKRFOQEFMKEOMSXHIKTI--