On Thu, 11 Aug 2016 12:01:46 +0530
"Aneesh Kumar K.V" <aneesh.ku...@linux.vnet.ibm.com> wrote:
> P J P <ppan...@redhat.com> writes:
>
> > 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>
>
> I am not sure relative path names need to be completely disallowed. What
The official linux client does not send relative paths: all ".." path
components are resolved in the VFS layer IIUC.
> we need is to disallow the access outside export path. virtfs-proxy was
> done primarily to handle such things.
My understanding is that the virtfs proxy was created to handle the case when
QEMU is running as non-root but root privilege is needed: to be able to call
lchown() or chmod() on any uid/gid when using the passthrough security model
for example.
> It does a chroot to the export path.
>
Yes it does since it is running as root and isn't supposed to access
anything outside the 9p mount path. But that doesn't help here since
the issue affects 9p-local, which is run by the QEMU process.
>
> > ---
> > 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
>
