Download raw body.
move "unix" pledge promise from gotd parent to auth process
On 2022/12/29 17:48:09 +0100, Stefan Sperling <stsp@stsp.name> wrote:
> The listen process now communicates the client UID/GID to the parent,
> and the auth process now verifies UID/GID on behalf of the parent.
>
> This allows us to remove the "unix" pledge promise from the parent,
> removing parent access to syscalls such as listen() and accept() in
> the AF_UNIX domain. The short-lived auth process seems like a better
> place for this than the parent which keeps running forever. Neither
> process would call listen() and accept() under normal circumstances.
> But pledge() would not prevent them from doing so.
>
> I don't want to trust the listener alone on UID/GID, so I need to
> perform a secondary check of the peer somewhere. Again, the auth
> process seems like a good place.
ok for me too
> diff 33223026dc355d2180b79d22cbd06044ee342411 468ed428fd1abc15f97fec583f29eb1455dce126
> commit - 33223026dc355d2180b79d22cbd06044ee342411
> commit + 468ed428fd1abc15f97fec583f29eb1455dce126
> blob - 57b17a1dd26385f232f8f989e4fa041babfeafe3
> blob + fd7f8c11aa319b18aa9f13289797a9809af40003
> --- gotd/auth.c
> +++ gotd/auth.c
> @@ -16,6 +16,7 @@
> */
>
> #include <sys/types.h>
> +#include <sys/socket.h>
> #include <sys/queue.h>
> #include <sys/uio.h>
>
> @@ -192,6 +193,8 @@ recv_authreq(struct imsg *imsg, struct gotd_imsgev *ie
> struct imsgbuf *ibuf = &iev->ibuf;
> struct gotd_imsg_auth iauth;
> size_t datalen;
> + uid_t euid;
> + gid_t egid;
>
> log_debug("authentication request received");
>
> @@ -201,6 +204,19 @@ recv_authreq(struct imsg *imsg, struct gotd_imsgev *ie
>
> memcpy(&iauth, imsg->data, datalen);
>
> + if (imsg->fd == -1)
> + return got_error(GOT_ERR_PRIVSEP_NO_FD);
> +
> + if (getpeereid(imsg->fd, &euid, &egid) == -1)
> + return got_error_from_errno("getpeerid");
> +
> + if (iauth.euid != euid)
> + return got_error(GOT_ERR_UID);
> + if (iauth.egid != egid)
> + return got_error(GOT_ERR_GID);
> +
> + log_debug("authenticating uid %d gid %d", euid, egid);
> +
> err = auth_check(&gotd_auth.repo->rules, gotd_auth.repo->name,
> iauth.euid, iauth.egid, iauth.required_auth);
> if (err) {
> blob - 4e693a39b4414ee735aa6d7f2649143c79ce3a88
> blob + 05f659daea632d0e305556351e4d6a5e97519fa0
> --- gotd/gotd.c
> +++ gotd/gotd.c
> @@ -1225,8 +1225,6 @@ recv_connect(uint32_t *client_id, struct imsg *imsg)
> size_t datalen;
> int s = -1;
> struct gotd_client *client = NULL;
> - uid_t euid;
> - gid_t egid;
>
> *client_id = 0;
>
> @@ -1246,11 +1244,6 @@ recv_connect(uint32_t *client_id, struct imsg *imsg)
> goto done;
> }
>
> - if (getpeereid(s, &euid, &egid) == -1) {
> - err = got_error_from_errno("getpeerid");
> - goto done;
> - }
> -
> client = calloc(1, sizeof(*client));
> if (client == NULL) {
> err = got_error_from_errno("calloc");
> @@ -1264,8 +1257,9 @@ recv_connect(uint32_t *client_id, struct imsg *imsg)
> client->fd = s;
> s = -1;
> client->delta_cache_fd = -1;
> - client->euid = euid;
> - client->egid = egid;
> + /* The auth process will verify UID/GID for us. */
> + client->euid = iconnect.euid;
> + client->egid = iconnect.egid;
> client->nref_updates = -1;
>
> imsg_init(&client->iev.ibuf, client->fd);
> @@ -2308,14 +2302,23 @@ start_auth_child(struct gotd_client *client, int requi
> struct gotd_repo *repo, char *argv0, const char *confpath,
> int daemonize, int verbosity)
> {
> + const struct got_error *err = NULL;
> struct gotd_child_proc *proc;
> struct gotd_imsg_auth iauth;
> + int fd;
>
> memset(&iauth, 0, sizeof(iauth));
>
> + fd = dup(client->fd);
> + if (fd == -1)
> + return got_error_from_errno("dup");
> +
> proc = calloc(1, sizeof(*proc));
> - if (proc == NULL)
> - return got_error_from_errno("calloc");
> + if (proc == NULL) {
> + err = got_error_from_errno("calloc");
> + close(fd);
> + return err;
> + }
>
> proc->type = PROC_AUTH;
> if (strlcpy(proc->repo_name, repo->name,
> @@ -2346,8 +2349,11 @@ start_auth_child(struct gotd_client *client, int requi
> iauth.required_auth = required_auth;
> iauth.client_id = client->id;
> if (gotd_imsg_compose_event(&proc->iev, GOTD_IMSG_AUTHENTICATE,
> - PROC_GOTD, -1, &iauth, sizeof(iauth)) == -1)
> + PROC_GOTD, fd, &iauth, sizeof(iauth)) == -1) {
> log_warn("imsg compose AUTHENTICATE");
> + close(fd);
> + /* Let the auth_timeout handler tidy up. */
> + }
>
> client->auth = proc;
> client->required_auth = required_auth;
> @@ -2562,7 +2568,7 @@ main(int argc, char **argv)
> case PROC_GOTD:
> #ifndef PROFILE
> if (pledge("stdio rpath wpath cpath proc exec "
> - "sendfd recvfd fattr flock unix unveil", NULL) == -1)
> + "sendfd recvfd fattr flock unveil", NULL) == -1)
> err(1, "pledge");
> #endif
> break;
> @@ -2576,7 +2582,7 @@ main(int argc, char **argv)
> break;
> case PROC_AUTH:
> #ifndef PROFILE
> - if (pledge("stdio getpw", NULL) == -1)
> + if (pledge("stdio getpw recvfd unix", NULL) == -1)
> err(1, "pledge");
> #endif
> auth_main(title, &gotd.repos, repo_path);
> blob - c1090adb4ecfd2e2c122cf9a6074590b01e560dd
> blob + fe0c0107c18a0d38f7565091ea1d3badb537969e
> --- gotd/gotd.h
> +++ gotd/gotd.h
> @@ -414,6 +414,8 @@ struct gotd_imsg_connect {
> /* Structure for GOTD_IMSG_CONNECT. */
> struct gotd_imsg_connect {
> uint32_t client_id;
> + uid_t euid;
> + gid_t egid;
> };
>
> /* Structure for GOTD_IMSG_AUTHENTICATE. */
> blob - 96427e857a4b693de2dec2665df312e682a43f73
> blob + e20369a78e149e0e24de1d635e24b7a6161e4d16
> --- gotd/listen.c
> +++ gotd/listen.c
> @@ -190,6 +190,8 @@ gotd_accept(int fd, short event, void *arg)
> int s = -1;
> struct gotd_listen_client *client = NULL;
> struct gotd_imsg_connect iconn;
> + uid_t euid;
> + gid_t egid;
>
> backoff.tv_sec = 1;
> backoff.tv_usec = 0;
> @@ -226,6 +228,11 @@ gotd_accept(int fd, short event, void *arg)
> if (listen_client_cnt >= GOTD_MAXCLIENTS)
> goto err;
>
> + if (getpeereid(s, &euid, &egid) == -1) {
> + log_warn("getpeerid");
> + goto err;
> + }
> +
> client = calloc(1, sizeof(*client));
> if (client == NULL) {
> log_warn("%s: calloc", __func__);
> @@ -235,10 +242,13 @@ gotd_accept(int fd, short event, void *arg)
> client->fd = s;
> s = -1;
> add_client(client);
> - log_debug("%s: new client connected on fd %d", __func__, client->fd);
> + log_debug("%s: new client connected on fd %d uid %d gid %d", __func__,
> + client->fd, euid, egid);
>
> memset(&iconn, 0, sizeof(iconn));
> iconn.client_id = client->id;
> + iconn.euid = euid;
> + iconn.egid = egid;
> s = dup(client->fd);
> if (s == -1) {
> log_warn("%s: dup", __func__);
> blob - bbfa823cda9c46a471aefb0a9263d11583de85b4
> blob + c951c76bc67d396a591c05f06bd362c98aa7f678
> --- include/got_error.h
> +++ include/got_error.h
> @@ -182,6 +182,8 @@
> #define GOT_ERR_REF_PROTECTED 164
> #define GOT_ERR_REF_BUSY 165
> #define GOT_ERR_COMMIT_BAD_AUTHOR 166
> +#define GOT_ERR_UID 167
> +#define GOT_ERR_GID 168
>
> struct got_error {
> int code;
> blob - 3b3a634ed0c2f2b1d0dd925891e217c07e57cf5c
> blob + 7e698531e6e75bd34bc4b2f0c280aded67f43de0
> --- lib/error.c
> +++ lib/error.c
> @@ -233,6 +233,8 @@ static const struct got_error got_errors[] = {
> { GOT_ERR_REF_BUSY, "reference cannot be updated; please try again" },
> { GOT_ERR_COMMIT_BAD_AUTHOR, "commit author formatting would "
> "make Git unhappy" },
> + { GOT_ERR_UID, "bad user ID" },
> + { GOT_ERR_GID, "bad group ID" },
> };
>
> static struct got_custom_error {
move "unix" pledge promise from gotd parent to auth process