From: Theo Buehler Subject: Re: Failing to cache time values should not be fatal early. To: Bob Beck Cc: jsing@openbsd.org, beck@openbsd.org, tech@openbsd.org Date: Sat, 6 Apr 2024 20:21:28 +0200 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. > > On Sat, Apr 06, 2024 at 11:42:55AM -0600, Bob Beck wrote: > > Populating the notbefore/notafter cache should not be fatal. > > > > The addition of the cache of the parsed notbefore/notafter was not > > meant to mark a certificate invalid at an earlier time, merely to > > avoid re-parsing times repeatedly (in things like rpki-client) > > > > This reverts to the earlier behaviour of saving the values as marked > > invalid, so that they may be considered in certificate validation > > later (which may then fail) > > > > This should correct Mischa's strange ftp behavior. > > > > We still have a long-standing bug here that has been exposed as to why > > extensions parsing failues fail the handshake even when server > > certificate validation is not requested - we really should not > > have to care if we are not validating the server certificate. but > > this appears to be a long standing OpenSSL inconsistency - we > > should just not pile failures on here to make it worse. > > > > ok? > > > > --- > > lib/libcrypto/x509/x509_internal.h | 2 +- > > lib/libcrypto/x509/x509_purp.c | 11 +++++++++-- > > lib/libcrypto/x509/x509_verify.c | 21 +++++---------------- > > 3 files changed, 15 insertions(+), 19 deletions(-) > > > > diff --git a/lib/libcrypto/x509/x509_internal.h b/lib/libcrypto/x509/x509_internal.h > > index 280d1ae46cf..d9da539a588 100644 > > --- a/lib/libcrypto/x509/x509_internal.h > > +++ b/lib/libcrypto/x509/x509_internal.h > > @@ -134,7 +134,7 @@ int x509_constraints_check(struct x509_constraints_names *names, > > struct x509_constraints_names *excluded, int *error); > > int x509_constraints_chain(STACK_OF(X509) *chain, int *error, > > int *depth); > > -int x509_verify_cert_info_populate(X509 *cert); > > +void x509_verify_cert_info_populate(X509 *cert); > > int x509_vfy_check_security_level(X509_STORE_CTX *ctx); > > > > __END_HIDDEN_DECLS > > diff --git a/lib/libcrypto/x509/x509_purp.c b/lib/libcrypto/x509/x509_purp.c > > index 53f4f2f9675..49d54f33516 100644 > > --- a/lib/libcrypto/x509/x509_purp.c > > +++ b/lib/libcrypto/x509/x509_purp.c > > @@ -559,8 +559,15 @@ 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; > > + /* > > + * Populate a cache of notbefore/notafter values > > + * to avoid re-parsing time later. > > + * > > + * These could be cached as invalid, and may fail > > + * later in certificate validation, but should > > + * not fail here. > > + */ > > + x509_verify_cert_info_populate(x); > > > > x->ex_flags |= EXFLAG_SET; > > } > > diff --git a/lib/libcrypto/x509/x509_verify.c b/lib/libcrypto/x509/x509_verify.c > > index 19bb925d9c6..83841d2a4c0 100644 > > --- a/lib/libcrypto/x509/x509_verify.c > > +++ b/lib/libcrypto/x509/x509_verify.c > > @@ -84,29 +84,18 @@ x509_verify_asn1_time_to_time_t(const ASN1_TIME *atime, int notAfter, > > * Cache certificate hash, and values parsed out of an X509. > > * called from cache_extensions() > > */ > > -int > > +void > > 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; > > + (void) x509_verify_asn1_time_to_time_t(X509_get_notBefore(cert), 0, > > + &cert->not_before); > > + (void) x509_verify_asn1_time_to_time_t(X509_get_notAfter(cert), 1, > > + &cert->not_after); > > } > > > > struct x509_verify_chain * > > -- > > 2.44.0 > >