On Fri, Jan 20, 2023 at 08:06:00PM +0000, Job Snijders wrote:
> While studying why X509_check_ca() is the ugly thing it is, tb@
> suggested x509v3_cache_extensions() might benefit from a wrapper to
> avoid duplication of locking and checking the stupid EXFLAG_INVALID
> flag. x509v3_cache_extensions() isn't a public function anyway.
>
> Passes regress & rpki-client.
>
> OK?
Cool, thanks. I think it's better to pull the EXFLAG_SET check into the
new function like beck has done in x509_verify_cert_cache_extensions()
which then becomes a simple wrapper (it's nice to keep the 'namespacing'
clean in x509_verify.c).
Details inline.
>
> Kind regards,
>
> Job
>
> Index: x509/x509_internal.h
> ===================================================================
> RCS file: /cvs/src/lib/libcrypto/x509/x509_internal.h,v
> retrieving revision 1.23
> diff -u -p -r1.23 x509_internal.h
> --- x509/x509_internal.h 26 Nov 2022 16:08:54 -0000 1.23
> +++ x509/x509_internal.h 20 Jan 2023 19:59:56 -0000
> @@ -94,7 +94,8 @@ int x509_vfy_check_policy(X509_STORE_CTX
> int x509_vfy_check_trust(X509_STORE_CTX *ctx);
> int x509_vfy_check_chain_extensions(X509_STORE_CTX *ctx);
> int x509_vfy_callback_indicate_completion(X509_STORE_CTX *ctx);
> -void x509v3_cache_extensions(X509 *x);
> +void x509v3_cache_extensions_internal(X509 *x);
remove the internal function here.
> +int x509v3_cache_extensions(X509 *x);
> X509 *x509_vfy_lookup_cert_match(X509_STORE_CTX *ctx, X509 *x);
>
> time_t x509_verify_asn1_time_to_time_t(const ASN1_TIME *atime, int notafter);
> Index: x509/x509_purp.c
> ===================================================================
> RCS file: /cvs/src/lib/libcrypto/x509/x509_purp.c,v
> retrieving revision 1.18
> diff -u -p -r1.18 x509_purp.c
> --- x509/x509_purp.c 26 Nov 2022 16:08:55 -0000 1.18
> +++ x509/x509_purp.c 20 Jan 2023 19:59:57 -0000
> @@ -76,7 +76,8 @@
> #define ns_reject(x, usage) \
> (((x)->ex_flags & EXFLAG_NSCERT) && !((x)->ex_nscert & (usage)))
>
> -void x509v3_cache_extensions(X509 *x);
> +void x509v3_cache_extensions_internal(X509 *x);
> +int x509v3_cache_extensions(X509 *x);
drop these two prototypes
>
> static int check_ssl_ca(const X509 *x);
> static int check_purpose_ssl_client(const X509_PURPOSE *xp, const X509 *x,
> @@ -132,12 +133,10 @@ X509_check_purpose(X509 *x, int id, int
> const X509_PURPOSE *pt;
>
> if (!(x->ex_flags & EXFLAG_SET)) {
> - CRYPTO_w_lock(CRYPTO_LOCK_X509);
> - x509v3_cache_extensions(x);
> - CRYPTO_w_unlock(CRYPTO_LOCK_X509);
> - if (x->ex_flags & EXFLAG_INVALID)
> + if (!x509v3_cache_extensions(x))
> return -1;
> }
This becomes
if (!x509v3_cache_extensions(x))
return -1;
> +
> if (id == -1)
> return 1;
> idx = X509_PURPOSE_get_by_id(id);
> @@ -450,7 +449,7 @@ setup_crldp(X509 *x)
> }
>
> void
static void
> -x509v3_cache_extensions(X509 *x)
> +x509v3_cache_extensions_internal(X509 *x)
> {
> BASIC_CONSTRAINTS *bs;
> PROXY_CERT_INFO_EXTENSION *pci;
> @@ -640,6 +639,19 @@ x509v3_cache_extensions(X509 *x)
> x->ex_flags |= EXFLAG_SET;
> }
>
> +/*
> + * 1 is success, 0 is failure.
> + */
> +int
> +x509v3_cache_extensions(X509 *x)
> +{
if ((x->ex_flags & EXFLAG_SET) == 0) {
CRYPTO_w_lock(CRYPTO_LOCK_X509);
x509v3_cache_extensions_internal(x);
CRYPTO_w_unlock(CRYPTO_LOCK_X509);
}
> + CRYPTO_w_lock(CRYPTO_LOCK_X509);
> + x509v3_cache_extensions_internal(x);
> + CRYPTO_w_unlock(CRYPTO_LOCK_X509);
> +
> + return ((x->ex_flags & EXFLAG_INVALID) == 0);
Drop outer parentheses.
> +}
> +
> /* CA checks common to all purposes
> * return codes:
> * 0 not a CA
> @@ -680,11 +692,8 @@ check_ca(const X509 *x)
> int
> X509_check_ca(X509 *x)
> {
> - if (!(x->ex_flags & EXFLAG_SET)) {
> - CRYPTO_w_lock(CRYPTO_LOCK_X509);
> + if (!(x->ex_flags & EXFLAG_SET))
> x509v3_cache_extensions(x);
> - CRYPTO_w_unlock(CRYPTO_LOCK_X509);
> - }
x509v3_cache_extensions(x)
>
> return check_ca(x);
> }
> @@ -896,19 +905,13 @@ X509_check_issued(X509 *issuer, X509 *su
> X509_get_issuer_name(subject)))
> return X509_V_ERR_SUBJECT_ISSUER_MISMATCH;
> if (!(issuer->ex_flags & EXFLAG_SET)) {
> - CRYPTO_w_lock(CRYPTO_LOCK_X509);
> - x509v3_cache_extensions(issuer);
> - CRYPTO_w_unlock(CRYPTO_LOCK_X509);
> + if (!x509v3_cache_extensions(issuer))
> + return X509_V_ERR_UNSPECIFIED;
> }
> - if (issuer->ex_flags & EXFLAG_INVALID)
> - return X509_V_ERR_UNSPECIFIED;
> if (!(subject->ex_flags & EXFLAG_SET)) {
> - CRYPTO_w_lock(CRYPTO_LOCK_X509);
> - x509v3_cache_extensions(subject);
> - CRYPTO_w_unlock(CRYPTO_LOCK_X509);
> + if (!x509v3_cache_extensions(subject))
> + return X509_V_ERR_UNSPECIFIED;
> }
> - if (subject->ex_flags & EXFLAG_INVALID)
> - return X509_V_ERR_UNSPECIFIED;
The entire hunk above becomes
if (!x509v3_cache_extensions(issuer))
return X509_V_ERR_UNSPECIFIED;
if (!x509v3_cache_extensions(subject))
return X509_V_ERR_UNSPECIFIED;
>
> if (subject->akid) {
> int ret = X509_check_akid(issuer, subject->akid);
> Index: x509/x509_verify.c
> ===================================================================
> RCS file: /cvs/src/lib/libcrypto/x509/x509_verify.c,v
> retrieving revision 1.62
> diff -u -p -r1.62 x509_verify.c
> --- x509/x509_verify.c 17 Jan 2023 23:49:28 -0000 1.62
> +++ x509/x509_verify.c 20 Jan 2023 19:59:57 -0000
> @@ -242,12 +242,9 @@ static int
> x509_verify_cert_cache_extensions(X509 *cert)
> {
return x509v3_cache_extensions(cert);
> if (!(cert->ex_flags & EXFLAG_SET)) {
> - CRYPTO_w_lock(CRYPTO_LOCK_X509);
> - x509v3_cache_extensions(cert);
> - CRYPTO_w_unlock(CRYPTO_LOCK_X509);
> + if (!x509v3_cache_extensions(cert))
> + return 0;
> }
> - if (cert->ex_flags & EXFLAG_INVALID)
> - return 0;
>
> return (cert->ex_flags & EXFLAG_SET);
> }
>
