From: Claudio Jeker <cjeker@diehard.n-r-g.com>
Subject: Re: isakmpd: don't reach into ASN1_STRING [step 2/2]
To: Theo Buehler <tb@theobuehler.org>
Cc: tech@openbsd.org, beck@openbsd.org
Date: Fri, 5 Dec 2025 08:46:13 +0100

On Fri, Dec 05, 2025 at 07:54:20AM +0100, Theo Buehler wrote:
> On Tue, Nov 25, 2025 at 07:07:31AM +0100, Theo Buehler wrote:
> > On Mon, Nov 24, 2025 at 03:31:58PM +0100, Theo Buehler wrote:
> > > beck and davidben have plans to make ASN1_STRING opaque in OpenSSL 4 [1].
> > > This breaks somewhere in the vicinity of 200 ports, so I don't think we'll
> > > flip the switch anytime soon in libressl, but I want base to be ready for
> > > that since that makes my life easier.
> > 
> > Here's the second step. This allows things that aren't really kosher in
> > modern X.509 like omitting seconds or allowing local times.
> > 
> > Since this transforms notBefore and notAfter into a string to be used in
> > a keynote assertion, existing libcrypto interfaces aren't suitable, so I
> > kept this mechanical by assigning the data and length fields to local
> > variables accessors and replacing tm->data by data and tm->length by
> > len, only occasionally fixing up whitespace and omitting parentheses.
> > 
> > The whole dance should be deduplicated, but I'll leave that for someone
> > else.
> 
> Anyone? It's a very simple diff... It's not my fault that it is so big :)

Looks good to me. I agree with your comment above about deduplication.
This code is spaghetti of the bad kind.
 
OK claudio@ but I did not test this. I no longer use isakmpd.

