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 <shawtao1...@gmail.com> > Suggested-by: Miklos Szeredi <mszer...@redhat.com> > Reviewed-by: Stefan Hajnoczi <stefa...@redhat.com> > Reviewed-by: Dr. David Alan Gilbert <dgilb...@redhat.com> > Signed-off-by: Vivek Goyal <vgo...@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
signature.asc
Description: PGP signature