From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:35116) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UZoXE-0001ho-Py for qemu-devel@nongnu.org; Tue, 07 May 2013 16:30:56 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UZoXC-0000iA-C8 for qemu-devel@nongnu.org; Tue, 07 May 2013 16:30:52 -0400 Received: from mail-vb0-x235.google.com ([2607:f8b0:400c:c02::235]:58049) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UZoXC-0000hv-5A for qemu-devel@nongnu.org; Tue, 07 May 2013 16:30:50 -0400 Received: by mail-vb0-f53.google.com with SMTP id i3so907438vbh.26 for ; Tue, 07 May 2013 13:30:49 -0700 (PDT) Sender: fluxion Date: Tue, 7 May 2013 15:28:55 -0500 From: mdroth Message-ID: <20130507202855.GB6474@vm> References: <1367927231-24291-1-git-send-email-aliguori@us.ibm.com> <518923DA.6030105@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <518923DA.6030105@redhat.com> 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: Eric Blake Cc: Anthony Liguori , Laszlo Ersek , qemu-devel@nongnu.org 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 > > > > 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 >