dbus: fixes from a code review of the branch

A pass over the whole branch before merge, mostly in libink since
that is the new code and the part exposed to the wire.  Grouped here
rather than scattered so the review is easy to read in one place.

libink parser and dispatch:

 - Bound reader lengths so a 32-bit size_t can't wrap a wire length
   past the guard and read out of bounds.  Reachable pre-auth on any
   bus, so it matters on the 32-bit targets Finit runs on.
 - Drop a peer when a reply send fails instead of limping on with a
   half-written frame; a built-in whose send failed used to fall
   through and put a second frame on the wire.

initctl:

 - Copy a D-Bus error name out of the reply before closing the client;
   the reply points into memory the close frees.  Both error paths now
   share one helper so this can't creep back.

Authorization:

 - Take the caller's groups from the kernel (SO_PEERCRED plus
   SO_PEERGROUPS) rather than getpwuid()/getgrouplist(), which go
   through NSS and can block PID 1 on a slow LDAP or SSSD backend.
   The check is now a lookup against the group resolved once at init,
   with no NSS and no 256 KiB array on the stack.  A caller reaching
   us through a broker carries no group set, so system-bus privileged
   methods are root-only; the local bus keeps group support.  See
   libink/README.md for the note on lifting that.

Shutdown:

 - Call dbus_exit() from the shutdown path so the server, its peers,
   and the socket are let go cleanly.  The teardown existed but nobody
   called it.

Tests, CI, docs:

 - A fuzz target for the message parser, run as a quick sweep in the
   suite and properly under libFuzzer in CI, with the corpus carried
   between runs.  The -as-uid tests drop groups the way a login does
   so SO_PEERGROUPS sees the right set, and widen the test socket to
   reach the per-method check behind the 0660 gate.  Bring the GitHub
   actions up to versions that run on Node 24, and tidy a few small
   things a /simplify pass turned up.

Signed-off-by: Joachim Wiberg <troglobit@gmail.com>

libink/README.md

@@ -52,6 +52,21 @@ the uev event loop bridge.  `initctl` uses the client half of the same
 library, so one wire-format implementation serves both ends.  If libink
 is ever spun out, that file is the cut line.
 
+Future work
+-----------
+
+Privileged methods over the *system* bus are root-only.  Finit learns
+the caller's uid from the broker with `GetConnectionUnixUser`, but that
+reply carries no group list, so it cannot honour `--with-group`
+membership there the way it does on the local bus (where the kernel
+hands over the group set via `SO_PEERGROUPS`).  Closing the gap means
+asking the broker `GetConnectionCredentials` and reading its
+`UnixGroupIDs`, which is a variant holding an `au` array — the reader
+(`marshal.c`) only decodes single-character variant signatures today,
+so it needs extending first.  Finit's own direct users reach it over
+the local bus, so this has not been pressing.
+
+
 Testing
 -------
 

libink/builtin.c

