diff mbox series

[Openvpn-devel,v6] Don't "undo" ifconfig on exit if it wasn't done

Message ID 20220810153006.18860-1-maximilian.fillinger@foxcrypto.com
State Accepted
Headers show
Series [Openvpn-devel,v6] Don't "undo" ifconfig on exit if it wasn't done | expand

Commit Message

Maximilian Fillinger Aug. 10, 2022, 5:30 a.m. UTC 
When running with --ifconfig-noexec, OpenVPN does not execute ifconfig,
but on exit, it still tries to "undo" the configuration it would have
done. This patch fixes it by extracting an undo_ifconfig() function from
close_tun(). The undo function is called before close_tun(), but only if
--ifconfig-noexec isn't set. This is symmetric to how open_tun() and
do_ifconfig() are used.

v2: Fix tabs-vs-spaces.
v3: Fix another style mistake.
v4: Move undo_ifconfig{4,6}() out of #ifdef TARGET_LINUX.
v5: Keep ctx argument in close_tun().
v6: Fix bug in non-Linux non-Windows version of undo_ifconfig_ipv6

Signed-off-by: Max Fillinger <maximilian.fillinger@foxcrypto.com>
---
 src/openvpn/init.c |   4 ++
 src/openvpn/tun.c  | 159 +++++++++++++++++++++++----------------------
 src/openvpn/tun.h  |   8 +++
 3 files changed, 95 insertions(+), 76 deletions(-)

Comments

Gert Doering Aug. 19, 2022, 7:04 a.m. UTC | #1 
Acked-by: Gert Doering <gert@greenie.muc.de>

I had to apply a bit of pressure to get the patch to apply - the 
context in tun.c got changed ( || TARGET_FREEBSD) in the meantime...
fairly trivial, and no actual code change.

Pushed to the new and renovated buildbot army, to see if it would break
any platforms (very convenient, this ;-) ).  Didn't!  Thanks for the v6
update!

The change is as we have discussed, and I still think it's a good and
clean way forward.  The "undo_ifconfig_ipv4() / _ipv6()" functions
are as they have been before (except that they now get compiled for
all platforms except WIN32 - the "linux only" part was a 2.5.0 oversight),
and the calls moved 1:1 from (TARGET_LINUX) close_tun() to a new
undo_ifconfig() function.  tun.c is still a mess, but this helps a bit.

While staring at the diff, I kicked a spurious blank line from the
end of undo_ifconfig()... :-)


I'm a bit unsure if we want to pull this up to release/2.5 - it won't
apply "as is" (due to all the DCO bits around it), but the actual change
could be done the same.  It's a bugfix, and brings back the 2.4 
"undo ifconfig" behaviour to all non-linux/non-Win32 platforms 
(which was an unintended change in 2.5.0).  So maybe we should?


Your patch has been applied to the master branch.

commit 0c4d40cb838eb0fad4a82bbed1198f6d359d445a
Author: Max Fillinger
Date:   Wed Aug 10 17:30:06 2022 +0200

     Don't undo ifconfig on exit if it wasn't done

     Signed-off-by: Max Fillinger <maximilian.fillinger@foxcrypto.com>
     Acked-by: Gert Doering <gert@greenie.muc.de>
     Message-Id: <20220810153006.18860-1-maximilian.fillinger@foxcrypto.com>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg24860.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
Gert Doering Aug. 20, 2022, 11:02 a.m. UTC | #2 
Hi,

On Fri, Aug 19, 2022 at 07:04:52PM +0200, Gert Doering wrote:
> The change is as we have discussed, and I still think it's a good and
> clean way forward.  The "undo_ifconfig_ipv4() / _ipv6()" functions
> are as they have been before (except that they now get compiled for
> all platforms except WIN32 - the "linux only" part was a 2.5.0 oversight),
> and the calls moved 1:1 from (TARGET_LINUX) close_tun() to a new
> undo_ifconfig() function.  tun.c is still a mess, but this helps a bit.

I did lots of testing on FreeBSD today, and it turns out that the
generic "undo" code is just a mess and clutters the log with garbage...

2022-08-20 22:58:02 /sbin/ifconfig tun4 0.0.0.0
ifconfig: ioctl (SIOCAIFADDR): Destination address required
2022-08-20 22:58:02 Generic ip addr del failed: external program exited with error status: 1
2022-08-20 22:58:02 /sbin/ifconfig tun4 del fd00:abcd:194:2::103d/64
ifconfig: del: bad value
2022-08-20 22:58:02 Generic ip -6 addr del failed: external program exited with error status: 1

... not Max' fault, but I wonder if this ever did something useful...

(I'm fairly sure "set ipv4 address to 0.0.0.0" was a valid way to undo
IPv4 ifconfig, but I do not think "del" was ever a valid ifconfig IPv6
syntax on *BSD)

