| Message ID | 20211207170211.3275837-5-arne@rfc2549.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Big buffer/frame refactoring patch set | expand |
Acked-by: Gert Doering <gert@greenie.muc.de> I have not tested the actuall ASSERT() crash, but went through the change asking myself "so, what will happen instead, now?" - handle_data_channel_packet() will now ignore all keys that are no longer KS_AUTH_TRUE, so if there is no other key, it will end up in "TLS error: local/remote TLS keys are out of sync" and drop the incoming packet (buf->len = 0). - tls_select_encryption_key() will also ignore those keys, and possibly return NULL. This is only called from tls_pre_encrypt(), which will log "TLS Warning: no data channel send key available" in this case, and drop the outgoing packlet (buf->len = 0). Looking at tls_deauthenticate(), the problem described sounds real - this function sets all keys to KS_AUTH_FALSE, without changing ks->state, thus the next incoming or outgoing packet for that peer would trigger the ASSERT() (and this looks like could happen on regular renegotiation, like on a username or CN change, so this sounds like an actual DoS vector againt a "master" server...). The patch should fix this, without introducing undesired new side effects (like, bypassing KS_AUTH_FALSE). Subjected to a full set of server tests (passed). Your patch has been applied to the master branch. commit 75f9fb6f5b9bf57578086c3e42870bba4a914d7c Author: Arne Schwabe Date: Tue Dec 7 18:01:54 2021 +0100 Fix triggering assertion of ks->authenticated after tls_deauthenticate Signed-off-by: Arne Schwabe <arne@rfc2549.org> Acked-by: Gert Doering <gert@greenie.muc.de> Message-Id: <20211207170211.3275837-5-arne@rfc2549.org> URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg23340.html Signed-off-by: Gert Doering <gert@greenie.muc.de> -- kind regards, Gert Doering
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c index 05096ee0a..8cbb129d2 100644 --- a/src/openvpn/ssl.c +++ b/src/openvpn/ssl.c @@ -3276,9 +3276,9 @@ handle_data_channel_packet(struct tls_multi *multi, * active side is the client which initiates connections). */ if (ks->state >= S_GENERATED_KEYS && key_id == ks->key_id + && ks->authenticated == KS_AUTH_TRUE && (floated || link_socket_actual_match(from, &ks->remote_addr))) { - ASSERT(ks->authenticated == KS_AUTH_TRUE); if (!ks->crypto_options.key_ctx_bi.initialized) { msg(D_MULTI_DROPPED, @@ -3861,9 +3861,8 @@ struct key_state *tls_select_encryption_key(struct tls_multi *multi) for (int i = 0; i < KEY_SCAN_SIZE; ++i) { struct key_state *ks = get_key_scan(multi, i); - if (ks->state >= S_GENERATED_KEYS) + if (ks->state >= S_GENERATED_KEYS && ks->authenticated == KS_AUTH_TRUE) { - ASSERT(ks->authenticated == KS_AUTH_TRUE); ASSERT(ks->crypto_options.key_ctx_bi.initialized); if (!ks_select)
When tls_deauthenticate is called (e.g. by management kicking of a client) the key auth state is changed to KS_AUTH_FALSE while the key state is still in S_GENERATED_KEYS. This triggers the assertion. Remove the assertions and instead check that the auth state is KS_AUTH_TRUE Signed-off-by: Arne Schwabe <arne@rfc2549.org> --- src/openvpn/ssl.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)