Re: [Qemu-devel] [PATCH] 9pfs: add check for relative path

[Greg Kurz <groug@kaod.org>]

On Fri, 19 Aug 2016 15:55:17 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:

> On 11 August 2016 at 06:13, P J P <ppandit@redhat.com> wrote:
> > From: Prasad J Pandit <pjp@fedoraproject.org>
> >
> > At various places in 9pfs back-end, it creates full path by
> > concatenating two path strings. It could lead to a path
> > traversal issue if one of the parameter was a relative path.
> > Add check to avoid it.
> >
> > Reported-by: Felix Wilhelm <fwilhelm@ernw.de>
> > Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> > ---
> >  hw/9pfs/9p-local.c | 31 +++++++++++++++++++++++++++----
> >  1 file changed, 27 insertions(+), 4 deletions(-)
> >
> > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> > index 3f271fc..c20331a 100644
> > --- a/hw/9pfs/9p-local.c
> > +++ b/hw/9pfs/9p-local.c
> > @@ -493,6 +493,9 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath *dir_path,
> >      char *buffer = NULL;
> >
> >      v9fs_string_init(&fullname);
> > +    if (strstr(name, "../")) {
> > +        return err;
> > +    }  
> 
> I think we also need to set errno in these error paths: the functions
> which call all these hooks in hw/9pfs/cofs.c all do:
>           err = s->ops->mknod(&s->ctx, &fidp->path, name->data, &cred);
>           if (err < 0) {
>               err = -errno;
>           } else {
>               /* success path */
>           }
>           return err;
> 
> so we must set errno appropriately if we're going to return -1.
> 

Indeed. Thanks Peter for pointing this out !

> Also, strstr(name, "../") is the wrong check. There are I think
> two possibilities here:
> 
> (1) the "name" parameter may only validly be a single pathname
> component. In this case we should be enforcing this by treating
> any string with a "/" in it as an error (and checking for "../"
> is not catching all the cases that should be errors).
> 
> (2) the "name" parameter may be a multiple-pathname-component value.
> In this case "../" catches too many cases, because "foo../bar" is
> a valid string which is not relative. You would need to check for
> (contains "/../" OR starts with "../" OR ends with "/.." OR is "..").
> 
> 
> On IRC Greg and I discussed this and Greg suggested that
> case (1) is what we have. We should check this though.
> 

And this seems to be the case indeed when looking at:

http://man.cat-v.org/plan_9/5/walk

It is also ok for a system to reduce the valid character set
for path components according to:

http://man.cat-v.org/plan_9/5/intro

Peter's suggestion to forbid / in names seems the way to go.

> Finally: what about the functions in this file which
> create a local filename by calling rpath() ? Are those
> all definitely OK or do some or all of those also need
> check code?
> 

This needs a closer look.

> thanks
> -- PMM

Thanks for your assistance Peter !

--
Greg