[PATCH 0/5] [PATCH v2] kvmtool: fix virtio 9p vulnerabilities

[PATCH 0/5] [PATCH v2] kvmtool: fix virtio 9p vulnerabilities

[G. Campana]

This patch series should fix different vulnerabilities found in virtio 9p
(http://www.spinics.net/lists/kvm/msg130505.html), but it definitely needs some
testing. By the way, the very same path traversal vulnerability was also found
in Qemu in August: http://www.openwall.com/lists/oss-security/2016/08/30/1
and the path traversal fix looks quite similar.

v2:
* merge some commits
* add an explicit commit message to each patch
* add a Signed-off-by: line

v1:


G. Campana (5):
  kvmtool: 9p: fix path traversal vulnerabilities
  kvmtool: 9p: fix sprintf vulnerabilities
  kvmtool: 9p: fix strcpy vulnerabilities
  kvmtool: 9p: refactor fixes with get_full_path()
  kvmtool: 9p: fix a buffer overflow in rel_to_abs

 virtio/9p.c | 199 +++++++++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 158 insertions(+), 41 deletions(-)

-- 
2.7.4

[PATCH 1/5] kvmtool: 9p: fix path traversal vulnerabilities

[G. Campana]

A path traversal exists because the guest can send "../" sequences to
the host 9p handlers. To fix this vulnerability, we ensure that path
components sent by the guest don't contain "../" sequences.

Signed-off-by: G. Campana <gcampana+kvm@quarkslab.com>
---
 virtio/9p.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/virtio/9p.c b/virtio/9p.c
index 49e7c5c..c3edc20 100644
--- a/virtio/9p.c
+++ b/virtio/9p.c
@@ -222,6 +222,21 @@ static bool is_dir(struct p9_fid *fid)
 	return S_ISDIR(st.st_mode);
 }
 
