[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/7] 9pfs: restrict open to regular files and di
From: |
Greg Kurz |
Subject: |
Re: [Qemu-devel] [PATCH 1/7] 9pfs: restrict open to regular files and directories |
Date: |
Wed, 11 Jan 2017 10:54:13 +0100 |
On Tue, 10 Jan 2017 08:38:27 -0600
Eric Blake <address@hidden> wrote:
> On 01/10/2017 08:32 AM, Greg Kurz wrote:
> > It really does not make sense for the 9P server to open anything else but
> > a regular file or a directory.
> >
> > Malicious code in a guest could for example create a named pipe, associate
> > it to a valid fid and pass it to the server in a RLOPEN message. This would
> > cause QEMU to hang in open(), waiting for someone to open the other end of
> > the pipe.
> >
> > Signed-off-by: Greg Kurz <address@hidden>
> > ---
> > hw/9pfs/9p.c | 5 ++++-
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > index fa58877570f6..edd7b97270e3 100644
> > --- a/hw/9pfs/9p.c
> > +++ b/hw/9pfs/9p.c
> > @@ -1462,7 +1462,7 @@ static void coroutine_fn v9fs_open(void *opaque)
> > goto out;
> > }
> > err += offset;
> > - } else {
> > + } else if (S_ISREG(stbuf.st_mode)) {
> > if (s->proto_version == V9FS_PROTO_2000L) {
>
> TOCTTOU race. You are checking the stat() results and only then calling
> open(), rather than calling open() first and validating fstat(). That
> means the guest can STILL cause you to open() a pipe by changing the
> file type in between the stat and the open.
>
You're right. And of course, this TOCTTOU race also affects the
file versus directory choice: we can end up with a fid with type
P9_FID_FILE whereas the underlying fd points to a directory... not
sure how the rest of the code copes with that. :-\
Side note: support for older versions of the protocol greatly
contribute to the overall obfuscation of the code... I wonder
if we have non-V9FS_PROTO_2000L users, and how I could deprecate
the damn thing...
> I think you need to rework this patch to open() first, then validate
> (closing the fd if necessary); the open can be done with O_NONBLOCK to
> avoid hanging on a pipe. Yes, that's more annoying, but that's life
> with TOCTTOU races.
>
Yeah... and even if linux doesn't implement O_NONBLOCK behaviour for files,
for completeness we should revert this with fcntl(). The fix should hence
be done at the backend level.
Good news, at least: the directory branch will eventually call
opendir()->open(O_DIRECTORY) and thus doesn't need fixing.
Also the fixing of the regular file case will also take care of
the P9_FID_FILE/directory discrepancy mentioned above.
Thanks for the advice anyway. :)
--
Greg
pgpS9uKJepytE.pgp
Description: OpenPGP digital signature
- [Qemu-devel] [PATCH 0/7] fix potential hangs in 9pfs, Greg Kurz, 2017/01/10
- [Qemu-devel] [PATCH 1/7] 9pfs: restrict open to regular files and directories, Greg Kurz, 2017/01/10
- [Qemu-devel] [PATCH 2/7] tests: virtio-9p: improve error reporting, Greg Kurz, 2017/01/10
- [Qemu-devel] [PATCH 3/7] tests: virtio-9p: add LOPEN operation test, Greg Kurz, 2017/01/10
- [Qemu-devel] [PATCH 4/7] tests: virtio-9p: TLOPEN should fail to open a FIFO, Greg Kurz, 2017/01/10
- [Qemu-devel] [PATCH 5/7] 9pfs: don't create files if pathname already exists, Greg Kurz, 2017/01/10
- [Qemu-devel] [PATCH 6/7] tests: virtio-9p: add LCREATE operation test, Greg Kurz, 2017/01/10
- [Qemu-devel] [PATCH 7/7] tests: virtio-9p: TLCREATE should fail if pathname exists, Greg Kurz, 2017/01/10