Re: [5/7] relayd: use imsg_get_ibuf() for variable-length CA key operations

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 <[email protected]>
> 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