So - this needs fixing, and platform-by-platform testing...

Technically, for most cases this won't be actually needed - if the
tun/tap interface is destroyed at program end ("ifconfig tun4 destroy"),
the IPv4/IPv6 config is gone anyway.   Only for persistant interfaces
it makes a difference.

gert
Gert Doering Aug. 20, 2022, 11:07 a.m. UTC | #3 
HI,

On Sat, Aug 20, 2022 at 11:02:46PM +0200, Gert Doering wrote:
> I did lots of testing on FreeBSD today, and it turns out that the
> generic "undo" code is just a mess and clutters the log with garbage...
> 
> 2022-08-20 22:58:02 /sbin/ifconfig tun4 0.0.0.0
> ifconfig: ioctl (SIOCAIFADDR): Destination address required

... interesting enough, on *tap* interfaces, OpenVPN can do that...

2022-08-20 23:05:34 /sbin/ifconfig tap0 0.0.0.0
ifconfig: WARNING: setting interface address without mask is deprecated,
default mask may not be correct.

... but the kernel logs a warning...

Aug 20 23:05:34 fbsd14 kernel: tap0: set address: WARNING: network mask should be specified; using historical default

*shake head*

gert
diff mbox series

Patch

diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index b6705921..ee800880 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -1913,6 +1913,10 @@  do_close_tun_simple(struct context *c)
     msg(D_CLOSE, "Closing TUN/TAP interface");
     if (c->c1.tuntap)
     {
+        if (!c->options.ifconfig_noexec)
+        {
+            undo_ifconfig(c->c1.tuntap, &c->net_ctx);
+        }
         close_tun(c->c1.tuntap, &c->net_ctx);
         c->c1.tuntap = NULL;
     }
diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
index f3152a52..2df36c74 100644
--- a/src/openvpn/tun.c
+++ b/src/openvpn/tun.c
@@ -1604,6 +1604,89 @@  do_ifconfig(struct tuntap *tt, const char *ifname, int tun_mtu,
     net_ctx_free(ctx);
 }
 
