openvpn-devel — OpenVPN developers list

Re: [Openvpn-devel] [PATCH] Improve error msg when all TAP adapters are in use "or disabled"

[Selva N. <sel...@gm...>]

Hi,

This looks good but can we do better? We don't check the error
(GetLastError()) after the CreateFile() failure -- can we determine
whether the error was due to permissions, busy file (in use) or
disabled device and print out a more specific error message? I'm not
sure what errors are triggered by CreateFile, so just wondering..

Selva

On Thu, Aug 6, 2020 at 3:02 PM Richard Bonhomme <tin...@gm...> wrote:
>
> Ref: https://github.com/OpenVPN/openvpn-gui/issues/356
>
> Signed-off-by: Richard Bonhomme <tin...@gm...>
> ---
>  src/openvpn/tun.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
> index cc7b65cf..44ca8450 100644
> --- a/src/openvpn/tun.c
> +++ b/src/openvpn/tun.c
> @@ -6436,7 +6436,7 @@ tun_open_device(struct tuntap *tt, const char *dev_node, const char **device_gui
>
>              if (!*device_guid)
>              {
> -                msg(M_FATAL, "All %s adapters on this system are currently in use.", print_windows_driver(tt->windows_driver));
> +                msg(M_FATAL, "All %s adapters on this system are currently in use or disabled.", print_windows_driver(tt->windows_driver));
>              }
>
>              if (tt->windows_driver != windows_driver)
> --
> 2.17.1
>
>
>
> _______________________________________________
> Openvpn-devel mailing list
> Ope...@li...
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [PATCH] Improve error msg when all TAP adapters are in use "or disabled"

[Richard B. <tin...@gm...>]

Ref: https://github.com/OpenVPN/openvpn-gui/issues/356

Signed-off-by: Richard Bonhomme <tin...@gm...>
---
 src/openvpn/tun.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
index cc7b65cf..44ca8450 100644
--- a/src/openvpn/tun.c
+++ b/src/openvpn/tun.c
@@ -6436,7 +6436,7 @@ tun_open_device(struct tuntap *tt, const char *dev_node, const char **device_gui
 
             if (!*device_guid)
             {
-                msg(M_FATAL, "All %s adapters on this system are currently in use.", print_windows_driver(tt->windows_driver));
+                msg(M_FATAL, "All %s adapters on this system are currently in use or disabled.", print_windows_driver(tt->windows_driver));
             }
 
             if (tt->windows_driver != windows_driver)
-- 
2.17.1

Re: [Openvpn-devel] [PATCH v2] Rework NCP compability logic and drop BF-CBC support by default

[tincanteksup <tin...@gm...>]

On 05/08/2020 21:25, Steffan Karger wrote:
> Hi,
> 
> No full review yet, but this version does seem to address my previous
> comments. Some minor nits I noticed on my first run through v2:
> 
> On 29-07-2020 13:38, Arne Schwabe wrote:
>> This reworks the NCP logic to be more strict about what is
>> considered an acceptable result of an NCP negotiation. It also
>> us to finally drop BF-CBC support by default.
>>
>> All new behaviour is currently limited to server/client
>> mode with pull enabled. P2p mode without pull does not change.
>>
>> New Server behaviour:
>> - when a client announces its supported ciphers through either
>>    OCC or IV_CIPHER/IV_NCP we reject the client with a
>>    AUTH_FAILED message if we have no common cipher.
>>
>> - When a client does not announce any cipher in either
>>    OCC or NCP we by reject it unless fallback-cipher is
>>    specified in either ccd or config.
>>
>> New client behaviour:
>> - When no cipher is pushed (or a cipher we refused to support)
>>    and we also cannot support the server's server announced in
>>    OCC we fail the connection and log why
>>
>> - If fallback-cipher is specified we will in the case that
>>    cipher is missing from occ use the fallback cipher instead
>>    of failing the connection
>>
>> Both client and server behaviour:
>> - We only announce --cipher xyz in occ if we are willing
>>    to support that cipher.
>>
>>    It means that we only announce the fallback-cipher if
>>    it is also contained in --data-ciphers
>>
>> Compatiblity behaviour:

Compatiblity -> Compatibility


>>
>> In 2.5 both client and server will automatically set
>> fallback-cipher xyz if --cipher xyz is in the config and
>> also add append the cipher to the end of data-ciphers.
>>
>> We log a warn user about this and point to --data-ciphers and
>> --fallback-cipher. This also happens if the configuration
>> contains an explicit --cipher BF-CBC.
>>
>> If --cipher is not set, we only warn that previous versions
>> allowed BF-CBC and point how to reenable BF-CBC. This might
>> break very few config where someone connects a very old
>> client to a 2.5 server but at some point we need to drop
>> the BF-CBC and those affected use will already have a the
>> scary SWEET32 warning in their logs.
>>
>> In short: If --cipher is explicitly set 2.6 will work the same as
>> 2.4 did. When --cipher is not set, BF-CBC support is dropped and
>> we warn about it.
>>
>> Examples how breaking the default BF-CBC will be logged:
>>
>> Client side:
>>   - Client connecting to server that does not push cipher but
>>     has --cipher in OCC
>>
>>      OPTIONS ERROR: failed to negotiate cipher with server.  Add the server's cipher ('BF-CBC') to --data-ciphers (currently 'AES-256-GCM:AES-128-CBC') if you want to connect to this server.
>>
>>   - Client connecting to a server that does not support OCC:
>>
>>     OPTIONS ERROR: failed to negotioate cipher with server. Configure --fallback-cipher if you want connect to this server.

negotioate -> *-)



>>
>> Server Side:
>>
>> - Server has a client only supporting BF-CBC connecting:
>>
>>    styx/IP PUSH: No common cipher between server and client. Server data-ciphers: 'CHACHA20-POLY1305:AES-128-GCM:AES-256-GCM:AES-256-CBC:AES-128-CBC', client supports cipher 'BF-CBC'.
>>
>>   - Client without OCC:
>>
>>     styx/IP PUSH:No NCP or OCC cipher data received from peer.
>>     styx/IP Use --fallback-cipher with the cipher the client is using if you want to allow the client to connect
>>
>> In all reject cases on the client:
>>
>>     AUTH: Received control message: AUTH_FAILED,Data channel cipher negotiation failed (no shared cipher)
>>
>> Signed-off-by: Arne Schwabe <ar...@rf...>
>>
>> Patch V2: rename fallback-cipher to data-ciphers-fallback
>>            add all corrections from Steffan
>>            Ignore occ cipher for clients sending IV_CIPHERS
>>            move client side ncp in its own function
>>            do not print INSECURE cipher warning if BF-CBC is not allowed
>>
>> Signed-off-by: Arne Schwabe <ar...@rf...>
>> ---
>>   doc/man-sections/protocol-options.rst |  22 ++++-
>>   src/openvpn/crypto.c                  |   4 +-
>>   src/openvpn/init.c                    |  18 ++--
>>   src/openvpn/multi.c                   | 135 ++++++++++++++++----------
>>   src/openvpn/options.c                 | 117 +++++++++++++++++-----
>>   src/openvpn/options.h                 |   2 +
>>   src/openvpn/ssl.c                     |  17 ++--
>>   src/openvpn/ssl_ncp.c                 |  82 +++++++++++++---
>>   src/openvpn/ssl_ncp.h                 |  18 ++--
>>   tests/unit_tests/openvpn/test_ncp.c   |  26 +++--
>>   10 files changed, 311 insertions(+), 130 deletions(-)


No other spelling or grammar to worry about.













>>
>> diff --git a/doc/man-sections/protocol-options.rst b/doc/man-sections/protocol-options.rst
>> index 051f1d32..69d3bc67 100644
>> --- a/doc/man-sections/protocol-options.rst
>> +++ b/doc/man-sections/protocol-options.rst
>> @@ -57,6 +57,9 @@ 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 and ``--data-ciphers``
>> +  or rarely `--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
>> @@ -183,8 +186,9 @@ configured in a compatible way between both the local and remote side.
>>     ``--server`` ), or if ``--pull`` is specified (client-side, implied by
>>     setting --client).
>>   
>> -  If both peers support and do not disable NCP, the negotiated cipher will
>> -  override the cipher specified by ``--cipher``.
>> +  If no common cipher is found during cipher negotiation, the  connection
>> +  is terminated. To support old clients/server that do not provide any cipher
>> +  negotiation support see ``data-ciphers-fallback``.
>>   
>>     Additionally, to allow for more smooth transition, if NCP is enabled,
>>     OpenVPN will inherit the cipher of the peer if that cipher is different
>> @@ -201,8 +205,18 @@ configured in a compatible way between both the local and remote side.
>>     This list is restricted to be 127 chars long after conversion to OpenVPN
>>     ciphers.
>>   
>> -  This option was called ``ncp-ciphers`` in OpenVPN 2.4 but has been renamed
>> -  to ``data-ciphers`` in OpenVPN 2.5 to more accurately reflect its meaning.
>> +  This option was called ``--ncp-ciphers`` in OpenVPN 2.4 but has been renamed
>> +  to ``--data-ciphers`` in OpenVPN 2.5 to more accurately reflect its meaning.
>> +
>> +--data-ciphers-fallback alg
>> +
>> +    Configure a cipher that is used to fall back to if we could not determine
>> +    which cipher the peer is willing to use.
>> +
>> +    This option should only be needed to
>> +    connect to peers that are running OpenVPN 2.3 and older version, and
>> +    have been configured with `--enable-small`
>> +    (typically used on routers or other embedded devices).
>>   
>>   --ncp-disable
>>     Disable "Negotiable Crypto Parameters". This completely disables cipher
>> diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
>> index e92a0dc1..3a0bfbec 100644
>> --- a/src/openvpn/crypto.c
>> +++ b/src/openvpn/crypto.c
>> @@ -727,7 +727,9 @@ warn_insecure_key_type(const char *ciphername, const cipher_kt_t *cipher)
>>       {
>>           msg(M_WARN, "WARNING: INSECURE cipher (%s) with block size less than 128"
>>               " bit (%d bit).  This allows attacks like SWEET32.  Mitigate by "
>> -            "using a --cipher with a larger block size (e.g. AES-256-CBC).",
>> +            "using a --cipher with a larger block size (e.g. AES-256-CBC). "
>> +            "Support for these insecure ciphers will be removed in "
>> +            "OpenVPN 2.6.",
>>               ciphername, cipher_kt_block_size(cipher)*8);
>>       }
>>   }
>> diff --git a/src/openvpn/init.c b/src/openvpn/init.c
>> index 1ea4735d..402d2652 100644
>> --- a/src/openvpn/init.c
>> +++ b/src/openvpn/init.c
>> @@ -2365,16 +2365,9 @@ do_deferred_options(struct context *c, const unsigned int found)
>>       /* process (potentially pushed) crypto options */
>>       if (c->options.pull)
>>       {
>> -        struct tls_session *session = &c->c2.tls_multi->session[TM_ACTIVE];
>> -        if (found & OPT_P_NCP)
>> -        {
>> -            msg(D_PUSH, "OPTIONS IMPORT: data channel crypto options modified");
>> -        }
>> -        else if (c->options.ncp_enabled)
>> +        if (!check_pull_client_ncp(c, found))
>>           {
>> -            /* If the server did not push a --cipher, we will switch to the
>> -             * remote cipher if it is in our ncp-ciphers list */
>> -            tls_poor_mans_ncp(&c->options, c->c2.tls_multi->remote_ciphername);
>> +            return false;
>>           }
>>           struct frame *frame_fragment = NULL;
>>   #ifdef ENABLE_FRAGMENT
>> @@ -2384,6 +2377,7 @@ do_deferred_options(struct context *c, const unsigned int found)
>>           }
>>   #endif
>>   
>> +        struct tls_session *session = &c->c2.tls_multi->session[TM_ACTIVE];
>>           if (!tls_session_update_crypto_params(session, &c->options, &c->c2.frame,
>>                                                 frame_fragment))
>>           {
>> @@ -2757,9 +2751,13 @@ do_init_crypto_tls_c1(struct context *c)
>>   #endif /* if P2MP */
>>           }
>>   
>> +        /* Do not warn if 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(&c->c1.ks.key_type, options->ciphername, options->authname,
>> -                      options->keysize, true, true);
>> +                      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/multi.c b/src/openvpn/multi.c
>> index 0f9c586b..79b5c0c3 100644
>> --- a/src/openvpn/multi.c
>> +++ b/src/openvpn/multi.c
>> @@ -1777,7 +1777,7 @@ multi_client_connect_setenv(struct multi_context *m,
>>    * - choosen cipher
>>    * - peer id
>>    */
>> -static void
>> +static bool
>>   multi_client_set_protocol_options(struct context *c)
>>   {
>>   
>> @@ -1807,56 +1807,85 @@ multi_client_set_protocol_options(struct context *c)
>>       }
>>   
>>       /* Select cipher if client supports Negotiable Crypto Parameters */
>> -    if (o->ncp_enabled)
>> +    if (!o->ncp_enabled)
>>       {
>> -        /* if we have already created our key, we cannot *change* our own
>> -         * cipher -> so log the fact and push the "what we have now" cipher
>> -         * (so the client is always told what we expect it to use)
>> -         */
>> -        const struct tls_session *session = &tls_multi->session[TM_ACTIVE];
>> -        if (session->key[KS_PRIMARY].crypto_options.key_ctx_bi.initialized)
>> +        return true;
>> +    }
>> +
>> +    /* if we have already created our key, we cannot *change* our own
>> +     * cipher -> so log the fact and push the "what we have now" cipher
>> +     * (so the client is always told what we expect it to use)
>> +     */
>> +    const struct tls_session *session = &tls_multi->session[TM_ACTIVE];
>> +    if (session->key[KS_PRIMARY].crypto_options.key_ctx_bi.initialized)
>> +    {
>> +        msg( M_INFO, "PUSH: client wants to negotiate cipher (NCP), but "
> 
> No space after ( .
> 
>> +             "server has already generated data channel keys, "
>> +             "re-sending previously negotiated cipher '%s'",
>> +             o->ciphername );
>> +        return true;
>> +    }
>> +
>> +    /*
>> +     * Push the first cipher from --data-ciphers to the client that
>> +     * the client announces to be supporting.
>> +     */
>> +    char *push_cipher = ncp_get_best_cipher(o->ncp_ciphers, peer_info,
>> +                                            tls_multi->remote_ciphername,
>> +                                            &o->gc);
>> +
>> +    if (push_cipher)
>> +    {
>> +        o->ciphername = push_cipher;
>> +        return true;
>> +    }
>> +
>> +    /* NCP cipher negotiation failed. Try to figure out why exactly it
>> +     * failed and give good error messages and potentially do a fallback
>> +     * for non NCP clients */
>> +    struct gc_arena gc = gc_new();
>> +    bool ret = false;
>> +
>> +    const char *peer_ciphers = tls_peer_ncp_list(peer_info, &gc);
>> +    /* If we are in a situation where we know the client ciphers, there is no
>> +     * reason to fall back to a cipher that will not be accepted by the other
>> +     * side, in this situation we fail the auth*/
>> +    if (strlen(peer_ciphers) > 0)
>> +    {
>> +        msg(M_INFO, "PUSH: No common cipher between server and client. "
>> +            "Server data-ciphers: '%s', client supported ciphers '%s'",
>> +            o->ncp_ciphers, peer_ciphers);
>> +    }
>> +    else if (tls_multi->remote_ciphername)
>> +    {
>> +        msg(M_INFO, "PUSH: No common cipher between server and client. "
>> +            "Server data-ciphers: '%s', client supports cipher '%s'",
>> +            o->ncp_ciphers, tls_multi->remote_ciphername);
>> +    }
>> +    else
>> +    {
>> +        msg(M_INFO, "PUSH: No NCP or OCC cipher data received from peer.");
>> +
>> +        if (o->enable_ncp_fallback && !tls_multi->remote_ciphername)
>>           {
>> -            msg( M_INFO, "PUSH: client wants to negotiate cipher (NCP), but "
>> -                 "server has already generated data channel keys, "
>> -                 "re-sending previously negotiated cipher '%s'",
>> -                 o->ciphername );
>> +            msg(M_INFO, "Using data channel cipher '%s' since "
>> +                "--data-ciphers-fallback is set.", o->ciphername);
>> +            ret = true;
>>           }
>>           else
>>           {
>> -            /*
>> -             * Push the first cipher from --data-ciphers to the client that
>> -             * the client announces to be supporting.
>> -             */
>> -            char *push_cipher = ncp_get_best_cipher(o->ncp_ciphers, o->ciphername,
>> -                                                    peer_info,
>> -                                                    tls_multi->remote_ciphername,
>> -                                                    &o->gc);
>> -
>> -            if (push_cipher)
>> -            {
>> -                o->ciphername = push_cipher;
>> -            }
>> -            else
>> -            {
>> -                struct gc_arena gc = gc_new();
>> -                const char *peer_ciphers = tls_peer_ncp_list(peer_info, &gc);
>> -                if (strlen(peer_ciphers) > 0)
>> -                {
>> -                    msg(M_INFO, "PUSH: No common cipher between server and "
>> -                        "client. Expect this connection not to work. Server "
>> -                        "data-ciphers: '%s', client supported ciphers '%s'",
>> -                        o->ncp_ciphers, peer_ciphers);
>> -                }
>> -                else
>> -                {
>> -                    msg(M_INFO, "No NCP data received from peer, falling back "
>> -                        "to --cipher '%s'. Peer reports in OCC --cipher '%s'",
>> -                        o->ciphername, np(tls_multi->remote_ciphername));
>> -                }
>> -                gc_free(&gc);
>> -            }
>> +            msg(M_INFO, "Use --data-ciphers-fallback with the cipher the "
>> +                "client is using if you want to allow the client to connect");
>>           }
>>       }
>> +    if (!ret)
>> +    {
>> +        auth_set_client_reason(tls_multi, "Data channel cipher negotiation "
>> +                                          "failed (no shared cipher)");
>> +    }
>> +
>> +    gc_free(&gc);
>> +    return ret;
>>   }
>>   
>>   /**
>> @@ -2322,7 +2351,7 @@ multi_client_connect_late_setup(struct multi_context *m,
>>       if (!mi->context.c2.push_ifconfig_defined)
>>       {
>>           msg(D_MULTI_ERRORS, "MULTI: no dynamic or static remote"
>> -            "--ifconfig address is available for %s",
>> +                            "--ifconfig address is available for %s",
>>               multi_instance_string(mi, false, &gc));
>>       }
>>   
>> @@ -2338,7 +2367,7 @@ multi_client_connect_late_setup(struct multi_context *m,
>>   
>>           /* JYFIXME -- this should cause the connection to fail */
>>           msg(D_MULTI_ERRORS, "MULTI ERROR: primary virtual IP for %s (%s)"
>> -            "violates tunnel network/netmask constraint (%s/%s)",
>> +                            "violates tunnel network/netmask constraint (%s/%s)",
>>               multi_instance_string(mi, false, &gc),
>>               print_in_addr_t(mi->context.c2.push_ifconfig_local, 0, &gc),
>>               ifconfig_constraint_network, ifconfig_constraint_netmask);
>> @@ -2387,7 +2416,7 @@ multi_client_connect_late_setup(struct multi_context *m,
>>       else if (mi->context.options.iroutes)
>>       {
>>           msg(D_MULTI_ERRORS, "MULTI: --iroute options rejected for %s -- iroute"
>> -            "only works with tun-style tunnels",
>> +                            "only works with tun-style tunnels",
>>               multi_instance_string(mi, false, &gc));
>>       }
>>   
>> @@ -2399,11 +2428,15 @@ multi_client_connect_late_setup(struct multi_context *m,
>>       mi->context.c2.context_auth = CAS_SUCCEEDED;
>>   
>>       /* authentication complete, calculate dynamic client specific options */
>> -    multi_client_set_protocol_options(&mi->context);
>> -
>> -    /* Generate data channel keys */
>> -    if (!multi_client_generate_tls_keys(&mi->context))
>> +    if (!multi_client_set_protocol_options(&mi->context))
>> +    {
>> +        mi->context.c2.context_auth = CAS_FAILED;
>> +    }
>> +    /* Generate data channel keys only if setting protocol options
>> +     * has not failed */
>> +    else if (!multi_client_generate_tls_keys(&mi->context))
>>       {
>> +
>>           mi->context.c2.context_auth = CAS_FAILED;
>>       }
>>   
>> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
>> index bc256b18..c53ef7f9 100644
>> --- a/src/openvpn/options.c
>> +++ b/src/openvpn/options.c
>> @@ -855,7 +855,6 @@ init_options(struct options *o, const bool init_gc)
>>   #if P2MP
>>       o->scheduled_exit_interval = 5;
>>   #endif
>> -    o->ciphername = "BF-CBC";
>>       o->ncp_enabled = true;
>>       o->ncp_ciphers = "AES-256-GCM:AES-128-GCM";
>>       o->authname = "SHA1";
>> @@ -2053,7 +2052,7 @@ options_postprocess_verify_ce(const struct options *options, const struct connec
>>       if (options->inetd)
>>       {
>>           msg(M_WARN, "DEPRECATED OPTION: --inetd mode is deprecated "
>> -                    "and will be removed in OpenVPN 2.6");
>> +            "and will be removed in OpenVPN 2.6");
>>       }
>>   
>>       if (options->lladdr && dev != DEV_TYPE_TAP)
>> @@ -3046,6 +3045,67 @@ options_postprocess_verify(const struct options *o)
>>       }
>>   }
>>   
>> +static void
>> +options_postprocess_cipher(struct options *o)
>> +{
>> +    if (!o->pull && !(o->mode == MODE_SERVER))
>> +    {
>> +        /* we are in the classic P2P mode */
>> +        o->ncp_enabled = false;
>> +        msg( M_WARN, "Cipher negotiation is disabled since neither "
>> +             "P2MP client nor server mode is enabled");
>> +
>> +        /* If the cipher is not set, use the old default ofo BF-CBC. We will
>> +         * warn that this is deprecated on cipher initialisation, no need
>> +         * to warn here as well */
>> +        if (!o->ciphername)
>> +        {
>> +            o->ciphername = "BF-CBC";
>> +        }
>> +        return;
>> +    }
>> +
>> +    /* pull or P2MP mode */
>> +    if (!o->ciphername)
>> +    {
>> +        if (!o->ncp_enabled)
>> +        {
>> +            msg(M_USAGE, "--ncp-disable needs an explicit --cipher or "
>> +                         "--data-ciphers-fallback config option");
>> +        }
>> +
>> +        msg(M_WARN, "--cipher is not set. Previous OpenVPN version 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.");
>> +
>> +        /* 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";
>> +    }
>> +    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;
> 
> Missing space after the last + .
> 
>> +        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;
>> +    }
>> +}
>> +
>>   static void
>>   options_postprocess_mutate(struct options *o)
>>   {
>> @@ -3058,6 +3118,7 @@ options_postprocess_mutate(struct options *o)
>>       helper_keepalive(o);
>>       helper_tcp_nodelay(o);
>>   
>> +    options_postprocess_cipher(o);
>>       options_postprocess_mutate_invariant(o);
>>   
>>       if (o->ncp_enabled)
>> @@ -3118,16 +3179,6 @@ options_postprocess_mutate(struct options *o)
>>               "include this in your server configuration");
>>           o->dh_file = NULL;
>>       }
>> -
>> -    /* cipher negotiation (NCP) currently assumes --pull or --mode server */
>> -    if (o->ncp_enabled
>> -        && !(o->pull || o->mode == MODE_SERVER) )
>> -    {
>> -        msg( M_WARN, "disabling NCP mode (--ncp-disable) because not "
>> -             "in P2MP client or server mode" );
>> -        o->ncp_enabled = false;
>> -    }
>> -
>>   #if ENABLE_MANAGEMENT
>>       if (o->http_proxy_override)
>>       {
>> @@ -3663,14 +3714,21 @@ options_string(const struct options *o,
>>        */
>>   
>>       buf_printf(&out, ",dev-type %s", dev_type_string(o->dev, o->dev_type));
>> -    buf_printf(&out, ",link-mtu %u", (unsigned int) calc_options_string_link_mtu(o, frame));
>> +    /* the link-mtu that we send has only a meaning if have a fixed
>> +     * cipher (p2p) or have a fallback cipher configured for older non
>> +     * ncp clients. But not sending it, will make even 2.4 complain
>> +     * about it missing. So still send it. */
>> +    buf_printf(&out, ",link-mtu %u",
>> +               (unsigned int) calc_options_string_link_mtu(o, frame));
>> +
>>       buf_printf(&out, ",tun-mtu %d", PAYLOAD_SIZE(frame));
>>       buf_printf(&out, ",proto %s",  proto_remote(o->ce.proto, remote));
>>   
>> +    bool p2p_nopull = o->mode == MODE_POINT_TO_POINT && !PULL_DEFINED(o);
>>       /* send tun_ipv6 only in peer2peer mode - in client/server mode, it
>>        * is usually pushed by the server, triggering a non-helpful warning
>>        */
>> -    if (o->ifconfig_ipv6_local && o->mode == MODE_POINT_TO_POINT && !PULL_DEFINED(o))
>> +    if (o->ifconfig_ipv6_local && p2p_nopull)
>>       {
>>           buf_printf(&out, ",tun-ipv6");
>>       }
>> @@ -3700,7 +3758,7 @@ options_string(const struct options *o,
>>           }
>>       }
>>   
>> -    if (tt && o->mode == MODE_POINT_TO_POINT && !PULL_DEFINED(o))
>> +    if (tt && p2p_nopull)
>>       {
>>           const char *ios = ifconfig_options_string(tt, remote, o->ifconfig_nowarn, gc);
>>           if (ios && strlen(ios))
>> @@ -3756,8 +3814,14 @@ options_string(const struct options *o,
>>   
>>           init_key_type(&kt, o->ciphername, o->authname, o->keysize, true,
>>                         false);
>> -
>> -        buf_printf(&out, ",cipher %s", cipher_kt_name(kt.cipher));
>> +        /* Only announce the cipher to our peer if we are willing to
>> +         * support it */
>> +        const char *ciphername = cipher_kt_name(kt.cipher);
>> +        if (p2p_nopull || !o->ncp_enabled
>> +            || tls_item_in_cipher_list(ciphername, o->ncp_ciphers))
>> +        {
>> +            buf_printf(&out, ",cipher %s", ciphername);
>> +        }
>>           buf_printf(&out, ",auth %s", md_kt_name(kt.digest));
>>           buf_printf(&out, ",keysize %d", kt.cipher_length * 8);
>>           if (o->shared_secret_file)
>> @@ -3875,7 +3939,8 @@ options_warning_safe_scan2(const int msglevel,
>>           || strprefix(p1, "keydir ")
>>           || strprefix(p1, "proto ")
>>           || strprefix(p1, "tls-auth ")
>> -        || strprefix(p1, "tun-ipv6"))
>> +        || strprefix(p1, "tun-ipv6")
>> +        || strprefix(p1, "cipher "))
>>       {
>>           return;
>>       }
>> @@ -7863,14 +7928,20 @@ add_option(struct options *options,
>>           VERIFY_PERMISSION(OPT_P_NCP|OPT_P_INSTANCE);
>>           options->ciphername = p[1];
>>       }
>> +    else if (streq(p[0], "data-ciphers-fallback") && p[1] && !p[2])
>> +    {
>> +        VERIFY_PERMISSION(OPT_P_GENERAL|OPT_P_INSTANCE);
>> +        options->ciphername = p[1];
>> +        options->enable_ncp_fallback = true;
>> +    }
>>       else if ((streq(p[0], "data-ciphers") || streq(p[0], "ncp-ciphers"))
>> -            && p[1] && !p[2])
>> +             && p[1] && !p[2])
>>       {
>>           VERIFY_PERMISSION(OPT_P_GENERAL|OPT_P_INSTANCE);
>>           if (streq(p[0], "ncp-ciphers"))
>>           {
>>               msg(M_INFO, "Note: Treating option '--ncp-ciphers' as "
>> -                        " '--data-ciphers' (renamed in OpenVPN 2.5).");
>> +                " '--data-ciphers' (renamed in OpenVPN 2.5).");
>>           }
>>           options->ncp_ciphers = p[1];
>>       }
>> @@ -7878,9 +7949,9 @@ add_option(struct options *options,
>>       {
>>           VERIFY_PERMISSION(OPT_P_GENERAL|OPT_P_INSTANCE);
>>           options->ncp_enabled = false;
>> -        msg(M_WARN, "DEPRECATED OPTION: ncp-disable. Disabling dynamic "
>> -                    "cipher negotiation is a deprecated debug feature that "
>> -                    "will be removed in OpenVPN 2.6");
>> +        msg(M_WARN, "DEPRECATED OPTION: ncp-disable. Disabling "
>> +            "cipher negotiation is a deprecated debug feature that "
>> +            "will be removed in OpenVPN 2.6");
>>       }
>>       else if (streq(p[0], "prng") && p[1] && !p[3])
>>       {
>> diff --git a/src/openvpn/options.h b/src/openvpn/options.h
>> index c5df2d18..877e9396 100644
>> --- a/src/openvpn/options.h
>> +++ b/src/openvpn/options.h
>> @@ -503,6 +503,8 @@ struct options
>>       bool shared_secret_file_inline;
>>       int key_direction;
>>       const char *ciphername;
>> +    bool enable_ncp_fallback;      /**< If defined fall back to
>> +                                    * ciphername if NCP fails */
>>       bool ncp_enabled;
>>       const char *ncp_ciphers;
>>       const char *authname;
>> diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
>> index 91ab3bf6..06dc9f8f 100644
>> --- a/src/openvpn/ssl.c
>> +++ b/src/openvpn/ssl.c
>> @@ -1932,13 +1932,14 @@ tls_session_update_crypto_params(struct tls_session *session,
>>           return true;
>>       }
>>   
>> -    if (!session->opt->server
>> -        && 0 != strcmp(options->ciphername, session->opt->config_ciphername)
>> +    bool cipher_allowed_as_fallback = options->enable_ncp_fallback
>> +                                      && streq(options->ciphername, session->opt->config_ciphername);
>> +
>> +    if (!session->opt->server && !cipher_allowed_as_fallback
>>           && !tls_item_in_cipher_list(options->ciphername, options->ncp_ciphers))
>>       {
>> -        msg(D_TLS_ERRORS, "Error: pushed cipher not allowed - %s not in %s or %s",
>> -            options->ciphername, session->opt->config_ciphername,
>> -            options->ncp_ciphers);
>> +        msg(D_TLS_ERRORS, "Error: pushed cipher not allowed - %s not in %s",
>> +            options->ciphername, options->ncp_ciphers);
>>           /* undo cipher push, abort connection setup */
>>           options->ciphername = session->opt->config_ciphername;
>>           return false;
>> @@ -1956,9 +1957,9 @@ tls_session_update_crypto_params(struct tls_session *session,
>>       }
>>       else
>>       {
>> -      /* Very hacky workaround and quick fix for frame calculation
>> -       * different when adjusting frame size when the original and new cipher
>> -       * are identical to avoid a regression with client without NCP */
>> +        /* Very hacky workaround and quick fix for frame calculation
>> +         * different when adjusting frame size when the original and new cipher
>> +         * are identical to avoid a regression with client without NCP */
>>           return tls_session_generate_data_channel_keys(session);
>>       }
>>   
>> diff --git a/src/openvpn/ssl_ncp.c b/src/openvpn/ssl_ncp.c
>> index 8e432fb7..2d3983c2 100644
>> --- a/src/openvpn/ssl_ncp.c
>> +++ b/src/openvpn/ssl_ncp.c
>> @@ -48,6 +48,7 @@
>>   #include "common.h"
>>   
>>   #include "ssl_ncp.h"
>> +#include "openvpn.h"
>>   
>>   /**
>>    * Return the Negotiable Crypto Parameters version advertised in the peer info
>> @@ -211,9 +212,8 @@ tls_peer_ncp_list(const char *peer_info, struct gc_arena *gc)
>>   }
>>   
>>   char *
>> -ncp_get_best_cipher(const char *server_list, const char *server_cipher,
>> -                    const char *peer_info,  const char *remote_cipher,
>> -                    struct gc_arena *gc)
>> +ncp_get_best_cipher(const char *server_list, const char *peer_info,
>> +                    const char *remote_cipher, struct gc_arena *gc)
>>   {
>>       /*
>>        * The gc of the parameter is tied to the VPN session, create a
>> @@ -226,7 +226,9 @@ ncp_get_best_cipher(const char *server_list, const char *server_cipher,
>>       const char *peer_ncp_list = tls_peer_ncp_list(peer_info, &gc_tmp);
>>   
>>       /* non-NCP client without OCC?  "assume nothing" */
>> -    if (remote_cipher == NULL)
>> +    /* For client doing the newer version of NCP (that send IV_CIPHER)
>> +     * we cannot assume that they will accept remote_cipher */
>> +    if (remote_cipher == NULL || strstr(peer_info, "IV_CIPHERS="))
> 
> Just noting the missing NULL check that Gert found with testing. Can you
> add a regression test while at it?
> 
>>       {
>>           remote_cipher = "";
>>       }
>> @@ -242,15 +244,6 @@ ncp_get_best_cipher(const char *server_list, const char *server_cipher,
>>               break;
>>           }
>>       }
>> -    /* We have not found a common cipher, as a last resort check if the
>> -     * server cipher can be used
>> -     */
>> -    if (token == NULL
>> -        && (tls_item_in_cipher_list(server_cipher, peer_ncp_list)
>> -            || streq(server_cipher, remote_cipher)))
>> -    {
>> -        token = server_cipher;
>> -    }
>>   
>>       char *ret = NULL;
>>       if (token != NULL)
>> @@ -262,16 +255,75 @@ ncp_get_best_cipher(const char *server_list, const char *server_cipher,
>>       return ret;
>>   }
>>   
>> -void
>> +/**
>> + * "Poor man's NCP": Use peer cipher if it is an allowed (NCP) cipher.
>> + * Allows non-NCP peers to upgrade their cipher individually.
>> + *
>> + * Returns true if we switched to the peer's cipher
>> + *
>> + * Make sure to call tls_session_update_crypto_params() after calling this
>> + * function.
>> + */
>> +static bool
>>   tls_poor_mans_ncp(struct options *o, const char *remote_ciphername)
>>   {
>> -    if (o->ncp_enabled && remote_ciphername
>> +    if (remote_ciphername
>>           && 0 != strcmp(o->ciphername, remote_ciphername))
>>       {
>>           if (tls_item_in_cipher_list(remote_ciphername, o->ncp_ciphers))
>>           {
>>               o->ciphername = string_alloc(remote_ciphername, &o->gc);
>>               msg(D_TLS_DEBUG_LOW, "Using peer cipher '%s'", o->ciphername);
>> +            return true;
>>           }
>>       }
>> +    return false;
>>   }
>> +
>> +bool
>> +check_pull_client_ncp(struct context *c, const int found)
>> +{
>> +    if (found & OPT_P_NCP)
>> +    {
>> +        msg(D_PUSH, "OPTIONS IMPORT: data channel crypto options modified");
>> +        return true;
>> +    }
>> +
>> +    if (!c->options.ncp_enabled)
>> +    {
>> +        return true;
>> +    }
>> +    /* If the server did not push a --cipher, we will switch to the
>> +     * remote cipher if it is in our ncp-ciphers list */
>> +    bool useremotecipher = tls_poor_mans_ncp(&c->options,
>> +                                             c->c2.tls_multi->remote_ciphername);
>> +
>> +
>> +    /* We could not figure out the peer's cipher but we have fallback
>> +     * enable */
> 
> enableD.
> 
>> +    if (!useremotecipher && c->options.enable_ncp_fallback)
>> +    {
>> +        return true;
>> +    }
>> +
>> +    /* We failed negotiation, give appropiate error message */
>> +    if (c->c2.tls_multi->remote_ciphername)
>> +    {
>> +        msg(D_TLS_ERRORS, "OPTIONS ERROR: failed to negotiate "
>> +            "cipher with server.  Add the server's "
>> +            "cipher ('%s') to --data-ciphers (currently '%s') if "
>> +            "you want to connect to this server.",
>> +            c->c2.tls_multi->remote_ciphername,
>> +            c->options.ncp_ciphers);
>> +        return false;
>> +
>> +    }
>> +    else
>> +    {
>> +        msg(D_TLS_ERRORS, "OPTIONS ERROR: failed to negotiate "
>> +            "cipher with server. Configure "
>> +            "--data-ciphers-fallback if you want to connect "
>> +            "to this server.");
>> +        return false;
>> +    }
>> +}
>> \ No newline at end of file
>> diff --git a/src/openvpn/ssl_ncp.h b/src/openvpn/ssl_ncp.h
>> index d00c222d..39158a56 100644
>> --- a/src/openvpn/ssl_ncp.h
>> +++ b/src/openvpn/ssl_ncp.h
>> @@ -40,14 +40,17 @@
>>   bool
>>   tls_peer_supports_ncp(const char *peer_info);
>>   
>> +/* forward declaration to break include dependency loop */
>> +struct context;
>> +
>>   /**
>> - * "Poor man's NCP": Use peer cipher if it is an allowed (NCP) cipher.
>> - * Allows non-NCP peers to upgrade their cipher individually.
>> + * Checks whether the cipher negotiation is in an acceptable state
>> + * and we continue to connect or should abort.
>>    *
>> - * Make sure to call tls_session_update_crypto_params() after calling this
>> - * function.
>> + * @return  Wether the client NCP process suceeded or failed
>>    */
>> -void tls_poor_mans_ncp(struct options *o, const char *remote_ciphername);
>> +bool
>> +check_pull_client_ncp(struct context *c, int found);
>>   
>>   /**
>>    * Iterates through the ciphers in server_list and return the first
>> @@ -67,9 +70,8 @@ void tls_poor_mans_ncp(struct options *o, const char *remote_ciphername);
>>    * cipher
>>    */
>>   char *
>> -ncp_get_best_cipher(const char *server_list, const char *server_cipher,
>> -                    const char *peer_info, const char *remote_cipher,
>> -                    struct gc_arena *gc);
>> +ncp_get_best_cipher(const char *server_list, const char *peer_info,
>> +                    const char *remote_cipher, struct gc_arena *gc);
>>   
>>   
>>   /**
>> diff --git a/tests/unit_tests/openvpn/test_ncp.c b/tests/unit_tests/openvpn/test_ncp.c
>> index 19432410..ea950030 100644
>> --- a/tests/unit_tests/openvpn/test_ncp.c
>> +++ b/tests/unit_tests/openvpn/test_ncp.c
>> @@ -139,21 +139,29 @@ test_poor_man(void **state)
>>       char *best_cipher;
>>   
>>       const char *serverlist = "CHACHA20_POLY1305:AES-128-GCM";
>> +    const char *serverlistbfcbc = "CHACHA20_POLY1305:AES-128-GCM:BF-CBC";
>>   
>> -    best_cipher = ncp_get_best_cipher(serverlist, "BF-CBC",
>> +    best_cipher = ncp_get_best_cipher(serverlist,
>> +                                      "IV_YOLO=NO\nIV_BAR=7",
>> +                                      "BF-CBC", &gc);
>> +
>> +    assert_ptr_equal(best_cipher, NULL);
>> +
>> +
>> +    best_cipher = ncp_get_best_cipher(serverlistbfcbc,
>>                                         "IV_YOLO=NO\nIV_BAR=7",
>>                                         "BF-CBC", &gc);
>>   
>>       assert_string_equal(best_cipher, "BF-CBC");
>>   
>> -    best_cipher = ncp_get_best_cipher(serverlist, "BF-CBC",
>> +
>> +    best_cipher = ncp_get_best_cipher(serverlist,
>>                                         "IV_NCP=1\nIV_BAR=7",
>>                                         "AES-128-GCM", &gc);
>>   
>>       assert_string_equal(best_cipher, "AES-128-GCM");
>>   
>> -    best_cipher = ncp_get_best_cipher(serverlist, "BF-CBC",
>> -                                      NULL,
>> +    best_cipher = ncp_get_best_cipher(serverlist, NULL,
>>                                         "AES-128-GCM", &gc);
>>   
>>       assert_string_equal(best_cipher, "AES-128-GCM");
>> @@ -170,29 +178,27 @@ test_ncp_best(void **state)
>>   
>>       const char *serverlist = "CHACHA20_POLY1305:AES-128-GCM:AES-256-GCM";
>>   
>> -    best_cipher = ncp_get_best_cipher(serverlist, "BF-CBC",
>> +    best_cipher = ncp_get_best_cipher(serverlist,
>>                                         "IV_YOLO=NO\nIV_NCP=2\nIV_BAR=7",
>>                                         "BF-CBC", &gc);
>>   
>>       assert_string_equal(best_cipher, "AES-128-GCM");
>>   
>>       /* Best cipher is in --cipher of client */
>> -    best_cipher = ncp_get_best_cipher(serverlist, "BF-CBC",
>> -                                      "IV_NCP=2\nIV_BAR=7",
>> +    best_cipher = ncp_get_best_cipher(serverlist, "IV_NCP=2\nIV_BAR=7",
>>                                         "CHACHA20_POLY1305", &gc);
>>   
>>       assert_string_equal(best_cipher, "CHACHA20_POLY1305");
>>   
>>       /* Best cipher is in --cipher of client */
>> -    best_cipher = ncp_get_best_cipher(serverlist, "BF-CBC",
>> -                                      "IV_CIPHERS=AES-128-GCM",
>> +    best_cipher = ncp_get_best_cipher(serverlist, "IV_CIPHERS=AES-128-GCM",
>>                                         "AES-256-CBC", &gc);
>>   
>>   
>>       assert_string_equal(best_cipher, "AES-128-GCM");
>>   
>>       /* IV_NCP=2 should be ignored if IV_CIPHERS is sent */
>> -    best_cipher = ncp_get_best_cipher(serverlist, "BF-CBC",
>> +    best_cipher = ncp_get_best_cipher(serverlist,
>>                                         "IV_FOO=7\nIV_CIPHERS=AES-256-GCM\nIV_NCP=2",
>>                                         "AES-256-CBC", &gc);
>>   
>>
> 
> I still try to find time to do more review and testing, but don't wait
> for me if someone else has taken a good look and/or given this patch a
> good beating.
> 
> -Steffan
> 
> 
> _______________________________________________
> Openvpn-devel mailing list
> Ope...@li...
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel
> 

Re: [Openvpn-devel] [PATCH v2] Rework NCP compability logic and drop BF-CBC support by default

[Steffan K. <st...@ka...>]

Hi,

No full review yet, but this version does seem to address my previous
comments. Some minor nits I noticed on my first run through v2:

On 29-07-2020 13:38, Arne Schwabe wrote:
> This reworks the NCP logic to be more strict about what is
> considered an acceptable result of an NCP negotiation. It also
> us to finally drop BF-CBC support by default.
> 
> All new behaviour is currently limited to server/client
> mode with pull enabled. P2p mode without pull does not change.
> 
> New Server behaviour:
> - when a client announces its supported ciphers through either
>   OCC or IV_CIPHER/IV_NCP we reject the client with a
>   AUTH_FAILED message if we have no common cipher.
> 
> - When a client does not announce any cipher in either
>   OCC or NCP we by reject it unless fallback-cipher is
>   specified in either ccd or config.
> 
> New client behaviour:
> - When no cipher is pushed (or a cipher we refused to support)
>   and we also cannot support the server's server announced in
>   OCC we fail the connection and log why
> 
> - If fallback-cipher is specified we will in the case that
>   cipher is missing from occ use the fallback cipher instead
>   of failing the connection
> 
> Both client and server behaviour:
> - We only announce --cipher xyz in occ if we are willing
>   to support that cipher.
> 
>   It means that we only announce the fallback-cipher if
>   it is also contained in --data-ciphers
> 
> Compatiblity behaviour:
> 
> In 2.5 both client and server will automatically set
> fallback-cipher xyz if --cipher xyz is in the config and
> also add append the cipher to the end of data-ciphers.
> 
> We log a warn user about this and point to --data-ciphers and
> --fallback-cipher. This also happens if the configuration
> contains an explicit --cipher BF-CBC.
> 
> If --cipher is not set, we only warn that previous versions
> allowed BF-CBC and point how to reenable BF-CBC. This might
> break very few config where someone connects a very old
> client to a 2.5 server but at some point we need to drop
> the BF-CBC and those affected use will already have a the
> scary SWEET32 warning in their logs.
> 
> In short: If --cipher is explicitly set 2.6 will work the same as
> 2.4 did. When --cipher is not set, BF-CBC support is dropped and
> we warn about it.
> 
> Examples how breaking the default BF-CBC will be logged:
> 
> Client side:
>  - Client connecting to server that does not push cipher but
>    has --cipher in OCC
> 
>     OPTIONS ERROR: failed to negotiate cipher with server.  Add the server's cipher ('BF-CBC') to --data-ciphers (currently 'AES-256-GCM:AES-128-CBC') if you want to connect to this server.
> 
>  - Client connecting to a server that does not support OCC:
> 
>    OPTIONS ERROR: failed to negotioate cipher with server. Configure --fallback-cipher if you want connect to this server.
> 
> Server Side:
> 
> - Server has a client only supporting BF-CBC connecting:
> 
>   styx/IP PUSH: No common cipher between server and client. Server data-ciphers: 'CHACHA20-POLY1305:AES-128-GCM:AES-256-GCM:AES-256-CBC:AES-128-CBC', client supports cipher 'BF-CBC'.
> 
>  - Client without OCC:
> 
>    styx/IP PUSH:No NCP or OCC cipher data received from peer.
>    styx/IP Use --fallback-cipher with the cipher the client is using if you want to allow the client to connect
> 
> In all reject cases on the client:
> 
>    AUTH: Received control message: AUTH_FAILED,Data channel cipher negotiation failed (no shared cipher)
> 
> Signed-off-by: Arne Schwabe <ar...@rf...>
> 
> Patch V2: rename fallback-cipher to data-ciphers-fallback
>           add all corrections from Steffan
>           Ignore occ cipher for clients sending IV_CIPHERS
>           move client side ncp in its own function
>           do not print INSECURE cipher warning if BF-CBC is not allowed
> 
> Signed-off-by: Arne Schwabe <ar...@rf...>
> ---
>  doc/man-sections/protocol-options.rst |  22 ++++-
>  src/openvpn/crypto.c                  |   4 +-
>  src/openvpn/init.c                    |  18 ++--
>  src/openvpn/multi.c                   | 135 ++++++++++++++++----------
>  src/openvpn/options.c                 | 117 +++++++++++++++++-----
>  src/openvpn/options.h                 |   2 +
>  src/openvpn/ssl.c                     |  17 ++--
>  src/openvpn/ssl_ncp.c                 |  82 +++++++++++++---
>  src/openvpn/ssl_ncp.h                 |  18 ++--
>  tests/unit_tests/openvpn/test_ncp.c   |  26 +++--
>  10 files changed, 311 insertions(+), 130 deletions(-)
> 
> diff --git a/doc/man-sections/protocol-options.rst b/doc/man-sections/protocol-options.rst
> index 051f1d32..69d3bc67 100644
> --- a/doc/man-sections/protocol-options.rst
> +++ b/doc/man-sections/protocol-options.rst
> @@ -57,6 +57,9 @@ 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 and ``--data-ciphers``
> +  or rarely `--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
> @@ -183,8 +186,9 @@ configured in a compatible way between both the local and remote side.
>    ``--server`` ), or if ``--pull`` is specified (client-side, implied by
>    setting --client).
>  
> -  If both peers support and do not disable NCP, the negotiated cipher will
> -  override the cipher specified by ``--cipher``.
> +  If no common cipher is found during cipher negotiation, the  connection
> +  is terminated. To support old clients/server that do not provide any cipher
> +  negotiation support see ``data-ciphers-fallback``.
>  
>    Additionally, to allow for more smooth transition, if NCP is enabled,
>    OpenVPN will inherit the cipher of the peer if that cipher is different
> @@ -201,8 +205,18 @@ configured in a compatible way between both the local and remote side.
>    This list is restricted to be 127 chars long after conversion to OpenVPN
>    ciphers.
>  
> -  This option was called ``ncp-ciphers`` in OpenVPN 2.4 but has been renamed
> -  to ``data-ciphers`` in OpenVPN 2.5 to more accurately reflect its meaning.
> +  This option was called ``--ncp-ciphers`` in OpenVPN 2.4 but has been renamed
> +  to ``--data-ciphers`` in OpenVPN 2.5 to more accurately reflect its meaning.
> +
> +--data-ciphers-fallback alg
> +
> +    Configure a cipher that is used to fall back to if we could not determine
> +    which cipher the peer is willing to use.
> +
> +    This option should only be needed to
> +    connect to peers that are running OpenVPN 2.3 and older version, and
> +    have been configured with `--enable-small`
> +    (typically used on routers or other embedded devices).
>  
>  --ncp-disable
>    Disable "Negotiable Crypto Parameters". This completely disables cipher
> diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
> index e92a0dc1..3a0bfbec 100644
> --- a/src/openvpn/crypto.c
> +++ b/src/openvpn/crypto.c
> @@ -727,7 +727,9 @@ warn_insecure_key_type(const char *ciphername, const cipher_kt_t *cipher)
>      {
>          msg(M_WARN, "WARNING: INSECURE cipher (%s) with block size less than 128"
>              " bit (%d bit).  This allows attacks like SWEET32.  Mitigate by "
> -            "using a --cipher with a larger block size (e.g. AES-256-CBC).",
> +            "using a --cipher with a larger block size (e.g. AES-256-CBC). "
> +            "Support for these insecure ciphers will be removed in "
> +            "OpenVPN 2.6.",
>              ciphername, cipher_kt_block_size(cipher)*8);
>      }
>  }
> diff --git a/src/openvpn/init.c b/src/openvpn/init.c
> index 1ea4735d..402d2652 100644
> --- a/src/openvpn/init.c
> +++ b/src/openvpn/init.c
> @@ -2365,16 +2365,9 @@ do_deferred_options(struct context *c, const unsigned int found)
>      /* process (potentially pushed) crypto options */
>      if (c->options.pull)
>      {
> -        struct tls_session *session = &c->c2.tls_multi->session[TM_ACTIVE];
> -        if (found & OPT_P_NCP)
> -        {
> -            msg(D_PUSH, "OPTIONS IMPORT: data channel crypto options modified");
> -        }
> -        else if (c->options.ncp_enabled)
> +        if (!check_pull_client_ncp(c, found))
>          {
> -            /* If the server did not push a --cipher, we will switch to the
> -             * remote cipher if it is in our ncp-ciphers list */
> -            tls_poor_mans_ncp(&c->options, c->c2.tls_multi->remote_ciphername);
> +            return false;
>          }
>          struct frame *frame_fragment = NULL;
>  #ifdef ENABLE_FRAGMENT
> @@ -2384,6 +2377,7 @@ do_deferred_options(struct context *c, const unsigned int found)
>          }
>  #endif
>  
> +        struct tls_session *session = &c->c2.tls_multi->session[TM_ACTIVE];
>          if (!tls_session_update_crypto_params(session, &c->options, &c->c2.frame,
>                                                frame_fragment))
>          {
> @@ -2757,9 +2751,13 @@ do_init_crypto_tls_c1(struct context *c)
>  #endif /* if P2MP */
>          }
>  
> +        /* Do not warn if 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(&c->c1.ks.key_type, options->ciphername, options->authname,
> -                      options->keysize, true, true);
> +                      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/multi.c b/src/openvpn/multi.c
> index 0f9c586b..79b5c0c3 100644
> --- a/src/openvpn/multi.c
> +++ b/src/openvpn/multi.c
> @@ -1777,7 +1777,7 @@ multi_client_connect_setenv(struct multi_context *m,
>   * - choosen cipher
>   * - peer id
>   */
> -static void
> +static bool
>  multi_client_set_protocol_options(struct context *c)
>  {
>  
> @@ -1807,56 +1807,85 @@ multi_client_set_protocol_options(struct context *c)
>      }
>  
>      /* Select cipher if client supports Negotiable Crypto Parameters */
> -    if (o->ncp_enabled)
> +    if (!o->ncp_enabled)
>      {
> -        /* if we have already created our key, we cannot *change* our own
> -         * cipher -> so log the fact and push the "what we have now" cipher
> -         * (so the client is always told what we expect it to use)
> -         */
> -        const struct tls_session *session = &tls_multi->session[TM_ACTIVE];
> -        if (session->key[KS_PRIMARY].crypto_options.key_ctx_bi.initialized)
> +        return true;
> +    }
> +
> +    /* if we have already created our key, we cannot *change* our own
> +     * cipher -> so log the fact and push the "what we have now" cipher
> +     * (so the client is always told what we expect it to use)
> +     */
> +    const struct tls_session *session = &tls_multi->session[TM_ACTIVE];
> +    if (session->key[KS_PRIMARY].crypto_options.key_ctx_bi.initialized)
> +    {
> +        msg( M_INFO, "PUSH: client wants to negotiate cipher (NCP), but "

No space after ( .

> +             "server has already generated data channel keys, "
> +             "re-sending previously negotiated cipher '%s'",
> +             o->ciphername );
> +        return true;
> +    }
> +
> +    /*
> +     * Push the first cipher from --data-ciphers to the client that
> +     * the client announces to be supporting.
> +     */
> +    char *push_cipher = ncp_get_best_cipher(o->ncp_ciphers, peer_info,
> +                                            tls_multi->remote_ciphername,
> +                                            &o->gc);
> +
> +    if (push_cipher)
> +    {
> +        o->ciphername = push_cipher;
> +        return true;
> +    }
> +
> +    /* NCP cipher negotiation failed. Try to figure out why exactly it
> +     * failed and give good error messages and potentially do a fallback
> +     * for non NCP clients */
> +    struct gc_arena gc = gc_new();
> +    bool ret = false;
> +
> +    const char *peer_ciphers = tls_peer_ncp_list(peer_info, &gc);
> +    /* If we are in a situation where we know the client ciphers, there is no
> +     * reason to fall back to a cipher that will not be accepted by the other
> +     * side, in this situation we fail the auth*/
> +    if (strlen(peer_ciphers) > 0)
> +    {
> +        msg(M_INFO, "PUSH: No common cipher between server and client. "
> +            "Server data-ciphers: '%s', client supported ciphers '%s'",
> +            o->ncp_ciphers, peer_ciphers);
> +    }
> +    else if (tls_multi->remote_ciphername)
> +    {
> +        msg(M_INFO, "PUSH: No common cipher between server and client. "
> +            "Server data-ciphers: '%s', client supports cipher '%s'",
> +            o->ncp_ciphers, tls_multi->remote_ciphername);
> +    }
> +    else
> +    {
> +        msg(M_INFO, "PUSH: No NCP or OCC cipher data received from peer.");
> +
> +        if (o->enable_ncp_fallback && !tls_multi->remote_ciphername)
>          {
> -            msg( M_INFO, "PUSH: client wants to negotiate cipher (NCP), but "
> -                 "server has already generated data channel keys, "
> -                 "re-sending previously negotiated cipher '%s'",
> -                 o->ciphername );
> +            msg(M_INFO, "Using data channel cipher '%s' since "
> +                "--data-ciphers-fallback is set.", o->ciphername);
> +            ret = true;
>          }
>          else
>          {
> -            /*
> -             * Push the first cipher from --data-ciphers to the client that
> -             * the client announces to be supporting.
> -             */
> -            char *push_cipher = ncp_get_best_cipher(o->ncp_ciphers, o->ciphername,
> -                                                    peer_info,
> -                                                    tls_multi->remote_ciphername,
> -                                                    &o->gc);
> -
> -            if (push_cipher)
> -            {
> -                o->ciphername = push_cipher;
> -            }
> -            else
> -            {
> -                struct gc_arena gc = gc_new();
> -                const char *peer_ciphers = tls_peer_ncp_list(peer_info, &gc);
> -                if (strlen(peer_ciphers) > 0)
> -                {
> -                    msg(M_INFO, "PUSH: No common cipher between server and "
> -                        "client. Expect this connection not to work. Server "
> -                        "data-ciphers: '%s', client supported ciphers '%s'",
> -                        o->ncp_ciphers, peer_ciphers);
> -                }
> -                else
> -                {
> -                    msg(M_INFO, "No NCP data received from peer, falling back "
> -                        "to --cipher '%s'. Peer reports in OCC --cipher '%s'",
> -                        o->ciphername, np(tls_multi->remote_ciphername));
> -                }
> -                gc_free(&gc);
> -            }
> +            msg(M_INFO, "Use --data-ciphers-fallback with the cipher the "
> +                "client is using if you want to allow the client to connect");
>          }
>      }
> +    if (!ret)
> +    {
> +        auth_set_client_reason(tls_multi, "Data channel cipher negotiation "
> +                                          "failed (no shared cipher)");
> +    }
> +
> +    gc_free(&gc);
> +    return ret;
>  }
>  
>  /**
> @@ -2322,7 +2351,7 @@ multi_client_connect_late_setup(struct multi_context *m,
>      if (!mi->context.c2.push_ifconfig_defined)
>      {
>          msg(D_MULTI_ERRORS, "MULTI: no dynamic or static remote"
> -            "--ifconfig address is available for %s",
> +                            "--ifconfig address is available for %s",
>              multi_instance_string(mi, false, &gc));
>      }
>  
> @@ -2338,7 +2367,7 @@ multi_client_connect_late_setup(struct multi_context *m,
>  
>          /* JYFIXME -- this should cause the connection to fail */
>          msg(D_MULTI_ERRORS, "MULTI ERROR: primary virtual IP for %s (%s)"
> -            "violates tunnel network/netmask constraint (%s/%s)",
> +                            "violates tunnel network/netmask constraint (%s/%s)",
>              multi_instance_string(mi, false, &gc),
>              print_in_addr_t(mi->context.c2.push_ifconfig_local, 0, &gc),
>              ifconfig_constraint_network, ifconfig_constraint_netmask);
> @@ -2387,7 +2416,7 @@ multi_client_connect_late_setup(struct multi_context *m,
>      else if (mi->context.options.iroutes)
>      {
>          msg(D_MULTI_ERRORS, "MULTI: --iroute options rejected for %s -- iroute"
> -            "only works with tun-style tunnels",
> +                            "only works with tun-style tunnels",
>              multi_instance_string(mi, false, &gc));
>      }
>  
> @@ -2399,11 +2428,15 @@ multi_client_connect_late_setup(struct multi_context *m,
>      mi->context.c2.context_auth = CAS_SUCCEEDED;
>  
>      /* authentication complete, calculate dynamic client specific options */
> -    multi_client_set_protocol_options(&mi->context);
> -
> -    /* Generate data channel keys */
> -    if (!multi_client_generate_tls_keys(&mi->context))
> +    if (!multi_client_set_protocol_options(&mi->context))
> +    {
> +        mi->context.c2.context_auth = CAS_FAILED;
> +    }
> +    /* Generate data channel keys only if setting protocol options
> +     * has not failed */
> +    else if (!multi_client_generate_tls_keys(&mi->context))
>      {
> +
>          mi->context.c2.context_auth = CAS_FAILED;
>      }
>  
> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> index bc256b18..c53ef7f9 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -855,7 +855,6 @@ init_options(struct options *o, const bool init_gc)
>  #if P2MP
>      o->scheduled_exit_interval = 5;
>  #endif
> -    o->ciphername = "BF-CBC";
>      o->ncp_enabled = true;
>      o->ncp_ciphers = "AES-256-GCM:AES-128-GCM";
>      o->authname = "SHA1";
> @@ -2053,7 +2052,7 @@ options_postprocess_verify_ce(const struct options *options, const struct connec
>      if (options->inetd)
>      {
>          msg(M_WARN, "DEPRECATED OPTION: --inetd mode is deprecated "
> -                    "and will be removed in OpenVPN 2.6");
> +            "and will be removed in OpenVPN 2.6");
>      }
>  
>      if (options->lladdr && dev != DEV_TYPE_TAP)
> @@ -3046,6 +3045,67 @@ options_postprocess_verify(const struct options *o)
>      }
>  }
>  
> +static void
> +options_postprocess_cipher(struct options *o)
> +{
> +    if (!o->pull && !(o->mode == MODE_SERVER))
> +    {
> +        /* we are in the classic P2P mode */
> +        o->ncp_enabled = false;
> +        msg( M_WARN, "Cipher negotiation is disabled since neither "
> +             "P2MP client nor server mode is enabled");
> +
> +        /* If the cipher is not set, use the old default ofo BF-CBC. We will
> +         * warn that this is deprecated on cipher initialisation, no need
> +         * to warn here as well */
> +        if (!o->ciphername)
> +        {
> +            o->ciphername = "BF-CBC";
> +        }
> +        return;
> +    }
> +
> +    /* pull or P2MP mode */
> +    if (!o->ciphername)
> +    {
> +        if (!o->ncp_enabled)
> +        {
> +            msg(M_USAGE, "--ncp-disable needs an explicit --cipher or "
> +                         "--data-ciphers-fallback config option");
> +        }
> +
> +        msg(M_WARN, "--cipher is not set. Previous OpenVPN version 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.");
> +
> +        /* 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";
> +    }
> +    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;

Missing space after the last + .

> +        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;
> +    }
> +}
> +
>  static void
>  options_postprocess_mutate(struct options *o)
>  {
> @@ -3058,6 +3118,7 @@ options_postprocess_mutate(struct options *o)
>      helper_keepalive(o);
>      helper_tcp_nodelay(o);
>  
> +    options_postprocess_cipher(o);
>      options_postprocess_mutate_invariant(o);
>  
>      if (o->ncp_enabled)
> @@ -3118,16 +3179,6 @@ options_postprocess_mutate(struct options *o)
>              "include this in your server configuration");
>          o->dh_file = NULL;
>      }
> -
> -    /* cipher negotiation (NCP) currently assumes --pull or --mode server */
> -    if (o->ncp_enabled
> -        && !(o->pull || o->mode == MODE_SERVER) )
> -    {
> -        msg( M_WARN, "disabling NCP mode (--ncp-disable) because not "
> -             "in P2MP client or server mode" );
> -        o->ncp_enabled = false;
> -    }
> -
>  #if ENABLE_MANAGEMENT
>      if (o->http_proxy_override)
>      {
> @@ -3663,14 +3714,21 @@ options_string(const struct options *o,
>       */
>  
>      buf_printf(&out, ",dev-type %s", dev_type_string(o->dev, o->dev_type));
> -    buf_printf(&out, ",link-mtu %u", (unsigned int) calc_options_string_link_mtu(o, frame));
> +    /* the link-mtu that we send has only a meaning if have a fixed
> +     * cipher (p2p) or have a fallback cipher configured for older non
> +     * ncp clients. But not sending it, will make even 2.4 complain
> +     * about it missing. So still send it. */
> +    buf_printf(&out, ",link-mtu %u",
> +               (unsigned int) calc_options_string_link_mtu(o, frame));
> +
>      buf_printf(&out, ",tun-mtu %d", PAYLOAD_SIZE(frame));
>      buf_printf(&out, ",proto %s",  proto_remote(o->ce.proto, remote));
>  
> +    bool p2p_nopull = o->mode == MODE_POINT_TO_POINT && !PULL_DEFINED(o);
>      /* send tun_ipv6 only in peer2peer mode - in client/server mode, it
>       * is usually pushed by the server, triggering a non-helpful warning
>       */
> -    if (o->ifconfig_ipv6_local && o->mode == MODE_POINT_TO_POINT && !PULL_DEFINED(o))
> +    if (o->ifconfig_ipv6_local && p2p_nopull)
>      {
>          buf_printf(&out, ",tun-ipv6");
>      }
> @@ -3700,7 +3758,7 @@ options_string(const struct options *o,
>          }
>      }
>  
> -    if (tt && o->mode == MODE_POINT_TO_POINT && !PULL_DEFINED(o))
> +    if (tt && p2p_nopull)
>      {
>          const char *ios = ifconfig_options_string(tt, remote, o->ifconfig_nowarn, gc);
>          if (ios && strlen(ios))
> @@ -3756,8 +3814,14 @@ options_string(const struct options *o,
>  
>          init_key_type(&kt, o->ciphername, o->authname, o->keysize, true,
>                        false);
> -
> -        buf_printf(&out, ",cipher %s", cipher_kt_name(kt.cipher));
> +        /* Only announce the cipher to our peer if we are willing to
> +         * support it */
> +        const char *ciphername = cipher_kt_name(kt.cipher);
> +        if (p2p_nopull || !o->ncp_enabled
> +            || tls_item_in_cipher_list(ciphername, o->ncp_ciphers))
> +        {
> +            buf_printf(&out, ",cipher %s", ciphername);
> +        }
>          buf_printf(&out, ",auth %s", md_kt_name(kt.digest));
>          buf_printf(&out, ",keysize %d", kt.cipher_length * 8);
>          if (o->shared_secret_file)
> @@ -3875,7 +3939,8 @@ options_warning_safe_scan2(const int msglevel,
>          || strprefix(p1, "keydir ")
>          || strprefix(p1, "proto ")
>          || strprefix(p1, "tls-auth ")
> -        || strprefix(p1, "tun-ipv6"))
> +        || strprefix(p1, "tun-ipv6")
> +        || strprefix(p1, "cipher "))
>      {
>          return;
>      }
> @@ -7863,14 +7928,20 @@ add_option(struct options *options,
>          VERIFY_PERMISSION(OPT_P_NCP|OPT_P_INSTANCE);
>          options->ciphername = p[1];
>      }
> +    else if (streq(p[0], "data-ciphers-fallback") && p[1] && !p[2])
> +    {
> +        VERIFY_PERMISSION(OPT_P_GENERAL|OPT_P_INSTANCE);
> +        options->ciphername = p[1];
> +        options->enable_ncp_fallback = true;
> +    }
>      else if ((streq(p[0], "data-ciphers") || streq(p[0], "ncp-ciphers"))
> -            && p[1] && !p[2])
> +             && p[1] && !p[2])
>      {
>          VERIFY_PERMISSION(OPT_P_GENERAL|OPT_P_INSTANCE);
>          if (streq(p[0], "ncp-ciphers"))
>          {
>              msg(M_INFO, "Note: Treating option '--ncp-ciphers' as "
> -                        " '--data-ciphers' (renamed in OpenVPN 2.5).");
> +                " '--data-ciphers' (renamed in OpenVPN 2.5).");
>          }
>          options->ncp_ciphers = p[1];
>      }
> @@ -7878,9 +7949,9 @@ add_option(struct options *options,
>      {
>          VERIFY_PERMISSION(OPT_P_GENERAL|OPT_P_INSTANCE);
>          options->ncp_enabled = false;
> -        msg(M_WARN, "DEPRECATED OPTION: ncp-disable. Disabling dynamic "
> -                    "cipher negotiation is a deprecated debug feature that "
> -                    "will be removed in OpenVPN 2.6");
> +        msg(M_WARN, "DEPRECATED OPTION: ncp-disable. Disabling "
> +            "cipher negotiation is a deprecated debug feature that "
> +            "will be removed in OpenVPN 2.6");
>      }
>      else if (streq(p[0], "prng") && p[1] && !p[3])
>      {
> diff --git a/src/openvpn/options.h b/src/openvpn/options.h
> index c5df2d18..877e9396 100644
> --- a/src/openvpn/options.h
> +++ b/src/openvpn/options.h
> @@ -503,6 +503,8 @@ struct options
>      bool shared_secret_file_inline;
>      int key_direction;
>      const char *ciphername;
> +    bool enable_ncp_fallback;      /**< If defined fall back to
> +                                    * ciphername if NCP fails */
>      bool ncp_enabled;
>      const char *ncp_ciphers;
>      const char *authname;
> diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
> index 91ab3bf6..06dc9f8f 100644
> --- a/src/openvpn/ssl.c
> +++ b/src/openvpn/ssl.c
> @@ -1932,13 +1932,14 @@ tls_session_update_crypto_params(struct tls_session *session,
>          return true;
>      }
>  
> -    if (!session->opt->server
> -        && 0 != strcmp(options->ciphername, session->opt->config_ciphername)
> +    bool cipher_allowed_as_fallback = options->enable_ncp_fallback
> +                                      && streq(options->ciphername, session->opt->config_ciphername);
> +
> +    if (!session->opt->server && !cipher_allowed_as_fallback
>          && !tls_item_in_cipher_list(options->ciphername, options->ncp_ciphers))
>      {
> -        msg(D_TLS_ERRORS, "Error: pushed cipher not allowed - %s not in %s or %s",
> -            options->ciphername, session->opt->config_ciphername,
> -            options->ncp_ciphers);
> +        msg(D_TLS_ERRORS, "Error: pushed cipher not allowed - %s not in %s",
> +            options->ciphername, options->ncp_ciphers);
>          /* undo cipher push, abort connection setup */
>          options->ciphername = session->opt->config_ciphername;
>          return false;
> @@ -1956,9 +1957,9 @@ tls_session_update_crypto_params(struct tls_session *session,
>      }
>      else
>      {
> -      /* Very hacky workaround and quick fix for frame calculation
> -       * different when adjusting frame size when the original and new cipher
> -       * are identical to avoid a regression with client without NCP */
> +        /* Very hacky workaround and quick fix for frame calculation
> +         * different when adjusting frame size when the original and new cipher
> +         * are identical to avoid a regression with client without NCP */
>          return tls_session_generate_data_channel_keys(session);
>      }
>  
> diff --git a/src/openvpn/ssl_ncp.c b/src/openvpn/ssl_ncp.c
> index 8e432fb7..2d3983c2 100644
> --- a/src/openvpn/ssl_ncp.c
> +++ b/src/openvpn/ssl_ncp.c
> @@ -48,6 +48,7 @@
>  #include "common.h"
>  
>  #include "ssl_ncp.h"
> +#include "openvpn.h"
>  
>  /**
>   * Return the Negotiable Crypto Parameters version advertised in the peer info
> @@ -211,9 +212,8 @@ tls_peer_ncp_list(const char *peer_info, struct gc_arena *gc)
>  }
>  
>  char *
> -ncp_get_best_cipher(const char *server_list, const char *server_cipher,
> -                    const char *peer_info,  const char *remote_cipher,
> -                    struct gc_arena *gc)
> +ncp_get_best_cipher(const char *server_list, const char *peer_info,
> +                    const char *remote_cipher, struct gc_arena *gc)
>  {
>      /*
>       * The gc of the parameter is tied to the VPN session, create a
> @@ -226,7 +226,9 @@ ncp_get_best_cipher(const char *server_list, const char *server_cipher,
>      const char *peer_ncp_list = tls_peer_ncp_list(peer_info, &gc_tmp);
>  
>      /* non-NCP client without OCC?  "assume nothing" */
> -    if (remote_cipher == NULL)
> +    /* For client doing the newer version of NCP (that send IV_CIPHER)
> +     * we cannot assume that they will accept remote_cipher */
> +    if (remote_cipher == NULL || strstr(peer_info, "IV_CIPHERS="))

Just noting the missing NULL check that Gert found with testing. Can you
add a regression test while at it?

>      {
>          remote_cipher = "";
>      }
> @@ -242,15 +244,6 @@ ncp_get_best_cipher(const char *server_list, const char *server_cipher,
>              break;
>          }
>      }
> -    /* We have not found a common cipher, as a last resort check if the
> -     * server cipher can be used
> -     */
> -    if (token == NULL
> -        && (tls_item_in_cipher_list(server_cipher, peer_ncp_list)
> -            || streq(server_cipher, remote_cipher)))
> -    {
> -        token = server_cipher;
> -    }
>  
>      char *ret = NULL;
>      if (token != NULL)
> @@ -262,16 +255,75 @@ ncp_get_best_cipher(const char *server_list, const char *server_cipher,
>      return ret;
>  }
>  
> -void
> +/**
> + * "Poor man's NCP": Use peer cipher if it is an allowed (NCP) cipher.
> + * Allows non-NCP peers to upgrade their cipher individually.
> + *
> + * Returns true if we switched to the peer's cipher
> + *
> + * Make sure to call tls_session_update_crypto_params() after calling this
> + * function.
> + */
> +static bool
>  tls_poor_mans_ncp(struct options *o, const char *remote_ciphername)
>  {
> -    if (o->ncp_enabled && remote_ciphername
> +    if (remote_ciphername
>          && 0 != strcmp(o->ciphername, remote_ciphername))
>      {
>          if (tls_item_in_cipher_list(remote_ciphername, o->ncp_ciphers))
>          {
>              o->ciphername = string_alloc(remote_ciphername, &o->gc);
>              msg(D_TLS_DEBUG_LOW, "Using peer cipher '%s'", o->ciphername);
> +            return true;
>          }
>      }
> +    return false;
>  }
> +
> +bool
> +check_pull_client_ncp(struct context *c, const int found)
> +{
> +    if (found & OPT_P_NCP)
> +    {
> +        msg(D_PUSH, "OPTIONS IMPORT: data channel crypto options modified");
> +        return true;
> +    }
> +
> +    if (!c->options.ncp_enabled)
> +    {
> +        return true;
> +    }
> +    /* If the server did not push a --cipher, we will switch to the
> +     * remote cipher if it is in our ncp-ciphers list */
> +    bool useremotecipher = tls_poor_mans_ncp(&c->options,
> +                                             c->c2.tls_multi->remote_ciphername);
> +
> +
> +    /* We could not figure out the peer's cipher but we have fallback
> +     * enable */

enableD.

> +    if (!useremotecipher && c->options.enable_ncp_fallback)
> +    {
> +        return true;
> +    }
> +
> +    /* We failed negotiation, give appropiate error message */
> +    if (c->c2.tls_multi->remote_ciphername)
> +    {
> +        msg(D_TLS_ERRORS, "OPTIONS ERROR: failed to negotiate "
> +            "cipher with server.  Add the server's "
> +            "cipher ('%s') to --data-ciphers (currently '%s') if "
> +            "you want to connect to this server.",
> +            c->c2.tls_multi->remote_ciphername,
> +            c->options.ncp_ciphers);
> +        return false;
> +
> +    }
> +    else
> +    {
> +        msg(D_TLS_ERRORS, "OPTIONS ERROR: failed to negotiate "
> +            "cipher with server. Configure "
> +            "--data-ciphers-fallback if you want to connect "
> +            "to this server.");
> +        return false;
> +    }
> +}
> \ No newline at end of file
> diff --git a/src/openvpn/ssl_ncp.h b/src/openvpn/ssl_ncp.h
> index d00c222d..39158a56 100644
> --- a/src/openvpn/ssl_ncp.h
> +++ b/src/openvpn/ssl_ncp.h
> @@ -40,14 +40,17 @@
>  bool
>  tls_peer_supports_ncp(const char *peer_info);
>  
> +/* forward declaration to break include dependency loop */
> +struct context;
> +
>  /**
> - * "Poor man's NCP": Use peer cipher if it is an allowed (NCP) cipher.
> - * Allows non-NCP peers to upgrade their cipher individually.
> + * Checks whether the cipher negotiation is in an acceptable state
> + * and we continue to connect or should abort.
>   *
> - * Make sure to call tls_session_update_crypto_params() after calling this
> - * function.
> + * @return  Wether the client NCP process suceeded or failed
>   */
> -void tls_poor_mans_ncp(struct options *o, const char *remote_ciphername);
> +bool
> +check_pull_client_ncp(struct context *c, int found);
>  
>  /**
>   * Iterates through the ciphers in server_list and return the first
> @@ -67,9 +70,8 @@ void tls_poor_mans_ncp(struct options *o, const char *remote_ciphername);
>   * cipher
>   */
>  char *
> -ncp_get_best_cipher(const char *server_list, const char *server_cipher,
> -                    const char *peer_info, const char *remote_cipher,
> -                    struct gc_arena *gc);
> +ncp_get_best_cipher(const char *server_list, const char *peer_info,
> +                    const char *remote_cipher, struct gc_arena *gc);
>  
>  
>  /**
> diff --git a/tests/unit_tests/openvpn/test_ncp.c b/tests/unit_tests/openvpn/test_ncp.c
> index 19432410..ea950030 100644
> --- a/tests/unit_tests/openvpn/test_ncp.c
> +++ b/tests/unit_tests/openvpn/test_ncp.c
> @@ -139,21 +139,29 @@ test_poor_man(void **state)
>      char *best_cipher;
>  
>      const char *serverlist = "CHACHA20_POLY1305:AES-128-GCM";
> +    const char *serverlistbfcbc = "CHACHA20_POLY1305:AES-128-GCM:BF-CBC";
>  
> -    best_cipher = ncp_get_best_cipher(serverlist, "BF-CBC",
> +    best_cipher = ncp_get_best_cipher(serverlist,
> +                                      "IV_YOLO=NO\nIV_BAR=7",
> +                                      "BF-CBC", &gc);
> +
> +    assert_ptr_equal(best_cipher, NULL);
> +
> +
> +    best_cipher = ncp_get_best_cipher(serverlistbfcbc,
>                                        "IV_YOLO=NO\nIV_BAR=7",
>                                        "BF-CBC", &gc);
>  
>      assert_string_equal(best_cipher, "BF-CBC");
>  
> -    best_cipher = ncp_get_best_cipher(serverlist, "BF-CBC",
> +
> +    best_cipher = ncp_get_best_cipher(serverlist,
>                                        "IV_NCP=1\nIV_BAR=7",
>                                        "AES-128-GCM", &gc);
>  
>      assert_string_equal(best_cipher, "AES-128-GCM");
>  
> -    best_cipher = ncp_get_best_cipher(serverlist, "BF-CBC",
> -                                      NULL,
> +    best_cipher = ncp_get_best_cipher(serverlist, NULL,
>                                        "AES-128-GCM", &gc);
>  
>      assert_string_equal(best_cipher, "AES-128-GCM");
> @@ -170,29 +178,27 @@ test_ncp_best(void **state)
>  
>      const char *serverlist = "CHACHA20_POLY1305:AES-128-GCM:AES-256-GCM";
>  
> -    best_cipher = ncp_get_best_cipher(serverlist, "BF-CBC",
> +    best_cipher = ncp_get_best_cipher(serverlist,
>                                        "IV_YOLO=NO\nIV_NCP=2\nIV_BAR=7",
>                                        "BF-CBC", &gc);
>  
>      assert_string_equal(best_cipher, "AES-128-GCM");
>  
>      /* Best cipher is in --cipher of client */
> -    best_cipher = ncp_get_best_cipher(serverlist, "BF-CBC",
> -                                      "IV_NCP=2\nIV_BAR=7",
> +    best_cipher = ncp_get_best_cipher(serverlist, "IV_NCP=2\nIV_BAR=7",
>                                        "CHACHA20_POLY1305", &gc);
>  
>      assert_string_equal(best_cipher, "CHACHA20_POLY1305");
>  
>      /* Best cipher is in --cipher of client */
> -    best_cipher = ncp_get_best_cipher(serverlist, "BF-CBC",
> -                                      "IV_CIPHERS=AES-128-GCM",
> +    best_cipher = ncp_get_best_cipher(serverlist, "IV_CIPHERS=AES-128-GCM",
>                                        "AES-256-CBC", &gc);
>  
>  
>      assert_string_equal(best_cipher, "AES-128-GCM");
>  
>      /* IV_NCP=2 should be ignored if IV_CIPHERS is sent */
> -    best_cipher = ncp_get_best_cipher(serverlist, "BF-CBC",
> +    best_cipher = ncp_get_best_cipher(serverlist,
>                                        "IV_FOO=7\nIV_CIPHERS=AES-256-GCM\nIV_NCP=2",
>                                        "AES-256-CBC", &gc);
>  
> 

I still try to find time to do more review and testing, but don't wait
for me if someone else has taken a good look and/or given this patch a
good beating.

-Steffan

[Openvpn-devel] [PATCH applied] Re: Fix compilation with --disable-lzo and --disable-lz4

[Gert D. <ge...@gr...>]

Acked-by: Gert Doering <ge...@gr...>

Thanks.  Tested that it indeed fixes things, and that it is 
not necessary for 2.4 (compile-tested only).

Added "trac #1308" to the commit message.

Your patch has been applied to the master branch.

commit dab34fdd0639c6de8c5ca759cca00b7e60da32f1
Author: Lev Stipakov
Date:   Wed Aug 5 06:25:48 2020 +0000

     Fix compilation with --disable-lzo and --disable-lz4

     Signed-off-by: Lev Stipakov <lst...@gm...>
     Acked-by: Gert Doering <ge...@gr...>
     Message-Id: <202...@gm...>
     URL: https://www.mail-archive.com/ope...@li.../msg20637.html
     Signed-off-by: Gert Doering <ge...@gr...>


--
kind regards,

Gert Doering

Re: [Openvpn-devel] [PATCH applied] Re: Log serial number of revoked certificate

[Gert D. <ge...@gr...>]

Hi,

On Wed, Aug 05, 2020 at 05:27:45PM +0500, Vladislav Grishenko wrote:
> Thank you.
> I'd appreciate if patch could be applied to release/2.4 too, no changes are
> required - related code is the same, just hunks offset in ssl_verify.c and
> ssl_verify_openssl.c
> I've tested 2.4.9 [git:release/2.4/7c428ca19a8df6b9+] with patch on sample
> certificates, please refer log is below:

OK.  Done.  Thanks :-)

commit 4ee2c1cd877b2e99b41fd248bf853329af825188 (HEAD -> release/2.4)

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                             ge...@gr...

Re: [Openvpn-devel] [PATCH applied] Re: Log serial number of revoked certificate

[Vladislav G. <the...@ya...>]

Hi, Gert

Thank you.
I'd appreciate if patch could be applied to release/2.4 too, no changes are
required - related code is the same, just hunks offset in ssl_verify.c and
ssl_verify_openssl.c
I've tested 2.4.9 [git:release/2.4/7c428ca19a8df6b9+] with patch on sample
certificates, please refer log is below:

OpenSSL, --crl-verify sample-keys/ca.crl
Wed Aug  5 17:18:49 2020 127.0.0.1:16001 VERIFY ERROR: depth=0,
error=certificate revoked: C=KG, ST=NA, O=OpenVPN-TEST, CN=client-revoked,
ema...@my..., serial=3
Wed Aug  5 17:18:49 2020 127.0.0.1:16001 OpenSSL: error:1417C086:SSL
routines:tls_process_client_certificate:certificate verify failed
Wed Aug  5 17:18:49 2020 127.0.0.1:16001 TLS_ERROR: BIO read
tls_read_plaintext error

mbedTLS, --crl-verify sample-keys/ca.crl
Wed Aug  5 17:25:28 2020 127.0.0.1:16001 VERIFY OK: depth=1, C=KG, ST=NA,
L=BISHKEK, O=OpenVPN-TEST, ema...@my...
Wed Aug  5 17:25:28 2020 127.0.0.1:16001 VERIFY ERROR: depth=0,
subject=C=KG, ST=NA, O=OpenVPN-TEST, CN=client-revoked,
ema...@my..., serial=3: The certificate has been revoked
(is on a CRL)
Wed Aug  5 17:25:28 2020 127.0.0.1:16001 TLS_ERROR: read tls_read_plaintext
error: X509 - Certificate verification failed, e.g. CRL, CA or signature
check failed

touch sample-keys/3, --crl-verify sample-keys/ dir
Wed Aug  5 17:18:12 2020 127.0.0.1:16001 VERIFY OK: depth=1, C=KG, ST=NA,
L=BISHKEK, O=OpenVPN-TEST, ema...@my...
Wed Aug  5 17:18:12 2020 127.0.0.1:16001 VERIFY CRL: depth=0, C=KG, ST=NA,
O=OpenVPN-TEST, CN=client-revoked, ema...@my..., serial=3
is revoked
Wed Aug  5 17:18:12 2020 127.0.0.1:16001 OpenSSL: error:1417C086:SSL
routines:tls_process_client_certificate:certificate verify failed
Wed Aug  5 17:18:12 2020 127.0.0.1:16001 TLS_ERROR: BIO read
tls_read_plaintext error

--
Best Regards, Vladislav Grishenko

-----Original Message-----
From: Gert Doering <ge...@gr...> 
Sent: Wednesday, August 5, 2020 4:55 PM
To: Vladislav Grishenko <the...@ya...>
Cc: ope...@li...
Subject: [PATCH applied] Re: Log serial number of revoked certificate

Your patch has been applied to the master branch.

I have not done much testing, just a test run "make check" on an OpenSSL and
mbedTLS build.

I have not looked into applying it to "release/2.4" - if you think it's
needed, let me know (or if it needs more work because the code has diverged
too much, send a 2.4 patch) - thanks.

commit 992e9cec40539a155afa9eae10502aa62f617965
Author: Vladislav Grishenko
Date:   Wed Aug 5 15:23:33 2020 +0500

     Log serial number of revoked certificate

     Signed-off-by: Vladislav Grishenko <the...@ya...>
     Acked-by: Lev Stipakov <lst...@gm...>
     Message-Id: <202...@ya...>
     URL:
https://www.mail-archive.com/ope...@li.../msg20642.ht
ml
     Signed-off-by: Gert Doering <ge...@gr...>


--
kind regards,

Gert Doering

[Openvpn-devel] [PATCH applied] Re: Log serial number of revoked certificate

[Gert D. <ge...@gr...>]

Your patch has been applied to the master branch.

I have not done much testing, just a test run "make check" on an
OpenSSL and mbedTLS build.

I have not looked into applying it to "release/2.4" - if you think 
it's needed, let me know (or if it needs more work because the code
has diverged too much, send a 2.4 patch) - thanks.

commit 992e9cec40539a155afa9eae10502aa62f617965
Author: Vladislav Grishenko
Date:   Wed Aug 5 15:23:33 2020 +0500

     Log serial number of revoked certificate

     Signed-off-by: Vladislav Grishenko <the...@ya...>
     Acked-by: Lev Stipakov <lst...@gm...>
     Message-Id: <202...@ya...>
     URL: https://www.mail-archive.com/ope...@li.../msg20642.html
     Signed-off-by: Gert Doering <ge...@gr...>


--
kind regards,

Gert Doering

Re: [Openvpn-devel] [PATCH v2] Rework NCP compability logic and drop BF-CBC support by default

[Gert D. <ge...@gr...>]

Hi,

On Wed, Aug 05, 2020 at 08:43:18AM +0200, Gert Doering wrote:
> Test run with "cipher bf-cbc" in all server configs next...

For completeness, this works nicely:

start client jobs...
22...
Test sets succeeded: 1 2 3 4 6 8.
Test sets failed: none.
23.small...
Test sets succeeded: 1 2 3 4.
Test sets failed: none.
23...
Test sets succeeded: 1 1a 1b 1d 2 2a 2b 2c 2d 3 4 5 6 8 8a 9.
Test sets failed: none.
24...
Test sets succeeded: 1 1a 1b 1c 1d 1e 2 2a 2b 2c 2d 2e 3 4 4a 5 6 8 8a 9.
Test sets failed: none.
master...
Test sets succeeded: 1 1a 1b 1c 1d 1e 2 2a 2b 2c 2d 2e 3 4 5 5a 5b 5c 5d 5v1 5v2 5v3 5w1 5w2 5w3 5w4 5x1 5x2 5x3 5x4 6 7 7x 8 8a 9 2f 4b.
Test sets failed: none.


so, if we break someone's existing server setup, the answer is

 "put 'fallback-cipher BF-CBC' into your config!"

(or 'cipher BF-CBC' explicitly, so it's 2.4-compatible)


Not sure how to tackle the "ccd/ push cipher is broken now with 2.4-NCP
clients" part.  I think this is useful functionality, but the current
patch does not allow this "unless the client is already using the to-be-
pushed cipher, or it's one of the two NCP=2 AEAD ciphers".  Which makes
it slightly less than useful...

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                             ge...@gr...

Re: [Openvpn-devel] [PATCH v2] Log serial number of revoked certificate

[Lev S. <lst...@gm...>]

Code looks good. Compiled and tested (openssl and revoked cert) on Ubuntu 20.04.

Acked-by: Lev Stipakov <lst...@gm...>

[Openvpn-devel] [PATCH v2] Log serial number of revoked certificate

[Vladislav G. <the...@ya...>]

As it appears commit 767e4c56becbfeea525e4695a810593f373883cd "Log
serial number of revoked certificate" hasn't survive refactoring
of CRL handling.

In most of situations admin of OpenVPN server needs to know which
particular certificate is used by client.
In the case when certificate is valid, environment variable can be
used for that but once it is revoked, no user scripts are invoked
so there is no way to get serial number, only subject is logged.

Let's log certificate serial in case it is revoked and additionally
log certificate depth & subject in crl-verify "dir" mode for better
consistency with crl file (non-dir) mode.

v2: log if serial is not availble, require it in crl-verify dir mode

Signed-off-by: Vladislav Grishenko <the...@ya...>
---
 src/openvpn/ssl_verify.c         | 14 +++++++++++---
 src/openvpn/ssl_verify_mbedtls.c |  5 +++--
 src/openvpn/ssl_verify_openssl.c |  5 +++--
 3 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c
index 844bc57d..97ccb93b 100644
--- a/src/openvpn/ssl_verify.c
+++ b/src/openvpn/ssl_verify.c
@@ -599,7 +599,8 @@ cleanup:
  * check peer cert against CRL directory
  */
 static result_t
-verify_check_crl_dir(const char *crl_dir, openvpn_x509_cert_t *cert)
+verify_check_crl_dir(const char *crl_dir, openvpn_x509_cert_t *cert,
+                     const char *subject, int cert_depth)
 {
     result_t ret = FAILURE;
     char fn[256];
@@ -607,6 +608,12 @@ verify_check_crl_dir(const char *crl_dir, openvpn_x509_cert_t *cert)
     struct gc_arena gc = gc_new();
 
     char *serial = backend_x509_get_serial(cert, &gc);
+    if (!serial)
+    {
+        msg(D_HANDSHAKE, "VERIFY CRL: depth=%d, %s, serial number is not available",
+            cert_depth, subject);
+        goto cleanup;
+    }
 
     if (!openvpn_snprintf(fn, sizeof(fn), "%s%c%s", crl_dir, OS_SPECIFIC_DIRSEP, serial))
     {
@@ -616,7 +623,8 @@ verify_check_crl_dir(const char *crl_dir, openvpn_x509_cert_t *cert)
     fd = platform_open(fn, O_RDONLY, 0);
     if (fd >= 0)
     {
-        msg(D_HANDSHAKE, "VERIFY CRL: certificate serial number %s is revoked", serial);
+        msg(D_HANDSHAKE, "VERIFY CRL: depth=%d, %s, serial=%s is revoked",
+            cert_depth, subject, serial);
         goto cleanup;
     }
 
@@ -758,7 +766,7 @@ verify_cert(struct tls_session *session, openvpn_x509_cert_t *cert, int cert_dep
     {
         if (opt->ssl_flags & SSLF_CRL_VERIFY_DIR)
         {
-            if (SUCCESS != verify_check_crl_dir(opt->crl_file, cert))
+            if (SUCCESS != verify_check_crl_dir(opt->crl_file, cert, subject, cert_depth))
             {
                 goto cleanup;
             }
diff --git a/src/openvpn/ssl_verify_mbedtls.c b/src/openvpn/ssl_verify_mbedtls.c
index fd31bbbd..93891038 100644
--- a/src/openvpn/ssl_verify_mbedtls.c
+++ b/src/openvpn/ssl_verify_mbedtls.c
@@ -68,6 +68,7 @@ verify_callback(void *session_obj, mbedtls_x509_crt *cert, int cert_depth,
         int ret = 0;
         char errstr[512] = { 0 };
         char *subject = x509_get_subject(cert, &gc);
+        char *serial = backend_x509_get_serial(cert, &gc);
 
         ret = mbedtls_x509_crt_verify_info(errstr, sizeof(errstr)-1, "", *flags);
         if (ret <= 0 && !openvpn_snprintf(errstr, sizeof(errstr),
@@ -82,8 +83,8 @@ verify_callback(void *session_obj, mbedtls_x509_crt *cert, int cert_depth,
 
         if (subject)
         {
-            msg(D_TLS_ERRORS, "VERIFY ERROR: depth=%d, subject=%s: %s",
-                cert_depth, subject, errstr);
+            msg(D_TLS_ERRORS, "VERIFY ERROR: depth=%d, subject=%s, serial=%s: %s",
+                cert_depth, subject, serial ? serial : "<not available>", errstr);
         }
         else
         {
diff --git a/src/openvpn/ssl_verify_openssl.c b/src/openvpn/ssl_verify_openssl.c
index ff14db23..454efeec 100644
--- a/src/openvpn/ssl_verify_openssl.c
+++ b/src/openvpn/ssl_verify_openssl.c
@@ -71,6 +71,7 @@ verify_callback(int preverify_ok, X509_STORE_CTX *ctx)
     {
         /* get the X509 name */
         char *subject = x509_get_subject(current_cert, &gc);
+        char *serial = backend_x509_get_serial(current_cert, &gc);
 
         if (!subject)
         {
@@ -89,10 +90,10 @@ verify_callback(int preverify_ok, X509_STORE_CTX *ctx)
         }
 
         /* Remote site specified a certificate, but it's not correct */
-        msg(D_TLS_ERRORS, "VERIFY ERROR: depth=%d, error=%s: %s",
+        msg(D_TLS_ERRORS, "VERIFY ERROR: depth=%d, error=%s: %s, serial=%s",
             X509_STORE_CTX_get_error_depth(ctx),
             X509_verify_cert_error_string(X509_STORE_CTX_get_error(ctx)),
-            subject);
+            subject, serial ? serial : "<not available>");
 
         ERR_clear_error();
 
-- 
2.17.1

Re: [Openvpn-devel] [PATCH] Skip existing interfaces on opening the first available utun on macOS

[Lev S. <lst...@gm...>]

Hi,

 +            ASSERT(snprintf(ifname, sizeof(ifname), "utun%d", utunnum));

Not sure about ASSERT here, because according to
https://linux.die.net/man/3/snprintf

> If an output error is encountered, a negative value is returned.

which won't trigger assert.

Otherwise looks good.

Compiled and tested on macOS.

Maybe Gert could remove ASSERT wrapping before commiting.

Acked-by: Lev Stipakov <lst...@gm...>

Re: [Openvpn-devel] [PATCH] Log serial number of revoked certificate

[Vladislav G. <the...@ya...>]

Hi, Lev
Thanks for review, I'll make improvements in V2. 

--
Best Regards, Vladislav Grishenko

-----Original Message-----
From: Lev Stipakov <lst...@gm...> 
Sent: Wednesday, August 5, 2020 1:29 PM
To: Vladislav Grishenko <the...@ya...>
Cc: openvpn-devel <ope...@li...>
Subject: Re: [Openvpn-devel] [PATCH] Log serial number of revoked certificate

Hi,

Compiled and tested on Ubuntu 20.04, looks good.

A few nit-picks:

> +verify_check_crl_dir(const char *crl_dir, int cert_depth, 
> +openvpn_x509_cert_t *cert, char *subject)

The last parameter could benefit from const to indicate that function is not going to modify it.


> -        msg(D_HANDSHAKE, "VERIFY CRL: certificate serial number %s is revoked", serial);
> +        msg(D_HANDSHAKE, "VERIFY CRL: depth=%d, %s, serial=%s is revoked",
> +            cert_depth, subject, serial);

Since you are modifying this line, could you add a NULL check to serial to and pass something like "<not available>" in this case?


> +            msg(D_TLS_ERRORS, "VERIFY ERROR: depth=%d, subject=%s, serial=%s: %s",
> +                cert_depth, subject, serial ? serial : "", errstr);

I would use "<not available>" in NULL case, otherwise the error message becomes funny.


> +        msg(D_TLS_ERRORS, "VERIFY ERROR: depth=%d, error=%s: %s, 
> + serial=%s",
>              X509_STORE_CTX_get_error_depth(ctx),
>              X509_verify_cert_error_string(X509_STORE_CTX_get_error(ctx)),
> -            subject);
> +            subject, serial ? serial : "");

Same as above.

Re: [Openvpn-devel] [PATCH] Log serial number of revoked certificate

[Lev S. <lst...@gm...>]

Hi,

Compiled and tested on Ubuntu 20.04, looks good.

A few nit-picks:

> +verify_check_crl_dir(const char *crl_dir, int cert_depth, openvpn_x509_cert_t *cert, char *subject)

The last parameter could benefit from const to indicate that function
is not going to modify it.


> -        msg(D_HANDSHAKE, "VERIFY CRL: certificate serial number %s is revoked", serial);
> +        msg(D_HANDSHAKE, "VERIFY CRL: depth=%d, %s, serial=%s is revoked",
> +            cert_depth, subject, serial);

Since you are modifying this line, could you add a NULL check to
serial to and pass
something like "<not available>" in this case?


> +            msg(D_TLS_ERRORS, "VERIFY ERROR: depth=%d, subject=%s, serial=%s: %s",
> +                cert_depth, subject, serial ? serial : "", errstr);

I would use "<not available>" in NULL case, otherwise the error
message becomes funny.


> +        msg(D_TLS_ERRORS, "VERIFY ERROR: depth=%d, error=%s: %s, serial=%s",
>              X509_STORE_CTX_get_error_depth(ctx),
>              X509_verify_cert_error_string(X509_STORE_CTX_get_error(ctx)),
> -            subject);
> +            subject, serial ? serial : "");

Same as above.

Re: [Openvpn-devel] [PATCH v2] Rework NCP compability logic and drop BF-CBC support by default

[Gert D. <ge...@gr...>]

HI,

On Wed, Aug 05, 2020 at 12:20:54AM +0200, Arne Schwabe wrote:
> > Is that intentional?
> 
> Yes. That is intentional. If you do not have any cipher option in the
> config, there is nowadays a very high change that you allow BF-CBC by
> "accident". I encountered this first-hand ("I do want to put as few
> option in a config as possible").

OK, I can see that line of reasoning.  This needs to be put very
prominently into the release notes.


Updating on server test success - with the core dump fix, but still
*no* "--cipher" in the server configs, I get

22...
Test sets succeeded: 8.
Test sets failed: 1 2 3 4 6.
23.small...
Test sets succeeded: none.
Test sets failed: 1 2 3 4.
23...
Test sets succeeded: 8 8a 9.
Test sets failed: 1 1a 1b 1d 2 2a 2b 2c 2d 3 4 5 6.
24...
Test sets succeeded: 1 1a 1b 1c 1d 1e 2 2b 2d 2e 3 5 6 8 8a 9.
Test sets failed: 2a 2c 4 4a.
master...
Test sets succeeded: 1 1a 1b 1c 1d 1e 2 2b 2c 2d 2e 3 5 5a 5b 5c 5d 5v1 5v2 5v3 5w1 5w2 5w3 5w4 5x1 5x2 5x3 5x4 6 7 7x 8 8a 9 2f.
Test sets failed: 2a 4 4b.

so the 2.2/2.3-small/2.3 failures are expected.

2a and 2c (for 2.4) is expected, because that's "--ncp-disable"


4 got broken somewhat accidently - this is tap tests, relying on a 
secondary client to be connected to ping "across the tap".  That client
is using pushed ccd ciphers, which fails in interesting ways

Aug  5 08:39:33 gentoo tap-udp-p2mp[2418]: freebsd-74-amd64/2001:608:0:814::f000:3 PUSH: No common cipher between server and client. Server data-ciphers: 'CAMELLIA-128-CBC', client supported ciphers 'AES-256-GCM:AES-128-GCM'

but this error message is misleading - the client has 

  ncp-ciphers CAMELLIA-128-CBC:AES-256-GCM:AES-128-GCM

in its config, but it's a 2.4 client so cannot signal this to the master.


So we *will* break pushed ciphers to 2.4 client swith non-AEAD ciphers 
here.  Any idea how to tackle this?


Test run with "cipher bf-cbc" in all server configs next...

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                             ge...@gr...

[Openvpn-devel] [PATCH] Fix compilation with --disable-lzo and --disable-lz4

[Lev S. <lst...@gm...>]

struct compress_options is defined under USE_COMP, therefore
compilation fails when it is referenced without that define.

Since function show_compression_warning, which uses aforementioned
struct, is only called under USE_COMP, it is safe to wrap its definition
under USE_COMP, which fixes compilation issue.

Signed-off-by: Lev Stipakov <lst...@gm...>
---
 src/openvpn/options.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index bc256b18..1c246f4b 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -5085,6 +5085,7 @@ set_user_script(struct options *options,
 #endif
 }
 
+#ifdef USE_COMP
 static void
 show_compression_warning(struct compress_options *info)
 {
@@ -5103,6 +5104,7 @@ show_compression_warning(struct compress_options *info)
         }
     }
 }
+#endif
 
 static void
 add_option(struct options *options,
-- 
2.25.1

Re: [Openvpn-devel] [PATCH v2] Rework NCP compability logic and drop BF-CBC support by default

[Arne S. <ar...@rf...>]

> 
> I'm not exactly sure what the code *does*, TBH, but de-fusing the check
> into
> 
>     if (remote_cipher == NULL 
>         || (peer_info && strstr(peer_info, "IV_CIPHERS=") ))
> 
> makes it no longer crash (and also pass the unit test).


Yes that is the right fix. My test client client had a push-peer-info in
it so I didn't catch this. :(


> 
> This patch also breaks connections from "default 2.3" clients, though in a 
> different way:
> 
> Aug  4 21:56:25 gentoo tun-udp-p2mp-112-mask[25184]: cron2-freebsd-tc-amd64-23/194.97.140.21:48168 PUSH: No common cipher between server and client. Server data-ciphers: 'AES-256-GCM:AES-128-GCM', client supports cipher 'BF-CBC'
> ug  4 21:56:27 gentoo tun-udp-p2mp-112-mask[25184]: cron2-freebsd-tc-amd64-23/194.97.140.21:48168 SENT CONTROL [cron2-freebsd-tc-amd64-23]: 'AUTH_FAILED,Data channel cipher negotiation failed (no shared cipher)' (status=1)
> 
> this is a server that has *no* "--cipher" in its config, and a client
> that has nothing either and no NCP - so it advertises "OCC cipher bf-cbc",
> which is no longer accepted on the server.
> 
> Is that intentional?

Yes. That is intentional. If you do not have any cipher option in the
config, there is nowadays a very high change that you allow BF-CBC by
"accident". I encountered this first-hand ("I do want to put as few
option in a config as possible").

Since 2.4 only warns about SWEET32/BF-CBC being if you actually
negotiate it (i.e. talking to a 2.3 client/server), many of these
probably were not even aware that they allowed BF-CBC.

If you really want BF-CBC with the new patch you need to either add it
to data-ciphers or explicitly set --cipher BF-CBC (that adds it to
data-cipher and data-ciphers-fallback).

When I am back from vacation, I can send a patch with better wording to
Changes.rst:

Removal of BF-CBC support in default configuratio:
- By default OpenVPN 2.5 will only accept AES-256-GCM and AES-128-GCM as
data ciphers. OpenVPN 2.4 allows AES-256-GCM,AES-128-GCM and BF-CBC when
no --cipher and --ncp-ption were present. Accepting BF-CBC can be
enabled by adding

    data-ciphers AES-256-GCM:AES-128-GCM:BF-CBC

for very old peers also

    data-ciphers-fallback BF-CBC

to offer backwards compatiblity with older config an *explicit*

    cipher BF-CBC

in the configuration will be automatically translated in the two
commands above. We strongly recommend to switching away from BF-CBC to a
more secure cipher.

Arne

Re: [Openvpn-devel] [PATCH v2] Rework NCP compability logic and drop BF-CBC support by default

[Gert D. <ge...@gr...>]

Hi,

On Wed, Jul 29, 2020 at 01:38:35PM +0200, Arne Schwabe wrote:
> This reworks the NCP logic to be more strict about what is
> considered an acceptable result of an NCP negotiation. It also
> us to finally drop BF-CBC support by default.

I think the goals are good, but there are two corner cases in the
code that are not quite right yet

[ RUN      ] test_extract_client_ciphers
[       OK ] test_extract_client_ciphers
[ RUN      ] test_poor_man
[  ERROR   ] --- Test failed with exception: Segmentation fault(11)
[  FAILED  ] test_poor_man
[ RUN      ] test_ncp_best
[       OK ] test_ncp_best
[==========] 4 test(s) run.
[  PASSED  ] 3 test(s).
[  FAILED  ] 1 test(s), listed below:
[  FAILED  ] test_poor_man

 1 FAILED TEST(S)
FAIL: ncp_testdriver


... and unfortunately, it also segfaults when a 2.2 client connects - 
so, on my server test rig all openvpn processes are gone after 2.2 
has tested...

2020-08-04 22:05:05 us=274264 194.97.140.21:43906 TLS: Initial packet from [AF_INET6]::ffff:194.97.140.21:43906, sid=3c3a2afa adfd47fa
2020-08-04 22:05:05 us=390684 194.97.140.21:43906 VERIFY OK: depth=1, C=US, ST=California, L=Pleasanton, O=OpenVPN community project, CN=OpenVPN community project CA, ema...@op...
2020-08-04 22:05:05 us=390938 194.97.140.21:43906 VERIFY OK: depth=0, C=DE, ST=Bavaria, L=Munich, O=OpenVPN community project, CN=cron2-freebsd-tc-amd64-22, ??=Gert Doering, ema...@gr...
2020-08-04 22:05:05 us=604124 194.97.140.21:43906 Control Channel: TLSv1.0, cipher TLS-ECDHE-RSA-WITH-AES-256-CBC-SHA, 2048 bit key
2020-08-04 22:05:05 us=604229 194.97.140.21:43906 [cron2-freebsd-tc-amd64-22] Peer Connection Initiated with [AF_INET6]::ffff:194.97.140.21:43906
2020-08-04 22:05:05 us=604292 cron2-freebsd-tc-amd64-22/194.97.140.21:43906 MULTI_sva: pool returned IPv4=10.204.1.18, IPv6=fd00:abcd:204:1::1003
2020-08-04 22:05:05 us=604386 cron2-freebsd-tc-amd64-22/194.97.140.21:43906 mcccp1 (enter): ret=3, deferred=0
2020-08-04 22:05:05 us=604438 cron2-freebsd-tc-amd64-22/194.97.140.21:43906 MULTI: Learn: 10.204.1.18 -> cron2-freebsd-tc-amd64-22/194.97.140.21:43906
2020-08-04 22:05:05 us=604531 cron2-freebsd-tc-amd64-22/194.97.140.21:43906 MULTI: primary virtual IP for cron2-freebsd-tc-amd64-22/194.97.140.21:43906: 10.204.1.18
2020-08-04 22:05:05 us=604614 cron2-freebsd-tc-amd64-22/194.97.140.21:43906 MULTI: Learn: fd00:abcd:204:1::1003 -> cron2-freebsd-tc-amd64-22/194.97.140.21:43906
2020-08-04 22:05:05 us=604658 cron2-freebsd-tc-amd64-22/194.97.140.21:43906 MULTI: primary virtual IPv6 for cron2-freebsd-tc-amd64-22/194.97.140.21:43906: fd00:abcd:204:1::1003

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff7d9112d in ?? () from /lib64/libc.so.6
(gdb) where
#0  0x00007ffff7d9112d in ?? () from /lib64/libc.so.6
#1  0x00005555555d2299 in ncp_get_best_cipher (server_list=0x55555562bf78 "AES-256-GCM:AES-128-GCM", peer_info=peer_info@entry=0x0, 
    remote_cipher=0x55555566a800 "BF-CBC", gc=gc@entry=0x55555563de50) at ssl_ncp.c:231
#2  0x000055555558ea38 in multi_client_set_protocol_options (c=0x55555563de50) at multi.c:1833
#3  multi_client_connect_late_setup (option_types_found=<optimized out>, mi=0x55555563dc90, m=0x7fffffffc410) at multi.c:2434
#4  multi_connection_established (mi=0x55555563dc90, m=0x7fffffffc410) at multi.c:2672
#5  multi_process_post (m=m@entry=0x7fffffffc410, mi=0x55555563dc90, flags=flags@entry=9) at multi.c:2989
#6  0x000055555558f802 in multi_process_incoming_link (m=m@entry=0x7fffffffc410, instance=instance@entry=0x55555563dc90, 
    mpp_flags=mpp_flags@entry=9) at multi.c:3361


It seems to be failing on "peer_info" being "NULL" - this used to be 
optional in 2.2, and we only made it "always send something" in 2.3 with
some of the more interesting IV_ variables.  If I call the 2.2 client
with "--push-peer-info", it will no longer core dump the server, but
then fail with

2020-08-04 22:08:30 us=121232 cron2-freebsd-tc-amd64-22/194.97.140.21:10872 PUSH: No common cipher between server and client. Server data-ciphers: 'AES-256-GCM:AES-128-GCM', client supports cipher 'BF-CBC'

(as in the 2.3 case below)

The culprit is this code in ncp_get_best_cipher():

    if (remote_cipher == NULL || strstr(peer_info, "IV_CIPHERS="))

- that's the only place where we do not check for "peer_info != NULL"
before looking into it.

I'm not exactly sure what the code *does*, TBH, but de-fusing the check
into

    if (remote_cipher == NULL 
        || (peer_info && strstr(peer_info, "IV_CIPHERS=") ))

makes it no longer crash (and also pass the unit test).



This patch also breaks connections from "default 2.3" clients, though in a 
different way:

Aug  4 21:56:25 gentoo tun-udp-p2mp-112-mask[25184]: cron2-freebsd-tc-amd64-23/194.97.140.21:48168 PUSH: No common cipher between server and client. Server data-ciphers: 'AES-256-GCM:AES-128-GCM', client supports cipher 'BF-CBC'
ug  4 21:56:27 gentoo tun-udp-p2mp-112-mask[25184]: cron2-freebsd-tc-amd64-23/194.97.140.21:48168 SENT CONTROL [cron2-freebsd-tc-amd64-23]: 'AUTH_FAILED,Data channel cipher negotiation failed (no shared cipher)' (status=1)

this is a server that has *no* "--cipher" in its config, and a client
that has nothing either and no NCP - so it advertises "OCC cipher bf-cbc",
which is no longer accepted on the server.

Is that intentional?

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                             ge...@gr...

Re: [Openvpn-devel] [PATCH] Skip existing interfaces on opening the first available utun on macOS

[Gert D. <ge...@gr...>]

Hi,

On Fri, Jul 31, 2020 at 12:06:29PM +0200, Arne Schwabe wrote:
> This avoids the error messages trying to open already used utuns.
> 
> Signed-off-by: Arne Schwabe <ar...@rf...>

I think the code looks good, but I have no time right now to test it
(I can build MacOS binaries but have no test environment set up
"just now").

Jonathan, do you have time?

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                             ge...@gr...

[Openvpn-devel] [PATCH applied] Re: client-connect: Add documentation for the deferred client connect feature

[Gert D. <ge...@gr...>]

Acked-by: Gert Doering <ge...@gr...>

The code is long in.  I thought I'd see some review by "not me"
on the docs, but since the 2.5_beta1 cutoff date is coming soon,
I just did this now.  Included all the latest suggestions from
Richard as well.

I have reworded the "script-options.rst" section somewhat, because
I found it a bit unclear in regards to "what does a script/plugin 
have to do, when?" - since I struggled with this, I now know :-)

Your patch has been applied to the master branch.

commit 71d56aea895cc13aad06048066251979162db3f3
Author: Arne Schwabe
Date:   Mon Jul 20 16:27:03 2020 +0200

     client-connect: Add documentation for the deferred client connect feature

     Signed-off-by: David Sommerseth <da...@op...>
     Signed-off-by: Arne Schwabe <ar...@rf...>
     Acked-by: Gert Doering <ge...@gr...>
     Message-Id: <202...@rf...>
     URL: https://www.mail-archive.com/ope...@li.../msg20511.html
     Signed-off-by: Gert Doering <ge...@gr...>


--
kind regards,

Gert Doering

Re: [Openvpn-devel] MSI Installer: Add .ovpn file

[Илья Ш. <chi...@gm...>]

Hello,

we used to put *.ovpn files on special dedicated website (some guide +
installer downloads + ovpn).
works as charm

чт, 30 июл. 2020 г. в 13:37, Robert Grätz <gra...@cm...>:

> Hello,
>
> I am very happy that 2.5 will be hopefully soon released.
>
> I want to integrate my config file inside the msi installer. I think
> that romansi mentioned that there will be a proper solution for this
> issue. Are there any news or documentation yet? I tried something with
> Microsoft Orca [1], but it doesn't work yet.
>
> Best regards
>
> Robert
>
>
> _______________________________________________
> Openvpn-devel mailing list
> Ope...@li...
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel
>

Re: [Openvpn-devel] MSI Installer: Add .ovpn file

[Samuli S. <sa...@op...>]

Hi,

I have not developed the MSI installer and know only the basics about
MSI. Simon Rozman (rozmansi) is the right person to ask about this.

That said, I think using a "Merge Module" might be the way forward. The
tap-windows6 "installer" is implemented as such (MSM).

Samuli

Il 30/07/20 11:21, Robert Grätz ha scritto:
> Hello,
> 
> I am very happy that 2.5 will be hopefully soon released.
> 
> I want to integrate my config file inside the msi installer. I think
> that romansi mentioned that there will be a proper solution for this
> issue. Are there any news or documentation yet? I tried something with
> Microsoft Orca [1], but it doesn't work yet.
> 
> Best regards
> 
> Robert
> 
> 
> _______________________________________________
> Openvpn-devel mailing list
> Ope...@li...
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [PATCH] Skip existing interfaces on opening the first available utun on macOS

[Arne S. <ar...@rf...>]

This avoids the error messages trying to open already used utuns.

Signed-off-by: Arne Schwabe <ar...@rf...>
---
 src/openvpn/tun.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
index cc7b65cf..b9e444b6 100644
--- a/src/openvpn/tun.c
+++ b/src/openvpn/tun.c
@@ -3023,6 +3023,13 @@ open_darwin_utun(const char *dev, const char *dev_type, const char *dev_node, st
     {
         for (utunnum = 0; utunnum<255; utunnum++)
         {
+            char ifname[20];
+            /* if the interface exists silently skip it */
+            ASSERT(snprintf(ifname, sizeof(ifname), "utun%d", utunnum));
+            if (if_nametoindex(ifname))
+            {
+                continue;
+            }
             fd = utun_open_helper(ctlInfo, utunnum);
             /* Break if the fd is valid,
              * or if early initialization failed (-2) */
-- 
2.26.2

[Openvpn-devel] MSI Installer: Add .ovpn file

[Robert G. <gra...@cm...>]

Hello,

I am very happy that 2.5 will be hopefully soon released.

I want to integrate my config file inside the msi installer. I think 
that romansi mentioned that there will be a proper solution for this 
issue. Are there any news or documentation yet? I tried something with 
Microsoft Orca [1], but it doesn't work yet.

Best regards

Robert

[Openvpn-devel] [PATCH v2] Rework NCP compability logic and drop BF-CBC support by default

[Arne S. <ar...@rf...>]

This reworks the NCP logic to be more strict about what is
considered an acceptable result of an NCP negotiation. It also
us to finally drop BF-CBC support by default.

All new behaviour is currently limited to server/client
mode with pull enabled. P2p mode without pull does not change.

New Server behaviour:
- when a client announces its supported ciphers through either
  OCC or IV_CIPHER/IV_NCP we reject the client with a
  AUTH_FAILED message if we have no common cipher.

- When a client does not announce any cipher in either
  OCC or NCP we by reject it unless fallback-cipher is
  specified in either ccd or config.

New client behaviour:
- When no cipher is pushed (or a cipher we refused to support)
  and we also cannot support the server's server announced in
  OCC we fail the connection and log why

- If fallback-cipher is specified we will in the case that
  cipher is missing from occ use the fallback cipher instead
  of failing the connection

Both client and server behaviour:
- We only announce --cipher xyz in occ if we are willing
  to support that cipher.

  It means that we only announce the fallback-cipher if
  it is also contained in --data-ciphers

Compatiblity behaviour:

In 2.5 both client and server will automatically set
fallback-cipher xyz if --cipher xyz is in the config and
also add append the cipher to the end of data-ciphers.

We log a warn user about this and point to --data-ciphers and
--fallback-cipher. This also happens if the configuration
contains an explicit --cipher BF-CBC.

If --cipher is not set, we only warn that previous versions
allowed BF-CBC and point how to reenable BF-CBC. This might
break very few config where someone connects a very old
client to a 2.5 server but at some point we need to drop
the BF-CBC and those affected use will already have a the
scary SWEET32 warning in their logs.

In short: If --cipher is explicitly set 2.6 will work the same as
2.4 did. When --cipher is not set, BF-CBC support is dropped and
we warn about it.

Examples how breaking the default BF-CBC will be logged:

Client side:
 - Client connecting to server that does not push cipher but
   has --cipher in OCC

    OPTIONS ERROR: failed to negotiate cipher with server.  Add the server's cipher ('BF-CBC') to --data-ciphers (currently 'AES-256-GCM:AES-128-CBC') if you want to connect to this server.

 - Client connecting to a server that does not support OCC:

   OPTIONS ERROR: failed to negotioate cipher with server. Configure --fallback-cipher if you want connect to this server.

Server Side:

- Server has a client only supporting BF-CBC connecting:

  styx/IP PUSH: No common cipher between server and client. Server data-ciphers: 'CHACHA20-POLY1305:AES-128-GCM:AES-256-GCM:AES-256-CBC:AES-128-CBC', client supports cipher 'BF-CBC'.

 - Client without OCC:

   styx/IP PUSH:No NCP or OCC cipher data received from peer.
   styx/IP Use --fallback-cipher with the cipher the client is using if you want to allow the client to connect

In all reject cases on the client:

   AUTH: Received control message: AUTH_FAILED,Data channel cipher negotiation failed (no shared cipher)

Signed-off-by: Arne Schwabe <ar...@rf...>

Patch V2: rename fallback-cipher to data-ciphers-fallback
          add all corrections from Steffan
          Ignore occ cipher for clients sending IV_CIPHERS
          move client side ncp in its own function
          do not print INSECURE cipher warning if BF-CBC is not allowed

Signed-off-by: Arne Schwabe <ar...@rf...>
---
 doc/man-sections/protocol-options.rst |  22 ++++-
 src/openvpn/crypto.c                  |   4 +-
 src/openvpn/init.c                    |  18 ++--
 src/openvpn/multi.c                   | 135 ++++++++++++++++----------
 src/openvpn/options.c                 | 117 +++++++++++++++++-----
 src/openvpn/options.h                 |   2 +
 src/openvpn/ssl.c                     |  17 ++--
 src/openvpn/ssl_ncp.c                 |  82 +++++++++++++---
 src/openvpn/ssl_ncp.h                 |  18 ++--
 tests/unit_tests/openvpn/test_ncp.c   |  26 +++--
 10 files changed, 311 insertions(+), 130 deletions(-)

diff --git a/doc/man-sections/protocol-options.rst b/doc/man-sections/protocol-options.rst
index 051f1d32..69d3bc67 100644
--- a/doc/man-sections/protocol-options.rst
+++ b/doc/man-sections/protocol-options.rst
@@ -57,6 +57,9 @@ 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 and ``--data-ciphers``
+  or rarely `--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
@@ -183,8 +186,9 @@ configured in a compatible way between both the local and remote side.
   ``--server`` ), or if ``--pull`` is specified (client-side, implied by
   setting --client).
 
-  If both peers support and do not disable NCP, the negotiated cipher will
-  override the cipher specified by ``--cipher``.
+  If no common cipher is found during cipher negotiation, the  connection
+  is terminated. To support old clients/server that do not provide any cipher
+  negotiation support see ``data-ciphers-fallback``.
 
   Additionally, to allow for more smooth transition, if NCP is enabled,
   OpenVPN will inherit the cipher of the peer if that cipher is different
@@ -201,8 +205,18 @@ configured in a compatible way between both the local and remote side.
   This list is restricted to be 127 chars long after conversion to OpenVPN
   ciphers.
 
-  This option was called ``ncp-ciphers`` in OpenVPN 2.4 but has been renamed
-  to ``data-ciphers`` in OpenVPN 2.5 to more accurately reflect its meaning.
+  This option was called ``--ncp-ciphers`` in OpenVPN 2.4 but has been renamed
+  to ``--data-ciphers`` in OpenVPN 2.5 to more accurately reflect its meaning.
+
+--data-ciphers-fallback alg
+
+    Configure a cipher that is used to fall back to if we could not determine
+    which cipher the peer is willing to use.
+
+    This option should only be needed to
+    connect to peers that are running OpenVPN 2.3 and older version, and
+    have been configured with `--enable-small`
+    (typically used on routers or other embedded devices).
 
 --ncp-disable
   Disable "Negotiable Crypto Parameters". This completely disables cipher
diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
index e92a0dc1..3a0bfbec 100644
--- a/src/openvpn/crypto.c
+++ b/src/openvpn/crypto.c
@@ -727,7 +727,9 @@ warn_insecure_key_type(const char *ciphername, const cipher_kt_t *cipher)
     {
         msg(M_WARN, "WARNING: INSECURE cipher (%s) with block size less than 128"
             " bit (%d bit).  This allows attacks like SWEET32.  Mitigate by "
-            "using a --cipher with a larger block size (e.g. AES-256-CBC).",
+            "using a --cipher with a larger block size (e.g. AES-256-CBC). "
+            "Support for these insecure ciphers will be removed in "
+            "OpenVPN 2.6.",
             ciphername, cipher_kt_block_size(cipher)*8);
     }
 }
diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index 1ea4735d..402d2652 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -2365,16 +2365,9 @@ do_deferred_options(struct context *c, const unsigned int found)
     /* process (potentially pushed) crypto options */
     if (c->options.pull)
     {
-        struct tls_session *session = &c->c2.tls_multi->session[TM_ACTIVE];
-        if (found & OPT_P_NCP)
-        {
-            msg(D_PUSH, "OPTIONS IMPORT: data channel crypto options modified");
-        }
-        else if (c->options.ncp_enabled)
+        if (!check_pull_client_ncp(c, found))
         {
-            /* If the server did not push a --cipher, we will switch to the
-             * remote cipher if it is in our ncp-ciphers list */
-            tls_poor_mans_ncp(&c->options, c->c2.tls_multi->remote_ciphername);
+            return false;
         }
         struct frame *frame_fragment = NULL;
 #ifdef ENABLE_FRAGMENT
@@ -2384,6 +2377,7 @@ do_deferred_options(struct context *c, const unsigned int found)
         }
 #endif
 
+        struct tls_session *session = &c->c2.tls_multi->session[TM_ACTIVE];
         if (!tls_session_update_crypto_params(session, &c->options, &c->c2.frame,
                                               frame_fragment))
         {
@@ -2757,9 +2751,13 @@ do_init_crypto_tls_c1(struct context *c)
 #endif /* if P2MP */
         }
 
+        /* Do not warn if 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(&c->c1.ks.key_type, options->ciphername, options->authname,
-                      options->keysize, true, true);
+                      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/multi.c b/src/openvpn/multi.c
index 0f9c586b..79b5c0c3 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -1777,7 +1777,7 @@ multi_client_connect_setenv(struct multi_context *m,
  * - choosen cipher
  * - peer id
  */
-static void
+static bool
 multi_client_set_protocol_options(struct context *c)
 {
 
@@ -1807,56 +1807,85 @@ multi_client_set_protocol_options(struct context *c)
     }
 
     /* Select cipher if client supports Negotiable Crypto Parameters */
-    if (o->ncp_enabled)
+    if (!o->ncp_enabled)
     {
-        /* if we have already created our key, we cannot *change* our own
-         * cipher -> so log the fact and push the "what we have now" cipher
-         * (so the client is always told what we expect it to use)
-         */
-        const struct tls_session *session = &tls_multi->session[TM_ACTIVE];
-        if (session->key[KS_PRIMARY].crypto_options.key_ctx_bi.initialized)
+        return true;
+    }
+
+    /* if we have already created our key, we cannot *change* our own
+     * cipher -> so log the fact and push the "what we have now" cipher
+     * (so the client is always told what we expect it to use)
+     */
+    const struct tls_session *session = &tls_multi->session[TM_ACTIVE];
+    if (session->key[KS_PRIMARY].crypto_options.key_ctx_bi.initialized)
+    {
+        msg( M_INFO, "PUSH: client wants to negotiate cipher (NCP), but "
+             "server has already generated data channel keys, "
+             "re-sending previously negotiated cipher '%s'",
+             o->ciphername );
+        return true;
+    }
+
+    /*
+     * Push the first cipher from --data-ciphers to the client that
+     * the client announces to be supporting.
+     */
+    char *push_cipher = ncp_get_best_cipher(o->ncp_ciphers, peer_info,
+                                            tls_multi->remote_ciphername,
+                                            &o->gc);
+
+    if (push_cipher)
+    {
+        o->ciphername = push_cipher;
+        return true;
+    }
+
+    /* NCP cipher negotiation failed. Try to figure out why exactly it
+     * failed and give good error messages and potentially do a fallback
+     * for non NCP clients */
+    struct gc_arena gc = gc_new();
+    bool ret = false;
+
+    const char *peer_ciphers = tls_peer_ncp_list(peer_info, &gc);
+    /* If we are in a situation where we know the client ciphers, there is no
+     * reason to fall back to a cipher that will not be accepted by the other
+     * side, in this situation we fail the auth*/
+    if (strlen(peer_ciphers) > 0)
+    {
+        msg(M_INFO, "PUSH: No common cipher between server and client. "
+            "Server data-ciphers: '%s', client supported ciphers '%s'",
+            o->ncp_ciphers, peer_ciphers);
+    }
+    else if (tls_multi->remote_ciphername)
+    {
+        msg(M_INFO, "PUSH: No common cipher between server and client. "
+            "Server data-ciphers: '%s', client supports cipher '%s'",
+            o->ncp_ciphers, tls_multi->remote_ciphername);
+    }
+    else
+    {
+        msg(M_INFO, "PUSH: No NCP or OCC cipher data received from peer.");
+
+        if (o->enable_ncp_fallback && !tls_multi->remote_ciphername)
         {
-            msg( M_INFO, "PUSH: client wants to negotiate cipher (NCP), but "
-                 "server has already generated data channel keys, "
-                 "re-sending previously negotiated cipher '%s'",
-                 o->ciphername );
+            msg(M_INFO, "Using data channel cipher '%s' since "
+                "--data-ciphers-fallback is set.", o->ciphername);
+            ret = true;
         }
         else
         {
-            /*
-             * Push the first cipher from --data-ciphers to the client that
-             * the client announces to be supporting.
-             */
-            char *push_cipher = ncp_get_best_cipher(o->ncp_ciphers, o->ciphername,
-                                                    peer_info,
-                                                    tls_multi->remote_ciphername,
-                                                    &o->gc);
-
-            if (push_cipher)
-            {
-                o->ciphername = push_cipher;
-            }
-            else
-            {
-                struct gc_arena gc = gc_new();
-                const char *peer_ciphers = tls_peer_ncp_list(peer_info, &gc);
-                if (strlen(peer_ciphers) > 0)
-                {
-                    msg(M_INFO, "PUSH: No common cipher between server and "
-                        "client. Expect this connection not to work. Server "
-                        "data-ciphers: '%s', client supported ciphers '%s'",
-                        o->ncp_ciphers, peer_ciphers);
-                }
-                else
-                {
-                    msg(M_INFO, "No NCP data received from peer, falling back "
-                        "to --cipher '%s'. Peer reports in OCC --cipher '%s'",
-                        o->ciphername, np(tls_multi->remote_ciphername));
-                }
-                gc_free(&gc);
-            }
+            msg(M_INFO, "Use --data-ciphers-fallback with the cipher the "
+                "client is using if you want to allow the client to connect");
         }
     }
+    if (!ret)
+    {
+        auth_set_client_reason(tls_multi, "Data channel cipher negotiation "
+                                          "failed (no shared cipher)");
+    }
+
+    gc_free(&gc);
+    return ret;
 }
 
 /**
@@ -2322,7 +2351,7 @@ multi_client_connect_late_setup(struct multi_context *m,
     if (!mi->context.c2.push_ifconfig_defined)
     {
         msg(D_MULTI_ERRORS, "MULTI: no dynamic or static remote"
-            "--ifconfig address is available for %s",
+                            "--ifconfig address is available for %s",
             multi_instance_string(mi, false, &gc));
     }
 
@@ -2338,7 +2367,7 @@ multi_client_connect_late_setup(struct multi_context *m,
 
         /* JYFIXME -- this should cause the connection to fail */
         msg(D_MULTI_ERRORS, "MULTI ERROR: primary virtual IP for %s (%s)"
-            "violates tunnel network/netmask constraint (%s/%s)",
+                            "violates tunnel network/netmask constraint (%s/%s)",
             multi_instance_string(mi, false, &gc),
             print_in_addr_t(mi->context.c2.push_ifconfig_local, 0, &gc),
             ifconfig_constraint_network, ifconfig_constraint_netmask);
@@ -2387,7 +2416,7 @@ multi_client_connect_late_setup(struct multi_context *m,
     else if (mi->context.options.iroutes)
     {
         msg(D_MULTI_ERRORS, "MULTI: --iroute options rejected for %s -- iroute"
-            "only works with tun-style tunnels",
+                            "only works with tun-style tunnels",
             multi_instance_string(mi, false, &gc));
     }
 
@@ -2399,11 +2428,15 @@ multi_client_connect_late_setup(struct multi_context *m,
     mi->context.c2.context_auth = CAS_SUCCEEDED;
 
     /* authentication complete, calculate dynamic client specific options */
-    multi_client_set_protocol_options(&mi->context);
-
-    /* Generate data channel keys */
-    if (!multi_client_generate_tls_keys(&mi->context))
+    if (!multi_client_set_protocol_options(&mi->context))
+    {
+        mi->context.c2.context_auth = CAS_FAILED;
+    }
+    /* Generate data channel keys only if setting protocol options
+     * has not failed */
+    else if (!multi_client_generate_tls_keys(&mi->context))
     {
+
         mi->context.c2.context_auth = CAS_FAILED;
     }
 
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index bc256b18..c53ef7f9 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -855,7 +855,6 @@ init_options(struct options *o, const bool init_gc)
 #if P2MP
     o->scheduled_exit_interval = 5;
 #endif
-    o->ciphername = "BF-CBC";
     o->ncp_enabled = true;
     o->ncp_ciphers = "AES-256-GCM:AES-128-GCM";
     o->authname = "SHA1";
@@ -2053,7 +2052,7 @@ options_postprocess_verify_ce(const struct options *options, const struct connec
     if (options->inetd)
     {
         msg(M_WARN, "DEPRECATED OPTION: --inetd mode is deprecated "
-                    "and will be removed in OpenVPN 2.6");
+            "and will be removed in OpenVPN 2.6");
     }
 
     if (options->lladdr && dev != DEV_TYPE_TAP)
@@ -3046,6 +3045,67 @@ options_postprocess_verify(const struct options *o)
     }
 }
 
+static void
+options_postprocess_cipher(struct options *o)
+{
+    if (!o->pull && !(o->mode == MODE_SERVER))
+    {
+        /* we are in the classic P2P mode */
+        o->ncp_enabled = false;
+        msg( M_WARN, "Cipher negotiation is disabled since neither "
+             "P2MP client nor server mode is enabled");
+
+        /* If the cipher is not set, use the old default ofo BF-CBC. We will
+         * warn that this is deprecated on cipher initialisation, no need
+         * to warn here as well */
+        if (!o->ciphername)
+        {
+            o->ciphername = "BF-CBC";
+        }
+        return;
+    }
+
+    /* pull or P2MP mode */
+    if (!o->ciphername)
+    {
+        if (!o->ncp_enabled)
+        {
+            msg(M_USAGE, "--ncp-disable needs an explicit --cipher or "
+                         "--data-ciphers-fallback config option");
+        }
+
+        msg(M_WARN, "--cipher is not set. Previous OpenVPN version 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.");
+
+        /* 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";
+    }
+    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;
+    }
+}
+
 static void
 options_postprocess_mutate(struct options *o)
 {
@@ -3058,6 +3118,7 @@ options_postprocess_mutate(struct options *o)
     helper_keepalive(o);
     helper_tcp_nodelay(o);
 
+    options_postprocess_cipher(o);
     options_postprocess_mutate_invariant(o);
 
     if (o->ncp_enabled)
@@ -3118,16 +3179,6 @@ options_postprocess_mutate(struct options *o)
             "include this in your server configuration");
         o->dh_file = NULL;
     }
-
-    /* cipher negotiation (NCP) currently assumes --pull or --mode server */
-    if (o->ncp_enabled
-        && !(o->pull || o->mode == MODE_SERVER) )
-    {
-        msg( M_WARN, "disabling NCP mode (--ncp-disable) because not "
-             "in P2MP client or server mode" );
-        o->ncp_enabled = false;
-    }
-
 #if ENABLE_MANAGEMENT
     if (o->http_proxy_override)
     {
@@ -3663,14 +3714,21 @@ options_string(const struct options *o,
      */
 
     buf_printf(&out, ",dev-type %s", dev_type_string(o->dev, o->dev_type));
-    buf_printf(&out, ",link-mtu %u", (unsigned int) calc_options_string_link_mtu(o, frame));
+    /* the link-mtu that we send has only a meaning if have a fixed
+     * cipher (p2p) or have a fallback cipher configured for older non
+     * ncp clients. But not sending it, will make even 2.4 complain
+     * about it missing. So still send it. */
+    buf_printf(&out, ",link-mtu %u",
+               (unsigned int) calc_options_string_link_mtu(o, frame));
+
     buf_printf(&out, ",tun-mtu %d", PAYLOAD_SIZE(frame));
     buf_printf(&out, ",proto %s",  proto_remote(o->ce.proto, remote));
 
+    bool p2p_nopull = o->mode == MODE_POINT_TO_POINT && !PULL_DEFINED(o);
     /* send tun_ipv6 only in peer2peer mode - in client/server mode, it
      * is usually pushed by the server, triggering a non-helpful warning
      */
-    if (o->ifconfig_ipv6_local && o->mode == MODE_POINT_TO_POINT && !PULL_DEFINED(o))
+    if (o->ifconfig_ipv6_local && p2p_nopull)
     {
         buf_printf(&out, ",tun-ipv6");
     }
@@ -3700,7 +3758,7 @@ options_string(const struct options *o,
         }
     }
 
-    if (tt && o->mode == MODE_POINT_TO_POINT && !PULL_DEFINED(o))
+    if (tt && p2p_nopull)
     {
         const char *ios = ifconfig_options_string(tt, remote, o->ifconfig_nowarn, gc);
         if (ios && strlen(ios))
@@ -3756,8 +3814,14 @@ options_string(const struct options *o,
 
         init_key_type(&kt, o->ciphername, o->authname, o->keysize, true,
                       false);
-
-        buf_printf(&out, ",cipher %s", cipher_kt_name(kt.cipher));
+        /* Only announce the cipher to our peer if we are willing to
+         * support it */
+        const char *ciphername = cipher_kt_name(kt.cipher);
+        if (p2p_nopull || !o->ncp_enabled
+            || tls_item_in_cipher_list(ciphername, o->ncp_ciphers))
+        {
+            buf_printf(&out, ",cipher %s", ciphername);
+        }
         buf_printf(&out, ",auth %s", md_kt_name(kt.digest));
         buf_printf(&out, ",keysize %d", kt.cipher_length * 8);
         if (o->shared_secret_file)
@@ -3875,7 +3939,8 @@ options_warning_safe_scan2(const int msglevel,
         || strprefix(p1, "keydir ")
         || strprefix(p1, "proto ")
         || strprefix(p1, "tls-auth ")
-        || strprefix(p1, "tun-ipv6"))
+        || strprefix(p1, "tun-ipv6")
+        || strprefix(p1, "cipher "))
     {
         return;
     }
@@ -7863,14 +7928,20 @@ add_option(struct options *options,
         VERIFY_PERMISSION(OPT_P_NCP|OPT_P_INSTANCE);
         options->ciphername = p[1];
     }
+    else if (streq(p[0], "data-ciphers-fallback") && p[1] && !p[2])
+    {
+        VERIFY_PERMISSION(OPT_P_GENERAL|OPT_P_INSTANCE);
+        options->ciphername = p[1];
+        options->enable_ncp_fallback = true;
+    }
     else if ((streq(p[0], "data-ciphers") || streq(p[0], "ncp-ciphers"))
-            && p[1] && !p[2])
+             && p[1] && !p[2])
     {
         VERIFY_PERMISSION(OPT_P_GENERAL|OPT_P_INSTANCE);
         if (streq(p[0], "ncp-ciphers"))
         {
             msg(M_INFO, "Note: Treating option '--ncp-ciphers' as "
-                        " '--data-ciphers' (renamed in OpenVPN 2.5).");
+                " '--data-ciphers' (renamed in OpenVPN 2.5).");
         }
         options->ncp_ciphers = p[1];
     }
@@ -7878,9 +7949,9 @@ add_option(struct options *options,
     {
         VERIFY_PERMISSION(OPT_P_GENERAL|OPT_P_INSTANCE);
         options->ncp_enabled = false;
-        msg(M_WARN, "DEPRECATED OPTION: ncp-disable. Disabling dynamic "
-                    "cipher negotiation is a deprecated debug feature that "
-                    "will be removed in OpenVPN 2.6");
+        msg(M_WARN, "DEPRECATED OPTION: ncp-disable. Disabling "
+            "cipher negotiation is a deprecated debug feature that "
+            "will be removed in OpenVPN 2.6");
     }
     else if (streq(p[0], "prng") && p[1] && !p[3])
     {
diff --git a/src/openvpn/options.h b/src/openvpn/options.h
index c5df2d18..877e9396 100644
--- a/src/openvpn/options.h
+++ b/src/openvpn/options.h
@@ -503,6 +503,8 @@ struct options
     bool shared_secret_file_inline;
     int key_direction;
     const char *ciphername;
+    bool enable_ncp_fallback;      /**< If defined fall back to
+                                    * ciphername if NCP fails */
     bool ncp_enabled;
     const char *ncp_ciphers;
     const char *authname;
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 91ab3bf6..06dc9f8f 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -1932,13 +1932,14 @@ tls_session_update_crypto_params(struct tls_session *session,
         return true;
     }
 
-    if (!session->opt->server
-        && 0 != strcmp(options->ciphername, session->opt->config_ciphername)
+    bool cipher_allowed_as_fallback = options->enable_ncp_fallback
+                                      && streq(options->ciphername, session->opt->config_ciphername);
+
+    if (!session->opt->server && !cipher_allowed_as_fallback
         && !tls_item_in_cipher_list(options->ciphername, options->ncp_ciphers))
     {
-        msg(D_TLS_ERRORS, "Error: pushed cipher not allowed - %s not in %s or %s",
-            options->ciphername, session->opt->config_ciphername,
-            options->ncp_ciphers);
+        msg(D_TLS_ERRORS, "Error: pushed cipher not allowed - %s not in %s",
+            options->ciphername, options->ncp_ciphers);
         /* undo cipher push, abort connection setup */
         options->ciphername = session->opt->config_ciphername;
         return false;
@@ -1956,9 +1957,9 @@ tls_session_update_crypto_params(struct tls_session *session,
     }
     else
     {
-      /* Very hacky workaround and quick fix for frame calculation
-       * different when adjusting frame size when the original and new cipher
-       * are identical to avoid a regression with client without NCP */
+        /* Very hacky workaround and quick fix for frame calculation
+         * different when adjusting frame size when the original and new cipher
+         * are identical to avoid a regression with client without NCP */
         return tls_session_generate_data_channel_keys(session);
     }
 
diff --git a/src/openvpn/ssl_ncp.c b/src/openvpn/ssl_ncp.c
index 8e432fb7..2d3983c2 100644
--- a/src/openvpn/ssl_ncp.c
+++ b/src/openvpn/ssl_ncp.c
@@ -48,6 +48,7 @@
 #include "common.h"
 
 #include "ssl_ncp.h"
+#include "openvpn.h"
 
 /**
  * Return the Negotiable Crypto Parameters version advertised in the peer info
@@ -211,9 +212,8 @@ tls_peer_ncp_list(const char *peer_info, struct gc_arena *gc)
 }
 
 char *
-ncp_get_best_cipher(const char *server_list, const char *server_cipher,
-                    const char *peer_info,  const char *remote_cipher,
-                    struct gc_arena *gc)
+ncp_get_best_cipher(const char *server_list, const char *peer_info,
+                    const char *remote_cipher, struct gc_arena *gc)
 {
     /*
      * The gc of the parameter is tied to the VPN session, create a
@@ -226,7 +226,9 @@ ncp_get_best_cipher(const char *server_list, const char *server_cipher,
     const char *peer_ncp_list = tls_peer_ncp_list(peer_info, &gc_tmp);
 
     /* non-NCP client without OCC?  "assume nothing" */
-    if (remote_cipher == NULL)
+    /* For client doing the newer version of NCP (that send IV_CIPHER)
+     * we cannot assume that they will accept remote_cipher */
+    if (remote_cipher == NULL || strstr(peer_info, "IV_CIPHERS="))
     {
         remote_cipher = "";
     }
@@ -242,15 +244,6 @@ ncp_get_best_cipher(const char *server_list, const char *server_cipher,
             break;
         }
     }
-    /* We have not found a common cipher, as a last resort check if the
-     * server cipher can be used
-     */
-    if (token == NULL
-        && (tls_item_in_cipher_list(server_cipher, peer_ncp_list)
-            || streq(server_cipher, remote_cipher)))
-    {
-        token = server_cipher;
-    }
 
     char *ret = NULL;
     if (token != NULL)
@@ -262,16 +255,75 @@ ncp_get_best_cipher(const char *server_list, const char *server_cipher,
     return ret;
 }
 
-void
+/**
+ * "Poor man's NCP": Use peer cipher if it is an allowed (NCP) cipher.
+ * Allows non-NCP peers to upgrade their cipher individually.
+ *
+ * Returns true if we switched to the peer's cipher
+ *
+ * Make sure to call tls_session_update_crypto_params() after calling this
+ * function.
+ */
+static bool
 tls_poor_mans_ncp(struct options *o, const char *remote_ciphername)
 {
-    if (o->ncp_enabled && remote_ciphername
+    if (remote_ciphername
         && 0 != strcmp(o->ciphername, remote_ciphername))
     {
         if (tls_item_in_cipher_list(remote_ciphername, o->ncp_ciphers))
         {
             o->ciphername = string_alloc(remote_ciphername, &o->gc);
             msg(D_TLS_DEBUG_LOW, "Using peer cipher '%s'", o->ciphername);
+            return true;
         }
     }
+    return false;
 }
+
+bool
+check_pull_client_ncp(struct context *c, const int found)
+{
+    if (found & OPT_P_NCP)
+    {
+        msg(D_PUSH, "OPTIONS IMPORT: data channel crypto options modified");
+        return true;
+    }
+
+    if (!c->options.ncp_enabled)
+    {
+        return true;
+    }
+    /* If the server did not push a --cipher, we will switch to the
+     * remote cipher if it is in our ncp-ciphers list */
+    bool useremotecipher = tls_poor_mans_ncp(&c->options,
+                                             c->c2.tls_multi->remote_ciphername);
+
+
+    /* We could not figure out the peer's cipher but we have fallback
+     * enable */
+    if (!useremotecipher && c->options.enable_ncp_fallback)
+    {
+        return true;
+    }
+
+    /* We failed negotiation, give appropiate error message */
+    if (c->c2.tls_multi->remote_ciphername)
+    {
+        msg(D_TLS_ERRORS, "OPTIONS ERROR: failed to negotiate "
+            "cipher with server.  Add the server's "
+            "cipher ('%s') to --data-ciphers (currently '%s') if "
+            "you want to connect to this server.",
+            c->c2.tls_multi->remote_ciphername,
+            c->options.ncp_ciphers);
+        return false;
+
+    }
+    else
+    {
+        msg(D_TLS_ERRORS, "OPTIONS ERROR: failed to negotiate "
+            "cipher with server. Configure "
+            "--data-ciphers-fallback if you want to connect "
+            "to this server.");
+        return false;
+    }
+}
\ No newline at end of file
diff --git a/src/openvpn/ssl_ncp.h b/src/openvpn/ssl_ncp.h
index d00c222d..39158a56 100644
--- a/src/openvpn/ssl_ncp.h
+++ b/src/openvpn/ssl_ncp.h
@@ -40,14 +40,17 @@
 bool
 tls_peer_supports_ncp(const char *peer_info);
 
+/* forward declaration to break include dependency loop */
+struct context;
+
 /**
- * "Poor man's NCP": Use peer cipher if it is an allowed (NCP) cipher.
- * Allows non-NCP peers to upgrade their cipher individually.
+ * Checks whether the cipher negotiation is in an acceptable state
+ * and we continue to connect or should abort.
  *
- * Make sure to call tls_session_update_crypto_params() after calling this
- * function.
+ * @return  Wether the client NCP process suceeded or failed
  */
-void tls_poor_mans_ncp(struct options *o, const char *remote_ciphername);
+bool
+check_pull_client_ncp(struct context *c, int found);
 
 /**
  * Iterates through the ciphers in server_list and return the first
@@ -67,9 +70,8 @@ void tls_poor_mans_ncp(struct options *o, const char *remote_ciphername);
  * cipher
  */
 char *
-ncp_get_best_cipher(const char *server_list, const char *server_cipher,
-                    const char *peer_info, const char *remote_cipher,
-                    struct gc_arena *gc);
+ncp_get_best_cipher(const char *server_list, const char *peer_info,
+                    const char *remote_cipher, struct gc_arena *gc);
 
 
 /**
diff --git a/tests/unit_tests/openvpn/test_ncp.c b/tests/unit_tests/openvpn/test_ncp.c
index 19432410..ea950030 100644
--- a/tests/unit_tests/openvpn/test_ncp.c
+++ b/tests/unit_tests/openvpn/test_ncp.c
@@ -139,21 +139,29 @@ test_poor_man(void **state)
     char *best_cipher;
 
     const char *serverlist = "CHACHA20_POLY1305:AES-128-GCM";
+    const char *serverlistbfcbc = "CHACHA20_POLY1305:AES-128-GCM:BF-CBC";
 
-    best_cipher = ncp_get_best_cipher(serverlist, "BF-CBC",
+    best_cipher = ncp_get_best_cipher(serverlist,
+                                      "IV_YOLO=NO\nIV_BAR=7",
+                                      "BF-CBC", &gc);
+
+    assert_ptr_equal(best_cipher, NULL);
+
+
+    best_cipher = ncp_get_best_cipher(serverlistbfcbc,
                                       "IV_YOLO=NO\nIV_BAR=7",
                                       "BF-CBC", &gc);
 
     assert_string_equal(best_cipher, "BF-CBC");
 
-    best_cipher = ncp_get_best_cipher(serverlist, "BF-CBC",
+
+    best_cipher = ncp_get_best_cipher(serverlist,
                                       "IV_NCP=1\nIV_BAR=7",
                                       "AES-128-GCM", &gc);
 
     assert_string_equal(best_cipher, "AES-128-GCM");
 
-    best_cipher = ncp_get_best_cipher(serverlist, "BF-CBC",
-                                      NULL,
+    best_cipher = ncp_get_best_cipher(serverlist, NULL,
                                       "AES-128-GCM", &gc);
 
     assert_string_equal(best_cipher, "AES-128-GCM");
@@ -170,29 +178,27 @@ test_ncp_best(void **state)
 
     const char *serverlist = "CHACHA20_POLY1305:AES-128-GCM:AES-256-GCM";
 
-    best_cipher = ncp_get_best_cipher(serverlist, "BF-CBC",
+    best_cipher = ncp_get_best_cipher(serverlist,
                                       "IV_YOLO=NO\nIV_NCP=2\nIV_BAR=7",
                                       "BF-CBC", &gc);
 
     assert_string_equal(best_cipher, "AES-128-GCM");
 
     /* Best cipher is in --cipher of client */
-    best_cipher = ncp_get_best_cipher(serverlist, "BF-CBC",
-                                      "IV_NCP=2\nIV_BAR=7",
+    best_cipher = ncp_get_best_cipher(serverlist, "IV_NCP=2\nIV_BAR=7",
                                       "CHACHA20_POLY1305", &gc);
 
     assert_string_equal(best_cipher, "CHACHA20_POLY1305");
 
     /* Best cipher is in --cipher of client */
-    best_cipher = ncp_get_best_cipher(serverlist, "BF-CBC",
-                                      "IV_CIPHERS=AES-128-GCM",
+    best_cipher = ncp_get_best_cipher(serverlist, "IV_CIPHERS=AES-128-GCM",
                                       "AES-256-CBC", &gc);
 
 
     assert_string_equal(best_cipher, "AES-128-GCM");
 
     /* IV_NCP=2 should be ignored if IV_CIPHERS is sent */
-    best_cipher = ncp_get_best_cipher(serverlist, "BF-CBC",
+    best_cipher = ncp_get_best_cipher(serverlist,
                                       "IV_FOO=7\nIV_CIPHERS=AES-256-GCM\nIV_NCP=2",
                                       "AES-256-CBC", &gc);
 
-- 
2.26.2