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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3541 bytes --]

On 05/07/2013 05:47 AM, Anthony Liguori wrote:
> From: Laszlo Ersek <lersek@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)

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]