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

[Greg Kurz <groug@kaod.org>]

On Thu, 11 Aug 2016 10:43:11 +0530
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>
> ---

With GDB attached to my QEMU, I could easily change the path names to have a
".." component just like a malicious guest would do, and cause the path traversal
issue to happen with renameat at least. And this patch indeed fixes the issue in
this case.

I'm still on holidays and I could not find time to do more testing and reviewing
though. But since the official linux 9p client does not send relative paths, I
don't expect any regression to happen if we apply the whole patch. I hence give:

Acked-by: Greg Kurz <groug@kaod.org>

Since this is a serious security issue, I shall send a pull request for this
patch tomorrow so that it gets into rc4. Also Cc'ing stable in case we deliver
another 2.6.x release one day.

If someone disagrees, please speak up ASAP.

Aneesh ? Michael ? Peter ?

>  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;
> +    }
>      v9fs_string_sprintf(&fullname, "%s/%s", dir_path->data, name);
>      path = fullname.data;
>  
> @@ -554,6 +557,9 @@ static int local_mkdir(FsContext *fs_ctx, V9fsPath *dir_path,
>      char *buffer = NULL;
>  
>      v9fs_string_init(&fullname);
> +    if (strstr(name, "../")) {
> +        return err;
> +    }
>      v9fs_string_sprintf(&fullname, "%s/%s", dir_path->data, name);
>      path = fullname.data;
>  
> @@ -663,6 +669,9 @@ static int local_open2(FsContext *fs_ctx, V9fsPath *dir_path, const char *name,
>      flags |= O_NOFOLLOW;
>  
>      v9fs_string_init(&fullname);
> +    if (strstr(name, "../")) {
> +        return err;
> +    }
>      v9fs_string_sprintf(&fullname, "%s/%s", dir_path->data, name);
>      path = fullname.data;
>  
> @@ -734,6 +743,9 @@ static int local_symlink(FsContext *fs_ctx, const char *oldpath,
>      char *buffer = NULL;
>  
>      v9fs_string_init(&fullname);
> +    if (strstr(name, "../")) {
> +        return err;
> +    }
>      v9fs_string_sprintf(&fullname, "%s/%s", dir_path->data, name);
>      newpath = fullname.data;
>  
> @@ -830,11 +842,14 @@ out:
>  static int local_link(FsContext *ctx, V9fsPath *oldpath,
>                        V9fsPath *dirpath, const char *name)
>  {
> -    int ret;
> +    int ret = -1;
>      V9fsString newpath;
>      char *buffer, *buffer1;
>  
>      v9fs_string_init(&newpath);
> +    if (strstr(name, "../")) {
> +        return ret;
> +    }
>      v9fs_string_sprintf(&newpath, "%s/%s", dirpath->data, name);
>  
>      buffer = rpath(ctx, oldpath->data);
> @@ -1059,6 +1074,9 @@ static int local_lremovexattr(FsContext *ctx, V9fsPath *fs_path,
>  static int local_name_to_path(FsContext *ctx, V9fsPath *dir_path,
>                                const char *name, V9fsPath *target)
>  {
> +    if (strstr(name, "../")) {
> +        return -1;
> +    }
>      if (dir_path) {
>          v9fs_string_sprintf((V9fsString *)target, "%s/%s",
>                              dir_path->data, name);
> @@ -1074,12 +1092,15 @@ static int local_renameat(FsContext *ctx, V9fsPath *olddir,
>                            const char *old_name, V9fsPath *newdir,
>                            const char *new_name)
>  {
> -    int ret;
> +    int ret = -1;
>      V9fsString old_full_name, new_full_name;
>  
>      v9fs_string_init(&old_full_name);
>      v9fs_string_init(&new_full_name);
>  
> +    if (strstr(old_name, "../") || strstr(new_name, "../")) {
> +        return ret;
> +    }
>      v9fs_string_sprintf(&old_full_name, "%s/%s", olddir->data, old_name);
>      v9fs_string_sprintf(&new_full_name, "%s/%s", newdir->data, new_name);
>  
> @@ -1092,12 +1113,14 @@ static int local_renameat(FsContext *ctx, V9fsPath *olddir,
>  static int local_unlinkat(FsContext *ctx, V9fsPath *dir,
>                            const char *name, int flags)
>  {
> -    int ret;
> +    int ret = -1;
>      V9fsString fullname;
>      char *buffer;
>  
>      v9fs_string_init(&fullname);
> -
> +    if (strstr(name, "../")) {
> +        return ret;
> +    }
>      v9fs_string_sprintf(&fullname, "%s/%s", dir->data, name);
>      if (ctx->export_flags & V9FS_SM_MAPPED_FILE) {
>          if (flags == AT_REMOVEDIR) {