diff --git a/doc/ChangeLog.md b/doc/ChangeLog.md index 9a3082cc..d543ae28 100644 --- a/doc/ChangeLog.md +++ b/doc/ChangeLog.md @@ -56,8 +56,13 @@ All relevant changes are documented in this file. `List`, `Dump` for [user-defined conditions](conditions.md), with a `ConditionChanged (ss)` signal. - Privileged methods reject non-root callers based on the kernel- - authenticated peer uid (`SO_PEERCRED`); read-only methods are open. + Privileged methods accept root and members of the `--with-group` + group; everyone else is refused. On the local bus the caller's uid + and groups come straight from the kernel (`SO_PEERCRED` + + `SO_PEERGROUPS`), so the check never blocks PID 1 on an NSS lookup. + Over the system bus the uid is resolved from the broker + (`GetConnectionUnixUser`) and privileged methods are root-only. + Read-only methods are open. See [D-Bus Integration](dbus.md) for the full surface, build flag, and `dbus-send`/`dbus-monitor` examples. diff --git a/doc/build.md b/doc/build.md index c018ff47..23c7c13a 100644 --- a/doc/build.md +++ b/doc/build.md @@ -44,8 +44,7 @@ Below are a few of the main switches to configure: * `--enable-static`: Build Finit statically. The plugins will be built-ins (.o files) and all external libraries, except the C library - will be linked statically. Privileged D-Bus methods then accept only - `root`, see [Authorization](dbus.md#authorization) + will be linked statically. * `--enable-kernel-cmdline`: Enable Finit pre-4.1 parsing of init args from `/proc/cmdline`, this is *not recommended* since Finit may be running as the diff --git a/doc/dbus.md b/doc/dbus.md index e57898a2..6c1e52d7 100644 --- a/doc/dbus.md +++ b/doc/dbus.md @@ -209,22 +209,22 @@ to `--with-group` at build time. That is the same set the socket mode already admits, so the two gates agree instead of the socket letting the group in and every method turning it away. -A build configured with `--enable-static` accepts only `root`. Group -membership comes from NSS, which the C library loads with `dlopen()`, and a -build meant to link statically cannot count on that, so the lookup is compiled -out rather than left to fail open. The socket mode is unchanged, so the group -can still connect, it just cannot invoke a privileged method. - -On the **local** bus the kernel decides this at `connect()`, supplementary -groups included, and `SO_PEERCRED` tells Finit exactly who is calling, so -privilege escalation through the bus is impossible. +On the **local** bus the kernel settles this at `connect()`: `SO_PEERCRED` +gives Finit the caller's uid and `SO_PEERGROUPS` its group set, both straight +from the kernel. Finit matches the group against `--with-group` itself, so +the check never touches NSS -- `getpwuid`/`getgrouplist` can block on a slow +LDAP or SSSD backend, and PID 1 must never block. Because the group set is +the caller's real credentials, privilege escalation through the bus is +impossible. On the **system** bus one connection carries every caller, so `SO_PEERCRED` -describes `dbus-daemon` rather than whoever asked. For a privileged method -Finit asks the bus driver `GetConnectionUnixUser` about the message sender and -holds the call until the answer arrives. Nothing blocks: the reply comes back -through the same event loop as everything else, and the held call is then -dispatched or refused on its merits. +describes `dbus-daemon` rather than whoever asked. Finit asks the bus driver +`GetConnectionUnixUser` about the message sender and holds the call until the +answer arrives -- nothing blocks, the reply comes back through the same event +loop as everything else. The broker does not report the caller's groups in +that reply, so **privileged methods over the system bus are root-only**; group +membership is honoured on the local bus only. Lifting that is future work, +see `libink/README.md`. Answers are cached per sender. A bus never reuses a unique name while it runs, so an answer holds for as long as that bus does; Finit empties the cache diff --git a/libink/README.md b/libink/README.md index fbbf64c1..91a0fea3 100644 --- a/libink/README.md +++ b/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 ------- diff --git a/libink/builtin.c b/libink/builtin.c index fa511c1a..000290c3 100644 --- a/libink/builtin.c +++ b/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 */ } diff --git a/libink/client.c b/libink/client.c index 6a0c60b4..c54dec53 100644 --- a/libink/client.c +++ b/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; diff --git a/libink/dispatch.c b/libink/dispatch.c index 5597407d..764a6b69 100644 --- a/libink/dispatch.c +++ b/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); } diff --git a/libink/internal.h b/libink/internal.h index e71fcf00..e94e4371 100644 --- a/libink/internal.h +++ b/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" */ diff --git a/libink/link.h b/libink/link.h index 40f6f4b9..59754869 100644 --- a/libink/link.h +++ b/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); diff --git a/libink/marshal.c b/libink/marshal.c index ecc41166..d04d49e2 100644 --- a/libink/marshal.c +++ b/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; } diff --git a/libink/proto.c b/libink/proto.c index 15068f14..09ccec90 100644 --- a/libink/proto.c +++ b/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))) diff --git a/libink/proto.h b/libink/proto.h index 0ea345b1..7acd477f 100644 --- a/libink/proto.h +++ b/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 diff --git a/libink/server.c b/libink/server.c index 9c32c5ce..ad44a278 100644 --- a/libink/server.c +++ b/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); diff --git a/src/dbus.c b/src/dbus.c index 8f18f1f0..a93f55cd 100644 --- a/src/dbus.c +++ b/src/dbus.c @@ -31,9 +31,6 @@ #ifdef HAVE_DBUS #include -#include -#include -#include #include #include #include @@ -138,9 +135,12 @@ static void expire_sweep(void *arg) (void)arg; expire_armed = 0; - /* _SAFE because expiring a call runs its callback, and a - * callback that ends up writing to a peer can drop it, which - * unlinks it from this very list. */ + /* Expiring a call runs its callback, which may drop the peer it + * belongs to; _SAFE keeps this loop walking when that peer is + * `p`. It does NOT cover a callback that drops a *different* + * peer, which would unlink our saved `tmp` -- no path does that + * today (the only callback here denies on the same connection), + * and if one ever does it must not free from under the sweep. */ TAILQ_FOREACH_SAFE(p, &peers, link, tmp) { if (!p->dead) live += link_connection_expire(p->conn, DBUS_CALL_TIMEOUT_MS); @@ -1292,49 +1292,33 @@ void dbus_notify_condition_change(const char *name, const char *state) * the same thing, rather than the socket admitting the wheel group and * every method turning it away. * - * On the local bus the kernel already made this decision at connect(), - * supplementary groups and all, so the lookup only confirms it. The - * system bus has no socket mode to lean on, which is why we ask here - * rather than trusting the connection. + * The group set is the caller's own, captured by libink from the kernel + * (SO_PEERCRED + SO_PEERGROUPS) at connect, so this never asks NSS -- + * getpwuid/getgrouplist can block on a slow LDAP/SSSD backend, and PID 1 + * must never block. The owning group's gid is resolved once, at init. * - * Deliberately uncached: /etc/group changes while Finit runs, and a - * privileged call is an operator action, not a hot path. + * A caller reaching us through a broker carries no group set (libink + * passes ngroups 0), so system-bus privileged methods are root-only. + * The wheel group acts over the local bus, which is where initctl and + * operators connect. See libink/README.md for lifting that limit. */ -static int caller_is_privileged(uid_t uid, void *userdata) +static gid_t privileged_gid = (gid_t)-1; /* DEFGROUP, resolved at init */ + +static int caller_is_privileged(uid_t uid, const gid_t *groups, int ngroups, + void *userdata) { (void)userdata; if (uid == 0) return 1; - /* Group membership comes from NSS, which the C library loads - * with dlopen(), so a build meant to link statically cannot - * count on it. Compiled out rather than left to fail open: - * root only there, see doc/dbus.md. */ -#ifndef ENABLE_STATIC - { - gid_t groups[NGROUPS_MAX]; - int ngroups = NGROUPS_MAX; - struct passwd *pw; - int gid, i; + if (privileged_gid == (gid_t)-1) + return 0; /* no owning group to match against */ - gid = getgroup(DEFGROUP); - if (gid < 0) - return 0; - - pw = getpwuid(uid); - if (!pw) - return 0; - - if (getgrouplist(pw->pw_name, pw->pw_gid, groups, &ngroups) < 0) - return 0; - - for (i = 0; i < ngroups; i++) { - if (groups[i] == (gid_t)gid) - return 1; - } + for (int i = 0; i < ngroups; i++) { + if (groups[i] == privileged_gid) + return 1; } -#endif return 0; } @@ -1641,7 +1625,10 @@ int dbus_init(uev_ctx_t *ctx) return 1; } - if (chown(FINIT_BUS_SOCKET, geteuid(), getgroup(DEFGROUP))) + /* Resolve the owning group once: the authorizer matches callers + * against it on every privileged call and must not hit NSS then. */ + privileged_gid = getgroup(DEFGROUP); + if (chown(FINIT_BUS_SOCKET, geteuid(), privileged_gid)) err(1, "Failed setting group %s on %s", DEFGROUP, FINIT_BUS_SOCKET); link_server_set_authorizer(server, caller_is_privileged, NULL); diff --git a/src/initctl.c b/src/initctl.c index e5f0df70..646cd092 100644 --- a/src/initctl.c +++ b/src/initctl.c @@ -386,18 +386,25 @@ static int do_startstop(int cmd, char *arg) * `c` is closed before exit either way. Use exact-match on the * fully-qualified error name; a substring match would misfire on a * future name that contained one of these as a prefix. */ -static void map_dbus_err(link_client_t *c, const char *method, const char *ident) +/* The reply view points into the client's rxbuf, which link_client_close() + * frees, so anyone who wants the error name past the close has to copy it + * out first. Both D-Bus error paths in this file do; keep the one copy + * here so a third caller cannot reintroduce the use-after-free. */ +static void take_dbus_err(link_client_t *c, char *buf, size_t len) { const link_reply_t *r = link_client_reply(c); + + if (r && r->error_name) + strlcpy(buf, r->error_name, len); + else if (len) + buf[0] = '\0'; +} + +static void map_dbus_err(link_client_t *c, const char *method, const char *ident) +{ char err[128]; - /* The reply view points into c->rxbuf; copy the error name out - * before link_client_close() frees the client. Otherwise the - * strcmps below read freed memory. */ - if (r && r->error_name) - strlcpy(err, r->error_name, sizeof(err)); - else - err[0] = '\0'; + take_dbus_err(c, err, sizeof(err)); link_client_close(c); if (!strcmp(err, "org.finit.Error.NoSuchService")) @@ -514,16 +521,16 @@ static int cond_dbus_call(link_client_t **bus, condop_t op, return 1; } if (rc == LINK_CALL_ERROR) { - const link_reply_t *r = link_client_reply(*bus); - const char *err = (r && r->error_name) ? r->error_name : ""; + char err[128]; + take_dbus_err(*bus, err, sizeof(err)); link_client_close(*bus); *bus = NULL; if (!strcmp(err, "org.freedesktop.DBus.Error.AccessDenied")) ERRX(1, "permission denied: cond %s requires root", method); ERRX(73, "Failed %s condition <%s>: %s", op == COND_SET ? "asserting" : "deasserting", - arg, *err ? err : "D-Bus error"); + arg, err[0] ? err : "D-Bus error"); } /* LINK_CALL_FAIL */ link_client_close(*bus); diff --git a/src/sig.c b/src/sig.c index 6db745d3..bdfc65b4 100644 --- a/src/sig.c +++ b/src/sig.c @@ -338,6 +338,11 @@ void do_shutdown(shutop_t op) /* We can no longer act on conditions, activate script runner */ cond_exit(); +#ifdef HAVE_DBUS + /* Close the bus: drop peers, free the server, unlink the socket. */ + dbus_exit(); +#endif + /* Reap 'em */ while (waitpid(-1, NULL, WNOHANG) > 0) ; diff --git a/test/dbus-authz.sh b/test/dbus-authz.sh index 66badd54..2027cd23 100755 --- a/test/dbus-authz.sh +++ b/test/dbus-authz.sh @@ -29,6 +29,10 @@ case "$allowed" in *) fail "Unexpected reply for group member: $allowed" ;; esac +# uid 2 is outside the group, so 0660 would stop it at connect; widen +# the test socket so the refusal comes from the method, which is the +# layer under test here. +bus_open_to_all say "A caller outside the group may not" set +e denied=$(texec "$CLIENT" call-s-as-uid 2 "$BUS" /org/finit/manager \ diff --git a/test/dbus-cond.sh b/test/dbus-cond.sh index f0e55da1..4f74bdab 100755 --- a/test/dbus-cond.sh +++ b/test/dbus-cond.sh @@ -56,6 +56,9 @@ case "$(cat /tmp/dbus-condrej.out)" in *) fail "Unexpected reply: $(cat /tmp/dbus-condrej.out)" ;; esac +# See dbus-manager.sh: widen the test socket so a non-member reaches +# the method-level authz behind the 0660 gate. +bus_open_to_all say "Cond1.Set from non-root is rejected with AccessDenied" set +e texec "$CLIENT" call-s-as-uid 1 "$BUS" /org/finit/cond \ @@ -67,3 +70,18 @@ case "$(cat /tmp/dbus-condauthz.out)" in *AccessDenied*) assert "Cond1 authz fires" 0 -eq 0 ;; *) fail "Unexpected reply: $(cat /tmp/dbus-condauthz.out)" ;; esac + +# initctl's own error path: a non-usr name comes back InvalidArgs from +# the server, which drove cond_dbus_call to read the error reply after +# freeing the client (a use-after-free ASan catches on this build). +# Exercise it here so the fix stays fixed. +say "initctl cond set of a rejected name reports cleanly, no use-after-free" +set +e +out=$(texec sh -c 'initctl cond set aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa' 2>&1) +ic_rc=$? +set -e +assert "initctl reported the rejection (rc=$ic_rc)" "$ic_rc" -ne 0 +case "$out" in + *[Ff]ailed*|*InvalidArgs*|*restricted*) assert "error surfaced, not a crash" 0 -eq 0 ;; + *) fail "Unexpected initctl output: $out" ;; +esac diff --git a/test/dbus-manager.sh b/test/dbus-manager.sh index c3bdce33..80de5368 100755 --- a/test/dbus-manager.sh +++ b/test/dbus-manager.sh @@ -39,6 +39,10 @@ case "$(cat /tmp/dbus-stop.out)" in *) fail "Unexpected error reply: $(cat /tmp/dbus-stop.out)" ;; esac +# Non-root callers here are outside the --with-group group; widen the +# test socket so they reach the per-method authz rather than being +# stopped at connect by the 0660 mode. +bus_open_to_all say "Manager1.Restart from non-root is rejected with AccessDenied" set +e texec "$CLIENT" call-s-as-uid 1 "$BUS" /org/finit/manager \ diff --git a/test/dbus-service.sh b/test/dbus-service.sh index e812651b..39424e9f 100755 --- a/test/dbus-service.sh +++ b/test/dbus-service.sh @@ -57,6 +57,9 @@ texec "$CLIENT" call-void "$BUS" /org/finit/service/keventd \ || fail "Service1.Restart returned non-zero" assert "Per-service Restart ok" 0 -eq 0 +# See dbus-manager.sh: widen the test socket so a non-member reaches +# the method-level authz behind the 0660 gate. +bus_open_to_all say "Service1.Restart from non-root is rejected with AccessDenied" set +e texec "$CLIENT" call-void-as-uid 1 "$BUS" /org/finit/service/keventd \ diff --git a/test/lib/dbus-setup.sh b/test/lib/dbus-setup.sh index 5b969da7..5fe0730c 100644 --- a/test/lib/dbus-setup.sh +++ b/test/lib/dbus-setup.sh @@ -18,3 +18,15 @@ fi say "Wait for $BUS to appear" retry "texec test -S $BUS" + +# The installed bus stays 0660 (root + the --with-group group), so a +# caller outside that group is stopped by the socket before any method +# runs. A few tests need to reach the per-method authorization behind +# that gate -- to prove a non-member is refused there too, not only at +# connect -- so they widen the *test* socket to 0666 first. Real +# deployments keep 0660; this only touches the runtime socket in the +# test namespace. +bus_open_to_all() +{ + texec chmod 0666 "$BUS" +} diff --git a/test/src/.gitignore b/test/src/.gitignore index 14824ddc..3b46f5da 100644 --- a/test/src/.gitignore +++ b/test/src/.gitignore @@ -4,3 +4,4 @@ /.deps/ /serv /dbus-auth-client +/fuzz-msg-parse diff --git a/test/src/dbus-auth-client.c b/test/src/dbus-auth-client.c index 14796647..c5908c34 100644 --- a/test/src/dbus-auth-client.c +++ b/test/src/dbus-auth-client.c @@ -42,6 +42,8 @@ */ #include +#include +#include #include #include #include @@ -52,6 +54,17 @@ #include "link.h" +/* The -as-uid modes drop all privileges before exiting, and LeakSanitizer + * cannot run under that (it needs ptrace on itself). Turn leak detection + * off for this test client via ASan's startup hook; it is a short-lived + * tool, and finit -- the process whose leaks actually matter -- is checked + * on its own. Harmless when the build has no sanitizer. */ +const char *__asan_default_options(void); +const char *__asan_default_options(void) +{ + return "detect_leaks=0"; +} + /* ---------- manual SASL: only mode_auth uses this ---------- */ static const char hex[] = "0123456789abcdef"; @@ -153,9 +166,15 @@ static int report_rc(link_client_t *c, int rc) return 2; } -/* Drop effective uid to argv-supplied value (decimal). */ +/* Become the argv-supplied uid (decimal), the way a login does: reset + * the group set from the target user's /etc/group entry before setuid, + * so the connection carries that user's real credentials. A bare + * setuid() would keep root's groups, and SO_PEERGROUPS would then hand + * the server a caller who still holds root's privileges -- masking the + * very authorization the -as-uid tests check. */ static int drop_uid(const char *uid_arg, const char *progname) { + struct passwd *pw; char *ep = NULL; long v; @@ -165,6 +184,22 @@ static int drop_uid(const char *uid_arg, const char *progname) fprintf(stderr, "%s: bad uid: %s\n", progname, uid_arg); return 2; } + + pw = getpwuid((uid_t)v); + if (pw) { + if (initgroups(pw->pw_name, pw->pw_gid) < 0) { + perror("initgroups"); + return 2; + } + if (setgid(pw->pw_gid) < 0) { + perror("setgid"); + return 2; + } + } else if (setgroups(0, NULL) < 0) { /* no entry: drop all groups */ + perror("setgroups"); + return 2; + } + if (setuid((uid_t)v) < 0) { perror("setuid"); return 2; diff --git a/test/src/fuzz-msg-parse.c b/test/src/fuzz-msg-parse.c index e9cb02c9..926fcee2 100644 --- a/test/src/fuzz-msg-parse.c +++ b/test/src/fuzz-msg-parse.c @@ -27,7 +27,7 @@ #include "internal.h" #define FRAME_MAX 512 -#define HDR_FIXED_SIZE 16 /* must agree with proto.c */ +#define HDR_FIXED_SIZE LINK_HDR_FIXED_SIZE /* from proto.h, no drift */ static void fail(const char *what, size_t size) {