Re: [Qemu-devel] [PATCH 11/29] 9pfs: local: lremovexattr: don't follow symlinks

[Greg Kurz <groug@kaod.org>]

[-- Attachment #1: Type: text/plain, Size: 3930 bytes --]

On Mon, 20 Feb 2017 15:40:45 +0100
Greg Kurz <groug@kaod.org> wrote:

> The local_lremovexattr() callback is vulnerable to symlink attacks because
> it calls lremovexattr() which follows symbolic links in all path elements but
> the rightmost one.
> 
> This patch converts local_lremovexattr() to rely on opendir_nofollow() and
> fremovexattrat_nofollow() instead.
> 
> This partly fixes CVE-2016-9602.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/9pfs/9p-posix-acl.c  |   10 ++--------
>  hw/9pfs/9p-xattr-user.c |    8 +-------
>  hw/9pfs/9p-xattr.c      |    8 +-------
>  3 files changed, 4 insertions(+), 22 deletions(-)
> 
> diff --git a/hw/9pfs/9p-posix-acl.c b/hw/9pfs/9p-posix-acl.c
> index 0154e2a7605f..c20409104135 100644
> --- a/hw/9pfs/9p-posix-acl.c
> +++ b/hw/9pfs/9p-posix-acl.c
> @@ -58,10 +58,8 @@ static int mp_pacl_removexattr(FsContext *ctx,
>                                 const char *path, const char *name)
>  {
>      int ret;
> -    char *buffer;
>  
> -    buffer = rpath(ctx, path);
> -    ret  = lremovexattr(buffer, MAP_ACL_ACCESS);
> +    ret = local_removexattr_nofollow(ctx, MAP_ACL_ACCESS, name);

While reworking on a new implementation fremovexattrat(), I realized the
above should be:

    ret = local_removexattr_nofollow(ctx, path, MAP_ACL_ACCESS);

>      if (ret == -1 && errno == ENODATA) {
>          /*
>           * We don't get ENODATA error when trying to remove a
> @@ -71,7 +69,6 @@ static int mp_pacl_removexattr(FsContext *ctx,
>          errno = 0;
>          ret = 0;
>      }
> -    g_free(buffer);
>      return ret;
>  }
>  
> @@ -111,10 +108,8 @@ static int mp_dacl_removexattr(FsContext *ctx,
>                                 const char *path, const char *name)
>  {
>      int ret;
> -    char *buffer;
>  
> -    buffer = rpath(ctx, path);
> -    ret  = lremovexattr(buffer, MAP_ACL_DEFAULT);
> +    ret = local_removexattr_nofollow(ctx, MAP_ACL_DEFAULT, name);

Same error here.

>      if (ret == -1 && errno == ENODATA) {
>          /*
>           * We don't get ENODATA error when trying to remove a
> @@ -124,7 +119,6 @@ static int mp_dacl_removexattr(FsContext *ctx,
>          errno = 0;
>          ret = 0;
>      }
> -    g_free(buffer);
>      return ret;
>  }
>  
> diff --git a/hw/9pfs/9p-xattr-user.c b/hw/9pfs/9p-xattr-user.c
> index 1840a5db66f3..2c90817b75a6 100644
> --- a/hw/9pfs/9p-xattr-user.c
> +++ b/hw/9pfs/9p-xattr-user.c
> @@ -81,9 +81,6 @@ static int mp_user_setxattr(FsContext *ctx, const char *path, const char *name,
>  static int mp_user_removexattr(FsContext *ctx,
>                                 const char *path, const char *name)
>  {
> -    char *buffer;
> -    int ret;
> -
>      if (strncmp(name, "user.virtfs.", 12) == 0) {
>          /*
>           * Don't allow fetch of user.virtfs namesapce
> @@ -92,10 +89,7 @@ static int mp_user_removexattr(FsContext *ctx,
>          errno = EACCES;
>          return -1;
>      }
> -    buffer = rpath(ctx, path);
> -    ret = lremovexattr(buffer, name);
> -    g_free(buffer);
> -    return ret;
> +    return local_removexattr_nofollow(ctx, path, name);
>  }
>  
>  XattrOperations mapped_user_xattr = {
> diff --git a/hw/9pfs/9p-xattr.c b/hw/9pfs/9p-xattr.c
> index 72ef820f16d7..6fbfbca7e9a0 100644
> --- a/hw/9pfs/9p-xattr.c
> +++ b/hw/9pfs/9p-xattr.c
> @@ -333,13 +333,7 @@ int pt_setxattr(FsContext *ctx, const char *path, const char *name, void *value,
>  
>  int pt_removexattr(FsContext *ctx, const char *path, const char *name)
>  {
> -    char *buffer;
> -    int ret;
> -
> -    buffer = rpath(ctx, path);
> -    ret = lremovexattr(path, name);
> -    g_free(buffer);
> -    return ret;
> +    return local_removexattr_nofollow(ctx, path, name);
>  }
>  
>  ssize_t notsup_getxattr(FsContext *ctx, const char *path, const char *name,
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]