[Virtio-fs] [PATCH] virtiofsd: Drop membership of all supplementary groups (CVE-2022-0358)

[Stefan Hajnoczi]

On Tue, Jan 25, 2022 at 01:51:14PM -0500, Vivek Goyal wrote:
> At the start, drop membership of all supplementary groups. This is
> not required.
> 
> If we have membership of "root" supplementary group and when we switch
> uid/gid using setresuid/setsgid, we still retain membership of existing
> supplemntary groups. And that can allow some operations which are not
> normally allowed.
> 
> For example, if root in guest creates a dir as follows.
> 
> $ mkdir -m 03777 test_dir
> 
> This sets SGID on dir as well as allows unprivileged users to write into
> this dir. 
> 
> And now as unprivileged user open file as follows.
> 
> $ su test
> $ fd = open("test_dir/priviledge_id", O_RDWR|O_CREAT|O_EXCL, 02755);
> 
> This will create SGID set executable in test_dir/.
> 
> And that's a problem because now an unpriviliged user can execute it,
> get egid=0 and get access to resources owned by "root" group. This is
> privilege escalation.
> 
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2044863
> Fixes: CVE-2022-0358
> Reported-by: JIETAO XIAO <shawtao1125 at gmail.com>
> Suggested-by: Miklos Szeredi <mszeredi at redhat.com>
> Reviewed-by: Stefan Hajnoczi <stefanha at redhat.com>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert at redhat.com>
> Signed-off-by: Vivek Goyal <vgoyal at redhat.com>
> ---
>  tools/virtiofsd/passthrough_ll.c |   26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> Index: rhvgoyal-qemu/tools/virtiofsd/passthrough_ll.c
> ===================================================================
> --- rhvgoyal-qemu.orig/tools/virtiofsd/passthrough_ll.c	2022-01-25 13:38:59.349534531 -0500
> +++ rhvgoyal-qemu/tools/virtiofsd/passthrough_ll.c	2022-01-25 13:39:10.140177868 -0500
> @@ -54,6 +54,7 @@
>  #include <sys/wait.h>
>  #include <sys/xattr.h>
>  #include <syslog.h>
> +#include <grp.h>
>  
>  #include "qemu/cutils.h"
>  #include "passthrough_helpers.h"
> @@ -1161,6 +1162,29 @@ static void lo_lookup(fuse_req_t req, fu
>  #define OURSYS_setresuid SYS_setresuid
>  #endif
>  
> +static void drop_supplementary_groups(void)
> +{
> +    int ret;
> +
> +    ret = getgroups(0, NULL);
> +    if (ret == -1) {
> +        fuse_log(FUSE_LOG_ERR, "getgroups() failed with error=%d:%s\n",
> +                 errno, strerror(errno));
> +        exit(1);
> +    }
> +
> +    if (!ret)
> +        return;
> +
> +    /* Drop all supplementary groups. We should not need it */
> +    ret = setgroups(0, NULL);
> +    if (ret == -1) {
> +        fuse_log(FUSE_LOG_ERR, "setgroups() failed with error=%d:%s\n",
> +                 errno, strerror(errno));
> +        exit(1);
> +    }
> +}
> +
>  /*
>   * Change to uid/gid of caller so that file is created with
>   * ownership of caller.
> @@ -3926,6 +3950,8 @@ int main(int argc, char *argv[])
>  
>      qemu_init_exec_dir(argv[0]);
>  
> +    drop_supplementary_groups();
> +
>      pthread_mutex_init(&lo.mutex, NULL);
>      lo.inodes = g_hash_table_new(lo_key_hash, lo_key_equal);
>      lo.root.fd = -1;
> 

Thanks, applied to my block tree:
https://gitlab.com/stefanha/qemu/commits/block

Stefan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/virtio-fs/attachments/20220126/db50b333/attachment.sig>