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

[Peter Maydell]

On 11 August 2016 at 06:13, P J P <address@hidden> wrote:
> From: Prasad J Pandit <address@hidden>
>
> 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 <address@hidden>
> Signed-off-by: Prasad J Pandit <address@hidden>
> ---
>  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.

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.

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?

thanks
-- PMM