[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: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH] qga: set umask 0077 when daemonizing (CVE-2013-2007) |
Date: |
Tue, 07 May 2013 09:55:06 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130402 Thunderbird/17.0.5 |
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)
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
Re: [Qemu-devel] [PATCH] qga: set umask 0077 when daemonizing (CVE-2013-2007), mdroth, 2013/05/07