On Fri, 19 Aug 2016 15:55:17 +0100
Peter Maydell <peter.mayd...@linaro.org> wrote:
> On 11 August 2016 at 06:13, P J P <ppan...@redhat.com> wrote:
> > From: Prasad J Pandit <p...@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 <fwilh...@ernw.de>
> > Signed-off-by: Prasad J Pandit <p...@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
