Fix duplicated PUSH_REPLY options

PATCH In review Lev Stipakov 3 participants 5 messages Sep 24, 2016 – Oct 10, 2016

Share & tools

Lev Stipakov

author

2016-09-24 10:23 UTC #0 lore raw

PATCH 1/1 Fix duplicated PUSH_REPLY options

Starting from https://github.com/OpenVPN/openvpn/commit/3a5a46cf2b7f6a8b8520c2513a8054deb48bfcbe,
we add peer-id and cipher values to context->options->push_list instead of adding those directly to buf (as done for client-specific values, like ifconfig). Since push_list is per child context, when options are added and context is reused - we got duplicates.

Fixed by adding options to buffer, as it was done previously.

Signed-off-by Lev Stipakov <[email protected]...>

---
 src/openvpn/push.c | 83 +++++++++++++++---------------------------------------
 1 file changed, 22 insertions(+), 61 deletions(-)

src/openvpn/push.c +22 ~~-61~~

index a1b999e..5220bf0 100644
@@ -40,30 +40,6 @@
 
 #if P2MP
 
-/**
- * Add an option to the push list by providing a format string.
- *
- * The string added to the push options is allocated in o->gc, so the caller
- * does not have to preserve anything.
- *
- * @param o		The current connection's options
- * @param msglevel	The message level to use when printing errors
- * @param fmt		Format string for the option
- * @param ...		Format string arguments
- *
- * @return true on success, false on failure.
- */
-static bool push_option_fmt(struct options *o, int msglevel,
-    const char *fmt, ...)
-#ifdef __GNUC__
-#if __USE_MINGW_ANSI_STDIO
-    __attribute__ ((format (gnu_printf, 3, 4)))
-#else
-    __attribute__ ((format (__printf__, 3, 4)))
-#endif
-#endif
-    ;
-
 /*
  * Auth username/password
  *

@@ -293,31 +269,31 @@ send_push_request (struct context *c)
 #if P2MP_SERVER
 
 /**
- * Prepare push options, based on local options and available peer info.
+ * Add peer specific options, based on local options and peer info.
  *
- * @param options	Connection options
  * @param tls_multi	TLS state structure for the current tunnel
+ * @param options	Connection options
+ * @param buf		buffer to where options are added
  *
- * @return true on success, false on failure.
  */
