Repository navigation
Conversation
|
Patch sent to openvpn-devel on 2026-09-10 as |
|
AFAICT that patch did not make it to the list. |
47ddf1f to
d0a54d3
Compare
linuxkd
changed the title
|
Right, the list rejected the first send (non-member). Subscribed now; the reworked two-patch series went out today as |
|
Patch looks good. I pushed it to gerrit to be able to see if it also works on all the other platforms that github actions are not testing (http://gerrit.openvpn.net/c/openvpn/+/1942). |
d0a54d3 to
628ce17
Compare
| if (strcmp("none", ciphername) == 0) | ||
| { | ||
| return NULL; | ||
| } |
There was a problem hiding this comment.
Could you if you make a v3 of this patch also include similar code to the md_get function? Even thought it does not cause a problem right now, it suffers the same fundamental problem.
There was a problem hiding this comment.
Done in v3: md_get() has the same guard. One thing to be aware of: md_get("none") used to fail fatally with "Message hash algorithm not found" and now returns NULL. No caller passes "none" today (md_kt_name(), md_kt_size() and md_defined() check first), so nothing changes, but say if you would rather see an ASSERT there.
| * operation in this thread would otherwise be what ERR_peek_error() | ||
| * returns, and a clean EOF gets reported as "cannot read CRL". | ||
| */ | ||
| ERR_clear_error(); |
There was a problem hiding this comment.
I would be more at easy with this code if we do something like
if (ERR_peek_error() != 0)
{
crypto_msg(D_LOW, "Warning OpenSSL error queue not empty on CRL load");
}
ERR_clear_error();
to still report this condition but not cause the problems.
There was a problem hiding this comment.
Done in v3, as you sketched it: report at D_LOW with the queued errors, then ERR_clear_error(). Note crypto_msg() only drains the queue when the level is enabled, so the explicit clear is what empties it at normal verbosity. Tested patch 2 alone on master with the "none" polluter still live: the D_LOW line fires with the stale unsupported entry printed above it, then "loaded 1 CRLs" and no false warning. v3 on the list as 20260930132707.51452-1-drew@linuxkids.com, branch updated.
cipher_get() hands EVP_CIPHER_fetch() whatever name it is given, and the
callers that only ask whether a cipher exists or which mode it has
(cipher_kt_mode_cbc/ofb_cfb/aead(), cipher_kt_block_size(),
cipher_kt_insecure()) treat NULL as "not that". For the "none" cipher
that is the expected answer, but under OpenSSL 3 the failed fetch also
pushes EVP_R_UNSUPPORTED ("digital envelope routines::unsupported,
Algorithm (none : 0)") onto the thread's error queue, and nothing pops
it.
"none" is what every server without --cipher carries in its
pre-negotiation key_type: the legacy BF-CBC default is not in
--data-ciphers, so do_init_crypto_tls() initialises the key_type with
cipher "none". Each new client instance walks it in init_instance() ->
do_init_crypto_tls() -> cipher_kt_mode_ofb_cfb("none") and in the frame
and OCC calculations, and tls_ctx_reload_crl() runs right after. Its
EOF test reads ERR_peek_error(), the OLDEST queued entry, so on the
first handshake after the CRL file changed it finds the stale
"unsupported" error and logs "CRL: cannot read CRL from file" for a CRL
it loaded fine (GitHub OpenVPN#1103). Traced with gdb on 2.7.0 and master
against OpenSSL 3.5.5.
Return NULL for "none" before touching OpenSSL, as cipher_kt_name()
already does. Real cipher names behave as before, and
cipher_valid_reason() still finds the OpenSSL reason on the queue when
it reports an unknown cipher. md_get() gets the same guard: no caller
passes "none" today (md_kt_name(), md_kt_size() and md_defined() check
first), but it has the same shape and would leave the same entry.
Left alone on purpose: cipher_kt_block_size()'s probe for the CBC
sibling of an AEAD cipher (CHACHA20-POLY1305 -> "CHACHA20-CBC") and
md_valid() leave the same kind of entry, but neither runs between
client instance creation and the CRL reload. The next commit makes that
reload robust against any leftover and reports one when it sees it.
With this change the queue is empty at multi_create_instance() and at
backend_tls_ctx_reload_crl() entry for UDP, TCP and CHACHA20-POLY1305
clients; CRL replacements give clean reloads (unpatched: a warning
every time).
Signed-off-by: Drew Blokzyl <drew@linuxkids.com>
backend_tls_ctx_reload_crl() treats a NULL from PEM_read_bio_X509_CRL() as EOF when ERR_peek_error() shows PEM_R_NO_START_LINE. ERR_peek_error() returns the OLDEST queued error, so any entry left behind earlier in the thread turns a clean EOF into a "CRL: cannot read CRL from file" warning, prints the unrelated errors as if they came from the CRL file, and still installs the CRLs already parsed. The previous commit removes the leftover that triggered this in practice; this one stops the loop from depending on the queue being clean at all. If the queue is not empty when the CRL is loaded, say so at D_LOW with the queued errors, since that is a bug somewhere else worth seeing, then start the loop from an empty queue so only errors raised by PEM_read_bio_X509_CRL() are visible. Test the error it raised last rather than the oldest one, and clear the queue on the EOF path instead of popping a single entry. Signed-off-by: Drew Blokzyl <drew@linuxkids.com>
628ce17 to
fe7dd55
Compare
This branch has not been deployed
3 participants
Root-cause follow-up to #1103. Review copy of the series on openvpn-devel: v1
<20260922140520.71500-1-drew@linuxkids.com>, v2<20260928143730.47047-1-drew@linuxkids.com>, v3<20260930132707.51452-1-drew@linuxkids.com>(2026-09-30). Gerrit change 1950 carries v1 of patch 1.The stale entry that misled the CRL reload is left by
cipher_get()being asked for the ciphernone: the pre-negotiation key_type every server without--ciphergets (BF-CBC default, not in--data-ciphers), walked bydo_init_crypto_tls()and the frame/OCC helpers for every new client instance. Details and the gdb trace in #1103.cipher_get()andmd_get()return NULL fornonebefore touching OpenSSL, ascipher_kt_name()already does. No error marks, no wolfSSL shim,cipher_valid_reason()keeps the OpenSSL reason on the queue.md_get("none")changes from a fatal to a NULL return; no caller passes it today. The CBC-sibling probe incipher_kt_block_size()andmd_valid()are named in the commit message and left alone.PEM_read_bio_X509_CRL()raised last.Validated on aarch64 Ubuntu 26.04 (OpenSSL 3.5.5, DCO) with gdb watching the queue: UDP, TCP, CHACHA20-POLY1305 and a plain client after it enter
multi_create_instance()andbackend_tls_ctx_reload_crl()with an empty queue, four CRL replacements give four clean reloads, a garbage CRL still fails withloaded 0 CRLs/VERIFY ERROR: CRL not loaded. Patch 2 alone on master (polluter still live): the D_LOW line fires with the stale entry, thenloaded 1 CRLs, no false warning.