From: Claudio Jeker Subject: Re: [5/7] relayd: use imsg_get_ibuf() for variable-length CA key operations To: Rafael Sadowski Cc: tech@openbsd.org Date: Mon, 8 Jun 2026 15:41:54 +0200 On Sun, Jun 07, 2026 at 09:03:34AM +0200, Rafael Sadowski wrote: > This is a series of commits for an imsg APIv2 rework in relayd. These > were committed and tested individually. They can be reviewed in their > entirety here: > > gothub: https://rsadowski.gothub.org/?action=summary&headref=imsg_v2&path=relayd.git > PR: https://codeberg.org/rsadowski/relayd/pulls/1/files > > commit 81d0e5b17456f16bdadfeaed58e7ae5c4995beee > Author: Rafael Sadowski > Date: Sat Jun 6 21:00:09 2026 +0200 > > relayd: use imsg_get_ibuf() for variable-length CA key operations > > The IMSG_CA_PRIVENC/PRIVDEC messages carry a ctl_keyop header followed > by cko_flen (request) or cko_tlen (response) trailing bytes, so the > exact-size imsg_get_data() cannot be used. Read the header with > imsg_get_ibuf() + ibuf_get() and take the payload from the same ibuf > via ibuf_data()/ibuf_size(). > > diff --git a/ca.c b/ca.c > index 9d4fc65..082f411 100644 > --- a/ca.c > +++ b/ca.c > @@ -219,6 +219,7 @@ ca_dispatch_parent(int fd, struct privsep_proc *p, struct imsg *imsg) > int > ca_dispatch_relay(int fd, struct privsep_proc *p, struct imsg *imsg) > { > + struct ibuf ibuf; > struct ctl_keyop cko; > EVP_PKEY *pkey; > RSA *rsa; > @@ -229,11 +230,19 @@ ca_dispatch_relay(int fd, struct privsep_proc *p, struct imsg *imsg) > switch (imsg_get_type(imsg)) { > case IMSG_CA_PRIVENC: > case IMSG_CA_PRIVDEC: > - IMSG_SIZE_CHECK(imsg, (&cko)); > - bcopy(imsg->data, &cko, sizeof(cko)); > + if (imsg_get_ibuf(imsg, &ibuf) == -1) { > + log_warn("%s: imsg_get_ibuf", __func__); > + return (-1); > + } > + > + if (ibuf_get(&ibuf, &cko, sizeof(cko)) == -1) { > + log_warn("%s: ibuf_get", __func__); > + return (-1); > + } > + > if (cko.cko_proc > env->sc_conf.prefork_relay) > fatalx("%s: invalid relay proc", __func__); > - if (IMSG_DATA_SIZE(imsg) != (sizeof(cko) + cko.cko_flen)) > + if (ibuf_size(&ibuf) != (size_t)cko.cko_flen) > fatalx("%s: invalid key operation", __func__); > > if ((pkey = pkey_find(env, cko.cko_hash)) == NULL) { > @@ -256,7 +265,8 @@ ca_dispatch_relay(int fd, struct privsep_proc *p, struct imsg *imsg) > DPRINTF("%s:%d: key hash %s proc %d", > __func__, __LINE__, cko.cko_hash, cko.cko_proc); > > - from = (u_char *)imsg->data + sizeof(cko); > + from = ibuf_data(&ibuf); I think this needs again a second refactor that kills from and just uses the ibuf in the right place. Ok this calls into libssl functions but this all looks not very safe to me. > + > if ((to = calloc(1, cko.cko_tlen)) == NULL) > fatalx("%s: calloc", __func__); > > @@ -309,13 +319,14 @@ static int > rsae_send_imsg(int flen, const u_char *from, u_char *to, RSA *rsa, > int padding, u_int cmd) > { > + struct ibuf ibuf; > struct privsep *ps = env->sc_ps; > struct pollfd pfd[1]; > struct ctl_keyop cko; > int ret = 0; > char *hash; > struct iovec iov[2]; > - struct imsgbuf *ibuf; > + struct imsgbuf *imsgbuf; > struct imsgev *iev; > struct imsg imsg; > int n, done = 0, cnt = 0; > @@ -326,7 +337,7 @@ rsae_send_imsg(int flen, const u_char *from, u_char *to, RSA *rsa, > return 0; > > iev = proc_iev(ps, PROC_CA, ps->ps_instance); > - ibuf = &iev->ibuf; > + imsgbuf = &iev->ibuf; > > /* > * XXX this could be nicer... > @@ -348,16 +359,16 @@ rsae_send_imsg(int flen, const u_char *from, u_char *to, RSA *rsa, > * Send a synchronous imsg because we cannot defer the RSA > * operation in OpenSSL. > */ > - if (imsg_composev(ibuf, cmd, 0, 0, -1, iov, cnt) == -1) { > + if (imsg_composev(imsgbuf, cmd, 0, 0, -1, iov, cnt) == -1) { > log_warn("%s: imsg_composev", __func__); > return -1; > } > - if (imsgbuf_flush(ibuf) == -1) { > + if (imsgbuf_flush(imsgbuf) == -1) { > log_warn("%s: imsgbuf_flush", __func__); > return -1; > } > > - pfd[0].fd = ibuf->fd; > + pfd[0].fd = imsgbuf->fd; > pfd[0].events = POLLIN; > while (!done) { > switch (poll(pfd, 1, RELAY_TLS_PRIV_TIMEOUT)) { > @@ -374,19 +385,28 @@ rsae_send_imsg(int flen, const u_char *from, u_char *to, RSA *rsa, > default: > break; > } > - if ((n = imsgbuf_read(ibuf)) == -1) > + if ((n = imsgbuf_read(imsgbuf)) == -1) > fatalx("imsgbuf_read"); > if (n == 0) > fatalx("pipe closed"); > > while (!done) { > - if ((n = imsg_get(ibuf, &imsg)) == -1) > + if ((n = imsgbuf_get(imsgbuf, &imsg)) == -1) > fatalx("imsg_get error"); > if (n == 0) > break; > > - IMSG_SIZE_CHECK(&imsg, (&cko)); > - memcpy(&cko, imsg.data, sizeof(cko)); > + if (imsg_get_ibuf(&imsg, &ibuf) == -1) { > + log_warn("%s: imsg_get_ibuf", __func__); > + imsg_free(&imsg); > + return (-1); > + } > + > + if (ibuf_get(&ibuf, &cko, sizeof(cko)) == -1) { > + log_warn("%s: ibuf_get", __func__); > + imsg_free(&imsg); > + return (-1); > + } > > /* > * Due to earlier timed out requests, there may be > @@ -401,7 +421,7 @@ rsae_send_imsg(int flen, const u_char *from, u_char *to, RSA *rsa, > continue; > } > > - if (imsg.hdr.type != cmd) > + if (imsg_get_type(&imsg) != cmd) > fatalx("invalid response"); > > ret = cko.cko_tlen; > @@ -410,10 +430,9 @@ rsae_send_imsg(int flen, const u_char *from, u_char *to, RSA *rsa, > __func__, cmd == IMSG_CA_PRIVENC ? > "enc" : "dec", cko.cko_hash); > } else if (ret > 0) { > - if (IMSG_DATA_SIZE(&imsg) != > - (sizeof(cko) + ret)) > + if (ibuf_size(&ibuf) != (size_t)ret) > fatalx("data size"); > - toptr = (u_char *)imsg.data + sizeof(cko); > + toptr = ibuf_data(&ibuf); > memcpy(to, toptr, ret); Isnt't this just complicated for: if (ibuf_get(&ibuf, &to, ret) == -1 || ibuf_size(&ibuf) != 0) > } > done = 1; > -- :wq Claudio