From: Stefan Sperling Subject: move "unix" pledge promise from gotd parent to auth process To: gameoftrees@openbsd.org Date: Thu, 29 Dec 2022 17:48:09 +0100 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. 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 {