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

Message ID 20220629170555.116087-1-maximilian.fillinger@foxcrypto.com
State Changes Requested
Headers show
Series
  • [Openvpn-devel,v5] Don't "undo" ifconfig on exit if it wasn't done
Related show

Commit Message

Max Fillinger June 29, 2022, 5:05 p.m.
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().

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

Comments

Antonio Quartulli July 11, 2022, 3:38 p.m. | #1
Hi,

On 29/06/2022 19:05, Max Fillinger wrote:
> 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().
> 
> Signed-off-by: Max Fillinger <maximilian.fillinger@foxcrypto.com>
> ---
>   src/openvpn/init.c |   4 ++
>   src/openvpn/tun.c  | 161 +++++++++++++++++++++++----------------------
>   src/openvpn/tun.h  |   8 +++
>   3 files changed, 96 insertions(+), 77 deletions(-)
> 
> diff --git a/src/openvpn/init.c b/src/openvpn/init.c
> index c9d05c31..f5ca2be8 100644
> --- a/src/openvpn/init.c
> +++ b/src/openvpn/init.c
> @@ -1873,6 +1873,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 5e7b8c49..feee83ef 100644
> --- a/src/openvpn/tun.c
> +++ b/src/openvpn/tun.c
> @@ -1605,6 +1605,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 /* ifdef TARGET_LINUX */

The comment should contain the same guard as the first if, i.e. "if 
defined(TARGET_LINUX)"

> +       /* Empty for _WIN32. */

Indentation of comment is wrong

> +}
> +
> +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, "Linux ip -6 addr del failed");

Error message is wrong, because it should say "Generic" as above.
> +
> +    argv_free(&argv);
> +    gc_free(&gc);
> +#endif /* ifdef TARGET_LINUX */

as above

> +       /* Empty for _WIN32. */

as above

> +}
> +
> +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);
> +    }
> +

spurious empty line

> +}
> +
>   static void
>   clear_tuntap(struct tuntap *tuntap)
>   {
> @@ -1846,7 +1929,7 @@ open_tun_generic(const char *dev, const char *dev_type, const char *dev_node,
>               msg(M_INFO, "DCO device %s opened", tunname);
>           }
>           else
> -#endif
> +#endif /* if defined(TARGET_LINUX) */
>           {
>               if (!dynamic_opened)
>               {
> @@ -2176,87 +2259,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 (!tt->options.disable_dco)
>       {
> diff --git a/src/openvpn/tun.h b/src/openvpn/tun.h
> index cf02bf43..6d4530f7 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);

Other than the nitpicks above, it looks good to me.

On top of that, most of the issues I highlighted already exist in the 
current code (it is just being moved). But this could be the chance to 
fix those on the fly.

Acked-by: Antonio Quartulli <a@unstable.cc>

Note: this change still requires more testing with non-linux/non-windows 
platforms.

Cheers,
Gert Doering Aug. 6, 2022, 2:20 p.m. | #2
HI,

On Wed, Jun 29, 2022 at 07:05:55PM +0200, Max Fillinger wrote:
> 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().

I did what I promised too many weeks ago, and pushed this change
towards the buildbot army...  alas, they did what I was afraid they
would do: complained.

+-DPLUGIN_LIBDIR=\"/usr/local/lib/openvpn/plugins\"  -Wall -g -O2 -std=c99 -MT
+tun.o -MD -MP -MF .deps/tun.Tpo -c -o tun.o tun.c
tun.c:1656:73: error: passing 'struct gc_arena' to parameter of incompatible
+type 'struct gc_arena *'; take the address with &
    const char *ifconfig_ipv6_local = print_in6_addr(tt->local_ipv6, 0, gc);
                                                                        ^~
                                                                        &
./socket.h:391:88: note: passing argument to parameter 'gc' here
const char *print_in6_addr(struct in6_addr addr6, unsigned int flags, struct
+gc_arena *gc);


so the "non-linux" bits of undo_ifconfig_ipv6() have been broken since
ever - but the patch now actually shows that to the compiler...

> +#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();

... and inded, this needs to be "&gc"...


Can you please send a v6 that changes this, and also rebases against
master?  The context has changed a bit, and one hunk (#endif /* comment */)
is no longer relevant, as that #endif never came into existance :-)

We should then be able to proceed more quickly...

thanks,

gert

Patch

diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index c9d05c31..f5ca2be8 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -1873,6 +1873,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 5e7b8c49..feee83ef 100644
--- a/src/openvpn/tun.c
+++ b/src/openvpn/tun.c
@@ -1605,6 +1605,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 /* ifdef 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, "Linux ip -6 addr del failed");
+
+    argv_free(&argv);
+    gc_free(&gc);
+#endif /* ifdef 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)
 {
@@ -1846,7 +1929,7 @@  open_tun_generic(const char *dev, const char *dev_type, const char *dev_node,
             msg(M_INFO, "DCO device %s opened", tunname);
         }
         else
-#endif
+#endif /* if defined(TARGET_LINUX) */
         {
             if (!dynamic_opened)
             {
@@ -2176,87 +2259,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 (!tt->options.disable_dco)
     {
diff --git a/src/openvpn/tun.h b/src/openvpn/tun.h
index cf02bf43..6d4530f7 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);