[Openvpn-devel,v4,3/5] Implement dynamic NCP negotiation

Message ID 20200217144339.3273-4-arne@rfc2549.org
State Accepted
Headers show
Series
  • NCP v2 patch set
Related show

Commit Message

Arne Schwabe Feb. 17, 2020, 2:43 p.m.
Our current NCP version is flawed in the way that it can only indicate support for
AES-256-GCM and AES-128-GCM. While configuring client and server with different
ncp-cipher configuration directive works, the server will blindly push the first
cipher of that list to the client if the client sends IV_NCP=2.

This patches introduces IV_CIPHER sent from the client to the server that
contains the full list of cipers that the client is willing to support (*).
The server will then pick the first ciphr of its own ncp-cipher list that
the client indicates support for.

We choose a textual representation of the ciphers instead of a binary sinc
a binary would mean that we would need to have a central place to maintain
a mapping between binary and the actual cipher name. Also the normal
ncp-cipher list is quite short, so this should not be problem. It also provides
the freedom to negioate new ciphers from SSL libraries without the need to
upgrade OpenVPN/its binary cipher table.

* the client/server will also accpt the cipher specifid in --cipher but eventually
we want to get rid of --ciper. So this patch keeps a reasonable backwards
compatbility (especially poor man's NCP) but does not encourage to use --cipher
for negotiation in documentation or warning messages.

Patch V2: Remove #include "ssl_ncp.h" Note to compile on windows the patch
          "Add strsep compat function" should be applied first

Patch V3: Use string_alloc with gc instead strdup()

Patch V4: Integrate using a short lived gc from patch 006 directly
          into this patch

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 doc/openvpn.8            |  10 +++-
 src/openvpn/init.c       |   1 -
 src/openvpn/push.c       |  31 ++++++++---
 src/openvpn/ssl.c        | 115 +++++++++++++++++++++++++++++++++++++--
 src/openvpn/ssl.h        |  34 ++++++++++++
 src/openvpn/ssl_common.h |   1 -
 6 files changed, 174 insertions(+), 18 deletions(-)

Comments

Lev Stipakov Feb. 17, 2020, 3:03 p.m. | #1
The previous version has been acked, the only difference is that
this adds temporary gc to avoid storing temp data for the
duration of tunnel.

Again, built and tested with MSVC.

Acked-by: Lev Stipakov <lstipakov@gmail.com>
Gert Doering Feb. 17, 2020, 7:25 p.m. | #2
Your patch has been applied to the master branch.

I have fixed a few misspellings in the commit message and reformatted
it slightly (git am wraps too long lines, resulting in funny output).

Code lightly tested (t_client) and stared-at-it a bit.

The algorithm is not very smart on the client side (yet), so if you put
100x the same client cipher on the command line, it will send exactly this
as IV_CIPHERS= to the remote side.  Not understanding the TLS handshake
bits well enough, I'm not sure what is happening but the string does
get capped after about 300 characters.  So, no ill effects, except an
incomplete string (5/5 will improve that).

commit 5bb71ab599a6c382b6bdc9e9cb449d11d64353b8
Author: Arne Schwabe
Date:   Mon Feb 17 15:43:37 2020 +0100

     Implement dynamic NCP negotiation

     Signed-off-by: Arne Schwabe <arne@rfc2549.org>
     Acked-by: Lev Stipakov <lstipakov@gmail.com>
     Message-Id: <20200217144339.3273-4-arne@rfc2549.org>
     URL: https://www.mail-archive.com/search?l=mid&q=20200217144339.3273-4-arne@rfc2549.org
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering

Patch

diff --git a/doc/openvpn.8 b/doc/openvpn.8
index b8f2f042..d631ebe7 100644
--- a/doc/openvpn.8
+++ b/doc/openvpn.8
@@ -3117,6 +3117,11 @@  IV_NCP=2 \-\- negotiable ciphers, client supports
 pushed by the server, a value of 2 or greater indicates client
 supports AES\-GCM\-128 and AES\-GCM\-256.
 
+IV_CIPHERS=<ncp\-ciphers> \-\- the client pushes the list of configured
+ciphers with the
+\.B -\-\ncp\-ciphers
+option to the server.
+
 IV_GUI_VER=<gui_id> <version> \-\- the UI version of a UI if one is
 running, for example "de.blinkt.openvpn 0.5.47" for the
 Android app.
@@ -4374,7 +4379,8 @@  is a colon\-separated list of ciphers, and defaults to
 
 For servers, the first cipher from
 .B cipher_list
-will be pushed to clients that support cipher negotiation.
+that is also supported by the client will be pushed to clients
+that support cipher negotiation.
 
 Cipher negotiation is enabled in client\-server mode only.  I.e. if
 .B \-\-mode
@@ -4398,7 +4404,7 @@  NCP server (v2.4+) with "\-\-cipher BF\-CBC" and "\-\-ncp\-ciphers
 AES\-256\-GCM:AES\-256\-CBC" set can either specify "\-\-cipher BF\-CBC" or
 "\-\-cipher AES\-256\-CBC" and both will work.
 
-Note, for using NCP with a OpenVPN 2.4 server this list must include
+Note, for using NCP with a OpenVPN 2.4 peer this list must include
 the AES\-256\-GCM and AES\-128\-GCM ciphers.
 .\"*********************************************************
 .TP
diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index 5c2f801a..9363769f 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -2826,7 +2826,6 @@  do_init_crypto_tls(struct context *c, const unsigned int flags)
     to.replay_time = options->replay_time;
     to.tcp_mode = link_socket_proto_connection_oriented(options->ce.proto);
     to.config_ciphername = c->c1.ciphername;
-    to.config_authname = c->c1.authname;
     to.config_ncp_ciphers = options->ncp_ciphers;
     to.ncp_enabled = options->ncp_enabled;
     to.transition_window = options->transition_window;
diff --git a/src/openvpn/push.c b/src/openvpn/push.c
index 368b6920..8b634051 100644
--- a/src/openvpn/push.c
+++ b/src/openvpn/push.c
@@ -429,7 +429,7 @@  prepare_push_reply(struct context *c, struct gc_arena *gc,
     prepare_auth_token_push_reply(tls_multi, gc, push_list);
 
     /* Push cipher if client supports Negotiable Crypto Parameters */
-    if (tls_peer_info_ncp_ver(peer_info) >= 2 && o->ncp_enabled)
+    if (o->ncp_enabled)
     {
         /* if we have already created our key, we cannot *change* our own
          * cipher -> so log the fact and push the "what we have now" cipher
@@ -445,17 +445,30 @@  prepare_push_reply(struct context *c, struct gc_arena *gc,
         }
         else
         {
-            /* Push the first cipher from --ncp-ciphers to the client.
-             * TODO: actual negotiation, instead of server dictatorship. */
-            char *push_cipher = string_alloc(o->ncp_ciphers, &o->gc);
-            o->ciphername = strtok(push_cipher, ":");
+            /*
+             * Push the first cipher from --ncp-ciphers to the client that
+             * the client announces to be supporting.
+             */
+            char *push_cipher = ncp_get_best_cipher(o->ncp_ciphers, o->ciphername,
+                                                    peer_info,
+                                                    tls_multi->remote_ciphername,
+                                                    &o->gc);
+
+            if (push_cipher)
+            {
+                o->ciphername = push_cipher;
+            }
+            else
+            {
+                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);
+            }
         }
         push_option_fmt(gc, push_list, M_USAGE, "cipher %s", o->ciphername);
     }
