[Openvpn-devel] [PATCH v4 0/5] NCP v2 patch set

All of lore.kernel.org
 help / color / mirror / Atom feed

* [Openvpn-devel] [PATCH v4 0/5] NCP v2 patch set
@ 2020-02-17 14:43 Arne Schwabe
  2020-02-17 14:43 ` [Openvpn-devel] [PATCH v4 1/5] Only announce IV_NCP=2 when we are willing to support these ciphers Arne Schwabe
                   ` (6 more replies)
  0 siblings, 7 replies; 22+ messages in thread
From: Arne Schwabe @ 2020-02-17 14:43 UTC (permalink / raw)
  To: openvpn-devel

Resend all patches to make applying them easier and sort them into the order
they should be applied to keep Windows happy

Arne Schwabe (5):
  Only announce IV_NCP=2 when we are willing to support these ciphers
  Add strsep compat function
  Implement dynamic NCP negotiation
  Move NCP related function into a seperate file and add unit tests
  Normalise ncp-ciphers option and restrict it to 127 bytes

 configure.ac                         |   2 +-
 doc/openvpn.8                        |  13 +-
 src/compat/Makefile.am               |   1 +
 src/compat/compat-strsep.c           |  61 ++++++
 src/compat/compat.h                  |   4 +
 src/compat/compat.vcxproj            |   3 +-
 src/openvpn/Makefile.am              |   1 +
 src/openvpn/init.c                   |   3 +-
 src/openvpn/multi.c                  |   1 +
 src/openvpn/openvpn.vcxproj          |   2 +
 src/openvpn/openvpn.vcxproj.filters  |   8 +-
 src/openvpn/options.c                |  15 +-
 src/openvpn/push.c                   |  32 +++-
 src/openvpn/ssl.c                    |  85 +--------
 src/openvpn/ssl.h                    |  31 ---
 src/openvpn/ssl_common.h             |   2 +-
 src/openvpn/ssl_ncp.c                | 274 +++++++++++++++++++++++++++
 src/openvpn/ssl_ncp.h                | 116 ++++++++++++
 tests/unit_tests/openvpn/Makefile.am |  18 +-
 tests/unit_tests/openvpn/test_ncp.c  | 207 ++++++++++++++++++++
 20 files changed, 752 insertions(+), 127 deletions(-)
 create mode 100644 src/compat/compat-strsep.c
 create mode 100644 src/openvpn/ssl_ncp.c
 create mode 100644 src/openvpn/ssl_ncp.h
 create mode 100644 tests/unit_tests/openvpn/test_ncp.c

-- 
2.25.0



^ permalink raw reply	[flat|nested] 22+ messages in thread

* [Openvpn-devel] [PATCH v4 1/5] Only announce IV_NCP=2 when we are willing to support these ciphers
  2020-02-17 14:43 [Openvpn-devel] [PATCH v4 0/5] NCP v2 patch set Arne Schwabe
@ 2020-02-17 14:43 ` Arne Schwabe
  2020-02-17 14:50   ` Lev Stipakov
  2020-02-17 18:33   ` [Openvpn-devel] [PATCH applied] " Gert Doering
  2020-02-17 14:43 ` [Openvpn-devel] [PATCH v4 2/5] Add strsep compat function Arne Schwabe
                   ` (5 subsequent siblings)
  6 siblings, 2 replies; 22+ messages in thread
From: Arne Schwabe @ 2020-02-17 14:43 UTC (permalink / raw)
  To: openvpn-devel

We currently always announce IV_NCP=2 when we support these ciphers even
when we do not accept them. This lead to a server pushing a AES-GCM-128
cipher to clients and the client then rejecting it.

Patch V2:  Remove unecessary restoring of ncp_ciphers
Patch V3:  Do not add ncp_ciphers in context

Signed-off-by: Arne Schwabe <arne@...1227...>
---
 doc/openvpn.8            | 2 ++
 src/openvpn/init.c       | 1 +
 src/openvpn/ssl.c        | 4 +++-
 src/openvpn/ssl_common.h | 1 +
 4 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/doc/openvpn.8 b/doc/openvpn.8
index 8feb3b9c..b8f2f042 100644
--- a/doc/openvpn.8
+++ b/doc/openvpn.8
@@ -4398,6 +4398,8 @@ 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
+the AES\-256\-GCM and AES\-128\-GCM ciphers.
 .\"*********************************************************
 .TP
 .B \-\-ncp\-disable
diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index 04207b61..5c2f801a 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -2827,6 +2827,7 @@ do_init_crypto_tls(struct context *c, const unsigned int flags)
     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;
     to.handshake_window = options->handshake_window;
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index e708fc93..b8351737 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -2322,7 +2322,9 @@ 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))
+            && (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))
         {
             buf_printf(&out, "IV_NCP=2\n");
         }
diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h
index 8dd08862..fb82f610 100644
--- a/src/openvpn/ssl_common.h
+++ b/src/openvpn/ssl_common.h
@@ -290,6 +290,7 @@ struct tls_options
 
     const char *config_ciphername;
     const char *config_authname;
+    const char *config_ncp_ciphers;
     bool ncp_enabled;
 
     bool tls_crypt_v2;
-- 
2.25.0



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [Openvpn-devel] [PATCH v4 1/5] Only announce IV_NCP=2 when we are willing to support these ciphers
  2020-02-17 14:43 ` [Openvpn-devel] [PATCH v4 1/5] Only announce IV_NCP=2 when we are willing to support these ciphers Arne Schwabe
@ 2020-02-17 14:50   ` Lev Stipakov
  2020-02-17 18:33   ` [Openvpn-devel] [PATCH applied] " Gert Doering
  1 sibling, 0 replies; 22+ messages in thread
From: Lev Stipakov @ 2020-02-17 14:50 UTC (permalink / raw)
  To: Arne Schwabe <arne@; +Cc: openvpn-devel

Previous version has been acked, I checked that there are no changes, so

Acked-by: Lev Stipakov <lstipakov@...277...>


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [Openvpn-devel] [PATCH applied] Re: Only announce IV_NCP=2 when we are willing to support these ciphers
  2020-02-17 14:43 ` [Openvpn-devel] [PATCH v4 1/5] Only announce IV_NCP=2 when we are willing to support these ciphers Arne Schwabe
  2020-02-17 14:50   ` Lev Stipakov
@ 2020-02-17 18:33   ` Gert Doering
  1 sibling, 0 replies; 22+ messages in thread
From: Gert Doering @ 2020-02-17 18:33 UTC (permalink / raw)
  To: Arne Schwabe <arne@; +Cc: openvpn-devel

Your patch has been applied to the master branch.

(Have stared at the patch a bit and gave it a t_client test run, did
not explicitly test the case without the required ciphers)

commit 868b200c3aef6ee5acfdf679770832018ebc7b70
Author: Arne Schwabe
Date:   Mon Feb 17 15:43:35 2020 +0100

     Only announce IV_NCP=2 when we are willing to support these ciphers

     Signed-off-by: Arne Schwabe <arne@...1227...>
     Acked-by: Lev Stipakov <lstipakov@...277...>
     Message-Id: <20200217144339.3273-2-arne@...1227...>
     URL: https://www.mail-archive.com/search?l=mid&q=20200217144339.3273-2-arne@...1227...
     Signed-off-by: Gert Doering <gert@...1296...>


--
kind regards,

Gert Doering



^ permalink raw reply	[flat|nested] 22+ messages in thread

* [Openvpn-devel] [PATCH v4 2/5] Add strsep compat function
  2020-02-17 14:43 [Openvpn-devel] [PATCH v4 0/5] NCP v2 patch set Arne Schwabe
  2020-02-17 14:43 ` [Openvpn-devel] [PATCH v4 1/5] Only announce IV_NCP=2 when we are willing to support these ciphers Arne Schwabe
@ 2020-02-17 14:43 ` Arne Schwabe
  2020-02-17 14:52   ` Lev Stipakov
  2020-02-17 18:39   ` [Openvpn-devel] [PATCH applied] " Gert Doering
  2020-02-17 14:43 ` [Openvpn-devel] [PATCH v4 3/5] Implement dynamic NCP negotiation Arne Schwabe
                   ` (4 subsequent siblings)
  6 siblings, 2 replies; 22+ messages in thread
From: Arne Schwabe @ 2020-02-17 14:43 UTC (permalink / raw)
  To: openvpn-devel

Some operating system do not have the strsep function. Since this API
is more "modern" (4.4BSD) than strtok, add it as compat function.

Signed-off-by: Arne Schwabe <arne@...1227...>
---
 configure.ac               |  2 +-
 src/compat/Makefile.am     |  1 +
 src/compat/compat-strsep.c | 61 ++++++++++++++++++++++++++++++++++++++
 src/compat/compat.h        |  4 +++
 src/compat/compat.vcxproj  |  3 +-
 5 files changed, 69 insertions(+), 2 deletions(-)
 create mode 100644 src/compat/compat-strsep.c

diff --git a/configure.ac b/configure.ac
index 3c057295..a47ef3e7 100644
--- a/configure.ac
+++ b/configure.ac
@@ -655,7 +655,7 @@ AC_CHECK_FUNCS([ \
 	ctime memset vsnprintf strdup \
 	setsid chdir putenv getpeername unlink \
 	chsize ftruncate execve getpeereid umask basename dirname access \
-	epoll_create \
+	epoll_create strsep \
 ])
 
 AC_CHECK_LIB(
diff --git a/src/compat/Makefile.am b/src/compat/Makefile.am
index b51f661e..2e94e943 100644
--- a/src/compat/Makefile.am
+++ b/src/compat/Makefile.am
@@ -30,4 +30,5 @@ libcompat_la_SOURCES = \
 	compat-inet_ntop.c \
 	compat-inet_pton.c \
 	compat-lz4.c compat-lz4.h \
+	compat-strsep.c \
 	compat-versionhelpers.h
diff --git a/src/compat/compat-strsep.c b/src/compat/compat-strsep.c
new file mode 100644
index 00000000..42ff6414
--- /dev/null
+++ b/src/compat/compat-strsep.c
@@ -0,0 +1,61 @@
+/*
+ *  OpenVPN -- An application to securely tunnel IP networks
+ *             over a single UDP port, with support for SSL/TLS-based
+ *             session authentication and key exchange,
+ *             packet encryption, packet authentication, and
+ *             packet compression.
+ *
+ *  Copyright (C) 2019 Arne Schwabe <arne@...1227...>
+ *  Copyright (C) 1992-2019 Free Software Foundation, Inc.
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2
+ *  as published by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License along
+ *  with this program; if not, write to the Free Software Foundation, Inc.,
+ *  51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+
+#ifdef HAVE_CONFIG_H
+#include "config.h"
+#elif defined(_MSC_VER)
+#include "config-msvc.h"
+#endif
+
+#ifndef HAVE_STRSEP
+#include <string.h>
+
+/*
+ * Modified version based on the glibc
+ */
+char *
+strsep(char **stringp, const char *delim)
+{
+    char *begin, *end;
+    begin = *stringp;
+    if (begin == NULL)
+    {
+        return NULL;
+    }
+    /* Find the end of the token.  */
+    end = begin + strcspn(begin, delim);
+    if (*end)
+    {
+        /* Terminate the token and set *STRINGP past NUL character.  */
+        *end++ = '\0';
+        *stringp = end;
+    }
+    else
+    {
+        /* No more delimiters; this is the last token.  */
+        *stringp = NULL;
+    }
+    return begin;
+}
+#endif
diff --git a/src/compat/compat.h b/src/compat/compat.h
index d5228989..592881df 100644
--- a/src/compat/compat.h
+++ b/src/compat/compat.h
@@ -70,4 +70,8 @@ int inet_pton(int af, const char *src, void *dst);
 
 #endif
 
+#ifndef HAVE_STRSEP
+char* strsep(char **stringp, const char *delim);
+#endif
+
 #endif /* COMPAT_H */
diff --git a/src/compat/compat.vcxproj b/src/compat/compat.vcxproj
index e388008a..0c4c7b0f 100644
--- a/src/compat/compat.vcxproj
+++ b/src/compat/compat.vcxproj
@@ -102,6 +102,7 @@
     <ClCompile Include="compat-inet_pton.c" />
     <ClCompile Include="compat-daemon.c" />
     <ClCompile Include="compat-lz4.c" />
+    <ClCompile Include="compat-strsep.c" />
   </ItemGroup>
   <ItemGroup>
     <ClInclude Include="compat.h" />
@@ -115,4 +116,4 @@
   <Import Project="$(VCTargetsPath)\Microsoft.Cpp.targets" />
   <ImportGroup Label="ExtensionTargets">
   </ImportGroup>
-</Project>
\ No newline at end of file
+</Project>
-- 
2.25.0



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [Openvpn-devel] [PATCH v4 2/5] Add strsep compat function
  2020-02-17 14:43 ` [Openvpn-devel] [PATCH v4 2/5] Add strsep compat function Arne Schwabe
@ 2020-02-17 14:52   ` Lev Stipakov
  2020-02-17 18:39   ` [Openvpn-devel] [PATCH applied] " Gert Doering
  1 sibling, 0 replies; 22+ messages in thread
From: Lev Stipakov @ 2020-02-17 14:52 UTC (permalink / raw)
  To: Arne Schwabe <arne@; +Cc: openvpn-devel

Builds with MSVC.

Acked-by: Lev Stipakov <lstipakov@...277...>

