Re: [Openvpn-devel] [PATCH] Fix problems with NCP and --inetd.

[Steffan Karger]

On 22 August 2016 at 21:22, Gert Doering  wrote:
> 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 
> ---
>  src/openvpn/options.c |  8 
>  src/openvpn/push.c| 21 -
>  2 files changed, 24 insertions(+), 5 deletions(-)
>
> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> index c100d4c..a2b22f4 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -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
> diff --git a/src/openvpn/push.c b/src/openvpn/push.c
> index 000c82f..4bdede9 100644
> --- a/src/openvpn/push.c
> +++ b/src/openvpn/push.c
> @@ -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, >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 = _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, >gc);
> + o->ciphername = strtok (push_cipher, ":");
> + push_option_fmt(o, M_USAGE, "cipher %s", o->ciphername);
> +   }
>  }
>return true;
>  }
> --
> 2.7.3

I'd suggest to wrap the long lines, but otherwise this looks good.  So, ACK.

-Steffan

--
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] Fix problems with NCP and --inetd.

[Steffan Karger]

On 22 August 2016 at 21:22, Gert Doering  wrote:
> 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 
> ---
>  src/openvpn/options.c |  8 
>  src/openvpn/push.c| 21 -
>  2 files changed, 24 insertions(+), 5 deletions(-)
>
> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> index c100d4c..a2b22f4 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -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
> diff --git a/src/openvpn/push.c b/src/openvpn/push.c
> index 000c82f..4bdede9 100644
> --- a/src/openvpn/push.c
> +++ b/src/openvpn/push.c
> @@ -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, >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 = _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, >gc);
> + o->ciphername = strtok (push_cipher, ":");
> + push_option_fmt(o, M_USAGE, "cipher %s", o->ciphername);
> +   }
>  }
>return true;
>  }
> --
> 2.7.3

I'd suggest to wrap the long lines, but otherwise this looks good.  So, ACK.

-Steffan

--
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [PATCH] Fix problems with NCP and --inetd.

[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_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 
---
 src/openvpn/options.c |  8 
 src/openvpn/push.c| 21 -
 2 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index c100d4c..a2b22f4 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -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
diff --git a/src/openvpn/push.c b/src/openvpn/push.c
index 000c82f..4bdede9 100644
--- a/src/openvpn/push.c
+++ b/src/openvpn/push.c
@@ -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, >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 = _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, >gc);
+ o->ciphername = strtok (push_cipher, ":");
+ push_option_fmt(o, M_USAGE, "cipher %s", o->ciphername);
+   }
 }
   return true;
 }
-- 
2.7.3


--
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [PATCH] Fix problems with NCP and --inetd.

[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_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 
---
 src/openvpn/options.c |  8 
 src/openvpn/push.c| 21 -
 2 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index c100d4c..a2b22f4 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -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
diff --git a/src/openvpn/push.c b/src/openvpn/push.c
index 000c82f..4bdede9 100644
--- a/src/openvpn/push.c
+++ b/src/openvpn/push.c
@@ -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, >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 = _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, >gc);
+ o->ciphername = strtok (push_cipher, ":");
+ push_option_fmt(o, M_USAGE, "cipher %s", o->ciphername);
+   }
 }
   return true;
 }
-- 
2.7.3


--
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel