Skip to contentSkip to conversation

Poor man's NCP for 2.3 clients.

PATCH New Gert Doering 2 participants 14 messages Nov 4, 2016 – Nov 24, 2016

lore
Share & tools
lore

Gert Doering

author
#0 lore raw

PATCH 1/1 Poor man's NCP for 2.3 clients.

Proper cipher negotiation needs clients that announce IV_NCP=1 and can handle pushing of ciphers.

2.3 clients can not do that. To work around that, here's a hacky patch to make it work.

On the client side, set

cipher AES-128-CBC
push-peer-info
setenv UV_CIPHER AES-128-CBC

(same cipher!). On the server side, run a client-connect script that will create a per-instance "cipher $UV_CIPHER" config line if UV_CIPHER is set.

To make this work, make "cipher" settable per-instance (which it effectively already is, options.c just did not learn about it) AND delay cipher initialization even if IV_NCP is not set if the clients sends UV_CIPHER.

Use with care, without the proper server-side script (or plugin), it will just cause non-working setups.

Signed-off-by Gert Doering <[email protected]...>
---
 src/openvpn/options.c | 2 +-
 src/openvpn/ssl.c     | 9 ++++++++-
 2 files changed, 9 insertions(+), 2 deletions(-)
src/openvpn/options.c +1 -1
index 552bf5a..5b943c5 100644
@@ -6677,7 +6677,7 @@ add_option (struct options *options,
     }
   else if (streq (p[0], "cipher") && p[1] && !p[2])
     {
-      VERIFY_PERMISSION (OPT_P_NCP);
+      VERIFY_PERMISSION (OPT_P_NCP|OPT_P_INSTANCE);
       options->ciphername = p[1];
     }
   else if (streq (p[0], "ncp-ciphers") && p[1] && !p[2])
src/openvpn/ssl.c +8 -1
index 4a0cd6f..5ee1882 100644
@@ -2258,7 +2258,14 @@ key_method_2_read (struct buffer *buf, struct tls_multi *multi, struct tls_sessi
   if (tls_peer_info_ncp_ver (multi->peer_info) < 2)
     {
       /* Peer does not support NCP */
-      session->opt->ncp_enabled = false;
+      if ( strstr (multi->peer_info, "UV_CIPHER=") != NULL )
+	{
+	  msg( M_INFO, "peer doesn't do NCP, but sends UV_CIPHER, leaving NCP enabled" );
+	}
+      else
+	{
+	  session->opt->ncp_enabled = false;
+	}
     }
 #endif
Signature · 1 line
2.7.3
#1 lore raw

PATCH 1/1 Poor man's NCP for non-NCP clients

Allows non-NCP clients (<= 2.3, or 2.4+ with --ncp-disable) to specify a --cipher that is different from the one in the server config, as long as the new cipher value is allowed (i.e. in --ncp-ciphers at the server side).

This patch was inspired by Gert's "Poor man's NCP for 2.3 clients" patch, but takes a different approach to avoid the need for server-side scripts or client-side 'setenv UV_*' tricks.

Signed-off-by Steffan Karger <[email protected]...>
---
 src/openvpn/options.c | 32 ++++++++++++++++++++++++++++++++
 src/openvpn/options.h | 14 ++++++++++++++
 src/openvpn/ssl.c     | 24 +++++++++++++++++++++---
 src/openvpn/ssl.h     |  7 +++++++
 4 files changed, 74 insertions(+), 3 deletions(-)
src/openvpn/options.c +32 -0
index a74de24..94cd3d4 100644
@@ -3431,6 +3431,38 @@ options_string_version (const char* s, struct gc_arena *gc)
   return BSTR (&out);
 }
 
+char *
+options_string_extract_option (const char *options_string,
+    size_t options_string_len, const char *opt_name, struct gc_arena *gc)
+{
+  char *ret = NULL;
+
+  const char *p = options_string;
+  while (p)
+    {
+      if (p == strstr(p, opt_name))
+	{
+	  const size_t opt_name_len = strlen(opt_name);
+	  if (strlen(p) > opt_name_len+1 && p[opt_name_len] == ' ')
+	    {
+	      /* option found, extract value */
+	      const char *start = &p[opt_name_len+1];
+	      const char *end = strchr (p, ',');
+	      size_t val_len = end ? end - start : strlen (start);
+	      ret = gc_malloc (val_len+1, true, gc);
+	      memcpy (ret, start, val_len);
+	      break;
+	    }
+	}
+      p = strchr (p, ',');
+      if (p)
+	{
+	  p++; /* skip delimiter */
+	}
+    }
+  return ret;
+}
+
 #endif /* ENABLE_OCC */
 
 static void
src/openvpn/options.h +14 -0
index a028556..a66fd84 100644
@@ -724,6 +724,20 @@ void options_warning_safe (char *actual, const char *expected, size_t actual_n);
 bool options_cmp_equal (char *actual, const char *expected);
 void options_warning (char *actual, const char *expected);
 
+/**
+ * Given an OpenVPN options string, extract the value of an option.
+ *
+ * @param options_string	Zero-terminated, comma-separated options string
+ * @param opt_name		The name of the option to extract
+ * @param gc			The gc to allocate the return value
+ *
+ * @return gc-allocated value of option with name opt_name if option was found,
+ *         or NULL otherwise.
+ */
+char *options_string_extract_option (const char *options_string,
+    size_t options_string_len, const char *opt_name, struct gc_arena *gc);
+
+
 #endif
 
 void options_postprocess (struct options *options);
src/openvpn/ssl.c +21 -3
index 1ae1547..4080ffb 100644
@@ -1723,8 +1723,8 @@ key_ctx_update_implicit_iv(struct key_ctx *ctx, uint8_t *key, size_t key_len) {
     }
 }
 