ma 17. helmik. 2020 klo 16.44 Arne Schwabe (arne@...1227...) kirjoitti:
>
> Some operating system do not have the strsep function. Since this API
> is more "modern" (4.4BSD) than strtok, add it as compat function.
>
> Signed-off-by: Arne Schwabe <arne@...1227...>
> ---
>  configure.ac               |  2 +-
>  src/compat/Makefile.am     |  1 +
>  src/compat/compat-strsep.c | 61 ++++++++++++++++++++++++++++++++++++++
>  src/compat/compat.h        |  4 +++
>  src/compat/compat.vcxproj  |  3 +-
>  5 files changed, 69 insertions(+), 2 deletions(-)
>  create mode 100644 src/compat/compat-strsep.c
>
> diff --git a/configure.ac b/configure.ac
> index 3c057295..a47ef3e7 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -655,7 +655,7 @@ AC_CHECK_FUNCS([ \
>         ctime memset vsnprintf strdup \
>         setsid chdir putenv getpeername unlink \
>         chsize ftruncate execve getpeereid umask basename dirname access \
> -       epoll_create \
> +       epoll_create strsep \
>  ])
>
>  AC_CHECK_LIB(
> diff --git a/src/compat/Makefile.am b/src/compat/Makefile.am
> index b51f661e..2e94e943 100644
> --- a/src/compat/Makefile.am
> +++ b/src/compat/Makefile.am
> @@ -30,4 +30,5 @@ libcompat_la_SOURCES = \
>         compat-inet_ntop.c \
>         compat-inet_pton.c \
>         compat-lz4.c compat-lz4.h \
> +       compat-strsep.c \
>         compat-versionhelpers.h
> diff --git a/src/compat/compat-strsep.c b/src/compat/compat-strsep.c
> new file mode 100644
> index 00000000..42ff6414
> --- /dev/null
> +++ b/src/compat/compat-strsep.c
> @@ -0,0 +1,61 @@
> +/*
> + *  OpenVPN -- An application to securely tunnel IP networks
> + *             over a single UDP port, with support for SSL/TLS-based
> + *             session authentication and key exchange,
> + *             packet encryption, packet authentication, and
> + *             packet compression.
> + *
> + *  Copyright (C) 2019 Arne Schwabe <arne@...1227...>
> + *  Copyright (C) 1992-2019 Free Software Foundation, Inc.
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License version 2
> + *  as published by the Free Software Foundation.
> + *
> + *  This program is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License along
> + *  with this program; if not, write to the Free Software Foundation, Inc.,
> + *  51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> + */
> +
> +#ifdef HAVE_CONFIG_H
> +#include "config.h"
> +#elif defined(_MSC_VER)
> +#include "config-msvc.h"
> +#endif
> +
> +#ifndef HAVE_STRSEP
> +#include <string.h>
> +
> +/*
> + * Modified version based on the glibc
> + */
> +char *
> +strsep(char **stringp, const char *delim)
> +{
> +    char *begin, *end;
> +    begin = *stringp;
> +    if (begin == NULL)
> +    {
> +        return NULL;
> +    }
> +    /* Find the end of the token.  */
> +    end = begin + strcspn(begin, delim);
> +    if (*end)
> +    {
> +        /* Terminate the token and set *STRINGP past NUL character.  */
> +        *end++ = '\0';
> +        *stringp = end;
> +    }
> +    else
> +    {
> +        /* No more delimiters; this is the last token.  */
> +        *stringp = NULL;
> +    }
> +    return begin;
> +}
> +#endif
> diff --git a/src/compat/compat.h b/src/compat/compat.h
> index d5228989..592881df 100644
> --- a/src/compat/compat.h
> +++ b/src/compat/compat.h
> @@ -70,4 +70,8 @@ int inet_pton(int af, const char *src, void *dst);
>
>  #endif
>
> +#ifndef HAVE_STRSEP
> +char* strsep(char **stringp, const char *delim);
> +#endif
> +
>  #endif /* COMPAT_H */
> diff --git a/src/compat/compat.vcxproj b/src/compat/compat.vcxproj
> index e388008a..0c4c7b0f 100644
> --- a/src/compat/compat.vcxproj
> +++ b/src/compat/compat.vcxproj
> @@ -102,6 +102,7 @@
>      <ClCompile Include="compat-inet_pton.c" />
>      <ClCompile Include="compat-daemon.c" />
>      <ClCompile Include="compat-lz4.c" />
> +    <ClCompile Include="compat-strsep.c" />
>    </ItemGroup>
>    <ItemGroup>
>      <ClInclude Include="compat.h" />
> @@ -115,4 +116,4 @@
>    <Import Project="$(VCTargetsPath)\Microsoft.Cpp.targets" />
>    <ImportGroup Label="ExtensionTargets">
>    </ImportGroup>
> -</Project>
> \ No newline at end of file
> +</Project>
> --
> 2.25.0
>
>
>
> _______________________________________________
> Openvpn-devel mailing list
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel



-- 
-Lev


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [Openvpn-devel] [PATCH applied] Re: Add strsep compat function
  2020-02-17 14:43 ` [Openvpn-devel] [PATCH v4 2/5] Add strsep compat function Arne Schwabe
  2020-02-17 14:52   ` Lev Stipakov
@ 2020-02-17 18:39   ` Gert Doering
  1 sibling, 0 replies; 22+ messages in thread
From: Gert Doering @ 2020-02-17 18:39 UTC (permalink / raw)
  To: Arne Schwabe <arne@; +Cc: openvpn-devel

Your patch has been applied to the master branch.

Not much to test here, yet, as we're not calling strsep() anywhere yet
(and I do not have a system without strsep() at hand, all my Unix boxes
have it, even AIX).

commit 0a88ef8c2a6b573f9f21b122bd4818265d39d710
Author: Arne Schwabe
Date:   Mon Feb 17 15:43:36 2020 +0100

     Add strsep compat function

     Signed-off-by: Arne Schwabe <arne@...1227...>
     Acked-by: Lev Stipakov <lstipakov@...277...>
     Message-Id: <20200217144339.3273-3-arne@...1227...>
     URL: https://www.mail-archive.com/search?l=mid&q=20200217144339.3273-3-arne@...1227...
     Signed-off-by: Gert Doering <gert@...1296...>


--
kind regards,

Gert Doering



^ permalink raw reply	[flat|nested] 22+ messages in thread

* [Openvpn-devel] [PATCH v4 3/5] Implement dynamic NCP negotiation
  2020-02-17 14:43 [Openvpn-devel] [PATCH v4 0/5] NCP v2 patch set Arne Schwabe
  2020-02-17 14:43 ` [Openvpn-devel] [PATCH v4 1/5] Only announce IV_NCP=2 when we are willing to support these ciphers Arne Schwabe
  2020-02-17 14:43 ` [Openvpn-devel] [PATCH v4 2/5] Add strsep compat function Arne Schwabe
@ 2020-02-17 14:43 ` Arne Schwabe
  2020-02-17 15:03   ` Lev Stipakov
  2020-02-17 19:25   ` [Openvpn-devel] [PATCH applied] " Gert Doering
  2020-02-17 14:43 ` [Openvpn-devel] [PATCH v4 4/5] Move NCP related function into a seperate file and add unit tests Arne Schwabe
                   ` (3 subsequent siblings)
  6 siblings, 2 replies; 22+ messages in thread
From: Arne Schwabe @ 2020-02-17 14:43 UTC (permalink / raw)
  To: openvpn-devel

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@...1227...>
---
 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(-)

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;
 
-- 
2.25.0



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [Openvpn-devel] [PATCH v4 3/5] Implement dynamic NCP negotiation
  2020-02-17 14:43 ` [Openvpn-devel] [PATCH v4 3/5] Implement dynamic NCP negotiation Arne Schwabe
@ 2020-02-17 15:03   ` Lev Stipakov
  2020-02-17 19:25   ` [Openvpn-devel] [PATCH applied] " Gert Doering
  1 sibling, 0 replies; 22+ messages in thread
From: Lev Stipakov @ 2020-02-17 15:03 UTC (permalink / raw)
  To: Arne Schwabe <arne@; +Cc: openvpn-devel

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@...277...>


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [Openvpn-devel] [PATCH applied] Re: Implement dynamic NCP negotiation
  2020-02-17 14:43 ` [Openvpn-devel] [PATCH v4 3/5] Implement dynamic NCP negotiation Arne Schwabe
  2020-02-17 15:03   ` Lev Stipakov
@ 2020-02-17 19:25   ` Gert Doering
  1 sibling, 0 replies; 22+ messages in thread
From: Gert Doering @ 2020-02-17 19:25 UTC (permalink / raw)
  To: Arne Schwabe <arne@; +Cc: openvpn-devel

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@...1227...>
     Acked-by: Lev Stipakov <lstipakov@...277...>
     Message-Id: <20200217144339.3273-4-arne@...1227...>
     URL: https://www.mail-archive.com/search?l=mid&q=20200217144339.3273-4-arne@...1227...
     Signed-off-by: Gert Doering <gert@...1296...>

--
kind regards,

Gert Doering

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [Openvpn-devel] [PATCH v4 4/5] Move NCP related function into a seperate file and add unit tests
  2020-02-17 14:43 [Openvpn-devel] [PATCH v4 0/5] NCP v2 patch set Arne Schwabe
                   ` (2 preceding siblings ...)
  2020-02-17 14:43 ` [Openvpn-devel] [PATCH v4 3/5] Implement dynamic NCP negotiation Arne Schwabe
@ 2020-02-17 14:43 ` Arne Schwabe
  2020-02-17 15:24   ` Lev Stipakov
  2020-02-20 17:31   ` David Sommerseth
  2020-02-17 14:43 ` [Openvpn-devel] [PATCH v4 5/5] Normalise ncp-ciphers option and restrict it to 127 bytes Arne Schwabe
                   ` (2 subsequent siblings)
  6 siblings, 2 replies; 22+ messages in thread
From: Arne Schwabe @ 2020-02-17 14:43 UTC (permalink / raw)
  To: openvpn-devel

This allows unit test the NCP functions. The ssl.c file has too
many dependencies to make unit testing of it viable.

Patch V2: Removing the include "ssl_ncp.h" from options.c for V2 of
          implement dynamic NCP forces a new version of this patch to
          add the #include in this patch. Merge VS studio file changes
          for ssl_ncp.[ch] into this patch

Patch V3: Regenerate for changes in earlier patches, apply Lev's changes
          to Visual Studio project file

Patch V4: Regenerate to also have the changes of earlier patches.

Signed-off-by: Arne Schwabe <arne@...1227...>
---
 src/openvpn/Makefile.am              |   1 +
 src/openvpn/init.c                   |   1 +
 src/openvpn/multi.c                  |   1 +
 src/openvpn/openvpn.vcxproj          |   2 +
 src/openvpn/openvpn.vcxproj.filters  |   8 +-
 src/openvpn/options.c                |   1 +
 src/openvpn/push.c                   |   1 +
 src/openvpn/ssl.c                    | 176 +-------------------
 src/openvpn/ssl.h                    |  65 --------
 src/openvpn/ssl_ncp.c                | 231 +++++++++++++++++++++++++++
 src/openvpn/ssl_ncp.h                | 101 ++++++++++++
 tests/unit_tests/openvpn/Makefile.am |  18 ++-
 tests/unit_tests/openvpn/test_ncp.c  | 179 +++++++++++++++++++++
 13 files changed, 544 insertions(+), 241 deletions(-)
 create mode 100644 src/openvpn/ssl_ncp.c
 create mode 100644 src/openvpn/ssl_ncp.h
 create mode 100644 tests/unit_tests/openvpn/test_ncp.c

diff --git a/src/openvpn/Makefile.am b/src/openvpn/Makefile.am
index d1bb99c2..2ea47cda 100644
--- a/src/openvpn/Makefile.am
+++ b/src/openvpn/Makefile.am
@@ -116,6 +116,7 @@ openvpn_SOURCES = \
 	ssl.c ssl.h  ssl_backend.h \
 	ssl_openssl.c ssl_openssl.h \
 	ssl_mbedtls.c ssl_mbedtls.h \
+	ssl_ncp.c ssl_ncp.h \
 	ssl_common.h \
 	ssl_verify.c ssl_verify.h ssl_verify_backend.h \
 	ssl_verify_openssl.c ssl_verify_openssl.h \
diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index 9363769f..d83d4366 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -49,6 +49,7 @@
 #include "ping.h"
 #include "mstats.h"
 #include "ssl_verify.h"
+#include "ssl_ncp.h"
 #include "tls_crypt.h"
 #include "forward.h"
 #include "auth_token.h"
diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index d594dd25..b82c68c4 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -45,6 +45,7 @@
 #include "gremlin.h"
 #include "mstats.h"
 #include "ssl_verify.h"
+#include "ssl_ncp.h"
 #include "vlan.h"
 #include <inttypes.h>
 
diff --git a/src/openvpn/openvpn.vcxproj b/src/openvpn/openvpn.vcxproj
index 614d720a..53ac5482 100644
--- a/src/openvpn/openvpn.vcxproj
+++ b/src/openvpn/openvpn.vcxproj
@@ -192,6 +192,7 @@
     <ClCompile Include="socks.c" />
     <ClCompile Include="ssl.c" />
     <ClCompile Include="ssl_openssl.c" />
+    <ClCompile Include="ssl_ncp.c" />
     <ClCompile Include="ssl_verify.c" />
     <ClCompile Include="ssl_verify_openssl.c" />
     <ClCompile Include="status.c" />
@@ -278,6 +279,7 @@
     <ClInclude Include="ssl.h" />
     <ClInclude Include="ssl_backend.h" />
     <ClInclude Include="ssl_common.h" />
+    <ClInclude Include="ssl_ncp.h" />
     <ClInclude Include="ssl_openssl.h" />
     <ClInclude Include="ssl_verify.h" />
     <ClInclude Include="ssl_verify_backend.h" />
diff --git a/src/openvpn/openvpn.vcxproj.filters b/src/openvpn/openvpn.vcxproj.filters
index 41e62d14..80eb52b3 100644
--- a/src/openvpn/openvpn.vcxproj.filters
+++ b/src/openvpn/openvpn.vcxproj.filters
@@ -243,6 +243,9 @@
     <ClCompile Include="ring_buffer.c">
       <Filter>Source Files</Filter>
     </ClCompile>
+    <ClCompile Include="ssl_ncp.c">
+      <Filter>Source Files</Filter>
+    </ClCompile>
   </ItemGroup>
   <ItemGroup>
     <ClInclude Include="base64.h">
@@ -506,10 +509,13 @@
     <ClInclude Include="ring_buffer.h">
       <Filter>Header Files</Filter>
     </ClInclude>
+    <ClInclude Include="ssl_ncp.h">
+      <Filter>Header Files</Filter>
+    </ClInclude>
   </ItemGroup>
   <ItemGroup>
     <ResourceCompile Include="openvpn_win32_resources.rc">
       <Filter>Resource Files</Filter>
     </ResourceCompile>
   </ItemGroup>
-</Project>
+</Project>
\ No newline at end of file
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index c459b260..d057975c 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -45,6 +45,7 @@
 #include "shaper.h"
 #include "crypto.h"
 #include "ssl.h"
+#include "ssl_ncp.h"
 #include "options.h"
 #include "misc.h"
 #include "socket.h"
diff --git a/src/openvpn/push.c b/src/openvpn/push.c
index 8b634051..71f22e94 100644
--- a/src/openvpn/push.c
+++ b/src/openvpn/push.c
@@ -33,6 +33,7 @@
 #include "options.h"
 #include "ssl.h"
 #include "ssl_verify.h"
+#include "ssl_ncp.h"
 #include "manage.h"
 
 #include "memdbg.h"
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 51b03c3b..e21279f1 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -59,6 +59,7 @@
 #include "ssl.h"
 #include "ssl_verify.h"
 #include "ssl_backend.h"
+#include "ssl_ncp.h"
 #include "auth_token.h"
 
 #include "memdbg.h"
@@ -67,6 +68,7 @@
 static const char ssl_default_options_string[] = "V0 UNDEF";
 #endif
 
+
 static inline const char *
 local_options_string(const struct tls_session *session)
 {
@@ -1914,119 +1916,6 @@ key_ctx_update_implicit_iv(struct key_ctx *ctx, uint8_t *key, size_t key_len)
     }
 }
 
-bool
-tls_item_in_cipher_list(const char *item, const char *list)
-{
-    char *tmp_ciphers = string_alloc(list, NULL);
-    char *tmp_ciphers_orig = tmp_ciphers;
-
-    const char *token = strtok(tmp_ciphers, ":");
-    while (token)
-    {
-        if (0 == strcmp(token, item))
-        {
-            break;
-        }
-        token = strtok(NULL, ":");
-    }
-    free(tmp_ciphers_orig);
-
-    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)
-{
-    if (o->ncp_enabled && remote_ciphername
-        && 0 != strcmp(o->ciphername, remote_ciphername))
-    {
-        if (tls_item_in_cipher_list(remote_ciphername, o->ncp_ciphers))
-        {
-            o->ciphername = string_alloc(remote_ciphername, &o->gc);
-            msg(D_TLS_DEBUG_LOW, "Using peer cipher '%s'", o->ciphername);
-        }
-    }
-}
-
 /**
  * Generate data channel keys for the supplied TLS session.
  *
@@ -2644,28 +2533,6 @@ 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)
 {
@@ -4255,45 +4122,6 @@ tls_update_remote_addr(struct tls_multi *multi, const struct link_socket_actual
     gc_free(&gc);
 }
 
-int
-tls_peer_info_ncp_ver(const char *peer_info)
-{
-    const char *ncpstr = peer_info ? strstr(peer_info, "IV_NCP=") : NULL;
-    if (ncpstr)
-    {
-        int ncp = 0;
-        int r = sscanf(ncpstr, "IV_NCP=%d", &ncp);
-        if (r == 1)
-        {
-            return ncp;
-        }
-    }
-    return 0;
-}
-
-bool
-tls_check_ncp_cipher_list(const char *list)
-{
-    bool unsupported_cipher_found = false;
-
-    ASSERT(list);
-
-    char *const tmp_ciphers = string_alloc(list, NULL);
-    const char *token = strtok(tmp_ciphers, ":");
-    while (token)
-    {
-        if (!cipher_kt_get(translate_cipher_name_from_openvpn(token)))
-        {
-            msg(M_WARN, "Unsupported cipher in --ncp-ciphers: %s", token);
-            unsupported_cipher_found = true;
-        }
-        token = strtok(NULL, ":");
-    }
-    free(tmp_ciphers);
-
-    return 0 < strlen(list) && !unsupported_cipher_found;
-}
-
 void
 show_available_tls_ciphers(const char *cipher_list,
                            const char *cipher_list_tls13,
diff --git a/src/openvpn/ssl.h b/src/openvpn/ssl.h
index 2a8871ed..3449d91e 100644
--- a/src/openvpn/ssl.h
+++ b/src/openvpn/ssl.h
@@ -489,15 +489,6 @@ bool tls_session_update_crypto_params(struct tls_session *session,
                                       struct frame *frame,
                                       struct frame *frame_fragment);
 
-/**
- * "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.
- */
-void tls_poor_mans_ncp(struct options *o, const char *remote_ciphername);
-
 #ifdef MANAGEMENT_DEF_AUTH
 static inline char *
 tls_get_peer_info(const struct tls_multi *multi)
@@ -506,62 +497,6 @@ tls_get_peer_info(const struct tls_multi *multi)
 }
 #endif
 
-/**
- * Return the Negotiable Crypto Parameters version advertised in the peer info
- * string, or 0 if none specified.
- */
-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.
- *
- * @param list          Colon-separated list of ciphers
- *
- * @returns true iff all ciphers in list are supported.
- */
-bool tls_check_ncp_cipher_list(const char *list);
-
-/**
- * Return true iff item is present in the colon-separated zero-terminated
- * cipher list.
- */
-bool tls_item_in_cipher_list(const char *item, const char *list);
-
-
 /*
  * inline functions
  */
diff --git a/src/openvpn/ssl_ncp.c b/src/openvpn/ssl_ncp.c
new file mode 100644
index 00000000..919b81e8
--- /dev/null
+++ b/src/openvpn/ssl_ncp.c
@@ -0,0 +1,231 @@
+/*
+ *  OpenVPN -- An application to securely tunnel IP networks
+ *             over a single TCP/UDP port, with support for SSL/TLS-based
+ *             session authentication and key exchange,
+ *             packet encryption, packet authentication, and
+ *             packet compression.
+ *
+ *  Copyright (C) 2002-2018 OpenVPN Inc <sales@...515...>
+ *  Copyright (C) 2010-2018 Fox Crypto B.V. <openvpn@...1435...>
+ *  Copyright (C) 2008-2013 David Sommerseth <dazo@...384...>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2
+ *  as published by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License along
+ *  with this program; if not, write to the Free Software Foundation, Inc.,
+ *  51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+
+/**
+ * @file Control Channel SSL/Data dynamic negotion Module
+ * This file is split from ssl.c to be able to unit test it.
+ */
+
+/*
+ * The routines in this file deal with dynamically negotiating
+ * the data channel HMAC and cipher keys through a TLS session.
+ *
+ * Both the TLS session and the data channel are multiplexed
+ * over the same TCP/UDP port.
+ */
+#ifdef HAVE_CONFIG_H
+#include "config.h"
+#elif defined(_MSC_VER)
+#include "config-msvc.h"
+#endif
+
+#include "syshead.h"
+#include "win32.h"
+
+#include "error.h"
+#include "common.h"
+
+#include "ssl_ncp.h"
+
+/**
+ * Return the Negotiable Crypto Parameters version advertised in the peer info
+ * string, or 0 if none specified.
+ */
+static int
+tls_peer_info_ncp_ver(const char *peer_info)
+{
+    const char *ncpstr = peer_info ? strstr(peer_info, "IV_NCP=") : NULL;
+    if (ncpstr)
+    {
+        int ncp = 0;
+        int r = sscanf(ncpstr, "IV_NCP=%d", &ncp);
+        if (r == 1)
+        {
+            return ncp;
+        }
+    }
+    return 0;
+}
+
+/**
+ * Returns whether the client supports NCP either by
+ * announcing IV_NCP>=2 or the IV_CIPHERS list
+ */
+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;
+    }
+}
+
+bool
+tls_check_ncp_cipher_list(const char *list)
+{
+    bool unsupported_cipher_found = false;
+
+    ASSERT(list);
+
+    char *const tmp_ciphers = string_alloc(list, NULL);
+    const char *token = strtok(tmp_ciphers, ":");
+    while (token)
+    {
+        if (!cipher_kt_get(translate_cipher_name_from_openvpn(token)))
+        {
+            msg(M_WARN, "Unsupported cipher in --ncp-ciphers: %s", token);
+            unsupported_cipher_found = true;
+        }
+        token = strtok(NULL, ":");
+    }
+    free(tmp_ciphers);
+
+    return 0 < strlen(list) && !unsupported_cipher_found;
+}
+
+bool
+tls_item_in_cipher_list(const char *item, const char *list)
+{
+    char *tmp_ciphers = string_alloc(list, NULL);
+    char *tmp_ciphers_orig = tmp_ciphers;
+
+    const char *token = strtok(tmp_ciphers, ":");
+    while (token)
+    {
+        if (0 == strcmp(token, item))
+        {
+            break;
+        }
+        token = strtok(NULL, ":");
+    }
+    free(tmp_ciphers_orig);
+
+    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)
+{
+    if (o->ncp_enabled && remote_ciphername
+        && 0 != strcmp(o->ciphername, remote_ciphername))
+    {
+        if (tls_item_in_cipher_list(remote_ciphername, o->ncp_ciphers))
+        {
+            o->ciphername = string_alloc(remote_ciphername, &o->gc);
+            msg(D_TLS_DEBUG_LOW, "Using peer cipher '%s'", o->ciphername);
+        }
+    }
+}
+
diff --git a/src/openvpn/ssl_ncp.h b/src/openvpn/ssl_ncp.h
new file mode 100644
index 00000000..1257b2b6
--- /dev/null
+++ b/src/openvpn/ssl_ncp.h
@@ -0,0 +1,101 @@
+/*
+ *  OpenVPN -- An application to securely tunnel IP networks
+ *             over a single TCP/UDP port, with support for SSL/TLS-based
+ *             session authentication and key exchange,
+ *             packet encryption, packet authentication, and
+ *             packet compression.
+ *
+ *  Copyright (C) 2002-2018 OpenVPN Inc <sales@...515...>
+ *  Copyright (C) 2010-2018 Fox Crypto B.V. <openvpn@...1435...>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2
+ *  as published by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License along
+ *  with this program; if not, write to the Free Software Foundation, Inc.,
+ *  51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+
+/**
+ * @file Control Channel SSL/Data dynamic negotion Module
+ * This file is split from ssl.h to be able to unit test it.
+ */
+
+#ifndef OPENVPN_SSL_NCP_H
+#define OPENVPN_SSL_NCP_H
+
+#include "buffer.h"
+#include "options.h"
+
+/**
+ * Returns whether the client supports NCP either by
+ * announcing IV_NCP>=2 or the IV_CIPHERS list
+ */
+bool
+tls_peer_supports_ncp(const char *peer_info);
+
+/**
+ * "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.
+ */
+void tls_poor_mans_ncp(struct options *o, const char *remote_ciphername);
+
+/**
+ * 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.
+ *
+ * @param list          Colon-separated list of ciphers
+ *
+ * @returns true iff all ciphers in list are supported.
+ */
+bool tls_check_ncp_cipher_list(const char *list);
+
+/**
+ * Return true iff item is present in the colon-separated zero-terminated
+ * cipher list.
+ */
+bool tls_item_in_cipher_list(const char *item, const char *list);
+
+#endif /* ifndef OPENVPN_SSL_NCP_H */
diff --git a/tests/unit_tests/openvpn/Makefile.am b/tests/unit_tests/openvpn/Makefile.am
index 60e84639..f0880a6b 100644
--- a/tests/unit_tests/openvpn/Makefile.am
+++ b/tests/unit_tests/openvpn/Makefile.am
@@ -6,7 +6,7 @@ if HAVE_LD_WRAP_SUPPORT
 test_binaries += argv_testdriver buffer_testdriver
 endif
 
-test_binaries += crypto_testdriver packet_id_testdriver auth_token_testdriver
+test_binaries += crypto_testdriver packet_id_testdriver auth_token_testdriver ncp_testdriver
 if HAVE_LD_WRAP_SUPPORT
 test_binaries += tls_crypt_testdriver
 endif
@@ -110,3 +110,19 @@ auth_token_testdriver_SOURCES = test_auth_token.c mock_msg.c \
 	$(openvpn_srcdir)/packet_id.c \
 	$(openvpn_srcdir)/platform.c \
 	$(openvpn_srcdir)/base64.c
+
+
+ncp_testdriver_CFLAGS  = @TEST_CFLAGS@ \
+	-I$(openvpn_includedir) -I$(compat_srcdir) -I$(openvpn_srcdir) \
+	$(OPTIONAL_CRYPTO_CFLAGS)
+ncp_testdriver_LDFLAGS = @TEST_LDFLAGS@ \
+	$(OPTIONAL_CRYPTO_LIBS)
+
+ncp_testdriver_SOURCES = test_ncp.c mock_msg.c \
+	$(openvpn_srcdir)/buffer.c \
+	$(openvpn_srcdir)/crypto.c \
+	$(openvpn_srcdir)/crypto_mbedtls.c \
+	$(openvpn_srcdir)/crypto_openssl.c \
+	$(openvpn_srcdir)/otime.c \
+	$(openvpn_srcdir)/packet_id.c \
+	$(openvpn_srcdir)/platform.c
diff --git a/tests/unit_tests/openvpn/test_ncp.c b/tests/unit_tests/openvpn/test_ncp.c
new file mode 100644
index 00000000..0c8652ae
--- /dev/null
+++ b/tests/unit_tests/openvpn/test_ncp.c
@@ -0,0 +1,179 @@
+/*
+ *  OpenVPN -- An application to securely tunnel IP networks
+ *             over a single UDP port, with support for SSL/TLS-based
+ *             session authentication and key exchange,
+ *             packet encryption, packet authentication, and
+ *             packet compression.
+ *
+ *  Copyright (C) 2019 Arne Schwabe <arne@...1227...>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2
+ *  as published by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License along
+ *  with this program; if not, write to the Free Software Foundation, Inc.,
+ *  51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+
+#ifdef HAVE_CONFIG_H
+#include "config.h"
+#elif defined(_MSC_VER)
+#include "config-msvc.h"
+#endif
+
+#include "syshead.h"
+
+#include <stdio.h>
+#include <unistd.h>
+#include <stdlib.h>
+#include <stdarg.h>
+#include <string.h>
+#include <setjmp.h>
+#include <cmocka.h>
+
+#include "ssl_ncp.c"
+
+/* Defines for use in the tests and the mock parse_line() */
+
+const char *bf_chacha = "BF-CBC:CHACHA20-POLY1305";
+const char *aes_ciphers = "AES-256-GCM:AES-128-GCM";
+
+static void
+test_check_ncp_ciphers_list(void **state)
+{
+    assert_true(tls_check_ncp_cipher_list(aes_ciphers));
+    assert_true(tls_check_ncp_cipher_list(bf_chacha));
+    assert_false(tls_check_ncp_cipher_list("vollbit"));
+    assert_false(tls_check_ncp_cipher_list("AES-256-GCM:vollbit"));
+}
+
+static void
+test_extract_client_ciphers(void **state)
+{
+    struct gc_arena gc = gc_new();
+    const char *client_peer_info;
+    char *peer_list;
+
+    client_peer_info = "foo=bar\nIV_foo=y\nIV_NCP=2";
+    peer_list = tls_peer_ncp_list(client_peer_info, &gc);
+    assert_string_equal(aes_ciphers,peer_list);
+    assert_true(tls_peer_supports_ncp(client_peer_info));
+
+    client_peer_info = "foo=bar\nIV_foo=y\nIV_NCP=2\nIV_CIPHERS=BF-CBC";
+    peer_list = tls_peer_ncp_list(client_peer_info, &gc);
+    assert_string_equal("BF-CBC", peer_list);
+    assert_true(tls_peer_supports_ncp(client_peer_info));
+
+    client_peer_info = "IV_NCP=2\nIV_CIPHERS=BF-CBC:FOO-BAR\nIV_BAR=7";
+    peer_list = tls_peer_ncp_list(client_peer_info, &gc);
+    assert_string_equal("BF-CBC:FOO-BAR", peer_list);
+    assert_true(tls_peer_supports_ncp(client_peer_info));
+
+    client_peer_info = "IV_CIPHERS=BF-CBC:FOO-BAR\nIV_BAR=7";
+    peer_list = tls_peer_ncp_list(client_peer_info, &gc);
+    assert_string_equal("BF-CBC:FOO-BAR", peer_list);
+    assert_true(tls_peer_supports_ncp(client_peer_info));
+
+    client_peer_info = "IV_YOLO=NO\nIV_BAR=7";
+    peer_list = tls_peer_ncp_list(client_peer_info, &gc);
+    assert_string_equal("", peer_list);
+    assert_false(tls_peer_supports_ncp(client_peer_info));
+
+    peer_list = tls_peer_ncp_list(NULL, &gc);
+    assert_string_equal("", peer_list);
+    assert_false(tls_peer_supports_ncp(client_peer_info));
+
+    gc_free(&gc);
+}
+
+static void
+test_poor_man(void **state)
+{
+    struct gc_arena gc = gc_new();
+    char *best_cipher;
+
+    const char *serverlist="CHACHA20_POLY1305:AES-128-GCM";
+
+    best_cipher = ncp_get_best_cipher(serverlist, "BF-CBC",
+                                      "IV_YOLO=NO\nIV_BAR=7",
+                                      "BF-CBC", &gc);
+
+    assert_string_equal(best_cipher, "BF-CBC");
+
+    best_cipher = ncp_get_best_cipher(serverlist, "BF-CBC",
+                                      "IV_NCP=1\nIV_BAR=7",
+                                      "AES-128-GCM", &gc);
+
+    assert_string_equal(best_cipher, "AES-128-GCM");
+
+    best_cipher = ncp_get_best_cipher(serverlist, "BF-CBC",
+                                      NULL,
+                                      "AES-128-GCM", &gc);
+
+    assert_string_equal(best_cipher, "AES-128-GCM");
+
+    gc_free(&gc);
+}
+
+
+static void
+test_ncp_best(void **state)
+{
+    struct gc_arena gc = gc_new();
+    char *best_cipher;
+
+    const char *serverlist="CHACHA20_POLY1305:AES-128-GCM:AES-256-GCM";
+
+    best_cipher = ncp_get_best_cipher(serverlist, "BF-CBC",
+                                      "IV_YOLO=NO\nIV_NCP=2\nIV_BAR=7",
+                                      "BF-CBC", &gc);
+
+    assert_string_equal(best_cipher, "AES-128-GCM");
+
+    /* Best cipher is in --cipher of client */
+    best_cipher = ncp_get_best_cipher(serverlist, "BF-CBC",
+                                      "IV_NCP=2\nIV_BAR=7",
+                                      "CHACHA20_POLY1305", &gc);
+
+    assert_string_equal(best_cipher, "CHACHA20_POLY1305");
+
+    /* Best cipher is in --cipher of client */
+    best_cipher = ncp_get_best_cipher(serverlist, "BF-CBC",
+                                      "IV_CIPHERS=AES-128-GCM",
+                                      "AES-256-CBC", &gc);
+
+
+    assert_string_equal(best_cipher, "AES-128-GCM");
+
+    /* IV_NCP=2 should be ignored if IV_CIPHERS is sent */
+    best_cipher = ncp_get_best_cipher(serverlist, "BF-CBC",
+                                      "IV_FOO=7\nIV_CIPHERS=AES-256-GCM\nIV_NCP=2",
+                                      "AES-256-CBC", &gc);
+
+    assert_string_equal(best_cipher, "AES-256-GCM");
+
+
+    gc_free(&gc);
+}
+
+
+
+
+const struct CMUnitTest ncp_tests[] = {
+    cmocka_unit_test(test_check_ncp_ciphers_list),
+    cmocka_unit_test(test_extract_client_ciphers),
+    cmocka_unit_test(test_poor_man),
+    cmocka_unit_test(test_ncp_best)
+};
+
+
+int main(void)
+{
+    return cmocka_run_group_tests(ncp_tests, NULL, NULL);
+}
-- 
2.25.0



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [Openvpn-devel] [PATCH v4 4/5] Move NCP related function into a seperate file and add unit tests
  2020-02-17 14:43 ` [Openvpn-devel] [PATCH v4 4/5] Move NCP related function into a seperate file and add unit tests Arne Schwabe
@ 2020-02-17 15:24   ` Lev Stipakov
  2020-02-20 17:31   ` David Sommerseth
  1 sibling, 0 replies; 22+ messages in thread
From: Lev Stipakov @ 2020-02-17 15:24 UTC (permalink / raw)
  To: Arne Schwabe <arne@; +Cc: openvpn-devel

Stared at the code, built and tested on MSVC and Ubuntu 18.

Acked-by: Lev Stipakov <lstipakov@...277...>


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Openvpn-devel] [PATCH v4 4/5] Move NCP related function into a seperate file and add unit tests
  2020-02-17 14:43 ` [Openvpn-devel] [PATCH v4 4/5] Move NCP related function into a seperate file and add unit tests Arne Schwabe
  2020-02-17 15:24   ` Lev Stipakov
