2:04 a.m.
I was debugging libvirt with OSX today, and got as far as finding the
problem with LOCAL_PEERCRED, then googled this only to find that Ryota
Ozaki had fixed the problems a few days ago!
However you still may find the following patch useful. It tightens up
the checking in the LOCAL_PEERCRED block, and in particular fixes the
unlocking of the socket in the error return path for invalid groups, by
using the same logic from SO_PEERCRED - have a 'goto cleanup' in all
return paths.
(Detail: I found that when getsockopt was being called with SOL_SOCKET,
cr_ngroups was typically <0, probably because it was uninitialised.
However once the check for this was tightened, it hung because the
socket wasn't being unlocked on return. So better to (a) initialise it
to a negative value anyway, and (b) fix the return path)
However I have not checked that NGROUPS is defined on other BSD-like
systems. You could just have "if (cr.cr_ngroups <= 0)" instead.
Regards,
Brian Candler.
--- src/rpc/virnetsocket.c.orig 2013-10-10 22:37:49.000000000 +0100
+++ src/rpc/virnetsocket.c 2013-10-12 22:51:57.000000000 +0100
@@ -1157,8 +1157,10 @@
{
struct xucred cr;
socklen_t cr_len = sizeof(cr);
+ int ret = -1;
virObjectLock(sock);
+ cr.cr_ngroups = -1;
# if defined(__APPLE__)
if (getsockopt(sock->fd, SOL_LOCAL, LOCAL_PEERCRED, &cr, &cr_len)
< 0) {
# else
@@ -1166,20 +1168,19 @@
# endif
virReportSystemError(errno, "%s",
_("Failed to get client socket identity"));
- virObjectUnlock(sock);
- return -1;
+ goto cleanup;
}
if (cr.cr_version != XUCRED_VERSION) {
virReportError(VIR_ERR_SYSTEM_ERROR, "%s",
_("Failed to get valid client socket identity"));
- return -1;
+ goto cleanup;
}
- if (cr.cr_ngroups == 0) {
+ if (cr.cr_ngroups <= 0 || cr.cr_ngroups > NGROUPS) {
virReportError(VIR_ERR_SYSTEM_ERROR, "%s",
_("Failed to get valid client socket identity
groups"));
- return -1;
+ goto cleanup;
}
/* PID and process creation time are not supported on BSDs */
@@ -1188,8 +1189,11 @@
*uid = cr.cr_uid;
*gid = cr.cr_gid;
+ ret = 0;
+
+cleanup:
virObjectUnlock(sock);
- return 0;
+ return ret;
}
#else
int virNetSocketGetUNIXIdentity(virNetSocketPtr sock ATTRIBUTE_UNUSED,
6:16 a.m.
On Sun, Oct 13, 2013 at 08:04:25AM +0100, Brian Candler wrote:
I was debugging libvirt with OSX today, and got as far as finding
the problem with LOCAL_PEERCRED, then googled this only to find that
Ryota Ozaki had fixed the problems a few days ago!
However you still may find the following patch useful. It tightens
up the checking in the LOCAL_PEERCRED block, and in particular fixes
the unlocking of the socket in the error return path for invalid
groups, by using the same logic from SO_PEERCRED - have a 'goto
cleanup' in all return paths.
(Detail: I found that when getsockopt was being called with
SOL_SOCKET, cr_ngroups was typically <0, probably because it was
uninitialised. However once the check for this was tightened, it
hung because the socket wasn't being unlocked on return. So better
to (a) initialise it to a negative value anyway, and (b) fix the
return path)
However I have not checked that NGROUPS is defined on other BSD-like
systems. You could just have "if (cr.cr_ngroups <= 0)" instead.
Regards,
Brian Candler.
--- src/rpc/virnetsocket.c.orig 2013-10-10 22:37:49.000000000 +0100
+++ src/rpc/virnetsocket.c 2013-10-12 22:51:57.000000000 +0100
@@ -1157,8 +1157,10 @@
{
struct xucred cr;
socklen_t cr_len = sizeof(cr);
+ int ret = -1;
virObjectLock(sock);
+ cr.cr_ngroups = -1;
# if defined(__APPLE__)
if (getsockopt(sock->fd, SOL_LOCAL, LOCAL_PEERCRED, &cr,
&cr_len) < 0) {
# else
@@ -1166,20 +1168,19 @@
# endif
virReportSystemError(errno, "%s",
_("Failed to get client socket identity"));
- virObjectUnlock(sock);
- return -1;
+ goto cleanup;
}
if (cr.cr_version != XUCRED_VERSION) {
virReportError(VIR_ERR_SYSTEM_ERROR, "%s",
_("Failed to get valid client socket identity"));
- return -1;
+ goto cleanup;
}
- if (cr.cr_ngroups == 0) {
+ if (cr.cr_ngroups <= 0 || cr.cr_ngroups > NGROUPS) {
virReportError(VIR_ERR_SYSTEM_ERROR, "%s",
_("Failed to get valid client socket
identity groups"));
- return -1;
+ goto cleanup;
}
/* PID and process creation time are not supported on BSDs */
@@ -1188,8 +1189,11 @@
*uid = cr.cr_uid;
*gid = cr.cr_gid;
+ ret = 0;
+
+cleanup:
virObjectUnlock(sock);
- return 0;
+ return ret;
}
#else
int virNetSocketGetUNIXIdentity(virNetSocketPtr sock ATTRIBUTE_UNUSED,
Unfortunately your patch does not apply since your mail client has
messed up line wrapping. Also there have been conflicting changes
to the code since your patch. I would fix it myself, but I don't
have ability to compile test code on BSD platforms.
Can you update your patch & re-send.
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 :|
11:08 a.m.
On 15/10/2013 12:16, Daniel P. Berrange wrote:
Unfortunately your patch does not apply since your mail client has
messed up line wrapping. Also there have been conflicting changes
to the code since your patch. I would fix it myself, but I don't
have ability to compile test code on BSD platforms.
Can you update your patch & re-send.
Sorry about that. Updated patch uploaded
to
https://bugzilla.redhat.com/show_bug.cgi?id=1019929 where Thunderbird
can't mangle it.
7:29 a.m.
On 10/16/2013 10:08 AM, Brian Candler wrote:
On 15/10/2013 12:16, Daniel P. Berrange wrote:
> Unfortunately your patch does not apply since your mail client has
> messed up line wrapping. Also there have been conflicting changes
> to the code since your patch. I would fix it myself, but I don't
> have ability to compile test code on BSD platforms.
>
> Can you update your patch & re-send.
Sorry about that. Updated patch uploaded to
https://bugzilla.redhat.com/show_bug.cgi?id=1019929 where Thunderbird
can't mangle it.
On the other hand, making someone chase a URL is less convenient than
attaching the patch (ideally, sending inline the way 'git send-email'
does things is preferred, but since 'git am' can also handle
attachments, it's still easier on the maintainer to send through the
list). That said, I went ahead and did the work for you this time around.
Here's what I pushed:
From aa0f09929d02ccdbf3ca9502a1fd39d90db0c690 Mon Sep 17 00:00:00 2001
From: Brian Candler <b.candler(a)pobox.com>
Date: Thu, 17 Oct 2013 06:21:57 -0600
Subject: [PATCH] better error checking for LOCAL_PEERCRED
This patch improves the error checking in the LOCAL_PEERCRED version
of virNetSocketGetUNIXIdentity, used by FreeBSD and Mac OSX.
1. The error return paths now correctly unlock the socket. This is
implemented in exactly the same way as the SO_PEERCRED version,
using "goto cleanup"
2. cr.cr_ngroups is initialised to -1, and cr.cr_ngroups is checked
for negative and overlarge values.
This means that if the getsockopt() call returns success but doesn't
actually update the xucred structure, this is now caught. This
happened previously when getsockopt was called with SOL_SOCKET
instead of SOL_LOCAL, prior to commit 5a468b3, and resulted in
random uids being accepted.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/rpc/virnetsocket.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c
index e8cdfa6..2e50f8c 100644
--- a/src/rpc/virnetsocket.c
+++ b/src/rpc/virnetsocket.c
@@ -1174,25 +1174,27 @@ int virNetSocketGetUNIXIdentity(virNetSocketPtr
sock,
{
struct xucred cr;
socklen_t cr_len = sizeof(cr);
+ int ret = -1;
+
virObjectLock(sock);
+ cr.cr_ngroups = -1;
if (getsockopt(sock->fd, VIR_SOL_PEERCRED, LOCAL_PEERCRED, &cr,
&cr_len) < 0) {
virReportSystemError(errno, "%s",
_("Failed to get client socket identity"));
- virObjectUnlock(sock);
- return -1;
+ goto cleanup;
}
if (cr.cr_version != XUCRED_VERSION) {
virReportError(VIR_ERR_SYSTEM_ERROR, "%s",
_("Failed to get valid client socket identity"));
- return -1;
+ goto cleanup;
}
- if (cr.cr_ngroups == 0) {
+ if (cr.cr_ngroups <= 0 || cr.cr_ngroups > NGROUPS) {
virReportError(VIR_ERR_SYSTEM_ERROR, "%s",
_("Failed to get valid client socket identity
groups"));
- return -1;
+ goto cleanup;
}
/* PID and process creation time are not supported on BSDs */
@@ -1201,8 +1203,11 @@ int virNetSocketGetUNIXIdentity(virNetSocketPtr sock,
*uid = cr.cr_uid;
*gid = cr.cr_gid;
+ ret = 0;
+
+cleanup:
virObjectUnlock(sock);
- return 0;
+ return ret;
}
#else
int virNetSocketGetUNIXIdentity(virNetSocketPtr sock ATTRIBUTE_UNUSED,
--
1.8.3.1
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
7:30 a.m.
On Thu, Oct 17, 2013 at 06:29:14AM -0600, Eric Blake wrote:
On 10/16/2013 10:08 AM, Brian Candler wrote:
> On 15/10/2013 12:16, Daniel P. Berrange wrote:
>> Unfortunately your patch does not apply since your mail client has
>> messed up line wrapping. Also there have been conflicting changes
>> to the code since your patch. I would fix it myself, but I don't
>> have ability to compile test code on BSD platforms.
>>
>> Can you update your patch & re-send.
> Sorry about that. Updated patch uploaded to
> https://bugzilla.redhat.com/show_bug.cgi?id=1019929 where Thunderbird
> can't mangle it.
On the other hand, making someone chase a URL is less convenient than
attaching the patch (ideally, sending inline the way 'git send-email'
does things is preferred, but since 'git am' can also handle
attachments, it's still easier on the maintainer to send through the
list). That said, I went ahead and did the work for you this time around.
Here's what I pushed:
From aa0f09929d02ccdbf3ca9502a1fd39d90db0c690 Mon Sep 17 00:00:00 2001
From: Brian Candler <b.candler(a)pobox.com>
Date: Thu, 17 Oct 2013 06:21:57 -0600
Subject: [PATCH] better error checking for LOCAL_PEERCRED
This patch improves the error checking in the LOCAL_PEERCRED version
of virNetSocketGetUNIXIdentity, used by FreeBSD and Mac OSX.
1. The error return paths now correctly unlock the socket. This is
implemented in exactly the same way as the SO_PEERCRED version,
using "goto cleanup"
2. cr.cr_ngroups is initialised to -1, and cr.cr_ngroups is checked
for negative and overlarge values.
This means that if the getsockopt() call returns success but doesn't
actually update the xucred structure, this is now caught. This
happened previously when getsockopt was called with SOL_SOCKET
instead of SOL_LOCAL, prior to commit 5a468b3, and resulted in
random uids being accepted.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/rpc/virnetsocket.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c
index e8cdfa6..2e50f8c 100644
--- a/src/rpc/virnetsocket.c
+++ b/src/rpc/virnetsocket.c
@@ -1174,25 +1174,27 @@ int virNetSocketGetUNIXIdentity(virNetSocketPtr
sock,
{
struct xucred cr;
socklen_t cr_len = sizeof(cr);
+ int ret = -1;
+
virObjectLock(sock);
+ cr.cr_ngroups = -1;
if (getsockopt(sock->fd, VIR_SOL_PEERCRED, LOCAL_PEERCRED, &cr,
&cr_len) < 0) {
Hehe, your email client is mangling line breaks too Eric :-P
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 :|
7:33 a.m.
On 17/10/2013 13:29, Eric Blake wrote:
On the other hand, making someone chase a URL is less convenient
than
attaching the patch (ideally, sending inline the way 'git send-email'
does things is preferred, but since 'git am' can also handle
attachments, it's still easier on the maintainer to send through the
list). That said, I went ahead and did the work for you this time around.
Thank
you Eric. Sorry for making you jump through hoops!
Regards,
Brian.
4084
days inactive
4088
days old
5 comments
3 participants
participants (3)
-
Brian Candler -
Daniel P. Berrange -
Eric Blake