On 05/07/2013 05:47 AM, Anthony Liguori 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.
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.5. > +/* 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 }, 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) == > -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); 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 = 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. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