@ 2020-02-20 17:31   ` David Sommerseth
  1 sibling, 0 replies; 22+ messages in thread
From: David Sommerseth @ 2020-02-20 17:31 UTC (permalink / raw)
  To: Arne Schwabe <arne@

On 17/02/2020 15:43, Arne Schwabe wrote:
> This allows unit test the NCP functions. The ssl.c file has too
> many dependencies to make unit testing of it viable.
> 
> Patch V2: Removing the include "ssl_ncp.h" from options.c for V2 of
>           implement dynamic NCP forces a new version of this patch to
>           add the #include in this patch. Merge VS studio file changes
>           for ssl_ncp.[ch] into this patch
> 
> Patch V3: Regenerate for changes in earlier patches, apply Lev's changes
>           to Visual Studio project file
> 
> Patch V4: Regenerate to also have the changes of earlier patches.
> 
> Signed-off-by: Arne Schwabe <arne@...1227...>
> ---
>  src/openvpn/Makefile.am              |   1 +
>  src/openvpn/init.c                   |   1 +
>  src/openvpn/multi.c                  |   1 +
>  src/openvpn/openvpn.vcxproj          |   2 +
>  src/openvpn/openvpn.vcxproj.filters  |   8 +-
>  src/openvpn/options.c                |   1 +
>  src/openvpn/push.c                   |   1 +
>  src/openvpn/ssl.c                    | 176 +-------------------
>  src/openvpn/ssl.h                    |  65 --------
>  src/openvpn/ssl_ncp.c                | 231 +++++++++++++++++++++++++++
>  src/openvpn/ssl_ncp.h                | 101 ++++++++++++
>  tests/unit_tests/openvpn/Makefile.am |  18 ++-
>  tests/unit_tests/openvpn/test_ncp.c  | 179 +++++++++++++++++++++
>  13 files changed, 544 insertions(+), 241 deletions(-)
>  create mode 100644 src/openvpn/ssl_ncp.c
>  create mode 100644 src/openvpn/ssl_ncp.h
>  create mode 100644 tests/unit_tests/openvpn/test_ncp.c
> 
Sorry, but this gets a NAK from me.

$ ./tests/unit_tests/openvpn/ncp_testdriver
[==========] Running 4 test(s).
[ RUN      ] test_check_ncp_ciphers_list
Unsupported cipher in --ncp-ciphers: AES-256-GCM
Unsupported cipher in --ncp-ciphers: AES-128-GCM
[  ERROR   ] --- tls_check_ncp_cipher_list(aes_ciphers)
[   LINE   ] --- test_ncp.c:50: error: Failure!
[  FAILED  ] test_check_ncp_ciphers_list
[ RUN      ] test_extract_client_ciphers
[       OK ] test_extract_client_ciphers
[ RUN      ] test_poor_man
[       OK ] test_poor_man
[ RUN      ] test_ncp_best
[       OK ] test_ncp_best
[==========] 4 test(s) run.
[  PASSED  ] 3 test(s).
[  FAILED  ] 1 test(s), listed below:
[  FAILED  ] test_check_ncp_ciphers_list

 1 FAILED TEST(S)

We can't have any failing tests ;-)

This is tested on RHEL-7.7 (openssl-1.0.2k-19) which I also do know have
AES-GCM support.


-- 
kind regards,

David Sommerseth
OpenVPN Inc



^ permalink raw reply	[flat|nested] 22+ messages in thread

* [Openvpn-devel] [PATCH v4 5/5] Normalise ncp-ciphers option and restrict it to 127 bytes
  2020-02-17 14:43 [Openvpn-devel] [PATCH v4 0/5] NCP v2 patch set Arne Schwabe
                   ` (3 preceding siblings ...)
  2020-02-17 14:43 ` [Openvpn-devel] [PATCH v4 4/5] Move NCP related function into a seperate file and add unit tests Arne Schwabe
@ 2020-02-17 14:43 ` Arne Schwabe
  2020-02-17 15:26   ` Lev Stipakov
  2020-02-21 10:07 ` [Openvpn-devel] [PATCH v5 4/5] Move NCP related function into a seperate file and add unit tests Arne Schwabe
  2020-02-21 10:07 ` [Openvpn-devel] [PATCH v5 5/5] Normalise ncp-ciphers option and restrict it to 127 bytes Arne Schwabe
  6 siblings, 1 reply; 22+ messages in thread
From: Arne Schwabe @ 2020-02-17 14:43 UTC (permalink / raw)
  To: openvpn-devel

In scenarios of mbed TLS vs OpenSSL we already normalise the ciphers
that are send via the wire protocol via OCC to not have a mismatch
warning between server and client. This is done by
translate_cipher_name_from_openvpn. The same applies also to the
ncp-ciphers list. Specifying non normalised names in ncp-ciphers will
cause negotation not to succeed if ciphers are not in the same form.
Therefore we will normalise the ciphers in options_postmutate.

The alternative and a lot less user friendly alternative would be to
bail if on of the ciphers in ncp-ciphers is not in its normalised form.

Also restrict the ncp-ciphers list to 127. This is somewhat arbitrary
but should prevent too large IV_CIPHER messages and problems sending
those. The server will accept also large IV_CIPHER values from clients.

Patch V2: Correct comment about normalising ciphers
Patch V3: Correct #ifdef statement

Signed-off-by: Arne Schwabe <arne@...1227...>
---
 doc/openvpn.8                       |  3 ++
 src/openvpn/options.c               | 14 ++++---
 src/openvpn/ssl_ncp.c               | 57 +++++++++++++++++++++++++----
 src/openvpn/ssl_ncp.h               | 19 +++++++++-
 tests/unit_tests/openvpn/test_ncp.c | 44 ++++++++++++++++++----
 5 files changed, 115 insertions(+), 22 deletions(-)

diff --git a/doc/openvpn.8 b/doc/openvpn.8
index d631ebe7..c0ec80f9 100644
--- a/doc/openvpn.8
+++ b/doc/openvpn.8
@@ -4406,6 +4406,9 @@ AES\-256\-GCM:AES\-256\-CBC" set can either specify "\-\-cipher BF\-CBC" or
 
 Note, for using NCP with a OpenVPN 2.4 peer this list must include
 the AES\-256\-GCM and AES\-128\-GCM ciphers.
+
+This list is restricted to be 127 chars long after conversion to OpenVPN
+ciphers.
 .\"*********************************************************
 .TP
 .B \-\-ncp\-disable
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index d057975c..06291970 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -2511,11 +2511,6 @@ options_postprocess_verify_ce(const struct options *options, const struct connec
     }
 #endif /* P2MP_SERVER */
 
-    if (options->ncp_enabled && !tls_check_ncp_cipher_list(options->ncp_ciphers))
-    {
-        msg(M_USAGE, "NCP cipher list contains unsupported ciphers.");
-    }
-
     if (options->keysize)
     {
         msg(M_WARN, "WARNING: --keysize is DEPRECATED and will be removed in OpenVPN 2.6");
@@ -3093,6 +3088,15 @@ options_postprocess_mutate(struct options *o)
 
     options_postprocess_mutate_invariant(o);
 
+    if (o->ncp_enabled)
+    {
+        o->ncp_ciphers = mutate_ncp_cipher_list(o->ncp_ciphers, &o->gc);
+        if (o->ncp_ciphers == NULL)
+        {
+            msg(M_USAGE, "NCP cipher list contains unsupported ciphers or is too long.");
+        }
+    }
+
     if (o->remote_list && !o->connection_list)
     {
         /*
diff --git a/src/openvpn/ssl_ncp.c b/src/openvpn/ssl_ncp.c
index 919b81e8..95824ad2 100644
--- a/src/openvpn/ssl_ncp.c
+++ b/src/openvpn/ssl_ncp.c
@@ -91,27 +91,70 @@ tls_peer_supports_ncp(const char *peer_info)
     }
 }
 
-bool
-tls_check_ncp_cipher_list(const char *list)
+char *
+mutate_ncp_cipher_list(const char *list, struct gc_arena *gc)
 {
-    bool unsupported_cipher_found = false;
+    bool error_found = false;
 
-    ASSERT(list);
+    struct buffer new_list  = alloc_buf(MAX_NCP_CIPHERS_LENGTH);
 
     char *const tmp_ciphers = string_alloc(list, NULL);
     const char *token = strtok(tmp_ciphers, ":");
     while (token)
     {
-        if (!cipher_kt_get(translate_cipher_name_from_openvpn(token)))
+        /*
+         * Going through a roundtrip by using translate_cipher_name_from_openvpn
+         * and translate_cipher_name_to_openvpn also normalises the cipher name,
+         * e.g. replacing AeS-128-gCm with AES-128-GCM
+         */
+        const char *cipher_name = translate_cipher_name_from_openvpn(token);
+        const cipher_kt_t *ktc = cipher_kt_get(cipher_name);
+        if (!ktc)
         {
             msg(M_WARN, "Unsupported cipher in --ncp-ciphers: %s", token);
-            unsupported_cipher_found = true;
+            error_found = true;
+        }
+        else
+        {
+            const char *ovpn_cipher_name =
+                translate_cipher_name_to_openvpn(cipher_kt_name(ktc));
+
+            if (buf_len(&new_list)> 0)
+            {
+                /* The next if condition ensure there is always space for
+                 * a :
+                 */
+                buf_puts(&new_list, ":");
+            }
+
+            /* Ensure buffer has capacity for cipher name + : + \0 */
+            if (!(buf_forward_capacity(&new_list) >
+                  strlen(ovpn_cipher_name) + 2))
+            {
+                msg(M_WARN, "Length of --ncp-ciphers is over the"
+                    "limit of 127 chars");
+                error_found = true;
+            }
+            else
+            {
+                buf_puts(&new_list, ovpn_cipher_name);
+            }
         }
         token = strtok(NULL, ":");
     }
+
+
+
+    char *ret = NULL;
+    if (!error_found && buf_len(&new_list) > 0)
+    {
+        buf_null_terminate(&new_list);
+        ret = string_alloc(buf_str(&new_list), gc);
+    }
     free(tmp_ciphers);
+    free_buf(&new_list);
 
-    return 0 < strlen(list) && !unsupported_cipher_found;
+    return ret;
 }
 
 bool
diff --git a/src/openvpn/ssl_ncp.h b/src/openvpn/ssl_ncp.h
index 1257b2b6..d00c222d 100644
--- a/src/openvpn/ssl_ncp.h
+++ b/src/openvpn/ssl_ncp.h
@@ -87,10 +87,17 @@ tls_peer_ncp_list(const char *peer_info, struct gc_arena *gc);
  * Check whether the ciphers in the supplied list are supported.
  *
  * @param list          Colon-separated list of ciphers
+ * @parms gc            gc_arena to allocate the returned string
  *
- * @returns true iff all ciphers in list are supported.
+ * @returns             colon separated string of normalised (via
+ *                      translate_cipher_name_from_openvpn) and
+ *                      zero terminated string iff all ciphers
+ *                      in list are supported and the total length
+ *                      is short than MAX_NCP_CIPHERS_LENGTH. NULL
+ *                      otherwise.
  */
-bool tls_check_ncp_cipher_list(const char *list);
+char *
+mutate_ncp_cipher_list(const char *list, struct gc_arena *gc);
 
 /**
  * Return true iff item is present in the colon-separated zero-terminated
@@ -98,4 +105,12 @@ bool tls_check_ncp_cipher_list(const char *list);
  */
 bool tls_item_in_cipher_list(const char *item, const char *list);
 
+/**
+ * The maximum length of a ncp-cipher string that is accepted.
+ *
+ * Since this list needs to be pushed as IV_CIPHERS, we are conservative
+ * about its length.
+ */
+#define MAX_NCP_CIPHERS_LENGTH 127
+
 #endif /* ifndef OPENVPN_SSL_NCP_H */
diff --git a/tests/unit_tests/openvpn/test_ncp.c b/tests/unit_tests/openvpn/test_ncp.c
index 0c8652ae..da210118 100644
--- a/tests/unit_tests/openvpn/test_ncp.c
+++ b/tests/unit_tests/openvpn/test_ncp.c
@@ -47,10 +47,34 @@ const char *aes_ciphers = "AES-256-GCM:AES-128-GCM";
 static void
 test_check_ncp_ciphers_list(void **state)
 {
-    assert_true(tls_check_ncp_cipher_list(aes_ciphers));
-    assert_true(tls_check_ncp_cipher_list(bf_chacha));
-    assert_false(tls_check_ncp_cipher_list("vollbit"));
-    assert_false(tls_check_ncp_cipher_list("AES-256-GCM:vollbit"));
+    struct gc_arena gc = gc_new();
+
+    assert_string_equal(mutate_ncp_cipher_list(aes_ciphers, &gc), aes_ciphers);
+
+    assert_string_equal(mutate_ncp_cipher_list(bf_chacha, &gc), bf_chacha);
+
+    assert_string_equal(mutate_ncp_cipher_list("BF-cbc:ChaCha20-Poly1305", &gc),
+                        bf_chacha);
+
+
+    assert_ptr_equal(mutate_ncp_cipher_list("vollbit", &gc), NULL);
+    assert_ptr_equal(mutate_ncp_cipher_list("AES-256-GCM:vollbit", &gc), NULL);
+    assert_ptr_equal(mutate_ncp_cipher_list("", &gc), NULL);
+
+    assert_ptr_equal(mutate_ncp_cipher_list(
+                         "ChaCha20-Poly1305:ChaCha20-Poly1305:ChaCha20-Poly1305:"
+                         "ChaCha20-Poly1305:ChaCha20-Poly1305:ChaCha20-Poly1305:"
+                         "ChaCha20-Poly1305", &gc), NULL);
+
+#ifdef ENABLE_CRYPTO_OPENSSL
+    assert_string_equal(mutate_ncp_cipher_list("id-aes128-GCM:id-aes256-GCM",
+        &gc), "AES-128-GCM:AES-256-GCM");
+#else
+    assert_string_equal(mutate_ncp_cipher_list("BLOWFISH-CBC",
+                                               &gc), "BF-CBC");
+#endif
+    gc_free(&gc);
+
 }
 
 static void
@@ -98,7 +122,7 @@ test_poor_man(void **state)
     struct gc_arena gc = gc_new();
     char *best_cipher;
 
-    const char *serverlist="CHACHA20_POLY1305:AES-128-GCM";
+    const char *serverlist = "CHACHA20_POLY1305:AES-128-GCM";
 
     best_cipher = ncp_get_best_cipher(serverlist, "BF-CBC",
                                       "IV_YOLO=NO\nIV_BAR=7",
@@ -128,7 +152,7 @@ test_ncp_best(void **state)
     struct gc_arena gc = gc_new();
     char *best_cipher;
 
-    const char *serverlist="CHACHA20_POLY1305:AES-128-GCM:AES-256-GCM";
+    const char *serverlist = "CHACHA20_POLY1305:AES-128-GCM:AES-256-GCM";
 
     best_cipher = ncp_get_best_cipher(serverlist, "BF-CBC",
                                       "IV_YOLO=NO\nIV_NCP=2\nIV_BAR=7",
@@ -164,7 +188,6 @@ test_ncp_best(void **state)
 
 
 
-
 const struct CMUnitTest ncp_tests[] = {
     cmocka_unit_test(test_check_ncp_ciphers_list),
     cmocka_unit_test(test_extract_client_ciphers),
@@ -173,7 +196,12 @@ const struct CMUnitTest ncp_tests[] = {
 };
 
 
-int main(void)
+int
+main(void)
 {
+#if defined(ENABLE_CRYPTO_OPENSSL)
+    OpenSSL_add_all_algorithms();
+#endif
+
     return cmocka_run_group_tests(ncp_tests, NULL, NULL);
 }
-- 
2.25.0



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [Openvpn-devel] [PATCH v4 5/5] Normalise ncp-ciphers option and restrict it to 127 bytes
  2020-02-17 14:43 ` [Openvpn-devel] [PATCH v4 5/5] Normalise ncp-ciphers option and restrict it to 127 bytes Arne Schwabe
