tools/virtiofsd/passthrough_ll.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+)
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@gmail.com>
Suggested-by: Miklos Szeredi <mszeredi@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Vivek Goyal <vgoyal@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;
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@gmail.com> > Suggested-by: Miklos Szeredi <mszeredi@redhat.com> > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > Signed-off-by: Vivek Goyal <vgoyal@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
* Stefan Hajnoczi (stefanha@redhat.com) wrote: > 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@gmail.com> > > Suggested-by: Miklos Szeredi <mszeredi@redhat.com> > > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> > > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > Signed-off-by: Vivek Goyal <vgoyal@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 Actually, I just posted it as a separate pull by itself. (I added {}'s around the if (!ret) { return; } to meet Qemu style guides). Dave > Stefan -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
© 2016 - 2024 Red Hat, Inc.