From: Theo Buehler <tb@theobuehler.org>
Subject: Re: relayd reload race crash
To: Rafael Sadowski <rafael@sizeofvoid.org>
Cc: Nick Owens <mischief@offblast.org>, tech@openbsd.org, Claudio Jeker <cjeker@diehard.n-r-g.com>
Date: Tue, 3 Mar 2026 15:47:33 +0100

On Tue, Mar 03, 2026 at 03:28:46PM +0100, Rafael Sadowski wrote:
> On Tue Mar 03, 2026 at 03:03:58PM +0100, Theo Buehler wrote:
> > > 
> > > diff --git a/ca.c b/ca.c
> > > index 167fb1c..3515977 100644
> > > --- a/ca.c
> > > +++ b/ca.c
> > > @@ -234,11 +234,33 @@ ca_dispatch_relay(int fd, struct privsep_proc *p, struct imsg *imsg)
> > >  			fatalx("%s: invalid relay proc", __func__);
> > >  		if (IMSG_DATA_SIZE(imsg) != (sizeof(cko) + cko.cko_flen))
> > >  			fatalx("%s: invalid key operation", __func__);
> > > -		if ((pkey = pkey_find(env, cko.cko_hash)) == NULL)
> > > -			fatalx("%s: invalid relay hash '%s'",
> > > +
> > > +		if ((pkey = pkey_find(env, cko.cko_hash)) == NULL) {
> > > +			log_warnx("%s: invalid relay hash '%s'",
> > >  			    __func__, cko.cko_hash);
> > > -		if ((rsa = EVP_PKEY_get1_RSA(pkey)) == NULL)
> > > -			fatalx("%s: invalid relay key", __func__);
> > > +			/* Signal failure to the waiting relay worker. */
> > > +			cko.cko_tlen = -1;
> > > +			iov[c].iov_base = &cko;
> > > +			iov[c++].iov_len = sizeof(cko);
> > > +			if (proc_composev_imsg(env->sc_ps, PROC_RELAY,
> > > +			    cko.cko_proc, imsg->hdr.type, -1, -1, iov,
> > > +			     c) == -1)
> > > +				log_warn("%s: proc_composev_imsg", __func__);
> > > +			break;
> > > +		}
> > > +
> > > +		if ((rsa = EVP_PKEY_get1_RSA(pkey)) == NULL) {
> > 
> > How does a partially initialized (or !RSA pkey end up in the sc_pkeys?
> 
> Thanks Theo,
> 
> I know what you mean, it should never happen, and if EVP_PKEY_get1_RSA
> fails, the message is sent.

Yes, I think it should remain a fatalx(). I haven't chased through all
the layers, but maybe there should be a corresponding check that
EVP_PKEY_get1_RSA() doesn't fail in pkey_add() or some more appropriate
place?

(fine with me to do this in a separate commit)

fwiw, this diff seems ok to me (haven't run it through regress).


> 
> As I see it, we only support RSA.
> 
> diff --git a/ca.c b/ca.c
> index 167fb1c..33e3003 100644
> --- a/ca.c
> +++ b/ca.c
> @@ -234,9 +234,21 @@ ca_dispatch_relay(int fd, struct privsep_proc *p, struct imsg *imsg)
>  			fatalx("%s: invalid relay proc", __func__);
>  		if (IMSG_DATA_SIZE(imsg) != (sizeof(cko) + cko.cko_flen))
>  			fatalx("%s: invalid key operation", __func__);
> -		if ((pkey = pkey_find(env, cko.cko_hash)) == NULL)
> -			fatalx("%s: invalid relay hash '%s'",
> +
> +		if ((pkey = pkey_find(env, cko.cko_hash)) == NULL) {
> +			log_warnx("%s: invalid relay hash '%s'",
>  			    __func__, cko.cko_hash);
> +			/* Signal failure to the waiting relay worker. */
> +			cko.cko_tlen = -1;
> +			iov[c].iov_base = &cko;
> +			iov[c++].iov_len = sizeof(cko);
> +			if (proc_composev_imsg(env->sc_ps, PROC_RELAY,
> +			    cko.cko_proc, imsg->hdr.type, -1, -1, iov,
> +			     c) == -1)
> +				log_warn("%s: proc_composev_imsg", __func__);
> +			break;
> +		}
> +
>  		if ((rsa = EVP_PKEY_get1_RSA(pkey)) == NULL)
>  			fatalx("%s: invalid relay key", __func__);
>  
> @@ -392,7 +404,11 @@ rsae_send_imsg(int flen, const u_char *from, u_char *to, RSA *rsa,
>  				fatalx("invalid response");
>  
>  			ret = cko.cko_tlen;
> -			if (ret > 0) {
> +			if (ret == -1) {
> +				log_warnx("%s: priv%s failed for key %s",
> +				    __func__, cmd == IMSG_CA_PRIVENC ?
> +				    "enc" : "dec", cko.cko_hash);
> +			} else if (ret > 0) {
>  				if (IMSG_DATA_SIZE(&imsg) !=
>  				    (sizeof(cko) + ret))
>  					fatalx("data size");