-static bool
-item_in_list(const char *item, const char *list)
+bool
+tls_item_in_cipher_list(const char *item, const char *list)
 {
   char *tmp_ciphers = string_alloc (list, NULL);
   char *tmp_ciphers_orig = tmp_ciphers;

@@ -1752,7 +1752,7 @@ tls_session_update_crypto_params(struct tls_session *session,
 
   if (!session->opt->server &&
       0 != strcmp(options->ciphername, session->opt->config_ciphername) &&
-      !item_in_list(options->ciphername, options->ncp_ciphers))
+      !tls_item_in_cipher_list(options->ciphername, options->ncp_ciphers))
     {
       msg (D_TLS_ERRORS, "Error: pushed cipher not allowed - %s not in %s or %s",
 	  options->ciphername, session->opt->config_ciphername,

@@ -2318,6 +2318,24 @@ key_method_2_read (struct buffer *buf, struct context *c, struct tls_session *se
       /* Peer does not support NCP */
       session->opt->ncp_enabled = false;
     }
+
+  /* "Poor man's NCP": Use client-cipher if it is an allowed (NCP) cipher.
+   * Allows non-NCP client to upgrade their cipher individually. */
+  char *options_string_remote_cipher = options_string_extract_option (options,
+      TLS_OPTIONS_LEN, "cipher", &gc);
+  if (!session->opt->ncp_enabled && options_string_remote_cipher &&
+      0 != strcmp(c->options.ciphername, options_string_remote_cipher))
+    {
+      if (tls_item_in_cipher_list(options_string_remote_cipher,
+	  c->options.ncp_ciphers))
+	{
+	  c->options.ciphername = string_alloc(options_string_remote_cipher,
+	      &c->options.gc);
+	  session->opt->ncp_enabled = true;
+	  msg (D_TLS_DEBUG_LOW, "Using client cipher '%s'", c->options.ciphername);
+	}
+    }
+
 #endif
 
   if (tls_session_user_pass_enabled(session))
src/openvpn/ssl.h +7 -0
index 990d8da..8f588c6 100644
@@ -508,6 +508,13 @@ int tls_peer_info_ncp_ver(const char *peer_info);
  */
 bool tls_check_ncp_cipher_list(const char *list);
 
+/**
+ * Return true iff item is present in the colon-separated zero-terminated
+ * cipher list.
+ */
+bool tls_item_in_cipher_list(const char *item, const char *list);
+
+
 /*
  * inline functions
  */
Signature · 1 line
2.7.4
#2 lore raw

PATCH v2 1/1 Poor man's NCP for non-NCP clients

Allows non-NCP clients (<= 2.3, or 2.4+ with --ncp-disable) to specify a --cipher that is different from the one in the server config, as long as the new cipher value is allowed (i.e. in --ncp-ciphers at the server side).

This patch was inspired by Gert's "Poor man's NCP for 2.3 clients" patch, but takes a different approach to avoid the need for server-side scripts or client-side 'setenv UV_*' tricks.

Signed-off-by Steffan Karger <[email protected]...>

---

v2: remove unused parameter from options_string_extract_option()
prototype.

 src/openvpn/options.c | 32 ++++++++++++++++++++++++++++++++
 src/openvpn/options.h | 14 ++++++++++++++
 src/openvpn/ssl.c     | 24 +++++++++++++++++++++---
 src/openvpn/ssl.h     |  7 +++++++
 4 files changed, 74 insertions(+), 3 deletions(-)
src/openvpn/options.c +32 -0
index a74de24..94cd3d4 100644
@@ -3431,6 +3431,38 @@ options_string_version (const char* s, struct gc_arena *gc)
   return BSTR (&out);
 }
 
+char *
+options_string_extract_option (const char *options_string,
+    size_t options_string_len, const char *opt_name, struct gc_arena *gc)
+{
+  char *ret = NULL;
+
+  const char *p = options_string;
+  while (p)
+    {
+      if (p == strstr(p, opt_name))
+	{
+	  const size_t opt_name_len = strlen(opt_name);
+	  if (strlen(p) > opt_name_len+1 && p[opt_name_len] == ' ')
+	    {
+	      /* option found, extract value */
+	      const char *start = &p[opt_name_len+1];
+	      const char *end = strchr (p, ',');
+	      size_t val_len = end ? end - start : strlen (start);
+	      ret = gc_malloc (val_len+1, true, gc);
+	      memcpy (ret, start, val_len);
+	      break;
+	    }
+	}
+      p = strchr (p, ',');
+      if (p)
+	{
+	  p++; /* skip delimiter */
+	}
+    }
+  return ret;
+}
+
 #endif /* ENABLE_OCC */
 
 static void
src/openvpn/options.h +14 -0
index a028556..a66fd84 100644
@@ -724,6 +724,20 @@ void options_warning_safe (char *actual, const char *expected, size_t actual_n);
 bool options_cmp_equal (char *actual, const char *expected);
 void options_warning (char *actual, const char *expected);
 
+/**
+ * Given an OpenVPN options string, extract the value of an option.
+ *
+ * @param options_string	Zero-terminated, comma-separated options string
+ * @param opt_name		The name of the option to extract
+ * @param gc			The gc to allocate the return value
+ *
+ * @return gc-allocated value of option with name opt_name if option was found,
+ *         or NULL otherwise.
+ */
+char *options_string_extract_option (const char *options_string,
+    size_t options_string_len, const char *opt_name, struct gc_arena *gc);
+
+
 #endif
 
 void options_postprocess (struct options *options);
src/openvpn/ssl.c +21 -3
index 1ae1547..4080ffb 100644
@@ -1723,8 +1723,8 @@ key_ctx_update_implicit_iv(struct key_ctx *ctx, uint8_t *key, size_t key_len) {
     }
 }
 
-static bool
-item_in_list(const char *item, const char *list)
+bool
+tls_item_in_cipher_list(const char *item, const char *list)
 {
   char *tmp_ciphers = string_alloc (list, NULL);
   char *tmp_ciphers_orig = tmp_ciphers;

@@ -1752,7 +1752,7 @@ tls_session_update_crypto_params(struct tls_session *session,
 
   if (!session->opt->server &&
       0 != strcmp(options->ciphername, session->opt->config_ciphername) &&
-      !item_in_list(options->ciphername, options->ncp_ciphers))
+      !tls_item_in_cipher_list(options->ciphername, options->ncp_ciphers))
     {
       msg (D_TLS_ERRORS, "Error: pushed cipher not allowed - %s not in %s or %s",
 	  options->ciphername, session->opt->config_ciphername,

@@ -2318,6 +2318,24 @@ key_method_2_read (struct buffer *buf, struct context *c, struct tls_session *se
       /* Peer does not support NCP */
       session->opt->ncp_enabled = false;
     }
+
+  /* "Poor man's NCP": Use client-cipher if it is an allowed (NCP) cipher.
+   * Allows non-NCP client to upgrade their cipher individually. */
+  char *options_string_remote_cipher = options_string_extract_option (options,
+      TLS_OPTIONS_LEN, "cipher", &gc);
+  if (!session->opt->ncp_enabled && options_string_remote_cipher &&
+      0 != strcmp(c->options.ciphername, options_string_remote_cipher))
+    {
+      if (tls_item_in_cipher_list(options_string_remote_cipher,
+	  c->options.ncp_ciphers))
+	{
+	  c->options.ciphername = string_alloc(options_string_remote_cipher,
+	      &c->options.gc);
+	  session->opt->ncp_enabled = true;
+	  msg (D_TLS_DEBUG_LOW, "Using client cipher '%s'", c->options.ciphername);
+	}
+    }
+
 #endif
 
   if (tls_session_user_pass_enabled(session))
src/openvpn/ssl.h +7 -0
index 990d8da..8f588c6 100644
@@ -508,6 +508,13 @@ int tls_peer_info_ncp_ver(const char *peer_info);
  */
 bool tls_check_ncp_cipher_list(const char *list);
 
+/**
+ * Return true iff item is present in the colon-separated zero-terminated
+ * cipher list.
+ */
+bool tls_item_in_cipher_list(const char *item, const char *list);
+
+
 /*
  * inline functions
  */
Signature · 1 line
2.7.4
#3 lore raw

PATCH v3 1/1 Poor man's NCP for non-NCP clients

Allows non-NCP clients (<= 2.3, or 2.4+ with --ncp-disable) to specify a --cipher that is different from the one in the server config, as long as the new cipher value is allowed (i.e. in --ncp-ciphers at the server side).

This patch was inspired by Gert's "Poor man's NCP for 2.3 clients" patch, but takes a different approach to avoid the need for server-side scripts or client-side 'setenv UV_*' tricks.

To achieve this cleanly, tls_session_update_crypto_parameters() had to be split into 'update' and 'generate keys' parts. I used the opportunity to introduce a simpler API that works for the other places where we used 'generate_key_expansion()' too.

Signed-off-by Steffan Karger <[email protected]...>

---
v2: remove unused argument from options_string_extract_option()
v3: don't do poor-man's NCP if NCP is disabled

 src/openvpn/init.c    |  4 ++-
 src/openvpn/options.c | 32 ++++++++++++++++++
 src/openvpn/options.h | 14 ++++++++
 src/openvpn/push.c    |  7 ++--
 src/openvpn/ssl.c     | 94 ++++++++++++++++++++++++++++-----------------------
 src/openvpn/ssl.h     | 19 +++++++++--
 6 files changed, 122 insertions(+), 48 deletions(-)
src/openvpn/init.c +3 -1
index 2ed633c..ba19ac4 100644
@@ -1928,7 +1928,9 @@ do_deferred_options (struct context *c, const unsigned int found)
 	msg (D_PUSH, "OPTIONS IMPORT: data channel crypto options modified");
       /* Do not regenerate keys if server sends an extra push request */
       if (!session->key[KS_PRIMARY].crypto_options.key_ctx_bi.initialized &&
-	  !tls_session_update_crypto_params(session, &c->options, &c->c2.frame))
+	  (!tls_session_update_crypto_params (session, &c->options,
+	      &c->c2.frame) ||
+	   !tls_session_generate_data_channel_keys(session)))
 	{
 	  msg (D_TLS_ERRORS, "OPTIONS ERROR: failed to import crypto options");
 	  return false;
src/openvpn/options.c +32 -0
index a74de24..778fcbd 100644
@@ -3431,6 +3431,38 @@ options_string_version (const char* s, struct gc_arena *gc)
   return BSTR (&out);
 }
 
+char *
+options_string_extract_option (const char *options_string,const char *opt_name,
+    struct gc_arena *gc)
+{
+  char *ret = NULL;
+
+  const char *p = options_string;
+  while (p)
+    {
+      if (p == strstr(p, opt_name))
+	{
+	  const size_t opt_name_len = strlen(opt_name);
+	  if (strlen(p) > opt_name_len+1 && p[opt_name_len] == ' ')
+	    {
+	      /* option found, extract value */
+	      const char *start = &p[opt_name_len+1];
+	      const char *end = strchr (p, ',');
+	      size_t val_len = end ? end - start : strlen (start);
+	      ret = gc_malloc (val_len+1, true, gc);
+	      memcpy (ret, start, val_len);
+	      break;
+	    }
+	}
+      p = strchr (p, ',');
+      if (p)
+	{
+	  p++; /* skip delimiter */
+	}
+    }
+  return ret;
+}
+
 #endif /* ENABLE_OCC */
 
 static void
src/openvpn/options.h +14 -0
index a028556..913bfde 100644
@@ -724,6 +724,20 @@ void options_warning_safe (char *actual, const char *expected, size_t actual_n);
 bool options_cmp_equal (char *actual, const char *expected);
 void options_warning (char *actual, const char *expected);
 
+/**
+ * Given an OpenVPN options string, extract the value of an option.
+ *
+ * @param options_string	Zero-terminated, comma-separated options string
+ * @param opt_name		The name of the option to extract
+ * @param gc			The gc to allocate the return value
+ *
+ * @return gc-allocated value of option with name opt_name if option was found,
+ *         or NULL otherwise.
+ */
+char *options_string_extract_option (const char *options_string,
+    const char *opt_name, struct gc_arena *gc);
+
+
 #endif
 
 void options_postprocess (struct options *options);
src/openvpn/push.c +4 -3
index b7539e6..01bc63c 100644
@@ -259,10 +259,11 @@ incoming_push_message (struct context *c, const struct buffer *buffer)
 	  struct tls_session *session = &c->c2.tls_multi->session[TM_ACTIVE];
 	  /* Do not regenerate keys if client send a second push request */
 	  if (!session->key[KS_PRIMARY].crypto_options.key_ctx_bi.initialized &&
-	      !tls_session_update_crypto_params (session, &c->options,
-		  &c->c2.frame))
+	      (!tls_session_update_crypto_params (session, &c->options,
+		  &c->c2.frame) ||
+	       !tls_session_generate_data_channel_keys(session)))
 	    {
-	      msg (D_TLS_ERRORS, "TLS Error: server generate_key_expansion failed");
+	      msg (D_TLS_ERRORS, "TLS Error: initializing data channel failed");
 	      goto error;
 	    }
 	}
src/openvpn/ssl.c +52 -42

147 lines not on this page · raw message

src/openvpn/ssl.h +17 -2
index 990d8da..f9bec37 100644
@@ -472,8 +472,16 @@ void tls_update_remote_addr (struct tls_multi *multi,
 			     const struct link_socket_actual *addr);
 
 /**
- * Update TLS session crypto parameters (cipher and auth) and derive data
- * channel keys based on the supplied options.
+ * Generate data channel keys for the supplied TLS session.
+ *
+ * This erases the source material used to generate the data channel keys, and
+ * can thus be called only once per session.
+ */
+bool tls_session_generate_data_channel_keys(struct tls_session *session);
+
+/**
+ * Update TLS session crypto parameters (cipher and auth) and recalculate
+ * resulting frame sizes.
  *
  * @param session	The TLS session to update.
  * @param options	The options to use when updating session.

@@ -508,6 +516,13 @@ int tls_peer_info_ncp_ver(const char *peer_info);
  */
 bool tls_check_ncp_cipher_list(const char *list);
 
+/**
+ * Return true iff item is present in the colon-separated zero-terminated
+ * cipher list.
+ */
+bool tls_item_in_cipher_list(const char *item, const char *list);
+
+
 /*
  * inline functions
  */
Signature · 1 line
2.7.4
#4 lore raw

PATCH v4 1/1 Poor man's NCP for non-NCP peers

Allows non-NCP peers (<= 2.3, or 2.4+ with --ncp-disable) to specify a --cipher that is different from the one in our config, as long as the new cipher value is allowed (i.e. in --ncp-ciphers at our side).

This works both client-to-server and server-to-client. I.e. a 2.4 client with "cipher BF-CBC" and "ncp-ciphers AES-256-GCM:AES-256-CBC" can connect to both a 2.3 server with "cipher BF-CBC" as well as a server with "cipher AES-256-CBC" in its config. The other way around, a 2.3 client with either "cipher BF-CBC" or "cipher AES-256-CBC" can connect to a 2.4 server with e.g. "cipher BF-CBC" and "ncp-ciphers AES-256-GCM:AES-256-CBC" in its config.

This patch was inspired by Gert's "Poor man's NCP for 2.3 clients" patch, but takes a different approach to avoid the need for server-side scripts or client-side 'setenv UV_*' tricks.

To achieve this cleanly, tls_session_update_crypto_parameters() had to be split into 'update' and 'generate keys' parts. I used the opportunity to introduce a simpler API that works for the other places where we used 'generate_key_expansion()' too.

Signed-off-by Steffan Karger <[email protected]...>

---
v2: remove unused argument from options_string_extract_option()
v3: don't do poor-man's NCP if NCP is disabled
v4: use strncmp instead of strstr to match prefix in
options_string_extract_option, add man page text and make clear that this feature works both client-to-server and server-to-client.

 doc/openvpn.8         | 14 ++++++--
 src/openvpn/init.c    |  4 ++-
 src/openvpn/options.c | 32 ++++++++++++++++++
 src/openvpn/options.h | 14 ++++++++
 src/openvpn/push.c    |  7 ++--
 src/openvpn/ssl.c     | 93 ++++++++++++++++++++++++++++-----------------------
 src/openvpn/ssl.h     | 19 +++++++++--
 7 files changed, 133 insertions(+), 50 deletions(-)
doc/openvpn.8 +12 -2
index 11564af..c9c53fa 100644
@@ -4147,9 +4147,9 @@ to disable encryption.
 As of OpenVPN 2.4, cipher negotiation (NCP) can override the cipher specified by
 .B \-\-cipher\fR.
 See
-.B \-\-ncp-ciphers
+.B \-\-ncp\-ciphers
 and
-.B \-\-ncp-disable
+.B \-\-ncp\-disable
 for more on NCP.
 
 .\"*********************************************************

@@ -4177,6 +4177,16 @@ If both peers support and do not disable NCP, the negotiated cipher will
 override the cipher specified by
 .B \-\-cipher\fR.
 
+Additionally, to allow for more smooth transition, if NCP is enabled, OpenVPN
+will inherit the cipher of the peer if that cipher is different from the local
+.B \-\-cipher
+setting, but the peer cipher is one of the ciphers specified in
+.B \-\-ncp\-ciphers\fR.
+E.g. a non-NCP client (<=2.3, or with \-\-ncp\-disabled set) connecting to a
+NCP server (2.4+) with "\-\-cipher BF-CBC" and "\-\-ncp-ciphers
+AES-256-GCM:AES-256-CBC" set can either specify "\-\-cipher BF-CBC" or
+"\-\-cipher AES-256-CBC" and both will work.
+
 .\"*********************************************************
 .TP
 .B \-\-ncp\-disable
src/openvpn/init.c +3 -1
index 2ed633c..ba19ac4 100644
@@ -1928,7 +1928,9 @@ do_deferred_options (struct context *c, const unsigned int found)
 	msg (D_PUSH, "OPTIONS IMPORT: data channel crypto options modified");
       /* Do not regenerate keys if server sends an extra push request */
       if (!session->key[KS_PRIMARY].crypto_options.key_ctx_bi.initialized &&
-	  !tls_session_update_crypto_params(session, &c->options, &c->c2.frame))
+	  (!tls_session_update_crypto_params (session, &c->options,
+	      &c->c2.frame) ||
+	   !tls_session_generate_data_channel_keys(session)))
 	{
 	  msg (D_TLS_ERRORS, "OPTIONS ERROR: failed to import crypto options");
 	  return false;
src/openvpn/options.c +32 -0
index a74de24..c23ae5e 100644
@@ -1642,6 +1642,8 @@ show_settings (const struct options *o)
   SHOW_STR (shared_secret_file);
   SHOW_INT (key_direction);
   SHOW_STR (ciphername);
+  SHOW_BOOL (ncp_enabled);
+  SHOW_STR (ncp_ciphers);
   SHOW_STR (authname);
   SHOW_STR (prng_hash);
   SHOW_INT (prng_nonce_secret_len);

@@ -3431,6 +3433,36 @@ options_string_version (const char* s, struct gc_arena *gc)
   return BSTR (&out);
 }
 
