From 65f6da8eeb84fbcea357765e13fa73d0169a143c Mon Sep 17 00:00:00 2001 From: Antonio Quartulli Date: Sat, 4 Sep 2021 11:56:26 +0200 Subject: [PATCH] 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 Signed-off-by: Antonio Quartulli Acked-by: Arne Schwabe 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 --- Changes.rst | 12 ++++++++ doc/man-sections/generic-options.rst | 2 ++ doc/man-sections/protocol-options.rst | 42 ++++++++++++++------------- src/openvpn/options.c | 38 ++++++++++++++---------- src/openvpn/ssl_ncp.c | 13 +++++++++ src/openvpn/ssl_ncp.h | 8 +++++ 6 files changed, 79 insertions(+), 36 deletions(-) diff --git a/Changes.rst b/Changes.rst index 8854cc3e..2393e31d 100644 --- a/Changes.rst +++ b/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. diff --git a/doc/man-sections/generic-options.rst b/doc/man-sections/generic-options.rst index a8d24572..8b26cd1a 100644 --- a/doc/man-sections/generic-options.rst +++ b/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 diff --git a/doc/man-sections/protocol-options.rst b/doc/man-sections/protocol-options.rst index 3ae09a56..4125b263 100644 --- a/doc/man-sections/protocol-options.rst +++ b/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. diff --git a/src/openvpn/options.c b/src/openvpn/options.c index 26305a90..4085502f 100644 --- a/src/openvpn/options.c +++ b/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. * diff --git a/src/openvpn/ssl_ncp.c b/src/openvpn/ssl_ncp.c index 6967e2bb..022a9dc3 100644 --- a/src/openvpn/ssl_ncp.c +++ b/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) { diff --git a/src/openvpn/ssl_ncp.h b/src/openvpn/ssl_ncp.h index 4a2601a2..09ddeb28 100644 --- a/src/openvpn/ssl_ncp.h +++ b/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.