[Openvpn-devel,v2] Handle connecting clients without NCP or OCC without crashing.

Message ID 20200713093252.30916-1-gert@greenie.muc.de
State Accepted
Headers show
Series
  • [Openvpn-devel,v2] Handle connecting clients without NCP or OCC without crashing.
Related show

Commit Message

Gert Doering July 13, 2020, 9:32 a.m.
ssl_ncp.c:ncp_get_best_cipher() would crash if a client connects without
NCP (or with a NCP cipher list that does not contain the first NCP cipher
in the server list) due to a NULL pointer strcmp().

Work around / fix by just assigning an empty string to remote_cipher here
("not NULL but will never match either").

Add new warning message in multi.c for the "we do not know what the
client can do" case (no NCP and non-helpful OCC), rewrapped the existing
message to keep line lenght limit.

Signed-off-by: Gert Doering <gert@greenie.muc.de>
---
 src/openvpn/multi.c   | 17 +++++++++++++----
 src/openvpn/ssl_ncp.c |  6 ++++++
 2 files changed, 19 insertions(+), 4 deletions(-)

Comments

Arne Schwabe July 13, 2020, 9:40 a.m. | #1
Am 13.07.20 um 11:32 schrieb Gert Doering:
> ssl_ncp.c:ncp_get_best_cipher() would crash if a client connects without
> NCP (or with a NCP cipher list that does not contain the first NCP cipher
> in the server list) due to a NULL pointer strcmp().
> 
> Work around / fix by just assigning an empty string to remote_cipher here
> ("not NULL but will never match either").
> 
> Add new warning message in multi.c for the "we do not know what the
> client can do" case (no NCP and non-helpful OCC), rewrapped the existing
> message to keep line lenght limit.                       tpyo

Acked-By: Arne Schwabe <arne@rfc2549.org>
Gert Doering July 13, 2020, 9:55 a.m. | #2
Patch has been applied to the master branch.

The whitespace dragon spotted a "== NULL )" mishap, which was dutifully corrected.

commit b15fcceb1dd8b4fc2bf89deff94832f2654c3ac3
Author: Gert Doering
Date:   Mon Jul 13 11:32:52 2020 +0200

     Handle connecting clients without NCP or OCC without crashing.

     Signed-off-by: Gert Doering <gert@greenie.muc.de>
     Acked-by: Arne Schwabe <arne@rfc2549.org>
     Message-Id: <20200713093252.30916-1-gert@greenie.muc.de>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg20309.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering

Patch

diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index a2af071a..c2ffcb9d 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -1833,10 +1833,19 @@  multi_client_set_protocol_options(struct context *c)
             {
                 struct gc_arena gc = gc_new();
                 const char *peer_ciphers = tls_peer_ncp_list(peer_info, &gc);
-                msg(M_INFO, "PUSH: No common cipher between server and client."
-                    "Expect this connection not to work. "
-                    "Server ncp-ciphers: '%s', client supported ciphers '%s'",
-                    o->ncp_ciphers, peer_ciphers);
+                if (strlen(peer_ciphers) > 0)
+                {
+                    msg(M_INFO, "PUSH: No common cipher between server and "
+                        "client. Expect this connection not to work. Server "
+                        "ncp-ciphers: '%s', client supported ciphers '%s'",
+                        o->ncp_ciphers, peer_ciphers);
+                }
+                else
+                {
+                    msg(M_INFO, "No NCP data received from peer, falling back "
+                        "to --cipher '%s'. Peer reports in OCC --cipher '%s'",
+                        o->ciphername, np(tls_multi->remote_ciphername));
+                }
                 gc_free(&gc);
             }
         }
diff --git a/src/openvpn/ssl_ncp.c b/src/openvpn/ssl_ncp.c
index ea1dc960..fe7f5855 100644
--- a/src/openvpn/ssl_ncp.c
+++ b/src/openvpn/ssl_ncp.c
@@ -225,6 +225,12 @@  ncp_get_best_cipher(const char *server_list, const char *server_cipher,
 
     const char *peer_ncp_list = tls_peer_ncp_list(peer_info, &gc_tmp);
 
+    /* non-NCP client without OCC?  "assume nothing" */
+    if (remote_cipher == NULL )
+    {
+        remote_cipher = "";
+    }
+
     char *tmp_ciphers = string_alloc(server_list, &gc_tmp);
 
     const char *token = strsep(&tmp_ciphers, ":");