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