Hi,
On 24 September 2016 at 12:23, Lev Stipakov <lstipa...@gmail.com> 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 <stef...@karger.me>
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 <stef...@karger.me>
---
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", &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;
}
--
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