+static void
+undo_ifconfig_ipv4(struct tuntap *tt, openvpn_net_ctx_t *ctx)
+{
+#if defined(TARGET_LINUX)
+    int netbits = netmask_to_netbits2(tt->remote_netmask);
+
+    if (is_tun_p2p(tt))
+    {
+        if (net_addr_ptp_v4_del(ctx, tt->actual_name, &tt->local,
+                                &tt->remote_netmask) < 0)
+        {
+            msg(M_WARN, "Linux can't del IP from iface %s",
+                tt->actual_name);
+        }
+    }
+    else
+    {
+        if (net_addr_v4_del(ctx, tt->actual_name, &tt->local, netbits) < 0)
+        {
+            msg(M_WARN, "Linux can't del IP from iface %s",
+                tt->actual_name);
+        }
+    }
+#elif !defined(_WIN32)  /* if !defined(TARGET_LINUX) && !defined(_WIN32) */
+    struct argv argv = argv_new();
+
+    argv_printf(&argv, "%s %s 0.0.0.0", IFCONFIG_PATH, tt->actual_name);
+
+    argv_msg(M_INFO, &argv);
+    openvpn_execve_check(&argv, NULL, 0, "Generic ip addr del failed");
+
+    argv_free(&argv);
+#endif /* if defined(TARGET_LINUX) */
+       /* Empty for _WIN32. */
+}
+
+static void
+undo_ifconfig_ipv6(struct tuntap *tt, openvpn_net_ctx_t *ctx)
+{
+#if defined(TARGET_LINUX)
+    if (net_addr_v6_del(ctx, tt->actual_name, &tt->local_ipv6,
+                        tt->netbits_ipv6) < 0)
+    {
+        msg(M_WARN, "Linux can't del IPv6 from iface %s", tt->actual_name);
+    }
+#elif !defined(_WIN32)  /* if !defined(TARGET_LINUX) && !defined(_WIN32) */
+    struct gc_arena gc = gc_new();
+    const char *ifconfig_ipv6_local = print_in6_addr(tt->local_ipv6, 0, &gc);
+    struct argv argv = argv_new();
+
+    argv_printf(&argv, "%s %s del %s/%d", IFCONFIG_PATH, tt->actual_name,
+                ifconfig_ipv6_local, tt->netbits_ipv6);
+
+    argv_msg(M_INFO, &argv);
+    openvpn_execve_check(&argv, NULL, 0, "Generic ip -6 addr del failed");
+
+    argv_free(&argv);
+    gc_free(&gc);
+#endif /* if defined(TARGET_LINUX) */
+       /* Empty for _WIN32. */
+}
+
+void
+undo_ifconfig(struct tuntap *tt, openvpn_net_ctx_t *ctx)
+{
+    if (tt->type != DEV_TYPE_NULL)
+    {
+        if (tt->did_ifconfig_setup)
+        {
+            undo_ifconfig_ipv4(tt, ctx);
+        }
+
+        if (tt->did_ifconfig_ipv6_setup)
+        {
+            undo_ifconfig_ipv6(tt, ctx);
+        }
+
+        /* release resources potentially allocated during undo */
+        net_ctx_reset(ctx);
+    }
+
+}
+
 static void
 clear_tuntap(struct tuntap *tuntap)
 {
@@ -2213,87 +2296,11 @@  tuncfg(const char *dev, const char *dev_type, const char *dev_node,
 
 #endif /* ENABLE_FEATURE_TUN_PERSIST */
 
-static void
-undo_ifconfig_ipv4(struct tuntap *tt, openvpn_net_ctx_t *ctx)
-{
-#if defined(TARGET_LINUX)
-    int netbits = netmask_to_netbits2(tt->remote_netmask);
-
-    if (is_tun_p2p(tt))
-    {
-        if (net_addr_ptp_v4_del(ctx, tt->actual_name, &tt->local,
-                                &tt->remote_netmask) < 0)
-        {
-            msg(M_WARN, "Linux can't del IP from iface %s",
-                tt->actual_name);
-        }
-    }
-    else
-    {
-        if (net_addr_v4_del(ctx, tt->actual_name, &tt->local, netbits) < 0)
-        {
-            msg(M_WARN, "Linux can't del IP from iface %s",
-                tt->actual_name);
-        }
-    }
-#else  /* ifndef TARGET_LINUX */
-    struct argv argv = argv_new();
-
-    argv_printf(&argv, "%s %s 0.0.0.0", IFCONFIG_PATH, tt->actual_name);
-
-    argv_msg(M_INFO, &argv);
-    openvpn_execve_check(&argv, NULL, 0, "Generic ip addr del failed");
-
-    argv_free(&argv);
-#endif /* ifdef TARGET_LINUX */
-}
-
-static void
-undo_ifconfig_ipv6(struct tuntap *tt, openvpn_net_ctx_t *ctx)
-{
-#if defined(TARGET_LINUX)
-    if (net_addr_v6_del(ctx, tt->actual_name, &tt->local_ipv6,
-                        tt->netbits_ipv6) < 0)
-    {
-        msg(M_WARN, "Linux can't del IPv6 from iface %s", tt->actual_name);
-    }
-#else  /* ifndef TARGET_LINUX */
-    struct gc_arena gc = gc_new();
-    const char *ifconfig_ipv6_local = print_in6_addr(tt->local_ipv6, 0, gc);
-    struct argv argv = argv_new();
-
-    argv_printf(&argv, "%s %s del %s/%d", IFCONFIG_PATH, tt->actual_name,
-                ifconfig_ipv6_local, tt->netbits_ipv6);
-
-    argv_msg(M_INFO, &argv);
-    openvpn_execve_check(&argv, NULL, 0, "Linux ip -6 addr del failed");
-
-    argv_free(&argv);
-    gc_free(&gc);
-#endif /* ifdef TARGET_LINUX */
-}
-
 void
 close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx)
 {
     ASSERT(tt);
 
-    if (tt->type != DEV_TYPE_NULL)
-    {
-        if (tt->did_ifconfig_setup)
-        {
-            undo_ifconfig_ipv4(tt, ctx);
-        }
-
-        if (tt->did_ifconfig_ipv6_setup)
-        {
-            undo_ifconfig_ipv6(tt, ctx);
-        }
-
-        /* release resources potentially allocated during undo */
-        net_ctx_reset(ctx);
-    }
-
 #ifdef TARGET_LINUX
     if (tun_dco_enabled(tt))
     {
diff --git a/src/openvpn/tun.h b/src/openvpn/tun.h
index 8ec8f51f..3fdfcce6 100644
--- a/src/openvpn/tun.h
+++ b/src/openvpn/tun.h
@@ -300,6 +300,14 @@  void do_ifconfig_setenv(const struct tuntap *tt,
 void do_ifconfig(struct tuntap *tt, const char *ifname, int tun_mtu,
                  const struct env_set *es, openvpn_net_ctx_t *ctx);
 
+/**
+ * undo_ifconfig - undo configuration of the tunnel interface
+ *
+ * @param tt    the tuntap interface context
+ * @param ctx   the networking API opaque context
+ */
+void undo_ifconfig(struct tuntap *tt, openvpn_net_ctx_t *ctx);
+
 bool is_dev_type(const char *dev, const char *dev_type, const char *match_type);
 
 int dev_type_enum(const char *dev, const char *dev_type);