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

Message ID 20200729113835.2415-1-arne@rfc2549.org
State Superseded
Headers show
Series
  • [Openvpn-devel,v2] Rework NCP compability logic and drop BF-CBC support by default
Related show

Commit Message

Arne Schwabe July 29, 2020, 11:38 a.m.
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 <arne@rfc2549.org>

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 <arne@rfc2549.org>
---
 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(-)

Comments

Gert Doering Aug. 4, 2020, 8:20 p.m. | #1
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, emailAddress=samuli@openvpn.net
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, emailAddress=gert@greenie.muc.de
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
Arne Schwabe Aug. 4, 2020, 10:20 p.m. | #2
> 
> 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
Gert Doering Aug. 5, 2020, 6:43 a.m. | #3
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
Gert Doering Aug. 5, 2020, 11:45 a.m. | #4
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
Steffan Karger Aug. 5, 2020, 8:25 p.m. | #5
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 <arne@rfc2549.org>
> 
> 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 <arne@rfc2549.org>
> ---
>  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
tincanteksup Aug. 5, 2020, 8:59 p.m. | #6
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 <arne@rfc2549.org>
>>
>> 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 <arne@rfc2549.org>
>> ---
>>   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
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel
>
Arne Schwabe Aug. 8, 2020, 6:11 p.m. | #7
> 
> 
> 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...


I am not sure we can fix this in a good way. The behaviour is bascially
blindly push a cipher no matter what the client announces.

What we would need to still support this behaviour is a


--data-cipher-force-cipher-if-only-iv_ncp2-present cipher

that picks that cipher if the client has only IV_NCP=2. That sounds like
a very ugly and obscure option. Or an option that is

--iv-ncp-2-is-data-ciphers "foo:AES-128-CBC:MySpecial-Cipher"

and then the server would translate IV_NCP=2 to that list instead of
"AES-256-GCM:AES-128-GCM"

All other options that I can come up break proper negotiation support.
The option names are of course silly and would need to be replaced by
better sounding alternatives. *If* we want to support this corner case I
would suggest the second alternative and implement it in a follow up
patch. The question is if this corner case is important enough to
support it.

Arne
Gert Doering Aug. 9, 2020, 7:46 a.m. | #8
Hi,

On Sat, Aug 08, 2020 at 08:11:13PM +0200, Arne Schwabe wrote:
> > 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...
> 
> I am not sure we can fix this in a good way. The behaviour is bascially
> blindly push a cipher no matter what the client announces.

Yes.  And set the server instance to use this cipher.

So something like "--data-cipher-override-push"...

The use of that option would, basically, be "I have lots of 2.4 clients,
I can not update their client configs all over night, but for whatever
reason I do not want to use AES-GCM ciphers (and not BF-CBC either)".  I
can not foresee a strong reason to not use AES-GCM, but maybe some sort
of embedded platform that performs great on ChaCha-Poly and very poorly
on AES...  or, worst case, AES is broken.

Not sure how realistic that is.


> What we would need to still support this behaviour is a
> 
> --data-cipher-force-cipher-if-only-iv_ncp2-present cipher
> 
> that picks that cipher if the client has only IV_NCP=2. That sounds like
> a very ugly and obscure option. Or an option that is
> 
> --iv-ncp-2-is-data-ciphers "foo:AES-128-CBC:MySpecial-Cipher"
> 
> and then the server would translate IV_NCP=2 to that list instead of
> "AES-256-GCM:AES-128-GCM"
> 
> All other options that I can come up break proper negotiation support.


Either this ("we pretend the client has really sent *this* list of 
ciphers") or we override negotiation by forcing a particular cipher
for this client (use on the server and push to client - which a 2.3
and 2.2 client will ignore with a warning).   

Overriding could come with a warning in the server log ("forcing a 
cipher that is not in the list advertised by the client, might break").


> The option names are of course silly and would need to be replaced by
> better sounding alternatives. *If* we want to support this corner case I
> would suggest the second alternative and implement it in a follow up
> patch. The question is if this corner case is important enough to
> support it.

Doing this in a followup patch sounds reasonable...  this could even
go into 2.5 later on (as it would fix a - small - regression).

So - let's get this in :-) - v3, please, which does not core dump.


Also, I think we need to better document what happens wrt cipher
negotiation depending on client/server combinations (2.2+2.3, 2.4,
2.5 to a 2.3 / 2.4 / 2.5 server, OCC/NCP/new NCP, ...).

Maybe a table in the Wiki?   Or something in doc/

We have your commit message, which is a good start, but if people get
all confused about this (like I was :-) ), they will not look into the
git logs.

gert

Patch

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);