-static bool
-prepare_push_reply (struct options *o, struct tls_multi *tls_multi)
+static void
+add_peer_specific_opts (struct tls_multi *tls_multi, struct options *o, struct buffer *buf)
 {
-  const char *optstr = NULL;
   const char * const peer_info = tls_multi->peer_info;
-
+  const char *optstr = NULL;
+  
   /* Send peer-id if client supports it */
-  optstr = peer_info ? strstr(peer_info, "IV_PROTO=") : NULL;
+  optstr = peer_info ? strstr (peer_info, "IV_PROTO=") : NULL;
   if (optstr)
-    {
-      int proto = 0;
-      int r = sscanf(optstr, "IV_PROTO=%d", &proto);
-      if ((r == 1) && (proto >= 2))
-	{
-	  push_option_fmt(o, M_USAGE, "peer-id %d", tls_multi->peer_id);
-	}
-    }
-
+  {
+    int proto = 0;
+    int r = sscanf (optstr, "IV_PROTO=%d", &proto);
+    if ((r == 1) && (proto >= 2))
+      {
+	buf_printf (buf, ",peer-id %d", tls_multi->peer_id);
+      }
+  }
+	
   /* Push cipher if client supports Negotiable Crypto Parameters */
   if (tls_peer_info_ncp_ver (peer_info) >= 2 && o->ncp_enabled)
     {

@@ -335,12 +311,11 @@ prepare_push_reply (struct options *o, struct tls_multi *tls_multi)
 	{
 	  /* 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);
+	  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);
+	  buf_printf (buf, ",cipher %s", o->ciphername);
 	}
     }
-  return true;
 }
 
 static bool

@@ -414,6 +389,8 @@ send_push_reply (struct context *c)
   if (multi_push)
     buf_printf (&buf, ",push-continuation 1");
 
+  add_peer_specific_opts (c->c2.tls_multi, &c->options, &buf);
+
   if (BLEN (&buf) > sizeof(cmd)-1)
     {
       const bool status = send_control_channel_string (c, BSTR (&buf), D_PUSH);

@@ -501,21 +478,6 @@ push_options (struct options *o, char **p, int msglevel, struct gc_arena *gc)
   push_option (o, opt, msglevel);
 }
 
-static bool push_option_fmt(struct options *o, int msglevel,
-    const char *format, ...)
-{
-  va_list arglist;
-  char tmp[256] = {0};
-  int len = -1;
-  va_start (arglist, format);
-  len = vsnprintf (tmp, sizeof(tmp), format, arglist);
-  va_end (arglist);
-  if (len > sizeof(tmp)-1)
-    return false;
-  push_option (o, string_alloc (tmp, &o->gc), msglevel);
-  return true;
-}
-
 void
 push_reset (struct options *o)
 {

@@ -580,8 +542,7 @@ process_incoming_push_request (struct context *c)
 	}
       else
 	{
-	  if (prepare_push_reply(&c->options, c->c2.tls_multi) &&
-	      send_push_reply (c))
+	  if (send_push_reply (c))
 	    {
 	      ret = PUSH_MSG_REQUEST;
 	      c->c2.sent_push_reply_expiry = now + 30;

Signature · 1 line

1.9.1

Steffan Karger

2016-09-29 17:49 UTC #1 lore raw

Hi,

Quoted 6 lines · Lev Stipakov

Starting from https://github.com/OpenVPN/openvpn/commit/3a5a46cf2b7f6a8b8520c2513a8054deb48bfcbe,
we add peer-id and cipher values to context->options->push_list instead of adding those directly to buf (as done for client-specific values, like ifconfig). Since push_list is per child context, when options are added and context is reused - we got duplicates.
Fixed by adding options to buffer, as it was done previously.

NAK. This reintroduces another issue, where the added options might not fit into buf, because we can't reserve space for variable-sized options (peer-id would be possible, by cipher would already be trickier).

This is a bug though (sorry!), so attached a different proposal to fix this. I didn't test this yet (need to leave now), but lev just announced on IRC that he was willing to test it.

-Steffan

From 957c54a7f4cefdc05e5e876195ef31a52c00f2b3 Mon Sep 17 00:00:00 2001
From: Steffan Karger <[email protected]>
Date: Thu, 29 Sep 2016 19:48:29 +0200
Subject: [PATCH] Fix duplicate PUSH_REPLY options

As reported by Lev Stipakov, starting from 3a5a46cf we add peer-id and cipher values to context->options->push_list instead of adding those directly to buf. Since push_list is preserved over sigusr1 restarts, we add duplicate values for peer-id and cipher.

Fixed by removing the previous values from the list before adding new ones.

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

---
 src/openvpn/errlevel.h | 1 +
 src/openvpn/options.c  | 1 +
 src/openvpn/push.c     | 6 ++++--
 3 files changed, 6 insertions(+), 2 deletions(-)

src/openvpn/errlevel.h +1 -0

index da600ab..ae1f8f4 100644
@@ -147,6 +147,7 @@
 #define D_PID_DEBUG          LOGLEV(7, 70, M_DEBUG)  /* show packet-id debugging info */
 #define D_PF_DROPPED_BCAST   LOGLEV(7, 71, M_DEBUG)  /* packet filter dropped a broadcast packet */
 #define D_PF_DEBUG           LOGLEV(7, 72, M_DEBUG)  /* packet filter debugging, must also define PF_DEBUG in pf.h */
+#define D_PUSH_DEBUG         LOGLEV(7, 73, M_DEBUG)  /* show push/pull debugging info */
 
 #define D_HANDSHAKE_VERBOSE  LOGLEV(8, 70, M_DEBUG)  /* show detailed description of each handshake */
 #define D_TLS_DEBUG_MED      LOGLEV(8, 70, M_DEBUG)  /* limited info from tls_session routines */

src/openvpn/options.c +1 -0

index 53b4617..ccb6b28 100644
@@ -5781,6 +5781,7 @@ add_option (struct options *options,
   else if (streq (p[0], "push-remove") && p[1] && !p[2])
     {
       VERIFY_PERMISSION (OPT_P_INSTANCE);
+      msg (D_PUSH, "PUSH_REMOVE '%s'", p[1]);
       push_remove_option (options,p[1]);
     }
   else if (streq (p[0], "ifconfig-pool") && p[1] && p[2] && !p[4])

src/openvpn/push.c +4 -2

index a1b999e..4d1dd34 100644
@@ -314,6 +314,7 @@ prepare_push_reply (struct options *o, struct tls_multi *tls_multi)
       int r = sscanf(optstr, "IV_PROTO=%d", &proto);
       if ((r == 1) && (proto >= 2))
 	{
+	  push_remove_option(o, "peer-id");
 	  push_option_fmt(o, M_USAGE, "peer-id %d", tls_multi->peer_id);
 	}
     }

@@ -337,6 +338,7 @@ prepare_push_reply (struct options *o, struct tls_multi *tls_multi)
 	   * TODO: actual negotiation, instead of server dictatorship. */
 	  char *push_cipher = string_alloc(o->ncp_ciphers, &o->gc);
 	  o->ciphername = strtok (push_cipher, ":");
+	  push_remove_option(o, "cipher");
 	  push_option_fmt(o, M_USAGE, "cipher %s", o->ciphername);
 	}
     }

@@ -525,7 +527,7 @@ push_reset (struct options *o)
 void
 push_remove_option (struct options *o, const char *p)
 {
-  msg( D_PUSH, "PUSH_REMOVE '%s'", p );
+  msg (D_PUSH_DEBUG, "PUSH_REMOVE searching for: '%s'", p);
 
   /* ifconfig-ipv6 is special, as not part of the push list */
   if ( streq( p, "ifconfig-ipv6" ))

@@ -544,7 +546,7 @@ push_remove_option (struct options *o, const char *p)
 	  if ( e->enable &&
                strncmp( e->option, p, strlen(p) ) == 0 )
 	    {
-	      msg (D_PUSH, "PUSH_REMOVE removing: '%s'", e->option);
+	      msg (D_PUSH_DEBUG, "PUSH_REMOVE removing: '%s'", e->option);
 	      e->enable = false;
 	    }

Signature · 1 line

2.7.4

Lev Stipakov

author

2016-10-01 13:30 UTC #2 lore raw

Hi,

ACK from me - tested that peer-id and cipher are not duplicated in PUSH_REPLY when client reconnects and push-continuation works.

-Lev

2016-09-29 20:49 GMT+03:00 Steffan Karger <[email protected]...>:

Quoted 24 lines

Hi,
Quoted 1 line · Lev Stipakov
Starting from https://github.com/OpenVPN/openvpn/commit/
3a5a46cf2b7f6a8b8520c2513a8054deb48bfcbe,
Quoted 1 line
we add peer-id and cipher values to context->options->push_list instead
of adding those directly
Quoted 1 line
to buf (as done for client-specific values, like ifconfig). Since
push_list is per child context,
Quoted 3 lines
when options are added and context is reused - we got duplicates.
Fixed by adding options to buffer, as it was done previously.
NAK. This reintroduces another issue, where the added options might not fit into buf, because we can't reserve space for variable-sized options (peer-id would be possible, by cipher would already be trickier).
This is a bug though (sorry!), so attached a different proposal to fix this. I didn't test this yet (need to leave now), but lev just announced on IRC that he was willing to test it.
-Steffan

Signature · 1 line

-Lev

Lev Stipakov

author

2016-10-01 14:05 UTC #3 lore raw

PATCH 1/1 Fix duplicated PUSH_REPLY options

In reply to Steffan Karger

Hi,

Attempt #2 - now with attached patch.

ACK from me - tested that peer-id and cipher are not duplicated in PUSH_REPLY when client reconnects and push-continuation works.

-Lev

---------- Forwarded message ----------
From: Steffan Karger <[email protected]...>
Date: 2016-09-29 20:49 GMT+03:00
Subject: Re: [Openvpn-devel] [PATCH] Fix duplicated PUSH_REPLY options
To: Lev Stipakov <[email protected]...>

Hi,

Quoted 1 line · Lev Stipakov

Starting from https://github.com/OpenVPN/openvpn/commit/3a5a46cf2b7f6a8b85

20c2513a8054deb48bfcbe,

Quoted 1 line

we add peer-id and cipher values to context->options->push_list instead

of adding those directly

Quoted 1 line

to buf (as done for client-specific values, like ifconfig). Since

push_list is per child context,

Quoted 3 lines

when options are added and context is reused - we got duplicates.
Fixed by adding options to buffer, as it was done previously.

This is a bug though (sorry!), so attached a different proposal to fix this. I didn't test this yet (need to leave now), but lev just announced on IRC that he was willing to test it.

-Steffan

Signature · 1 line

-Lev

Fixed by removing the previous values from the list before adding new ones.

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

---
 src/openvpn/errlevel.h | 1 +
 src/openvpn/options.c  | 1 +
 src/openvpn/push.c     | 6 ++++--
 3 files changed, 6 insertions(+), 2 deletions(-)

src/openvpn/errlevel.h +1 -0

index da600ab..ae1f8f4 100644
@@ -147,6 +147,7 @@
 #define D_PID_DEBUG          LOGLEV(7, 70, M_DEBUG)  /* show packet-id debugging info */
 #define D_PF_DROPPED_BCAST   LOGLEV(7, 71, M_DEBUG)  /* packet filter dropped a broadcast packet */
 #define D_PF_DEBUG           LOGLEV(7, 72, M_DEBUG)  /* packet filter debugging, must also define PF_DEBUG in pf.h */
+#define D_PUSH_DEBUG         LOGLEV(7, 73, M_DEBUG)  /* show push/pull debugging info */
 
 #define D_HANDSHAKE_VERBOSE  LOGLEV(8, 70, M_DEBUG)  /* show detailed description of each handshake */
 #define D_TLS_DEBUG_MED      LOGLEV(8, 70, M_DEBUG)  /* limited info from tls_session routines */

src/openvpn/options.c +1 -0

index 53b4617..ccb6b28 100644
@@ -5781,6 +5781,7 @@ add_option (struct options *options,
   else if (streq (p[0], "push-remove") && p[1] && !p[2])
     {
       VERIFY_PERMISSION (OPT_P_INSTANCE);
+      msg (D_PUSH, "PUSH_REMOVE '%s'", p[1]);
       push_remove_option (options,p[1]);
     }
   else if (streq (p[0], "ifconfig-pool") && p[1] && p[2] && !p[4])

src/openvpn/push.c +4 -2

index a1b999e..4d1dd34 100644
@@ -314,6 +314,7 @@ prepare_push_reply (struct options *o, struct tls_multi *tls_multi)
       int r = sscanf(optstr, "IV_PROTO=%d", &proto);
       if ((r == 1) && (proto >= 2))
 	{
+	  push_remove_option(o, "peer-id");
 	  push_option_fmt(o, M_USAGE, "peer-id %d", tls_multi->peer_id);
 	}
     }

@@ -337,6 +338,7 @@ prepare_push_reply (struct options *o, struct tls_multi *tls_multi)
 	   * TODO: actual negotiation, instead of server dictatorship. */
 	  char *push_cipher = string_alloc(o->ncp_ciphers, &o->gc);
 	  o->ciphername = strtok (push_cipher, ":");
+	  push_remove_option(o, "cipher");
 	  push_option_fmt(o, M_USAGE, "cipher %s", o->ciphername);
 	}
     }