+/* path is always absolute */
+static bool path_is_illegal(const char *path)
+{
+	size_t len;
+
+	if (strstr(path, "/../") != NULL)
+		return true;
+
+	len = strlen(path);
+	if (len >= 3 && strcmp(path + len - 3, "/..") == 0)
+		return true;
+
+	return false;
+}
+
 static void virtio_p9_open(struct p9_dev *p9dev,
 			   struct p9_pdu *pdu, u32 *outlen)
 {
@@ -278,6 +293,11 @@ static void virtio_p9_create(struct p9_dev *p9dev,
 	flags = virtio_p9_openflags(flags);
 
 	sprintf(full_path, "%s/%s", dfid->abs_path, name);
+	if (path_is_illegal(full_path)) {
+		errno = EACCES;
+		goto err_out;
+	}
+
 	fd = open(full_path, flags | O_CREAT, mode);
 	if (fd < 0)
 		goto err_out;
@@ -319,6 +339,11 @@ static void virtio_p9_mkdir(struct p9_dev *p9dev,
 	dfid = get_fid(p9dev, dfid_val);
 
 	sprintf(full_path, "%s/%s", dfid->abs_path, name);
+	if (path_is_illegal(full_path)) {
+		errno = EACCES;
+		goto err_out;
+	}
+
 	ret = mkdir(full_path, mode);
 	if (ret < 0)
 		goto err_out;
@@ -776,6 +801,11 @@ static void virtio_p9_rename(struct p9_dev *p9dev,
 	new_fid = get_fid(p9dev, new_fid_val);
 
 	sprintf(full_path, "%s/%s", new_fid->abs_path, new_name);
+	if (path_is_illegal(full_path)) {
+		errno = EACCES;
+		goto err_out;
+	}
+
 	ret = rename(fid->abs_path, full_path);
 	if (ret < 0)
 		goto err_out;
@@ -860,6 +890,11 @@ static void virtio_p9_mknod(struct p9_dev *p9dev,
 
 	dfid = get_fid(p9dev, fid_val);
 	sprintf(full_path, "%s/%s", dfid->abs_path, name);
+	if (path_is_illegal(full_path)) {
+		errno = EACCES;
+		goto err_out;
+	}
+
 	ret = mknod(full_path, mode, makedev(major, minor));
 	if (ret < 0)
 		goto err_out;
@@ -927,6 +962,11 @@ static void virtio_p9_symlink(struct p9_dev *p9dev,
 
 	dfid = get_fid(p9dev, fid_val);
 	sprintf(new_name, "%s/%s", dfid->abs_path, name);
+	if (path_is_illegal(new_name)) {
+		errno = EACCES;
+		goto err_out;
+	}
+
 	ret = symlink(old_path, new_name);
 	if (ret < 0)
 		goto err_out;
@@ -962,6 +1002,11 @@ static void virtio_p9_link(struct p9_dev *p9dev,
 	dfid = get_fid(p9dev, dfid_val);
 	fid =  get_fid(p9dev, fid_val);
 	sprintf(full_path, "%s/%s", dfid->abs_path, name);
+	if (path_is_illegal(full_path)) {
+		errno = EACCES;
+		goto err_out;
+	}
+
 	ret = link(fid->abs_path, full_path);
 	if (ret < 0)
 		goto err_out;
@@ -1079,6 +1124,11 @@ static void virtio_p9_renameat(struct p9_dev *p9dev,
 
 	sprintf(old_full_path, "%s/%s", old_dfid->abs_path, old_name);
 	sprintf(new_full_path, "%s/%s", new_dfid->abs_path, new_name);
+	if (path_is_illegal(old_full_path) || path_is_illegal(new_full_path)) {
+		errno = EACCES;
+		goto err_out;
+	}
+
 	ret = rename(old_full_path, new_full_path);
 	if (ret < 0)
 		goto err_out;
@@ -1112,6 +1162,11 @@ static void virtio_p9_unlinkat(struct p9_dev *p9dev,
 	fid = get_fid(p9dev, fid_val);
 
 	sprintf(full_path, "%s/%s", fid->abs_path, name);
+	if (path_is_illegal(full_path)) {
+		errno = EACCES;
+		goto err_out;
+	}
+
 	ret = remove(full_path);
 	if (ret < 0)
 		goto err_out;
-- 
2.7.4

[PATCH 2/5] kvmtool: 9p: fix sprintf vulnerabilities

[G. Campana]

Use snprintf instead of sprintf to avoid buffer overflow
vulnerabilities.

Signed-off-by: G. Campana <gcampana+kvm@quarkslab.com>
---
 virtio/9p.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 70 insertions(+), 11 deletions(-)

diff --git a/virtio/9p.c b/virtio/9p.c
index c3edc20..4116252 100644
--- a/virtio/9p.c
+++ b/virtio/9p.c
@@ -280,6 +280,7 @@ static void virtio_p9_create(struct p9_dev *p9dev,
 {
 	int fd, ret;
 	char *name;
+	size_t size;
 	struct stat st;
 	struct p9_qid qid;
 	struct p9_fid *dfid;
@@ -292,12 +293,26 @@ static void virtio_p9_create(struct p9_dev *p9dev,
 
 	flags = virtio_p9_openflags(flags);
 
-	sprintf(full_path, "%s/%s", dfid->abs_path, name);
+	ret = snprintf(full_path, sizeof(full_path), "%s/%s", dfid->abs_path, name);
+	if (ret >= (int)sizeof(full_path)) {
+		errno = ENAMETOOLONG;
+		goto err_out;
+	}
+
 	if (path_is_illegal(full_path)) {
 		errno = EACCES;
 		goto err_out;
 	}
 
+	size = sizeof(dfid->abs_path) - (dfid->path - dfid->abs_path);
+	ret = snprintf(dfid->path, size, "%s/%s", dfid->path, name);
+	if (ret >= (int)size) {
+		errno = ENAMETOOLONG;
+		if (size > 0)
+			dfid->path[size] = '\x00';
+		goto err_out;
+	}
+
 	fd = open(full_path, flags | O_CREAT, mode);
 	if (fd < 0)
 		goto err_out;
@@ -310,7 +325,6 @@ static void virtio_p9_create(struct p9_dev *p9dev,
 	if (ret < 0)
 		goto err_out;
 
-	sprintf(dfid->path, "%s/%s", dfid->path, name);
 	stat2qid(&st, &qid);
 	virtio_p9_pdu_writef(pdu, "Qd", &qid, 0);
 	*outlen = pdu->write_offset;
@@ -338,7 +352,12 @@ static void virtio_p9_mkdir(struct p9_dev *p9dev,
 			    &name, &mode, &gid);
 	dfid = get_fid(p9dev, dfid_val);
 
-	sprintf(full_path, "%s/%s", dfid->abs_path, name);
+	ret = snprintf(full_path, sizeof(full_path), "%s/%s", dfid->abs_path, name);
+	if (ret >= (int)sizeof(full_path)) {
+		errno = ENAMETOOLONG;
+		goto err_out;
+	}
+
 	if (path_is_illegal(full_path)) {
 		errno = EACCES;
 		goto err_out;
@@ -393,11 +412,16 @@ static void virtio_p9_walk(struct p9_dev *p9dev,
 			char tmp[PATH_MAX] = {0};
 			char full_path[PATH_MAX];
 			char *str;
+			int ret;
 
 			virtio_p9_pdu_readf(pdu, "s", &str);
 
 			/* Format the new path we're 'walk'ing into */
-			sprintf(tmp, "%s/%s", new_fid->path, str);
+			ret = snprintf(tmp, sizeof(tmp), "%s/%s", new_fid->path, str);
+			if (ret >= (int)sizeof(tmp)) {
+				errno = ENAMETOOLONG;
+				goto err_out;
+			}
 
 			free(str);
 
@@ -800,7 +824,12 @@ static void virtio_p9_rename(struct p9_dev *p9dev,
 	fid = get_fid(p9dev, fid_val);
 	new_fid = get_fid(p9dev, new_fid_val);
 
-	sprintf(full_path, "%s/%s", new_fid->abs_path, new_name);
+	ret = snprintf(full_path, sizeof(full_path), "%s/%s", new_fid->abs_path, new_name);
+	if (ret >= (int)sizeof(full_path)) {
+		errno = ENAMETOOLONG;
+		goto err_out;
+	}
+
 	if (path_is_illegal(full_path)) {
 		errno = EACCES;
 		goto err_out;
@@ -889,7 +918,12 @@ static void virtio_p9_mknod(struct p9_dev *p9dev,
 			    &major, &minor, &gid);
 
 	dfid = get_fid(p9dev, fid_val);
-	sprintf(full_path, "%s/%s", dfid->abs_path, name);
+	ret = snprintf(full_path, sizeof(full_path), "%s/%s", dfid->abs_path, name);
+	if (ret >= (int)sizeof(full_path)) {
+		errno = ENAMETOOLONG;
+		goto err_out;
+	}
+
 	if (path_is_illegal(full_path)) {
 		errno = EACCES;
 		goto err_out;
@@ -961,7 +995,12 @@ static void virtio_p9_symlink(struct p9_dev *p9dev,
 	virtio_p9_pdu_readf(pdu, "dssd", &fid_val, &name, &old_path, &gid);
 
 	dfid = get_fid(p9dev, fid_val);
-	sprintf(new_name, "%s/%s", dfid->abs_path, name);
+	ret = snprintf(new_name, sizeof(new_name), "%s/%s", dfid->abs_path, name);
+	if (ret >= (int)sizeof(new_name)) {
+		errno = ENAMETOOLONG;
+		goto err_out;
+	}
+
 	if (path_is_illegal(new_name)) {
 		errno = EACCES;
 		goto err_out;
@@ -1001,7 +1040,12 @@ static void virtio_p9_link(struct p9_dev *p9dev,
 
 	dfid = get_fid(p9dev, dfid_val);
 	fid =  get_fid(p9dev, fid_val);
-	sprintf(full_path, "%s/%s", dfid->abs_path, name);
+	ret = snprintf(full_path, sizeof(full_path), "%s/%s", dfid->abs_path, name);
+	if (ret >= (int)sizeof(full_path)) {
+		errno = ENAMETOOLONG;
+		goto err_out;
+	}
+
 	if (path_is_illegal(full_path)) {
 		errno = EACCES;
 		goto err_out;
@@ -1122,8 +1166,18 @@ static void virtio_p9_renameat(struct p9_dev *p9dev,
 	old_dfid = get_fid(p9dev, old_dfid_val);
 	new_dfid = get_fid(p9dev, new_dfid_val);
 
-	sprintf(old_full_path, "%s/%s", old_dfid->abs_path, old_name);
-	sprintf(new_full_path, "%s/%s", new_dfid->abs_path, new_name);
+	ret = snprintf(old_full_path, sizeof(old_full_path), "%s/%s", old_dfid->abs_path, old_name);
+	if (ret >= (int)sizeof(old_full_path)) {
+		errno = ENAMETOOLONG;
+		goto err_out;
+	}
+
+	ret = snprintf(new_full_path, sizeof(new_full_path), "%s/%s", new_dfid->abs_path, new_name);
+	if (ret >= (int)sizeof(new_full_path)) {
+		errno = ENAMETOOLONG;
+		goto err_out;
+	}
+
 	if (path_is_illegal(old_full_path) || path_is_illegal(new_full_path)) {
 		errno = EACCES;
 		goto err_out;
@@ -1161,7 +1215,12 @@ static void virtio_p9_unlinkat(struct p9_dev *p9dev,
 	virtio_p9_pdu_readf(pdu, "dsd", &fid_val, &name, &flags);
 	fid = get_fid(p9dev, fid_val);
 
-	sprintf(full_path, "%s/%s", fid->abs_path, name);
+	ret = snprintf(full_path, sizeof(full_path), "%s/%s", fid->abs_path, name);
+	if (ret >= (int)sizeof(full_path)) {
+		errno = ENAMETOOLONG;
+		goto err_out;
+	}
+
 	if (path_is_illegal(full_path)) {
 		errno = EACCES;
 		goto err_out;
-- 
2.7.4

[PATCH 3/5] kvmtool: 9p: fix strcpy vulnerabilities

[G. Campana]

Use strncpy instead of strcpy to avoid buffer overflow vulnerabilities.

Signed-off-by: G. Campana <gcampana+kvm@quarkslab.com>
---
 virtio/9p.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 56 insertions(+), 17 deletions(-)

diff --git a/virtio/9p.c b/virtio/9p.c
index 4116252..22aa268 100644
--- a/virtio/9p.c
+++ b/virtio/9p.c
@@ -28,6 +28,7 @@ static struct p9_fid *find_or_create_fid(struct p9_dev *dev, u32 fid)
 {
 	struct rb_node *node = dev->fids.rb_node;
 	struct p9_fid *pfid = NULL;
+	size_t len;
 
 	while (node) {
 		struct p9_fid *cur = rb_entry(node, struct p9_fid, node);
@@ -45,9 +46,15 @@ static struct p9_fid *find_or_create_fid(struct p9_dev *dev, u32 fid)
 	if (!pfid)
 		return NULL;
 
+	len = strlen(dev->root_dir);
+	if (len >= sizeof(pfid->abs_path)) {
+		free(pfid);
+		return NULL;
+	}
+
 	pfid->fid = fid;
-	strcpy(pfid->abs_path, dev->root_dir);
-	pfid->path = pfid->abs_path + strlen(dev->root_dir);
+	strncpy(pfid->abs_path, dev->root_dir, sizeof(pfid->abs_path));
+	pfid->path = pfid->abs_path + strlen(pfid->abs_path);
 
 	insert_new_fid(dev, pfid);
 
@@ -386,6 +393,19 @@ err_out:
 	return;
 }
 
+static int join_path(struct p9_fid *fid, const char *name)
+{
+	size_t len, size;
+
+	size = sizeof(fid->abs_path) - (fid->path - fid->abs_path);
+	len = strlen(name);
+	if (len >= size)
+		return -1;
+
+	strncpy(fid->path, name, size);
+	return 0;
+}
+
 static void virtio_p9_walk(struct p9_dev *p9dev,
 			   struct p9_pdu *pdu, u32 *outlen)
 {
@@ -404,7 +424,11 @@ static void virtio_p9_walk(struct p9_dev *p9dev,
 	if (nwname) {
 		struct p9_fid *fid = get_fid(p9dev, fid_val);
 
-		strcpy(new_fid->path, fid->path);
+		if (join_path(new_fid, fid->path) != 0) {
+			errno = ENAMETOOLONG;
+			goto err_out;
+		}
+
 		/* skip the space for count */
 		pdu->write_offset += sizeof(u16);
 		for (i = 0; i < nwname; i++) {
@@ -429,7 +453,10 @@ static void virtio_p9_walk(struct p9_dev *p9dev,
 				goto err_out;
 
 			stat2qid(&st, &wqid);
-			strcpy(new_fid->path, tmp);
+			if (join_path(new_fid, tmp) != 0) {
+				errno = ENAMETOOLONG;
+				goto err_out;
+			}
 			new_fid->uid = fid->uid;
 			nwqid++;
 			virtio_p9_pdu_writef(pdu, "Q", &wqid);
@@ -440,7 +467,10 @@ static void virtio_p9_walk(struct p9_dev *p9dev,
 		 */
 		pdu->write_offset += sizeof(u16);
 		old_fid = get_fid(p9dev, fid_val);
-		strcpy(new_fid->path, old_fid->path);
+		if (join_path(new_fid, old_fid->path) != 0) {
+			errno = ENAMETOOLONG;
+			goto err_out;
+		}
 		new_fid->uid    = old_fid->uid;
 	}
 	*outlen = pdu->write_offset;
@@ -476,7 +506,10 @@ static void virtio_p9_attach(struct p9_dev *p9dev,
 
 	fid = get_fid(p9dev, fid_val);
 	fid->uid = uid;
-	strcpy(fid->path, "/");
+	if (join_path(fid, "/") != 0) {
+		errno = ENAMETOOLONG;
+		goto err_out;
+	}
 
 	virtio_p9_pdu_writef(pdu, "Q", &qid);
 	*outlen = pdu->write_offset;
@@ -1120,20 +1153,24 @@ static int virtio_p9_ancestor(char *path, char *ancestor)
 	return 0;
 }
 
-static void virtio_p9_fix_path(char *fid_path, char *old_name, char *new_name)
+static int virtio_p9_fix_path(struct p9_fid *fid, char *old_name, char *new_name)
 {
-	char tmp_name[PATH_MAX];
+	int ret;
+	char *p, tmp_name[PATH_MAX];
 	size_t rp_sz = strlen(old_name);
 
-	if (rp_sz == strlen(fid_path)) {
+	if (rp_sz == strlen(fid->path)) {
 		/* replace the full name */
-		strcpy(fid_path, new_name);
-		return;
+		p = new_name;
+	} else {
+		/* save the trailing path details */
+		ret = snprintf(tmp_name, sizeof(tmp_name), "%s%s", new_name, fid->path + rp_sz);
+		if (ret >= (int)sizeof(tmp_name))
+			return -1;
+		p = tmp_name;
 	}
-	/* save the trailing path details */
-	strcpy(tmp_name, fid_path + rp_sz);
-	sprintf(fid_path, "%s%s", new_name, tmp_name);
-	return;
+
+	return join_path(fid, p);
 }
 
 static void rename_fids(struct p9_dev *p9dev, char *old_name, char *new_name)
@@ -1144,7 +1181,7 @@ static void rename_fids(struct p9_dev *p9dev, char *old_name, char *new_name)
 		struct p9_fid *fid = rb_entry(node, struct p9_fid, node);
 
 		if (fid->fid != P9_NOFID && virtio_p9_ancestor(fid->path, old_name)) {
-				virtio_p9_fix_path(fid->path, old_name, new_name);
+				virtio_p9_fix_path(fid, old_name, new_name);
 		}
 		node = rb_next(node);
 	}
@@ -1544,7 +1581,9 @@ int virtio_9p__register(struct kvm *kvm, const char *root, const char *tag_name)
 		goto free_p9dev;
 	}
 
-	strcpy(p9dev->root_dir, root);
+	strncpy(p9dev->root_dir, root, sizeof(p9dev->root_dir));
+	p9dev->root_dir[sizeof(p9dev->root_dir)-1] = '\x00';
+
 	p9dev->config->tag_len = strlen(tag_name);
 	if (p9dev->config->tag_len > MAX_TAG_LEN) {
 		err = -EINVAL;
-- 
2.7.4

[PATCH 4/5] kvmtool: 9p: refactor fixes with get_full_path()

[G. Campana]

The code responsible of path verification is identical in several
functions. Move it to a new function.

Signed-off-by: G. Campana <gcampana+kvm@quarkslab.com>
---
 virtio/9p.c | 111 ++++++++++++++++++++----------------------------------------
 1 file changed, 36 insertions(+), 75 deletions(-)

diff --git a/virtio/9p.c b/virtio/9p.c
index 22aa268..b611643 100644
--- a/virtio/9p.c
+++ b/virtio/9p.c
@@ -244,6 +244,31 @@ static bool path_is_illegal(const char *path)
 	return false;
 }
 
+static int get_full_path_helper(char *full_path, size_t size,
+			 const char *dirname, const char *name)
+{
+	int ret;
+
+	ret = snprintf(full_path, size, "%s/%s", dirname, name);
+	if (ret >= (int)sizeof(full_path)) {
+		errno = ENAMETOOLONG;
+		return -1;
+	}
+
+	if (path_is_illegal(full_path)) {
+		errno = EACCES;
+		return -1;
+	}
+
+	return 0;
+}
+
+static int get_full_path(char *full_path, size_t size, struct p9_fid *fid,
+			 const char *name)
+{
+	return get_full_path_helper(full_path, size, fid->abs_path, name);
+}
+
 static void virtio_p9_open(struct p9_dev *p9dev,
 			   struct p9_pdu *pdu, u32 *outlen)
 {
@@ -298,18 +323,8 @@ static void virtio_p9_create(struct p9_dev *p9dev,
 			    &name, &flags, &mode, &gid);
 	dfid = get_fid(p9dev, dfid_val);
 
-	flags = virtio_p9_openflags(flags);
-
-	ret = snprintf(full_path, sizeof(full_path), "%s/%s", dfid->abs_path, name);
-	if (ret >= (int)sizeof(full_path)) {
-		errno = ENAMETOOLONG;
-		goto err_out;
-	}
-
-	if (path_is_illegal(full_path)) {
-		errno = EACCES;
+	if (get_full_path(full_path, sizeof(full_path), dfid, name) != 0)
 		goto err_out;
-	}
 
 	size = sizeof(dfid->abs_path) - (dfid->path - dfid->abs_path);
 	ret = snprintf(dfid->path, size, "%s/%s", dfid->path, name);
@@ -320,6 +335,8 @@ static void virtio_p9_create(struct p9_dev *p9dev,
 		goto err_out;
 	}
 
+	flags = virtio_p9_openflags(flags);
+
 	fd = open(full_path, flags | O_CREAT, mode);
 	if (fd < 0)
 		goto err_out;
@@ -359,16 +376,8 @@ static void virtio_p9_mkdir(struct p9_dev *p9dev,
 			    &name, &mode, &gid);
 	dfid = get_fid(p9dev, dfid_val);
 
-	ret = snprintf(full_path, sizeof(full_path), "%s/%s", dfid->abs_path, name);
-	if (ret >= (int)sizeof(full_path)) {
-		errno = ENAMETOOLONG;
-		goto err_out;
-	}
-
-	if (path_is_illegal(full_path)) {
-		errno = EACCES;
+	if (get_full_path(full_path, sizeof(full_path), dfid, name) != 0)
 		goto err_out;
-	}
 
 	ret = mkdir(full_path, mode);
 	if (ret < 0)
@@ -857,16 +866,8 @@ static void virtio_p9_rename(struct p9_dev *p9dev,
 	fid = get_fid(p9dev, fid_val);
 	new_fid = get_fid(p9dev, new_fid_val);
 
-	ret = snprintf(full_path, sizeof(full_path), "%s/%s", new_fid->abs_path, new_name);
-	if (ret >= (int)sizeof(full_path)) {
-		errno = ENAMETOOLONG;
-		goto err_out;
-	}
-
-	if (path_is_illegal(full_path)) {
-		errno = EACCES;
+	if (get_full_path(full_path, sizeof(full_path), new_fid, new_name) != 0)
 		goto err_out;
-	}
 
 	ret = rename(fid->abs_path, full_path);
 	if (ret < 0)
@@ -951,16 +952,9 @@ static void virtio_p9_mknod(struct p9_dev *p9dev,
 			    &major, &minor, &gid);
 
 	dfid = get_fid(p9dev, fid_val);
-	ret = snprintf(full_path, sizeof(full_path), "%s/%s", dfid->abs_path, name);
-	if (ret >= (int)sizeof(full_path)) {
-		errno = ENAMETOOLONG;
-		goto err_out;
-	}
 
-	if (path_is_illegal(full_path)) {
-		errno = EACCES;
+	if (get_full_path(full_path, sizeof(full_path), dfid, name) != 0)
 		goto err_out;
-	}
 
 	ret = mknod(full_path, mode, makedev(major, minor));
 	if (ret < 0)
@@ -1028,16 +1022,9 @@ static void virtio_p9_symlink(struct p9_dev *p9dev,
 	virtio_p9_pdu_readf(pdu, "dssd", &fid_val, &name, &old_path, &gid);
 
 	dfid = get_fid(p9dev, fid_val);
-	ret = snprintf(new_name, sizeof(new_name), "%s/%s", dfid->abs_path, name);
-	if (ret >= (int)sizeof(new_name)) {
-		errno = ENAMETOOLONG;
-		goto err_out;
-	}
 
-	if (path_is_illegal(new_name)) {
-		errno = EACCES;
+	if (get_full_path(new_name, sizeof(new_name), dfid, name) != 0)
 		goto err_out;
-	}
 
 	ret = symlink(old_path, new_name);
 	if (ret < 0)
@@ -1073,16 +1060,9 @@ static void virtio_p9_link(struct p9_dev *p9dev,
 
 	dfid = get_fid(p9dev, dfid_val);
 	fid =  get_fid(p9dev, fid_val);
-	ret = snprintf(full_path, sizeof(full_path), "%s/%s", dfid->abs_path, name);
-	if (ret >= (int)sizeof(full_path)) {
-		errno = ENAMETOOLONG;
-		goto err_out;
-	}
 
-	if (path_is_illegal(full_path)) {
-		errno = EACCES;
+	if (get_full_path(full_path, sizeof(full_path), dfid, name) != 0)
 		goto err_out;
-	}
 
 	ret = link(fid->abs_path, full_path);
 	if (ret < 0)
@@ -1203,22 +1183,11 @@ static void virtio_p9_renameat(struct p9_dev *p9dev,
 	old_dfid = get_fid(p9dev, old_dfid_val);
 	new_dfid = get_fid(p9dev, new_dfid_val);
 
-	ret = snprintf(old_full_path, sizeof(old_full_path), "%s/%s", old_dfid->abs_path, old_name);
-	if (ret >= (int)sizeof(old_full_path)) {
-		errno = ENAMETOOLONG;
-		goto err_out;
-	}
-
-	ret = snprintf(new_full_path, sizeof(new_full_path), "%s/%s", new_dfid->abs_path, new_name);
-	if (ret >= (int)sizeof(new_full_path)) {
-		errno = ENAMETOOLONG;
+	if (get_full_path(old_full_path, sizeof(old_full_path), old_dfid, old_name) != 0)
 		goto err_out;
-	}
 
-	if (path_is_illegal(old_full_path) || path_is_illegal(new_full_path)) {
-		errno = EACCES;
+	if (get_full_path(new_full_path, sizeof(new_full_path), new_dfid, new_name) != 0)
 		goto err_out;
-	}
 
 	ret = rename(old_full_path, new_full_path);
 	if (ret < 0)
@@ -1252,16 +1221,8 @@ static void virtio_p9_unlinkat(struct p9_dev *p9dev,
 	virtio_p9_pdu_readf(pdu, "dsd", &fid_val, &name, &flags);
 	fid = get_fid(p9dev, fid_val);
 
-	ret = snprintf(full_path, sizeof(full_path), "%s/%s", fid->abs_path, name);
-	if (ret >= (int)sizeof(full_path)) {
-		errno = ENAMETOOLONG;
-		goto err_out;
-	}
-
-	if (path_is_illegal(full_path)) {
-		errno = EACCES;
+	if (get_full_path(full_path, sizeof(full_path), fid, name) != 0)
 		goto err_out;
-	}
 
 	ret = remove(full_path);
 	if (ret < 0)
-- 
2.7.4

[PATCH 5/5] kvmtool: 9p: fix a buffer overflow in rel_to_abs

[G. Campana]

Make use of get_full_path_helper() instead of sprintf.

Signed-off-by: G. Campana <gcampana+kvm@quarkslab.com>
---
 virtio/9p.c | 29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/virtio/9p.c b/virtio/9p.c
index b611643..09da7f3 100644
--- a/virtio/9p.c
+++ b/virtio/9p.c
@@ -91,15 +91,6 @@ static struct p9_fid *get_fid(struct p9_dev *p9dev, int fid)
 	return new;
 }
 
-/* Warning: Immediately use value returned from this function */
-static const char *rel_to_abs(struct p9_dev *p9dev,
-			      const char *path, char *abs_path)
-{
-	sprintf(abs_path, "%s/%s", p9dev->root_dir, path);
-
-	return abs_path;
-}
-
 static void stat2qid(struct stat *st, struct p9_qid *qid)
 {
 	*qid = (struct p9_qid) {
@@ -269,6 +260,19 @@ static int get_full_path(char *full_path, size_t size, struct p9_fid *fid,
 	return get_full_path_helper(full_path, size, fid->abs_path, name);
 }
 
+static int stat_rel(struct p9_dev *p9dev, const char *path, struct stat *st)
+{
+	char full_path[PATH_MAX];
+
+	if (get_full_path_helper(full_path, sizeof(full_path), p9dev->root_dir, path) != 0)
+		return -1;
+
+	if (lstat(full_path, st) != 0)
+		return -1;
+
+	return 0;
+}
+
 static void virtio_p9_open(struct p9_dev *p9dev,
 			   struct p9_pdu *pdu, u32 *outlen)
 {
@@ -443,7 +447,6 @@ static void virtio_p9_walk(struct p9_dev *p9dev,
 		for (i = 0; i < nwname; i++) {
 			struct stat st;
 			char tmp[PATH_MAX] = {0};
-			char full_path[PATH_MAX];
 			char *str;
 			int ret;
 
@@ -458,7 +461,7 @@ static void virtio_p9_walk(struct p9_dev *p9dev,
 
 			free(str);
 
-			if (lstat(rel_to_abs(p9dev, tmp, full_path), &st) < 0)
+			if (stat_rel(p9dev, tmp, &st) != 0)
 				goto err_out;
 
 			stat2qid(&st, &wqid);
@@ -612,7 +615,6 @@ static void virtio_p9_readdir(struct p9_dev *p9dev,
 	struct stat st;
 	struct p9_fid *fid;
 	struct dirent *dent;
-	char full_path[PATH_MAX];
 	u64 offset, old_offset;
 
 	rcount = 0;
@@ -643,7 +645,8 @@ static void virtio_p9_readdir(struct p9_dev *p9dev,
 			break;
 		}
 		old_offset = dent->d_off;
-		lstat(rel_to_abs(p9dev, dent->d_name, full_path), &st);
+		if (stat_rel(p9dev, dent->d_name, &st) != 0)
+			memset(&st, -1, sizeof(st));
 		stat2qid(&st, &qid);
 		read = pdu->write_offset;
 		virtio_p9_pdu_writef(pdu, "Qqbs", &qid, dent->d_off,
-- 
2.7.4

Re: [PATCH 0/5] [PATCH v2] kvmtool: fix virtio 9p vulnerabilities

[Will Deacon]

On Thu, Nov 10, 2016 at 04:21:06PM +0100, G. Campana wrote:
> This patch series should fix different vulnerabilities found in virtio 9p
> (http://www.spinics.net/lists/kvm/msg130505.html), but it definitely needs some
> testing. By the way, the very same path traversal vulnerability was also found
> in Qemu in August: http://www.openwall.com/lists/oss-security/2016/08/30/1
> and the path traversal fix looks quite similar.

I applied patches 1-4, but patch 5 actually breaks things for me:

[    0.659365] Freeing unused kernel memory: 1024K (ffff800000c50000 - ffff800000d50000)
[    0.661269] Kernel panic - not syncing: Requested init /virt/init failed (error -36).
[    0.662542] CPU: 3 PID: 1 Comm: swapper/0 Not tainted 4.9.0-rc4-00005-gf43365ee17f8 #1
[    0.664009] Hardware name: linux,dummy-virt (DT)
[    0.664868] Call trace:
[    0.665332] [<ffff000008088428>] dump_backtrace+0x0/0x1a8
[    0.666342] [<ffff0000080885e4>] show_stack+0x14/0x20
[    0.667284] [<ffff000008376fac>] dump_stack+0x94/0xb8
[    0.668236] [<ffff000008166d64>] panic+0x114/0x27c
[    0.669131] [<ffff00000889bc30>] kernel_init+0xa0/0x100
[    0.670112] [<ffff000008082e80>] ret_from_fork+0x10/0x50
[    0.671118] SMP: stopping secondary CPUs
[    0.682308] Kernel Offset: disabled
[    0.682889] Memory Limit: none
[    0.683390] ---[ end Kernel panic - not syncing: Requested init /virt/init failed (error -36).

I tried replacing the memset of -1 with code to skip to the next file,
but that didn't seem to help.

Will

Re: [PATCH 0/5] [PATCH v2] kvmtool: fix virtio 9p vulnerabilities

[G. Campana]

On 11/18/2016 06:55 PM, Will Deacon wrote:
> On Thu, Nov 10, 2016 at 04:21:06PM +0100, G. Campana wrote:
>> This patch series should fix different vulnerabilities found in virtio 9p
>> (http://www.spinics.net/lists/kvm/msg130505.html), but it definitely needs some
>> testing. By the way, the very same path traversal vulnerability was also found
>> in Qemu in August: http://www.openwall.com/lists/oss-security/2016/08/30/1
>> and the path traversal fix looks quite similar.
> 
> I applied patches 1-4, but patch 5 actually breaks things for me:
> 
> [    0.659365] Freeing unused kernel memory: 1024K (ffff800000c50000 - ffff800000d50000)
> [    0.661269] Kernel panic - not syncing: Requested init /virt/init failed (error -36).
> [    0.662542] CPU: 3 PID: 1 Comm: swapper/0 Not tainted 4.9.0-rc4-00005-gf43365ee17f8 #1
> [    0.664009] Hardware name: linux,dummy-virt (DT)
> [    0.664868] Call trace:
> [    0.665332] [<ffff000008088428>] dump_backtrace+0x0/0x1a8
> [    0.666342] [<ffff0000080885e4>] show_stack+0x14/0x20
> [    0.667284] [<ffff000008376fac>] dump_stack+0x94/0xb8
> [    0.668236] [<ffff000008166d64>] panic+0x114/0x27c
> [    0.669131] [<ffff00000889bc30>] kernel_init+0xa0/0x100
> [    0.670112] [<ffff000008082e80>] ret_from_fork+0x10/0x50
> [    0.671118] SMP: stopping secondary CPUs
> [    0.682308] Kernel Offset: disabled
> [    0.682889] Memory Limit: none
> [    0.683390] ---[ end Kernel panic - not syncing: Requested init /virt/init failed (error -36).
> 
> I tried replacing the memset of -1 with code to skip to the next file,
> but that didn't seem to help.
> 
> Will
> 
I introduced an error in patch 4 of v2: sizeof(full_path) must be
replaced by size.

+	ret = snprintf(full_path, size, "%s/%s", dirname, name);
+	if (ret >= (int)sizeof(full_path)) {

I send a new patch series right now.

Re: [PATCH 0/5] [PATCH v2] kvmtool: fix virtio 9p vulnerabilities

[Andre Przywara]

Hi,

On 21/11/16 10:25, G. Campana wrote:
> On 11/18/2016 06:55 PM, Will Deacon wrote:
>> On Thu, Nov 10, 2016 at 04:21:06PM +0100, G. Campana wrote:
>>> This patch series should fix different vulnerabilities found in virtio 9p
>>> (http://www.spinics.net/lists/kvm/msg130505.html), but it definitely needs some
>>> testing. By the way, the very same path traversal vulnerability was also found
>>> in Qemu in August: http://www.openwall.com/lists/oss-security/2016/08/30/1
>>> and the path traversal fix looks quite similar.
>>
>> I applied patches 1-4, but patch 5 actually breaks things for me:

You seem to have missed this sentence: Will has merged the first four
patches already, please update your repository from [1].

>>
>> [    0.659365] Freeing unused kernel memory: 1024K (ffff800000c50000 - ffff800000d50000)
>> [    0.661269] Kernel panic - not syncing: Requested init /virt/init failed (error -36).
>> [    0.662542] CPU: 3 PID: 1 Comm: swapper/0 Not tainted 4.9.0-rc4-00005-gf43365ee17f8 #1
>> [    0.664009] Hardware name: linux,dummy-virt (DT)
>> [    0.664868] Call trace:
>> [    0.665332] [<ffff000008088428>] dump_backtrace+0x0/0x1a8
>> [    0.666342] [<ffff0000080885e4>] show_stack+0x14/0x20
>> [    0.667284] [<ffff000008376fac>] dump_stack+0x94/0xb8
>> [    0.668236] [<ffff000008166d64>] panic+0x114/0x27c
>> [    0.669131] [<ffff00000889bc30>] kernel_init+0xa0/0x100
>> [    0.670112] [<ffff000008082e80>] ret_from_fork+0x10/0x50
>> [    0.671118] SMP: stopping secondary CPUs
>> [    0.682308] Kernel Offset: disabled
>> [    0.682889] Memory Limit: none
>> [    0.683390] ---[ end Kernel panic - not syncing: Requested init /virt/init failed (error -36).
>>
>> I tried replacing the memset of -1 with code to skip to the next file,
>> but that didn't seem to help.
>>
>> Will
>>
> I introduced an error in patch 4 of v2: sizeof(full_path) must be
> replaced by size.
> 
> +	ret = snprintf(full_path, size, "%s/%s", dirname, name);
> +	if (ret >= (int)sizeof(full_path)) {

Can you do a patch on top of the latest HEAD?

Cheers,
Andre.

[1] git://git.kernel.org/pub/scm/linux/kernel/git/will/kvmtool.git

[PATCH 0/5] [PATCH v2] kvmtool: 9p: fix regression introduced by previous patch

[G. Campana]

Signed-off-by: G. Campana <gcampana+kvm@quarkslab.com>
---
 virtio/9p.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/virtio/9p.c b/virtio/9p.c
index 337bf75..7185bb7 100644
--- a/virtio/9p.c
+++ b/virtio/9p.c
@@ -250,7 +250,7 @@ static int get_full_path_helper(char *full_path, size_t size,
 	int ret;
 
 	ret = snprintf(full_path, size, "%s/%s", dirname, name);
-	if (ret >= (int)sizeof(full_path)) {
+	if (ret >= (int)size) {
 		errno = ENAMETOOLONG;
 		return -1;
 	}
-- 
2.7.4