Fix NCP behaviour on TLS reconnect.
PATCH 1/1 Fix NCP behaviour on TLS reconnect.
If a client reconnects on a soft-restart from the same port (due to --bind in use on the client), both sides will handle this as a "reconnect" and not a "full new connect" internally, re-using existing crypto context.
The client will still ask the server for pushed options, and the server code to handle this refuses to do NCP if a key has already been negotiated (because there is no way to *change* the cipher after that) - which ends up in "the client uses the non-negotiated cipher from the config file, while the server uses the previously-negotiated NCP cipher", and nothing works.
The easy workaround: if we find us in the situation that we think NCP has already been done, just re-push "cipher o->ciphername" with the current cipher for this client context.
All credits for this go to Stefan Behrens <[email protected]...> who found and diagnosed the issue in trac #887, came up with a first patch to solve the issue quite similar to this (simplified) one, and helped testing.
Trac: #887
--- src/openvpn/push.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
src/openvpn/push.c +6 -4
@@ -372,15 +372,17 @@ prepare_push_reply(struct context *c, struct gc_arena *gc,/* Push cipher if client supports Negotiable Crypto Parameters */ if (tls_peer_info_ncp_ver (peer_info) >=2 && o->ncp_enabled) { -/* if we have already created our key, we cannot change our own -* cipher, so disable NCP and warn = explain why +/* 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, " -"ignoring client request" ); +"re-sending previously negotiated cipher '%s'" , + o->ciphername ); }else {
@@ -388,8 +390,8 @@ prepare_push_reply(struct context *c, struct gc_arena *gc,* 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 (gc, push_list,M_USAGE ,"cipher %s" , o->ciphername); } +push_option_fmt (gc, push_list,M_USAGE ,"cipher %s" , o->ciphername); }else if (o->ncp_enabled) {
Signature · 1 line
2.10.2
Hi,
Quoted 3 lines · Gert Doering
If a client reconnects on a soft-restart from the same port (due to --bind in use on the client), both sides will handle this as a "reconnect" and not a "full new connect" internally, re-using existing crypto context.
This is actually a "hard reset", rather than a soft-restart. A "soft reset" does not trigger a new pull/push, just a new TLS key negotiation.
Quoted 48 lines
The client will still ask the server for pushed options, and the server code to handle this refuses to do NCP if a key has already been negotiated (because there is no way to *change* the cipher after that) - which ends up in "the client uses the non-negotiated cipher from the config file, while the server uses the previously-negotiated NCP cipher", and nothing works.
The easy workaround: if we find us in the situation that we think NCP has already been done, just re-push "cipher
o->ciphername" with the current cipher for this client context.All credits for this go to Stefan Behrens <[email protected]...> who found and diagnosed the issue in trac #887, came up with a first patch to solve the issue quite similar to this (simplified) one, and helped testing.
Trac: #887
--- src/openvpn/push.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)src/openvpn/push.c +5
-3@@ -372,15 +372,17 @@ prepare_push_reply(struct context *c, struct gc_arena *gc,/* Push cipher if client supports Negotiable Crypto Parameters */ if (tls_peer_info_ncp_ver (peer_info) >=2 && o->ncp_enabled) { -/* if we have already created our key, we cannot change our own -* cipher, so disable NCP and warn = explain why +/* 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, " -"ignoring client request" ); +"re-sending previously negotiated cipher '%s'" , + o->ciphername ); }else {
src/openvpn/push.c prepare_push_reply(struct context *c, struct gc_arena *gc, · line 397
Quoted 9 lines
src/openvpn/push.c +1
-1@@ -388,8 +390,8 @@ prepare_push_reply(struct context *c, struct gc_arena *gc,* 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 (gc, push_list,M_USAGE ,"cipher %s" , o->ciphername); } +push_option_fmt (gc, push_list,M_USAGE ,"cipher %s" , o->ciphername); }else if (o->ncp_enabled) {
Otherwise, patch looks good, builds fine and does as specifiek. So ACK, if you do s/soft/hard/ in the commit msg.
-Steffan
Patch has been applied to the master and release/2.4 branch, with the soft/hard correction pointed out by Steffan.
Thanks to everyone helping here.
commit 5634cecf71ee9a92227bc9c8414c614d1b741abb (master) commit 13c05ca4e9da88ef30a778c16a97f0c0d767b448 (release/2.4)
Author: Gert Doering Date: Thu May 18 12:22:46 2017 +0200
Fix NCP behaviour on TLS reconnect.
Acked-by: Steffan Karger <[email protected]...>
Message-Id: <[email protected]...>
URL: https://www.mail-archive.com/[email protected]/msg14666.html
Signed-off-by: Gert Doering <[email protected]...>--
kind regards,
Gert Doering