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(&kt, 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(&kt, "none", o->authname, o->keysize, true,
> +                          false);
> +            ciphername = cipher_kt_name(kt.cipher);
> +            keysize = 128;
> +        }
> +        else
> +        {
> +            init_key_type(&kt, 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

Attachment: signature.asc
Description: PGP signature

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

Reply via email to