Re: [Qemu-devel] [PATCH] qga: set umask 0077 when daemonizing (CVE-2013-2007)
On 5/7/2013 at 05:47 AM, Anthony Liguori aligu...@us.ibm.com wrote:
From: Laszlo Ersek ler...@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.
Signed-off-by: Laszlo Ersek ler...@redhat.com
Signed-off-by: Anthony Liguori aligu...@us.ibm.com
---
qga/commands-posix.c | 123
+--
qga/main.c | 2 +-
2 files changed, 120 insertions(+), 5 deletions(-)
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 3b5c536..04c6951 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -18,6 +18,9 @@
#include unistd.h
#include errno.h
#include fcntl.h
+#include stdio.h
+#include string.h
+#include sys/stat.h
#include inttypes.h
#include qga/guest-agent-core.h
#include qga-qmp-commands.h
@@ -237,9 +240,122 @@ static GuestFileHandle *guest_file_handle_find(int64_t
id, Error **err)
return NULL;
}
+typedef const char * const ccpc;
+
+/* 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
},
+{ (ccpc[]){ w, wb, NULL }, O_WRONLY | O_CREAT | O_TRUNC
},
+{ (ccpc[]){ a, ab, NULL }, O_WRONLY | O_CREAT | O_APPEND
},
+{ (ccpc[]){ r+, rb+, r+b, NULL }, O_RDWR
},
+{ (ccpc[]){ w+, wb+, w+b, NULL }, O_RDWR | O_CREAT | O_TRUNC
},
+{ (ccpc[]){ a+, ab+, a+b, NULL }, O_RDWR | O_CREAT | O_APPEND }
+};
+
+static int
+find_open_flag(const char *mode_str, Error **err)
+{
+unsigned mode;
+
+for (mode = 0; mode ARRAY_SIZE(guest_file_open_modes); ++mode) {
+ccpc *form;
+
+form = guest_file_open_modes[mode].forms;
+while (*form != NULL strcmp(*form, mode_str) != 0) {
+++form;
+}
+if (*form != NULL) {
+break;
+}
+}
+
+if (mode == ARRAY_SIZE(guest_file_open_modes)) {
+error_setg(err, invalid file open mode '%s', mode_str);
+return -1;
+}
+return guest_file_open_modes[mode].oflag_base | O_NOCTTY | O_NONBLOCK;
+}
+
+#define DEFAULT_NEW_FILE_MODE (S_IRUSR | S_IWUSR | \
+ S_IRGRP | S_IWGRP | \
+ S_IROTH | S_IWOTH)
+
+static FILE *
+safe_open_or_create(const char *path, const char *mode, Error **err)
+{
+Error *local_err = NULL;
+int oflag;
+
+oflag = find_open_flag(mode, local_err);
+if (local_err == NULL) {
+int fd;
+
+/* If the caller wants / allows creation of a new file, we
implement it
+ * with a two step process: open() + (open() / fchmod()).
+ *
+ * First we insist on creating the file exclusively as a new file.
If
+ * that succeeds, we're free to set any file-mode bits on it. (The
+ * motivation is that we want to set those file-mode bits
independently
+ * of the current umask.)
+ *
+ * If the exclusive creation fails because the file already exists
+ * (EEXIST is not possible for any other reason), we just attempt
to
+ * open the file, but in this case we won't be allowed to change
the
+ * file-mode bits on the preexistent file.
+ *
+ * The pathname should never disappear between the two open()s in
+ * practice. If it happens, then someone very likely tried to race
us.
+ * In this case just go ahead and report the ENOENT from the second
+ * open() to the caller.
+ *
+ * If the caller wants to open a preexistent file, then the first
+ * open() is decisive and its third argument is ignored, and the
second
+ * open() and the fchmod() are never called.
+ */
+fd = open(path, oflag | ((oflag O_CREAT) ? O_EXCL : 0), 0);
+if (fd == -1 errno == EEXIST) {
+oflag = ~(unsigned)O_CREAT;
+fd = open(path, oflag);
+}
+
+if (fd == -1) {
+error_setg_errno(local_err, errno, failed to open file '%s'
+ (mode: '%s'), path, mode);
+
[Qemu-devel] [PATCH] qga: set umask 0077 when daemonizing (CVE-2013-2007)
From: Laszlo Ersek ler...@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.
Signed-off-by: Laszlo Ersek ler...@redhat.com
Signed-off-by: Anthony Liguori aligu...@us.ibm.com
---
qga/commands-posix.c | 123 +--
qga/main.c | 2 +-
2 files changed, 120 insertions(+), 5 deletions(-)
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 3b5c536..04c6951 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -18,6 +18,9 @@
#include unistd.h
#include errno.h
#include fcntl.h
+#include stdio.h
+#include string.h
+#include sys/stat.h
#include inttypes.h
#include qga/guest-agent-core.h
#include qga-qmp-commands.h
@@ -237,9 +240,122 @@ static GuestFileHandle *guest_file_handle_find(int64_t
id, Error **err)
return NULL;
}
+typedef const char * const ccpc;
+
+/* 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 },
+{ (ccpc[]){ w, wb, NULL }, O_WRONLY | O_CREAT | O_TRUNC },
+{ (ccpc[]){ a, ab, NULL }, O_WRONLY | O_CREAT | O_APPEND },
+{ (ccpc[]){ r+, rb+, r+b, NULL }, O_RDWR},
+{ (ccpc[]){ w+, wb+, w+b, NULL }, O_RDWR | O_CREAT | O_TRUNC },
+{ (ccpc[]){ a+, ab+, a+b, NULL }, O_RDWR | O_CREAT | O_APPEND }
+};
+
+static int
+find_open_flag(const char *mode_str, Error **err)
+{
+unsigned mode;
+
+for (mode = 0; mode ARRAY_SIZE(guest_file_open_modes); ++mode) {
+ccpc *form;
+
+form = guest_file_open_modes[mode].forms;
+while (*form != NULL strcmp(*form, mode_str) != 0) {
+++form;
+}
+if (*form != NULL) {
+break;
+}
+}
+
+if (mode == ARRAY_SIZE(guest_file_open_modes)) {
+error_setg(err, invalid file open mode '%s', mode_str);
+return -1;
+}
+return guest_file_open_modes[mode].oflag_base | O_NOCTTY | O_NONBLOCK;
+}
+
+#define DEFAULT_NEW_FILE_MODE (S_IRUSR | S_IWUSR | \
+ S_IRGRP | S_IWGRP | \
+ S_IROTH | S_IWOTH)
+
+static FILE *
+safe_open_or_create(const char *path, const char *mode, Error **err)
+{
+Error *local_err = NULL;
+int oflag;
+
+oflag = find_open_flag(mode, local_err);
+if (local_err == NULL) {
+int fd;
+
+/* If the caller wants / allows creation of a new file, we implement it
+ * with a two step process: open() + (open() / fchmod()).
+ *
+ * First we insist on creating the file exclusively as a new file. If
+ * that succeeds, we're free to set any file-mode bits on it. (The
+ * motivation is that we want to set those file-mode bits independently
+ * of the current umask.)
+ *
+ * If the exclusive creation fails because the file already exists
+ * (EEXIST is not possible for any other reason), we just attempt to
+ * open the file, but in this case we won't be allowed to change the
+ * file-mode bits on the preexistent file.
+ *
+ * The pathname should never disappear between the two open()s in
+ * practice. If it happens, then someone very likely tried to race us.
+ * In this case just go ahead and report the ENOENT from the second
+ * open() to the caller.
+ *
+ * If the caller wants to open a preexistent file, then the first
+ * open() is decisive and its third argument is ignored, and the second
+ * open() and the fchmod() are never called.
+ */
+fd = open(path, oflag | ((oflag O_CREAT) ? O_EXCL : 0), 0);
+if (fd == -1 errno == EEXIST) {
+oflag = ~(unsigned)O_CREAT;
+fd = open(path, oflag);
+}
+
+if (fd == -1) {
+error_setg_errno(local_err, errno, failed to open file '%s'
+ (mode: '%s'), path, mode);
+} else {
+qemu_set_cloexec(fd);
+
+if ((oflag O_CREAT) fchmod(fd, DEFAULT_NEW_FILE_MODE) == -1) {
+error_setg_errno(local_err, errno, failed to set permission
+
Re: [Qemu-devel] [PATCH] qga: set umask 0077 when daemonizing (CVE-2013-2007)
On 05/07/2013 05:47 AM, Anthony Liguori wrote:
From: Laszlo Ersek ler...@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
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)
Applied. Thanks. Regards, Anthony Liguori
Re: [Qemu-devel] [PATCH] qga: set umask 0077 when daemonizing (CVE-2013-2007)
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 ler...@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)
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
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)
On 05/07/2013 02:28 PM, mdroth wrote: 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? C99 and C11 don't care about permission bits of files created by fopen() - that's a concept added by POSIX. POSIX says that fopen() creates new files with respect to the current umask settings (in other words, 0666 minus bits that umask forbids). But since we weren't forbidding any bits, that meant we were getting 0666 files. http://pubs.opengroup.org/onlinepubs/9699919799/functions/fopen.html This patch intentionally leaves things unchanged so that any file created via guest-file-open still has mode 0666, just as it was pre-patch (it's just that the mode bits are now set via fchmod instead of implied via a umask of 0). But pre-patch, we handed any string on to fopen (whether bogus, such as read, or valid C11, such as wx, or even glibc extension, such as we) and whether it succeeded or failed was dependent on the extensions supported in the version of libc running the guest agent. Now post-patch, we only accept a finite set of mode strings (those documented in C99) and explicitly reject others, even if they used to succeed. The documentation in qga/qapi-schema.json doesn't mention anything about the permissions given to created files (other than what you can infer by chasing down the POSIX rather than the C99 definition of fopen), and it only says that @mode is as per fopen() without saying whether it is C99 or C11 fopen(). 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. It boils down to fopen() mode argument being puny when compared to open()'s bit flags and optional mode_t argument. We inherently limited ourselves by designing after the higher-level C interface instead of the lower-level POSIX interface. So yes, hopefully when we design a new more powerful qga command, we will put more thought into designing it to do everything that we really want. -- 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)
Anthony Liguori aligu...@us.ibm.com writes: Applied. Thanks. Hi, This was an automated response so it doesn't acknowledge the fact that since this was a CVE, I applied the original patch regardless of review feedback to avoid any confusion about whether the CVE has been addressed. In the past, we've modified the patches published with CVEs because of feedback on the list and this creates tremendous confusion. This even resulted in a distro including an incorrect patches because they mistakenly thought a CVE wasn't addressed. Please do review and provide feedback for this patch and we'll incorporate that in follow-ups as Laszlo has already done. And as usual, thanks to everyone involved in reporting, reviewing, and coordinating the handling of this CVE! Regards, Anthony Liguori Regards, Anthony Liguori
