From: Omar Polo Subject: Re: move "unix" pledge promise from gotd parent to auth process To: Stefan Sperling Cc: gameoftrees@openbsd.org Date: Thu, 29 Dec 2022 18:46:02 +0100 On 2022/12/29 17:48:09 +0100, Stefan Sperling 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 > +#include > #include > #include > > @@ -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 {