@@ -525,7 +527,7 @@ push_reset (struct options *o)
 void
 push_remove_option (struct options *o, const char *p)
 {
-  msg( D_PUSH, "PUSH_REMOVE '%s'", p );
+  msg (D_PUSH_DEBUG, "PUSH_REMOVE searching for: '%s'", p);
 
   /* ifconfig-ipv6 is special, as not part of the push list */
   if ( streq( p, "ifconfig-ipv6" ))

@@ -544,7 +546,7 @@ push_remove_option (struct options *o, const char *p)
 	  if ( e->enable &&
                strncmp( e->option, p, strlen(p) ) == 0 )
 	    {
-	      msg (D_PUSH, "PUSH_REMOVE removing: '%s'", e->option);
+	      msg (D_PUSH_DEBUG, "PUSH_REMOVE removing: '%s'", e->option);
 	      e->enable = false;
 	    }

Signature · 1 line

2.7.4

Gert Doering

2016-10-10 13:30 UTC #4 lore raw

Re: [Openvpn-devel] [PATCH applied] Fix duplicate PUSH_REPLY options

In reply to Steffan Karger

Thanks.

Your patch has been applied to the master branch

commit 974ec19daa6c9d4e954912b3743c7101637f1d33

Author: Steffan Karger
Date:   Thu Sep 29 19:48:29 2016 +0200

     Fix duplicate PUSH_REPLY options

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

--
kind regards,

Gert Doering