plaisthos has uploaded this change for review. (
http://gerrit.openvpn.net/c/openvpn/+/1950?usp=email )
Change subject: Drop the OpenSSL errors a failed cipher/digest lookup leaves
behind
......................................................................
Drop the OpenSSL errors a failed cipher/digest lookup leaves behind
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).
Change-Id: I440c4c73865fdbd80f2c607e83c50e345a0e2438
Signed-off-by: Drew Blokzyl <[email protected]>
Signed-off-by: Arne Schwabe <[email protected]>
---
M src/openvpn/crypto_openssl.c
M src/openvpn/openssl_compat.h
2 files changed, 33 insertions(+), 1 deletion(-)
git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/50/1950/1
diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
index 29c5fa6..575db98 100644
--- a/src/openvpn/crypto_openssl.c
+++ b/src/openvpn/crypto_openssl.c
@@ -569,7 +569,17 @@
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 @@
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 @@
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 098bdd5..3029f7a 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;
--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1950?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: I440c4c73865fdbd80f2c607e83c50e345a0e2438
Gerrit-Change-Number: 1950
Gerrit-PatchSet: 1
Gerrit-Owner: plaisthos <[email protected]>
Gerrit-CC: openvpn-devel <[email protected]>
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