@ 2020-02-17 15:26   ` Lev Stipakov
  0 siblings, 0 replies; 22+ messages in thread
From: Lev Stipakov @ 2020-02-17 15:26 UTC (permalink / raw)
  To: Arne Schwabe <arne@; +Cc: openvpn-devel

Stared at the code, built and tested on MSVC and Ubuntu 18.
Cipher "chacha20-POLY1305" correctly translated into "CHACHA20-POLY1305".
Also (updated) unit tests passed.

Acked-by: Lev Stipakov <lstipakov@...277...>


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [Openvpn-devel] [PATCH v5 4/5] Move NCP related function into a seperate file and add unit tests
  2020-02-17 14:43 [Openvpn-devel] [PATCH v4 0/5] NCP v2 patch set Arne Schwabe
                   ` (4 preceding siblings ...)
  2020-02-17 14:43 ` [Openvpn-devel] [PATCH v4 5/5] Normalise ncp-ciphers option and restrict it to 127 bytes Arne Schwabe
@ 2020-02-21 10:07 ` Arne Schwabe
  2020-03-09 13:22   ` [Openvpn-devel] [PATCH applied] " Gert Doering
  2020-02-21 10:07 ` [Openvpn-devel] [PATCH v5 5/5] Normalise ncp-ciphers option and restrict it to 127 bytes Arne Schwabe
  6 siblings, 1 reply; 22+ messages in thread
