[Openvpn-devel,1/9] Implement optional cipher in --data-ciphers prefixed with ?

Message ID 20211201180727.2496903-1-arne@rfc2549.org
State Changes Requested
Headers show
Series
  • [Openvpn-devel,1/9] Implement optional cipher in --data-ciphers prefixed with ?
Related show

Commit Message

Arne Schwabe Dec. 1, 2021, 6:07 p.m.
This allows to use the same configuration multiple platforms/ssl libraries
and include optional algorithms that are not available on all platforms

For example "AES-256-GCM:AES-128-GCM:?CHACHA20-POLY1305" can be used to
emulate the default behaviour of OpenVPN 2.6.

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 Changes.rst                           |  4 ++++
 doc/man-sections/protocol-options.rst |  7 +++++++
 src/openvpn/ssl_ncp.c                 | 16 ++++++++++++++--
 tests/unit_tests/openvpn/test_ncp.c   | 11 +++++++++++
 4 files changed, 36 insertions(+), 2 deletions(-)

Comments

Gert Doering Dec. 5, 2021, 5:11 p.m. | #1
Hi,

On Wed, Dec 01, 2021 at 07:07:19PM +0100, Arne Schwabe wrote:
> This allows to use the same configuration multiple platforms/ssl libraries
> and include optional algorithms that are not available on all platforms
> 
> For example "AES-256-GCM:AES-128-GCM:?CHACHA20-POLY1305" can be used to
> emulate the default behaviour of OpenVPN 2.6.

NAK on this, for two small and one big reason...

> diff --git a/Changes.rst b/Changes.rst
> index 7cceffcdb..c1a04deed 100644
> --- a/Changes.rst
> +++ b/Changes.rst
> @@ -58,6 +58,10 @@ OpenSSL 3.0 support
>      (and other deprecated) algorithm by default and the new option ``--providers``
>      allows loading the legacy provider to renable these algorithms.
>  
> +Optional ciphers in ``--data-ciphers``
> +    Ciphers in ``--data-ciphers`` can now be prefixes with a ``?`` to mark
> +    those as optional and only use them if the SSL library supports them.

Typo, "prefixed"

> +
>  Deprecated features
>  -------------------
>  ``inetd`` has been removed
> diff --git a/doc/man-sections/protocol-options.rst b/doc/man-sections/protocol-options.rst
> index c7aa6b0e3..7095b6f4d 100644
> diff --git a/src/openvpn/ssl_ncp.c b/src/openvpn/ssl_ncp.c
> index 022a9dc3b..b0b248aae 100644
> --- a/src/openvpn/ssl_ncp.c
> +++ b/src/openvpn/ssl_ncp.c
> @@ -109,7 +109,18 @@ mutate_ncp_cipher_list(const char *list, struct gc_arena *gc)
>           * (and translate_cipher_name_from_openvpn/
>           * translate_cipher_name_to_openvpn) also normalises the cipher name,
>           * e.g. replacing AeS-128-gCm with AES-128-GCM
> +         *
> +         * ciphers that have ? in front of them are considered optional and
> +         * OpenVPN will only warn if they are not found (and remove them from
> +         * the list)
>           */
> +
> +        bool optional = false;
> +        if (token[0] == '?')
> +        {
> +            token= token + 1;

The whitespace dragon says this should be "token++;"

(These are small, and I could fix that on the fly).


The problem is here:

> @@ -121,8 +132,9 @@ mutate_ncp_cipher_list(const char *list, struct gc_arena *gc)
>          }
>          if (!ktc && strcmp(token, "none") != 0)
>          {
> -            msg(M_WARN, "Unsupported cipher in --data-ciphers: %s", token);
> -            error_found = true;
> +            const char* optstr = optional ? "optional ": "";
> +            msg(M_WARN, "Unsupported %scipher in --data-ciphers: %s", optstr, token);
> +            error_found = !optional;
>          }

This only works if the last cipher in the list is "borken or optional".

If you do

  openvpn --data-ciphers Vollbit:Littlebit:AES-256-CBC:BF-CBC:?nixbit

it will log

2021-12-05 18:09:27 Unsupported cipher in --data-ciphers: Vollbit
2021-12-05 18:09:27 Unsupported cipher in --data-ciphers: Littlebit
2021-12-05 18:09:27 Unsupported optional cipher in --data-ciphers: nixbit

... and go ahead...

Dec  5 18:09:28 gentoo tap-tcp-p2p[26765]: peer info: IV_CIPHERS=AES-256-CBC:BF-CBC

without flagging "Vollbit" and "Littlebit" as hard errors.

If the "nixbit" cipher is removed

  openvpn --data-ciphers Vollbit:Littlebit:AES-256-CBC:BF-CBC

will lead to

2021-12-05 18:10:29 Unsupported cipher in --data-ciphers: Vollbit
2021-12-05 18:10:29 Unsupported cipher in --data-ciphers: Littlebit
Options error: --data-ciphers list contains unsupported ciphers or is too long.


Sorry for excercising my special ability of breaking everything :-)

gert
Gert Doering Dec. 6, 2021, 3:41 p.m. | #2
Hi,

