Re: [Qemu-devel] [PATCH] qga: set umask 0077 when daemonizing (CVE-2013-2007)

[mdroth]

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 <address@hidden>
> > 
> > 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
>