From: Arne Schwabe @ 2020-02-21 10:07 UTC (permalink / raw)
  To: openvpn-devel

This allows unit test the NCP functions. The ssl.c file has too
many dependencies to make unit testing of it viable.

Patch V2: Removing the include "ssl_ncp.h" from options.c for V2 of
          implement dynamic NCP forces a new version of this patch to
          add the #include in this patch. Merge VS studio file changes
          for ssl_ncp.[ch] into this patch

Patch V3: Regenerate for changes in earlier patches, apply Lev's changes
          to Visual Studio project file

Patch V4: Regenerate to also have the changes of earlier patches.

Patch V5: Fix unit tests for crypto library missing chacha20-poly1305

Signed-off-by: Arne Schwabe <arne@...1227...>
---
 src/openvpn/Makefile.am              |   1 +
 src/openvpn/init.c                   |   1 +
 src/openvpn/multi.c                  |   1 +
 src/openvpn/openvpn.vcxproj          |   2 +
 src/openvpn/openvpn.vcxproj.filters  |   8 +-
 src/openvpn/options.c                |   1 +
 src/openvpn/push.c                   |   1 +
 src/openvpn/ssl.c                    | 176 +-------------------
 src/openvpn/ssl.h                    |  65 --------
 src/openvpn/ssl_ncp.c                | 231 +++++++++++++++++++++++++++
 src/openvpn/ssl_ncp.h                | 101 ++++++++++++
 tests/unit_tests/openvpn/Makefile.am |  18 ++-
 tests/unit_tests/openvpn/test_ncp.c  | 184 +++++++++++++++++++++
 13 files changed, 549 insertions(+), 241 deletions(-)
 create mode 100644 src/openvpn/ssl_ncp.c
 create mode 100644 src/openvpn/ssl_ncp.h
 create mode 100644 tests/unit_tests/openvpn/test_ncp.c

diff --git a/src/openvpn/Makefile.am b/src/openvpn/Makefile.am
index d1bb99c2..2ea47cda 100644
--- a/src/openvpn/Makefile.am
+++ b/src/openvpn/Makefile.am
@@ -116,6 +116,7 @@ openvpn_SOURCES = \
 	ssl.c ssl.h  ssl_backend.h \
 	ssl_openssl.c ssl_openssl.h \
 	ssl_mbedtls.c ssl_mbedtls.h \
+	ssl_ncp.c ssl_ncp.h \
 	ssl_common.h \
 	ssl_verify.c ssl_verify.h ssl_verify_backend.h \
 	ssl_verify_openssl.c ssl_verify_openssl.h \
diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index 1cfffbbe..824b6667 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -49,6 +49,7 @@
 #include "ping.h"
 #include "mstats.h"
 #include "ssl_verify.h"
+#include "ssl_ncp.h"
 #include "tls_crypt.h"
 #include "forward.h"
 #include "auth_token.h"
diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index 279cab23..da0aeeba 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -45,6 +45,7 @@
 #include "gremlin.h"
 #include "mstats.h"
 #include "ssl_verify.h"
+#include "ssl_ncp.h"
 #include "vlan.h"
 #include <inttypes.h>
 
diff --git a/src/openvpn/openvpn.vcxproj b/src/openvpn/openvpn.vcxproj
index 614d720a..53ac5482 100644
--- a/src/openvpn/openvpn.vcxproj
+++ b/src/openvpn/openvpn.vcxproj
@@ -192,6 +192,7 @@
     <ClCompile Include="socks.c" />
     <ClCompile Include="ssl.c" />
     <ClCompile Include="ssl_openssl.c" />
+    <ClCompile Include="ssl_ncp.c" />
     <ClCompile Include="ssl_verify.c" />
     <ClCompile Include="ssl_verify_openssl.c" />
     <ClCompile Include="status.c" />
@@ -278,6 +279,7 @@
     <ClInclude Include="ssl.h" />
     <ClInclude Include="ssl_backend.h" />
     <ClInclude Include="ssl_common.h" />
+    <ClInclude Include="ssl_ncp.h" />
     <ClInclude Include="ssl_openssl.h" />
     <ClInclude Include="ssl_verify.h" />
     <ClInclude Include="ssl_verify_backend.h" />
diff --git a/src/openvpn/openvpn.vcxproj.filters b/src/openvpn/openvpn.vcxproj.filters
index 41e62d14..80eb52b3 100644
--- a/src/openvpn/openvpn.vcxproj.filters
+++ b/src/openvpn/openvpn.vcxproj.filters
@@ -243,6 +243,9 @@
     <ClCompile Include="ring_buffer.c">
       <Filter>Source Files</Filter>
     </ClCompile>
+    <ClCompile Include="ssl_ncp.c">
+      <Filter>Source Files</Filter>
+    </ClCompile>
   </ItemGroup>
   <ItemGroup>
     <ClInclude Include="base64.h">
@@ -506,10 +509,13 @@
     <ClInclude Include="ring_buffer.h">
       <Filter>Header Files</Filter>
     </ClInclude>
+    <ClInclude Include="ssl_ncp.h">
+      <Filter>Header Files</Filter>
+    </ClInclude>
   </ItemGroup>
   <ItemGroup>
     <ResourceCompile Include="openvpn_win32_resources.rc">
       <Filter>Resource Files</Filter>
     </ResourceCompile>
   </ItemGroup>
-</Project>
+</Project>
\ No newline at end of file
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 6c0e52e6..c3229156 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -45,6 +45,7 @@
 #include "shaper.h"
 #include "crypto.h"
 #include "ssl.h"
+#include "ssl_ncp.h"
 #include "options.h"
 #include "misc.h"
 #include "socket.h"
diff --git a/src/openvpn/push.c b/src/openvpn/push.c
index 8b634051..71f22e94 100644
--- a/src/openvpn/push.c
+++ b/src/openvpn/push.c
@@ -33,6 +33,7 @@
 #include "options.h"
 #include "ssl.h"
 #include "ssl_verify.h"
+#include "ssl_ncp.h"
 #include "manage.h"
 
 #include "memdbg.h"
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 51b03c3b..e21279f1 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -59,6 +59,7 @@
 #include "ssl.h"
 #include "ssl_verify.h"
 #include "ssl_backend.h"
+#include "ssl_ncp.h"
 #include "auth_token.h"
 
 #include "memdbg.h"
@@ -67,6 +68,7 @@
 static const char ssl_default_options_string[] = "V0 UNDEF";
 #endif
 
+
 static inline const char *
 local_options_string(const struct tls_session *session)
 {
@@ -1914,119 +1916,6 @@ key_ctx_update_implicit_iv(struct key_ctx *ctx, uint8_t *key, size_t key_len)
     }
 }
 
-bool
-tls_item_in_cipher_list(const char *item, const char *list)
-{
-    char *tmp_ciphers = string_alloc(list, NULL);
-    char *tmp_ciphers_orig = tmp_ciphers;
-
-    const char *token = strtok(tmp_ciphers, ":");
-    while (token)
-    {
-        if (0 == strcmp(token, item))
-        {
-            break;
-        }
-        token = strtok(NULL, ":");
-    }
-    free(tmp_ciphers_orig);
-
-    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)
-{
-    if (o->ncp_enabled && remote_ciphername
-        && 0 != strcmp(o->ciphername, remote_ciphername))
-    {
-        if (tls_item_in_cipher_list(remote_ciphername, o->ncp_ciphers))
-        {
-            o->ciphername = string_alloc(remote_ciphername, &o->gc);
-            msg(D_TLS_DEBUG_LOW, "Using peer cipher '%s'", o->ciphername);
-        }
-    }
-}
-
 /**
  * Generate data channel keys for the supplied TLS session.
  *
@@ -2644,28 +2533,6 @@ 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)
 {
@@ -4255,45 +4122,6 @@ tls_update_remote_addr(struct tls_multi *multi, const struct link_socket_actual
     gc_free(&gc);
 }
 
-int
-tls_peer_info_ncp_ver(const char *peer_info)
-{
-    const char *ncpstr = peer_info ? strstr(peer_info, "IV_NCP=") : NULL;
-    if (ncpstr)
-    {
-        int ncp = 0;
-        int r = sscanf(ncpstr, "IV_NCP=%d", &ncp);
-        if (r == 1)
-        {
-            return ncp;
-        }
-    }
-    return 0;
-}
-
-bool
-tls_check_ncp_cipher_list(const char *list)
-{
-    bool unsupported_cipher_found = false;
-
-    ASSERT(list);
-
-    char *const tmp_ciphers = string_alloc(list, NULL);
-    const char *token = strtok(tmp_ciphers, ":");
-    while (token)
-    {
-        if (!cipher_kt_get(translate_cipher_name_from_openvpn(token)))
-        {
-            msg(M_WARN, "Unsupported cipher in --ncp-ciphers: %s", token);
-            unsupported_cipher_found = true;
-        }
-        token = strtok(NULL, ":");
-    }
-    free(tmp_ciphers);
-
-    return 0 < strlen(list) && !unsupported_cipher_found;
-}
-
 void
 show_available_tls_ciphers(const char *cipher_list,
                            const char *cipher_list_tls13,
diff --git a/src/openvpn/ssl.h b/src/openvpn/ssl.h
index 2a8871ed..3449d91e 100644
--- a/src/openvpn/ssl.h
+++ b/src/openvpn/ssl.h
@@ -489,15 +489,6 @@ bool tls_session_update_crypto_params(struct tls_session *session,
                                       struct frame *frame,
                                       struct frame *frame_fragment);
 
-/**
- * "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.
- */
-void tls_poor_mans_ncp(struct options *o, const char *remote_ciphername);
-
 #ifdef MANAGEMENT_DEF_AUTH
 static inline char *
 tls_get_peer_info(const struct tls_multi *multi)
@@ -506,62 +497,6 @@ tls_get_peer_info(const struct tls_multi *multi)
 }
 #endif
 
-/**
- * Return the Negotiable Crypto Parameters version advertised in the peer info
- * string, or 0 if none specified.
- */
-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.
- *
- * @param list          Colon-separated list of ciphers
- *
- * @returns true iff all ciphers in list are supported.
- */
-bool tls_check_ncp_cipher_list(const char *list);
-
-/**
- * Return true iff item is present in the colon-separated zero-terminated
- * cipher list.
- */
-bool tls_item_in_cipher_list(const char *item, const char *list);
-
-
 /*
  * inline functions
  */
