On Fri, Jan 20, 2023 at 09:35:08PM +0100, Theo Buehler wrote:
> 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.
Thanks for the feedback!
I think all your points have been addressed in the below changeset.
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 21:09:59 -0000
@@ -94,7 +94,7 @@ 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);
+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 21:09:59 -0000
@@ -76,8 +76,6 @@
#define ns_reject(x, usage) \
(((x)->ex_flags & EXFLAG_NSCERT) && !((x)->ex_nscert & (usage)))
-void x509v3_cache_extensions(X509 *x);
-
static int check_ssl_ca(const X509 *x);
static int check_purpose_ssl_client(const X509_PURPOSE *xp, const X509 *x,
int ca);
@@ -131,13 +129,9 @@ X509_check_purpose(X509 *x, int id, int
int idx;
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)
- return -1;
- }
+ if (!x509v3_cache_extensions(x))
+ return -1;
+
if (id == -1)
return 1;
idx = X509_PURPOSE_get_by_id(id);
@@ -449,8 +443,8 @@ setup_crldp(X509 *x)
setup_dp(x, sk_DIST_POINT_value(x->crldp, i));
}
-void
-x509v3_cache_extensions(X509 *x)
+static void
+x509v3_cache_extensions_internal(X509 *x)
{
BASIC_CONSTRAINTS *bs;
PROXY_CERT_INFO_EXTENSION *pci;
@@ -640,6 +634,21 @@ 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)) {
+ CRYPTO_w_lock(CRYPTO_LOCK_X509);
+ x509v3_cache_extensions_internal(x);
+ CRYPTO_w_unlock(CRYPTO_LOCK_X509);
+ }
+
+ return (x->ex_flags & EXFLAG_INVALID) == 0;
+}
+
/* CA checks common to all purposes
* return codes:
* 0 not a CA
@@ -680,11 +689,7 @@ check_ca(const X509 *x)
int
X509_check_ca(X509 *x)
{
- if (!(x->ex_flags & EXFLAG_SET)) {
- CRYPTO_w_lock(CRYPTO_LOCK_X509);
- x509v3_cache_extensions(x);
- CRYPTO_w_unlock(CRYPTO_LOCK_X509);
- }
+ x509v3_cache_extensions(x);
return check_ca(x);
}
@@ -895,19 +900,10 @@ X509_check_issued(X509 *issuer, X509 *su
if (X509_NAME_cmp(X509_get_subject_name(issuer),
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 (issuer->ex_flags & EXFLAG_INVALID)
+
+ if (!x509v3_cache_extensions(issuer))
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 (subject->ex_flags & EXFLAG_INVALID)
+ if (!x509v3_cache_extensions(subject))
return X509_V_ERR_UNSPECIFIED;
if (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 21:09:59 -0000
@@ -241,15 +241,7 @@ x509_verify_ctx_clear(struct x509_verify
static int
x509_verify_cert_cache_extensions(X509 *cert)
{
- if (!(cert->ex_flags & EXFLAG_SET)) {
- CRYPTO_w_lock(CRYPTO_LOCK_X509);
- x509v3_cache_extensions(cert);
- CRYPTO_w_unlock(CRYPTO_LOCK_X509);
- }
- if (cert->ex_flags & EXFLAG_INVALID)
- return 0;
-
- return (cert->ex_flags & EXFLAG_SET);
+ return x509v3_cache_extensions(cert);
}
static int
