Fix problems with NCP and --inetd.
PATCH 1/1 Fix problems with NCP and --inetd.
NCP only works with --pull or --mode server, leading to breakage in --inetd mode (because that has --tls-server, but not --mode server, but clients can still ask for PUSH_REQUEST).
Fix by turning off o->ncp_enable unless (pull or mode server), and double-fix by logging an appropriate message and refusing to change ciphers if the server has already set up its keys.
Trac #715
Signed-off-by Gert Doering <[email protected]...>
--- src/openvpn/options.c | 8 ++++++++ src/openvpn/push.c | 21 ++++++++++++++++----- 2 files changed, 24 insertions(+), 5 deletions(-)
src/openvpn/options.c +8 -0
@@ -2614,6 +2614,14 @@ options_postprocess_mutate (struct options *o)if (streq (o->dh_file,"none" )) 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; + }#endif #if ENABLE_MANAGEMENT
src/openvpn/push.c +16 -5
@@ -321,11 +321,22 @@ prepare_push_reply (struct options *o, struct tls_multi *tls_multi)/* Push cipher if client supports Negotiable Crypto Parameters */ if (tls_peer_info_ncp_ver (peer_info) >=2 && o->ncp_enabled) { -/* Push the first cipher from --ncp-ciphers to the client. -* TODO: actual negotiation, instead of server dictatorship. */ -char *push_cipher =string_alloc (o->ncp_ciphers, &o->gc); - o->ciphername =strtok (push_cipher,":" ); -push_option_fmt (o,M_USAGE ,"cipher %s" , o->ciphername); +/* if we have already created our key, we cannot change our own +* cipher, so disable NCP and warn = explain why +*/ +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, ignoring client request" ); + } +else + { +/* Push the first cipher from --ncp-ciphers to the client. +* TODO: actual negotiation, instead of server dictatorship. */ +char *push_cipher =string_alloc (o->ncp_ciphers, &o->gc); + o->ciphername =strtok (push_cipher,":" ); +push_option_fmt (o,M_USAGE ,"cipher %s" , o->ciphername); + } }return true; }
Signature · 1 line
2.7.3
Quoted 68 lines · Gert Doering
NCP only works with --pull or --mode server, leading to breakage in --inetd mode (because that has --tls-server, but not --mode server, but clients can still ask for
PUSH_REQUEST).Fix by turning off
o->ncp_enableunless (pull or mode server), and double-fix by logging an appropriate message and refusing to change ciphers if the server has already set up its keys.Trac #715
Signed-off-by Gert Doering <[email protected]...>--- src/openvpn/options.c | 8 ++++++++ src/openvpn/push.c | 21 ++++++++++++++++----- 2 files changed, 24 insertions(+), 5 deletions(-)src/openvpn/options.c +8
-0@@ -2614,6 +2614,14 @@ options_postprocess_mutate (struct options *o)if (streq (o->dh_file,"none" )) 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; + }#endif #if ENABLE_MANAGEMENT src/openvpn/push.c +16
-6@@ -321,11 +321,22 @@ prepare_push_reply (struct options *o, struct tls_multi *tls_multi)/* Push cipher if client supports Negotiable Crypto Parameters */ if (tls_peer_info_ncp_ver (peer_info) >=2 && o->ncp_enabled) { -/* Push the first cipher from --ncp-ciphers to the client. -* TODO: actual negotiation, instead of server dictatorship. */ -char *push_cipher =string_alloc (o->ncp_ciphers, &o->gc); - o->ciphername =strtok (push_cipher,":" ); -push_option_fmt (o,M_USAGE ,"cipher %s" , o->ciphername); +/* if we have already created our key, we cannot change our own +* cipher, so disable NCP and warn = explain why +*/ +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, ignoring client request" ); + } +else + { +/* Push the first cipher from --ncp-ciphers to the client. +* TODO: actual negotiation, instead of server dictatorship. */ +char *push_cipher =string_alloc (o->ncp_ciphers, &o->gc); + o->ciphername =strtok (push_cipher,":" ); +push_option_fmt (o,M_USAGE ,"cipher %s" , o->ciphername); + } }return true; } --
Quoted 1 line
2.7.3
I'd suggest to wrap the long lines, but otherwise this looks good. So, ACK.
-Steffan
PATCH v2 1/1 Fix problems with NCP and --inetd.
In reply to Gert DoeringNCP only works with --pull or --mode server, leading to breakage in --inetd mode (because that has --tls-server, but not --mode server, but clients can still ask for PUSH_REQUEST).
Fix by turning off o->ncp_enable unless (pull or mode server), and double-fix by logging an appropriate message and refusing to change ciphers if the server has already set up its keys.
v2: wrap long msg() text lines
Trac #715
Signed-off-by Gert Doering <[email protected]...>
--- src/openvpn/options.c | 9 +++++++++ src/openvpn/push.c | 23 ++++++++++++++++++----- 2 files changed, 27 insertions(+), 5 deletions(-)
src/openvpn/options.c +9 -0
@@ -2614,6 +2614,15 @@ options_postprocess_mutate (struct options *o)if (streq (o->dh_file,"none" )) 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; + }#endif #if ENABLE_MANAGEMENT
src/openvpn/push.c +18 -5
@@ -321,11 +321,24 @@ prepare_push_reply (struct options *o, struct tls_multi *tls_multi)/* Push cipher if client supports Negotiable Crypto Parameters */ if (tls_peer_info_ncp_ver (peer_info) >=2 && o->ncp_enabled) { -/* Push the first cipher from --ncp-ciphers to the client. -* TODO: actual negotiation, instead of server dictatorship. */ -char *push_cipher =string_alloc (o->ncp_ciphers, &o->gc); - o->ciphername =strtok (push_cipher,":" ); -push_option_fmt (o,M_USAGE ,"cipher %s" , o->ciphername); +/* if we have already created our key, we cannot change our own +* cipher, so disable NCP and warn = explain why +*/ +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, " +"ignoring client request" ); + } +else + { +/* Push the first cipher from --ncp-ciphers to the client. +* TODO: actual negotiation, instead of server dictatorship. */ +char *push_cipher =string_alloc (o->ncp_ciphers, &o->gc); + o->ciphername =strtok (push_cipher,":" ); +push_option_fmt (o,M_USAGE ,"cipher %s" , o->ciphername); + } }return true; }
Signature · 1 line
2.7.3
Quoted 1 line · Gert Doering
v2: wrap long
msg()text lines
Thanks. ACK.
-Steffan
Your patch has been applied to the master branch.
commit d90249f73353c175ed9e7dd0a450cd084a729e20
Author: Gert Doering Date: Mon Aug 22 22:24:47 2016 +0200
Fix problems with NCP and --inetd.
Trac: 715
Signed-off-by: Gert Doering <[email protected]...>
Acked-by: Steffan Karger <[email protected]...>
Message-Id: [email protected]...
URL: https://www.mail-archive.com/[email protected]/msg00060.html
Signed-off-by: David Sommerseth <[email protected]...>--
kind regards,
David Sommerseth