[Openvpn-devel] Fwd: [PATCH] Fix duplicated PUSH_REPLY options

[Lev Stipakov]

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 
Date: 2016-09-29 20:49 GMT+03:00
Subject: Re: [Openvpn-devel] [PATCH] Fix duplicated PUSH_REPLY options
To: Lev Stipakov 


Hi,

On 24 September 2016 at 12:23, Lev Stipakov  wrote:
> Starting from https://github.com/OpenVPN/openvpn/commit/3a5a46cf2b7f6a8b85
20c2513a8054deb48bfcbe,
> 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



-- 
-Lev
From 957c54a7f4cefdc05e5e876195ef31a52c00f2b3 Mon Sep 17 00:00:00 2001
From: Steffan Karger 
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 
---
 src/openvpn/errlevel.h | 1 +
 src/openvpn/options.c  | 1 +
 src/openvpn/push.c | 6 --
 3 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/src/openvpn/errlevel.h b/src/openvpn/errlevel.h
index da600ab..ae1f8f4 100644
--- a/src/openvpn/errlevel.h
+++ b/src/openvpn/errlevel.h
@@ -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 */
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 53b4617..ccb6b28 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -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])
diff --git a/src/openvpn/push.c b/src/openvpn/push.c
index a1b999e..4d1dd34 100644
--- a/src/openvpn/push.c
+++ b/src/openvpn/push.c
@@ -314,6 +314,7 @@ prepare_push_reply (struct options *o, struct tls_multi *tls_multi)
   int r = sscanf(optstr, "IV_PROTO=%d", );
   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, >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;
 	}
 
-- 
2.7.4

--
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! 

Re: [Openvpn-devel] [PATCH] Fix duplicated PUSH_REPLY options

[Lev Stipakov]

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 :

> Hi,
>
> On 24 September 2016 at 12:23, Lev Stipakov  wrote:
> > 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
>



-- 
-Lev
--
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