On Tue, May 07, 2013 at 09:55:06AM -0600, Eric Blake wrote: > 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)
Got a pointer? I can fix up the docs if need be, but fopen() docs that qemu-ga points at seem to indicate 0666 will be used for new files. That's no longer the case? > > 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 Yes, shame it wasn't there at the start. a guest-file-open-full or something with support for specifying creation mode will have to do it. > 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 >
