On 11 August 2016 at 06:13, P J P <ppan...@redhat.com> wrote:
> 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>
> ---
> 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
