| Message ID | 20200809141922.7853-1-arne@rfc2549.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [Openvpn-devel,v3,1/2] Rework NCP compability logic and drop BF-CBC support by default | expand |
spelling/grammar Couple of typos and some suggested grammar improvements. On 09/08/2020 15:19, 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. It is also used to > > 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 we reject (remove by) > 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 - When no cipher or only a cipher we refuse to support is pushed and we also cannot support the server's cipher announced in OCC then 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: 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. In 2.5 both client and server will automatically set fallback-cipher xyz. If --cipher xyz is in the config then also 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. We log and warn the user > > If --cipher is not set, we only warn that previous versions > allowed BF-CBC and point how to reenable BF-CBC. This might re-enable > break very few config where someone connects a very old break a small number of configs (reword) > 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 and those affected will (remove use) already have the (remove a) > scary SWEET32 warning in their logs. > > In short: If --cipher is explicitly set 2.6 will work the same as set then 2.6 (insert then) > 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. negotiate > > 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: rejected > > 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 > > Patch V3: fix minor style, add null check when client sends no peerinfo at > all > > 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 | 83 +++++++++++++--- > src/openvpn/ssl_ncp.h | 18 ++-- > tests/unit_tests/openvpn/test_ncp.c | 29 ++++-- > 10 files changed, 315 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 clients (remove /server .. I think this is what was meant?) > + 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..91cfbebf 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 1c246f4b..ee3fd763 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 old default of (remove o) > + * 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. */ about it being missing No further issues. > + 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; > } > @@ -7865,14 +7930,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]; > } > @@ -7880,9 +7951,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..f522b8f0 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,10 @@ 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 || > + (peer_info && strstr(peer_info, "IV_CIPHERS="))) > { > remote_cipher = ""; > } > @@ -242,15 +245,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 +256,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 > + * 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..a4334c89 100644 > --- a/tests/unit_tests/openvpn/test_ncp.c > +++ b/tests/unit_tests/openvpn/test_ncp.c > @@ -139,25 +139,36 @@ 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"); > > + best_cipher = ncp_get_best_cipher(serverlist, NULL,NULL, &gc); > + assert_ptr_equal(best_cipher, NULL); > + > gc_free(&gc); > } > > @@ -170,29 +181,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); > >
Am 09.08.20 um 18:14 schrieb tincanteksup: >> + If no common cipher is found during cipher negotiation, the >> connection >> + is terminated. To support old clients/server that do not provide >> any cipher > > clients > (remove /server .. I think this is what was meant?) No, old clients/servers. If you connect to an old server you might need that option too. See the follow up documentation patch. Arne
Acked-by: Gert Doering <gert@greenie.muc.de> As discussed on IRC, I have rewritten parts of the commit message to take the v3 changes and Richard's language comments into account. I have also removed the whitespace change hunks from multi.c that are not correct according to the Whitespace Governor. I can't claim that I understand every single aspect of the change set, but I have thoroughly tested this on the server testbed. It nicely exploded until I added "cipher BF-CBC" to all server.conf settings - as documented and logged. After changing this, all tests pass - 2.2 to master clients, with and without OCC, with and without --disable-ncp. CAVEAT: setting an per-instance cipher from ccd/ is ignored now, if the client is 2.4 (IV_NCP=2, but no IV_CIPHER) and is not already advertising the cipher in question (and in this case, it does need not do anything). After some discussion between Arne, Steffan and me, we have decided to document this, and bring back the functionality in a followup patch if a compelling use case is found why we actually need it. I have also tested the client side of this against a slightly older master server, and that worked as well. The --ncp-disable cases "with cipher not set in the config" break (as documented), and want "--cipher bf-cbc" added to the client config. CAVEAT: the particular funky case of "--client vs. a --tls-server server" (--inetd) breaks, because the server decides that it wants to "push BF-CBC", which the *client* then rejects based on "not allowed - BF-CBC not in AES-256-GCM:AES-128-GCM" - I think this might work if the server were not pushing anything, but it's a strange corner case anyway. So, just documenting this here (adding "--cipher BF-CBC" makes it work). Your patch has been applied to the master branch. commit 2c1d8c33d99d1d6d7902ea5845d7327aa6db9363 Author: Arne Schwabe Date: Sun Aug 9 16:19:21 2020 +0200 Rework NCP compability logic and drop BF-CBC support by default Signed-off-by: Arne Schwabe <arne@rfc2549.org> Signed-off-by: Arne Schwabe <arne@rfc2549.org> Acked-by: Gert Doering <gert@greenie.muc.de> Message-Id: <20200809141922.7853-1-arne@rfc2549.org> URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg20656.html Signed-off-by: Gert Doering <gert@greenie.muc.de> -- kind regards, Gert Doering
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..91cfbebf 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 1c246f4b..ee3fd763 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; } @@ -7865,14 +7930,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]; } @@ -7880,9 +7951,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..f522b8f0 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,10 @@ 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 || + (peer_info && strstr(peer_info, "IV_CIPHERS="))) { remote_cipher = ""; } @@ -242,15 +245,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 +256,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 + * 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..a4334c89 100644 --- a/tests/unit_tests/openvpn/test_ncp.c +++ b/tests/unit_tests/openvpn/test_ncp.c @@ -139,25 +139,36 @@ 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"); + best_cipher = ncp_get_best_cipher(serverlist, NULL,NULL, &gc); + assert_ptr_equal(best_cipher, NULL); + gc_free(&gc); } @@ -170,29 +181,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 Patch V3: fix minor style, add null check when client sends no peerinfo at all 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 | 83 +++++++++++++--- src/openvpn/ssl_ncp.h | 18 ++-- tests/unit_tests/openvpn/test_ncp.c | 29 ++++-- 10 files changed, 315 insertions(+), 130 deletions(-)