diff --git a/src/openvpn/ssl_ncp.c b/src/openvpn/ssl_ncp.c
new file mode 100644
index 00000000..919b81e8
--- /dev/null
+++ b/src/openvpn/ssl_ncp.c
@@ -0,0 +1,231 @@
+/*
+ *  OpenVPN -- An application to securely tunnel IP networks
+ *             over a single TCP/UDP port, with support for SSL/TLS-based
+ *             session authentication and key exchange,
+ *             packet encryption, packet authentication, and
+ *             packet compression.
+ *
+ *  Copyright (C) 2002-2018 OpenVPN Inc <sales@...515...>
+ *  Copyright (C) 2010-2018 Fox Crypto B.V. <openvpn@...1435...>
+ *  Copyright (C) 2008-2013 David Sommerseth <dazo@...384...>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2
+ *  as published by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License along
+ *  with this program; if not, write to the Free Software Foundation, Inc.,
+ *  51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+
+/**
+ * @file Control Channel SSL/Data dynamic negotion Module
+ * This file is split from ssl.c to be able to unit test it.
+ */
+
+/*
+ * The routines in this file deal with dynamically negotiating
+ * the data channel HMAC and cipher keys through a TLS session.
+ *
+ * Both the TLS session and the data channel are multiplexed
+ * over the same TCP/UDP port.
+ */
+#ifdef HAVE_CONFIG_H
+#include "config.h"
+#elif defined(_MSC_VER)
+#include "config-msvc.h"
+#endif
+
+#include "syshead.h"
+#include "win32.h"
+
+#include "error.h"
+#include "common.h"
+
+#include "ssl_ncp.h"
+
+/**
+ * Return the Negotiable Crypto Parameters version advertised in the peer info
+ * string, or 0 if none specified.
+ */
+static int
+tls_peer_info_ncp_ver(const char *peer_info)
+{
+    const char *ncpstr = peer_info ? strstr(peer_info, "IV_NCP=") : NULL;
+    if (ncpstr)
+    {
+        int ncp = 0;
+        int r = sscanf(ncpstr, "IV_NCP=%d", &ncp);
+        if (r == 1)
+        {
+            return ncp;
+        }
+    }
+    return 0;
+}
+
+/**
+ * Returns whether the client supports NCP either by
+ * announcing IV_NCP>=2 or the IV_CIPHERS list
+ */
+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;
+    }
+}
+
+bool
+tls_check_ncp_cipher_list(const char *list)
+{
+    bool unsupported_cipher_found = false;
+
+    ASSERT(list);
+
+    char *const tmp_ciphers = string_alloc(list, NULL);
+    const char *token = strtok(tmp_ciphers, ":");
+    while (token)
+    {
+        if (!cipher_kt_get(translate_cipher_name_from_openvpn(token)))
+        {
+            msg(M_WARN, "Unsupported cipher in --ncp-ciphers: %s", token);
+            unsupported_cipher_found = true;
+        }
+        token = strtok(NULL, ":");
+    }
+    free(tmp_ciphers);
+
+    return 0 < strlen(list) && !unsupported_cipher_found;
+}
+
+bool
+tls_item_in_cipher_list(const char *item, const char *list)
+{
+    char *tmp_ciphers = string_alloc(list, NULL);
+    char *tmp_ciphers_orig = tmp_ciphers;
+
+    const char *token = strtok(tmp_ciphers, ":");
+    while (token)
+    {
+        if (0 == strcmp(token, item))
+        {
+            break;
+        }
+        token = strtok(NULL, ":");
+    }
+    free(tmp_ciphers_orig);
+
+    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)
+{
+    if (o->ncp_enabled && remote_ciphername
+        && 0 != strcmp(o->ciphername, remote_ciphername))
+    {
+        if (tls_item_in_cipher_list(remote_ciphername, o->ncp_ciphers))
+        {
+            o->ciphername = string_alloc(remote_ciphername, &o->gc);
+            msg(D_TLS_DEBUG_LOW, "Using peer cipher '%s'", o->ciphername);
+        }
+    }
+}
+
diff --git a/src/openvpn/ssl_ncp.h b/src/openvpn/ssl_ncp.h
new file mode 100644
index 00000000..1257b2b6
--- /dev/null
+++ b/src/openvpn/ssl_ncp.h
@@ -0,0 +1,101 @@
+/*
+ *  OpenVPN -- An application to securely tunnel IP networks
+ *             over a single TCP/UDP port, with support for SSL/TLS-based
+ *             session authentication and key exchange,
+ *             packet encryption, packet authentication, and
+ *             packet compression.
+ *
+ *  Copyright (C) 2002-2018 OpenVPN Inc <sales@...515...>
+ *  Copyright (C) 2010-2018 Fox Crypto B.V. <openvpn@...1435...>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2
+ *  as published by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License along
+ *  with this program; if not, write to the Free Software Foundation, Inc.,
+ *  51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+
+/**
+ * @file Control Channel SSL/Data dynamic negotion Module
+ * This file is split from ssl.h to be able to unit test it.
+ */
+
+#ifndef OPENVPN_SSL_NCP_H
+#define OPENVPN_SSL_NCP_H
+
+#include "buffer.h"
+#include "options.h"
+
+/**
+ * Returns whether the client supports NCP either by
+ * announcing IV_NCP>=2 or the IV_CIPHERS list
+ */
+bool
+tls_peer_supports_ncp(const char *peer_info);
+
+/**
+ * "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.
+ */
+void tls_poor_mans_ncp(struct options *o, const char *remote_ciphername);
+
+/**
+ * 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.
+ *
+ * @param list          Colon-separated list of ciphers
+ *
+ * @returns true iff all ciphers in list are supported.
+ */
+bool tls_check_ncp_cipher_list(const char *list);
+
+/**
+ * Return true iff item is present in the colon-separated zero-terminated
+ * cipher list.
+ */
+bool tls_item_in_cipher_list(const char *item, const char *list);
+
+#endif /* ifndef OPENVPN_SSL_NCP_H */
diff --git a/tests/unit_tests/openvpn/Makefile.am b/tests/unit_tests/openvpn/Makefile.am
index 60e84639..f0880a6b 100644
--- a/tests/unit_tests/openvpn/Makefile.am
+++ b/tests/unit_tests/openvpn/Makefile.am
@@ -6,7 +6,7 @@ if HAVE_LD_WRAP_SUPPORT
 test_binaries += argv_testdriver buffer_testdriver
 endif
 
-test_binaries += crypto_testdriver packet_id_testdriver auth_token_testdriver
+test_binaries += crypto_testdriver packet_id_testdriver auth_token_testdriver ncp_testdriver
 if HAVE_LD_WRAP_SUPPORT
 test_binaries += tls_crypt_testdriver
 endif
@@ -110,3 +110,19 @@ auth_token_testdriver_SOURCES = test_auth_token.c mock_msg.c \
 	$(openvpn_srcdir)/packet_id.c \
 	$(openvpn_srcdir)/platform.c \
 	$(openvpn_srcdir)/base64.c
+
+
+ncp_testdriver_CFLAGS  = @TEST_CFLAGS@ \
+	-I$(openvpn_includedir) -I$(compat_srcdir) -I$(openvpn_srcdir) \
+	$(OPTIONAL_CRYPTO_CFLAGS)
+ncp_testdriver_LDFLAGS = @TEST_LDFLAGS@ \
+	$(OPTIONAL_CRYPTO_LIBS)
+
+ncp_testdriver_SOURCES = test_ncp.c mock_msg.c \
+	$(openvpn_srcdir)/buffer.c \
+	$(openvpn_srcdir)/crypto.c \
+	$(openvpn_srcdir)/crypto_mbedtls.c \
+	$(openvpn_srcdir)/crypto_openssl.c \
+	$(openvpn_srcdir)/otime.c \
+	$(openvpn_srcdir)/packet_id.c \
+	$(openvpn_srcdir)/platform.c
diff --git a/tests/unit_tests/openvpn/test_ncp.c b/tests/unit_tests/openvpn/test_ncp.c
new file mode 100644
index 00000000..3fc886c5
--- /dev/null
+++ b/tests/unit_tests/openvpn/test_ncp.c
@@ -0,0 +1,184 @@
+/*
+ *  OpenVPN -- An application to securely tunnel IP networks
+ *             over a single UDP port, with support for SSL/TLS-based
+ *             session authentication and key exchange,
+ *             packet encryption, packet authentication, and
+ *             packet compression.
+ *
+ *  Copyright (C) 2019 Arne Schwabe <arne@...1227...>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2
+ *  as published by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License along
+ *  with this program; if not, write to the Free Software Foundation, Inc.,
+ *  51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+
+#ifdef HAVE_CONFIG_H
+#include "config.h"
+#elif defined(_MSC_VER)
+#include "config-msvc.h"
+#endif
+
+#include "syshead.h"
+
+#include <stdio.h>
+#include <unistd.h>
+#include <stdlib.h>
+#include <stdarg.h>
+#include <string.h>
+#include <setjmp.h>
+#include <cmocka.h>
+
+#include "ssl_ncp.c"
+
+/* Defines for use in the tests and the mock parse_line() */
+
+const char *bf_chacha = "BF-CBC:CHACHA20-POLY1305";
+const char *aes_ciphers = "AES-256-GCM:AES-128-GCM";
+
+static void
+test_check_ncp_ciphers_list(void **state)
+{
+    bool have_chacha = cipher_kt_get("CHACHA20-POLY1305");
+
+    assert_true(tls_check_ncp_cipher_list(aes_ciphers));
+    assert_true(have_chacha == tls_check_ncp_cipher_list(bf_chacha));
+    assert_false(tls_check_ncp_cipher_list("vollbit"));
+    assert_false(tls_check_ncp_cipher_list("AES-256-GCM:vollbit"));
+}
+
+static void
+test_extract_client_ciphers(void **state)
+{
+    struct gc_arena gc = gc_new();
+    const char *client_peer_info;
+    const char *peer_list;
+
+    client_peer_info = "foo=bar\nIV_foo=y\nIV_NCP=2";
+    peer_list = tls_peer_ncp_list(client_peer_info, &gc);
+    assert_string_equal(aes_ciphers,peer_list);
+    assert_true(tls_peer_supports_ncp(client_peer_info));
+
+    client_peer_info = "foo=bar\nIV_foo=y\nIV_NCP=2\nIV_CIPHERS=BF-CBC";
+    peer_list = tls_peer_ncp_list(client_peer_info, &gc);
+    assert_string_equal("BF-CBC", peer_list);
+    assert_true(tls_peer_supports_ncp(client_peer_info));
+
+    client_peer_info = "IV_NCP=2\nIV_CIPHERS=BF-CBC:FOO-BAR\nIV_BAR=7";
+    peer_list = tls_peer_ncp_list(client_peer_info, &gc);
+    assert_string_equal("BF-CBC:FOO-BAR", peer_list);
+    assert_true(tls_peer_supports_ncp(client_peer_info));
+
+    client_peer_info = "IV_CIPHERS=BF-CBC:FOO-BAR\nIV_BAR=7";
+    peer_list = tls_peer_ncp_list(client_peer_info, &gc);
+    assert_string_equal("BF-CBC:FOO-BAR", peer_list);
+    assert_true(tls_peer_supports_ncp(client_peer_info));
+
+    client_peer_info = "IV_YOLO=NO\nIV_BAR=7";
+    peer_list = tls_peer_ncp_list(client_peer_info, &gc);
+    assert_string_equal("", peer_list);
+    assert_false(tls_peer_supports_ncp(client_peer_info));
+
+    peer_list = tls_peer_ncp_list(NULL, &gc);
+    assert_string_equal("", peer_list);
+    assert_false(tls_peer_supports_ncp(client_peer_info));
+
+    gc_free(&gc);
+}
+
+static void
+test_poor_man(void **state)
+{
+    struct gc_arena gc = gc_new();
+    char *best_cipher;
+
+    const char *serverlist="CHACHA20_POLY1305:AES-128-GCM";
+
+    best_cipher = ncp_get_best_cipher(serverlist, "BF-CBC",
+                                      "IV_YOLO=NO\nIV_BAR=7",
+                                      "BF-CBC", &gc);
+
+    assert_string_equal(best_cipher, "BF-CBC");
+
+    best_cipher = ncp_get_best_cipher(serverlist, "BF-CBC",
+                                      "IV_NCP=1\nIV_BAR=7",
+                                      "AES-128-GCM", &gc);
+
+    assert_string_equal(best_cipher, "AES-128-GCM");
+
+    best_cipher = ncp_get_best_cipher(serverlist, "BF-CBC",
+                                      NULL,
+                                      "AES-128-GCM", &gc);
+
+    assert_string_equal(best_cipher, "AES-128-GCM");
+
+    gc_free(&gc);
+}
+
+
+static void
+test_ncp_best(void **state)
+{
+    struct gc_arena gc = gc_new();
+    char *best_cipher;
+
+    const char *serverlist="CHACHA20_POLY1305:AES-128-GCM:AES-256-GCM";
+
+    best_cipher = ncp_get_best_cipher(serverlist, "BF-CBC",
+                                      "IV_YOLO=NO\nIV_NCP=2\nIV_BAR=7",
+                                      "BF-CBC", &gc);
+
+    assert_string_equal(best_cipher, "AES-128-GCM");
+
+    /* Best cipher is in --cipher of client */
+    best_cipher = ncp_get_best_cipher(serverlist, "BF-CBC",
+                                      "IV_NCP=2\nIV_BAR=7",
+                                      "CHACHA20_POLY1305", &gc);
+
+    assert_string_equal(best_cipher, "CHACHA20_POLY1305");
+
+    /* Best cipher is in --cipher of client */
+    best_cipher = ncp_get_best_cipher(serverlist, "BF-CBC",
+                                      "IV_CIPHERS=AES-128-GCM",
+                                      "AES-256-CBC", &gc);
+
+
+    assert_string_equal(best_cipher, "AES-128-GCM");
+
+    /* IV_NCP=2 should be ignored if IV_CIPHERS is sent */
+    best_cipher = ncp_get_best_cipher(serverlist, "BF-CBC",
+                                      "IV_FOO=7\nIV_CIPHERS=AES-256-GCM\nIV_NCP=2",
+                                      "AES-256-CBC", &gc);
+
+    assert_string_equal(best_cipher, "AES-256-GCM");
+
+
+    gc_free(&gc);
+}
+
+
+
+
+const struct CMUnitTest ncp_tests[] = {
+    cmocka_unit_test(test_check_ncp_ciphers_list),
+    cmocka_unit_test(test_extract_client_ciphers),
+    cmocka_unit_test(test_poor_man),
+    cmocka_unit_test(test_ncp_best)
+};
+
+
+int main(void)
+{
+#if defined(ENABLE_CRYPTO_OPENSSL)
+    OpenSSL_add_all_algorithms();
+#endif
+    return cmocka_run_group_tests(ncp_tests, NULL, NULL);
+}
-- 
2.25.0



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [Openvpn-devel] [PATCH applied] Re: Move NCP related function into a seperate file and add unit tests
  2020-02-21 10:07 ` [Openvpn-devel] [PATCH v5 4/5] Move NCP related function into a seperate file and add unit tests Arne Schwabe
@ 2020-03-09 13:22   ` Gert Doering
  0 siblings, 0 replies; 22+ messages in thread
From: Gert Doering @ 2020-03-09 13:22 UTC (permalink / raw)
  To: Arne Schwabe <arne@; +Cc: openvpn-devel

Your patch has been applied to the master branch.

Taking the ACK from Lev on the v4 patch, verified that v5 has the same
code changes ("diff -uw v4.txt v5.txt") and that the difference is indeed
only in the test module.

Tested on FreeBSD 11.3 / openssl 1.0.2s which does not have chacha20-poly
and *does* have AES-GCM.  The output is:

[ RUN      ] test_check_ncp_ciphers_list
Unsupported cipher in --ncp-ciphers: CHACHA20-POLY1305
Unsupported cipher in --ncp-ciphers: vollbit
Unsupported cipher in --ncp-ciphers: vollbit
[       OK ] test_check_ncp_ciphers_list

.. which looks good to me. 

Given David's comment about breakage on RHEL 7.7, I tried building
and testing on "CentOS 7.7.1908" with "OpenSSL 1.0.2k 19.el7", which 
hopefully relates.  It also passes nicely...

[==========] Running 4 test(s).
[ RUN      ] test_check_ncp_ciphers_list
Unsupported cipher in --ncp-ciphers: CHACHA20-POLY1305
Unsupported cipher in --ncp-ciphers: vollbit
Unsupported cipher in --ncp-ciphers: vollbit
[       OK ] test_check_ncp_ciphers_list

So, I consider David's NAK to be sufficiently addressed :-)

commit 1828f9c199cc8cdf4e9d8d0bf072e726604f69dc
Author: Arne Schwabe
Date:   Fri Feb 21 11:07:46 2020 +0100

     Move NCP related function into a seperate file and add unit tests

     Signed-off-by: Arne Schwabe <arne@...1227...>
     Acked-by: Lev Stipakov <lstipakov@...277...>
     Acked-by: Gert Doering <gert@...1296...>
     Message-Id: <20200221100746.7065-1-arne@...1227...>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg19499.html
     Signed-off-by: Gert Doering <gert@...1296...>

--
kind regards,

Gert Doering

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [Openvpn-devel] [PATCH v5 5/5] Normalise ncp-ciphers option and restrict it to 127 bytes
  2020-02-17 14:43 [Openvpn-devel] [PATCH v4 0/5] NCP v2 patch set Arne Schwabe
                   ` (5 preceding siblings ...)
  2020-02-21 10:07 ` [Openvpn-devel] [PATCH v5 4/5] Move NCP related function into a seperate file and add unit tests Arne Schwabe
@ 2020-02-21 10:07 ` Arne Schwabe
  2020-03-09 13:51   ` Gert Doering
  6 siblings, 1 reply; 22+ messages in thread
From: Arne Schwabe @ 2020-02-21 10:07 UTC (permalink / raw)
  To: openvpn-devel

In scenarios of mbed TLS vs OpenSSL we already normalise the ciphers
that are send via the wire protocol via OCC to not have a mismatch
warning between server and client. This is done by
translate_cipher_name_from_openvpn. The same applies also to the
ncp-ciphers list. Specifying non normalised names in ncp-ciphers will
cause negotation not to succeed if ciphers are not in the same form.
Therefore we will normalise the ciphers in options_postmutate.

