Re: [Qemu-devel] [PATCH 1/7] 9pfs: restrict open to regular files and directories

[Greg Kurz <groug@kaod.org>]

[-- Attachment #1: Type: text/plain, Size: 2535 bytes --]

On Tue, 10 Jan 2017 08:38:27 -0600
Eric Blake <eblake@redhat.com> 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 <groug@kaod.org>
> > ---
> >  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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]