-    else if (o->ncp_enabled)
-    {
-        tls_poor_mans_ncp(o, tls_multi->remote_ciphername);
-    }
 
     return true;
 }
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index b8351737..51b03c3b 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -1934,6 +1934,85 @@  tls_item_in_cipher_list(const char *item, const char *list)
     return token != NULL;
 }
 
+const char *
+tls_peer_ncp_list(const char *peer_info, struct gc_arena *gc)
+{
+    /* Check if the peer sends the IV_CIPHERS list */
+    const char *ncp_ciphers_start;
+    if (peer_info && (ncp_ciphers_start = strstr(peer_info, "IV_CIPHERS=")))
+    {
+        ncp_ciphers_start += strlen("IV_CIPHERS=");
+        const char *ncp_ciphers_end = strstr(ncp_ciphers_start, "\n");
+        if (!ncp_ciphers_end)
+        {
+            /* IV_CIPHERS is at end of the peer_info list and no '\n'
+             * follows */
+            ncp_ciphers_end = ncp_ciphers_start + strlen(ncp_ciphers_start);
+        }
+
+        char *ncp_ciphers_peer = string_alloc(ncp_ciphers_start, gc);
+        /* NULL terminate the copy at the right position */
+        ncp_ciphers_peer[ncp_ciphers_end - ncp_ciphers_start] = '\0';
+        return ncp_ciphers_peer;
+
+    }
+    else if (tls_peer_info_ncp_ver(peer_info)>=2)
+    {
+        /* If the peer announces IV_NCP=2 then it supports the AES GCM
+         * ciphers */
+        return "AES-256-GCM:AES-128-GCM";
+    }
+    else
+    {
+        return "";
+    }
+}
+
+char *
+ncp_get_best_cipher(const char *server_list, const char *server_cipher,
+                    const char *peer_info,  const char *remote_cipher,
+                    struct gc_arena *gc)
+{
+    /*
+     * The gc of the parameter is tied to the VPN session, create a
+     * short lived gc arena that is only valid for the duration of
+     * this function
+     */
+    struct gc_arena gc_tmp = gc_new();
+
+    const char *peer_ncp_list = tls_peer_ncp_list(peer_info, &gc_tmp);
+    char *tmp_ciphers = string_alloc(server_list, &gc_tmp);
+
+    const char *token = strsep(&tmp_ciphers, ":");
+    while (token)
+    {
+        if (tls_item_in_cipher_list(token, peer_ncp_list)
+            || streq(token, remote_cipher))
+        {
+            break;
+        }
+        token = strsep(&tmp_ciphers, ":");
+    }
+    /* We have not found a common cipher, as a last resort check if the
+     * server cipher can be used
+     */
+    if (token == NULL
+        && (tls_item_in_cipher_list(server_cipher, peer_ncp_list)
+            || streq(server_cipher, remote_cipher)))
+    {
+        token = server_cipher;
+    }
+
+    char *ret = NULL;
+    if (token != NULL)
+    {
+        ret = string_alloc(token, gc);
+    }
+
+    gc_free(&gc_tmp);
+    return ret;
+}
+
 void
 tls_poor_mans_ncp(struct options *o, const char *remote_ciphername)
 {
@@ -2322,11 +2401,15 @@  push_peer_info(struct buffer *buf, struct tls_session *session)
 
         /* support for Negotiable Crypto Parameters */
         if (session->opt->ncp_enabled
-            && (session->opt->mode == MODE_SERVER || session->opt->pull)
-            && tls_item_in_cipher_list("AES-128-GCM", session->opt->config_ncp_ciphers)
-            && tls_item_in_cipher_list("AES-256-GCM", session->opt->config_ncp_ciphers))
+            && (session->opt->mode == MODE_SERVER || session->opt->pull))
         {
-            buf_printf(&out, "IV_NCP=2\n");
+            if (tls_item_in_cipher_list("AES-128-GCM", session->opt->config_ncp_ciphers)
+                && tls_item_in_cipher_list("AES-256-GCM", session->opt->config_ncp_ciphers))
+            {
+
+                buf_printf(&out, "IV_NCP=2\n");
+            }
+            buf_printf(&out, "IV_CIPHERS=%s\n", session->opt->config_ncp_ciphers);
         }
 
         /* push compression status */
