[libvirt] [PATCH] daemon: Fix core dumps if unix_sock_group is set
Setting unix_sock_group to something else than default "root" in /etc/libvirt/libvirtd.conf prevents system libvirtd from dumping core on crash. This is because we used setgid(unix_sock_group) before binding to /var/run/libvirt/libvirt-sock* and setgid() back to original group. However, if a process changes its effective or filesystem group ID, it will be forbidden from leaving core dumps unless fs.suid_dumpable sysctl is set to something else then 0 (and it is 0 by default). Changing socket's group ownership after bind works better. And we can do so without introducing a race condition since we loosen access rights by changing the group from root to something else. --- daemon/libvirtd.c | 17 ++++++++--------- 1 files changed, 8 insertions(+), 9 deletions(-) diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 2df9337..b1539b1 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -542,7 +542,6 @@ static int qemudListenUnix(struct qemud_server *server, char *path, int readonly, int auth) { struct qemud_socket *sock; mode_t oldmask; - gid_t oldgrp; char ebuf[1024]; if (VIR_ALLOC(sock) < 0) { @@ -579,21 +578,21 @@ static int qemudListenUnix(struct qemud_server *server, if (sock->addr.data.un.sun_path[0] == '@') sock->addr.data.un.sun_path[0] = '\0'; - oldgrp = getgid(); oldmask = umask(readonly ? ~unix_sock_ro_mask : ~unix_sock_rw_mask); - if (server->privileged && setgid(unix_sock_gid)) { - VIR_ERROR(_("Failed to set group ID to %d"), unix_sock_gid); - goto cleanup; - } - if (bind(sock->fd, &sock->addr.data.sa, sock->addr.len) < 0) { VIR_ERROR(_("Failed to bind socket to '%s': %s"), path, virStrerror(errno, ebuf, sizeof ebuf)); goto cleanup; } umask(oldmask); - if (server->privileged && setgid(oldgrp)) { - VIR_ERROR(_("Failed to restore group ID to %d"), oldgrp); + + /* chown() doesn't work for abstract sockets but we use them only + * if libvirtd runs unprivileged + */ + if (server->privileged && chown(path, -1, unix_sock_gid)) { + VIR_ERROR(_("Failed to change group ID of '%s' to %d: %s"), + path, unix_sock_gid, + virStrerror(errno, ebuf, sizeof ebuf)); goto cleanup; } -- 1.7.4.rc1
On Fri, Jan 07, 2011 at 12:50:25PM +0100, Jiri Denemark wrote:
Setting unix_sock_group to something else than default "root" in /etc/libvirt/libvirtd.conf prevents system libvirtd from dumping core on crash. This is because we used setgid(unix_sock_group) before binding to /var/run/libvirt/libvirt-sock* and setgid() back to original group. However, if a process changes its effective or filesystem group ID, it will be forbidden from leaving core dumps unless fs.suid_dumpable sysctl is set to something else then 0 (and it is 0 by default).
Changing socket's group ownership after bind works better. And we can do so without introducing a race condition since we loosen access rights by changing the group from root to something else.
If you use fchown(sock->fd) then you avoid any possible race issues.
--- daemon/libvirtd.c | 17 ++++++++--------- 1 files changed, 8 insertions(+), 9 deletions(-)
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 2df9337..b1539b1 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -542,7 +542,6 @@ static int qemudListenUnix(struct qemud_server *server, char *path, int readonly, int auth) { struct qemud_socket *sock; mode_t oldmask; - gid_t oldgrp; char ebuf[1024];
if (VIR_ALLOC(sock) < 0) { @@ -579,21 +578,21 @@ static int qemudListenUnix(struct qemud_server *server, if (sock->addr.data.un.sun_path[0] == '@') sock->addr.data.un.sun_path[0] = '\0';
- oldgrp = getgid(); oldmask = umask(readonly ? ~unix_sock_ro_mask : ~unix_sock_rw_mask); - if (server->privileged && setgid(unix_sock_gid)) { - VIR_ERROR(_("Failed to set group ID to %d"), unix_sock_gid); - goto cleanup; - } - if (bind(sock->fd, &sock->addr.data.sa, sock->addr.len) < 0) { VIR_ERROR(_("Failed to bind socket to '%s': %s"), path, virStrerror(errno, ebuf, sizeof ebuf)); goto cleanup; } umask(oldmask); - if (server->privileged && setgid(oldgrp)) { - VIR_ERROR(_("Failed to restore group ID to %d"), oldgrp); + + /* chown() doesn't work for abstract sockets but we use them only + * if libvirtd runs unprivileged + */
fchown() would work on abstract sockets too
+ if (server->privileged && chown(path, -1, unix_sock_gid)) { + VIR_ERROR(_("Failed to change group ID of '%s' to %d: %s"), + path, unix_sock_gid, + virStrerror(errno, ebuf, sizeof ebuf)); goto cleanup; }
Regards, Daniel
Setting unix_sock_group to something else than default "root" in /etc/libvirt/libvirtd.conf prevents system libvirtd from dumping core on crash. This is because we used setgid(unix_sock_group) before binding to /var/run/libvirt/libvirt-sock* and setgid() back to original group. However, if a process changes its effective or filesystem group ID, it will be forbidden from leaving core dumps unless fs.suid_dumpable sysctl is set to something else then 0 (and it is 0 by default).
Changing socket's group ownership after bind works better. And we can do so without introducing a race condition since we loosen access rights by changing the group from root to something else.
If you use fchown(sock->fd) then you avoid any possible race issues.
Except that it doesn't work. That was the first thing I tried but fchown() doesn't seem to work on unix sockets. The socket will still ended up with root:root ownership regardless on where I put fchown() -- either before bind() to avoid race issues or after it, which wouldn't be any better than chown(). Jirka
On 01/07/2011 05:30 AM, Jiri Denemark wrote:
Setting unix_sock_group to something else than default "root" in /etc/libvirt/libvirtd.conf prevents system libvirtd from dumping core on crash. This is because we used setgid(unix_sock_group) before binding to /var/run/libvirt/libvirt-sock* and setgid() back to original group. However, if a process changes its effective or filesystem group ID, it will be forbidden from leaving core dumps unless fs.suid_dumpable sysctl is set to something else then 0 (and it is 0 by default).
Changing socket's group ownership after bind works better. And we can do so without introducing a race condition since we loosen access rights by changing the group from root to something else.
If you use fchown(sock->fd) then you avoid any possible race issues.
Except that it doesn't work. That was the first thing I tried but fchown() doesn't seem to work on unix sockets. The socket will still ended up with root:root ownership regardless on where I put fchown() -- either before bind() to avoid race issues or after it, which wouldn't be any better than chown().
POSIX states that fchown() on pipes and sockets is allowed (but not required) to fail with EINVAL. I think it's a POSIX-compliance bug in the Linux kernel that it silently succeeds but ignores the change request, but to be truly portable, we have to use chown() rather than fchown() to avoid falling foul of the undefined behavior in the first place (whether or not we can convince kernel folks to either make fchown() fail with EINVAL or succeed at doing what we want). So, I don't see any other alternatives, and your patch looks like the way to go. ACK as-is. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
If you use fchown(sock->fd) then you avoid any possible race issues.
Except that it doesn't work. That was the first thing I tried but fchown() doesn't seem to work on unix sockets. The socket will still ended up with root:root ownership regardless on where I put fchown() -- either before bind() to avoid race issues or after it, which wouldn't be any better than chown().
POSIX states that fchown() on pipes and sockets is allowed (but not required) to fail with EINVAL. I think it's a POSIX-compliance bug in the Linux kernel that it silently succeeds but ignores the change request, but to be truly portable, we have to use chown() rather than fchown() to avoid falling foul of the undefined behavior in the first place (whether or not we can convince kernel folks to either make fchown() fail with EINVAL or succeed at doing what we want).
So, I don't see any other alternatives, and your patch looks like the way to go. ACK as-is.
Thanks, pushed. Jirka
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Jiri Denemark