[Openvpn-devel,v2] Default to --cipher BF-CBC if not set and compat-mode < 2.4.0

Message ID 20211105150742.2909443-1-arne@rfc2549.org
State Accepted
Headers show
Series
  • [Openvpn-devel,v2] Default to --cipher BF-CBC if not set and compat-mode < 2.4.0
Related show

Commit Message

Arne Schwabe Nov. 5, 2021, 3:07 p.m.
When we try to make a configuration compatible to a version earlier
than 2.4.0 we probably need to have a --cipher configured since NCP
is not available. In configuration where --cipher is not specified
we default to BF-CBC to support these old clients.

Note that with OpenSSL 3.0 you will also need to enable the legacy
provider otherwise we bail out since BF-CBC is no longer supported.

Also move the condition so BF-CBC gets included in the data-ciphers
list.

Patch v2: move the comment to a better place.

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 src/openvpn/options.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

Comments

Antonio Quartulli Feb. 4, 2022, 4:51 p.m. | #1
Hi,

On 05/11/2021 16:07, Arne Schwabe wrote:
> When we try to make a configuration compatible to a version earlier
> than 2.4.0 we probably need to have a --cipher configured since NCP
> is not available. In configuration where --cipher is not specified
> we default to BF-CBC to support these old clients.
> 
> Note that with OpenSSL 3.0 you will also need to enable the legacy
> provider otherwise we bail out since BF-CBC is no longer supported.
> 
> Also move the condition so BF-CBC gets included in the data-ciphers
> list.
> 
> Patch v2: move the comment to a better place.
> 
> Signed-off-by: Arne Schwabe <arne@rfc2549.org>

Reviewed-by: Antonio Quartulli <a@unstable.cc>

Unfortunately I cannot fully ACK this patch because I'd need to compile 
2.3 to run a test, but this turned to be a mission impossible (due to 
OpenSSL compatibility issues).

The change makes sense and it should do what we expect.

If anybody wants to test against an openvpn2.3 peer is most welcome.

Cheers,
Arne Schwabe Feb. 4, 2022, 7:12 p.m. | #2
Am 04.02.22 um 17:51 schrieb Antonio Quartulli:
> Hi,
> 
> On 05/11/2021 16:07, Arne Schwabe wrote:
>> When we try to make a configuration compatible to a version earlier
>> than 2.4.0 we probably need to have a --cipher configured since NCP
>> is not available. In configuration where --cipher is not specified
>> we default to BF-CBC to support these old clients.
>>
>> Note that with OpenSSL 3.0 you will also need to enable the legacy
>> provider otherwise we bail out since BF-CBC is no longer supported.
>>
>> Also move the condition so BF-CBC gets included in the data-ciphers
>> list.
>>
>> Patch v2: move the comment to a better place.
>>
>> Signed-off-by: Arne Schwabe <arne@rfc2549.org>
> 
> Reviewed-by: Antonio Quartulli <a@unstable.cc>
> 
> Unfortunately I cannot fully ACK this patch because I'd need to compile 
> 2.3 to run a test, but this turned to be a mission impossible (due to 
> OpenSSL compatibility issues).
> 
> The change makes sense and it should do what we expect.
> 
> If anybody wants to test against an openvpn2.3 peer is most welcome.

A 2.4 client with ncp-disable should work nicely as well. We just do not 
make ourselves compatible with 2.4.0 with ncp-disable if you select 
2.4.0, you need 2.3.0 for that.

Arne
Antonio Quartulli Feb. 7, 2022, 8:49 a.m. | #3
Hi,

On 04/02/2022 20:12, Arne Schwabe wrote:
> A 2.4 client with ncp-disable should work nicely as well. We just do not 
> make ourselves compatible with 2.4.0 with ncp-disable if you select 
> 2.4.0, you need 2.3.0 for that.

Thanks for the hint!

Went the lazy route and tested against 2.4.11 with --ncp-disable.

Server: 2.4.11 with --ncp-disable (to mimic 2.3.x) and no --cipher 
specified (BF-CBC is therefore the default).

Client: master + this patch and no --cipher specified.

* First attempt: wrong cipher negotiated and connection breaks.
* Second attempt: add --compat-mode 2.3.0 to client (to exercise the 
code implemented by this patch) and I could happily connect to the 
2.3.x-like server using BF-CBC.



Acked-by: Antonio Quartulli <a@unstable.cc>
Gert Doering Feb. 13, 2022, 2:10 p.m. | #4
I have not given this much testing, as Antonio has tested the relevant
combinations.  Code looks good.  Client-tested.

Your patch has been applied to the master branch.

commit fd72a3b99dcb110953d8466bfe6c47dab3a29657
Author: Arne Schwabe
Date:   Fri Nov 5 16:07:42 2021 +0100

     Default to --cipher BF-CBC if not set and compat-mode < 2.4.0

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


--
kind regards,

Gert Doering

Patch

diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 4dc70e4f3..6751084af 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -3186,6 +3186,19 @@  options_set_backwards_compatible_options(struct options *o)
         }
     }
 
+    if (need_compatibility_before(o, 20400))
+    {
+        if (!o->ciphername)
+        {
+            /* If ciphername is not set default to BF-CBC when targeting these
+             * old versions that do not have NCP */
+            o->ciphername = "BF-CBC";
+        }
+        /* Versions < 2.4.0 additionally might be compiled with --enable-small and
+         * not have OCC strings required for "poor man's NCP" */
+        o->enable_ncp_fallback = true;
+    }
+
     /* 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.
@@ -3198,13 +3211,6 @@  options_set_backwards_compatible_options(struct options *o)
         append_cipher_to_ncp_list(o, o->ciphername);
     }
 
-    /* Versions < 2.4.0 additionally might be compiled with --enable-small and
-     * not have OCC strings required for "poor man's NCP" */
-    if (o->ciphername && need_compatibility_before(o, 20400))
-    {
-        o->enable_ncp_fallback = true;
-    }
-
 #ifdef USE_COMP
     /* Compression is deprecated and we do not want to announce support for it
      * by default anymore, additionally DCO breaks with compression.