+char *
+options_string_extract_option (const char *options_string,const char *opt_name,
+    struct gc_arena *gc)
+{
+  char *ret = NULL;
+  const size_t opt_name_len = strlen(opt_name);
+
+  const char *p = options_string;
+  while (p)
+    {
+      if (0 == strncmp(p, opt_name, opt_name_len) &&
+	  strlen(p) > (opt_name_len+1) && p[opt_name_len] == ' ')
+	{
+	  /* option found, extract value */
+	  const char *start = &p[opt_name_len+1];
+	  const char *end = strchr (p, ',');
+	  size_t val_len = end ? end - start : strlen (start);
+	  ret = gc_malloc (val_len+1, true, gc);
+	  memcpy (ret, start, val_len);
+	  break;
+	}
+      p = strchr (p, ',');
+      if (p)
+	{
+	  p++; /* skip delimiter */
+	}
+    }
+  return ret;
+}
+
 #endif /* ENABLE_OCC */
 
 static void
src/openvpn/options.h +14 -0
index a028556..913bfde 100644
@@ -724,6 +724,20 @@ void options_warning_safe (char *actual, const char *expected, size_t actual_n);
 bool options_cmp_equal (char *actual, const char *expected);
 void options_warning (char *actual, const char *expected);
 
+/**
+ * Given an OpenVPN options string, extract the value of an option.
+ *
+ * @param options_string	Zero-terminated, comma-separated options string
+ * @param opt_name		The name of the option to extract
+ * @param gc			The gc to allocate the return value
+ *
+ * @return gc-allocated value of option with name opt_name if option was found,
+ *         or NULL otherwise.
+ */
+char *options_string_extract_option (const char *options_string,
+    const char *opt_name, struct gc_arena *gc);
+
+
 #endif
 
 void options_postprocess (struct options *options);
