From 75b414fa95f89221ae913be697c66e3cb24f166e Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Thu, 18 Sep 2025 10:00:41 -0400 Subject: [PATCH] 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 --- src/openvpn/openssl_compat.h | 15 --------- src/openvpn/ssl_openssl.c | 54 +++++++++++++++++++------------- src/openvpn/ssl_openssl.h | 1 + src/openvpn/ssl_verify_openssl.c | 18 +---------- 4 files changed, 34 insertions(+), 54 deletions(-) diff --git a/src/openvpn/openssl_compat.h b/src/openvpn/openssl_compat.h index 143a3cf632a..aab2f4b7235 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 aa1ac11b764..ed06c05b33d 100644 --- a/src/openvpn/ssl_openssl.c +++ b/src/openvpn/ssl_openssl.c @@ -141,6 +141,8 @@ tls_ctx_free(struct tls_root_ctx *ctx) 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 @@ tls_ctx_set_tls_versions(struct tls_root_ctx *ctx, unsigned int ssl_flags) 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 @@ tls_ctx_set_options(struct tls_root_ctx *ctx, unsigned int ssl_flags) 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 @@ void 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 @@ backend_tls_ctx_reload_crl(struct tls_root_ctx *ssl_ctx, const char *crl_file, b 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 @@ backend_tls_ctx_reload_crl(struct tls_root_ctx *ssl_ctx, const char *crl_file, b 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 @@ backend_tls_ctx_reload_crl(struct tls_root_ctx *ssl_ctx, const char *crl_file, b 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 08a3d532881..4b729e1cb72 100644 --- a/src/openvpn/ssl_openssl.h +++ b/src/openvpn/ssl_openssl.h @@ -41,6 +41,7 @@ struct tls_root_ctx 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 0f257e2dfdc..7af68eacac4 100644 --- a/src/openvpn/ssl_verify_openssl.c +++ b/src/openvpn/ssl_verify_openssl.c @@ -783,23 +783,7 @@ tls_verify_crl_missing(const struct tls_options *opt) 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) */