[libvirt] [PATCH] [RFC] virSetUIDGID: Don't leak supplementary groups
The LXC driver uses virSetUIDGID() to become UID/GID 0. It passes an empty groups list to virSetUIDGID() to get rid of all supplementary groups from the host side. But virSetUIDGID() calls setgroups() only if the supplied list is larger than 0. This leads to a container root with unrelated supplementary groups. In most cases this issue is unoticed as libvirtd runs as UID/GID 0 without any supplementary groups. Signed-off-by: Richard Weinberger <richard@nod.at> --- I've marked that patch as RFC as I'm not sure if all users of virSetUIDGID() expect this behavior too. Thanks, //richard --- src/util/virutil.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/virutil.c b/src/util/virutil.c index cddc78a..ea697a3 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -1103,7 +1103,7 @@ virSetUIDGID(uid_t uid, gid_t gid, gid_t *groups ATTRIBUTE_UNUSED, } # if HAVE_SETGROUPS - if (ngroups && setgroups(ngroups, groups) < 0) { + if (setgroups(ngroups, groups) < 0) { virReportSystemError(errno, "%s", _("cannot set supplemental groups")); return -1; -- 2.4.2
On Tue, Jun 23, 2015 at 01:48:42PM +0200, Richard Weinberger wrote:
The LXC driver uses virSetUIDGID() to become UID/GID 0. It passes an empty groups list to virSetUIDGID() to get rid of all supplementary groups from the host side. But virSetUIDGID() calls setgroups() only if the supplied list is larger than 0. This leads to a container root with unrelated supplementary groups. In most cases this issue is unoticed as libvirtd runs as UID/GID 0 without any supplementary groups.
Signed-off-by: Richard Weinberger <richard@nod.at> --- I've marked that patch as RFC as I'm not sure if all users of virSetUIDGID() expect this behavior too.
I went through the callers and I see no reason why setgroups should not be called. ACK. I also can't think of a use case in which we'd like to keep the supplemental groups.
Thanks, //richard --- src/util/virutil.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/virutil.c b/src/util/virutil.c index cddc78a..ea697a3 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -1103,7 +1103,7 @@ virSetUIDGID(uid_t uid, gid_t gid, gid_t *groups ATTRIBUTE_UNUSED, }
# if HAVE_SETGROUPS - if (ngroups && setgroups(ngroups, groups) < 0) { + if (setgroups(ngroups, groups) < 0) { virReportSystemError(errno, "%s", _("cannot set supplemental groups")); return -1; -- 2.4.2
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Jun 24, 2015 at 11:19 AM, Martin Kletzander <mkletzan@redhat.com> wrote:
On Tue, Jun 23, 2015 at 01:48:42PM +0200, Richard Weinberger wrote:
The LXC driver uses virSetUIDGID() to become UID/GID 0. It passes an empty groups list to virSetUIDGID() to get rid of all supplementary groups from the host side. But virSetUIDGID() calls setgroups() only if the supplied list is larger than 0. This leads to a container root with unrelated supplementary groups. In most cases this issue is unoticed as libvirtd runs as UID/GID 0 without any supplementary groups.
Signed-off-by: Richard Weinberger <richard@nod.at> --- I've marked that patch as RFC as I'm not sure if all users of virSetUIDGID() expect this behavior too.
I went through the callers and I see no reason why setgroups should not be called. ACK. I also can't think of a use case in which we'd like to keep the supplemental groups.
Ping? -- Thanks, //richard
On Tue, Nov 17, 2015 at 10:02:36PM +0100, Richard Weinberger wrote:
On Wed, Jun 24, 2015 at 11:19 AM, Martin Kletzander <mkletzan@redhat.com> wrote:
On Tue, Jun 23, 2015 at 01:48:42PM +0200, Richard Weinberger wrote:
The LXC driver uses virSetUIDGID() to become UID/GID 0. It passes an empty groups list to virSetUIDGID() to get rid of all supplementary groups from the host side. But virSetUIDGID() calls setgroups() only if the supplied list is larger than 0. This leads to a container root with unrelated supplementary groups. In most cases this issue is unoticed as libvirtd runs as UID/GID 0 without any supplementary groups.
Signed-off-by: Richard Weinberger <richard@nod.at> --- I've marked that patch as RFC as I'm not sure if all users of virSetUIDGID() expect this behavior too.
I went through the callers and I see no reason why setgroups should not be called. ACK. I also can't think of a use case in which we'd like to keep the supplemental groups.
Ping?
Oh, sorry, I didn't realize you don't have push access. Would you happen to have these patches around somewhere? The originals got archived automatically. If you send them to me, I'll push them, it would be easier than me sucking it out of the ML archive (the same applies for the other patch: "bind mount container TTYs"). Martin
-- Thanks, //richard
On Wed, Nov 18, 2015 at 07:35:39AM +0100, Martin Kletzander wrote:
On Tue, Nov 17, 2015 at 10:02:36PM +0100, Richard Weinberger wrote:
On Wed, Jun 24, 2015 at 11:19 AM, Martin Kletzander <mkletzan@redhat.com> wrote:
On Tue, Jun 23, 2015 at 01:48:42PM +0200, Richard Weinberger wrote:
The LXC driver uses virSetUIDGID() to become UID/GID 0. It passes an empty groups list to virSetUIDGID() to get rid of all supplementary groups from the host side. But virSetUIDGID() calls setgroups() only if the supplied list is larger than 0. This leads to a container root with unrelated supplementary groups. In most cases this issue is unoticed as libvirtd runs as UID/GID 0 without any supplementary groups.
Signed-off-by: Richard Weinberger <richard@nod.at> --- I've marked that patch as RFC as I'm not sure if all users of virSetUIDGID() expect this behavior too.
I went through the callers and I see no reason why setgroups should not be called. ACK. I also can't think of a use case in which we'd like to keep the supplemental groups.
Ping?
Oh, sorry, I didn't realize you don't have push access. Would you happen to have these patches around somewhere? The originals got archived automatically. If you send them to me, I'll push them, it would be easier than me sucking it out of the ML archive (the same applies for the other patch: "bind mount container TTYs").
Don't worry, I've pushed them all. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Tue, Jun 23, 2015 at 01:48:42PM +0200, Richard Weinberger wrote:
The LXC driver uses virSetUIDGID() to become UID/GID 0. It passes an empty groups list to virSetUIDGID() to get rid of all supplementary groups from the host side. But virSetUIDGID() calls setgroups() only if the supplied list is larger than 0. This leads to a container root with unrelated supplementary groups. In most cases this issue is unoticed as libvirtd runs as UID/GID 0 without any supplementary groups.
Signed-off-by: Richard Weinberger <richard@nod.at> --- I've marked that patch as RFC as I'm not sure if all users of virSetUIDGID() expect this behavior too.
ACK & pushed - I concur with Martin that this is good practice. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Tue, Jun 23, 2015 at 01:48:42PM +0200, Richard Weinberger wrote:
The LXC driver uses virSetUIDGID() to become UID/GID 0. It passes an empty groups list to virSetUIDGID() to get rid of all supplementary groups from the host side. But virSetUIDGID() calls setgroups() only if the supplied list is larger than 0. This leads to a container root with unrelated supplementary groups. In most cases this issue is unoticed as libvirtd runs as UID/GID 0 without any supplementary groups.
Signed-off-by: Richard Weinberger <richard@nod.at> --- I've marked that patch as RFC as I'm not sure if all users of virSetUIDGID() expect this behavior too.
Thanks, //richard --- src/util/virutil.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/virutil.c b/src/util/virutil.c index cddc78a..ea697a3 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -1103,7 +1103,7 @@ virSetUIDGID(uid_t uid, gid_t gid, gid_t *groups ATTRIBUTE_UNUSED, }
# if HAVE_SETGROUPS - if (ngroups && setgroups(ngroups, groups) < 0) { + if (setgroups(ngroups, groups) < 0) {
After running unit tests I see this causes a failure in virCommand. We were using 'ngroups != NULL' as a crude check to skip setgroups() when unprivileged. The better way to check this is by doing 'gid != (gid_t_-1' as we use on the line above which calls setgid(). So I'll push this instead: - if (ngroups && setgroups(ngroups, groups) < 0) { + if (gid != (gid_t)-1 && setgroups(ngroups, groups) < 0) { Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (4)
-
Daniel P. Berrange -
Martin Kletzander -
Richard Weinberger -
Richard Weinberger