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

[Aneesh Kumar K.V]

P J P <address@hidden> writes:

> 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>

I am not sure relative path names need to be completely disallowed. What
we need is to disallow the access outside export path. virtfs-proxy was
done primarily to handle such things. It does a chroot to the export
path. 


> ---
>  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) {
> -- 
> 2.5.5