Re: [Openvpn-devel] [PATCH v2] Allow running a default configuration with TLS libraries without BF-CBC

2021-02-14 Thread Gert Doering
Hi,

On Mon, Jan 25, 2021 at 01:43:30PM +0100, Arne Schwabe wrote:
> Modern TLS libraries might drop Blowfish by default or distributions
> might disable Blowfish in OpenSSL/mbed TLS. We still signal OCC
> options with BF-CBC compatible strings. To avoid requiring BF-CBC
> for this, special this one usage of BF-CBC enough to avoid a hard
> requirement on Blowfish in the default configuration.

I was about to merge this, based on Antonio's ACK, but this part of
the code confuses me:

> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> index b81137cf..d52057cc 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -3836,18 +3856,32 @@ options_string(const struct options *o,
> + (TLS_SERVER == true)
> <= 1);
>  
> -init_key_type(, o->ciphername, o->authname, o->keysize, true,
> -  false);
> +/* Skip resolving BF-CBC to allow SSL libraries without BF-CBC
> + * to work here in the default configuration */
> +const char *ciphername = o->ciphername;
> +int keysize;
> +
> +if (strcmp(o->ciphername, "BF-CBC") == 0) {
> +init_key_type(, "none", o->authname, o->keysize, true,
> +  false);
> +ciphername = cipher_kt_name(kt.cipher);
> +keysize = 128;
> +}
> +else
> +{
> +init_key_type(, o->ciphername, o->authname, o->keysize, true,
> +  false);
> +keysize = kt.cipher_length * 8;
> +}
>  /* Only announce the cipher to our peer if we are willing to
>   * support it */
> -const char *ciphername = cipher_kt_name(kt.cipher);

So the old code always sends "cipher_kt_name(kt.cipher)".

The new code adds a special case handling for "BF-CBC", calling
init_key_type(none), but then does the "cipher_kt_name()" only for
the "BF-CBC/none" case, no more for the "all other ciphers".

This looks like the wrong way around - shouldn't it do the

> +ciphername = cipher_kt_name(kt.cipher);

for the "not BF-CBC" case, and "ciphername = o->cipher" for "only BF-CBC"?


Call me confused...

As a side note, it seems to fail two of my t_client test cases, 
namely "talking to a 2.3 server with --cipher BF-CBC" and "talking to 
a 2.4 server with --ncp-disable", so maybe that code needs some more 
discussion.  I have not investigated more into these failures, first 
want to understand what the code tries to do.

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 g...@greenie.muc.de


signature.asc
Description: PGP signature
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH v2] Allow running a default configuration with TLS libraries without BF-CBC

2021-01-29 Thread Antonio Quartulli
Hi,

On 25/01/2021 13:43, Arne Schwabe wrote:
> Modern TLS libraries might drop Blowfish by default or distributions
> might disable Blowfish in OpenSSL/mbed TLS. We still signal OCC
> options with BF-CBC compatible strings. To avoid requiring BF-CBC
> for this, special this one usage of BF-CBC enough to avoid a hard
> requirement on Blowfish in the default configuration.
> 
> Signed-off-by: Arne Schwabe 
> 
> Patch v2: add more clarifying comment, do not warn about OCC only insecure
>   ciphers, code improvements
> 
> Signed-off-by: Arne Schwabe 

Acked-by: Antonio Quartulli 

-- 
Antonio Quartulli


___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


[Openvpn-devel] [PATCH v2] Allow running a default configuration with TLS libraries without BF-CBC

2021-01-25 Thread Arne Schwabe
Modern TLS libraries might drop Blowfish by default or distributions
might disable Blowfish in OpenSSL/mbed TLS. We still signal OCC
options with BF-CBC compatible strings. To avoid requiring BF-CBC
for this, special this one usage of BF-CBC enough to avoid a hard
requirement on Blowfish in the default configuration.

Signed-off-by: Arne Schwabe 

Patch v2: add more clarifying comment, do not warn about OCC only insecure
  ciphers, code improvements