The alternative and a lot less user friendly alternative would be to
bail if on of the ciphers in ncp-ciphers is not in its normalised form.

Also restrict the ncp-ciphers list to 127. This is somewhat arbitrary
but should prevent too large IV_CIPHER messages and problems sending
those. The server will accept also large IV_CIPHER values from clients.

Patch V2: Correct comment about normalising ciphers
Patch V3: Correct #ifdef statement
Patch V5: Fix tests with OpenSSL 1.0.2 and libraries missing Chacha

Signed-off-by: Arne Schwabe <arne@...1227...>
---
 doc/openvpn.8                       |  3 ++
 src/openvpn/options.c               | 14 ++++---
 src/openvpn/ssl_ncp.c               | 57 +++++++++++++++++++++++++----
 src/openvpn/ssl_ncp.h               | 19 +++++++++-
 tests/unit_tests/openvpn/test_ncp.c | 44 ++++++++++++++++++----
 5 files changed, 115 insertions(+), 22 deletions(-)

diff --git a/doc/openvpn.8 b/doc/openvpn.8
index d631ebe7..c0ec80f9 100644
--- a/doc/openvpn.8
+++ b/doc/openvpn.8
@@ -4406,6 +4406,9 @@ AES\-256\-GCM:AES\-256\-CBC" set can either specify "\-\-cipher BF\-CBC" or
 
 Note, for using NCP with a OpenVPN 2.4 peer this list must include
 the AES\-256\-GCM and AES\-128\-GCM ciphers.
+
+This list is restricted to be 127 chars long after conversion to OpenVPN
+ciphers.
 .\"*********************************************************
 .TP
 .B \-\-ncp\-disable
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index c3229156..6ad2f82e 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -2511,11 +2511,6 @@ options_postprocess_verify_ce(const struct options *options, const struct connec
     }
 #endif /* P2MP_SERVER */
 
-    if (options->ncp_enabled && !tls_check_ncp_cipher_list(options->ncp_ciphers))
-    {
-        msg(M_USAGE, "NCP cipher list contains unsupported ciphers.");
-    }
-
     if (options->keysize)
     {
         msg(M_WARN, "WARNING: --keysize is DEPRECATED and will be removed in OpenVPN 2.6");
@@ -3093,6 +3088,15 @@ options_postprocess_mutate(struct options *o)
 
     options_postprocess_mutate_invariant(o);
 
+    if (o->ncp_enabled)
+    {
+        o->ncp_ciphers = mutate_ncp_cipher_list(o->ncp_ciphers, &o->gc);
+        if (o->ncp_ciphers == NULL)
+        {
+            msg(M_USAGE, "NCP cipher list contains unsupported ciphers or is too long.");
+        }
+    }
+
     if (o->remote_list && !o->connection_list)
     {
         /*
diff --git a/src/openvpn/ssl_ncp.c b/src/openvpn/ssl_ncp.c
index 919b81e8..95824ad2 100644
--- a/src/openvpn/ssl_ncp.c
+++ b/src/openvpn/ssl_ncp.c
@@ -91,27 +91,70 @@ tls_peer_supports_ncp(const char *peer_info)
     }
 }
 
-bool
-tls_check_ncp_cipher_list(const char *list)
+char *
+mutate_ncp_cipher_list(const char *list, struct gc_arena *gc)
 {
-    bool unsupported_cipher_found = false;
+    bool error_found = false;
 
-    ASSERT(list);
+    struct buffer new_list  = alloc_buf(MAX_NCP_CIPHERS_LENGTH);
 
     char *const tmp_ciphers = string_alloc(list, NULL);
     const char *token = strtok(tmp_ciphers, ":");
     while (token)
     {
-        if (!cipher_kt_get(translate_cipher_name_from_openvpn(token)))
+        /*
+         * Going through a roundtrip by using translate_cipher_name_from_openvpn
+         * and translate_cipher_name_to_openvpn also normalises the cipher name,
+         * e.g. replacing AeS-128-gCm with AES-128-GCM
+         */
+        const char *cipher_name = translate_cipher_name_from_openvpn(token);
+        const cipher_kt_t *ktc = cipher_kt_get(cipher_name);
+        if (!ktc)
         {
             msg(M_WARN, "Unsupported cipher in --ncp-ciphers: %s", token);
-            unsupported_cipher_found = true;
+            error_found = true;
+        }
+        else
+        {
+            const char *ovpn_cipher_name =
+                translate_cipher_name_to_openvpn(cipher_kt_name(ktc));
+
+            if (buf_len(&new_list)> 0)
+            {
+                /* The next if condition ensure there is always space for
+                 * a :
+                 */
+                buf_puts(&new_list, ":");
+            }
+
+            /* Ensure buffer has capacity for cipher name + : + \0 */
+            if (!(buf_forward_capacity(&new_list) >
+                  strlen(ovpn_cipher_name) + 2))
+            {
+                msg(M_WARN, "Length of --ncp-ciphers is over the"
+                    "limit of 127 chars");
+                error_found = true;
+            }
+            else
+            {
+                buf_puts(&new_list, ovpn_cipher_name);
+            }
         }
         token = strtok(NULL, ":");
     }
+
+
+
+    char *ret = NULL;
+    if (!error_found && buf_len(&new_list) > 0)
+    {
+        buf_null_terminate(&new_list);
+        ret = string_alloc(buf_str(&new_list), gc);
+    }
     free(tmp_ciphers);
+    free_buf(&new_list);
 
-    return 0 < strlen(list) && !unsupported_cipher_found;
+    return ret;
 }
 
 bool
diff --git a/src/openvpn/ssl_ncp.h b/src/openvpn/ssl_ncp.h
index 1257b2b6..d00c222d 100644
--- a/src/openvpn/ssl_ncp.h
+++ b/src/openvpn/ssl_ncp.h
@@ -87,10 +87,17 @@ tls_peer_ncp_list(const char *peer_info, struct gc_arena *gc);
  * Check whether the ciphers in the supplied list are supported.
  *
  * @param list          Colon-separated list of ciphers
+ * @parms gc            gc_arena to allocate the returned string
  *
- * @returns true iff all ciphers in list are supported.
+ * @returns             colon separated string of normalised (via
+ *                      translate_cipher_name_from_openvpn) and
+ *                      zero terminated string iff all ciphers
+ *                      in list are supported and the total length
+ *                      is short than MAX_NCP_CIPHERS_LENGTH. NULL
+ *                      otherwise.
  */
-bool tls_check_ncp_cipher_list(const char *list);
+char *
+mutate_ncp_cipher_list(const char *list, struct gc_arena *gc);
 
 /**
  * Return true iff item is present in the colon-separated zero-terminated
@@ -98,4 +105,12 @@ bool tls_check_ncp_cipher_list(const char *list);
  */
 bool tls_item_in_cipher_list(const char *item, const char *list);
 
+/**
+ * The maximum length of a ncp-cipher string that is accepted.
+ *
+ * Since this list needs to be pushed as IV_CIPHERS, we are conservative
+ * about its length.
+ */
+#define MAX_NCP_CIPHERS_LENGTH 127
+
 #endif /* ifndef OPENVPN_SSL_NCP_H */
diff --git a/tests/unit_tests/openvpn/test_ncp.c b/tests/unit_tests/openvpn/test_ncp.c
index 3fc886c5..84edc7b8 100644
--- a/tests/unit_tests/openvpn/test_ncp.c
+++ b/tests/unit_tests/openvpn/test_ncp.c
@@ -47,12 +47,40 @@ const char *aes_ciphers = "AES-256-GCM:AES-128-GCM";
 static void
 test_check_ncp_ciphers_list(void **state)
 {
+    struct gc_arena gc = gc_new();
     bool have_chacha = cipher_kt_get("CHACHA20-POLY1305");
 
-    assert_true(tls_check_ncp_cipher_list(aes_ciphers));
-    assert_true(have_chacha == tls_check_ncp_cipher_list(bf_chacha));
-    assert_false(tls_check_ncp_cipher_list("vollbit"));
-    assert_false(tls_check_ncp_cipher_list("AES-256-GCM:vollbit"));
+
+    assert_string_equal(mutate_ncp_cipher_list(aes_ciphers, &gc), aes_ciphers);
+
+    if (have_chacha)
+    {
+        assert_string_equal(mutate_ncp_cipher_list(bf_chacha, &gc), bf_chacha);
+        assert_string_equal(mutate_ncp_cipher_list("BF-CBC:ChaCha20-Poly1305", &gc),
+                            bf_chacha);
+    }
+    else
+    {
+        assert_ptr_equal(mutate_ncp_cipher_list(bf_chacha, &gc), NULL);
+    }
+
+    assert_ptr_equal(mutate_ncp_cipher_list("vollbit", &gc), NULL);
+    assert_ptr_equal(mutate_ncp_cipher_list("AES-256-GCM:vollbit", &gc), NULL);
+    assert_ptr_equal(mutate_ncp_cipher_list("", &gc), NULL);
+
+    assert_ptr_equal(mutate_ncp_cipher_list(
+                         "ChaCha20-Poly1305:ChaCha20-Poly1305:ChaCha20-Poly1305:"
+                         "ChaCha20-Poly1305:ChaCha20-Poly1305:ChaCha20-Poly1305:"
+                         "ChaCha20-Poly1305", &gc), NULL);
+
+#ifdef ENABLE_CRYPTO_OPENSSL
+    assert_string_equal(mutate_ncp_cipher_list("id-aes128-GCM:id-aes256-GCM",
+                                               &gc), "AES-128-GCM:AES-256-GCM");
+#else
+    assert_string_equal(mutate_ncp_cipher_list("BLOWFISH-CBC",
+                                               &gc), "BF-CBC");
+#endif
+    gc_free(&gc);
 }
 
 static void
@@ -100,7 +128,7 @@ test_poor_man(void **state)
     struct gc_arena gc = gc_new();
     char *best_cipher;
 
