do not include --cipher value in data-ciphers

The --cipher option has been there since a while, but it became more and
more confusing since the introduction of NCP (data cipher negotiation).

The fallback cipher can now be specified via --data-cipher-fallback,
while the list of accepted ciphers is specified via --data-ciphers.

--cipher can still be used for compatibility reasons, but won't affect
the cipher negotiation.

Adjust manpage to make clear that using --cipher in today's config really
is a thing from the past, and --data-ciphers should be used instead.

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
Signed-off-by: Antonio Quartulli <a@unstable.cc>
Acked-by: Arne Schwabe <arne@rfc2549.org>
Message-Id: <20210904095629.6273-5-a@unstable.cc>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg22799.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>

Changes.rst

@@ -71,6 +71,18 @@ Deprecated features
     This option mainly served a role as debug option when NCP was first
     introduced. It should now no longer be necessary.
 
+``--cipher`` argument is no longer appended to ``--data-ciphers``
+    by default. Data cipher negotiation has been introduced in 2.4.0
+    and been significantly improved in 2.5.0. The implicit fallback
+    to the cipher specified in ``--cipher`` has been removed.
+    Effectively, ``--cipher`` is a no-op in TLS mode now, and will
+    only have an effect in pre-shared-key mode (``--secret``).
+    From now on ``--cipher`` should not be used in new configurations
+    for TLS mode.
+    Should backwards compatibility with older OpenVPN peers be
+    required, please see the ``--compat-mode`` instead.
+
+
 Compression no longer enabled by default
     Unless an explicit compression option is specified in the configuration,
     ``--allow-compression`` defaults to ``no`` in OpeNVPN 2.6.0.

doc/man-sections/generic-options.rst

@@ -66,6 +66,8 @@ which mode OpenVPN is configured as.
 
   - 2.5.x or lower: ``--allow-compression asym`` is automatically added
     to the configuration if no other compression options are present.
+  - 2.4.x or lower: The cipher in ``--cipher`` is appended to
+    ``--data-ciphers``
 
 --config file
   Load additional config options from ``file`` where each line corresponds

doc/man-sections/protocol-options.rst

@@ -57,26 +57,28 @@ configured in a compatible way between both the local and remote side.
   http://www.cs.ucsd.edu/users/mihir/papers/hmac.html
 
 --cipher alg
