Created attachment 86021 [details] [review] avoid SCM_CREDS errors for AF_INET Hi, When SCM_CREDS is defined, as is for GNU/Hurd and GNU/kFreeBSD sending a credentials byte, together with credentials data with sendmsg() is not allowed if the domain is not AF_LOCAL. This causes problems to send that byte for domain != AF_LOCAL for cases when authentication is done with other means. On Linux and kFreeBSD this creates no problems, SCM_CREDS are not fdefined on Linux and LOCAL_CREDS are used on kFreeBSD. The first part of the patch use send() when a zero credentials byte is sent and sendmsg() reports errno == EINVAL. The second part is similar: recvmsg() is not receiving the CMSGCREDS structure, since send() is used instead of sendmsg(). However, if the number of received bytes is one and that byte is zero, a credentials byte was sent. Maybe the patches could be improved, e.g. checking if domain != AF_LOCAL. The main problem is that the domain is not available in these functions write_credentials_byte() or _dbus_read_credentials_socket()
(In reply to comment #0) > On Linux and kFreeBSD this creates no problems, SCM_CREDS are not > fdefined on Linux and LOCAL_CREDS are used on kFreeBSD. As far as I can see, SCM_CREDS are what we use on (k)FreeBSD, although I haven't tested that recently; the HAVE_CMSGCRED code path explicitly turns off LOCAL_CREDS so we don't try to do two mechanisms at the same time, and Bug #60340 indicates that LOCAL_CREDS hasn't actually worked since 2009. On that bug, I've been trying to reduce the technical debt associated with credentials-passing: nobody seems to know which mechanisms are used on which OSs (except sometimes their own), and many code paths are rarely (never?) tested. So: which credentials-passing mechanisms does Hurd implement? Which existing OS does Hurd's credentials-passing support aim to mimic? How does it differ from how that OS works? For D-Bus, what we ideally want is a way to determine the credentials that the peer process had at connect() time, without the peer process having to do anything special to get that information to us - for instance, Linux/OpenBSD SO_PEERCRED, Solaris getpeerucred() or the cross-platform getpeereid(). We prefer SO_PEERCRED over getpeereid() if both are available, because it provides both uid and pid, whereas getpeereid() only provides the uid. Does Hurd have any such mechanism? If not, would it be possible to add one, ideally source-compatible with either Linux or OpenBSD? That would make the more complicated SCM_CREDS code path irrelevant.
(In reply to comment #0) > avoid SCM_CREDS errors for AF_INET Why are you using D-Bus over TCP when you have Unix sockets available? I suspect this might mean D-Bus over TCP has never worked on (k)FreeBSD either... > The second part is similar: recvmsg() is not receiving the > CMSGCREDS structure, since send() is used instead of sendmsg(). > However, if the number of received bytes is one and that byte > is zero, a credentials byte was sent. I feel as though there ought to be some way to iterate over 0 or more items of ancillary data, rather than assuming that there is exactly one? (Or is this a zero-or-one thing?) If the message contains ancillary data of type SCM_CREDS and length at least CMSG_LEN, we should use that to fill in pid_read and uid_read. If not, that shouldn't be an error; we should just leave uid_read = DBUS_UID_UNSET and pid_read = DBUS_PID_UNSET, and carry on. I think this might be simpler to implement if we deleted LOCAL_CREDS support first (see Bug #60340 for patch) - as far as I can tell, every OS that supports LOCAL_CREDS also supports getpeereid(), which is much, much simpler to use. Then SCM_CREDS would be the only awkward mechanism that requires help from the peer.
Comment on attachment 86021 [details] [review] avoid SCM_CREDS errors for AF_INET Review of attachment 86021 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-sysdeps-unix.c @@ +1629,5 @@ > > + /* sendmsg() only accepts domain == PF_UNIX*/ > +#if defined(HAVE_CMSGCRED) > + if (bytes_written < 0 && errno == EINVAL) > + bytes_written = send (server_fd, buf, 1, 0); This doesn't deal with EINTR from send(). Move it above the EINTR check, perhaps? @@ +1769,5 @@ > } > > #if defined(HAVE_CMSGCRED) || defined(LOCAL_CREDS) > + /* Special case for a zero credentials byte written */ > + if (bytes_read == 1 && buf == '\0') This is not correct. Having bypassed the cmsg_len/cmsg_type check, the HAVE_CMSGCRED block further down will dereference (struct cmsgcred *) CMSG_DATA (&cmsg.hdr), which, without that check, is not necessarily long enough for that to be valid. Similarly, the LOCAL_CREDS block will dereference cmsg.cred.sc_uid without knowing that cred is long enough. I think the right logic is probably to move the length/type check into the "#elif defined(HAVE_CMSGCRED)" further down, and if the message doesn't contain credentials, leave the uid and pid set to their "unset" values. That will have the same practical effect as running D-Bus on a version of Hurd that is too old to have credentials-passing: EXTERNAL authentication won't work, and it will automatically fall back to DBUS_COOKIE_SHA1 authentication.
Marking this as blocked by Bug #60340 - I don't want to modify the various credentials-passing implementations until we've documented which ones are used where, which ones have been tested, etc.
On Wed, 2013-09-18 at 08:20 +0000, bugzilla-daemon@freedesktop.org wrote: > Comment # 1 on bug 69492 from Simon McVittie > (In reply to comment #0) > > On Linux and kFreeBSD this creates no problems, SCM_CREDS are not > > defined on Linux and LOCAL_CREDS are used on kFreeBSD. Sorry, I should have checked if LOCAL_CREDS is used on kFreeBSD. Obviously not. > As far as I can see, SCM_CREDS are what we use on (k)FreeBSD, although I > haven't tested that recently; the HAVE_CMSGCRED code path explicitly turns off > LOCAL_CREDS so we don't try to do two mechanisms at the same time, and Bug > #60340 indicates that LOCAL_CREDS hasn't actually worked since 2009. OK! > On that bug, I've been trying to reduce the technical debt associated with > credentials-passing: nobody seems to know which mechanisms are used on which > OSs (except sometimes their own), and many code paths are rarely (never?) > tested. > > So: which credentials-passing mechanisms does Hurd implement? Which existing OS > does Hurd's credentials-passing support aim to mimic? How does it differ from > how that OS works? It's still up to implementation, I'm currently working on that. I believe we work along the SCM_CREDS path similar to BSD. See http://lists.gnu.org/archive/html/bug-hurd/2013-08/msg00076.html for test code on SCM_CREDS and results for Linux, kFreeBSD and Hurd. From the header file /usr/include/i386-gnu/bits/socket.h: /* Structure delivered by SCM_CREDS. This describes the identity of the sender of the data simultaneously received on the socket. By BSD convention, this is included only when a sender on a AF_LOCAL socket sends cmsg data of this type and size; the sender's structure is ignored, and the system fills in the various IDs of the sender process. */ In the current implementation the senders structure is checked for validity, not ignored (as for kFreeBSD), but that can easily be changed. Any feedback welomed. > For D-Bus, what we ideally want is a way to determine the credentials that the > peer process had at connect() time, without the peer process having to do > anything special to get that information to us - for instance, Linux/OpenBSD > SO_PEERCRED, Solaris getpeerucred() or the cross-platform getpeereid(). We > prefer SO_PEERCRED over getpeereid() if both are available, because it provides > both uid and pid, whereas getpeereid() only provides the uid. > > Does Hurd have any such mechanism? If not, would it be possible to add one, > ideally source-compatible with either Linux or OpenBSD? That would make the > more complicated SCM_CREDS code path irrelevant. Sorry, Hurd does not have support for SO_PEERCRED (another non-posix feature). Nothing is really wrong with SCM_CREDS, it is the zero credentials bit that complicates stuff (again non-posix I think) Svante
On Wed, 2013-09-18 at 08:39 +0000, bugzilla-daemon@freedesktop.org wrote: > Comment # 2 on bug 69492 from Simon McVittie > (In reply to comment #0) > > avoid SCM_CREDS errors for AF_INET > > Why are you using D-Bus over TCP when you have Unix sockets available? > > I suspect this might mean D-Bus over TCP has never worked on (k)FreeBSD > either... I don't try to do that, it is one of the test cases, test-corrupt, for dbus that calls sendmsg for AF_INET, which fails on all arches), but the credentials byte needs to be written, to be received by recvmsg(). So that dbus eventually finds the authentication via DBUS_COOKIE_SHA1 > > The second part is similar: recvmsg() is not receiving the > > CMSGCREDS structure, since send() is used instead of sendmsg(). > > However, if the number of received bytes is one and that byte > > is zero, a credentials byte was sent. > > I feel as though there ought to be some way to iterate over 0 or more items of > ancillary data, rather than assuming that there is exactly one? (Or is this a > zero-or-one thing?) > > If the message contains ancillary data of type SCM_CREDS and length at least > CMSG_LEN, we should use that to fill in pid_read and uid_read. > > If not, that shouldn't be an error; we should just leave uid_read = > DBUS_UID_UNSET and pid_read = DBUS_PID_UNSET, and carry on. > > I think this might be simpler to implement if we deleted LOCAL_CREDS support > first (see Bug #60340 for patch) - as far as I can tell, every OS that supports > LOCAL_CREDS also supports getpeereid(), which is much, much simpler to use. > Then SCM_CREDS would be the only awkward mechanism that requires help from the > peer. I think both Hurd and kFreeBSD can use SCM_CREDS, no need for getpeereid()? And retire LOCAL_CREDS? BTW: with my SCM_CREDS patches for GNU/Hurd all 15+2 tests for build-debug, and 11+0 tests for build works.
On Wed, 2013-09-18 at 08:46 +0000, bugzilla-daemon@freedesktop.org wrote: > Comment # 3 on bug 69492 from Simon McVittie > Comment on attachment 86021 [details] [review] [review] > avoid SCM_CREDS errors for AF_INET > > Review of attachment 86021 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: dbus/dbus-sysdeps-unix.c > @@ +1629,5 @@ > > > > + /* sendmsg() only accepts domain == PF_UNIX*/ > > +#if defined(HAVE_CMSGCRED) > > + if (bytes_written < 0 && errno == EINVAL) > > + bytes_written = send (server_fd, buf, 1, 0); > > This doesn't deal with EINTR from send(). Move it above the EINTR check, > perhaps? Well, it cannot be in the again: loop, there either sendmsg() or send are called(), depending on if HAVE_SCMCREDS is defined or not. > @@ +1769,5 @@ > > } > > > > #if defined(HAVE_CMSGCRED) || defined(LOCAL_CREDS) > > + /* Special case for a zero credentials byte written */ > > + if (bytes_read == 1 && buf == '\0') > > This is not correct. Having bypassed the cmsg_len/cmsg_type check, the > HAVE_CMSGCRED block further down will dereference (struct cmsgcred *) CMSG_DATA > (&cmsg.hdr), which, without that check, is not necessarily long enough for that > to be valid. Yes, the special case bytes_read == 1 && buf='\0' has to be added to that code also. Should I do that? > Similarly, the LOCAL_CREDS block will dereference cmsg.cred.sc_uid without > knowing that cred is long enough. Maybe retire LOCAL_CREDS? > I think the right logic is probably to move the length/type check into the > "#elif defined(HAVE_CMSGCRED)" further down, and if the message doesn't contain > credentials, leave the uid and pid set to their "unset" values. That will have > the same practical effect as running D-Bus on a version of Hurd that is too old > to have credentials-passing: EXTERNAL authentication won't work, and it will > automatically fall back to DBUS_COOKIE_SHA1 authentication. Maybe that would be a better solution. Additionally, Hurd does not yet support credentials passing, i.e. SCM_CREDS, that's what I'm working on. SCM_RIGHTS is supported.
(In reply to comment #5) > It's still up to implementation, I'm currently working on that. I believe we > work along the SCM_CREDS path similar to BSD. "Similar to BSD" doesn't tell me much - if my research is correct, OpenBSD has SO_PEERCRED and possibly getpeereid(), NetBSD has LOCAL_PEEREID, LOCAL_CREDS and getpeereid(), and FreeBSD/Dragonfly have SCM_CREDS and possibly others. "Intended to be compatible with FreeBSD version n" would be useful information though :-) Linux also has SCM_CREDENTIALS, which appears to be analogous to SCM_CREDS - but we don't use it in D-Bus, because SO_PEERCRED is so much easier, and this stuff is already a maze of twisty getsockopts, all different. > > For D-Bus, what we ideally want is a way to determine the credentials that the > > peer process had at connect() time, without the peer process having to do > > anything special to get that information to us > > Sorry, Hurd does not have support for SO_PEERCRED (another non-posix > feature). Is *any* of this stuff POSIX? As far as I'm aware, it's all extensions beyond POSIX (which is partly why it's such a mess). Hurd doesn't currently have support for SCM_CREDS either, if you're still writing it; what I'm trying to suggest is that if you're adding any solution to the "who is at the other end of this socket?" problem, from a userland developer point of view, by far the easiest thing to deal with is something the same "shape" as SO_PEERCRED, DJB getpeereid() or Solaris getpeerucred(). > Nothing is really wrong with SCM_CREDS, it is the zero > credentials bit that complicates stuff The thing that I find annoying about SCM_CREDS is that it's a two-step process. As used in D-Bus (pseudocode): client: sendmsg("\0", SCM_CREDS { uid smcv, pid 123, gid users }) service: recvmsg(and give me the SCM_CREDS too, if there are any) If either the client or the service fails to do it correctly, it doesn't work. Meanwhile, SO_PEERCRED, DJB getpeereid() and Solaris getpeerucred() work like this: client: (nothing special, just send some data) service: hey, kernel, who is this client? oh, it's pid 123 owned by smcv:users Half as many things that can go wrong :-)
(In reply to comment #7) > Well, it cannot be in the again: loop, there either sendmsg() or send > are called(), depending on if HAVE_SCMCREDS is defined or not. Couldn't you replace the sendmsg() there with sendmsg-or-send? We do similar things elsewhere, for instance in _dbus_open_socket(): #ifdef SOCK_CLOEXEC dbus_bool_t cloexec_done; *fd_p = socket (domain, type | SOCK_CLOEXEC, protocol); cloexec_done = *fd_p >= 0; /* Check if kernel seems to be too old to know SOCK_CLOEXEC */ if (*fd_p < 0 && (errno == EINVAL || errno == EPROTOTYPE)) #endif { *fd_p = socket (domain, type, protocol); } (imagine that was inside an EINTR loop.) > > > #if defined(HAVE_CMSGCRED) || defined(LOCAL_CREDS) > > > + /* Special case for a zero credentials byte written */ > > > + if (bytes_read == 1 && buf == '\0') > > > > This is not correct. Having bypassed the cmsg_len/cmsg_type check, the > > HAVE_CMSGCRED block further down will dereference (struct cmsgcred *) CMSG_DATA > > (&cmsg.hdr), which, without that check, is not necessarily long enough for that > > to be valid. > > Yes, the special case bytes_read == 1 && buf='\0' has to be added to > that code also. Should I do that? I think you might be missing the point of that byte. The D-Bus stream is defined to start with a message containing one zero byte. If the OS is one that requires us to send at least one byte with our credentials, and the socket is one that can send credentials, we send the credentials alongside that byte. If the OS is one that doesn't require us to do anything special to send credentials, or if the socket is one that can't do credentials, we still send the byte (so that the stream-level view of the protocol remains the same), but it's basically ignored. So, receiving "\0" at the beginning of the D-Bus stream tells you nothing: either it's a message with credentials in (in which case it's the credentials that are the interesting part, and we remember them for use with EXTERNAL authentication, and throw away the byte), or it's a message without credentials in (in which case we throw it away, remember that we don't know the credentials, and don't allow EXTERNAL authentication to succeed). > Maybe retire LOCAL_CREDS? Yeah, if/when someone reviews my commits, I will. (In reply to comment #6) > I don't try to do that, it is one of the test cases, test-corrupt, for > dbus that calls sendmsg for AF_INET, which fails on all arches) Ah, I see. Nice to see someone other than me running the test cases! test-corrupt certainly works on x86-64 Linux, but perhaps that's only because x86-64 Linux doesn't have SCM_CREDS. The TCP test cases are meant to fail EXTERNAL authentication and transparently fall through to DBUS_COOKIE_SHA1, yes. > I think both Hurd and kFreeBSD can use SCM_CREDS, no need for > getpeereid()? And retire LOCAL_CREDS? There are platforms other than Debian. NetBSD doesn't have anything that D-Bus currently supports, other than LOCAL_CREDS (but that's broken) and getpeereid(). > BTW: with my SCM_CREDS patches for GNU/Hurd all 15+2 tests for > build-debug, and 11+0 tests for build works. Interesting... I wonder what credentials it thinks you have? Is this dbus git master, or the dbus-1.6 branch? I'd be somewhat surprised if the version of test-dbus-daemon in master passes.
(In reply to comment #2) > I feel as though there ought to be some way to iterate over 0 or more items > of ancillary data, rather than assuming that there is exactly one? (Or is > this a zero-or-one thing?) Indeed there is. From the Linux man page cmsg(3): for (cmsg = CMSG_FIRSTHDR(&msgh); cmsg != NULL; cmsg = CMSG_NXTHDR(&msgh,cmsg)) { if (cmsg->cmsg_level == IPPROTO_IP && cmsg->cmsg_type == IP_TTL) { ttlptr = (int *) CMSG_DATA(cmsg); received_ttl = *ttlptr; break; } } Assuming that works on FreeBSD (and Hurd), that's clearly what we should be doing: we should ignore non-SCM_CREDS cmsgs, and if there are no csmgs at all, that shouldn't be an error. I have some patches which might work; starting up a kFreeBSD virtual machine...
Created attachment 86076 [details] [review] If sendmsg() with SCM_CREDS fails with EINVAL, retry with send() Perhaps some OSs accept and ignore attempts to send a SCM_CREDS message on a non-Unix socket, but GNU/kFreeBSD doesn't (and presumably FreeBSD doesn't either). Based on a patch by Svante Signell. --- This is basically the first half of your patch.
Created attachment 86077 [details] [review] _dbus_read_credentials_socket: look at all cmsg headers, not just the first If there are no cmsg headers, don't fail: this fixes receiving credentials on TCP sockets under at least GNU/kFreeBSD, and probably FreeBSD too. If there's more than one cmsg header, ignore any that don't look like valid SCM_CREDS. --- This is how we should have been doing csmgcred all along, I suspect. It assumes at least Attachment #85786 [details] from Bug #60340, and replaces the second half of your patch. Does it work on Hurd? With this patch, on Debian GNU/kFreeBSD, test-refs fails ("creating thread '': Error creating thread: Resource temporarily unavailable"), and bus-test fails reporting a 2 block memory leak, but those aren't regressions. test-loopback, test-corrupt etc. previously failed, and now pass.
(In reply to comment #5) > In the current implementation the senders structure is checked for > validity, not ignored (as for kFreeBSD), but that can easily be changed. > Any feedback welomed. If you're implementing an API that originated in FreeBSD, I would recommend behaving exactly like FreeBSD, whatever that means. In particular, anything that was considered to be valid usage in FreeBSD should still be considered valid (otherwise, existing correct code will have to change, potentially with a #ifdef __GNU__ or whatever, and nobody wants that). D-Bus' write_credentials_byte() assumes that a zero-filled struct cmsgcred is OK to send, and will be filled in with the right values instead of being treated as an attempt to impersonate root.
On Wed, 2013-09-18 at 15:34 +0000, bugzilla-daemon@freedesktop.org wrote: > Comment # 8 on bug 69492 from Simon McVittie > (In reply to comment #5) > > It's still up to implementation, I'm currently working on that. I believe we > > work along the SCM_CREDS path similar to BSD. > > "Similar to BSD" doesn't tell me much - if my research is correct, OpenBSD has > SO_PEERCRED and possibly getpeereid(), NetBSD has LOCAL_PEEREID, LOCAL_CREDS > and getpeereid(), and FreeBSD/Dragonfly have SCM_CREDS and possibly others. > "Intended to be compatible with FreeBSD version n" would be useful information > though :-) The only version of BSD is I have tested is kFreeBSD 9.1-1-686, see the test program and the results in http://lists.gnu.org/archive/html/bug-hurd/2013-08/msg00076.html One annoying thing with the kFreeBSD implementation is that that implementation completely disregards the senders structure: "the sender's structure is ignored". This is not the behaviour in Linux, for SCM_CREDENTIALS. > Linux also has SCM_CREDENTIALS, which appears to be analogous to SCM_CREDS - > but we don't use it in D-Bus, because SO_PEERCRED is so much easier, and this > stuff is already a maze of twisty getsockopts, all different. OK, but SO_PEERCRED does not report euid, egid or groups (maybe not interesting/needed for dbus) > > Sorry, Hurd does not have support for SO_PEERCRED (another non-posix > > feature). > > Is *any* of this stuff POSIX? As far as I'm aware, it's all extensions beyond > POSIX (which is partly why it's such a mess). Agreed :( > Hurd doesn't currently have support for SCM_CREDS either, if you're still > writing it; what I'm trying to suggest is that if you're adding any solution to > the "who is at the other end of this socket?" problem, from a userland > developer point of view, by far the easiest thing to deal with is something the > same "shape" as SO_PEERCRED, DJB getpeereid() or Solaris getpeerucred(). > > > Nothing is really wrong with SCM_CREDS, it is the zero > > credentials bit that complicates stuff > > The thing that I find annoying about SCM_CREDS is that it's a two-step process. > As used in D-Bus (pseudocode): > > client: sendmsg("\0", SCM_CREDS { uid smcv, pid 123, gid users }) > service: recvmsg(and give me the SCM_CREDS too, if there are any) Is the zero byte really needed for *BSD? > If either the client or the service fails to do it correctly, it doesn't work. > Meanwhile, SO_PEERCRED, DJB getpeereid() and Solaris getpeerucred() work like > this: > > client: (nothing special, just send some data) > service: hey, kernel, who is this client? oh, it's pid 123 owned by smcv:users > > Half as many things that can go wrong :-) Yes, this seems to be much simpler. One problem is that since all of these features are non-POSIX, they are frowned upon, including SCM_CREDS. I assume SCM_CREDS is a BSD inheritance. Can't this be solved by POSIX-only compliant functions?
On Wed, 2013-09-18 at 16:52 +0000, bugzilla-daemon@freedesktop.org wrote: > Comment # 11 on bug 69492 from Simon McVittie > Created attachment 86076 [details] [review] [review] > _dbus_read_credentials_socket: look at all cmsg headers, not just the first > > Perhaps some OSs accept and ignore attempts to send a SCM_CREDS > message on a non-Unix socket, but GNU/kFreeBSD doesn't (and presumably > FreeBSD doesn't either). > > Based on a patch by Svante Signell. You can add Hurd to the comment in your patch: for instance, FreeBSD + * only allows that on AF_UNIX -> for instance, FreeBSD and Hurd + * only allows that on AF_UNIX This I know for sure, since this is the case we encounter when trying to send credentials over AF_INET etc.
On Wed, 2013-09-18 at 15:53 +0000, bugzilla-daemon@freedesktop.org wrote: > > > BTW: with my SCM_CREDS patches for GNU/Hurd all 15+2 tests for > > build-debug, and 11+0 tests for build works. > > Interesting... I wonder what credentials it thinks you have? > > Is this dbus git master, or the dbus-1.6 branch? I'd be somewhat surprised if > the version of test-dbus-daemon in master passes. Tests run on 1.7.4 build-debug: dbus-test excluded, due to out of memory problems (works with the installed patched version: 1.6.14) =================== All 14 tests passed =================== ======================================================================== 1 of 2 tests failed run-test.sh: test-autolaunch failed This test requires ssh -Y/-X (it will work, it worked for 1.6.14 etc)
On Wed, 2013-09-18 at 17:04 +0000, bugzilla-daemon@freedesktop.org wrote: > Comment # 13 on bug 69492 from Simon McVittie > (In reply to comment #5) > > In the current implementation the senders structure is checked for > > validity, not ignored (as for kFreeBSD), but that can easily be changed. > > Any feedback welomed. > > If you're implementing an API that originated in FreeBSD, I would recommend > behaving exactly like FreeBSD, whatever that means. In particular, anything > that was considered to be valid usage in FreeBSD should still be considered > valid (otherwise, existing correct code will have to change, potentially with a > #ifdef __GNU__ or whatever, and nobody wants that). > > D-Bus' write_credentials_byte() assumes that a zero-filled struct cmsgcred is > OK to send, and will be filled in with the right values instead of being > treated as an attempt to impersonate root. I think this is the case -z in the test code I referred to so it would be OK to send zero-filled data in the cmsgcred structure. Conditioned on that the headers are correct: cmsg.hdr.cmsg_len = CMSG_LEN (sizeof (struct cmsgcred)); cmsg.hdr.cmsg_level = SOL_SOCKET; cmsg.hdr.cmsg_type = SCM_CREDS; The case I referred to is when credentials data is sent and not checked for kFreeBSD while they are for the current implementation for Hurd and SCM_CREDENTIALS in Linux.
On Wed, 2013-09-18 at 16:58 +0000, bugzilla-daemon@freedesktop.org wrote: > This is how we should have been doing csmgcred all along, I suspect. It assumes > at least Attachment #85786 [details] from Bug #60340, and replaces the second half of > your patch. Does it work on Hurd? Nice resolving of this problem. I can check. Should I just download with git clone etc and build? Any pointer? > With this patch, on Debian GNU/kFreeBSD, test-refs fails ("creating thread '': > Error creating thread: Resource temporarily unavailable"), and bus-test fails > reporting a 2 block memory leak, but those aren't regressions. test-loopback, > test-corrupt etc. previously failed, and now pass. A comment regarding the test on Hurd: I had to set DBUS_FATAL_WARNINGS=0 DBUS_BLOCK_ON_ABORT=0 in the build{,-debug}/test/Makefile since SO_REUSEADDR in setsockopt is not supported, giving numerous warnings: Failed to set socket option"/tmp/dbus-PUlA7GBit1": Protocol not available Can this be silenced in any way? I'm impressed with your prompt replies to this problem, thanks!
(In reply to comment #14) > OK, but SO_PEERCRED does not report euid, egid or groups (maybe not > interesting/needed for dbus) No, we aren't particularly interested in euid != uid, or groups. > > client: sendmsg("\0", SCM_CREDS { uid smcv, pid 123, gid users }) > > service: recvmsg(and give me the SCM_CREDS too, if there are any) > > Is the zero byte really needed for *BSD? Perhaps sending a 0-byte in-band message would work on some OSs, but it's known (or at least rumoured) not to work on others (but nobody has documented which ones). We send it anyway, regardless of whether it's needed or even whether we're sending credentials at all, so that the "in-band" part of the D-Bus stream is consistent - if you look at the bytestream in tcpdump or something, it always starts with "\0" followed by one of the SASL commands documented in the D-Bus Specification. The credentials themselves are "out-of-band data" and don't appear in the bytestream. > Can't this be solved > by POSIX-only compliant functions? No, POSIX does not define a way to do this (perhaps precisely because it's different on every platform, and POSIX is a "document existing practice" standard, not a "forced standard"). (In reply to comment #15) > You can add Hurd to the comment in your patch: > for instance, FreeBSD > + * only allows that on AF_UNIX -> > for instance, FreeBSD and Hurd > + * only allows that on AF_UNIX > > This I know for sure, since this is the case we encounter when trying to > send credentials over AF_INET etc. I avoided saying anything one way or the other about Hurd because I had the impression that the implementation of SCM_CREDS on Hurd was unreleased (and possibly unfinished). When it's in a release and is API, I'd be happy to add it to the relevant comments. (In reply to comment #16) > run-test.sh: test-autolaunch failed > This test requires ssh -Y/-X (it will work, it worked for 1.6.14 etc) Yeah, that test is just generally not very good (I have bugs open). (In reply to comment #18) > Nice resolving of this problem. I can check. Should I just download with > git clone etc and build? Any pointer? Clone it, autogen.sh, make check. You might want to hack the autolaunch test so it just exits 0 - it's somewhat broken if you aren't running it under X11 or you don't have an installed D-Bus environment (see other bugs). > A comment regarding the test on Hurd: I had to set > DBUS_FATAL_WARNINGS=0 > DBUS_BLOCK_ON_ABORT=0 > in the build{,-debug}/test/Makefile > since SO_REUSEADDR in setsockopt is not supported You shouldn't have to set DBUS_BLOCK_ON_ABORT if you just turn off fatal warnings. However, I do consider warnings during the regression test to be a test failure (or at least, an indication that a platform can't be a "first-class citizen") - it would be better to avoid those warnings. > Failed to set socket option"/tmp/dbus-PUlA7GBit1": Protocol not > available Does Hurd not support SO_REUSEADDR at all, or does it just limit support to those sockets where it actually makes a difference (TCP/UDP) or something? We could make this only conditionally a warning if necessary.
On Thu, 2013-09-19 at 12:15 +0000, bugzilla-daemon@freedesktop.org wrote: > Comment # 19 on bug 69492 from Simon McVittie > (In reply to comment #15) > I avoided saying anything one way or the other about Hurd because I had the > impression that the implementation of SCM_CREDS on Hurd was unreleased (and > possibly unfinished). When it's in a release and is API, I'd be happy to add it > to the relevant comments. OK, I'll publish the patches for SCM_CREDS soon to bug-hurd/debian-hurd for review. Nice to have a much clearer implementation of this stuff in dbus :) > (In reply to comment #16) > > run-test.sh: test-autolaunch failed > > This test requires ssh -Y/-X (it will work, it worked for 1.6.14 etc) > > Yeah, that test is just generally not very good (I have bugs open). OK > (In reply to comment #18) > > Nice resolving of this problem. I can check. Should I just download with > > git clone etc and build? Any pointer? > > Clone it, autogen.sh, make check. Where is that repository: Using git://anongit.freedesktop.org/dbus/dbus only gives master. Where is the one with your patches? > You might want to hack the autolaunch test so it just exits 0 - it's somewhat > broken if you aren't running it under X11 or you don't have an installed D-Bus > environment (see other bugs). > > > A comment regarding the test on Hurd: I had to set > > DBUS_FATAL_WARNINGS=0 > > DBUS_BLOCK_ON_ABORT=0 > > in the build{,-debug}/test/Makefile > > since SO_REUSEADDR in setsockopt is not supported > > You shouldn't have to set DBUS_BLOCK_ON_ABORT if you just turn off fatal > warnings. However, I do consider warnings during the regression test to be a > test failure (or at least, an indication that a platform can't be a > "first-class citizen") - it would be better to avoid those warnings. I'll try the tests with DBUS_BLOCK_ON_ABORT=1 > > Failed to set socket option"/tmp/dbus-PUlA7GBit1": Protocol not > > available > > Does Hurd not support SO_REUSEADDR at all, or does it just limit support to > those sockets where it actually makes a difference (TCP/UDP) or something? > > We could make this only conditionally a warning if necessary. They are supported for AF_INET, AF_INET6?, not for AX_UNIX. Yes, a conditional waning would be nice to get rid of the spurious warning printouts cluttering and hiding the real problems.
(In reply to comment #20) On Thu, 2013-09-19 at 17:04 +0200, Svante Signell wrote: > On Thu, 2013-09-19 at 12:15 +0000, bugzilla-daemon@freedesktop.org > wrote: .. > > (In reply to comment #18) > > > Nice resolving of this problem. I can check. Should I just download with > > > git clone etc and build? Any pointer? > > > > Clone it, autogen.sh, make check. > > Where is that repository: Using > git://anongit.freedesktop.org/dbus/dbus > only gives master. Where is the one with your patches? I found it: git clone git://people.freedesktop.org/~smcv/dbus -b no-local-creds-60340 > > You might want to hack the autolaunch test so it just exits 0 - it's somewhat > > broken if you aren't running it under X11 or you don't have an installed D-Bus > > environment (see other bugs). > > > > > A comment regarding the test on Hurd: I had to set > > > DBUS_FATAL_WARNINGS=0 > > > DBUS_BLOCK_ON_ABORT=0 > > > in the build{,-debug}/test/Makefile > > > since SO_REUSEADDR in setsockopt is not supported > > > > You shouldn't have to set DBUS_BLOCK_ON_ABORT if you just turn off fatal > > warnings. However, I do consider warnings during the regression test to be a > > test failure (or at least, an indication that a platform can't be a > > "first-class citizen") - it would be better to avoid those warnings. > > I'll try the tests with DBUS_BLOCK_ON_ABORT=1 Good news: =================== All 14 tests passed =================== Only bus-test print out messages infinitely. Had to disable it. Works for installed 1.6.14 as reported earlier (I have not installed this branch). and (ssh -Y) ================== All 2 tests passed ==================
(In reply to comment #21) > > Where is that repository: Using > > git://anongit.freedesktop.org/dbus/dbus > > only gives master. Where is the one with your patches? > > I found it: > git clone git://people.freedesktop.org/~smcv/dbus -b > no-local-creds-60340 Yes - sorry, I thought I'd mentioned that. I can't merge this until one of the other maintainers OKs it. > Only bus-test print out messages infinitely. Had to disable it. It produces a *lot* of output (it's retrying things over and over to test the OOM code paths!), but is meant to terminate eventually...
Ping!
(In reply to comment #22) > I can't merge this until one of the other maintainers OKs it. If you're happy with both of those patches (in terms of both testing and code review), please say so. If I've had positive review from a non-maintainer and the other maintainers don't respond, I'll eventually time out and apply them anyway. This is only going to go into master (development branch), although I might backport them to Debian's 1.6-based packages. (In reply to comment #20) > > Does Hurd not support SO_REUSEADDR at all, or does it just limit support to > > those sockets where it actually makes a difference (TCP/UDP) or something? > > > > We could make this only conditionally a warning if necessary. > > They are supported for AF_INET, AF_INET6?, not for AX_UNIX. Yes, a > conditional waning would be nice to get rid of the spurious warning > printouts cluttering and hiding the real problems. Patches welcome. My inclination would be to change the use in _dbus_listen_unix_socket() from _dbus_warn() to _dbus_verbose(); the use in _dbus_listen_tcp_socket() is fine as it is. Alternatively, if you can point to documentation or source code stating that SO_REUSEADDR doesn't do anything on AF_UNIX sockets in Linux and at least one of the *BSD family, I'd be OK with removing the SO_REUSEADDR from _dbus_listen_unix_socket() altogether, thus saving a syscall. Hurd seems somewhat reluctant to make pragmatic decisions for the benefit of existing software: for instance, if SO_REUSEADDR has no reason to have any effect on Unix sockets, I don't see that there would be any harm in accepting it and doing nothing. According to the Debian buildd logs, kFreeBSD doesn't trigger that warning either.
Created attachment 87546 [details] test_setsockopt.c On Thu, 2013-10-10 at 11:11 +0000, bugzilla-daemon@freedesktop.org wrote: > Comment # 24 on bug 69492 from Simon McVittie > (In reply to comment #22) > > I can't merge this until one of the other maintainers OKs it. > > If you're happy with both of those patches (in terms of both testing and code > review), please say so. If I've had positive review from a non-maintainer and > the other maintainers don't respond, I'll eventually time out and apply them > anyway. > > This is only going to go into master (development branch), although I might > backport them to Debian's 1.6-based packages. I'm about to publish the GNU/Hurd patches for SCM_CREDS support and hope for a positive reply (even if not POSIX). Together with your dbus (and glib2.0) patches a very lot of applications run correctly, especially X window managers and X applications (including emacs in X :) > (In reply to comment #20) > > > Does Hurd not support SO_REUSEADDR at all, or does it just limit support to > > > those sockets where it actually makes a difference (TCP/UDP) or something? > > > > > > We could make this only conditionally a warning if necessary. > > > > They are supported for AF_INET, AF_INET6?, not for AF_UNIX. Yes, a > > conditional waning would be nice to get rid of the spurious warning > > printouts cluttering and hiding the real problems. Looks like SO_REUSEADDR is supported for getsockopt/setsockopt for AF_INET* sockets, but not propagated to bind yet, see the attached test program, when run twice on Hurd (or within the TIMEOUT period) "Address already in use" happens. This will be fixed in due time. > Patches welcome. My inclination would be to change the use in > _dbus_listen_unix_socket() from _dbus_warn() to _dbus_verbose(); the use in > _dbus_listen_tcp_socket() is fine as it is. > > Alternatively, if you can point to documentation or source code stating that > SO_REUSEADDR doesn't do anything on AF_UNIX sockets in Linux and at least one > of the *BSD family, I'd be OK with removing the SO_REUSEADDR from > _dbus_listen_unix_socket() altogether, thus saving a syscall. I found a question about this problem at stackoverflow http://stackoverflow.com/questions/15716302/so-reuseaddr-and-af-unix where the conclusion is that this option should not be used on AF_UNIX sockets. I wrote a test program verifying this, see attachment. Tested on Linux and Hurd (not yet kFreeBSD) and bind bails out on "Address already in use" when run a second time even with the SO_REUSEADDR option. > Hurd seems somewhat reluctant to make pragmatic decisions for the benefit of > existing software: for instance, if SO_REUSEADDR has no reason to have any > effect on Unix sockets, I don't see that there would be any harm in accepting > it and doing nothing. According to the Debian buildd logs, kFreeBSD doesn't > trigger that warning either. See above, I think SO_REUSEADDR should never be used for AF_UNIX sockets.
Created attachment 87547 [details] test_SO_REUSEADDR_AF_UNIX.c
Created attachment 87548 [details] test_setsockopt.c On Sun, 2013-10-13 at 12:20 +0200, Svante Signell wrote: > On Thu, 2013-10-10 at 11:11 +0000, bugzilla-daemon@freedesktop.org > Looks like SO_REUSEADDR is supported for getsockopt/setsockopt for > AF_INET* sockets, but not propagated to bind yet, see the attached test > program, when run twice on Hurd (or within the TIMEOUT period) "Address > already in use" happens. This will be fixed in due time. Looks like the first attachment was not recognized (from gnulib tests). Resending.
On Sun, 2013-10-13 at 12:20 +0200, Svante Signell wrote: > On Thu, 2013-10-10 at 11:11 +0000, bugzilla-daemon@freedesktop.org > wrote: > Looks like SO_REUSEADDR is supported for getsockopt/setsockopt for > AF_INET* sockets, but not propagated to bind yet, see the attached test > program, when run twice on Hurd (or within the TIMEOUT period) "Address > already in use" happens. This will be fixed in due time. > > > Patches welcome. My inclination would be to change the use in > > _dbus_listen_unix_socket() from _dbus_warn() to _dbus_verbose(); the use in > > _dbus_listen_tcp_socket() is fine as it is. Yes, _dbus_listen_tcp_socket() is fine in my opinion. > > Alternatively, if you can point to documentation or source code stating that > > SO_REUSEADDR doesn't do anything on AF_UNIX sockets in Linux and at least one > > of the *BSD family, I'd be OK with removing the SO_REUSEADDR from > > _dbus_listen_unix_socket() altogether, thus saving a syscall. > > I found a question about this problem at stackoverflow > http://stackoverflow.com/questions/15716302/so-reuseaddr-and-af-unix > where the conclusion is that this option should not be used on AF_UNIX > sockets. I wrote a test program verifying this, see attachment. Tested > on Linux and Hurd (not yet kFreeBSD) and bind bails out on "Address > already in use" when run a second time even with the SO_REUSEADDR > option. The bind() error is now verified also on kFreeBSD, SO_REUSEADDR is accepted by setsockopt, but does not have any effect on the subsequent bind() call, which fails (as expected). Please remove this option from _dbus_listen_unix_socket(). I think the socket test code can be removed too, since if not using SO_REUSEADDR there is no need to check if the socket is present already (see also the previous attachment test_SO_REUSEADDR_AF_UNIX.c): int _dbus_listen_unix_socket() ... else { /* Discussed security implications of this with Nalin, * and we couldn't think of where it would kick our ass, but * it still seems a bit sucky. It also has non-security suckage; * really we'd prefer to exit if the socket is already in use. * But there doesn't seem to be a good way to do this. * * Just to be extra careful, I threw in the stat() - clearly * the stat() can't *fix* any security issue, but it at least * avoids inadvertent/accidental data loss. */ { struct stat sb; if (stat (path, &sb) == 0 && S_ISSOCK (sb.st_mode)) unlink (path); } ... strncpy (addr.sun_path, path, path_len); } reuseaddr = 1; if (setsockopt (listen_fd, SOL_SOCKET, SO_REUSEADDR, &reuseaddr, sizeof(reuseaddr))==-1) { _dbus_warn ("Failed to set socket option\"%s\": %s", path, _dbus_strerror (errno)); }
(In reply to comment #28) > I think the socket test code can be removed > too, since if not using SO_REUSEADDR there is no need to check if the > socket is present already I'm not going to remove the conditional unlink() unless there's a real-world reason why we should.
Created attachment 88478 [details] [review] _dbus_listen_unix_socket: don't try to set SO_REUSEADDR On Hurd, the setsockopt() fails. Svante Signell confirmed that on at least Linux and kFreeBSD, SO_REUSEADDR "succeeds" on Unix sockets, but doesn't have any practical effect; so rather than making the failure not issue a warning, we might as well not bother with the syscall at all.
(In reply to comment #22) > I can't merge this until one of the other maintainers OKs it. Hello maintainers, have any of you looked at my three patches on this bug? (Please ignore Svante's attachments, they're ad-hoc test code rather than patches for dbus.) Svante/Chengwei, if you have reviewed my three patches and you think they're a correct and appropriate fix for this, please say so. If you do, I'll eventually stop waiting for a maintainer to review them, and just apply them anyway. Test results ("with these patches, dbus works well on (Hurd|kFreeBSD|FreeBSD)" or whatever) also welcome.
Comment on attachment 86076 [details] [review] If sendmsg() with SCM_CREDS fails with EINVAL, retry with send() Review of attachment 86076 [details] [review]: ----------------------------------------------------------------- Looks good to me.
Comment on attachment 86077 [details] [review] _dbus_read_credentials_socket: look at all cmsg headers, not just the first Review of attachment 86077 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-sysdeps-unix.c @@ +1756,4 @@ > > + for (cmsgp = CMSG_FIRSTHDR (&msg); > + cmsgp != NULL; > + cmsgp = CMSG_NXTHDR (&msg, cmsgp)) indent the above two lines one more space? @@ +1764,5 @@ > + { > + cred = (struct cmsgcred *) CMSG_DATA (cmsgp); > + pid_read = cred->cmcred_pid; > + uid_read = cred->cmcred_euid; > + } This looks apparently much robust than only check the first cmsg, if there are any other types of cmsg. However, if here are more than one SCM_CREDS, the last one wins, this is a behaviour change compare with the previously we take the first one. Should we just "break;" once we find the first one?
Comment on attachment 88478 [details] [review] _dbus_listen_unix_socket: don't try to set SO_REUSEADDR Review of attachment 88478 [details] [review]: ----------------------------------------------------------------- Yes, indeed, after reading several long pages on stackoverflow, http://stackoverflow.com/questions/15716302/so-reuseaddr-and-af-unix, http://stackoverflow.com/questions/14388706/socket-options-so-reuseaddr-and-so-reuseport-how-do-they-differ-do-they-mean-t, it seems SO_REUSEADDR always fail for unix domain socket. So I'm totally agree to delete it there, meanwhile, in fact, SO_REUSEADDR doesn't used in the tcp:nonce-tcp transports where it may useful.
On Thu, 2013-11-28 at 09:07 +0000, bugzilla-daemon@freedesktop.org wrote: > Comment # 33 on bug 69492 from Chengwei Yang > Comment on attachment 86077 [details] [review] [review] > _dbus_read_credentials_socket: look at all cmsg headers, not just the first > > Review of attachment 86077 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: dbus/dbus-sysdeps-unix.c > @@ +1756,4 @@ > > > > + for (cmsgp = CMSG_FIRSTHDR (&msg); > > + cmsgp != NULL; > > + cmsgp = CMSG_NXTHDR (&msg, cmsgp)) > > indent the above two lines one more space? > > @@ +1764,5 @@ > > + { > > + cred = (struct cmsgcred *) CMSG_DATA (cmsgp); > > + pid_read = cred->cmcred_pid; > > + uid_read = cred->cmcred_euid; > > + } > > This looks apparently much robust than only check the first cmsg, if there are > any other types of cmsg. However, if here are more than one SCM_CREDS, the last > one wins, this is a behaviour change compare with the previously we take the > first one. Should we just "break;" once we find the first one? That would be the best solution: Is there any idea to request more than one credentials structure? The test code at http://lists.gnu.org/archive/html/bug-hurd/2013-11/msg00346.html can easily be modified to request more than one SCM_CREDS structure, but I did not find any reason to do that. Code tested on Linux/kFreeBSD/(patched)Hurd.
(In reply to comment #33) > indent the above two lines one more space? I usually indent continued lines by 2 indentation units (4 spaces here) rather than lining things up vertically, but yeah, wasting time lining things up is closer to the spirit of D-Bus' coding style. > This looks apparently much robust than only check the first cmsg, if there > are any other types of cmsg. However, if here are more than one SCM_CREDS, > the last one wins, this is a behaviour change compare with the previously we > take the first one. Should we just "break;" once we find the first one? Well spotted. If we saw more than one SCM_CREDS but they all had the same contents, it wouldn't matter which one we chose. If we saw more than one SCM_CREDS with different contents, what would that even mean? A SCM_CREDS structure is a trusted message from the kernel saying "the peer says it has these credentials, and I've confirmed that it has either those exact credentials, or enough privileges to be allowed to fake them". The only sensible interpretation I can think of for multiple distinct instances of SCM_CREDS would be "the peer has the union of all these credentials, simultaneously" (for instance, you could maybe use that to represent a uid and a list of group IDs, at least in theory). If that can somehow happen, then the pedantically-correct thing to do would be to give the process the union of all the credentials. However, I think picking one arbitrarily (either the first or the last) is a reasonable approximation - it can only make processes less capable, not more capable, so it isn't a vulnerability. If that ever happens, I think you're right that picking the first one preserves existing behaviour better than picking the last one, so I'll add the "break;" that you suggested. In practice, I doubt any kernels actually allow multiple distinct SCM_CREDS.
Fixed in git for 1.7.10.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.