From patchwork Thu Apr 16 17:41:35 2026 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Gert Doering X-Patchwork-Id: 4894 Return-Path: Delivered-To: patchwork@openvpn.net Received: by 2002:a05:7000:fe4a:b0:84a:48f:a1fd with SMTP id da10csp1096046mac; Thu, 16 Apr 2026 10:42:10 -0700 (PDT) X-Forwarded-Encrypted: i=2; AFNElJ+zTwyatT3FtMKvqYMS9DNZCcueUW1M3Iauk7l+Z4bztdKsuTRw848o8XBOwyHWiB7HGQVZZPvKEE8=@openvpn.net X-Received: by 2002:a05:6870:7a13:b0:428:336f:c03d with SMTP id 586e51a60fabf-4288f03f536mr204723fac.7.1776361329750; Thu, 16 Apr 2026 10:42:09 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1776361329; cv=none; d=google.com; s=arc-20240605; b=iaf0jnYQgXJL5/ZxoaBP4BF+/CaIgZ0LYCp3d/6s7+8gbC4UHG5lqL6MSOTBLrWSPM pPFanLfZFIClTn0Uzmw6CktrdmPE2qjAEc0gQaTSB1mAPnBnds9aZhDwuItb+6PQOylB gbrnYAwIL7kE6qLLrKEsPkD47h6bq0Gqtj56ZEv0WzaDDfxzjYXcstTbQUtLPjwnDTOq R2I8QrNFhgv+mHPXgOtXLGh5JTi7/mLR9Qusv9+d3EeQuD+vfJj/JbdmhxV0IhOSjdU0 RL6DZ2uumTq5zrKOn7/AKAXrhVy8XmK9AjyxLB+6elrzaJP39xZpzObvu0IslKpLfq7E k6gw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20240605; h=errors-to:content-transfer-encoding:list-subscribe:list-help :list-post:list-archive:list-unsubscribe:list-id:precedence:subject :mime-version:references:in-reply-to:message-id:date:to:from :dkim-signature:dkim-signature:dkim-signature; bh=91rl4a596mpif8533I4AClvsnoW/+Wrgg7EYJMyyC1c=; fh=4NbAC/LsuMLI0S0hprUlLSLCiHwg6SCAifhH718Jh0Q=; b=WXoI/gjV7Eim1VJGyD9GCMpBIUn/Gtfo/PNWLH+0bD4ioGtfEALlpqUGNb1G76ZJBC yy/SX/LMlOjE4TQtk8xozkinJvPK87MsrqIk0rn9vr8N+bqUsi/7sXYSnqY87wn0Eo6U BG7kPDMr+N//8qbWBP1frytZruC3itu0ah5UyOuOgl6xmUaBSShPd3nS/S+Le88TuQ+e 7i2EgvdHoEEugSjmNsm1LSqwB4soLo1W0g9k74k+cHbetqZzLsuh8E8HcayfU4YNpgf+ FRsj3jiKtPOEyclCCr5osIhVC9hKascNul1LThQYWWNfboE2TXtr7T6qif7RsduavxuY Pmxg==; dara=google.com ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@lists.sourceforge.net header.s=beta header.b=kfbCb5uD; dkim=neutral (body hash did not verify) header.i=@sourceforge.net header.s=x header.b="C/6cYgCC"; dkim=neutral (body hash did not verify) header.i=@sf.net header.s=x header.b=lPhGqRY2; spf=pass (google.com: domain of openvpn-devel-bounces@lists.sourceforge.net designates 216.105.38.7 as permitted sender) smtp.mailfrom=openvpn-devel-bounces@lists.sourceforge.net; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=muc.de Received: from lists.sourceforge.net (lists.sourceforge.net. [216.105.38.7]) by mx.google.com with ESMTPS id 586e51a60fabf-42822b90ea9si865125fac.281.2026.04.16.10.42.09 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Thu, 16 Apr 2026 10:42:09 -0700 (PDT) Received-SPF: pass (google.com: domain of openvpn-devel-bounces@lists.sourceforge.net designates 216.105.38.7 as permitted sender) client-ip=216.105.38.7; Authentication-Results: mx.google.com; dkim=pass header.i=@lists.sourceforge.net header.s=beta header.b=kfbCb5uD; dkim=neutral (body hash did not verify) header.i=@sourceforge.net header.s=x header.b="C/6cYgCC"; dkim=neutral (body hash did not verify) header.i=@sf.net header.s=x header.b=lPhGqRY2; spf=pass (google.com: domain of openvpn-devel-bounces@lists.sourceforge.net designates 216.105.38.7 as permitted sender) smtp.mailfrom=openvpn-devel-bounces@lists.sourceforge.net; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=muc.de DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.sourceforge.net; s=beta; h=Content-Transfer-Encoding:Content-Type: List-Subscribe:List-Help:List-Post:List-Archive:List-Unsubscribe:List-Id: Subject:MIME-Version:References:In-Reply-To:Message-ID:Date:To:From:Sender: Reply-To:Cc:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=91rl4a596mpif8533I4AClvsnoW/+Wrgg7EYJMyyC1c=; b=kfbCb5uDmcAVj92mqZ9XUaooce sW67sXHEYSa1QVOtN28GdknuKlLEYtKWAYGXfHS3oR9SEnjdnqOynDnq++Vs4D+lZksCqld9mu8yG ilD6CNHTW/z2UgRq9k8+BPpQilxUz5d9wmcalBZHCwPLIwvxF542LTki0iC+Ieugw4Hc=; Received: from [127.0.0.1] (helo=sfs-ml-1.v29.lw.sourceforge.com) by sfs-ml-1.v29.lw.sourceforge.com with esmtp (Exim 4.95) (envelope-from ) id 1wDQiv-0008VZ-MH; Thu, 16 Apr 2026 17:41:53 +0000 Received: from [172.30.29.66] (helo=mx.sourceforge.net) by sfs-ml-1.v29.lw.sourceforge.com with esmtps (TLS1.2) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.95) (envelope-from ) id 1wDQit-0008VQ-Eb for openvpn-devel@lists.sourceforge.net; Thu, 16 Apr 2026 17:41:51 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=sourceforge.net; s=x; h=Content-Transfer-Encoding:MIME-Version:References: In-Reply-To:Message-ID:Date:Subject:To:From:Sender:Reply-To:Cc:Content-Type: Content-ID:Content-Description:Resent-Date:Resent-From:Resent-Sender: Resent-To:Resent-Cc:Resent-Message-ID:List-Id:List-Help:List-Unsubscribe: List-Subscribe:List-Post:List-Owner:List-Archive; bh=y51oy2hmPgzC+PB+03dSwhjojCn6kDn1pxzXTsD7aJo=; b=C/6cYgCCnYTSK4pZlYwNRuFxc8 bV/GfpNRzxbVTNcppX/izI1RspKxr3V1eBCDbdXMjUFXzT4P5B/j5G2Ru1uII6+DLHAhIq0OEYVYH GXPh8d87s6a/sKSlYojISsv+uPUafoqZjKYQnOoj38rMYxqiqnlW3RJ3Nm5P2DDmvI5E=; DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=sf.net; s=x ; h=Content-Transfer-Encoding:MIME-Version:References:In-Reply-To:Message-ID: Date:Subject:To:From:Sender:Reply-To:Cc:Content-Type:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:List-Id:List-Help:List-Unsubscribe:List-Subscribe: List-Post:List-Owner:List-Archive; bh=y51oy2hmPgzC+PB+03dSwhjojCn6kDn1pxzXTsD7aJo=; b=lPhGqRY2VoVaK+l2ilnbhCZDvA zThUh9oxbT6ZLmEtdL7HaB8Pclg5s/6mmoa9XwWNXKt5wCYN2ZMgCGGJBH8PHgWwjSrIT0KeNFRuB WYz4QyPCFUeNQqd+8ZDTZUI8DwWT2Y6WupIMMNj9kBLq7zjCjRkIAkPfgnwSjgM2zTPk=; Received: from [193.149.48.129] (helo=blue.greenie.muc.de) by sfi-mx-2.v28.lw.sourceforge.com with esmtps (TLS1.2:ECDHE-RSA-AES256-GCM-SHA384:256) (Exim 4.95) id 1wDQis-0005R8-4n for openvpn-devel@lists.sourceforge.net; Thu, 16 Apr 2026 17:41:51 +0000 Received: from blue.greenie.muc.de (localhost [127.0.0.1]) by blue.greenie.muc.de (8.18.1/8.18.1) with ESMTP id 63GHfhfS028948 for ; Thu, 16 Apr 2026 19:41:43 +0200 Received: (from gert@localhost) by blue.greenie.muc.de (8.18.1/8.18.1/Submit) id 63GHfgwZ028947 for openvpn-devel@lists.sourceforge.net; Thu, 16 Apr 2026 19:41:42 +0200 From: Gert Doering To: openvpn-devel@lists.sourceforge.net Date: Thu, 16 Apr 2026 19:41:35 +0200 Message-ID: <20260416174142.28918-1-gert@greenie.muc.de> X-Mailer: git-send-email 2.52.0 In-Reply-To: References: MIME-Version: 1.0 X-Spam-Score: 1.3 (+) X-Spam-Report: Spam detection software, running on the system "sfi-spamd-1.hosts.colo.sdot.me", has NOT identified this incoming email as spam. The original message has been attached to this so you can view it or label similar future email. If you have any questions, see the administrator of that system for details. Content preview: From: David Benjamin 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. Content analysis details: (1.3 points, 5.0 required) pts rule name description ---- ---------------------- -------------------------------------------------- 1.3 RDNS_NONE Delivered to internal network by a host with no rDNS X-Headers-End: 1wDQis-0005R8-4n Subject: [Openvpn-devel] [PATCH v12] ssl_openssl: Fix some CRL mixups X-BeenThere: openvpn-devel@lists.sourceforge.net X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: openvpn-devel-bounces@lists.sourceforge.net X-getmail-retrieved-from-mailbox: Inbox X-GMAIL-THRID: =?utf-8?q?1862649857954731299?= X-GMAIL-MSGID: =?utf-8?q?1862649857954731299?= From: David Benjamin 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. Add .clang-format section for STACK_OF since we otherwise format the line as STACK_OF(X509_CRL) * crls Signed-off-by: David Benjamin Change-Id: I31ac2a763209114267c35c4a9182a12d8d82f6fe Signed-off-by: Arne Schwabe Acked-by: Arne Schwabe Acked-by: MaxF Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1289 --- This change was reviewed on Gerrit and approved by at least one developer. I request to merge it to master. Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1289 This mail reflects revision 12 of this Change. Acked-by according to Gerrit (reflected above): Arne Schwabe MaxF diff --git a/.clang-format b/.clang-format index cbec0a4..4774d1e 100644 --- a/.clang-format +++ b/.clang-format @@ -43,7 +43,10 @@ SpacesBeforeTrailingComments: '2' SpacesInParens: Never TabWidth: '4' -TypeNames: [DWORD] +TypeNames: + - 'DWORD' + - 'STACK_OF' +TypenameMacros: ['STACK_OF'] UseTab: Never WhitespaceSensitiveMacros: [_STRINGIFY] --- diff --git a/src/openvpn/openssl_compat.h b/src/openvpn/openssl_compat.h index 8fdb39a..3494ce6 100644 --- a/src/openvpn/openssl_compat.h +++ b/src/openvpn/openssl_compat.h @@ -61,21 +61,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 e8d7262..04e3521 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 */ } @@ -304,6 +306,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) { @@ -346,6 +364,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); @@ -1374,6 +1393,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) @@ -1381,20 +1401,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); @@ -1410,7 +1418,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; @@ -1436,18 +1450,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 3926e91..373dc10 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 96ca1a4..ef8231c 100644 --- a/src/openvpn/ssl_verify_openssl.c +++ b/src/openvpn/ssl_verify_openssl.c @@ -798,23 +798,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) */