[Openvpn-devel,v2] Refactor NCP-negotiable options handling
Commit Message
From: Lev Stipakov <lev@openvpn.net>
NCP negotiation can alter options. On reconnect
client sends possibly altered options while server
expects original values. This leads to warnings
in log and, if server uses --opt-verify, breaks
reconnect.
Fix by decouple setting/unsetting NCP options from
the state of TLS context. At startup (and once per sighup)
we load original values to c->c1, which persists over
sigusr1 (restart). When tearing tunnel down we restore
(possibly altered) options back to original values.
Trac: #1105
Signed-off-by: Lev Stipakov <lev@openvpn.net>
---
v2: refined commit message
src/openvpn/init.c | 40 +++++++++++++++++++++++++++++++---------
1 file changed, 31 insertions(+), 9 deletions(-)
Comments
Hi,
On 20-09-18 15:12, Lev Stipakov wrote:
> From: Lev Stipakov <lev@openvpn.net>
>
> NCP negotiation can alter options. On reconnect
> client sends possibly altered options while server
> expects original values. This leads to warnings
> in log and, if server uses --opt-verify, breaks
> reconnect.
>
> Fix by decouple setting/unsetting NCP options from
> the state of TLS context. At startup (and once per sighup)
> we load original values to c->c1, which persists over
> sigusr1 (restart). When tearing tunnel down we restore
> (possibly altered) options back to original values.
>
> Trac: #1105
>
> Signed-off-by: Lev Stipakov <lev@openvpn.net>
> ---
> v2: refined commit message
>
> src/openvpn/init.c | 40 +++++++++++++++++++++++++++++++---------
> 1 file changed, 31 insertions(+), 9 deletions(-)
>
> diff --git a/src/openvpn/init.c b/src/openvpn/init.c
> index f432106..9353527 100644
> --- a/src/openvpn/init.c
> +++ b/src/openvpn/init.c
> @@ -612,6 +612,33 @@ uninit_proxy(struct context *c)
> uninit_proxy_dowork(c);
> }
>
> +/*
> + * Assign NCP-negotiable options to context->c1
> + * from context->options (initially config values).
> + * They persist over sigusr1 restart.
> + */
> +static void
> +do_set_ncp_options(struct context *c)
> +{
> + c->c1.ciphername = c->options.ciphername;
> + c->c1.authname = c->options.authname;
> + c->c1.keysize = c->options.keysize;
> +}
> +
> +/*
> + * Restore NCP-negotiable options from c->c1 to
> + * c->options. The latter ones can be altered by
> + * pushed options and therefore need to be restored
> + * to original values on sigusr1 restart.
> + */
> +static void
> +do_unset_ncp_options(struct context *c)
> +{
> + c->options.ciphername = c->c1.ciphername;
> + c->options.authname = c->c1.authname;
> + c->options.keysize = c->c1.keysize;
> +}
> +
> void
> context_init_1(struct context *c)
> {
> @@ -621,6 +648,8 @@ context_init_1(struct context *c)
>
> init_connection_list(c);
>
> + do_set_ncp_options(c);
> +
> #if defined(ENABLE_PKCS11)
> if (c->first_time)
> {
> @@ -2593,10 +2622,6 @@ do_init_crypto_tls_c1(struct context *c)
> /* initialize tls-auth/crypt key */
> do_init_tls_wrap_key(c);
>
> - c->c1.ciphername = options->ciphername;
> - c->c1.authname = options->authname;
> - c->c1.keysize = options->keysize;
> -
> #if 0 /* was: #if ENABLE_INLINE_FILES -- Note that enabling this code will break restarts */
> if (options->priv_key_file_inline)
> {
> @@ -2609,11 +2634,6 @@ do_init_crypto_tls_c1(struct context *c)
> {
> msg(D_INIT_MEDIUM, "Re-using SSL/TLS context");
>
> - /* Restore pre-NCP cipher options */
> - c->options.ciphername = c->c1.ciphername;
> - c->options.authname = c->c1.authname;
> - c->options.keysize = c->c1.keysize;
> -
> /*
> * tls-auth/crypt key can be configured per connection block, therefore
> * we must reload it as it may have changed
> @@ -4293,6 +4313,8 @@ close_instance(struct context *c)
> /* free key schedules */
> do_close_free_key_schedule(c, (c->mode == CM_P2P || c->mode == CM_TOP));
>
> + do_unset_ncp_options(c);
> +
> /* close TCP/UDP connection */
> do_close_link_socket(c);
>
>
Finally got to testing this patch, and my testing confirms that this is
the right fix. So:
Acked-by: Steffan Karger <steffan@karger.me>
-Steffan
Your patch has been applied to the master branch and release/2.4 branch.
I would appreciate a followup-patch, though, that explains a bit better
what "do_set_ncp_options()" *does* - basically "save the initial set
of config options into a storage space from where it can be recovered
later on, to get back the original options before NCP overwrote them"
- I understand that now (after asking Steffan) but the next one to
stumble across it will ask the same question...
commit 5fa25eeb7fefdbb17ad639d72fe46f393989159f (master)
commit d2ff5164e68e5101b1da2d2d818e23eb7851dc9f (release/2.4)
Author: Lev Stipakov
Date: Thu Sep 20 16:12:34 2018 +0300
Refactor NCP-negotiable options handling
Signed-off-by: Lev Stipakov <lev@openvpn.net>
Acked-by: Steffan Karger <steffan.karger@fox-it.com>
Message-Id: <1537449154-26879-1-git-send-email-lstipakov@gmail.com>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg17477.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
--
kind regards,
Gert Doering
@@ -612,6 +612,33 @@ uninit_proxy(struct context *c)
uninit_proxy_dowork(c);
}
+/*
+ * Assign NCP-negotiable options to context->c1
+ * from context->options (initially config values).
+ * They persist over sigusr1 restart.
+ */
+static void
+do_set_ncp_options(struct context *c)
+{
+ c->c1.ciphername = c->options.ciphername;
+ c->c1.authname = c->options.authname;
+ c->c1.keysize = c->options.keysize;
+}
+
+/*
+ * Restore NCP-negotiable options from c->c1 to
+ * c->options. The latter ones can be altered by
+ * pushed options and therefore need to be restored
+ * to original values on sigusr1 restart.
+ */
+static void
+do_unset_ncp_options(struct context *c)
+{
+ c->options.ciphername = c->c1.ciphername;
+ c->options.authname = c->c1.authname;
+ c->options.keysize = c->c1.keysize;
+}
+
void
context_init_1(struct context *c)
{
@@ -621,6 +648,8 @@ context_init_1(struct context *c)
init_connection_list(c);
+ do_set_ncp_options(c);
+
#if defined(ENABLE_PKCS11)
if (c->first_time)
{
@@ -2593,10 +2622,6 @@ do_init_crypto_tls_c1(struct context *c)
/* initialize tls-auth/crypt key */
do_init_tls_wrap_key(c);
- c->c1.ciphername = options->ciphername;
- c->c1.authname = options->authname;
- c->c1.keysize = options->keysize;
-
#if 0 /* was: #if ENABLE_INLINE_FILES -- Note that enabling this code will break restarts */
if (options->priv_key_file_inline)
{
@@ -2609,11 +2634,6 @@ do_init_crypto_tls_c1(struct context *c)
{
msg(D_INIT_MEDIUM, "Re-using SSL/TLS context");
- /* Restore pre-NCP cipher options */
- c->options.ciphername = c->c1.ciphername;
- c->options.authname = c->c1.authname;
- c->options.keysize = c->c1.keysize;
-
/*
* tls-auth/crypt key can be configured per connection block, therefore
* we must reload it as it may have changed
@@ -4293,6 +4313,8 @@ close_instance(struct context *c)
/* free key schedules */
do_close_free_key_schedule(c, (c->mode == CM_P2P || c->mode == CM_TOP));
+ do_unset_ncp_options(c);
+
/* close TCP/UDP connection */
do_close_link_socket(c);