From patchwork Thu Jul 9 10:15:56 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: [Openvpn-devel,1/8] Deprecate ncp-disable and add improved ncp to Changes.rst X-Patchwork-Submitter: Arne Schwabe X-Patchwork-Id: 1222 Message-Id: <20200709101603.11941-1-arne@rfc2549.org> To: openvpn-devel@lists.sourceforge.net Date: Thu, 9 Jul 2020 12:15:56 +0200 From: Arne Schwabe List-Id: Signed-off-by: Arne Schwabe Acked-by: Gert Doering --- Changes.rst | 18 ++++++++++++++++++ src/openvpn/options.c | 5 ++++- 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/Changes.rst b/Changes.rst index 00dd6ed8..2752d29b 100644 --- a/Changes.rst +++ b/Changes.rst @@ -13,6 +13,24 @@ ChaCha20-Poly1305 cipher support Added support for using the ChaCha20-Poly1305 cipher in the OpenVPN data channel. +Improved Data channel cipher negotiation + OpenVPN clients will now signal all supported cipher from the + ``ncp-ciphers`` option to the server via ``IV_CIPHERS``. OpenVPN + servers will select the first common cipher from the ``ncp-ciphers`` + list instead of blindly pushing the first cipher of the list. This + allows to use a configuration like + ``ncp-ciphers ChaCha20-Poly1305:AES-256-GCM`` on the server that + prefers ChaCha20-Poly1305 but uses it only if the client supports it. + +Deprecated features +------------------- +For an up-to-date list of all deprecated options, see this wiki page: +https://community.openvpn.net/openvpn/wiki/DeprecatedOptions + +- ``ncp-disable`` has been deprecated + With the improved and matured data channel cipher negioation, the use + of ``ncp-disable`` should not be necessary anymore. + Overview of changes in 2.4 ========================== diff --git a/src/openvpn/options.c b/src/openvpn/options.c index a72b677a..75871b46 100644 --- a/src/openvpn/options.c +++ b/src/openvpn/options.c @@ -545,7 +545,7 @@ static const char usage_message[] = " (default=%s).\n" " Set alg=none to disable encryption.\n" "--ncp-ciphers list : List of ciphers that are allowed to be negotiated.\n" - "--ncp-disable : Disable cipher negotiation.\n" + "--ncp-disable : (DEPRECATED) Disable cipher negotiation.\n" "--prng alg [nsl] : For PRNG, use digest algorithm alg, and\n" " nonce_secret_len=nsl. Set alg=none to disable PRNG.\n" #ifdef HAVE_EVP_CIPHER_CTX_SET_KEY_LENGTH @@ -7904,6 +7904,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 negioating is a depracted debug feature that will " + "be removed in OpenVPN 2.6"); } else if (streq(p[0], "prng") && p[1] && !p[3]) { From patchwork Thu Jul 9 10:15:57 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: [Openvpn-devel,2/8] Make key_state->authenticated more state machine like X-Patchwork-Submitter: Arne Schwabe X-Patchwork-Id: 1223 Message-Id: <20200709101603.11941-2-arne@rfc2549.org> To: openvpn-devel@lists.sourceforge.net Date: Thu, 9 Jul 2020 12:15:57 +0200 From: Arne Schwabe List-Id: This order the states from unauthenticated to authenticated and also changes the comparison for KS_AUTH_FALSE from != to > It also add comments and documents part using the state machine better. Remove a now obsolete comment and two obsolete ifdefs. While keeping the ifdef in ssl_verify would save a few bytes of code, this is too minor to justify keeping the ifdef Signed-off-by: Arne Schwabe Acked-by: Antonio Quartulli --- src/openvpn/multi.c | 12 +++++++++--- src/openvpn/ssl.c | 7 ++++--- src/openvpn/ssl_common.h | 22 ++++++++++++++++------ src/openvpn/ssl_verify.c | 18 +++++++----------- 4 files changed, 36 insertions(+), 23 deletions(-) diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c index f1ced9b7..f1332c8d 100644 --- a/src/openvpn/multi.c +++ b/src/openvpn/multi.c @@ -2352,12 +2352,12 @@ multi_process_post(struct multi_context *m, struct multi_instance *mi, const uns if (!IS_SIG(&mi->context) && ((flags & MPP_PRE_SELECT) || ((flags & MPP_CONDITIONAL_PRE_SELECT) && !ANY_OUT(&mi->context)))) { #if defined(ENABLE_ASYNC_PUSH) && defined(ENABLE_DEF_AUTH) - bool was_authenticated = false; + bool was_unauthenticated = true; struct key_state *ks = NULL; if (mi->context.c2.tls_multi) { ks = &mi->context.c2.tls_multi->session[TM_ACTIVE].key[KS_PRIMARY]; - was_authenticated = ks->authenticated; + was_unauthenticated = (ks->authenticated == KS_AUTH_FALSE); } #endif @@ -2366,7 +2366,13 @@ multi_process_post(struct multi_context *m, struct multi_instance *mi, const uns pre_select(&mi->context); #if defined(ENABLE_ASYNC_PUSH) && defined(ENABLE_DEF_AUTH) - if (ks && ks->auth_control_file && ks->auth_deferred && !was_authenticated) + /* + * if we see the state transition from unauthenticated to deferred + * and a auth_control_file, we assume it got just added and add + * inotify watch to that file + */ + if (ks && ks->auth_control_file && was_unauthenticated + && (ks->authenticated == KS_AUTH_DEFERRED)) { /* watch acf file */ long watch_descriptor = inotify_add_watch(m->top.c2.inotify_fd, ks->auth_control_file, IN_CLOSE_WRITE | IN_ONESHOT); diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c index 9df7552d..f3fe0ecf 100644 --- a/src/openvpn/ssl.c +++ b/src/openvpn/ssl.c @@ -2466,7 +2466,7 @@ key_method_2_write(struct buffer *buf, struct tls_session *session) if (session->opt->server && !(session->opt->ncp_enabled && session->opt->mode == MODE_SERVER && ks->key_id <= 0)) { - if (ks->authenticated != KS_AUTH_FALSE) + if (ks->authenticated > KS_AUTH_FALSE) { if (!tls_session_generate_data_channel_keys(session)) { @@ -2659,7 +2659,7 @@ key_method_2_read(struct buffer *buf, struct tls_multi *multi, struct tls_sessio secure_memzero(up, sizeof(*up)); /* Perform final authentication checks */ - if (ks->authenticated != KS_AUTH_FALSE) + if (ks->authenticated > KS_AUTH_FALSE) { verify_final_auth_checks(multi, session); } @@ -2684,7 +2684,7 @@ key_method_2_read(struct buffer *buf, struct tls_multi *multi, struct tls_sessio * Call OPENVPN_PLUGIN_TLS_FINAL plugin if defined, for final * veto opportunity over authentication decision. */ - if ((ks->authenticated != KS_AUTH_FALSE) + if ((ks->authenticated > KS_AUTH_FALSE) && plugin_defined(session->opt->plugins, OPENVPN_PLUGIN_TLS_FINAL)) { key_state_export_keying_material(&ks->ks_ssl, session); @@ -2715,6 +2715,7 @@ key_method_2_read(struct buffer *buf, struct tls_multi *multi, struct tls_sessio return true; error: + ks->authenticated = KS_AUTH_FALSE; secure_memzero(ks->key_src, sizeof(*ks->key_src)); if (up) { diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h index fdf589b5..dd82b77d 100644 --- a/src/openvpn/ssl_common.h +++ b/src/openvpn/ssl_common.h @@ -127,10 +127,23 @@ struct key_source2 { struct key_source server; /**< Random provided by server. */ }; + +/** + * This refelects the (server side) state auf authentication after the TLS + * session has been established and key_method_2_read is called. If async auth + * is enabled the state will first move to KS_AUTH_DEFERRED before eventually + * being set to KS_AUTH_TRUE or KS_AUTH_FALSE + * Only KS_AUTH_TRUE is fully authenticated + */ enum ks_auth_state { - KS_AUTH_FALSE, - KS_AUTH_TRUE, - KS_AUTH_DEFERRED + KS_AUTH_FALSE, /**< Key state is not authenticated */ + KS_AUTH_DEFERRED, /**< Key state authentication is being deferred, + * by async auth */ + KS_AUTH_TRUE /**< Key state is authenticated. TLS and user/pass + * succeeded. This include AUTH_PENDING/OOB + * authentication as those hold the + * connection artifically in KS_AUTH_DEFERRED + */ }; /** @@ -194,8 +207,6 @@ struct key_state enum ks_auth_state authenticated; time_t auth_deferred_expire; -#ifdef ENABLE_DEF_AUTH - /* If auth_deferred is true, authentication is being deferred */ #ifdef MANAGEMENT_DEF_AUTH unsigned int mda_key_id; unsigned int mda_status; @@ -205,7 +216,6 @@ struct key_state time_t acf_last_mod; char *auth_control_file; #endif -#endif }; /** Control channel wrapping (--tls-auth/--tls-crypt) context */ diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c index e28f1f3a..990fba99 100644 --- a/src/openvpn/ssl_verify.c +++ b/src/openvpn/ssl_verify.c @@ -950,7 +950,7 @@ tls_authentication_status(struct tls_multi *multi, const int latency) if (DECRYPT_KEY_ENABLED(multi, ks)) { active = true; - if (ks->authenticated != KS_AUTH_FALSE) + if (ks->authenticated > KS_AUTH_FALSE) { #ifdef ENABLE_DEF_AUTH unsigned int s1 = ACF_DISABLED; @@ -1249,6 +1249,9 @@ verify_user_pass_management(struct tls_session *session, /* * Main username/password verification entry point + * + * Will set session->ks[KS_PRIMARY].authenticated according to + * result of the username/password verifcation */ void verify_user_pass(struct user_pass *up, struct tls_multi *multi, @@ -1414,17 +1417,10 @@ verify_user_pass(struct user_pass *up, struct tls_multi *multi, */ send_push_reply_auth_token(multi); } -#ifdef ENABLE_DEF_AUTH msg(D_HANDSHAKE, "TLS: Username/Password authentication %s for username '%s' %s", (ks->authenticated == KS_AUTH_DEFERRED) ? "deferred" : "succeeded", up->username, (session->opt->ssl_flags & SSLF_USERNAME_AS_COMMON_NAME) ? "[CN SET]" : ""); -#else - msg(D_HANDSHAKE, "TLS: Username/Password authentication %s for username '%s' %s", - "succeeded", - up->username, - (session->opt->ssl_flags & SSLF_USERNAME_AS_COMMON_NAME) ? "[CN SET]" : ""); -#endif } else { @@ -1445,7 +1441,7 @@ verify_final_auth_checks(struct tls_multi *multi, struct tls_session *session) } /* Don't allow the CN to change once it's been locked */ - if (ks->authenticated != KS_AUTH_FALSE && multi->locked_cn) + if (ks->authenticated > KS_AUTH_FALSE && multi->locked_cn) { const char *cn = session->common_name; if (cn && strcmp(cn, multi->locked_cn)) @@ -1461,7 +1457,7 @@ verify_final_auth_checks(struct tls_multi *multi, struct tls_session *session) } /* Don't allow the cert hashes to change once they have been locked */ - if (ks->authenticated != KS_AUTH_FALSE && multi->locked_cert_hash_set) + if (ks->authenticated > KS_AUTH_FALSE && multi->locked_cert_hash_set) { const struct cert_hash_set *chs = session->cert_hash_set; if (chs && !cert_hash_compare(chs, multi->locked_cert_hash_set)) @@ -1475,7 +1471,7 @@ verify_final_auth_checks(struct tls_multi *multi, struct tls_session *session) } /* verify --client-config-dir based authentication */ - if (ks->authenticated != KS_AUTH_FALSE && session->opt->client_config_dir_exclusive) + if (ks->authenticated > KS_AUTH_FALSE && session->opt->client_config_dir_exclusive) { struct gc_arena gc = gc_new(); From patchwork Thu Jul 9 10:15:58 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: [Openvpn-devel,3/8] Extract process_incoming_push_reply from process_incoming_push_msg X-Patchwork-Submitter: Arne Schwabe X-Patchwork-Id: 1219 Message-Id: <20200709101603.11941-3-arne@rfc2549.org> To: openvpn-devel@lists.sourceforge.net Date: Thu, 9 Jul 2020 12:15:58 +0200 From: Arne Schwabe List-Id: This is a small refactoring to make both function more readable. It also eliminates the ret variable in process_incoming_push_msg that now serves no purpose anymore. Signed-off-by: Arne Schwabe Acked-by: Antonio Quartulli --- src/openvpn/push.c | 113 +++++++++++++++++++++++++-------------------- 1 file changed, 64 insertions(+), 49 deletions(-) diff --git a/src/openvpn/push.c b/src/openvpn/push.c index 56d652a3..d74323db 100644 --- a/src/openvpn/push.c +++ b/src/openvpn/push.c @@ -855,6 +855,63 @@ push_update_digest(md_ctx_t *ctx, struct buffer *buf, const struct options *opt) } } +static int +process_incoming_push_reply(struct context *c, + unsigned int permission_mask, + const unsigned int *option_types_found, + struct buffer *buf) +{ + int ret = PUSH_MSG_ERROR; + const uint8_t ch = buf_read_u8(buf); + if (ch == ',') + { + struct buffer buf_orig = (*buf); + if (!c->c2.pulled_options_digest_init_done) + { + c->c2.pulled_options_state = md_ctx_new(); + md_ctx_init(c->c2.pulled_options_state, md_kt_get("SHA256")); + c->c2.pulled_options_digest_init_done = true; + } + if (!c->c2.did_pre_pull_restore) + { + pre_pull_restore(&c->options, &c->c2.gc); + c->c2.did_pre_pull_restore = true; + } + if (apply_push_options(&c->options, + buf, + permission_mask, + option_types_found, + c->c2.es)) + { + push_update_digest(c->c2.pulled_options_state, &buf_orig, + &c->options); + switch (c->options.push_continuation) + { + case 0: + case 1: + md_ctx_final(c->c2.pulled_options_state, + c->c2.pulled_options_digest.digest); + md_ctx_cleanup(c->c2.pulled_options_state); + md_ctx_free(c->c2.pulled_options_state); + c->c2.pulled_options_state = NULL; + c->c2.pulled_options_digest_init_done = false; + ret = PUSH_MSG_REPLY; + break; + + case 2: + ret = PUSH_MSG_CONTINUATION; + break; + } + } + } + else if (ch == '\0') + { + ret = PUSH_MSG_REPLY; + } + /* show_settings (&c->options); */ + return ret; +} + int process_incoming_push_msg(struct context *c, const struct buffer *buffer, @@ -862,64 +919,22 @@ process_incoming_push_msg(struct context *c, unsigned int permission_mask, unsigned int *option_types_found) { - int ret = PUSH_MSG_ERROR; struct buffer buf = *buffer; if (buf_string_compare_advance(&buf, "PUSH_REQUEST")) { - ret = process_incoming_push_request(c); + return process_incoming_push_request(c); } else if (honor_received_options && buf_string_compare_advance(&buf, "PUSH_REPLY")) { - const uint8_t ch = buf_read_u8(&buf); - if (ch == ',') - { - struct buffer buf_orig = buf; - if (!c->c2.pulled_options_digest_init_done) - { - c->c2.pulled_options_state = md_ctx_new(); - md_ctx_init(c->c2.pulled_options_state, md_kt_get("SHA256")); - c->c2.pulled_options_digest_init_done = true; - } - if (!c->c2.did_pre_pull_restore) - { - pre_pull_restore(&c->options, &c->c2.gc); - c->c2.did_pre_pull_restore = true; - } - if (apply_push_options(&c->options, - &buf, - permission_mask, - option_types_found, - c->c2.es)) - { - push_update_digest(c->c2.pulled_options_state, &buf_orig, - &c->options); - switch (c->options.push_continuation) - { - case 0: - case 1: - md_ctx_final(c->c2.pulled_options_state, c->c2.pulled_options_digest.digest); - md_ctx_cleanup(c->c2.pulled_options_state); - md_ctx_free(c->c2.pulled_options_state); - c->c2.pulled_options_state = NULL; - c->c2.pulled_options_digest_init_done = false; - ret = PUSH_MSG_REPLY; - break; - - case 2: - ret = PUSH_MSG_CONTINUATION; - break; - } - } - } - else if (ch == '\0') - { - ret = PUSH_MSG_REPLY; - } - /* show_settings (&c->options); */ + return process_incoming_push_reply(c, permission_mask, + option_types_found, &buf); + } + else + { + return PUSH_MSG_ERROR; } - return ret; } From patchwork Thu Jul 9 10:15:59 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: [Openvpn-devel,4/8] Move protocol option negotiation from push_prepare to new function X-Patchwork-Submitter: Arne Schwabe X-Patchwork-Id: 1220 Message-Id: <20200709101603.11941-4-arne@rfc2549.org> To: openvpn-devel@lists.sourceforge.net Date: Thu, 9 Jul 2020 12:15:59 +0200 From: Arne Schwabe List-Id: This clean ups the code and removes the surprising side effects of preparing a push reply to also select protocol options. We also remember if we have seen a push request without async push. This improves reaction time if deferred auth is involved like managment interface deferred auth. The other benefit is removing a number of ifdefs. Signed-off-by: Arne Schwabe Acked-by: Gert Doering --- src/openvpn/forward.c | 4 +-- src/openvpn/multi.c | 81 ++++++++++++++++++++++++++++++++++++++++--- src/openvpn/openvpn.h | 2 -- src/openvpn/push.c | 66 +++++------------------------------ 4 files changed, 88 insertions(+), 65 deletions(-) diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c index 885cf126..5c4370a8 100644 --- a/src/openvpn/forward.c +++ b/src/openvpn/forward.c @@ -1123,8 +1123,8 @@ process_incoming_link_part1(struct context *c, struct link_socket_info *lsi, boo } /* - * Drop non-TLS packet if client-connect script/plugin has not - * yet succeeded. + * Drop non-TLS packet if client-connect script/plugin and cipher selection + * has not yet succeeded. */ if (c->c2.context_auth != CAS_SUCCEEDED) { diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c index f1332c8d..f04c4c90 100644 --- a/src/openvpn/multi.c +++ b/src/openvpn/multi.c @@ -824,8 +824,8 @@ multi_create_instance(struct multi_context *m, const struct mroute_addr *real) mi->did_cid_hash = true; #endif -#ifdef ENABLE_ASYNC_PUSH mi->context.c2.push_request_received = false; +#ifdef ENABLE_ASYNC_PUSH mi->inotify_watch = -1; #endif @@ -1772,6 +1772,78 @@ multi_client_connect_setenv(struct multi_context *m, gc_free(&gc); } +/** + * Calculates the options that depend on the client capabilities + * based on local options and available peer info + * - choosen cipher + * - peer id + */ +static void +multi_client_set_protocol_options(struct context *c) +{ + + const char *optstr = NULL; + struct tls_multi *tls_multi = c->c2.tls_multi; + const char *const peer_info = tls_multi->peer_info; + struct options *o = &c->options; + + /* Send peer-id if client supports it */ + optstr = peer_info ? strstr(peer_info, "IV_PROTO=") : NULL; + if (optstr) + { + int proto = 0; + int r = sscanf(optstr, "IV_PROTO=%d", &proto); + if ((r == 1) && (proto >= 2)) + { + tls_multi->use_peer_id = true; + } + } + + /* Select cipher if client supports Negotiable Crypto Parameters */ + 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) + { + 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 ); + } + else + { + /* + * Push the first cipher from --ncp-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); + msg(M_INFO, "PUSH: No common cipher between server and client." + "Expect this connection not to work. " + "Server ncp-ciphers: '%s', client supported ciphers '%s'", + o->ncp_ciphers, peer_ciphers); + gc_free(&gc); + } + } + } +} + + /* * Called as soon as the SSL/TLS connection authenticates. * @@ -2074,13 +2146,14 @@ script_failed: /* set context-level authentication flag */ mi->context.c2.context_auth = CAS_SUCCEEDED; -#ifdef ENABLE_ASYNC_PUSH - /* authentication complete, send push reply */ + /* authentication complete, calculate dynamic client specific options */ + multi_client_set_protocol_options(&mi->context); + + /* send push reply if ready*/ if (mi->context.c2.push_request_received) { process_incoming_push_request(&mi->context); } -#endif } else { diff --git a/src/openvpn/openvpn.h b/src/openvpn/openvpn.h index 4609af3e..a1308852 100644 --- a/src/openvpn/openvpn.h +++ b/src/openvpn/openvpn.h @@ -432,9 +432,7 @@ struct context_2 #if P2MP /* --ifconfig endpoints to be pushed to client */ -#ifdef ENABLE_ASYNC_PUSH bool push_request_received; -#endif bool push_ifconfig_defined; time_t sent_push_reply_expiry; in_addr_t push_ifconfig_local; diff --git a/src/openvpn/push.c b/src/openvpn/push.c index d74323db..92a28a14 100644 --- a/src/openvpn/push.c +++ b/src/openvpn/push.c @@ -437,7 +437,7 @@ prepare_auth_token_push_reply(struct tls_multi *tls_multi, struct gc_arena *gc, } /** - * Prepare push options, based on local options and available peer info. + * Prepare push options, based on local options * * @param context context structure storing data for VPN tunnel * @param gc gc arena for allocating push options @@ -449,9 +449,7 @@ bool prepare_push_reply(struct context *c, struct gc_arena *gc, struct push_list *push_list) { - const char *optstr = NULL; struct tls_multi *tls_multi = c->c2.tls_multi; - const char *const peer_info = tls_multi->peer_info; struct options *o = &c->options; /* ipv6 */ @@ -480,18 +478,10 @@ prepare_push_reply(struct context *c, struct gc_arena *gc, 0, gc)); } - /* Send peer-id if client supports it */ - optstr = peer_info ? strstr(peer_info, "IV_PROTO=") : NULL; - if (optstr) + if (tls_multi->use_peer_id) { - int proto = 0; - int r = sscanf(optstr, "IV_PROTO=%d", &proto); - if ((r == 1) && (proto >= 2)) - { - push_option_fmt(gc, push_list, M_USAGE, "peer-id %d", - tls_multi->peer_id); - tls_multi->use_peer_id = true; - } + push_option_fmt(gc, push_list, M_USAGE, "peer-id %d", + tls_multi->peer_id); } /* * If server uses --auth-gen-token and we have an auth token @@ -499,47 +489,11 @@ prepare_push_reply(struct context *c, struct gc_arena *gc, */ prepare_auth_token_push_reply(tls_multi, gc, push_list); - /* Push cipher if client supports Negotiable Crypto Parameters */ - 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) - { - 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 ); - } - else - { - /* - * Push the first cipher from --ncp-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 - { - const char *peer_ciphers = tls_peer_ncp_list(peer_info, gc); - msg(M_INFO, "PUSH: No common cipher between server and client." - "Expect this connection not to work. " - "Server ncp-ciphers: '%s', client supported ciphers '%s'", - o->ncp_ciphers, peer_ciphers); - } - } - push_option_fmt(gc, push_list, M_USAGE, "cipher %s", o->ciphername); - } + /* + * Push the selected cipher, at this point the cipher has been + * already negioated and been fixed + */ + push_option_fmt(gc, push_list, M_USAGE, "cipher %s", o->ciphername); return true; } @@ -794,9 +748,7 @@ process_incoming_push_request(struct context *c) { int ret = PUSH_MSG_ERROR; -#ifdef ENABLE_ASYNC_PUSH c->c2.push_request_received = true; -#endif if (tls_authentication_status(c->c2.tls_multi, 0) == TLS_AUTHENTICATION_FAILED || c->c2.context_auth == CAS_FAILED) { const char *client_reason = tls_client_reason(c->c2.tls_multi); From patchwork Thu Jul 9 10:16:00 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: [Openvpn-devel,5/8] Generate data channel keys after connect options have been parsed X-Patchwork-Submitter: Arne Schwabe X-Patchwork-Id: 1216 Message-Id: <20200709101603.11941-5-arne@rfc2549.org> To: openvpn-devel@lists.sourceforge.net Date: Thu, 9 Jul 2020 12:16:00 +0200 From: Arne Schwabe List-Id: The simplify the control flow, it makes more sense to generate the data keys when all the prerequisites for generating the data channel keys (ncp cipher selection etc) are met instead of delaying it to the next incoming PUSH_REQUEST message. This also eliminates the need for the hack introduced by commit 3b06b57d9 to generate the data channel keys on the async file close event. Signed-off-by: Arne Schwabe Acked-by: Gert Doering --- src/openvpn/multi.c | 54 ++++++++++++++++++++++++++------------------- src/openvpn/push.c | 27 ++++------------------- 2 files changed, 35 insertions(+), 46 deletions(-) diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c index f04c4c90..810e489a 100644 --- a/src/openvpn/multi.c +++ b/src/openvpn/multi.c @@ -1843,6 +1843,30 @@ multi_client_set_protocol_options(struct context *c) } } +/** + * Generates the data channel keys + */ +static bool +multi_client_generate_tls_keys(struct context *c) +{ + struct frame *frame_fragment = NULL; +#ifdef ENABLE_FRAGMENT + if (c->options.ce.fragment) + { + frame_fragment = &c->c2.frame_fragment; + } +#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)) + { + msg(D_TLS_ERRORS, "TLS Error: initializing data channel failed"); + register_signal(c, SIGUSR1, "process-push-msg-failed"); + return false; + } + + return true; +} /* * Called as soon as the SSL/TLS connection authenticates. @@ -2149,7 +2173,13 @@ script_failed: /* authentication complete, calculate dynamic client specific options */ multi_client_set_protocol_options(&mi->context); - /* send push reply if ready*/ + /* Generate data channel keys */ + if (!multi_client_generate_tls_keys(&mi->context)) + { + mi->context.c2.context_auth = CAS_FAILED; + } + + /* send push reply if ready */ if (mi->context.c2.push_request_received) { process_incoming_push_request(&mi->context); @@ -2205,28 +2235,6 @@ multi_process_file_closed(struct multi_context *m, const unsigned int mpp_flags) { /* continue authentication, perform NCP negotiation and send push_reply */ multi_process_post(m, mi, mpp_flags); - - /* With NCP and deferred authentication, we perform cipher negotiation and - * data channel keys generation on incoming push request, assuming that auth - * succeeded. When auth succeeds in between push requests and async push is used, - * we send push reply immediately. Above multi_process_post() call performs - * NCP negotiation and here we do keys generation. */ - - struct context *c = &mi->context; - struct frame *frame_fragment = NULL; -#ifdef ENABLE_FRAGMENT - if (c->options.ce.fragment) - { - frame_fragment = &c->c2.frame_fragment; - } -#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)) - { - msg(D_TLS_ERRORS, "TLS Error: initializing data channel failed"); - register_signal(c, SIGUSR1, "init-data-channel-failed"); - } } else { diff --git a/src/openvpn/push.c b/src/openvpn/push.c index 92a28a14..5bc4328c 100644 --- a/src/openvpn/push.c +++ b/src/openvpn/push.c @@ -359,30 +359,11 @@ incoming_push_message(struct context *c, const struct buffer *buffer) } event_timeout_clear(&c->c2.push_request_interval); } - else if (status == PUSH_MSG_REQUEST) - { - if (c->options.mode == MODE_SERVER) - { - struct frame *frame_fragment = NULL; -#ifdef ENABLE_FRAGMENT - if (c->options.ce.fragment) - { - frame_fragment = &c->c2.frame_fragment; - } -#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)) - { - msg(D_TLS_ERRORS, "TLS Error: initializing data channel failed"); - goto error; - } - } - } goto cleanup; + error: - register_signal(c, SIGUSR1, "process-push-msg-failed"); + register_signal(c, SIGUSR1, "process-push-msg-failed"); cleanup: gc_free(&gc); } @@ -748,7 +729,6 @@ process_incoming_push_request(struct context *c) { int ret = PUSH_MSG_ERROR; - c->c2.push_request_received = true; if (tls_authentication_status(c->c2.tls_multi, 0) == TLS_AUTHENTICATION_FAILED || c->c2.context_auth == CAS_FAILED) { const char *client_reason = tls_client_reason(c->c2.tls_multi); @@ -810,7 +790,7 @@ push_update_digest(md_ctx_t *ctx, struct buffer *buf, const struct options *opt) static int process_incoming_push_reply(struct context *c, unsigned int permission_mask, - const unsigned int *option_types_found, + unsigned int *option_types_found, struct buffer *buf) { int ret = PUSH_MSG_ERROR; @@ -875,6 +855,7 @@ process_incoming_push_msg(struct context *c, if (buf_string_compare_advance(&buf, "PUSH_REQUEST")) { + c->c2.push_request_received = true; return process_incoming_push_request(c); } else if (honor_received_options From patchwork Thu Jul 9 10:16:01 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: [Openvpn-devel,6/8] Cleanup: Remove special case code for old poor man's NCP. X-Patchwork-Submitter: Arne Schwabe X-Patchwork-Id: 1221 Message-Id: <20200709101603.11941-6-arne@rfc2549.org> To: openvpn-devel@lists.sourceforge.net Date: Thu, 9 Jul 2020 12:16:01 +0200 From: Arne Schwabe List-Id: Ever since the NCPv2 the ncp_get_best_cipher uses the global options->ncp_enabled option and ignore the tls_session->ncp_enabled option. The server side's poor man's NCP is implemented as seeing the list of supported ciphers from the peer as just one cipher so this special handling for poor man's NCP of the older NCP here is not needed anymore. Theoretically we can now get rid of tls_session->ncp_enabled but doing so requires more refactoring since options is not available in the methods that still use it. And when we remove ncp-disable the variable will be removed anyway. This commit moves the data channel key generation for the corner case of a client not supporting NCP but having the same cipher as the server to the same function that also generates data channel keys for NCP and poort man's NCP. This has an unintended side effect of changing the calculated frame size for this special case. The old path did call tls_session_update_crypto_params. To avoid this change in behaviour, this patch adds a hacky workaround for this. A proper solution for this needs still be found but this allows the patch set to be merged. Document the remaining usage of tls_poor_mans_ncp better. Signed-off-by: Arne Schwabe Acked-by: Gert Doering --- src/openvpn/init.c | 2 ++ src/openvpn/ssl.c | 21 +++++++-------------- 2 files changed, 9 insertions(+), 14 deletions(-) diff --git a/src/openvpn/init.c b/src/openvpn/init.c index 91b919d5..e9c01629 100644 --- a/src/openvpn/init.c +++ b/src/openvpn/init.c @@ -2376,6 +2376,8 @@ do_deferred_options(struct context *c, const unsigned int found) } else if (c->options.ncp_enabled) { + /* 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); } struct frame *frame_fragment = NULL; diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c index f3fe0ecf..668bcbd9 100644 --- a/src/openvpn/ssl.c +++ b/src/openvpn/ssl.c @@ -1986,6 +1986,12 @@ tls_session_update_crypto_params(struct tls_session *session, options->keysize = 0; } } + else + { + /* Very hacky workaround and quick fix for our calculation + * not correct to avoid a regression */ + return tls_session_generate_data_channel_keys(session); + } init_key_type(&session->opt->key_type, options->ciphername, options->authname, options->keysize, true, true); @@ -2463,8 +2469,7 @@ key_method_2_write(struct buffer *buf, struct tls_session *session) * generation is postponed until after the pull/push, so we can process pushed * cipher directives. */ - if (session->opt->server && !(session->opt->ncp_enabled - && session->opt->mode == MODE_SERVER && ks->key_id <= 0)) + if (session->opt->server && !(session->opt->mode == MODE_SERVER && ks->key_id <= 0)) { if (ks->authenticated > KS_AUTH_FALSE) { @@ -2616,18 +2621,6 @@ key_method_2_read(struct buffer *buf, struct tls_multi *multi, struct tls_sessio multi->remote_ciphername = options_string_extract_option(options, "cipher", NULL); - if (!tls_peer_supports_ncp(multi->peer_info)) - { - /* Peer does not support NCP, but leave NCP enabled if the local and - * remote cipher do not match to attempt 'poor-man's NCP'. - */ - if (multi->remote_ciphername == NULL - || 0 == strcmp(multi->remote_ciphername, multi->opt.config_ciphername)) - { - session->opt->ncp_enabled = false; - } - } - if (tls_session_user_pass_enabled(session)) { /* Perform username/password authentication */ From patchwork Thu Jul 9 10:16:02 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: [Openvpn-devel,7/8] Removed unused definition X-Patchwork-Submitter: Arne Schwabe X-Patchwork-Id: 1217 Message-Id: <20200709101603.11941-7-arne@rfc2549.org> To: openvpn-devel@lists.sourceforge.net Date: Thu, 9 Jul 2020 12:16:02 +0200 From: Arne Schwabe List-Id: Signed-off-by: Arne Schwabe Acked-By: Gert Doering --- src/openvpn/ssl.c | 5 +++-- src/openvpn/ssl.h | 7 ------- 2 files changed, 3 insertions(+), 9 deletions(-) diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c index 668bcbd9..4ee4c245 100644 --- a/src/openvpn/ssl.c +++ b/src/openvpn/ssl.c @@ -1988,8 +1988,9 @@ tls_session_update_crypto_params(struct tls_session *session, } else { - /* Very hacky workaround and quick fix for our calculation - * not correct to avoid a regression */ + /* 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.h b/src/openvpn/ssl.h index 2f6f7657..58a9b0d4 100644 --- a/src/openvpn/ssl.h +++ b/src/openvpn/ssl.h @@ -89,13 +89,6 @@ #define TLS_MULTI_HORIZON 2 /* call tls_multi_process frequently for n seconds after * every packet sent/received action */ -/* - * The SSL/TLS worker thread will wait at most this many seconds for the - * interprocess communication pipe to the main thread to be ready to accept - * writes. - */ -#define TLS_MULTI_THREAD_SEND_TIMEOUT 5 - /* Interval that tls_multi_process should call tls_authentication_status */ #define TLS_MULTI_AUTH_STATUS_INTERVAL 10 From patchwork Thu Jul 9 10:16:03 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: [Openvpn-devel,8/8] Code cleanup: remove superflous variable X-Patchwork-Submitter: Arne Schwabe X-Patchwork-Id: 1218 Message-Id: <20200709101603.11941-8-arne@rfc2549.org> To: openvpn-devel@lists.sourceforge.net Date: Thu, 9 Jul 2020 12:16:03 +0200 From: Arne Schwabe List-Id: Signed-off-by: Arne Schwabe Acked-by: Antonio Quartulli --- src/openvpn/ssl.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c index 4ee4c245..54a23011 100644 --- a/src/openvpn/ssl.c +++ b/src/openvpn/ssl.c @@ -1231,11 +1231,10 @@ lame_duck_must_die(const struct tls_session *session, interval_t *wakeup) const struct key_state *lame = &session->key[KS_LAME_DUCK]; if (lame->state >= S_INITIAL) { - const time_t local_now = now; ASSERT(lame->must_die); /* a lame duck key must always have an expiration */ - if (local_now < lame->must_die) + if (now < lame->must_die) { - compute_earliest_wakeup(wakeup, lame->must_die - local_now); + compute_earliest_wakeup(wakeup, lame->must_die - now); return false; } else