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

2016-10-10 Thread 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
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! http://sdm.link/slashdot___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

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

2016-10-01 Thread 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

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

2016-09-24 Thread 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.

Signed-off-by: Lev Stipakov 
---
 src/openvpn/push.c | 83 +++---
 1 file changed, 22 insertions(+), 61 deletions(-)

diff --git a/src/openvpn/push.c b/src/openvpn/push.c
index a1b999e..5220bf0 100644
--- a/src/openvpn/push.c
+++ b/src/openvpn/push.c
@@ -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 oThe 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_multiTLS 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", );
-  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", );
+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, >gc);
+ char *push_cipher = string_alloc (o->ncp_ciphers, >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 (, ",push-continuation 1");
 
+  add_peer_specific_opts (c->c2.tls_multi, >options, );
+
   if (BLEN () > sizeof(cmd)-1)
 {
   const bool status = send_control_channel_string (c, BSTR (), 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, >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(>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;
-- 
1.9.1