From: Theo Buehler Subject: Re: isakmpd: don't reach into ASN1_STRING [step 2/2] To: tech@openbsd.org Cc: beck@openbsd.org Date: Fri, 5 Dec 2025 07:54:20 +0100 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 :) 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'; }