-  This option is deprecated for server-client mode. ``--data-ciphers``
-  or possibly `--data-ciphers-fallback`` should be used instead.
-
-  Encrypt data channel packets with cipher algorithm ``alg``.
-
-  The default is :code:`BF-CBC`, an abbreviation for Blowfish in Cipher
-  Block Chaining mode. When cipher negotiation (NCP) is allowed,
-  OpenVPN 2.4 and newer on both client and server side will automatically
-  upgrade to :code:`AES-256-GCM`.  See ``--data-ciphers`` for more details
-  on NCP.
-
-  Using :code:`BF-CBC` is no longer recommended, because of its 64-bit
-  block size. This small block size allows attacks based on collisions, as
-  demonstrated by SWEET32. See
-  https://community.openvpn.net/openvpn/wiki/SWEET32
-  for details. Due to this, support for :code:`BF-CBC`, :code:`DES`,
-  :code:`CAST5`, :code:`IDEA` and :code:`RC2` ciphers will be removed in
-  OpenVPN 2.6.
-
-  To see other ciphers that are available with OpenVPN, use the
+  This option should not be used any longer in TLS mode and still
+  exists for two reasons:
+      * compatibility with old configurations still carrying it
+	around;
+      * allow users connecting to OpenVPN peers older than 2.6.0
+	to have ``--cipher`` configured the same way as the remote
+	counterpart. This can avoid MTU/frame size warnings.
+  Before 2.4.0, this option was used to select the cipher to be
+  configured on the data channel, however, later versions usually
+  ignored this directive in favour of a negotiated cipher.
+  Starting with 2.6.0, this option is always ignored in TLS mode
+  when it comes to configuring the cipher and will only control the
+  cipher for ``--secret`` pre-shared-key mode (note: this mode is
+  deprecated strictly not recommended).
+
+  If you wish to specify the cipher to use on the data channel,
+  please see ``--data-ciphers`` (for regular negotiation) and
+  ``--data-ciphers-fallback`` (for a fallback option when the
+  negotiation cannot take place because the other peer is old or
+  has negotiation disabled).
+
+  To see ciphers that are available with OpenVPN, use the
   ``--show-ciphers`` option.
 
   Set ``alg`` to :code:`none` to disable encryption.

src/openvpn/options.c

@@ -3120,26 +3120,20 @@ options_postprocess_cipher(struct options *o)
         /* We still need to set the ciphername to BF-CBC since various other
          * parts of OpenVPN assert that the ciphername is set */
         o->ciphername = "BF-CBC";
+
+        msg(M_INFO, "Note: --cipher is not set. OpenVPN versions before 2.6 "
+                    "defaulted to BF-CBC as fallback when cipher negotiation "
+                    "failed in this case. If you need this fallback please add "
+                    "'--data-ciphers-fallback 'BF-CBC' to your configuration "
+                    "and/or add BF-CBC to --data-ciphers.");
     }
     else if (!o->enable_ncp_fallback
              && !tls_item_in_cipher_list(o->ciphername, o->ncp_ciphers))
     {
-        msg(M_WARN, "DEPRECATED OPTION: --cipher set to '%s' but missing in"
-            " --data-ciphers (%s). Future OpenVPN version will "
-            "ignore --cipher for cipher negotiations. "
-            "Add '%s' to --data-ciphers or change --cipher '%s' to "
-            "--data-ciphers-fallback '%s' to silence this warning.",
-            o->ciphername, o->ncp_ciphers, o->ciphername,
-            o->ciphername, o->ciphername);
-        o->enable_ncp_fallback = true;
-
-        /* Append the --cipher to ncp_ciphers to allow it in NCP */
-        size_t newlen = strlen(o->ncp_ciphers) + 1 + strlen(o->ciphername) + 1;
-        char *ncp_ciphers = gc_malloc(newlen, false, &o->gc);
-
-        ASSERT(openvpn_snprintf(ncp_ciphers, newlen, "%s:%s", o->ncp_ciphers,
-                                o->ciphername));
-        o->ncp_ciphers = ncp_ciphers;
+        msg(M_WARN, "DEPRECATED OPTION: --cipher set to '%s' but missing in "
+            "--data-ciphers (%s). OpenVPN ignores --cipher for cipher "
+            "negotiations. ",
+            o->ciphername, o->ncp_ciphers);
     }
 }
 
@@ -3170,6 +3164,18 @@ need_compatibility_before(const struct options *o, unsigned int version)
 static void
 options_set_backwards_compatible_options(struct options *o)
 {
+    /* Versions < 2.5.0 do need --cipher in the list of accepted ciphers.
+     * Version 2.4 might probably does not need it but NCP was not so
+     * good with 2.4 and ncp-disable might be more common on 2.4 peers.
+     * Only do this iif --cipher is not explicitly (BF-CBC). This is not
+     * 100% correct backwards compatible behaviour but 2.5 already behaved like
+     * this */
+    if (o->ciphername && need_compatibility_before(o, 20500)
+        && !tls_item_in_cipher_list(o->ciphername, o->ncp_ciphers))
+    {
+        append_cipher_to_ncp_list(o, o->ciphername);
+    }
+
     /* Compression is deprecated and we do not want to announce support for it
      * by default anymore, additionally DCO breaks with compression.
      *

src/openvpn/ssl_ncp.c

@@ -172,6 +172,19 @@ mutate_ncp_cipher_list(const char *list, struct gc_arena *gc)
     return ret;
 }
 
+
+void
+append_cipher_to_ncp_list(struct options *o, const char *ciphername)
+{
+    /* Append the --cipher to ncp_ciphers to allow it in NCP */
+    size_t newlen = strlen(o->ncp_ciphers) + 1 + strlen(ciphername) + 1;
+    char *ncp_ciphers = gc_malloc(newlen, false, &o->gc);
+
+    ASSERT(openvpn_snprintf(ncp_ciphers, newlen, "%s:%s", o->ncp_ciphers,
+                            ciphername));
+    o->ncp_ciphers = ncp_ciphers;
+}
+
 bool
 tls_item_in_cipher_list(const char *item, const char *list)
 {

src/openvpn/ssl_ncp.h

@@ -102,6 +102,14 @@ tls_peer_ncp_list(const char *peer_info, struct gc_arena *gc);
 char *
 mutate_ncp_cipher_list(const char *list, struct gc_arena *gc);
 
+/**
+ * Appends the cipher specified by the ciphernamer parameter to to
+ * the o->ncp_ciphers list.
+ * @param o             options struct to modify. Its gc is also used
+ * @param ciphername    the ciphername to add
+ */
+void append_cipher_to_ncp_list(struct options *o, const char *ciphername);
+
 /**
  * Return true iff item is present in the colon-separated zero-terminated
  * cipher list.