From: Job Snijders <job@openbsd.org>
Subject: Re: rpki-client: add an X509 reference to cert early on
To: Theo Buehler <tb@theobuehler.org>
Cc: tech@openbsd.org
Date: Wed, 2 Jul 2025 10:24:56 +0000

OK job@

On Wed, Jul 02, 2025 at 07:57:45AM +0200, Theo Buehler wrote:
> In order to deal with SKI and AKI, it's cleaner to have the libcrypto
> cert available in struct cert when parsing them so that the extension
> handlers can all have the same signature. Hoisting the assignment up
> in cert_parse_ee_cert() for this is very simple.
> 
> In cert_parse_pre() we own the X509 from the start, so we take an extra
> reference which we must release before exit. In the error path there's
> an X509_free() and cert_free() releases the extra reference.
> 
> Again this will become a bit simpler in a few more steps.
> 
> diff --git a/usr.sbin/rpki-client/cert.c b/usr.sbin/rpki-client/cert.c
> index 388e9099795..3c2693d5af9 100644
> --- a/usr.sbin/rpki-client/cert.c
> +++ b/usr.sbin/rpki-client/cert.c
> @@ -1198,8 +1198,9 @@ cert_check_purpose(const char *fn, X509 *x)
>   */
>  
>  static int
> -cert_parse_extensions(const char *fn, struct cert *cert, X509 *x)
> +cert_parse_extensions(const char *fn, struct cert *cert)
>  {
> +	X509		*x = cert->x509;
>  	X509_EXTENSION	*ext;
>  	ASN1_OBJECT	*obj;
>  	int		 extsz, i, nid;
> @@ -1327,6 +1328,14 @@ cert_parse_ee_cert(const char *fn, int talid, X509 *x)
>  	if ((cert = calloc(1, sizeof(struct cert))) == NULL)
>  		err(1, NULL);
>  
> +	if (!X509_up_ref(x)) {
> +		warnx("%s: X509_up_ref failed", fn);
> +		goto out;
> +	}
> +
> +	cert->x509 = x;
> +	cert->talid = talid;
> +
>  	if (X509_get_version(x) != 2) {
>  		warnx("%s: RFC 6487 4.1: X.509 version must be v3", fn);
>  		goto out;
> @@ -1335,7 +1344,7 @@ cert_parse_ee_cert(const char *fn, int talid, X509 *x)
>  	if (!cert_check_subject_and_issuer(fn, x))
>  		goto out;
>  
> -	if (!cert_parse_extensions(fn, cert, x))
> +	if (!cert_parse_extensions(fn, cert))
>  		goto out;
>  
>  	if (cert->purpose != CERT_PURPOSE_EE) {
> @@ -1344,14 +1353,6 @@ cert_parse_ee_cert(const char *fn, int talid, X509 *x)
>  		goto out;
>  	}
>  
> -	if (!X509_up_ref(x)) {
> -		warnx("%s: X509_up_ref failed", fn);
> -		goto out;
> -	}
> -
> -	cert->x509 = x;
> -	cert->talid = talid;
> -
>  	if (!constraints_validate(fn, cert))
>  		goto out;
>  
> @@ -1395,6 +1396,12 @@ cert_parse_pre(const char *fn, const unsigned char *der, size_t len)
>  		goto out;
>  	}
>  
> +	if (!X509_up_ref(x)) {
> +		warnx("%s: X509_up_ref failed", fn);
> +		goto out;
> +	}
> +	cert->x509 = x;
> +
>  	if (!x509_cache_extensions(x, fn))
>  		goto out;
>  
> @@ -1430,7 +1437,7 @@ cert_parse_pre(const char *fn, const unsigned char *der, size_t len)
>  	if (!cert_check_subject_and_issuer(fn, x))
>  		goto out;
>  
> -	if (!cert_parse_extensions(fn, cert, x))
> +	if (!cert_parse_extensions(fn, cert))
>  		goto out;
>  
>  	if (!x509_get_aki(x, fn, &cert->aki))
> @@ -1494,7 +1501,7 @@ cert_parse_pre(const char *fn, const unsigned char *der, size_t len)
>  		goto out;
>  	}
>  
> -	cert->x509 = x;
> +	X509_free(x);
>  	return cert;
>  
>   out:
>