[Openvpn-devel] Don't "undo" ifconfig when given --ifconfig-noexec

Message ID 20211117175424.17195-1-maximilian.fillinger@foxcrypto.com
State New
Headers show
Series
  • [Openvpn-devel] Don't "undo" ifconfig when given --ifconfig-noexec
Related show

Commit Message

Maximilian Fillinger Nov. 17, 2021, 5:54 p.m.
When running with --ifconfig-noexec on Linux, OpenVPN may still delete
the ip address from the tun interface on exit, because it tries to undo
the ifconfig that did not actually happen.

This commit reintroduces the did_ifconfig member to struct tuntap so
that we can check if ifconfig was actually done before trying to undo
it. It's behind an #ifdef because it's only used on Linux, and that was
the reason why it was removed before.

Signed-off-by: Max Fillinger <maximilian.fillinger@foxcrypto.com>
---
 src/openvpn/tun.c | 6 +++++-
 src/openvpn/tun.h | 3 +++
 2 files changed, 8 insertions(+), 1 deletion(-)

Comments

Gert Doering Nov. 17, 2021, 6:48 p.m. | #1
Hi,

On Wed, Nov 17, 2021 at 06:54:24PM +0100, Max Fillinger wrote:
> When running with --ifconfig-noexec on Linux, OpenVPN may still delete
> the ip address from the tun interface on exit, because it tries to undo
> the ifconfig that did not actually happen.
> 
> This commit reintroduces the did_ifconfig member to struct tuntap so
> that we can check if ifconfig was actually done before trying to undo
> it. It's behind an #ifdef because it's only used on Linux, and that was
> the reason why it was removed before.
> 
> Signed-off-by: Max Fillinger <maximilian.fillinger@foxcrypto.com>

I'm sure that this will work, but I wonder why we need to do this
in such a complicated way - the code here is

close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx)
...
        if (tt->did_ifconfig_setup)
        {
            undo_ifconfig_ipv4(tt, ctx);
        }
 
        if (tt->did_ifconfig_ipv6_setup)
        {
            undo_ifconfig_ipv6(tt, ctx);
        }

... so why is "did_ifconfig_setup" true, if ifconfig wasn't done?

Or, phrased differently, what is did_ifconfig_setup used for, across
the code, and can we just "not set it to true" if ifconfig-noexec
is in effect?  Or does it have nasty side effects?

gert
Maximilian Fillinger Nov. 17, 2021, 7 p.m. | #2
> ... so why is "did_ifconfig_setup" true, if ifconfig wasn't done?
> 
> Or, phrased differently, what is did_ifconfig_setup used for, across the
> code, and can we just "not set it to true" if ifconfig-noexec is in
> effect?  Or does it have nasty side effects?

tt->did_ifconfig_setup is set in init_tun, which sets up the tuntap struct but
doesn't do ifconfig yet. This is executed independently of --ifconfig-noexec.
There are probably some things to refactor there. Maybe we could remove
did_ifconfig_setup and instead use did_ifconfig on all platforms.

But to be honest, I'm scared of touching it.

Also, could we backport it to 2.5? If so, isn't it better to keep the fix small?

Patch

diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
index 75d5eaf7..32e739fc 100644
--- a/src/openvpn/tun.c
+++ b/src/openvpn/tun.c
@@ -1601,6 +1601,10 @@  do_ifconfig(struct tuntap *tt, const char *ifname, int tun_mtu,
         do_ifconfig_ipv6(tt, ifname, tun_mtu, es, ctx);
     }
 
+#ifdef TARGET_LINUX
+    tt->did_ifconfig = true;
+#endif
+
     /* release resources potentially allocated during interface setup */
     net_ctx_free(ctx);
 }
@@ -2190,7 +2194,7 @@  close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx)
 {
     ASSERT(tt);
 
-    if (tt->type != DEV_TYPE_NULL)
+    if (tt->type != DEV_TYPE_NULL && tt->did_ifconfig)
     {
         if (tt->did_ifconfig_setup)
         {
diff --git a/src/openvpn/tun.h b/src/openvpn/tun.h
index aa1e47b5..1f579e34 100644
--- a/src/openvpn/tun.h
+++ b/src/openvpn/tun.h
@@ -162,6 +162,9 @@  struct tuntap
 
     bool did_ifconfig_setup;
     bool did_ifconfig_ipv6_setup;
+#ifdef TARGET_LINUX
+    bool did_ifconfig;
+#endif
 
     bool persistent_if;         /* if existed before, keep on program end */