Re: [Openvpn-devel] [PATCH] Fix NCP behaviour on TLS reconnect.

[Steffan Karger]

Hi,

On 18-05-17 12:22, Gert Doering wrote:
> 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.

> 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 
> 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(-)
> 
> diff --git a/src/openvpn/push.c b/src/openvpn/push.c
> index 8c3104e..bcef0ef 100644
> --- a/src/openvpn/push.c
> +++ b/src/openvpn/push.c
> @@ -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 = _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, >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

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [PATCH] Fix NCP behaviour on TLS reconnect.

[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.

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

diff --git a/src/openvpn/push.c b/src/openvpn/push.c
index 8c3104e..bcef0ef 100644
--- a/src/openvpn/push.c
+++ b/src/openvpn/push.c
@@ -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 = _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, >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)
 {
-- 
2.10.2


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel