[Openvpn-devel,04/21] Fix triggering assertion of ks->authticated after tls_deauthenticate

Message ID 20211207170211.3275837-5-arne@rfc2549.org
State Accepted
Headers show
Series
  • Big buffer/frame refactoring patch set
Related show

Commit Message

Arne Schwabe Dec. 7, 2021, 5:01 p.m.
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(-)

Comments

Gert Doering Dec. 14, 2021, 8:23 a.m. | #1
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

Patch

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)