From: Bob Beck Subject: Re: isakmpd: don't reach into ASN1_STRING [step 2/2] To: Theo Buehler Cc: tech@openbsd.org, beck@openbsd.org Date: Fri, 5 Dec 2025 12:14:55 -0700 On Fri, Dec 05, 2025 at 11:07:49AM -0700, Bob Beck wrote: > > > > On Nov 24, 2025, at 23:07, 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. > > I *think* this is ok? But that code is uglier than my butthole after a week of hiking. > > I suspect a lot of the nonconformant crap mattered 20 years ago and is not used at all today so could be removed. > > But don???t do that here. I.E. I suspect this would work fine (passes regress) but I'd just land yours for now, if we want to do this we put it in and ask isakmp people to test later. diff --git a/sbin/isakmpd/x509.c b/sbin/isakmpd/x509.c index 107ac833e8f..23d0baacbce 100644 --- a/sbin/isakmpd/x509.c +++ b/sbin/isakmpd/x509.c @@ -93,6 +93,31 @@ static LIST_HEAD(x509_list, x509_hash) *x509_tab = 0; /* Works both as a maximum index and a mask. */ static int bucket_mask; +static int convert_certificate_time(const ASN1_TIME *time, char *out, size_t out_len) { + ASN1_GENERALIZEDTIME *gentime = NULL; + const char *buf; + int ret = 0; + + if (!ASN1_TIME_check(time)) + goto err; + + buf = ASN1_STRING_get0_data(time); + if (ASN1_STRING_type(time) != V_ASN1_GENERALIZEDTIME || + strlen(buf) != 15) { + if (ASN1_TIME_to_generalizedtime(time, &gentime) == NULL) + goto err; + buf = ASN1_STRING_get0_data(gentime); + } + /* out is expected to be generalizedtime without the Z */ + if (snprintf(out, out_len, "%14s", buf) >= out_len) + goto err; + + ret = 1; + err: + ASN1_TIME_free(gentime); + return ret; +} + /* * Given an X509 certificate, create a KeyNote assertion where * Issuer/Subject -> Authorizer/Licensees. @@ -116,7 +141,6 @@ x509_generate_kn(int id, X509 *cert) time_t tt; char before[15], after[15], *timecomp, *timecomp2; ASN1_TIME *tm; - int i; LOG_DBG((LOG_POLICY, 90, "x509_generate_kn: generating KeyNote policy for certificate %p", @@ -233,86 +257,13 @@ 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') { - timecomp = "GMTTimeOfDay"; - i = tm->length - 2; - } else { - timecomp = "LocalTimeOfDay"; - i = tm->length - 1; - } - - for (; i >= 0; i--) { - if (tm->data[i] < '0' || tm->data[i] > '9') { - LOG_DBG((LOG_POLICY, 30, - "x509_generate_kn: invalid data in " - "NotValidBefore time field")); - goto fail; - } - } - - if (tm->type == V_ASN1_UTCTIME) { - if ((tm->length < 10) || (tm->length > 13)) { - LOG_DBG((LOG_POLICY, 30, - "x509_generate_kn: invalid length " - "of NotValidBefore time field (%d)", - tm->length)); - 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')) { - LOG_DBG((LOG_POLICY, 30, - "x509_generate_kn: invalid value in " - "NotValidBefore time field")); - goto fail; - } - /* Stupid UTC tricks. */ - if (tm->data[0] < '5') - snprintf(before, sizeof before, "20%s", - tm->data); - else - snprintf(before, sizeof before, "19%s", - tm->data); - } else { /* V_ASN1_GENERICTIME */ - if ((tm->length < 12) || (tm->length > 15)) { - LOG_DBG((LOG_POLICY, 30, - "x509_generate_kn: invalid length of " - "NotValidBefore time field (%d)", - tm->length)); - 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')) { - LOG_DBG((LOG_POLICY, 30, - "x509_generate_kn: invalid value in " - "NotValidBefore time field")); - goto fail; - } - snprintf(before, sizeof before, "%s", tm->data); - } - - /* Fix missing seconds. */ - if (tm->length < 12) { - before[12] = '0'; - before[13] = '0'; + if (!convert_certificate_time(tm, before, sizeof(before))) { + LOG_DBG((LOG_POLICY, 30, + "x509_generate_kn: invalid value in " + "NotValidBefore time field")); + goto fail; } - /* This will overwrite trailing 'Z'. */ - before[14] = '\0'; + timecomp = "GMTTimeOfDay"; } tm = X509_get_notAfter(cert); @@ -330,85 +281,13 @@ 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') { - timecomp2 = "GMTTimeOfDay"; - i = tm->length - 2; - } else { - timecomp2 = "LocalTimeOfDay"; - i = tm->length - 1; - } - - for (; i >= 0; i--) { - if (tm->data[i] < '0' || tm->data[i] > '9') { - LOG_DBG((LOG_POLICY, 30, - "x509_generate_kn: invalid data in " - "NotValidAfter time field")); - goto fail; - } - } - - if (tm->type == V_ASN1_UTCTIME) { - if ((tm->length < 10) || (tm->length > 13)) { - LOG_DBG((LOG_POLICY, 30, - "x509_generate_kn: invalid length of " - "NotValidAfter time field (%d)", - tm->length)); - 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')) { - LOG_DBG((LOG_POLICY, 30, - "x509_generate_kn: invalid value in " - "NotValidAfter time field")); - goto fail; - } - /* Stupid UTC tricks. */ - if (tm->data[0] < '5') - snprintf(after, sizeof after, "20%s", - tm->data); - else - snprintf(after, sizeof after, "19%s", - tm->data); - } else { /* V_ASN1_GENERICTIME */ - if ((tm->length < 12) || (tm->length > 15)) { - LOG_DBG((LOG_POLICY, 30, - "x509_generate_kn: invalid length of " - "NotValidAfter time field (%d)", - tm->length)); - 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')) { - LOG_DBG((LOG_POLICY, 30, - "x509_generate_kn: invalid value in " - "NotValidAfter time field")); - goto fail; - } - snprintf(after, sizeof after, "%s", tm->data); - } - - /* Fix missing seconds. */ - if (tm->length < 12) { - after[12] = '0'; - after[13] = '0'; + if (!convert_certificate_time(tm, after, sizeof(after))) { + LOG_DBG((LOG_POLICY, 30, + "x509_generate_kn: invalid value in " + "NotAfter time field")); + goto fail; } - after[14] = '\0'; /* This will overwrite trailing 'Z' */ + timecomp2 = "GMTTimeOfDay"; } if (asprintf(&buf, fmt, skey, ikey, timecomp, before, timecomp2, @@ -417,7 +296,7 @@ x509_generate_kn(int id, X509 *cert) "failed to allocate memory for KeyNote credential"); goto fail; } - + free(ikey); ikey = NULL; free(skey); -- 2.52.0