@@ -457,6 +457,12 @@ static int handle_remove_match(link_connection_t *conn, const struct link_msg *m
 
 /* ---------- entry point ---------- */
 
+/* Returns 0 when a built-in handled the call and its reply went out,
+ * -1 when a built-in handled it but the send failed (the caller must
+ * drop the connection, its stream is now half-written), and 1 when
+ * the member is not a built-in and the caller should try the object
+ * tree.  The three are distinct so a failed send is never mistaken
+ * for "not mine". */
 int __handle_builtin(link_connection_t *conn, const struct link_msg *m)
 {
 	/* Hello and AddMatch/RemoveMatch are per-connection state, and on
@@ -501,5 +507,5 @@ int __handle_builtin(link_connection_t *conn, const struct link_msg *m)
 	if (member_is(m, "org.freedesktop.DBus.Properties", "GetAll"))
 		return handle_properties_get_all(conn, m);
 
-	return -1;	/* not a built-in */
+	return 1;	/* not a built-in, let the caller try the object tree */
 }

libink/client.c

@@ -133,7 +133,7 @@ static int read_one(link_client_t *c, struct link_msg *msg)
 
 	memset(msg, 0, sizeof(*msg));
 
-	if (read_full(c->fd, c->rxbuf, 16) < 0)
+	if (read_full(c->fd, c->rxbuf, LINK_HDR_FIXED_SIZE) < 0)
 		return -1;
 	if (c->rxbuf[0] != 'l')
 		return -1;
@@ -154,12 +154,13 @@ static int read_one(link_client_t *c, struct link_msg *msg)
 	if (fields_len > sizeof(c->rxbuf) || body_len > sizeof(c->rxbuf))
 		return -1;
 
-	body_off = (uint32_t)ALIGN_UP(16u + fields_len, 8u);
+	body_off = (uint32_t)ALIGN_UP(LINK_HDR_FIXED_SIZE + fields_len, 8u);
 	total    = body_off + body_len;
-	if (total > sizeof(c->rxbuf) || total < 16)
+	if (total > sizeof(c->rxbuf) || total < LINK_HDR_FIXED_SIZE)
 		return -1;
 
-	if (read_full(c->fd, c->rxbuf + 16, total - 16) < 0)
+	if (read_full(c->fd, c->rxbuf + LINK_HDR_FIXED_SIZE,
+		      total - LINK_HDR_FIXED_SIZE) < 0)
 		return -1;
 	c->rxlen = total;
 

libink/dispatch.c

@@ -526,12 +526,13 @@ int __dispatch_message(link_connection_t *conn, const struct link_msg *m, size_t
 
 /* Who may invoke a privileged method.  Without an authorizer, only
  * root, which is what libink can decide on its own. */
-static int caller_may(link_server_t *srv, uid_t uid)
+static int caller_may(link_server_t *srv, uid_t uid,
+		      const gid_t *groups, int ngroups)
 {
 	if (uid == LINK_UID_UNKNOWN)
 		return 0;
 	if (srv && srv->authorizer)
-		return srv->authorizer(uid, srv->authz_userdata);
+		return srv->authorizer(uid, groups, ngroups, srv->authz_userdata);
 
 	return uid == 0;
 }
@@ -610,8 +611,9 @@ static int dispatch_call(link_connection_t *conn, const struct link_msg *m,
 	 * state-changing built-in without first adding equivalent
 	 * authorisation inside __handle_builtin. */
 	rc = __handle_builtin(conn, m);
-	if (rc >= 0)
-		return rc;	/* 0 = handled OK, 1 = built-in but failed; <0 = not a built-in */
+	if (rc <= 0)
+		return rc;	/* 0 = handled & sent, -1 = send failed, drop */
+	/* rc == 1: not a built-in, fall through to the object tree. */
 
 	o = find_object(conn->server, m->path);
 	if (!o) {
@@ -667,7 +669,12 @@ static int dispatch_call(link_connection_t *conn, const struct link_msg *m,
 				call_uid = (uid_t)-1;
 		}
 
-		if (!caller_may(conn->server, call_uid)) {
+		/* Local peers carry a kernel-captured group set; a call
+		 * resolved over a broker carries only its uid, so the
+		 * authorizer sees no groups and falls back to root-only. */
+		if (!caller_may(conn->server, call_uid,
+				known_uid ? NULL : conn->peer_groups,
+				known_uid ? 0    : conn->peer_ngroups)) {
 			__dbg("denied %s, caller uid %d is not privileged",
 			      m->member, (int)call_uid);
 			return __send_error(conn, m,
@@ -683,22 +690,17 @@ static int dispatch_call(link_connection_t *conn, const struct link_msg *m,
 	__r_init(&call.read_cursor, m->body, m->body_avail);
 
 	rc = meth->handler(&call, e->userdata);
-	if (rc < 0 && !call.reply_consumed && !call.error_sent) {
-		/* Handler returned an error without sending one. */
-		__send_error(conn, m,
-				"org.freedesktop.DBus.Error.Failed",
-				"Handler failed");
-		return 0;
-	}
 
-	if (!call.reply_consumed && !call.error_sent) {
-		/* Handler returned 0 but never produced a reply; treat as
-		 * empty reply with out_sig "". */
-		__send_method_return(conn, m, NULL, NULL, 0);
-		return 0;
-	}
+	/* Every path returns the status of whatever it put on the wire,
+	 * so a failed send propagates out and the read loop drops the
+	 * peer rather than leaving a half-written frame on a live link.
+	 *
+	 * The handler sent its own reply or error: its return value is
+	 * that send's status. */
+	if (call.error_sent)
+		return rc;
 
-	if (call.reply_consumed && !call.error_sent) {
+	if (call.reply_consumed) {
 		blen = __w_finish(&call.reply_writer);
 		if (blen < 0)
 			return __send_error(conn, m,
@@ -708,5 +710,12 @@ static int dispatch_call(link_connection_t *conn, const struct link_msg *m,
 					       conn->txbuf, (size_t)blen);
 	}
 
-	return 0;
+	/* Nothing sent yet: a negative return becomes an error reply, a
+	 * zero return an empty method return. */
+	if (rc < 0)
+		return __send_error(conn, m,
+			"org.freedesktop.DBus.Error.Failed",
+			"Handler failed");
+
+	return __send_method_return(conn, m, NULL, NULL, 0);
 }

libink/internal.h

@@ -28,6 +28,12 @@ typedef enum {
 #define LINK_MATCH_RULE_MAX    256	/* per-peer match rule cap */
 #define LINK_MATCH_PEER_CAP    16	/* max active match rules per peer */
 #define LINK_PENDING_CAP        4	/* outbound calls awaiting a reply */
+/* Primary gid plus supplementary groups captured from SO_PEERGROUPS at
+ * accept, so the authorizer can answer group membership without an NSS
+ * lookup in PID 1.  A peer in more groups than this keeps only its
+ * primary gid (see link_server_accept), which fails closed.  32 covers
+ * a normal login; a directory environment can exceed it. */
+#define LINK_PEER_GROUPS_MAX   32
 /* Staging for an outgoing method call.  Generous on purpose: headers
  * for the calls libink makes run to ~150 B, and both the synchronous
  * and the connection-side path build into these, so one answer rather
@@ -142,6 +148,8 @@ struct link_match {
 struct link_connection {
 	int                 fd;
 	uid_t               peer_uid;
+	gid_t               peer_groups[LINK_PEER_GROUPS_MAX];
+	int                 peer_ngroups;	/* 0 until captured at accept */
 
 	char                guid[33];
 	char                unique_name[LINK_UNIQUE_NAME_LEN]; /* ":1.N" */

libink/link.h

@@ -135,10 +135,15 @@ typedef int (*link_uid_resolver_t)(link_connection_t *conn, const char *sender,
 void link_server_set_uid_resolver(link_server_t *server, link_uid_resolver_t cb,
 				  void *userdata);
 
-/* May `uid` invoke a LINK_METHOD_PRIVILEGED method?  Return non-zero
- * to allow.  Who counts as privileged is the embedder's policy, not
- * the library's; with no authorizer installed only uid 0 may. */
-typedef int (*link_authorizer_t)(uid_t uid, void *userdata);
+/* May this caller invoke a LINK_METHOD_PRIVILEGED method?  Return
+ * non-zero to allow.  `groups` is the caller's group set (primary gid
+ * first), captured from the kernel with SO_PEERGROUPS so the embedder
+ * can decide membership without an NSS lookup that would block PID 1;
+ * `ngroups` is 0 when the set is unknown, e.g. a call arriving over a
+ * broker, where only `uid` is available.  Who counts as privileged is
+ * the embedder's policy; with no authorizer installed only uid 0 may. */
+typedef int (*link_authorizer_t)(uid_t uid, const gid_t *groups, int ngroups,
+				 void *userdata);
 
 void link_server_set_authorizer(link_server_t *server, link_authorizer_t cb,
 				void *userdata);

libink/marshal.c

@@ -246,7 +246,10 @@ static int read_string_like(struct link_reader *r, const char **out)
 
 	if (__r_u32(r, &len) < 0)
 		return -1;
-	if (r->off + (size_t)len + 1 > r->cap) {
+	/* Subtractive so a 0xffffffff length cannot wrap the sum past
+	 * the guard on a 32-bit size_t: off <= cap always holds, and
+	 * the string needs len content bytes plus a nul. */
+	if (len >= r->cap - r->off) {
 		r->err = 1;
 		return -1;
 	}
@@ -364,16 +367,15 @@ int __r_align(struct link_reader *r, size_t n)
 int __r_array_begin(struct link_reader *r, size_t *out_end)
 {
 	uint32_t array_bytes;
-	size_t   end;
 
 	if (__r_u32(r, &array_bytes) < 0)
 		return -1;
-	end = r->off + (size_t)array_bytes;
-	if (end > r->cap) {
+	/* Subtractive, see read_string_like(): off <= cap always. */
+	if (array_bytes > r->cap - r->off) {
 		r->err = 1;
 		return -1;
 	}
-	*out_end = end;
+	*out_end = r->off + (size_t)array_bytes;
 	return 0;
 }
 

libink/proto.c

@@ -20,7 +20,7 @@
 
 #include "proto.h"
 
-#define HDR_FIXED_SIZE 16
+#define HDR_FIXED_SIZE LINK_HDR_FIXED_SIZE
 #define MAX_MSG_SIZE   (128 * 1024)	/* sanity limit for PID 1 */
 #define ALIGN_UP(x, n) (((x) + (n) - 1) & ~((size_t)((n) - 1)))
 

libink/proto.h

@@ -12,6 +12,11 @@
 
 #include "link.h"		/* LINK_MSG_* type codes */
 
+/* Fixed portion of the message header: endian, type, flags, version,
+ * body length, serial, and the field-array length -- 16 bytes before
+ * the variable header fields begin. */
+#define LINK_HDR_FIXED_SIZE  16
+
 /* Message flags. */
 #define LINK_FLAG_NO_REPLY_EXPECTED  0x01
 #define LINK_FLAG_NO_AUTO_START      0x02

libink/server.c

@@ -153,10 +153,37 @@ int link_server_accept(link_server_t *srv, link_connection_t **out)
 	conn->auth   = LINK_AUTH_NUL;
 	conn->server = srv;
 
-	if (getsockopt(cfd, SOL_SOCKET, SO_PEERCRED, &cred, &credlen) == 0)
+	if (getsockopt(cfd, SOL_SOCKET, SO_PEERCRED, &cred, &credlen) == 0) {
 		conn->peer_uid = cred.uid;
-	else
+
+		/* Capture the peer's group set from the kernel so the
+		 * authorizer never has to ask NSS (which can block PID 1).
+		 * Primary gid first, then the supplementary groups.
+		 *
+		 * SO_PEERGROUPS is all-or-nothing: too small a buffer gets
+		 * ERANGE and fills nothing, so a peer with more than fits
+		 * is captured as the primary gid alone.  A member of the
+		 * owning group through a supplementary slot beyond the cap
+		 * is then denied -- it fails closed, never open.  Needs
+		 * Linux 4.13; on older kernels the primary gid stands. */
+		conn->peer_groups[0] = cred.gid;
+		conn->peer_ngroups   = 1;
+#ifdef SO_PEERGROUPS
+		{
+			gid_t     sup[LINK_PEER_GROUPS_MAX - 1];
+			socklen_t len = sizeof(sup);
+
+			if (getsockopt(cfd, SOL_SOCKET, SO_PEERGROUPS, sup, &len) == 0) {
+				int n = (int)(len / sizeof(sup[0]));
+
+				for (int i = 0; i < n; i++)
+					conn->peer_groups[conn->peer_ngroups++] = sup[i];
+			}
+		}
+#endif
+	} else {
 		conn->peer_uid = (uid_t)-1;
+	}
 
 	__auth_generate_guid(conn->guid);