diff mbox series

[Openvpn-devel,1/2] Drop the OpenSSL errors a failed cipher/digest lookup leaves behind

Message ID 20260922140520.71500-2-drew@linuxkids.com
State New
Headers
Series Stop failed cipher/digest lookups from polluting the OpenSSL error queue |

Commit Message

Drew Blokzyl Sept. 22, 2026, 2:05 p.m. UTC 
  cipher_get() looks a cipher up with EVP_CIPHER_fetch() and hands the
result, possibly NULL, to callers that only care whether it exists:
cipher_valid_reason(), cipher_kt_mode_cbc/ofb_cfb/aead(),
cipher_kt_block_size(), cipher_kt_insecure(). A failed fetch is a normal
outcome for them, but under OpenSSL 3 it also pushes an
EVP_R_UNSUPPORTED error ("digital envelope routines::unsupported,
Algorithm (none : 0)") onto the thread's error queue, and nobody pops it.

The common way to get there is not exotic. A server that does not set
--cipher gets the legacy default BF-CBC, which is not in --data-ciphers,
so do_init_crypto_tls() initialises the pre-negotiation key_type with
cipher "none". Every new client instance then runs init_instance() ->
do_init_crypto_tls() -> cipher_kt_mode_ofb_cfb("none"), and the frame
and OCC calculations (calculate_crypto_overhead(), frame_calculate_*())
walk the same key_type, each fetching "none" and failing.
cipher_kt_block_size() adds a second case for AEAD ciphers whose CBC
sibling does not exist (CHACHA20-POLY1305 -> "CHACHA20-CBC").
md_valid() has the same shape for digests.

The stale entry then misleads code that classifies an unrelated failure
with ERR_peek_error(), which returns the OLDEST queued entry. The visible
symptom is backend_tls_ctx_reload_crl() logging "CRL: cannot read CRL
from file" on the first handshake after the CRL file changes although
the CRL loaded fine (GitHub #1103). Traced with gdb on OpenVPN 2.7.0 and
master with OpenSSL 3.5.5: the single entry on the queue at reload entry
is the cipher_kt_mode_ofb_cfb("none") fetch from do_init_crypto_tls()
of that same client instance.

Bracket the probing fetches with ERR_set_mark()/ERR_pop_to_mark() so a
failed lookup leaves the queue as it found it; the return value already
carries the answer these callers want. wolfSSL's compatibility layer has
no error marks, so openssl_compat.h maps them to ERR_clear_error() there.

With this change the error queue is empty at multi_create_instance() and
at backend_tls_ctx_reload_crl() entry for UDP, TCP and CHACHA20-POLY1305
clients, and the spurious warning is gone: three CRL replacements, three
handshakes, zero warnings (unpatched: three of three).

Signed-off-by: Drew Blokzyl <drew@linuxkids.com>
---
 src/openvpn/crypto_openssl.c | 16 +++++++++++++++-
 src/openvpn/openssl_compat.h | 18 ++++++++++++++++++
 2 files changed, 33 insertions(+), 1 deletion(-)
diff mbox series

Patch

diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
index 29c5fa68..575db985 100644
--- a/src/openvpn/crypto_openssl.c
+++ b/src/openvpn/crypto_openssl.c
@@ -569,7 +569,17 @@  cipher_get(const char *ciphername)
     ASSERT(ciphername);
 
     ciphername = translate_cipher_name_from_openvpn(ciphername);
-    return EVP_CIPHER_fetch(NULL, ciphername, NULL);
+
+    /* A failed fetch leaves an EVP "unsupported" error on the thread's
+     * error queue. Callers legitimately probe names OpenSSL does not know
+     * ("none" for the not-yet-negotiated key_type, the CBC sibling of an
+     * AEAD cipher, user supplied names) and only look at the return value,
+     * so drop whatever the fetch raised instead of leaving it for an
+     * unrelated ERR_peek_error() to misinterpret later. */
+    ERR_set_mark();
+    evp_cipher_type *cipher = EVP_CIPHER_fetch(NULL, ciphername, NULL);
+    ERR_pop_to_mark();
+    return cipher;
 }
 
 bool
@@ -692,7 +702,9 @@  cipher_kt_block_size(const char *ciphername)
 
     strcpy(mode_str, "-CBC");
 
+    ERR_set_mark();
     cbc_cipher = EVP_CIPHER_fetch(NULL, translate_cipher_name_from_openvpn(name), NULL);
+    ERR_pop_to_mark();
     if (cbc_cipher)
     {
         block_size = EVP_CIPHER_block_size(cbc_cipher);
@@ -1001,7 +1013,9 @@  md_get(const char *digest)
 bool
 md_valid(const char *digest)
 {
+    ERR_set_mark();
     evp_md_type *md = EVP_MD_fetch(NULL, digest, NULL);
+    ERR_pop_to_mark();
     bool valid = (md != NULL);
     EVP_MD_free(md);
     return valid;
diff --git a/src/openvpn/openssl_compat.h b/src/openvpn/openssl_compat.h
index 098bdd56..3029f7ac 100644
--- a/src/openvpn/openssl_compat.h
+++ b/src/openvpn/openssl_compat.h
@@ -47,6 +47,24 @@ 
 
 /* Define the type of error. This is something that is less
  * intrusive than casts everywhere */
+#if defined(ENABLE_CRYPTO_WOLFSSL)
+/* wolfSSL's OpenSSL compatibility layer has no error queue marks. The
+ * callers use them to drop what a failed lookup raised, so fall back to
+ * clearing the queue. */
+static inline int
+ERR_set_mark(void)
+{
+    return 1;
+}
+
+static inline int
+ERR_pop_to_mark(void)
+{
+    ERR_clear_error();
+    return 1;
+}
+#endif /* defined(ENABLE_CRYPTO_WOLFSSL) */
+
 #if defined(OPENSSL_IS_AWSLC)
 typedef uint32_t openssl_err_t;
 typedef size_t openssl_stack_size_t;