5:50 a.m.
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
6:17 a.m.
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
6:30 a.m.
> 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
5:57 p.m.
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
4:43 a.m.
>> 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
5101
days inactive
5104
days old
4 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Jiri Denemark