Re: [Virtio-fs] [PATCH] virtiofsd: jail lo->proc_self_fd

[Miklos Szeredi]

On Wed, Apr 29, 2020 at 5:00 PM Vivek Goyal <address@hidden> wrote:
>
> On Wed, Apr 29, 2020 at 04:47:19PM +0200, Miklos Szeredi wrote:
> > On Wed, Apr 29, 2020 at 4:36 PM Vivek Goyal <address@hidden> wrote:
> > >
> > > On Wed, Apr 29, 2020 at 02:47:33PM +0200, Miklos Szeredi wrote:
> > > > While it's not possible to escape the proc filesystem through
> > > > lo->proc_self_fd, it is possible to escape to the root of the proc
> > > > filesystem itself through "../..".
> > >
> > > Hi Miklos,
> > >
> > > So this attack will work with some form of *at(lo->proc_self_fd, "../..")
> > > call?
> >
> > Right.
> >
> > >
> > > >
> > > > Use a temporary mount for opening lo->proc_self_fd, that has it's root 
> > > > at
> > > > /proc/self/fd/, preventing access to the ancestor directories.
> > >
> > > Does this mean that now similar attack can happen using "../.." on tmpdir
> > > fd instead and be able to look at peers of tmpdir. Or it is blocked
> > > due to mount point or something else.
> >
> > No, because tmpdir is detached, the root of that tree will be the
> > directory pointed to by the fd.  ".." will just lead to the same
> > directory.
>
> Aha.. got it. Thanks.
>
> One more question. We seem to be using PID namespaces. Can't we mount
> fresh instance of proc so that we don't see other processes apart from
> virtiofd. May be we are already doing it. I have not checked it yet. If
> yes, that should mitigate this concern?

Yes, it's doing that already just above this patch:

    /* The child must remount /proc to use the new pid namespace */
    if (mount("proc", "/proc", "proc",
              MS_NODEV | MS_NOEXEC | MS_NOSUID | MS_RELATIME, NULL) < 0) {
        fuse_log(FUSE_LOG_ERR, "mount(/proc): %m\n");
        exit(1);
    }

Thanks,
Miklos