Signed-off-by: Arne Schwabe 
---
 src/openvpn/init.c| 32 ++
 src/openvpn/options.c | 46 +--
 2 files changed, 64 insertions(+), 14 deletions(-)

diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index c3493c42..df0c7ebc 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -2770,14 +2770,30 @@ do_init_crypto_tls_c1(struct context *c)
 #endif /* if P2MP */
 }
 
-/* Do not warn if we only have BF-CBC in options->ciphername
- * because it is still the default cipher */
-bool warn = !streq(options->ciphername, "BF-CBC")
- || options->enable_ncp_fallback;
-/* Get cipher & hash algorithms */
-init_key_type(>c1.ks.key_type, options->ciphername, 
options->authname,
-  options->keysize, true, warn);
-
+   /*
+* BF-CBC is allowed to be used only when explicitly configured
+* as NCP-fallback or when NCP has been disabled.
+* In all other cases don't attempt to initialize BF-CBC as it
+* may not even be supported by the underlying SSL library.
+*
+* Therefore, the key structure has to be initialized when:
+* - any non-BF-CBC cipher was selected; or
+* - BF-CBC is selected and NCP is disabled (explicit request to
+*   use the BF-CBC cipher); or
+* - BF-CBC is selected, NCP is enabled and fallback is enabled
+*   (BF-CBC will be the fallback).
+*
+* Note that BF-CBC will still be part of the OCC string to retain
+* backwards compatibility with older clients.
+*/
+if (!streq(options->ciphername, "BF-CBC") || !options->ncp_enabled
+|| options->enable_ncp_fallback)
+{
+/* Do not warn if the if the cipher is used only in OCC */
+bool warn = !options->ncp_enabled || options->enable_ncp_fallback;
+init_key_type(>c1.ks.key_type, options->ciphername, 
options->authname,
+  options->keysize, true, warn);
+}
 /* Initialize PRNG with config-specified digest */
 prng_init(options->prng_hash, options->prng_nonce_secret_len);
 
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index b81137cf..d52057cc 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -3664,9 +3664,29 @@ calc_options_string_link_mtu(const struct options *o, 
const struct frame *frame)
 {
 struct frame fake_frame = *frame;
 struct key_type fake_kt;
-init_key_type(_kt, o->ciphername, o->authname, o->keysize, true,
-  false);
+
 frame_remove_from_extra_frame(_frame, crypto_max_overhead());
+
+
+/* o->ciphername might be BF-CBC even though the underlying SSL library
+ * does not support it. For this reason we workaround this corner case
+ * by pretending to have no encryption enabled and by manually adding
+ * the required packet overhead to the MTU computation.
+ */
+const char* ciphername = o->ciphername;
+
+if (strcmp(o->ciphername, "BF-CBC") == 0)
+{
+/* none has no overhead, so use this to later add only --auth
+ * overhead */
+
+/* overhead of BF-CBC: 64 bit block size, 64 bit IV size */
+frame_add_to_extra_frame(_frame, 64/8 + 64/8);
+}
+
+init_key_type(_kt, ciphername, o->authname, o->keysize, true,
+  false);
+
 crypto_adjust_frame_parameters(_frame, _kt, o->replay,
cipher_kt_mode_ofb_cfb(fake_kt.cipher));
 frame_finalize(_frame, o->ce.link_mtu_defined, o->ce.link_mtu,
@@ -3836,18 +3856,32 @@ options_string(const struct options *o,
+ (TLS_SERVER == true)
<= 1);
 
-init_key_type(, o->ciphername, o->authname, o->keysize, true,
-  false);
+/* Skip resolving BF-CBC to allow SSL libraries without BF-CBC
+ * to work here in the default configuration */
+const char *ciphername = o->ciphername;
+int keysize;
+
+if (strcmp(o->ciphername, "BF-CBC") == 0) {
+init_key_type(, "none", o->authname, o->keysize, true,
+  false);
+ciphername = cipher_kt_name(kt.cipher);
+keysize = 128;
+}
+else
+{
+init_key_type(, o->ciphername, o->authname, o->keysize, true,
+