diff mbox series

[Openvpn-devel,v1] Fix logic when pushed cipher triggers tun reopen and ignore more options

Message ID 20251029065316.10182-1-gert@greenie.muc.de
State New
Headers show
Series [Openvpn-devel,v1] Fix logic when pushed cipher triggers tun reopen and ignore more options | expand

Commit Message

Gert Doering Oct. 29, 2025, 6:53 a.m. UTC 
From: Arne Schwabe <arne@rfc2549.org>

The logic was inverted. Only when link-mtu is used, pushing a cipher can
change the MTU and not the other way round. (found by zeropath)

Also ignore a few more options that should not trigger a reopen of tun
in push message.

Reported-By: contact@joshua.hu
Found-By: Zeropath
Change-Id: I76eb584024610a6054a069340adbac988abf686c
Signed-off-by: Arne Schwabe <arne-openvpn@rfc2549.org>
Acked-by: Gert Doering <gert@greenie.muc.de>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1321
---

This change was reviewed on Gerrit and approved by at least one
developer. I request to merge it to master.

Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1321
This mail reflects revision 1 of this Change.

Signed-off-by line for the author was added as per our policy.

Acked-by according to Gerrit (reflected above):
Gert Doering <gert@greenie.muc.de>

Comments

Gert Doering Oct. 29, 2025, 7:22 a.m. UTC | #1 
So there was a bit of discussion about this in gerrit, and the logic is
confusing and twisted.  The tun device needs to reopened on a cipher
change *if* the tun-mtu ("ifconfig tun0 mtu 1400") could change - and
if the config has tun_mtu_defined, the tun MTU is fixed, so the cipher
could be ignored.  I think there are still funny edge cases (like
"no tun-mtu or link-mtu defined", what then?) and the check maybe should
be double-checked for "if (!opt->ce.link_mtu_defined)" - because
"link-mtu <set>" is the trigger for "tun mtu <calculated>", so in that
case we can't skip this.

OTOH, this is a small edge of an edge case, namely "cipher *change* upon
reconnect", which is rare enough - so for this to make a real difference
you need --user nobody or windows-dco (so "reopen tun" would fail) *and*
a cipher change.

I have changed the Reported-by:/Found-by: tags to look "like on the last
few commits", so it's consistent (basically adding URL and full name).

Your patch has been applied to the master branch.

commit 911a69dc1af20bc54557a208b6fd4e76261b2bb2
Author: Arne Schwabe
Date:   Wed Oct 29 07:53:10 2025 +0100

     Fix logic when pushed cipher triggers tun reopen and ignore more options

     Signed-off-by: Arne Schwabe <arne-openvpn@rfc2549.org>
     Acked-by: Gert Doering <gert@greenie.muc.de>
     Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1321
     Message-Id: <20251029065316.10182-1-gert@greenie.muc.de>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg33989.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/push.c b/src/openvpn/push.c
index 2c717c7..d7063e6 100644
--- a/src/openvpn/push.c
+++ b/src/openvpn/push.c
@@ -1025,15 +1025,25 @@ 
     char line[OPTION_PARM_SIZE];
     while (buf_parse(buf, ',', line, sizeof(line)))
     {
-        /* peer-id and auth-token might change on restart and this should not trigger reopening tun
+        /* peer-id and auth-token might change on restart and this should not
+         * trigger reopening tun
+         * Also other options that only affect the control channel should
+         * not trigger a reopen of the tun device
          */
-        if (strprefix(line, "peer-id ") || strprefix(line, "auth-token ")
-            || strprefix(line, "auth-token-user "))
+        if (strprefix(line, "peer-id ")
+            || strprefix(line, "auth-token ")
+            || strprefix(line, "auth-token-user")
+            || strprefix(line, "protocol-flags ")
+            || strprefix(line, "key-derivation ")
+            || strprefix(line, "explicit-exit-notify ")
+            || strprefix(line, "ping ")
+            || strprefix(line, "ping-restart ")
+            || strprefix(line, "ping-timer "))
         {
             continue;
         }
         /* tun reopen only needed if cipher change can change tun MTU */
-        if (strprefix(line, "cipher ") && !opt->ce.tun_mtu_defined)
+        if (strprefix(line, "cipher ") && opt->ce.tun_mtu_defined)
         {
             continue;
         }