On Sat, Apr 06, 2024 at 08:04:38PM +0200, Theo Buehler wrote:
> 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.
> 
> It was a side effect of making x509 opaque. We then needed to call
> X509_get_key_usage(x) which could then fail due to the extension
> caching. Previously we would ignore caching failures:
> 
> https://github.com/openbsd/src/commit/ad70d4758940e74a630416c8072729a9c6845f30
> 
> > ok?
> 
> While I remain unconvinced, I'm not against doing something like this,
> but I don't think this diff is 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.
> > +	 */
> 
> Why do you only use half the screen for this comment?
> 
> This only asserts things but doesn't really explain why time caching is
> treated differently from all the rest in here.

Because it is a cache. it was never meant to introduce a failure at
this level.

and I am using half the screen for the comment, because we introduced
a failure here before because we did not remember this as being
important ;)

> 
> > +	x509_verify_cert_info_populate(x);
> 
> I would just have done
> 
> 	(void)x509_verify_cert_info_populate(x);
> 
> here and have left the rest alone.

Again, prior to your commit above, this function returned void, as it
was simply an opportunistic cache, keeping it returning void makes this
as clear as it was before your change.  this function should not have
a "failure mode".

> 
> >  	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;
> 
> Thanks to great API design, We can't know these aren't NULL. If they
> are, this will crash in x509_verify_asn1_time_to_time_t(), so please
> don't remove these checks and don't use nested function calls.

Ahh yes, that bug was there before, but really we should just make
x509_verify_asn1_time_to_time_t do the right thing.

> 
> > -
> > -	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
> >