Re: [Virtio-fs] [PATCH v2] virtiofsd: prevent opening of special files (CVE-2020-35517)

[Greg Kurz]

On Wed, 27 Jan 2021 16:52:56 +0100
Miklos Szeredi <mszeredi@redhat.com> wrote:

> On Wed, Jan 27, 2021 at 4:47 PM Miklos Szeredi <mszeredi@redhat.com> wrote:
> >
> > On Wed, Jan 27, 2021 at 4:35 PM Greg Kurz <groug@kaod.org> wrote:
> > >
> > > On Wed, 27 Jan 2021 16:22:49 +0100
> > > Miklos Szeredi <mszeredi@redhat.com> wrote:
> > >
> > > > On Wed, Jan 27, 2021 at 4:09 PM Greg Kurz <groug@kaod.org> wrote:
> > > > >
> > > > > On Wed, 27 Jan 2021 15:09:50 +0100
> > > > > Miklos Szeredi <mszeredi@redhat.com> wrote:
> > > > > > The semantics of O_CREATE are that it can fail neither because the
> > > > > > file exists nor because it doesn't.  This doesn't matter if the
> > > > > > exported tree is not modified outside of a single guest, because of
> > > > > > locking provided by the guest kernel.
> > > > > >
> > > > >
> > > > > Wrong. O_CREAT can legitimately fail with ENOENT if one element
> > > >
> > > > Let me make my  statement more precise:
> > > >
> > > > O_CREAT cannot fail with ENOENT if parent directory exists throughout
> > > > the operation.
> > > >
> > >
> > > True, but I still don't see what guarantees guest userspace that the
> > > parent directory doesn't go away... I must have missed something.
> > > Please elaborate.
> >
> > Generally there's no guarantee, however there can be certain
> > situations where the caller can indeed rely on the existence of the
> > parent (e.g. /tmp).
> 
> Example from the virtiofs repo:
> 
> https://gitlab.com/virtio-fs/ireg/-/blob/master/ireg.c#L315
> https://gitlab.com/virtio-fs/ireg/-/blob/master/ireg.c#L382
> 
> In that case breaking O_CREAT would be harmless, since no two
> instances are allowed anyway, so it would just give a confusing error.
> But it is breakage and it probably wouldn't be hard to find much worse
> breakage in real life applications.
> 

Ok, I get your point : a guest userspace application can't expect
to hit ENOENT when doing open("/some_dir/foo", O_CREAT) even if
someone else is doing unlink("/some_dir/foo"), which is a different
case than somebody doing rmdir("/some_dir").

But we still have a TOCTOU : the open(O_CREAT|O_EXCL) acts as
the check to use open(O_PATH) and retry+timeout can't fix it
reliably.

A possible fix for the case where the race comes from the
client itself would be to serialize FUSE requests that
create/remove paths in the same directory. I don't know
enough the code yet to assess if it's doable though.

Then this would leave the case where something else on
the host is racing with virtiofsd. IMHO this belongs to
the broader family of "bad things the host can do
in our back". This requires a bigger hammer than
adding band-aids here and there IMHO. So in the
scope of this patch, I don't think we should retry
and timeout, but just return whatever errno that
makes sense.

> Thanks,
> Miklos
>