From: Claudio Jeker Subject: Re: xlock: convert to new imsg API To: Theo Buehler , tech@openbsd.org Date: Thu, 18 Jan 2024 15:22:39 +0100 On Thu, Jan 18, 2024 at 03:16:01PM +0100, Claudio Jeker wrote: > On Thu, Jan 18, 2024 at 03:07:35PM +0100, Theo Buehler wrote: > > On Thu, Jan 18, 2024 at 03:04:37PM +0100, Theo Buehler wrote: > > > On Thu, Jan 18, 2024 at 02:33:25PM +0100, Claudio Jeker wrote: > > > > This diff converts the privsep code in xlock to use the new imsg API. > > > > Overall I think the resulting code is easier to understand which is > > > > important. > > > > > > > > Use a do { } while (n == 0); loop to read until a full message was > > > > received. I'm not sure if this pathological case can be triggered but > > > > better to fix this. > > > > > > ok, but please see below > > > > > > > -- > > > > :wq Claudio > > > > > > > > Index: app/xlockmore/xlock/privsep.c > > > > =================================================================== > > > > RCS file: /cvs/xenocara/app/xlockmore/xlock/privsep.c,v > > > > diff -u -p -r1.3 privsep.c > > > > --- app/xlockmore/xlock/privsep.c 14 Dec 2023 09:44:15 -0000 1.3 > > > > +++ app/xlockmore/xlock/privsep.c 5 Jan 2024 16:56:28 -0000 > > > > @@ -118,7 +118,6 @@ send_cmd(struct imsgbuf *ibuf, char *use > > > > warn("imsg_add"); > > > > return -1; > > > > } > > > > - wbuf->fd = -1; > > > > imsg_close(ibuf, wbuf); > > > > > > Out of curiosity, do you know where this idiom of setting the fd to -1 > > > before imsg_close() from? Did people malloc ibufs before this was > > > centralized in libutil? > > No idea. At least I did not see this pattern often. > I think there was never a case where wbuf->fd had to be set by hand. > > > > > if ((n = msgbuf_write(&ibuf->w)) == -1 && errno != EAGAIN) { > > > > @@ -130,63 +129,65 @@ send_cmd(struct imsgbuf *ibuf, char *use > > > > return 0; > > > > } > > > > > > > > +static char * > > > > +ibuf_get_string(struct ibuf *buf, size_t len) > > > > +{ > > > > + char *str; > > > > + > > > > + if (ibuf_size(buf) < len) { > > > > + errno = EBADMSG; > > > > + return (NULL); > > > > + } > > > > + str = strndup(ibuf_data(buf), len); > > > > + if (str == NULL) > > > > + return (NULL); > > > > + buf->rpos += len; > > > > + return (str); > > > > +} > > > > + > > > > static int > > > > receive_cmd(struct imsgbuf *ibuf, char **name, char **pass, char **style) > > > > { > > > > struct imsg imsg; > > > > + struct ibuf buf; > > > > struct priv_cmd_hdr hdr; > > > > - ssize_t n, nread, datalen; > > > > - void *data; > > > > + ssize_t n, nread; > > > > > > Not your bug, but please initialize *name, *pass, and *style to NULL, > > > otherwise there will be bad frees in priv_init() if you goto fail before > > > the last ibuf_get_string(). > > Right. New diff below. > > > ... and of course the freezeros need to be done conditionally on user/pass > > != NULL > > > > Uhm. The man page tells me an other story: > The freezero() function is similar to the free() function except it > ensures memory is explicitly discarded. If ptr is NULL, no action > occurs. If ptr is not NULL, the size argument must be equal to or > smaller than the size of the earlier allocation that returned ptr. On the other hand the strlen(user) used as argument to freezero will explode. Also shouldn't that be strlen(user) + 1? New version below. -- :wq Claudio Index: xlock/privsep.c =================================================================== RCS file: /cvs/xenocara/app/xlockmore/xlock/privsep.c,v diff -u -p -r1.3 privsep.c --- xlock/privsep.c 14 Dec 2023 09:44:15 -0000 1.3 +++ xlock/privsep.c 18 Jan 2024 14:21:08 -0000 @@ -118,7 +118,6 @@ send_cmd(struct imsgbuf *ibuf, char *use warn("imsg_add"); return -1; } - wbuf->fd = -1; imsg_close(ibuf, wbuf); if ((n = msgbuf_write(&ibuf->w)) == -1 && errno != EAGAIN) { @@ -130,63 +129,64 @@ send_cmd(struct imsgbuf *ibuf, char *use return 0; } +static char * +ibuf_get_string(struct ibuf *buf, size_t len) +{ + char *str; + + if (ibuf_size(buf) < len) { + errno = EBADMSG; + return (NULL); + } + str = strndup(ibuf_data(buf), len); + if (str == NULL) + return (NULL); + buf->rpos += len; + return (str); +} + static int receive_cmd(struct imsgbuf *ibuf, char **name, char **pass, char **style) { struct imsg imsg; + struct ibuf buf; struct priv_cmd_hdr hdr; - ssize_t n, nread, datalen; - void *data; + ssize_t n, nread; - if ((nread = imsg_read(ibuf)) == -1 && errno != EAGAIN) { - warn("imsg_read"); - return -1; - } - if (nread == 0) { - /* parent exited */ - exit(0); - } - if ((n = imsg_get(ibuf, &imsg)) == -1) { - warnx("imsg_get"); - return -1; - } - if (imsg.hdr.type != XLOCK_CHECKPW_CMD) { + do { + if ((nread = imsg_read(ibuf)) == -1 && errno != EAGAIN) { + warn("imsg_read"); + return -1; + } + if (nread == 0) { + /* parent exited */ + exit(0); + } + if ((n = imsg_get(ibuf, &imsg)) == -1) { + warnx("imsg_get"); + return -1; + } + } while (n == 0); + if (imsg_get_type(&imsg) != XLOCK_CHECKPW_CMD) { warnx("invalid command"); - imsg_free(&imsg); - return -1; - } - datalen = imsg.hdr.len - IMSG_HEADER_SIZE; - data = imsg.data; - if (datalen < sizeof(struct priv_cmd_hdr)) { - warnx("truncated header"); - imsg_free(&imsg); - return -1; + goto fail; } - memcpy(&hdr, data, sizeof(struct priv_cmd_hdr)); - if (datalen != sizeof(hdr) + hdr.namelen + hdr.passlen + hdr.stylelen) { - warnx("truncated strings"); - imsg_free(&imsg); - return -1; + if (imsg_get_ibuf(&imsg, &buf) == -1 || + ibuf_get(&buf, &hdr, sizeof(hdr)) == -1 || + (*name = ibuf_get_string(&buf, hdr.namelen)) == NULL || + (*pass = ibuf_get_string(&buf, hdr.passlen)) == NULL) { + warn("truncated message"); + goto fail; } - data += sizeof(struct priv_cmd_hdr); - *name = strndup(data, hdr.namelen); - if (*name == NULL) - goto nomem; - data += hdr.namelen; - *pass = strndup(data, hdr.passlen); - if (*pass == NULL) - goto nomem; - data += hdr.passlen; if (hdr.stylelen != 0) { - *style = strndup(data, hdr.stylelen); - if (*style == NULL) - goto nomem; - } else - *style = NULL; + if ((*style = ibuf_get_string(&buf, hdr.stylelen)) == NULL) { + warn("truncated message"); + goto fail; + } + } imsg_free(&imsg); return 0; -nomem: - warn("strndup"); +fail: imsg_free(&imsg); return -1; } @@ -202,21 +202,21 @@ send_result(struct imsgbuf *ibuf, int re static int receive_result(struct imsgbuf *ibuf, int *presult) { - ssize_t nread, n; + ssize_t n, nread; int retval = 0; struct imsg imsg; - if ((nread = imsg_read(ibuf)) == -1 && errno != EAGAIN) - return -1; - if (nread == 0) - return -1; - if ((n = imsg_get(ibuf, &imsg)) == -1) - return -1; - if (imsg.hdr.len - IMSG_HEADER_SIZE != sizeof(int)) - retval = -1; - if (imsg.hdr.type != XLOCK_CHECKPW_RESULT) + do { + if ((nread = imsg_read(ibuf)) == -1 && errno != EAGAIN) + return -1; + if (nread == 0) + return -1; + if ((n = imsg_get(ibuf, &imsg)) == -1) + return -1; + } while (n == 0); + if (imsg_get_type(&imsg) != XLOCK_CHECKPW_RESULT || + imsg_get_data(&imsg, presult, sizeof(*presult)) == -1) retval = -1; - memcpy(presult, imsg.data, sizeof(int)); imsg_free(&imsg); return retval; } @@ -267,13 +267,16 @@ priv_init(gid_t gid) err(1, "pledge"); while (1) { + user = pass = style = NULL; if (receive_cmd(&child_ibuf, &user, &pass, &style) == -1) { warn("receive_cmd"); result = 0; } else result = pw_check(user, pass, style); - freezero(user, strlen(user)); - freezero(pass, strlen(pass)); + if (user != NULL) + freezero(user, strlen(user) + 1); + if (pass != NULL) + freezero(pass, strlen(pass) + 1); free(style); if (send_result(&child_ibuf, result) == -1) warn("send_result");