Re: kvmtool: vulnerabilities in 9p virtio

["André Przywara" <andre.przywara@arm.com>]

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
> --