On Sun, Dec 05, 2021 at 06:11:14PM +0100, Gert Doering wrote:
> On Wed, Dec 01, 2021 at 07:07:19PM +0100, Arne Schwabe wrote:
> > This allows to use the same configuration multiple platforms/ssl libraries
> > and include optional algorithms that are not available on all platforms
> > 
> > For example "AES-256-GCM:AES-128-GCM:?CHACHA20-POLY1305" can be used to
> > emulate the default behaviour of OpenVPN 2.6.
> 
> NAK on this, for two small and one big reason...

Turns out that this is an ACK after all.  It was not *that* late yesterday,
but it seems my brain already went south...  I merged this, tested it,
figured I did not like the result, and forgot to "git reset" the tree...

... and today it slipped out with the next patch.

Thus:

commit 766044507497c41f0319159c37992788ecb681e6
Author: Arne Schwabe <arne@rfc2549.org>
Date:   Wed Dec 1 19:07:19 2021 +0100

    Implement optional cipher in --data-ciphers prefixed with ?

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


(the patch does not *break* anything, does what it says on the lid, just
is not strict enough about erroring out - and Arne already sent a final
one for that)

Sorry for the confusion.

gert

Patch

diff --git a/Changes.rst b/Changes.rst
index 7cceffcdb..c1a04deed 100644
--- a/Changes.rst
+++ b/Changes.rst
@@ -58,6 +58,10 @@  OpenSSL 3.0 support
     (and other deprecated) algorithm by default and the new option ``--providers``
     allows loading the legacy provider to renable these algorithms.
 
+Optional ciphers in ``--data-ciphers``
+    Ciphers in ``--data-ciphers`` can now be prefixes with a ``?`` to mark
+    those as optional and only use them if the SSL library supports them.
+
 Deprecated features
 -------------------
 ``inetd`` has been removed
diff --git a/doc/man-sections/protocol-options.rst b/doc/man-sections/protocol-options.rst
index c7aa6b0e3..7095b6f4d 100644
--- a/doc/man-sections/protocol-options.rst
+++ b/doc/man-sections/protocol-options.rst
@@ -204,6 +204,13 @@  configured in a compatible way between both the local and remote side.
   supported by the client will be pushed to clients that support cipher
   negotiation.
 
+  Starting with OpenVPN 2.6 a cipher can be prefixed with a :code:`?` to mark
+  it as optional. This allows including ciphers in the list that may not be
+  available on all platforms.
+  E.g. :code:`AES-256-GCM:AES-128-GCM:?CHACHA20-POLY1305` would only enable
+  Chacha20-Poly1305 if the underlying SSL library (and its configuration)
+  supports it.
+
   Cipher negotiation is enabled in client-server mode only. I.e. if
   ``--mode`` is set to 'server' (server-side, implied by setting
   ``--server`` ), or if ``--pull`` is specified (client-side, implied by
diff --git a/src/openvpn/ssl_ncp.c b/src/openvpn/ssl_ncp.c
index 022a9dc3b..b0b248aae 100644
--- a/src/openvpn/ssl_ncp.c
+++ b/src/openvpn/ssl_ncp.c
@@ -109,7 +109,18 @@  mutate_ncp_cipher_list(const char *list, struct gc_arena *gc)
          * (and translate_cipher_name_from_openvpn/
          * translate_cipher_name_to_openvpn) also normalises the cipher name,
          * e.g. replacing AeS-128-gCm with AES-128-GCM
+         *
+         * ciphers that have ? in front of them are considered optional and
+         * OpenVPN will only warn if they are not found (and remove them from
+         * the list)
          */
+
+        bool optional = false;
+        if (token[0] == '?')
+        {
+            token= token + 1;
+            optional = true;
+        }
         const cipher_kt_t *ktc = cipher_kt_get(token);
         if (strcmp(token, "none") == 0)
         {
@@ -121,8 +132,9 @@  mutate_ncp_cipher_list(const char *list, struct gc_arena *gc)
         }
         if (!ktc && strcmp(token, "none") != 0)
         {
-            msg(M_WARN, "Unsupported cipher in --data-ciphers: %s", token);
-            error_found = true;
+            const char* optstr = optional ? "optional ": "";
+            msg(M_WARN, "Unsupported %scipher in --data-ciphers: %s", optstr, token);
+            error_found = !optional;
         }
         else
         {
diff --git a/tests/unit_tests/openvpn/test_ncp.c b/tests/unit_tests/openvpn/test_ncp.c
index 6fb0c0e51..faf09a36c 100644
--- a/tests/unit_tests/openvpn/test_ncp.c
+++ b/tests/unit_tests/openvpn/test_ncp.c
@@ -84,6 +84,17 @@  test_check_ncp_ciphers_list(void **state)
         assert_ptr_equal(mutate_ncp_cipher_list(bf_chacha, &gc), NULL);
     }
 
+    /* Check that optional ciphers work */
+    assert_string_equal(mutate_ncp_cipher_list("AES-256-GCM:?vollbit:AES-128-GCM", &gc),
+                        aes_ciphers);
+
+    /* Check that optional ciphers work */
+    assert_string_equal(mutate_ncp_cipher_list("?AES-256-GCM:?AES-128-GCM", &gc),
+                        aes_ciphers);
+
+    /* All unsupported should still yield an empty list */
+    assert_ptr_equal(mutate_ncp_cipher_list("?kugelfisch:?grasshopper", &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");