Re: [libvirt] PATCH: better error checking for LOCAL_PEERCRED

2013-10-17 Thread Daniel P. Berrange 
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.cand...@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 ebl...@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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] PATCH: better error checking for LOCAL_PEERCRED

2013-10-17 Thread Eric Blake 
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.cand...@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 ebl...@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



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] PATCH: better error checking for LOCAL_PEERCRED

2013-10-17 Thread Brian Candler 

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.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] PATCH: better error checking for LOCAL_PEERCRED

2013-10-16 Thread Brian Candler 

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.


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] PATCH: better error checking for LOCAL_PEERCRED

2013-10-15 Thread Daniel P. Berrange 
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.orig2013-10-10 22:37:49.0 +0100
 +++ src/rpc/virnetsocket.c2013-10-12 22:51:57.0 +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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] PATCH: better error checking for LOCAL_PEERCRED

2013-10-14 Thread Brian Candler 
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.


Regards,

Brian Candler.

--- src/rpc/virnetsocket.c.orig 2013-10-10 22:37:49.0 +0100
+++ src/rpc/virnetsocket.c  2013-10-12 22:51:57.0 +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,

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] PATCH: better error checking for LOCAL_PEERCRED

2013-10-13 Thread Brian Candler 
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.orig2013-10-10 22:37:49.0 +0100
+++ src/rpc/virnetsocket.c2013-10-12 22:51:57.0 +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,

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list