On Mon, Aug 03, 2020 at 09:57:15AM -0400, Vivek Goyal wrote:
> On Mon, Aug 03, 2020 at 10:54:59AM +0100, Stefan Hajnoczi wrote:
> > On Thu, Jul 30, 2020 at 03:47:35PM -0400, Vivek Goyal wrote:
> > > In sandbox=NONE mode, lo->source points to the directory which is being
> > > exported. We have not done any chroot()/pivot_root(). So open lo->source.
> > > 
> > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > > ---
> > >  tools/virtiofsd/passthrough_ll.c | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> > > index 76ef891105..a6fa816b6c 100644
> > > --- a/tools/virtiofsd/passthrough_ll.c
> > > +++ b/tools/virtiofsd/passthrough_ll.c
> > > @@ -3209,7 +3209,10 @@ static void setup_root(struct lo_data *lo, struct lo_inode *root)
> > >      int fd, res;
> > >      struct stat stat;
> > >  
> > > -    fd = open("/", O_PATH);
> > > +    if (lo->sandbox == SANDBOX_NONE)
> > > +        fd = open(lo->source, O_PATH);
> > > +    else
> > > +        fd = open("/", O_PATH);
> > 
> > Up until now virtiofsd has been able to assume that path traversal has
> > the shared directory as "/".
> > 
> > Now this is no longer true and it is necessary to audit all syscalls
> > that take path arguments. They must ensure that:
> > 1. Path components are safe (no ".." or "/" allowed)
> > 2. Symlinks are not followed.
> 
> This code does not change the path client is passing in and we are
> already doing the checks on passed in paths/names. So existing checks
> should work even for this case, isn't it?
> 
> lo_lookup() {
>     if (strchr(name, '/')) {
>         fuse_reply_err(req, EINVAL);
>         return;
>     }
> }
> 
> lo_do_lookup() {
>     /* Do not allow escaping root directory */
>     if (dir == &lo->root && strcmp(name, "..") == 0) {
>         name = ".";
>     }
> }

Yes, hopefully paths are already checked and syscalls do not follow
symlinks. However, we wouldn't have noticed if either of those were
missing since the pivot_root(2) ensured that path traversal stays within
the shared directory.

Now that a layer of protection has been removed it's necessary to check
again whether everything is really alright.

> 
> > 
> > Did you audit all syscalls made by passthrough_ll.c?
> > 
> > virtiofsd still needs to restrict the client to the shared directory for
> > two reasons:
> > 1. The guest may not be trusted. An unprivileged sandbox=none mount can
> >    be used with a malicious guest.
> > 2. If accidental escapes are possible then the guest could accidentally
> >    corrupt or delete files outside the shared directory.
> 
> Even if escape is possible, its no different than a malicious user
> application running. Given sandbox=none can be used in case of
> unpriviliged mode, that means user app can only affect files owned by
> that user.

Users may run untrusted guests. Those guests should not gain access to
the user's files outside the shared directory.

> If doing chroot/pivot_root is must, then we need additional capabilities.

I think it's not a must, but just an extra layer of security. What I
don't know 100% is whether virtiofsd accidentally relies on that extra
layer of security today since it never ran without it :).

Stefan