Attention is currently required from: flichtenheld, plaisthos.
Hello plaisthos, flichtenheld,
I'd like you to do a code review.
Please visit
http://gerrit.openvpn.net/c/openvpn/+/1289?usp=email
to review the following change.
Change subject: ssl_openssl: Fix some CRL mixups
......................................................................
ssl_openssl: Fix some CRL mixups
There are two ways to load CRLs in OpenSSL. They can be loaded at the
X509_STORE, shared across verifications, or loaded per verification at
the X509_STORE_CTX.
OpenVPN currently does the former. However, it also supports CRL
reloading, and tries to reload the CRL file before each connection.
OpenSSL does not really have a good way to unload objects from an
X509_STORE. OpenVPN currently does it by grabbing the
STACK_OF(X509_OBJECT) out of the X509_STORE and manually deleting all
the CRLs from it.
This mutates an OpenSSL internal object which bumps into problems if
OpenSSL ever switches to a more efficient representation. See
https://github.com/openssl/openssl/pull/28599
(It's also not thread-safe, though it doesn't look like that impacts
OpenVPN? Actually even reading that list doesn't work. See
CVE-2024-0397. This OpenSSL API was simply broken.)
Additionally, this seems to cause two OpenVPN features to not work
together. I gather backend_tls_ctx_reload_crl is trying to clear the
CRLs loaded from last time it ran. But tls_ctx_load_ca with a ca_file
can also load CRLs. tls_ctx_load_ca with ca_path will also pick up CRLs
and backend_tls_ctx_reload_crl actually ends up clobbering some state
X509_LOOKUP_hash_dir internally maintains on the X509_STORE. Likewise,
tls_verify_crl_missing can get confused between
backend_tls_ctx_reload_crl's crl_file-based CRLs and CRLs from
tls_ctx_load_ca.
Avoid all this by tracking the two CRLs separately. crl_file-based CRLs
now go onto a STACK_OF(X509_CRL) tracked on the tls_root_ctx. Now this
field can be freely reloaded by OpenVPN without reconfiguring OpenSSL.
Instead, pass the current value into OpenSSL at verification time. To
do so, we need to use the SSL_CTX_set_cert_verify_callback, which allows
swapping out the X509_verify_cert call, and also tweaking the
X509_STORE_CTX configuration before starting certificate verification.
Context: SSL_CTX_set_cert_verify_callback and the existing
verify_callback are not the same. SSL_CTX_set_cert_verify_callback wraps
the verification while verify_callback is called multiple times
throughout verification. It's too late to reconfigure X509_STORE_CTX in
verify_callback. verify_callback is usually not what you want.
Sometimes current_cert and error_depth don't quite line up, and
cert_hash_remember may end up called multiple times for a single
certificate.
I suspect some of the other verify_callback logic would also be better
done in the new callback, but I've left it alone to keep this change
minimal. verify_callback is really only usable for suppressing errors.
Application bookkeeping is better down elsewhere.
Signed-off-by: David Benjamin <[email protected]>
Change-Id: I31ac2a763209114267c35c4a9182a12d8d82f6fe
---
M src/openvpn/openssl_compat.h
M src/openvpn/ssl_openssl.c
M src/openvpn/ssl_openssl.h
M src/openvpn/ssl_verify_openssl.c
4 files changed, 34 insertions(+), 54 deletions(-)
git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/89/1289/1
diff --git a/src/openvpn/openssl_compat.h b/src/openvpn/openssl_compat.h
index e3e7cf8..f4e3ca9 100644
--- a/src/openvpn/openssl_compat.h
+++ b/src/openvpn/openssl_compat.h
@@ -52,21 +52,6 @@
/* Functionality missing in LibreSSL before 3.5 */
#if defined(LIBRESSL_VERSION_NUMBER) && LIBRESSL_VERSION_NUMBER < 0x3050000fL
-/**
- * Destroy a X509 object
- *
- * @param obj X509 object
- */
-static inline void
-X509_OBJECT_free(X509_OBJECT *obj)
-{
- if (obj)
- {
- X509_OBJECT_free_contents(obj);
- OPENSSL_free(obj);
- }
-}
-
#define EVP_CTRL_AEAD_SET_TAG EVP_CTRL_GCM_SET_TAG
#define EVP_CTRL_AEAD_GET_TAG EVP_CTRL_GCM_GET_TAG
#endif
diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
index 434df7d..d331334 100644
--- a/src/openvpn/ssl_openssl.c
+++ b/src/openvpn/ssl_openssl.c
@@ -141,6 +141,8 @@
ASSERT(NULL != ctx);
SSL_CTX_free(ctx->ctx);
ctx->ctx = NULL;
+ sk_X509_CRL_pop_free(ctx->crls, X509_CRL_free);
+ ctx->crls = NULL;
unload_xkey_provider(); /* in case it is loaded */
}
@@ -302,6 +304,22 @@
return true;
}
+static int
+cert_verify_callback(X509_STORE_CTX *ctx, void *arg)
+{
+ struct tls_session *session;
+ SSL *ssl;
+
+ ssl = X509_STORE_CTX_get_ex_data(ctx,
SSL_get_ex_data_X509_STORE_CTX_idx());
+ ASSERT(ssl);
+ session = SSL_get_ex_data(ssl, mydata_index);
+ ASSERT(session);
+
+ /* Configure CRLs. */
+ X509_STORE_CTX_set0_crls(ctx, session->opt->ssl_ctx.crls);
+ return X509_verify_cert(ctx);
+}
+
bool
tls_ctx_set_options(struct tls_root_ctx *ctx, unsigned int ssl_flags)
{
@@ -344,6 +362,7 @@
verify_flags = SSL_VERIFY_PEER;
}
SSL_CTX_set_verify(ctx->ctx, verify_flags, verify_callback);
+ SSL_CTX_set_cert_verify_callback(ctx->ctx, cert_verify_callback, NULL);
SSL_CTX_set_info_callback(ctx->ctx, info_callback);
@@ -1318,6 +1337,7 @@
backend_tls_ctx_reload_crl(struct tls_root_ctx *ssl_ctx, const char *crl_file,
bool crl_inline)
{
BIO *in = NULL;
+ STACK_OF(X509_CRL) *crls = NULL;
X509_STORE *store = SSL_CTX_get_cert_store(ssl_ctx->ctx);
if (!store)
@@ -1325,20 +1345,8 @@
crypto_msg(M_FATAL, "Cannot get certificate store");
}
- /* Always start with a cleared CRL list, for that we
- * we need to manually find the CRL object from the stack
- * and remove it */
- STACK_OF(X509_OBJECT) *objs = X509_STORE_get0_objects(store);
- for (int i = 0; i < sk_X509_OBJECT_num(objs); i++)
- {
- X509_OBJECT *obj = sk_X509_OBJECT_value(objs, i);
- ASSERT(obj);
- if (X509_OBJECT_get_type(obj) == X509_LU_CRL)
- {
- sk_X509_OBJECT_delete(objs, i);
- X509_OBJECT_free(obj);
- }
- }
+ sk_X509_CRL_pop_free(ssl_ctx->crls, X509_CRL_free);
+ ssl_ctx->crls = NULL;
X509_STORE_set_flags(store, X509_V_FLAG_CRL_CHECK |
X509_V_FLAG_CRL_CHECK_ALL);
@@ -1354,7 +1362,13 @@
if (in == NULL)
{
msg(M_WARN, "CRL: cannot read: %s", print_key_filename(crl_file,
crl_inline));
- goto end;
+ return;
+ }
+
+ crls = sk_X509_CRL_new_null();
+ if (crls == NULL)
+ {
+ crypto_msg(M_FATAL, "CRL: cannot create CRL list");
}
int num_crls_loaded = 0;
@@ -1380,18 +1394,14 @@
break;
}
- if (!X509_STORE_add_crl(store, crl))
+ if (!sk_X509_CRL_push(crls, crl))
{
- X509_CRL_free(crl);
- crypto_msg(M_WARN, "CRL: cannot add %s to store",
- print_key_filename(crl_file, crl_inline));
- break;
+ crypto_msg(M_FATAL, "CRL: cannot add CRL to list");
}
- X509_CRL_free(crl);
num_crls_loaded++;
}
msg(M_INFO, "CRL: loaded %d CRLs from file %s", num_crls_loaded, crl_file);
-end:
+ ssl_ctx->crls = crls;
BIO_free(in);
}
diff --git a/src/openvpn/ssl_openssl.h b/src/openvpn/ssl_openssl.h
index 2c1f36d..e5149dd 100644
--- a/src/openvpn/ssl_openssl.h
+++ b/src/openvpn/ssl_openssl.h
@@ -41,6 +41,7 @@
SSL_CTX *ctx;
time_t crl_last_mtime;
off_t crl_last_size;
+ STACK_OF(X509_CRL) *crls;
};
struct key_state_ssl
diff --git a/src/openvpn/ssl_verify_openssl.c b/src/openvpn/ssl_verify_openssl.c
index 40d117b..29c6dad 100644
--- a/src/openvpn/ssl_verify_openssl.c
+++ b/src/openvpn/ssl_verify_openssl.c
@@ -792,23 +792,7 @@
return false;
}
- X509_STORE *store = SSL_CTX_get_cert_store(opt->ssl_ctx.ctx);
- if (!store)
- {
- crypto_msg(M_FATAL, "Cannot get certificate store");
- }
-
- STACK_OF(X509_OBJECT) *objs = X509_STORE_get0_objects(store);
- for (int i = 0; i < sk_X509_OBJECT_num(objs); i++)
- {
- X509_OBJECT *obj = sk_X509_OBJECT_value(objs, i);
- ASSERT(obj);
- if (X509_OBJECT_get_type(obj) == X509_LU_CRL)
- {
- return false;
- }
- }
- return true;
+ return opt->ssl_ctx.crls == NULL || sk_X509_CRL_num(opt->ssl_ctx.crls) ==
0;
}
#endif /* defined(ENABLE_CRYPTO_OPENSSL) */
--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1289?usp=email
To unsubscribe, or for help writing mail filters, visit
http://gerrit.openvpn.net/settings?usp=email
Gerrit-MessageType: newchange
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I31ac2a763209114267c35c4a9182a12d8d82f6fe
Gerrit-Change-Number: 1289
Gerrit-PatchSet: 1
Gerrit-Owner: davidben <[email protected]>
Gerrit-Reviewer: flichtenheld <[email protected]>
Gerrit-Reviewer: plaisthos <[email protected]>
Gerrit-CC: openvpn-devel <[email protected]>
Gerrit-Attention: plaisthos <[email protected]>
Gerrit-Attention: flichtenheld <[email protected]>
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
