[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] qga: set umask 0077 when daemonizing (CVE-2013-
From: |
mdroth |
Subject: |
Re: [Qemu-devel] [PATCH] qga: set umask 0077 when daemonizing (CVE-2013-2007) |
Date: |
Tue, 7 May 2013 15:28:55 -0500 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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
>
Re: [Qemu-devel] [PATCH] qga: set umask 0077 when daemonizing (CVE-2013-2007),
mdroth <=
Re: [Qemu-devel] [PATCH] qga: set umask 0077 when daemonizing (CVE-2013-2007), Anthony Liguori, 2013/05/07
Re: [Qemu-devel] [PATCH] qga: set umask 0077 when daemonizing (CVE-2013-2007), Bruce Rogers, 2013/05/09