266 lines
8.6 KiB
Diff
266 lines
8.6 KiB
Diff
|
From 90f53d5951c6a4fc8def3e2af0f90c31ac9a717c Mon Sep 17 00:00:00 2001
|
||
|
From: Laszlo Ersek <lersek@redhat.com>
|
||
|
Date: Wed, 24 Apr 2013 13:13:18 +0200
|
||
|
Subject: [PATCH] qga: set umask 0077 when daemonizing (CVE-2013-2007)
|
||
|
|
||
|
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 <lersek@redhat.com>
|
||
|
Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
|
||
|
(cherry picked from commit c689b4f1bac352dcfd6ecb9a1d45337de0f1de67)
|
||
|
|
||
|
Conflicts:
|
||
|
qga/commands-posix.c
|
||
|
qga/main.c
|
||
|
---
|
||
|
error.c | 28 ++++++++++++
|
||
|
error.h | 15 +++++++
|
||
|
qemu-ga.c | 2 +-
|
||
|
qga/commands-posix.c | 122 +++++++++++++++++++++++++++++++++++++++++++++++++--
|
||
|
4 files changed, 163 insertions(+), 4 deletions(-)
|
||
|
|
||
|
diff --git a/error.c b/error.c
|
||
|
index 1f05fc4..128d88c 100644
|
||
|
--- a/error.c
|
||
|
+++ b/error.c
|
||
|
@@ -43,6 +43,34 @@ void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
|
||
|
*errp = err;
|
||
|
}
|
||
|
|
||
|
+void error_set_errno(Error **errp, int os_errno, ErrorClass err_class,
|
||
|
+ const char *fmt, ...)
|
||
|
+{
|
||
|
+ Error *err;
|
||
|
+ char *msg1;
|
||
|
+ va_list ap;
|
||
|
+
|
||
|
+ if (errp == NULL) {
|
||
|
+ return;
|
||
|
+ }
|
||
|
+ assert(*errp == NULL);
|
||
|
+
|
||
|
+ err = g_malloc0(sizeof(*err));
|
||
|
+
|
||
|
+ va_start(ap, fmt);
|
||
|
+ msg1 = g_strdup_vprintf(fmt, ap);
|
||
|
+ if (os_errno != 0) {
|
||
|
+ err->msg = g_strdup_printf("%s: %s", msg1, strerror(os_errno));
|
||
|
+ g_free(msg1);
|
||
|
+ } else {
|
||
|
+ err->msg = msg1;
|
||
|
+ }
|
||
|
+ va_end(ap);
|
||
|
+ err->err_class = err_class;
|
||
|
+
|
||
|
+ *errp = err;
|
||
|
+}
|
||
|
+
|
||
|
Error *error_copy(const Error *err)
|
||
|
{
|
||
|
Error *err_new;
|
||
|
diff --git a/error.h b/error.h
|
||
|
index 96fc203..4d52e73 100644
|
||
|
--- a/error.h
|
||
|
+++ b/error.h
|
||
|
@@ -30,6 +30,21 @@ typedef struct Error Error;
|
||
|
void error_set(Error **err, ErrorClass err_class, const char *fmt, ...) GCC_FMT_ATTR(3, 4);
|
||
|
|
||
|
/**
|
||
|
+ * Set an indirect pointer to an error given a ErrorClass value and a
|
||
|
+ * printf-style human message, followed by a strerror() string if
|
||
|
+ * @os_error is not zero.
|
||
|
+ */
|
||
|
+void error_set_errno(Error **err, int os_error, ErrorClass err_class, const char *fmt, ...) GCC_FMT_ATTR(4, 5);
|
||
|
+
|
||
|
+/**
|
||
|
+ * Same as error_set(), but sets a generic error
|
||
|
+ */
|
||
|
+#define error_setg(err, fmt, ...) \
|
||
|
+ error_set(err, ERROR_CLASS_GENERIC_ERROR, fmt, ## __VA_ARGS__)
|
||
|
+#define error_setg_errno(err, os_error, fmt, ...) \
|
||
|
+ error_set_errno(err, os_error, ERROR_CLASS_GENERIC_ERROR, fmt, ## __VA_ARGS__)
|
||
|
+
|
||
|
+/**
|
||
|
* Returns true if an indirect pointer to an error is pointing to a valid
|
||
|
* error object.
|
||
|
*/
|
||
|
diff --git a/qemu-ga.c b/qemu-ga.c
|
||
|
index b747470..45514a2 100644
|
||
|
--- a/qemu-ga.c
|
||
|
+++ b/qemu-ga.c
|
||
|
@@ -421,7 +421,7 @@ static void become_daemon(const char *pidfile)
|
||
|
}
|
||
|
}
|
||
|
|
||
|
- umask(0);
|
||
|
+ umask(S_IRWXG | S_IRWXO);
|
||
|
sid = setsid();
|
||
|
if (sid < 0) {
|
||
|
goto fail;
|
||
|
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
|
||
|
index ce90421..7510669 100644
|
||
|
--- a/qga/commands-posix.c
|
||
|
+++ b/qga/commands-posix.c
|
||
|
@@ -15,6 +15,9 @@
|
||
|
#include <sys/types.h>
|
||
|
#include <sys/ioctl.h>
|
||
|
#include <sys/wait.h>
|
||
|
+#include <stdio.h>
|
||
|
+#include <string.h>
|
||
|
+#include <sys/stat.h>
|
||
|
#include "qga/guest-agent-core.h"
|
||
|
#include "qga-qmp-commands.h"
|
||
|
#include "qerror.h"
|
||
|
@@ -125,9 +128,122 @@ static GuestFileHandle *guest_file_handle_find(int64_t id)
|
||
|
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 "
|
||
|
+ "0%03o on new file '%s' (mode: '%s')",
|
||
|
+ (unsigned)DEFAULT_NEW_FILE_MODE, path, mode);
|
||
|
+ } else {
|
||
|
+ FILE *f;
|
||
|
+
|
||
|
+ f = fdopen(fd, mode);
|
||
|
+ if (f == NULL) {
|
||
|
+ error_setg_errno(&local_err, errno, "failed to associate "
|
||
|
+ "stdio stream with file descriptor %d, "
|
||
|
+ "file '%s' (mode: '%s')", fd, path, mode);
|
||
|
+ } else {
|
||
|
+ return f;
|
||
|
+ }
|
||
|
+ }
|
||
|
+
|
||
|
+ close(fd);
|
||
|
+ }
|
||
|
+ }
|
||
|
+
|
||
|
+ error_propagate(err, local_err);
|
||
|
+ return NULL;
|
||
|
+}
|
||
|
+
|
||
|
int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode, Error **err)
|
||
|
{
|
||
|
FILE *fh;
|
||
|
+ Error *local_err = NULL;
|
||
|
int fd;
|
||
|
int64_t ret = -1;
|
||
|
|
||
|
@@ -135,9 +251,9 @@ int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode, E
|
||
|
mode = "r";
|
||
|
}
|
||
|
slog("guest-file-open called, filepath: %s, mode: %s", path, mode);
|
||
|
- fh = fopen(path, mode);
|
||
|
- if (!fh) {
|
||
|
- error_set(err, QERR_OPEN_FILE_FAILED, path);
|
||
|
+ fh = safe_open_or_create(path, mode, &local_err);
|
||
|
+ if (local_err != NULL) {
|
||
|
+ error_propagate(err, local_err);
|
||
|
return -1;
|
||
|
}
|
||
|
|