From: Bob Beck Subject: Re: Failing to cache time values should not be fatal early. To: Theo Buehler Cc: jsing@openbsd.org, beck@openbsd.org, tech@openbsd.org Date: Sat, 6 Apr 2024 12:59:22 -0600 On Sat, Apr 06, 2024 at 08:21:28PM +0200, Theo Buehler wrote: > On Sat, Apr 06, 2024 at 12:14:13PM -0600, Bob Beck wrote: > > > > *alternatively* the cache here was added mainly because timegm() > > gmtime() are kind of horribly inefficient and so repeatly beating on > > them mattered as demonstrated by claudio profiling things in > > rpki-client. At the time I added this cache in the glorious spirit of > > OpenSSL that no performance problem can't be solved by adding another > > thing to a sea of global state (and it worked, we avoided the time > > calls) ;) > > > > What has changed now is we brought in the BoringSSL style time > > conversion using a julien date in seconds based on the unix epoch in > > zulu time, rather than the OpenSSL julien date conversion of days from > > some random year important only to time nerds for Byzantine reasons > > (<- dad joke), so we no longer need to use timegm() and gmtime(). > > These BoringSSL style conversions aren't slow. So I don't believe > > caching these values will have significant performance impact anymore, > > and perhaps we could just remove the caching of these values entirely > > if people prefer. > > Right. Unless removing this cache slows down rpki-client significantly, > I would prefer this. Yeah, I think I agree. -------------------------------------------------------- Remove not_before and not_after cacheing. This was added because our former use of timegm() and gmtime() was expensive in things like rpki-client. Now that we deal with time conversion internally we no longer have the performance issue, so remove this cache as it adds complexity and confuses people where failure modes get added at the wrong level. While we are at this correct a bug where x509_verify_asn1_time_to_time_t was not NULL safe. ok? --- lib/libcrypto/x509/x509_local.h | 2 -- lib/libcrypto/x509/x509_purp.c | 3 --- lib/libcrypto/x509/x509_verify.c | 44 ++++++++------------------------ lib/libcrypto/x509/x509_vfy.c | 22 ++-------------- 4 files changed, 12 insertions(+), 59 deletions(-) diff --git a/lib/libcrypto/x509/x509_local.h b/lib/libcrypto/x509/x509_local.h index 73cc582d7bc..f8ffd739c33 100644 --- a/lib/libcrypto/x509/x509_local.h +++ b/lib/libcrypto/x509/x509_local.h @@ -188,8 +188,6 @@ struct x509_st { struct ASIdentifiers_st *rfc3779_asid; #endif unsigned char hash[X509_CERT_HASH_LEN]; - time_t not_before; - time_t not_after; X509_CERT_AUX *aux; } /* X509 */; diff --git a/lib/libcrypto/x509/x509_purp.c b/lib/libcrypto/x509/x509_purp.c index 53f4f2f9675..e0e009c5b3c 100644 --- a/lib/libcrypto/x509/x509_purp.c +++ b/lib/libcrypto/x509/x509_purp.c @@ -559,9 +559,6 @@ x509v3_cache_extensions_internal(X509 *x) if (!x509_extension_oids_are_unique(x)) x->ex_flags |= EXFLAG_INVALID; - if (!x509_verify_cert_info_populate(x)) - x->ex_flags |= EXFLAG_INVALID; - x->ex_flags |= EXFLAG_SET; } diff --git a/lib/libcrypto/x509/x509_verify.c b/lib/libcrypto/x509/x509_verify.c index 19bb925d9c6..4dc8f0e4ae5 100644 --- a/lib/libcrypto/x509/x509_verify.c +++ b/lib/libcrypto/x509/x509_verify.c @@ -52,6 +52,9 @@ x509_verify_asn1_time_to_time_t(const ASN1_TIME *atime, int notAfter, struct tm tm = { 0 }; int type; + if (atime == NULL) + return 0; + type = ASN1_time_parse(atime->data, atime->length, &tm, atime->type); if (type == -1) return 0; @@ -80,35 +83,6 @@ x509_verify_asn1_time_to_time_t(const ASN1_TIME *atime, int notAfter, return asn1_time_tm_to_time_t(&tm, out); } -/* - * Cache certificate hash, and values parsed out of an X509. - * called from cache_extensions() - */ -int -x509_verify_cert_info_populate(X509 *cert) -{ - const ASN1_TIME *notBefore, *notAfter; - - /* - * Parse and save the cert times, or remember that they - * are unacceptable/unparsable. - */ - - cert->not_before = cert->not_after = -1; - - if ((notBefore = X509_get_notBefore(cert)) == NULL) - return 0; - if ((notAfter = X509_get_notAfter(cert)) == NULL) - return 0; - - if (!x509_verify_asn1_time_to_time_t(notBefore, 0, &cert->not_before)) - return 0; - if (!x509_verify_asn1_time_to_time_t(notAfter, 1, &cert->not_after)) - return 0; - - return 1; -} - struct x509_verify_chain * x509_verify_chain_new(void) { @@ -840,26 +814,28 @@ x509_verify_set_check_time(struct x509_verify_ctx *ctx) static int x509_verify_cert_times(X509 *cert, time_t *cmp_time, int *error) { - time_t when; + time_t when, not_before, not_after; if (cmp_time == NULL) when = time(NULL); else when = *cmp_time; - if (cert->not_before == -1) { + if (!x509_verify_asn1_time_to_time_t(X509_get_notBefore(cert), 0, + ¬_before)) { *error = X509_V_ERR_ERROR_IN_CERT_NOT_BEFORE_FIELD; return 0; } - if (when < cert->not_before) { + if (when < not_before) { *error = X509_V_ERR_CERT_NOT_YET_VALID; return 0; } - if (cert->not_after == -1) { + if (!x509_verify_asn1_time_to_time_t(X509_get_notAfter(cert), 1, + ¬_after)) { *error = X509_V_ERR_ERROR_IN_CERT_NOT_AFTER_FIELD; return 0; } - if (when > cert->not_after) { + if (when > not_after) { *error = X509_V_ERR_CERT_HAS_EXPIRED; return 0; } diff --git a/lib/libcrypto/x509/x509_vfy.c b/lib/libcrypto/x509/x509_vfy.c index 53996586391..2ca18febdd8 100644 --- a/lib/libcrypto/x509/x509_vfy.c +++ b/lib/libcrypto/x509/x509_vfy.c @@ -1744,18 +1744,6 @@ verify_cb_cert(X509_STORE_CTX *ctx, X509 *x, int depth, int err) return ctx->verify_cb(0, ctx); } - -/* Mimic OpenSSL '0 for failure' ick */ -static int -time_t_bogocmp(time_t a, time_t b) -{ - if (a == -1 || b == -1) - return 0; - if (a <= b) - return -1; - return 1; -} - /* * Check certificate validity times. * @@ -1777,10 +1765,7 @@ x509_check_cert_time(X509_STORE_CTX *ctx, X509 *x, int depth) else ptime = time(NULL); - if (x->ex_flags & EXFLAG_SET) - i = time_t_bogocmp(x->not_before, ptime); - else - i = X509_cmp_time(X509_get_notBefore(x), &ptime); + i = X509_cmp_time(X509_get_notBefore(x), &ptime); if (i >= 0 && depth < 0) return 0; @@ -1791,10 +1776,7 @@ x509_check_cert_time(X509_STORE_CTX *ctx, X509 *x, int depth) X509_V_ERR_CERT_NOT_YET_VALID)) return 0; - if (x->ex_flags & EXFLAG_SET) - i = time_t_bogocmp(x->not_after, ptime); - else - i = X509_cmp_time_internal(X509_get_notAfter(x), &ptime, 1); + i = X509_cmp_time_internal(X509_get_notAfter(x), &ptime, 1); if (i <= 0 && depth < 0) return 0; -- 2.44.0