src/openvpn/push.c +4 -3
index b7539e6..01bc63c 100644
@@ -259,10 +259,11 @@ incoming_push_message (struct context *c, const struct buffer *buffer)
 	  struct tls_session *session = &c->c2.tls_multi->session[TM_ACTIVE];
 	  /* Do not regenerate keys if client send a second push request */
 	  if (!session->key[KS_PRIMARY].crypto_options.key_ctx_bi.initialized &&
-	      !tls_session_update_crypto_params (session, &c->options,
-		  &c->c2.frame))
+	      (!tls_session_update_crypto_params (session, &c->options,
+		  &c->c2.frame) ||
+	       !tls_session_generate_data_channel_keys(session)))
 	    {
-	      msg (D_TLS_ERRORS, "TLS Error: server generate_key_expansion failed");
+	      msg (D_TLS_ERRORS, "TLS Error: initializing data channel failed");
 	      goto error;
 	    }
 	}
src/openvpn/ssl.c +51 -42

146 lines not on this page · raw message

src/openvpn/ssl.h +17 -2
index 990d8da..f9bec37 100644
@@ -472,8 +472,16 @@ void tls_update_remote_addr (struct tls_multi *multi,
 			     const struct link_socket_actual *addr);
 
 /**
- * Update TLS session crypto parameters (cipher and auth) and derive data
- * channel keys based on the supplied options.
+ * Generate data channel keys for the supplied TLS session.
+ *
+ * This erases the source material used to generate the data channel keys, and
+ * can thus be called only once per session.
+ */
+bool tls_session_generate_data_channel_keys(struct tls_session *session);
+
+/**
+ * Update TLS session crypto parameters (cipher and auth) and recalculate
+ * resulting frame sizes.
  *
  * @param session	The TLS session to update.
  * @param options	The options to use when updating session.

@@ -508,6 +516,13 @@ int tls_peer_info_ncp_ver(const char *peer_info);
  */
 bool tls_check_ncp_cipher_list(const char *list);
 
+/**
+ * Return true iff item is present in the colon-separated zero-terminated
+ * cipher list.
+ */
+bool tls_item_in_cipher_list(const char *item, const char *list);
+
+
 /*
  * inline functions
  */
Signature · 1 line
2.7.4
author
#5 lore raw

Re: [Openvpn-devel] [PATCH v4] Poor man's NCP for non-NCP peers

Hi,

I totally want to have this, and overall the changes all make sense (so ACK for the refactoring part, needs a few windows held side by side in addition to tkdiff... :-) ), but something puzzles me...

On Tue, Nov 22, 2016 at 08:57:11PM +0100, Steffan Karger wrote:
[..]

