Recent linux kernels (linux-v4.13 and newer) allow for getting a list of auxiliary groups of a socket peer in a race-free way using SO_PEERGROUPS getsockopt option. For more details on SO_PEERGROUPS see the commit message, https://patchwork.ozlabs.org/patch/778713/ dbus-daemon currently uses getgrouplist() when it is available. We should use SO_PEERGROUPS if possible and fallback to getgrouplist() otherwise.
I'd be happy to review a patch, but I'm not intending to implement this myself any time soon (other priorities). High-level design: This would have to work a lot like add_linux_security_label_to_credentials(); then the groups would need to be propagated through the library in the same way that's currently done for the LSM label, so that bus_connection_get_unix_groups() could use that instead of looking up the uid in the DBusUserDatabase (a cache around the system passwd/group database, e.g. glibc nsswitch). DBusCredentials would need to distinguish between: - we do know the group vector (new Linux; group vector is returned directly, without consulting DBusUserDatabase) - we don't know the group vector (non-Linux, old Linux; current implementation of bus_connection_get_unix_groups() is used)
Does SO_PEERGROUPS guarantee to include the primary gid, or guarantee to include only the auxiliary/supplementary groups, or does it not make an API guarantee either way? (This is the same ambiguity as getgroups().) If it doesn't, then the primary gid would have to come from the struct ucred, but only get put in the DBusCredentials if SO_PEERGROUPS worked (to avoid breaking the DBusUserDatabase case).
SO_PEERGROUPS returns the auxiliary groups of the current process as tracked by the kernel. On linux, this does *not* necessarily include the user's primary group. In most cases, you want to add it in after fetching the array. Also note that the aux-groups cannot contain duplicates, and are unsorted.
(In reply to David Herrmann from comment #3) > SO_PEERGROUPS returns the auxiliary groups of the current process as tracked > by the kernel. On linux, this does *not* necessarily include the user's > primary group. In most cases, you want to add it in after fetching the array. Thanks, that's useful information. So the primary group ID from the struct ucred would probably need to be passed in to add_groups_to_credentials() as a parameter so that it could be included in the DBusCredentials.
Created attachment 136756 [details] [review] [1/11] DBusTransport, DBusConnection: Add internal getter for the credentials We have a lot of dbus_connection_get_foo() and _dbus_transport_get_foo() that are actually rather redundant.
Created attachment 136757 [details] [review] sysdeps: Get complete group vector from Linux SO_PEERGROUPS if possible
Created attachment 136758 [details] [review] [3/11] credentials: Add test coverage for groups
Created attachment 136759 [details] [review] [4/11] _dbus_credentials_to_string_append: Remove useless join = FALSE It can't actually matter in practice, because we never know the Unix uid and Unix groups but not pid, and we never have a Windows SID and also a Linux security label; but resetting join to FALSE can only ever result in us outputting something like "foo=123bar=456" instead of the intended form with a space in the middle.
Created attachment 136760 [details] [review] [5/11] credentials: Add test coverage for stringification
Created attachment 136762 [details] [review] [6/11] DBusCredentials: Add _dbus_clear_credentials() Not to be confused with _dbus_credentials_clear(), which does something different (this is a little unfortunate, but the fact that they take different types should clarify which is which). --- This is analogous to GLib g_clear_object(), and libdbus dbus_clear_message(), dbus_clear_connection() etc.: it unrefs (if not NULL) and nullifies a variable or struct member given as a pointer.
Created attachment 136763 [details] [review] [7/11] loopback test: Display credentials received --- On a development laptop with Linux 4.14, strace says this is working as intended: getsockopt(7, SOL_SOCKET, SO_PEERCRED, {pid=20344, uid=1000, gid=1000}, [12]) = 0 getsockopt(7, SOL_SOCKET, SO_PEERSEC, 0x55654423bc80, [1024]) = -1 ENOPROTOOPT (Protocol not available) getsockopt(7, SOL_SOCKET, SO_PEERGROUPS, "\4\0\0\0\24..."..., [1024->72]) = 0 ... MSG: Credentials: uid=1000 pid=20344 gid=4 gid=20 ... gid=1000
Created attachment 136764 [details] [review] [8/11] bus: Try to get groups directly from credentials, not userdb If we avoid consulting the userdb, then it's one less chance to deadlock.
Created attachment 136766 [details] [review] [9/11] bus: Get loginfo string bits from DBusCredentials This saves a couple of _dbus_strdup/dbus_free pairs. --- I'm not using _dbus_credentials_to_string_append() here because that's a larger change, and would result in the loginfo containing (potentially many) group IDs and being rather long; but I could do that as a later change. We'd still need to append comm (the command corresponding to the pid, as read by rummaging in /proc) if desired.
Created attachment 136767 [details] [review] [10/11] sysdeps: Document what _dbus_credentials_new_from_current_process has It only has the most important credentials, not the full set. --- I think it's correct that we don't get the LSM label or the complete group vector here. We can't easily get the LSM label, and we rarely actually care about either of those. The most important thing (which perhaps I should edit this commit to document) is that it does have everything that _dbus_credentials_same_user() needs.
Created attachment 136768 [details] [review] [11/11] bus driver: Use DBusCredentials to fill credentials structure --- I haven't tested it, but I think this gives us a small functional improvement: previously, GetConnectionCredentials("org.freedesktop.DBus") on Windows would not give us the dbus-daemon's Windows SID (I didn't implement that because I wasn't going to test it); but now we get that information "for free".
*** Bug 97821 has been marked as a duplicate of this bug. ***
Comment on attachment 136757 [details] [review] sysdeps: Get complete group vector from Linux SO_PEERGROUPS if possible Review of attachment 136757 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-credentials.c @@ +181,5 @@ > +static int > +cmp_gidp (const void *a_, const void *b_) > +{ > + const gid_t *a = a_; > + const gid_t *b = b_; This needs to be dbus_gid_t (they are not the same, dbus_gid_t is unsigned long and gid_t is whatever the kernel feels like using). This broke the build for Windows, and would result in incorrect comparisons on 64-bit big-endian Unix machines (where unsigned long is typically 64-bit and gid_t is typically 32-bit).
Created attachment 136770 [details] [review] [2/11 v2] sysdeps: Get complete group vector from Linux SO_PEERGROUPS if possible --- s/gid_t/dbus_gid_t/
Comment on attachment 136756 [details] [review] [1/11] DBusTransport, DBusConnection: Add internal getter for the credentials Review of attachment 136756 [details] [review]: ----------------------------------------------------------------- r+
Comment on attachment 136758 [details] [review] [3/11] credentials: Add test coverage for groups Review of attachment 136758 [details] [review]: ----------------------------------------------------------------- Some minor comments/nitpicks. ::: dbus/dbus-credentials-util.c @@ +46,5 @@ > { > DBusCredentials *credentials; > + static const struct > + { > + int n; unsigned? @@ +80,5 @@ > + if (group_vector) > + { > + dbus_gid_t *copy; > + > + copy = dbus_new0 (dbus_gid_t, group_vectors[group_vector - 1].n); Nitpick: Maybe add an `assert (group_vector > 0 && group_vector <= G_N_ELEMENTS (group_vectors))` at the top of the function? (Using a non-GLib equivalent for G_N_ELEMENTS() if you wish.)
Comment on attachment 136759 [details] [review] [4/11] _dbus_credentials_to_string_append: Remove useless join = FALSE Review of attachment 136759 [details] [review]: ----------------------------------------------------------------- r+ definitely
Comment on attachment 136760 [details] [review] [5/11] credentials: Add test coverage for stringification Review of attachment 136760 [details] [review]: ----------------------------------------------------------------- r+
Comment on attachment 136762 [details] [review] [6/11] DBusCredentials: Add _dbus_clear_credentials() Review of attachment 136762 [details] [review]: ----------------------------------------------------------------- r+, trivially
Comment on attachment 136763 [details] [review] [7/11] loopback test: Display credentials received Review of attachment 136763 [details] [review]: ----------------------------------------------------------------- r+ with one nitpick (feel free to ignore or push directly with a fix) ::: test/loopback.c @@ +261,4 @@ > test_main_context_iterate (f->ctx, TRUE); > } > > + creds = _dbus_connection_get_credentials (f->server_conn); Nitpick: This block is ripe for a comment at the top summarising what it does. For example: /* Log and superficially check the credentials at the server. */
Comment on attachment 136764 [details] [review] [8/11] bus: Try to get groups directly from credentials, not userdb Review of attachment 136764 [details] [review]: ----------------------------------------------------------------- A couple of comments. ::: bus/connection.c @@ +1041,5 @@ > > + credentials = _dbus_connection_get_credentials (connection); > + > + if (credentials != NULL && > + _dbus_credentials_get_unix_gids (credentials, &groups_borrowed, Is it definitely always safe to convert between (const dbus_gid_t **) and (const unsigned long **) as you’re doing with groups_borrowed here? @@ +1044,5 @@ > + if (credentials != NULL && > + _dbus_credentials_get_unix_gids (credentials, &groups_borrowed, > + n_groups)) > + { > + int i; unsigned? Though I guess it needs to match the type of `n_groups`, so ignore me.
Comment on attachment 136766 [details] [review] [9/11] bus: Get loginfo string bits from DBusCredentials Review of attachment 136766 [details] [review]: ----------------------------------------------------------------- r+
Comment on attachment 136767 [details] [review] [10/11] sysdeps: Document what _dbus_credentials_new_from_current_process has Review of attachment 136767 [details] [review]: ----------------------------------------------------------------- r+
Comment on attachment 136768 [details] [review] [11/11] bus driver: Use DBusCredentials to fill credentials structure Review of attachment 136768 [details] [review]: ----------------------------------------------------------------- r+
Comment on attachment 136770 [details] [review] [2/11 v2] sysdeps: Get complete group vector from Linux SO_PEERGROUPS if possible Review of attachment 136770 [details] [review]: ----------------------------------------------------------------- r- with a couple of questions. ::: dbus/dbus-credentials.c @@ +49,5 @@ > struct DBusCredentials { > int refcount; > dbus_uid_t unix_uid; > + dbus_gid_t *unix_gids; > + int n_unix_gids; unsigned? Although I imagine by this point in the patch series, you’ll be quite bored of repeating to me whatever justification you have for leaving it signed. ::: dbus/dbus-sysdeps-unix.c @@ +1824,5 @@ > + } > + > + /* Allocate an extra space for the primary group ID */ > + n_gids = ((size_t) len) / sizeof (gid_t); > + converted_gids = dbus_new (dbus_gid_t, n_gids + 1); Might it be possible for (n_gids + 1) to overflow if (len == MAXINT * sizeof (gid_t))? (I realise this is a corner case of corner cases.) @@ +1826,5 @@ > + /* Allocate an extra space for the primary group ID */ > + n_gids = ((size_t) len) / sizeof (gid_t); > + converted_gids = dbus_new (dbus_gid_t, n_gids + 1); > + > + if (converted_gids == 0) s/0/NULL/?
(In reply to Philip Withnall from comment #20) > ::: dbus/dbus-credentials-util.c > @@ +46,5 @@ > > { > > DBusCredentials *credentials; > > + static const struct > > + { > > + int n; > > unsigned? I suppose so; libdbus uses signed int for array lengths and indices, but this isn't libdbus API/ABI. > Nitpick: Maybe add an `assert (group_vector > 0 && group_vector <= > G_N_ELEMENTS (group_vectors))` at the top of the function? (Using a non-GLib > equivalent for G_N_ELEMENTS() if you wish.) Yeah, good idea. This is libdbus code, so no GLib, but we already have an "I can't believe it's not G_N_ELEMENTS" macro. (In reply to Philip Withnall from comment #25) > Is it definitely always safe to convert between (const dbus_gid_t **) and > (const unsigned long **) as you’re doing with groups_borrowed here? Yes, dbus_gid_t is hard-coded to be unsigned long (and I deliberately didn't add a cast, so the compiler would stop us if that changed). But it would be more obviously correct if I changed the type, and perhaps added a static assertion that assigning a dbus_gid_t to an unsigned long is lossless. > unsigned? Though I guess it needs to match the type of `n_groups`, so ignore > me. Yes, in this case it needs to match the type of n_groups, and libdbus conventionally uses signed int. I believe there might be some sort of, possibly spurious, rationale about making sure that decrementing past 0 doesn't result in libdbus thinking a massive amount of memory has been allocated and being happy to overflow a buffer to an arbitrary extent? If we do that then we're already living in undefined-behaviour-land, which is why I think that rationale might be spurious; but it's too late to make the ints in our public API be size_t, and the internal interfaces might as well be consistent with the public API. """ The GString feature of accepting negative numbers for "length of string" is also absent, because it could keep us from detecting bogus huge lengths. i.e. if we passed in some bogus huge length it would be taken to mean "current length of string" instead of "broken crack" """ — DBusString API documentation (In reply to Philip Withnall from comment #29) > > + int n_unix_gids; > > unsigned? Same as previous. > ::: dbus/dbus-sysdeps-unix.c > @@ +1824,5 @@ > > + } > > + > > + /* Allocate an extra space for the primary group ID */ > > + n_gids = ((size_t) len) / sizeof (gid_t); > > + converted_gids = dbus_new (dbus_gid_t, n_gids + 1); > > Might it be possible for (n_gids + 1) to overflow if (len == MAXINT * sizeof > (gid_t))? If the Linux kernel gives us multiple gigabytes of group IDs then I think we have more significant problems than this, not least of which is that Linux's socklen_t is int, so len can't get larger than MAXINT however hard we try. (There's a Linus rant somewhere about how POSIX briefly tried to change the type that is now socklen_t from the int traditionally used in BSD sockets to size_t, and how that would have been an unacceptable ABI break; so I think it's highly unlikely that Linux socklen_t on a new architecture will be larger than int.) I could add a static assertion, or make the function fail early if len gets comically huge, if you want. 65536 supplementary groups ought to be enough for anyone, and indeed that's as many as current Linux will ever let you have (even when 31-bit group IDs are enabled). > s/0/NULL/? Yes, good catch.
Created attachment 137397 [details] [review] [02/11 v3] sysdeps: Get complete group vector from Linux SO_PEERGROUPS if possible --- Use size_t for number of group IDs. Use NULL rather than 0 when checking whether malloc failed. Early-return if the kernel claims we have some ridiculous number of group IDs, to guarantee we don't overflow.
Created attachment 137398 [details] [review] [03/11 v2] credentials: Add test coverage for groups --- Assert that group_vector is in range. Use size_t for number of groups.
Created attachment 137399 [details] [review] [07/11 v2] loopback test: Display credentials received Reviewed-by: Philip Withnall <withnall@endlessm.com> [smcv: Add a comment as requested] Signed-off-by: Simon McVittie <smcv@collabora.com>
Created attachment 137400 [details] [review] [08/11 v2] bus: Try to get groups directly from credentials, not userdb --- Be more careful with types
I have also tested these on a live system and they seem to work.
Comment on attachment 137397 [details] [review] [02/11 v3] sysdeps: Get complete group vector from Linux SO_PEERGROUPS if possible Review of attachment 137397 [details] [review]: ----------------------------------------------------------------- r+
Comment on attachment 137398 [details] [review] [03/11 v2] credentials: Add test coverage for groups Review of attachment 137398 [details] [review]: ----------------------------------------------------------------- r+
Comment on attachment 137399 [details] [review] [07/11 v2] loopback test: Display credentials received Review of attachment 137399 [details] [review]: ----------------------------------------------------------------- r+
Comment on attachment 137400 [details] [review] [08/11 v2] bus: Try to get groups directly from credentials, not userdb Review of attachment 137400 [details] [review]: ----------------------------------------------------------------- r+
Fixed in git for 1.13.4.
Comment on attachment 137400 [details] [review] [08/11 v2] bus: Try to get groups directly from credentials, not userdb Review of attachment 137400 [details] [review]: ----------------------------------------------------------------- ::: bus/connection.c @@ +1056,5 @@ > + > + *n_groups = n; > + *groups = dbus_new (unsigned long, n); > + > + if (groups == NULL) coverity mentioned that this is an issue at CID 265358: *groups is referenced in line 1058 but afterwards groups is checked against null. If the intention is to check the return value of dbus_new it should be if (*groups == NULL) otherwise the check need to be placed before line 1058
Created attachment 138213 [details] [review] Fix null pointer dereferences in bus_connection_get_unix_groups Null-checking "groups" suggests that it may be null, but it has already been dereferenced on all paths leading to the check. Coverity ID 265358. Bug:
Comment on attachment 138213 [details] [review] Fix null pointer dereferences in bus_connection_get_unix_groups Review of attachment 138213 [details] [review]: ----------------------------------------------------------------- The change is obviously correct (thanks!), the commit message not so much. > Fix null pointer dereferences in bus_connection_get_unix_groups > > Null-checking "groups" suggests that it may be null, but it has already > been dereferenced on all paths leading to the check. The interface of this function is that it takes groups != NULL, and all callers do that (at least I'm fairly sure they do), so dereferencing groups is never a NULL dereference. The null pointer dereference here is that on an OOM condition, we might dereference (*groups) while *that* pointer is NULL. So I think a better commit message would be something like this: """ bus_connection_get_unix_groups: NULL-check *groups, not groups groups is never NULL here, but *groups can be NULL on OOM, and that's the check that was intended. """
(In reply to Simon McVittie from comment #43) > So I think a better commit message would be something like this: > > """ > bus_connection_get_unix_groups: NULL-check *groups, not groups or perhaps bus_connection_get_unix_groups: Correct check for OOM
Created attachment 138221 [details] [review] bus_connection_get_unix_groups: NULL-check *groups, not groups groups is never NULL here, but *groups can be NULL on OOM, and that's the check that was intended. Coverity ID 265358. - fixed commit message
Comment on attachment 138221 [details] [review] bus_connection_get_unix_groups: NULL-check *groups, not groups Review of attachment 138221 [details] [review]: ----------------------------------------------------------------- Ideal, please apply
- last patch applied to master
*** Bug 9328 has been marked as a duplicate of this bug. ***
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.