-    const char *serverlist="CHACHA20_POLY1305:AES-128-GCM";
+    const char *serverlist = "CHACHA20_POLY1305:AES-128-GCM";
 
     best_cipher = ncp_get_best_cipher(serverlist, "BF-CBC",
                                       "IV_YOLO=NO\nIV_BAR=7",
@@ -130,7 +158,7 @@ test_ncp_best(void **state)
     struct gc_arena gc = gc_new();
     char *best_cipher;
 
-    const char *serverlist="CHACHA20_POLY1305:AES-128-GCM:AES-256-GCM";
+    const char *serverlist = "CHACHA20_POLY1305:AES-128-GCM:AES-256-GCM";
 
     best_cipher = ncp_get_best_cipher(serverlist, "BF-CBC",
                                       "IV_YOLO=NO\nIV_NCP=2\nIV_BAR=7",
@@ -166,7 +194,6 @@ test_ncp_best(void **state)
 
 
 
-
 const struct CMUnitTest ncp_tests[] = {
     cmocka_unit_test(test_check_ncp_ciphers_list),
     cmocka_unit_test(test_extract_client_ciphers),
@@ -175,7 +202,8 @@ const struct CMUnitTest ncp_tests[] = {
 };
 
 
-int main(void)
+int
+main(void)
 {
 #if defined(ENABLE_CRYPTO_OPENSSL)
     OpenSSL_add_all_algorithms();
-- 
2.25.0



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [Openvpn-devel] [PATCH v5 5/5] Normalise ncp-ciphers option and restrict it to 127 bytes
  2020-02-21 10:07 ` [Openvpn-devel] [PATCH v5 5/5] Normalise ncp-ciphers option and restrict it to 127 bytes Arne Schwabe
@ 2020-03-09 13:51   ` Gert Doering
  2020-03-12 11:36     ` [Openvpn-devel] [PATCH v6 " Arne Schwabe
  0 siblings, 1 reply; 22+ messages in thread
From: Gert Doering @ 2020-03-09 13:51 UTC (permalink / raw)
  To: Arne Schwabe <arne@; +Cc: openvpn-devel

[-- Attachment #1: Type: text/plain, Size: 1078 bytes --]

Hi,

On Fri, Feb 21, 2020 at 11:07:54AM +0100, Arne Schwabe wrote:
> Patch V5: Fix tests with OpenSSL 1.0.2 and libraries missing Chacha

Could you test this with mbedTLS 2.19, please?  Maybe it's a local thing,
but the test crashes here, with

[ RUN      ] test_check_ncp_ciphers_list
Unsupported cipher in --ncp-ciphers: ChaCha20-Poly1305
[  ERROR   ] --- Test failed with exception: Segmentation fault(11)
[  FAILED  ] test_check_ncp_ciphers_list


It passes for OpenSSL 1.0.2 on FreeBSD, CentOS 7.7 and OpenSSL 1.1.1 on
Linux.


Also, for v6, please fix that missing space :-)

Length of --ncp-ciphers is over thelimit of 127 chars
                                 ^^^

gert
-- 
"If was one thing all people took for granted, was conviction that if you 
 feed honest figures into a computer, honest figures come out. Never doubted 
 it myself till I met a computer with a sense of humor."
                             Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany                             gert@...1296...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 630 bytes --]

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [Openvpn-devel] [PATCH v6 5/5] Normalise ncp-ciphers option and restrict it to 127 bytes
  2020-03-09 13:51   ` Gert Doering
@ 2020-03-12 11:36     ` Arne Schwabe
  2020-03-27 15:19       ` David Sommerseth
  2020-03-27 15:26       ` [Openvpn-devel] [PATCH applied] " Gert Doering
  0 siblings, 2 replies; 22+ messages in thread
From: Arne Schwabe @ 2020-03-12 11:36 UTC (permalink / raw)
  To: openvpn-devel

In scenarios of mbed TLS vs OpenSSL we already normalise the ciphers
that are send via the wire protocol via OCC to not have a mismatch
warning between server and client. This is done by
translate_cipher_name_from_openvpn. The same applies also to the
ncp-ciphers list. Specifying non normalised names in ncp-ciphers will
cause negotation not to succeed if ciphers are not in the same form.
Therefore we will normalise the ciphers in options_postmutate.

The alternative and a lot less user friendly alternative would be to
bail if on of the ciphers in ncp-ciphers is not in its normalised form.

Also restrict the ncp-ciphers list to 127. This is somewhat arbitrary
but should prevent too large IV_CIPHER messages and problems sending
those. The server will accept also large IV_CIPHER values from clients.

Patch V2: Correct comment about normalising ciphers
Patch V3: Correct #ifdef statement
Patch V5: Fix tests with OpenSSL 1.0.2 and libraries missing Chacha
Patch V6: Fix unit tests for mbed tls, which recognises ChaCha20-Poly1305
          only when used with all uppercase, fix missing space in message

Signed-off-by: Arne Schwabe <arne@...1227...>
---
 doc/openvpn.8                       |  3 ++
 src/openvpn/options.c               | 14 ++++---
 src/openvpn/ssl_ncp.c               | 57 +++++++++++++++++++++++++----
 src/openvpn/ssl_ncp.h               | 19 +++++++++-
 tests/unit_tests/openvpn/test_ncp.c | 54 +++++++++++++++++++++++----
 5 files changed, 125 insertions(+), 22 deletions(-)

diff --git a/doc/openvpn.8 b/doc/openvpn.8
index 864f94e8..c5b16981 100644
--- a/doc/openvpn.8
+++ b/doc/openvpn.8
@@ -4406,6 +4406,9 @@ AES\-256\-GCM:AES\-256\-CBC" set can either specify "\-\-cipher BF\-CBC" or
 
 Note, for using NCP with a OpenVPN 2.4 peer this list must include
 the AES\-256\-GCM and AES\-128\-GCM ciphers.
+
+This list is restricted to be 127 chars long after conversion to OpenVPN
+ciphers.
 .\"*********************************************************
 .TP
 .B \-\-ncp\-disable
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index e79a1215..5127f653 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -2516,11 +2516,6 @@ options_postprocess_verify_ce(const struct options *options, const struct connec
     }
 #endif /* P2MP_SERVER */
 
-    if (options->ncp_enabled && !tls_check_ncp_cipher_list(options->ncp_ciphers))
-    {
-        msg(M_USAGE, "NCP cipher list contains unsupported ciphers.");
-    }
-
     if (options->keysize)
     {
         msg(M_WARN, "WARNING: --keysize is DEPRECATED and will be removed in OpenVPN 2.6");
@@ -3098,6 +3093,15 @@ options_postprocess_mutate(struct options *o)
 
     options_postprocess_mutate_invariant(o);
 
+    if (o->ncp_enabled)
+    {
+        o->ncp_ciphers = mutate_ncp_cipher_list(o->ncp_ciphers, &o->gc);
+        if (o->ncp_ciphers == NULL)
+        {
+            msg(M_USAGE, "NCP cipher list contains unsupported ciphers or is too long.");
+        }
+    }
+
     if (o->remote_list && !o->connection_list)
     {
         /*
diff --git a/src/openvpn/ssl_ncp.c b/src/openvpn/ssl_ncp.c
index d884e2b5..9ed6ff5f 100644
--- a/src/openvpn/ssl_ncp.c
+++ b/src/openvpn/ssl_ncp.c
@@ -91,27 +91,70 @@ tls_peer_supports_ncp(const char *peer_info)
     }
 }
 
-bool
-tls_check_ncp_cipher_list(const char *list)
+char *
+mutate_ncp_cipher_list(const char *list, struct gc_arena *gc)
 {
-    bool unsupported_cipher_found = false;
+    bool error_found = false;
 
-    ASSERT(list);
+    struct buffer new_list  = alloc_buf(MAX_NCP_CIPHERS_LENGTH);
 
     char *const tmp_ciphers = string_alloc(list, NULL);
     const char *token = strtok(tmp_ciphers, ":");
     while (token)
     {
-        if (!cipher_kt_get(translate_cipher_name_from_openvpn(token)))
+        /*
+         * Going through a roundtrip by using translate_cipher_name_from_openvpn
+         * and translate_cipher_name_to_openvpn also normalises the cipher name,
+         * e.g. replacing AeS-128-gCm with AES-128-GCM
+         */
+        const char *cipher_name = translate_cipher_name_from_openvpn(token);
+        const cipher_kt_t *ktc = cipher_kt_get(cipher_name);
+        if (!ktc)
         {
             msg(M_WARN, "Unsupported cipher in --ncp-ciphers: %s", token);
-            unsupported_cipher_found = true;
+            error_found = true;
+        }
+        else
+        {
+            const char *ovpn_cipher_name =
+                translate_cipher_name_to_openvpn(cipher_kt_name(ktc));
+
+            if (buf_len(&new_list)> 0)
+            {
+                /* The next if condition ensure there is always space for
+                 * a :
+                 */
+                buf_puts(&new_list, ":");
+            }
+
+            /* Ensure buffer has capacity for cipher name + : + \0 */
+            if (!(buf_forward_capacity(&new_list) >
+                  strlen(ovpn_cipher_name) + 2))
+            {
+                msg(M_WARN, "Length of --ncp-ciphers is over the "
+                    "limit of 127 chars");
+                error_found = true;
+            }
+            else
+            {
+                buf_puts(&new_list, ovpn_cipher_name);
+            }
         }
         token = strtok(NULL, ":");
     }
+
+
+
+    char *ret = NULL;
+    if (!error_found && buf_len(&new_list) > 0)
+    {
+        buf_null_terminate(&new_list);
+        ret = string_alloc(buf_str(&new_list), gc);
+    }
     free(tmp_ciphers);
+    free_buf(&new_list);
 
-    return 0 < strlen(list) && !unsupported_cipher_found;
+    return ret;
 }
 
 bool
diff --git a/src/openvpn/ssl_ncp.h b/src/openvpn/ssl_ncp.h
index 1257b2b6..d00c222d 100644
--- a/src/openvpn/ssl_ncp.h
+++ b/src/openvpn/ssl_ncp.h
@@ -87,10 +87,17 @@ tls_peer_ncp_list(const char *peer_info, struct gc_arena *gc);
  * Check whether the ciphers in the supplied list are supported.
  *
  * @param list          Colon-separated list of ciphers
+ * @parms gc            gc_arena to allocate the returned string
  *
- * @returns true iff all ciphers in list are supported.
+ * @returns             colon separated string of normalised (via
+ *                      translate_cipher_name_from_openvpn) and
+ *                      zero terminated string iff all ciphers
+ *                      in list are supported and the total length
+ *                      is short than MAX_NCP_CIPHERS_LENGTH. NULL
+ *                      otherwise.
  */
-bool tls_check_ncp_cipher_list(const char *list);
+char *
+mutate_ncp_cipher_list(const char *list, struct gc_arena *gc);
 
 /**
  * Return true iff item is present in the colon-separated zero-terminated
@@ -98,4 +105,12 @@ bool tls_check_ncp_cipher_list(const char *list);
  */
 bool tls_item_in_cipher_list(const char *item, const char *list);
 
+/**
+ * The maximum length of a ncp-cipher string that is accepted.
+ *
+ * Since this list needs to be pushed as IV_CIPHERS, we are conservative
+ * about its length.
+ */
+#define MAX_NCP_CIPHERS_LENGTH 127
+
 #endif /* ifndef OPENVPN_SSL_NCP_H */
diff --git a/tests/unit_tests/openvpn/test_ncp.c b/tests/unit_tests/openvpn/test_ncp.c
index 3fc886c5..f8d03b35 100644
--- a/tests/unit_tests/openvpn/test_ncp.c
+++ b/tests/unit_tests/openvpn/test_ncp.c
@@ -47,12 +47,50 @@ const char *aes_ciphers = "AES-256-GCM:AES-128-GCM";
 static void
 test_check_ncp_ciphers_list(void **state)
 {
+    struct gc_arena gc = gc_new();
     bool have_chacha = cipher_kt_get("CHACHA20-POLY1305");
 
-    assert_true(tls_check_ncp_cipher_list(aes_ciphers));
-    assert_true(have_chacha == tls_check_ncp_cipher_list(bf_chacha));
-    assert_false(tls_check_ncp_cipher_list("vollbit"));
-    assert_false(tls_check_ncp_cipher_list("AES-256-GCM:vollbit"));
+
+
+    assert_string_equal(mutate_ncp_cipher_list(aes_ciphers, &gc), aes_ciphers);
+
+    if (have_chacha)
+    {
+        assert_string_equal(mutate_ncp_cipher_list(bf_chacha, &gc), bf_chacha);
+        assert_string_equal(mutate_ncp_cipher_list("BF-CBC:CHACHA20-POLY1305", &gc),
+                            bf_chacha);
+    }
+    else
+    {
+        assert_ptr_equal(mutate_ncp_cipher_list(bf_chacha, &gc), NULL);
+    }
+
+    /* For testing that with OpenSSL 1.1.0+ that also accepts ciphers in
+       a different spelling the normalised cipher output is the same */
+    bool have_chacha_mixed_case = cipher_kt_get("ChaCha20-Poly1305");
+    if (have_chacha_mixed_case)
+    {
+      assert_string_equal(mutate_ncp_cipher_list("BF-CBC:ChaCha20-Poly1305", &gc),
+			  bf_chacha);
+    }
+
+    assert_ptr_equal(mutate_ncp_cipher_list("vollbit", &gc), NULL);
+    assert_ptr_equal(mutate_ncp_cipher_list("AES-256-GCM:vollbit", &gc), NULL);
+    assert_ptr_equal(mutate_ncp_cipher_list("", &gc), NULL);
+
+    assert_ptr_equal(mutate_ncp_cipher_list(
+                         "ChaCha20-Poly1305:ChaCha20-Poly1305:ChaCha20-Poly1305:"
+                         "ChaCha20-Poly1305:ChaCha20-Poly1305:ChaCha20-Poly1305:"
+                         "ChaCha20-Poly1305", &gc), NULL);
+
+#ifdef ENABLE_CRYPTO_OPENSSL
+    assert_string_equal(mutate_ncp_cipher_list("id-aes128-GCM:id-aes256-GCM",
+                                               &gc), "AES-128-GCM:AES-256-GCM");
+#else
+    assert_string_equal(mutate_ncp_cipher_list("BLOWFISH-CBC",
+                                               &gc), "BF-CBC");
+#endif
+    gc_free(&gc);
 }
 
 static void
@@ -100,7 +138,7 @@ test_poor_man(void **state)
     struct gc_arena gc = gc_new();
     char *best_cipher;
 
-    const char *serverlist="CHACHA20_POLY1305:AES-128-GCM";
+    const char *serverlist = "CHACHA20_POLY1305:AES-128-GCM";
 
     best_cipher = ncp_get_best_cipher(serverlist, "BF-CBC",
                                       "IV_YOLO=NO\nIV_BAR=7",
@@ -130,7 +168,7 @@ test_ncp_best(void **state)
     struct gc_arena gc = gc_new();
     char *best_cipher;
 
-    const char *serverlist="CHACHA20_POLY1305:AES-128-GCM:AES-256-GCM";
+    const char *serverlist = "CHACHA20_POLY1305:AES-128-GCM:AES-256-GCM";
 
     best_cipher = ncp_get_best_cipher(serverlist, "BF-CBC",
                                       "IV_YOLO=NO\nIV_NCP=2\nIV_BAR=7",
@@ -166,7 +204,6 @@ test_ncp_best(void **state)
 
 
 
-
 const struct CMUnitTest ncp_tests[] = {
     cmocka_unit_test(test_check_ncp_ciphers_list),
     cmocka_unit_test(test_extract_client_ciphers),
@@ -175,7 +212,8 @@ const struct CMUnitTest ncp_tests[] = {
 };
 
 
-int main(void)
+int
+main(void)
 {
 #if defined(ENABLE_CRYPTO_OPENSSL)
     OpenSSL_add_all_algorithms();
-- 
2.25.0



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [Openvpn-devel] [PATCH v6 5/5] Normalise ncp-ciphers option and restrict it to 127 bytes
  2020-03-12 11:36     ` [Openvpn-devel] [PATCH v6 " Arne Schwabe
@ 2020-03-27 15:19       ` David Sommerseth
  2020-03-27 15:26       ` [Openvpn-devel] [PATCH applied] " Gert Doering
  1 sibling, 0 replies; 22+ messages in thread
From: David Sommerseth @ 2020-03-27 15:19 UTC (permalink / raw)
  To: Arne Schwabe <arne@


[-- Attachment #1.1: Type: text/plain, Size: 1908 bytes --]

On 12/03/2020 12:36, Arne Schwabe wrote:
> In scenarios of mbed TLS vs OpenSSL we already normalise the ciphers
> that are send via the wire protocol via OCC to not have a mismatch
> warning between server and client. This is done by
> translate_cipher_name_from_openvpn. The same applies also to the
> ncp-ciphers list. Specifying non normalised names in ncp-ciphers will
> cause negotation not to succeed if ciphers are not in the same form.
> Therefore we will normalise the ciphers in options_postmutate.
> 
> The alternative and a lot less user friendly alternative would be to
> bail if on of the ciphers in ncp-ciphers is not in its normalised form.
> 
> Also restrict the ncp-ciphers list to 127. This is somewhat arbitrary
> but should prevent too large IV_CIPHER messages and problems sending
> those. The server will accept also large IV_CIPHER values from clients.
> 
> Patch V2: Correct comment about normalising ciphers
> Patch V3: Correct #ifdef statement
> Patch V5: Fix tests with OpenSSL 1.0.2 and libraries missing Chacha
> Patch V6: Fix unit tests for mbed tls, which recognises ChaCha20-Poly1305
>           only when used with all uppercase, fix missing space in message
> 
> Signed-off-by: Arne Schwabe <arne@...1227...>
> ---
>  doc/openvpn.8                       |  3 ++
>  src/openvpn/options.c               | 14 ++++---
>  src/openvpn/ssl_ncp.c               | 57 +++++++++++++++++++++++++----
>  src/openvpn/ssl_ncp.h               | 19 +++++++++-
>  tests/unit_tests/openvpn/test_ncp.c | 54 +++++++++++++++++++++++----
>  5 files changed, 125 insertions(+), 22 deletions(-)
> 

I've only done quick code review and built it on RHEL7 not finding any issues.
 Code looks reasonable, so I don't see any reason to hold this back any more.

Acked-by: David Sommerseth <davids@...515...>

-- 
kind regards,

David Sommerseth
OpenVPN Inc



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [Openvpn-devel] [PATCH applied] Re: Normalise ncp-ciphers option and restrict it to 127 bytes
  2020-03-12 11:36     ` [Openvpn-devel] [PATCH v6 " Arne Schwabe
  2020-03-27 15:19       ` David Sommerseth
@ 2020-03-27 15:26       ` Gert Doering
  1 sibling, 0 replies; 22+ messages in thread
From: Gert Doering @ 2020-03-27 15:26 UTC (permalink / raw)
  To: Arne Schwabe <arne@; +Cc: openvpn-devel

Your patch has been applied to the master branch.

(v5 and v6 indeed differ only in the cmocka tests and should fix the
crash in v5 with mbedtls)

commit be4531564e2be7c8a0222e6923e3f7580b358cab
Author: Arne Schwabe
Date:   Thu Mar 12 12:36:54 2020 +0100

     Normalise ncp-ciphers option and restrict it to 127 bytes

     Signed-off-by: Arne Schwabe <arne@...1227...>
     Acked-by: David Sommerseth <davids@...515...>
     Message-Id: <20200312113654.16184-1-arne@...1227...>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg19546.html
     Signed-off-by: Gert Doering <gert@...1296...>


--
kind regards,

Gert Doering



^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2020-03-27 15:26 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-02-17 14:43 [Openvpn-devel] [PATCH v4 0/5] NCP v2 patch set Arne Schwabe
2020-02-17 14:43 ` [Openvpn-devel] [PATCH v4 1/5] Only announce IV_NCP=2 when we are willing to support these ciphers Arne Schwabe
2020-02-17 14:50   ` Lev Stipakov
2020-02-17 18:33   ` [Openvpn-devel] [PATCH applied] " Gert Doering
2020-02-17 14:43 ` [Openvpn-devel] [PATCH v4 2/5] Add strsep compat function Arne Schwabe
2020-02-17 14:52   ` Lev Stipakov
2020-02-17 18:39   ` [Openvpn-devel] [PATCH applied] " Gert Doering
2020-02-17 14:43 ` [Openvpn-devel] [PATCH v4 3/5] Implement dynamic NCP negotiation Arne Schwabe
2020-02-17 15:03   ` Lev Stipakov
2020-02-17 19:25   ` [Openvpn-devel] [PATCH applied] " Gert Doering
2020-02-17 14:43 ` [Openvpn-devel] [PATCH v4 4/5] Move NCP related function into a seperate file and add unit tests Arne Schwabe
2020-02-17 15:24   ` Lev Stipakov
2020-02-20 17:31   ` David Sommerseth
2020-02-17 14:43 ` [Openvpn-devel] [PATCH v4 5/5] Normalise ncp-ciphers option and restrict it to 127 bytes Arne Schwabe
2020-02-17 15:26   ` Lev Stipakov
2020-02-21 10:07 ` [Openvpn-devel] [PATCH v5 4/5] Move NCP related function into a seperate file and add unit tests Arne Schwabe
2020-03-09 13:22   ` [Openvpn-devel] [PATCH applied] " Gert Doering
2020-02-21 10:07 ` [Openvpn-devel] [PATCH v5 5/5] Normalise ncp-ciphers option and restrict it to 127 bytes Arne Schwabe
2020-03-09 13:51   ` Gert Doering
2020-03-12 11:36     ` [Openvpn-devel] [PATCH v6 " Arne Schwabe
2020-03-27 15:19       ` David Sommerseth
2020-03-27 15:26       ` [Openvpn-devel] [PATCH applied] " Gert Doering

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.