@@ -2561,6 +2644,28 @@  error:
     return false;
 }
 
+/**
+ * Returns whether the client supports NCP either by
+ * announcing IV_NCP>=2 or the IV_CIPHERS list
+ */
+static bool
+tls_peer_supports_ncp(const char *peer_info)
+{
+    if (!peer_info)
+    {
+        return false;
+    }
+    else if (tls_peer_info_ncp_ver(peer_info) >= 2
+             || strstr(peer_info, "IV_CIPHERS="))
+    {
+        return true;
+    }
+    else
+    {
+        return false;
+    }
+}
+
 static bool
 key_method_2_read(struct buffer *buf, struct tls_multi *multi, struct tls_session *session)
 {
@@ -2633,7 +2738,7 @@  key_method_2_read(struct buffer *buf, struct tls_multi *multi, struct tls_sessio
     multi->remote_ciphername =
         options_string_extract_option(options, "cipher", NULL);
 
-    if (tls_peer_info_ncp_ver(multi->peer_info) < 2)
+    if (!tls_peer_supports_ncp(multi->peer_info))
     {
         /* Peer does not support NCP, but leave NCP enabled if the local and
          * remote cipher do not match to attempt 'poor-man's NCP'.
diff --git a/src/openvpn/ssl.h b/src/openvpn/ssl.h
index a944ca3a..2a8871ed 100644
--- a/src/openvpn/ssl.h
+++ b/src/openvpn/ssl.h
@@ -512,6 +512,40 @@  tls_get_peer_info(const struct tls_multi *multi)
  */
 int tls_peer_info_ncp_ver(const char *peer_info);
 
+/**
+ * Iterates through the ciphers in server_list and return the first
+ * cipher that is also supported by the peer according to the IV_NCP
+ * and IV_CIPHER values in peer_info.
+ *
+ * We also accept a cipher that is the remote cipher of the client for
+ * "Poor man's NCP": Use peer cipher if it is an allowed (NCP) cipher.
+ * Allows non-NCP peers to upgrade their cipher individually.
+ *
+ * Make sure to call tls_session_update_crypto_params() after calling this
+ * function.
+ *
+ * @param gc   gc arena that is ONLY used to allocate the returned string
+ *
+ * @returns NULL if no common cipher is available, otherwise the best common
+ * cipher
+ */
+char *
+ncp_get_best_cipher(const char *server_list, const char *server_cipher,
+                    const char *peer_info, const char *remote_cipher,
+                    struct gc_arena *gc);
+
+
+/**
+ * Returns the support cipher list from the peer according to the IV_NCP
+ * and IV_CIPHER values in peer_info.
+ *
+ * @returns Either a string containing the ncp list that is either static
+ * or allocated via gc. If no information is available an empty string
+ * ("") is returned.
+ */
+const char *
+tls_peer_ncp_list(const char *peer_info, struct gc_arena *gc);
+
 /**
  * Check whether the ciphers in the supplied list are supported.
  *
diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h
index fb82f610..998ea3c4 100644
--- a/src/openvpn/ssl_common.h
+++ b/src/openvpn/ssl_common.h
@@ -289,7 +289,6 @@  struct tls_options
     bool tcp_mode;
 
     const char *config_ciphername;
-    const char *config_authname;
     const char *config_ncp_ciphers;
     bool ncp_enabled;