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.

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

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

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