From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44842) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bXjXA-0005y8-6n for qemu-devel@nongnu.org; Thu, 11 Aug 2016 02:32:05 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bXjX5-0004I5-TX for qemu-devel@nongnu.org; Thu, 11 Aug 2016 02:32:03 -0400 Received: from mx0b-001b2d01.pphosted.com ([148.163.158.5]:7648 helo=mx0a-001b2d01.pphosted.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bXjX5-0004Hz-OV for qemu-devel@nongnu.org; Thu, 11 Aug 2016 02:31:59 -0400 Received: from pps.filterd (m0098414.ppops.net [127.0.0.1]) by mx0b-001b2d01.pphosted.com (8.16.0.11/8.16.0.11) with SMTP id u7B6U7PA086570 for ; Thu, 11 Aug 2016 02:31:58 -0400 Received: from e36.co.us.ibm.com (e36.co.us.ibm.com [32.97.110.154]) by mx0b-001b2d01.pphosted.com with ESMTP id 24qm9sxtqw-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Thu, 11 Aug 2016 02:31:58 -0400 Received: from localhost by e36.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 11 Aug 2016 00:31:57 -0600 From: "Aneesh Kumar K.V" In-Reply-To: <1470892391-4917-1-git-send-email-ppandit@redhat.com> References: <1470892391-4917-1-git-send-email-ppandit@redhat.com> Date: Thu, 11 Aug 2016 12:01:46 +0530 MIME-Version: 1.0 Content-Type: text/plain Message-Id: <87popfzui5.fsf@skywalker.in.ibm.com> Subject: Re: [Qemu-devel] [PATCH] 9pfs: add check for relative path List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: P J P , Qemu Developers Cc: Prasad J Pandit , Greg Kurz , "Michael S. Tsirkin" , Felix Wilhelm P J P writes: > From: Prasad J Pandit > > 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 > Signed-off-by: Prasad J Pandit 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