* [PATCH 1/5] kvmtool: 9p: fix path traversal vulnerabilities
2016-11-10 15:21 [PATCH 0/5] [PATCH v2] kvmtool: fix virtio 9p vulnerabilities G. Campana
@ 2016-11-10 15:21 ` G. Campana
2016-11-10 15:21 ` [PATCH 2/5] kvmtool: 9p: fix sprintf vulnerabilities G. Campana
` (4 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: G. Campana @ 2016-11-10 15:21 UTC (permalink / raw)
To: Will.Deacon; +Cc: kvm, andre.przywara, gcampana+kvm
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/5] kvmtool: 9p: fix sprintf vulnerabilities
2016-11-10 15:21 [PATCH 0/5] [PATCH v2] kvmtool: fix virtio 9p vulnerabilities G. Campana
2016-11-10 15:21 ` [PATCH 1/5] kvmtool: 9p: fix path traversal vulnerabilities G. Campana
@ 2016-11-10 15:21 ` G. Campana
2016-11-10 15:21 ` [PATCH 3/5] kvmtool: 9p: fix strcpy vulnerabilities G. Campana
` (3 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: G. Campana @ 2016-11-10 15:21 UTC (permalink / raw)
To: Will.Deacon; +Cc: kvm, andre.przywara, gcampana+kvm
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 3/5] kvmtool: 9p: fix strcpy vulnerabilities
2016-11-10 15:21 [PATCH 0/5] [PATCH v2] kvmtool: fix virtio 9p vulnerabilities G. Campana
2016-11-10 15:21 ` [PATCH 1/5] kvmtool: 9p: fix path traversal vulnerabilities G. Campana
2016-11-10 15:21 ` [PATCH 2/5] kvmtool: 9p: fix sprintf vulnerabilities G. Campana
@ 2016-11-10 15:21 ` G. Campana
2016-11-10 15:21 ` [PATCH 4/5] kvmtool: 9p: refactor fixes with get_full_path() G. Campana
` (2 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: G. Campana @ 2016-11-10 15:21 UTC (permalink / raw)
To: Will.Deacon; +Cc: kvm, andre.przywara, gcampana+kvm
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 4/5] kvmtool: 9p: refactor fixes with get_full_path()
2016-11-10 15:21 [PATCH 0/5] [PATCH v2] kvmtool: fix virtio 9p vulnerabilities G. Campana
` (2 preceding siblings ...)
2016-11-10 15:21 ` [PATCH 3/5] kvmtool: 9p: fix strcpy vulnerabilities G. Campana
@ 2016-11-10 15:21 ` G. Campana
2016-11-10 15:21 ` [PATCH 5/5] kvmtool: 9p: fix a buffer overflow in rel_to_abs G. Campana
2016-11-18 17:55 ` [PATCH 0/5] [PATCH v2] kvmtool: fix virtio 9p vulnerabilities Will Deacon
5 siblings, 0 replies; 10+ messages in thread
From: G. Campana @ 2016-11-10 15:21 UTC (permalink / raw)
To: Will.Deacon; +Cc: kvm, andre.przywara, gcampana+kvm
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 5/5] kvmtool: 9p: fix a buffer overflow in rel_to_abs
2016-11-10 15:21 [PATCH 0/5] [PATCH v2] kvmtool: fix virtio 9p vulnerabilities G. Campana
` (3 preceding siblings ...)
2016-11-10 15:21 ` [PATCH 4/5] kvmtool: 9p: refactor fixes with get_full_path() G. Campana
@ 2016-11-10 15:21 ` G. Campana
2016-11-18 17:55 ` [PATCH 0/5] [PATCH v2] kvmtool: fix virtio 9p vulnerabilities Will Deacon
5 siblings, 0 replies; 10+ messages in thread
From: G. Campana @ 2016-11-10 15:21 UTC (permalink / raw)
To: Will.Deacon; +Cc: kvm, andre.przywara, gcampana+kvm
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 0/5] [PATCH v2] kvmtool: fix virtio 9p vulnerabilities
2016-11-10 15:21 [PATCH 0/5] [PATCH v2] kvmtool: fix virtio 9p vulnerabilities G. Campana
` (4 preceding siblings ...)
2016-11-10 15:21 ` [PATCH 5/5] kvmtool: 9p: fix a buffer overflow in rel_to_abs G. Campana
@ 2016-11-18 17:55 ` Will Deacon
2016-11-21 10:25 ` G. Campana
5 siblings, 1 reply; 10+ messages in thread
From: Will Deacon @ 2016-11-18 17:55 UTC (permalink / raw)
To: G. Campana; +Cc: kvm, andre.przywara
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/5] [PATCH v2] kvmtool: fix virtio 9p vulnerabilities
2016-11-18 17:55 ` [PATCH 0/5] [PATCH v2] kvmtool: fix virtio 9p vulnerabilities Will Deacon
@ 2016-11-21 10:25 ` G. Campana
2016-11-21 10:33 ` Andre Przywara
0 siblings, 1 reply; 10+ messages in thread
From: G. Campana @ 2016-11-21 10:25 UTC (permalink / raw)
To: Will Deacon, G. Campana; +Cc: kvm, andre.przywara
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/5] [PATCH v2] kvmtool: fix virtio 9p vulnerabilities
2016-11-21 10:25 ` G. Campana
@ 2016-11-21 10:33 ` Andre Przywara
2016-11-21 10:48 ` [PATCH 0/5] [PATCH v2] kvmtool: 9p: fix regression introduced by previous patch G. Campana
0 siblings, 1 reply; 10+ messages in thread
From: Andre Przywara @ 2016-11-21 10:33 UTC (permalink / raw)
To: G. Campana, Will Deacon; +Cc: kvm
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 0/5] [PATCH v2] kvmtool: 9p: fix regression introduced by previous patch
2016-11-21 10:33 ` Andre Przywara
@ 2016-11-21 10:48 ` G. Campana
0 siblings, 0 replies; 10+ messages in thread
From: G. Campana @ 2016-11-21 10:48 UTC (permalink / raw)
To: Will.Deacon; +Cc: kvm, andre.przywara, 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
^ permalink raw reply related [flat|nested] 10+ messages in thread