src/openvpn/ssl.c key_method_2_read (struct buffer *buf, struct context *c, struct tls_session *se · line 2327

Quoted 11 lines
src/openvpn/ssl.c +7 -0
@@ -2320,6 +2318,26 @@ key_method_2_read (struct buffer *buf, struct context *c, struct tls_session *se
       /* Peer does not support NCP */
       session->opt->ncp_enabled = false;
     }
+
+  /* "Poor man's NCP": Use peer cipher if it is an allowed (NCP) cipher.
+   * Allows non-NCP client to upgrade their cipher individually. */
+  char *remote_cipher = options_string_extract_option (options, "cipher", &gc);
+  if (c->options.ncp_enabled && !session->opt->ncp_enabled && remote_cipher &&
+      0 != strcmp(c->options.ciphername, remote_cipher))
+    {

This looks totally logical and all, until I try to compile it - and then the compiler ruins the day for me...

../../../openvpn/src/openvpn/ssl.c: In function 'key_method_2_read':
../../../openvpn/src/openvpn/ssl.c:2313:7: error: 'c' undeclared (first use in this function)
if (c->options.ncp_enabled && !session->opt->ncp_enabled && remote_cipher &&

       ^

... and I can not seriously argue with it here... - there is no "c".

(David pointed out that *his* patch brings in the "c" here, but that one didn't receive enough love yet, so is not in *my* tree...)

So - this needs to be sorted out.

There is another catch here - options_string_extract_option() is wrapped in an #ifdef ENABLE_OCC, which is turned off if you configure --enable-small (so linking breaks). That second catch can be easily fixed by wrapping this whole code new code block in #ifdef ENABLE_OCC as well (if the data is just not enable in this case) or by moving the "extract" function after the corresponding #endif...

Finally, moving over tls_limit_reneg_bytes() broke the whitespace in weird ways - it got mangled to

tls_limit_reneg_bytes (session->opt->key_type.cipher,

        &session->opt->renegotiate_bytes);

while it was

tls_limit_reneg_bytes (session->opt->key_type.cipher,

                         &session->opt->renegotiate_bytes);

(I could fix that on the fly, and also the #ifdef ENABLE_OCC, but not the "c" one)

"Can I have a v5, please...?"

gert

Signature · 4 lines
USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...
#6 lore raw

PATCH v5 1/2 Poor man's NCP for non-NCP peers

Allows non-NCP peers (<= 2.3, or 2.4+ with --ncp-disable) to specify a --cipher that is different from the one in our config, as long as the new cipher value is allowed (i.e. in --ncp-ciphers at our side).

This works both client-to-server and server-to-client. I.e. a 2.4 client with "cipher BF-CBC" and "ncp-ciphers AES-256-GCM:AES-256-CBC" can connect to both a 2.3 server with "cipher BF-CBC" as well as a server with "cipher AES-256-CBC" in its config. The other way around, a 2.3 client with either "cipher BF-CBC" or "cipher AES-256-CBC" can connect to a 2.4 server with e.g. "cipher BF-CBC" and "ncp-ciphers AES-256-GCM:AES-256-CBC" in its config.

This patch was inspired by Gert's "Poor man's NCP for 2.3 clients" patch, but takes a different approach to avoid the need for server-side scripts or client-side 'setenv UV_*' tricks.

Signed-off-by Steffan Karger <[email protected]...>

---
v2: remove unused argument from options_string_extract_option()
v3: don't do poor-man's NCP if NCP is disabled
v4: use strncmp instead of strstr to match prefix in
options_string_extract_option, add man page text and make clear that this feature works both client-to-server and server-to-client.
v5: move poor-mans-ncp logic from key_method_2_read() to the place
where we do regular NCP too. This makes that we no longer need access to the struct context in key_method_2_read, and is cleaner in general, and makes the patch a bit smaller.

 doc/openvpn.8            | 14 ++++++++++++--
 src/openvpn/init.c       |  8 +++++++-
 src/openvpn/options.c    | 32 ++++++++++++++++++++++++++++++++
 src/openvpn/options.h    | 14 ++++++++++++++
 src/openvpn/push.c       |  6 +++++-
 src/openvpn/ssl.c        | 39 ++++++++++++++++++++++++++-------------
 src/openvpn/ssl.h        | 16 ++++++++++++++++
 src/openvpn/ssl_common.h |  2 ++
 8 files changed, 114 insertions(+), 17 deletions(-)
doc/openvpn.8 +12 -2
index 171bd72..4356597 100644
@@ -4147,9 +4147,9 @@ to disable encryption.
 As of OpenVPN 2.4, cipher negotiation (NCP) can override the cipher specified by
 .B \-\-cipher\fR.
 See
-.B \-\-ncp-ciphers
+.B \-\-ncp\-ciphers
 and
-.B \-\-ncp-disable
+.B \-\-ncp\-disable
 for more on NCP.
 
 .\"*********************************************************

@@ -4177,6 +4177,16 @@ If both peers support and do not disable NCP, the negotiated cipher will
 override the cipher specified by
 .B \-\-cipher\fR.
 
+Additionally, to allow for more smooth transition, if NCP is enabled, OpenVPN
+will inherit the cipher of the peer if that cipher is different from the local
+.B \-\-cipher
+setting, but the peer cipher is one of the ciphers specified in
+.B \-\-ncp\-ciphers\fR.
+E.g. a non-NCP client (<=2.3, or with \-\-ncp\-disabled set) connecting to a
+NCP server (2.4+) with "\-\-cipher BF-CBC" and "\-\-ncp-ciphers
+AES-256-GCM:AES-256-CBC" set can either specify "\-\-cipher BF-CBC" or
+"\-\-cipher AES-256-CBC" and both will work.
+
 .\"*********************************************************
 .TP
 .B \-\-ncp\-disable
src/openvpn/init.c +7 -1
index 2ed633c..1954947 100644
@@ -1925,7 +1925,13 @@ do_deferred_options (struct context *c, const unsigned int found)
     {
       struct tls_session *session = &c->c2.tls_multi->session[TM_ACTIVE];
       if (found & OPT_P_NCP)
-	msg (D_PUSH, "OPTIONS IMPORT: data channel crypto options modified");
+	{
+	  msg (D_PUSH, "OPTIONS IMPORT: data channel crypto options modified");
+	}
+      else if (c->options.ncp_enabled)
+	{
+	  tls_poor_mans_ncp(&c->options, c->c2.tls_multi->remote_ciphername);
+	}
       /* Do not regenerate keys if server sends an extra push request */
       if (!session->key[KS_PRIMARY].crypto_options.key_ctx_bi.initialized &&
 	  !tls_session_update_crypto_params(session, &c->options, &c->c2.frame))
src/openvpn/options.c +32 -0
index 102b4d0..7d042a8 100644
@@ -1643,6 +1643,8 @@ show_settings (const struct options *o)
   SHOW_STR (shared_secret_file);
   SHOW_INT (key_direction);
   SHOW_STR (ciphername);
+  SHOW_BOOL (ncp_enabled);
+  SHOW_STR (ncp_ciphers);
   SHOW_STR (authname);
   SHOW_STR (prng_hash);
   SHOW_INT (prng_nonce_secret_len);

@@ -3434,6 +3436,36 @@ options_string_version (const char* s, struct gc_arena *gc)
 
 #endif /* ENABLE_OCC */
 
+char *
+options_string_extract_option (const char *options_string,const char *opt_name,
+    struct gc_arena *gc)
+{
+  char *ret = NULL;
+  const size_t opt_name_len = strlen(opt_name);
+
+  const char *p = options_string;
+  while (p)
+    {
+      if (0 == strncmp(p, opt_name, opt_name_len) &&
+	  strlen(p) > (opt_name_len+1) && p[opt_name_len] == ' ')
+	{
+	  /* option found, extract value */
+	  const char *start = &p[opt_name_len+1];
+	  const char *end = strchr (p, ',');
+	  size_t val_len = end ? end - start : strlen (start);
+	  ret = gc_malloc (val_len+1, true, gc);
+	  memcpy (ret, start, val_len);
+	  break;
+	}
+      p = strchr (p, ',');
+      if (p)
+	{
+	  p++; /* skip delimiter */
+	}
+    }
+  return ret;
+}
+
 static void
 foreign_option (struct options *o, char *argv[], int len, struct env_set *es)
 {
src/openvpn/options.h +14 -0
index a028556..913bfde 100644
@@ -724,6 +724,20 @@ void options_warning_safe (char *actual, const char *expected, size_t actual_n);
 bool options_cmp_equal (char *actual, const char *expected);
 void options_warning (char *actual, const char *expected);
 
+/**
+ * Given an OpenVPN options string, extract the value of an option.
+ *
+ * @param options_string	Zero-terminated, comma-separated options string
+ * @param opt_name		The name of the option to extract
+ * @param gc			The gc to allocate the return value
+ *
+ * @return gc-allocated value of option with name opt_name if option was found,
+ *         or NULL otherwise.
+ */
+char *options_string_extract_option (const char *options_string,
+    const char *opt_name, struct gc_arena *gc);
+
+
 #endif
 
 void options_postprocess (struct options *options);
src/openvpn/push.c +5 -1
index b7539e6..9953079 100644
@@ -262,7 +262,7 @@ incoming_push_message (struct context *c, const struct buffer *buffer)
 	      !tls_session_update_crypto_params (session, &c->options,
 		  &c->c2.frame))
 	    {
-	      msg (D_TLS_ERRORS, "TLS Error: server generate_key_expansion failed");
+	      msg (D_TLS_ERRORS, "TLS Error: initializing data channel failed");
 	      goto error;
 	    }
 	}

@@ -370,6 +370,10 @@ prepare_push_reply (struct context *c, struct gc_arena *gc,
 	  push_option_fmt(gc, push_list, M_USAGE, "cipher %s", o->ciphername);
 	}
     }
+  else if (o->ncp_enabled)
+    {
+      tls_poor_mans_ncp (o, tls_multi->remote_ciphername);
+    }
 
   /* If server uses --auth-gen-token and we have an auth token
    * to send to the client
src/openvpn/ssl.c +26 -13
index 16b9cb7..4e84a71 100644
@@ -1216,6 +1216,8 @@ tls_multi_free (struct tls_multi *multi, bool clear)
       free (multi->auth_token);
     }
 
+  free (multi->remote_ciphername);
+
   for (i = 0; i < TM_SIZE; ++i)
     tls_session_free (&multi->session[i], false);
 

@@ -1723,8 +1725,8 @@ key_ctx_update_implicit_iv(struct key_ctx *ctx, uint8_t *key, size_t key_len) {
     }
 }
 
-static bool
-item_in_list(const char *item, const char *list)
+bool
+tls_item_in_cipher_list(const char *item, const char *list)
 {
   char *tmp_ciphers = string_alloc (list, NULL);
   char *tmp_ciphers_orig = tmp_ciphers;

@@ -1741,6 +1743,20 @@ item_in_list(const char *item, const char *list)
   return token != NULL;
 }
 
+void
+tls_poor_mans_ncp(struct options *o, const char *remote_ciphername)
+{
+  if (o->ncp_enabled && remote_ciphername &&
+      0 != strcmp(o->ciphername, remote_ciphername))
+    {
+      if (tls_item_in_cipher_list(remote_ciphername, o->ncp_ciphers))
+	{
+	  o->ciphername = string_alloc(remote_ciphername, &o->gc);
+	  msg (D_TLS_DEBUG_LOW, "Using peer cipher '%s'", o->ciphername);
+	}
+    }
+}
+
 bool
 tls_session_update_crypto_params(struct tls_session *session,
     const struct options *options, struct frame *frame)

@@ -1752,7 +1768,7 @@ tls_session_update_crypto_params(struct tls_session *session,
 
   if (!session->opt->server &&
       0 != strcmp(options->ciphername, session->opt->config_ciphername) &&
-      !item_in_list(options->ciphername, options->ncp_ciphers))
+      !tls_item_in_cipher_list(options->ciphername, options->ncp_ciphers))
     {
       msg (D_TLS_ERRORS, "Error: pushed cipher not allowed - %s not in %s or %s",
 	  options->ciphername, session->opt->config_ciphername,

@@ -2152,12 +2168,11 @@ key_method_2_write (struct buffer *buf, struct tls_session *session)
     goto error;
 
   /* Generate tunnel keys if we're a TLS server.
-   * If we're a p2mp server and IV_NCP >= 2 is negotiated, the first key
-   * generation is postponed until after the pull/push, so we can process pushed
-   * cipher directives.
+   * If we're a p2mp server, the first key 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)
 	{

@@ -2312,11 +2327,9 @@ key_method_2_read (struct buffer *buf, struct tls_multi *multi, struct tls_sessi
   if ( multi->peer_info )
       output_peer_info_env (session->opt->es, multi->peer_info);
 
-  if (tls_peer_info_ncp_ver (multi->peer_info) < 2)
-    {
-      /* Peer does not support NCP */
-      session->opt->ncp_enabled = false;
-    }
+  free (multi->remote_ciphername);
+  multi->remote_ciphername =
+      options_string_extract_option (options, "cipher", NULL);
 #endif
 
   if (tls_session_user_pass_enabled(session))
src/openvpn/ssl.h +16 -0
index 777b621..047eb24 100644
@@ -489,6 +489,15 @@ void tls_update_remote_addr (struct tls_multi *multi,
 bool tls_session_update_crypto_params(struct tls_session *session,
     const struct options *options, struct frame *frame);
 
+/**
+ * "Poor man's NCP": Use peer cipher if it is an allowed (NCP) cipher.
+ * Allows non-NCP peers to upgrade their cipher individually.
+ *
+ * Make sure to call tls_session_update_crypto_params() after calling this
+ * function.
+ */
+void tls_poor_mans_ncp(struct options *o, const char *remote_ciphername);
+
 #ifdef MANAGEMENT_DEF_AUTH
 static inline char *
 tls_get_peer_info(const struct tls_multi *multi)

@@ -512,6 +521,13 @@ int tls_peer_info_ncp_ver(const char *peer_info);
  */
 bool tls_check_ncp_cipher_list(const char *list);
 
+/**
+ * Return true iff item is present in the colon-separated zero-terminated
+ * cipher list.
+ */
+bool tls_item_in_cipher_list(const char *item, const char *list);
+
+
 /*
  * inline functions
  */
src/openvpn/ssl_common.h +2 -0
index 28702af..7938f41 100644
@@ -540,6 +540,8 @@ struct tls_multi
   uint32_t peer_id;
   bool use_peer_id;
 
+  char *remote_ciphername;	/**< cipher specified in peer's config file */
+
   char *auth_token;      /**< If server sends a generated auth-token,
                           *   this is the token to use for future
                           *   user/pass authentications in this session.
Signature · 1 line
2.7.4
#7 lore raw

PATCH 2/2 Refactor data channel key generation API

Originally for "poor man's NPC", I introduced a simpler API for generating data channel keys. That refactoring is no longer needed for that patch, but I believe still worth a patch on it's own.

This patch should not change any functionality.

Signed-off-by Steffan Karger <[email protected]...>
---
 src/openvpn/ssl.c | 75 ++++++++++++++++++++++++++-----------------------------
 1 file changed, 35 insertions(+), 40 deletions(-)
src/openvpn/ssl.c +35 -40
index 4e84a71..7eed231 100644
@@ -1757,15 +1757,44 @@ tls_poor_mans_ncp(struct options *o, const char *remote_ciphername)
     }
 }
 
-bool
-tls_session_update_crypto_params(struct tls_session *session,
-    const struct options *options, struct frame *frame)
+/**
+ * Generate data channel keys for the supplied TLS session.
+ *
+ * This erases the source material used to generate the data channel keys, and
+ * can thus be called only once per session.
+ */
+static bool
+tls_session_generate_data_channel_keys(struct tls_session *session)
 {
   bool ret = false;
   struct key_state *ks = &session->key[KS_PRIMARY];	/* primary key */
+  const struct session_id *client_sid = session->opt->server ?
+      &ks->session_id_remote : &session->session_id;
+  const struct session_id *server_sid = !session->opt->server ?
+      &ks->session_id_remote : &session->session_id;
 
   ASSERT (ks->authenticated);
 
+  if (!generate_key_expansion (&ks->crypto_options.key_ctx_bi,
+      &session->opt->key_type, ks->key_src, client_sid, server_sid,
+      session->opt->server))
+    {
+      msg (D_TLS_ERRORS, "TLS Error: generate_key_expansion failed");
+      goto cleanup;
+    }
+  tls_limit_reneg_bytes (session->opt->key_type.cipher,
+			 &session->opt->renegotiate_bytes);
+
+  ret = true;
+cleanup:
+  CLEAR (*ks->key_src);
+  return ret;
+}
+
+bool
+tls_session_update_crypto_params(struct tls_session *session,
+    const struct options *options, struct frame *frame)
+{
   if (!session->opt->server &&
       0 != strcmp(options->ciphername, session->opt->config_ciphername) &&
       !tls_item_in_cipher_list(options->ciphername, options->ncp_ciphers))

@@ -1793,23 +1822,7 @@ tls_session_update_crypto_params(struct tls_session *session,
   frame_init_mssfix(frame, options);
   frame_print (frame, D_MTU_INFO, "Data Channel MTU parms");
 
-  const struct session_id *client_sid = session->opt->server ?
-      &ks->session_id_remote : &session->session_id;
-  const struct session_id *server_sid = !session->opt->server ?
-      &ks->session_id_remote : &session->session_id;
-  if (!generate_key_expansion (&ks->crypto_options.key_ctx_bi,
-      &session->opt->key_type, ks->key_src, client_sid, server_sid,
-      session->opt->server))
-    {
-      msg (D_TLS_ERRORS, "TLS Error: server generate_key_expansion failed");
-      goto cleanup;
-    }
-  tls_limit_reneg_bytes (session->opt->key_type.cipher,
-			 &session->opt->renegotiate_bytes);
-  ret = true;
-cleanup:
-  CLEAR (*ks->key_src);
-  return ret;
+  return tls_session_generate_data_channel_keys (session);
 }
 
 static bool

@@ -2176,21 +2189,12 @@ key_method_2_write (struct buffer *buf, struct tls_session *session)
     {
       if (ks->authenticated)
 	{
-	  if (!generate_key_expansion (&ks->crypto_options.key_ctx_bi,
-				       &session->opt->key_type,
-				       ks->key_src,
-				       &ks->session_id_remote,
-				       &session->session_id,
-				       true))
+	  if (!tls_session_generate_data_channel_keys (session))
 	    {
 	      msg (D_TLS_ERRORS, "TLS Error: server generate_key_expansion failed");
 	      goto error;
 	    }
 	}
-		      
-      CLEAR (*ks->key_src);
-      tls_limit_reneg_bytes (session->opt->key_type.cipher,
-			     &session->opt->renegotiate_bytes);
     }
 
   return true;

@@ -2405,20 +2409,11 @@ key_method_2_read (struct buffer *buf, struct tls_multi *multi, struct tls_sessi
    */
   if (!session->opt->server && (!session->opt->pull || ks->key_id > 0))
     {
-      if (!generate_key_expansion (&ks->crypto_options.key_ctx_bi,
-				   &session->opt->key_type,
-				   ks->key_src,
-				   &session->session_id,
-				   &ks->session_id_remote,
-				   false))
+      if (!tls_session_generate_data_channel_keys (session))
 	{
 	  msg (D_TLS_ERRORS, "TLS Error: client generate_key_expansion failed");
 	  goto error;
 	}
-
-      CLEAR (*ks->key_src);
-      tls_limit_reneg_bytes (session->opt->key_type.cipher,
-			     &session->opt->renegotiate_bytes);
     }
 
   gc_free (&gc);
Signature · 1 line
2.7.4
author
#8 lore raw

[Openvpn-devel] [PATCH applied] Re: Refactor data channel key generation API

ACK. This is slightly different from the refactoring in "v4" (as tls_session_generate_data_channel_keys() is now "static" and no longer needs to be called externally, so the actual change is much smaller). The change to ssl.c is mostly identical, though, so this was now reviewed and tested twice :-) - "just code being moved around". Passes all tests on server and client side.

Your patch has been applied to the master branch.

commit e2ffdb7c83faaee6541c248e8d83eb3dfb5a32f1

Author: Steffan Karger
Date:   Wed Nov 23 21:02:05 2016 +0100
     Refactor data channel key generation API

     Signed-off-by: Steffan Karger <[email protected]...>
     Acked-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]/msg13216.html
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

#9 lore raw

PATCH v6 1/2 Poor man's NCP for non-NCP peers

In reply to Steffan Karger

Allows non-NCP peers (<= 2.3, or 2.4+ with --ncp-disable) to specify a --cipher that is different from the one in our config, as long as the new cipher value is allowed (i.e. in --ncp-ciphers at our side).

This works both client-to-server and server-to-client. I.e. a 2.4 client with "cipher BF-CBC" and "ncp-ciphers AES-256-GCM:AES-256-CBC" can connect to both a 2.3 server with "cipher BF-CBC" as well as a server with "cipher AES-256-CBC" in its config. The other way around, a 2.3 client with either "cipher BF-CBC" or "cipher AES-256-CBC" can connect to a 2.4 server with e.g. "cipher BF-CBC" and "ncp-ciphers AES-256-GCM:AES-256-CBC" in its config.

This patch was inspired by Gert's "Poor man's NCP for 2.3 clients" patch, but takes a different approach to avoid the need for server-side scripts or client-side 'setenv UV_*' tricks.

Signed-off-by Steffan Karger <[email protected]...>

---
v2: remove unused argument from options_string_extract_option()
v3: don't do poor-man's NCP if NCP is disabled
v4: use strncmp instead of strstr to match prefix in
options_string_extract_option, add man page text and make clear that this feature works both client-to-server and server-to-client.
v5: move poor-mans-ncp logic from key_method_2_read() to the place
where we do regular NCP too. This makes that we no longer need access to the struct context in key_method_2_read, and is cleaner in general, and makes the patch a bit smaller.
v6: don't break p2mp-to-no-pull-client setups

 doc/openvpn.8            | 14 ++++++++++++--
 src/openvpn/init.c       | 10 ++++++++--
 src/openvpn/options.c    | 32 ++++++++++++++++++++++++++++++++
 src/openvpn/options.h    | 14 ++++++++++++++
 src/openvpn/push.c       |  6 +++++-
 src/openvpn/ssl.c        | 36 ++++++++++++++++++++++++++++++------
 src/openvpn/ssl.h        | 16 ++++++++++++++++
 src/openvpn/ssl_common.h |  2 ++
 8 files changed, 119 insertions(+), 11 deletions(-)
doc/openvpn.8 +12 -2
index 171bd72..4356597 100644
@@ -4147,9 +4147,9 @@ to disable encryption.
 As of OpenVPN 2.4, cipher negotiation (NCP) can override the cipher specified by
 .B \-\-cipher\fR.
 See
-.B \-\-ncp-ciphers
+.B \-\-ncp\-ciphers
 and
-.B \-\-ncp-disable
+.B \-\-ncp\-disable
 for more on NCP.
 
 .\"*********************************************************

@@ -4177,6 +4177,16 @@ If both peers support and do not disable NCP, the negotiated cipher will
 override the cipher specified by
 .B \-\-cipher\fR.
 
+Additionally, to allow for more smooth transition, if NCP is enabled, OpenVPN
+will inherit the cipher of the peer if that cipher is different from the local
+.B \-\-cipher
+setting, but the peer cipher is one of the ciphers specified in
+.B \-\-ncp\-ciphers\fR.
+E.g. a non-NCP client (<=2.3, or with \-\-ncp\-disabled set) connecting to a
+NCP server (2.4+) with "\-\-cipher BF-CBC" and "\-\-ncp-ciphers
+AES-256-GCM:AES-256-CBC" set can either specify "\-\-cipher BF-CBC" or
+"\-\-cipher AES-256-CBC" and both will work.
+
 .\"*********************************************************
 .TP
 .B \-\-ncp\-disable
src/openvpn/init.c +8 -2
index 2ed633c..03302a6 100644
@@ -1925,8 +1925,14 @@ do_deferred_options (struct context *c, const unsigned int found)
     {
       struct tls_session *session = &c->c2.tls_multi->session[TM_ACTIVE];
       if (found & OPT_P_NCP)
-	msg (D_PUSH, "OPTIONS IMPORT: data channel crypto options modified");
-      /* Do not regenerate keys if server sends an extra push request */
+	{
+	  msg (D_PUSH, "OPTIONS IMPORT: data channel crypto options modified");
+	}
+      else if (c->options.ncp_enabled)
+	{
+	  tls_poor_mans_ncp(&c->options, c->c2.tls_multi->remote_ciphername);
+	}
+      /* Do not regenerate keys if server sends an extra push reply */
       if (!session->key[KS_PRIMARY].crypto_options.key_ctx_bi.initialized &&
 	  !tls_session_update_crypto_params(session, &c->options, &c->c2.frame))
 	{
src/openvpn/options.c +32 -0
index 102b4d0..7d042a8 100644
@@ -1643,6 +1643,8 @@ show_settings (const struct options *o)
   SHOW_STR (shared_secret_file);
   SHOW_INT (key_direction);
   SHOW_STR (ciphername);
+  SHOW_BOOL (ncp_enabled);
+  SHOW_STR (ncp_ciphers);
   SHOW_STR (authname);
   SHOW_STR (prng_hash);
   SHOW_INT (prng_nonce_secret_len);

@@ -3434,6 +3436,36 @@ options_string_version (const char* s, struct gc_arena *gc)
 
 #endif /* ENABLE_OCC */
 
+char *
+options_string_extract_option (const char *options_string,const char *opt_name,
+    struct gc_arena *gc)
+{
+  char *ret = NULL;
+  const size_t opt_name_len = strlen(opt_name);
+
+  const char *p = options_string;
+  while (p)
+    {
+      if (0 == strncmp(p, opt_name, opt_name_len) &&
+	  strlen(p) > (opt_name_len+1) && p[opt_name_len] == ' ')
+	{
+	  /* option found, extract value */
+	  const char *start = &p[opt_name_len+1];
+	  const char *end = strchr (p, ',');
+	  size_t val_len = end ? end - start : strlen (start);
+	  ret = gc_malloc (val_len+1, true, gc);
+	  memcpy (ret, start, val_len);
+	  break;
+	}
+      p = strchr (p, ',');
+      if (p)
+	{
+	  p++; /* skip delimiter */
+	}
+    }
+  return ret;
+}
+
 static void
 foreign_option (struct options *o, char *argv[], int len, struct env_set *es)
 {
src/openvpn/options.h +14 -0
index a028556..913bfde 100644
@@ -724,6 +724,20 @@ void options_warning_safe (char *actual, const char *expected, size_t actual_n);
 bool options_cmp_equal (char *actual, const char *expected);
 void options_warning (char *actual, const char *expected);
 
+/**
+ * Given an OpenVPN options string, extract the value of an option.
+ *
+ * @param options_string	Zero-terminated, comma-separated options string
+ * @param opt_name		The name of the option to extract
+ * @param gc			The gc to allocate the return value
+ *
+ * @return gc-allocated value of option with name opt_name if option was found,
+ *         or NULL otherwise.
+ */
+char *options_string_extract_option (const char *options_string,
+    const char *opt_name, struct gc_arena *gc);
+
+
 #endif
 
 void options_postprocess (struct options *options);
src/openvpn/push.c +5 -1
index b7539e6..9953079 100644
@@ -262,7 +262,7 @@ incoming_push_message (struct context *c, const struct buffer *buffer)
 	      !tls_session_update_crypto_params (session, &c->options,
 		  &c->c2.frame))
 	    {
-	      msg (D_TLS_ERRORS, "TLS Error: server generate_key_expansion failed");
+	      msg (D_TLS_ERRORS, "TLS Error: initializing data channel failed");
 	      goto error;
 	    }
 	}

@@ -370,6 +370,10 @@ prepare_push_reply (struct context *c, struct gc_arena *gc,
 	  push_option_fmt(gc, push_list, M_USAGE, "cipher %s", o->ciphername);
 	}
     }
+  else if (o->ncp_enabled)
+    {
+      tls_poor_mans_ncp (o, tls_multi->remote_ciphername);
+    }
 
   /* If server uses --auth-gen-token and we have an auth token
    * to send to the client
src/openvpn/ssl.c +30 -6
index 16b9cb7..ff7e837 100644
@@ -1216,6 +1216,8 @@ tls_multi_free (struct tls_multi *multi, bool clear)
       free (multi->auth_token);
     }
 
+  free (multi->remote_ciphername);
+
   for (i = 0; i < TM_SIZE; ++i)
     tls_session_free (&multi->session[i], false);
 

@@ -1723,8 +1725,8 @@ key_ctx_update_implicit_iv(struct key_ctx *ctx, uint8_t *key, size_t key_len) {
     }
 }
 
-static bool
-item_in_list(const char *item, const char *list)
+bool
+tls_item_in_cipher_list(const char *item, const char *list)
 {
   char *tmp_ciphers = string_alloc (list, NULL);
   char *tmp_ciphers_orig = tmp_ciphers;

@@ -1741,6 +1743,20 @@ item_in_list(const char *item, const char *list)
   return token != NULL;
 }
 
+void
+tls_poor_mans_ncp(struct options *o, const char *remote_ciphername)
+{
+  if (o->ncp_enabled && remote_ciphername &&
+      0 != strcmp(o->ciphername, remote_ciphername))
+    {
+      if (tls_item_in_cipher_list(remote_ciphername, o->ncp_ciphers))
+	{
+	  o->ciphername = string_alloc(remote_ciphername, &o->gc);
+	  msg (D_TLS_DEBUG_LOW, "Using peer cipher '%s'", o->ciphername);
+	}
+    }
+}
+
 bool
 tls_session_update_crypto_params(struct tls_session *session,
     const struct options *options, struct frame *frame)

@@ -1752,7 +1768,7 @@ tls_session_update_crypto_params(struct tls_session *session,
 
   if (!session->opt->server &&
       0 != strcmp(options->ciphername, session->opt->config_ciphername) &&
-      !item_in_list(options->ciphername, options->ncp_ciphers))
+      !tls_item_in_cipher_list(options->ciphername, options->ncp_ciphers))
     {
       msg (D_TLS_ERRORS, "Error: pushed cipher not allowed - %s not in %s or %s",
 	  options->ciphername, session->opt->config_ciphername,

@@ -2311,13 +2327,21 @@ key_method_2_read (struct buffer *buf, struct tls_multi *multi, struct tls_sessi
   multi->peer_info = read_string_alloc (buf);
   if ( multi->peer_info )
       output_peer_info_env (session->opt->es, multi->peer_info);
+#endif
+
+  free (multi->remote_ciphername);
+  multi->remote_ciphername =
+      options_string_extract_option (options, "cipher", NULL);
 
   if (tls_peer_info_ncp_ver (multi->peer_info) < 2)
     {
-      /* Peer does not support NCP */
-      session->opt->ncp_enabled = false;
+      /* 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 (0 == strcmp(multi->remote_ciphername, multi->opt.config_ciphername))
+       {
+         session->opt->ncp_enabled = false;
+       }
     }
-#endif
 
   if (tls_session_user_pass_enabled(session))
     {
src/openvpn/ssl.h +16 -0
index 777b621..047eb24 100644
@@ -489,6 +489,15 @@ void tls_update_remote_addr (struct tls_multi *multi,
 bool tls_session_update_crypto_params(struct tls_session *session,
     const struct options *options, struct frame *frame);
 
+/**
+ * "Poor man's NCP": Use peer cipher if it is an allowed (NCP) cipher.
+ * Allows non-NCP peers to upgrade their cipher individually.
+ *
+ * Make sure to call tls_session_update_crypto_params() after calling this
+ * function.
+ */
+void tls_poor_mans_ncp(struct options *o, const char *remote_ciphername);
+
 #ifdef MANAGEMENT_DEF_AUTH
 static inline char *
 tls_get_peer_info(const struct tls_multi *multi)

@@ -512,6 +521,13 @@ int tls_peer_info_ncp_ver(const char *peer_info);
  */
 bool tls_check_ncp_cipher_list(const char *list);
 
+/**
+ * Return true iff item is present in the colon-separated zero-terminated
+ * cipher list.
+ */
+bool tls_item_in_cipher_list(const char *item, const char *list);
+
+
 /*
  * inline functions
  */
src/openvpn/ssl_common.h +2 -0
index 28702af..7938f41 100644
@@ -540,6 +540,8 @@ struct tls_multi
   uint32_t peer_id;
   bool use_peer_id;
 
+  char *remote_ciphername;	/**< cipher specified in peer's config file */
+
   char *auth_token;      /**< If server sends a generated auth-token,
                           *   this is the token to use for future
                           *   user/pass authentications in this session.
Signature · 1 line
2.7.4
author
#10 lore raw

[Openvpn-devel] [PATCH applied] Re: Poor man's NCP for non-NCP peers

ACK. We made it :-)

I've made three changes (as discussed on IRC) to avoid another round for "minor nuisances"

- moved prototype in options.h outside #ifdef ENABLE_OCC (warning if --disable-small)

- undo the moving of #if P2MP_SERVER in key_method_2_read() (would break --disable-server)

- add an extra "if (multi->remote_ciphername == NULL || ...) clause to the reasons for setting opt->ncp_enabled = false (remote_ciphername will be NULL if talking to a peer that has been

    built without ENABLE_OCC aka --enable-small)

I've tested this on the "t_server" testbed (2.2, 2.3, master clients, plus one extra with --enable-small), client-with-patch against an older server (falling back to BF-CBC, complaining :-) ) and of course full t_client tests (though nothing in the change suspects a possible client failure in that test set).

Your patch has been applied to the master branch. Thanks!

commit 6e5ad2fa0b1e7ca2e05e0a38f47ed5561cda63b0

Author: Steffan Karger
Date:   Wed Nov 23 22:21:44 2016 +0100
     Poor man's NCP for non-NCP peers

     Signed-off-by: Steffan Karger <[email protected]...>
     Acked-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]/msg13218.html
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

author
#11 lore raw

Re: [Openvpn-devel] [PATCH applied] Re: Poor man's NCP for non-NCP peers

Hi,

Quoted 1 line · Gert Doering

ACK. We made it :-)

I *did* find a way to break it, I think - haven't tested, need to sleep - just copying what I wrote to IRC just now since everyone is asleep already...

23:30 <@cron2> syzzer: I *do* have a potential way to use this to break things 23:30 <@cron2> server with NCP enabled, --cipher foo
23:30 <@cron2> client with NCP disabled, --cipher bar
23:30 <@cron2> server will now use "cipher bar", while client will do "cipher

               foo"...

23:31 <@cron2> so I think we should do a "v6a amendment" which disables this on
the client if --ncp-disable is set (so 2.4 to 2.4 will either do *real* NCP, or *no* NCP, but no half-assed two-way poorman)
23:31 <@cron2> this makes testing more annoying because you can't talk to a 2.4
server to test the client side :-) - but it's *meant* to be a 2.3<->2.4 migration feature

gert

Signature · 4 lines
USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...
#12 lore raw

Re: [Openvpn-devel] [PATCH applied] Re: Poor man's NCP for non-NCP peers

Hi,

Quoted 11 lines · Gert Doering
Quoted 1 line · Gert Doering

ACK. We made it :-)

I *did* find a way to break it, I think - haven't tested, need to sleep - just copying what I wrote to IRC just now since everyone is asleep already...

23:30 <@cron2> syzzer: I *do* have a potential way to use this to break things 23:30 <@cron2> server with NCP enabled, --cipher foo
23:30 <@cron2> client with NCP disabled, --cipher bar
23:30 <@cron2> server will now use "cipher bar", while client will do "cipher

Quoted 7 lines
               foo"...

23:31 <@cron2> so I think we should do a "v6a amendment" which disables this on
the client if --ncp-disable is set (so 2.4 to 2.4 will either do *real* NCP, or *no* NCP, but no half-assed two-way poorman)
23:31 <@cron2> this makes testing more annoying because you can't talk to a 2.4
server to test the client side :-) - but it's *meant* to be a 2.3<->2.4 migration feature

This will end up with the server and client using cipher bar (if the server has --ncp-ciphers <somehting>:bar). The client-side poor-man's NCP is already guarded by "if (c->options.ncp_enabled)" (in do_deferred_options()).

-Steffan

author
#13 lore raw

Re: [Openvpn-devel] [PATCH applied] Re: Poor man's NCP for non-NCP peers

Hi,

Quoted 4 lines · Steffan Karger

This will end up with the server and client using cipher bar (if the server has --ncp-ciphers <somehting>:bar). The client-side poor-man's NCP is already guarded by "if (c->options.ncp_enabled)" (in do_deferred_options()).

Good point. Seems I was not very much awake anymore :-)

  /* process (potentially pushed) crypto options */
  if (c->options.pull)
    {
      struct tls_session *session = &c->c2.tls_multi->session[TM_ACTIVE];
      if (found & OPT_P_NCP)
        {
          msg (D_PUSH, "OPTIONS IMPORT: data channel crypto options modified");
        }
      else if (c->options.ncp_enabled)
        {
          tls_poor_mans_ncp(&c->options, c->c2.tls_multi->remote_ciphername);
        }

... all fine, then.

thanks,

gert

Signature · 4 lines
USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...