> Index: x509.c
> ===================================================================
> RCS file: /cvs/src/sbin/isakmpd/x509.c,v
> diff -u -p -r1.127 x509.c
> --- x509.c	25 Nov 2025 05:12:44 -0000	1.127
> +++ x509.c	25 Nov 2025 05:43:37 -0000
> @@ -116,7 +116,8 @@ x509_generate_kn(int id, X509 *cert)
>  	time_t	tt;
>  	char	before[15], after[15], *timecomp, *timecomp2;
>  	ASN1_TIME *tm;
> -	int	i;
> +	const unsigned char *data;
> +	int	i, len;
>  
>  	LOG_DBG((LOG_POLICY, 90,
>  	    "x509_generate_kn: generating KeyNote policy for certificate %p",
> @@ -220,8 +221,8 @@ x509_generate_kn(int id, X509 *cert)
>  	key = NULL;
>  
>  	if (((tm = X509_get_notBefore(cert)) == NULL) ||
> -	    (tm->type != V_ASN1_UTCTIME &&
> -		tm->type != V_ASN1_GENERALIZEDTIME)) {
> +	    (ASN1_STRING_type(tm) != V_ASN1_UTCTIME &&
> +	     ASN1_STRING_type(tm) != V_ASN1_GENERALIZEDTIME)) {
>  		struct tm *ltm;
>  
>  		tt = time(NULL);
> @@ -233,16 +234,18 @@ x509_generate_kn(int id, X509 *cert)
>  		strftime(before, 14, "%Y%m%d%H%M%S", ltm);
>  		timecomp = "LocalTimeOfDay";
>  	} else {
> -		if (tm->data[tm->length - 1] == 'Z') {
> +		data = ASN1_STRING_get0_data(tm);
> +		len = ASN1_STRING_length(tm);
> +		if (data[len - 1] == 'Z') {
>  			timecomp = "GMTTimeOfDay";
> -			i = tm->length - 2;
> +			i = len - 2;
>  		} else {
>  			timecomp = "LocalTimeOfDay";
> -			i = tm->length - 1;
> +			i = len - 1;
>  		}
>  
>  		for (; i >= 0; i--) {
> -			if (tm->data[i] < '0' || tm->data[i] > '9') {
> +			if (data[i] < '0' || data[i] > '9') {
>  				LOG_DBG((LOG_POLICY, 30,
>  				    "x509_generate_kn: invalid data in "
>  				    "NotValidBefore time field"));
> @@ -250,64 +253,64 @@ x509_generate_kn(int id, X509 *cert)
>  			}
>  		}
>  
> -		if (tm->type == V_ASN1_UTCTIME) {
> -			if ((tm->length < 10) || (tm->length > 13)) {
> +		if (ASN1_STRING_type(tm) == V_ASN1_UTCTIME) {
> +			if (len < 10 || len > 13) {
>  				LOG_DBG((LOG_POLICY, 30,
>  				    "x509_generate_kn: invalid length "
>  				    "of NotValidBefore time field (%d)",
> -				    tm->length));
> +				    len));
>  				goto fail;
>  			}
>  			/* Validity checks.  */
> -			if ((tm->data[2] != '0' && tm->data[2] != '1') ||
> -			    (tm->data[2] == '0' && tm->data[3] == '0') ||
> -			    (tm->data[2] == '1' && tm->data[3] > '2') ||
> -			    (tm->data[4] > '3') ||
> -			    (tm->data[4] == '0' && tm->data[5] == '0') ||
> -			    (tm->data[4] == '3' && tm->data[5] > '1') ||
> -			    (tm->data[6] > '2') ||
> -			    (tm->data[6] == '2' && tm->data[7] > '3') ||
> -			    (tm->data[8] > '5')) {
> +			if ((data[2] != '0' && data[2] != '1') ||
> +			    (data[2] == '0' && data[3] == '0') ||
> +			    (data[2] == '1' && data[3] > '2') ||
> +			    (data[4] > '3') ||
> +			    (data[4] == '0' && data[5] == '0') ||
> +			    (data[4] == '3' && data[5] > '1') ||
> +			    (data[6] > '2') ||
> +			    (data[6] == '2' && data[7] > '3') ||
> +			    (data[8] > '5')) {
>  				LOG_DBG((LOG_POLICY, 30,
>  				    "x509_generate_kn: invalid value in "
>  				    "NotValidBefore time field"));
>  				goto fail;
>  			}
>  			/* Stupid UTC tricks.  */
> -			if (tm->data[0] < '5')
> +			if (data[0] < '5')
>  				snprintf(before, sizeof before, "20%s",
> -				    tm->data);
> +				    data);
>  			else
>  				snprintf(before, sizeof before, "19%s",
> -				    tm->data);
> -		} else {	/* V_ASN1_GENERICTIME */
> -			if ((tm->length < 12) || (tm->length > 15)) {
> +				    data);
> +		} else {	/* V_ASN1_GENERALIZEDTIME */
> +			if (len < 12 || len > 15) {
>  				LOG_DBG((LOG_POLICY, 30,
>  				    "x509_generate_kn: invalid length of "
>  				    "NotValidBefore time field (%d)",
> -				    tm->length));
> +				    len));
>  				goto fail;
>  			}
>  			/* Validity checks.  */
> -			if ((tm->data[4] != '0' && tm->data[4] != '1') ||
> -			    (tm->data[4] == '0' && tm->data[5] == '0') ||
> -			    (tm->data[4] == '1' && tm->data[5] > '2') ||
> -			    (tm->data[6] > '3') ||
> -			    (tm->data[6] == '0' && tm->data[7] == '0') ||
> -			    (tm->data[6] == '3' && tm->data[7] > '1') ||
> -			    (tm->data[8] > '2') ||
> -			    (tm->data[8] == '2' && tm->data[9] > '3') ||
> -			    (tm->data[10] > '5')) {
> +			if ((data[4] != '0' && data[4] != '1') ||
> +			    (data[4] == '0' && data[5] == '0') ||
> +			    (data[4] == '1' && data[5] > '2') ||
> +			    (data[6] > '3') ||
> +			    (data[6] == '0' && data[7] == '0') ||
> +			    (data[6] == '3' && data[7] > '1') ||
> +			    (data[8] > '2') ||
> +			    (data[8] == '2' && data[9] > '3') ||
> +			    (data[10] > '5')) {
>  				LOG_DBG((LOG_POLICY, 30,
>  				    "x509_generate_kn: invalid value in "
>  				    "NotValidBefore time field"));
>  				goto fail;
>  			}
> -			snprintf(before, sizeof before, "%s", tm->data);
> +			snprintf(before, sizeof before, "%s", data);
>  		}
>  
>  		/* Fix missing seconds.  */
> -		if (tm->length < 12) {
> +		if (len < 12) {
>  			before[12] = '0';
>  			before[13] = '0';
>  		}
> @@ -317,8 +320,8 @@ x509_generate_kn(int id, X509 *cert)
>  
>  	tm = X509_get_notAfter(cert);
>  	if (tm == NULL ||
> -	    (tm->type != V_ASN1_UTCTIME &&
> -		tm->type != V_ASN1_GENERALIZEDTIME)) {
> +	    (ASN1_STRING_type(tm) != V_ASN1_UTCTIME &&
> +	     ASN1_STRING_type(tm) != V_ASN1_GENERALIZEDTIME)) {
>  		struct tm *ltm;
>  
>  		tt = time(0);
> @@ -330,16 +333,18 @@ x509_generate_kn(int id, X509 *cert)
>  		strftime(after, 14, "%Y%m%d%H%M%S", ltm);
>  		timecomp2 = "LocalTimeOfDay";
>  	} else {
> -		if (tm->data[tm->length - 1] == 'Z') {
> +		data = ASN1_STRING_get0_data(tm);
> +		len = ASN1_STRING_length(tm);
> +		if (data[len - 1] == 'Z') {
>  			timecomp2 = "GMTTimeOfDay";
> -			i = tm->length - 2;
> +			i = len - 2;
>  		} else {
>  			timecomp2 = "LocalTimeOfDay";
> -			i = tm->length - 1;
> +			i = len - 1;
>  		}
>  
>  		for (; i >= 0; i--) {
> -			if (tm->data[i] < '0' || tm->data[i] > '9') {
> +			if (data[i] < '0' || data[i] > '9') {
>  				LOG_DBG((LOG_POLICY, 30,
>  				    "x509_generate_kn: invalid data in "
>  				    "NotValidAfter time field"));
> @@ -347,64 +352,64 @@ x509_generate_kn(int id, X509 *cert)
>  			}
>  		}
>  
> -		if (tm->type == V_ASN1_UTCTIME) {
> -			if ((tm->length < 10) || (tm->length > 13)) {
> +		if (ASN1_STRING_type(tm) == V_ASN1_UTCTIME) {
> +			if (len < 10 || len > 13) {
>  				LOG_DBG((LOG_POLICY, 30,
>  				    "x509_generate_kn: invalid length of "
>  				    "NotValidAfter time field (%d)",
> -				    tm->length));
> +				    len));
>  				goto fail;
>  			}
>  			/* Validity checks. */
> -			if ((tm->data[2] != '0' && tm->data[2] != '1') ||
> -			    (tm->data[2] == '0' && tm->data[3] == '0') ||
> -			    (tm->data[2] == '1' && tm->data[3] > '2') ||
> -			    (tm->data[4] > '3') ||
> -			    (tm->data[4] == '0' && tm->data[5] == '0') ||
> -			    (tm->data[4] == '3' && tm->data[5] > '1') ||
> -			    (tm->data[6] > '2') ||
> -			    (tm->data[6] == '2' && tm->data[7] > '3') ||
> -			    (tm->data[8] > '5')) {
> +			if ((data[2] != '0' && data[2] != '1') ||
> +			    (data[2] == '0' && data[3] == '0') ||
> +			    (data[2] == '1' && data[3] > '2') ||
> +			    (data[4] > '3') ||
> +			    (data[4] == '0' && data[5] == '0') ||
> +			    (data[4] == '3' && data[5] > '1') ||
> +			    (data[6] > '2') ||
> +			    (data[6] == '2' && data[7] > '3') ||
> +			    (data[8] > '5')) {
>  				LOG_DBG((LOG_POLICY, 30,
>  				    "x509_generate_kn: invalid value in "
>  				    "NotValidAfter time field"));
>  				goto fail;
>  			}
>  			/* Stupid UTC tricks.  */
> -			if (tm->data[0] < '5')
> +			if (data[0] < '5')
>  				snprintf(after, sizeof after, "20%s",
> -				    tm->data);
> +				    data);
>  			else
>  				snprintf(after, sizeof after, "19%s",
> -				    tm->data);
> -		} else {	/* V_ASN1_GENERICTIME */
> -			if ((tm->length < 12) || (tm->length > 15)) {
> +				    data);
> +		} else {	/* V_ASN1_GENERALIZEDTIME */
> +			if (len < 12 || len > 15) {
>  				LOG_DBG((LOG_POLICY, 30,
>  				    "x509_generate_kn: invalid length of "
>  				    "NotValidAfter time field (%d)",
> -				    tm->length));
> +				    len));
>  				goto fail;
>  			}
>  			/* Validity checks.  */
> -			if ((tm->data[4] != '0' && tm->data[4] != '1') ||
> -			    (tm->data[4] == '0' && tm->data[5] == '0') ||
> -			    (tm->data[4] == '1' && tm->data[5] > '2') ||
> -			    (tm->data[6] > '3') ||
> -			    (tm->data[6] == '0' && tm->data[7] == '0') ||
> -			    (tm->data[6] == '3' && tm->data[7] > '1') ||
> -			    (tm->data[8] > '2') ||
> -			    (tm->data[8] == '2' && tm->data[9] > '3') ||
> -			    (tm->data[10] > '5')) {
> +			if ((data[4] != '0' && data[4] != '1') ||
> +			    (data[4] == '0' && data[5] == '0') ||
> +			    (data[4] == '1' && data[5] > '2') ||
> +			    (data[6] > '3') ||
> +			    (data[6] == '0' && data[7] == '0') ||
> +			    (data[6] == '3' && data[7] > '1') ||
> +			    (data[8] > '2') ||
> +			    (data[8] == '2' && data[9] > '3') ||
> +			    (data[10] > '5')) {
>  				LOG_DBG((LOG_POLICY, 30,
>  				    "x509_generate_kn: invalid value in "
>  				    "NotValidAfter time field"));
>  				goto fail;
>  			}
> -			snprintf(after, sizeof after, "%s", tm->data);
> +			snprintf(after, sizeof after, "%s", data);
>  		}
>  
>  		/* Fix missing seconds.  */
> -		if (tm->length < 12) {
> +		if (len < 12) {
>  			after[12] = '0';
>  			after[13] = '0';
>  		}
> 

-- 
:wq Claudio